fix(oauth-desktop): Call fxaLogin before fxaOAuthLogin with expected data during signin #17744
Conversation
LZoog
force-pushed
the
FXA-10388
branch
2 times, most recently
from
f4d2e8c
to
a3e7068
Compare
…data during signin Because: * We want to support the new oauth desktop flow, and there is a bug during signin because we were sending fxaOAuthLogin first This commit: * Refactors signin/utils to swap the order these web channel messages fire * Fixes an additional intermittent bug for 2FA and new account signins caused by sending keyFetchToken and unwrapBKey * Adds oauth desktop config * Renames oauthCode to oauthData in oauth hook fixes FXA-10388
| @@ -295,7 +295,7 @@ | |||
| "id": "5882386c6d801776", | |||
| "hashedSecret": "71b5283536f1f1c331eca2f75c58a5947d7a7ac54164eadb4b33a889afe89fbf", | |||
| "imageUri": "", | |||
| "redirectUri": "urn:ietf:wg:oauth:2.0:oob:oauth-redirect-webchannel", | |||
| "redirectUri": "urn:ietf:wg:oauth:2.0:oob:oauth-redirect-webchannel,http://localhost:3030/oauth/success/5882386c6d801776", | |||
There was a problem hiding this comment.
Non-blocking question: Just want to verify if it intentional to also include this localhost as well even though this is the dev.json since we've had oauth desktop supported before and we hadn't needed this then.
| @@ -12,6 +12,8 @@ Available options: | |||
| - `FIREFOX_DEBUGGER=true` - open the [Browser Toolbox](https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox) on start (NOTE: `false` by default for speed). | |||
| - `FXA_DESKTOP_CONTEXT` - context value for the fxa-content-server: `context=[value]` (NOTE: `fx_desktop_v3` is default). | |||
|
|
|||
| Tip: run `FXA_DESKTOP_CONTEXT=oauth_webchannel_v1 yarn firefox` to test the OAuth Desktop flow. | |||
There was a problem hiding this comment.
Thanks for updating this doc!
Boring nit: you can turn this into a fancy tip if you'd like: https://github.com/orgs/community/discussions/16925
😄
Tip
More useful information here.
There was a problem hiding this comment.
Tip
😱 Thanks for the tip!
| // TODO in FXA-9872, make oauth_webchannel_v1 the default context and | ||
| // change these values for fx_desktop_v3. (Also, update the README note) |
There was a problem hiding this comment.
Kudos for useful comments that point to tasks to do and docs to update. 👌
| // NOTE, Oauth desktop needs to add `service=sync` as a query parameter for this | ||
| // to take users to the inline recovery key flow (SYNC-4408). (We may want | ||
| // check for client ID to determine oauth desktop instead, TBD slight refactor for | ||
| // FXA-10313). |
There was a problem hiding this comment.
For my own sanity: we had checked the source offline for app services and saw that service=sync is not used there for consumers of the oauth fxaclient (currently only our mobile browsers). However, I forgot to check desktop separately as well, which has a bunch of tests for fx_desktop_v3 which still exist: https://searchfox.org/mozilla-central/search?q=service%3Dsync&path=&case=false®exp=false
| @@ -160,7 +160,10 @@ const SigninTokenCode = ({ | |||
|
|
|||
| await GleanMetrics.isDone(); | |||
|
|
|||
| const { error: navError } = await handleNavigation(navigationOptions); | |||
| const { error: navError } = await handleNavigation(navigationOptions, { | |||
| handleFxaLogin: false, | |||
There was a problem hiding this comment.
Non-blocking question: why does the signin token not have handleFxaLogin enabled?
| @@ -7,7 +7,7 @@ import Signin from '.'; | |||
| import VerificationMethods from '../../constants/verification-methods'; | |||
| import VerificationReasons from '../../constants/verification-reasons'; | |||
| import { MozServices } from '../../lib/types'; | |||
| import { AppContext, IntegrationType } from '../../models'; | |||
| import { AppContext, Integration, IntegrationType } from '../../models'; | |||
There was a problem hiding this comment.
General feedback about the file, not this change in particular: this file seems to be the meat of the changes and the refactoring looks good to me, but you might want keener eyes.
There are complex code paths here that are core to how signin would work, but I don't see tests that cover all these branches. Do we have code coverage to gauge how much our testing covers? It seems it would be easy to break multiple things if I change this one file IIUC?
Because:
This commit:
fixes FXA-10388
Most of this was already accounted for in page-level tests. I'm going to be taking a better look at our test coverage in FXA-10328 next.
Test this by running
yarn firefoxfor our desktop v3 flow andFXA_DESKTOP_CONTEXT=oauth_webchannel_v1 yarn firefoxfor the oauth desktop flow. Check out a standard Sync signin (requires email code), a new Sync account signin (no code), and 2FA signin. Users should be fully logged into Sync.