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 handling of token refresh while offline #89200

Closed
RMacfarlane opened this issue Jan 23, 2020 · 3 comments
Closed

Improve handling of token refresh while offline #89200

RMacfarlane opened this issue Jan 23, 2020 · 3 comments
Assignees
Labels
authentication Issues with the Authentication platform feature-request Request for new features or functionality verified Verification succeeded
Milestone

Comments

@RMacfarlane
Copy link
Contributor

Issue Type: Bug

Currently, the built in account extension will attempt to refresh the access token before it expires. If it fails, it deletes the stored refresh token and sends a change event to core that no sessions are available.

If the user goes offline after signing in, then refresh will fail due to the lack of connection. (Or in @sbatten's case, changing networks also seems to cause refresh to fail). When this happens, it's confusing to show a badge on the gear to sign in again. One way we could handle this is for the extension to still remove the session that now has an expired token, but keep the refresh token if the error may be temporary, and retry again a bit later. In the sessions change event, the core could test to see if the user is online before updating the UI, and would need to tell the sync service to stop.

@sandy081 Thoughts?

VS Code version: Code - Insiders 1.42.0-insider (c4d53a1, 2020-01-23T05:38:55.065Z)
OS version: Darwin x64 18.7.0

@RMacfarlane RMacfarlane added bug Issue identified by VS Code Team member as probable bug authentication Issues with the Authentication platform labels Jan 23, 2020
@RMacfarlane RMacfarlane added this to the January 2020 milestone Jan 23, 2020
@RMacfarlane RMacfarlane self-assigned this Jan 23, 2020
@sandy081
Copy link
Member

in the sessions change event, the core could test to see if the user is online before updating the UI, and would need to tell the sync service to stop.

Not sure if I understand this. How can core could test this? It might be that this could be different for different auth providers.

IMO all this logic should be in the extension and extension should signal core with proper events.

Looks related to #88577 (comment)

@RMacfarlane
Copy link
Contributor Author

RMacfarlane commented Feb 7, 2020

OK, I've just made a change to add an "Unavailable" status for auth in the userDataSync workbench contribution. Now, when refresh fails due to a network error, it will retain the refresh token, but remove the access token from the session and send a change event. This sets the "Unavailable" status while the extension continues retrying up to 3 times to refresh. The auth token gets set to undefined in the shared process.

Still pending:

  • What do we show in the gear menu when auth status is "Unavailable"?
  • Need to add another long timeout to the extension to check for reconnection

@RMacfarlane
Copy link
Contributor Author

Added a 30 minute poll when refreshing fails to check if the network is available again. b98f647

Since #89538 tracks the offline UX, this is complete

@RMacfarlane RMacfarlane added feature-request Request for new features or functionality verification-needed Verification of issue is requested and removed bug Issue identified by VS Code Team member as probable bug labels Feb 7, 2020
@connor4312 connor4312 added verified Verification succeeded and removed verification-needed Verification of issue is requested labels Feb 26, 2020
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
authentication Issues with the Authentication platform feature-request Request for new features or functionality verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants