From 2e2cf2e0dfe7cc25d993651ed75cff1909323d35 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Thu, 12 Jun 2025 14:46:01 +0100 Subject: [PATCH] Add UpdateCollectionCommand and associated interface with validation logic * Implement UpdateCollectionCommand to handle collection updates with organization checks and access permissions. * Introduce IUpdateCollectionCommand interface for defining the collection update contract. * Add unit tests for UpdateCollectionCommand to validate various scenarios including permission checks and error handling. --- .../Interfaces/IUpdateCollectionCommand.cs | 17 ++ .../UpdateCollectionCommand.cs | 59 ++++++ .../UpdateCollectionCommandTests.cs | 169 ++++++++++++++++++ 3 files changed, 245 insertions(+) create mode 100644 src/Core/OrganizationFeatures/OrganizationCollections/Interfaces/IUpdateCollectionCommand.cs create mode 100644 src/Core/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommand.cs create mode 100644 test/Core.Test/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommandTests.cs diff --git a/src/Core/OrganizationFeatures/OrganizationCollections/Interfaces/IUpdateCollectionCommand.cs b/src/Core/OrganizationFeatures/OrganizationCollections/Interfaces/IUpdateCollectionCommand.cs new file mode 100644 index 0000000000..94d4d1d1f8 --- /dev/null +++ b/src/Core/OrganizationFeatures/OrganizationCollections/Interfaces/IUpdateCollectionCommand.cs @@ -0,0 +1,17 @@ +using Bit.Core.Entities; +using Bit.Core.Models.Data; + +namespace Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces; + +public interface IUpdateCollectionCommand +{ + /// + /// Updates a collection. + /// + /// The collection to update. + /// (Optional) The groups that will have access to the collection. + /// (Optional) The users that will have access to the collection. + /// The updated collection. + Task UpdateAsync(Collection collection, IEnumerable groups = null, + IEnumerable users = null); +} diff --git a/src/Core/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommand.cs b/src/Core/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommand.cs new file mode 100644 index 0000000000..3985b6a919 --- /dev/null +++ b/src/Core/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommand.cs @@ -0,0 +1,59 @@ +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Models.Data; +using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces; +using Bit.Core.Repositories; +using Bit.Core.Services; + +namespace Bit.Core.OrganizationFeatures.OrganizationCollections; + +public class UpdateCollectionCommand : IUpdateCollectionCommand +{ + private readonly IEventService _eventService; + private readonly IOrganizationRepository _organizationRepository; + private readonly ICollectionRepository _collectionRepository; + + public UpdateCollectionCommand( + IEventService eventService, + IOrganizationRepository organizationRepository, + ICollectionRepository collectionRepository) + { + _eventService = eventService; + _organizationRepository = organizationRepository; + _collectionRepository = collectionRepository; + } + + public async Task UpdateAsync(Collection collection, IEnumerable groups = null, + IEnumerable users = null) + { + var org = await _organizationRepository.GetByIdAsync(collection.OrganizationId); + if (org == null) + { + throw new BadRequestException("Organization not found"); + } + + var groupsList = groups?.ToList(); + var usersList = users?.ToList(); + + // Cannot use Manage with ReadOnly/HidePasswords permissions + var invalidAssociations = groupsList?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords)); + if (invalidAssociations?.Any() ?? false) + { + throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true."); + } + + // A collection should always have someone with Can Manage permissions + var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false; + var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false; + if (!groupHasManageAccess && !userHasManageAccess && !org.AllowAdminAccessToAllCollectionItems) + { + throw new BadRequestException( + "At least one member or group must have can manage permission."); + } + + await _collectionRepository.ReplaceAsync(collection, org.UseGroups ? groupsList : null, usersList); + await _eventService.LogCollectionEventAsync(collection, Enums.EventType.Collection_Updated); + + return collection; + } +} diff --git a/test/Core.Test/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommandTests.cs b/test/Core.Test/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommandTests.cs new file mode 100644 index 0000000000..5147157750 --- /dev/null +++ b/test/Core.Test/OrganizationFeatures/OrganizationCollections/UpdateCollectionCommandTests.cs @@ -0,0 +1,169 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Exceptions; +using Bit.Core.Models.Data; +using Bit.Core.OrganizationFeatures.OrganizationCollections; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Test.AutoFixture; +using Bit.Core.Test.AutoFixture.OrganizationFixtures; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; + +namespace Bit.Core.Test.OrganizationFeatures.OrganizationCollections; + +[SutProviderCustomize] +[OrganizationCustomize] +public class UpdateCollectionCommandTests +{ + [Theory, BitAutoData] + public async Task UpdateAsync_WithoutGroupsAndUsers_ReplacesCollection( + Organization organization, Collection collection, SutProvider sutProvider) + { + var creationDate = collection.CreationDate; + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); + var utcNow = DateTime.UtcNow; + + await sutProvider.Sut.UpdateAsync(collection, null, null); + + await sutProvider.GetDependency() + .Received(1) + .ReplaceAsync( + collection, + Arg.Is>(l => l == null), + Arg.Is>(l => l == null)); + await sutProvider.GetDependency() + .Received(1) + .LogCollectionEventAsync(collection, EventType.Collection_Updated); + Assert.Equal(collection.CreationDate, creationDate); + Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1)); + } + + [Theory, BitAutoData] + public async Task UpdateAsync_WithGroupsAndUsers_ReplacesCollectionWithGroupsAndUsers( + Organization organization, Collection collection, + [CollectionAccessSelectionCustomize(true)] IEnumerable groups, + IEnumerable users, + SutProvider sutProvider) + { + var creationDate = collection.CreationDate; + organization.UseGroups = true; + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); + var utcNow = DateTime.UtcNow; + + await sutProvider.Sut.UpdateAsync(collection, groups, users); + + await sutProvider.GetDependency() + .Received(1) + .ReplaceAsync( + collection, + Arg.Is>(l => l.Any(i => i.Manage == true)), + Arg.Any>()); + await sutProvider.GetDependency() + .Received(1) + .LogCollectionEventAsync(collection, EventType.Collection_Updated); + Assert.Equal(collection.CreationDate, creationDate); + Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1)); + } + + [Theory, BitAutoData] + public async Task UpdateAsync_WithOrganizationUseGroupDisabled_ReplacesCollectionWithoutGroups( + Organization organization, Collection collection, + [CollectionAccessSelectionCustomize] IEnumerable groups, + [CollectionAccessSelectionCustomize(true)] IEnumerable users, + SutProvider sutProvider) + { + var creationDate = collection.CreationDate; + organization.UseGroups = false; + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); + var utcNow = DateTime.UtcNow; + + await sutProvider.Sut.UpdateAsync(collection, groups, users); + + await sutProvider.GetDependency() + .Received(1) + .ReplaceAsync( + collection, + Arg.Is>(l => l == null), + Arg.Is>(l => l.Any(i => i.Manage == true))); + await sutProvider.GetDependency() + .Received(1) + .LogCollectionEventAsync(collection, EventType.Collection_Updated); + Assert.Equal(collection.CreationDate, creationDate); + Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1)); + } + + [Theory, BitAutoData] + public async Task UpdateAsync_WithNonExistingOrganizationId_ThrowsBadRequest( + Collection collection, SutProvider sutProvider) + { + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateAsync(collection)); + Assert.Contains("Organization not found", ex.Message); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default, default, default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .LogCollectionEventAsync(default, default); + } + + [Theory, BitAutoData] + public async Task UpdateAsync_WithoutManageAccess_ThrowsBadRequest( + Organization organization, Collection collection, + [CollectionAccessSelectionCustomize] IEnumerable users, + SutProvider sutProvider) + { + organization.AllowAdminAccessToAllCollectionItems = false; + sutProvider.GetDependency() + .GetByIdAsync(organization.Id) + .Returns(organization); + + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateAsync(collection, null, users)); + Assert.Contains("At least one member or group must have can manage permission.", ex.Message); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default, default, default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .LogCollectionEventAsync(default, default); + } + + [Theory, BitAutoData] + public async Task UpdateAsync_WithInvalidManageAssociations_ThrowsBadRequest( + Organization organization, Collection collection, SutProvider sutProvider) + { + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + + var invalidGroups = new List + { + new() { Id = Guid.NewGuid(), Manage = true, HidePasswords = true } + }; + + var ex = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateAsync(collection, invalidGroups, null)); + Assert.Contains("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true.", ex.Message); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default, default, default); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .LogCollectionEventAsync(default, default); + } +}