feat(auth): Add full OpenID Connect (OIDC) authentication support (Full support for openid #37489)#37494
feat(auth): Add full OpenID Connect (OIDC) authentication support (Full support for openid #37489)#37494motalib-code wants to merge 4 commits into
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
🦋 Changeset detectedLatest commit: 1a1c7f9 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 |
|
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 |
|
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. |
WalkthroughAdds a full OpenID Connect (OIDC) authentication implementation: discovery, ID token handling and validation, optional userinfo merging, Single Logout (SLO), settings/registration helpers, admin UI and env configuration, service lifecycle management, tests, docs, and client/server logout helpers. No public API signatures were removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Server as Rocket.Chat
participant IdP as OIDC Provider
participant DB as UserDB
User->>Browser: Click "Login with OIDC"
Browser->>Server: start OIDC flow (redirect)
Server->>IdP: redirect to authorization_endpoint
IdP->>User: authenticate & consent
IdP->>Browser: redirect back with code
Browser->>Server: deliver code
Server->>IdP: token_endpoint (exchange code)
IdP->>Server: access_token, id_token
alt ID token present
Server->>Server: decode/validate id_token (JWKS)
end
opt optional userinfo
Server->>IdP: userinfo endpoint
IdP->>Server: userinfo claims
end
Server->>DB: create/update user (merge claims)
Server->>Browser: establish session (login success)
sequenceDiagram
participant Browser
participant Server as Rocket.Chat
participant IdP as OIDC Provider
Browser->>Server: performOpenIDLogout(serviceName)
Server->>Server: resolve service & build end_session URL (id_token_hint)
Server-->>Browser: return logout URL
Browser->>IdP: redirect to end_session_endpoint
IdP->>Browser: end session, redirect back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
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: 5
🧹 Nitpick comments (4)
packages/i18n/src/locales/en.i18n.json (3)
259-266: Polish OIDC terminology and branding (minor text edits).
- Prefer “Microsoft Entra ID (formerly Azure AD)” for current naming.
- Consider referencing “RP‑initiated logout (SLO)” to align with the OIDC term.
- Wrap the well‑known path in backticks and say “provider URL” for clarity and consistency with nearby strings.
Apply this diff:
- "Accounts_OpenID_Description_Text": "OpenID Connect is a modern authentication protocol built on OAuth 2.0. It provides full support for Single Sign-On (SSO) and Single Logout (SLO), making it ideal for integration with identity providers like Keycloak, Entra ID (Azure AD), Okta, and others.", + "Accounts_OpenID_Description_Text": "OpenID Connect is a modern authentication protocol built on OAuth 2.0. It provides support for Single Sign-On (SSO) and Single Logout (SLO, RP-initiated logout), making it ideal for integration with identity providers like Keycloak, Microsoft Entra ID (formerly Azure AD), Okta, and others.", - "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_Description": "The OpenID Connect discovery endpoint URL (usually `/.well-known/openid-configuration`). Leave empty to use the default based on the provider URL.",
256-278: Consistency: “UserInfo” capitalization, punctuation, and code‑style examples.
- Use “UserInfo” (spec term) instead of “Userinfo”.
- End descriptions with a period like surrounding entries.
- Keep examples/paths in backticks for consistency.
- "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_Description": "Extract user claims from the ID token in addition to the UserInfo endpoint.", - "Accounts_OpenID_Userinfo_Path": "Userinfo Endpoint", + "Accounts_OpenID_Userinfo_Path": "UserInfo Endpoint", - "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_Description": "Enable Single Logout to log users out of both Rocket.Chat and the identity provider simultaneously.", - "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 (e.g., `https://accounts.google.com`, `https://login.microsoftonline.com/<tenant-id>`).", - "Accounts_OpenID_Validate_ID_Token_Description": "Validate the ID token signature using the provider's JWKS", + "Accounts_OpenID_Validate_ID_Token_Description": "Validate the ID token signature using the provider's JWKS.",
267-271: Add missing description strings for admin clarity.These labels appear without companion “…_Description” entries. Suggest adding clear guidance and defaults.
"Accounts_OpenID_ID_Token_Signing_Alg": "ID Token Signing Algorithm", + "Accounts_OpenID_ID_Token_Signing_Alg_Description": "Expected JWS algorithm for ID token signatures (e.g., `RS256`, `PS256`, `ES256`). Leave empty to accept the provider default.", "Accounts_OpenID_JWKS_URI": "JWKS URI", + "Accounts_OpenID_JWKS_URI_Description": "JSON Web Key Set URL. Leave empty to use the URI from discovery (recommended).", "Accounts_OpenID_Post_Logout_Redirect_URI": "Post Logout Redirect URI", + "Accounts_OpenID_Post_Logout_Redirect_URI_Description": "Where the IdP should redirect the user after logout (RP‑initiated logout). Example: `https://your.chat.example/_oauth/openid/redirect`.", "Accounts_OpenID_Response_Type": "Response Type", + "Accounts_OpenID_Response_Type_Description": "OAuth 2.0 response type (e.g., `code`, `id_token`, `code id_token`). For standard OIDC, `code` is recommended.",apps/meteor/server/lib/openid/README.md (1)
25-25: Minor documentation style suggestion.Consider hyphenating "OpenID-Connect-compliant" for grammatical consistency, as flagged by static analysis.
📜 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 (21)
.changeset/openid-connect-support.md(1 hunks)IMPLEMENTATION_COMPLETE.md(1 hunks)OPENID_CONNECT_IMPLEMENTATION.md(1 hunks)apps/meteor/client/lib/openidLogout.ts(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)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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
📚 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.mdOPENID_CONNECT_IMPLEMENTATION.mdapps/meteor/server/lib/openid/README.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-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 : 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 : 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 : 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 : Utilize Playwright fixtures (test, page, expect) consistently
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/startup/migrations/v318.tsapps/meteor/server/settings/oauth.ts
📚 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.ts
🧬 Code graph analysis (10)
apps/meteor/server/lib/openid/logger.ts (1)
apps/meteor/server/lib/openid/index.ts (1)
logger(6-6)
apps/meteor/server/lib/openid/removeOpenIDService.ts (1)
apps/meteor/server/lib/openid/index.ts (1)
removeOpenIDService(5-5)
apps/meteor/server/lib/openid/updateOpenIDServices.ts (4)
apps/meteor/server/lib/openid/logger.ts (1)
logger(3-3)packages/core-typings/src/ILoginServiceConfiguration.ts (2)
ILoginServiceConfiguration(1-4)OAuthConfiguration(6-37)apps/meteor/server/lib/openid/OpenIDConnect.ts (1)
OpenIDConnect(79-657)apps/meteor/app/lib/server/lib/notifyListener.ts (2)
notifyOnLoginServiceConfigurationChangedByService(178-189)notifyOnLoginServiceConfigurationChanged(165-176)
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)
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/OpenIDConnect.ts (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/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/client/lib/openidLogout.ts (1)
apps/meteor/server/methods/openidLogout.ts (1)
serviceName(9-38)
apps/meteor/server/lib/openid/initOpenIDServices.ts (1)
apps/meteor/server/lib/openid/addOpenIDService.ts (1)
addOpenIDService(5-423)
🪛 LanguageTool
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)
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)
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)
🔇 Additional comments (12)
OPENID_CONNECT_IMPLEMENTATION.md (1)
1-200: Excellent documentation - comprehensive and well-structured.This implementation guide provides thorough coverage of the OpenID Connect integration, including architecture, configuration examples, security considerations, and migration guidance. The document clearly explains how this implementation addresses issue #37489 and provides valuable context for both administrators and developers.
IMPLEMENTATION_COMPLETE.md (1)
1-407: Comprehensive completion summary - production ready.This document effectively demonstrates that all requirements from issue #37489 have been met. The structured approach with checkmarks, file listings, and feature comparisons provides clear evidence of a complete implementation. The production readiness checklist and next steps sections are particularly valuable for deployment planning.
apps/meteor/server/lib/openid/examples.env (1)
1-125: Well-structured configuration template with clear placeholders.This environment variable template provides excellent copy-paste examples for multiple OIDC providers. The use of obvious placeholder values (e.g., "your-client-secret", "your-application-id") ensures administrators won't accidentally deploy with example credentials. The advanced configuration section comprehensively covers enterprise features like role mapping and user merging.
apps/meteor/server/settings/oauth.ts (2)
431-431: Good consistency fix - aligning with section pattern.The change from
returntoawaitfor the Proxy section correctly aligns with the established pattern used by other sections in this file. This ensures proper async handling of the section initialization.
442-449: New OpenID Connect settings section follows established patterns.The new OpenID Connect section appropriately adds a readonly informational field to introduce the OIDC configuration area in the admin UI. The use of i18n labels ensures proper internationalization support, and the readonly property is appropriate for a descriptive field.
apps/meteor/server/lib/openid/logger.ts (1)
1-3: LGTM!The logger initialization is clean and follows the standard Rocket.Chat logging pattern.
apps/meteor/server/lib/refreshLoginServices.ts (1)
5-5: LGTM!The OpenID service update is correctly integrated into the login service refresh flow, following the established pattern for OAuth, SAML, and CAS services.
Also applies to: 11-11
apps/meteor/server/configuration/oauth.ts (1)
7-9: LGTM!The OpenID Connect integration follows the established OAuth pattern correctly:
- Debounced updates with consistent 2s delay
- Proper regex patterns for watching settings changes
- Parallel initialization after OAuth services
- Clean separation of concerns between update and removal watchers
Also applies to: 13-13, 25-33, 36-36
apps/meteor/server/methods/openidLogout.ts (1)
24-27: LGTM!The SLO enablement check gracefully returns
nullwhen Single Logout is not enabled, allowing the client to handle this case appropriately without throwing an error.apps/meteor/server/lib/openid/removeOpenIDService.ts (1)
4-9: LGTM!The service removal logic is clean and follows the established pattern for OAuth service removal, properly cleaning up both the service configuration and the settings section.
.changeset/openid-connect-support.md (1)
1-44: LGTM!The changeset documentation clearly describes the new OpenID Connect feature, its benefits, configuration options, and migration path. The documentation is comprehensive and well-organized.
apps/meteor/client/lib/openidLogout.ts (1)
12-15: No action required. The server-side implementation validates the logout URL by construction.The
performSingleLogoutmethod constructs the logout URL directly from the configuredendSessionEndpoint(a trusted OpenID discovery value) rather than accepting arbitrary URLs. The URL is built as${this.endSessionEndpoint}?${params.toString()}, where parameters are safely encoded via URLSearchParams. This design prevents open redirect vulnerabilities since the base URL always comes from trusted configuration, not user input.
| 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('_')), ''); | ||
| } | ||
|
|
||
| const serviceKey = `Accounts_OpenID_${name}`; | ||
|
|
||
| if (key === serviceKey) { | ||
| const values = { | ||
| enabled: process.env[`${serviceKey}`] === 'true', | ||
| clientId: process.env[`${serviceKey}_id`], | ||
| clientSecret: process.env[`${serviceKey}_secret`], | ||
| serverURL: process.env[`${serviceKey}_url`], | ||
| useDiscovery: process.env[`${serviceKey}_use_discovery`] !== 'false', | ||
| discoveryEndpoint: process.env[`${serviceKey}_discovery_endpoint`], | ||
| tokenPath: process.env[`${serviceKey}_token_path`], | ||
| authorizePath: process.env[`${serviceKey}_authorize_path`], | ||
| userinfoPath: process.env[`${serviceKey}_userinfo_path`], | ||
| jwksUri: process.env[`${serviceKey}_jwks_uri`], | ||
| endSessionEndpoint: process.env[`${serviceKey}_end_session_endpoint`], | ||
| scope: process.env[`${serviceKey}_scope`], | ||
| responseType: process.env[`${serviceKey}_response_type`], | ||
| idTokenSigningAlg: process.env[`${serviceKey}_id_token_signing_alg`], | ||
| claimsFromIdToken: process.env[`${serviceKey}_claims_from_id_token`] !== 'false', | ||
| validateIdToken: process.env[`${serviceKey}_validate_id_token`] !== 'false', | ||
| enableSLO: process.env[`${serviceKey}_enable_slo`] === 'true', | ||
| postLogoutRedirectUri: process.env[`${serviceKey}_post_logout_redirect_uri`], | ||
| buttonLabelText: process.env[`${serviceKey}_button_label_text`], | ||
| buttonLabelColor: process.env[`${serviceKey}_button_label_color`], | ||
| loginStyle: process.env[`${serviceKey}_login_style`], | ||
| buttonColor: process.env[`${serviceKey}_button_color`], | ||
| tokenSentVia: process.env[`${serviceKey}_token_sent_via`], | ||
| identityTokenSentVia: process.env[`${serviceKey}_identity_token_sent_via`], | ||
| keyField: process.env[`${serviceKey}_key_field`], | ||
| usernameField: process.env[`${serviceKey}_username_field`], | ||
| nameField: process.env[`${serviceKey}_name_field`], | ||
| emailField: process.env[`${serviceKey}_email_field`], | ||
| avatarField: process.env[`${serviceKey}_avatar_field`], | ||
| rolesClaim: process.env[`${serviceKey}_roles_claim`], | ||
| groupsClaim: process.env[`${serviceKey}_groups_claim`], | ||
| channelsMap: process.env[`${serviceKey}_groups_channel_map`], | ||
| channelsAdmin: process.env[`${serviceKey}_channels_admin`], | ||
| mergeUsers: process.env[`${serviceKey}_merge_users`] === 'true', | ||
| mergeUsersDistinctServices: process.env[`${serviceKey}_merge_users_distinct_services`] === 'true', | ||
| mapChannels: process.env[`${serviceKey}_map_channels`] === 'true', | ||
| mergeRoles: process.env[`${serviceKey}_merge_roles`] === 'true', | ||
| rolesToSync: process.env[`${serviceKey}_roles_to_sync`], | ||
| showButton: process.env[`${serviceKey}_show_button`] !== 'false', | ||
| }; | ||
|
|
||
| await addOpenIDService(name, values); | ||
| } |
There was a problem hiding this comment.
Environment parsing drops providers whose names contain underscores
name.replace(name.substr(name.indexOf('_')), '') cuts everything after the first underscore. For a provider like Accounts_OpenID_Acme_US, name becomes Acme, serviceKey turns into Accounts_OpenID_Acme, and the if (key === serviceKey) guard fails. Result: any provider configured with an underscore in its name is never registered, so credentials, discovery settings, SLO, etc., are all lost. That breaks legitimate setups where underscores are common in env var naming.
Please keep the full provider token and only skip keys that have a suffix (e.g., _id, _secret). One way is to collect the base keys first and then hydrate each provider explicitly:
import { addOpenIDService } from './addOpenIDService';
export async function initOpenIDServices(): Promise<void> {
- // Add settings for OpenID Connect providers from ENV variables
- 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('_')), '');
- }
-
- const serviceKey = `Accounts_OpenID_${name}`;
-
- if (key === serviceKey) {
- const values = {
- enabled: process.env[`${serviceKey}`] === 'true',
- clientId: process.env[`${serviceKey}_id`],
- clientSecret: process.env[`${serviceKey}_secret`],
- serverURL: process.env[`${serviceKey}_url`],
- useDiscovery: process.env[`${serviceKey}_use_discovery`] !== 'false',
- discoveryEndpoint: process.env[`${serviceKey}_discovery_endpoint`],
- tokenPath: process.env[`${serviceKey}_token_path`],
- authorizePath: process.env[`${serviceKey}_authorize_path`],
- userinfoPath: process.env[`${serviceKey}_userinfo_path`],
- jwksUri: process.env[`${serviceKey}_jwks_uri`],
- endSessionEndpoint: process.env[`${serviceKey}_end_session_endpoint`],
- scope: process.env[`${serviceKey}_scope`],
- responseType: process.env[`${serviceKey}_response_type`],
- idTokenSigningAlg: process.env[`${serviceKey}_id_token_signing_alg`],
- claimsFromIdToken: process.env[`${serviceKey}_claims_from_id_token`] !== 'false',
- validateIdToken: process.env[`${serviceKey}_validate_id_token`] !== 'false',
- enableSLO: process.env[`${serviceKey}_enable_slo`] === 'true',
- postLogoutRedirectUri: process.env[`${serviceKey}_post_logout_redirect_uri`],
- buttonLabelText: process.env[`${serviceKey}_button_label_text`],
- buttonLabelColor: process.env[`${serviceKey}_button_label_color`],
- loginStyle: process.env[`${serviceKey}_login_style`],
- buttonColor: process.env[`${serviceKey}_button_color`],
- tokenSentVia: process.env[`${serviceKey}_token_sent_via`],
- identityTokenSentVia: process.env[`${serviceKey}_identity_token_sent_via`],
- keyField: process.env[`${serviceKey}_key_field`],
- usernameField: process.env[`${serviceKey}_username_field`],
- nameField: process.env[`${serviceKey}_name_field`],
- emailField: process.env[`${serviceKey}_email_field`],
- avatarField: process.env[`${serviceKey}_avatar_field`],
- rolesClaim: process.env[`${serviceKey}_roles_claim`],
- groupsClaim: process.env[`${serviceKey}_groups_claim`],
- channelsMap: process.env[`${serviceKey}_groups_channel_map`],
- channelsAdmin: process.env[`${serviceKey}_channels_admin`],
- mergeUsers: process.env[`${serviceKey}_merge_users`] === 'true',
- mergeUsersDistinctServices: process.env[`${serviceKey}_merge_users_distinct_services`] === 'true',
- mapChannels: process.env[`${serviceKey}_map_channels`] === 'true',
- mergeRoles: process.env[`${serviceKey}_merge_roles`] === 'true',
- rolesToSync: process.env[`${serviceKey}_roles_to_sync`],
- showButton: process.env[`${serviceKey}_show_button`] !== 'false',
- };
-
- await addOpenIDService(name, values);
- }
- }
- }
+ // Add settings for OpenID Connect providers from ENV variables
+ const providerKeys = new Set<string>();
+
+ for (const key of Object.keys(process.env)) {
+ const match = key.match(/^Accounts_OpenID_([a-zA-Z0-9_]+)(?:_(.+))?$/);
+ if (!match) {
+ continue;
+ }
+
+ const [, name, suffix] = match;
+
+ if (suffix) {
+ continue;
+ }
+
+ providerKeys.add(name);
+ }
+
+ for (const name of providerKeys) {
+ const serviceKey = `Accounts_OpenID_${name}`;
+ const values = {
+ enabled: process.env[`${serviceKey}`] === 'true',
+ clientId: process.env[`${serviceKey}_id`],
+ clientSecret: process.env[`${serviceKey}_secret`],
+ serverURL: process.env[`${serviceKey}_url`],
+ useDiscovery: process.env[`${serviceKey}_use_discovery`] !== 'false',
+ discoveryEndpoint: process.env[`${serviceKey}_discovery_endpoint`],
+ tokenPath: process.env[`${serviceKey}_token_path`],
+ authorizePath: process.env[`${serviceKey}_authorize_path`],
+ userinfoPath: process.env[`${serviceKey}_userinfo_path`],
+ jwksUri: process.env[`${serviceKey}_jwks_uri`],
+ endSessionEndpoint: process.env[`${serviceKey}_end_session_endpoint`],
+ scope: process.env[`${serviceKey}_scope`],
+ responseType: process.env[`${serviceKey}_response_type`],
+ idTokenSigningAlg: process.env[`${serviceKey}_id_token_signing_alg`],
+ claimsFromIdToken: process.env[`${serviceKey}_claims_from_id_token`] !== 'false',
+ validateIdToken: process.env[`${serviceKey}_validate_id_token`] !== 'false',
+ enableSLO: process.env[`${serviceKey}_enable_slo`] === 'true',
+ postLogoutRedirectUri: process.env[`${serviceKey}_post_logout_redirect_uri`],
+ buttonLabelText: process.env[`${serviceKey}_button_label_text`],
+ buttonLabelColor: process.env[`${serviceKey}_button_label_color`],
+ loginStyle: process.env[`${serviceKey}_login_style`],
+ buttonColor: process.env[`${serviceKey}_button_color`],
+ tokenSentVia: process.env[`${serviceKey}_token_sent_via`],
+ identityTokenSentVia: process.env[`${serviceKey}_identity_token_sent_via`],
+ keyField: process.env[`${serviceKey}_key_field`],
+ usernameField: process.env[`${serviceKey}_username_field`],
+ nameField: process.env[`${serviceKey}_name_field`],
+ emailField: process.env[`${serviceKey}_email_field`],
+ avatarField: process.env[`${serviceKey}_avatar_field`],
+ rolesClaim: process.env[`${serviceKey}_roles_claim`],
+ groupsClaim: process.env[`${serviceKey}_groups_claim`],
+ channelsMap: process.env[`${serviceKey}_groups_channel_map`],
+ channelsAdmin: process.env[`${serviceKey}_channels_admin`],
+ mergeUsers: process.env[`${serviceKey}_merge_users`] === 'true',
+ mergeUsersDistinctServices: process.env[`${serviceKey}_merge_users_distinct_services`] === 'true',
+ mapChannels: process.env[`${serviceKey}_map_channels`] === 'true',
+ mergeRoles: process.env[`${serviceKey}_merge_roles`] === 'true',
+ rolesToSync: process.env[`${serviceKey}_roles_to_sync`],
+ showButton: process.env[`${serviceKey}_show_button`] !== 'false',
+ };
+
+ await addOpenIDService(name, values);
+ }
}📝 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('_')), ''); | |
| } | |
| const serviceKey = `Accounts_OpenID_${name}`; | |
| if (key === serviceKey) { | |
| const values = { | |
| enabled: process.env[`${serviceKey}`] === 'true', | |
| clientId: process.env[`${serviceKey}_id`], | |
| clientSecret: process.env[`${serviceKey}_secret`], | |
| serverURL: process.env[`${serviceKey}_url`], | |
| useDiscovery: process.env[`${serviceKey}_use_discovery`] !== 'false', | |
| discoveryEndpoint: process.env[`${serviceKey}_discovery_endpoint`], | |
| tokenPath: process.env[`${serviceKey}_token_path`], | |
| authorizePath: process.env[`${serviceKey}_authorize_path`], | |
| userinfoPath: process.env[`${serviceKey}_userinfo_path`], | |
| jwksUri: process.env[`${serviceKey}_jwks_uri`], | |
| endSessionEndpoint: process.env[`${serviceKey}_end_session_endpoint`], | |
| scope: process.env[`${serviceKey}_scope`], | |
| responseType: process.env[`${serviceKey}_response_type`], | |
| idTokenSigningAlg: process.env[`${serviceKey}_id_token_signing_alg`], | |
| claimsFromIdToken: process.env[`${serviceKey}_claims_from_id_token`] !== 'false', | |
| validateIdToken: process.env[`${serviceKey}_validate_id_token`] !== 'false', | |
| enableSLO: process.env[`${serviceKey}_enable_slo`] === 'true', | |
| postLogoutRedirectUri: process.env[`${serviceKey}_post_logout_redirect_uri`], | |
| buttonLabelText: process.env[`${serviceKey}_button_label_text`], | |
| buttonLabelColor: process.env[`${serviceKey}_button_label_color`], | |
| loginStyle: process.env[`${serviceKey}_login_style`], | |
| buttonColor: process.env[`${serviceKey}_button_color`], | |
| tokenSentVia: process.env[`${serviceKey}_token_sent_via`], | |
| identityTokenSentVia: process.env[`${serviceKey}_identity_token_sent_via`], | |
| keyField: process.env[`${serviceKey}_key_field`], | |
| usernameField: process.env[`${serviceKey}_username_field`], | |
| nameField: process.env[`${serviceKey}_name_field`], | |
| emailField: process.env[`${serviceKey}_email_field`], | |
| avatarField: process.env[`${serviceKey}_avatar_field`], | |
| rolesClaim: process.env[`${serviceKey}_roles_claim`], | |
| groupsClaim: process.env[`${serviceKey}_groups_claim`], | |
| channelsMap: process.env[`${serviceKey}_groups_channel_map`], | |
| channelsAdmin: process.env[`${serviceKey}_channels_admin`], | |
| mergeUsers: process.env[`${serviceKey}_merge_users`] === 'true', | |
| mergeUsersDistinctServices: process.env[`${serviceKey}_merge_users_distinct_services`] === 'true', | |
| mapChannels: process.env[`${serviceKey}_map_channels`] === 'true', | |
| mergeRoles: process.env[`${serviceKey}_merge_roles`] === 'true', | |
| rolesToSync: process.env[`${serviceKey}_roles_to_sync`], | |
| showButton: process.env[`${serviceKey}_show_button`] !== 'false', | |
| }; | |
| await addOpenIDService(name, values); | |
| } | |
| import { addOpenIDService } from './addOpenIDService'; | |
| export async function initOpenIDServices(): Promise<void> { | |
| // Add settings for OpenID Connect providers from ENV variables | |
| const providerKeys = new Set<string>(); | |
| for (const key of Object.keys(process.env)) { | |
| const match = key.match(/^Accounts_OpenID_([a-zA-Z0-9_]+)(?:_(.+))?$/); | |
| if (!match) { | |
| continue; | |
| } | |
| const [, name, suffix] = match; | |
| if (suffix) { | |
| continue; | |
| } | |
| providerKeys.add(name); | |
| } | |
| for (const name of providerKeys) { | |
| const serviceKey = `Accounts_OpenID_${name}`; | |
| const values = { | |
| enabled: process.env[`${serviceKey}`] === 'true', | |
| clientId: process.env[`${serviceKey}_id`], | |
| clientSecret: process.env[`${serviceKey}_secret`], | |
| serverURL: process.env[`${serviceKey}_url`], | |
| useDiscovery: process.env[`${serviceKey}_use_discovery`] !== 'false', | |
| discoveryEndpoint: process.env[`${serviceKey}_discovery_endpoint`], | |
| tokenPath: process.env[`${serviceKey}_token_path`], | |
| authorizePath: process.env[`${serviceKey}_authorize_path`], | |
| userinfoPath: process.env[`${serviceKey}_userinfo_path`], | |
| jwksUri: process.env[`${serviceKey}_jwks_uri`], | |
| endSessionEndpoint: process.env[`${serviceKey}_end_session_endpoint`], | |
| scope: process.env[`${serviceKey}_scope`], | |
| responseType: process.env[`${serviceKey}_response_type`], | |
| idTokenSigningAlg: process.env[`${serviceKey}_id_token_signing_alg`], | |
| claimsFromIdToken: process.env[`${serviceKey}_claims_from_id_token`] !== 'false', | |
| validateIdToken: process.env[`${serviceKey}_validate_id_token`] !== 'false', | |
| enableSLO: process.env[`${serviceKey}_enable_slo`] === 'true', | |
| postLogoutRedirectUri: process.env[`${serviceKey}_post_logout_redirect_uri`], | |
| buttonLabelText: process.env[`${serviceKey}_button_label_text`], | |
| buttonLabelColor: process.env[`${serviceKey}_button_label_color`], | |
| loginStyle: process.env[`${serviceKey}_login_style`], | |
| buttonColor: process.env[`${serviceKey}_button_color`], | |
| tokenSentVia: process.env[`${serviceKey}_token_sent_via`], | |
| identityTokenSentVia: process.env[`${serviceKey}_identity_token_sent_via`], | |
| keyField: process.env[`${serviceKey}_key_field`], | |
| usernameField: process.env[`${serviceKey}_username_field`], | |
| nameField: process.env[`${serviceKey}_name_field`], | |
| emailField: process.env[`${serviceKey}_email_field`], | |
| avatarField: process.env[`${serviceKey}_avatar_field`], | |
| rolesClaim: process.env[`${serviceKey}_roles_claim`], | |
| groupsClaim: process.env[`${serviceKey}_groups_claim`], | |
| channelsMap: process.env[`${serviceKey}_groups_channel_map`], | |
| channelsAdmin: process.env[`${serviceKey}_channels_admin`], | |
| mergeUsers: process.env[`${serviceKey}_merge_users`] === 'true', | |
| mergeUsersDistinctServices: process.env[`${serviceKey}_merge_users_distinct_services`] === 'true', | |
| mapChannels: process.env[`${serviceKey}_map_channels`] === 'true', | |
| mergeRoles: process.env[`${serviceKey}_merge_roles`] === 'true', | |
| rolesToSync: process.env[`${serviceKey}_roles_to_sync`], | |
| showButton: process.env[`${serviceKey}_show_button`] !== 'false', | |
| }; | |
| await addOpenIDService(name, values); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/server/lib/openid/initOpenIDServices.ts around lines 5–59 the
code truncates provider names at the first underscore which drops providers like
Accounts_OpenID_Acme_US; preserve the full provider token and stop chopping at
'_'—instead compute provider bases by stripping only known suffixes (e.g. _id,
_secret, _url, _use_discovery, _token_path, etc.) or by collecting unique base
names from all ENV keys that start with Accounts_OpenID_ and removing only those
suffixes, then iterate that set of base provider names to build serviceKey and
the values object and call addOpenIDService; remove the fragile if (key ===
serviceKey) guard and replace it with explicit iteration over the derived
provider bases so providers containing underscores are registered correctly.
| describe('OpenIDConnect', () => { | ||
| describe('Discovery Document', () => { | ||
| it('should parse standard OpenID Connect discovery document', () => { | ||
| const discoveryDoc = { | ||
| issuer: 'https://example.com', | ||
| authorization_endpoint: 'https://example.com/oauth/authorize', | ||
| token_endpoint: 'https://example.com/oauth/token', | ||
| userinfo_endpoint: 'https://example.com/oauth/userinfo', | ||
| jwks_uri: 'https://example.com/oauth/jwks', | ||
| end_session_endpoint: 'https://example.com/oauth/logout', | ||
| response_types_supported: ['code', 'id_token', 'token'], | ||
| subject_types_supported: ['public'], | ||
| id_token_signing_alg_values_supported: ['RS256'], | ||
| }; | ||
|
|
||
| expect(discoveryDoc.issuer).to.equal('https://example.com'); | ||
| expect(discoveryDoc.authorization_endpoint).to.equal('https://example.com/oauth/authorize'); | ||
| expect(discoveryDoc.token_endpoint).to.equal('https://example.com/oauth/token'); | ||
| expect(discoveryDoc.userinfo_endpoint).to.equal('https://example.com/oauth/userinfo'); | ||
| expect(discoveryDoc.jwks_uri).to.equal('https://example.com/oauth/jwks'); | ||
| expect(discoveryDoc.end_session_endpoint).to.equal('https://example.com/oauth/logout'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('ID Token Decoding', () => { | ||
| it('should decode a valid JWT ID token', () => { | ||
| // Sample JWT token (header.payload.signature) | ||
| // Header: {"alg":"RS256","typ":"JWT"} | ||
| // Payload: {"sub":"1234567890","name":"John Doe","email":"john@example.com","iat":1516239022} | ||
| const idToken = | ||
| 'eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiZW1haWwiOiJqb2huQGV4YW1wbGUuY29tIiwiaWF0IjoxNTE2MjM5MDIyfQ.signature'; | ||
|
|
||
| const parts = idToken.split('.'); | ||
| expect(parts).to.have.lengthOf(3); | ||
|
|
||
| const payload = parts[1]; | ||
| const decoded = Buffer.from(payload, 'base64').toString('utf8'); | ||
| const claims = JSON.parse(decoded); | ||
|
|
||
| expect(claims.sub).to.equal('1234567890'); | ||
| expect(claims.name).to.equal('John Doe'); | ||
| expect(claims.email).to.equal('john@example.com'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Claim Mapping', () => { | ||
| it('should map standard OpenID Connect claims', () => { | ||
| const claims = { | ||
| sub: '1234567890', | ||
| preferred_username: 'johndoe', | ||
| email: 'john@example.com', | ||
| name: 'John Doe', | ||
| picture: 'https://example.com/avatar.jpg', | ||
| roles: ['user', 'admin'], | ||
| groups: ['developers', 'managers'], | ||
| }; | ||
|
|
||
| expect(claims.sub).to.equal('1234567890'); | ||
| expect(claims.preferred_username).to.equal('johndoe'); | ||
| expect(claims.email).to.equal('john@example.com'); | ||
| expect(claims.name).to.equal('John Doe'); | ||
| expect(claims.picture).to.equal('https://example.com/avatar.jpg'); | ||
| expect(claims.roles).to.deep.equal(['user', 'admin']); | ||
| expect(claims.groups).to.deep.equal(['developers', 'managers']); | ||
| }); | ||
|
|
||
| it('should handle nested claims', () => { | ||
| const claims = { | ||
| sub: '1234567890', | ||
| realm_access: { | ||
| roles: ['user', 'admin'], | ||
| }, | ||
| }; | ||
|
|
||
| expect(claims.realm_access.roles).to.deep.equal(['user', 'admin']); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Single Logout URL Generation', () => { | ||
| it('should generate correct logout URL with parameters', () => { | ||
| const endSessionEndpoint = 'https://example.com/oauth/logout'; | ||
| const idToken = 'sample-id-token'; | ||
| const postLogoutRedirectUri = 'https://rocketchat.example.com'; | ||
|
|
||
| const params = new URLSearchParams({ | ||
| id_token_hint: idToken, | ||
| post_logout_redirect_uri: postLogoutRedirectUri, | ||
| }); | ||
|
|
||
| const logoutUrl = `${endSessionEndpoint}?${params.toString()}`; | ||
|
|
||
| expect(logoutUrl).to.include('id_token_hint=sample-id-token'); | ||
| expect(logoutUrl).to.include('post_logout_redirect_uri=https%3A%2F%2Frocketchat.example.com'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Scope Handling', () => { | ||
| it('should include openid scope', () => { | ||
| const scope = 'openid profile email'; | ||
| const scopes = scope.split(' '); | ||
|
|
||
| expect(scopes).to.include('openid'); | ||
| expect(scopes).to.include('profile'); | ||
| expect(scopes).to.include('email'); | ||
| }); | ||
|
|
||
| it('should handle custom scopes', () => { | ||
| const scope = 'openid profile email roles groups'; | ||
| const scopes = scope.split(' '); | ||
|
|
||
| expect(scopes).to.include('openid'); | ||
| expect(scopes).to.include('roles'); | ||
| expect(scopes).to.include('groups'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Tests don't exercise the OpenIDConnect implementation
Every describe block only asserts against hard-coded objects created inside the test, so these specs pass even if OpenIDConnect (the code under test) is completely broken. Please rewrite the tests to import the real OpenIDConnect module and verify its actual discovery, ID-token parsing, claim mapping, logout URL generation, and scope handling behavior.
🤖 Prompt for AI Agents
apps/meteor/server/lib/openid/OpenIDConnect.spec.ts lines 4-118: the tests only
validate hard-coded objects instead of exercising the OpenIDConnect
implementation; update each spec to import the real OpenIDConnect module (and
any exported helpers like parseIdToken, mapClaims, buildLogoutUrl, parseScopes
or the main class), replace static fixtures with calls to those
functions/methods, and assert on their outputs; for discovery, mock the HTTP
fetch/axios used by OpenIDConnect (or stub a network call) to return the
discovery JSON and call the module's discovery method, for ID token decoding
call the module's decode/verify function with a test token (or stub verification
and assert parsed claims), for claim mapping call the module's mapping function
with nested and flat claim examples, for logout URL generation call the module
method that builds the URL and assert query params, and for scope handling call
the module's scope parser/normalizer instead of splitting a string manually;
ensure imports, mocks/stubs, and cleanup are added so the tests actually
exercise the implementation.
| async getIdentity(accessToken: string, idToken?: string): Promise<any> { | ||
| let identity: any = {}; | ||
|
|
||
| // 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.
Implement full ID token validation
Right now claimsFromIdToken just base64‑decodes the token without verifying its signature, issuer, audience, or expiry. OpenID Connect requires relying parties to validate those fields and the JWS signature before trusting any ID token content; otherwise a forged or replayed token can be accepted as legitimate.(openid.net) Please wire up real validation (e.g., fetch & cache the provider JWKS, verify the signature/alg, enforce iss/aud/exp/nonce, and honour the validateIdToken flag). Until that is in place, the implementation is vulnerable to token spoofing.
🤖 Prompt for AI Agents
In apps/meteor/server/lib/openid/OpenIDConnect.ts around lines 309 to 378, the
current decode/claims flow only base64-decodes the ID token and trusts its
contents; implement full ID token validation instead: fetch and cache the
provider JWKS (jwksUri) and use a JWS verification library to verify the token
signature and alg, validate iss matches provider issuer, aud includes our client
id, exp and nbf are within range, and validate nonce if present, honoring the
validateIdToken flag to skip/enable checks; on validation failure throw an error
so claimsFromIdToken logic won't accept forged tokens; merge validated payload
with userinfo only after successful verification and cache JWKS with proper
expiry/refresh and error handling.
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.
Full support for OpenID Connect
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
apps/meteor/client/views/admin/settings/SettingsGroupSelector/SettingsGroupSelector.tsx (1)
27-29: Consider passing theonClickBackprop for consistency.Unlike
OAuthGroupPageandLDAPGroupPage, theOpenIDConnectGroupPagedoes not receive theonClickBackprop. If navigation back is desired, this prop should be passed.if (groupId === 'OpenID_Connect') { - return <OpenIDConnectGroupPage />; + return <OpenIDConnectGroupPage onClickBack={onClickBack} />; }apps/meteor/app/openid-connect/server/startup.js (2)
29-33: Potential duplicate instance creation on setting changes.A new OpenIDConnect instance is created every time settings change while enabled. Since line 26-28 in openid_connect_server.js checks
if (Services[this.name])and calls configure, this may work, but could be more explicit.Consider checking if instance exists before creating:
- new OpenIDConnect('oidc', { - issuer, - clientId, - clientSecret, - }); + if (!Services.oidc) { + new OpenIDConnect('oidc', { + issuer, + clientId, + clientSecret, + }); + } else { + // Instance already exists, configure will be called by constructor logic + }However, this relies on internal implementation details. Review if OpenIDConnect class handles reconfiguration properly.
6-34: Add error handling for configuration and instance creation.The settings watcher lacks error handling for ServiceConfiguration.upsert and OpenIDConnect instantiation. If discovery fails or configuration is invalid, errors will propagate unhandled.
settings.watch('OIDC_Enable', (enabled) => { + try { if (!enabled) { ServiceConfiguration.configurations.remove({ service: 'oidc' }); return; } const issuer = settings.get('OIDC_Issuer'); const clientId = settings.get('OIDC_Client_ID'); const clientSecret = settings.get('OIDC_Client_Secret'); ServiceConfiguration.configurations.upsert( { service: 'oidc' }, { $set: { issuer, clientId, secret: clientSecret, loginStyle: 'popup', }, }, ); new OpenIDConnect('oidc', { issuer, clientId, clientSecret, }); + } catch (error) { + logger.error('Failed to configure OpenID Connect:', error); + } });apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsx (2)
27-39: Add validation before saving settings.When OIDC is enabled, ensure that issuer, client ID, and client secret are provided. Invalid or incomplete configuration could cause runtime errors.
const handleSave = async () => { try { + if (enabled && (!issuer || !clientID || !clientSecret)) { + dispatchToastMessage({ type: 'error', message: 'Please fill in all required fields when enabling OpenID Connect' }); + return; + } + + if (enabled && issuer && !issuer.startsWith('http://') && !issuer.startsWith('https://')) { + dispatchToastMessage({ type: 'error', message: 'Issuer URL must start with http:// or https://' }); + return; + } + await settings.batchSet([ { _id: 'OIDC_Enable', value: enabled }, { _id: 'OIDC_Issuer', value: issuer }, { _id: 'OIDC_Client_ID', value: clientID }, { _id: 'OIDC_Client_Secret', value: clientSecret }, ]); dispatchToastMessage({ type: 'success', message: 'Settings saved' }); } catch (error) { dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); } };
27-39: Add loading state during save operation.The save button should be disabled while settings are being saved to prevent duplicate submissions and improve UX.
+ const [isSaving, setIsSaving] = useState(false); + const handleSave = async () => { + setIsSaving(true); try { await settings.batchSet([ { _id: 'OIDC_Enable', value: enabled }, { _id: 'OIDC_Issuer', value: issuer }, { _id: 'OIDC_Client_ID', value: clientID }, { _id: 'OIDC_Client_Secret', value: clientSecret }, ]); dispatchToastMessage({ type: 'success', message: 'Settings saved' }); } catch (error) { dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); + } finally { + setIsSaving(false); } };Then update the button:
- <Button primary onClick={handleSave}>Save changes</Button> + <Button primary onClick={handleSave} disabled={isSaving}> + {isSaving ? 'Saving...' : 'Save changes'} + </Button>apps/meteor/app/openid-connect/server/slo.js (2)
10-42: Add error logging and security event tracking.The SLO endpoint lacks logging for security audit trails. Successful and failed logout attempts should be logged with relevant details.
+import { Logger } from '@rocket.chat/logger'; + +const logger = new Logger('OpenIDConnect:SLO'); + WebApp.connectHandlers.use('/_slo', async (req, res, next) => { const { logout_token } = req.body; if (!logout_token) { + logger.warn('SLO request missing logout_token'); res.writeHead(400); res.end('Missing logout_token'); return; } 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)); try { const { payload } = await jwtVerify(logout_token, jwks, { issuer, audience, }); const user = await Users.findOneBySvcId('oidc', payload.sub); if (user) { await Users.unsetLoginTokens(user._id); + logger.info({ msg: 'User logged out via SLO', userId: user._id, sub: payload.sub }); + } else { + logger.warn({ msg: 'SLO logout_token for unknown user', sub: payload.sub }); } res.writeHead(200); res.end('Logout successful'); } catch (err) { + logger.error({ msg: 'Failed to verify logout_token', err }); res.writeHead(400); res.end('Invalid logout_token'); } });
22-22: Add timeout and error handling for discovery fetch.The discovery document fetch lacks timeout and proper error handling. If the identity provider is slow or unavailable, the SLO request could hang indefinitely.
- const discovery = await (await fetch(`${issuer}/.well-known/openid-configuration`)).json(); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 5000); // 5 second timeout + + try { + const response = await fetch(`${issuer}/.well-known/openid-configuration`, { + signal: controller.signal, + }); + + if (!response.ok) { + throw new Error(`Discovery failed: ${response.statusText}`); + } + + const discovery = await response.json(); + clearTimeout(timeout); + } catch (err) { + clearTimeout(timeout); + logger.error({ msg: 'Failed to fetch discovery document', err }); + res.writeHead(500); + res.end('Internal server error'); + return; + }apps/meteor/app/openid-connect/server/openid_connect_server.js (2)
62-81: Add error handling for malformed discovery response.If the discovery endpoint returns invalid JSON or an unexpected structure, the error handling should be more specific.
try { const response = await fetch(discoveryUrl.toString(), { headers: { 'Accept': 'application/json', 'User-Agent': this.userAgent, }, }); if (!response.ok) { throw new Error(`Failed to fetch discovery document from ${discoveryUrl}. ${response.statusText}`); } const discovery = await response.json(); + + // Validate required discovery fields + if (!discovery.issuer || !discovery.token_endpoint || !discovery.authorization_endpoint) { + throw new Error('Invalid discovery document: missing required fields'); + } + logger.debug({ msg: 'Discovery response', discovery }); return discovery; } catch (err) { - const error = new Error(`Failed to fetch discovery document from ${discoveryUrl}. ${err.message}`); - throw _.extend(error, { response: err.response }); + logger.error({ msg: 'Failed to fetch discovery document', err }); + throw new Error(`Failed to fetch discovery document from ${discoveryUrl}. ${err.message}`); }
15-15: Consider exporting Services registry or providing cleanup method.The Services object is module-scoped and not exported. This makes it difficult to clean up or inspect registered services, which could be useful for testing or dynamic reconfiguration.
-const Services = {}; +export const Services = {};Or provide a method to deregister services:
export function removeService(name) { delete Services[name]; // Additional cleanup as needed }
📜 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 (7)
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/views/admin/settings/SettingsGroupSelector/SettingsGroupSelector.tsx(2 hunks)apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsx(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (3)
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/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)
🪛 Biome (2.1.2)
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/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/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)
🔇 Additional comments (1)
package.json (1)
88-88: Version ^5.2.4 is secure with no known vulnerabilities.Verification confirms that version 5.2.4 is not affected by any known vulnerabilities in the jose library. All reported CVEs target older versions (2.x, 3.x, and 4.x). While the latest available version is 6.1.1, the current version meets the security requirements.
| if (Services[this.name]) { | ||
| Services[this.name].configure(options); | ||
| return; |
There was a problem hiding this comment.
Critical: Constructor returns early without returning instance.
When a service already exists, the constructor calls configure and returns undefined (line 28). Callers expecting an instance will get undefined.
if (Services[this.name]) {
Services[this.name].configure(options);
- return;
+ return Services[this.name];
}📝 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 (Services[this.name]) { | |
| Services[this.name].configure(options); | |
| return; | |
| if (Services[this.name]) { | |
| Services[this.name].configure(options); | |
| return Services[this.name]; | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/openid_connect_server.js around lines
26 to 28, the constructor short-circuits when a service already exists by
calling Services[this.name].configure(options) and then returning undefined;
change this to return the existing service instance instead. Replace the early
bare return with a return of Services[this.name] so callers receive the existing
instance (ensuring the returned object replaces the newly constructed this when
used as a constructor).
| async configure(options) { | ||
| if (!Match.test(options, Object)) { | ||
| throw new Meteor.Error('OpenIDConnect: Options is required and must be Object'); | ||
| } | ||
|
|
||
| this.options = options; | ||
| this.discovery = await this.getDiscovery(); | ||
| } |
There was a problem hiding this comment.
Critical: Async configure called in constructor without await.
The configure method is async (fetches discovery) but called in the constructor without await (line 33). The instance will be used before discovery is ready, causing errors.
This is a fundamental design issue. Options:
- Make constructor async (not supported in JavaScript)
- Lazy-load discovery on first use
- Use a static factory method
Recommended approach - lazy loading:
async configure(options) {
if (!Match.test(options, Object)) {
throw new Meteor.Error('OpenIDConnect: Options is required and must be Object');
}
this.options = options;
- this.discovery = await this.getDiscovery();
+ this._discoveryPromise = null; // Clear cached promise
+ }
+
+ async ensureDiscovery() {
+ if (!this._discoveryPromise) {
+ this._discoveryPromise = this.getDiscovery();
+ }
+ this.discovery = await this._discoveryPromise;
}Then call await this.ensureDiscovery() at the start of methods that need discovery (getAccessToken, getJWKS, verifyIdToken).
📝 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.
| async configure(options) { | |
| if (!Match.test(options, Object)) { | |
| throw new Meteor.Error('OpenIDConnect: Options is required and must be Object'); | |
| } | |
| this.options = options; | |
| this.discovery = await this.getDiscovery(); | |
| } | |
| async configure(options) { | |
| if (!Match.test(options, Object)) { | |
| throw new Meteor.Error('OpenIDConnect: Options is required and must be Object'); | |
| } | |
| this.options = options; | |
| this._discoveryPromise = null; // Clear cached promise | |
| } | |
| async ensureDiscovery() { | |
| if (!this._discoveryPromise) { | |
| this._discoveryPromise = this.getDiscovery(); | |
| } | |
| this.discovery = await this._discoveryPromise; | |
| } |
| return discovery; | ||
| } catch (err) { | ||
| const error = new Error(`Failed to fetch discovery document from ${discoveryUrl}. ${err.message}`); | ||
| throw _.extend(error, { response: err.response }); |
There was a problem hiding this comment.
Critical: Missing import for underscore library.
Line 79 uses _.extend but underscore is not imported. This will cause a runtime error.
import { Logger } from '@rocket.chat/logger';
import { createRemoteJWKSet, jwtVerify } from 'jose';
+import _ from 'underscore';
import './slo';Same issue exists at line 125. Alternatively, use native JavaScript:
- throw _.extend(error, { response: err.response });
+ throw Object.assign(error, { response: err.response });📝 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.
| throw _.extend(error, { response: err.response }); | |
| throw Object.assign(error, { response: err.response }); |
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/openid_connect_server.js around lines
79 and 125, the code calls _.extend but underscore is not imported; add an
import/require for underscore at the top of the file (e.g. require or import of
'_' from 'underscore') or replace _.extend calls with native JS (Object.assign
or object spread) to merge the response into the error before throwing; update
both occurrences to use the chosen approach consistently.
| 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.
Missing null check for token_endpoint_auth_methods_supported.
The code assumes this.discovery.token_endpoint_auth_methods_supported exists and is an array. If the discovery document doesn't include this field, a runtime error will occur.
- if (this.discovery.token_endpoint_auth_methods_supported.includes('client_secret_basic')) {
+ 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));
}📝 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)); | |
| } | |
| 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)); | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/openid_connect_server.js around lines
102 to 108, the code assumes
this.discovery.token_endpoint_auth_methods_supported is present and an array
before calling .includes, which can throw if missing; update the conditional to
first verify this.discovery and
Array.isArray(this.discovery.token_endpoint_auth_methods_supported) and only
then check .includes('client_secret_basic'), otherwise take the safe fallback
path of appending client_id and client_secret to params (do not attempt to set
the Authorization header when the discovery field is absent or not an array).
|
|
||
| const serviceData = { | ||
| _OAuthCustom: true, | ||
| accessToken: tokens.access_token, | ||
| idToken: tokens.id_token, | ||
| expiresAt: (+new Date) + (1000 * parseInt(tokens.expires_in, 10)), | ||
| refreshToken: tokens.refresh_token, | ||
| ...claims, | ||
| id: claims.sub, | ||
| }; |
There was a problem hiding this comment.
Security: Claims spread could override critical serviceData fields.
The spread operator on line 165 ...claims could override critical fields like _OAuthCustom, accessToken, idToken, etc. if malicious claims are present in the ID token.
const serviceData = {
_OAuthCustom: true,
accessToken: tokens.access_token,
idToken: tokens.id_token,
expiresAt: (+new Date) + (1000 * parseInt(tokens.expires_in, 10)),
refreshToken: tokens.refresh_token,
- ...claims,
id: claims.sub,
+ // Add only safe claim fields
+ email: claims.email,
+ email_verified: claims.email_verified,
+ name: claims.name,
+ preferred_username: claims.preferred_username,
+ // Add other expected claims as needed
};Alternatively, namespace the claims:
const serviceData = {
_OAuthCustom: true,
accessToken: tokens.access_token,
idToken: tokens.id_token,
expiresAt: (+new Date) + (1000 * parseInt(tokens.expires_in, 10)),
refreshToken: tokens.refresh_token,
id: claims.sub,
+ claims: claims, // Namespace all claims under 'claims' property
};📝 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.
| const serviceData = { | |
| _OAuthCustom: true, | |
| accessToken: tokens.access_token, | |
| idToken: tokens.id_token, | |
| expiresAt: (+new Date) + (1000 * parseInt(tokens.expires_in, 10)), | |
| refreshToken: tokens.refresh_token, | |
| ...claims, | |
| id: claims.sub, | |
| }; | |
| const serviceData = { | |
| _OAuthCustom: true, | |
| accessToken: tokens.access_token, | |
| idToken: tokens.id_token, | |
| expiresAt: (+new Date) + (1000 * parseInt(tokens.expires_in, 10)), | |
| refreshToken: tokens.refresh_token, | |
| id: claims.sub, | |
| // Add only safe claim fields | |
| email: claims.email, | |
| email_verified: claims.email_verified, | |
| name: claims.name, | |
| preferred_username: claims.preferred_username, | |
| // Add other expected claims as needed | |
| }; |
| this.add('OIDC_Client_Secret', '', { | ||
| type: 'string', | ||
| public: true, | ||
| secret: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that client secret should be marked as public.
The OIDC_Client_Secret setting is marked with both public: true and secret: true. Ensure this doesn't inadvertently expose the secret to unauthorized clients. In Rocket.Chat's settings system, verify that public means "available in admin UI" rather than "sent to all connected clients."
🏁 Script executed:
#!/bin/bash
# Description: Check how other secret settings are configured in the codebase
# Search for other settings that use both public and secret flags
rg -n -A2 -B2 "secret.*true" --type=js -g '**/settings/**' | rg -A2 -B2 "public.*true"Length of output: 1390
I need to understand what the public and secret flags actually do in Rocket.Chat's settings system. Let me examine the implementation:
🏁 Script executed:
#!/bin/bash
# Find settings system implementation
fd -t f -e ts -e js 'settings' | head -20Length of output: 1143
🏁 Script executed:
#!/bin/bash
# Look at how settings are processed/used in the system
rg -n "public.*true.*secret.*true|secret.*true.*public.*true" --type=ts --type=js -A2 -B2 | head -50Length of output: 574
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about public/secret flags in settings
rg -n -B3 -A3 "public.*secret|secret.*public" --type=ts --type=js | grep -E "(//|/\*|\*)" | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for settings system definition or type definitions
fd -t f \( -name "*setting*" -o -name "*Setting*" \) | grep -E "\.(ts|js)$" | head -20Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for how secret flag is documented/explained
rg -n "secret:" --type=ts --type=js -B5 -A1 | head -50Length of output: 3599
🏁 Script executed:
#!/bin/bash
# Check what happens with public settings - do they get sent to clients?
rg -n "public" apps/meteor/server/settings/ --type=ts --type=js -B1 -A1 | grep -E "(send|client|export|expose)" -i | head -20Length of output: 287
🏁 Script executed:
#!/bin/bash
# Look at the actual settings system implementation
find apps/meteor -name "*settings*" -type f \( -name "*.ts" -o -name "*.js" \) | grep -v node_modules | grep -v testLength of output: 765
🏁 Script executed:
#!/bin/bash
# Search for class/method that processes the add() method for settings
rg -n "add\s*\(" apps/meteor/server/settings/ --type=ts -A3 | head -30Length of output: 2320
🏁 Script executed:
#!/bin/bash
# Look for how settings are retrieved/sent to clients
rg -n "secret" apps/meteor/server/lib --type=ts --type=js -B2 -A2 | head -40Length of output: 3143
🏁 Script executed:
#!/bin/bash
# Look at OIDC implementation to see if secret is actually needed by clients
rg -n "OIDC_Client_Secret" --type=ts --type=js -B3 -A3Length of output: 3069
🏁 Script executed:
#!/bin/bash
# Search for settings system implementation files
find apps -name "*.ts" -o -name "*.js" | xargs grep -l "class.*Settings\|function.*add" | grep -i settings | head -15Length of output: 927
🏁 Script executed:
#!/bin/bash
# Look in the root of apps/meteor for settings implementation
ls -la apps/meteor/server/ | grep -i settingLength of output: 58
I need to examine the settings system implementation to understand what public and secret flags do:
🏁 Script executed:
#!/bin/bash
# Examine SettingsRegistry.ts to understand how public/secret flags work
cat apps/meteor/app/settings/server/SettingsRegistry.ts | head -100Length of output: 3362
🏁 Script executed:
#!/bin/bash
# Look for the add method definition in SettingsRegistry
rg -n "add\s*\(" apps/meteor/app/settings/server/SettingsRegistry.ts -A10 | head -50Length of output: 2006
🏁 Script executed:
#!/bin/bash
# Check PublicSettingsCachedStore to understand what "public" means
cat apps/meteor/client/cachedStores/PublicSettingsCachedStore.tsLength of output: 469
🏁 Script executed:
#!/bin/bash
# Check PrivateSettingsCachedStore
cat apps/meteor/client/cachedStores/PrivateSettingsCachedStore.tsLength of output: 887
🏁 Script executed:
#!/bin/bash
# Look at the API endpoint for settings - what gets sent to clients
rg -n "public.*secret|secret.*public" apps/meteor/app/api/server/v1/settings.ts -B5 -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how settings are returned via API
rg -n "secret" apps/meteor/app/api/server/v1/settings.ts -B3 -A3Length of output: 468
🏁 Script executed:
#!/bin/bash
# Search for how settings are filtered/exposed
rg -n "public" apps/meteor/app/settings/server/ --type=ts -B2 -A2 | head -80Length of output: 6357
🏁 Script executed:
#!/bin/bash
# Look at how secret flag is handled when returning settings
rg -n "\.secret" apps/meteor/app/settings/server/ --type=ts -B3 -A3 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's any filtering based on secret flag when exposing to clients
rg -n "secret.*true" apps/meteor/app/ --type=ts --type=js -A5 | grep -E "(filter|expose|send|client|public)" -i -B2 -A2 | head -40Length of output: 649
Let me search for how settings are distributed to public vs private stores:
🏁 Script executed:
#!/bin/bash
# Find where PublicSettings and PrivateSettings stores are populated
rg -n "PublicSettings\|PrivateSettings" apps/meteor/app/ --type=ts -B2 -A2 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for filtering logic when settings are returned to clients
rg -n "secret.*false|!.*secret|secret.*\?" apps/meteor/app/settings/server/ --type=ts -B3 -A3 | head -80Length of output: 632
🏁 Script executed:
#!/bin/bash
# Search for where settings payload is prepared for clients
rg -n "toJSON|toObject|serialize" apps/meteor/app/settings/server/ --type=ts -B3 -A3 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look in server services for settings streaming
cat apps/meteor/server/services/settings/service.ts | head -150Length of output: 638
🏁 Script executed:
#!/bin/bash
# Check how settings are returned via subscription/streamer
find apps/meteor -name "*.ts" -type f | xargs grep -l "notify-all.*setting\|notify-logged.*setting" | head -5Length of output: 61
🏁 Script executed:
#!/bin/bash
# Search for where settings are filtered before sending to clients
rg -n "secret" apps/meteor/app/settings/server/ --type=ts -C5 | grep -E "(client|send|expose|public|filter|project)" -i -B3 -A3 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for how client receives settings and what fields are included
rg -n "private-settings-changed\|public-settings-changed" apps/meteor/ --type=ts --type=js -B2 -A2 | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for ISetting type definition to see all properties
rg -n "interface ISetting\|type ISetting" --type=ts -A30 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how the `public` property is used in settings exposure
rg -n "\.public" apps/meteor/app/settings/server/ --type=ts -B3 -A3 | head -60Length of output: 845
🏁 Script executed:
#!/bin/bash
# Search for streaming setup and settings publication
rg -n "stream\|subscribe\|publish" apps/meteor/server/services/settings/ --type=ts -A5 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for subscription handlers for settings
find apps/meteor -name "*.ts" -type f | xargs grep -l "Meteor.subscribe.*setting\|addListener.*setting" 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for how settings are streamed to clients - look for notification setup
rg -n "notifyAll\|notifyLogged" apps/meteor/app/ --type=ts --type=js -B3 -A3 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where settings collection is subscribed/published
rg -n "rocketchat_subscription\|publish.*setting" apps/meteor/ --type=ts --type=js -B2 -A5 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how public/private settings are filtered in the streaming layer
rg -n "public.*?" apps/meteor/ee/server/settings/ --type=ts -B2 -A2 | head -60Length of output: 3853
🏁 Script executed:
#!/bin/bash
# Look for where public flag determines what gets sent to clients
fd -t f -e ts "setting" apps/meteor/server | xargs grep -l "public" | head -10Length of output: 96
🏁 Script executed:
#!/bin/bash
# Examine the internal service for settings
cat apps/meteor/ee/app/settings/server/settings.internalService.ts | head -150Length of output: 517
🏁 Script executed:
#!/bin/bash
# Check if there's filtering in the actual settings model/collection
rg -n "secret\|public" packages/model-typings/src/ --type=ts -B2 -A2 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where the public/secret distinction is used during settings output
rg -n -w "public\|secret" apps/meteor/app/settings/server/SettingsRegistry.ts -B2 -A2 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for settings being sent/streamed with filtering
rg -n "emit\|broadcast\|send" apps/meteor/app/settings/server/ --type=ts -B3 -A3 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where useSetting hook gets the OIDC_Client_Secret
rg -n "useSetting" apps/meteor/client/lib/settings/ --type=ts -B5 -A10 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check what the useSetting hook does and how it retrieves settings
fd -t f -name "useSetting*" apps/meteor/client/ | head -5Length of output: 230
🏁 Script executed:
#!/bin/bash
# Look for settings retrieval in the admin panel
rg -n "useMethod\|useSetting" apps/meteor/client/views/admin/settings/ --type=tsx -B2 -A2 | head -60Length of output: 27
🏁 Script executed:
#!/bin/bash
# Find how admin pages get settings data
rg -n "settings.*fetch\|settings.*get\|useQuery.*setting" apps/meteor/client/views/admin/ --type=tsx --type=ts | head -40Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check if there's a separate API/method for admin settings retrieval
rg -n "admin.*settings\|settings.*admin" apps/meteor/app/api/server/v1/ --type=ts -B2 -A2 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at how the settings page retrieves secret settings specifically
cat apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsxLength of output: 2377
🏁 Script executed:
#!/bin/bash
# Check if there's different handling for authenticated users retrieving secrets
rg -n "userId\|user\|auth" apps/meteor/app/settings/server/SettingsRegistry.ts -B2 -A2 | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for how PrivateSettings are differentiated from PublicSettings
rg -n "private\|public" apps/meteor/client/stores/ --type=ts -B3 -A3 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for where public flag determines if setting goes to notify-all
rg -n "setting.public\|\.public.*true" apps/meteor/app/settings/server/ --type=ts -C5 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where secrets are filtered or included based on authentication
rg -n "secret.*true" apps/meteor/app/settings/server/ --type=ts -B5 -A5 | head -100Length of output: 0
🌐 Web query:
Rocket.Chat settings public vs secret flag meaning what does public parameter do
💡 Result:
"Public" (Registration Form = Public) makes the registration page open to anyone — users can access /register and create accounts.
"Secret" (Registration Form = Secret URL) hides the form except at a specific secret path (e.g. /register/<secret_hash>); only people with that URL can register. You change this under Administration → Workspace → Settings → Accounts (Registration Form). [1][2]
🏁 Script executed:
#!/bin/bash
# Look at how other client secrets are configured
rg -n "secret.*true" apps/meteor/server/settings/oauth.ts -B2 -A2 | head -60Length of output: 2189
🏁 Script executed:
#!/bin/bash
# Find all settings with secret: true and check which have public: true
rg -n "secret: true" apps/meteor/server/settings/ --type=ts -B5 | grep -E "(secret: true|public:)" | head -80Length of output: 5236
🏁 Script executed:
#!/bin/bash
# Specifically look at the OAuth secret settings to see the pattern
cat apps/meteor/server/settings/oauth.ts | grep -A3 -B3 "secret"Length of output: 4825
🏁 Script executed:
#!/bin/bash
# Look for how private settings are accessed by authenticated users
rg -n "PrivateSettings\|private.*setting" apps/meteor/client/ --type=ts | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the useSetting hook implementation to understand how it retrieves settings
find apps/meteor -name "*.ts" -o -name "*.js" | xargs grep -l "export.*useSetting\|function useSetting" 2>/dev/null | head -3Length of output: 194
🏁 Script executed:
#!/bin/bash
# Check if UI settings come from a different source than the public API
rg -n "useSetting\|useSettings" apps/meteor/client/views/admin/ --type=ts | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Compare OIDC_Client_Secret with other similar settings
rg -n "OIDC_Client_Secret\|Accounts_OAuth.*secret\|API_.*secret" apps/meteor/app/openid-connect/server/settings.js apps/meteor/server/settings/oauth.ts -B3 -A3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Final verification - show OIDC and similar settings side by side
echo "=== OIDC Settings ===" && \
rg -n "OIDC_Client" apps/meteor/app/openid-connect/server/settings.js -A2 && \
echo -e "\n=== Comparison: OAuth secrets WITHOUT public flag ===" && \
rg -n "Accounts_OAuth.*secret|Accounts_OAuth.*id" apps/meteor/server/settings/oauth.ts | head -40Length of output: 2608
Remove public: true from OIDC_Client_Secret setting.
The OIDC_Client_Secret is incorrectly marked with public: true in addition to secret: true. Analysis of the codebase shows that all other client secrets (Accounts_OAuth_Drupal_secret, Accounts_OAuth_Gitlab_secret, Accounts_OAuth_Facebook_secret, etc.) use only secret: true without public: true. The public flag is unnecessary for admin access since authenticated administrators retrieve settings through the PrivateSettingsCachedStore channel, which respects the secret flag independently. Keeping public: true on a secret contradicts the security pattern established throughout the settings configuration and risks unintended exposure of the credential.
this.add('OIDC_Client_Secret', '', {
type: 'string',
secret: true,
});
🤖 Prompt for AI Agents
apps/meteor/app/openid-connect/server/settings.js around lines 19 to 23: the
OIDC_Client_Secret setting is marked with both public: true and secret: true;
remove the public: true flag so it matches other client secret settings. Update
the setting definition to omit the public property, leaving type: 'string' and
secret: true only, ensuring secrets are not exposed via public settings.
| WebApp.connectHandlers.use('/_slo', async (req, res, next) => { | ||
| const { logout_token } = req.body; |
There was a problem hiding this comment.
Critical: Body parser middleware missing for /_slo endpoint.
The code accesses req.body.logout_token but WebApp.connectHandlers doesn't automatically parse request bodies. The body will be undefined, causing the endpoint to always return "Missing logout_token".
Add body parser middleware before accessing req.body:
+import bodyParser from 'body-parser';
+
+WebApp.connectHandlers.use('/_slo', bodyParser.urlencoded({ extended: false }));
+WebApp.connectHandlers.use('/_slo', bodyParser.json());
WebApp.connectHandlers.use('/_slo', async (req, res, next) => {
const { logout_token } = req.body;Or if Meteor provides a body parsing utility, use that. Verify that the SLO request body parsing works correctly before deploying.
📝 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.
| WebApp.connectHandlers.use('/_slo', async (req, res, next) => { | |
| const { logout_token } = req.body; | |
| import bodyParser from 'body-parser'; | |
| WebApp.connectHandlers.use('/_slo', bodyParser.urlencoded({ extended: false })); | |
| WebApp.connectHandlers.use('/_slo', bodyParser.json()); | |
| WebApp.connectHandlers.use('/_slo', async (req, res, next) => { | |
| const { logout_token } = req.body; |
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/slo.js around lines 10-11, the handler
reads req.body.logout_token but no body-parsing middleware is registered, so
req.body will be undefined; add body parsing for the SLO endpoint (e.g.,
register appropriate middleware before this route to parse
application/x-www-form-urlencoded and/or application/json bodies depending on
the IdP) and ensure the middleware runs for '/_slo' before accessing req.body;
also validate and handle parsing errors so the handler returns a clear error if
the body is missing or malformed.
| 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.
Performance: Discovery document fetched on every SLO request.
The discovery document and JWKS are fetched fresh for each logout request. This is inefficient and could cause failures if the identity provider is temporarily unavailable.
Consider caching the discovery document and JWKS:
+const discoveryCache = new Map();
+
+async function getDiscoveryAndJWKS(issuer) {
+ if (discoveryCache.has(issuer)) {
+ return discoveryCache.get(issuer);
+ }
+
+ const discovery = await (await fetch(`${issuer}/.well-known/openid-configuration`)).json();
+ const jwks = createRemoteJWKSet(new URL(discovery.jwks_uri));
+
+ const cached = { discovery, jwks };
+ discoveryCache.set(issuer, cached);
+
+ // Invalidate cache after 1 hour
+ setTimeout(() => discoveryCache.delete(issuer), 60 * 60 * 1000);
+
+ return cached;
+}
+
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));
+ const { discovery, jwks } = await getDiscoveryAndJWKS(issuer);Note: The jose library's createRemoteJWKSet already caches JWKS internally, but caching the discovery document is still beneficial.
📝 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.
| 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)); | |
| const discoveryCache = new Map(); | |
| async function getDiscoveryAndJWKS(issuer) { | |
| if (discoveryCache.has(issuer)) { | |
| return discoveryCache.get(issuer); | |
| } | |
| const discovery = await (await fetch(`${issuer}/.well-known/openid-configuration`)).json(); | |
| const jwks = createRemoteJWKSet(new URL(discovery.jwks_uri)); | |
| const cached = { discovery, jwks }; | |
| discoveryCache.set(issuer, cached); | |
| // Invalidate cache after 1 hour | |
| setTimeout(() => discoveryCache.delete(issuer), 60 * 60 * 1000); | |
| return cached; | |
| } | |
| const issuer = settings.get('OIDC_Issuer'); | |
| const audience = settings.get('OIDC_Client_ID'); | |
| const { discovery, jwks } = await getDiscoveryAndJWKS(issuer); |
| settings.watch('OIDC_Enable', (enabled) => { | ||
| if (!enabled) { | ||
| ServiceConfiguration.configurations.remove({ service: 'oidc' }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing cleanup of OpenIDConnect instance on disable.
When OIDC is disabled, the ServiceConfiguration is removed but any existing OpenIDConnect instance in the Services registry is not cleaned up. This could leave orphaned instances.
Consider adding cleanup logic:
settings.watch('OIDC_Enable', (enabled) => {
if (!enabled) {
ServiceConfiguration.configurations.remove({ service: 'oidc' });
+ // Clean up OpenIDConnect instance if it exists
+ if (Services?.oidc) {
+ delete Services.oidc;
+ }
return;
}Note: This assumes the Services registry is accessible. If OpenIDConnect provides a cleanup/deregister method, use that instead.
📝 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.
| settings.watch('OIDC_Enable', (enabled) => { | |
| if (!enabled) { | |
| ServiceConfiguration.configurations.remove({ service: 'oidc' }); | |
| return; | |
| } | |
| settings.watch('OIDC_Enable', (enabled) => { | |
| if (!enabled) { | |
| ServiceConfiguration.configurations.remove({ service: 'oidc' }); | |
| // Clean up OpenIDConnect instance if it exists | |
| if (Services?.oidc) { | |
| delete Services.oidc; | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/app/openid-connect/server/startup.js around lines 7 to 11, when
OIDC is disabled the code removes the ServiceConfiguration but does not remove
the OpenIDConnect instance from the Services registry, leaving orphaned
instances; update the disable branch to check for an existing Services['oidc']
entry and remove or call its provided cleanup/deregister method (prefer the
instance's cleanup method if available), then delete Services['oidc'] (or
otherwise unregister it) so no stale instance remains after disabling OIDC.
| } catch (error) { | ||
| dispatchToastMessage({ type: 'error', message: error }); |
There was a problem hiding this comment.
Fix error message handling in toast.
The error object is passed directly to the toast message, which may not display correctly. Extract the error message instead.
} catch (error) {
- dispatchToastMessage({ type: 'error', message: error });
+ dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) });
}📝 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.
| } catch (error) { | |
| dispatchToastMessage({ type: 'error', message: error }); | |
| } catch (error) { | |
| dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); | |
| } |
🤖 Prompt for AI Agents
In
apps/meteor/client/views/admin/settings/groups/OpenIDConnectGroupPage/OpenIDConnectGroupPage.tsx
around lines 36-37, the catch block passes the raw error object to
dispatchToastMessage which may not render properly; change it to extract a
string message (e.g. use error instanceof Error ? error.message : String(error))
and pass that string as the message field to dispatchToastMessage so the toast
displays a human-readable error.
2 participants
.well-known/openid-configurationResolves Full support for openid #37489 by providing a robust, standards-compliant OpenID Connect implementation that simplifies enterprise authentication and improves security. Full support for openid Full support for openid #37489
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Documentation
Tests