Auth/PM-5712 - Extension & Desktop Account Switcher - Fix incorrect e…

…nv 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.
bitwarden · Feb 25, 2025 · 44d50a7 · 44d50a7
1 parent cec1174
commit 44d50a7
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 87 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
@@ -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<Account | null>;
   let authStatusSubject: ReplaySubject<Record<UserId, AuthenticationStatus>>;
 
+  let envBSubject: BehaviorSubject<Environment | undefined>;
+  const mockHostName = "mockHostName";
+  const mockEnv: Partial<Environment> = {
+    getHostname: () => mockHostName,
+  };
+
   const accountService = mock<AccountService>();
   const avatarService = mock<AvatarService>();
   const messagingService = mock<MessagingService>();
@@ -41,6 +50,9 @@ describe("AccountSwitcherService", () => {
     accountService.activeAccount$ = activeAccountSubject;
     authService.authStatuses$ = authStatusSubject;
 
+    envBSubject = new BehaviorSubject<Environment | undefined>(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
@@ -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
@@ -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
@@ -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<Environment | undefined>;
+  abstract getEnvironment$(userId: UserId): Observable<Environment | undefined>;
 
   /**
    * @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
@@ -304,95 +304,55 @@ 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);
-    });
-
-    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);
+      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 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);
+    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 env = await firstValueFrom(sut.getEnvironment$(alternateTestUser));
-        expect(env.getHostname()).toBe(expectedHost);
-      },
-    );
+      const selfHostUserUrls = new EnvironmentUrls();
+      selfHostUserUrls.base = "https://base.example.com";
+      setUserData(Region.SelfHosted, selfHostUserUrls, alternateTestUser);
 
-    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());
+      await switchUser(testUser);
 
-      const env = await firstValueFrom(sut.getEnvironment$());
-      expect(env.getHostname()).toBe("base.example.com");
+      const env = await firstValueFrom(sut.getEnvironment$(alternateTestUser));
+      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());
+  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);
 
-      const env = await firstValueFrom(sut.getEnvironment$());
-      expect(env.getHostname()).toBe("vault.example.com");
-    });
+      await switchUser(testUser);
 
-    it("gets it from saved self host config from passed in user when there is an active user", async () => {
-      setGlobalData(Region.US, new EnvironmentUrls());
-      setUserData(Region.EU, new EnvironmentUrls());
+      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, alternateTestUser);
+      setUserData(Region.SelfHosted, selfHostUserUrls);
 
       await switchUser(testUser);
 
-      const env = await firstValueFrom(sut.getEnvironment$(alternateTestUser));
-      expect(env.getHostname()).toBe("base.example.com");
+      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
@@ -271,19 +271,8 @@ export class DefaultEnvironmentService implements EnvironmentService {
     }
   }
 
-  getEnvironment$(userId?: UserId): Observable<Environment | undefined> {
-    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<Environment | undefined> {
+    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<Environment | undefined> {
-    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) {