Auth/PM-3515 - Lock component Tech Debt Clean up (#10332)

* PM-3515 - Lock component - remove isUnlocked check on lock comp load b/c lock guard should cover all cases with its existing logic for all clients.

* PM-3515 - VaultTimeoutSettingsSvc - Add new canLock method

* PM-3515 - Refactor logic out of lock component that belongs in lock guard. Update lock guard to reject route activation if a user can't lock whereas we used to log the user out when they landed on the lock comp.

* PM-3515 - WIP on testing all lock guard scenarios

* PM-3515 - Refactor lock guard tests + add more tests

* PM-3515 - LockGuard - if TDE user that is authN directly navigates from login-init to lock for whatever reason (only possible on web with url bar), reject that navigation directly instead of throwing them up to the redirect guard

* PM-3515 - More LockGuard tests

* PM-3515 - Update comment

libs/angular/src/auth/components/lock.component.ts

@@ -16,14 +16,12 @@ import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractio
 import { KdfConfigService } from "@bitwarden/common/auth/abstractions/kdf-config.service";
 import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction";
 import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
-import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
 import { VerificationType } from "@bitwarden/common/auth/enums/verification-type";
 import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason";
 import {
   MasterPasswordVerification,
   MasterPasswordVerificationResponse,
 } from "@bitwarden/common/auth/types/verification";
-import { VaultTimeoutAction } from "@bitwarden/common/enums/vault-timeout-action.enum";
 import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service";
 import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
@@ -316,36 +314,6 @@ export class LockComponent implements OnInit, OnDestroy {
   }
 
   private async load(userId: UserId) {
-    // TODO: Investigate PM-3515
-
-    // The loading of the lock component works as follows:
-    //   1. If the user is unlocked, we're here in error so we navigate to the home page
-    //   2. First, is locking a valid timeout action?  If not, we will log the user out.
-    //   3. If locking IS a valid timeout action, we proceed to show the user the lock screen.
-    //      The user will be able to unlock as follows:
-    //        - If they have a PIN set, they will be presented with the PIN input
-    //        - If they have a master password and no PIN, they will be presented with the master password input
-    //        - If they have biometrics enabled, they will be presented with the biometric prompt
-
-    const isUnlocked = await firstValueFrom(
-      this.authService
-        .authStatusFor$(userId)
-        .pipe(map((status) => status === AuthenticationStatus.Unlocked)),
-    );
-    if (isUnlocked) {
-      // navigate to home
-      await this.router.navigate(["/"]);
-      return;
-    }
-
-    const availableVaultTimeoutActions = await firstValueFrom(
-      this.vaultTimeoutSettingsService.availableVaultTimeoutActions$(userId),
-    );
-    const supportsLock = availableVaultTimeoutActions.includes(VaultTimeoutAction.Lock);
-    if (!supportsLock) {
-      return await this.vaultTimeoutService.logOut(userId);
-    }
-
     this.pinLockType = await this.pinService.getPinLockType(userId);
 
     const ephemeralPinSet = await this.pinService.getPinKeyEncryptedUserKeyEphemeral(userId);

libs/angular/src/auth/guards/lock.guard.spec.ts

@@ -0,0 +1,235 @@
+import { TestBed } from "@angular/core/testing";
+import { Router } from "@angular/router";
+import { RouterTestingModule } from "@angular/router/testing";
+import { MockProxy, mock } from "jest-mock-extended";
+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 { 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";
+import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
+import { ClientType } from "@bitwarden/common/enums";
+import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service";
+import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
+import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
+import { UserId } from "@bitwarden/common/types/guid";
+
+import { lockGuard } from "./lock.guard";
+
+interface SetupParams {
+  authStatus: AuthenticationStatus;
+  canLock?: boolean;
+  isLegacyUser?: boolean;
+  clientType?: ClientType;
+  everHadUserKey?: boolean;
+  supportsDeviceTrust?: boolean;
+  hasMasterPassword?: boolean;
+}
+
+describe("lockGuard", () => {
+  const setup = (setupParams: SetupParams) => {
+    const authService: MockProxy<AuthService> = mock<AuthService>();
+    authService.authStatusFor$.mockReturnValue(of(setupParams.authStatus));
+
+    const vaultTimeoutSettingsService: MockProxy<VaultTimeoutSettingsService> =
+      mock<VaultTimeoutSettingsService>();
+    vaultTimeoutSettingsService.canLock.mockResolvedValue(setupParams.canLock);
+
+    const cryptoService: MockProxy<CryptoService> = mock<CryptoService>();
+    cryptoService.isLegacyUser.mockResolvedValue(setupParams.isLegacyUser);
+    cryptoService.everHadUserKey$ = of(setupParams.everHadUserKey);
+
+    const platformUtilService: MockProxy<PlatformUtilsService> = mock<PlatformUtilsService>();
+    platformUtilService.getClientType.mockReturnValue(setupParams.clientType);
+
+    const messagingService: MockProxy<MessagingService> = mock<MessagingService>();
+
+    const deviceTrustService: MockProxy<DeviceTrustServiceAbstraction> =
+      mock<DeviceTrustServiceAbstraction>();
+    deviceTrustService.supportsDeviceTrust$ = of(setupParams.supportsDeviceTrust);
+
+    const userVerificationService: MockProxy<UserVerificationService> =
+      mock<UserVerificationService>();
+    userVerificationService.hasMasterPassword.mockResolvedValue(setupParams.hasMasterPassword);
+
+    const accountService: MockProxy<AccountService> = mock<AccountService>();
+    const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null);
+    accountService.activeAccount$ = activeAccountSubject;
+    activeAccountSubject.next(
+      Object.assign(
+        {
+          name: "Test User 1",
+          email: "test@email.com",
+          emailVerified: true,
+        } as AccountInfo,
+        { id: "test-id" as UserId },
+      ),
+    );
+
+    const testBed = TestBed.configureTestingModule({
+      imports: [
+        RouterTestingModule.withRoutes([
+          { path: "", component: EmptyComponent },
+          { path: "lock", component: EmptyComponent, canActivate: [lockGuard()] },
+          { path: "non-lock-route", component: EmptyComponent },
+          { path: "migrate-legacy-encryption", component: EmptyComponent },
+        ]),
+      ],
+      providers: [
+        { provide: AuthService, useValue: authService },
+        { provide: MessagingService, useValue: messagingService },
+        { provide: AccountService, useValue: accountService },
+        { provide: VaultTimeoutSettingsService, useValue: vaultTimeoutSettingsService },
+        { provide: CryptoService, useValue: cryptoService },
+        { provide: PlatformUtilsService, useValue: platformUtilService },
+        { provide: DeviceTrustServiceAbstraction, useValue: deviceTrustService },
+        { provide: UserVerificationService, useValue: userVerificationService },
+      ],
+    });
+
+    return {
+      router: testBed.inject(Router),
+      messagingService,
+    };
+  };
+
+  it("should be created", () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+    });
+    expect(router).toBeTruthy();
+  });
+
+  it("should redirect to the root route when the user is Unlocked", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Unlocked,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/");
+  });
+
+  it("should redirect to the root route when the user is LoggedOut", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.LoggedOut,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/");
+  });
+
+  it("should allow navigation to the lock route when the user is Locked and they can lock", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/lock");
+  });
+
+  it("should allow navigation to the lock route when the user is locked, they can lock, and device trust is not supported", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+      supportsDeviceTrust: false,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/lock");
+  });
+
+  it("should not allow navigation to the lock route when canLock is false", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: false,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/");
+  });
+
+  it("should log user out if they are a legacy user on a desktop client", async () => {
+    const { router, messagingService } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+      isLegacyUser: true,
+      clientType: ClientType.Desktop,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/");
+    expect(messagingService.send).toHaveBeenCalledWith("logout");
+  });
+
+  it("should log user out if they are a legacy user on a browser extension client", async () => {
+    const { router, messagingService } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+      isLegacyUser: true,
+      clientType: ClientType.Browser,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/");
+    expect(messagingService.send).toHaveBeenCalledWith("logout");
+  });
+
+  it("should send the user to migrate-legacy-encryption if they are a legacy user on a web client", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+      isLegacyUser: true,
+      clientType: ClientType.Web,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/migrate-legacy-encryption");
+  });
+
+  it("should allow navigation to the lock route when device trust is supported, the user has a MP, and the user is coming from the login-initiated page", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+      isLegacyUser: false,
+      clientType: ClientType.Web,
+      everHadUserKey: false,
+      supportsDeviceTrust: true,
+      hasMasterPassword: true,
+    });
+
+    await router.navigate(["lock"], { queryParams: { from: "login-initiated" } });
+    expect(router.url).toBe("/lock?from=login-initiated");
+  });
+
+  it("should allow navigation to the lock route when TDE is disabled, the user doesn't have a MP, and the user has had a user key", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+      supportsDeviceTrust: false,
+      hasMasterPassword: false,
+      everHadUserKey: true,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/lock");
+  });
+
+  it("should not allow navigation to the lock route when device trust is supported and the user has not ever had a user key", async () => {
+    const { router } = setup({
+      authStatus: AuthenticationStatus.Locked,
+      canLock: true,
+      isLegacyUser: false,
+      clientType: ClientType.Web,
+      everHadUserKey: false,
+      supportsDeviceTrust: true,
+      hasMasterPassword: false,
+    });
+
+    await router.navigate(["lock"]);
+    expect(router.url).toBe("/");
+  });
+});

