Skip to content

Commit

Permalink
Make autofill username comparison case insensitive
Browse files Browse the repository at this point in the history
  • Loading branch information
CDRussell committed Feb 26, 2025
1 parent e1692af commit 256c04e
Show file tree
Hide file tree
Showing 18 changed files with 344 additions and 61 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<PasswordStoreEventListener>,
) : InternalAutofillStore, AutofillDeclineStore {

Expand Down Expand Up @@ -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)
Expand All @@ -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!!)
}
Expand All @@ -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()
}

Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -35,7 +34,6 @@ interface AutofillDeduplicationBestMatchFinder {

@ContributesBinding(AppScope::class)
class RealAutofillDeduplicationBestMatchFinder @Inject constructor(
private val urlMatcher: AutofillUrlMatcher,
private val matchTypeDetector: AutofillDeduplicationMatchTypeDetector,
) : AutofillDeduplicationBestMatchFinder {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<LoginCredentials>): Map<Pair<String?, String?>, List<LoginCredentials>>
suspend fun groupDuplicateCredentials(logins: List<LoginCredentials>): Map<Pair<String?, String?>, List<LoginCredentials>>
}

@ContributesBinding(AppScope::class)
class RealAutofillDeduplicationUsernameAndPasswordMatcher @Inject constructor() : AutofillDeduplicationUsernameAndPasswordMatcher {
class RealAutofillDeduplicationUsernameAndPasswordMatcher @Inject constructor(
private val usernameComparer: AutofillUsernameComparer,
) : AutofillDeduplicationUsernameAndPasswordMatcher {

override fun groupDuplicateCredentials(logins: List<LoginCredentials>): Map<Pair<String?, String?>, List<LoginCredentials>> {
return logins.groupBy { it.username to it.password }
override suspend fun groupDuplicateCredentials(logins: List<LoginCredentials>): Map<Pair<String?, String?>, List<LoginCredentials>> {
return usernameComparer.groupUsernamesAndPasswords(logins)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import javax.inject.Inject

interface AutofillLoginDeduplicator {

fun deduplicate(
suspend fun deduplicate(
originalUrl: String,
logins: List<LoginCredentials>,
): List<LoginCredentials>
Expand All @@ -35,7 +35,7 @@ class RealAutofillLoginDeduplicator @Inject constructor(
private val bestMatchFinder: AutofillDeduplicationBestMatchFinder,
) : AutofillLoginDeduplicator {

override fun deduplicate(
override suspend fun deduplicate(
originalUrl: String,
logins: List<LoginCredentials>,
): List<LoginCredentials> {
Expand Down
Original file line number Diff line number Diff line change
@@ -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<LoginCredentials>): Map<Pair<String?, String?>, List<LoginCredentials>>
}

@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<LoginCredentials>): Map<Pair<String?, String?>, List<LoginCredentials>> {
return if (autofillFeature.ignoreCaseOnUsernameComparisons().isEnabled()) {
logins.groupBy { it.username?.lowercase() to it.password }
} else {
logins.groupBy { it.username to it.password }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -56,6 +57,7 @@ fun fakeAutofillStore(
coroutineScope,
),
autofillFeature = autofillFeature,
usernameComparer = mock(),
)

fun fakeStorage(
Expand All @@ -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<LoginCredentials>,
): List<LoginCredentials> = logins
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class FakeSecureStore(
private val urlMatcher: AutofillUrlMatcher,
) : SecureStorage {

private val credentials = mutableListOf<WebsiteLoginDetailsWithCredentials>()
private val credentials = mutableMapOf<Long, WebsiteLoginDetailsWithCredentials>()

override suspend fun addWebsiteLoginDetailsWithCredentials(
websiteLoginDetailsWithCredentials: WebsiteLoginDetailsWithCredentials,
Expand All @@ -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
}

Expand All @@ -57,12 +57,12 @@ class FakeSecureStore(

override suspend fun websiteLoginDetails(): Flow<List<WebsiteLoginDetails>> {
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<List<WebsiteLoginDetailsWithCredentials>> {
Expand All @@ -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)
Expand All @@ -83,25 +83,26 @@ class FakeSecureStore(

override suspend fun websiteLoginDetailsWithCredentials(): Flow<List<WebsiteLoginDetailsWithCredentials>> {
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<Long>) {
credentials.removeAll { ids.contains(it.details.id) }
ids.forEach {
credentials.remove(it)
}
}

override suspend fun addToNeverSaveList(domain: String) {
Expand Down
Loading

0 comments on commit 256c04e

Please sign in to comment.