From 95b7652ca98b2d7aafaf5bf4776d009a4abbc145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Tom=C3=A9?= <108268980+r-tome@users.noreply.github.com> Date: Tue, 8 Aug 2023 16:54:10 +0100 Subject: [PATCH] [AC-1443] Update manager permission to only see collections they have access to (#3071) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [AC-1443] Changed CurrentContext.ViewAllCollections to only check if the user can edit or delete any collection * [AC-1443] Renamed ICollectionService.GetOrganizationCollections to GetOrganizationCollectionsAsync * [AC-1443] Changed CollectionService.GetOrganizationCollectionsAsync to first check CurrentContext.ViewAssignedCollections instead
Added unit tests * [AC-1443] Added new unit test to check for Exception when user does not have permission --- src/Api/Controllers/CollectionsController.cs | 4 +- .../OrganizationExportController.cs | 2 +- src/Core/Context/CurrentContext.cs | 2 +- src/Core/Services/ICollectionService.cs | 2 +- .../Implementations/CollectionService.cs | 4 +- .../Controllers/CollectionsControllerTests.cs | 4 +- .../Services/CollectionServiceTests.cs | 55 ++++++++++++++++++- 7 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/Api/Controllers/CollectionsController.cs b/src/Api/Controllers/CollectionsController.cs index 4fc2ffc20..879ec077d 100644 --- a/src/Api/Controllers/CollectionsController.cs +++ b/src/Api/Controllers/CollectionsController.cs @@ -112,7 +112,7 @@ public class CollectionsController : Controller [HttpGet("")] public async Task> Get(Guid orgId) { - IEnumerable orgCollections = await _collectionService.GetOrganizationCollections(orgId); + IEnumerable orgCollections = await _collectionService.GetOrganizationCollectionsAsync(orgId); var responses = orgCollections.Select(c => new CollectionResponseModel(c)); return new ListResponseModel(responses); @@ -209,7 +209,7 @@ public class CollectionsController : Controller throw new NotFoundException(); } - var userCollections = await _collectionService.GetOrganizationCollections(orgId); + var userCollections = await _collectionService.GetOrganizationCollectionsAsync(orgId); var filteredCollections = userCollections.Where(c => collectionIds.Contains(c.Id) && c.OrganizationId == orgId); if (!filteredCollections.Any()) diff --git a/src/Api/Controllers/OrganizationExportController.cs b/src/Api/Controllers/OrganizationExportController.cs index 1261b938c..be5aa0e14 100644 --- a/src/Api/Controllers/OrganizationExportController.cs +++ b/src/Api/Controllers/OrganizationExportController.cs @@ -40,7 +40,7 @@ public class OrganizationExportController : Controller { var userId = _userService.GetProperUserId(User).Value; - IEnumerable orgCollections = await _collectionService.GetOrganizationCollections(organizationId); + IEnumerable orgCollections = await _collectionService.GetOrganizationCollectionsAsync(organizationId); (IEnumerable orgCiphers, Dictionary> collectionCiphersGroupDict) = await _cipherService.GetOrganizationCiphers(userId, organizationId); if (_currentContext.ClientVersion == null || _currentContext.ClientVersion >= new Version("2023.1.0")) diff --git a/src/Core/Context/CurrentContext.cs b/src/Core/Context/CurrentContext.cs index 512ab4431..627e4f584 100644 --- a/src/Core/Context/CurrentContext.cs +++ b/src/Core/Context/CurrentContext.cs @@ -341,7 +341,7 @@ public class CurrentContext : ICurrentContext public async Task ViewAllCollections(Guid orgId) { - return await CreateNewCollections(orgId) || await EditAnyCollection(orgId) || await DeleteAnyCollection(orgId); + return await EditAnyCollection(orgId) || await DeleteAnyCollection(orgId); } public async Task EditAssignedCollections(Guid orgId) diff --git a/src/Core/Services/ICollectionService.cs b/src/Core/Services/ICollectionService.cs index 5da8d639e..931993dac 100644 --- a/src/Core/Services/ICollectionService.cs +++ b/src/Core/Services/ICollectionService.cs @@ -7,5 +7,5 @@ public interface ICollectionService { Task SaveAsync(Collection collection, IEnumerable groups = null, IEnumerable users = null, Guid? assignUserId = null); Task DeleteUserAsync(Collection collection, Guid organizationUserId); - Task> GetOrganizationCollections(Guid organizationId); + Task> GetOrganizationCollectionsAsync(Guid organizationId); } diff --git a/src/Core/Services/Implementations/CollectionService.cs b/src/Core/Services/Implementations/CollectionService.cs index 16698a77a..006c8c5cf 100644 --- a/src/Core/Services/Implementations/CollectionService.cs +++ b/src/Core/Services/Implementations/CollectionService.cs @@ -96,9 +96,9 @@ public class CollectionService : ICollectionService await _eventService.LogOrganizationUserEventAsync(orgUser, Enums.EventType.OrganizationUser_Updated); } - public async Task> GetOrganizationCollections(Guid organizationId) + public async Task> GetOrganizationCollectionsAsync(Guid organizationId) { - if (!await _currentContext.ViewAllCollections(organizationId) && !await _currentContext.ManageUsers(organizationId) && !await _currentContext.ManageGroups(organizationId) && !await _currentContext.AccessImportExport(organizationId)) + if (!await _currentContext.ViewAssignedCollections(organizationId) && !await _currentContext.ManageUsers(organizationId) && !await _currentContext.ManageGroups(organizationId) && !await _currentContext.AccessImportExport(organizationId)) { throw new NotFoundException(); } diff --git a/test/Api.Test/Controllers/CollectionsControllerTests.cs b/test/Api.Test/Controllers/CollectionsControllerTests.cs index 8a3f944ff..c035a8bc5 100644 --- a/test/Api.Test/Controllers/CollectionsControllerTests.cs +++ b/test/Api.Test/Controllers/CollectionsControllerTests.cs @@ -171,7 +171,7 @@ public class CollectionsControllerTests .Returns(user.Id); sutProvider.GetDependency() - .GetOrganizationCollections(orgId) + .GetOrganizationCollectionsAsync(orgId) .Returns(collections); // Act @@ -237,7 +237,7 @@ public class CollectionsControllerTests .Returns(user.Id); sutProvider.GetDependency() - .GetOrganizationCollections(orgId) + .GetOrganizationCollectionsAsync(orgId) .Returns(collections); // Act diff --git a/test/Core.Test/Services/CollectionServiceTests.cs b/test/Core.Test/Services/CollectionServiceTests.cs index 4577e591d..d5b5f15cc 100644 --- a/test/Core.Test/Services/CollectionServiceTests.cs +++ b/test/Core.Test/Services/CollectionServiceTests.cs @@ -1,4 +1,5 @@ -using Bit.Core.Entities; +using Bit.Core.Context; +using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; using Bit.Core.Models.Data; @@ -185,4 +186,56 @@ public class CollectionServiceTest .LogOrganizationUserEventAsync(default, default); } + [Theory, BitAutoData] + public async Task GetOrganizationCollectionsAsync_WithViewAssignedCollectionsTrue_ReturnsAssignedCollections( + CollectionDetails collectionDetails, Guid organizationId, Guid userId, SutProvider sutProvider) + { + collectionDetails.OrganizationId = organizationId; + + sutProvider.GetDependency().UserId.Returns(userId); + sutProvider.GetDependency() + .GetManyByUserIdAsync(userId) + .Returns(new List { collectionDetails }); + sutProvider.GetDependency().ViewAssignedCollections(organizationId).Returns(true); + + var result = await sutProvider.Sut.GetOrganizationCollectionsAsync(organizationId); + + Assert.Single(result); + Assert.Equal(collectionDetails, result.First()); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetManyByOrganizationIdAsync(default); + await sutProvider.GetDependency().Received(1).GetManyByUserIdAsync(userId); + } + + [Theory, BitAutoData] + public async Task GetOrganizationCollectionsAsync_WithViewAllCollectionsTrue_ReturnsAllOrganizationCollections( + Collection collection, Guid organizationId, Guid userId, SutProvider sutProvider) + { + sutProvider.GetDependency().UserId.Returns(userId); + sutProvider.GetDependency() + .GetManyByOrganizationIdAsync(organizationId) + .Returns(new List { collection }); + sutProvider.GetDependency().ViewAssignedCollections(organizationId).Returns(true); + sutProvider.GetDependency().ViewAllCollections(organizationId).Returns(true); + + var result = await sutProvider.Sut.GetOrganizationCollectionsAsync(organizationId); + + Assert.Single(result); + Assert.Equal(collection, result.First()); + + await sutProvider.GetDependency().Received(1).GetManyByOrganizationIdAsync(organizationId); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetManyByUserIdAsync(default); + } + + [Theory, BitAutoData] + public async Task GetOrganizationCollectionsAsync_WithViewAssignedCollectionsFalse_ThrowsBadRequestException( + Guid organizationId, SutProvider sutProvider) + { + sutProvider.GetDependency().ViewAssignedCollections(organizationId).Returns(false); + + await Assert.ThrowsAsync(() => sutProvider.Sut.GetOrganizationCollectionsAsync(organizationId)); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetManyByOrganizationIdAsync(default); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().GetManyByUserIdAsync(default); + } }