From 4ff544233efb4a7b6542ed0ed37ce89cbdf2f891 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 09:17:49 +1000 Subject: [PATCH 01/17] Move PUT tests into separate file --- .../OrganizationUserControllerPutTests.cs | 288 ++++++++++++++++++ .../OrganizationUsersControllerTests.cs | 265 ---------------- 2 files changed, 288 insertions(+), 265 deletions(-) create mode 100644 test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs new file mode 100644 index 000000000000..1e3ce00a814f --- /dev/null +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -0,0 +1,288 @@ +using System.Security.Claims; +using Bit.Api.AdminConsole.Controllers; +using Bit.Api.AdminConsole.Models.Request.Organizations; +using Bit.Api.Models.Request; +using Bit.Api.Vault.AuthorizationHandlers.Collections; +using Bit.Core; +using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; +using Bit.Core.Context; +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Utilities; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using NSubstitute; +using Xunit; +namespace Bit.Api.Test.AdminConsole.Controllers; + +[ControllerCustomize(typeof(OrganizationUsersController))] +[SutProviderCustomize] +public class OrganizationUserControllerPutTests +{ + [Theory] + [BitAutoData] + public async Task Put_Success(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(false); + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); + + // Save these for later - organizationUser object will be mutated + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), + savingUserId, + Arg.Is>(cas => + cas.All(c => model.Collections.Any(m => m.Id == c.Id))), + model.Groups); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_CannotAddSelfToCollections(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + // Updating self + organizationUser.UserId = savingUserId; + organizationAbility.AllowAdminAccessToAllCollectionItems = false; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + + // User is not currently assigned to any collections, which means they're adding themselves + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + new List())); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List()); + + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + Assert.Contains("You cannot add yourself to a collection.", exception.Message); + } + [Theory] + [BitAutoData] + public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_DoesNotUpdateGroups(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + // Updating self + organizationUser.UserId = savingUserId; + organizationAbility.AllowAdminAccessToAllCollectionItems = false; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); + + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), + savingUserId, + Arg.Is>(cas => + cas.All(c => model.Collections.Any(m => m.Id == c.Id))), + null); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateSelf_WithAllowAdminAccessToAllCollectionItems_DoesUpdateGroups(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + // Updating self + organizationUser.UserId = savingUserId; + organizationAbility.AllowAdminAccessToAllCollectionItems = true; + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); + + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), + savingUserId, + Arg.Is>(cas => + cas.All(c => model.Collections.Any(m => m.Id == c.Id))), + model.Groups); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + + var editedCollectionId = CoreHelpers.GenerateComb(); + var readonlyCollectionId1 = CoreHelpers.GenerateComb(); + var readonlyCollectionId2 = CoreHelpers.GenerateComb(); + + var currentCollectionAccess = new List + { + new() + { + Id = editedCollectionId, + HidePasswords = true, + Manage = false, + ReadOnly = true + }, + new() + { + Id = readonlyCollectionId1, + HidePasswords = false, + Manage = true, + ReadOnly = false + }, + new() + { + Id = readonlyCollectionId2, + HidePasswords = false, + Manage = false, + ReadOnly = false + }, + }; + + // User is upgrading editedCollectionId to manage + model.Collections = new List + { + new() { Id = editedCollectionId, HidePasswords = false, Manage = true, ReadOnly = false } + }; + + // Save these for later - organizationUser object will be mutated + var orgUserId = organizationUser.Id; + var orgUserEmail = organizationUser.Email; + + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + currentCollectionAccess)); + + var currentCollections = currentCollectionAccess + .Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(currentCollections); + + // Authorize the editedCollection + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Success()); + + // Do not authorize the readonly collections + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == readonlyCollectionId1 || c.Id == readonlyCollectionId2), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Failed()); + + await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); + + // Expect all collection access (modified and unmodified) to be saved + await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => + ou.Type == model.Type && + ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && + ou.AccessSecretsManager == model.AccessSecretsManager && + ou.Id == orgUserId && + ou.Email == orgUserEmail), + savingUserId, + Arg.Is>(cas => + cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) && + cas.First(c => c.Id == editedCollectionId).Manage == true && + cas.First(c => c.Id == editedCollectionId).ReadOnly == false && + cas.First(c => c.Id == editedCollectionId).HidePasswords == false), + model.Groups); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) + { + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); + var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) + .Returns(collections); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Failed()); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + Assert.Contains("You must have Can Manage permission", exception.Message); + } + + private void Put_Setup(SutProvider sutProvider, OrganizationAbility organizationAbility, + OrganizationUser organizationUser, Guid savingUserId, OrganizationUserUpdateRequestModel model, bool authorizeAll) + { + var orgId = organizationAbility.Id = organizationUser.OrganizationId; + + sutProvider.GetDependency().ManageUsers(orgId).Returns(true); + sutProvider.GetDependency().GetByIdAsync(organizationUser.Id).Returns(organizationUser); + sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) + .Returns(organizationAbility); + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); + + if (authorizeAll) + { + // Simple case: saving user can edit all collections, all collection access is replaced + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); + var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) + .Returns(collections); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Success()); + } + } +} diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index f2898db2e4a2..97b9a4a1eeef 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -1,13 +1,11 @@ using System.Security.Claims; using Bit.Api.AdminConsole.Controllers; using Bit.Api.AdminConsole.Models.Request.Organizations; -using Bit.Api.Models.Request; using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; -using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Entities; using Bit.Core.Auth.Repositories; @@ -187,239 +185,6 @@ public async Task Invite_NotAuthorizedToGiveAccessToCollections_Throws(Organizat Assert.Contains("You are not authorized", exception.Message); } - [Theory] - [BitAutoData] - public async Task Put_Success(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(false); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); - - // Save these for later - organizationUser object will be mutated - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); - - await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => - ou.Type == model.Type && - ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && - ou.AccessSecretsManager == model.AccessSecretsManager && - ou.Id == orgUserId && - ou.Email == orgUserEmail), - savingUserId, - Arg.Is>(cas => - cas.All(c => model.Collections.Any(m => m.Id == c.Id))), - model.Groups); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_CannotAddSelfToCollections(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - // Updating self - organizationUser.UserId = savingUserId; - organizationAbility.AllowAdminAccessToAllCollectionItems = false; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); - - // User is not currently assigned to any collections, which means they're adding themselves - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - new List())); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List()); - - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); - Assert.Contains("You cannot add yourself to a collection.", exception.Message); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_DoesNotUpdateGroups(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - // Updating self - organizationUser.UserId = savingUserId; - organizationAbility.AllowAdminAccessToAllCollectionItems = false; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); - - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); - - await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => - ou.Type == model.Type && - ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && - ou.AccessSecretsManager == model.AccessSecretsManager && - ou.Id == orgUserId && - ou.Email == orgUserEmail), - savingUserId, - Arg.Is>(cas => - cas.All(c => model.Collections.Any(m => m.Id == c.Id))), - null); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateSelf_WithAllowAdminAccessToAllCollectionItems_DoesUpdateGroups(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - // Updating self - organizationUser.UserId = savingUserId; - organizationAbility.AllowAdminAccessToAllCollectionItems = true; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); - - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); - - await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => - ou.Type == model.Type && - ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && - ou.AccessSecretsManager == model.AccessSecretsManager && - ou.Id == orgUserId && - ou.Email == orgUserEmail), - savingUserId, - Arg.Is>(cas => - cas.All(c => model.Collections.Any(m => m.Id == c.Id))), - model.Groups); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); - - var editedCollectionId = CoreHelpers.GenerateComb(); - var readonlyCollectionId1 = CoreHelpers.GenerateComb(); - var readonlyCollectionId2 = CoreHelpers.GenerateComb(); - - var currentCollectionAccess = new List - { - new() - { - Id = editedCollectionId, - HidePasswords = true, - Manage = false, - ReadOnly = true - }, - new() - { - Id = readonlyCollectionId1, - HidePasswords = false, - Manage = true, - ReadOnly = false - }, - new() - { - Id = readonlyCollectionId2, - HidePasswords = false, - Manage = false, - ReadOnly = false - }, - }; - - // User is upgrading editedCollectionId to manage - model.Collections = new List - { - new() { Id = editedCollectionId, HidePasswords = false, Manage = true, ReadOnly = false } - }; - - // Save these for later - organizationUser object will be mutated - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - currentCollectionAccess)); - - var currentCollections = currentCollectionAccess - .Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(currentCollections); - - // Authorize the editedCollection - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) - .Returns(AuthorizationResult.Success()); - - // Do not authorize the readonly collections - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == readonlyCollectionId1 || c.Id == readonlyCollectionId2), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) - .Returns(AuthorizationResult.Failed()); - - await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model); - - // Expect all collection access (modified and unmodified) to be saved - await sutProvider.GetDependency().Received(1).UpdateUserAsync(Arg.Is(ou => - ou.Type == model.Type && - ou.Permissions == CoreHelpers.ClassToJsonData(model.Permissions) && - ou.AccessSecretsManager == model.AccessSecretsManager && - ou.Id == orgUserId && - ou.Email == orgUserEmail), - savingUserId, - Arg.Is>(cas => - cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) && - cas.First(c => c.Id == editedCollectionId).Manage == true && - cas.First(c => c.Id == editedCollectionId).ReadOnly == false && - cas.First(c => c.Id == editedCollectionId).HidePasswords == false), - model.Groups); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(OrganizationUserUpdateRequestModel model, - OrganizationUser organizationUser, OrganizationAbility organizationAbility, - SutProvider sutProvider, Guid savingUserId) - { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); - - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); - var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) - .Returns(collections); - - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) - .Returns(AuthorizationResult.Failed()); - - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); - Assert.Contains("You must have Can Manage permission", exception.Message); - } - [Theory] [BitAutoData] public async Task Get_ReturnsUsers( @@ -536,36 +301,6 @@ public async Task GetAccountRecoveryDetails_WithoutManageResetPasswordPermission await Assert.ThrowsAsync(async () => await sutProvider.Sut.GetAccountRecoveryDetails(organizationId, bulkRequestModel)); } - private void Put_Setup(SutProvider sutProvider, OrganizationAbility organizationAbility, - OrganizationUser organizationUser, Guid savingUserId, OrganizationUserUpdateRequestModel model, bool authorizeAll) - { - var orgId = organizationAbility.Id = organizationUser.OrganizationId; - - sutProvider.GetDependency().ManageUsers(orgId).Returns(true); - sutProvider.GetDependency().GetByIdAsync(organizationUser.Id).Returns(organizationUser); - sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) - .Returns(organizationAbility); - sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); - - if (authorizeAll) - { - // Simple case: saving user can edit all collections, all collection access is replaced - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); - var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) - .Returns(collections); - - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), - Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyUserAccess))) - .Returns(AuthorizationResult.Success()); - } - } - private void Get_Setup(OrganizationAbility organizationAbility, ICollection organizationUsers, SutProvider sutProvider) From c51d2bc33e217203b80e56403c9f525d78c54be9 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 10:23:24 +1000 Subject: [PATCH 02/17] Fix sut and add tests --- .../OrganizationUsersController.cs | 27 +++- .../OrganizationUserControllerPutTests.cs | 140 ++++++++++++------ 2 files changed, 117 insertions(+), 50 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index fd8149454b80..d3d2c89c5e23 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -402,13 +402,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(); @@ -419,9 +421,23 @@ 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 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) 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)); @@ -435,11 +451,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 diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index 1e3ce00a814f..e3a227f33190 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -18,6 +18,7 @@ using Microsoft.AspNetCore.Authorization; using NSubstitute; using Xunit; + namespace Bit.Api.Test.AdminConsole.Controllers; [ControllerCustomize(typeof(OrganizationUsersController))] @@ -30,9 +31,8 @@ public async Task Put_Success(OrganizationUserUpdateRequestModel model, OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(false); - - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + Put_Setup_AuthorizeAll(sutProvider, organizationUser, model); // Save these for later - organizationUser object will be mutated var orgUserId = organizationUser.Id; @@ -54,18 +54,17 @@ await sutProvider.GetDependency().Received(1).Up [Theory] [BitAutoData] - public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_CannotAddSelfToCollections(OrganizationUserUpdateRequestModel model, + public async Task Put_NoAdminAccess_CannotAddSelfToCollections(OrganizationUserUpdateRequestModel model, OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { // Updating self organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = false; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); - // User is not currently assigned to any collections, which means they're adding themselves + // User is not currently assigned to any collections, so all this collection access is new sutProvider.GetDependency() .GetByIdWithCollectionsAsync(organizationUser.Id) .Returns(new Tuple>(organizationUser, @@ -74,24 +73,30 @@ public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_Can .GetManyByManyIdsAsync(Arg.Any>()) .Returns(new List()); - var orgUserId = organizationUser.Id; - var orgUserEmail = organizationUser.Email; - var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); Assert.Contains("You cannot add yourself to a collection.", exception.Message); } [Theory] [BitAutoData] - public async Task Put_UpdateSelf_WithoutAllowAdminAccessToAllCollectionItems_DoesNotUpdateGroups(OrganizationUserUpdateRequestModel model, + public async Task Put_NoAdminAccess_CannotAddSelfToGroups(OrganizationUserUpdateRequestModel model, OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { // Updating self organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = false; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + + // Not changing any collection access + model.Collections = new List(); + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + new List())); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List()); var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; @@ -107,21 +112,31 @@ await sutProvider.GetDependency().Received(1).Up savingUserId, Arg.Is>(cas => cas.All(c => model.Collections.Any(m => m.Id == c.Id))), + // Main assertion: groups are not updated (are null) null); } [Theory] [BitAutoData] - public async Task Put_UpdateSelf_WithAllowAdminAccessToAllCollectionItems_DoesUpdateGroups(OrganizationUserUpdateRequestModel model, + public async Task Put_WithAdminAccess_CanAddSelfToGroups(OrganizationUserUpdateRequestModel model, OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { // Updating self organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = true; - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, true); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + + // Not changing any collection access + model.Collections = new List(); + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + new List())); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(new List()); var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; @@ -142,12 +157,12 @@ await sutProvider.GetDependency().Received(1).Up [Theory] [BitAutoData] - public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(OrganizationUserUpdateRequestModel model, + public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections(OrganizationUserUpdateRequestModel model, OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + Put_Setup_AuthorizeAll(sutProvider, organizationUser, model); var editedCollectionId = CoreHelpers.GenerateComb(); var readonlyCollectionId1 = CoreHelpers.GenerateComb(); @@ -196,8 +211,15 @@ public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUp var currentCollections = currentCollectionAccess .Select(cas => new Collection { Id = cas.Id }).ToList(); sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) + .GetManyByManyIdsAsync(Arg.Is>(guids => guids + .ToHashSet() + .SetEquals(currentCollectionAccess.Select(c => c.Id).ToHashSet()))) .Returns(currentCollections); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Is>(guids => guids + .ToHashSet() + .SetEquals(model.Collections.Select(c => c.Id).ToHashSet()))) + .Returns(model.Collections.Select(cas => new Collection { Id = cas.Id}).ToList()); // Authorize the editedCollection sutProvider.GetDependency() @@ -235,9 +257,9 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, model, false); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + // Target user is currently assigned to the POSTed collections sutProvider.GetDependency() .GetByIdWithCollectionsAsync(organizationUser.Id) .Returns(new Tuple>(organizationUser, @@ -247,6 +269,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) .Returns(collections); + // But the saving user does not have permission to update them sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) @@ -256,33 +279,66 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection Assert.Contains("You must have Can Manage permission", exception.Message); } - private void Put_Setup(SutProvider sutProvider, OrganizationAbility organizationAbility, - OrganizationUser organizationUser, Guid savingUserId, OrganizationUserUpdateRequestModel model, bool authorizeAll) + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(OrganizationUserUpdateRequestModel model, + OrganizationUser organizationUser, OrganizationAbility organizationAbility, + SutProvider sutProvider, Guid savingUserId) { + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + + // The target user is not currently assigned to any collections, so we're granting access for the first time + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + new List())); + var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) + .Returns(collections); + + // But the saving user does not have permission to assign access to the collections + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Failed()); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); + Assert.Contains("You must have Can Manage permission", exception.Message); + } + + private void Put_Setup(SutProvider sutProvider, + OrganizationAbility organizationAbility, OrganizationUser organizationUser, Guid savingUserId) + { + // FCv1 is now fully enabled + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + var orgId = organizationAbility.Id = organizationUser.OrganizationId; sutProvider.GetDependency().ManageUsers(orgId).Returns(true); - sutProvider.GetDependency().GetByIdAsync(organizationUser.Id).Returns(organizationUser); + sutProvider.GetDependency().GetByIdAsync(organizationUser.Id) + .Returns(organizationUser); sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) .Returns(organizationAbility); sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); + } - if (authorizeAll) - { - // Simple case: saving user can edit all collections, all collection access is replaced - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); - var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) - .Returns(collections); - - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), - Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyUserAccess))) - .Returns(AuthorizationResult.Success()); - } + private void Put_Setup_AuthorizeAll(SutProvider sutProvider, + OrganizationUser organizationUser, OrganizationUserUpdateRequestModel model) + { + // Simple case: saving user can edit all collections, all collection access is replaced + sutProvider.GetDependency() + .GetByIdWithCollectionsAsync(organizationUser.Id) + .Returns(new Tuple>(organizationUser, + model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); + var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) + .Returns(collections); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Success()); } } From 2ceb86eb456c21b4cffb6c30773dad55b0d28e10 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 10:41:46 +1000 Subject: [PATCH 03/17] Simplify tests --- .../OrganizationUserControllerPutTests.cs | 112 +++++------------- 1 file changed, 27 insertions(+), 85 deletions(-) diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index e3a227f33190..037cf9155ad3 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -32,7 +32,12 @@ public async Task Put_Success(OrganizationUserUpdateRequestModel model, SutProvider sutProvider, Guid savingUserId) { Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); - Put_Setup_AuthorizeAll(sutProvider, organizationUser, model); + + // Authorize all changes for basic happy path test + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Any(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) + .Returns(AuthorizationResult.Success()); // Save these for later - organizationUser object will be mutated var orgUserId = organizationUser.Id; @@ -62,16 +67,7 @@ public async Task Put_NoAdminAccess_CannotAddSelfToCollections(OrganizationUserU organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = false; - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); - - // User is not currently assigned to any collections, so all this collection access is new - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - new List())); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List()); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); Assert.Contains("You cannot add yourself to a collection.", exception.Message); @@ -86,17 +82,10 @@ public async Task Put_NoAdminAccess_CannotAddSelfToGroups(OrganizationUserUpdate organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = false; - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); // Not changing any collection access model.Collections = new List(); - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - new List())); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List()); var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; @@ -126,17 +115,10 @@ public async Task Put_WithAdminAccess_CanAddSelfToGroups(OrganizationUserUpdateR organizationUser.UserId = savingUserId; organizationAbility.AllowAdminAccessToAllCollectionItems = true; - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); // Not changing any collection access model.Collections = new List(); - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - new List())); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(new List()); var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; @@ -161,9 +143,6 @@ public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections( OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); - Put_Setup_AuthorizeAll(sutProvider, organizationUser, model); - var editedCollectionId = CoreHelpers.GenerateComb(); var readonlyCollectionId1 = CoreHelpers.GenerateComb(); var readonlyCollectionId2 = CoreHelpers.GenerateComb(); @@ -193,6 +172,8 @@ public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections( }, }; + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess); + // User is upgrading editedCollectionId to manage model.Collections = new List { @@ -203,24 +184,6 @@ public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections( var orgUserId = organizationUser.Id; var orgUserEmail = organizationUser.Email; - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - currentCollectionAccess)); - - var currentCollections = currentCollectionAccess - .Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids - .ToHashSet() - .SetEquals(currentCollectionAccess.Select(c => c.Id).ToHashSet()))) - .Returns(currentCollections); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids - .ToHashSet() - .SetEquals(model.Collections.Select(c => c.Id).ToHashSet()))) - .Returns(model.Collections.Select(cas => new Collection { Id = cas.Id}).ToList()); - // Authorize the editedCollection sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), @@ -257,21 +220,15 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); - // Target user is currently assigned to the POSTed collections - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); - var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) - .Returns(collections); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, + currentCollectionAccess: model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList()); + + var postedCollectionIds = model.Collections.Select(c => c.Id).ToHashSet(); // But the saving user does not have permission to update them sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); @@ -285,21 +242,13 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(O OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); - // The target user is not currently assigned to any collections, so we're granting access for the first time - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(organizationUser.Id) - .Returns(new Tuple>(organizationUser, - new List())); - var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) - .Returns(collections); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); + var postedCollectionIds = model.Collections.Select(c => c.Id).ToHashSet(); // But the saving user does not have permission to assign access to the collections sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); @@ -308,7 +257,8 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(O } private void Put_Setup(SutProvider sutProvider, - OrganizationAbility organizationAbility, OrganizationUser organizationUser, Guid savingUserId) + OrganizationAbility organizationAbility, OrganizationUser organizationUser, Guid savingUserId, + List currentCollectionAccess = null) { // FCv1 is now fully enabled sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); @@ -321,24 +271,16 @@ private void Put_Setup(SutProvider sutProvider, sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) .Returns(organizationAbility); sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); - } - private void Put_Setup_AuthorizeAll(SutProvider sutProvider, - OrganizationUser organizationUser, OrganizationUserUpdateRequestModel model) - { - // Simple case: saving user can edit all collections, all collection access is replaced + // OrganizationUserRepository: return the user with current collection access sutProvider.GetDependency() .GetByIdWithCollectionsAsync(organizationUser.Id) .Returns(new Tuple>(organizationUser, - model.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); - var collections = model.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) - .Returns(collections); + currentCollectionAccess ?? [])); - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), - Arg.Is>(r => r.Contains(BulkCollectionOperations.ModifyUserAccess))) - .Returns(AuthorizationResult.Success()); + // Collection repository: return mock Collection objects for any ids passed in + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>().Select(guid => new Collection { Id = guid }).ToList()); } } From 9e9b4a3fa9df86b278088822e68e9ecbc2d1f894 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 10:50:36 +1000 Subject: [PATCH 04/17] Dont require list when Ienumerable will do --- .../Controllers/OrganizationUserControllerPutTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index 037cf9155ad3..60e626ee45d5 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -52,7 +52,7 @@ await sutProvider.GetDependency().Received(1).Up ou.Id == orgUserId && ou.Email == orgUserEmail), savingUserId, - Arg.Is>(cas => + Arg.Is>(cas => cas.All(c => model.Collections.Any(m => m.Id == c.Id))), model.Groups); } @@ -99,7 +99,7 @@ await sutProvider.GetDependency().Received(1).Up ou.Id == orgUserId && ou.Email == orgUserEmail), savingUserId, - Arg.Is>(cas => + Arg.Is>(cas => cas.All(c => model.Collections.Any(m => m.Id == c.Id))), // Main assertion: groups are not updated (are null) null); @@ -132,7 +132,7 @@ await sutProvider.GetDependency().Received(1).Up ou.Id == orgUserId && ou.Email == orgUserEmail), savingUserId, - Arg.Is>(cas => + Arg.Is>(cas => cas.All(c => model.Collections.Any(m => m.Id == c.Id))), model.Groups); } @@ -206,7 +206,7 @@ await sutProvider.GetDependency().Received(1).Up ou.Id == orgUserId && ou.Email == orgUserEmail), savingUserId, - Arg.Is>(cas => + Arg.Is>(cas => cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) && cas.First(c => c.Id == editedCollectionId).Manage == true && cas.First(c => c.Id == editedCollectionId).ReadOnly == false && From 402a1be98a68d7b62afcf088ead48030cd0b4ec8 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 12:08:47 +1000 Subject: [PATCH 05/17] Tests: remove null default --- .../Controllers/OrganizationUserControllerPutTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index 60e626ee45d5..4efa029fc42f 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -31,7 +31,7 @@ public async Task Put_Success(OrganizationUserUpdateRequestModel model, OrganizationUser organizationUser, OrganizationAbility organizationAbility, SutProvider sutProvider, Guid savingUserId) { - Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId); + Put_Setup(sutProvider, organizationAbility, organizationUser, savingUserId, currentCollectionAccess: []); // Authorize all changes for basic happy path test sutProvider.GetDependency() @@ -258,7 +258,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(O private void Put_Setup(SutProvider sutProvider, OrganizationAbility organizationAbility, OrganizationUser organizationUser, Guid savingUserId, - List currentCollectionAccess = null) + List currentCollectionAccess) { // FCv1 is now fully enabled sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); From f30a70dad3e4d47e1cbb9e759c8e89b43075a0c4 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 13:28:27 +1000 Subject: [PATCH 06/17] Refine existing GroupController tests --- .../Controllers/GroupsControllerPutTests.cs | 273 +++++++++++++++ .../Controllers/GroupsControllerTests.cs | 313 ------------------ 2 files changed, 273 insertions(+), 313 deletions(-) create mode 100644 test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs new file mode 100644 index 000000000000..555dc8c981eb --- /dev/null +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs @@ -0,0 +1,273 @@ +using System.Security.Claims; +using Bit.Api.AdminConsole.Controllers; +using Bit.Api.AdminConsole.Models.Request; +using Bit.Api.Models.Request; +using Bit.Api.Vault.AuthorizationHandlers.Collections; +using Bit.Core; +using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Context; +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Models.Data; +using Bit.Core.Models.Data.Organizations; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Core.Utilities; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; +using NSubstitute; +using Xunit; + +namespace Bit.Api.Test.AdminConsole.Controllers; + +[ControllerCustomize(typeof(GroupsController))] +[SutProviderCustomize] +public class GroupsControllerPutTests +{ + [Theory] + [BitAutoData] + public async Task Put_WithAdminAccess_Success(Organization organization, Group group, + GroupRequestModel groupRequestModel, List existingCollectionAccess, + OrganizationUser savingUser, SutProvider sutProvider) + { + Put_Setup(sutProvider, organization, true, group, savingUser, existingCollectionAccess, []); + + var requestModelCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + + // Authorize all changes for basic happy path test + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Any(), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Success()); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), + Arg.Is(o => o.Id == organization.Id), + // Should overwrite any existing collections + Arg.Is>(access => + access.All(c => requestModelCollectionIds.Contains(c.Id))), + Arg.Is>(guids => guids.ToHashSet().SetEquals(groupRequestModel.Users.ToHashSet()))); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_NoAdminAccess_CannotAddSelfToGroup(Organization organization, Group group, + GroupRequestModel groupRequestModel, OrganizationUser savingUser, List currentGroupUsers, + SutProvider sutProvider) + { + // Not updating collections + groupRequestModel.Collections = []; + + Put_Setup(sutProvider, organization, false, group, savingUser, + currentCollectionAccess: [], currentGroupUsers); + + // Saving user is trying to add themselves to the group + var updatedUsers = groupRequestModel.Users.ToList(); + updatedUsers.Add(savingUser.Id); + groupRequestModel.Users = updatedUsers; + + var exception = await + Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); + + Assert.Contains("You cannot add yourself to groups", exception.Message); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_NoAdminAccess_AlreadyInGroup_Success(Organization organization, Group group, + GroupRequestModel groupRequestModel, OrganizationUser savingUser, List currentGroupUsers, + SutProvider sutProvider) + { + // Not changing collection access + groupRequestModel.Collections = []; + + // Saving user is trying to add themselves to the group + var updatedUsers = groupRequestModel.Users.ToList(); + updatedUsers.Add(savingUser.Id); + groupRequestModel.Users = updatedUsers; + + // But! they are already a member of the group + currentGroupUsers.Add(savingUser.Id); + + Put_Setup(sutProvider, organization, false, group, savingUser, currentCollectionAccess: [], currentGroupUsers); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), + Arg.Is(o => o.Id == organization.Id), + Arg.Any>(), + Arg.Any>()); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_NoAdminAccess_ProviderUser_Success(Organization organization, Group group, + GroupRequestModel groupRequestModel, List currentGroupUsers, SutProvider sutProvider) + { + // Make collection authorization pass, it's not being tested here + groupRequestModel.Collections = Array.Empty(); + + Put_Setup(sutProvider, organization, false, group, null, currentCollectionAccess: [], currentGroupUsers); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), + Arg.Is(o => o.Id == organization.Id), + Arg.Any>(), + Arg.Any>()); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_DoesNotOverwriteUnauthorizedCollections(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, OrganizationUser savingUser) + { + var editedCollectionId = CoreHelpers.GenerateComb(); + var readonlyCollectionId1 = CoreHelpers.GenerateComb(); + var readonlyCollectionId2 = CoreHelpers.GenerateComb(); + + var currentCollectionAccess = new List + { + new() + { + Id = editedCollectionId, + HidePasswords = true, + Manage = false, + ReadOnly = true + }, + new() + { + Id = readonlyCollectionId1, + HidePasswords = false, + Manage = true, + ReadOnly = false + }, + new() + { + Id = readonlyCollectionId2, + HidePasswords = false, + Manage = false, + ReadOnly = false + }, + }; + + Put_Setup(sutProvider, organization, false, group, savingUser, currentCollectionAccess, currentGroupUsers: []); + + // User is upgrading editedCollectionId to manage + groupRequestModel.Collections = new List + { + new() { Id = editedCollectionId, HidePasswords = false, Manage = true, ReadOnly = false } + }; + + // Authorize the editedCollection + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Success()); + + // Do not authorize the readonly collections + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == readonlyCollectionId1 || c.Id == readonlyCollectionId2), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + // Expect all collection access (modified and unmodified) to be saved + await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), + Arg.Is(o => o.Id == organization.Id), + Arg.Is>(cas => + cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) && + cas.First(c => c.Id == editedCollectionId).Manage == true && + cas.First(c => c.Id == editedCollectionId).ReadOnly == false && + cas.First(c => c.Id == editedCollectionId).HidePasswords == false), + Arg.Any>()); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, OrganizationUser savingUser) + { + // Group is currently assigned to the POSTed collections + Put_Setup(sutProvider, organization, false, group, savingUser, + groupRequestModel.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList(), + []); + + var postedCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + + // But the saving user does not have permission to update them + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); + Assert.Contains("You must have Can Manage permission", exception.Message); + } + + private void Put_Setup(SutProvider sutProvider, Organization organization, + bool adminAccess, Group group, OrganizationUser? savingUser, List currentCollectionAccess, + List currentGroupUsers) + { + // FCv1 is now fully enabled + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); + + var orgId = organization.Id = group.OrganizationId; + + // Arrange org and orgAbility + sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); + sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) + .Returns(new OrganizationAbility + { + Id = organization.Id, + AllowAdminAccessToAllCollectionItems = adminAccess + }); + + // Arrange user + // If no savingUser provided, they're not an org user, just return a random guid + sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUser?.UserId ?? CoreHelpers.GenerateComb()); + sutProvider.GetDependency().ManageGroups(orgId).Returns(true); + + // Arrange repositories + sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id).Returns(currentGroupUsers ?? []); + sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) + .Returns(new Tuple>(group, currentCollectionAccess ?? [])); + if (savingUser != null) + { + sutProvider.GetDependency().GetByOrganizationAsync(orgId, savingUser.UserId.Value) + .Returns(savingUser); + } + + // Collection repository: return mock Collection objects for any ids passed in + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>().Select(guid => new Collection { Id = guid }).ToList()); + } +} diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs index 6f077fbd3e90..f3227310fd03 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs @@ -1,12 +1,10 @@ using System.Security.Claims; using Bit.Api.AdminConsole.Controllers; using Bit.Api.AdminConsole.Models.Request; -using Bit.Api.Models.Request; using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; -using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Exceptions; @@ -14,7 +12,6 @@ using Bit.Core.Models.Data.Organizations; using Bit.Core.Repositories; using Bit.Core.Services; -using Bit.Core.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Microsoft.AspNetCore.Authorization; @@ -118,314 +115,4 @@ public async Task Post_NotAuthorizedToGiveAccessToCollections_Throws(Organizatio await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .CreateGroupAsync(default, default, default, default); } - - [Theory] - [BitAutoData] - public async Task Put_AdminsCanAccessAllCollections_Success(Organization organization, Group group, - GroupRequestModel groupRequestModel, List existingCollectionAccess, - SutProvider sutProvider) - { - group.OrganizationId = organization.Id; - - // Enable FC and v1, set Collection Management Setting - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility { Id = organization.Id, AllowAdminAccessToAllCollectionItems = true }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) - .Returns(new Tuple>(group, existingCollectionAccess)); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(existingCollectionAccess.Select(c => c.Id)) - .Returns(existingCollectionAccess.Select(c => new Collection { Id = c.Id }).ToList()); - sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); - - var requestModelCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); - - var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); - - await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); - await sutProvider.GetDependency().Received(1).UpdateGroupAsync( - Arg.Is(g => - g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), - Arg.Is(o => o.Id == organization.Id), - // Should overwrite any existing collections - Arg.Is>(access => - access.All(c => requestModelCollectionIds.Contains(c.Id))), - Arg.Any>()); - Assert.Equal(groupRequestModel.Name, response.Name); - Assert.Equal(organization.Id, response.OrganizationId); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateMembers_AdminsCannotAccessAllCollections_CannotAddSelfToGroup(Organization organization, Group group, - GroupRequestModel groupRequestModel, OrganizationUser savingOrganizationUser, List currentGroupUsers, - SutProvider sutProvider) - { - group.OrganizationId = organization.Id; - - // Enable FC and v1 - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility - { - Id = organization.Id, - AllowAdminAccessToAllCollectionItems = false, - }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - // Saving user is trying to add themselves to the group - var updatedUsers = groupRequestModel.Users.ToList(); - updatedUsers.Add(savingOrganizationUser.Id); - groupRequestModel.Users = updatedUsers; - - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) - .Returns(new Tuple>(group, new List())); - sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); - sutProvider.GetDependency() - .GetByOrganizationAsync(organization.Id, Arg.Any()) - .Returns(savingOrganizationUser); - sutProvider.GetDependency().GetProperUserId(Arg.Any()) - .Returns(savingOrganizationUser.UserId); - sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id) - .Returns(currentGroupUsers); - - var exception = await - Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); - - Assert.Contains("You cannot add yourself to groups", exception.Message); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateMembers_AdminsCannotAccessAllCollections_AlreadyInGroup_Success(Organization organization, Group group, - GroupRequestModel groupRequestModel, OrganizationUser savingOrganizationUser, List currentGroupUsers, - SutProvider sutProvider) - { - group.OrganizationId = organization.Id; - - // Enable FC and v1 - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility - { - Id = organization.Id, - AllowAdminAccessToAllCollectionItems = false, - }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - // Saving user is trying to add themselves to the group - var updatedUsers = groupRequestModel.Users.ToList(); - updatedUsers.Add(savingOrganizationUser.Id); - groupRequestModel.Users = updatedUsers; - - // But! they are already a member of the group - currentGroupUsers.Add(savingOrganizationUser.Id); - - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) - .Returns(new Tuple>(group, new List())); - sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); - sutProvider.GetDependency() - .GetByOrganizationAsync(organization.Id, Arg.Any()) - .Returns(savingOrganizationUser); - sutProvider.GetDependency().GetProperUserId(Arg.Any()) - .Returns(savingOrganizationUser.UserId); - sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id) - .Returns(currentGroupUsers); - - // Make collection authorization pass, it's not being tested here - groupRequestModel.Collections = Array.Empty(); - - var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); - - await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); - await sutProvider.GetDependency().Received(1).UpdateGroupAsync( - Arg.Is(g => - g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), - Arg.Is(o => o.Id == organization.Id), - Arg.Any>(), - Arg.Any>()); - Assert.Equal(groupRequestModel.Name, response.Name); - Assert.Equal(organization.Id, response.OrganizationId); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateMembers_AdminsCannotAccessAllCollections_ProviderUser_Success(Organization organization, Group group, - GroupRequestModel groupRequestModel, List currentGroupUsers, Guid savingUserId, - SutProvider sutProvider) - { - group.OrganizationId = organization.Id; - - // Enable FC and v1 - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( - new OrganizationAbility - { - Id = organization.Id, - AllowAdminAccessToAllCollectionItems = false, - }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().GetByIdWithCollectionsAsync(group.Id) - .Returns(new Tuple>(group, new List())); - sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); - sutProvider.GetDependency() - .GetByOrganizationAsync(organization.Id, Arg.Any()) - .Returns((OrganizationUser)null); // Provider is not an OrganizationUser, so it will always return null - sutProvider.GetDependency().GetProperUserId(Arg.Any()) - .Returns(savingUserId); - sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id) - .Returns(currentGroupUsers); - - // Make collection authorization pass, it's not being tested here - groupRequestModel.Collections = Array.Empty(); - - var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); - - await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); - await sutProvider.GetDependency().Received(1).UpdateGroupAsync( - Arg.Is(g => - g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), - Arg.Is(o => o.Id == organization.Id), - Arg.Any>(), - Arg.Any>()); - Assert.Equal(groupRequestModel.Name, response.Name); - Assert.Equal(organization.Id, response.OrganizationId); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateCollections_OnlyUpdatesCollectionsTheSavingUserCanUpdate(GroupRequestModel groupRequestModel, - Group group, Organization organization, - SutProvider sutProvider, Guid savingUserId) - { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organization, group, savingUserId); - - var editedCollectionId = CoreHelpers.GenerateComb(); - var readonlyCollectionId1 = CoreHelpers.GenerateComb(); - var readonlyCollectionId2 = CoreHelpers.GenerateComb(); - - var currentCollectionAccess = new List - { - new() - { - Id = editedCollectionId, - HidePasswords = true, - Manage = false, - ReadOnly = true - }, - new() - { - Id = readonlyCollectionId1, - HidePasswords = false, - Manage = true, - ReadOnly = false - }, - new() - { - Id = readonlyCollectionId2, - HidePasswords = false, - Manage = false, - ReadOnly = false - }, - }; - - // User is upgrading editedCollectionId to manage - groupRequestModel.Collections = new List - { - new() { Id = editedCollectionId, HidePasswords = false, Manage = true, ReadOnly = false } - }; - - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(group.Id) - .Returns(new Tuple>(group, - currentCollectionAccess)); - - var currentCollections = currentCollectionAccess - .Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Any>()) - .Returns(currentCollections); - - // Authorize the editedCollection - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == editedCollectionId), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) - .Returns(AuthorizationResult.Success()); - - // Do not authorize the readonly collections - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => c.Id == readonlyCollectionId1 || c.Id == readonlyCollectionId2), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) - .Returns(AuthorizationResult.Failed()); - - var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); - - // Expect all collection access (modified and unmodified) to be saved - await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); - await sutProvider.GetDependency().Received(1).UpdateGroupAsync( - Arg.Is(g => - g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), - Arg.Is(o => o.Id == organization.Id), - Arg.Is>(cas => - cas.Select(c => c.Id).SequenceEqual(currentCollectionAccess.Select(c => c.Id)) && - cas.First(c => c.Id == editedCollectionId).Manage == true && - cas.First(c => c.Id == editedCollectionId).ReadOnly == false && - cas.First(c => c.Id == editedCollectionId).HidePasswords == false), - Arg.Any>()); - Assert.Equal(groupRequestModel.Name, response.Name); - Assert.Equal(organization.Id, response.OrganizationId); - } - - [Theory] - [BitAutoData] - public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollections(GroupRequestModel groupRequestModel, - Group group, Organization organization, - SutProvider sutProvider, Guid savingUserId) - { - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - Put_Setup(sutProvider, organization, group, savingUserId); - - sutProvider.GetDependency() - .GetByIdWithCollectionsAsync(group.Id) - .Returns(new Tuple>(group, - groupRequestModel.Collections.Select(cas => cas.ToSelectionReadOnly()).ToList())); - var collections = groupRequestModel.Collections.Select(cas => new Collection { Id = cas.Id }).ToList(); - sutProvider.GetDependency() - .GetManyByManyIdsAsync(Arg.Is>(guids => guids.SequenceEqual(collections.Select(c => c.Id)))) - .Returns(collections); - - sutProvider.GetDependency() - .AuthorizeAsync(Arg.Any(), Arg.Is(c => collections.Contains(c)), - Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) - .Returns(AuthorizationResult.Failed()); - - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); - Assert.Contains("You must have Can Manage permission", exception.Message); - } - - private void Put_Setup(SutProvider sutProvider, Organization organization, - Group group, Guid savingUserId) - { - var orgId = organization.Id = group.OrganizationId; - - sutProvider.GetDependency().ManageGroups(orgId).Returns(true); - sutProvider.GetDependency().GetOrganizationAbilityAsync(orgId) - .Returns(new OrganizationAbility - { - Id = organization.Id, - AllowAdminAccessToAllCollectionItems = false - }); - - sutProvider.GetDependency().GetManyUserIdsByIdAsync(group.Id).Returns(new List()); - sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(savingUserId); - sutProvider.GetDependency().GetByOrganizationAsync(orgId, savingUserId).Returns(new OrganizationUser - { - Id = savingUserId - }); - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - } } From 7bf8ac60f67e4ab970136ed227625c892b233bcd Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 13:30:43 +1000 Subject: [PATCH 07/17] Add test --- .../Controllers/GroupsControllerPutTests.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs index 555dc8c981eb..2e8badfc9a68 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs @@ -113,6 +113,35 @@ await sutProvider.GetDependency().Received(1).UpdateGroupAs Assert.Equal(organization.Id, response.OrganizationId); } + [Theory] + [BitAutoData] + public async Task Put_UpdateMembers_WithAdminAccess_CanAddSelfToGroup(Organization organization, Group group, + GroupRequestModel groupRequestModel, OrganizationUser savingUser, List currentGroupUsers, + SutProvider sutProvider) + { + // Not updating collections + groupRequestModel.Collections = []; + + Put_Setup(sutProvider, organization, true, group, savingUser, + currentCollectionAccess: [], currentGroupUsers); + + // Saving user is trying to add themselves to the group + var updatedUsers = groupRequestModel.Users.ToList(); + updatedUsers.Add(savingUser.Id); + groupRequestModel.Users = updatedUsers; + + var response = await sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel); + + await sutProvider.GetDependency().Received(1).UpdateGroupAsync( + Arg.Is(g => + g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), + Arg.Is(o => o.Id == organization.Id), + Arg.Any>(), + Arg.Is>(guids => guids.ToHashSet().SetEquals(groupRequestModel.Users.ToHashSet()))); + Assert.Equal(groupRequestModel.Name, response.Name); + Assert.Equal(organization.Id, response.OrganizationId); + } + [Theory] [BitAutoData] public async Task Put_UpdateMembers_NoAdminAccess_ProviderUser_Success(Organization organization, Group group, From 893817532d84d096a2286cecc8dc28af361aa400 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 13:32:03 +1000 Subject: [PATCH 08/17] Add test --- .../Controllers/GroupsController.cs | 34 ++++++++++++++----- .../Controllers/GroupsControllerPutTests.cs | 21 ++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index 9749691583c6..0f99943a3207 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -175,13 +175,20 @@ await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, /// private async Task 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 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] } } + // 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 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] } } - 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 diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs index 2e8badfc9a68..970cc002f6f7 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs @@ -261,6 +261,27 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection Assert.Contains("You must have Can Manage permission", exception.Message); } + [Theory] + [BitAutoData] + public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(GroupRequestModel groupRequestModel, + Group group, Organization organization, + SutProvider sutProvider, OrganizationUser savingUser) + { + // Group is not assigned to the POSTed collections + Put_Setup(sutProvider, organization, false, group, savingUser, [], []); + + var postedCollectionIds = groupRequestModel.Collections.Select(c => c.Id).ToHashSet(); + + // But the saving user does not have permission to update them + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), Arg.Is(c => postedCollectionIds.Contains(c.Id)), + Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) + .Returns(AuthorizationResult.Failed()); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); + Assert.Contains("You must have Can Manage permission", exception.Message); + } + private void Put_Setup(SutProvider sutProvider, Organization organization, bool adminAccess, Group group, OrganizationUser? savingUser, List currentCollectionAccess, List currentGroupUsers) From 0de626baeffdec8ed0de30c373123c150ce69980 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 9 Jul 2024 13:42:33 +1000 Subject: [PATCH 09/17] dotnet format --- src/Api/AdminConsole/Controllers/GroupsController.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index 0f99943a3207..ae63a893daa1 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -175,7 +175,7 @@ await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, /// private async Task Put_vNext(Guid orgId, Guid id, [FromBody] GroupRequestModel model) { - if (!await _currentContext.ManageGroups(orgId)) + if (!await _currentContext.ManageGroups(orgId)) { throw new NotFoundException(); } From 790d7c1eff213358cdd3361ca056abdb50f2f019 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Fri, 12 Jul 2024 09:29:49 +1000 Subject: [PATCH 10/17] Verify orgIds --- .../OrganizationUsersController.cs | 3 +- .../IUpdateOrganizationUserCommand.cs | 3 +- .../UpdateOrganizationUserCommand.cs | 63 ++++++- .../UpdateOrganizationUserCommandTests.cs | 166 ++++++++++++++++-- 4 files changed, 206 insertions(+), 29 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 5597a00e4e7d..5941196280bd 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -429,7 +429,8 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd BulkCollectionOperations.ModifyUserAccess)) .Succeeded) { - throw new BadRequestException("You must have Can Manage permissions to edit a collection's membership"); + throw new BadRequestException( + $"You must have Can Manage permissions to edit the membership for collection id {collection.Id}."); } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs index 254a97b75efb..6f774504808c 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/Interfaces/IUpdateOrganizationUserCommand.cs +++ b/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 collections, IEnumerable? groups); + Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, + IEnumerable? collectionAccess, IEnumerable? groupAccess); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index 8dcac12ddf43..7b6f634f264b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/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 UpdateOrganizationUserCommand( IOrganizationRepository organizationRepository, IOrganizationUserRepository organizationUserRepository, ICountNewSmSeatsRequiredQuery countNewSmSeatsRequiredQuery, - IUpdateSecretsManagerSubscriptionCommand updateSecretsManagerSubscriptionCommand) + IUpdateSecretsManagerSubscriptionCommand updateSecretsManagerSubscriptionCommand, + ICollectionRepository collectionRepository, + IGroupRepository groupRepository) { _eventService = eventService; _organizationService = organizationService; @@ -34,11 +39,25 @@ public UpdateOrganizationUserCommand( _organizationUserRepository = organizationUserRepository; _countNewSmSeatsRequiredQuery = countNewSmSeatsRequiredQuery; _updateSecretsManagerSubscriptionCommand = updateSecretsManagerSubscriptionCommand; + _collectionRepository = collectionRepository; + _groupRepository = groupRepository; } + /// + /// Update an organization user. + /// + /// The modified user to save. + /// The userId of the currently logged in user who is making the change. + /// The user's updated collection access. If set to null, this removes all collection access. + /// The user's updated group access. If set to null, groups are not updated. + /// public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, - IEnumerable collections, IEnumerable? groups) + IEnumerable? collectionAccess, IEnumerable? groupAccess) { + // Avoid multiple enumeration + collectionAccess = collectionAccess?.ToList(); + groupAccess = groupAccess?.ToList(); + if (user.Id.Equals(default(Guid))) { throw new BadRequestException("Invite the user first."); @@ -46,6 +65,34 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, var originalUser = await _organizationUserRepository.GetByIdAsync(user.Id); + if (user.OrganizationId != originalUser.OrganizationId) + { + throw new BadRequestException("You cannot change a member's organization id."); + } + + if (collectionAccess?.Any() == true) + { + var collections = await _collectionRepository + .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); + if (collections.Count != collectionAccess.Count() || + collections.Any(c => c.OrganizationId != originalUser.OrganizationId)) + { + throw new BadRequestException( + "A collection does not exist or you do not have permission to grant access to it."); + } + } + + if (groupAccess?.Any() == true) + { + var groups = await _groupRepository.GetManyByManyIds(groupAccess); + if (groups.Count != groupAccess.Count() || + groups.Any(g => g.OrganizationId != originalUser.OrganizationId)) + { + throw new BadRequestException( + "A group does not exist or you do not have permission to grant access to it."); + } + } + if (savingUserId.HasValue) { await _organizationService.ValidateOrganizationUserUpdatePermissions(user.OrganizationId, user.Type, originalUser.Type, user.GetPermissions()); @@ -66,9 +113,9 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, throw new BadRequestException("The AccessAll property has been deprecated by collection enhancements. Assign the user to collections instead."); } - if (organization.FlexibleCollections && collections?.Any() == true) + if (organization.FlexibleCollections && collectionAccess?.Any() == true) { - 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."); @@ -92,13 +139,13 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, if (user.AccessAll) { // We don't need any collections if we're flagged to have all access. - collections = new List(); + collectionAccess = new List(); } - 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); diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs index ce2981f35a19..a37e4b72b9ec 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs @@ -1,6 +1,7 @@ using System.Text.Json; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers; +using Bit.Core.AdminConsole.Repositories; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; @@ -21,7 +22,7 @@ public class UpdateOrganizationUserCommandTests { [Theory, BitAutoData] public async Task UpdateUserAsync_NoUserId_Throws(OrganizationUser user, Guid? savingUserId, - ICollection collections, IEnumerable groups, SutProvider sutProvider) + ICollection collections, List groups, SutProvider sutProvider) { user.Id = default(Guid); var exception = await Assert.ThrowsAsync( @@ -29,41 +30,138 @@ public async Task UpdateUserAsync_NoUserId_Throws(OrganizationUser user, Guid? s Assert.Contains("invite the user first", exception.Message.ToLowerInvariant()); } + [Theory, BitAutoData] + public async Task UpdateUserAsync_DifferentOrganizationId_Throws(OrganizationUser user, OrganizationUser originalUser, + Guid? savingUserId, SutProvider sutProvider) + { + sutProvider.GetDependency().GetByIdAsync(user.Id).Returns(originalUser); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, null)); + Assert.Contains("cannot change a member's organization id", exception.Message.ToLowerInvariant()); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_CollectionsBelongToDifferentOrganization_Throws(OrganizationUser user, OrganizationUser originalUser, + ICollection collectionAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return collections with different organizationIds from the repository + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); + + Assert.Contains("collection does not exist or you do not have permission to grant access", + exception.Message.ToLowerInvariant()); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_CollectionsDoNotExist_Throws(OrganizationUser user, OrganizationUser originalUser, + ICollection collectionAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return matching collections, except that 1 is missing + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = user.OrganizationId }).ToList(); + result.RemoveAt(0); + return result; + }); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); + + Assert.Contains("collection does not exist or you do not have permission to grant access", + exception.Message.ToLowerInvariant()); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_GroupsBelongToDifferentOrganization_Throws(OrganizationUser user, OrganizationUser originalUser, + ICollection groupAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return collections with different organizationIds from the repository + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); + + Assert.Contains("group does not exist or you do not have permission to grant access", + exception.Message.ToLowerInvariant()); + } + + [Theory, BitAutoData] + public async Task UpdateUserAsync_GroupsDoNotExist_Throws(OrganizationUser user, OrganizationUser originalUser, + ICollection groupAccess, Guid? savingUserId, SutProvider sutProvider, + Organization organization) + { + Setup(sutProvider, organization, user, originalUser); + + // Return matching collections, except that 1 is missing + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList(); + result.RemoveAt(0); + return result; + }); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); + + Assert.Contains("group does not exist or you do not have permission to grant access", + exception.Message.ToLowerInvariant()); + } + [Theory, BitAutoData] public async Task UpdateUserAsync_Passes( Organization organization, OrganizationUser oldUserData, OrganizationUser newUserData, ICollection collections, - IEnumerable groups, + List groups, Permissions permissions, [OrganizationUser(type: OrganizationUserType.Owner)] OrganizationUser savingUser, SutProvider sutProvider) { - var organizationRepository = sutProvider.GetDependency(); - var organizationUserRepository = sutProvider.GetDependency(); - var organizationService = sutProvider.GetDependency(); - - organizationRepository.GetByIdAsync(organization.Id).Returns(organization); + Setup(sutProvider, organization, newUserData, oldUserData); - newUserData.Id = oldUserData.Id; - newUserData.UserId = oldUserData.UserId; - newUserData.OrganizationId = savingUser.OrganizationId = oldUserData.OrganizationId = organization.Id; newUserData.Permissions = JsonSerializer.Serialize(permissions, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase, }); - organizationUserRepository.GetByIdAsync(oldUserData.Id).Returns(oldUserData); - organizationUserRepository.GetManyByOrganizationAsync(savingUser.OrganizationId, OrganizationUserType.Owner) - .Returns(new List { savingUser }); - organizationService - .HasConfirmedOwnersExceptAsync( - newUserData.OrganizationId, - Arg.Is>(i => i.Contains(newUserData.Id))) - .Returns(true); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); + + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); await sutProvider.Sut.UpdateUserAsync(newUserData, savingUser.UserId, collections, groups); + var organizationService = sutProvider.GetDependency(); await organizationService.Received(1).ValidateOrganizationUserUpdatePermissions( newUserData.OrganizationId, newUserData.Type, @@ -84,7 +182,7 @@ public async Task UpdateUserAsync_WithFlexibleCollections_WithAccessAll_Throws( [OrganizationUser(type: OrganizationUserType.User)] OrganizationUser newUserData, [OrganizationUser(type: OrganizationUserType.Owner, status: OrganizationUserStatusType.Confirmed)] OrganizationUser savingUser, ICollection collections, - IEnumerable groups, + List groups, SutProvider sutProvider) { organization.FlexibleCollections = true; @@ -94,6 +192,16 @@ public async Task UpdateUserAsync_WithFlexibleCollections_WithAccessAll_Throws( newUserData.Permissions = CoreHelpers.ClassToJsonData(new Permissions()); newUserData.AccessAll = true; + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); + + sutProvider.GetDependency() + .GetManyByManyIds(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Group { Id = guid, OrganizationId = oldUserData.OrganizationId }).ToList()); + sutProvider.GetDependency() .GetByIdAsync(organization.Id) .Returns(organization); @@ -117,4 +225,24 @@ public async Task UpdateUserAsync_WithFlexibleCollections_WithAccessAll_Throws( Assert.Contains("the accessall property has been deprecated", exception.Message.ToLowerInvariant()); } + + private void Setup(SutProvider sutProvider, Organization organization, + OrganizationUser newUser, OrganizationUser oldUser) + { + var organizationRepository = sutProvider.GetDependency(); + var organizationUserRepository = sutProvider.GetDependency(); + var organizationService = sutProvider.GetDependency(); + + organizationRepository.GetByIdAsync(organization.Id).Returns(organization); + + newUser.Id = oldUser.Id; + newUser.UserId = oldUser.UserId; + newUser.OrganizationId = oldUser.OrganizationId = organization.Id; + organizationUserRepository.GetByIdAsync(oldUser.Id).Returns(oldUser); + organizationService + .HasConfirmedOwnersExceptAsync( + oldUser.OrganizationId, + Arg.Is>(i => i.Contains(oldUser.Id))) + .Returns(true); + } } From 836e4bb9f9fa1daf8e734506d9f966b5d487ab65 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Fri, 12 Jul 2024 10:31:58 +1000 Subject: [PATCH 11/17] Improve group validation --- .../Groups/UpdateGroupCommand.cs | 77 ++++++-- .../UpdateOrganizationUserCommand.cs | 4 + .../Groups/UpdateGroupCommandTests.cs | 168 +++++++++++++++++- 3 files changed, 227 insertions(+), 22 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index ba2aa2b8bd81..3a2a165678fc 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/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 collections = null, - IEnumerable userIds = null) + ICollection? collections = null, + IEnumerable? 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 collections = null, - IEnumerable userIds = null) + ICollection? collections = null, + IEnumerable? 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 collections = null) + private async Task SaveGroupWithCollectionsAsync(Group group, IEnumerable? collections = null) { group.RevisionDate = DateTime.UtcNow; @@ -69,7 +76,7 @@ private async Task GroupRepositoryUpdateGroupAsync(Group group, IEnumerable userIds, EventSystemUser? systemUser = null) + private async Task SaveGroupUsersAsync(Group group, IEnumerable userIds, EventSystemUser? systemUser = null) { var newUserIds = userIds as Guid[] ?? userIds.ToArray(); var originalUserIds = await _groupRepository.GetManyUserIdsByIdAsync(group.Id); @@ -97,8 +104,12 @@ await _eventService.LogOrganizationUserEventsAsync(users.Select(u => } } - private static void Validate(Organization organization, Group group, IEnumerable collections) + private async Task ValidateAsync(Organization organization, Group group, ICollection? collectionAccess, + IEnumerable? memberAccess) { + // Avoid multiple enumeration + memberAccess = memberAccess?.ToList(); + if (organization == null) { throw new BadRequestException("Organization not found"); @@ -109,6 +120,40 @@ private static void Validate(Organization organization, Group group, IEnumerable 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) + { + var collections = await _collectionRepository + .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); + if (collections.Count != collectionAccess.Count() || + collections.Any(c => c.OrganizationId != originalGroup.OrganizationId)) + { + throw new BadRequestException( + "A collection does not exist or you do not have permission to grant access to it."); + } + } + + if (memberAccess?.Any() == true) + { + var organizationUsers = await _organizationUserRepository.GetManyAsync(memberAccess); + if (organizationUsers.Count != memberAccess.Count() || + organizationUsers.Any(ou => ou.OrganizationId != originalGroup.OrganizationId)) + { + throw new BadRequestException( + "A member does not exist or you do not have permission to modify their group access."); + } + } + if (organization.FlexibleCollections) { if (group.AccessAll) @@ -116,7 +161,7 @@ private static void Validate(Organization organization, Group group, IEnumerable 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."); diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index 7b6f634f264b..8481cb842c8d 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -64,6 +64,10 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, } var originalUser = await _organizationUserRepository.GetByIdAsync(user.Id); + if (originalUser == null) + { + throw new NotFoundException("User not found."); + } if (user.OrganizationId != originalUser.OrganizationId) { diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs index 1b21574fdb0f..98efb0871be6 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs @@ -1,11 +1,14 @@ using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups; using Bit.Core.AdminConsole.Repositories; +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.OrganizationFixtures; +using Bit.Core.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Bit.Test.Common.Helpers; @@ -18,8 +21,13 @@ namespace Bit.Core.Test.AdminConsole.OrganizationFeatures.Groups; public class UpdateGroupCommandTests { [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] - public async Task UpdateGroup_Success(SutProvider sutProvider, Group group, Organization organization) + public async Task UpdateGroup_Success(SutProvider sutProvider, Group group, Group oldGroup, + Organization organization) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + await sutProvider.Sut.UpdateGroupAsync(group, organization); await sutProvider.GetDependency().Received(1).ReplaceAsync(group); @@ -28,8 +36,13 @@ public async Task UpdateGroup_Success(SutProvider sutProvide } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = false), BitAutoData] - public async Task UpdateGroup_WithCollections_Success(SutProvider sutProvider, Group group, Organization organization, List collections) + public async Task UpdateGroup_WithCollections_Success(SutProvider sutProvider, Group group, + Group oldGroup, Organization organization, List collections) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + await sutProvider.Sut.UpdateGroupAsync(group, organization, collections); await sutProvider.GetDependency().Received(1).ReplaceAsync(group, collections); @@ -38,8 +51,13 @@ public async Task UpdateGroup_WithCollections_Success(SutProvider sutProvider, Group group, Organization organization, EventSystemUser eventSystemUser) + public async Task UpdateGroup_WithEventSystemUser_Success(SutProvider sutProvider, Group group, + Group oldGroup, Organization organization, EventSystemUser eventSystemUser) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + await sutProvider.Sut.UpdateGroupAsync(group, organization, eventSystemUser); await sutProvider.GetDependency().Received(1).ReplaceAsync(group); @@ -48,8 +66,13 @@ public async Task UpdateGroup_WithEventSystemUser_Success(SutProvider sutProvider, Group group, EventSystemUser eventSystemUser) + public async Task UpdateGroup_WithNullOrganization_Throws(SutProvider sutProvider, Group group, + Group oldGroup, EventSystemUser eventSystemUser) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, null, eventSystemUser)); Assert.Contains("Organization not found", exception.Message); @@ -59,8 +82,13 @@ public async Task UpdateGroup_WithNullOrganization_Throws(SutProvider sutProvider, Organization organization, Group group, EventSystemUser eventSystemUser) + public async Task UpdateGroup_WithUseGroupsAsFalse_Throws(SutProvider sutProvider, + Organization organization, Group group, Group oldGroup, EventSystemUser eventSystemUser) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + var exception = await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, organization, eventSystemUser)); Assert.Contains("This organization cannot use groups", exception.Message); @@ -71,8 +99,12 @@ public async Task UpdateGroup_WithUseGroupsAsFalse_Throws(SutProvider sutProvider, Organization organization, Group group) + SutProvider sutProvider, Organization organization, Group group, Group oldGroup) { + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + group.AccessAll = true; organization.FlexibleCollections = true; @@ -83,4 +115,128 @@ public async Task UpdateGroup_WithFlexibleCollections_WithAccessAll_Throws( await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().CreateAsync(default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogGroupEventAsync(default, default, default); } + + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_GroupBelongsToDifferentOrganization_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + ArrangeCollections(sutProvider, group); + + // Mismatching orgId + oldGroup.OrganizationId = CoreHelpers.GenerateComb(); + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateGroupAsync(group, organization)); + Assert.Contains("cannot change a group's organization id", exception.Message); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_CollectionsBelongsToDifferentOrganization_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, List collectionAccess) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); + Assert.Contains("A collection does not exist or you do not have permission", exception.Message); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_CollectionsDoNotExist_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, List collectionAccess) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeUsers(sutProvider, group); + + // Return result is missing a collection + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new Collection { Id = guid, OrganizationId = group.OrganizationId }).ToList(); + result.RemoveAt(0); + return result; + }); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); + Assert.Contains("A collection does not exist or you do not have permission", exception.Message); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_MemberBelongsToDifferentOrganization_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, IEnumerable userAccess) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeCollections(sutProvider, group); + + sutProvider.GetDependency() + .GetManyAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new OrganizationUser { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); + Assert.Contains("A member does not exist or you do not have permission", exception.Message); + } + + [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] + public async Task UpdateGroup_MemberDoesNotExist_Throws(SutProvider sutProvider, + Group group, Group oldGroup, Organization organization, IEnumerable userAccess) + { + group.AccessAll = false; + ArrangeGroup(sutProvider, group, oldGroup); + ArrangeCollections(sutProvider, group); + + sutProvider.GetDependency() + .GetManyAsync(Arg.Any>()) + .Returns(callInfo => + { + var result = callInfo.Arg>() + .Select(guid => new OrganizationUser { Id = guid, OrganizationId = group.OrganizationId }) + .ToList(); + result.RemoveAt(0); + return result; + }); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); + Assert.Contains("A member does not exist or you do not have permission", exception.Message); + } + + private void ArrangeGroup(SutProvider sutProvider, Group group, Group oldGroup) + { + oldGroup.OrganizationId = group.OrganizationId; + oldGroup.Id = group.Id; + sutProvider.GetDependency().GetByIdAsync(group.Id).Returns(oldGroup); + } + + private void ArrangeCollections(SutProvider sutProvider, Group group) + { + sutProvider.GetDependency() + .GetManyByManyIdsAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new Collection() { Id = guid, OrganizationId = group.OrganizationId }).ToList()); + } + + private void ArrangeUsers(SutProvider sutProvider, Group group) + { + sutProvider.GetDependency() + .GetManyAsync(Arg.Any>()) + .Returns(callInfo => callInfo.Arg>() + .Select(guid => new OrganizationUser { Id = guid, OrganizationId = group.OrganizationId }).ToList()); + } } From 9438ce822470ba9c725f842ef41e8730176d8f34 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Fri, 12 Jul 2024 11:16:04 +1000 Subject: [PATCH 12/17] Refactor to return invalid ids --- .../Groups/UpdateGroupCommand.cs | 58 ++++++++++++++----- .../UpdateOrganizationUserCommand.cs | 58 ++++++++++++++----- .../Groups/UpdateGroupCommandTests.cs | 8 +-- .../UpdateOrganizationUserCommandTests.cs | 8 +-- 4 files changed, 94 insertions(+), 38 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index 3a2a165678fc..4c4c4822d3d4 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs @@ -133,25 +133,12 @@ private async Task ValidateAsync(Organization organization, Group group, ICollec if (collectionAccess?.Any() == true) { - var collections = await _collectionRepository - .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); - if (collections.Count != collectionAccess.Count() || - collections.Any(c => c.OrganizationId != originalGroup.OrganizationId)) - { - throw new BadRequestException( - "A collection does not exist or you do not have permission to grant access to it."); - } + await ValidateCollectionAccessAsync(originalGroup, collectionAccess); } if (memberAccess?.Any() == true) { - var organizationUsers = await _organizationUserRepository.GetManyAsync(memberAccess); - if (organizationUsers.Count != memberAccess.Count() || - organizationUsers.Any(ou => ou.OrganizationId != originalGroup.OrganizationId)) - { - throw new BadRequestException( - "A member does not exist or you do not have permission to modify their group access."); - } + await ValidateMemberAccessAsync(originalGroup, memberAccess.ToList()); } if (organization.FlexibleCollections) @@ -168,4 +155,45 @@ private async Task ValidateAsync(Organization organization, Group group, ICollec } } } + + private async Task ValidateCollectionAccessAsync(Group originalGroup, + ICollection 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) + { + throw new BadRequestException($"Invalid collection id {invalidCollection.Id}."); + } + } + + private async Task ValidateMemberAccessAsync(Group originalGroup, + ICollection 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}."); + } + } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index 8481cb842c8d..1cbaff396d3b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -76,25 +76,12 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, if (collectionAccess?.Any() == true) { - var collections = await _collectionRepository - .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); - if (collections.Count != collectionAccess.Count() || - collections.Any(c => c.OrganizationId != originalUser.OrganizationId)) - { - throw new BadRequestException( - "A collection does not exist or you do not have permission to grant access to it."); - } + await ValidateCollectionAccessAsync(originalUser, collectionAccess.ToList()); } if (groupAccess?.Any() == true) { - var groups = await _groupRepository.GetManyByManyIds(groupAccess); - if (groups.Count != groupAccess.Count() || - groups.Any(g => g.OrganizationId != originalUser.OrganizationId)) - { - throw new BadRequestException( - "A group does not exist or you do not have permission to grant access to it."); - } + await ValidateGroupAccessAsync(originalUser, groupAccess.ToList()); } if (savingUserId.HasValue) @@ -154,4 +141,45 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, await _eventService.LogOrganizationUserEventAsync(user, EventType.OrganizationUser_Updated); } + + private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser, + ICollection 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 != originalUser.OrganizationId); + if (invalidCollection != default) + { + throw new BadRequestException($"Invalid collection id {invalidCollection.Id}."); + } + } + + private async Task ValidateGroupAccessAsync(OrganizationUser originalUser, + ICollection 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 BadRequestException($"Invalid group id {missingGroupId}."); + } + + var invalidGroup = groups.FirstOrDefault(g => g.OrganizationId != originalUser.OrganizationId); + if (invalidGroup != default) + { + // Use generic error message to avoid enumeration + throw new BadRequestException($"Invalid group id {invalidGroup.Id}."); + } + } } diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs index 98efb0871be6..93ac841968a8 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs @@ -148,7 +148,7 @@ public async Task UpdateGroup_CollectionsBelongsToDifferentOrganization_Throws(S var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); - Assert.Contains("A collection does not exist or you do not have permission", exception.Message); + Assert.Contains("Invalid collection id", exception.Message); } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] @@ -172,7 +172,7 @@ public async Task UpdateGroup_CollectionsDoNotExist_Throws(SutProvider( () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); - Assert.Contains("A collection does not exist or you do not have permission", exception.Message); + Assert.Contains("Invalid collection id", exception.Message); } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] @@ -190,7 +190,7 @@ public async Task UpdateGroup_MemberBelongsToDifferentOrganization_Throws(SutPro var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); - Assert.Contains("A member does not exist or you do not have permission", exception.Message); + Assert.Contains("Invalid member id", exception.Message); } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] @@ -214,7 +214,7 @@ public async Task UpdateGroup_MemberDoesNotExist_Throws(SutProvider( () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); - Assert.Contains("A member does not exist or you do not have permission", exception.Message); + Assert.Contains("Invalid member id", exception.Message); } private void ArrangeGroup(SutProvider sutProvider, Group group, Group oldGroup) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs index a37e4b72b9ec..40f9c7fc208e 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs @@ -57,7 +57,7 @@ public async Task UpdateUserAsync_CollectionsBelongToDifferentOrganization_Throw var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); - Assert.Contains("collection does not exist or you do not have permission to grant access", + Assert.Contains("invalid collection id", exception.Message.ToLowerInvariant()); } @@ -82,7 +82,7 @@ public async Task UpdateUserAsync_CollectionsDoNotExist_Throws(OrganizationUser var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); - Assert.Contains("collection does not exist or you do not have permission to grant access", + Assert.Contains("invalid collection id", exception.Message.ToLowerInvariant()); } @@ -102,7 +102,7 @@ public async Task UpdateUserAsync_GroupsBelongToDifferentOrganization_Throws(Org var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); - Assert.Contains("group does not exist or you do not have permission to grant access", + Assert.Contains("invalid group id", exception.Message.ToLowerInvariant()); } @@ -127,7 +127,7 @@ public async Task UpdateUserAsync_GroupsDoNotExist_Throws(OrganizationUser user, var exception = await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); - Assert.Contains("group does not exist or you do not have permission to grant access", + Assert.Contains("invalid group id", exception.Message.ToLowerInvariant()); } From b5194e4d218f1428d6cf7ee46dee9dc14a928cc7 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Fri, 12 Jul 2024 11:23:11 +1000 Subject: [PATCH 13/17] Add comment --- .../OrganizationFeatures/Groups/UpdateGroupCommand.cs | 1 + .../OrganizationUsers/UpdateOrganizationUserCommand.cs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index 4c4c4822d3d4..485f93a2daa9 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs @@ -173,6 +173,7 @@ private async Task ValidateCollectionAccessAsync(Group originalGroup, 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}."); } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index 1cbaff396d3b..da1b4e9532f8 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -159,6 +159,7 @@ private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser, var invalidCollection = collections.FirstOrDefault(c => c.OrganizationId != originalUser.OrganizationId); if (invalidCollection != default) { + // Use generic error message to avoid enumeration throw new BadRequestException($"Invalid collection id {invalidCollection.Id}."); } } From 1f738777d470ccefbe339838f74b0e9719c78280 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 16 Jul 2024 08:50:09 +1000 Subject: [PATCH 14/17] Use NotFoundExceptions --- .../Controllers/GroupsController.cs | 2 +- .../OrganizationUsersController.cs | 3 +-- .../Groups/UpdateGroupCommand.cs | 23 ++++++++----------- .../UpdateOrganizationUserCommand.cs | 17 +++++--------- .../Controllers/GroupsControllerPutTests.cs | 6 ++--- .../OrganizationUserControllerPutTests.cs | 6 ++--- .../Groups/UpdateGroupCommandTests.cs | 19 +++++---------- .../UpdateOrganizationUserCommandTests.cs | 23 ++++--------------- 8 files changed, 32 insertions(+), 67 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index ae63a893daa1..2eae7a959665 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -212,7 +212,7 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] BulkCollectionOperations.ModifyGroupAccess)) .Succeeded) { - throw new BadRequestException("You must have Can Manage permissions to edit a collection's membership"); + throw new NotFoundException(); } } diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 5941196280bd..420961d14b25 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -429,8 +429,7 @@ private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpd BulkCollectionOperations.ModifyUserAccess)) .Succeeded) { - throw new BadRequestException( - $"You must have Can Manage permissions to edit the membership for collection id {collection.Id}."); + throw new NotFoundException(); } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index 365d592cf332..8da6453b61b6 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs @@ -112,7 +112,7 @@ private async Task ValidateAsync(Organization organization, Group group, ICollec if (organization == null) { - throw new BadRequestException("Organization not found"); + throw new NotFoundException(); } if (!organization.UseGroups) @@ -121,14 +121,9 @@ private async Task ValidateAsync(Organization organization, Group group, ICollec } var originalGroup = await _groupRepository.GetByIdAsync(group.Id); - if (originalGroup == null) + if (originalGroup == null || originalGroup.OrganizationId != group.OrganizationId) { - throw new NotFoundException("Group not found."); - } - - if (originalGroup.OrganizationId != group.OrganizationId) - { - throw new BadRequestException("You cannot change a group's organization id."); + throw new NotFoundException(); } if (collectionAccess?.Any() == true) @@ -164,14 +159,14 @@ private async Task ValidateCollectionAccessAsync(Group originalGroup, .FirstOrDefault(cas => !collectionIds.Contains(cas.Id)); if (missingCollectionId != default) { - throw new BadRequestException($"Invalid collection id {missingCollectionId}."); + throw new NotFoundException(); } 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}."); + throw new NotFoundException(); } } @@ -181,17 +176,17 @@ private async Task ValidateMemberAccessAsync(Group originalGroup, var members = await _organizationUserRepository.GetManyAsync(memberAccess); var memberIds = members.Select(g => g.Id); - var missingMemberId = memberAccess.FirstOrDefault(gId => !memberIds.Contains(gId)); + var missingMemberId = memberAccess.FirstOrDefault(mId => !memberIds.Contains(mId)); if (missingMemberId != default) { - throw new BadRequestException($"Invalid member id {missingMemberId}."); + throw new NotFoundException(); } - var invalidMember = members.FirstOrDefault(g => g.OrganizationId != originalGroup.OrganizationId); + var invalidMember = members.FirstOrDefault(m => m.OrganizationId != originalGroup.OrganizationId); if (invalidMember != default) { // Use generic error message to avoid enumeration - throw new BadRequestException($"Invalid member id {invalidMember.Id}."); + throw new NotFoundException(); } } } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index f6df5adb6cf6..3b019e122ea5 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -64,14 +64,9 @@ public async Task UpdateUserAsync(OrganizationUser user, Guid? savingUserId, } var originalUser = await _organizationUserRepository.GetByIdAsync(user.Id); - if (originalUser == null) + if (originalUser == null || user.OrganizationId != originalUser.OrganizationId) { - throw new NotFoundException("User not found."); - } - - if (user.OrganizationId != originalUser.OrganizationId) - { - throw new BadRequestException("You cannot change a member's organization id."); + throw new NotFoundException(); } if (collectionAccess?.Any() == true) @@ -146,14 +141,14 @@ private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser, .FirstOrDefault(cas => !collectionIds.Contains(cas.Id)); if (missingCollectionId != default) { - throw new BadRequestException($"Invalid collection id {missingCollectionId}."); + throw new NotFoundException(); } var invalidCollection = collections.FirstOrDefault(c => c.OrganizationId != originalUser.OrganizationId); if (invalidCollection != default) { // Use generic error message to avoid enumeration - throw new BadRequestException($"Invalid collection id {invalidCollection.Id}."); + throw new NotFoundException(); } } @@ -166,14 +161,14 @@ private async Task ValidateGroupAccessAsync(OrganizationUser originalUser, var missingGroupId = groupAccess.FirstOrDefault(gId => !groupIds.Contains(gId)); if (missingGroupId != default) { - throw new BadRequestException($"Invalid group id {missingGroupId}."); + throw new NotFoundException(); } var invalidGroup = groups.FirstOrDefault(g => g.OrganizationId != originalUser.OrganizationId); if (invalidGroup != default) { // Use generic error message to avoid enumeration - throw new BadRequestException($"Invalid group id {invalidGroup.Id}."); + throw new NotFoundException(); } } } diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs index 970cc002f6f7..3a8e4831afc4 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerPutTests.cs @@ -257,8 +257,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) .Returns(AuthorizationResult.Failed()); - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); - Assert.Contains("You must have Can Manage permission", exception.Message); + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); } [Theory] @@ -278,8 +277,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(G Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) .Returns(AuthorizationResult.Failed()); - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); - Assert.Contains("You must have Can Manage permission", exception.Message); + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organization.Id, group.Id, groupRequestModel)); } private void Put_Setup(SutProvider sutProvider, Organization organization, diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs index 8af0b8f62b8a..0b9c18c57019 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUserControllerPutTests.cs @@ -232,8 +232,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotUpdateCollection Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); - Assert.Contains("You must have Can Manage permission", exception.Message); + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); } [Theory] @@ -252,8 +251,7 @@ public async Task Put_UpdateCollections_ThrowsIfSavingUserCannotAddCollections(O Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyUserAccess))) .Returns(AuthorizationResult.Failed()); - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); - Assert.Contains("You must have Can Manage permission", exception.Message); + await Assert.ThrowsAsync(() => sutProvider.Sut.Put(organizationAbility.Id, organizationUser.Id, model)); } private void Put_Setup(SutProvider sutProvider, diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs index ab8a9a4a931d..adbc3c43534e 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommandTests.cs @@ -82,9 +82,7 @@ public async Task UpdateGroup_WithNullOrganization_Throws(SutProvider(async () => await sutProvider.Sut.UpdateGroupAsync(group, null, eventSystemUser)); - - Assert.Contains("Organization not found", exception.Message); + await Assert.ThrowsAsync(async () => await sutProvider.Sut.UpdateGroupAsync(group, null, eventSystemUser)); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().CreateAsync(default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogGroupEventAsync(default, default, default); @@ -137,8 +135,7 @@ public async Task UpdateGroup_GroupBelongsToDifferentOrganization_Throws(SutProv // Mismatching orgId oldGroup.OrganizationId = CoreHelpers.GenerateComb(); - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateGroupAsync(group, organization)); - Assert.Contains("cannot change a group's organization id", exception.Message); + await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateGroupAsync(group, organization)); } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] @@ -154,9 +151,8 @@ public async Task UpdateGroup_CollectionsBelongsToDifferentOrganization_Throws(S .Returns(callInfo => callInfo.Arg>() .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); - var exception = await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); - Assert.Contains("Invalid collection id", exception.Message); } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] @@ -178,9 +174,8 @@ public async Task UpdateGroup_CollectionsDoNotExist_Throws(SutProvider( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateGroupAsync(group, organization, collectionAccess)); - Assert.Contains("Invalid collection id", exception.Message); } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] @@ -196,9 +191,8 @@ public async Task UpdateGroup_MemberBelongsToDifferentOrganization_Throws(SutPro .Returns(callInfo => callInfo.Arg>() .Select(guid => new OrganizationUser { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); - var exception = await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); - Assert.Contains("Invalid member id", exception.Message); } [Theory, OrganizationCustomize(UseGroups = true, FlexibleCollections = true), BitAutoData] @@ -219,9 +213,8 @@ public async Task UpdateGroup_MemberDoesNotExist_Throws(SutProvider( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateGroupAsync(group, organization, null, userAccess)); - Assert.Contains("Invalid member id", exception.Message); } private void ArrangeGroup(SutProvider sutProvider, Group group, Group oldGroup) diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs index eef61fb67905..4aed8192adbe 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommandTests.cs @@ -36,9 +36,8 @@ public async Task UpdateUserAsync_DifferentOrganizationId_Throws(OrganizationUse { sutProvider.GetDependency().GetByIdAsync(user.Id).Returns(originalUser); - var exception = await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, null)); - Assert.Contains("cannot change a member's organization id", exception.Message.ToLowerInvariant()); } [Theory, BitAutoData] @@ -54,11 +53,8 @@ public async Task UpdateUserAsync_CollectionsBelongToDifferentOrganization_Throw .Returns(callInfo => callInfo.Arg>() .Select(guid => new Collection { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); - var exception = await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); - - Assert.Contains("invalid collection id", - exception.Message.ToLowerInvariant()); } [Theory, BitAutoData] @@ -79,11 +75,8 @@ public async Task UpdateUserAsync_CollectionsDoNotExist_Throws(OrganizationUser return result; }); - var exception = await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, collectionAccess, null)); - - Assert.Contains("invalid collection id", - exception.Message.ToLowerInvariant()); } [Theory, BitAutoData] @@ -99,11 +92,8 @@ public async Task UpdateUserAsync_GroupsBelongToDifferentOrganization_Throws(Org .Returns(callInfo => callInfo.Arg>() .Select(guid => new Group { Id = guid, OrganizationId = CoreHelpers.GenerateComb() }).ToList()); - var exception = await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); - - Assert.Contains("invalid group id", - exception.Message.ToLowerInvariant()); } [Theory, BitAutoData] @@ -124,11 +114,8 @@ public async Task UpdateUserAsync_GroupsDoNotExist_Throws(OrganizationUser user, return result; }); - var exception = await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.UpdateUserAsync(user, savingUserId, null, groupAccess)); - - Assert.Contains("invalid group id", - exception.Message.ToLowerInvariant()); } [Theory, BitAutoData] From 3bdf34c5758e7582baee54eaee47305d0875ad40 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 16 Jul 2024 09:31:56 +1000 Subject: [PATCH 15/17] Fix error messages --- src/Api/AdminConsole/Controllers/GroupsController.cs | 2 +- .../AdminConsole/Controllers/OrganizationUsersController.cs | 2 +- .../AdminConsole/Controllers/GroupsControllerTests.cs | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index 2eae7a959665..1d5d1a931910 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -135,7 +135,7 @@ public async Task Post(Guid orgId, [FromBody] GroupRequestMo .Succeeded; if (!authorized) { - throw new NotFoundException("You are not authorized to grant access to these collections."); + throw new NotFoundException(); } } diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 420961d14b25..30308420626c 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -230,7 +230,7 @@ public async Task Invite(Guid orgId, [FromBody] OrganizationUserInviteRequestMod .Succeeded; if (!authorized) { - throw new NotFoundException("You are not authorized to grant access to these collections."); + throw new NotFoundException(); } } diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs index f3227310fd03..eb223317f4d6 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs @@ -108,9 +108,7 @@ public async Task Post_NotAuthorizedToGiveAccessToCollections_Throws(Organizatio Arg.Is>(reqs => reqs.Contains(BulkCollectionOperations.ModifyGroupAccess))) .Returns(AuthorizationResult.Failed()); - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Post(organization.Id, groupRequestModel)); - - Assert.Contains("You are not authorized to grant access to these collections.", exception.Message); + await Assert.ThrowsAsync(() => sutProvider.Sut.Post(organization.Id, groupRequestModel)); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .CreateGroupAsync(default, default, default, default); From 251714bfdcb8cf4818b9ca1af7ea0fe7d0ac3fd3 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 16 Jul 2024 09:39:35 +1000 Subject: [PATCH 16/17] Tweaks --- .../OrganizationFeatures/Groups/UpdateGroupCommand.cs | 6 +++--- .../OrganizationUsers/UpdateOrganizationUserCommand.cs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs index 8da6453b61b6..b471b1d88020 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/Groups/UpdateGroupCommand.cs @@ -110,7 +110,7 @@ private async Task ValidateAsync(Organization organization, Group group, ICollec // Avoid multiple enumeration memberAccess = memberAccess?.ToList(); - if (organization == null) + if (organization == null || organization.Id != group.OrganizationId) { throw new NotFoundException(); } @@ -155,9 +155,9 @@ private async Task ValidateCollectionAccessAsync(Group originalGroup, .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); var collectionIds = collections.Select(c => c.Id); - var missingCollectionId = collectionAccess + var missingCollection = collectionAccess .FirstOrDefault(cas => !collectionIds.Contains(cas.Id)); - if (missingCollectionId != default) + if (missingCollection != default) { throw new NotFoundException(); } diff --git a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs index 3b019e122ea5..a37d94fb645b 100644 --- a/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs +++ b/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserCommand.cs @@ -137,9 +137,9 @@ private async Task ValidateCollectionAccessAsync(OrganizationUser originalUser, .GetManyByManyIdsAsync(collectionAccess.Select(c => c.Id)); var collectionIds = collections.Select(c => c.Id); - var missingCollectionId = collectionAccess + var missingCollection = collectionAccess .FirstOrDefault(cas => !collectionIds.Contains(cas.Id)); - if (missingCollectionId != default) + if (missingCollection != default) { throw new NotFoundException(); } From c706d2e69db45d826ca3439f3a7d7470e46698d8 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 16 Jul 2024 09:40:27 +1000 Subject: [PATCH 17/17] Fix test --- .../Controllers/OrganizationUsersControllerTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index 97b9a4a1eeef..deb4c6818859 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -181,8 +181,7 @@ public async Task Invite_NotAuthorizedToGiveAccessToCollections_Throws(Organizat .Returns(AuthorizationResult.Failed()); sutProvider.GetDependency().GetProperUserId(Arg.Any()).Returns(userId); - var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.Invite(organizationAbility.Id, model)); - Assert.Contains("You are not authorized", exception.Message); + await Assert.ThrowsAsync(() => sutProvider.Sut.Invite(organizationAbility.Id, model)); } [Theory]