Skip to content

fix(vault): skip key id validation for organization - #8219

Open
quexten wants to merge 1 commit into
mainfrom
km/organization-key-id-skip-validation
Open

fix(vault): skip key id validation for organization#8219
quexten wants to merge 1 commit into
mainfrom
km/organization-key-id-skip-validation

Conversation

@quexten

@quexten quexten commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

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

Accidentally, we check the user-key-id for all cipher updates including organization cipher updates. We have follow-up work to post and check the organization key id, but this is not currently the case. We should ignore it, otherwise cipher updates will fail.

@quexten
quexten requested a review from a team as a code owner August 15, 2026 07:22
@quexten
quexten requested a review from nikwithak August 15, 2026 07:22
@github-actions

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the change that skips the EncryptedByKeyId check for organization-owned ciphers across Post, PostCreate, Put, and PutShare in CiphersController, plus the new IsOrganizationCipher helper on CipherRequestModel. Verified all four call sites of ValidateCipherEncryptedByUser are updated consistently and that the deprecated EncryptedFor ownership check still runs for organization ciphers. Put correctly derives organization ownership from the stored cipher rather than the client-supplied model, and PutShare correctly hardcodes the organization path since sharing always re-encrypts under the organization key. Test coverage was extended for each affected endpoint, and no user-owned cipher path loses its key id validation.

Code Review Details

No findings.

Notes considered and dismissed during validation:

  • Post/PostCreate derive organization ownership from the client-supplied OrganizationId, but the resulting cipher is stored as an organization cipher (UserId is null) and organization membership is enforced immediately after, so there is no path where a user-owned cipher escapes key id validation.
  • The [KeyId] model attribute still validates the format of EncryptedByKeyId for organization ciphers; only the equality comparison is skipped.

@quexten quexten added ai-review Request a Claude code review t:bugfix Change Type - Bugfix labels Aug 15, 2026
@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

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

❗ There is a different number of reports uploaded between BASE (bc45404) and HEAD (c4e3847). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (bc45404) HEAD (c4e3847)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8219      +/-   ##
==========================================
- Coverage   68.56%   63.28%   -5.29%     
==========================================
  Files        2381     2381              
  Lines      103902   103906       +4     
  Branches     9405     9406       +1     
==========================================
- Hits        71245    65754    -5491     
- Misses      30310    35903    +5593     
+ Partials     2347     2249      -98     

☔ View full report in Codecov by Harness.
📢 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant