-
Notifications
You must be signed in to change notification settings - Fork 1k
[PM-41292] feat: Add heuristic detection for identity autofill fields #7233
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
Changes from 5 commits
4026f63
7c0f8b8
80a572d
2f8f5d9
a59dfb4
036ba24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,51 @@ | ||
| package com.x8bit.bitwarden.data.autofill.model | ||
|
|
||
| /** | ||
| * Autofill hints used to determine what data an input field is associated with. | ||
| * Autofill hints used to determine what data an input field is associated with, grouped by the | ||
| * [AutofillView] partition they belong to. | ||
| */ | ||
| enum class AutofillHint { | ||
| CARD_CARDHOLDER, | ||
| CARD_EXPIRATION_DATE, | ||
| CARD_EXPIRATION_MONTH, | ||
| CARD_EXPIRATION_YEAR, | ||
| CARD_NUMBER, | ||
| CARD_SECURITY_CODE, | ||
| CARD_BRAND, | ||
| PASSWORD, | ||
| USERNAME, | ||
| IDENTITY_PERSON_NAME_FULL, | ||
| IDENTITY_PERSON_NAME_PREFIX, | ||
| IDENTITY_PERSON_NAME_GIVEN, | ||
| IDENTITY_PERSON_NAME_MIDDLE, | ||
| IDENTITY_PERSON_NAME_FAMILY, | ||
| IDENTITY_POSTAL_ADDRESS_FULL, | ||
| IDENTITY_ADDRESS_STREET, | ||
| IDENTITY_ADDRESS_LOCALITY, | ||
| IDENTITY_ADDRESS_REGION, | ||
| IDENTITY_ADDRESS_COUNTRY, | ||
| IDENTITY_POSTAL_CODE, | ||
| IDENTITY_PHONE_FULL, | ||
| IDENTITY_COMPANY, | ||
| IDENTITY_EMAIL, | ||
| IDENTITY_SSN, | ||
| IDENTITY_PASSPORT_NUMBER, | ||
| IDENTITY_LICENSE_NUMBER, | ||
| sealed interface AutofillHint { | ||
| /** | ||
| * Hints for the [AutofillView.Card] partition. | ||
| */ | ||
| enum class Card : AutofillHint { | ||
| BRAND, | ||
| CARDHOLDER, | ||
| EXPIRATION_DATE, | ||
| EXPIRATION_MONTH, | ||
| EXPIRATION_YEAR, | ||
| NUMBER, | ||
| SECURITY_CODE, | ||
| } | ||
|
|
||
| /** | ||
| * Hints for the [AutofillView.Login] partition. | ||
| */ | ||
| enum class Login : AutofillHint { | ||
| PASSWORD, | ||
| USERNAME, | ||
| } | ||
|
|
||
| /** | ||
| * Hints for the [AutofillView.Identity] partition. | ||
| */ | ||
| enum class Identity : AutofillHint { | ||
| ADDRESS_COUNTRY, | ||
| ADDRESS_LOCALITY, | ||
| ADDRESS_REGION, | ||
| ADDRESS_STREET, | ||
| COMPANY, | ||
| EMAIL, | ||
| LICENSE_NUMBER, | ||
| PASSPORT_NUMBER, | ||
| PERSON_NAME_FAMILY, | ||
| PERSON_NAME_FULL, | ||
| PERSON_NAME_GIVEN, | ||
| PERSON_NAME_MIDDLE, | ||
| PERSON_NAME_PREFIX, | ||
| POSTAL_ADDRESS_FULL, | ||
| POSTAL_CODE, | ||
| PHONE_FULL, | ||
| SSN, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,8 @@ import com.x8bit.bitwarden.data.autofill.util.buildPackageNameOrNull | |||||||||||||
| import com.x8bit.bitwarden.data.autofill.util.buildUriOrNull | ||||||||||||||
| import com.x8bit.bitwarden.data.autofill.util.getInlinePresentationSpecs | ||||||||||||||
| import com.x8bit.bitwarden.data.autofill.util.getMaxInlineSuggestionsCount | ||||||||||||||
| import com.x8bit.bitwarden.data.autofill.util.isEmailField | ||||||||||||||
| import com.x8bit.bitwarden.data.autofill.util.isPhoneField | ||||||||||||||
| import com.x8bit.bitwarden.data.autofill.util.toAutofillView | ||||||||||||||
| import com.x8bit.bitwarden.data.autofill.util.website | ||||||||||||||
| import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager | ||||||||||||||
|
|
@@ -118,16 +120,27 @@ class AutofillParserImpl( | |||||||||||||
| fillRequest: FillRequest?, | ||||||||||||||
| ): AutofillRequest { | ||||||||||||||
| Timber.d("Parsing AssistStructure -- ${fillRequest?.id}") | ||||||||||||||
| // Identity classification/fulfillment ship together: until this flag is on, every node | ||||||||||||||
| // must classify exactly as it did before identity heuristics existed, so behaviors like | ||||||||||||||
| // updateForMissingUsernameFields's Unused-only promotion keep working unchanged. | ||||||||||||||
| val isIdentityAutofillEnabled = featureFlagManager.getFeatureFlag(FlagKey.IdentityAutofill) | ||||||||||||||
| // Parse the `assistStructure` into internal models. | ||||||||||||||
| val traversalDataList = assistStructure.traverse() | ||||||||||||||
| val traversalDataList = assistStructure.traverse( | ||||||||||||||
| isIdentityAutofillEnabled = isIdentityAutofillEnabled, | ||||||||||||||
| ) | ||||||||||||||
| val urlBarWebsite = traversalDataList | ||||||||||||||
| .flatMap { it.urlBarWebsites } | ||||||||||||||
| .firstOrNull() | ||||||||||||||
| // Heuristic views: the focused node's candidates with unfillable (Unused) fields removed, | ||||||||||||||
| // falling back to all fillable views when nothing has focus. | ||||||||||||||
| // falling back to all fillable views when nothing has focus. Identity is also excluded | ||||||||||||||
| // here for now -- Identity partition construction lands in Phase D, so until then a field | ||||||||||||||
| // classified as Identity must keep falling through exactly as it would have as Unused | ||||||||||||||
| // (e.g. resolving to a sibling Login/Card field on the same form, or its Unused-only | ||||||||||||||
| // promotion in updateForMissingUsernameFields), not become the focused view and force this | ||||||||||||||
| // request to Unfillable. | ||||||||||||||
| val autofillViews = traversalDataList | ||||||||||||||
| .selectCandidateAutofillViews(urlBarWebsite = urlBarWebsite) { | ||||||||||||||
| it !is AutofillView.Unused | ||||||||||||||
| it !is AutofillView.Unused && it !is AutofillView.Identity | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| val isFillAssistEnabled = featureFlagManager | ||||||||||||||
|
|
@@ -187,8 +200,7 @@ class AutofillParserImpl( | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| is AutofillView.Identity -> { | ||||||||||||||
| // Identity partition construction is wired up in a later phase; treating it as | ||||||||||||||
| // unfillable for now is a no-op since nothing yet classifies a view as Identity. | ||||||||||||||
| // Identity partition construction lands in Phase D. Unfillable until then. | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. β CRITICAL: Classifying identity fields today (with no Identity partition) turns requests that used to be fillable into Details and fixBefore this PR an identity-looking field (e.g. Now those fields resolve to Traceable regressions:
This contradicts the PR description's "behavior-neutral in production until a later stacked phase". Suggested fix β keep Identity out of the heuristic candidate set until Phase D lands, e.g. in val autofillViews = traversalDataList
.selectCandidateAutofillViews(urlBarWebsite = urlBarWebsite) {
it !is AutofillView.Unused && it !is AutofillView.Identity
}or gate the new identity heuristics behind a feature flag so classification and fulfillment ship together.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @claude fixed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude finished @aj-rosado's task in 3m 31s ββ View job Re-checked against
|
||||||||||||||
| Test | Flag | Expected | If the gate moved above |
|---|---|---|---|
β¦Card Number when hint matches (ViewNodeExtensionsTest.kt:329-340) |
false |
Card.Number |
Unused β β fails |
β¦Login Username when is EditText and isUsernameField (:568-578) |
false |
Login.Username |
Unused β β fails |
Both are heuristic-path tests (setupUnsupportedInputFieldViewNode(), no autofillHints), so they run through the when rather than short-circuiting in firstSupportedAutofillHintOrNull β that's the part that matters. The inline comments you added on each (// Flag off pins the identity gate below the card/username heuristics.) are what stops someone "tidying" them back to true later, which was the real risk.
Coverage note, not a request: the remaining non-identity heuristics (password, the other six card branches) are still only exercised with the flag on. That's fine β the hoist failure mode is all-or-nothing, so two red tests is enough signal.
While I was in the file, the other items from this round check out too:
- Exhaustive
whens βbuildCardView/buildLoginView/buildIdentityView(AutofillViewBuilderExtensions.kt:13,:79,:98) eachwhenover an enum with noelse, soAutofillHint(AutofillHint.kt:7-50) being a sealed interface of three enums makes the compiler enforce them. Adding a hint constant now breaks the build at the dispatcher rather than silently falling through. - The ordering note β present at
ViewNodeExtensions.kt:169-170: "Identity heuristics only below here; anything else would stop matching when the flag is off. Add non-identity heuristics above this branch."
Nothing open from my side. The only carry-forward is housekeeping: the PR description still claims "the container-redirect dedup filter in traversal is updated to special-case AutofillView.Identity", which isn't in the diff β worth correcting so Phase D doesn't inherit the assumption, since that filter will need the change before whole-identity fill works.
(Static analysis only β ./gradlew isn't permitted in this environment, so CI remains the source of truth for these two tests passing.)
β’ branch PM-41292/identity-autofill-heuristic-field-detection
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| package com.x8bit.bitwarden.data.autofill.util | ||
|
|
||
| import android.app.assist.AssistStructure | ||
| import com.x8bit.bitwarden.data.autofill.model.AutofillHint | ||
| import com.x8bit.bitwarden.data.autofill.model.AutofillView | ||
|
|
||
| /** | ||
| * Builds an [AutofillView.Card] for the given card-related [autofillHint]. | ||
| */ | ||
| internal fun AssistStructure.ViewNode.buildCardView( | ||
| autofillOptions: List<String>, | ||
| autofillViewData: AutofillView.Data, | ||
| autofillHint: AutofillHint.Card, | ||
| ): AutofillView.Card = when (autofillHint) { | ||
| AutofillHint.Card.EXPIRATION_MONTH -> { | ||
| val monthValue = this | ||
| .autofillValue | ||
| ?.extractMonthValue( | ||
| autofillOptions = autofillOptions, | ||
| ) | ||
|
|
||
| AutofillView.Card.ExpirationMonth( | ||
| data = autofillViewData, | ||
| monthValue = monthValue, | ||
| ) | ||
| } | ||
|
|
||
| AutofillHint.Card.EXPIRATION_YEAR -> { | ||
| val yearValue = this | ||
| .autofillValue | ||
| ?.extractYearValue( | ||
| autofillOptions = autofillOptions, | ||
| ) | ||
|
|
||
| AutofillView.Card.ExpirationYear( | ||
| data = autofillViewData, | ||
| yearValue = yearValue, | ||
| ) | ||
| } | ||
|
|
||
| AutofillHint.Card.EXPIRATION_DATE -> { | ||
| AutofillView.Card.ExpirationDate( | ||
| data = autofillViewData, | ||
| ) | ||
| } | ||
|
|
||
| AutofillHint.Card.NUMBER -> { | ||
| AutofillView.Card.Number( | ||
| data = autofillViewData, | ||
| ) | ||
| } | ||
|
|
||
| AutofillHint.Card.SECURITY_CODE -> { | ||
| AutofillView.Card.SecurityCode( | ||
| data = autofillViewData, | ||
| ) | ||
| } | ||
|
|
||
| AutofillHint.Card.CARDHOLDER -> { | ||
| AutofillView.Card.CardholderName( | ||
| data = autofillViewData, | ||
| ) | ||
| } | ||
|
|
||
| AutofillHint.Card.BRAND -> { | ||
| val brandValue = this.autofillValue | ||
| ?.extractCardBrandValue( | ||
| autofillOptions = autofillOptions, | ||
| ) | ||
| AutofillView.Card.Brand( | ||
| data = autofillViewData, | ||
| brandValue = brandValue, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Builds an [AutofillView.Login] for the given login-related [autofillHint]. | ||
| */ | ||
| internal fun buildLoginView( | ||
| autofillViewData: AutofillView.Data, | ||
| autofillHint: AutofillHint.Login, | ||
| ): AutofillView.Login = when (autofillHint) { | ||
| AutofillHint.Login.PASSWORD -> { | ||
| AutofillView.Login.Password( | ||
| data = autofillViewData, | ||
| ) | ||
| } | ||
|
|
||
| AutofillHint.Login.USERNAME -> { | ||
| AutofillView.Login.Username( | ||
| data = autofillViewData, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Builds an [AutofillView.Identity] for the given identity-related [autofillHint]. | ||
| */ | ||
| internal fun buildIdentityView( | ||
| autofillViewData: AutofillView.Data, | ||
| autofillHint: AutofillHint.Identity, | ||
| ): AutofillView.Identity = when (autofillHint) { | ||
| AutofillHint.Identity.PERSON_NAME_FULL -> { | ||
| AutofillView.Identity.PersonNameFull(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.PERSON_NAME_PREFIX -> { | ||
| AutofillView.Identity.PersonNamePrefix(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.PERSON_NAME_GIVEN -> { | ||
| AutofillView.Identity.PersonNameGiven(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.PERSON_NAME_MIDDLE -> { | ||
| AutofillView.Identity.PersonNameMiddle(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.PERSON_NAME_FAMILY -> { | ||
| AutofillView.Identity.PersonNameFamily(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.POSTAL_ADDRESS_FULL -> { | ||
| AutofillView.Identity.PostalAddressFull(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.ADDRESS_STREET -> { | ||
| AutofillView.Identity.AddressStreet(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.ADDRESS_LOCALITY -> { | ||
| AutofillView.Identity.AddressLocality(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.ADDRESS_REGION -> { | ||
| AutofillView.Identity.AddressRegion(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.ADDRESS_COUNTRY -> { | ||
| AutofillView.Identity.AddressCountry(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.POSTAL_CODE -> { | ||
| AutofillView.Identity.PostalCode(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.PHONE_FULL -> { | ||
| AutofillView.Identity.PhoneFull(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.COMPANY -> { | ||
| AutofillView.Identity.Company(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.EMAIL -> { | ||
| // Produced by AutofillParserImpl's traverse(), not by this dispatch. | ||
| AutofillView.Identity.Email(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.SSN -> { | ||
| AutofillView.Identity.Ssn(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.PASSPORT_NUMBER -> { | ||
| AutofillView.Identity.PassportNumber(data = autofillViewData) | ||
| } | ||
|
|
||
| AutofillHint.Identity.LICENSE_NUMBER -> { | ||
| AutofillView.Identity.LicenseNumber(data = autofillViewData) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice