Skip to content

Commit

Permalink
[PM-9342] Inline menu does not show on username field for a form that…
Browse files Browse the repository at this point in the history 
… has a password field with an invalid autocomplete value (#9860)

* [PM-9342] Inline menu does not show on username field for a form that has a password field with an invalid autocomplete value

* [PM-9342] Incorporating logic to handle multiple autocomplete values within a captured set of page details

* [PM-9342] Incorporating logic to handle multiple autocomplete values within a captured set of page details

* [PM-9342] Changing logic for how we identify new password fields to reflect a more assertive qualification

* [PM-9342] Adding feedback from code review
  • Loading branch information
@cagonzalezcs
cagonzalezcs committed Jun 28, 2024
1 parent a613d9c commit 4e3fb99
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 25 deletions.
84 changes: 69 additions & 15 deletions apps/browser/src/autofill/services/inline-menu-field-qualification.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,23 @@ describe("InlineMenuFieldQualificationService", () => {
);
});

it("has a keyword value that indicates the field is for a create account form", () => {
const field = mock<AutofillField>({
type: "password",
placeholder: "create account password",
autoCompleteType: "",
});

expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
false,
);
});

it("has a type that is an excluded type", () => {
AutoFillConstants.ExcludedAutofillLoginTypes.forEach((excludedType) => {
const field = mock<AutofillField>({
type: excludedType,
autoCompleteType: "",
});

expect(
Expand All @@ -53,6 +66,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: index === 0 ? attribute : "",
htmlName: index === 1 ? attribute : "",
placeholder: index > 1 ? attribute : "",
autoCompleteType: "",
});

expect(
Expand All @@ -67,6 +81,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "not-password",
htmlName: "not-password",
placeholder: "not-password",
autoCompleteType: "",
});

expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
Expand All @@ -80,6 +95,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "something-else",
htmlName: "something-else",
placeholder: "something-else",
autoCompleteType: "",
});

expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
Expand All @@ -93,6 +109,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "search",
htmlName: "something-else",
placeholder: "something-else",
autoCompleteType: "",
});

expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
Expand All @@ -112,12 +129,14 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "",
autoCompleteType: "",
});
const secondField = mock<AutofillField>({
type: "password",
htmlID: "some-other-password",
htmlName: "some-other-password",
placeholder: "some-other-password",
autoCompleteType: "",
});
pageDetails.fields = [field, secondField];

Expand All @@ -133,18 +152,21 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "",
autoCompleteType: "",
});
const usernameField = mock<AutofillField>({
type: "text",
htmlID: "user-username",
htmlName: "user-username",
placeholder: "user-username",
autoCompleteType: "",
});
const secondUsernameField = mock<AutofillField>({
type: "text",
htmlID: "some-other-user-username",
htmlName: "some-other-user-username",
placeholder: "some-other-user-username",
autoCompleteType: "",
});
pageDetails.fields = [field, usernameField, secondUsernameField];

Expand Down Expand Up @@ -186,13 +208,15 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "validFormId",
autoCompleteType: "",
});
const secondField = mock<AutofillField>({
type: "password",
htmlID: "some-other-password",
htmlName: "some-other-password",
placeholder: "some-other-password",
form: "validFormId",
autoCompleteType: "",
});
pageDetails.fields = [field, secondField];

Expand All @@ -218,12 +242,35 @@ describe("InlineMenuFieldQualificationService", () => {
);
});

it("is structured on a page with a single set of username and password fields", () => {
const field = mock<AutofillField>({
type: "password",
htmlID: "user-password",
htmlName: "user-password",
placeholder: "user-password",
autoCompleteType: "",
});
const usernameField = mock<AutofillField>({
type: "text",
htmlID: "user-username",
htmlName: "user-username",
placeholder: "user-username",
autoCompleteType: "",
});
pageDetails.fields = [field, usernameField];

expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
true,
);
});

