fix(dexie-cloud-addon): fall back to fetchTokens when refresh fails - #2326
Conversation
Server-side, refresh tokens issued via client_credentials (impersonation) now carry a client_id claim and get checked against that client row on every 'refresh_token' grant (dexie-cloud PR #195) -- so a revoked or expired API client's refresh tokens correctly stop working, even if the refresh token's own exp claim (up to 3650 days) hasn't been reached yet. Client-side, login() had no recovery path for that case: a locally unexpired refreshTokenExpiration was trusted blindly, and loadAccessToken() throwing on a 401 propagated straight out of login() uncaught. Now: a refresh failure (other than an OAuth redirect) falls through to the same re-authentication path used for 'no refresh token', re-running fetchTokens exactly as on first login. Customers with their own fetchTokens (e.g. server-side impersonation-based auth) get self-healing behavior: either silent re-mint under the hood if their own session is still valid, or their own challenge/login flow triggered if not -- same as first login.
📝 WalkthroughWalkthroughThe login flow now catches refresh-token failures and continues to full re-authentication. It still rethrows ChangesAuthentication flow
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
addons/dexie-cloud/src/authentication/login.ts (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the refresh failure branches.
Add tests that verify:
- A successful
loadAccessToken()returnsfalseand does not invokefetchTokens.- A non-
OAuthRedirectErrorfailure invokes the existing authentication path.- An
OAuthRedirectErroris rethrown and does not invokefetchTokens.This is a critical authentication recovery path, and the current build does not verify its runtime behavior.
🤖 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 `@addons/dexie-cloud/src/authentication/login.ts` around lines 42 - 48, Add regression tests around the login flow containing loadAccessToken: verify successful token loading returns false without calling fetchTokens, non-OAuthRedirectError failures continue into the existing authentication path, and OAuthRedirectError is rethrown without calling fetchTokens. Use the existing authentication test setup and mocks, asserting both outcomes and fetchTokens invocation behavior.
🤖 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.
Nitpick comments:
In `@addons/dexie-cloud/src/authentication/login.ts`:
- Around line 42-48: Add regression tests around the login flow containing
loadAccessToken: verify successful token loading returns false without calling
fetchTokens, non-OAuthRedirectError failures continue into the existing
authentication path, and OAuthRedirectError is rethrown without calling
fetchTokens. Use the existing authentication test setup and mocks, asserting
both outcomes and fetchTokens invocation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a18b645-1b7a-41dd-bdff-8d3b3a43b6b9
📒 Files selected for processing (1)
addons/dexie-cloud/src/authentication/login.ts
Why
Server-side, refresh tokens issued via the
client_credentialsgrant (impersonation) now carry aclient_idclaim and get checked against that exactinternal.clientsrow on everyrefresh_tokengrant exchange — see dexie/dexie-cloud#195. This means a revoked or expired (expires_at) API client's refresh tokens correctly stop working, even though the refresh token's ownexpclaim can be up to 3650 days out.Client-side,
login()had no recovery path for that case: a locally unexpiredrefreshTokenExpirationwas trusted blindly, andloadAccessToken()throwing on a 401 propagated straight out oflogin()uncaught.This matters most for customers using their own
fetchTokensconfig (e.g. a backend that impersonates end-users viaclient_credentials+claims). If their backend's own API client is later rotated (dexie-cloud rotate) or expires, all of that customer's end-users would previously get an uncaught error on next refresh, with no built-in recovery.What changed
In
login(), a refresh failure (other than anOAuthRedirectError, which means the page is navigating away) now falls through to the exact same re-authentication path already used for the "no refresh token" case — re-runningfetchTokensexactly as on first login, instead of throwing.Practical effect for customers with custom
fetchTokens: this becomes self-healing. Their endpoint can either silently re-mint tokens under the hood (if their own session/auth is still valid), or trigger their own challenge/login flow (if not) — same UX as a first-time login.Testing
pnpm build— clean.login()/the auth flow in this addon's karma suite (onlymax-unsynced-mutationsexists today) — verified via manual trace of the control flow instead. Noting this explicitly rather than claiming test coverage that doesn't exist.Related
client_idclaim + refresh-token revocation check +dexie-cloud rotate). Recommend releasing together, or at minimum this addon fix before/alongside the server release, so the new server-side enforcement doesn't strand existing custom-auth integrations.Summary by CodeRabbit