[PM-24683] Add updateKdf function #383
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
==========================================
+ Coverage 76.53% 77.08% +0.54%
==========================================
Files 269 269
Lines 25312 25768 +456
==========================================
+ Hits 19373 19862 +489
+ Misses 5939 5906 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ption-options-to-identity-sync-response
…r and docs improvements
mzieniukbw
dismissed stale reviews from dani-garcia and themself
The base branch was changed.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, PartialEq)] | ||
| pub struct IdentityUserDecryptionOptionsResponseModel { |
There was a problem hiding this comment.
Question: Why not use #[serde(rename_all = "camelCase")] rather than renaming the field?
There was a problem hiding this comment.
This response model was actually leftover from a previous upstream PR. Removed since its not needed.
| .master_password | ||
| .as_ref() | ||
| .map(|p| { | ||
| .map(|p| -> Result<String, CryptoError> { |
There was a problem hiding this comment.
Is this need here? We're already typing Ok::<String, CryptoError>( below, seems like the compiler should be able to infer it.
There was a problem hiding this comment.
If we keep it we should remove the type hint for Ok.
There was a problem hiding this comment.
I removed this and didn't get any compiler issues so I'm assuming its able to infer.
| /// re-encrypted with the new password. This returns the new encrypted user key and the new | ||
| /// password hash but does not update sdk state. | ||
| /// | ||
| /// Note: This is deprecated and `make_update_password` should be used instead |
There was a problem hiding this comment.
Do. we have a task for tracking the removal? And ideally a task to assign to mobile to clean up their implementation?
There was a problem hiding this comment.
Created the following Tech Debt tasks:
2. https://bitwarden.atlassian.net/browse/PM-25651
3. https://bitwarden.atlassian.net/browse/PM-25652
| .master_password | ||
| .as_ref() | ||
| .map(|p| { | ||
| .map(|p| -> Result<String, CryptoError> { |
There was a problem hiding this comment.
If we keep it we should remove the type hint for Ok.
crates/bitwarden-core/src/auth/api/response/identity_user_decryption_options_response.rs
Outdated
Show resolved
Hide resolved
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24683
📔 Objective
Exposes functionality to update the KDF, with the new masterpassword unlock data, and masterpassword authentication data models. These can be directly passed to the server models.
⏰ 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