Add UpdateAccountCryptographicState repository function

[quexten]

🎟️ Tracking

📔 Objective

Unblock v2 account registration related tasks.

📸 Screenshots

⏰ 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 @quexten's task —— View job

---

Code Review Complete

Tasks

• Analyze PR context and recent feedback
• Review code changes for security, correctness, and best practices
• Provide comprehensive review feedback

---

Overall Assessment

This PR adds a new repository method for updating user cryptographic state as part of v2 account registration. While the implementation follows Bitwarden patterns and handles transactions correctly, there are critical issues with the test suite that must be addressed before merge.

❌ Critical Issues

1. Test Suite Contains Invalid V1 Test Case

File: test/Infrastructure.EFIntegration.Test/Repositories/UserRepositoryTests.cs:325-348

The test creates V1 account keys data (without SignatureKeyPairData and SecurityStateData) and attempts to pass it to SetV2AccountCryptographicStateAsync:

var accountKeysDataV1 = new UserAccountKeysData
{
    PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData(
        wrappedPrivateKey: "v1-wrapped-private-key",
        publicKey: "v1-public-key"
    )
};
await sut.SetV2AccountCryptographicStateAsync(createdUser.Id, accountKeysDataV1);

Problem:

• accountKeysDataV1.IsV2Encryption() returns false because all V2 fields are null
• SetV2AccountCryptographicStateAsync checks if (!accountKeysData.IsV2Encryption()) and throws ArgumentException
• This test will fail immediately and block CI

Fix Required: Either:

1. Remove the V1 test entirely (since method is V2-only per documentation), or
2. Change it to verify V1 data properly throws ArgumentException

2. Null-Forgiving Operators Risk Runtime Exceptions

File: src/Infrastructure.Dapper/Repositories/UserRepository.cs:318-323

SecurityState = accountKeysData.SecurityStateData!.SecurityState,
SecurityVersion = accountKeysData.SecurityStateData!.SecurityVersion,
// ... more null-forgiving operators

Problem: If IsV2Encryption() validation is ever bypassed or removed, these null-forgiving operators (!) will cause NullReferenceException at runtime.

Recommendation: Add defensive null checks:

if (accountKeysData.SecurityStateData == null || accountKeysData.SignatureKeyPairData == null)
{
    throw new InvalidOperationException("V2 encryption data incomplete.");
}

⚠️ Important Issues

3. Missing User Existence Validation in Dapper

File: src/Infrastructure.Dapper/Repositories/UserRepository.cs:310-328

The Dapper implementation calls the stored procedure without verifying the UPDATE affected any rows. If userId doesn't exist:

• The stored procedure UPDATE affects 0 rows
• It then attempts to INSERT into UserSignatureKeyPair with a non-existent UserId
• This may succeed or fail depending on foreign key constraints, leading to inconsistent behavior

Comparison: The EF implementation properly validates user existence at line 261-265:

var userEntity = await dbContext.Users.FindAsync(userId);
if (userEntity == null)
{
    throw new ArgumentException("User not found", nameof(userId));
}

Recommendation: Add similar validation to Dapper implementation or modify the stored procedure to return affected row count.

4. Insufficient Test Coverage

File: test/Infrastructure.EFIntegration.Test/Repositories/UserRepositoryTests.cs

Missing test cases:

• ❌ User not found scenario (should throw ArgumentException)
• ❌ V1 data passed to V2 method (should throw ArgumentException)
• ❌ Updating existing signature key pair (test only covers creation)
• ❌ Transaction rollback on failure
• ❌ Additional updateUserDataActions execution and failure

Recommendation: Add comprehensive error case testing:

[CiSkippedTheory, EfUserAutoData]
public async Task SetV2AccountCryptographicStateAsync_InvalidV1Data_ThrowsArgumentException(...)
{
    var accountKeysDataV1 = new UserAccountKeysData { ... };
    await Assert.ThrowsAsync<ArgumentException>(() => 
        sut.SetV2AccountCryptographicStateAsync(userId, accountKeysDataV1));
}

[CiSkippedTheory, EfUserAutoData]
public async Task SetV2AccountCryptographicStateAsync_UserNotFound_ThrowsArgumentException(...)
{
    await Assert.ThrowsAsync<ArgumentException>(() => 
        sut.SetV2AccountCryptographicStateAsync(Guid.NewGuid(), validV2Data));
}

5. Stored Procedure Lacks Input Validation

File: src/Sql/dbo/Stored Procedures/User_UpdateAccountCryptographicState.sql

The stored procedure accepts all parameters as nullable and doesn't validate whether the user exists. Silent failures can occur if the user doesn't exist.

Recommendation: Add validation at the beginning:

IF NOT EXISTS (SELECT 1 FROM [dbo].[User] WHERE [Id] = @Id)
BEGIN
    RAISERROR('User not found', 16, 1)
    RETURN
END

🎨 Technical Debt Concerns

6. Inconsistent Signature Key Pair ID Generation

• EF Implementation (line 302): Uses SetNewId() after setting properties
• Dapper Implementation (line 302): Uses CoreHelpers.GenerateComb() directly

Recommendation: Call SetNewId() before setting other properties for consistency.

7. Documentation Typo

File: src/Core/Repositories/IUserRepository.cs:50

/// The provided MUST be a V2 encryption state.

Should be: "The provided data MUST be a V2 encryption state."

✅ Security Assessment - Positive Findings

1. ✅ Cryptographic data handling follows existing patterns (encrypted strings)
2. ✅ No logging of sensitive cryptographic data
3. ✅ Zero-knowledge principles maintained (server stores encrypted keys only)
4. ✅ Proper transaction isolation prevents race conditions
5. ✅ Parameterized queries prevent SQL injection
6. ✅ V2 encryption validation enforces data integrity via IsV2Encryption() check

✅ Transaction Atomicity - Correct Implementation

Both implementations correctly handle transactions:

1. ✅ Explicit transaction scope for user + signature key pair updates
2. ✅ Optional additional actions executed within same transaction
3. ✅ Proper rollback on exception
4. ✅ Async commit/rollback operations

✅ Adherence to Bitwarden Patterns

• ✅ Follows existing repository patterns
• ✅ Consistent with other transaction-based update methods
• ✅ Proper separation of Dapper and EF implementations
• ✅ Uses CoreHelpers.GenerateComb() for ID generation
• ✅ Follows nullable reference type conventions

Summary of Required Actions

Must fix before merge:

1. ❌ Fix or remove V1 test case (lines 325-348 in UserRepositoryTests.cs)
2. ❌ Add error case test coverage (user not found, invalid V1 data)
3. ⚠️ Add user existence validation to Dapper implementation

Should address:

1. Add validation to stored procedure
2. Fix documentation typo in IUserRepository.cs
3. Consider defensive null checks instead of null-forgiving operators

Consider for future:

1. Add test for existing signature key pair update scenario
2. Add audit logging for cryptographic state changes (without sensitive data)

---

The implementation demonstrates solid understanding of transaction management and cryptographic data handling, but the test suite issues must be resolved before merge as the current V1 test will cause CI failures.

[github-actions]

Checkmarx One – Scan Summary & Details – 5091afa9-74cd-4635-b6f6-85f272e1f5e8

New Issues (3)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 427	details Method at line 427 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow... ID: VU8sgdorwW4N9vJXTUl%2BCpjmMcY%3D Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1519	details Method at line 1519 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: dMGF5qNfAN72zlvQcA1MgbhHv%2Fc%3D Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1395	details Method at line 1395 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: iOCFr11iI9znjDnv46yLfiS4aDY%3D Attack Vector

Fixed Issues (6)

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

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 507
	CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 97
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 207
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 87
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 97
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 300

[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

[codecov]

Codecov Report

❌ Patch coverage is 0% with 104 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.66%. Comparing base (b1390c9) to head (b4681cc).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
...ure.EntityFramework/Repositories/UserRepository.cs	0.00%	51 Missing ⚠️
...frastructure.Dapper/Repositories/UserRepository.cs	0.00%	43 Missing ⚠️
...e/KeyManagement/Models/Data/UserAccountKeysData.cs	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6669      +/-   ##
==========================================
+ Coverage   53.65%   57.66%   +4.01%     
==========================================
  Files        1926     1914      -12     
  Lines       85720    85061     -659     
  Branches     7687     7610      -77     
==========================================
+ Hits        45989    49049    +3060     
+ Misses      37958    34177    -3781     
- Partials     1773     1835      +62     

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

[mzieniukbw]

I am not entirely sure whether we can and should use the same command for SSO JIT and master password user registration flows, since in one we update the existing user and in the other we need to create the user with all the keys.

[mzieniukbw]

With the Id set, this will trigger Update, instead of Create / Insert, is it intended ?
Note: when Id is omitted, upsert would trigger Create and set the Id automatically.

[quexten]

I have changed this to CreateAsync, and added docs clarifying this command should only be used for setting the initial keys, not updating.

[mzieniukbw]

Since we want to use this for user registration for now, it would make sense to call CreateAsync directly.

[mzieniukbw]

While this is true for SSO JIT, user registration with master password will not have user created yet (check

server/src/Identity/Controllers/AccountsController.cs

Line 157 in 5fb69e4

await _registerUserCommand.RegisterUserViaEmailVerificationToken(user, model.MasterPasswordHash,

).
Unless the intention is to ensure that user is created first without the keys, and then the keys are updated with this command. Does not feel great.

[quexten]

As mentioned in the other comment, I've now updated the comment and the command is now meant for just setting initial keys, so JIT password / regular MP have to find a different solution / write their own command that atomically sets all their data.

[mzieniukbw]

🤔 For the case where user is created, maybe it would make more sense to provide User user instead of Guid ?

[quexten]

Makes sense. Updated to take a user. I also added a comment since the user that is passed in does get mutated in the process, which may be unintuitive.

[mzieniukbw]

some docs would be useful

[mkincaid-bw]

LGTM