Auth/PM-32102 - (1) Create ConvertUserToKeyConnectorCommand (2) Remove salt on key connector conversion#7692
Conversation
Introduces an opinionated in-memory mutation that nulls MasterPassword and MasterPasswordSalt together, preserving the credential/salt invariant. Updates RevisionDate and AccountRevisionDate; intentionally leaves LastPasswordChangeDate untouched since this is credential removal rather than a password change.
Extracts the Key Connector conversion logic into a dedicated command in the Key Management domain, mirroring the SetKeyConnectorKeyCommand pattern. The command composes IMasterPasswordService.PrepareClearMasterPassword to clear the credential — including the salt — before flipping UsesKeyConnector, optionally writing the wrapped user key, persisting, and emitting the migration event.
…nnectorCommand Injects IConvertUserToKeyConnectorCommand and switches both PostConvertToKeyConnectorAsync and PostEnrollToKeyConnectorAsync to use it instead of IUserService.ConvertToKeyConnectorAsync. Unit tests are retargeted to the new dependency. The integration tests now assert MasterPasswordSalt is nulled alongside MasterPassword, which is the behavior change the ticket exists to enforce.
The method's only production callers were the two endpoints on AccountsKeyManagementController, which now depend on IConvertUserToKeyConnectorCommand directly. Deleting it continues the ongoing decomposition of UserService. CheckCanUseKeyConnector stays in place — it is still called by the legacy SetKeyConnectorKeyAsync, which is scheduled for removal in PM-27328.
JaredSnider-Bitwarden
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7692 +/- ##
==========================================
- Coverage 64.86% 60.45% -4.41%
==========================================
Files 2140 2141 +1
Lines 94629 94666 +37
Branches 8445 8448 +3
==========================================
- Hits 61377 57233 -4144
- Misses 31156 35428 +4272
+ Partials 2096 2005 -91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| var now = _timeProvider.GetUtcNow().UtcDateTime; | ||
| user.RevisionDate = user.AccountRevisionDate = now; | ||
|
|
There was a problem hiding this comment.
🤔 should this also update the LastPasswordChangeDate?
It does not in the precedent (ConvertToKeyConnectorAsync), so this would constitute a behavior change.
However, AccountRevisionDate can and will be updated by discrete future actions, and IMO this does represent an intent to change (destructively, in this case) the user's master password -- is there (and should there be) another durable marker of when this user was converted to Key Connector?
| { | ||
| ArgumentNullException.ThrowIfNull(user); | ||
|
|
||
| var canUseResult = CheckCanUseKeyConnector(user); |
There was a problem hiding this comment.
🎨 Should we either rename canUseResult or add some clarifying comments? The logic is square: CanUseKeyConnector will return null if user can convert (otherwise, an IdentityResult or throw) but the method itself requires a click-in to understand that IMO. checkKeyConnectorProblemResult maybe? checkKeyConnectorIdentityResult could be good and is an established pattern as well I believe.
Just looking for ways to flag that there is no significant confirmation of success, only significant confirmation of failure.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-32102
📔 Objective
📸 Screenshots