From 0c7abc8cd3b2a9cbd485503d8c62f706c9b81e20 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Mon, 16 Dec 2024 13:50:12 -0600 Subject: [PATCH 01/15] add account created date to the account information --- libs/common/spec/fake-account-service.ts | 6 ++++ .../src/auth/abstractions/account.service.ts | 8 +++++ .../src/auth/services/account.service.spec.ts | 31 ++++++++++++++++++- .../src/auth/services/account.service.ts | 5 +++ .../src/platform/sync/default-sync.service.ts | 1 + 5 files changed, 50 insertions(+), 1 deletion(-) diff --git a/libs/common/spec/fake-account-service.ts b/libs/common/spec/fake-account-service.ts index 05e44d5db18..4e3e23ddae1 100644 --- a/libs/common/spec/fake-account-service.ts +++ b/libs/common/spec/fake-account-service.ts @@ -17,6 +17,7 @@ export function mockAccountServiceWith( name: "name", email: "email", emailVerified: true, + createdDate: undefined, }, }; @@ -93,6 +94,10 @@ export class FakeAccountService implements AccountService { await this.mock.setAccountEmailVerified(userId, emailVerified); } + async setAccountCreationDate(userId: UserId, createdDate: string): Promise { + await this.mock.setAccountCreationDate(userId, createdDate); + } + async switchAccount(userId: UserId): Promise { const next = userId == null ? null : { id: userId, ...this.accountsSubject["_buffer"]?.[0]?.[userId] }; @@ -112,4 +117,5 @@ const loggedOutInfo: AccountInfo = { name: undefined, email: "", emailVerified: false, + createdDate: undefined, }; diff --git a/libs/common/src/auth/abstractions/account.service.ts b/libs/common/src/auth/abstractions/account.service.ts index 094e005e656..b249365495c 100644 --- a/libs/common/src/auth/abstractions/account.service.ts +++ b/libs/common/src/auth/abstractions/account.service.ts @@ -12,6 +12,7 @@ export type AccountInfo = { email: string; emailVerified: boolean; name: string | undefined; + createdDate?: string; }; export type Account = { id: UserId } & AccountInfo; @@ -73,6 +74,13 @@ export abstract class AccountService { * @param emailVerified */ abstract setAccountEmailVerified(userId: UserId, emailVerified: boolean): Promise; + /** + * Updates the `accounts$` observable with the creation date of the account. + * + * @param userId + * @param creationDate + */ + abstract setAccountCreationDate(userId: UserId, creationDate: string): Promise; /** * Updates the `activeAccount$` observable with the new active account. * @param userId diff --git a/libs/common/src/auth/services/account.service.spec.ts b/libs/common/src/auth/services/account.service.spec.ts index 227949156ee..90f88efa54c 100644 --- a/libs/common/src/auth/services/account.service.spec.ts +++ b/libs/common/src/auth/services/account.service.spec.ts @@ -70,7 +70,7 @@ describe("accountService", () => { let accountsState: FakeGlobalState>; let activeAccountIdState: FakeGlobalState; const userId = Utils.newGuid() as UserId; - const userInfo = { email: "email", name: "name", emailVerified: true }; + const userInfo = { email: "email", name: "name", emailVerified: true, createdDate: "" }; beforeEach(() => { messagingService = mock(); @@ -224,6 +224,35 @@ describe("accountService", () => { }); }); + describe("setAccountCreationDate", () => { + const today = new Date(); + const yesterday = new Date(); + yesterday.setDate(yesterday.getDate() - 1); + + const initialState = { [userId]: userInfo }; + initialState[userId].createdDate = yesterday.toISOString(); + + beforeEach(() => { + accountsState.stateSubject.next(initialState); + }); + + it("updates the account", async () => { + await sut.setAccountCreationDate(userId, today.toISOString()); + const currentState = await firstValueFrom(accountsState.state$); + + expect(currentState).toEqual({ + [userId]: { ...userInfo, createdDate: today.toISOString() }, + }); + }); + + it("does not update if the email is the same", async () => { + await sut.setAccountCreationDate(userId, yesterday.toISOString()); + const currentState = await firstValueFrom(accountsState.state$); + + expect(currentState).toEqual(initialState); + }); + }); + describe("clean", () => { beforeEach(() => { accountsState.stateSubject.next({ [userId]: userInfo }); diff --git a/libs/common/src/auth/services/account.service.ts b/libs/common/src/auth/services/account.service.ts index d4479815c5d..409801c2e9b 100644 --- a/libs/common/src/auth/services/account.service.ts +++ b/libs/common/src/auth/services/account.service.ts @@ -46,6 +46,7 @@ const LOGGED_OUT_INFO: AccountInfo = { email: "", emailVerified: false, name: undefined, + createdDate: undefined, }; /** @@ -141,6 +142,10 @@ export class AccountServiceImplementation implements InternalAccountService { await this.setAccountInfo(userId, { emailVerified }); } + async setAccountCreationDate(userId: UserId, createdDate: string): Promise { + await this.setAccountInfo(userId, { createdDate }); + } + async clean(userId: UserId) { await this.setAccountInfo(userId, LOGGED_OUT_INFO); await this.removeAccountActivity(userId); diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index 9e36aa69417..67fd3a5e3b2 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -191,6 +191,7 @@ export class DefaultSyncService extends CoreSyncService { await this.avatarService.setSyncAvatarColor(response.id, response.avatarColor); await this.tokenService.setSecurityStamp(response.securityStamp, response.id); await this.accountService.setAccountEmailVerified(response.id, response.emailVerified); + await this.accountService.setAccountCreationDate(response.id, response.creationDate); await this.billingAccountProfileStateService.setHasPremium( response.premiumPersonally, From cd2cccd8c38689b21ae533aca1b5afa99d4a55ac Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Mon, 16 Dec 2024 13:57:49 -0600 Subject: [PATCH 02/15] set permanent dismissal flag when the user selects that they can access their email --- ...-verification-notice-page-one.component.ts | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts index 62ae22f5b22..7014f12f949 100644 --- a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts +++ b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts @@ -65,18 +65,21 @@ export class NewDeviceVerificationNoticePageOneComponent implements OnInit { } submit = async () => { - if (this.formGroup.controls.hasEmailAccess.value === 0) { - await this.router.navigate(["new-device-notice/setup"]); - } else if (this.formGroup.controls.hasEmailAccess.value === 1) { - await this.newDeviceVerificationNoticeService.updateNewDeviceVerificationNoticeState( - this.currentUserId, - { - last_dismissal: new Date(), - permanent_dismissal: false, - }, - ); + const doesNotHaveEmailAccess = this.formGroup.controls.hasEmailAccess.value === 0; + + await this.newDeviceVerificationNoticeService.updateNewDeviceVerificationNoticeState( + this.currentUserId, + { + last_dismissal: new Date(), + permanent_dismissal: !doesNotHaveEmailAccess, + }, + ); - await this.router.navigate(["/vault"]); + if (doesNotHaveEmailAccess) { + await this.router.navigate(["new-device-notice/setup"]); + return; } + + await this.router.navigate(["/vault"]); }; } From 54f733201162f5e82a9ec84ecd81cddc90e95455 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Mon, 16 Dec 2024 13:59:22 -0600 Subject: [PATCH 03/15] update the logic of device verification notice --- ...w-device-verification-notice.guard.spec.ts | 223 ++++++++++++++++++ .../new-device-verification-notice.guard.ts | 80 ++++++- 2 files changed, 293 insertions(+), 10 deletions(-) create mode 100644 libs/angular/src/vault/guards/new-device-verification-notice.guard.spec.ts 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 new file mode 100644 index 00000000000..c314101b278 --- /dev/null +++ b/libs/angular/src/vault/guards/new-device-verification-notice.guard.spec.ts @@ -0,0 +1,223 @@ +import { TestBed } from "@angular/core/testing"; +import { ActivatedRouteSnapshot, Router, RouterStateSnapshot } from "@angular/router"; +import { BehaviorSubject } from "rxjs"; + +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { PolicyType } from "@bitwarden/common/admin-console/enums"; +import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; + +import { NewDeviceVerificationNoticeService } from "../../../../vault/src/services/new-device-verification-notice.service"; + +import { NewDeviceVerificationNoticeGuard } from "./new-device-verification-notice.guard"; + +describe("NewDeviceVerificationNoticeGuard", () => { + const _state = Object.freeze({}) as RouterStateSnapshot; + const emptyRoute = Object.freeze({ queryParams: {} }) as ActivatedRouteSnapshot; + const eightDaysAgo = new Date(); + eightDaysAgo.setDate(eightDaysAgo.getDate() - 8); + + const account = { + id: "account-id", + createdDate: eightDaysAgo.toISOString(), + } as unknown as Account; + + const activeAccount$ = new BehaviorSubject(account); + + const createUrlTree = jest.fn(); + const getFeatureFlag = jest.fn().mockResolvedValue(null); + const isSelfHost = jest.fn().mockResolvedValue(false); + const getTwoFactorProviders = jest.fn().mockResolvedValue({ data: [] }); + const policyAppliesToActiveUser$ = jest.fn().mockReturnValue(new BehaviorSubject(false)); + const noticeState$ = jest.fn().mockReturnValue(new BehaviorSubject(null)); + + beforeEach(() => { + getFeatureFlag.mockClear(); + isSelfHost.mockClear(); + getTwoFactorProviders.mockClear(); + policyAppliesToActiveUser$.mockClear(); + createUrlTree.mockClear(); + + const eightDaysAgo = new Date(); + eightDaysAgo.setDate(eightDaysAgo.getDate() - 8); + + account.createdDate = eightDaysAgo.toISOString(); + + TestBed.configureTestingModule({ + providers: [ + { provide: Router, useValue: { createUrlTree } }, + { provide: ConfigService, useValue: { getFeatureFlag } }, + { provide: NewDeviceVerificationNoticeService, useValue: { noticeState$ } }, + { provide: AccountService, useValue: { activeAccount$ } }, + { provide: PlatformUtilsService, useValue: { isSelfHost } }, + { provide: ApiService, useValue: { getTwoFactorProviders } }, + { provide: PolicyService, useValue: { policyAppliesToActiveUser$ } }, + ], + }); + }); + + function newDeviceGuard(route?: ActivatedRouteSnapshot) { + // Run the guard within injection context so `inject` works as you'd expect + // Pass state object to make TypeScript happy + return TestBed.runInInjectionContext(async () => + NewDeviceVerificationNoticeGuard(route ?? emptyRoute, _state), + ); + } + + // Baseline, all defaults should allow the guard to pass + it("returns true", async () => { + expect(await newDeviceGuard()).toBe(true); + }); + + describe("fromNewDeviceVerification", () => { + const route = { + queryParams: { fromNewDeviceVerification: "true" }, + } as unknown as ActivatedRouteSnapshot; + + it("returns `true` when `fromNewDeviceVerification` is present", async () => { + expect(await newDeviceGuard(route)).toBe(true); + }); + + it("does not execute other logic", async () => { + // `fromNewDeviceVerification` param should exit early, + // not foolproof but a quick way to test that other logic isn't executed + await newDeviceGuard(route); + + expect(getFeatureFlag).not.toHaveBeenCalled(); + expect(isSelfHost).not.toHaveBeenCalled(); + expect(getTwoFactorProviders).not.toHaveBeenCalled(); + expect(policyAppliesToActiveUser$).not.toHaveBeenCalled(); + }); + }); + + describe("missing current account", () => { + afterAll(() => { + // reset `activeAccount$` observable + activeAccount$.next(account); + }); + + it("redirects to login when account is missing", async () => { + activeAccount$.next(null); + + await newDeviceGuard(); + + expect(createUrlTree).toHaveBeenCalledWith(["/login"]); + }); + }); + + it("returns `true` when 2FA is enabled", async () => { + getTwoFactorProviders.mockResolvedValueOnce({ data: [{ enabled: false }, { enabled: true }] }); + + expect(await newDeviceGuard()).toBe(true); + }); + + it("returns `true` when the user is self hosted", async () => { + isSelfHost.mockReturnValueOnce(true); + + expect(await newDeviceGuard()).toBe(true); + }); + + it("returns `true` SSO is required", async () => { + policyAppliesToActiveUser$.mockReturnValueOnce(new BehaviorSubject(true)); + + expect(await newDeviceGuard()).toBe(true); + expect(policyAppliesToActiveUser$).toHaveBeenCalledWith(PolicyType.RequireSso); + }); + + it("returns `true` when the account was created less than a week ago", async () => { + const sixDaysAgo = new Date(); + sixDaysAgo.setDate(sixDaysAgo.getDate() - 6); + account.createdDate = sixDaysAgo.toISOString(); + + expect(await newDeviceGuard()).toBe(true); + }); + + describe("temp flag", () => { + beforeEach(() => { + getFeatureFlag.mockImplementation((key) => { + if (key === FeatureFlag.NewDeviceVerificationTemporaryDismiss) { + return Promise.resolve(true); + } + + return Promise.resolve(false); + }); + }); + + afterAll(() => { + getFeatureFlag.mockReturnValue(false); + }); + + it("redirects to notice when the user has not dismissed it", async () => { + noticeState$.mockReturnValueOnce(new BehaviorSubject(null)); + + await newDeviceGuard(); + + expect(createUrlTree).toHaveBeenCalledWith(["/new-device-notice"]); + expect(noticeState$).toHaveBeenCalledWith(account.id); + }); + + it("redirects to notice when the user dismissed it more than 7 days ago", async () => { + const eighteenDaysAgo = new Date(); + eighteenDaysAgo.setDate(eighteenDaysAgo.getDate() - 18); + + noticeState$.mockReturnValueOnce( + new BehaviorSubject({ last_dismissal: eighteenDaysAgo.toISOString() }), + ); + + await newDeviceGuard(); + + expect(createUrlTree).toHaveBeenCalledWith(["/new-device-notice"]); + }); + + it("returns true when the user dismissed less than 7 days ago", async () => { + const fourDaysAgo = new Date(); + fourDaysAgo.setDate(fourDaysAgo.getDate() - 4); + + noticeState$.mockReturnValueOnce( + new BehaviorSubject({ last_dismissal: fourDaysAgo.toISOString() }), + ); + + expect(await newDeviceGuard()).toBe(true); + }); + }); + + describe("permanent flag", () => { + beforeEach(() => { + getFeatureFlag.mockImplementation((key) => { + if (key === FeatureFlag.NewDeviceVerificationPermanentDismiss) { + return Promise.resolve(true); + } + + return Promise.resolve(false); + }); + }); + + afterAll(() => { + getFeatureFlag.mockReturnValue(false); + }); + + it("redirects when the user has not dismissed", async () => { + noticeState$.mockReturnValueOnce(new BehaviorSubject(null)); + + await newDeviceGuard(); + + expect(createUrlTree).toHaveBeenCalledWith(["/new-device-notice"]); + + noticeState$.mockReturnValueOnce(new BehaviorSubject({ permanent_dismissal: null })); + + await newDeviceGuard(); + + expect(createUrlTree).toHaveBeenCalledTimes(2); + expect(createUrlTree).toHaveBeenCalledWith(["/new-device-notice"]); + }); + + it("returns `true` when the user has dismissed", async () => { + noticeState$.mockReturnValueOnce(new BehaviorSubject({ permanent_dismissal: true })); + + expect(await newDeviceGuard()).toBe(true); + }); + }); +}); 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 a37097e3583..10164173428 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 @@ -2,9 +2,13 @@ import { inject } from "@angular/core"; import { ActivatedRouteSnapshot, CanActivateFn, Router } from "@angular/router"; import { Observable, firstValueFrom, map } from "rxjs"; +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { NewDeviceVerificationNoticeService } from "../../../../vault/src/services/new-device-verification-notice.service"; @@ -15,18 +19,14 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( const configService = inject(ConfigService); const newDeviceVerificationNoticeService = inject(NewDeviceVerificationNoticeService); const accountService = inject(AccountService); + const platformUtilsService = inject(PlatformUtilsService); + const apiService = inject(ApiService); + const policyService = inject(PolicyService); if (route.queryParams["fromNewDeviceVerification"]) { return true; } - const tempNoticeFlag = await configService.getFeatureFlag( - FeatureFlag.NewDeviceVerificationTemporaryDismiss, - ); - const permNoticeFlag = await configService.getFeatureFlag( - FeatureFlag.NewDeviceVerificationPermanentDismiss, - ); - const currentAcct$: Observable = accountService.activeAccount$.pipe( map((acct) => acct), ); @@ -36,16 +36,76 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( return router.createUrlTree(["/login"]); } + const has2FAEnabled = await hasATwoFactorProviderEnabled(apiService); + const isSelfHosted = await platformUtilsService.isSelfHost(); + const requiresSSO = await isSSORequired(policyService); + const isProfileLessThanWeekOld = await profileIsLessThanWeekOld(currentAcct); + + // When any of the following are true, the device verification notice is + // not applicable for the user. + if (has2FAEnabled || isSelfHosted || requiresSSO || isProfileLessThanWeekOld) { + return true; + } + + const tempNoticeFlag = await configService.getFeatureFlag( + FeatureFlag.NewDeviceVerificationTemporaryDismiss, + ); + const permNoticeFlag = await configService.getFeatureFlag( + FeatureFlag.NewDeviceVerificationPermanentDismiss, + ); + const userItems$ = newDeviceVerificationNoticeService.noticeState$(currentAcct.id); const userItems = await firstValueFrom(userItems$); + // Show the notice when: + // - The temp notice flag is enabled + // - The user hasn't dismissed the notice or the user dismissed it more than 7 days ago if ( - userItems?.last_dismissal == null && - (userItems?.permanent_dismissal == null || !userItems?.permanent_dismissal) && - (tempNoticeFlag || permNoticeFlag) + tempNoticeFlag && + (!userItems?.last_dismissal || isMoreThan7DaysAgo(userItems?.last_dismissal)) ) { return router.createUrlTree(["/new-device-notice"]); } + // Show the notice when: + // - The permanent notice flag is enabled + // - The user hasn't dismissed the notice + if (permNoticeFlag && !userItems?.permanent_dismissal) { + return router.createUrlTree(["/new-device-notice"]); + } + return true; }; + +/** Returns true has one 2FA provider enabled */ +async function hasATwoFactorProviderEnabled(apiService: ApiService): Promise { + const twoFactorProviders = await apiService.getTwoFactorProviders(); + + return twoFactorProviders.data.some((provider) => provider.enabled); +} + +/** Returns true when the user's profile is less than a week old */ +async function profileIsLessThanWeekOld(account: Account): Promise { + return !isMoreThan7DaysAgo(account.createdDate); +} + +/** Returns true when the user is required to login via SSO */ +async function isSSORequired(policyService: PolicyService) { + return firstValueFrom(policyService.policyAppliesToActiveUser$(PolicyType.RequireSso)); +} + +/** Returns the true when the date given is older than 7 days */ +function isMoreThan7DaysAgo(date?: string | Date): boolean { + if (!date) { + return false; + } + + const inputDate = new Date(date).getTime(); + const today = new Date().getTime(); + + const differenceInMS = today - inputDate; + const msInADay = 1000 * 60 * 60 * 24; + const differenceInDays = Math.round(differenceInMS / msInADay); + + return differenceInDays > 7; +} From 46600c53a6127fa5fd28262d5458f2e9e4554a5b Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Mon, 16 Dec 2024 21:21:27 -0600 Subject: [PATCH 04/15] add service to cache the profile creation date to avoid calling the API multiple times --- ...w-device-verification-notice.guard.spec.ts | 14 +++--- .../new-device-verification-notice.guard.ts | 11 +++-- .../services/vault-profile.service.spec.ts | 46 +++++++++++++++++++ .../vault/services/vault-profile.service.ts | 31 +++++++++++++ 4 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 libs/angular/src/vault/services/vault-profile.service.spec.ts create mode 100644 libs/angular/src/vault/services/vault-profile.service.ts 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 c314101b278..eaf6260ed44 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 @@ -11,6 +11,7 @@ import { ConfigService } from "@bitwarden/common/platform/abstractions/config/co import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { NewDeviceVerificationNoticeService } from "../../../../vault/src/services/new-device-verification-notice.service"; +import { VaultProfileService } from "../services/vault-profile.service"; import { NewDeviceVerificationNoticeGuard } from "./new-device-verification-notice.guard"; @@ -22,7 +23,6 @@ describe("NewDeviceVerificationNoticeGuard", () => { const account = { id: "account-id", - createdDate: eightDaysAgo.toISOString(), } as unknown as Account; const activeAccount$ = new BehaviorSubject(account); @@ -33,6 +33,7 @@ describe("NewDeviceVerificationNoticeGuard", () => { const getTwoFactorProviders = jest.fn().mockResolvedValue({ data: [] }); const policyAppliesToActiveUser$ = jest.fn().mockReturnValue(new BehaviorSubject(false)); const noticeState$ = jest.fn().mockReturnValue(new BehaviorSubject(null)); + const getProfileCreationDate = jest.fn().mockResolvedValue(eightDaysAgo); beforeEach(() => { getFeatureFlag.mockClear(); @@ -41,11 +42,6 @@ describe("NewDeviceVerificationNoticeGuard", () => { policyAppliesToActiveUser$.mockClear(); createUrlTree.mockClear(); - const eightDaysAgo = new Date(); - eightDaysAgo.setDate(eightDaysAgo.getDate() - 8); - - account.createdDate = eightDaysAgo.toISOString(); - TestBed.configureTestingModule({ providers: [ { provide: Router, useValue: { createUrlTree } }, @@ -55,6 +51,7 @@ describe("NewDeviceVerificationNoticeGuard", () => { { provide: PlatformUtilsService, useValue: { isSelfHost } }, { provide: ApiService, useValue: { getTwoFactorProviders } }, { provide: PolicyService, useValue: { policyAppliesToActiveUser$ } }, + { provide: VaultProfileService, useValue: { getProfileCreationDate } }, ], }); }); @@ -127,10 +124,11 @@ describe("NewDeviceVerificationNoticeGuard", () => { expect(policyAppliesToActiveUser$).toHaveBeenCalledWith(PolicyType.RequireSso); }); - it("returns `true` when the account was created less than a week ago", async () => { + it("returns `true` when the profile was created less than a week ago", async () => { const sixDaysAgo = new Date(); sixDaysAgo.setDate(sixDaysAgo.getDate() - 6); - account.createdDate = sixDaysAgo.toISOString(); + + getProfileCreationDate.mockResolvedValueOnce(sixDaysAgo); expect(await newDeviceGuard()).toBe(true); }); 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 10164173428..f242365015a 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 @@ -11,6 +11,7 @@ import { ConfigService } from "@bitwarden/common/platform/abstractions/config/co import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { NewDeviceVerificationNoticeService } from "../../../../vault/src/services/new-device-verification-notice.service"; +import { VaultProfileService } from "../services/vault-profile.service"; export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( route: ActivatedRouteSnapshot, @@ -22,6 +23,7 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( const platformUtilsService = inject(PlatformUtilsService); const apiService = inject(ApiService); const policyService = inject(PolicyService); + const vaultProfileService = inject(VaultProfileService); if (route.queryParams["fromNewDeviceVerification"]) { return true; @@ -39,7 +41,7 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( const has2FAEnabled = await hasATwoFactorProviderEnabled(apiService); const isSelfHosted = await platformUtilsService.isSelfHost(); const requiresSSO = await isSSORequired(policyService); - const isProfileLessThanWeekOld = await profileIsLessThanWeekOld(currentAcct); + const isProfileLessThanWeekOld = await profileIsLessThanWeekOld(vaultProfileService); // When any of the following are true, the device verification notice is // not applicable for the user. @@ -85,8 +87,11 @@ async function hasATwoFactorProviderEnabled(apiService: ApiService): Promise { - return !isMoreThan7DaysAgo(account.createdDate); +async function profileIsLessThanWeekOld( + vaultProfileService: VaultProfileService, +): Promise { + const creationDate = await vaultProfileService.getProfileCreationDate(); + return !isMoreThan7DaysAgo(creationDate); } /** Returns true when the user is required to login via SSO */ diff --git a/libs/angular/src/vault/services/vault-profile.service.spec.ts b/libs/angular/src/vault/services/vault-profile.service.spec.ts new file mode 100644 index 00000000000..f56cb1fc052 --- /dev/null +++ b/libs/angular/src/vault/services/vault-profile.service.spec.ts @@ -0,0 +1,46 @@ +import { TestBed } from "@angular/core/testing"; + +import { ApiService } from "@bitwarden/common/abstractions/api.service"; + +import { VaultProfileService } from "./vault-profile.service"; + +describe("VaultProfileService", () => { + let service: VaultProfileService; + const hardcodedDateString = "2024-02-24T12:00:00Z"; + + const getProfile = jest.fn().mockResolvedValue({ creationDate: hardcodedDateString }); + + beforeEach(() => { + getProfile.mockClear(); + + TestBed.configureTestingModule({ + providers: [{ provide: ApiService, useValue: { getProfile } }], + }); + + jest.useFakeTimers(); + jest.setSystemTime(new Date("2024-02-22T00:00:00Z")); + service = TestBed.runInInjectionContext(() => new VaultProfileService()); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("calls `getProfile` when stored profile date is not set", async () => { + expect(service["profileCreatedDate"]).toBeNull(); + + const date = await service.getProfileCreationDate(); + + expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); + expect(getProfile).toHaveBeenCalled(); + }); + + it("does not call `getProfile` when the date is already stored", async () => { + service["profileCreatedDate"] = hardcodedDateString; + + const date = await service.getProfileCreationDate(); + + expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); + expect(getProfile).not.toHaveBeenCalled(); + }); +}); diff --git a/libs/angular/src/vault/services/vault-profile.service.ts b/libs/angular/src/vault/services/vault-profile.service.ts new file mode 100644 index 00000000000..906744778a0 --- /dev/null +++ b/libs/angular/src/vault/services/vault-profile.service.ts @@ -0,0 +1,31 @@ +import { Injectable, inject } from "@angular/core"; + +import { ApiService } from "@bitwarden/common/abstractions/api.service"; + +@Injectable({ + providedIn: "root", +}) +/** Class to provide profile level details without having to call the API each time. */ +export class VaultProfileService { + private apiService = inject(ApiService); + + /** Profile creation stored as a string. */ + private profileCreatedDate: string | null = null; + + /** + * Returns the creation date of the profile. + * Note: `Date`s are mutable in JS, creating a new + * instance is important to avoid unwanted changes. + */ + async getProfileCreationDate(): Promise { + if (this.profileCreatedDate) { + return Promise.resolve(new Date(this.profileCreatedDate)); + } + + const profile = await this.apiService.getProfile(); + + this.profileCreatedDate = profile.creationDate; + + return new Date(this.profileCreatedDate); + } +} From 093011c0f1f56e5de4294b2f7455531853b7c364 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 17 Dec 2024 13:16:59 -0600 Subject: [PATCH 05/15] update step one logic for new device verification + add tests --- ...fication-notice-page-one.component.spec.ts | 176 ++++++++++++++++++ ...-verification-notice-page-one.component.ts | 47 ++++- 2 files changed, 214 insertions(+), 9 deletions(-) create mode 100644 libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts diff --git a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts new file mode 100644 index 00000000000..a273fbcd2d7 --- /dev/null +++ b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts @@ -0,0 +1,176 @@ +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { By } from "@angular/platform-browser"; +import { Router } from "@angular/router"; +import { BehaviorSubject } from "rxjs"; + +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; + +import { NewDeviceVerificationNoticeService } from "../../services/new-device-verification-notice.service"; + +import { NewDeviceVerificationNoticePageOneComponent } from "./new-device-verification-notice-page-one.component"; + +describe("NewDeviceVerificationNoticePageOneComponent", () => { + let component: NewDeviceVerificationNoticePageOneComponent; + let fixture: ComponentFixture; + + const activeAccount$ = new BehaviorSubject({ email: "test@example.com", id: "acct-1" }); + const navigate = jest.fn().mockResolvedValue(null); + const updateNewDeviceVerificationNoticeState = jest.fn().mockResolvedValue(null); + const getFeatureFlag = jest.fn().mockResolvedValue(null); + + beforeEach(async () => { + navigate.mockClear(); + updateNewDeviceVerificationNoticeState.mockClear(); + getFeatureFlag.mockClear(); + + await TestBed.configureTestingModule({ + providers: [ + { provide: I18nService, useValue: { t: (...key: string[]) => key.join(" ") } }, + { provide: Router, useValue: { navigate } }, + { provide: AccountService, useValue: { activeAccount$ } }, + { + provide: NewDeviceVerificationNoticeService, + useValue: { updateNewDeviceVerificationNoticeState }, + }, + { provide: PlatformUtilsService, useValue: { getClientType: () => false } }, + { provide: ConfigService, useValue: { getFeatureFlag } }, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(NewDeviceVerificationNoticePageOneComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("sets initial properties", () => { + expect(component["currentEmail"]).toBe("test@example.com"); + expect(component["currentUserId"]).toBe("acct-1"); + expect(component["formMessage"]).toBe( + "newDeviceVerificationNoticePageOneFormContent test@example.com", + ); + }); + + describe("temporary flag submission", () => { + beforeEach(() => { + getFeatureFlag.mockImplementation((key) => { + if (key === FeatureFlag.NewDeviceVerificationTemporaryDismiss) { + return Promise.resolve(true); + } + + return Promise.resolve(false); + }); + }); + + describe("no email access", () => { + beforeEach(() => { + component["formGroup"].controls.hasEmailAccess.setValue(0); + fixture.detectChanges(); + + const submit = fixture.debugElement.query(By.css('button[type="submit"]')); + submit.nativeElement.click(); + }); + + it("redirects to step two ", () => { + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith(["new-device-notice/setup"]); + }); + + it("does not update notice state", () => { + expect(getFeatureFlag).not.toHaveBeenCalled(); + expect(updateNewDeviceVerificationNoticeState).not.toHaveBeenCalled(); + }); + }); + + describe("has email access", () => { + beforeEach(() => { + component["formGroup"].controls.hasEmailAccess.setValue(1); + fixture.detectChanges(); + + jest.useFakeTimers(); + jest.setSystemTime(new Date("2024-03-03T00:00:00.000Z")); + const submit = fixture.debugElement.query(By.css('button[type="submit"]')); + submit.nativeElement.click(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("redirects to the vault", () => { + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith(["/vault"]); + }); + + it("updates notice state with a new date", () => { + expect(updateNewDeviceVerificationNoticeState).toHaveBeenCalledWith("acct-1", { + last_dismissal: new Date("2024-03-03T00:00:00.000Z"), + permanent_dismissal: false, + }); + }); + }); + }); + + describe("permanent flag submission", () => { + beforeEach(() => { + getFeatureFlag.mockImplementation((key) => { + if (key === FeatureFlag.NewDeviceVerificationPermanentDismiss) { + return Promise.resolve(true); + } + + return Promise.resolve(false); + }); + }); + + describe("no email access", () => { + beforeEach(() => { + component["formGroup"].controls.hasEmailAccess.setValue(0); + fixture.detectChanges(); + + const submit = fixture.debugElement.query(By.css('button[type="submit"]')); + submit.nativeElement.click(); + }); + + it("redirects to step two", () => { + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith(["new-device-notice/setup"]); + }); + + it("does not update notice state", () => { + expect(getFeatureFlag).not.toHaveBeenCalled(); + expect(updateNewDeviceVerificationNoticeState).not.toHaveBeenCalled(); + }); + }); + + describe("has email access", () => { + beforeEach(() => { + component["formGroup"].controls.hasEmailAccess.setValue(1); + fixture.detectChanges(); + + jest.useFakeTimers(); + jest.setSystemTime(new Date("2024-04-04T00:00:00.000Z")); + const submit = fixture.debugElement.query(By.css('button[type="submit"]')); + submit.nativeElement.click(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("redirects to the vault ", () => { + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith(["/vault"]); + }); + + it("updates notice state with a new date", () => { + expect(updateNewDeviceVerificationNoticeState).toHaveBeenCalledWith("acct-1", { + last_dismissal: new Date("2024-04-04T00:00:00.000Z"), + permanent_dismissal: true, + }); + }); + }); + }); +}); diff --git a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts index 7014f12f949..70ac4073a0d 100644 --- a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts +++ b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.ts @@ -7,6 +7,8 @@ import { firstValueFrom, Observable } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { ClientType } from "@bitwarden/common/enums"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { UserId } from "@bitwarden/common/types/guid"; import { @@ -18,7 +20,10 @@ import { TypographyModule, } from "@bitwarden/components"; -import { NewDeviceVerificationNoticeService } from "./../../services/new-device-verification-notice.service"; +import { + NewDeviceVerificationNotice, + NewDeviceVerificationNoticeService, +} from "./../../services/new-device-verification-notice.service"; @Component({ standalone: true, @@ -51,6 +56,7 @@ export class NewDeviceVerificationNoticePageOneComponent implements OnInit { private accountService: AccountService, private newDeviceVerificationNoticeService: NewDeviceVerificationNoticeService, private platformUtilsService: PlatformUtilsService, + private configService: ConfigService, ) { this.isDesktop = this.platformUtilsService.getClientType() === ClientType.Desktop; } @@ -67,19 +73,42 @@ export class NewDeviceVerificationNoticePageOneComponent implements OnInit { submit = async () => { const doesNotHaveEmailAccess = this.formGroup.controls.hasEmailAccess.value === 0; - await this.newDeviceVerificationNoticeService.updateNewDeviceVerificationNoticeState( - this.currentUserId, - { - last_dismissal: new Date(), - permanent_dismissal: !doesNotHaveEmailAccess, - }, - ); - if (doesNotHaveEmailAccess) { await this.router.navigate(["new-device-notice/setup"]); return; } + const tempNoticeFlag = await this.configService.getFeatureFlag( + FeatureFlag.NewDeviceVerificationTemporaryDismiss, + ); + const permNoticeFlag = await this.configService.getFeatureFlag( + FeatureFlag.NewDeviceVerificationPermanentDismiss, + ); + + let newNoticeState: NewDeviceVerificationNotice | null = null; + + // When the temporary flag is enabled, only update the `last_dismissal` + if (tempNoticeFlag) { + newNoticeState = { + last_dismissal: new Date(), + permanent_dismissal: false, + }; + } else if (permNoticeFlag) { + // When the per flag is enabled, only update the `last_dismissal` + newNoticeState = { + last_dismissal: new Date(), + permanent_dismissal: true, + }; + } + + // This shouldn't occur as the user shouldn't get here unless one of the flags is active. + if (newNoticeState) { + await this.newDeviceVerificationNoticeService.updateNewDeviceVerificationNoticeState( + this.currentUserId!, + newNoticeState, + ); + } + await this.router.navigate(["/vault"]); }; } From a0e6ae5ab067df1e592d2a18752978a2a814956b Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 17 Dec 2024 13:17:28 -0600 Subject: [PATCH 06/15] update step two logic for new device verification + add tests - remove remind me later link for permanent logic --- ...erification-notice-page-two.component.html | 6 +- ...fication-notice-page-two.component.spec.ts | 175 ++++++++++++++++++ ...-verification-notice-page-two.component.ts | 10 +- 3 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.spec.ts diff --git a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.html b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.html index 270b4126252..66a61f3b8df 100644 --- a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.html +++ b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.html @@ -8,6 +8,7 @@ (click)="navigateToTwoStepLogin($event)" buttonType="primary" class="tw-w-full tw-mt-4" + data-testid="two-factor" > {{ "turnOnTwoStepLogin" | i18n }} {{ "changeAcctEmail" | i18n }} -
- +
+ {{ "remindMeLater" | i18n }}
diff --git a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.spec.ts b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.spec.ts new file mode 100644 index 00000000000..92f0494776a --- /dev/null +++ b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.spec.ts @@ -0,0 +1,175 @@ +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { By } from "@angular/platform-browser"; +import { Router } from "@angular/router"; +import { BehaviorSubject } from "rxjs"; + +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { ClientType } from "@bitwarden/common/enums"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; + +import { NewDeviceVerificationNoticeService } from "../../services/new-device-verification-notice.service"; + +import { NewDeviceVerificationNoticePageTwoComponent } from "./new-device-verification-notice-page-two.component"; + +describe("NewDeviceVerificationNoticePageTwoComponent", () => { + let component: NewDeviceVerificationNoticePageTwoComponent; + let fixture: ComponentFixture; + + const activeAccount$ = new BehaviorSubject({ email: "test@example.com", id: "acct-1" }); + const environment$ = new BehaviorSubject({ getWebVaultUrl: () => "vault.bitwarden.com" }); + const navigate = jest.fn().mockResolvedValue(null); + const updateNewDeviceVerificationNoticeState = jest.fn().mockResolvedValue(null); + const getFeatureFlag = jest.fn().mockResolvedValue(false); + const getClientType = jest.fn().mockReturnValue(ClientType.Browser); + const launchUri = jest.fn(); + + beforeEach(async () => { + navigate.mockClear(); + updateNewDeviceVerificationNoticeState.mockClear(); + getFeatureFlag.mockClear(); + getClientType.mockClear(); + launchUri.mockClear(); + + await TestBed.configureTestingModule({ + providers: [ + { provide: I18nService, useValue: { t: (...key: string[]) => key.join(" ") } }, + { provide: Router, useValue: { navigate } }, + { provide: AccountService, useValue: { activeAccount$ } }, + { provide: EnvironmentService, useValue: { environment$ } }, + { + provide: NewDeviceVerificationNoticeService, + useValue: { updateNewDeviceVerificationNoticeState }, + }, + { provide: PlatformUtilsService, useValue: { getClientType, launchUri } }, + { provide: ConfigService, useValue: { getFeatureFlag } }, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(NewDeviceVerificationNoticePageTwoComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("sets initial properties", () => { + expect(component["currentUserId"]).toBe("acct-1"); + expect(component["permanentFlagEnabled"]).toBe(false); + }); + + describe("change email", () => { + const changeEmailButton = () => + fixture.debugElement.query(By.css('[data-testid="change-email"]')); + + describe("web", () => { + beforeEach(() => { + component["isWeb"] = true; + fixture.detectChanges(); + }); + + it("navigates to settings", () => { + changeEmailButton().nativeElement.click(); + + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith(["/settings/account"], { + queryParams: { fromNewDeviceVerification: true }, + }); + expect(launchUri).not.toHaveBeenCalled(); + }); + }); + + describe("browser/desktop", () => { + beforeEach(() => { + component["isWeb"] = false; + fixture.detectChanges(); + }); + + it("launches to settings", () => { + changeEmailButton().nativeElement.click(); + + expect(navigate).not.toHaveBeenCalled(); + expect(launchUri).toHaveBeenCalledWith( + "vault.bitwarden.com/#/settings/account/?fromNewDeviceVerification=true", + ); + }); + }); + }); + + describe("enable 2fa", () => { + const changeEmailButton = () => + fixture.debugElement.query(By.css('[data-testid="two-factor"]')); + + describe("web", () => { + beforeEach(() => { + component["isWeb"] = true; + fixture.detectChanges(); + }); + + it("navigates to two factor settings", () => { + changeEmailButton().nativeElement.click(); + + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith(["/settings/security/two-factor"], { + queryParams: { fromNewDeviceVerification: true }, + }); + expect(launchUri).not.toHaveBeenCalled(); + }); + }); + + describe("browser/desktop", () => { + beforeEach(() => { + component["isWeb"] = false; + fixture.detectChanges(); + }); + + it("launches to two factor settings", () => { + changeEmailButton().nativeElement.click(); + + expect(navigate).not.toHaveBeenCalled(); + expect(launchUri).toHaveBeenCalledWith( + "vault.bitwarden.com/#/settings/security/two-factor/?fromNewDeviceVerification=true", + ); + }); + }); + }); + + describe("remind me later", () => { + const remindMeLater = () => + fixture.debugElement.query(By.css('[data-testid="remind-me-later"]')); + + beforeEach(() => { + jest.useFakeTimers(); + jest.setSystemTime(new Date("2024-02-02T00:00:00.000Z")); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("navigates to the vault", () => { + remindMeLater().nativeElement.click(); + + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith(["/vault"]); + }); + + it("updates notice state", () => { + remindMeLater().nativeElement.click(); + + expect(updateNewDeviceVerificationNoticeState).toHaveBeenCalledTimes(1); + expect(updateNewDeviceVerificationNoticeState).toHaveBeenCalledWith("acct-1", { + last_dismissal: new Date("2024-02-02T00:00:00.000Z"), + permanent_dismissal: false, + }); + }); + + it("is hidden when the permanent flag is enabled", async () => { + getFeatureFlag.mockResolvedValueOnce(true); + await component.ngOnInit(); + fixture.detectChanges(); + + expect(remindMeLater()).toBeNull(); + }); + }); +}); diff --git a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.ts b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.ts index 630a2fd516c..b3634dcc28f 100644 --- a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.ts +++ b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-two.component.ts @@ -6,6 +6,8 @@ import { firstValueFrom, Observable } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { ClientType } from "@bitwarden/common/enums"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { Environment, EnvironmentService, @@ -25,6 +27,7 @@ import { NewDeviceVerificationNoticeService } from "../../services/new-device-ve export class NewDeviceVerificationNoticePageTwoComponent implements OnInit { protected isWeb: boolean; protected isDesktop: boolean; + protected permanentFlagEnabled = false; readonly currentAcct$: Observable = this.accountService.activeAccount$; private currentUserId: UserId | null = null; private env$: Observable = this.environmentService.environment$; @@ -35,12 +38,17 @@ export class NewDeviceVerificationNoticePageTwoComponent implements OnInit { private accountService: AccountService, private platformUtilsService: PlatformUtilsService, private environmentService: EnvironmentService, + private configService: ConfigService, ) { this.isWeb = this.platformUtilsService.getClientType() === ClientType.Web; this.isDesktop = this.platformUtilsService.getClientType() === ClientType.Desktop; } async ngOnInit() { + this.permanentFlagEnabled = await this.configService.getFeatureFlag( + FeatureFlag.NewDeviceVerificationPermanentDismiss, + ); + const currentAcct = await firstValueFrom(this.currentAcct$); if (!currentAcct) { return; @@ -83,7 +91,7 @@ export class NewDeviceVerificationNoticePageTwoComponent implements OnInit { async remindMeLaterSelect() { await this.newDeviceVerificationNoticeService.updateNewDeviceVerificationNoticeState( - this.currentUserId, + this.currentUserId!, { last_dismissal: new Date(), permanent_dismissal: false, From be1434e583bfe81c31b8dcdb00ac8c4a1c33e277 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 17 Dec 2024 14:50:12 -0600 Subject: [PATCH 07/15] migrate 2FA check to use the profile property rather than hitting the API directly. The API for 2FA providers is only available on web so it didn't work for browser & native. --- ...w-device-verification-notice.guard.spec.ts | 17 +++++++----- .../new-device-verification-notice.guard.ts | 12 ++++----- .../services/vault-profile.service.spec.ts | 22 +++++++++++++++- .../vault/services/vault-profile.service.ts | 26 ++++++++++++++++++- 4 files changed, 61 insertions(+), 16 deletions(-) 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 eaf6260ed44..fbf30f0a0d4 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 @@ -2,7 +2,6 @@ import { TestBed } from "@angular/core/testing"; import { ActivatedRouteSnapshot, Router, RouterStateSnapshot } from "@angular/router"; import { BehaviorSubject } from "rxjs"; -import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -30,7 +29,7 @@ describe("NewDeviceVerificationNoticeGuard", () => { const createUrlTree = jest.fn(); const getFeatureFlag = jest.fn().mockResolvedValue(null); const isSelfHost = jest.fn().mockResolvedValue(false); - const getTwoFactorProviders = jest.fn().mockResolvedValue({ data: [] }); + const getProfileTwoFactorEnabled = jest.fn().mockResolvedValue(false); const policyAppliesToActiveUser$ = jest.fn().mockReturnValue(new BehaviorSubject(false)); const noticeState$ = jest.fn().mockReturnValue(new BehaviorSubject(null)); const getProfileCreationDate = jest.fn().mockResolvedValue(eightDaysAgo); @@ -38,7 +37,8 @@ describe("NewDeviceVerificationNoticeGuard", () => { beforeEach(() => { getFeatureFlag.mockClear(); isSelfHost.mockClear(); - getTwoFactorProviders.mockClear(); + getProfileCreationDate.mockClear(); + getProfileTwoFactorEnabled.mockClear(); policyAppliesToActiveUser$.mockClear(); createUrlTree.mockClear(); @@ -49,9 +49,11 @@ describe("NewDeviceVerificationNoticeGuard", () => { { provide: NewDeviceVerificationNoticeService, useValue: { noticeState$ } }, { provide: AccountService, useValue: { activeAccount$ } }, { provide: PlatformUtilsService, useValue: { isSelfHost } }, - { provide: ApiService, useValue: { getTwoFactorProviders } }, { provide: PolicyService, useValue: { policyAppliesToActiveUser$ } }, - { provide: VaultProfileService, useValue: { getProfileCreationDate } }, + { + provide: VaultProfileService, + useValue: { getProfileCreationDate, getProfileTwoFactorEnabled }, + }, ], }); }); @@ -85,7 +87,8 @@ describe("NewDeviceVerificationNoticeGuard", () => { expect(getFeatureFlag).not.toHaveBeenCalled(); expect(isSelfHost).not.toHaveBeenCalled(); - expect(getTwoFactorProviders).not.toHaveBeenCalled(); + expect(getProfileTwoFactorEnabled).not.toHaveBeenCalled(); + expect(getProfileCreationDate).not.toHaveBeenCalled(); expect(policyAppliesToActiveUser$).not.toHaveBeenCalled(); }); }); @@ -106,7 +109,7 @@ describe("NewDeviceVerificationNoticeGuard", () => { }); it("returns `true` when 2FA is enabled", async () => { - getTwoFactorProviders.mockResolvedValueOnce({ data: [{ enabled: false }, { enabled: true }] }); + getProfileTwoFactorEnabled.mockResolvedValueOnce(true); expect(await newDeviceGuard()).toBe(true); }); 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 f242365015a..2bb746f32b9 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 @@ -2,7 +2,6 @@ import { inject } from "@angular/core"; import { ActivatedRouteSnapshot, CanActivateFn, Router } from "@angular/router"; import { Observable, firstValueFrom, map } from "rxjs"; -import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -21,7 +20,6 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( const newDeviceVerificationNoticeService = inject(NewDeviceVerificationNoticeService); const accountService = inject(AccountService); const platformUtilsService = inject(PlatformUtilsService); - const apiService = inject(ApiService); const policyService = inject(PolicyService); const vaultProfileService = inject(VaultProfileService); @@ -38,7 +36,7 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( return router.createUrlTree(["/login"]); } - const has2FAEnabled = await hasATwoFactorProviderEnabled(apiService); + const has2FAEnabled = await hasATwoFactorProviderEnabled(vaultProfileService); const isSelfHosted = await platformUtilsService.isSelfHost(); const requiresSSO = await isSSORequired(policyService); const isProfileLessThanWeekOld = await profileIsLessThanWeekOld(vaultProfileService); @@ -80,10 +78,10 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( }; /** Returns true has one 2FA provider enabled */ -async function hasATwoFactorProviderEnabled(apiService: ApiService): Promise { - const twoFactorProviders = await apiService.getTwoFactorProviders(); - - return twoFactorProviders.data.some((provider) => provider.enabled); +async function hasATwoFactorProviderEnabled( + vaultProfileService: VaultProfileService, +): Promise { + return vaultProfileService.getProfileTwoFactorEnabled(); } /** Returns true when the user's profile is less than a week old */ diff --git a/libs/angular/src/vault/services/vault-profile.service.spec.ts b/libs/angular/src/vault/services/vault-profile.service.spec.ts index f56cb1fc052..1802f91ab08 100644 --- a/libs/angular/src/vault/services/vault-profile.service.spec.ts +++ b/libs/angular/src/vault/services/vault-profile.service.spec.ts @@ -8,7 +8,9 @@ describe("VaultProfileService", () => { let service: VaultProfileService; const hardcodedDateString = "2024-02-24T12:00:00Z"; - const getProfile = jest.fn().mockResolvedValue({ creationDate: hardcodedDateString }); + const getProfile = jest + .fn() + .mockResolvedValue({ creationDate: hardcodedDateString, twoFactorEnabled: true }); beforeEach(() => { getProfile.mockClear(); @@ -43,4 +45,22 @@ describe("VaultProfileService", () => { expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); expect(getProfile).not.toHaveBeenCalled(); }); + + it("calls `getProfile` when stored 2FA property is not stored", async () => { + expect(service["profile2FAEnabled"]).toBeNull(); + + const twoFactorEnabled = await service.getProfileTwoFactorEnabled(); + + expect(twoFactorEnabled).toBe(true); + expect(getProfile).toHaveBeenCalled(); + }); + + it("does not call `getProfile` when 2FA property is already stored", async () => { + service["profile2FAEnabled"] = false; + + const twoFactorEnabled = await service.getProfileTwoFactorEnabled(); + + expect(twoFactorEnabled).toBe(false); + expect(getProfile).not.toHaveBeenCalled(); + }); }); diff --git a/libs/angular/src/vault/services/vault-profile.service.ts b/libs/angular/src/vault/services/vault-profile.service.ts index 906744778a0..64a15f0fc84 100644 --- a/libs/angular/src/vault/services/vault-profile.service.ts +++ b/libs/angular/src/vault/services/vault-profile.service.ts @@ -1,6 +1,7 @@ import { Injectable, inject } from "@angular/core"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { ProfileResponse } from "@bitwarden/common/models/response/profile.response"; @Injectable({ providedIn: "root", @@ -12,6 +13,9 @@ export class VaultProfileService { /** Profile creation stored as a string. */ private profileCreatedDate: string | null = null; + /** True when 2FA is enabled on the profile. */ + private profile2FAEnabled: boolean | null = null; + /** * Returns the creation date of the profile. * Note: `Date`s are mutable in JS, creating a new @@ -22,10 +26,30 @@ export class VaultProfileService { return Promise.resolve(new Date(this.profileCreatedDate)); } + const profile = await this.fetchAndCacheProfile(); + + return new Date(profile.creationDate); + } + + /** + * Returns whether there is a 2FA provider on the profile. + */ + async getProfileTwoFactorEnabled(): Promise { + if (this.profile2FAEnabled !== null) { + return Promise.resolve(this.profile2FAEnabled); + } + + const profile = await this.fetchAndCacheProfile(); + + return profile.twoFactorEnabled; + } + + private async fetchAndCacheProfile(): Promise { const profile = await this.apiService.getProfile(); this.profileCreatedDate = profile.creationDate; + this.profile2FAEnabled = profile.twoFactorEnabled; - return new Date(this.profileCreatedDate); + return profile; } } From e842b7599e1f4fc70d8f9cf7b85e52cfeb31d102 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 17 Dec 2024 15:03:52 -0600 Subject: [PATCH 08/15] remove unneeded account related changes - profile creation is used from other sources --- libs/common/spec/fake-account-service.ts | 6 ---- .../src/auth/abstractions/account.service.ts | 8 ----- .../src/auth/services/account.service.spec.ts | 31 +------------------ .../src/auth/services/account.service.ts | 5 --- .../src/platform/sync/default-sync.service.ts | 1 - 5 files changed, 1 insertion(+), 50 deletions(-) diff --git a/libs/common/spec/fake-account-service.ts b/libs/common/spec/fake-account-service.ts index 4e3e23ddae1..05e44d5db18 100644 --- a/libs/common/spec/fake-account-service.ts +++ b/libs/common/spec/fake-account-service.ts @@ -17,7 +17,6 @@ export function mockAccountServiceWith( name: "name", email: "email", emailVerified: true, - createdDate: undefined, }, }; @@ -94,10 +93,6 @@ export class FakeAccountService implements AccountService { await this.mock.setAccountEmailVerified(userId, emailVerified); } - async setAccountCreationDate(userId: UserId, createdDate: string): Promise { - await this.mock.setAccountCreationDate(userId, createdDate); - } - async switchAccount(userId: UserId): Promise { const next = userId == null ? null : { id: userId, ...this.accountsSubject["_buffer"]?.[0]?.[userId] }; @@ -117,5 +112,4 @@ const loggedOutInfo: AccountInfo = { name: undefined, email: "", emailVerified: false, - createdDate: undefined, }; diff --git a/libs/common/src/auth/abstractions/account.service.ts b/libs/common/src/auth/abstractions/account.service.ts index b249365495c..094e005e656 100644 --- a/libs/common/src/auth/abstractions/account.service.ts +++ b/libs/common/src/auth/abstractions/account.service.ts @@ -12,7 +12,6 @@ export type AccountInfo = { email: string; emailVerified: boolean; name: string | undefined; - createdDate?: string; }; export type Account = { id: UserId } & AccountInfo; @@ -74,13 +73,6 @@ export abstract class AccountService { * @param emailVerified */ abstract setAccountEmailVerified(userId: UserId, emailVerified: boolean): Promise; - /** - * Updates the `accounts$` observable with the creation date of the account. - * - * @param userId - * @param creationDate - */ - abstract setAccountCreationDate(userId: UserId, creationDate: string): Promise; /** * Updates the `activeAccount$` observable with the new active account. * @param userId diff --git a/libs/common/src/auth/services/account.service.spec.ts b/libs/common/src/auth/services/account.service.spec.ts index 90f88efa54c..227949156ee 100644 --- a/libs/common/src/auth/services/account.service.spec.ts +++ b/libs/common/src/auth/services/account.service.spec.ts @@ -70,7 +70,7 @@ describe("accountService", () => { let accountsState: FakeGlobalState>; let activeAccountIdState: FakeGlobalState; const userId = Utils.newGuid() as UserId; - const userInfo = { email: "email", name: "name", emailVerified: true, createdDate: "" }; + const userInfo = { email: "email", name: "name", emailVerified: true }; beforeEach(() => { messagingService = mock(); @@ -224,35 +224,6 @@ describe("accountService", () => { }); }); - describe("setAccountCreationDate", () => { - const today = new Date(); - const yesterday = new Date(); - yesterday.setDate(yesterday.getDate() - 1); - - const initialState = { [userId]: userInfo }; - initialState[userId].createdDate = yesterday.toISOString(); - - beforeEach(() => { - accountsState.stateSubject.next(initialState); - }); - - it("updates the account", async () => { - await sut.setAccountCreationDate(userId, today.toISOString()); - const currentState = await firstValueFrom(accountsState.state$); - - expect(currentState).toEqual({ - [userId]: { ...userInfo, createdDate: today.toISOString() }, - }); - }); - - it("does not update if the email is the same", async () => { - await sut.setAccountCreationDate(userId, yesterday.toISOString()); - const currentState = await firstValueFrom(accountsState.state$); - - expect(currentState).toEqual(initialState); - }); - }); - describe("clean", () => { beforeEach(() => { accountsState.stateSubject.next({ [userId]: userInfo }); diff --git a/libs/common/src/auth/services/account.service.ts b/libs/common/src/auth/services/account.service.ts index 409801c2e9b..d4479815c5d 100644 --- a/libs/common/src/auth/services/account.service.ts +++ b/libs/common/src/auth/services/account.service.ts @@ -46,7 +46,6 @@ const LOGGED_OUT_INFO: AccountInfo = { email: "", emailVerified: false, name: undefined, - createdDate: undefined, }; /** @@ -142,10 +141,6 @@ export class AccountServiceImplementation implements InternalAccountService { await this.setAccountInfo(userId, { emailVerified }); } - async setAccountCreationDate(userId: UserId, createdDate: string): Promise { - await this.setAccountInfo(userId, { createdDate }); - } - async clean(userId: UserId) { await this.setAccountInfo(userId, LOGGED_OUT_INFO); await this.removeAccountActivity(userId); diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index 67fd3a5e3b2..9e36aa69417 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -191,7 +191,6 @@ export class DefaultSyncService extends CoreSyncService { await this.avatarService.setSyncAvatarColor(response.id, response.avatarColor); await this.tokenService.setSecurityStamp(response.securityStamp, response.id); await this.accountService.setAccountEmailVerified(response.id, response.emailVerified); - await this.accountService.setAccountCreationDate(response.id, response.creationDate); await this.billingAccountProfileStateService.setHasPremium( response.premiumPersonally, From a3322f889990de5b848780c29d9090b8620d8ca7 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 18 Dec 2024 11:31:14 -0600 Subject: [PATCH 09/15] remove obsolete test --- .../new-device-verification-notice-page-one.component.spec.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts index a273fbcd2d7..c6eccb78739 100644 --- a/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts +++ b/libs/vault/src/components/new-device-verification-notice/new-device-verification-notice-page-one.component.spec.ts @@ -49,9 +49,6 @@ describe("NewDeviceVerificationNoticePageOneComponent", () => { it("sets initial properties", () => { expect(component["currentEmail"]).toBe("test@example.com"); expect(component["currentUserId"]).toBe("acct-1"); - expect(component["formMessage"]).toBe( - "newDeviceVerificationNoticePageOneFormContent test@example.com", - ); }); describe("temporary flag submission", () => { From b2b94c2f1425a27ffac49d4d3c2f27b885160c03 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 18 Dec 2024 11:39:04 -0600 Subject: [PATCH 10/15] store the profile id within the vault service --- libs/angular/src/vault/services/vault-profile.service.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/angular/src/vault/services/vault-profile.service.ts b/libs/angular/src/vault/services/vault-profile.service.ts index 64a15f0fc84..71008822f49 100644 --- a/libs/angular/src/vault/services/vault-profile.service.ts +++ b/libs/angular/src/vault/services/vault-profile.service.ts @@ -10,6 +10,8 @@ import { ProfileResponse } from "@bitwarden/common/models/response/profile.respo export class VaultProfileService { private apiService = inject(ApiService); + private profileId: string | null = null; + /** Profile creation stored as a string. */ private profileCreatedDate: string | null = null; @@ -47,6 +49,7 @@ export class VaultProfileService { private async fetchAndCacheProfile(): Promise { const profile = await this.apiService.getProfile(); + this.profileId = profile.id; this.profileCreatedDate = profile.creationDate; this.profile2FAEnabled = profile.twoFactorEnabled; From e34b3459e6504690e2726327f951f728baa84596 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 18 Dec 2024 12:24:22 -0600 Subject: [PATCH 11/15] remove unused map --- .../vault/guards/new-device-verification-notice.guard.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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 2bb746f32b9..8c3f11f3947 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 @@ -1,6 +1,6 @@ import { inject } from "@angular/core"; import { ActivatedRouteSnapshot, CanActivateFn, Router } from "@angular/router"; -import { Observable, firstValueFrom, map } from "rxjs"; +import { Observable, firstValueFrom } from "rxjs"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; @@ -27,9 +27,7 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( return true; } - const currentAcct$: Observable = accountService.activeAccount$.pipe( - map((acct) => acct), - ); + const currentAcct$: Observable = accountService.activeAccount$; const currentAcct = await firstValueFrom(currentAcct$); if (!currentAcct) { From 9a4295f66a2b6963f12cc51edfaf56edbd9642cd Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 18 Dec 2024 12:39:37 -0600 Subject: [PATCH 12/15] store the associated profile id so account for profile switching in the extension --- .../new-device-verification-notice.guard.ts | 13 +++- .../services/vault-profile.service.spec.ts | 76 +++++++++++++------ .../vault/services/vault-profile.service.ts | 12 +-- 3 files changed, 68 insertions(+), 33 deletions(-) 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 8c3f11f3947..0dfdafb028b 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 @@ -34,10 +34,13 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( return router.createUrlTree(["/login"]); } - const has2FAEnabled = await hasATwoFactorProviderEnabled(vaultProfileService); + const has2FAEnabled = await hasATwoFactorProviderEnabled(vaultProfileService, currentAcct.id); const isSelfHosted = await platformUtilsService.isSelfHost(); const requiresSSO = await isSSORequired(policyService); - const isProfileLessThanWeekOld = await profileIsLessThanWeekOld(vaultProfileService); + const isProfileLessThanWeekOld = await profileIsLessThanWeekOld( + vaultProfileService, + currentAcct.id, + ); // When any of the following are true, the device verification notice is // not applicable for the user. @@ -78,15 +81,17 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( /** Returns true has one 2FA provider enabled */ async function hasATwoFactorProviderEnabled( vaultProfileService: VaultProfileService, + userId: string, ): Promise { - return vaultProfileService.getProfileTwoFactorEnabled(); + return vaultProfileService.getProfileTwoFactorEnabled(userId); } /** Returns true when the user's profile is less than a week old */ async function profileIsLessThanWeekOld( vaultProfileService: VaultProfileService, + userId: string, ): Promise { - const creationDate = await vaultProfileService.getProfileCreationDate(); + const creationDate = await vaultProfileService.getProfileCreationDate(userId); return !isMoreThan7DaysAgo(creationDate); } diff --git a/libs/angular/src/vault/services/vault-profile.service.spec.ts b/libs/angular/src/vault/services/vault-profile.service.spec.ts index 1802f91ab08..51708ed1422 100644 --- a/libs/angular/src/vault/services/vault-profile.service.spec.ts +++ b/libs/angular/src/vault/services/vault-profile.service.spec.ts @@ -6,11 +6,16 @@ import { VaultProfileService } from "./vault-profile.service"; describe("VaultProfileService", () => { let service: VaultProfileService; + const userId = "profile-id"; const hardcodedDateString = "2024-02-24T12:00:00Z"; const getProfile = jest .fn() - .mockResolvedValue({ creationDate: hardcodedDateString, twoFactorEnabled: true }); + .mockResolvedValue({ + creationDate: hardcodedDateString, + twoFactorEnabled: true, + id: "new-user-id", + }); beforeEach(() => { getProfile.mockClear(); @@ -22,45 +27,70 @@ describe("VaultProfileService", () => { jest.useFakeTimers(); jest.setSystemTime(new Date("2024-02-22T00:00:00Z")); service = TestBed.runInInjectionContext(() => new VaultProfileService()); + service["userId"] = userId; }); afterEach(() => { jest.useRealTimers(); }); - it("calls `getProfile` when stored profile date is not set", async () => { - expect(service["profileCreatedDate"]).toBeNull(); + describe("getProfileCreationDate", () => { + it("calls `getProfile` when stored profile date is not set", async () => { + expect(service["profileCreatedDate"]).toBeNull(); - const date = await service.getProfileCreationDate(); + const date = await service.getProfileCreationDate(userId); - expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); - expect(getProfile).toHaveBeenCalled(); - }); + expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); + expect(getProfile).toHaveBeenCalled(); + }); - it("does not call `getProfile` when the date is already stored", async () => { - service["profileCreatedDate"] = hardcodedDateString; + it("calls `getProfile` when stored profile id does not match", async () => { + service["profileCreatedDate"] = hardcodedDateString; + service["userId"] = "old-user-id"; - const date = await service.getProfileCreationDate(); + const date = await service.getProfileCreationDate(userId); - expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); - expect(getProfile).not.toHaveBeenCalled(); - }); + expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); + expect(getProfile).toHaveBeenCalled(); + }); - it("calls `getProfile` when stored 2FA property is not stored", async () => { - expect(service["profile2FAEnabled"]).toBeNull(); + it("does not call `getProfile` when the date is already stored", async () => { + service["profileCreatedDate"] = hardcodedDateString; - const twoFactorEnabled = await service.getProfileTwoFactorEnabled(); + const date = await service.getProfileCreationDate(userId); - expect(twoFactorEnabled).toBe(true); - expect(getProfile).toHaveBeenCalled(); + expect(date.toISOString()).toBe("2024-02-24T12:00:00.000Z"); + expect(getProfile).not.toHaveBeenCalled(); + }); }); - it("does not call `getProfile` when 2FA property is already stored", async () => { - service["profile2FAEnabled"] = false; + describe("getProfileTwoFactorEnabled", () => { + it("calls `getProfile` when stored 2FA property is not stored", async () => { + expect(service["profile2FAEnabled"]).toBeNull(); - const twoFactorEnabled = await service.getProfileTwoFactorEnabled(); + const twoFactorEnabled = await service.getProfileTwoFactorEnabled(userId); + + expect(twoFactorEnabled).toBe(true); + expect(getProfile).toHaveBeenCalled(); + }); - expect(twoFactorEnabled).toBe(false); - expect(getProfile).not.toHaveBeenCalled(); + it("calls `getProfile` when stored profile id does not match", async () => { + service["profile2FAEnabled"] = false; + service["userId"] = "old-user-id"; + + const twoFactorEnabled = await service.getProfileTwoFactorEnabled(userId); + + expect(twoFactorEnabled).toBe(true); + expect(getProfile).toHaveBeenCalled(); + }); + + it("does not call `getProfile` when 2FA property is already stored", async () => { + service["profile2FAEnabled"] = false; + + const twoFactorEnabled = await service.getProfileTwoFactorEnabled(userId); + + expect(twoFactorEnabled).toBe(false); + expect(getProfile).not.toHaveBeenCalled(); + }); }); }); diff --git a/libs/angular/src/vault/services/vault-profile.service.ts b/libs/angular/src/vault/services/vault-profile.service.ts index 71008822f49..1d9c17cd549 100644 --- a/libs/angular/src/vault/services/vault-profile.service.ts +++ b/libs/angular/src/vault/services/vault-profile.service.ts @@ -10,7 +10,7 @@ import { ProfileResponse } from "@bitwarden/common/models/response/profile.respo export class VaultProfileService { private apiService = inject(ApiService); - private profileId: string | null = null; + private userId: string | null = null; /** Profile creation stored as a string. */ private profileCreatedDate: string | null = null; @@ -23,8 +23,8 @@ export class VaultProfileService { * Note: `Date`s are mutable in JS, creating a new * instance is important to avoid unwanted changes. */ - async getProfileCreationDate(): Promise { - if (this.profileCreatedDate) { + async getProfileCreationDate(userId: string): Promise { + if (this.profileCreatedDate && userId === this.userId) { return Promise.resolve(new Date(this.profileCreatedDate)); } @@ -36,8 +36,8 @@ export class VaultProfileService { /** * Returns whether there is a 2FA provider on the profile. */ - async getProfileTwoFactorEnabled(): Promise { - if (this.profile2FAEnabled !== null) { + async getProfileTwoFactorEnabled(userId: string): Promise { + if (this.profile2FAEnabled !== null && userId === this.userId) { return Promise.resolve(this.profile2FAEnabled); } @@ -49,7 +49,7 @@ export class VaultProfileService { private async fetchAndCacheProfile(): Promise { const profile = await this.apiService.getProfile(); - this.profileId = profile.id; + this.userId = profile.id; this.profileCreatedDate = profile.creationDate; this.profile2FAEnabled = profile.twoFactorEnabled; From e048d612a24f3e51b937cb8c3d108b046143780e Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 18 Dec 2024 13:02:22 -0600 Subject: [PATCH 13/15] add comment for temporary service and ticket number to remove --- libs/angular/src/vault/services/vault-profile.service.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libs/angular/src/vault/services/vault-profile.service.ts b/libs/angular/src/vault/services/vault-profile.service.ts index 1d9c17cd549..b368a973781 100644 --- a/libs/angular/src/vault/services/vault-profile.service.ts +++ b/libs/angular/src/vault/services/vault-profile.service.ts @@ -6,7 +6,13 @@ import { ProfileResponse } from "@bitwarden/common/models/response/profile.respo @Injectable({ providedIn: "root", }) -/** Class to provide profile level details without having to call the API each time. */ +/** + * Class to provide profile level details without having to call the API each time. + * NOTE: This is a temporary service and can be replaced once the `UnauthenticatedExtensionUIRefresh` flag goes live. + * The `UnauthenticatedExtensionUIRefresh` introduces a sync that takes place upon logging in. These details can then + * be added to account object and retrieved from there. + * TODO: PM-16202 + */ export class VaultProfileService { private apiService = inject(ApiService); From a801035231622c525e34d8cef293431e959ec3da Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 18 Dec 2024 13:24:08 -0600 Subject: [PATCH 14/15] formatting --- .../src/vault/services/vault-profile.service.spec.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/libs/angular/src/vault/services/vault-profile.service.spec.ts b/libs/angular/src/vault/services/vault-profile.service.spec.ts index 51708ed1422..7761503253a 100644 --- a/libs/angular/src/vault/services/vault-profile.service.spec.ts +++ b/libs/angular/src/vault/services/vault-profile.service.spec.ts @@ -9,13 +9,11 @@ describe("VaultProfileService", () => { const userId = "profile-id"; const hardcodedDateString = "2024-02-24T12:00:00Z"; - const getProfile = jest - .fn() - .mockResolvedValue({ - creationDate: hardcodedDateString, - twoFactorEnabled: true, - id: "new-user-id", - }); + const getProfile = jest.fn().mockResolvedValue({ + creationDate: hardcodedDateString, + twoFactorEnabled: true, + id: "new-user-id", + }); beforeEach(() => { getProfile.mockClear(); From a57b6b79bad07123312267057d3e32c05b290a4b Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 19 Dec 2024 08:58:40 -0600 Subject: [PATCH 15/15] move up logic for feature flags --- ...ew-device-verification-notice.guard.spec.ts | 13 +++++++------ .../new-device-verification-notice.guard.ts | 18 +++++++++++------- 2 files changed, 18 insertions(+), 13 deletions(-) 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 fbf30f0a0d4..e278113a653 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 @@ -27,7 +27,13 @@ describe("NewDeviceVerificationNoticeGuard", () => { const activeAccount$ = new BehaviorSubject(account); const createUrlTree = jest.fn(); - const getFeatureFlag = jest.fn().mockResolvedValue(null); + const getFeatureFlag = jest.fn().mockImplementation((key) => { + if (key === FeatureFlag.NewDeviceVerificationTemporaryDismiss) { + return Promise.resolve(true); + } + + return Promise.resolve(false); + }); const isSelfHost = jest.fn().mockResolvedValue(false); const getProfileTwoFactorEnabled = jest.fn().mockResolvedValue(false); const policyAppliesToActiveUser$ = jest.fn().mockReturnValue(new BehaviorSubject(false)); @@ -66,11 +72,6 @@ describe("NewDeviceVerificationNoticeGuard", () => { ); } - // Baseline, all defaults should allow the guard to pass - it("returns true", async () => { - expect(await newDeviceGuard()).toBe(true); - }); - describe("fromNewDeviceVerification", () => { const route = { queryParams: { fromNewDeviceVerification: "true" }, 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 0dfdafb028b..20550e0e8cf 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 @@ -27,6 +27,17 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( return true; } + const tempNoticeFlag = await configService.getFeatureFlag( + FeatureFlag.NewDeviceVerificationTemporaryDismiss, + ); + const permNoticeFlag = await configService.getFeatureFlag( + FeatureFlag.NewDeviceVerificationPermanentDismiss, + ); + + if (!tempNoticeFlag && !permNoticeFlag) { + return true; + } + const currentAcct$: Observable = accountService.activeAccount$; const currentAcct = await firstValueFrom(currentAcct$); @@ -48,13 +59,6 @@ export const NewDeviceVerificationNoticeGuard: CanActivateFn = async ( return true; } - const tempNoticeFlag = await configService.getFeatureFlag( - FeatureFlag.NewDeviceVerificationTemporaryDismiss, - ); - const permNoticeFlag = await configService.getFeatureFlag( - FeatureFlag.NewDeviceVerificationPermanentDismiss, - ); - const userItems$ = newDeviceVerificationNoticeService.noticeState$(currentAcct.id); const userItems = await firstValueFrom(userItems$);