PM-23649: CXF Import note #382
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abergs
requested review from
addisonbeck and
Hinton
and removed request for
addisonbeck
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## cxf/sample #382 +/- ##
==============================================
+ Coverage 74.37% 74.61% +0.23%
==============================================
Files 253 254 +1
Lines 21918 22138 +220
==============================================
+ Hits 16302 16518 +216
- Misses 5616 5620 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hinton
reviewed
Aug 14, 2025
Comment on lines
78
to
258
|
|
||
| assert_eq!(cipher.fields.len(), 0); // Notes don't have custom fields | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_note_as_part_of_login() { | ||
| use credential_exchange_format::{BasicAuthCredential, Credential, Item}; | ||
|
|
||
| use crate::{cxf::import::parse_item, CipherType, ImportingCipher}; | ||
|
|
||
| let item = Item { | ||
| id: [0, 1, 2, 3, 4, 5, 6].as_ref().into(), | ||
| creation_at: Some(1706613834), | ||
| modified_at: Some(1706623773), | ||
| title: "Login with Note".to_string(), | ||
| subtitle: None, | ||
| favorite: None, | ||
| credentials: vec![ | ||
| Credential::BasicAuth(Box::new(BasicAuthCredential { | ||
| username: Some("testuser".to_string().into()), | ||
| password: Some("testpass".to_string().into()), | ||
| })), | ||
| Credential::Note(Box::new(NoteCredential { | ||
| content: "This note should be added to the login cipher." | ||
| .to_string() | ||
| .into(), | ||
| })), | ||
| ], | ||
| tags: None, | ||
| extensions: None, | ||
| scope: None, | ||
| }; | ||
|
|
||
| let ciphers: Vec<ImportingCipher> = parse_item(item); | ||
| assert_eq!(ciphers.len(), 1); // Should create only one cipher (Login with note content) | ||
| let cipher = ciphers.first().unwrap(); | ||
|
|
||
| assert_eq!(cipher.name, "Login with Note"); | ||
| assert_eq!( | ||
| cipher.notes, | ||
| Some("This note should be added to the login cipher.".to_string()) | ||
| ); | ||
|
|
||
| match &cipher.r#type { | ||
| CipherType::Login(_) => (), // Should be a Login cipher | ||
| _ => panic!("Expected Login cipher with note content"), | ||
| }; | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_note_as_part_of_api_key() { | ||
| use credential_exchange_format::{ApiKeyCredential, Credential, Item}; | ||
|
|
||
| use crate::{cxf::import::parse_item, CipherType, ImportingCipher}; | ||
|
|
||
| let item = Item { | ||
| id: [0, 1, 2, 3, 4, 5, 6].as_ref().into(), | ||
| creation_at: Some(1706613834), | ||
| modified_at: Some(1706623773), | ||
| title: "API Key with Note".to_string(), | ||
| subtitle: None, | ||
| favorite: None, | ||
| credentials: vec![ | ||
| Credential::ApiKey(Box::new(ApiKeyCredential { | ||
| key: Some("api-key-12345".to_string().into()), | ||
| username: Some("api-user".to_string().into()), | ||
| key_type: Some("Bearer".to_string().into()), | ||
| url: None, | ||
| valid_from: None, | ||
| expiry_date: None, | ||
| })), | ||
| Credential::Note(Box::new(NoteCredential { | ||
| content: "This note should be added to the API key cipher." | ||
| .to_string() | ||
| .into(), | ||
| })), | ||
| ], | ||
| tags: None, | ||
| extensions: None, | ||
| scope: None, | ||
| }; | ||
|
|
||
| let ciphers: Vec<ImportingCipher> = parse_item(item); | ||
| assert_eq!(ciphers.len(), 1); // Should create only one cipher (SecureNote with note content) | ||
| let cipher = ciphers.first().unwrap(); | ||
|
|
||
| assert_eq!(cipher.name, "API Key with Note"); | ||
| assert_eq!( | ||
| cipher.notes, | ||
| Some("This note should be added to the API key cipher.".to_string()) | ||
| ); | ||
|
|
||
| match &cipher.r#type { | ||
| CipherType::SecureNote(_) => (), // Should be a SecureNote cipher | ||
| _ => panic!("Expected SecureNote cipher with note content"), | ||
| }; | ||
|
|
||
| // Should have API key fields | ||
| assert!(!cipher.fields.is_empty()); | ||
| } |
There was a problem hiding this comment.
issue: This is testing the import logic.
There was a problem hiding this comment.
Yes it does, but that's because the way we handle notes are kind tied into the import logic. The reason I'm keeping it in note.rs is because the whole world falls on my head in terms of merge conflicts if I don't while I do these separate PRs. I'm happy to move refactor the tests once all the mapping is in the same branch.
|
Hinton
approved these changes
Aug 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-23649
📔 Objective
This PR maps the Note type, and adds mapping the note for existing vault items.
Note: A separate task has been created to track that we're mapping notes for all other cipheritems (some are not created yet).
⏰ 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