[AC-2068] Allows Users to read all users/groups when Flexible Collections is enabled (#3720)

* [AC-2068] Allow any member of an org to read all users for that organization with flexible collections * [AC-2068] Allow any member of an org to read all groups for that organization with flexible collections * [AC-2068] Formatting
2024-11-21 12:05:42 +01:00 · 2024-01-30 09:53:56 -08:00 · 2024-01-30 09:53:56 -08:00 · ca2915494d
commit ca2915494d
parent 7180a6618e
4 changed files with 12 additions and 270 deletions
--- a/src/Api/Vault/AuthorizationHandlers/Groups/GroupAuthorizationHandler.cs
+++ b/src/Api/Vault/AuthorizationHandlers/Groups/GroupAuthorizationHandler.cs
@ -1,8 +1,5 @@
 #nullable enable
 using Bit.Core.Context;
-using Bit.Core.Enums;
-using Bit.Core.Models.Data.Organizations;
-using Bit.Core.Services;
 using Microsoft.AspNetCore.Authorization;

 namespace Bit.Api.Vault.AuthorizationHandlers.Groups;
@ -14,17 +11,10 @@ namespace Bit.Api.Vault.AuthorizationHandlers.Groups;
 public class GroupAuthorizationHandler : AuthorizationHandler<GroupOperationRequirement>
 {
    private readonly ICurrentContext _currentContext;
-    private readonly IFeatureService _featureService;
-    private readonly IApplicationCacheService _applicationCacheService;

-    public GroupAuthorizationHandler(
-        ICurrentContext currentContext,
-        IFeatureService featureService,
-        IApplicationCacheService applicationCacheService)
+    public GroupAuthorizationHandler(ICurrentContext currentContext)
    {
        _currentContext = currentContext;
-        _featureService = featureService;
-        _applicationCacheService = applicationCacheService;
    }

    protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context,
@ -56,22 +46,8 @@ public class GroupAuthorizationHandler : AuthorizationHandler<GroupOperationRequ
    private async Task CanReadAllAsync(AuthorizationHandlerContext context, GroupOperationRequirement requirement,
        CurrentContextOrganization? org)
    {
-        // Owners, Admins, and users with any of ManageGroups, ManageUsers, EditAnyCollection, DeleteAnyCollection, CreateNewCollections permissions can always read all groups
-        if (org is
-        { 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 })
-        {
-            context.Succeed(requirement);
-            return;
-        }
-
-        // Check for non-null org here: the user must be apart of the organization for this setting to take affect
-        // If the limit collection management setting is disabled, allow any user to read all groups
-        if (await GetOrganizationAbilityAsync(org) is { LimitCollectionCreationDeletion: false })
+        // All users of an organization can read all groups belonging to the organization for collection access management
+        if (org is not null)
        {
            context.Succeed(requirement);
            return;
@ -83,16 +59,4 @@ public class GroupAuthorizationHandler : AuthorizationHandler<GroupOperationRequ
            context.Succeed(requirement);
        }
    }
-
-    private async Task<OrganizationAbility?> GetOrganizationAbilityAsync(CurrentContextOrganization? organization)
-    {
-        // If the CurrentContextOrganization is null, then the user isn't a member of the org so the setting is
-        // irrelevant
-        if (organization == null)
-        {
-            return null;
-        }
-
-        return await _applicationCacheService.GetOrganizationAbilityAsync(organization.Id);
-    }
 }
--- a/src/Api/Vault/AuthorizationHandlers/OrganizationUsers/OrganizationUserAuthorizationHandler.cs
+++ b/src/Api/Vault/AuthorizationHandlers/OrganizationUsers/OrganizationUserAuthorizationHandler.cs
@ -1,8 +1,5 @@
 #nullable enable
 using Bit.Core.Context;
-using Bit.Core.Enums;
-using Bit.Core.Models.Data.Organizations;
-using Bit.Core.Services;
 using Microsoft.AspNetCore.Authorization;

 namespace Bit.Api.Vault.AuthorizationHandlers.OrganizationUsers;
@ -14,17 +11,10 @@ namespace Bit.Api.Vault.AuthorizationHandlers.OrganizationUsers;
 public class OrganizationUserAuthorizationHandler : AuthorizationHandler<OrganizationUserOperationRequirement>
 {
    private readonly ICurrentContext _currentContext;
-    private readonly IFeatureService _featureService;
-    private readonly IApplicationCacheService _applicationCacheService;

-    public OrganizationUserAuthorizationHandler(
-        ICurrentContext currentContext,
-        IFeatureService featureService,
-        IApplicationCacheService applicationCacheService)
+    public OrganizationUserAuthorizationHandler(ICurrentContext currentContext)
    {
        _currentContext = currentContext;
-        _featureService = featureService;
-        _applicationCacheService = applicationCacheService;
    }

    protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context,
@ -55,26 +45,10 @@ public class OrganizationUserAuthorizationHandler : AuthorizationHandler<Organiz
    private async Task CanReadAllAsync(AuthorizationHandlerContext context, OrganizationUserOperationRequirement requirement,
        CurrentContextOrganization? org)
    {
-        // 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
-        { 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 })
+        // All users of an organization can read all other users of that organization for collection access management
+        if (org is not null)
        {
            context.Succeed(requirement);
-            return;
-        }
-
-        // Check for non-null org here: the user must be apart of the organization for this setting to take affect
-        // If the limit collection management setting is disabled, allow any user to read all organization users
-        if (await GetOrganizationAbilityAsync(org) is { LimitCollectionCreationDeletion: false })
-        {
-            context.Succeed(requirement);
-            return;
        }

        // Allow provider users to read all organization users if they are a provider for the target organization
@ -83,16 +57,4 @@ public class OrganizationUserAuthorizationHandler : AuthorizationHandler<Organiz
            context.Succeed(requirement);
        }
    }
