[PM-14247] vNextCollectionService fixes (#12362)

* Fix interface to not take observables

* Filter out null orgKeys during transitional state

libs/admin-console/src/common/collections/abstractions/vnext-collection.service.ts

@@ -9,8 +9,8 @@ import { TreeNode } from "@bitwarden/common/vault/models/domain/tree-node";
 import { CollectionData, Collection, CollectionView } from "../models";
 
 export abstract class vNextCollectionService {
-  encryptedCollections$: (userId$: Observable<UserId>) => Observable<Collection[]>;
+  encryptedCollections$: (userId: UserId) => Observable<Collection[]>;
-  decryptedCollections$: (userId$: Observable<UserId>) => Observable<CollectionView[]>;
+  decryptedCollections$: (userId: UserId) => Observable<CollectionView[]>;
   upsert: (collection: CollectionData | CollectionData[], userId: UserId) => Promise<any>;
   replace: (collections: { [id: string]: CollectionData }, userId: UserId) => Promise<any>;
   /**
@@ -22,7 +22,7 @@ export abstract class vNextCollectionService {
    * Clear decrypted and encrypted state.
    * Used for logging out.
    */
-  clear: (userId: string) => Promise<void>;
+  clear: (userId: UserId) => Promise<void>;
   delete: (id: string | string[], userId: UserId) => Promise<any>;
   encrypt: (model: CollectionView) => Promise<Collection>;
   /**
@@ -30,7 +30,7 @@ export abstract class vNextCollectionService {
    */
   decryptMany: (
     collections: Collection[],
-    orgKeys?: Record<OrganizationId, OrgKey>,
+    orgKeys?: Record<OrganizationId, OrgKey> | null,
   ) => Promise<CollectionView[]>;
   /**
    * Transforms the input CollectionViews into TreeNodes

libs/admin-console/src/common/collections/services/default-vnext-collection.service.spec.ts

@@ -1,5 +1,5 @@
 import { mock, MockProxy } from "jest-mock-extended";
-import { firstValueFrom, of, ReplaySubject } from "rxjs";
+import { first, firstValueFrom, of, ReplaySubject, takeWhile } from "rxjs";
 
 import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
@@ -87,7 +87,7 @@ describe("DefaultvNextCollectionService", () => {
         [org2]: orgKey2,
       });
 
-      const result = await firstValueFrom(collectionService.decryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.decryptedCollections$(userId));
 
       // Assert emitted values
       expect(result.length).toBe(2);
@@ -121,11 +121,38 @@ describe("DefaultvNextCollectionService", () => {
       cryptoKeys.next({});
 
       const encryptedCollections = await firstValueFrom(
-        collectionService.encryptedCollections$(of(userId)),
+        collectionService.encryptedCollections$(userId),
       );
 
       expect(encryptedCollections.length).toBe(0);
     });
+
+    it("handles undefined orgKeys", (done) => {
+      // Arrange test data
+      const org1 = Utils.newGuid() as OrganizationId;
+      const collection1 = collectionDataFactory(org1);
+
+      const org2 = Utils.newGuid() as OrganizationId;
+      const collection2 = collectionDataFactory(org2);
+
+      // Emit a non-null value after the first undefined value has propagated
+      // This will cause the collections to emit, calling done()
+      cryptoKeys.pipe(first()).subscribe((val) => {
+        cryptoKeys.next({});
+      });
+
+      collectionService
+        .decryptedCollections$(userId)
+        .pipe(takeWhile((val) => val.length != 2))
+        .subscribe({ complete: () => done() });
+
+      // Arrange dependencies
+      void setEncryptedState([collection1, collection2]).then(() => {
+        // Act: emit undefined
+        cryptoKeys.next(undefined);
+        keyService.activeUserOrgKeys$ = of(undefined);
+      });
+    });
   });
 
   describe("encryptedCollections$", () => {
@@ -137,7 +164,7 @@ describe("DefaultvNextCollectionService", () => {
       // Arrange dependencies
       await setEncryptedState([collection1, collection2]);
 
-      const result = await firstValueFrom(collectionService.encryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.encryptedCollections$(userId));
 
       expect(result.length).toBe(2);
       expect(result).toIncludeAllPartialMembers([
@@ -156,7 +183,7 @@ describe("DefaultvNextCollectionService", () => {
       await setEncryptedState(null);
 
       const decryptedCollections = await firstValueFrom(
-        collectionService.encryptedCollections$(of(userId)),
+        collectionService.encryptedCollections$(userId),
       );
       expect(decryptedCollections.length).toBe(0);
     });
@@ -176,7 +203,7 @@ describe("DefaultvNextCollectionService", () => {
 
       await collectionService.upsert([updatedCollection1, newCollection3], userId);
 
-      const result = await firstValueFrom(collectionService.encryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.encryptedCollections$(userId));
       expect(result.length).toBe(3);
       expect(result).toIncludeAllPartialMembers([
         {
@@ -201,7 +228,7 @@ describe("DefaultvNextCollectionService", () => {
 
       await collectionService.upsert(collection1, userId);
 
-      const result = await firstValueFrom(collectionService.encryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.encryptedCollections$(userId));
       expect(result.length).toBe(1);
       expect(result).toIncludeAllPartialMembers([
         {
@@ -224,7 +251,7 @@ describe("DefaultvNextCollectionService", () => {
         userId,
       );
 
-      const result = await firstValueFrom(collectionService.encryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.encryptedCollections$(userId));
       expect(result.length).toBe(1);
       expect(result).toIncludeAllPartialMembers([
         {
@@ -241,15 +268,11 @@ describe("DefaultvNextCollectionService", () => {
     await collectionService.clearDecryptedState(userId);
 
     // Encrypted state remains
-    const encryptedState = await firstValueFrom(
+    const encryptedState = await firstValueFrom(collectionService.encryptedCollections$(userId));
-      collectionService.encryptedCollections$(of(userId)),
-    );
     expect(encryptedState.length).toEqual(2);
 
     // Decrypted state is cleared
-    const decryptedState = await firstValueFrom(
+    const decryptedState = await firstValueFrom(collectionService.decryptedCollections$(userId));
-      collectionService.decryptedCollections$(of(userId)),
-    );
     expect(decryptedState.length).toEqual(0);
   });
 
@@ -260,15 +283,11 @@ describe("DefaultvNextCollectionService", () => {
     await collectionService.clear(userId);
 
     // Encrypted state is cleared
-    const encryptedState = await firstValueFrom(
+    const encryptedState = await firstValueFrom(collectionService.encryptedCollections$(userId));
-      collectionService.encryptedCollections$(of(userId)),
-    );
     expect(encryptedState.length).toEqual(0);
 
     // Decrypted state is cleared
-    const decryptedState = await firstValueFrom(
+    const decryptedState = await firstValueFrom(collectionService.decryptedCollections$(userId));
-      collectionService.decryptedCollections$(of(userId)),
-    );
     expect(decryptedState.length).toEqual(0);
   });
 
@@ -280,7 +299,7 @@ describe("DefaultvNextCollectionService", () => {
 
       await collectionService.delete(collection1.id, userId);
 
-      const result = await firstValueFrom(collectionService.encryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.encryptedCollections$(userId));
       expect(result.length).toEqual(1);
       expect(result[0]).toMatchObject({ id: collection2.id });
     });
@@ -293,7 +312,7 @@ describe("DefaultvNextCollectionService", () => {
 
       await collectionService.delete([collection1.id, collection3.id], userId);
 
-      const result = await firstValueFrom(collectionService.encryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.encryptedCollections$(userId));
       expect(result.length).toEqual(1);
       expect(result[0]).toMatchObject({ id: collection2.id });
     });
@@ -304,7 +323,7 @@ describe("DefaultvNextCollectionService", () => {
 
       await collectionService.delete(collection1.id, userId);
 
-      const result = await firstValueFrom(collectionService.encryptedCollections$(of(userId)));
+      const result = await firstValueFrom(collectionService.encryptedCollections$(userId));
       expect(result.length).toEqual(0);
     });
   });

libs/admin-console/src/common/collections/services/default-vnext-collection.service.ts

@@ -1,6 +1,6 @@
 // FIXME: Update this file to be type safe and remove this and next line
 // @ts-strict-ignore
-import { combineLatest, firstValueFrom, map, Observable, of, switchMap } from "rxjs";
+import { combineLatest, filter, firstValueFrom, map } from "rxjs";
 
 import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
@@ -30,9 +30,8 @@ export class DefaultvNextCollectionService implements vNextCollectionService {
     protected stateProvider: StateProvider,
   ) {}
 
-  encryptedCollections$(userId$: Observable<UserId>) {
+  encryptedCollections$(userId: UserId) {
-    return userId$.pipe(
+    return this.encryptedState(userId).state$.pipe(
-      switchMap((userId) => this.encryptedState(userId).state$),
       map((collections) => {
         if (collections == null) {
           return [];
@@ -43,11 +42,8 @@ export class DefaultvNextCollectionService implements vNextCollectionService {
     );
   }
 
-  decryptedCollections$(userId$: Observable<UserId>) {
+  decryptedCollections$(userId: UserId) {
-    return userId$.pipe(
+    return this.decryptedState(userId).state$.pipe(map((collections) => collections ?? []));
-      switchMap((userId) => this.decryptedState(userId).state$),
-      map((collections) => collections ?? []),
-    );
   }
 
   async upsert(toUpdate: CollectionData | CollectionData[], userId: UserId): Promise<void> {
@@ -78,14 +74,14 @@ export class DefaultvNextCollectionService implements vNextCollectionService {
       throw new Error("User ID is required.");
     }
 
-    await this.decryptedState(userId).forceValue(null);
+    await this.decryptedState(userId).forceValue([]);
   }
 
   async clear(userId: UserId): Promise<void> {
     await this.encryptedState(userId).update(() => null);
     // This will propagate from the encrypted state update, but by doing it explicitly
     // the promise doesn't resolve until the update is complete.
-    await this.decryptedState(userId).forceValue(null);
+    await this.decryptedState(userId).forceValue([]);
   }
 
   async delete(id: CollectionId | CollectionId[], userId: UserId): Promise<any> {
@@ -125,7 +121,7 @@ export class DefaultvNextCollectionService implements vNextCollectionService {
   // See https://bitwarden.atlassian.net/browse/PM-12375
   async decryptMany(
     collections: Collection[],
-    orgKeys?: Record<OrganizationId, OrgKey>,
+    orgKeys?: Record<OrganizationId, OrgKey> | null,
   ): Promise<CollectionView[]> {
     if (collections == null || collections.length === 0) {
       return [];
@@ -153,7 +149,7 @@ export class DefaultvNextCollectionService implements vNextCollectionService {
       collectionCopy.id = c.id;
       collectionCopy.organizationId = c.organizationId;
       const parts = c.name != null ? c.name.replace(/^\/+|\/+$/g, "").split(NestingDelimiter) : [];
-      ServiceUtils.nestedTraverse(nodes, 0, parts, collectionCopy, null, NestingDelimiter);
+      ServiceUtils.nestedTraverse(nodes, 0, parts, collectionCopy, undefined, NestingDelimiter);
     });
     return nodes;
   }
@@ -181,14 +177,14 @@ export class DefaultvNextCollectionService implements vNextCollectionService {
    * @returns a SingleUserState for decrypted collection data.
    */
   private decryptedState(userId: UserId): DerivedState<CollectionView[]> {
-    const encryptedCollectionsWithKeys = this.encryptedState(userId).combinedState$.pipe(
+    const encryptedCollectionsWithKeys$ = combineLatest([
-      switchMap(([userId, collectionData]) =>
+      this.encryptedCollections$(userId),
-        combineLatest([of(collectionData), this.keyService.orgKeys$(userId)]),
+      // orgKeys$ can emit null during brief moments on unlock and lock/logout, we want to ignore those intermediate states
-      ),
+      this.keyService.orgKeys$(userId).pipe(filter((orgKeys) => orgKeys != null)),
-    );
+    ]);
 
     return this.stateProvider.getDerived(
-      encryptedCollectionsWithKeys,
+      encryptedCollectionsWithKeys$,
       DECRYPTED_COLLECTION_DATA_KEY,
       {
         collectionService: this,

libs/admin-console/src/common/collections/services/vnext-collection.state.ts

@@ -21,7 +21,7 @@ export const ENCRYPTED_COLLECTION_DATA_KEY = UserKeyDefinition.record<Collection
 );
 
 export const DECRYPTED_COLLECTION_DATA_KEY = new DeriveDefinition<
-  [Record<CollectionId, CollectionData>, Record<OrganizationId, OrgKey>],
+  [Collection[], Record<OrganizationId, OrgKey> | null],
   CollectionView[],
   { collectionService: vNextCollectionService }
 >(COLLECTION_DATA, "decryptedCollections", {
@@ -31,7 +31,6 @@ export const DECRYPTED_COLLECTION_DATA_KEY = new DeriveDefinition<
       return [];
     }
 
-    const data = Object.values(collections).map((c) => new Collection(c));
+    return await collectionService.decryptMany(collections, orgKeys);
-    return await collectionService.decryptMany(data, orgKeys);
   },
 });