[PM-25818] Migrate Basic Cipher Create, Edit, and Get Operations to SDK

[gbubemismith]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-25818

📔 Objective

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  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 confirmed
  issue 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

[github-actions]

Checkmarx One – Scan Summary & Details – 24e3be60-0d10-4d8f-b0ca-40bdfcb00023

Great job! No new security vulnerabilities introduced in this pull request

[nick-livefront]

Just a handful of questions from me. I'm following this and the implementation but I would say that my rust experience is a little lack luster on if there are improvements to be made on that thread. I will hold off on approving to hopefully solicit some more impactful feedback, but I can if I'm the gateway between moving this forward.

[nick-livefront]

❓ Probably an edge case, the current CipherView defaults these values to false rather than true. Should this logic follow suit or is the deviation intentional?

[nick-livefront]

I did not know you could link to other entities in a comment with [...], TIL!

[nikwithak]

Would it be better to use decrypt_list_failures here to avoid blocking when only some of them fail to decrypt?

[nikwithak]

Thanks for adding list_ciphers_with_failures! Do you think we still need the list_ciphers for any use cases, or could we remove it altogether?

[codecov]

Codecov Report

❌ Patch coverage is 78.29648% with 265 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.08%. Comparing base (b5d27e4) to head (ed2f8e7).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...bitwarden-vault/src/cipher/cipher_client/create.rs	85.35%	46 Missing ⚠️
crates/bitwarden-vault/src/cipher/login.rs	47.05%	36 Missing ⚠️
...s/bitwarden-vault/src/cipher/cipher_client/edit.rs	94.46%	33 Missing ⚠️
...es/bitwarden-vault/src/cipher/cipher_client/get.rs	0.00%	33 Missing ⚠️
...tes/bitwarden-vault/src/cipher/cipher_view_type.rs	48.27%	30 Missing ⚠️
crates/bitwarden-vault/src/cipher/identity.rs	0.00%	22 Missing ⚠️
crates/bitwarden-vault/src/cipher/field.rs	0.00%	15 Missing ⚠️
crates/bitwarden-vault/src/cipher/card.rs	0.00%	10 Missing ⚠️
crates/bitwarden-vault/src/cipher/cipher.rs	84.12%	10 Missing ⚠️
crates/bitwarden-vault/src/cipher/secure_note.rs	0.00%	10 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
- Coverage   78.10%   78.08%   -0.02%     
==========================================
  Files         283      291       +8     
  Lines       27628    28885    +1257     
==========================================
+ Hits        21579    22556     +977     
- Misses       6049     6329     +280     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nikwithak]

Okay I think I got everything @Hinton @shane-melton - a bit of a refactor with the recent commits, so please look carefully to make sure I didn't miss anything

[Hinton]

I think you only updated edit and forgot about create?

[Hinton]

I think if you want a nicer interface for this you can create a internal struct that encapsulates the outside data.

[nikwithak]

If we were re-using it elsewhere I think that would make sense, but since the need for it is isolated to this one function, I'd prefer to leave it as-is if you're fine with it.

[Hinton]

The problem is that both CipherEditRequest and CompositeEncryptable are public. Which means that anyone (both internal to the vault crate but also externally) can call encrypt_composite on it. With the current logic you get back a CipherEditRequest which someone might believe they can post to our API. Without knowing that it lacks several required fields.

If it was just internal misuse I would be a bit less concerned but I could imagine someone calling encrypt on CipherEditRequest.

Encapsulating it in an internal struct prevents this sort of misuse. It's not the biggest thing but encoding this into the types prevents this from ever occurring.

[nikwithak]

Yes, makes perfect sense. I added a CipherEditRequestInternal, and modified the password history function to update it directly (rather than return the result). I also changed the password history tests to test on the output of CipherEditRequestInternal::new to make sure it gets set properly during the conversion.

[nikwithak]

@Hinton I took this approach for CipherCreateRequest too, since we had a prior discussion about flagging key as something the consumer shouldn't set - with this new approach, we have the same protections around that field on create.

[nikwithak]

I think you only updated edit and forgot about create?

Nice catch - I updated type_data: Option<CipherViewType> -> r#type: CipherViewType. I did away with the Default implementation as well in favor of just updating the test generator function, to avoid making assumptions about the CipherType.

I also added in the wasm_bindgen attributes to the CiphersClient::edit and CiphersClient::create with this update.

[Hinton]

I think the two visbility issues are the only blockers now.,

[Hinton]

issue: Should the methods here be public?

If yes: Please document them.
If no: Please lower the visibility and remove the allow(missing_docs)

I think no since we don't want people to manually modify the history.

[nikwithak]

Updated to pub(crate)

[Hinton]

question: Does this need to be public? If not you can lower the visibility and remove the allow. You can probably also get rid of the private trait.

[nikwithak]

I did away with this trait altogether - since we no longer have Option<CipherViewType> in the structs, it sin't needed. I just implemented those functions on CipherViewType directly.

[claude]

Claude finished @gbubemismith's task —— View job

---

Code Review: Migrate Basic Cipher Create, Edit, and Get Operations to SDK

Overall Assessment

This is a substantial and well-structured PR that successfully migrates cipher CRUD operations to the SDK. The code demonstrates good architecture with proper separation of concerns, robust error handling, and comprehensive testing. There are several strengths worth highlighting and a few areas for improvement.

🎯 Strengths

Architecture & Design

• Clean separation: Operations are properly split into separate modules (create.rs, edit.rs, get.rs)
• Type safety: Good use of Rust's type system with proper error variants and structured data
• Enum-based design: The new CipherViewType enum provides better type safety than the previous flattened structure

Security Implementation

• Cipher key encryption: Proper implementation of cipher key generation and encryption with feature flag support
• URI checksums: Correctly implemented URI checksum generation for login items
• FIDO2 re-encryption: Handles FIDO2 credential re-encryption when cipher keys change
• Password history: Comprehensive password history tracking with proper limits and field change detection

Error Handling

• Comprehensive error types: Well-defined error enums with proper error propagation
• Graceful degradation: The decrypt_list_with_failures approach handles partial decryption failures gracefully

⚠️ Areas for Improvement

1. Public API Surface (High Priority)

Issue: Several methods and structs are marked as public but shouldn't be exposed to external consumers.

Files affected:

• password_history.rs:92-105 - new_password and new_field methods
• cipher_view_type.rs:34-104 - Helper methods marked with #[allow(unused)]

Recommendation: Make these methods pub(crate) or private to prevent misuse.

2. Documentation (Medium Priority)

Issue: Missing documentation on several public APIs.

Files affected:

• create.rs:59 - key field needs documentation about being internal-only
• edit.rs:54 - CipherEditRequest struct needs documentation
• Various public methods lack comprehensive documentation

Recommendation: Add comprehensive documentation, especially for public APIs that will be used by clients.

3. Code Organization (Medium Priority)

Issue: The CipherEditRequestInternal struct duplicates some logic and could be better encapsulated.

Location: edit.rs:103-140

Recommendation: Consider making the encryption implementation private to prevent external misuse, as noted in reviewer feedback.

4. Test Coverage Gaps (Medium Priority)

Issue: Codecov shows 78.14% patch coverage with 264 lines missing coverage.

Key areas lacking coverage:

• cipher_view_type.rs - 30 missing lines (48.27% coverage)
• get.rs - 33 missing lines (0% coverage)
• Various edge cases in cipher type handling

Recommendation: Add tests for the uncovered code paths, especially error conditions and edge cases.

🔧 Technical Implementation Review

Create Operations (create.rs)

✅ Strengths:

• Proper cipher key generation with feature flag support
• Comprehensive encryption implementation
• Good test coverage with mocked API calls

⚠️ Considerations:

• URI checksum generation is correctly implemented
• Encrypted_for field is properly set internally

Edit Operations (edit.rs)

✅ Strengths:

• Sophisticated password history tracking
• Handles both login passwords and hidden field changes
• Proper revision date management
• Excellent test coverage with multiple scenarios

⚠️ Considerations:

• Complex password history logic is well-tested but could benefit from more edge case coverage
• The intermediary CipherEditRequestInternal struct is a good design choice for safety

Get Operations (get.rs)

✅ Strengths:

• Clean, simple implementation
• Proper error handling with ItemNotFoundError

⚠️ Considerations:

• Very low test coverage (0% according to Codecov)
• Could benefit from additional tests for error conditions

🔍 Security Review

The security implementation appears sound:

1. Encryption patterns: Follows established patterns for symmetric encryption
2. Key management: Proper use of cipher keys with feature flag support
3. Data integrity: URI checksums and proper FIDO2 handling
4. History tracking: Secure password history with proper limits

📝 Recommendations

High Priority

1. Reduce public API surface - Make internal helper methods private
2. Add missing documentation - Especially for client-facing APIs

Medium Priority

1. Improve test coverage - Focus on get.rs and edge cases
2. Consider API ergonomics - The conversion from CipherView to CipherEditRequest could be smoother

Low Priority

1. Code organization - Consider if some complex logic could be extracted into smaller, focused functions

🎉 Conclusion

This is a solid implementation that successfully migrates cipher operations to the SDK while maintaining security and adding useful features like password history tracking. The code demonstrates good Rust practices and thorough testing. With the suggested improvements around API visibility and test coverage, this will be a robust foundation for client applications.

The reviewers' feedback has been well-addressed, and the implementation follows Bitwarden's established patterns for encryption and data handling.

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.9% Duplication on New Code

See analysis details on SonarQube Cloud

[Hinton]

Looks good to me, I think you can simplify the internal structs and just wrap the request.

[Hinton]

suggestion: I think we can just wrap the public request?

Suggested change

	struct CipherCreateRequestInternal {
	organization_id: Option<OrganizationId>,
	folder_id: Option<FolderId>,
	name: String,
	notes: Option<String>,
	favorite: bool,
	reprompt: CipherRepromptType,
	r#type: CipherViewType,
	fields: Vec<FieldView>,
	key: Option<EncString>,
	}
	struct CipherCreateRequestInternal {
	request: CipherCreateRequest,
	key: Option<EncString>,
	}

[Hinton]

suggestion: Wrap with the request and you should be able to avoid some of the duplicate code?