From 803df964e102cd0b928f56e3eae8fab4ac25683d Mon Sep 17 00:00:00 2001 From: Chris Narkiewicz Date: Mon, 3 Feb 2020 22:28:49 +0000 Subject: [PATCH] Fix exception handling in NextcloudClient Fixes #393 Fixes nextcloud/android#5245 Fixes nextcloud/android#5305 Fixes nextcloud/android#5310 Fixes nextcloud/android#5331 Fixes nextcloud/android#5333 Signed-off-by: Chris Narkiewicz --- build.gradle | 3 +- .../com/nextcloud/common/NextcloudClient.kt | 53 ++++--- .../com/nextcloud/common/OkHttpMethodBase.kt | 23 ++- .../com/nextcloud/common/ResponseOrError.kt | 33 +++++ .../nextcloud/common/NextcloudClientTest.kt | 134 ++++++++++++++++++ .../nextcloud/common/OkHttpMethodBaseTest.kt | 80 +++++++++++ .../org.mockito.plugins.MockMaker | 1 + 7 files changed, 298 insertions(+), 29 deletions(-) create mode 100644 src/main/java/com/nextcloud/common/ResponseOrError.kt create mode 100644 src/test/java/com/nextcloud/common/NextcloudClientTest.kt create mode 100644 src/test/java/com/nextcloud/common/OkHttpMethodBaseTest.kt create mode 100644 src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker diff --git a/build.gradle b/build.gradle index d5e8843820..87585ad260 100644 --- a/build.gradle +++ b/build.gradle @@ -64,7 +64,7 @@ dependencies { compileOnly 'org.projectlombok:lombok:1.18.10' annotationProcessor 'org.projectlombok:lombok:1.18.10' - implementation "androidx.core:core-ktx:1.1.0" + implementation "androidx.core:core-ktx:1.2.0" implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version" spotbugsPlugins 'com.h3xstream.findsecbugs:findsecbugs-plugin:1.10.1' @@ -73,6 +73,7 @@ dependencies { // dependencies for tests testImplementation 'junit:junit:4.13' testImplementation 'org.mockito:mockito-core:3.2.4' + testImplementation 'com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0' // dependencies for instrumented tests // JUnit4 Rules diff --git a/src/main/java/com/nextcloud/common/NextcloudClient.kt b/src/main/java/com/nextcloud/common/NextcloudClient.kt index 4e05190af4..2985a82bd9 100644 --- a/src/main/java/com/nextcloud/common/NextcloudClient.kt +++ b/src/main/java/com/nextcloud/common/NextcloudClient.kt @@ -2,6 +2,7 @@ * * @author Tobias Kaminsky * Copyright (C) 2019 Tobias Kaminsky + * Copyright (C) 2020 Chris Narkiewicz * Copyright (C) 2019 Nextcloud GmbH * * Permission is hereby granted, free of charge, to any person obtaining a copy @@ -38,10 +39,7 @@ import com.owncloud.android.lib.common.network.RedirectionPath import com.owncloud.android.lib.common.operations.RemoteOperation import com.owncloud.android.lib.common.operations.RemoteOperationResult import com.owncloud.android.lib.common.utils.Log_OC -import okhttp3.CookieJar -import okhttp3.OkHttpClient -import okhttp3.Request -import okhttp3.Response +import okhttp3.* import org.apache.commons.httpclient.HttpStatus import java.io.IOException import java.util.concurrent.TimeUnit @@ -50,42 +48,53 @@ import javax.net.ssl.SSLSession import javax.net.ssl.TrustManager -class NextcloudClient(var baseUri: Uri, val context: Context) { +class NextcloudClient(var baseUri: Uri, val context: Context, client: OkHttpClient) { lateinit var credentials: String lateinit var userId: String lateinit var request: Request var followRedirects = true; - val client: OkHttpClient - + val client = client + companion object { @JvmStatic val TAG = NextcloudClient::class.java.simpleName - } - init { - val trustManager = AdvancedX509TrustManager(NetworkUtils.getKnownServersStore(context)) - val sslContext = SSLContext.getInstance("TLSv1") - sslContext.init(null, arrayOf(trustManager), null) - val sslSocketFactory = sslContext.socketFactory + private fun createDefaultClient(context: Context) : OkHttpClient { + val trustManager = AdvancedX509TrustManager(NetworkUtils.getKnownServersStore(context)) + val sslContext = SSLContext.getInstance("TLSv1") + sslContext.init(null, arrayOf(trustManager), null) + val sslSocketFactory = sslContext.socketFactory - client = OkHttpClient.Builder() - .cookieJar(CookieJar.NO_COOKIES) - .callTimeout(DEFAULT_DATA_TIMEOUT_LONG, TimeUnit.MILLISECONDS) - .sslSocketFactory(sslSocketFactory, trustManager) - .hostnameVerifier { asdf: String?, usdf: SSLSession? -> true } - .build() + return OkHttpClient.Builder() + .cookieJar(CookieJar.NO_COOKIES) + .callTimeout(DEFAULT_DATA_TIMEOUT_LONG, TimeUnit.MILLISECONDS) + .sslSocketFactory(sslSocketFactory, trustManager) + .hostnameVerifier { _: String?, _: SSLSession? -> true } + .build() + } } + + constructor(baseUri: Uri, context: Context) : this(baseUri, context, createDefaultClient(context)) fun execute(remoteOperation: RemoteOperation): RemoteOperationResult { - return remoteOperation.run(this) + return try { + remoteOperation.run(this) + } catch (ex: Exception) { + RemoteOperationResult(ex) + } } fun execute(method: OkHttpMethodBase): Int { return method.execute(this) } - fun execute(request: Request): Response { - return client.newCall(request).execute() + internal fun execute(request: Request): ResponseOrError { + return try { + val response = client.newCall(request).execute() + ResponseOrError(response) + } catch (ex: IOException) { + ResponseOrError(ex) + } } fun getRequestHeader(name: String): String? { diff --git a/src/main/java/com/nextcloud/common/OkHttpMethodBase.kt b/src/main/java/com/nextcloud/common/OkHttpMethodBase.kt index 30d956eece..c596e00266 100644 --- a/src/main/java/com/nextcloud/common/OkHttpMethodBase.kt +++ b/src/main/java/com/nextcloud/common/OkHttpMethodBase.kt @@ -33,6 +33,8 @@ import okhttp3.Headers import okhttp3.HttpUrl import okhttp3.Request import okhttp3.Response +import java.io.IOException +import java.lang.Exception /** * Common base class for all new OkHttpMethods @@ -40,7 +42,7 @@ import okhttp3.Response abstract class OkHttpMethodBase(var uri: String, val useOcsApiRequestHeader: Boolean) { companion object { - const val UNKOWN_STATUS_CODE: Int = -1; + const val UNKNOWN_STATUS_CODE: Int = -1 } private var response: Response? = null @@ -94,7 +96,7 @@ abstract class OkHttpMethodBase(var uri: String, } fun getStatusCode(): Int { - return response?.code() ?: UNKOWN_STATUS_CODE + return response?.code() ?: UNKNOWN_STATUS_CODE } fun getStatusText(): String { @@ -109,6 +111,11 @@ abstract class OkHttpMethodBase(var uri: String, return response?.header(name) } + /** + * Execute operation using nextcloud client. + * + * @return HTTP return code or [UNKNOWN_STATUS_CODE] in case of network error. + */ fun execute(nextcloudClient: NextcloudClient): Int { val temp = requestBuilder .url(buildQueryParameter()) @@ -123,12 +130,16 @@ abstract class OkHttpMethodBase(var uri: String, val request = temp.build() - response = nextcloudClient.client.newCall(request).execute() + try { + response = nextcloudClient.client.newCall(request).execute() + } catch (ex: IOException) { + return UNKNOWN_STATUS_CODE + } - if (nextcloudClient.followRedirects) { - return nextcloudClient.followRedirection(this).getLastStatus() + return if (nextcloudClient.followRedirects) { + nextcloudClient.followRedirection(this).getLastStatus() } else { - return response?.code() ?: UNKOWN_STATUS_CODE + response?.code() ?: UNKNOWN_STATUS_CODE } } } diff --git a/src/main/java/com/nextcloud/common/ResponseOrError.kt b/src/main/java/com/nextcloud/common/ResponseOrError.kt new file mode 100644 index 0000000000..3f8bc0c0c1 --- /dev/null +++ b/src/main/java/com/nextcloud/common/ResponseOrError.kt @@ -0,0 +1,33 @@ +/* Nextcloud Android Library is available under MIT license + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz @ + * Copyright (C) 2020 Nextcloud 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.nextcloud.common + +import okhttp3.Response + +internal data class ResponseOrError private constructor(val result: Response?, val error: Exception?) { + constructor(result: Response) : this(result, null) + constructor(error: Exception) : this(null, error) +} diff --git a/src/test/java/com/nextcloud/common/NextcloudClientTest.kt b/src/test/java/com/nextcloud/common/NextcloudClientTest.kt new file mode 100644 index 0000000000..5a9948c780 --- /dev/null +++ b/src/test/java/com/nextcloud/common/NextcloudClientTest.kt @@ -0,0 +1,134 @@ +/** + * Nextcloud Android Library is available under MIT license + * + * @author Chris Narkiewicz + * Copyright (C) 2019 Chris Narkiewicz + * Copyright (C) 2019 Nextcloud 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.nextcloud.common + +import android.content.Context +import android.net.Uri +import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.verify +import com.nhaarman.mockitokotlin2.whenever +import com.owncloud.android.lib.common.operations.RemoteOperation +import com.owncloud.android.lib.common.operations.RemoteOperationResult +import okhttp3.Call +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.Response +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.MockitoAnnotations +import java.io.IOException + +class NextcloudClientTest { + + @Mock + lateinit var context: Context + + @Mock + lateinit var uri: Uri + + @Mock + lateinit var okHttpClient: OkHttpClient + + lateinit var nextcloudClient: NextcloudClient + + @Before + fun setUp() { + MockitoAnnotations.initMocks(this) + nextcloudClient = NextcloudClient(uri, context, okHttpClient) + } + + @Test + fun `exceptions handled when RemoteOperations is executed`() { + // GIVEN + // failing operations + // operations throws any kind of exception + val exception = RuntimeException("test exception") + val operation = object : RemoteOperation() { + override fun run(client: NextcloudClient?): RemoteOperationResult { + throw exception + } + } + + // WHEN + // operation is executed + val response = nextcloudClient.execute(operation) + + // THEN + // exception is not propagated + // error result is returned + assertSame("Exception should be returned", exception, response.exception) + } + + @Test + fun `exceptions raised by okhttp are returned`() { + // GIVEN + // failing okhttp request + val request = mock() + val call = mock() + whenever(okHttpClient.newCall(request)).thenReturn(call) + val expectedException = IOException() + whenever(call.execute()).thenThrow(expectedException) + + // WHEN + // request is executed + val result = nextcloudClient.execute(request) + + // THEN + // okhttp call is executed and throws + // exception is caught internally + // exception is returned + verify(call).execute() + assertNull(result.result) + assertSame(expectedException, result.error) + } + + @Test + fun `result returned by okhttp are returned`() { + // GIVEN + // okhttp request + val request = mock() + val okHttpResponse = mock() + val call = mock() + whenever(okHttpClient.newCall(request)).thenReturn(call) + whenever(call.execute()).thenReturn(okHttpResponse) + + // WHEN + // request is executed + val result = nextcloudClient.execute(request) + + // THEN + // okhttp call is executed and throws + // exception is caught internally + // exception is returned + verify(call).execute() + assertSame(okHttpResponse, result.result) + assertNull(result.error) + } +} diff --git a/src/test/java/com/nextcloud/common/OkHttpMethodBaseTest.kt b/src/test/java/com/nextcloud/common/OkHttpMethodBaseTest.kt new file mode 100644 index 0000000000..f1e2699231 --- /dev/null +++ b/src/test/java/com/nextcloud/common/OkHttpMethodBaseTest.kt @@ -0,0 +1,80 @@ +/** + * Nextcloud Android Library is available under MIT license + * + * @author Chris Narkiewicz + * Copyright (C) 2019 Chris Narkiewicz + * Copyright (C) 2019 Nextcloud 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.nextcloud.common + +import android.content.Context +import android.net.Uri +import com.nhaarman.mockitokotlin2.* +import okhttp3.* +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.MockitoAnnotations +import java.io.IOException + +class OkHttpMethodBaseTest { + + @Mock + lateinit var context: Context + + @Mock + lateinit var uri: Uri + + @Mock + lateinit var okHttpClient: OkHttpClient + + lateinit var nextcloudClient: NextcloudClient + + @Before + fun setUp() { + MockitoAnnotations.initMocks(this) + nextcloudClient = NextcloudClient(uri, context, okHttpClient) + nextcloudClient.credentials = Credentials.basic("username", "password") + } + + @Test + fun `exceptions throws by OkHttpMethodBase are handled`() { + // GIVEN + // failing method + val method = object : OkHttpMethodBase("http://example.com", true) {} + val call = mock() + whenever(okHttpClient.newCall(any())).thenReturn(call) + whenever(call.execute()).thenThrow(IOException::class.java) + + // WHEN + // method is called + val code = method.execute(nextcloudClient) + + // THEN + // okhttp call was executed + // exception is not propagated + // error code is returned instead + verify(call).execute() + assertEquals(OkHttpMethodBase.UNKNOWN_STATUS_CODE, code) + } +} diff --git a/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker b/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker new file mode 100644 index 0000000000..b7f1944b18 --- /dev/null +++ b/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker @@ -0,0 +1 @@ + mock-maker-inline