Auth/pm 35392/master password service foundation#7530
Conversation
…te pw flags, tag data update ticket.
Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new The crypto contract is preserved: the server stores a hash-of-hash of the client-supplied authentication hash, plaintext master passwords are never received, KDF/salt invariants are enforced on the update paths, and Test coverage is thorough — happy paths, validation failures, KDF rotation directions, salt/KDF independence per field, hydration guards, security-stamp flag behavior across all three tiers, and the deferred-validation contract for Code Review DetailsNo new findings. All previously identified items are either resolved in earlier commits on this PR or tracked in linked Jira tasks (PM-34905, PM-35395, PM-35501). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7530 +/- ##
==========================================
+ Coverage 59.25% 63.81% +4.56%
==========================================
Files 2082 2087 +5
Lines 91994 92262 +268
Branches 8177 8200 +23
==========================================
+ Hits 54507 58880 +4373
+ Misses 35549 31356 -4193
- Partials 1938 2026 +88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
I can't begin to say how giddy I am with the changes you've made. I'm so stoked to see that the job you've done is top notch. Thank you 🙏
- Do a verbiage check to make sure we are always using set initial or update existing. No concept of "change" anymore as it's ambiguous and we need to be crystal clear about the operations we are performing. This will be feedback to bring into all the rest of the prs as well.
- ⛏️ Throughout the MasterPasswordService I left comments on some of the operations happening throughout the set initial and update existing functions and didn't make the comments uniform. Would you mind addressing that? For example the
PrepareSetInitialMasterPasswordAsyncsays// Update time markers on the userbut I omitted that onPrepareUpdateExistingMasterPasswordAsync.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
New Issues (4)Checkmarx found the following issues in this Pull Request
|
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
There was a problem hiding this comment.
Really excellent work here. Most of the items I found were small improvements or nit picks. Thank you for your efforts to build this foundation!
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
There was a problem hiding this comment.
Stellar work. 🏅 
!
|
* feat(mp-service): Add MasterPasswordService foundation. * docs(mp-service): Resolve incoming comments, document contract. * feat(mp-service): Add KDF-setting helper and DI. * test(mp-service): Add tests. * feat(mp-service): Add enforecement in Build delegate for stamp/validate pw flags, tag data update ticket. * refactor(mp-service): Align validate/hash/compose/execute pattern. * test(mp-service): Tighten test assertions. * refactor(mp-service) chants: unlock and authenticate. * docs(mp-service): Re-fit some XML doc comment tags for general support. * docs(mp-service): Address review comment feedback. * refactor(mp-service): Apply result.Tx handling to all OneOf returns. * docs(mp-service): Refine unlock vs authentication data comments. * refactor(mp-service): Rename for saveExistingData (too much existing). * docs(mp-service): Restore PM-34905 userrepository TODOs. * refactor(mp-service): Apply test naming clarification. * refactor(mp-service): Make service internal to Core. * docs(mp-service): Update method comment formats: what, use when, constraints. * docs(mp-service): Update interface docs for consistency. * refactor(mp-service): Rename internal helpers to Apply, add documentation. * docs(mp-service): Add summary and use-when annotations to data models. * docs(mp-service): Add annotation preferring non-Build API verbs where possible. * test(mp-service): Refactor data model tests into discrete files. * test(mp-service): Address additional coverage cases. * docs(mp-service): Spelling. * refactor(mp-service): Extract user security stamp rotation to its own helper. * docs(mp-service): Clarify authentication hash documentation.
3 participants
🎟️ Tracking
PM-35392
📔 Objective
Introduces the
MasterPasswordService, an orchestrating/coordinating surface for Master Password initial set and update operations.No call sites are connected as part of this PR. For ease of review, those are specced in detail in connected tickets:
A suite of unit tests has been added for the service.
📸 Screenshots
N/A