From d5fde726778cdce7d3119e4f850e6a59d725f1f0 Mon Sep 17 00:00:00 2001 From: Matthias Urhahn Date: Fri, 20 Nov 2020 12:49:08 +0100 Subject: [PATCH] Refactor ENF version check --- .../DefaultDiagnosisKeyProvider.kt | 10 +-- .../modules/version/DefaultENFVersion.kt | 32 +++------ .../nearby/modules/version/ENFVersion.kt | 18 ++--- .../version/OutdatedENFVersionException.kt | 6 ++ .../DefaultDiagnosisKeyProviderTest.kt | 10 ++- .../modules/version/DefaultENFVersionTest.kt | 69 +++++++++++-------- 6 files changed, 73 insertions(+), 72 deletions(-) create mode 100644 Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/OutdatedENFVersionException.kt diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProvider.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProvider.kt index bf0c6f5ccb9..5d4ce8b4fa8 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProvider.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProvider.kt @@ -25,13 +25,9 @@ class DefaultDiagnosisKeyProvider @Inject constructor( return true } - // Check version of ENF - try { - enfVersion.requireAtLeast(ENFVersion.V16) - } catch (e: Exception) { - Timber.e(e) - throw e - } + // Check version of ENF, WindowMode since v1.5, but version check since v1.6 + // Will throw if requirement is not satisfied + enfVersion.requireMinimumVersion(ENFVersion.V1_6) if (!submissionQuota.consumeQuota(1)) { Timber.w("No key files submitted because not enough quota available.") diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersion.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersion.kt index 044bff1afb9..7652fb055bd 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersion.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersion.kt @@ -9,7 +9,6 @@ import javax.inject.Singleton import kotlin.coroutines.resume import kotlin.coroutines.resumeWithException import kotlin.coroutines.suspendCoroutine -import kotlin.math.abs @Singleton class DefaultENFVersion @Inject constructor( @@ -23,22 +22,19 @@ class DefaultENFVersion @Inject constructor( null } - override suspend fun requireAtLeast(compareVersion: Long) { - if (!isAtLeast(compareVersion)) { - throw ENFVersion.Companion.UnsupportedENFVersionException() - } - } - - override suspend fun isAtLeast(compareVersion: Long): Boolean { - if (!compareVersion.isCorrectVersionLength) throw IllegalArgumentException("given version has incorrect length") - - return try { - internalGetENFClientVersion() >= compareVersion + override suspend fun requireMinimumVersion(required: Long) { + try { + val currentVersion = internalGetENFClientVersion() + if (currentVersion < required) { + val error = OutdatedENFVersionException(current = currentVersion, required = required) + Timber.e(error, "Version requirement not satisfied.") + throw error + } else { + Timber.d("Version requirement satisfied: current=$currentVersion, required=$required") + } } catch (apiException: ApiException) { if (apiException.statusCode != CommonStatusCodes.API_NOT_CONNECTED) { throw apiException - } else { - return false } } } @@ -48,12 +44,4 @@ class DefaultENFVersion @Inject constructor( .addOnSuccessListener { cont.resume(it) } .addOnFailureListener { cont.resumeWithException(it) } } - - // check if a raw long has the correct length to be considered an API version - private val Long.isCorrectVersionLength - get(): Boolean = abs(this).toString().length == GOOGLE_API_VERSION_FIELD_LENGTH - - companion object { - private const val GOOGLE_API_VERSION_FIELD_LENGTH = 8 - } } diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/ENFVersion.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/ENFVersion.kt index 0a01015653f..b7d16994a91 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/ENFVersion.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/ENFVersion.kt @@ -1,24 +1,18 @@ package de.rki.coronawarnapp.nearby.modules.version interface ENFVersion { - suspend fun getENFClientVersion(): Long? - /** - * Indicates if the client runs above a certain version - * - * @return isAboveVersion, if connected to an old unsupported version, return false + * May return null if the API is currently not connected. */ - suspend fun isAtLeast(compareVersion: Long): Boolean + suspend fun getENFClientVersion(): Long? /** - * Throws an [UnsupportedENFVersionException] if the client runs an old unsupported version of the ENF + * Throws an [OutdatedENFVersionException] if the client runs an old unsupported version of the ENF + * If the API is currently not connected, no exception will be thrown, we expect this to only be a temporary state */ - suspend fun requireAtLeast(compareVersion: Long) + suspend fun requireMinimumVersion(required: Long) companion object { - const val V16 = 16000000L - const val V15 = 15000000L - - class UnsupportedENFVersionException : Exception("The client runs an old unsupported version of the ENF") + const val V1_6 = 16000000L } } diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/OutdatedENFVersionException.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/OutdatedENFVersionException.kt new file mode 100644 index 00000000000..5cf38d6fb2e --- /dev/null +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/version/OutdatedENFVersionException.kt @@ -0,0 +1,6 @@ +package de.rki.coronawarnapp.nearby.modules.version + +class OutdatedENFVersionException( + val current: Long, + val required: Long +) : Exception("Client is using an outdated ENF version: current=$current, required=$required") diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProviderTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProviderTest.kt index 0082ef6707c..a8fdf34442f 100644 --- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProviderTest.kt +++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/diagnosiskeyprovider/DefaultDiagnosisKeyProviderTest.kt @@ -2,6 +2,7 @@ package de.rki.coronawarnapp.nearby.modules.diagnosiskeyprovider import com.google.android.gms.nearby.exposurenotification.ExposureNotificationClient import de.rki.coronawarnapp.nearby.modules.version.ENFVersion +import de.rki.coronawarnapp.nearby.modules.version.OutdatedENFVersionException import io.kotest.matchers.shouldBe import io.mockk.MockKAnnotations import io.mockk.clearAllMocks @@ -33,7 +34,7 @@ class DefaultDiagnosisKeyProviderTest : BaseTest() { coEvery { googleENFClient.provideDiagnosisKeys(any>()) } returns MockGMSTask.forValue(null) - coEvery { enfVersion.requireAtLeast(any()) } returns Unit + coEvery { enfVersion.requireMinimumVersion(any()) } returns Unit } @AfterEach @@ -49,11 +50,14 @@ class DefaultDiagnosisKeyProviderTest : BaseTest() { @Test fun `provide diagnosis keys with outdated ENF versions`() { - coEvery { enfVersion.requireAtLeast(any()) } throws ENFVersion.Companion.UnsupportedENFVersionException() + coEvery { enfVersion.requireMinimumVersion(any()) } throws OutdatedENFVersionException( + current = 9000, + required = 5000 + ) val provider = createProvider() - assertThrows { + assertThrows { runBlockingTest { provider.provideDiagnosisKeys(exampleKeyFiles) } shouldBe false } diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersionTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersionTest.kt index 4f30d19a1f9..6d9e563b93d 100644 --- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersionTest.kt +++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/version/DefaultENFVersionTest.kt @@ -2,21 +2,21 @@ package de.rki.coronawarnapp.nearby.modules.version import com.google.android.gms.common.api.ApiException import com.google.android.gms.common.api.CommonStatusCodes.API_NOT_CONNECTED +import com.google.android.gms.common.api.CommonStatusCodes.INTERNAL_ERROR import com.google.android.gms.common.api.Status import com.google.android.gms.nearby.exposurenotification.ExposureNotificationClient +import io.kotest.assertions.throwables.shouldNotThrowAny +import io.kotest.assertions.throwables.shouldThrow import io.kotest.matchers.shouldBe -import io.mockk.Called import io.mockk.MockKAnnotations import io.mockk.clearAllMocks import io.mockk.every import io.mockk.impl.annotations.MockK -import io.mockk.verify import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runBlockingTest import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test -import org.junit.jupiter.api.assertThrows import testhelpers.gms.MockGMSTask @ExperimentalCoroutinesApi @@ -39,61 +39,74 @@ internal class DefaultENFVersionTest { ) @Test - fun `isAbove API v16 is true for v17`() { + fun `current version is newer than the required version`() { every { client.version } returns MockGMSTask.forValue(17000000L) runBlockingTest { - createInstance().isAtLeast(ENFVersion.V16) shouldBe true + createInstance().apply { + getENFClientVersion() shouldBe 17000000L + shouldNotThrowAny { + requireMinimumVersion(ENFVersion.V1_6) + } + } } } @Test - fun `isAbove API v16 is false for v15`() { + fun `current version is older than the required version`() { every { client.version } returns MockGMSTask.forValue(15000000L) runBlockingTest { - createInstance().isAtLeast(ENFVersion.V16) shouldBe false - } - } + createInstance().apply { + getENFClientVersion() shouldBe 15000000L - @Test - fun `isAbove API v16 throws IllegalArgument for invalid version`() { - assertThrows { - runBlockingTest { - createInstance().isAtLeast(1L) + shouldThrow { + requireMinimumVersion(ENFVersion.V1_6) + } } - verify { client.version wasNot Called } } } @Test - fun `isAbove API v16 false when APIException for too low version`() { - every { client.version } returns MockGMSTask.forError(ApiException(Status(API_NOT_CONNECTED))) + fun `current version is equal to the required version`() { + every { client.version } returns MockGMSTask.forValue(16000000L) runBlockingTest { - createInstance().isAtLeast(ENFVersion.V16) shouldBe false + createInstance().apply { + getENFClientVersion() shouldBe ENFVersion.V1_6 + shouldNotThrowAny { + requireMinimumVersion(ENFVersion.V1_6) + } + } } } @Test - fun `require API v16 throws UnsupportedENFVersionException for v15`() { - every { client.version } returns MockGMSTask.forValue(ENFVersion.V15) + fun `API_NOT_CONNECTED exceptions are not treated as failures`() { + every { client.version } returns MockGMSTask.forError(ApiException(Status(API_NOT_CONNECTED))) - assertThrows { - runBlockingTest { - createInstance().requireAtLeast(ENFVersion.V16) + runBlockingTest { + createInstance().apply { + getENFClientVersion() shouldBe null + shouldNotThrowAny { + requireMinimumVersion(ENFVersion.V1_6) + } } } } @Test - fun `require API v15 does not throw for v16`() { - every { client.version } returns MockGMSTask.forValue(ENFVersion.V16) + fun `rethrows unexpected exceptions`() { + every { client.version } returns MockGMSTask.forError(ApiException(Status(INTERNAL_ERROR))) runBlockingTest { - createInstance().requireAtLeast(ENFVersion.V15) - } + createInstance().apply { + getENFClientVersion() shouldBe null - verify { client.version } + shouldThrow { + requireMinimumVersion(ENFVersion.V1_6) + } + } + } } }