feat: add connected-accounts disconnect and listing with orphan cook… - #2784
feat: add connected-accounts disconnect and listing with orphan cook…#2784Piyush-85 wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2784 +/- ##
==========================================
+ Coverage 87.98% 88.58% +0.59%
==========================================
Files 80 80
Lines 11514 11962 +448
Branches 2386 2486 +100
==========================================
+ Hits 10131 10596 +465
+ Misses 1338 1321 -17
Partials 45 45 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConnected-account types and errors were added. ChangesConnected Accounts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant Auth0Client
participant AuthClient
participant MyAccountAPI
participant SessionStore
Request->>Auth0Client: Request connected-account operation
Auth0Client->>AuthClient: Mint My Account token
AuthClient->>MyAccountAPI: List or delete connected accounts
MyAccountAPI-->>AuthClient: Return account data or status
AuthClient-->>Auth0Client: Return result or typed error
Auth0Client->>SessionStore: Reconcile cached token sets
SessionStore-->>Request: Persist session and cookie changes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/server/client.ts (1)
1827-1827: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
connectAccountdoesn't use the newgetMyAccountAccessTokenpath.Unlike
disconnectAccount/getConnectedAccounts, this still callsgetAccessToken(options)even when a request was supplied, so a refreshed token set isn't persisted in middleware/Pages Router usage. Since this overload takes nores, either document the limitation or thread a response through.🤖 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 `@src/server/client.ts` at line 1827, Update connectAccount to use the getMyAccountAccessToken path when a request is supplied, ensuring refreshed tokens are persisted for middleware/Pages Router usage. Because connectAccount has no response parameter, either thread the response through its API and call path or explicitly document the persistence limitation; do not leave the request path using getAccessToken alone.src/server/auth-client.ts (1)
4495-4507: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valuePartial-failure information is discarded.
If the second of three deletes fails, the accounts already removed server-side are dropped along with the returned tuple, so
Auth0Client.disconnectAccountthrows and skips pruning the now-invalid cached connection tokens. Consider surfacing the removed subset (e.g. on the error) so callers can reconcile.🤖 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 `@src/server/auth-client.ts` around lines 4495 - 4507, Update the matching-account deletion flow in Auth0Client.disconnectAccount so a later delete failure preserves and surfaces the already removed accounts instead of returning only the error. Attach or otherwise propagate the removed subset alongside the delete error, and ensure the caller can use it to prune invalid cached connection tokens while retaining the existing all-success result.
🤖 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 `@src/server/auth-client.ts`:
- Around line 4364-4397: Bound the pagination loop around the connected-accounts
fetch by enforcing a finite maximum page count and tracking previously seen next
tokens. Before each request, stop or return the existing failed-to-list error
when the limit is reached or the token repeats, while preserving normal account
accumulation and pagination for valid changing tokens.
In `@src/server/client.ts`:
- Around line 1185-1191: Update the token-set replacement logic in the
existingTokenSet branch to replace only the specific entry matched by the
retrieval lookup, rather than every entry sharing options.connection when
login_hint is absent. Reuse the matched token set’s identity or index from the
preceding find operation while preserving all other connectionTokenSets entries.
In `@src/server/session/stateless-session-store.ts`:
- Around line 149-162: Update the orphan-cookie cleanup loop in the session
store method containing getConnectionTokenSetsCookies so that each deleted
cookie is removed from both resCookies and reqCookies. Preserve the existing
index filtering and cookie deletion options, ensuring subsequent get/set
operations in the same request cannot reassemble deleted __FC_i cookies.
---
Nitpick comments:
In `@src/server/auth-client.ts`:
- Around line 4495-4507: Update the matching-account deletion flow in
Auth0Client.disconnectAccount so a later delete failure preserves and surfaces
the already removed accounts instead of returning only the error. Attach or
otherwise propagate the removed subset alongside the delete error, and ensure
the caller can use it to prune invalid cached connection tokens while retaining
the existing all-success result.
In `@src/server/client.ts`:
- Line 1827: Update connectAccount to use the getMyAccountAccessToken path when
a request is supplied, ensuring refreshed tokens are persisted for
middleware/Pages Router usage. Because connectAccount has no response parameter,
either thread the response through its API and call path or explicitly document
the persistence limitation; do not leave the request path using getAccessToken
alone.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7d17720-5819-4f3c-a35b-a786084fdb55
📒 Files selected for processing (11)
src/errors/index.tssrc/errors/my-account-errors.tssrc/server/auth-client.test.tssrc/server/auth-client.tssrc/server/client.test.tssrc/server/client.tssrc/server/session/stateless-session-store.test.tssrc/server/session/stateless-session-store.tssrc/types/connected-accounts.tssrc/types/index.tssrc/types/token-vault.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/client.ts (2)
1959-1982: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPost-mint session re-read reads request cookies, which
saveToSessionnever updates. Both reconciliation paths re-read viagetSessionFromAuthClientto avoid clobbering a rotated refresh token, but for middleware and Pages Router the rotated session was only written tores.cookies/ response headers, so the re-read returns the pre-mint snapshot and the clobbering can still occur.
src/server/client.ts#L1959-L1982: indisconnectAccount, obtain the post-mint session from the mint path (e.g. havegetMyAccountAccessTokenreturn the persisted session) rather than re-reading request cookies.src/server/client.ts#L2067-L2093: apply the same change ingetConnectedAccountsso reconciliation prunes from the actually-persisted session.🤖 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 `@src/server/client.ts` around lines 1959 - 1982, Update disconnectAccount at src/server/client.ts lines 1959-1982 to use the session persisted by the getMyAccountAccessToken mint path instead of re-reading request cookies, then prune and save that session. Apply the same persisted-session flow in getConnectedAccounts at src/server/client.ts lines 2067-2093; both sites must reconcile against the post-mint session so rotated tokens are preserved.
1845-1863: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoute the My Account token mint through the request-aware
getAccessTokenoverload.
connectAccount(options, req)resolvesnormalizedReqbefore minting, but then callsthis.getAccessToken(getMyAccountTokenOpts), which uses the App Router signature without a response object. In middleware/Path Router contexts,refresh: truetoken updates are not persisted becausesaveToSession()takes the App Router cookies path. Use a helper that callsgetAccessToken(req, res, options)when a response is available, andgetAccessToken(req, options)when only middleware/request context is available, with no-op cookie persistence for App Router calls as needed.🤖 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 `@src/server/client.ts` around lines 1845 - 1863, The My Account token mint in connectAccount must use the request-aware getAccessToken overload so refreshed tokens persist correctly. After resolving normalizedReq and the available response, call getAccessToken(req, res, getMyAccountTokenOpts) when a response exists, otherwise call getAccessToken(req, getMyAccountTokenOpts); preserve the App Router path with no-op cookie persistence where required.
🧹 Nitpick comments (3)
src/server/auth-client.ts (2)
4363-4409: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCycle guard looks correct; consider de-duplicating accumulated accounts.
The guard bounds pages and stops on a repeated token, but a misbehaving server that echoes
nextonce still yields the same account twice inaccounts.disconnectAccountwould then issue a duplicate DELETE for that id, likely turning a success intofailed_to_delete. Deduping byaccount.idwhile accumulating removes that edge case.🤖 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 `@src/server/auth-client.ts` around lines 4363 - 4409, Deduplicate accounts in the pagination loop before appending them to accounts, using account.id as the uniqueness key. Update the accumulation logic around the body.accounts iteration so repeated IDs are retained only once, while preserving the existing field mapping and pagination behavior.
6891-6925: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNear-duplicate of
buildConnectAccountErrorResponse.Both helpers differ only in the error class and message text. A single generic builder (taking a factory + message) would keep the
MyAccountApiErrorcause-parsing logic in one place.🤖 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 `@src/server/auth-client.ts` around lines 6891 - 6925, The error-response helpers duplicate the response parsing and MyAccountApiError cause construction. Refactor buildConnectAccountErrorResponse and buildDisconnectAccountErrorResponse into one generic builder that accepts the appropriate error factory and action message, preserving each helper’s public return type and error-specific behavior while centralizing JSON parsing and fallback handling.src/server/client.test.ts (1)
1197-1232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest mocks the very read it is validating.
Stubbing
getSessionFromAuthClientwith two queued values asserts the code calls it twice, not that a real re-read observes the rotated session. A variant driving the store through actual request/response cookies (as the gh-2450 test does) would cover the Pages Router / middleware path where request cookies are not updated bysaveToSession.🤖 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 `@src/server/client.test.ts` around lines 1197 - 1232, Replace the queued getSessionFromAuthClient mocks in the disconnectAccount test with a realistic session-store flow that drives rotation through actual request/response cookies, following the gh-2450 test pattern. Ensure the test verifies the re-read observes the rotated refresh token while disconnectAccount still prunes the requested connection, without mocking the getSessionFromAuthClient read being validated.
🤖 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.
Outside diff comments:
In `@src/server/client.ts`:
- Around line 1959-1982: Update disconnectAccount at src/server/client.ts lines
1959-1982 to use the session persisted by the getMyAccountAccessToken mint path
instead of re-reading request cookies, then prune and save that session. Apply
the same persisted-session flow in getConnectedAccounts at src/server/client.ts
lines 2067-2093; both sites must reconcile against the post-mint session so
rotated tokens are preserved.
- Around line 1845-1863: The My Account token mint in connectAccount must use
the request-aware getAccessToken overload so refreshed tokens persist correctly.
After resolving normalizedReq and the available response, call
getAccessToken(req, res, getMyAccountTokenOpts) when a response exists,
otherwise call getAccessToken(req, getMyAccountTokenOpts); preserve the App
Router path with no-op cookie persistence where required.
---
Nitpick comments:
In `@src/server/auth-client.ts`:
- Around line 4363-4409: Deduplicate accounts in the pagination loop before
appending them to accounts, using account.id as the uniqueness key. Update the
accumulation logic around the body.accounts iteration so repeated IDs are
retained only once, while preserving the existing field mapping and pagination
behavior.
- Around line 6891-6925: The error-response helpers duplicate the response
parsing and MyAccountApiError cause construction. Refactor
buildConnectAccountErrorResponse and buildDisconnectAccountErrorResponse into
one generic builder that accepts the appropriate error factory and action
message, preserving each helper’s public return type and error-specific behavior
while centralizing JSON parsing and fallback handling.
In `@src/server/client.test.ts`:
- Around line 1197-1232: Replace the queued getSessionFromAuthClient mocks in
the disconnectAccount test with a realistic session-store flow that drives
rotation through actual request/response cookies, following the gh-2450 test
pattern. Ensure the test verifies the re-read observes the rotated refresh token
while disconnectAccount still prunes the requested connection, without mocking
the getSessionFromAuthClient read being validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 864094a6-6a2e-423e-bfed-b9f661a152bb
📒 Files selected for processing (5)
src/server/auth-client.test.tssrc/server/auth-client.tssrc/server/client.test.tssrc/server/client.tssrc/server/session/stateless-session-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/session/stateless-session-store.ts
| // disconnected), the trailing higher-index cookies would otherwise linger as | ||
| // orphans and be re-assembled into the session on the next read. Delete any | ||
| // `__FC_i` present in the request whose index is beyond the current length. | ||
| for (const cookie of this.getConnectionTokenSetsCookies(reqCookies)) { |
There was a problem hiding this comment.
this loop is the __FC side of the 431 fix. the connection token cookies are positional (__FC_0..n-1), so when the array shrinks (someone disconnects an account) the higher-index cookies don't get overwritten by the write above. leave them and the browser keeps sending them, they get re-assembled into the session on the next read, and the request header grows until you hit 431. so here i just delete any __FC_i in the request whose index is past the current count.
the reqCookies.delete() is the easy-to-miss part: storeInCookie writes reqCookies too for read-after-write in the same middleware request, so a res-only deletion would still be re-read and re-assembled within that same request. deleting from both keeps it gone.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/server/auth-client.ts`:
- Around line 4370-4372: Update the pagination loop in the account-fetching
method containing the pages, next, and seenNext checks so exceeding MAX_PAGES or
detecting a repeated next cursor returns or propagates an error instead of
breaking with partial account data. Preserve normal pagination results while
ensuring downstream reconciliation cannot treat truncated results as successful.
- Around line 4412-4417: Replace the DPoPError instanceof checks in both catch
blocks with a shared predicate that identifies DPoP failures by the caught
error’s error.code value. Reuse that predicate when selecting the error message,
preserving the existing fallback message and DPoP-specific message behavior.
In `@src/server/client.ts`:
- Around line 1828-1833: Update the Connect Account token-minting flow around
resolveRequestContext, getSessionFromAuthClient, and connectAccountResponse to
pass normalizedReq instead of re-resolving ambient request context. Propagate
any refreshed session cookies produced during minting onto
connectAccountResponse, preserving the resolved auth client and normalized
request throughout middleware/MCD flows.
- Around line 1960-1967: The session pruning flow around
getSessionFromAuthClient must not re-read the session from request cookies after
token minting. Make the token-mint path return or expose its updated session
snapshot, then use that value for pruning and saving, preserving the existing
fallback only when no updated session is available; apply the same change to the
corresponding flow near the second occurrence.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88e1f336-5491-4da5-92e6-add285d9fac9
📒 Files selected for processing (4)
src/server/auth-client.tssrc/server/client.tssrc/types/index.tssrc/types/token-vault.ts
…ross connect/disconnect/list
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server/auth-client.ts (1)
6945-6954: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
messagein the type guard.The guard narrows to
{ code: DPoPErrorCode; message: string }but only checkscode. All three call sites then assigne.messageto the returned error message. If the thrown value has a matchingcodeand no stringmessage, the SDK surfacesundefinedas the error message. Add amessagecheck so the narrowed type matches what is verified.♻️ Proposed refactor
function isDPoPError( e: unknown ): e is { code: DPoPErrorCode; message: string } { return ( typeof e === "object" && e !== null && "code" in e && + typeof (e as { message?: unknown }).message === "string" && Object.values(DPoPErrorCode).includes((e as { code: DPoPErrorCode }).code) ); }🤖 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 `@src/server/auth-client.ts` around lines 6945 - 6954, Update the isDPoPError type guard to also verify that e has a message property whose value is a string, alongside the existing DPoPErrorCode check, so its narrowed type is fully validated before callers use e.message.src/server/client.test.ts (1)
900-906: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for
mintMyAccountToken.These suites mock the private
mintMyAccountToken, so the callers are verified against a stubbed contract only. The method's own logic stays untested:persist: trueversuspersist: false, thesessionChangedcomputation fromgetSessionChangesAfterGetAccessToken, thefinalizeSessioncall, and theMISSING_SESSIONthrow. A regression insidemintMyAccountTokenwould keep all of these tests green.Add one focused suite that calls
mintMyAccountTokenwith a real session and a stubbedauthClient.getTokenSet, and assert the returnedsession,sessionChanged, and whethersaveToSessionran.Also applies to: 1108-1115
🤖 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 `@src/server/client.test.ts` around lines 900 - 906, Add focused direct tests for the private mintMyAccountToken method, using a real session and a stubbed authClient.getTokenSet; cover both persist: true and persist: false, assert the returned session and sessionChanged from getSessionChangesAfterGetAccessToken, verify finalizeSession/saveToSession behavior, and assert MISSING_SESSION is thrown when no session exists. Keep existing caller tests unchanged.
🤖 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 `@src/server/auth-client.ts`:
- Around line 6945-6954: Update the isDPoPError type guard to also verify that e
has a message property whose value is a string, alongside the existing
DPoPErrorCode check, so its narrowed type is fully validated before callers use
e.message.
In `@src/server/client.test.ts`:
- Around line 900-906: Add focused direct tests for the private
mintMyAccountToken method, using a real session and a stubbed
authClient.getTokenSet; cover both persist: true and persist: false, assert the
returned session and sessionChanged from getSessionChangesAfterGetAccessToken,
verify finalizeSession/saveToSession behavior, and assert MISSING_SESSION is
thrown when no session exists. Keep existing caller tests unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0abadc16-575e-40af-b5ab-0972b6ee1497
📒 Files selected for processing (4)
src/server/auth-client.test.tssrc/server/auth-client.tssrc/server/client.test.tssrc/server/client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/auth-client.test.ts
2 participants
📋 Changes
Adds disconnect connected accounts and list connected accounts capability, and fixes a connection-token cookie leak that could grow the request header size until it hit an HTTP 431.
New public methods on
Auth0Client:disconnectAccount({ connection })— unlinks all connected accounts for a connection via the My Account API, then prunes the matching cached connection tokens from the session. It is connection-scoped, not per-account.getConnectedAccounts()— returnsConnectedAccount[]Login-hint support for multiple accounts on one connection:
ConnectionTokenSetgains an optional login hint. WhengetAccessTokenForConnection({ connection, login_hint })is called, the login hint is forwarded to the federated-connection token exchange and stamped onto the resulting cached token. Subsequent calls match cached tokens on both connection and login hint, falling back to connection-only when no hint is supplied (preserving existing single-account behavior).This lets an app hold and refresh access tokens for several accounts on the same connection without one account's refresh overwriting a sibling's cached token. The hint is a token-fetch-time discriminator supplied on
getAccessTokenForConnection;connectAccountitself does not take or persist a login hint, so the app is responsible for passing the samelogin_hintit uses to obtain each account's token.The 431 fix (stateless session store):
📎 References
Addresses GH-2450 (HTTP 431 caused by connection-token cookie accumulation).
🎯 Testing
Run the unit suite. Added coverage includes:
disconnectAccountandgetConnectedAccountsfacades across App Router and Pages Router paths, plus theconnectAccountoverloads.listConnectedAccountspagination, field mapping (including orgId), and error paths.Summary by CodeRabbit
login_hintsupport for selecting and caching account tokens.