2952/proxy (#2961)

* replace hard-coded strings with existing constants

* proxy port

* * custom proxy port and hostname inputs
* typesafety, refactor, linting, unit tests

* relocate ProxyConfiguration in app structure

* remove unused editTextPreference fn

* allow preference category to have no title

* refactor proxy prefs hierarchy/dependency

app/src/main/java/com/keylesspalace/tusky/components/preference/ProxyPreferencesFragment.kt

@@ -19,9 +19,11 @@ import android.os.Bundle
 import androidx.preference.PreferenceFragmentCompat
 import com.keylesspalace.tusky.R
 import com.keylesspalace.tusky.settings.PrefKeys
-import com.keylesspalace.tusky.settings.editTextPreference
+import com.keylesspalace.tusky.settings.ProxyConfiguration
 import com.keylesspalace.tusky.settings.makePreferenceScreen
+import com.keylesspalace.tusky.settings.preferenceCategory
 import com.keylesspalace.tusky.settings.switchPreference
+import com.keylesspalace.tusky.settings.validatedEditTextPreference
 import kotlin.system.exitProcess
 
 class ProxyPreferencesFragment : PreferenceFragmentCompat() {
@@ -36,18 +38,29 @@ class ProxyPreferencesFragment : PreferenceFragmentCompat() {
                 setDefaultValue(false)
             }
 
-            editTextPreference {
+            preferenceCategory { category ->
-                setTitle(R.string.pref_title_http_proxy_server)
+                category.dependency = PrefKeys.HTTP_PROXY_ENABLED
-                key = PrefKeys.HTTP_PROXY_SERVER
+                category.isIconSpaceReserved = false
-                isIconSpaceReserved = false
-                setSummaryProvider { text }
-            }
 
-            editTextPreference {
+                validatedEditTextPreference(null, ProxyConfiguration::isValidHostname) {
-                setTitle(R.string.pref_title_http_proxy_port)
+                    setTitle(R.string.pref_title_http_proxy_server)
-                key = PrefKeys.HTTP_PROXY_PORT
+                    key = PrefKeys.HTTP_PROXY_SERVER
-                isIconSpaceReserved = false
+                    isIconSpaceReserved = false
-                setSummaryProvider { text }
+                    setSummaryProvider { text }
+                }
+
+                val portErrorMessage = getString(
+                    R.string.pref_title_http_proxy_port_message,
+                    ProxyConfiguration.MIN_PROXY_PORT,
+                    ProxyConfiguration.MAX_PROXY_PORT
+                )
+
+                validatedEditTextPreference(portErrorMessage, ProxyConfiguration::isValidProxyPort) {
+                    setTitle(R.string.pref_title_http_proxy_port)
+                    key = PrefKeys.HTTP_PROXY_PORT
+                    isIconSpaceReserved = false
+                    setSummaryProvider { text }
+                }
             }
         }
     }

app/src/main/java/com/keylesspalace/tusky/di/NetworkModule.kt

@@ -18,6 +18,7 @@ package com.keylesspalace.tusky.di
 import android.content.Context
 import android.content.SharedPreferences
 import android.os.Build
+import android.util.Log
 import at.connyduck.calladapter.networkresult.NetworkResultCallAdapterFactory
 import com.google.gson.Gson
 import com.google.gson.GsonBuilder
@@ -27,6 +28,10 @@ import com.keylesspalace.tusky.json.Rfc3339DateJsonAdapter
 import com.keylesspalace.tusky.network.InstanceSwitchAuthInterceptor
 import com.keylesspalace.tusky.network.MastodonApi
 import com.keylesspalace.tusky.network.MediaUploadApi
+import com.keylesspalace.tusky.settings.PrefKeys.HTTP_PROXY_ENABLED
+import com.keylesspalace.tusky.settings.PrefKeys.HTTP_PROXY_PORT
+import com.keylesspalace.tusky.settings.PrefKeys.HTTP_PROXY_SERVER
+import com.keylesspalace.tusky.settings.ProxyConfiguration
 import com.keylesspalace.tusky.util.getNonNullString
 import dagger.Module
 import dagger.Provides
@@ -38,6 +43,7 @@ import retrofit2.Retrofit
 import retrofit2.adapter.rxjava3.RxJava3CallAdapterFactory
 import retrofit2.converter.gson.GsonConverterFactory
 import retrofit2.create
+import java.net.IDN
 import java.net.InetSocketAddress
 import java.net.Proxy
 import java.util.Date
@@ -64,9 +70,9 @@ class NetworkModule {
         context: Context,
         preferences: SharedPreferences
     ): OkHttpClient {
-        val httpProxyEnabled = preferences.getBoolean("httpProxyEnabled", false)
+        val httpProxyEnabled = preferences.getBoolean(HTTP_PROXY_ENABLED, false)
-        val httpServer = preferences.getNonNullString("httpProxyServer", "")
+        val httpServer = preferences.getNonNullString(HTTP_PROXY_SERVER, "")
-        val httpPort = preferences.getNonNullString("httpProxyPort", "-1").toIntOrNull() ?: -1
+        val httpPort = preferences.getNonNullString(HTTP_PROXY_PORT, "-1").toIntOrNull() ?: -1
         val cacheSize = 25 * 1024 * 1024L // 25 MiB
         val builder = OkHttpClient.Builder()
             .addInterceptor { chain ->
@@ -87,10 +93,13 @@ class NetworkModule {
             .writeTimeout(30, TimeUnit.SECONDS)
             .cache(Cache(context.cacheDir, cacheSize))
 
-        if (httpProxyEnabled && httpServer.isNotEmpty() && httpPort > 0 && httpPort < 65535) {
+        if (httpProxyEnabled) {
-            val address = InetSocketAddress.createUnresolved(httpServer, httpPort)
+            ProxyConfiguration.create(httpServer, httpPort)?.also { conf ->
-            builder.proxy(Proxy(Proxy.Type.HTTP, address))
+                val address = InetSocketAddress.createUnresolved(IDN.toASCII(conf.hostname), conf.port)
+                builder.proxy(Proxy(Proxy.Type.HTTP, address))
+            } ?: Log.w(TAG, "Invalid proxy configuration: ($httpServer, $httpPort)")
         }
+
         return builder
             .apply {
                 addInterceptor(InstanceSwitchAuthInterceptor(accountManager))
@@ -132,4 +141,8 @@ class NetworkModule {
             .build()
             .create()
     }
+
+    companion object {
+        private const val TAG = "NetworkModule"
+    }
 }

app/src/main/java/com/keylesspalace/tusky/settings/ProxyConfiguration.kt

@@ -0,0 +1,32 @@
+package com.keylesspalace.tusky.settings
+
+import java.net.IDN
+
+class ProxyConfiguration private constructor(
+    val hostname: String,
+    val port: Int
+) {
+    companion object {
+        fun create(hostname: String, port: Int): ProxyConfiguration? {
+            if (isValidHostname(IDN.toASCII(hostname)) && isValidProxyPort(port)) {
+                return ProxyConfiguration(hostname, port)
+            }
+            return null
+        }
+        fun isValidProxyPort(value: Any): Boolean = when (value) {
+            is String -> if (value == "") true else value.runCatching(String::toInt).map(
+                PROXY_RANGE::contains
+            ).getOrDefault(false)
+            is Int -> PROXY_RANGE.contains(value)
+            else -> false
+        }
+        fun isValidHostname(hostname: String): Boolean =
+            IP_ADDRESS_REGEX.matches(hostname) || HOSTNAME_REGEX.matches(hostname)
+        const val MIN_PROXY_PORT = 0
+        const val MAX_PROXY_PORT = 65535
+    }
+}
+
+private val PROXY_RANGE = IntRange(ProxyConfiguration.MIN_PROXY_PORT, ProxyConfiguration.MAX_PROXY_PORT)
+private val IP_ADDRESS_REGEX = Regex("^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$")
+private val HOSTNAME_REGEX = Regex("^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\\-]*[A-Za-z0-9])$")

app/src/main/java/com/keylesspalace/tusky/settings/SettingsDSL.kt

@@ -1,8 +1,10 @@
 package com.keylesspalace.tusky.settings
 
 import android.content.Context
+import android.widget.Button
 import androidx.activity.result.ActivityResultRegistryOwner
 import androidx.annotation.StringRes
+import androidx.core.widget.doAfterTextChanged
 import androidx.lifecycle.LifecycleOwner
 import androidx.preference.CheckBoxPreference
 import androidx.preference.EditTextPreference
@@ -50,10 +52,25 @@ inline fun PreferenceParent.switchPreference(
     return pref
 }
 
-inline fun PreferenceParent.editTextPreference(
+inline fun PreferenceParent.validatedEditTextPreference(
+    errorMessage: String?,
+    crossinline isValid: (a: String) -> Boolean,
     builder: EditTextPreference.() -> Unit
 ): EditTextPreference {
     val pref = EditTextPreference(context)
+    pref.setOnBindEditTextListener { editText ->
+        editText.doAfterTextChanged { editable ->
+            requireNotNull(editable)
+            val btn = editText.rootView.findViewById<Button>(android.R.id.button1)
+            if (isValid(editable.toString())) {
+                editText.error = null
+                btn.isEnabled = true
+            } else {
+                editText.error = errorMessage
+                btn.isEnabled = false
+            }
+        }
+    }
     builder(pref)
     addPref(pref)
     return pref
@@ -69,12 +86,12 @@ inline fun PreferenceParent.checkBoxPreference(
 }
 
 inline fun PreferenceParent.preferenceCategory(
-    @StringRes title: Int,
+    @StringRes title: Int? = null,
     builder: PreferenceParent.(PreferenceCategory) -> Unit
 ) {
     val category = PreferenceCategory(context)
     addPref(category)
-    category.setTitle(title)
+    title?.run(category::setTitle)
     val newParent = PreferenceParent(context) { category.addPreference(it) }
     builder(newParent, category)
 }

app/src/main/res/values/strings.xml

@@ -282,6 +282,7 @@
     <string name="pref_title_http_proxy_enable">Enable HTTP proxy</string>
     <string name="pref_title_http_proxy_server">HTTP proxy server</string>
     <string name="pref_title_http_proxy_port">HTTP proxy port</string>
+    <string name="pref_title_http_proxy_port_message">Port should be between %d and %d</string>
 
     <string name="pref_default_post_privacy">Default post privacy</string>
     <string name="pref_default_post_language">Default posting language</string>

app/src/test/java/com/keylesspalace/tusky/entity/ProxyConfigurationTest.kt

@@ -0,0 +1,46 @@
+package com.keylesspalace.tusky.entity
+
+import com.keylesspalace.tusky.settings.ProxyConfiguration
+import org.junit.Assert
+import org.junit.Test
+
+class ProxyConfigurationTest {
+    @Test
+    fun `serialized non-int is not valid proxy port`() {
+        Assert.assertFalse(ProxyConfiguration.isValidProxyPort("should fail"))
+        Assert.assertFalse(ProxyConfiguration.isValidProxyPort("1.5"))
+    }
+
+    @Test
+    fun `number outside port range is not valid`() {
+        Assert.assertFalse(ProxyConfiguration.isValidProxyPort("${ProxyConfiguration.MIN_PROXY_PORT - 1}"))
+        Assert.assertFalse(ProxyConfiguration.isValidProxyPort("${ProxyConfiguration.MAX_PROXY_PORT + 1}"))
+    }
+
+    @Test
+    fun `number in port range, inclusive of min and max, is valid`() {
+        Assert.assertTrue(ProxyConfiguration.isValidProxyPort(ProxyConfiguration.MIN_PROXY_PORT))
+        Assert.assertTrue(ProxyConfiguration.isValidProxyPort(ProxyConfiguration.MAX_PROXY_PORT))
+        Assert.assertTrue(ProxyConfiguration.isValidProxyPort((ProxyConfiguration.MIN_PROXY_PORT + ProxyConfiguration.MAX_PROXY_PORT) / 2))
+    }
+
+    @Test
+    fun `create with invalid port yields null`() {
+        Assert.assertNull(ProxyConfiguration.create("hostname", ProxyConfiguration.MIN_PROXY_PORT - 1))
+    }
+
+    @Test
+    fun `create with invalid hostname yields null`() {
+        Assert.assertNull(ProxyConfiguration.create(".", ProxyConfiguration.MIN_PROXY_PORT))
+    }
+
+    @Test
+    fun `create with valid hostname and port yields the config object`() {
+        Assert.assertTrue(ProxyConfiguration.create("hostname", ProxyConfiguration.MIN_PROXY_PORT) is ProxyConfiguration)
+    }
+
+    @Test
+    fun `unicode hostname allowed`() {
+        Assert.assertTrue(ProxyConfiguration.create("federação.social", ProxyConfiguration.MIN_PROXY_PORT) is ProxyConfiguration)
+    }
+}