From d5c2817e69f7d8cce7e9e9af74fd7659b2eea633 Mon Sep 17 00:00:00 2001 From: Tyler James Leonhardt Date: Thu, 21 Sep 2023 14:09:25 -0700 Subject: [PATCH] Use email for label & use label to group results in Account menu (#193727) So, when you make a new session in the Microsoft Identity stack, depending on the scopes you pass in you might get: * A token with a name & email * A token with just a name Additionally, Microsoft has like 3-4 concepts of essentially an "id" but depending on what you're trying to access, you might get a different value. This historical behavior leads to 2 awkward things: 1. The account menu shows two accounts, one with name & email, one with just email. 2. The account menu shows two of the same accounts, but their underlying id is different So, to fix this, we're just gonna lean on the labels. In other words, if there are two accounts that share the same label, then they will be grouped together. The previous behavior was hurting the Azure Account folks and the Q# folks and with this change, I believe they both should be happy. Interestingly enough, when I inherited this code, it use to do this, but I changed it to use the id as that seemed "more correct"... so it a way, this is reverting a change I did a while back. Fixes https://github.com/microsoft/vscode/issues/184218 --- .../microsoft-authentication/src/AADHelper.ts | 9 +--- .../parts/activitybar/activitybarActions.ts | 45 ++++++++++--------- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/extensions/microsoft-authentication/src/AADHelper.ts b/extensions/microsoft-authentication/src/AADHelper.ts index 5e72873b3ef..c87d40fe51c 100644 --- a/extensions/microsoft-authentication/src/AADHelper.ts +++ b/extensions/microsoft-authentication/src/AADHelper.ts @@ -524,13 +524,6 @@ export class AzureActiveDirectoryService { throw e; } - let label; - if (claims.name && claims.email) { - label = `${claims.name} - ${claims.email}`; - } else { - label = claims.email ?? claims.unique_name ?? claims.preferred_username ?? 'user@example.com'; - } - const id = `${claims.tid}/${(claims.oid ?? (claims.altsecid ?? '' + claims.ipd ?? ''))}`; const sessionId = existingId || `${id}/${randomUUID()}`; this._logger.trace(`[${scopeData.scopeStr}] '${sessionId}' Token response parsed successfully.`); @@ -543,7 +536,7 @@ export class AzureActiveDirectoryService { scope: scopeData.scopeStr, sessionId, account: { - label, + label: claims.email ?? claims.preferred_username ?? claims.unique_name ?? 'user@example.com', id, type: claims.tid === MSA_TID || claims.tid === MSA_PASSTHRU_TID ? MicrosoftAccountType.MSA : MicrosoftAccountType.AAD } diff --git a/src/vs/workbench/browser/parts/activitybar/activitybarActions.ts b/src/vs/workbench/browser/parts/activitybar/activitybarActions.ts index 3fc1bdc4e82..72be1131bda 100644 --- a/src/vs/workbench/browser/parts/activitybar/activitybarActions.ts +++ b/src/vs/workbench/browser/parts/activitybar/activitybarActions.ts @@ -351,7 +351,7 @@ export class AccountsActivityActionViewItem extends MenuActivityActionViewItem { if (account.canSignOut) { const signOutAction = disposables.add(new Action('signOut', localize('signOut', "Sign Out"), undefined, true, async () => { const allSessions = await this.authenticationService.getSessions(providerId); - const sessionsForAccount = allSessions.filter(s => s.account.id === account.id); + const sessionsForAccount = allSessions.filter(s => s.account.label === account.label); return await this.authenticationService.removeAccountSessions(providerId, account.label, sessionsForAccount); })); providerSubMenuActions.push(signOutAction); @@ -399,34 +399,35 @@ export class AccountsActivityActionViewItem extends MenuActivityActionViewItem { private async addOrUpdateAccount(providerId: string, account: AuthenticationSessionAccount): Promise { let accounts = this.groupedAccounts.get(providerId); - if (accounts) { - const existingAccount = accounts.find(a => a.id === account.id); - if (existingAccount) { - // Update the label if it has changed - if (existingAccount.label !== account.label) { - existingAccount.label = account.label; - } - return; - } - } else { + if (!accounts) { accounts = []; this.groupedAccounts.set(providerId, accounts); } const sessionFromEmbedder = await this.sessionFromEmbedder.value; - // If the session stored from the embedder allows sign out, then we can treat it and all others as sign out-able - let canSignOut = !!sessionFromEmbedder?.canSignOut; - if (!canSignOut) { - if (sessionFromEmbedder?.id) { - const sessions = (await this.authenticationService.getSessions(providerId)).filter(s => s.account.id === account.id); - canSignOut = !sessions.some(s => s.id === sessionFromEmbedder.id); - } else { - // The default if we don't have a session from the embedder is to allow sign out - canSignOut = true; - } + let canSignOut = true; + if ( + sessionFromEmbedder // if we have a session from the embedder + && !sessionFromEmbedder.canSignOut // and that session says we can't sign out + && (await this.authenticationService.getSessions(providerId)) // and that session is associated with the account we are adding/updating + .some(s => + s.id === sessionFromEmbedder.id + && s.account.id === account.id + ) + ) { + canSignOut = false; } - accounts.push({ ...account, canSignOut }); + const existingAccount = accounts.find(a => a.label === account.label); + if (existingAccount) { + // if we have an existing account and we discover that we + // can't sign out of it, update the account to mark it as "can't sign out" + if (!canSignOut) { + existingAccount.canSignOut = canSignOut; + } + } else { + accounts.push({ ...account, canSignOut }); + } } private removeAccount(providerId: string, account: AuthenticationSessionAccount): void {