[PM-25821] Migrate Cipher Admin operation API calls to SDK #560

nikwithak · 2025-11-15T00:53:25Z

🎟️ 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:

delete
delete_many
delete_as_admin
delete_many_as_admin
soft_delete
soft_delete_many
soft_delete_as_admin
soft_delete_many_as_admin
restore
restore_many
restore_as_admin
restore_many_as_admin
edit_as_admin
list_org_ciphers
create_as_admin

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

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 · 2025-11-15T00:57:53Z

Checkmarx One – Scan Summary & Details – 9d652621-fd4c-4e3b-bed9-63ba3cb17fcd

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

github-actions · 2025-11-15T00:59:45Z

🔍 SDK Breaking Change Detection Results

SDK Version: vault/pm-25821/cipher-admin-ops (ca6ae3f)
Completed: 2025-12-08 18:57:52 UTC
Total Time: 217s

Client	Status	Details
typescript	✅ No breaking changes detected	TypeScript compilation passed with new SDK version - View Details

Breaking change detection completed. View SDK workflow

codecov · 2025-11-15T01:08:23Z

Codecov Report

❌ Patch coverage is 76.31579% with 360 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.64%. Comparing base (0d52f61) to head (ca6ae3f).

Files with missing lines	Patch %	Lines
crates/bitwarden-vault/src/cipher/cipher.rs	50.79%	62 Missing ⚠️
...arden-vault/src/cipher/cipher_client/admin/edit.rs	71.85%	56 Missing ⚠️
...den-vault/src/cipher/cipher_client/admin/delete.rs	71.26%	50 Missing ⚠️
...bitwarden-vault/src/cipher/cipher_client/delete.rs	83.39%	43 Missing ⚠️
...den-vault/src/cipher/cipher_client/admin/create.rs	70.79%	33 Missing ⚠️
...s/bitwarden-vault/src/cipher/cipher_client/edit.rs	11.42%	31 Missing ⚠️
...en-vault/src/cipher/cipher_client/admin/restore.rs	85.92%	28 Missing ⚠️
...itwarden-vault/src/cipher/cipher_client/restore.rs	89.07%	26 Missing ⚠️
...warden-vault/src/cipher/cipher_client/admin/get.rs	0.00%	22 Missing ⚠️
...bitwarden-vault/src/cipher/cipher_client/create.rs	94.19%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
+ Coverage   78.59%   78.64%   +0.05%     
==========================================
  Files         281      288       +7     
  Lines       29131    30635    +1504     
==========================================
+ Hits        22895    24093    +1198     
- Misses       6236     6542     +306

☔ 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.

…ling

… vault/pm-25821/cipher-admin-ops

claude · 2025-11-26T22:16:50Z

Claude finished @nikwithak's task —— View job

Code Review Complete

Todo List

Gather PR context and understand changes
Run comprehensive code review using bitwarden-code-reviewer agent
Post review findings

Overall Assessment: REQUEST CHANGES

I've completed a comprehensive code review of PR #560. The code follows good architectural patterns and has strong test coverage, but there are 2 critical issues that must be fixed before merge.

Critical Issues Found

❌ Finding 1: Dead Code - Unused KeyStore instantiation in restore_many

Location: crates/bitwarden-vault/src/cipher/cipher_client/restore.rs:60-65

The restore_many function creates a new KeyStore instance with a randomly generated key, but this store is never used. The function then correctly uses the key_store parameter for decryption. This is dead code that creates unnecessary cryptographic key material in memory.

let store: KeyStore<KeyIds> = KeyStore::default();
#[allow(deprecated)]
let _ = store.context_mut().set_symmetric_key(
    SymmetricKeyId::User,
    SymmetricCryptoKey::make_aes256_cbc_hmac_key(),
);

Fix: Remove lines 60-65 entirely. The function correctly uses the key_store parameter for decryption.

⚠️ Finding 2: organization_id hardcoded to None in restore_many

Location: crates/bitwarden-vault/src/cipher/cipher_client/restore.rs:70

The organization_id field is hardcoded to None in the API request model, even though restore operations can apply to organization ciphers. This differs from the admin version (restore_many_as_admin) which correctly accepts and passes an organization_id parameter.

api.put_restore_many(Some(CipherBulkRestoreRequestModel {
    ids: cipher_ids.into_iter().map(|id| id.to_string()).collect(),
    organization_id: None,  // ← Hardcoded to None
}))

Fix: Add an organization_id: Option<OrganizationId> parameter to the restore_many function (matching the delete_many pattern) and pass it to the request model.

Impact: This prevents users from restoring organization ciphers using the non-admin endpoint.

Important Design Issues

♻️ Finding 3: Admin operations don't update local repository

Location: All admin operation files

Admin operations explicitly do not update the local repository (as documented: "Affects server data only, does not modify local state"), while non-admin operations do. This creates an inconsistency where admin users must manually refresh their local state after admin operations.

Recommendation: This appears intentional, but document this behavioral difference prominently in the public API docs. Since the PR description notes "these operations are not currently used anywhere," this should be validated during client integration.

🎨 Finding 4: Missing test coverage for error paths

Location: Multiple test files

Codecov reports 90.27% patch coverage with 135 lines missing. The tests have good happy-path coverage but limited error-path testing.

Recommendation: Add tests for:

API call failures (network errors, 4xx/5xx responses)
Decryption failures with malformed data
Repository errors
Edge cases (empty lists, missing IDs)

❓ Finding 5: Inconsistent API between admin and non-admin edit operations

Location: edit.rs vs admin/edit.rs

The admin edit function requires passing original_cipher_view as a parameter, while the non-admin version fetches it from the repository internally. This creates an inconsistent API.

Question: Why does the admin endpoint require the caller to provide this? Consider documenting the reason or making the API consistent.

Positive Observations

The PR demonstrates several excellent patterns:

✅ Clean separation between admin and non-admin operations via separate client types
✅ Consistent error handling using the bitwarden_error macro
✅ Good test coverage for happy paths with mocked API clients
✅ Proper use of repository pattern for state management
✅ No security vulnerabilities identified
✅ No hardcoded secrets or sensitive data exposure
✅ Proper encryption key handling through KeyStore abstraction
✅ Adheres to CLAUDE.md architectural guidelines

Security Considerations

No security vulnerabilities identified. The code properly handles encryption keys, has no logging of sensitive data, no hardcoded credentials, and follows established cryptographic patterns.

Recommendations Summary

MUST FIX before merge:

Remove dead code in restore_many (lines 60-65 in restore.rs)
Add organization_id parameter to restore_many function

SHOULD address:
3. Add test coverage for error paths to reach >95% coverage
4. Document the admin vs non-admin local state behavior difference
5. Clarify why admin edit requires original_cipher_view parameter

CONSIDER:
6. Whether admin operations should have an option to update local state
7. Adding integration tests once client integration begins

Files reviewed: All 13 changed files including cipher.rs, cipher_client/mod.rs, and all admin operation files.

Hinton · 2025-11-27T11:43:15Z

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

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

crates/bitwarden-vault/src/cipher/cipher_client/create.rs

Hinton · 2025-11-27T11:54:31Z

crates/bitwarden-vault/src/cipher/cipher_client/create.rs

+    /// 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
+    }


The contract here is weird. CipherCreteRequest allows you to specify an organization id. That implies you can make orgnaizational items.

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?

Yea that seems saner imho. Ideally we can combine the server endpoints at some point?

crates/bitwarden-vault/src/cipher/cipher_client/create.rs

crates/bitwarden-vault/src/cipher/cipher_client/edit.rs

Hinton · 2025-11-27T13:02:33Z

crates/bitwarden-vault/src/cipher/cipher_client/edit.rs

+    #[error(transparent)]
+    Decrypt(#[from] DecryptError),


question: What operation requires decrypt?

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)

In the long term I would love to have Cipher be an internal concern of the SDK and we only expose decrypted views. Now in the meanwhile we have to make tradeoffs to move us forward and I don't think it's an issue.

Upcasting it to a CryptoError is probably better. I think that should be as simple as doing CryptoError::Decrypt(err)

crates/bitwarden-vault/src/cipher/cipher_client/edit.rs

Hinton · 2025-11-27T13:05:36Z

crates/bitwarden-vault/src/cipher/cipher.rs

