Skip to content
This repository has been archived by the owner. It is now read-only.

"Manage Account" and sign-in confirmation interact badly #4252

Closed
rfk opened this issue Oct 7, 2016 · 17 comments
Closed

"Manage Account" and sign-in confirmation interact badly #4252

rfk opened this issue Oct 7, 2016 · 17 comments

Comments

@rfk
Copy link
Member

@rfk rfk commented Oct 7, 2016

(Making a stand-alone issue for this, is was reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1308038 and also independently by @mhammond)

STR:

  1. Sign in to sync in a browser
  2. Simulate a loss of localStorage state by going to https://accounts.firefox.com and using the web console to execute localStorage.clear()
  3. Go to about:preferences#sync and click "Manage Account". This will open a new tab pointing to https://accounts.firefox.com/settings
  4. Re-enter your password to sign back in to web content (since it doesn't have a sessionToken in localStorage any more).

Expected result:

After entering my password, I am taken to the Firefox Accounts settings page, while my browser continues syncing away happily.

Actual result:

I get the sign-in confirmation screen. My browser starts using the new sessionToken I just generated by logging in, and finds it can no longer sync because the sessionToken is not verified. When I click through the email to verify the session, I am taken to the "session confirmed" success page rather than the settings page.

Video demonstration: https://rfk.id.au/static/scratch/manage_account.mov

@rfk
Copy link
Member Author

@rfk rfk commented Oct 7, 2016

My browser starts using the new sessionToken I just generated by logging in,
and finds it can no longer sync because the sessionToken is not verified.

@mhammond had browser logs from when this happened, and was able to confirm that we send a "fxaccounts:login" webchannel message as a result of the login at step (4), which causes the browser to disconnect from sync and re-connect with the new, unverified sessionToken.

@rfk
Copy link
Member Author

@rfk rfk commented Oct 7, 2016

There are a lot of things we could do to make this interaction better, in decreasing order of closeness-to-what-the-user-expects:

  1. Avoid having to log in again the first place, by using the sessionToken that's held by the browser. This would require cooperation from the browser so it's no good for a short-term fix.
  2. Avoid having to do sign-in confirmation again. I'm not sure we can do that reliably, except as a special case of more general "have we seen this IP address before?" rules.
  3. Not send the "fxaccounts:login" webchannel message, so that the browser's syncing doesn't get disrupted. This probably has some downsides w.r.t. coordination between web content and browser though.
  4. Take the user to the settings page like they asked, rather than the sign-in success page.

I wonder whether (3) and/or (4) are simple enough to do as a short-term fix here?

@mhammond
Copy link
Member

@mhammond mhammond commented Oct 7, 2016

The other part of this that is really bad UX from the user's POV is that Sync was working correctly, but simply clicking manage and verifying your password breaks Sync until the verification flow completes. At a stretch, the user might expect they can't use the "manage" facilities until verification, but not that the process would cause Sync to stop working.

@rfk
Copy link
Member Author

@rfk rfk commented Oct 7, 2016

  1. Simulate a loss of localStorage state by going to https://accounts.firefox.com
    and using the web console to execute localStorage.clear()

Also, it's not clear how Mark or the bugzilla reporter got themselves into a situation where the web content didn't have a sessionToken, but AFAWCT they did.

@mhammond
Copy link
Member

@mhammond mhammond commented Oct 7, 2016

Clearing all history is one way to reproduce this - I'm not sure which of the checkboxes is actually relevant, but selecting them all reproduces the problem.

@splewako
Copy link

@splewako splewako commented Oct 21, 2016

Simulate a loss of localStorage state by going to https://accounts.firefox.com and using the web console to execute localStorage.clear()

Also, it's not clear how Mark or the bugzilla reporter got themselves into a situation where the web content didn't have a sessionToken, but AFAWCT they did.

Hi, I'm bug 1308038 reporter. Don't know (didn't test yet) but could custom cookie settings possibly affect this? (no third party, allow for session)

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 21, 2016

(no third party, allow for session)

Allow for session might be the root cause, testing to see.

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 21, 2016

@splewako - do you have Firefox set to clear your history on Firefox close?

I'm wondering if it's the first or the second (or other) here:

screen shot 2016-10-21 at 11 10 59

screen shot 2016-10-21 at 11 11 12

@splewako
Copy link

@splewako splewako commented Oct 21, 2016

do you have Firefox set to clear your history on Firefox close?

No, my settings look like on the first image.

@splewako
Copy link

@splewako splewako commented Oct 21, 2016

On the other hand, there was some sort of history clearing almost for sure (manual, selective, via addon) since I enabled sync for the first time.

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 21, 2016

I can confirm with @splewako's settings (the first image above), restarting Firefox and then clicking "Manage Account" then entering the password causes Firefox to stop syncing. :/

Thanks for help tracking this down @splewako!

@rfk
Copy link
Member Author

@rfk rfk commented Oct 24, 2016

@shane-tomlinson so I understand, is the theory here that "keep until I close Firefox" causes the localStorage state to be discarded when shutting down Firefox, and hence forces you to re-log-in when accessing account settings on the web?

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 24, 2016

is the theory here that "keep until I close Firefox" causes the localStorage state to be discarded when shutting down Firefox, and hence forces you to re-log-in when accessing account settings on the web?

Yes.

@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Nov 16, 2016

from mtg:

Still not sure what is going on here.... not 100%....

possible ideas:

  • manage account sends the session token to web context and content-server recovers it's localstorage state.
  • sync component in firefox checks local storage for the content server and writes to it if sync is logged in but content server is not (complex idea, maybe insane?)
  • webchannel handshake
  • never send logout even, ONLY when you are singing in as a new user.

Why do we even have logout?
How is signing confirmation working with this?

@rfk
Copy link
Member Author

@rfk rfk commented Nov 21, 2016

manage account sends the session token to web context and
content-server recovers it's localstorage state.

Having slept on this a little, there may be something simple we can do here that will help this flow, without getting too "interesting" from a secrets-management standpoint. Strawman:

  • When linking out to web content, the browser includes is sessionToken id in the URL hash fragment, like https://accounts.firefox.com/settings?service=sync&context=fx_desktop_v3#sessionTokenId=abcdef1234567890
  • Web content checks this against what's in localStorage. If such a sessionToken does not exist, then it has somehow got out of sync with the browser's login state and needs to recover.
  • The simplest recovery strategy would be to just prompt the user to re-login like we do right now, but to pass along the sessionTokenId as an extra parameter, and to use that to bypass the signin confirmation step.

This would not be a complete fix, but it could be a step in the right direction.

I wonder if we can get any metrics on how often this is happening to users in practice...

shane-tomlinson pushed a commit that referenced this issue Feb 2, 2017
Perform a handshake with the browser to use the browser as the canonical
source of truth when it comes to signed in accounts.

issue #4252
https://bugzilla.mozilla.org/show_bug.cgi?id=1308038
shane-tomlinson pushed a commit that referenced this issue Mar 2, 2017
Perform a handshake with the browser to use the browser as the canonical
source of truth when it comes to signed in accounts. A new broker, the fx_desktop_v4
broker has been added so that we only attempt the handshake in browsers with
support.

fixes #4252
https://bugzilla.mozilla.org/show_bug.cgi?id=1308038
shane-tomlinson pushed a commit that referenced this issue Mar 3, 2017
Perform a handshake with the browser to use the browser as the canonical
source of truth when it comes to signed in accounts. A new broker, the fx_desktop_v4
broker has been added so that we only attempt the handshake in browsers with
support.

fixes #4252
https://bugzilla.mozilla.org/show_bug.cgi?id=1308038
shane-tomlinson pushed a commit that referenced this issue Mar 3, 2017
Perform a handshake with the browser to use the browser as the canonical
source of truth when it comes to signed in accounts. A new broker, the fx_desktop_v4
broker has been added so that we only attempt the handshake in browsers with
support.

fixes #4252
https://bugzilla.mozilla.org/show_bug.cgi?id=1308038
shane-tomlinson pushed a commit that referenced this issue Mar 3, 2017
Perform a handshake with the browser to use the browser as the canonical
source of truth when it comes to signed in accounts. A new broker, the fx_desktop_v4
broker has been added so that we only attempt the handshake in browsers with
support.

fixes #4252
https://bugzilla.mozilla.org/show_bug.cgi?id=1308038
shane-tomlinson pushed a commit that referenced this issue May 24, 2017
The move is so these mixins can be used by any objects. This is an extraction
from #4695.

issue #4252
shane-tomlinson pushed a commit that referenced this issue May 24, 2017
This is an extraction from #4695
issue #4252
shane-tomlinson pushed a commit that referenced this issue May 24, 2017
The move is so these mixins can be used by any objects. This is an extraction
from #4695.

issue #4252
vladikoff added a commit that referenced this issue May 24, 2017
…dikoff

This is an extraction from #4695
issue #4252
vladikoff added a commit that referenced this issue May 24, 2017
#5099) r=vladikoff

The move is so these mixins can be used by any objects. This is an extraction
from #4695.

issue #4252
@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Jun 19, 2017

This should be fixed with #4695 and Firefox 55, closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants