[PM-7169][PM-5267] Remove auth status from account info (#8539)

* remove active account unlocked from state service * Remove status from account service `AccountInfo` * Fixup lingering usages of status Fixup missed factories * Fixup account info usage * fixup CLI build * Fixup current account type * Add helper for all auth statuses to auth service * Fix tests * Uncomment mistakenly commented code * Rework logged out account exclusion tests * Correct test description * Avoid getters returning observables * fixup type
2025-12-12 22:33:35 +00:00 · 2024-04-12 02:25:45 -05:00
parent c7ea35280d
commit 8d698d9d84
31 changed files with 200 additions and 304 deletions
--- a/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts
+++ b/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts
@@ -6,6 +6,7 @@ import { Subject, firstValueFrom, map, switchMap, takeUntil } from "rxjs";
 import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service";
 import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
 import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
 import { VaultTimeoutAction } from "@bitwarden/common/enums/vault-timeout-action.enum";
 import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
@@ -32,6 +33,7 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy {
    private location: Location,
    private router: Router,
    private vaultTimeoutSettingsService: VaultTimeoutSettingsService,
+    private authService: AuthService,
  ) {}

  get accountLimit() {
@@ -42,13 +44,14 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy {
    return this.accountSwitcherService.SPECIAL_ADD_ACCOUNT_ID;
  }

-  get availableAccounts$() {
-    return this.accountSwitcherService.availableAccounts$;
-  }
-
-  get currentAccount$() {
-    return this.accountService.activeAccount$;
-  }
+  readonly availableAccounts$ = this.accountSwitcherService.availableAccounts$;
+  readonly currentAccount$ = this.accountService.activeAccount$.pipe(
+    switchMap((a) =>
+      a == null
+        ? null
+        : this.authService.activeAccountStatus$.pipe(map((s) => ({ ...a, status: s }))),
+    ),
+  );

  async ngOnInit() {
    const availableVaultTimeoutActions = await firstValueFrom(
--- a/apps/browser/src/auth/popup/account-switching/current-account.component.ts
+++ b/apps/browser/src/auth/popup/account-switching/current-account.component.ts
@@ -4,6 +4,7 @@ import { ActivatedRoute, Router } from "@angular/router";
 import { Observable, combineLatest, switchMap } from "rxjs";

 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+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 { UserId } from "@bitwarden/common/types/guid";
@@ -29,12 +30,14 @@ export class CurrentAccountComponent {
    private router: Router,
    private location: Location,
    private route: ActivatedRoute,
+    private authService: AuthService,
  ) {
    this.currentAccount$ = combineLatest([
      this.accountService.activeAccount$,
      this.avatarService.avatarColor$,
+      this.authService.activeAccountStatus$,
    ]).pipe(
-      switchMap(async ([account, avatarColor]) => {
+      switchMap(async ([account, avatarColor, accountStatus]) => {
        if (account == null) {
          return null;
        }
@@ -42,7 +45,7 @@ export class CurrentAccountComponent {
          id: account.id,
          name: account.name || account.email,
          email: account.email,
-          status: account.status,
+          status: accountStatus,
          avatarColor,
        };

--- 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
@@ -1,7 +1,8 @@
 import { matches, mock } from "jest-mock-extended";
-import { BehaviorSubject, firstValueFrom, of, timeout } from "rxjs";
+import { BehaviorSubject, ReplaySubject, firstValueFrom, of, timeout } from "rxjs";

 import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+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";
@@ -12,22 +13,29 @@ import { UserId } from "@bitwarden/common/types/guid";
 import { AccountSwitcherService } from "./account-switcher.service";

 describe("AccountSwitcherService", () => {
-  const accountsSubject = new BehaviorSubject<Record<UserId, AccountInfo>>(null);
-  const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);
+  let accountsSubject: BehaviorSubject<Record<UserId, AccountInfo>>;
+  let activeAccountSubject: BehaviorSubject<{ id: UserId } & AccountInfo>;
+  let authStatusSubject: ReplaySubject<Record<UserId, AuthenticationStatus>>;

  const accountService = mock<AccountService>();
  const avatarService = mock<AvatarService>();
  const messagingService = mock<MessagingService>();
  const environmentService = mock<EnvironmentService>();
  const logService = mock<LogService>();
+  const authService = mock<AuthService>();

  let accountSwitcherService: AccountSwitcherService;

  beforeEach(() => {
    jest.resetAllMocks();
+    accountsSubject = new BehaviorSubject<Record<UserId, AccountInfo>>(null);
+    activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);
+    authStatusSubject = new ReplaySubject<Record<UserId, AuthenticationStatus>>(1);

+    // Use subject to allow for easy updates
    accountService.accounts$ = accountsSubject;
    accountService.activeAccount$ = activeAccountSubject;
+    authService.authStatuses$ = authStatusSubject;

    accountSwitcherService = new AccountSwitcherService(
      accountService,
@@ -35,48 +43,59 @@ describe("AccountSwitcherService", () => {
      messagingService,
      environmentService,
      logService,
+      authService,
    );
  });

+  afterEach(() => {
+    accountsSubject.complete();
+    activeAccountSubject.complete();
+    authStatusSubject.complete();
+  });
+
  describe("availableAccounts$", () => {
-    it("should return all accounts and an add account option when accounts are less than 5", async () => {
-      const user1AccountInfo: AccountInfo = {
+    it("should return all logged in accounts and an add account option when accounts are less than 5", async () => {
+      const accountInfo: AccountInfo = {
        name: "Test User 1",
        email: "test1@email.com",
-        status: AuthenticationStatus.Unlocked,
      };

      avatarService.getUserAvatarColor$.mockReturnValue(of("#cccccc"));
-      accountsSubject.next({
-        "1": user1AccountInfo,
-      } as Record<UserId, AccountInfo>);
-
-      activeAccountSubject.next(Object.assign(user1AccountInfo, { id: "1" as UserId }));
+      accountsSubject.next({ ["1" as UserId]: accountInfo, ["2" as UserId]: accountInfo });
+      authStatusSubject.next({
+        ["1" as UserId]: AuthenticationStatus.Unlocked,
+        ["2" as UserId]: AuthenticationStatus.Locked,
+      });
+      activeAccountSubject.next(Object.assign(accountInfo, { id: "1" as UserId }));

      const accounts = await firstValueFrom(
        accountSwitcherService.availableAccounts$.pipe(timeout(20)),
      );
-      expect(accounts).toHaveLength(2);
+      expect(accounts).toHaveLength(3);
      expect(accounts[0].id).toBe("1");
      expect(accounts[0].isActive).toBeTruthy();
-
-      expect(accounts[1].id).toBe("addAccount");
+      expect(accounts[1].id).toBe("2");
      expect(accounts[1].isActive).toBeFalsy();
+
+      expect(accounts[2].id).toBe("addAccount");
+      expect(accounts[2].isActive).toBeFalsy();
    });

    it.each([5, 6])(
      "should return only accounts if there are %i accounts",
      async (numberOfAccounts) => {
        const seedAccounts: Record<UserId, AccountInfo> = {};
+        const seedStatuses: Record<UserId, AuthenticationStatus> = {};
        for (let i = 0; i < numberOfAccounts; i++) {
          seedAccounts[`${i}` as UserId] = {
            email: `test${i}@email.com`,
            name: "Test User ${i}",
-            status: AuthenticationStatus.Unlocked,
          };
+          seedStatuses[`${i}` as UserId] = AuthenticationStatus.Unlocked;
        }
        avatarService.getUserAvatarColor$.mockReturnValue(of("#cccccc"));
        accountsSubject.next(seedAccounts);
+        authStatusSubject.next(seedStatuses);
        activeAccountSubject.next(
          Object.assign(seedAccounts["1" as UserId], { id: "1" as UserId }),
        );
@@ -89,6 +108,26 @@ describe("AccountSwitcherService", () => {
        });
      },
    );
+
+    it("excludes logged out accounts", async () => {
+      const user1AccountInfo: AccountInfo = {
+        name: "Test User 1",
+        email: "",
+      };
+      accountsSubject.next({ ["1" as UserId]: user1AccountInfo });
+      authStatusSubject.next({ ["1" as UserId]: AuthenticationStatus.LoggedOut });
+      accountsSubject.next({
+        "1": user1AccountInfo,
+      } as Record<UserId, AccountInfo>);
+
+      const accounts = await firstValueFrom(
+        accountSwitcherService.availableAccounts$.pipe(timeout(20)),
+      );
+
+      // Add account only
+      expect(accounts).toHaveLength(1);
+      expect(accounts[0].id).toBe("addAccount");
+    });
  });

  describe("selectAccount", () => {
--- 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
@@ -11,6 +11,7 @@ import {
 } from "rxjs";

 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+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";
@@ -48,25 +49,27 @@ export class AccountSwitcherService {
    private messagingService: MessagingService,
    private environmentService: EnvironmentService,
    private logService: LogService,
+    authService: AuthService,
  ) {
    this.availableAccounts$ = combineLatest([
-      this.accountService.accounts$,
+      accountService.accounts$,
+      authService.authStatuses$,
      this.accountService.activeAccount$,
    ]).pipe(
-      switchMap(async ([accounts, activeAccount]) => {
-        const accountEntries = Object.entries(accounts).filter(
-          ([_, account]) => account.status !== AuthenticationStatus.LoggedOut,
+      switchMap(async ([accounts, accountStatuses, activeAccount]) => {
+        const loggedInIds = Object.keys(accounts).filter(
+          (id: UserId) => accountStatuses[id] !== AuthenticationStatus.LoggedOut,
        );
        // Accounts shouldn't ever be more than ACCOUNT_LIMIT but just in case do a greater than
-        const hasMaxAccounts = accountEntries.length >= this.ACCOUNT_LIMIT;
+        const hasMaxAccounts = loggedInIds.length >= this.ACCOUNT_LIMIT;
        const options: AvailableAccount[] = await Promise.all(
-          accountEntries.map(async ([id, account]) => {
+          loggedInIds.map(async (id: UserId) => {
            return {
-              name: account.name ?? account.email,
-              email: account.email,
+              name: accounts[id].name ?? accounts[id].email,
+              email: accounts[id].email,
              id: id,
              server: (await this.environmentService.getEnvironment(id))?.getHostname(),
-              status: account.status,
+              status: accountStatuses[id],
              isActive: id === activeAccount?.id,
              avatarColor: await firstValueFrom(
                this.avatarService.getUserAvatarColor$(id as UserId),
--- a/apps/browser/src/background/main.background.ts
+++ b/apps/browser/src/background/main.background.ts
@@ -779,14 +779,14 @@ export default class MainBackground {
      this.apiService,
      this.stateProvider,
      this.logService,
-      this.accountService,
+      this.authService,
    );
    this.eventCollectionService = new EventCollectionService(
      this.cipherService,
      this.stateProvider,
      this.organizationService,
      this.eventUploadService,
-      this.accountService,
+      this.authService,
    );
    this.totpService = new TotpService(this.cryptoFunctionService, this.logService);

--- a/apps/browser/src/background/service-factories/event-collection-service.factory.ts
+++ b/apps/browser/src/background/service-factories/event-collection-service.factory.ts
@@ -5,7 +5,10 @@ import {
  organizationServiceFactory,
  OrganizationServiceInitOptions,
 } from "../../admin-console/background/service-factories/organization-service.factory";
-import { accountServiceFactory } from "../../auth/background/service-factories/account-service.factory";
+import {
+  authServiceFactory,
+  AuthServiceInitOptions,
+} from "../../auth/background/service-factories/auth-service.factory";
 import {
  FactoryOptions,
  CachedServices,
@@ -29,7 +32,8 @@ export type EventCollectionServiceInitOptions = EventCollectionServiceOptions &
  CipherServiceInitOptions &
  StateServiceInitOptions &
  OrganizationServiceInitOptions &
-  EventUploadServiceInitOptions;
+  EventUploadServiceInitOptions &
+  AuthServiceInitOptions;

 export function eventCollectionServiceFactory(
  cache: { eventCollectionService?: AbstractEventCollectionService } & CachedServices,
@@ -45,7 +49,7 @@ export function eventCollectionServiceFactory(
        await stateProviderFactory(cache, opts),
        await organizationServiceFactory(cache, opts),
        await eventUploadServiceFactory(cache, opts),
-        await accountServiceFactory(cache, opts),
+        await authServiceFactory(cache, opts),
      ),
  );
 }
--- a/apps/browser/src/background/service-factories/event-upload-service.factory.ts
+++ b/apps/browser/src/background/service-factories/event-upload-service.factory.ts
@@ -1,7 +1,10 @@
 import { EventUploadService as AbstractEventUploadService } from "@bitwarden/common/abstractions/event/event-upload.service";
 import { EventUploadService } from "@bitwarden/common/services/event/event-upload.service";

-import { accountServiceFactory } from "../../auth/background/service-factories/account-service.factory";
+import {
+  AuthServiceInitOptions,
+  authServiceFactory,
+} from "../../auth/background/service-factories/auth-service.factory";
 import {
  ApiServiceInitOptions,
  apiServiceFactory,
@@ -23,7 +26,8 @@ type EventUploadServiceOptions = FactoryOptions;
 export type EventUploadServiceInitOptions = EventUploadServiceOptions &
  ApiServiceInitOptions &
  StateServiceInitOptions &
-  LogServiceInitOptions;
+  LogServiceInitOptions &
+  AuthServiceInitOptions;

 export function eventUploadServiceFactory(
  cache: { eventUploadService?: AbstractEventUploadService } & CachedServices,
@@ -38,7 +42,7 @@ export function eventUploadServiceFactory(
        await apiServiceFactory(cache, opts),
        await stateProviderFactory(cache, opts),
        await logServiceFactory(cache, opts),
-        await accountServiceFactory(cache, opts),
+        await authServiceFactory(cache, opts),
      ),
  );
 }
--- a/apps/browser/src/platform/popup/header.component.ts
+++ b/apps/browser/src/platform/popup/header.component.ts
@@ -1,8 +1,10 @@
 import { Component, Input } from "@angular/core";
-import { Observable, map } from "rxjs";
+import { Observable, combineLatest, map, of, switchMap } from "rxjs";

 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
 import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
+import { UserId } from "@bitwarden/common/types/guid";

 import { enableAccountSwitching } from "../flags";

@@ -14,14 +16,18 @@ export class HeaderComponent {
  @Input() noTheme = false;
  @Input() hideAccountSwitcher = false;
  authedAccounts$: Observable<boolean>;
-  constructor(accountService: AccountService) {
+  constructor(accountService: AccountService, authService: AuthService) {
    this.authedAccounts$ = accountService.accounts$.pipe(
-      map((accounts) => {
+      switchMap((accounts) => {
        if (!enableAccountSwitching()) {
-          return false;
+          return of(false);
        }

-        return Object.values(accounts).some((a) => a.status !== AuthenticationStatus.LoggedOut);
+        return combineLatest(
+          Object.keys(accounts).map((id) => authService.authStatusFor$(id as UserId)),
+        ).pipe(
+          map((statuses) => statuses.some((status) => status !== AuthenticationStatus.LoggedOut)),
+        );
      }),
    );
  }
--- a/apps/browser/src/popup/app.component.ts
+++ b/apps/browser/src/popup/app.component.ts
@@ -1,9 +1,10 @@
 import { ChangeDetectorRef, Component, NgZone, OnDestroy, OnInit } from "@angular/core";
 import { NavigationEnd, Router, RouterOutlet } from "@angular/router";
 import { ToastrService } from "ngx-toastr";
-import { filter, concatMap, Subject, takeUntil, firstValueFrom } from "rxjs";
+import { filter, concatMap, Subject, takeUntil, firstValueFrom, map } from "rxjs";

 import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
+import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
 import { BroadcasterService } from "@bitwarden/common/platform/abstractions/broadcaster.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { DialogService, SimpleDialogOptions } from "@bitwarden/components";
@@ -57,8 +58,9 @@ export class AppComponent implements OnInit, OnDestroy {
      this.activeUserId = userId;
    });

-    this.stateService.activeAccountUnlocked$
+    this.authService.activeAccountStatus$
      .pipe(
+        map((status) => status === AuthenticationStatus.Unlocked),
        filter((unlocked) => unlocked),
        concatMap(async () => {
          await this.recordActivity();
--- a/apps/cli/src/bw.ts
+++ b/apps/cli/src/bw.ts
@@ -678,7 +678,7 @@ export class Main {
      this.apiService,
      this.stateProvider,
      this.logService,
-      this.accountService,
+      this.authService,
    );

    this.eventCollectionService = new EventCollectionService(
@@ -686,7 +686,7 @@ export class Main {
      this.stateProvider,
      this.organizationService,
      this.eventUploadService,
-      this.accountService,
+      this.authService,
    );
  }