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

---
 .../OrganizationUserAuthorizationHandler.cs   | 39 +++++++++----------
 ...ganizationUserAuthorizationHandlerTests.cs | 21 ++++++----
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/src/Api/Vault/AuthorizationHandlers/OrganizationUsers/OrganizationUserAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/OrganizationUsers/OrganizationUserAuthorizationHandler.cs
index 38297bb5c7..e7c4f64927 100644
--- a/src/Api/Vault/AuthorizationHandlers/OrganizationUsers/OrganizationUserAuthorizationHandler.cs
+++ b/src/Api/Vault/AuthorizationHandlers/OrganizationUsers/OrganizationUserAuthorizationHandler.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;
@@ -57,29 +58,27 @@ public class OrganizationUserAuthorizationHandler : AuthorizationHandler<Organiz
     }
 
     private async Task CanReadAllAsync(AuthorizationHandlerContext context, OrganizationUserOperationRequirement requirement,
-        CurrentContextOrganization org)
+        CurrentContextOrganization? org)
     {
-        if (org != null)
+        // If the limit collection management setting is disabled, allow any user to read all organization users
+        // Otherwise, Owners, Admins, and users with any of ManageGroups, ManageUsers, EditAnyCollection, DeleteAnyCollection, CreateNewCollections permissions can always read all organization users
+        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 organization users 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/OrganizationUserAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/OrganizationUserAuthorizationHandlerTests.cs
index 6a97a1f346..3fe15b24c1 100644
--- a/test/Api.Test/Vault/AuthorizationHandlers/OrganizationUserAuthorizationHandlerTests.cs
+++ b/test/Api.Test/Vault/AuthorizationHandlers/OrganizationUserAuthorizationHandlerTests.cs
@@ -26,6 +26,7 @@ public class OrganizationUserAuthorizationHandlerTests
         CurrentContextOrganization organization)
     {
         organization.Type = userType;
+        organization.LimitCollectionCreationDeletion = true;
         organization.Permissions = new Permissions();
 
         var context = new AuthorizationHandlerContext(
@@ -46,6 +47,10 @@ public class OrganizationUserAuthorizationHandlerTests
         Guid userId,
         SutProvider<OrganizationUserAuthorizationHandler> sutProvider, CurrentContextOrganization organization)
     {
+        organization.Type = OrganizationUserType.User;
+        organization.LimitCollectionCreationDeletion = true;
+        organization.Permissions = new Permissions();
+
         var context = new AuthorizationHandlerContext(
             new[] { OrganizationUserOperations.ReadAll(organization.Id) },
             new ClaimsPrincipal(),
@@ -64,18 +69,21 @@ public class OrganizationUserAuthorizationHandlerTests
     }
 
     [Theory]
-    [BitAutoData(true, false, false, false)]
-    [BitAutoData(false, true, false, false)]
-    [BitAutoData(false, false, true, false)]
-    [BitAutoData(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 editAnyCollection, bool deleteAnyCollection, bool manageGroups,
+        bool manageUsers, bool limitCollectionCreationDeletion,
         SutProvider<OrganizationUserAuthorizationHandler> sutProvider,
         CurrentContextOrganization organization)
     {
         var actingUserId = Guid.NewGuid();
 
         organization.Type = OrganizationUserType.Custom;
+        organization.LimitCollectionCreationDeletion = limitCollectionCreationDeletion;
         organization.Permissions = new Permissions
         {
             EditAnyCollection = editAnyCollection,
@@ -114,8 +122,7 @@ public class OrganizationUserAuthorizationHandlerTests
             EditAnyCollection = false,
             DeleteAnyCollection = false,
             ManageGroups = false,
-            ManageUsers = false,
-            AccessImportExport = false
+            ManageUsers = false
         };
 
         var context = new AuthorizationHandlerContext(