From 4c4c52487ee5f2e1d8318ffad7d60dbf8bc68ae0 Mon Sep 17 00:00:00 2001 From: Nik Gilmore Date: Wed, 21 Jan 2026 17:47:03 -0800 Subject: [PATCH] Address PR feedback --- .../bulk-delete-dialog.component.ts | 10 +- .../src/vault/services/cipher.service.spec.ts | 104 ++++++------------ .../src/vault/services/cipher.service.ts | 39 +++---- 3 files changed, 53 insertions(+), 100 deletions(-) diff --git a/apps/web/src/app/vault/individual-vault/bulk-action-dialogs/bulk-delete-dialog/bulk-delete-dialog.component.ts b/apps/web/src/app/vault/individual-vault/bulk-action-dialogs/bulk-delete-dialog/bulk-delete-dialog.component.ts index b7796102f29..aed163dd71e 100644 --- a/apps/web/src/app/vault/individual-vault/bulk-action-dialogs/bulk-delete-dialog/bulk-delete-dialog.component.ts +++ b/apps/web/src/app/vault/individual-vault/bulk-action-dialogs/bulk-delete-dialog/bulk-delete-dialog.component.ts @@ -146,17 +146,13 @@ export class BulkDeleteDialogComponent { } private async deleteCiphersAdmin(ciphers: string[]): Promise { + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); if (this.permanent) { - await this.cipherService.deleteManyWithServer( - ciphers, - await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)), - true, - this.organization.id, - ); + await this.cipherService.deleteManyWithServer(ciphers, userId, true, this.organization.id); } else { await this.cipherService.softDeleteManyWithServer( ciphers, - await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)), + userId, true, this.organization.id, ); diff --git a/libs/common/src/vault/services/cipher.service.spec.ts b/libs/common/src/vault/services/cipher.service.spec.ts index 838e48aa366..e2fd3792e6d 100644 --- a/libs/common/src/vault/services/cipher.service.spec.ts +++ b/libs/common/src/vault/services/cipher.service.spec.ts @@ -117,6 +117,8 @@ describe("Cipher Service", () => { let cipherService: CipherService; let encryptionContext: EncryptionContext; + // BehaviorSubject for SDK feature flag - allows tests to change the value after service instantiation + let sdkCrudFeatureFlag$: BehaviorSubject; beforeEach(() => { encryptService.encryptFileData.mockReturnValue(Promise.resolve(ENCRYPTED_BYTES)); @@ -132,6 +134,10 @@ describe("Cipher Service", () => { (window as any).bitwardenContainerService = new ContainerService(keyService, encryptService); + // Create BehaviorSubject for SDK feature flag - tests can update this to change behavior + sdkCrudFeatureFlag$ = new BehaviorSubject(false); + configService.getFeatureFlag$.mockReturnValue(sdkCrudFeatureFlag$.asObservable()); + cipherService = new CipherService( keyService, domainSettingsService, @@ -218,9 +224,6 @@ describe("Cipher Service", () => { }); it("should call apiService.postCipherAdmin when orgAdmin param is true and the cipher orgId != null", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); const spy = jest .spyOn(apiService, "postCipherAdmin") .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); @@ -233,9 +236,6 @@ describe("Cipher Service", () => { }); it("should call apiService.postCipher when orgAdmin param is true and the cipher orgId is null", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); encryptionContext.cipher.organizationId = null!; const spy = jest .spyOn(apiService, "postCipher") @@ -249,9 +249,6 @@ describe("Cipher Service", () => { }); it("should call apiService.postCipherCreate if collectionsIds != null", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); encryptionContext.cipher.collectionIds = ["123"]; const spy = jest .spyOn(apiService, "postCipherCreate") @@ -265,9 +262,6 @@ describe("Cipher Service", () => { }); it("should call apiService.postCipher when orgAdmin and collectionIds logic is false", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); const spy = jest .spyOn(apiService, "postCipher") .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); @@ -280,9 +274,7 @@ describe("Cipher Service", () => { }); it("should delegate to cipherSdkService when feature flag is enabled", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const cipherView = new CipherView(encryptionContext.cipher); const expectedResult = new CipherView(encryptionContext.cipher); @@ -315,9 +307,9 @@ describe("Cipher Service", () => { }); it("should call apiService.putCipherAdmin when orgAdmin param is true", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const testCipher = new Cipher(cipherData); testCipher.organizationId = orgId; @@ -336,9 +328,6 @@ describe("Cipher Service", () => { }); it("should call apiService.putCipher if cipher.edit is true", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); encryptionContext.cipher.edit = true; const spy = jest .spyOn(apiService, "putCipher") @@ -352,9 +341,6 @@ describe("Cipher Service", () => { }); it("should call apiService.putPartialCipher when orgAdmin, and edit are false", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); encryptionContext.cipher.edit = false; const spy = jest .spyOn(apiService, "putPartialCipher") @@ -368,9 +354,7 @@ describe("Cipher Service", () => { }); it("should delegate to cipherSdkService when feature flag is enabled", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const testCipher = new Cipher(cipherData); const cipherView = new CipherView(testCipher); @@ -392,9 +376,7 @@ describe("Cipher Service", () => { }); it("should delegate to cipherSdkService with orgAdmin when feature flag is enabled", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const testCipher = new Cipher(cipherData); const cipherView = new CipherView(testCipher); @@ -1013,9 +995,9 @@ describe("Cipher Service", () => { const testCipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; it("should call apiService.deleteCipher when feature flag is disabled", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest.spyOn(apiService, "deleteCipher").mockResolvedValue(undefined); @@ -1025,9 +1007,9 @@ describe("Cipher Service", () => { }); it("should call apiService.deleteCipherAdmin when feature flag is disabled and asAdmin is true", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest.spyOn(apiService, "deleteCipherAdmin").mockResolvedValue(undefined); @@ -1037,9 +1019,7 @@ describe("Cipher Service", () => { }); it("should use SDK to delete cipher when feature flag is enabled", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "deleteWithServer") @@ -1053,9 +1033,7 @@ describe("Cipher Service", () => { }); it("should use SDK admin delete when feature flag is enabled and asAdmin is true", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "deleteWithServer") @@ -1076,9 +1054,9 @@ describe("Cipher Service", () => { ]; it("should call apiService.deleteManyCiphers when feature flag is disabled", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest.spyOn(apiService, "deleteManyCiphers").mockResolvedValue(undefined); @@ -1088,9 +1066,9 @@ describe("Cipher Service", () => { }); it("should call apiService.deleteManyCiphersAdmin when feature flag is disabled and asAdmin is true", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest.spyOn(apiService, "deleteManyCiphersAdmin").mockResolvedValue(undefined); @@ -1100,9 +1078,7 @@ describe("Cipher Service", () => { }); it("should use SDK to delete multiple ciphers when feature flag is enabled", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "deleteManyWithServer") @@ -1116,9 +1092,7 @@ describe("Cipher Service", () => { }); it("should use SDK admin delete many when feature flag is enabled and asAdmin is true", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "deleteManyWithServer") @@ -1136,9 +1110,9 @@ describe("Cipher Service", () => { const testCipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; it("should call apiService.putDeleteCipher when feature flag is disabled", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest.spyOn(apiService, "putDeleteCipher").mockResolvedValue(undefined); @@ -1148,9 +1122,9 @@ describe("Cipher Service", () => { }); it("should call apiService.putDeleteCipherAdmin when feature flag is disabled and asAdmin is true", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest.spyOn(apiService, "putDeleteCipherAdmin").mockResolvedValue(undefined); @@ -1160,9 +1134,7 @@ describe("Cipher Service", () => { }); it("should use SDK to soft delete cipher when feature flag is enabled", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "softDeleteWithServer") @@ -1176,9 +1148,7 @@ describe("Cipher Service", () => { }); it("should use SDK admin soft delete when feature flag is enabled and asAdmin is true", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "softDeleteWithServer") @@ -1199,9 +1169,9 @@ describe("Cipher Service", () => { ]; it("should call apiService.putDeleteManyCiphers when feature flag is disabled", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest.spyOn(apiService, "putDeleteManyCiphers").mockResolvedValue(undefined); @@ -1211,9 +1181,9 @@ describe("Cipher Service", () => { }); it("should call apiService.putDeleteManyCiphersAdmin when feature flag is disabled and asAdmin is true", async () => { - configService.getFeatureFlag + configService.getFeatureFlag$ .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(false); + .mockReturnValue(of(false)); const apiSpy = jest .spyOn(apiService, "putDeleteManyCiphersAdmin") @@ -1225,9 +1195,7 @@ describe("Cipher Service", () => { }); it("should use SDK to soft delete multiple ciphers when feature flag is enabled", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "softDeleteManyWithServer") @@ -1241,9 +1209,7 @@ describe("Cipher Service", () => { }); it("should use SDK admin soft delete many when feature flag is enabled and asAdmin is true", async () => { - configService.getFeatureFlag - .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) - .mockResolvedValue(true); + sdkCrudFeatureFlag$.next(true); const sdkServiceSpy = jest .spyOn(cipherSdkService, "softDeleteManyWithServer") diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index ab394bc211e..7ba3cc94aa1 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -106,6 +106,13 @@ export class CipherService implements CipherServiceAbstraction { */ private clearCipherViewsForUser$: Subject = new Subject(); + /** + * Observable exposing the feature flag status for using the SDK for cipher CRUD operations. + */ + private readonly sdkCipherCrudEnabled$: Observable = this.configService.getFeatureFlag$( + FeatureFlag.PM27632_SdkCipherCrudOperations, + ); + constructor( private keyService: KeyService, private domainSettingsService: DomainSettingsService, @@ -909,9 +916,7 @@ export class CipherService implements CipherServiceAbstraction { userId: UserId, orgAdmin?: boolean, ): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return ( @@ -970,9 +975,7 @@ export class CipherService implements CipherServiceAbstraction { originalCipherView?: CipherView, orgAdmin?: boolean, ): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return await this.updateWithServerUsingSdk(cipherView, userId, originalCipherView, orgAdmin); @@ -1390,9 +1393,7 @@ export class CipherService implements CipherServiceAbstraction { } async deleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return this.deleteWithServerUsingSdk(id, userId, asAdmin); } @@ -1421,9 +1422,7 @@ export class CipherService implements CipherServiceAbstraction { asAdmin = false, orgId?: OrganizationId, ): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return this.deleteManyWithServerUsingSdk(ids, userId, asAdmin, orgId); } @@ -1606,9 +1605,7 @@ export class CipherService implements CipherServiceAbstraction { } async softDeleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return this.softDeleteWithServerUsingSdk(id, userId, asAdmin); } @@ -1637,9 +1634,7 @@ export class CipherService implements CipherServiceAbstraction { asAdmin = false, orgId?: OrganizationId, ): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return this.softDeleteManyWithServerUsingSdk(ids, userId, asAdmin, orgId); } @@ -1698,9 +1693,7 @@ export class CipherService implements CipherServiceAbstraction { } async restoreWithServer(id: string, userId: UserId, asAdmin = false): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return await this.restoreWithServerUsingSdk(id, userId, asAdmin); } @@ -1729,9 +1722,7 @@ export class CipherService implements CipherServiceAbstraction { * The Org Vault will pass those ids an array as well as the orgId when calling bulkRestore */ async restoreManyWithServer(ids: string[], userId: UserId, orgId?: string): Promise { - const useSdk = await this.configService.getFeatureFlag( - FeatureFlag.PM27632_SdkCipherCrudOperations, - ); + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); if (useSdk) { return await this.restoreManyWithServerUsingSdk(ids, userId, orgId); }