From b444eed0b5b61fb66eb89d1c90ee0ded89a3f6d9 Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Wed, 30 Aug 2023 10:18:20 -0500 Subject: [PATCH] [PM-3589] Context Menu No Longer Shows Autofill Ciphers (#6085) * [PM-3589] Context Menu No Longer Shows Autofill Ciphers * [PM-3589] Ensuring that passwordless users can also access ciphers that require reprompt * [PM-3589] Fixing jest test * [PM-3589] Fixing issue where context menu autofill does not allow filling when passwordless setup is in place --- .../cipher-context-menu-handler.spec.ts | 57 +------------------ .../browser/cipher-context-menu-handler.ts | 14 +---- .../context-menu-clicked-handler.spec.ts | 5 +- .../browser/context-menu-clicked-handler.ts | 21 +++++-- .../browser/src/background/main.background.ts | 6 +- .../popup/components/vault/view.component.ts | 10 +--- 6 files changed, 32 insertions(+), 81 deletions(-) diff --git a/apps/browser/src/autofill/browser/cipher-context-menu-handler.spec.ts b/apps/browser/src/autofill/browser/cipher-context-menu-handler.spec.ts index dbe391ce4a..d7cac8d44b 100644 --- a/apps/browser/src/autofill/browser/cipher-context-menu-handler.spec.ts +++ b/apps/browser/src/autofill/browser/cipher-context-menu-handler.spec.ts @@ -1,7 +1,6 @@ import { mock, MockProxy } from "jest-mock-extended"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; -import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherRepromptType } from "@bitwarden/common/vault/enums/cipher-reprompt-type"; @@ -14,7 +13,6 @@ describe("CipherContextMenuHandler", () => { let mainContextMenuHandler: MockProxy; let authService: MockProxy; let cipherService: MockProxy; - let userVerificationService: MockProxy; let sut: CipherContextMenuHandler; @@ -22,17 +20,10 @@ describe("CipherContextMenuHandler", () => { mainContextMenuHandler = mock(); authService = mock(); cipherService = mock(); - userVerificationService = mock(); - userVerificationService.hasMasterPassword.mockResolvedValue(true); jest.spyOn(MainContextMenuHandler, "removeAll").mockResolvedValue(); - sut = new CipherContextMenuHandler( - mainContextMenuHandler, - authService, - cipherService, - userVerificationService - ); + sut = new CipherContextMenuHandler(mainContextMenuHandler, authService, cipherService); }); afterEach(() => jest.resetAllMocks()); @@ -78,7 +69,7 @@ describe("CipherContextMenuHandler", () => { expect(mainContextMenuHandler.noLogins).toHaveBeenCalledTimes(1); }); - it("only adds valid ciphers", async () => { + it("only adds login ciphers including ciphers that require reprompt", async () => { authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked); mainContextMenuHandler.init.mockResolvedValue(true); @@ -90,47 +81,6 @@ describe("CipherContextMenuHandler", () => { name: "Test Cipher", login: { username: "Test Username" }, }; - - cipherService.getAllDecryptedForUrl.mockResolvedValue([ - null, // invalid cipher - undefined, // invalid cipher - { type: CipherType.Card }, // invalid cipher - { type: CipherType.Login, reprompt: CipherRepromptType.Password }, // invalid cipher - realCipher, // valid cipher - ] as any[]); - - await sut.update("https://test.com"); - - expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledTimes(1); - - expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith("https://test.com"); - - expect(mainContextMenuHandler.loadOptions).toHaveBeenCalledTimes(2); - - expect(mainContextMenuHandler.loadOptions).toHaveBeenCalledWith( - "Test Cipher (Test Username)", - "5", - "https://test.com", - realCipher - ); - }); - - it("adds ciphers with master password reprompt if the user does not have a master password", async () => { - authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked); - - // User does not have a master password, or has one but hasn't logged in with it (key connector user or TDE user) - userVerificationService.hasMasterPasswordAndMasterKeyHash.mockResolvedValue(false); - - mainContextMenuHandler.init.mockResolvedValue(true); - - const realCipher = { - id: "5", - type: CipherType.Login, - reprompt: CipherRepromptType.None, - name: "Test Cipher", - login: { username: "Test Username" }, - }; - const repromptCipher = { id: "6", type: CipherType.Login, @@ -143,8 +93,8 @@ describe("CipherContextMenuHandler", () => { null, // invalid cipher undefined, // invalid cipher { type: CipherType.Card }, // invalid cipher - repromptCipher, // valid cipher realCipher, // valid cipher + repromptCipher, ] as any[]); await sut.update("https://test.com"); @@ -153,7 +103,6 @@ describe("CipherContextMenuHandler", () => { expect(cipherService.getAllDecryptedForUrl).toHaveBeenCalledWith("https://test.com"); - // Should call this twice, once for each valid cipher expect(mainContextMenuHandler.loadOptions).toHaveBeenCalledTimes(2); expect(mainContextMenuHandler.loadOptions).toHaveBeenCalledWith( diff --git a/apps/browser/src/autofill/browser/cipher-context-menu-handler.ts b/apps/browser/src/autofill/browser/cipher-context-menu-handler.ts index 1d1be8f838..fe6479aae5 100644 --- a/apps/browser/src/autofill/browser/cipher-context-menu-handler.ts +++ b/apps/browser/src/autofill/browser/cipher-context-menu-handler.ts @@ -1,5 +1,4 @@ import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; -import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { Utils } from "@bitwarden/common/platform/misc/utils"; @@ -12,7 +11,6 @@ import { authServiceFactory, AuthServiceInitOptions, } from "../../auth/background/service-factories/auth-service.factory"; -import { userVerificationServiceFactory } from "../../auth/background/service-factories/user-verification-service.factory"; import { Account } from "../../models/account"; import { CachedServices } from "../../platform/background/service-factories/factory-options"; import { BrowserApi } from "../../platform/browser/browser-api"; @@ -39,8 +37,7 @@ export class CipherContextMenuHandler { constructor( private mainContextMenuHandler: MainContextMenuHandler, private authService: AuthService, - private cipherService: CipherService, - private userVerificationService: UserVerificationService + private cipherService: CipherService ) {} static async create(cachedServices: CachedServices) { @@ -79,8 +76,7 @@ export class CipherContextMenuHandler { return new CipherContextMenuHandler( await MainContextMenuHandler.mv3Create(cachedServices), await authServiceFactory(cachedServices, serviceOptions), - await cipherServiceFactory(cachedServices, serviceOptions), - await userVerificationServiceFactory(cachedServices, serviceOptions) + await cipherServiceFactory(cachedServices, serviceOptions) ); } @@ -180,11 +176,7 @@ export class CipherContextMenuHandler { } private async updateForCipher(url: string, cipher: CipherView) { - if ( - cipher == null || - cipher.type !== CipherType.Login || - (await this.userVerificationService.hasMasterPasswordAndMasterKeyHash()) - ) { + if (cipher == null || cipher.type !== CipherType.Login) { return; } diff --git a/apps/browser/src/autofill/browser/context-menu-clicked-handler.spec.ts b/apps/browser/src/autofill/browser/context-menu-clicked-handler.spec.ts index a9dbcbaacc..021d15df89 100644 --- a/apps/browser/src/autofill/browser/context-menu-clicked-handler.spec.ts +++ b/apps/browser/src/autofill/browser/context-menu-clicked-handler.spec.ts @@ -3,6 +3,7 @@ import { mock, MockProxy } from "jest-mock-extended"; import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service"; import { TotpService } from "@bitwarden/common/abstractions/totp.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherRepromptType } from "@bitwarden/common/vault/enums/cipher-reprompt-type"; import { CipherType } from "@bitwarden/common/vault/enums/cipher-type"; @@ -63,6 +64,7 @@ describe("ContextMenuClickedHandler", () => { let cipherService: MockProxy; let totpService: MockProxy; let eventCollectionService: MockProxy; + let userVerificationService: MockProxy; let sut: ContextMenuClickedHandler; @@ -82,7 +84,8 @@ describe("ContextMenuClickedHandler", () => { authService, cipherService, totpService, - eventCollectionService + eventCollectionService, + userVerificationService ); }); diff --git a/apps/browser/src/autofill/browser/context-menu-clicked-handler.ts b/apps/browser/src/autofill/browser/context-menu-clicked-handler.ts index 38e605abe7..9a14ea06da 100644 --- a/apps/browser/src/autofill/browser/context-menu-clicked-handler.ts +++ b/apps/browser/src/autofill/browser/context-menu-clicked-handler.ts @@ -1,6 +1,7 @@ import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service"; import { TotpService } from "@bitwarden/common/abstractions/totp.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { EventType } from "@bitwarden/common/enums"; import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; @@ -14,6 +15,7 @@ import { AuthServiceInitOptions, } from "../../auth/background/service-factories/auth-service.factory"; import { totpServiceFactory } from "../../auth/background/service-factories/totp-service.factory"; +import { userVerificationServiceFactory } from "../../auth/background/service-factories/user-verification-service.factory"; import LockedVaultPendingNotificationsItem from "../../background/models/lockedVaultPendingNotificationsItem"; import { eventCollectionServiceFactory } from "../../background/service-factories/event-collection-service.factory"; import { Account } from "../../models/account"; @@ -56,7 +58,8 @@ export class ContextMenuClickedHandler { private authService: AuthService, private cipherService: CipherService, private totpService: TotpService, - private eventCollectionService: EventCollectionService + private eventCollectionService: EventCollectionService, + private userVerificationService: UserVerificationService ) {} static async mv3Create(cachedServices: CachedServices) { @@ -109,7 +112,8 @@ export class ContextMenuClickedHandler { await authServiceFactory(cachedServices, serviceOptions), await cipherServiceFactory(cachedServices, serviceOptions), await totpServiceFactory(cachedServices, serviceOptions), - await eventCollectionServiceFactory(cachedServices, serviceOptions) + await eventCollectionServiceFactory(cachedServices, serviceOptions), + await userVerificationServiceFactory(cachedServices, serviceOptions) ); } @@ -204,7 +208,7 @@ export class ContextMenuClickedHandler { return; } - if (cipher.reprompt !== CipherRepromptType.None) { + if (await this.isPasswordRepromptRequired(cipher)) { await BrowserApi.tabSendMessageData(tab, "passwordReprompt", { cipherId: cipher.id, action: AUTOFILL_ID, @@ -218,7 +222,7 @@ export class ContextMenuClickedHandler { this.copyToClipboard({ text: cipher.login.username, tab: tab }); break; case COPY_PASSWORD_ID: - if (cipher.reprompt !== CipherRepromptType.None) { + if (await this.isPasswordRepromptRequired(cipher)) { await BrowserApi.tabSendMessageData(tab, "passwordReprompt", { cipherId: cipher.id, action: COPY_PASSWORD_ID, @@ -230,7 +234,7 @@ export class ContextMenuClickedHandler { break; case COPY_VERIFICATIONCODE_ID: - if (cipher.reprompt !== CipherRepromptType.None) { + if (await this.isPasswordRepromptRequired(cipher)) { await BrowserApi.tabSendMessageData(tab, "passwordReprompt", { cipherId: cipher.id, action: COPY_VERIFICATIONCODE_ID, @@ -246,6 +250,13 @@ export class ContextMenuClickedHandler { } } + private async isPasswordRepromptRequired(cipher: CipherView): Promise { + return ( + cipher.reprompt === CipherRepromptType.Password && + (await this.userVerificationService.hasMasterPasswordAndMasterKeyHash()) + ); + } + private async getIdentifier(tab: chrome.tabs.Tab, info: chrome.contextMenus.OnClickData) { return new Promise((resolve, reject) => { BrowserApi.sendTabsMessage( diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 617acc2bf7..c65a869763 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -635,7 +635,8 @@ export default class MainBackground { this.authService, this.cipherService, this.totpService, - this.eventCollectionService + this.eventCollectionService, + this.userVerificationService ); this.contextMenusBackground = new ContextMenusBackground(contextMenuClickedHandler); @@ -670,8 +671,7 @@ export default class MainBackground { this.cipherContextMenuHandler = new CipherContextMenuHandler( this.mainContextMenuHandler, this.authService, - this.cipherService, - this.userVerificationService + this.cipherService ); } } diff --git a/apps/browser/src/vault/popup/components/vault/view.component.ts b/apps/browser/src/vault/popup/components/vault/view.component.ts index a70a11475c..8f45547737 100644 --- a/apps/browser/src/vault/popup/components/vault/view.component.ts +++ b/apps/browser/src/vault/popup/components/vault/view.component.ts @@ -170,8 +170,8 @@ export class ViewComponent extends BaseViewComponent { switch (this.loadAction) { case AUTOFILL_ID: - this.fillCipher(); - return; + await this.fillCipher(); + break; case COPY_USERNAME_ID: await this.copy(this.cipher.login.username, "username", "Username"); break; @@ -186,7 +186,7 @@ export class ViewComponent extends BaseViewComponent { } if (this.inPopout && this.loadAction) { - this.close(); + setTimeout(() => this.close(), 1000); } } @@ -238,10 +238,6 @@ export class ViewComponent extends BaseViewComponent { const didAutofill = await this.doAutofill(); if (didAutofill) { this.platformUtilsService.showToast("success", null, this.i18nService.t("autoFillSuccess")); - - if (this.inPopout) { - this.close(); - } } }