[PM-33162] Refactor user key rotation

[Thomas-Avery]

🎟️ Tracking

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

📔 Objective

The objective of this PR is to refactor the current change password and user key rotation endpoint to use, via composition, a new base data model. This is preparation of adding a new endpoint to support none password change userKey rotation that will share the same base data model and processing logic.

Note to Auth reviewers:
The only auth code changes was moving MasterPasswordUnlockAndAuthenticationDataModel to KM ownership.
MasterPasswordUnlockAndAuthenticationDataModel the request model is only used on the KM key rotation endpoint. Let me know if you don't agree and want to still retain ownership.

[github-actions]

Checkmarx One – Scan Summary & Details – 565c5765-d077-438a-ac0b-de7862ab6a13

---

New Issues (4)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 105	details Method at line 105 of /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs gets a parameter from a user request from model. Thi... Attack Vector
2		CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 105	details Method at line 105 of /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs gets a parameter from a user request from model. Thi... Attack Vector
3		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1592	details Method at line 1592 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector
4		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1419	details Method at line 1419 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector

---

Fixed Issues (3)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 105
	CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 105
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 293

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.71%. Comparing base (2efacd5) to head (73f6b59).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7201      +/-   ##
==========================================
+ Coverage   57.69%   57.71%   +0.01%     
==========================================
  Files        2035     2035              
  Lines       89672    89690      +18     
  Branches     7993     7990       -3     
==========================================
+ Hits        51738    51762      +24     
+ Misses      36074    36068       -6     
  Partials     1860     1860              

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Thomas-Avery]

KM team, I've realized changing this method name will effect the SDK name of the generated API bindings. Do we think that is worth it or should I just revert it back to the original name?

[quexten]

I'm OK with it, it just means we need to fix the breaking API bindings PR. I.e we run the API bindings automation on the SDK repo and do a rename on that branch, and review. Should be fairly low effort?

[Patrick-Pimentel-Bitwarden]

1. Could you help me understand why we don't use the unlock and authentication data types separately for key rotation request model?
2. Could we take the time here to update rotate account keys to standardize using the two independent request models (MasterPasswordUnlockData/MasterPasswordAuthenticationData) to make applying validation consistent and bring this endpoint in line with how all the other endpoints that concern unlock and authentication work?

[quexten]

Looks good from my side; Only one question

[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

[Thomas-Avery]

https://github.com/bitwarden/server/pull/7230/changes changed the behavior of GetMasterPasswordSalt which effects the MasterPasswordUnlockData and MasterPasswordAuthenticationData's ValidateSaltUnchangedForUser. At test runtime BitAutoData will optionally fill out MasterPasswordSalt which now would take precedent for GetMasterPasswordSalt and break expectations in the tests. For the fix just setup all the tests to fill User.MasterPasswordSalt as null.