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

authentication.onDidChangeSessions should fire when access is revoked by a user #104008

Open
eamodio opened this issue Aug 4, 2020 · 4 comments
Assignees
Labels
authentication Issues with the Authentication platform feature-request Request for new features or functionality under-discussion Issue is under discussion for relevance, priority, approach

Comments

@eamodio
Copy link
Contributor

eamodio commented Aug 4, 2020

Testing #103871

Currently the authentication.onDidChangeSessions doesn't fire if the user revokes an extension's access to a login. I would except that it would fire in that case, or have another event that an extension can listen for.

@RMacfarlane RMacfarlane added feature-request Request for new features or functionality under-discussion Issue is under discussion for relevance, priority, approach authentication Issues with the Authentication platform labels Nov 3, 2020
@TylerLeonhardt
Copy link
Member

Agreed. This is similar to saying "I want to logout for just this extension"

@TylerLeonhardt
Copy link
Member

In the case of the GitHub Pull Request extension, it basically logs in once and then maintains an Octokit object for all future GitHub API calls:

https://github.com/microsoft/vscode-pull-request-github/blob/c9911fffef4fa09330cdda57fc4d130af67636ac/src/github/credentials.ts#L170-L171

there isn't a mechanism, today at least, that would invalidate that access token that was given to the client. It will eventually expire and then calling getSession again will not give you a session if you have unchecked the check box. This is why the GHPR extension continues to work fine after you deny it... but once you reload, it will stop working (I wonder if this is something we should do... similar to Workspace Trust's model of reloading)...

Yeah as I think about it some more I think we may have to reload the window if auth is denied to ensure that an access token is no longer used. This would mean that #104008 would no longer make sense but we can continue this discussion there. Thanks for the feedback!

From #118486

@TylerLeonhardt
Copy link
Member

In standup we talked about this... we decided that an authentication.onDidChangeSessions would be better than restarted the extension host because we want to trust that extensions do the right thing and some extensions will need to clean up correctly when they get "logged out". (revoke access tokens, etc)

@TylerLeonhardt
Copy link
Member

@sbatten mentioned that we could also send an event to the AuthProvider and allow the Auth Provider to revoke a session based on its own logic. Here's the AAD example:
https://docs.microsoft.com/en-us/machine-learning-server/operationalize/how-to-manage-access-tokens#revoke-refresh-tokens

This is probably the most robust way to implement revoking given our platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication Issues with the Authentication platform feature-request Request for new features or functionality under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants