Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[AC-2086] Update CanDelete to handle V1 flag logic #3979

Merged
merged 12 commits into from
May 8, 2024
Merged

[AC-2086] Update CanDelete to handle V1 flag logic #3979

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5cef1b2
feat: Update authorization handler to handle V1 collection enhancemen…
vincentsalucci Apr 11, 2024
9ab6b91
feat: update tests to account for new V1 flag/setting logic, refs AC-…
vincentsalucci Apr 15, 2024
2d5f085
Merge branch 'main' into ac/ac-2086/collection-limit-admin-access
vincentsalucci Apr 23, 2024
ff379e2
Merge branch 'main' into ac/ac-2086/collection-limit-admin-access
vincentsalucci Apr 30, 2024
c6d1eb9
feat: update CanDelete with all collection enhancement combinations, …
vincentsalucci Apr 30, 2024
2167175
feat: add tests for new delete flows, refs AC-2086
vincentsalucci May 2, 2024
24dcae3
Merge branch 'main' into ac/ac-2086/collection-limit-admin-access
vincentsalucci May 2, 2024
6af746c
fix: update new conditionals with bool return value, refs AC-2086
vincentsalucci May 2, 2024
f66881a
feat: simplify conditional in regards to LimitCollectionCreationDelet…
vincentsalucci May 3, 2024
2f27fb3
feat: simplify AllowAdminAccessToAllCollectionItems conditional, refs…
vincentsalucci May 3, 2024
64990a3
feat: add unit test making sure admins can't delete collections witho…
vincentsalucci May 3, 2024
c7867d3
Merge branch 'main' into ac/ac-2086/collection-limit-admin-access
vincentsalucci May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 25 additions & 9 deletions src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs
View file
Open in desktop
Expand Up @@ -225,25 +225,41 @@ private async Task<bool> CanUpdateGroupAccessAsync(ICollection<Collection> resou

private async Task<bool> CanDeleteAsync(ICollection<Collection> resources, CurrentContextOrganization? org)
{
// Owners, Admins, and users with DeleteAnyCollection permission can always delete collections
if (org is
{ Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or
{ Permissions.DeleteAnyCollection: true })
// Users with DeleteAnyCollection permission can always delete collections
if (org is { Permissions.DeleteAnyCollection: true })
{
return true;
}

// Check for non-null org here: the user must be apart of the organization for this setting to take affect
// The limit collection management setting is disabled,
// ensure acting user has manage permissions for all collections being deleted
if (await GetOrganizationAbilityAsync(org) is { LimitCollectionCreationDeletion: false })
var organizationAbility = await GetOrganizationAbilityAsync(org);

// If AllowAdminAccessToAllCollectionItems is true, Owners and Admins can delete any collection, regardless of LimitCollectionCreationDeletion setting
if ((organizationAbility is { AllowAdminAccessToAllCollectionItems: true } ||
!_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) &&
org is { Type: OrganizationUserType.Owner or OrganizationUserType.Admin })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up separating these lines to make the logic a little clearer - see CanUpdateCollectionAsync.

{
return true;
}

var canManageCollections = await CanManageCollectionsAsync(resources);

// LimitCollectionCreationDeletion is false, AllowAdminAccessToAllCollectionItems setting is irrelevant.
// Ensure acting user has manage permissions for all collections being deleted
if (organizationAbility is { LimitCollectionCreationDeletion: false })
{
var canManageCollections = await CanManageCollectionsAsync(resources);
if (canManageCollections)
{
return true;
}
}
else
// LimitCollectionCreationDeletion is true, only Owners and Admins can delete collections they manage
{
if (org is { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } && canManageCollections)
{
return true;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How you've written this works and I think this has turned out pretty well after much discussion about the business logic side of things.

One suggestion, I think this could be expressed more succinctly:

var canDeleteManagedCollections = organizationAbility is { LimitCollectionCreationDeletion: false } ||
  org is { Type: OrganizationUserType.Owner or OrganizationUserType.Admin };
if (canDeleteManagedCollections && await canManageCollections(resources))
{
  return true;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!


// Allow providers to delete collections if they are a provider for the target organization
return await _currentContext.ProviderUserForOrgAsync(_targetOrganizationId);
Expand Down
202 changes: 186 additions & 16 deletions test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case for:

  • owner/admin
  • deleting a collection they don't manage
  • admins can manage all collections and items is set to false
  • expect: not authorized

or if it's here and I'm missing it, just point me to it 😁

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I've added the test you're looking for!

Expand Up @@ -903,10 +903,39 @@ public class BulkCollectionAuthorizationHandlerTests
}
}

[Theory, BitAutoData, CollectionCustomization]
public async Task CanDeleteAsync_WithDeleteAnyCollectionPermission_Success(
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<Collection> collections,
CurrentContextOrganization organization)
{
var actingUserId = Guid.NewGuid();

organization.Type = OrganizationUserType.Custom;
organization.Permissions = new Permissions
{
DeleteAnyCollection = true
};

ArrangeOrganizationAbility(sutProvider, organization, true);

var context = new AuthorizationHandlerContext(
new[] { BulkCollectionOperations.Delete },
new ClaimsPrincipal(),
collections);

sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}

[Theory, CollectionCustomization]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task CanDeleteAsync_WhenAdminOrOwner_Success(
public async Task CanDeleteAsync_WhenAdminOrOwner_AllowAdminAccessToAllCollectionItemsTrue_Success(
OrganizationUserType userType,
Guid userId, SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<Collection> collections,
Expand All @@ -930,37 +959,36 @@ public class BulkCollectionAuthorizationHandlerTests
Assert.True(context.HasSucceeded);
}

[Theory, BitAutoData, CollectionCustomization]
public async Task CanDeleteAsync_WithDeleteAnyCollectionPermission_Success(
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
[Theory, CollectionCustomization]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task CanDeleteAsync_WhenAdminOrOwner_V1FlagDisabled_Success(
OrganizationUserType userType,
Guid userId, SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<Collection> collections,
CurrentContextOrganization organization)
{
var actingUserId = Guid.NewGuid();

organization.Type = OrganizationUserType.Custom;
organization.Permissions = new Permissions
{
DeleteAnyCollection = true
};
organization.Type = userType;
organization.Permissions = new Permissions();

ArrangeOrganizationAbility(sutProvider, organization, true);
ArrangeOrganizationAbility(sutProvider, organization, true, false);

var context = new AuthorizationHandlerContext(
new[] { BulkCollectionOperations.Delete },
new ClaimsPrincipal(),
collections);

sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
sutProvider.GetDependency<ICurrentContext>().UserId.Returns(userId);
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(false);

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}

[Theory, BitAutoData, CollectionCustomization]
public async Task CanDeleteAsync_WithManageCollectionPermission_Success(
public async Task CanDeleteAsync_WhenUser_LimitCollectionCreationDeletionFalse_WithCanManagePermission_Success(
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<CollectionDetails> collections,
CurrentContextOrganization organization)
Expand Down Expand Up @@ -991,6 +1019,145 @@ public class BulkCollectionAuthorizationHandlerTests
Assert.True(context.HasSucceeded);
}

[Theory, CollectionCustomization]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
[BitAutoData(OrganizationUserType.User)]
public async Task CanDeleteAsync_LimitCollectionCreationDeletionFalse_AllowAdminAccessToAllCollectionItemsFalse_WithCanManagePermission_Success(
OrganizationUserType userType,
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<CollectionDetails> collections,
CurrentContextOrganization organization)
{
var actingUserId = Guid.NewGuid();

organization.Type = userType;
organization.Permissions = new Permissions();

ArrangeOrganizationAbility(sutProvider, organization, false, false);

sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
sutProvider.GetDependency<ICollectionRepository>().GetManyByUserIdAsync(actingUserId, Arg.Any<bool>()).Returns(collections);
sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true);

foreach (var c in collections)
{
c.Manage = true;
}

var context = new AuthorizationHandlerContext(
new[] { BulkCollectionOperations.Delete },
new ClaimsPrincipal(),
collections);

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}

[Theory, CollectionCustomization]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task CanDeleteAsync_WhenAdminOrOwner_LimitCollectionCreationDeletionTrue_AllowAdminAccessToAllCollectionItemsFalse_WithCanManagePermission_Success(
OrganizationUserType userType,
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<CollectionDetails> collections,
CurrentContextOrganization organization)
{
var actingUserId = Guid.NewGuid();

organization.Type = userType;
organization.Permissions = new Permissions();

ArrangeOrganizationAbility(sutProvider, organization, true, false);

sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
sutProvider.GetDependency<ICollectionRepository>().GetManyByUserIdAsync(actingUserId, Arg.Any<bool>()).Returns(collections);
sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true);

foreach (var c in collections)
{
c.Manage = true;
}

var context = new AuthorizationHandlerContext(
new[] { BulkCollectionOperations.Delete },
new ClaimsPrincipal(),
collections);

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}

[Theory, BitAutoData, CollectionCustomization]
public async Task CanDeleteAsync_WhenUser_LimitCollectionCreationDeletionTrue_AllowAdminAccessToAllCollectionItemsTrue_Failure(
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<CollectionDetails> collections,
CurrentContextOrganization organization)
{
var actingUserId = Guid.NewGuid();

organization.Type = OrganizationUserType.User;
organization.Permissions = new Permissions();

ArrangeOrganizationAbility(sutProvider, organization, true);

sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
sutProvider.GetDependency<ICollectionRepository>().GetManyByUserIdAsync(actingUserId, Arg.Any<bool>()).Returns(collections);
sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true);

foreach (var c in collections)
{
c.Manage = true;
}

var context = new AuthorizationHandlerContext(
new[] { BulkCollectionOperations.Delete },
new ClaimsPrincipal(),
collections);

await sutProvider.Sut.HandleAsync(context);

Assert.False(context.HasSucceeded);
}

[Theory, BitAutoData, CollectionCustomization]
public async Task CanDeleteAsync_WhenUser_LimitCollectionCreationDeletionTrue_AllowAdminAccessToAllCollectionItemsFalse_Failure(
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<CollectionDetails> collections,
CurrentContextOrganization organization)
{
var actingUserId = Guid.NewGuid();

organization.Type = OrganizationUserType.User;
organization.Permissions = new Permissions();

ArrangeOrganizationAbility(sutProvider, organization, true, false);

sutProvider.GetDependency<ICurrentContext>().UserId.Returns(actingUserId);
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
sutProvider.GetDependency<ICollectionRepository>().GetManyByUserIdAsync(actingUserId, Arg.Any<bool>()).Returns(collections);
sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true);

foreach (var c in collections)
{
c.Manage = true;
}

var context = new AuthorizationHandlerContext(
new[] { BulkCollectionOperations.Delete },
new ClaimsPrincipal(),
collections);

await sutProvider.Sut.HandleAsync(context);

Assert.False(context.HasSucceeded);
}

[Theory, CollectionCustomization]
[BitAutoData(OrganizationUserType.User)]
[BitAutoData(OrganizationUserType.Custom)]
Expand Down Expand Up @@ -1102,7 +1269,8 @@ public class BulkCollectionAuthorizationHandlerTests
{ collections.First().OrganizationId,
new OrganizationAbility
{
LimitCollectionCreationDeletion = true
LimitCollectionCreationDeletion = true,
AllowAdminAccessToAllCollectionItems = true
}
}
};
Expand Down Expand Up @@ -1177,12 +1345,14 @@ public class BulkCollectionAuthorizationHandlerTests

private static void ArrangeOrganizationAbility(
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
CurrentContextOrganization organization, bool limitCollectionCreationDeletion)
CurrentContextOrganization organization, bool limitCollectionCreationDeletion,
bool allowAdminAccessToAllCollectionItems = true)
{
var organizationAbility = new OrganizationAbility();
organizationAbility.Id = organization.Id;
organizationAbility.FlexibleCollections = true;
organizationAbility.LimitCollectionCreationDeletion = limitCollectionCreationDeletion;
organizationAbility.AllowAdminAccessToAllCollectionItems = allowAdminAccessToAllCollectionItems;

sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilityAsync(organizationAbility.Id)
.Returns(organizationAbility);
Expand Down