From 99fcbaa89923d5b8dbb69c471eb99841650c1197 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Wed, 21 Dec 2022 22:25:03 +0100 Subject: [PATCH 01/10] Fix showing SSL errors if no scheme was specified --- .../mealient/ui/baseurl/BaseURLViewModel.kt | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt index 42afc2b..1c8fbed 100644 --- a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt +++ b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt @@ -12,6 +12,7 @@ import gq.kirmanak.mealient.logging.Logger import gq.kirmanak.mealient.ui.OperationUiState import kotlinx.coroutines.launch import javax.inject.Inject +import javax.net.ssl.SSLHandshakeException @HiltViewModel class BaseURLViewModel @Inject constructor( @@ -27,24 +28,38 @@ class BaseURLViewModel @Inject constructor( fun saveBaseUrl(baseURL: String) { logger.v { "saveBaseUrl() called with: baseURL = $baseURL" } _uiState.value = OperationUiState.Progress() - val hasPrefix = ALLOWED_PREFIXES.any { baseURL.startsWith(it) } - var url = baseURL.takeIf { hasPrefix } ?: WITH_PREFIX_FORMAT.format(baseURL) - url = url.trimStart().trimEnd { it == '/' || it.isWhitespace() } - viewModelScope.launch { checkBaseURL(url) } + viewModelScope.launch { checkBaseURL(baseURL) } } private suspend fun checkBaseURL(baseURL: String) { logger.v { "checkBaseURL() called with: baseURL = $baseURL" } - if (baseURL == serverInfoRepo.getUrl()) { + + val hasPrefix = ALLOWED_PREFIXES.any { baseURL.startsWith(it) } + val urlWithPrefix = baseURL.takeIf { hasPrefix } ?: WITH_PREFIX_FORMAT.format(baseURL) + val url = urlWithPrefix.trimStart().trimEnd { it == '/' || it.isWhitespace() } + + logger.d { "checkBaseURL: Created URL = $url, with prefix = $urlWithPrefix" } + if (url == serverInfoRepo.getUrl()) { logger.d { "checkBaseURL: new URL matches current" } _uiState.value = OperationUiState.fromResult(Result.success(Unit)) return } - val result = serverInfoRepo.tryBaseURL(baseURL) + + val result: Result = serverInfoRepo.tryBaseURL(url).recoverCatching { + logger.e(it) { "checkBaseURL: trying to recover" } + if (hasPrefix.not() && it.cause is SSLHandshakeException) { + val unencryptedUrl = url.replace("https", "http") + serverInfoRepo.tryBaseURL(unencryptedUrl).getOrThrow() + } else { + throw it + } + } + if (result.isSuccess) { authRepo.logout() recipeRepo.clearLocalData() } + logger.i { "checkBaseURL: result is $result" } _uiState.value = OperationUiState.fromResult(result) } From 8e509894f80c8801e9c1cda5f272bbee1152aeef Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Wed, 21 Dec 2022 22:26:27 +0100 Subject: [PATCH 02/10] Bump app version --- app/build.gradle.kts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 273a884..77bd899 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -16,8 +16,8 @@ plugins { android { defaultConfig { applicationId = "gq.kirmanak.mealient" - versionCode = 25 - versionName = "0.3.10" + versionCode = 26 + versionName = "0.3.11" testInstrumentationRunner = "gq.kirmanak.mealient.MealientTestRunner" testInstrumentationRunnerArguments += mapOf("clearPackageData" to "true") } From e8077f460a518c8b1d566a888404f3ae20f3c66e Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Wed, 21 Dec 2022 22:26:50 +0100 Subject: [PATCH 03/10] Improve logging of base URL recovery --- .../java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt index 1c8fbed..77fae73 100644 --- a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt +++ b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt @@ -46,7 +46,7 @@ class BaseURLViewModel @Inject constructor( } val result: Result = serverInfoRepo.tryBaseURL(url).recoverCatching { - logger.e(it) { "checkBaseURL: trying to recover" } + logger.e(it) { "checkBaseURL: trying to recover, had prefix = $hasPrefix" } if (hasPrefix.not() && it.cause is SSLHandshakeException) { val unencryptedUrl = url.replace("https", "http") serverInfoRepo.tryBaseURL(unencryptedUrl).getOrThrow() From 04474c30552fecc44bdd56497fff019e55676ede Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Wed, 21 Dec 2022 22:36:02 +0100 Subject: [PATCH 04/10] Remove redundant trim from base URL VM --- .../java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt index 77fae73..b6cd82d 100644 --- a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt +++ b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt @@ -36,9 +36,9 @@ class BaseURLViewModel @Inject constructor( val hasPrefix = ALLOWED_PREFIXES.any { baseURL.startsWith(it) } val urlWithPrefix = baseURL.takeIf { hasPrefix } ?: WITH_PREFIX_FORMAT.format(baseURL) - val url = urlWithPrefix.trimStart().trimEnd { it == '/' || it.isWhitespace() } + val url = urlWithPrefix.trimEnd { it == '/' } - logger.d { "checkBaseURL: Created URL = $url, with prefix = $urlWithPrefix" } + logger.d { "checkBaseURL: Created URL = \"$url\", with prefix = \"$urlWithPrefix\"" } if (url == serverInfoRepo.getUrl()) { logger.d { "checkBaseURL: new URL matches current" } _uiState.value = OperationUiState.fromResult(Result.success(Unit)) From d74ce4bb2a9b652cde1d8f31d278f929d94a89a3 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Wed, 21 Dec 2022 22:36:16 +0100 Subject: [PATCH 05/10] Improve Base URL interceptor error reporting --- .../kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt index 4b40e26..b64ae1f 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt @@ -4,7 +4,7 @@ import gq.kirmanak.mealient.datasource.LocalInterceptor import gq.kirmanak.mealient.datasource.ServerUrlProvider import gq.kirmanak.mealient.logging.Logger import kotlinx.coroutines.runBlocking -import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import okhttp3.HttpUrl.Companion.toHttpUrl import okhttp3.Interceptor import okhttp3.Response import java.io.IOException @@ -37,6 +37,7 @@ class BaseUrlInterceptor @Inject constructor( } private fun getBaseUrl() = runBlocking { - serverUrlProvider.getUrl()?.toHttpUrlOrNull() ?: throw IOException("Base URL is unknown") + val url = serverUrlProvider.getUrl() ?: throw IOException("Base URL is unknown") + url.runCatching { toHttpUrl() }.recover { throw IOException(it) }.getOrThrow() } } \ No newline at end of file From feb9110f47e0d80bd04f3d12d32f653ac45ad8e3 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Wed, 21 Dec 2022 22:39:09 +0100 Subject: [PATCH 06/10] Remove Exception name from error messages --- .../main/kotlin/gq/kirmanak/mealient/datasource/NetworkError.kt | 2 +- .../gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/NetworkError.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/NetworkError.kt index 50953a4..024c234 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/NetworkError.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/NetworkError.kt @@ -1,6 +1,6 @@ package gq.kirmanak.mealient.datasource -sealed class NetworkError(cause: Throwable) : RuntimeException(cause) { +sealed class NetworkError(cause: Throwable) : RuntimeException(cause.message, cause) { class Unauthorized(cause: Throwable) : NetworkError(cause) class NoServerConnection(cause: Throwable) : NetworkError(cause) class NotMealie(cause: Throwable) : NetworkError(cause) diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt index b64ae1f..8796804 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt @@ -38,6 +38,6 @@ class BaseUrlInterceptor @Inject constructor( private fun getBaseUrl() = runBlocking { val url = serverUrlProvider.getUrl() ?: throw IOException("Base URL is unknown") - url.runCatching { toHttpUrl() }.recover { throw IOException(it) }.getOrThrow() + url.runCatching { toHttpUrl() }.recover { throw IOException(it.message, it) }.getOrThrow() } } \ No newline at end of file From 0bf4747d180cfa95ea82ac7f17729e1d8c6334f7 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Wed, 21 Dec 2022 22:44:47 +0100 Subject: [PATCH 07/10] Inline scheme names --- .../gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt index b6cd82d..e2bd49e 100644 --- a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt +++ b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt @@ -34,8 +34,8 @@ class BaseURLViewModel @Inject constructor( private suspend fun checkBaseURL(baseURL: String) { logger.v { "checkBaseURL() called with: baseURL = $baseURL" } - val hasPrefix = ALLOWED_PREFIXES.any { baseURL.startsWith(it) } - val urlWithPrefix = baseURL.takeIf { hasPrefix } ?: WITH_PREFIX_FORMAT.format(baseURL) + val hasPrefix = listOf("http://", "https://").any { baseURL.startsWith(it) } + val urlWithPrefix = baseURL.takeIf { hasPrefix } ?: "https://%s".format(baseURL) val url = urlWithPrefix.trimEnd { it == '/' } logger.d { "checkBaseURL: Created URL = \"$url\", with prefix = \"$urlWithPrefix\"" } @@ -64,8 +64,4 @@ class BaseURLViewModel @Inject constructor( _uiState.value = OperationUiState.fromResult(result) } - companion object { - private val ALLOWED_PREFIXES = listOf("http://", "https://") - private const val WITH_PREFIX_FORMAT = "https://%s" - } } \ No newline at end of file From 05058091e943ea8f098b723c8350c020b473562e Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Thu, 22 Dec 2022 18:39:31 +0100 Subject: [PATCH 08/10] Add BaseURLViewModel tests --- .../ui/baseurl/BaseURLViewModelTest.kt | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt b/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt index c0fd1d2..3250c6f 100644 --- a/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt +++ b/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt @@ -4,17 +4,21 @@ import com.google.common.truth.Truth.assertThat import gq.kirmanak.mealient.data.auth.AuthRepo import gq.kirmanak.mealient.data.baseurl.ServerInfoRepo import gq.kirmanak.mealient.data.recipes.RecipeRepo +import gq.kirmanak.mealient.datasource.NetworkError import gq.kirmanak.mealient.test.AuthImplTestData.TEST_BASE_URL import gq.kirmanak.mealient.test.BaseUnitTest import gq.kirmanak.mealient.ui.OperationUiState import io.mockk.coEvery import io.mockk.coVerify +import io.mockk.coVerifyOrder import io.mockk.impl.annotations.MockK import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.* import org.junit.Before import org.junit.Test import java.io.IOException +import java.net.UnknownHostException +import javax.net.ssl.SSLHandshakeException @OptIn(ExperimentalCoroutinesApi::class) class BaseURLViewModelTest : BaseUnitTest() { @@ -105,4 +109,35 @@ class BaseURLViewModelTest : BaseUnitTest() { advanceUntilIdle() assertThat(subject.uiState.value).isInstanceOf(OperationUiState.Failure::class.java) } + + @Test + fun `when saving base url with no prefix and https throws expect http attempt`() = runTest { + coEvery { serverInfoRepo.getUrl() } returns null + val err = NetworkError.MalformedUrl(SSLHandshakeException("test")) + coEvery { serverInfoRepo.tryBaseURL("https://test") } returns Result.failure(err) + coEvery { serverInfoRepo.tryBaseURL("http://test") } returns Result.success(Unit) + subject.saveBaseUrl("test") + coVerifyOrder { + serverInfoRepo.tryBaseURL("https://test") + serverInfoRepo.tryBaseURL("http://test") + } + } + + @Test + fun `when saving base url with no prefix and https throws non ssl expect no http`() = runTest { + coEvery { serverInfoRepo.getUrl() } returns null + val err = NetworkError.MalformedUrl(UnknownHostException()) + coEvery { serverInfoRepo.tryBaseURL("https://test") } returns Result.failure(err) + subject.saveBaseUrl("test") + coVerify(inverse = true) { serverInfoRepo.tryBaseURL("http://test") } + } + + @Test + fun `when saving base url with https prefix and https throws expect no http call`() = runTest { + coEvery { serverInfoRepo.getUrl() } returns null + val err = NetworkError.MalformedUrl(SSLHandshakeException("test")) + coEvery { serverInfoRepo.tryBaseURL("https://test") } returns Result.failure(err) + subject.saveBaseUrl("https://test") + coVerify(inverse = true) { serverInfoRepo.tryBaseURL("http://test") } + } } \ No newline at end of file From 67eb1eca8ba70978795581687d4013dfe2366031 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Thu, 22 Dec 2022 18:43:18 +0100 Subject: [PATCH 09/10] Simplify getBaseUrl method --- .../mealient/datasource/impl/BaseUrlInterceptor.kt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt index 8796804..ad00f26 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/BaseUrlInterceptor.kt @@ -38,6 +38,11 @@ class BaseUrlInterceptor @Inject constructor( private fun getBaseUrl() = runBlocking { val url = serverUrlProvider.getUrl() ?: throw IOException("Base URL is unknown") - url.runCatching { toHttpUrl() }.recover { throw IOException(it.message, it) }.getOrThrow() + url.runCatching { + toHttpUrl() + }.fold( + onSuccess = { it }, + onFailure = { throw IOException(it.message, it) }, + ) } } \ No newline at end of file From f3221124056429ca1133ed7752e302037d6b8722 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Thu, 22 Dec 2022 18:48:46 +0100 Subject: [PATCH 10/10] Fallback to HTTP if 443 is closed too --- .../gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt | 8 ++++---- .../kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt | 3 +-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt index e2bd49e..015e659 100644 --- a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt +++ b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt @@ -8,11 +8,11 @@ import dagger.hilt.android.lifecycle.HiltViewModel import gq.kirmanak.mealient.data.auth.AuthRepo import gq.kirmanak.mealient.data.baseurl.ServerInfoRepo import gq.kirmanak.mealient.data.recipes.RecipeRepo +import gq.kirmanak.mealient.datasource.NetworkError import gq.kirmanak.mealient.logging.Logger import gq.kirmanak.mealient.ui.OperationUiState import kotlinx.coroutines.launch import javax.inject.Inject -import javax.net.ssl.SSLHandshakeException @HiltViewModel class BaseURLViewModel @Inject constructor( @@ -47,11 +47,11 @@ class BaseURLViewModel @Inject constructor( val result: Result = serverInfoRepo.tryBaseURL(url).recoverCatching { logger.e(it) { "checkBaseURL: trying to recover, had prefix = $hasPrefix" } - if (hasPrefix.not() && it.cause is SSLHandshakeException) { + if (hasPrefix || it is NetworkError.NotMealie) { + throw it + } else { val unencryptedUrl = url.replace("https", "http") serverInfoRepo.tryBaseURL(unencryptedUrl).getOrThrow() - } else { - throw it } } diff --git a/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt b/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt index 3250c6f..59b10ea 100644 --- a/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt +++ b/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt @@ -17,7 +17,6 @@ import kotlinx.coroutines.test.* import org.junit.Before import org.junit.Test import java.io.IOException -import java.net.UnknownHostException import javax.net.ssl.SSLHandshakeException @OptIn(ExperimentalCoroutinesApi::class) @@ -126,7 +125,7 @@ class BaseURLViewModelTest : BaseUnitTest() { @Test fun `when saving base url with no prefix and https throws non ssl expect no http`() = runTest { coEvery { serverInfoRepo.getUrl() } returns null - val err = NetworkError.MalformedUrl(UnknownHostException()) + val err = NetworkError.NotMealie(IOException()) coEvery { serverInfoRepo.tryBaseURL("https://test") } returns Result.failure(err) subject.saveBaseUrl("test") coVerify(inverse = true) { serverInfoRepo.tryBaseURL("http://test") }