-
-    private async Task<OrganizationAbility?> GetOrganizationAbilityAsync(CurrentContextOrganization? organization)
-    {
-        // If the CurrentContextOrganization is null, then the user isn't a member of the org so the setting is
-        // irrelevant
-        if (organization == null)
-        {
-            return null;
-        }
-
-        return await _applicationCacheService.GetOrganizationAbilityAsync(organization.Id);
-    }
 }
--- a/test/Api.Test/Vault/AuthorizationHandlers/GroupAuthorizationHandlerTests.cs
+++ b/test/Api.Test/Vault/AuthorizationHandlers/GroupAuthorizationHandlerTests.cs
@ -3,8 +3,6 @@ using Bit.Api.Vault.AuthorizationHandlers.Groups;
 using Bit.Core.Context;
 using Bit.Core.Enums;
 using Bit.Core.Models.Data;
-using Bit.Core.Models.Data.Organizations;
-using Bit.Core.Services;
 using Bit.Test.Common.AutoFixture;
 using Bit.Test.Common.AutoFixture.Attributes;
 using Microsoft.AspNetCore.Authorization;
@ -19,7 +17,9 @@ public class GroupAuthorizationHandlerTests
    [Theory]
    [BitAutoData(OrganizationUserType.Admin)]
    [BitAutoData(OrganizationUserType.Owner)]
