feat: add Bitbucket Cloud OAuth identity provider#924
Conversation
- Add BitbucketCloudIdentityProviderConfig schema (provider: "bitbucket-cloud") - Add createBitbucketCloudProvider() in sso.ts with id override and repository scope - Add bitbucket-cloud token refresh support using HTTP Basic Auth - Add IdentityProviderType derived from schema union for type-safe provider constants - Add PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS constant - Add Bitbucket Cloud provider info to getAuthProviderInfo() utility - Add idp.mdx docs for Bitbucket Cloud OAuth consumer setup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughAdds Bitbucket Cloud as a supported OAuth identity provider (SSO and account-linked permission syncing), updating schemas, types, provider wiring, token-refresh logic, backend permission-sync filtering, docs, and UI metadata. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/backend/src/ee/accountPermissionSyncer.ts (1)
173-272: Add a defensiveelseclause to prevent silent permission wipe for future providers.
PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERSand theif/else ifbranches insiderunJobare implicitly coupled. When'bitbucket-cloud'(or any future provider) is added to that constant without a matching branch here,aggregatedRepoIdsstays empty and the transaction at lines 275-293 willdeleteManyall existing permissions while creating none — silently revoking all repo access for every affected user.🛡️ Proposed fix — defensive else clause
repos.forEach(repo => aggregatedRepoIds.add(repo.id)); + } else { + throw new Error(`Unsupported identity provider for permission syncing: ${account.provider}. Add a handler branch in runJob.`); } return Array.from(aggregatedRepoIds);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/ee/accountPermissionSyncer.ts` around lines 173 - 272, The permission-collection logic in runJob risks returning an empty aggregatedRepoIds for unknown providers (e.g., when PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS is extended) which can silently delete all permissions; add a defensive else branch after the account.provider checks that either throws a clear error or logs and returns early when account.provider is unrecognized, referencing the aggregatedRepoIds variable and the runJob/account.provider check so the function fails fast instead of allowing the later deleteMany to run with an empty set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/configuration/idp.mdx`:
- Around line 172-173: The docs currently claim Bitbucket Cloud supports
"permission syncing" but the codebase (see the account permission syncer branch
in runJob and the PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS constant which
excludes 'bitbucket-cloud') does not schedule Bitbucket Cloud syncs; update the
docs in idp.mdx (references around the sentence mentioning "permission syncing"
and the Repositories: Read note at lines ~183–185) to either remove
permission-syncing references for Bitbucket Cloud or explicitly mark permission
syncing as "not yet implemented / coming soon", and add a brief TODO or link to
the tracking issue so readers know it's planned; use the symbols runJob and
PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS to verify the implementation status
while editing the text.
In `@packages/db/prisma/schema.prisma`:
- Around line 409-411: The schema comment describing "matches the `provider`
field of the `IdentityProviderConfig` schema" is misplaced on the `type` field;
update the Prisma model by removing that note from the `type` field and adding
it to the `provider` field so the comment sits directly above `provider` (the
field used for provider identifiers like "github"/"gitlab"); verify the `type`
field remains documented for account category semantics and that only the
`provider` field carries the referenced note.
In `@packages/web/src/ee/features/sso/sso.ts`:
- Around line 225-245: The Bitbucket Cloud provider defined in
createBitbucketCloudProvider is missing the required "email" OAuth scope, so
update the authorization.params.scope array inside createBitbucketCloudProvider
to include "email" (alongside "account" and the conditional "repository" scope)
so requests to /2.0/user/emails will be authorized; ensure the "email" string is
included in the same join(' ') list used for authorization.params.scope in the
Bitbucket provider configuration.
---
Nitpick comments:
In `@packages/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 173-272: The permission-collection logic in runJob risks returning
an empty aggregatedRepoIds for unknown providers (e.g., when
PERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERS is extended) which can silently
delete all permissions; add a defensive else branch after the account.provider
checks that either throws a clear error or logs and returns early when
account.provider is unrecognized, referencing the aggregatedRepoIds variable and
the runJob/account.provider check so the function fails fast instead of allowing
the later deleteMany to run with an empty set.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHANGELOG.mddocs/docs/configuration/idp.mdxdocs/snippets/schemas/v3/identityProvider.schema.mdxdocs/snippets/schemas/v3/index.schema.mdxpackages/backend/src/constants.tspackages/backend/src/ee/accountPermissionSyncer.tspackages/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/index.server.tspackages/shared/src/types.tspackages/web/src/ee/features/permissionSyncing/tokenRefresh.tspackages/web/src/ee/features/sso/sso.tspackages/web/src/lib/utils.tsschemas/v3/identityProvider.json
Summary
BitbucketCloudIdentityProviderConfigschema (provider: "bitbucket-cloud") withssoandaccount_linkingpurpose supportsso.tswith the ID overridden to"bitbucket-cloud", requestingaccount+repositoryscopes when permission syncing is enabledbitbucket-cloudtoken refresh intokenRefresh.tsusing HTTP Basic Auth (Bitbucket's required auth method, unlike GitHub/GitLab which use request body params)IdentityProviderType(derived from the schema union) andPERMISSION_SYNC_SUPPORTED_IDENTITY_PROVIDERSconstant for type-safe provider handling in the account permission syncerdocs/configuration/idp.mdxTest plan
<host>/api/auth/callback/bitbucket-cloud, Account: Read scope, and Repositories: Read scope"provider": "bitbucket-cloud"identity provider toconfig.jsonand verify login flow worksAccountrow is created withprovider = 'bitbucket-cloud'expires_at = 1on the linked account in the DB and trigger a session refresh; verify token is refreshed via logs🤖 Generated with Claude Code
Summary by CodeRabbit