Skip to content

[PM-42198] feat: Send encryptedByKeyId on cipher requests - #7287

Merged
aj-rosado merged 2 commits into
mainfrom
PM-42198/send-cipher-encrypted-by-key-id
Aug 21, 2026
Merged

[PM-42198] feat: Send encryptedByKeyId on cipher requests#7287
aj-rosado merged 2 commits into
mainfrom
PM-42198/send-cipher-encrypted-by-key-id

Conversation

@aj-rosado

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

Reference: bitwarden/clients#22444 (wiring only — the enrolment migration in
that PR is out of scope here).

📔 Objective

The SDK reports the id of the key it used to encrypt a vault item. Sending
that to the server allows the write to be validated against the expected key.

The value comes from the SDK's EncryptionContext and should not be
synthesised from app state.

This mirrors the existing encryptedFor plumbing, so every write path that
already carries an EncryptionContext picks it up with no call-site change:
create, create in organization, update, cipher key migration, share, bulk
share and CXF import. The new parameter is required with no default, so future
callers have to decide explicitly rather than silently omit the field.

Notes for reviewers

  • Share and bulk share include the id, matching the SDK's own
    From<EncryptionContext> conversions. The web client's CipherShareRequest
    drops it, which looks unintentional — worth confirming with the author of
    clients#22444.
  • Nothing is persisted. SyncResponseJson.Cipher and
    toEncryptedNetworkCipherResponse are untouched.
  • Verified against US dev: the field is sent and writes are accepted.

📸 Screenshots

N/A — no UI changes.

@aj-rosado aj-rosado 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 app:authenticator Bitwarden Authenticator app context t:feature Change Type - Feature Development labels Aug 20, 2026
@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-reported encryptedByKeyId from EncryptionContext into CipherJsonRequest and CipherWithIdJsonRequest, mirroring the existing encryptedFor plumbing so create, update, key migration, share, bulk share and CXF import pick it up with no call-site changes. I re-traced every consumer: the only production mapping is EncryptionContext.toEncryptedNetworkCipher(), and VaultMigrationManagerImpl carries the value through toCipherWithIdJsonRequest for bulk share, so the id always originates from the SDK rather than app state. The required-with-no-default parameter on Cipher.toEncryptedNetworkCipher forces explicit decisions at future call sites, and toEncryptedNetworkCipherResponse/SyncResponseJson.Cipher are correctly untouched since nothing is persisted. The latest commit replaces the size-only assertions with full-object equality in CredentialExchangeImportManagerTest and adds CipherWithIdJsonRequestTest with a matching createMockCipherWithIdJsonRequest fixture; the fixture defaults line up with createMockCipherJsonRequest and the fixed clock used by createMockSdkCipher.

Code Review Details

No findings.

@aj-rosado
aj-rosado marked this pull request as ready for review August 20, 2026 19:20
@aj-rosado
aj-rosado requested review from a team and david-livefront as code owners August 20, 2026 19:20
@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 86.24%. Comparing base (f5a96eb) to head (91c8c5f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7287      +/-   ##
==========================================
+ Coverage   86.23%   86.24%   +0.01%     
==========================================
  Files         894      894              
  Lines       65445    65524      +79     
  Branches     9834     9857      +23     
==========================================
+ Hits        56438    56514      +76     
+ Misses       5512     5508       -4     
- Partials     3495     3502       +7     
Flag Coverage Δ
app-data 17.83% <100.00%> (-0.02%) ⬇️
app-ui-auth-tools 18.96% <0.00%> (+0.09%) ⬆️
app-ui-platform 16.37% <0.00%> (-0.02%) ⬇️
app-ui-vault 27.29% <0.00%> (-0.04%) ⬇️
authenticator 6.07% <0.00%> (-0.03%) ⬇️
lib-core-network-bridge 4.10% <0.00%> (+0.01%) ⬆️
lib-data-ui 1.20% <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 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.


assertEquals("mockId-1", result.id)
assertEquals(request.encryptedFor, result.encryptedFor)
assertEquals(request.encryptedByKeyId, result.encryptedByKeyId)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just assert the full result


val cipher = capturedRequest.captured.ciphers.first()
assertEquals(DEFAULT_CIPHER.encryptedFor, cipher.encryptedFor)
assertEquals(DEFAULT_CIPHER.encryptedByKeyId, cipher.encryptedByKeyId)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just assert the entire cipher is correct

/**
* Converts a Bitwarden SDK [Cipher] object to a corresponding
* [SyncResponseJson.Cipher] object.
* [CipherJsonRequest] object.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@aj-rosado
aj-rosado added this pull request to the merge queue Aug 21, 2026
Merged via the queue into main with commit d817f6b Aug 21, 2026
26 checks passed
@aj-rosado
aj-rosado deleted the PM-42198/send-cipher-encrypted-by-key-id branch August 21, 2026 12:43
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:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants