[PM-39564] feat: Fill Assist support email field key for email-based login forms - #7107
Conversation
…assist-data-layer
…emoved unnecessary deserialization tests
…assist-data-layer
…ow updates Updating code to schema with required values
…assist-data-layer
github-actions
Bot
added
the
app:password-manager
aj-rosado
changed the title
…l-assist-option-settings # Conflicts: # app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserImpl.kt
…/fill-assist-username-support-emaill
…l-assist-option-settings # Conflicts: # app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserImpl.kt # app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/manager/FillAssistManagerTest.kt
…/fill-assist-username-support-emaill # Conflicts: # app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/FillAssistViewNodeExtensions.kt
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7107 +/- ##
==========================================
- Coverage 86.09% 85.81% -0.28%
==========================================
Files 885 928 +43
Lines 64733 66514 +1781
Branches 9638 9685 +47
==========================================
+ Hits 55729 57082 +1353
- Misses 5547 5964 +417
- Partials 3457 3468 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
# Conflicts: # app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/parser/AutofillParserTests.kt
…l-assist-option-settings
…l-assist-option-settings
…/fill-assist-username-support-emaill
# Conflicts: # app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/FillAssistViewNodeExtensions.kt # app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/autofill/AutoFillViewModel.kt # app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/FillAssistViewNodeExtensionsTest.kt # app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/autofill/AutoFillViewModelTest.kt
Review SummaryReviewed the fill-assist This is a clean, focused, and well-tested change. All exhaustive Findings
❓ Question — Notes (no action required)
Verification
Overall: LGTM pending the description/validation-strictness clarification. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ) { | ||
| val value = when (autofillView) { | ||
| is AutofillView.Login.Email -> { | ||
| if (!autofillCipher.username.isValidEmail()) { |
There was a problem hiding this comment.
❓ The PR description states the guard uses isValidEmail(useStrictValidation = false) (only checks for the presence of @), but this calls isValidEmail() which defaults to useStrictValidation = true — requiring a full local@domain.tld match.
The strict regex is ^[A-Za-z0-9._%+-/*]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$, so usernames a browser type="email" field would otherwise accept (e.g. admin@localhost, or user@intranet with no TLD) will be skipped and left unfilled.
Is strict validation the intended behavior here? If so, consider updating the PR description to match. If non-strict was intended, pass useStrictValidation = false. The KDoc on Login.Email and the tests are consistent with the current (strict) implementation, so this is likely just a stale description — flagging to reconcile.
There was a problem hiding this comment.
@claude description updated to match the behaviour
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 29s —— View job
Reconciliation confirmed ✅
Thanks @aj-rosado — the description now matches the implementation.
To confirm the resolution: the guard at FilledDataBuilderImpl.kt:157 calls isValidEmail(), which uses the default useStrictValidation = true:
is AutofillView.Login.Email -> {
if (!autofillCipher.username.isValidEmail()) {
return@mapNotNull null
}
autofillCipher.username
}So the strict regex (^[A-Za-z0-9._%+-/*]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$) is what governs whether the type="email" field gets filled. The KDoc on Login.Email and the unit tests were already consistent with this strict behaviour, and the description is now aligned too.
No code change needed — my original comment was a doc/code consistency flag, and it's now resolved.
| } | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
This suppression is not needed, right?
| verify(exactly = 1) { autofillViewEmail.buildFilledItemOrNull(emailUsername) } | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
| isVaultLocked = false, | ||
| ) | ||
| coEvery { autofillCipherProvider.getLoginAutofillCiphers(uri = URI) } returns | ||
| listOf(autofillCipher) |
There was a problem hiding this comment.
Can we format this:
```kotlin
coEvery {
autofillCipherProvider.getLoginAutofillCiphers(uri = URI)
} returns listOf(autofillCipher)| isVaultLocked = false, | ||
| ) | ||
| coEvery { autofillCipherProvider.getLoginAutofillCiphers(uri = URI) } returns | ||
| listOf(autofillCipher) |
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-39564
📔 Objective
Fill-assist rules in the forms JSON use
"email"as the field key for sites where users log in with their email address (e.g.{ "email": ["input[type='email']"] }). Previously this key was unrecognised — it fell through toelse -> nullintoAutofillViewForFieldKey, so autofill never triggered for those fields.This PR adds a dedicated
AutofillView.Login.Emailsealed subtype and wires it through the fill pipeline with a validation guard: the email field is only filled when the cipher's stored username mathes the email pattern If the username is not an email address the field is skipped, avoiding placing a non-email value into antype="email"input.AutofillView.Login.Emailadded alongsideUsernameandPassword"email"key maps toLogin.EmailintoAutofillViewForFieldKeyFilledDataBuilderImplguardsLogin.Email: skips fill whenusernamedoes not passisValidEmail(useStrictValidation = false)AutofillPartitionExtensions.usernameSaveValueupdated to captureLogin.Emailtext values so saving credentials from email-only forms preserves the typed valuewhenexpressions onAutofillViewupdated (AutofillViewExtensions,AutofillParserImpl.updateWebsiteIfNecessary)