[PM-22105] Extract CollectionService.SaveAsync into commands (#5959)

* Add CreateCollectionCommand and associated interface with validation logic

* Implement CreateCollectionCommand to handle collection creation with organization checks and access permissions.
* Introduce ICreateCollectionCommand interface for defining the collection creation contract.
* Add unit tests for CreateCollectionCommand to validate various scenarios including permission checks and error handling.

* 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.

* Add scoped services for collection commands

* Register ICreateCollectionCommand and IUpdateCollectionCommand in the service collection for handling collection creation and updates.

* Refactor CollectionsController to use command interfaces for collection creation and updates

* Updated CollectionsController to utilize ICreateCollectionCommand and IUpdateCollectionCommand for handling collection creation and updates, replacing calls to ICollectionService.
* Adjusted related unit tests to verify the new command implementations.

* Refactor ICollectionService and CollectionService to remove SaveAsync method

* Removed the SaveAsync method from ICollectionService and its implementation in CollectionService.
* Updated related tests in CollectionServiceTests to reflect the removal of SaveAsync, ensuring existing functionality remains intact.

* Remove unused organization repository dependency from CollectionServiceTests

src/Api/Controllers/CollectionsController.cs

@@ -20,6 +20,8 @@ public class CollectionsController : Controller
 {
     private readonly ICollectionRepository _collectionRepository;
     private readonly ICollectionService _collectionService;
+    private readonly ICreateCollectionCommand _createCollectionCommand;
+    private readonly IUpdateCollectionCommand _updateCollectionCommand;
     private readonly IDeleteCollectionCommand _deleteCollectionCommand;
     private readonly IUserService _userService;
     private readonly IAuthorizationService _authorizationService;
@@ -29,6 +31,8 @@ public class CollectionsController : Controller
     public CollectionsController(
         ICollectionRepository collectionRepository,
         ICollectionService collectionService,
+        ICreateCollectionCommand createCollectionCommand,
+        IUpdateCollectionCommand updateCollectionCommand,
         IDeleteCollectionCommand deleteCollectionCommand,
         IUserService userService,
         IAuthorizationService authorizationService,
@@ -37,6 +41,8 @@ public class CollectionsController : Controller
     {
         _collectionRepository = collectionRepository;
         _collectionService = collectionService;
+        _createCollectionCommand = createCollectionCommand;
+        _updateCollectionCommand = updateCollectionCommand;
         _deleteCollectionCommand = deleteCollectionCommand;
         _userService = userService;
         _authorizationService = authorizationService;
@@ -153,7 +159,7 @@ public class CollectionsController : Controller
         var groups = model.Groups?.Select(g => g.ToSelectionReadOnly());
         var users = model.Users?.Select(g => g.ToSelectionReadOnly()).ToList() ?? new List<CollectionAccessSelection>();
 
-        await _collectionService.SaveAsync(collection, groups, users);
+        await _createCollectionCommand.CreateAsync(collection, groups, users);
 
         if (!_currentContext.UserId.HasValue || (_currentContext.GetOrganization(orgId) == null && await _currentContext.ProviderUserForOrgAsync(orgId)))
         {
@@ -179,7 +185,7 @@ public class CollectionsController : Controller
 
         var groups = model.Groups?.Select(g => g.ToSelectionReadOnly());
         var users = model.Users?.Select(g => g.ToSelectionReadOnly());
-        await _collectionService.SaveAsync(model.ToCollection(collection), groups, users);
+        await _updateCollectionCommand.UpdateAsync(model.ToCollection(collection), groups, users);
 
         if (!_currentContext.UserId.HasValue || (_currentContext.GetOrganization(collection.OrganizationId) == null && await _currentContext.ProviderUserForOrgAsync(collection.OrganizationId)))
         {

src/Api/Public/Controllers/CollectionsController.cs

@@ -2,6 +2,7 @@
 using Bit.Api.Models.Public.Request;
 using Bit.Api.Models.Public.Response;
 using Bit.Core.Context;
+using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces;
 using Bit.Core.Repositories;
 using Bit.Core.Services;
 using Microsoft.AspNetCore.Authorization;
@@ -14,18 +15,18 @@ namespace Bit.Api.Public.Controllers;
 public class CollectionsController : Controller
 {
     private readonly ICollectionRepository _collectionRepository;
-    private readonly ICollectionService _collectionService;
+    private readonly IUpdateCollectionCommand _updateCollectionCommand;
     private readonly ICurrentContext _currentContext;
     private readonly IApplicationCacheService _applicationCacheService;
 
     public CollectionsController(
         ICollectionRepository collectionRepository,
-        ICollectionService collectionService,
+        IUpdateCollectionCommand updateCollectionCommand,
         ICurrentContext currentContext,
         IApplicationCacheService applicationCacheService)
     {
         _collectionRepository = collectionRepository;
-        _collectionService = collectionService;
+        _updateCollectionCommand = updateCollectionCommand;
         _currentContext = currentContext;
         _applicationCacheService = applicationCacheService;
     }
@@ -93,7 +94,7 @@ public class CollectionsController : Controller
         }
         var updatedCollection = model.ToCollection(existingCollection);
         var associations = model.Groups?.Select(c => c.ToCollectionAccessSelection()).ToList();
-        await _collectionService.SaveAsync(updatedCollection, associations);
+        await _updateCollectionCommand.UpdateAsync(updatedCollection, associations, null);
         var response = new CollectionResponseModel(updatedCollection, associations);
         return new JsonResult(response);
     }

src/Core/OrganizationFeatures/OrganizationCollections/CreateCollectionCommand.cs

@@ -0,0 +1,70 @@
+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 CreateCollectionCommand : ICreateCollectionCommand
+{
+    private readonly IEventService _eventService;
+    private readonly IOrganizationRepository _organizationRepository;
+    private readonly ICollectionRepository _collectionRepository;
+
+    public CreateCollectionCommand(
+        IEventService eventService,
+        IOrganizationRepository organizationRepository,
+        ICollectionRepository collectionRepository)
+    {
+        _eventService = eventService;
+        _organizationRepository = organizationRepository;
+        _collectionRepository = collectionRepository;
+    }
+
+    public async Task<Collection> CreateAsync(Collection collection, IEnumerable<CollectionAccessSelection> groups = null,
+        IEnumerable<CollectionAccessSelection> 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.");
+        }
+
+        // Check max collections limit
+        if (org.MaxCollections.HasValue)
+        {
+            var collectionCount = await _collectionRepository.GetCountByOrganizationIdAsync(org.Id);
+            if (org.MaxCollections.Value <= collectionCount)
+            {
+                throw new BadRequestException("You have reached the maximum number of collections " +
+                $"({org.MaxCollections.Value}) for this organization.");
+            }
+        }
+
+        await _collectionRepository.CreateAsync(collection, org.UseGroups ? groupsList : null, usersList);
+        await _eventService.LogCollectionEventAsync(collection, Enums.EventType.Collection_Created);
+
+        return collection;
+    }
+}

src/Core/OrganizationFeatures/OrganizationCollections/Interfaces/ICreateCollectionCommand.cs

@@ -0,0 +1,17 @@
+using Bit.Core.Entities;
+using Bit.Core.Models.Data;
+
+namespace Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces;
+
+public interface ICreateCollectionCommand
+{
+    /// <summary>
+    /// Creates a new collection.
+    /// </summary>
+    /// <param name="collection">The collection to create.</param>
+    /// <param name="groups">(Optional) The groups that will have access to the collection.</param>
+    /// <param name="users">(Optional) The users that will have access to the collection.</param>
+    /// <returns>The created collection.</returns>
+    Task<Collection> CreateAsync(Collection collection, IEnumerable<CollectionAccessSelection> groups = null,
+        IEnumerable<CollectionAccessSelection> users = null);
+}

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
+{
+    /// <summary>
+    /// Updates a collection.
+    /// </summary>
+    /// <param name="collection">The collection to update.</param>
+    /// <param name="groups">(Optional) The groups that will have access to the collection.</param>
+    /// <param name="users">(Optional) The users that will have access to the collection.</param>
+    /// <returns>The updated collection.</returns>
+    Task<Collection> UpdateAsync(Collection collection, IEnumerable<CollectionAccessSelection> groups = null,
+        IEnumerable<CollectionAccessSelection> users = null);
+}

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<Collection> UpdateAsync(Collection collection, IEnumerable<CollectionAccessSelection> groups = null,
+        IEnumerable<CollectionAccessSelection> 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;
+    }
+}

src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs

@@ -143,6 +143,8 @@ public static class OrganizationServiceCollectionExtensions
 
     public static void AddOrganizationCollectionCommands(this IServiceCollection services)
     {
+        services.AddScoped<ICreateCollectionCommand, CreateCollectionCommand>();
+        services.AddScoped<IUpdateCollectionCommand, UpdateCollectionCommand>();
         services.AddScoped<IDeleteCollectionCommand, DeleteCollectionCommand>();
         services.AddScoped<IBulkAddCollectionAccessCommand, BulkAddCollectionAccessCommand>();
     }

src/Core/Services/ICollectionService.cs

@@ -1,10 +1,8 @@
 using Bit.Core.Entities;
-using Bit.Core.Models.Data;
 
 namespace Bit.Core.Services;
 
 public interface ICollectionService
 {
-    Task SaveAsync(Collection collection, IEnumerable<CollectionAccessSelection> groups = null, IEnumerable<CollectionAccessSelection> users = null);
     Task DeleteUserAsync(Collection collection, Guid organizationUserId);
 }

src/Core/Services/Implementations/CollectionService.cs

@@ -2,7 +2,6 @@
 
 using Bit.Core.Entities;
 using Bit.Core.Exceptions;
-using Bit.Core.Models.Data;
 using Bit.Core.Repositories;
 
 namespace Bit.Core.Services;
@@ -10,71 +9,20 @@ namespace Bit.Core.Services;
 public class CollectionService : ICollectionService
 {
     private readonly IEventService _eventService;
-    private readonly IOrganizationRepository _organizationRepository;
     private readonly IOrganizationUserRepository _organizationUserRepository;
     private readonly ICollectionRepository _collectionRepository;
 
     public CollectionService(
         IEventService eventService,
-        IOrganizationRepository organizationRepository,
         IOrganizationUserRepository organizationUserRepository,
         ICollectionRepository collectionRepository)
     {
         _eventService = eventService;
-        _organizationRepository = organizationRepository;
         _organizationUserRepository = organizationUserRepository;
         _collectionRepository = collectionRepository;
     }
 
-    public async Task SaveAsync(Collection collection, IEnumerable<CollectionAccessSelection>? groups = null,
-        IEnumerable<CollectionAccessSelection>? 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.");
-        }
-
-        if (collection.Id == default(Guid))
-        {
-            if (org.MaxCollections.HasValue)
-            {
-                var collectionCount = await _collectionRepository.GetCountByOrganizationIdAsync(org.Id);
-                if (org.MaxCollections.Value <= collectionCount)
-                {
-                    throw new BadRequestException("You have reached the maximum number of collections " +
-                    $"({org.MaxCollections.Value}) for this organization.");
-                }
-            }
-
-            await _collectionRepository.CreateAsync(collection, org.UseGroups ? groupsList : null, usersList);
-            await _eventService.LogCollectionEventAsync(collection, Enums.EventType.Collection_Created);
-        }
-        else
-        {
-            await _collectionRepository.ReplaceAsync(collection, org.UseGroups ? groupsList : null, usersList);
-            await _eventService.LogCollectionEventAsync(collection, Enums.EventType.Collection_Updated);
-        }
-    }
 
     public async Task DeleteUserAsync(Collection collection, Guid organizationUserId)
     {

test/Api.Test/Controllers/CollectionsControllerTests.cs

@@ -9,7 +9,6 @@ using Bit.Core.Exceptions;
 using Bit.Core.Models.Data;
 using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces;
 using Bit.Core.Repositories;
-using Bit.Core.Services;
 using Bit.Test.Common.AutoFixture;
 using Bit.Test.Common.AutoFixture.Attributes;
 using Microsoft.AspNetCore.Authorization;
@@ -38,9 +37,11 @@ public class CollectionsControllerTests
 
         _ = await sutProvider.Sut.Post(organization.Id, collectionRequest);
 
-        await sutProvider.GetDependency<ICollectionService>()
+        await sutProvider.GetDependency<ICreateCollectionCommand>()
             .Received(1)
-            .SaveAsync(Arg.Any<Collection>(), Arg.Any<IEnumerable<CollectionAccessSelection>>(),
+            .CreateAsync(Arg.Is<Collection>(c =>
+                c.Name == collectionRequest.Name && c.ExternalId == collectionRequest.ExternalId && c.OrganizationId == organization.Id),
+                Arg.Any<IEnumerable<CollectionAccessSelection>>(),
                 Arg.Any<IEnumerable<CollectionAccessSelection>>());
     }
 
@@ -64,9 +65,9 @@ public class CollectionsControllerTests
 
         _ = await sutProvider.Sut.Put(collection.OrganizationId, collection.Id, collectionRequest);
 
-        await sutProvider.GetDependency<ICollectionService>()
+        await sutProvider.GetDependency<IUpdateCollectionCommand>()
             .Received(1)
-            .SaveAsync(ExpectedCollection(), Arg.Any<IEnumerable<CollectionAccessSelection>>(),
+            .UpdateAsync(ExpectedCollection(), Arg.Any<IEnumerable<CollectionAccessSelection>>(),
                 Arg.Any<IEnumerable<CollectionAccessSelection>>());
     }
 

test/Core.Test/OrganizationFeatures/OrganizationCollections/CreateCollectionCommandTests.cs

@@ -0,0 +1,202 @@
+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 CreateCollectionCommandTests
+{
+    [Theory, BitAutoData]
+    public async Task CreateAsync_WithoutGroupsAndUsers_CreatesCollection(
+        Organization organization, Collection collection,
+        SutProvider<CreateCollectionCommand> sutProvider)
+    {
+        collection.Id = default;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+        var utcNow = DateTime.UtcNow;
+
+        await sutProvider.Sut.CreateAsync(collection, null, null);
+
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .Received(1)
+            .CreateAsync(
+                collection,
+                Arg.Is<List<CollectionAccessSelection>>(l => l == null),
+                Arg.Is<List<CollectionAccessSelection>>(l => l == null));
+        await sutProvider.GetDependency<IEventService>()
+            .Received(1)
+            .LogCollectionEventAsync(collection, EventType.Collection_Created);
+        Assert.True(collection.CreationDate - utcNow < TimeSpan.FromSeconds(1));
+        Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1));
+    }
+
+    [Theory, BitAutoData]
+    public async Task CreateAsync_WithGroupsAndUsers_CreatesCollectionWithGroupsAndUsers(
+        Organization organization, Collection collection,
+        [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> groups,
+        IEnumerable<CollectionAccessSelection> users,
+        SutProvider<CreateCollectionCommand> sutProvider)
+    {
+        collection.Id = default;
+        organization.UseGroups = true;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+        var utcNow = DateTime.UtcNow;
+
+        await sutProvider.Sut.CreateAsync(collection, groups, users);
+
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .Received(1)
+            .CreateAsync(
+                collection,
+                Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)),
+                Arg.Any<List<CollectionAccessSelection>>());
+        await sutProvider.GetDependency<IEventService>()
+            .Received(1)
+            .LogCollectionEventAsync(collection, EventType.Collection_Created);
+        Assert.True(collection.CreationDate - utcNow < TimeSpan.FromSeconds(1));
+        Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1));
+    }
+
+    [Theory, BitAutoData]
+    public async Task CreateAsync_WithOrganizationUseGroupDisabled_CreatesCollectionWithoutGroups(
+        Organization organization, Collection collection,
+        [CollectionAccessSelectionCustomize] IEnumerable<CollectionAccessSelection> groups,
+        [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> users,
+        SutProvider<CreateCollectionCommand> sutProvider)
+    {
+        collection.Id = default;
+        organization.UseGroups = false;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+        var utcNow = DateTime.UtcNow;
+
+        await sutProvider.Sut.CreateAsync(collection, groups, users);
+
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .Received(1)
+            .CreateAsync(
+                collection,
+                Arg.Is<List<CollectionAccessSelection>>(l => l == null),
+                Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)));
+        await sutProvider.GetDependency<IEventService>()
+            .Received(1)
+            .LogCollectionEventAsync(collection, EventType.Collection_Created);
+        Assert.True(collection.CreationDate - utcNow < TimeSpan.FromSeconds(1));
+        Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1));
+    }
+
+    [Theory, BitAutoData]
+    public async Task CreateAsync_WithNonExistingOrganizationId_ThrowsBadRequest(
+        Collection collection, SutProvider<CreateCollectionCommand> sutProvider)
+    {
+        collection.Id = default;
+        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.CreateAsync(collection));
+        Assert.Contains("Organization not found", ex.Message);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default, default, default);
+        await sutProvider.GetDependency<IEventService>()
+            .DidNotReceiveWithAnyArgs()
+            .LogCollectionEventAsync(default, default);
+    }
+
+    [Theory, BitAutoData]
+    public async Task CreateAsync_WithoutManageAccess_ThrowsBadRequest(
+        Organization organization, Collection collection,
+        [CollectionAccessSelectionCustomize] IEnumerable<CollectionAccessSelection> users,
+        SutProvider<CreateCollectionCommand> sutProvider)
+    {
+        collection.Id = default;
+        organization.AllowAdminAccessToAllCollectionItems = false;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+
+        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.CreateAsync(collection, null, users));
+        Assert.Contains("At least one member or group must have can manage permission.", ex.Message);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default, default, default);
+        await sutProvider.GetDependency<IEventService>()
+            .DidNotReceiveWithAnyArgs()
+            .LogCollectionEventAsync(default, default);
+    }
+
+    [Theory, BitAutoData]
+    public async Task CreateAsync_WithExceedsOrganizationMaxCollections_ThrowsBadRequest(
+        Organization organization, Collection collection,
+        [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> users,
+        SutProvider<CreateCollectionCommand> sutProvider)
+    {
+        collection.Id = default;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+        sutProvider.GetDependency<ICollectionRepository>()
+            .GetCountByOrganizationIdAsync(organization.Id)
+            .Returns(organization.MaxCollections.Value);
+
+        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.CreateAsync(collection, null, users));
+        Assert.Equal($@"You have reached the maximum number of collections ({organization.MaxCollections.Value}) for this organization.", ex.Message);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default, default, default);
+        await sutProvider.GetDependency<IEventService>()
+            .DidNotReceiveWithAnyArgs()
+            .LogCollectionEventAsync(default, default);
+    }
+
+    [Theory, BitAutoData]
+    public async Task CreateAsync_WithInvalidManageAssociations_ThrowsBadRequest(
+        Organization organization, Collection collection, SutProvider<CreateCollectionCommand> sutProvider)
+    {
+        collection.Id = default;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+
+        var invalidGroups = new List<CollectionAccessSelection>
+        {
+            new() { Id = Guid.NewGuid(), Manage = true, ReadOnly = true }
+        };
+
+        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.CreateAsync(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<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .CreateAsync(default, default, default);
+        await sutProvider.GetDependency<IEventService>()
+            .DidNotReceiveWithAnyArgs()
+            .LogCollectionEventAsync(default, default);
+    }
+}

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<UpdateCollectionCommand> sutProvider)
+    {
+        var creationDate = collection.CreationDate;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+        var utcNow = DateTime.UtcNow;
+
+        await sutProvider.Sut.UpdateAsync(collection, null, null);
+
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .Received(1)
+            .ReplaceAsync(
+                collection,
+                Arg.Is<List<CollectionAccessSelection>>(l => l == null),
+                Arg.Is<List<CollectionAccessSelection>>(l => l == null));
+        await sutProvider.GetDependency<IEventService>()
+            .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<CollectionAccessSelection> groups,
+        IEnumerable<CollectionAccessSelection> users,
+        SutProvider<UpdateCollectionCommand> sutProvider)
+    {
+        var creationDate = collection.CreationDate;
+        organization.UseGroups = true;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+        var utcNow = DateTime.UtcNow;
+
+        await sutProvider.Sut.UpdateAsync(collection, groups, users);
+
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .Received(1)
+            .ReplaceAsync(
+                collection,
+                Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)),
+                Arg.Any<List<CollectionAccessSelection>>());
+        await sutProvider.GetDependency<IEventService>()
+            .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<CollectionAccessSelection> groups,
+        [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> users,
+        SutProvider<UpdateCollectionCommand> sutProvider)
+    {
+        var creationDate = collection.CreationDate;
+        organization.UseGroups = false;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+        var utcNow = DateTime.UtcNow;
+
+        await sutProvider.Sut.UpdateAsync(collection, groups, users);
+
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .Received(1)
+            .ReplaceAsync(
+                collection,
+                Arg.Is<List<CollectionAccessSelection>>(l => l == null),
+                Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)));
+        await sutProvider.GetDependency<IEventService>()
+            .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<UpdateCollectionCommand> sutProvider)
+    {
+        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.UpdateAsync(collection));
+        Assert.Contains("Organization not found", ex.Message);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .ReplaceAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .ReplaceAsync(default, default, default);
+        await sutProvider.GetDependency<IEventService>()
+            .DidNotReceiveWithAnyArgs()
+            .LogCollectionEventAsync(default, default);
+    }
+
+    [Theory, BitAutoData]
+    public async Task UpdateAsync_WithoutManageAccess_ThrowsBadRequest(
+        Organization organization, Collection collection,
+        [CollectionAccessSelectionCustomize] IEnumerable<CollectionAccessSelection> users,
+        SutProvider<UpdateCollectionCommand> sutProvider)
+    {
+        organization.AllowAdminAccessToAllCollectionItems = false;
+        sutProvider.GetDependency<IOrganizationRepository>()
+            .GetByIdAsync(organization.Id)
+            .Returns(organization);
+
+        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.UpdateAsync(collection, null, users));
+        Assert.Contains("At least one member or group must have can manage permission.", ex.Message);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .ReplaceAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .ReplaceAsync(default, default, default);
+        await sutProvider.GetDependency<IEventService>()
+            .DidNotReceiveWithAnyArgs()
+            .LogCollectionEventAsync(default, default);
+    }
+
+    [Theory, BitAutoData]
+    public async Task UpdateAsync_WithInvalidManageAssociations_ThrowsBadRequest(
+        Organization organization, Collection collection, SutProvider<UpdateCollectionCommand> sutProvider)
+    {
+        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
+
+        var invalidGroups = new List<CollectionAccessSelection>
+        {
+            new() { Id = Guid.NewGuid(), Manage = true, HidePasswords = true }
+        };
+
+        var ex = await Assert.ThrowsAsync<BadRequestException>(() => 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<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .ReplaceAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>()
+            .DidNotReceiveWithAnyArgs()
+            .ReplaceAsync(default, default, default);
+        await sutProvider.GetDependency<IEventService>()
+            .DidNotReceiveWithAnyArgs()
+            .LogCollectionEventAsync(default, default);
+    }
+}

test/Core.Test/Services/CollectionServiceTests.cs

@@ -2,10 +2,8 @@
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
-using Bit.Core.Models.Data;
 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;
@@ -18,135 +16,12 @@ namespace Bit.Core.Test.Services;
 [OrganizationCustomize]
 public class CollectionServiceTest
 {
-    [Theory, BitAutoData]
-    public async Task SaveAsync_DefaultIdWithUsers_CreatesCollectionInTheRepository(Collection collection, Organization organization, [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> users, SutProvider<CollectionService> sutProvider)
-    {
-        collection.Id = default;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
-        var utcNow = DateTime.UtcNow;
-
-        await sutProvider.Sut.SaveAsync(collection, null, users);
-
-        await sutProvider.GetDependency<ICollectionRepository>().Received()
-            .CreateAsync(collection, Arg.Is<List<CollectionAccessSelection>>(l => l == null),
-                Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)));
-        await sutProvider.GetDependency<IEventService>().Received()
-            .LogCollectionEventAsync(collection, EventType.Collection_Created);
-        Assert.True(collection.CreationDate - utcNow < TimeSpan.FromSeconds(1));
-        Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1));
-    }
-
-    [Theory, BitAutoData]
-    public async Task SaveAsync_DefaultIdWithGroupsAndUsers_CreateCollectionWithGroupsAndUsersInRepository(Collection collection,
-        [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> groups, IEnumerable<CollectionAccessSelection> users, Organization organization, SutProvider<CollectionService> sutProvider)
-    {
-        collection.Id = default;
-        organization.UseGroups = true;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
-        var utcNow = DateTime.UtcNow;
-
-        await sutProvider.Sut.SaveAsync(collection, groups, users);
-
-        await sutProvider.GetDependency<ICollectionRepository>().Received()
-            .CreateAsync(collection, Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)),
-                Arg.Any<List<CollectionAccessSelection>>());
-        await sutProvider.GetDependency<IEventService>().Received()
-            .LogCollectionEventAsync(collection, EventType.Collection_Created);
-        Assert.True(collection.CreationDate - utcNow < TimeSpan.FromSeconds(1));
-        Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1));
-    }
-
-    [Theory, BitAutoData]
-    public async Task SaveAsync_NonDefaultId_ReplacesCollectionInRepository(Collection collection, Organization organization, [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> users, SutProvider<CollectionService> sutProvider)
-    {
-        var creationDate = collection.CreationDate;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
-        var utcNow = DateTime.UtcNow;
-
-        await sutProvider.Sut.SaveAsync(collection, null, users);
-
-        await sutProvider.GetDependency<ICollectionRepository>().Received().ReplaceAsync(collection,
-            Arg.Is<List<CollectionAccessSelection>>(l => l == null),
-            Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)));
-        await sutProvider.GetDependency<IEventService>().Received()
-            .LogCollectionEventAsync(collection, EventType.Collection_Updated);
-        Assert.Equal(collection.CreationDate, creationDate);
-        Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1));
-    }
-
-    [Theory, BitAutoData]
-    public async Task SaveAsync_OrganizationNotUseGroup_CreateCollectionWithoutGroupsInRepository(Collection collection,
-        [CollectionAccessSelectionCustomize] IEnumerable<CollectionAccessSelection> groups, [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> users,
-        Organization organization, SutProvider<CollectionService> sutProvider)
-    {
-        collection.Id = default;
-        organization.UseGroups = false;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
-        var utcNow = DateTime.UtcNow;
-
-        await sutProvider.Sut.SaveAsync(collection, groups, users);
-
-        await sutProvider.GetDependency<ICollectionRepository>().Received().CreateAsync(collection,
-            Arg.Is<List<CollectionAccessSelection>>(l => l == null),
-            Arg.Is<List<CollectionAccessSelection>>(l => l.Any(i => i.Manage == true)));
-        await sutProvider.GetDependency<IEventService>().Received()
-            .LogCollectionEventAsync(collection, EventType.Collection_Created);
-        Assert.True(collection.CreationDate - utcNow < TimeSpan.FromSeconds(1));
-        Assert.True(collection.RevisionDate - utcNow < TimeSpan.FromSeconds(1));
-    }
-
-    [Theory, BitAutoData]
-    public async Task SaveAsync_NonExistingOrganizationId_ThrowsBadRequest(Collection collection, SutProvider<CollectionService> sutProvider)
-    {
-        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.SaveAsync(collection));
-        Assert.Contains("Organization not found", ex.Message);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().CreateAsync(default);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().CreateAsync(default, default, default);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().ReplaceAsync(default);
-        await sutProvider.GetDependency<IEventService>().DidNotReceiveWithAnyArgs().LogCollectionEventAsync(default, default);
-    }
-
-    [Theory, BitAutoData]
-    public async Task SaveAsync_NoManageAccess_ThrowsBadRequest(Collection collection, Organization organization,
-        [CollectionAccessSelectionCustomize] IEnumerable<CollectionAccessSelection> users, SutProvider<CollectionService> sutProvider)
-    {
-        collection.Id = default;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
-        organization.AllowAdminAccessToAllCollectionItems = false;
-
-        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.SaveAsync(collection, null, users));
-        Assert.Contains("At least one member or group must have can manage permission.", ex.Message);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().CreateAsync(default);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().CreateAsync(default, default, default);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().ReplaceAsync(default);
-        await sutProvider.GetDependency<IEventService>().DidNotReceiveWithAnyArgs().LogCollectionEventAsync(default, default);
-    }
-
-    [Theory, BitAutoData]
-    public async Task SaveAsync_ExceedsOrganizationMaxCollections_ThrowsBadRequest(Collection collection,
-        Organization organization, [CollectionAccessSelectionCustomize(true)] IEnumerable<CollectionAccessSelection> users,
-        SutProvider<CollectionService> sutProvider)
-    {
-        collection.Id = default;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
-        sutProvider.GetDependency<ICollectionRepository>().GetCountByOrganizationIdAsync(organization.Id)
-            .Returns(organization.MaxCollections.Value);
-
-        var ex = await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.SaveAsync(collection, null, users));
-        Assert.Equal($@"You have reached the maximum number of collections ({organization.MaxCollections.Value}) for this organization.", ex.Message);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().CreateAsync(default);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().CreateAsync(default, default, default);
-        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().ReplaceAsync(default);
-        await sutProvider.GetDependency<IEventService>().DidNotReceiveWithAnyArgs().LogCollectionEventAsync(default, default);
-    }
-
     [Theory, BitAutoData]
     public async Task DeleteUserAsync_DeletesValidUserWhoBelongsToCollection(Collection collection,
         Organization organization, OrganizationUser organizationUser, SutProvider<CollectionService> sutProvider)
     {
         collection.OrganizationId = organization.Id;
         organizationUser.OrganizationId = organization.Id;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
         sutProvider.GetDependency<IOrganizationUserRepository>().GetByIdAsync(organizationUser.Id)
             .Returns(organizationUser);
 
@@ -162,7 +37,6 @@ public class CollectionServiceTest
         OrganizationUser organizationUser, SutProvider<CollectionService> sutProvider)
     {
         collection.OrganizationId = organization.Id;
-        sutProvider.GetDependency<IOrganizationRepository>().GetByIdAsync(organization.Id).Returns(organization);
         sutProvider.GetDependency<IOrganizationUserRepository>().GetByIdAsync(organizationUser.Id)
             .Returns(organizationUser);