Consider ports for autofill matching and for displaying logins (#2824)

<!--
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)
    3. Make sure these changes are tested in API 23 and API 26
If your PR does not involve UI changes, you can remove the **UI
changes** section
-->

Task/Issue URL:
https://app.asana.com/0/488551667048375/1203892213154868/f

### Description
Considers port numbers when comparing urls, and displays port number in
the UI if set.

### Steps to test this PR

#### Testing when saved site contains a port
- [x] Add a login for `fill.dev`
- [x] Visit `fill.dev/form/login-simple` and verify it offers to
autofill as normal
- [x] Open credential management screen; verify it is listed as a
`suggested` site
- [x] Edit the login and append a port to the URL (e.g.,
`fill.dev:9000`)
- [x] Verify the port number is visible in the list item url (assuming a
title isn't given)
- [x] Visit the login page again (refreshing if required); verify you
are **not prompted** to autofill (as ports don't match)
- [x] Open credential management screen; verify it is **not** listed as
a `suggested` site


#### Testing when visited site contains a port
This is harder, unless you happen to know a publicly accessible website
which uses a non-standard web port. If not, simulate the test by
hardcoding a URL for the current site when launching creds management
screen.
- [x] `return AutofillManagementActivity.intentShowSuggestion(context,
"fill.dev:9000")` on `AutofillManagementActivity`, line 346
- [x] Open credential management screen; verify it is listed as a
`suggested` site

autofill/autofill-api/src/main/java/com/duckduckgo/autofill/api/urlmatcher/AutofillUrlMatcher.kt

@@ -29,5 +29,9 @@ interface AutofillUrlMatcher {
      */
     fun cleanRawUrl(rawUrl: String): String
 
-    data class ExtractedUrlParts(val eTldPlus1: String?, val subdomain: String?)
+    data class ExtractedUrlParts(
+        val eTldPlus1: String?,
+        val subdomain: String?,
+        val port: Int? = null,
+    )
 }

autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/management/LoginCredentialTitleExtractor.kt

@@ -18,7 +18,6 @@ package com.duckduckgo.autofill.impl.ui.credential.management
 
 import com.duckduckgo.autofill.api.domain.app.LoginCredentials
 import com.duckduckgo.autofill.api.urlmatcher.AutofillUrlMatcher
-import com.duckduckgo.autofill.api.urlmatcher.AutofillUrlMatcher.ExtractedUrlParts
 import com.duckduckgo.di.scopes.FragmentScope
 import com.squareup.anvil.annotations.ContributesBinding
 import javax.inject.Inject
@@ -38,14 +37,8 @@ class TitleOrDomainExtractor @Inject constructor(
             return title
         }
 
-        return urlMatcher.extractUrlPartsForAutofill(credential.domain).format()
-    }
-
-    private fun ExtractedUrlParts.format(): String {
-        return if (subdomain.isNullOrBlank()) {
-            eTldPlus1 ?: ""
-        } else {
-            "$subdomain.$eTldPlus1"
-        }
+        return credential.domain?.let {
+            urlMatcher.cleanRawUrl(it)
+        } ?: ""
     }
 }

autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/management/TitleOrDomainExtractorTest.kt

@@ -16,47 +16,40 @@
 
 package com.duckduckgo.autofill.impl.ui.credential.management
 
+import androidx.test.ext.junit.runners.AndroidJUnit4
 import com.duckduckgo.autofill.api.domain.app.LoginCredentials
 import com.duckduckgo.autofill.api.urlmatcher.AutofillUrlMatcher
-import com.duckduckgo.autofill.api.urlmatcher.AutofillUrlMatcher.ExtractedUrlParts
+import com.duckduckgo.autofill.store.urlmatcher.AutofillDomainNameUrlMatcher
 import org.junit.Assert.assertEquals
 import org.junit.Test
-import org.mockito.kotlin.mock
-import org.mockito.kotlin.never
-import org.mockito.kotlin.verify
-import org.mockito.kotlin.whenever
+import org.junit.runner.RunWith
 
+@RunWith(AndroidJUnit4::class)
 class TitleOrDomainExtractorTest {
 
-    private val domainFormatter: AutofillUrlMatcher = mock()
+    private val domainFormatter: AutofillUrlMatcher = AutofillDomainNameUrlMatcher()
     private val testee = TitleOrDomainExtractor(domainFormatter)
 
     @Test
     fun whenTitleIsNullAndNoSubdomainThenDomainIsUsed() {
-        whenever(domainFormatter.extractUrlPartsForAutofill("example.com")).thenReturn(ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null))
         val result = testee.extract(creds(title = null, domain = "example.com"))
         assertEquals("example.com", result)
     }
 
     @Test
     fun whenTitleIsNullAndDomainHasSubdomainThenCombinedSubdomainAndDomainIsUsed() {
-        whenever(domainFormatter.extractUrlPartsForAutofill("example.com")).thenReturn(
-            ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = "foo"),
-        )
-        val result = testee.extract(creds(title = null, domain = "example.com"))
+        val result = testee.extract(creds(title = null, domain = "foo.example.com"))
         assertEquals("foo.example.com", result)
     }
 
     @Test
     fun whenTitleIsEmptyThenDomainIsUsed() {
-        whenever(domainFormatter.extractUrlPartsForAutofill("example.com")).thenReturn(ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null))
         val result = testee.extract(creds(title = "", domain = "example.com"))
         assertEquals("example.com", result)
     }
 
     @Test
     fun whenTitleIsBlankThenDomainIsUsed() {
-        whenever(domainFormatter.extractUrlPartsForAutofill("example.com")).thenReturn(ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null))
         val result = testee.extract(creds(title = "  ", domain = "example.com"))
         assertEquals("example.com", result)
     }
@@ -65,7 +58,24 @@ class TitleOrDomainExtractorTest {
     fun whenTitleIsPopulatedThenDomainNotUsed() {
         val result = testee.extract(creds(title = "Site", domain = "example.com"))
         assertEquals("Site", result)
-        verify(domainFormatter, never()).extractUrlPartsForAutofill("example.com")
+    }
+
+    @Test
+    fun whenPortIsNullThenPortOmitted() {
+        val result = testee.extract(creds(title = null, domain = "example.com"))
+        assertEquals("example.com", result)
+    }
+
+    @Test
+    fun whenPortSpecifiedInCredentialThenPortIncluded() {
+        val result = testee.extract(creds(title = null, domain = "example.com:9000"))
+        assertEquals("example.com:9000", result)
+    }
+
+    @Test
+    fun whenIsIpAddressThenIpAddressIsUsed() {
+        val result = testee.extract(creds(domain = "192.168.0.1", title = null))
+        assertEquals("192.168.0.1", result)
     }
 
     private fun creds(

autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/management/suggestion/SuggestionMatcherTest.kt

@@ -77,6 +77,34 @@ class SuggestionMatcherTest {
         assertEquals(1, suggestions.size)
     }
 
+    @Test
+    fun whenPortIncludedInSavedSiteAndNotInVisitedSiteThenNotASuggestion() {
+        val creds = listOf(creds("example.com:8080"))
+        val suggestions = testee.getSuggestions("example.com", creds)
+        assertEquals(0, suggestions.size)
+    }
+
+    @Test
+    fun whenPortIncludedInVisitedSiteAndNotInSavedSiteThenNotASuggestion() {
+        val creds = listOf(creds("example.com"))
+        val suggestions = testee.getSuggestions("example.com:8080", creds)
+        assertEquals(0, suggestions.size)
+    }
+
+    @Test
+    fun whenPortIncludedInVisitedSiteDiffersFromPortInSavedSiteThenNotASuggestion() {
+        val creds = listOf(creds("example.com:9000"))
+        val suggestions = testee.getSuggestions("example.com:8080", creds)
+        assertEquals(0, suggestions.size)
+    }
+
+    @Test
+    fun whenPortIncludedInVisitedSiteMatchesPortInSavedSiteThenNotASuggestion() {
+        val creds = listOf(creds("example.com:9000"))
+        val suggestions = testee.getSuggestions("example.com:9000", creds)
+        assertEquals(1, suggestions.size)
+    }
+
     private fun creds(domain: String): LoginCredentials {
         return LoginCredentials(id = 0, domain = domain, username = "username", password = "password")
     }

autofill/autofill-store/src/main/java/com/duckduckgo/autofill/store/SecureStoreBackedAutofillStore.kt

@@ -200,7 +200,7 @@ class SecureStoreBackedAutofillStore(
         username: String?,
         password: String?,
     ): ContainsCredentialsResult {
-        val url = autofillUrlMatcher.cleanRawUrl(rawUrl) ?: return NoMatch
+        val url = autofillUrlMatcher.cleanRawUrl(rawUrl)
         val credentials = secureStorage.websiteLoginDetailsWithCredentialsForDomain(url).firstOrNull() ?: return NoMatch
 
         var exactMatchFound = false

autofill/autofill-store/src/main/java/com/duckduckgo/autofill/store/urlmatcher/AutofillDomainNameUrlMatcher.kt

@@ -27,17 +27,19 @@ import timber.log.Timber
 class AutofillDomainNameUrlMatcher @Inject constructor() : AutofillUrlMatcher {
 
     override fun extractUrlPartsForAutofill(originalUrl: String?): ExtractedUrlParts {
-        if (originalUrl == null) return ExtractedUrlParts(null, null)
+        if (originalUrl == null) return unextractable()
 
         val normalizedUrl = originalUrl.normalizeScheme()
         return try {
-            val eTldPlus1 = normalizedUrl.toHttpUrl().topPrivateDomain()
+            val httpUrl = normalizedUrl.toHttpUrl()
+            val eTldPlus1 = httpUrl.topPrivateDomain()
             val domain = originalUrl.extractDomain()
+            val port = httpUrl.port
             val subdomain = determineSubdomain(domain, eTldPlus1)
-            ExtractedUrlParts(eTldPlus1, subdomain)
+            ExtractedUrlParts(eTldPlus1, subdomain, port)
         } catch (e: IllegalArgumentException) {
             Timber.w("Unable to parse e-tld+1 from $originalUrl")
-            ExtractedUrlParts(null, null)
+            unextractable()
         }
     }
 
@@ -57,6 +59,9 @@ class AutofillDomainNameUrlMatcher @Inject constructor() : AutofillUrlMatcher {
         visitedSite: ExtractedUrlParts,
         savedSite: ExtractedUrlParts,
     ): Boolean {
+        // ports must match (both being null is considered a match)
+        if (visitedSite.port != savedSite.port) return false
+
         // e-tld+1 must match
         if (!identicalEffectiveTldPlusOne(visitedSite, savedSite)) return false
 
@@ -105,6 +110,10 @@ class AutofillDomainNameUrlMatcher @Inject constructor() : AutofillUrlMatcher {
         return this
     }
 
+    private fun unextractable(): ExtractedUrlParts {
+        return ExtractedUrlParts(null, null, null)
+    }
+
     companion object {
         private const val WWW = "www"
     }

autofill/autofill-store/src/test/java/com/duckduckgo/autofill/store/urlmatcher/AutofillDomainNameUrlMatcherTest.kt

@@ -17,6 +17,7 @@
 package com.duckduckgo.autofill.store.urlmatcher
 
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.duckduckgo.autofill.api.urlmatcher.AutofillUrlMatcher.ExtractedUrlParts
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertNull
@@ -176,6 +177,34 @@ class AutofillDomainNameUrlMatcherTest {
         assertTrue(testee.matchingForAutofill(visitedSite, savedSite))
     }
 
+    @Test
+    fun whenSavedSiteMatchesVisitedExceptForPortThenNotMatchingForAutofill() {
+        val savedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = 8000)
+        val visitedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = 1000)
+        assertFalse(testee.matchingForAutofill(visitedSite, savedSite))
+    }
+
+    @Test
+    fun whenSavedSiteMatchesVisitedAndEqualPortsThenMatchingForAutofill() {
+        val savedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = 8000)
+        val visitedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = 8000)
+        assertTrue(testee.matchingForAutofill(visitedSite, savedSite))
+    }
+
+    @Test
+    fun whenSavedSiteMatchesVisitedAndSavedSiteMissingPortThenNotMatchingForAutofill() {
+        val savedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = null)
+        val visitedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = 8000)
+        assertFalse(testee.matchingForAutofill(visitedSite, savedSite))
+    }
+
+    @Test
+    fun whenSavedSiteMatchesVisitedAndVisitedSiteMissingPortThenNotMatchingForAutofill() {
+        val savedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = 8000)
+        val visitedSite = ExtractedUrlParts(eTldPlus1 = "example.com", subdomain = null, port = null)
+        assertFalse(testee.matchingForAutofill(visitedSite, savedSite))
+    }
+
     @Test
     fun whenSavedSiteContainsUppercaseWwwSubdomainAndVisitedSiteDoesNotThenMatchingForAutofill() {
         val savedSite = testee.extractUrlPartsForAutofill("WWW.example.com")
@@ -239,6 +268,10 @@ class AutofillDomainNameUrlMatcherTest {
         assertEquals("foo.com", testee.cleanRawUrl("foo.com"))
         assertEquals("foo.com:9000", testee.cleanRawUrl("foo.com:9000"))
         assertEquals("fuu.foo.com", testee.cleanRawUrl("fuu.foo.com"))
+        assertEquals("192.168.0.1", testee.cleanRawUrl("192.168.0.1"))
+        assertEquals("192.168.0.1:9000", testee.cleanRawUrl("192.168.0.1:9000"))
+        assertEquals("192.168.0.1", testee.cleanRawUrl("http://192.168.0.1"))
+        assertEquals("192.168.0.1:9000", testee.cleanRawUrl("http://192.168.0.1:9000"))
         assertEquals("fuu.foo.com:9000", testee.cleanRawUrl("fuu.foo.com:9000"))
         assertEquals("RandomText", testee.cleanRawUrl("thisIs@RandomText"))
     }