Respect preferred GitHub account across authentication scopes - #8940
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: alexr00 <38270282+alexr00@users.noreply.github.com>
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
Copilot Can you explain why this happens? As far as I can see we're always asking for a specific auth provider id already. Are you saying that getSession can return a session with a different auth provider ID than we ask for?
No—the provider ID remains |
Pull request was converted to draft
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved account-switch races and stale cache paths can expose or retain data from the previous account.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates GitHub authentication to honor the preferred account and refresh account-specific state during account switching.
Changes:
- Constrains session selection to the preferred account.
- Clears repository, review, issue, notification, and UI state.
- Adds regression coverage for authentication and cache cleanup.
File summaries
| File | Description |
|---|---|
src/view/webviewViewCoordinator.ts |
Resets active PR webview state |
src/view/reviewsManager.ts |
Clears and refreshes review state |
src/view/reviewManager.ts |
Clears account-specific review state |
src/view/prsTreeModel.ts |
Supports silent cache clearing |
src/view/prsTreeDataProvider.ts |
Clears PR tree state |
src/view/createPullRequestHelper.ts |
Resets PR creation state |
src/test/view/reviewManager.test.ts |
Tests review-state cleanup |
src/test/view/prsTree.test.ts |
Tests tree cleanup |
src/test/notifications/notificationsManager.test.ts |
Tests notification cleanup |
src/test/issues/stateManager.test.ts |
Tests issue cleanup |
src/test/github/pullRequestOverview.test.ts |
Tests panel cleanup |
src/test/github/folderRepositoryManager.test.ts |
Tests repository cache cleanup |
src/test/github/credentials.test.ts |
Tests preferred-account selection |
src/notifications/notificationsManager.ts |
Clears notification state |
src/issues/stateManager.ts |
Clears and refreshes issue state |
src/github/repositoriesManager.ts |
Propagates repository cleanup |
src/github/issueOverview.ts |
Clears issue panels |
src/github/folderRepositoryManager.ts |
Clears repository and user caches |
src/github/credentials.ts |
Handles preferred-account sessions |
src/extension.ts |
Coordinates account-change cleanup |
Review details
Suppressed comments (6)
src/github/credentials.ts:66
- These account-constrained probes only cover
findExistingSession. The richer-scope upgrade path ingetHubEnsureAdditionalScopescallsinitialize(..., requireScopes=true), which bypassesfindExistingSessionand requestsSCOPES_WITH_ADDITIONALwithout anaccount; if the preferred account only has the default scopes while another account has the richer session, that upgrade can still silently switch identities. Thread the preferred account through the forced-scope path as well.
const scopesInPreferenceOrder = [SCOPES_WITH_ADDITIONAL, SCOPES_OLD, SCOPES_OLDEST];
for (const scopes of scopesInPreferenceOrder) {
const session = await getSession(authProviderId, scopes, { silent: true, account: preferredSession.account });
src/github/folderRepositoryManager.ts:497
- Clearing this promise breaks the serialization guard in
updateRepositories. If a repository refresh is already in flight when the account changes, the auth handler immediately starts another refresh; the olddoUpdateRepositoriescan then finish later and replace_githubRepositoriesor related caches with results from the previous auth generation. Keep the in-flight operation serialized, or discard results from an older auth generation instead of resetting this reference.
this._updatingRepositories = undefined;
src/github/folderRepositoryManager.ts:700
- Once a cache kind is added to
_invalidatedUserCaches, this check always returnsundefinedfor that manager because the set is never cleared.getOrgProjectshas no in-memory cache and therefore performs a fresh API fetch on every lookup after an account change instead of reusing the newly fetched data. Clear the marker after a successful account-scoped refresh or retain the result in memory.
if (this._invalidatedUserCaches.has(userKind)) {
return undefined;
src/github/folderRepositoryManager.ts:490
totalFetchedPagesis another per-query pagination cache (declared later in this class), but this auth reset only clears_repositoryPageInformation. After an account switch,fetchPagedDatacan reuse the previous account's page count and stop early or request the wrong pages for the new account. CleartotalFetchedPageshere as part of the reset.
clearForAuthChange(): void {
this._sessionIgnoredRemoteNames.clear();
this._inaccessibleRepos.clear();
this._repositoryPageInformation.clear();
this._gitBlameCache = {};
src/issues/stateManager.ts:218
- The auth-change cleanup clears query and user caches but leaves
state.currentIssueintact. ConsequentlycurrentIssues(), the status bar, and issue commands can continue exposing or operating on the old account'sIssueModelafter credentials switch. Reset and dispose the current issue as part of this account-change path, and notify the current-issue listeners.
for (const state of this._singleRepoStates.values()) {
if (state) {
state.issueCollection.clear();
state.userMap = undefined;
}
src/view/prsTreeDataProvider.ts:372
- On the account-change path this only clears the query cache.
PrsTreeModel.forceClearCache(true)leaves_queriedPullRequestsandcopilotStateModelpopulated with old-account models/statuses, which are still read by tree item tooltips, badges, and Copilot state queries after the tree is rebuilt. Use an auth-specific model reset that clears those account-bound stores and their notifications as well.
clear() {
this.prsTreeModel.forceClearCache(true);
this._children.forEach(child => child.dispose());
this._children = [];
this._onDidChangeTreeData.fire();
- Files reviewed: 21/21 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect account-switch races and stale account-scoped state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (9)
Previously missed (2) — in code that hasn't changed since the last review.
src/issues/stateManager.ts:218
- This clears issue query/user caches but leaves each
state.currentIssuepointing at anIssueModelfrom the previous account.currentIssues()and the issue status-bar actions will therefore continue to expose and operate on that stale issue after an account switch; clear or re-resolve the current issues and publish the corresponding current-issue/status updates here.
src/view/reviewManager.ts:1532 - Resetting these fields does not invalidate
_validateStatusInProgress. A validation started before the account event can continue after this method returns, restore_lastCommitShaor the active PR from the old identity, and the laterupdateState(true)is queued behind it. Add a generation guard/cancellation or await and discard the old validation before publishing state.
src/github/credentials.ts:164
- Environment-token authentication stores
_accountIdasundefined(line 226). If that active token is removed,newSessionis also undefined, sohasAccountChanged(undefined, undefined)is false and the extension skips its account-change clearing path, leaving the token's PR/issue/panel caches in place after sign-out. Treat removal of an active session as an account-state change or track the environment token's identity separately.
accountChanged = hasAccountChanged(this._accountId, newSession.session);
src/github/folderRepositoryManager.ts:489
totalFetchedPagesis also account-dependent pagination state, but this reset only clears_repositoryPageInformation. After switching accounts,fetchPagedDatastill compares the new result against the previous account's page count, so it can stop after an earlier repository/page and omit PRs visible only to the new account. CleartotalFetchedPageshere as well.
this._repositoryPageInformation.clear();
src/notifications/notificationsManager.ts:320
- Replacing the map protects
getNotifications, butmarkAsRead,markAsDone, andmarkPullRequestsstill await provider calls and then mutatethis._notificationswithout checking the cache generation. A completion from the old cache can delete or fire against a replacement-account entry, andmarkPullRequestscan act through the new provider. Capture the cache/account generation and ignore stale completions afterclear().
this._notifications = new Map();
src/view/prsTreeDataProvider.ts:372
- The auth-change clear only drops
_cachedPRsand_allCachedPRs;PrsTreeModelstill retains_queriedPullRequestsstatus entries andcopilotStateModel's account-bound PR/notification models. After switching accounts, those entries can keep old PR decorations or Copilot badge data even though the tree children were reset. Add an auth-specific model reset, including the necessary decoration and badge invalidation events, instead of only disposing the tree nodes.
clear() {
this.prsTreeModel.forceClearCache(true);
this._children.forEach(child => child.dispose());
this._children = [];
this._onDidChangeTreeData.fire();
src/view/prsTreeDataProvider.ts:372
- Disposing
_childrendoes not invalidate an in-flightgetChildren(undefined). After its awaits ingetChildren, that call can still assign its old result to_childrenafter this method has emptied it, reintroducing pre-switch nodes. Add a generation/token check before publishing the result.
clear() {
this.prsTreeModel.forceClearCache(true);
this._children.forEach(child => child.dispose());
this._children = [];
this._onDidChangeTreeData.fire();
src/view/prsTreeDataProvider.ts:370
forceClearCache(true)only removes the current maps; an already runninggetPullRequestsForQuery/getAllPullRequestscan still set them after this clear. Because the query lock serializes the next load behind that completion, the new-account refresh can then reuse the old-account result instead of fetching again. Add a generation check before caching and returning async results.
this.prsTreeModel.forceClearCache(true);
this._children.forEach(child => child.dispose());
src/view/prsTreeModel.ts:204
- This cache reset does not clear
copilotStateModel, including its cached PR models and notification set. After an account switch, Copilot counts and notification badges can therefore continue to reference the previous account until the watcher performs a later poll. Reset that account-scoped state as part of auth cleanup.
public forceClearCache(silent: boolean = false) {
this._cachedPRs.clear();
this._allCachedPRs.clear();
if (!silent) {
this._onDidChangeData.fire();
}
- Files reviewed: 21/21 changed files
- Comments generated: 2
- Review effort level: Lite
Account switching could retain credentials from another GitHub identity when accounts had different scope-specific sessions, exposing stale private PRs and causing unauthorized actions under the wrong account.