Skip to content

feat(kdf-change): port to sdk - #1297

Open
quexten wants to merge 6 commits into
mainfrom
pm-change-kdf-sdk-port
Open

feat(kdf-change): port to sdk#1297
quexten wants to merge 6 commits into
mainfrom
pm-change-kdf-sdk-port

Conversation

@quexten

@quexten quexten commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

bitwarden/clients#22032
https://bitwarden.atlassian.net/browse/PM-40984

Moves KDF-Change fully to the SDK, including API calls and state management.

Added TS tests to test the public API contract of the SDK.

@quexten quexten added the BEEEP label Jul 24, 2026
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: pm-change-kdf-sdk-port (3c4eada)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
android ✅ No breaking changes detected Compilation passed with new SDK version - View Details
typescript ❌ Breaking changes detected Compilation failed with new SDK version. A corresponding pull request addressing the breaking changes must be ready for merge in bitwarden/clients. - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.83333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.84%. Comparing base (a91e406) to head (80248d2).

Files with missing lines Patch % Lines
crates/bitwarden-core/src/client/test_accounts.rs 82.35% 3 Missing ⚠️
crates/bitwarden-crypto/src/keys/kdf.rs 0.00% 3 Missing ⚠️
...bitwarden-user-crypto-management/src/change_kdf.rs 99.27% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1297      +/-   ##
==========================================
+ Coverage   85.81%   85.84%   +0.03%     
==========================================
  Files         490      491       +1     
  Lines       70590    70755     +165     
==========================================
+ Hits        60574    60738     +164     
- Misses      10016    10017       +1     

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

@quexten
quexten marked this pull request as ready for review July 27, 2026 04:42
@quexten
quexten requested review from a team as code owners July 27, 2026 04:42
@quexten
quexten requested review from djsmith85 and mzieniukbw July 27, 2026 04:42
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the port of the KDF-change flow into the SDK: the new change_kdf operation on UserCryptoManagementClient, the kdf_config addition to the key-management state bridge, the TryFrom<JsValue> impl required for Kdf, and the test-only API-client injection path on ClientBuilder. The core crypto flow is correct — the salt is reused consistently across old-auth, new-auth, and unlock derivation; the key store context is scoped away from the await on the server request; and local state is persisted only after the server call succeeds. Test coverage is strong (success, API-failure-does-not-persist, and missing-unlock-data cases).

Code Review Details

No blocking findings.

Notes considered and intentionally not raised as findings:

  • The TryFrom<wasm_bindgen::JsValue> for Kdf impl is required by the state_bridge! macro's generated getter (bitwarden-state-bridge-macro/src/lib.rs:292) now that kdf_config: Kdf is bridged. Necessary and correct.
  • test-fixtures gating on with_api_configurations / from_api_client keeps the mock-injection path out of production builds; build() always uses ApiConfigurations::new when the feature is off.
  • serde-wasm-bindgen is an existing workspace dependency being enabled for bitwarden-crypto, not a net-new dependency — no AppSec review needed.
  • Post-success persistence spans three calls (set_masterpassword_unlock_data, set_kdf_config, set_user_master_password_unlock) and is non-atomic, but this matches the established transitional state-bridge pattern and is recoverable on next sync.
  • UpdateKdfResponse / make_update_kdf are deprecated (not removed), preserving backward compatibility for key-connector until it migrates.

@quexten
quexten requested a review from a team as a code owner July 29, 2026 11:50

@theMickster theMickster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving CODEOWNERS change on-behalf of tech-leads.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants