diff --git a/libs/admin-console/src/common/collections/abstractions/vnext-collection.service.ts b/libs/admin-console/src/common/collections/abstractions/vnext-collection.service.ts index 53098cdcc27..e1b2a5759a1 100644 --- a/libs/admin-console/src/common/collections/abstractions/vnext-collection.service.ts +++ b/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) => Observable; - decryptedCollections$: (userId$: Observable) => Observable; + encryptedCollections$: (userId: UserId) => Observable; + decryptedCollections$: (userId: UserId) => Observable; upsert: (collection: CollectionData | CollectionData[], userId: UserId) => Promise; replace: (collections: { [id: string]: CollectionData }, userId: UserId) => Promise; /** @@ -22,7 +22,7 @@ export abstract class vNextCollectionService { * Clear decrypted and encrypted state. * Used for logging out. */ - clear: (userId: string) => Promise; + clear: (userId: UserId) => Promise; delete: (id: string | string[], userId: UserId) => Promise; encrypt: (model: CollectionView) => Promise; /** @@ -30,7 +30,7 @@ export abstract class vNextCollectionService { */ decryptMany: ( collections: Collection[], - orgKeys?: Record, + orgKeys?: Record | null, ) => Promise; /** * Transforms the input CollectionViews into TreeNodes diff --git a/libs/admin-console/src/common/collections/services/default-vnext-collection.service.spec.ts b/libs/admin-console/src/common/collections/services/default-vnext-collection.service.spec.ts index 54c4470d414..4aa54429aad 100644 --- a/libs/admin-console/src/common/collections/services/default-vnext-collection.service.spec.ts +++ b/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( - collectionService.encryptedCollections$(of(userId)), - ); + const encryptedState = await firstValueFrom(collectionService.encryptedCollections$(userId)); expect(encryptedState.length).toEqual(2); // Decrypted state is cleared - const decryptedState = await firstValueFrom( - collectionService.decryptedCollections$(of(userId)), - ); + const decryptedState = await firstValueFrom(collectionService.decryptedCollections$(userId)); expect(decryptedState.length).toEqual(0); }); @@ -260,15 +283,11 @@ describe("DefaultvNextCollectionService", () => { await collectionService.clear(userId); // Encrypted state is cleared - const encryptedState = await firstValueFrom( - collectionService.encryptedCollections$(of(userId)), - ); + const encryptedState = await firstValueFrom(collectionService.encryptedCollections$(userId)); expect(encryptedState.length).toEqual(0); // Decrypted state is cleared - const decryptedState = await firstValueFrom( - collectionService.decryptedCollections$(of(userId)), - ); + const decryptedState = await firstValueFrom(collectionService.decryptedCollections$(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); }); }); diff --git a/libs/admin-console/src/common/collections/services/default-vnext-collection.service.ts b/libs/admin-console/src/common/collections/services/default-vnext-collection.service.ts index 5f2985b8400..2d5a083592b 100644 --- a/libs/admin-console/src/common/collections/services/default-vnext-collection.service.ts +++ b/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) { - return userId$.pipe( - switchMap((userId) => this.encryptedState(userId).state$), + encryptedCollections$(userId: UserId) { + return this.encryptedState(userId).state$.pipe( map((collections) => { if (collections == null) { return []; @@ -43,11 +42,8 @@ export class DefaultvNextCollectionService implements vNextCollectionService { ); } - decryptedCollections$(userId$: Observable) { - return userId$.pipe( - switchMap((userId) => this.decryptedState(userId).state$), - map((collections) => collections ?? []), - ); + decryptedCollections$(userId: UserId) { + return this.decryptedState(userId).state$.pipe(map((collections) => collections ?? [])); } async upsert(toUpdate: CollectionData | CollectionData[], userId: UserId): Promise { @@ -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 { 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 { @@ -125,7 +121,7 @@ export class DefaultvNextCollectionService implements vNextCollectionService { // See https://bitwarden.atlassian.net/browse/PM-12375 async decryptMany( collections: Collection[], - orgKeys?: Record, + orgKeys?: Record | null, ): Promise { 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 { - const encryptedCollectionsWithKeys = this.encryptedState(userId).combinedState$.pipe( - switchMap(([userId, collectionData]) => - combineLatest([of(collectionData), this.keyService.orgKeys$(userId)]), - ), - ); + const encryptedCollectionsWithKeys$ = combineLatest([ + this.encryptedCollections$(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, diff --git a/libs/admin-console/src/common/collections/services/vnext-collection.state.ts b/libs/admin-console/src/common/collections/services/vnext-collection.state.ts index 533308f3cc7..331c80436f7 100644 --- a/libs/admin-console/src/common/collections/services/vnext-collection.state.ts +++ b/libs/admin-console/src/common/collections/services/vnext-collection.state.ts @@ -21,7 +21,7 @@ export const ENCRYPTED_COLLECTION_DATA_KEY = UserKeyDefinition.record, Record], + [Collection[], Record | 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(data, orgKeys); + return await collectionService.decryptMany(collections, orgKeys); }, });