Reworks authentication for integrations

Adds modal prompt when disconnecting integrations
Adds sign out option to internal integration auth providers
Moves auth provider registration/lifecyle into Container

src/container.ts

@@ -26,6 +26,7 @@ import {
 import { Commands } from './constants';
 import { GitFileSystemProvider } from './git/fsProvider';
 import { GitProviderService } from './git/gitProviderService';
+import { GitLabAuthenticationProvider } from './git/remotes/gitlab';
 import { LineHoverController } from './hovers/lineHoverController';
 import { Keyboard } from './keyboard';
 import { Logger } from './logger';
@@ -204,6 +205,7 @@ export class Container {
 		context.subscriptions.push((this._timelineView = new TimelineWebviewView(this)));
 
 		context.subscriptions.push((this._integrationAuthentication = new IntegrationAuthenticationService(this)));
+		context.subscriptions.push(new GitLabAuthenticationProvider(this));
 
 		if (config.terminalLinks.enabled) {
 			context.subscriptions.push((this._terminalLinks = new GitTerminalLinkProvider(this)));
@@ -389,7 +391,9 @@ export class Container {
 
 	private async _loadGitLabApi() {
 		try {
-			return new (await import(/* webpackChunkName: "gitlab" */ './plus/gitlab/gitlab')).GitLabApi();
+			const gitlab = new (await import(/* webpackChunkName: "gitlab" */ './plus/gitlab/gitlab')).GitLabApi(this);
+			this.context.subscriptions.push(gitlab);
+			return gitlab;
 		} catch (ex) {
 			Logger.error(ex);
 			return undefined;

src/git/remotes/gitlab.ts

@@ -1,4 +1,4 @@
-import { AuthenticationSession, Range, Uri, window } from 'vscode';
+import { AuthenticationSession, Disposable, Range, Uri, window } from 'vscode';
 import type { Autolink, DynamicAutolinkReference } from '../../annotations/autolinks';
 import { AutolinkReference, AutolinkType } from '../../config';
 import { Container } from '../../container';
@@ -28,18 +28,12 @@ const rangeRegex = /^L(\d+)(?:-(\d+))?$/;
 const authProvider = Object.freeze({ id: 'gitlab', scopes: ['read_api', 'read_user', 'read_repository'] });
 
 export class GitLabRemote extends RichRemoteProvider {
-	private static authenticationProvider: GitLabAuthenticationProvider | undefined;
-
 	protected get authProvider() {
 		return authProvider;
 	}
 
 	constructor(domain: string, path: string, protocol?: string, name?: string, custom: boolean = false) {
 		super(domain, path, protocol, name, custom);
-
-		if (GitLabRemote.authenticationProvider == null) {
-			GitLabRemote.authenticationProvider = new GitLabAuthenticationProvider(Container.instance);
-		}
 	}
 
 	get apiBaseUrl() {
@@ -225,29 +219,29 @@ export class GitLabRemote extends RichRemoteProvider {
 		return super.connect();
 	}
 
-	@log()
-	override disconnect(silent: boolean = false): void {
-		super.disconnect(silent);
-
-		if (silent) return;
-
-		async function promptToClearAuthentication(this: GitLabRemote) {
-			const clear = { title: 'Clear Authentication' };
-			const cancel = { title: 'Cancel', isCloseAffordance: true };
-			const result = await window.showWarningMessage(
-				`Rich integration with GitLab as been disconnected.\n\nDo you also want to clear your saved authentication?`,
-				{ modal: true },
-				clear,
-				cancel,
-			);
-
-			if (result === clear) {
-				void Container.instance.integrationAuthentication.deleteSession(this.id, this.authDescriptor);
-			}
-		}
-
-		void promptToClearAuthentication.call(this);
-	}
+	// @log()
+	// override disconnect(silent: boolean = false): void {
+	// 	super.disconnect(silent);
+
+	// 	if (!silent) {
+	// 		async function promptToClearAuthentication(this: GitLabRemote) {
+	// 			const clear = { title: 'Clear Authentication' };
+	// 			const cancel = { title: 'Cancel', isCloseAffordance: true };
+	// 			const result = await window.showWarningMessage(
+	// 				`Rich integration with GitLab as been disconnected.\n\nDo you also want to clear your saved authentication?`,
+	// 				{ modal: true },
+	// 				clear,
+	// 				cancel,
+	// 			);
+
+	// 			if (result === clear) {
+	// 				void Container.instance.integrationAuthentication.deleteSession(this.id, this.authDescriptor);
+	// 			}
+	// 		}
+
+	// 		void promptToClearAuthentication.call(this);
+	// 	}
+	// }
 
 	async getLocalInfoFromRemoteUri(
 		repository: Repository,
@@ -425,9 +419,15 @@ export class GitLabRemote extends RichRemoteProvider {
 	}
 }
 
-class GitLabAuthenticationProvider implements IntegrationAuthenticationProvider {
+export class GitLabAuthenticationProvider implements Disposable, IntegrationAuthenticationProvider {
+	private readonly _disposable: Disposable;
+
 	constructor(container: Container) {
-		container.context.subscriptions.push(container.integrationAuthentication.registerProvider('gitlab', this));
+		this._disposable = container.integrationAuthentication.registerProvider('gitlab', this);
+	}
+
+	dispose() {
+		this._disposable.dispose();
 	}
 
 	getSessionId(descriptor?: IntegrationAuthenticationSessionDescriptor): string {

src/git/remotes/provider.ts

@@ -5,8 +5,10 @@ import {
 	env,
 	Event,
 	EventEmitter,
+	MessageItem,
 	Range,
 	Uri,
+	window,
 } from 'vscode';
 import { DynamicAutolinkReference } from '../../annotations/autolinks';
 import { AutolinkReference } from '../../config';
@@ -295,7 +297,7 @@ export abstract class RichRemoteProvider extends RemoteProvider {
 				if (e.key !== this.key) return;
 
 				if (e.reason === 'disconnected') {
-					this.disconnect(true);
+					void this.disconnect(true);
 				} else if (e.reason === 'connected') {
 					void this.ensureSession(false);
 				}
@@ -304,12 +306,11 @@ export abstract class RichRemoteProvider extends RemoteProvider {
 		);
 	}
 
-	protected get authDescriptor(): IntegrationAuthenticationSessionDescriptor {
-		return { domain: this.domain, scopes: this.authProvider.scopes };
-	}
-
 	abstract get apiBaseUrl(): string;
 	protected abstract get authProvider(): { id: string; scopes: string[] };
+	protected get authProviderDescriptor(): IntegrationAuthenticationSessionDescriptor {
+		return { domain: this.domain, scopes: this.authProvider.scopes };
+	}
 
 	private get key() {
 		return this.custom ? `${this.name}:${this.domain}` : this.name;
@@ -350,15 +351,46 @@ export abstract class RichRemoteProvider extends RemoteProvider {
 	}
 
 	@log()
-	disconnect(silent: boolean = false): void {
+	async disconnect(silent: boolean = false): Promise<void> {
 		const connected = this._session != null;
 
+		const container = Container.instance;
+
+		if (connected && !silent) {
+			const disable = { title: 'Disable' };
+			const signout = { title: 'Disable & Sign Out' };
+			const cancel = { title: 'Cancel', isCloseAffordance: true };
+
+			let result: MessageItem | undefined;
+			if (container.integrationAuthentication.hasProvider(this.authProvider.id)) {
+				result = await window.showWarningMessage(
+					`Are you sure you want to disable the rich integration with ${this.name}?\n\nNote: signing out clears the saved authentication.`,
+					{ modal: true },
+					disable,
+					signout,
+					cancel,
+				);
+			} else {
+				result = await window.showWarningMessage(
+					`Are you sure you want to disable the rich integration with ${this.name}?`,
+					{ modal: true },
+					disable,
+					cancel,
+				);
+			}
+
+			if (result == null || result === cancel) return;
+			if (result === signout) {
+				void Container.instance.integrationAuthentication.deleteSession(this.id, this.authProviderDescriptor);
+			}
+		}
+
 		this.invalidClientExceptionCount = 0;
 		this._prsByCommit.clear();
 		this._session = null;
 
 		if (connected) {
-			void Container.instance.storage.storeWorkspace(this.connectedKey, false);
+			void container.storage.storeWorkspace(this.connectedKey, false);
 
 			this._onDidChange.fire();
 			if (!silent) {
@@ -367,6 +399,14 @@ export abstract class RichRemoteProvider extends RemoteProvider {
 		}
 	}
 
+	@log()
+	async reauthenticate(): Promise<void> {
+		if (this._session === undefined) return;
+
+		this._session = undefined;
+		void (await this.ensureSession(true, true));
+	}
+
 	@gate()
 	@debug<RichRemoteProvider['isConnected']>({
 		exit: connected => `returned ${connected}`,
@@ -578,7 +618,10 @@ export abstract class RichRemoteProvider extends RemoteProvider {
 	): Promise<PullRequest | undefined>;
 
 	@gate()
-	private async ensureSession(createIfNeeded: boolean): Promise<AuthenticationSession | undefined> {
+	private async ensureSession(
+		createIfNeeded: boolean,
+		forceNewSession: boolean = false,
+	): Promise<AuthenticationSession | undefined> {
 		if (this._session != null) return this._session;
 		if (!configuration.get('integrations.enabled')) return undefined;
 
@@ -595,13 +638,14 @@ export abstract class RichRemoteProvider extends RemoteProvider {
 			if (container.integrationAuthentication.hasProvider(this.authProvider.id)) {
 				session = await container.integrationAuthentication.getSession(
 					this.authProvider.id,
-					this.authDescriptor,
-					{ createIfNeeded: createIfNeeded },
+					this.authProviderDescriptor,
+					{ createIfNeeded: createIfNeeded, forceNewSession: forceNewSession },
 				);
 			} else {
 				session = await authentication.getSession(this.authProvider.id, this.authProvider.scopes, {
 					createIfNone: createIfNeeded,
 					silent: !createIfNeeded,
+					forceNewSession: forceNewSession,
 				});
 			}
 		} catch (ex) {
@@ -638,7 +682,7 @@ export abstract class RichRemoteProvider extends RemoteProvider {
 		this.invalidClientExceptionCount++;
 
 		if (this.invalidClientExceptionCount >= 5) {
-			this.disconnect();
+			void this.disconnect();
 		}
 	}
 }

src/plus/gitlab/gitlab.ts

@@ -1,8 +1,9 @@
 import type { HttpsProxyAgent } from 'https-proxy-agent';
-import { Disposable, Event, EventEmitter, Uri, window } from 'vscode';
+import { Disposable, Uri, window } from 'vscode';
 import { fetch, getProxyAgent, RequestInit, Response } from '@env/fetch';
 import { isWeb } from '@env/platform';
 import { configuration, CustomRemoteType } from '../../configuration';
+import type { Container } from '../../container';
 import {
 	AuthenticationError,
 	AuthenticationErrorReason,
@@ -27,15 +28,10 @@ import {
 } from './models';
 
 export class GitLabApi implements Disposable {
-	private readonly _onDidReauthenticate = new EventEmitter<void>();
-	get onDidReauthenticate(): Event<void> {
-		return this._onDidReauthenticate.event;
-	}
-
 	private _disposable: Disposable | undefined;
 	private _projectIds = new Map<string, Promise<string | undefined>>();
 
-	constructor() {
+	constructor(_container: Container) {
 		this._disposable = Disposable.from(
 			configuration.onDidChange(e => {
 				if (configuration.changed(e, 'proxy') || configuration.changed(e, 'remotes')) {
@@ -147,7 +143,7 @@ export class GitLabApi implements Disposable {
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return undefined;
 
-			throw this.handleException(ex, cc);
+			throw this.handleException(ex, provider, cc);
 		}
 	}
 
@@ -178,7 +174,7 @@ export class GitLabApi implements Disposable {
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return undefined;
 
-			throw this.handleException(ex, cc);
+			throw this.handleException(ex, provider, cc);
 		}
 	}
 
@@ -229,7 +225,7 @@ export class GitLabApi implements Disposable {
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return undefined;
 
-			throw this.handleException(ex, cc);
+			throw this.handleException(ex, provider, cc);
 		}
 	}
 
@@ -332,7 +328,7 @@ export class GitLabApi implements Disposable {
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return undefined;
 
-			throw this.handleException(ex, cc);
+			throw this.handleException(ex, provider, cc);
 		}
 	}
 
@@ -472,7 +468,7 @@ export class GitLabApi implements Disposable {
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return undefined;
 
-			throw this.handleException(ex, cc);
+			throw this.handleException(ex, provider, cc);
 		}
 	}
 
@@ -520,7 +516,7 @@ export class GitLabApi implements Disposable {
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return undefined;
 
-			throw this.handleException(ex, cc);
+			throw this.handleException(ex, provider, cc);
 		}
 	}
 
@@ -595,7 +591,7 @@ $search: String!
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return [];
 
-			this.handleException(ex, cc);
+			this.handleException(ex, provider, cc);
 			return [];
 		}
 	}
@@ -658,7 +654,7 @@ $search: String!
 		} catch (ex) {
 			if (ex instanceof ProviderRequestNotFoundError) return undefined;
 
-			this.handleException(ex, cc);
+			this.handleException(ex, provider, cc);
 			return undefined;
 		}
 	}
@@ -830,17 +826,17 @@ $search: String!
 		}
 	}
 
-	private handleException(ex: Error, cc: LogCorrelationContext | undefined): Error {
+	private handleException(ex: Error, provider: RichRemoteProvider, cc: LogCorrelationContext | undefined): Error {
 		Logger.error(ex, cc);
 		debugger;
 
 		if (ex instanceof AuthenticationError) {
-			void this.showAuthenticationErrorMessage(ex);
+			void this.showAuthenticationErrorMessage(ex, provider);
 		}
 		return ex;
 	}
 
-	private async showAuthenticationErrorMessage(ex: AuthenticationError) {
+	private async showAuthenticationErrorMessage(ex: AuthenticationError, provider: RichRemoteProvider) {
 		if (ex.reason === AuthenticationErrorReason.Unauthorized || ex.reason === AuthenticationErrorReason.Forbidden) {
 			const confirm = 'Reauthenticate';
 			const result = await window.showErrorMessage(
@@ -851,7 +847,7 @@ $search: String!
 			);
 
 			if (result === confirm) {
-				this._onDidReauthenticate.fire();
+				await provider.reauthenticate();
 			}
 		} else {
 			void window.showErrorMessage(ex.message);

src/plus/integrationAuthentication.ts

@@ -67,14 +67,18 @@ export class IntegrationAuthenticationService implements Disposable {
 	async getSession(
 		providerId: string,
 		descriptor?: IntegrationAuthenticationSessionDescriptor,
-		options?: { createIfNeeded?: boolean },
+		options?: { createIfNeeded?: boolean; forceNewSession?: boolean },
 	): Promise<AuthenticationSession | undefined> {
 		const provider = this.providers.get(providerId);
 		if (provider == null) throw new Error(`Provider with id ${providerId} not registered`);
 
-		let storedSession: StoredSession | undefined;
-
 		const key = this.getSecretKey(providerId, provider.getSessionId(descriptor));
+
+		if (options?.forceNewSession) {
+			await this.container.storage.deleteSecret(key);
+		}
+
+		let storedSession: StoredSession | undefined;
 		try {
 			const sessionJSON = await this.container.storage.getSecret(key);
 			if (sessionJSON) {