From 91884904c703632a8bf2b1f8fe39ba38b02cfe20 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 16 Aug 2026 00:49:05 -0700 Subject: [PATCH] github-authentication - simplify avatar persistence Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../github-authentication/src/github.ts | 85 ++++--------------- .../src/test/github.test.ts | 43 ---------- 2 files changed, 16 insertions(+), 112 deletions(-) delete mode 100644 extensions/github-authentication/src/test/github.test.ts diff --git a/extensions/github-authentication/src/github.ts b/extensions/github-authentication/src/github.ts index 05bce7c2a9bd9c..d18ce1dd252b7b 100644 --- a/extensions/github-authentication/src/github.ts +++ b/extensions/github-authentication/src/github.ts @@ -14,16 +14,8 @@ import { crypto } from './node/crypto'; import { TIMED_OUT_ERROR, USER_CANCELLATION_ERROR } from './common/errors'; import { GitHubSocialSignInProvider, isSocialSignInProvider } from './flows'; -/** - * The stored (JSON) form of a vscode.Uri pointing to the account's avatar. - */ -interface StoredAccountIcon { - scheme: string; - authority?: string; - path?: string; - query?: string; - fragment?: string; -} +// `vscode` doesn't publicly export `UriComponents`, so derive the exact shape from `Uri.from`. +type UriComponents = Parameters[0]; interface SessionData { id: string; @@ -33,31 +25,12 @@ interface SessionData { // Unfortunately, for some time the id was a number, so we need to support both. // This can be removed once we are confident that all users have migrated to the new id. id: string | number; - // `undefined` means the avatar has not been looked up yet, `null` means a lookup - // completed and found no avatar, and a `StoredAccountIcon` is a resolved avatar. - icon?: StoredAccountIcon | null; + icon?: UriComponents; }; scopes: string[]; accessToken: string; } -/** - * Whether a stored session's account icon still needs to be looked up. - */ -export function needsAccountIconLookup(session: SessionData): boolean { - return !session.account || session.account.icon === undefined; -} - -/** - * Serializes an account icon for storage, using `null` to mark a completed lookup that found no avatar. - */ -export function serializeAccountIcon(icon: vscode.Uri | undefined, hasNoAvatar: boolean): StoredAccountIcon | null | undefined { - if (icon) { - return { scheme: icon.scheme, authority: icon.authority, path: icon.path, query: icon.query, fragment: icon.fragment }; - } - return hasNoAvatar ? null : undefined; -} - export enum AuthProviderType { github = 'github', githubEnterprise = 'github-enterprise' @@ -165,7 +138,6 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid private readonly _telemetryReporter: ExperimentationTelemetry; private readonly _keychain: Keychain; private readonly _accountsSeen = new Set(); - private readonly _sessionsWithoutAvatars = new WeakSet(); private readonly _disposable: vscode.Disposable | undefined; private _sessionsPromise: Promise; @@ -310,25 +282,21 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid // the sessions to migrate away from the bad number usage. // TODO@TylerLeonhardt: Remove this after we are confident that all users have migrated to the new id. let seenNumberAccountId: boolean = false; - // Sessions that were stored before the account icon was introduced are re-stored - // once an icon has been fetched so that we don't refetch it on every read. - let seenIconUpdate: boolean = false; + // Re-store newly verified accounts so future reads do not need another lookup. + let seenAccountUpdate: boolean = false; // TODO: eventually remove this Set because we should only have one session per set of scopes. const scopesSeen = new Set(); const sessionPromises = sessionData.map(async (session: SessionData): Promise => { // For GitHub scope list, order doesn't matter so we immediately sort the scopes const scopesStr = [...session.scopes].sort().join(' '); let userInfo: { id: string; accountName: string; avatarUrl: string | undefined } | undefined; - if (needsAccountIconLookup(session)) { - const needsAccount = !session.account; + if (!session.account) { try { userInfo = await this._githubServer.getUserInfo(session.accessToken); - seenIconUpdate = true; - if (needsAccount) { - this._logger.info(`Verified session with the following scopes: ${scopesStr}`); - } + seenAccountUpdate = true; + this._logger.info(`Verified session with the following scopes: ${scopesStr}`); } catch (e) { - if (e.message === 'Unauthorized' && needsAccount) { + if (e.message === 'Unauthorized') { return undefined; } } @@ -346,13 +314,10 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid } else { accountId = userInfo?.id ?? ''; } - let icon: vscode.Uri | undefined; - if (session.account?.icon?.scheme) { - icon = vscode.Uri.from(session.account.icon); - } else if (userInfo?.avatarUrl) { - icon = vscode.Uri.parse(userInfo.avatarUrl); - } - const resolvedSession: vscode.AuthenticationSession = { + const icon = session.account?.icon + ? vscode.Uri.from(session.account.icon) + : userInfo?.avatarUrl ? vscode.Uri.parse(userInfo.avatarUrl) : undefined; + return { id: session.id, account: { label: session.account @@ -366,10 +331,6 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid scopes: session.scopes, accessToken: session.accessToken }; - if (!icon && (session.account?.icon === null || userInfo)) { - this._sessionsWithoutAvatars.add(resolvedSession); - } - return resolvedSession; }); const verifiedSessions = (await Promise.allSettled(sessionPromises)) @@ -378,7 +339,7 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid .filter((p?: T): p is T => Boolean(p)); this._logger.info(`Got ${verifiedSessions.length} verified sessions.`); - if (seenNumberAccountId || seenIconUpdate || verifiedSessions.length !== sessionData.length) { + if (seenNumberAccountId || seenAccountUpdate || verifiedSessions.length !== sessionData.length) { await this.storeSessions(verifiedSessions); } @@ -388,17 +349,7 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid private async storeSessions(sessions: vscode.AuthenticationSession[]): Promise { this._logger.info(`Storing ${sessions.length} sessions...`); this._sessionsPromise = Promise.resolve(sessions); - const storedSessions: SessionData[] = sessions.map(session => ({ - id: session.id, - account: { - label: session.account.label, - id: session.account.id, - icon: serializeAccountIcon(session.account.icon, this._sessionsWithoutAvatars.has(session)), - }, - scopes: [...session.scopes], - accessToken: session.accessToken - })); - await this._keychain.setToken(JSON.stringify(storedSessions)); + await this._keychain.setToken(JSON.stringify(sessions)); this._logger.info(`Stored ${sessions.length} sessions!`); } @@ -468,16 +419,12 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid private async tokenToSession(token: string, scopes: string[]): Promise { const userInfo = await this._githubServer.getUserInfo(token); - const session: vscode.AuthenticationSession = { + return { id: crypto.getRandomValues(new Uint32Array(2)).reduce((prev, curr) => prev += curr.toString(16), ''), accessToken: token, account: { label: userInfo.accountName, id: userInfo.id, icon: userInfo.avatarUrl ? vscode.Uri.parse(userInfo.avatarUrl) : undefined }, scopes }; - if (!session.account.icon) { - this._sessionsWithoutAvatars.add(session); - } - return session; } public async removeSession(id: string) { diff --git a/extensions/github-authentication/src/test/github.test.ts b/extensions/github-authentication/src/test/github.test.ts deleted file mode 100644 index e3a776d9d17e0d..00000000000000 --- a/extensions/github-authentication/src/test/github.test.ts +++ /dev/null @@ -1,43 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import * as assert from 'assert'; -import * as vscode from 'vscode'; -import { needsAccountIconLookup, serializeAccountIcon } from '../github'; - -suite('account avatar caching', () => { - test('a pending session needs a lookup, and its serialized no-avatar result no longer needs one', () => { - const pendingSession = { - id: 'session1', - account: { id: 'account1', label: 'Some One' }, - scopes: [], - accessToken: 'token' - }; - const noAvatarIcon = serializeAccountIcon(undefined, true); - const cachedNoAvatarSession = { - id: 'session1', - account: { id: 'account1', label: 'Some One', icon: noAvatarIcon }, - scopes: [], - accessToken: 'token' - }; - - assert.deepStrictEqual( - [needsAccountIconLookup(pendingSession), noAvatarIcon, needsAccountIconLookup(cachedNoAvatarSession)], - [true, null, false] - ); - }); - - test('a resolved avatar URI is serialized as-is and is never replaced by null', () => { - const icon = vscode.Uri.parse('https://example.com/avatar.png'); - - assert.deepStrictEqual(serializeAccountIcon(icon, true), { - scheme: icon.scheme, - authority: icon.authority, - path: icon.path, - query: icon.query, - fragment: icon.fragment - }); - }); -});