Skip to content

chore: Use private key accessor#6808

Merged
david-livefront merged 1 commit intomainfrom
update-private-key
Apr 20, 2026
Merged

chore: Use private key accessor#6808
david-livefront merged 1 commit intomainfrom
update-private-key

Conversation

@david-livefront
Copy link
Copy Markdown
Collaborator

@david-livefront david-livefront commented Apr 17, 2026

🎟️ Tracking

NA

📔 Objective

This PR updates the way we access the private key from the login response to ensure we are always attempting to retrieve it from the accountKeys before falling back to the legacy privateKey property. Additionally, a new AccountKeysJson.toAccountCryptographicState extension was added to simplify the transformation to a WrappedAccountCryptographicState.

@david-livefront david-livefront added the ai-review-vnext Request a Claude code review using the vNext workflow label Apr 17, 2026
@david-livefront david-livefront requested a review from a team as a code owner April 17, 2026 19:29
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:tech-debt Change Type - Tech debt labels Apr 17, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 17, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the refactor that introduces AccountKeysJson?.toAccountCryptographicState(privateKey) and a local GetTokenResponseJson.Success.privateKeyOrNull() accessor, replacing several copies of the same four-field boilerplate across AuthRepositoryImpl, AuthenticatorBridgeRepositoryImpl, VaultLockManagerImpl, and VaultRepositoryImpl. Traced every call site to confirm behavioral equivalence: because AccountKeysJson.publicKeyEncryptionKeyPair.wrappedPrivateKey is a non-null String in the schema, accountKeys != null still implies a non-null wrapped private key, and the nullable-receiver extension preserves the V2/V1 selection in createWrappedAccountCryptographicState. The isNewKeyConnectorUser check was tightened from loginResponse.privateKey == null to loginResponse.privateKeyOrNull() == null, which is a minor correctness improvement for returning key-connector users that have only accountKeys populated. Test expectations in AuthRepositoryTest were updated from "privateKey" to "mockWrappedPrivateKey-1" to match the accessor's new preference for accountKeys.publicKeyEncryptionKeyPair.wrappedPrivateKey, keeping coverage aligned with the production change.

Code Review Details

No findings.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

❌ Patch coverage is 89.65517% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.91%. Comparing base (6e16daf) to head (501c503).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...twarden/data/auth/repository/AuthRepositoryImpl.kt 90.00% 0 Missing and 2 partials ⚠️
.../auth/repository/util/AccountKeysJsonExtensions.kt 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6808      +/-   ##
==========================================
+ Coverage   85.55%   85.91%   +0.35%     
==========================================
  Files         959      858     -101     
  Lines       61062    59599    -1463     
  Branches     8651     8560      -91     
==========================================
- Hits        52242    51203    -1039     
+ Misses       5813     5444     -369     
+ Partials     3007     2952      -55     
Flag Coverage Δ
app-data 17.12% <89.65%> (-0.61%) ⬇️
app-ui-auth-tools 20.37% <0.00%> (-0.32%) ⬇️
app-ui-platform 15.66% <0.00%> (-0.19%) ⬇️
app-ui-vault 26.55% <0.00%> (-0.03%) ⬇️
authenticator 6.69% <0.00%> (-0.02%) ⬇️
lib-core-network-bridge 4.28% <0.00%> (+0.01%) ⬆️
lib-data-ui 1.03% <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 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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 17, 2026

Logo
Checkmarx One – Scan Summary & Details81a221e7-b2ee-48a7-b895-e8dd46cee096

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

fakeAuthDiskSource.assertPrivateKey(
userId = USER_ID_1,
privateKey = "privateKey",
privateKey = "mockWrappedPrivateKey-1",
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These test updates are because we now favor the private key from the AccountKeysJson instead of the private key directly on the login response.

@david-livefront
Copy link
Copy Markdown
Collaborator Author

Thanks @SaintPatrck

@david-livefront david-livefront added this pull request to the merge queue Apr 20, 2026
Merged via the queue into main with commit d0aa74c Apr 20, 2026
32 checks passed
@david-livefront david-livefront deleted the update-private-key branch April 20, 2026 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow app:password-manager Bitwarden Password Manager app context t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants