Login: Keep the mTLS certificate when handling the OAuth redirect

opencloudApp/src/main/java/eu/opencloud/android/extensions/ThrowableExt.kt

@@ -52,6 +52,7 @@ import eu.opencloud.android.domain.exceptions.ServerResponseTimeoutException
 import eu.opencloud.android.domain.exceptions.ServiceUnavailableException
 import eu.opencloud.android.domain.exceptions.SpecificForbiddenException
 import eu.opencloud.android.domain.exceptions.UnauthorizedException
+import eu.opencloud.android.domain.exceptions.UnhandledHttpCodeException
 import eu.opencloud.android.domain.exceptions.validation.FileNameException
 import java.util.Locale
 
@@ -98,6 +99,10 @@ fun Throwable.parseError(
             is ServiceUnavailableException -> resources.getString(R.string.service_unavailable)
             is SpecificForbiddenException -> resources.getString(R.string.uploads_view_upload_status_failed_permission_error)
             is UnauthorizedException -> resources.getString(R.string.auth_unauthorized)
+            // Naming the status beats "unknown error": it is the only clue the user can pass on to an admin.
+            is UnhandledHttpCodeException ->
+                if (httpCode > 0) resources.getString(R.string.error_http_code, httpCode)
+                else resources.getString(R.string.common_error_unknown)
             is NetworkErrorException -> resources.getString(R.string.network_error_message)
             is ResourceLockedException -> resources.getString(R.string.resource_locked_error_message)
             else -> resources.getString(R.string.common_error_unknown)

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

@@ -222,6 +222,7 @@
     <string name="common_loading">Loading…</string>
     <string name="common_unknown">unknown</string>
     <string name="common_error_unknown">unknown error</string>
+    <string name="error_http_code">Server returned an unexpected error (HTTP %1$d)</string>
     <string name="common_pending">Pending</string>
     <string name="common_important">Important</string>
     <string name="change_password">Change password</string>
@@ -377,6 +378,7 @@
     <string name="auth_expired_basic_auth_toast">Please enter the current password</string>
     <string name="auth_connecting_auth_server">Connecting to authentication server …</string>
     <string name="auth_unsupported_auth_method">The server does not support this authentication method</string>
+    <string name="auth_forbidden_check_client_cert">Server refused the connection. If it requires a client certificate, check the one selected under Connection.</string>
     <string name="auth_fail_get_user_name">Your server is not returning a correct user ID. Please contact an administrator.</string>
     <string name="auth_can_not_auth_against_server">Cannot authenticate to this server</string>
     <string name="auth_account_does_not_exist">Account does not exist in the device yet</string>

opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/GetRemoteStatusOperation.kt

@@ -61,7 +61,9 @@ class GetRemoteStatusOperation : RemoteOperation<RemoteServerInfo>() {
             val requester = StatusRequester()
             val requestResult = requester.request(baseUrl, client)
             val result = requester.handleRequestResult(requestResult, baseUrl)
-            updateClientBaseUrl(client, result.data.baseUrl)
+            // data is only set on success; dereferencing it unconditionally turned every failed
+            // status check into an opaque NPE result instead of the actual HTTP error.
+            result.data?.let { updateClientBaseUrl(client, it.baseUrl) }
             return result
         } catch (e: JSONException) {
             Timber.e(e, "JSON is not correct")

opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/StatusRequester.kt

@@ -91,10 +91,14 @@ internal class StatusRequester {
         requestResult: RequestResult,
         baseUrl: String
     ): RemoteOperationResult<RemoteServerInfo> {
+        // Check the status code before touching the body. A failed response is very often not JSON at
+        // all (a reverse proxy error page, an mTLS rejection, a captive portal, an empty body), and
+        // parsing it first would throw and hide the real HTTP error behind INSTANCE_NOT_CONFIGURED.
+        if (!requestResult.status.isSuccess()) {
+            return RemoteOperationResult(requestResult.getMethod)
+        }
         val respJSON = JSONObject(requestResult.getMethod.getResponseBodyAsString())
-        return if (!requestResult.status.isSuccess()) {
-            RemoteOperationResult(requestResult.getMethod)
-        } else if (!respJSON.getBoolean(NODE_INSTALLED)) {
+        return if (!respJSON.getBoolean(NODE_INSTALLED)) {
             RemoteOperationResult(RemoteOperationResult.ResultCode.INSTANCE_NOT_CONFIGURED)
         } else {
             val ocVersion = OpenCloudVersion(respJSON.getString(NODE_VERSION), respJSON.getString(NODE_PRODUCTVERSION))

opencloudComLibrary/src/test/java/eu/opencloud/android/lib/StatusRequesterHandleResultTest.kt

@@ -0,0 +1,135 @@
+/* openCloud Android Library is available under MIT license
+*   Copyright (C) 2021 ownCloud GmbH.
+*
+*   Permission is hereby granted, free of charge, to any person obtaining a copy
+*   of this software and associated documentation files (the "Software"), to deal
+*   in the Software without restriction, including without limitation the rights
+*   to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+*   copies of the Software, and to permit persons to whom the Software is
+*   furnished to do so, subject to the following conditions:
+*
+*   The above copyright notice and this permission notice shall be included in
+*   all copies or substantial portions of the Software.
+*
+*   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+*   EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+*   MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+*   NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+*   BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+*   ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+*   CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+*   THE SOFTWARE.
+*
+*/
+
+package eu.opencloud.android.lib
+
+import android.os.Build
+import eu.opencloud.android.lib.common.http.methods.nonwebdav.GetMethod
+import eu.opencloud.android.lib.common.operations.RemoteOperationResult
+import eu.opencloud.android.lib.resources.status.StatusRequester
+import okhttp3.MediaType.Companion.toMediaType
+import okhttp3.Protocol
+import okhttp3.Request
+import okhttp3.Response
+import okhttp3.ResponseBody.Companion.toResponseBody
+import org.junit.Assert.assertEquals
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+import java.net.URL
+
+/**
+ * The status endpoint is the first thing hit when adding or re-authenticating an account, so the
+ * failure it reports is the failure the user sees on the login screen. It used to parse the body as
+ * JSON before looking at the status code, which turned every non-JSON error response into a bogus
+ * "malformed server configuration".
+ */
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [Build.VERSION_CODES.O], manifest = Config.NONE)
+class StatusRequesterHandleResultTest {
+
+    private val requester = StatusRequester()
+
+    @Test
+    fun `handle request result - ko - forbidden with an html body reports the http error`() {
+        val result = requester.handleRequestResult(requestResult(403, CLOUDFLARE_MTLS_ERROR_PAGE, HTML), BASE_URL)
+
+        assertEquals(RemoteOperationResult.ResultCode.FORBIDDEN, result.code)
+        assertEquals(403, result.httpCode)
+    }
+
+    @Test
+    fun `handle request result - ko - bad request with an nginx no-certificate body reports the http error`() {
+        val result = requester.handleRequestResult(requestResult(400, NGINX_MTLS_ERROR_PAGE, HTML), BASE_URL)
+
+        assertEquals(RemoteOperationResult.ResultCode.UNHANDLED_HTTP_CODE, result.code)
+        assertEquals(400, result.httpCode)
+    }
+
+    @Test
+    fun `handle request result - ko - bad gateway with an html body reports the http error`() {
+        val result = requester.handleRequestResult(requestResult(502, "<html><body>Bad gateway</body></html>", HTML), BASE_URL)
+
+        assertEquals(RemoteOperationResult.ResultCode.UNHANDLED_HTTP_CODE, result.code)
+        assertEquals(502, result.httpCode)
+    }
+
+    @Test
+    fun `handle request result - ko - unauthorized with an empty body reports the http error`() {
+        val result = requester.handleRequestResult(requestResult(401, "", HTML), BASE_URL)
+
+        assertEquals(RemoteOperationResult.ResultCode.UNAUTHORIZED, result.code)
+    }
+
+    @Test
+    fun `handle request result - ko - not installed`() {
+        val body = """{"installed":false,"version":"10.0.0.0","productversion":"1.0.0"}"""
+
+        val result = requester.handleRequestResult(requestResult(200, body, JSON), BASE_URL)
+
+        assertEquals(RemoteOperationResult.ResultCode.INSTANCE_NOT_CONFIGURED, result.code)
+    }
+
+    @Test
+    fun `handle request result - ok - installed over https`() {
+        val body = """{"installed":true,"version":"10.0.0.0","productversion":"1.0.0"}"""
+
+        val result = requester.handleRequestResult(requestResult(200, body, JSON), BASE_URL)
+
+        assertEquals(RemoteOperationResult.ResultCode.OK_SSL, result.code)
+        assertEquals(BASE_URL, result.data.baseUrl)
+    }
+
+    private fun requestResult(code: Int, body: String, contentType: String): StatusRequester.RequestResult {
+        val url = URL(STATUS_URL)
+        val response = Response.Builder()
+            .request(Request.Builder().url(url).build())
+            .protocol(Protocol.HTTP_1_1)
+            .code(code)
+            .message("")
+            .body(body.toResponseBody(contentType.toMediaType()))
+            .build()
+        val getMethod = GetMethod(url).apply { this.response = response }
+        return StatusRequester.RequestResult(getMethod, code, STATUS_URL)
+    }
+
+    companion object {
+        private const val BASE_URL = "https://cloud.somewhere.com"
+        private const val STATUS_URL = "$BASE_URL/status.php"
+        private const val HTML = "text/html"
+        private const val JSON = "application/json"
+
+        /** What Cloudflare returns when the client certificate is missing on an mTLS-protected host. */
+        private const val CLOUDFLARE_MTLS_ERROR_PAGE =
+            "<html><head><title>403 Forbidden</title></head><body>No required SSL certificate was sent</body></html>"
+
+        /** What nginx returns in the same situation, verbatim from client.badssl.com. Note the unclosed <hr>. */
+        private const val NGINX_MTLS_ERROR_PAGE =
+            "<html>\n<head><title>400 No required SSL certificate was sent</title></head>\n" +
+                    "<body bgcolor=\"white\">\n<center><h1>400 Bad Request</h1></center>\n" +
+                    "<center>No required SSL certificate was sent</center>\n" +
+                    "<hr><center>nginx/1.10.3 (Ubuntu)</center>\n</body>\n</html>"
+    }
+}

opencloudData/src/main/java/eu/opencloud/android/data/RemoteOperationHandler.kt

@@ -109,7 +109,7 @@ private fun <T> handleRemoteOperationResult(
         RemoteOperationResult.ResultCode.ACCOUNT_NOT_NEW -> throw AccountNotNewException()
         RemoteOperationResult.ResultCode.ACCOUNT_NOT_THE_SAME -> throw AccountNotTheSameException()
         RemoteOperationResult.ResultCode.OK_REDIRECT_TO_NON_SECURE_CONNECTION -> throw RedirectToNonSecureException()
-        RemoteOperationResult.ResultCode.UNHANDLED_HTTP_CODE -> throw UnhandledHttpCodeException()
+        RemoteOperationResult.ResultCode.UNHANDLED_HTTP_CODE -> throw UnhandledHttpCodeException(remoteOperationResult.httpCode)
         RemoteOperationResult.ResultCode.UNKNOWN_ERROR -> throw UnknownErrorException()
         RemoteOperationResult.ResultCode.CANCELLED -> throw CancelledException()
         RemoteOperationResult.ResultCode.INVALID_LOCAL_FILE_NAME -> throw InvalidLocalFileNameException()

opencloudDomain/src/main/java/eu/opencloud/android/domain/exceptions/UnhandledHttpCodeException.kt

@@ -21,4 +21,10 @@ package eu.opencloud.android.domain.exceptions
 
 import java.lang.Exception
 
-class UnhandledHttpCodeException : Exception()
+/**
+ * An HTTP status code we have no specific handling for. [httpCode] is 0 when the status is unknown.
+ *
+ * Note: the message is deliberately left null. Throwable.parseError() returns the message verbatim when
+ * there is one, which would show the raw server phrase to the user.
+ */
+class UnhandledHttpCodeException(val httpCode: Int = 0) : Exception()