From 68ab8fc56e64b3d687b5993a565e4bd93115eb22 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Sat, 19 Feb 2022 01:56:43 -0500 Subject: [PATCH] Fixes #1818: adds reauthentication support --- src/errors.ts | 8 +- src/premium/github/github.ts | 144 ++++++++++++++++++++++---------- src/premium/github/githubGitProvider.ts | 62 ++++++++++---- 3 files changed, 147 insertions(+), 67 deletions(-) diff --git a/src/errors.ts b/src/errors.ts index 19c0cf3..33b186b 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -56,7 +56,7 @@ export class AuthenticationError extends Error { let message; let reason: AuthenticationErrorReason | undefined; if (messageOrReason == null) { - message = `Unable to get required authentication session for '${id}`; + message = `Unable to get required authentication session for '${id}'`; } else if (typeof messageOrReason === 'string') { message = messageOrReason; reason = undefined; @@ -64,13 +64,13 @@ export class AuthenticationError extends Error { reason = messageOrReason; switch (reason) { case AuthenticationErrorReason.UserDidNotConsent: - message = `'${id} authentication is required for this operation`; + message = `'${id}' authentication is required for this operation`; break; case AuthenticationErrorReason.Unauthorized: - message = `The provided '${id}' credentials are either invalid or expired`; + message = `Your '${id}' credentials are either invalid or expired`; break; case AuthenticationErrorReason.Forbidden: - message = `The provided '${id}' credentials do not have the required access`; + message = `Your '${id}' credentials do not have the required access`; break; } } diff --git a/src/premium/github/github.ts b/src/premium/github/github.ts index 2f66270..e364db6 100644 --- a/src/premium/github/github.ts +++ b/src/premium/github/github.ts @@ -2,7 +2,7 @@ import { Octokit } from '@octokit/core'; import { GraphqlResponseError } from '@octokit/graphql'; import { RequestError } from '@octokit/request-error'; import type { Endpoints, OctokitResponse, RequestParameters } from '@octokit/types'; -import { window } from 'vscode'; +import { Event, EventEmitter, window } from 'vscode'; import { fetch } from '@env/fetch'; import { isWeb } from '@env/platform'; import { @@ -32,6 +32,11 @@ const emptyPagedResult: PagedResult = Object.freeze({ values: [] }); const emptyBlameResult: GitHubBlame = Object.freeze({ ranges: [] }); export class GitHubApi { + private readonly _onDidReauthenticate = new EventEmitter(); + get onDidReauthenticate(): Event { + return this._onDidReauthenticate.event; + } + @debug({ args: { 0: p => p.name, 1: '' } }) async getAccountForCommit( provider: RichRemoteProvider, @@ -102,7 +107,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -170,7 +175,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -222,7 +227,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -292,7 +297,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -392,7 +397,7 @@ export class GitHubApi { return GitHubPullRequest.from(prs[0], provider); } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -485,7 +490,7 @@ export class GitHubApi { return GitHubPullRequest.from(prs[0], provider); } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -561,7 +566,7 @@ export class GitHubApi { return { ranges: ranges, viewer: rsp.viewer?.name }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, emptyBlameResult); + return this.handleException(ex, cc, emptyBlameResult); } } @@ -639,7 +644,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, emptyPagedResult); + return this.handleException(ex, cc, emptyPagedResult); } } @@ -685,7 +690,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } // const results = await this.getCommits(token, owner, repo, ref, { limit: 1 }); @@ -777,7 +782,7 @@ export class GitHubApi { return branches; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, []); + return this.handleException(ex, cc, []); } } @@ -824,7 +829,7 @@ export class GitHubApi { return count; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -893,7 +898,7 @@ export class GitHubApi { return branches; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, []); + return this.handleException(ex, cc, []); } } @@ -1031,7 +1036,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, emptyPagedResult); + return this.handleException(ex, cc, emptyPagedResult); } } @@ -1091,7 +1096,7 @@ export class GitHubApi { return commit != null ? { values: [commit], viewer: rsp.viewer.name } : emptyPagedResult; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, emptyPagedResult); + return this.handleException(ex, cc, emptyPagedResult); } } @@ -1179,7 +1184,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -1257,7 +1262,7 @@ export class GitHubApi { return date; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -1280,7 +1285,7 @@ export class GitHubApi { return rsp.data; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, []); + return this.handleException(ex, cc, []); } } @@ -1318,7 +1323,7 @@ export class GitHubApi { return rsp.repository?.defaultBranchRef?.name ?? undefined; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -1359,7 +1364,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -1399,7 +1404,7 @@ export class GitHubApi { return rsp.repository.visibility === 'PUBLIC' ? RepositoryVisibility.Public : RepositoryVisibility.Private; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -1482,7 +1487,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, emptyPagedResult); + return this.handleException(ex, cc, emptyPagedResult); } } @@ -1561,7 +1566,7 @@ export class GitHubApi { return rsp?.repository?.object?.history.nodes?.[0]?.oid ?? undefined; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -1639,7 +1644,7 @@ export class GitHubApi { }; } catch (ex) { debugger; - return this.handleRequestError(ex, cc, undefined); + return this.handleException(ex, cc, undefined); } } @@ -1704,9 +1709,14 @@ export class GitHubApi { case 'FORBIDDEN': throw new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); } + + void window.showErrorMessage(`GitHub request failed: ${ex.errors?.[0]?.message ?? ex.message}`, 'OK'); + } else if (ex instanceof RequestError) { + this.handleRequestError(ex); + } else { + void window.showErrorMessage(`GitHub request failed: ${ex.message}`, 'OK'); } - void window.showErrorMessage(`Unable to complete GitHub request: ${ex.message}`); throw ex; } } @@ -1720,39 +1730,81 @@ export class GitHubApi { return (await this.octokit(token).request(route, options)) as any; } catch (ex) { if (ex instanceof RequestError) { - switch (ex.status) { - case 404: // Not found - case 410: // Gone - case 422: // Unprocessable Entity - throw new ProviderRequestNotFoundError(ex); - // case 429: //Too Many Requests - case 401: // Unauthorized - throw new AuthenticationError('github', AuthenticationErrorReason.Unauthorized, ex); - case 403: // Forbidden - throw new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); - case 500: // Internal Server Error - if (ex.response != null) { - // TODO@eamodio: Handle GitHub down errors - } - break; - default: - if (ex.status >= 400 && ex.status < 500) throw new ProviderRequestClientError(ex); - break; - } + this.handleRequestError(ex); + } else { + void window.showErrorMessage(`GitHub request failed: ${ex.message}`, 'OK'); } - void window.showErrorMessage(`Unable to complete GitHub request: ${ex.message}`); throw ex; } } - private handleRequestError(ex: unknown | Error, cc: LogCorrelationContext | undefined, defaultValue: T): T { + private handleRequestError(ex: RequestError): void { + switch (ex.status) { + case 404: // Not found + case 410: // Gone + case 422: // Unprocessable Entity + throw new ProviderRequestNotFoundError(ex); + // case 429: //Too Many Requests + case 401: // Unauthorized + throw new AuthenticationError('github', AuthenticationErrorReason.Unauthorized, ex); + case 403: // Forbidden + throw new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); + case 500: // Internal Server Error + if (ex.response != null) { + void window.showErrorMessage( + 'GitHub failed to respond and might be experiencing issues. Please visit the [GitHub status page](https://githubstatus.com) for more information.', + 'OK', + ); + } + break; + case 502: // Bad Gateway + // GitHub seems to return this status code for timeouts + if (ex.message.includes('timeout')) { + void window.showErrorMessage('GitHub request timed out', 'OK'); + return; + } + break; + default: + if (ex.status >= 400 && ex.status < 500) throw new ProviderRequestClientError(ex); + break; + } + + void window.showErrorMessage( + `GitHub request failed: ${(ex.response as any)?.errors?.[0]?.message ?? ex.message}`, + 'OK', + ); + } + + private handleException(ex: unknown | Error, cc: LogCorrelationContext | undefined, defaultValue: T): T { if (ex instanceof ProviderRequestNotFoundError) return defaultValue; Logger.error(ex, cc); debugger; + + if (ex instanceof AuthenticationError) { + void this.showAuthenticationErrorMessage(ex); + } throw ex; } + + private async showAuthenticationErrorMessage(ex: AuthenticationError) { + if (ex.reason === AuthenticationErrorReason.Unauthorized || ex.reason === AuthenticationErrorReason.Forbidden) { + const confirm = 'Reauthenticate'; + const result = await window.showErrorMessage( + `${ex.message}. Would you like to try reauthenticating${ + ex.reason === AuthenticationErrorReason.Forbidden ? ' to provide additional access' : '' + }?`, + confirm, + ); + + if (result === confirm) { + this._onDidReauthenticate.fire(); + } + } else { + void window.showErrorMessage(ex.message, 'OK'); + } + } } export interface GitHubBlame { diff --git a/src/premium/github/githubGitProvider.ts b/src/premium/github/githubGitProvider.ts index 9c52d4c..e8ec592 100644 --- a/src/premium/github/githubGitProvider.ts +++ b/src/premium/github/githubGitProvider.ts @@ -128,9 +128,13 @@ export class GitHubGitProvider implements GitProvider, Disposable { private readonly _repoInfoCache = new Map(); private readonly _tagsCache = new Map>>(); + private readonly _disposables: Disposable[] = []; + constructor(private readonly container: Container) {} - dispose() {} + dispose() { + this._disposables.forEach(d => d.dispose()); + } private onRepositoryChanged(repo: Repository, e: RepositoryChangeEvent) { // if (e.changed(RepositoryChange.Config, RepositoryChangeComparisonMode.Any)) { @@ -493,7 +497,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { const ref = !uri.sha || uri.sha === 'HEAD' ? (await metadata.getRevision()).revision : uri.sha; const blame = await github.getBlame( - session?.accessToken, + session.accessToken, metadata.repo.owner, metadata.repo.name, ref, @@ -630,7 +634,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { const ref = !uri.sha || uri.sha === 'HEAD' ? (await metadata.getRevision()).revision : uri.sha; const blame = await github.getBlame( - session?.accessToken, + session.accessToken, metadata.repo.owner, metadata.repo.name, ref, @@ -796,7 +800,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { while (true) { const result = await github.getBranches( - session?.accessToken, + session.accessToken, metadata.repo.owner, metadata.repo.name, { cursor: cursor }, @@ -879,7 +883,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { try { const { metadata, github, session } = await this.ensureRepositoryContext(repoPath); - const commit = await github.getCommit(session?.accessToken, metadata.repo.owner, metadata.repo.name, ref); + const commit = await github.getCommit(session.accessToken, metadata.repo.owner, metadata.repo.name, ref); if (commit == null) return undefined; const { viewer = session.account.label } = commit; @@ -942,7 +946,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { if (options?.branch) { branches = await github.getCommitOnBranch( - session?.accessToken, + session.accessToken, metadata.repo.owner, metadata.repo.name, options?.branch, @@ -951,7 +955,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { ); } else { branches = await github.getCommitBranches( - session?.accessToken, + session.accessToken, metadata.repo.owner, metadata.repo.name, ref, @@ -1008,7 +1012,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { const ref = !options?.ref || options.ref === 'HEAD' ? (await metadata.getRevision()).revision : options.ref; const commit = await github.getCommitForFile( - session?.accessToken, + session.accessToken, metadata.repo.owner, metadata.repo.name, ref, @@ -1080,7 +1084,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { try { const { metadata, github, session } = await this.ensureRepositoryContext(repoPath); - const results = await github.getContributors(session?.accessToken, metadata.repo.owner, metadata.repo.name); + const results = await github.getContributors(session.accessToken, metadata.repo.owner, metadata.repo.name); const currentUser = await this.getCurrentUser(repoPath); const contributors = []; @@ -1127,7 +1131,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { try { const { metadata, github, session } = await this.ensureRepositoryContext(repoPath); - user = await github.getCurrentUser(session?.accessToken, metadata.repo.owner, metadata.repo.name); + user = await github.getCurrentUser(session.accessToken, metadata.repo.owner, metadata.repo.name); this._repoInfoCache.set(repoPath, { ...repo, user: user ?? null }); return user; @@ -1149,7 +1153,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { try { const { metadata, github, session } = await this.ensureRepositoryContext(repoPath); - return await github.getDefaultBranchName(session?.accessToken, metadata.repo.owner, metadata.repo.name); + return await github.getDefaultBranchName(session.accessToken, metadata.repo.owner, metadata.repo.name); } catch (ex) { Logger.error(ex, cc); debugger; @@ -1229,7 +1233,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { const { metadata, github, session } = await this.ensureRepositoryContext(repoPath); const ref = !options?.ref || options.ref === 'HEAD' ? (await metadata.getRevision()).revision : options.ref; - const result = await github.getCommits(session?.accessToken, metadata.repo.owner, metadata.repo.name, ref, { + const result = await github.getCommits(session.accessToken, metadata.repo.owner, metadata.repo.name, ref, { all: options?.all, authors: options?.authors, after: options?.cursor, @@ -1462,7 +1466,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { const { metadata, github, session } = await this.ensureRepositoryContext(repoPath); const result = await github.searchCommits( - session?.accessToken, + session.accessToken, `repo:${metadata.repo.owner}/${metadata.repo.name}+${query.join('+').trim()}`, { cursor: options?.cursor, @@ -1783,7 +1787,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { // } const ref = !options?.ref || options.ref === 'HEAD' ? (await metadata.getRevision()).revision : options.ref; - const result = await github.getCommits(session?.accessToken, metadata.repo.owner, metadata.repo.name, ref, { + const result = await github.getCommits(session.accessToken, metadata.repo.owner, metadata.repo.name, ref, { all: options?.all, after: options?.cursor, path: relativePath, @@ -2245,7 +2249,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { while (true) { const result = await github.getTags( - session?.accessToken, + session.accessToken, metadata.repo.owner, metadata.repo.name, { cursor: cursor }, @@ -2521,7 +2525,7 @@ export class GitHubGitProvider implements GitProvider, Disposable { let github; let session; try { - [github, session] = await Promise.all([this.container.github, this.ensureSession()]); + [github, session] = await Promise.all([this.ensureGitHub(), this.ensureSession()]); } catch (ex) { debugger; if (ex instanceof AuthenticationError) { @@ -2544,6 +2548,24 @@ export class GitHubGitProvider implements GitProvider, Disposable { return { github: github, metadata: metadata, remotehub: remotehub, session: session }; } + private _github: GitHubApi | undefined; + @gate() + private async ensureGitHub() { + if (this._github == null) { + const github = await this.container.github; + if (github != null) { + this._disposables.push( + github.onDidReauthenticate(() => { + this._sessionPromise = undefined; + void this.ensureSession(true); + }), + ); + } + this._github = github; + } + return this._github; + } + /** Only use this if you NEED non-promise access to RemoteHub */ private _remotehub: RemoteHubApi | undefined; private _remotehubPromise: Promise | undefined; @@ -2570,10 +2592,16 @@ export class GitHubGitProvider implements GitProvider, Disposable { } private _sessionPromise: Promise | undefined; - private async ensureSession(): Promise { + private async ensureSession(force: boolean = false): Promise { if (this._sessionPromise == null) { async function getSession(): Promise { try { + if (force) { + return await authentication.getSession('github', githubAuthenticationScopes, { + forceNewSession: true, + }); + } + return await authentication.getSession('github', githubAuthenticationScopes, { createIfNone: true, });