[AC-2847] Simplify OrganizationUser and Group PUT methods and tests

src/Api/AdminConsole/Controllers/GroupsController.cs

@@ -175,13 +175,20 @@
     /// </summary>
     private async Task<GroupResponseModel> Put_vNext(Guid orgId, Guid id, [FromBody] GroupRequestModel model)
     {
+        if (!await _currentContext.ManageGroups(orgId))
+        {
+            throw new NotFoundException();
+        }
+
         var (group, currentAccess) = await _groupRepository.GetByIdWithCollectionsAsync(id);
-        if (group == null || !await _currentContext.ManageGroups(group.OrganizationId))
+        if (group == null || group.OrganizationId != orgId)
         {
             throw new NotFoundException();
         }
 
-        // Check whether the user is permitted to add themselves to the group
+        // Authorization check:
+        // If admins are not allowed access to all collections, you cannot add yourself to a group.
+        // No error is thrown for this, we just don't update groups.
         var orgAbility = await _applicationCacheService.GetOrganizationAbilityAsync(orgId);
         if (!orgAbility.AllowAdminAccessToAllCollectionItems)
         {
@@ -195,9 +202,23 @@
             }
         }
 
+        // Authorization check:
+        // You must have authorization to ModifyUserAccess for all collections being saved
+        var postedCollections = await _collectionRepository
+            .GetManyByManyIdsAsync(model.Collections.Select(c => c.Id));
+        foreach (var collection in postedCollections)
+        {
+            if (!(await _authorizationService.AuthorizeAsync(User, collection,
+                    BulkCollectionOperations.ModifyGroupAccess))
+                .Succeeded)
+            {
+                throw new BadRequestException("You must have Can Manage permissions to edit a collection's membership");
+            }
+        }
+
         // The client only sends collections that the saving user has permissions to edit.
-        // On the server side, we need to (1) confirm this and (2) concat these with the collections that the user
-        // can't edit before saving to the database.
+        // We need to combine these with collections that the user doesn't have permissions for, so that we don't
+        // accidentally overwrite those
         var currentCollections = await _collectionRepository
             .GetManyByManyIdsAsync(currentAccess.Select(cas => cas.Id));
 
@@ -211,11 +232,6 @@
             }
         }
 
-        if (model.Collections.Any(c => readonlyCollectionIds.Contains(c.Id)))
-        {
-            throw new BadRequestException("You must have Can Manage permissions to edit a collection's membership");
-        }
-
         var editedCollectionAccess = model.Collections
             .Select(c => c.ToSelectionReadOnly());
         var readonlyCollectionAccess = currentAccess

src/Api/AdminConsole/Controllers/OrganizationUsersController.cs

@@ -400,13 +400,15 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd
         var userId = _userService.GetProperUserId(User).Value;
         var editingSelf = userId == organizationUser.UserId;
 
+        // Authorization check:
         // If admins are not allowed access to all collections, you cannot add yourself to a group.
-        // In this case we just don't update groups.
+        // No error is thrown for this, we just don't update groups.
         var organizationAbility = await _applicationCacheService.GetOrganizationAbilityAsync(orgId);
         var groupsToSave = editingSelf && !organizationAbility.AllowAdminAccessToAllCollectionItems
             ? null
             : model.Groups;
 
+        // Authorization check:
         // If admins are not allowed access to all collections, you cannot add yourself to collections.
         // This is not caught by the requirement below that you can ModifyUserAccess and must be checked separately
         var currentAccessIds = currentAccess.Select(c => c.Id).ToHashSet();
@@ -417,9 +419,24 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd
             throw new BadRequestException("You cannot add yourself to a collection.");
         }
 
+        // Authorization check:
+        // You must have authorization to ModifyUserAccess for all collections being saved
+        var postedCollections = await _collectionRepository
+            .GetManyByManyIdsAsync(model.Collections.Select(c => c.Id));
+        foreach (var collection in postedCollections)
+        {
+            if (!(await _authorizationService.AuthorizeAsync(User, collection,
+                    BulkCollectionOperations.ModifyUserAccess))
+                .Succeeded)
+            {
+                throw new BadRequestException(
+                    $"You must have Can Manage permissions to edit the membership for collection id {collection.Id}.");
+            }
+        }
+
         // The client only sends collections that the saving user has permissions to edit.
-        // On the server side, we need to (1) make sure the user has permissions for these collections, and
-        // (2) concat these with the collections that the user can't edit before saving to the database.
+        // We need to combine these with collections that the user doesn't have permissions for, so that we don't
+        // accidentally overwrite those
         var currentCollections = await _collectionRepository
             .GetManyByManyIdsAsync(currentAccess.Select(cas => cas.Id));
 
@@ -433,11 +450,6 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd
             }
         }
 
-        if (model.Collections.Any(c => readonlyCollectionIds.Contains(c.Id)))
-        {
-            throw new BadRequestException("You must have Can Manage permissions to edit a collection's membership");
-        }
-
         var editedCollectionAccess = model.Collections
             .Select(c => c.ToSelectionReadOnly());
         var readonlyCollectionAccess = currentAccess

src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs

@@ -1,4 +1,6 @@
-using Bit.Core.AdminConsole.Entities;
+#nullable enable
+
+using Bit.Core.AdminConsole.Entities;
 using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces;
 using Bit.Core.AdminConsole.Repositories;
 using Bit.Core.Enums;
@@ -14,48 +16,53 @@
     private readonly IEventService _eventService;
     private readonly IGroupRepository _groupRepository;
     private readonly IOrganizationUserRepository _organizationUserRepository;
+    private readonly ICollectionRepository _collectionRepository;
 
     public UpdateGroupCommand(
         IEventService eventService,
         IGroupRepository groupRepository,
-        IOrganizationUserRepository organizationUserRepository)
+        IOrganizationUserRepository organizationUserRepository,
+        ICollectionRepository collectionRepository)
     {
         _eventService = eventService;
         _groupRepository = groupRepository;
         _organizationUserRepository = organizationUserRepository;
+        _collectionRepository = collectionRepository;
     }
 
     public async Task UpdateGroupAsync(Group group, Organization organization,
-        ICollection<CollectionAccessSelection> collections = null,
-        IEnumerable<Guid> userIds = null)
+        ICollection<CollectionAccessSelection>? collections = null,
+        IEnumerable<Guid>? userIds = null)
     {
-        Validate(organization, group, collections);
-        await GroupRepositoryUpdateGroupAsync(group, collections);
+        await ValidateAsync(organization, group, collections, userIds);
+
+        await SaveGroupWithCollectionsAsync(group, collections);
 
         if (userIds != null)
         {
-            await GroupRepositoryUpdateUsersAsync(group, userIds);
+            await SaveGroupUsersAsync(group, userIds);
         }
 
         await _eventService.LogGroupEventAsync(group, Core.Enums.EventType.Group_Updated);
     }
 
     public async Task UpdateGroupAsync(Group group, Organization organization, EventSystemUser systemUser,
-        ICollection<CollectionAccessSelection> collections = null,
-        IEnumerable<Guid> userIds = null)
+        ICollection<CollectionAccessSelection>? collections = null,
+        IEnumerable<Guid>? userIds = null)
     {
-        Validate(organization, group, collections);
-        await GroupRepositoryUpdateGroupAsync(group, collections);
+        await ValidateAsync(organization, group, collections, userIds);
+
+        await SaveGroupWithCollectionsAsync(group, collections);
 
         if (userIds != null)
         {
-            await GroupRepositoryUpdateUsersAsync(group, userIds, systemUser);
+            await SaveGroupUsersAsync(group, userIds, systemUser);
         }
 
         await _eventService.LogGroupEventAsync(group, Core.Enums.EventType.Group_Updated, systemUser);
     }
 
-    private async Task GroupRepositoryUpdateGroupAsync(Group group, IEnumerable<CollectionAccessSelection> collections = null)
+    private async Task SaveGroupWithCollectionsAsync(Group group, IEnumerable<CollectionAccessSelection>? collections = null)
     {
         group.RevisionDate = DateTime.UtcNow;
 
@@ -69,7 +76,7 @@
         }
     }
 
-    private async Task GroupRepositoryUpdateUsersAsync(Group group, IEnumerable<Guid> userIds, EventSystemUser? systemUser = null)
+    private async Task SaveGroupUsersAsync(Group group, IEnumerable<Guid> userIds, EventSystemUser? systemUser = null)
     {
         var newUserIds = userIds as Guid[] ?? userIds.ToArray();
         var originalUserIds = await _groupRepository.GetManyUserIdsByIdAsync(group.Id);
@@ -97,8 +104,12 @@
         }
     }
 
-    private static void Validate(Organization organization, Group group, IEnumerable<CollectionAccessSelection> collections)
+    private async Task ValidateAsync(Organization organization, Group group, ICollection<CollectionAccessSelection>? collectionAccess,
+        IEnumerable<Guid>? memberAccess)
     {
+        // Avoid multiple enumeration
+        memberAccess = memberAccess?.ToList();
+
         if (organization == null)
         {
             throw new BadRequestException("Organization not found");
@@ -109,18 +120,81 @@
             throw new BadRequestException("This organization cannot use groups.");
         }
 
+        var originalGroup = await _groupRepository.GetByIdAsync(group.Id);
+        if (originalGroup == null)
+        {
+            throw new NotFoundException("Group not found.");
+        }
+
+        if (originalGroup.OrganizationId != group.OrganizationId)
+        {
+            throw new BadRequestException("You cannot change a group's organization id.");
+        }
+
+        if (collectionAccess?.Any() == true)
+        {
+            await ValidateCollectionAccessAsync(originalGroup, collectionAccess);
+        }
+
+        if (memberAccess?.Any() == true)
+        {
+            await ValidateMemberAccessAsync(originalGroup, memberAccess.ToList());
+        }
+
         if (organization.FlexibleCollections)
         {
             if (group.AccessAll)
             {
                 throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the group to collections instead.");
             }
 
-            var invalidAssociations = collections?.Where(cas => cas.Manage && (cas.ReadOnly || cas.HidePasswords));
+            var invalidAssociations = collectionAccess?.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.");
             }
         }
     }
+
+    private async Task ValidateCollectionAccessAsync(Group originalGroup,
+        ICollection<CollectionAccessSelection> collectionAccess)
+    {
+        var collections = await _collectionRepository
+            .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id));
+        var collectionIds = collections.Select(c => c.Id);
+
+        var missingCollectionId = collectionAccess
+            .FirstOrDefault(cas => !collectionIds.Contains(cas.Id));
+        if (missingCollectionId != default)
+        {
+            throw new BadRequestException($"Invalid collection id {missingCollectionId}.");
+        }
+
+        var invalidCollection = collections.FirstOrDefault(c => c.OrganizationId != originalGroup.OrganizationId);
+        if (invalidCollection != default)
+        {
+            // Use generic error message to avoid enumeration
+            throw new BadRequestException($"Invalid collection id {invalidCollection.Id}.");
+        }
+    }
+
+    private async Task ValidateMemberAccessAsync(Group originalGroup,
+        ICollection<Guid> memberAccess)
+    {
+        var members = await _organizationUserRepository.GetManyAsync(memberAccess);
+        var memberIds = members.Select(g => g.Id);
+
+        var missingMemberId = memberAccess.FirstOrDefault(gId => !memberIds.Contains(gId));
+        if (missingMemberId != default)
+        {
+            throw new BadRequestException($"Invalid member id {missingMemberId}.");
+        }
+
+        var invalidMember = members.FirstOrDefault(g => g.OrganizationId != originalGroup.OrganizationId);
+        if (invalidMember != default)
+        {
+            // Use generic error message to avoid enumeration
+            throw new BadRequestException($"Invalid member id {invalidMember.Id}.");
+        }
+    }
 }

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs

@@ -6,5 +6,6 @@ namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interface
 
 public interface IUpdateOrganizationUserCommand
 {
-    Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, IEnumerable<CollectionAccessSelection> collections, IEnumerable<Guid>? groups);
+    Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId,
+        IEnumerable<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess);
 }