From 44d50a70c25249a82b6db40341b9f3a4f6965135 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Tue, 25 Feb 2025 17:58:26 -0500 Subject: [PATCH] Auth/PM-5712 - Extension & Desktop Account Switcher - Fix incorrect env showing when adding new accounts (#13362) * PM-5712 - Refactor env service to require user id instead of having global and active user state fallbacks per working session with Justin. * PM-5712 - AccountSwitcherService tests - fix tests and add env assertions. --- .../services/account-switcher.service.spec.ts | 19 +++- .../services/account-switcher.service.ts | 3 +- .../app/layout/account-switcher.component.ts | 8 +- .../abstractions/environment.service.ts | 2 +- .../default-environment.service.spec.ts | 104 ++++++------------ .../services/default-environment.service.ts | 20 +--- 6 files changed, 65 insertions(+), 91 deletions(-) diff --git a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts index 3084c3e5407..1fdd0b1ecf2 100644 --- a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts +++ b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts @@ -9,7 +9,10 @@ import { import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AvatarService } from "@bitwarden/common/auth/abstractions/avatar.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; -import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; +import { + Environment, + EnvironmentService, +} from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { UserId } from "@bitwarden/common/types/guid"; @@ -21,6 +24,12 @@ describe("AccountSwitcherService", () => { let activeAccountSubject: BehaviorSubject; let authStatusSubject: ReplaySubject>; + let envBSubject: BehaviorSubject; + const mockHostName = "mockHostName"; + const mockEnv: Partial = { + getHostname: () => mockHostName, + }; + const accountService = mock(); const avatarService = mock(); const messagingService = mock(); @@ -41,6 +50,9 @@ describe("AccountSwitcherService", () => { accountService.activeAccount$ = activeAccountSubject; authService.authStatuses$ = authStatusSubject; + envBSubject = new BehaviorSubject(mockEnv as Environment); + environmentService.getEnvironment$.mockReturnValue(envBSubject); + accountSwitcherService = new AccountSwitcherService( accountService, avatarService, @@ -79,11 +91,16 @@ describe("AccountSwitcherService", () => { expect(accounts).toHaveLength(3); expect(accounts[0].id).toBe("1"); expect(accounts[0].isActive).toBeTruthy(); + + expect(accounts[0].server).toBe(mockHostName); + expect(accounts[1].id).toBe("2"); expect(accounts[1].isActive).toBeFalsy(); + expect(accounts[1].server).toBe(mockHostName); expect(accounts[2].id).toBe("addAccount"); expect(accounts[2].isActive).toBeFalsy(); + expect(accounts[2].server).toBe(undefined); }); it.each([5, 6])( diff --git a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts index 535df3ec6bb..bfed7dc1408 100644 --- a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts +++ b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts @@ -66,11 +66,12 @@ export class AccountSwitcherService { const hasMaxAccounts = loggedInIds.length >= this.ACCOUNT_LIMIT; const options: AvailableAccount[] = await Promise.all( loggedInIds.map(async (id: UserId) => { + const userEnv = await firstValueFrom(this.environmentService.getEnvironment$(id)); return { name: accounts[id].name ?? accounts[id].email, email: accounts[id].email, id: id, - server: (await this.environmentService.getEnvironment(id))?.getHostname(), + server: userEnv?.getHostname(), status: accountStatuses[id], isActive: id === activeAccount?.id, avatarColor: await firstValueFrom( diff --git a/apps/desktop/src/app/layout/account-switcher.component.ts b/apps/desktop/src/app/layout/account-switcher.component.ts index db8c2a85bde..d8ffa5ae546 100644 --- a/apps/desktop/src/app/layout/account-switcher.component.ts +++ b/apps/desktop/src/app/layout/account-switcher.component.ts @@ -110,7 +110,9 @@ export class AccountSwitcherComponent implements OnInit { name: active.name, email: active.email, avatarColor: await firstValueFrom(this.avatarService.avatarColor$), - server: (await this.environmentService.getEnvironment())?.getHostname(), + server: ( + await firstValueFrom(this.environmentService.getEnvironment$(active.id)) + )?.getHostname(), }; }), ); @@ -221,7 +223,9 @@ export class AccountSwitcherComponent implements OnInit { email: baseAccounts[userId].email, authenticationStatus: await this.authService.getAuthStatus(userId), avatarColor: await firstValueFrom(this.avatarService.getUserAvatarColor$(userId as UserId)), - server: (await this.environmentService.getEnvironment(userId))?.getHostname(), + server: ( + await firstValueFrom(this.environmentService.getEnvironment$(userId as UserId)) + )?.getHostname(), }; } diff --git a/libs/common/src/platform/abstractions/environment.service.ts b/libs/common/src/platform/abstractions/environment.service.ts index 8d32fc4231d..4a10f856893 100644 --- a/libs/common/src/platform/abstractions/environment.service.ts +++ b/libs/common/src/platform/abstractions/environment.service.ts @@ -128,7 +128,7 @@ export abstract class EnvironmentService { /** * Get the environment from state. Useful if you need to get the environment for another user. */ - abstract getEnvironment$(userId?: string): Observable; + abstract getEnvironment$(userId: UserId): Observable; /** * @deprecated Use {@link getEnvironment$} instead. diff --git a/libs/common/src/platform/services/default-environment.service.spec.ts b/libs/common/src/platform/services/default-environment.service.spec.ts index 870f887c160..553f80f83b8 100644 --- a/libs/common/src/platform/services/default-environment.service.spec.ts +++ b/libs/common/src/platform/services/default-environment.service.spec.ts @@ -304,85 +304,21 @@ describe("EnvironmentService", () => { }); }); - describe("getEnvironment", () => { + describe("getEnvironment$", () => { it.each([ { region: Region.US, expectedHost: "bitwarden.com" }, { region: Region.EU, expectedHost: "bitwarden.eu" }, - ])("gets it from user data if there is an active user", async ({ region, expectedHost }) => { - setGlobalData(Region.US, new EnvironmentUrls()); - setUserData(region, new EnvironmentUrls()); + ])("gets it from the passed in userId: %s", async ({ region, expectedHost }) => { + setUserData(Region.US, new EnvironmentUrls()); + setUserData(region, new EnvironmentUrls(), alternateTestUser); await switchUser(testUser); - const env = await firstValueFrom(sut.getEnvironment$()); - expect(env.getHostname()).toBe(expectedHost); + const env = await firstValueFrom(sut.getEnvironment$(alternateTestUser)); + expect(env?.getHostname()).toBe(expectedHost); }); - it.each([ - { region: Region.US, expectedHost: "bitwarden.com" }, - { region: Region.EU, expectedHost: "bitwarden.eu" }, - ])("gets it from global data if there is no active user", async ({ region, expectedHost }) => { - setGlobalData(region, new EnvironmentUrls()); - setUserData(Region.US, new EnvironmentUrls()); - - const env = await firstValueFrom(sut.getEnvironment$()); - expect(env.getHostname()).toBe(expectedHost); - }); - - it.each([ - { region: Region.US, expectedHost: "bitwarden.com" }, - { region: Region.EU, expectedHost: "bitwarden.eu" }, - ])( - "gets it from global state if there is no active user even if a user id is passed in.", - async ({ region, expectedHost }) => { - setGlobalData(region, new EnvironmentUrls()); - setUserData(Region.US, new EnvironmentUrls()); - - const env = await firstValueFrom(sut.getEnvironment$(testUser)); - expect(env.getHostname()).toBe(expectedHost); - }, - ); - - it.each([ - { region: Region.US, expectedHost: "bitwarden.com" }, - { region: Region.EU, expectedHost: "bitwarden.eu" }, - ])( - "gets it from the passed in userId if there is any active user: %s", - async ({ region, expectedHost }) => { - setGlobalData(Region.US, new EnvironmentUrls()); - setUserData(Region.US, new EnvironmentUrls()); - setUserData(region, new EnvironmentUrls(), alternateTestUser); - - await switchUser(testUser); - - const env = await firstValueFrom(sut.getEnvironment$(alternateTestUser)); - expect(env.getHostname()).toBe(expectedHost); - }, - ); - - it("gets it from base url saved in self host config", async () => { - const globalSelfHostUrls = new EnvironmentUrls(); - globalSelfHostUrls.base = "https://base.example.com"; - setGlobalData(Region.SelfHosted, globalSelfHostUrls); - setUserData(Region.EU, new EnvironmentUrls()); - - const env = await firstValueFrom(sut.getEnvironment$()); - expect(env.getHostname()).toBe("base.example.com"); - }); - - it("gets it from webVault url saved in self host config", async () => { - const globalSelfHostUrls = new EnvironmentUrls(); - globalSelfHostUrls.webVault = "https://vault.example.com"; - globalSelfHostUrls.base = "https://base.example.com"; - setGlobalData(Region.SelfHosted, globalSelfHostUrls); - setUserData(Region.EU, new EnvironmentUrls()); - - const env = await firstValueFrom(sut.getEnvironment$()); - expect(env.getHostname()).toBe("vault.example.com"); - }); - - it("gets it from saved self host config from passed in user when there is an active user", async () => { - setGlobalData(Region.US, new EnvironmentUrls()); + it("gets env from saved self host config from passed in user when there is a different active user", async () => { setUserData(Region.EU, new EnvironmentUrls()); const selfHostUserUrls = new EnvironmentUrls(); @@ -392,7 +328,31 @@ describe("EnvironmentService", () => { await switchUser(testUser); const env = await firstValueFrom(sut.getEnvironment$(alternateTestUser)); - expect(env.getHostname()).toBe("base.example.com"); + expect(env?.getHostname()).toBe("base.example.com"); + }); + }); + + describe("getEnvironment (deprecated)", () => { + it("gets self hosted env from active user when no user passed in", async () => { + const selfHostUserUrls = new EnvironmentUrls(); + selfHostUserUrls.base = "https://base.example.com"; + setUserData(Region.SelfHosted, selfHostUserUrls); + + await switchUser(testUser); + + const env = await sut.getEnvironment(); + expect(env?.getHostname()).toBe("base.example.com"); + }); + + it("gets self hosted env from passed in user", async () => { + const selfHostUserUrls = new EnvironmentUrls(); + selfHostUserUrls.base = "https://base.example.com"; + setUserData(Region.SelfHosted, selfHostUserUrls); + + await switchUser(testUser); + + const env = await sut.getEnvironment(testUser); + expect(env?.getHostname()).toBe("base.example.com"); }); }); diff --git a/libs/common/src/platform/services/default-environment.service.ts b/libs/common/src/platform/services/default-environment.service.ts index ac3e39b2bb3..df55693ba0b 100644 --- a/libs/common/src/platform/services/default-environment.service.ts +++ b/libs/common/src/platform/services/default-environment.service.ts @@ -271,19 +271,8 @@ export class DefaultEnvironmentService implements EnvironmentService { } } - getEnvironment$(userId?: UserId): Observable { - if (userId == null) { - return this.environment$; - } - - return this.activeAccountId$.pipe( - switchMap((activeUserId) => { - // Previous rules dictated that we only get from user scoped state if there is an active user. - if (activeUserId == null) { - return this.globalState.state$; - } - return this.stateProvider.getUser(userId ?? activeUserId, USER_ENVIRONMENT_KEY).state$; - }), + getEnvironment$(userId: UserId): Observable { + return this.stateProvider.getUser(userId, USER_ENVIRONMENT_KEY).state$.pipe( map((state) => { return this.buildEnvironment(state?.region, state?.urls); }), @@ -294,7 +283,10 @@ export class DefaultEnvironmentService implements EnvironmentService { * @deprecated Use getEnvironment$ instead. */ async getEnvironment(userId?: UserId): Promise { - return firstValueFrom(this.getEnvironment$(userId)); + // Add backwards compatibility support for null userId + const definedUserId = userId ?? (await firstValueFrom(this.activeAccountId$)); + + return firstValueFrom(this.getEnvironment$(definedUserId)); } async seedUserEnvironment(userId: UserId) {