-
Notifications
You must be signed in to change notification settings - Fork 999
[PM-37255] feat: Integrate fill-assist targeting rules into autofill parser #7066
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
base: main
Are you sure you want to change the base?
Changes from all commits
c646372
f57a7d0
8b6ce8b
09692af
00b2479
61db0cd
b718f56
f66485f
0b39ad2
b25404e
31a9cf7
bbdc2f1
ad2aada
9c5d53c
d6f0a47
6c0bfa5
8b05d40
1db11d1
20f9642
66b7f36
3a2fe1e
1a2ea61
6814294
02f1319
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 |
|---|---|---|
|
|
@@ -3,17 +3,22 @@ package com.x8bit.bitwarden.data.autofill.parser | |
| import android.app.assist.AssistStructure | ||
| import android.service.autofill.FillRequest | ||
| import android.view.autofill.AutofillId | ||
| import androidx.core.net.toUri | ||
| import com.bitwarden.core.data.manager.model.FlagKey | ||
| import com.x8bit.bitwarden.data.autofill.manager.FillAssistManager | ||
| import com.x8bit.bitwarden.data.autofill.model.AutofillAppInfo | ||
| import com.x8bit.bitwarden.data.autofill.model.AutofillPartition | ||
| import com.x8bit.bitwarden.data.autofill.model.AutofillRequest | ||
| import com.x8bit.bitwarden.data.autofill.model.AutofillView | ||
| import com.x8bit.bitwarden.data.autofill.model.ViewNodeTraversalData | ||
| import com.x8bit.bitwarden.data.autofill.util.buildFillAssistViews | ||
| 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.toAutofillView | ||
| import com.x8bit.bitwarden.data.autofill.util.website | ||
| import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager | ||
| import com.x8bit.bitwarden.data.platform.repository.SettingsRepository | ||
| import timber.log.Timber | ||
|
|
||
|
|
@@ -50,12 +55,30 @@ private val URL_BARS: Map<String, String> = mapOf( | |
| "com.brave.browser_nightly" to "url_bar", | ||
| ) | ||
|
|
||
| /** | ||
| * A list of categories from Fill Assist that are used for [AutofillView.Login] | ||
| */ | ||
| private val LOGIN_FILL_ASSIST_CATEGORIES: List<String> = listOf( | ||
| "account-login", | ||
| "account-creation", | ||
| "account-update", | ||
| ) | ||
|
|
||
| /** | ||
| * A list of categories from Fill Assist that are used for [AutofillView.Card] | ||
| */ | ||
| private val CARD_FILL_ASSIST_CATEGORIES: List<String> = listOf( | ||
| "payment-card", | ||
| ) | ||
|
|
||
| /** | ||
| * The default [AutofillParser] implementation for the app. This is a tool for parsing autofill data | ||
| * from the OS into domain models. | ||
| */ | ||
| class AutofillParserImpl( | ||
| private val settingsRepository: SettingsRepository, | ||
| private val fillAssistManager: FillAssistManager, | ||
| private val featureFlagManager: FeatureFlagManager, | ||
| ) : AutofillParser { | ||
| override fun parse( | ||
| autofillAppInfo: AutofillAppInfo, | ||
|
|
@@ -100,56 +123,44 @@ class AutofillParserImpl( | |
| val urlBarWebsite = traversalDataList | ||
| .flatMap { it.urlBarWebsites } | ||
| .firstOrNull() | ||
| val autofillViews = traversalDataList.toAutofillViews(urlBarWebsite = urlBarWebsite) | ||
|
|
||
| // Take only the autofill views from the node that currently has focus. | ||
| // Then remove all the fields that cannot be filled with data. | ||
| // We fallback to taking all the fillable views if nothing has focus. | ||
| val autofillViewsList = traversalDataList.map { it.autofillViews } | ||
| val autofillViews = (autofillViewsList | ||
| .filter { views -> views.any { it.data.isFocused } } | ||
| .flatten() | ||
| .filter { it !is AutofillView.Unused } | ||
| .takeUnless { it.isEmpty() } | ||
| ?: autofillViewsList | ||
| .flatten() | ||
| .filter { it !is AutofillView.Unused }) | ||
| .map { it.updateWebsiteIfNecessary(website = urlBarWebsite) } | ||
|
|
||
| // Find the focused view, or fallback to the first fillable item on the screen (so | ||
| // we at least have something to hook into) | ||
|
Collaborator
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. Can we leave in the comments in place? Autofill is confusing enough as it is, I want to make sure we leave goo notes for the next person. // Find the focused view, or fallback to the first fillable item on the screen (so
// we at least have something to hook into)
val focusedView = autofillViews
.firstOrNull { it.data.isFocused }
?: autofillViews.firstOrNull()
// The view is unfillable if there are no focused views.
?: return AutofillRequest.Unfillable |
||
| val focusedView = autofillViews | ||
| .firstOrNull { it.data.isFocused } | ||
| val focusedView = autofillViews.firstOrNull { it.data.isFocused } | ||
| ?: autofillViews.firstOrNull() | ||
| ?: return AutofillRequest.Unfillable | ||
|
|
||
| val packageName = | ||
| traversalDataList.buildPackageNameOrNull(assistStructure = assistStructure) | ||
|
Collaborator
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. Can we clean this up a bit val packageName = traversalDataList.buildPackageNameOrNull(
assistStructure = assistStructure,
) |
||
| val uri = focusedView.buildUriOrNull(packageName = packageName) | ||
|
|
||
| if (focusedView == null) { | ||
| // The view is unfillable if there are no focused views. | ||
| if ((settingsRepository.blockedAutofillUris + BLOCK_LISTED_URIS).contains(uri)) { | ||
| return AutofillRequest.Unfillable | ||
| } | ||
|
|
||
| val packageName = traversalDataList.buildPackageNameOrNull( | ||
| val effectiveViews = resolveEffectiveViews( | ||
| assistStructure = assistStructure, | ||
| ) | ||
| val uri = focusedView.buildUriOrNull( | ||
| packageName = packageName, | ||
| autofillViews = autofillViews, | ||
| uri = uri, | ||
| focusedView = focusedView, | ||
| urlBarWebsite = urlBarWebsite, | ||
| ) | ||
|
|
||
| val blockListedURIs = settingsRepository.blockedAutofillUris + BLOCK_LISTED_URIS | ||
| if (blockListedURIs.contains(uri)) { | ||
| // The view is unfillable if the URI is block listed. | ||
|
Collaborator
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. Same here, let's leave the comments in for posterity. |
||
| return AutofillRequest.Unfillable | ||
| } | ||
| val effectiveFocusedView = effectiveViews | ||
| .firstOrNull { it.data.isFocused } | ||
| ?: effectiveViews.firstOrNull() | ||
| ?: return AutofillRequest.Unfillable | ||
|
|
||
| // Choose the first focused partition of data for fulfillment. | ||
| val partition = when (focusedView) { | ||
| val partition = when (effectiveFocusedView) { | ||
| is AutofillView.Card -> { | ||
| AutofillPartition.Card( | ||
| views = autofillViews.filterIsInstance<AutofillView.Card>(), | ||
| views = effectiveViews.filterIsInstance<AutofillView.Card>(), | ||
| ) | ||
| } | ||
|
|
||
| is AutofillView.Login -> { | ||
| AutofillPartition.Login( | ||
| views = autofillViews.filterIsInstance<AutofillView.Login>(), | ||
| views = effectiveViews.filterIsInstance<AutofillView.Login>(), | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -166,7 +177,6 @@ class AutofillParserImpl( | |
|
|
||
| // Get inline information if available | ||
| val isInlineAutofillEnabled = settingsRepository.isInlineAutofillEnabled | ||
| Timber.d("Autofill request isInlineEnabled=$isInlineAutofillEnabled -- ${fillRequest?.id}") | ||
|
Collaborator
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. Why are we removing this? |
||
| val maxInlineSuggestionsCount = fillRequest.getMaxInlineSuggestionsCount( | ||
| autofillAppInfo = autofillAppInfo, | ||
| isInlineAutofillEnabled = isInlineAutofillEnabled, | ||
|
|
@@ -185,6 +195,46 @@ class AutofillParserImpl( | |
| uri = uri, | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Returns the effective [AutofillView] list for filling. Applies fill-assist targeting rules | ||
| * when the feature flag is enabled and the host rules cover the current partition type; | ||
| * otherwise returns the heuristic [autofillViews]. | ||
| */ | ||
| private fun resolveEffectiveViews( | ||
|
Collaborator
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. What do you think about making this an extension function? private fun List<AutofillView>.toEffectiveViews(
assistStructure: AssistStructure,
uri: String?,
focusedView: AutofillView,
urlBarWebsite: String?,
): List<AutofillView> { |
||
| assistStructure: AssistStructure, | ||
| autofillViews: List<AutofillView>, | ||
| uri: String?, | ||
| focusedView: AutofillView, | ||
| urlBarWebsite: String?, | ||
| ): List<AutofillView> { | ||
| val hostRules = uri | ||
| ?.takeUnless { it.startsWith("androidapp://") } | ||
| ?.toUri() | ||
| ?.host | ||
| ?.takeIf { featureFlagManager.getFeatureFlag(FlagKey.FillAssistTargetingRules) } | ||
|
Collaborator
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. Can we just return early instead of putting this check in the chain? if (!featureFlagManager.getFeatureFlag(FlagKey.FillAssistTargetingRules)) {
return autofillViews
} |
||
| ?.let { host -> | ||
| fillAssistManager.getFillAssistRules()?.hostRules?.get(host.removePrefix("www.")) | ||
| } | ||
| ?: return autofillViews | ||
|
|
||
| val coversCurrentPartition = hostRules.any { rule -> | ||
| when (focusedView) { | ||
| is AutofillView.Card -> rule.category in CARD_FILL_ASSIST_CATEGORIES | ||
| is AutofillView.Login -> rule.category in LOGIN_FILL_ASSIST_CATEGORIES | ||
| is AutofillView.Unused -> false | ||
| } | ||
| } | ||
|
|
||
| return if (coversCurrentPartition) { | ||
| assistStructure.buildFillAssistViews( | ||
| hostRules = hostRules, | ||
| urlBarWebsite = urlBarWebsite, | ||
| ) | ||
| } else { | ||
| autofillViews | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -201,6 +251,27 @@ private fun AssistStructure.traverse(): List<ViewNodeTraversalData> = | |
| ?.updateForMissingUsernameFields() | ||
| } | ||
|
|
||
| /** | ||
| * Assembles the [AutofillView] list from this [ViewNodeTraversalData] list. | ||
| * Take only the autofill views from the node that currently has focus. | ||
| * Then remove all the fields that cannot be filled with data. | ||
| * We fall back to taking all the fillable views if nothing has focus. | ||
| */ | ||
| private fun List<ViewNodeTraversalData>.toAutofillViews( | ||
| urlBarWebsite: String?, | ||
| ): List<AutofillView> { | ||
| val viewsLists = map { it.autofillViews } | ||
| return (viewsLists | ||
| .filter { views -> views.any { it.data.isFocused } } | ||
| .flatten() | ||
| .filter { it !is AutofillView.Unused } | ||
| .takeUnless { it.isEmpty() } | ||
| ?: viewsLists | ||
| .flatten() | ||
| .filter { it !is AutofillView.Unused }) | ||
| .map { it.updateWebsiteIfNecessary(website = urlBarWebsite) } | ||
|
Collaborator
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. I like that this was moved out into it's own method. What do you think of this minor update: private fun List<ViewNodeTraversalData>.toAutofillViews(
urlBarWebsite: String?,
): List<AutofillView> {
val viewsLists = map { it.autofillViews }
val autofillViewLists = viewsLists
.filter { views -> views.any { it.data.isFocused } }
.flatten()
.filter { it !is AutofillView.Unused }
.takeUnless { it.isEmpty() }
?: viewsLists
.flatten()
.filter { it !is AutofillView.Unused }
return autofillViewLists.map { it.updateWebsiteIfNecessary(website = urlBarWebsite) }
}I have never found that original block of code particularly readable π |
||
| } | ||
|
|
||
| /** | ||
| * This helper function updates the [ViewNodeTraversalData] if necessary for missing password | ||
| * fields that were marked invalid because they contained a specific `hint` or `idEntry`. If the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| package com.x8bit.bitwarden.data.autofill.util | ||
|
|
||
| import android.app.assist.AssistStructure | ||
| import com.x8bit.bitwarden.data.autofill.model.AutofillView | ||
| import com.x8bit.bitwarden.data.autofill.model.FillAssistRules | ||
|
|
||
| /** | ||
| * Traverses the [AssistStructure] and returns a list of [AutofillView]s classified by the | ||
| * provided [hostRules]. Only view nodes whose [android.view.ViewStructure.HtmlInfo] attributes | ||
| * match a [FillAssistRules.SelectorClause] are included; unmatched nodes are omitted (no | ||
| * heuristic fallback). | ||
| */ | ||
| internal fun AssistStructure.buildFillAssistViews( | ||
| hostRules: List<FillAssistRules.HostRule>, | ||
| urlBarWebsite: String?, | ||
| ): List<AutofillView> = | ||
| (0 until windowNodeCount) | ||
| .mapNotNull { getWindowNodeAt(it).rootViewNode } | ||
| .flatMap { it.traverseForFillAssist(hostRules = hostRules, parentWebsite = urlBarWebsite) } | ||
|
|
||
| private fun AssistStructure.ViewNode.traverseForFillAssist( | ||
| hostRules: List<FillAssistRules.HostRule>, | ||
| parentWebsite: String?, | ||
| ): List<AutofillView> { | ||
| val website = this.website ?: parentWebsite | ||
| val ownView = autofillId?.let { id -> | ||
| hostRules | ||
| .flatMap { it.fields.entries } | ||
| .filter { (_, alternatives) -> | ||
| alternatives.any { | ||
| htmlInfo?.matchesSelectorClause(it) ?: false | ||
| } | ||
| } | ||
| .takeIf { it.isNotEmpty() } | ||
| ?.let { matchingEntries -> | ||
| val data = toAutofillViewData(autofillId = id, website = website) | ||
| matchingEntries.firstNotNullOfOrNull { (key, _) -> | ||
| key.toAutofillViewForFieldKey( | ||
| data, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| val childViews = (0 until childCount) | ||
| .flatMap { index -> | ||
| getChildAt(index).traverseForFillAssist( | ||
| hostRules = hostRules, | ||
| parentWebsite = website, | ||
| ) | ||
| } | ||
| return listOfNotNull(ownView) + childViews | ||
| } | ||
|
|
||
| private fun String.toAutofillViewForFieldKey(data: AutofillView.Data): AutofillView? = when (this) { | ||
| "username" -> AutofillView.Login.Username(data = data) | ||
| "password", "newPassword" -> AutofillView.Login.Password(data = data) | ||
| "cardNumber" -> AutofillView.Card.Number(data = data) | ||
| "cardholderName" -> AutofillView.Card.CardholderName(data = data) | ||
| "cardExpirationDate" -> AutofillView.Card.ExpirationDate(data = data) | ||
| "cardExpirationMonth" -> AutofillView.Card.ExpirationMonth(data = data, monthValue = null) | ||
| "cardExpirationYear" -> AutofillView.Card.ExpirationYear(data = data, yearValue = null) | ||
| "cardCvv" -> AutofillView.Card.SecurityCode(data = data) | ||
| "cardType" -> AutofillView.Card.Brand(data = data, brandValue = null) | ||
| else -> null | ||
| } |
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.
The new
buildFillAssistViewsis awfully similar to the existing logic for parsing the assist structure. It's hard for me to tell if it is possible but can we optimize this to parse the structure only once?