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

Delete duplicate BulkCollectionOperation.ReadWithAccess #4029

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Delete duplicate BulkCollectionOperation.ReadWithAccess #4029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78b48d1
Delete duplicate BulkCollectionOperation.ReadWithAccess
eliykat Apr 29, 2024
a80962b
dotnet format
eliykat Apr 29, 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
7 changes: 5 additions & 2 deletions src/Api/Controllers/CollectionsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,8 +553,11 @@
{
// New flexible collections logic
var (collection, access) = await _collectionRepository.GetByIdWithAccessAsync(id);
var authorized = (await _authorizationService.AuthorizeAsync(User, collection, BulkCollectionOperations.ReadWithAccess)).Succeeded;
if (!authorized)
var authorizationResult = await _authorizationService.AuthorizeAsync(
User,
collection,
new[] { BulkCollectionOperations.Read, BulkCollectionOperations.ReadAccess });

Check warning on line 559 in src/Api/Controllers/CollectionsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/Controllers/CollectionsController.cs#L556-L559 

Added lines #L556 - L559 were not covered by tests
if (!authorizationResult.Succeeded)
{
throw new NotFoundException();
}
Expand Down
38 changes: 1 addition & 37 deletions src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,6 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
await CanReadAsync(context, requirement, resources, org);
break;

case not null when requirement == BulkCollectionOperations.ReadWithAccess:
await CanReadWithAccessAsync(context, requirement, resources, org);
break;

case not null when requirement == BulkCollectionOperations.Update:
case not null when requirement == BulkCollectionOperations.ModifyAccess:
case not null when requirement == BulkCollectionOperations.ImportCiphers:
Expand Down Expand Up @@ -124,38 +120,6 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
if (org is
{ Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or
{ Permissions.EditAnyCollection: true } or
{ Permissions.DeleteAnyCollection: true })
{
context.Succeed(requirement);
return;
}

// The acting user is a member of the target organization,
// ensure they have access for the collection being read
if (org is not null)
{
var canManageCollections = await CanManageCollectionsAsync(resources);
if (canManageCollections)
{
context.Succeed(requirement);
return;
}
}

// Allow provider users to read collections if they are a provider for the target organization
if (await _currentContext.ProviderUserForOrgAsync(_targetOrganizationId))
{
context.Succeed(requirement);
}
}

private async Task CanReadWithAccessAsync(AuthorizationHandlerContext context, IAuthorizationRequirement requirement,
ICollection<Collection> resources, CurrentContextOrganization? org)
{
// Owners, Admins, and users with EditAnyCollection, DeleteAnyCollection or ManageUsers permission can always read a collection
if (org is
{ Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or
{ Permissions.EditAnyCollection: true } or
{ Permissions.DeleteAnyCollection: true } or
{ Permissions.ManageUsers: true })
{
Expand All @@ -164,7 +128,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
}

// The acting user is a member of the target organization,
// ensure they have access with manage permission for the collection being read
// ensure they have access for the collection being read
if (org is not null)
{
var canManageCollections = await CanManageCollectionsAsync(resources);
Expand Down
11 changes: 7 additions & 4 deletions src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@ public class BulkCollectionOperationRequirement : OperationAuthorizationRequirem
public static class BulkCollectionOperations
{
public static readonly BulkCollectionOperationRequirement Create = new() { Name = nameof(Create) };
/// <summary>
/// Represents reading the Collection object. Does not include reading user and group access - see ReadAccess.
/// </summary>
public static readonly BulkCollectionOperationRequirement Read = new() { Name = nameof(Read) };
/// <summary>
/// Represents reading the user and group access to a collection.
/// </summary>
public static readonly BulkCollectionOperationRequirement ReadAccess = new() { Name = nameof(ReadAccess) };
public static readonly BulkCollectionOperationRequirement ReadWithAccess = new() { Name = nameof(ReadWithAccess) };
public static readonly BulkCollectionOperationRequirement Update = new() { Name = nameof(Update) };
/// <summary>
/// The operation that represents creating, updating, or removing collection access.
/// Combined together to allow for a single requirement to be used for each operation
/// as they all currently share the same underlying authorization logic.
/// Represents creating, updating, or removing collection access.
/// </summary>
public static readonly BulkCollectionOperationRequirement ModifyAccess = new() { Name = nameof(ModifyAccess) };
public static readonly BulkCollectionOperationRequirement Delete = new() { Name = nameof(Delete) };
Expand Down
3 changes: 2 additions & 1 deletion test/Api.Test/Controllers/CollectionsControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ await sutProvider.GetDependency<ICollectionService>()
Arg.Any<object>(),
Arg.Is<IEnumerable<IAuthorizationRequirement>>(requirements =>
requirements.Cast<BulkCollectionOperationRequirement>().All(operation =>
operation.Name == nameof(BulkCollectionOperations.ReadWithAccess))))
operation.Name == nameof(BulkCollectionOperations.Read) ||
operation.Name == nameof(BulkCollectionOperations.ReadAccess))))
.Returns(AuthorizationResult.Success());

await sutProvider.Sut.GetManyWithDetails(organizationAbility.Id);
Expand Down
172 changes: 0 additions & 172 deletions test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,178 +362,6 @@ public class BulkCollectionAuthorizationHandlerTests
}
}

[Theory, CollectionCustomization]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task CanReadWithAccessAsync_WhenAdminOrOwner_Success(
OrganizationUserType userType,
Guid userId, SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<Collection> collections,
CurrentContextOrganization organization)
{
organization.Type = userType;
organization.Permissions = new Permissions();

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

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

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}

[Theory, CollectionCustomization]
[BitAutoData(true, false, false)]
[BitAutoData(false, true, false)]
[BitAutoData(false, false, true)]

public async Task CanReadWithAccessAsync_WhenCustomUserWithRequiredPermissions_Success(
bool editAnyCollection, bool deleteAnyCollection, bool manageUsers,
SutProvider<BulkCollectionAuthorizationHandler> sutProvider,
ICollection<Collection> collections,
CurrentContextOrganization organization)
{
var actingUserId = Guid.NewGuid();

organization.Type = OrganizationUserType.Custom;
organization.Permissions = new Permissions
{
EditAnyCollection = editAnyCollection,
DeleteAnyCollection = deleteAnyCollection,
ManageUsers = manageUsers
};

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

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

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}

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

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

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

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

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

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}

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

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

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

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

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

await sutProvider.Sut.HandleAsync(context);

Assert.False(context.HasSucceeded);
}

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

organization.Type = userType;
organization.Permissions = new Permissions
{
EditAnyCollection = false,
DeleteAnyCollection = false,
ManageGroups = false,
ManageUsers = false
};

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

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

await sutProvider.Sut.HandleAsync(context);

Assert.False(context.HasSucceeded);
}

[Theory, BitAutoData, CollectionCustomization]
public async Task CanReadWithAccessAsync_WhenMissingOrgAccess_NoSuccess(
Guid userId,
ICollection<Collection> collections,
SutProvider<BulkCollectionAuthorizationHandler> sutProvider)
{
sutProvider.GetDependency<ICurrentContext>().UserId.Returns(userId);
sutProvider.GetDependency<ICurrentContext>().GetOrganization(Arg.Any<Guid>()).Returns((CurrentContextOrganization)null);

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

await sutProvider.Sut.HandleAsync(context);

Assert.False(context.HasSucceeded);
}

[Theory, CollectionCustomization]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
Expand Down