From 3f8f5bc1fab1f87d9376c8192c83cfec90fb2d64 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 3 Oct 2024 10:33:24 -0400 Subject: [PATCH 1/5] [PM-7535] Remove Uses of `getUserId` (#10837) * Remove Uses of `getUserId` * Fix Test --- .../settings/account-security.component.ts | 6 +++-- apps/cli/src/commands/get.command.ts | 6 ++++- .../service-container/service-container.ts | 9 +++---- .../src/app/accounts/settings.component.ts | 27 +++++++++---------- apps/desktop/src/app/app.component.ts | 9 ++++--- .../app/layout/account-switcher.component.ts | 5 +++- apps/desktop/src/auth/lock.component.spec.ts | 5 ++-- apps/desktop/src/auth/lock.component.ts | 4 +-- apps/web/src/app/app.component.ts | 3 +-- .../settings/account/profile.component.ts | 12 ++++----- .../security/security-keys.component.ts | 13 ++++++--- .../platform/abstractions/state.service.ts | 8 ++++++ .../vault-timeout.service.spec.ts | 2 -- 13 files changed, 63 insertions(+), 46 deletions(-) diff --git a/apps/browser/src/auth/popup/settings/account-security.component.ts b/apps/browser/src/auth/popup/settings/account-security.component.ts index c546db3c97..20286435ed 100644 --- a/apps/browser/src/auth/popup/settings/account-security.component.ts +++ b/apps/browser/src/auth/popup/settings/account-security.component.ts @@ -559,9 +559,11 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { } async fingerprint() { - const fingerprint = await this.cryptoService.getFingerprint( - await this.stateService.getUserId(), + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), ); + const publicKey = await firstValueFrom(this.cryptoService.userPublicKey$(activeUserId)); + const fingerprint = await this.cryptoService.getFingerprint(activeUserId, publicKey); const dialogRef = FingerprintDialogComponent.open(this.dialogService, { fingerprint, diff --git a/apps/cli/src/commands/get.command.ts b/apps/cli/src/commands/get.command.ts index 3b2b18c66e..2829b3ee58 100644 --- a/apps/cli/src/commands/get.command.ts +++ b/apps/cli/src/commands/get.command.ts @@ -551,7 +551,11 @@ export class GetCommand extends DownloadCommand { private async getFingerprint(id: string) { let fingerprint: string[] = null; if (id === "me") { - fingerprint = await this.cryptoService.getFingerprint(await this.stateService.getUserId()); + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); + const publicKey = await firstValueFrom(this.cryptoService.userPublicKey$(activeUserId)); + fingerprint = await this.cryptoService.getFingerprint(activeUserId, publicKey); } else if (Utils.isGuid(id)) { try { const response = await this.apiService.getUserPublicKey(id); diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index c339990ab9..c586728197 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/apps/cli/src/service-container/service-container.ts @@ -2,7 +2,7 @@ import * as fs from "fs"; import * as path from "path"; import * as jsdom from "jsdom"; -import { firstValueFrom } from "rxjs"; +import { firstValueFrom, map } from "rxjs"; import { OrganizationUserApiService, @@ -119,7 +119,6 @@ import { import { SendApiService } from "@bitwarden/common/tools/send/services/send-api.service"; import { SendStateProvider } from "@bitwarden/common/tools/send/services/send-state.provider"; import { SendService } from "@bitwarden/common/tools/send/services/send.service"; -import { UserId } from "@bitwarden/common/types/guid"; import { VaultTimeoutStringType } from "@bitwarden/common/types/vault-timeout.type"; import { InternalFolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { CipherService } from "@bitwarden/common/vault/services/cipher.service"; @@ -788,13 +787,13 @@ export class ServiceContainer { this.authService.logOut(() => { /* Do nothing */ }); - const userId = (await this.stateService.getUserId()) as UserId; + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))); await Promise.all([ - this.eventUploadService.uploadEvents(userId as UserId), + this.eventUploadService.uploadEvents(userId), this.cryptoService.clearKeys(), this.cipherService.clear(userId), this.folderService.clear(userId), - this.collectionService.clear(userId as UserId), + this.collectionService.clear(userId), ]); await this.stateEventRunnerService.handleEvent("logout", userId); diff --git a/apps/desktop/src/app/accounts/settings.component.ts b/apps/desktop/src/app/accounts/settings.component.ts index d65283598c..efe7cfa8e2 100644 --- a/apps/desktop/src/app/accounts/settings.component.ts +++ b/apps/desktop/src/app/accounts/settings.component.ts @@ -196,22 +196,23 @@ export class SettingsComponent implements OnInit, OnDestroy { async ngOnInit() { this.vaultTimeoutOptions = await this.generateVaultTimeoutOptions(); - - this.userHasMasterPassword = await this.userVerificationService.hasMasterPassword(); - - this.isWindows = (await this.platformUtilsService.getDevice()) === DeviceType.WindowsDesktop; + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); this.isLinux = (await this.platformUtilsService.getDevice()) === DeviceType.LinuxDesktop; - if ((await this.stateService.getUserId()) == null) { + if (activeAccount == null || activeAccount.id == null) { return; } - this.currentUserEmail = await firstValueFrom( - this.accountService.activeAccount$.pipe(map((a) => a?.email)), - ); - this.currentUserId = (await this.stateService.getUserId()) as UserId; + this.userHasMasterPassword = await this.userVerificationService.hasMasterPassword(); + + this.isWindows = this.platformUtilsService.getDevice() === DeviceType.WindowsDesktop; + + this.currentUserEmail = activeAccount.email; + this.currentUserId = activeAccount.id; this.availableVaultTimeoutActions$ = this.refreshTimeoutSettings$.pipe( - switchMap(() => this.vaultTimeoutSettingsService.availableVaultTimeoutActions$()), + switchMap(() => + this.vaultTimeoutSettingsService.availableVaultTimeoutActions$(activeAccount.id), + ), ); // Load timeout policy @@ -236,12 +237,8 @@ export class SettingsComponent implements OnInit, OnDestroy { }), ); - const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id; - // Load initial values - this.userHasPinSet = await this.pinService.isPinSet(userId); - - const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + this.userHasPinSet = await this.pinService.isPinSet(activeAccount.id); const initialValues = { vaultTimeout: await firstValueFrom( diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index fee477e4ca..4fd5091272 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -235,7 +235,8 @@ export class AppComponent implements OnInit, OnDestroy { this.modalService.closeAll(); if ( message.userId == null || - message.userId === (await this.stateService.getUserId()) + message.userId === + (await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id)))) ) { await this.router.navigate(["lock"]); } @@ -274,9 +275,11 @@ export class AppComponent implements OnInit, OnDestroy { await this.openModal(PremiumComponent, this.premiumRef); break; case "showFingerprintPhrase": { - const fingerprint = await this.cryptoService.getFingerprint( - await this.stateService.getUserId(), + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), ); + const publicKey = await firstValueFrom(this.cryptoService.userPublicKey$(activeUserId)); + const fingerprint = await this.cryptoService.getFingerprint(activeUserId, publicKey); const dialogRef = FingerprintDialogComponent.open(this.dialogService, { fingerprint }); await firstValueFrom(dialogRef.closed); break; diff --git a/apps/desktop/src/app/layout/account-switcher.component.ts b/apps/desktop/src/app/layout/account-switcher.component.ts index c6dbadade4..147c0c498e 100644 --- a/apps/desktop/src/app/layout/account-switcher.component.ts +++ b/apps/desktop/src/app/layout/account-switcher.component.ts @@ -201,8 +201,11 @@ export class AccountSwitcherComponent implements OnInit { }): Promise<{ [userId: string]: InactiveAccount }> { const inactiveAccounts: { [userId: string]: InactiveAccount } = {}; + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); for (const userId in baseAccounts) { - if (userId == null || userId === (await this.stateService.getUserId())) { + if (userId == null || userId === activeUserId) { continue; } diff --git a/apps/desktop/src/auth/lock.component.spec.ts b/apps/desktop/src/auth/lock.component.spec.ts index 2d867019a8..6bf3442882 100644 --- a/apps/desktop/src/auth/lock.component.spec.ts +++ b/apps/desktop/src/auth/lock.component.spec.ts @@ -461,11 +461,10 @@ describe("LockComponent", () => { }); describe("canUseBiometric", () => { - it("should call getUserId() on stateService", async () => { - stateServiceMock.getUserId.mockResolvedValue("userId"); + it("should call biometric.enabled with current active user", async () => { await component["canUseBiometric"](); - expect(ipc.keyManagement.biometric.enabled).toHaveBeenCalledWith("userId"); + expect(ipc.keyManagement.biometric.enabled).toHaveBeenCalledWith(mockUserId); }); }); diff --git a/apps/desktop/src/auth/lock.component.ts b/apps/desktop/src/auth/lock.component.ts index 13451d402f..6ab39f92a6 100644 --- a/apps/desktop/src/auth/lock.component.ts +++ b/apps/desktop/src/auth/lock.component.ts @@ -1,6 +1,6 @@ import { Component, NgZone, OnDestroy, OnInit } from "@angular/core"; import { ActivatedRoute, Router } from "@angular/router"; -import { firstValueFrom, switchMap } from "rxjs"; +import { firstValueFrom, map, switchMap } from "rxjs"; import { LockComponent as BaseLockComponent } from "@bitwarden/angular/auth/components/lock.component"; import { PinServiceAbstraction } from "@bitwarden/auth/common"; @@ -182,7 +182,7 @@ export class LockComponent extends BaseLockComponent implements OnInit, OnDestro } private async canUseBiometric() { - const userId = await this.stateService.getUserId(); + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))); return await ipc.keyManagement.biometric.enabled(userId); } diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index 483e1a52c4..578bc9111c 100644 --- a/apps/web/src/app/app.component.ts +++ b/apps/web/src/app/app.component.ts @@ -23,7 +23,6 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { StateEventRunnerService } from "@bitwarden/common/platform/state"; import { SyncService } from "@bitwarden/common/platform/sync"; -import { UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CollectionService } from "@bitwarden/common/vault/abstractions/collection.service"; import { InternalFolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; @@ -277,7 +276,7 @@ export class AppComponent implements OnDestroy, OnInit { await this.displayLogoutReason(logoutReason); await this.eventUploadService.uploadEvents(); - const userId = (await this.stateService.getUserId()) as UserId; + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))); const logoutPromise = firstValueFrom( this.authService.authStatusFor$(userId).pipe( diff --git a/apps/web/src/app/auth/settings/account/profile.component.ts b/apps/web/src/app/auth/settings/account/profile.component.ts index 8b659e579d..70cb70c581 100644 --- a/apps/web/src/app/auth/settings/account/profile.component.ts +++ b/apps/web/src/app/auth/settings/account/profile.component.ts @@ -1,13 +1,12 @@ import { Component, OnDestroy, OnInit } from "@angular/core"; import { FormControl, FormGroup } from "@angular/forms"; -import { Subject, takeUntil } from "rxjs"; +import { firstValueFrom, map, Subject, takeUntil } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { UpdateProfileRequest } from "@bitwarden/common/auth/models/request/update-profile.request"; import { ProfileResponse } from "@bitwarden/common/models/response/profile.response"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; -import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { DialogService, ToastService } from "@bitwarden/components"; import { ChangeAvatarDialogComponent } from "./change-avatar-dialog.component"; @@ -30,8 +29,7 @@ export class ProfileComponent implements OnInit, OnDestroy { constructor( private apiService: ApiService, private i18nService: I18nService, - private platformUtilsService: PlatformUtilsService, - private stateService: StateService, + private accountService: AccountService, private dialogService: DialogService, private toastService: ToastService, ) {} @@ -39,7 +37,9 @@ export class ProfileComponent implements OnInit, OnDestroy { async ngOnInit() { this.profile = await this.apiService.getProfile(); this.loading = false; - this.fingerprintMaterial = await this.stateService.getUserId(); + this.fingerprintMaterial = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); this.formGroup.get("name").setValue(this.profile.name); this.formGroup.get("email").setValue(this.profile.email); diff --git a/apps/web/src/app/auth/settings/security/security-keys.component.ts b/apps/web/src/app/auth/settings/security/security-keys.component.ts index 8de629dc83..a8892def5c 100644 --- a/apps/web/src/app/auth/settings/security/security-keys.component.ts +++ b/apps/web/src/app/auth/settings/security/security-keys.component.ts @@ -1,8 +1,9 @@ import { Component, OnInit, ViewChild, ViewContainerRef } from "@angular/core"; +import { firstValueFrom, map } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; -import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { DialogService } from "@bitwarden/components"; import { ApiKeyComponent } from "./api-key.component"; @@ -21,7 +22,7 @@ export class SecurityKeysComponent implements OnInit { constructor( private userVerificationService: UserVerificationService, - private stateService: StateService, + private accountService: AccountService, private apiService: ApiService, private dialogService: DialogService, ) {} @@ -31,7 +32,9 @@ export class SecurityKeysComponent implements OnInit { } async viewUserApiKey() { - const entityId = await this.stateService.getUserId(); + const entityId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); await ApiKeyComponent.open(this.dialogService, { data: { keyType: "user", @@ -47,7 +50,9 @@ export class SecurityKeysComponent implements OnInit { } async rotateUserApiKey() { - const entityId = await this.stateService.getUserId(); + const entityId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); await ApiKeyComponent.open(this.dialogService, { data: { keyType: "user", diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index 1ffe5b4353..619bab8f03 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -58,6 +58,14 @@ export abstract class StateService { setCryptoMasterKeyAuto: (value: string, options?: StorageOptions) => Promise; getDuckDuckGoSharedKey: (options?: StorageOptions) => Promise; setDuckDuckGoSharedKey: (value: string, options?: StorageOptions) => Promise; + + /** + * @deprecated Use `TokenService.hasAccessToken$()` or `AuthService.authStatusFor$` instead. + */ getIsAuthenticated: (options?: StorageOptions) => Promise; + + /** + * @deprecated Use `AccountService.activeAccount$` instead. + */ getUserId: (options?: StorageOptions) => Promise; } diff --git a/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts index 487a2578b5..942b8ce451 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts @@ -138,8 +138,6 @@ describe("VaultTimeoutService", () => { return new BehaviorSubject(accounts[userId]?.vaultTimeout); }); - stateService.getUserId.mockResolvedValue(globalSetups?.userId); - // Set desired user active and known users on accounts service : note the only thing that matters here is that the ID are set if (globalSetups?.userId) { accountService.activeAccountSubject.next({ From 386bcaeabd25752a0c7acfa43f6d2e33d0d6f574 Mon Sep 17 00:00:00 2001 From: Jonathan Prusik Date: Thu, 3 Oct 2024 13:28:47 -0400 Subject: [PATCH 2/5] fix fido2 cipher row component subname not having proper layout/styling (#11389) --- .../autofill/popup/fido2/fido2-cipher-row.component.html | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/browser/src/autofill/popup/fido2/fido2-cipher-row.component.html b/apps/browser/src/autofill/popup/fido2/fido2-cipher-row.component.html index 0328a91bff..8b2504696d 100644 --- a/apps/browser/src/autofill/popup/fido2/fido2-cipher-row.component.html +++ b/apps/browser/src/autofill/popup/fido2/fido2-cipher-row.component.html @@ -15,7 +15,9 @@ class="bwi bwi-collection text-muted" > - {{ getSubName(cipher) }} - {{ cipher.subTitle }} + +
{{ getSubName(cipher) }}
+
{{ cipher.subTitle }}
+ From 8104e2bb7e71d29e506e14584287000a52cc69a2 Mon Sep 17 00:00:00 2001 From: Jonathan Prusik Date: Thu, 3 Oct 2024 15:01:46 -0400 Subject: [PATCH 3/5] update fido2 overwrite passkey dialog type (#11392) --- apps/browser/src/autofill/popup/fido2/fido2.component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/autofill/popup/fido2/fido2.component.ts b/apps/browser/src/autofill/popup/fido2/fido2.component.ts index c389e9ad5b..cf0fd90a8f 100644 --- a/apps/browser/src/autofill/popup/fido2/fido2.component.ts +++ b/apps/browser/src/autofill/popup/fido2/fido2.component.ts @@ -287,7 +287,7 @@ export class Fido2Component implements OnInit, OnDestroy { const confirmed = await this.dialogService.openSimpleDialog({ title: { key: "overwritePasskey" }, content: { key: "overwritePasskeyAlert" }, - type: "info", + type: "warning", }); if (!confirmed) { From f2d3311cb4b2f074027f0d7c592b0e0657c74945 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Thu, 3 Oct 2024 16:42:54 -0500 Subject: [PATCH 4/5] [deps] AC: Update url to v0.11.4 (#10392) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- package-lock.json | 13 ++++++++----- package.json | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6498bb54c3..f5e233c232 100644 --- a/package-lock.json +++ b/package-lock.json @@ -178,7 +178,7 @@ "tsconfig-paths-webpack-plugin": "4.1.0", "type-fest": "2.19.0", "typescript": "5.1.6", - "url": "0.11.3", + "url": "0.11.4", "util": "0.12.5", "wait-on": "8.0.1", "webpack": "5.94.0", @@ -37794,14 +37794,17 @@ "license": "MIT" }, "node_modules/url": { - "version": "0.11.3", - "resolved": "https://registry.npmjs.org/url/-/url-0.11.3.tgz", - "integrity": "sha512-6hxOLGfZASQK/cijlZnZJTq8OXAkt/3YGfQX45vvMYXpZoo8NdWZcY73K108Jf759lS1Bv/8wXnHDTSz17dSRw==", + "version": "0.11.4", + "resolved": "https://registry.npmjs.org/url/-/url-0.11.4.tgz", + "integrity": "sha512-oCwdVC7mTuWiPyjLUz/COz5TLk6wgp0RCsN+wHZ2Ekneac9w8uuV0njcbbie2ME+Vs+d6duwmYuR3HgQXs1fOg==", "dev": true, "license": "MIT", "dependencies": { "punycode": "^1.4.1", - "qs": "^6.11.2" + "qs": "^6.12.3" + }, + "engines": { + "node": ">= 0.4" } }, "node_modules/url-parse": { diff --git a/package.json b/package.json index d16b7d7036..ed4e95c012 100644 --- a/package.json +++ b/package.json @@ -139,7 +139,7 @@ "tsconfig-paths-webpack-plugin": "4.1.0", "type-fest": "2.19.0", "typescript": "5.1.6", - "url": "0.11.3", + "url": "0.11.4", "util": "0.12.5", "wait-on": "8.0.1", "webpack": "5.94.0", From bf38f2dfeec7ba51de9d85eb0ce8e32198eb3926 Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Thu, 3 Oct 2024 16:53:45 -0500 Subject: [PATCH 5/5] [PM-12775] Password autofill should not occur within 2FA fields (#11303) --- .../autofill/content/autofill-init.spec.ts | 2 +- .../src/autofill/content/autofill-init.ts | 2 +- .../autofill/services/autofill-constants.ts | 12 ++++--- .../services/autofill.service.spec.ts | 20 ++++------- .../src/autofill/services/autofill.service.ts | 20 +++++++++-- ...inline-menu-field-qualification.service.ts | 33 ++++++++++++++----- 6 files changed, 57 insertions(+), 32 deletions(-) diff --git a/apps/browser/src/autofill/content/autofill-init.spec.ts b/apps/browser/src/autofill/content/autofill-init.spec.ts index ebfbda75b5..b98d297d13 100644 --- a/apps/browser/src/autofill/content/autofill-init.spec.ts +++ b/apps/browser/src/autofill/content/autofill-init.spec.ts @@ -74,7 +74,7 @@ describe("AutofillInit", () => { Object.defineProperty(document, "readyState", { value: "complete", writable: true }); autofillInit.init(); - jest.advanceTimersByTime(250); + jest.advanceTimersByTime(750); expect(sendExtensionMessageSpy).toHaveBeenCalledWith("bgCollectPageDetails", { sender: "autofillInit", diff --git a/apps/browser/src/autofill/content/autofill-init.ts b/apps/browser/src/autofill/content/autofill-init.ts index e901000dbb..6c34508cb0 100644 --- a/apps/browser/src/autofill/content/autofill-init.ts +++ b/apps/browser/src/autofill/content/autofill-init.ts @@ -78,7 +78,7 @@ class AutofillInit implements AutofillInitInterface { this.clearCollectPageDetailsOnLoadTimeout(); this.collectPageDetailsOnLoadTimeout = setTimeout( () => this.sendExtensionMessage("bgCollectPageDetails", { sender: "autofillInit" }), - 250, + 750, ); }; diff --git a/apps/browser/src/autofill/services/autofill-constants.ts b/apps/browser/src/autofill/services/autofill-constants.ts index c379daaf2d..656387f00b 100644 --- a/apps/browser/src/autofill/services/autofill-constants.ts +++ b/apps/browser/src/autofill/services/autofill-constants.ts @@ -34,28 +34,30 @@ export class AutoFillConstants { "totpcode", "2facode", "approvals_code", - "code", "mfacode", - "otc", "otc-code", - "otp", + "onetimecode", "otp-code", "otpcode", - "pin", + "onetimepassword", "security_code", "twofactor", "twofa", "twofactorcode", "verificationCode", + "verification code", ]; + static readonly AmbiguousTotpFieldNames: string[] = ["code", "pin", "otc", "otp"]; + static readonly SearchFieldNames: string[] = ["search", "query", "find", "go"]; static readonly FieldIgnoreList: string[] = ["captcha", "findanything", "forgot"]; static readonly PasswordFieldExcludeList: string[] = [ + "hint", ...AutoFillConstants.FieldIgnoreList, - "onetimepassword", + ...AutoFillConstants.TotpFieldNames, ]; static readonly ExcludedAutofillLoginTypes: string[] = [ diff --git a/apps/browser/src/autofill/services/autofill.service.spec.ts b/apps/browser/src/autofill/services/autofill.service.spec.ts index 455c171e59..7bd08caaf3 100644 --- a/apps/browser/src/autofill/services/autofill.service.spec.ts +++ b/apps/browser/src/autofill/services/autofill.service.spec.ts @@ -2260,29 +2260,23 @@ describe("AutofillService", () => { options, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledTimes(4); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 1, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( usernameField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 2, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( emailField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 3, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( telephoneField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 4, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( totpField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenNthCalledWith( - 5, + expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenCalledWith( nonViewableField, AutoFillConstants.UsernameFieldNames, ); @@ -2328,6 +2322,7 @@ describe("AutofillService", () => { it("will not attempt to fuzzy match a totp field if totp autofill is not allowed", async () => { options.allowTotpAutofill = false; + jest.spyOn(autofillService as any, "findMatchingFieldIndex"); await autofillService["generateLoginFillScript"]( fillScript, @@ -2336,7 +2331,7 @@ describe("AutofillService", () => { options, ); - expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenCalledWith( + expect(autofillService["findMatchingFieldIndex"]).not.toHaveBeenCalledWith( expect.anything(), AutoFillConstants.TotpFieldNames, ); @@ -2386,7 +2381,6 @@ describe("AutofillService", () => { false, false, ); - expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenCalled(); expect(AutofillService.fillByOpid).toHaveBeenCalledTimes(2); expect(AutofillService.fillByOpid).toHaveBeenNthCalledWith( 1, diff --git a/apps/browser/src/autofill/services/autofill.service.ts b/apps/browser/src/autofill/services/autofill.service.ts index de79fd974e..ea68b80e84 100644 --- a/apps/browser/src/autofill/services/autofill.service.ts +++ b/apps/browser/src/autofill/services/autofill.service.ts @@ -887,7 +887,10 @@ export default class AutofillService implements AutofillServiceInterface { options.allowTotpAutofill && f.viewable && (f.type === "text" || f.type === "number") && - (AutofillService.fieldIsFuzzyMatch(f, AutoFillConstants.TotpFieldNames) || + (AutofillService.fieldIsFuzzyMatch(f, [ + ...AutoFillConstants.TotpFieldNames, + ...AutoFillConstants.AmbiguousTotpFieldNames, + ]) || f.autoCompleteType === "one-time-code") ) { totps.push(f); @@ -2558,6 +2561,11 @@ export default class AutofillService implements AutofillServiceInterface { return; } + // We want to avoid treating TOTP fields as password fields + if (AutofillService.fieldIsFuzzyMatch(f, AutoFillConstants.TotpFieldNames)) { + return; + } + const isLikePassword = () => { if (f.type !== "text") { return false; @@ -2670,12 +2678,18 @@ export default class AutofillService implements AutofillServiceInterface { (withoutForm || f.form === passwordField.form) && (canBeHidden || f.viewable) && (f.type === "text" || f.type === "number") && - AutofillService.fieldIsFuzzyMatch(f, AutoFillConstants.TotpFieldNames) + AutofillService.fieldIsFuzzyMatch(f, [ + ...AutoFillConstants.TotpFieldNames, + ...AutoFillConstants.AmbiguousTotpFieldNames, + ]) ) { totpField = f; if ( - this.findMatchingFieldIndex(f, AutoFillConstants.TotpFieldNames) > -1 || + this.findMatchingFieldIndex(f, [ + ...AutoFillConstants.TotpFieldNames, + ...AutoFillConstants.AmbiguousTotpFieldNames, + ]) > -1 || f.autoCompleteType === "one-time-code" ) { // We found an exact match. No need to keep looking. diff --git a/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts b/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts index 0b04b83ce4..9a98f65f7a 100644 --- a/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts +++ b/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts @@ -30,7 +30,6 @@ export class InlineMenuFieldQualificationService this.webAuthnAutocompleteValue, ]); private fieldIgnoreListString = AutoFillConstants.FieldIgnoreList.join(","); - private passwordFieldExcludeListString = AutoFillConstants.PasswordFieldExcludeList.join(","); private currentPasswordAutocompleteValue = "current-password"; private newPasswordAutoCompleteValue = "new-password"; private autofillFieldKeywordsMap: AutofillKeywordsMap = new WeakMap(); @@ -927,7 +926,7 @@ export class InlineMenuFieldQualificationService return false; } - return !(this.passwordFieldExcludeListString.indexOf(cleanedValue) > -1); + return !AutoFillConstants.PasswordFieldExcludeList.some((i) => cleanedValue.indexOf(i) > -1); } /** @@ -1094,13 +1093,29 @@ export class InlineMenuFieldQualificationService ]; const keywordsSet = new Set(); for (let i = 0; i < keywords.length; i++) { - if (typeof keywords[i] === "string") { - keywords[i] - .toLowerCase() - .replace(/-/g, "") - .replace(/[^a-zA-Z0-9]+/g, "|") - .split("|") - .forEach((keyword) => keywordsSet.add(keyword)); + if (keywords[i] && typeof keywords[i] === "string") { + let keywordEl = keywords[i].toLowerCase(); + keywordsSet.add(keywordEl); + + // Remove hyphens from all potential keywords, we want to treat these as a single word. + keywordEl = keywordEl.replace(/-/g, ""); + + // Split the keyword by non-alphanumeric characters to get the keywords without treating a space as a separator. + keywordEl.split(/[^\p{L}\d]+/gu).forEach((keyword) => { + if (keyword) { + keywordsSet.add(keyword); + } + }); + + // Collapse all spaces and split by non-alphanumeric characters to get the keywords + keywordEl + .replace(/\s/g, "") + .split(/[^\p{L}\d]+/gu) + .forEach((keyword) => { + if (keyword) { + keywordsSet.add(keyword); + } + }); } }