[PM-9363] Disable cipher key encryption for older server versions #4006
Conversation
|
No New Or Fixed Issues Found |
| @@ -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")) | |||
There was a problem hiding this comment.
⛏️ Making "2024.2.0" a constant with a descriptive name would be helpful. "CIPHER_KEY_ENC_MIN_SERVER_VERSION" maybe?
| /** | ||
| * Checks if the server version is greater than another provided version, returns true if it is. | ||
| */ | ||
| fun isServerVersionAtLeast(serverConfig: ServerConfigRepository, version: String): Boolean { |
There was a problem hiding this comment.
🎨 Can we change serverConfig argument to type ServerConfig? or possibly even serverVersion: String? instead of passing the repository?
There was a problem hiding this comment.
I ended up using ServerConfig? instead of another string since I kept second guessing myself on which string was which regardless of the names
mpbw2
requested review from
brian-livefront,
david-livefront,
dseverns-livefront,
ahaisting-livefront and
a team
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4006 +/- ##
==========================================
- Coverage 88.85% 88.83% -0.02%
==========================================
Files 425 426 +1
Lines 35050 35073 +23
Branches 5138 5149 +11
==========================================
+ Hits 31144 31158 +14
- Misses 2112 2113 +1
- Partials 1794 1802 +8 ☔ View full report in Codecov by Sentry. |
| get() = mapOf( | ||
| CIPHER_KEY_ENCRYPTION_KEY to | ||
| isServerVersionAtLeast( | ||
| serverConfigRepository.getLocalServerConfig(), |
There was a problem hiding this comment.
Can you just access it via the stateFlow directly?
serverConfigRepository.serverConfigStateFlow.value| val otherPart = otherVersionParts.getOrNull(i)?.toIntOrNull() ?: 0 | ||
|
|
||
| if (serverPart > otherPart) { | ||
| return true |
There was a problem hiding this comment.
Can we lift the return
There was a problem hiding this comment.
We have to walk through the entire loop to know if the version parts are the same. Added a comment for clarity.
| val otherVersionParts = getVersionComponents(version) | ||
|
|
||
| if (serverVersionParts == null || otherVersionParts == null) { | ||
| return false |
| } | ||
| } | ||
|
|
||
| return true // Versions are equal |
There was a problem hiding this comment.
Comment should be above the return not on the same line.
Can we also add a test for this case
| get() = mapOf( | ||
| CIPHER_KEY_ENCRYPTION_KEY to | ||
| isServerVersionAtLeast( | ||
| serverConfigRepository.serverConfigStateFlow.value, |
| /** | ||
| * Checks if the server version is greater than another provided version, returns true if it is. | ||
| */ | ||
| fun isServerVersionAtLeast(serverConfig: ServerConfig?, version: String?): Boolean { |
There was a problem hiding this comment.
Last question, why would the version ever be null?
There was a problem hiding this comment.
It shouldn't be, but I felt weird skipping the null check further down just to satisfy the IDE so I made it nullable. Thoughts?
There was a problem hiding this comment.
I think we can make version nonnull and just do the empty check
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-9363
📔 Objective
Disable cipher key encryption in SDK if server version < 2024.2.0
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes