Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix FXIOS-9190 Hide username-less logins from autofill #20766

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Bugfix FXIOS-9190 Hide username-less logins from autofill #20766

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
692a45f
Hides Usernameless-logins from autofill sheet
MattLichtenstein Jun 20, 2024
27980ce
Unit tests
MattLichtenstein Jun 21, 2024
e7bd60c
lint
MattLichtenstein Jun 21, 2024
a7085d8
Remove unit tests
MattLichtenstein Jun 24, 2024
f5947a9
improved control flow readability
MattLichtenstein Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions firefox-ios/Client/Coordinators/Browser/BrowserCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,9 +567,9 @@ class BrowserCoordinator: BaseCoordinator,
}

@MainActor
func showSavedLoginAutofill(tabURL: URL, currentRequestId: String) {
func showSavedLoginAutofill(tabURL: URL, currentRequestId: String, field: FocusFieldType) {
let bottomSheetCoordinator = makeCredentialAutofillCoordinator()
bottomSheetCoordinator.showSavedLoginAutofill(tabURL: tabURL, currentRequestId: currentRequestId)
bottomSheetCoordinator.showSavedLoginAutofill(tabURL: tabURL, currentRequestId: currentRequestId, field: field)
}

func showAddressAutofill(frame: WKFrameInfo?) {
Expand Down
2 changes: 1 addition & 1 deletion firefox-ios/Client/Coordinators/Browser/BrowserNavigationHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ protocol BrowserNavigationHandler: AnyObject, QRCodeNavigationHandler {

/// Displays an autofill interface for saved logins, allowing the user to select from stored login credentials
/// to autofill login forms on the specified web page.
func showSavedLoginAutofill(tabURL: URL, currentRequestId: String)
func showSavedLoginAutofill(tabURL: URL, currentRequestId: String, field: FocusFieldType)

/// Shows an AddressAutofill view for selecting addresses in order to autofill forms.
func showAddressAutofill(frame: WKFrameInfo?)
Expand Down
3 changes: 2 additions & 1 deletion firefox-ios/Client/Coordinators/CredentialAutofillCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,10 @@ class CredentialAutofillCoordinator: BaseCoordinator {
}

@MainActor
func showSavedLoginAutofill(tabURL: URL, currentRequestId: String) {
func showSavedLoginAutofill(tabURL: URL, currentRequestId: String, field: FocusFieldType) {
let viewModel = LoginListViewModel(
tabURL: tabURL,
field: field,
loginStorage: profile.logins,
logger: logger,
onLoginCellTap: { [weak self] login in
Expand Down
1 change: 1 addition & 0 deletions firefox-ios/Client/Frontend/Autofill/LoginAutofillView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ struct LoginAutofillView: View {
windowUUID: .XCTestDefaultUUID,
viewModel: LoginListViewModel(
tabURL: URL(string: "http://www.example.com", invalidCharacters: false)!,
field: FocusFieldType.username,
loginStorage: MockLoginStorage(),
logger: MockLogger(),
onLoginCellTap: { _ in },
Expand Down
3 changes: 2 additions & 1 deletion firefox-ios/Client/Frontend/Autofill/LoginCellView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ struct LoginCellView: View {
.foregroundColor(iconPrimary)
.alignmentGuide(.midAccountAndName) { $0[VerticalAlignment.center] }
VStack(alignment: .leading) {
Text(login.decryptedUsername.isEmpty ? login.hostname : login.decryptedUsername)
Text(login.decryptedUsername.isEmpty ? String.PasswordAutofill.LoginListCellNoUsername
: login.decryptedUsername)
.font(.body)
.foregroundColor(textColor)
.alignmentGuide(.midAccountAndName) { $0[VerticalAlignment.center] }
Expand Down
1 change: 1 addition & 0 deletions firefox-ios/Client/Frontend/Autofill/LoginListView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ struct LoginListView_Previews: PreviewProvider {
windowUUID: .XCTestDefaultUUID,
viewModel: LoginListViewModel(
tabURL: URL(string: "http://www.example.com", invalidCharacters: false)!,
field: FocusFieldType.username,
loginStorage: MockLoginStorage(),
logger: MockLogger(),
onLoginCellTap: { _ in },
Expand Down
4 changes: 4 additions & 0 deletions firefox-ios/Client/Frontend/Autofill/LoginListViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class LoginListViewModel: ObservableObject {
@Published var logins: [EncryptedLogin] = []

private let tabURL: URL
private let field: FocusFieldType
private let loginStorage: LoginStorage
private let logger: Logger
let onLoginCellTap: (EncryptedLogin) -> Void
Expand All @@ -24,12 +25,14 @@ class LoginListViewModel: ObservableObject {

init(
tabURL: URL,
field: FocusFieldType,
loginStorage: LoginStorage,
logger: Logger,
onLoginCellTap: @escaping (EncryptedLogin) -> Void,
manageLoginInfoAction: @escaping () -> Void
) {
self.tabURL = tabURL
self.field = field
self.loginStorage = loginStorage
self.logger = logger
self.onLoginCellTap = onLoginCellTap
Expand All @@ -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 }
Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

Copy link
Contributor

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?

Copy link
Contributor

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.

guard let recordHostnameURL = URL(string: login.hostname) else { return false }
return recordHostnameURL.baseDomain == tabURL.baseDomain
}
Expand Down
8 changes: 8 additions & 0 deletions firefox-ios/Client/Frontend/Autofill/LoginStorage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ class MockLoginStorage: LoginStorage {
persistence: .permanent
),
protectionSpace: URLProtectionSpace.fromOrigin("https://test.com")
),
EncryptedLogin(
credentials: URLCredential(
user: "",
password: "doubletest",
persistence: .permanent
),
protectionSpace: URLProtectionSpace.fromOrigin("https://test.com")
)
]

Expand Down
18 changes: 15 additions & 3 deletions firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2781,6 +2781,7 @@ extension BrowserViewController: LegacyTabDelegate {
guard let tabURL = tab?.url else { return }
let logins = (try? await self?.profile.logins.listLogins()) ?? []
let loginsForCurrentTab = logins.filter { login in
if field == FocusFieldType.username && login.decryptedUsername.isEmpty { return false }
guard let recordHostnameURL = URL(string: login.hostname) else { return false }
return recordHostnameURL.baseDomain == tabURL.baseDomain
}
Expand All @@ -2792,14 +2793,17 @@ extension BrowserViewController: LegacyTabDelegate {
method: .view,
object: .loginsAutofillPromptShown
)
} else {
Copy link
Contributor

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.

tab?.webView?.accessoryView.reloadViewFor(.standard)
}
tab?.webView?.accessoryView.savedLoginsClosure = {
Task { @MainActor [weak self] in
// Dismiss keyboard
webView?.resignFirstResponder()
self?.authenticateSelectSavedLoginsClosureBottomSheet(
tabURL: tabURL,
currentRequestId: currentRequestId
currentRequestId: currentRequestId,
field: field
)
}
}
Expand Down Expand Up @@ -2848,11 +2852,19 @@ extension BrowserViewController: LegacyTabDelegate {
tab.addContentScript(FocusHelper(tab: tab), name: FocusHelper.name())
}

private func authenticateSelectSavedLoginsClosureBottomSheet(tabURL: URL, currentRequestId: String) {
private func authenticateSelectSavedLoginsClosureBottomSheet(
tabURL: URL,
currentRequestId: String,
field: FocusFieldType
) {
appAuthenticator.getAuthenticationState { [unowned self] state in
switch state {
case .deviceOwnerAuthenticated:
self.navigationHandler?.showSavedLoginAutofill(tabURL: tabURL, currentRequestId: currentRequestId)
self.navigationHandler?.showSavedLoginAutofill(
tabURL: tabURL,
currentRequestId: currentRequestId,
field: field
)
case .deviceOwnerFailed:
// Keep showing bvc
break
Expand Down
5 changes: 5 additions & 0 deletions firefox-ios/Client/Frontend/Strings.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6318,6 +6318,11 @@ extension String {
tableName: "PasswordAutofill",
value: "Use saved password?",
comment: "This label is used in the password list screen header as a question, prompting the user if they want to use a saved password for logging in.")
public static let LoginListCellNoUsername = MZLocalizedString(
key: "PasswordAutofill.LoginListCellNoUsername.v129",
tableName: "PasswordAutofill",
value: "(no username)",
comment: "This label is used in a cell found in the list of autofill login options in place of an actual username to denote that no username was saved for this login")
public static let ManagePasswordsButton = MZLocalizedString(
key: "PasswordAutofill.ManagePasswordsButton.v124",
tableName: "PasswordAutofill",
Expand Down
25 changes: 24 additions & 1 deletion firefox-ios/firefox-ios-tests/Tests/ClientTests/Autofill/LoginListViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class LoginListViewModelTests: XCTestCase {
func testInitialization() {
let viewModel = LoginListViewModel(
tabURL: URL(string: "https://example.com")!,
field: FocusFieldType.username,
loginStorage: MockLoginStorage(),
logger: MockLogger(),
onLoginCellTap: { _ in },
Expand All @@ -25,11 +26,12 @@ class LoginListViewModelTests: XCTestCase {
}

@MainActor
func testFetchLoginsSuccess() async {
func testFetchLoginsSuccessUsernameField() async {
let mockLoginStorage = MockLoginStorage()

let viewModel = LoginListViewModel(
tabURL: URL(string: "https://test.com")!,
field: FocusFieldType.username,
loginStorage: mockLoginStorage,
logger: MockLogger(),
onLoginCellTap: { _ in },
Expand All @@ -41,6 +43,24 @@ class LoginListViewModelTests: XCTestCase {
XCTAssertEqual(viewModel.logins.count, 2)
}

@MainActor
func testFetchLoginsSuccessPaswordField() async {
let mockLoginStorage = MockLoginStorage()

let viewModel = LoginListViewModel(
tabURL: URL(string: "https://test.com")!,
field: FocusFieldType.password,
loginStorage: mockLoginStorage,
logger: MockLogger(),
onLoginCellTap: { _ in },
manageLoginInfoAction: { }
)

await viewModel.fetchLogins()

XCTAssertEqual(viewModel.logins.count, 3)
}

@MainActor
func testFetchLoginsFailure() async {
let mockLoginStorage = MockLoginStorage()
Expand All @@ -51,6 +71,7 @@ class LoginListViewModelTests: XCTestCase {

let viewModel = LoginListViewModel(
tabURL: URL(string: "https://example.com")!,
field: FocusFieldType.username,
loginStorage: mockLoginStorage,
logger: mockLogger,
onLoginCellTap: { _ in },
Expand All @@ -70,6 +91,7 @@ class LoginListViewModelTests: XCTestCase {

let viewModel = LoginListViewModel(
tabURL: URL(string: "https://example.com")!,
field: FocusFieldType.username,
loginStorage: MockLoginStorage(),
logger: MockLogger(),
onLoginCellTap: { _ in didTapLogin = true },
Expand All @@ -95,6 +117,7 @@ class LoginListViewModelTests: XCTestCase {

let viewModel = LoginListViewModel(
tabURL: URL(string: "https://example.com")!,
field: FocusFieldType.username,
loginStorage: MockLoginStorage(),
logger: MockLogger(),
onLoginCellTap: { _ in },
Expand Down
3 changes: 2 additions & 1 deletion firefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/BrowserCoordinatorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,8 @@ final class BrowserCoordinatorTests: XCTestCase {
let subject = createSubject()
let testURL = URL(string: "https://example.com")!
let currentRequestId = "testRequestID"
subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId)
let field = FocusFieldType.username
subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId, field: field)

XCTAssertEqual(subject.childCoordinators.count, 1)
XCTAssertTrue(subject.childCoordinators.first is CredentialAutofillCoordinator)
Expand Down
9 changes: 6 additions & 3 deletions ...firefox-ios-tests/Tests/ClientTests/Coordinators/CredentialAutofillCoordinatorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ final class CredentialAutofillCoordinatorTests: XCTestCase {

let testURL = URL(string: "https://example.com")!
let currentRequestId = "testRequestID"
let field = FocusFieldType.username

subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId)
subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId, field: field)

XCTAssertTrue(router.presentedViewController is BottomSheetViewController)
XCTAssertEqual(router.presentCalled, 1)
Expand All @@ -74,8 +75,9 @@ final class CredentialAutofillCoordinatorTests: XCTestCase {

let testURL = URL(string: "https://example.com")!
let currentRequestId = "testRequestID"
let field = FocusFieldType.username

subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId)
subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId, field: field)

if let bottomSheetViewController = router.presentedViewController as? BottomSheetViewController {
bottomSheetViewController.loadViewIfNeeded()
Expand All @@ -98,8 +100,9 @@ final class CredentialAutofillCoordinatorTests: XCTestCase {

let testURL = URL(string: "https://example.com")!
let currentRequestId = "testRequestID"
let field = FocusFieldType.username

subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId)
subject.showSavedLoginAutofill(tabURL: testURL, currentRequestId: currentRequestId, field: field)

if let bottomSheetViewController = router.presentedViewController as? BottomSheetViewController {
bottomSheetViewController.loadViewIfNeeded()
Expand Down
2 changes: 1 addition & 1 deletion ...x-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/Mocks/MockBrowserCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class MockBrowserCoordinator: BrowserNavigationHandler, ParentCoordinatorDelegat
showCreditCardAutofillCalled += 1
}

func showSavedLoginAutofill(tabURL: URL, currentRequestId: String) {
func showSavedLoginAutofill(tabURL: URL, currentRequestId: String, field: FocusFieldType) {
showLoginAutofillCalled += 1
}

Expand Down