From 0d829b7398cc1425426b5d78333757c8fc1ec441 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Mon, 12 Aug 2024 15:51:57 -0400 Subject: [PATCH] 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 --- .../src/auth/components/lock.component.ts | 32 --- .../src/auth/guards/lock.guard.spec.ts | 235 ++++++++++++++++++ libs/angular/src/auth/guards/lock.guard.ts | 21 +- .../vault-timeout-settings.service.ts | 6 + .../vault-timeout-settings.service.spec.ts | 32 +++ .../vault-timeout-settings.service.ts | 9 +- 6 files changed, 297 insertions(+), 38 deletions(-) create mode 100644 libs/angular/src/auth/guards/lock.guard.spec.ts diff --git a/libs/angular/src/auth/components/lock.component.ts b/libs/angular/src/auth/components/lock.component.ts index 88b042c5b8..28cc7d810d 100644 --- a/libs/angular/src/auth/components/lock.component.ts +++ b/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); diff --git a/libs/angular/src/auth/guards/lock.guard.spec.ts b/libs/angular/src/auth/guards/lock.guard.spec.ts new file mode 100644 index 0000000000..7ff7feb920 --- /dev/null +++ b/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 = mock(); + authService.authStatusFor$.mockReturnValue(of(setupParams.authStatus)); + + const vaultTimeoutSettingsService: MockProxy = + mock(); + vaultTimeoutSettingsService.canLock.mockResolvedValue(setupParams.canLock); + + const cryptoService: MockProxy = mock(); + cryptoService.isLegacyUser.mockResolvedValue(setupParams.isLegacyUser); + cryptoService.everHadUserKey$ = of(setupParams.everHadUserKey); + + const platformUtilService: MockProxy = mock(); + platformUtilService.getClientType.mockReturnValue(setupParams.clientType); + + const messagingService: MockProxy = mock(); + + const deviceTrustService: MockProxy = + mock(); + deviceTrustService.supportsDeviceTrust$ = of(setupParams.supportsDeviceTrust); + + const userVerificationService: MockProxy = + mock(); + userVerificationService.hasMasterPassword.mockResolvedValue(setupParams.hasMasterPassword); + + const accountService: MockProxy = mock(); + 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("/"); + }); +}); diff --git a/libs/angular/src/auth/guards/lock.guard.ts b/libs/angular/src/auth/guards/lock.guard.ts index 8cd5290ebc..440e6931a0 100644 --- a/libs/angular/src/auth/guards/lock.guard.ts +++ b/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 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; diff --git a/libs/common/src/abstractions/vault-timeout/vault-timeout-settings.service.ts b/libs/common/src/abstractions/vault-timeout/vault-timeout-settings.service.ts index 5bf38f3b57..69539e6fea 100644 --- a/libs/common/src/abstractions/vault-timeout/vault-timeout-settings.service.ts +++ b/libs/common/src/abstractions/vault-timeout/vault-timeout-settings.service.ts @@ -25,6 +25,12 @@ export abstract class VaultTimeoutSettingsService { */ availableVaultTimeoutActions$: (userId?: string) => Observable; + /** + * Evaluates the user's available vault timeout actions and returns a boolean representing + * if the user can lock or not + */ + canLock: (userId: string) => Promise; + /** * 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, diff --git a/libs/common/src/services/vault-timeout/vault-timeout-settings.service.spec.ts b/libs/common/src/services/vault-timeout/vault-timeout-settings.service.spec.ts index e9839fc4e6..177c75ed5b 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout-settings.service.spec.ts +++ b/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( diff --git a/libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts b/libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts index d48729e9c6..e6587ade70 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts +++ b/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 { return defer(() => this.getAvailableVaultTimeoutActions(userId)); } + async canLock(userId: UserId): Promise { + const availableVaultTimeoutActions: VaultTimeoutAction[] = await firstValueFrom( + this.availableVaultTimeoutActions$(userId), + ); + return availableVaultTimeoutActions?.includes(VaultTimeoutAction.Lock) || false; + } + async isBiometricLockSet(userId?: string): Promise { const biometricUnlockPromise = userId == null