[PM-5266] Create Avatar Service (#7905)

* rename file, move, and update imports * refactor and implement StateProvider * remove comments * add migration * use 'disk-local' for web * add JSDoc comments * move AvatarService before SyncService * create factory * replace old method with observable in story * fix tests * add tests for migration * receive most recent avatarColor emission * move logic to component * fix CLI dependency * remove BehaviorSubject * cleanup * use UserKeyDefinition * avoid extra write * convert to observable * fix tests
2026-01-07 11:03:30 +00:00 · 2024-03-14 09:56:48 -07:00
parent 10d503c15f
commit 65b7ca7177
25 changed files with 403 additions and 165 deletions
--- a/apps/browser/src/auth/background/service-factories/avatar-service.factory.ts
+++ b/apps/browser/src/auth/background/service-factories/avatar-service.factory.ts
@@ -0,0 +1,38 @@
+import { AvatarService as AvatarServiceAbstraction } from "@bitwarden/common/auth/abstractions/avatar.service";
+import { AvatarService } from "@bitwarden/common/auth/services/avatar.service";
+
+import {
+  ApiServiceInitOptions,
+  apiServiceFactory,
+} from "../../../platform/background/service-factories/api-service.factory";
+import {
+  CachedServices,
+  factory,
+  FactoryOptions,
+} from "../../../platform/background/service-factories/factory-options";
+import {
+  stateProviderFactory,
+  StateProviderInitOptions,
+} from "../../../platform/background/service-factories/state-provider.factory";
+
+type AvatarServiceFactoryOptions = FactoryOptions;
+
+export type AvatarServiceInitOptions = AvatarServiceFactoryOptions &
+  ApiServiceInitOptions &
+  StateProviderInitOptions;
+
+export function avatarServiceFactory(
+  cache: { avatarService?: AvatarServiceAbstraction } & CachedServices,
+  opts: AvatarServiceInitOptions,
+): Promise<AvatarServiceAbstraction> {
+  return factory(
+    cache,
+    "avatarService",
+    opts,
+    async () =>
+      new AvatarService(
+        await apiServiceFactory(cache, opts),
+        await stateProviderFactory(cache, opts),
+      ),
+  );
+}
--- a/apps/browser/src/auth/popup/account-switching/current-account.component.ts
+++ b/apps/browser/src/auth/popup/account-switching/current-account.component.ts
@@ -1,32 +1,61 @@
 import { Location } from "@angular/common";
 import { Component } from "@angular/core";
 import { ActivatedRoute, Router } from "@angular/router";
+import { Observable, combineLatest, switchMap } from "rxjs";

-import { CurrentAccountService } from "./services/current-account.service";
+import { AccountService } from "@bitwarden/common/auth/abstractions/account.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";
+
+export type CurrentAccount = {
+  id: UserId;
+  name: string | undefined;
+  email: string;
+  status: AuthenticationStatus;
+  avatarColor: string;
+};

@Component({
  selector: "app-current-account",
  templateUrl: "current-account.component.html",
 })
 export class CurrentAccountComponent {
+  currentAccount$: Observable<CurrentAccount>;
+
  constructor(
-    private currentAccountService: CurrentAccountService,
+    private accountService: AccountService,
+    private avatarService: AvatarService,
    private router: Router,
    private location: Location,
    private route: ActivatedRoute,
-  ) {}
+  ) {
+    this.currentAccount$ = combineLatest([
+      this.accountService.activeAccount$,
+      this.avatarService.avatarColor$,
+    ]).pipe(
+      switchMap(async ([account, avatarColor]) => {
+        if (account == null) {
+          return null;
+        }
+        const currentAccount: CurrentAccount = {
+          id: account.id,
+          name: account.name || account.email,
+          email: account.email,
+          status: account.status,
+          avatarColor,
+        };

-  get currentAccount$() {
-    return this.currentAccountService.currentAccount$;
+        return currentAccount;
+      }),
+    );
  }

  async currentAccountClicked() {
    if (this.route.snapshot.data.state.includes("account-switcher")) {
      this.location.back();
    } else {
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      this.router.navigate(["/account-switcher"]);
+      await this.router.navigate(["/account-switcher"]);
    }
  }
 }
--- 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,12 +1,12 @@
 import { matches, mock } from "jest-mock-extended";
-import { BehaviorSubject, firstValueFrom, timeout } from "rxjs";
+import { BehaviorSubject, firstValueFrom, of, timeout } from "rxjs";

 import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.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 { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
-import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
 import { UserId } from "@bitwarden/common/types/guid";

 import { AccountSwitcherService } from "./account-switcher.service";
@@ -16,7 +16,7 @@ describe("AccountSwitcherService", () => {
  const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);

  const accountService = mock<AccountService>();
-  const stateService = mock<StateService>();
+  const avatarService = mock<AvatarService>();
  const messagingService = mock<MessagingService>();
  const environmentService = mock<EnvironmentService>();
  const logService = mock<LogService>();
@@ -25,11 +25,13 @@ describe("AccountSwitcherService", () => {

  beforeEach(() => {
    jest.resetAllMocks();
+
    accountService.accounts$ = accountsSubject;
    accountService.activeAccount$ = activeAccountSubject;
+
    accountSwitcherService = new AccountSwitcherService(
      accountService,
-      stateService,
+      avatarService,
      messagingService,
      environmentService,
      logService,
@@ -44,6 +46,7 @@ describe("AccountSwitcherService", () => {
        status: AuthenticationStatus.Unlocked,
      };

+      avatarService.getUserAvatarColor$.mockReturnValue(of("#cccccc"));
      accountsSubject.next({
        "1": user1AccountInfo,
      } as Record<UserId, AccountInfo>);
@@ -72,6 +75,7 @@ describe("AccountSwitcherService", () => {
            status: AuthenticationStatus.Unlocked,
          };
        }
+        avatarService.getUserAvatarColor$.mockReturnValue(of("#cccccc"));
        accountsSubject.next(seedAccounts);
        activeAccountSubject.next(
          Object.assign(seedAccounts["1" as UserId], { id: "1" as UserId }),
--- 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,11 +11,11 @@ import {
 } from "rxjs";

 import { AccountService } from "@bitwarden/common/auth/abstractions/account.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 { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
-import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
 import { UserId } from "@bitwarden/common/types/guid";

 import { fromChromeEvent } from "../../../../platform/browser/from-chrome-event";
@@ -44,7 +44,7 @@ export class AccountSwitcherService {

  constructor(
    private accountService: AccountService,
-    private stateService: StateService,
+    private avatarService: AvatarService,
    private messagingService: MessagingService,
    private environmentService: EnvironmentService,
    private logService: LogService,
@@ -68,7 +68,9 @@ export class AccountSwitcherService {
              server: await this.environmentService.getHost(id),
              status: account.status,
              isActive: id === activeAccount?.id,
-              avatarColor: await this.stateService.getAvatarColor({ userId: id }),
+              avatarColor: await firstValueFrom(
+                this.avatarService.getUserAvatarColor$(id as UserId),
+              ),
            };
          }),
        );
--- a/apps/browser/src/auth/popup/account-switching/services/current-account.service.ts
+++ b/apps/browser/src/auth/popup/account-switching/services/current-account.service.ts
@@ -1,44 +0,0 @@
-import { Injectable } from "@angular/core";
-import { Observable, switchMap } from "rxjs";
-
-import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
-import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
-import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
-import { UserId } from "@bitwarden/common/types/guid";
-
-export type CurrentAccount = {
-  id: UserId;
-  name: string | undefined;
-  email: string;
-  status: AuthenticationStatus;
-  avatarColor: string;
-};
-
-@Injectable({
-  providedIn: "root",
-})
-export class CurrentAccountService {
-  currentAccount$: Observable<CurrentAccount>;
-
-  constructor(
-    private accountService: AccountService,
-    private stateService: StateService,
-  ) {
-    this.currentAccount$ = this.accountService.activeAccount$.pipe(
-      switchMap(async (account) => {
-        if (account == null) {
-          return null;
-        }
-        const currentAccount: CurrentAccount = {
-          id: account.id,
-          name: account.name || account.email,
-          email: account.email,
-          status: account.status,
-          avatarColor: await this.stateService.getAvatarColor({ userId: account.id }),
-        };
-
-        return currentAccount;
-      }),
-    );
-  }
-}
--- a/apps/browser/src/background/main.background.ts
+++ b/apps/browser/src/background/main.background.ts
@@ -8,7 +8,6 @@ import {
  AuthRequestServiceAbstraction,
  AuthRequestService,
 } from "@bitwarden/auth/common";
-import { AvatarUpdateService as AvatarUpdateServiceAbstraction } from "@bitwarden/common/abstractions/account/avatar-update.service";
 import { ApiService as ApiServiceAbstraction } from "@bitwarden/common/abstractions/api.service";
 import { AuditService as AuditServiceAbstraction } from "@bitwarden/common/abstractions/audit.service";
 import { EventCollectionService as EventCollectionServiceAbstraction } from "@bitwarden/common/abstractions/event/event-collection.service";
@@ -39,6 +38,7 @@ import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authenticatio
 import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason";
 import { AccountServiceImplementation } from "@bitwarden/common/auth/services/account.service";
 import { AuthService } from "@bitwarden/common/auth/services/auth.service";
+import { AvatarService } from "@bitwarden/common/auth/services/avatar.service";
 import { DeviceTrustCryptoService } from "@bitwarden/common/auth/services/device-trust-crypto.service.implementation";
 import { DevicesServiceImplementation } from "@bitwarden/common/auth/services/devices/devices.service.implementation";
 import { DevicesApiServiceImplementation } from "@bitwarden/common/auth/services/devices-api.service.implementation";
@@ -117,7 +117,6 @@ import { DefaultStateProvider } from "@bitwarden/common/platform/state/implement
 import { StateEventRegistrarService } from "@bitwarden/common/platform/state/state-event-registrar.service";
 /* eslint-enable import/no-restricted-paths */
 import { DefaultThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service";
-import { AvatarUpdateService } from "@bitwarden/common/services/account/avatar-update.service";
 import { ApiService } from "@bitwarden/common/services/api.service";
 import { AuditService } from "@bitwarden/common/services/audit.service";
 import { EventCollectionService } from "@bitwarden/common/services/event/event-collection.service";
@@ -125,6 +124,7 @@ import { EventUploadService } from "@bitwarden/common/services/event/event-uploa
 import { NotificationsService } from "@bitwarden/common/services/notifications.service";
 import { SearchService } from "@bitwarden/common/services/search.service";
 import { VaultTimeoutSettingsService } from "@bitwarden/common/services/vault-timeout/vault-timeout-settings.service";
+import { AvatarService as AvatarServiceAbstraction } from "@bitwarden/common/src/auth/abstractions/avatar.service";
 import {
  PasswordGenerationService,
  PasswordGenerationServiceAbstraction,
@@ -288,7 +288,7 @@ export default class MainBackground {
  fido2UserInterfaceService: Fido2UserInterfaceServiceAbstraction;
  fido2AuthenticatorService: Fido2AuthenticatorServiceAbstraction;
  fido2ClientService: Fido2ClientServiceAbstraction;
-  avatarUpdateService: AvatarUpdateServiceAbstraction;
+  avatarService: AvatarServiceAbstraction;
  mainContextMenuHandler: MainContextMenuHandler;
  cipherContextMenuHandler: CipherContextMenuHandler;
  configService: BrowserConfigService;
@@ -685,7 +685,11 @@ export default class MainBackground {
      this.fileUploadService,
      this.sendService,
    );
+
+    this.avatarService = new AvatarService(this.apiService, this.stateProvider);
+
    this.providerService = new ProviderService(this.stateProvider);
+
    this.syncService = new SyncService(
      this.apiService,
      this.domainSettingsService,
@@ -703,6 +707,7 @@ export default class MainBackground {
      this.folderApiService,
      this.organizationService,
      this.sendApiService,
+      this.avatarService,
      logoutCallback,
    );
    this.eventUploadService = new EventUploadService(
@@ -943,8 +948,6 @@ export default class MainBackground {
      this.apiService,
    );

-    this.avatarUpdateService = new AvatarUpdateService(this.apiService, this.stateService);
-
    if (!this.popupOnlyContext) {
      this.mainContextMenuHandler = new MainContextMenuHandler(
        this.stateService,
--- a/apps/browser/src/background/runtime.background.ts
+++ b/apps/browser/src/background/runtime.background.ts
@@ -130,9 +130,6 @@ export default class RuntimeBackground {
            await this.main.refreshBadge();
            await this.main.refreshMenu();
          }, 2000);
-          // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
-          // eslint-disable-next-line @typescript-eslint/no-floating-promises
-          this.main.avatarUpdateService.loadColorFromState();
          this.configService.triggerServerConfigFetch();
        }
        break;