fix(signin): Fix flaky vpn signin functional tests#20543
Conversation
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
from
204d9de to
76e1354
Compare
dschom
changed the title
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
3 times, most recently
from
75d701f to
860f9cf
Compare
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
from
860f9cf to
0473650
Compare
| sendFxaLogin(navigationOptions); | ||
| } | ||
|
|
||
| // Non-OAuth path: send Login then navigate. |
There was a problem hiding this comment.
Not sure this is right - isOAuth is going to be true if the integration is OAuthNative (oauth desktop Sync, or any other service flow). The only non-oauth integration that should send web channel messages is the old FxDesktopV3. Haven't tested this locally though.
edit: I should have seen that further down in the isOAuth check, you've got this chunk of code there too and you left the comment 👍 . Can we add a comment about this only being for fx_desktop_v3? We should be able to remove it later when we don't care to support fx_desktop_v3 any longer.
LZoog
left a comment
LZoog
left a comment
There was a problem hiding this comment.
This looks good, thank you for investigating!! r+, but just noting, I didn't test the flows locally. Our functional tests have a desktop v3 test though so might be good to verify there's a test there that sends fxa_login if we don't have one already but I know we have coverage for login + oauth so LGTM.
| // IMPORTANT: run /oauth/authorization (HTTP) BEFORE firing any web | ||
| // channel messages. firefox.fxaLogin/fxaOAuthLogin dispatch synchronously | ||
| // but the browser processes them asynchronously, and the browser's | ||
| // handler for fxaLogin can mutate session state in ways that race with |
There was a problem hiding this comment.
Just wondering, what ways does it mutate the session state?
| // "unconfirmed session" error. The only exception to this is the type C | ||
| // unverified session noted above. | ||
| if (isOAuth) { | ||
| // IMPORTANT: run /oauth/authorization (HTTP) BEFORE firing any web |
There was a problem hiding this comment.
I feel like if I'm looking at this file later, I might be confused by what run /oauth/authorization means? It makes me think of the VPN test and us navigating to /authorization there but I think we're just talking about doing the oauth dance/getting the oauth data back.
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
8 times, most recently
from
98f5d19 to
32d465c
Compare
dschom
changed the title
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
2 times, most recently
from
9d6b748 to
8f2d42a
Compare
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
from
8f2d42a to
c066cf4
Compare
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
5 times, most recently
from
4dc3b5b to
42c01f8
Compare
Because: * In handleNavigation's verified-OAuth path, sendFxaLogin (the fxaccounts:login webchannel event) was dispatched synchronously BEFORE awaiting getOAuthNavigationTarget (the /v1/oauth/authorization HTTP call). The browser's async handler for that webchannel message can mutate session state, racing with the in-flight HTTP request. * This produced intermittent 401 "Token not found" failures on the HTTP call — most reproducibly when the same browser context goes through two consecutive OAuth flows (e.g. the vpn integration test signing into Sync then VPN). The first /oauth/authorization succeeded; the second one would 401 even though the session remained valid for /account/profile, /session/status, and /oauth/token in between. This commit: * Reorders handleNavigation so getOAuthNavigationTarget runs first, obtaining the OAuth code while the session is still untouched, and THEN fires fxaLogin and fxaOAuthLogin. The Login → OAuthLogin order is preserved (still required for Desktop OAuth per FXA-10388). * Adds a v1-account regression test alongside the existing vpn integration test, using target.createAuthClient(1) so the v1→v2 upgrade path is exercised on the first sign-in even after testAccountTracker defaults to v2 (separate PR). closes FXA-13687
dschom
force-pushed
the
worktree-fix-flaky-vpn-test
branch
from
42c01f8 to
da4534b
Compare
|
@dschom Feel like haven't seen this issue in a while, can we merge or close? |
4 participants
Because
vpn integration > authorization flow - user already signed into Firefoxwas flaking with a 401 on the second/v1/oauth/authorization.firefox.fxaLogin's async WebChannel handler mutates session state mid-flight, racing with a later OAuth flow's/v1/oauth/authorizationHTTP call.This pull request
NavigationOptions.isCachedSignin, set insignInWithCachedAccount.handleNavigation, defersfxaLoginuntil aftergetOAuthNavigationTargetonly whenisCachedSignin && isOAuth. First-flow (password) signins keep the original ordering — pairing'spair_authorizehandler relies on Firefox seeingfxaLoginfirst, and the previous blanket-reorder attempt deterministically brokepairing/pairingFlow.spec.ts.Companion: #20556 (testAccountTracker → v2; independent).
Issue that this pull request solves
Closes: FXA-13687
Checklist
Put an
xin the boxes that apply