Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-12399] Show card and identity ciphers in the autofill suggestions only if the active tab appears to have fields for that type of cipher #11913

Merged
merged 3 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
autoSubmitLoginBackground: AutoSubmitLoginBackground;
sdkService: SdkService;
cipherAuthorizationService: CipherAuthorizationService;
inlineMenuFieldQualificationService: InlineMenuFieldQualificationService;

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

this.inlineMenuFieldQualificationService = new InlineMenuFieldQualificationService();

Check warning on line 1254 in apps/browser/src/background/main.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/background/main.background.ts#L1254

Added line #L1254 was not covered by tests
}

async bootstrap() {
Expand Down Expand Up @@ -1630,7 +1633,6 @@
this.themeStateService,
);
} else {
const inlineMenuFieldQualificationService = new InlineMenuFieldQualificationService();
this.overlayBackground = new OverlayBackground(
this.logService,
this.cipherService,
Expand All @@ -1643,7 +1645,7 @@
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 { 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";

Check warning on line 116 in apps/browser/src/popup/services/services.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/popup/services/services.module.ts#L116

Added line #L116 was not covered by tests
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 @@
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 @@
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 { 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 @@
if (!tab) {
return of([]);
}
return this.autofillService.collectPageDetailsFromTab$(tab);
return this.autofillService
.collectPageDetailsFromTab$(tab)
.pipe(timeout({ first: 1500, with: () => of([]) }));

Check warning on line 80 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L80

Added line #L80 was not covered by tests
}),
shareReplay({ refCount: false, bufferSize: 1 }),
);

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

Check warning on line 91 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L90-L91

Added lines #L90 - L91 were not covered by tests

try {

Check warning on line 93 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L93

Added line #L93 was not covered by tests
if (!pageDetails) {
throw Error("No page details were provided");

Check warning on line 95 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L95

Added line #L95 was not covered by tests
}

for (const details of pageDetails) {
for (const field of details.details.fields) {

Check warning on line 99 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L98-L99

Added lines #L98 - L99 were not covered by tests
if (!pageHasCardFields) {
pageHasCardFields = this.inlineMenuFieldQualificationService.isFieldForCreditCardForm(

Check warning on line 101 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L101

Added line #L101 was not covered by tests
field,
details.details,
);
}

if (!pageHasIdentityFields) {
pageHasIdentityFields =

Check warning on line 108 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L108

Added line #L108 was not covered by tests
this.inlineMenuFieldQualificationService.isFieldForIdentityForm(
field,
details.details,
);
}
}
}
} catch (error) {
// no-op on failure; do not show extra cipher types
this.logService.warning(error.message);

Check warning on line 118 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L118

Added line #L118 was not covered by tests
}

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

Check warning on line 121 in apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/vault/popup/services/vault-popup-autofill.service.ts#L121

Added line #L121 was not covered by tests
}),
);

constructor(
private autofillService: AutofillService,
private i18nService: I18nService,
Expand All @@ -87,6 +132,8 @@
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
Loading