fix(core): explicitly persist tokens on OAuth2 re-auth#582
Conversation
ProcessAuthorizationCallback relied solely on the DLGT_TOKEN_UPDATE notification chain (setTokens -> notify -> Module.onTokenUpdate -> upsertCredential) to persist freshly-issued OAuth tokens. In production the chain has been observed to silently no-op: callers complete OAuth on the provider side (issuing new tokens and invalidating the prior refresh_token), but the new tokens never land in the DB. The user- visible /api/authorize call appears to succeed, the existing credential is reused with stale token data, and downstream API calls fail with 401 once the provider rotates the old refresh_token. Changes: - Bootstrap the Module with the existing entity (looked up via findEntitiesByUserIdAndModuleName) so the API requester is preloaded with prior tokens. Replaces the `entity = null` TODO. - Belt-and-suspenders: explicitly call onTokenUpdate after getToken on the OAuth2 path. The notification chain remains in place; this ensures persistence happens even when the chain doesn't fire. - Add diagnostic logging at entry, after getToken, after persistence, and after restore so future investigations are not blind. - Wrap POST /api/authorize handler with try/catch + console.error so failures surface in CloudWatch instead of returning silently. - restoreIntegrationsForEntity returns the count of flipped integrations for logging. Tests: existing re-auth status restoration tests pass; new test locks in that upsertCredential is called once on the OAuth2 path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db461ccd7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existingEntity = | ||
| existingEntities && existingEntities.length > 0 | ||
| ? existingEntities[0] | ||
| : null; |
There was a problem hiding this comment.
Using existingEntities[0] makes OAuth2 re-auth pick an arbitrary prior entity when a user has multiple entities for the same module, because repository findMany calls do not enforce ordering. That arbitrary entity's credential is then preloaded into Module, and OAuth2Requester.setTokens preserves an existing refresh_token when the provider does not return one, so a re-auth for account B can persist account A's refresh token and break subsequent refreshes for the updated credential.
Useful? React with 👍 / 👎.
| const module = await getModuleInstanceFromType.execute( | ||
| userId, | ||
| params.entityType, | ||
| { state: req.query.state } | ||
| params.entityType | ||
| ); |
There was a problem hiding this comment.
The GET /api/authorize path no longer forwards req.query.state to getModuleInstanceFromType.execute, so modules that rely on the optional state round-trip cannot include the caller-provided value in their authorization URL. This regresses CSRF/callback correlation flows that depend on a custom state token being propagated into the OAuth requester.
Useful? React with 👍 / 👎.
Reinstates the `{ state: req.query.state }` third argument to
getModuleInstanceFromType.execute in the GET /api/authorize handler.
Was inadvertently dropped when applying this branch's changes from a
working tree based on a commit predating f660549 (feat(core): forward
OAuth state from /api/authorize to module API).
Without this, modules that hardcode &state=${this.state} in their
authorize URL (e.g. api-module-hubspot) interpolate state=null again.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
🚀 PR was released in |
2 participants
Summary
ProcessAuthorizationCallbackrelied solely on theDLGT_TOKEN_UPDATEnotification chain (setTokens→notify→Module.onTokenUpdate→upsertCredential) to persist freshly-issued OAuth tokens. In production this chain has been observed to silently no-op: the OAuth provider issues new tokens (and invalidates the priorrefresh_token), but the new tokens never land in the DB. The user-visible/api/authorizecall appears to succeed, the existing credential is reused with stale token data, and downstream API calls fail with 401 once the provider rotates the old refresh_token.This was diagnosed in a Pipedrive integration: re-installs created new Integration rows pointing at a 3-month-old credential whose tokens had been retired by Pipedrive during today's OAuth flow, but never replaced in our DB. The auth Lambda logs were silent because none of the existing code logs at the relevant boundaries.
Changes
process-authorization-callback.jsnow looks up the existing entity viafindEntitiesByUserIdAndModuleNameand passes it tonew Module(...)so the API requester is preloaded with prior tokens. Replaces the long-standing// todo: check if we need to pass entity to Module, right now it's nullTODO.await this.onTokenUpdate(module, moduleDefinition, userId)aftergetToken. The notification chain stays in place; this ensures persistence happens even when the chain silently no-ops. Idempotent against the notification path becauseupsertCredentialis keyed byexternalId.getToken, after persistence, and after restore — so future re-auth bugs aren't invisible in CloudWatch.POST /api/authorizewith try/catch +console.errorso handler failures surface instead of returning silently.restoreIntegrationsForEntityreturns the count of flipped integrations for logging.Test plan
ProcessAuthorizationCallbackre-auth status restoration tests passpersists credentials explicitly on OAuth2 re-auth (belt-and-suspenders)— locks in thatupsertCredentialis called exactly once on the OAuth2 path[Frigg] processAuthorizationCallback ...log lines and that the credential'supdatedAtmatches the OAuth callback time (not a later auth-flip)ERRORflips it back toENABLEDand the integration resumes syncing🤖 Generated with Claude Code
Version
Published prerelease version:
v2.0.0-next.82Changelog
🚀 Enhancement
@friggframework/core,@friggframework/devtools,@friggframework/eslint-config,@friggframework/prettier-config,@friggframework/schemas,@friggframework/serverless-plugin,@friggframework/test,@friggframework/ui🐛 Bug Fix
@friggframework/coreAuthors: 1