From 598139d739b709c38a31a89000100c98a40f09d8 Mon Sep 17 00:00:00 2001 From: Patrick-Pimentel-Bitwarden Date: Thu, 20 Feb 2025 16:45:19 -0500 Subject: [PATCH] fix(recovery-code-login): [PM-18474] Fix for Recovery Code Login (#13497) * fix(recovery-code-login): [PM-18474] Fix for Recovery Code Login - Fixed the recovery code login to work with the new device verification notice flow. * test(recovery-code-login): [PM-18474] Fix for Recovery Code Login - Tests added. --- .../auth/recover-two-factor.component.spec.ts | 133 ++++++++++++++++++ .../app/auth/recover-two-factor.component.ts | 36 ++--- ...w-device-verification-notice.guard.spec.ts | 12 +- .../new-device-verification-notice.guard.ts | 5 + ...device-verification-notice.service.spec.ts | 27 ++++ .../new-device-verification-notice.service.ts | 24 ++++ 6 files changed, 210 insertions(+), 27 deletions(-) create mode 100644 apps/web/src/app/auth/recover-two-factor.component.spec.ts diff --git a/apps/web/src/app/auth/recover-two-factor.component.spec.ts b/apps/web/src/app/auth/recover-two-factor.component.spec.ts new file mode 100644 index 0000000000..e9e75dca50 --- /dev/null +++ b/apps/web/src/app/auth/recover-two-factor.component.spec.ts @@ -0,0 +1,133 @@ +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { Router } from "@angular/router"; +import { mock, MockProxy } from "jest-mock-extended"; + +import { + LoginStrategyServiceAbstraction, + LoginSuccessHandlerService, + PasswordLoginCredentials, +} from "@bitwarden/auth/common"; +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AuthResult } from "@bitwarden/common/auth/models/domain/auth-result"; +import { TwoFactorRecoveryRequest } from "@bitwarden/common/auth/models/request/two-factor-recovery.request"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; +import { ToastService } from "@bitwarden/components"; +import { KeyService } from "@bitwarden/key-management"; +import { I18nPipe } from "@bitwarden/ui-common"; +import { NewDeviceVerificationNoticeService } from "@bitwarden/vault"; + +import { RecoverTwoFactorComponent } from "./recover-two-factor.component"; + +describe("RecoverTwoFactorComponent", () => { + let component: RecoverTwoFactorComponent; + let fixture: ComponentFixture; + + // Mock Services + let mockRouter: MockProxy; + let mockApiService: MockProxy; + let mockPlatformUtilsService: MockProxy; + let mockI18nService: MockProxy; + let mockKeyService: MockProxy; + let mockLoginStrategyService: MockProxy; + let mockToastService: MockProxy; + let mockConfigService: MockProxy; + let mockLoginSuccessHandlerService: MockProxy; + let mockLogService: MockProxy; + let mockNewDeviceVerificationNoticeService: MockProxy; + + beforeEach(() => { + mockRouter = mock(); + mockApiService = mock(); + mockPlatformUtilsService = mock(); + mockI18nService = mock(); + mockKeyService = mock(); + mockLoginStrategyService = mock(); + mockToastService = mock(); + mockConfigService = mock(); + mockLoginSuccessHandlerService = mock(); + mockLogService = mock(); + mockNewDeviceVerificationNoticeService = mock(); + + TestBed.configureTestingModule({ + declarations: [RecoverTwoFactorComponent], + providers: [ + { provide: Router, useValue: mockRouter }, + { provide: ApiService, useValue: mockApiService }, + { provide: PlatformUtilsService, mockPlatformUtilsService }, + { provide: I18nService, useValue: mockI18nService }, + { provide: KeyService, useValue: mockKeyService }, + { provide: LoginStrategyServiceAbstraction, useValue: mockLoginStrategyService }, + { provide: ToastService, useValue: mockToastService }, + { provide: ConfigService, useValue: mockConfigService }, + { provide: LoginSuccessHandlerService, useValue: mockLoginSuccessHandlerService }, + { provide: LogService, useValue: mockLogService }, + { + provide: NewDeviceVerificationNoticeService, + useValue: mockNewDeviceVerificationNoticeService, + }, + ], + imports: [I18nPipe], + }); + + fixture = TestBed.createComponent(RecoverTwoFactorComponent); + component = fixture.componentInstance; + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe("handleRecoveryLogin", () => { + it("should log in successfully and navigate to the two-factor settings page", async () => { + // Arrange + const request = new TwoFactorRecoveryRequest(); + request.recoveryCode = "testRecoveryCode"; + request.email = "test@example.com"; + + const authResult = new AuthResult(); + mockLoginStrategyService.logIn.mockResolvedValue(authResult); + + // Act + await component["handleRecoveryLogin"](request); + + // Assert + expect(mockLoginStrategyService.logIn).toHaveBeenCalledWith( + expect.any(PasswordLoginCredentials), + ); + expect(mockToastService.showToast).toHaveBeenCalledWith({ + variant: "success", + title: "", + message: mockI18nService.t("youHaveBeenLoggedIn"), + }); + expect( + mockNewDeviceVerificationNoticeService.updateNewDeviceVerificationSkipNoticeState, + ).toHaveBeenCalledWith(authResult.userId, true); + expect(mockRouter.navigate).toHaveBeenCalledWith(["/settings/security/two-factor"]); + }); + + it("should handle login errors and redirect to login page", async () => { + // Arrange + const request = new TwoFactorRecoveryRequest(); + request.recoveryCode = "testRecoveryCode"; + request.email = "test@example.com"; + + const error = new Error("Login failed"); + mockLoginStrategyService.logIn.mockRejectedValue(error); + + // Act + await component["handleRecoveryLogin"](request); + + // Assert + expect(mockLogService.error).toHaveBeenCalledWith( + "Error logging in automatically: ", + error.message, + ); + expect(mockRouter.navigate).toHaveBeenCalledWith(["/login"], { + queryParams: { email: request.email }, + }); + }); + }); +}); diff --git a/apps/web/src/app/auth/recover-two-factor.component.ts b/apps/web/src/app/auth/recover-two-factor.component.ts index ee39175046..996b324ce5 100644 --- a/apps/web/src/app/auth/recover-two-factor.component.ts +++ b/apps/web/src/app/auth/recover-two-factor.component.ts @@ -18,6 +18,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { ToastService } from "@bitwarden/components"; import { KeyService } from "@bitwarden/key-management"; +import { NewDeviceVerificationNoticeService } from "@bitwarden/vault"; @Component({ selector: "app-recover-two-factor", @@ -51,6 +52,7 @@ export class RecoverTwoFactorComponent implements OnInit { private configService: ConfigService, private loginSuccessHandlerService: LoginSuccessHandlerService, private logService: LogService, + private newDeviceVerificationNoticeService: NewDeviceVerificationNoticeService, ) {} async ngOnInit() { @@ -134,6 +136,14 @@ export class RecoverTwoFactorComponent implements OnInit { }); await this.loginSuccessHandlerService.run(authResult.userId); + + // Before routing, set the state to skip the new device notification. This is a temporary + // fix and will be cleaned up in PM-18485. + await this.newDeviceVerificationNoticeService.updateNewDeviceVerificationSkipNoticeState( + authResult.userId, + true, + ); + await this.router.navigate(["/settings/security/two-factor"]); } catch (error) { // If login errors, redirect to login page per product. Don't show error @@ -141,30 +151,4 @@ export class RecoverTwoFactorComponent implements OnInit { await this.router.navigate(["/login"], { queryParams: { email: request.email } }); } } - - /** - * Extracts an error message from the error object. - */ - private extractErrorMessage(error: unknown): string { - let errorMessage: string = this.i18nService.t("unexpectedError"); - if (error && typeof error === "object" && "validationErrors" in error) { - const validationErrors = error.validationErrors; - if (validationErrors && typeof validationErrors === "object") { - errorMessage = Object.keys(validationErrors) - .map((key) => { - const messages = (validationErrors as Record)[key]; - return Array.isArray(messages) ? messages.join(" ") : messages; - }) - .join(" "); - } - } else if ( - error && - typeof error === "object" && - "message" in error && - typeof error.message === "string" - ) { - errorMessage = error.message; - } - return errorMessage; - } } diff --git a/libs/angular/src/vault/guards/new-device-verification-notice.guard.spec.ts b/libs/angular/src/vault/guards/new-device-verification-notice.guard.spec.ts index 938d5b3252..53e2cef0d0 100644 --- a/libs/angular/src/vault/guards/new-device-verification-notice.guard.spec.ts +++ b/libs/angular/src/vault/guards/new-device-verification-notice.guard.spec.ts @@ -36,6 +36,7 @@ describe("NewDeviceVerificationNoticeGuard", () => { const isSelfHost = jest.fn().mockReturnValue(false); const getProfileTwoFactorEnabled = jest.fn().mockResolvedValue(false); const noticeState$ = jest.fn().mockReturnValue(new BehaviorSubject(null)); + const skipState$ = jest.fn().mockReturnValue(new BehaviorSubject(null)); const getProfileCreationDate = jest.fn().mockResolvedValue(eightDaysAgo); const hasMasterPasswordAndMasterKeyHash = jest.fn().mockResolvedValue(true); const getUserSSOBound = jest.fn().mockResolvedValue(false); @@ -50,12 +51,13 @@ describe("NewDeviceVerificationNoticeGuard", () => { hasMasterPasswordAndMasterKeyHash.mockClear(); getUserSSOBound.mockClear(); getUserSSOBoundAdminOwner.mockClear(); + skipState$.mockClear(); TestBed.configureTestingModule({ providers: [ { provide: Router, useValue: { createUrlTree } }, { provide: ConfigService, useValue: { getFeatureFlag } }, - { provide: NewDeviceVerificationNoticeService, useValue: { noticeState$ } }, + { provide: NewDeviceVerificationNoticeService, useValue: { noticeState$, skipState$ } }, { provide: AccountService, useValue: { activeAccount$ } }, { provide: PlatformUtilsService, useValue: { isSelfHost } }, { provide: UserVerificationService, useValue: { hasMasterPasswordAndMasterKeyHash } }, @@ -144,6 +146,14 @@ describe("NewDeviceVerificationNoticeGuard", () => { expect(await newDeviceGuard()).toBe(true); }); + it("returns `true` when the skip state value is set to true", async () => { + skipState$.mockReturnValueOnce(new BehaviorSubject(true)); + + expect(await newDeviceGuard()).toBe(true); + expect(skipState$.mock.calls[0][0]).toBe("account-id"); + expect(skipState$.mock.calls.length).toBe(1); + }); + describe("SSO bound", () => { beforeEach(() => { getFeatureFlag.mockImplementation((key) => { diff --git a/libs/angular/src/vault/guards/new-device-verification-notice.guard.ts b/libs/angular/src/vault/guards/new-device-verification-notice.guard.ts index 8d4a7742bc..4e26149249 100644 --- a/libs/angular/src/vault/guards/new-device-verification-notice.guard.ts +++ b/libs/angular/src/vault/guards/new-device-verification-notice.guard.ts @@ -44,6 +44,11 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( return router.createUrlTree(["/login"]); } + // Currently used by the auth recovery login flow and will get cleaned up in PM-18485. + if (await firstValueFrom(newDeviceVerificationNoticeService.skipState$(currentAcct.id))) { + return true; + } + try { const isSelfHosted = platformUtilsService.isSelfHost(); const userIsSSOUser = await ssoAppliesToUser( diff --git a/libs/vault/src/services/new-device-verification-notice.service.spec.ts b/libs/vault/src/services/new-device-verification-notice.service.spec.ts index b774b1a05a..186a844c16 100644 --- a/libs/vault/src/services/new-device-verification-notice.service.spec.ts +++ b/libs/vault/src/services/new-device-verification-notice.service.spec.ts @@ -14,6 +14,7 @@ import { NewDeviceVerificationNoticeService, NewDeviceVerificationNotice, NEW_DEVICE_VERIFICATION_NOTICE_KEY, + SKIP_NEW_DEVICE_VERIFICATION_NOTICE, } from "./new-device-verification-notice.service"; describe("New Device Verification Notice", () => { @@ -21,6 +22,7 @@ describe("New Device Verification Notice", () => { const userId = Utils.newGuid() as UserId; let newDeviceVerificationService: NewDeviceVerificationNoticeService; let mockNoticeState: FakeSingleUserState; + let mockSkipState: FakeSingleUserState; let stateProvider: FakeStateProvider; let accountService: FakeAccountService; @@ -28,6 +30,7 @@ describe("New Device Verification Notice", () => { accountService = mockAccountServiceWith(userId); stateProvider = new FakeStateProvider(accountService); mockNoticeState = stateProvider.singleUser.getFake(userId, NEW_DEVICE_VERIFICATION_NOTICE_KEY); + mockSkipState = stateProvider.singleUser.getFake(userId, SKIP_NEW_DEVICE_VERIFICATION_NOTICE); newDeviceVerificationService = new NewDeviceVerificationNoticeService(stateProvider); }); @@ -82,4 +85,28 @@ describe("New Device Verification Notice", () => { expect(result).toEqual(newState); }); }); + + describe("skipNotice state", () => { + it("emits skip notice state", async () => { + const shouldSkip = true; + await stateProvider.setUserState(SKIP_NEW_DEVICE_VERIFICATION_NOTICE, shouldSkip, userId); + + const result = await firstValueFrom(newDeviceVerificationService.skipState$(userId)); + + expect(result).toBe(shouldSkip); + }); + + it("should update the skip notice state", async () => { + const initialSkipState = false; + const updatedSkipState = true; + mockSkipState.nextState(initialSkipState); + await newDeviceVerificationService.updateNewDeviceVerificationSkipNoticeState( + userId, + updatedSkipState, + ); + + const result = await firstValueFrom(newDeviceVerificationService.skipState$(userId)); + expect(result).toBe(updatedSkipState); + }); + }); }); diff --git a/libs/vault/src/services/new-device-verification-notice.service.ts b/libs/vault/src/services/new-device-verification-notice.service.ts index a925cf09ec..a0d77f09f5 100644 --- a/libs/vault/src/services/new-device-verification-notice.service.ts +++ b/libs/vault/src/services/new-device-verification-notice.service.ts @@ -42,6 +42,15 @@ export const NEW_DEVICE_VERIFICATION_NOTICE_KEY = }, ); +export const SKIP_NEW_DEVICE_VERIFICATION_NOTICE = new UserKeyDefinition( + NEW_DEVICE_VERIFICATION_NOTICE, + "shouldSkip", + { + deserializer: (data: boolean) => data, + clearOn: ["logout"], + }, +); + @Injectable() export class NewDeviceVerificationNoticeService { constructor(private stateProvider: StateProvider) {} @@ -62,4 +71,19 @@ export class NewDeviceVerificationNoticeService { return { ...newState }; }); } + + private skipState(userId: UserId): SingleUserState { + return this.stateProvider.getUser(userId, SKIP_NEW_DEVICE_VERIFICATION_NOTICE); + } + + skipState$(userId: UserId): Observable { + return this.skipState(userId).state$; + } + + async updateNewDeviceVerificationSkipNoticeState( + userId: UserId, + shouldSkip: boolean, + ): Promise { + await this.skipState(userId).update(() => shouldSkip); + } }