[AC-2847] Simplify OrganizationUser and Group PUT methods and tests (#4479)

* refactor controller logic
* add additional validation checks to update commands
* refactor and improve tests

src/Api/AdminConsole/Controllers/GroupsController.cs

@@ -135,7 +135,7 @@ public class GroupsController : Controller
                 .Succeeded;
             if (!authorized)
             {
-                throw new NotFoundException("You are not authorized to grant access to these collections.");
+                throw new NotFoundException();
             }
         }
 
@@ -175,13 +175,20 @@ public class GroupsController : Controller
     /// </summary>
     private async Task<GroupResponseModel> Put_vNext(Guid orgId, Guid id, [FromBody] GroupRequestModel model)
     {
-        var (group, currentAccess) = await _groupRepository.GetByIdWithCollectionsAsync(id);
-        if (group == null || !await _currentContext.ManageGroups(group.OrganizationId))
+        if (!await _currentContext.ManageGroups(orgId))
         {
             throw new NotFoundException();
         }
 
-        // Check whether the user is permitted to add themselves to the group
+        var (group, currentAccess) = await _groupRepository.GetByIdWithCollectionsAsync(id);
+        if (group == null || group.OrganizationId != orgId)
+        {
+            throw new NotFoundException();
+        }
+
+        // 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 @@ public class GroupsController : Controller
             }
         }
 
+        // 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 NotFoundException();
+            }
+        }
+
         // 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 @@ public class GroupsController : Controller
             }
         }
 
-        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

@@ -230,7 +230,7 @@ public class OrganizationUsersController : Controller
                 .Succeeded;
             if (!authorized)
             {
-                throw new NotFoundException("You are not authorized to grant access to these collections.");
+                throw new NotFoundException();
             }
         }
 
@@ -400,13 +400,15 @@ public class OrganizationUsersController : Controller
         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,23 @@ public class OrganizationUsersController : Controller
             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 NotFoundException();
+            }
+        }
+
         // 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 +449,6 @@ public class OrganizationUsersController : Controller
             }
         }
 
-        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 @@ public class UpdateGroupCommand : IUpdateGroupCommand
     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 @@ public class UpdateGroupCommand : IUpdateGroupCommand
         }
     }
 
-    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,11 +104,15 @@ public class UpdateGroupCommand : IUpdateGroupCommand
         }
     }
 
-    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)
     {
-        if (organization == null)
+        // Avoid multiple enumeration
+        memberAccess = memberAccess?.ToList();
+
+        if (organization == null || organization.Id != group.OrganizationId)
         {
-            throw new BadRequestException("Organization not found");
+            throw new NotFoundException();
         }
 
         if (!organization.UseGroups)
@@ -109,15 +120,73 @@ public class UpdateGroupCommand : IUpdateGroupCommand
             throw new BadRequestException("This organization cannot use groups.");
         }
 
+        var originalGroup = await _groupRepository.GetByIdAsync(group.Id);
+        if (originalGroup == null || originalGroup.OrganizationId != group.OrganizationId)
+        {
+            throw new NotFoundException();
+        }
+
+        if (collectionAccess?.Any() == true)
+        {
+            await ValidateCollectionAccessAsync(originalGroup, collectionAccess);
+        }
+
+        if (memberAccess?.Any() == true)
+        {
+            await ValidateMemberAccessAsync(originalGroup, memberAccess.ToList());
+        }
+
         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 missingCollection = collectionAccess
+            .FirstOrDefault(cas => !collectionIds.Contains(cas.Id));
+        if (missingCollection != default)
+        {
+            throw new NotFoundException();
+        }
+
+        var invalidCollection = collections.FirstOrDefault(c => c.OrganizationId != originalGroup.OrganizationId);
+        if (invalidCollection != default)
+        {
+            // Use generic error message to avoid enumeration
+            throw new NotFoundException();
+        }
+    }
+
+    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(mId => !memberIds.Contains(mId));
+        if (missingMemberId != default)
+        {
+            throw new NotFoundException();
+        }
+
+        var invalidMember = members.FirstOrDefault(m => m.OrganizationId != originalGroup.OrganizationId);
+        if (invalidMember != default)
+        {
+            // Use generic error message to avoid enumeration
+            throw new NotFoundException();
+        }
+    }
 }

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, List<CollectionAccessSelection> collections, IEnumerable<Guid>? groups);
+    Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId,
+        List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess);
 }

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs

@@ -1,5 +1,6 @@
 #nullable enable
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
+using Bit.Core.AdminConsole.Repositories;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
@@ -19,6 +20,8 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
     private readonly IOrganizationUserRepository _organizationUserRepository;
     private readonly ICountNewSmSeatsRequiredQuery _countNewSmSeatsRequiredQuery;
     private readonly IUpdateSecretsManagerSubscriptionCommand _updateSecretsManagerSubscriptionCommand;
+    private readonly ICollectionRepository _collectionRepository;
+    private readonly IGroupRepository _groupRepository;
 
     public UpdateOrganizationUserCommand(
         IEventService eventService,
@@ -26,7 +29,9 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
         IOrganizationRepository organizationRepository,
         IOrganizationUserRepository organizationUserRepository,
         ICountNewSmSeatsRequiredQuery countNewSmSeatsRequiredQuery,
-        IUpdateSecretsManagerSubscriptionCommand updateSecretsManagerSubscriptionCommand)
+        IUpdateSecretsManagerSubscriptionCommand updateSecretsManagerSubscriptionCommand,
+        ICollectionRepository collectionRepository,
+        IGroupRepository groupRepository)
     {
         _eventService = eventService;
         _organizationService = organizationService;
@@ -34,17 +39,45 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
         _organizationUserRepository = organizationUserRepository;
         _countNewSmSeatsRequiredQuery = countNewSmSeatsRequiredQuery;
         _updateSecretsManagerSubscriptionCommand = updateSecretsManagerSubscriptionCommand;
+        _collectionRepository = collectionRepository;
+        _groupRepository = groupRepository;
     }
 
+    /// <summary>
+    /// Update an organization user.
+    /// </summary>
+    /// <param name="user">The modified user to save.</param>
+    /// <param name="savingUserId">The userId of the currently logged in user who is making the change.</param>
+    /// <param name="collectionAccess">The user's updated collection access. If set to null, this removes all collection access.</param>
+    /// <param name="groupAccess">The user's updated group access. If set to null, groups are not updated.</param>
+    /// <exception cref="BadRequestException"></exception>
     public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId,
-        List<CollectionAccessSelection>? collections, IEnumerable<Guid>? groups)
+        List<CollectionAccessSelection>? collectionAccess, IEnumerable<Guid>? groupAccess)
     {
+        // Avoid multiple enumeration
+        collectionAccess = collectionAccess?.ToList();
+        groupAccess = groupAccess?.ToList();
+
         if (user.Id.Equals(default(Guid)))
         {
             throw new BadRequestException("Invite the user first.");
         }
 
         var originalUser = await _organizationUserRepository.GetByIdAsync(user.Id);
+        if (originalUser == null || user.OrganizationId != originalUser.OrganizationId)
+        {
+            throw new NotFoundException();
+        }
+
+        if (collectionAccess?.Any() == true)
+        {
+            await ValidateCollectionAccessAsync(originalUser, collectionAccess.ToList());
+        }
+
+        if (groupAccess?.Any() == true)
+        {
+            await ValidateGroupAccessAsync(originalUser, groupAccess.ToList());
+        }
 
         if (savingUserId.HasValue)
         {
@@ -64,9 +97,9 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
             throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the user to collections instead.");
         }
 
-        if (collections?.Count > 0)
+        if (collectionAccess?.Count > 0)
         {
-            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())
             {
                 throw new BadRequestException("The Manage property is mutually exclusive and cannot be true while the ReadOnly or HidePasswords properties are also true.");
@@ -87,13 +120,55 @@ public class UpdateOrganizationUserCommand : IUpdateOrganizationUserCommand
             }
         }
 
-        await _organizationUserRepository.ReplaceAsync(user, collections);
+        await _organizationUserRepository.ReplaceAsync(user, collectionAccess);
 
-        if (groups != null)
+        if (groupAccess != null)
         {
-            await _organizationUserRepository.UpdateGroupsAsync(user.Id, groups);
+            await _organizationUserRepository.UpdateGroupsAsync(user.Id, groupAccess);
         }
 
         await _eventService.LogOrganizationUserEventAsync(user, EventType.OrganizationUser_Updated);
     }
+
+    private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser,
+        ICollection<CollectionAccessSelection> collectionAccess)
+    {
+        var collections = await _collectionRepository
+            .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id));
+        var collectionIds = collections.Select(c => c.Id);
+
+        var missingCollection = collectionAccess
+            .FirstOrDefault(cas => !collectionIds.Contains(cas.Id));
+        if (missingCollection != default)
+        {
+            throw new NotFoundException();
+        }
+
+        var invalidCollection = collections.FirstOrDefault(c => c.OrganizationId != originalUser.OrganizationId);
+        if (invalidCollection != default)
+        {
+            // Use generic error message to avoid enumeration
+            throw new NotFoundException();
+        }
+    }
+
+    private async Task ValidateGroupAccessAsync(OrganizationUser originalUser,
+        ICollection<Guid> groupAccess)
+    {
+        var groups = await _groupRepository.GetManyByManyIds(groupAccess);
+        var groupIds = groups.Select(g => g.Id);
+
+        var missingGroupId = groupAccess.FirstOrDefault(gId => !groupIds.Contains(gId));
+        if (missingGroupId != default)
+        {
+            throw new NotFoundException();
+        }
+
+        var invalidGroup = groups.FirstOrDefault(g => g.OrganizationId != originalUser.OrganizationId);
+        if (invalidGroup != default)
+        {
+            // Use generic error message to avoid enumeration
+            throw new NotFoundException();
+        }
+    }
 }