[PM-32802] fix: 400 error when archiving/unarchiving org-owned ciphers

[SaintPatrck]

🎟️ Tracking

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

📔 Objective

Fix a 400 error when users with restricted permissions ("can view, hidden passwords" or "can edit, hidden passwords") attempt to archive or unarchive org-owned ciphers.

Root cause: archiveCipher() and unarchiveCipher() in CipherManagerImpl unnecessarily called encryptCipherAndCheckForMigration() before the archive/unarchive API call. When the SDK generated a new cipher key (migration), this triggered a full PUT /api/ciphers/{id} update that the server rejected with HTTP 400 for users lacking edit permissions — aborting the archive before it was ever attempted.

Fix: Remove the encryption/migration check from archive and unarchive flows entirely. These operations now call the archive/unarchive API directly and upsert the server-returned cipher to disk, following the existing restoreCipher pattern. The server handles all state changes; no client-side encryption is needed.

Changes:

• CiphersApi — Archive/unarchive return SyncResponseJson.Cipher instead of Unit
• CiphersService/Impl — Return typed ArchiveCipherResponseJson / UnarchiveCipherResponseJson with 400 error recovery
• CipherManagerImpl — Remove encryptCipherAndCheckForMigration from archive/unarchive; upsert server response directly
• Response models — Add ArchiveCipherResponseJson and UnarchiveCipherResponseJson sealed classes
• Tests — Updated CipherManagerTest and CiphersServiceTest to match new behavior

[claude]

Claude finished @SaintPatrck's task in 5m 15s —— View job

---

Code Review: PR #6592

• Gather context and read changed files
• Review code changes for correctness, security, and patterns
• Post review findings

---

Summary

This PR correctly fixes the 400 error by removing the unnecessary encryptCipherAndCheckForMigration() call from archive/unarchive flows. The root cause analysis is sound — archive/unarchive are metadata-only operations that don't modify cipher content, so the full cipher PUT (and potential key migration) was never needed.

Findings

No new issues found. The previous review's collectionIds preservation finding has been addressed in commit 4a97f6434.

Detailed Analysis

Correctness — The fix is sound. Archive and unarchive only toggle a server-side state flag and don't require client-side encryption. Removing encryptCipherAndCheckForMigration() eliminates the unnecessary full PUT /api/ciphers/{id} that was causing the 400 for users without edit permissions.

Pattern Consistency — The implementation follows established codebase patterns well:

• The service layer (CiphersServiceImpl.kt:43-75) uses the same .toResult().map { Success }.recoverCatching { ... } pattern as updateCipher, createCipher, and importCiphers
• The response models (ArchiveCipherResponseJson, UnarchiveCipherResponseJson) mirror UpdateCipherResponseJson's structure with Success/Invalid sealed variants implementing InvalidJsonResponse
• The manager layer preserves collectionIds from the local cipher view before disk save, matching restoreCipher (line 357), updateCipher (line 391), and createAttachmentForResult (line 565)

Error Handling — 400 errors are caught at the service layer via recoverCatching and surfaced as Invalid response types. At the manager layer, Invalid is converted to IllegalStateException with the server's validation error message, which then maps to the appropriate Error result type. This is consistent with how updateCipher and createCipher handle validation errors.

Comment at lines 252-254 — The explanatory comment on softDeleteCipher clarifying why it retains the encryptCipherAndCheckForMigration() call is a good addition for future maintainability.

Test Coverage — Tests are comprehensive:

• 4 service-layer tests covering success and invalid response paths for both endpoints
• 8 manager-layer tests covering no active user, service failure, invalid response, and success for both archive and unarchive
• Migration-specific tests were correctly removed since the migration check is no longer part of the flow

Security — No concerns. No encrypted content is modified or transmitted during archive/unarchive operations.

Minor Observations (Non-Blocking)

The suppress annotations in CiphersServiceTest.kt flagged by david-livefront appear to be already noted.

Overall, this is a clean, well-structured fix that correctly addresses the root cause.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 1d4e3643-f03e-49a2-99e1-d641c0ba3c82

---

Fixed Issues (2)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	Use_of_Hardcoded_Password	/network/src/test/kotlin/com/bitwarden/network/service/CiphersServiceTest.kt: 70
	Use_of_Hardcoded_Password	/network/src/test/kotlin/com/bitwarden/network/service/CiphersServiceTest.kt: 78

[codecov]

Codecov Report

❌ Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.95%. Comparing base (e509d60) to head (775d881).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...om/bitwarden/network/service/CiphersServiceImpl.kt	90.90%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6592      +/-   ##
==========================================
+ Coverage   85.78%   85.95%   +0.17%     
==========================================
  Files         937      800     -137     
  Lines       59541    57157    -2384     
  Branches     8324     8270      -54     
==========================================
- Hits        51075    49130    -1945     
+ Misses       5567     5154     -413     
+ Partials     2899     2873      -26     

Flag	Coverage Δ
app-data	17.57% <55.10%> (-0.26%)	⬇️
app-ui-auth-tools	20.94% <0.00%> (-0.33%)	⬇️
app-ui-platform	15.12% <0.00%> (-0.65%)	⬇️
app-ui-vault	25.87% <0.00%> (-0.67%)	⬇️
authenticator	6.45% <0.00%> (-0.01%)	⬇️
lib-core-network-bridge	4.30% <40.81%> (+0.02%)	⬆️
lib-data-ui	0.90% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[david-livefront]

So this was completely unnecessary?

[SaintPatrck]

At some point it was because the archive endpoints were not returning the entire updated cipher, it was only returning the "mini" version. This was never updated when that changed.