[AC-1443] Update manager permission to only see collections they have…

… access to (#3071) * [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
bitwarden · Aug 8, 2023 · 95b7652 · 95b7652
1 parent 5275f22
commit 95b7652
Show file tree

Hide file tree

Showing 7 changed files with 63 additions and 10 deletions.
diff --git a/src/Api/Controllers/CollectionsController.cs b/src/Api/Controllers/CollectionsController.cs
@@ -112,7 +112,7 @@ public async Task<ListResponseModel<CollectionAccessDetailsResponseModel>> GetMa
     [HttpGet("")]
     public async Task<ListResponseModel<CollectionResponseModel>> Get(Guid orgId)
     {
-        IEnumerable<Collection> orgCollections = await _collectionService.GetOrganizationCollections(orgId);
+        IEnumerable<Collection> orgCollections = await _collectionService.GetOrganizationCollectionsAsync(orgId);
 
         var responses = orgCollections.Select(c => new CollectionResponseModel(c));
         return new ListResponseModel<CollectionResponseModel>(responses);
@@ -209,7 +209,7 @@ public async Task DeleteMany([FromBody] CollectionBulkDeleteRequestModel model)
             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
@@ -40,7 +40,7 @@ public async Task<IActionResult> Export(Guid organizationId)
     {
         var userId = _userService.GetProperUserId(User).Value;
 
-        IEnumerable<Collection> orgCollections = await _collectionService.GetOrganizationCollections(organizationId);
+        IEnumerable<Collection> orgCollections = await _collectionService.GetOrganizationCollectionsAsync(organizationId);
         (IEnumerable<CipherOrganizationDetails> orgCiphers, Dictionary<Guid, IGrouping<Guid, CollectionCipher>> 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
@@ -341,7 +341,7 @@ public async Task<bool> DeleteAnyCollection(Guid orgId)
 
     public async Task<bool> ViewAllCollections(Guid orgId)
     {
-        return await CreateNewCollections(orgId) || await EditAnyCollection(orgId) || await DeleteAnyCollection(orgId);
+        return await EditAnyCollection(orgId) || await DeleteAnyCollection(orgId);
     }
 
     public async Task<bool> EditAssignedCollections(Guid orgId)

diff --git a/src/Core/Services/ICollectionService.cs b/src/Core/Services/ICollectionService.cs
@@ -7,5 +7,5 @@ public interface ICollectionService
 {
     Task SaveAsync(Collection collection, IEnumerable<CollectionAccessSelection> groups = null, IEnumerable<CollectionAccessSelection> users = null, Guid? assignUserId = null);
     Task DeleteUserAsync(Collection collection, Guid organizationUserId);
-    Task<IEnumerable<Collection>> GetOrganizationCollections(Guid organizationId);
+    Task<IEnumerable<Collection>> GetOrganizationCollectionsAsync(Guid organizationId);
 }
diff --git a/src/Core/Services/Implementations/CollectionService.cs b/src/Core/Services/Implementations/CollectionService.cs
@@ -96,9 +96,9 @@ public async Task DeleteUserAsync(Collection collection, Guid organizationUserId
         await _eventService.LogOrganizationUserEventAsync(orgUser, Enums.EventType.OrganizationUser_Updated);
     }
 
-    public async Task<IEnumerable<Collection>> GetOrganizationCollections(Guid organizationId)
+    public async Task<IEnumerable<Collection>> 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
@@ -171,7 +171,7 @@ public async Task DeleteMany_Success(Guid orgId, User user, Collection collectio
             .Returns(user.Id);
 
         sutProvider.GetDependency<ICollectionService>()
-            .GetOrganizationCollections(orgId)
+            .GetOrganizationCollectionsAsync(orgId)
             .Returns(collections);
 
         // Act
@@ -237,7 +237,7 @@ public async Task DeleteMany_UserCanNotAccessCollections_FiltersOutInvalid(Guid
             .Returns(user.Id);
 
         sutProvider.GetDependency<ICollectionService>()
-            .GetOrganizationCollections(orgId)
+            .GetOrganizationCollectionsAsync(orgId)
             .Returns(collections);
 
         // Act

diff --git 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 @@ await sutProvider.GetDependency<IEventService>().DidNotReceiveWithAnyArgs()
             .LogOrganizationUserEventAsync(default, default);
     }
 
+    [Theory, BitAutoData]
+    public async Task GetOrganizationCollectionsAsync_WithViewAssignedCollectionsTrue_ReturnsAssignedCollections(
+        CollectionDetails collectionDetails, Guid organizationId, Guid userId, SutProvider<CollectionService> sutProvider)
+    {
+        collectionDetails.OrganizationId = organizationId;
+
+        sutProvider.GetDependency<ICurrentContext>().UserId.Returns(userId);
+        sutProvider.GetDependency<ICollectionRepository>()
+            .GetManyByUserIdAsync(userId)
+            .Returns(new List<CollectionDetails> { collectionDetails });
+        sutProvider.GetDependency<ICurrentContext>().ViewAssignedCollections(organizationId).Returns(true);
+
+        var result = await sutProvider.Sut.GetOrganizationCollectionsAsync(organizationId);
+
+        Assert.Single(result);
+        Assert.Equal(collectionDetails, result.First());
+
+        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().GetManyByOrganizationIdAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>().Received(1).GetManyByUserIdAsync(userId);
+    }
+
+    [Theory, BitAutoData]
+    public async Task GetOrganizationCollectionsAsync_WithViewAllCollectionsTrue_ReturnsAllOrganizationCollections(
+        Collection collection, Guid organizationId, Guid userId, SutProvider<CollectionService> sutProvider)
+    {
+        sutProvider.GetDependency<ICurrentContext>().UserId.Returns(userId);
+        sutProvider.GetDependency<ICollectionRepository>()
+            .GetManyByOrganizationIdAsync(organizationId)
+            .Returns(new List<Collection> { collection });
+        sutProvider.GetDependency<ICurrentContext>().ViewAssignedCollections(organizationId).Returns(true);
+        sutProvider.GetDependency<ICurrentContext>().ViewAllCollections(organizationId).Returns(true);
+
+        var result = await sutProvider.Sut.GetOrganizationCollectionsAsync(organizationId);
+
+        Assert.Single(result);
+        Assert.Equal(collection, result.First());
+
+        await sutProvider.GetDependency<ICollectionRepository>().Received(1).GetManyByOrganizationIdAsync(organizationId);
+        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().GetManyByUserIdAsync(default);
+    }
+
+    [Theory, BitAutoData]
+    public async Task GetOrganizationCollectionsAsync_WithViewAssignedCollectionsFalse_ThrowsBadRequestException(
+        Guid organizationId, SutProvider<CollectionService> sutProvider)
+    {
+        sutProvider.GetDependency<ICurrentContext>().ViewAssignedCollections(organizationId).Returns(false);
+
+        await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.GetOrganizationCollectionsAsync(organizationId));
+
+        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().GetManyByOrganizationIdAsync(default);
+        await sutProvider.GetDependency<ICollectionRepository>().DidNotReceiveWithAnyArgs().GetManyByUserIdAsync(default);
+    }
 }