fix: surface permission sync issues requiring user action#1484
fix: surface permission sync issues requiring user action#1484brendan-kellam wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughPermission synchronization now persists classified account issues, clears cached permissions on permanent failures, exposes issue status through APIs, retries after reauthentication, and presents actionable banner and linked-account recovery states. ChangesPermission sync issue lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WebAuth
participant WorkerApi
participant PermissionSyncer
participant Database
participant PermissionSyncBanner
User->>WebAuth: reauthenticate linked account
WebAuth->>WorkerApi: request account permission sync
WorkerApi->>PermissionSyncer: start sync job
PermissionSyncer->>Database: record or clear permission-sync issue
PermissionSyncBanner->>Database: poll permission-sync status
Database-->>PermissionSyncBanner: issue or syncing status
PermissionSyncBanner-->>User: show recovery or progress banner
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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 |
This comment has been minimized.
This comment has been minimized.
brendan-kellam
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| update: { | ||
| permissionSyncedAt: new Date(), | ||
| permissionSyncIssue: null, | ||
| permissionSyncIssueAt: null, |
There was a problem hiding this comment.
Completed job clears newer issue
High Severity
onJobCompleted always nulls permissionSyncIssue for the account, with no check against a newer fail-closed update. Concurrent jobs for the same account are allowed (schedulePermissionSyncForAccount never dedupes; default concurrency is 8), so a later successful completion can erase an issue after another job already cleared permissions, leaving empty access and no banner.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c149678. Configure here.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/web/src/features/workerApi/actions.ts (1)
64-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the underlying error before returning a generic failure.
The catch block discards the actual error (network failure, timeout, schema mismatch), unlike the equivalent scheduling path in
auth.tswhich logs it. This makes production failures hard to diagnose.♻️ Suggested fix
export const triggerAccountPermissionSync = async (accountId: string) => sew(() => withAuth(({ role }) => withMinimumOrgRole(role, OrgRole.MEMBER, async () => { try { return await requestAccountPermissionSync(accountId); - } catch { + } catch (error) { + logger.error(`Failed to trigger account permission sync for account ${accountId}: ${error instanceof Error ? error.message : String(error)}`); return unexpectedError('Failed to trigger account permission sync'); } }) ) );🤖 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/features/workerApi/actions.ts` around lines 64 - 74, Update the catch block in triggerAccountPermissionSync to capture the underlying error and log it before returning the existing generic unexpectedError response, matching the diagnostic behavior used by the equivalent scheduling path in auth.ts.packages/web/src/auth.ts (1)
228-240: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider not awaiting the recovery sync inside
events.signIn.
requestAccountPermissionSyncis awaited directly in the sign-in event, which Auth.js awaits before completing the response — this can add up to the client's 5s timeout to sign-in latency for accounts with an existing permission issue if the worker is slow. Since this is a fire-and-forget scheduling call (errors are already caught/logged and don't affect sign-in outcome), consider not awaiting it so sign-in isn't delayed by worker availability.♻️ Suggested fix
if ( updatedAccount.permissionSyncIssue !== null && env.PERMISSION_SYNC_ENABLED === 'true' ) { - try { - if (await hasEntitlement('permission-syncing')) { - await requestAccountPermissionSync(updatedAccount.id); - } - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - logger.error(`Failed to schedule permission sync after reauthentication for account ${updatedAccount.id}: ${message}`); - } + hasEntitlement('permission-syncing') + .then((entitled) => entitled ? requestAccountPermissionSync(updatedAccount.id) : undefined) + .catch((error) => { + const message = error instanceof Error ? error.message : String(error); + logger.error(`Failed to schedule permission sync after reauthentication for account ${updatedAccount.id}: ${message}`); + }); }🤖 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/auth.ts` around lines 228 - 240, Update the recovery sync block in the events.signIn flow to invoke requestAccountPermissionSync without awaiting its completion, while retaining the existing error capture and logger.error handling so failures remain logged without delaying sign-in.
🤖 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/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 259-273: Update the fail-closed cleanup warning in the account
permission sync flow to remove account.user.email from the log message,
retaining the account.id and existing cleanup details and error message. Do not
alter the transaction or cleanup behavior.
---
Nitpick comments:
In `@packages/web/src/auth.ts`:
- Around line 228-240: Update the recovery sync block in the events.signIn flow
to invoke requestAccountPermissionSync without awaiting its completion, while
retaining the existing error capture and logger.error handling so failures
remain logged without delaying sign-in.
In `@packages/web/src/features/workerApi/actions.ts`:
- Around line 64-74: Update the catch block in triggerAccountPermissionSync to
capture the underlying error and log it before returning the existing generic
unexpectedError response, matching the diagnostic behavior used by the
equivalent scheduling path in auth.ts.
🪄 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: 99b97fda-b5b7-47a9-a62e-c231dc3cc2ad
📒 Files selected for processing (19)
CHANGELOG.mdpackages/backend/src/ee/accountPermissionSyncer.test.tspackages/backend/src/ee/accountPermissionSyncer.tspackages/db/prisma/migrations/20260722144824_add_account_permission_sync_issue/migration.sqlpackages/db/prisma/schema.prismapackages/web/src/app/(app)/components/banners/bannerResolver.test.tspackages/web/src/app/(app)/components/banners/bannerResolver.tsxpackages/web/src/app/(app)/components/banners/permissionSyncBanner.test.tsxpackages/web/src/app/(app)/components/banners/permissionSyncBanner.tsxpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/api/(server)/ee/permissionSyncStatus/api.test.tspackages/web/src/app/api/(server)/ee/permissionSyncStatus/api.tspackages/web/src/auth.tspackages/web/src/ee/features/sso/actions.tspackages/web/src/ee/features/sso/components/linkedAccountProviderCard.test.tsxpackages/web/src/ee/features/sso/components/linkedAccountProviderCard.tsxpackages/web/src/features/workerApi/actions.tspackages/web/src/features/workerApi/client.server.test.tspackages/web/src/features/workerApi/client.server.ts
| const details = PERMISSION_CLEANUP_DETAILS[cleanupDecision.reason]; | ||
| const [{ count }] = await this.db.$transaction([ | ||
| this.db.accountToRepoPermission.deleteMany({ | ||
| where: { accountId: account.id }, | ||
| }), | ||
| this.db.account.update({ | ||
| where: { id: account.id }, | ||
| data: { | ||
| permissionSyncIssue: details.issue, | ||
| permissionSyncIssueAt: new Date(), | ||
| }, | ||
| }), | ||
| ]); | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| const reason = PERMISSION_CLEANUP_REASON_MESSAGES[cleanupDecision.reason]; | ||
| logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${reason}: ${message}`); | ||
| logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid logging user email in the fail-closed cleanup log line.
account.user.email is included in the warn log for fail-closed cleanup. This mirrors existing (unchanged) patterns elsewhere in the file, but static analysis flags it as PII exposure in logs (CWE-532). Consider logging account.id only, or a redacted/hashed identifier.
🔒 Suggested fix
- logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message}`);
+ logger.warn(`Cleared ${count} permission row(s) for account ${account.id} — fail-closed cleanup triggered by ${details.message}: ${message}`);📝 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 details = PERMISSION_CLEANUP_DETAILS[cleanupDecision.reason]; | |
| const [{ count }] = await this.db.$transaction([ | |
| this.db.accountToRepoPermission.deleteMany({ | |
| where: { accountId: account.id }, | |
| }), | |
| this.db.account.update({ | |
| where: { id: account.id }, | |
| data: { | |
| permissionSyncIssue: details.issue, | |
| permissionSyncIssueAt: new Date(), | |
| }, | |
| }), | |
| ]); | |
| const message = error instanceof Error ? error.message : String(error); | |
| const reason = PERMISSION_CLEANUP_REASON_MESSAGES[cleanupDecision.reason]; | |
| logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${reason}: ${message}`); | |
| logger.warn(`Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message}`); | |
| const details = PERMISSION_CLEANUP_DETAILS[cleanupDecision.reason]; | |
| const [{ count }] = await this.db.$transaction([ | |
| this.db.accountToRepoPermission.deleteMany({ | |
| where: { accountId: account.id }, | |
| }), | |
| this.db.account.update({ | |
| where: { id: account.id }, | |
| data: { | |
| permissionSyncIssue: details.issue, | |
| permissionSyncIssueAt: new Date(), | |
| }, | |
| }), | |
| ]); | |
| const message = error instanceof Error ? error.message : String(error); | |
| logger.warn(`Cleared ${count} permission row(s) for account ${account.id} — fail-closed cleanup triggered by ${details.message}: ${message}`); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 272-272: Avoid logging sensitive data
Context: logger.warn(Cleared ${count} permission row(s) for account ${account.id} (user ${account.user.email ?? 'unknown'}) — fail-closed cleanup triggered by ${details.message}: ${message})
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
🤖 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 259 - 273,
Update the fail-closed cleanup warning in the account permission sync flow to
remove account.user.email from the log message, retaining the account.id and
existing cleanup details and error message. Do not alter the transaction or
cleanup behavior.
Source: Linters/SAST tools
a985fac to
3cc17b8
Compare
2c03c15 to
5873631
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5873631. Configure here.
| router.refresh(); | ||
| } | ||
| }, [syncJobId, syncStatusData, displayName, toast, router]); | ||
| }, [syncJobId, syncJobStatus, displayName, toast, router]); |
There was a problem hiding this comment.
Sync status poll can hang UI
Medium Severity
When getAccountSyncStatus fails, the refetchInterval stops polling due to missing status data. However, isSyncing remains true, causing the UI to get stuck on 'Syncing...' without a terminal toast or resolution until a page reload.
Reviewed by Cursor Bugbot for commit 5873631. Configure here.
| description="Sourcebot is syncing what repositories you have access to from a code host. This may take a minute." | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
First sync hides action-required banner
Low Severity
When any account still has hasPendingFirstSync, the banner always renders the syncing state and skips the action-required UI. A second linked account with a non-syncing permissionSyncIssue then stays hidden behind “Syncing repository access,” so reconnect guidance is deferred until unrelated first-sync work finishes.
Reviewed by Cursor Bugbot for commit 5873631. Configure here.
1 participant
Stack created with GitHub Stacks CLI
Fixes SOU-1560
Fixes SOU-1177
Summary
Testing
yarn workspace @sourcebot/backend test --run(201 tests)yarn workspace @sourcebot/web test --run(1,095 tests)Part of stack #1483. Depends on #1482.
Note
Medium Risk
Changes EE repository permission state and auth recovery flows; behavior is scoped to classified fail-closed sync failures and is covered by new lifecycle tests, but incorrect issue classification could still mislead users or delay access restoration.
Overview
When account permission sync fails closed and cached repo access is cleared, the backend now persists a structured issue on the linked account (
REAUTHENTICATION_REQUIREDorINSUFFICIENT_SCOPE) in the same transaction as the permission wipe, and clears that issue only after a successful sync.The permission sync status API and app banner expose these issues (non-dismissible warning with a link to linked accounts), including a syncing state while recovery jobs run. Linked account cards use the new issue type instead of generic token-refresh error text and steer users toward reconnect/reauthorize rather than “Refresh Permissions.”
After OAuth reauthentication, if the account still had an open permission sync issue, the web app queues a permission sync via a shared server-side worker client (also used by the manual trigger action).
Reviewed by Cursor Bugbot for commit 2c03c15. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation