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 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. 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) {