Migrate provider service to state provider (#8173)

* Migrate existing provider data to StateProvider

Migrate existing provider data to StateProvider

* Rework the ProviderService to call StateProvider

* Unit test the ProviderService

* Update DI to reflect ProviderService's new args

* Add ProviderService to logout chains across products

* Remove provider related stateService methods

* Update libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.spec.ts

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>

* Cover up a copy/paste job

* Compare equality over entire array in a test

---------

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>

apps/browser/src/background/main.background.ts

@@ -700,7 +700,7 @@ export default class MainBackground {
       this.fileUploadService,
       this.sendService,
     );
-    this.providerService = new ProviderService(this.stateService);
+    this.providerService = new ProviderService(this.stateProvider);
     this.syncService = new SyncService(
       this.apiService,
       this.settingsService,
@@ -1114,12 +1114,12 @@ export default class MainBackground {
       this.keyConnectorService.clear(),
       this.vaultFilterService.clear(),
       this.biometricStateService.logout(userId),
-      /*
-      We intentionally do not clear:
-        - autofillSettingsService
-        - badgeSettingsService
-        - userNotificationSettingsService
-      */
+      this.providerService.save(null, userId),
+      /* We intentionally do not clear:
+       *  - autofillSettingsService
+       *  - badgeSettingsService
+       *  - userNotificationSettingsService
+       */
     ]);
 
     //Needs to be checked before state is cleaned

apps/browser/src/popup/services/services.module.ts

@@ -24,7 +24,6 @@ import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaul
 import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service";
 import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
 import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
-import { ProviderService } from "@bitwarden/common/admin-console/abstractions/provider.service";
 import { AccountService as AccountServiceAbstraction } from "@bitwarden/common/auth/abstractions/account.service";
 import { AuthService as AuthServiceAbstraction } from "@bitwarden/common/auth/abstractions/auth.service";
 import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction";
@@ -436,11 +435,6 @@ function getBgService<T>(service: keyof MainBackground) {
         AccountServiceAbstraction,
       ],
     },
-    {
-      provide: ProviderService,
-      useFactory: getBgService<ProviderService>("providerService"),
-      deps: [],
-    },
     {
       provide: SECURE_STORAGE,
       useFactory: getBgService<AbstractStorageService>("secureStorageService"),

apps/cli/src/bw.ts

@@ -388,7 +388,7 @@ export class Main {
       this.stateProvider,
     );
 
-    this.providerService = new ProviderService(this.stateService);
+    this.providerService = new ProviderService(this.stateProvider);
 
     this.organizationService = new OrganizationService(this.stateService, this.stateProvider);
 
@@ -655,6 +655,7 @@ export class Main {
       this.collectionService.clear(userId as UserId),
       this.policyService.clear(userId),
       this.passwordGenerationService.clear(),
+      this.providerService.save(null, userId as UserId),
     ]);
 
     await this.stateEventRunnerService.handleEvent("logout", userId as UserId);

apps/desktop/src/app/app.component.ts

@@ -23,6 +23,7 @@ import { SettingsService } from "@bitwarden/common/abstractions/settings.service
 import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service";
 import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service";
 import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
+import { ProviderService } from "@bitwarden/common/admin-console/abstractions/provider.service";
 import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
 import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service";
 import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
