feat(core): support revokeToken in stateful strategy#234
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds OAuth token revocation to stateful and stateless session strategies, delegates the API handler to those strategies, introduces shared provider revocation and error handling, fixes array merging, and adds extensive action/API tests. ChangesOAuth token revocation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant SessionStrategy
participant OAuthProvider
participant Adapter
Client->>API: Request OAuth token revocation
API->>SessionStrategy: revokeToken(oauth, headers, disconnect)
SessionStrategy->>OAuthProvider: Revoke access token
OAuthProvider-->>SessionStrategy: Revocation response
SessionStrategy->>Adapter: Unlink OAuth account
SessionStrategy-->>API: Updated headers
API-->>Client: Success or mapped error 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: 7
🧹 Nitpick comments (3)
packages/core/test/actions/providers/tokens/revoke/stateful.test.ts (1)
581-593: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBody contents are never asserted, so
tokenHintmapping is untested.
expect.any(URLSearchParams)passes regardless of content. Since this test exists specifically to coverparams.tokenHint/extra params, assert the serialized body to confirmtoken_type_hint=refresh_tokenand thattokenHintisn't leaked as a raw param.🧪 Proposed assertion
expect(mockFetch).toHaveBeenCalled() + const body = mockFetch.mock.calls[0][1].body as URLSearchParams + expect(body.get("token_type_hint")).toBe("refresh_token") + expect(body.get("tokenHint")).toBeNull() expect(mockFetch).toHaveBeenCalledWith(🤖 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/actions/providers/tokens/revoke/stateful.test.ts` around lines 581 - 593, Strengthen the request assertion in the token revocation test by inspecting the URLSearchParams body rather than only matching its type. Verify it serializes token_type_hint=refresh_token and does not contain a raw tokenHint parameter, while preserving the existing request expectations.packages/core/test/api/stateful/revokeToken.test.ts (2)
192-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the
oauthAccountEntitypreset instead of re-declaring it 8 times.This identical literal is repeated at Lines 256-267, 312-323, 363-374, 417-428, 471-482, 630-641, 690-701, 767-778 and duplicates the
oauthAccountEntityfixture added inpackages/core/test/presets.ts(Lines 119-130), which this file already imports from.♻️ Proposed refactor
-import { authInstance, jose, oauthCustomService, sessionEntityWithUser } from "`@test/presets.ts`" +import { authInstance, jose, oauthAccountEntity, oauthCustomService, sessionEntityWithUser } from "`@test/presets.ts`"- const getOAuthAccountMock = vi.fn().mockResolvedValue({ - accountId: "account-123", - accessToken: "access-token", - refreshToken: "refresh-token", - idToken: "id-token", - tokenType: "Bearer", - scopes: "scope1 scope2", - issuer: "https://example.com", - accessTokenExpiresAt: new Date(Date.now() + 3600 * 1000), - refreshTokenExpiresAt: new Date(Date.now() + 7200 * 1000), - updatedAt: new Date(), - }) + const getOAuthAccountMock = vi.fn().mockResolvedValue(oauthAccountEntity)🤖 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/revokeToken.test.ts` around lines 192 - 203, The revoke-token tests repeatedly redeclare the same OAuth account fixture instead of using the existing preset. Replace each identical getOAuthAccountMock resolved value in the affected test cases with the imported oauthAccountEntity preset, preserving any test-specific overrides through the established fixture-copy pattern.
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the global Fetch stubs between these tests.
packages/core/vitest.config.tsenablesunstubEnvs, sovi.stubEnv("BASE_URL", ...)is isolated, but it does not restorevi.stubGlobal("fetch", ...)unlessunstubGlobalsis enabled globally. AddingafterEach(() => vi.unstubAllGlobals())in these describe blocks is enough to keepmockResolvedValueOnce/mockRejectedValueOncemocks from leaking into later cases.🤖 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/revokeToken.test.ts` at line 22, Add afterEach cleanup calling vi.unstubAllGlobals() in the describe blocks covering revokeToken tests in packages/core/test/api/stateful/revokeToken.test.ts and packages/core/test/actions/providers/tokens/revoke/stateful.test.ts, so fetch stubs and one-time mock values are reset between tests.
🤖 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 263-268: Update the revokeToken contract and its implementations
to accept an optional skipCSRFCheck parameter matching destroySession, and
ensure each strategy performs its own CSRF validation unless explicitly skipped.
Propagate the parameter through all revokeToken callers while preserving
existing token-revocation behavior.
In `@packages/core/src/api/revokeToken.ts`:
- Around line 54-55: In the catch block of the token-revocation flow, remove the
console.error call and rely on the existing
ctx.logger?.log("OAUTH_ACCESS_TOKEN_ERROR", ...) structured logging instead.
Preserve the current error handling and ensure the error is not emitted through
an unstructured console path.
In `@packages/core/src/session/stateful.ts`:
- Around line 944-948: The revokeToken flow must perform an internal
verifyCSRFToken self-check before mutating OAuth/session state, matching
destroySession’s behavior. Update revokeToken and its related session
interfaces/callers as needed so the validation is always enforced without
relying solely on external callers.
In `@packages/core/src/session/stateless.ts`:
- Around line 334-357: Update revokeToken to perform the same internal CSRF
validation used by destroySession before revoking tokens or clearing the cookie,
reusing the existing verifyCSRFToken flow and preserving the current disconnect
and header behavior.
- Around line 334-352: Update revokeToken so the disconnect=true path skips
cookie verification and OAuth token parsing, allowing local cleanup even with
stale or malformed cookies. Validate provider immediately after resolving
oauth[oauthId], and throw the established UNSUPPORTED_OAUTH_CONFIGURATION error
when it is missing; only verify/parse the cookie and call revokeProviderToken in
the !disconnect branch, removing the non-null assertion.
In `@packages/core/test/actions/providers/tokens/revoke/stateful.test.ts`:
- Around line 410-439: Update the test “handles malformed provider token cookie”
to include the provider access-token cookie,
`__Secure-aura-auth.access_token.oauth-provider`, with a deliberately invalid or
garbage value in the Request Cookie header. Keep the existing assertions
verifying the failed response and that account status is not updated.
In `@packages/core/test/api/stateful/revokeToken.test.ts`:
- Around line 81-114: The test named “throws error when CSRF token is invalid”
does not exercise CSRF rejection because the default configuration skips the
check. Update the test setup around authInstance to explicitly set
skipCSRFCheck: false and assert CSRF_TOKEN_MISMATCH without OAuth lookup, or
rename the test and assertions to reflect the no-CSRF-check behavior; ensure the
test’s title, expected error, and mock-call assertions describe the same path.
---
Nitpick comments:
In `@packages/core/test/actions/providers/tokens/revoke/stateful.test.ts`:
- Around line 581-593: Strengthen the request assertion in the token revocation
test by inspecting the URLSearchParams body rather than only matching its type.
Verify it serializes token_type_hint=refresh_token and does not contain a raw
tokenHint parameter, while preserving the existing request expectations.
In `@packages/core/test/api/stateful/revokeToken.test.ts`:
- Around line 192-203: The revoke-token tests repeatedly redeclare the same
OAuth account fixture instead of using the existing preset. Replace each
identical getOAuthAccountMock resolved value in the affected test cases with the
imported oauthAccountEntity preset, preserving any test-specific overrides
through the established fixture-copy pattern.
- Line 22: Add afterEach cleanup calling vi.unstubAllGlobals() in the describe
blocks covering revokeToken tests in
packages/core/test/api/stateful/revokeToken.test.ts and
packages/core/test/actions/providers/tokens/revoke/stateful.test.ts, so fetch
stubs and one-time mock values are reset between tests.
🪄 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: 97e5b6dd-20d5-4597-88d3-d7ad465b9df7
📒 Files selected for processing (11)
packages/core/src/@types/session.tspackages/core/src/api/revokeToken.tspackages/core/src/session/stateful.tspackages/core/src/session/stateless.tspackages/core/src/shared/errors.tspackages/core/src/shared/logger.tspackages/core/src/shared/utils.tspackages/core/src/shared/utils/revoke-token.tspackages/core/test/actions/providers/tokens/revoke/stateful.test.tspackages/core/test/api/stateful/revokeToken.test.tspackages/core/test/presets.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds support for
revokeToken()in the experimental Stateful session strategy.With this change,
revokeToken()is now fully supported by both available session strategies:The implementation ensures consistent behavior across both strategies, allowing applications to revoke OAuth or OpenID Connect (OIDC) provider tokens regardless of the configured session storage mechanism.
Key Changes
revokeToken().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