+    #[error(transparent)]
+    Api(#[from] ApiError),


The methods putting Api errors in CipherError should really be updated to not use cipher error.

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.

Yea sounds good.

Hinton · 2025-11-27T13:06:36Z

crates/bitwarden-vault/src/cipher/cipher.rs

+            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(),


reflection: Putting default values here can be dangerous, because there is nothing to indicate the Cipher model is incomplete.

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 From implementation,

Return the CipherMiniResponseModel as-is, or create a MiniCipherView type 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.

I think we just need to be careful. In some ways It would be nice to make an alias to indicate "hey you can't trust this object", or define a new struct with fields optional.

This is not a blocking.

I added a new PartialCipher trait that can merge with an Option<Cipher>, replacing the TryFrom implementaiton: 58e01df

… vault/pm-25821/cipher-admin-ops

…ponses

Hinton

Most of my issues are resolved and I think this looks generally good. There are couple of smaller things and improvements to be made but once resolved feel free to not request another review from me.

Hinton · 2025-12-18T13:43:41Z

crates/bitwarden-vault/src/cipher/cipher_client/admin/mod.rs

+mod get;
+mod restore;
+
+#[allow(missing_docs)]


suggestion: Document the purpose of this client and compare it against the regular CipherClient?

Hinton · 2025-12-18T13:46:18Z

crates/bitwarden-vault/src/cipher/cipher.rs

+            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(),


I think we just need to be careful. In some ways It would be nice to make an alias to indicate "hey you can't trust this object", or define a new struct with fields optional.

This is not a blocking.

Hinton · 2025-12-18T13:46:28Z

crates/bitwarden-vault/src/cipher/cipher.rs

+    #[error(transparent)]
+    Api(#[from] ApiError),


Yea sounds good.

Hinton · 2025-12-18T13:47:47Z

crates/bitwarden-vault/src/cipher/cipher_client/edit.rs

+    #[error(transparent)]
+    Decrypt(#[from] DecryptError),


In the long term I would love to have Cipher be an internal concern of the SDK and we only expose decrypted views. Now in the meanwhile we have to make tradeoffs to move us forward and I don't think it's an issue.

Hinton · 2025-12-19T09:11:25Z

crates/bitwarden-vault/src/cipher/cipher_client/admin/delete.rs

+    pub async fn delete_many(
+        &self,
+        cipher_ids: Vec<CipherId>,
+        organization_id: Option<OrganizationId>,


question: should organization_id be required? Looking at the server code it is.

Hinton · 2025-12-19T09:21:10Z

crates/bitwarden-vault/src/cipher/cipher_client/delete.rs

+async fn delete_cipher(
+    cipher_id: CipherId,
+    api_client: &bitwarden_api_api::apis::ApiClient,
+    repository: &(impl Repository<Cipher> + ?Sized),


For folders we use generics instead of anonymous type parameters:

pub(super) async fn create_folder<R: Repository<Folder> + ?Sized>( key_store: &KeyStore<KeyIds>, api_client: &bitwarden_api_api::apis::ApiClient, repository: &R, request: FolderAddEditRequest, ) -> Result<FolderView, CreateFolderError> {

Do we want to be consistent? I don't believe there is a performance hit for either pattern though.

Hinton · 2025-12-19T09:23:43Z

crates/bitwarden-vault/src/cipher/cipher_client/edit.rs

+    #[error(transparent)]
+    Decrypt(#[from] DecryptError),


Upcasting it to a CryptoError is probably better. I think that should be as simple as doing CryptoError::Decrypt(err)

Hinton · 2025-12-19T09:25:45Z

crates/bitwarden-vault/src/cipher/cipher_client/get.rs

+    VaultParse(#[from] VaultParseError),
+    #[error(transparent)]
    RepositoryError(#[from] RepositoryError),
+    #[error(transparent)]
+    Api(#[from] ApiError),


issue: Remove? Now that you have the admin error these can be removed right?

Hinton · 2025-12-19T09:27:39Z

crates/bitwarden-vault/src/cipher/cipher.rs

        Ok(())
    }
+
+    pub(crate) fn soft_delete(&mut self) {


suggestion: Document?

Suggested change

pub(crate) fn soft_delete(&mut self) {

/// Marks the cipher as soft deleted by setting `deletion_date` to now.

pub(crate) fn soft_delete(&mut self) {

Hinton · 2025-12-19T09:28:41Z

crates/bitwarden-vault/src/cipher/cipher.rs

+/// Used to convert from API response models to full Cipher structs,
+/// without losing local data that may not be present in the API response.
+pub(crate) trait PartialCipher {
+    fn merge_with_cipher(self, cipher: Option<Cipher>) -> Result<Cipher, VaultParseError>;


question: Why accept an optional cipher? The function might be nicer if cipher is required and you just do and_then on callers.

nikwithak added 6 commits November 13, 2025 14:21

Add create and edit admin operations

f426fba

Add delete operations

3756f38

Add soft delete (PUT delete) operations

99d1685

Add update_collection call for ciphers

0cd1aee

Add get_ciphers_for_org to CiphersClient

9f82e22

Add admin endpoints for update and restore

f89f43d

nikwithak added 5 commits November 18, 2025 16:22

Add CipherError::Api(ApiError) variant

fe95edd

Clean up error handling

bd13498

index on vault/pm-25821/cipher-admin-ops: bd13498 Clean up error hand…

c173c98

…ling

Consolidate delete & restore operations

079cb89

Cleanup and logic consolidation

f9eaafa

nikwithak requested a review from gbubemismith November 22, 2025 00:19

nikwithak added 12 commits November 21, 2025 16:20

Update docs for CiphersClient::list_org_ciphers

4e48743

Add separate delete_as_admin functions

f3a06f6

Add tests for new create methods

eb3291a

Add tests for edit admin cipher endpoints

74714e6

Add tests for delete cipher endpoints

3330a78

Update repository when a cipher is soft-deleted

8307f07

Update tests for delete

6485b2e

Move restore operations to separate file

28880f9

Fix test_restore_many tests

0356b2d

Fix soft_delete_as_admin test

2246af5

Housekeeping - remove comments & warnings

0a558c1

Fix edit_as_admin tests

7dc66a5

nikwithak requested a review from Hinton November 26, 2025 22:01

nikwithak force-pushed the vault/pm-25821/cipher-admin-ops branch from aa8a96d to 7dc66a5 Compare November 26, 2025 22:15

Merge branch 'main' of https://github.com/bitwarden/sdk-internal into…

28aac12

… vault/pm-25821/cipher-admin-ops

nikwithak marked this pull request as ready for review November 26, 2025 22:15

nikwithak requested review from a team as code owners November 26, 2025 22:15

Fix clippy warnings

a0ba6e3

Hinton reviewed Nov 27, 2025

View reviewed changes

nikwithak added 14 commits December 3, 2025 12:38

Add soft-delete funciton to Cipher

f90129b

Move cipher admin functions to separate client

8c633b9

Move delete logic to isolated functions, outside of CiphersClient

3e38626

Move restore functions to isolated functions, remove wiremock use

7467c01

Move admin delete ops to new CipherAdminClient

6d59590

Move restore operations to CipherAdminClient

ef3fef3

Move create admin operations to CreateAdminClient

715f75c

Improve docs on delete.rs

feb335f

Move admin edit operations to CipherAdminController

1f6c8ab

Fix cipher admin create tests

a1c61ec

Fix edit cipher admin tests

f029f56

Remove helper function for get_api_configurations in CiphersClient

8cbf8c8

Move list_org_ciphers operation to admin client

8aa3692

Housekeeping: Remove commented code, change ::into -> ::from

16e28b0

nikwithak requested a review from Hinton December 5, 2025 22:32

nikwithak added 3 commits December 8, 2025 10:44

Merge branch 'main' of https://github.com/bitwarden/sdk-internal into…

ca6ae3f

… vault/pm-25821/cipher-admin-ops

Remove test code added to real function by mistake

1cf82de

Add admin() function to get CipherAdminClient

a3c29f0

nikwithak requested a review from shane-melton December 16, 2025 18:46

nikwithak added 2 commits December 16, 2025 16:54

Fix date string format for API requests

2077e04

Change TryFrom implementations to PartialCipher on partial server res…

58e01df

…ponses

Hinton reviewed Dec 19, 2025

View reviewed changes

	pub(crate) fn soft_delete(&mut self) {
	/// Marks the cipher as soft deleted by setting `deletion_date` to now.
	pub(crate) fn soft_delete(&mut self) {

[PM-25821] Migrate Cipher Admin operation API calls to SDK #560

Are you sure you want to change the base?

[PM-25821] Migrate Cipher Admin operation API calls to SDK #560

Conversation

nikwithak commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

github-actions bot commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 SDK Breaking Change Detection Results

Uh oh!

codecov bot commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude bot commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete

Todo List

Overall Assessment: REQUEST CHANGES

Critical Issues Found

❌ Finding 1: Dead Code - Unused KeyStore instantiation in restore_many

⚠️ Finding 2: organization_id hardcoded to None in restore_many

Important Design Issues

♻️ Finding 3: Admin operations don't update local repository

🎨 Finding 4: Missing test coverage for error paths

❓ Finding 5: Inconsistent API between admin and non-admin edit operations

Positive Observations

Security Considerations

Recommendations Summary

Uh oh!

Hinton commented Nov 27, 2025

Uh oh!

Hinton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton left a comment

Choose a reason for hiding this comment

nikwithak commented Nov 15, 2025 •

edited

Loading

github-actions bot commented Nov 15, 2025 •

edited

Loading

github-actions bot commented Nov 15, 2025 •

edited

Loading

codecov bot commented Nov 15, 2025 •

edited

Loading

claude bot commented Nov 26, 2025 •

edited

Loading

Hinton Dec 19, 2025 •

edited

Loading