docs(mp-service): Address review comment feedback.

Author: enmande

src/Core/Auth/UserFeatures/UserMasterPassword/Interfaces/IMasterPasswordService.cs

@@ -7,7 +7,7 @@
 namespace Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces;
 
 /// <summary>
-/// Centralized mutation point for all master password set, change, and rotate operations.
+/// Centralized mutation point for all master password set and update operations.
 /// Provides consistent validation, password hashing, and timestamp management across every
 /// flow that establishes or updates a user's master password.
 ///
@@ -170,8 +170,8 @@ public interface IMasterPasswordService
 
     /// <summary>
     /// Applies a new master password and updated KDF parameters over the user's existing ones
-    /// and persists the updated user to the database. Salt must remain unchanged; KDF is
-    /// intentionally allowed to change. Use for KDF rotation flows.
+    /// and persists the updated user to the database. Salt must remain unchanged; KDF 
+    /// validation is intentionally skipped. Use for KDF rotation flows.
     /// </summary>
     /// <param name="user">
     /// The user object to mutate and persist. Must already have a master password;

src/Core/Auth/UserFeatures/UserMasterPassword/MasterPasswordService.cs

@@ -51,6 +51,10 @@ public async Task<OneOf<User, IdentityError[]>> PrepareSetInitialMasterPasswordA
         EnsureUserIsHydrated(user);
         setInitialData.ValidateDataForUser(user);
 
+        // Note: Keep "unlock then authenticate" pattern in mind.
+        // This is a purposeful inversion of that principle:
+        // Authentication is derivative of unlock, and is the mechanism for validation;
+        // eager validation keeps the logic easier to reason about, so it is performed first.
         var result = await UpdateExistingPasswordHashAsync(
             user,
             setInitialData.MasterPasswordAuthentication.MasterPasswordAuthenticationHash,
@@ -67,7 +71,7 @@ public async Task<OneOf<User, IdentityError[]>> PrepareSetInitialMasterPasswordA
         // Set salt on the user
         user.MasterPasswordSalt = setInitialData.MasterPasswordUnlock.Salt;
 
-        // Always override the master password hint, even if it's null.
+        // Always override the master password hint, even if it's null
         user.MasterPasswordHint = setInitialData.MasterPasswordHint;
 
         // Update time markers on the user
@@ -103,6 +107,10 @@ public UpdateUserData BuildUpdateUserDelegateSetInitialMasterPassword(
 
         return async (connection, transaction) =>
         {
+            // Note: Keep "unlock then authenticate" pattern in mind.
+            // This is a purposeful inversion of that principle:
+            // Authentication is derivative of unlock, and is the mechanism for validation;
+            // eager validation keeps the logic easier to reason about, so it is performed first.
             if (setInitialData.ValidatePassword)
             {
                 var validate = await ValidatePasswordInternalAsync(user,
@@ -139,6 +147,10 @@ public async Task<OneOf<User, IdentityError[]>> PrepareUpdateExistingMasterPassw
         EnsureUserIsHydrated(user);
         updateExistingData.ValidateDataForUser(user);
 
+        // Note: Keep "unlock then authenticate" pattern in mind.
+        // This is a purposeful inversion of that principle:
+        // Authentication is derivative of unlock, and is the mechanism for validation;
+        // eager validation keeps the logic easier to reason about, so it is performed first.
         var result = await UpdateExistingPasswordHashAsync(
             user,
             updateExistingData.MasterPasswordAuthentication.MasterPasswordAuthenticationHash,
@@ -150,13 +162,13 @@ public async Task<OneOf<User, IdentityError[]>> PrepareUpdateExistingMasterPassw
             return result.Errors.ToArray();
         }
 
-        var now = _timeProvider.GetUtcNow().UtcDateTime;
-
         user.Key = updateExistingData.MasterPasswordUnlock.MasterKeyWrappedUserKey;
 
-        // Always override the master password hint, even if it's null.
+        // Always override the master password hint, even if it's null
         user.MasterPasswordHint = updateExistingData.MasterPasswordHint;
 
+        // Update time markers on the user
+        var now = _timeProvider.GetUtcNow().UtcDateTime;
         user.LastPasswordChangeDate = now;
         user.RevisionDate = user.AccountRevisionDate = now;
 
@@ -170,6 +182,10 @@ public async Task<OneOf<User, IdentityError[]>> SaveUpdateExistingMasterPassword
         EnsureUserIsHydrated(user);
         updateExistingExistingData.ValidateDataForUser(user);
 
+        // Note: Keep "unlock then authenticate" pattern in mind.
+        // This is a purposeful inversion of that principle:
+        // Authentication is derivative of unlock, and is the mechanism for validation;
+        // eager validation keeps the logic easier to reason about, so it is performed first.
         var result = await UpdateExistingPasswordHashAsync(
             user,
             updateExistingExistingData.MasterPasswordAuthentication.MasterPasswordAuthenticationHash,
@@ -181,14 +197,14 @@ public async Task<OneOf<User, IdentityError[]>> SaveUpdateExistingMasterPassword
             return result.Errors.ToArray();
         }
 
-        var now = _timeProvider.GetUtcNow().UtcDateTime;
-
         user.Key = updateExistingExistingData.MasterPasswordUnlock.MasterKeyWrappedUserKey;
         SetKdfStateOnUser(user, updateExistingExistingData.MasterPasswordUnlock.Kdf);
 
-        // Always override the master password hint, even if it's null.
+        // Always override the master password hint, even if it's null
         user.MasterPasswordHint = updateExistingExistingData.MasterPasswordHint;
 
+        // Update time markers on the user
+        var now = _timeProvider.GetUtcNow().UtcDateTime;
         user.LastPasswordChangeDate = now;
         user.LastKdfChangeDate = now;
         user.RevisionDate = user.AccountRevisionDate = now;