From 80bd6943b330c4b8100986b7f4f4ea33a436bb07 Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:34:40 +0200 Subject: [PATCH] Fix wrong account selected Fixes https://github.com/microsoft/vscode-pull-request-github/issues/8789 --- src/extension.ts | 21 +++-- src/github/credentials.ts | 61 +++++++++--- src/github/folderRepositoryManager.ts | 17 ++-- src/github/repositoriesManager.ts | 6 +- src/issues/stateManager.ts | 15 +++ src/notifications/notificationsProvider.ts | 2 + src/test/github/credentials.test.ts | 102 +++++++++++++++++++++ 7 files changed, 192 insertions(+), 32 deletions(-) create mode 100644 src/test/github/credentials.test.ts diff --git a/src/extension.ts b/src/extension.ts index d3a4c65a38..c9d237eadb 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -10,6 +10,7 @@ import { LiveShare } from 'vsls/vscode.js'; import { PostCommitCommandsProvider, Repository } from './api/api'; import { GitApiImpl } from './api/api1'; import { registerCommands } from './commands'; +import { AuthProvider } from './common/authentication'; import { commands, contexts } from './common/executeCommands'; import { isSubmodule } from './common/gitUtils'; import Logger from './common/logger'; @@ -81,15 +82,6 @@ async function init( context.subscriptions.push(Logger); Logger.appendLine('Git repository found, initializing review manager and pr tree view.', ACTIVATION); - context.subscriptions.push(credentialStore.onDidChangeSessions(async e => { - if (e.provider.id === 'github') { - await reposManager.clearCredentialCache(); - if (reviewsManager) { - reviewsManager.reviewManagers.forEach(reviewManager => reviewManager.updateState(true)); - } - } - })); - context.subscriptions.push( git.onDidPublish(async e => { // Only notify on branch publish events @@ -276,6 +268,17 @@ async function init( context.subscriptions.push(issuesFeatures); await issuesFeatures.initialize(); + context.subscriptions.push(credentialStore.onDidChangeSessions(async e => { + if (e.provider.id !== AuthProvider.github && e.provider.id !== AuthProvider.githubEnterprise) { + return; + } + await reposManager.refreshRepositories(); + await Promise.all(reviewsManager.reviewManagers.map(reviewManager => reviewManager.updateState(true))); + tree.refreshAll(true); + await issueStateManager.refreshForAuthChange(); + notificationsManager.refresh(); + })); + const workspaceContextProvider = new WorkspaceContextProvider(reposManager, git); context.subscriptions.push(workspaceContextProvider); context.subscriptions.push(vscode.chat.registerChatWorkspaceContextProvider('githubpr', workspaceContextProvider)); diff --git a/src/github/credentials.ts b/src/github/credentials.ts index b633428403..e1e9ae9ee1 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -38,6 +38,45 @@ const SCOPES_WITH_ADDITIONAL = ['read:user', 'user:email', 'repo', 'workflow', ' const LAST_USED_SCOPES_GITHUB_KEY = 'githubPullRequest.lastUsedScopes'; const LAST_USED_SCOPES_ENTERPRISE_KEY = 'githubPullRequest.lastUsedScopesEnterprise'; +type AuthenticationSessionGetter = ( + authProviderId: string, + scopes: readonly string[], + options: vscode.AuthenticationGetSessionOptions, +) => Thenable; + +interface ExistingSession { + session: vscode.AuthenticationSession; + scopes: string[]; +} + +export async function findExistingSession( + authProviderId: AuthProvider, + getSession: AuthenticationSessionGetter = (providerId, scopes, options) => vscode.authentication.getSession(providerId, scopes, options), +): Promise { + // Establish the preferred account with the normal scopes before looking for broader sessions. + // Otherwise, a single broader session from another account can override the workspace preference. + const scopePreferences = [ + { scopes: SCOPES_OLD, broaderScopes: [SCOPES_WITH_ADDITIONAL] }, + { scopes: SCOPES_OLDEST, broaderScopes: [SCOPES_WITH_ADDITIONAL, SCOPES_OLD] }, + { scopes: SCOPES_WITH_ADDITIONAL, broaderScopes: [] }, + ]; + + for (const preference of scopePreferences) { + const session = await getSession(authProviderId, preference.scopes, { silent: true }); + if (!session) { + continue; + } + + for (const broaderScopes of preference.broaderScopes) { + const broaderSession = await getSession(authProviderId, broaderScopes, { silent: true, account: session.account }); + if (broaderSession) { + return { session: broaderSession, scopes: broaderScopes }; + } + } + return { session, scopes: preference.scopes }; + } +} + export interface GitHub { octokit: LoggingOctokit; graphql: LoggingApolloClient; @@ -89,11 +128,14 @@ export class CredentialStore extends Disposable { if ((this._githubAPI || this._githubEnterpriseAPI) && !currentProvider) { return; } + let sessionChanged = false; if (currentProvider) { - const newSession = await this.getSession(currentProvider, { silent: true }, currentProvider === AuthProvider.github ? this._scopes : this._scopesEnterprise, true); - if (newSession.session?.id === this._sessionId) { + const newSession = await this.getSession(currentProvider, { silent: true }, currentProvider === AuthProvider.github ? this._scopes : this._scopesEnterprise, false); + const currentSessionId = currentProvider === AuthProvider.github ? this._sessionId : this._enterpriseSessionId; + if (newSession.session?.id === currentSessionId) { return; } + sessionChanged = true; if (currentProvider === AuthProvider.github) { this._githubAPI = undefined; this._sessionId = undefined; @@ -114,6 +156,9 @@ export class CredentialStore extends Disposable { await Promise.all(promises); if (this.isAnyAuthenticated()) { this._onDidGetSession.fire(); + if (sessionChanged && !this._isSamling) { + this._onDidChangeSessions.fire(e); + } } else if (!this._isSamling) { this._onDidChangeSessions.fire(e); } @@ -537,7 +582,7 @@ export class CredentialStore extends Disposable { } private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions, scopes: string[], requireScopes: boolean): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean, scopes: string[] }> { - const existingSession = (getAuthSessionOptions.forceNewSession || requireScopes) ? undefined : await this.findExistingScopes(authProviderId); + const existingSession = (getAuthSessionOptions.forceNewSession || requireScopes) ? undefined : await findExistingSession(authProviderId); if (existingSession?.session) { return { session: existingSession.session, isNew: false, scopes: existingSession.scopes }; } @@ -546,16 +591,6 @@ export class CredentialStore extends Disposable { return { session, isNew: !!session, scopes: requireScopes ? scopes : SCOPES_OLD }; } - private async findExistingScopes(authProviderId: AuthProvider): Promise<{ session: vscode.AuthenticationSession, scopes: string[] } | undefined> { - const scopesInPreferenceOrder = [SCOPES_WITH_ADDITIONAL, SCOPES_OLD, SCOPES_OLDEST]; - for (const scopes of scopesInPreferenceOrder) { - const session = await vscode.authentication.getSession(authProviderId, scopes, { silent: true }); - if (session) { - return { session, scopes }; - } - } - } - private async createHub(token: string, authProviderId: AuthProvider): Promise { let baseUrl = 'https://api.github.com'; let enterpriseServerUri: vscode.Uri | undefined; diff --git a/src/github/folderRepositoryManager.ts b/src/github/folderRepositoryManager.ts index b93563ce4c..9807c8a53e 100644 --- a/src/github/folderRepositoryManager.ts +++ b/src/github/folderRepositoryManager.ts @@ -443,11 +443,11 @@ export class FolderRepositoryManager extends Disposable { } private _updatingRepositories: Promise | undefined; - async updateRepositories(silent: boolean = false): Promise { + async updateRepositories(silent: boolean = false, clearUserCache: boolean = false): Promise { if (this._updatingRepositories) { await this._updatingRepositories; } - this._updatingRepositories = this.doUpdateRepositories(silent); + this._updatingRepositories = this.doUpdateRepositories(silent, clearUserCache); return this._updatingRepositories; } @@ -487,7 +487,7 @@ export class FolderRepositoryManager extends Disposable { } } - private async doUpdateRepositories(silent: boolean): Promise { + private async doUpdateRepositories(silent: boolean, clearUserCache: boolean): Promise { if (this._git.state === 'uninitialized') { Logger.appendLine('Cannot updates repositories as git is uninitialized', this.id); @@ -577,11 +577,12 @@ export class FolderRepositoryManager extends Disposable { } } + const shouldClearUserCache = clearUserCache || repositoriesAdded.length > 0; if (this.activePullRequest) { - this.getMentionableUsers(repositoriesAdded.length > 0); + this.getMentionableUsers(shouldClearUserCache); } - this.getAssignableUsers(repositoriesAdded.length > 0); + this.getAssignableUsers(shouldClearUserCache); if (isAuthenticated && activeRemotes.length) { this.state = ReposManagerState.RepositoriesLoaded; // On first activation, associate local branches with PRs @@ -732,6 +733,7 @@ export class FolderRepositoryManager extends Disposable { async getMentionableUsers(clearCache?: boolean): Promise<{ [key: string]: IAccount[] }> { if (clearCache) { delete this._mentionableUsers; + delete this._fetchMentionableUsersPromise; } if (this._mentionableUsers) { @@ -739,7 +741,7 @@ export class FolderRepositoryManager extends Disposable { return this._mentionableUsers; } - const globalStateMentionableUsers = await this.getCachedFromGlobalState('mentionableUsers'); + const globalStateMentionableUsers = clearCache ? undefined : await this.getCachedFromGlobalState('mentionableUsers'); if (!this._fetchMentionableUsersPromise) { this._fetchMentionableUsersPromise = this.createFetchMentionableUsersPromise(); @@ -752,6 +754,7 @@ export class FolderRepositoryManager extends Disposable { async getAssignableUsers(clearCache?: boolean): Promise<{ [key: string]: IAccount[] }> { if (clearCache) { delete this._assignableUsers; + delete this._fetchAssignableUsersPromise; } if (this._assignableUsers) { @@ -759,7 +762,7 @@ export class FolderRepositoryManager extends Disposable { return this._assignableUsers; } - const globalStateAssignableUsers = await this.getCachedFromGlobalState('assignableUsers'); + const globalStateAssignableUsers = clearCache ? undefined : await this.getCachedFromGlobalState('assignableUsers'); if (!this._fetchAssignableUsersPromise) { const cache: { [key: string]: IAccount[] } = {}; diff --git a/src/github/repositoriesManager.ts b/src/github/repositoriesManager.ts index beff98508b..78958f5a88 100644 --- a/src/github/repositoriesManager.ts +++ b/src/github/repositoriesManager.ts @@ -252,9 +252,9 @@ export class RepositoriesManager extends Disposable { return this._credentialStore; } - async clearCredentialCache(): Promise { - await this._credentialStore.reset(); - this.updateState(ReposManagerState.NeedsAuthentication); + async refreshRepositories(): Promise { + await Promise.all(this._folderManagers.map(folderManager => folderManager.updateRepositories(false, true))); + this.updateState(); } async authenticate(enterprise?: boolean): Promise { diff --git a/src/issues/stateManager.ts b/src/issues/stateManager.ts index 8d2ea3843f..e865208f38 100644 --- a/src/issues/stateManager.ts +++ b/src/issues/stateManager.ts @@ -209,6 +209,21 @@ export class StateManager { } } + async refreshForAuthChange() { + this.resolvedIssues.clear(); + for (const state of this._singleRepoStates.values()) { + if (state) { + state.issueCollection.clear(); + state.userMap = undefined; + } + } + if (this.manager.credentialStore.isAnyAuthenticated()) { + await this.refresh(); + } else { + this._onDidChangeIssueData.fire(); + } + } + private async doInitialize() { this.cleanIssueState(); this._queries = vscode.workspace diff --git a/src/notifications/notificationsProvider.ts b/src/notifications/notificationsProvider.ts index ba101c353b..d3482ac9ec 100644 --- a/src/notifications/notificationsProvider.ts +++ b/src/notifications/notificationsProvider.ts @@ -45,6 +45,8 @@ export class NotificationsProvider extends Disposable { this._authProvider = AuthProvider.githubEnterprise; } else if (_credentialStore.isAuthenticated(AuthProvider.github)) { this._authProvider = AuthProvider.github; + } else { + this._authProvider = undefined; } }; setAuthProvider(); diff --git a/src/test/github/credentials.test.ts b/src/test/github/credentials.test.ts new file mode 100644 index 0000000000..14718f7e4b --- /dev/null +++ b/src/test/github/credentials.test.ts @@ -0,0 +1,102 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { strictEqual, deepStrictEqual } from 'assert'; +import * as vscode from 'vscode'; +import { AuthProvider } from '../../common/authentication'; +import { findExistingSession } from '../../github/credentials'; + +const oldestScopes = ['read:user', 'user:email', 'repo']; +const defaultScopes = [...oldestScopes, 'workflow']; +const additionalScopes = [...defaultScopes, 'project', 'read:org']; + +function scopesEqual(actual: readonly string[], expected: readonly string[]): boolean { + return actual.length === expected.length && actual.every((scope, index) => scope === expected[index]); +} + +function createSession(id: string, accountId: string, scopes: string[]): vscode.AuthenticationSession { + return { + id, + accessToken: `${id}-token`, + account: { + id: accountId, + label: accountId, + }, + scopes, + }; +} + +describe('CredentialStore', function () { + describe('findExistingSession', function () { + it('keeps broader scope lookup on the preferred account', async function () { + const firstAccountAdditional = createSession('first-additional', 'first', additionalScopes); + const secondAccountDefault = createSession('second-default', 'second', defaultScopes); + const requests: { scopes: readonly string[], accountId?: string }[] = []; + + const result = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { + requests.push({ scopes, accountId: options.account?.id }); + if (options.account?.id === 'second') { + return undefined; + } + if (scopesEqual(scopes, defaultScopes)) { + return secondAccountDefault; + } + if (scopesEqual(scopes, additionalScopes)) { + return firstAccountAdditional; + } + return undefined; + }); + + strictEqual(result?.session, secondAccountDefault); + deepStrictEqual(result?.scopes, defaultScopes); + deepStrictEqual(requests, [ + { scopes: defaultScopes, accountId: undefined }, + { scopes: additionalScopes, accountId: 'second' }, + ]); + }); + + it('uses broader scopes when they belong to the preferred account', async function () { + const preferredDefault = createSession('preferred-default', 'preferred', defaultScopes); + const preferredAdditional = createSession('preferred-additional', 'preferred', additionalScopes); + + const result = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { + if (options.account?.id === 'preferred' && scopesEqual(scopes, additionalScopes)) { + return preferredAdditional; + } + if (!options.account && scopesEqual(scopes, defaultScopes)) { + return preferredDefault; + } + return undefined; + }); + + strictEqual(result?.session, preferredAdditional); + deepStrictEqual(result?.scopes, additionalScopes); + }); + + it('falls back to legacy and additional-only sessions', async function () { + const legacySession = createSession('legacy', 'legacy', oldestScopes); + const legacyResult = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { + if (!options.account && scopesEqual(scopes, oldestScopes)) { + return legacySession; + } + return undefined; + }); + + strictEqual(legacyResult?.session, legacySession); + deepStrictEqual(legacyResult?.scopes, oldestScopes); + + const additionalSession = createSession('additional', 'additional', additionalScopes); + const additionalResult = await findExistingSession(AuthProvider.github, async (_providerId, scopes, options) => { + if (!options.account && scopesEqual(scopes, additionalScopes)) { + return additionalSession; + } + return undefined; + }); + + strictEqual(additionalResult?.session, additionalSession); + deepStrictEqual(additionalResult?.scopes, additionalScopes); + }); + }); +});