[PM-30370] Add salt to vault sync response

[ike-kottlowski]

🎟️ Tracking

PM-30370

📔 Objective

On sync retrieve the User.GetMasterPasswordSalt() method instead of the email.

[claude]

Claude finished @ike-kottlowski's task in 2m 47s —— View job

---

Code Review: PM-30370 Add salt to vault sync response

• Gather context and review PR diff
• Analyze changed files
• Post review findings

---

Critical Finding

GetMasterPasswordSalt() never uses the MasterPasswordSalt property — src/Core/Entities/User.cs:117-120

The method currently always returns Email.ToLowerInvariant().Trim(), completely ignoring the MasterPasswordSalt property:

public string GetMasterPasswordSalt()
{
    return Email.ToLowerInvariant().Trim();
}

This means:

1. The two new tests (Get_HaveExplicitMasterPasswordSalt_SaltReturnedInSync and Constructor_UserWithExplicitSalt_UsesMasterPasswordSalt) that set MasterPasswordSalt = "explicit-salt-value" and assert on that value will fail.
2. The PR's goal — returning an explicit salt when one exists — does not actually work yet.

The fix should add a null-coalescing fallback in the method:

public string GetMasterPasswordSalt()
{
    return MasterPasswordSalt ?? Email.ToLowerInvariant().Trim();
}

Fix this →

---

Other Changes Look Good

• SyncResponseModel.cs: Correctly swaps user.Email.ToLowerInvariant() for user.GetMasterPasswordSalt() — this is the right call site change.
• Unit tests (SyncResponseModelTests.cs): Good coverage with both null-salt and explicit-salt scenarios. The user.MasterPasswordSalt = null setup on the existing test is a nice touch.
• Integration test (SyncControllerTests.cs): End-to-end test correctly sets explicit salt and verifies it round-trips through sync.
• Unit test (SyncControllerTests.cs): Assertion updated consistently.

All tests are well-structured and will pass once GetMasterPasswordSalt() is fixed.

---

[claude]

❌ CRITICAL: GetMasterPasswordSalt() never reads User.MasterPasswordSalt -- it always returns Email.ToLowerInvariant().Trim(), so the new explicit-salt tests will fail and the feature does not work.

Details and fix

Commit 0240c94ad message says "add null coalesce to GetMasterPasswordSalt" but the method body in src/Core/Entities/User.cs line 117-120 was never updated:

public string GetMasterPasswordSalt()
{
    return Email.ToLowerInvariant().Trim();
}

It should be:

public string GetMasterPasswordSalt()
{
    return MasterPasswordSalt ?? Email.ToLowerInvariant().Trim();
}

Without this change, both Get_HaveExplicitMasterPasswordSalt_SaltReturnedInSync and Constructor_UserWithExplicitSalt_UsesMasterPasswordSalt will fail because they set MasterPasswordSalt = "explicit-salt-value" but the method ignores it and returns the email.

[claude]

🚨 CRITICAL — GetMasterPasswordSalt() in src/Core/Entities/User.cs:117-120 never checks the MasterPasswordSalt property — it always returns Email.ToLowerInvariant().Trim(). The two new tests that set MasterPasswordSalt = "explicit-salt-value" will fail.

Suggested fix in User.cs:

public string GetMasterPasswordSalt()
{
    return MasterPasswordSalt ?? Email.ToLowerInvariant().Trim();
}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Checkmarx One – Scan Summary & Details – 8de3d6fe-f817-4f02-abbe-b1a064f1f837

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