Adds authentication prompt & rejection prompt

Avoids repeated authentication prompts
Persists provider disconnect across reloads

src/constants.ts

@@ -133,6 +133,7 @@ export enum GlyphChars {
 
 export enum GlobalState {
 	Avatars = 'gitlens:avatars',
+	DisallowConnectionPrefix = 'gitlens:disallow:connection:',
 	Version = 'gitlensVersion',
 }
 

src/git/remotes/factory.ts

@@ -1,12 +1,12 @@
 'use strict';
-import { CustomRemoteType, RemotesConfig } from '../../configuration';
-import { Logger } from '../../logger';
 import { AzureDevOpsRemote } from './azure-devops';
 import { BitbucketRemote } from './bitbucket';
 import { BitbucketServerRemote } from './bitbucket-server';
+import { CustomRemoteType, RemotesConfig } from '../../configuration';
 import { CustomRemote } from './custom';
 import { GitHubRemote } from './github';
 import { GitLabRemote } from './gitlab';
+import { Logger } from '../../logger';
 import { RemoteProvider, RemoteProviderWithApi } from './provider';
 
 export { RemoteProvider, RemoteProviderWithApi };

src/git/remotes/github.ts

@@ -4,10 +4,7 @@ import { DynamicAutolinkReference } from '../../annotations/autolinks';
 import { AutolinkReference } from '../../config';
 import { Container } from '../../container';
 import { GitHubPullRequest } from '../../github/github';
-import { IssueOrPullRequest } from '../models/issue';
-import { Account, GitRevision } from '../models/models';
-import { PullRequest, PullRequestState } from '../models/pullRequest';
-import { Repository } from '../models/repository';
+import { Account, GitRevision, IssueOrPullRequest, PullRequest, PullRequestState, Repository } from '../models/models';
 import { RemoteProviderWithApi } from './provider';
 
 const issueEnricher3rdParyRegex = /\b(\w+\\?-?\w+(?!\\?-)\/\w+\\?-?\w+(?!\\?-))\\?#([0-9]+)\b/g;
@@ -155,7 +152,7 @@ export class GitHubRemote extends RemoteProviderWithApi {
 		return `${this.baseUrl}?path=${fileName}${line}`;
 	}
 
-	protected async onGetAccountForCommit(
+	protected async getProviderAccountForCommit(
 		{ accessToken }: AuthenticationSession,
 		ref: string,
 		options?: {
@@ -169,7 +166,7 @@ export class GitHubRemote extends RemoteProviderWithApi {
 		});
 	}
 
-	protected async onGetAccountForEmail(
+	protected async getProviderAccountForEmail(
 		{ accessToken }: AuthenticationSession,
 		email: string,
 		options?: {
@@ -183,7 +180,7 @@ export class GitHubRemote extends RemoteProviderWithApi {
 		});
 	}
 
-	protected async onGetIssueOrPullRequest(
+	protected async getProviderIssueOrPullRequest(
 		{ accessToken }: AuthenticationSession,
 		id: string,
 	): Promise<IssueOrPullRequest | undefined> {
@@ -193,7 +190,7 @@ export class GitHubRemote extends RemoteProviderWithApi {
 		});
 	}
 
-	protected async onGetPullRequestForBranch(
+	protected async getProviderPullRequestForBranch(
 		{ accessToken }: AuthenticationSession,
 		branch: string,
 		options?: {
@@ -212,7 +209,7 @@ export class GitHubRemote extends RemoteProviderWithApi {
 		});
 	}
 
-	protected async onGetPullRequestForCommit(
+	protected async getProviderPullRequestForCommit(
 		{ accessToken }: AuthenticationSession,
 		ref: string,
 	): Promise<PullRequest | undefined> {

src/git/remotes/provider.ts

@@ -6,34 +6,20 @@ import {
 	env,
 	Event,
 	EventEmitter,
+	MessageItem,
 	Range,
 	Uri,
 	window,
 } from 'vscode';
 import { DynamicAutolinkReference } from '../../annotations/autolinks';
 import { AutolinkReference } from '../../config';
+import { GlobalState } from '../../constants';
 import { Container } from '../../container';
 import { Logger } from '../../logger';
 import { Messages } from '../../messages';
 import { Account, GitLogCommit, IssueOrPullRequest, PullRequest, PullRequestState, Repository } from '../models/models';
 import { debug, gate, log, Promises } from '../../system';
 
-const _onDidChangeAuthentication = new EventEmitter<{ reason: 'connected' | 'disconnected'; key: string }>();
-
-export class Authentication {
-	static get onDidChange(): Event<{ reason: 'connected' | 'disconnected'; key: string }> {
-		return _onDidChangeAuthentication.event;
-	}
-}
-
-export class AuthenticationError extends Error {
-	constructor(private original: Error) {
-		super(original.message);
-
-		Error.captureStackTrace(this, AuthenticationError);
-	}
-}
-
 export enum RemoteResourceType {
 	Branch = 'branch',
 	Branches = 'branches',
@@ -216,7 +202,23 @@ export abstract class RemoteProvider {
 	}
 }
 
-// TODO@eamodio revisit how once authed, all remotes are always connected, even after a restart
+const _onDidChangeAuthentication = new EventEmitter<{ reason: 'connected' | 'disconnected'; key: string }>();
+
+export class Authentication {
+	static get onDidChange(): Event<{ reason: 'connected' | 'disconnected'; key: string }> {
+		return _onDidChangeAuthentication.event;
+	}
+}
+
+export class AuthenticationError extends Error {
+	constructor(private original: Error) {
+		super(original.message);
+
+		Error.captureStackTrace(this, AuthenticationError);
+	}
+}
+
+// TODO@eamodio revisit how once authenticated, all remotes are always connected, even after a restart
 
 export abstract class RemoteProviderWithApi extends RemoteProvider {
 	static is(provider: RemoteProvider | undefined): provider is RemoteProviderWithApi {
@@ -248,18 +250,37 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		);
 	}
 
+	abstract get apiBaseUrl(): string;
+	protected abstract get authProvider(): { id: string; scopes: string[] };
+
 	private get key() {
 		return this.custom ? `${this.name}:${this.domain}` : this.name;
 	}
 
+	private get disallowConnectionKey() {
+		return `${GlobalState.DisallowConnectionPrefix}${this.key}`;
+	}
+
+	get maybeConnected(): boolean | undefined {
+		if (this._session === undefined) return undefined;
+
+		return this._session !== null;
+	}
+
+	protected _session: AuthenticationSession | null | undefined;
+	protected session() {
+		if (this._session === undefined) {
+			return this.ensureSession(false);
+		}
+		return this._session ?? undefined;
+	}
+
 	private onAuthenticationSessionsChanged(e: AuthenticationSessionsChangeEvent) {
 		if (e.provider.id === this.authProvider.id) {
 			this.disconnect();
 		}
 	}
 
-	abstract get apiBaseUrl(): string;
-
 	@log()
 	async connect(): Promise<boolean> {
 		try {
@@ -279,6 +300,8 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		this._session = null;
 
 		if (disconnected) {
+			void Container.context.globalState.update(this.disallowConnectionKey, true);
+
 			this._onDidChange.fire();
 			if (!silent) {
 				_onDidChangeAuthentication.fire({ reason: 'disconnected', key: this.key });
@@ -294,12 +317,6 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		return (await this.session()) != null;
 	}
 
-	get maybeConnected(): boolean | undefined {
-		if (this._session === undefined) return undefined;
-
-		return this._session !== null;
-	}
-
 	@gate()
 	@debug()
 	async getAccountForCommit(
@@ -314,7 +331,7 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		if (!connected) return undefined;
 
 		try {
-			const author = await this.onGetAccountForCommit(this._session!, ref, options);
+			const author = await this.getProviderAccountForCommit(this._session!, ref, options);
 			this.invalidAuthenticationCount = 0;
 			return author;
 		} catch (ex) {
@@ -327,6 +344,14 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		}
 	}
 
+	protected abstract getProviderAccountForCommit(
+		session: AuthenticationSession,
+		ref: string,
+		options?: {
+			avatarSize?: number;
+		},
+	): Promise<Account | undefined>;
+
 	@gate()
 	@debug()
 	async getAccountForEmail(
@@ -341,7 +366,7 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		if (!connected) return undefined;
 
 		try {
-			const author = await this.onGetAccountForEmail(this._session!, email, options);
+			const author = await this.getProviderAccountForEmail(this._session!, email, options);
 			this.invalidAuthenticationCount = 0;
 			return author;
 		} catch (ex) {
@@ -354,6 +379,14 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		}
 	}
 
+	protected abstract getProviderAccountForEmail(
+		session: AuthenticationSession,
+		email: string,
+		options?: {
+			avatarSize?: number;
+		},
+	): Promise<Account | undefined>;
+
 	@gate()
 	@debug()
 	async getIssueOrPullRequest(id: string): Promise<IssueOrPullRequest | undefined> {
@@ -363,7 +396,7 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		if (!connected) return undefined;
 
 		try {
-			const issueOrPullRequest = await this.onGetIssueOrPullRequest(this._session!, id);
+			const issueOrPullRequest = await this.getProviderIssueOrPullRequest(this._session!, id);
 			this.invalidAuthenticationCount = 0;
 			return issueOrPullRequest;
 		} catch (ex) {
@@ -376,6 +409,11 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		}
 	}
 
+	protected abstract getProviderIssueOrPullRequest(
+		session: AuthenticationSession,
+		id: string,
+	): Promise<IssueOrPullRequest | undefined>;
+
 	@gate()
 	@debug()
 	async getPullRequestForBranch(
@@ -392,7 +430,7 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		if (!connected) return undefined;
 
 		try {
-			const pr = await this.onGetPullRequestForBranch(this._session!, branch, options);
+			const pr = await this.getProviderPullRequestForBranch(this._session!, branch, options);
 			this.invalidAuthenticationCount = 0;
 			return pr;
 		} catch (ex) {
@@ -404,6 +442,15 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 			return undefined;
 		}
 	}
+	protected abstract getProviderPullRequestForBranch(
+		session: AuthenticationSession,
+		branch: string,
+		options?: {
+			avatarSize?: number;
+			include?: PullRequestState[];
+			limit?: number;
+		},
+	): Promise<PullRequest | undefined>;
 
 	private _prsByCommit = new Map<string, Promise<PullRequest | null> | PullRequest | null>();
 
@@ -420,97 +467,113 @@ export abstract class RemoteProviderWithApi extends RemoteProvider {
 		return pr.then(pr => pr ?? undefined);
 	}
 
-	protected abstract get authProvider(): { id: string; scopes: string[] };
+	@debug()
+	private async getPullRequestForCommitCore(ref: string) {
+		const cc = Logger.getCorrelationContext();
 
-	protected abstract onGetAccountForCommit(
-		session: AuthenticationSession,
-		ref: string,
-		options?: {
-			avatarSize?: number;
-		},
-	): Promise<Account | undefined>;
+		const connected = this.maybeConnected ?? (await this.isConnected());
+		if (!connected) return null;
 
-	protected abstract onGetAccountForEmail(
-		session: AuthenticationSession,
-		email: string,
-		options?: {
-			avatarSize?: number;
-		},
-	): Promise<Account | undefined>;
+		try {
+			const pr = (await this.getProviderPullRequestForCommit(this._session!, ref)) ?? null;
+			this._prsByCommit.set(ref, pr);
+			this.invalidAuthenticationCount = 0;
+			return pr;
+		} catch (ex) {
+			Logger.error(ex, cc);
 
-	protected abstract onGetIssueOrPullRequest(
-		session: AuthenticationSession,
-		id: string,
-	): Promise<IssueOrPullRequest | undefined>;
+			this._prsByCommit.delete(ref);
 
-	protected abstract onGetPullRequestForBranch(
-		session: AuthenticationSession,
-		branch: string,
-		options?: {
-			avatarSize?: number;
-			include?: PullRequestState[];
-			limit?: number;
-		},
-	): Promise<PullRequest | undefined>;
+			if (ex instanceof AuthenticationError) {
+				this.handleAuthenticationException();
+			}
+			return null;
+		}
+	}
 
-	protected abstract onGetPullRequestForCommit(
+	protected abstract getProviderPullRequestForCommit(
 		session: AuthenticationSession,
 		ref: string,
 	): Promise<PullRequest | undefined>;
 
-	protected _session: AuthenticationSession | null | undefined;
-	protected session() {
-		if (this._session === undefined) {
-			return this.ensureSession(false);
-		}
-		return this._session ?? undefined;
-	}
-
-	private async ensureSession(createIfNone: boolean) {
+	@gate()
+	private async ensureSession(createIfNone: boolean): Promise<AuthenticationSession | undefined> {
 		if (this._session != null) return this._session;
 
+		if (createIfNone) {
+			await Container.context.globalState.update(this.disallowConnectionKey, undefined);
+		} else if (Container.context.globalState.get<boolean>(this.disallowConnectionKey)) {
+			return undefined;
+		}
+
 		let session;
 		try {
 			session = await authentication.getSession(this.authProvider.id, this.authProvider.scopes, {
 				createIfNone: createIfNone,
 			});
 		} catch (ex) {
-			// TODO@eamodio	save that the user rejected auth?
+			await Container.context.globalState.update(this.disallowConnectionKey, true);
+
+			if (ex instanceof Error && ex.message.includes('User did not consent')) {
+				if (!createIfNone) {
+					void this.promptToRetyConnection();
+				}
+				return undefined;
+			}
+
+			session = null;
+		}
+
+		if (session === undefined && !createIfNone) {
+			await Container.context.globalState.update(this.disallowConnectionKey, true);
+
+			void this.promptToConnect();
 		}
 
 		this._session = session ?? null;
 		this.invalidAuthenticationCount = 0;
 
-		if (session != null && createIfNone) {
-			this._onDidChange.fire();
-			_onDidChangeAuthentication.fire({ reason: 'connected', key: this.key });
+		if (session != null) {
+			await Container.context.globalState.update(this.disallowConnectionKey, undefined);
+
+			if (createIfNone) {
+				this._onDidChange.fire();
+				_onDidChangeAuthentication.fire({ reason: 'connected', key: this.key });
+			}
 		}
 
 		return session ?? undefined;
 	}
 
 	@gate()
-	@debug()
-	private async getPullRequestForCommitCore(ref: string) {
-		const cc = Logger.getCorrelationContext();
-
-		const connected = this.maybeConnected ?? (await this.isConnected());
-		if (!connected) return null;
+	private async promptToConnect() {
+		const connect: MessageItem = { title: `Connect to ${this.name}` };
+		const cancel: MessageItem = { title: 'Cancel', isCloseAffordance: true };
+
+		const result = await window.showInformationMessage(
+			`GitLens would like to connect to ${this.name} to provide a rich integration with pull requests, issues, avatars, and more. If you choose to connect, you will be prompted to authenticate with ${this.name}.`,
+			connect,
+			cancel,
+		);
 
-		try {
-			const pr = (await this.onGetPullRequestForCommit(this._session!, ref)) ?? null;
-			this._prsByCommit.set(ref, pr);
-			this.invalidAuthenticationCount = 0;
-			return pr;
-		} catch (ex) {
-			Logger.error(ex, cc);
+		if (result === connect) {
+			void this.connect();
+		}
+	}
 
-			this._prsByCommit.delete(ref);
+	@gate()
+	private async promptToRetyConnection() {
+		const connect: MessageItem = { title: `Connect to ${this.name}` };
+		const cancel: MessageItem = { title: 'Cancel', isCloseAffordance: true };
+
+		const result = await window.showErrorMessage(
+			`GitLens wants to connect to ${this.name} to provide a rich integration with pull requests, issues, avatars, and more. If you choose to connect, you will be prompted again to allow authentication with ${this.name}.`,
+			connect,
+			cancel,
+		);
 
-			if (ex instanceof AuthenticationError) {
-				this.handleAuthenticationException();
-			}
-			return null;
+		if (result === connect) {
+			void this.connect();
 		}
 	}