Extract ResourceDownloader from ContactsSyncManager, add tests (#1849)

* Add ResourceDownloader and tests

- Introduce `ResourceDownloader` class for downloading external resources
- Add unit tests for `ResourceDownloader`
- Refactor `ContactsSyncManager` to use `ResourceDownloader`

* KDoc

- Add detailed documentation for `download` method
- Clarify authentication handling and return behavior

* Minor changes

app/src/androidTest/kotlin/at/bitfire/davdroid/sync/ResourceDownloaderTest.kt

@@ -0,0 +1,109 @@
+/*
+ * Copyright © All Contributors. See LICENSE and AUTHORS in the root directory for details.
+ */
+
+package at.bitfire.davdroid.sync
+
+import android.accounts.Account
+import at.bitfire.dav4jvm.HttpUtils.toKtorUrl
+import at.bitfire.davdroid.settings.AccountSettings
+import at.bitfire.davdroid.settings.Credentials
+import at.bitfire.davdroid.sync.account.TestAccount
+import at.bitfire.davdroid.util.SensitiveString.Companion.toSensitiveString
+import dagger.hilt.android.testing.HiltAndroidRule
+import dagger.hilt.android.testing.HiltAndroidTest
+import io.ktor.http.HttpHeaders
+import kotlinx.coroutines.test.runTest
+import okhttp3.mockwebserver.MockResponse
+import okhttp3.mockwebserver.MockWebServer
+import org.junit.After
+import org.junit.Assert.assertArrayEquals
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Assume
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+import java.net.InetAddress
+import javax.inject.Inject
+
+@HiltAndroidTest
+class ResourceDownloaderTest {
+
+    @get:Rule
+    val hiltRule = HiltAndroidRule(this)
+
+    @Inject
+    lateinit var accountSettingsFactory: AccountSettings.Factory
+
+    @Inject
+    lateinit var resourceDownloaderFactory: ResourceDownloader.Factory
+
+    lateinit var account: Account
+    lateinit var server: MockWebServer
+
+    @Before
+    fun setUp() {
+        hiltRule.inject()
+        server = MockWebServer().apply {
+            start()
+        }
+
+        account = TestAccount.create()
+
+        // add credentials to test account so that we can check whether they have been sent
+        val settings = accountSettingsFactory.create(account)
+        settings.credentials(Credentials("test", "test".toSensitiveString()))
+    }
+
+    @After
+    fun tearDown() {
+        TestAccount.remove(account)
+        server.close()
+    }
+
+
+    @Test
+    fun testDownload_ExternalDomain() = runTest {
+        val baseUrl = server.url("/")
+
+        // URL should be http://localhost, replace with http://127.0.0.1 to have other domain
+        Assume.assumeTrue(baseUrl.host == "localhost")
+        val baseUrlIp = baseUrl.newBuilder()
+            .host(InetAddress.getByName(baseUrl.host).hostAddress!!)
+            .build()
+
+        server.enqueue(MockResponse()
+            .setResponseCode(200)
+            .setBody("TEST"))
+
+        val downloader = resourceDownloaderFactory.create(account, baseUrl.host)
+        val result = downloader.download(baseUrlIp.toKtorUrl())
+
+        // authentication was NOT sent because request is not for original domain
+        val sentAuth = server.takeRequest().getHeader(HttpHeaders.Authorization)
+        assertNull(sentAuth)
+
+        // and result is OK
+        assertArrayEquals("TEST".toByteArray(), result)
+    }
+
+    @Test
+    fun testDownload_SameDomain() = runTest {
+        server.enqueue(MockResponse()
+            .setResponseCode(200)
+            .setBody("TEST"))
+
+        val baseUrl = server.url("/")
+        val downloader = resourceDownloaderFactory.create(account, baseUrl.host)
+        val result = downloader.download(baseUrl.toKtorUrl())
+
+        // authentication was sent
+        val sentAuth = server.takeRequest().getHeader(HttpHeaders.Authorization)
+        assertEquals("Basic dGVzdDp0ZXN0", sentAuth)
+
+        // and result is OK
+        assertArrayEquals("TEST".toByteArray(), result)
+    }
+
+}

app/src/main/kotlin/at/bitfire/davdroid/sync/ContactsSyncManager.kt

@@ -7,6 +7,7 @@ package at.bitfire.davdroid.sync
 import android.accounts.Account
 import android.content.ContentProviderClient
 import android.text.format.Formatter
+import at.bitfire.dav4jvm.ktor.toUrlOrNull
 import at.bitfire.dav4jvm.okhttp.DavAddressBook
 import at.bitfire.dav4jvm.okhttp.MultiResponseCallback
 import at.bitfire.dav4jvm.okhttp.Response
@@ -24,7 +25,6 @@ import at.bitfire.davdroid.Constants
 import at.bitfire.davdroid.R
 import at.bitfire.davdroid.db.Collection
 import at.bitfire.davdroid.di.SyncDispatcher
-import at.bitfire.davdroid.network.HttpClientBuilder
 import at.bitfire.davdroid.resource.LocalAddress
 import at.bitfire.davdroid.resource.LocalAddressBook
 import at.bitfire.davdroid.resource.LocalContact
@@ -46,21 +46,18 @@ import dagger.assisted.AssistedInject
 import ezvcard.VCardVersion
 import ezvcard.io.CannotParseException
 import kotlinx.coroutines.CoroutineDispatcher
+import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.runInterruptible
 import okhttp3.HttpUrl
-import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 import okhttp3.MediaType
 import okhttp3.MediaType.Companion.toMediaTypeOrNull
 import okhttp3.OkHttpClient
-import okhttp3.Request
 import okhttp3.RequestBody.Companion.toRequestBody
 import java.io.ByteArrayOutputStream
-import java.io.IOException
 import java.io.Reader
 import java.io.StringReader
 import java.util.Optional
 import java.util.logging.Level
-import javax.inject.Provider
 import kotlin.jvm.optionals.getOrNull
 
 /**
@@ -111,7 +108,7 @@ class ContactsSyncManager @AssistedInject constructor(
     @Assisted val syncFrameworkUpload: Boolean,
     val dirtyVerifier: Optional<ContactDirtyVerifier>,
     accountSettingsFactory: AccountSettings.Factory,
-    private val httpClientBuilder: Provider<HttpClientBuilder>,
+    private val resourceDownloaderFactory: ResourceDownloader.Factory,
     @SyncDispatcher syncDispatcher: CoroutineDispatcher
 ): SyncManager<LocalAddress, LocalAddressBook, DavAddressBook>(
     account,
@@ -151,11 +148,6 @@ class ContactsSyncManager @AssistedInject constructor(
         GroupMethod.CATEGORIES -> CategoriesStrategy(localAddressBook)
     }
 
-    /**
-     * Used to download images which are referenced by URL
-     */
-    private lateinit var resourceDownloader: ResourceDownloader
-
 
     override fun prepare(): Boolean {
         if (dirtyVerifier.isPresent) {
@@ -165,7 +157,6 @@ class ContactsSyncManager @AssistedInject constructor(
         }
 
         davCollection = DavAddressBook(httpClient, collection.url)
-        resourceDownloader = ResourceDownloader(davCollection.location)
 
         logger.info("Contact group strategy: ${groupStrategy::class.java.simpleName}")
         return true
@@ -371,11 +362,20 @@ class ContactsSyncManager @AssistedInject constructor(
                         }
 
                         processCard(
-                            response.href.lastSegment,
+                            fileName = response.href.lastSegment,
-                            eTag,
+                            eTag = eTag,
-                            StringReader(card),
+                            reader = StringReader(card),
-                            isJCard,
+                            jCard = isJCard,
-                            resourceDownloader
+                            downloader = object : Contact.Downloader {
+                                override fun download(url: String, accepts: String): ByteArray? {
+                                    // download external resource (like a photo) from an URL
+                                    val httpUrl = url.toUrlOrNull() ?: return null
+                                    val downloader = resourceDownloaderFactory.create(account, davCollection.location.host)
+                                    return runBlocking(syncDispatcher) {
+                                        downloader.download(httpUrl)
+                                    }
+                                }
+                            }
                         )
                     }
                 }
@@ -481,43 +481,6 @@ class ContactsSyncManager @AssistedInject constructor(
     }
 
 
-    // downloader helper class
-
-    private inner class ResourceDownloader(
-        val baseUrl: HttpUrl
-    ): Contact.Downloader {
-
-        override fun download(url: String, accepts: String): ByteArray? {
-            val httpUrl = url.toHttpUrlOrNull()
-            if (httpUrl == null) {
-                logger.log(Level.SEVERE, "Invalid external resource URL", url)
-                return null
-            }
-
-            // authenticate only against a certain host, and only upon request
-            val hostHttpClient = httpClientBuilder
-                .get()
-                .fromAccount(account, onlyHost = baseUrl.host)
-                .followRedirects(true)      // allow redirects
-                .build()
-            try {
-                val response = hostHttpClient.newCall(Request.Builder()
-                    .get()
-                    .url(httpUrl)
-                    .build()).execute()
-
-                if (response.isSuccessful)
-                    return response.body.bytes()
-                else
-                    logger.warning("Couldn't download external resource")
-            } catch(e: IOException) {
-                logger.log(Level.SEVERE, "Couldn't download external resource", e)
-            }
-
-            return null
-        }
-    }
-
     override fun notifyInvalidResourceTitle(): String =
             context.getString(R.string.sync_invalid_contact)
 

app/src/main/kotlin/at/bitfire/davdroid/sync/ResourceDownloader.kt

@@ -0,0 +1,74 @@
+/*
+ * Copyright © All Contributors. See LICENSE and AUTHORS in the root directory for details.
+ */
+
+package at.bitfire.davdroid.sync
+
+import android.accounts.Account
+import at.bitfire.davdroid.network.HttpClientBuilder
+import dagger.assisted.Assisted
+import dagger.assisted.AssistedFactory
+import dagger.assisted.AssistedInject
+import io.ktor.client.request.get
+import io.ktor.client.statement.bodyAsBytes
+import io.ktor.http.Url
+import io.ktor.http.isSuccess
+import java.io.IOException
+import java.util.logging.Level
+import java.util.logging.Logger
+import javax.inject.Provider
+
+/**
+ * Downloads a separate resource that is referenced during synchronization, for instance in
+ * a vCard with `PHOTO:<external URL>`.
+ *
+ * The [ResourceDownloader] only sends authentication for URLs on the same domain as the
+ * original URL. For instance, if the vCard that references a photo is taken from
+ * `example.com` ([originalHost]), then [download] will send authentication
+ * when downloading `https://example.com/photo.jpg`, but not for `https://external-hoster.com/photo.jpg`.
+ *
+ * @param account       account to build authentication from
+ * @param originalHost  client only authenticates for the domain of this host
+ */
+class ResourceDownloader @AssistedInject constructor(
+    @Assisted private val account: Account,
+    @Assisted private val originalHost: String,
+    private val httpClientBuilder: Provider<HttpClientBuilder>,
+    private val logger: Logger
+) {
+
+    @AssistedFactory
+    interface Factory {
+        fun create(account: Account, originalHost: String): ResourceDownloader
+    }
+
+    /**
+     * Downloads the given resource and returns it as an in-memory blob.
+     *
+     * Authentication is handled as described in [ResourceDownloader].
+     *
+     * @param url       URL of the resource to download
+     *
+     * @return blob of requested resource, or `null` on error
+     */
+    suspend fun download(url: Url): ByteArray? {
+        httpClientBuilder
+            .get()
+            .fromAccount(account, onlyHost = originalHost)  // restricts authentication to original domain
+            .followRedirects(true)      // allow redirects
+            .buildKtor()
+            .use { httpClient ->
+                try {
+                    val response = httpClient.get(url)
+                    if (response.status.isSuccess())
+                        return response.bodyAsBytes()
+                    else
+                        logger.warning("Couldn't download external resource (${response.status})")
+                } catch(e: IOException) {
+                    logger.log(Level.SEVERE, "Couldn't download external resource", e)
+                }
+            }
+        return null
+    }
+
+}