fix(web): Support multiple IDPs of the same type#1177
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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
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.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/shared/src/env.server.ts`:
- Around line 118-120: getIdentityProviderConfigs (and the per-lookup
getIdentityProviderConfig) currently reloads/parses/validates the config on
every call; memoize the normalized IDP config in a module-level cache (store
either the normalized object or a Promise resolving to it) so loadConfig/env
normalization runs once, then have getIdentityProviderConfigs return the cached
value and have getIdentityProviderConfig read from that cached map. Implement
the cache as a top-level variable (e.g., identityProviderConfigCache or
identityProviderConfigPromise) and ensure both getIdentityProviderConfigs and
getIdentityProviderConfig reference it (initializing it on first call) to avoid
repeated file/network reads.
🪄 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: 914f2998-bd09-49a4-bf8b-e3f6804fd059
📒 Files selected for processing (7)
packages/backend/src/ee/accountPermissionSyncer.tspackages/backend/src/ee/tokenRefresh.tspackages/shared/src/env.server.tspackages/shared/src/index.server.tspackages/web/src/ee/features/sso/actions.tspackages/web/src/ee/features/sso/sso.tspackages/web/src/lib/encryptedPrismaAdapter.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/backend/src/ee/accountPermissionSyncer.ts
- packages/web/src/ee/features/sso/actions.ts
- packages/backend/src/ee/tokenRefresh.ts
- packages/web/src/ee/features/sso/sso.ts
Explain the object form of `identityProviders` (keyed by id) and how the callback URL is derived from the chosen id, with a self-hosted + gitlab.com example. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/docs/configuration/idp.mdx (1)
649-657: ⚡ Quick winRewrite the opening paragraph in second person for docs-style compliance.
The new section opens in third person (“By default, each provider...”), which conflicts with the docs guideline to write in second person and present tense.
As per coding guidelines, "Write documentation in second person ('you') and present tense. Keep sentences short and direct. Lead with what the user needs to know."
Suggested edit
-By default, each provider in the `identityProviders` array is identified by an **id** equal to its `provider` value. This id determines the provider's OAuth **callback URL** (sometimes called the redirect URL): +By default, you identify each provider in the `identityProviders` array by an **id** equal to its `provider` value. That id determines the OAuth **callback URL** (sometimes called the redirect URL):🤖 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 `@docs/docs/configuration/idp.mdx` around lines 649 - 657, Rewrite the opening paragraph to address the reader in second person and present tense: start by telling the user what they need to know about the identityProviders array (that each provider is identified by an id equal to its provider value), explain that this id determines the OAuth callback URL format (<sourcebot_url>/api/auth/callback/<id>), and note that the array form supports only one instance per provider type; keep sentences short, direct, and lead with the key action (how to configure multiple instances by switching identityProviders to the object form and assigning unique ids).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 `@docs/docs/configuration/idp.mdx`:
- Line 13: The sentence currently implies the top-level identityProviders in the
config file must be an array; update the phrasing to state that the top-level
identityProviders supports both array and object forms (e.g., an array of
providers or an object keyed by provider id). Edit the line referencing "[config
file] ... in the top-level `identityProviders` array" to instead mention "in the
top-level `identityProviders` (supports array or object forms)" and ensure the
term `identityProviders` is used exactly so readers can find examples elsewhere
in the docs.
---
Nitpick comments:
In `@docs/docs/configuration/idp.mdx`:
- Around line 649-657: Rewrite the opening paragraph to address the reader in
second person and present tense: start by telling the user what they need to
know about the identityProviders array (that each provider is identified by an
id equal to its provider value), explain that this id determines the OAuth
callback URL format (<sourcebot_url>/api/auth/callback/<id>), and note that the
array form supports only one instance per provider type; keep sentences short,
direct, and lead with the key action (how to configure multiple instances by
switching identityProviders to the object form and assigning unique ids).
🪄 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: ad42bb71-ba66-41bc-b293-d6f3c6aeb90c
📒 Files selected for processing (1)
docs/docs/configuration/idp.mdx
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:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation