Vault/pm-7580/resolve-cipher-update-race (#8806)

* Resolve updated values from updates

Uses the now returned updated values from cipher service to guarantee-return the updated cipher for CLI edits

* Use updated cipher for creation

* Use updated cipher for editing collections

* Await async methods

Cipher data more closely approximates server responses. TODO: this should really use actual response types

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.updateWithServer(encCipher);
-      const updatedCipher = await this.cipherService.get(cipher.id);
       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.saveCollectionsWithServer(cipher);
-      const updatedCipher = await this.cipherService.get(cipher.id);
       const decCipher = await updatedCipher.decrypt(
         await this.cipherService.getKeyForCipherKeyDecryption(updatedCipher),
       );

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.createWithServer(cipher);
-      const newCipher = await this.cipherService.get(cipher.id);
       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),
       );

libs/common/src/vault/abstractions/cipher.service.ts

@@ -47,8 +47,24 @@ export abstract class CipherService {
   updateLastUsedDate: (id: string) => Promise<void>;
   updateLastLaunchedDate: (id: string) => Promise<void>;
   saveNeverDomain: (domain: string) => Promise<void>;
-  createWithServer: (cipher: Cipher, orgAdmin?: boolean) => Promise<any>;
+  /**
-  updateWithServer: (cipher: Cipher, orgAdmin?: boolean, isNotClone?: boolean) => Promise<any>;
+   * 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<Cipher>;
+  /**
+   * 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<Cipher>;
   shareWithServer: (
     cipher: CipherView,
     organizationId: string,
@@ -70,7 +86,14 @@ export abstract class CipherService {
     data: ArrayBuffer,
     admin?: boolean,
   ) => Promise<Cipher>;
-  saveCollectionsWithServer: (cipher: Cipher) => Promise<any>;
+  /**
+   * 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<Cipher>;
   /**
    * Bulk update collections for many ciphers with the server
    * @param orgId
@@ -84,7 +107,13 @@ export abstract class CipherService {
     collectionIds: CollectionId[],
     removeCollections: boolean,
   ) => Promise<void>;
-  upsert: (cipher: CipherData | CipherData[]) => Promise<any>;
+  /**
+   * 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<Record<CipherId, CipherData>>;
   replace: (ciphers: { [id: string]: CipherData }) => Promise<any>;
   clear: (userId: string) => Promise<any>;
   moveManyWithServer: (ids: string[], folderId: string) => Promise<any>;

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<any>(cipherObj));
+        .mockImplementation(() => Promise.resolve<any>(cipherObj.toCipherData()));
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      await cipherService.createWithServer(cipherObj, true);
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      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<any>(cipherObj));
+        .mockImplementation(() => Promise.resolve<any>(cipherObj.toCipherData()));
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      await cipherService.createWithServer(cipherObj, true);
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      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<any>(cipherObj));
+        .mockImplementation(() => Promise.resolve<any>(cipherObj.toCipherData()));
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      await cipherService.createWithServer(cipherObj);
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      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<any>(cipherObj));
+        .mockImplementation(() => Promise.resolve<any>(cipherObj.toCipherData()));
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      await cipherService.createWithServer(cipherObj);
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      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<any>(cipherObj));
+        .mockImplementation(() => Promise.resolve<any>(cipherObj.toCipherData()));
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      await cipherService.updateWithServer(cipherObj, true, true);
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      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<any>(cipherObj));
+        .mockImplementation(() => Promise.resolve<any>(cipherObj.toCipherData()));
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      await cipherService.updateWithServer(cipherObj);
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      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<any>(cipherObj));
+        .mockImplementation(() => Promise.resolve<any>(cipherObj.toCipherData()));
-      // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
+      await cipherService.updateWithServer(cipherObj);
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      cipherService.updateWithServer(cipherObj);
       const expectedObj = new CipherPartialRequest(cipherObj);
 
       expect(spy).toHaveBeenCalled();

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<any> {
+  async createWithServer(cipher: Cipher, orgAdmin?: boolean): Promise<Cipher> {
     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<any> {
+  async updateWithServer(
+    cipher: Cipher,
+    orgAdmin?: boolean,
+    isNotClone?: boolean,
+  ): Promise<Cipher> {
     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<any> {
+  async saveCollectionsWithServer(cipher: Cipher): Promise<Cipher> {
     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<any> {
+  async upsert(cipher: CipherData | CipherData[]): Promise<Record<CipherId, CipherData>> {
     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<CipherId, CipherData>) => Record<CipherId, CipherData>,
-  ) {
+  ): Promise<Record<CipherId, CipherData>> {
     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<any> {