[PM-27642] - fix mp reprompt issue (#17131)

* fix mp reprompt issue * fix logic. add more specs * update vault popup autofill spec * update jsdoc
2025-12-06 00:13:28 +00:00 · 2025-10-30 13:19:12 -07:00
parent 0d543c7059
commit 4a71503875
4 changed files with 180 additions and 106 deletions
--- a/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.spec.ts
+++ b/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.spec.ts
@@ -153,99 +153,14 @@ describe("ItemMoreOptionsComponent", () => {
      expect(autofillSvc.doAutofill).toHaveBeenCalledTimes(1);
      expect(autofillSvc.doAutofill).toHaveBeenCalledWith(
        expect.objectContaining({ id: "cipher-1" }),
-        false,
+        true,
+        true,
      );
      expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
      expect(dialogService.openSimpleDialog).not.toHaveBeenCalled();
    });

-    it("opens the autofill confirmation dialog with filtered saved URLs when the feature flag is enabled and search text is present", async () => {
-      featureFlag$.next(true);
-      hasSearchText$.next(true);
-      autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com/path" });
-      const openSpy = mockConfirmDialogResult(AutofillConfirmationDialogResult.Canceled);
-
-      await component.doAutofill();
-
-      expect(openSpy).toHaveBeenCalledTimes(1);
-      const args = openSpy.mock.calls[0][1];
-      expect(args.data.currentUrl).toBe("https://page.example.com/path");
-      expect(args.data.savedUrls).toEqual(["https://one.example.com", "https://two.example.com/a"]);
-    });
-
-    it("does nothing when the user cancels the autofill confirmation dialog", async () => {
-      featureFlag$.next(true);
-      autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
-      mockConfirmDialogResult(AutofillConfirmationDialogResult.Canceled);
-
-      await component.doAutofill();
-
-      expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
-      expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
-    });
-
-    it("calls the autofill service to autofill when the user selects 'AutofilledOnly'", async () => {
-      featureFlag$.next(true);
-      autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
-      mockConfirmDialogResult(AutofillConfirmationDialogResult.AutofilledOnly);
-
-      await component.doAutofill();
-
-      expect(autofillSvc.doAutofill).toHaveBeenCalledTimes(1);
-      expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
-    });
-
-    it("calls the autofill service to doAutofillAndSave when the user selects 'AutofillAndUrlAdded'", async () => {
-      featureFlag$.next(true);
-      autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
-      mockConfirmDialogResult(AutofillConfirmationDialogResult.AutofillAndUrlAdded);
-
-      await component.doAutofill();
-
-      expect(autofillSvc.doAutofillAndSave).toHaveBeenCalledTimes(1);
-      expect(autofillSvc.doAutofillAndSave.mock.calls[0][1]).toBe(false);
-      expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
-    });
-
-    it("shows the exact match dialog when the uri match strategy is Exact and no URIs match", async () => {
-      featureFlag$.next(true);
-      uriMatchStrategy$.next(UriMatchStrategy.Exact);
-      hasSearchText$.next(true);
-      autofillSvc.currentAutofillTab$.next({ url: "https://no-match.example.com" });
-
-      await component.doAutofill();
-
-      expect(dialogService.openSimpleDialog).toHaveBeenCalledTimes(1);
-      expect(dialogService.openSimpleDialog).toHaveBeenCalledWith(
-        expect.objectContaining({
-          title: expect.objectContaining({ key: "cannotAutofill" }),
-          content: expect.objectContaining({ key: "cannotAutofillExactMatch" }),
-          type: "info",
-        }),
-      );
-      expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
-      expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
-    });
-
-    it("hides the 'Fill and Save' button when showAutofillConfirmation$ is true", async () => {
-      // Enable both feature flag and search text → makes showAutofillConfirmation$ true
-      featureFlag$.next(true);
-      hasSearchText$.next(true);
-
-      fixture.detectChanges();
-      await fixture.whenStable();
-
-      const fillAndSaveButton = fixture.nativeElement.querySelector(
-        "button[bitMenuItem]:not([disabled])",
-      );
-
-      const buttonText = fillAndSaveButton?.textContent?.trim().toLowerCase() ?? "";
-      expect(buttonText.includes("fillAndSave".toLowerCase())).toBe(false);
-    });
-
-    it("call the passwordService to passwordRepromptCheck if their cipher has password reprompt enabled", async () => {
-      baseCipher.reprompt = 2; // Master Password reprompt enabled
-      featureFlag$.next(true);
+    it("calls the passwordService to passwordRepromptCheck", async () => {
      autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
      mockConfirmDialogResult(AutofillConfirmationDialogResult.AutofilledOnly);

@@ -256,7 +171,6 @@ describe("ItemMoreOptionsComponent", () => {

    it("does nothing if the user fails master password reprompt", async () => {
      baseCipher.reprompt = 2; // Master Password reprompt enabled
-      featureFlag$.next(true);
      autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
      passwordRepromptService.passwordRepromptCheck.mockResolvedValue(false); // Reprompt fails
      mockConfirmDialogResult(AutofillConfirmationDialogResult.AutofilledOnly);
@@ -266,5 +180,132 @@ describe("ItemMoreOptionsComponent", () => {
      expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
      expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
    });
+
+    describe("autofill confirmation dialog", () => {
+      beforeEach(() => {
+        featureFlag$.next(true);
+        hasSearchText$.next(true);
+        passwordRepromptService.passwordRepromptCheck.mockResolvedValue(true);
+      });
+
+      it("opens the autofill confirmation dialog with filtered saved URLs when the feature flag is enabled and search text is present", async () => {
+        autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com/path" });
+        const openSpy = mockConfirmDialogResult(AutofillConfirmationDialogResult.Canceled);
+
+        await component.doAutofill();
+
+        expect(openSpy).toHaveBeenCalledTimes(1);
+        const args = openSpy.mock.calls[0][1];
+        expect(args.data.currentUrl).toBe("https://page.example.com/path");
+        expect(args.data.savedUrls).toEqual([
+          "https://one.example.com",
+          "https://two.example.com/a",
+        ]);
+      });
+
+      it("does nothing when the user cancels the autofill confirmation dialog", async () => {
+        autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
+        mockConfirmDialogResult(AutofillConfirmationDialogResult.Canceled);
+
+        await component.doAutofill();
+
+        expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
+        expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
+      });
+
+      it("calls the autofill service to autofill when the user selects 'AutofilledOnly'", async () => {
+        autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
+        mockConfirmDialogResult(AutofillConfirmationDialogResult.AutofilledOnly);
+
+        await component.doAutofill();
+
+        expect(autofillSvc.doAutofill).toHaveBeenCalledWith(
+          expect.objectContaining({ id: "cipher-1" }),
+          true,
+          true,
+        );
+        expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
+      });
+
+      it("calls the autofill service to doAutofillAndSave when the user selects 'AutofillAndUrlAdded'", async () => {
+        autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
+        mockConfirmDialogResult(AutofillConfirmationDialogResult.AutofillAndUrlAdded);
+
+        await component.doAutofill();
+
+        expect(autofillSvc.doAutofillAndSave).toHaveBeenCalledWith(
+          expect.objectContaining({ id: "cipher-1" }),
+          false,
+          true,
+        );
+        expect(autofillSvc.doAutofillAndSave.mock.calls[0][1]).toBe(false);
+        expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
+      });
+
+      describe("URI match strategy handling", () => {
+        it("shows the exact match dialog when the uri match strategy is Exact", async () => {
+          uriMatchStrategy$.next(UriMatchStrategy.Exact);
+          autofillSvc.currentAutofillTab$.next({ url: "https://no-match.example.com" });
+
+          await component.doAutofill();
+
+          expect(dialogService.openSimpleDialog).toHaveBeenCalledTimes(1);
+          expect(dialogService.openSimpleDialog).toHaveBeenCalledWith(
+            expect.objectContaining({
+              title: expect.objectContaining({ key: "cannotAutofill" }),
+              content: expect.objectContaining({ key: "cannotAutofillExactMatch" }),
+              type: "info",
+            }),
+          );
+          expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
+          expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
+        });
+
+        it("shows the exact match dialog and not the password reprompt dialog when the uri match strategy is Exact and the item has master password reprompt enabled", async () => {
+          uriMatchStrategy$.next(UriMatchStrategy.Exact);
+          autofillSvc.currentAutofillTab$.next({ url: "https://no-match.example.com" });
+
+          await component.doAutofill();
+
+          expect(dialogService.openSimpleDialog).toHaveBeenCalledTimes(1);
+          expect(dialogService.openSimpleDialog).toHaveBeenCalledWith(
+            expect.objectContaining({
+              title: expect.objectContaining({ key: "cannotAutofill" }),
+              content: expect.objectContaining({ key: "cannotAutofillExactMatch" }),
+              type: "info",
+            }),
+          );
+          expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
+          expect(passwordRepromptService.passwordRepromptCheck).not.toHaveBeenCalled();
+          expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
+        });
+      });
+
+      it("hides the 'Fill and Save' button when showAutofillConfirmation$ is true", async () => {
+        // Enable both feature flag and search text → makes showAutofillConfirmation$ true
+
+        fixture.detectChanges();
+        await fixture.whenStable();
+
+        const fillAndSaveButton = fixture.nativeElement.querySelector(
+          "button[bitMenuItem]:not([disabled])",
+        );
+
+        const buttonText = fillAndSaveButton?.textContent?.trim().toLowerCase() ?? "";
+        expect(buttonText.includes("fillAndSave".toLowerCase())).toBe(false);
+      });
+
+      it("does nothing if the user fails master password reprompt", async () => {
+        baseCipher.reprompt = 2; // Master Password reprompt enabled
+        autofillSvc.currentAutofillTab$.next({ url: "https://page.example.com" });
+        passwordRepromptService.passwordRepromptCheck.mockResolvedValue(false); // Reprompt fails
+        mockConfirmDialogResult(AutofillConfirmationDialogResult.AutofilledOnly);
+
+        await component.doAutofill();
+
+        expect(autofillSvc.doAutofill).not.toHaveBeenCalled();
+        expect(autofillSvc.doAutofillAndSave).not.toHaveBeenCalled();
+      });
+    });
  });
 });
--- a/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.ts
+++ b/apps/browser/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.ts
@@ -202,17 +202,6 @@ export class ItemMoreOptionsComponent {
  async doAutofill() {
    const cipher = await this.cipherService.getFullCipherView(this.cipher);

-    if (!(await this.passwordRepromptService.passwordRepromptCheck(this.cipher))) {
-      return;
-    }
-
-    const showAutofillConfirmation = await firstValueFrom(this.showAutofillConfirmation$);
-
-    if (!showAutofillConfirmation) {
-      await this.vaultPopupAutofillService.doAutofill(cipher, false);
-      return;
-    }
-
    const uriMatchStrategy = await firstValueFrom(this.uriMatchStrategy$);
    if (uriMatchStrategy === UriMatchStrategy.Exact) {
      await this.dialogService.openSimpleDialog({
@@ -225,6 +214,17 @@ export class ItemMoreOptionsComponent {
      return;
    }

+    if (!(await this.passwordRepromptService.passwordRepromptCheck(this.cipher))) {
+      return;
+    }
+
+    const showAutofillConfirmation = await firstValueFrom(this.showAutofillConfirmation$);
+
+    if (!showAutofillConfirmation) {
+      await this.vaultPopupAutofillService.doAutofill(cipher, true, true);
+      return;
+    }
+
    const currentTab = await firstValueFrom(this.vaultPopupAutofillService.currentAutofillTab$);

    if (!currentTab?.url) {
@@ -250,10 +250,10 @@ export class ItemMoreOptionsComponent {
      case AutofillConfirmationDialogResult.Canceled:
        return;
      case AutofillConfirmationDialogResult.AutofilledOnly:
-        await this.vaultPopupAutofillService.doAutofill(cipher);
+        await this.vaultPopupAutofillService.doAutofill(cipher, true, true);
        return;
      case AutofillConfirmationDialogResult.AutofillAndUrlAdded:
-        await this.vaultPopupAutofillService.doAutofillAndSave(cipher, false);
+        await this.vaultPopupAutofillService.doAutofillAndSave(cipher, false, true);
        return;
    }
  }
--- 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
@@ -262,6 +262,18 @@ describe("VaultPopupAutofillService", () => {
        );
      });

+      it("skips password prompt when skipPasswordReprompt is true", async () => {
+        mockCipher.id = "cipher-with-reprompt";
+        mockCipher.reprompt = CipherRepromptType.Password;
+        mockAutofillService.doAutoFill.mockResolvedValue(null);
+
+        const result = await service.doAutofill(mockCipher, true, true);
+
+        expect(result).toBe(true);
+        expect(mockPasswordRepromptService.showPasswordPrompt).not.toHaveBeenCalled();
+        expect(mockAutofillService.doAutoFill).toHaveBeenCalled();
+      });
+
      describe("closePopup", () => {
        beforeEach(() => {
          jest.spyOn(BrowserApi, "closePopup").mockImplementation();
--- a/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts
+++ b/apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts
@@ -231,8 +231,10 @@ export class VaultPopupAutofillService {
    cipher: CipherView,
    tab: chrome.tabs.Tab,
    pageDetails: PageDetail[],
+    skipPasswordReprompt = false,
  ): Promise<boolean> {
    if (
+      !skipPasswordReprompt &&
      cipher.reprompt !== CipherRepromptType.None &&
      !(await this.passwordRepromptService.showPasswordPrompt())
    ) {
@@ -314,12 +316,22 @@ export class VaultPopupAutofillService {
   * Will copy any TOTP code to the clipboard if available after successful autofill.
   * @param cipher
   * @param closePopup If true, will close the popup window after successful autofill. Defaults to true.
+   * @param skipPasswordReprompt If true, skips the master password reprompt even if the cipher requires it. Defaults to false.
   */
-  async doAutofill(cipher: CipherView, closePopup = true): Promise<boolean> {
+  async doAutofill(
+    cipher: CipherView,
+    closePopup = true,
+    skipPasswordReprompt = false,
+  ): Promise<boolean> {
    const tab = await firstValueFrom(this.currentAutofillTab$);
    const pageDetails = await firstValueFrom(this._currentPageDetails$);

-    const didAutofill = await this._internalDoAutofill(cipher, tab, pageDetails);
+    const didAutofill = await this._internalDoAutofill(
+      cipher,
+      tab,
+      pageDetails,
+      skipPasswordReprompt,
+    );

    if (didAutofill && closePopup) {
      await this._closePopup(cipher, tab);
@@ -350,7 +362,11 @@ export class VaultPopupAutofillService {
   * @param closePopup If true, will close the popup window after successful autofill.
   * If false, will show a success toast instead. Defaults to true.
   */
-  async doAutofillAndSave(cipher: CipherView, closePopup = true): Promise<boolean> {
+  async doAutofillAndSave(
+    cipher: CipherView,
+    closePopup = true,
+    skipPasswordReprompt = false,
+  ): Promise<boolean> {
    // We can only save URIs for login ciphers
    if (cipher.type !== CipherType.Login) {
      return false;
@@ -359,7 +375,12 @@ export class VaultPopupAutofillService {
    const pageDetails = await firstValueFrom(this._currentPageDetails$);
    const tab = await firstValueFrom(this.currentAutofillTab$);

-    const didAutofill = await this._internalDoAutofill(cipher, tab, pageDetails);
+    const didAutofill = await this._internalDoAutofill(
+      cipher,
+      tab,
+      pageDetails,
+      skipPasswordReprompt,
+    );

    if (!didAutofill) {
      return false;