[PM-27657] KDF silent update with MasterPasswordUnlock data

[andrebispo5]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27657

📔 Objective

This pull request updates the logic for updating KDF (Key Derivation Function) settings in the vault lock management code to support additional authentication methods, and ensures that the corresponding unit test is updated to reflect this change. The main focus is to allow KDF updates when unlocking with either the standard password or the master password unlock method.

Vault lock management improvements:

• Updated VaultLockManagerImpl to support KDF updates for both InitUserCryptoMethod.Password and InitUserCryptoMethod.MasterPasswordUnlock by extracting the password from either method and passing it to kdfManager.updateKdfToMinimumsIfNeeded.

Testing improvements:

• Updated the VaultLockManagerTest to explicitly call kdfManager.updateKdfToMinimumsIfNeeded with the master password unlock init method, ensuring the new logic is covered by tests.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @andrebispo5's task —— View job

---

Code Review Complete ✓

I've reviewed the changes in this PR that add KDF silent update support for the MasterPasswordUnlock authentication method.

---

Summary of Changes

The PR modifies the VaultLockManagerImpl to support KDF updates for both InitUserCryptoMethod.Password and InitUserCryptoMethod.MasterPasswordUnlock. The implementation uses a when expression to extract the password from either method and pass it to kdfManager.updateKdfToMinimumsIfNeeded. The test suite was updated to verify this new behavior.

Files Changed:

• VaultLockManagerImpl.kt:753-775 - Refactored updateKdfIfNeeded to handle both Password and MasterPasswordUnlock methods
• VaultLockManagerTest.kt:1868 - Added test verification for MasterPasswordUnlock KDF update

---

Critical Issues

❌ Incomplete Test Coverage (Severity: High)

The test at line 1868 only verifies that updateKdfToMinimumsIfNeeded is called with the correct password for the MasterPasswordUnlock method. However, it doesn't test the actual branching logic in the when expression.

Missing Test Case: There should be a dedicated test method that specifically unlocks the vault using InitUserCryptoMethod.MasterPasswordUnlock and verifies:

1. The vault unlocks successfully
2. kdfManager.updateKdfToMinimumsIfNeeded is called with the correct password
3. The behavior is consistent with the Password method

Current Issue: The existing test unlockVault with initUserCryptoMethod masterPasswordUnlock success should hash and store master password at line 1772 already uses MasterPasswordUnlock, but the verification at line 1868 was added to a different test that uses Password method. This makes the test verification inconsistent with the test's purpose.

Suggested Test Structure

@Test
fun `unlockVault with MasterPasswordUnlock should update KDF to minimums if needed`() = runTest {
    // Test setup with InitUserCryptoMethod.MasterPasswordUnlock
    // ...
    
    val result = vaultLockManager.unlockVault(
        userId = USER_ID,
        initUserCryptoMethod = InitUserCryptoMethod.MasterPasswordUnlock(
            password = masterPassword,
            masterPasswordUnlock = mockMasterPasswordUnlockData
        ),
        // ... other params
    )
    
    assertEquals(VaultUnlockResult.Success, result)
    coVerify(exactly = 1) {
        kdfManager.updateKdfToMinimumsIfNeeded(password = masterPassword)
    }
}

---

Suggested Improvements

⚠️ Test Verification Placement (Priority: Medium)

The verification at line 1868 is placed in a test that uses InitUserCryptoMethod.Password, not MasterPasswordUnlock. While this tests the Password path, it doesn't actually verify the new MasterPasswordUnlock branch added in this PR.

Recommendation: Either:

1. Modify the existing test at line 1772 to include the KDF verification, or
2. Create a new dedicated test specifically for the MasterPasswordUnlock + KDF update scenario

---

💡 Code Quality (Priority: Low)

The when expression at line 754-764 uses an exhaustive pattern that includes all InitUserCryptoMethod cases. This is good defensive programming and ensures compile-time safety when new methods are added.

However, the implementation at line 260-285 (hashAndStoreMasterPassword) uses a similar pattern. Consider extracting the password extraction logic into a private helper function to reduce duplication:

Suggested Refactoring

private fun extractPasswordOrNull(initUserCryptoMethod: InitUserCryptoMethod): String? =
    when (initUserCryptoMethod) {
        is InitUserCryptoMethod.Password -> initUserCryptoMethod.password
        is InitUserCryptoMethod.MasterPasswordUnlock -> initUserCryptoMethod.password
        else -> null
    }

private suspend fun updateKdfIfNeeded(initUserCryptoMethod: InitUserCryptoMethod) {
    val password = extractPasswordOrNull(initUserCryptoMethod) ?: return
    // ... existing implementation
}

private suspend fun hashAndStoreMasterPassword(...) {
    val password = extractPasswordOrNull(initUserCryptoMethod) ?: return
    // ... existing implementation
}

This would make the code more maintainable and reduce the risk of the two implementations diverging.

---

Good Practices Observed

✓ Exhaustive when expression ensures compile-time safety
✓ Consistent error handling with UpdateKdfMinimumsResult.Error
✓ Early return pattern for non-password methods
✓ Proper logging of KDF update failures

---

Action Items

1. [REQUIRED] Add proper test coverage for the MasterPasswordUnlock branch in updateKdfIfNeeded
2. [RECOMMENDED] Move the KDF verification to the appropriate test or create a dedicated test
3. [OPTIONAL] Consider extracting password extraction logic to reduce duplication

---

Coverage Report Analysis

The Codecov report shows 77.78% patch coverage with 2 lines missing coverage at VaultLockManagerImpl.kt:754-756. This confirms that the new MasterPasswordUnlock branch is not being tested, which aligns with the critical issue identified above.

---

[github-actions]

Checkmarx One – Scan Summary & Details – c936ae9e-e932-4090-a581-537271a5b726

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

[codecov]

Codecov Report

❌ Patch coverage is 46.66667% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.81%. Comparing base (d86443c) to head (b557607).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...twarden/data/vault/manager/VaultLockManagerImpl.kt	46.66%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6113      +/-   ##
==========================================
- Coverage   84.82%   84.81%   -0.02%     
==========================================
  Files         721      735      +14     
  Lines       52824    53144     +320     
  Branches     7667     7676       +9     
==========================================
+ Hits        44808    45074     +266     
- Misses       5328     5379      +51     
- Partials     2688     2691       +3     

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