From 7c77c267f9e2258d4f5be4d5f2f93b83ec31b6ee Mon Sep 17 00:00:00 2001 From: Christian Schabesberger Date: Wed, 2 Mar 2022 16:09:04 +0100 Subject: [PATCH] apply changes according to review --- owncloudComLibrary/build.gradle | 2 -- .../android/lib/common/ConnectionValidator.kt | 3 +-- .../android/lib/common/OwnCloudClient.java | 24 ++++++++----------- .../lib/common/http/methods/HttpBaseMethod.kt | 2 +- .../operations/RemoteOperationResult.java | 6 +++-- .../files/GetBaseUrlRemoteOperation.kt | 17 ++++++++----- .../lib/resources/status/StatusRequester.kt | 16 ++++--------- 7 files changed, 32 insertions(+), 38 deletions(-) diff --git a/owncloudComLibrary/build.gradle b/owncloudComLibrary/build.gradle index c98d293f..e9f20869 100644 --- a/owncloudComLibrary/build.gradle +++ b/owncloudComLibrary/build.gradle @@ -7,8 +7,6 @@ dependencies { implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion" api 'com.gitlab.ownclouders:dav4android:oc_support_2.1.5' api 'com.github.AppDevNext.Logcat:LogcatCore:2.2.2' - debugImplementation 'com.facebook.stetho:stetho:1.5.1' - debugImplementation 'com.facebook.stetho:stetho-okhttp3:1.5.1' // Moshi implementation("com.squareup.moshi:moshi-kotlin:$moshiVersion") { diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/ConnectionValidator.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/ConnectionValidator.kt index a793b4fd..d5f2aabd 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/ConnectionValidator.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/ConnectionValidator.kt @@ -19,7 +19,6 @@ class ConnectionValidator( val context: Context, val clearCookiesOnValidation: Boolean ) { - fun validate(baseClient: OwnCloudClient, singleSessionManager: SingleSessionManager): Boolean { try { var validationRetryCount = 0 @@ -182,6 +181,6 @@ class ConnectionValidator( } companion object { - val VALIDATION_RETRY_COUNT = 3 + private val VALIDATION_RETRY_COUNT = 3 } } diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/OwnCloudClient.java b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/OwnCloudClient.java index 7aae9c28..09b255e2 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/OwnCloudClient.java +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/OwnCloudClient.java @@ -27,7 +27,6 @@ package com.owncloud.android.lib.common; import android.net.Uri; -import at.bitfire.dav4jvm.exception.HttpException; import com.owncloud.android.lib.common.accounts.AccountUtils; import com.owncloud.android.lib.common.authentication.OwnCloudCredentials; import com.owncloud.android.lib.common.authentication.OwnCloudCredentialsFactory; @@ -35,12 +34,10 @@ import com.owncloud.android.lib.common.authentication.OwnCloudCredentialsFactory import com.owncloud.android.lib.common.http.HttpClient; import com.owncloud.android.lib.common.http.HttpConstants; import com.owncloud.android.lib.common.http.methods.HttpBaseMethod; -import com.owncloud.android.lib.common.network.RedirectionPath; import com.owncloud.android.lib.common.utils.RandomUtils; import com.owncloud.android.lib.resources.status.OwnCloudVersion; import okhttp3.Cookie; import okhttp3.HttpUrl; -import org.apache.commons.lang3.exception.ExceptionUtils; import timber.log.Timber; import java.io.IOException; @@ -49,7 +46,6 @@ import java.util.List; import static com.owncloud.android.lib.common.http.HttpConstants.AUTHORIZATION_HEADER; import static com.owncloud.android.lib.common.http.HttpConstants.HTTP_MOVED_PERMANENTLY; -import static com.owncloud.android.lib.common.http.HttpConstants.OC_X_REQUEST_ID; public class OwnCloudClient extends HttpClient { @@ -137,12 +133,7 @@ public class OwnCloudClient extends HttpClient { status = method.execute(); - if (!mFollowRedirects && - !method.getFollowRedirects() && - mConnectionValidator != null && - (status == HttpConstants.HTTP_MOVED_TEMPORARILY || - (!(mCredentials instanceof OwnCloudAnonymousCredentials) && - status == HttpConstants.HTTP_UNAUTHORIZED))) { + if (shouldConnectionValidatorBeCalled(method, status)) { retry = mConnectionValidator.validate(this, mSingleSessionManager); // retry on success fail on no success } else if(method.getFollowPermanentRedirects() && status == HTTP_MOVED_PERMANENTLY) { retry = true; @@ -154,6 +145,15 @@ public class OwnCloudClient extends HttpClient { return status; } + private boolean shouldConnectionValidatorBeCalled(HttpBaseMethod method, int status) { + return !mFollowRedirects && + !method.getFollowRedirects() && + mConnectionValidator != null && + (status == HttpConstants.HTTP_MOVED_TEMPORARILY || + (!(mCredentials instanceof OwnCloudAnonymousCredentials) && + status == HttpConstants.HTTP_UNAUTHORIZED)); + } + /** * Exhausts a not interesting HTTP response. Encouraged by HttpClient documentation. * @@ -250,10 +250,6 @@ public class OwnCloudClient extends HttpClient { this.mAccount = account; } - public boolean getFollowRedirects() { - return mFollowRedirects; - } - public void setFollowRedirects(boolean followRedirects) { this.mFollowRedirects = followRedirects; } 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 bc8fe1ce..ee1d2c0e 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 @@ -166,7 +166,7 @@ abstract class HttpBaseMethod constructor(url: URL) { open fun getFollowRedirects() = okHttpClient.followRedirects - open fun setFollPermanentRedirects(followRedirects: Boolean) { + open fun setFollowPermanentRedirects(followRedirects: Boolean) { _followPermanentRedirects = followRedirects } diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/operations/RemoteOperationResult.java b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/operations/RemoteOperationResult.java index a30a43a5..7430771d 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/operations/RemoteOperationResult.java +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/operations/RemoteOperationResult.java @@ -60,6 +60,8 @@ public class RemoteOperationResult * Generated - should be refreshed every time the class changes!! */ private static final long serialVersionUID = 4968939884332372230L; + private static final String LOCATION = "location"; + private static final String WWW_AUTHENTICATE = "www-authenticate"; private boolean mSuccess = false; private int mHttpCode = -1; @@ -257,11 +259,11 @@ public class RemoteOperationResult this(httpCode, httpPhrase); if (headers != null) { for (Map.Entry> header : headers.toMultimap().entrySet()) { - if ("location".equalsIgnoreCase(header.getKey())) { + if (LOCATION.equalsIgnoreCase(header.getKey())) { mRedirectedLocation = header.getValue().get(0); continue; } - if ("www-authenticate".equalsIgnoreCase(header.getKey())) { + if (WWW_AUTHENTICATE.equalsIgnoreCase(header.getKey())) { for (String value: header.getValue()) { mAuthenticate.add(value.toLowerCase()); } diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/GetBaseUrlRemoteOperation.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/GetBaseUrlRemoteOperation.kt index b0b3865c..904a9bea 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/GetBaseUrlRemoteOperation.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/files/GetBaseUrlRemoteOperation.kt @@ -45,16 +45,21 @@ class GetBaseUrlRemoteOperation : RemoteOperation() { val stringUrl = client.baseFilesWebDavUri.toString() val propFindMethod = PropfindMethod(URL(stringUrl), 0, DavUtils.allPropset).apply { - setReadTimeout(TIMEOUT.toLong(), TimeUnit.SECONDS) - setConnectionTimeout(TIMEOUT.toLong(), TimeUnit.SECONDS) + setReadTimeout(TIMEOUT, TimeUnit.SECONDS) + setConnectionTimeout(TIMEOUT, TimeUnit.SECONDS) } val status = client.executeHttpMethod(propFindMethod) - if (isSuccess(status)) RemoteOperationResult(RemoteOperationResult.ResultCode.OK).apply { - data = propFindMethod.getFinalUrl().toString() + if (isSuccess(status)) { + RemoteOperationResult(RemoteOperationResult.ResultCode.OK).apply { + data = propFindMethod.getFinalUrl().toString() + } + } else { + RemoteOperationResult(propFindMethod).apply { + data = null + } } - else RemoteOperationResult(propFindMethod).apply { data = null } } catch (e: Exception) { Timber.e(e, "Could not get actuall (or redirected) base URL from base url (/).") RemoteOperationResult(e) @@ -67,6 +72,6 @@ class GetBaseUrlRemoteOperation : RemoteOperation() { /** * Maximum time to wait for a response from the server in milliseconds. */ - private const val TIMEOUT = 10000 + private const val TIMEOUT = 10_000L } } 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 b201a937..55381f90 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 @@ -73,20 +73,18 @@ internal class StatusRequester { data class RequestResult( val getMethod: GetMethod, val status: Int, - val redirectedToUnsecureLocation: Boolean, val lastLocation: String ) fun request(baseLocation: String, client: OwnCloudClient): RequestResult { - var currentLocation = baseLocation + OwnCloudClient.STATUS_PATH - var redirectedToUnsecureLocation = false + val currentLocation = baseLocation + OwnCloudClient.STATUS_PATH var status: Int val getMethod = getGetMethod(currentLocation) - getMethod.setFollPermanentRedirects(true) + getMethod.setFollowPermanentRedirects(true) status = client.executeHttpMethod(getMethod) - return RequestResult(getMethod, status, redirectedToUnsecureLocation, getMethod.getFinalUrl().toString()) + return RequestResult(getMethod, status, getMethod.getFinalUrl().toString()) } private fun Int.isSuccess() = this == HttpConstants.HTTP_OK @@ -106,12 +104,8 @@ internal class StatusRequester { // the version object will be returned even if the version is invalid, no error code; // every app will decide how to act if (ocVersion.isVersionValid() == false) val result: RemoteOperationResult = - if (requestResult.redirectedToUnsecureLocation) { - RemoteOperationResult(RemoteOperationResult.ResultCode.OK_REDIRECT_TO_NON_SECURE_CONNECTION) - } else { - if (baseUrl.startsWith(HTTPS_SCHEME)) RemoteOperationResult(RemoteOperationResult.ResultCode.OK_SSL) - else RemoteOperationResult(RemoteOperationResult.ResultCode.OK_NO_SSL) - } + if (baseUrl.startsWith(HTTPS_SCHEME)) RemoteOperationResult(RemoteOperationResult.ResultCode.OK_SSL) + else RemoteOperationResult(RemoteOperationResult.ResultCode.OK_NO_SSL) val finalUrl = URL(requestResult.lastLocation) val finalBaseUrl = URL( finalUrl.protocol,