From 328b84eea0f93d5ede787509ccf5f859594fb0c4 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Thu, 22 May 2025 12:49:14 -0700 Subject: [PATCH 1/4] Add-userid-to-encryption-methods (#5838) * Add userId to auth success response * Validate user that encrypted a cipher matches the user posting the request * Remove userId from auth success we don't want to expand this response model --- .../Vault/Controllers/CiphersController.cs | 69 +++++++++++++++++++ .../Models/Request/CipherRequestModel.cs | 4 ++ 2 files changed, 73 insertions(+) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 4f105128ea..251362589e 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -151,6 +151,16 @@ public class CiphersController : Controller public async Task Post([FromBody] CipherRequestModel model) { var user = await _userService.GetUserByPrincipalAsync(User); + + // Validate the model was encrypted for the posting user + if (model.EncryptedFor != null) + { + if (model.EncryptedFor != user.Id) + { + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + var cipher = model.ToCipherDetails(user.Id); if (cipher.OrganizationId.HasValue && !await _currentContext.OrganizationUser(cipher.OrganizationId.Value)) { @@ -170,6 +180,16 @@ public class CiphersController : Controller public async Task PostCreate([FromBody] CipherCreateRequestModel model) { var user = await _userService.GetUserByPrincipalAsync(User); + + // Validate the model was encrypted for the posting user + if (model.Cipher.EncryptedFor != null) + { + if (model.Cipher.EncryptedFor != user.Id) + { + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + var cipher = model.Cipher.ToCipherDetails(user.Id); if (cipher.OrganizationId.HasValue && !await _currentContext.OrganizationUser(cipher.OrganizationId.Value)) { @@ -192,6 +212,16 @@ public class CiphersController : Controller } var userId = _userService.GetProperUserId(User).Value; + + // Validate the model was encrypted for the posting user + if (model.Cipher.EncryptedFor != null) + { + if (model.Cipher.EncryptedFor != userId) + { + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + await _cipherService.SaveAsync(cipher, userId, model.Cipher.LastKnownRevisionDate, model.CollectionIds, true, false); var response = new CipherMiniResponseModel(cipher, _globalSettings, false); @@ -209,6 +239,15 @@ public class CiphersController : Controller throw new NotFoundException(); } + // Validate the model was encrypted for the posting user + if (model.EncryptedFor != null) + { + if (model.EncryptedFor != user.Id) + { + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + ValidateClientVersionForFido2CredentialSupport(cipher); var collectionIds = (await _collectionCipherRepository.GetManyByUserIdCipherIdAsync(user.Id, id)).Select(c => c.CollectionId).ToList(); @@ -237,6 +276,15 @@ public class CiphersController : Controller var userId = _userService.GetProperUserId(User).Value; var cipher = await _cipherRepository.GetOrganizationDetailsByIdAsync(id); + // Validate the model was encrypted for the posting user + if (model.EncryptedFor != null) + { + if (model.EncryptedFor != userId) + { + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + ValidateClientVersionForFido2CredentialSupport(cipher); if (cipher == null || !cipher.OrganizationId.HasValue || @@ -658,6 +706,15 @@ public class CiphersController : Controller throw new NotFoundException(); } + // Validate the model was encrypted for the posting user + if (model.Cipher.EncryptedFor != null) + { + if (model.Cipher.EncryptedFor != user.Id) + { + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + ValidateClientVersionForFido2CredentialSupport(cipher); var original = cipher.Clone(); @@ -1019,6 +1076,18 @@ public class CiphersController : Controller var ciphers = await _cipherRepository.GetManyByUserIdAsync(userId, withOrganizations: false); var ciphersDict = ciphers.ToDictionary(c => c.Id); + // Validate the model was encrypted for the posting user + foreach (var cipher in model.Ciphers) + { + if (cipher.EncryptedFor != null) + { + if (cipher.EncryptedFor != userId) + { + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + } + var shareCiphers = new List<(Cipher, DateTime?)>(); foreach (var cipher in model.Ciphers) { diff --git a/src/Api/Vault/Models/Request/CipherRequestModel.cs b/src/Api/Vault/Models/Request/CipherRequestModel.cs index 89eda415b1..5c288ab66d 100644 --- a/src/Api/Vault/Models/Request/CipherRequestModel.cs +++ b/src/Api/Vault/Models/Request/CipherRequestModel.cs @@ -11,6 +11,10 @@ namespace Bit.Api.Vault.Models.Request; public class CipherRequestModel { + /// + /// The Id of the user that encrypted the cipher. It should always represent a UserId. + /// + public Guid? EncryptedFor { get; set; } public CipherType Type { get; set; } [StringLength(36)] From 83478f9c698411da1d4ea539a0a3303775530f4d Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Fri, 23 May 2025 11:27:37 +1000 Subject: [PATCH 2/4] [PM-13274] [Unified] Add integration tests for creating and updating collections (#5814) --- .../AdminConsole/OrganizationTestHelpers.cs | 11 +- .../CollectionRepositoryCreateTests.cs | 105 +++++++++++++ .../CollectionRepositoryReplaceTests.cs | 147 ++++++++++++++++++ .../CollectionRepositoryTests.cs | 145 +---------------- 4 files changed, 263 insertions(+), 145 deletions(-) create mode 100644 test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryCreateTests.cs create mode 100644 test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryReplaceTests.cs rename test/Infrastructure.IntegrationTest/{Vault/Repositories => AdminConsole/Repositories/CollectionRepository}/CollectionRepositoryTests.cs (76%) diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs b/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs index e631280bb3..144bff9dcb 100644 --- a/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs @@ -1,5 +1,6 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Billing.Enums; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Repositories; @@ -26,15 +27,23 @@ public static class OrganizationTestHelpers }); } + /// + /// Creates an Enterprise organization. + /// public static Task CreateTestOrganizationAsync(this IOrganizationRepository organizationRepository, string identifier = "test") => organizationRepository.CreateAsync(new Organization { Name = $"{identifier}-{Guid.NewGuid()}", BillingEmail = "billing@example.com", // TODO: EF does not enforce this being NOT NULL - Plan = "Test", // TODO: EF does not enforce this being NOT NULl + Plan = "Enterprise (Annually)", // TODO: EF does not enforce this being NOT NULl + PlanType = PlanType.EnterpriseAnnually }); + /// + /// Creates a confirmed Owner for the specified organization and user. + /// Does not include any cryptographic material. + /// public static Task CreateTestOrganizationUserAsync( this IOrganizationUserRepository organizationUserRepository, Organization organization, diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryCreateTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryCreateTests.cs new file mode 100644 index 0000000000..8a51f201dc --- /dev/null +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryCreateTests.cs @@ -0,0 +1,105 @@ +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Entities; +using Bit.Core.Models.Data; +using Bit.Core.Repositories; +using Xunit; + +namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.CollectionRepository; + +public class CollectionRepositoryCreateTests +{ + [DatabaseTheory, DatabaseData] + public async Task CreateAsync_WithAccess_Works( + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + IGroupRepository groupRepository, + ICollectionRepository collectionRepository) + { + // Arrange + var organization = await organizationRepository.CreateTestOrganizationAsync(); + + var user1 = await userRepository.CreateTestUserAsync(); + var orgUser1 = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user1); + + var user2 = await userRepository.CreateTestUserAsync(); + var orgUser2 = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user2); + + var group1 = await groupRepository.CreateTestGroupAsync(organization); + var group2 = await groupRepository.CreateTestGroupAsync(organization); + + var collection = new Collection + { + Name = "Test Collection Name", + OrganizationId = organization.Id, + }; + + // Act + await collectionRepository.CreateAsync(collection, + [ + new CollectionAccessSelection { Id = group1.Id, Manage = true, HidePasswords = true, ReadOnly = false, }, + new CollectionAccessSelection { Id = group2.Id, Manage = false, HidePasswords = false, ReadOnly = true, }, + ], + [ + new CollectionAccessSelection { Id = orgUser1.Id, Manage = true, HidePasswords = false, ReadOnly = true }, + new CollectionAccessSelection { Id = orgUser2.Id, Manage = false, HidePasswords = true, ReadOnly = false }, + ] + ); + + // Assert + var (actualCollection, actualAccess) = await collectionRepository.GetByIdWithAccessAsync(collection.Id); + + Assert.NotNull(actualCollection); + Assert.Equal("Test Collection Name", actualCollection.Name); + + var groups = actualAccess.Groups.ToArray(); + Assert.Equal(2, groups.Length); + Assert.Single(groups, g => g.Id == group1.Id && g.Manage && g.HidePasswords && !g.ReadOnly); + Assert.Single(groups, g => g.Id == group2.Id && !g.Manage && !g.HidePasswords && g.ReadOnly); + + var users = actualAccess.Users.ToArray(); + Assert.Equal(2, users.Length); + Assert.Single(users, u => u.Id == orgUser1.Id && u.Manage && !u.HidePasswords && u.ReadOnly); + Assert.Single(users, u => u.Id == orgUser2.Id && !u.Manage && u.HidePasswords && !u.ReadOnly); + + // Clean up data + await userRepository.DeleteAsync(user1); + await userRepository.DeleteAsync(user2); + await organizationRepository.DeleteAsync(organization); + await groupRepository.DeleteManyAsync([group1.Id, group2.Id]); + await organizationUserRepository.DeleteManyAsync([orgUser1.Id, orgUser2.Id]); + } + + /// + /// Makes sure that the sproc handles empty sets. + /// + [DatabaseTheory, DatabaseData] + public async Task CreateAsync_WithNoAccess_Works( + IOrganizationRepository organizationRepository, + ICollectionRepository collectionRepository) + { + // Arrange + var organization = await organizationRepository.CreateTestOrganizationAsync(); + + var collection = new Collection + { + Name = "Test Collection Name", + OrganizationId = organization.Id, + }; + + // Act + await collectionRepository.CreateAsync(collection, [], []); + + // Assert + var (actualCollection, actualAccess) = await collectionRepository.GetByIdWithAccessAsync(collection.Id); + + Assert.NotNull(actualCollection); + Assert.Equal("Test Collection Name", actualCollection.Name); + + Assert.Empty(actualAccess.Groups); + Assert.Empty(actualAccess.Users); + + // Clean up + await organizationRepository.DeleteAsync(organization); + } +} diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryReplaceTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryReplaceTests.cs new file mode 100644 index 0000000000..df01276493 --- /dev/null +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryReplaceTests.cs @@ -0,0 +1,147 @@ +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Entities; +using Bit.Core.Models.Data; +using Bit.Core.Repositories; +using Xunit; + +namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.CollectionRepository; + +public class CollectionRepositoryReplaceTests +{ + [DatabaseTheory, DatabaseData] + public async Task ReplaceAsync_WithAccess_Works( + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + IGroupRepository groupRepository, + ICollectionRepository collectionRepository) + { + // Arrange + var organization = await organizationRepository.CreateTestOrganizationAsync(); + + var user1 = await userRepository.CreateTestUserAsync(); + var orgUser1 = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user1); + + var user2 = await userRepository.CreateTestUserAsync(); + var orgUser2 = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user2); + + var user3 = await userRepository.CreateTestUserAsync(); + var orgUser3 = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user3); + + var group1 = await groupRepository.CreateTestGroupAsync(organization); + var group2 = await groupRepository.CreateTestGroupAsync(organization); + var group3 = await groupRepository.CreateTestGroupAsync(organization); + + var collection = new Collection + { + Name = "Test Collection Name", + OrganizationId = organization.Id, + }; + + await collectionRepository.CreateAsync(collection, + [ + new CollectionAccessSelection { Id = group1.Id, Manage = true, HidePasswords = true, ReadOnly = false, }, + new CollectionAccessSelection { Id = group2.Id, Manage = false, HidePasswords = false, ReadOnly = true, }, + ], + [ + new CollectionAccessSelection { Id = orgUser1.Id, Manage = true, HidePasswords = false, ReadOnly = true }, + new CollectionAccessSelection { Id = orgUser2.Id, Manage = false, HidePasswords = true, ReadOnly = false }, + ] + ); + + // Act + collection.Name = "Updated Collection Name"; + + await collectionRepository.ReplaceAsync(collection, + [ + // Delete group1 + // Update group2: + new CollectionAccessSelection { Id = group2.Id, Manage = true, HidePasswords = true, ReadOnly = false, }, + // Add group3: + new CollectionAccessSelection { Id = group3.Id, Manage = false, HidePasswords = false, ReadOnly = true, }, + ], + [ + // Delete orgUser1 + // Update orgUser2: + new CollectionAccessSelection { Id = orgUser2.Id, Manage = false, HidePasswords = false, ReadOnly = true }, + // Add orgUser3: + new CollectionAccessSelection { Id = orgUser3.Id, Manage = true, HidePasswords = false, ReadOnly = true }, + ] + ); + + // Assert + var (actualCollection, actualAccess) = await collectionRepository.GetByIdWithAccessAsync(collection.Id); + + Assert.NotNull(actualCollection); + Assert.Equal("Updated Collection Name", actualCollection.Name); + + var groups = actualAccess.Groups.ToArray(); + Assert.Equal(2, groups.Length); + Assert.Single(groups, g => g.Id == group2.Id && g.Manage && g.HidePasswords && !g.ReadOnly); + Assert.Single(groups, g => g.Id == group3.Id && !g.Manage && !g.HidePasswords && g.ReadOnly); + + var users = actualAccess.Users.ToArray(); + + Assert.Equal(2, users.Length); + Assert.Single(users, u => u.Id == orgUser2.Id && !u.Manage && !u.HidePasswords && u.ReadOnly); + Assert.Single(users, u => u.Id == orgUser3.Id && u.Manage && !u.HidePasswords && u.ReadOnly); + + // Clean up data + await userRepository.DeleteAsync(user1); + await userRepository.DeleteAsync(user2); + await userRepository.DeleteAsync(user3); + await organizationRepository.DeleteAsync(organization); + } + + /// + /// Makes sure that the sproc handles empty sets. + /// + [DatabaseTheory, DatabaseData] + public async Task ReplaceAsync_WithNoAccess_Works( + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + IGroupRepository groupRepository, + ICollectionRepository collectionRepository) + { + // Arrange + var organization = await organizationRepository.CreateTestOrganizationAsync(); + + var user = await userRepository.CreateTestUserAsync(); + var orgUser = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user); + + var group = await groupRepository.CreateTestGroupAsync(organization); + + var collection = new Collection + { + Name = "Test Collection Name", + OrganizationId = organization.Id, + }; + + await collectionRepository.CreateAsync(collection, + [ + new CollectionAccessSelection { Id = group.Id, Manage = true, HidePasswords = false, ReadOnly = true }, + ], + [ + new CollectionAccessSelection { Id = orgUser.Id, Manage = true, HidePasswords = false, ReadOnly = true }, + ]); + + // Act + collection.Name = "Updated Collection Name"; + + await collectionRepository.ReplaceAsync(collection, [], []); + + // Assert + var (actualCollection, actualAccess) = await collectionRepository.GetByIdWithAccessAsync(collection.Id); + + Assert.NotNull(actualCollection); + Assert.Equal("Updated Collection Name", actualCollection.Name); + + Assert.Empty(actualAccess.Groups); + Assert.Empty(actualAccess.Users); + + // Clean up + await userRepository.DeleteAsync(user); + await organizationRepository.DeleteAsync(organization); + } +} diff --git a/test/Infrastructure.IntegrationTest/Vault/Repositories/CollectionRepositoryTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs similarity index 76% rename from test/Infrastructure.IntegrationTest/Vault/Repositories/CollectionRepositoryTests.cs rename to test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs index 268d46ef6b..b96998415d 100644 --- a/test/Infrastructure.IntegrationTest/Vault/Repositories/CollectionRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/CollectionRepository/CollectionRepositoryTests.cs @@ -7,7 +7,7 @@ using Bit.Core.Models.Data; using Bit.Core.Repositories; using Xunit; -namespace Bit.Infrastructure.IntegrationTest.Repositories; +namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.CollectionRepository; public class CollectionRepositoryTests { @@ -463,147 +463,4 @@ public class CollectionRepositoryTests Assert.False(c3.Unmanaged); }); } - - [DatabaseTheory, DatabaseData] - public async Task ReplaceAsync_Works( - IUserRepository userRepository, - IOrganizationRepository organizationRepository, - IOrganizationUserRepository organizationUserRepository, - IGroupRepository groupRepository, - ICollectionRepository collectionRepository) - { - var user = await userRepository.CreateAsync(new User - { - Name = "Test User", - Email = $"test+{Guid.NewGuid()}@email.com", - ApiKey = "TEST", - SecurityStamp = "stamp", - }); - - var organization = await organizationRepository.CreateAsync(new Organization - { - Name = "Test Org", - PlanType = PlanType.EnterpriseAnnually, - Plan = "Test Plan", - BillingEmail = "billing@email.com" - }); - - var orgUser1 = await organizationUserRepository.CreateAsync(new OrganizationUser - { - OrganizationId = organization.Id, - UserId = user.Id, - Status = OrganizationUserStatusType.Confirmed, - }); - - var orgUser2 = await organizationUserRepository.CreateAsync(new OrganizationUser - { - OrganizationId = organization.Id, - UserId = user.Id, - Status = OrganizationUserStatusType.Confirmed, - }); - - var orgUser3 = await organizationUserRepository.CreateAsync(new OrganizationUser - { - OrganizationId = organization.Id, - UserId = user.Id, - Status = OrganizationUserStatusType.Confirmed, - }); - - var group1 = await groupRepository.CreateAsync(new Group - { - Name = "Test Group #1", - OrganizationId = organization.Id, - }); - - var group2 = await groupRepository.CreateAsync(new Group - { - Name = "Test Group #2", - OrganizationId = organization.Id, - }); - - var group3 = await groupRepository.CreateAsync(new Group - { - Name = "Test Group #3", - OrganizationId = organization.Id, - }); - - var collection = new Collection - { - Name = "Test Collection Name", - OrganizationId = organization.Id, - }; - - await collectionRepository.CreateAsync(collection, - [ - new CollectionAccessSelection { Id = group1.Id, Manage = true, HidePasswords = true, ReadOnly = false, }, - new CollectionAccessSelection { Id = group2.Id, Manage = false, HidePasswords = false, ReadOnly = true, }, - ], - [ - new CollectionAccessSelection { Id = orgUser1.Id, Manage = true, HidePasswords = false, ReadOnly = true }, - new CollectionAccessSelection { Id = orgUser2.Id, Manage = false, HidePasswords = true, ReadOnly = false }, - ] - ); - - collection.Name = "Updated Collection Name"; - - await collectionRepository.ReplaceAsync(collection, - [ - // Should delete group1 - new CollectionAccessSelection { Id = group2.Id, Manage = true, HidePasswords = true, ReadOnly = false, }, - // Should add group3 - new CollectionAccessSelection { Id = group3.Id, Manage = false, HidePasswords = false, ReadOnly = true, }, - ], - [ - // Should delete orgUser1 - new CollectionAccessSelection { Id = orgUser2.Id, Manage = false, HidePasswords = false, ReadOnly = true }, - // Should add orgUser3 - new CollectionAccessSelection { Id = orgUser3.Id, Manage = true, HidePasswords = false, ReadOnly = true }, - ] - ); - - // Assert it - var info = await collectionRepository.GetByIdWithPermissionsAsync(collection.Id, user.Id, true); - - Assert.NotNull(info); - - Assert.Equal("Updated Collection Name", info.Name); - - var groups = info.Groups.ToArray(); - - Assert.Equal(2, groups.Length); - - var actualGroup2 = Assert.Single(groups.Where(g => g.Id == group2.Id)); - - Assert.True(actualGroup2.Manage); - Assert.True(actualGroup2.HidePasswords); - Assert.False(actualGroup2.ReadOnly); - - var actualGroup3 = Assert.Single(groups.Where(g => g.Id == group3.Id)); - - Assert.False(actualGroup3.Manage); - Assert.False(actualGroup3.HidePasswords); - Assert.True(actualGroup3.ReadOnly); - - var users = info.Users.ToArray(); - - Assert.Equal(2, users.Length); - - var actualOrgUser2 = Assert.Single(users.Where(u => u.Id == orgUser2.Id)); - - Assert.False(actualOrgUser2.Manage); - Assert.False(actualOrgUser2.HidePasswords); - Assert.True(actualOrgUser2.ReadOnly); - - var actualOrgUser3 = Assert.Single(users.Where(u => u.Id == orgUser3.Id)); - - Assert.True(actualOrgUser3.Manage); - Assert.False(actualOrgUser3.HidePasswords); - Assert.True(actualOrgUser3.ReadOnly); - - // Clean up data - await userRepository.DeleteAsync(user); - await organizationRepository.DeleteAsync(organization); - await groupRepository.DeleteManyAsync([group1.Id, group2.Id, group3.Id]); - await organizationUserRepository.DeleteManyAsync([orgUser1.Id, orgUser2.Id, orgUser3.Id]); - } } From 198d96e1555d9c899db36728b37cbbcfc75f99f3 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Fri, 23 May 2025 11:45:41 +1000 Subject: [PATCH 3/4] [PM-21612] [Unified] Fix unhandled error when editing an invited member (#5817) * Check for UserId instead of passing potentially default value to bump account revision date method. * Pass explicit UserId into CipherRepository.CreateAsync method used for imports. --- src/Core/Entities/User.cs | 5 ++ .../ImportFeatures/ImportCiphersCommand.cs | 2 +- .../Vault/Repositories/ICipherRepository.cs | 5 +- .../Vault/Repositories/CipherRepository.cs | 4 +- .../OrganizationUserRepository.cs | 24 +++-- .../Repositories/DatabaseContextExtensions.cs | 14 ++- .../Vault/Repositories/CipherRepository.cs | 7 +- .../ImportCiphersAsyncCommandTests.cs | 4 +- .../OrganizationRepositoryTests.cs | 3 +- .../OrganizationUserRepositoryTests.cs | 8 +- .../AutoFixture/OrganizationUserFixtures.cs | 1 + .../Vault/AutoFixture/CipherFixtures.cs | 1 + .../Repositories/CipherRepositoryTests.cs | 3 +- .../AdminConsole/OrganizationTestHelpers.cs | 21 +++++ .../OrganizationUserReplaceTests.cs | 88 +++++++++++++++++++ .../OrganizationUserRepositoryTests.cs | 2 +- 16 files changed, 169 insertions(+), 23 deletions(-) create mode 100644 test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserReplaceTests.cs rename test/Infrastructure.IntegrationTest/AdminConsole/Repositories/{ => OrganizationUserRepository}/OrganizationUserRepositoryTests.cs (99%) diff --git a/src/Core/Entities/User.cs b/src/Core/Entities/User.cs index b3a6a9592e..08981ca2d3 100644 --- a/src/Core/Entities/User.cs +++ b/src/Core/Entities/User.cs @@ -36,6 +36,11 @@ public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFac public string? TwoFactorRecoveryCode { get; set; } public string? EquivalentDomains { get; set; } public string? ExcludedGlobalEquivalentDomains { get; set; } + /// + /// The Account Revision Date is used to check if new sync needs to occur. It should be updated + /// whenever a change is made that affects a client's sync data; for example, updating their vault or + /// organization membership. + /// public DateTime AccountRevisionDate { get; set; } = DateTime.UtcNow; public string? Key { get; set; } public string? PublicKey { get; set; } diff --git a/src/Core/Tools/ImportFeatures/ImportCiphersCommand.cs b/src/Core/Tools/ImportFeatures/ImportCiphersCommand.cs index 3c58dca183..fd7a82172c 100644 --- a/src/Core/Tools/ImportFeatures/ImportCiphersCommand.cs +++ b/src/Core/Tools/ImportFeatures/ImportCiphersCommand.cs @@ -115,7 +115,7 @@ public class ImportCiphersCommand : IImportCiphersCommand } // Create it all - await _cipherRepository.CreateAsync(ciphers, newFolders); + await _cipherRepository.CreateAsync(importingUserId, ciphers, newFolders); // push await _pushService.PushSyncVaultAsync(importingUserId); diff --git a/src/Core/Vault/Repositories/ICipherRepository.cs b/src/Core/Vault/Repositories/ICipherRepository.cs index f6767fada2..46742c6aa3 100644 --- a/src/Core/Vault/Repositories/ICipherRepository.cs +++ b/src/Core/Vault/Repositories/ICipherRepository.cs @@ -32,7 +32,10 @@ public interface ICipherRepository : IRepository Task DeleteByUserIdAsync(Guid userId); Task DeleteByOrganizationIdAsync(Guid organizationId); Task UpdateCiphersAsync(Guid userId, IEnumerable ciphers); - Task CreateAsync(IEnumerable ciphers, IEnumerable folders); + /// + /// Create ciphers and folders for the specified UserId. Must not be used to create organization owned items. + /// + Task CreateAsync(Guid userId, IEnumerable ciphers, IEnumerable folders); Task CreateAsync(IEnumerable ciphers, IEnumerable collections, IEnumerable collectionCiphers, IEnumerable collectionUsers); Task SoftDeleteAsync(IEnumerable ids, Guid userId); diff --git a/src/Infrastructure.Dapper/Vault/Repositories/CipherRepository.cs b/src/Infrastructure.Dapper/Vault/Repositories/CipherRepository.cs index 3df365330c..e0a89b1685 100644 --- a/src/Infrastructure.Dapper/Vault/Repositories/CipherRepository.cs +++ b/src/Infrastructure.Dapper/Vault/Repositories/CipherRepository.cs @@ -484,7 +484,7 @@ public class CipherRepository : Repository, ICipherRepository } } - public async Task CreateAsync(IEnumerable ciphers, IEnumerable folders) + public async Task CreateAsync(Guid userId, IEnumerable ciphers, IEnumerable folders) { if (!ciphers.Any()) { @@ -518,7 +518,7 @@ public class CipherRepository : Repository, ICipherRepository await connection.ExecuteAsync( $"[{Schema}].[User_BumpAccountRevisionDate]", - new { Id = ciphers.First().UserId }, + new { Id = userId }, commandType: CommandType.StoredProcedure, transaction: transaction); transaction.Commit(); diff --git a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs index 10d92357fe..5ef59d51db 100644 --- a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs @@ -1,4 +1,5 @@ -using AutoMapper; +using System.Diagnostics; +using AutoMapper; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers.Models; using Bit.Core.Enums; @@ -7,11 +8,12 @@ using Bit.Core.Models.Data; using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.Repositories; using Bit.Infrastructure.EntityFramework.Models; +using Bit.Infrastructure.EntityFramework.Repositories; using Bit.Infrastructure.EntityFramework.Repositories.Queries; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; -namespace Bit.Infrastructure.EntityFramework.Repositories; +namespace Bit.Infrastructure.EntityFramework.AdminConsole.Repositories; public class OrganizationUserRepository : Repository, IOrganizationUserRepository { @@ -440,15 +442,23 @@ public class OrganizationUserRepository : Repository requestedCollections) diff --git a/src/Infrastructure.EntityFramework/Repositories/DatabaseContextExtensions.cs b/src/Infrastructure.EntityFramework/Repositories/DatabaseContextExtensions.cs index 6e954e030c..40f2a79887 100644 --- a/src/Infrastructure.EntityFramework/Repositories/DatabaseContextExtensions.cs +++ b/src/Infrastructure.EntityFramework/Repositories/DatabaseContextExtensions.cs @@ -1,4 +1,6 @@ -using System.Diagnostics; +#nullable enable + +using System.Diagnostics; using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.Auth.Enums; using Bit.Core.Enums; @@ -11,8 +13,18 @@ namespace Bit.Infrastructure.EntityFramework.Repositories; public static class DatabaseContextExtensions { + /// + /// Bump the account revision date for the user. + /// The caller is responsible for providing a valid UserId (not a null or default Guid) for a user that exists + /// in the database. + /// public static async Task UserBumpAccountRevisionDateAsync(this DatabaseContext context, Guid userId) { + if (userId == Guid.Empty) + { + throw new ArgumentException("Invalid UserId."); + } + var user = await context.Users.FindAsync(userId); Debug.Assert(user is not null, "The user id is expected to be validated as a true-in database user before making this call."); user.AccountRevisionDate = DateTime.UtcNow; diff --git a/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs b/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs index 090c36ff29..befb835e26 100644 --- a/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs +++ b/src/Infrastructure.EntityFramework/Vault/Repositories/CipherRepository.cs @@ -142,8 +142,10 @@ public class CipherRepository : Repository ciphers, IEnumerable folders) + public async Task CreateAsync(Guid userId, IEnumerable ciphers, + IEnumerable folders) { + ciphers = ciphers.ToList(); if (!ciphers.Any()) { return; @@ -156,7 +158,8 @@ public class CipherRepository : Repository>(ciphers); await dbContext.BulkCopyAsync(base.DefaultBulkCopyOptions, cipherEntities); - await dbContext.UserBumpAccountRevisionDateAsync(ciphers.First().UserId.GetValueOrDefault()); + await dbContext.UserBumpAccountRevisionDateAsync(userId); + await dbContext.SaveChangesAsync(); } } diff --git a/test/Core.Test/Tools/ImportFeatures/ImportCiphersAsyncCommandTests.cs b/test/Core.Test/Tools/ImportFeatures/ImportCiphersAsyncCommandTests.cs index 89e6d152cc..f73a628940 100644 --- a/test/Core.Test/Tools/ImportFeatures/ImportCiphersAsyncCommandTests.cs +++ b/test/Core.Test/Tools/ImportFeatures/ImportCiphersAsyncCommandTests.cs @@ -49,7 +49,7 @@ public class ImportCiphersAsyncCommandTests await sutProvider.Sut.ImportIntoIndividualVaultAsync(folders, ciphers, folderRelationships, importingUserId); // Assert - await sutProvider.GetDependency().Received(1).CreateAsync(ciphers, Arg.Any>()); + await sutProvider.GetDependency().Received(1).CreateAsync(importingUserId, ciphers, Arg.Any>()); await sutProvider.GetDependency().Received(1).PushSyncVaultAsync(importingUserId); } @@ -77,7 +77,7 @@ public class ImportCiphersAsyncCommandTests await sutProvider.Sut.ImportIntoIndividualVaultAsync(folders, ciphers, folderRelationships, importingUserId); - await sutProvider.GetDependency().Received(1).CreateAsync(ciphers, Arg.Any>()); + await sutProvider.GetDependency().Received(1).CreateAsync(importingUserId, ciphers, Arg.Any>()); await sutProvider.GetDependency().Received(1).PushSyncVaultAsync(importingUserId); } diff --git a/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationRepositoryTests.cs b/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationRepositoryTests.cs index e8bafaea5b..e5ad4f505a 100644 --- a/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationRepositoryTests.cs +++ b/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationRepositoryTests.cs @@ -7,6 +7,7 @@ using Bit.Core.Test.AutoFixture.Attributes; using Bit.Infrastructure.EFIntegration.Test.AutoFixture; using Bit.Infrastructure.EFIntegration.Test.Repositories.EqualityComparers; using Bit.Infrastructure.EntityFramework.AdminConsole.Models; +using Bit.Infrastructure.EntityFramework.AdminConsole.Repositories; using Xunit; using EfRepo = Bit.Infrastructure.EntityFramework.Repositories; using Organization = Bit.Core.AdminConsole.Entities.Organization; @@ -161,7 +162,7 @@ public class OrganizationRepositoryTests [CiSkippedTheory, EfOrganizationUserAutoData] public async Task SearchUnassignedAsync_Works(OrganizationUser orgUser, User user, Organization org, - List efOrgUserRepos, List efOrgRepos, List efUserRepos, + List efOrgUserRepos, List efOrgRepos, List efUserRepos, SqlRepo.OrganizationUserRepository sqlOrgUserRepo, SqlRepo.OrganizationRepository sqlOrgRepo, SqlRepo.UserRepository sqlUserRepo) { orgUser.Type = OrganizationUserType.Owner; diff --git a/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs b/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs index 21d4ca3476..b1f9968e14 100644 --- a/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs +++ b/test/Infrastructure.EFIntegration.Test/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs @@ -24,7 +24,7 @@ public class OrganizationUserRepositoryTests { [CiSkippedTheory, EfOrganizationUserAutoData] public async Task CreateAsync_Works_DataMatches(OrganizationUser orgUser, User user, Organization org, - OrganizationUserCompare equalityComparer, List suts, + OrganizationUserCompare equalityComparer, List suts, List efOrgRepos, List efUserRepos, SqlRepo.OrganizationUserRepository sqlOrgUserRepo, SqlRepo.UserRepository sqlUserRepo, SqlRepo.OrganizationRepository sqlOrgRepo) @@ -67,7 +67,7 @@ public class OrganizationUserRepositoryTests User user, Organization org, OrganizationUserCompare equalityComparer, - List suts, + List suts, List efUserRepos, List efOrgRepos, SqlRepo.OrganizationUserRepository sqlOrgUserRepo, @@ -113,7 +113,7 @@ public class OrganizationUserRepositoryTests } [CiSkippedTheory, EfOrganizationUserAutoData] - public async Task DeleteAsync_Works_DataMatches(OrganizationUser orgUser, User user, Organization org, List suts, + public async Task DeleteAsync_Works_DataMatches(OrganizationUser orgUser, User user, Organization org, List suts, List efUserRepos, List efOrgRepos, SqlRepo.OrganizationUserRepository sqlOrgUserRepo, SqlRepo.UserRepository sqlUserRepo, SqlRepo.OrganizationRepository sqlOrgRepo) @@ -188,7 +188,7 @@ public class OrganizationUserRepositoryTests List efPolicyRepository, List efUserRepository, List efOrganizationRepository, - List suts, + List suts, List efProviderRepository, List efProviderOrganizationRepository, List efProviderUserRepository, diff --git a/test/Infrastructure.EFIntegration.Test/AutoFixture/OrganizationUserFixtures.cs b/test/Infrastructure.EFIntegration.Test/AutoFixture/OrganizationUserFixtures.cs index 191b48852b..8435f2734a 100644 --- a/test/Infrastructure.EFIntegration.Test/AutoFixture/OrganizationUserFixtures.cs +++ b/test/Infrastructure.EFIntegration.Test/AutoFixture/OrganizationUserFixtures.cs @@ -7,6 +7,7 @@ using Bit.Core.Entities; using Bit.Core.Models.Data; using Bit.Core.Test.AutoFixture.OrganizationUserFixtures; using Bit.Core.Test.AutoFixture.UserFixtures; +using Bit.Infrastructure.EntityFramework.AdminConsole.Repositories; using Bit.Infrastructure.EntityFramework.Repositories; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; diff --git a/test/Infrastructure.EFIntegration.Test/Vault/AutoFixture/CipherFixtures.cs b/test/Infrastructure.EFIntegration.Test/Vault/AutoFixture/CipherFixtures.cs index 65b4e4f6d0..7eb4a91ee9 100644 --- a/test/Infrastructure.EFIntegration.Test/Vault/AutoFixture/CipherFixtures.cs +++ b/test/Infrastructure.EFIntegration.Test/Vault/AutoFixture/CipherFixtures.cs @@ -5,6 +5,7 @@ using Bit.Core.Test.AutoFixture.UserFixtures; using Bit.Core.Vault.Entities; using Bit.Core.Vault.Models.Data; using Bit.Infrastructure.EFIntegration.Test.AutoFixture.Relays; +using Bit.Infrastructure.EntityFramework.AdminConsole.Repositories; using Bit.Infrastructure.EntityFramework.Repositories; using Bit.Infrastructure.EntityFramework.Vault.Repositories; using Bit.Test.Common.AutoFixture; diff --git a/test/Infrastructure.EFIntegration.Test/Vault/Repositories/CipherRepositoryTests.cs b/test/Infrastructure.EFIntegration.Test/Vault/Repositories/CipherRepositoryTests.cs index 3618d5dd0e..689bd5e243 100644 --- a/test/Infrastructure.EFIntegration.Test/Vault/Repositories/CipherRepositoryTests.cs +++ b/test/Infrastructure.EFIntegration.Test/Vault/Repositories/CipherRepositoryTests.cs @@ -9,6 +9,7 @@ using Bit.Infrastructure.EntityFramework.Repositories.Queries; using Bit.Test.Common.AutoFixture.Attributes; using LinqToDB; using Xunit; +using EfAdminConsoleRepo = Bit.Infrastructure.EntityFramework.AdminConsole.Repositories; using EfRepo = Bit.Infrastructure.EntityFramework.Repositories; using EfVaultRepo = Bit.Infrastructure.EntityFramework.Vault.Repositories; using SqlRepo = Bit.Infrastructure.Dapper.Repositories; @@ -112,7 +113,7 @@ public class CipherRepositoryTests [CiSkippedTheory, EfOrganizationCipherCustomize, BitAutoData] public async Task CreateAsync_BumpsOrgUserAccountRevisionDates(Cipher cipher, List users, List orgUsers, Collection collection, Organization org, List suts, List efUserRepos, List efOrgRepos, - List efOrgUserRepos, List efCollectionRepos) + List efOrgUserRepos, List efCollectionRepos) { var savedCiphers = new List(); foreach (var sut in suts) diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs b/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs index 144bff9dcb..10361877d8 100644 --- a/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/OrganizationTestHelpers.cs @@ -56,6 +56,17 @@ public static class OrganizationTestHelpers Type = OrganizationUserType.Owner }); + public static Task CreateTestOrganizationUserInviteAsync( + this IOrganizationUserRepository organizationUserRepository, + Organization organization) + => organizationUserRepository.CreateAsync(new OrganizationUser + { + OrganizationId = organization.Id, + UserId = null, // Invites are not linked to a UserId + Status = OrganizationUserStatusType.Invited, + Type = OrganizationUserType.Owner + }); + public static Task CreateTestGroupAsync( this IGroupRepository groupRepository, Organization organization, @@ -63,4 +74,14 @@ public static class OrganizationTestHelpers => groupRepository.CreateAsync( new Group { OrganizationId = organization.Id, Name = $"{identifier} {Guid.NewGuid()}" } ); + + public static Task CreateTestCollectionAsync( + this ICollectionRepository collectionRepository, + Organization organization, + string identifier = "test") + => collectionRepository.CreateAsync(new Collection + { + OrganizationId = organization.Id, + Name = $"{identifier} {Guid.NewGuid()}" + }); } diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserReplaceTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserReplaceTests.cs new file mode 100644 index 0000000000..0b38ddc172 --- /dev/null +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserReplaceTests.cs @@ -0,0 +1,88 @@ +using Bit.Core.Enums; +using Bit.Core.Models.Data; +using Bit.Core.Repositories; +using Xunit; + +namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.OrganizationUserRepository; + +public class OrganizationUserReplaceTests +{ + /// + /// Specifically tests OrganizationUsers in the invited state, which is unique because + /// they're not linked to a UserId. + /// + [DatabaseTheory, DatabaseData] + public async Task ReplaceAsync_WithCollectionAccess_WhenUserIsInvited_Success( + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + var organization = await organizationRepository.CreateTestOrganizationAsync(); + + var orgUser = await organizationUserRepository.CreateTestOrganizationUserInviteAsync(organization); + + // Act: update the user, including collection access so we test this overloaded method + orgUser.Type = OrganizationUserType.Admin; + orgUser.AccessSecretsManager = true; + var collection = await collectionRepository.CreateTestCollectionAsync(organization); + + await organizationUserRepository.ReplaceAsync(orgUser, [ + new CollectionAccessSelection { Id = collection.Id, Manage = true } + ]); + + // Assert + var (actualOrgUser, actualCollections) = await organizationUserRepository.GetByIdWithCollectionsAsync(orgUser.Id); + Assert.NotNull(actualOrgUser); + Assert.Equal(OrganizationUserType.Admin, actualOrgUser.Type); + Assert.True(actualOrgUser.AccessSecretsManager); + + var collectionAccess = Assert.Single(actualCollections); + Assert.Equal(collection.Id, collectionAccess.Id); + Assert.True(collectionAccess.Manage); + } + + /// + /// Tests OrganizationUsers in the Confirmed status, which is a stand-in for all other + /// non-Invited statuses (which are all linked to a UserId). + /// + /// + /// + /// + [DatabaseTheory, DatabaseData] + public async Task ReplaceAsync_WithCollectionAccess_WhenUserIsConfirmed_Success( + IUserRepository userRepository, + IOrganizationRepository organizationRepository, + IOrganizationUserRepository organizationUserRepository, + ICollectionRepository collectionRepository) + { + var organization = await organizationRepository.CreateTestOrganizationAsync(); + + var user = await userRepository.CreateTestUserAsync(); + // OrganizationUser is linked with the User in the Confirmed status + var orgUser = await organizationUserRepository.CreateTestOrganizationUserAsync(organization, user); + + // Act: update the user, including collection access so we test this overloaded method + orgUser.Type = OrganizationUserType.Admin; + orgUser.AccessSecretsManager = true; + var collection = await collectionRepository.CreateTestCollectionAsync(organization); + + await organizationUserRepository.ReplaceAsync(orgUser, [ + new CollectionAccessSelection { Id = collection.Id, Manage = true } + ]); + + // Assert + var (actualOrgUser, actualCollections) = await organizationUserRepository.GetByIdWithCollectionsAsync(orgUser.Id); + Assert.NotNull(actualOrgUser); + Assert.Equal(OrganizationUserType.Admin, actualOrgUser.Type); + Assert.True(actualOrgUser.AccessSecretsManager); + + var collectionAccess = Assert.Single(actualCollections); + Assert.Equal(collection.Id, collectionAccess.Id); + Assert.True(collectionAccess.Manage); + + // Account revision date should be updated to a later date + var actualUser = await userRepository.GetByIdAsync(user.Id); + Assert.NotNull(actualUser); + Assert.True(actualUser.AccountRevisionDate.CompareTo(user.AccountRevisionDate) > 0); + } +} diff --git a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs similarity index 99% rename from test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs rename to test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs index fd759e4777..0df5dcfb50 100644 --- a/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepository/OrganizationUserRepositoryTests.cs @@ -8,7 +8,7 @@ using Bit.Core.Repositories; using Bit.Core.Utilities; using Xunit; -namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories; +namespace Bit.Infrastructure.IntegrationTest.AdminConsole.Repositories.OrganizationUserRepository; public class OrganizationUserRepositoryTests { From 542941818a0b86785ef717313dc60beaded03f38 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Fri, 23 May 2025 10:31:10 -0400 Subject: [PATCH 4/4] Disallow non ascii in equivalent domain (#5852) * Test malicious domain change * Add tests to detect non-ascii characters * Revert "Test malicious domain change" This reverts commit 0602bf6d844b611304aba139e9f49cd38594273a. * Remove confusing comment from when I was going to detect problems differently * Update test/Core.Test/Utilities/StaticStoreTests.cs Co-authored-by: Matt Bishop * Update test/Core.Test/Utilities/StaticStoreTests.cs Co-authored-by: Matt Bishop --------- Co-authored-by: Matt Bishop --- test/Core.Test/Utilities/StaticStoreTests.cs | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/Core.Test/Utilities/StaticStoreTests.cs b/test/Core.Test/Utilities/StaticStoreTests.cs index e5e2da6a82..05c6d358e5 100644 --- a/test/Core.Test/Utilities/StaticStoreTests.cs +++ b/test/Core.Test/Utilities/StaticStoreTests.cs @@ -28,4 +28,31 @@ public class StaticStoreTests Assert.NotNull(plan); Assert.Equal(planType, plan.Type); } + + [Fact] + public void StaticStore_GlobalEquivalentDomains_OnlyAsciiAllowed() + { + // Ref: https://daniel.haxx.se/blog/2025/05/16/detecting-malicious-unicode/ + // URLs can contain unicode characters that to a computer would point to completely seperate domains but to the + // naked eye look completely identical. For example 'g' and 'ց' look incredibly similar but when included in a + // URL would lead you somewhere different. There is an opening for an attacker to contribute to Bitwarden with a + // url update that could be missed in code review and then if they got a user to that URL Bitwarden could + // consider it equivalent with a cipher in the users vault and offer autofill when we should not. + // GitHub does now show a warning on non-ascii characters but it could still be missed. + // https://github.blog/changelog/2025-05-01-github-now-provides-a-warning-about-hidden-unicode-text/ + + // To defend against this: + // Loop through all equivalent domains and fail if any contain a non-ascii character + // non-ascii character can make a valid URL so it's possible that in the future we have a domain + // we want to allow list, that should be done through `continue`ing in the below foreach loop + // only if the domain strictly equals (do NOT use InvariantCulture comparison) the one added to our allow list. + foreach (var domain in StaticStore.GlobalDomains.SelectMany(p => p.Value)) + { + for (var i = 0; i < domain.Length; i++) + { + var character = domain[i]; + Assert.True(char.IsAscii(character), $"Domain: {domain} contains non-ASCII character: '{character}' at index: {i}"); + } + } + } }