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" + } +}