Implement OpenID Connect Authentication Strategy #2149
Conversation
…, Authentik etc.)
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request introduces comprehensive support for OpenID Connect (OIDC) authentication across the application. The implementation adds new environment variables, updates configuration schemas, extends the authentication strategy, and modifies client-side and server-side components to enable generic OIDC login. The changes allow self-hosted instances to integrate with identity providers like Keycloak or Authelia by configuring OpenID-specific parameters. Changes
Sequence DiagramsequenceDiagram
participant User
participant Client
participant Server
participant OpenIDProvider
User->>Client: Click OpenID Login
Client->>Server: Request OpenID Authentication
Server->>OpenIDProvider: Redirect to Authorization
OpenIDProvider->>User: Authentication Prompt
User->>OpenIDProvider: Authenticate
OpenIDProvider->>Server: Return Authentication Code
Server->>OpenIDProvider: Request Token
OpenIDProvider->>Server: Provide Token
Server->>Server: Validate User
Server->>Client: Return Authentication Token
Client->>User: Log In Successful
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
View your CI Pipeline Execution ↗ for commit 227870a.
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/server/src/main.ts (1)
25-33:⚠️ Potential issueCritical security improvements needed for session configuration.
Several security concerns need to be addressed:
- Using
ACCESS_TOKEN_SECRETfor session secret is not recommended as it serves a different purpose- Memory session store (default) is not suitable for production
- Missing essential security options:
- Session expiry
- Cookie same-site policy
- Cookie max-age
Apply these security improvements:
app.use( session({ resave: false, saveUninitialized: false, - secret: configService.getOrThrow("ACCESS_TOKEN_SECRET"), + secret: configService.getOrThrow("SESSION_SECRET"), // Use a dedicated secret + store: new RedisStore({ // Or another production-ready session store + url: configService.get("REDIS_URL"), + }), cookie: { httpOnly: true, secure: process.env.NODE_ENV === "production", + sameSite: "lax", + maxAge: 24 * 60 * 60 * 1000, // 24 hours }, + name: "sid", // Don't reveal technology stack }), );Don't forget to:
- Add
SESSION_SECRETto your environment variables- Set up a production-ready session store (Redis recommended)
- Update the configuration schema to include new variables
🧹 Nitpick comments (4)
apps/server/src/auth/strategy/openid.strategy.ts (3)
57-71: Handle user creation errors more specificallyIn the nested
try-catchblocks, catching all errors and assuming the user already exists may mask other issues during user creation. It's better to catch specific errors related to user existence (e.g., unique constraint violations) and handle other exceptions appropriately.Consider revising the error handling as follows:
} catch (error) { - throw new BadRequestException(ErrorMessage.UserAlreadyExists); + if (/* check if error indicates user already exists */) { + throw new BadRequestException(ErrorMessage.UserAlreadyExists); + } else { + throw error; + } }Replace the pseudo-code comment with actual error checking logic based on your application's error handling mechanisms.
60-60: Consider retrieving locale from the OpenID profileCurrently, the
localeis hardcoded as"en-US". If the OpenID provider supplies locale information, consider using it to set the user's preferred locale.You can modify the code to extract the locale:
locale: "en-US", + // Assuming the profile contains locale information + // locale: profile._json.locale || "en-US",Ensure to verify the actual path to the locale information in the
profileobject.
33-73: Simplify and improve error handling in thevalidatemethodThe
validatemethod can be refactored to enhance readability and maintainability. Nestedtry-catchblocks can be flattened, and the logic for user lookup and creation can be made clearer.Consider refactoring the method:
- Use
awaitwith propertry-catchblocks for each asynchronous operation.- Separate user lookup and creation into distinct functions if appropriate.
- Handle specific exceptions rather than using broad catches.
This refactor will make the code easier to understand and maintain.
apps/client/src/pages/auth/_components/social-auth.tsx (1)
36-47: Consider adding error handling and loading states.While the OpenID button implementation follows the existing pattern, consider enhancing user experience with:
- Loading state during authentication
- Error handling for failed authentication attempts
- Feedback for users when OpenID server is unreachable
Example implementation:
{providers.includes("openid") && ( <Button asChild size="lg" + disabled={isLoading} className="w-full !bg-[#dc2626] !text-white hover:!bg-[#dc2626]/80" > <a href="/api/auth/openid"> - <Fingerprint className="mr-3 size-4" /> + {isLoading ? ( + <Spinner className="mr-3 size-4" /> + ) : ( + <Fingerprint className="mr-3 size-4" /> + )} {t`OpenID`} </a> </Button> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (52)
apps/client/src/locales/af-ZA/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/am-ET/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ar-SA/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/bg-BG/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/bn-BD/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ca-ES/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/cs-CZ/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/da-DK/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/de-DE/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/el-GR/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/en-US/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/es-ES/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/fa-IR/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/fi-FI/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/fr-FR/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/he-IL/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/hi-IN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/hu-HU/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/id-ID/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/it-IT/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ja-JP/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/km-KH/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/kn-IN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ko-KR/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/lt-LT/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/lv-LV/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ml-IN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/mr-IN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ms-MY/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ne-NP/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/nl-NL/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/no-NO/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/or-IN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/pl-PL/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/pt-BR/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/pt-PT/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ro-RO/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ru-RU/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/sk-SK/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/sq-AL/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/sr-SP/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/sv-SE/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/ta-IN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/te-IN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/th-TH/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/tr-TR/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/uk-UA/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/uz-UZ/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/vi-VN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/zh-CN/messages.pois excluded by!apps/client/src/locales/*/messages.poapps/client/src/locales/zh-TW/messages.pois excluded by!apps/client/src/locales/*/messages.popnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.env.example(1 hunks)apps/client/src/pages/auth/_components/social-auth.tsx(2 hunks)apps/server/src/auth/auth.controller.ts(2 hunks)apps/server/src/auth/auth.module.ts(2 hunks)apps/server/src/auth/auth.service.ts(1 hunks)apps/server/src/auth/guards/openid.guard.ts(1 hunks)apps/server/src/auth/strategy/openid.strategy.ts(1 hunks)apps/server/src/config/schema.ts(1 hunks)apps/server/src/main.ts(2 hunks)libs/dto/src/auth/providers.ts(1 hunks)libs/dto/src/user/user.ts(1 hunks)package.json(6 hunks)tools/compose/nginx-proxy-manager.yml(1 hunks)tools/compose/simple.yml(1 hunks)tools/compose/swarm.yml(1 hunks)tools/compose/traefik-secure.yml(1 hunks)tools/compose/traefik.yml(1 hunks)tools/prisma/migrations/20250113145008_add_openid_provider_to_enums/migration.sql(1 hunks)tools/prisma/schema.prisma(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tools/compose/nginx-proxy-manager.yml
- tools/compose/traefik.yml
🔇 Additional comments (15)
apps/server/src/auth/guards/openid.guard.ts (1)
1-5:OpenIDGuardis correctly implementedThe
OpenIDGuardclass correctly extendsAuthGuardwith the "openid" strategy, enabling OpenID authentication in the application.libs/dto/src/auth/providers.ts (1)
4-4: Added "openid" to authentication providersThe addition of "openid" to the
authProvidersSchemacorrectly includes OpenID as a valid authentication provider.libs/dto/src/user/user.ts (1)
29-29: LGTM! Provider enum updated correctly.The addition of "openid" to the provider enum is consistent with the OpenID Connect implementation while maintaining backward compatibility.
apps/server/src/auth/auth.module.ts (1)
67-95: LGTM! OpenID strategy configuration is well-implemented.The implementation follows the established pattern for OAuth providers and includes proper validation of all required OpenID configuration parameters with appropriate fallback to DummyStrategy.
apps/server/src/auth/auth.controller.ts (1)
151-166: LGTM! OpenID endpoints are properly implemented.The OpenID authentication endpoints follow the established OAuth flow pattern, with proper guard usage and consistent error handling.
tools/prisma/migrations/20250113145008_add_openid_provider_to_enums/migration.sql (2)
1-2: LGTM! Migration is correctly implemented.The migration properly adds the 'openid' value to the Provider enum using the correct PostgreSQL syntax.
Line range hint
73-79: Verify callback URL configuration security.Ensure that the OpenID callback URL configuration is properly validated and matches the URL registered with the OpenID provider to prevent callback URL manipulation attacks.
Run this script to check the callback URL configuration:
✅ Verification successful
Callback URL configuration is properly secured ✅
The implementation follows security best practices with environment-based configuration, URL validation through zod schema, and no dynamic manipulation of callback paths that could lead to callback URL manipulation attacks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check OpenID callback URL configuration across the codebase # Look for hardcoded callback URLs or dynamic URL construction rg -g '!{*.md,*.json}' -i 'callback.*url|redirect.*url'Length of output: 3955
tools/prisma/schema.prisma (1)
14-14: LGTM! The Provider enum extension is correct.The addition of 'openid' to the Provider enum is properly implemented and aligns with the existing authentication providers.
.env.example (2)
72-79: LGTM! OpenID configuration variables are well-structured.The OpenID configuration section follows the established pattern and includes all necessary OIDC parameters.
75-76: Ensure secure handling of OpenID credentials.The
OPENID_CLIENT_IDandOPENID_CLIENT_SECRETare sensitive credentials. Ensure these are properly secured in production environments and never committed to version control.✅ Verification successful
OpenID credential handling verified as secure
The codebase demonstrates secure handling of OpenID credentials:
- Credentials are properly externalized to environment variables
- Configuration is validated through schema checks
- No hardcoded credentials found in any files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any OpenID credentials are accidentally committed rg -g '!.env.example' "OPENID_(CLIENT_ID|CLIENT_SECRET)=[^$]"Length of output: 63
Script:
#!/bin/bash # Search for any environment or configuration files fd -t f "^\.env" -X echo "=== {} ===" -X cat {} # Search for any potential credential patterns with different cases rg -i "openid.*client.*(?:id|secret).*=" --type-not example # Look for how these credentials are used in the code ast-grep --pattern 'process.env.OPENID_$_'Length of output: 325
Script:
#!/bin/bash # List all .env files echo "=== Environment Files ===" fd "^\.env" # Search for OpenID credentials in all files except .env.example echo -e "\n=== Credential Patterns ===" rg -i "openid.*client" -g "!.env.example" # Look for environment variable usage echo -e "\n=== Credential Usage ===" rg "OPENID_CLIENT" -g "!.env.example"Length of output: 2788
tools/compose/simple.yml (1)
109-117: LGTM! OpenID configuration in simple deployment is correct.The OpenID environment variables are properly configured for the simple deployment scenario, with the correct callback URL port (3000) matching the container's exposed port.
tools/compose/traefik-secure.yml (1)
118-125: LGTM! OpenID configuration in secure deployment is correct.The OpenID environment variables are properly configured for secure deployment:
- Uses HTTPS protocol
- Consistent with the domain configuration
- Follows the same pattern as other OAuth providers
tools/compose/swarm.yml (1)
129-136: LGTM! OpenID configuration in swarm deployment is correct.The OpenID environment variables are properly configured for swarm deployment:
- Uses HTTPS protocol
- Consistent with the domain configuration
- Maintains consistency with other deployment configurations
package.json (2)
4-4: LGTM! OpenID Connect dependencies are properly configured.The version bump and addition of OpenID Connect related dependencies align well with the PR objectives. The necessary packages for implementing OpenID Connect authentication are included:
- express-session for session management
- passport-openidconnect for the authentication strategy
- Corresponding TypeScript type definitions
Also applies to: 65-65, 79-79, 196-196, 220-220
176-176: Verify the @tanstack/react-query update.The update of @tanstack/react-query from ^5.64.0 to ^5.64.1 appears unrelated to the OpenID Connect implementation. Please ensure this update was intentional and verify its compatibility.
Run this script to check the changelog for any breaking changes:
✅ Verification successful
The @tanstack/react-query update to 5.64.1 is safe.
This patch update only adds TypeScript type improvements for query/mutation keys with no runtime changes. While unrelated to OpenID Connect, keeping dependencies up to date is good practice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check @tanstack/react-query changelog for version 5.64.1 gh api repos/TanStack/query/releases | jq '.[] | select(.tag_name == "v5.64.1") | .body'Length of output: 1105
Here's a quick demo of it working:
Screen.Recording.2025-01-13.at.3.46.26.PM.mov
To get started with OpenID configuration, just make sure to run the latest version of the image (v4.3.7) and add the following environment variables (also documented in the
.env.examplefile):You should have received these values from OpenID Servers such as Keycloak or Authentik.
Fixes #703.
Summary by CodeRabbit
New Features
Configuration
Dependencies
Version