Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-9363] Disable cipher key encryption for older server versions #4006

Merged
merged 5 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -16,7 +17,8 @@ class FeatureFlagManagerImpl(
) : FeatureFlagManager {

override val sdkFeatureFlags: Map<String, Boolean>
get() = mapOf(CIPHER_KEY_ENCRYPTION_KEY to true)
get() = mapOf(CIPHER_KEY_ENCRYPTION_KEY to
isServerVersionAtLeast(serverConfigRepository, "2024.2.0"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ›๏ธ Making "2024.2.0" a constant with a descriptive name would be helpful. "CIPHER_KEY_ENC_MIN_SERVER_VERSION" maybe?


override fun <T : Any> getFeatureFlagFlow(key: FlagKey<T>): Flow<T> =
serverConfigRepository
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐ŸŽจ Can we change serverConfig argument to type ServerConfig? or possibly even serverVersion: String? instead of passing the repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up using ServerConfig? instead of another string since I kept second guessing myself on which string was which regardless of the names

val serverVersion = serverConfig
.getLocalServerConfig()
?.serverData
?.version

val serverVersionParts = getVersionComponents(serverVersion)
val otherVersionParts = getVersionComponents(version)

if (serverVersionParts == null || otherVersionParts == null) {
return false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test

}

for (i in serverVersionParts.indices) {
val serverPart = serverVersionParts.getOrNull(i)?.toIntOrNull() ?: 0
val otherPart = otherVersionParts.getOrNull(i)?.toIntOrNull() ?: 0

if (serverPart > otherPart) {
return true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we lift the return

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to walk through the entire loop to know if the version parts are the same. Added a comment for clarity.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, missed that

} else if (serverPart < otherPart) {
return false
}
}

return true // Versions are equal
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment should be above the return not on the same line.

Can we also add a test for this case

}

/**
* Extracts the version components from a version string, disregarding any suffixes.
*/
private fun getVersionComponents(version: String?): List<String>? {
val versionComponents = version?.split(SUFFIX_SEPARATOR)?.first()
return versionComponents?.split(VERSION_SEPARATOR)
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ class FakeServerConfigRepository : ServerConfigRepository {
return serverConfigValue
}

override fun getLocalServerConfig(): ServerConfig? {
return SERVER_CONFIG
}

override val serverConfigStateFlow: StateFlow<ServerConfig?>
get() = mutableServerConfigFlow
}
Expand Down
Loading