[PM-25821] Migrate Cipher Admin operation API calls to SDK #560
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
==========================================
+ Coverage 79.46% 80.18% +0.72%
==========================================
Files 302 304 +2
Lines 32331 33697 +1366
==========================================
+ Hits 25691 27020 +1329
- Misses 6640 6677 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nikwithak
force-pushed
the
vault/pm-25821/cipher-admin-ops
branch
from
aa8a96d to
7dc66a5
Compare
… vault/pm-25821/cipher-admin-ops
|
Claude finished @nikwithak's task —— View job Code Review CompleteI've reviewed this PR which migrates Cipher Admin operation API calls to the SDK. Here's my comprehensive feedback: SummaryThis PR successfully implements 15 new admin operation methods for PR Title & Description Assessment💭 Finding 1: The PR title and description are clear and appropriate, though the description could benefit from more detail about the test coverage strategy. Suggested enhancementConsider adding to the description:
Critical Issues❌ Finding 2: Repository update inconsistency in delete operations (delete.rs:28-42) The // delete_as_admin - no repository update
pub async fn delete_as_admin(&self, cipher_id: CipherId) -> Result<(), ApiError> {
let configs = self.get_api_configurations().await;
let api = configs.api_client.ciphers_api();
api.delete_admin(cipher_id.into()).await?;
Ok(()) // ⚠️ Repository not updated
}
// delete - has repository update
pub async fn delete(&self, cipher_id: CipherId) -> Result<(), DeleteCipherError> {
let configs = self.get_api_configurations().await;
let api = configs.api_client.ciphers_api();
api.delete(cipher_id.into()).await?;
self.get_repository()?.remove(cipher_id.to_string()).await?; // ✓ Repository updated
Ok(())
}Impact: Admin users may see deleted ciphers still appear in their local state until a full sync occurs. Recommendation: Either:
❌ Finding 3: Inconsistent collection consumption patterns (delete.rs:54, 69, 139) Methods use inconsistent approaches for consuming
// Inconsistency example:
// delete_many uses iter() + clone
ids: cipher_ids.iter().map(|id| id.to_string()).collect(),
// ...then later
for cipher_id in cipher_ids { // This works but is confusing
// delete_many_as_admin uses into_iter()
ids: cipher_ids.into_iter().map(|id| id.to_string()).collect(),Recommendation: Use Repository update failures in batch operations don't roll back API changes, potentially causing state divergence: for cipher_id in cipher_ids {
self.get_repository()?.remove(cipher_id.to_string()).await?; // ⚠️ Fails partway through
}If the repository update fails on the 3rd of 5 ciphers, the first 2 are removed from local state but the last 3 remain, even though all 5 were deleted on the server. Recommendation: Consider collecting results and handling partial failures explicitly, or document that repository sync should be used to recover from such scenarios. Minor Issues & Improvements
The method has an explicit Recommendation: Document that admin endpoints:
♻️ Finding 6: Duplicate test setup code across files (delete.rs:170-227, restore.rs:140-197, create.rs:388-445, edit.rs:531-588) The Technical debt: This violates DRY principles and makes test maintenance harder. If the crypto initialization logic needs to change, it must be updated in 4 places. Recommendation: Extract to a shared test utilities module, e.g., 🎨 Finding 7: Test organization inconsistency (restore.rs:239-505) The restore tests have very comprehensive coverage including failure modes and edge cases, while other operation files have more basic coverage. This inconsistency makes it unclear what the coverage standard is. Recommendation: Consider documenting test coverage expectations in contributing docs or add similar coverage levels to other operations. 🎨 Finding 8: Ambiguous method naming for batch operations Methods like pub async fn delete_many(
&self,
cipher_ids: Vec<CipherId>,
organization_id: Option<OrganizationId>, // Not obvious from method name
) -> Result<(), DeleteCipherError>Recommendation: Consider method names like 💭 Finding 9: Low test coverage reported by Codecov (5% patch coverage) The PR has 342 lines without test coverage despite having extensive unit tests. This suggests the tests aren't running in CI or aren't being counted properly. Investigation needed: The test code itself is comprehensive, so this might be:
Recommendation: Verify tests are actually running in CI and investigate coverage reporting configuration. Security Considerations✓ No sensitive data logging detected: The code properly avoids logging cipher IDs, encrypted data, or other sensitive information in error paths. ✓ Error types properly abstracted: Using 💭 Finding 10: Consider rate limiting implications for batch operations Methods like for cipher_id in cipher_ids {
self.get_repository()?.remove(cipher_id.to_string()).await?;
}For large batches, this could cause performance issues or rate limiting. Consider whether batched repository operations would be more efficient. Good Practices ObservedThe following excellent practices were noted:
Action Items for Author
Testing RecommendationsThe unit tests are well-written, but consider adding:
|
|
Be mindful that the autogenerated API bindings do not have any graceful handling of malformed data. I.e. an enum outside the valid range errors out. https://bitwarden.atlassian.net/browse/PM-6169 tracks improvements to this, and if this improvement is required reach out to platform to ensure it's prioritized. CC @trmartin4 |
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
I would consider moving all the admin commands into a separate client. While they share some common logic I think they fundamentally behave differently and it might be confusing for consumers to call an admin method and not have the state react?
CiphersClient & CiphersAdminClient
| /// Generate a new key for the cipher, re-encrypting internal data, if necessary, and stores the | ||
| /// encrypted key to the cipher data. | ||
| fn generate_cipher_key( | ||
| pub(crate) fn generate_cipher_key( |
There was a problem hiding this comment.
question: Is this change needed? It doesn't look like it's used outside this module.
There was a problem hiding this comment.
Originally I had re-used this for a separate admin file, but I ended up adding it to the same file. I thought I already removed this, will fix.
There was a problem hiding this comment.
I left this in (changed to pub(super) so it can be re-used by the CipherAdminClient since it shares the request types.
| repository: &R, | ||
| encrypted_for: UserId, | ||
| request: CipherCreateRequestInternal, | ||
| collection_ids: Vec<CollectionId>, |
There was a problem hiding this comment.
question: CipherCreateRequest has organization_id, is there a reason organization is part of the cipher create request while collections are part of the method? Collections are tightly coupled to an organization so this split does not make much sense.
There was a problem hiding this comment.
This object ends up mapping directly to the CreateCipherRequestModel, which is re-used across the different endpoints to create a cipher (POST /cipher, POST /cipher/create, POST /cipher/admin), even though POST /cipher does not take collection_ids. I will play around with ways to re-structure these in a way that fits what you're describing.
There was a problem hiding this comment.
I think we can just leave it for now, but would be nice to clean it up in the future.
| if as_admin && cipher_request.organization_id.is_some() { | ||
| cipher = api_client | ||
| .ciphers_api() | ||
| .post_admin(Some(CipherCreateRequestModel { | ||
| collection_ids: Some(collection_ids.into_iter().map(Into::into).collect()), | ||
| cipher: Box::new(cipher_request), | ||
| })) | ||
| .await? | ||
| .try_into()?; | ||
| } else if !collection_ids.is_empty() { | ||
| cipher = api_client | ||
| .ciphers_api() | ||
| .post_create(Some(CipherCreateRequestModel { | ||
| collection_ids: Some(collection_ids.into_iter().map(Into::into).collect()), | ||
| cipher: Box::new(cipher_request), | ||
| })) | ||
| .await | ||
| .map_err(ApiError::from)? | ||
| .try_into()?; | ||
| repository | ||
| .set(require!(cipher.id).to_string(), cipher.clone()) | ||
| .await?; | ||
| } else { | ||
| cipher = api_client | ||
| .ciphers_api() | ||
| .post(Some(cipher_request)) | ||
| .await | ||
| .map_err(ApiError::from)? | ||
| .try_into()?; | ||
| repository | ||
| .set(require!(cipher.id).to_string(), cipher.clone()) | ||
| .await?; |
There was a problem hiding this comment.
question: What's going on here? It doesn't really make much sense to have three different endpoints for creating ciphers.
- Is
ciphers/createnot identical to doing post onciphers? - From a REST perspective having a dedicated admin endpoint is also quite weird.
There was a problem hiding this comment.
The three endpoints is because of the way the backend is structured today.
POST /ciphersis only used for creating an individual cipher. Even though it accepts anorganization_idin the request, it does not have anywhere to addcollection_ids.POST /ciphers/createis how we create an org cipher, as it accepts thecollection_ids.- The admin endpoint has its own uses that do a separate set of permission checks than the standard endpoint.
There was a problem hiding this comment.
I updated this so that it only calls two endpoints (POST /ciphers or POST /ciphers/create), adn moved the third (/ciphers/admin) to a separate file / client.
| /// Creates a new [Cipher] and saves it to the server. | ||
| pub async fn create( | ||
| &self, | ||
| request: CipherCreateRequest, | ||
| ) -> Result<CipherView, CreateCipherError> { | ||
| self.create_cipher(request, vec![], false).await | ||
| } | ||
|
|
||
| /// Creates a new [Cipher] for an organization, and saves it to the server. | ||
| pub async fn create_org_cipher( | ||
| &self, | ||
| request: CipherCreateRequest, | ||
| collection_ids: Vec<CollectionId>, | ||
| ) -> Result<CipherView, CreateCipherError> { | ||
| self.create_cipher(request, collection_ids, false).await | ||
| } |
There was a problem hiding this comment.
The contract here is weird. CipherCreteRequest allows you to specify an organization id. That implies you can make orgnaizational items.
There was a problem hiding this comment.
I agree here - the CipherCreateRequest was built to match the valid fields on the API request model, but when we create an org cipher, the client calls the separate (POST /ciphers/create) endpoint, which takes the collection_Ids. The standard POST /ciphers endpoint does not have collection_ids in the response model.
I'm thinking of modifying the CipherCreateRequest to have both org_id and collection_ids, and writing the logic to call the POST /ciphers/create endpoint iff they are both present on the request; LMK your thoughts?
| use wiremock::{ | ||
| Mock, ResponseTemplate, | ||
| matchers::{method, path}, | ||
| }; |
There was a problem hiding this comment.
issue: We've stopped using wiremock for api mocking. You can look at crates/bitwarden-vault/src/folder/edit.rs for up to date exampels of using ApiClient::new_mocked.
There was a problem hiding this comment.
Updated - now uses standalone methods, and updated unit tests to test those standalones using ApiClient::new_mocked with passed-in dependencies.
|
|
||
| impl CipherEditRequest { | ||
| fn generate_cipher_key( | ||
| pub(crate) fn generate_cipher_key( |
There was a problem hiding this comment.
question: this doesn't seem used outside of this module.
| #[error(transparent)] | ||
| Decrypt(#[from] DecryptError), |
There was a problem hiding this comment.
question: What operation requires decrypt?
There was a problem hiding this comment.
Since the function input for these requests is plaintext, and the SDK handles encryption before sending to the server, we return the updated object decrypted as well. example
We return a CryptoError in other instances (e.g. the edit_cipher implementation here), but this call returns a DecryptError. I will look into casting it approriately to use the same error.
Tangentially: Do you think it makes sense to use this approach - returning the decrypted CipherView object to the consumer, rather than the encrypted Cipher (all of it locally only)
| // putCipherAdmin(id, request: CipherRequest) | ||
| // ciphers_id_admin_put | ||
| #[allow(missing_docs)] // TODO: add docs |
| #[error(transparent)] | ||
| Api(#[from] ApiError), |
There was a problem hiding this comment.
The methods putting Api errors in CipherError should really be updated to not use cipher error.
There was a problem hiding this comment.
The only one still using CipherError is the share operations, which calls existing functions that currently returns CipherError already (e.g. https://github.com/bitwarden/sdk-internal/blob/vault/pm-25821/cipher-admin-ops/crates/bitwarden-vault/src/cipher/cipher_client/share_cipher.rs#L180-L184) - I think we can migrate this one to its own error type in the future.
| edit: Default::default(), | ||
| favorite: Default::default(), | ||
| folder_id: Default::default(), | ||
| permissions: Default::default(), | ||
| view_password: Default::default(), | ||
| local_data: Default::default(), | ||
| collection_ids: Default::default(), |
There was a problem hiding this comment.
reflection: Putting default values here can be dangerous, because there is nothing to indicate the Cipher model is incomplete.
There was a problem hiding this comment.
Unfortunately several API operations don't return the full cipher, only the CipherMiniResponseModel - for the Admin operations, since we don't update the repository, the intent is to still return a consistent type the user can handle, if needed - the concern is valid though. I think we can:
- Merge it with the known existing Cipher data (if it's available),
- Continue with this approach, knowing it's primarily for the admin operations which don't update local state - if this route, I can move the logic to be private to the admin operations, rather than a blanket
Fromimplementation, - Return the
CipherMiniResponseModelas-is, or create aMiniCipherViewtype for mapping / returning,which doesn't expose these fields at all.
Do you have a preference? Feel free to ping me on Slack when you're free for a deeper discussion - there are a handful of API operations that currently only return a subset of data like this.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-25821
📔 Objective
Migrates the logic for orchestrating API calls for several operations related to the Admin API functionality. Adds the following operations to
CiphersClient, which call the appropriate API endpoints:PR Notes: Sorry for the heft - The line count is high on this PR, but a majority of it is unit tests. This should have been split into multiple tickets, in hindsight.
Note also that this hasn't been tested directly with the client yet, and so additional changes may be needed if bugs are discovered when integrating into the clients (future tickets) - these operations are not currently used anywhere.
⏰ 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