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

API support for multiple AuthenticationSessions with the same scopes. #152399

Closed
philliphoff opened this issue Jun 16, 2022 · 11 comments · Fixed by #224877
Closed

API support for multiple AuthenticationSessions with the same scopes. #152399

philliphoff opened this issue Jun 16, 2022 · 11 comments · Fixed by #224877
Assignees
Labels
api api-finalization authentication Issues with the Authentication platform feature-request Request for new features or functionality on-testplan
Milestone

Comments

@philliphoff
Copy link

Currently VS Code provides an API (vscode.authentication.getSession()) that allows extensions to get an AuthenticationSession for a given set of scopes. Depending on the options passed it, if there are no existing sessions, VS Code may prompt the user to sign in. If there are more than one session with those scopes, VS Code may prompt the user to select which one to use. At the end of the day, however, the extension is returned at most one AuthenticationSession that corresponds to a single account.

In some cases extensions may want to know about all the AuthenticationSession instances associated with a specific set of scopes, in order to provide multi-account experiences or to have its own UX for choosing which account is used in a specific setting (where VS Code's current selection UX might not be appropriate). For example, a tree view that shows assets associated with multiple accounts rather than requiring the user to constantly switch between accounts.

What I propose is a new function, getSessions() with similar semantics as that same method on the AuthenticationProvider. If matching sessions exist, all will be returned. If none exist, an empty array is returned. Calling the function will not result in any user prompts (to select between accounts or to sign in) aside, perhaps, from the initial "do you allow this extension to access these accounts" prompt.

declare module 'vscode' {
    export namespace authentication {
        export function getSessions(providerId: string, scopes: readonly string[]): Thenable<readonly AuthenticationSession[]>;
    }
}
@TylerLeonhardt TylerLeonhardt added feature-request Request for new features or functionality api authentication Issues with the Authentication platform labels Jun 17, 2022
@TylerLeonhardt TylerLeonhardt added this to the Backlog milestone Jun 17, 2022
@TylerLeonhardt TylerLeonhardt modified the milestones: Backlog, March 2023 Mar 14, 2023
TylerLeonhardt added a commit that referenced this issue Mar 14, 2023
TylerLeonhardt added a commit that referenced this issue Mar 14, 2023
* Initial getSessions API

ref #152399

* include provider id in key
@TylerLeonhardt TylerLeonhardt modified the milestones: March 2023, April 2023 Mar 20, 2023
@TylerLeonhardt
Copy link
Member

I'm merging #177263 into this... In the end, here's what we'll do:

  • Update vscode.d.ts here to define this { detail: string } as:
export interface AuthenticationForceNewSessionOptions {
	detail: string;
}
  • Add to the getSessions proposed API file... to make it:
	/**
	 * Options used in {@link authentication.getSession} to be used when forcing the recreation of an {@link AuthenticationSession}.
	 */
	export interface AuthenticationForceNewSessionOptions {
		readonly session?: AuthenticationSession;
	}

	export namespace authentication {
		/**
		 * Get all authentication sessions matching the desired scopes that this extension has access to. In order to request access,
		 * use {@link getSession}. To request an additional account, specify {@link AuthenticationGetSessionOptions.clearSessionPreference}
		 * and {@link AuthenticationGetSessionOptions.createIfNone} together.
		 *
		 * Currently, there are only two authentication providers that are contributed from built in extensions
		 * to the editor that implement GitHub and Microsoft authentication: their providerId's are 'github' and 'microsoft'.
		 *
		 * @param providerId The id of the provider to use
		 * @param scopes A list of scopes representing the permissions requested. These are dependent on the authentication provider
		 * @returns A thenable that resolves to a readonly array of authentication sessions.
		 */
		export function getSessions(providerId: string, scopes: readonly string[]): Thenable<readonly AuthenticationSession[]>;
	}

@TylerLeonhardt
Copy link
Member

One thing left to be talked about is how the AuthProvider receives the session that needs to be recreated. There are a couple options:

  • AuthenticationProvider needs to explicitly implement a recreateSession(...)
  • AuthenticationProvider#createSession(...) takes in a 2nd parameter:
createSession(scopes: readonly string[], options?: { session: AuthenticationSession }): Thenable<AuthenticationSession>;

TylerLeonhardt added a commit that referenced this issue Apr 17, 2023
This will allow you to have workspace level preferences of accounts. The ideal flow is that an extension can ask for the preferred session (silently) first, to not annoy the user... and if the extension/user wants to use a different account, the extension can ask to clear the session preference and the user will select the account they want to use.

Also serializes the adding of accounts in the menu which allows every account shows up instead of just the last account (a bug I noticed doing this work)

ref #152399
TylerLeonhardt added a commit that referenced this issue Apr 17, 2023
This will allow you to have workspace level preferences of accounts. The ideal flow is that an extension can ask for the preferred session (silently) first, to not annoy the user... and if the extension/user wants to use a different account, the extension can ask to clear the session preference and the user will select the account they want to use.

Also serializes the adding of accounts in the menu which allows every account shows up instead of just the last account (a bug I noticed doing this work)

ref #152399
@philliphoff
Copy link
Author

@bwateratmsft and @alexweininger can hopefully provide feedback on the overall plan, as I'm not as involved in VS Code extension development these days.

Of the two recreate session options, I think it depends on whether we really mean "re-creation" or "replacement". If the former, where the session is essentially the same but just "refreshed", then a separate recreateSession() function feels better. If the latter, such as if the user needs to replace a session with another having different scopes, then reusing the createSession() function feels better. (That said, I think I'd want to use a more descriptive parameter name, such as oldSession, previousSession, existingSession, etc. to make it clear it's being replaced.)

@TylerLeonhardt
Copy link
Member

API Sync likes the 2nd bullet, and I agree that maybe existingSession is a more descriptive parameter.

@TylerLeonhardt
Copy link
Member

Got some other ideas... this would help manipulate the session preference so that forcing a new session would "just work".

export interface AuthenticationGetSessionOptions {
  clearSessionPreference?: boolean | AuthenticationSession;
}

or

export interface AuthenticationGetSessionOptions {
  clearSessionPreference?: boolean | { newSessionPreference: AuthenticationSession };
}

There's also this, but I'm not the biggest fan.

export interface AuthenticationGetSessionOptions {
  silent?: boolean | AuthenticationSession;
}

Also, it has come to my attention that even today, the AuthProvider is not informed as to which session is the preferred session... in other words, which one to re-create... so we still need:

createSession(scopes: readonly string[], options?: { session: AuthenticationSession }): Thenable<AuthenticationSession>;

@bwateratmsft
Copy link
Contributor

the AuthProvider is not informed as to which session is the preferred session... in other words, which one to re-create...

Isn't calling getSession with a specific AuthenticationSession passed (by one of those means above) into clearSessionPreference the answer to that question?

@TylerLeonhardt
Copy link
Member

Not fully. If no session is passed in to clearSessionPreference, then the preferred session still needs to be passed in to the auth provider's createSession call so the auth provider knows what session needs to be recreated

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Jun 3, 2024

Revisiting this... I have a new proposal. I have not liked how getSessions coupled accounts and scopes so closely together. This has exposed issues like #213947

Instead, it would be better to separate accounts and the scopes... so that means the following proposal:

declare module 'vscode' {
	// FOR THE CONSUMER

	export namespace authentication {
		/**
		 * Get all accounts that the user is logged in to for the specified provider.
		 * Use this paired with {@link getSession} in order to get an authentication session for a specific account.
		 *
		 * Currently, there are only two authentication providers that are contributed from built in extensions
		 * to the editor that implement GitHub and Microsoft authentication: their providerId's are 'github' and 'microsoft'.
		 *
		 * Note: Getting accounts does not imply that your extension has access to that account or its authentication sessions. You can verify access to the account by calling {@link getSession}.
		 *  
 		 * @param providerId The id of the provider to use
 		 * @returns A thenable that resolves to a readonly array of authentication accounts.
 		 */
		export function getAccounts(providerId: string): Thenable<readonly AuthenticationSessionAccountInformation[]>;
	}

	export interface AuthenticationGetSessionOptions {
		/**
	 	* The account that you would like to get a session for. This is passed down to the Authentication Provider to be used for creating the correct session. 
	 	*/
		account?: AuthenticationSessionAccountInformation;
	}

	// FOR THE AUTH PROVIDER

	export interface AuthenticationProviderSessionOptions {
		/**
		 * The account that is being asked about. If this is passed in, the provider should
		 * attempt to return the sessions that are only related to this account.
		 */
		account?: AuthenticationSessionAccountInformation;
	}

	export interface AuthenticationProvider {
		/**
		 * Get a list of sessions.
		 * @param scopes An optional list of scopes. If provided, the sessions returned should match
		 * these permissions, otherwise all sessions should be returned.
		 * @param options Additional options for getting sessions.
		 * @returns A promise that resolves to an array of authentication sessions.
		 */
		getSessions(scopes: readonly string[], options: AuthenticationProviderSessionOptions): Thenable<AuthenticationSession>;
		/**
		 * Prompts a user to login.
		 *
		 * If login is successful, the onDidChangeSessions event should be fired.
		 *
		 * If login fails, a rejected promise should be returned.
		 *
		 * If the provider has specified that it does not support multiple accounts,
		 * then this should never be called if there is already an existing session matching these
		 * scopes.
		 * @param scopes A list of scopes, permissions, that the new session should be created with.
		 * @param options Additional options for creating a session.
		 * @returns A promise that resolves to an authentication session.
		 */
		createSession(scopes: readonly string[], options: AuthenticationProviderSessionOptions): Thenable<AuthenticationSession>;
	}
}

This actually follows a pretty established model that MSALjs uses where you get an account and then you pass that account in to get a token for that account.

This also is a nicer fit for multiple GitHub Auth as well and I think that it would be easier to support multiple GitHub accounts that way.

@TylerLeonhardt
Copy link
Member

Moving to July for potential finalization

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization authentication Issues with the Authentication platform feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants