-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(oauth-desktop): Call fxaLogin before fxaOAuthLogin with expected data during signin #17744
Conversation
a3e7068
to
ee4c67e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with mostly optional nits or questions.
I am a bit curious about the future maintainability of some of the logic here and how we could keep that testable as future frontend changes happen. These are just thoughts out loud though. 🙂
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…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
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 firefox
for our desktop v3 flow andFXA_DESKTOP_CONTEXT=oauth_webchannel_v1 yarn firefox
for 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.