From 35e77fb58fc924f7cc606f12f35e860a8b371fe3 Mon Sep 17 00:00:00 2001
From: Matthias Urhahn <matthias.urhahn@sap.com>
Date: Fri, 20 Nov 2020 13:05:09 +0100
Subject: [PATCH] Refactor ENF version check (#1680)
---
.../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 bf0c6f5cc..5d4ce8b4f 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 044bff1af..7652fb055 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 0a0101565..b7d16994a 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 000000000..5cf38d6fb
--- /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 0082ef670..a8fdf3444 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<List<File>>()) } 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<ENFVersion.Companion.UnsupportedENFVersionException> {
+ assertThrows<OutdatedENFVersionException> {
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 4f30d19a1..6d9e563b9 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<IllegalArgumentException> {
- runBlockingTest {
- createInstance().isAtLeast(1L)
+ shouldThrow<OutdatedENFVersionException> {
+ 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<ENFVersion.Companion.UnsupportedENFVersionException> {
- 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<ApiException> {
+ requireMinimumVersion(ENFVersion.V1_6)
+ }
+ }
+ }
}
}
--
GitLab