From 8ab4eecc8a8bb05c19f562f648c10f0d30b2f911 Mon Sep 17 00:00:00 2001 From: Robyn MacCallum Date: Fri, 23 Feb 2024 13:16:42 -0500 Subject: [PATCH] [SM-1065] SmOnboarding state provider (#8037) * Create state definition * Create SmOnboardingTaskService * Replace usage of stateService value with state new state provider * Migrate old state values to state provider * Fix injection of SmOnboardingTasksService * Remove smOnboardingTasks from state * Fix state provider imports * Fix migration after merge from main * Move null handling to SMOnboardingTasksService --- .../overview/overview.component.ts | 17 +- .../overview/sm-onboarding-tasks.service.ts | 34 +++ .../platform/abstractions/state.service.ts | 7 - .../src/platform/models/domain/account.ts | 1 - .../src/platform/services/state.service.ts | 22 -- .../src/platform/state/state-definitions.ts | 5 + libs/common/src/state-migrations/migrate.ts | 6 +- ...-onboarding-key-to-state-providers.spec.ts | 200 ++++++++++++++++++ ...ve-sm-onboarding-key-to-state-providers.ts | 53 +++++ 9 files changed, 307 insertions(+), 38 deletions(-) create mode 100644 bitwarden_license/bit-web/src/app/secrets-manager/overview/sm-onboarding-tasks.service.ts create mode 100644 libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.spec.ts create mode 100644 libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.ts diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/overview/overview.component.ts b/bitwarden_license/bit-web/src/app/secrets-manager/overview/overview.component.ts index c916d6f48b..8dff4cf92f 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/overview/overview.component.ts +++ b/bitwarden_license/bit-web/src/app/secrets-manager/overview/overview.component.ts @@ -11,12 +11,12 @@ import { distinctUntilChanged, take, share, + firstValueFrom, } from "rxjs"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; -import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { DialogService } from "@bitwarden/components"; import { ProjectListView } from "../models/view/project-list.view"; @@ -47,6 +47,8 @@ import { import { ServiceAccountService } from "../service-accounts/service-account.service"; import { SecretsListComponent } from "../shared/secrets-list.component"; +import { SMOnboardingTasks, SMOnboardingTasksService } from "./sm-onboarding-tasks.service"; + type Tasks = { [organizationId: string]: OrganizationTasks; }; @@ -71,6 +73,7 @@ export class OverviewComponent implements OnInit, OnDestroy { protected showOnboarding = false; protected loading = true; protected organizationEnabled = false; + protected onboardingTasks$: Observable; protected view$: Observable<{ allProjects: ProjectListView[]; @@ -87,12 +90,14 @@ export class OverviewComponent implements OnInit, OnDestroy { private serviceAccountService: ServiceAccountService, private dialogService: DialogService, private organizationService: OrganizationService, - private stateService: StateService, private platformUtilsService: PlatformUtilsService, private i18nService: I18nService, + private smOnboardingTasksService: SMOnboardingTasksService, ) {} ngOnInit() { + this.onboardingTasks$ = this.smOnboardingTasksService.smOnboardingTasks$; + const orgId$ = this.route.params.pipe( map((p) => p.organizationId), distinctUntilChanged(), @@ -184,7 +189,7 @@ export class OverviewComponent implements OnInit, OnDestroy { organizationId: string, orgTasks: OrganizationTasks, ): Promise { - const prevTasks = ((await this.stateService.getSMOnboardingTasks()) || {}) as Tasks; + const prevTasks = (await firstValueFrom(this.onboardingTasks$)) as Tasks; const newlyCompletedOrgTasks = Object.fromEntries( Object.entries(orgTasks).filter(([_k, v]) => v === true), ); @@ -196,12 +201,12 @@ export class OverviewComponent implements OnInit, OnDestroy { ...prevTasks[organizationId], ...newlyCompletedOrgTasks, }; - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.stateService.setSMOnboardingTasks({ + + await this.smOnboardingTasksService.setSmOnboardingTasks({ ...prevTasks, [organizationId]: nextOrgTasks, }); + return nextOrgTasks as OrganizationTasks; } diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/overview/sm-onboarding-tasks.service.ts b/bitwarden_license/bit-web/src/app/secrets-manager/overview/sm-onboarding-tasks.service.ts new file mode 100644 index 0000000000..77a218bdf8 --- /dev/null +++ b/bitwarden_license/bit-web/src/app/secrets-manager/overview/sm-onboarding-tasks.service.ts @@ -0,0 +1,34 @@ +import { Injectable } from "@angular/core"; +import { Observable, map } from "rxjs"; + +import { + ActiveUserState, + KeyDefinition, + SM_ONBOARDING_DISK, + StateProvider, +} from "@bitwarden/common/platform/state"; + +export type SMOnboardingTasks = Record>; + +const SM_ONBOARDING_TASKS_KEY = new KeyDefinition(SM_ONBOARDING_DISK, "tasks", { + deserializer: (b) => b, +}); + +@Injectable({ + providedIn: "root", +}) +export class SMOnboardingTasksService { + private smOnboardingTasks: ActiveUserState; + smOnboardingTasks$: Observable; + + constructor(private stateProvider: StateProvider) { + this.smOnboardingTasks = this.stateProvider.getActive(SM_ONBOARDING_TASKS_KEY); + this.smOnboardingTasks$ = this.smOnboardingTasks.state$.pipe(map((tasks) => tasks ?? {})); + } + + async setSmOnboardingTasks(newState: SMOnboardingTasks): Promise { + await this.smOnboardingTasks.update(() => { + return { ...newState }; + }); + } +} diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index a76e0463ab..96a870f93a 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -424,13 +424,6 @@ export abstract class StateService { getAvatarColor: (options?: StorageOptions) => Promise; setAvatarColor: (value: string, options?: StorageOptions) => Promise; - getSMOnboardingTasks: ( - options?: StorageOptions, - ) => Promise>>; - setSMOnboardingTasks: ( - value: Record>, - options?: StorageOptions, - ) => Promise; /** * fetches string value of URL user tried to navigate to while unauthenticated. * @param options Defines the storage options for the URL; Defaults to session Storage. diff --git a/libs/common/src/platform/models/domain/account.ts b/libs/common/src/platform/models/domain/account.ts index a5272d933f..d161616366 100644 --- a/libs/common/src/platform/models/domain/account.ts +++ b/libs/common/src/platform/models/domain/account.ts @@ -223,7 +223,6 @@ export class AccountSettings { serverConfig?: ServerConfigData; approveLoginRequests?: boolean; avatarColor?: string; - smOnboardingTasks?: Record>; trustDeviceChoiceForDecryption?: boolean; /** @deprecated July 2023, left for migration purposes*/ diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index 17e300f08b..cbbfa89b69 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -2154,28 +2154,6 @@ export class StateService< ); } - async getSMOnboardingTasks( - options?: StorageOptions, - ): Promise>> { - return ( - await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskLocalOptions())) - )?.settings?.smOnboardingTasks; - } - - async setSMOnboardingTasks( - value: Record>, - options?: StorageOptions, - ): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()), - ); - account.settings.smOnboardingTasks = value; - return await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultOnDiskLocalOptions()), - ); - } - async getDeepLinkRedirectUrl(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 221d9d5ceb..fca584237f 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -66,3 +66,8 @@ export const VAULT_FILTER_DISK = new StateDefinition("vaultFilter", "disk", { export const NEW_WEB_LAYOUT_BANNER_DISK = new StateDefinition("newWebLayoutBanner", "disk", { web: "disk-local", }); + +// Secrets Manager +export const SM_ONBOARDING_DISK = new StateDefinition("smOnboarding", "disk", { + web: "disk-local", +}); diff --git a/libs/common/src/state-migrations/migrate.ts b/libs/common/src/state-migrations/migrate.ts index 7192c6e070..330c1f6a8d 100644 --- a/libs/common/src/state-migrations/migrate.ts +++ b/libs/common/src/state-migrations/migrate.ts @@ -18,6 +18,7 @@ import { PrivateKeyMigrator } from "./migrations/20-move-private-key-to-state-pr import { CollectionMigrator } from "./migrations/21-move-collections-state-to-state-provider"; import { CollapsedGroupingsMigrator } from "./migrations/22-move-collapsed-groupings-to-state-provider"; import { MoveBiometricPromptsToStateProviders } from "./migrations/23-move-biometric-prompts-to-state-providers"; +import { SmOnboardingTasksMigrator } from "./migrations/24-move-sm-onboarding-key-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"; @@ -28,7 +29,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting import { MinVersionMigrator } from "./migrations/min-version"; export const MIN_VERSION = 2; -export const CURRENT_VERSION = 23; +export const CURRENT_VERSION = 24; export type MinVersion = typeof MIN_VERSION; export function createMigrationBuilder() { @@ -54,7 +55,8 @@ export function createMigrationBuilder() { .with(PrivateKeyMigrator, 19, 20) .with(CollectionMigrator, 20, 21) .with(CollapsedGroupingsMigrator, 21, 22) - .with(MoveBiometricPromptsToStateProviders, 22, CURRENT_VERSION); + .with(MoveBiometricPromptsToStateProviders, 22, 23) + .with(SmOnboardingTasksMigrator, 23, CURRENT_VERSION); } export async function currentVersion( diff --git a/libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.spec.ts b/libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.spec.ts new file mode 100644 index 0000000000..b507d8dcf1 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.spec.ts @@ -0,0 +1,200 @@ +import { MockProxy, any } from "jest-mock-extended"; + +import { MigrationHelper } from "../migration-helper"; +import { mockMigrationHelper } from "../migration-helper.spec"; + +import { SmOnboardingTasksMigrator } from "./24-move-sm-onboarding-key-to-state-providers"; + +function exampleJSON() { + return { + authenticatedAccounts: ["user-1", "user-2", "user-3"], + "user-1": { + settings: { + smOnboardingTasks: { + "0bd005de-c722-473b-a00c-b10101006fcd": { + createProject: true, + createSecret: true, + createServiceAccount: true, + importSecrets: true, + }, + "2f0d26ec-493a-4ed7-9183-b10d013597c8": { + createProject: false, + createSecret: true, + createServiceAccount: false, + importSecrets: true, + }, + }, + someOtherProperty: "Some other value", + }, + otherStuff: "otherStuff", + }, + "user-2": { + settings: { + smOnboardingTasks: { + "000000-0000000-0000000-000000000": { + createProject: false, + createSecret: false, + createServiceAccount: false, + importSecrets: false, + }, + }, + someOtherProperty: "Some other value", + }, + otherStuff: "otherStuff", + }, + }; +} + +function rollbackJSON() { + return { + "user_user-1_smOnboarding_tasks": { + "0bd005de-c722-473b-a00c-b10101006fcd": { + createProject: true, + createSecret: true, + createServiceAccount: true, + importSecrets: true, + }, + "2f0d26ec-493a-4ed7-9183-b10d013597c8": { + createProject: false, + createSecret: true, + createServiceAccount: false, + importSecrets: true, + }, + }, + "user_user-2_smOnboarding_tasks": { + "000000-0000000-0000000-000000000": { + createProject: false, + createSecret: false, + createServiceAccount: false, + importSecrets: false, + }, + }, + authenticatedAccounts: ["user-1", "user-2"], + "user-1": { + settings: { + someOtherProperty: "Some other value", + }, + otherStuff: "otherStuff", + }, + "user-2": { + settings: { + someOtherProperty: "Some other value", + }, + otherStuff: "otherStuff", + }, + }; +} + +describe("SmOnboardingTasksMigrator", () => { + let helper: MockProxy; + let sut: SmOnboardingTasksMigrator; + + const keyDefinitionLike = { + key: "tasks", + stateDefinition: { name: "smOnboarding" }, + }; + + describe("migrate", () => { + beforeEach(() => { + helper = mockMigrationHelper(exampleJSON(), 23); + sut = new SmOnboardingTasksMigrator(23, 24); + }); + + it("should remove smOnboardingTasks from all accounts", async () => { + await sut.migrate(helper); + expect(helper.set).toHaveBeenCalledWith("user-1", { + settings: { + someOtherProperty: "Some other value", + }, + otherStuff: "otherStuff", + }); + }); + + it("should set smOnboardingTasks provider value for each account", async () => { + await sut.migrate(helper); + + expect(helper.setToUser).toHaveBeenCalledWith("user-1", keyDefinitionLike, { + "0bd005de-c722-473b-a00c-b10101006fcd": { + createProject: true, + createSecret: true, + createServiceAccount: true, + importSecrets: true, + }, + "2f0d26ec-493a-4ed7-9183-b10d013597c8": { + createProject: false, + createSecret: true, + createServiceAccount: false, + importSecrets: true, + }, + }); + + expect(helper.setToUser).toHaveBeenCalledWith("user-2", keyDefinitionLike, { + "000000-0000000-0000000-000000000": { + createProject: false, + createSecret: false, + createServiceAccount: false, + importSecrets: false, + }, + }); + }); + }); + + describe("rollback", () => { + beforeEach(() => { + helper = mockMigrationHelper(rollbackJSON(), 23); + sut = new SmOnboardingTasksMigrator(23, 24); + }); + + it.each(["user-1", "user-2"])("should null out new values", async (userId) => { + await sut.rollback(helper); + + expect(helper.setToUser).toHaveBeenCalledWith(userId, keyDefinitionLike, null); + }); + + it("should add smOnboardingTasks back to accounts", async () => { + await sut.rollback(helper); + + expect(helper.set).toHaveBeenCalledWith("user-1", { + settings: { + smOnboardingTasks: { + "0bd005de-c722-473b-a00c-b10101006fcd": { + createProject: true, + createSecret: true, + createServiceAccount: true, + importSecrets: true, + }, + "2f0d26ec-493a-4ed7-9183-b10d013597c8": { + createProject: false, + createSecret: true, + createServiceAccount: false, + importSecrets: true, + }, + }, + someOtherProperty: "Some other value", + }, + otherStuff: "otherStuff", + }); + + expect(helper.set).toHaveBeenCalledWith("user-2", { + settings: { + smOnboardingTasks: { + "000000-0000000-0000000-000000000": { + createProject: false, + createSecret: false, + createServiceAccount: false, + importSecrets: false, + }, + }, + someOtherProperty: "Some other value", + }, + otherStuff: "otherStuff", + }); + }); + + it("should not try to restore values to missing accounts", async () => { + await sut.rollback(helper); + + expect(helper.set).not.toHaveBeenCalledWith("user-3", any()); + }); + }); +}); diff --git a/libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.ts b/libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.ts new file mode 100644 index 0000000000..bcd6374586 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/24-move-sm-onboarding-key-to-state-providers.ts @@ -0,0 +1,53 @@ +import { KeyDefinitionLike, MigrationHelper } from "../migration-helper"; +import { Migrator } from "../migrator"; + +type ExpectedAccountType = { + settings?: { + smOnboardingTasks?: Record>; + }; +}; + +export const SM_ONBOARDING_TASKS: KeyDefinitionLike = { + key: "tasks", + stateDefinition: { name: "smOnboarding" }, +}; + +export class SmOnboardingTasksMigrator extends Migrator<23, 24> { + async migrate(helper: MigrationHelper): Promise { + const legacyAccounts = await helper.getAccounts(); + + await Promise.all( + legacyAccounts.map(async ({ userId, account }) => { + // Move account data + if (account?.settings?.smOnboardingTasks != null) { + await helper.setToUser(userId, SM_ONBOARDING_TASKS, account.settings.smOnboardingTasks); + + // Delete old account data + delete account.settings.smOnboardingTasks; + await helper.set(userId, account); + } + }), + ); + } + + async rollback(helper: MigrationHelper): Promise { + async function rollbackUser(userId: string, account: ExpectedAccountType) { + const smOnboardingTasks = await helper.getFromUser>>( + userId, + SM_ONBOARDING_TASKS, + ); + if (smOnboardingTasks) { + account ??= {}; + account.settings ??= {}; + + account.settings.smOnboardingTasks = smOnboardingTasks; + await helper.setToUser(userId, SM_ONBOARDING_TASKS, null); + await helper.set(userId, account); + } + } + + const accounts = await helper.getAccounts(); + + await Promise.all(accounts.map(({ userId, account }) => rollbackUser(userId, account))); + } +}