diff --git a/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillCredentialDialogs.kt b/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillCredentialDialogs.kt index 7e58d3eb9036..3c180fa88838 100644 --- a/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillCredentialDialogs.kt +++ b/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillCredentialDialogs.kt @@ -88,10 +88,10 @@ interface CredentialUpdateExistingCredentialsDialog { sealed interface CredentialUpdateType : Parcelable { @Parcelize - object Username : CredentialUpdateType + data object Username : CredentialUpdateType @Parcelize - object Password : CredentialUpdateType + data object Password : CredentialUpdateType } companion object { diff --git a/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillFeature.kt b/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillFeature.kt index 2ea28c74cb4d..a2153df0228b 100644 --- a/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillFeature.kt +++ b/autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/AutofillFeature.kt @@ -128,4 +128,10 @@ interface AutofillFeature { */ @Toggle.DefaultValue(defaultValue = true) fun newScrollBehaviourInPasswordManagementScreen(): Toggle + + /** + * Kill switch for making case insensitive checks on existing username matches + */ + @Toggle.DefaultValue(defaultValue = true) + fun ignoreCaseOnUsernameComparisons(): Toggle } diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStore.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStore.kt index 83f5b71fe814..c0c0e8bcbdc5 100644 --- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStore.kt +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStore.kt @@ -31,6 +31,7 @@ import com.duckduckgo.autofill.impl.securestorage.WebsiteLoginDetailsWithCredent import com.duckduckgo.autofill.impl.store.InternalAutofillStore import com.duckduckgo.autofill.impl.ui.credential.saving.declines.AutofillDeclineStore import com.duckduckgo.autofill.impl.urlmatcher.AutofillUrlMatcher +import com.duckduckgo.autofill.impl.username.AutofillUsernameComparer import com.duckduckgo.autofill.store.AutofillPrefsStore import com.duckduckgo.autofill.store.LastUpdatedTimeProvider import com.duckduckgo.autofill.sync.SyncCredentialsListener @@ -58,6 +59,7 @@ class SecureStoreBackedAutofillStore @Inject constructor( private val autofillUrlMatcher: AutofillUrlMatcher, private val syncCredentialsListener: SyncCredentialsListener, private val autofillFeature: AutofillFeature, + private val usernameComparer: AutofillUsernameComparer, passwordStoreEventListenersPlugins: PluginPoint, ) : InternalAutofillStore, AutofillDeclineStore { @@ -171,7 +173,7 @@ class SecureStoreBackedAutofillStore @Inject constructor( val matchingCredentials = secureStorage.websiteLoginDetailsWithCredentialsForDomain(url) .firstOrNull() - ?.filter(filter) + ?.filter { filter(it) } if (matchingCredentials.isNullOrEmpty()) { Timber.w("Cannot update credentials as no credentials were found for %s", url) @@ -182,9 +184,19 @@ class SecureStoreBackedAutofillStore @Inject constructor( var updatedCredentials: WebsiteLoginDetailsWithCredentials? = null - matchingCredentials.forEach { - val modifiedDetails = it.details.copy(username = credentials.username, lastUpdatedMillis = lastUpdatedTimeProvider.getInMillis()) - val modified = it.copy(password = credentials.password, details = modifiedDetails) + matchingCredentials.forEach { existingCredential -> + val modifiedDetails = existingCredential.details.copy( + // only update username if that was the update type + username = if (updateType == Username) credentials.username else existingCredential.details.username, + lastUpdatedMillis = lastUpdatedTimeProvider.getInMillis(), + ) + + val modified = existingCredential.copy( + // only update password if that was the update type + password = if (updateType == Password) credentials.password else existingCredential.password, + details = modifiedDetails, + ) + updatedCredentials = secureStorage.updateWebsiteLoginDetailsWithCredentials(modified)?.also { syncCredentialsListener.onCredentialUpdated(it.details.id!!) } @@ -193,10 +205,13 @@ class SecureStoreBackedAutofillStore @Inject constructor( return updatedCredentials?.toLoginCredentials() } - private fun filterMatchingUsername(credentials: LoginCredentials): (WebsiteLoginDetailsWithCredentials) -> Boolean = - { it.details.username == credentials.username } + private fun filterMatchingUsername(credentials: LoginCredentials): suspend (WebsiteLoginDetailsWithCredentials) -> Boolean = { + // we only update password when usernames are equal + usernameComparer.isEqual(it.details.username, credentials.username) + } - private fun filterMatchingPassword(credentials: LoginCredentials): (WebsiteLoginDetailsWithCredentials) -> Boolean = { + private fun filterMatchingPassword(credentials: LoginCredentials): suspend (WebsiteLoginDetailsWithCredentials) -> Boolean = { + // we only update username if stored username is null or empty, and only when password matches it.password == credentials.password && it.details.username.isNullOrEmpty() } @@ -340,11 +355,12 @@ class SecureStoreBackedAutofillStore @Inject constructor( } } - private fun usernameMatch( + private suspend fun usernameMatch( credentials: WebsiteLoginDetailsWithCredentials, username: String?, ): Boolean { - return credentials.details.username != null && credentials.details.username == username + // special case where we don't want to consider two null usernames as equals + return credentials.details.username != null && usernameComparer.isEqual(credentials.details.username, username) } private fun usernameMissing( diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationBestMatchFinder.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationBestMatchFinder.kt index b81f37a30948..99b158d219df 100644 --- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationBestMatchFinder.kt +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationBestMatchFinder.kt @@ -20,7 +20,6 @@ import com.duckduckgo.autofill.api.domain.app.LoginCredentials import com.duckduckgo.autofill.impl.deduper.AutofillDeduplicationMatchTypeDetector.MatchType.NotAMatch import com.duckduckgo.autofill.impl.deduper.AutofillDeduplicationMatchTypeDetector.MatchType.PartialMatch import com.duckduckgo.autofill.impl.deduper.AutofillDeduplicationMatchTypeDetector.MatchType.PerfectMatch -import com.duckduckgo.autofill.impl.urlmatcher.AutofillUrlMatcher import com.duckduckgo.di.scopes.AppScope import com.squareup.anvil.annotations.ContributesBinding import javax.inject.Inject @@ -35,7 +34,6 @@ interface AutofillDeduplicationBestMatchFinder { @ContributesBinding(AppScope::class) class RealAutofillDeduplicationBestMatchFinder @Inject constructor( - private val urlMatcher: AutofillUrlMatcher, private val matchTypeDetector: AutofillDeduplicationMatchTypeDetector, ) : AutofillDeduplicationBestMatchFinder { diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationUsernameAndPasswordMatcher.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationUsernameAndPasswordMatcher.kt index 35de6a2e1cf6..13f9d90188a4 100644 --- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationUsernameAndPasswordMatcher.kt +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillDeduplicationUsernameAndPasswordMatcher.kt @@ -17,19 +17,22 @@ package com.duckduckgo.autofill.impl.deduper import com.duckduckgo.autofill.api.domain.app.LoginCredentials +import com.duckduckgo.autofill.impl.username.AutofillUsernameComparer import com.duckduckgo.di.scopes.AppScope import com.squareup.anvil.annotations.ContributesBinding import javax.inject.Inject interface AutofillDeduplicationUsernameAndPasswordMatcher { - fun groupDuplicateCredentials(logins: List): Map, List> + suspend fun groupDuplicateCredentials(logins: List): Map, List> } @ContributesBinding(AppScope::class) -class RealAutofillDeduplicationUsernameAndPasswordMatcher @Inject constructor() : AutofillDeduplicationUsernameAndPasswordMatcher { +class RealAutofillDeduplicationUsernameAndPasswordMatcher @Inject constructor( + private val usernameComparer: AutofillUsernameComparer, +) : AutofillDeduplicationUsernameAndPasswordMatcher { - override fun groupDuplicateCredentials(logins: List): Map, List> { - return logins.groupBy { it.username to it.password } + override suspend fun groupDuplicateCredentials(logins: List): Map, List> { + return usernameComparer.groupUsernamesAndPasswords(logins) } } diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillLoginDeduplicator.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillLoginDeduplicator.kt index b47d1abc296d..87d90f1c1530 100644 --- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillLoginDeduplicator.kt +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/deduper/AutofillLoginDeduplicator.kt @@ -23,7 +23,7 @@ import javax.inject.Inject interface AutofillLoginDeduplicator { - fun deduplicate( + suspend fun deduplicate( originalUrl: String, logins: List, ): List @@ -35,7 +35,7 @@ class RealAutofillLoginDeduplicator @Inject constructor( private val bestMatchFinder: AutofillDeduplicationBestMatchFinder, ) : AutofillLoginDeduplicator { - override fun deduplicate( + override suspend fun deduplicate( originalUrl: String, logins: List, ): List { diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/username/AutofillUsernameComparer.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/username/AutofillUsernameComparer.kt new file mode 100644 index 000000000000..9fc37c97c6f0 --- /dev/null +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/username/AutofillUsernameComparer.kt @@ -0,0 +1,58 @@ +/* + * Copyright (c) 2025 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.autofill.impl.username + +import com.duckduckgo.autofill.api.AutofillFeature +import com.duckduckgo.autofill.api.domain.app.LoginCredentials +import com.duckduckgo.common.utils.DispatcherProvider +import com.duckduckgo.di.scopes.AppScope +import com.squareup.anvil.annotations.ContributesBinding +import javax.inject.Inject +import kotlinx.coroutines.withContext + +interface AutofillUsernameComparer { + suspend fun isEqual( + username1: String?, + username2: String?, + ): Boolean + + suspend fun groupUsernamesAndPasswords(logins: List): Map, List> +} + +@ContributesBinding(AppScope::class) +class RealAutofillUsernameComparer @Inject constructor( + private val autofillFeature: AutofillFeature, + private val dispatchers: DispatcherProvider, +) : AutofillUsernameComparer { + + override suspend fun isEqual(username1: String?, username2: String?): Boolean { + return withContext(dispatchers.io()) { + if (username1 == null && username2 == null) return@withContext true + if (username1 == null) return@withContext false + if (username2 == null) return@withContext false + + username1.equals(username2, ignoreCase = autofillFeature.ignoreCaseOnUsernameComparisons().isEnabled()) + } + } + override suspend fun groupUsernamesAndPasswords(logins: List): Map, List> { + return if (autofillFeature.ignoreCaseOnUsernameComparisons().isEnabled()) { + logins.groupBy { it.username?.lowercase() to it.password } + } else { + logins.groupBy { it.username to it.password } + } + } +} diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/AutofillStoreFixtures.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/AutofillStoreFixtures.kt index cd5d62fb0eea..16fa92bb3f3f 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/AutofillStoreFixtures.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/AutofillStoreFixtures.kt @@ -34,6 +34,7 @@ import com.duckduckgo.autofill.sync.SyncCredentialsListener import com.duckduckgo.autofill.sync.inMemoryAutofillDatabase import com.duckduckgo.common.utils.DispatcherProvider import kotlinx.coroutines.CoroutineScope +import org.mockito.kotlin.mock fun fakeAutofillStore( secureStorage: SecureStorage = fakeStorage(), @@ -56,6 +57,7 @@ fun fakeAutofillStore( coroutineScope, ), autofillFeature = autofillFeature, + usernameComparer = mock(), ) fun fakeStorage( @@ -73,7 +75,7 @@ fun lastUpdatedTimeProvider() = object : LastUpdatedTimeProvider { fun autofillDomainNameUrlMatcher() = AutofillDomainNameUrlMatcher(TestUrlUnicodeNormalizer()) fun noopDeduplicator() = object : AutofillLoginDeduplicator { - override fun deduplicate( + override suspend fun deduplicate( originalUrl: String, logins: List, ): List = logins diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/FakeSecureStorage.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/FakeSecureStorage.kt index e3f3d260ce71..7fbdb85a3a8d 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/FakeSecureStorage.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/FakeSecureStorage.kt @@ -29,7 +29,7 @@ class FakeSecureStore( private val urlMatcher: AutofillUrlMatcher, ) : SecureStorage { - private val credentials = mutableListOf() + private val credentials = mutableMapOf() override suspend fun addWebsiteLoginDetailsWithCredentials( websiteLoginDetailsWithCredentials: WebsiteLoginDetailsWithCredentials, @@ -38,7 +38,7 @@ class FakeSecureStore( val credentialWithId: WebsiteLoginDetailsWithCredentials = websiteLoginDetailsWithCredentials.copy( details = websiteLoginDetailsWithCredentials.details.copy(id = id), ) - credentials.add(credentialWithId) + credentials[id] = credentialWithId return credentialWithId } @@ -57,12 +57,12 @@ class FakeSecureStore( override suspend fun websiteLoginDetails(): Flow> { return flow { - emit(credentials.map { it.details }) + emit(credentials.values.map { it.details }) } } override suspend fun getWebsiteLoginDetailsWithCredentials(id: Long): WebsiteLoginDetailsWithCredentials? { - return credentials.firstOrNull() { it.details.id == id } + return credentials[id] } override suspend fun websiteLoginDetailsWithCredentialsForDomain(domain: String): Flow> { @@ -73,7 +73,7 @@ class FakeSecureStore( } } - private fun domainLookup(domain: String) = credentials + private fun domainLookup(domain: String) = credentials.values .filter { it.details.domain?.contains(domain) == true } .filter { val visitedSite = urlMatcher.extractUrlPartsForAutofill(domain) @@ -83,25 +83,26 @@ class FakeSecureStore( override suspend fun websiteLoginDetailsWithCredentials(): Flow> { return flow { - emit(credentials) + emit(credentials.values.toList()) } } override suspend fun updateWebsiteLoginDetailsWithCredentials( websiteLoginDetailsWithCredentials: WebsiteLoginDetailsWithCredentials, ): WebsiteLoginDetailsWithCredentials { - credentials.indexOfFirst { it.details.id == websiteLoginDetailsWithCredentials.details.id }.also { - credentials[it] = websiteLoginDetailsWithCredentials - } + val id = websiteLoginDetailsWithCredentials.details.id ?: return websiteLoginDetailsWithCredentials + credentials[id] = websiteLoginDetailsWithCredentials return websiteLoginDetailsWithCredentials } override suspend fun deleteWebsiteLoginDetailsWithCredentials(id: Long) { - credentials.removeAll { it.details.id == id } + credentials.remove(id) } override suspend fun deleteWebSiteLoginDetailsWithCredentials(ids: List) { - credentials.removeAll { ids.contains(it.details.id) } + ids.forEach { + credentials.remove(it) + } } override suspend fun addToNeverSaveList(domain: String) { diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStoreTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStoreTest.kt index ccbb77181686..f9fe59671cfa 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStoreTest.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/SecureStoreBackedAutofillStoreTest.kt @@ -22,6 +22,7 @@ import com.duckduckgo.autofill.FakeSecureStore import com.duckduckgo.autofill.api.AutofillFeature import com.duckduckgo.autofill.api.CredentialUpdateExistingCredentialsDialog.CredentialUpdateType import com.duckduckgo.autofill.api.CredentialUpdateExistingCredentialsDialog.CredentialUpdateType.Password +import com.duckduckgo.autofill.api.CredentialUpdateExistingCredentialsDialog.CredentialUpdateType.Username import com.duckduckgo.autofill.api.ExistingCredentialMatchDetector.ContainsCredentialsResult import com.duckduckgo.autofill.api.ExistingCredentialMatchDetector.ContainsCredentialsResult.ExactMatch import com.duckduckgo.autofill.api.ExistingCredentialMatchDetector.ContainsCredentialsResult.NoMatch @@ -35,6 +36,7 @@ import com.duckduckgo.autofill.impl.securestorage.WebsiteLoginDetails import com.duckduckgo.autofill.impl.securestorage.WebsiteLoginDetailsWithCredentials import com.duckduckgo.autofill.impl.urlmatcher.AutofillDomainNameUrlMatcher import com.duckduckgo.autofill.impl.urlmatcher.AutofillUrlMatcher +import com.duckduckgo.autofill.impl.username.RealAutofillUsernameComparer import com.duckduckgo.autofill.store.AutofillPrefsStore import com.duckduckgo.autofill.store.LastUpdatedTimeProvider import com.duckduckgo.autofill.sync.CredentialsFixtures.toLoginCredentials @@ -78,6 +80,7 @@ class SecureStoreBackedAutofillStoreTest { private lateinit var secureStore: FakeSecureStore private val autofillUrlMatcher: AutofillUrlMatcher = AutofillDomainNameUrlMatcher(TestUrlUnicodeNormalizer()) + private val usernameComparer = RealAutofillUsernameComparer(autofillFeature, coroutineTestRule.testDispatcherProvider) @Before fun setUp() { @@ -412,6 +415,67 @@ class SecureStoreBackedAutofillStoreTest { } } + @Test + fun whenUpdatingPasswordWithUsernameInDifferentCasingThenOriginalUsernameUntouched() = runTest { + setupTesteeWithAutofillAvailable() + val url = "example.com" + storeCredentials(1, url, "username", "password123") + + val incomingCredentials = LoginCredentials( + domain = url, + username = "USERNAME", + password = "newpassword", + id = 1, + ) + + testee.updateCredentials(url, incomingCredentials, updateType = Password) + + testee.getCredentials(url).run { + this.assertHasLoginCredentials(url, "username", "newpassword", UPDATED_INITIAL_LAST_UPDATED) + } + } + + @Test + fun whenUpdateUsernameCalledWithMatchingPasswordAndOriginalAlreadyHasPopulatedUsernameThenIsNotUpdated() = runTest { + setupTesteeWithAutofillAvailable() + val url = "example.com" + storeCredentials(1, url, "originalUsername", "originalPassword") + + val incomingCredentials = LoginCredentials( + domain = url, + username = "username", + password = "newpassword", + id = 1, + ) + + testee.updateCredentials(url, incomingCredentials, updateType = Username) + + testee.getCredentials(url).run { + this.assertHasLoginCredentials(url, "originalUsername", "originalPassword") + } + } + + @Test + fun whenUpdateUsernameCalledWithMatchingPasswordAndOriginalHasEmptyUsernameThenIsUpdated() = runTest { + setupTesteeWithAutofillAvailable() + val url = "example.com" + storeCredentials(1, url, "", "originalPassword") + + val incomingCredentials = LoginCredentials( + domain = url, + username = "newUsername", + password = "originalPassword", + id = 1, + ) + + val updated = testee.updateCredentials(url, incomingCredentials, updateType = Username) + assertNotNull(updated) + + testee.getCredentials(url).run { + this.assertHasLoginCredentials(url, "newUsername", "originalPassword", UPDATED_INITIAL_LAST_UPDATED) + } + } + @Test fun whenDomainIsUpdatedTheCleanRawUrl() = runTest { setupTesteeWithAutofillAvailable() @@ -690,6 +754,7 @@ class SecureStoreBackedAutofillStoreTest { coroutineTestRule.testScope, ), autofillFeature = autofillFeature, + usernameComparer = usernameComparer, ) } diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealAutofillLoginDeduplicatorTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealAutofillLoginDeduplicatorTest.kt index 2e921f478ade..e2f4ce1f97ac 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealAutofillLoginDeduplicatorTest.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealAutofillLoginDeduplicatorTest.kt @@ -1,22 +1,32 @@ package com.duckduckgo.autofill.impl.deduper import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.duckduckgo.autofill.api.AutofillFeature import com.duckduckgo.autofill.api.domain.app.LoginCredentials import com.duckduckgo.autofill.impl.encoding.TestUrlUnicodeNormalizer import com.duckduckgo.autofill.impl.urlmatcher.AutofillDomainNameUrlMatcher +import com.duckduckgo.autofill.impl.username.RealAutofillUsernameComparer +import com.duckduckgo.common.test.CoroutineTestRule +import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory import kotlinx.coroutines.test.runTest import org.junit.Assert.* +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @RunWith(AndroidJUnit4::class) class RealAutofillLoginDeduplicatorTest { + + @get:Rule + val coroutineTestRule: CoroutineTestRule = CoroutineTestRule() + private val urlMatcher = AutofillDomainNameUrlMatcher(TestUrlUnicodeNormalizer()) private val matchTypeDetector = RealAutofillDeduplicationMatchTypeDetector(urlMatcher) + private val autofillFeature = FakeFeatureToggleFactory.create(AutofillFeature::class.java) + private val usernameComparer = RealAutofillUsernameComparer(autofillFeature, coroutineTestRule.testDispatcherProvider) private val testee = RealAutofillLoginDeduplicator( - usernamePasswordMatcher = RealAutofillDeduplicationUsernameAndPasswordMatcher(), + usernamePasswordMatcher = RealAutofillDeduplicationUsernameAndPasswordMatcher(usernameComparer), bestMatchFinder = RealAutofillDeduplicationBestMatchFinder( - urlMatcher = urlMatcher, matchTypeDetector = matchTypeDetector, ), ) @@ -28,7 +38,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenSingleEntryInThenSingleEntryReturned() { + fun whenSingleEntryInThenSingleEntryReturned() = runTest { val inputList = listOf( aLogin("domain", "username", "password"), ) @@ -37,7 +47,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesCompletelyUnrelatedThenNoDeduplication() { + fun whenEntriesCompletelyUnrelatedThenNoDeduplication() = runTest { val inputList = listOf( aLogin("domain_A", "username_A", "password_A"), aLogin("domain_B", "username_B", "password_B"), @@ -49,7 +59,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareUsernameAndPasswordButNotDomainThenDeduped() { + fun whenEntriesShareUsernameAndPasswordButNotDomainThenDeduped() = runTest { val inputList = listOf( aLogin("foo.com", "username", "password"), aLogin("bar.com", "username", "password"), @@ -59,7 +69,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareDomainAndUsernameButNotPasswordThenNoDeduplication() { + fun whenEntriesShareDomainAndUsernameButNotPasswordThenNoDeduplication() = runTest { val inputList = listOf( aLogin("example.com", "username", "123"), aLogin("example.com", "username", "xyz"), @@ -71,7 +81,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareDomainAndPasswordButNotUsernameThenNoDeduplication() { + fun whenEntriesShareDomainAndPasswordButNotUsernameThenNoDeduplication() = runTest { val inputList = listOf( aLogin("example.com", "user_A", "password"), aLogin("example.com", "user_B", "password"), @@ -83,7 +93,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareMultipleCredentialsWhichArePerfectDomainMatchesThenDeduped() { + fun whenEntriesShareMultipleCredentialsWhichArePerfectDomainMatchesThenDeduped() = runTest { val inputList = listOf( aLogin("example.com", "username", "password"), aLogin("example.com", "username", "password"), @@ -93,7 +103,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareMultipleCredentialsWhichArePartialDomainMatchesThenDeduped() { + fun whenEntriesShareMultipleCredentialsWhichArePartialDomainMatchesThenDeduped() = runTest { val inputList = listOf( aLogin("a.example.com", "username", "password"), aLogin("b.example.com", "username", "password"), @@ -103,7 +113,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareMultipleCredentialsWhichAreNotDomainMatchesThenDeduped() { + fun whenEntriesShareMultipleCredentialsWhichAreNotDomainMatchesThenDeduped() = runTest { val inputList = listOf( aLogin("foo.com", "username", "password"), aLogin("bar.com", "username", "password"), @@ -113,7 +123,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareCredentialsAcrossPerfectAndPartialMatchesThenDedupedToPerfectMatch() { + fun whenEntriesShareCredentialsAcrossPerfectAndPartialMatchesThenDedupedToPerfectMatch() = runTest { val inputList = listOf( aLogin("example.com", "username", "password"), aLogin("a.example.com", "username", "password"), @@ -124,7 +134,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareCredentialsAcrossPerfectAndNonDomainMatchesThenDedupedToPerfectMatch() { + fun whenEntriesShareCredentialsAcrossPerfectAndNonDomainMatchesThenDedupedToPerfectMatch() = runTest { val inputList = listOf( aLogin("example.com", "username", "password"), aLogin("bar.com", "username", "password"), @@ -135,7 +145,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareCredentialsAcrossPartialAndNonDomainMatchesThenDedupedToPerfectMatch() { + fun whenEntriesShareCredentialsAcrossPartialAndNonDomainMatchesThenDedupedToPerfectMatch() = runTest { val inputList = listOf( aLogin("a.example.com", "username", "password"), aLogin("bar.com", "username", "password"), @@ -146,7 +156,7 @@ class RealAutofillLoginDeduplicatorTest { } @Test - fun whenEntriesShareCredentialsAcrossPerfectAndPartialAndNonDomainMatchesThenDedupedToPerfectMatch() { + fun whenEntriesShareCredentialsAcrossPerfectAndPartialAndNonDomainMatchesThenDedupedToPerfectMatch() = runTest { val inputList = listOf( aLogin("a.example.com", "username", "password"), aLogin("example.com", "username", "password"), diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorBestMatchFinderTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorBestMatchFinderTest.kt index 8d7495c1096b..e342e1fc94fa 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorBestMatchFinderTest.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorBestMatchFinderTest.kt @@ -14,7 +14,6 @@ class RealLoginDeduplicatorBestMatchFinderTest { private val urlMatcher = AutofillDomainNameUrlMatcher(TestUrlUnicodeNormalizer()) private val matchTypeDetector = RealAutofillDeduplicationMatchTypeDetector(urlMatcher) private val testee = RealAutofillDeduplicationBestMatchFinder( - urlMatcher = urlMatcher, matchTypeDetector = matchTypeDetector, ) diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorUsernameAndPasswordMatcherTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorUsernameAndPasswordMatcherTest.kt index 08ec6cf0d8f2..f8358a937a01 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorUsernameAndPasswordMatcherTest.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/deduper/RealLoginDeduplicatorUsernameAndPasswordMatcherTest.kt @@ -1,21 +1,36 @@ package com.duckduckgo.autofill.impl.deduper +import android.annotation.SuppressLint +import com.duckduckgo.autofill.api.AutofillFeature import com.duckduckgo.autofill.api.domain.app.LoginCredentials -import org.junit.Assert.* +import com.duckduckgo.autofill.impl.username.RealAutofillUsernameComparer +import com.duckduckgo.common.test.CoroutineTestRule +import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory +import com.duckduckgo.feature.toggles.api.Toggle.State +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Rule import org.junit.Test +@SuppressLint("DenyListedApi") class RealLoginDeduplicatorUsernameAndPasswordMatcherTest { - private val testee = RealAutofillDeduplicationUsernameAndPasswordMatcher() + + @get:Rule + val coroutineTestRule: CoroutineTestRule = CoroutineTestRule() + private val autofillFeature = FakeFeatureToggleFactory.create(AutofillFeature::class.java) + private val usernameComparer = RealAutofillUsernameComparer(autofillFeature, coroutineTestRule.testDispatcherProvider) + private val testee = RealAutofillDeduplicationUsernameAndPasswordMatcher(usernameComparer) @Test - fun whenEmptyListInThenEmptyListOut() { + fun whenEmptyListInThenEmptyListOut() = runTest { val input = emptyList() val output = testee.groupDuplicateCredentials(input) assertTrue(output.isEmpty()) } @Test - fun whenSingleEntryInThenSingleEntryOut() { + fun whenSingleEntryInThenSingleEntryOut() = runTest { val input = listOf( creds("username", "password"), ) @@ -24,7 +39,7 @@ class RealLoginDeduplicatorUsernameAndPasswordMatcherTest { } @Test - fun whenMultipleEntriesWithNoDuplicationAtAllThenNumberOfGroupsReturnedMatchesNumberOfEntriesInputted() { + fun whenMultipleEntriesWithNoDuplicationAtAllThenNumberOfGroupsReturnedMatchesNumberOfEntriesInputted() = runTest { val input = listOf( creds("username_a", "password_x"), creds("username_b", "password_y"), @@ -35,7 +50,7 @@ class RealLoginDeduplicatorUsernameAndPasswordMatcherTest { } @Test - fun whenEntriesMatchOnUsernameButNotPasswordThenNotGrouped() { + fun whenEntriesMatchOnUsernameButNotPasswordThenNotGrouped() = runTest { val input = listOf( creds("username", "password_x"), creds("username", "password_y"), @@ -45,7 +60,7 @@ class RealLoginDeduplicatorUsernameAndPasswordMatcherTest { } @Test - fun whenEntriesMatchOnPasswordButNotUsernameThenNotGrouped() { + fun whenEntriesMatchOnPasswordButNotUsernameThenNotGrouped() = runTest { val input = listOf( creds("username_a", "password"), creds("username_b", "password"), @@ -55,7 +70,7 @@ class RealLoginDeduplicatorUsernameAndPasswordMatcherTest { } @Test - fun whenEntriesMatchOnUsernameAndPasswordThenGrouped() { + fun whenEntriesMatchOnUsernameAndPasswordThenGrouped() = runTest { val input = listOf( creds("username", "password"), creds("username", "password"), @@ -64,6 +79,20 @@ class RealLoginDeduplicatorUsernameAndPasswordMatcherTest { assertEquals(1, output.size) } + @Test + fun whenEntriesMatchOnUsernameWithDifferentCasesAndPasswordThenGrouped() = runTest { + val input = listOf( + creds("username", "password"), + creds("USERNAME", "password"), + ) + + assertEquals(1, testee.groupDuplicateCredentials(input).size) + + // test with feature flag disabled + autofillFeature.ignoreCaseOnUsernameComparisons().setRawStoredState(State(enable = false)) + assertEquals(2, testee.groupDuplicateCredentials(input).size) + } + private fun creds(username: String, password: String): LoginCredentials { return LoginCredentials(username = username, password = password, domain = "domain") } diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/service/AutofillServiceSuggestionsTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/service/AutofillServiceSuggestionsTest.kt index e213ea94d247..7b29a3318210 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/service/AutofillServiceSuggestionsTest.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/service/AutofillServiceSuggestionsTest.kt @@ -139,7 +139,7 @@ class AutofillServiceSuggestionsTest { ), ), loginDeduplicator = object : AutofillLoginDeduplicator { - override fun deduplicate( + override suspend fun deduplicate( originalUrl: String, logins: List, ): List { diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/username/RealAutofillUsernameComparerTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/username/RealAutofillUsernameComparerTest.kt new file mode 100644 index 000000000000..0f3f3cca1418 --- /dev/null +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/username/RealAutofillUsernameComparerTest.kt @@ -0,0 +1,78 @@ +package com.duckduckgo.autofill.impl.username + +import android.annotation.SuppressLint +import com.duckduckgo.autofill.api.AutofillFeature +import com.duckduckgo.common.test.CoroutineTestRule +import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory +import com.duckduckgo.feature.toggles.api.Toggle.State +import kotlinx.coroutines.test.runTest +import org.junit.Assert.* +import org.junit.Rule +import org.junit.Test + +@SuppressLint("DenyListedApi") +class RealAutofillUsernameComparerTest { + + private val feature = FakeFeatureToggleFactory.create(AutofillFeature::class.java) + + @get:Rule + val coroutineTestRule: CoroutineTestRule = CoroutineTestRule() + private val testee = RealAutofillUsernameComparer( + autofillFeature = feature, + dispatchers = coroutineTestRule.testDispatcherProvider, + ) + + @Test + fun whenBothUsernamesAreNullThenAreEqual() = runTest { + assertTrue(testee.isEqual(null, null)) + } + + @Test + fun whenBothUsernamesAreEmptyThenAreEqual() = runTest { + assertTrue(testee.isEqual("", "")) + } + + @Test + fun whenFirstUsernameIsNullAndOtherIsNotThenAreNotEqual() = runTest { + assertFalse(testee.isEqual(null, "user")) + } + + @Test + fun whenFirstUsernameIsEmptyAndOtherIsNotThenAreNotEqual() = runTest { + assertFalse(testee.isEqual("", "user")) + } + + @Test + fun whenSecondUsernameIsNullAndOtherIsNotThenAreNotEqual() = runTest { + assertFalse(testee.isEqual("user", null)) + } + + @Test + fun whenSecondUsernameIsEmptyAndOtherIsNotThenAreNotEqual() = runTest { + assertFalse(testee.isEqual("user", "")) + } + + @Test + fun whenOneUsernameIsEmptyAndOtherIsNotThenAreNotEqual() = runTest { + assertFalse(testee.isEqual("", "user2")) + } + + @Test + fun whenUsernamesAreTotallyDifferentThenAreNotEqual() = runTest { + assertFalse(testee.isEqual("user1", "user2")) + } + + @Test + fun whenUsernamesAreIdenticalIncludingCaseThenAreEqual() = runTest { + assertTrue(testee.isEqual("user", "user")) + } + + @Test + fun whenUsernamesAreIdenticalApartFromCaseThenAreEqual() = runTest { + assertTrue(testee.isEqual("user", "USER")) + + // check when feature flag disabled + feature.ignoreCaseOnUsernameComparisons().setRawStoredState(State(enable = false)) + assertFalse(testee.isEqual("user", "USER")) + } +} diff --git a/autofill/autofill-internal/src/main/java/com/duckduckgo/autofill/internal/AutofillInternalSettingsActivity.kt b/autofill/autofill-internal/src/main/java/com/duckduckgo/autofill/internal/AutofillInternalSettingsActivity.kt index 1c458fe7d1d8..e48f0db05c15 100644 --- a/autofill/autofill-internal/src/main/java/com/duckduckgo/autofill/internal/AutofillInternalSettingsActivity.kt +++ b/autofill/autofill-internal/src/main/java/com/duckduckgo/autofill/internal/AutofillInternalSettingsActivity.kt @@ -444,6 +444,16 @@ class AutofillInternalSettingsActivity : DuckDuckGoActivity() { } } + binding.addMixedCaseUsernameDuplicates.setClickListener { + lifecycleScope.launch(dispatchers.io()) { + val credentials = mutableListOf() + credentials.add(sampleCredentials("https://autofill.me", username = "username")) + credentials.add(sampleCredentials("https://autofill.me", username = "UseRNamE")) + credentials.add(sampleCredentials("https://autofill.me", username = "USERNAME")) + credentials.save() + } + } + binding.clearAllSavedLoginsButton.setClickListener { lifecycleScope.launch(dispatchers.io()) { val count = autofillStore.getCredentialCount().first() diff --git a/autofill/autofill-internal/src/main/res/layout/activity_autofill_internal_settings.xml b/autofill/autofill-internal/src/main/res/layout/activity_autofill_internal_settings.xml index 32f3abc68a67..64b854825d50 100644 --- a/autofill/autofill-internal/src/main/res/layout/activity_autofill_internal_settings.xml +++ b/autofill/autofill-internal/src/main/res/layout/activity_autofill_internal_settings.xml @@ -44,6 +44,12 @@ android:layout_height="wrap_content" app:primaryText="@string/autofillDevSettingsLoginsSectionTitle" /> + + + app:primaryText="@string/autofillDevSettingsAddMixedUsernameCaseLogins" + app:secondaryText="@string/autofillDevSettingsAddMixedUsernameCaseLogins" /> - + app:primaryText="@string/autofillDevSettingsClearLogins" + tools:secondaryText="@string/autofillDevSettingsClearLoginsSubtitle" /> Adds multiple logins for https://fill.dev with same username and password Add duplicate logins (across different subdomains) Adds multiple logins for https://fill.dev with same username and password but across different subdomains + Add mixed case username duplicates + Adds multiple duplicate logins, different only from the case of the username Autofill Survey Previously Seen Surveys