fix(knowledge): resolve connector tokens as the credential owner, not the KB owner - #6024
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Adds The add-connector OAuth picker excludes service accounts, since connectors don’t support that auth path today. Reviewed by Cursor Bugbot for commit 3f89cce. Configure here. |
Greptile SummaryFixes KB connector OAuth token resolution so syncs and connector PATCH use the credential account owner (scoped to the KB workspace), not the knowledge base owner.
Confidence Score: 5/5The PR appears safe to merge; prior Greptile findings are not outstanding against this HEAD and no new follow-up-eligible defects were established. No blocking failure remains for connector credential token identity resolution on the changed paths.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/credentials/access.ts | New shared helper resolves OAuth token identity by credential account owner with workspace and permission gates. |
| apps/sim/lib/knowledge/connectors/sync-engine.ts | Sync uses credential owner userId for OAuth token refresh instead of KB owner. |
| apps/sim/app/api/knowledge/[id]/connectors/[connectorId]/route.ts | PATCH OAuth path uses the same identity resolution and requires KB workspace context. |
| apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/add-connector-modal/add-connector-modal.tsx | Filters service-account credentials out of the connector picker. |
| apps/sim/lib/credentials/access.test.ts | Covers owner resolution, workspace mismatch, service accounts, legacy account ids, and departed owners. |
Sequence Diagram
sequenceDiagram
participant Sync as executeSync / PATCH
participant Resolve as resolveCredentialTokenIdentity
participant Cred as credential table
participant Acct as account table
participant Perm as getUserEntityPermissions
participant OAuth as refreshAccessTokenIfNeeded
Sync->>Resolve: credentialId, workspaceId
Resolve->>Cred: load credential
alt wrong workspace / unsupported type
Resolve-->>Sync: null
else service_account
Resolve-->>Sync: kind service_account
Sync->>OAuth: refresh(credentialId, callerUserId)
else oauth
Resolve->>Acct: "accountId -> userId"
Resolve->>Perm: owner still in workspace?
alt no access or missing account
Resolve-->>Sync: null
else ok
Resolve-->>Sync: kind oauth, userId
Sync->>OAuth: refresh(credentialId, ownerUserId)
end
end
Reviews (3): Last reviewed commit: "fix(knowledge): resolve connector tokens..." | Re-trigger Greptile
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bfb1d1c. Configure here.
… the KB owner Connector syncs read OAuth tokens as the knowledge base owner. Token reads are scoped to account.userId, so a shared workspace credential authorized by any other member resolved no token at all. Adds resolveCredentialTokenIdentity, matching the ownership resolution authorizeCredentialUse and getCredentialOwner already use, and applies it in the sync engine and the connector PATCH route. Also stops the connector credential picker from offering service accounts. The credential list returns them alongside OAuth accounts and the picker rendered them unfiltered (21 of 30 connectors affected), but no connector can authenticate with one: the sync engine passes no scopes (a Google service account throws) and drops the cloudId/domain/authStyle an Atlassian service account resolves with.
bfb1d1c to
3f89cce
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3f89cce. Configure here.
Summary
account.userId, so a shared workspace credential authorized by a different member resolved no token at all — surfacing asFailed to obtain access token for credential X (provider: confluence)resolveCredentialTokenIdentityand used it in the sync engine and the connectorPATCHroute, matching howauthorizeCredentialUse(65 request-bound routes) andgetCredentialOwner(webhooks + subscription-renewal cron) already resolve credentialsReported from a live workspace: the KB was owned by one member while the Confluence credential was authorized by another, so every sync failed and swapping service accounts couldn't help.
Type of Change
Testing
Verified against production data: all 102 existing credential-backed connectors resolve to the same user before and after this change, so it is a no-op for every current connector and only alters the already-broken case. Also confirmed zero connectors would be newly rejected by the added workspace/permission guards, and zero use service-account credentials.
830 tests passing across credentials/oauth/webhooks (9 new), typecheck clean,
check:api-validationpassing.Checklist