Skip to content

[PM-31631] update password pre-login salt response#7469

Merged
ike-kottlowski merged 19 commits intomainfrom
auth/pm-31631/update-password-prelogin-salt-response
May 5, 2026
Merged

[PM-31631] update password pre-login salt response#7469
ike-kottlowski merged 19 commits intomainfrom
auth/pm-31631/update-password-prelogin-salt-response

Conversation

@ike-kottlowski
Copy link
Copy Markdown
Contributor

@ike-kottlowski ike-kottlowski commented Apr 14, 2026

🎟️ Tracking

PM-31631

📔 Objective

With the addition of the salt property used to calculate hash values for passwords we need to add salt to the response object for password/prelogin. Otherwise the client won't know which algorithm and salt to use to when validating for password authentication.

📸 Screenshots

Insomnia API Tooling showing request getting both email and null responses from prelogin/password endpoint

insomnia-12.5.0_Xs8Iy93mz0.mp4

@ike-kottlowski ike-kottlowski added the ai-review-vnext Request a Claude code review using the vNext workflow label Apr 14, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 14, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the latest commits (dfbc28aee, 460cdb59f) on top of the original change. The PR returns the user's stored MasterPasswordSalt from /accounts/prelogin/password and routes non-existent users through EnumerationProtectionHelpers to deterministically pick a salt (email or null). Both prior review threads are resolved: the self-hosted startup warning now uses CoreHelpers.SettingHasValue to align with the controller's enable check, and the email-normalization rationale is now documented inline. Unit and integration test coverage exercises the new branches (stored salt, null stored salt, default-KDF determinism, casing independence).

Code Review Details
  • ♻️ : Stale XML comment on Salt property says "Not used yet, just the email from the request at this time" — after this PR Salt is sourced from UserKdfInformation.MasterPasswordSalt or the deterministic default-KDF path, so the comment is now misleading.
    • src/Identity/Models/Response/Accounts/PasswordPreloginResponseModel.cs:36

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.62%. Comparing base (27ae3d5) to head (460cdb5).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/Identity/Startup.cs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7469      +/-   ##
==========================================
- Coverage   63.87%   59.62%   -4.25%     
==========================================
  Files        2088     2096       +8     
  Lines       92350    92574     +224     
  Branches     8205     8231      +26     
==========================================
- Hits        58987    55201    -3786     
- Misses      31337    35427    +4090     
+ Partials     2026     1946      -80     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 14, 2026

Logo
Checkmarx One – Scan Summary & Detailsddd1c08e-7f72-4bbc-bd35-7f5a036f6ee3

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

@ike-kottlowski ike-kottlowski marked this pull request as ready for review April 18, 2026 02:22
@ike-kottlowski ike-kottlowski requested a review from a team as a code owner April 18, 2026 02:22
Comment thread src/Identity/Startup.cs Outdated
Co-authored-by: Copilot <copilot@github.com>
Comment thread src/Identity/Controllers/AccountsController.cs
Added a comment to clarify email normalization process.
Copy link
Copy Markdown
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

Excellent work.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 1, 2026

@ike-kottlowski ike-kottlowski merged commit da5fc0e into main May 5, 2026
46 of 47 checks passed
@ike-kottlowski ike-kottlowski deleted the auth/pm-31631/update-password-prelogin-salt-response branch May 5, 2026 20:30
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants