From 34ec442961e7f54dd23e381ccfb012ad548d93ea Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 16 Sep 2021 16:42:56 -0400 Subject: [PATCH] Updating Fenix to work with the new logins API Switched to always using `Login` instead of the `SavedPassword` alias. Made `MasterPasswordTipProvider.saveLogins()` call `importLoginsAsync()`. This is needed because it's the only method that inputs a `Login` rather than a `LoginEntry`. Moved the `SavedLoginsStorageController.kt.syncAndUpdateList` call to inside `add()` and `update()`. This simplifies the error handling a bit. Refactored dupe-checking code to use findLoginToUpdate() Refactored `AddLoginFragment` / `EditLoginFragment` to put the username error handling code all in 1 method. I think it's easier to follow the logic of showing/hiding the error labels when it's all in one place. This fixes issues #24103 and #24104. I would love to address #24102, but I'm not sure what the correct behavior is there so I just kept that the same. --- .../java/org/mozilla/fenix/components/Core.kt | 19 +- .../providers/MasterPasswordTipProvider.kt | 27 +-- .../settings/logins/LoginsFragmentStore.kt | 14 +- .../SavedLoginsStorageController.kt | 148 ++++++------- .../logins/fragment/AddLoginFragment.kt | 92 ++++---- .../logins/fragment/EditLoginFragment.kt | 68 +++--- .../logins/interactor/AddLoginInteractor.kt | 8 +- .../logins/interactor/EditLoginInteractor.kt | 4 +- .../settings/logins/AddLoginInteractorTest.kt | 4 +- .../logins/EditLoginInteractorTest.kt | 6 +- .../settings/logins/LoginDetailViewTest.kt | 2 +- .../logins/LoginsFragmentStoreTest.kt | 4 +- .../SavedLoginsStorageControllerTest.kt | 203 +++++++++++++++--- 13 files changed, 351 insertions(+), 248 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/components/Core.kt b/app/src/main/java/org/mozilla/fenix/components/Core.kt index 1606b4c54..4f60fbdfc 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Core.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Core.kt @@ -294,7 +294,7 @@ class Core( // We can fully initialize GeckoEngine without initialized our storage. val lazyHistoryStorage = lazyMonitored { PlacesHistoryStorage(context, crashReporter) } val lazyBookmarksStorage = lazyMonitored { PlacesBookmarksStorage(context) } - val lazyPasswordsStorage = lazyMonitored { SyncableLoginsStorage(context, passwordsEncryptionKey) } + val lazyPasswordsStorage = lazyMonitored { SyncableLoginsStorage(context, lazySecurePrefs) } val lazyAutofillStorage = lazyMonitored { AutofillCreditCardsAddressesStorage(context, lazySecurePrefs) } /** @@ -418,23 +418,6 @@ class Core( // Temporary. See https://github.com/mozilla-mobile/fenix/issues/19155 private val lazySecurePrefs = lazyMonitored { getSecureAbove22Preferences() } - - private val passwordsEncryptionKey by lazyMonitored { - getSecureAbove22Preferences().getString(PASSWORDS_KEY) - ?: generateEncryptionKey(KEY_STRENGTH).also { - if (context.settings().passwordsEncryptionKeyGenerated) { - // We already had previously generated an encryption key, but we have lost it - crashReporter.submitCaughtException( - IllegalStateException( - "Passwords encryption key for passwords storage was lost and we generated a new one" - ) - ) - } - context.settings().recordPasswordsEncryptionKeyGenerated() - getSecureAbove22Preferences().putString(PASSWORDS_KEY, it) - } - } - val trackingProtectionPolicyFactory = TrackingProtectionPolicyFactory(context.settings()) /** diff --git a/app/src/main/java/org/mozilla/fenix/components/tips/providers/MasterPasswordTipProvider.kt b/app/src/main/java/org/mozilla/fenix/components/tips/providers/MasterPasswordTipProvider.kt index 95e582962..cfa54b7af 100644 --- a/app/src/main/java/org/mozilla/fenix/components/tips/providers/MasterPasswordTipProvider.kt +++ b/app/src/main/java/org/mozilla/fenix/components/tips/providers/MasterPasswordTipProvider.kt @@ -19,11 +19,9 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.launch import kotlinx.coroutines.withContext -import mozilla.components.service.sync.logins.IdCollisionException +import mozilla.components.concept.storage.Login import mozilla.components.service.sync.logins.InvalidRecordException import mozilla.components.service.sync.logins.LoginsStorageException -import mozilla.components.service.sync.logins.ServerPassword -import mozilla.components.service.sync.logins.toLogin import mozilla.components.support.migration.FennecLoginsMPImporter import mozilla.components.support.migration.FennecProfile import org.mozilla.fenix.R @@ -186,21 +184,16 @@ class MasterPasswordTipProvider( } } - private fun saveLogins(logins: List, dialog: AlertDialog) { + private fun saveLogins(logins: List, dialog: AlertDialog) { CoroutineScope(IO).launch { - logins.map { it.toLogin() }.forEach { - try { - context.components.core.passwordsStorage.add(it) - } catch (e: InvalidRecordException) { - // This record was invalid and we couldn't save this login - context.components.analytics.crashReporter.submitCaughtException(e) - } catch (e: IdCollisionException) { - // Nonempty ID was provided - context.components.analytics.crashReporter.submitCaughtException(e) - } catch (e: LoginsStorageException) { - // Some other error occurred - context.components.analytics.crashReporter.submitCaughtException(e) - } + try { + context.components.core.passwordsStorage.importLoginsAsync(logins) + } catch (e: InvalidRecordException) { + // This record was invalid and we couldn't save this login + context.components.analytics.crashReporter.submitCaughtException(e) + } catch (e: LoginsStorageException) { + // Some other error occurred + context.components.analytics.crashReporter.submitCaughtException(e) } withContext(Dispatchers.Main) { // Step 3: Dismiss this dialog and show the success dialog diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt index 3a1d743fa..beff18787 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt @@ -31,7 +31,7 @@ data class SavedLogin( fun Login.mapToSavedLogin(): SavedLogin = SavedLogin( - guid = this.guid!!, + guid = this.guid, origin = this.origin, username = this.username, password = this.password, @@ -55,7 +55,7 @@ sealed class LoginsAction : Action { data class UpdateLoginsList(val list: List) : LoginsAction() data class UpdateCurrentLogin(val item: SavedLogin) : LoginsAction() data class SortLogins(val sortingStrategy: SortingStrategy) : LoginsAction() - data class ListOfDupes(val dupeList: List) : LoginsAction() + data class DuplicateLogin(val dupe: SavedLogin?) : LoginsAction() data class LoginSelected(val item: SavedLogin) : LoginsAction() } @@ -67,8 +67,7 @@ sealed class LoginsAction : Action { * @property sortingStrategy sorting strategy selected by the user (Currently we support * sorting alphabetically and by last used) * @property highlightedItem The current selected sorting strategy from the sort menu - * @property duplicateLogins The current list of possible duplicates for a selected login origin, - * httpRealm, and formActionOrigin + * @property duplicateLogin Duplicate login for the current add/save login form */ data class LoginsListState( val isLoading: Boolean = false, @@ -78,7 +77,7 @@ data class LoginsListState( val searchedForText: String?, val sortingStrategy: SortingStrategy, val highlightedItem: SavedLoginsSortingStrategyMenu.Item, - val duplicateLogins: List + val duplicateLogin: SavedLogin? = null, ) : State fun createInitialLoginsListState(settings: Settings) = LoginsListState( @@ -88,7 +87,6 @@ fun createInitialLoginsListState(settings: Settings) = LoginsListState( searchedForText = null, sortingStrategy = settings.savedLoginsSortingStrategy, highlightedItem = settings.savedLoginsMenuHighlightedItem, - duplicateLogins = emptyList() // assume on load there are no dupes ) /** @@ -132,9 +130,9 @@ private fun savedLoginsStateReducer( filteredItems = emptyList() ) } - is LoginsAction.ListOfDupes -> { + is LoginsAction.DuplicateLogin -> { state.copy( - duplicateLogins = action.dupeList + duplicateLogin = action.dupe ) } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt index 69efa7eed..fa7d21b2b 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt @@ -15,6 +15,7 @@ import kotlinx.coroutines.async import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import mozilla.components.concept.storage.Login +import mozilla.components.concept.storage.LoginEntry import mozilla.components.service.sync.logins.InvalidRecordException import mozilla.components.service.sync.logins.LoginsStorageException import mozilla.components.service.sync.logins.NoSuchRecordException @@ -25,6 +26,7 @@ import org.mozilla.fenix.settings.logins.LoginsFragmentStore import org.mozilla.fenix.settings.logins.fragment.EditLoginFragmentDirections import org.mozilla.fenix.settings.logins.fragment.AddLoginFragmentDirections import org.mozilla.fenix.settings.logins.mapToSavedLogin +import org.mozilla.fenix.settings.logins.SavedLogin /** * Controller for all saved logins interactions with the password storage component @@ -58,22 +60,20 @@ open class SavedLoginsStorageController( } } - fun add(hostnameText: String, usernameText: String, passwordText: String) { + // Create a [LoginEntry] for the add login dialog + private fun loginEntryForAdd(originText: String, usernameText: String, passwordText: String) = LoginEntry( + origin = originText, + username = usernameText, + password = passwordText, + // Implicitly fill in httpRealm with the origin + httpRealm = originText + ) + + fun add(originText: String, usernameText: String, passwordText: String) { var saveLoginJob: Deferred? = null lifecycleScope.launch(ioDispatcher) { saveLoginJob = async { - val loginToSave = Login( - guid = null, - origin = hostnameText, - username = usernameText, - password = passwordText, - httpRealm = hostnameText - ) - val newLoginId = add(loginToSave) - if (newLoginId.isNotEmpty()) { - val newLogin = passwordsStorage.get(newLoginId) - syncAndUpdateList(newLogin!!) - } + add(loginEntryForAdd(originText, usernameText, passwordText)) } saveLoginJob?.await() withContext(Dispatchers.Main) { @@ -89,39 +89,38 @@ open class SavedLoginsStorageController( } } - private suspend fun add(loginToSave: Login): String { - var newLoginId = "" + private suspend fun add(loginEntryToSave: LoginEntry) { try { - newLoginId = passwordsStorage.add(loginToSave) + val encryptedLogin = passwordsStorage.add(loginEntryToSave) + syncAndUpdateList(passwordsStorage.decryptLogin(encryptedLogin)) } catch (loginException: LoginsStorageException) { Log.e( "Add new login", "Failed to add new login.", loginException ) } - return newLoginId + } + + // Create a [LoginEntry] for the edit login dialog + private suspend fun loginEntryForSave(loginId: String, usernameText: String, passwordText: String): LoginEntry { + // must retrieve from storage to get the httpsRealm and formActionOrigin + val oldLogin = passwordsStorage.get(loginId)!! + return LoginEntry( + // Copied from the existing login + origin = oldLogin.origin, + httpRealm = oldLogin.httpRealm, + formActionOrigin = oldLogin.formActionOrigin, + // New values + username = usernameText, + password = passwordText, + ) } fun save(loginId: String, usernameText: String, passwordText: String) { var saveLoginJob: Deferred? = null lifecycleScope.launch(ioDispatcher) { saveLoginJob = async { - // must retrieve from storage to get the httpsRealm and formActionOrigin - val oldLogin = passwordsStorage.get(loginId) - - // Update requires a Login type, which needs at least one of - // httpRealm or formActionOrigin - val loginToSave = Login( - guid = loginId, - origin = oldLogin?.origin!!, - username = usernameText, // new value - password = passwordText, // new value - httpRealm = oldLogin.httpRealm, - formActionOrigin = oldLogin.formActionOrigin - ) - - save(loginToSave) - syncAndUpdateList(loginToSave) + save(loginId, loginEntryForSave(loginId, usernameText, passwordText)) } saveLoginJob?.await() withContext(Dispatchers.Main) { @@ -139,9 +138,10 @@ open class SavedLoginsStorageController( } } - private suspend fun save(loginToSave: Login) { + private suspend fun save(guid: String, loginEntryToSave: LoginEntry) { try { - passwordsStorage.update(loginToSave) + val encryptedLogin = passwordsStorage.update(guid, loginEntryToSave) + syncAndUpdateList(passwordsStorage.decryptLogin(encryptedLogin)) } catch (loginException: LoginsStorageException) { when (loginException) { is NoSuchRecordException, @@ -168,62 +168,40 @@ open class SavedLoginsStorageController( ) } - fun findPotentialDuplicates(loginId: String) { - var deferredLogin: Deferred>? = null - val fetchLoginJob = lifecycleScope.launch(ioDispatcher) { - deferredLogin = async { - val login = getLogin(loginId) - passwordsStorage.getPotentialDupesIgnoringUsername(login!!) - } - val fetchedDuplicatesList = deferredLogin?.await() - fetchedDuplicatesList?.let { list -> - withContext(Dispatchers.Main) { - val savedLoginList = list.map { it.mapToSavedLogin() } - loginsFragmentStore.dispatch( - LoginsAction.ListOfDupes( - savedLoginList - ) - ) - } - } + fun findDuplicateForAdd(originText: String, usernameText: String, passwordText: String) { + lifecycleScope.launch(ioDispatcher) { + findDuplicate( + loginEntryForAdd(originText, usernameText, passwordText) + ) } - fetchLoginJob.invokeOnCompletion { - if (it is CancellationException) { - deferredLogin?.cancel() - } + } + + fun findDuplicateForSave(loginId: String, usernameText: String, passwordText: String) { + lifecycleScope.launch(ioDispatcher) { + findDuplicate( + loginEntryForSave(loginId, usernameText, passwordText), + loginId + ) } } - fun findPotentialDuplicates(hostnameText: String, usernameText: String, passwordText: String) { - var deferredLogin: Deferred>? = null - val fetchLoginJob = lifecycleScope.launch(ioDispatcher) { - deferredLogin = async { - val login = Login( - guid = null, - origin = hostnameText, - username = usernameText, - password = passwordText, - httpRealm = hostnameText - ) - passwordsStorage.getPotentialDupesIgnoringUsername(login) - } - val fetchedDuplicatesList = deferredLogin?.await() - fetchedDuplicatesList?.let { list -> - withContext(Dispatchers.Main) { - val savedLoginList = list.map { it.mapToSavedLogin() } - loginsFragmentStore.dispatch( - LoginsAction.ListOfDupes( - savedLoginList - ) - ) - } - } + suspend fun findDuplicate(entry: LoginEntry, currentGuid: String? = null) { + // Ensure that we have a valid, non-blank password. The value doesn't + // matter for dupe-checking and we want to make sure that + // findLoginToUpdate() doesn't throw an error because the [LoginEntry] + // is invalid + val validEntry = if (entry.password.isNotEmpty()) entry else entry.copy(password="password") + var dupe = try { + passwordsStorage.findLoginToUpdate(validEntry)?.mapToSavedLogin() + } catch (e: LoginsStorageException) { + // If the entry was invalid, then consider it not a dupe + null } - fetchLoginJob.invokeOnCompletion { - if (it is CancellationException) { - deferredLogin?.cancel() - } + if (dupe != null && dupe.guid == currentGuid) { + // If the found login matches the current login, don't consider it a dupe + dupe = null } + loginsFragmentStore.dispatch(LoginsAction.DuplicateLogin(dupe)) } fun fetchLoginDetails(loginId: String) { diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/AddLoginFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/AddLoginFragment.kt index c2353524c..e99732f1f 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/AddLoginFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/AddLoginFragment.kt @@ -48,11 +48,12 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login) { private lateinit var loginsFragmentStore: LoginsFragmentStore private lateinit var interactor: AddLoginInteractor - private var listOfPossibleDupes: List? = null + private var duplicateLogin: SavedLogin? = null private var validPassword = true private var validUsername = true private var validHostname = false + private var usernameChanged = false private var _binding: FragmentAddLoginBinding? = null private val binding get() = _binding!! @@ -82,9 +83,11 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login) { setUpClickListeners() setUpTextListeners() + findDuplicate() consumeFrom(loginsFragmentStore) { - listOfPossibleDupes = loginsFragmentStore.state.duplicateLogins + duplicateLogin = loginsFragmentStore.state.duplicateLogin + updateUsernameField() } } @@ -172,11 +175,7 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login) { binding.inputLayoutHostname.error = null binding.inputLayoutHostname.errorIconDrawable = null - interactor.findPotentialDuplicates( - hostnameText = h.toString(), - binding.usernameText.text.toString(), - binding.passwordText.text.toString() - ) + findDuplicate() } } setSaveButtonState() @@ -193,19 +192,10 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login) { binding.usernameText.addTextChangedListener(object : TextWatcher { override fun afterTextChanged(u: Editable?) { - when { - u.toString().isEmpty() -> { - binding.clearUsernameTextButton.isVisible = false - setUsernameError() - } - else -> { - setDupeError() - binding.inputLayoutUsername.error = null - binding.inputLayoutUsername.errorIconDrawable = null - } - } - binding.clearUsernameTextButton.isEnabled = u.toString().isNotEmpty() + usernameChanged = true + updateUsernameField() setSaveButtonState() + findDuplicate() } override fun beforeTextChanged(u: CharSequence?, start: Int, count: Int, after: Int) { @@ -244,28 +234,51 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login) { }) } - private fun isDupe(username: String): Boolean = - loginsFragmentStore.state.duplicateLogins.filter { it.username == username }.any() + private fun findDuplicate() { + interactor.findDuplicate( + binding.hostnameText.text.toString(), + binding.usernameText.text.toString(), + binding.passwordText.text.toString(), + ) + } - private fun setDupeError() { - if (isDupe(binding.usernameText.text.toString())) { - binding.inputLayoutUsername.let { + private fun updateUsernameField() { + val currentValue = binding.usernameText.text.toString() + val layout = binding.inputLayoutUsername + val clearButton = binding.clearUsernameTextButton + when { + currentValue.isEmpty() && usernameChanged -> { + // Invalid username because it's empty (although this is not true when editing logins) validUsername = false - it.error = context?.getString(R.string.saved_login_duplicate) - it.setErrorIconDrawable(R.drawable.mozac_ic_warning_with_bottom_padding) - it.setErrorIconTintList( + layout.error = context?.getString(R.string.saved_login_username_required) + layout.setErrorIconDrawable(R.drawable.mozac_ic_warning_with_bottom_padding) + layout.setErrorIconTintList( ColorStateList.valueOf( ContextCompat.getColor(requireContext(), R.color.design_error) ) ) - binding.clearUsernameTextButton.isVisible = false } - } else { - validUsername = true - binding.inputLayoutUsername.error = null - binding.inputLayoutUsername.errorIconDrawable = null - binding.clearUsernameTextButton.isVisible = true + duplicateLogin != null -> { + // Invalid username because it's a dupe of another login + validUsername = false + layout.error = context?.getString(R.string.saved_login_duplicate) + layout.setErrorIconDrawable(R.drawable.mozac_ic_warning_with_bottom_padding) + layout.setErrorIconTintList( + ColorStateList.valueOf( + ContextCompat.getColor(requireContext(), R.color.design_error) + ) + ) + } + else -> { + // Valid username + validUsername = true + layout.error = null + layout.errorIconDrawable = null + } } + clearButton.isVisible = validUsername + clearButton.isEnabled = validUsername + setSaveButtonState() } private fun setPasswordError() { @@ -281,19 +294,6 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login) { } } - private fun setUsernameError() { - binding.inputLayoutUsername.let { layout -> - validUsername = false - layout.error = context?.getString(R.string.saved_login_username_required) - layout.setErrorIconDrawable(R.drawable.mozac_ic_warning_with_bottom_padding) - layout.setErrorIconTintList( - ColorStateList.valueOf( - ContextCompat.getColor(requireContext(), R.color.design_error) - ) - ) - } - } - private fun setHostnameError() { binding.inputLayoutHostname.let { layout -> validHostname = false diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt index cadb1f17f..3eeb2c232 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt @@ -51,7 +51,7 @@ class EditLoginFragment : Fragment(R.layout.fragment_edit_login) { private lateinit var interactor: EditLoginInteractor private lateinit var oldLogin: SavedLogin - private var listOfPossibleDupes: List? = null + private var duplicateLogin: SavedLogin? = null private var usernameChanged = false private var passwordChanged = false @@ -86,7 +86,6 @@ class EditLoginFragment : Fragment(R.layout.fragment_edit_login) { ) loginsFragmentStore.dispatch(LoginsAction.UpdateCurrentLogin(args.savedLoginItem)) - interactor.findPotentialDuplicates(args.savedLoginItem.guid) // initialize editable values binding.hostnameText.text = args.savedLoginItem.origin.toEditable() @@ -99,9 +98,11 @@ class EditLoginFragment : Fragment(R.layout.fragment_edit_login) { setUpClickListeners() setUpTextListeners() togglePasswordReveal(binding.passwordText, binding.revealPasswordButton) + findDuplicate() consumeFrom(loginsFragmentStore) { - listOfPossibleDupes = loginsFragmentStore.state.duplicateLogins + duplicateLogin = loginsFragmentStore.state.duplicateLogin + updateUsernameField() } } @@ -150,20 +151,8 @@ class EditLoginFragment : Fragment(R.layout.fragment_edit_login) { binding.usernameText.addTextChangedListener(object : TextWatcher { override fun afterTextChanged(u: Editable?) { - when (oldLogin.username) { - u.toString() -> { - usernameChanged = false - validUsername = true - binding.inputLayoutUsername.error = null - binding.inputLayoutUsername.errorIconDrawable = null - binding.clearUsernameTextButton.isVisible = true - } - else -> { - usernameChanged = true - setDupeError() - } - } - binding.clearUsernameTextButton.isEnabled = u.toString().isNotEmpty() + updateUsernameField() + findDuplicate() setSaveButtonState() } @@ -215,30 +204,45 @@ class EditLoginFragment : Fragment(R.layout.fragment_edit_login) { }) } - private fun isDupe(username: String): Boolean = - loginsFragmentStore.state.duplicateLogins.filter { it.username == username }.any() + private fun findDuplicate() { + interactor.findDuplicate( + oldLogin.guid, + binding.usernameText.text.toString(), + binding.passwordText.text.toString(), + ) + } - private fun setDupeError() { - if (isDupe(binding.usernameText.text.toString())) { - binding.inputLayoutUsername.let { + private fun updateUsernameField() { + val currentValue = binding.usernameText.text.toString() + val layout = binding.inputLayoutUsername + val clearButton = binding.clearUsernameTextButton + when { + (duplicateLogin == null || oldLogin.username == currentValue) -> { + // Valid login, either because there's no dupe or because the + // existing login was already a dupe and the username hasn't + // changed + usernameChanged = oldLogin.username != currentValue + validUsername = true + layout.error = null + layout.errorIconDrawable = null + clearButton.isVisible = true + } + else -> { + // Invalid login because it's a dupe of another one usernameChanged = true validUsername = false - it.error = context?.getString(R.string.saved_login_duplicate) - it.setErrorIconDrawable(R.drawable.mozac_ic_warning_with_bottom_padding) - it.setErrorIconTintList( + layout.error = context?.getString(R.string.saved_login_duplicate) + layout.setErrorIconDrawable(R.drawable.mozac_ic_warning_with_bottom_padding) + layout.setErrorIconTintList( ColorStateList.valueOf( ContextCompat.getColor(requireContext(), R.color.design_error) ) ) - binding.clearUsernameTextButton.isVisible = false + clearButton.isVisible = false } - } else { - usernameChanged = true - validUsername = true - binding.inputLayoutUsername.error = null - binding.inputLayoutUsername.errorIconDrawable = null - binding.clearUsernameTextButton.isVisible = true } + clearButton.isEnabled = currentValue.isNotEmpty() + setSaveButtonState() } private fun setPasswordError() { diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/AddLoginInteractor.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/AddLoginInteractor.kt index 8b1421d6f..76d9a340f 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/AddLoginInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/AddLoginInteractor.kt @@ -14,11 +14,11 @@ import org.mozilla.fenix.settings.logins.controller.SavedLoginsStorageController class AddLoginInteractor( private val savedLoginsController: SavedLoginsStorageController ) { - fun findPotentialDuplicates(hostnameText: String, usernameText: String, passwordText: String) { - savedLoginsController.findPotentialDuplicates(hostnameText, usernameText, passwordText) + fun findDuplicate(originText: String, usernameText: String, passwordText: String) { + savedLoginsController.findDuplicateForAdd(originText, usernameText, passwordText) } - fun onAddLogin(hostnameText: String, usernameText: String, passwordText: String) { - savedLoginsController.add(hostnameText, usernameText, passwordText) + fun onAddLogin(originText: String, usernameText: String, passwordText: String) { + savedLoginsController.add(originText, usernameText, passwordText) } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/EditLoginInteractor.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/EditLoginInteractor.kt index 3039dd693..2e635047b 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/EditLoginInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/interactor/EditLoginInteractor.kt @@ -14,8 +14,8 @@ import org.mozilla.fenix.settings.logins.controller.SavedLoginsStorageController class EditLoginInteractor( private val savedLoginsController: SavedLoginsStorageController ) { - fun findPotentialDuplicates(loginId: String) { - savedLoginsController.findPotentialDuplicates(loginId) + fun findDuplicate(loginId: String, usernameText: String, passwordText: String) { + savedLoginsController.findDuplicateForSave(loginId, usernameText, passwordText) } fun onSaveLogin(loginId: String, usernameText: String, passwordText: String) { diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/AddLoginInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/AddLoginInteractorTest.kt index 7fbf38656..c55461c66 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/AddLoginInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/AddLoginInteractorTest.kt @@ -21,14 +21,14 @@ class AddLoginInteractorTest { @Test fun findPotentialDupesTest() { - interactor.findPotentialDuplicates( + interactor.findDuplicate( hostname, username, password ) verify { - loginsController.findPotentialDuplicates( + loginsController.findDuplicateForAdd( hostname, username, password diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/EditLoginInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/EditLoginInteractorTest.kt index 9200c5d1a..dfc81af9a 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/EditLoginInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/EditLoginInteractorTest.kt @@ -15,10 +15,10 @@ class EditLoginInteractorTest { private val interactor = EditLoginInteractor(loginsController) @Test - fun findPotentialDupesTest() { + fun findDuplicateTest() { val id = "anyId" - interactor.findPotentialDuplicates(id) - verify { loginsController.findPotentialDuplicates(id) } + interactor.findDuplicate(id, "username", "password") + verify { loginsController.findDuplicateForSave(id, "username", "password") } } @Test diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginDetailViewTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginDetailViewTest.kt index dad10740b..1d7b5669e 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginDetailViewTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginDetailViewTest.kt @@ -31,7 +31,7 @@ class LoginDetailViewTest { searchedForText = null, sortingStrategy = SortingStrategy.LastUsed, highlightedItem = SavedLoginsSortingStrategyMenu.Item.LastUsedSort, - duplicateLogins = listOf() + duplicateLogin = null, ) private lateinit var view: ViewGroup diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt index 99156f390..4b64a4433 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt @@ -33,7 +33,7 @@ class LoginsFragmentStoreTest { searchedForText = null, sortingStrategy = SortingStrategy.LastUsed, highlightedItem = SavedLoginsSortingStrategyMenu.Item.LastUsedSort, - duplicateLogins = listOf() + duplicateLogin = null, ) @Test @@ -50,7 +50,7 @@ class LoginsFragmentStoreTest { searchedForText = null, sortingStrategy = SortingStrategy.LastUsed, highlightedItem = SavedLoginsSortingStrategyMenu.Item.LastUsedSort, - duplicateLogins = emptyList() + duplicateLogin = null, ), createInitialLoginsListState(settings) ) diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt index aa46116e5..770f89435 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt @@ -6,17 +6,18 @@ package org.mozilla.fenix.settings.logins import androidx.navigation.NavController import androidx.navigation.NavDestination -import io.mockk.Runs import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every -import io.mockk.just import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestCoroutineDispatcher import kotlinx.coroutines.test.TestCoroutineScope import kotlinx.coroutines.test.runBlockingTest +import mozilla.components.concept.storage.EncryptedLogin import mozilla.components.concept.storage.Login +import mozilla.components.concept.storage.LoginEntry +import mozilla.components.service.sync.logins.InvalidRecordException import mozilla.components.service.sync.logins.SyncableLoginsStorage import mozilla.components.support.test.rule.MainCoroutineRule import org.junit.After @@ -93,7 +94,7 @@ class SavedLoginsStorageControllerTest { ) coEvery { passwordsStorage.list() } returns listOf(login) - controller.fetchLoginDetails(login.guid!!) + controller.fetchLoginDetails(login.guid) val expectedLogin = login.mapToSavedLogin() @@ -117,17 +118,13 @@ class SavedLoginsStorageControllerTest { httpRealm = "httpRealm", formActionOrigin = "" ) - - coEvery { passwordsStorage.get(any()) } returns oldLogin - coEvery { passwordsStorage.update(any()) } just Runs - - controller.save(oldLogin.guid!!, "newUsername", "newPassword") - - val directions = - EditLoginFragmentDirections.actionEditLoginFragmentToLoginDetailFragment( - oldLogin.guid!! - ) - + val oldLoginEncrypted = EncryptedLogin( + guid = "id", + origin = "https://www.test.co.gov.org", + httpRealm = "httpRealm", + formActionOrigin = "", + secFields = "fake-encrypted-data", + ) val newLogin = Login( guid = "id", origin = "https://www.test.co.gov.org", @@ -137,11 +134,22 @@ class SavedLoginsStorageControllerTest { formActionOrigin = "" ) + coEvery { passwordsStorage.get(any()) } returns oldLogin + coEvery { passwordsStorage.update(any(), any()) } returns oldLoginEncrypted + coEvery { passwordsStorage.decryptLogin(any()) } returns newLogin + + controller.save(oldLogin.guid, "newUsername", "newPassword") + + val directions = + EditLoginFragmentDirections.actionEditLoginFragmentToLoginDetailFragment( + oldLogin.guid + ) + val expectedNewList = listOf(newLogin.mapToSavedLogin()) coVerify { - passwordsStorage.get(oldLogin.guid!!) - passwordsStorage.update(newLogin) + passwordsStorage.get(oldLogin.guid) + passwordsStorage.update(newLogin.guid, newLogin.toEntry()) loginsFragmentStore.dispatch( LoginsAction.UpdateLoginsList( expectedNewList @@ -152,14 +160,14 @@ class SavedLoginsStorageControllerTest { } @Test - fun `WHEN finding login dupes, THEN update duplicates in the store`() = scope.runBlockingTest { + fun `WHEN login dupe is found for save, THEN update duplicate in the store`() = scope.runBlockingTest { val login = Login( guid = "id", origin = "https://www.test.co.gov.org", username = "user123", password = "securePassword1", httpRealm = "httpRealm", - formActionOrigin = "" + formActionOrigin = null, ) val login2 = Login( @@ -168,28 +176,167 @@ class SavedLoginsStorageControllerTest { username = "user1234", password = "securePassword1", httpRealm = "httpRealm", - formActionOrigin = "" + formActionOrigin = null, ) coEvery { passwordsStorage.get(any()) } returns login + coEvery { + passwordsStorage.findLoginToUpdate(any()) + } returns login2 - val dupeList = listOf(login2) + // Simulate calling findDuplicateForSave after the user set the username field to the login2's username + controller.findDuplicateForSave(login.guid, login2.username, login.password) + coVerify { + passwordsStorage.get(login.guid) + passwordsStorage.findLoginToUpdate(LoginEntry( + origin = login.origin, + httpRealm = login.httpRealm, + formActionOrigin = login.formActionOrigin, + username = login2.username, + password = login.password + )) + loginsFragmentStore.dispatch( + LoginsAction.DuplicateLogin(login2.mapToSavedLogin()) + ) + } + } + + @Test + fun `WHEN login dupe is not found for save, THEN update duplicate in the store`() = scope.runBlockingTest { + val login = Login( + guid = "id", + origin = "https://www.test.co.gov.org", + username = "user123", + password = "securePassword1", + httpRealm = "httpRealm", + formActionOrigin = null, + ) + + coEvery { passwordsStorage.get(any()) } returns login coEvery { - passwordsStorage.getPotentialDupesIgnoringUsername(any()) - } returns dupeList + passwordsStorage.findLoginToUpdate(any()) + } returns null - controller.findPotentialDuplicates(login.guid!!) + // Simulate calling findDuplicateForSave after the user set the username field to a new value + controller.findDuplicateForSave(login.guid, "new-username", login.password) - val expectedDupeList = dupeList.map { it.mapToSavedLogin() } + coVerify { + passwordsStorage.get(login.guid) + passwordsStorage.findLoginToUpdate(LoginEntry( + origin = login.origin, + httpRealm = login.httpRealm, + formActionOrigin = login.formActionOrigin, + username = "new-username", + password = login.password + )) + loginsFragmentStore.dispatch( + LoginsAction.DuplicateLogin(null) + ) + } + } + + @Test + fun `WHEN login dupe is found for add, THEN update duplicate in the store`() = scope.runBlockingTest { + val login = Login( + guid = "id", + origin = "https://www.test.co.gov.org", + username = "user1234", + password = "securePassword1", + httpRealm = "httpRealm", + formActionOrigin = null, + ) + + coEvery { + passwordsStorage.findLoginToUpdate(any()) + } returns login + + // Simulate calling findDuplicateForAdd after the user set the origin/username fields to match login + controller.findDuplicateForAdd(login.origin, login.username, "new-password") coVerify { - passwordsStorage.getPotentialDupesIgnoringUsername(login) + passwordsStorage.findLoginToUpdate(LoginEntry( + origin = login.origin, + httpRealm = login.origin, + formActionOrigin = null, + username = login.username, + password = "new-password", + )) loginsFragmentStore.dispatch( - LoginsAction.ListOfDupes( - dupeList = expectedDupeList - ) + LoginsAction.DuplicateLogin(login.mapToSavedLogin()) + ) + } + } + + @Test + fun `WHEN login dupe is not found for add, THEN update duplicate in the store`() = scope.runBlockingTest { + coEvery { + passwordsStorage.findLoginToUpdate(any()) + } returns null + + // Simulate calling findDuplicateForAdd after the user set the origin/username field to new values + val origin = "https://new-origin.example.com" + controller.findDuplicateForAdd(origin, "username", "password") + + coVerify { + passwordsStorage.findLoginToUpdate(LoginEntry( + origin = origin, + httpRealm = origin, + formActionOrigin = null, + username = "username", + password = "password", + )) + loginsFragmentStore.dispatch( + LoginsAction.DuplicateLogin(null) ) } } + + @Test + fun `WHEN findLoginToUpdate throws THEN update duplicate in the store`() = scope.runBlockingTest { + coEvery { + passwordsStorage.findLoginToUpdate(any()) + } throws InvalidRecordException("InvalidOrigin") + + // Simulate calling findDuplicateForAdd with an invalid origin + val origin = "https://" + controller.findDuplicateForAdd(origin, "username", "password") + + coVerify { + passwordsStorage.findLoginToUpdate(LoginEntry( + origin = origin, + httpRealm = origin, + formActionOrigin = null, + username = "username", + password = "password", + )) + loginsFragmentStore.dispatch( + LoginsAction.DuplicateLogin(null) + ) + } + } + + @Test + fun `WHEN dupe checking THEN always use a non-blank password`() = scope.runBlockingTest { + // If the user hasn't entered a password yet, we should use a dummy + // password to send a valid login entry to findLoginToUpdate() + + coEvery { + passwordsStorage.findLoginToUpdate(any()) + } throws InvalidRecordException("InvalidOrigin") + + // Simulate calling findDuplicateForAdd with an invalid origin + val origin = "https://example.com/" + controller.findDuplicateForAdd(origin, "username", "") + + coVerify { + passwordsStorage.findLoginToUpdate(LoginEntry( + origin = origin, + httpRealm = origin, + formActionOrigin = null, + username = "username", + password = "password", + )) + } + } }