diff --git a/apps/cli/src/commands/edit.command.ts b/apps/cli/src/commands/edit.command.ts index 3d4f9529ad..e64ff8b551 100644 --- a/apps/cli/src/commands/edit.command.ts +++ b/apps/cli/src/commands/edit.command.ts @@ -86,8 +86,7 @@ export class EditCommand { cipherView = CipherExport.toView(req, cipherView); const encCipher = await this.cipherService.encrypt(cipherView); try { - await this.cipherService.updateWithServer(encCipher); - const updatedCipher = await this.cipherService.get(cipher.id); + const updatedCipher = await this.cipherService.updateWithServer(encCipher); const decCipher = await updatedCipher.decrypt( await this.cipherService.getKeyForCipherKeyDecryption(updatedCipher), ); @@ -111,8 +110,7 @@ export class EditCommand { cipher.collectionIds = req; try { - await this.cipherService.saveCollectionsWithServer(cipher); - const updatedCipher = await this.cipherService.get(cipher.id); + const updatedCipher = await this.cipherService.saveCollectionsWithServer(cipher); const decCipher = await updatedCipher.decrypt( await this.cipherService.getKeyForCipherKeyDecryption(updatedCipher), ); diff --git a/apps/cli/src/vault/create.command.ts b/apps/cli/src/vault/create.command.ts index b813227109..78ee04e73c 100644 --- a/apps/cli/src/vault/create.command.ts +++ b/apps/cli/src/vault/create.command.ts @@ -80,8 +80,7 @@ export class CreateCommand { private async createCipher(req: CipherExport) { const cipher = await this.cipherService.encrypt(CipherExport.toView(req)); try { - await this.cipherService.createWithServer(cipher); - const newCipher = await this.cipherService.get(cipher.id); + const newCipher = await this.cipherService.createWithServer(cipher); const decCipher = await newCipher.decrypt( await this.cipherService.getKeyForCipherKeyDecryption(newCipher), ); @@ -142,12 +141,11 @@ export class CreateCommand { } try { - await this.cipherService.saveAttachmentRawWithServer( + const updatedCipher = await this.cipherService.saveAttachmentRawWithServer( cipher, fileName, new Uint8Array(fileBuf).buffer, ); - const updatedCipher = await this.cipherService.get(cipher.id); const decCipher = await updatedCipher.decrypt( await this.cipherService.getKeyForCipherKeyDecryption(updatedCipher), ); diff --git a/libs/common/src/vault/abstractions/cipher.service.ts b/libs/common/src/vault/abstractions/cipher.service.ts index 501fd87665..22e2c54a59 100644 --- a/libs/common/src/vault/abstractions/cipher.service.ts +++ b/libs/common/src/vault/abstractions/cipher.service.ts @@ -47,8 +47,24 @@ export abstract class CipherService { updateLastUsedDate: (id: string) => Promise; updateLastLaunchedDate: (id: string) => Promise; saveNeverDomain: (domain: string) => Promise; - createWithServer: (cipher: Cipher, orgAdmin?: boolean) => Promise; - updateWithServer: (cipher: Cipher, orgAdmin?: boolean, isNotClone?: boolean) => Promise; + /** + * Create a cipher with the server + * + * @param cipher The cipher to create + * @param orgAdmin If true, the request is submitted as an organization admin request + * + * @returns A promise that resolves to the created cipher + */ + createWithServer: (cipher: Cipher, orgAdmin?: boolean) => Promise; + /** + * Update a cipher with the server + * @param cipher The cipher to update + * @param orgAdmin If true, the request is submitted as an organization admin request + * @param isNotClone If true, the cipher is not a clone and should be treated as a new cipher + * + * @returns A promise that resolves to the updated cipher + */ + updateWithServer: (cipher: Cipher, orgAdmin?: boolean, isNotClone?: boolean) => Promise; shareWithServer: ( cipher: CipherView, organizationId: string, @@ -70,7 +86,14 @@ export abstract class CipherService { data: ArrayBuffer, admin?: boolean, ) => Promise; - saveCollectionsWithServer: (cipher: Cipher) => Promise; + /** + * Save the collections for a cipher with the server + * + * @param cipher The cipher to save collections for + * + * @returns A promise that resolves when the collections have been saved + */ + saveCollectionsWithServer: (cipher: Cipher) => Promise; /** * Bulk update collections for many ciphers with the server * @param orgId @@ -84,7 +107,13 @@ export abstract class CipherService { collectionIds: CollectionId[], removeCollections: boolean, ) => Promise; - upsert: (cipher: CipherData | CipherData[]) => Promise; + /** + * Update the local store of CipherData with the provided data. Values are upserted into the existing store. + * + * @param cipher The cipher data to upsert. Can be a single CipherData object or an array of CipherData objects. + * @returns A promise that resolves to a record of updated cipher store, keyed by their cipher ID. Returns all ciphers, not just those updated + */ + upsert: (cipher: CipherData | CipherData[]) => Promise>; replace: (ciphers: { [id: string]: CipherData }) => Promise; clear: (userId: string) => Promise; moveManyWithServer: (ids: string[], folderId: string) => Promise; diff --git a/libs/common/src/vault/services/cipher.service.spec.ts b/libs/common/src/vault/services/cipher.service.spec.ts index 28c4bfc653..9b03753118 100644 --- a/libs/common/src/vault/services/cipher.service.spec.ts +++ b/libs/common/src/vault/services/cipher.service.spec.ts @@ -174,23 +174,20 @@ 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)); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - cipherService.createWithServer(cipherObj, true); + .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); + await cipherService.createWithServer(cipherObj, true); const expectedObj = new CipherCreateRequest(cipherObj); 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; const spy = jest .spyOn(apiService, "postCipher") - .mockImplementation(() => Promise.resolve(cipherObj)); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - cipherService.createWithServer(cipherObj, true); + .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); + await cipherService.createWithServer(cipherObj, true); const expectedObj = new CipherRequest(cipherObj); expect(spy).toHaveBeenCalled(); @@ -201,10 +198,8 @@ describe("Cipher Service", () => { cipherObj.collectionIds = ["123"]; const spy = jest .spyOn(apiService, "postCipherCreate") - .mockImplementation(() => Promise.resolve(cipherObj)); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - cipherService.createWithServer(cipherObj); + .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); + await cipherService.createWithServer(cipherObj); const expectedObj = new CipherCreateRequest(cipherObj); expect(spy).toHaveBeenCalled(); @@ -214,10 +209,8 @@ 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)); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - cipherService.createWithServer(cipherObj); + .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); + await cipherService.createWithServer(cipherObj); const expectedObj = new CipherRequest(cipherObj); expect(spy).toHaveBeenCalled(); @@ -229,10 +222,8 @@ describe("Cipher Service", () => { it("should call apiService.putCipherAdmin when orgAdmin and isNotClone params are true", async () => { const spy = jest .spyOn(apiService, "putCipherAdmin") - .mockImplementation(() => Promise.resolve(cipherObj)); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - cipherService.updateWithServer(cipherObj, true, true); + .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); + await cipherService.updateWithServer(cipherObj, true, true); const expectedObj = new CipherRequest(cipherObj); expect(spy).toHaveBeenCalled(); @@ -243,10 +234,8 @@ describe("Cipher Service", () => { cipherObj.edit = true; const spy = jest .spyOn(apiService, "putCipher") - .mockImplementation(() => Promise.resolve(cipherObj)); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - cipherService.updateWithServer(cipherObj); + .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); + await cipherService.updateWithServer(cipherObj); const expectedObj = new CipherRequest(cipherObj); expect(spy).toHaveBeenCalled(); @@ -257,10 +246,8 @@ describe("Cipher Service", () => { cipherObj.edit = false; const spy = jest .spyOn(apiService, "putPartialCipher") - .mockImplementation(() => Promise.resolve(cipherObj)); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - cipherService.updateWithServer(cipherObj); + .mockImplementation(() => Promise.resolve(cipherObj.toCipherData())); + await cipherService.updateWithServer(cipherObj); const expectedObj = new CipherPartialRequest(cipherObj); expect(spy).toHaveBeenCalled(); diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 7d06b3185f..4a13196c9c 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -573,7 +573,7 @@ export class CipherService implements CipherServiceAbstraction { await this.domainSettingsService.setNeverDomains(domains); } - async createWithServer(cipher: Cipher, orgAdmin?: boolean): Promise { + async createWithServer(cipher: Cipher, orgAdmin?: boolean): Promise { let response: CipherResponse; if (orgAdmin && cipher.organizationId != null) { const request = new CipherCreateRequest(cipher); @@ -588,10 +588,16 @@ export class CipherService implements CipherServiceAbstraction { cipher.id = response.id; const data = new CipherData(response, cipher.collectionIds); - await this.upsert(data); + const updated = await this.upsert(data); + // No local data for new ciphers + return new Cipher(updated[cipher.id as CipherId]); } - async updateWithServer(cipher: Cipher, orgAdmin?: boolean, isNotClone?: boolean): Promise { + async updateWithServer( + cipher: Cipher, + orgAdmin?: boolean, + isNotClone?: boolean, + ): Promise { let response: CipherResponse; if (orgAdmin && isNotClone) { const request = new CipherRequest(cipher); @@ -605,7 +611,9 @@ export class CipherService implements CipherServiceAbstraction { } const data = new CipherData(response, cipher.collectionIds); - await this.upsert(data); + const updated = await this.upsert(data); + // updating with server does not change local data + return new Cipher(updated[cipher.id as CipherId], cipher.localData); } async shareWithServer( @@ -732,11 +740,13 @@ export class CipherService implements CipherServiceAbstraction { return new Cipher(cData); } - async saveCollectionsWithServer(cipher: Cipher): Promise { + async saveCollectionsWithServer(cipher: Cipher): Promise { const request = new CipherCollectionsRequest(cipher.collectionIds); const response = await this.apiService.putCipherCollections(cipher.id, request); const data = new CipherData(response); - await this.upsert(data); + const updated = await this.upsert(data); + // Collection updates don't change local data + return new Cipher(updated[cipher.id as CipherId], cipher.localData); } /** @@ -782,9 +792,9 @@ export class CipherService implements CipherServiceAbstraction { await this.encryptedCiphersState.update(() => ciphers); } - async upsert(cipher: CipherData | CipherData[]): Promise { + async upsert(cipher: CipherData | CipherData[]): Promise> { const ciphers = cipher instanceof CipherData ? [cipher] : cipher; - await this.updateEncryptedCipherState((current) => { + return await this.updateEncryptedCipherState((current) => { ciphers.forEach((c) => (current[c.id as CipherId] = c)); return current; }); @@ -796,12 +806,13 @@ export class CipherService implements CipherServiceAbstraction { private async updateEncryptedCipherState( update: (current: Record) => Record, - ) { + ): Promise> { await this.clearDecryptedCiphersState(); - await this.encryptedCiphersState.update((current) => { + const [, updatedCiphers] = await this.encryptedCiphersState.update((current) => { const result = update(current ?? {}); return result; }); + return updatedCiphers; } async clear(userId?: string): Promise {