From e9865c1cec212ed4dda58adf238a8d854479350c Mon Sep 17 00:00:00 2001 From: SmithThe4th Date: Tue, 6 Feb 2024 15:15:22 -0500 Subject: [PATCH] [PM-5275] Migrate state in Fido2ClientService to State Providers (#7745) * added state definition and key definition * created vault settings service * created enable passkeys migrations * created enable passkeys migrations * renamed the state definition * created vault settings service * updated enable passkey key definition * updated references with vault settings service * renamed files to avoid conflict * removed set and get enable passkeys from state service * removed comment * fixed comments * added readonly keyword * removed service registartion from service module * removed readonly keyword from abstract class * swicted to used optional chaining * renamed files * added disk-local argument for web --- .../browser/src/background/main.background.ts | 6 ++ .../src/popup/settings/options.component.ts | 7 +- .../src/services/jslib-services.module.ts | 7 ++ .../platform/abstractions/state.service.ts | 2 - .../src/platform/services/state.service.ts | 18 ---- .../src/platform/state/state-definitions.ts | 4 + libs/common/src/state-migrations/migrate.ts | 6 +- ...ove-folder-state-to-state-provider.spec.ts | 2 +- ...enable-passkeys-to-state-providers.spec.ts | 84 +++++++++++++++++++ ...move-enable-passkeys-to-state-providers.ts | 36 ++++++++ .../vault-settings/vault-settings.service.ts | 17 ++++ .../fido2/fido2-client.service.spec.ts | 17 +++- .../services/fido2/fido2-client.service.ts | 4 +- .../key-state/enable-passkey.state.ts | 9 ++ .../vault-settings/vault-settings.service.ts | 29 +++++++ 15 files changed, 218 insertions(+), 30 deletions(-) create mode 100644 libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.spec.ts create mode 100644 libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.ts create mode 100644 libs/common/src/vault/abstractions/vault-settings/vault-settings.service.ts create mode 100644 libs/common/src/vault/services/key-state/enable-passkey.state.ts create mode 100644 libs/common/src/vault/services/vault-settings/vault-settings.service.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 43f7371812..2f47c4d71f 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -119,6 +119,7 @@ import { InternalFolderService as InternalFolderServiceAbstraction } from "@bitw import { SyncNotifierService as SyncNotifierServiceAbstraction } from "@bitwarden/common/vault/abstractions/sync/sync-notifier.service.abstraction"; import { SyncService as SyncServiceAbstraction } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { TotpService as TotpServiceAbstraction } from "@bitwarden/common/vault/abstractions/totp.service"; +import { VaultSettingsService as VaultSettingsServiceAbstraction } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { CipherService } from "@bitwarden/common/vault/services/cipher.service"; import { CollectionService } from "@bitwarden/common/vault/services/collection.service"; @@ -130,6 +131,7 @@ import { FolderService } from "@bitwarden/common/vault/services/folder/folder.se import { SyncNotifierService } from "@bitwarden/common/vault/services/sync/sync-notifier.service"; import { SyncService } from "@bitwarden/common/vault/services/sync/sync.service"; import { TotpService } from "@bitwarden/common/vault/services/totp.service"; +import { VaultSettingsService } from "@bitwarden/common/vault/services/vault-settings/vault-settings.service"; import { IndividualVaultExportService, IndividualVaultExportServiceAbstraction, @@ -268,6 +270,7 @@ export default class MainBackground { fido2Service: Fido2ServiceAbstraction; individualVaultExportService: IndividualVaultExportServiceAbstraction; organizationVaultExportService: OrganizationVaultExportServiceAbstraction; + vaultSettingsService: VaultSettingsServiceAbstraction; // Passed to the popup for Safari to workaround issues with theming, downloading, etc. backgroundWindow = window; @@ -590,6 +593,8 @@ export default class MainBackground { this.accountService, ); + this.vaultSettingsService = new VaultSettingsService(this.stateProvider); + this.vaultTimeoutService = new VaultTimeoutService( this.cipherService, this.folderService, @@ -719,6 +724,7 @@ export default class MainBackground { this.configService, this.authService, this.stateService, + this.vaultSettingsService, this.logService, ); diff --git a/apps/browser/src/popup/settings/options.component.ts b/apps/browser/src/popup/settings/options.component.ts index eb8025b431..9d96c002cd 100644 --- a/apps/browser/src/popup/settings/options.component.ts +++ b/apps/browser/src/popup/settings/options.component.ts @@ -1,4 +1,5 @@ import { Component, OnInit } from "@angular/core"; +import { firstValueFrom } from "rxjs"; import { AbstractThemingService } from "@bitwarden/angular/platform/services/theming/theming.service.abstraction"; import { SettingsService } from "@bitwarden/common/abstractions/settings.service"; @@ -7,6 +8,7 @@ import { MessagingService } from "@bitwarden/common/platform/abstractions/messag import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { ThemeType } from "@bitwarden/common/platform/enums"; import { TotpService } from "@bitwarden/common/vault/abstractions/totp.service"; +import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; import { UriMatchType } from "@bitwarden/common/vault/enums"; import { enableAccountSwitching } from "../../platform/flags"; @@ -47,6 +49,7 @@ export class OptionsComponent implements OnInit { i18nService: I18nService, private themingService: AbstractThemingService, private settingsService: SettingsService, + private vaultSettingsService: VaultSettingsService, ) { this.themeOptions = [ { name: i18nService.t("default"), value: ThemeType.System }, @@ -102,7 +105,7 @@ export class OptionsComponent implements OnInit { this.enableBadgeCounter = !(await this.stateService.getDisableBadgeCounter()); - this.enablePasskeys = await this.stateService.getEnablePasskeys(); + this.enablePasskeys = await firstValueFrom(this.vaultSettingsService.enablePasskeys$); this.theme = await this.stateService.getTheme(); @@ -123,7 +126,7 @@ export class OptionsComponent implements OnInit { } async updateEnablePasskeys() { - await this.stateService.setEnablePasskeys(this.enablePasskeys); + await this.vaultSettingsService.setEnablePasskeys(this.enablePasskeys); } async updateContextMenuItem() { diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index b6daf63300..91275315b2 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -172,6 +172,7 @@ import { import { SyncNotifierService as SyncNotifierServiceAbstraction } from "@bitwarden/common/vault/abstractions/sync/sync-notifier.service.abstraction"; import { SyncService as SyncServiceAbstraction } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { TotpService as TotpServiceAbstraction } from "@bitwarden/common/vault/abstractions/totp.service"; +import { VaultSettingsService as VaultSettingsServiceAbstraction } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; import { CipherService } from "@bitwarden/common/vault/services/cipher.service"; import { CollectionService } from "@bitwarden/common/vault/services/collection.service"; import { CipherFileUploadService } from "@bitwarden/common/vault/services/file-upload/cipher-file-upload.service"; @@ -180,6 +181,7 @@ import { FolderService } from "@bitwarden/common/vault/services/folder/folder.se import { SyncNotifierService } from "@bitwarden/common/vault/services/sync/sync-notifier.service"; import { SyncService } from "@bitwarden/common/vault/services/sync/sync.service"; import { TotpService } from "@bitwarden/common/vault/services/totp.service"; +import { VaultSettingsService } from "@bitwarden/common/vault/services/vault-settings/vault-settings.service"; import { VaultExportService, VaultExportServiceAbstraction, @@ -902,6 +904,11 @@ import { ModalService } from "./modal.service"; useClass: DefaultBiometricStateService, deps: [StateProvider], }, + { + provide: VaultSettingsServiceAbstraction, + useClass: VaultSettingsService, + deps: [StateProvider], + }, ], }) export class JslibServicesModule {} diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index a2bd893f41..e4ac51ed7b 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -252,8 +252,6 @@ export abstract class StateService { value: boolean, options?: StorageOptions, ) => Promise; - getEnablePasskeys: (options?: StorageOptions) => Promise; - setEnablePasskeys: (value: boolean, options?: StorageOptions) => Promise; getDisableContextMenuItem: (options?: StorageOptions) => Promise; setDisableContextMenuItem: (value: boolean, options?: StorageOptions) => Promise; /** diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index 164542e150..67ee47098d 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -1211,24 +1211,6 @@ export class StateService< ); } - async getEnablePasskeys(options?: StorageOptions): Promise { - return ( - (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) - ?.enablePasskeys ?? true - ); - } - - async setEnablePasskeys(value: boolean, options?: StorageOptions): Promise { - const globals = await this.getGlobals( - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - globals.enablePasskeys = value; - await this.saveGlobals( - globals, - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - } - async getDisableContextMenuItem(options?: StorageOptions): Promise { return ( (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index 1ce74f7fbd..9a1f21e298 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -39,3 +39,7 @@ export const PROVIDERS_DISK = new StateDefinition("providers", "disk"); export const FOLDER_DISK = new StateDefinition("folder", "disk", { web: "memory" }); export const SYNC_STATE = new StateDefinition("sync", "disk", { web: "memory" }); + +export const VAULT_SETTINGS_DISK = new StateDefinition("vaultSettings", "disk", { + web: "disk-local", +}); diff --git a/libs/common/src/state-migrations/migrate.ts b/libs/common/src/state-migrations/migrate.ts index cb62b483a3..05c26cb5ee 100644 --- a/libs/common/src/state-migrations/migrate.ts +++ b/libs/common/src/state-migrations/migrate.ts @@ -12,6 +12,7 @@ import { ProviderKeyMigrator } from "./migrations/13-move-provider-keys-to-state import { MoveBiometricClientKeyHalfToStateProviders } from "./migrations/14-move-biometric-client-key-half-state-to-providers"; import { FolderMigrator } from "./migrations/15-move-folder-state-to-state-provider"; import { LastSyncMigrator } from "./migrations/16-move-last-sync-to-state-provider"; +import { EnablePasskeysMigrator } from "./migrations/17-move-enable-passkeys-to-state-providers"; import { FixPremiumMigrator } from "./migrations/3-fix-premium"; import { RemoveEverBeenUnlockedMigrator } from "./migrations/4-remove-ever-been-unlocked"; import { AddKeyTypeToOrgKeysMigrator } from "./migrations/5-add-key-type-to-org-keys"; @@ -22,7 +23,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting import { MinVersionMigrator } from "./migrations/min-version"; export const MIN_VERSION = 2; -export const CURRENT_VERSION = 16; +export const CURRENT_VERSION = 17; export type MinVersion = typeof MIN_VERSION; export async function migrate( @@ -54,7 +55,8 @@ export async function migrate( .with(ProviderKeyMigrator, 12, 13) .with(MoveBiometricClientKeyHalfToStateProviders, 13, 14) .with(FolderMigrator, 14, 15) - .with(LastSyncMigrator, 15, CURRENT_VERSION) + .with(LastSyncMigrator, 15, 16) + .with(EnablePasskeysMigrator, 16, CURRENT_VERSION) .migrate(migrationHelper); } diff --git a/libs/common/src/state-migrations/migrations/15-move-folder-state-to-state-provider.spec.ts b/libs/common/src/state-migrations/migrations/15-move-folder-state-to-state-provider.spec.ts index 06d67fe3ad..d990bf8f14 100644 --- a/libs/common/src/state-migrations/migrations/15-move-folder-state-to-state-provider.spec.ts +++ b/libs/common/src/state-migrations/migrations/15-move-folder-state-to-state-provider.spec.ts @@ -120,7 +120,7 @@ describe("FolderMigrator", () => { describe("rollback", () => { beforeEach(() => { - helper = mockMigrationHelper(rollbackJSON(), 14); + helper = mockMigrationHelper(rollbackJSON(), 15); sut = new FolderMigrator(14, 15); }); diff --git a/libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.spec.ts b/libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.spec.ts new file mode 100644 index 0000000000..d589e05df2 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.spec.ts @@ -0,0 +1,84 @@ +import { MockProxy } from "jest-mock-extended"; + +import { MigrationHelper } from "../migration-helper"; +import { mockMigrationHelper } from "../migration-helper.spec"; + +import { EnablePasskeysMigrator } from "./17-move-enable-passkeys-to-state-providers"; + +function exampleJSON() { + return { + global: { + enablePasskeys: true, + otherStuff: "otherStuff1", + }, + authenticatedAccounts: ["user-1", "user-2"], + "user-1": { + settings: { + otherStuff: "otherStuff2", + }, + otherStuff: "otherStuff3", + }, + "user-2": { + settings: { + otherStuff: "otherStuff4", + }, + otherStuff: "otherStuff5", + }, + }; +} + +function rollbackJSON() { + return { + global_vaultSettings_enablePasskeys: true, + global: { + otherStuff: "otherStuff1", + }, + authenticatedAccounts: ["user-1", "user-2"], + "user-1": { + settings: { + otherStuff: "otherStuff2", + }, + otherStuff: "otherStuff3", + }, + "user-2": { + settings: { + otherStuff: "otherStuff4", + }, + otherStuff: "otherStuff5", + }, + }; +} + +describe("EnablePasskeysMigrator", () => { + let helper: MockProxy; + let sut: EnablePasskeysMigrator; + + describe("migrate", () => { + beforeEach(() => { + helper = mockMigrationHelper(exampleJSON(), 16); + sut = new EnablePasskeysMigrator(16, 17); + }); + + it("should remove enablePasskeys from global", async () => { + await sut.migrate(helper); + expect(helper.set).toHaveBeenCalledWith("global", { + otherStuff: "otherStuff1", + }); + }); + }); + + describe("rollback", () => { + beforeEach(() => { + helper = mockMigrationHelper(rollbackJSON(), 17); + sut = new EnablePasskeysMigrator(16, 17); + }); + + it("should move enablePasskeys to global", async () => { + await sut.rollback(helper); + expect(helper.set).toHaveBeenCalledWith("global", { + enablePasskeys: true, + otherStuff: "otherStuff1", + }); + }); + }); +}); diff --git a/libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.ts b/libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.ts new file mode 100644 index 0000000000..fb332e51ac --- /dev/null +++ b/libs/common/src/state-migrations/migrations/17-move-enable-passkeys-to-state-providers.ts @@ -0,0 +1,36 @@ +import { KeyDefinitionLike, MigrationHelper } from "../migration-helper"; +import { Migrator } from "../migrator"; + +type ExpectedGlobalType = { + enablePasskeys?: boolean; +}; + +const USER_ENABLE_PASSKEYS: KeyDefinitionLike = { + key: "enablePasskeys", + stateDefinition: { + name: "vaultSettings", + }, +}; + +export class EnablePasskeysMigrator extends Migrator<16, 17> { + async migrate(helper: MigrationHelper): Promise { + const global = await helper.get("global"); + + if (global?.enablePasskeys != null) { + await helper.setToGlobal(USER_ENABLE_PASSKEYS, global.enablePasskeys); + delete global?.enablePasskeys; + await helper.set("global", global); + } + } + + async rollback(helper: MigrationHelper): Promise { + let global = await helper.get("global"); + const globalEnablePasskeys = await helper.getFromGlobal(USER_ENABLE_PASSKEYS); + + if (globalEnablePasskeys != null) { + global = Object.assign(global ?? {}, { enablePasskeys: globalEnablePasskeys }); + await helper.set("global", global); + await helper.setToGlobal(USER_ENABLE_PASSKEYS, undefined); + } + } +} diff --git a/libs/common/src/vault/abstractions/vault-settings/vault-settings.service.ts b/libs/common/src/vault/abstractions/vault-settings/vault-settings.service.ts new file mode 100644 index 0000000000..9e935b763c --- /dev/null +++ b/libs/common/src/vault/abstractions/vault-settings/vault-settings.service.ts @@ -0,0 +1,17 @@ +import { Observable } from "rxjs"; +/** + * Service for managing vault settings. + */ +export abstract class VaultSettingsService { + /** + * An observable monitoring the state of the enable passkeys setting. + * The observable updates when the setting changes. + */ + enablePasskeys$: Observable; + + /** + * Saves the enable passkeys setting to disk. + * @param value The new value for the passkeys setting. + */ + setEnablePasskeys: (value: boolean) => Promise; +} diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts b/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts index 670d623d9f..ad812e309f 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts @@ -17,6 +17,7 @@ import { CreateCredentialParams, FallbackRequestedError, } from "../../abstractions/fido2/fido2-client.service.abstraction"; +import { VaultSettingsService } from "../../abstractions/vault-settings/vault-settings.service"; import { Fido2AuthenticatorService } from "./fido2-authenticator.service"; import { Fido2ClientService } from "./fido2-client.service"; @@ -32,6 +33,7 @@ describe("FidoAuthenticatorService", () => { let configService!: MockProxy; let authService!: MockProxy; let stateService!: MockProxy; + let vaultSettingsService: MockProxy; let client!: Fido2ClientService; let tab!: chrome.tabs.Tab; @@ -40,10 +42,17 @@ describe("FidoAuthenticatorService", () => { configService = mock(); authService = mock(); stateService = mock(); + vaultSettingsService = mock(); - client = new Fido2ClientService(authenticator, configService, authService, stateService); + client = new Fido2ClientService( + authenticator, + configService, + authService, + stateService, + vaultSettingsService, + ); configService.serverConfig$ = of({ environment: { vault: VaultUrl } } as any); - stateService.getEnablePasskeys.mockResolvedValue(true); + vaultSettingsService.enablePasskeys$ = of(true); authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked); tab = { id: 123, windowId: 456 } as chrome.tabs.Tab; }); @@ -226,7 +235,7 @@ describe("FidoAuthenticatorService", () => { it("should throw FallbackRequestedError if passkeys state is not enabled", async () => { const params = createParams(); - stateService.getEnablePasskeys.mockResolvedValue(false); + vaultSettingsService.enablePasskeys$ = of(false); const result = async () => await client.createCredential(params, tab); @@ -396,7 +405,7 @@ describe("FidoAuthenticatorService", () => { it("should throw FallbackRequestedError if passkeys state is not enabled", async () => { const params = createParams(); - stateService.getEnablePasskeys.mockResolvedValue(false); + vaultSettingsService.enablePasskeys$ = of(false); const result = async () => await client.assertCredential(params, tab); diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.ts b/libs/common/src/vault/services/fido2/fido2-client.service.ts index 836afdd1c9..05243f0523 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.ts @@ -26,6 +26,7 @@ import { UserRequestedFallbackAbortReason, UserVerification, } from "../../abstractions/fido2/fido2-client.service.abstraction"; +import { VaultSettingsService } from "../../abstractions/vault-settings/vault-settings.service"; import { isValidRpId } from "./domain-utils"; import { Fido2Utils } from "./fido2-utils"; @@ -42,11 +43,12 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { private configService: ConfigServiceAbstraction, private authService: AuthService, private stateService: StateService, + private vaultSettingsService: VaultSettingsService, private logService?: LogService, ) {} async isFido2FeatureEnabled(hostname: string, origin: string): Promise { - const userEnabledPasskeys = await this.stateService.getEnablePasskeys(); + const userEnabledPasskeys = await firstValueFrom(this.vaultSettingsService.enablePasskeys$); const isUserLoggedIn = (await this.authService.getAuthStatus()) !== AuthenticationStatus.LoggedOut; diff --git a/libs/common/src/vault/services/key-state/enable-passkey.state.ts b/libs/common/src/vault/services/key-state/enable-passkey.state.ts new file mode 100644 index 0000000000..dccbf8fd11 --- /dev/null +++ b/libs/common/src/vault/services/key-state/enable-passkey.state.ts @@ -0,0 +1,9 @@ +import { VAULT_SETTINGS_DISK, KeyDefinition } from "../../../platform/state"; + +export const USER_ENABLE_PASSKEYS = new KeyDefinition( + VAULT_SETTINGS_DISK, + "enablePasskeys", + { + deserializer: (obj) => obj, + }, +); diff --git a/libs/common/src/vault/services/vault-settings/vault-settings.service.ts b/libs/common/src/vault/services/vault-settings/vault-settings.service.ts new file mode 100644 index 0000000000..004c18f426 --- /dev/null +++ b/libs/common/src/vault/services/vault-settings/vault-settings.service.ts @@ -0,0 +1,29 @@ +import { Observable, map } from "rxjs"; + +import { GlobalState, StateProvider } from "../../../platform/state"; +import { VaultSettingsService as VaultSettingsServiceAbstraction } from "../../abstractions/vault-settings/vault-settings.service"; +import { USER_ENABLE_PASSKEYS } from "../key-state/enable-passkey.state"; + +/** + * {@link VaultSettingsServiceAbstraction} + */ +export class VaultSettingsService implements VaultSettingsServiceAbstraction { + private enablePasskeysState: GlobalState = + this.stateProvider.getGlobal(USER_ENABLE_PASSKEYS); + + /** + * {@link VaultSettingsServiceAbstraction.enablePasskeys$} + */ + readonly enablePasskeys$: Observable = this.enablePasskeysState.state$.pipe( + map((x) => x ?? false), + ); + + constructor(private stateProvider: StateProvider) {} + + /** + * {@link VaultSettingsServiceAbstraction.setEnablePasskeys} + */ + async setEnablePasskeys(value: boolean): Promise { + await this.enablePasskeysState.update(() => value); + } +}