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

Should there be something like AuthProvider.getSession(args) #91546

Closed
jrieken opened this issue Feb 26, 2020 · 5 comments
Closed

Should there be something like AuthProvider.getSession(args) #91546

jrieken opened this issue Feb 26, 2020 · 5 comments
Assignees
Labels
authentication Issues with the Authentication platform under-discussion Issue is under discussion for relevance, priority, approach

Comments

@jrieken
Copy link
Member

jrieken commented Feb 26, 2020

re #91309

The test plan item defines getCachedSesssionOrLogin, my sample extension does something similar so that makes me think if there should be just something like AuthenticationProvider.getSession(scopes:string[]) which (a) picks a matching session if available or (b) does the login-dance. What's the use-case for having the sessions-array?

@jrieken
Copy link
Member Author

jrieken commented Feb 26, 2020

On another look I would also question why providers are exposed and why we don't have a function on the namespace.

export namespace authentication {
  
  export function registerAuthenticationProvider(provider: AuthenticationProvider): Disposable;

  export const onDidChangeAuthenticationProviders: Event<AuthenticationProvidersChangeEvent>;

  export function getSession(providerId:string, scopes:string[]) : Thenable<Session>;
}

This would be much more in line with how file system providers work - with them you register a file system for a scheme and consumers/clients go through vscode.workspace.fs without knowing with what provider they are dealing with

@RMacfarlane
Copy link
Contributor

What's the use-case for having the sessions-array?

Settings sync uses this on start up. If sync is on, it checks if there is an available session and uses it. If not, it shows an indicator on the gear for the user to sign in, waiting for the user to take action instead of popping open a browser window immediately.

If we move to exposing a function instead of providers, we also need to expose login, logout, and the session change event. I had moved to that model at one point, but then changed back because it was very verbose.

@RMacfarlane RMacfarlane added authentication Issues with the Authentication platform under-discussion Issue is under discussion for relevance, priority, approach labels Feb 26, 2020
@jrieken
Copy link
Member Author

jrieken commented Feb 27, 2020

Settings sync uses this on start up. If sync is on, it checks if there is an available session and uses it. If not, it shows an indicator on the gear for the user to sign in,

Which means there is a need for function hasSession(providerId:string, scopes), right?

I had moved to that model at one point, but then changed back because it was very verbose.

Not sure I understand - it cannot be more than what the providers have and we have the chance to add utils around the provider so that extensions don't need to write boiler plate code (like finding a session)

we also need to expose login, logout

Wanted to ask about logout specifically? What's the use-case for extensions? I understand that a provider needs to implement this but why does an extension need to call it (and what does it mean for other consumers of a session)? I was assuming that I, as a user, can trigger logout but that this isn't the business of a third extension.

@RMacfarlane
Copy link
Contributor

Which means there is a need for function hasSession(providerId:string, scopes), right?

Yes, correct.

The argument for moving back to only exposing "providers" was something along the lines that it was simplifying/cleaner, it created less API. I don't really have strong opinions either way, I just would like to decide on something and stop switching it back and forth :)

I agree about logout, I can't think of an actual use case for this.

@RMacfarlane
Copy link
Contributor

Closing since we have finalized on a single getSession with a flag for controlling if auth should be performed if no session is found

@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 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 under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

2 participants