feat(auth): Add full OpenID Connect (OIDC) authentication support (Full support for openid #37489)#37498
Conversation
- Implement native OpenID Connect authentication method - Add automatic endpoint discovery via `.well-known/openid-configuration` - Support ID token validation and claims extraction - Enable Single Logout (SLO) for centralized authentication - Add compatibility with major identity providers (Keycloak, Microsoft Entra ID, Okta) - Create comprehensive documentation and configuration examples - Implement server and client-side logout methods - Add support for configurable claim mappings and role extraction - Ensure backward compatibility with existing OAuth configurations Resolves RocketChat#37489 by providing a robust, standards-compliant OpenID Connect implementation that simplifies enterprise authentication and improves security. Full support for openid RocketChat#37489
This commit introduces full support for OpenID Connect (OIDC) as a first-class authentication method in Rocket.Chat. This addresses the limitations of the existing Custom OAuth implementation and provides a more robust and feature-rich integration with OIDC providers. Key features of this implementation include: - **Dedicated OpenID Connect Service:** A new, self-contained OIDC service that handles the entire authentication flow, from discovery to token validation. - **Single Log-Out (SLO):** Implements the OIDC Back-Channel Logout specification, allowing users to be logged out of Rocket.Chat when they log out of the OIDC provider. - **Admin UI Configuration:** A new settings page in the admin panel for configuring and enabling the OIDC provider. - **ID Token and Claims Handling:** The implementation now correctly parses the `id_token` and extracts user information from the claims. - **Dynamic Configuration:** The service is dynamically configured based on the OIDC provider's discovery document. This change resolves issue RocketChat#37489.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 34c2091 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
motalib-code seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis pull request adds comprehensive OpenID Connect (OIDC) authentication support to Rocket.Chat as a native authentication method. It includes automatic discovery, ID token validation via JWKS, Single Logout (SLO), provider configuration management, admin UI settings, and support for multiple enterprise providers with role/group mapping and user merging capabilities. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Rocket.Chat<br/>Client
participant Server as Rocket.Chat<br/>Server
participant IdP as OpenID<br/>Provider
rect rgba(100, 150, 200, 0.2)
Note over User,IdP: Authentication Flow
User->>Client: Click OpenID Login
Client->>Server: Redirect to /oauth/authorize?client_id=...
Server->>IdP: Authorization request
IdP->>User: Show login form
User->>IdP: Enter credentials
IdP->>User: Redirect with authorization code
User->>Server: Follow redirect with code
Server->>IdP: Exchange code for tokens (POST /token)
IdP->>Server: Return access_token + id_token
Server->>IdP: Fetch JWKS from jwks_uri
IdP->>Server: Return public keys
Server->>Server: Verify id_token signature
Server->>IdP: Fetch user info from userinfo endpoint
IdP->>Server: Return user claims
Server->>Server: Normalize identity & merge/create user
Server->>Client: Redirect to dashboard (logged in)
Client->>User: Display dashboard
end
rect rgba(200, 100, 100, 0.2)
Note over User,IdP: Single Logout (SLO) Flow
User->>Client: Click logout
Client->>Server: Call openid.performSingleLogout()
Server->>Server: Build logout URL with end_session_endpoint
Server->>Client: Return logout URL + id_token_hint
Client->>IdP: Redirect to end_session_endpoint
IdP->>IdP: Revoke session & tokens
IdP->>User: Redirect to post_logout_redirect_uri
User->>Client: Back at app
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
apps/meteor/app/openid-connect/server/slo.js (1)
2-2: Remove unused import.The
Accountsimport is not used anywhere in this file.Apply this diff to remove the unused import:
-import { Accounts } from 'meteor/accounts-base';apps/meteor/server/lib/openid/removeOpenIDService.ts (1)
7-9: Consider adding error handling for removal operations.The removal operations could fail silently if the service or settings section doesn't exist or if there's a database error. Consider adding try-catch blocks with appropriate logging.
Example:
export async function removeOpenIDService(mainSettingId: string): Promise<void> { + if (!mainSettingId.startsWith('Accounts_OpenID-')) { + throw new Error(`Invalid mainSettingId: expected to start with 'Accounts_OpenID-', got '${mainSettingId}'`); + } + const serviceName = mainSettingId.replace('Accounts_OpenID-', ''); + try { await ServiceConfiguration.configurations.removeAsync({ service: serviceName }); await settingsRegistry.removeSection(`OpenID Connect: ${serviceName}`); + } catch (error) { + console.error(`Failed to remove OpenID service ${serviceName}:`, error); + throw error; + } }apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsx (1)
27-39: Consider adding loading state during save operation.The save operation is asynchronous but provides no visual feedback. Adding a loading state would improve user experience by disabling the button and showing a spinner during the save.
Example:
const [isSaving, setIsSaving] = useState(false); const handleSave = async () => { setIsSaving(true); try { await settings.batchSet([...]); dispatchToastMessage({ type: 'success', message: 'Settings saved' }); } catch (error) { dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); } finally { setIsSaving(false); } }; // In JSX: <Button primary disabled={isSaving} onClick={handleSave}> {isSaving ? 'Saving...' : 'Save changes'} </Button>apps/meteor/server/lib/openid/initOpenIDServices.ts (1)
16-56: Standardize boolean parsing for consistency.The boolean parsing is inconsistent: some use
=== 'true'(strict true), others use!== 'false'(default true). This inconsistency makes the code harder to understand and maintain.Consider using a helper function:
const parseBooleanEnv = (value: string | undefined, defaultValue: boolean): boolean => { if (value === undefined) return defaultValue; return value === 'true'; }; // Then use consistently: const values = { enabled: parseBooleanEnv(process.env[`${serviceKey}`], false), useDiscovery: parseBooleanEnv(process.env[`${serviceKey}_use_discovery`], true), claimsFromIdToken: parseBooleanEnv(process.env[`${serviceKey}_claims_from_id_token`], true), // ... etc };apps/meteor/server/lib/openid/addOpenIDService.ts (1)
9-423: Consider batching settings registration for better performance.The function performs over 30 sequential
await settingsRegistry.add()calls. If the settings registry supports batch operations, consider using Promise.all() to register settings concurrently, which would significantly reduce initialization time.Example:
const settingsToAdd = [ { key: `Accounts_OpenID-${name}`, value: values.enabled || false, options: {...} }, { key: `Accounts_OpenID-${name}-url`, value: values.serverURL || '', options: {...} }, // ... all other settings ]; await Promise.all(settingsToAdd.map(({key, value, options}) => settingsRegistry.add(key, value, options) ));Note: Only apply if settingsRegistry.add() operations are truly independent and don't have ordering requirements.
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts (1)
1-119: Tests validate concepts but don't exercise the OpenIDConnect implementation.These tests verify understanding of OpenID Connect concepts (discovery documents, JWT structure, claim formats, etc.) but don't actually test the
OpenIDConnectclass implementation inOpenIDConnect.ts. They serve as good specification/documentation tests but don't provide code coverage.Consider adding unit tests that:
- Instantiate the OpenIDConnect class with test configurations
- Mock HTTP requests (fetch) for discovery, token exchange, and userinfo endpoints
- Test error handling paths (invalid tokens, failed discovery, network errors)
- Test token validation logic
- Test user merging and claim extraction logic
Example structure:
describe('OpenIDConnect class', () => { let oidc: OpenIDConnect; beforeEach(() => { // Mock fetch globally oidc = new OpenIDConnect('test', { serverURL: 'https://test.example.com', /* ... */ }); }); it('should load discovery document on initialization', async () => { // Test actual loadDiscoveryDocument method }); it('should handle discovery failure gracefully', async () => { // Test error case }); // ... more tests });Based on learnings.
packages/i18n/src/locales/en.i18n.json (1)
256-278: OIDC entries: add missing descriptions and tighten copy for consistency.
- Several settings lack “_Description” companions (Response Type, Userinfo Endpoint, JWKS URI, ID Token alg, Post‑Logout Redirect URI). Add them for parity with surrounding OAuth strings.
- Normalize examples/backticks and end descriptions with periods for consistency.
- Prefer backticks for claims and scope names.
Proposed patch:
@@ - "Accounts_OpenID_Claims_From_ID_Token": "Extract Claims from ID Token", - "Accounts_OpenID_Claims_From_ID_Token_Description": "Extract user claims from the ID token in addition to the userinfo endpoint", + "Accounts_OpenID_Claims_From_ID_Token": "Extract Claims from ID Token", + "Accounts_OpenID_Claims_From_ID_Token_Description": "Extract user claims from the ID token in addition to the userinfo endpoint.", @@ - "Accounts_OpenID_Description": "OpenID Connect Authentication", + "Accounts_OpenID_Description": "OpenID Connect Authentication", @@ - "Accounts_OpenID_Discovery_Endpoint": "Discovery Endpoint", - "Accounts_OpenID_Discovery_Endpoint_Description": "The OpenID Connect discovery endpoint URL (usually /.well-known/openid-configuration). Leave empty to use the default based on the server URL.", + "Accounts_OpenID_Discovery_Endpoint": "Discovery Endpoint", + "Accounts_OpenID_Discovery_Endpoint_Description": "The OpenID Connect discovery endpoint URL (usually `/.well-known/openid-configuration`). Leave empty to use the default based on the server URL.", @@ - "Accounts_OpenID_Enable_SLO": "Enable Single Logout (SLO)", - "Accounts_OpenID_Enable_SLO_Description": "Enable Single Logout to log users out of both Rocket.Chat and the identity provider simultaneously", + "Accounts_OpenID_Enable_SLO": "Enable Single Logout (SLO)", + "Accounts_OpenID_Enable_SLO_Description": "Enable Single Logout to log users out of both Rocket.Chat and the identity provider simultaneously.", @@ - "Accounts_OpenID_End_Session_Endpoint": "End Session Endpoint", - "Accounts_OpenID_End_Session_Endpoint_Description": "The logout endpoint URL for Single Logout. This is usually discovered automatically.", + "Accounts_OpenID_End_Session_Endpoint": "End Session Endpoint", + "Accounts_OpenID_End_Session_Endpoint_Description": "The logout endpoint URL for Single Logout. This is usually discovered automatically.", @@ - "Accounts_OpenID_ID_Token_Signing_Alg": "ID Token Signing Algorithm", + "Accounts_OpenID_ID_Token_Signing_Alg": "ID Token Signing Algorithm", + "Accounts_OpenID_ID_Token_Signing_Alg_Description": "Expected JWS alg for ID tokens (for example, `RS256`, `ES256`). Leave empty to accept the provider's metadata.", @@ - "Accounts_OpenID_JWKS_URI": "JWKS URI", + "Accounts_OpenID_JWKS_URI": "JWKS URI", + "Accounts_OpenID_JWKS_URI_Description": "JWKS endpoint used to validate ID token signatures. Usually discovered automatically.", @@ - "Accounts_OpenID_Post_Logout_Redirect_URI": "Post Logout Redirect URI", + "Accounts_OpenID_Post_Logout_Redirect_URI": "Post-Logout Redirect URI", + "Accounts_OpenID_Post_Logout_Redirect_URI_Description": "Where the provider should redirect the user after logout. Must be registered at the identity provider.", @@ - "Accounts_OpenID_Response_Type": "Response Type", + "Accounts_OpenID_Response_Type": "Response Type", + "Accounts_OpenID_Response_Type_Description": "OAuth 2.0 response type. Use `code` (Authorization Code + PKCE) unless you have a specific reason to use implicit or hybrid flows.", @@ - "Accounts_OpenID_Scope_Description": "OpenID Connect scopes (space-separated). Must include 'openid'. Common scopes: openid, profile, email", + "Accounts_OpenID_Scope_Description": "OpenID Connect scopes (space-separated). Must include `openid`. Common scopes: `openid`, `profile`, `email`.", @@ - "Accounts_OpenID_URL_Description": "The base URL of your OpenID Connect provider (e.g., https://accounts.google.com, https://login.microsoftonline.com/tenant-id)", + "Accounts_OpenID_URL_Description": "The base URL of your OpenID Connect provider. Example: `https://accounts.google.com`, `https://login.microsoftonline.com/<tenant-id>`.", @@ - "Accounts_OpenID_Use_Discovery": "Use OpenID Discovery", - "Accounts_OpenID_Use_Discovery_Description": "Automatically discover OpenID Connect endpoints from the discovery document. Recommended for most providers.", + "Accounts_OpenID_Use_Discovery": "Use OpenID Discovery", + "Accounts_OpenID_Use_Discovery_Description": "Automatically discover OpenID Connect endpoints from the discovery document. Recommended for most providers.", @@ - "Accounts_OpenID_Username_Field_Description": "Standard OpenID Connect claim for username. Common values: preferred_username, sub, email", + "Accounts_OpenID_Username_Field_Description": "OpenID Connect claim to derive the username. Common values: `preferred_username`, `sub`, `email`.", @@ - "Accounts_OpenID_Userinfo_Path": "Userinfo Endpoint", + "Accounts_OpenID_Userinfo_Path": "Userinfo Endpoint", + "Accounts_OpenID_Userinfo_Path_Description": "Userinfo endpoint URL. Usually discovered automatically; override only if discovery is disabled.", @@ - "Accounts_OpenID_Validate_ID_Token": "Validate ID Token", - "Accounts_OpenID_Validate_ID_Token_Description": "Validate the ID token signature using the provider's JWKS", + "Accounts_OpenID_Validate_ID_Token": "Validate ID Token", + "Accounts_OpenID_Validate_ID_Token_Description": "Validate the ID token signature using the provider's JWKS.",
- Please confirm the UI reads the new “_Description” keys; if not, I can align with whatever the settings renderer expects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
.changeset/openid-connect-support.md(1 hunks)IMPLEMENTATION_COMPLETE.md(1 hunks)OPENID_CONNECT_IMPLEMENTATION.md(1 hunks)apps/meteor/app/openid-connect/server/openid_connect_server.js(1 hunks)apps/meteor/app/openid-connect/server/settings.js(1 hunks)apps/meteor/app/openid-connect/server/slo.js(1 hunks)apps/meteor/app/openid-connect/server/startup.js(1 hunks)apps/meteor/client/lib/openidLogout.ts(1 hunks)apps/meteor/client/views/admin/settings/SettingsGroupSelector/SettingsGroupSelector.tsx(2 hunks)apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsx(1 hunks)apps/meteor/server/configuration/oauth.ts(2 hunks)apps/meteor/server/lib/openid/OpenIDConnect.spec.ts(1 hunks)apps/meteor/server/lib/openid/OpenIDConnect.ts(1 hunks)apps/meteor/server/lib/openid/QUICK_START.md(1 hunks)apps/meteor/server/lib/openid/README.md(1 hunks)apps/meteor/server/lib/openid/addOpenIDService.ts(1 hunks)apps/meteor/server/lib/openid/examples.env(1 hunks)apps/meteor/server/lib/openid/index.ts(1 hunks)apps/meteor/server/lib/openid/initOpenIDServices.ts(1 hunks)apps/meteor/server/lib/openid/logger.ts(1 hunks)apps/meteor/server/lib/openid/removeOpenIDService.ts(1 hunks)apps/meteor/server/lib/openid/updateOpenIDServices.ts(1 hunks)apps/meteor/server/lib/refreshLoginServices.ts(1 hunks)apps/meteor/server/methods/openidLogout.ts(1 hunks)apps/meteor/server/settings/oauth.ts(2 hunks)apps/meteor/server/startup/migrations/v318.ts(1 hunks)package.json(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: When generating tests, provide complete runnable TypeScript test files with proper imports, clear describe/test blocks, and adherence to these guidelines
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use test.beforeAll() and test.afterAll() for setup and teardown
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/lib/openid/removeOpenIDService.tsapps/meteor/server/settings/oauth.tsapps/meteor/server/startup/migrations/v318.ts
📚 Learning: 2025-09-16T22:09:18.041Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/test-cases.mdc:0-0
Timestamp: 2025-09-16T22:09:18.041Z
Learning: Use Rocket.Chat primary documentation and provided reference files for guidance
Applied to files:
apps/meteor/server/lib/openid/QUICK_START.mdapps/meteor/server/lib/openid/README.mdOPENID_CONNECT_IMPLEMENTATION.md
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Follow the Playwright Testing Guide and Rocket.Chat documentation as primary references
Applied to files:
apps/meteor/server/lib/openid/QUICK_START.md
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
apps/meteor/app/openid-connect/server/startup.js
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/server/configuration/oauth.tsapps/meteor/server/settings/oauth.ts
🧬 Code graph analysis (13)
apps/meteor/client/lib/openidLogout.ts (1)
apps/meteor/server/methods/openidLogout.ts (1)
serviceName(9-38)
apps/meteor/server/lib/openid/logger.ts (1)
apps/meteor/server/lib/openid/index.ts (1)
logger(6-6)
apps/meteor/server/lib/openid/updateOpenIDServices.ts (4)
apps/meteor/server/lib/openid/logger.ts (1)
logger(3-3)apps/meteor/server/lib/openid/OpenIDConnect.ts (1)
OpenIDConnect(79-657)packages/models/src/index.ts (1)
LoginServiceConfiguration(182-182)apps/meteor/app/lib/server/lib/notifyListener.ts (2)
notifyOnLoginServiceConfigurationChangedByService(178-189)notifyOnLoginServiceConfigurationChanged(165-176)
apps/meteor/server/methods/openidLogout.ts (3)
apps/meteor/server/lib/openid/OpenIDConnect.ts (1)
Services(660-660)apps/meteor/server/lib/openid/index.ts (2)
Services(1-1)logger(6-6)apps/meteor/server/lib/openid/logger.ts (1)
logger(3-3)
apps/meteor/server/lib/openid/removeOpenIDService.ts (1)
apps/meteor/server/lib/openid/index.ts (1)
removeOpenIDService(5-5)
apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsx (2)
packages/ui-contexts/src/index.ts (3)
useToastMessageDispatch(75-75)useSetting(69-69)useSettings(70-70)apps/meteor/app/openid-connect/server/slo.js (1)
issuer(19-19)
apps/meteor/server/lib/openid/OpenIDConnect.ts (4)
apps/meteor/server/lib/openid/logger.ts (1)
logger(3-3)apps/meteor/server/methods/openidLogout.ts (1)
serviceName(9-38)apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnUserChange(413-425)
apps/meteor/server/lib/openid/initOpenIDServices.ts (1)
apps/meteor/server/lib/openid/addOpenIDService.ts (1)
addOpenIDService(5-423)
apps/meteor/server/lib/refreshLoginServices.ts (2)
apps/meteor/server/lib/openid/updateOpenIDServices.ts (1)
updateOpenIDServices(12-118)apps/meteor/app/meteor-accounts-saml/server/lib/settings.ts (1)
loadSamlServiceProviders(111-148)
apps/meteor/app/openid-connect/server/openid_connect_server.js (1)
apps/meteor/app/openid-connect/server/slo.js (3)
discovery(22-22)jwks(23-23)jwtVerify(26-29)
apps/meteor/app/openid-connect/server/startup.js (2)
apps/meteor/app/openid-connect/server/slo.js (1)
issuer(19-19)apps/meteor/app/openid-connect/server/openid_connect_server.js (1)
OpenIDConnect(17-179)
apps/meteor/server/lib/openid/addOpenIDService.ts (1)
apps/meteor/server/lib/openid/index.ts (1)
addOpenIDService(2-2)
apps/meteor/server/configuration/oauth.ts (3)
apps/meteor/server/lib/openid/updateOpenIDServices.ts (1)
updateOpenIDServices(12-118)apps/meteor/server/lib/openid/removeOpenIDService.ts (1)
removeOpenIDService(4-9)apps/meteor/server/lib/openid/initOpenIDServices.ts (1)
initOpenIDServices(3-62)
🪛 Biome (2.1.2)
apps/meteor/app/openid-connect/server/openid_connect_server.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 3-4: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 6-7: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 7-8: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 8-9: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 9-10: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 16-179: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
apps/meteor/app/openid-connect/server/slo.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 3-4: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 7-8: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
apps/meteor/app/openid-connect/server/startup.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 3-4: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
apps/meteor/app/openid-connect/server/settings.js
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🪛 LanguageTool
IMPLEMENTATION_COMPLETE.md
[grammar] ~76-~76: Use a hyphen to join words.
Context: ... Google - ✅ Auth0 - ✅ Any OpenID Connect compliant provider **Configuration Exam...
(QB_NEW_EN_HYPHEN)
apps/meteor/server/lib/openid/README.md
[grammar] ~25-~25: Use a hyphen to join words.
Context: ...gle** - Auth0 - Any OpenID Connect compliant provider ## Configuration ...
(QB_NEW_EN_HYPHEN)
OPENID_CONNECT_IMPLEMENTATION.md
[grammar] ~30-~30: Use a hyphen to join words.
Context: ...oogle - Auth0 - Any OpenID Connect compliant provider 4. **Enterprise Feat...
(QB_NEW_EN_HYPHEN)
[style] ~171-~171: ‘old Custom’ might be wordy. Consider a shorter alternative.
Context: ...ogout works (if enabled) 4. Disable the old Custom OAuth service 5. Remove the old Custom ...
(EN_WORDINESS_PREMIUM_OLD_CUSTOM)
[style] ~172-~172: ‘old Custom’ might be wordy. Consider a shorter alternative.
Context: ... old Custom OAuth service 5. Remove the old Custom OAuth configuration ## Security Consid...
(EN_WORDINESS_PREMIUM_OLD_CUSTOM)
🪛 markdownlint-cli2 (0.18.1)
IMPLEMENTATION_COMPLETE.md
12-12: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
46-46: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
83-83: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
126-126: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (26)
.changeset/openid-connect-support.md (1)
1-44: Comprehensive and well-documented changeset.The changelog entry clearly describes the new OpenID Connect feature, its benefits, configuration options, and migration guidance. This will help users understand and adopt the new functionality.
apps/meteor/server/lib/refreshLoginServices.ts (2)
5-5: LGTM: OpenID service update integration.The import follows the established pattern for other login services.
11-11: LGTM: Parallel service updates.Adding
updateOpenIDServices()to thePromise.allSettledarray ensures OpenID services are refreshed alongside OAuth, SAML, and CAS services. The use ofallSettledprevents one service's failure from blocking others.apps/meteor/server/lib/openid/logger.ts (1)
1-3: LGTM: Clean logger instantiation.The logger setup follows the standard pattern used elsewhere in the codebase.
apps/meteor/server/settings/oauth.ts (2)
431-440: Important fix: Removed premature return from Proxy section.Changing from an early
returntoawaitfor the Proxy section allows subsequent sections (like the new OpenID Connect section) to be properly registered. This was likely a bug that prevented adding new sections after Proxy.
442-449: LGTM: OpenID Connect settings section.The new section follows the established pattern and appropriately uses a read-only description field with i18n support. This provides a clear entry point for OpenID Connect configuration in the admin UI.
apps/meteor/client/views/admin/settings/SettingsGroupSelector/SettingsGroupSelector.tsx (2)
8-8: LGTM: Component import.The import follows the established pattern for settings group pages.
27-29: LGTM: Routing for OpenID Connect settings.The conditional rendering follows the same pattern as other authentication method settings pages (OAuth, LDAP), providing a consistent admin UI experience.
apps/meteor/app/openid-connect/server/startup.js (1)
1-4: Static analysis warnings can be ignored.The Biome warnings about "Illegal use of an import declaration outside of a module" are false positives. Meteor's build system handles ES6 module syntax in .js files correctly.
apps/meteor/server/configuration/oauth.ts (5)
7-9: LGTM: OpenID service imports.The imports mirror the OAuth service structure, maintaining consistency in the codebase.
13-13: LGTM: Debounced service updates.Using a 2-second debounce for OpenID service updates matches the OAuth pattern and prevents excessive updates when multiple settings change simultaneously.
25-27: LGTM: OpenID settings watcher.The regex pattern
/^Accounts_OpenID-.+/correctly captures all OpenID-related settings and triggers service updates, following the established OAuth pattern.
29-33: LGTM: Service removal watcher.The pattern correctly identifies individual OpenID service toggles and removes the service when disabled, maintaining parity with OAuth service management.
36-36: LGTM: OpenID initialization.Initializing OpenID services after OAuth services ensures proper startup sequencing.
package.json (1)
87-88: Version 5.2.4 is free from known vulnerabilities and acceptable to use.The latest version of jose is 6.1.1, but all listed security advisories affect versions ≤ 4.15.4, meaning version 5.2.4 is not vulnerable to any of the reported advisories. No action required unless upgrading to 6.1.1 for additional features or improvements is desired.
apps/meteor/client/lib/openidLogout.ts (1)
3-20: LGTM!The implementation correctly wraps the Meteor method call in a Promise, handles errors appropriately, and performs the redirect when a logout URL is returned. The
resolve()after the redirect is acceptable since it allows for proper cleanup in cases where the redirect is delayed or doesn't occur.apps/meteor/server/startup/migrations/v318.ts (1)
7-46: LGTM!This migration provides a helpful informational notice to users about the new OpenID Connect support. The approach of checking for existing Custom OAuth configurations and logging detailed guidance is user-friendly and non-destructive. The migration properly avoids automatic changes that could break existing configurations.
apps/meteor/server/lib/openid/examples.env (1)
1-125: LGTM!The configuration examples are comprehensive and well-organized, covering multiple popular identity providers (Microsoft Entra ID, Keycloak, Okta, Google, Auth0) with clear sections for advanced configuration options including claim mappings, role/group mapping, user merging, and UI customization.
apps/meteor/server/lib/openid/README.md (1)
1-264: LGTM!This is excellent comprehensive documentation that covers all aspects of the OpenID Connect implementation including features, configuration (via environment variables and Admin UI), provider-specific examples, Single Logout setup, claim mapping, troubleshooting, architecture overview, and security considerations. The documentation provides clear guidance for users migrating from Custom OAuth to OpenID Connect.
apps/meteor/server/lib/openid/QUICK_START.md (1)
1-214: LGTM!This quick start guide provides clear, actionable steps for setting up OpenID Connect with popular providers. The 5-minute setup approach, provider-specific configurations, troubleshooting section, and testing checklist make this highly valuable for users. The guide nicely complements the comprehensive README.md with a more streamlined onboarding experience.
apps/meteor/server/methods/openidLogout.ts (1)
8-38: LGTM!The implementation demonstrates excellent defensive programming with comprehensive validation at each step: input validation, user authentication check, service existence verification, SLO enablement check, and proper error handling with structured error messages. The use of the logger for warnings and errors aids in debugging, and the method correctly returns
nullwhen SLO is disabled rather than throwing an error, allowing graceful degradation.apps/meteor/server/lib/openid/addOpenIDService.ts (1)
5-8: LGTM: Name normalization is appropriate.The name normalization (lowercase, remove invalid chars, capitalize) ensures consistent service naming across the system.
apps/meteor/server/lib/openid/updateOpenIDServices.ts (1)
109-115: LGTM: Service removal logic is safe and well-structured.The service removal path properly checks for service existence, tracks deletion count, and only notifies when actual deletion occurs. The optional chaining and conditional logic prevent errors.
OPENID_CONNECT_IMPLEMENTATION.md (1)
1-203: Excellent comprehensive documentation.This documentation clearly explains the OpenID Connect implementation, its differences from Custom OAuth, configuration examples, architecture, and security considerations. It effectively addresses the requirements from issue #37489 and provides practical guidance for users.
Minor optional improvement: Consider addressing the grammar suggestions from static analysis (hyphenating "OpenID-Connect-compliant" on line 30 and simplifying "old Custom OAuth" on lines 171-172), though these are purely stylistic.
apps/meteor/server/lib/openid/index.ts (1)
1-6: LGTM: Clean barrel export pattern.This index file provides a clean, centralized entry point for the OpenID Connect module, following standard TypeScript/ES module patterns.
apps/meteor/app/openid-connect/server/settings.js (1)
1-24: No alignment issues or conflicts found—the two settings registration paths serve distinct purposes.The 4 global OIDC settings in
settings.js(OIDC_Enable, OIDC_Issuer, OIDC_Client_ID, OIDC_Client_Secret) are foundational configuration used bystartup.jsandslo.js. The 40+ provider-specific settings registered byaddOpenIDService.tsare dynamically created per OIDC provider with namespacing (Accounts_OpenID-${providerName}-*), organized in separate groups and sections. The architecture is already self-documenting through distinct naming, grouping, and sectioning—no changes needed.
| if (this.discovery.token_endpoint_auth_methods_supported.includes('client_secret_basic')) { | ||
| const b64 = Buffer.from(`${config.clientId}:${OAuth.openSecret(config.secret)}`).toString('base64'); | ||
| headers.Authorization = `Basic ${b64}`; | ||
| } else { | ||
| params.append('client_id', config.clientId); | ||
| params.append('client_secret', OAuth.openSecret(config.secret)); | ||
| } |
There was a problem hiding this comment.
Guard discovery metadata before calling .includes
token_endpoint_auth_methods_supported is OPTIONAL in the discovery document, and when a provider omits it the current code throws TypeError: Cannot read properties of undefined (reading 'includes'), aborting the token exchange for an otherwise compliant IdP. Please guard the field (and default to client_secret_basic) before checking for client_secret_basic.
- if (this.discovery.token_endpoint_auth_methods_supported.includes('client_secret_basic')) {
+ const authMethods = this.discovery.token_endpoint_auth_methods_supported;
+ if (Array.isArray(authMethods) ? authMethods.includes('client_secret_basic') : true) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.discovery.token_endpoint_auth_methods_supported.includes('client_secret_basic')) { | |
| const b64 = Buffer.from(`${config.clientId}:${OAuth.openSecret(config.secret)}`).toString('base64'); | |
| headers.Authorization = `Basic ${b64}`; | |
| } else { | |
| params.append('client_id', config.clientId); | |
| params.append('client_secret', OAuth.openSecret(config.secret)); | |
| } | |
| const authMethods = this.discovery.token_endpoint_auth_methods_supported; | |
| if (Array.isArray(authMethods) ? authMethods.includes('client_secret_basic') : true) { | |
| const b64 = Buffer.from(`${config.clientId}:${OAuth.openSecret(config.secret)}`).toString('base64'); | |
| headers.Authorization = `Basic ${b64}`; | |
| } else { | |
| params.append('client_id', config.clientId); | |
| params.append('client_secret', OAuth.openSecret(config.secret)); | |
| } |
| this.add('OIDC_Client_Secret', '', { | ||
| type: 'string', | ||
| public: true, | ||
| secret: true, | ||
| }); |
There was a problem hiding this comment.
Critical: Remove 'public: true' from client secret setting.
The OIDC_Client_Secret setting is marked as both public: true and secret: true, which is a security vulnerability. The public: true flag would expose the client secret to clients, defeating the purpose of secret: true.
Apply this diff:
this.add('OIDC_Client_Secret', '', {
type: 'string',
- public: true,
secret: true,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.add('OIDC_Client_Secret', '', { | |
| type: 'string', | |
| public: true, | |
| secret: true, | |
| }); | |
| this.add('OIDC_Client_Secret', '', { | |
| type: 'string', | |
| secret: true, | |
| }); |
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/settings.js around lines 19 to 23, the
OIDC_Client_Secret setting is incorrectly declared with both public: true and
secret: true; remove the public: true property so the setting is only secret
(keep type: 'string' and secret: true) to ensure the client secret is not
exposed to clients and update any related tests or code references if they
relied on the public flag.
| WebApp.connectHandlers.use('/_slo', async (req, res, next) => { | ||
| const { logout_token } = req.body; | ||
|
|
||
| if (!logout_token) { | ||
| res.writeHead(400); | ||
| res.end('Missing logout_token'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Request body is not parsed - SLO endpoint will not work.
req.body will be undefined because WebApp's Connect handlers do not automatically parse request bodies. This will cause the endpoint to always return "Missing logout_token" even when a valid token is provided.
You need to add body parsing middleware. Apply this diff:
+import bodyParser from 'body-parser';
+
-WebApp.connectHandlers.use('/_slo', async (req, res, next) => {
+WebApp.connectHandlers.use('/_slo', bodyParser.urlencoded({ extended: false }));
+WebApp.connectHandlers.use('/_slo', bodyParser.json());
+WebApp.connectHandlers.use('/_slo', async (req, res) => {
const { logout_token } = req.body;Note: Also remove the unused next parameter if body parsing is added.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/slo.js around lines 10-17, the handler
reads req.body but WebApp.connectHandlers does not parse request bodies, so
req.body is undefined; update the file to parse the request body (either by
adding a body-parsing middleware registered on WebApp.connectHandlers before
this route or by reading request data inside this handler: accumulate
req.on('data')/('end') and parse as application/json or
application/x-www-form-urlencoded to extract logout_token), ensure proper error
handling for parse failures, and remove the unused next parameter from the
handler signature.
| const issuer = settings.get('OIDC_Issuer'); | ||
| const audience = settings.get('OIDC_Client_ID'); | ||
|
|
||
| const discovery = await (await fetch(`${issuer}/.well-known/openid-configuration`)).json(); | ||
| const jwks = createRemoteJWKSet(new URL(discovery.jwks_uri)); |
There was a problem hiding this comment.
Add error handling for discovery document fetch.
The discovery document fetch and JWKS URI access lack error handling. Network failures, invalid JSON responses, or missing jwks_uri fields will cause unhandled promise rejections.
Wrap this code in a try-catch block and validate the response:
+ try {
const issuer = settings.get('OIDC_Issuer');
const audience = settings.get('OIDC_Client_ID');
const discovery = await (await fetch(`${issuer}/.well-known/openid-configuration`)).json();
+
+ if (!discovery.jwks_uri) {
+ throw new Error('jwks_uri not found in discovery document');
+ }
+
const jwks = createRemoteJWKSet(new URL(discovery.jwks_uri));
+ } catch (err) {
+ res.writeHead(500);
+ res.end('Failed to load discovery document');
+ return;
+ }Note: You'll need to adjust the try-catch scope to properly handle the flow.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/slo.js around lines 19 to 23, the fetch
of the discovery document and creation of the JWKS set lack error handling; wrap
the network and JSON parsing operations in a try-catch, first await fetch and
check response.ok and throw a descriptive error if not, then await
response.json() inside try and validate that discovery.jwks_uri exists and is a
valid URL before calling createRemoteJWKSet(new URL(...)); on any error log or
throw a clear error so the calling flow can handle it and avoid unhandled
promise rejections.
| const user = await Users.findOneBySvcId('oidc', payload.sub); | ||
| if (user) { | ||
| await Users.unsetLoginTokens(user._id); | ||
| } |
There was a problem hiding this comment.
Validate payload.sub exists before use.
The code assumes payload.sub exists without validation. If the logout token doesn't contain a sub claim, this could lead to unexpected behavior.
Add validation:
const { payload } = await jwtVerify(logout_token, jwks, {
issuer,
audience,
});
+ if (!payload.sub) {
+ res.writeHead(400);
+ res.end('Invalid logout_token: missing sub claim');
+ return;
+ }
+
const user = await Users.findOneBySvcId('oidc', payload.sub);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/slo.js around lines 31 to 34, the code
uses payload.sub without validating it; check that payload exists and
payload.sub is a non-empty string before calling Users.findOneBySvcId and
Users.unsetLoginTokens. If payload.sub is missing or invalid, early-return (or
log a warning/error) and avoid calling database functions; ensure any logging
includes enough context (e.g., issuer/session id) to aid debugging.
| <Field.Label>Client Secret</Field.Label> | ||
| <Field.Row> | ||
| <TextInput value={clientSecret} onChange={(e) => setClientSecret(e.target.value)} /> | ||
| </Field.Row> |
There was a problem hiding this comment.
Use password-type input for client secret field.
The client secret is displayed as plain text, which is a security concern. Use a password-type input or Fuselage's PasswordInput component to mask the secret value.
Apply this diff:
<Field>
<Field.Label>Client Secret</Field.Label>
<Field.Row>
- <TextInput value={clientSecret} onChange={(e) => setClientSecret(e.target.value)} />
+ <TextInput type='password' value={clientSecret} onChange={(e) => setClientSecret(e.target.value)} />
</Field.Row>
</Field>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Field.Label>Client Secret</Field.Label> | |
| <Field.Row> | |
| <TextInput value={clientSecret} onChange={(e) => setClientSecret(e.target.value)} /> | |
| </Field.Row> | |
| <Field> | |
| <Field.Label>Client Secret</Field.Label> | |
| <Field.Row> | |
| <TextInput type='password' value={clientSecret} onChange={(e) => setClientSecret(e.target.value)} /> | |
| </Field.Row> | |
| </Field> |
🤖 Prompt for AI Agents
In
apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsx
around lines 63 to 66, the client secret field uses a plain TextInput which
exposes the secret; replace it with a password-masked input (e.g., Fuselage's
PasswordInput or a TextInput with type="password") to hide the value, keep the
value and onChange handler exactly as-is, and add the necessary import for
PasswordInput at the top of the file if using that component.
| for await (const key of Object.keys(process.env)) { | ||
| if (/Accounts_OpenID_[a-zA-Z0-9_-]+$/.test(key)) { | ||
| let name = key.replace('Accounts_OpenID_', ''); | ||
|
|
||
| if (name.indexOf('_') > -1) { | ||
| name = name.replace(name.substr(name.indexOf('_')), ''); | ||
| } |
There was a problem hiding this comment.
Simplify to synchronous iteration and clarify name extraction logic.
for awaitis unnecessary here sinceObject.keys()returns a regular array, not an async iterable.- The name extraction logic on lines 9-11 is confusing: it removes everything after the first underscore, which would turn
Accounts_OpenID_Azure_ADinto justAzure. This seems unintended.
Apply this diff to fix both issues:
- for await (const key of Object.keys(process.env)) {
+ for (const key of Object.keys(process.env)) {
if (/Accounts_OpenID_[a-zA-Z0-9_-]+$/.test(key)) {
- let name = key.replace('Accounts_OpenID_', '');
-
- if (name.indexOf('_') > -1) {
- name = name.replace(name.substr(name.indexOf('_')), '');
- }
+ const name = key.replace('Accounts_OpenID_', '').split('_')[0];
const serviceKey = `Accounts_OpenID_${name}`;This makes it clearer that we're extracting only the first segment after Accounts_OpenID_.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for await (const key of Object.keys(process.env)) { | |
| if (/Accounts_OpenID_[a-zA-Z0-9_-]+$/.test(key)) { | |
| let name = key.replace('Accounts_OpenID_', ''); | |
| if (name.indexOf('_') > -1) { | |
| name = name.replace(name.substr(name.indexOf('_')), ''); | |
| } | |
| for (const key of Object.keys(process.env)) { | |
| if (/Accounts_OpenID_[a-zA-Z0-9_-]+$/.test(key)) { | |
| const name = key.replace('Accounts_OpenID_', '').split('_')[0]; | |
| const serviceKey = `Accounts_OpenID_${name}`; |
🤖 Prompt for AI Agents
In apps/meteor/server/lib/openid/initOpenIDServices.ts around lines 5 to 11,
replace the unnecessary async iteration and the confusing substring logic: use a
synchronous loop (for...of) over Object.keys(process.env) instead of for await,
and extract the provider name by splitting on '_' and taking the first segment
(name = name.split('_')[0]) so the intent of "first segment after
Accounts_OpenID_" is explicit and readable.
| // OpenID Connect: Extract claims from ID token if enabled | ||
| if (this.claimsFromIdToken && idToken) { | ||
| try { | ||
| identity = this.decodeIdToken(idToken); | ||
| logger.debug({ msg: 'Claims from ID token', identity }); | ||
| } catch (error: any) { | ||
| logger.warn(`Failed to decode ID token: ${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| // Fetch additional user info from userinfo endpoint | ||
| if (this.userinfoPath) { | ||
| const params: Record<string, string> = {}; | ||
| const headers: Record<string, string> = { | ||
| 'User-Agent': this.userAgent, | ||
| 'Accept': 'application/json', | ||
| }; | ||
|
|
||
| if (this.identityTokenSentVia === 'header') { | ||
| headers.Authorization = `Bearer ${accessToken}`; | ||
| } else { | ||
| params[this.accessTokenParam] = accessToken; | ||
| } | ||
|
|
||
| try { | ||
| const url = new URL(this.userinfoPath); | ||
| Object.keys(params).forEach((key) => url.searchParams.append(key, params[key])); | ||
|
|
||
| const request = await fetch(url.toString(), { | ||
| method: 'GET', | ||
| headers, | ||
| }); | ||
|
|
||
| if (!request.ok) { | ||
| throw new Error(request.statusText); | ||
| } | ||
|
|
||
| const userinfoResponse = await request.json(); | ||
| logger.debug({ msg: 'Userinfo response', userinfoResponse }); | ||
|
|
||
| // Merge userinfo with ID token claims (userinfo takes precedence) | ||
| identity = { ...identity, ...userinfoResponse }; | ||
| } catch (err: any) { | ||
| logger.warn(`Failed to fetch userinfo from ${this.userinfoPath}: ${err.message}`); | ||
| // Continue with ID token claims if userinfo fails | ||
| } | ||
| } | ||
|
|
||
| return this.normalizeIdentity(identity); | ||
| } | ||
|
|
||
| decodeIdToken(idToken: string): any { | ||
| // Simple JWT decode (payload only, no signature verification here) | ||
| // For production, you should verify the signature using jwksUri | ||
| try { | ||
| const parts = idToken.split('.'); | ||
| if (parts.length !== 3) { | ||
| throw new Error('Invalid ID token format'); | ||
| } | ||
|
|
||
| const payload = parts[1]; | ||
| const decoded = Buffer.from(payload, 'base64').toString('utf8'); | ||
| return JSON.parse(decoded); | ||
| } catch (error: any) { | ||
| throw new Error(`Failed to decode ID token: ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
ID tokens are never verified
claimsFromIdToken defaults to true, but decodeIdToken only base64-decodes the payload and never verifies the signature or validates issuer/audience, despite validateIdToken being true by default. This lets any caller forge an ID token and impersonate users—exactly what the OpenID Connect Core spec forbids. Please validate the ID token with the OP’s JWKS (for example via createRemoteJWKSet/jwtVerify) and only fall back to userinfo if verification succeeds.
- if (this.claimsFromIdToken && idToken) {
- try {
- identity = this.decodeIdToken(idToken);
+ if (this.claimsFromIdToken && idToken) {
+ try {
+ if (this.validateIdToken) {
+ const jwks = await this.getJWKS();
+ const { payload } = await jwtVerify(idToken, jwks, {
+ issuer: this.discoveryDocument?.issuer ?? this.serverURL,
+ audience: config.clientId,
+ });
+ identity = payload;
+ } else {
+ identity = this.decodeIdToken(idToken);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/server/lib/openid/OpenIDConnect.ts around lines 312 to 378, the
current decodeIdToken simply base64-decodes the JWT and never verifies signature
or validates issuer/audience even though validateIdToken defaults to true;
replace this behavior by performing full JWT verification when validateIdToken
is true: fetch the provider JWKS (use createRemoteJWKSet with the configured
jwksUri), call jwtVerify on the idToken with appropriate verification options
(issuer, audience/client_id, algorithms), and extract the claims from the
verified token; if verification fails, log/throw and do not accept the ID token
claims (only fall back to userinfo if verification fails or validateIdToken is
false); keep a minimal non-verified decode path only when validateIdToken is
explicitly disabled.
| export async function removeOpenIDService(mainSettingId: string): Promise<void> { | ||
| const serviceName = mainSettingId.replace('Accounts_OpenID-', ''); | ||
|
|
There was a problem hiding this comment.
Validate input parameter before string replacement.
The function assumes mainSettingId always starts with 'Accounts_OpenID-' without validation. If called with an unexpected value, it could derive an incorrect service name and potentially remove the wrong service.
Add input validation:
export async function removeOpenIDService(mainSettingId: string): Promise<void> {
+ if (!mainSettingId.startsWith('Accounts_OpenID-')) {
+ throw new Error(`Invalid mainSettingId: expected to start with 'Accounts_OpenID-', got '${mainSettingId}'`);
+ }
+
const serviceName = mainSettingId.replace('Accounts_OpenID-', '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function removeOpenIDService(mainSettingId: string): Promise<void> { | |
| const serviceName = mainSettingId.replace('Accounts_OpenID-', ''); | |
| export async function removeOpenIDService(mainSettingId: string): Promise<void> { | |
| if (!mainSettingId.startsWith('Accounts_OpenID-')) { | |
| throw new Error(`Invalid mainSettingId: expected to start with 'Accounts_OpenID-', got '${mainSettingId}'`); | |
| } | |
| const serviceName = mainSettingId.replace('Accounts_OpenID-', ''); |
🤖 Prompt for AI Agents
In apps/meteor/server/lib/openid/removeOpenIDService.ts around lines 4 to 6, the
function blindly derives serviceName by replacing 'Accounts_OpenID-' without
validating mainSettingId; add input validation to ensure mainSettingId is a
non-empty string and startsWith('Accounts_OpenID-') before computing
serviceName, and if the check fails either throw a clear Error or log and return
early to avoid removing the wrong service; then derive serviceName using
substring after the known prefix to be explicit.
| const data: Partial<ILoginServiceConfiguration & Omit<OAuthConfiguration, '_id'>> = { | ||
| clientId: settings.get(`${key}-id`), | ||
| secret: settings.get(`${key}-secret`), | ||
| custom: true, | ||
| _OpenIDConnect: true, | ||
| serverURL: settings.get(`${key}-url`), | ||
| useDiscovery: settings.get(`${key}-use_discovery`), | ||
| discoveryEndpoint: settings.get(`${key}-discovery_endpoint`), | ||
| tokenPath: settings.get(`${key}-token_path`), | ||
| authorizePath: settings.get(`${key}-authorize_path`), | ||
| userinfoPath: settings.get(`${key}-userinfo_path`), | ||
| jwksUri: settings.get(`${key}-jwks_uri`), | ||
| endSessionEndpoint: settings.get(`${key}-end_session_endpoint`), | ||
| scope: settings.get(`${key}-scope`), | ||
| responseType: settings.get(`${key}-response_type`), | ||
| idTokenSigningAlg: settings.get(`${key}-id_token_signing_alg`), | ||
| claimsFromIdToken: settings.get(`${key}-claims_from_id_token`), | ||
| validateIdToken: settings.get(`${key}-validate_id_token`), | ||
| enableSLO: settings.get(`${key}-enable_slo`), | ||
| postLogoutRedirectUri: settings.get(`${key}-post_logout_redirect_uri`), | ||
| buttonLabelText: settings.get(`${key}-button_label_text`), | ||
| buttonLabelColor: settings.get(`${key}-button_label_color`), | ||
| loginStyle: settings.get(`${key}-login_style`), | ||
| buttonColor: settings.get(`${key}-button_color`), | ||
| tokenSentVia: settings.get(`${key}-token_sent_via`), | ||
| identityTokenSentVia: settings.get(`${key}-identity_token_sent_via`), | ||
| keyField: settings.get(`${key}-key_field`), | ||
| usernameField: settings.get(`${key}-username_field`), | ||
| emailField: settings.get(`${key}-email_field`), | ||
| nameField: settings.get(`${key}-name_field`), | ||
| avatarField: settings.get(`${key}-avatar_field`), | ||
| rolesClaim: settings.get(`${key}-roles_claim`), | ||
| groupsClaim: settings.get(`${key}-groups_claim`), | ||
| channelsMap: settings.get(`${key}-groups_channel_map`), | ||
| channelsAdmin: settings.get(`${key}-channels_admin`), | ||
| mergeUsers: settings.get(`${key}-merge_users`), | ||
| mergeUsersDistinctServices: settings.get(`${key}-merge_users_distinct_services`), | ||
| mapChannels: settings.get(`${key}-map_channels`), | ||
| mergeRoles: settings.get(`${key}-merge_roles`), | ||
| rolesToSync: settings.get(`${key}-roles_to_sync`), | ||
| showButton: settings.get(`${key}-show_button`), | ||
| }; | ||
|
|
||
| // Initialize OpenID Connect service | ||
| new OpenIDConnect(serviceKey, { | ||
| serverURL: data.serverURL as string, | ||
| useDiscovery: data.useDiscovery as boolean, | ||
| discoveryEndpoint: data.discoveryEndpoint as string, | ||
| tokenPath: data.tokenPath as string, | ||
| authorizePath: data.authorizePath as string, | ||
| userinfoPath: data.userinfoPath as string, | ||
| jwksUri: data.jwksUri as string, | ||
| endSessionEndpoint: data.endSessionEndpoint as string, | ||
| scope: data.scope as string, | ||
| responseType: data.responseType as string, | ||
| idTokenSigningAlg: data.idTokenSigningAlg as string, | ||
| claimsFromIdToken: data.claimsFromIdToken as boolean, | ||
| validateIdToken: data.validateIdToken as boolean, | ||
| enableSLO: data.enableSLO as boolean, | ||
| postLogoutRedirectUri: data.postLogoutRedirectUri as string, | ||
| loginStyle: data.loginStyle as 'redirect' | 'popup' | '', | ||
| tokenSentVia: data.tokenSentVia as 'header' | 'payload', | ||
| identityTokenSentVia: data.identityTokenSentVia as 'header' | 'payload' | 'default', | ||
| keyField: data.keyField as 'username' | 'email', | ||
| usernameField: data.usernameField as string, | ||
| emailField: data.emailField as string, | ||
| nameField: data.nameField as string, | ||
| avatarField: data.avatarField as string, | ||
| rolesClaim: data.rolesClaim as string, | ||
| groupsClaim: data.groupsClaim as string, | ||
| mapChannels: data.mapChannels as boolean, | ||
| channelsMap: data.channelsMap as string, | ||
| channelsAdmin: data.channelsAdmin as string, | ||
| mergeUsers: data.mergeUsers as boolean, | ||
| mergeUsersDistinctServices: data.mergeUsersDistinctServices as boolean, | ||
| mergeRoles: data.mergeRoles as boolean, | ||
| rolesToSync: data.rolesToSync as string, | ||
| accessTokenParam: 'access_token', | ||
| showButton: data.showButton as boolean, | ||
| buttonLabelText: data.buttonLabelText as string, | ||
| buttonLabelColor: data.buttonLabelColor as string, | ||
| buttonColor: data.buttonColor as string, | ||
| }); |
There was a problem hiding this comment.
Add type guards or validation before type assertions.
The code performs numerous type assertions (as string, as boolean, etc.) on settings values without validation. If a setting returns an unexpected type or undefined, this could cause runtime errors or incorrect configuration.
Consider adding a helper to safely cast settings values:
function getSettingString(key: string, defaultValue = ''): string {
const value = settings.get(key);
return typeof value === 'string' ? value : defaultValue;
}
function getSettingBoolean(key: string, defaultValue = false): boolean {
const value = settings.get(key);
return typeof value === 'boolean' ? value : defaultValue;
}
// Then use:
const data = {
clientId: getSettingString(`${key}-id`),
secret: getSettingString(`${key}-secret`),
serverURL: getSettingString(`${key}-url`),
useDiscovery: getSettingBoolean(`${key}-use_discovery`, true),
// ... etc
};2 participants
Implement native OpenID Connect authentication method
Add automatic endpoint discovery via .well-known/openid-configuration
Support ID token validation and claims extraction
Enable Single Logout (SLO) for centralized authentication
Add compatibility with major identity providers (Keycloak, Microsoft Entra ID, Okta)
Create comprehensive documentation and configuration examples
Implement server and client-side logout methods
Add support for configurable claim mappings and role extraction
Ensure backward compatibility with existing OAuth configurations
Resolves #37489 by providing a robust, standards-compliant OpenID Connect implementation that simplifies enterprise authentication and improves security. Full support for openid #37489
Summary by CodeRabbit
Release Notes
New Features
Documentation