@@ -151,6 +152,7 @@ export class AppComponent implements OnInit, OnDestroy {
     private dialogService: DialogService,
     private biometricStateService: BiometricStateService,
     private stateEventRunnerService: StateEventRunnerService,
+    private providerService: ProviderService,
   ) {}
 
   ngOnInit() {
@@ -584,6 +586,7 @@ export class AppComponent implements OnInit, OnDestroy {
       await this.policyService.clear(userBeingLoggedOut);
       await this.keyConnectorService.clear();
       await this.biometricStateService.logout(userBeingLoggedOut as UserId);
+      await this.providerService.save(null, userBeingLoggedOut as UserId);
 
       await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut as UserId);
 

libs/angular/src/services/jslib-services.module.ts

@@ -745,7 +745,7 @@ import { ModalService } from "./modal.service";
     {
       provide: ProviderServiceAbstraction,
       useClass: ProviderService,
-      deps: [StateServiceAbstraction],
+      deps: [StateProvider],
     },
     {
       provide: TwoFactorServiceAbstraction,

libs/common/src/admin-console/abstractions/provider.service.ts

@@ -1,8 +1,9 @@
+import { UserId } from "../../types/guid";
 import { ProviderData } from "../models/data/provider.data";
 import { Provider } from "../models/domain/provider";
 
 export abstract class ProviderService {
   get: (id: string) => Promise<Provider>;
   getAll: () => Promise<Provider[]>;
-  save: (providers: { [id: string]: ProviderData }) => Promise<any>;
+  save: (providers: { [id: string]: ProviderData }, userId?: UserId) => Promise<any>;
 }

libs/common/src/admin-console/services/provider.service.spec.ts

@@ -1,7 +1,56 @@
+import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../spec";
+import { FakeActiveUserState } from "../../../spec/fake-state";
+import { Utils } from "../../platform/misc/utils";
+import { UserId } from "../../types/guid";
 import { ProviderUserStatusType, ProviderUserType } from "../enums";
 import { ProviderData } from "../models/data/provider.data";
+import { Provider } from "../models/domain/provider";
 
-import { PROVIDERS } from "./provider.service";
+import { PROVIDERS, ProviderService } from "./provider.service";
+
+/**
+ * It is easier to read arrays than records in code, but we store a record
+ * in state. This helper methods lets us build provider arrays in tests
+ * and easily map them to records before storing them in state.
+ */
+function arrayToRecord(input: ProviderData[]): Record<string, ProviderData> {
+  if (input == null) {
+    return undefined;
+  }
+  return Object.fromEntries(input?.map((i) => [i.id, i]));
+}
+
+/**
+ * Builds a simple mock `ProviderData[]` array that can be used in tests
+ * to populate state.
+ * @param count The number of organizations to populate the list with. The
+ * function returns undefined if this is less than 1. The default value is 1.
+ * @param suffix A string to append to data fields on each provider.
+ * This defaults to the index of the organization in the list.
+ * @returns a `ProviderData[]` array that can be used to populate
+ * stateProvider.
+ */
+function buildMockProviders(count = 1, suffix?: string): ProviderData[] {
+  if (count < 1) {
+    return undefined;
+  }
+
+  function buildMockProvider(id: string, name: string): ProviderData {
+    const data = new ProviderData({} as any);
+    data.id = id;
+    data.name = name;
+
+    return data;
+  }
+
+  const mockProviders = [];
+  for (let i = 0; i < count; i++) {
+    const s = suffix ? suffix + i.toString() : i.toString();
+    mockProviders.push(buildMockProvider("provider" + s, "provider" + s));
+  }
+
+  return mockProviders;
+}
 
 describe("PROVIDERS key definition", () => {
   const sut = PROVIDERS;
@@ -21,3 +70,75 @@ describe("PROVIDERS key definition", () => {
     expect(result).toEqual(expectedResult);
   });
 });
+
+describe("ProviderService", () => {
+  let providerService: ProviderService;
+
+  const fakeUserId = Utils.newGuid() as UserId;
+  let fakeAccountService: FakeAccountService;
+  let fakeStateProvider: FakeStateProvider;
+  let fakeActiveUserState: FakeActiveUserState<Record<string, ProviderData>>;
+
+  beforeEach(async () => {
+    fakeAccountService = mockAccountServiceWith(fakeUserId);
+    fakeStateProvider = new FakeStateProvider(fakeAccountService);
+    fakeActiveUserState = fakeStateProvider.activeUser.getFake(PROVIDERS);
+    providerService = new ProviderService(fakeStateProvider);
+  });
+
+  describe("getAll()", () => {
+    it("Returns an array of all providers stored in state", async () => {
+      const mockData: ProviderData[] = buildMockProviders(5);
+      fakeActiveUserState.nextState(arrayToRecord(mockData));
+      const providers = await providerService.getAll();
+      expect(providers).toHaveLength(5);
+      expect(providers).toEqual(mockData.map((x) => new Provider(x)));
+    });
+
+    it("Returns an empty array if no providers are found in state", async () => {
+      const mockData: ProviderData[] = undefined;
+      fakeActiveUserState.nextState(arrayToRecord(mockData));
+      const result = await providerService.getAll();
+      expect(result).toEqual([]);
+    });
+  });
+
+  describe("get()", () => {
+    it("Returns a single provider from state that matches the specified id", async () => {
+      const mockData = buildMockProviders(5);
+      fakeActiveUserState.nextState(arrayToRecord(mockData));
+      const result = await providerService.get(mockData[3].id);
+      expect(result).toEqual(new Provider(mockData[3]));
+    });
+
+    it("Returns undefined if the specified provider id is not found", async () => {
+      const result = await providerService.get("this-provider-does-not-exist");
+      expect(result).toBe(undefined);
+    });
+  });
+
+  describe("save()", () => {
+    it("replaces the entire provider list in state for the active user", async () => {
+      const originalData = buildMockProviders(10);
+      fakeActiveUserState.nextState(arrayToRecord(originalData));
+
+      const newData = buildMockProviders(10, "newData");
+      await providerService.save(arrayToRecord(newData));
+
+      const result = await providerService.getAll();
+
+      expect(result).toEqual(newData);
+      expect(result).not.toEqual(originalData);
+    });
+
+    // This is more or less a test for logouts
+    it("can replace state with null", async () => {
+      const originalData = buildMockProviders(2);
+      fakeActiveUserState.nextState(arrayToRecord(originalData));
+      await providerService.save(null);
+      const result = await providerService.getAll();
+      expect(result).toEqual([]);
+      expect(result).not.toEqual(originalData);
+    });
+  });
+});

libs/common/src/admin-console/services/provider.service.ts

@@ -1,5 +1,7 @@
-import { StateService } from "../../platform/abstractions/state.service";
-import { KeyDefinition, PROVIDERS_DISK } from "../../platform/state";
+import { Observable, map, firstValueFrom } from "rxjs";
+
+import { KeyDefinition, PROVIDERS_DISK, StateProvider } from "../../platform/state";
+import { UserId } from "../../types/guid";
 import { ProviderService as ProviderServiceAbstraction } from "../abstractions/provider.service";
 import { ProviderData } from "../models/data/provider.data";
 import { Provider } from "../models/domain/provider";
@@ -8,32 +10,34 @@ export const PROVIDERS = KeyDefinition.record<ProviderData>(PROVIDERS_DISK, "pro
   deserializer: (obj: ProviderData) => obj,
 });
 
+function mapToSingleProvider(providerId: string) {
+  return map<Provider[], Provider>((providers) => providers?.find((p) => p.id === providerId));
+}
+
 export class ProviderService implements ProviderServiceAbstraction {
-  constructor(private stateService: StateService) {}
+  constructor(private stateProvider: StateProvider) {}
+
+  private providers$(userId?: UserId): Observable<Provider[] | undefined> {
+    return this.stateProvider
+      .getUserState$(PROVIDERS, userId)
+      .pipe(this.mapProviderRecordToArray());
+  }
+
+  private mapProviderRecordToArray() {
+    return map<Record<string, ProviderData>, Provider[]>((providers) =>
+      Object.values(providers ?? {})?.map((o) => new Provider(o)),
+    );
+  }
 
   async get(id: string): Promise<Provider> {
-    const providers = await this.stateService.getProviders();
-    // eslint-disable-next-line
-    if (providers == null || !providers.hasOwnProperty(id)) {
-      return null;
-    }
-
-    return new Provider(providers[id]);
+    return await firstValueFrom(this.providers$().pipe(mapToSingleProvider(id)));
   }
 
   async getAll(): Promise<Provider[]> {
-    const providers = await this.stateService.getProviders();
-    const response: Provider[] = [];
-    for (const id in providers) {
-      // eslint-disable-next-line
-      if (providers.hasOwnProperty(id)) {
-        response.push(new Provider(providers[id]));
-      }
-    }
-    return response;
+    return await firstValueFrom(this.providers$());
   }
 
-  async save(providers: { [id: string]: ProviderData }) {
-    await this.stateService.setProviders(providers);
+  async save(providers: { [id: string]: ProviderData }, userId?: UserId) {
+    await this.stateProvider.setUserState(PROVIDERS, providers, userId);
   }
 }

libs/common/src/platform/abstractions/state.service.ts

@@ -2,7 +2,6 @@ import { Observable } from "rxjs";
 
 import { OrganizationData } from "../../admin-console/models/data/organization.data";
 import { PolicyData } from "../../admin-console/models/data/policy.data";
-import { ProviderData } from "../../admin-console/models/data/provider.data";
 import { Policy } from "../../admin-console/models/domain/policy";
 import { AdminAuthRequestStorable } from "../../auth/models/domain/admin-auth-req-storable";
 import { ForceSetPasswordReason } from "../../auth/models/domain/force-set-password-reason";
@@ -371,8 +370,6 @@ export abstract class StateService<T extends Account = Account> {
    * Sets the user's Pin, encrypted by the user key
    */
   setProtectedPin: (value: string, options?: StorageOptions) => Promise<void>;
-  getProviders: (options?: StorageOptions) => Promise<{ [id: string]: ProviderData }>;
-  setProviders: (value: { [id: string]: ProviderData }, options?: StorageOptions) => Promise<void>;
   getRefreshToken: (options?: StorageOptions) => Promise<string>;
   setRefreshToken: (value: string, options?: StorageOptions) => Promise<void>;
   getRememberedEmail: (options?: StorageOptions) => Promise<string>;

libs/common/src/platform/models/domain/account.ts

@@ -2,7 +2,6 @@ import { Jsonify } from "type-fest";
 
 import { OrganizationData } from "../../../admin-console/models/data/organization.data";
 import { PolicyData } from "../../../admin-console/models/data/policy.data";
-import { ProviderData } from "../../../admin-console/models/data/provider.data";
 import { Policy } from "../../../admin-console/models/domain/policy";
 import { AdminAuthRequestStorable } from "../../../auth/models/domain/admin-auth-req-storable";
 import { ForceSetPasswordReason } from "../../../auth/models/domain/force-set-password-reason";
@@ -96,7 +95,6 @@ export class AccountData {
   addEditCipherInfo?: AddEditCipherInfo;
   eventCollection?: EventData[];
   organizations?: { [id: string]: OrganizationData };
-  providers?: { [id: string]: ProviderData };
 
   static fromJSON(obj: DeepJsonify<AccountData>): AccountData {
     if (obj == null) {

libs/common/src/platform/services/state.service.ts

@@ -3,7 +3,6 @@ import { Jsonify, JsonValue } from "type-fest";
 
 import { OrganizationData } from "../../admin-console/models/data/organization.data";
 import { PolicyData } from "../../admin-console/models/data/policy.data";
-import { ProviderData } from "../../admin-console/models/data/provider.data";
 import { Policy } from "../../admin-console/models/domain/policy";
 import { AccountService } from "../../auth/abstractions/account.service";
 import { AuthenticationStatus } from "../../auth/enums/authentication-status";
@@ -1821,27 +1820,6 @@ export class StateService<
     );
   }
 
-  @withPrototypeForObjectValues(ProviderData)
-  async getProviders(options?: StorageOptions): Promise<{ [id: string]: ProviderData }> {
-    return (
-      await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions()))
-    )?.data?.providers;
-  }
-
-  async setProviders(
-    value: { [id: string]: ProviderData },
-    options?: StorageOptions,
-  ): Promise<void> {
-    const account = await this.getAccount(
-      this.reconcileOptions(options, await this.defaultOnDiskOptions()),
-    );
-    account.data.providers = value;
-    await this.saveAccount(
-      account,
-      this.reconcileOptions(options, await this.defaultOnDiskOptions()),
-    );
-  }
-
   async getRefreshToken(options?: StorageOptions): Promise<string> {
     options = await this.getTimeoutBasedStorageOptions(options);
     return (await this.getAccount(options))?.tokens?.refreshToken;

libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.spec.ts

@@ -0,0 +1,145 @@
+import { any, MockProxy } from "jest-mock-extended";
+
+import { MigrationHelper } from "../migration-helper";
+import { mockMigrationHelper } from "../migration-helper.spec";
+
+import { ProviderMigrator } from "./28-move-provider-state-to-state-provider";
+
+function exampleProvider1() {
+  return JSON.stringify({
+    id: "id",
+    name: "name",
+    status: 0,
+    type: 0,
+    enabled: true,
+    useEvents: true,
+  });
+}
+
+function exampleJSON() {
+  return {
+    global: {
+      otherStuff: "otherStuff1",
+    },
+    authenticatedAccounts: ["user-1", "user-2"],
+    "user-1": {
+      data: {
+        providers: {
+          "provider-id-1": exampleProvider1(),
+          "provider-id-2": {
+            // ...
+          },
+        },
+        otherStuff: "overStuff2",
+      },
+      otherStuff: "otherStuff3",
+    },
+    "user-2": {
+      data: {
+        otherStuff: "otherStuff4",
+      },
+      otherStuff: "otherStuff5",
+    },
+  };
+}
+
+function rollbackJSON() {
+  return {
+    "user_user-1_providers_providers": {
+      "provider-id-1": exampleProvider1(),
+      "provider-id-2": {
+        // ...
+      },
+    },
+    "user_user-2_providers_providers": null as any,
+    global: {
+      otherStuff: "otherStuff1",
+    },
+    authenticatedAccounts: ["user-1", "user-2"],
+    "user-1": {
+      data: {
+        otherStuff: "overStuff2",
+      },
+      otherStuff: "otherStuff3",
+    },
+    "user-2": {
+      data: {
+        otherStuff: "otherStuff4",
+      },
+      otherStuff: "otherStuff5",
+    },
+  };
+}
+
+describe("ProviderMigrator", () => {
+  let helper: MockProxy<MigrationHelper>;
+  let sut: ProviderMigrator;
+  const keyDefinitionLike = {
+    key: "providers",
+    stateDefinition: {
+      name: "providers",
+    },
+  };
+
+  describe("migrate", () => {
+    beforeEach(() => {
+      helper = mockMigrationHelper(exampleJSON(), 28);
+      sut = new ProviderMigrator(27, 28);
+    });
+
+    it("should remove providers from all accounts", async () => {
+      await sut.migrate(helper);
+      expect(helper.set).toHaveBeenCalledWith("user-1", {
+        data: {
+          otherStuff: "overStuff2",
+        },
+        otherStuff: "otherStuff3",
+      });
+    });
+
+    it("should set providers value for each account", async () => {
+      await sut.migrate(helper);
+
+      expect(helper.setToUser).toHaveBeenCalledWith("user-1", keyDefinitionLike, {
+        "provider-id-1": exampleProvider1(),
+        "provider-id-2": {
+          // ...
+        },
+      });
+    });
+  });
+
+  describe("rollback", () => {
+    beforeEach(() => {
+      helper = mockMigrationHelper(rollbackJSON(), 27);
+      sut = new ProviderMigrator(27, 28);
+    });
+
+    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 explicit value back to accounts", async () => {
+      await sut.rollback(helper);
+
+      expect(helper.set).toHaveBeenCalledWith("user-1", {
+        data: {
+          providers: {
+            "provider-id-1": exampleProvider1(),
+            "provider-id-2": {
+              // ...
+            },
+          },
+          otherStuff: "overStuff2",
+        },
+        otherStuff: "otherStuff3",
+      });
+    });
+
+    it("should not try to restore values to missing accounts", async () => {
+      await sut.rollback(helper);
+      expect(helper.set).not.toHaveBeenCalledWith("user-3", any());
+    });
+  });
+});

libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.ts

@@ -0,0 +1,71 @@
+import { Jsonify } from "type-fest";
+
+import { KeyDefinitionLike, MigrationHelper } from "../migration-helper";
+import { Migrator } from "../migrator";
+
+enum ProviderUserStatusType {
+  Invited = 0,
+  Accepted = 1,
+  Confirmed = 2,
+  Revoked = -1,
+}
+
+enum ProviderUserType {
+  ProviderAdmin = 0,
+  ServiceUser = 1,
+}
+
+type ProviderData = {
+  id: string;
+  name: string;
+  status: ProviderUserStatusType;
+  type: ProviderUserType;
+  enabled: boolean;
+  userId: string;
+  useEvents: boolean;
+};
+
+type ExpectedAccountType = {
+  data?: {
+    providers?: Record<string, Jsonify<ProviderData>>;
+  };
+};
+
+const USER_PROVIDERS: KeyDefinitionLike = {
+  key: "providers",
+  stateDefinition: {
+    name: "providers",
+  },
+};
+
+export class ProviderMigrator extends Migrator<27, 28> {
+  async migrate(helper: MigrationHelper): Promise<void> {
+    const accounts = await helper.getAccounts<ExpectedAccountType>();
+    async function migrateAccount(userId: string, account: ExpectedAccountType): Promise<void> {
+      const value = account?.data?.providers;
+      if (value != null) {
+        await helper.setToUser(userId, USER_PROVIDERS, value);
+        delete account.data.providers;
+        await helper.set(userId, account);
+      }
+    }
+
+    await Promise.all(accounts.map(({ userId, account }) => migrateAccount(userId, account)));
+  }
+
+  async rollback(helper: MigrationHelper): Promise<void> {
+    const accounts = await helper.getAccounts<ExpectedAccountType>();
+    async function rollbackAccount(userId: string, account: ExpectedAccountType): Promise<void> {
+      const value = await helper.getFromUser(userId, USER_PROVIDERS);
+      if (account) {
+        account.data = Object.assign(account.data ?? {}, {
+          providers: value,
+        });
+        await helper.set(userId, account);
+      }
+      await helper.setToUser(userId, USER_PROVIDERS, null);
+    }
+
+    await Promise.all(accounts.map(({ userId, account }) => rollbackAccount(userId, account)));
+  }
+}