From 44249c38e00da27ec83f6b9e8dbca6644b3161b9 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 15 Jan 2026 03:52:00 -0500 Subject: [PATCH 1/7] Add some integration tests for the Server project (#6839) * Add some integration tests for the Server project * Not sure why this project got removed? * Format * capture debug output * Update tests to work with the now legacy WebHostBuilder - I accidentally had the updated Program locally and that was why tests were working for me locally * Formatting...again --- bitwarden-server.sln | 8 ++ .../Properties/AssemblyInfo.cs | 1 + .../Server.IntegrationTest.csproj | 23 ++++ test/Server.IntegrationTest/Server.cs | 45 ++++++++ test/Server.IntegrationTest/ServerTests.cs | 102 ++++++++++++++++++ util/Server/Program.cs | 10 +- 6 files changed, 187 insertions(+), 2 deletions(-) create mode 100644 test/Server.IntegrationTest/Properties/AssemblyInfo.cs create mode 100644 test/Server.IntegrationTest/Server.IntegrationTest.csproj create mode 100644 test/Server.IntegrationTest/Server.cs create mode 100644 test/Server.IntegrationTest/ServerTests.cs diff --git a/bitwarden-server.sln b/bitwarden-server.sln index ae9571a4a5..409906e2d0 100644 --- a/bitwarden-server.sln +++ b/bitwarden-server.sln @@ -140,10 +140,13 @@ EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SeederApi", "util\SeederApi\SeederApi.csproj", "{9F08DFBB-482B-4C9D-A5F4-6BDA6EC2E68F}" EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SeederApi.IntegrationTest", "test\SeederApi.IntegrationTest\SeederApi.IntegrationTest.csproj", "{A2E067EF-609C-4D13-895A-E054C61D48BB}" +EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SSO.Test", "bitwarden_license\test\SSO.Test\SSO.Test.csproj", "{7D98784C-C253-43FB-9873-25B65C6250D6}" EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Sso.IntegrationTest", "bitwarden_license\test\Sso.IntegrationTest\Sso.IntegrationTest.csproj", "{FFB09376-595B-6F93-36F0-70CAE90AFECB}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Server.IntegrationTest", "test\Server.IntegrationTest\Server.IntegrationTest.csproj", "{E75E1F10-BC6F-4EB1-BA75-D897C45AEA0D}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -372,6 +375,10 @@ Global {FFB09376-595B-6F93-36F0-70CAE90AFECB}.Debug|Any CPU.Build.0 = Debug|Any CPU {FFB09376-595B-6F93-36F0-70CAE90AFECB}.Release|Any CPU.ActiveCfg = Release|Any CPU {FFB09376-595B-6F93-36F0-70CAE90AFECB}.Release|Any CPU.Build.0 = Release|Any CPU + {E75E1F10-BC6F-4EB1-BA75-D897C45AEA0D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {E75E1F10-BC6F-4EB1-BA75-D897C45AEA0D}.Debug|Any CPU.Build.0 = Debug|Any CPU + {E75E1F10-BC6F-4EB1-BA75-D897C45AEA0D}.Release|Any CPU.ActiveCfg = Release|Any CPU + {E75E1F10-BC6F-4EB1-BA75-D897C45AEA0D}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -432,6 +439,7 @@ Global {A2E067EF-609C-4D13-895A-E054C61D48BB} = {DD5BD056-4AAE-43EF-BBD2-0B569B8DA84F} {7D98784C-C253-43FB-9873-25B65C6250D6} = {287CFF34-BBDB-4BC4-AF88-1E19A5A4679B} {FFB09376-595B-6F93-36F0-70CAE90AFECB} = {287CFF34-BBDB-4BC4-AF88-1E19A5A4679B} + {E75E1F10-BC6F-4EB1-BA75-D897C45AEA0D} = {DD5BD056-4AAE-43EF-BBD2-0B569B8DA84F} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {E01CBF68-2E20-425F-9EDB-E0A6510CA92F} diff --git a/test/Server.IntegrationTest/Properties/AssemblyInfo.cs b/test/Server.IntegrationTest/Properties/AssemblyInfo.cs new file mode 100644 index 0000000000..80afc76e2e --- /dev/null +++ b/test/Server.IntegrationTest/Properties/AssemblyInfo.cs @@ -0,0 +1 @@ +[assembly: CaptureTrace] diff --git a/test/Server.IntegrationTest/Server.IntegrationTest.csproj b/test/Server.IntegrationTest/Server.IntegrationTest.csproj new file mode 100644 index 0000000000..362ada84a0 --- /dev/null +++ b/test/Server.IntegrationTest/Server.IntegrationTest.csproj @@ -0,0 +1,23 @@ + + + + Exe + enable + + + + + + + + + + + + + + + + + + diff --git a/test/Server.IntegrationTest/Server.cs b/test/Server.IntegrationTest/Server.cs new file mode 100644 index 0000000000..073dbffb5a --- /dev/null +++ b/test/Server.IntegrationTest/Server.cs @@ -0,0 +1,45 @@ +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; + +namespace Bit.Server.IntegrationTest; + +public class Server : WebApplicationFactory +{ + public string? ContentRoot { get; set; } + public string? WebRoot { get; set; } + public bool ServeUnknown { get; set; } + public bool? WebVault { get; set; } + public string? AppIdLocation { get; set; } + + protected override IWebHostBuilder? CreateWebHostBuilder() + { + var args = new List + { + "/contentRoot", + ContentRoot ?? "", + "/webRoot", + WebRoot ?? "", + "/serveUnknown", + ServeUnknown.ToString().ToLowerInvariant(), + }; + + if (WebVault.HasValue) + { + args.Add("/webVault"); + args.Add(WebVault.Value.ToString().ToLowerInvariant()); + } + + if (!string.IsNullOrEmpty(AppIdLocation)) + { + args.Add("/appIdLocation"); + args.Add(AppIdLocation); + } + + var builder = WebHostBuilderFactory.CreateFromTypesAssemblyEntryPoint([.. args]) + ?? throw new InvalidProgramException("Could not create builder from assembly."); + + builder.UseSetting("TEST_CONTENTROOT_SERVER", ContentRoot); + return builder; + } +} diff --git a/test/Server.IntegrationTest/ServerTests.cs b/test/Server.IntegrationTest/ServerTests.cs new file mode 100644 index 0000000000..e432f53775 --- /dev/null +++ b/test/Server.IntegrationTest/ServerTests.cs @@ -0,0 +1,102 @@ +using System.Net; +using System.Runtime.CompilerServices; + +namespace Bit.Server.IntegrationTest; + +public class ServerTests +{ + [Fact] + public async Task AttachmentsStyleUse() + { + using var tempDir = new TempDir(); + + await tempDir.WriteAsync("my-file.txt", "Hello!"); + + using var server = new Server + { + ContentRoot = tempDir.Info.FullName, + WebRoot = ".", + ServeUnknown = true, + }; + + var client = server.CreateClient(); + + var response = await client.GetAsync("/my-file.txt", TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("Hello!", await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task WebVaultStyleUse() + { + using var tempDir = new TempDir(); + + await tempDir.WriteAsync("index.html", ""); + await tempDir.WriteAsync(Path.Join("app", "file.js"), "AppStuff"); + await tempDir.WriteAsync(Path.Join("locales", "file.json"), "LocalesStuff"); + await tempDir.WriteAsync(Path.Join("fonts", "file.ttf"), "FontsStuff"); + await tempDir.WriteAsync(Path.Join("connectors", "file.js"), "ConnectorsStuff"); + await tempDir.WriteAsync(Path.Join("scripts", "file.js"), "ScriptsStuff"); + await tempDir.WriteAsync(Path.Join("images", "file.avif"), "ImagesStuff"); + await tempDir.WriteAsync(Path.Join("test", "file.json"), "{}"); + + using var server = new Server + { + ContentRoot = tempDir.Info.FullName, + WebRoot = ".", + ServeUnknown = false, + WebVault = true, + AppIdLocation = Path.Join(tempDir.Info.FullName, "test", "file.json"), + }; + + var client = server.CreateClient(); + + // Going to root should return the default file + var response = await client.GetAsync("", TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("", await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + // No caching on the default document + Assert.Null(response.Headers.CacheControl?.MaxAge); + + await ExpectMaxAgeAsync("app/file.js", TimeSpan.FromDays(14)); + await ExpectMaxAgeAsync("locales/file.json", TimeSpan.FromDays(14)); + await ExpectMaxAgeAsync("fonts/file.ttf", TimeSpan.FromDays(14)); + await ExpectMaxAgeAsync("connectors/file.js", TimeSpan.FromDays(14)); + await ExpectMaxAgeAsync("scripts/file.js", TimeSpan.FromDays(14)); + await ExpectMaxAgeAsync("images/file.avif", TimeSpan.FromDays(7)); + + response = await client.GetAsync("app-id.json", TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("application/json", response.Content.Headers.ContentType?.MediaType); + + async Task ExpectMaxAgeAsync(string path, TimeSpan maxAge) + { + response = await client.GetAsync(path); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.NotNull(response.Headers.CacheControl); + Assert.Equal(maxAge, response.Headers.CacheControl.MaxAge); + } + } + + private class TempDir([CallerMemberName] string test = null!) : IDisposable + { + public DirectoryInfo Info { get; } = Directory.CreateTempSubdirectory(test); + + public void Dispose() + { + Info.Delete(recursive: true); + } + + public async Task WriteAsync(string fileName, string content) + { + var fullPath = Path.Join(Info.FullName, fileName); + var directory = Path.GetDirectoryName(fullPath); + if (directory != null) + { + Directory.CreateDirectory(directory); + } + + await File.WriteAllTextAsync(fullPath, content, TestContext.Current.CancellationToken); + } + } +} diff --git a/util/Server/Program.cs b/util/Server/Program.cs index a2d7e5f687..3d563830ab 100644 --- a/util/Server/Program.cs +++ b/util/Server/Program.cs @@ -6,6 +6,13 @@ namespace Bit.Server; public class Program { public static void Main(string[] args) + { + var builder = CreateWebHostBuilder(args); + var host = builder.Build(); + host.Run(); + } + + public static IWebHostBuilder CreateWebHostBuilder(string[] args) { var config = new ConfigurationBuilder() .AddCommandLine(args) @@ -37,7 +44,6 @@ public class Program builder.UseWebRoot(webRoot); } - var host = builder.Build(); - host.Run(); + return builder; } } From 2e0e103076f6ec44f267bbe9b2b117f0df2ef79e Mon Sep 17 00:00:00 2001 From: cyprain-okeke <108260115+cyprain-okeke@users.noreply.github.com> Date: Thu, 15 Jan 2026 09:55:43 +0100 Subject: [PATCH 2/7] Fix the currency culture invariant (#6812) --- src/Core/Billing/Extensions/InvoiceExtensions.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Core/Billing/Extensions/InvoiceExtensions.cs b/src/Core/Billing/Extensions/InvoiceExtensions.cs index d62959c09a..774b6b93b2 100644 --- a/src/Core/Billing/Extensions/InvoiceExtensions.cs +++ b/src/Core/Billing/Extensions/InvoiceExtensions.cs @@ -1,4 +1,5 @@ -using System.Text.RegularExpressions; +using System.Globalization; +using System.Text.RegularExpressions; using Stripe; namespace Bit.Core.Billing.Extensions; @@ -51,7 +52,7 @@ public static class InvoiceExtensions if (string.IsNullOrEmpty(priceInfo) && line.Quantity > 0) { var pricePerItem = (line.Amount / 100m) / line.Quantity; - priceInfo = $"(at ${pricePerItem:F2} / month)"; + priceInfo = string.Format(CultureInfo.InvariantCulture, "(at ${0:F2} / month)", pricePerItem); } var taxDescription = $"{line.Quantity} × Tax {priceInfo}"; @@ -70,7 +71,7 @@ public static class InvoiceExtensions if (tax > 0) { var taxAmount = tax / 100m; - items.Add($"1 × Tax (at ${taxAmount:F2} / month)"); + items.Add(string.Format(CultureInfo.InvariantCulture, "1 × Tax (at ${0:F2} / month)", taxAmount)); } return items; From c7e364a39c072e3d6c9409b7fcd898e5dad130d3 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Thu, 15 Jan 2026 06:00:31 -0800 Subject: [PATCH 3/7] chore(flag): add pm-27086-update-authentication-apis-for-input-password feature flag --- src/Core/Constants.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index 7cf00621c1..6f42778b6b 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -164,6 +164,7 @@ public static class FeatureFlagKeys public const string MarketingInitiatedPremiumFlow = "pm-26140-marketing-initiated-premium-flow"; public const string RedirectOnSsoRequired = "pm-1632-redirect-on-sso-required"; public const string PrefetchPasswordPrelogin = "pm-23801-prefetch-password-prelogin"; + public const string PM27086_UpdateAuthenticationApisForInputPassword = "pm-27086-update-authentication-apis-for-input-password"; /* Autofill Team */ public const string SSHAgent = "ssh-agent"; From 8cb80305341098673771e863b8b5266ad19bdce0 Mon Sep 17 00:00:00 2001 From: Patrick-Pimentel-Bitwarden Date: Thu, 15 Jan 2026 15:55:27 -0500 Subject: [PATCH 4/7] feat(register): [PM-27084] Account Register Uses New Data Types (#6715) * feat(register): [PM-27084] Account Register Uses New Data Types - Implementation * test(register): [PM-27084] Account Register Uses New Data Types - Added tests --- .../Request/Accounts/PasswordRequestModel.cs | 6 +- .../SetInitialPasswordRequestModel.cs | 1 - .../Accounts/RegisterFinishRequestModel.cs | 201 ++++++- src/Core/Entities/User.cs | 6 +- .../Models/Api/Request}/KdfRequestModel.cs | 11 +- ...rPasswordAuthenticationDataRequestModel.cs | 6 +- .../MasterPasswordUnlockDataRequestModel.cs | 6 +- .../Data/MasterPasswordAuthenticationData.cs | 5 + ...sterPasswordUnlockAndAuthenticationData.cs | 3 +- .../Models/Data/MasterPasswordUnlockData.cs | 5 + src/Core/Utilities/KdfSettingsValidator.cs | 1 + .../Controllers/AccountsController.cs | 61 ++- .../Controllers/AccountsControllerTest.cs | 4 +- .../Controllers/AccountsControllerTests.cs | 1 - .../SetInitialPasswordRequestModelTests.cs | 1 - .../RegisterFinishRequestModelFixtures.cs | 4 +- .../RegisterFinishRequestModelTests.cs | 183 +++++++ .../Controllers/AccountsControllerTests.cs | 502 +++++++++++++++++- .../Factories/IdentityApplicationFactory.cs | 101 +++- 19 files changed, 1045 insertions(+), 63 deletions(-) rename src/{Api/KeyManagement/Models/Requests => Core/KeyManagement/Models/Api/Request}/KdfRequestModel.cs (59%) rename src/{Api/KeyManagement/Models/Requests => Core/KeyManagement/Models/Api/Request}/MasterPasswordAuthenticationDataRequestModel.cs (71%) rename src/{Api/KeyManagement/Models/Requests => Core/KeyManagement/Models/Api/Request}/MasterPasswordUnlockDataRequestModel.cs (71%) diff --git a/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs b/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs index 8fa51e9f34..ab8c727852 100644 --- a/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs +++ b/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs @@ -1,7 +1,5 @@ -#nullable enable - -using System.ComponentModel.DataAnnotations; -using Bit.Api.KeyManagement.Models.Requests; +using System.ComponentModel.DataAnnotations; +using Bit.Core.KeyManagement.Models.Api.Request; namespace Bit.Api.Auth.Models.Request.Accounts; diff --git a/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs b/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs index 55ffdca94b..37a7901fee 100644 --- a/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs +++ b/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs @@ -1,5 +1,4 @@ using System.ComponentModel.DataAnnotations; -using Bit.Api.KeyManagement.Models.Requests; using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Auth.Models.Data; using Bit.Core.Entities; diff --git a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs index 0ac7dbbcb4..cb66540a6b 100644 --- a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs +++ b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs @@ -1,6 +1,6 @@ -#nullable enable -using Bit.Core.Entities; +using Bit.Core.Entities; using Bit.Core.Enums; +using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.Utilities; namespace Bit.Core.Auth.Models.Api.Request.Accounts; @@ -21,19 +21,32 @@ public class RegisterFinishRequestModel : IValidatableObject public required string Email { get; set; } public string? EmailVerificationToken { get; set; } + public MasterPasswordAuthenticationDataRequestModel? MasterPasswordAuthentication { get; set; } + public MasterPasswordUnlockDataRequestModel? MasterPasswordUnlock { get; set; } + + // PM-28143 - Remove property below (made optional during migration to MasterPasswordUnlockData) [StringLength(1000)] - public required string MasterPasswordHash { get; set; } + // Made optional but there will still be a thrown error if it does not exist either here or + // in the MasterPasswordAuthenticationData. + public string? MasterPasswordHash { get; set; } [StringLength(50)] public string? MasterPasswordHint { get; set; } - public required string UserSymmetricKey { get; set; } + // PM-28143 - Remove property below (made optional during migration to MasterPasswordUnlockData) + // Made optional but there will still be a thrown error if it does not exist either here or + // in the MasterPasswordAuthenticationData. + public string? UserSymmetricKey { get; set; } public required KeysRequestModel UserAsymmetricKeys { get; set; } - public required KdfType Kdf { get; set; } - public required int KdfIterations { get; set; } + // PM-28143 - Remove line below (made optional during migration to MasterPasswordUnlockData) + public KdfType? Kdf { get; set; } + // PM-28143 - Remove line below (made optional during migration to MasterPasswordUnlockData) + public int? KdfIterations { get; set; } + // PM-28143 - Remove line below public int? KdfMemory { get; set; } + // PM-28143 - Remove line below public int? KdfParallelism { get; set; } public Guid? OrganizationUserId { get; set; } @@ -54,11 +67,14 @@ public class RegisterFinishRequestModel : IValidatableObject { Email = Email, MasterPasswordHint = MasterPasswordHint, - Kdf = Kdf, - KdfIterations = KdfIterations, - KdfMemory = KdfMemory, - KdfParallelism = KdfParallelism, - Key = UserSymmetricKey, + Kdf = (KdfType)(MasterPasswordUnlock?.Kdf.KdfType ?? Kdf)!, + KdfIterations = (int)(MasterPasswordUnlock?.Kdf.Iterations ?? KdfIterations)!, + // 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 = MasterPasswordUnlock?.Salt ?? Email.ToLower().Trim(), + Key = MasterPasswordUnlock?.MasterKeyWrappedUserKey ?? UserSymmetricKey }; UserAsymmetricKeys.ToUser(user); @@ -72,7 +88,9 @@ public class RegisterFinishRequestModel : IValidatableObject { return RegisterFinishTokenType.EmailVerification; } - if (!string.IsNullOrEmpty(OrgInviteToken) && OrganizationUserId.HasValue) + if (!string.IsNullOrEmpty(OrgInviteToken) + && OrganizationUserId.HasValue + && OrganizationUserId.Value != Guid.Empty) { return RegisterFinishTokenType.OrganizationInvite; } @@ -80,11 +98,15 @@ public class RegisterFinishRequestModel : IValidatableObject { return RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan; } - if (!string.IsNullOrWhiteSpace(AcceptEmergencyAccessInviteToken) && AcceptEmergencyAccessId.HasValue) + if (!string.IsNullOrWhiteSpace(AcceptEmergencyAccessInviteToken) + && AcceptEmergencyAccessId.HasValue + && AcceptEmergencyAccessId.Value != Guid.Empty) { return RegisterFinishTokenType.EmergencyAccessInvite; } - if (!string.IsNullOrWhiteSpace(ProviderInviteToken) && ProviderUserId.HasValue) + if (!string.IsNullOrWhiteSpace(ProviderInviteToken) + && ProviderUserId.HasValue + && ProviderUserId.Value != Guid.Empty) { return RegisterFinishTokenType.ProviderInvite; } @@ -92,9 +114,156 @@ public class RegisterFinishRequestModel : IValidatableObject throw new InvalidOperationException("Invalid token type."); } - public IEnumerable Validate(ValidationContext validationContext) { - return KdfSettingsValidator.Validate(Kdf, KdfIterations, KdfMemory, KdfParallelism); + // 1. Authentication data containing hash and hash at root level check + if (MasterPasswordAuthentication != null && MasterPasswordHash != null) + { + if (MasterPasswordAuthentication.MasterPasswordAuthenticationHash != MasterPasswordHash) + { + yield return new ValidationResult( + $"{nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash)} and root level {nameof(MasterPasswordHash)} provided and are not equal. Only provide one.", + [nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash), nameof(MasterPasswordHash)]); + } + } // 1.5 if there is no master password hash that is unacceptable even though they are both optional in the model + else if (MasterPasswordAuthentication == null && MasterPasswordHash == null) + { + yield return new ValidationResult( + $"{nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash)} and {nameof(MasterPasswordHash)} not found on request, one needs to be defined.", + [nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash), nameof(MasterPasswordHash)]); + } + + // 2. Validate kdf settings. + if (MasterPasswordUnlock != null) + { + foreach (var validationResult in KdfSettingsValidator.Validate(MasterPasswordUnlock.ToData().Kdf)) + { + yield return validationResult; + } + } + + if (MasterPasswordAuthentication != null) + { + foreach (var validationResult in KdfSettingsValidator.Validate(MasterPasswordAuthentication.ToData().Kdf)) + { + yield return validationResult; + } + } + + // 3. Validate root kdf values if kdf values are not in the unlock and authentication. + if (MasterPasswordUnlock == null && MasterPasswordAuthentication == null) + { + var hasMissingRequiredKdfInputs = false; + if (Kdf == null) + { + yield return new ValidationResult($"{nameof(Kdf)} not found on RequestModel", [nameof(Kdf)]); + hasMissingRequiredKdfInputs = true; + } + if (KdfIterations == null) + { + yield return new ValidationResult($"{nameof(KdfIterations)} not found on RequestModel", [nameof(KdfIterations)]); + hasMissingRequiredKdfInputs = true; + } + + if (!hasMissingRequiredKdfInputs) + { + foreach (var validationResult in KdfSettingsValidator.Validate( + Kdf!.Value, + KdfIterations!.Value, + KdfMemory, + KdfParallelism)) + { + yield return validationResult; + } + } + } + else if (MasterPasswordUnlock == null && MasterPasswordAuthentication != null) + { + // Authentication provided but Unlock missing + yield return new ValidationResult($"{nameof(MasterPasswordUnlock)} not found on RequestModel", [nameof(MasterPasswordUnlock)]); + } + else if (MasterPasswordUnlock != null && MasterPasswordAuthentication == null) + { + // Unlock provided but Authentication missing + yield return new ValidationResult($"{nameof(MasterPasswordAuthentication)} not found on RequestModel", [nameof(MasterPasswordAuthentication)]); + } + + // 3. Lastly, validate access token type and presence. Must be done last because of yield break. + RegisterFinishTokenType tokenType; + var tokenTypeResolved = true; + try + { + tokenType = GetTokenType(); + } + catch (InvalidOperationException) + { + tokenTypeResolved = false; + tokenType = default; + } + + if (!tokenTypeResolved) + { + yield return new ValidationResult("No valid registration token provided"); + yield break; + } + + switch (tokenType) + { + case RegisterFinishTokenType.EmailVerification: + if (string.IsNullOrEmpty(EmailVerificationToken)) + { + yield return new ValidationResult( + $"{nameof(EmailVerificationToken)} absent when processing register/finish.", + [nameof(EmailVerificationToken)]); + } + break; + case RegisterFinishTokenType.OrganizationInvite: + if (string.IsNullOrEmpty(OrgInviteToken)) + { + yield return new ValidationResult( + $"{nameof(OrgInviteToken)} absent when processing register/finish.", + [nameof(OrgInviteToken)]); + } + break; + case RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan: + if (string.IsNullOrEmpty(OrgSponsoredFreeFamilyPlanToken)) + { + yield return new ValidationResult( + $"{nameof(OrgSponsoredFreeFamilyPlanToken)} absent when processing register/finish.", + [nameof(OrgSponsoredFreeFamilyPlanToken)]); + } + break; + case RegisterFinishTokenType.EmergencyAccessInvite: + if (string.IsNullOrEmpty(AcceptEmergencyAccessInviteToken)) + { + yield return new ValidationResult( + $"{nameof(AcceptEmergencyAccessInviteToken)} absent when processing register/finish.", + [nameof(AcceptEmergencyAccessInviteToken)]); + } + if (!AcceptEmergencyAccessId.HasValue || AcceptEmergencyAccessId.Value == Guid.Empty) + { + yield return new ValidationResult( + $"{nameof(AcceptEmergencyAccessId)} absent when processing register/finish.", + [nameof(AcceptEmergencyAccessId)]); + } + break; + case RegisterFinishTokenType.ProviderInvite: + if (string.IsNullOrEmpty(ProviderInviteToken)) + { + yield return new ValidationResult( + $"{nameof(ProviderInviteToken)} absent when processing register/finish.", + [nameof(ProviderInviteToken)]); + } + if (!ProviderUserId.HasValue || ProviderUserId.Value == Guid.Empty) + { + yield return new ValidationResult( + $"{nameof(ProviderUserId)} absent when processing register/finish.", + [nameof(ProviderUserId)]); + } + break; + default: + yield return new ValidationResult("Invalid registration finish request"); + break; + } } } diff --git a/src/Core/Entities/User.cs b/src/Core/Entities/User.cs index 669e32bcbe..422dc37c6e 100644 --- a/src/Core/Entities/User.cs +++ b/src/Core/Entities/User.cs @@ -7,8 +7,6 @@ using Bit.Core.KeyManagement.Models.Data; using Bit.Core.Utilities; using Microsoft.AspNetCore.Identity; -#nullable enable - namespace Bit.Core.Entities; public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFactorProvidersUser @@ -51,7 +49,7 @@ public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFac public string? Key { get; set; } /// /// The raw public key, without a signature from the user's signature key. - /// + /// public string? PublicKey { get; set; } /// /// User key wrapped private key. @@ -107,6 +105,8 @@ public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFac public DateTime? LastKeyRotationDate { get; set; } public DateTime? LastEmailChangeDate { get; set; } public bool VerifyDevices { get; set; } = true; + // PM-28827 Uncomment below line. + // public string? MasterPasswordSalt { get; set; } public string GetMasterPasswordSalt() { diff --git a/src/Api/KeyManagement/Models/Requests/KdfRequestModel.cs b/src/Core/KeyManagement/Models/Api/Request/KdfRequestModel.cs similarity index 59% rename from src/Api/KeyManagement/Models/Requests/KdfRequestModel.cs rename to src/Core/KeyManagement/Models/Api/Request/KdfRequestModel.cs index 904304a633..edcd7f760f 100644 --- a/src/Api/KeyManagement/Models/Requests/KdfRequestModel.cs +++ b/src/Core/KeyManagement/Models/Api/Request/KdfRequestModel.cs @@ -1,10 +1,11 @@ using System.ComponentModel.DataAnnotations; using Bit.Core.Enums; using Bit.Core.KeyManagement.Models.Data; +using Bit.Core.Utilities; -namespace Bit.Api.KeyManagement.Models.Requests; +namespace Bit.Core.KeyManagement.Models.Api.Request; -public class KdfRequestModel +public class KdfRequestModel : IValidatableObject { [Required] public required KdfType KdfType { get; init; } @@ -23,4 +24,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/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs b/src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs similarity index 71% rename from src/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs rename to src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs index 4f70a1135f..04c22cc3a6 100644 --- a/src/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs +++ b/src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs @@ -1,8 +1,12 @@ using System.ComponentModel.DataAnnotations; using Bit.Core.KeyManagement.Models.Data; -namespace Bit.Api.KeyManagement.Models.Requests; +namespace Bit.Core.KeyManagement.Models.Api.Request; +/// +/// Use this datatype when interfacing with requests to create a separation of concern. +/// See to use for commands, queries, services. +/// public class MasterPasswordAuthenticationDataRequestModel { public required KdfRequestModel Kdf { get; init; } diff --git a/src/Api/KeyManagement/Models/Requests/MasterPasswordUnlockDataRequestModel.cs b/src/Core/KeyManagement/Models/Api/Request/MasterPasswordUnlockDataRequestModel.cs similarity index 71% rename from src/Api/KeyManagement/Models/Requests/MasterPasswordUnlockDataRequestModel.cs rename to src/Core/KeyManagement/Models/Api/Request/MasterPasswordUnlockDataRequestModel.cs index e1d7863cae..8d7df86374 100644 --- a/src/Api/KeyManagement/Models/Requests/MasterPasswordUnlockDataRequestModel.cs +++ b/src/Core/KeyManagement/Models/Api/Request/MasterPasswordUnlockDataRequestModel.cs @@ -2,8 +2,12 @@ using Bit.Core.KeyManagement.Models.Data; using Bit.Core.Utilities; -namespace Bit.Api.KeyManagement.Models.Requests; +namespace Bit.Core.KeyManagement.Models.Api.Request; +/// +/// Use this datatype when interfacing with requests to create a separation of concern. +/// See to use for commands, queries, services. +/// public class MasterPasswordUnlockDataRequestModel { public required KdfRequestModel Kdf { get; init; } diff --git a/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs b/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs index 1bc7006cef..6e53dfa744 100644 --- a/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs +++ b/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs @@ -1,8 +1,13 @@ using Bit.Core.Entities; using Bit.Core.Exceptions; +using Bit.Core.KeyManagement.Models.Api.Request; namespace Bit.Core.KeyManagement.Models.Data; +/// +/// Use this datatype when interfacing with commands, queries, services to create a separation of concern. +/// See to use for requests. +/// public class MasterPasswordAuthenticationData { public required KdfSettings Kdf { get; init; } diff --git a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs index ad3a0b692b..b79ce8bce1 100644 --- a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs +++ b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs @@ -1,5 +1,4 @@ -#nullable enable -using Bit.Core.Entities; +using Bit.Core.Entities; using Bit.Core.Enums; namespace Bit.Core.KeyManagement.Models.Data; diff --git a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs index cb18ed2a78..f8139cba99 100644 --- a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs +++ b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs @@ -1,8 +1,13 @@ using Bit.Core.Entities; using Bit.Core.Exceptions; +using Bit.Core.KeyManagement.Models.Api.Request; namespace Bit.Core.KeyManagement.Models.Data; +/// +/// Use this datatype when interfacing with commands, queries, services to create a separation of concern. +/// See to use for requests. +/// public class MasterPasswordUnlockData { public required KdfSettings Kdf { get; init; } diff --git a/src/Core/Utilities/KdfSettingsValidator.cs b/src/Core/Utilities/KdfSettingsValidator.cs index f89e8ddb66..e5690ad469 100644 --- a/src/Core/Utilities/KdfSettingsValidator.cs +++ b/src/Core/Utilities/KdfSettingsValidator.cs @@ -6,6 +6,7 @@ namespace Bit.Core.Utilities; public static class KdfSettingsValidator { + // PM-28143 - Remove below when fixing ticket public static IEnumerable Validate(KdfType kdfType, int kdfIterations, int? kdfMemory, int? kdfParallelism) { switch (kdfType) diff --git a/src/Identity/Controllers/AccountsController.cs b/src/Identity/Controllers/AccountsController.cs index b7d4342c1b..e9807fb1fc 100644 --- a/src/Identity/Controllers/AccountsController.cs +++ b/src/Identity/Controllers/AccountsController.cs @@ -1,8 +1,4 @@ -// FIXME: Update this file to be null safe and then delete the line below -#nullable disable - -using System.Diagnostics; -using System.Text; +using System.Text; using Bit.Core; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models.Api.Request.Accounts; @@ -42,7 +38,7 @@ public class AccountsController : Controller private readonly IFeatureService _featureService; private readonly IDataProtectorTokenFactory _registrationEmailVerificationTokenDataFactory; - private readonly byte[] _defaultKdfHmacKey = null; + private readonly byte[]? _defaultKdfHmacKey = null; private static readonly List _defaultKdfResults = [ // The first result (index 0) should always return the "normal" default. @@ -145,40 +141,55 @@ public class AccountsController : Controller [HttpPost("register/finish")] public async Task PostRegisterFinish([FromBody] RegisterFinishRequestModel model) { - var user = model.ToUser(); + User user = model.ToUser(); // Users will either have an emailed token or an email verification token - not both. - IdentityResult identityResult = null; + IdentityResult? identityResult = null; + + // PM-28143 - Just use the MasterPasswordAuthenticationData.MasterPasswordAuthenticationHash + string masterPasswordAuthenticationHash = model.MasterPasswordAuthentication?.MasterPasswordAuthenticationHash + ?? model.MasterPasswordHash!; switch (model.GetTokenType()) { case RegisterFinishTokenType.EmailVerification: - identityResult = - await _registerUserCommand.RegisterUserViaEmailVerificationToken(user, model.MasterPasswordHash, - model.EmailVerificationToken); - + identityResult = await _registerUserCommand.RegisterUserViaEmailVerificationToken( + user, + masterPasswordAuthenticationHash, + model.EmailVerificationToken!); return ProcessRegistrationResult(identityResult, user); + case RegisterFinishTokenType.OrganizationInvite: - identityResult = await _registerUserCommand.RegisterUserViaOrganizationInviteToken(user, model.MasterPasswordHash, - model.OrgInviteToken, model.OrganizationUserId); - + identityResult = await _registerUserCommand.RegisterUserViaOrganizationInviteToken( + user, + masterPasswordAuthenticationHash, + model.OrgInviteToken!, + model.OrganizationUserId); return ProcessRegistrationResult(identityResult, user); + case RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan: - identityResult = await _registerUserCommand.RegisterUserViaOrganizationSponsoredFreeFamilyPlanInviteToken(user, model.MasterPasswordHash, model.OrgSponsoredFreeFamilyPlanToken); - + identityResult = await _registerUserCommand.RegisterUserViaOrganizationSponsoredFreeFamilyPlanInviteToken( + user, + masterPasswordAuthenticationHash, + model.OrgSponsoredFreeFamilyPlanToken!); return ProcessRegistrationResult(identityResult, user); + case RegisterFinishTokenType.EmergencyAccessInvite: - Debug.Assert(model.AcceptEmergencyAccessId.HasValue); - identityResult = await _registerUserCommand.RegisterUserViaAcceptEmergencyAccessInviteToken(user, model.MasterPasswordHash, - model.AcceptEmergencyAccessInviteToken, model.AcceptEmergencyAccessId.Value); - + identityResult = await _registerUserCommand.RegisterUserViaAcceptEmergencyAccessInviteToken( + user, + masterPasswordAuthenticationHash, + model.AcceptEmergencyAccessInviteToken!, + (Guid)model.AcceptEmergencyAccessId!); return ProcessRegistrationResult(identityResult, user); + case RegisterFinishTokenType.ProviderInvite: - Debug.Assert(model.ProviderUserId.HasValue); - identityResult = await _registerUserCommand.RegisterUserViaProviderInviteToken(user, model.MasterPasswordHash, - model.ProviderInviteToken, model.ProviderUserId.Value); - + identityResult = await _registerUserCommand.RegisterUserViaProviderInviteToken( + user, + masterPasswordAuthenticationHash, + model.ProviderInviteToken!, + (Guid)model.ProviderUserId!); return ProcessRegistrationResult(identityResult, user); + default: throw new BadRequestException("Invalid registration finish request"); } diff --git a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs index d055418f3a..9860775e31 100644 --- a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs +++ b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs @@ -3,7 +3,6 @@ using System.Text.Json; using Bit.Api.Auth.Models.Request.Accounts; using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; -using Bit.Api.KeyManagement.Models.Requests; using Bit.Api.Models.Response; using Bit.Core; using Bit.Core.Auth.Entities; @@ -12,6 +11,7 @@ using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.Repositories; using Bit.Core.Entities; using Bit.Core.Enums; +using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.KeyManagement.Repositories; using Bit.Core.Models.Data; using Bit.Core.Platform.Push; @@ -378,7 +378,7 @@ public class AccountsControllerTest : IClassFixture, IAsy Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); var content = await response.Content.ReadAsStringAsync(); - Assert.Contains("KDF settings are invalid", content); + Assert.Contains("The model state is invalid", content); } [Fact] diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index 6cddd341d5..665d1e52c1 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -1,7 +1,6 @@ using System.Security.Claims; using Bit.Api.Auth.Controllers; using Bit.Api.Auth.Models.Request.Accounts; -using Bit.Api.KeyManagement.Models.Requests; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; using Bit.Core.Auth.Models.Api.Request.Accounts; diff --git a/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs b/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs index ce8ba1811e..97e69dacbc 100644 --- a/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs +++ b/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs @@ -1,6 +1,5 @@ using System.ComponentModel.DataAnnotations; using Bit.Api.Auth.Models.Request.Accounts; -using Bit.Api.KeyManagement.Models.Requests; using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Entities; using Bit.Core.Enums; diff --git a/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs b/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs index a751a16f31..22fca7ab59 100644 --- a/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs +++ b/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs @@ -29,7 +29,9 @@ internal class RegisterFinishRequestModelCustomization : ICustomization .With(o => o.OrgInviteToken, OrgInviteToken) .With(o => o.OrgSponsoredFreeFamilyPlanToken, OrgSponsoredFreeFamilyPlanToken) .With(o => o.AcceptEmergencyAccessInviteToken, AcceptEmergencyAccessInviteToken) - .With(o => o.ProviderInviteToken, ProviderInviteToken)); + .With(o => o.ProviderInviteToken, ProviderInviteToken) + .Without(o => o.MasterPasswordAuthentication) + .Without(o => o.MasterPasswordUnlock)); } } diff --git a/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs b/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs index 588ca878fc..3c099ce962 100644 --- a/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs +++ b/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs @@ -1,5 +1,6 @@ using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Enums; +using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Test.Common.AutoFixture.Attributes; using Xunit; @@ -7,6 +8,17 @@ namespace Bit.Core.Test.Auth.Models.Api.Request.Accounts; public class RegisterFinishRequestModelTests { + private static List Validate(RegisterFinishRequestModel model) + { + var results = new List(); + System.ComponentModel.DataAnnotations.Validator.TryValidateObject( + model, + new System.ComponentModel.DataAnnotations.ValidationContext(model), + results, + true); + return results; + } + [Theory] [BitAutoData] public void GetTokenType_Returns_EmailVerification(string email, string masterPasswordHash, @@ -170,4 +182,175 @@ public class RegisterFinishRequestModelTests Assert.Equal(userAsymmetricKeys.PublicKey, result.PublicKey); Assert.Equal(userAsymmetricKeys.EncryptedPrivateKey, result.PrivateKey); } + + [Fact] + public void Validate_WhenBothAuthAndRootHashProvidedButNotEqual_ReturnsMismatchError() + { + var model = new RegisterFinishRequestModel + { + Email = "user@example.com", + MasterPasswordHash = "root-hash", + UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, + // Provide both unlock and authentication with valid KDF so only the mismatch rule fires + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterKeyWrappedUserKey = "wrapped", + Salt = "salt" + }, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterPasswordAuthenticationHash = "auth-hash", // different than root + Salt = "salt" + }, + // Provide any valid token so we don't fail token validation + EmailVerificationToken = "token" + }; + + var results = Validate(model); + + Assert.Contains(results, r => + r.ErrorMessage == $"{nameof(MasterPasswordAuthenticationDataRequestModel.MasterPasswordAuthenticationHash)} and root level {nameof(RegisterFinishRequestModel.MasterPasswordHash)} provided and are not equal. Only provide one."); + } + + [Fact] + public void Validate_WhenAuthProvidedButUnlockMissing_ReturnsUnlockMissingError() + { + var model = new RegisterFinishRequestModel + { + Email = "user@example.com", + UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterPasswordAuthenticationHash = "auth-hash", + Salt = "salt" + }, + EmailVerificationToken = "token" + }; + + var results = Validate(model); + + Assert.Contains(results, r => r.ErrorMessage == "MasterPasswordUnlock not found on RequestModel"); + } + + [Fact] + public void Validate_WhenUnlockProvidedButAuthMissing_ReturnsAuthMissingError() + { + var model = new RegisterFinishRequestModel + { + Email = "user@example.com", + UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterKeyWrappedUserKey = "wrapped", + Salt = "salt" + }, + EmailVerificationToken = "token" + }; + + var results = Validate(model); + + Assert.Contains(results, r => r.ErrorMessage == "MasterPasswordAuthentication not found on RequestModel"); + } + + [Fact] + public void Validate_WhenNeitherAuthNorUnlock_AndRootKdfMissing_ReturnsBothRootKdfErrors() + { + var model = new RegisterFinishRequestModel + { + Email = "user@example.com", + UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, + // No MasterPasswordUnlock, no MasterPasswordAuthentication + // No root Kdf and KdfIterations to trigger both errors + EmailVerificationToken = "token" + }; + + var results = Validate(model); + + Assert.Contains(results, r => r.ErrorMessage == $"{nameof(RegisterFinishRequestModel.Kdf)} not found on RequestModel"); + Assert.Contains(results, r => r.ErrorMessage == $"{nameof(RegisterFinishRequestModel.KdfIterations)} not found on RequestModel"); + } + + [Fact] + public void Validate_WhenAuthAndRootHashBothMissing_ReturnsMissingHashErrorOnly() + { + var model = new RegisterFinishRequestModel + { + Email = "user@example.com", + UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, + // Both MasterPasswordAuthentication and MasterPasswordHash are missing + MasterPasswordAuthentication = null, + MasterPasswordHash = null, + // Provide valid root KDF to avoid root KDF errors + Kdf = KdfType.PBKDF2_SHA256, + KdfIterations = AuthConstants.PBKDF2_ITERATIONS.Default, + EmailVerificationToken = "token" // avoid token error + }; + + var results = Validate(model); + + // Only the new missing hash error should be present + Assert.Single(results); + Assert.Equal($"{nameof(MasterPasswordAuthenticationDataRequestModel.MasterPasswordAuthenticationHash)} and {nameof(RegisterFinishRequestModel.MasterPasswordHash)} not found on request, one needs to be defined.", results[0].ErrorMessage); + Assert.Contains(nameof(MasterPasswordAuthenticationDataRequestModel.MasterPasswordAuthenticationHash), results[0].MemberNames); + Assert.Contains(nameof(RegisterFinishRequestModel.MasterPasswordHash), results[0].MemberNames); + } + + [Fact] + public void Validate_WhenAllFieldsValidWithSubModels_IsValid() + { + var model = new RegisterFinishRequestModel + { + Email = "user@example.com", + UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterKeyWrappedUserKey = "wrapped", + Salt = "salt" + }, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterPasswordAuthenticationHash = "auth-hash", + Salt = "salt" + }, + EmailVerificationToken = "token" + }; + + var results = Validate(model); + + Assert.Empty(results); + } + + [Fact] + public void Validate_WhenNoValidRegistrationTokenProvided_ReturnsTokenErrorOnly() + { + var model = new RegisterFinishRequestModel + { + Email = "user@example.com", + UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterKeyWrappedUserKey = "wrapped", + Salt = "salt" + }, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, + MasterPasswordAuthenticationHash = "auth-hash", + Salt = "salt" + } + // No token fields set + }; + + var results = Validate(model); + + Assert.Single(results); + Assert.Equal("No valid registration token provided", results[0].ErrorMessage); + } } diff --git a/test/Identity.Test/Controllers/AccountsControllerTests.cs b/test/Identity.Test/Controllers/AccountsControllerTests.cs index 42e033bdd7..86e461d155 100644 --- a/test/Identity.Test/Controllers/AccountsControllerTests.cs +++ b/test/Identity.Test/Controllers/AccountsControllerTests.cs @@ -1,4 +1,5 @@ -using System.Reflection; +using System.ComponentModel.DataAnnotations; +using System.Reflection; using System.Text; using Bit.Core; using Bit.Core.Auth.Models.Api.Request.Accounts; @@ -9,6 +10,7 @@ using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; +using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.Models.Data; using Bit.Core.Repositories; using Bit.Core.Services; @@ -590,6 +592,504 @@ public class AccountsControllerTests : IDisposable await Assert.ThrowsAsync(() => _sut.PostRegisterVerificationEmailClicked(requestModel)); } + // PM-28143 - When removing the old properties, update this test to just test the new properties working + // as expected. + [Theory, BitAutoData] + public async Task PostRegisterFinish_EmailVerification_BothDataForms_ProduceEquivalentOutcomes( + string email, + string emailVerificationToken, + string masterPasswordHash, + string masterKeyWrappedUserKey, + string publicKey, + string encryptedPrivateKey) + { + // Arrange: new-form model (MasterPasswordAuthenticationData + MasterPasswordUnlockData) + + var kdfData = new KdfRequestModel + { + KdfType = KdfType.Argon2id, + Iterations = AuthConstants.ARGON2_ITERATIONS.Default, + Memory = AuthConstants.ARGON2_MEMORY.Default, + Parallelism = AuthConstants.ARGON2_PARALLELISM.Default + }; + + var newModel = new RegisterFinishRequestModel + { + Email = email, + EmailVerificationToken = emailVerificationToken, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = kdfData, + MasterPasswordAuthenticationHash = masterPasswordHash, + Salt = email // salt choice is not validated here during registration + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = kdfData, + MasterKeyWrappedUserKey = masterKeyWrappedUserKey, + Salt = email + }, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + // Arrange: legacy-form model (MasterPasswordHash + legacy KDF + UserSymmetricKey) + var legacyModel = new RegisterFinishRequestModel + { + Email = email, + EmailVerificationToken = emailVerificationToken, + MasterPasswordHash = masterPasswordHash, + Kdf = KdfType.Argon2id, + KdfIterations = AuthConstants.ARGON2_ITERATIONS.Default, + KdfMemory = AuthConstants.ARGON2_MEMORY.Default, + KdfParallelism = AuthConstants.ARGON2_PARALLELISM.Default, + UserSymmetricKey = masterKeyWrappedUserKey, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + var newUser = newModel.ToUser(); + var legacyUser = legacyModel.ToUser(); + + _registerUserCommand + .RegisterUserViaEmailVerificationToken(Arg.Any(), masterPasswordHash, emailVerificationToken) + .Returns(Task.FromResult(IdentityResult.Success)); + + // Act: call with new form + var newResult = await _sut.PostRegisterFinish(newModel); + // Act: call with legacy form + var legacyResult = await _sut.PostRegisterFinish(legacyModel); + + // Assert: outcomes are identical in effect (success response) + Assert.NotNull(newResult); + Assert.NotNull(legacyResult); + + // Assert: effective users are equivalent + Assert.Equal(legacyUser.Email, newUser.Email); + Assert.Equal(legacyUser.MasterPasswordHint, newUser.MasterPasswordHint); + Assert.Equal(legacyUser.Kdf, newUser.Kdf); + Assert.Equal(legacyUser.KdfIterations, newUser.KdfIterations); + Assert.Equal(legacyUser.KdfMemory, newUser.KdfMemory); + Assert.Equal(legacyUser.KdfParallelism, newUser.KdfParallelism); + Assert.Equal(legacyUser.Key, newUser.Key); + Assert.Equal(legacyUser.PublicKey, newUser.PublicKey); + Assert.Equal(legacyUser.PrivateKey, newUser.PrivateKey); + + // Assert: hash forwarded identically from both inputs + await _registerUserCommand.Received(2).RegisterUserViaEmailVerificationToken( + Arg.Is(u => + u.Email == newUser.Email && + u.Kdf == newUser.Kdf && + u.KdfIterations == newUser.KdfIterations && + u.KdfMemory == newUser.KdfMemory && + u.KdfParallelism == newUser.KdfParallelism && + u.Key == newUser.Key), + masterPasswordHash, + emailVerificationToken); + + await _registerUserCommand.Received(2).RegisterUserViaEmailVerificationToken( + Arg.Is(u => + u.Email == legacyUser.Email && + u.Kdf == legacyUser.Kdf && + u.KdfIterations == legacyUser.KdfIterations && + u.KdfMemory == legacyUser.KdfMemory && + u.KdfParallelism == legacyUser.KdfParallelism && + u.Key == legacyUser.Key), + masterPasswordHash, + emailVerificationToken); + } + + // PM-28143 - When removing the old properties, update this test to just test the new properties working + // as expected. + [Theory, BitAutoData] + public async Task PostRegisterFinish_OrgInvite_BothDataForms_ProduceEquivalentOutcomes( + string email, + string orgInviteToken, + Guid organizationUserId, + string masterPasswordHash, + string masterKeyWrappedUserKey, + string publicKey, + string encryptedPrivateKey) + { + var kdfData = new KdfRequestModel + { + KdfType = KdfType.Argon2id, + Iterations = AuthConstants.ARGON2_ITERATIONS.Default, + Memory = AuthConstants.ARGON2_MEMORY.Default, + Parallelism = AuthConstants.ARGON2_PARALLELISM.Default + }; + + // Arrange: new-form model (MasterPasswordAuthenticationData + MasterPasswordUnlockData) + var newModel = new RegisterFinishRequestModel + { + Email = email, + OrgInviteToken = orgInviteToken, + OrganizationUserId = organizationUserId, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = kdfData, + MasterPasswordAuthenticationHash = masterPasswordHash, + Salt = email + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = kdfData, + MasterKeyWrappedUserKey = masterKeyWrappedUserKey, + Salt = email + }, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + // Arrange: legacy-form model (MasterPasswordHash + legacy KDF + UserSymmetricKey) + var legacyModel = new RegisterFinishRequestModel + { + Email = email, + OrgInviteToken = orgInviteToken, + OrganizationUserId = organizationUserId, + MasterPasswordHash = masterPasswordHash, + Kdf = kdfData.KdfType, + KdfIterations = kdfData.Iterations, + KdfMemory = kdfData.Memory, + KdfParallelism = kdfData.Parallelism, + UserSymmetricKey = masterKeyWrappedUserKey, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + var newUser = newModel.ToUser(); + var legacyUser = legacyModel.ToUser(); + + _registerUserCommand + .RegisterUserViaOrganizationInviteToken(Arg.Any(), masterPasswordHash, orgInviteToken, organizationUserId) + .Returns(Task.FromResult(IdentityResult.Success)); + + // Act + var newResult = await _sut.PostRegisterFinish(newModel); + var legacyResult = await _sut.PostRegisterFinish(legacyModel); + + // Assert success + Assert.NotNull(newResult); + Assert.NotNull(legacyResult); + + // Assert: effective users are equivalent + Assert.Equal(legacyUser.Email, newUser.Email); + Assert.Equal(legacyUser.MasterPasswordHint, newUser.MasterPasswordHint); + Assert.Equal(legacyUser.Kdf, newUser.Kdf); + Assert.Equal(legacyUser.KdfIterations, newUser.KdfIterations); + Assert.Equal(legacyUser.KdfMemory, newUser.KdfMemory); + Assert.Equal(legacyUser.KdfParallelism, newUser.KdfParallelism); + Assert.Equal(legacyUser.Key, newUser.Key); + Assert.Equal(legacyUser.PublicKey, newUser.PublicKey); + Assert.Equal(legacyUser.PrivateKey, newUser.PrivateKey); + + // Assert: hash forwarded identically from both inputs + await _registerUserCommand.Received(2).RegisterUserViaOrganizationInviteToken( + Arg.Is(u => + u.Email == newUser.Email && + u.Kdf == newUser.Kdf && + u.KdfIterations == newUser.KdfIterations && + u.KdfMemory == newUser.KdfMemory && + u.KdfParallelism == newUser.KdfParallelism && + u.Key == newUser.Key), + masterPasswordHash, + orgInviteToken, + organizationUserId); + + await _registerUserCommand.Received(2).RegisterUserViaOrganizationInviteToken( + Arg.Is(u => + u.Email == legacyUser.Email && + u.Kdf == legacyUser.Kdf && + u.KdfIterations == legacyUser.KdfIterations && + u.KdfMemory == legacyUser.KdfMemory && + u.KdfParallelism == legacyUser.KdfParallelism && + u.Key == legacyUser.Key), + masterPasswordHash, + orgInviteToken, + organizationUserId); + } + + [Theory, BitAutoData] + public async Task PostRegisterFinish_NewForm_UsesUnlockDataForKdfAndKey_WhenRootFieldsNull( + string email, + string emailVerificationToken, + string masterPasswordHash, + string masterKeyWrappedUserKey, + int iterations, + string publicKey, + string encryptedPrivateKey) + { + // Arrange: Provide only unlock-data KDF + key; leave root KDF fields null + var unlockKdf = new KdfRequestModel + { + KdfType = KdfType.PBKDF2_SHA256, + Iterations = iterations + }; + + var model = new RegisterFinishRequestModel + { + Email = email, + EmailVerificationToken = emailVerificationToken, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + // present but not used by ToUser for KDF/Key + Kdf = unlockKdf, + MasterPasswordAuthenticationHash = masterPasswordHash, + Salt = email + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = unlockKdf, + MasterKeyWrappedUserKey = masterKeyWrappedUserKey, + Salt = email + }, + // root KDF fields intentionally null + Kdf = null, + KdfIterations = null, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + _registerUserCommand + .RegisterUserViaEmailVerificationToken(Arg.Any(), masterPasswordHash, emailVerificationToken) + .Returns(Task.FromResult(IdentityResult.Success)); + + // Act + var _ = await _sut.PostRegisterFinish(model); + + // Assert: The user passed to command uses unlock-data values + await _registerUserCommand.Received(1).RegisterUserViaEmailVerificationToken( + Arg.Is(u => + u.Email == email && + u.Kdf == unlockKdf.KdfType && + u.KdfIterations == unlockKdf.Iterations && + u.Key == masterKeyWrappedUserKey), + masterPasswordHash, + emailVerificationToken); + } + + [Theory, BitAutoData] + public async Task PostRegisterFinish_LegacyForm_UsesRootFields_WhenUnlockDataNull( + string email, + string emailVerificationToken, + string masterPasswordHash, + string legacyKey, + string publicKey, + string encryptedPrivateKey) + { + // Arrange: Provide only legacy root KDF + key; no unlock-data provided + var model = new RegisterFinishRequestModel + { + Email = email, + EmailVerificationToken = emailVerificationToken, + MasterPasswordHash = masterPasswordHash, + Kdf = KdfType.PBKDF2_SHA256, + KdfIterations = AuthConstants.PBKDF2_ITERATIONS.Default, + UserSymmetricKey = legacyKey, + MasterPasswordUnlock = null, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + _registerUserCommand + .RegisterUserViaEmailVerificationToken(Arg.Any(), masterPasswordHash, emailVerificationToken) + .Returns(Task.FromResult(IdentityResult.Success)); + + // Act + var _ = await _sut.PostRegisterFinish(model); + + // Assert: The user passed to command uses root values + await _registerUserCommand.Received(1).RegisterUserViaEmailVerificationToken( + Arg.Is(u => + u.Email == email && + u.Kdf == KdfType.PBKDF2_SHA256 && + u.KdfIterations == AuthConstants.PBKDF2_ITERATIONS.Default && + u.Key == legacyKey), + masterPasswordHash, + emailVerificationToken); + } + + [Theory, BitAutoData] + public void RegisterFinishRequestModel_Validate_Throws_WhenUnlockAndAuthDataMismatch( + string email, + string authHash, + string masterKeyWrappedUserKey, + string publicKey, + string encryptedPrivateKey) + { + // Arrange: authentication and unlock have different KDF and/or salt + var authKdf = new KdfRequestModel + { + KdfType = KdfType.PBKDF2_SHA256, + Iterations = AuthConstants.PBKDF2_ITERATIONS.Default + }; + var unlockKdf = new KdfRequestModel + { + KdfType = KdfType.Argon2id, + Iterations = AuthConstants.ARGON2_ITERATIONS.Default, + Memory = AuthConstants.ARGON2_MEMORY.Default, + Parallelism = AuthConstants.ARGON2_PARALLELISM.Default + }; + + var model = new RegisterFinishRequestModel + { + Email = email, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = authKdf, + MasterPasswordAuthenticationHash = authHash, + Salt = email + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = unlockKdf, + MasterKeyWrappedUserKey = masterKeyWrappedUserKey, + Salt = email + }, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + // Provide a minimal valid token type to satisfy model-level token validation + model.EmailVerificationToken = "test-token"; + + var ctx = new ValidationContext(model); + + // Act + var results = model.Validate(ctx).ToList(); + + // Assert mismatched auth/unlock is allowed + Assert.Empty(results); + } + + [Theory, BitAutoData] + public void RegisterFinishRequestModel_Validate_Throws_WhenSaltMismatch( + string email, + string authHash, + string masterKeyWrappedUserKey, + string publicKey, + string encryptedPrivateKey) + { + var unlockKdf = new KdfRequestModel + { + KdfType = KdfType.Argon2id, + Iterations = AuthConstants.ARGON2_ITERATIONS.Default, + Memory = AuthConstants.ARGON2_MEMORY.Default, + Parallelism = AuthConstants.ARGON2_PARALLELISM.Default + }; + + var model = new RegisterFinishRequestModel + { + Email = email, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = unlockKdf, + MasterPasswordAuthenticationHash = authHash, + Salt = email + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = unlockKdf, + MasterKeyWrappedUserKey = masterKeyWrappedUserKey, + // Intentionally different salt to force mismatch + Salt = email + ".mismatch" + }, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + // Provide a minimal valid token type to satisfy model-level token validation + model.EmailVerificationToken = "test-token"; + + var ctx = new ValidationContext(model); + + // Act + var results = model.Validate(ctx).ToList(); + + // Assert mismatched salts between auth/unlock are allowed + Assert.Empty(results); + } + + [Theory, BitAutoData] + public void RegisterFinishRequestModel_Validate_Throws_WhenAuthHashAndRootHashMismatch( + string email, + string authHash, + string differentRootHash, + string masterKeyWrappedUserKey, + string publicKey, + string encryptedPrivateKey) + { + // Arrange: same KDF/salt, but authentication hash differs from legacy root hash + var kdf = new KdfRequestModel + { + KdfType = KdfType.PBKDF2_SHA256, + Iterations = AuthConstants.PBKDF2_ITERATIONS.Default + }; + + var model = new RegisterFinishRequestModel + { + Email = email, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = kdf, + MasterPasswordAuthenticationHash = authHash, + Salt = email + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = kdf, + MasterKeyWrappedUserKey = masterKeyWrappedUserKey, + Salt = email + }, + // Intentionally set the legacy field to a different value to trigger the throw + MasterPasswordHash = differentRootHash, + UserAsymmetricKeys = new KeysRequestModel + { + PublicKey = publicKey, + EncryptedPrivateKey = encryptedPrivateKey + } + }; + + // Provide a minimal valid token type to satisfy model-level token validation + model.EmailVerificationToken = "test-token"; + + var ctx = new ValidationContext(model); + + // Act + var results = model.Validate(ctx).ToList(); + + // Assert: validation result exists with expected message and member names + var mismatchResult = Assert.Single(results.Where(r => + r.ErrorMessage == + "MasterPasswordAuthenticationHash and root level MasterPasswordHash provided and are not equal. Only provide one.")); + Assert.Contains("MasterPasswordAuthenticationHash", mismatchResult.MemberNames); + Assert.Contains("MasterPasswordHash", mismatchResult.MemberNames); + } + private void SetDefaultKdfHmacKey(byte[]? newKey) { var fieldInfo = typeof(AccountsController).GetField("_defaultKdfHmacKey", BindingFlags.NonPublic | BindingFlags.Instance); diff --git a/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs b/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs index ba12d1e1f4..e190dda427 100644 --- a/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs +++ b/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs @@ -3,10 +3,13 @@ using System.Collections.Concurrent; using System.Net.Http.Json; +using System.Text; using System.Text.Json; +using Bit.Core; using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Entities; using Bit.Core.Enums; +using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.Services; using Bit.Identity; using Bit.Test.Common.Helpers; @@ -23,6 +26,7 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase public const string DefaultDeviceIdentifier = "92b9d953-b9b6-4eaf-9d3e-11d57144dfeb"; public const string DefaultUserEmail = "DefaultEmail@bitwarden.com"; public const string DefaultUserPasswordHash = "default_password_hash"; + private const string DefaultEncryptedString = "2.3Uk+WNBIoU5xzmVFNcoWzz==|1MsPIYuRfdOHfu/0uY6H2Q==|/98sp4wb6pHP1VTZ9JcNCYgQjEUMFPlqJgCwRk1YXKg="; /// /// A dictionary to store registration tokens for email verification. We cannot substitute the IMailService more than once, so @@ -195,6 +199,68 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase RegisterFinishRequestModel requestModel, bool marketingEmails = true) { + // Ensure required fields for registration finish are present. + // Prefer legacy-path defaults (root fields) to minimize changes to tests. + // PM-28143 - When MasterPasswordAuthenticationData is required, delete all handling of MasterPasswordHash. + requestModel.MasterPasswordHash ??= DefaultUserPasswordHash; + // PM-28143 - When KDF is sourced exclusively from MasterPasswordUnlockData, delete the root Kdf defaults below. + requestModel.Kdf ??= KdfType.PBKDF2_SHA256; + requestModel.KdfIterations ??= AuthConstants.PBKDF2_ITERATIONS.Default; + // Ensure a symmetric key is provided when no unlock data is present + // PM-28143 - When MasterPasswordUnlockData is required, delete the UserSymmetricKey fallback block below. + if (requestModel.MasterPasswordUnlock == null && string.IsNullOrWhiteSpace(requestModel.UserSymmetricKey)) + { + requestModel.UserSymmetricKey = "user_symmetric_key"; + } + + // Align unlock/auth data KDF with root KDF so login uses the provided master password hash. + // PM-28143 - After removing root Kdf fields, build KDF exclusively from MasterPasswordUnlockData.Kdf and delete this alignment section. + var effectiveKdfType = requestModel.Kdf ?? KdfType.PBKDF2_SHA256; + var effectiveIterations = requestModel.KdfIterations ?? AuthConstants.PBKDF2_ITERATIONS.Default; + int? effectiveMemory = null; + int? effectiveParallelism = null; + if (effectiveKdfType == KdfType.Argon2id) + { + effectiveIterations = AuthConstants.ARGON2_ITERATIONS.InsideRange(effectiveIterations) + ? effectiveIterations + : AuthConstants.ARGON2_ITERATIONS.Default; + effectiveMemory = AuthConstants.ARGON2_MEMORY.Default; + effectiveParallelism = AuthConstants.ARGON2_PARALLELISM.Default; + } + + var alignedKdf = new KdfRequestModel + { + KdfType = effectiveKdfType, + Iterations = effectiveIterations, + Memory = effectiveMemory, + Parallelism = effectiveParallelism + }; + + if (requestModel.MasterPasswordUnlock != null) + { + var unlock = requestModel.MasterPasswordUnlock; + // Always force a valid encrypted string for tests to avoid model validation failures. + requestModel.MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = alignedKdf, + MasterKeyWrappedUserKey = unlock.MasterKeyWrappedUserKey, + Salt = string.IsNullOrWhiteSpace(unlock.Salt) ? requestModel.Email : unlock.Salt + }; + } + + if (requestModel.MasterPasswordAuthentication != null) + { + // Ensure registration uses the same hash the tests will provide at login. + // PM-28143 - When MasterPasswordAuthenticationData is the only source of the auth hash, + // stop overriding it from MasterPasswordHash and delete this whole reassignment block. + requestModel.MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = alignedKdf, + MasterPasswordAuthenticationHash = requestModel.MasterPasswordHash, + Salt = requestModel.Email + }; + } + var sendVerificationEmailReqModel = new RegisterSendVerificationEmailRequestModel { Email = requestModel.Email, @@ -211,8 +277,11 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase requestModel.EmailVerificationToken = RegistrationTokens[requestModel.Email]; var postRegisterFinishHttpContext = await PostRegisterFinishAsync(requestModel); - - Assert.Equal(StatusCodes.Status200OK, postRegisterFinishHttpContext.Response.StatusCode); + if (postRegisterFinishHttpContext.Response.StatusCode != StatusCodes.Status200OK) + { + var body = await ReadResponseBodyAsync(postRegisterFinishHttpContext); + Assert.Fail($"register/finish failed (status {postRegisterFinishHttpContext.Response.StatusCode}). Body: {body}"); + } var database = GetDatabaseContext(); var user = await database.Users @@ -222,4 +291,32 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase return user; } + + private static async Task ReadResponseBodyAsync(HttpContext ctx) + { + try + { + if (ctx?.Response?.Body == null) + { + return ""; + } + var stream = ctx.Response.Body; + if (stream.CanSeek) + { + stream.Seek(0, SeekOrigin.Begin); + } + using var reader = new StreamReader(stream, Encoding.UTF8, detectEncodingFromByteOrderMarks: false, leaveOpen: true); + var text = await reader.ReadToEndAsync(); + if (stream.CanSeek) + { + stream.Seek(0, SeekOrigin.Begin); + } + return string.IsNullOrWhiteSpace(text) ? "" : text; + } + catch (Exception ex) + { + return $""; + } + } + } From 029a5f6a2df73f74c7f095dfba60b475edbec244 Mon Sep 17 00:00:00 2001 From: Patrick-Pimentel-Bitwarden Date: Thu, 15 Jan 2026 16:19:16 -0500 Subject: [PATCH 5/7] Revert "feat(register): [PM-27084] Account Register Uses New Data Types (#6715)" (#6854) This reverts commit 8cb80305341098673771e863b8b5266ad19bdce0. --- .../Request/Accounts/PasswordRequestModel.cs | 6 +- .../SetInitialPasswordRequestModel.cs | 1 + .../Models/Requests}/KdfRequestModel.cs | 11 +- ...rPasswordAuthenticationDataRequestModel.cs | 6 +- .../MasterPasswordUnlockDataRequestModel.cs | 6 +- .../Accounts/RegisterFinishRequestModel.cs | 201 +------ src/Core/Entities/User.cs | 6 +- .../Data/MasterPasswordAuthenticationData.cs | 5 - ...sterPasswordUnlockAndAuthenticationData.cs | 3 +- .../Models/Data/MasterPasswordUnlockData.cs | 5 - src/Core/Utilities/KdfSettingsValidator.cs | 1 - .../Controllers/AccountsController.cs | 61 +-- .../Controllers/AccountsControllerTest.cs | 4 +- .../Controllers/AccountsControllerTests.cs | 1 + .../SetInitialPasswordRequestModelTests.cs | 1 + .../RegisterFinishRequestModelFixtures.cs | 4 +- .../RegisterFinishRequestModelTests.cs | 183 ------- .../Controllers/AccountsControllerTests.cs | 502 +----------------- .../Factories/IdentityApplicationFactory.cs | 101 +--- 19 files changed, 63 insertions(+), 1045 deletions(-) rename src/{Core/KeyManagement/Models/Api/Request => Api/KeyManagement/Models/Requests}/KdfRequestModel.cs (59%) rename src/{Core/KeyManagement/Models/Api/Request => Api/KeyManagement/Models/Requests}/MasterPasswordAuthenticationDataRequestModel.cs (71%) rename src/{Core/KeyManagement/Models/Api/Request => Api/KeyManagement/Models/Requests}/MasterPasswordUnlockDataRequestModel.cs (71%) diff --git a/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs b/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs index ab8c727852..8fa51e9f34 100644 --- a/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs +++ b/src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs @@ -1,5 +1,7 @@ -using System.ComponentModel.DataAnnotations; -using Bit.Core.KeyManagement.Models.Api.Request; +#nullable enable + +using System.ComponentModel.DataAnnotations; +using Bit.Api.KeyManagement.Models.Requests; namespace Bit.Api.Auth.Models.Request.Accounts; diff --git a/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs b/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs index 37a7901fee..55ffdca94b 100644 --- a/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs +++ b/src/Api/Auth/Models/Request/Accounts/SetInitialPasswordRequestModel.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using Bit.Api.KeyManagement.Models.Requests; using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Auth.Models.Data; using Bit.Core.Entities; diff --git a/src/Core/KeyManagement/Models/Api/Request/KdfRequestModel.cs b/src/Api/KeyManagement/Models/Requests/KdfRequestModel.cs similarity index 59% rename from src/Core/KeyManagement/Models/Api/Request/KdfRequestModel.cs rename to src/Api/KeyManagement/Models/Requests/KdfRequestModel.cs index edcd7f760f..904304a633 100644 --- a/src/Core/KeyManagement/Models/Api/Request/KdfRequestModel.cs +++ b/src/Api/KeyManagement/Models/Requests/KdfRequestModel.cs @@ -1,11 +1,10 @@ using System.ComponentModel.DataAnnotations; using Bit.Core.Enums; using Bit.Core.KeyManagement.Models.Data; -using Bit.Core.Utilities; -namespace Bit.Core.KeyManagement.Models.Api.Request; +namespace Bit.Api.KeyManagement.Models.Requests; -public class KdfRequestModel : IValidatableObject +public class KdfRequestModel { [Required] public required KdfType KdfType { get; init; } @@ -24,10 +23,4 @@ public class KdfRequestModel : IValidatableObject 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/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs b/src/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs similarity index 71% rename from src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs rename to src/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs index 04c22cc3a6..4f70a1135f 100644 --- a/src/Core/KeyManagement/Models/Api/Request/MasterPasswordAuthenticationDataRequestModel.cs +++ b/src/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs @@ -1,12 +1,8 @@ using System.ComponentModel.DataAnnotations; using Bit.Core.KeyManagement.Models.Data; -namespace Bit.Core.KeyManagement.Models.Api.Request; +namespace Bit.Api.KeyManagement.Models.Requests; -/// -/// Use this datatype when interfacing with requests to create a separation of concern. -/// See to use for commands, queries, services. -/// public class MasterPasswordAuthenticationDataRequestModel { public required KdfRequestModel Kdf { get; init; } diff --git a/src/Core/KeyManagement/Models/Api/Request/MasterPasswordUnlockDataRequestModel.cs b/src/Api/KeyManagement/Models/Requests/MasterPasswordUnlockDataRequestModel.cs similarity index 71% rename from src/Core/KeyManagement/Models/Api/Request/MasterPasswordUnlockDataRequestModel.cs rename to src/Api/KeyManagement/Models/Requests/MasterPasswordUnlockDataRequestModel.cs index 8d7df86374..e1d7863cae 100644 --- a/src/Core/KeyManagement/Models/Api/Request/MasterPasswordUnlockDataRequestModel.cs +++ b/src/Api/KeyManagement/Models/Requests/MasterPasswordUnlockDataRequestModel.cs @@ -2,12 +2,8 @@ using Bit.Core.KeyManagement.Models.Data; using Bit.Core.Utilities; -namespace Bit.Core.KeyManagement.Models.Api.Request; +namespace Bit.Api.KeyManagement.Models.Requests; -/// -/// Use this datatype when interfacing with requests to create a separation of concern. -/// See to use for commands, queries, services. -/// public class MasterPasswordUnlockDataRequestModel { public required KdfRequestModel Kdf { get; init; } diff --git a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs index cb66540a6b..0ac7dbbcb4 100644 --- a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs +++ b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs @@ -1,6 +1,6 @@ -using Bit.Core.Entities; +#nullable enable +using Bit.Core.Entities; using Bit.Core.Enums; -using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.Utilities; namespace Bit.Core.Auth.Models.Api.Request.Accounts; @@ -21,32 +21,19 @@ public class RegisterFinishRequestModel : IValidatableObject public required string Email { get; set; } public string? EmailVerificationToken { get; set; } - public MasterPasswordAuthenticationDataRequestModel? MasterPasswordAuthentication { get; set; } - public MasterPasswordUnlockDataRequestModel? MasterPasswordUnlock { get; set; } - - // PM-28143 - Remove property below (made optional during migration to MasterPasswordUnlockData) [StringLength(1000)] - // Made optional but there will still be a thrown error if it does not exist either here or - // in the MasterPasswordAuthenticationData. - public string? MasterPasswordHash { get; set; } + public required string MasterPasswordHash { get; set; } [StringLength(50)] public string? MasterPasswordHint { get; set; } - // PM-28143 - Remove property below (made optional during migration to MasterPasswordUnlockData) - // Made optional but there will still be a thrown error if it does not exist either here or - // in the MasterPasswordAuthenticationData. - public string? UserSymmetricKey { get; set; } + public required string UserSymmetricKey { get; set; } public required KeysRequestModel UserAsymmetricKeys { get; set; } - // PM-28143 - Remove line below (made optional during migration to MasterPasswordUnlockData) - public KdfType? Kdf { get; set; } - // PM-28143 - Remove line below (made optional during migration to MasterPasswordUnlockData) - public int? KdfIterations { get; set; } - // PM-28143 - Remove line below + public required KdfType Kdf { get; set; } + public required int KdfIterations { get; set; } public int? KdfMemory { get; set; } - // PM-28143 - Remove line below public int? KdfParallelism { get; set; } public Guid? OrganizationUserId { get; set; } @@ -67,14 +54,11 @@ public class RegisterFinishRequestModel : IValidatableObject { Email = Email, MasterPasswordHint = MasterPasswordHint, - Kdf = (KdfType)(MasterPasswordUnlock?.Kdf.KdfType ?? Kdf)!, - KdfIterations = (int)(MasterPasswordUnlock?.Kdf.Iterations ?? KdfIterations)!, - // 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 = MasterPasswordUnlock?.Salt ?? Email.ToLower().Trim(), - Key = MasterPasswordUnlock?.MasterKeyWrappedUserKey ?? UserSymmetricKey + Kdf = Kdf, + KdfIterations = KdfIterations, + KdfMemory = KdfMemory, + KdfParallelism = KdfParallelism, + Key = UserSymmetricKey, }; UserAsymmetricKeys.ToUser(user); @@ -88,9 +72,7 @@ public class RegisterFinishRequestModel : IValidatableObject { return RegisterFinishTokenType.EmailVerification; } - if (!string.IsNullOrEmpty(OrgInviteToken) - && OrganizationUserId.HasValue - && OrganizationUserId.Value != Guid.Empty) + if (!string.IsNullOrEmpty(OrgInviteToken) && OrganizationUserId.HasValue) { return RegisterFinishTokenType.OrganizationInvite; } @@ -98,15 +80,11 @@ public class RegisterFinishRequestModel : IValidatableObject { return RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan; } - if (!string.IsNullOrWhiteSpace(AcceptEmergencyAccessInviteToken) - && AcceptEmergencyAccessId.HasValue - && AcceptEmergencyAccessId.Value != Guid.Empty) + if (!string.IsNullOrWhiteSpace(AcceptEmergencyAccessInviteToken) && AcceptEmergencyAccessId.HasValue) { return RegisterFinishTokenType.EmergencyAccessInvite; } - if (!string.IsNullOrWhiteSpace(ProviderInviteToken) - && ProviderUserId.HasValue - && ProviderUserId.Value != Guid.Empty) + if (!string.IsNullOrWhiteSpace(ProviderInviteToken) && ProviderUserId.HasValue) { return RegisterFinishTokenType.ProviderInvite; } @@ -114,156 +92,9 @@ public class RegisterFinishRequestModel : IValidatableObject throw new InvalidOperationException("Invalid token type."); } + public IEnumerable Validate(ValidationContext validationContext) { - // 1. Authentication data containing hash and hash at root level check - if (MasterPasswordAuthentication != null && MasterPasswordHash != null) - { - if (MasterPasswordAuthentication.MasterPasswordAuthenticationHash != MasterPasswordHash) - { - yield return new ValidationResult( - $"{nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash)} and root level {nameof(MasterPasswordHash)} provided and are not equal. Only provide one.", - [nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash), nameof(MasterPasswordHash)]); - } - } // 1.5 if there is no master password hash that is unacceptable even though they are both optional in the model - else if (MasterPasswordAuthentication == null && MasterPasswordHash == null) - { - yield return new ValidationResult( - $"{nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash)} and {nameof(MasterPasswordHash)} not found on request, one needs to be defined.", - [nameof(MasterPasswordAuthentication.MasterPasswordAuthenticationHash), nameof(MasterPasswordHash)]); - } - - // 2. Validate kdf settings. - if (MasterPasswordUnlock != null) - { - foreach (var validationResult in KdfSettingsValidator.Validate(MasterPasswordUnlock.ToData().Kdf)) - { - yield return validationResult; - } - } - - if (MasterPasswordAuthentication != null) - { - foreach (var validationResult in KdfSettingsValidator.Validate(MasterPasswordAuthentication.ToData().Kdf)) - { - yield return validationResult; - } - } - - // 3. Validate root kdf values if kdf values are not in the unlock and authentication. - if (MasterPasswordUnlock == null && MasterPasswordAuthentication == null) - { - var hasMissingRequiredKdfInputs = false; - if (Kdf == null) - { - yield return new ValidationResult($"{nameof(Kdf)} not found on RequestModel", [nameof(Kdf)]); - hasMissingRequiredKdfInputs = true; - } - if (KdfIterations == null) - { - yield return new ValidationResult($"{nameof(KdfIterations)} not found on RequestModel", [nameof(KdfIterations)]); - hasMissingRequiredKdfInputs = true; - } - - if (!hasMissingRequiredKdfInputs) - { - foreach (var validationResult in KdfSettingsValidator.Validate( - Kdf!.Value, - KdfIterations!.Value, - KdfMemory, - KdfParallelism)) - { - yield return validationResult; - } - } - } - else if (MasterPasswordUnlock == null && MasterPasswordAuthentication != null) - { - // Authentication provided but Unlock missing - yield return new ValidationResult($"{nameof(MasterPasswordUnlock)} not found on RequestModel", [nameof(MasterPasswordUnlock)]); - } - else if (MasterPasswordUnlock != null && MasterPasswordAuthentication == null) - { - // Unlock provided but Authentication missing - yield return new ValidationResult($"{nameof(MasterPasswordAuthentication)} not found on RequestModel", [nameof(MasterPasswordAuthentication)]); - } - - // 3. Lastly, validate access token type and presence. Must be done last because of yield break. - RegisterFinishTokenType tokenType; - var tokenTypeResolved = true; - try - { - tokenType = GetTokenType(); - } - catch (InvalidOperationException) - { - tokenTypeResolved = false; - tokenType = default; - } - - if (!tokenTypeResolved) - { - yield return new ValidationResult("No valid registration token provided"); - yield break; - } - - switch (tokenType) - { - case RegisterFinishTokenType.EmailVerification: - if (string.IsNullOrEmpty(EmailVerificationToken)) - { - yield return new ValidationResult( - $"{nameof(EmailVerificationToken)} absent when processing register/finish.", - [nameof(EmailVerificationToken)]); - } - break; - case RegisterFinishTokenType.OrganizationInvite: - if (string.IsNullOrEmpty(OrgInviteToken)) - { - yield return new ValidationResult( - $"{nameof(OrgInviteToken)} absent when processing register/finish.", - [nameof(OrgInviteToken)]); - } - break; - case RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan: - if (string.IsNullOrEmpty(OrgSponsoredFreeFamilyPlanToken)) - { - yield return new ValidationResult( - $"{nameof(OrgSponsoredFreeFamilyPlanToken)} absent when processing register/finish.", - [nameof(OrgSponsoredFreeFamilyPlanToken)]); - } - break; - case RegisterFinishTokenType.EmergencyAccessInvite: - if (string.IsNullOrEmpty(AcceptEmergencyAccessInviteToken)) - { - yield return new ValidationResult( - $"{nameof(AcceptEmergencyAccessInviteToken)} absent when processing register/finish.", - [nameof(AcceptEmergencyAccessInviteToken)]); - } - if (!AcceptEmergencyAccessId.HasValue || AcceptEmergencyAccessId.Value == Guid.Empty) - { - yield return new ValidationResult( - $"{nameof(AcceptEmergencyAccessId)} absent when processing register/finish.", - [nameof(AcceptEmergencyAccessId)]); - } - break; - case RegisterFinishTokenType.ProviderInvite: - if (string.IsNullOrEmpty(ProviderInviteToken)) - { - yield return new ValidationResult( - $"{nameof(ProviderInviteToken)} absent when processing register/finish.", - [nameof(ProviderInviteToken)]); - } - if (!ProviderUserId.HasValue || ProviderUserId.Value == Guid.Empty) - { - yield return new ValidationResult( - $"{nameof(ProviderUserId)} absent when processing register/finish.", - [nameof(ProviderUserId)]); - } - break; - default: - yield return new ValidationResult("Invalid registration finish request"); - break; - } + return KdfSettingsValidator.Validate(Kdf, KdfIterations, KdfMemory, KdfParallelism); } } diff --git a/src/Core/Entities/User.cs b/src/Core/Entities/User.cs index 422dc37c6e..669e32bcbe 100644 --- a/src/Core/Entities/User.cs +++ b/src/Core/Entities/User.cs @@ -7,6 +7,8 @@ using Bit.Core.KeyManagement.Models.Data; using Bit.Core.Utilities; using Microsoft.AspNetCore.Identity; +#nullable enable + namespace Bit.Core.Entities; public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFactorProvidersUser @@ -49,7 +51,7 @@ public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFac public string? Key { get; set; } /// /// The raw public key, without a signature from the user's signature key. - /// + /// public string? PublicKey { get; set; } /// /// User key wrapped private key. @@ -105,8 +107,6 @@ public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFac public DateTime? LastKeyRotationDate { get; set; } public DateTime? LastEmailChangeDate { get; set; } public bool VerifyDevices { get; set; } = true; - // PM-28827 Uncomment below line. - // public string? MasterPasswordSalt { get; set; } public string GetMasterPasswordSalt() { diff --git a/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs b/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs index 6e53dfa744..1bc7006cef 100644 --- a/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs +++ b/src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs @@ -1,13 +1,8 @@ using Bit.Core.Entities; using Bit.Core.Exceptions; -using Bit.Core.KeyManagement.Models.Api.Request; namespace Bit.Core.KeyManagement.Models.Data; -/// -/// Use this datatype when interfacing with commands, queries, services to create a separation of concern. -/// See to use for requests. -/// public class MasterPasswordAuthenticationData { public required KdfSettings Kdf { get; init; } diff --git a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs index b79ce8bce1..ad3a0b692b 100644 --- a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs +++ b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs @@ -1,4 +1,5 @@ -using Bit.Core.Entities; +#nullable enable +using Bit.Core.Entities; using Bit.Core.Enums; namespace Bit.Core.KeyManagement.Models.Data; diff --git a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs index f8139cba99..cb18ed2a78 100644 --- a/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs +++ b/src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs @@ -1,13 +1,8 @@ using Bit.Core.Entities; using Bit.Core.Exceptions; -using Bit.Core.KeyManagement.Models.Api.Request; namespace Bit.Core.KeyManagement.Models.Data; -/// -/// Use this datatype when interfacing with commands, queries, services to create a separation of concern. -/// See to use for requests. -/// public class MasterPasswordUnlockData { public required KdfSettings Kdf { get; init; } diff --git a/src/Core/Utilities/KdfSettingsValidator.cs b/src/Core/Utilities/KdfSettingsValidator.cs index e5690ad469..f89e8ddb66 100644 --- a/src/Core/Utilities/KdfSettingsValidator.cs +++ b/src/Core/Utilities/KdfSettingsValidator.cs @@ -6,7 +6,6 @@ namespace Bit.Core.Utilities; public static class KdfSettingsValidator { - // PM-28143 - Remove below when fixing ticket public static IEnumerable Validate(KdfType kdfType, int kdfIterations, int? kdfMemory, int? kdfParallelism) { switch (kdfType) diff --git a/src/Identity/Controllers/AccountsController.cs b/src/Identity/Controllers/AccountsController.cs index e9807fb1fc..b7d4342c1b 100644 --- a/src/Identity/Controllers/AccountsController.cs +++ b/src/Identity/Controllers/AccountsController.cs @@ -1,4 +1,8 @@ -using System.Text; +// FIXME: Update this file to be null safe and then delete the line below +#nullable disable + +using System.Diagnostics; +using System.Text; using Bit.Core; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models.Api.Request.Accounts; @@ -38,7 +42,7 @@ public class AccountsController : Controller private readonly IFeatureService _featureService; private readonly IDataProtectorTokenFactory _registrationEmailVerificationTokenDataFactory; - private readonly byte[]? _defaultKdfHmacKey = null; + private readonly byte[] _defaultKdfHmacKey = null; private static readonly List _defaultKdfResults = [ // The first result (index 0) should always return the "normal" default. @@ -141,55 +145,40 @@ public class AccountsController : Controller [HttpPost("register/finish")] public async Task PostRegisterFinish([FromBody] RegisterFinishRequestModel model) { - User user = model.ToUser(); + var user = model.ToUser(); // Users will either have an emailed token or an email verification token - not both. - IdentityResult? identityResult = null; - - // PM-28143 - Just use the MasterPasswordAuthenticationData.MasterPasswordAuthenticationHash - string masterPasswordAuthenticationHash = model.MasterPasswordAuthentication?.MasterPasswordAuthenticationHash - ?? model.MasterPasswordHash!; + IdentityResult identityResult = null; switch (model.GetTokenType()) { case RegisterFinishTokenType.EmailVerification: - identityResult = await _registerUserCommand.RegisterUserViaEmailVerificationToken( - user, - masterPasswordAuthenticationHash, - model.EmailVerificationToken!); - return ProcessRegistrationResult(identityResult, user); + identityResult = + await _registerUserCommand.RegisterUserViaEmailVerificationToken(user, model.MasterPasswordHash, + model.EmailVerificationToken); + return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.OrganizationInvite: - identityResult = await _registerUserCommand.RegisterUserViaOrganizationInviteToken( - user, - masterPasswordAuthenticationHash, - model.OrgInviteToken!, - model.OrganizationUserId); - return ProcessRegistrationResult(identityResult, user); + identityResult = await _registerUserCommand.RegisterUserViaOrganizationInviteToken(user, model.MasterPasswordHash, + model.OrgInviteToken, model.OrganizationUserId); + return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.OrgSponsoredFreeFamilyPlan: - identityResult = await _registerUserCommand.RegisterUserViaOrganizationSponsoredFreeFamilyPlanInviteToken( - user, - masterPasswordAuthenticationHash, - model.OrgSponsoredFreeFamilyPlanToken!); - return ProcessRegistrationResult(identityResult, user); + identityResult = await _registerUserCommand.RegisterUserViaOrganizationSponsoredFreeFamilyPlanInviteToken(user, model.MasterPasswordHash, model.OrgSponsoredFreeFamilyPlanToken); + return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.EmergencyAccessInvite: - identityResult = await _registerUserCommand.RegisterUserViaAcceptEmergencyAccessInviteToken( - user, - masterPasswordAuthenticationHash, - model.AcceptEmergencyAccessInviteToken!, - (Guid)model.AcceptEmergencyAccessId!); - return ProcessRegistrationResult(identityResult, user); + Debug.Assert(model.AcceptEmergencyAccessId.HasValue); + identityResult = await _registerUserCommand.RegisterUserViaAcceptEmergencyAccessInviteToken(user, model.MasterPasswordHash, + model.AcceptEmergencyAccessInviteToken, model.AcceptEmergencyAccessId.Value); + return ProcessRegistrationResult(identityResult, user); case RegisterFinishTokenType.ProviderInvite: - identityResult = await _registerUserCommand.RegisterUserViaProviderInviteToken( - user, - masterPasswordAuthenticationHash, - model.ProviderInviteToken!, - (Guid)model.ProviderUserId!); - return ProcessRegistrationResult(identityResult, user); + Debug.Assert(model.ProviderUserId.HasValue); + identityResult = await _registerUserCommand.RegisterUserViaProviderInviteToken(user, model.MasterPasswordHash, + model.ProviderInviteToken, model.ProviderUserId.Value); + return ProcessRegistrationResult(identityResult, user); default: throw new BadRequestException("Invalid registration finish request"); } diff --git a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs index 9860775e31..d055418f3a 100644 --- a/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs +++ b/test/Api.IntegrationTest/Controllers/AccountsControllerTest.cs @@ -3,6 +3,7 @@ using System.Text.Json; using Bit.Api.Auth.Models.Request.Accounts; using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; +using Bit.Api.KeyManagement.Models.Requests; using Bit.Api.Models.Response; using Bit.Core; using Bit.Core.Auth.Entities; @@ -11,7 +12,6 @@ using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.Repositories; using Bit.Core.Entities; using Bit.Core.Enums; -using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.KeyManagement.Repositories; using Bit.Core.Models.Data; using Bit.Core.Platform.Push; @@ -378,7 +378,7 @@ public class AccountsControllerTest : IClassFixture, IAsy Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); var content = await response.Content.ReadAsStringAsync(); - Assert.Contains("The model state is invalid", content); + Assert.Contains("KDF settings are invalid", content); } [Fact] diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index 665d1e52c1..6cddd341d5 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -1,6 +1,7 @@ using System.Security.Claims; using Bit.Api.Auth.Controllers; using Bit.Api.Auth.Models.Request.Accounts; +using Bit.Api.KeyManagement.Models.Requests; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; using Bit.Core.Auth.Models.Api.Request.Accounts; diff --git a/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs b/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs index 97e69dacbc..ce8ba1811e 100644 --- a/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs +++ b/test/Api.Test/Auth/Models/Request/Accounts/SetInitialPasswordRequestModelTests.cs @@ -1,5 +1,6 @@ using System.ComponentModel.DataAnnotations; using Bit.Api.Auth.Models.Request.Accounts; +using Bit.Api.KeyManagement.Models.Requests; using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Entities; using Bit.Core.Enums; diff --git a/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs b/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs index 22fca7ab59..a751a16f31 100644 --- a/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs +++ b/test/Core.Test/Auth/AutoFixture/RegisterFinishRequestModelFixtures.cs @@ -29,9 +29,7 @@ internal class RegisterFinishRequestModelCustomization : ICustomization .With(o => o.OrgInviteToken, OrgInviteToken) .With(o => o.OrgSponsoredFreeFamilyPlanToken, OrgSponsoredFreeFamilyPlanToken) .With(o => o.AcceptEmergencyAccessInviteToken, AcceptEmergencyAccessInviteToken) - .With(o => o.ProviderInviteToken, ProviderInviteToken) - .Without(o => o.MasterPasswordAuthentication) - .Without(o => o.MasterPasswordUnlock)); + .With(o => o.ProviderInviteToken, ProviderInviteToken)); } } diff --git a/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs b/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs index 3c099ce962..588ca878fc 100644 --- a/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs +++ b/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs @@ -1,6 +1,5 @@ using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Enums; -using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Test.Common.AutoFixture.Attributes; using Xunit; @@ -8,17 +7,6 @@ namespace Bit.Core.Test.Auth.Models.Api.Request.Accounts; public class RegisterFinishRequestModelTests { - private static List Validate(RegisterFinishRequestModel model) - { - var results = new List(); - System.ComponentModel.DataAnnotations.Validator.TryValidateObject( - model, - new System.ComponentModel.DataAnnotations.ValidationContext(model), - results, - true); - return results; - } - [Theory] [BitAutoData] public void GetTokenType_Returns_EmailVerification(string email, string masterPasswordHash, @@ -182,175 +170,4 @@ public class RegisterFinishRequestModelTests Assert.Equal(userAsymmetricKeys.PublicKey, result.PublicKey); Assert.Equal(userAsymmetricKeys.EncryptedPrivateKey, result.PrivateKey); } - - [Fact] - public void Validate_WhenBothAuthAndRootHashProvidedButNotEqual_ReturnsMismatchError() - { - var model = new RegisterFinishRequestModel - { - Email = "user@example.com", - MasterPasswordHash = "root-hash", - UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, - // Provide both unlock and authentication with valid KDF so only the mismatch rule fires - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterKeyWrappedUserKey = "wrapped", - Salt = "salt" - }, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterPasswordAuthenticationHash = "auth-hash", // different than root - Salt = "salt" - }, - // Provide any valid token so we don't fail token validation - EmailVerificationToken = "token" - }; - - var results = Validate(model); - - Assert.Contains(results, r => - r.ErrorMessage == $"{nameof(MasterPasswordAuthenticationDataRequestModel.MasterPasswordAuthenticationHash)} and root level {nameof(RegisterFinishRequestModel.MasterPasswordHash)} provided and are not equal. Only provide one."); - } - - [Fact] - public void Validate_WhenAuthProvidedButUnlockMissing_ReturnsUnlockMissingError() - { - var model = new RegisterFinishRequestModel - { - Email = "user@example.com", - UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterPasswordAuthenticationHash = "auth-hash", - Salt = "salt" - }, - EmailVerificationToken = "token" - }; - - var results = Validate(model); - - Assert.Contains(results, r => r.ErrorMessage == "MasterPasswordUnlock not found on RequestModel"); - } - - [Fact] - public void Validate_WhenUnlockProvidedButAuthMissing_ReturnsAuthMissingError() - { - var model = new RegisterFinishRequestModel - { - Email = "user@example.com", - UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterKeyWrappedUserKey = "wrapped", - Salt = "salt" - }, - EmailVerificationToken = "token" - }; - - var results = Validate(model); - - Assert.Contains(results, r => r.ErrorMessage == "MasterPasswordAuthentication not found on RequestModel"); - } - - [Fact] - public void Validate_WhenNeitherAuthNorUnlock_AndRootKdfMissing_ReturnsBothRootKdfErrors() - { - var model = new RegisterFinishRequestModel - { - Email = "user@example.com", - UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, - // No MasterPasswordUnlock, no MasterPasswordAuthentication - // No root Kdf and KdfIterations to trigger both errors - EmailVerificationToken = "token" - }; - - var results = Validate(model); - - Assert.Contains(results, r => r.ErrorMessage == $"{nameof(RegisterFinishRequestModel.Kdf)} not found on RequestModel"); - Assert.Contains(results, r => r.ErrorMessage == $"{nameof(RegisterFinishRequestModel.KdfIterations)} not found on RequestModel"); - } - - [Fact] - public void Validate_WhenAuthAndRootHashBothMissing_ReturnsMissingHashErrorOnly() - { - var model = new RegisterFinishRequestModel - { - Email = "user@example.com", - UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, - // Both MasterPasswordAuthentication and MasterPasswordHash are missing - MasterPasswordAuthentication = null, - MasterPasswordHash = null, - // Provide valid root KDF to avoid root KDF errors - Kdf = KdfType.PBKDF2_SHA256, - KdfIterations = AuthConstants.PBKDF2_ITERATIONS.Default, - EmailVerificationToken = "token" // avoid token error - }; - - var results = Validate(model); - - // Only the new missing hash error should be present - Assert.Single(results); - Assert.Equal($"{nameof(MasterPasswordAuthenticationDataRequestModel.MasterPasswordAuthenticationHash)} and {nameof(RegisterFinishRequestModel.MasterPasswordHash)} not found on request, one needs to be defined.", results[0].ErrorMessage); - Assert.Contains(nameof(MasterPasswordAuthenticationDataRequestModel.MasterPasswordAuthenticationHash), results[0].MemberNames); - Assert.Contains(nameof(RegisterFinishRequestModel.MasterPasswordHash), results[0].MemberNames); - } - - [Fact] - public void Validate_WhenAllFieldsValidWithSubModels_IsValid() - { - var model = new RegisterFinishRequestModel - { - Email = "user@example.com", - UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterKeyWrappedUserKey = "wrapped", - Salt = "salt" - }, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterPasswordAuthenticationHash = "auth-hash", - Salt = "salt" - }, - EmailVerificationToken = "token" - }; - - var results = Validate(model); - - Assert.Empty(results); - } - - [Fact] - public void Validate_WhenNoValidRegistrationTokenProvided_ReturnsTokenErrorOnly() - { - var model = new RegisterFinishRequestModel - { - Email = "user@example.com", - UserAsymmetricKeys = new KeysRequestModel { PublicKey = "pk", EncryptedPrivateKey = "sk" }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterKeyWrappedUserKey = "wrapped", - Salt = "salt" - }, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = new KdfRequestModel { KdfType = KdfType.PBKDF2_SHA256, Iterations = AuthConstants.PBKDF2_ITERATIONS.Default }, - MasterPasswordAuthenticationHash = "auth-hash", - Salt = "salt" - } - // No token fields set - }; - - var results = Validate(model); - - Assert.Single(results); - Assert.Equal("No valid registration token provided", results[0].ErrorMessage); - } } diff --git a/test/Identity.Test/Controllers/AccountsControllerTests.cs b/test/Identity.Test/Controllers/AccountsControllerTests.cs index 86e461d155..42e033bdd7 100644 --- a/test/Identity.Test/Controllers/AccountsControllerTests.cs +++ b/test/Identity.Test/Controllers/AccountsControllerTests.cs @@ -1,5 +1,4 @@ -using System.ComponentModel.DataAnnotations; -using System.Reflection; +using System.Reflection; using System.Text; using Bit.Core; using Bit.Core.Auth.Models.Api.Request.Accounts; @@ -10,7 +9,6 @@ using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; -using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.Models.Data; using Bit.Core.Repositories; using Bit.Core.Services; @@ -592,504 +590,6 @@ public class AccountsControllerTests : IDisposable await Assert.ThrowsAsync(() => _sut.PostRegisterVerificationEmailClicked(requestModel)); } - // PM-28143 - When removing the old properties, update this test to just test the new properties working - // as expected. - [Theory, BitAutoData] - public async Task PostRegisterFinish_EmailVerification_BothDataForms_ProduceEquivalentOutcomes( - string email, - string emailVerificationToken, - string masterPasswordHash, - string masterKeyWrappedUserKey, - string publicKey, - string encryptedPrivateKey) - { - // Arrange: new-form model (MasterPasswordAuthenticationData + MasterPasswordUnlockData) - - var kdfData = new KdfRequestModel - { - KdfType = KdfType.Argon2id, - Iterations = AuthConstants.ARGON2_ITERATIONS.Default, - Memory = AuthConstants.ARGON2_MEMORY.Default, - Parallelism = AuthConstants.ARGON2_PARALLELISM.Default - }; - - var newModel = new RegisterFinishRequestModel - { - Email = email, - EmailVerificationToken = emailVerificationToken, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = kdfData, - MasterPasswordAuthenticationHash = masterPasswordHash, - Salt = email // salt choice is not validated here during registration - }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = kdfData, - MasterKeyWrappedUserKey = masterKeyWrappedUserKey, - Salt = email - }, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - // Arrange: legacy-form model (MasterPasswordHash + legacy KDF + UserSymmetricKey) - var legacyModel = new RegisterFinishRequestModel - { - Email = email, - EmailVerificationToken = emailVerificationToken, - MasterPasswordHash = masterPasswordHash, - Kdf = KdfType.Argon2id, - KdfIterations = AuthConstants.ARGON2_ITERATIONS.Default, - KdfMemory = AuthConstants.ARGON2_MEMORY.Default, - KdfParallelism = AuthConstants.ARGON2_PARALLELISM.Default, - UserSymmetricKey = masterKeyWrappedUserKey, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - var newUser = newModel.ToUser(); - var legacyUser = legacyModel.ToUser(); - - _registerUserCommand - .RegisterUserViaEmailVerificationToken(Arg.Any(), masterPasswordHash, emailVerificationToken) - .Returns(Task.FromResult(IdentityResult.Success)); - - // Act: call with new form - var newResult = await _sut.PostRegisterFinish(newModel); - // Act: call with legacy form - var legacyResult = await _sut.PostRegisterFinish(legacyModel); - - // Assert: outcomes are identical in effect (success response) - Assert.NotNull(newResult); - Assert.NotNull(legacyResult); - - // Assert: effective users are equivalent - Assert.Equal(legacyUser.Email, newUser.Email); - Assert.Equal(legacyUser.MasterPasswordHint, newUser.MasterPasswordHint); - Assert.Equal(legacyUser.Kdf, newUser.Kdf); - Assert.Equal(legacyUser.KdfIterations, newUser.KdfIterations); - Assert.Equal(legacyUser.KdfMemory, newUser.KdfMemory); - Assert.Equal(legacyUser.KdfParallelism, newUser.KdfParallelism); - Assert.Equal(legacyUser.Key, newUser.Key); - Assert.Equal(legacyUser.PublicKey, newUser.PublicKey); - Assert.Equal(legacyUser.PrivateKey, newUser.PrivateKey); - - // Assert: hash forwarded identically from both inputs - await _registerUserCommand.Received(2).RegisterUserViaEmailVerificationToken( - Arg.Is(u => - u.Email == newUser.Email && - u.Kdf == newUser.Kdf && - u.KdfIterations == newUser.KdfIterations && - u.KdfMemory == newUser.KdfMemory && - u.KdfParallelism == newUser.KdfParallelism && - u.Key == newUser.Key), - masterPasswordHash, - emailVerificationToken); - - await _registerUserCommand.Received(2).RegisterUserViaEmailVerificationToken( - Arg.Is(u => - u.Email == legacyUser.Email && - u.Kdf == legacyUser.Kdf && - u.KdfIterations == legacyUser.KdfIterations && - u.KdfMemory == legacyUser.KdfMemory && - u.KdfParallelism == legacyUser.KdfParallelism && - u.Key == legacyUser.Key), - masterPasswordHash, - emailVerificationToken); - } - - // PM-28143 - When removing the old properties, update this test to just test the new properties working - // as expected. - [Theory, BitAutoData] - public async Task PostRegisterFinish_OrgInvite_BothDataForms_ProduceEquivalentOutcomes( - string email, - string orgInviteToken, - Guid organizationUserId, - string masterPasswordHash, - string masterKeyWrappedUserKey, - string publicKey, - string encryptedPrivateKey) - { - var kdfData = new KdfRequestModel - { - KdfType = KdfType.Argon2id, - Iterations = AuthConstants.ARGON2_ITERATIONS.Default, - Memory = AuthConstants.ARGON2_MEMORY.Default, - Parallelism = AuthConstants.ARGON2_PARALLELISM.Default - }; - - // Arrange: new-form model (MasterPasswordAuthenticationData + MasterPasswordUnlockData) - var newModel = new RegisterFinishRequestModel - { - Email = email, - OrgInviteToken = orgInviteToken, - OrganizationUserId = organizationUserId, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = kdfData, - MasterPasswordAuthenticationHash = masterPasswordHash, - Salt = email - }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = kdfData, - MasterKeyWrappedUserKey = masterKeyWrappedUserKey, - Salt = email - }, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - // Arrange: legacy-form model (MasterPasswordHash + legacy KDF + UserSymmetricKey) - var legacyModel = new RegisterFinishRequestModel - { - Email = email, - OrgInviteToken = orgInviteToken, - OrganizationUserId = organizationUserId, - MasterPasswordHash = masterPasswordHash, - Kdf = kdfData.KdfType, - KdfIterations = kdfData.Iterations, - KdfMemory = kdfData.Memory, - KdfParallelism = kdfData.Parallelism, - UserSymmetricKey = masterKeyWrappedUserKey, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - var newUser = newModel.ToUser(); - var legacyUser = legacyModel.ToUser(); - - _registerUserCommand - .RegisterUserViaOrganizationInviteToken(Arg.Any(), masterPasswordHash, orgInviteToken, organizationUserId) - .Returns(Task.FromResult(IdentityResult.Success)); - - // Act - var newResult = await _sut.PostRegisterFinish(newModel); - var legacyResult = await _sut.PostRegisterFinish(legacyModel); - - // Assert success - Assert.NotNull(newResult); - Assert.NotNull(legacyResult); - - // Assert: effective users are equivalent - Assert.Equal(legacyUser.Email, newUser.Email); - Assert.Equal(legacyUser.MasterPasswordHint, newUser.MasterPasswordHint); - Assert.Equal(legacyUser.Kdf, newUser.Kdf); - Assert.Equal(legacyUser.KdfIterations, newUser.KdfIterations); - Assert.Equal(legacyUser.KdfMemory, newUser.KdfMemory); - Assert.Equal(legacyUser.KdfParallelism, newUser.KdfParallelism); - Assert.Equal(legacyUser.Key, newUser.Key); - Assert.Equal(legacyUser.PublicKey, newUser.PublicKey); - Assert.Equal(legacyUser.PrivateKey, newUser.PrivateKey); - - // Assert: hash forwarded identically from both inputs - await _registerUserCommand.Received(2).RegisterUserViaOrganizationInviteToken( - Arg.Is(u => - u.Email == newUser.Email && - u.Kdf == newUser.Kdf && - u.KdfIterations == newUser.KdfIterations && - u.KdfMemory == newUser.KdfMemory && - u.KdfParallelism == newUser.KdfParallelism && - u.Key == newUser.Key), - masterPasswordHash, - orgInviteToken, - organizationUserId); - - await _registerUserCommand.Received(2).RegisterUserViaOrganizationInviteToken( - Arg.Is(u => - u.Email == legacyUser.Email && - u.Kdf == legacyUser.Kdf && - u.KdfIterations == legacyUser.KdfIterations && - u.KdfMemory == legacyUser.KdfMemory && - u.KdfParallelism == legacyUser.KdfParallelism && - u.Key == legacyUser.Key), - masterPasswordHash, - orgInviteToken, - organizationUserId); - } - - [Theory, BitAutoData] - public async Task PostRegisterFinish_NewForm_UsesUnlockDataForKdfAndKey_WhenRootFieldsNull( - string email, - string emailVerificationToken, - string masterPasswordHash, - string masterKeyWrappedUserKey, - int iterations, - string publicKey, - string encryptedPrivateKey) - { - // Arrange: Provide only unlock-data KDF + key; leave root KDF fields null - var unlockKdf = new KdfRequestModel - { - KdfType = KdfType.PBKDF2_SHA256, - Iterations = iterations - }; - - var model = new RegisterFinishRequestModel - { - Email = email, - EmailVerificationToken = emailVerificationToken, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - // present but not used by ToUser for KDF/Key - Kdf = unlockKdf, - MasterPasswordAuthenticationHash = masterPasswordHash, - Salt = email - }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = unlockKdf, - MasterKeyWrappedUserKey = masterKeyWrappedUserKey, - Salt = email - }, - // root KDF fields intentionally null - Kdf = null, - KdfIterations = null, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - _registerUserCommand - .RegisterUserViaEmailVerificationToken(Arg.Any(), masterPasswordHash, emailVerificationToken) - .Returns(Task.FromResult(IdentityResult.Success)); - - // Act - var _ = await _sut.PostRegisterFinish(model); - - // Assert: The user passed to command uses unlock-data values - await _registerUserCommand.Received(1).RegisterUserViaEmailVerificationToken( - Arg.Is(u => - u.Email == email && - u.Kdf == unlockKdf.KdfType && - u.KdfIterations == unlockKdf.Iterations && - u.Key == masterKeyWrappedUserKey), - masterPasswordHash, - emailVerificationToken); - } - - [Theory, BitAutoData] - public async Task PostRegisterFinish_LegacyForm_UsesRootFields_WhenUnlockDataNull( - string email, - string emailVerificationToken, - string masterPasswordHash, - string legacyKey, - string publicKey, - string encryptedPrivateKey) - { - // Arrange: Provide only legacy root KDF + key; no unlock-data provided - var model = new RegisterFinishRequestModel - { - Email = email, - EmailVerificationToken = emailVerificationToken, - MasterPasswordHash = masterPasswordHash, - Kdf = KdfType.PBKDF2_SHA256, - KdfIterations = AuthConstants.PBKDF2_ITERATIONS.Default, - UserSymmetricKey = legacyKey, - MasterPasswordUnlock = null, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - _registerUserCommand - .RegisterUserViaEmailVerificationToken(Arg.Any(), masterPasswordHash, emailVerificationToken) - .Returns(Task.FromResult(IdentityResult.Success)); - - // Act - var _ = await _sut.PostRegisterFinish(model); - - // Assert: The user passed to command uses root values - await _registerUserCommand.Received(1).RegisterUserViaEmailVerificationToken( - Arg.Is(u => - u.Email == email && - u.Kdf == KdfType.PBKDF2_SHA256 && - u.KdfIterations == AuthConstants.PBKDF2_ITERATIONS.Default && - u.Key == legacyKey), - masterPasswordHash, - emailVerificationToken); - } - - [Theory, BitAutoData] - public void RegisterFinishRequestModel_Validate_Throws_WhenUnlockAndAuthDataMismatch( - string email, - string authHash, - string masterKeyWrappedUserKey, - string publicKey, - string encryptedPrivateKey) - { - // Arrange: authentication and unlock have different KDF and/or salt - var authKdf = new KdfRequestModel - { - KdfType = KdfType.PBKDF2_SHA256, - Iterations = AuthConstants.PBKDF2_ITERATIONS.Default - }; - var unlockKdf = new KdfRequestModel - { - KdfType = KdfType.Argon2id, - Iterations = AuthConstants.ARGON2_ITERATIONS.Default, - Memory = AuthConstants.ARGON2_MEMORY.Default, - Parallelism = AuthConstants.ARGON2_PARALLELISM.Default - }; - - var model = new RegisterFinishRequestModel - { - Email = email, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = authKdf, - MasterPasswordAuthenticationHash = authHash, - Salt = email - }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = unlockKdf, - MasterKeyWrappedUserKey = masterKeyWrappedUserKey, - Salt = email - }, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - // Provide a minimal valid token type to satisfy model-level token validation - model.EmailVerificationToken = "test-token"; - - var ctx = new ValidationContext(model); - - // Act - var results = model.Validate(ctx).ToList(); - - // Assert mismatched auth/unlock is allowed - Assert.Empty(results); - } - - [Theory, BitAutoData] - public void RegisterFinishRequestModel_Validate_Throws_WhenSaltMismatch( - string email, - string authHash, - string masterKeyWrappedUserKey, - string publicKey, - string encryptedPrivateKey) - { - var unlockKdf = new KdfRequestModel - { - KdfType = KdfType.Argon2id, - Iterations = AuthConstants.ARGON2_ITERATIONS.Default, - Memory = AuthConstants.ARGON2_MEMORY.Default, - Parallelism = AuthConstants.ARGON2_PARALLELISM.Default - }; - - var model = new RegisterFinishRequestModel - { - Email = email, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = unlockKdf, - MasterPasswordAuthenticationHash = authHash, - Salt = email - }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = unlockKdf, - MasterKeyWrappedUserKey = masterKeyWrappedUserKey, - // Intentionally different salt to force mismatch - Salt = email + ".mismatch" - }, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - // Provide a minimal valid token type to satisfy model-level token validation - model.EmailVerificationToken = "test-token"; - - var ctx = new ValidationContext(model); - - // Act - var results = model.Validate(ctx).ToList(); - - // Assert mismatched salts between auth/unlock are allowed - Assert.Empty(results); - } - - [Theory, BitAutoData] - public void RegisterFinishRequestModel_Validate_Throws_WhenAuthHashAndRootHashMismatch( - string email, - string authHash, - string differentRootHash, - string masterKeyWrappedUserKey, - string publicKey, - string encryptedPrivateKey) - { - // Arrange: same KDF/salt, but authentication hash differs from legacy root hash - var kdf = new KdfRequestModel - { - KdfType = KdfType.PBKDF2_SHA256, - Iterations = AuthConstants.PBKDF2_ITERATIONS.Default - }; - - var model = new RegisterFinishRequestModel - { - Email = email, - MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = kdf, - MasterPasswordAuthenticationHash = authHash, - Salt = email - }, - MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = kdf, - MasterKeyWrappedUserKey = masterKeyWrappedUserKey, - Salt = email - }, - // Intentionally set the legacy field to a different value to trigger the throw - MasterPasswordHash = differentRootHash, - UserAsymmetricKeys = new KeysRequestModel - { - PublicKey = publicKey, - EncryptedPrivateKey = encryptedPrivateKey - } - }; - - // Provide a minimal valid token type to satisfy model-level token validation - model.EmailVerificationToken = "test-token"; - - var ctx = new ValidationContext(model); - - // Act - var results = model.Validate(ctx).ToList(); - - // Assert: validation result exists with expected message and member names - var mismatchResult = Assert.Single(results.Where(r => - r.ErrorMessage == - "MasterPasswordAuthenticationHash and root level MasterPasswordHash provided and are not equal. Only provide one.")); - Assert.Contains("MasterPasswordAuthenticationHash", mismatchResult.MemberNames); - Assert.Contains("MasterPasswordHash", mismatchResult.MemberNames); - } - private void SetDefaultKdfHmacKey(byte[]? newKey) { var fieldInfo = typeof(AccountsController).GetField("_defaultKdfHmacKey", BindingFlags.NonPublic | BindingFlags.Instance); diff --git a/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs b/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs index e190dda427..ba12d1e1f4 100644 --- a/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs +++ b/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs @@ -3,13 +3,10 @@ using System.Collections.Concurrent; using System.Net.Http.Json; -using System.Text; using System.Text.Json; -using Bit.Core; using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Entities; using Bit.Core.Enums; -using Bit.Core.KeyManagement.Models.Api.Request; using Bit.Core.Services; using Bit.Identity; using Bit.Test.Common.Helpers; @@ -26,7 +23,6 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase public const string DefaultDeviceIdentifier = "92b9d953-b9b6-4eaf-9d3e-11d57144dfeb"; public const string DefaultUserEmail = "DefaultEmail@bitwarden.com"; public const string DefaultUserPasswordHash = "default_password_hash"; - private const string DefaultEncryptedString = "2.3Uk+WNBIoU5xzmVFNcoWzz==|1MsPIYuRfdOHfu/0uY6H2Q==|/98sp4wb6pHP1VTZ9JcNCYgQjEUMFPlqJgCwRk1YXKg="; /// /// A dictionary to store registration tokens for email verification. We cannot substitute the IMailService more than once, so @@ -199,68 +195,6 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase RegisterFinishRequestModel requestModel, bool marketingEmails = true) { - // Ensure required fields for registration finish are present. - // Prefer legacy-path defaults (root fields) to minimize changes to tests. - // PM-28143 - When MasterPasswordAuthenticationData is required, delete all handling of MasterPasswordHash. - requestModel.MasterPasswordHash ??= DefaultUserPasswordHash; - // PM-28143 - When KDF is sourced exclusively from MasterPasswordUnlockData, delete the root Kdf defaults below. - requestModel.Kdf ??= KdfType.PBKDF2_SHA256; - requestModel.KdfIterations ??= AuthConstants.PBKDF2_ITERATIONS.Default; - // Ensure a symmetric key is provided when no unlock data is present - // PM-28143 - When MasterPasswordUnlockData is required, delete the UserSymmetricKey fallback block below. - if (requestModel.MasterPasswordUnlock == null && string.IsNullOrWhiteSpace(requestModel.UserSymmetricKey)) - { - requestModel.UserSymmetricKey = "user_symmetric_key"; - } - - // Align unlock/auth data KDF with root KDF so login uses the provided master password hash. - // PM-28143 - After removing root Kdf fields, build KDF exclusively from MasterPasswordUnlockData.Kdf and delete this alignment section. - var effectiveKdfType = requestModel.Kdf ?? KdfType.PBKDF2_SHA256; - var effectiveIterations = requestModel.KdfIterations ?? AuthConstants.PBKDF2_ITERATIONS.Default; - int? effectiveMemory = null; - int? effectiveParallelism = null; - if (effectiveKdfType == KdfType.Argon2id) - { - effectiveIterations = AuthConstants.ARGON2_ITERATIONS.InsideRange(effectiveIterations) - ? effectiveIterations - : AuthConstants.ARGON2_ITERATIONS.Default; - effectiveMemory = AuthConstants.ARGON2_MEMORY.Default; - effectiveParallelism = AuthConstants.ARGON2_PARALLELISM.Default; - } - - var alignedKdf = new KdfRequestModel - { - KdfType = effectiveKdfType, - Iterations = effectiveIterations, - Memory = effectiveMemory, - Parallelism = effectiveParallelism - }; - - if (requestModel.MasterPasswordUnlock != null) - { - var unlock = requestModel.MasterPasswordUnlock; - // Always force a valid encrypted string for tests to avoid model validation failures. - requestModel.MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel - { - Kdf = alignedKdf, - MasterKeyWrappedUserKey = unlock.MasterKeyWrappedUserKey, - Salt = string.IsNullOrWhiteSpace(unlock.Salt) ? requestModel.Email : unlock.Salt - }; - } - - if (requestModel.MasterPasswordAuthentication != null) - { - // Ensure registration uses the same hash the tests will provide at login. - // PM-28143 - When MasterPasswordAuthenticationData is the only source of the auth hash, - // stop overriding it from MasterPasswordHash and delete this whole reassignment block. - requestModel.MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel - { - Kdf = alignedKdf, - MasterPasswordAuthenticationHash = requestModel.MasterPasswordHash, - Salt = requestModel.Email - }; - } - var sendVerificationEmailReqModel = new RegisterSendVerificationEmailRequestModel { Email = requestModel.Email, @@ -277,11 +211,8 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase requestModel.EmailVerificationToken = RegistrationTokens[requestModel.Email]; var postRegisterFinishHttpContext = await PostRegisterFinishAsync(requestModel); - if (postRegisterFinishHttpContext.Response.StatusCode != StatusCodes.Status200OK) - { - var body = await ReadResponseBodyAsync(postRegisterFinishHttpContext); - Assert.Fail($"register/finish failed (status {postRegisterFinishHttpContext.Response.StatusCode}). Body: {body}"); - } + + Assert.Equal(StatusCodes.Status200OK, postRegisterFinishHttpContext.Response.StatusCode); var database = GetDatabaseContext(); var user = await database.Users @@ -291,32 +222,4 @@ public class IdentityApplicationFactory : WebApplicationFactoryBase return user; } - - private static async Task ReadResponseBodyAsync(HttpContext ctx) - { - try - { - if (ctx?.Response?.Body == null) - { - return ""; - } - var stream = ctx.Response.Body; - if (stream.CanSeek) - { - stream.Seek(0, SeekOrigin.Begin); - } - using var reader = new StreamReader(stream, Encoding.UTF8, detectEncodingFromByteOrderMarks: false, leaveOpen: true); - var text = await reader.ReadToEndAsync(); - if (stream.CanSeek) - { - stream.Seek(0, SeekOrigin.Begin); - } - return string.IsNullOrWhiteSpace(text) ? "" : text; - } - catch (Exception ex) - { - return $""; - } - } - } From 51d90cce3de3bc11bf0b71db91df720ecf622207 Mon Sep 17 00:00:00 2001 From: mkincaid-bw Date: Thu, 15 Jan 2026 13:43:23 -0800 Subject: [PATCH 6/7] Add Entity Framework migration validation to verify_migrations script (#6817) * Add Entity Framework migration validation to verify_migrations script Enhances dev/verify_migrations.ps1 to validate EF migration files in addition to SQL migrations. The script now validates migrations in util/MySqlMigrations, util/PostgresMigrations, and util/SqliteMigrations directories. Validation includes: - Correct naming format (YYYYMMDDHHMMSS_Description.cs) - Both .cs and .Designer.cs files exist as pairs - Chronological ordering of timestamps - Excludes DatabaseContextModelSnapshot.cs files The script provides comprehensive reporting for all migration types with a summary showing which validations passed or failed. Co-Authored-By: Claude Sonnet 4.5 * Fix: Validate all EF migration files instead of silently ignoring malformed names Previously, migration files that didn't match the expected pattern were silently ignored during validation. This could allow incorrectly named files to slip through. Now the script explicitly tracks and reports any migration files that don't match the required YYYYMMDDHHMMSS_Description.cs format, ensuring all new migration files are properly validated. Addresses feedback from PR review to prevent malformed migration files from being overlooked. Co-Authored-By: Claude Sonnet 4.5 --------- Co-authored-by: Claude Sonnet 4.5 --- dev/verify_migrations.ps1 | 320 ++++++++++++++++++++++++++++++++------ 1 file changed, 270 insertions(+), 50 deletions(-) diff --git a/dev/verify_migrations.ps1 b/dev/verify_migrations.ps1 index ad0d34cef1..ce1754e684 100644 --- a/dev/verify_migrations.ps1 +++ b/dev/verify_migrations.ps1 @@ -5,12 +5,19 @@ Validates that new database migration files follow naming conventions and chronological order. .DESCRIPTION - This script validates migration files in util/Migrator/DbScripts/ to ensure: + This script validates migration files to ensure: + + For SQL migrations in util/Migrator/DbScripts/: 1. New migrations follow the naming format: YYYY-MM-DD_NN_Description.sql 2. New migrations are chronologically ordered (filename sorts after existing migrations) 3. Dates use leading zeros (e.g., 2025-01-05, not 2025-1-5) 4. A 2-digit sequence number is included (e.g., _00, _01) + For Entity Framework migrations in util/MySqlMigrations, util/PostgresMigrations, util/SqliteMigrations: + 1. New migrations follow the naming format: YYYYMMDDHHMMSS_Description.cs + 2. Each migration has both .cs and .Designer.cs files + 3. New migrations are chronologically ordered (timestamp sorts after existing migrations) + .PARAMETER BaseRef The base git reference to compare against (e.g., 'main', 'HEAD~1') @@ -58,75 +65,288 @@ $currentMigrations = git ls-tree -r --name-only $CurrentRef -- "$migrationPath/" # Find added migrations $addedMigrations = $currentMigrations | Where-Object { $_ -notin $baseMigrations } +$sqlValidationFailed = $false + if ($addedMigrations.Count -eq 0) { - Write-Host "No new migration files added." - exit 0 + Write-Host "No new SQL migration files added." + Write-Host "" +} +else { + Write-Host "New SQL migration files detected:" + $addedMigrations | ForEach-Object { Write-Host " $_" } + Write-Host "" + + # Get the last migration from base reference + if ($baseMigrations.Count -eq 0) { + Write-Host "No previous SQL migrations found (initial commit?). Skipping chronological validation." + Write-Host "" + } + else { + $lastBaseMigration = Split-Path -Leaf ($baseMigrations | Select-Object -Last 1) + Write-Host "Last SQL migration in base reference: $lastBaseMigration" + Write-Host "" + + # Required format regex: YYYY-MM-DD_NN_Description.sql + $formatRegex = '^[0-9]{4}-[0-9]{2}-[0-9]{2}_[0-9]{2}_.+\.sql$' + + foreach ($migration in $addedMigrations) { + $migrationName = Split-Path -Leaf $migration + + # Validate NEW migration filename format + if ($migrationName -notmatch $formatRegex) { + Write-Host "ERROR: Migration '$migrationName' does not match required format" + Write-Host "Required format: YYYY-MM-DD_NN_Description.sql" + Write-Host " - YYYY: 4-digit year" + Write-Host " - MM: 2-digit month with leading zero (01-12)" + Write-Host " - DD: 2-digit day with leading zero (01-31)" + Write-Host " - NN: 2-digit sequence number (00, 01, 02, etc.)" + Write-Host "Example: 2025-01-15_00_MyMigration.sql" + $sqlValidationFailed = $true + continue + } + + # Compare migration name with last base migration (using ordinal string comparison) + if ([string]::CompareOrdinal($migrationName, $lastBaseMigration) -lt 0) { + Write-Host "ERROR: New migration '$migrationName' is not chronologically after '$lastBaseMigration'" + $sqlValidationFailed = $true + } + else { + Write-Host "OK: '$migrationName' is chronologically after '$lastBaseMigration'" + } + } + + Write-Host "" + } + + if ($sqlValidationFailed) { + Write-Host "FAILED: One or more SQL migrations are incorrectly named or not in chronological order" + Write-Host "" + Write-Host "All new SQL migration files must:" + Write-Host " 1. Follow the naming format: YYYY-MM-DD_NN_Description.sql" + Write-Host " 2. Use leading zeros in dates (e.g., 2025-01-05, not 2025-1-5)" + Write-Host " 3. Include a 2-digit sequence number (e.g., _00, _01)" + Write-Host " 4. Have a filename that sorts after the last migration in base" + Write-Host "" + Write-Host "To fix this issue:" + Write-Host " 1. Locate your migration file(s) in util/Migrator/DbScripts/" + Write-Host " 2. Rename to follow format: YYYY-MM-DD_NN_Description.sql" + Write-Host " 3. Ensure the date is after $lastBaseMigration" + Write-Host "" + Write-Host "Example: 2025-01-15_00_AddNewFeature.sql" + } + else { + Write-Host "SUCCESS: All new SQL migrations are correctly named and in chronological order" + } + + Write-Host "" } -Write-Host "New migration files detected:" -$addedMigrations | ForEach-Object { Write-Host " $_" } +# =========================================================================================== +# Validate Entity Framework Migrations +# =========================================================================================== + +Write-Host "===================================================================" +Write-Host "Validating Entity Framework Migrations" +Write-Host "===================================================================" Write-Host "" -# Get the last migration from base reference -if ($baseMigrations.Count -eq 0) { - Write-Host "No previous migrations found (initial commit?). Skipping validation." - exit 0 -} +$efMigrationPaths = @( + @{ Path = "util/MySqlMigrations/Migrations"; Name = "MySQL" }, + @{ Path = "util/PostgresMigrations/Migrations"; Name = "Postgres" }, + @{ Path = "util/SqliteMigrations/Migrations"; Name = "SQLite" } +) -$lastBaseMigration = Split-Path -Leaf ($baseMigrations | Select-Object -Last 1) -Write-Host "Last migration in base reference: $lastBaseMigration" -Write-Host "" +$efValidationFailed = $false -# Required format regex: YYYY-MM-DD_NN_Description.sql -$formatRegex = '^[0-9]{4}-[0-9]{2}-[0-9]{2}_[0-9]{2}_.+\.sql$' +foreach ($migrationPathInfo in $efMigrationPaths) { + $efPath = $migrationPathInfo.Path + $dbName = $migrationPathInfo.Name -$validationFailed = $false + Write-Host "-------------------------------------------------------------------" + Write-Host "Checking $dbName EF migrations in $efPath" + Write-Host "-------------------------------------------------------------------" + Write-Host "" -foreach ($migration in $addedMigrations) { - $migrationName = Split-Path -Leaf $migration + # Get list of migrations from base reference + try { + $baseMigrations = git ls-tree -r --name-only $BaseRef -- "$efPath/" 2>$null | Where-Object { $_ -like "*.cs" -and $_ -notlike "*DatabaseContextModelSnapshot.cs" } | Sort-Object + if ($LASTEXITCODE -ne 0) { + Write-Host "Warning: Could not retrieve $dbName migrations from base reference '$BaseRef'" + $baseMigrations = @() + } + } + catch { + Write-Host "Warning: Could not retrieve $dbName migrations from base reference '$BaseRef'" + $baseMigrations = @() + } - # Validate NEW migration filename format - if ($migrationName -notmatch $formatRegex) { - Write-Host "ERROR: Migration '$migrationName' does not match required format" - Write-Host "Required format: YYYY-MM-DD_NN_Description.sql" - Write-Host " - YYYY: 4-digit year" - Write-Host " - MM: 2-digit month with leading zero (01-12)" - Write-Host " - DD: 2-digit day with leading zero (01-31)" - Write-Host " - NN: 2-digit sequence number (00, 01, 02, etc.)" - Write-Host "Example: 2025-01-15_00_MyMigration.sql" - $validationFailed = $true + # Get list of migrations from current reference + $currentMigrations = git ls-tree -r --name-only $CurrentRef -- "$efPath/" | Where-Object { $_ -like "*.cs" -and $_ -notlike "*DatabaseContextModelSnapshot.cs" } | Sort-Object + + # Find added migrations + $addedMigrations = $currentMigrations | Where-Object { $_ -notin $baseMigrations } + + if ($addedMigrations.Count -eq 0) { + Write-Host "No new $dbName EF migration files added." + Write-Host "" continue } - # Compare migration name with last base migration (using ordinal string comparison) - if ([string]::CompareOrdinal($migrationName, $lastBaseMigration) -lt 0) { - Write-Host "ERROR: New migration '$migrationName' is not chronologically after '$lastBaseMigration'" - $validationFailed = $true + Write-Host "New $dbName EF migration files detected:" + $addedMigrations | ForEach-Object { Write-Host " $_" } + Write-Host "" + + # Get the last migration from base reference + if ($baseMigrations.Count -eq 0) { + Write-Host "No previous $dbName migrations found. Skipping chronological validation." + Write-Host "" } else { - Write-Host "OK: '$migrationName' is chronologically after '$lastBaseMigration'" + $lastBaseMigration = Split-Path -Leaf ($baseMigrations | Select-Object -Last 1) + Write-Host "Last $dbName migration in base reference: $lastBaseMigration" + Write-Host "" } + + # Required format regex: YYYYMMDDHHMMSS_Description.cs or YYYYMMDDHHMMSS_Description.Designer.cs + $efFormatRegex = '^[0-9]{14}_.+\.cs$' + + # Group migrations by base name (without .Designer.cs suffix) + $migrationGroups = @{} + $unmatchedFiles = @() + + foreach ($migration in $addedMigrations) { + $migrationName = Split-Path -Leaf $migration + + # Extract base name (remove .Designer.cs or .cs) + if ($migrationName -match '^([0-9]{14}_.+?)(?:\.Designer)?\.cs$') { + $baseName = $matches[1] + if (-not $migrationGroups.ContainsKey($baseName)) { + $migrationGroups[$baseName] = @() + } + $migrationGroups[$baseName] += $migrationName + } + else { + # Track files that don't match the expected pattern + $unmatchedFiles += $migrationName + } + } + + # Flag any files that don't match the expected pattern + if ($unmatchedFiles.Count -gt 0) { + Write-Host "ERROR: The following migration files do not match the required format:" + foreach ($unmatchedFile in $unmatchedFiles) { + Write-Host " - $unmatchedFile" + } + Write-Host "" + Write-Host "Required format: YYYYMMDDHHMMSS_Description.cs or YYYYMMDDHHMMSS_Description.Designer.cs" + Write-Host " - YYYYMMDDHHMMSS: 14-digit timestamp (Year, Month, Day, Hour, Minute, Second)" + Write-Host " - Description: Descriptive name using PascalCase" + Write-Host "Example: 20250115120000_AddNewFeature.cs and 20250115120000_AddNewFeature.Designer.cs" + Write-Host "" + $efValidationFailed = $true + } + + foreach ($baseName in $migrationGroups.Keys | Sort-Object) { + $files = $migrationGroups[$baseName] + + # Validate format + $mainFile = "$baseName.cs" + $designerFile = "$baseName.Designer.cs" + + if ($mainFile -notmatch $efFormatRegex) { + Write-Host "ERROR: Migration '$mainFile' does not match required format" + Write-Host "Required format: YYYYMMDDHHMMSS_Description.cs" + Write-Host " - YYYYMMDDHHMMSS: 14-digit timestamp (Year, Month, Day, Hour, Minute, Second)" + Write-Host "Example: 20250115120000_AddNewFeature.cs" + $efValidationFailed = $true + continue + } + + # Check that both .cs and .Designer.cs files exist + $hasCsFile = $files -contains $mainFile + $hasDesignerFile = $files -contains $designerFile + + if (-not $hasCsFile) { + Write-Host "ERROR: Missing main migration file: $mainFile" + $efValidationFailed = $true + } + + if (-not $hasDesignerFile) { + Write-Host "ERROR: Missing designer file: $designerFile" + Write-Host "Each EF migration must have both a .cs and .Designer.cs file" + $efValidationFailed = $true + } + + if (-not $hasCsFile -or -not $hasDesignerFile) { + continue + } + + # Compare migration timestamp with last base migration (using ordinal string comparison) + if ($baseMigrations.Count -gt 0) { + if ([string]::CompareOrdinal($mainFile, $lastBaseMigration) -lt 0) { + Write-Host "ERROR: New migration '$mainFile' is not chronologically after '$lastBaseMigration'" + $efValidationFailed = $true + } + else { + Write-Host "OK: '$mainFile' is chronologically after '$lastBaseMigration'" + } + } + else { + Write-Host "OK: '$mainFile' (no previous migrations to compare)" + } + } + + Write-Host "" +} + +if ($efValidationFailed) { + Write-Host "FAILED: One or more EF migrations are incorrectly named or not in chronological order" + Write-Host "" + Write-Host "All new EF migration files must:" + Write-Host " 1. Follow the naming format: YYYYMMDDHHMMSS_Description.cs" + Write-Host " 2. Include both .cs and .Designer.cs files" + Write-Host " 3. Have a timestamp that sorts after the last migration in base" + Write-Host "" + Write-Host "To fix this issue:" + Write-Host " 1. Locate your migration file(s) in the respective Migrations directory" + Write-Host " 2. Ensure both .cs and .Designer.cs files exist" + Write-Host " 3. Rename to follow format: YYYYMMDDHHMMSS_Description.cs" + Write-Host " 4. Ensure the timestamp is after the last migration" + Write-Host "" + Write-Host "Example: 20250115120000_AddNewFeature.cs and 20250115120000_AddNewFeature.Designer.cs" +} +else { + Write-Host "SUCCESS: All new EF migrations are correctly named and in chronological order" } Write-Host "" +Write-Host "===================================================================" +Write-Host "Validation Summary" +Write-Host "===================================================================" + +if ($sqlValidationFailed -or $efValidationFailed) { + if ($sqlValidationFailed) { + Write-Host "❌ SQL migrations validation FAILED" + } + else { + Write-Host "✓ SQL migrations validation PASSED" + } + + if ($efValidationFailed) { + Write-Host "❌ EF migrations validation FAILED" + } + else { + Write-Host "✓ EF migrations validation PASSED" + } -if ($validationFailed) { - Write-Host "FAILED: One or more migrations are incorrectly named or not in chronological order" Write-Host "" - Write-Host "All new migration files must:" - Write-Host " 1. Follow the naming format: YYYY-MM-DD_NN_Description.sql" - Write-Host " 2. Use leading zeros in dates (e.g., 2025-01-05, not 2025-1-5)" - Write-Host " 3. Include a 2-digit sequence number (e.g., _00, _01)" - Write-Host " 4. Have a filename that sorts after the last migration in base" - Write-Host "" - Write-Host "To fix this issue:" - Write-Host " 1. Locate your migration file(s) in util/Migrator/DbScripts/" - Write-Host " 2. Rename to follow format: YYYY-MM-DD_NN_Description.sql" - Write-Host " 3. Ensure the date is after $lastBaseMigration" - Write-Host "" - Write-Host "Example: 2025-01-15_00_AddNewFeature.sql" + Write-Host "OVERALL RESULT: FAILED" exit 1 } - -Write-Host "SUCCESS: All new migrations are correctly named and in chronological order" -exit 0 +else { + Write-Host "✓ SQL migrations validation PASSED" + Write-Host "✓ EF migrations validation PASSED" + Write-Host "" + Write-Host "OVERALL RESULT: SUCCESS" + exit 0 +} From ebb0712e335385b25a191e3fa4574db9e8caf410 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Fri, 16 Jan 2026 08:49:25 +1000 Subject: [PATCH 7/7] [PM-28555] Add idempotent sproc to create My Items collections (#6801) * Add sproc to create multiple default collections. SqlBulkCopy implementation is overkill for most cases. This provides a lighter weight sproc implementation for smaller data sets. * DRY up collection arrangement * DRY up tests because bulk and non-bulk share same behavior * use EF native AddRange instead of bulk insert, because we expect smaller data sizes on self-host --- .../Collections/CollectionUtils.cs | 53 ++++++++++++ ...maticallyConfirmOrganizationUserCommand.cs | 19 +---- .../ConfirmOrganizationUserCommand.cs | 22 ++--- ...rganizationDataOwnershipPolicyValidator.cs | 5 +- .../Repositories/ICollectionRepository.cs | 17 +++- src/Infrastructure.Dapper/DapperHelpers.cs | 15 ++++ .../Repositories/CollectionRepository.cs | 82 ++++++++++--------- .../Repositories/CollectionRepository.cs | 49 ++--------- .../Repositories/DatabaseContext.cs | 2 - .../Collection_CreateDefaultCollections.sql | 69 ++++++++++++++++ .../AutomaticallyConfirmUsersCommandTests.cs | 21 ++--- .../ConfirmOrganizationUserCommandTests.cs | 29 ++----- ...zationDataOwnershipPolicyValidatorTests.cs | 24 +++--- .../CreateDefaultCollectionsBulkTests.cs | 53 ++++++++++++ ...=> CreateDefaultCollectionsSharedTests.cs} | 62 +++++++------- .../CreateDefaultCollectionsTests.cs | 52 ++++++++++++ ...00_Collection_CreateDefaultCollections.sql | 70 ++++++++++++++++ 17 files changed, 449 insertions(+), 195 deletions(-) create mode 100644 src/Core/AdminConsole/OrganizationFeatures/Collections/CollectionUtils.cs create mode 100644 src/Sql/dbo/AdminConsole/Stored Procedures/Collection_CreateDefaultCollections.sql create mode 100644 test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsBulkTests.cs rename test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/{UpsertDefaultCollectionsTests.cs => CreateDefaultCollectionsSharedTests.cs} (69%) create mode 100644 test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsTests.cs create mode 100644 util/Migrator/DbScripts/2026-01-13_00_Collection_CreateDefaultCollections.sql diff --git a/src/Core/AdminConsole/OrganizationFeatures/Collections/CollectionUtils.cs b/src/Core/AdminConsole/OrganizationFeatures/Collections/CollectionUtils.cs new file mode 100644 index 0000000000..116992146f --- /dev/null +++ b/src/Core/AdminConsole/OrganizationFeatures/Collections/CollectionUtils.cs @@ -0,0 +1,53 @@ +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Utilities; + +namespace Bit.Core.AdminConsole.OrganizationFeatures.Collections; + +public static class CollectionUtils +{ + /// + /// Arranges Collection and CollectionUser objects to create default user collections. + /// + /// The organization ID. + /// The IDs for organization users who need default collections. + /// The encrypted string to use as the default collection name. + /// A tuple containing the collections and collection users. + public static (ICollection collections, ICollection collectionUsers) + BuildDefaultUserCollections(Guid organizationId, IEnumerable organizationUserIds, + string defaultCollectionName) + { + var now = DateTime.UtcNow; + + var collectionUsers = new List(); + var collections = new List(); + + foreach (var orgUserId in organizationUserIds) + { + var collectionId = CoreHelpers.GenerateComb(); + + collections.Add(new Collection + { + Id = collectionId, + OrganizationId = organizationId, + Name = defaultCollectionName, + CreationDate = now, + RevisionDate = now, + Type = CollectionType.DefaultUserCollection, + DefaultUserCollectionEmail = null + + }); + + collectionUsers.Add(new CollectionUser + { + CollectionId = collectionId, + OrganizationUserId = orgUserId, + ReadOnly = false, + HidePasswords = false, + Manage = true, + }); + } + + return (collections, collectionUsers); + } +} diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs index 1b488677ae..0292381857 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs @@ -4,9 +4,7 @@ using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.OrganizationConfirmation; using Bit.Core.AdminConsole.OrganizationFeatures.Policies; using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements; -using Bit.Core.Entities; using Bit.Core.Enums; -using Bit.Core.Models.Data; using Bit.Core.Platform.Push; using Bit.Core.Repositories; using Bit.Core.Services; @@ -83,19 +81,10 @@ public class AutomaticallyConfirmOrganizationUserCommand(IOrganizationUserReposi return; } - await collectionRepository.CreateAsync( - new Collection - { - OrganizationId = request.Organization!.Id, - Name = request.DefaultUserCollectionName, - Type = CollectionType.DefaultUserCollection - }, - groups: null, - [new CollectionAccessSelection - { - Id = request.OrganizationUser!.Id, - Manage = true - }]); + await collectionRepository.CreateDefaultCollectionsAsync( + request.Organization!.Id, + [request.OrganizationUser!.Id], + request.DefaultUserCollectionName); } catch (Exception ex) { diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs index 0b82ac7ea4..02f3346ba6 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs @@ -14,7 +14,6 @@ using Bit.Core.Billing.Enums; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; -using Bit.Core.Models.Data; using Bit.Core.Platform.Push; using Bit.Core.Repositories; using Bit.Core.Services; @@ -294,21 +293,10 @@ public class ConfirmOrganizationUserCommand : IConfirmOrganizationUserCommand return; } - var defaultCollection = new Collection - { - OrganizationId = organizationUser.OrganizationId, - Name = defaultUserCollectionName, - Type = CollectionType.DefaultUserCollection - }; - var collectionUser = new CollectionAccessSelection - { - Id = organizationUser.Id, - ReadOnly = false, - HidePasswords = false, - Manage = true - }; - - await _collectionRepository.CreateAsync(defaultCollection, groups: null, users: [collectionUser]); + await _collectionRepository.CreateDefaultCollectionsAsync( + organizationUser.OrganizationId, + [organizationUser.Id], + defaultUserCollectionName); } /// @@ -339,7 +327,7 @@ public class ConfirmOrganizationUserCommand : IConfirmOrganizationUserCommand return; } - await _collectionRepository.UpsertDefaultCollectionsAsync(organizationId, eligibleOrganizationUserIds, defaultUserCollectionName); + await _collectionRepository.CreateDefaultCollectionsAsync(organizationId, eligibleOrganizationUserIds, defaultUserCollectionName); } /// diff --git a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidator.cs b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidator.cs index 7a47baa65a..104a5751ff 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidator.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidator.cs @@ -57,14 +57,15 @@ public class OrganizationDataOwnershipPolicyValidator( var userOrgIds = requirements .Select(requirement => requirement.GetDefaultCollectionRequestOnPolicyEnable(policyUpdate.OrganizationId)) .Where(request => request.ShouldCreateDefaultCollection) - .Select(request => request.OrganizationUserId); + .Select(request => request.OrganizationUserId) + .ToList(); if (!userOrgIds.Any()) { return; } - await collectionRepository.UpsertDefaultCollectionsAsync( + await collectionRepository.CreateDefaultCollectionsBulkAsync( policyUpdate.OrganizationId, userOrgIds, defaultCollectionName); diff --git a/src/Core/Repositories/ICollectionRepository.cs b/src/Core/Repositories/ICollectionRepository.cs index f86147ca7d..3f3b71d2d5 100644 --- a/src/Core/Repositories/ICollectionRepository.cs +++ b/src/Core/Repositories/ICollectionRepository.cs @@ -64,11 +64,22 @@ public interface ICollectionRepository : IRepository IEnumerable users, IEnumerable groups); /// - /// Creates default user collections for the specified organization users if they do not already have one. + /// Creates default user collections for the specified organization users. + /// Filters internally to only create collections for users who don't already have one. /// /// The Organization ID. /// The Organization User IDs to create default collections for. /// The encrypted string to use as the default collection name. - /// - Task UpsertDefaultCollectionsAsync(Guid organizationId, IEnumerable organizationUserIds, string defaultCollectionName); + Task CreateDefaultCollectionsAsync(Guid organizationId, IEnumerable organizationUserIds, string defaultCollectionName); + + /// + /// Creates default user collections for the specified organization users using bulk insert operations. + /// Use this if you need to create collections for > ~1k users. + /// Filters internally to only create collections for users who don't already have one. + /// + /// The Organization ID. + /// The Organization User IDs to create default collections for. + /// The encrypted string to use as the default collection name. + Task CreateDefaultCollectionsBulkAsync(Guid organizationId, IEnumerable organizationUserIds, string defaultCollectionName); + } diff --git a/src/Infrastructure.Dapper/DapperHelpers.cs b/src/Infrastructure.Dapper/DapperHelpers.cs index 9a119e1e32..4384a6f752 100644 --- a/src/Infrastructure.Dapper/DapperHelpers.cs +++ b/src/Infrastructure.Dapper/DapperHelpers.cs @@ -160,6 +160,21 @@ public static class DapperHelpers return ids.ToArrayTVP("GuidId"); } + public static DataTable ToTwoGuidIdArrayTVP(this IEnumerable<(Guid id1, Guid id2)> values) + { + var table = new DataTable(); + table.SetTypeName("[dbo].[TwoGuidIdArray]"); + table.Columns.Add("Id1", typeof(Guid)); + table.Columns.Add("Id2", typeof(Guid)); + + foreach (var value in values) + { + table.Rows.Add(value.id1, value.id2); + } + + return table; + } + public static DataTable ToArrayTVP(this IEnumerable values, string columnName) { var table = new DataTable(); diff --git a/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs b/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs index 9985b41d56..1531703427 100644 --- a/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs +++ b/src/Infrastructure.Dapper/Repositories/CollectionRepository.cs @@ -1,6 +1,7 @@ using System.Data; using System.Diagnostics.CodeAnalysis; using System.Text.Json; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Models.Data; @@ -360,7 +361,45 @@ public class CollectionRepository : Repository, ICollectionRep } } - public async Task UpsertDefaultCollectionsAsync(Guid organizationId, IEnumerable organizationUserIds, string defaultCollectionName) + public async Task CreateDefaultCollectionsAsync(Guid organizationId, IEnumerable organizationUserIds, string defaultCollectionName) + { + organizationUserIds = organizationUserIds.ToList(); + if (!organizationUserIds.Any()) + { + return; + } + + var organizationUserCollectionIds = organizationUserIds + .Select(ou => (ou, CoreHelpers.GenerateComb())) + .ToTwoGuidIdArrayTVP(); + + await using var connection = new SqlConnection(ConnectionString); + await connection.OpenAsync(); + await using var transaction = connection.BeginTransaction(); + + try + { + await connection.ExecuteAsync( + "[dbo].[Collection_CreateDefaultCollections]", + new + { + OrganizationId = organizationId, + DefaultCollectionName = defaultCollectionName, + OrganizationUserCollectionIds = organizationUserCollectionIds + }, + commandType: CommandType.StoredProcedure, + transaction: transaction); + + await transaction.CommitAsync(); + } + catch + { + await transaction.RollbackAsync(); + throw; + } + } + + public async Task CreateDefaultCollectionsBulkAsync(Guid organizationId, IEnumerable organizationUserIds, string defaultCollectionName) { organizationUserIds = organizationUserIds.ToList(); if (!organizationUserIds.Any()) @@ -377,7 +416,8 @@ public class CollectionRepository : Repository, ICollectionRep var missingDefaultCollectionUserIds = organizationUserIds.Except(orgUserIdWithDefaultCollection); - var (collectionUsers, collections) = BuildDefaultCollectionForUsers(organizationId, missingDefaultCollectionUserIds, defaultCollectionName); + var (collections, collectionUsers) = + CollectionUtils.BuildDefaultUserCollections(organizationId, missingDefaultCollectionUserIds, defaultCollectionName); if (!collectionUsers.Any() || !collections.Any()) { @@ -387,11 +427,11 @@ public class CollectionRepository : Repository, ICollectionRep await BulkResourceCreationService.CreateCollectionsAsync(connection, transaction, collections); await BulkResourceCreationService.CreateCollectionsUsersAsync(connection, transaction, collectionUsers); - transaction.Commit(); + await transaction.CommitAsync(); } catch { - transaction.Rollback(); + await transaction.RollbackAsync(); throw; } } @@ -421,40 +461,6 @@ public class CollectionRepository : Repository, ICollectionRep return organizationUserIds.ToHashSet(); } - private (List collectionUser, List collection) BuildDefaultCollectionForUsers(Guid organizationId, IEnumerable missingDefaultCollectionUserIds, string defaultCollectionName) - { - var collectionUsers = new List(); - var collections = new List(); - - foreach (var orgUserId in missingDefaultCollectionUserIds) - { - var collectionId = CoreHelpers.GenerateComb(); - - collections.Add(new Collection - { - Id = collectionId, - OrganizationId = organizationId, - Name = defaultCollectionName, - CreationDate = DateTime.UtcNow, - RevisionDate = DateTime.UtcNow, - Type = CollectionType.DefaultUserCollection, - DefaultUserCollectionEmail = null - - }); - - collectionUsers.Add(new CollectionUser - { - CollectionId = collectionId, - OrganizationUserId = orgUserId, - ReadOnly = false, - HidePasswords = false, - Manage = true, - }); - } - - return (collectionUsers, collections); - } - public class CollectionWithGroupsAndUsers : Collection { public CollectionWithGroupsAndUsers() { } diff --git a/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs b/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs index 5aa156d1f8..74150246b1 100644 --- a/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs +++ b/src/Infrastructure.EntityFramework/Repositories/CollectionRepository.cs @@ -1,11 +1,10 @@ using AutoMapper; +using Bit.Core.AdminConsole.OrganizationFeatures.Collections; using Bit.Core.Enums; using Bit.Core.Models.Data; using Bit.Core.Repositories; -using Bit.Core.Utilities; using Bit.Infrastructure.EntityFramework.Models; using Bit.Infrastructure.EntityFramework.Repositories.Queries; -using LinqToDB.EntityFrameworkCore; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; @@ -794,7 +793,7 @@ public class CollectionRepository : Repository organizationUserIds, string defaultCollectionName) + public async Task CreateDefaultCollectionsAsync(Guid organizationId, IEnumerable organizationUserIds, string defaultCollectionName) { organizationUserIds = organizationUserIds.ToList(); if (!organizationUserIds.Any()) @@ -808,15 +807,15 @@ public class CollectionRepository : Repository>(collections)); + await dbContext.CollectionUsers.AddRangeAsync(Mapper.Map>(collectionUsers)); await dbContext.SaveChangesAsync(); } @@ -844,37 +843,7 @@ public class CollectionRepository : Repository collectionUser, List collection) BuildDefaultCollectionForUsers(Guid organizationId, IEnumerable missingDefaultCollectionUserIds, string defaultCollectionName) - { - var collectionUsers = new List(); - var collections = new List(); - - foreach (var orgUserId in missingDefaultCollectionUserIds) - { - var collectionId = CoreHelpers.GenerateComb(); - - collections.Add(new Collection - { - Id = collectionId, - OrganizationId = organizationId, - Name = defaultCollectionName, - CreationDate = DateTime.UtcNow, - RevisionDate = DateTime.UtcNow, - Type = CollectionType.DefaultUserCollection, - DefaultUserCollectionEmail = null - - }); - - collectionUsers.Add(new CollectionUser - { - CollectionId = collectionId, - OrganizationUserId = orgUserId, - ReadOnly = false, - HidePasswords = false, - Manage = true, - }); - } - - return (collectionUsers, collections); - } + public Task CreateDefaultCollectionsBulkAsync(Guid organizationId, IEnumerable organizationUserIds, + string defaultCollectionName) => + CreateDefaultCollectionsAsync(organizationId, organizationUserIds, defaultCollectionName); } diff --git a/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs b/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs index 7b67a63912..a0ee0376c0 100644 --- a/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs +++ b/src/Infrastructure.EntityFramework/Repositories/DatabaseContext.cs @@ -17,8 +17,6 @@ using Microsoft.EntityFrameworkCore.Storage.ValueConversion; using DP = Microsoft.AspNetCore.DataProtection; -#nullable enable - namespace Bit.Infrastructure.EntityFramework.Repositories; public class DatabaseContext : DbContext diff --git a/src/Sql/dbo/AdminConsole/Stored Procedures/Collection_CreateDefaultCollections.sql b/src/Sql/dbo/AdminConsole/Stored Procedures/Collection_CreateDefaultCollections.sql new file mode 100644 index 0000000000..4e671bd1e4 --- /dev/null +++ b/src/Sql/dbo/AdminConsole/Stored Procedures/Collection_CreateDefaultCollections.sql @@ -0,0 +1,69 @@ +-- Creates default user collections for organization users +-- Filters out existing default collections at database level +CREATE PROCEDURE [dbo].[Collection_CreateDefaultCollections] + @OrganizationId UNIQUEIDENTIFIER, + @DefaultCollectionName VARCHAR(MAX), + @OrganizationUserCollectionIds AS [dbo].[TwoGuidIdArray] READONLY -- OrganizationUserId, CollectionId +AS +BEGIN + SET NOCOUNT ON + + DECLARE @Now DATETIME2(7) = GETUTCDATE() + + -- Filter to only users who don't have default collections + SELECT ids.Id1, ids.Id2 + INTO #FilteredIds + FROM @OrganizationUserCollectionIds ids + WHERE NOT EXISTS ( + SELECT 1 + FROM [dbo].[CollectionUser] cu + INNER JOIN [dbo].[Collection] c ON c.Id = cu.CollectionId + WHERE c.OrganizationId = @OrganizationId + AND c.[Type] = 1 -- CollectionType.DefaultUserCollection + AND cu.OrganizationUserId = ids.Id1 + ); + + -- Insert collections only for users who don't have default collections yet + INSERT INTO [dbo].[Collection] + ( + [Id], + [OrganizationId], + [Name], + [CreationDate], + [RevisionDate], + [Type], + [ExternalId], + [DefaultUserCollectionEmail] + ) + SELECT + ids.Id2, -- CollectionId + @OrganizationId, + @DefaultCollectionName, + @Now, + @Now, + 1, -- CollectionType.DefaultUserCollection + NULL, + NULL + FROM + #FilteredIds ids; + + -- Insert collection user mappings + INSERT INTO [dbo].[CollectionUser] + ( + [CollectionId], + [OrganizationUserId], + [ReadOnly], + [HidePasswords], + [Manage] + ) + SELECT + ids.Id2, -- CollectionId + ids.Id1, -- OrganizationUserId + 0, -- ReadOnly = false + 0, -- HidePasswords = false + 1 -- Manage = true + FROM + #FilteredIds ids; + + DROP TABLE #FilteredIds; +END diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUsers/AutomaticallyConfirmUsersCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUsers/AutomaticallyConfirmUsersCommandTests.cs index 180750a9d0..252fb89c87 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUsers/AutomaticallyConfirmUsersCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/AutoConfirmUsers/AutomaticallyConfirmUsersCommandTests.cs @@ -10,7 +10,6 @@ using Bit.Core.AdminConsole.Utilities.v2; using Bit.Core.AdminConsole.Utilities.v2.Validation; using Bit.Core.Entities; using Bit.Core.Enums; -using Bit.Core.Models.Data; using Bit.Core.Platform.Push; using Bit.Core.Repositories; using Bit.Core.Services; @@ -204,14 +203,10 @@ public class AutomaticallyConfirmUsersCommandTests await sutProvider.GetDependency() .Received(1) - .CreateAsync( - Arg.Is(c => - c.OrganizationId == organization.Id && - c.Name == defaultCollectionName && - c.Type == CollectionType.DefaultUserCollection), - Arg.Is>(groups => groups == null), - Arg.Is>(access => - access.FirstOrDefault(x => x.Id == organizationUser.Id && x.Manage) != null)); + .CreateDefaultCollectionsAsync( + organization.Id, + Arg.Is>(ids => ids.Single() == organizationUser.Id), + defaultCollectionName); } [Theory] @@ -253,9 +248,7 @@ public class AutomaticallyConfirmUsersCommandTests await sutProvider.GetDependency() .DidNotReceive() - .CreateAsync(Arg.Any(), - Arg.Any>(), - Arg.Any>()); + .CreateDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); } [Theory] @@ -291,9 +284,7 @@ public class AutomaticallyConfirmUsersCommandTests var collectionException = new Exception("Collection creation failed"); sutProvider.GetDependency() - .CreateAsync(Arg.Any(), - Arg.Any>(), - Arg.Any>()) + .CreateDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()) .ThrowsAsync(collectionException); // Act diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommandTests.cs index 65359b8304..6643f26eb5 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommandTests.cs @@ -13,7 +13,6 @@ using Bit.Core.Billing.Enums; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; -using Bit.Core.Models.Data; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Platform.Push; using Bit.Core.Repositories; @@ -493,15 +492,10 @@ public class ConfirmOrganizationUserCommandTests await sutProvider.GetDependency() .Received(1) - .CreateAsync( - Arg.Is(c => - c.Name == collectionName && - c.OrganizationId == organization.Id && - c.Type == CollectionType.DefaultUserCollection), - Arg.Any>(), - Arg.Is>(cu => - cu.Single().Id == orgUser.Id && - cu.Single().Manage)); + .CreateDefaultCollectionsAsync( + organization.Id, + Arg.Is>(ids => ids.Single() == orgUser.Id), + collectionName); } [Theory, BitAutoData] @@ -522,7 +516,7 @@ public class ConfirmOrganizationUserCommandTests await sutProvider.GetDependency() .DidNotReceive() - .UpsertDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); + .CreateDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); } [Theory, BitAutoData] @@ -539,24 +533,15 @@ public class ConfirmOrganizationUserCommandTests sutProvider.GetDependency().GetManyAsync(default).ReturnsForAnyArgs(new[] { orgUser }); sutProvider.GetDependency().GetManyAsync(default).ReturnsForAnyArgs(new[] { user }); - var policyDetails = new PolicyDetails - { - OrganizationId = org.Id, - OrganizationUserId = orgUser.Id, - IsProvider = false, - OrganizationUserStatus = orgUser.Status, - OrganizationUserType = orgUser.Type, - PolicyType = PolicyType.OrganizationDataOwnership - }; sutProvider.GetDependency() .GetAsync(orgUser.UserId!.Value) - .Returns(new OrganizationDataOwnershipPolicyRequirement(OrganizationDataOwnershipState.Disabled, [policyDetails])); + .Returns(new OrganizationDataOwnershipPolicyRequirement(OrganizationDataOwnershipState.Disabled, [])); await sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, collectionName); await sutProvider.GetDependency() .DidNotReceive() - .UpsertDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); + .CreateDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); } [Theory, BitAutoData] diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidatorTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidatorTests.cs index 93cbde89ec..dd2f1d76e8 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidatorTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Policies/PolicyValidators/OrganizationDataOwnershipPolicyValidatorTests.cs @@ -38,7 +38,7 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await sutProvider.GetDependency() .DidNotReceive() - .UpsertDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); + .CreateDefaultCollectionsBulkAsync(Arg.Any(), Arg.Any>(), Arg.Any()); } [Theory, BitAutoData] @@ -60,7 +60,7 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await sutProvider.GetDependency() .DidNotReceive() - .UpsertDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); + .CreateDefaultCollectionsBulkAsync(Arg.Any(), Arg.Any>(), Arg.Any()); } [Theory, BitAutoData] @@ -86,7 +86,7 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await collectionRepository .DidNotReceive() - .UpsertDefaultCollectionsAsync( + .CreateDefaultCollectionsBulkAsync( Arg.Any(), Arg.Any>(), Arg.Any()); @@ -172,10 +172,10 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Act await sut.ExecuteSideEffectsAsync(policyRequest, postUpdatedPolicy, previousPolicyState); - // Assert + // Assert - Should call with all user IDs (repository does internal filtering) await collectionRepository .Received(1) - .UpsertDefaultCollectionsAsync( + .CreateDefaultCollectionsBulkAsync( policyUpdate.OrganizationId, Arg.Is>(ids => ids.Count() == 3), _defaultUserCollectionName); @@ -210,7 +210,7 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await sutProvider.GetDependency() .DidNotReceive() - .UpsertDefaultCollectionsAsync(Arg.Any(), Arg.Any>(), Arg.Any()); + .CreateDefaultCollectionsBulkAsync(Arg.Any(), Arg.Any>(), Arg.Any()); } private static IPolicyRepository ArrangePolicyRepository(IEnumerable policyDetails) @@ -251,7 +251,7 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await sutProvider.GetDependency() .DidNotReceiveWithAnyArgs() - .UpsertDefaultCollectionsAsync(default, default, default); + .CreateDefaultCollectionsBulkAsync(default, default, default); } [Theory, BitAutoData] @@ -273,7 +273,7 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await sutProvider.GetDependency() .DidNotReceiveWithAnyArgs() - .UpsertDefaultCollectionsAsync(default, default, default); + .CreateDefaultCollectionsBulkAsync(default, default, default); } [Theory, BitAutoData] @@ -299,7 +299,7 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await collectionRepository .DidNotReceiveWithAnyArgs() - .UpsertDefaultCollectionsAsync( + .CreateDefaultCollectionsBulkAsync( default, default, default); @@ -336,10 +336,10 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Act await sut.ExecutePostUpsertSideEffectAsync(policyRequest, postUpdatedPolicy, previousPolicyState); - // Assert + // Assert - Should call with all user IDs (repository does internal filtering) await collectionRepository .Received(1) - .UpsertDefaultCollectionsAsync( + .CreateDefaultCollectionsBulkAsync( policyUpdate.OrganizationId, Arg.Is>(ids => ids.Count() == 3), _defaultUserCollectionName); @@ -367,6 +367,6 @@ public class OrganizationDataOwnershipPolicyValidatorTests // Assert await sutProvider.GetDependency() .DidNotReceiveWithAnyArgs() - .UpsertDefaultCollectionsAsync(default, default, default); + .CreateDefaultCollectionsBulkAsync(default, default, default); } } diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsBulkTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsBulkTests.cs new file mode 100644 index 0000000000..712ad7d62e --- /dev/null +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsBulkTests.cs @@ -0,0 +1,53 @@ +using Bit.Core.Repositories; +using Xunit; + +namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.CollectionRepository; + + +public class CreateDefaultCollectionsBulkAsyncTests +{ + [Theory, DatabaseData] + public async Task CreateDefaultCollectionsBulkAsync_CreatesDefaultCollections_Success( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + await CreateDefaultCollectionsSharedTests.CreatesDefaultCollections_Success( + collectionRepository.CreateDefaultCollectionsBulkAsync, + organizationRepository, + userRepository, + organizationUserRepository, + collectionRepository); + } + + [Theory, DatabaseData] + public async Task CreateDefaultCollectionsBulkAsync_CreatesForNewUsersOnly_AndIgnoresExistingUsers( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + await CreateDefaultCollectionsSharedTests.CreatesForNewUsersOnly_AndIgnoresExistingUsers( + collectionRepository.CreateDefaultCollectionsBulkAsync, + organizationRepository, + userRepository, + organizationUserRepository, + collectionRepository); + } + + [Theory, DatabaseData] + public async Task CreateDefaultCollectionsBulkAsync_IgnoresAllExistingUsers( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + await CreateDefaultCollectionsSharedTests.IgnoresAllExistingUsers( + collectionRepository.CreateDefaultCollectionsBulkAsync, + organizationRepository, + userRepository, + organizationUserRepository, + collectionRepository); + } +} diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/UpsertDefaultCollectionsTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsSharedTests.cs similarity index 69% rename from test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/UpsertDefaultCollectionsTests.cs rename to test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsSharedTests.cs index 64dffa473f..0fb4a5b446 100644 --- a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/UpsertDefaultCollectionsTests.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsSharedTests.cs @@ -6,10 +6,14 @@ using Xunit; namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.CollectionRepository; -public class UpsertDefaultCollectionsTests +/// +/// Shared tests for CreateDefaultCollections methods - both bulk and non-bulk implementations, +/// as they share the same behavior. Both test suites call the tests in this class. +/// +public static class CreateDefaultCollectionsSharedTests { - [Theory, DatabaseData] - public async Task UpsertDefaultCollectionsAsync_ShouldCreateDefaultCollection_WhenUsersDoNotHaveDefaultCollection( + public static async Task CreatesDefaultCollections_Success( + Func, string, Task> createDefaultCollectionsFunc, IOrganizationRepository organizationRepository, IUserRepository userRepository, IOrganizationUserRepository organizationUserRepository, @@ -21,14 +25,13 @@ public class UpsertDefaultCollectionsTests var resultOrganizationUsers = await Task.WhenAll( CreateUserForOrgAsync(userRepository, organizationUserRepository, organization), CreateUserForOrgAsync(userRepository, organizationUserRepository, organization) - ); + ); - - var affectedOrgUserIds = resultOrganizationUsers.Select(organizationUser => organizationUser.Id); + var affectedOrgUserIds = resultOrganizationUsers.Select(organizationUser => organizationUser.Id).ToList(); var defaultCollectionName = $"default-name-{organization.Id}"; // Act - await collectionRepository.UpsertDefaultCollectionsAsync(organization.Id, affectedOrgUserIds, defaultCollectionName); + await createDefaultCollectionsFunc(organization.Id, affectedOrgUserIds, defaultCollectionName); // Assert await AssertAllUsersHaveOneDefaultCollectionAsync(collectionRepository, resultOrganizationUsers, organization.Id); @@ -36,8 +39,8 @@ public class UpsertDefaultCollectionsTests await CleanupAsync(organizationRepository, userRepository, organization, resultOrganizationUsers); } - [Theory, DatabaseData] - public async Task UpsertDefaultCollectionsAsync_ShouldUpsertCreateDefaultCollection_ForUsersWithAndWithoutDefaultCollectionsExist( + public static async Task CreatesForNewUsersOnly_AndIgnoresExistingUsers( + Func, string, Task> createDefaultCollectionsFunc, IOrganizationRepository organizationRepository, IUserRepository userRepository, IOrganizationUserRepository organizationUserRepository, @@ -51,31 +54,30 @@ public class UpsertDefaultCollectionsTests CreateUserForOrgAsync(userRepository, organizationUserRepository, organization) ); - var arrangedOrgUserIds = arrangedOrganizationUsers.Select(organizationUser => organizationUser.Id); + var arrangedOrgUserIds = arrangedOrganizationUsers.Select(organizationUser => organizationUser.Id).ToList(); var defaultCollectionName = $"default-name-{organization.Id}"; + await CreateUsersWithExistingDefaultCollectionsAsync(createDefaultCollectionsFunc, collectionRepository, organization.Id, arrangedOrgUserIds, defaultCollectionName, arrangedOrganizationUsers); - await CreateUsersWithExistingDefaultCollectionsAsync(collectionRepository, organization.Id, arrangedOrgUserIds, defaultCollectionName, arrangedOrganizationUsers); - - var newOrganizationUsers = new List() + var newOrganizationUsers = new List { await CreateUserForOrgAsync(userRepository, organizationUserRepository, organization) }; - var affectedOrgUsers = newOrganizationUsers.Concat(arrangedOrganizationUsers); - var affectedOrgUserIds = affectedOrgUsers.Select(organizationUser => organizationUser.Id); + var affectedOrgUsers = newOrganizationUsers.Concat(arrangedOrganizationUsers).ToList(); + var affectedOrgUserIds = affectedOrgUsers.Select(organizationUser => organizationUser.Id).ToList(); // Act - await collectionRepository.UpsertDefaultCollectionsAsync(organization.Id, affectedOrgUserIds, defaultCollectionName); + await createDefaultCollectionsFunc(organization.Id, affectedOrgUserIds, defaultCollectionName); // Assert - await AssertAllUsersHaveOneDefaultCollectionAsync(collectionRepository, arrangedOrganizationUsers, organization.Id); + await AssertAllUsersHaveOneDefaultCollectionAsync(collectionRepository, affectedOrgUsers, organization.Id); await CleanupAsync(organizationRepository, userRepository, organization, affectedOrgUsers); } - [Theory, DatabaseData] - public async Task UpsertDefaultCollectionsAsync_ShouldNotCreateDefaultCollection_WhenUsersAlreadyHaveOne( + public static async Task IgnoresAllExistingUsers( + Func, string, Task> createDefaultCollectionsFunc, IOrganizationRepository organizationRepository, IUserRepository userRepository, IOrganizationUserRepository organizationUserRepository, @@ -89,26 +91,29 @@ public class UpsertDefaultCollectionsTests CreateUserForOrgAsync(userRepository, organizationUserRepository, organization) ); - var affectedOrgUserIds = resultOrganizationUsers.Select(organizationUser => organizationUser.Id); + var affectedOrgUserIds = resultOrganizationUsers.Select(organizationUser => organizationUser.Id).ToList(); var defaultCollectionName = $"default-name-{organization.Id}"; + await CreateUsersWithExistingDefaultCollectionsAsync(createDefaultCollectionsFunc, collectionRepository, organization.Id, affectedOrgUserIds, defaultCollectionName, resultOrganizationUsers); - await CreateUsersWithExistingDefaultCollectionsAsync(collectionRepository, organization.Id, affectedOrgUserIds, defaultCollectionName, resultOrganizationUsers); + // Act - Try to create again, should silently filter and not create duplicates + await createDefaultCollectionsFunc(organization.Id, affectedOrgUserIds, defaultCollectionName); - // Act - await collectionRepository.UpsertDefaultCollectionsAsync(organization.Id, affectedOrgUserIds, defaultCollectionName); - - // Assert + // Assert - Original collections should remain unchanged, still only one per user await AssertAllUsersHaveOneDefaultCollectionAsync(collectionRepository, resultOrganizationUsers, organization.Id); await CleanupAsync(organizationRepository, userRepository, organization, resultOrganizationUsers); } - private static async Task CreateUsersWithExistingDefaultCollectionsAsync(ICollectionRepository collectionRepository, - Guid organizationId, IEnumerable affectedOrgUserIds, string defaultCollectionName, + private static async Task CreateUsersWithExistingDefaultCollectionsAsync( + Func, string, Task> createDefaultCollectionsFunc, + ICollectionRepository collectionRepository, + Guid organizationId, + IEnumerable affectedOrgUserIds, + string defaultCollectionName, OrganizationUser[] resultOrganizationUsers) { - await collectionRepository.UpsertDefaultCollectionsAsync(organizationId, affectedOrgUserIds, defaultCollectionName); + await createDefaultCollectionsFunc(organizationId, affectedOrgUserIds, defaultCollectionName); await AssertAllUsersHaveOneDefaultCollectionAsync(collectionRepository, resultOrganizationUsers, organizationId); } @@ -131,7 +136,6 @@ public class UpsertDefaultCollectionsTests private static async Task CreateUserForOrgAsync(IUserRepository userRepository, IOrganizationUserRepository organizationUserRepository, Organization organization) { - var user = await userRepository.CreateTestUserAsync(); var orgUser = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user); diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsTests.cs new file mode 100644 index 0000000000..bd894e9ca5 --- /dev/null +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CreateDefaultCollectionsTests.cs @@ -0,0 +1,52 @@ +using Bit.Core.Repositories; +using Xunit; + +namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.CollectionRepository; + +public class CreateDefaultCollectionsAsyncTests +{ + [Theory, DatabaseData] + public async Task CreateDefaultCollectionsAsync_CreatesDefaultCollections_Success( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + await CreateDefaultCollectionsSharedTests.CreatesDefaultCollections_Success( + collectionRepository.CreateDefaultCollectionsAsync, + organizationRepository, + userRepository, + organizationUserRepository, + collectionRepository); + } + + [Theory, DatabaseData] + public async Task CreateDefaultCollectionsAsync_CreatesForNewUsersOnly_AndIgnoresExistingUsers( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + await CreateDefaultCollectionsSharedTests.CreatesForNewUsersOnly_AndIgnoresExistingUsers( + collectionRepository.CreateDefaultCollectionsAsync, + organizationRepository, + userRepository, + organizationUserRepository, + collectionRepository); + } + + [Theory, DatabaseData] + public async Task CreateDefaultCollectionsAsync_IgnoresAllExistingUsers( + IOrganizationRepository organizationRepository, + IUserRepository userRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + await CreateDefaultCollectionsSharedTests.IgnoresAllExistingUsers( + collectionRepository.CreateDefaultCollectionsAsync, + organizationRepository, + userRepository, + organizationUserRepository, + collectionRepository); + } +} diff --git a/util/Migrator/DbScripts/2026-01-13_00_Collection_CreateDefaultCollections.sql b/util/Migrator/DbScripts/2026-01-13_00_Collection_CreateDefaultCollections.sql new file mode 100644 index 0000000000..c7935db5e8 --- /dev/null +++ b/util/Migrator/DbScripts/2026-01-13_00_Collection_CreateDefaultCollections.sql @@ -0,0 +1,70 @@ +-- Creates default user collections for organization users +-- Filters out existing default collections at database level +CREATE OR ALTER PROCEDURE [dbo].[Collection_CreateDefaultCollections] + @OrganizationId UNIQUEIDENTIFIER, + @DefaultCollectionName VARCHAR(MAX), + @OrganizationUserCollectionIds AS [dbo].[TwoGuidIdArray] READONLY -- OrganizationUserId, CollectionId +AS +BEGIN + SET NOCOUNT ON + + DECLARE @Now DATETIME2(7) = GETUTCDATE() + + -- Filter to only users who don't have default collections + SELECT ids.Id1, ids.Id2 + INTO #FilteredIds + FROM @OrganizationUserCollectionIds ids + WHERE NOT EXISTS ( + SELECT 1 + FROM [dbo].[CollectionUser] cu + INNER JOIN [dbo].[Collection] c ON c.Id = cu.CollectionId + WHERE c.OrganizationId = @OrganizationId + AND c.[Type] = 1 -- CollectionType.DefaultUserCollection + AND cu.OrganizationUserId = ids.Id1 + ); + + -- Insert collections only for users who don't have default collections yet + INSERT INTO [dbo].[Collection] + ( + [Id], + [OrganizationId], + [Name], + [CreationDate], + [RevisionDate], + [Type], + [ExternalId], + [DefaultUserCollectionEmail] + ) + SELECT + ids.Id2, -- CollectionId + @OrganizationId, + @DefaultCollectionName, + @Now, + @Now, + 1, -- CollectionType.DefaultUserCollection + NULL, + NULL + FROM + #FilteredIds ids; + + -- Insert collection user mappings + INSERT INTO [dbo].[CollectionUser] + ( + [CollectionId], + [OrganizationUserId], + [ReadOnly], + [HidePasswords], + [Manage] + ) + SELECT + ids.Id2, -- CollectionId + ids.Id1, -- OrganizationUserId + 0, -- ReadOnly = false + 0, -- HidePasswords = false + 1 -- Manage = true + FROM + #FilteredIds ids; + + DROP TABLE #FilteredIds; +END +GO