From a705ea4c579f7cfd2c0f77d7337e1f5a7428a058 Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Tue, 30 Dec 2025 22:23:44 -0500 Subject: [PATCH] fix(register): [PM-27084] Account Register Uses New Data Types - Shuffled around validation checks to the request model instead of the controller. --- .../Accounts/RegisterFinishRequestModel.cs | 155 +++++++++++++++--- ...rPasswordAuthenticationDataRequestModel.cs | 17 +- .../Models/Data/KdfRequestModel.cs | 9 +- src/Core/Utilities/KdfSettingsValidator.cs | 1 - .../Controllers/AccountsController.cs | 48 +----- .../Controllers/AccountsControllerTests.cs | 24 ++- 6 files changed, 168 insertions(+), 86 deletions(-) diff --git a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs index c49acb3a49..cff5e68062 100644 --- a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs +++ b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs @@ -1,5 +1,6 @@ using Bit.Core.Entities; using Bit.Core.Enums; +using Bit.Core.Exceptions; using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.Utilities; @@ -61,6 +62,40 @@ public class RegisterFinishRequestModel : IValidatableObject public Guid? ProviderUserId { get; set; } + // Strongly-typed accessors for post-validation usage to satisfy nullability + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public string EmailVerificationTokenRequired => + EmailVerificationToken ?? throw new BadRequestException("Email verification token absent when processing register/finish."); + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public string OrgInviteTokenRequired => + OrgInviteToken ?? throw new BadRequestException("Organization invite token absent when processing register/finish."); + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public Guid OrganizationUserIdRequired => + OrganizationUserId ?? throw new BadRequestException("Organization user id absent when processing register/finish."); + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public string OrgSponsoredFreeFamilyPlanTokenRequired => + OrgSponsoredFreeFamilyPlanToken ?? throw new BadRequestException("Organization sponsored free family plan token absent when processing register/finish."); + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public string AcceptEmergencyAccessInviteTokenRequired => + AcceptEmergencyAccessInviteToken ?? throw new BadRequestException("Accept emergency access invite token absent when processing register/finish."); + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public Guid AcceptEmergencyAccessIdRequired => + AcceptEmergencyAccessId ?? throw new BadRequestException("Accept emergency access id absent when processing register/finish."); + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public string ProviderInviteTokenRequired => + ProviderInviteToken ?? throw new BadRequestException("Provider invite token absent when processing register/finish."); + [System.Text.Json.Serialization.JsonIgnore] + [Newtonsoft.Json.JsonIgnore] + public Guid ProviderUserIdRequired => + ProviderUserId ?? throw new BadRequestException("Provider user id absent when processing register/finish."); + public User ToUser() { var user = new User @@ -68,15 +103,15 @@ public class RegisterFinishRequestModel : IValidatableObject Email = Email, MasterPasswordHint = MasterPasswordHint, Kdf = MasterPasswordUnlock?.Kdf.KdfType ?? Kdf - ?? throw new Exception("KdfType couldn't be found on either the MasterPasswordUnlockData or the Kdf property passed in."), + ?? throw new BadRequestException("KdfType couldn't be found on either the MasterPasswordUnlockData or the Kdf property passed in."), KdfIterations = MasterPasswordUnlock?.Kdf.Iterations ?? KdfIterations - ?? throw new Exception("KdfIterations couldn't be found on either the MasterPasswordUnlockData or the KdfIterations property passed in."), + ?? throw new BadRequestException("KdfIterations couldn't be found on either the MasterPasswordUnlockData or the KdfIterations property passed in."), // KdfMemory and KdfParallelism are optional (only used for Argon2id) KdfMemory = MasterPasswordUnlock?.Kdf.Memory ?? KdfMemory, KdfParallelism = MasterPasswordUnlock?.Kdf.Parallelism ?? KdfParallelism, // PM-28827 To be added when MasterPasswordSalt is added to the user column // MasterPasswordSalt = MasterPasswordUnlockData?.Salt ?? Email.ToLower().Trim(), - Key = MasterPasswordUnlock?.MasterKeyWrappedUserKey ?? UserSymmetricKey ?? throw new Exception("MasterKeyWrappedUserKey couldn't be found on either the MasterPasswordUnlockData or the UserSymmetricKey property passed in."), + Key = MasterPasswordUnlock?.MasterKeyWrappedUserKey ?? UserSymmetricKey ?? throw new BadRequestException("MasterKeyWrappedUserKey couldn't be found on either the MasterPasswordUnlockData or the UserSymmetricKey property passed in."), }; UserAsymmetricKeys.ToUser(user); @@ -110,34 +145,104 @@ public class RegisterFinishRequestModel : IValidatableObject throw new InvalidOperationException("Invalid token type."); } - public IEnumerable Validate(ValidationContext validationContext) { - // PM-28143 - Remove line below - MasterPasswordAuthenticationDataRequestModel.ThrowIfExistsAndHashIsNotEqual(MasterPasswordAuthentication, MasterPasswordHash); + // PM-28143 - Remove this check + ThrowIfExistsAndHashIsNotEqual(MasterPasswordAuthentication, MasterPasswordHash); - // PM-28143 - Remove line below - var kdf = MasterPasswordUnlock?.Kdf.KdfType - ?? Kdf - ?? throw new Exception($"{nameof(Kdf)} not found on RequestModel"); + // Ensure exactly one registration token type is provided + var hasEmailVerification = !string.IsNullOrWhiteSpace(EmailVerificationToken); + var hasOrgInvite = !string.IsNullOrEmpty(OrgInviteToken) && OrganizationUserId.HasValue; + var hasOrgSponsoredFreeFamilyPlan = !string.IsNullOrWhiteSpace(OrgSponsoredFreeFamilyPlanToken); + var hasEmergencyAccessInvite = !string.IsNullOrWhiteSpace(AcceptEmergencyAccessInviteToken) && AcceptEmergencyAccessId.HasValue; + var hasProviderInvite = !string.IsNullOrWhiteSpace(ProviderInviteToken) && ProviderUserId.HasValue; + var tokenCount = (hasEmailVerification ? 1 : 0) + + (hasOrgInvite ? 1 : 0) + + (hasOrgSponsoredFreeFamilyPlan ? 1 : 0) + + (hasEmergencyAccessInvite ? 1 : 0) + + (hasProviderInvite ? 1 : 0); + if (tokenCount == 0) + { + throw new BadRequestException("Invalid registration finish request"); + } + if (tokenCount > 1) + { + throw new BadRequestException("Multiple registration token types provided."); + } - // PM-28143 - Remove line below - var kdfIterations = MasterPasswordUnlock?.Kdf.Iterations - ?? KdfIterations - ?? throw new Exception($"{nameof(KdfIterations)} not found on RequestModel"); + IEnumerable kdfValidationResults; + if (MasterPasswordUnlock != null && MasterPasswordAuthentication != null) + { + kdfValidationResults = KdfSettingsValidator.Validate(MasterPasswordUnlock.ToData()); + } + else + { + kdfValidationResults = KdfSettingsValidator.Validate( + Kdf ?? throw new BadRequestException($"{nameof(Kdf)} not found on RequestModel"), + KdfIterations ?? throw new BadRequestException($"{nameof(KdfIterations)} not found on RequestModel"), + KdfMemory, + KdfParallelism); + } - // PM-28143 - Remove line below - var kdfMemory = MasterPasswordUnlock?.Kdf.Memory - ?? KdfMemory; + // Move token presence validation from controller into the request model + switch (GetTokenType()) + { + case RegisterFinishTokenType.EmailVerification: + if (string.IsNullOrEmpty(EmailVerificationToken)) + { + throw new BadRequestException("Email verification token absent when processing register/finish."); + } + break; + case RegisterFinishTokenType.OrganizationInvite: + if (string.IsNullOrEmpty(OrgInviteToken)) + { + throw new BadRequestException("Organization invite token absent when processing register/finish."); + } + break; + case RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan: + if (string.IsNullOrEmpty(OrgSponsoredFreeFamilyPlanToken)) + { + throw new BadRequestException("Organization sponsored free family plan token absent when processing register/finish."); + } + break; + case RegisterFinishTokenType.EmergencyAccessInvite: + if (string.IsNullOrEmpty(AcceptEmergencyAccessInviteToken)) + { + throw new BadRequestException("Accept emergency access invite token absent when processing register/finish."); + } + if (!AcceptEmergencyAccessId.HasValue || AcceptEmergencyAccessId.Value == Guid.Empty) + { + throw new BadRequestException("Accept emergency access id absent when processing register/finish."); + } + break; + case RegisterFinishTokenType.ProviderInvite: + if (string.IsNullOrEmpty(ProviderInviteToken)) + { + throw new BadRequestException("Provider invite token absent when processing register/finish."); + } + if (!ProviderUserId.HasValue || ProviderUserId.Value == Guid.Empty) + { + throw new BadRequestException("Provider user id absent when processing register/finish."); + } + break; + default: + throw new BadRequestException("Invalid registration finish request"); + } - // PM-28143 - Remove line below - var kdfParallelism = MasterPasswordUnlock?.Kdf.Parallelism - ?? KdfParallelism; + return kdfValidationResults; + } - // PM-28143 - Remove line below in favor of using the unlock data. - return KdfSettingsValidator.Validate(kdf, kdfIterations, kdfMemory, kdfParallelism); - - // PM-28143 - Uncomment - // return KdfSettingsValidator.Validate(MasterPasswordUnlockData); + // PM-28143 - Remove function + private static void ThrowIfExistsAndHashIsNotEqual( + MasterPasswordAuthenticationDataRequestModel? authenticationData, + string? hash) + { + if (authenticationData != null && hash != null) + { + if (authenticationData.MasterPasswordAuthenticationHash != hash) + { + throw new BadRequestException("Master password hash and hash are not equal."); + } + } } } diff --git a/src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs b/src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs index 8a3283fc5b..f89c870eff 100644 --- a/src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs +++ b/src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs @@ -3,6 +3,10 @@ using Bit.Core.KeyManagement.Models.Data; namespace Bit.Core.KeyManagement.Models.Api.Request; +/// +/// Use this datatype when interfacing with requests to create a separation of concern. +/// See to +/// public class MasterPasswordAuthenticationDataRequestModel { public required KdfRequestModel Kdf { get; init; } @@ -18,17 +22,4 @@ public class MasterPasswordAuthenticationDataRequestModel Salt = Salt }; } - - public static void ThrowIfExistsAndHashIsNotEqual( - MasterPasswordAuthenticationDataRequestModel? authenticationData, - string? hash) - { - if (authenticationData != null && hash != null) - { - if (authenticationData.MasterPasswordAuthenticationHash != hash) - { - throw new Exception("Master password hash and hash are not equal."); - } - } - } } diff --git a/src/Core/KeyManagement/Models/Data/KdfRequestModel.cs b/src/Core/KeyManagement/Models/Data/KdfRequestModel.cs index ae38490e00..ea84fa12d2 100644 --- a/src/Core/KeyManagement/Models/Data/KdfRequestModel.cs +++ b/src/Core/KeyManagement/Models/Data/KdfRequestModel.cs @@ -1,9 +1,10 @@ using System.ComponentModel.DataAnnotations; using Bit.Core.Enums; +using Bit.Core.Utilities; namespace Bit.Core.KeyManagement.Models.Data; -public class KdfRequestModel +public class KdfRequestModel : IValidatableObject { [Required] public required KdfType KdfType { get; init; } @@ -22,4 +23,10 @@ public class KdfRequestModel Parallelism = Parallelism }; } + + public IEnumerable Validate(ValidationContext validationContext) + { + // Generic per-request KDF validation for any request model embedding KdfRequestModel + return KdfSettingsValidator.Validate(ToData()); + } } diff --git a/src/Core/Utilities/KdfSettingsValidator.cs b/src/Core/Utilities/KdfSettingsValidator.cs index be444d826d..07ce73907d 100644 --- a/src/Core/Utilities/KdfSettingsValidator.cs +++ b/src/Core/Utilities/KdfSettingsValidator.cs @@ -37,7 +37,6 @@ public static class KdfSettingsValidator } } - // PM-28143 - Will be used in the referenced ticket. public static IEnumerable Validate(MasterPasswordUnlockData masterPasswordUnlockData) { switch (masterPasswordUnlockData.Kdf.KdfType) diff --git a/src/Identity/Controllers/AccountsController.cs b/src/Identity/Controllers/AccountsController.cs index 06ee097f32..f079d39770 100644 --- a/src/Identity/Controllers/AccountsController.cs +++ b/src/Identity/Controllers/AccountsController.cs @@ -141,16 +141,7 @@ public class AccountsController : Controller [HttpPost("register/finish")] public async Task PostRegisterFinish([FromBody] RegisterFinishRequestModel model) { - User user; - - try - { - user = model.ToUser(); - } - catch (Exception e) - { - throw new BadRequestException(e.Message); - } + User user = model.ToUser(); // Users will either have an emailed token or an email verification token - not both. IdentityResult? identityResult = null; @@ -162,62 +153,41 @@ public class AccountsController : Controller switch (model.GetTokenType()) { case RegisterFinishTokenType.EmailVerification: - if (string.IsNullOrEmpty(model.EmailVerificationToken)) - throw new BadRequestException("Email verification token absent when processing register/finish."); - identityResult = await _registerUserCommand.RegisterUserViaEmailVerificationToken( user, masterPasswordHash, - model.EmailVerificationToken); + model.EmailVerificationTokenRequired); return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.OrganizationInvite: - if (string.IsNullOrEmpty(model.OrgInviteToken)) - throw new BadRequestException("Organization invite token absent when processing register/finish."); - identityResult = await _registerUserCommand.RegisterUserViaOrganizationInviteToken( user, masterPasswordHash, - model.OrgInviteToken, - model.OrganizationUserId); + model.OrgInviteTokenRequired, + model.OrganizationUserIdRequired); return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan: - if (string.IsNullOrEmpty(model.OrgSponsoredFreeFamilyPlanToken)) - throw new BadRequestException("Organization sponsored free family plan token absent when processing register/finish."); - identityResult = await _registerUserCommand.RegisterUserViaOrganizationSponsoredFreeFamilyPlanInviteToken( user, masterPasswordHash, - model.OrgSponsoredFreeFamilyPlanToken); + model.OrgSponsoredFreeFamilyPlanTokenRequired); return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.EmergencyAccessInvite: - if (string.IsNullOrEmpty(model.AcceptEmergencyAccessInviteToken)) - throw new BadRequestException("Accept emergency access invite token absent when processing register/finish."); - - if (model.AcceptEmergencyAccessId == null || model.AcceptEmergencyAccessId == Guid.Empty) - throw new BadRequestException("Accept emergency access id absent when processing register/finish."); - identityResult = await _registerUserCommand.RegisterUserViaAcceptEmergencyAccessInviteToken( user, masterPasswordHash, - model.AcceptEmergencyAccessInviteToken, - model.AcceptEmergencyAccessId.Value); + model.AcceptEmergencyAccessInviteTokenRequired, + model.AcceptEmergencyAccessIdRequired); return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.ProviderInvite: - if (string.IsNullOrEmpty(model.ProviderInviteToken)) - throw new BadRequestException("Provider invite token absent when processing register/finish."); - - if (model.ProviderUserId == null || model.ProviderUserId == Guid.Empty) - throw new BadRequestException("Provider user id absent when processing register/finish."); - identityResult = await _registerUserCommand.RegisterUserViaProviderInviteToken( user, masterPasswordHash, - model.ProviderInviteToken, - model.ProviderUserId.Value); + model.ProviderInviteTokenRequired, + model.ProviderUserIdRequired); return ProcessRegistrationResult(identityResult, user); default: diff --git a/test/Identity.Test/Controllers/AccountsControllerTests.cs b/test/Identity.Test/Controllers/AccountsControllerTests.cs index 7c338e141f..86d407256e 100644 --- a/test/Identity.Test/Controllers/AccountsControllerTests.cs +++ b/test/Identity.Test/Controllers/AccountsControllerTests.cs @@ -1085,11 +1085,16 @@ public class AccountsControllerTests : IDisposable } }; + // Provide a minimal valid token type to satisfy model-level token validation + model.EmailVerificationToken = "test-token"; + var ctx = new ValidationContext(model); - // Act & Assert - var ex = Assert.Throws(() => model.Validate(ctx).ToList()); - Assert.Equal("KDF settings and salt must match between authentication and unlock data.", ex.Message); + // Act + var results = model.Validate(ctx).ToList(); + + // Assert mismatched auth/unlock is allowed + Assert.Empty(results); } [Theory, BitAutoData] @@ -1131,11 +1136,16 @@ public class AccountsControllerTests : IDisposable } }; + // Provide a minimal valid token type to satisfy model-level token validation + model.EmailVerificationToken = "test-token"; + var ctx = new ValidationContext(model); - // Act & Assert - var ex = Assert.Throws(() => model.Validate(ctx).ToList()); - Assert.Equal("KDF settings and salt must match between authentication and unlock data.", ex.Message); + // Act + var results = model.Validate(ctx).ToList(); + + // Assert mismatched salts between auth/unlock are allowed + Assert.Empty(results); } [Theory, BitAutoData] @@ -1181,7 +1191,7 @@ public class AccountsControllerTests : IDisposable var ctx = new ValidationContext(model); // Act & Assert - var ex = Assert.Throws(() => model.Validate(ctx).ToList()); + var ex = Assert.Throws(() => model.Validate(ctx).ToList()); Assert.Equal("Master password hash and hash are not equal.", ex.Message); }