From 409e37b9135f63062bad345f07bbf58fff31d5f8 Mon Sep 17 00:00:00 2001 From: Christian Schabesberger Date: Fri, 18 Sep 2020 15:46:03 +0200 Subject: [PATCH] remove redirect code from status requestor --- build.gradle | 1 + owncloudComLibrary/build.gradle | 4 ++ .../android/lib/common/http/HttpClient.java | 15 ++--- .../lib/common/http/methods/HttpBaseMethod.kt | 6 ++ .../http/methods/RedirectChainHandler.kt | 38 +++++++++++ .../CheckPathExistenceRemoteOperation.kt | 3 + .../status/GetRemoteStatusOperation.kt | 11 ++-- .../lib/resources/status/StatusRequester.kt | 49 +++----------- .../android/lib/StatusRequestorTest.kt | 65 ------------------- 9 files changed, 75 insertions(+), 117 deletions(-) create mode 100644 owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/RedirectChainHandler.kt delete mode 100644 owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt diff --git a/build.gradle b/build.gradle index f51df2b2..1a2bf07e 100644 --- a/build.gradle +++ b/build.gradle @@ -13,6 +13,7 @@ buildscript { classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlinVersion" classpath "org.jetbrains.kotlin:kotlin-allopen:$kotlinVersion" } + } allprojects { diff --git a/owncloudComLibrary/build.gradle b/owncloudComLibrary/build.gradle index 0f7261e9..53277d97 100644 --- a/owncloudComLibrary/build.gradle +++ b/owncloudComLibrary/build.gradle @@ -48,4 +48,8 @@ android { sourceCompatibility JavaVersion.VERSION_1_8 targetCompatibility JavaVersion.VERSION_1_8 } + + kotlinOptions { + jvmTarget = JavaVersion.VERSION_1_8.toString() + } } diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/HttpClient.java b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/HttpClient.java index 9c6284c9..7dcd1461 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/HttpClient.java +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/HttpClient.java @@ -52,6 +52,7 @@ import java.util.concurrent.TimeUnit; * Client used to perform network operations * * @author David González Verdugo + * @author Christian Schabesberger */ public class HttpClient { private static OkHttpClient sOkHttpClient; @@ -69,9 +70,7 @@ public class HttpClient { final SSLSocketFactory sslSocketFactory = sslContext.getSocketFactory(); - // Automatic cookie handling, NOT PERSISTENT - final CookieJar cookieJar = getNewCookieJar(); - sOkHttpClient = buildOkHttpClient(cookieJar, sslSocketFactory, trustManager); + sOkHttpClient = buildNewOkHttpClient(sslSocketFactory, trustManager); } catch (Exception e) { Timber.e(e, "Could not setup SSL system."); @@ -110,29 +109,27 @@ public class HttpClient { List nonDuplicatedCookiesList = new ArrayList<>(nonDuplicatedCookiesSet); sCookieStore.put(url.host(), nonDuplicatedCookiesList); - System.out.println("set cookiestore size " + url.toString() + " " + sCookieStore.size()); } @Override public List loadForRequest(HttpUrl url) { - System.out.println("get cookiestore size " + url.toString() + " " + sCookieStore.size()); List cookies = sCookieStore.get(url.host()); return cookies != null ? cookies : new ArrayList<>(); } }; } - private static OkHttpClient buildOkHttpClient(CookieJar cookieJar, SSLSocketFactory sslSocketFactory, - X509TrustManager trustManager) { + private static OkHttpClient buildNewOkHttpClient(SSLSocketFactory sslSocketFactory, + X509TrustManager trustManager) { OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder() .protocols(Arrays.asList(Protocol.HTTP_1_1)) .readTimeout(HttpConstants.DEFAULT_DATA_TIMEOUT, TimeUnit.MILLISECONDS) .writeTimeout(HttpConstants.DEFAULT_DATA_TIMEOUT, TimeUnit.MILLISECONDS) .connectTimeout(HttpConstants.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS) - .followRedirects(false) + .followRedirects(true) .sslSocketFactory(sslSocketFactory, trustManager) .hostnameVerifier((placeholder1, placeholder2) -> true) - .cookieJar(cookieJar); + .cookieJar(getNewCookieJar()); // TODO: Not verifying the hostname against certificate. ask owncloud security human if this is ok. //.hostnameVerifier(new BrowserCompatHostnameVerifier()); return clientBuilder.build(); diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/HttpBaseMethod.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/HttpBaseMethod.kt index 0c4c7ada..35ac7a21 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/HttpBaseMethod.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/HttpBaseMethod.kt @@ -134,6 +134,12 @@ abstract class HttpBaseMethod constructor(url: URL) { .build() } + fun addRedirectChainHandler(redirectChainHandler: RedirectChainHandler) { + okHttpClient = okHttpClient.newBuilder() + .addNetworkInterceptor(redirectChainHandler) + .build() + } + /************ *** Call *** ************/ diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/RedirectChainHandler.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/RedirectChainHandler.kt new file mode 100644 index 00000000..ee996f8c --- /dev/null +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/methods/RedirectChainHandler.kt @@ -0,0 +1,38 @@ +package com.owncloud.android.lib.common.http.methods + +import com.owncloud.android.lib.resources.status.HttpScheme +import com.owncloud.android.lib.resources.status.HttpScheme.HTTPS_SCHEME +import com.owncloud.android.lib.resources.status.HttpScheme.HTTP_SCHEME +import okhttp3.Interceptor +import okhttp3.Response + +class RedirectChainHandler : Interceptor { + private val _redirectChain = arrayListOf() + + val redirectChain: List + get() = _redirectChain + + /** + * Is only true if http and https requests where in the request chain. + */ + val hasBeenRedirectedUnsecureLocation: Boolean + get() { + var containsHttpsRequests = false + var containsHttpRequests = false + for(url in _redirectChain) { + when { + url.startsWith(HTTPS_SCHEME) -> containsHttpsRequests = true + url.startsWith("$HTTP_SCHEME://") -> containsHttpRequests = true + } + + } + return containsHttpRequests && containsHttpsRequests + } + + override fun intercept(chain: Interceptor.Chain): Response { + _redirectChain.add(chain.request().url.toString()) + return chain.proceed(chain.request()) + } +} + + diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/CheckPathExistenceRemoteOperation.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/CheckPathExistenceRemoteOperation.kt index 46e35ae3..ef9f3f5e 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/CheckPathExistenceRemoteOperation.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/CheckPathExistenceRemoteOperation.kt @@ -90,10 +90,13 @@ class CheckPathExistenceRemoteOperation( client.setFollowRedirects(false) var status = client.executeHttpMethod(propFindMethod) + /* if (previousFollowRedirects) { redirectionPath = client.followRedirection(propFindMethod) status = redirectionPath?.lastStatus!! } + + */ handleResult(requestUrl, status, propFindMethod) } catch (e: Exception) { val result = RemoteOperationResult(e) diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/GetRemoteStatusOperation.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/GetRemoteStatusOperation.kt index ba3b4311..5222e8b9 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/GetRemoteStatusOperation.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/GetRemoteStatusOperation.kt @@ -44,11 +44,15 @@ import timber.log.Timber class GetRemoteStatusOperation : RemoteOperation() { override fun run(client: OwnCloudClient): RemoteOperationResult { + //try to connect with ssl if (client.baseUri.scheme.isNullOrEmpty()) client.baseUri = Uri.parse("$HTTPS_SCHEME://${client.baseUri}") - var result = tryToConnect(client) - if (result.code != ResultCode.OK_SSL && !result.isSslRecoverableException) { + + //try to connect without ssl + if (result.code != ResultCode.OK_SSL + && !result.isSslRecoverableException + && !result.isRedirectToNonSecureConnection) { Timber.d("Establishing secure connection failed, trying non secure connection") client.baseUri = client.baseUri.buildUpon().scheme(HTTP_SCHEME).build() result = tryToConnect(client) @@ -59,10 +63,9 @@ class GetRemoteStatusOperation : RemoteOperation() { private fun tryToConnect(client: OwnCloudClient): RemoteOperationResult { val baseUrl = client.baseUri.toString() - client.setFollowRedirects(false) return try { val requester = StatusRequester() - val requestResult = requester.requestAndFollowRedirects(baseUrl, client) + val requestResult = requester.requestStatus(baseUrl, client) requester.handleRequestResult(requestResult, baseUrl) } catch (e: JSONException) { RemoteOperationResult(ResultCode.INSTANCE_NOT_CONFIGURED) diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt index 2dbb3653..e438e56a 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt @@ -26,32 +26,15 @@ package com.owncloud.android.lib.resources.status import com.owncloud.android.lib.common.OwnCloudClient import com.owncloud.android.lib.common.http.HttpConstants +import com.owncloud.android.lib.common.http.methods.RedirectChainHandler import com.owncloud.android.lib.common.http.methods.nonwebdav.GetMethod import com.owncloud.android.lib.common.operations.RemoteOperationResult import com.owncloud.android.lib.resources.status.HttpScheme.HTTPS_SCHEME -import com.owncloud.android.lib.resources.status.HttpScheme.HTTP_SCHEME import org.json.JSONObject import java.net.URL import java.util.concurrent.TimeUnit internal class StatusRequester { - - private fun checkIfConnectionIsRedirectedToNoneSecure( - isConnectionSecure: Boolean, - baseUrl: String, - redirectedUrl: String - ): Boolean { - return isConnectionSecure || - (baseUrl.startsWith(HTTPS_SCHEME) && redirectedUrl.startsWith(HTTP_SCHEME)) - } - - fun updateLocationWithRedirectPath(oldLocation: String, redirectedLocation: String): String { - if (!redirectedLocation.startsWith("/")) - return redirectedLocation - val oldLocationURL = URL(oldLocation) - return URL(oldLocationURL.protocol, oldLocationURL.host, oldLocationURL.port, redirectedLocation).toString() - } - private fun getGetMethod(url: String): GetMethod { return GetMethod(URL(url)).apply { setReadTimeout(TRY_CONNECTION_TIMEOUT, TimeUnit.SECONDS) @@ -66,32 +49,20 @@ internal class StatusRequester { val redirectedToUnsecureLocation: Boolean ) - fun requestAndFollowRedirects(baseLocation: String, client: OwnCloudClient): RequestResult { + fun requestStatus(baseLocation: String, client: OwnCloudClient): RequestResult { var currentLocation = baseLocation + OwnCloudClient.STATUS_PATH - var redirectedToUnsecureLocation = false var status: Int - while (true) { - val getMethod = getGetMethod(currentLocation) + val getMethod = getGetMethod(currentLocation) + val redirectChainHandler = RedirectChainHandler() + getMethod.addRedirectChainHandler(redirectChainHandler) - status = client.executeHttpMethod(getMethod) - val result = - if (status.isSuccess()) RemoteOperationResult(RemoteOperationResult.ResultCode.OK) - else RemoteOperationResult(getMethod) + status = client.executeHttpMethod(getMethod) + val result = + if (status.isSuccess()) RemoteOperationResult(RemoteOperationResult.ResultCode.OK) + else RemoteOperationResult(getMethod) - if (result.redirectedLocation.isNullOrEmpty() || result.isSuccess) { - return RequestResult(getMethod, status, result, redirectedToUnsecureLocation) - } else { - val nextLocation = updateLocationWithRedirectPath(currentLocation, result.redirectedLocation) - redirectedToUnsecureLocation = - checkIfConnectionIsRedirectedToNoneSecure( - redirectedToUnsecureLocation, - currentLocation, - nextLocation - ) - currentLocation = nextLocation - } - } + return RequestResult(getMethod, status, result, redirectChainHandler.hasBeenRedirectedUnsecureLocation) } private fun Int.isSuccess() = this == HttpConstants.HTTP_OK diff --git a/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt b/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt deleted file mode 100644 index 15988187..00000000 --- a/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt +++ /dev/null @@ -1,65 +0,0 @@ -/* ownCloud Android Library is available under MIT license -* Copyright (C) 2020 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 com.owncloud.android.lib - -import com.owncloud.android.lib.resources.status.StatusRequester -import org.junit.Assert.assertEquals -import org.junit.Test - -class StatusRequestorTest { - private val requestor = StatusRequester() - - @Test - fun `update location with an absolute path`() { - val newLocation = requestor.updateLocationWithRedirectPath(TEST_DOMAIN, "$TEST_DOMAIN$SUB_PATH") - assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) - } - - @Test - - fun `update location with a smaller absolute path`() { - val newLocation = requestor.updateLocationWithRedirectPath("$TEST_DOMAIN$SUB_PATH", TEST_DOMAIN) - assertEquals(TEST_DOMAIN, newLocation) - } - - @Test - fun `update location with a relative path`() { - val newLocation = requestor.updateLocationWithRedirectPath(TEST_DOMAIN, SUB_PATH) - assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) - } - - @Test - fun `update location by replacing the relative path`() { - val newLocation = requestor.updateLocationWithRedirectPath( - "$TEST_DOMAIN/some/other/subdir", SUB_PATH - ) - assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) - } - - companion object { - const val TEST_DOMAIN = "https://cloud.somewhere.com" - const val SUB_PATH = "/subdir" - } -}