[PM-25675] - fix NormalCipherPermissions.CanDelete

[jaasen-livefront]

🎟️ Tracking

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

📔 Objective

When fetching organization-owned ciphers, the SQL view (UserCipherDetails) injects the current user’s Id into cipherDetails.UserId even when the cipher’s true UserId in the DB is null.

This caused NormalCipherPermissions to treat org ciphers as personal items, because the old logic only checked for matching user.Id and cipher.UserId. Adding a check for cipher.OrganizationId fixed the issue. Updating a couple tests to test the correct method and be a bit sturdier should help avoid regressions.

📸 Screenshots

Screen.Recording.2025-12-01.at.4.32.15.PM.mov

⏰ 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

[claude]

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

---

Code Review Complete

Tasks

• Read changed files and examine diff
• Review the permissions fix logic
• Review test changes
• Evaluate PR title and description
• Check for security implications
• Post comprehensive review findings
• Post inline comments

---

Summary

This PR fixes a critical bug in NormalCipherPermissions.CanDelete where organization-owned ciphers were incorrectly treated as personal items. The root cause is that the SQL UserCipherDetails view returns the current user's ID in the UserId field even for organization ciphers (line 37-39 in UserCipherDetails.sql shows the join condition C.[UserId] IS NULL but the view still projects C.UserId). The fix correctly adds a null check for OrganizationId before allowing personal cipher deletion.

Findings

Finding 1: Test change may not accurately reflect production behavior (test/Core.Test/Vault/Authorization/Permissions/NormalCipherPermissionTests.cs:95)

💭 The CanDelete_WhenCipherIsOwnedByOrganization test was updated to set UserId = user.Id for organization ciphers. While this matches the SQL view behavior (where UserCipherDetails injects the user's ID), it creates test data that differs from the actual database state (where org ciphers have UserId = null).

Context and implications

According to the PR description and SQL view analysis:

• In the database: Organization ciphers have UserId = null
• After SQL view projection: UserCipherDetails injects the current user's ID into the UserId field

The updated test now uses UserId = user.Id which matches what the application layer receives, but masks the underlying database state that caused the bug. This could make the test less effective at catching similar issues.

Question: Was this change intentional to match the SQL view behavior? Or should the test maintain UserId = null to better represent the actual data model and ensure the fix prevents the regression?

---

Finding 2: Incomplete fix - CanRestore may have the same issue

⚠️ The CanRestore method delegates to CanDelete (line 36 in NormalCipherPermissions.cs), so it inherits the fix. However, the test case CanRestore_WhenCipherIsOwnedByOrganization (line 17-31) still uses UserId = null, unlike the updated CanDelete test. This inconsistency suggests the test scenarios may not be equivalent.

Recommendation: Review whether CanRestore_WhenCipherIsOwnedByOrganization should also be updated with UserId = user.Id to match the SQL view behavior, or if there's a reason for the difference.

---

Finding 3: PR description quality

💭 The PR description is well-structured and includes:

• ✅ Jira reference in tracking section
• ✅ Clear objective explaining what changed and why
• ✅ Screen recording demonstrating the fix
• ✅ Completed checklist

The description effectively explains the root cause (SQL view behavior) and the solution (adding OrganizationId check).

---

Good Practices Observed

• Minimal, focused change addressing the specific bug
• Test coverage for both methods (CanDelete and CanRestore)
• Test robustness improvement (OrganizationAbility now includes Id property)

---

Security Analysis

✅ No security concerns identified. The fix correctly enforces ownership boundaries between personal and organization ciphers, which is critical for maintaining zero-knowledge architecture. By checking OrganizationId == null before allowing personal cipher deletion, the code now properly distinguishes between personal and organizational resources.

---

Action Items

1. Clarify test data strategy: Decide whether tests should use UserId = null (database reality) or UserId = user.Id (SQL view projection) for organization ciphers, and apply consistently across all test cases
2. Consider integration test: Add an integration test that exercises the full SQL view → application code path to catch similar view-related issues

[github-actions]

Checkmarx One – Scan Summary & Details – 469780db-143c-489a-8b23-f805b1abaeb2

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

[claude]

💭 This test now sets UserId = user.Id for organization ciphers, which matches what the SQL view (UserCipherDetails) returns to the application layer. However, this differs from the actual database state where org ciphers have UserId = null.

Question: Is this intentional to match the SQL view behavior? Or should the test maintain UserId = null to better represent the underlying data model?

For comparison, the CanRestore_WhenCipherIsOwnedByOrganization test (line 23) still uses UserId = null. Should that be updated for consistency, or is there a reason for the difference?

[claude]

👍 Excellent fix! Adding the cipherDetails.OrganizationId == null check correctly distinguishes between personal and organization ciphers, even when the SQL view injects the user's ID into UserId for org ciphers.

This properly enforces the ownership boundary: personal ciphers can be deleted by their owner (UserId matches and no OrganizationId), while org ciphers must go through the organization permission checks below.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 53.41%. Comparing base (3559759) to head (6351178).
⚠️ Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
...thorization/Permissions/NormalCipherPermissions.cs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6666      +/-   ##
==========================================
+ Coverage   53.10%   53.41%   +0.31%     
==========================================
  Files        1903     1910       +7     
  Lines       84877    85156     +279     
  Branches     7633     7651      +18     
==========================================
+ Hits        45071    45484     +413     
+ Misses      38055    37913     -142     
- Partials     1751     1759       +8     

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