Skip to content

Commit

Permalink
De-duplicate login suggestions offered to a user for autofilling cred…
Browse files Browse the repository at this point in the history
…entials (#3702)

<!--
Note: This checklist is a reminder of our shared engineering
expectations.
The items in Bold are required
If your PR involves UI changes:
1. Upload screenshots or screencasts that illustrate the changes before
/ after
2. Add them under the UI changes section (feel free to add more columns
if needed)
If your PR does not involve UI changes, you can remove the **UI
changes** section

At a minimum, make sure your changes are tested in API 23 and one of the
more recent API levels available.
-->

Task/Issue URL:
https://app.asana.com/0/1202552961248957/1205369440069078/f

### Description
De-duplicates logins which are offered for autofilling into a page. The
de-duplication only happens at the UI layer, and only for the
bottom-sheet autofill prompt (duplicates continue to be supported in the
DB and in the login management screens).

Rules for de-duplicating are described in [Validating business rules for
de-duplicating
logins](https://app.asana.com/0/1205729146579121/1205729146579121/f),
but to summarise:

- only considered a duplicate if both username and password are perfect
matches
- de-deduplication is applied at the last step before presenting the
autofill login options, meaning all of the existing rules on which saved
logins are considered still apply; this just filters that list down if
it contains duplicates.
- when de-duplication happens, it might be that saved logins had
multiple subdomains (e.g., a login saved for `a.example.com` and
`example.com` both had the same username/password). In this case, we
favour the _best match_ and pick that one to show to the user from the
duplicates.


### Steps to test this PR

Install fresh `internal` build.

#### Duplicates on the same E-TLD+1 and subdomain
- [ ] Add a login (`domain=fill.dev`, `username=user`,
`password=password`)
- [ ] Add a login (`domain=fill.dev`, `username=user`,
`password=password`) (a duplicate)
- [ ] Visit https://fill.dev/form/login-simple and verify you only see
one option in the autofill prompt

#### Duplicates on the same E-TLD+1 / different subdomain
- [ ] Edit one of the saved logins to add a subdomain
(`domain=example.fill.dev`, `username=user`, `password=password`)
- [ ] Visit https://fill.dev/form/login-simple and verify you only see
one option in the autofill prompt
- [ ] Verify you do **not** see "From example.fill.dev" (this confirms
it picked the _best_ match out of the dupes)

#### Duplicates across subdomains (same E-TLD+1)
- [ ] Edit the saved login that still has `fill.dev` to now also have a
subdomain (`domain=foo.fill.dev`, `username=user`, `password=password`)
- [ ] Visit https://fill.dev/form/login-simple and verify you only see
one option in the autofill prompt
- [ ] Verify you **do** see the subdomain that is offered. e.g., "From
foo.fill.dev" (it will pick the one that was modified most recently)

#### Mis-matching username means no de-duplication
- [ ] Delete all logins
- [ ] Add a login (`domain=fill.dev`, `username=user`,
`password=password`)
- [ ] Add a login (`domain=fill.dev`, `username=different`,
`password=password`) (different username)
- [ ] Visit https://fill.dev/form/login-simple and verify you see two
options offered (no de-duplication)

### Mis-matching password means no de-duplication
- [ ] Delete all logins
- [ ] Add a login (`domain=fill.dev`, `username=user`,
`password=password`)
- [ ] Add a login (`domain=fill.dev`, `username=user`,
`password=different`) (different password)
- [ ] Visit https://fill.dev/form/login-simple and verify you see two
options offered (no de-duplication)



### UI changes

![combined](https://github.com/duckduckgo/Android/assets/1336281/d54b817f-a084-4e7a-bf32-9e1c75653f28)
  • Loading branch information
CDRussell committed Oct 26, 2023
1 parent e2c7437 commit 68ed6f2
Show file tree
Hide file tree
Showing 12 changed files with 771 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import com.duckduckgo.autofill.api.domain.app.LoginTriggerType
import com.duckduckgo.autofill.api.email.EmailManager
import com.duckduckgo.autofill.api.passwordgeneration.AutomaticSavedLoginsMonitor
import com.duckduckgo.autofill.api.store.AutofillStore
import com.duckduckgo.autofill.impl.deduper.AutofillLoginDeduplicator
import com.duckduckgo.autofill.impl.domain.javascript.JavascriptCredentials
import com.duckduckgo.autofill.impl.email.incontext.availability.EmailProtectionInContextRecentInstallChecker
import com.duckduckgo.autofill.impl.email.incontext.store.EmailProtectionInContextDataStore
Expand Down Expand Up @@ -108,6 +109,7 @@ class AutofillStoredBackJavascriptInterface @Inject constructor(
private val emailManager: EmailManager,
private val inContextDataStore: EmailProtectionInContextDataStore,
private val recentInstallChecker: EmailProtectionInContextRecentInstallChecker,
private val loginDeduplicator: AutofillLoginDeduplicator,
) : AutofillJavascriptInterface {

override var callback: Callback? = null
Expand Down Expand Up @@ -209,10 +211,13 @@ class AutofillStoredBackJavascriptInterface @Inject constructor(

val credentials = filterRequestedSubtypes(request, matches)

if (credentials.isEmpty()) {
val dedupedCredentials = loginDeduplicator.deduplicate(url, credentials)
Timber.v("Original autofill credentials list size: %d, after de-duping: %d", credentials.size, dedupedCredentials.size)

if (dedupedCredentials.isEmpty()) {
callback?.noCredentialsAvailable(url)
} else {
callback?.onCredentialsAvailableToInject(url, credentials, triggerType)
callback?.onCredentialsAvailableToInject(url, dedupedCredentials, triggerType)
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* Copyright (c) 2023 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.deduper

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

interface AutofillDeduplicationBestMatchFinder {

fun findBestMatch(
originalUrl: String,
logins: List<LoginCredentials>,
): LoginCredentials?
}

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

override fun findBestMatch(
originalUrl: String,
logins: List<LoginCredentials>,
): LoginCredentials? {
// perfect matches are those where the subdomain and e-tld+1 match
val perfectMatches = mutableListOf<LoginCredentials>()

// partial matches are those where only e-tld+1 matches
val partialMatches = mutableListOf<LoginCredentials>()

// non-matches are those where neither subdomain nor e-tld+1 match
val nonMatches = mutableListOf<LoginCredentials>()

categoriseEachLogin(logins, originalUrl, perfectMatches, partialMatches, nonMatches)

if (perfectMatches.isEmpty() && partialMatches.isEmpty() && nonMatches.isEmpty()) {
return null
}

return if (perfectMatches.isNotEmpty()) {
bestPerfectMatch(perfectMatches)
} else if (partialMatches.isNotEmpty()) {
bestPartialMatch(partialMatches)
} else {
bestNonMatch(nonMatches)
}
}

private fun categoriseEachLogin(
logins: List<LoginCredentials>,
originalUrl: String,
perfectMatches: MutableList<LoginCredentials>,
partialMatches: MutableList<LoginCredentials>,
nonMatches: MutableList<LoginCredentials>,
) {
logins.forEach {
when (matchTypeDetector.detectMatchType(originalUrl, it)) {
PerfectMatch -> perfectMatches.add(it)
PartialMatch -> partialMatches.add(it)
NotAMatch -> nonMatches.add(it)
}
}
}

private fun bestPerfectMatch(perfectMatches: List<LoginCredentials>): LoginCredentials {
return perfectMatches.sortedWith(AutofillDeduplicationLoginComparator()).first()
}

private fun bestPartialMatch(partialMatches: MutableList<LoginCredentials>): LoginCredentials {
return partialMatches.sortedWith(AutofillDeduplicationLoginComparator()).first()
}

private fun bestNonMatch(nonMatches: MutableList<LoginCredentials>): LoginCredentials {
return nonMatches.sortedWith(AutofillDeduplicationLoginComparator()).first()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright (c) 2023 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.deduper

import com.duckduckgo.autofill.api.domain.app.LoginCredentials
import com.duckduckgo.autofill.impl.deduper.AutofillDeduplicationMatchTypeDetector.MatchType
import com.duckduckgo.autofill.impl.urlmatcher.AutofillUrlMatcher
import com.duckduckgo.di.scopes.AppScope
import com.squareup.anvil.annotations.ContributesBinding
import javax.inject.Inject

interface AutofillDeduplicationMatchTypeDetector {

fun detectMatchType(
originalUrl: String,
login: LoginCredentials,
): MatchType

sealed interface MatchType {
object PerfectMatch : MatchType
object PartialMatch : MatchType
object NotAMatch : MatchType
}
}

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

override fun detectMatchType(
originalUrl: String,
login: LoginCredentials,
): MatchType {
val visitedSiteParts = urlMatcher.extractUrlPartsForAutofill(originalUrl)
val savedSiteParts = urlMatcher.extractUrlPartsForAutofill(login.domain)

if (!urlMatcher.matchingForAutofill(visitedSiteParts, savedSiteParts)) {
return MatchType.NotAMatch
}

return if (visitedSiteParts.subdomain == savedSiteParts.subdomain) {
MatchType.PerfectMatch
} else {
MatchType.PartialMatch
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright (c) 2023 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.deduper

import com.duckduckgo.autofill.api.domain.app.LoginCredentials
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>>
}

@ContributesBinding(AppScope::class)
class RealAutofillDeduplicationUsernameAndPasswordMatcher @Inject constructor() : AutofillDeduplicationUsernameAndPasswordMatcher {

override fun groupDuplicateCredentials(logins: List<LoginCredentials>): Map<Pair<String?, String?>, List<LoginCredentials>> {
return logins.groupBy { it.username to it.password }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright (c) 2023 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.deduper

import com.duckduckgo.autofill.api.domain.app.LoginCredentials
import com.duckduckgo.di.scopes.AppScope
import com.squareup.anvil.annotations.ContributesBinding
import javax.inject.Inject

interface AutofillLoginDeduplicator {

fun deduplicate(
originalUrl: String,
logins: List<LoginCredentials>,
): List<LoginCredentials>
}

@ContributesBinding(AppScope::class)
class RealAutofillLoginDeduplicator @Inject constructor(
private val usernamePasswordMatcher: AutofillDeduplicationUsernameAndPasswordMatcher,
private val bestMatchFinder: AutofillDeduplicationBestMatchFinder,
) : AutofillLoginDeduplicator {

override fun deduplicate(
originalUrl: String,
logins: List<LoginCredentials>,
): List<LoginCredentials> {
val dedupedLogins = mutableListOf<LoginCredentials>()

val groups = usernamePasswordMatcher.groupDuplicateCredentials(logins)
groups.forEach {
val bestMatchForGroup = bestMatchFinder.findBestMatch(originalUrl, it.value)
if (bestMatchForGroup != null) {
dedupedLogins.add(bestMatchForGroup)
}
}

return dedupedLogins
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright (c) 2023 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.deduper

import com.duckduckgo.autofill.api.domain.app.LoginCredentials

class AutofillDeduplicationLoginComparator : Comparator<LoginCredentials> {
override fun compare(
o1: LoginCredentials,
o2: LoginCredentials,
): Int {
val lastModifiedComparison = compareLastModified(o1.lastUpdatedMillis, o2.lastUpdatedMillis)
if (lastModifiedComparison != 0) return lastModifiedComparison

// last updated matches, fallback to domain
return compareDomains(o1.domain, o2.domain)
}

private fun compareLastModified(
lastModified1: Long?,
lastModified2: Long?,
): Int {
if (lastModified1 == null && lastModified2 == null) return 0

if (lastModified1 == null) return -1
if (lastModified2 == null) return 1
return lastModified2.compareTo(lastModified1)
}

private fun compareDomains(
domain1: String?,
domain2: String?,
): Int {
if (domain1 == null && domain2 == null) return 0
if (domain1 == null) return -1
if (domain2 == null) return 1
return domain1.compareTo(domain2)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.duckduckgo.autofill.api.email.EmailManager
import com.duckduckgo.autofill.api.passwordgeneration.AutomaticSavedLoginsMonitor
import com.duckduckgo.autofill.api.store.AutofillStore
import com.duckduckgo.autofill.impl.AutofillStoredBackJavascriptInterface.UrlProvider
import com.duckduckgo.autofill.impl.deduper.AutofillLoginDeduplicator
import com.duckduckgo.autofill.impl.email.incontext.availability.EmailProtectionInContextRecentInstallChecker
import com.duckduckgo.autofill.impl.email.incontext.store.EmailProtectionInContextDataStore
import com.duckduckgo.autofill.impl.jsbridge.AutofillMessagePoster
Expand Down Expand Up @@ -75,6 +76,7 @@ class AutofillStoredBackJavascriptInterfaceTest {
private val inContextDataStore: EmailProtectionInContextDataStore = mock()
private val recentInstallChecker: EmailProtectionInContextRecentInstallChecker = mock()
private val testWebView = WebView(getApplicationContext())
private val loginDeduplicator: AutofillLoginDeduplicator = NoopDeduplicator()
private lateinit var testee: AutofillStoredBackJavascriptInterface

private val testCallback = TestCallback()
Expand All @@ -99,6 +101,7 @@ class AutofillStoredBackJavascriptInterfaceTest {
emailManager = emailManager,
inContextDataStore = inContextDataStore,
recentInstallChecker = recentInstallChecker,
loginDeduplicator = loginDeduplicator,
)
testee.callback = testCallback
testee.webView = testWebView
Expand Down Expand Up @@ -390,4 +393,8 @@ class AutofillStoredBackJavascriptInterfaceTest {
// no-op
}
}

private class NoopDeduplicator : AutofillLoginDeduplicator {
override fun deduplicate(originalUrl: String, logins: List<LoginCredentials>): List<LoginCredentials> = logins
}
}
Loading

0 comments on commit 68ed6f2

Please sign in to comment.