diff --git a/src/gitProviders/GitHubContactServiceProvider.ts b/src/gitProviders/GitHubContactServiceProvider.ts index bc3973cfef..1d88f69a8e 100644 --- a/src/gitProviders/GitHubContactServiceProvider.ts +++ b/src/gitProviders/GitHubContactServiceProvider.ts @@ -125,7 +125,7 @@ export class GitHubContactServiceProvider implements ContactServiceProvider { } const origin = await this.pullRequestManager.folderManagers[0]?.getOrigin(); if (origin) { - const currentUser = origin.hub.currentUser ? await origin.hub.currentUser : undefined; + const currentUser = await origin.getAuthenticatedUser(); if (currentUser) { return currentUser.login; } diff --git a/src/github/credentials.ts b/src/github/credentials.ts index e1e9ae9ee1..9abe709905 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -80,8 +80,8 @@ export async function findExistingSession( export interface GitHub { octokit: LoggingOctokit; graphql: LoggingApolloClient; - currentUser?: Promise; - isEmu?: Promise; + currentUser?: IAccount; + isEmu?: boolean; } interface AuthResult { @@ -102,6 +102,7 @@ export class CredentialStore extends Disposable { private _isSamling: boolean = false; private _handlingAuthError: Map> = new Map(); private _lastAuthErrorHandledAt: Map = new Map(); + private _currentUserRequests: WeakMap> = new WeakMap(); // Cooldown long enough to absorb retries from in-flight requests that were // issued with the now-invalid token, but short enough that a token that // is invalidated again soon after re-auth will still trigger another prompt. @@ -551,34 +552,53 @@ export class CredentialStore extends Disposable { } public async isCurrentUser(authProviderId: AuthProvider, username: string): Promise { - const api = authProviderId === AuthProvider.github ? this._githubAPI : this._githubEnterpriseAPI; - return (await api?.currentUser)?.login === username; + return (await this.getCurrentUser(authProviderId))?.login === username; } public async getIsEmu(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); - return !!(await github?.isEmu); + await this.ensureCurrentUser(github); + return !!github?.isEmu; } - public getCurrentUser(authProviderId: AuthProvider): Promise { + public async getCurrentUser(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); - const octokit = github?.octokit; - return (octokit && github?.currentUser)!; - } - - private setCurrentUser(github: GitHub): void { - const getUser: ReturnType = new Promise((resolve, reject) => { - Logger.debug('Getting current user', CredentialStore.ID); - github.octokit.call(github.octokit.api.users.getAuthenticated, {}).then(result => { - Logger.debug(`Got current user ${result.data.login}`, CredentialStore.ID); - resolve(result); - }).catch(e => { - Logger.error(`Failed to get current user: ${e}, ${e.message}`, CredentialStore.ID); - reject(e); - }); - }); - github.currentUser = getUser.then(result => convertRESTUserToAccount(result.data)); - github.isEmu = getUser.then(result => result.data.plan?.name === 'emu_user'); + await this.ensureCurrentUser(github); + return github?.currentUser!; + } + + private ensureCurrentUser(github: GitHub | undefined): Promise { + if (!github || (github.currentUser && github.isEmu !== undefined)) { + return Promise.resolve(); + } + + const existingRequest = this._currentUserRequests.get(github); + if (existingRequest) { + return existingRequest; + } + + const request = this.setCurrentUser(github); + this._currentUserRequests.set(github, request); + const clearRequest = () => { + if (this._currentUserRequests.get(github) === request) { + this._currentUserRequests.delete(github); + } + }; + void request.then(clearRequest, clearRequest); + return request; + } + + private async setCurrentUser(github: GitHub): Promise { + Logger.debug('Getting current user', CredentialStore.ID); + try { + const result = await github.octokit.call(github.octokit.api.users.getAuthenticated, {}); + Logger.debug(`Got current user ${result.data.login}`, CredentialStore.ID); + github.currentUser = convertRESTUserToAccount(result.data); + github.isEmu = result.data.plan?.name === 'emu_user'; + } catch (e) { + Logger.error(`Failed to get current user: ${e}, ${e.message}`, CredentialStore.ID); + throw e; + } } private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions, scopes: string[], requireScopes: boolean): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean, scopes: string[] }> { @@ -654,7 +674,7 @@ export class CredentialStore extends Disposable { octokit: new LoggingOctokit(octokit, rateLogger), graphql: new LoggingApolloClient(graphql, rateLogger), }; - this.setCurrentUser(github); + void this.ensureCurrentUser(github).catch(() => undefined); return github; } } diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index fb6abd413d..cd825eae00 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -424,7 +424,7 @@ export class GitHubRepository extends Disposable { repo }); Logger.debug(`Fetch metadata for repo ${owner}/${repo} - done`, this.id); - const metadata = { ...result.data, currentUser: await this._hub?.currentUser }; + const metadata = { ...result.data, currentUser: await this.getAuthenticatedUser() }; return metadata; } diff --git a/src/test/github/credentials.test.ts b/src/test/github/credentials.test.ts index 14718f7e4b..e6163eac31 100644 --- a/src/test/github/credentials.test.ts +++ b/src/test/github/credentials.test.ts @@ -4,9 +4,14 @@ *--------------------------------------------------------------------------------------------*/ import { strictEqual, deepStrictEqual } from 'assert'; +import { Octokit } from '@octokit/rest'; +import { createSandbox, SinonSandbox } from 'sinon'; import * as vscode from 'vscode'; import { AuthProvider } from '../../common/authentication'; -import { findExistingSession } from '../../github/credentials'; +import { CredentialStore, findExistingSession, GitHub } from '../../github/credentials'; +import { LoggingApolloClient, LoggingOctokit, RateLogger } from '../../github/loggingOctokit'; +import { MockExtensionContext } from '../mocks/mockExtensionContext'; +import { MockTelemetry } from '../mocks/mockTelemetry'; const oldestScopes = ['read:user', 'user:email', 'repo']; const defaultScopes = [...oldestScopes, 'workflow']; @@ -29,6 +34,16 @@ function createSession(id: string, accountId: string, scopes: string[]): vscode. } describe('CredentialStore', function () { + let sinon: SinonSandbox; + + beforeEach(function () { + sinon = createSandbox(); + }); + + afterEach(function () { + sinon.restore(); + }); + describe('findExistingSession', function () { it('keeps broader scope lookup on the preferred account', async function () { const firstAccountAdditional = createSession('first-additional', 'first', additionalScopes); @@ -99,4 +114,55 @@ describe('CredentialStore', function () { deepStrictEqual(additionalResult?.scopes, additionalScopes); }); }); + + it('retries the current user request after a failure', async function () { + const telemetry = new MockTelemetry(); + const credentialStore = new CredentialStore(telemetry, new MockExtensionContext()); + const github: GitHub = { + octokit: new LoggingOctokit(new Octokit(), new RateLogger(telemetry, false)), + graphql: {} as LoggingApolloClient, + }; + sinon.stub(credentialStore, 'getHub').returns(github); + const getAuthenticatedUser = sinon.stub(github.octokit, 'call'); + const error = new Error('Connect Timeout Error'); + getAuthenticatedUser.onFirstCall().rejects(error); + getAuthenticatedUser.onSecondCall().resolves({ + data: { + login: 'octocat', + node_id: 'MDQ6VXNlcjE=', + html_url: 'https://github.com/octocat', + avatar_url: 'https://github.com/images/error/octocat_happy.gif', + type: 'User', + plan: { name: 'emu_user' }, + } + }); + + deepStrictEqual(await Promise.allSettled([ + credentialStore.getCurrentUser(AuthProvider.github), + credentialStore.getIsEmu(AuthProvider.github), + ]), [ + { status: 'rejected', reason: error }, + { status: 'rejected', reason: error }, + ]); + strictEqual(getAuthenticatedUser.callCount, 1); + strictEqual(github.currentUser, undefined); + strictEqual(github.isEmu, undefined); + const [currentUser, isEmu] = await Promise.all([ + credentialStore.getCurrentUser(AuthProvider.github), + credentialStore.getIsEmu(AuthProvider.github), + ]); + const cachedCurrentUser = await credentialStore.getCurrentUser(AuthProvider.github); + + deepStrictEqual({ + requests: getAuthenticatedUser.callCount, + login: currentUser.login, + isEmu, + cachedLogin: cachedCurrentUser.login, + }, { + requests: 2, + login: 'octocat', + isEmu: true, + cachedLogin: 'octocat', + }); + }); });