From 9f9cb0d13d0bc88c24e3adcac0b864a0ec336fc3 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Fri, 30 May 2025 10:50:54 -0700 Subject: [PATCH] Add-userid-to-encryption-methods (#14844) * Get userId from response if available This is a small improvement for the Auth team which avoids inspection of the access token, sometimes. * Initialize sdk clients with a userId * return both Cipher and encryptedFor when encrypting a cipher Update cipher api requests to include encryptedFor attribute * Prefer named types with documentation * Update sdk to latest * Fixup types * Fixup tests * Revert getting userId from identity token response --------- Co-authored-by: Shane --- .../notification.background.spec.ts | 17 +- .../background/notification.background.ts | 5 +- .../autofill/popup/fido2/fido2.component.ts | 6 +- .../vault-popup-autofill.service.spec.ts | 2 +- .../vault/components/add-edit.component.ts | 14 +- .../fido2/fido2-authenticator.service.spec.ts | 15 +- .../services/sdk/default-sdk.service.ts | 8 +- .../src/vault/abstractions/cipher.service.ts | 15 +- .../src/vault/models/data/field.data.ts | 2 +- .../request/cipher-bulk-share.request.ts | 9 +- .../models/request/cipher-create.request.ts | 6 +- .../models/request/cipher-share.request.ts | 6 +- .../models/request/cipher-with-id.request.ts | 6 +- .../vault/models/request/cipher.request.ts | 7 +- .../src/vault/models/view/cipher.view.ts | 2 +- .../src/vault/services/cipher.service.spec.ts | 168 +++++++++--------- .../src/vault/services/cipher.service.ts | 140 ++++++--------- .../services/default-cipher-form.service.ts | 9 +- .../assign-collections.component.ts | 2 +- 19 files changed, 212 insertions(+), 227 deletions(-) diff --git a/apps/browser/src/autofill/background/notification.background.spec.ts b/apps/browser/src/autofill/background/notification.background.spec.ts index 009efd7ff3..b161200215 100644 --- a/apps/browser/src/autofill/background/notification.background.spec.ts +++ b/apps/browser/src/autofill/background/notification.background.spec.ts @@ -69,8 +69,9 @@ describe("NotificationBackground", () => { const accountService = mock(); const organizationService = mock(); + const userId = "testId" as UserId; const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>({ - id: "testId" as UserId, + id: userId, email: "test@example.com", emailVerified: true, name: "Test User", @@ -1141,8 +1142,11 @@ describe("NotificationBackground", () => { convertAddLoginQueueMessageToCipherViewSpy.mockReturnValueOnce(cipherView); editItemSpy.mockResolvedValueOnce(undefined); cipherEncryptSpy.mockResolvedValueOnce({ - ...cipherView, - id: "testId", + cipher: { + ...cipherView, + id: "testId", + }, + encryptedFor: userId, }); sendMockExtensionMessage(message, sender); @@ -1188,6 +1192,13 @@ 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); diff --git a/apps/browser/src/autofill/background/notification.background.ts b/apps/browser/src/autofill/background/notification.background.ts index a73141b7e4..cb6a67c813 100644 --- a/apps/browser/src/autofill/background/notification.background.ts +++ b/apps/browser/src/autofill/background/notification.background.ts @@ -719,9 +719,10 @@ export default class NotificationBackground { return; } - const cipher = await this.cipherService.encrypt(newCipher, activeUserId); + const encrypted = await this.cipherService.encrypt(newCipher, activeUserId); + const { cipher } = encrypted; try { - await this.cipherService.createWithServer(cipher); + await this.cipherService.createWithServer(encrypted); await BrowserApi.tabSendMessageData(tab, "saveCipherAttemptCompleted", { itemName: newCipher?.name && String(newCipher?.name), cipherId: cipher?.id && String(cipher?.id), diff --git a/apps/browser/src/autofill/popup/fido2/fido2.component.ts b/apps/browser/src/autofill/popup/fido2/fido2.component.ts index 6b7d912019..996d1bb617 100644 --- a/apps/browser/src/autofill/popup/fido2/fido2.component.ts +++ b/apps/browser/src/autofill/popup/fido2/fido2.component.ts @@ -442,10 +442,10 @@ export class Fido2Component implements OnInit, OnDestroy { ); this.buildCipher(name, username); - const cipher = await this.cipherService.encrypt(this.cipher, activeUserId); + const encrypted = await this.cipherService.encrypt(this.cipher, activeUserId); try { - await this.cipherService.createWithServer(cipher); - this.cipher.id = cipher.id; + await this.cipherService.createWithServer(encrypted); + this.cipher.id = encrypted.cipher.id; } catch (e) { this.logService.error(e); } 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 415aeb3108..73c3fed327 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 @@ -353,7 +353,7 @@ describe("VaultPopupAutofillService", () => { }); it("should add a URI to the cipher and save with the server", async () => { - const mockEncryptedCipher = {} as Cipher; + const mockEncryptedCipher = { cipher: {} as Cipher, encryptedFor: mockUserId }; mockCipherService.encrypt.mockResolvedValue(mockEncryptedCipher); const result = await service.doAutofillAndSave(mockCipher); expect(result).toBe(true); diff --git a/libs/angular/src/vault/components/add-edit.component.ts b/libs/angular/src/vault/components/add-edit.component.ts index 8175372cae..8cc79a22df 100644 --- a/libs/angular/src/vault/components/add-edit.component.ts +++ b/libs/angular/src/vault/components/add-edit.component.ts @@ -26,11 +26,13 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl import { SdkService } from "@bitwarden/common/platform/abstractions/sdk/sdk.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { CollectionId, UserId } from "@bitwarden/common/types/guid"; -import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { + CipherService, + EncryptionContext, +} from "@bitwarden/common/vault/abstractions/cipher.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { CipherType, SecureNoteType } from "@bitwarden/common/vault/enums"; import { CipherRepromptType } from "@bitwarden/common/vault/enums/cipher-reprompt-type"; -import { Cipher } from "@bitwarden/common/vault/models/domain/cipher"; import { CardView } from "@bitwarden/common/vault/models/view/card.view"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { FolderView } from "@bitwarden/common/vault/models/view/folder.view"; @@ -740,17 +742,17 @@ export class AddEditComponent implements OnInit, OnDestroy { return this.cipherService.encrypt(this.cipher, userId); } - protected saveCipher(cipher: Cipher) { + protected saveCipher(data: EncryptionContext) { let orgAdmin = this.organization?.canEditAllCiphers; // if a cipher is unassigned we want to check if they are an admin or have permission to edit any collection - if (!cipher.collectionIds) { + if (!data.cipher.collectionIds) { orgAdmin = this.organization?.canEditUnassignedCiphers; } return this.cipher.id == null - ? this.cipherService.createWithServer(cipher, orgAdmin) - : this.cipherService.updateWithServer(cipher, orgAdmin); + ? this.cipherService.createWithServer(data, orgAdmin) + : this.cipherService.updateWithServer(data, orgAdmin); } protected deleteCipher(userId: UserId) { 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 5c377e1a98..78ae8253ee 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 @@ -6,7 +6,7 @@ import { BehaviorSubject, of } from "rxjs"; import { mockAccountServiceWith } from "../../../../spec"; import { Account } from "../../../auth/abstractions/account.service"; import { UserId } from "../../../types/guid"; -import { CipherService } from "../../../vault/abstractions/cipher.service"; +import { CipherService, EncryptionContext } from "../../../vault/abstractions/cipher.service"; import { SyncService } from "../../../vault/abstractions/sync/sync.service.abstraction"; import { CipherRepromptType } from "../../../vault/enums/cipher-reprompt-type"; import { CipherType } from "../../../vault/enums/cipher-type"; @@ -36,8 +36,9 @@ type ParentWindowReference = string; const RpId = "bitwarden.com"; describe("FidoAuthenticatorService", () => { + const userId = "testId" as UserId; const activeAccountSubject = new BehaviorSubject({ - id: "testId" as UserId, + id: userId, email: "test@example.com", emailVerified: true, name: "Test User", @@ -254,7 +255,7 @@ describe("FidoAuthenticatorService", () => { cipherId: existingCipher.id, userVerified: false, }); - cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as Cipher); + cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as EncryptionContext); await authenticator.makeCredential(params, windowReference); @@ -325,7 +326,7 @@ describe("FidoAuthenticatorService", () => { cipherId: existingCipher.id, userVerified: false, }); - cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as Cipher); + cipherService.encrypt.mockResolvedValue(encryptedCipher as unknown as EncryptionContext); cipherService.updateWithServer.mockRejectedValue(new Error("Internal error")); const result = async () => await authenticator.makeCredential(params, windowReference); @@ -357,13 +358,13 @@ describe("FidoAuthenticatorService", () => { cipherService.decrypt.mockResolvedValue(cipher); cipherService.encrypt.mockImplementation(async (cipher) => { cipher.login.fido2Credentials[0].credentialId = credentialId; // Replace id for testability - return {} as any; + return { cipher: {} as any as Cipher, encryptedFor: userId }; }); - cipherService.createWithServer.mockImplementation(async (cipher) => { + cipherService.createWithServer.mockImplementation(async ({ cipher }) => { cipher.id = cipherId; return cipher; }); - cipherService.updateWithServer.mockImplementation(async (cipher) => { + cipherService.updateWithServer.mockImplementation(async ({ cipher }) => { cipher.id = cipherId; return cipher; }); 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 6be89a4b37..d9f7ba19a6 100644 --- a/libs/common/src/platform/services/sdk/default-sdk.service.ts +++ b/libs/common/src/platform/services/sdk/default-sdk.service.ts @@ -180,9 +180,7 @@ export class DefaultSdkService implements SdkService { return () => client?.markForDisposal(); }); }), - tap({ - finalize: () => this.sdkClientCache.delete(userId), - }), + tap({ finalize: () => this.sdkClientCache.delete(userId) }), shareReplay({ refCount: true, bufferSize: 1 }), ); @@ -205,9 +203,7 @@ export class DefaultSdkService implements SdkService { method: { decryptedKey: { decrypted_user_key: userKey.keyB64 } }, kdfParams: kdfParams.kdfType === KdfType.PBKDF2_SHA256 - ? { - pBKDF2: { iterations: kdfParams.iterations }, - } + ? { pBKDF2: { iterations: kdfParams.iterations } } : { argon2id: { iterations: kdfParams.iterations, diff --git a/libs/common/src/vault/abstractions/cipher.service.ts b/libs/common/src/vault/abstractions/cipher.service.ts index fc80905816..91f8006d15 100644 --- a/libs/common/src/vault/abstractions/cipher.service.ts +++ b/libs/common/src/vault/abstractions/cipher.service.ts @@ -21,6 +21,12 @@ import { CipherView } from "../models/view/cipher.view"; import { FieldView } from "../models/view/field.view"; import { AddEditCipherInfo } from "../types/add-edit-cipher-info"; +export type EncryptionContext = { + cipher: Cipher; + /** The Id of the user that encrypted the cipher. It should always represent a UserId, even for Organization-owned ciphers */ + encryptedFor: UserId; +}; + export abstract class CipherService implements UserKeyRotationDataProvider { abstract cipherViews$(userId: UserId): Observable; abstract ciphers$(userId: UserId): Observable>; @@ -42,7 +48,7 @@ export abstract class CipherService implements UserKeyRotationDataProvider; + ): Promise; abstract encryptFields(fieldsModel: FieldView[], key: SymmetricCryptoKey): Promise; abstract encryptField(fieldModel: FieldView, key: SymmetricCryptoKey): Promise; abstract get(id: string, userId: UserId): Promise; @@ -94,7 +100,10 @@ export abstract class CipherService implements UserKeyRotationDataProvider; + abstract createWithServer( + { cipher, encryptedFor }: EncryptionContext, + orgAdmin?: boolean, + ): Promise; /** * Update a cipher with the server * @param cipher The cipher to update @@ -104,7 +113,7 @@ export abstract class CipherService implements UserKeyRotationDataProvider; diff --git a/libs/common/src/vault/models/data/field.data.ts b/libs/common/src/vault/models/data/field.data.ts index b9daf7fa42..cf9df69a6b 100644 --- a/libs/common/src/vault/models/data/field.data.ts +++ b/libs/common/src/vault/models/data/field.data.ts @@ -7,7 +7,7 @@ export class FieldData { type: FieldType; name: string; value: string; - linkedId: LinkedIdType; + linkedId: LinkedIdType | null; constructor(response?: FieldApi) { if (response == null) { diff --git a/libs/common/src/vault/models/request/cipher-bulk-share.request.ts b/libs/common/src/vault/models/request/cipher-bulk-share.request.ts index 4f56297d0a..d0c394bea0 100644 --- a/libs/common/src/vault/models/request/cipher-bulk-share.request.ts +++ b/libs/common/src/vault/models/request/cipher-bulk-share.request.ts @@ -1,5 +1,6 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore +import { UserId } from "../../../types/guid"; import { Cipher } from "../domain/cipher"; import { CipherWithIdRequest } from "./cipher-with-id.request"; @@ -8,11 +9,15 @@ export class CipherBulkShareRequest { ciphers: CipherWithIdRequest[]; collectionIds: string[]; - constructor(ciphers: Cipher[], collectionIds: string[]) { + constructor( + ciphers: Cipher[], + collectionIds: string[], + readonly encryptedFor: UserId, + ) { if (ciphers != null) { this.ciphers = []; ciphers.forEach((c) => { - this.ciphers.push(new CipherWithIdRequest(c)); + this.ciphers.push(new CipherWithIdRequest({ cipher: c, encryptedFor })); }); } this.collectionIds = collectionIds; diff --git a/libs/common/src/vault/models/request/cipher-create.request.ts b/libs/common/src/vault/models/request/cipher-create.request.ts index 9c3be5544b..e992ebed9b 100644 --- a/libs/common/src/vault/models/request/cipher-create.request.ts +++ b/libs/common/src/vault/models/request/cipher-create.request.ts @@ -1,4 +1,4 @@ -import { Cipher } from "../domain/cipher"; +import { EncryptionContext } from "../../abstractions/cipher.service"; import { CipherRequest } from "./cipher.request"; @@ -6,8 +6,8 @@ export class CipherCreateRequest { cipher: CipherRequest; collectionIds: string[]; - constructor(cipher: Cipher) { - this.cipher = new CipherRequest(cipher); + constructor({ cipher, encryptedFor }: EncryptionContext) { + this.cipher = new CipherRequest({ cipher, encryptedFor }); this.collectionIds = cipher.collectionIds; } } diff --git a/libs/common/src/vault/models/request/cipher-share.request.ts b/libs/common/src/vault/models/request/cipher-share.request.ts index 4043599ce0..17c46168ef 100644 --- a/libs/common/src/vault/models/request/cipher-share.request.ts +++ b/libs/common/src/vault/models/request/cipher-share.request.ts @@ -1,4 +1,4 @@ -import { Cipher } from "../domain/cipher"; +import { EncryptionContext } from "../../abstractions/cipher.service"; import { CipherRequest } from "./cipher.request"; @@ -6,8 +6,8 @@ export class CipherShareRequest { cipher: CipherRequest; collectionIds: string[]; - constructor(cipher: Cipher) { - this.cipher = new CipherRequest(cipher); + constructor({ cipher, encryptedFor }: EncryptionContext) { + this.cipher = new CipherRequest({ cipher, encryptedFor }); this.collectionIds = cipher.collectionIds; } } diff --git a/libs/common/src/vault/models/request/cipher-with-id.request.ts b/libs/common/src/vault/models/request/cipher-with-id.request.ts index f291e34264..0b04f50fb1 100644 --- a/libs/common/src/vault/models/request/cipher-with-id.request.ts +++ b/libs/common/src/vault/models/request/cipher-with-id.request.ts @@ -1,12 +1,12 @@ -import { Cipher } from "../domain/cipher"; +import { EncryptionContext } from "../../abstractions/cipher.service"; import { CipherRequest } from "./cipher.request"; export class CipherWithIdRequest extends CipherRequest { id: string; - constructor(cipher: Cipher) { - super(cipher); + constructor({ cipher, encryptedFor }: EncryptionContext) { + super({ cipher, encryptedFor }); this.id = cipher.id; } } diff --git a/libs/common/src/vault/models/request/cipher.request.ts b/libs/common/src/vault/models/request/cipher.request.ts index 5b77ee7508..2e3b2efbed 100644 --- a/libs/common/src/vault/models/request/cipher.request.ts +++ b/libs/common/src/vault/models/request/cipher.request.ts @@ -1,5 +1,7 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore +import { UserId } from "../../../types/guid"; +import { EncryptionContext } from "../../abstractions/cipher.service"; import { CipherRepromptType } from "../../enums/cipher-reprompt-type"; import { CipherType } from "../../enums/cipher-type"; import { CardApi } from "../api/card.api"; @@ -10,12 +12,12 @@ import { LoginUriApi } from "../api/login-uri.api"; import { LoginApi } from "../api/login.api"; import { SecureNoteApi } from "../api/secure-note.api"; import { SshKeyApi } from "../api/ssh-key.api"; -import { Cipher } from "../domain/cipher"; import { AttachmentRequest } from "./attachment.request"; import { PasswordHistoryRequest } from "./password-history.request"; export class CipherRequest { + encryptedFor: UserId; type: CipherType; folderId: string; organizationId: string; @@ -36,8 +38,9 @@ export class CipherRequest { reprompt: CipherRepromptType; key: string; - constructor(cipher: Cipher) { + constructor({ cipher, encryptedFor }: EncryptionContext) { this.type = cipher.type; + this.encryptedFor = encryptedFor; this.folderId = cipher.folderId; this.organizationId = cipher.organizationId; this.name = cipher.name ? cipher.name.encryptedString : null; diff --git a/libs/common/src/vault/models/view/cipher.view.ts b/libs/common/src/vault/models/view/cipher.view.ts index 1f73903a5b..e182025a33 100644 --- a/libs/common/src/vault/models/view/cipher.view.ts +++ b/libs/common/src/vault/models/view/cipher.view.ts @@ -25,7 +25,7 @@ export class CipherView implements View, InitializerMetadata { readonly initializerKey = InitializerKey.CipherView; id: string = null; - organizationId: string = null; + organizationId: string | undefined = null; folderId: string = null; name: string = null; notes: string = null; diff --git a/libs/common/src/vault/services/cipher.service.spec.ts b/libs/common/src/vault/services/cipher.service.spec.ts index 9e56bac2ca..1a0b156877 100644 --- a/libs/common/src/vault/services/cipher.service.spec.ts +++ b/libs/common/src/vault/services/cipher.service.spec.ts @@ -27,6 +27,7 @@ import { ContainerService } from "../../platform/services/container.service"; import { CipherId, UserId } from "../../types/guid"; import { CipherKey, OrgKey, UserKey } from "../../types/key"; import { CipherEncryptionService } from "../abstractions/cipher-encryption.service"; +import { EncryptionContext } from "../abstractions/cipher.service"; import { CipherFileUploadService } from "../abstractions/file-upload/cipher-file-upload.service"; import { FieldType } from "../enums"; import { CipherRepromptType } from "../enums/cipher-reprompt-type"; @@ -78,36 +79,12 @@ const cipherData: CipherData = { }, passwordHistory: [{ password: "EncryptedString", lastUsedDate: "2022-01-31T12:00:00.000Z" }], attachments: [ - { - id: "a1", - url: "url", - size: "1100", - sizeName: "1.1 KB", - fileName: "file", - key: "EncKey", - }, - { - id: "a2", - url: "url", - size: "1100", - sizeName: "1.1 KB", - fileName: "file", - key: "EncKey", - }, + { id: "a1", url: "url", size: "1100", sizeName: "1.1 KB", fileName: "file", key: "EncKey" }, + { id: "a2", url: "url", size: "1100", sizeName: "1.1 KB", fileName: "file", key: "EncKey" }, ], fields: [ - { - name: "EncryptedString", - value: "EncryptedString", - type: FieldType.Text, - linkedId: null, - }, - { - name: "EncryptedString", - value: "EncryptedString", - type: FieldType.Hidden, - linkedId: null, - }, + { name: "EncryptedString", value: "EncryptedString", type: FieldType.Text, linkedId: null }, + { name: "EncryptedString", value: "EncryptedString", type: FieldType.Hidden, linkedId: null }, ], }; const mockUserId = Utils.newGuid() as UserId; @@ -133,7 +110,7 @@ describe("Cipher Service", () => { const userId = "TestUserId" as UserId; let cipherService: CipherService; - let cipherObj: Cipher; + let encryptionContext: EncryptionContext; beforeEach(() => { encryptService.encryptFileData.mockReturnValue(Promise.resolve(ENCRYPTED_BYTES)); @@ -159,7 +136,7 @@ describe("Cipher Service", () => { cipherEncryptionService, ); - cipherObj = new Cipher(cipherData); + encryptionContext = { cipher: new Cipher(cipherData), encryptedFor: userId }; }); afterEach(() => { @@ -192,33 +169,33 @@ describe("Cipher Service", () => { it("should call apiService.postCipherAdmin when orgAdmin param is true and the cipher orgId != null", async () => { const spy = jest .spyOn(apiService, "postCipherAdmin") - .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); - await cipherService.createWithServer(cipherObj, true); - const expectedObj = new CipherCreateRequest(cipherObj); + .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); + await cipherService.createWithServer(encryptionContext, true); + const expectedObj = new CipherCreateRequest(encryptionContext); expect(spy).toHaveBeenCalled(); expect(spy).toHaveBeenCalledWith(expectedObj); }); it("should call apiService.postCipher when orgAdmin param is true and the cipher orgId is null", async () => { - cipherObj.organizationId = null; + encryptionContext.cipher.organizationId = null!; const spy = jest .spyOn(apiService, "postCipher") - .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); - await cipherService.createWithServer(cipherObj, true); - const expectedObj = new CipherRequest(cipherObj); + .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); + await cipherService.createWithServer(encryptionContext, true); + const expectedObj = new CipherRequest(encryptionContext); expect(spy).toHaveBeenCalled(); expect(spy).toHaveBeenCalledWith(expectedObj); }); it("should call apiService.postCipherCreate if collectionsIds != null", async () => { - cipherObj.collectionIds = ["123"]; + encryptionContext.cipher.collectionIds = ["123"]; const spy = jest .spyOn(apiService, "postCipherCreate") - .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); - await cipherService.createWithServer(cipherObj); - const expectedObj = new CipherCreateRequest(cipherObj); + .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); + await cipherService.createWithServer(encryptionContext); + const expectedObj = new CipherCreateRequest(encryptionContext); expect(spy).toHaveBeenCalled(); expect(spy).toHaveBeenCalledWith(expectedObj); @@ -227,9 +204,9 @@ describe("Cipher Service", () => { it("should call apiService.postCipher when orgAdmin and collectionIds logic is false", async () => { const spy = jest .spyOn(apiService, "postCipher") - .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); - await cipherService.createWithServer(cipherObj); - const expectedObj = new CipherRequest(cipherObj); + .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); + await cipherService.createWithServer(encryptionContext); + const expectedObj = new CipherRequest(encryptionContext); expect(spy).toHaveBeenCalled(); expect(spy).toHaveBeenCalledWith(expectedObj); @@ -240,36 +217,36 @@ describe("Cipher Service", () => { it("should call apiService.putCipherAdmin when orgAdmin param is true", async () => { const spy = jest .spyOn(apiService, "putCipherAdmin") - .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); - await cipherService.updateWithServer(cipherObj, true); - const expectedObj = new CipherRequest(cipherObj); + .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); + await cipherService.updateWithServer(encryptionContext, true); + const expectedObj = new CipherRequest(encryptionContext); expect(spy).toHaveBeenCalled(); - expect(spy).toHaveBeenCalledWith(cipherObj.id, expectedObj); + expect(spy).toHaveBeenCalledWith(encryptionContext.cipher.id, expectedObj); }); it("should call apiService.putCipher if cipher.edit is true", async () => { - cipherObj.edit = true; + encryptionContext.cipher.edit = true; const spy = jest .spyOn(apiService, "putCipher") - .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); - await cipherService.updateWithServer(cipherObj); - const expectedObj = new CipherRequest(cipherObj); + .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); + await cipherService.updateWithServer(encryptionContext); + const expectedObj = new CipherRequest(encryptionContext); expect(spy).toHaveBeenCalled(); - expect(spy).toHaveBeenCalledWith(cipherObj.id, expectedObj); + expect(spy).toHaveBeenCalledWith(encryptionContext.cipher.id, expectedObj); }); it("should call apiService.putPartialCipher when orgAdmin, and edit are false", async () => { - cipherObj.edit = false; + encryptionContext.cipher.edit = false; const spy = jest .spyOn(apiService, "putPartialCipher") - .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); - await cipherService.updateWithServer(cipherObj); - const expectedObj = new CipherPartialRequest(cipherObj); + .mockImplementation(() => Promise.resolve(encryptionContext.cipher.toCipherData())); + await cipherService.updateWithServer(encryptionContext); + const expectedObj = new CipherPartialRequest(encryptionContext.cipher); expect(spy).toHaveBeenCalled(); - expect(spy).toHaveBeenCalledWith(cipherObj.id, expectedObj); + expect(spy).toHaveBeenCalledWith(encryptionContext.cipher.id, expectedObj); }); }); @@ -293,6 +270,15 @@ describe("Cipher Service", () => { jest.spyOn(cipherService as any, "getAutofillOnPageLoadDefault").mockResolvedValue(true); }); + it("should return the encrypting user id", async () => { + keyService.getOrgKey.mockReturnValue( + Promise.resolve(new SymmetricCryptoKey(new Uint8Array(32)) as OrgKey), + ); + + const { encryptedFor } = await cipherService.encrypt(cipherView, userId); + expect(encryptedFor).toEqual(userId); + }); + describe("login encryption", () => { it("should add a uri hash to login uris", async () => { encryptService.hash.mockImplementation((value) => Promise.resolve(`${value} hash`)); @@ -304,9 +290,9 @@ describe("Cipher Service", () => { Promise.resolve(new SymmetricCryptoKey(new Uint8Array(32)) as OrgKey), ); - const domain = await cipherService.encrypt(cipherView, userId); + const { cipher } = await cipherService.encrypt(cipherView, userId); - expect(domain.login.uris).toEqual([ + expect(cipher.login.uris).toEqual([ { uri: new EncString("uri has been encrypted"), uriChecksum: new EncString("uri hash has been encrypted"), @@ -325,7 +311,7 @@ describe("Cipher Service", () => { it("is null when feature flag is false", async () => { configService.getFeatureFlag.mockResolvedValue(false); - const cipher = await cipherService.encrypt(cipherView, userId); + const { cipher } = await cipherService.encrypt(cipherView, userId); expect(cipher.key).toBeNull(); }); @@ -338,7 +324,7 @@ describe("Cipher Service", () => { it("is null when the cipher is not viewPassword", async () => { cipherView.viewPassword = false; - const cipher = await cipherService.encrypt(cipherView, userId); + const { cipher } = await cipherService.encrypt(cipherView, userId); expect(cipher.key).toBeNull(); }); @@ -346,7 +332,7 @@ describe("Cipher Service", () => { it("is defined when the cipher is viewPassword", async () => { cipherView.viewPassword = true; - const cipher = await cipherService.encrypt(cipherView, userId); + const { cipher } = await cipherService.encrypt(cipherView, userId); expect(cipher.key).toBeDefined(); }); @@ -393,7 +379,13 @@ describe("Cipher Service", () => { it("is called when cipher viewPassword is false and original cipher has a key", async () => { cipherView.viewPassword = false; - await cipherService.encrypt(cipherView, userId, undefined, undefined, cipherObj); + await cipherService.encrypt( + cipherView, + userId, + undefined, + undefined, + encryptionContext.cipher, + ); expect(cipherService["encryptCipherWithCipherKey"]).toHaveBeenCalled(); }); @@ -416,22 +408,17 @@ describe("Cipher Service", () => { stateService.getUserId.mockResolvedValue(mockUserId); - const keys = { - userKey: originalUserKey, - } as CipherDecryptionKeys; + const keys = { userKey: originalUserKey } as CipherDecryptionKeys; keyService.cipherDecryptionKeys$.mockReturnValue(of(keys)); - const cipher1 = new CipherView(cipherObj); - cipher1.id = "Cipher 1"; + const cipher1 = new CipherView(encryptionContext.cipher); + cipher1.id = "Cipher 1" as CipherId; cipher1.organizationId = null; - const cipher2 = new CipherView(cipherObj); - cipher2.id = "Cipher 2"; + const cipher2 = new CipherView(encryptionContext.cipher); + cipher2.id = "Cipher 2" as CipherId; cipher2.organizationId = null; - decryptedCiphers = new BehaviorSubject({ - Cipher1: cipher1, - Cipher2: cipher2, - }); + decryptedCiphers = new BehaviorSubject({ [cipher1.id]: cipher1, [cipher2.id]: cipher2 }); jest .spyOn(cipherService, "cipherViews$") .mockImplementation((userId: UserId) => @@ -462,19 +449,19 @@ describe("Cipher Service", () => { }); it("throws if the original user key is null", async () => { - await expect(cipherService.getRotatedData(null, newUserKey, mockUserId)).rejects.toThrow( + await expect(cipherService.getRotatedData(null!, newUserKey, mockUserId)).rejects.toThrow( "Original user key is required to rotate ciphers", ); }); it("throws if the new user key is null", async () => { - await expect(cipherService.getRotatedData(originalUserKey, null, mockUserId)).rejects.toThrow( - "New user key is required to rotate ciphers", - ); + await expect( + cipherService.getRotatedData(originalUserKey, null!, mockUserId), + ).rejects.toThrow("New user key is required to rotate ciphers"); }); it("throws if the user has any failed to decrypt ciphers", async () => { - const badCipher = new CipherView(cipherObj); + const badCipher = new CipherView(encryptionContext.cipher); badCipher.id = "Cipher 3"; badCipher.organizationId = null; badCipher.decryptionFailure = true; @@ -488,12 +475,15 @@ describe("Cipher Service", () => { describe("decrypt", () => { it("should call decrypt method of CipherEncryptionService when feature flag is true", async () => { configService.getFeatureFlag.mockResolvedValue(true); - cipherEncryptionService.decrypt.mockResolvedValue(new CipherView(cipherObj)); + cipherEncryptionService.decrypt.mockResolvedValue(new CipherView(encryptionContext.cipher)); - const result = await cipherService.decrypt(cipherObj, userId); + const result = await cipherService.decrypt(encryptionContext.cipher, userId); - expect(result).toEqual(new CipherView(cipherObj)); - expect(cipherEncryptionService.decrypt).toHaveBeenCalledWith(cipherObj, userId); + expect(result).toEqual(new CipherView(encryptionContext.cipher)); + expect(cipherEncryptionService.decrypt).toHaveBeenCalledWith( + encryptionContext.cipher, + userId, + ); }); it("should call legacy decrypt when feature flag is false", async () => { @@ -501,12 +491,14 @@ describe("Cipher Service", () => { configService.getFeatureFlag.mockResolvedValue(false); cipherService.getKeyForCipherKeyDecryption = jest.fn().mockResolvedValue(mockUserKey); encryptService.decryptToBytes.mockResolvedValue(new Uint8Array(32)); - jest.spyOn(cipherObj, "decrypt").mockResolvedValue(new CipherView(cipherObj)); + jest + .spyOn(encryptionContext.cipher, "decrypt") + .mockResolvedValue(new CipherView(encryptionContext.cipher)); - const result = await cipherService.decrypt(cipherObj, userId); + const result = await cipherService.decrypt(encryptionContext.cipher, userId); - expect(result).toEqual(new CipherView(cipherObj)); - expect(cipherObj.decrypt).toHaveBeenCalledWith(mockUserKey); + expect(result).toEqual(new CipherView(encryptionContext.cipher)); + expect(encryptionContext.cipher.decrypt).toHaveBeenCalledWith(mockUserKey); }); }); diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 2693d9d464..0c948fe0c6 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -33,7 +33,10 @@ 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 { CipherService as CipherServiceAbstraction } from "../abstractions/cipher.service"; +import { + CipherService as CipherServiceAbstraction, + EncryptionContext, +} from "../abstractions/cipher.service"; import { CipherFileUploadService } from "../abstractions/file-upload/cipher-file-upload.service"; import { FieldType } from "../enums"; import { CipherType } from "../enums/cipher-type"; @@ -196,7 +199,7 @@ export class CipherService implements CipherServiceAbstraction { keyForCipherEncryption?: SymmetricCryptoKey, keyForCipherKeyDecryption?: SymmetricCryptoKey, originalCipher: Cipher = null, - ): Promise { + ): Promise { if (model.id != null) { if (originalCipher == null) { originalCipher = await this.get(model.id, userId); @@ -230,18 +233,24 @@ export class CipherService implements CipherServiceAbstraction { keyForCipherEncryption ||= userOrOrgKey; // If the caller has provided a key for cipher key decryption, use it. Otherwise, use the user or org key. keyForCipherKeyDecryption ||= userOrOrgKey; - return this.encryptCipherWithCipherKey( - model, - cipher, - keyForCipherEncryption, - keyForCipherKeyDecryption, - ); + return { + cipher: await this.encryptCipherWithCipherKey( + model, + cipher, + keyForCipherEncryption, + keyForCipherKeyDecryption, + ), + encryptedFor: userId, + }; } else { keyForCipherEncryption ||= await this.getKeyForCipherKeyDecryption(cipher, userId); // We want to ensure that the cipher key is null if cipher key encryption is disabled // so that decryption uses the proper key. cipher.key = null; - return this.encryptCipher(model, cipher, keyForCipherEncryption); + return { + cipher: await this.encryptCipher(model, cipher, keyForCipherEncryption), + encryptedFor: userId, + }; } } @@ -261,19 +270,14 @@ export class CipherService implements CipherServiceAbstraction { attachment.size = model.size; attachment.sizeName = model.sizeName; attachment.url = model.url; - const promise = this.encryptObjProperty( - model, - attachment, - { - fileName: null, + const promise = this.encryptObjProperty(model, attachment, { fileName: null }, key).then( + async () => { + if (model.key != null) { + attachment.key = await this.encryptService.wrapSymmetricKey(model.key, key); + } + encAttachments.push(attachment); }, - key, - ).then(async () => { - if (model.key != null) { - attachment.key = await this.encryptService.wrapSymmetricKey(model.key, key); - } - encAttachments.push(attachment); - }); + ); promises.push(promise); }); @@ -306,15 +310,7 @@ export class CipherService implements CipherServiceAbstraction { fieldModel.value = "false"; } - await this.encryptObjProperty( - fieldModel, - field, - { - name: null, - value: null, - }, - key, - ); + await this.encryptObjProperty(fieldModel, field, { name: null, value: null }, key); return field; } @@ -345,14 +341,7 @@ export class CipherService implements CipherServiceAbstraction { const ph = new Password(); ph.lastUsedDate = phModel.lastUsedDate; - await this.encryptObjProperty( - phModel, - ph, - { - password: null, - }, - key, - ); + await this.encryptObjProperty(phModel, ph, { password: null }, key); return ph; } @@ -705,9 +694,7 @@ export class CipherService implements CipherServiceAbstraction { if (ciphersLocalData[cipherId]) { ciphersLocalData[cipherId].lastUsedDate = new Date().getTime(); } else { - ciphersLocalData[cipherId] = { - lastUsedDate: new Date().getTime(), - }; + ciphersLocalData[cipherId] = { lastUsedDate: new Date().getTime() }; } await this.localDataState(userId).update(() => ciphersLocalData); @@ -735,10 +722,7 @@ export class CipherService implements CipherServiceAbstraction { } const currentTime = new Date().getTime(); - ciphersLocalData[id as CipherId] = { - lastLaunched: currentTime, - lastUsedDate: currentTime, - }; + ciphersLocalData[id as CipherId] = { lastLaunched: currentTime, lastUsedDate: currentTime }; await this.localDataState(userId).update(() => ciphersLocalData); @@ -770,18 +754,21 @@ export class CipherService implements CipherServiceAbstraction { await this.domainSettingsService.setNeverDomains(domains); } - async createWithServer(cipher: Cipher, orgAdmin?: boolean): Promise { + async createWithServer( + { cipher, encryptedFor }: EncryptionContext, + orgAdmin?: boolean, + ): Promise { let response: CipherResponse; if (orgAdmin && cipher.organizationId != null) { - const request = new CipherCreateRequest(cipher); + const request = new CipherCreateRequest({ cipher, encryptedFor }); response = await this.apiService.postCipherAdmin(request); const data = new CipherData(response, cipher.collectionIds); return new Cipher(data); } else if (cipher.collectionIds != null) { - const request = new CipherCreateRequest(cipher); + const request = new CipherCreateRequest({ cipher, encryptedFor }); response = await this.apiService.postCipherCreate(request); } else { - const request = new CipherRequest(cipher); + const request = new CipherRequest({ cipher, encryptedFor }); response = await this.apiService.postCipher(request); } cipher.id = response.id; @@ -792,15 +779,18 @@ export class CipherService implements CipherServiceAbstraction { return new Cipher(updated[cipher.id as CipherId]); } - async updateWithServer(cipher: Cipher, orgAdmin?: boolean): Promise { + async updateWithServer( + { cipher, encryptedFor }: EncryptionContext, + orgAdmin?: boolean, + ): Promise { let response: CipherResponse; if (orgAdmin) { - const request = new CipherRequest(cipher); + const request = new CipherRequest({ cipher, encryptedFor }); response = await this.apiService.putCipherAdmin(cipher.id, request); const data = new CipherData(response, cipher.collectionIds); return new Cipher(data, cipher.localData); } else if (cipher.edit) { - const request = new CipherRequest(cipher); + const request = new CipherRequest({ cipher, encryptedFor }); response = await this.apiService.putCipher(cipher.id, request); } else { const request = new CipherPartialRequest(cipher); @@ -854,12 +844,12 @@ export class CipherService implements CipherServiceAbstraction { cipher.collectionIds = collectionIds; promises.push( this.encryptSharedCipher(cipher, userId).then((c) => { - encCiphers.push(c); + encCiphers.push(c.cipher); }), ); } await Promise.all(promises); - const request = new CipherBulkShareRequest(encCiphers, collectionIds); + const request = new CipherBulkShareRequest(encCiphers, collectionIds, userId); try { await this.apiService.putShareCiphers(request); } catch (e) { @@ -921,8 +911,8 @@ 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); - cipher = await this.encrypt(model, userId); - await this.updateWithServer(cipher); + const reEncrypted = await this.encrypt(model, userId); + await this.updateWithServer(reEncrypted); } const encFileName = await this.encryptService.encryptString(filename, cipherEncKey); @@ -1482,7 +1472,7 @@ export class CipherService implements CipherServiceAbstraction { // In the case of a cipher that is being shared with an organization, we want to decrypt the // cipher key with the user's key and then re-encrypt it with the organization's key. - private async encryptSharedCipher(model: CipherView, userId: UserId): Promise { + private async encryptSharedCipher(model: CipherView, userId: UserId): Promise { const keyForCipherKeyDecryption = await this.keyService.getUserKeyWithLegacySupport(userId); return await this.encrypt(model, userId, null, keyForCipherKeyDecryption); } @@ -1584,10 +1574,7 @@ export class CipherService implements CipherServiceAbstraction { fd.append( "data", Buffer.from(encData.buffer) as any, - { - filepath: encFileName.encryptedString, - contentType: "application/octet-stream", - } as any, + { filepath: encFileName.encryptedString, contentType: "application/octet-stream" } as any, ); } else { throw e; @@ -1649,11 +1636,7 @@ export class CipherService implements CipherServiceAbstraction { await this.encryptObjProperty( model.login, cipher.login, - { - username: null, - password: null, - totp: null, - }, + { username: null, password: null, totp: null }, key, ); @@ -1663,14 +1646,7 @@ export class CipherService implements CipherServiceAbstraction { for (let i = 0; i < model.login.uris.length; i++) { const loginUri = new LoginUri(); loginUri.match = model.login.uris[i].match; - await this.encryptObjProperty( - model.login.uris[i], - loginUri, - { - uri: null, - }, - key, - ); + await this.encryptObjProperty(model.login.uris[i], loginUri, { uri: null }, key); const uriHash = await this.encryptService.hash(model.login.uris[i].uri, "sha256"); loginUri.uriChecksum = await this.encryptService.encryptString(uriHash, key); cipher.login.uris.push(loginUri); @@ -1766,11 +1742,7 @@ export class CipherService implements CipherServiceAbstraction { await this.encryptObjProperty( model.sshKey, cipher.sshKey, - { - privateKey: null, - publicKey: null, - keyFingerprint: null, - }, + { privateKey: null, publicKey: null, keyFingerprint: null }, key, ); return; @@ -1855,15 +1827,7 @@ export class CipherService implements CipherServiceAbstraction { } await Promise.all([ - this.encryptObjProperty( - model, - cipher, - { - name: null, - notes: null, - }, - key, - ), + this.encryptObjProperty(model, cipher, { name: null, notes: null }, key), this.encryptCipherData(cipher, model, key), this.encryptFields(model.fields, key).then((fields) => { cipher.fields = fields; 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 68eac4f0da..99f853d4c8 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 @@ -29,19 +29,20 @@ export class DefaultCipherFormService implements CipherFormService { async saveCipher(cipher: CipherView, config: CipherFormConfig): Promise { // Passing the original cipher is important here as it is responsible for appending to password history const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); - const encryptedCipher = await this.cipherService.encrypt( + const encrypted = await this.cipherService.encrypt( cipher, activeUserId, null, null, config.originalCipher ?? null, ); + const encryptedCipher = encrypted.cipher; let savedCipher: Cipher; // Creating a new cipher if (cipher.id == null) { - savedCipher = await this.cipherService.createWithServer(encryptedCipher, config.admin); + savedCipher = await this.cipherService.createWithServer(encrypted, config.admin); return await this.cipherService.decrypt(savedCipher, activeUserId); } @@ -64,13 +65,13 @@ export class DefaultCipherFormService implements CipherFormService { ); // If the collectionIds are the same, update the cipher normally } else if (isSetEqual(originalCollectionIds, newCollectionIds)) { - savedCipher = await this.cipherService.updateWithServer(encryptedCipher, config.admin); + savedCipher = await this.cipherService.updateWithServer(encrypted, config.admin); } else { // 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( - encryptedCipher, + encrypted, config.admin || originalCollectionIds.size === 0, ); diff --git a/libs/vault/src/components/assign-collections.component.ts b/libs/vault/src/components/assign-collections.component.ts index faa2dae072..4a0bd1fc67 100644 --- a/libs/vault/src/components/assign-collections.component.ts +++ b/libs/vault/src/components/assign-collections.component.ts @@ -506,7 +506,7 @@ export class AssignCollectionsComponent implements OnInit, OnDestroy, AfterViewI private async updateAssignedCollections(cipherView: CipherView, userId: UserId) { const { collections } = this.formGroup.getRawValue(); cipherView.collectionIds = collections.map((i) => i.id as CollectionId); - const cipher = await this.cipherService.encrypt(cipherView, userId); + const { cipher } = await this.cipherService.encrypt(cipherView, userId); if (this.params.isSingleCipherAdmin) { await this.cipherService.saveCollectionsWithServerAdmin(cipher); } else {