feat(settings): support step-up auth in sign-in flow - #20992
Open
StaberindeZA wants to merge 2 commits into
Open
feat(settings): support step-up auth in sign-in flow#20992StaberindeZA wants to merge 2 commits into
StaberindeZA wants to merge 2 commits into
Conversation
Because: * RFC 9470 step-up requires the authorization endpoint to re-challenge when a session doesn't meet the requested authentication level or is older than max_age; max_age was rejected with a 400 and no freshness check existed This commit: * accepts max_age in both /authorization payload schemas * enforces max_age freshness in validateRequestedGrant, with a small leeway so a just-completed challenge (incl. max_age=0) is not looped * throws INSUFFICIENT_ACR_VALUES (errno 170) for both acr_values and max_age failures, the signal the frontend routes to step-up (was 120, which the frontend ignored) * adds unit tests for the acr and max_age paths Closes FXA-12856
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements RFC 9470 step-up authentication support across FxA Settings (frontend) and the auth-server (backend), enabling RPs to request re-challenge via acr_values and/or max_age without forcing password re-entry, and fixing passkey sessions being misrouted into TOTP enrollment.
Changes:
- Frontend: adjusts sign-in navigation for step-up vs Settings-originated AAL upgrades, avoids forcing TOTP enrollment for passkey sessions, and forwards
max_ageto the backend. - Backend: accepts
max_agein/authorizationpayload schemas and enforces session freshness invalidateRequestedGrantwith a small leeway to avoid challenge loops. - Error signaling: uses
errno 170 (INSUFFICIENT_ACR_VALUES)for ACR/max_age failures so the frontend step-up handlers trigger correctly; client typing updated to includemax_age.
React review (fxa-check-react)
No React/TSX issues found in the changed TSX file.
Clean categories: Component Design & Hooks, Performance, Accessibility (a11y), State Management, Testing Patterns (React/TSX)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signin/utils.ts | Updates navigation logic for step-up vs Settings AAL upgrades and prevents passkey sessions from being forced into TOTP setup. |
| packages/fxa-settings/src/pages/Signin/utils.test.ts | Adds/updates routing tests covering passkey AAL2 behavior and step-up vs Settings-originated AAL upgrades. |
| packages/fxa-settings/src/models/integrations/oauth-web-integration.ts | Stops treating max_age=0 as a password-prompt trigger; keeps prompt=login as the only password re-entry signal. |
| packages/fxa-settings/src/models/integrations/oauth-web-integration.test.ts | Adds coverage for updated wantsLogin() behavior around max_age. |
| packages/fxa-settings/src/lib/oauth/hooks.tsx | Forwards max_age to auth-server when creating OAuth codes. |
| packages/fxa-auth-server/test/remote/oauth_api.in.spec.ts | Updates expectations to errno 170 and the new message for insufficient ACR values. |
| packages/fxa-auth-server/lib/routes/oauth/authorization.js | Extends Joi validation schemas to accept max_age. |
| packages/fxa-auth-server/lib/oauth/grant.spec.ts | Updates ACR mismatch assertions to errno 170 and adds max_age freshness/leeway tests. |
| packages/fxa-auth-server/lib/oauth/grant.js | Enforces max_age freshness with leeway and switches ACR failures to AppError.insufficientACRValues (errno 170). |
| packages/fxa-auth-server/docs/swagger/shared/descriptions.ts | Documents the new maxAge swagger description for max_age. |
| packages/fxa-auth-client/lib/client.ts | Adds max_age typing and forwards it in createOAuthCode request options. |
Suppressed comments (1)
packages/fxa-settings/src/pages/Signin/utils.test.ts:702
- This assertion can miss a call to navigate('/settings', { ... }) because it only checks for the exact single-arg call shape. Consider asserting based on the first argument across all calls so the test remains correct if navigate options are added later.
expect(mockNavigate).not.toHaveBeenCalledWith('/settings');
Comment on lines
+603
to
+606
| expect(mockNavigate).not.toHaveBeenCalledWith( | ||
| '/inline_totp_setup?client_id=abc', | ||
| expect.objectContaining({ replace: true }) | ||
| expect.anything() | ||
| ); |
Because: * an RP requesting step-up (acr_values=AAL2 / max_age) must be challenged for a second factor without prompting for a password, and passkey users were wrongly forced into TOTP enrollment they don't need This commit: * decouples max_age from the password prompt and forwards max_age to the backend so it can enforce authentication freshness * does not force TOTP enrollment for passkey sessions (already AAL2) Closes #FXA-12861
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.
Because
This pull request
Issue that this pull request solves
Closes: FXA-12856, FXA-12861
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.