feat(ee): Add ability to link external accounts #595
Conversation
…th_provider_config
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces comprehensive identity provider configuration support for multiple authentication types (GitHub, GitLab, Google, Okta, Keycloak, Microsoft Entra ID, GCP IAP), renames the GitHub App type discriminator from "githubApp" to "github" across schemas and code, implements permission-syncing features with UI components and server actions for linking/unlinking integration accounts, and refactors the provider metadata system with new types and schemas. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Layout as App Layout
participant Action as getIntegrationProviderStates()
participant DB as Prisma DB
participant LinkUI as LinkAccounts Component
User->>Layout: Access app (has permission-syncing entitlement)
Layout->>Action: Fetch provider states
Action->>DB: Query identity provider configs & linked accounts
DB-->>Action: Provider configs + user linked accounts
Action-->>Layout: IntegrationIdentityProviderState[]
alt Any provider unlinked
Layout->>LinkUI: Render LinkAccounts prompt
LinkUI->>User: Show required/optional providers to link
User->>LinkUI: Click "Connect" on provider
LinkUI->>User: Redirect to OAuth flow (signIn)
else All required providers linked
Layout->>User: Proceed to normal app flow
end
sequenceDiagram
participant JWT as JWT Callback
participant Token as tokenRefresh Module
participant OAuth as OAuth Provider
participant DB as Prisma DB
JWT->>JWT: Linking integration provider & permission-syncing enabled
JWT->>JWT: Store tokens in token.integrationTokens
JWT->>Token: refreshIntegrationTokens(currentTokens, userId)
loop For each provider
Token->>Token: Check if token close to expiry (5min buffer)
alt Token needs refresh
Token->>OAuth: POST refresh token request
OAuth-->>Token: New accessToken, refreshToken, expiresAt
Token->>DB: Persist refreshed tokens
Token->>Token: Update in-memory token map
else Token valid
Token->>Token: Keep existing token
end
end
Token-->>JWT: Updated IntegrationTokensMap
JWT->>JWT: Return token with refreshed integrationTokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 |
msukkari
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/auth.ts (1)
57-142: Fix provider list mutation before it duplicates entries.
providerspoints directly at the module-leveleeIdentityProviders; every invocation ofgetProviders()mutates that shared array, so the email/credentials providers are appended over and over. This quickly yields duplicate provider entries on the sign-in page and in downstream logic. Clone the array before pushing extras so the EE list stays immutable at module scope.- const providers: IdentityProvider[] = eeIdentityProviders; + const providers: IdentityProvider[] = [...eeIdentityProviders];
♻️ Duplicate comments (3)
CHANGELOG.md (1)
24-25: Remove duplicate "Removed" section.This duplicates the entry at lines 21–22. Keep only one instance.
Apply this diff to remove the duplicate lines:
- Removed built-in secret manager. [#592](https://github.com/sourcebot-dev/sourcebot/pull/592) - -## Removed -- Removed built-in secret manager. [#592](https://github.com/sourcebot-dev/sourcebot/pull/592) -Result (lines 21–23 should be):
## Removed - Removed built-in secret manager. [#592](https://github.com/sourcebot-dev/sourcebot/pull/592)docs/snippets/schemas/v3/app.schema.mdx (1)
11-11: Documentation update reflects schema discriminator change.This documentation snippet reflects the same discriminator change from
"githubApp"to"github"that was reviewed inschemas/v3/app.json. See the verification script in that review.Also applies to: 73-73
packages/web/src/ee/features/sso/sso.ts (1)
77-104: Prevent duplicate provider registration when configs exist.We still append the legacy env-based providers even after loading
identityProvidersfrom config. If an operator defines a provider in config but leaves the same env vars set (e.g.,AUTH_EE_GITHUB_CLIENT_ID), we register two providers with the sameid, which breaks NextAuth’s provider map (last one wins, callbacks become unpredictable). Please gate the legacy path so it only runs whenidentityProvidersis absent/empty.- // @deprecate - if (env.AUTH_EE_GITHUB_CLIENT_ID && env.AUTH_EE_GITHUB_CLIENT_SECRET) { + if (identityProviders.length === 0 && env.AUTH_EE_GITHUB_CLIENT_ID && env.AUTH_EE_GITHUB_CLIENT_SECRET) { providers.push({ provider: createGitHubProvider(env.AUTH_EE_GITHUB_CLIENT_ID, env.AUTH_EE_GITHUB_CLIENT_SECRET, env.AUTH_EE_GITHUB_BASE_URL), purpose: "sso" }); }Repeat the same guard for the other legacy blocks so we only fall back when config is unused.
🧹 Nitpick comments (2)
packages/web/src/ee/features/permissionSyncing/components/providerInfo.tsx (1)
13-22: Consider removing the unnecessary Fragment wrapper.The Fragment wrapper on Lines 14 and 21 is unnecessary since you're returning only a single
divelement. You can simplify the code by returning thedivdirectly.Apply this diff:
return ( - <> - <div className="flex items-center gap-2"> - <span className="text-sm font-medium text-foreground"> - {providerInfo.displayName} - </span> - {showBadge && <ProviderBadge required={required} />} - </div> - </> + <div className="flex items-center gap-2"> + <span className="text-sm font-medium text-foreground"> + {providerInfo.displayName} + </span> + {showBadge && <ProviderBadge required={required} />} + </div> );packages/web/src/ee/features/permissionSyncing/components/linkAccounts.tsx (1)
20-30: Consider providing user feedback on errors.When the skip action fails (Line 25), the error is only logged to the console. Users have no indication that the operation failed. Consider adding a toast notification or other UI feedback mechanism to inform users of errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
CHANGELOG.md(1 hunks)docs/snippets/schemas/v3/app.schema.mdx(2 hunks)docs/snippets/schemas/v3/authProvider.schema.mdx(1 hunks)docs/snippets/schemas/v3/identityProvider.schema.mdx(1 hunks)docs/snippets/schemas/v3/index.schema.mdx(3 hunks)packages/backend/src/ee/githubAppManager.ts(1 hunks)packages/schemas/src/v3/app.schema.ts(2 hunks)packages/schemas/src/v3/app.type.ts(1 hunks)packages/schemas/src/v3/authProvider.schema.ts(1 hunks)packages/schemas/src/v3/authProvider.type.ts(1 hunks)packages/schemas/src/v3/identityProvider.schema.ts(1 hunks)packages/schemas/src/v3/identityProvider.type.ts(1 hunks)packages/schemas/src/v3/index.schema.ts(3 hunks)packages/schemas/src/v3/index.type.ts(4 hunks)packages/shared/src/utils.ts(1 hunks)packages/web/src/app/[domain]/layout.tsx(3 hunks)packages/web/src/app/[domain]/settings/layout.tsx(3 hunks)packages/web/src/app/[domain]/settings/permission-syncing/page.tsx(1 hunks)packages/web/src/app/components/authMethodSelector.tsx(2 hunks)packages/web/src/app/invite/page.tsx(3 hunks)packages/web/src/app/login/components/loginForm.tsx(1 hunks)packages/web/src/app/login/page.tsx(2 hunks)packages/web/src/app/onboard/page.tsx(2 hunks)packages/web/src/app/signup/page.tsx(2 hunks)packages/web/src/auth.ts(5 hunks)packages/web/src/ee/features/permissionSyncing/actions.ts(1 hunks)packages/web/src/ee/features/permissionSyncing/components/integrationProviderCard.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/components/linkAccounts.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/components/linkButton.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/components/linkedAccountsSettings.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/components/providerBadge.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/components/providerIcon.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/components/providerInfo.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/components/unlinkButton.tsx(1 hunks)packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts(1 hunks)packages/web/src/ee/features/permissionSyncing/types.ts(1 hunks)packages/web/src/ee/features/sso/sso.ts(1 hunks)packages/web/src/features/chat/actions.ts(3 hunks)packages/web/src/initialize.ts(1 hunks)packages/web/src/lib/authProviders.ts(0 hunks)packages/web/src/lib/constants.ts(1 hunks)packages/web/src/lib/identityProviders.ts(1 hunks)schemas/v3/app.json(1 hunks)schemas/v3/identityProvider.json(1 hunks)schemas/v3/index.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/web/src/lib/authProviders.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
packages/web/src/app/[domain]/settings/permission-syncing/page.tsxpackages/web/src/ee/features/permissionSyncing/types.tspackages/schemas/src/v3/app.type.tspackages/web/src/ee/features/permissionSyncing/components/providerInfo.tsxpackages/schemas/src/v3/app.schema.tspackages/web/src/lib/identityProviders.tspackages/web/src/initialize.tspackages/schemas/src/v3/authProvider.schema.tspackages/web/src/ee/features/permissionSyncing/components/providerBadge.tsxpackages/web/src/ee/features/permissionSyncing/actions.tsschemas/v3/index.jsonpackages/shared/src/utils.tspackages/web/src/ee/features/permissionSyncing/components/providerIcon.tsxdocs/snippets/schemas/v3/app.schema.mdxschemas/v3/app.jsonpackages/web/src/app/[domain]/settings/layout.tsxdocs/snippets/schemas/v3/authProvider.schema.mdxpackages/backend/src/ee/githubAppManager.tspackages/web/src/app/signup/page.tsxpackages/web/src/app/[domain]/layout.tsxpackages/web/src/app/invite/page.tsxpackages/web/src/ee/features/permissionSyncing/components/integrationProviderCard.tsxpackages/schemas/src/v3/identityProvider.schema.tspackages/web/src/ee/features/permissionSyncing/tokenRefresh.tsCHANGELOG.mdpackages/web/src/lib/constants.tspackages/schemas/src/v3/identityProvider.type.tspackages/web/src/app/login/page.tsxpackages/web/src/ee/features/permissionSyncing/components/linkedAccountsSettings.tsxpackages/web/src/auth.tspackages/web/src/ee/features/permissionSyncing/components/linkAccounts.tsxpackages/schemas/src/v3/authProvider.type.tsdocs/snippets/schemas/v3/identityProvider.schema.mdxpackages/web/src/app/components/authMethodSelector.tsxdocs/snippets/schemas/v3/index.schema.mdxpackages/web/src/app/login/components/loginForm.tsxschemas/v3/identityProvider.jsonpackages/web/src/ee/features/permissionSyncing/components/linkButton.tsxpackages/web/src/app/onboard/page.tsxpackages/web/src/ee/features/sso/sso.tspackages/schemas/src/v3/index.schema.tspackages/web/src/ee/features/permissionSyncing/components/unlinkButton.tsxpackages/web/src/features/chat/actions.tspackages/schemas/src/v3/index.type.ts
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
24-24: Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (19)
packages/web/src/features/chat/actions.ts (1)
192-192: LGTM! Clean removal of unused parameter.The callback no longer needs the
orgparameter since it's not referenced in the function body.packages/web/src/initialize.ts (1)
71-92: Verify the behavioral change for anonymous access forcing.The anonymous access forcing logic now executes unconditionally, even when
CONFIG_PATHwas previously absent. Ensure this behavioral change is intentional and won't cause issues in deployments whereCONFIG_PATHis not configured.The logic itself is well-structured with appropriate entitlement checks and logging.
packages/web/src/app/components/authMethodSelector.tsx (3)
11-14: LGTM - Type refactor aligns with identity provider metadata system.The update from
AuthProvider[]toIdentityProviderMetadata[]is consistent with the broader refactoring in this PR.
18-26: LGTM - Backward compatible prop addition.The new optional
securityNoticeClosableprop with a default value offalsemaintains backward compatibility while enabling control over the security notice's closable state.
38-42: All verification checks passed.The codebase correctly implements the purpose field filtering:
IdentityProviderMetadatainterface properly definespurpose: "sso" | "integration"inpackages/web/src/lib/identityProviders.tsgetIdentityProviderMetadata()consistently extracts and passes thepurposefield from providers- All providers from
getProviders()are correctly assignedpurpose: "sso"The filtering logic at lines 38–42 is sound and will work as intended.
packages/schemas/src/v3/app.type.ts (1)
9-9: LGTM - Type definition consistent with schema update.This change mirrors the discriminator update in the JSON schema. See verification script in
schemas/v3/app.jsonreview.packages/web/src/lib/constants.ts (1)
27-27: LGTM - Cookie constant follows naming conventions.The new constant
OPTIONAL_PROVIDERS_LINK_SKIPPED_COOKIE_NAMEfollows the established pattern for cookie name constants in this file.packages/web/src/app/onboard/page.tsx (1)
9-9: LGTM - Consistent refactor to identity provider metadata.The update from
getAuthProviders()togetIdentityProviderMetadata()is consistent with the broader refactoring across the codebase.Also applies to: 44-44
packages/web/src/ee/features/permissionSyncing/components/providerBadge.tsx (1)
1-16: LGTM - Simple and effective badge component.The
ProviderBadgecomponent correctly implements a clean visual indicator for required vs. optional providers with appropriate variant selection.packages/web/src/app/login/page.tsx (1)
5-5: LGTM - Consistent refactor to identity provider metadata.The update from
getAuthProviders()togetIdentityProviderMetadata()aligns with the identity provider metadata system introduced in this PR.Also applies to: 28-28
schemas/v3/app.json (1)
9-9: All discriminator usages successfully updated.Verification confirms the breaking change has been fully applied:
- Schema definition (schemas/v3/app.json) updated:
"const": "github"- All configuration files (auth.json, basic.json, self-hosted.json, etc.) use
"type": "github"- Runtime code (githubAppManager.ts:53) filters by
app.type === 'github'- No remaining references to the old
"githubApp"discriminator foundpackages/web/src/app/[domain]/settings/layout.tsx (2)
14-14: LGTM!The entitlement check is properly integrated and follows the existing patterns in the file.
Also applies to: 72-72
120-125: LGTM!The conditional navigation item is correctly implemented, following the established patterns for other entitlement-gated items in the sidebar.
schemas/v3/index.json (1)
128-135: LGTM!The new
identityProvidersproperty is correctly structured and follows the established schema patterns for array properties in this configuration file.packages/web/src/app/invite/page.tsx (2)
10-10: LGTM!The migration from
getAuthProviderstogetIdentityProviderMetadatais correctly implemented and the import/usage are consistent.Also applies to: 33-33
60-60: LGTM!The type signature update correctly reflects the migration to the new identity provider system.
packages/web/src/ee/features/permissionSyncing/components/linkAccounts.tsx (1)
32-32: LGTM!The
canSkiplogic correctly verifies that all required providers are linked, and the sort function properly prioritizes required providers in the display order.Also applies to: 45-46
packages/schemas/src/v3/app.schema.ts (1)
1-1: Source schema has been correctly updated.The source schema file
schemas/v3/app.jsoncontains the updated discriminator value"const": "github"and no lingering"githubApp"references exist anywhere in the repository. The generated filepackages/schemas/src/v3/app.schema.tscorrectly reflects this change and will persist across regenerations since the source is the authoritative definition.packages/web/src/ee/features/permissionSyncing/components/linkButton.tsx (1)
13-17: No changes needed—redirectTois the correct property.The
redirectToproperty is the newer Auth.js API and current preferred option name, replacing the oldercallbackUrlin client parameters. The code in linkButton.tsx is already using the correct, modern property name.Likely an incorrect or invalid review comment.
packages/web/src/ee/features/permissionSyncing/components/providerIcon.tsx
Show resolved
Hide resolved
|
**EDIT: #600 resolves this by moving the join table to be between Discussed offline: there is a bug atm where if permission syncing is enabled and one or more integrations are configured, then private repositories that require signin via the integrations will not be accessible (until the next user driven permission sync job runs). This is because The hack fix is to reset the The longer term fix is to support on-demand job queuing s.t., the webapp could schedule a user permission sync job on account linking. |
msukkari
changed the title
This comment has been minimized.
This comment has been minimized.
msukkari
changed the title
Adds a new
identityProvidertop level object to the config file. This allows you to define one or more identity providers which will be used by Sourcebot.There are two different types of identity providers:
ssoandintegration.ssoproviders are used to auth, whileintegrationproviders are linked after authentication and are (currently) used for permission syncing. This allows users to login with a different identity provider than their code host platform (ex. login usingOktabut still be able to connect toGitHubfor permission syncing). It also sets the foundation to be able to oauth into multiple code host platforms to support permission syncing across multiple code host platformsTODO:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Configuration