Tools/pm 31067/move sends api calls to sdk 2#756
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
harr1424
left a comment
harr1424
left a comment
There was a problem hiding this comment.
Looks good overall! I've added some suggestions to update comments and also the return type you noticed on the call earlier.
Co-authored-by: Daniel García <dani-garcia@users.noreply.github.com>
Co-authored-by: John Harrington <84741727+harr1424@users.noreply.github.com>
Co-authored-by: John Harrington <84741727+harr1424@users.noreply.github.com>
coroiu
left a comment
coroiu
left a comment
There was a problem hiding this comment.
I haven't had a change to review the whole PR yet but I wanted to post my response regarding Oscars comments
….com:bitwarden/sdk-internal into tools/pm-31067/move-sends-api-calls-to-sdk-2
| edit_send( | ||
| key_store, | ||
| &config.api_client, | ||
| repository.as_ref(), | ||
| send_id, | ||
| request, | ||
| ) |
There was a problem hiding this comment.
suggestion: don't split out implementation into separate functions like this. If you need to organize the code into multiple files then you can create multiple impl SendClient { blocks (on in each file)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #756 +/- ##
==========================================
+ Coverage 81.59% 81.94% +0.34%
==========================================
Files 343 347 +4
Lines 40624 41835 +1211
==========================================
+ Hits 33149 34283 +1134
- Misses 7475 7552 +77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
To resolve the linting errors related to |
| /// Email addresses for OTP authentication. | ||
| /// **Note**: Mutually exclusive with `new_password`. If both are set, | ||
| /// only password authentication will be used. | ||
| pub emails: Vec<String>, | ||
| pub auth_type: AuthType, |
There was a problem hiding this comment.
The new view_type is nice. Can we do the same for auth state?
pub enum SendAuthType {
None,
Password(String),
Emails(Vec<String>),
}| let file = if let SendViewType::File(f) = self.view_type.clone() { | ||
| Some(Box::new(bitwarden_api_api::models::SendFileModel { | ||
| id: f.id.clone(), | ||
| file_name: Some(f.file_name.encrypt(ctx, send_key)?.to_string()), | ||
| size: f.size.as_ref().and_then(|s| s.parse::<i64>().ok()), | ||
| size_name: f.size_name.clone(), | ||
| })) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let text = if let SendViewType::Text(t) = self.view_type.clone() { | ||
| Some(Box::new(bitwarden_api_api::models::SendTextModel { | ||
| text: t | ||
| .text | ||
| .as_ref() | ||
| .map(|txt| txt.encrypt(ctx, send_key)) | ||
| .transpose()? | ||
| .map(|e| e.to_string()), | ||
| hidden: Some(t.hidden), | ||
| })) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let t = if let SendViewType::File(_) = self.view_type { | ||
| bitwarden_api_api::models::SendType::File | ||
| } else { | ||
| bitwarden_api_api::models::SendType::Text | ||
| }; |
There was a problem hiding this comment.
suggestion: I think using a match that returns a tuple would be nicer here.
The main reason is to improve readability. It's easier to reason about one branch than it is to reason about 3 "different" branches.
let (send_type, file, text) = match self.view_type.clone() {
SendViewType::File(f) => (
bitwarden_api_api::models::SendType::File,
Some(Box::new(bitwarden_api_api::models::SendFileModel {
id: f.id.clone(),
file_name: Some(f.file_name.encrypt(ctx, send_key)?.to_string()),
size: f.size.as_ref().and_then(|s| s.parse::<i64>().ok()),
size_name: f.size_name.clone(),
})),
None,
),
SendViewType::Text(t) => (
bitwarden_api_api::models::SendType::Text,
None,
Some(Box::new(bitwarden_api_api::models::SendTextModel {
text: t
.text
.as_ref()
.map(|txt| txt.encrypt(ctx, send_key))
.transpose()?
.map(|e| e.to_string()),
hidden: Some(t.hidden),
})),
),
};Since it's used in both create and edit it can be extracted into a method on SendViewType to ensure consistent behaviour.
| deletion_date: self.deletion_date.to_rfc3339(), | ||
| file, | ||
| text, | ||
| password: self.password.clone(), |
There was a problem hiding this comment.
issue: The password should go through pbkdf2 right?
This is the code fo the current encrypt_composite in SendView
self.new_password.as_ref().map(|password| {
let password = bitwarden_crypto::pbkdf2(password.as_bytes(), &k, SEND_ITERATIONS);
B64::from(password.as_slice()).to_string()
}),
|
6 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31067
📔 Objective
Add calls to the SendClient that adds/edits/gets/lists sends via API calls.
🚨 Breaking Changes