From 7ab88ee5f12168afe90001841eeb051176439f4b Mon Sep 17 00:00:00 2001 From: mpbw2 <59324545+mpbw2@users.noreply.github.com> Date: Tue, 1 Oct 2024 09:27:27 -0400 Subject: [PATCH 1/5] Disable cipher key encryption for older server versions --- .../manager/FeatureFlagManagerImpl.kt | 4 +- .../repository/ServerConfigRepository.kt | 5 ++ .../repository/ServerConfigRepositoryImpl.kt | 4 ++ .../data/platform/util/ServerVersionUtils.kt | 46 +++++++++++++++++++ .../util/FakeServerConfigRepository.kt | 4 ++ 5 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt index d27c9578105..24208b104ee 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt @@ -3,6 +3,7 @@ package com.x8bit.bitwarden.data.platform.manager import com.x8bit.bitwarden.data.platform.datasource.disk.model.ServerConfig import com.x8bit.bitwarden.data.platform.manager.model.FlagKey import com.x8bit.bitwarden.data.platform.repository.ServerConfigRepository +import com.x8bit.bitwarden.data.platform.util.isServerVersionAtLeast import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map @@ -16,7 +17,8 @@ class FeatureFlagManagerImpl( ) : FeatureFlagManager { override val sdkFeatureFlags: Map - get() = mapOf(CIPHER_KEY_ENCRYPTION_KEY to true) + get() = mapOf(CIPHER_KEY_ENCRYPTION_KEY to + isServerVersionAtLeast(serverConfigRepository, "2024.2.0")) override fun getFeatureFlagFlow(key: FlagKey): Flow = serverConfigRepository diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt index 03aaae82598..5f0a63ea48c 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt @@ -18,4 +18,9 @@ interface ServerConfigRepository { * updates the values using server side data. */ suspend fun getServerConfig(forceRefresh: Boolean): ServerConfig? + + /** + * Gets the state [ServerConfig] synchronously from local storage only. + */ + fun getLocalServerConfig(): ServerConfig? } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt index df86fc32788..463a9dc6d58 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt @@ -59,6 +59,10 @@ class ServerConfigRepositoryImpl( return localConfig } + override fun getLocalServerConfig(): ServerConfig? { + return configDiskSource.serverConfig + } + private companion object { private const val MINIMUM_CONFIG_SYNC_INTERVAL_SEC: Long = 60 * 60 } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt new file mode 100644 index 00000000000..1354143c0d5 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt @@ -0,0 +1,46 @@ +package com.x8bit.bitwarden.data.platform.util + +import com.x8bit.bitwarden.data.platform.repository.ServerConfigRepository +import kotlin.text.split +import kotlin.text.toIntOrNull + +private const val VERSION_SEPARATOR = "." +private const val SUFFIX_SEPARATOR = "-" + +/** + * Checks if the server version is greater than another provided version, returns true if it is. + */ + fun isServerVersionAtLeast(serverConfig: ServerConfigRepository, version: String): Boolean { + val serverVersion = serverConfig + .getLocalServerConfig() + ?.serverData + ?.version + + val serverVersionParts = getVersionComponents(serverVersion) + val otherVersionParts = getVersionComponents(version) + + if (serverVersionParts == null || otherVersionParts == null) { + return false + } + + for (i in serverVersionParts.indices) { + val serverPart = serverVersionParts.getOrNull(i)?.toIntOrNull() ?: 0 + val otherPart = otherVersionParts.getOrNull(i)?.toIntOrNull() ?: 0 + + if (serverPart > otherPart) { + return true + } else if (serverPart < otherPart) { + return false + } + } + + return true // Versions are equal +} + +/** + * Extracts the version components from a version string, disregarding any suffixes. + */ +private fun getVersionComponents(version: String?): List? { + val versionComponents = version?.split(SUFFIX_SEPARATOR)?.first() + return versionComponents?.split(VERSION_SEPARATOR) +} diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt index a5d3239c8b7..0a957a3b035 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt @@ -27,6 +27,10 @@ class FakeServerConfigRepository : ServerConfigRepository { return serverConfigValue } + override fun getLocalServerConfig(): ServerConfig? { + return SERVER_CONFIG + } + override val serverConfigStateFlow: StateFlow get() = mutableServerConfigFlow } From 95dfe5e65431452ff59b61d256579f82d379a5cd Mon Sep 17 00:00:00 2001 From: mpbw2 <59324545+mpbw2@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:17:02 -0400 Subject: [PATCH 2/5] tweaks and simple version tests --- .../manager/FeatureFlagManagerImpl.kt | 10 ++++++-- .../data/platform/util/ServerVersionUtils.kt | 5 ++-- .../manager/FeatureFlagManagerTest.kt | 25 +++++++++++++++++++ 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt index 24208b104ee..bc859961de7 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt @@ -8,6 +8,7 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map private const val CIPHER_KEY_ENCRYPTION_KEY = "enableCipherKeyEncryption" +private const val CIPHER_KEY_ENC_MIN_SERVER_VERSION = "2024.2.0" /** * Primary implementation of [FeatureFlagManager]. @@ -17,8 +18,13 @@ class FeatureFlagManagerImpl( ) : FeatureFlagManager { override val sdkFeatureFlags: Map - get() = mapOf(CIPHER_KEY_ENCRYPTION_KEY to - isServerVersionAtLeast(serverConfigRepository, "2024.2.0")) + get() = mapOf( + CIPHER_KEY_ENCRYPTION_KEY to + isServerVersionAtLeast( + serverConfigRepository.getLocalServerConfig(), + CIPHER_KEY_ENC_MIN_SERVER_VERSION, + ), + ) override fun getFeatureFlagFlow(key: FlagKey): Flow = serverConfigRepository diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt index 1354143c0d5..ca3ab07baea 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt @@ -1,6 +1,6 @@ package com.x8bit.bitwarden.data.platform.util -import com.x8bit.bitwarden.data.platform.repository.ServerConfigRepository +import com.x8bit.bitwarden.data.platform.datasource.disk.model.ServerConfig import kotlin.text.split import kotlin.text.toIntOrNull @@ -10,9 +10,8 @@ private const val SUFFIX_SEPARATOR = "-" /** * Checks if the server version is greater than another provided version, returns true if it is. */ - fun isServerVersionAtLeast(serverConfig: ServerConfigRepository, version: String): Boolean { + fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String): Boolean { val serverVersion = serverConfig - .getLocalServerConfig() ?.serverData ?.version diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt index 8f5466c8b48..8420a4a82fd 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt @@ -7,6 +7,7 @@ import com.x8bit.bitwarden.data.platform.datasource.network.model.ConfigResponse import com.x8bit.bitwarden.data.platform.datasource.network.model.ConfigResponseJson.ServerJson import com.x8bit.bitwarden.data.platform.manager.model.FlagKey import com.x8bit.bitwarden.data.platform.repository.util.FakeServerConfigRepository +import com.x8bit.bitwarden.data.platform.util.isServerVersionAtLeast import kotlinx.coroutines.test.runTest import kotlinx.serialization.json.JsonPrimitive import org.junit.Test @@ -33,6 +34,30 @@ class FeatureFlagManagerTest { assertEquals(expected, actual) } + @Test + fun `server version is at least supplied version`() { + val result = + isServerVersionAtLeast( + fakeServerConfigRepository.getLocalServerConfig(), + "2024.2.0", + ) + + // This relies on the fake server version being "2024.7.0" + assertTrue(result) + } + + @Test + fun `server version is not at least supplied version`() { + val result = + isServerVersionAtLeast( + fakeServerConfigRepository.getLocalServerConfig(), + "2024.12.0-suffix", + ) + + // This relies on the fake server version being "2024.7.0" + assertFalse(result) + } + @Test fun `ServerConfigRepository flow with value should trigger new flags`() = runTest { fakeServerConfigRepository.serverConfigValue = null From ecf64073a194541a041c345c49c81cc20258959b Mon Sep 17 00:00:00 2001 From: mpbw2 <59324545+mpbw2@users.noreply.github.com> Date: Thu, 3 Oct 2024 15:48:11 -0400 Subject: [PATCH 3/5] adjustments --- .../manager/FeatureFlagManagerImpl.kt | 2 +- .../repository/ServerConfigRepository.kt | 5 ---- .../repository/ServerConfigRepositoryImpl.kt | 4 --- .../data/platform/util/ServerVersionUtils.kt | 12 ++++++--- .../manager/FeatureFlagManagerTest.kt | 27 +++++++++++++++++-- .../util/FakeServerConfigRepository.kt | 4 --- 6 files changed, 35 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt index bc859961de7..947dabb1dfa 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt @@ -21,7 +21,7 @@ class FeatureFlagManagerImpl( get() = mapOf( CIPHER_KEY_ENCRYPTION_KEY to isServerVersionAtLeast( - serverConfigRepository.getLocalServerConfig(), + serverConfigRepository.serverConfigStateFlow.value, CIPHER_KEY_ENC_MIN_SERVER_VERSION, ), ) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt index 5f0a63ea48c..03aaae82598 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepository.kt @@ -18,9 +18,4 @@ interface ServerConfigRepository { * updates the values using server side data. */ suspend fun getServerConfig(forceRefresh: Boolean): ServerConfig? - - /** - * Gets the state [ServerConfig] synchronously from local storage only. - */ - fun getLocalServerConfig(): ServerConfig? } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt index 463a9dc6d58..df86fc32788 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/ServerConfigRepositoryImpl.kt @@ -59,10 +59,6 @@ class ServerConfigRepositoryImpl( return localConfig } - override fun getLocalServerConfig(): ServerConfig? { - return configDiskSource.serverConfig - } - private companion object { private const val MINIMUM_CONFIG_SYNC_INTERVAL_SEC: Long = 60 * 60 } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt index ca3ab07baea..44b2257c191 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt @@ -10,15 +10,19 @@ private const val SUFFIX_SEPARATOR = "-" /** * Checks if the server version is greater than another provided version, returns true if it is. */ - fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String): Boolean { +fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String?): Boolean { val serverVersion = serverConfig ?.serverData ?.version + if (serverVersion.isNullOrEmpty() || version.isNullOrEmpty()) { + return false + } + val serverVersionParts = getVersionComponents(serverVersion) val otherVersionParts = getVersionComponents(version) - if (serverVersionParts == null || otherVersionParts == null) { + if (serverVersionParts.isNullOrEmpty() || otherVersionParts.isNullOrEmpty()) { return false } @@ -33,7 +37,9 @@ private const val SUFFIX_SEPARATOR = "-" } } - return true // Versions are equal + + // Versions are equal + return true } /** diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt index 8420a4a82fd..1c7a6f70dc1 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt @@ -38,7 +38,7 @@ class FeatureFlagManagerTest { fun `server version is at least supplied version`() { val result = isServerVersionAtLeast( - fakeServerConfigRepository.getLocalServerConfig(), + fakeServerConfigRepository.serverConfigStateFlow.value, "2024.2.0", ) @@ -50,7 +50,7 @@ class FeatureFlagManagerTest { fun `server version is not at least supplied version`() { val result = isServerVersionAtLeast( - fakeServerConfigRepository.getLocalServerConfig(), + fakeServerConfigRepository.serverConfigStateFlow.value, "2024.12.0-suffix", ) @@ -58,6 +58,29 @@ class FeatureFlagManagerTest { assertFalse(result) } + @Test + fun `server version is the same as supplied version`() { + val result = + isServerVersionAtLeast( + fakeServerConfigRepository.serverConfigStateFlow.value, + "2024.7.0", + ) + + // This relies on the fake server version being "2024.7.0" + assertTrue(result) + } + + @Test + fun `server version is not the same as blank supplied version`() { + val result = + isServerVersionAtLeast( + fakeServerConfigRepository.serverConfigStateFlow.value, + "", + ) + + assertFalse(result) + } + @Test fun `ServerConfigRepository flow with value should trigger new flags`() = runTest { fakeServerConfigRepository.serverConfigValue = null diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt index 0a957a3b035..a5d3239c8b7 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt @@ -27,10 +27,6 @@ class FakeServerConfigRepository : ServerConfigRepository { return serverConfigValue } - override fun getLocalServerConfig(): ServerConfig? { - return SERVER_CONFIG - } - override val serverConfigStateFlow: StateFlow get() = mutableServerConfigFlow } From e6204b1ad4e64372bcd4cb47f67a2e0803777c8c Mon Sep 17 00:00:00 2001 From: mpbw2 <59324545+mpbw2@users.noreply.github.com> Date: Thu, 3 Oct 2024 16:40:19 -0400 Subject: [PATCH 4/5] adjustments --- .../x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt index 44b2257c191..a0e80087169 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt @@ -26,6 +26,7 @@ fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String?): Boole return false } + // Must iterate through all indices to establish if versions are equal for (i in serverVersionParts.indices) { val serverPart = serverVersionParts.getOrNull(i)?.toIntOrNull() ?: 0 val otherPart = otherVersionParts.getOrNull(i)?.toIntOrNull() ?: 0 @@ -37,7 +38,6 @@ fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String?): Boole } } - // Versions are equal return true } From f47edb37d66c31fa310d4d9a6afa465bf6d0bbec Mon Sep 17 00:00:00 2001 From: mpbw2 <59324545+mpbw2@users.noreply.github.com> Date: Thu, 3 Oct 2024 17:19:22 -0400 Subject: [PATCH 5/5] adjustments --- .../x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt index a0e80087169..6d5ed31e2f1 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/ServerVersionUtils.kt @@ -10,12 +10,12 @@ private const val SUFFIX_SEPARATOR = "-" /** * Checks if the server version is greater than another provided version, returns true if it is. */ -fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String?): Boolean { +fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String): Boolean { val serverVersion = serverConfig ?.serverData ?.version - if (serverVersion.isNullOrEmpty() || version.isNullOrEmpty()) { + if (serverVersion.isNullOrEmpty() || version.isEmpty()) { return false }