[PM-35393] MasterPasswordService auth integration#7575
[PM-35393] MasterPasswordService auth integration#7575
Conversation
routing. Add structured cryptographic data support to all Auth password endpoints, routing new payloads to MasterPasswordService-backed commands while preserving legacy paths for backward compatibility (PM-33141 removal).
enmande
changed the title
… and model validation returns.
Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR integrates Auth-domain callers (self-service password change, admin temp-password replacement, emergency access takeover, TDE offboarding, SSO JIT, TDE set-password) with the Code Review DetailsNo new blocking, important, or debt-level findings. Substantive concerns raised in the five existing review threads (test coverage for Minor observations (informational, not posted as inline comments to keep signal high):
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7575 +/- ##
========================================
Coverage 59.62% 59.62%
========================================
Files 2097 2101 +4
Lines 92611 92914 +303
Branches 8249 8295 +46
========================================
+ Hits 55215 55397 +182
- Misses 35443 35562 +119
- Partials 1953 1955 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7575 +/- ##
==========================================
+ Coverage 59.73% 59.79% +0.05%
==========================================
Files 2102 2107 +5
Lines 92691 93031 +340
Branches 8257 8304 +47
==========================================
+ Hits 55371 55627 +256
- Misses 35366 35442 +76
- Partials 1954 1962 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ike-kottlowski
left a comment
ike-kottlowski
left a comment
There was a problem hiding this comment.
Just some concerns around the removal of salt tests.
By removing them I think we are lessening our testing coverage for ensuring the salt remains unchanged. I know there maybe concerns about business logic locations and checking the salt in a JIT User Provision unit test suite may not be the correct palace for it but having it in the wrong place is better than not having it at all.
There was a problem hiding this comment.
🎨 : there are no CODEOWNERS for this directory. I think it's probably best if we move this to an Auth owned space.
There was a problem hiding this comment.
❓ : Why were the salt related test removed? they should still pass with your changes.
There was a problem hiding this comment.
Excellent question 🔎
I think it just comes down to visibility within this PR.
We added a number of tests with the foundation PR that would support these cases when wired up, but move them to the new authorities. Most of those authorities now are the data models, for validation at the boundary. For example, this test and the one(s) above it should be covering this now where ValidateDataForUser is called -- synchronously in the set initial master password delegate.
We had both in play until this wire-up came in to preserve continuity; the connection here allows us to deprecate these tests (otherwise I believe they all are strictly duplicative).
|
|
||
| [Theory] | ||
| [BitAutoData] | ||
| public async Task ChangePasswordAsync_PassesCorrectDataToMasterPasswordService( |
There was a problem hiding this comment.
🎨 : this feels duplicative of the first success test. Can we just add the data assertion from this one to the success one?
There was a problem hiding this comment.
Yes, refactored in 123bab3. One indexed on the service invocation, and the other on the side effects. Realistically, this should be a single collection of assertions.
There was a problem hiding this comment.
🎨 : The salt tests are removed here as well. Can you speak to that?
There was a problem hiding this comment.
Excellent question 🔎
I think it just comes down to visibility within this PR.
We added a number of tests with the foundation PR that would support these cases when wired up, but move them to the new authorities. Most of those authorities now are the data models, for validation at the boundary. For example, this test and the one(s) above it should be covering this now where ValidateDataForUser is called -- synchronously in the set initial master password delegate.
We had both in play until this wire-up came in to preserve continuity; the connection here allows us to deprecate these tests (otherwise I believe they all are strictly duplicative).
|
2 participants
🎟️ Tracking
PM-35393
📔 Objective
Integrate Auth-domain callers with
MasterPasswordService.Wires five password-mutation flows to the service added in #7530 :
All relevant request models now accept both payload variants ("new":
AuthenticationData+UnlockData, or "legacy":NewMasterPasswordHash+Key) for backward compatibility.KDF validation is enforced at the request model layer whenever new payloads are present.
Legacy entry points on
IUserServiceare marked[Obsolete]with specific replacement guidance. Legacy fields are tracked for removal in PM-33141.📸 Screenshots
Master Password Self-Service
User enmande updates their Master Password using self-service.
main).pm-35393__mp-self-service.mov
TDE Offboarding
NOTE: This organization retains SSO requirement throughout; zed remains authenticated to SSO for the duration of this test, so SSO challenge is not represented in this video.
pm-35393__tde-offboarding.mov
Emergency Access Takeover
pm-35393__account-recovery.mov