fix(web): Support multiple IDPs of the same type#1177
Open
brendan-kellam wants to merge 8 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThis PR refactors identity-provider handling to support multiple instances of the same type by replacing a single ChangesIdentity provider model and flow migration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
brendan-kellam
force-pushed
the
bkellam/multi-idp
branch
2 times, most recently
from
32010eb to
cc9dcc0
Compare
brendan-kellam
changed the title
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/backend/src/ee/tokenRefresh.ts (1)
154-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
providerIdwhen building the GitLab refresh callback.The refresh path still hardcodes
redirect_urito/api/auth/callback/gitlab. After this PR, secondary GitLab providers authorize against/api/auth/callback/<providerId>, and GitLab expects refreshes to reuse that same URI. Custom-id GitLab accounts will stop refreshing once their access token expires.Proposed fix
const refreshOAuthToken = async ( providerId: string, providerType: SupportedProviderType, refreshToken: string, ): Promise<OAuthTokenResponse | null> => { @@ - const result = await tryRefreshToken(providerType, refreshToken, envCredentials); + const result = await tryRefreshToken(providerId, providerType, refreshToken, envCredentials); @@ - const result = await tryRefreshToken(providerType, refreshToken, { clientId, clientSecret, baseUrl }); + const result = await tryRefreshToken(providerId, providerType, refreshToken, { clientId, clientSecret, baseUrl }); @@ const tryRefreshToken = async ( + providerId: string, providerType: SupportedProviderType, refreshToken: string, credentials: ProviderCredentials, ): Promise<OAuthTokenResponse | null> => { @@ if (providerType === 'gitlab') { - bodyParams.redirect_uri = new URL('/api/auth/callback/gitlab', env.AUTH_URL).toString(); + bodyParams.redirect_uri = new URL(`/api/auth/callback/${providerId}`, env.AUTH_URL).toString(); }Based on the PR objective and stack context that auth callbacks now use configured provider ids.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/src/ee/tokenRefresh.ts` around lines 154 - 257, The GitLab redirect_uri is hardcoded to "/api/auth/callback/gitlab" in tryRefreshToken; change tryRefreshToken to accept providerId (add parameter providerId: string) and update both call sites in refreshOAuthToken (the envCredentials branch and the idpConfig branch) to pass providerId into tryRefreshToken; then build redirect_uri using `/api/auth/callback/${providerId}` when providerType === 'gitlab' so secondary/custom-id GitLab providers reuse their provider-specific callback. Ensure the updated tryRefreshToken signature is applied wherever it’s called.packages/web/src/app/login/components/loginForm.tsx (1)
46-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnhandled provider types are being tracked as GitHub logins.
The default branch now records every unsupported
providerTypeaswa_login_with_github, so providers likeauthentik,bitbucket-cloud, andbitbucket-serverwill be misattributed in analytics. Please add explicit mappings for the supported OAuth types here, or fall back to a neutral/unknown event instead of GitHub.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/login/components/loginForm.tsx` around lines 46 - 65, The function getLoginEventName currently maps unknown providerType values to "wa_login_with_github", causing misattribution; update getLoginEventName to add explicit cases for additional supported providers (e.g., "authentik", "bitbucket-cloud", "bitbucket-server") mapping to distinct analytics event names, and change the default fallback to a neutral/unknown event (e.g., "wa_login_with_unknown") instead of GitHub so unsupported providers aren’t tracked as GitHub; locate and modify the switch inside getLoginEventName to add the new case labels and adjust the default return accordingly.
🧹 Nitpick comments (3)
packages/web/src/lib/encryptedPrismaAdapter.ts (2)
49-60: 💤 Low valueEnsure
linkAccounthandles async errors gracefully.
resolveProviderTypecan fail if config loading throws (e.g., disk error, malformed YAML). The current implementation does not wrap the call in try-catch, so an exception will propagate and fail the OAuth flow. Consider whether this is the desired behavior or if a fallback is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/encryptedPrismaAdapter.ts` around lines 49 - 60, Wrap the resolveProviderType(account.provider) call in a try/catch inside linkAccount so async failures don’t bubble out and break the OAuth flow; on error capture the original error, log it (e.g., console.error or your logger), set a safe fallback value for providerType (e.g., 'unknown' or null), then continue to call encryptAccountTokens(account) and prisma.account.create(...) as before; alternatively, if you prefer failing fast, rethrow a new Error that includes context and the original error to make the failure explicit.
28-35: Cache the config-derived provider mapping inresolveProviderType
packages/web/src/lib/encryptedPrismaAdapter.tscallsloadConfig(env.CONFIG_PATH)insideresolveProviderType, so everylinkAccounttriggers configfetch/readFile, JSON parse, and AJV validation.packages/shared/src/env.server.tsloadConfighas no module-scope memoization; it directly reads from the config path each time, so concurrent OAuth callbacks can cause repeated I/O/CPU.- Cache the normalized
identityProviders(or the resolved config) behind a module-scope promise (optionally with TTL/invalidation if the config can change at runtime).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/encryptedPrismaAdapter.ts` around lines 28 - 35, resolveProviderType currently calls loadConfig(env.CONFIG_PATH) on every invocation causing repeated I/O/parse/validation; introduce a module-scope cached promise or variable (e.g., cachedConfigPromise or cachedIdentityProviders) that loads and normalizes config.identityProviders once and is reused by resolveProviderType (falling back to providerId if mapping missing); ensure the cache is populated by calling loadConfig(env.CONFIG_PATH) once and storing either the full resolved config or a mapping, and optionally add a TTL/invalidate function if runtime config changes must be supported.packages/web/src/ee/features/sso/actions.ts (1)
33-35: 🏗️ Heavy liftMove
getLinkedAccountsout of the server-action module.This is still a read path inside a
'use server'actions file. Repo guidance reserves server actions for mutations, so this fetch should live in a plain server helper (or an API route if the client needs it), while this module stays focused on mutating actions.As per coding guidelines,
packages/web/src/**/*.ts: "Server actions should be used for mutations (POST/PUT/DELETE operations), not for data fetching."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/ee/features/sso/actions.ts` around lines 33 - 35, getLinkedAccounts is a read-only data fetch currently defined in the server-actions module; move it out into a plain server helper (or an API route) so 'use server' actions remain mutation-only. Create a new non-action module and export the function there, preserving the implementation that wraps sew/withAuth/withMinimumOrgRole (references: getLinkedAccounts, sew, withAuth, withMinimumOrgRole, OrgRole.MEMBER), then update any imports that consume getLinkedAccounts to point to the new helper and remove it from the actions file so the actions module contains only mutation functions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 53-54: Move the changelog entry "- Fixed issue where using
multiple identity providers of the same type (e.g., gitlab) would result in
unexpected behaviours.
[`#1177`](https://github.com/sourcebot-dev/sourcebot/pull/1177)" out of the
historical "4.17.2" release block and append it to the bottom of the
"[Unreleased]" section; ensure it remains under the "### Fixed" subsection
within that “[Unreleased]” header so the PR entry follows the repository's
changelog placement rules.
In `@packages/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 245-251: syncAccountPermissions currently throws when
config.identityProviders[account.providerId] is missing, breaking installs that
rely on legacy auth env vars; change the lookup for idpConfig in
syncAccountPermissions to fall back to the existing legacy method used by
ensureFreshAccountToken (reuse the same fallback logic/path) so that when
config.identityProviders is undefined or does not contain account.providerId you
use the legacy env-var-based IDP config instead of throwing; modify the
idpConfig resolution (the variable named idpConfig and its lookup of
config.identityProviders[account.providerId]) to attempt the config.json value
first and then the legacy fallback used elsewhere, preserving the thrown error
only if both are absent.
In `@packages/web/src/ee/features/sso/actions.ts`:
- Around line 77-88: The loop in the SSO action that iterates over
Object.entries(config.identityProviders ?? {}) is adding all unlinked providers
to the result; change it to only consider providers intended for account linking
by filtering where providerConfig.purpose === 'account_linking' before pushing
into result (i.e., restrict the iteration or add an if that skips
non-account_linking providers), keeping the existing fields (providerId,
providerType, displayName, isLinked, isAccountLinkingProvider, required,
supportsPermissionSync) and preserving the required =
providerConfig.accountLinkingRequired fallback logic and permission sync check
via doesIdpSupportPermissionSyncing.
---
Outside diff comments:
In `@packages/backend/src/ee/tokenRefresh.ts`:
- Around line 154-257: The GitLab redirect_uri is hardcoded to
"/api/auth/callback/gitlab" in tryRefreshToken; change tryRefreshToken to accept
providerId (add parameter providerId: string) and update both call sites in
refreshOAuthToken (the envCredentials branch and the idpConfig branch) to pass
providerId into tryRefreshToken; then build redirect_uri using
`/api/auth/callback/${providerId}` when providerType === 'gitlab' so
secondary/custom-id GitLab providers reuse their provider-specific callback.
Ensure the updated tryRefreshToken signature is applied wherever it’s called.
In `@packages/web/src/app/login/components/loginForm.tsx`:
- Around line 46-65: The function getLoginEventName currently maps unknown
providerType values to "wa_login_with_github", causing misattribution; update
getLoginEventName to add explicit cases for additional supported providers
(e.g., "authentik", "bitbucket-cloud", "bitbucket-server") mapping to distinct
analytics event names, and change the default fallback to a neutral/unknown
event (e.g., "wa_login_with_unknown") instead of GitHub so unsupported providers
aren’t tracked as GitHub; locate and modify the switch inside getLoginEventName
to add the new case labels and adjust the default return accordingly.
---
Nitpick comments:
In `@packages/web/src/ee/features/sso/actions.ts`:
- Around line 33-35: getLinkedAccounts is a read-only data fetch currently
defined in the server-actions module; move it out into a plain server helper (or
an API route) so 'use server' actions remain mutation-only. Create a new
non-action module and export the function there, preserving the implementation
that wraps sew/withAuth/withMinimumOrgRole (references: getLinkedAccounts, sew,
withAuth, withMinimumOrgRole, OrgRole.MEMBER), then update any imports that
consume getLinkedAccounts to point to the new helper and remove it from the
actions file so the actions module contains only mutation functions.
In `@packages/web/src/lib/encryptedPrismaAdapter.ts`:
- Around line 49-60: Wrap the resolveProviderType(account.provider) call in a
try/catch inside linkAccount so async failures don’t bubble out and break the
OAuth flow; on error capture the original error, log it (e.g., console.error or
your logger), set a safe fallback value for providerType (e.g., 'unknown' or
null), then continue to call encryptAccountTokens(account) and
prisma.account.create(...) as before; alternatively, if you prefer failing fast,
rethrow a new Error that includes context and the original error to make the
failure explicit.
- Around line 28-35: resolveProviderType currently calls
loadConfig(env.CONFIG_PATH) on every invocation causing repeated
I/O/parse/validation; introduce a module-scope cached promise or variable (e.g.,
cachedConfigPromise or cachedIdentityProviders) that loads and normalizes
config.identityProviders once and is reused by resolveProviderType (falling back
to providerId if mapping missing); ensure the cache is populated by calling
loadConfig(env.CONFIG_PATH) once and storing either the full resolved config or
a mapping, and optionally add a TTL/invalidate function if runtime config
changes must be supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ba93271-72e6-4824-972b-bbe591dcc18b
📒 Files selected for processing (30)
CHANGELOG.mddocs/snippets/schemas/v3/identityProvider.schema.mdxdocs/snippets/schemas/v3/index.schema.mdxpackages/backend/src/api.tspackages/backend/src/ee/accountPermissionSyncer.tspackages/backend/src/ee/repoPermissionSyncer.tspackages/backend/src/ee/tokenRefresh.tspackages/db/prisma/migrations/20260530203443_add_provider_id_and_type_to_account/migration.sqlpackages/db/prisma/schema.prismapackages/schemas/src/v3/identityProvider.schema.tspackages/schemas/src/v3/identityProvider.type.tspackages/schemas/src/v3/index.schema.tspackages/schemas/src/v3/index.type.tspackages/shared/src/constants.tspackages/shared/src/env.server.tspackages/shared/src/index.server.tspackages/web/src/app/(app)/settings/linked-accounts/page.tsxpackages/web/src/app/api/(server)/ee/permissionSyncStatus/api.tspackages/web/src/app/components/authMethodSelector.tsxpackages/web/src/app/login/components/loginForm.tsxpackages/web/src/auth.tspackages/web/src/ee/features/sso/actions.tspackages/web/src/ee/features/sso/components/connectAccountsCard.tsxpackages/web/src/ee/features/sso/components/linkedAccountProviderCard.tsxpackages/web/src/ee/features/sso/sso.tspackages/web/src/lib/encryptedPrismaAdapter.tspackages/web/src/lib/identityProviders.tspackages/web/src/lib/utils.tsschemas/v3/identityProvider.jsonschemas/v3/index.json
Comment on lines
+245
to
+251
| const idpConfig = config.identityProviders ? | ||
| config.identityProviders[account.providerId] : | ||
| undefined; | ||
|
|
||
| if (!idpConfig) { | ||
| throw new Error(`Unable to find IDP config in config.json.`); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep a legacy fallback when identityProviders[providerId] is missing.
syncAccountPermissions() now hard-fails unless the linked account resolves to config.identityProviders[account.providerId]. That regresses installs still using the deprecated auth env vars: ensureFreshAccountToken() can still refresh those accounts, but every permission-sync job will then die here with Unable to find IDP config in config.json. Please reuse the same legacy fallback here instead of requiring config.json outright.
Based on the PR objective's backward-compatibility requirement and the provided token-refresh fallback context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/ee/accountPermissionSyncer.ts` around lines 245 - 251,
syncAccountPermissions currently throws when
config.identityProviders[account.providerId] is missing, breaking installs that
rely on legacy auth env vars; change the lookup for idpConfig in
syncAccountPermissions to fall back to the existing legacy method used by
ensureFreshAccountToken (reuse the same fallback logic/path) so that when
config.identityProviders is undefined or does not contain account.providerId you
use the legacy env-var-based IDP config instead of throwing; modify the
idpConfig resolution (the variable named idpConfig and its lookup of
config.identityProviders[account.providerId]) to attempt the config.json value
first and then the legacy fallback used elsewhere, preserving the thrown error
only if both are absent.
Comment on lines
+77
to
88
| for (const [id, providerConfig] of Object.entries(config.identityProviders ?? {})) { | ||
| const account = accounts.find((account) => account.providerId === id); | ||
| if (!account) { | ||
| result.push({ | ||
| provider: providerConfig.provider, | ||
| providerId: id, | ||
| providerType: providerConfig.provider, | ||
| displayName: providerConfig.displayName, | ||
| isLinked: false, | ||
| isAccountLinkingProvider: providerConfig.purpose === 'account_linking', | ||
| required: providerConfig.purpose === 'account_linking' ? (providerConfig.accountLinkingRequired ?? false) : false, | ||
| supportsPermissionSync: permissionSyncEnabled && PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS.includes(providerConfig.provider), | ||
| supportsPermissionSync: permissionSyncEnabled && doesIdpSupportPermissionSyncing(providerConfig.provider), | ||
| }); |
Contributor
There was a problem hiding this comment.
Filter the “unlinked” list to account_linking providers before pushing.
This loop currently adds every configured-but-unlinked provider. packages/web/src/app/(app)/settings/linked-accounts/page.tsx renders the full getLinkedAccounts() result, so login-only providers will now show up as connectable linked accounts in Settings.
Suggested fix
for (const [id, providerConfig] of Object.entries(config.identityProviders ?? {})) {
+ if (providerConfig.purpose !== 'account_linking') {
+ continue;
+ }
+
const account = accounts.find((account) => account.providerId === id);
if (!account) {
result.push({
providerId: id,
providerType: providerConfig.provider,
displayName: providerConfig.displayName,
isLinked: false,
- isAccountLinkingProvider: providerConfig.purpose === 'account_linking',
- required: providerConfig.purpose === 'account_linking' ? (providerConfig.accountLinkingRequired ?? false) : false,
+ isAccountLinkingProvider: true,
+ required: providerConfig.accountLinkingRequired ?? false,
supportsPermissionSync: permissionSyncEnabled && doesIdpSupportPermissionSyncing(providerConfig.provider),
});
}
}📝 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.
Suggested change
| for (const [id, providerConfig] of Object.entries(config.identityProviders ?? {})) { | |
| const account = accounts.find((account) => account.providerId === id); | |
| if (!account) { | |
| result.push({ | |
| provider: providerConfig.provider, | |
| providerId: id, | |
| providerType: providerConfig.provider, | |
| displayName: providerConfig.displayName, | |
| isLinked: false, | |
| isAccountLinkingProvider: providerConfig.purpose === 'account_linking', | |
| required: providerConfig.purpose === 'account_linking' ? (providerConfig.accountLinkingRequired ?? false) : false, | |
| supportsPermissionSync: permissionSyncEnabled && PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS.includes(providerConfig.provider), | |
| supportsPermissionSync: permissionSyncEnabled && doesIdpSupportPermissionSyncing(providerConfig.provider), | |
| }); | |
| for (const [id, providerConfig] of Object.entries(config.identityProviders ?? {})) { | |
| if (providerConfig.purpose !== 'account_linking') { | |
| continue; | |
| } | |
| const account = accounts.find((account) => account.providerId === id); | |
| if (!account) { | |
| result.push({ | |
| providerId: id, | |
| providerType: providerConfig.provider, | |
| displayName: providerConfig.displayName, | |
| isLinked: false, | |
| isAccountLinkingProvider: true, | |
| required: providerConfig.accountLinkingRequired ?? false, | |
| supportsPermissionSync: permissionSyncEnabled && doesIdpSupportPermissionSyncing(providerConfig.provider), | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/web/src/ee/features/sso/actions.ts` around lines 77 - 88, The loop
in the SSO action that iterates over Object.entries(config.identityProviders ??
{}) is adding all unlinked providers to the result; change it to only consider
providers intended for account linking by filtering where providerConfig.purpose
=== 'account_linking' before pushing into result (i.e., restrict the iteration
or add an if that skips non-account_linking providers), keeping the existing
fields (providerId, providerType, displayName, isLinked,
isAccountLinkingProvider, required, supportsPermissionSync) and preserving the
required = providerConfig.accountLinkingRequired fallback logic and permission
sync check via doesIdpSupportPermissionSyncing.
brendan-kellam
force-pushed
the
bkellam/multi-idp
branch
from
ee0d37f to
2579245
Compare
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for multiple IDP providers of the same type. This is mostly useful in the scenario where you have code hosted in e.g., gitlab.com and a self-hosted gitlab instance and you want to use permission syncing. In this scenario, users need a mechanism of linking their account from gitlab.com and the self-hosted gitlab deployment.
Here's a example config:
TODOs:
objectform ofidentityProvidersSummary by CodeRabbit
New Features
Bug Fixes