PM-25012: Cipher versioning data types #433
Conversation
nikwithak
changed the base branch from
main
to
vault/cipher-versioning-with-data
|
New Issues (5)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## vault/cipher-versioning-with-data #433 +/- ##
====================================================================
Coverage ? 74.77%
====================================================================
Files ? 256
Lines ? 22417
Branches ? 0
====================================================================
Hits ? 16762
Misses ? 5655
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nikwithak
changed the title
| .unwrap_or_default() | ||
| } | ||
|
|
||
| /// Extracts and sets the CipherType-specific fields from the opaque `data` field. |
There was a problem hiding this comment.
can we add some more documentation here ?
| /// Extracts and sets the CipherType-specific fields from the opaque `data` field. | ||
| pub(crate) fn populate_cipher_types(&mut self) { | ||
| let Ok(data) = | ||
| serde_json::from_str::<serde_json::Value>(self.data.as_deref().unwrap_or("{}")) |
There was a problem hiding this comment.
❓ looks like we are doing a double deserialization. Would it be better if we did this directly on the target type
There was a problem hiding this comment.
Are you referring to the fact that we're doing from_str here, and from_value below?
Since we're only worried about the CipherType deserializations in this function, I think I can do away with the from_str call on this line, and only deserialize inside the match (with from_str instead of from_value).
However it sounds like I may need to refactor this anyway - if we need to keep #[deny_unknown_fields] like called out below, then we can't use serde to deserialize and I'll need to manually extract each of the values from data here - see discussion below.
There was a problem hiding this comment.
Since we're only worried about the CipherType deserializations in this function, I think I can do away with the from_str call on this line, and only deserialize inside the match (with from_str instead of from_value).
Yeah this is what I am referring to
| crate::CipherType::Login => self.login = serde_json::from_value(data).ok(), | ||
| crate::CipherType::SecureNote => self.secure_note = serde_json::from_value(data).ok(), | ||
| crate::CipherType::Card => self.card = serde_json::from_value(data).ok(), | ||
| crate::CipherType::Identity => self.identity = serde_json::from_value(data).ok(), | ||
| crate::CipherType::SshKey => self.ssh_key = serde_json::from_value(data).ok(), |
There was a problem hiding this comment.
❓ what happens if deserialization fails?
There was a problem hiding this comment.
If the deserialization fails, then we set the value to None. The cases I can think of where serialization fails are that data doesn't contain the required values for that CipherType.
Is it preferable to fail entirely and return an error in that case? It would mean that the data stored in the cipher's.data is inconsistent with the schema we've defined.
There was a problem hiding this comment.
@gbubemismith I added error handling - I added some additional error types to the existing VaultParseError and CipherError types in the new revision to handle these cases.
| use crate::{cipher::cipher::CopyableCipherFields, Cipher, VaultParseError}; | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone)] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields)] |
There was a problem hiding this comment.
we shouldn't remove the deny_unknown_fields
There was a problem hiding this comment.
I think we always want deny_unknown_fields to keep things in sync
What are we trying to keep in sync - is it that we don't get additional data from the server that we don't expect?
To me that makes sense if we need to make sure that the server isn't giving us additional data that we aren't expecting. But with this change, we are no longer using the server response for these fields, and are instead extracting it ourselves from the data field on the client-side - with that change, these values simply become windowed views into the datafield, and I don't think we are gaining value from rejecting unknown fields anymore.
There was a problem hiding this comment.
I synced with @gbubemismith offline on this, and right now we agreed that there's no need to keep deny_unknown_fields
|
@nikwithak Nice work so far. I think we want to target the |
nikwithak
changed the base branch from
vault/cipher-versioning-with-data
to
vault/feature/cipher-versioning
nikwithak
changed the base branch from
vault/feature/cipher-versioning
to
vault/cipher-versioning-with-data
I changed the target to |
|
I have a PR that has the api generated code, I can merge mine first |
🎟️ Tracking
PM-25012
📔 Objective
Moves the deserialization of the
dataobject into the SDK - as part of running cipher migrations, the CipherType objects will be extracted instead of being passed in from the client.⏰ Reminders before review
team
🦮 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