-
Notifications
You must be signed in to change notification settings - Fork 222
bug(settings): Abandoning sync sign flow can cause down stream problems #20098
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ const CONFIGS = { | |
| }, | ||
| }; | ||
|
|
||
| const isNightly = /Nightly/gi.test(process.env.FIREFOX_BIN || ''); | ||
| const env = process.env.FXA_ENV || 'local'; | ||
| const FXA_DESKTOP_CONTEXT = | ||
| process.env.FXA_DESKTOP_CONTEXT || 'oauth_webchannel_v1'; | ||
|
|
@@ -65,7 +66,14 @@ if (!fxaEnv) { | |
| }; | ||
| } | ||
|
|
||
| const nightlyProfile = isNightly | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, in about a month we can have this on Release too (it's supposed to be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, when testing I had to restart nightly alot. On successful sign in with smart window, smart window is then 'signed in', and I actually am not sure if there's a way to 'sign out' other than restarting.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you currently have to switch to classic and then sign out from there. It seems to be ever-evolving hence my "I think" lol. |
||
| ? { | ||
| 'browser.smartwindow.enabled': true, | ||
| } | ||
| : {}; | ||
|
|
||
| const fxaProfile = { | ||
| ...nightlyProfile, | ||
| // enable debugger and toolbox | ||
| 'devtools.chrome.enabled': true, | ||
| 'devtools.debugger.remote-enabled': true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -221,7 +221,13 @@ export const App = ({ | |
| integration.data?.service || '' | ||
| ); | ||
|
|
||
| if (userFromBrowser?.sessionToken) { | ||
| // Don't intialize session state from partially succesful firefox logins (ie sync, relay, smartwindow). | ||
| // This reprensents an abandonded login. Basically the user hasn't actually confirmed the session yet, so | ||
| // don't assume it's valid. | ||
| if ( | ||
| userFromBrowser?.sessionToken && | ||
| userFromBrowser.verified === true | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ) { | ||
| const isValidSession = await session.isValid( | ||
| userFromBrowser.sessionToken | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,9 +131,10 @@ export class Session implements SessionData { | |
|
|
||
| async isValid(token: string): Promise<boolean> { | ||
| try { | ||
| await this.authClient.sessionStatus(token); | ||
| setSessionVerified(true); | ||
| return true; | ||
| const resp = await this.authClient.sessionStatus(token); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a big problem. It was updating the cache such that the session was always. 'verified' when in fact it very well might not have been. When I finally fixed this, everything else fell into place. |
||
| const isVerified = resp.state === 'verified'; | ||
| setSessionVerified(isVerified); | ||
| return isVerified; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aha, this totally makes sense given what we did at the GQL level with this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was bug was driving me crazy... I couldn't wrap my head around why the session would magically become verified 😅. I wouldn't doubt this will fix several other issues... since it was very common to get a false positive on session verified state.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah man, I just mentioned this in Slack, but I just realized now that |
||
| } catch (e) { | ||
| setSessionVerified(false); | ||
| return false; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -513,17 +513,33 @@ const getOAuthNavigationTarget = async ( | |
| ); | ||
| if (error) { | ||
| if ( | ||
| error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno || | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This state is definitely possible, and like the other two state indicates we need a confirmation code step. It happens when the session is in 'mustVerify' mode. |
||
| error.errno === AuthUiErrors.TOTP_REQUIRED.errno || | ||
| error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno | ||
| ) { | ||
| GleanMetrics.login.error({ event: { reason: error.message } }); | ||
| // If user already has TOTP enabled, send them to enter their code instead of setup | ||
| const hasTotp = | ||
| locationState.verificationMethod === VerificationMethods.TOTP_2FA; | ||
|
|
||
| const to = (() => { | ||
| // If the user doesn't have totp, and encountered an unverified_session error, send them | ||
| // to signin_token_code, this is a 'mustVerify' case | ||
| if ( | ||
| locationState.verificationMethod !== VerificationMethods.TOTP_2FA && | ||
| error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno | ||
| ) { | ||
| return `/signin_token_code${navigationOptions.queryParams || ''}`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file just gets hairer and hairier 😅 (Does this send the email also?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we should rethink how we do this... Using errors like this for flow control isn't ideal. |
||
| } | ||
|
|
||
| // If user already has TOTP enabled, send them to enter their code instead of setup | ||
| if (locationState.verificationMethod === VerificationMethods.TOTP_2FA) { | ||
| return `/signin_totp_code${navigationOptions.queryParams || ''}`; | ||
| } | ||
|
|
||
| // Otherwise, we are dealing with an RP that requires totp setup. Send them there. | ||
| return `/inline_totp_setup${navigationOptions.queryParams || ''}`; | ||
| })(); | ||
|
|
||
| return { | ||
| to: hasTotp | ||
| ? `/signin_totp_code${navigationOptions.queryParams || ''}` | ||
| : `/inline_totp_setup${navigationOptions.queryParams || ''}`, | ||
| to, | ||
| locationState, | ||
| }; | ||
| } | ||
|
|
||
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.
Just noting, this tests the Sync flow from over a year ago / non-oauth. I think we have multiples of these around so maybe we file a ticket to adjust our tests?