From 590d5b5f23be1869be69010ddb71080cbf8fece7 Mon Sep 17 00:00:00 2001 From: Paolo Stivanin Date: Tue, 4 Aug 2026 15:25:05 +0200 Subject: [PATCH] Login: Keep the mTLS certificate when handling the OAuth redirect If Android kills the app process while the OAuth Custom Tab is in the foreground, the browser redirect is handled by a brand new LoginActivity (isTaskRoot == true). That intent carries no extras, so loginAction fell back to ACTION_CREATE and userAccount was null. restoreClientCertAlias() ran before restoreAuthState(), took the "fresh login" branch and reset clientManager.loginClientCertAlias to null, so the recovery /status.php request went out with no client certificate. On an mTLS-protected host (for instance behind Cloudflare) that comes back as an HTML 403. StatusRequester parsed the body as JSON before looking at the status code, so the JSONException was mapped to INSTANCE_NOT_CONFIGURED and the login screen reported "Malformed server configuration" instead of the real HTTP error. The screen was also left unrecoverable: the dead, single-use authorization code stayed armed, the auth state was never cleared and the url field stayed empty. Logging in fresh from there then wrote a null KEY_MTLS_CERT_ALIAS onto the account, breaking every later connection and leaving a reinstall as the only way out. - Restore the persisted auth state at the top of onCreate on the redirect leg, before anything downstream reads loginAction or userAccount. - Key restoreClientCertAlias() on userAccount instead of loginAction. - Only overwrite the stored alias on login when the user actually picked or removed a certificate on this screen. - On a failed server check during the redirect leg, drop the dead authorization code, clear the auth state and refill the url field. - Check the HTTP status before parsing the status body as JSON, and stop dereferencing the (success-only) data of a failed result. - Report a 403 during login as a possible client certificate problem rather than the generic "Permission error". --- .../authentication/LoginActivity.kt | 97 ++++++++++++-- opencloudApp/src/main/res/values/strings.xml | 1 + .../status/GetRemoteStatusOperation.kt | 4 +- .../lib/resources/status/StatusRequester.kt | 10 +- .../lib/StatusRequesterHandleResultTest.kt | 120 ++++++++++++++++++ 5 files changed, 214 insertions(+), 18 deletions(-) create mode 100644 opencloudComLibrary/src/test/java/eu/opencloud/android/lib/StatusRequesterHandleResultTest.kt diff --git a/opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/LoginActivity.kt b/opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/LoginActivity.kt index 1569a098e1..a1e97b67d3 100644 --- a/opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/LoginActivity.kt +++ b/opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/LoginActivity.kt @@ -60,11 +60,13 @@ import eu.opencloud.android.databinding.AccountSetupBinding import eu.opencloud.android.domain.authentication.oauth.model.ClientRegistrationInfo import eu.opencloud.android.domain.authentication.oauth.model.ResponseType import eu.opencloud.android.domain.authentication.oauth.model.TokenRequest +import eu.opencloud.android.domain.exceptions.ForbiddenException import eu.opencloud.android.domain.exceptions.NoNetworkConnectionException import eu.opencloud.android.domain.exceptions.OpencloudVersionNotSupportedException import eu.opencloud.android.domain.exceptions.SSLErrorCode import eu.opencloud.android.domain.exceptions.SSLErrorException import eu.opencloud.android.domain.exceptions.ServerNotReachableException +import eu.opencloud.android.domain.exceptions.SpecificForbiddenException import eu.opencloud.android.domain.exceptions.UnauthorizedException import eu.opencloud.android.domain.server.model.ServerInfo import eu.opencloud.android.extensions.checkPasscodeEnforced @@ -112,6 +114,7 @@ private const val KEY_AUTH_OIDC_SUPPORTED = "KEY_AUTH_OIDC_SUPPORTED" private const val KEY_AUTH_LOGIN_ACTION = "KEY_AUTH_LOGIN_ACTION" private const val KEY_AUTH_USER_ACCOUNT = "KEY_AUTH_USER_ACCOUNT" private const val KEY_AUTH_MTLS_CERT_ALIAS = "KEY_AUTH_MTLS_CERT_ALIAS" +private const val KEY_AUTH_MTLS_CERT_ALIAS_CHANGED = "KEY_AUTH_MTLS_CERT_ALIAS_CHANGED" // KeyChain.choosePrivateKeyAlias: -1 means no port constraint on the host hint. private const val KEYCHAIN_NO_PORT = -1 @@ -130,6 +133,9 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted // Alias (from the Android KeyChain) of the client certificate to present for mTLS during login. private var clientCertAlias: String? = null + /** True once the user picked or removed a certificate on this screen, so null means "removed". */ + private var clientCertAliasChangedByUser = false + private var loginAction: Byte = ACTION_CREATE private var authTokenType: String? = null private var userAccount: Account? = null @@ -166,8 +172,18 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted authTokenType = intent.getStringExtra(KEY_AUTH_TOKEN_TYPE) userAccount = intent.getParcelableExtra(EXTRA_ACCOUNT) + // The OAuth redirect intent comes from the browser and carries no extras, so loginAction and + // userAccount read above are wrong on the instance that handles it: they default to "create a + // new account". Restore the persisted auth state here, before anything downstream reads them, + // in particular restoreClientCertAlias() — otherwise the mTLS certificate bound to the account + // is never loaded and the /status.php request below goes out without a client certificate. + val isOAuthRedirect = isOAuthRedirectIntent(intent) + if (isOAuthRedirect && savedInstanceState == null) { + restoreAuthState() + } + // Get values from savedInstanceState - if (savedInstanceState == null && authTokenType == null && userAccount != null) { + if (savedInstanceState == null && authTokenType == null && userAccount != null && !isOAuthRedirect) { authenticationViewModel.supportsOAuth2((userAccount as Account).name) } else if (savedInstanceState != null) { authTokenType = savedInstanceState.getString(KEY_AUTH_TOKEN_TYPE) @@ -202,7 +218,15 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted } if (savedInstanceState == null) { - if (userAccount != null) { + if (isOAuthRedirect) { + // serverBaseUrl already came from the restored auth state. Deliberately skip + // getBaseUrl(): its observer calls checkOcServer(), which would race with the + // redirect handling at the end of onCreate. Fill the url field so the screen stays + // usable if the flow below fails. + if (::serverBaseUrl.isInitialized) { + binding.hostUrlInput.setText(serverBaseUrl) + } + } else if (userAccount != null) { authenticationViewModel.getBaseUrl((userAccount as Account).name) } else { serverBaseUrl = getString(R.string.server_url).trim() @@ -259,12 +283,19 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted initLiveDataObservers() - if (intent.data != null && (intent.data?.getQueryParameter("code") != null || intent.data?.getQueryParameter("error") != null)) { - if (savedInstanceState == null) { - restoreAuthState() - } - if (authenticationViewModel.serverInfo.value?.peekContent()?.getStoredData() == null - && ::serverBaseUrl.isInitialized && serverBaseUrl.isNotEmpty()) { + if (isOAuthRedirect) { + val haveServerBaseUrl = ::serverBaseUrl.isInitialized && serverBaseUrl.isNotEmpty() + if (!haveServerBaseUrl) { + // No persisted auth state, so there is no server to exchange the code against. + // Fail visibly instead of crashing later on an uninitialized serverBaseUrl. + Timber.e("OAuth redirect received but no server base url was persisted") + clearAuthState() + binding.serverStatusText.run { + text = getString(R.string.auth_oauth_error) + setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0) + isVisible = true + } + } else if (authenticationViewModel.serverInfo.value?.peekContent()?.getStoredData() == null) { // Process death: serverInfo is gone. Re-fetch it before processing the OAuth response. // Store the intent as pending — getServerInfoIsSuccess will process it via checkServerType bypass. pendingAuthorizationIntent = intent @@ -285,17 +316,26 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted * account exists. */ private fun restoreClientCertAlias(savedInstanceState: Bundle?) { + clientCertAliasChangedByUser = + savedInstanceState?.getBoolean(KEY_AUTH_MTLS_CERT_ALIAS_CHANGED) ?: false clientCertAlias = when { savedInstanceState != null -> savedInstanceState.getString(KEY_AUTH_MTLS_CERT_ALIAS) - loginAction != ACTION_CREATE -> userAccount?.let { + // Keyed on userAccount rather than loginAction: on the OAuth redirect leg the account is + // recovered from the persisted auth state, and keying on loginAction there used to drop + // the certificate. A fresh login has no account and correctly resolves to null. + else -> userAccount?.let { AccountManager.get(this).getUserData(it, AccountUtils.Constants.KEY_MTLS_CERT_ALIAS) } - else -> null } clientManager.loginClientCertAlias = clientCertAlias updateClientCertStatus() } + /** An OAuth redirect is an intent whose data carries either an authorization code or an error. */ + private fun isOAuthRedirectIntent(intent: Intent): Boolean = + intent.data != null && + (intent.data?.getQueryParameter("code") != null || intent.data?.getQueryParameter("error") != null) + /** * If this onCreate is an OAuth redirect, either forward it to the existing instance * (when not task root) or let it proceed. Otherwise, track this instance so the @@ -303,8 +343,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted * @return true if onCreate should return early (redirect was forwarded). */ private fun handleOAuthRedirectOnCreate(): Boolean { - val hasOAuthData = intent.data != null && - (intent.data?.getQueryParameter("code") != null || intent.data?.getQueryParameter("error") != null) + val hasOAuthData = isOAuthRedirectIntent(intent) if (hasOAuthData) { Timber.d("OAuth redirect detected with code or error parameter") @@ -609,6 +648,21 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted private fun getServerInfoIsError(uiResult: UIResult.Error) { updateCenteredRefreshButtonVisibility(shouldBeVisible = true) + + // Failing here on the OAuth return leg strands the flow. The authorization code is single-use + // and short-lived, so it is already dead: drop it instead of letting checkServerType() replay + // it on the next attempt (which would fail with a misleading authorization error), and drop + // the persisted auth state so a retry starts a clean browser round trip. + if (pendingAuthorizationIntent != null) { + Timber.w("Server check failed while handling the OAuth redirect; discarding the authorization code") + pendingAuthorizationIntent = null + clearAuthState() + } + // Keep the screen recoverable: the redirect instance may have an empty url field. + if (binding.hostUrlInput.text.isNullOrBlank() && ::serverBaseUrl.isInitialized && serverBaseUrl.isNotEmpty()) { + binding.hostUrlInput.setText(serverBaseUrl) + } + when { uiResult.error is CertificateCombinedException -> showUntrustedCertDialog(uiResult.error) @@ -628,6 +682,13 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0) } + // A 403 on the status endpoint during login is nearly always a rejected or missing client + // certificate (the generic "Permission error" wording is useless here). + uiResult.error is ForbiddenException || uiResult.error is SpecificForbiddenException -> binding.serverStatusText.run { + text = getString(R.string.auth_forbidden_check_client_cert) + setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0) + } + else -> binding.serverStatusText.run { text = uiResult.error?.parseError("", resources, true) setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0) @@ -665,8 +726,13 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted } // Persist the mTLS client certificate chosen during login to the account, then clear the - // login-time transient so it does not leak into later anonymous clients. - am.setUserData(account, AccountUtils.Constants.KEY_MTLS_CERT_ALIAS, clientCertAlias?.takeIf { it.isNotBlank() }) + // login-time transient so it does not leak into later anonymous clients. Only write when this + // screen actually holds a certificate or the user explicitly changed it: a re-login instance + // that failed to resolve the account's alias must not silently wipe it, which would break + // every subsequent connection and leave reinstalling as the only way out. + if (clientCertAlias != null || clientCertAliasChangedByUser || loginAction == ACTION_CREATE) { + am.setUserData(account, AccountUtils.Constants.KEY_MTLS_CERT_ALIAS, clientCertAlias?.takeIf { it.isNotBlank() }) + } clientManager.loginClientCertAlias = null authenticationViewModel.discoverAccount(accountName = accountName, discoveryNeeded = loginAction == ACTION_CREATE) @@ -1144,6 +1210,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted // Null = user cancelled the picker; keep the current selection. if (alias == null) return clientCertAlias = alias + clientCertAliasChangedByUser = true clientManager.loginClientCertAlias = alias updateClientCertStatus() } @@ -1193,6 +1260,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted private fun clearClientCert() { clientCertAlias = null + clientCertAliasChangedByUser = true clientManager.loginClientCertAlias = null updateClientCertStatus() } @@ -1208,6 +1276,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted outState.putString(KEY_CODE_CHALLENGE, authenticationViewModel.codeChallenge) outState.putString(KEY_OIDC_STATE, authenticationViewModel.oidcState) outState.putString(KEY_AUTH_MTLS_CERT_ALIAS, clientCertAlias) + outState.putBoolean(KEY_AUTH_MTLS_CERT_ALIAS_CHANGED, clientCertAliasChangedByUser) } override fun finish() { diff --git a/opencloudApp/src/main/res/values/strings.xml b/opencloudApp/src/main/res/values/strings.xml index 55c256587f..6f97a918a9 100644 --- a/opencloudApp/src/main/res/values/strings.xml +++ b/opencloudApp/src/main/res/values/strings.xml @@ -377,6 +377,7 @@ Please enter the current password Connecting to authentication server … The server does not support this authentication method + Server refused the connection. If it requires a client certificate, check the one selected under Connection. Your server is not returning a correct user ID. Please contact an administrator. Cannot authenticate to this server Account does not exist in the device yet diff --git a/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/GetRemoteStatusOperation.kt b/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/GetRemoteStatusOperation.kt index 8d5add1ed8..75c641f4fb 100644 --- a/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/GetRemoteStatusOperation.kt +++ b/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/GetRemoteStatusOperation.kt @@ -61,7 +61,9 @@ class GetRemoteStatusOperation : RemoteOperation() { 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") diff --git a/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/StatusRequester.kt b/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/StatusRequester.kt index 8c461cb883..a72f1c74e4 100644 --- a/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/StatusRequester.kt +++ b/opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/status/StatusRequester.kt @@ -91,10 +91,14 @@ internal class StatusRequester { requestResult: RequestResult, baseUrl: String ): RemoteOperationResult { + // 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)) diff --git a/opencloudComLibrary/src/test/java/eu/opencloud/android/lib/StatusRequesterHandleResultTest.kt b/opencloudComLibrary/src/test/java/eu/opencloud/android/lib/StatusRequesterHandleResultTest.kt new file mode 100644 index 0000000000..c11a2655dc --- /dev/null +++ b/opencloudComLibrary/src/test/java/eu/opencloud/android/lib/StatusRequesterHandleResultTest.kt @@ -0,0 +1,120 @@ +/* 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 gateway with an html body reports the http error`() { + val result = requester.handleRequestResult(requestResult(502, "Bad gateway", 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 = + "403 ForbiddenNo required SSL certificate was sent" + } +}