Add Support for OpenID Connect (OAuth2/OIDC) logins to NocoDB #11117
Conversation
📝 Walkthrough## Walkthrough
The changes introduce OpenID Connect (OIDC) authentication support to the application. New OIDC endpoints are added to the authentication controller, and an OIDC strategy is implemented using Passport's OAuth2 mechanism. The authentication middleware is updated to handle OIDC sign-in flows in addition to existing providers. The authentication module is updated to register the new OIDC strategy. Token validation logic is enhanced to check for expiration, and a comprehensive test suite for the OIDC strategy is added. No existing public interfaces are changed, but new methods and a provider are introduced for OIDC functionality.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------|
| packages/nc-gui/middleware/03.auth.global.ts | Expanded middleware to support OIDC sign-in alongside Google and GitHub; dynamic provider selection; improved query parameter checks. |
| packages/nocodb/src/models/UserRefreshToken.ts | Added expiration check in `getByToken` to return `null` for expired tokens. |
| packages/nocodb/src/modules/auth/auth.controller.ts | Added OIDC authentication endpoints: `/auth/oidc`, `/auth/oidc/callback`, and `/auth/oidc/genTokenByCode`. |
| packages/nocodb/src/modules/auth/auth.module.ts | Registered `OidcStrategyProvider` in authentication module providers. |
| packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts | Added test suite for `OidcStrategy` covering user info retrieval, user creation, and error handling. |
| packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts | Implemented `OidcStrategy` class for OIDC authentication; exported provider for dependency injection. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Frontend
participant AuthMiddleware
participant AuthController
participant OidcStrategy
participant UsersService
participant UserModel
User->>Frontend: Initiate sign-in (OIDC)
Frontend->>AuthMiddleware: Request with OIDC state/code
AuthMiddleware->>Frontend: Redirect to /auth/oidc if OIDC params present
Frontend->>AuthController: GET /auth/oidc
AuthController->>OidcStrategy: Trigger authenticate()
OidcStrategy->>User: Redirect to OIDC provider for login
User->>OidcStrategy: Callback with code
OidcStrategy->>AuthController: Callback with code
AuthController->>OidcStrategy: validate(code)
OidcStrategy->>OIDC Provider: Fetch userinfo
OIDC Provider-->>OidcStrategy: Return user profile
OidcStrategy->>UserModel: Find user by email
alt User exists
UserModel-->>OidcStrategy: Return user
else User does not exist
OidcStrategy->>UsersService: Register new user
UsersService-->>OidcStrategy: Return new user
end
OidcStrategy-->>AuthController: Return user
AuthController->>Frontend: Set refresh token, respond with login successAssessment against linked issues
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/nocodb/src/modules/auth/auth.controller.ts (1)
139-151: Comprehensive OIDC callback handling with appropriate error management.The callback implementation correctly:
- Sets refresh tokens
- Builds a redirect URL with appropriate parameters
- Includes error handling to redirect users to the sign-in page with an error message in case of failure
Consider adding logging to capture authentication failures for monitoring purposes.
async oidcAuthenticateCallback(@Req() req: NcRequest, @Res() res: Response) { try { await this.setRefreshToken({ req, res }); const redirectUrl = `${req.ncSiteUrl}?state=oidc&code=success`; return res.redirect(redirectUrl); } catch (e) { + console.error('OIDC authentication callback failed:', e); return res.redirect( `${req.ncSiteUrl}/signin?error=Authentication+failed`, ); } }packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts (2)
119-120: Consider avoiding thedeleteoperator for environment variables.Using the
deleteoperator onprocess.envis flagged by static analysis for potential performance concerns. While this is only used in tests, you could replace it with setting the variable toundefined:- delete process.env.NC_OIDC_USERINFO_URL; + process.env.NC_OIDC_USERINFO_URL = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
69-157: Add a dedicated test case for scenarios withreq.ncBase 8000 Id.The test suite comprehensively covers missing or invalid OIDC configuration, user profile retrieval, and user creation. However, a scenario verifying that
req.ncBaseIdtriggers base-specific role assignment isn't explicitly tested. Consider adding a test case to confirm that roles retrieved viaBaseUser.getare properly merged into the final authenticated user object.🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts (3)
48-52: Remove returned values or update function signature to avoid returning within avoidmethod.Static analysis flags returning values in a method typed as
void. If you wish to short-circuit, consider removing the direct return value fromthis.fail()orsuper.authenticate()and simply usereturn;afterward:@@ -48,5 +48,7 @@ - return this.fail({ + this.fail({ message: 'OIDC configuration not found. ...', }); + return; @@ -82,1 +84,2 @@ - return super.authenticate(req, authOptions); + super.authenticate(req, authOptions); + return;Also applies to: 82-82
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-51: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
117-126: Use async/await for consistent error handling instead of.then().catch().Most of this method uses async/await. However, the base user lookup at lines 118-123 uses promise chaining. Updating it to async/await would keep the style uniform and make error handling clearer:
- if (req.ncBaseId) { - BaseUser.get(req.context, req.ncBaseId, user.id) - .then(async (baseUser) => { - user.roles = baseUser?.roles || user.roles; - done(null, sanitiseUserObj(user)); - }) - .catch((e) => done(e)); - } else { - return done(null, sanitiseUserObj(user)); - } + if (!req.ncBaseId) { + return done(null, sanitiseUserObj(user)); + } + try { + const baseUser = await BaseUser.get(req.context, req.ncBaseId, user.id); + user.roles = baseUser?.roles || user.roles; + return done(null, sanitiseUserObj(user)); + } catch (e) { + return done(e); + }
129-139: Rename the newly created user variable to avoid overshadowing.Reusing the variable name
usercan cause confusion when reading the code. Consider renaming it to clarify that it is a newly created user:- const user = await this.usersService.registerNewUserIfAllowed({ + const newUser = await this.usersService.registerNewUserIfAllowed({ email_verification_token: null, email: email, password: '', salt, req, display_name: userInfo.name || userInfo.preferred_username || email.split('@')[0], } as any); - return done(null, sanitiseUserObj(user)); + return done(null, sanitiseUserObj(newUser));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/nocodb/package.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
packages/nc-gui/middleware/03.auth.global.ts(3 hunks)packages/nocodb/src/models/UserRefreshToken.ts(1 hunks)packages/nocodb/src/modules/auth/auth.controller.ts(1 hunks)packages/nocodb/src/modules/auth/auth.module.ts(2 hunks)packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts(1 hunks)packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/nocodb/src/modules/auth/auth.module.ts (1)
packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts (1)
OidcStrategyProvider(147-153)
packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts (1)
packages/nocodb/src/models/BaseUser.ts (1)
BaseUser(22-530)
🪛 Biome (1.9.4)
packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts
[error] 48-51: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 82-82: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🔇 Additional comments (9)
packages/nocodb/src/models/UserRefreshToken.ts (1)
120-122: Important security improvement for token validation.Adding explicit expiration check for refresh tokens is a good security practice that prevents the use of expired tokens. This change ensures that even if a token exists in the database but has passed its expiration date, it will be treated as invalid.
packages/nocodb/src/modules/auth/auth.module.ts (2)
11-11: LGTM - Clean import of OIDC strategy provider.
27-27: Correctly registered OIDC provider in the auth module.The OIDC strategy provider is now properly registered in the module's providers array, making it available for dependency injection throughout the authentication system.
packages/nc-gui/middleware/03.auth.global.ts (4)
49-50: Good expansion of auth provider check in middleware.The condition now correctly checks for both Google and OIDC auth being enabled, which aligns with the PR objective of adding OIDC support while maintaining existing functionality.
136-138: Comprehensive URL parameter detection for OIDC.The condition has been expanded to detect OIDC-specific parameters like
state=oidcandcode=success, which is essential for properly handling OIDC callbacks in addition to the existing OAuth flows.
144-146: Properly identifying OIDC provider from state parameter.The code correctly determines the authentication provider based on the state parameter, supporting multiple providers including the new OIDC option.
151-153: Added credentials handling for cross-origin requests.Adding
withCredentials: trueensures proper cookie handling during cross-origin requests, which is important for OIDC flows where cookies might be used for state management.packages/nocodb/src/modules/auth/auth.controller.ts (2)
125-131: Well-structured OIDC token generation endpoint.This endpoint for OIDC token generation follows the same pattern as the existing Google endpoint, ensuring consistency across authentication methods. The implementation correctly uses the OIDC auth guard and sets refresh tokens.
133-137: Clean implementation of OIDC authentication initiation.This endpoint follows the established pattern for authentication initiation and appropriately delegates request handling to the OIDC strategy.
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts (1)
120-120: Avoid using the delete operator on environment variables.Using
delete process.env.NC_OIDC_USERINFO_URLcan negatively impact performance and is not recommended. Instead, assign it toundefinedto simulate the variable being unset:- delete process.env.NC_OIDC_USERINFO_URL; + process.env.NC_OIDC_USERINFO_URL = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/nocodb/package.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
packages/nc-gui/middleware/03.auth.global.ts(3 hunks)packages/nocodb/src/models/UserRefreshToken.ts(1 hunks)packages/nocodb/src/modules/auth/auth.controller.ts(1 hunks)packages/nocodb/src/modules/auth/auth.module.ts(2 hunks)packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts(1 hunks)packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nocodb/src/modules/auth/auth.module.ts
- packages/nocodb/src/models/UserRefreshToken.ts
- packages/nc-gui/middleware/03.auth.global.ts
- packages/nocodb/src/modules/auth/auth.controller.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts (3)
packages/nocodb/src/modules/auth/auth.controller.ts (1)
refreshToken(67-78)packages/nc-gui/lib/types.ts (1)
User(617-617)packages/nocodb/src/models/BaseUser.ts (1)
BaseUser(22-530)
🪛 Biome (1.9.4)
packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts
[error] 48-51: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 82-82: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (10)
packages/nocodb/src/strategies/oidc.strategy/OidcStrategySource.spec.ts (5)
6-9: Mock setup appears correct and thorough.Mocking
axiosand the~/modelsbefore importing them ensures isolation in your tests, preventing unexpected side effects. This approach helps maintain test reliability.
19-20: Comprehensive user-related mocks.The
mockUsersService.registerNewUserIfAllowedstub andUser.getByEmailmock are well structured and aligned with the tested logic. This separation of concerns makes it easier to verify user creation vs. existing user scenarios.Also applies to: 53-54
74-95: Good coverage for existing user scenario.Validating that the user info is fetched and the existing user is retrieved by email thoroughly tests your OIDC integration path for returning users.
96-117: New user registration path is tested correctly.Ensuring the test covers a scenario where the user doesn’t exist and is subsequently created lets you confirm that the OIDC strategy gracefully handles first-time logins.
137-156: Robust error handling test for missing email.Simulating a userinfo response without an email helps confirm the strategy fails early and reports an appropriate error, preventing incomplete user records from being created.
packages/nocodb/src/strategies/oidc.strategy/oidc.strategy.ts (5)
39-47: Clear environment variable checks.Ensuring that each OIDC-related environment variable is present before proceeding helps prevent misconfiguration and produces actionable error messages for the user or administrator.
48-51: Returning from a function with a 'void' signature is normal in Passport flows.Although the static analysis warns about returning a value in a function typed with
void, in Passport-based strategiesthis.fail(...)orsuper.authenticate(...)are standard patterns. The warning can be safely ignored as this is how Passport signals authentication failure or success internally.Also applies to: 82-82
🧰 Tools
🪛 Biome (1.9.4)
[error] 48-51: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
60-63: Dynamically constructed callback URL is a nice touch.By defaulting to
process.env.NC_OIDC_CALLBACK_URLor deriving one fromreq.ncSiteUrl, you ensure flexibility across different deployment environments.
96-111: Fetching user info and validating essential fields.Your approach to retrieve user info from the OIDC userinfo endpoint and check for a valid email is fundamental for accurate user identification. The explicit error messaging for missing profile data adds clarity.
117-127: Logic for merging base-level roles and optionally registering new users is well-structured.Conditionally retrieving the base user roles (if
ncBaseIdis provided) and falling back to user creation if no existing user is found keeps the code organized and extends easily to multi-tenant or multi-base contexts.Also applies to: 128-139
Hi, new user and first-time contributor. This product is 🔥🔥🔥. It's a privilege to give back to OSS.
Change Summary
OpenID Connect (OIDC) is an identity authentication protocol built on top of OAuth 2.0 that has become an industry standard. It has also been a long-standing and popular feature request, with open issues going back to 2021 [1][2].
closes: #742
closes: #517
Status Quo
As of today, many forms of auth are only supported in the Enterprise edition of NocoDB, while the OSS version only allows for password/Google auth. The frontend already supports it, and the backend is very-capable of it. In this PR we do not modify frontend components, nor do we introduce any new database schema changes. We just plugin the missing pieces.
Change type
Test/ Verification
Verified locally with an OpenID Connect provider (Auth0).
Additional information / screenshots (optional)
Existing Env Vars
New Env Vars