[PM-27278] add AccountKeysRequestModel to RegisterFinishRequestModel for account encryption v2 support by eligrubb · Pull Request #6798 · bitwarden/server

eligrubb · 2026-01-05T16:17:41Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27278
Stacks on top of #6855.

📔 Objective

Deprecating V1 User Asymmetric Key information in favor of new V2 User Asymmetric Account Keys structure.

This PR adds support for the new AccountKeys structure while maintaining support for the legacy UserAsymmetricKey-based flow. Validation is updated to check either AccountKeys or UserAsymmetricKeys are updated. Tests include modeling for both scenarios.

📸 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

…tial changes

…ed up reference to master password hash

…d more comments and fixed up some long lines.

…unts controller no longer nullish allowed.

…e thrown error messages more appropriate

…ed tests.

…tModel.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

…ed comments for tests.

…t additions to make sure properties that were once required are still required.

…ted tests and added comments for the future.

…ed new checks from review.

…tModel.cs Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com>

…d new checks for master password authentication data.

…d some tests up.

…d more tests.

…ved invalid check.

…fled around validation checks to the request model instead of the controller.

…Added comments to clarify how these data types should be used.

…Fixed import.

…Updated docs around the validation

…Removed troublesome null safeguarding.

…Removed debug file.

…Fixed error in register finish model validation.

…ed tests.

…ed accounts controller tests.

…essed concerns from reviewer.

…d up tests a little more.

…epts-new-data-types

…ressed more feedback. No longer overriding the master password hash.

…ed tests.

…s-repush

…epts-new-data-types-repush

…d constant for feature flag.

…s-repush

…for account encryption v2 support

claude · 2026-02-04T07:01:02Z

Claude finished @eligrubb's task in 2m 13s —— View job

Code Review Summary: PR #6798

PM-27278 add AccountKeysRequestModel to RegisterFinishRequestModel for account encryption v2 support

Overview

This PR introduces V2 account encryption support for user registration. The changes allow the RegisterFinish endpoint to handle both legacy (V1) and new (V2) encryption schemes during the registration process.

Key Changes Reviewed

New data models: RegisterFinishData, modifications to UserAccountKeysData, MasterPasswordUnlockData, MasterPasswordAuthenticationData
Modified RegisterFinishRequestModel with new properties and comprehensive validation
Updated IRegisterUserCommand interface to pass RegisterFinishData instead of string masterPasswordHash
New repository method SetRegisterFinishUserData with corresponding SQL stored procedure
Flow changes in UserService.CreateUserAsync to handle V2 encryption

Review Result

APPROVED - The implementation is well-structured and maintains backward compatibility.

Positive Observations

Backward Compatibility: The code gracefully handles both V1 and V2 encryption schemes through the IsV2Encryption() check
Validation: Comprehensive input validation in RegisterFinishRequestModel.Validate() with clear error messages
Transaction Safety: The V2 encryption flow properly uses transactions via SetV2AccountCryptographicStateAsync
Security: No sensitive data exposed in logs or error messages; zero-knowledge principles maintained
Established Patterns: Uses existing patterns like UpdateUserData delegate for transactional operations

Test Coverage Note

While codecov reports low coverage, the PR includes unit tests in:

RegisterFinishRequestModelTests.cs - validation and data conversion tests
AccountsControllerTests.cs - integration tests for the registration flow

Reviewed with Claude Code

quexten · 2026-02-06T09:09:08Z

src/Core/Services/Implementations/UserService.cs

+        var result = await CreateAsync(user, registerFinishData.MasterPasswordAuthenticationData.MasterPasswordAuthenticationHash);
+        if (result.Succeeded)
+        {
+            var setRegisterFinishUserDataTask = _userRepository.SetRegisterFinishUserData(user.Id, registerFinishData);


Question: SetRegisterFinishUserData sets:
{

KDF Settings

MasterKey Protected Userkey
} // masterpassword unlock
and
{
public key
private key
} // keypair

But SetV2AccountCryptographicState already writes the keypair

server/src/Infrastructure.Dapper/Repositories/UserRepository.cs

Line 317 in 0159052

PublicKey = accountKeysData.PublicKeyEncryptionKeyPairData.PublicKey,

So essentially we are writing the keypair twice here. Could we remove the key-pair from the registerFinishUserData task / SQL sproc?

quexten · 2026-02-06T09:09:59Z

util/Migrator/DbScripts/2026-01-30_00_User_SetRegisterFinishUserData.sql

+    @KdfMemory INT,
+    @KdfParallelism INT,
+    @Key VARCHAR(MAX),
+    @PublicKey VARCHAR(MAX),


As per the other comment, we can probably remove this. Maybe it makes sense to just rename to "SetMasterPasswordUnlockData"?

Done. I went with SetMasterPasswordUnlockUserData

quexten · 2026-02-06T09:12:47Z

test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs

+        // Arrange
+        // V1 model and fields to be removed with
+        // https://bitwarden.atlassian.net/browse/PM-27326
+        var legacyModel = new RegisterFinishRequestModel


Nit/quesion (don't have to act on it): I wonder whether these would be better served as separate tests, one legacy one not.

I'm open to either, whichever is seen as more maintainable. I put the testing the legacy & new models in one test because I wanted to match what Auth had already been doing in the file. So if Auth feels strongly one way or the other I'd like to go with that.

quexten · 2026-02-06T09:13:27Z

test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs

+        // all should be default/unset, with the exception of email and masterPasswordHint
+        Assert.Equal(email, newResult.Email);
+        Assert.Equal(masterPasswordHint, newResult.MasterPasswordHint);
+        Assert.Equal(KdfType.PBKDF2_SHA256, newResult.Kdf);


Question: Don't e expect argon2 for v2? Subsequently memory and praallelism should also be set?

This test in particular is checking the return of the ToUser function, which I changed for v2 users to only set the email and MasterPasswordHint. All others values in the User entity will be the default value, which for Kdf settings means PBKDF2 for type & iterations, and null for memory & parallelism

quexten · 2026-02-06T09:21:09Z

test/Identity.Test/Controllers/AccountsControllerTests.cs

+        var kdfModel = new KdfRequestModel
+        {
+            KdfType = KdfType.Argon2id,
+            Iterations = AuthConstants.ARGON2_ITERATIONS.Default


Missing memory / parallelism?

quexten · 2026-02-06T09:21:29Z

test/Identity.Test/Controllers/AccountsControllerTests.cs

+        var kdfModel = new KdfRequestModel
+        {
+            KdfType = KdfType.Argon2id,
+            Iterations = AuthConstants.ARGON2_ITERATIONS.Default


Missing Memory / parallelism

quexten · 2026-02-06T09:21:50Z

test/Identity.Test/Controllers/AccountsControllerTests.cs

-        var user = model.ToUser();
+        var kdfModel = new KdfRequestModel
+        {
+            KdfType = KdfType.Argon2id,


Memory / parallelism?

quexten · 2026-02-06T09:23:33Z

test/Identity.Test/Controllers/AccountsControllerTests.cs

+            KdfMemory = kdfData.Memory,
+            KdfParallelism = kdfData.Parallelism,
+            UserSymmetricKey = masterKeyWrappedUserKey,
+            UserAsymmetricKeys = new KeysRequestModel


Which user is this in the test? A V1 user should use PBKDF2, a V2 User Argon2 but a v2 user should also have the other required values, not just public key / encrypted private key?

Good catch, this should be a v1 user. Changed the values to PBKDF2.

sonarqubecloud · 2026-02-06T23:10:02Z

Quality Gate passed

Issues
7 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

Patrick-Pimentel-Bitwarden and others added 30 commits December 9, 2025 16:13

feat(register): [PM-27084] Account Register Uses New Data Types - Ini…

6c7daa6

…tial changes

feat(register): [PM-27084] Account Register Uses New Data Types - Fix…

1535fa3

…ed up reference to master password hash

fix(register): [PM-27084] Account Register Uses New Data Types - Adde…

9000474

…d more comments and fixed up some long lines.

fix(register): [PM-27084] Account Register Uses New Data Types - Acco…

d7ddf2e

…unts controller no longer nullish allowed.

docs(register): [PM-27084] Account Register Uses New Data Types - Mad…

08f3acd

…e thrown error messages more appropriate

test(register): [PM-27084] Account Register Uses New Data Types - Add…

f47dac9

…ed tests.

Update src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishReques…

bb2ba14

…tModel.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

Merge branch 'main' into auth/pm-27084/register-accepts-new-data-types

7cb55eb

docs(register): [PM-27084] Account Register Uses New Data Types - Add…

b33e1ac

…ed comments for tests.

test(register): [PM-27084] Account Register Uses New Data Types - Tes…

c21d7b4

…t additions to make sure properties that were once required are still required.

test(register): [PM-27084] Account Register Uses New Data Types - Fix…

aa8e8cc

…ted tests and added comments for the future.

test(register): [PM-27084] Account Register Uses New Data Types - Add…

5b2edef

…ed new checks from review.

Merge branch 'main' into auth/pm-27084/register-accepts-new-data-types

57f69ad

Update src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishReques…

4f81d75

…tModel.cs Co-authored-by: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com>

fix(register): [PM-27084] Account Register Uses New Data Types - Adde…

fac1d4b

…d new checks for master password authentication data.

fix(register): [PM-27084] Account Register Uses New Data Types - Fixe…

2d9786e

…d some tests up.

fix(register): [PM-27084] Account Register Uses New Data Types - Adde…

cc895d0

…d more tests.

fix(register): [PM-27084] Account Register Uses New Data Types - Remo…

4475649

…ved invalid check.

fix(register): [PM-27084] Account Register Uses New Data Types - Shuf…

a705ea4

…fled around validation checks to the request model instead of the controller.

comment(register): [PM-27084] Account Register Uses New Data Types - …

227f3a0

…Added comments to clarify how these data types should be used.

comment(register): [PM-27084] Account Register Uses New Data Types - …

fb54d21

…Fixed import.

comment(register): [PM-27084] Account Register Uses New Data Types - …

b0cce70

…Updated docs around the validation

comment(register): [PM-27084] Account Register Uses New Data Types - …

af2f704

…Removed troublesome null safeguarding.

comment(register): [PM-27084] Account Register Uses New Data Types - …

015d2ed

…Removed debug file.

Merge branch 'main' into auth/pm-27084/register-accepts-new-data-types

67ff0da

comment(register): [PM-27084] Account Register Uses New Data Types - …

fc507a4

…Fixed error in register finish model validation.

test(register): [PM-27084] Account Register Uses New Data Types - Fix…

05d8cc5

…ed tests.

test(register): [PM-27084] Account Register Uses New Data Types - Fix…

e531ab1

…ed accounts controller tests.

fix(register): [PM-27084] Account Register Uses New Data Types - Addr…

28640d0

…essed concerns from reviewer.

fix(register): [PM-27084] Account Register Uses New Data Types - Fixe…

260b289

…d up tests a little more.

Patrick-Pimentel-Bitwarden added 3 commits January 12, 2026 15:20

Merge remote-tracking branch 'origin' into auth/pm-27084/register-acc…

f73904d

…epts-new-data-types

test(register): [PM-27084] Account Register Uses New Data Types - Add…

f92d7d2

…ressed more feedback. No longer overriding the master password hash.

test(register): [PM-27084] Account Register Uses New Data Types - Fix…

6301dbf

…ed tests.

Base automatically changed from auth/pm-27084/register-accepts-new-data-types to main January 15, 2026 20:55

Patrick-Pimentel-Bitwarden added 3 commits January 15, 2026 16:26

Merge branch 'main' into auth/pm-27084/register-accepts-new-data-type…

01c8171

…s-repush

Merge remote-tracking branch 'origin' into auth/pm-27084/register-acc…

968692c

…epts-new-data-types-repush

fix(register): [PM-27084] Account Register Uses New Data Types - Adde…

e14f6a4

…d constant for feature flag.

eligrubb force-pushed the km/pm-27278/register-password-based-account-v2 branch from 64585b7 to 2a7a753 Compare February 2, 2026 09:54

eligrubb changed the base branch from main to auth/pm-27084/register-accepts-new-data-types-repush February 2, 2026 09:57

bnagawiecki and others added 3 commits February 2, 2026 14:25

Merge branch 'main' into auth/pm-27084/register-accepts-new-data-type…

fb98850

…s-repush

Merge branch 'main' into auth/pm-27084/register-accepts-new-data-type…

d9b53c4

…s-repush

[PM-27278] add AccountKeysRequestModel to RegisterFinishRequestModel …

c026649

…for account encryption v2 support

eligrubb force-pushed the km/pm-27278/register-password-based-account-v2 branch from 2a7a753 to c026649 Compare February 4, 2026 05:55

eligrubb added the ai-review Request a Claude code review label Feb 4, 2026

Fixed lint and build errors.

a247343

eligrubb removed the ai-review Request a Claude code review label Feb 4, 2026

eligrubb marked this pull request as ready for review February 4, 2026 07:10

eligrubb requested review from a team as code owners February 4, 2026 07:10

eligrubb requested review from quexten and rr-bw and removed request for a team February 4, 2026 07:10

Base automatically changed from auth/pm-27084/register-accepts-new-data-types-repush to main February 4, 2026 15:04

rr-bw requested a review from enmande February 4, 2026 17:33

quexten reviewed Feb 6, 2026

View reviewed changes

eligrubb added 2 commits February 6, 2026 11:37

Merge main into branch

6bcbb69

Addressing code review comments

4341d21

Conversation

eligrubb commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

claude bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Summary: PR #6798

Overview

Key Changes Reviewed

Review Result

Positive Observations

Test Coverage Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 6, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eligrubb commented Jan 5, 2026 •

edited

Loading

claude bot commented Feb 4, 2026 •

edited

Loading