From 196018fa6b55ef34c50065f2a3b621bdd867c247 Mon Sep 17 00:00:00 2001 From: Christian Schabesberger Date: Thu, 17 Sep 2020 12:23:45 +0200 Subject: [PATCH] fix according to review --- .../status/GetRemoteStatusOperation.kt | 15 ++--- .../lib/resources/status/HttpScheme.kt | 30 +++++++++ ...{StatusRequestor.kt => StatusRequester.kt} | 62 +++++++++++++------ .../android/lib/StatusRequestorTest.kt | 57 ++++++++++++----- 4 files changed, 119 insertions(+), 45 deletions(-) create mode 100644 owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/HttpScheme.kt rename owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/{StatusRequestor.kt => StatusRequester.kt} (71%) 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 e9c1c10b..ba3b4311 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 @@ -28,10 +28,11 @@ import com.owncloud.android.lib.common.OwnCloudClient import com.owncloud.android.lib.common.operations.RemoteOperation import com.owncloud.android.lib.common.operations.RemoteOperationResult import com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode +import com.owncloud.android.lib.resources.status.HttpScheme.HTTPS_SCHEME +import com.owncloud.android.lib.resources.status.HttpScheme.HTTP_SCHEME import org.json.JSONException import timber.log.Timber - /** * Checks if the server is valid * @@ -41,14 +42,10 @@ import timber.log.Timber * @author Abel García de Prada */ class GetRemoteStatusOperation : RemoteOperation() { - companion object { - const val HTTPS_SCHEME = "https" - const val HTTP_SCHEME = "http" - } override fun run(client: OwnCloudClient): RemoteOperationResult { if (client.baseUri.scheme.isNullOrEmpty()) - client.baseUri = Uri.parse(HTTPS_SCHEME + "://" + client.baseUri.toString()) + client.baseUri = Uri.parse("$HTTPS_SCHEME://${client.baseUri}") var result = tryToConnect(client) if (result.code != ResultCode.OK_SSL && !result.isSslRecoverableException) { @@ -64,9 +61,9 @@ class GetRemoteStatusOperation : RemoteOperation() { val baseUrl = client.baseUri.toString() client.setFollowRedirects(false) return try { - val requestor = StatusRequestor() - val requestResult = requestor.requestAndFollowRedirects(baseUrl, client) - requestor.handleRequestResult(requestResult, baseUrl) + val requester = StatusRequester() + val requestResult = requester.requestAndFollowRedirects(baseUrl, client) + requester.handleRequestResult(requestResult, baseUrl) } catch (e: JSONException) { RemoteOperationResult(ResultCode.INSTANCE_NOT_CONFIGURED) } catch (e: Exception) { diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/HttpScheme.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/HttpScheme.kt new file mode 100644 index 00000000..091ce146 --- /dev/null +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/HttpScheme.kt @@ -0,0 +1,30 @@ +/* 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.resources.status + +object HttpScheme { + const val HTTP_SCHEME = "http" + const val HTTPS_SCHEME = "https" +} diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequestor.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt similarity index 71% rename from owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequestor.kt rename to owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt index c36c3426..14af1cee 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequestor.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt @@ -1,26 +1,40 @@ +/* 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.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.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 StatusRequestor { - - companion object { - /** - * Maximum time to wait for a response from the server when the connection is being tested, - * in MILLISECONDs. - */ - private const val TRY_CONNECTION_TIMEOUT: Long = 5000 - private const val NODE_INSTALLED = "installed" - private const val HTTPS_SCHEME = "https" - private const val HTTP_SCHEME = "http" - private const val NODE_VERSION = "version" - } +internal class StatusRequester { private fun checkIfConnectionIsRedirectedToNoneSecure( isConnectionSecure: Boolean, @@ -28,9 +42,7 @@ internal class StatusRequestor { redirectedUrl: String ): Boolean { return isConnectionSecure || - (baseUrl.startsWith(HTTPS_SCHEME) && redirectedUrl.startsWith( - HTTP_SCHEME - )) + (baseUrl.startsWith(HTTPS_SCHEME) && redirectedUrl.startsWith(HTTP_SCHEME)) } fun updateLocationWithRedirectPath(oldLocation: String, redirectedLocation: String): String { @@ -64,7 +76,7 @@ internal class StatusRequestor { status = client.executeHttpMethod(getMethod) val result = - if (isSuccess(status)) RemoteOperationResult(RemoteOperationResult.ResultCode.OK) + if (status.isSuccess()) RemoteOperationResult(RemoteOperationResult.ResultCode.OK) else RemoteOperationResult(getMethod) if (result.redirectedLocation.isNullOrEmpty() || result.isSuccess) { @@ -82,13 +94,13 @@ internal class StatusRequestor { } } - private fun isSuccess(status: Int): Boolean = status == HttpConstants.HTTP_OK + private fun Int.isSuccess() = this == HttpConstants.HTTP_OK fun handleRequestResult( requestResult: RequestResult, baseUrl: String ): RemoteOperationResult { - if (!isSuccess(requestResult.status)) + if (!requestResult.status.isSuccess()) return RemoteOperationResult(requestResult.getMethod) val respJSON = JSONObject(requestResult.getMethod.getResponseBodyAsString() ?: "") @@ -110,4 +122,14 @@ internal class StatusRequestor { result.data = ocVersion return result } -} \ No newline at end of file + + companion object { + /** + * Maximum time to wait for a response from the server when the connection is being tested, + * in MILLISECONDs. + */ + private const val TRY_CONNECTION_TIMEOUT: Long = 5000 + private const val NODE_INSTALLED = "installed" + private const val NODE_VERSION = "version" + } +} diff --git a/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt b/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt index f1ca501b..107b5238 100644 --- a/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt +++ b/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt @@ -1,42 +1,67 @@ +/* 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.StatusRequestor +import com.owncloud.android.lib.resources.status.StatusRequester import org.junit.Assert.assertEquals import org.junit.Test class StatusRequestorTest { - private val requestor = StatusRequestor() + private val requestor = StatusRequester() @Test fun `update location with an absolute path`() { - val newLocation = requestor.updateLocationWithRedirectPath( - "https://cloud.somewhere.com", "https://cloud.somewhere.com/subdir" - ) - assertEquals("https://cloud.somewhere.com/subdir", newLocation) + val newLocation = requestor.updateLocationWithRedirectPath(TEST_DOMAIN, "$TEST_DOMAIN/subdir") + assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) } @Test - fun `update location with a smaler aboslute path`() { - val newLocation = requestor.updateLocationWithRedirectPath( - "https://cloud.somewhere.com/subdir", "https://cloud.somewhere.com/" - ) - assertEquals("https://cloud.somewhere.com/", newLocation) + 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( - "https://cloud.somewhere.com", "/subdir" + TEST_DOMAIN, SUB_PATH ) - assertEquals("https://cloud.somewhere.com/subdir", newLocation) + assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) } @Test fun `update location by replacing the relative path`() { val newLocation = requestor.updateLocationWithRedirectPath( - "https://cloud.somewhere.com/some/other/subdir", "/subdir" + "$TEST_DOMAIN/some/other/subdir", SUB_PATH ) - assertEquals("https://cloud.somewhere.com/subdir", newLocation) + assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) } -} \ No newline at end of file + + companion object { + const val TEST_DOMAIN = "https://cloud.somewhere.com" + const val SUB_PATH = "/subdir" + } +}