feat(core): support refreshUserInfo in stateful strategy#237
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR adds ChangesRefresh user info flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RefreshUserInfoAPI
participant SessionStrategy
participant OAuthProvider
Client->>RefreshUserInfoAPI: Submit refresh request
RefreshUserInfoAPI->>OAuthProvider: Fetch user info
OAuthProvider-->>RefreshUserInfoAPI: Return user info
RefreshUserInfoAPI->>SessionStrategy: Refresh session and headers
SessionStrategy-->>RefreshUserInfoAPI: Return session and headers
RefreshUserInfoAPI-->>Client: Return refreshed session response
Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/core/src/api/refreshUserInfo.ts (1)
31-34: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid duplicate CSRF validation for stateful refreshes.
This validates CSRF before delegation, then the stateful strategy delegates to
refreshSession, which validates it again. Keep API-level validation as the single owner and use an already-validated strategy path; double validation can be incorrect for stateful/rotating CSRF implementations.Also applies to: 79-83
🤖 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/core/src/api/refreshUserInfo.ts` around lines 31 - 34, The refreshUserInfo validation chain currently invokes verifyCSRFToken before delegating to the stateful refresh path, which validates CSRF again. Update the flow around createValidation and the stateful strategy delegation to use an already-validated strategy path, keeping API-level CSRF validation as the sole validation owner while preserving provider, session, and other existing checks.packages/core/test/api/stateful/refreshUserInfo.test.ts (4)
236-350: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent global-stub cleanup across both new stateful suites. Both files call
vi.stubGlobal("fetch", ...)per test and clean up only in some tests;packages/core/vitest.config.tssets onlyunstubEnvs: true, with nounstubGlobalsorrestoreMocks, so leakedfetchstubs and module spies make results order-dependent.
packages/core/test/api/stateful/refreshUserInfo.test.ts#L236-L350: add a suite-levelafterEach(() => { vi.unstubAllGlobals(); vi.restoreAllMocks() })and drop the ad-hoc per-testvi.unstubAllGlobals()calls; this also restores thecreateSchemaRegistrymodule spy.packages/core/test/api/stateful/getAccessToken.test.ts#L385-L473: add the same suite-levelafterEachso thefetchstubs from the refresh tests don't leak into the expiry-window tests.🤖 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/core/test/api/stateful/refreshUserInfo.test.ts` around lines 236 - 350, Add suite-level afterEach cleanup in packages/core/test/api/stateful/refreshUserInfo.test.ts around the refreshUserInfo tests and remove any ad-hoc per-test vi.unstubAllGlobals() calls; invoke both vi.unstubAllGlobals() and vi.restoreAllMocks() so fetch stubs and the createSchemaRegistry spy are reset. Add the same afterEach cleanup in packages/core/test/api/stateful/getAccessToken.test.ts for its stateful access-token tests, with no other changes required at either site.
302-311: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThese assertions lock in 4 identical session lookups per request.
The
@todoacknowledges it, but encodingtoHaveBeenNthCalledWith(1..4, sessionToken)in five tests makes the redundancy hard to remove later — any de-duplication in the strategy breaks all of them. Consider assertingtoHaveBeenCalledWith(sessionToken)and tracking the optimization separately. Want me to open an issue for the duplicate lookups?🤖 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/core/test/api/stateful/refreshUserInfo.test.ts` around lines 302 - 311, Replace the ordered getSessionByTokenMock assertions in the affected refresh-user-info tests with a non-order-specific assertion that it was called with sessionToken, without asserting exactly four calls. Preserve the existing revokeSessionMock, getOAuthAccountMock, and updateOAuthTokensMock expectations, and track duplicate lookup optimization separately rather than encoding it in these tests.
811-924: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the repeated success-path arrangement and assertions.
This test, plus the ones at Lines 236-350, 987-1098, and 1144-1255, share an identical ~40-line mock setup and an identical ~60-line assertion tail; only the CSRF input mode and the final response check differ. A
setupSuccessfulRefresh()helper returning the mocks plus anexpectRefreshedSession(mocks)assertion helper would cut ~250 lines and keep future strategy changes to a single edit site.🤖 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/core/test/api/stateful/refreshUserInfo.test.ts` around lines 811 - 924, Extract the shared successful refresh setup from the affected success-path tests into a setupSuccessfulRefresh() helper that returns the configured mocks and relevant test values. Extract the common session, schema, and update assertions into expectRefreshedSession(mocks), while keeping each test’s CSRF input and final response assertions local. Replace the duplicated setup and assertion blocks in the identified tests with these helpers.
46-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name doesn't match the scenario, and the true "missing token" path is uncovered.
A session cookie is sent here (
aura-auth.session_token=invalid-token) andgetSessionByTokenis asserted to have been called, so this exercises "session not found for token", identical to the test at Lines 91-138 and Lines 768-809. The branch where nosession_tokencookie exists at all (early return with cleared cookies inrefreshSession,packages/core/src/session/stateful.ts) is never exercised.Rename this to reflect the lookup-miss, and add a case with no session cookie.
🤖 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/core/test/api/stateful/refreshUserInfo.test.ts` around lines 46 - 89, The existing test should be renamed to describe a missing session lookup rather than a missing token. Add a separate test for refreshUserInfo with no session_token cookie, covering the early refreshSession return and verifying the expected failure response with cleared cookies and no session lookup or downstream calls.
🤖 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/core/src/`@types/session.ts:
- Around line 277-288: Update the exported SessionStrategy contract containing
refreshUserInfo so this newly added unstable API does not become a required
member for existing third-party strategies; make it optional while preserving
its current signature, or otherwise defer the mandatory interface change to the
next major release.
In `@packages/core/src/session/stateless.ts`:
- Around line 394-404: Update the header construction in the session response
flow around newHeaders to seed HeadersBuilder with secureApiHeaders instead of
the incoming headers, then retain only intended response additions such as the
session Set-Cookie header. Ensure the subsequent toUnionHeaders call does not
reintroduce caller-controlled request headers.
- Around line 398-404: Update the session construction in the stateless refresh
flow around getStandardSession so the returned session’s numeric JWT exp value
is converted to an ISO-8601 string before returning. Preserve the existing
mergedHeaders behavior and ensure the result conforms to the public
Session.expires contract like the regular getSession path.
In `@packages/core/test/api/stateful/refreshUserInfo.test.ts`:
- Around line 506-558: Update the mockFetch response in the “handles getUserInfo
missing required user fields” test to include valid JSON response headers,
allowing payload validation to run; then change the expected error.code to the
same schema/format validation code asserted by the comparable test at Lines
451-504.
In `@packages/core/vitest.config.ts`:
- Line 43: Remove test/actions/providers/user/refresh/stateless.test.ts from the
exclude list in the Vitest configuration so the new stateless refresh test runs
in the suite; if it must remain disabled, remove the configuration exclusion and
mark the relevant test with describe.skip plus a `@todo` in the test file.
---
Nitpick comments:
In `@packages/core/src/api/refreshUserInfo.ts`:
- Around line 31-34: The refreshUserInfo validation chain currently invokes
verifyCSRFToken before delegating to the stateful refresh path, which validates
CSRF again. Update the flow around createValidation and the stateful strategy
delegation to use an already-validated strategy path, keeping API-level CSRF
validation as the sole validation owner while preserving provider, session, and
other existing checks.
In `@packages/core/test/api/stateful/refreshUserInfo.test.ts`:
- Around line 236-350: Add suite-level afterEach cleanup in
packages/core/test/api/stateful/refreshUserInfo.test.ts around the
refreshUserInfo tests and remove any ad-hoc per-test vi.unstubAllGlobals()
calls; invoke both vi.unstubAllGlobals() and vi.restoreAllMocks() so fetch stubs
and the createSchemaRegistry spy are reset. Add the same afterEach cleanup in
packages/core/test/api/stateful/getAccessToken.test.ts for its stateful
access-token tests, with no other changes required at either site.
- Around line 302-311: Replace the ordered getSessionByTokenMock assertions in
the affected refresh-user-info tests with a non-order-specific assertion that it
was called with sessionToken, without asserting exactly four calls. Preserve the
existing revokeSessionMock, getOAuthAccountMock, and updateOAuthTokensMock
expectations, and track duplicate lookup optimization separately rather than
encoding it in these tests.
- Around line 811-924: Extract the shared successful refresh setup from the
affected success-path tests into a setupSuccessfulRefresh() helper that returns
the configured mocks and relevant test values. Extract the common session,
schema, and update assertions into expectRefreshedSession(mocks), while keeping
each test’s CSRF input and final response assertions local. Replace the
duplicated setup and assertion blocks in the identified tests with these
helpers.
- Around line 46-89: The existing test should be renamed to describe a missing
session lookup rather than a missing token. Add a separate test for
refreshUserInfo with no session_token cookie, covering the early refreshSession
return and verifying the expected failure response with cleared cookies and no
session lookup or downstream calls.
🪄 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: 42090a64-f757-42c5-a646-3c15b459ee53
📒 Files selected for processing (10)
packages/core/src/@types/session.tspackages/core/src/api/refreshUserInfo.tspackages/core/src/session/stateful.tspackages/core/src/session/stateless.tspackages/core/test/actions/providers/user/refresh/stateful.test.tspackages/core/test/actions/providers/user/refresh/stateless.test.tspackages/core/test/api/stateful/getAccessToken.test.tspackages/core/test/api/stateful/getProviderTokens.test.tspackages/core/test/api/stateful/refreshUserInfo.test.tspackages/core/vitest.config.ts
1 participant
Description
This pull request adds Stateful session support for the
refreshUserInfo()API and thePOST /providers/:provider/user/refreshendpoint.With this change, the user profile refresh APIs now support both available session strategies:
The implementation ensures consistent behavior across both strategies, allowing applications to refresh the authenticated user's profile information and synchronize the session with the latest data retrieved from the configured OAuth or OpenID Connect (OIDC) provider, regardless of the configured session storage mechanism.
Key Changes
api.refreshUserInfo().POST /providers/:provider/user/refreshendpoint.Note
This PR is part of the ongoing effort to implement the Stateful session strategy. To keep reviews focused and manageable, the implementation has been split into a series of smaller pull requests, each covering a specific aspect of the feature.
Related PRs
@coderabbitai ignore