From 080d1088c4f2eef0dbac61bffcc3afa294098c5f Mon Sep 17 00:00:00 2001
From: Rui Tome <rtome@bitwarden.com>
Date: Tue, 31 Oct 2023 16:35:01 +0000
Subject: [PATCH] [AC-1139] Addressing PR suggestions

---
 .../BulkCollectionAuthorizationHandler.cs     | 23 +++-------------
 .../CollectionAuthorizationHandler.cs         | 27 +++----------------
 ...BulkCollectionAuthorizationHandlerTests.cs |  2 +-
 .../CollectionAuthorizationHandlerTests.cs    |  6 ++---
 4 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs
index 253eb3feaf..2136a64d7d 100644
--- a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs
+++ b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs
@@ -116,10 +116,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<Colle
             await _currentContext.ProviderUserForOrgAsync(org.Id))
         {
             context.Succeed(requirement);
-            return;
         }
-
-        context.Fail();
     }
 
     private async Task CanReadAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement,
@@ -132,15 +129,11 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<Colle
             return;
         }
 
-        var canManageCollections = await CanManageCollectionsAsync(targetCollections, org, requireManagePermission: false);
+        var canManageCollections = await HasCollectionAccessAsync(targetCollections, org, requireManagePermission: false);
         if (canManageCollections)
         {
             context.Succeed(requirement);
         }
-        else
-        {
-            context.Fail();
-        }
     }
 
     private async Task CanDeleteAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement,
@@ -163,15 +156,11 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<Colle
             return;
         }
 
-        var canManageCollections = await CanManageCollectionsAsync(targetCollections, org, requireManagePermission: true);
+        var canManageCollections = await HasCollectionAccessAsync(targetCollections, org, requireManagePermission: true);
         if (canManageCollections)
         {
             context.Succeed(requirement);
         }
-        else
-        {
-            context.Fail();
-        }
     }
 
     /// <summary>
@@ -190,18 +179,14 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<Colle
             return;
         }
 
-        var canManageCollections = await CanManageCollectionsAsync(targetCollections, org, requireManagePermission: true);
+        var canManageCollections = await HasCollectionAccessAsync(targetCollections, org, requireManagePermission: true);
         if (canManageCollections)
         {
             context.Succeed(requirement);
         }
-        else
-        {
-            context.Fail();
-        }
     }
 
-    private async Task<bool> CanManageCollectionsAsync(
+    private async Task<bool> HasCollectionAccessAsync(
         ICollection<Collection> targetCollections,
         CurrentContextOrganization org,
         bool requireManagePermission)
diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs
index 7082a72bce..f326db9cae 100644
--- a/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs
+++ b/src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs
@@ -35,20 +35,12 @@ public class CollectionAuthorizationHandler : AuthorizationHandler<CollectionOpe
             throw new FeatureUnavailableException("Flexible collections is OFF when it should be ON.");
         }
 
-        if (!_currentContext.UserId.HasValue)
+        if (!_currentContext.UserId.HasValue || requirement.OrganizationId == default)
         {
-            context.Fail();
             return;
         }
 
-        var targetOrganizationId = requirement.OrganizationId;
-        if (targetOrganizationId == default)
-        {
-            context.Fail();
-            return;
-        }
-
-        var org = _currentContext.GetOrganization(targetOrganizationId);
+        var org = _currentContext.GetOrganization(requirement.OrganizationId);
 
         switch (requirement)
         {
@@ -79,18 +71,12 @@ public class CollectionAuthorizationHandler : AuthorizationHandler<CollectionOpe
                 return;
             }
         }
-        else
-        {
+
             // Check if acting user is a provider user for the target organization
             if (await _currentContext.ProviderUserForOrgAsync(requirement.OrganizationId))
             {
                 context.Succeed(requirement);
-                return;
             }
-        }
-
-        // Acting user is neither a member of the target organization or a provider user, fail
-        context.Fail();
     }
 
     private async Task CanReadAllWithAccessAsync(AuthorizationHandlerContext context, CollectionOperationRequirement requirement,
@@ -109,17 +95,12 @@ public class CollectionAuthorizationHandler : AuthorizationHandler<CollectionOpe
                 return;
             }
         }
-        else
-        {
+
             // Check if acting user is a provider user for the target organization
             if (await _currentContext.ProviderUserForOrgAsync(requirement.OrganizationId))
             {
                 context.Succeed(requirement);
-                return;
             }
-        }
 
-        // Acting user is neither a member of the target organization or a provider user, fail
-        context.Fail();
     }
 }
diff --git a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs
index b08a3be0bb..229ad9eb1e 100644
--- a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs
+++ b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs
@@ -216,7 +216,7 @@ public class BulkCollectionAuthorizationHandlerTests
         sutProvider.GetDependency<ICollectionRepository>().GetManyByUserIdAsync(actingUserId).Returns(collectionDetails);
 
         await sutProvider.Sut.HandleAsync(context);
-        Assert.True(context.HasFailed);
+        Assert.False(context.HasSucceeded);
         sutProvider.GetDependency<ICurrentContext>().ReceivedWithAnyArgs().GetOrganization(default);
         await sutProvider.GetDependency<ICollectionRepository>().ReceivedWithAnyArgs()
             .GetManyByUserIdAsync(default);
diff --git a/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs
index b447d6b418..3a7b2fec60 100644
--- a/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs
+++ b/test/Api.Test/Vault/AuthorizationHandlers/CollectionAuthorizationHandlerTests.cs
@@ -55,7 +55,7 @@ public class CollectionAuthorizationHandlerTests
 
         await sutProvider.Sut.HandleAsync(context);
 
-        Assert.True(expectedSuccess ? context.HasSucceeded : context.HasFailed);
+        Assert.Equal(expectedSuccess, context.HasSucceeded);
     }
 
     [Theory, BitAutoData]
@@ -95,7 +95,7 @@ public class CollectionAuthorizationHandlerTests
         sutProvider.GetDependency<ICurrentContext>().UserId.Returns((Guid?)null);
 
         await sutProvider.Sut.HandleAsync(context);
-        Assert.True(context.HasFailed);
+        Assert.False(context.HasSucceeded);
     }
 
     [Theory, BitAutoData]
@@ -114,6 +114,6 @@ public class CollectionAuthorizationHandlerTests
         sutProvider.GetDependency<ICurrentContext>().GetOrganization(Arg.Any<Guid>()).Returns((CurrentContextOrganization)null);
 
         await sutProvider.Sut.HandleAsync(context);
-        Assert.True(context.HasFailed);
+        Assert.False(context.HasSucceeded);
     }
 }