Skip to content

[PM-42197] feat: Thread encryptedByKeyId through cipher requests - #2987

Merged
fedemkr merged 4 commits into
mainfrom
PM-42197/encryptedByKeyId-on-cipher-requests
Aug 21, 2026
Merged

[PM-42197] feat: Thread encryptedByKeyId through cipher requests#2987
fedemkr merged 4 commits into
mainfrom
PM-42197/encryptedByKeyId-on-cipher-requests

Conversation

@fedemkr

@fedemkr fedemkr commented Aug 20, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

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

📔 Objective

The SDK's EncryptionContext now exposes encryptedByKeyId: String? — the hex-encoded ID of
the specific key used to encrypt a vault item (nil for legacy V1 AES-CBC-HMAC ciphers).

This PR threads that field through the entire cipher-request call chain wherever encryptedFor
is already passed:

  • CipherRequestModel — new encryptedByKeyId: String? property serialized in JSON
  • CipherCreateRequestModel, BulkShareCiphersRequestModel — forwarded to CipherRequestModel
  • All API request types (AddCipherRequest, AddCipherWithCollectionsRequest,
    UpdateCipherRequest, ShareCipherRequest, BulkShareCiphersRequest)
  • CipherAPIService protocol + APIService implementation
  • CipherService protocol + DefaultCipherService implementation
  • CipherEncryptionMediator — reads from EncryptionContext.encryptedByKeyId
  • VaultRepository — passes through for addCipher, updateCipher, shareCipher,
    bulkShareCiphers
  • Fido2CredentialStoreService — passes through for both add and update paths

Also adds userKeyId: String? to UserDecryptionResponseModel (groundwork for a follow-up
that will consume the server's active key ID in the decryption response).

@fedemkr fedemkr added the ai-review Request a Claude code review label Aug 20, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:feature labels Aug 20, 2026
@fedemkr fedemkr changed the title [PM-42197] feat: Thread encryptedByKeyId through cipher requests and add userKeyId to decryption response [PM-42197] feat: Thread encryptedByKeyId through cipher requests Aug 20, 2026
@fedemkr
fedemkr marked this pull request as ready for review August 20, 2026 22:20
@fedemkr
fedemkr requested review from a team and matt-livefront as code owners August 20, 2026 22:20
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR threads the SDK's EncryptionContext.encryptedByKeyId through the cipher request chain (request models, API requests, CipherAPIService, CipherService, CipherEncryptionMediator, VaultRepository, Fido2CredentialStoreService, and the CXF import stack) and adds an optional userKeyId to UserDecryptionResponseModel. The latest commit resolves the previously flagged bulk-share issue: bulkShareCiphersWithServer / bulkShareCiphers / BulkShareCiphersRequest now take [EncryptionContext] end to end, so BulkShareCiphersRequestModel builds each CipherRequestModel from its own context instead of stamping the first context's key ID onto every cipher. Service and API protocol methods take the new parameter without a default, so the compiler enforced updates at every call site; all four CipherService callers pass the encrypting context's key ID rather than nil. The new fields are optional and omitted from JSON when nil, so V1 accounts see no wire change.

Code Review Details

No findings.

Verified during this pass: the guard let encryptedFor = encryptionContexts.first?.encryptedForguard !encryptionContexts.isEmpty change in VaultRepository.bulkShareCiphers is behavior-preserving (SDK's encryptedFor is non-optional, so the old binding only guarded emptiness); no remaining CipherRequestModel construction path bypasses the new field; and userKeyId decodes to nil for existing fixtures via synthesized Codable.

Minor, not posted inline: the request-level initializers (AddCipherRequest, UpdateCipherRequest, ShareCipherRequest, AddCipherWithCollectionsRequest, CipherCreateRequestModel, CipherRequestModel) default encryptedByKeyId to nil, which weakens the compiler enforcement the service/API protocols deliberately keep, though every current call site passes it explicitly.

Comment thread BitwardenShared/Core/Vault/Repositories/VaultRepository.swift Outdated
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.80%. Comparing base (3efa9c7) to head (6d862b7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2987      +/-   ##
==========================================
+ Coverage   79.51%   81.80%   +2.28%     
==========================================
  Files        1169     1046     -123     
  Lines       75029    67798    -7231     
==========================================
- Hits        59660    55461    -4199     
+ Misses      15369    12337    -3032     

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

matt-livefront
matt-livefront previously approved these changes Aug 21, 2026
@fedemkr
fedemkr requested a review from matt-livefront August 21, 2026 16:21
@fedemkr
fedemkr merged commit dede418 into main Aug 21, 2026
24 checks passed
@fedemkr
fedemkr deleted the PM-42197/encryptedByKeyId-on-cipher-requests branch August 21, 2026 16:26
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 app:password-manager Bitwarden Password Manager app context t:feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants