-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Bugfix FXIOS-9190 Hide username-less logins from autofill #20766
Bugfix FXIOS-9190 Hide username-less logins from autofill #20766
Conversation
- Only hides the login when accessing the LoginAutofillView from the username html field - Also hides the "Use saved password" button from the accessory view when selecting a username html field and there are no logins for the host URL with a username
@@ -40,6 +43,7 @@ class LoginListViewModel: ObservableObject { | |||
do { | |||
let logins = try await loginStorage.listLogins() | |||
self.logins = logins.filter { login in | |||
if field == FocusFieldType.username && login.decryptedUsername.isEmpty { return false } |
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.
Is EncryptedLogin.decryptedUsername
the best way to check if a login has a username? I noticed some fragility with that and keychain when trying to add tests in this commit
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.
in the meantime, I have removed tests
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.
Maybe @nbhasin2 knows the answer?
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.
Ideally you can check if you have decrypted logins before as keychain can be unreliable at times.
However from a quick glance its fine as we deal with per field encryption.
Client.app: Coverage: 30.43
Generated by 🚫 Danger Swift against f5947a9 |
@@ -40,6 +43,7 @@ class LoginListViewModel: ObservableObject { | |||
do { | |||
let logins = try await loginStorage.listLogins() | |||
self.logins = logins.filter { login in | |||
if field == FocusFieldType.username && login.decryptedUsername.isEmpty { return false } |
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.
Maybe @nbhasin2 knows the answer?
@@ -2792,14 +2793,17 @@ extension BrowserViewController: LegacyTabDelegate { | |||
method: .view, | |||
object: .loginsAutofillPromptShown | |||
) | |||
} else { |
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.
Nit: I learned that it's better to start with the positive case. So instead of checking for if !loginsForCurrentTab.isEmpty
, check if loginsForCurrentTab.isEmpty
first. Basically the if would be switched around.
📜 Tickets
Jira ticket
Github issue
💡 Description
Although not possible to create directly from Firefox for iOS, it is possible that saved logins with empty username fields can be synced over (i.e. via desktop). To manage this, we will still show synced username-less logins in the logins list (settings -> passwords), but will conditionally hide them from the login autofill sheet that is presented when attempting to autofill a login. The UX changes are as follows:
(no username)
above the********
password mark instead of the URL hostnameUsed saved password
accessory view button will no longer appear above the system keyboard when a username field gains first responder unless there are saved logins that contain a username📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120
)