-    public async Task CanReadAllAsync_WhenAdminOrOwner_Success(
+    [BitAutoData(OrganizationUserType.User)]
+    [BitAutoData(OrganizationUserType.Custom)]
+    public async Task CanReadAllAsync_WhenMemberOfOrg_Success(
        OrganizationUserType userType,
        Guid userId, SutProvider<GroupAuthorizationHandler> sutProvider,
        CurrentContextOrganization organization)
@ -27,8 +27,6 @@ public class GroupAuthorizationHandlerTests
        organization.Type = userType;
        organization.Permissions = new Permissions();

-        ArrangeOrganizationAbility(sutProvider, organization, true);
-
        var context = new AuthorizationHandlerContext(
            new[] { GroupOperations.ReadAll(organization.Id) },
            new ClaimsPrincipal(),
@ -50,8 +48,6 @@ public class GroupAuthorizationHandlerTests
        organization.Type = OrganizationUserType.User;
        organization.Permissions = new Permissions();

-        ArrangeOrganizationAbility(sutProvider, organization, true);
-
        var context = new AuthorizationHandlerContext(
            new[] { GroupOperations.ReadAll(organization.Id) },
            new ClaimsPrincipal(),
@ -69,87 +65,12 @@ public class GroupAuthorizationHandlerTests
        Assert.True(context.HasSucceeded);
    }

-    [Theory]
-    [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 limitCollectionCreationDeletion,
-        SutProvider<GroupAuthorizationHandler> sutProvider,
-        CurrentContextOrganization organization)
-    {
-        var actingUserId = Guid.NewGuid();
-
-        organization.Type = OrganizationUserType.Custom;
-        organization.Permissions = new Permissions
-        {
-            EditAnyCollection = editAnyCollection,
-            DeleteAnyCollection = deleteAnyCollection,
-            ManageGroups = manageGroups,
-            ManageUsers = manageUsers
-        };
-
-        ArrangeOrganizationAbility(sutProvider, organization, limitCollectionCreationDeletion);
-
-        var context = new AuthorizationHandlerContext(
-            new[] { GroupOperations.ReadAll(organization.Id) },
-            new ClaimsPrincipal(),
-            null);
-
-        sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
-        sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
-
-        await sutProvider.Sut.HandleAsync(context);
-
-        Assert.True(context.HasSucceeded);
-    }
-
-    [Theory]
-    [BitAutoData(OrganizationUserType.User)]
-    [BitAutoData(OrganizationUserType.Custom)]
-    public async Task CanReadAllAsync_WhenMissingPermissions_NoSuccess(
-        OrganizationUserType userType,
-        SutProvider<GroupAuthorizationHandler> sutProvider,
-        CurrentContextOrganization organization)
-    {
-        var actingUserId = Guid.NewGuid();
-
-        organization.Type = userType;
-        organization.Permissions = new Permissions
-        {
-            EditAnyCollection = false,
-            DeleteAnyCollection = false,
-            ManageGroups = false,
-            ManageUsers = false,
-            AccessImportExport = false
-        };
-
-        ArrangeOrganizationAbility(sutProvider, organization, true);
-
-        var context = new AuthorizationHandlerContext(
-            new[] { GroupOperations.ReadAll(organization.Id) },
-            new ClaimsPrincipal(),
-            null);
-
-        sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
-        sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
-        sutProvider.GetDependency<ICurrentContext>().ProviderUserForOrgAsync(Arg.Any<Guid>()).Returns(false);
-
-        await sutProvider.Sut.HandleAsync(context);
-
-        Assert.False(context.HasSucceeded);
-    }
-
    [Theory, BitAutoData]
    public async Task CanReadAllAsync_WhenMissingOrgAccess_NoSuccess(
        Guid userId,
        CurrentContextOrganization organization,
        SutProvider<GroupAuthorizationHandler> sutProvider)
    {
-        ArrangeOrganizationAbility(sutProvider, organization, true);

        var context = new AuthorizationHandlerContext(
            new[] { GroupOperations.ReadAll(organization.Id) },
@ -201,17 +122,4 @@ public class GroupAuthorizationHandlerTests
        Assert.False(context.HasSucceeded);
        Assert.True(context.HasFailed);
    }
-
-    private static void ArrangeOrganizationAbility(
-        SutProvider<GroupAuthorizationHandler> sutProvider,
-        CurrentContextOrganization organization, bool limitCollectionCreationDeletion)
-    {
-        var organizationAbility = new OrganizationAbility();
-        organizationAbility.Id = organization.Id;
-        organizationAbility.FlexibleCollections = true;
-        organizationAbility.LimitCollectionCreationDeletion = limitCollectionCreationDeletion;
-
-        sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilityAsync(organizationAbility.Id)
-            .Returns(organizationAbility);
-    }
 }
--- a/test/Api.Test/Vault/AuthorizationHandlers/OrganizationUserAuthorizationHandlerTests.cs
+++ b/test/Api.Test/Vault/AuthorizationHandlers/OrganizationUserAuthorizationHandlerTests.cs
@ -3,8 +3,6 @@ using Bit.Api.Vault.AuthorizationHandlers.OrganizationUsers;
 using Bit.Core.Context;
 using Bit.Core.Enums;
 using Bit.Core.Models.Data;
-using Bit.Core.Models.Data.Organizations;
-using Bit.Core.Services;
 using Bit.Test.Common.AutoFixture;
 using Bit.Test.Common.AutoFixture.Attributes;
 using Microsoft.AspNetCore.Authorization;
@ -19,7 +17,9 @@ public class OrganizationUserAuthorizationHandlerTests
    [Theory]
    [BitAutoData(OrganizationUserType.Admin)]
    [BitAutoData(OrganizationUserType.Owner)]
-    public async Task CanReadAllAsync_WhenAdminOrOwner_Success(
+    [BitAutoData(OrganizationUserType.User)]
+    [BitAutoData(OrganizationUserType.Custom)]
+    public async Task CanReadAllAsync_WhenMemberOfOrg_Success(
        OrganizationUserType userType,
        Guid userId, SutProvider<OrganizationUserAuthorizationHandler> sutProvider,
        CurrentContextOrganization organization)
@ -27,8 +27,6 @@ public class OrganizationUserAuthorizationHandlerTests
        organization.Type = userType;
        organization.Permissions = new Permissions();

-        ArrangeOrganizationAbility(sutProvider, organization, true);
-
        var context = new AuthorizationHandlerContext(
            new[] { OrganizationUserOperations.ReadAll(organization.Id) },
            new ClaimsPrincipal(),
@ -50,8 +48,6 @@ public class OrganizationUserAuthorizationHandlerTests
        organization.Type = OrganizationUserType.User;
        organization.Permissions = new Permissions();

-        ArrangeOrganizationAbility(sutProvider, organization, true);
-
        var context = new AuthorizationHandlerContext(
            new[] { OrganizationUserOperations.ReadAll(organization.Id) },
            new ClaimsPrincipal(),
@ -69,87 +65,12 @@ public class OrganizationUserAuthorizationHandlerTests
        Assert.True(context.HasSucceeded);
    }

-    [Theory]
-    [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 limitCollectionCreationDeletion,
-        SutProvider<OrganizationUserAuthorizationHandler> sutProvider,
-        CurrentContextOrganization organization)
-    {
-        var actingUserId = Guid.NewGuid();
-
-        organization.Type = OrganizationUserType.Custom;
-        organization.Permissions = new Permissions
-        {
-            EditAnyCollection = editAnyCollection,
-            DeleteAnyCollection = deleteAnyCollection,
-            ManageGroups = manageGroups,
-            ManageUsers = manageUsers
-        };
-
-        ArrangeOrganizationAbility(sutProvider, organization, limitCollectionCreationDeletion);
-
-        var context = new AuthorizationHandlerContext(
-            new[] { OrganizationUserOperations.ReadAll(organization.Id) },
-            new ClaimsPrincipal(),
-            null);
-
-        sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
-        sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
-
-        await sutProvider.Sut.HandleAsync(context);
-
-        Assert.True(context.HasSucceeded);
-    }
-
-    [Theory]
-    [BitAutoData(OrganizationUserType.User)]
-    [BitAutoData(OrganizationUserType.Custom)]
-    public async Task CanReadAllAsync_WhenMissingPermissions_NoSuccess(
-        OrganizationUserType userType,
-        SutProvider<OrganizationUserAuthorizationHandler> sutProvider,
-        CurrentContextOrganization organization)
-    {
-        var actingUserId = Guid.NewGuid();
-
-        organization.Type = userType;
-        organization.Permissions = new Permissions
-        {
-            EditAnyCollection = false,
-            DeleteAnyCollection = false,
-            ManageGroups = false,
-            ManageUsers = false
-        };
-
-        ArrangeOrganizationAbility(sutProvider, organization, true);
-
-        var context = new AuthorizationHandlerContext(
-            new[] { OrganizationUserOperations.ReadAll(organization.Id) },
-            new ClaimsPrincipal(),
-            null);
-
-        sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
-        sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
-        sutProvider.GetDependency<ICurrentContext>().ProviderUserForOrgAsync(Arg.Any<Guid>()).Returns(false);
-
-        await sutProvider.Sut.HandleAsync(context);
-
-        Assert.False(context.HasSucceeded);
-    }
-
    [Theory, BitAutoData]
    public async Task HandleRequirementAsync_WhenMissingOrgAccess_NoSuccess(
        Guid userId,
        CurrentContextOrganization organization,
        SutProvider<OrganizationUserAuthorizationHandler> sutProvider)
    {
-        ArrangeOrganizationAbility(sutProvider, organization, true);
-
        var context = new AuthorizationHandlerContext(
            new[] { OrganizationUserOperations.ReadAll(organization.Id) },
            new ClaimsPrincipal(),
@ -198,17 +119,4 @@ public class OrganizationUserAuthorizationHandlerTests

        Assert.True(context.HasFailed);
    }
-
-    private static void ArrangeOrganizationAbility(
-        SutProvider<OrganizationUserAuthorizationHandler> sutProvider,
-        CurrentContextOrganization organization, bool limitCollectionCreationDeletion)
-    {
-        var organizationAbility = new OrganizationAbility();
-        organizationAbility.Id = organization.Id;
-        organizationAbility.FlexibleCollections = true;
-        organizationAbility.LimitCollectionCreationDeletion = limitCollectionCreationDeletion;
-
-        sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilityAsync(organizationAbility.Id)
-            .Returns(organizationAbility);
-    }
 }