[PM-27012] Transfer Archived Cipher to Org

[nick-livefront]

🎟️ Tracking

PM-27012
Client PR: bitwarden/clients#17592

📔 Objective

With the change in underlying architecture, archived ciphers will be tied to the user rather than to the cipher. This removes checks that originally removed the ability to share a cipher. The bug ticket addressed here is around enforcing data ownership so that is what I captured in the videos.

📸 Screenshots

web	desktop	browser
data-ownership-archived-web.mov	data-ownership-archived-extension.mov	data-ownership-archived-extension.mov

🦮 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

[claude]

Claude finished @nick-livefront's task —— View job

---

Code Review Complete

Summary

This PR removes archived cipher validation checks that previously prevented sharing archived items with organizations. The changes align with a new architecture where archived state is tied to users rather than ciphers. The removals are clean and appropriately covered by corresponding test deletions.

Findings

Finding 1: PR title and objective need clarification to accurately reflect the scope of changes

💭 The PR title "PM-27012 Transfer Archived Cipher to Org" and objective focus on "data ownership" but the code changes specifically remove validation checks that prevented sharing archived ciphers. The PR description states "removes checks that originally removed the ability to share a cipher" which is accurate, but the title/objective could be clearer.

Suggested improvements

Title suggestion:

[PM-27012] Allow Sharing Archived Ciphers to Organizations

Objective clarification:
The objective should explicitly state that this change removes server-side validation that prevented archived ciphers from being shared/transferred to organizations, since the new architecture handles archived state at the user level rather than the cipher level.

---

Finding 2: Consider documenting the architectural change that enables this modification

💭 The removed validation checks were protecting against what was previously considered invalid state (archived ciphers in organizations). With the architecture change making archived state user-specific, these checks are no longer necessary. However, there's no inline documentation explaining why these checks were safe to remove.

Recommendation

Consider adding a comment near the sharing logic (e.g., in CipherService.cs:ValidateCipherCanBeShared method around line 972-1014) that briefly documents:

• Why archived ciphers can now be shared (user-level vs cipher-level archived state)
• Reference to the ticket or architectural decision

This would help future maintainers understand the context of this change.

---

Finding 3: Verify client-side handling of archived ciphers in organizations

💭 While the server-side validation is removed, it's important to ensure the client applications properly handle the user-level archived state when displaying/managing organization ciphers. The PR description references a client PR (bitwarden/clients#17592), which is good practice.

Verification needed

Confirm that:

1. Client PR properly implements user-level archived state filtering
2. Organization admins cannot see which users have archived specific org ciphers (zero-knowledge principle)
3. Sharing an archived cipher correctly maintains/clears the archived state as appropriate

---

Good Practices Observed

• Comprehensive test cleanup matching production code changes
• Consistent removal of validation across all three sharing endpoints (PutShare, PutShareMany, and service layer)
• Video demonstrations showing functionality across all client platforms

Action Items

1. ✅ Review and potentially update PR title/objective to more accurately reflect the scope (removing archived cipher sharing restrictions)
2. 💭 Consider adding inline documentation explaining the architectural context for this change
3. 💭 Verify client-side implementation handles user-level archived state correctly (reference linked client PR)

---

[github-actions]

Checkmarx One – Scan Summary & Details – 778a965f-3d07-4a01-b6a7-eeac03e5d802

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.06%. Comparing base (1cc787f) to head (ea6dcc5).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6626      +/-   ##
==========================================
+ Coverage   52.99%   53.06%   +0.07%     
==========================================
  Files        1909     1899      -10     
  Lines       84828    84581     -247     
  Branches     7634     7600      -34     
==========================================
- Hits        44951    44885      -66     
+ Misses      38125    37951     -174     
+ Partials     1752     1745       -7     

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