it("has a type of `text` with an attribute that indicates the field is a password field", () => {
const field = mock<AutofillField>({
type: "text",
htmlID: null,
htmlName: "user-password",
placeholder: "user-password",
autoCompleteType: "",
});

expect(inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails)).toBe(
Expand All @@ -247,6 +294,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlID: "user-username",
htmlName: "user-username",
placeholder: "user-username",
autoCompleteType: "",
});
pageDetails.fields = [field, usernameField];

Expand All @@ -273,20 +321,23 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "user-password",
placeholder: "user-password",
form: "validFormId",
autoCompleteType: "",
});
const secondPasswordField = mock<AutofillField>({
type: "password",
htmlID: "some-other-password",
htmlName: "some-other-password",
placeholder: "some-other-password",
form: "anotherFormId",
autoCompleteType: "",
});
const usernameField = mock<AutofillField>({
type: "text",
htmlID: "user-username",
htmlName: "user-username",
placeholder: "user-username",
form: "validFormId",
autoCompleteType: "",
});
pageDetails.fields = [field, secondPasswordField, usernameField];

Expand All @@ -310,6 +361,7 @@ describe("InlineMenuFieldQualificationService", () => {
htmlName: "some-other-password",
placeholder: "some-other-password",
form: "anotherFormId",
autoCompleteType: "",
});
pageDetails.fields = [field, secondPasswordField];

Expand Down Expand Up @@ -347,21 +399,23 @@ describe("InlineMenuFieldQualificationService", () => {
});
});

["new", "change", "neue", "ändern"].forEach((keyword) => {
it(`has a keyword of ${keyword} that indicates a 'new or changed' username is being filled`, () => {
const field = mock<AutofillField>({
type: "text",
autoCompleteType: "",
htmlID: "user-username",
htmlName: "user-username",
placeholder: `${keyword} username`,
});

expect(
inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails),
).toBe(false);
});
});
["new", "change", "neue", "ändern", "register", "create", "registration"].forEach(
(keyword) => {
it(`has a keyword of ${keyword} that indicates a 'new or changed' username is being filled`, () => {
const field = mock<AutofillField>({
type: "text",
autoCompleteType: "",
htmlID: "user-username",
htmlName: "user-username",
placeholder: `${keyword} username`,
});

expect(
inlineMenuFieldQualificationService.isFieldForLoginForm(field, pageDetails),
).toBe(false);
});
},
);

describe("does not have a parent form element", () => {
beforeEach(() => {
Expand Down
93 changes: 83 additions & 10 deletions apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,18 @@ export class InlineMenuFieldQualificationService
private usernameAutocompleteValues = new Set(["username", "email"]);
private fieldIgnoreListString = AutoFillConstants.FieldIgnoreList.join(",");
private passwordFieldExcludeListString = AutoFillConstants.PasswordFieldExcludeList.join(",");
private currentPasswordAutocompleteValues = new Set(["current-password"]);
private newPasswordAutoCompleteValues = new Set(["new-password"]);
private autofillFieldKeywordsMap: WeakMap<AutofillField, string> = new WeakMap();
private autocompleteDisabledValues = new Set(["off", "false"]);
private newFieldKeywords = new Set(["new", "change", "neue", "ändern"]);
private accountCreationFieldKeywords = new Set([
"register",
"registration",
"create",
"confirm",
...this.newFieldKeywords,
]);
private inlineMenuFieldQualificationFlagSet = false;

constructor() {
Expand Down Expand Up @@ -62,7 +71,12 @@ export class InlineMenuFieldQualificationService
): boolean {
// If the provided field is set with an autocomplete value of "current-password", we should assume that
// the page developer intends for this field to be interpreted as a password field for a login form.
if (field.autoCompleteType === "current-password") {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.currentPasswordAutocompleteValues,
)
) {
return true;
}

Expand Down Expand Up @@ -95,7 +109,11 @@ export class InlineMenuFieldQualificationService
// If a single username field or less is present on the page, then we can assume that the
// provided field is for a login form. This will only be the case if the field does not
// explicitly have its autocomplete attribute set to "off" or "false".
return !this.autocompleteDisabledValues.has(field.autoCompleteType);

return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}

// If the field has a form parent and there are multiple visible password fields
Expand All @@ -117,7 +135,10 @@ export class InlineMenuFieldQualificationService

// If the field has a form parent and no username field exists and the field has an
// autocomplete attribute set to "off" or "false", this is not a password field
return !this.autocompleteDisabledValues.has(field.autoCompleteType);
return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}

/**
Expand All @@ -132,7 +153,9 @@ export class InlineMenuFieldQualificationService
): boolean {
// If the provided field is set with an autocomplete of "username", we should assume that
// the page developer intends for this field to be interpreted as a username field.
if (this.usernameAutocompleteValues.has(field.autoCompleteType)) {
if (
this.fieldContainsAutocompleteValues(field.autoCompleteType, this.usernameAutocompleteValues)
) {
const newPasswordFieldsInPageDetails = pageDetails.fields.filter(this.isNewPasswordField);
return newPasswordFieldsInPageDetails.length === 0;
}
Expand Down Expand Up @@ -175,15 +198,23 @@ export class InlineMenuFieldQualificationService
// If the page does not contain any password fields, it might be part of a multistep login form.
// That will only be the case if the field does not explicitly have its autocomplete attribute
// set to "off" or "false".
return !this.autocompleteDisabledValues.has(field.autoCompleteType);
return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}

// If the field is structured within a form, but no password fields are present in the form,
// we need to consider whether the field is part of a multistep login form.
if (passwordFieldsInPageDetails.length === 0) {
// If the field's autocomplete is set to a disabled value, we should assume that the field is
// not part of a login form.
if (this.autocompleteDisabledValues.has(field.autoCompleteType)) {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
)
) {
return false;
}

Expand Down Expand Up @@ -212,7 +243,10 @@ export class InlineMenuFieldQualificationService

// If no visible password fields are found, this field might be part of a multipart form.
// Check for an invalid autocompleteType to determine if the field is part of a login form.
return !this.autocompleteDisabledValues.has(field.autoCompleteType);
return !this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.autocompleteDisabledValues,
);
}

/**
Expand All @@ -237,7 +271,13 @@ export class InlineMenuFieldQualificationService
* @param field - The field to validate
*/
private isCurrentPasswordField = (field: AutofillField): boolean => {
if (field.autoCompleteType === "new-password") {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.newPasswordAutoCompleteValues,
) ||
this.keywordsFoundInFieldData(field, [...this.accountCreationFieldKeywords])
) {
return false;
}

Expand All @@ -250,11 +290,19 @@ export class InlineMenuFieldQualificationService
* @param field - The field to validate
*/
private isNewPasswordField = (field: AutofillField): boolean => {
if (field.autoCompleteType === "current-password") {
if (
this.fieldContainsAutocompleteValues(
field.autoCompleteType,
this.currentPasswordAutocompleteValues,
)
) {
return false;
}

return this.isPasswordField(field);
return (
this.isPasswordField(field) &&
this.keywordsFoundInFieldData(field, [...this.accountCreationFieldKeywords])
);
};

/**
Expand Down Expand Up @@ -422,6 +470,31 @@ export class InlineMenuFieldQualificationService
return keywordValues;
}

/**
* Separates the provided field data into space-separated values and checks if any
* of the values are present in the provided set of autocomplete values.
*
* @param fieldAutocompleteValue - The field autocomplete value to validate
* @param compareValues - The set of autocomplete values to check against
*/
private fieldContainsAutocompleteValues(
fieldAutocompleteValue: string,
compareValues: Set<string>,
) {
if (!fieldAutocompleteValue) {
return false;
}

const autocompleteValueParts = fieldAutocompleteValue.split(" ");
for (let index = 0; index < autocompleteValueParts.length; index++) {
if (compareValues.has(autocompleteValueParts[index])) {
return true;
}
}

return false;
}

/**
* This method represents the previous rudimentary approach to qualifying fields for login forms.
*
Expand Down

0 comments on commit 4e3fb99

Please sign in to comment.