From 1cb06001ec36a30bbb7e9e15a57bef5e2f87429f Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Fri, 16 Oct 2020 01:21:41 -0400 Subject: [PATCH] Adds authentication prompt & rejection prompt Avoids repeated authentication prompts Persists provider disconnect across reloads --- src/constants.ts | 1 + src/git/remotes/factory.ts | 4 +- src/git/remotes/github.ts | 15 ++- src/git/remotes/provider.ts | 241 ++++++++++++++++++++++++++++---------------- 4 files changed, 161 insertions(+), 100 deletions(-) diff --git a/src/constants.ts b/src/constants.ts index fa37e53..1a521e1 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -133,6 +133,7 @@ export enum GlyphChars { export enum GlobalState { Avatars = 'gitlens:avatars', + DisallowConnectionPrefix = 'gitlens:disallow:connection:', Version = 'gitlensVersion', } diff --git a/src/git/remotes/factory.ts b/src/git/remotes/factory.ts index 254a5a5..efd5d4d 100644 --- a/src/git/remotes/factory.ts +++ b/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 }; diff --git a/src/git/remotes/github.ts b/src/git/remotes/github.ts index 0e3c913..8408a86 100644 --- a/src/git/remotes/github.ts +++ b/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 { @@ -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 { diff --git a/src/git/remotes/provider.ts b/src/git/remotes/provider.ts index 46cc2a9..22c2b6b 100644 --- a/src/git/remotes/provider.ts +++ b/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 { 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; + @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; + @gate() @debug() async getIssueOrPullRequest(id: string): Promise { @@ -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; + @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; private _prsByCommit = new Map | 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; + const connected = this.maybeConnected ?? (await this.isConnected()); + if (!connected) return null; - protected abstract onGetAccountForEmail( - session: AuthenticationSession, - email: string, - options?: { - avatarSize?: number; - }, - ): Promise; + 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; + this._prsByCommit.delete(ref); - protected abstract onGetPullRequestForBranch( - session: AuthenticationSession, - branch: string, - options?: { - avatarSize?: number; - include?: PullRequestState[]; - limit?: number; - }, - ): Promise; + if (ex instanceof AuthenticationError) { + this.handleAuthenticationException(); + } + return null; + } + } - protected abstract onGetPullRequestForCommit( + protected abstract getProviderPullRequestForCommit( session: AuthenticationSession, ref: string, ): Promise; - 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 { if (this._session != null) return this._session; + if (createIfNone) { + await Container.context.globalState.update(this.disallowConnectionKey, undefined); + } else if (Container.context.globalState.get(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(); } }