feat: SDKCore - implement startLogin() - authorization code grant start (ENG-4786)#203
Open
mrudatsprint wants to merge 12 commits into
Open
feat: SDKCore - implement startLogin() - authorization code grant start (ENG-4786)#203mrudatsprint wants to merge 12 commits into
mrudatsprint wants to merge 12 commits into
Conversation
…t (ENG-4786) - Add Pkce module (generateCodeVerifier, generateCodeChallenge) with RFC 7636 Appendix B test vector coverage; runs under @vitest-environment node - Extend RedirectHelper to persist code_verifier as a second colon-delimited segment alongside state; add public getCodeVerifier() getter; add test file - SDKCore: construct DPoPManager when config.useDpop is true; startLogin() is now async — DPoP branch calls getOrCreateKeyPair()/getThumbprint() and generates PKCE params then redirects to /oauth2/authorize directly; isLoggedIn delegates to DPoPManager.isLoggedIn in DPoP mode (not app.at_exp cookie) - SDKCore.test.ts: add DPoP-mode describe block with mocked DPoPManager and Pkce (jsdom lacks crypto.subtle); all existing cookie-mode tests unaffected - e2e/dpop-smoke.test.ts: replace local generatePkce() helper with shared Pkce module; add Tier 0 tests exercising SDKCore.startLogin() in DPoP mode end-to-end (no live FusionAuth required for Tier 0) - Export Pkce from packages/core/src/index.ts Note: yarn test:core cannot run in this sandbox environment due to a missing @rollup/rollup-linux-arm64-gnu native binary (arch mismatch); TypeScript compilation (tsc --noEmit) and ESLint/Prettier are clean.
…edirect assertion Without the explicit jsdom annotation, vitest inherits the 'node' environment from DPoPManager.test.ts when the full suite runs, causing 'document is not defined' and 'window is not defined' failures in all SDKCore tests. Also corrects the handlePreRedirect spy assertion: cookie-mode startLogin() passes one argument (state), not two — the codeVerifier arg is only added in DPoP mode.
SDKCore's constructor calls scheduleTokenExpiration() which calls
getAccessTokenExpirationMoment(). In a Node/Playwright process document
doesn't exist, so CookieHelpers catches the ReferenceError and logs
'Error accessing cookies...' to console.error. The tests still pass, but the
stderr noise is confusing.
Fix: extract a shared DPOP_CONFIG constant in the Tier 0 describe block that
includes a no-op cookieAdapter ({ at_exp: () => undefined }). This causes
getAccessTokenExpirationMoment() to take the adapter path and skip
document.cookie entirely, eliminating the noise.
Also fixes T0-1 where the await core.startLogin() call was accidentally
dropped during the previous config refactor.
mrudatsprint
changed the base branch from
main
to
miker/eng-4784/central-coordinator
July 18, 2026 21:18
mrudatsprint
changed the title
…ormat RedirectHelper now stores nonce:codeVerifier:state (three colon-delimited segments) instead of the previous nonce:state (two segments). The Angular sdkcore/ directory is generated by 'yarn get-sdk-core' which copies packages/core/src/ verbatim — so in CI the Angular RedirectHelper picks up the updated parser automatically. The test was writing the old two-segment format 'abc123:/welcome-page', which the new parser splits as [nonce='abc123', codeVerifier='/welcome-page', state=''] — returning undefined for state instead of '/welcome-page'. Fix: write 'abc123::/welcome-page' (empty codeVerifier segment, matching cookie mode where no verifier is stored).
…value format
RedirectHelper now stores nonce:codeVerifier:state (three colon-delimited
segments) instead of nonce:state (two segments). Both sdk-vue and sdk-react
import SDKCore directly from @fusionauth-sdk/core (via the @fusionauth-sdk/*
tsconfig path alias), so their tests exercise the live, current
RedirectHelper — same root cause as the earlier Angular fix.
- packages/sdk-vue/src/createFusionAuth/createFusionAuth.test.ts: was seeding
the old 2-segment format ('rAnd0mStR1ng:<state>'), causing the new state
getter to return undefined instead of the expected state value. Fixed to
'rAnd0mStR1ng::<state>' (empty codeVerifier segment).
- packages/sdk-react/src/components/providers/FusionAuthProvider.test.tsx:
had the same stale 2-segment seed, but wasn't caught by CI because the
assertion only checked toHaveBeenCalled() (no argument check). Fixed the
seed format and strengthened the assertion to toHaveBeenCalledWith(stateValue)
to restore real coverage of the callback argument.
FusionAuth (as the Authorization Server) never issues a use_dpop_nonce
challenge itself — per FusionAuth's DPoP docs, nonce enforcement is a
Resource Server responsibility implemented by your own APIs, not something
FusionAuth's own endpoints (e.g. /oauth2/userinfo) do. This is why the
existing T2-3 test can only assert structurally ('either outcome is a pass')
against a real FusionAuth instance.
T2-4 adds a self-contained, deterministic test that mocks globalThis.fetch
to simulate a Resource Server 401 response with a use_dpop_nonce challenge
(WWW-Authenticate + DPoP-Nonce headers), then verifies:
- exactly one retry occurs (not zero, not more than one)
- the first proof has no nonce claim
- the retried proof carries the exact server-issued nonce claim
- both proofs target the same htu/htm
Uses its own fresh DPoPManager (via the existing makeManager() helper) so it
does not depend on shared state/order from the Tier 1 tests, and requires no
live FusionAuth instance.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the initial SDKCore-side wiring for starting an OAuth2 Authorization Code + PKCE flow when DPoP mode is enabled, including generating/persisting the PKCE verifier and redirecting directly to FusionAuth’s /oauth2/authorize with dpop_jkt and PKCE parameters. The PR also updates redirect-state storage format and adjusts unit/e2e tests accordingly across the supported framework SDKs.
Changes:
- Add
SDKCore.startLogin()DPoP-mode path: generate/load DPoP keypair, computedpop_jkt, generate PKCE verifier/challenge, persist verifier, and redirect to/oauth2/authorize. - Introduce shared PKCE utilities (
Pkce) with RFC test vectors and update DPoP e2e smoke tests to use them. - Update
RedirectHelperlocalStorage format to include an (optional) PKCE verifier and add unit tests; adjust React/Vue/Angular tests for the new format.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-vue/src/createFusionAuth/createFusionAuth.test.ts | Updates redirect-value test fixture to new nonce:verifier:state format. |
| packages/sdk-react/src/components/providers/FusionAuthProvider.test.tsx | Updates redirect-value format and tightens onRedirect expectation. |
| packages/sdk-angular/projects/fusionauth-angular-sdk/src/lib/fusion-auth.service.spec.ts | Updates redirect-value test fixture to new format. |
| packages/core/src/SDKCore/SDKCore.ts | Implements DPoP-mode startLogin() and routes isLoggedIn through DPoPManager when enabled. |
| packages/core/src/SDKCore/SDKCore.test.ts | Adds/updates tests for DPoP-mode startLogin() URL shape + verifier persistence; adds jsdom annotation rationale. |
| packages/core/src/RedirectHelper/RedirectHelper.ts | Extends redirect marker storage format to include PKCE verifier; adds getCodeVerifier(); updates state parsing. |
| packages/core/src/RedirectHelper/RedirectHelper.test.ts | Adds unit coverage for new redirect storage format and verifier retrieval. |
| packages/core/src/Pkce/Pkce.ts | Adds PKCE verifier/challenge generation utilities. |
| packages/core/src/Pkce/Pkce.test.ts | Adds RFC 7636 test-vector coverage for PKCE implementation (node env). |
| packages/core/src/Pkce/index.ts | Exports PKCE utilities from package barrel. |
| packages/core/src/index.ts | Re-exports Pkce from core package root. |
| e2e/tests/dpop-smoke.test.ts | Integrates SDKCore.startLogin() into Tier 0 smoke tests and refactors existing tiers to use shared PKCE utilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ENG-4786) Reverts SDKCore.startLogin()'s signature from 'async ... Promise<void>' back to plain 'void', matching the public SDKContext/framework-wrapper types exactly (SDKContext.ts, FusionAuthProviderContext.ts, Vue's FusionAuth<T>, Angular's SDKContext.ts all still declare startLogin: (state?) => void). Although tsc --noEmit already reported zero errors thanks to TypeScript's void-returning-function compatibility rule, the underlying concern was real: none of the three framework wrappers (React's useRedirecting, Vue's login(), Angular's startLogin()) awaited or caught the promise, so a DPoP async failure (e.g. crypto.subtle unavailable, IndexedDB blocked) would surface as an unhandled promise rejection. - SDKCore.ts: startLogin() is synchronous again. In DPoP mode it fires a new private async startDpopLogin() and catches failures via the new optional SDKConfig.onLoginFailure callback (falls back to console.error), following the existing onAutoRefreshFailure convention. Cookie mode is unchanged. - SDKConfig.ts: add onLoginFailure?: (error: Error) => void. - SDKCore.test.ts: DPoP startLogin() tests now call startLogin() without awaiting it and use vi.waitFor() to wait for window.location.assign before asserting. Added two new tests covering onLoginFailure and the console.error fallback. - e2e/tests/dpop-smoke.test.ts: added a createAssignWaiter() helper (a deferred promise resolved when window.location.assign is called) and reworked T0-1/T0-2/T0-3 to use it instead of awaiting startLogin() directly. T0-3 now explicitly waits for core1's redirect before swapping IndexedDB for core2, preserving the original sequential-completion guarantee that awaiting startLogin() used to provide implicitly. No changes needed to SDKContext.ts, FusionAuthProviderContext.ts, Vue's types, Angular's types/service, or any framework wrapper implementation — zero blast radius outside packages/core, as intended.
…tHelper.ts Addresses a Copilot PR review comment. The class-level doc comment said state is retrieved by joining segments 'after index 1 (skipping the verifier segment)' — phrasing left over from before the codeVerifier segment existed. The storage format is nonce:codeVerifier:state (3 segments), and the actual implementation (line 85) skips both the nonce (index 0) and codeVerifier (index 1) segments, with state starting at index 2 — not just 'the verifier segment' as the old wording implied. Doc-only change; no logic or test changes needed.
… review) RedirectHelper.state and getCodeVerifier() always assumed the current 3-segment storage format (nonce:codeVerifier:state). If a user initiates a login redirect on a pre-DPoP SDK version (which wrote the legacy 2-segment nonce:state format) and the app is upgraded to a newer SDK version before they land back — e.g. a deploy that happens while they're on FusionAuth's hosted login page — the leftover legacy value would be misparsed: state would resolve to undefined instead of the real value. Fix: detect the legacy format unambiguously. The current writer (handlePreRedirect) always includes a codeVerifier segment, even when empty, so any value it produces has at least two colons. A stored value with exactly one colon can therefore only be the legacy format. - state getter: if there are exactly 2 segments (1 colon), treat the second segment as the legacy state directly, instead of destructuring past index 1 (which only works for the 3-segment format). - getCodeVerifier(): same legacy-format guard, since a 2-segment value never carried a code_verifier — prevents misreading a fragment of a legacy state value as a verifier. - Documented (as a comment, not a test) the known acceptable limitation: a legacy state value that itself contained a colon is indistinguishable from a current-format value with a non-empty codeVerifier — an inherent ambiguity in a delimiter-based format without a version marker, accepted given the narrow redirect-round-trip window. - RedirectHelper.test.ts: added 4 tests seeding localStorage directly with the legacy format, covering handlePostRedirect's callback value (including empty legacy state), marker cleanup, and getCodeVerifier()'s undefined result.
mrudatsprint
commented
Jul 21, 2026
mrudatsprint
left a comment
mrudatsprint
left a comment
Collaborator
Author
There was a problem hiding this comment.
self-review
| @@ -1,17 +1,20 @@ | |||
| /** | |||
| * DPoP Smoke Tests — pre-SDKCore wiring | |||
| * DPoP Smoke Tests — pre-SDKCore wiring + SDKCore.startLogin() integration | |||
Collaborator
Author
There was a problem hiding this comment.
When DPoP is fully implemented this file will be deleted and new end to end tests will be added to endpoints.test.ts
This file is a way to test the current state of the DPoP implementation with a live FusionAuth server.
| * A class responsible for storing pre-redirect values in localStorage and | ||
| * cleaning them up afterward. | ||
| * | ||
| * Storage format: `${randomNonce}:${codeVerifier ?? ''}:${state ?? ''}` |
Collaborator
Author
There was a problem hiding this comment.
Before DPoP mode was added the Storage format was ${randomNonce}:${state ?? ''}
Therefore, backwards compatibility is being handled.
| * `startLogin()` itself can remain synchronous (`void`) while still | ||
| * performing the necessary async key-pair/PKCE work before redirecting. | ||
| */ | ||
| private async startDpopLogin(state?: string): Promise<void> { |
Collaborator
Author
There was a problem hiding this comment.
Yes, the authorization code grant flow is started. The code will be exchanged in the next PR.
mrudatsprint
marked this pull request as ready for review
July 21, 2026 17:14
mrudatsprint
changed the title
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
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.
Issue:
Description:
The start of implementing the Authorization Code Grant with PKCE when DPoP mode is enabled:
/oauth2/authorizeappendingdpop_jktand PKCEcode challenge