[PM-1071] Display import-details-dialog on successful import (#4817)

* Prefer callback over error-flow to prompt for password

Remove error-flow to request file password
Prefer callback, which has to be provided when retrieving/creating an instance.
Delete ImportError
Call BitwardenPasswordProtector for all Bitwarden json imports, as it extends BitwardenJsonImporter
Throw errors instead of returning
Return ImportResult
Fix and extend tests import.service
Replace "@fluffy-spoon/substitute" with "jest-mock-extended"

* Fix up test cases

Delete bitwarden-json-importer.spec.ts
Add test case to ensure bitwarden-json-importer.ts is called given unencrypted or account-protected files

* Move file-password-prompt into dialog-folder

* Add import success dialog

* Fix typo

* Only list the type when at least one got imported

* update copy based on design feedback

* Remove unnecessary /index import

* Remove promptForPassword_callback from interface

PR feedback from @MGibson1 that giving every importer the ability to request a password is unnecessary. Instead, we can pass the callback into the constructor for every importer that needs this functionality

* Remove unneeded import of BitwardenJsonImporter

* Fix spec constructor

* Fixed organizational import

Added an else statement, or else we'd import into an org and then also import into an individual vault

libs/importer/src/importers/bitwarden/bitwarden-json-importer.ts

@@ -13,7 +13,10 @@ export class BitwardenJsonImporter extends BaseImporter implements Importer {
   private results: any;
   private result: ImportResult;
 
-  constructor(protected cryptoService: CryptoService, protected i18nService: I18nService) {
+  protected constructor(
+    protected cryptoService: CryptoService,
+    protected i18nService: I18nService
+  ) {
     super();
   }
 
@@ -21,13 +24,6 @@ export class BitwardenJsonImporter extends BaseImporter implements Importer {
     this.result = new ImportResult();
     this.results = JSON.parse(data);
     if (this.results == null || this.results.items == null) {
-      if (this.results?.passwordProtected) {
-        this.result.success = false;
-        this.result.missingPassword = true;
-        this.result.errorMessage = this.i18nService.t("importPasswordRequired");
-        return this.result;
-      }
-
       this.result.success = false;
       return this.result;
     }

libs/importer/src/importers/bitwarden/bitwarden-password-protected-importer.ts

@@ -13,19 +13,41 @@ import { BitwardenPasswordProtectedFileFormat } from "./bitwarden-password-prote
 export class BitwardenPasswordProtectedImporter extends BitwardenJsonImporter implements Importer {
   private key: SymmetricCryptoKey;
 
-  constructor(cryptoService: CryptoService, i18nService: I18nService, private password: string) {
+  constructor(
+    cryptoService: CryptoService,
+    i18nService: I18nService,
+    private promptForPassword_callback: () => Promise<string>
+  ) {
     super(cryptoService, i18nService);
   }
 
   async parse(data: string): Promise<ImportResult> {
     const result = new ImportResult();
-    const parsedData = JSON.parse(data);
+    const parsedData: BitwardenPasswordProtectedFileFormat = JSON.parse(data);
+
+    if (!parsedData) {
+      result.success = false;
+      return result;
+    }
+
+    // File is unencrypted
+    if (!parsedData?.encrypted) {
+      return await super.parse(data);
+    }
+
+    // File is account-encrypted
+    if (!parsedData?.passwordProtected) {
+      return await super.parse(data);
+    }
+
     if (this.cannotParseFile(parsedData)) {
       result.success = false;
       return result;
     }
 
-    if (!(await this.checkPassword(parsedData))) {
+    // File is password-protected
+    const password = await this.promptForPassword_callback();
+    if (!(await this.checkPassword(parsedData, password))) {
       result.success = false;
       result.errorMessage = this.i18nService.t("invalidFilePassword");
       return result;
@@ -36,9 +58,12 @@ export class BitwardenPasswordProtectedImporter extends BitwardenJsonImporter im
     return await super.parse(clearTextData);
   }
 
-  private async checkPassword(jdoc: BitwardenPasswordProtectedFileFormat): Promise<boolean> {
+  private async checkPassword(
+    jdoc: BitwardenPasswordProtectedFileFormat,
+    password: string
+  ): Promise<boolean> {
     this.key = await this.cryptoService.makePinKey(
-      this.password,
+      password,
       jdoc.salt,
       jdoc.kdfType,
       new KdfConfig(jdoc.kdfIterations, jdoc.kdfMemory, jdoc.kdfParallelism)

libs/importer/src/index.ts

@@ -1,6 +1,6 @@
 export { ImportType, ImportOption } from "./models/import-options";
 
-export { ImportError } from "./models/import-error";
+export { ImportResult } from "./models/import-result";
 
 export { ImportApiServiceAbstraction } from "./services/import-api.service.abstraction";
 export { ImportApiService } from "./services/import-api.service";

libs/importer/src/models/import-error.ts

@@ -1,5 +0,0 @@
-export class ImportError extends Error {
-  constructor(message?: string, public passwordRequired: boolean = false) {
-    super(message);
-  }
-}

libs/importer/src/models/import-result.ts

@@ -4,7 +4,6 @@ import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";
 
 export class ImportResult {
   success = false;
-  missingPassword = false;
   errorMessage: string;
   ciphers: CipherView[] = [];
   folders: FolderView[] = [];

libs/importer/src/services/import.service.abstraction.ts

@@ -1,6 +1,6 @@
 import { Importer } from "../importers/importer";
-import { ImportError } from "../models/import-error";
 import { ImportOption, ImportType } from "../models/import-options";
+import { ImportResult } from "../models/import-result";
 
 export abstract class ImportServiceAbstraction {
   featuredImportOptions: readonly ImportOption[];
@@ -10,10 +10,10 @@ export abstract class ImportServiceAbstraction {
     importer: Importer,
     fileContents: string,
     organizationId?: string
-  ) => Promise<ImportError>;
+  ) => Promise<ImportResult>;
   getImporter: (
     format: ImportType | "bitwardenpasswordprotected",
-    organizationId: string,
-    password?: string
+    promptForPassword_callback: () => Promise<string>,
+    organizationId: string
   ) => Importer;
 }

libs/importer/src/services/import.service.spec.ts

@@ -1,5 +1,4 @@
-// eslint-disable-next-line no-restricted-imports
-import { Substitute, SubstituteOf } from "@fluffy-spoon/substitute";
+import { mock, MockProxy } from "jest-mock-extended";
 
 import { CryptoService } from "@bitwarden/common/abstractions/crypto.service";
 import { I18nService } from "@bitwarden/common/abstractions/i18n.service";
@@ -16,20 +15,20 @@ import { ImportService } from "./import.service";
 
 describe("ImportService", () => {
   let importService: ImportService;
-  let cipherService: SubstituteOf<CipherService>;
-  let folderService: SubstituteOf<FolderService>;
-  let importApiService: SubstituteOf<ImportApiServiceAbstraction>;
-  let i18nService: SubstituteOf<I18nService>;
-  let collectionService: SubstituteOf<CollectionService>;
-  let cryptoService: SubstituteOf<CryptoService>;
+  let cipherService: MockProxy<CipherService>;
+  let folderService: MockProxy<FolderService>;
+  let importApiService: MockProxy<ImportApiServiceAbstraction>;
+  let i18nService: MockProxy<I18nService>;
+  let collectionService: MockProxy<CollectionService>;
+  let cryptoService: MockProxy<CryptoService>;
 
   beforeEach(() => {
-    cipherService = Substitute.for<CipherService>();
-    folderService = Substitute.for<FolderService>();
-    importApiService = Substitute.for<ImportApiServiceAbstraction>();
-    i18nService = Substitute.for<I18nService>();
-    collectionService = Substitute.for<CollectionService>();
-    cryptoService = Substitute.for<CryptoService>();
+    cipherService = mock<CipherService>();
+    folderService = mock<FolderService>();
+    importApiService = mock<ImportApiServiceAbstraction>();
+    i18nService = mock<I18nService>();
+    collectionService = mock<CollectionService>();
+    cryptoService = mock<CryptoService>();
 
     importService = new ImportService(
       cipherService,
@@ -46,12 +45,15 @@ describe("ImportService", () => {
       let importer: Importer;
       const organizationId = Utils.newGuid();
       const password = Utils.newGuid();
+      const promptForPassword_callback = async () => {
+        return password;
+      };
 
       beforeEach(() => {
         importer = importService.getImporter(
           "bitwardenpasswordprotected",
-          organizationId,
-          password
+          promptForPassword_callback,
+          organizationId
         );
       });
 
@@ -59,12 +61,13 @@ describe("ImportService", () => {
         expect(importer).toBeInstanceOf(BitwardenPasswordProtectedImporter);
       });
 
-      it("has the appropriate organization Id", () => {
-        expect(importer.organizationId).toEqual(organizationId);
+      it("has the promptForPassword_callback set", async () => {
+        expect(importer.promptForPassword_callback).not.toBeNull();
+        expect(await importer.promptForPassword_callback()).toEqual(password);
       });
 
-      it("has the appropriate password", () => {
-        expect(Object.entries(importer)).toEqual(expect.arrayContaining([["password", password]]));
+      it("has the appropriate organization Id", () => {
+        expect(importer.organizationId).toEqual(organizationId);
       });
     });
   });

libs/importer/src/services/import.service.ts

@@ -20,7 +20,6 @@ import {
   AvastJsonImporter,
   AviraCsvImporter,
   BitwardenCsvImporter,
-  BitwardenJsonImporter,
   BitwardenPasswordProtectedImporter,
   BlackBerryCsvImporter,
   BlurCsvImporter,
@@ -76,7 +75,6 @@ import {
   ZohoVaultCsvImporter,
 } from "../importers";
 import { Importer } from "../importers/importer";
-import { ImportError } from "../models/import-error";
 import {
   featuredImportOptions,
   ImportOption,
@@ -109,48 +107,55 @@ export class ImportService implements ImportServiceAbstraction {
     importer: Importer,
     fileContents: string,
     organizationId: string = null
-  ): Promise<ImportError> {
+  ): Promise<ImportResult> {
     const importResult = await importer.parse(fileContents);
-    if (importResult.success) {
-      if (importResult.folders.length === 0 && importResult.ciphers.length === 0) {
-        return new ImportError(this.i18nService.t("importNothingError"));
-      } else if (importResult.ciphers.length > 0) {
-        const halfway = Math.floor(importResult.ciphers.length / 2);
-        const last = importResult.ciphers.length - 1;
-
-        if (
-          this.badData(importResult.ciphers[0]) &&
-          this.badData(importResult.ciphers[halfway]) &&
-          this.badData(importResult.ciphers[last])
-        ) {
-          return new ImportError(this.i18nService.t("importFormatError"));
-        }
-      }
-      try {
-        await this.postImport(importResult, organizationId);
-      } catch (error) {
-        const errorResponse = new ErrorResponse(error, 400);
-        return this.handleServerError(errorResponse, importResult);
-      }
-      return null;
-    } else {
+    if (!importResult.success) {
       if (!Utils.isNullOrWhitespace(importResult.errorMessage)) {
-        return new ImportError(importResult.errorMessage, importResult.missingPassword);
-      } else {
-        return new ImportError(
-          this.i18nService.t("importFormatError"),
-          importResult.missingPassword
-        );
+        throw new Error(importResult.errorMessage);
+      }
+      throw new Error(this.i18nService.t("importFormatError"));
+    }
+
+    if (importResult.folders.length === 0 && importResult.ciphers.length === 0) {
+      throw new Error(this.i18nService.t("importNothingError"));
+    }
+
+    if (importResult.ciphers.length > 0) {
+      const halfway = Math.floor(importResult.ciphers.length / 2);
+      const last = importResult.ciphers.length - 1;
+
+      if (
+        this.badData(importResult.ciphers[0]) &&
+        this.badData(importResult.ciphers[halfway]) &&
+        this.badData(importResult.ciphers[last])
+      ) {
+        throw new Error(this.i18nService.t("importFormatError"));
       }
     }
+
+    try {
+      if (organizationId != null) {
+        await this.handleOrganizationalImport(importResult, organizationId);
+      } else {
+        await this.handleIndividualImport(importResult);
+      }
+    } catch (error) {
+      const errorResponse = new ErrorResponse(error, 400);
+      throw this.handleServerError(errorResponse, importResult);
+    }
+    return importResult;
   }
 
   getImporter(
     format: ImportType | "bitwardenpasswordprotected",
-    organizationId: string = null,
-    password: string = null
+    promptForPassword_callback: () => Promise<string>,
+    organizationId: string = null
   ): Importer {
-    const importer = this.getImporterInstance(format, password);
+    if (promptForPassword_callback == null) {
+      return null;
+    }
+
+    const importer = this.getImporterInstance(format, promptForPassword_callback);
     if (importer == null) {
       return null;
     }
@@ -158,7 +163,10 @@ export class ImportService implements ImportServiceAbstraction {
     return importer;
   }
 
-  private getImporterInstance(format: ImportType | "bitwardenpasswordprotected", password: string) {
+  private getImporterInstance(
+    format: ImportType | "bitwardenpasswordprotected",
+    promptForPassword_callback: () => Promise<string>
+  ) {
     if (format == null) {
       return null;
     }
@@ -167,12 +175,11 @@ export class ImportService implements ImportServiceAbstraction {
       case "bitwardencsv":
         return new BitwardenCsvImporter();
       case "bitwardenjson":
-        return new BitwardenJsonImporter(this.cryptoService, this.i18nService);
       case "bitwardenpasswordprotected":
         return new BitwardenPasswordProtectedImporter(
           this.cryptoService,
           this.i18nService,
-          password
+          promptForPassword_callback
         );
       case "lastpasscsv":
       case "passboltcsv":
@@ -294,46 +301,46 @@ export class ImportService implements ImportServiceAbstraction {
     }
   }
 
-  private async postImport(importResult: ImportResult, organizationId: string = null) {
-    if (organizationId == null) {
-      const request = new ImportCiphersRequest();
-      for (let i = 0; i < importResult.ciphers.length; i++) {
-        const c = await this.cipherService.encrypt(importResult.ciphers[i]);
-        request.ciphers.push(new CipherRequest(c));
-      }
-      if (importResult.folders != null) {
-        for (let i = 0; i < importResult.folders.length; i++) {
-          const f = await this.folderService.encrypt(importResult.folders[i]);
-          request.folders.push(new FolderWithIdRequest(f));
-        }
-      }
-      if (importResult.folderRelationships != null) {
-        importResult.folderRelationships.forEach((r) =>
-          request.folderRelationships.push(new KvpRequest(r[0], r[1]))
-        );
-      }
-      return await this.importApiService.postImportCiphers(request);
-    } else {
-      const request = new ImportOrganizationCiphersRequest();
-      for (let i = 0; i < importResult.ciphers.length; i++) {
-        importResult.ciphers[i].organizationId = organizationId;
-        const c = await this.cipherService.encrypt(importResult.ciphers[i]);
-        request.ciphers.push(new CipherRequest(c));
-      }
-      if (importResult.collections != null) {
-        for (let i = 0; i < importResult.collections.length; i++) {
-          importResult.collections[i].organizationId = organizationId;
-          const c = await this.collectionService.encrypt(importResult.collections[i]);
-          request.collections.push(new CollectionWithIdRequest(c));
-        }
-      }
-      if (importResult.collectionRelationships != null) {
-        importResult.collectionRelationships.forEach((r) =>
-          request.collectionRelationships.push(new KvpRequest(r[0], r[1]))
-        );
-      }
-      return await this.importApiService.postImportOrganizationCiphers(organizationId, request);
+  private async handleIndividualImport(importResult: ImportResult) {
+    const request = new ImportCiphersRequest();
+    for (let i = 0; i < importResult.ciphers.length; i++) {
+      const c = await this.cipherService.encrypt(importResult.ciphers[i]);
+      request.ciphers.push(new CipherRequest(c));
     }
+    if (importResult.folders != null) {
+      for (let i = 0; i < importResult.folders.length; i++) {
+        const f = await this.folderService.encrypt(importResult.folders[i]);
+        request.folders.push(new FolderWithIdRequest(f));
+      }
+    }
+    if (importResult.folderRelationships != null) {
+      importResult.folderRelationships.forEach((r) =>
+        request.folderRelationships.push(new KvpRequest(r[0], r[1]))
+      );
+    }
+    return await this.importApiService.postImportCiphers(request);
+  }
+
+  private async handleOrganizationalImport(importResult: ImportResult, organizationId: string) {
+    const request = new ImportOrganizationCiphersRequest();
+    for (let i = 0; i < importResult.ciphers.length; i++) {
+      importResult.ciphers[i].organizationId = organizationId;
+      const c = await this.cipherService.encrypt(importResult.ciphers[i]);
+      request.ciphers.push(new CipherRequest(c));
+    }
+    if (importResult.collections != null) {
+      for (let i = 0; i < importResult.collections.length; i++) {
+        importResult.collections[i].organizationId = organizationId;
+        const c = await this.collectionService.encrypt(importResult.collections[i]);
+        request.collections.push(new CollectionWithIdRequest(c));
+      }
+    }
+    if (importResult.collectionRelationships != null) {
+      importResult.collectionRelationships.forEach((r) =>
+        request.collectionRelationships.push(new KvpRequest(r[0], r[1]))
+      );
+    }
+    return await this.importApiService.postImportOrganizationCiphers(organizationId, request);
   }
 
   private badData(c: CipherView) {
@@ -345,9 +352,9 @@ export class ImportService implements ImportServiceAbstraction {
     );
   }
 
-  private handleServerError(errorResponse: ErrorResponse, importResult: ImportResult): ImportError {
+  private handleServerError(errorResponse: ErrorResponse, importResult: ImportResult): Error {
     if (errorResponse.validationErrors == null) {
-      return new ImportError(errorResponse.message);
+      return new Error(errorResponse.message);
     }
 
     let errorMessage = "";
@@ -385,6 +392,6 @@ export class ImportService implements ImportServiceAbstraction {
       errorMessage += "[" + itemType + '] "' + item.name + '": ' + value;
     });
 
-    return new ImportError(errorMessage);
+    return new Error(errorMessage);
   }
 }