Skip to content
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

Improve the settings sync flows with authentication #94766

Closed
sandy081 opened this issue Apr 9, 2020 · 6 comments · Fixed by #95438
Closed

Improve the settings sync flows with authentication #94766

sandy081 opened this issue Apr 9, 2020 · 6 comments · Fixed by #95438
Assignees
Labels
authentication Issues with the Authentication platform on-testplan polish Cleanup and polish issue settings-sync ux User experience issues
Milestone

Comments

@sandy081
Copy link
Member

sandy081 commented Apr 9, 2020

Improve the settings sync flows with authentication

Sign in flow

Signing in showing too many notifications and not consistent. Sign in from Accounts UI and I see following dialogs

a. For the first time ever

image

image

b. Afterwards

image

image

Sign out flow

Signing out is showing following notification

image

There should be a sign in button instead of Turn on. Because user did not turn off sync here. It shall be same as the action button that is shown in the gear icon

image

Above notification is also shown even when settings sync is turned off

Also do we need successfully sign out notification here?

Error flow

Errors while signing in

Following notifications are shown which are not helpful

image

Cross Window Flow

Sign in/Sign out from one window is prompting turn on/turn off sync notification on another window

#94176

  • Open two windows with sync enabled
  • Sign out from one window and you get a turn off notification in that window
  • But it is not expected to get the same notification in another window
  • Now sign in from one window and you get following notification in another window

image

Others

@sandy081
Copy link
Member Author

@RMacfarlane I am seeing the following dialog even if sync is turned off

image

@RMacfarlane
Copy link
Contributor

Ah, yeah, the problem is that I don't have a clear signal of when a token is no longer being used. I track that something has read a token for an account during the current session. Would changing the dialog text to the more accurate text The account x has been used during this session by: be better?

@RMacfarlane
Copy link
Contributor

RMacfarlane commented Apr 16, 2020

The more involved fix would be to return both a value and a dispose method that can be called when using getAccessToken(), such that you could call dispose when you no longer care about the access token. This is cumbersome because I then have to keep track of each individual token instead of access at a session level, and seems a little weird for other consumers of the API. Normally if I'm using auth, I would just call getAccessToken() every time I'm about to make a request.

@sandy081
Copy link
Member Author

Is it possible to logout only for a given session?

@RMacfarlane
Copy link
Contributor

Yeah, that's possible.

However, I'm not sure we want to do that when turning off sync - I don't think I would expect to sign out when turning off sync. Since multiple extensions can read the same session, you potentially stop something else from working by removing that session. I previously made a change to enforce that each session could only be read by one extension, but then undid it because that makes a single sign on impossible. Maybe I could reference count and only really do the logout when nothing depends on the session anymore 🤔

@sandy081
Copy link
Member Author

sandy081 commented Apr 17, 2020

I see. Makes sense. Thanks for explanation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
authentication Issues with the Authentication platform on-testplan polish Cleanup and polish issue settings-sync ux User experience issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants