From 87555eaabdb0703010e478f28283479f2611d01f Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Mon, 26 Jan 2026 12:07:31 -0600 Subject: [PATCH 1/9] remove risk insights for premium feature flag (#18446) --- libs/common/src/enums/feature-flag.enum.ts | 2 -- .../cipher-view/cipher-view.component.spec.ts | 21 ------------------- .../src/cipher-view/cipher-view.component.ts | 19 +++-------------- 3 files changed, 3 insertions(+), 39 deletions(-) diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index f761aea1b08..77df258ad3a 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -65,7 +65,6 @@ export enum FeatureFlag { PM22134SdkCipherListView = "pm-22134-sdk-cipher-list-view", PM22136_SdkCipherEncryption = "pm-22136-sdk-cipher-encryption", CipherKeyEncryption = "cipher-key-encryption", - RiskInsightsForPremium = "pm-23904-risk-insights-for-premium", VaultLoadingSkeletons = "pm-25081-vault-skeleton-loaders", BrowserPremiumSpotlight = "pm-23384-browser-premium-spotlight", MigrateMyVaultToMyItems = "pm-20558-migrate-myvault-to-myitems", @@ -129,7 +128,6 @@ export const DefaultFeatureFlagValue = { [FeatureFlag.PM19941MigrateCipherDomainToSdk]: FALSE, [FeatureFlag.PM22134SdkCipherListView]: FALSE, [FeatureFlag.PM22136_SdkCipherEncryption]: FALSE, - [FeatureFlag.RiskInsightsForPremium]: FALSE, [FeatureFlag.VaultLoadingSkeletons]: FALSE, [FeatureFlag.BrowserPremiumSpotlight]: FALSE, [FeatureFlag.MigrateMyVaultToMyItems]: FALSE, diff --git a/libs/vault/src/cipher-view/cipher-view.component.spec.ts b/libs/vault/src/cipher-view/cipher-view.component.spec.ts index 18a5132781b..2300565035e 100644 --- a/libs/vault/src/cipher-view/cipher-view.component.spec.ts +++ b/libs/vault/src/cipher-view/cipher-view.component.spec.ts @@ -8,7 +8,6 @@ import { CollectionService } from "@bitwarden/admin-console/common"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { AccountService, Account } from "@bitwarden/common/auth/abstractions/account.service"; import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions"; -import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -42,11 +41,9 @@ describe("CipherViewComponent", () => { let mockLogService: LogService; let mockCipherRiskService: CipherRiskService; let mockBillingAccountProfileStateService: BillingAccountProfileStateService; - let mockConfigService: ConfigService; // Mock data let mockCipherView: CipherView; - let featureFlagEnabled$: BehaviorSubject; let hasPremiumFromAnySource$: BehaviorSubject; let activeAccount$: BehaviorSubject; @@ -57,7 +54,6 @@ describe("CipherViewComponent", () => { email: "test@example.com", } as Account); - featureFlagEnabled$ = new BehaviorSubject(false); hasPremiumFromAnySource$ = new BehaviorSubject(true); // Create service mocks @@ -83,9 +79,6 @@ describe("CipherViewComponent", () => { .fn() .mockReturnValue(hasPremiumFromAnySource$); - mockConfigService = mock(); - mockConfigService.getFeatureFlag$ = jest.fn().mockReturnValue(featureFlagEnabled$); - // Setup mock cipher view mockCipherView = new CipherView(); mockCipherView.id = "cipher-id"; @@ -110,7 +103,6 @@ describe("CipherViewComponent", () => { provide: BillingAccountProfileStateService, useValue: mockBillingAccountProfileStateService, }, - { provide: ConfigService, useValue: mockConfigService }, ], schemas: [NO_ERRORS_SCHEMA], }) @@ -145,7 +137,6 @@ describe("CipherViewComponent", () => { beforeEach(() => { // Reset observables to default values for this test suite - featureFlagEnabled$.next(true); hasPremiumFromAnySource$.next(true); // Setup default mock for computeCipherRiskForUser (individual tests can override) @@ -162,18 +153,6 @@ describe("CipherViewComponent", () => { component = fixture.componentInstance; }); - it("returns false when feature flag is disabled", fakeAsync(() => { - featureFlagEnabled$.next(false); - - const cipher = createLoginCipherView(); - fixture.componentRef.setInput("cipher", cipher); - fixture.detectChanges(); - tick(); - - expect(mockCipherRiskService.computeCipherRiskForUser).not.toHaveBeenCalled(); - expect(component.passwordIsAtRisk()).toBe(false); - })); - it("returns false when cipher has no login password", fakeAsync(() => { const cipher = createLoginCipherView(); cipher.login = {} as any; // No password diff --git a/libs/vault/src/cipher-view/cipher-view.component.ts b/libs/vault/src/cipher-view/cipher-view.component.ts index b5c063df51e..26e3f18b542 100644 --- a/libs/vault/src/cipher-view/cipher-view.component.ts +++ b/libs/vault/src/cipher-view/cipher-view.component.ts @@ -13,8 +13,6 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { isCardExpired } from "@bitwarden/common/autofill/utils"; import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions"; -import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; -import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { getByIds } from "@bitwarden/common/platform/misc"; @@ -113,7 +111,6 @@ export class CipherViewComponent { private logService: LogService, private cipherRiskService: CipherRiskService, private billingAccountService: BillingAccountProfileStateService, - private configService: ConfigService, ) {} readonly resolvedCollections = toSignal( @@ -248,19 +245,9 @@ export class CipherViewComponent { * The password is only evaluated when the user is premium and has edit access to the cipher. */ readonly passwordIsAtRisk = toSignal( - combineLatest([ - this.activeUserId$, - this.cipher$, - this.configService.getFeatureFlag$(FeatureFlag.RiskInsightsForPremium), - ]).pipe( - switchMap(([userId, cipher, featureEnabled]) => { - if ( - !featureEnabled || - !cipher.hasLoginPassword || - !cipher.edit || - cipher.organizationId || - cipher.isDeleted - ) { + combineLatest([this.activeUserId$, this.cipher$]).pipe( + switchMap(([userId, cipher]) => { + if (!cipher.hasLoginPassword || !cipher.edit || cipher.organizationId || cipher.isDeleted) { return of(false); } return this.switchPremium$( From 06c8c7316d71b1d3a799a29dde55e88ea9ad2d1b Mon Sep 17 00:00:00 2001 From: Nik Gilmore Date: Mon, 26 Jan 2026 11:43:35 -0800 Subject: [PATCH 2/9] [PM-30301][PM-30302] Use SDK for Create and Update cipher operations (#18149) * Migrate create and edit operations to use SDK for ciphers * WIP: Adds admin call to edit ciphers with SDK * Add client version to SDK intialization settings * Remove console.log statements * Adds originalCipherId and collectionIds to updateCipher * Update tests for new cipehrService interfaces * Rename SdkCipherOperations feature flag * Add call to Admin edit SDK if flag is passed * Add tests for SDK path * Revert changes to .npmrc * Remove outdated comments * Fix feature flag name * Fix UUID format in cipher.service.spec.ts * Update calls to cipherService.updateWithServer and .createWithServer to new interface * Update CLI and Desktop to use new cipherSErvice interfaces * Fix tests for new cipherService interface change * Bump sdk-internal and commercial-sdk-internal versions to 0.2.0-main.439 * Fix linting errors * Fix typescript errors impacted by this chnage * Fix caching issue on browser extension when using SDK cipher ops. * Remove commented code * Fix bug causing race condition due to not consuming / awaiting observable. * Add missing 'await' to decrypt call * Clean up unnecessary else statements and fix function naming * Add comments for this.clearCache * Add tests for SDK CipherView conversion functions * Replace sdkservice with cipher-sdk.service * Fix import issues in browser * Fix import issues in cli * Fix type issues * Fix type issues * Fix type issues * Fix test that fails sporadically due to timing issue --- .../notification.background.spec.ts | 19 +- .../background/notification.background.ts | 9 +- .../autofill/popup/fido2/fido2.component.ts | 5 +- .../browser/src/background/main.background.ts | 6 + .../item-more-options.component.ts | 3 +- .../vault-popup-autofill.service.spec.ts | 3 +- .../services/vault-popup-autofill.service.ts | 3 +- apps/cli/src/commands/edit.command.ts | 4 +- .../service-container/service-container.ts | 6 + apps/cli/src/vault/create.command.ts | 9 +- .../desktop-fido2-user-interface.service.ts | 10 +- .../encrypted-message-handler.service.ts | 6 +- .../vault/individual-vault/vault.component.ts | 3 +- .../src/services/jslib-services.module.ts | 10 + libs/common/src/enums/feature-flag.enum.ts | 2 + .../fido2/fido2-authenticator.service.spec.ts | 31 +- .../fido2/fido2-authenticator.service.ts | 6 +- .../services/sdk/default-sdk.service.ts | 7 +- .../services/sdk/register-sdk.service.ts | 7 +- .../vault/abstractions/cipher-sdk.service.ts | 37 ++ .../src/vault/abstractions/cipher.service.ts | 13 +- .../src/vault/models/view/cipher.view.spec.ts | 362 ++++++++++++++++++ .../src/vault/models/view/cipher.view.ts | 86 ++++- .../vault/services/cipher-sdk.service.spec.ts | 246 ++++++++++++ .../src/vault/services/cipher-sdk.service.ts | 82 ++++ .../src/vault/services/cipher.service.spec.ts | 164 +++++++- .../src/vault/services/cipher.service.ts | 75 +++- .../services/default-cipher-form.service.ts | 37 +- 28 files changed, 1126 insertions(+), 125 deletions(-) create mode 100644 libs/common/src/vault/abstractions/cipher-sdk.service.ts create mode 100644 libs/common/src/vault/services/cipher-sdk.service.spec.ts create mode 100644 libs/common/src/vault/services/cipher-sdk.service.ts diff --git a/apps/browser/src/autofill/background/notification.background.spec.ts b/apps/browser/src/autofill/background/notification.background.spec.ts index ab16788ea6f..a927c75dba0 100644 --- a/apps/browser/src/autofill/background/notification.background.spec.ts +++ b/apps/browser/src/autofill/background/notification.background.spec.ts @@ -767,7 +767,6 @@ describe("NotificationBackground", () => { let createWithServerSpy: jest.SpyInstance; let updateWithServerSpy: jest.SpyInstance; let folderExistsSpy: jest.SpyInstance; - let cipherEncryptSpy: jest.SpyInstance; beforeEach(() => { activeAccountStatusMock$.next(AuthenticationStatus.Unlocked); @@ -791,7 +790,6 @@ describe("NotificationBackground", () => { createWithServerSpy = jest.spyOn(cipherService, "createWithServer"); updateWithServerSpy = jest.spyOn(cipherService, "updateWithServer"); folderExistsSpy = jest.spyOn(notificationBackground as any, "folderExists"); - cipherEncryptSpy = jest.spyOn(cipherService, "encrypt"); accountService.activeAccount$ = activeAccountSubject; }); @@ -1190,13 +1188,7 @@ describe("NotificationBackground", () => { folderExistsSpy.mockResolvedValueOnce(false); convertAddLoginQueueMessageToCipherViewSpy.mockReturnValueOnce(cipherView); editItemSpy.mockResolvedValueOnce(undefined); - cipherEncryptSpy.mockResolvedValueOnce({ - cipher: { - ...cipherView, - id: "testId", - }, - encryptedFor: userId, - }); + createWithServerSpy.mockResolvedValueOnce(cipherView); sendMockExtensionMessage(message, sender); await flushPromises(); @@ -1205,7 +1197,6 @@ describe("NotificationBackground", () => { queueMessage, null, ); - expect(cipherEncryptSpy).toHaveBeenCalledWith(cipherView, "testId"); expect(createWithServerSpy).toHaveBeenCalled(); expect(tabSendMessageDataSpy).toHaveBeenCalledWith( sender.tab, @@ -1241,13 +1232,6 @@ describe("NotificationBackground", () => { folderExistsSpy.mockResolvedValueOnce(true); convertAddLoginQueueMessageToCipherViewSpy.mockReturnValueOnce(cipherView); editItemSpy.mockResolvedValueOnce(undefined); - cipherEncryptSpy.mockResolvedValueOnce({ - cipher: { - ...cipherView, - id: "testId", - }, - encryptedFor: userId, - }); const errorMessage = "fetch error"; createWithServerSpy.mockImplementation(() => { throw new Error(errorMessage); @@ -1256,7 +1240,6 @@ describe("NotificationBackground", () => { sendMockExtensionMessage(message, sender); await flushPromises(); - expect(cipherEncryptSpy).toHaveBeenCalledWith(cipherView, "testId"); expect(createWithServerSpy).toThrow(errorMessage); expect(tabSendMessageSpy).not.toHaveBeenCalledWith(sender.tab, { command: "addedCipher", diff --git a/apps/browser/src/autofill/background/notification.background.ts b/apps/browser/src/autofill/background/notification.background.ts index 1cbf915b06a..f8459cf8f23 100644 --- a/apps/browser/src/autofill/background/notification.background.ts +++ b/apps/browser/src/autofill/background/notification.background.ts @@ -866,13 +866,11 @@ export default class NotificationBackground { return; } - const encrypted = await this.cipherService.encrypt(newCipher, activeUserId); - const { cipher } = encrypted; try { - await this.cipherService.createWithServer(encrypted); + const resultCipher = await this.cipherService.createWithServer(newCipher, activeUserId); await BrowserApi.tabSendMessageData(tab, "saveCipherAttemptCompleted", { itemName: newCipher?.name && String(newCipher?.name), - cipherId: cipher?.id && String(cipher?.id), + cipherId: resultCipher?.id && String(resultCipher?.id), }); await BrowserApi.tabSendMessage(tab, { command: "addedCipher" }); } catch (error) { @@ -910,7 +908,6 @@ export default class NotificationBackground { await BrowserApi.tabSendMessage(tab, { command: "editedCipher" }); return; } - const cipher = await this.cipherService.encrypt(cipherView, userId); try { if (!cipherView.edit) { @@ -939,7 +936,7 @@ export default class NotificationBackground { return; } - await this.cipherService.updateWithServer(cipher); + await this.cipherService.updateWithServer(cipherView, userId); await BrowserApi.tabSendMessageData(tab, "saveCipherAttemptCompleted", { itemName: cipherView?.name && String(cipherView?.name), diff --git a/apps/browser/src/autofill/popup/fido2/fido2.component.ts b/apps/browser/src/autofill/popup/fido2/fido2.component.ts index c1982d27d24..5720419f909 100644 --- a/apps/browser/src/autofill/popup/fido2/fido2.component.ts +++ b/apps/browser/src/autofill/popup/fido2/fido2.component.ts @@ -444,10 +444,9 @@ export class Fido2Component implements OnInit, OnDestroy { ); this.buildCipher(name, username); - const encrypted = await this.cipherService.encrypt(this.cipher, activeUserId); try { - await this.cipherService.createWithServer(encrypted); - this.cipher.id = encrypted.cipher.id; + const result = await this.cipherService.createWithServer(this.cipher, activeUserId); + this.cipher.id = result.id; } catch (e) { this.logService.error(e); } diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 58a7eb99ec6..660fcb97bcf 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -194,6 +194,7 @@ import { SendService } from "@bitwarden/common/tools/send/services/send.service" import { InternalSendService as InternalSendServiceAbstraction } from "@bitwarden/common/tools/send/services/send.service.abstraction"; import { UserId } from "@bitwarden/common/types/guid"; import { CipherEncryptionService } from "@bitwarden/common/vault/abstractions/cipher-encryption.service"; +import { CipherSdkService } from "@bitwarden/common/vault/abstractions/cipher-sdk.service"; import { CipherService as CipherServiceAbstraction } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherFileUploadService as CipherFileUploadServiceAbstraction } from "@bitwarden/common/vault/abstractions/file-upload/cipher-file-upload.service"; import { FolderApiServiceAbstraction } from "@bitwarden/common/vault/abstractions/folder/folder-api.service.abstraction"; @@ -211,6 +212,7 @@ import { CipherAuthorizationService, DefaultCipherAuthorizationService, } from "@bitwarden/common/vault/services/cipher-authorization.service"; +import { DefaultCipherSdkService } from "@bitwarden/common/vault/services/cipher-sdk.service"; import { CipherService } from "@bitwarden/common/vault/services/cipher.service"; import { DefaultCipherEncryptionService } from "@bitwarden/common/vault/services/default-cipher-encryption.service"; import { CipherFileUploadService } from "@bitwarden/common/vault/services/file-upload/cipher-file-upload.service"; @@ -367,6 +369,7 @@ export default class MainBackground { apiService: ApiServiceAbstraction; hibpApiService: HibpApiService; environmentService: BrowserEnvironmentService; + cipherSdkService: CipherSdkService; cipherService: CipherServiceAbstraction; folderService: InternalFolderServiceAbstraction; userDecryptionOptionsService: InternalUserDecryptionOptionsServiceAbstraction; @@ -973,6 +976,8 @@ export default class MainBackground { this.logService, ); + this.cipherSdkService = new DefaultCipherSdkService(this.sdkService, this.logService); + this.cipherService = new CipherService( this.keyService, this.domainSettingsService, @@ -988,6 +993,7 @@ export default class MainBackground { this.logService, this.cipherEncryptionService, this.messagingService, + this.cipherSdkService, ); this.folderService = new FolderService( this.keyService, diff --git a/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.ts b/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.ts index f881b07282b..d7de51ad20f 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.ts @@ -277,8 +277,7 @@ export class ItemMoreOptionsComponent { this.accountService.activeAccount$.pipe(map((a) => a?.id)), )) as UserId; - const encryptedCipher = await this.cipherService.encrypt(cipher, activeUserId); - await this.cipherService.updateWithServer(encryptedCipher); + await this.cipherService.updateWithServer(cipher, activeUserId); this.toastService.showToast({ variant: "success", message: this.i18nService.t( diff --git a/apps/browser/src/vault/popup/services/vault-popup-autofill.service.spec.ts b/apps/browser/src/vault/popup/services/vault-popup-autofill.service.spec.ts index 5818c6e32ff..94542009a89 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-autofill.service.spec.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-autofill.service.spec.ts @@ -378,8 +378,7 @@ describe("VaultPopupAutofillService", () => { expect(result).toBe(true); expect(mockCipher.login.uris).toHaveLength(1); expect(mockCipher.login.uris[0].uri).toBe(mockCurrentTab.url); - expect(mockCipherService.encrypt).toHaveBeenCalledWith(mockCipher, mockUserId); - expect(mockCipherService.updateWithServer).toHaveBeenCalledWith(mockEncryptedCipher); + expect(mockCipherService.updateWithServer).toHaveBeenCalledWith(mockCipher, mockUserId); }); it("should add a URI to the cipher when there are no existing URIs", async () => { diff --git a/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts b/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts index 6feeec29efc..025088e029e 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts @@ -426,8 +426,7 @@ export class VaultPopupAutofillService { const activeUserId = await firstValueFrom( this.accountService.activeAccount$.pipe(map((a) => a?.id)), ); - const encCipher = await this.cipherService.encrypt(cipher, activeUserId); - await this.cipherService.updateWithServer(encCipher); + await this.cipherService.updateWithServer(cipher, activeUserId); this.messagingService.send("editedCipher"); return true; } catch { diff --git a/apps/cli/src/commands/edit.command.ts b/apps/cli/src/commands/edit.command.ts index d95e8333dca..dbcb0489187 100644 --- a/apps/cli/src/commands/edit.command.ts +++ b/apps/cli/src/commands/edit.command.ts @@ -138,10 +138,8 @@ export class EditCommand { ); } - const encCipher = await this.cipherService.encrypt(cipherView, activeUserId); try { - const updatedCipher = await this.cipherService.updateWithServer(encCipher); - const decCipher = await this.cipherService.decrypt(updatedCipher, activeUserId); + const decCipher = await this.cipherService.updateWithServer(cipherView, activeUserId); const res = new CipherResponse(decCipher); return Response.success(res); } catch (e) { diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index bc3d3153b13..7bb8da27040 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/apps/cli/src/service-container/service-container.ts @@ -147,11 +147,13 @@ import { SendService } from "@bitwarden/common/tools/send/services/send.service" import { UserId } from "@bitwarden/common/types/guid"; import { CipherArchiveService } from "@bitwarden/common/vault/abstractions/cipher-archive.service"; import { CipherEncryptionService } from "@bitwarden/common/vault/abstractions/cipher-encryption.service"; +import { CipherSdkService } from "@bitwarden/common/vault/abstractions/cipher-sdk.service"; import { InternalFolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { CipherAuthorizationService, DefaultCipherAuthorizationService, } from "@bitwarden/common/vault/services/cipher-authorization.service"; +import { DefaultCipherSdkService } from "@bitwarden/common/vault/services/cipher-sdk.service"; import { CipherService } from "@bitwarden/common/vault/services/cipher.service"; import { DefaultCipherArchiveService } from "@bitwarden/common/vault/services/default-cipher-archive.service"; import { DefaultCipherEncryptionService } from "@bitwarden/common/vault/services/default-cipher-encryption.service"; @@ -254,6 +256,7 @@ export class ServiceContainer { twoFactorApiService: TwoFactorApiService; hibpApiService: HibpApiService; environmentService: EnvironmentService; + cipherSdkService: CipherSdkService; cipherService: CipherService; folderService: InternalFolderService; organizationUserApiService: OrganizationUserApiService; @@ -794,6 +797,8 @@ export class ServiceContainer { this.logService, ); + this.cipherSdkService = new DefaultCipherSdkService(this.sdkService, this.logService); + this.cipherService = new CipherService( this.keyService, this.domainSettingsService, @@ -809,6 +814,7 @@ export class ServiceContainer { this.logService, this.cipherEncryptionService, this.messagingService, + this.cipherSdkService, ); this.cipherArchiveService = new DefaultCipherArchiveService( diff --git a/apps/cli/src/vault/create.command.ts b/apps/cli/src/vault/create.command.ts index d826766dc65..e1a91966afd 100644 --- a/apps/cli/src/vault/create.command.ts +++ b/apps/cli/src/vault/create.command.ts @@ -103,10 +103,11 @@ export class CreateCommand { return Response.error("Creating this item type is restricted by organizational policy."); } - const cipher = await this.cipherService.encrypt(CipherExport.toView(req), activeUserId); - const newCipher = await this.cipherService.createWithServer(cipher); - const decCipher = await this.cipherService.decrypt(newCipher, activeUserId); - const res = new CipherResponse(decCipher); + const newCipher = await this.cipherService.createWithServer( + CipherExport.toView(req), + activeUserId, + ); + const res = new CipherResponse(newCipher); return Response.success(res); } catch (e) { return Response.error(e); diff --git a/apps/desktop/src/autofill/services/desktop-fido2-user-interface.service.ts b/apps/desktop/src/autofill/services/desktop-fido2-user-interface.service.ts index cf29370840d..432448faba3 100644 --- a/apps/desktop/src/autofill/services/desktop-fido2-user-interface.service.ts +++ b/apps/desktop/src/autofill/services/desktop-fido2-user-interface.service.ts @@ -299,12 +299,11 @@ export class DesktopFido2UserInterfaceSession implements Fido2UserInterfaceSessi throw new Error("No active user ID found!"); } - const encCipher = await this.cipherService.encrypt(cipher, activeUserId); - try { - const createdCipher = await this.cipherService.createWithServer(encCipher); + const createdCipher = await this.cipherService.createWithServer(cipher, activeUserId); + const encryptedCreatedCipher = await this.cipherService.encrypt(createdCipher, activeUserId); - return createdCipher; + return encryptedCreatedCipher.cipher; } catch { throw new Error("Unable to create cipher"); } @@ -316,8 +315,7 @@ export class DesktopFido2UserInterfaceSession implements Fido2UserInterfaceSessi this.accountService.activeAccount$.pipe( map(async (a) => { if (a) { - const encCipher = await this.cipherService.encrypt(cipher, a.id); - await this.cipherService.updateWithServer(encCipher); + await this.cipherService.updateWithServer(cipher, a.id); } }), ), diff --git a/apps/desktop/src/services/encrypted-message-handler.service.ts b/apps/desktop/src/services/encrypted-message-handler.service.ts index 366a144c021..ccbc7c539d0 100644 --- a/apps/desktop/src/services/encrypted-message-handler.service.ts +++ b/apps/desktop/src/services/encrypted-message-handler.service.ts @@ -166,8 +166,7 @@ export class EncryptedMessageHandlerService { try { const activeUserId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); - const encrypted = await this.cipherService.encrypt(cipherView, activeUserId); - await this.cipherService.createWithServer(encrypted); + await this.cipherService.createWithServer(cipherView, activeUserId); // Notify other clients of new login await this.messagingService.send("addedCipher"); @@ -212,9 +211,8 @@ export class EncryptedMessageHandlerService { cipherView.login.password = credentialUpdatePayload.password; cipherView.login.username = credentialUpdatePayload.userName; cipherView.login.uris[0].uri = credentialUpdatePayload.uri; - const encrypted = await this.cipherService.encrypt(cipherView, activeUserId); - await this.cipherService.updateWithServer(encrypted); + await this.cipherService.updateWithServer(cipherView, activeUserId); // Notify other clients of update await this.messagingService.send("editedCipher"); diff --git a/apps/web/src/app/vault/individual-vault/vault.component.ts b/apps/web/src/app/vault/individual-vault/vault.component.ts index 5ca3a11d5ab..532757852a3 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -1536,8 +1536,7 @@ export class VaultComponent implements OnInit, OnDestr const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); const cipherFullView = await this.cipherService.getFullCipherView(cipher); cipherFullView.favorite = !cipherFullView.favorite; - const encryptedCipher = await this.cipherService.encrypt(cipherFullView, activeUserId); - await this.cipherService.updateWithServer(encryptedCipher); + await this.cipherService.updateWithServer(cipherFullView, activeUserId); this.toastService.showToast({ variant: "success", diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 7b504548ff5..1ecf7fe3e3d 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -303,6 +303,7 @@ import { import { CipherArchiveService } from "@bitwarden/common/vault/abstractions/cipher-archive.service"; import { CipherEncryptionService } from "@bitwarden/common/vault/abstractions/cipher-encryption.service"; import { CipherRiskService } from "@bitwarden/common/vault/abstractions/cipher-risk.service"; +import { CipherSdkService } from "@bitwarden/common/vault/abstractions/cipher-sdk.service"; import { CipherService as CipherServiceAbstraction } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherFileUploadService as CipherFileUploadServiceAbstraction } from "@bitwarden/common/vault/abstractions/file-upload/cipher-file-upload.service"; import { FolderApiServiceAbstraction } from "@bitwarden/common/vault/abstractions/folder/folder-api.service.abstraction"; @@ -321,6 +322,7 @@ import { CipherAuthorizationService, DefaultCipherAuthorizationService, } from "@bitwarden/common/vault/services/cipher-authorization.service"; +import { DefaultCipherSdkService } from "@bitwarden/common/vault/services/cipher-sdk.service"; import { CipherService } from "@bitwarden/common/vault/services/cipher.service"; import { DefaultCipherArchiveService } from "@bitwarden/common/vault/services/default-cipher-archive.service"; import { DefaultCipherEncryptionService } from "@bitwarden/common/vault/services/default-cipher-encryption.service"; @@ -590,6 +592,11 @@ const safeProviders: SafeProvider[] = [ useClass: DefaultDomainSettingsService, deps: [StateProvider, PolicyServiceAbstraction, AccountService], }), + safeProvider({ + provide: CipherSdkService, + useClass: DefaultCipherSdkService, + deps: [SdkService, LogService], + }), safeProvider({ provide: CipherServiceAbstraction, useFactory: ( @@ -607,6 +614,7 @@ const safeProviders: SafeProvider[] = [ logService: LogService, cipherEncryptionService: CipherEncryptionService, messagingService: MessagingServiceAbstraction, + cipherSdkService: CipherSdkService, ) => new CipherService( keyService, @@ -623,6 +631,7 @@ const safeProviders: SafeProvider[] = [ logService, cipherEncryptionService, messagingService, + cipherSdkService, ), deps: [ KeyService, @@ -639,6 +648,7 @@ const safeProviders: SafeProvider[] = [ LogService, CipherEncryptionService, MessagingServiceAbstraction, + CipherSdkService, ], }), safeProvider({ diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index 77df258ad3a..94656d48826 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -68,6 +68,7 @@ export enum FeatureFlag { VaultLoadingSkeletons = "pm-25081-vault-skeleton-loaders", BrowserPremiumSpotlight = "pm-23384-browser-premium-spotlight", MigrateMyVaultToMyItems = "pm-20558-migrate-myvault-to-myitems", + PM27632_SdkCipherCrudOperations = "pm-27632-cipher-crud-operations-to-sdk", /* Platform */ IpcChannelFramework = "ipc-channel-framework", @@ -130,6 +131,7 @@ export const DefaultFeatureFlagValue = { [FeatureFlag.PM22136_SdkCipherEncryption]: FALSE, [FeatureFlag.VaultLoadingSkeletons]: FALSE, [FeatureFlag.BrowserPremiumSpotlight]: FALSE, + [FeatureFlag.PM27632_SdkCipherCrudOperations]: FALSE, [FeatureFlag.MigrateMyVaultToMyItems]: FALSE, /* Auth */ diff --git a/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts b/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts index 9c50bd1ab65..6223e4274bf 100644 --- a/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts +++ b/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts @@ -254,17 +254,17 @@ describe("FidoAuthenticatorService", () => { } it("should save credential to vault if request confirmed by user", async () => { - const encryptedCipher = Symbol(); userInterfaceSession.confirmNewCredential.mockResolvedValue({ cipherId: existingCipher.id, userVerified: false, }); - cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as EncryptionContext); await authenticator.makeCredential(params, windowReference); - const saved = cipherService.encrypt.mock.lastCall?.[0]; - expect(saved).toEqual( + const savedCipher = cipherService.updateWithServer.mock.lastCall?.[0]; + const actualUserId = cipherService.updateWithServer.mock.lastCall?.[1]; + expect(actualUserId).toEqual(userId); + expect(savedCipher).toEqual( expect.objectContaining({ type: CipherType.Login, name: existingCipher.name, @@ -288,7 +288,6 @@ describe("FidoAuthenticatorService", () => { }), }), ); - expect(cipherService.updateWithServer).toHaveBeenCalledWith(encryptedCipher); }); /** Spec: If the user does not consent or if user verification fails, return an error code equivalent to "NotAllowedError" and terminate the operation. */ @@ -361,17 +360,14 @@ describe("FidoAuthenticatorService", () => { cipherService.getAllDecrypted.mockResolvedValue([await cipher]); cipherService.decrypt.mockResolvedValue(cipher); - cipherService.encrypt.mockImplementation(async (cipher) => { - cipher.login.fido2Credentials[0].credentialId = credentialId; // Replace id for testability - return { cipher: {} as any as Cipher, encryptedFor: userId }; - }); - cipherService.createWithServer.mockImplementation(async ({ cipher }) => { - cipher.id = cipherId; + cipherService.createWithServer.mockImplementation(async (cipherView, _userId) => { + cipherView.id = cipherId; return cipher; }); - cipherService.updateWithServer.mockImplementation(async ({ cipher }) => { - cipher.id = cipherId; - return cipher; + cipherService.updateWithServer.mockImplementation(async (cipherView, _userId) => { + cipherView.id = cipherId; + cipherView.login.fido2Credentials[0].credentialId = credentialId; // Replace id for testability + return cipherView; }); }); @@ -701,14 +697,11 @@ describe("FidoAuthenticatorService", () => { /** Spec: Increment the credential associated signature counter */ it("should increment counter and save to server when stored counter is larger than zero", async () => { - const encrypted = Symbol(); - cipherService.encrypt.mockResolvedValue(encrypted as any); ciphers[0].login.fido2Credentials[0].counter = 9000; await authenticator.getAssertion(params, windowReference); - expect(cipherService.updateWithServer).toHaveBeenCalledWith(encrypted); - expect(cipherService.encrypt).toHaveBeenCalledWith( + expect(cipherService.updateWithServer).toHaveBeenCalledWith( expect.objectContaining({ id: ciphers[0].id, login: expect.objectContaining({ @@ -725,8 +718,6 @@ describe("FidoAuthenticatorService", () => { /** Spec: Authenticators that do not implement a signature counter leave the signCount in the authenticator data constant at zero. */ it("should not save to server when stored counter is zero", async () => { - const encrypted = Symbol(); - cipherService.encrypt.mockResolvedValue(encrypted as any); ciphers[0].login.fido2Credentials[0].counter = 0; await authenticator.getAssertion(params, windowReference); diff --git a/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts b/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts index d1081e9f7b2..1b150207290 100644 --- a/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts +++ b/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts @@ -187,8 +187,7 @@ export class Fido2AuthenticatorService< if (Utils.isNullOrEmpty(cipher.login.username)) { cipher.login.username = fido2Credential.userName; } - const reencrypted = await this.cipherService.encrypt(cipher, activeUserId); - await this.cipherService.updateWithServer(reencrypted); + await this.cipherService.updateWithServer(cipher, activeUserId); await this.cipherService.clearCache(activeUserId); credentialId = fido2Credential.credentialId; } catch (error) { @@ -328,8 +327,7 @@ export class Fido2AuthenticatorService< const activeUserId = await firstValueFrom( this.accountService.activeAccount$.pipe(getUserId), ); - const encrypted = await this.cipherService.encrypt(selectedCipher, activeUserId); - await this.cipherService.updateWithServer(encrypted); + await this.cipherService.updateWithServer(selectedCipher, activeUserId); await this.cipherService.clearCache(activeUserId); } diff --git a/libs/common/src/platform/services/sdk/default-sdk.service.ts b/libs/common/src/platform/services/sdk/default-sdk.service.ts index 5084f5f5f18..e2c9c77e204 100644 --- a/libs/common/src/platform/services/sdk/default-sdk.service.ts +++ b/libs/common/src/platform/services/sdk/default-sdk.service.ts @@ -80,7 +80,7 @@ export class DefaultSdkService implements SdkService { client$ = this.environmentService.environment$.pipe( concatMap(async (env) => { await SdkLoadService.Ready; - const settings = this.toSettings(env); + const settings = await this.toSettings(env); const client = await this.sdkClientFactory.createSdkClient( new JsTokenProvider(this.apiService), settings, @@ -210,7 +210,7 @@ export class DefaultSdkService implements SdkService { return undefined; } - const settings = this.toSettings(env); + const settings = await this.toSettings(env); const client = await this.sdkClientFactory.createSdkClient( new JsTokenProvider(this.apiService, userId), settings, @@ -322,11 +322,12 @@ export class DefaultSdkService implements SdkService { client.platform().load_flags(featureFlagMap); } - private toSettings(env: Environment): ClientSettings { + private async toSettings(env: Environment): Promise { return { apiUrl: env.getApiUrl(), identityUrl: env.getIdentityUrl(), deviceType: toSdkDevice(this.platformUtilsService.getDevice()), + bitwardenClientVersion: await this.platformUtilsService.getApplicationVersionNumber(), userAgent: this.userAgent ?? navigator.userAgent, }; } diff --git a/libs/common/src/platform/services/sdk/register-sdk.service.ts b/libs/common/src/platform/services/sdk/register-sdk.service.ts index a222807640f..073c5c0560c 100644 --- a/libs/common/src/platform/services/sdk/register-sdk.service.ts +++ b/libs/common/src/platform/services/sdk/register-sdk.service.ts @@ -62,7 +62,7 @@ export class DefaultRegisterSdkService implements RegisterSdkService { client$ = this.environmentService.environment$.pipe( concatMap(async (env) => { await SdkLoadService.Ready; - const settings = this.toSettings(env); + const settings = await this.toSettings(env); const client = await this.sdkClientFactory.createSdkClient( new JsTokenProvider(this.apiService), settings, @@ -137,7 +137,7 @@ export class DefaultRegisterSdkService implements RegisterSdkService { return undefined; } - const settings = this.toSettings(env); + const settings = await this.toSettings(env); const client = await this.sdkClientFactory.createSdkClient( new JsTokenProvider(this.apiService, userId), settings, @@ -185,12 +185,13 @@ export class DefaultRegisterSdkService implements RegisterSdkService { client.platform().load_flags(featureFlagMap); } - private toSettings(env: Environment): ClientSettings { + private async toSettings(env: Environment): Promise { return { apiUrl: env.getApiUrl(), identityUrl: env.getIdentityUrl(), deviceType: toSdkDevice(this.platformUtilsService.getDevice()), userAgent: this.userAgent ?? navigator.userAgent, + bitwardenClientVersion: await this.platformUtilsService.getApplicationVersionNumber(), }; } } diff --git a/libs/common/src/vault/abstractions/cipher-sdk.service.ts b/libs/common/src/vault/abstractions/cipher-sdk.service.ts new file mode 100644 index 00000000000..1037bfc2b92 --- /dev/null +++ b/libs/common/src/vault/abstractions/cipher-sdk.service.ts @@ -0,0 +1,37 @@ +import { UserId } from "@bitwarden/common/types/guid"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; + +/** + * Service responsible for cipher operations using the SDK. + */ +export abstract class CipherSdkService { + /** + * Creates a new cipher on the server using the SDK. + * + * @param cipherView The cipher view to create + * @param userId The user ID to use for SDK client + * @param orgAdmin Whether this is an organization admin operation + * @returns A promise that resolves to the created cipher view + */ + abstract createWithServer( + cipherView: CipherView, + userId: UserId, + orgAdmin?: boolean, + ): Promise; + + /** + * Updates a cipher on the server using the SDK. + * + * @param cipher The cipher view to update + * @param userId The user ID to use for SDK client + * @param originalCipherView The original cipher view before changes (optional, used for admin operations) + * @param orgAdmin Whether this is an organization admin operation + * @returns A promise that resolves to the updated cipher view + */ + abstract updateWithServer( + cipher: CipherView, + userId: UserId, + originalCipherView?: CipherView, + orgAdmin?: boolean, + ): Promise; +} diff --git a/libs/common/src/vault/abstractions/cipher.service.ts b/libs/common/src/vault/abstractions/cipher.service.ts index 203984075f7..1db5f8d38a7 100644 --- a/libs/common/src/vault/abstractions/cipher.service.ts +++ b/libs/common/src/vault/abstractions/cipher.service.ts @@ -119,9 +119,11 @@ export abstract class CipherService implements UserKeyRotationDataProvider; + ): Promise; + /** * Update a cipher with the server * @param cipher The cipher to update @@ -131,10 +133,11 @@ export abstract class CipherService implements UserKeyRotationDataProvider; + ): Promise; /** * Move a cipher to an organization by re-encrypting its keys with the organization's key. diff --git a/libs/common/src/vault/models/view/cipher.view.spec.ts b/libs/common/src/vault/models/view/cipher.view.spec.ts index 475fe9e23f3..1c7017d5d89 100644 --- a/libs/common/src/vault/models/view/cipher.view.spec.ts +++ b/libs/common/src/vault/models/view/cipher.view.spec.ts @@ -353,4 +353,366 @@ describe("CipherView", () => { }); }); }); + + // Note: These tests use jest.requireActual() because the file has jest.mock() calls + // at the top that mock LoginView, FieldView, etc. Those mocks are needed for other tests + // but interfere with these tests which need the real implementations. + describe("toSdkCreateCipherRequest", () => { + it("maps all properties correctly for a login cipher", () => { + const { FieldView: RealFieldView } = jest.requireActual("./field.view"); + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + + const cipherView = new CipherView(); + cipherView.organizationId = "000f2a6e-da5e-4726-87ed-1c5c77322c3c"; + cipherView.folderId = "41b22db4-8e2a-4ed2-b568-f1186c72922f"; + cipherView.collectionIds = ["b0473506-3c3c-4260-a734-dfaaf833ab6f"]; + cipherView.name = "Test Login"; + cipherView.notes = "Test notes"; + cipherView.type = CipherType.Login; + cipherView.favorite = true; + cipherView.reprompt = CipherRepromptType.Password; + + const field = new RealFieldView(); + field.name = "testField"; + field.value = "testValue"; + field.type = SdkFieldType.Text; + cipherView.fields = [field]; + + cipherView.login = new RealLoginView(); + cipherView.login.username = "testuser"; + cipherView.login.password = "testpass"; + + const result = cipherView.toSdkCreateCipherRequest(); + + expect(result.organizationId).toEqual(asUuid("000f2a6e-da5e-4726-87ed-1c5c77322c3c")); + expect(result.folderId).toEqual(asUuid("41b22db4-8e2a-4ed2-b568-f1186c72922f")); + expect(result.collectionIds).toEqual([asUuid("b0473506-3c3c-4260-a734-dfaaf833ab6f")]); + expect(result.name).toBe("Test Login"); + expect(result.notes).toBe("Test notes"); + expect(result.favorite).toBe(true); + expect(result.reprompt).toBe(CipherRepromptType.Password); + expect(result.fields).toHaveLength(1); + expect(result.fields![0]).toMatchObject({ + name: "testField", + value: "testValue", + type: SdkFieldType.Text, + }); + expect(result.type).toHaveProperty("login"); + expect((result.type as any).login).toMatchObject({ + username: "testuser", + password: "testpass", + }); + }); + + it("handles undefined organizationId and folderId", () => { + const { SecureNoteView: RealSecureNoteView } = jest.requireActual("./secure-note.view"); + + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + cipherView.type = CipherType.SecureNote; + cipherView.secureNote = new RealSecureNoteView(); + + const result = cipherView.toSdkCreateCipherRequest(); + + expect(result.organizationId).toBeUndefined(); + expect(result.folderId).toBeUndefined(); + expect(result.name).toBe("Test Cipher"); + }); + + it("handles empty collectionIds array", () => { + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + cipherView.collectionIds = []; + cipherView.type = CipherType.Login; + cipherView.login = new RealLoginView(); + + const result = cipherView.toSdkCreateCipherRequest(); + + expect(result.collectionIds).toEqual([]); + }); + + it("defaults favorite to false when undefined", () => { + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + cipherView.favorite = undefined as any; + cipherView.type = CipherType.Login; + cipherView.login = new RealLoginView(); + + const result = cipherView.toSdkCreateCipherRequest(); + + expect(result.favorite).toBe(false); + }); + + it("defaults reprompt to None when undefined", () => { + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + cipherView.reprompt = undefined as any; + cipherView.type = CipherType.Login; + cipherView.login = new RealLoginView(); + + const result = cipherView.toSdkCreateCipherRequest(); + + expect(result.reprompt).toBe(CipherRepromptType.None); + }); + + test.each([ + ["Login", CipherType.Login, "login.view", "LoginView"], + ["Card", CipherType.Card, "card.view", "CardView"], + ["Identity", CipherType.Identity, "identity.view", "IdentityView"], + ["SecureNote", CipherType.SecureNote, "secure-note.view", "SecureNoteView"], + ["SshKey", CipherType.SshKey, "ssh-key.view", "SshKeyView"], + ])( + "creates correct type property for %s cipher", + (typeName: string, cipherType: CipherType, moduleName: string, className: string) => { + const module = jest.requireActual(`./${moduleName}`); + const ViewClass = module[className]; + + const cipherView = new CipherView(); + cipherView.name = `Test ${typeName}`; + cipherView.type = cipherType; + + // Set the appropriate view property + const viewPropertyName = typeName.charAt(0).toLowerCase() + typeName.slice(1); + (cipherView as any)[viewPropertyName] = new ViewClass(); + + const result = cipherView.toSdkCreateCipherRequest(); + + const typeKey = typeName.charAt(0).toLowerCase() + typeName.slice(1); + expect(result.type).toHaveProperty(typeKey); + }, + ); + }); + + describe("toSdkUpdateCipherRequest", () => { + it("maps all properties correctly for an update request", () => { + const { FieldView: RealFieldView } = jest.requireActual("./field.view"); + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + + const cipherView = new CipherView(); + cipherView.id = "0a54d80c-14aa-4ef8-8c3a-7ea99ce5b602"; + cipherView.organizationId = "000f2a6e-da5e-4726-87ed-1c5c77322c3c"; + cipherView.folderId = "41b22db4-8e2a-4ed2-b568-f1186c72922f"; + cipherView.name = "Updated Login"; + cipherView.notes = "Updated notes"; + cipherView.type = CipherType.Login; + cipherView.favorite = true; + cipherView.reprompt = CipherRepromptType.Password; + cipherView.revisionDate = new Date("2022-01-02T12:00:00.000Z"); + cipherView.archivedDate = new Date("2022-01-03T12:00:00.000Z"); + cipherView.key = new EncString("cipher-key"); + + const mockField = new RealFieldView(); + mockField.name = "testField"; + mockField.value = "testValue"; + cipherView.fields = [mockField]; + + cipherView.login = new RealLoginView(); + cipherView.login.username = "testuser"; + + const result = cipherView.toSdkUpdateCipherRequest(); + + expect(result.id).toEqual(asUuid("0a54d80c-14aa-4ef8-8c3a-7ea99ce5b602")); + expect(result.organizationId).toEqual(asUuid("000f2a6e-da5e-4726-87ed-1c5c77322c3c")); + expect(result.folderId).toEqual(asUuid("41b22db4-8e2a-4ed2-b568-f1186c72922f")); + expect(result.name).toBe("Updated Login"); + expect(result.notes).toBe("Updated notes"); + expect(result.favorite).toBe(true); + expect(result.reprompt).toBe(CipherRepromptType.Password); + expect(result.revisionDate).toBe("2022-01-02T12:00:00.000Z"); + expect(result.archivedDate).toBe("2022-01-03T12:00:00.000Z"); + expect(result.fields).toHaveLength(1); + expect(result.fields![0]).toMatchObject({ + name: "testField", + value: "testValue", + }); + expect(result.type).toHaveProperty("login"); + expect((result.type as any).login).toMatchObject({ + username: "testuser", + }); + expect(result.key).toBeDefined(); + }); + + it("handles undefined optional properties", () => { + const { SecureNoteView: RealSecureNoteView } = jest.requireActual("./secure-note.view"); + + const cipherView = new CipherView(); + cipherView.id = "0a54d80c-14aa-4ef8-8c3a-7ea99ce5b602"; + cipherView.name = "Test Cipher"; + cipherView.type = CipherType.SecureNote; + cipherView.secureNote = new RealSecureNoteView(); + cipherView.revisionDate = new Date("2022-01-02T12:00:00.000Z"); + + const result = cipherView.toSdkUpdateCipherRequest(); + + expect(result.organizationId).toBeUndefined(); + expect(result.folderId).toBeUndefined(); + expect(result.archivedDate).toBeUndefined(); + expect(result.key).toBeUndefined(); + }); + + it("converts dates to ISO strings", () => { + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + + const cipherView = new CipherView(); + cipherView.id = "0a54d80c-14aa-4ef8-8c3a-7ea99ce5b602"; + cipherView.name = "Test Cipher"; + cipherView.type = CipherType.Login; + cipherView.login = new RealLoginView(); + cipherView.revisionDate = new Date("2022-05-15T10:30:00.000Z"); + cipherView.archivedDate = new Date("2022-06-20T14:45:00.000Z"); + + const result = cipherView.toSdkUpdateCipherRequest(); + + expect(result.revisionDate).toBe("2022-05-15T10:30:00.000Z"); + expect(result.archivedDate).toBe("2022-06-20T14:45:00.000Z"); + }); + + it("includes attachments when present", () => { + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + const { AttachmentView: RealAttachmentView } = jest.requireActual("./attachment.view"); + + const cipherView = new CipherView(); + cipherView.id = "0a54d80c-14aa-4ef8-8c3a-7ea99ce5b602"; + cipherView.name = "Test Cipher"; + cipherView.type = CipherType.Login; + cipherView.login = new RealLoginView(); + + const attachment1 = new RealAttachmentView(); + attachment1.id = "attachment-id-1"; + attachment1.fileName = "file1.txt"; + + const attachment2 = new RealAttachmentView(); + attachment2.id = "attachment-id-2"; + attachment2.fileName = "file2.pdf"; + + cipherView.attachments = [attachment1, attachment2]; + + const result = cipherView.toSdkUpdateCipherRequest(); + + expect(result.attachments).toHaveLength(2); + }); + + test.each([ + ["Login", CipherType.Login, "login.view", "LoginView"], + ["Card", CipherType.Card, "card.view", "CardView"], + ["Identity", CipherType.Identity, "identity.view", "IdentityView"], + ["SecureNote", CipherType.SecureNote, "secure-note.view", "SecureNoteView"], + ["SshKey", CipherType.SshKey, "ssh-key.view", "SshKeyView"], + ])( + "creates correct type property for %s cipher", + (typeName: string, cipherType: CipherType, moduleName: string, className: string) => { + const module = jest.requireActual(`./${moduleName}`); + const ViewClass = module[className]; + + const cipherView = new CipherView(); + cipherView.id = "0a54d80c-14aa-4ef8-8c3a-7ea99ce5b602"; + cipherView.name = `Test ${typeName}`; + cipherView.type = cipherType; + + // Set the appropriate view property + const viewPropertyName = typeName.charAt(0).toLowerCase() + typeName.slice(1); + (cipherView as any)[viewPropertyName] = new ViewClass(); + + const result = cipherView.toSdkUpdateCipherRequest(); + + const typeKey = typeName.charAt(0).toLowerCase() + typeName.slice(1); + expect(result.type).toHaveProperty(typeKey); + }, + ); + }); + + describe("getSdkCipherViewType", () => { + it("returns login type for Login cipher", () => { + const { LoginView: RealLoginView } = jest.requireActual("./login.view"); + + const cipherView = new CipherView(); + cipherView.type = CipherType.Login; + cipherView.login = new RealLoginView(); + cipherView.login.username = "testuser"; + cipherView.login.password = "testpass"; + + const result = cipherView.getSdkCipherViewType(); + + expect(result).toHaveProperty("login"); + expect((result as any).login).toMatchObject({ + username: "testuser", + password: "testpass", + }); + }); + + it("returns card type for Card cipher", () => { + const { CardView: RealCardView } = jest.requireActual("./card.view"); + + const cipherView = new CipherView(); + cipherView.type = CipherType.Card; + cipherView.card = new RealCardView(); + cipherView.card.cardholderName = "John Doe"; + cipherView.card.number = "4111111111111111"; + + const result = cipherView.getSdkCipherViewType(); + + expect(result).toHaveProperty("card"); + expect((result as any).card.cardholderName).toBe("John Doe"); + expect((result as any).card.number).toBe("4111111111111111"); + }); + + it("returns identity type for Identity cipher", () => { + const { IdentityView: RealIdentityView } = jest.requireActual("./identity.view"); + + const cipherView = new CipherView(); + cipherView.type = CipherType.Identity; + cipherView.identity = new RealIdentityView(); + cipherView.identity.firstName = "John"; + cipherView.identity.lastName = "Doe"; + + const result = cipherView.getSdkCipherViewType(); + + expect(result).toHaveProperty("identity"); + expect((result as any).identity.firstName).toBe("John"); + expect((result as any).identity.lastName).toBe("Doe"); + }); + + it("returns secureNote type for SecureNote cipher", () => { + const { SecureNoteView: RealSecureNoteView } = jest.requireActual("./secure-note.view"); + + const cipherView = new CipherView(); + cipherView.type = CipherType.SecureNote; + cipherView.secureNote = new RealSecureNoteView(); + + const result = cipherView.getSdkCipherViewType(); + + expect(result).toHaveProperty("secureNote"); + }); + + it("returns sshKey type for SshKey cipher", () => { + const { SshKeyView: RealSshKeyView } = jest.requireActual("./ssh-key.view"); + + const cipherView = new CipherView(); + cipherView.type = CipherType.SshKey; + cipherView.sshKey = new RealSshKeyView(); + cipherView.sshKey.privateKey = "privateKeyData"; + cipherView.sshKey.publicKey = "publicKeyData"; + + const result = cipherView.getSdkCipherViewType(); + + expect(result).toHaveProperty("sshKey"); + expect((result as any).sshKey.privateKey).toBe("privateKeyData"); + expect((result as any).sshKey.publicKey).toBe("publicKeyData"); + }); + + it("defaults to empty login for unknown cipher type", () => { + const cipherView = new CipherView(); + cipherView.type = 999 as CipherType; + + const result = cipherView.getSdkCipherViewType(); + + expect(result).toHaveProperty("login"); + }); + }); }); diff --git a/libs/common/src/vault/models/view/cipher.view.ts b/libs/common/src/vault/models/view/cipher.view.ts index 89f59665681..0909d0bda80 100644 --- a/libs/common/src/vault/models/view/cipher.view.ts +++ b/libs/common/src/vault/models/view/cipher.view.ts @@ -1,7 +1,12 @@ import { EncString } from "@bitwarden/common/key-management/crypto/models/enc-string"; import { asUuid, uuidAsString } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; import { ItemView } from "@bitwarden/common/vault/models/view/item.view"; -import { CipherView as SdkCipherView } from "@bitwarden/sdk-internal"; +import { + CipherCreateRequest, + CipherEditRequest, + CipherViewType, + CipherView as SdkCipherView, +} from "@bitwarden/sdk-internal"; import { View } from "../../../models/view/view"; import { InitializerMetadata } from "../../../platform/interfaces/initializer-metadata.interface"; @@ -332,6 +337,85 @@ export class CipherView implements View, InitializerMetadata { return cipherView; } + /** + * Maps CipherView to an SDK CipherCreateRequest + * + * @returns {CipherCreateRequest} The SDK cipher create request object + */ + toSdkCreateCipherRequest(): CipherCreateRequest { + const sdkCipherCreateRequest: CipherCreateRequest = { + organizationId: this.organizationId ? asUuid(this.organizationId) : undefined, + collectionIds: this.collectionIds ? this.collectionIds.map((i) => asUuid(i)) : [], + folderId: this.folderId ? asUuid(this.folderId) : undefined, + name: this.name ?? "", + notes: this.notes, + favorite: this.favorite ?? false, + reprompt: this.reprompt ?? CipherRepromptType.None, + fields: this.fields?.map((f) => f.toSdkFieldView()), + type: this.getSdkCipherViewType(), + }; + + return sdkCipherCreateRequest; + } + + /** + * Maps CipherView to an SDK CipherEditRequest + * + * @returns {CipherEditRequest} The SDK cipher edit request object + */ + toSdkUpdateCipherRequest(): CipherEditRequest { + const sdkCipherEditRequest: CipherEditRequest = { + id: asUuid(this.id), + organizationId: this.organizationId ? asUuid(this.organizationId) : undefined, + folderId: this.folderId ? asUuid(this.folderId) : undefined, + name: this.name ?? "", + notes: this.notes, + favorite: this.favorite ?? false, + reprompt: this.reprompt ?? CipherRepromptType.None, + fields: this.fields?.map((f) => f.toSdkFieldView()), + type: this.getSdkCipherViewType(), + revisionDate: this.revisionDate?.toISOString(), + archivedDate: this.archivedDate?.toISOString(), + attachments: this.attachments?.map((a) => a.toSdkAttachmentView()), + key: this.key?.toSdk(), + }; + + return sdkCipherEditRequest; + } + + /** + * Returns the SDK CipherViewType object for the cipher. + * + * @returns {CipherViewType} The SDK CipherViewType for the cipher.t + */ + getSdkCipherViewType(): CipherViewType { + let viewType: CipherViewType; + switch (this.type) { + case CipherType.Card: + viewType = { card: this.card?.toSdkCardView() }; + break; + case CipherType.Identity: + viewType = { identity: this.identity?.toSdkIdentityView() }; + break; + case CipherType.Login: + viewType = { login: this.login?.toSdkLoginView() }; + break; + case CipherType.SecureNote: + viewType = { secureNote: this.secureNote?.toSdkSecureNoteView() }; + break; + case CipherType.SshKey: + viewType = { sshKey: this.sshKey?.toSdkSshKeyView() }; + break; + default: + viewType = { + // Default to empty login - should not be valid code path. + login: new LoginView().toSdkLoginView(), + }; + break; + } + return viewType; + } + /** * Maps CipherView to SdkCipherView * diff --git a/libs/common/src/vault/services/cipher-sdk.service.spec.ts b/libs/common/src/vault/services/cipher-sdk.service.spec.ts new file mode 100644 index 00000000000..bd3feb4619e --- /dev/null +++ b/libs/common/src/vault/services/cipher-sdk.service.spec.ts @@ -0,0 +1,246 @@ +import { mock } from "jest-mock-extended"; +import { of } from "rxjs"; + +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { SdkService } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; +import { UserId, CipherId, OrganizationId } from "@bitwarden/common/types/guid"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; + +import { CipherType } from "../enums/cipher-type"; + +import { DefaultCipherSdkService } from "./cipher-sdk.service"; + +describe("DefaultCipherSdkService", () => { + const sdkService = mock(); + const logService = mock(); + const userId = "test-user-id" as UserId; + const cipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; + const orgId = "4ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b21" as OrganizationId; + + let cipherSdkService: DefaultCipherSdkService; + let mockSdkClient: any; + let mockCiphersSdk: any; + let mockAdminSdk: any; + let mockVaultSdk: any; + + beforeEach(() => { + // Mock the SDK client chain for admin operations + mockAdminSdk = { + create: jest.fn(), + edit: jest.fn(), + }; + mockCiphersSdk = { + create: jest.fn(), + edit: jest.fn(), + admin: jest.fn().mockReturnValue(mockAdminSdk), + }; + mockVaultSdk = { + ciphers: jest.fn().mockReturnValue(mockCiphersSdk), + }; + const mockSdkValue = { + vault: jest.fn().mockReturnValue(mockVaultSdk), + }; + mockSdkClient = { + take: jest.fn().mockReturnValue({ + value: mockSdkValue, + [Symbol.dispose]: jest.fn(), + }), + }; + + // Mock sdkService to return the mock client + sdkService.userClient$.mockReturnValue(of(mockSdkClient)); + + cipherSdkService = new DefaultCipherSdkService(sdkService, logService); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe("createWithServer()", () => { + it("should create cipher using SDK when orgAdmin is false", async () => { + const cipherView = new CipherView(); + cipherView.id = cipherId; + cipherView.type = CipherType.Login; + cipherView.name = "Test Cipher"; + cipherView.organizationId = orgId; + + const mockSdkCipherView = cipherView.toSdkCipherView(); + mockCiphersSdk.create.mockResolvedValue(mockSdkCipherView); + + const result = await cipherSdkService.createWithServer(cipherView, userId, false); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: cipherView.name, + organizationId: expect.anything(), + }), + ); + expect(result).toBeInstanceOf(CipherView); + expect(result?.name).toBe(cipherView.name); + }); + + it("should create cipher using SDK admin API when orgAdmin is true", async () => { + const cipherView = new CipherView(); + cipherView.id = cipherId; + cipherView.type = CipherType.Login; + cipherView.name = "Test Admin Cipher"; + cipherView.organizationId = orgId; + + const mockSdkCipherView = cipherView.toSdkCipherView(); + mockAdminSdk.create.mockResolvedValue(mockSdkCipherView); + + const result = await cipherSdkService.createWithServer(cipherView, userId, true); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: cipherView.name, + }), + ); + expect(result).toBeInstanceOf(CipherView); + expect(result?.name).toBe(cipherView.name); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + + await expect(cipherSdkService.createWithServer(cipherView, userId)).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to create cipher"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + + mockCiphersSdk.create.mockRejectedValue(new Error("SDK error")); + + await expect(cipherSdkService.createWithServer(cipherView, userId)).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to create cipher"), + ); + }); + }); + + describe("updateWithServer()", () => { + it("should update cipher using SDK when orgAdmin is false", async () => { + const cipherView = new CipherView(); + cipherView.id = cipherId; + cipherView.type = CipherType.Login; + cipherView.name = "Updated Cipher"; + cipherView.organizationId = orgId; + + const mockSdkCipherView = cipherView.toSdkCipherView(); + mockCiphersSdk.edit.mockResolvedValue(mockSdkCipherView); + + const result = await cipherSdkService.updateWithServer(cipherView, userId, undefined, false); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.edit).toHaveBeenCalledWith( + expect.objectContaining({ + id: expect.anything(), + name: cipherView.name, + }), + ); + expect(result).toBeInstanceOf(CipherView); + expect(result.name).toBe(cipherView.name); + }); + + it("should update cipher using SDK admin API when orgAdmin is true", async () => { + const cipherView = new CipherView(); + cipherView.id = cipherId; + cipherView.type = CipherType.Login; + cipherView.name = "Updated Admin Cipher"; + cipherView.organizationId = orgId; + + const originalCipherView = new CipherView(); + originalCipherView.id = cipherId; + originalCipherView.name = "Original Cipher"; + + const mockSdkCipherView = cipherView.toSdkCipherView(); + mockAdminSdk.edit.mockResolvedValue(mockSdkCipherView); + + const result = await cipherSdkService.updateWithServer( + cipherView, + userId, + originalCipherView, + true, + ); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.edit).toHaveBeenCalledWith( + expect.objectContaining({ + id: expect.anything(), + name: cipherView.name, + }), + originalCipherView.toSdkCipherView(), + ); + expect(result).toBeInstanceOf(CipherView); + expect(result.name).toBe(cipherView.name); + }); + + it("should update cipher using SDK admin API without originalCipherView", async () => { + const cipherView = new CipherView(); + cipherView.id = cipherId; + cipherView.type = CipherType.Login; + cipherView.name = "Updated Admin Cipher"; + cipherView.organizationId = orgId; + + const mockSdkCipherView = cipherView.toSdkCipherView(); + mockAdminSdk.edit.mockResolvedValue(mockSdkCipherView); + + const result = await cipherSdkService.updateWithServer(cipherView, userId, undefined, true); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.edit).toHaveBeenCalledWith( + expect.objectContaining({ + id: expect.anything(), + name: cipherView.name, + }), + expect.anything(), // Empty CipherView - timestamps vary so we just verify it was called + ); + expect(result).toBeInstanceOf(CipherView); + expect(result.name).toBe(cipherView.name); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + + await expect( + cipherSdkService.updateWithServer(cipherView, userId, undefined, false), + ).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to update cipher"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + const cipherView = new CipherView(); + cipherView.name = "Test Cipher"; + + mockCiphersSdk.edit.mockRejectedValue(new Error("SDK error")); + + await expect( + cipherSdkService.updateWithServer(cipherView, userId, undefined, false), + ).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to update cipher"), + ); + }); + }); +}); diff --git a/libs/common/src/vault/services/cipher-sdk.service.ts b/libs/common/src/vault/services/cipher-sdk.service.ts new file mode 100644 index 00000000000..06f5d3eb961 --- /dev/null +++ b/libs/common/src/vault/services/cipher-sdk.service.ts @@ -0,0 +1,82 @@ +import { firstValueFrom, switchMap, catchError } from "rxjs"; + +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { SdkService } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; +import { UserId } from "@bitwarden/common/types/guid"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; +import { CipherView as SdkCipherView } from "@bitwarden/sdk-internal"; + +import { CipherSdkService } from "../abstractions/cipher-sdk.service"; + +export class DefaultCipherSdkService implements CipherSdkService { + constructor( + private sdkService: SdkService, + private logService: LogService, + ) {} + + async createWithServer( + cipherView: CipherView, + userId: UserId, + orgAdmin?: boolean, + ): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + const sdkCreateRequest = cipherView.toSdkCreateCipherRequest(); + let result: SdkCipherView; + if (orgAdmin) { + result = await ref.value.vault().ciphers().admin().create(sdkCreateRequest); + } else { + result = await ref.value.vault().ciphers().create(sdkCreateRequest); + } + return CipherView.fromSdkCipherView(result); + }), + catchError((error: unknown) => { + this.logService.error(`Failed to create cipher: ${error}`); + throw error; + }), + ), + ); + } + + async updateWithServer( + cipher: CipherView, + userId: UserId, + originalCipherView?: CipherView, + orgAdmin?: boolean, + ): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + const sdkUpdateRequest = cipher.toSdkUpdateCipherRequest(); + let result: SdkCipherView; + if (orgAdmin) { + result = await ref.value + .vault() + .ciphers() + .admin() + .edit( + sdkUpdateRequest, + originalCipherView?.toSdkCipherView() || new CipherView().toSdkCipherView(), + ); + } else { + result = await ref.value.vault().ciphers().edit(sdkUpdateRequest); + } + return CipherView.fromSdkCipherView(result); + }), + catchError((error: unknown) => { + this.logService.error(`Failed to update cipher: ${error}`); + throw error; + }), + ), + ); + } +} diff --git a/libs/common/src/vault/services/cipher.service.spec.ts b/libs/common/src/vault/services/cipher.service.spec.ts index 153bb01403c..4f98ba62a1c 100644 --- a/libs/common/src/vault/services/cipher.service.spec.ts +++ b/libs/common/src/vault/services/cipher.service.spec.ts @@ -28,6 +28,7 @@ import { ContainerService } from "../../platform/services/container.service"; import { CipherId, UserId, OrganizationId, CollectionId } from "../../types/guid"; import { CipherKey, OrgKey, UserKey } from "../../types/key"; import { CipherEncryptionService } from "../abstractions/cipher-encryption.service"; +import { CipherSdkService } from "../abstractions/cipher-sdk.service"; import { EncryptionContext } from "../abstractions/cipher.service"; import { CipherFileUploadService } from "../abstractions/file-upload/cipher-file-upload.service"; import { SearchService } from "../abstractions/search.service"; @@ -54,9 +55,9 @@ function encryptText(clearText: string | Uint8Array) { const ENCRYPTED_BYTES = mock(); const cipherData: CipherData = { - id: "id", - organizationId: "4ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b2" as OrganizationId, - folderId: "folderId", + id: "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId, + organizationId: "4ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b21" as OrganizationId, + folderId: "6ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b23", edit: true, viewPassword: true, organizationUseTotp: true, @@ -109,9 +110,10 @@ describe("Cipher Service", () => { const stateProvider = new FakeStateProvider(accountService); const cipherEncryptionService = mock(); const messageSender = mock(); + const cipherSdkService = mock(); const userId = "TestUserId" as UserId; - const orgId = "4ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b2" as OrganizationId; + const orgId = "4ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b21" as OrganizationId; let cipherService: CipherService; let encryptionContext: EncryptionContext; @@ -145,6 +147,7 @@ describe("Cipher Service", () => { logService, cipherEncryptionService, messageSender, + cipherSdkService, ); encryptionContext = { cipher: new Cipher(cipherData), encryptedFor: userId }; @@ -207,11 +210,22 @@ describe("Cipher Service", () => { }); describe("createWithServer()", () => { + beforeEach(() => { + jest.spyOn(cipherService, "encrypt").mockResolvedValue(encryptionContext); + jest.spyOn(cipherService, "decrypt").mockImplementation(async (cipher) => { + return new CipherView(cipher); + }); + }); + 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())); - await cipherService.createWithServer(encryptionContext, true); + const cipherView = new CipherView(encryptionContext.cipher); + await cipherService.createWithServer(cipherView, userId, true); const expectedObj = new CipherCreateRequest(encryptionContext); expect(spy).toHaveBeenCalled(); @@ -219,11 +233,15 @@ 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") .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); - await cipherService.createWithServer(encryptionContext, true); + const cipherView = new CipherView(encryptionContext.cipher); + await cipherService.createWithServer(cipherView, userId, true); const expectedObj = new CipherRequest(encryptionContext); expect(spy).toHaveBeenCalled(); @@ -231,11 +249,15 @@ 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") .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); - await cipherService.createWithServer(encryptionContext); + const cipherView = new CipherView(encryptionContext.cipher); + await cipherService.createWithServer(cipherView, userId); const expectedObj = new CipherCreateRequest(encryptionContext); expect(spy).toHaveBeenCalled(); @@ -243,35 +265,86 @@ 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())); - await cipherService.createWithServer(encryptionContext); + const cipherView = new CipherView(encryptionContext.cipher); + await cipherService.createWithServer(cipherView, userId); const expectedObj = new CipherRequest(encryptionContext); expect(spy).toHaveBeenCalled(); expect(spy).toHaveBeenCalledWith(expectedObj); }); + + it("should delegate to cipherSdkService when feature flag is enabled", async () => { + configService.getFeatureFlag + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockResolvedValue(true); + + const cipherView = new CipherView(encryptionContext.cipher); + const expectedResult = new CipherView(encryptionContext.cipher); + + const cipherSdkServiceSpy = jest + .spyOn(cipherSdkService, "createWithServer") + .mockResolvedValue(expectedResult); + + const clearCacheSpy = jest.spyOn(cipherService, "clearCache"); + const apiSpy = jest.spyOn(apiService, "postCipher"); + + const result = await cipherService.createWithServer(cipherView, userId); + + expect(cipherSdkServiceSpy).toHaveBeenCalledWith(cipherView, userId, undefined); + expect(apiSpy).not.toHaveBeenCalled(); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + expect(result).toBeInstanceOf(CipherView); + }); }); describe("updateWithServer()", () => { + beforeEach(() => { + jest.spyOn(cipherService, "encrypt").mockResolvedValue(encryptionContext); + jest.spyOn(cipherService, "decrypt").mockImplementation(async (cipher) => { + return new CipherView(cipher); + }); + jest.spyOn(cipherService, "upsert").mockResolvedValue({ + [cipherData.id as CipherId]: cipherData, + }); + }); + it("should call apiService.putCipherAdmin when orgAdmin param is true", async () => { + configService.getFeatureFlag + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockResolvedValue(false); + + const testCipher = new Cipher(cipherData); + testCipher.organizationId = orgId; + const testContext = { cipher: testCipher, encryptedFor: userId }; + jest.spyOn(cipherService, "encrypt").mockResolvedValue(testContext); + const spy = jest .spyOn(apiService, "putCipherAdmin") - .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); - await cipherService.updateWithServer(encryptionContext, true); - const expectedObj = new CipherRequest(encryptionContext); + .mockImplementation(() => Promise.resolve(testCipher.toCipherData())); + const cipherView = new CipherView(testCipher); + await cipherService.updateWithServer(cipherView, userId, undefined, true); + const expectedObj = new CipherRequest(testContext); expect(spy).toHaveBeenCalled(); - expect(spy).toHaveBeenCalledWith(encryptionContext.cipher.id, expectedObj); + expect(spy).toHaveBeenCalledWith(testCipher.id, expectedObj); }); 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") .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); - await cipherService.updateWithServer(encryptionContext); + const cipherView = new CipherView(encryptionContext.cipher); + await cipherService.updateWithServer(cipherView, userId); const expectedObj = new CipherRequest(encryptionContext); expect(spy).toHaveBeenCalled(); @@ -279,16 +352,79 @@ 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") .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); - await cipherService.updateWithServer(encryptionContext); + const cipherView = new CipherView(encryptionContext.cipher); + await cipherService.updateWithServer(cipherView, userId); const expectedObj = new CipherPartialRequest(encryptionContext.cipher); expect(spy).toHaveBeenCalled(); expect(spy).toHaveBeenCalledWith(encryptionContext.cipher.id, expectedObj); }); + + it("should delegate to cipherSdkService when feature flag is enabled", async () => { + configService.getFeatureFlag + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockResolvedValue(true); + + const testCipher = new Cipher(cipherData); + const cipherView = new CipherView(testCipher); + const expectedResult = new CipherView(testCipher); + + const cipherSdkServiceSpy = jest + .spyOn(cipherSdkService, "updateWithServer") + .mockResolvedValue(expectedResult); + + const clearCacheSpy = jest.spyOn(cipherService, "clearCache"); + const apiSpy = jest.spyOn(apiService, "putCipher"); + + const result = await cipherService.updateWithServer(cipherView, userId); + + expect(cipherSdkServiceSpy).toHaveBeenCalledWith(cipherView, userId, undefined, undefined); + expect(apiSpy).not.toHaveBeenCalled(); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + expect(result).toBeInstanceOf(CipherView); + }); + + it("should delegate to cipherSdkService with orgAdmin when feature flag is enabled", async () => { + configService.getFeatureFlag + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockResolvedValue(true); + + const testCipher = new Cipher(cipherData); + const cipherView = new CipherView(testCipher); + const originalCipherView = new CipherView(testCipher); + const expectedResult = new CipherView(testCipher); + + const cipherSdkServiceSpy = jest + .spyOn(cipherSdkService, "updateWithServer") + .mockResolvedValue(expectedResult); + + const clearCacheSpy = jest.spyOn(cipherService, "clearCache"); + const apiSpy = jest.spyOn(apiService, "putCipherAdmin"); + + const result = await cipherService.updateWithServer( + cipherView, + userId, + originalCipherView, + true, + ); + + expect(cipherSdkServiceSpy).toHaveBeenCalledWith( + cipherView, + userId, + originalCipherView, + true, + ); + expect(apiSpy).not.toHaveBeenCalled(); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + expect(result).toBeInstanceOf(CipherView); + }); }); describe("encrypt", () => { diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 2e0adc892e3..53d7666e304 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -42,6 +42,7 @@ import { CipherId, CollectionId, OrganizationId, UserId } from "../../types/guid import { OrgKey, UserKey } from "../../types/key"; import { filterOutNullish, perUserCache$ } from "../../vault/utils/observable-utilities"; import { CipherEncryptionService } from "../abstractions/cipher-encryption.service"; +import { CipherSdkService } from "../abstractions/cipher-sdk.service"; import { CipherService as CipherServiceAbstraction, EncryptionContext, @@ -120,6 +121,7 @@ export class CipherService implements CipherServiceAbstraction { private logService: LogService, private cipherEncryptionService: CipherEncryptionService, private messageSender: MessageSender, + private cipherSdkService: CipherSdkService, ) {} localData$(userId: UserId): Observable> { @@ -903,6 +905,40 @@ export class CipherService implements CipherServiceAbstraction { } async createWithServer( + cipherView: CipherView, + userId: UserId, + orgAdmin?: boolean, + ): Promise { + const useSdk = await this.configService.getFeatureFlag( + FeatureFlag.PM27632_SdkCipherCrudOperations, + ); + + if (useSdk) { + return ( + (await this.createWithServerUsingSdk(cipherView, userId, orgAdmin)) || new CipherView() + ); + } + + const encrypted = await this.encrypt(cipherView, userId); + const result = await this.createWithServer_legacy(encrypted, orgAdmin); + return await this.decrypt(result, userId); + } + + private async createWithServerUsingSdk( + cipherView: CipherView, + userId: UserId, + orgAdmin?: boolean, + ): Promise { + const resultCipherView = await this.cipherSdkService.createWithServer( + cipherView, + userId, + orgAdmin, + ); + await this.clearCache(userId); + return resultCipherView; + } + + private async createWithServer_legacy( { cipher, encryptedFor }: EncryptionContext, orgAdmin?: boolean, ): Promise { @@ -929,6 +965,42 @@ export class CipherService implements CipherServiceAbstraction { } async updateWithServer( + cipherView: CipherView, + userId: UserId, + originalCipherView?: CipherView, + orgAdmin?: boolean, + ): Promise { + const useSdk = await this.configService.getFeatureFlag( + FeatureFlag.PM27632_SdkCipherCrudOperations, + ); + + if (useSdk) { + return await this.updateWithServerUsingSdk(cipherView, userId, originalCipherView, orgAdmin); + } + + const encrypted = await this.encrypt(cipherView, userId); + const updatedCipher = await this.updateWithServer_legacy(encrypted, orgAdmin); + const updatedCipherView = await this.decrypt(updatedCipher, userId); + return updatedCipherView; + } + + async updateWithServerUsingSdk( + cipher: CipherView, + userId: UserId, + originalCipherView?: CipherView, + orgAdmin?: boolean, + ): Promise { + const resultCipherView = await this.cipherSdkService.updateWithServer( + cipher, + userId, + originalCipherView, + orgAdmin, + ); + await this.clearCache(userId); + return resultCipherView; + } + + async updateWithServer_legacy( { cipher, encryptedFor }: EncryptionContext, orgAdmin?: boolean, ): Promise { @@ -1119,8 +1191,7 @@ export class CipherService implements CipherServiceAbstraction { //in order to keep item and it's attachments with the same encryption level if (cipher.key != null && !cipherKeyEncryptionEnabled) { const model = await this.decrypt(cipher, userId); - const reEncrypted = await this.encrypt(model, userId); - await this.updateWithServer(reEncrypted); + await this.updateWithServer(model, userId); } const encFileName = await this.encryptService.encryptString(filename, cipherEncKey); diff --git a/libs/vault/src/cipher-form/services/default-cipher-form.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form.service.ts index 59c583f980b..8566e51d74f 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form.service.ts +++ b/libs/vault/src/cipher-form/services/default-cipher-form.service.ts @@ -37,14 +37,13 @@ export class DefaultCipherFormService implements CipherFormService { // Creating a new cipher if (cipher.id == null || cipher.id === "") { - const encrypted = await this.cipherService.encrypt(cipher, activeUserId); - savedCipher = await this.cipherService.createWithServer(encrypted, config.admin); - return await this.cipherService.decrypt(savedCipher, activeUserId); + return await this.cipherService.createWithServer(cipher, activeUserId, config.admin); } if (config.originalCipher == null) { throw new Error("Original cipher is required for updating an existing cipher"); } + const originalCipherView = await this.decryptCipher(config.originalCipher); // Updating an existing cipher @@ -66,35 +65,31 @@ export class DefaultCipherFormService implements CipherFormService { ); // If the collectionIds are the same, update the cipher normally } else if (isSetEqual(originalCollectionIds, newCollectionIds)) { - const encrypted = await this.cipherService.encrypt( + const savedCipherView = await this.cipherService.updateWithServer( cipher, activeUserId, - null, - null, - config.originalCipher, + originalCipherView, + config.admin, ); - savedCipher = await this.cipherService.updateWithServer(encrypted, config.admin); + savedCipher = await this.cipherService + .encrypt(savedCipherView, activeUserId) + .then((res) => res.cipher); } else { - const encrypted = await this.cipherService.encrypt( - cipher, - activeUserId, - null, - null, - config.originalCipher, - ); - const encryptedCipher = encrypted.cipher; - // Updating a cipher with collection changes is not supported with a single request currently // First update the cipher with the original collectionIds - encryptedCipher.collectionIds = config.originalCipher.collectionIds; - await this.cipherService.updateWithServer( - encrypted, + cipher.collectionIds = config.originalCipher.collectionIds; + const newCipher = await this.cipherService.updateWithServer( + cipher, + activeUserId, + originalCipherView, config.admin || originalCollectionIds.size === 0, ); // Then save the new collection changes separately - encryptedCipher.collectionIds = cipher.collectionIds; + newCipher.collectionIds = cipher.collectionIds; + // TODO: Remove after migrating all SDK ops + const { cipher: encryptedCipher } = await this.cipherService.encrypt(newCipher, activeUserId); if (config.admin || originalCollectionIds.size === 0) { // When using an admin config or the cipher was unassigned, update collections as an admin savedCipher = await this.cipherService.saveCollectionsWithServerAdmin(encryptedCipher); From 8b9211ea620f5cfcbe908fed31e390ae06268d1e Mon Sep 17 00:00:00 2001 From: Jordan Aasen <166539328+jaasen-livefront@users.noreply.github.com> Date: Mon, 26 Jan 2026 11:52:30 -0800 Subject: [PATCH 3/9] do not show badge/button in AC (#18489) --- .../reports/pages/cipher-report.component.ts | 1 + .../vault-item-dialog.component.html | 2 +- .../vault-item-dialog.component.spec.ts | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts b/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts index d8519b86094..f775ed84ede 100644 --- a/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts +++ b/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts @@ -193,6 +193,7 @@ export abstract class CipherReportComponent implements OnDestroy { formConfig, activeCollectionId, disableForm, + isAdminConsoleAction: true, }); const result = await lastValueFrom(this.vaultItemDialogRef.closed); diff --git a/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.html b/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.html index 059347709f0..ec06c740f24 100644 --- a/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.html +++ b/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.html @@ -3,7 +3,7 @@ {{ title }} - @if (isCipherArchived) { + @if (isCipherArchived && !params.isAdminConsoleAction) { {{ "archived" | i18n }} } diff --git a/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts b/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts index 63b5071d1f5..9a048b7a8b3 100644 --- a/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts +++ b/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts @@ -303,6 +303,25 @@ describe("VaultItemDialogComponent", () => { }); }); + describe("archive badge", () => { + it('should show "archived" badge when the item is archived and not an admin console action', () => { + component.setTestCipher({ isArchived: true }); + component.setTestParams({ mode: "view" }); + fixture.detectChanges(); + const archivedBadge = fixture.debugElement.query(By.css("span[bitBadge]")); + expect(archivedBadge).toBeTruthy(); + expect(archivedBadge.nativeElement.textContent.trim()).toBe("archived"); + }); + + it('should not show "archived" badge when the item is archived and is an admin console action', () => { + component.setTestCipher({ isArchived: true }); + component.setTestParams({ mode: "view", isAdminConsoleAction: true }); + fixture.detectChanges(); + const archivedBadge = fixture.debugElement.query(By.css("span[bitBadge]")); + expect(archivedBadge).toBeFalsy(); + }); + }); + describe("submitButtonText$", () => { it("should return 'unArchiveAndSave' when premium is false and cipher is archived", (done) => { jest.spyOn(component as any, "userHasPremium$", "get").mockReturnValue(of(false)); From 5e8801f7ff5a71a91d7455088b387aae103c7b17 Mon Sep 17 00:00:00 2001 From: Jordan Aasen <166539328+jaasen-livefront@users.noreply.github.com> Date: Mon, 26 Jan 2026 12:00:03 -0800 Subject: [PATCH 4/9] [PM-29244] - don't use filename for download attachment label (#18444) * don't use filename for download attachment label * fix scroll position in browser vault * Revert "fix scroll position in browser vault" This reverts commit 8e415f2c899c3d2b6b029e1b013f85dc131b3468. * fix test --- apps/browser/src/_locales/en/messages.json | 3 +++ .../download-attachment/download-attachment.component.html | 2 +- .../download-attachment/download-attachment.component.spec.ts | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index 61085828cf2..8e2c3279687 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -5001,6 +5001,9 @@ } } }, + "downloadAttachmentLabel": { + "message": "Download Attachment" + }, "downloadBitwarden": { "message": "Download Bitwarden" }, diff --git a/libs/vault/src/components/download-attachment/download-attachment.component.html b/libs/vault/src/components/download-attachment/download-attachment.component.html index 9d80f36818a..c6665c5d569 100644 --- a/libs/vault/src/components/download-attachment/download-attachment.component.html +++ b/libs/vault/src/components/download-attachment/download-attachment.component.html @@ -5,6 +5,6 @@ buttonType="main" size="small" type="button" - [label]="'downloadAttachmentName' | i18n: attachment().fileName" + [label]="'downloadAttachmentLabel' | i18n" > } diff --git a/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts b/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts index 3bbc375fdfc..a46ce28fca8 100644 --- a/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts +++ b/libs/vault/src/components/download-attachment/download-attachment.component.spec.ts @@ -108,7 +108,7 @@ describe("DownloadAttachmentComponent", () => { it("renders delete button", () => { const deleteButton = fixture.debugElement.query(By.css("button")); - expect(deleteButton.attributes["aria-label"]).toBe("downloadAttachmentName"); + expect(deleteButton.attributes["aria-label"]).toBe("downloadAttachmentLabel"); }); describe("download attachment", () => { From ad577860be3f9f43836b56017b4985232eca7aca Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Mon, 26 Jan 2026 14:01:53 -0600 Subject: [PATCH 5/9] [PM-28060] Remove Skeleton Feature Flag (#18456) * remove skeleton ff * remove unneeded templates --- .../popup/send-v2/send-v2.component.html | 2 +- .../popup/send-v2/send-v2.component.spec.ts | 2 - .../tools/popup/send-v2/send-v2.component.ts | 18 +-- .../vault-v2-search.component.spec.ts | 123 ++++++------------ .../vault-search/vault-v2-search.component.ts | 39 ++---- .../vault-v2/vault-v2.component.html | 95 ++++++-------- .../vault-v2/vault-v2.component.spec.ts | 2 + .../components/vault-v2/vault-v2.component.ts | 14 +- libs/common/src/enums/feature-flag.enum.ts | 2 - 9 files changed, 96 insertions(+), 201 deletions(-) diff --git a/apps/browser/src/tools/popup/send-v2/send-v2.component.html b/apps/browser/src/tools/popup/send-v2/send-v2.component.html index 47ecd7564dc..48295fda35d 100644 --- a/apps/browser/src/tools/popup/send-v2/send-v2.component.html +++ b/apps/browser/src/tools/popup/send-v2/send-v2.component.html @@ -1,4 +1,4 @@ - + diff --git a/apps/browser/src/tools/popup/send-v2/send-v2.component.spec.ts b/apps/browser/src/tools/popup/send-v2/send-v2.component.spec.ts index dfbfabf8d5e..dc4b935c6c8 100644 --- a/apps/browser/src/tools/popup/send-v2/send-v2.component.spec.ts +++ b/apps/browser/src/tools/popup/send-v2/send-v2.component.spec.ts @@ -11,7 +11,6 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AvatarService } from "@bitwarden/common/auth/abstractions/avatar.service"; import { BillingAccountProfileStateService } from "@bitwarden/common/billing/abstractions"; -import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; @@ -110,7 +109,6 @@ describe("SendV2Component", () => { provide: BillingAccountProfileStateService, useValue: { hasPremiumFromAnySource$: of(false) }, }, - { provide: ConfigService, useValue: mock() }, { provide: EnvironmentService, useValue: mock() }, { provide: LogService, useValue: mock() }, { provide: PlatformUtilsService, useValue: mock() }, diff --git a/apps/browser/src/tools/popup/send-v2/send-v2.component.ts b/apps/browser/src/tools/popup/send-v2/send-v2.component.ts index f36a475a805..8c1edee79dc 100644 --- a/apps/browser/src/tools/popup/send-v2/send-v2.component.ts +++ b/apps/browser/src/tools/popup/send-v2/send-v2.component.ts @@ -11,8 +11,6 @@ import { PolicyService } from "@bitwarden/common/admin-console/abstractions/poli import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { getUserId } from "@bitwarden/common/auth/services/account.service"; -import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; -import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { SendType } from "@bitwarden/common/tools/send/types/send-type"; import { PremiumUpgradePromptService } from "@bitwarden/common/vault/abstractions/premium-upgrade-prompt.service"; import { SearchService } from "@bitwarden/common/vault/abstractions/search.service"; @@ -84,30 +82,17 @@ export class SendV2Component implements OnDestroy { protected listState: SendState | null = null; protected sends$ = this.sendItemsService.filteredAndSortedSends$; - private skeletonFeatureFlag$ = this.configService.getFeatureFlag$( - FeatureFlag.VaultLoadingSkeletons, - ); protected sendsLoading$ = this.sendItemsService.loading$.pipe( distinctUntilChanged(), shareReplay({ bufferSize: 1, refCount: true }), ); - /** Spinner Loading State */ - protected showSpinnerLoaders$ = combineLatest([ - this.sendsLoading$, - this.skeletonFeatureFlag$, - ]).pipe(map(([loading, skeletonsEnabled]) => loading && !skeletonsEnabled)); - /** Skeleton Loading State */ protected showSkeletonsLoaders$ = combineLatest([ this.sendsLoading$, this.searchService.isSendSearching$, - this.skeletonFeatureFlag$, ]).pipe( - map( - ([loading, cipherSearching, skeletonsEnabled]) => - (loading || cipherSearching) && skeletonsEnabled, - ), + map(([loading, cipherSearching]) => loading || cipherSearching), distinctUntilChanged(), skeletonLoadingDelay(), ); @@ -128,7 +113,6 @@ export class SendV2Component implements OnDestroy { protected sendListFiltersService: SendListFiltersService, private policyService: PolicyService, private accountService: AccountService, - private configService: ConfigService, private searchService: SearchService, ) { combineLatest([ diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.spec.ts index 37c4804e600..ca73a7332ee 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.spec.ts @@ -4,7 +4,6 @@ import { FormsModule } from "@angular/forms"; import { BehaviorSubject } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; -import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { SearchTextDebounceInterval } from "@bitwarden/common/vault/services/search.service"; import { SearchModule } from "@bitwarden/components"; @@ -20,7 +19,6 @@ describe("VaultV2SearchComponent", () => { const searchText$ = new BehaviorSubject(""); const loading$ = new BehaviorSubject(false); - const featureFlag$ = new BehaviorSubject(true); const applyFilter = jest.fn(); const createComponent = () => { @@ -31,7 +29,6 @@ describe("VaultV2SearchComponent", () => { beforeEach(async () => { applyFilter.mockClear(); - featureFlag$.next(true); await TestBed.configureTestingModule({ imports: [VaultV2SearchComponent, CommonModule, SearchModule, JslibModule, FormsModule], @@ -49,12 +46,6 @@ describe("VaultV2SearchComponent", () => { loading$, }, }, - { - provide: ConfigService, - useValue: { - getFeatureFlag$: jest.fn(() => featureFlag$), - }, - }, { provide: I18nService, useValue: { t: (key: string) => key } }, ], }).compileComponents(); @@ -70,91 +61,55 @@ describe("VaultV2SearchComponent", () => { }); describe("debouncing behavior", () => { - describe("when feature flag is enabled", () => { - beforeEach(() => { - featureFlag$.next(true); - createComponent(); - }); - - it("debounces search text changes when not loading", fakeAsync(() => { - loading$.next(false); - - component.searchText = "test"; - component.onSearchTextChanged(); - - expect(applyFilter).not.toHaveBeenCalled(); - - tick(SearchTextDebounceInterval); - - expect(applyFilter).toHaveBeenCalledWith("test"); - expect(applyFilter).toHaveBeenCalledTimes(1); - })); - - it("should not debounce search text changes when loading", fakeAsync(() => { - loading$.next(true); - - component.searchText = "test"; - component.onSearchTextChanged(); - - tick(0); - - expect(applyFilter).toHaveBeenCalledWith("test"); - expect(applyFilter).toHaveBeenCalledTimes(1); - })); - - it("cancels previous debounce when new text is entered", fakeAsync(() => { - loading$.next(false); - - component.searchText = "test"; - component.onSearchTextChanged(); - - tick(SearchTextDebounceInterval / 2); - - component.searchText = "test2"; - component.onSearchTextChanged(); - - tick(SearchTextDebounceInterval / 2); - - expect(applyFilter).not.toHaveBeenCalled(); - - tick(SearchTextDebounceInterval / 2); - - expect(applyFilter).toHaveBeenCalledWith("test2"); - expect(applyFilter).toHaveBeenCalledTimes(1); - })); + beforeEach(() => { + createComponent(); }); - describe("when feature flag is disabled", () => { - beforeEach(() => { - featureFlag$.next(false); - createComponent(); - }); + it("debounces search text changes when not loading", fakeAsync(() => { + loading$.next(false); - it("debounces search text changes", fakeAsync(() => { - component.searchText = "test"; - component.onSearchTextChanged(); + component.searchText = "test"; + component.onSearchTextChanged(); - expect(applyFilter).not.toHaveBeenCalled(); + expect(applyFilter).not.toHaveBeenCalled(); - tick(SearchTextDebounceInterval); + tick(SearchTextDebounceInterval); - expect(applyFilter).toHaveBeenCalledWith("test"); - expect(applyFilter).toHaveBeenCalledTimes(1); - })); + expect(applyFilter).toHaveBeenCalledWith("test"); + expect(applyFilter).toHaveBeenCalledTimes(1); + })); - it("ignores loading state and always debounces", fakeAsync(() => { - loading$.next(true); + it("should not debounce search text changes when loading", fakeAsync(() => { + loading$.next(true); - component.searchText = "test"; - component.onSearchTextChanged(); + component.searchText = "test"; + component.onSearchTextChanged(); - expect(applyFilter).not.toHaveBeenCalled(); + tick(0); - tick(SearchTextDebounceInterval); + expect(applyFilter).toHaveBeenCalledWith("test"); + expect(applyFilter).toHaveBeenCalledTimes(1); + })); - expect(applyFilter).toHaveBeenCalledWith("test"); - expect(applyFilter).toHaveBeenCalledTimes(1); - })); - }); + it("cancels previous debounce when new text is entered", fakeAsync(() => { + loading$.next(false); + + component.searchText = "test"; + component.onSearchTextChanged(); + + tick(SearchTextDebounceInterval / 2); + + component.searchText = "test2"; + component.onSearchTextChanged(); + + tick(SearchTextDebounceInterval / 2); + + expect(applyFilter).not.toHaveBeenCalled(); + + tick(SearchTextDebounceInterval / 2); + + expect(applyFilter).toHaveBeenCalledWith("test2"); + expect(applyFilter).toHaveBeenCalledTimes(1); + })); }); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts index 154cd49c5a3..3419bd30ea0 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts @@ -7,17 +7,13 @@ import { Subscription, combineLatest, debounce, - debounceTime, distinctUntilChanged, filter, map, - switchMap, timer, } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; -import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; -import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { SearchTextDebounceInterval } from "@bitwarden/common/vault/services/search.service"; import { SearchModule } from "@bitwarden/components"; @@ -40,7 +36,6 @@ export class VaultV2SearchComponent { constructor( private vaultPopupItemsService: VaultPopupItemsService, private vaultPopupLoadingService: VaultPopupLoadingService, - private configService: ConfigService, private ngZone: NgZone, ) { this.subscribeToLatestSearchText(); @@ -63,31 +58,19 @@ export class VaultV2SearchComponent { } subscribeToApplyFilter(): void { - this.configService - .getFeatureFlag$(FeatureFlag.VaultLoadingSkeletons) + combineLatest([this.searchText$, this.loading$]) .pipe( - switchMap((enabled) => { - if (!enabled) { - return this.searchText$.pipe( - debounceTime(SearchTextDebounceInterval), - distinctUntilChanged(), - ); - } - - return combineLatest([this.searchText$, this.loading$]).pipe( - debounce(([_, isLoading]) => { - // If loading apply immediately to avoid stale searches. - // After loading completes, debounce to avoid excessive searches. - const delayTime = isLoading ? 0 : SearchTextDebounceInterval; - return timer(delayTime); - }), - distinctUntilChanged( - ([prevText, prevLoading], [newText, newLoading]) => - prevText === newText && prevLoading === newLoading, - ), - map(([text, _]) => text), - ); + debounce(([_, isLoading]) => { + // If loading apply immediately to avoid stale searches. + // After loading completes, debounce to avoid excessive searches. + const delayTime = isLoading ? 0 : SearchTextDebounceInterval; + return timer(delayTime); }), + distinctUntilChanged( + ([prevText, prevLoading], [newText, newLoading]) => + prevText === newText && prevLoading === newLoading, + ), + map(([text, _]) => text), takeUntilDestroyed(), ) .subscribe((text) => { diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html index 34454371f21..20871b4b134 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html @@ -1,4 +1,4 @@ - + @@ -8,37 +8,28 @@ - -
- - {{ "yourVaultIsEmpty" | i18n }} - -

- {{ "emptyVaultDescription" | i18n }} -

- - - {{ "newLogin" | i18n }} - - -
- - - @if (skeletonFeatureFlag$ | async) { - - + @if (vaultState === VaultStateEnum.Empty) { + +
+ + {{ "yourVaultIsEmpty" | i18n }} + +

+ {{ "emptyVaultDescription" | i18n }} +

+ + + {{ "newLogin" | i18n }} + + +
- } @else { - } - - - - - - - - - @if (skeletonFeatureFlag$ | async) { - - + @if (vaultState === null) { + + @if (!(loading$ | async)) { + + + + } - } @else { - } diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.spec.ts index 2c94d9c226b..e3b72c3319f 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.spec.ts @@ -1,6 +1,7 @@ import { ChangeDetectionStrategy, Component, input, NO_ERRORS_SCHEMA } from "@angular/core"; import { TestBed, fakeAsync, flush, tick } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; +import { provideNoopAnimations } from "@angular/platform-browser/animations"; import { ActivatedRoute, Router } from "@angular/router"; import { RouterTestingModule } from "@angular/router/testing"; import { mock } from "jest-mock-extended"; @@ -243,6 +244,7 @@ describe("VaultV2Component", () => { await TestBed.configureTestingModule({ imports: [VaultV2Component, RouterTestingModule], providers: [ + provideNoopAnimations(), { provide: VaultPopupItemsService, useValue: itemsSvc }, { provide: VaultPopupListFiltersService, useValue: filtersSvc }, { provide: VaultPopupScrollPositionService, useValue: scrollSvc }, diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts index 4678e2733eb..c58b7b20d2f 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts @@ -158,10 +158,6 @@ export class VaultV2Component implements OnInit, OnDestroy { }), ); - protected skeletonFeatureFlag$ = this.configService.getFeatureFlag$( - FeatureFlag.VaultLoadingSkeletons, - ); - protected premiumSpotlightFeatureFlag$ = this.configService.getFeatureFlag$( FeatureFlag.BrowserPremiumSpotlight, ); @@ -216,20 +212,14 @@ export class VaultV2Component implements OnInit, OnDestroy { PremiumUpgradeDialogComponent.open(this.dialogService); } - /** When true, show spinner loading state */ - protected showSpinnerLoaders$ = combineLatest([this.loading$, this.skeletonFeatureFlag$]).pipe( - map(([loading, skeletonsEnabled]) => loading && !skeletonsEnabled), - ); - /** When true, show skeleton loading state with debouncing to prevent flicker */ protected showSkeletonsLoaders$ = combineLatest([ this.loading$, this.searchService.isCipherSearching$, this.vaultItemsTransferService.transferInProgress$, - this.skeletonFeatureFlag$, ]).pipe( - map(([loading, cipherSearching, transferInProgress, skeletonsEnabled]) => { - return (loading || cipherSearching || transferInProgress) && skeletonsEnabled; + map(([loading, cipherSearching, transferInProgress]) => { + return loading || cipherSearching || transferInProgress; }), distinctUntilChanged(), skeletonLoadingDelay(), diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index 94656d48826..0086524a47f 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -65,7 +65,6 @@ export enum FeatureFlag { PM22134SdkCipherListView = "pm-22134-sdk-cipher-list-view", PM22136_SdkCipherEncryption = "pm-22136-sdk-cipher-encryption", CipherKeyEncryption = "cipher-key-encryption", - VaultLoadingSkeletons = "pm-25081-vault-skeleton-loaders", BrowserPremiumSpotlight = "pm-23384-browser-premium-spotlight", MigrateMyVaultToMyItems = "pm-20558-migrate-myvault-to-myitems", PM27632_SdkCipherCrudOperations = "pm-27632-cipher-crud-operations-to-sdk", @@ -129,7 +128,6 @@ export const DefaultFeatureFlagValue = { [FeatureFlag.PM19941MigrateCipherDomainToSdk]: FALSE, [FeatureFlag.PM22134SdkCipherListView]: FALSE, [FeatureFlag.PM22136_SdkCipherEncryption]: FALSE, - [FeatureFlag.VaultLoadingSkeletons]: FALSE, [FeatureFlag.BrowserPremiumSpotlight]: FALSE, [FeatureFlag.PM27632_SdkCipherCrudOperations]: FALSE, [FeatureFlag.MigrateMyVaultToMyItems]: FALSE, From 36b648f5d7ad62b5d3f40e1f72a724f7f85b9894 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Mon, 26 Jan 2026 20:25:23 +0000 Subject: [PATCH 6/9] [deps]: Update taiki-e/install-action action to v2.66.7 (#18570) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- .github/workflows/lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 81d79df569c..6a5f6774474 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -142,7 +142,7 @@ jobs: run: cargo +nightly udeps --workspace --all-features --all-targets - name: Install cargo-deny - uses: taiki-e/install-action@2e9d707ef49c9b094d45955b60c7e5c0dfedeb14 # v2.66.5 + uses: taiki-e/install-action@542cebaaed782771e619bd5609d97659d109c492 # v2.66.7 with: tool: cargo-deny@0.18.6 From e2fa296b042f3c433786a15a6c4a41909c194fc2 Mon Sep 17 00:00:00 2001 From: Todd Martin <106564991+trmartin4@users.noreply.github.com> Date: Mon, 26 Jan 2026 17:40:27 -0500 Subject: [PATCH 7/9] chore(deps): Added override for package-lock.json --- .github/CODEOWNERS | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index d1266a174e4..3884bfda063 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -84,6 +84,7 @@ apps/web/src/app/billing @bitwarden/team-billing-dev libs/angular/src/billing @bitwarden/team-billing-dev libs/common/src/billing @bitwarden/team-billing-dev libs/billing @bitwarden/team-billing-dev +libs/pricing @bitwarden/team-billing-dev bitwarden_license/bit-web/src/app/billing @bitwarden/team-billing-dev ## Platform team files ## @@ -227,7 +228,9 @@ apps/web/src/locales/en/messages.json **/tsconfig.json @bitwarden/team-platform-dev **/jest.config.js @bitwarden/team-platform-dev **/project.jsons @bitwarden/team-platform-dev -libs/pricing @bitwarden/team-billing-dev +# Platform override specifically for the package-lock.json in +# native-messaging-test-runner so that Platform can manage all lock file updates +apps/desktop/native-messaging-test-runner/package-lock.json @bitwarden/team-platform-dev # Claude related files .claude/ @bitwarden/team-ai-sme From 60c28dd182eb7cbdd73956eb19509976c1c875b5 Mon Sep 17 00:00:00 2001 From: Leslie Tilton <23057410+Banrion@users.noreply.github.com> Date: Mon, 26 Jan 2026 17:05:42 -0600 Subject: [PATCH 8/9] [PM-31203] Change Phishing Url Check to use a Cursor Based Search (#18561) * Initial changes to look at phishing indexeddb service and removal of obsolete compression code * Convert background update to rxjs format and trigger via subject. Update test cases * Added addUrls function to use instead of saveUrls so appending daily does not clear all urls * Added debug logs to phishing-indexeddb service * Added a fallback url when downloading phishing url list * Remove obsolete comments * Fix testUrl default, false scenario and test cases * Add default return on isPhishingWebAddress * Added log statement * Change hostname to href in hasUrl check * Save fallback response * Fix matching subpaths in links. Update test cases * Fix meta data updates storing last checked instead of last updated * Update QA phishing url to be normalized * Filter web addresses * Return previous meta to keep subscription alive * Change indexeddb lookup from loading all to cursor search * fix(phishing): improve performance and fix URL matching in phishing detection Problem: The cursor-based search takes ~25 seconds to scan the entire phishing database. For non-phishing URLs (99% of cases), this full scan runs to completion every time. Before these fixes, opening a new tab triggered this sequence: 1. chrome://newtab/ fires a phishing check 2. Sequential concatMap blocks while cursor scans all 500k+ URLs (~25 sec) 3. User pastes actual URL and hits enter 4. That URL's check waits in queue behind the chrome:// check 5. Total delay: ~50+ seconds for a simple "open tab, paste link" workflow Even for legitimate phishing checks, the cursor search could take up to 25 seconds per URL when the fast hasUrl lookup misses due to trailing slash mismatches. Changes: phishing-data.service.ts: - Add protocol filter to early-return for non-http(s) URLs, avoiding expensive IndexedDB operations for chrome://, about:, file:// URLs - Add trailing slash normalization for hasUrl lookup - browsers add trailing slashes but DB entries may not have them, causing O(1) lookups to miss and fall back to O(n) cursor search unnecessarily - Add debug logging for hasUrl checks and timing metrics for cursor-based search to aid performance debugging phishing-detection.service.ts: - Replace concatMap with mergeMap for parallel tab processing - each tab check now runs independently instead of sequentially - Add concurrency limit of 5 to prevent overwhelming IndexedDB while still allowing parallel execution Result: - New tabs are instant (no IndexedDB calls for non-web URLs) - One slow phishing check doesn't block other tabs - Common URL patterns hit the fast O(1) path instead of O(n) cursor scan * performance debug logs * disable custom match because too slow * spec fix --------- Co-authored-by: Alex --- .../phishing-detection/phishing-resources.ts | 4 + .../services/phishing-data.service.spec.ts | 42 ++++------ .../services/phishing-data.service.ts | 73 ++++++++++++++-- .../services/phishing-detection.service.ts | 54 ++++++++---- .../phishing-indexeddb.service.spec.ts | 83 +++++++++++++++++++ .../services/phishing-indexeddb.service.ts | 54 ++++++++++++ 6 files changed, 259 insertions(+), 51 deletions(-) diff --git a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts index 88068987dd7..6595104207a 100644 --- a/apps/browser/src/dirt/phishing-detection/phishing-resources.ts +++ b/apps/browser/src/dirt/phishing-detection/phishing-resources.ts @@ -7,6 +7,8 @@ export type PhishingResource = { todayUrl: string; /** Matcher used to decide whether a given URL matches an entry from this resource */ match: (url: URL, entry: string) => boolean; + /** Whether to use the custom matcher. If false, only exact hasUrl lookups are used. Default: true */ + useCustomMatcher?: boolean; }; export const PhishingResourceType = Object.freeze({ @@ -56,6 +58,8 @@ export const PHISHING_RESOURCES: Record { if (!entry) { return false; diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts index d633c0612f5..2d6c7a5a651 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.spec.ts @@ -40,6 +40,7 @@ describe("PhishingDataService", () => { // Set default mock behaviors mockIndexedDbService.hasUrl.mockResolvedValue(false); mockIndexedDbService.loadAllUrls.mockResolvedValue([]); + mockIndexedDbService.findMatchingUrl.mockResolvedValue(false); mockIndexedDbService.saveUrls.mockResolvedValue(undefined); mockIndexedDbService.addUrls.mockResolvedValue(undefined); mockIndexedDbService.saveUrlsFromStream.mockResolvedValue(undefined); @@ -90,7 +91,7 @@ describe("PhishingDataService", () => { it("should NOT detect QA test addresses - different subpath", async () => { mockIndexedDbService.hasUrl.mockResolvedValue(false); - mockIndexedDbService.loadAllUrls.mockResolvedValue([]); + mockIndexedDbService.findMatchingUrl.mockResolvedValue(false); const url = new URL("https://phishing.testcategory.com/other"); const result = await service.isPhishingWebAddress(url); @@ -120,70 +121,65 @@ describe("PhishingDataService", () => { expect(result).toBe(true); expect(mockIndexedDbService.hasUrl).toHaveBeenCalledWith("http://phish.com/testing-param"); // Should not fall back to custom matcher when hasUrl returns true - expect(mockIndexedDbService.loadAllUrls).not.toHaveBeenCalled(); + expect(mockIndexedDbService.findMatchingUrl).not.toHaveBeenCalled(); }); - it("should fall back to custom matcher when hasUrl returns false", async () => { + it("should return false when hasUrl returns false (custom matcher disabled)", async () => { // Mock hasUrl to return false (no direct href match) mockIndexedDbService.hasUrl.mockResolvedValue(false); - // Mock loadAllUrls to return phishing URLs for custom matcher - mockIndexedDbService.loadAllUrls.mockResolvedValue(["http://phish.com/path"]); const url = new URL("http://phish.com/path"); const result = await service.isPhishingWebAddress(url); - expect(result).toBe(true); + // Custom matcher is currently disabled (useCustomMatcher: false), so result is false + expect(result).toBe(false); expect(mockIndexedDbService.hasUrl).toHaveBeenCalledWith("http://phish.com/path"); - expect(mockIndexedDbService.loadAllUrls).toHaveBeenCalled(); + // Custom matcher should NOT be called since it's disabled + expect(mockIndexedDbService.findMatchingUrl).not.toHaveBeenCalled(); }); it("should not detect a safe web address", async () => { // Mock hasUrl to return false mockIndexedDbService.hasUrl.mockResolvedValue(false); - // Mock loadAllUrls to return phishing URLs that don't match - mockIndexedDbService.loadAllUrls.mockResolvedValue(["http://phish.com", "http://badguy.net"]); const url = new URL("http://safe.com"); const result = await service.isPhishingWebAddress(url); expect(result).toBe(false); expect(mockIndexedDbService.hasUrl).toHaveBeenCalledWith("http://safe.com/"); - expect(mockIndexedDbService.loadAllUrls).toHaveBeenCalled(); + // Custom matcher is disabled, so findMatchingUrl should NOT be called + expect(mockIndexedDbService.findMatchingUrl).not.toHaveBeenCalled(); }); - it("should not match against root web address with subpaths using custom matcher", async () => { + it("should not match against root web address with subpaths (custom matcher disabled)", async () => { // Mock hasUrl to return false (no direct href match) mockIndexedDbService.hasUrl.mockResolvedValue(false); - // Mock loadAllUrls to return entry that matches with subpath - mockIndexedDbService.loadAllUrls.mockResolvedValue(["http://phish.com/login"]); const url = new URL("http://phish.com/login/page"); const result = await service.isPhishingWebAddress(url); expect(result).toBe(false); expect(mockIndexedDbService.hasUrl).toHaveBeenCalledWith("http://phish.com/login/page"); - expect(mockIndexedDbService.loadAllUrls).toHaveBeenCalled(); + // Custom matcher is disabled, so findMatchingUrl should NOT be called + expect(mockIndexedDbService.findMatchingUrl).not.toHaveBeenCalled(); }); - it("should not match against root web address with different subpaths using custom matcher", async () => { + it("should not match against root web address with different subpaths (custom matcher disabled)", async () => { // Mock hasUrl to return false (no direct hostname match) mockIndexedDbService.hasUrl.mockResolvedValue(false); - // Mock loadAllUrls to return entry that matches with subpath - mockIndexedDbService.loadAllUrls.mockResolvedValue(["http://phish.com/login/page1"]); const url = new URL("http://phish.com/login/page2"); const result = await service.isPhishingWebAddress(url); expect(result).toBe(false); expect(mockIndexedDbService.hasUrl).toHaveBeenCalledWith("http://phish.com/login/page2"); - expect(mockIndexedDbService.loadAllUrls).toHaveBeenCalled(); + // Custom matcher is disabled, so findMatchingUrl should NOT be called + expect(mockIndexedDbService.findMatchingUrl).not.toHaveBeenCalled(); }); it("should handle IndexedDB errors gracefully", async () => { // Mock hasUrl to throw error mockIndexedDbService.hasUrl.mockRejectedValue(new Error("hasUrl error")); - // Mock loadAllUrls to also throw error - mockIndexedDbService.loadAllUrls.mockRejectedValue(new Error("IndexedDB error")); const url = new URL("http://phish.com/about"); const result = await service.isPhishingWebAddress(url); @@ -193,10 +189,8 @@ describe("PhishingDataService", () => { "[PhishingDataService] IndexedDB lookup via hasUrl failed", expect.any(Error), ); - expect(logService.error).toHaveBeenCalledWith( - "[PhishingDataService] Error running custom matcher", - expect.any(Error), - ); + // Custom matcher is disabled, so no custom matcher error is expected + expect(mockIndexedDbService.findMatchingUrl).not.toHaveBeenCalled(); }); }); diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts index 10268fa7f93..c34a94ecced 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts @@ -153,8 +153,18 @@ export class PhishingDataService { * @returns True if the URL is a known phishing web address, false otherwise */ async isPhishingWebAddress(url: URL): Promise { + this.logService.debug("[PhishingDataService] isPhishingWebAddress called for: " + url.href); + + // Skip non-http(s) protocols - phishing database only contains web URLs + // This prevents expensive fallback checks for chrome://, about:, file://, etc. + if (url.protocol !== "http:" && url.protocol !== "https:") { + this.logService.debug("[PhishingDataService] Skipping non-http(s) protocol: " + url.protocol); + return false; + } + // Quick check for QA/dev test addresses if (this._testWebAddresses.includes(url.href)) { + this.logService.info("[PhishingDataService] Found test web address: " + url.href); return true; } @@ -162,28 +172,73 @@ export class PhishingDataService { try { // Quick lookup: check direct presence of href in IndexedDB - const hasUrl = await this.indexedDbService.hasUrl(url.href); + // Also check without trailing slash since browsers add it but DB entries may not have it + const urlHref = url.href; + const urlWithoutTrailingSlash = urlHref.endsWith("/") ? urlHref.slice(0, -1) : null; + + this.logService.debug("[PhishingDataService] Checking hasUrl on this string: " + urlHref); + let hasUrl = await this.indexedDbService.hasUrl(urlHref); + + // If not found and URL has trailing slash, try without it + if (!hasUrl && urlWithoutTrailingSlash) { + this.logService.debug( + "[PhishingDataService] Checking hasUrl without trailing slash: " + + urlWithoutTrailingSlash, + ); + hasUrl = await this.indexedDbService.hasUrl(urlWithoutTrailingSlash); + } + if (hasUrl) { + this.logService.info( + "[PhishingDataService] Found phishing web address through direct lookup: " + urlHref, + ); return true; } } catch (err) { this.logService.error("[PhishingDataService] IndexedDB lookup via hasUrl failed", err); } - // If a custom matcher is provided, iterate stored entries and apply the matcher. - if (resource && resource.match) { + // If a custom matcher is provided and enabled, use cursor-based search. + // This avoids loading all URLs into memory and allows early exit on first match. + // Can be disabled via useCustomMatcher: false for performance reasons. + if (resource && resource.match && resource.useCustomMatcher !== false) { try { - const entries = await this.indexedDbService.loadAllUrls(); - for (const entry of entries) { - if (resource.match(url, entry)) { - return true; - } + this.logService.debug( + "[PhishingDataService] Starting cursor-based search for: " + url.href, + ); + const startTime = performance.now(); + + const found = await this.indexedDbService.findMatchingUrl((entry) => + resource.match(url, entry), + ); + + const endTime = performance.now(); + const duration = (endTime - startTime).toFixed(2); + this.logService.debug( + `[PhishingDataService] Cursor-based search completed in ${duration}ms for: ${url.href} (found: ${found})`, + ); + + if (found) { + this.logService.info( + "[PhishingDataService] Found phishing web address through custom matcher: " + url.href, + ); + } else { + this.logService.debug( + "[PhishingDataService] No match found, returning false for: " + url.href, + ); } + return found; } catch (err) { this.logService.error("[PhishingDataService] Error running custom matcher", err); + this.logService.debug( + "[PhishingDataService] Returning false due to error for: " + url.href, + ); + return false; } - return false; } + this.logService.debug( + "[PhishingDataService] No custom matcher, returning false for: " + url.href, + ); return false; } diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts index 815007e1d4c..6ca5bad8942 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts @@ -1,10 +1,10 @@ import { - concatMap, distinctUntilChanged, EMPTY, filter, map, merge, + mergeMap, Subject, switchMap, tap, @@ -43,6 +43,7 @@ export class PhishingDetectionService { private static _tabUpdated$ = new Subject(); private static _ignoredHostnames = new Set(); private static _didInit = false; + private static _activeSearchCount = 0; static initialize( logService: LogService, @@ -63,7 +64,7 @@ export class PhishingDetectionService { tap((message) => logService.debug(`[PhishingDetectionService] user selected continue for ${message.url}`), ), - concatMap(async (message) => { + mergeMap(async (message) => { const url = new URL(message.url); this._ignoredHostnames.add(url.hostname); await BrowserApi.navigateTabToUrl(message.tabId, url); @@ -88,23 +89,40 @@ export class PhishingDetectionService { prev.ignored === curr.ignored, ), tap((event) => logService.debug(`[PhishingDetectionService] processing event:`, event)), - concatMap(async ({ tabId, url, ignored }) => { - if (ignored) { - // The next time this host is visited, block again - this._ignoredHostnames.delete(url.hostname); - return; - } - const isPhishing = await phishingDataService.isPhishingWebAddress(url); - if (!isPhishing) { - return; - } - - const phishingWarningPage = new URL( - BrowserApi.getRuntimeURL("popup/index.html#/security/phishing-warning") + - `?phishingUrl=${url.toString()}`, + // Use mergeMap for parallel processing - each tab check runs independently + // Concurrency limit of 5 prevents overwhelming IndexedDB + mergeMap(async ({ tabId, url, ignored }) => { + this._activeSearchCount++; + const searchId = `${tabId}-${Date.now()}`; + logService.debug( + `[PhishingDetectionService] Search STARTED [${searchId}] for ${url.href} (active: ${this._activeSearchCount}/5)`, ); - await BrowserApi.navigateTabToUrl(tabId, phishingWarningPage); - }), + const startTime = performance.now(); + + try { + if (ignored) { + // The next time this host is visited, block again + this._ignoredHostnames.delete(url.hostname); + return; + } + const isPhishing = await phishingDataService.isPhishingWebAddress(url); + if (!isPhishing) { + return; + } + + const phishingWarningPage = new URL( + BrowserApi.getRuntimeURL("popup/index.html#/security/phishing-warning") + + `?phishingUrl=${url.toString()}`, + ); + await BrowserApi.navigateTabToUrl(tabId, phishingWarningPage); + } finally { + this._activeSearchCount--; + const duration = (performance.now() - startTime).toFixed(2); + logService.debug( + `[PhishingDetectionService] Search FINISHED [${searchId}] for ${url.href} in ${duration}ms (active: ${this._activeSearchCount}/5)`, + ); + } + }, 5), ); const onCancelCommand$ = messageListener diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.spec.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.spec.ts index 99e101cc199..98835a5b366 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.spec.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.spec.ts @@ -435,6 +435,89 @@ describe("PhishingIndexedDbService", () => { }); }); + describe("findMatchingUrl", () => { + it("returns true when matcher finds a match", async () => { + mockStore.set("https://example.com", { url: "https://example.com" }); + mockStore.set("https://phishing.net", { url: "https://phishing.net" }); + mockStore.set("https://test.org", { url: "https://test.org" }); + + const matcher = (url: string) => url.includes("phishing"); + const result = await service.findMatchingUrl(matcher); + + expect(result).toBe(true); + expect(mockDb.transaction).toHaveBeenCalledWith("phishing-urls", "readonly"); + expect(mockObjectStore.openCursor).toHaveBeenCalled(); + }); + + it("returns false when no URLs match", async () => { + mockStore.set("https://example.com", { url: "https://example.com" }); + mockStore.set("https://test.org", { url: "https://test.org" }); + + const matcher = (url: string) => url.includes("notfound"); + const result = await service.findMatchingUrl(matcher); + + expect(result).toBe(false); + }); + + it("returns false when store is empty", async () => { + const matcher = (url: string) => url.includes("anything"); + const result = await service.findMatchingUrl(matcher); + + expect(result).toBe(false); + }); + + it("exits early on first match without iterating all records", async () => { + mockStore.set("https://match1.com", { url: "https://match1.com" }); + mockStore.set("https://match2.com", { url: "https://match2.com" }); + mockStore.set("https://match3.com", { url: "https://match3.com" }); + + const matcherCallCount = jest + .fn() + .mockImplementation((url: string) => url.includes("match2")); + await service.findMatchingUrl(matcherCallCount); + + // Matcher should be called for match1.com and match2.com, but NOT match3.com + // because it exits early on first match + expect(matcherCallCount).toHaveBeenCalledWith("https://match1.com"); + expect(matcherCallCount).toHaveBeenCalledWith("https://match2.com"); + expect(matcherCallCount).not.toHaveBeenCalledWith("https://match3.com"); + expect(matcherCallCount).toHaveBeenCalledTimes(2); + }); + + it("supports complex matcher logic", async () => { + mockStore.set("https://example.com/path", { url: "https://example.com/path" }); + mockStore.set("https://test.org", { url: "https://test.org" }); + mockStore.set("https://phishing.net/login", { url: "https://phishing.net/login" }); + + const matcher = (url: string) => { + return url.includes("phishing") && url.includes("login"); + }; + const result = await service.findMatchingUrl(matcher); + + expect(result).toBe(true); + }); + + it("returns false on error", async () => { + const error = new Error("IndexedDB error"); + mockOpenRequest.error = error; + (global.indexedDB.open as jest.Mock).mockImplementation(() => { + setTimeout(() => { + mockOpenRequest.onerror?.(); + }, 0); + return mockOpenRequest; + }); + + const matcher = (url: string) => url.includes("test"); + const result = await service.findMatchingUrl(matcher); + + expect(result).toBe(false); + expect(logService.error).toHaveBeenCalledWith( + "[PhishingIndexedDbService] Cursor search failed", + expect.any(Error), + ); + }); + }); + describe("database initialization", () => { it("creates object store with keyPath on upgrade", async () => { mockDb.objectStoreNames.contains.mockReturnValue(false); diff --git a/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.ts b/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.ts index fe0f10da221..ea4b7987607 100644 --- a/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.ts +++ b/apps/browser/src/dirt/phishing-detection/services/phishing-indexeddb.service.ts @@ -195,6 +195,60 @@ export class PhishingIndexedDbService { }); } + /** + * Checks if any URL in the database matches the given matcher function. + * Uses a cursor to iterate through records without loading all into memory. + * Returns immediately on first match for optimal performance. + * + * @param matcher - Function that tests each URL and returns true if it matches + * @returns `true` if any URL matches, `false` if none match or on error + */ + async findMatchingUrl(matcher: (url: string) => boolean): Promise { + this.logService.debug("[PhishingIndexedDbService] Searching for matching URL with cursor..."); + + let db: IDBDatabase | null = null; + try { + db = await this.openDatabase(); + return await this.cursorSearch(db, matcher); + } catch (error) { + this.logService.error("[PhishingIndexedDbService] Cursor search failed", error); + return false; + } finally { + db?.close(); + } + } + + /** + * Performs cursor-based search through all URLs. + * Tests each URL with the matcher without accumulating records in memory. + */ + private cursorSearch(db: IDBDatabase, matcher: (url: string) => boolean): Promise { + return new Promise((resolve, reject) => { + const req = db + .transaction(this.STORE_NAME, "readonly") + .objectStore(this.STORE_NAME) + .openCursor(); + req.onerror = () => reject(req.error); + req.onsuccess = (e) => { + const cursor = (e.target as IDBRequest).result; + if (cursor) { + const url = (cursor.value as PhishingUrlRecord).url; + // Test the URL immediately without accumulating in memory + if (matcher(url)) { + // Found a match + resolve(true); + return; + } + // No match, continue to next record + cursor.continue(); + } else { + // Reached end of records without finding a match + resolve(false); + } + }; + }); + } + /** * Saves phishing URLs directly from a stream. * Processes data incrementally to minimize memory usage. From 748c7c544624eb6154c5318c048c1e196b397dc1 Mon Sep 17 00:00:00 2001 From: Nik Gilmore Date: Mon, 26 Jan 2026 15:55:49 -0800 Subject: [PATCH 9/9] [PM-30303] Migrate Cipher Delete Operations to use SDK (#18275) --- .../bulk-delete-dialog.component.ts | 12 +- .../vault/abstractions/cipher-sdk.service.ts | 74 ++++- .../src/vault/abstractions/cipher.service.ts | 26 +- .../vault/services/cipher-sdk.service.spec.ts | 288 ++++++++++++++++++ .../src/vault/services/cipher-sdk.service.ts | 185 ++++++++++- .../src/vault/services/cipher.service.spec.ts | 254 ++++++++++++++- .../src/vault/services/cipher.service.ts | 79 ++++- 7 files changed, 880 insertions(+), 38 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 46f2b5da735..9fcb6f0cec1 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 @@ -12,7 +12,6 @@ import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { CollectionId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; -import { CipherBulkDeleteRequest } from "@bitwarden/common/vault/models/request/cipher-bulk-delete.request"; import { UnionOfValues } from "@bitwarden/common/vault/types/union-of-values"; import { CenterPositionStrategy, @@ -148,11 +147,16 @@ export class BulkDeleteDialogComponent { } private async deleteCiphersAdmin(ciphers: string[]): Promise { - const deleteRequest = new CipherBulkDeleteRequest(ciphers, this.organization.id); + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); if (this.permanent) { - return await this.apiService.deleteManyCiphersAdmin(deleteRequest); + await this.cipherService.deleteManyWithServer(ciphers, userId, true, this.organization.id); } else { - return await this.apiService.putDeleteManyCiphersAdmin(deleteRequest); + await this.cipherService.softDeleteManyWithServer( + ciphers, + userId, + true, + this.organization.id, + ); } } diff --git a/libs/common/src/vault/abstractions/cipher-sdk.service.ts b/libs/common/src/vault/abstractions/cipher-sdk.service.ts index 1037bfc2b92..3101531eda6 100644 --- a/libs/common/src/vault/abstractions/cipher-sdk.service.ts +++ b/libs/common/src/vault/abstractions/cipher-sdk.service.ts @@ -1,4 +1,4 @@ -import { UserId } from "@bitwarden/common/types/guid"; +import { OrganizationId, UserId } from "@bitwarden/common/types/guid"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; /** @@ -34,4 +34,76 @@ export abstract class CipherSdkService { originalCipherView?: CipherView, orgAdmin?: boolean, ): Promise; + + /** + * Deletes a cipher on the server using the SDK. + * + * @param id The cipher ID to delete + * @param userId The user ID to use for SDK client + * @param asAdmin Whether this is an organization admin operation + * @returns A promise that resolves when the cipher is deleted + */ + abstract deleteWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; + + /** + * Deletes multiple ciphers on the server using the SDK. + * + * @param ids The cipher IDs to delete + * @param userId The user ID to use for SDK client + * @param asAdmin Whether this is an organization admin operation + * @param orgId The organization ID (required when asAdmin is true) + * @returns A promise that resolves when the ciphers are deleted + */ + abstract deleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin?: boolean, + orgId?: OrganizationId, + ): Promise; + + /** + * Soft deletes a cipher on the server using the SDK. + * + * @param id The cipher ID to soft delete + * @param userId The user ID to use for SDK client + * @param asAdmin Whether this is an organization admin operation + * @returns A promise that resolves when the cipher is soft deleted + */ + abstract softDeleteWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; + + /** + * Soft deletes multiple ciphers on the server using the SDK. + * + * @param ids The cipher IDs to soft delete + * @param userId The user ID to use for SDK client + * @param asAdmin Whether this is an organization admin operation + * @param orgId The organization ID (required when asAdmin is true) + * @returns A promise that resolves when the ciphers are soft deleted + */ + abstract softDeleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin?: boolean, + orgId?: OrganizationId, + ): Promise; + + /** + * Restores a soft-deleted cipher on the server using the SDK. + * + * @param id The cipher ID to restore + * @param userId The user ID to use for SDK client + * @param asAdmin Whether this is an organization admin operation + * @returns A promise that resolves when the cipher is restored + */ + abstract restoreWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; + + /** + * Restores multiple soft-deleted ciphers on the server using the SDK. + * + * @param ids The cipher IDs to restore + * @param userId The user ID to use for SDK client + * @param orgId The organization ID (determines whether to use admin API) + * @returns A promise that resolves when the ciphers are restored + */ + abstract restoreManyWithServer(ids: string[], userId: UserId, orgId?: string): Promise; } diff --git a/libs/common/src/vault/abstractions/cipher.service.ts b/libs/common/src/vault/abstractions/cipher.service.ts index 1db5f8d38a7..4b544b2a34e 100644 --- a/libs/common/src/vault/abstractions/cipher.service.ts +++ b/libs/common/src/vault/abstractions/cipher.service.ts @@ -230,8 +230,13 @@ export abstract class CipherService implements UserKeyRotationDataProvider; abstract moveManyWithServer(ids: string[], folderId: string, userId: UserId): Promise; abstract delete(id: string | string[], userId: UserId): Promise; - abstract deleteWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; - abstract deleteManyWithServer(ids: string[], userId: UserId, asAdmin?: boolean): Promise; + abstract deleteWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; + abstract deleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin?: boolean, + orgId?: OrganizationId, + ): Promise; abstract deleteAttachment( id: string, revisionDate: string, @@ -247,14 +252,19 @@ export abstract class CipherService implements UserKeyRotationDataProvider number; - abstract softDelete(id: string | string[], userId: UserId): Promise; - abstract softDeleteWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; - abstract softDeleteManyWithServer(ids: string[], userId: UserId, asAdmin?: boolean): Promise; + abstract softDelete(id: string | string[], userId: UserId): Promise; + abstract softDeleteWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; + abstract softDeleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin?: boolean, + orgId?: OrganizationId, + ): Promise; abstract restore( cipher: { id: string; revisionDate: string } | { id: string; revisionDate: string }[], userId: UserId, - ): Promise; - abstract restoreWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; + ): Promise; + abstract restoreWithServer(id: string, userId: UserId, asAdmin?: boolean): Promise; abstract restoreManyWithServer(ids: string[], userId: UserId, orgId?: string): Promise; abstract getKeyForCipherKeyDecryption(cipher: Cipher, userId: UserId): Promise; abstract setAddEditCipherInfo(value: AddEditCipherInfo, userId: UserId): Promise; @@ -275,7 +285,7 @@ export abstract class CipherService implements UserKeyRotationDataProvider; /** - * Decrypts a cipher using either the SDK or the legacy method based on the feature flag. + * Decrypts a cipher using either the use-sdk-cipheroperationsSDK or the legacy method based on the feature flag. * @param cipher The cipher to decrypt. * @param userId The user ID to use for decryption. * @returns A promise that resolves to the decrypted cipher view. diff --git a/libs/common/src/vault/services/cipher-sdk.service.spec.ts b/libs/common/src/vault/services/cipher-sdk.service.spec.ts index bd3feb4619e..cb21ff28133 100644 --- a/libs/common/src/vault/services/cipher-sdk.service.spec.ts +++ b/libs/common/src/vault/services/cipher-sdk.service.spec.ts @@ -28,10 +28,22 @@ describe("DefaultCipherSdkService", () => { mockAdminSdk = { create: jest.fn(), edit: jest.fn(), + delete: jest.fn().mockResolvedValue(undefined), + delete_many: jest.fn().mockResolvedValue(undefined), + soft_delete: jest.fn().mockResolvedValue(undefined), + soft_delete_many: jest.fn().mockResolvedValue(undefined), + restore: jest.fn().mockResolvedValue(undefined), + restore_many: jest.fn().mockResolvedValue(undefined), }; mockCiphersSdk = { create: jest.fn(), edit: jest.fn(), + delete: jest.fn().mockResolvedValue(undefined), + delete_many: jest.fn().mockResolvedValue(undefined), + soft_delete: jest.fn().mockResolvedValue(undefined), + soft_delete_many: jest.fn().mockResolvedValue(undefined), + restore: jest.fn().mockResolvedValue(undefined), + restore_many: jest.fn().mockResolvedValue(undefined), admin: jest.fn().mockReturnValue(mockAdminSdk), }; mockVaultSdk = { @@ -243,4 +255,280 @@ describe("DefaultCipherSdkService", () => { ); }); }); + + describe("deleteWithServer()", () => { + const testCipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; + + it("should delete cipher using SDK when asAdmin is false", async () => { + await cipherSdkService.deleteWithServer(testCipherId, userId, false); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.delete).toHaveBeenCalledWith(testCipherId); + expect(mockCiphersSdk.admin).not.toHaveBeenCalled(); + }); + + it("should delete cipher using SDK admin API when asAdmin is true", async () => { + await cipherSdkService.deleteWithServer(testCipherId, userId, true); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.delete).toHaveBeenCalledWith(testCipherId); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + + await expect(cipherSdkService.deleteWithServer(testCipherId, userId)).rejects.toThrow( + "SDK not available", + ); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to delete cipher"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + mockCiphersSdk.delete.mockRejectedValue(new Error("SDK error")); + + await expect(cipherSdkService.deleteWithServer(testCipherId, userId)).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to delete cipher"), + ); + }); + }); + + describe("deleteManyWithServer()", () => { + const testCipherIds = [ + "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId, + "6ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b23" as CipherId, + ]; + + it("should delete multiple ciphers using SDK when asAdmin is false", async () => { + await cipherSdkService.deleteManyWithServer(testCipherIds, userId, false); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.delete_many).toHaveBeenCalledWith(testCipherIds); + expect(mockCiphersSdk.admin).not.toHaveBeenCalled(); + }); + + it("should delete multiple ciphers using SDK admin API when asAdmin is true", async () => { + await cipherSdkService.deleteManyWithServer(testCipherIds, userId, true, orgId); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.delete_many).toHaveBeenCalledWith(testCipherIds, orgId); + }); + + it("should throw error when asAdmin is true but orgId is missing", async () => { + await expect( + cipherSdkService.deleteManyWithServer(testCipherIds, userId, true, undefined), + ).rejects.toThrow("Organization ID is required for admin delete."); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + + await expect(cipherSdkService.deleteManyWithServer(testCipherIds, userId)).rejects.toThrow( + "SDK not available", + ); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to delete multiple ciphers"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + mockCiphersSdk.delete_many.mockRejectedValue(new Error("SDK error")); + + await expect(cipherSdkService.deleteManyWithServer(testCipherIds, userId)).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to delete multiple ciphers"), + ); + }); + }); + + describe("softDeleteWithServer()", () => { + const testCipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; + + it("should soft delete cipher using SDK when asAdmin is false", async () => { + await cipherSdkService.softDeleteWithServer(testCipherId, userId, false); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.soft_delete).toHaveBeenCalledWith(testCipherId); + expect(mockCiphersSdk.admin).not.toHaveBeenCalled(); + }); + + it("should soft delete cipher using SDK admin API when asAdmin is true", async () => { + await cipherSdkService.softDeleteWithServer(testCipherId, userId, true); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.soft_delete).toHaveBeenCalledWith(testCipherId); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + + await expect(cipherSdkService.softDeleteWithServer(testCipherId, userId)).rejects.toThrow( + "SDK not available", + ); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to soft delete cipher"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + mockCiphersSdk.soft_delete.mockRejectedValue(new Error("SDK error")); + + await expect(cipherSdkService.softDeleteWithServer(testCipherId, userId)).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to soft delete cipher"), + ); + }); + }); + + describe("softDeleteManyWithServer()", () => { + const testCipherIds = [ + "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId, + "6ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b23" as CipherId, + ]; + + it("should soft delete multiple ciphers using SDK when asAdmin is false", async () => { + await cipherSdkService.softDeleteManyWithServer(testCipherIds, userId, false); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.soft_delete_many).toHaveBeenCalledWith(testCipherIds); + expect(mockCiphersSdk.admin).not.toHaveBeenCalled(); + }); + + it("should soft delete multiple ciphers using SDK admin API when asAdmin is true", async () => { + await cipherSdkService.softDeleteManyWithServer(testCipherIds, userId, true, orgId); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.soft_delete_many).toHaveBeenCalledWith(testCipherIds, orgId); + }); + + it("should throw error when asAdmin is true but orgId is missing", async () => { + await expect( + cipherSdkService.softDeleteManyWithServer(testCipherIds, userId, true, undefined), + ).rejects.toThrow("Organization ID is required for admin soft delete."); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + + await expect( + cipherSdkService.softDeleteManyWithServer(testCipherIds, userId), + ).rejects.toThrow("SDK not available"); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to soft delete multiple ciphers"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + mockCiphersSdk.soft_delete_many.mockRejectedValue(new Error("SDK error")); + + await expect( + cipherSdkService.softDeleteManyWithServer(testCipherIds, userId), + ).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to soft delete multiple ciphers"), + ); + }); + }); + + describe("restoreWithServer()", () => { + const testCipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; + + it("should restore cipher using SDK when asAdmin is false", async () => { + await cipherSdkService.restoreWithServer(testCipherId, userId, false); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.restore).toHaveBeenCalledWith(testCipherId); + expect(mockCiphersSdk.admin).not.toHaveBeenCalled(); + }); + + it("should restore cipher using SDK admin API when asAdmin is true", async () => { + await cipherSdkService.restoreWithServer(testCipherId, userId, true); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.restore).toHaveBeenCalledWith(testCipherId); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + + await expect(cipherSdkService.restoreWithServer(testCipherId, userId)).rejects.toThrow( + "SDK not available", + ); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to restore cipher"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + mockCiphersSdk.restore.mockRejectedValue(new Error("SDK error")); + + await expect(cipherSdkService.restoreWithServer(testCipherId, userId)).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to restore cipher"), + ); + }); + }); + + describe("restoreManyWithServer()", () => { + const testCipherIds = [ + "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId, + "6ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b23" as CipherId, + ]; + + it("should restore multiple ciphers using SDK when orgId is not provided", async () => { + await cipherSdkService.restoreManyWithServer(testCipherIds, userId); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.restore_many).toHaveBeenCalledWith(testCipherIds); + expect(mockCiphersSdk.admin).not.toHaveBeenCalled(); + }); + + it("should restore multiple ciphers using SDK admin API when orgId is provided", async () => { + const orgIdString = orgId as string; + await cipherSdkService.restoreManyWithServer(testCipherIds, userId, orgIdString); + + expect(sdkService.userClient$).toHaveBeenCalledWith(userId); + expect(mockVaultSdk.ciphers).toHaveBeenCalled(); + expect(mockCiphersSdk.admin).toHaveBeenCalled(); + expect(mockAdminSdk.restore_many).toHaveBeenCalledWith(testCipherIds, orgIdString); + }); + + it("should throw error and log when SDK client is not available", async () => { + sdkService.userClient$.mockReturnValue(of(null)); + + await expect(cipherSdkService.restoreManyWithServer(testCipherIds, userId)).rejects.toThrow( + "SDK not available", + ); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to restore multiple ciphers"), + ); + }); + + it("should throw error and log when SDK throws an error", async () => { + mockCiphersSdk.restore_many.mockRejectedValue(new Error("SDK error")); + + await expect(cipherSdkService.restoreManyWithServer(testCipherIds, userId)).rejects.toThrow(); + expect(logService.error).toHaveBeenCalledWith( + expect.stringContaining("Failed to restore multiple ciphers"), + ); + }); + }); }); diff --git a/libs/common/src/vault/services/cipher-sdk.service.ts b/libs/common/src/vault/services/cipher-sdk.service.ts index 06f5d3eb961..9757b3d2cc7 100644 --- a/libs/common/src/vault/services/cipher-sdk.service.ts +++ b/libs/common/src/vault/services/cipher-sdk.service.ts @@ -1,8 +1,8 @@ import { firstValueFrom, switchMap, catchError } from "rxjs"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; -import { SdkService } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; -import { UserId } from "@bitwarden/common/types/guid"; +import { SdkService, asUuid } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; +import { OrganizationId, UserId } from "@bitwarden/common/types/guid"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { CipherView as SdkCipherView } from "@bitwarden/sdk-internal"; @@ -79,4 +79,185 @@ export class DefaultCipherSdkService implements CipherSdkService { ), ); } + + async deleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + if (asAdmin) { + await ref.value.vault().ciphers().admin().delete(asUuid(id)); + } else { + await ref.value.vault().ciphers().delete(asUuid(id)); + } + }), + catchError((error: unknown) => { + this.logService.error(`Failed to delete cipher: ${error}`); + throw error; + }), + ), + ); + } + + async deleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin = false, + orgId?: OrganizationId, + ): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + if (asAdmin) { + if (orgId == null) { + throw new Error("Organization ID is required for admin delete."); + } + await ref.value + .vault() + .ciphers() + .admin() + .delete_many( + ids.map((id) => asUuid(id)), + asUuid(orgId), + ); + } else { + await ref.value + .vault() + .ciphers() + .delete_many(ids.map((id) => asUuid(id))); + } + }), + catchError((error: unknown) => { + this.logService.error(`Failed to delete multiple ciphers: ${error}`); + throw error; + }), + ), + ); + } + + async softDeleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + if (asAdmin) { + await ref.value.vault().ciphers().admin().soft_delete(asUuid(id)); + } else { + await ref.value.vault().ciphers().soft_delete(asUuid(id)); + } + }), + catchError((error: unknown) => { + this.logService.error(`Failed to soft delete cipher: ${error}`); + throw error; + }), + ), + ); + } + + async softDeleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin = false, + orgId?: OrganizationId, + ): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + if (asAdmin) { + if (orgId == null) { + throw new Error("Organization ID is required for admin soft delete."); + } + await ref.value + .vault() + .ciphers() + .admin() + .soft_delete_many( + ids.map((id) => asUuid(id)), + asUuid(orgId), + ); + } else { + await ref.value + .vault() + .ciphers() + .soft_delete_many(ids.map((id) => asUuid(id))); + } + }), + catchError((error: unknown) => { + this.logService.error(`Failed to soft delete multiple ciphers: ${error}`); + throw error; + }), + ), + ); + } + + async restoreWithServer(id: string, userId: UserId, asAdmin = false): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + if (asAdmin) { + await ref.value.vault().ciphers().admin().restore(asUuid(id)); + } else { + await ref.value.vault().ciphers().restore(asUuid(id)); + } + }), + catchError((error: unknown) => { + this.logService.error(`Failed to restore cipher: ${error}`); + throw error; + }), + ), + ); + } + + async restoreManyWithServer(ids: string[], userId: UserId, orgId?: string): Promise { + return await firstValueFrom( + this.sdkService.userClient$(userId).pipe( + switchMap(async (sdk) => { + if (!sdk) { + throw new Error("SDK not available"); + } + using ref = sdk.take(); + + // No longer using an asAdmin Param. Org Vault bulkRestore will assess if an item is unassigned or editable + // The Org Vault will pass those ids an array as well as the orgId when calling bulkRestore + if (orgId) { + await ref.value + .vault() + .ciphers() + .admin() + .restore_many( + ids.map((id) => asUuid(id)), + asUuid(orgId), + ); + } else { + await ref.value + .vault() + .ciphers() + .restore_many(ids.map((id) => asUuid(id))); + } + }), + catchError((error: unknown) => { + this.logService.error(`Failed to restore multiple ciphers: ${error}`); + throw error; + }), + ), + ); + } } diff --git a/libs/common/src/vault/services/cipher.service.spec.ts b/libs/common/src/vault/services/cipher.service.spec.ts index 4f98ba62a1c..07444d5d1c6 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, @@ -280,9 +286,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 +319,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; @@ -368,9 +372,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 +394,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); @@ -1009,6 +1009,238 @@ describe("Cipher Service", () => { }); }); + describe("deleteWithServer()", () => { + const testCipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; + + it("should call apiService.deleteCipher when feature flag is disabled", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest.spyOn(apiService, "deleteCipher").mockResolvedValue(undefined); + + await cipherService.deleteWithServer(testCipherId, userId); + + expect(apiSpy).toHaveBeenCalledWith(testCipherId); + }); + + it("should call apiService.deleteCipherAdmin when feature flag is disabled and asAdmin is true", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest.spyOn(apiService, "deleteCipherAdmin").mockResolvedValue(undefined); + + await cipherService.deleteWithServer(testCipherId, userId, true); + + expect(apiSpy).toHaveBeenCalledWith(testCipherId); + }); + + it("should use SDK to delete cipher when feature flag is enabled", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "deleteWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.deleteWithServer(testCipherId, userId, false); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherId, userId, false); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + + it("should use SDK admin delete when feature flag is enabled and asAdmin is true", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "deleteWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.deleteWithServer(testCipherId, userId, true); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherId, userId, true); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + }); + + describe("deleteManyWithServer()", () => { + const testCipherIds = [ + "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId, + "6ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b23" as CipherId, + ]; + + it("should call apiService.deleteManyCiphers when feature flag is disabled", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest.spyOn(apiService, "deleteManyCiphers").mockResolvedValue(undefined); + + await cipherService.deleteManyWithServer(testCipherIds, userId); + + expect(apiSpy).toHaveBeenCalled(); + }); + + it("should call apiService.deleteManyCiphersAdmin when feature flag is disabled and asAdmin is true", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest.spyOn(apiService, "deleteManyCiphersAdmin").mockResolvedValue(undefined); + + await cipherService.deleteManyWithServer(testCipherIds, userId, true, orgId); + + expect(apiSpy).toHaveBeenCalled(); + }); + + it("should use SDK to delete multiple ciphers when feature flag is enabled", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "deleteManyWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.deleteManyWithServer(testCipherIds, userId, false); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherIds, userId, false, undefined); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + + it("should use SDK admin delete many when feature flag is enabled and asAdmin is true", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "deleteManyWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.deleteManyWithServer(testCipherIds, userId, true, orgId); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherIds, userId, true, orgId); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + }); + + describe("softDeleteWithServer()", () => { + const testCipherId = "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId; + + it("should call apiService.putDeleteCipher when feature flag is disabled", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest.spyOn(apiService, "putDeleteCipher").mockResolvedValue(undefined); + + await cipherService.softDeleteWithServer(testCipherId, userId); + + expect(apiSpy).toHaveBeenCalledWith(testCipherId); + }); + + it("should call apiService.putDeleteCipherAdmin when feature flag is disabled and asAdmin is true", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest.spyOn(apiService, "putDeleteCipherAdmin").mockResolvedValue(undefined); + + await cipherService.softDeleteWithServer(testCipherId, userId, true); + + expect(apiSpy).toHaveBeenCalledWith(testCipherId); + }); + + it("should use SDK to soft delete cipher when feature flag is enabled", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "softDeleteWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.softDeleteWithServer(testCipherId, userId, false); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherId, userId, false); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + + it("should use SDK admin soft delete when feature flag is enabled and asAdmin is true", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "softDeleteWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.softDeleteWithServer(testCipherId, userId, true); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherId, userId, true); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + }); + + describe("softDeleteManyWithServer()", () => { + const testCipherIds = [ + "5ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b22" as CipherId, + "6ff8c0b2-1d3e-4f8c-9b2d-1d3e4f8c0b23" as CipherId, + ]; + + it("should call apiService.putDeleteManyCiphers when feature flag is disabled", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest.spyOn(apiService, "putDeleteManyCiphers").mockResolvedValue(undefined); + + await cipherService.softDeleteManyWithServer(testCipherIds, userId); + + expect(apiSpy).toHaveBeenCalled(); + }); + + it("should call apiService.putDeleteManyCiphersAdmin when feature flag is disabled and asAdmin is true", async () => { + configService.getFeatureFlag$ + .calledWith(FeatureFlag.PM27632_SdkCipherCrudOperations) + .mockReturnValue(of(false)); + + const apiSpy = jest + .spyOn(apiService, "putDeleteManyCiphersAdmin") + .mockResolvedValue(undefined); + + await cipherService.softDeleteManyWithServer(testCipherIds, userId, true, orgId); + + expect(apiSpy).toHaveBeenCalled(); + }); + + it("should use SDK to soft delete multiple ciphers when feature flag is enabled", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "softDeleteManyWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.softDeleteManyWithServer(testCipherIds, userId, false); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherIds, userId, false, undefined); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + + it("should use SDK admin soft delete many when feature flag is enabled and asAdmin is true", async () => { + sdkCrudFeatureFlag$.next(true); + + const sdkServiceSpy = jest + .spyOn(cipherSdkService, "softDeleteManyWithServer") + .mockResolvedValue(undefined); + const clearCacheSpy = jest.spyOn(cipherService as any, "clearCache"); + + await cipherService.softDeleteManyWithServer(testCipherIds, userId, true, orgId); + + expect(sdkServiceSpy).toHaveBeenCalledWith(testCipherIds, userId, true, orgId); + expect(clearCacheSpy).toHaveBeenCalledWith(userId); + }); + }); + describe("replace (no upsert)", () => { // In order to set up initial state we need to manually update the encrypted state // which will result in an emission. All tests will have this baseline emission. diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 53d7666e304..1fc455a1ae9 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); @@ -1389,7 +1392,14 @@ export class CipherService implements CipherServiceAbstraction { await this.encryptedCiphersState(userId).update(() => ciphers); } - async deleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { + async deleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); + if (useSdk) { + await this.cipherSdkService.deleteWithServer(id, userId, asAdmin); + await this.clearCache(userId); + return; + } + if (asAdmin) { await this.apiService.deleteCipherAdmin(id); } else { @@ -1399,7 +1409,19 @@ export class CipherService implements CipherServiceAbstraction { await this.delete(id, userId); } - async deleteManyWithServer(ids: string[], userId: UserId, asAdmin = false): Promise { + async deleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin = false, + orgId?: OrganizationId, + ): Promise { + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); + if (useSdk) { + await this.cipherSdkService.deleteManyWithServer(ids, userId, asAdmin, orgId); + await this.clearCache(userId); + return; + } + const request = new CipherBulkDeleteRequest(ids); if (asAdmin) { await this.apiService.deleteManyCiphersAdmin(request); @@ -1539,7 +1561,7 @@ export class CipherService implements CipherServiceAbstraction { }; } - async softDelete(id: string | string[], userId: UserId): Promise { + async softDelete(id: string | string[], userId: UserId): Promise { let ciphers = await firstValueFrom(this.ciphers$(userId)); if (ciphers == null) { return; @@ -1567,7 +1589,14 @@ export class CipherService implements CipherServiceAbstraction { }); } - async softDeleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { + async softDeleteWithServer(id: string, userId: UserId, asAdmin = false): Promise { + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); + if (useSdk) { + await this.cipherSdkService.softDeleteWithServer(id, userId, asAdmin); + await this.clearCache(userId); + return; + } + if (asAdmin) { await this.apiService.putDeleteCipherAdmin(id); } else { @@ -1577,7 +1606,19 @@ export class CipherService implements CipherServiceAbstraction { await this.softDelete(id, userId); } - async softDeleteManyWithServer(ids: string[], userId: UserId, asAdmin = false): Promise { + async softDeleteManyWithServer( + ids: string[], + userId: UserId, + asAdmin = false, + orgId?: OrganizationId, + ): Promise { + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); + if (useSdk) { + await this.cipherSdkService.softDeleteManyWithServer(ids, userId, asAdmin, orgId); + await this.clearCache(userId); + return; + } + const request = new CipherBulkDeleteRequest(ids); if (asAdmin) { await this.apiService.putDeleteManyCiphersAdmin(request); @@ -1621,7 +1662,14 @@ export class CipherService implements CipherServiceAbstraction { }); } - async restoreWithServer(id: string, userId: UserId, asAdmin = false): Promise { + async restoreWithServer(id: string, userId: UserId, asAdmin = false): Promise { + const useSdk = await firstValueFrom(this.sdkCipherCrudEnabled$); + if (useSdk) { + await this.cipherSdkService.restoreWithServer(id, userId, asAdmin); + await this.clearCache(userId); + return; + } + let response; if (asAdmin) { response = await this.apiService.putRestoreCipherAdmin(id); @@ -1637,6 +1685,13 @@ 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 firstValueFrom(this.sdkCipherCrudEnabled$); + if (useSdk) { + await this.cipherSdkService.restoreManyWithServer(ids, userId, orgId); + await this.clearCache(userId); + return; + } + let response; if (orgId) {