Skip to content

Commit

Permalink
[PM-12399] Show card and identity ciphers in the autofill suggestions…
Browse files Browse the repository at this point in the history
… only if the active tab appears to have fields for that type of cipher (#11913)

* show card and identity ciphers in the autofill suggestions only if the active tab appears to have fields for that type of cipher

* handle cases where collectPageDetailsFromTab$ does not emit

* update tests
  • Loading branch information
jprusik authored Nov 13, 2024
1 parent 204eb31 commit 3a293bb
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 4 deletions.
6 changes: 4 additions & 2 deletions apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ export default class MainBackground {
autoSubmitLoginBackground: AutoSubmitLoginBackground;
sdkService: SdkService;
cipherAuthorizationService: CipherAuthorizationService;
inlineMenuFieldQualificationService: InlineMenuFieldQualificationService;

onUpdatedRan: boolean;
onReplacedRan: boolean;
Expand Down Expand Up @@ -1249,6 +1250,8 @@ export default class MainBackground {
this.collectionService,
this.organizationService,
);

this.inlineMenuFieldQualificationService = new InlineMenuFieldQualificationService();
}

async bootstrap() {
Expand Down Expand Up @@ -1630,7 +1633,6 @@ export default class MainBackground {
this.themeStateService,
);
} else {
const inlineMenuFieldQualificationService = new InlineMenuFieldQualificationService();
this.overlayBackground = new OverlayBackground(
this.logService,
this.cipherService,
Expand All @@ -1643,7 +1645,7 @@ export default class MainBackground {
this.platformUtilsService,
this.vaultSettingsService,
this.fido2ActiveRequestManager,
inlineMenuFieldQualificationService,
this.inlineMenuFieldQualificationService,
this.themeStateService,
this.totpService,
() => this.generatePassword(),
Expand Down
2 changes: 2 additions & 0 deletions apps/browser/src/popup/services/services.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ import { ExtensionAnonLayoutWrapperDataService } from "../../auth/popup/extensio
import { ExtensionLoginComponentService } from "../../auth/popup/login/extension-login-component.service";
import { AutofillService as AutofillServiceAbstraction } from "../../autofill/services/abstractions/autofill.service";
import AutofillService from "../../autofill/services/autofill.service";
import { InlineMenuFieldQualificationService } from "../../autofill/services/inline-menu-field-qualification.service";
import { ForegroundBrowserBiometricsService } from "../../key-management/biometrics/foreground-browser-biometrics";
import { BrowserKeyService } from "../../key-management/browser-key.service";
import { BrowserApi } from "../../platform/browser/browser-api";
Expand Down Expand Up @@ -167,6 +168,7 @@ const safeProviders: SafeProvider[] = [
safeProvider(DebounceNavigationService),
safeProvider(DialogService),
safeProvider(PopupCloseWarningService),
safeProvider(InlineMenuFieldQualificationService),
safeProvider({
provide: DEFAULT_VAULT_TIMEOUT,
useValue: VaultTimeoutStringType.OnRestart,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { BehaviorSubject, of } from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { Utils } from "@bitwarden/common/platform/misc/utils";
Expand All @@ -19,6 +20,7 @@ import { LoginView } from "@bitwarden/common/vault/models/view/login.view";
import { ToastService } from "@bitwarden/components";
import { PasswordRepromptService } from "@bitwarden/vault";

import { InlineMenuFieldQualificationService } from "../../../../../browser/src/autofill/services/inline-menu-field-qualification.service";
import {
AutoFillOptions,
AutofillService,
Expand Down Expand Up @@ -46,13 +48,21 @@ describe("VaultPopupAutofillService", () => {
const mockPasswordRepromptService = mock<PasswordRepromptService>();
const mockCipherService = mock<CipherService>();
const mockMessagingService = mock<MessagingService>();
const mockInlineMenuFieldQualificationService = mock<InlineMenuFieldQualificationService>();
const mockLogService = mock<LogService>();

const mockUserId = Utils.newGuid() as UserId;
const accountService: FakeAccountService = mockAccountServiceWith(mockUserId);

beforeEach(() => {
jest.spyOn(BrowserPopupUtils, "inPopout").mockReturnValue(false);
jest.spyOn(BrowserApi, "getTabFromCurrentWindow").mockResolvedValue(mockCurrentTab);
jest
.spyOn(mockInlineMenuFieldQualificationService, "isFieldForCreditCardForm")
.mockReturnValue(true);
jest
.spyOn(mockInlineMenuFieldQualificationService, "isFieldForIdentityForm")
.mockReturnValue(true);

mockAutofillService.collectPageDetailsFromTab$.mockReturnValue(new BehaviorSubject([]));

Expand All @@ -70,6 +80,14 @@ describe("VaultPopupAutofillService", () => {
provide: AccountService,
useValue: accountService,
},
{
provide: InlineMenuFieldQualificationService,
useValue: mockInlineMenuFieldQualificationService,
},
{
provide: LogService,
useValue: mockLogService,
},
],
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ import {
startWith,
Subject,
switchMap,
timeout,
} from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
Expand All @@ -23,6 +25,7 @@ import { LoginUriView } from "@bitwarden/common/vault/models/view/login-uri.view
import { ToastService } from "@bitwarden/components";
import { PasswordRepromptService } from "@bitwarden/vault";

import { InlineMenuFieldQualificationService } from "../../../../../browser/src/autofill/services/inline-menu-field-qualification.service";
import {
AutofillService,
PageDetail,
Expand Down Expand Up @@ -72,11 +75,53 @@ export class VaultPopupAutofillService {
if (!tab) {
return of([]);
}
return this.autofillService.collectPageDetailsFromTab$(tab);
return this.autofillService
.collectPageDetailsFromTab$(tab)
.pipe(timeout({ first: 1500, with: () => of([]) }));
}),
shareReplay({ refCount: false, bufferSize: 1 }),
);

nonLoginCipherTypesOnPage$: Observable<{
[CipherType.Card]: boolean;
[CipherType.Identity]: boolean;
}> = this._currentPageDetails$.pipe(
map((pageDetails) => {
let pageHasCardFields = false;
let pageHasIdentityFields = false;

try {
if (!pageDetails) {
throw Error("No page details were provided");
}

for (const details of pageDetails) {
for (const field of details.details.fields) {
if (!pageHasCardFields) {
pageHasCardFields = this.inlineMenuFieldQualificationService.isFieldForCreditCardForm(
field,
details.details,
);
}

if (!pageHasIdentityFields) {
pageHasIdentityFields =
this.inlineMenuFieldQualificationService.isFieldForIdentityForm(
field,
details.details,
);
}
}
}
} catch (error) {
// no-op on failure; do not show extra cipher types
this.logService.warning(error.message);
}

return { [CipherType.Card]: pageHasCardFields, [CipherType.Identity]: pageHasIdentityFields };
}),
);

constructor(
private autofillService: AutofillService,
private i18nService: I18nService,
Expand All @@ -87,6 +132,8 @@ export class VaultPopupAutofillService {
private messagingService: MessagingService,
private route: ActivatedRoute,
private accountService: AccountService,
private logService: LogService,
private inlineMenuFieldQualificationService: InlineMenuFieldQualificationService,
) {
this._currentPageDetails$.subscribe();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault
import { CipherType } from "@bitwarden/common/vault/enums";
import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";

import { InlineMenuFieldQualificationService } from "../../../../../browser/src/autofill/services/inline-menu-field-qualification.service";
import { BrowserApi } from "../../../platform/browser/browser-api";

import { VaultPopupAutofillService } from "./vault-popup-autofill.service";
Expand All @@ -39,6 +40,7 @@ describe("VaultPopupItemsService", () => {
const collectionService = mock<CollectionService>();
const vaultAutofillServiceMock = mock<VaultPopupAutofillService>();
const syncServiceMock = mock<SyncService>();
const inlineMenuFieldQualificationServiceMock = mock<InlineMenuFieldQualificationService>();

beforeEach(() => {
allCiphers = cipherFactory(10);
Expand Down Expand Up @@ -78,6 +80,11 @@ describe("VaultPopupItemsService", () => {
url: "https://example.com",
} as chrome.tabs.Tab);

vaultAutofillServiceMock.nonLoginCipherTypesOnPage$ = new BehaviorSubject({
[CipherType.Card]: true,
[CipherType.Identity]: true,
});

mockOrg = {
id: "org1",
name: "Organization 1",
Expand Down Expand Up @@ -105,6 +112,10 @@ describe("VaultPopupItemsService", () => {
{ provide: CollectionService, useValue: collectionService },
{ provide: VaultPopupAutofillService, useValue: vaultAutofillServiceMock },
{ provide: SyncService, useValue: syncServiceMock },
{
provide: InlineMenuFieldQualificationService,
useValue: inlineMenuFieldQualificationServiceMock,
},
],
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,13 @@ export class VaultPopupItemsService {
private _otherAutoFillTypes$: Observable<CipherType[]> = combineLatest([
this.vaultSettingsService.showCardsCurrentTab$,
this.vaultSettingsService.showIdentitiesCurrentTab$,
this.vaultPopupAutofillService.nonLoginCipherTypesOnPage$,
]).pipe(
map(([showCards, showIdentities]) => {
map(([showCardsSettingEnabled, showIdentitiesSettingEnabled, nonLoginCipherTypesOnPage]) => {
const showCards = showCardsSettingEnabled && nonLoginCipherTypesOnPage[CipherType.Card];
const showIdentities =
showIdentitiesSettingEnabled && nonLoginCipherTypesOnPage[CipherType.Identity];

return [
...(showCards ? [CipherType.Card] : []),
...(showIdentities ? [CipherType.Identity] : []),
Expand Down

0 comments on commit 3a293bb

Please sign in to comment.