fix(settings,auth): Fix sumo login issue#20601
Conversation
| } else if (!sessionVerified) { | ||
| // we are inferring verification method and verification reason here | ||
| // ideally we would check the server directly to allow for more verification reasons | ||
| } else if (!sessionVerified && mustVerify) { |
There was a problem hiding this comment.
@LZoog All the other changes, support this small update to the front end logic... I'm still pondering if there's a simpler way to achieve this, but basically we should invoking line 176, if the session doesn't actually require verification...
There was a problem hiding this comment.
Smells a bit to me that we are handling these checks client-side. Perhaps we could update /session/resend_code to confirm whether the session still needs verification and return if a code was sent or not, to inform client-side navigation?
There was a problem hiding this comment.
I also considered this and I mostly agree with what you are saying. However, this was deemed a riskier option and it still doesn't entirely clean this up...
The two issues with that approach seem to be:
-
In this case, we would make the call, and then fail. The reality is that we do not need the extra call. So the client side is still do something unnecessary / wrong.
-
The other, probably bigger, concern is that this could lead to scenarios where a user is being prompted by the UI for a code, the do something to invoke
session.resendCode. We then start erroring on the server side, and the user is now stuck.
I guess we've had enough issues around verified session states, that I'm a little hesitant to do something that might result in other issue. In someways, I think exposing the account model state to the front end more fully is better than making unnecessary calls and then seeing waiting for errors. It's possible that by exposing this flag to the front end, we can actually make better decisions about what to do. This scenario is actually a good example of that.
The other thing I don't quite get, and maybe this yet another way to address the problem. Why are we invoking resendCode here? Shouldn't that send happen on the page that is asking for the code?
There was a problem hiding this comment.
The server generally handles sending the code. It's not usually triggered by the frontend except here. There must be something else funky that's leading to the exception.
Because: - Adding `mustVerify` to `/session/status` and gating `cachedSignIn` on it broke existing tests that didn't yet know about the new field. This commit: - Adds `mustVerify: false` to the seven `/session/status` assertions in session.spec.ts so they match the handler's new response shape. - Adds `mustVerify: true` to the two `cachedSigninHandler` mocks in container.test.tsx that expect the unverified branch to fire. - Drops the `signinConfirmation.skipForNewAccounts.enabled=false` configOverrides from the remote session_tests suite — under defaults, mustVerify cleanly tracks `wantsKeys`, which is what the keys=true/keys=false round-trip tests are checking. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Because: - The whole point of FXA-12972 is to stop sending verifyLoginCode emails to non-Sync OAuth RPs (e.g. SUMO) when the auth-server has decided the session does not need verification. Without a test pinning this behavior, a future regression would be silent. This commit: - Adds a cachedSigninHandler test asserting that with mustVerify=false and sessionVerified=false, no verification code is sent and verificationMethod/verificationReason stay undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fy expectations Because: - The previous attempt removed the configOverrides block to make the keys=true/keys=false round-trip tests deterministic under defaults. That broke the existing 'reauth has sane session-verification behaviour' tests (which depend on the forced-confirmation behaviour to keep new-account sessions in the 'unverified' state) and the keys=true case (which under defaults gets pre-verified and never sets mustVerify=true). - The keys=false test as authored is unworkable under either config: with the override, every new login sets mustVerify=true regardless of the keys argument; without it, new accounts skip signin confirmation entirely and mustVerify stays false but the session is also auto-verified. This commit: - Restores the signinConfirmation.skipForNewAccounts.enabled=false configOverrides block, with a comment explaining why. - Updates 'succeeds with valid token' to expect mustVerify=true (which is what the server actually returns when signin confirmation is forced). - Replaces the keys=true/keys=false pair with a single test that exposes the mustVerify field for a session that requires verification. The falsy case is already covered by the unit tests in lib/routes/session.spec.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dschom
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes an FxA Settings sign-in regression where cachedSignIn would resend verification codes (and strand users on the verification page) whenever sessionVerified=false, even when the auth-server had determined the session did not require verification (mustVerify=false, e.g., SUMO-style OAuth RPs). It does this by exposing mustVerify via /session/status and gating the resend/verification routing behavior on that flag, with accompanying unit + remote integration tests.
Changes:
- Expose
mustVerifyonGET /session/status(auth-server response schema + handler return) and add it to the auth-clientSessionStatustype. - Update
fxa-settingscachedSignInto only route/send verification codes for unverified sessions whenmustVerify=true. - Add/extend tests in
fxa-settingsandfxa-auth-serverto cover the new behavior and ensure deterministic auth-server session-state behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signin/utils.ts | Gate the unverified-session verification flow on mustVerify to prevent unnecessary resend + verification routing. |
| packages/fxa-settings/src/pages/Signin/utils.test.ts | Add unit coverage for cachedSignIn, including mustVerify branches and error handling. |
| packages/fxa-settings/src/pages/Signin/container.test.tsx | Add a regression test ensuring unverified + mustVerify=false skips verification/resend. |
| packages/fxa-settings/src/models/mocks.tsx | Update auth client mock sessionStatus details to include mustVerify. |
| packages/fxa-auth-server/test/remote/session_tests.in.spec.ts | Make session-state deterministic via config override and assert mustVerify is exposed as expected. |
| packages/fxa-auth-server/lib/routes/session.spec.ts | Update /session/status route unit test expectations to include mustVerify. |
| packages/fxa-auth-server/lib/routes/session.js | Add mustVerify to /session/status response schema and handler output. |
| packages/fxa-auth-client/lib/client.ts | Extend SessionStatus type with mustVerify. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Because
verifyLoginCodeemails on signin even though the auth-server had correctly decided the session did not need verification (mustVerify=false).cachedSignInin fxa-settings firingPOST /session/resend_codebased solely onsessionVerified=falseflag and ignoring themustVerifyflag.mustVerifyguard that fxa-content-server still had atviews/base.js#checkAuthorization.This pull request
mustVerifyon theGET /session/statusresponse (auth-server Joi schema + handler return; auth-clientSessionStatustype).cachedSignIn(packages/fxa-settings/src/pages/Signin/utils.ts) onmustVerify=true, restoring parity with the legacy content-server guard. WhenmustVerify=false, no resend fires andverificationReason/verificationMethodstay undefined so the user routes past the verification page instead of being stranded on it.cachedSignIn(none existed): both branches, TOTP active, OAuthprompt=none, already-verified sessions, and theSESSION_EXPIREDerror path.packages/fxa-auth-server/test/remote/session_tests.in.spec.tsto round-tripmustVerifyforkeys=trueandkeys=false, and configures that suite to start the auth-server withsigninConfirmation.skipForNewAccounts.enabled=falseso the new assertions are deterministic.Issue that this pull request solves
Closes: FXA-12972
Checklist
Put an
xin the boxes that applyHow to review (Optional)
packages/fxa-settings/src/pages/Signin/utils.ts— the front end fix (one new&& mustVerifyguard).packages/fxa-auth-server/lib/routes/session.js— the new field on/session/status.Other information
Tracking this bug down was a little twisty, so a full diagnosis of problem is attached to FXA-12972
Also worth noting, that we tried to create a smoke-test to cover this edge case but could not, since the state hinged on the
signinConfirmation.skipForNewAccountsoption which is set to 48 hours currently. Becuase this time lapse is quite large, there's no really a good way to trip this state in a functional test. The interactions between these flags and options (mustVerify, sessionVerified, accountVerified, lockedAt, skipForNewAccounts, etc...) is pretty complex, so it'd be worth while clearly demarcating these states.The unit-level coverage proves the fix; however, in order to get functional test coverage we need either a pre-provisioned long-lived fixture account on each environment or an admin-API path to alter an existing account state. A follow spike was added to look into how to make get better test coverage around this.