libs/angular/src/auth/guards/lock.guard.ts

@@ -7,6 +7,8 @@ import {
 } from "@angular/router";
 import { firstValueFrom } from "rxjs";
 
+import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service";
+import { 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";
@@ -19,7 +21,7 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl
 /**
  * Only allow access to this route if the vault is locked.
  * If TDE is enabled then the user must also have had a user key at some point.
- * Otherwise redirect to root.
+ * Otherwise reject navigation.
  *
  * TODO: This should return Observable<boolean | UrlTree> once we can remove all the promises
  */
@@ -35,12 +37,22 @@ export function lockGuard(): CanActivateFn {
     const messagingService = inject(MessagingService);
     const router = inject(Router);
     const userVerificationService = inject(UserVerificationService);
+    const vaultTimeoutSettingsService = inject(VaultTimeoutSettingsService);
+    const accountService = inject(AccountService);
 
-    const authStatus = await authService.getAuthStatus();
+    const activeUser = await firstValueFrom(accountService.activeAccount$);
+
+    const authStatus = await firstValueFrom(authService.authStatusFor$(activeUser.id));
     if (authStatus !== AuthenticationStatus.Locked) {
       return router.createUrlTree(["/"]);
     }
 
+    // if user can't lock, they can't access the lock screen
+    const canLock = await vaultTimeoutSettingsService.canLock(activeUser.id);
+    if (!canLock) {
+      return false;
+    }
+
     // If legacy user on web, redirect to migration page
     if (await cryptoService.isLegacyUser()) {
       if (platformUtilService.getClientType() === ClientType.Web) {
@@ -65,11 +77,10 @@ export function lockGuard(): CanActivateFn {
       return true;
     }
 
-    // If authN user with TDE directly navigates to lock, kick them upwards so redirect guard can
-    // properly route them to the login decryption options component.
+    // If authN user with TDE directly navigates to lock, reject that navigation
     const everHadUserKey = await firstValueFrom(cryptoService.everHadUserKey$);
     if (tdeEnabled && !everHadUserKey) {
-      return router.createUrlTree(["/"]);
+      return false;
     }
 
     return true;

libs/common/src/abstractions/vault-timeout/vault-timeout-settings.service.ts

@@ -25,6 +25,12 @@ export abstract class VaultTimeoutSettingsService {
    */
   availableVaultTimeoutActions$: (userId?: string) => Observable<VaultTimeoutAction[]>;
 
+  /**
+   * Evaluates the user's available vault timeout actions and returns a boolean representing
+   * if the user can lock or not
+   */
+  canLock: (userId: string) => Promise<boolean>;
+
   /**
    * Gets the vault timeout action for the given user id. The returned value is
    * calculated based on the current state, if a max vault timeout policy applies to the user,

libs/common/src/services/vault-timeout/vault-timeout-settings.service.spec.ts

@@ -127,6 +127,38 @@ describe("VaultTimeoutSettingsService", () => {
     });
   });
 
+  describe("canLock", () => {
+    it("returns true if the user can lock", async () => {
+      jest
+        .spyOn(vaultTimeoutSettingsService, "availableVaultTimeoutActions$")
+        .mockReturnValue(of([VaultTimeoutAction.Lock]));
+
+      const result = await vaultTimeoutSettingsService.canLock("userId" as UserId);
+
+      expect(result).toBe(true);
+    });
+
+    it("returns false if the user only has the log out vault timeout action", async () => {
+      jest
+        .spyOn(vaultTimeoutSettingsService, "availableVaultTimeoutActions$")
+        .mockReturnValue(of([VaultTimeoutAction.LogOut]));
+
+      const result = await vaultTimeoutSettingsService.canLock("userId" as UserId);
+
+      expect(result).toBe(false);
+    });
+
+    it("returns false if the user has no vault timeout actions", async () => {
+      jest
+        .spyOn(vaultTimeoutSettingsService, "availableVaultTimeoutActions$")
+        .mockReturnValue(of([]));
+
+      const result = await vaultTimeoutSettingsService.canLock("userId" as UserId);
+
+      expect(result).toBe(false);
+    });
+  });
+
   describe("getVaultTimeoutActionByUserId$", () => {
     it("should throw an error if no user id is provided", async () => {
       expect(() => vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(null)).toThrow(

libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts

@@ -90,10 +90,17 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
     await this.cryptoService.refreshAdditionalKeys();
   }
 
-  availableVaultTimeoutActions$(userId?: string) {
+  availableVaultTimeoutActions$(userId?: string): Observable<VaultTimeoutAction[]> {
     return defer(() => this.getAvailableVaultTimeoutActions(userId));
   }
 
+  async canLock(userId: UserId): Promise<boolean> {
+    const availableVaultTimeoutActions: VaultTimeoutAction[] = await firstValueFrom(
+      this.availableVaultTimeoutActions$(userId),
+    );
+    return availableVaultTimeoutActions?.includes(VaultTimeoutAction.Lock) || false;
+  }
+
   async isBiometricLockSet(userId?: string): Promise<boolean> {
     const biometricUnlockPromise =
       userId == null