BEEEP - Auth AccountService Improvements (#11779)

* BEEEP Adjacent - AccountService misc improvements - (1) prefer null over undefined and (2) add new Account type * LockCompV2 - Fix activeAccount type per PR feedback * AccountService - update getUserId per PR feedback.
bitwarden · Oct 30, 2024 · 4de7cb8 · 4de7cb8
1 parent 690e175
commit 4de7cb8
Show file tree

Hide file tree

Showing 12 changed files with 56 additions and 40 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
@@ -1,7 +1,11 @@
 import { matches, mock } from "jest-mock-extended";
 import { BehaviorSubject, ReplaySubject, firstValueFrom, of, timeout } from "rxjs";
 
-import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import {
+  Account,
+  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";
@@ -14,7 +18,7 @@ import { AccountSwitcherService } from "./account-switcher.service";
 
 describe("AccountSwitcherService", () => {
   let accountsSubject: BehaviorSubject<Record<UserId, AccountInfo>>;
-  let activeAccountSubject: BehaviorSubject<{ id: UserId } & AccountInfo>;
+  let activeAccountSubject: BehaviorSubject<Account | null>;
   let authStatusSubject: ReplaySubject<Record<UserId, AuthenticationStatus>>;
 
   const accountService = mock<AccountService>();
@@ -29,7 +33,7 @@ describe("AccountSwitcherService", () => {
   beforeEach(() => {
     jest.resetAllMocks();
     accountsSubject = new BehaviorSubject<Record<UserId, AccountInfo>>(null);
-    activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);
+    activeAccountSubject = new BehaviorSubject<Account | null>(null);
     authStatusSubject = new ReplaySubject<Record<UserId, AuthenticationStatus>>(1);
 
     // Use subject to allow for easy updates

diff --git a/apps/web/src/app/key-management/key-rotation/user-key-rotation.service.ts b/apps/web/src/app/key-management/key-rotation/user-key-rotation.service.ts
@@ -1,7 +1,7 @@
 import { Injectable } from "@angular/core";
 import { firstValueFrom } from "rxjs";
 
-import { AccountInfo } from "@bitwarden/common/auth/abstractions/account.service";
+import { Account } from "@bitwarden/common/auth/abstractions/account.service";
 import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust.service.abstraction";
 import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
 import { VerificationType } from "@bitwarden/common/auth/enums/verification-type";
@@ -46,10 +46,7 @@ export class UserKeyRotationService {
    * Creates a new user key and re-encrypts all required data with the it.
    * @param masterPassword current master password (used for validation)
    */
-  async rotateUserKeyAndEncryptedData(
-    masterPassword: string,
-    user: { id: UserId } & AccountInfo,
-  ): Promise<void> {
+  async rotateUserKeyAndEncryptedData(masterPassword: string, user: Account): Promise<void> {
     this.logService.info("[Userkey rotation] Starting user key rotation...");
     if (!masterPassword) {
       this.logService.info("[Userkey rotation] Invalid master password provided. Aborting!");

diff --git a/libs/angular/src/auth/guards/auth.guard.spec.ts b/libs/angular/src/auth/guards/auth.guard.spec.ts
@@ -5,7 +5,11 @@ import { MockProxy, mock } from "jest-mock-extended";
 import { BehaviorSubject } from "rxjs";
 
 import { EmptyComponent } from "@bitwarden/angular/platform/guard/feature-flag.guard.spec";
-import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import {
+  Account,
+  AccountInfo,
+  AccountService,
+} from "@bitwarden/common/auth/abstractions/account.service";
 import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
 import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service";
 import { MasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction";
@@ -30,7 +34,7 @@ describe("AuthGuard", () => {
       keyConnectorServiceRequiresAccountConversion,
     );
     const accountService: MockProxy<AccountService> = mock<AccountService>();
-    const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);
+    const activeAccountSubject = new BehaviorSubject<Account | null>(null);
     accountService.activeAccount$ = activeAccountSubject;
     activeAccountSubject.next(
       Object.assign(

diff --git a/libs/angular/src/auth/guards/lock.guard.spec.ts b/libs/angular/src/auth/guards/lock.guard.spec.ts
@@ -6,7 +6,11 @@ import { BehaviorSubject, of } from "rxjs";
 
 import { EmptyComponent } from "@bitwarden/angular/platform/guard/feature-flag.guard.spec";
 import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service";
-import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import {
+  Account,
+  AccountInfo,
+  AccountService,
+} from "@bitwarden/common/auth/abstractions/account.service";
 import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
 import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust.service.abstraction";
 import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
@@ -56,7 +60,7 @@ describe("lockGuard", () => {
     userVerificationService.hasMasterPassword.mockResolvedValue(setupParams.hasMasterPassword);
 
     const accountService: MockProxy<AccountService> = mock<AccountService>();
-    const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);
+    const activeAccountSubject = new BehaviorSubject<Account | null>(null);
     accountService.activeAccount$ = activeAccountSubject;
     activeAccountSubject.next(
       Object.assign(

diff --git a/libs/auth/src/angular/lock/lock.component.ts b/libs/auth/src/angular/lock/lock.component.ts
@@ -7,7 +7,7 @@ import { BehaviorSubject, firstValueFrom, Subject, switchMap, take, takeUntil }
 import { JslibModule } from "@bitwarden/angular/jslib.module";
 import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
 import { MasterPasswordPolicyOptions } from "@bitwarden/common/admin-console/models/domain/master-password-policy-options";
-import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust.service.abstraction";
 import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction";
 import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
@@ -26,7 +26,6 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl
 import { KeySuffixOptions } from "@bitwarden/common/platform/enums";
 import { SyncService } from "@bitwarden/common/platform/sync";
 import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength";
-import { UserId } from "@bitwarden/common/types/guid";
 import { UserKey } from "@bitwarden/common/types/key";
 import {
   AsyncActionsModule,
@@ -73,7 +72,7 @@ const clientTypeToSuccessRouteRecord: Partial<Record<ClientType, string>> = {
 export class LockV2Component implements OnInit, OnDestroy {
   private destroy$ = new Subject<void>();
 
-  activeAccount: { id: UserId | undefined } & AccountInfo;
+  activeAccount: Account | null;
 
   clientType: ClientType;
   ClientType = ClientType;
@@ -202,11 +201,15 @@ export class LockV2Component implements OnInit, OnDestroy {
       .subscribe();
   }
 
-  private async handleActiveAccountChange(activeAccount: { id: UserId | undefined } & AccountInfo) {
+  private async handleActiveAccountChange(activeAccount: Account | null) {
     this.activeAccount = activeAccount;
 
     this.resetDataOnActiveAccountChange();
 
+    if (activeAccount == null) {
+      return;
+    }
+
     this.setEmailAsPageSubtitle(activeAccount.email);
 
     this.unlockOptions = await firstValueFrom(

diff --git a/libs/common/spec/fake-account-service.ts b/libs/common/spec/fake-account-service.ts
@@ -1,7 +1,7 @@
 import { mock } from "jest-mock-extended";
 import { ReplaySubject, combineLatest, map } from "rxjs";
 
-import { AccountInfo, AccountService } from "../src/auth/abstractions/account.service";
+import { Account, AccountInfo, AccountService } from "../src/auth/abstractions/account.service";
 import { UserId } from "../src/types/guid";
 
 export function mockAccountServiceWith(
@@ -30,7 +30,7 @@ export class FakeAccountService implements AccountService {
   // eslint-disable-next-line rxjs/no-exposed-subjects -- test class
   accountsSubject = new ReplaySubject<Record<UserId, AccountInfo>>(1);
   // eslint-disable-next-line rxjs/no-exposed-subjects -- test class
-  activeAccountSubject = new ReplaySubject<{ id: UserId } & AccountInfo>(1);
+  activeAccountSubject = new ReplaySubject<Account | null>(1);
   // eslint-disable-next-line rxjs/no-exposed-subjects -- test class
   accountActivitySubject = new ReplaySubject<Record<UserId, Date>>(1);
   private _activeUserId: UserId;

diff --git a/libs/common/src/auth/abstractions/account.service.ts b/libs/common/src/auth/abstractions/account.service.ts
@@ -12,6 +12,8 @@ export type AccountInfo = {
   name: string | undefined;
 };
 
+export type Account = { id: UserId } & AccountInfo;
+
 export function accountInfoEqual(a: AccountInfo, b: AccountInfo) {
   if (a == null && b == null) {
     return true;
@@ -32,7 +34,8 @@ export function accountInfoEqual(a: AccountInfo, b: AccountInfo) {
 
 export abstract class AccountService {
   accounts$: Observable<Record<UserId, AccountInfo>>;
-  activeAccount$: Observable<{ id: UserId | undefined } & AccountInfo>;
+
+  activeAccount$: Observable<Account | null>;
 
   /**
    * Observable of the last activity time for each account.
@@ -41,7 +44,7 @@ export abstract class AccountService {
   /** Account list in order of descending recency */
   sortedUserIds$: Observable<UserId[]>;
   /** Next account that is not the current active account */
-  nextUpAccount$: Observable<{ id: UserId } & AccountInfo>;
+  nextUpAccount$: Observable<Account>;
   /**
    * Updates the `accounts$` observable with the new account data.
    *

diff --git a/libs/common/src/auth/services/account.service.spec.ts b/libs/common/src/auth/services/account.service.spec.ts
@@ -88,10 +88,10 @@ describe("accountService", () => {
   });
 
   describe("activeAccount$", () => {
-    it("should emit undefined if no account is active", () => {
+    it("should emit null if no account is active", () => {
       const emissions = trackEmissions(sut.activeAccount$);
 
-      expect(emissions).toEqual([undefined]);
+      expect(emissions).toEqual([null]);
     });
 
     it("should emit the active account", async () => {
@@ -100,7 +100,7 @@ describe("accountService", () => {
       activeAccountIdState.stateSubject.next(userId);
 
       expect(emissions).toEqual([
-        undefined, // initial value
+        null, // initial value
         { id: userId, ...userInfo },
       ]);
     });
@@ -258,10 +258,10 @@ describe("accountService", () => {
       activeAccountIdState.stateSubject.next(userId);
     });
 
-    it("should emit undefined if no account is provided", async () => {
+    it("should emit null if no account is provided", async () => {
       await sut.switchAccount(null);
       const currentState = await firstValueFrom(sut.activeAccount$);
-      expect(currentState).toBeUndefined();
+      expect(currentState).toBeNull();
     });
 
     it("should throw if the account does not exist", () => {

diff --git a/libs/common/src/auth/services/account.service.ts b/libs/common/src/auth/services/account.service.ts
@@ -8,6 +8,7 @@ import {
 } from "rxjs";
 
 import {
+  Account,
   AccountInfo,
   InternalAccountService,
   accountInfoEqual,
@@ -48,9 +49,9 @@ const LOGGED_OUT_INFO: AccountInfo = {
 /**
  * An rxjs map operator that extracts the UserId from an account, or throws if the account or UserId are null.
  */
-export const getUserId = map<{ id: UserId | undefined }, UserId>((account) => {
-  if (account?.id == null) {
-    throw new Error("Null account or account ID");
+export const getUserId = map<Account | null, UserId>((account) => {
+  if (account == null) {
+    throw new Error("Null or undefined account");
   }
 
   return account.id;
@@ -59,19 +60,19 @@ export const getUserId = map<{ id: UserId | undefined }, UserId>((account) => {
 /**
  * An rxjs map operator that extracts the UserId from an account, or returns undefined if the account or UserId are null.
  */
-export const getOptionalUserId = map<{ id: UserId | undefined }, UserId | undefined>(
-  (account) => account?.id ?? undefined,
+export const getOptionalUserId = map<Account | null, UserId | null>(
+  (account) => account?.id ?? null,
 );
 
 export class AccountServiceImplementation implements InternalAccountService {
   private accountsState: GlobalState<Record<UserId, AccountInfo>>;
   private activeAccountIdState: GlobalState<UserId | undefined>;
 
   accounts$: Observable<Record<UserId, AccountInfo>>;
-  activeAccount$: Observable<{ id: UserId | undefined } & AccountInfo>;
+  activeAccount$: Observable<Account | null>;
   accountActivity$: Observable<Record<UserId, Date>>;
   sortedUserIds$: Observable<UserId[]>;
-  nextUpAccount$: Observable<{ id: UserId } & AccountInfo>;
+  nextUpAccount$: Observable<Account>;
 
   constructor(
     private messagingService: MessagingService,
@@ -86,7 +87,7 @@ export class AccountServiceImplementation implements InternalAccountService {
     );
     this.activeAccount$ = this.activeAccountIdState.state$.pipe(
       combineLatestWith(this.accounts$),
-      map(([id, accounts]) => (id ? { id, ...(accounts[id] as AccountInfo) } : undefined)),
+      map(([id, accounts]) => (id ? ({ id, ...(accounts[id] as AccountInfo) } as Account) : null)),
       distinctUntilChanged((a, b) => a?.id === b?.id && accountInfoEqual(a, b)),
       shareReplay({ bufferSize: 1, refCount: false }),
     );

diff --git a/libs/common/src/auth/services/password-reset-enrollment.service.implementation.spec.ts b/libs/common/src/auth/services/password-reset-enrollment.service.implementation.spec.ts
@@ -9,12 +9,12 @@ import { KeyService } from "../../../../key-management/src/abstractions/key.serv
 import { OrganizationApiServiceAbstraction } from "../../admin-console/abstractions/organization/organization-api.service.abstraction";
 import { OrganizationAutoEnrollStatusResponse } from "../../admin-console/models/response/organization-auto-enroll-status.response";
 import { I18nService } from "../../platform/abstractions/i18n.service";
-import { AccountInfo, AccountService } from "../abstractions/account.service";
+import { Account, AccountInfo, AccountService } from "../abstractions/account.service";
 
 import { PasswordResetEnrollmentServiceImplementation } from "./password-reset-enrollment.service.implementation";
 
 describe("PasswordResetEnrollmentServiceImplementation", () => {
-  const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);
+  const activeAccountSubject = new BehaviorSubject<Account | null>(null);
 
   let organizationApiService: MockProxy<OrganizationApiServiceAbstraction>;
   let accountService: MockProxy<AccountService>;

diff --git a/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts b/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts
@@ -3,7 +3,7 @@ import { TextEncoder } from "util";
 import { mock, MockProxy } from "jest-mock-extended";
 import { BehaviorSubject, of } from "rxjs";
 
-import { AccountInfo, AccountService } from "../../../auth/abstractions/account.service";
+import { Account, AccountService } from "../../../auth/abstractions/account.service";
 import { UserId } from "../../../types/guid";
 import { CipherService } from "../../../vault/abstractions/cipher.service";
 import { SyncService } from "../../../vault/abstractions/sync/sync.service.abstraction";
@@ -33,7 +33,7 @@ import { guidToRawFormat } from "./guid-utils";
 const RpId = "bitwarden.com";
 
 describe("FidoAuthenticatorService", () => {
-  const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>({
+  const activeAccountSubject = new BehaviorSubject<Account | null>({
     id: "testId" as UserId,
     email: "[email protected]",
     emailVerified: true,

diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts b/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts
@@ -8,7 +8,7 @@ import { Jsonify } from "type-fest";
 
 import { awaitAsync, trackEmissions } from "../../../../spec";
 import { FakeStorageService } from "../../../../spec/fake-storage.service";
-import { AccountInfo } from "../../../auth/abstractions/account.service";
+import { Account } from "../../../auth/abstractions/account.service";
 import { UserId } from "../../../types/guid";
 import { LogService } from "../../abstractions/log.service";
 import { StorageServiceProvider } from "../../services/storage-service.provider";
@@ -47,7 +47,7 @@ describe("DefaultActiveUserState", () => {
   const storageServiceProvider = mock<StorageServiceProvider>();
   const stateEventRegistrarService = mock<StateEventRegistrarService>();
   const logService = mock<LogService>();
-  let activeAccountSubject: BehaviorSubject<{ id: UserId } & AccountInfo>;
+  let activeAccountSubject: BehaviorSubject<Account | null>;
 
   let singleUserStateProvider: DefaultSingleUserStateProvider;
 
@@ -63,7 +63,7 @@ describe("DefaultActiveUserState", () => {
       logService,
     );
 
-    activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(undefined);
+    activeAccountSubject = new BehaviorSubject<Account | null>(null);
 
     userState = new DefaultActiveUserState(
       testKeyDefinition,