Skip to content

[PM-21926] [PM-30350] [PM-32389] Read salt from database#7230

Open
ike-kottlowski wants to merge 16 commits intomainfrom
auth/pm-21926/read-salt-from-database
Open

[PM-21926] [PM-30350] [PM-32389] Read salt from database#7230
ike-kottlowski wants to merge 16 commits intomainfrom
auth/pm-21926/read-salt-from-database

Conversation

@ike-kottlowski
Copy link
Contributor

@ike-kottlowski ike-kottlowski commented Mar 16, 2026

🎟️ Tracking

PM-21926
PM-30350
PM-32389

📔 Objective

Add MasterPasswordSalt to the response for ReadKdfByEmail and OranizationAccountRevovery in the User Repository.

I've removed a Test pertaining to Emergency Access that tries to check the salt against the User.Email which may not be the case in the future. So, it has been removed.

Notes

The AC Repository updates for account recovery are handled in this PR which is in draft until this one is merged.

AC PR: #7231

@ike-kottlowski ike-kottlowski added the ai-review Request a Claude code review label Mar 16, 2026
@claude
Copy link
Contributor

claude bot commented Mar 16, 2026

Claude finished @ike-kottlowski's task in 2m 34s —— View job


Code Review: PM-21926 PM-30350 PM-32389 Read salt from database

  • Gather context and read changed files
  • Perform code review
  • Post review findings

Overall Assessment: APPROVE

This PR adds MasterPasswordSalt to the UserKdfInformation model and updates both Dapper stored procedures and EF queries to return it. The User.GetMasterPasswordSalt() method now reads from the database column with a safe fallback to the legacy email-based salt. Integration tests cover PBKDF2, Argon2id, null salt, and non-existent email cases. Migration scripts use CREATE OR ALTER for safe idempotent deployment.

Code Review Details

No new findings. The changes are well-structured, backward-compatible, and thoroughly tested. CI checks (Checkmarx, SonarCloud, Codecov) are all passing. Previous review feedback from mkincaid-bw (SQL formatting and file renaming) has been addressed.

@ike-kottlowski ike-kottlowski marked this pull request as ready for review March 16, 2026 22:08
@ike-kottlowski ike-kottlowski requested a review from a team as a code owner March 16, 2026 22:08
@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.81%. Comparing base (753c8c8) to head (9e274ef).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7230      +/-   ##
==========================================
+ Coverage   57.68%   61.81%   +4.12%     
==========================================
  Files        2035     2035              
  Lines       89645    89647       +2     
  Branches     7993     7994       +1     
==========================================
+ Hits        51709    55412    +3703     
+ Misses      36072    32289    -3783     
- Partials     1864     1946      +82     

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

github-actions bot commented Mar 16, 2026

Logo
Checkmarx One – Scan Summary & Details1d125b0f-fe94-4000-8eb6-9d9862f63f9a

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

Copy link
Contributor

@mkincaid-bw mkincaid-bw left a comment

Choose a reason for hiding this comment

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

Minor changes requested.

@ike-kottlowski ike-kottlowski changed the title [PM-21926] Read salt from database [PM-21926] [PM-30350] Read salt from database Mar 18, 2026
@ike-kottlowski ike-kottlowski changed the title [PM-21926] [PM-30350] Read salt from database [PM-21926] [PM-30350] [PM-32389] Read salt from database Mar 18, 2026
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes are good! Another step closer. Thank you.

@sonarqubecloud
Copy link

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants