From 0b24fe1a9b89e2c8e52c495d0d534df32710ed0e Mon Sep 17 00:00:00 2001
From: Rui Tome <rtome@bitwarden.com>
Date: Fri, 24 Nov 2023 11:17:47 +0000
Subject: [PATCH] =?UTF-8?q?[AC-1139]=C2=A0Rewrote=20GroupAuthorizationHand?=
 =?UTF-8?q?ler=20to=20be=20similar=20to=20other=20AuthHandlers;=20Revisite?=
 =?UTF-8?q?d=20unit=20tests?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../Groups/GroupAuthorizationHandler.cs       | 39 +++++++++----------
 .../GroupAuthorizationHandlerTests.cs         | 22 +++++++----
 2 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/src/Api/Vault/AuthorizationHandlers/Groups/GroupAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Groups/GroupAuthorizationHandler.cs
index ec48338451..a80b3db928 100644
--- a/src/Api/Vault/AuthorizationHandlers/Groups/GroupAuthorizationHandler.cs
+++ b/src/Api/Vault/AuthorizationHandlers/Groups/GroupAuthorizationHandler.cs
@@ -1,4 +1,5 @@
-using Bit.Core;
+#nullable enable
+using Bit.Core;
 using Bit.Core.Context;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
@@ -58,29 +59,27 @@ public class GroupAuthorizationHandler : AuthorizationHandler<GroupOperationRequ
     }
 
     private async Task CanReadAllAsync(AuthorizationHandlerContext context, GroupOperationRequirement requirement,
-        CurrentContextOrganization org)
+        CurrentContextOrganization? org)
     {
-        if (org != null)
+        // If the limit collection management setting is disabled, allow any user to read all groups
+        // Otherwise, Owners, Admins, and users with any of ManageGroups, ManageUsers, EditAnyCollection, DeleteAnyCollection, CreateNewCollections permissions can always read all groups
+        if (org is
+        { LimitCollectionCreationDeletion: false } or
+        { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or
+        { Permissions.ManageGroups: true } or
+        { Permissions.ManageUsers: true } or
+        { Permissions.EditAnyCollection: true } or
+        { Permissions.DeleteAnyCollection: true } or
+        { Permissions.CreateNewCollections: true })
         {
-            // Acting user is a member of the target organization, check permissions
-            if (org.Type is OrganizationUserType.Owner or OrganizationUserType.Admin ||
-                org.Permissions.ManageGroups ||
-                org.Permissions.ManageUsers ||
-                org.Permissions.EditAnyCollection ||
-                org.Permissions.DeleteAnyCollection ||
-                org.Permissions.CreateNewCollections ||
-                !org.LimitCollectionCreationDeletion)
-            {
-                context.Succeed(requirement);
-            }
+            context.Succeed(requirement);
+            return;
         }
-        else
+
+        // Allow provider users to read all groups if they are a provider for the target organization
+        if (await _currentContext.ProviderUserForOrgAsync(requirement.OrganizationId))
         {
-            // Check if acting user is a provider user for the target organization
-            if (await _currentContext.ProviderUserForOrgAsync(requirement.OrganizationId))
-            {
-                context.Succeed(requirement);
-            }
+            context.Succeed(requirement);
         }
     }
 }
diff --git a/test/Api.Test/Vault/AuthorizationHandlers/GroupAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/GroupAuthorizationHandlerTests.cs
index 3a611f4a80..36e4ff5fae 100644
--- a/test/Api.Test/Vault/AuthorizationHandlers/GroupAuthorizationHandlerTests.cs
+++ b/test/Api.Test/Vault/AuthorizationHandlers/GroupAuthorizationHandlerTests.cs
@@ -26,6 +26,7 @@ public class GroupAuthorizationHandlerTests
         CurrentContextOrganization organization)
     {
         organization.Type = userType;
+        organization.LimitCollectionCreationDeletion = true;
         organization.Permissions = new Permissions();
 
         var context = new AuthorizationHandlerContext(
@@ -46,6 +47,10 @@ public class GroupAuthorizationHandlerTests
         Guid userId,
         SutProvider<GroupAuthorizationHandler> sutProvider, CurrentContextOrganization organization)
     {
+        organization.Type = OrganizationUserType.User;
+        organization.LimitCollectionCreationDeletion = true;
+        organization.Permissions = new Permissions();
+
         var context = new AuthorizationHandlerContext(
             new[] { GroupOperations.ReadAll(organization.Id) },
             new ClaimsPrincipal(),
@@ -64,26 +69,27 @@ public class GroupAuthorizationHandlerTests
     }
 
     [Theory]
-    [BitAutoData(true, false, false, false, false)]
-    [BitAutoData(false, true, false, false, false)]
-    [BitAutoData(false, false, true, false, false)]
-    [BitAutoData(false, false, false, true, false)]
-    [BitAutoData(false, false, false, false, true)]
+    [BitAutoData(true, false, false, false, true)]
+    [BitAutoData(false, true, false, false, true)]
+    [BitAutoData(false, false, true, false, true)]
+    [BitAutoData(false, false, false, true, true)]
+    [BitAutoData(false, false, false, false, false)]
     public async Task CanReadAllAsync_WhenCustomUserWithRequiredPermissions_Success(
-        bool editAnyCollection, bool deleteAnyCollection, bool manageGroups, bool manageUsers, bool accessImportExport,
+        bool editAnyCollection, bool deleteAnyCollection, bool manageGroups,
+        bool manageUsers, bool limitCollectionCreationDeletion,
         SutProvider<GroupAuthorizationHandler> sutProvider,
         CurrentContextOrganization organization)
     {
         var actingUserId = Guid.NewGuid();
 
         organization.Type = OrganizationUserType.Custom;
+        organization.LimitCollectionCreationDeletion = limitCollectionCreationDeletion;
         organization.Permissions = new Permissions
         {
             EditAnyCollection = editAnyCollection,
             DeleteAnyCollection = deleteAnyCollection,
             ManageGroups = manageGroups,
-            ManageUsers = manageUsers,
-            AccessImportExport = accessImportExport
+            ManageUsers = manageUsers
         };
 
         var context = new AuthorizationHandlerContext(