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

[PM-12479] - Adding group-details endpoint #4959

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+241 โˆ’139
Open

[PM-12479] - Adding group-details endpoint #4959

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3621491
Added group-details endpoint.
jrmccannon Oct 29, 2024
c5cd432
added feature flag.
jrmccannon Oct 30, 2024
8fecf61
Added stubbed out CanViewDetails Requirement.
jrmccannon Oct 30, 2024
80d090f
Adding ReadDetail operation logic and tests.
jrmccannon Oct 30, 2024
6cfe7b3
Moving Group auth into Admin Console core code.
jrmccannon Oct 31, 2024
ccdb29c
Moved OrgScope to shared folder. Moved auth stuff to admin console foโ€ฆ
jrmccannon Oct 31, 2024
3ae2399
formatting
jrmccannon Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 38 additions & 17 deletions src/Api/AdminConsole/Controllers/GroupsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using Bit.Api.Models.Response;
using Bit.Api.Vault.AuthorizationHandlers.Collections;
using Bit.Api.Vault.AuthorizationHandlers.Groups;
using Bit.Core;
using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.AdminConsole.Services;
Expand All @@ -16,7 +17,7 @@

namespace Bit.Api.AdminConsole.Controllers;

[Route("organizations/{orgId}/groups")]
[Route("organizations/{orgId}")]
Comment on lines -19 to +20
Copy link
Member

Choose a reason for hiding this comment

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

Why remove groups from the top-level route here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The top level was removed since the new route would be organizations/{id}/group-details since group details isn't following /groups. I believe the only way to override that would be to put a / at the start of the route, but then you'd override any additional route pathing that was set up higher in the stack.

Copy link
Member

Choose a reason for hiding this comment

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

In that case I suggest leaving groups in the top-level route and using details at the method level, i.e. organizations/{id}/groups/details. (We already do this for collection details.) My main concern is that all our controllers use a controller-level route, this would be an outlier that is easily missed. This also keeps all groups-related resources under the groups path which I think might be better REST design (although that might be arguable).

Copy link
Member

Choose a reason for hiding this comment

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

I see I specified group-details in the Jira ticket, that's my bad!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. Easy fix. I'll move group back to the top and make this one details. I'll update the client code accordingly as well.

[Authorize("Application")]
public class GroupsController : Controller
{
Expand Down Expand Up @@ -64,7 +65,7 @@ public GroupsController(
_collectionRepository = collectionRepository;
}

[HttpGet("{id}")]
[HttpGet("groups/{id}")]
public async Task<GroupResponseModel> Get(string orgId, string id)
{
var group = await _groupRepository.GetByIdAsync(new Guid(id));
Expand All @@ -76,7 +77,7 @@ public async Task<GroupResponseModel> Get(string orgId, string id)
return new GroupResponseModel(group);
}

[HttpGet("{id}/details")]
[HttpGet("groups/{id}/details")]
public async Task<GroupDetailsResponseModel> GetDetails(string orgId, string id)
{
var groupDetails = await _groupRepository.GetByIdWithCollectionsAsync(new Guid(id));
Expand All @@ -88,11 +89,31 @@ public async Task<GroupDetailsResponseModel> GetDetails(string orgId, string id)
return new GroupDetailsResponseModel(groupDetails.Item1, groupDetails.Item2);
}

[HttpGet("")]
public async Task<ListResponseModel<GroupDetailsResponseModel>> Get(Guid orgId)
[HttpGet("groups")]
public async Task<ListResponseModel<GroupDetailsResponseModel>> GetOrganizationGroups(Guid orgId)
{
var authorized =
(await _authorizationService.AuthorizeAsync(User, GroupOperations.ReadAll(orgId))).Succeeded;
var authorized = (await _authorizationService.AuthorizeAsync(User, GroupOperations.ReadAll(orgId))).Succeeded;
if (!authorized)
{
throw new NotFoundException();
}

if (_featureService.IsEnabled(FeatureFlagKeys.SecureOrgGroupDetails))
{
var groups = await _groupRepository.GetManyByOrganizationIdAsync(orgId);
var responses = groups.Select(g => new GroupDetailsResponseModel(g, []));
return new ListResponseModel<GroupDetailsResponseModel>(responses);
}

var groupDetails = await _groupRepository.GetManyWithCollectionsByOrganizationIdAsync(orgId);
var detailResponses = groupDetails.Select(g => new GroupDetailsResponseModel(g.Item1, g.Item2));
return new ListResponseModel<GroupDetailsResponseModel>(detailResponses);
}

[HttpGet("group-details")]
public async Task<ListResponseModel<GroupDetailsResponseModel>> GetOrganizationGroupDetails(Guid orgId)
{
var authorized = (await _authorizationService.AuthorizeAsync(User, GroupOperations.ReadDetails(orgId))).Succeeded;
if (!authorized)
{
throw new NotFoundException();
Expand All @@ -103,7 +124,7 @@ public async Task<ListResponseModel<GroupDetailsResponseModel>> Get(Guid orgId)
return new ListResponseModel<GroupDetailsResponseModel>(responses);
}

[HttpGet("{id}/users")]
[HttpGet("groups/{id}/users")]
public async Task<IEnumerable<Guid>> GetUsers(string orgId, string id)
{
var idGuid = new Guid(id);
Expand All @@ -117,7 +138,7 @@ public async Task<IEnumerable<Guid>> GetUsers(string orgId, string id)
return groupIds;
}

[HttpPost("")]
[HttpPost("groups")]
public async Task<GroupResponseModel> Post(Guid orgId, [FromBody] GroupRequestModel model)
{
if (!await _currentContext.ManageGroups(orgId))
Expand Down Expand Up @@ -145,8 +166,8 @@ public async Task<GroupResponseModel> Post(Guid orgId, [FromBody] GroupRequestMo
return new GroupResponseModel(group);
}

[HttpPut("{id}")]
[HttpPost("{id}")]
[HttpPut("groups/{id}")]
[HttpPost("groups/{id}")]
public async Task<GroupResponseModel> Put(Guid orgId, Guid id, [FromBody] GroupRequestModel model)
{
if (!await _currentContext.ManageGroups(orgId))
Expand Down Expand Up @@ -220,8 +241,8 @@ public async Task<GroupResponseModel> Put(Guid orgId, Guid id, [FromBody] GroupR
return new GroupResponseModel(group);
}

[HttpDelete("{id}")]
[HttpPost("{id}/delete")]
[HttpDelete("groups/{id}")]
[HttpPost("groups/{id}/delete")]
public async Task Delete(string orgId, string id)
{
var group = await _groupRepository.GetByIdAsync(new Guid(id));
Expand All @@ -233,8 +254,8 @@ public async Task Delete(string orgId, string id)
await _deleteGroupCommand.DeleteAsync(group);
}

[HttpDelete("")]
[HttpPost("delete")]
[HttpDelete("groups/")]
[HttpPost("groups/delete")]
public async Task BulkDelete([FromBody] GroupBulkRequestModel model)
{
var groups = await _groupRepository.GetManyByManyIds(model.Ids);
Expand All @@ -250,8 +271,8 @@ public async Task BulkDelete([FromBody] GroupBulkRequestModel model)
await _deleteGroupCommand.DeleteManyAsync(groups);
}

[HttpDelete("{id}/user/{orgUserId}")]
[HttpPost("{id}/delete-user/{orgUserId}")]
[HttpDelete("groups/{id}/user/{orgUserId}")]
[HttpPost("groups/{id}/delete-user/{orgUserId}")]
public async Task Delete(string orgId, string id, string orgUserId)
{
var group = await _groupRepository.GetByIdAsync(new Guid(id));
Expand Down
19 changes: 19 additions & 0 deletions src/Api/Vault/AuthorizationHandlers/Groups/GroupAuthorizationHandler.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.

This class needs to be updated a little to follow the newest patterns.

  1. move it out of Vault ownership - this was a mistake when it was first merged. I suggest Core/AdminConsole/OrganizationFeatures/Groups/Authorization.
  2. see the OrganizationUserUserMiniDetailsAuthorizationHandler as an example for how to structure handlers. Note in particular:
    • use of OrganizationScope as the resource, rather than putting the orgId in the requirement (this represents "read all resources of X type for org Y" rather than particular resource IDs)
    • calling context.Succeed from the public method - relatively minor but cleans up the implementation a little
    • I would've said separate handlers and OperationRequirement classes for separate resource types (e.g. GroupAuthorizationHandler vs GroupDetailsAuthorizationHandler) but that causes a lot of overhead - so I think what you've done here is OK. Just make the operations very explicit, e.g. ReadAllGroups vs. ReadAllGroupDetails.

We're still refining our implementation of these so I'm not sure how clear this all is. See the Authentication Deep Dive for more info or feel free to send me a meeting invite if you want to sync on it.

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
๏ปฟ#nullable enable
using Bit.Core.Context;
using Bit.Core.Enums;
using Microsoft.AspNetCore.Authorization;

namespace Bit.Api.Vault.AuthorizationHandlers.Groups;
Expand Down Expand Up @@ -40,6 +41,9 @@ protected override async Task HandleRequirementAsync(AuthorizationHandlerContext
case not null when requirement.Name == nameof(GroupOperations.ReadAll):
await CanReadAllAsync(context, requirement, org);
break;
case not null when requirement.Name == nameof(GroupOperations.ReadDetails):
await CanViewGroupDetailsAsync(context, requirement, org);
break;
}
}

Expand All @@ -59,4 +63,19 @@ private async Task CanReadAllAsync(AuthorizationHandlerContext context, GroupOpe
context.Succeed(requirement);
}
}

private async Task CanViewGroupDetailsAsync(AuthorizationHandlerContext context, GroupOperationRequirement requirement,
CurrentContextOrganization? org)
{
if (org is not null
&& (org.Permissions.ManageUsers
|| org.Permissions.ManageGroups
|| org.Type == OrganizationUserType.Admin
|| org.Type == OrganizationUserType.Owner
|| await _currentContext.ProviderUserForOrgAsync(org.Id)))
{

context.Succeed(requirement);
}
}
}
5 changes: 5 additions & 0 deletions src/Api/Vault/AuthorizationHandlers/Groups/GroupOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,9 @@ public static GroupOperationRequirement ReadAll(Guid organizationId)
{
return new GroupOperationRequirement(nameof(ReadAll), organizationId);
}

public static GroupOperationRequirement ReadDetails(Guid organizationId)
{
return new GroupOperationRequirement(nameof(ReadDetails), organizationId);
}
}
1 change: 1 addition & 0 deletions src/Core/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ public static class FeatureFlagKeys
public const string StorageReseedRefactor = "storage-reseed-refactor";
public const string TrialPayment = "PM-8163-trial-payment";
public const string RemoveServerVersionHeader = "remove-server-version-header";
public const string SecureOrgGroupDetails = "pm-12479-secure-org-group-details";
public const string AccessIntelligence = "pm-13227-access-intelligence";
public const string VerifiedSsoDomainEndpoint = "pm-12337-refactor-sso-details-endpoint";
public const string PM12275_MultiOrganizationEnterprises = "pm-12275-multi-organization-enterprises";
Expand Down
133 changes: 133 additions & 0 deletions test/Api.Test/Vault/AuthorizationHandlers/GroupAuthorizationHandlerTests.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.

This is optional depending on how deep you want to go with testing. But xUnit supports an attribute called MemberData which can be used to parameterize tests with complex objects: https://hamidmosalla.com/2017/02/25/xunit-theory-working-with-inlinedata-memberdata-classdata/ (scroll down to the Member Data heading). All these tests are quite similar, I wonder if you could combine almost all of them by defining an object containing data for all test cases (e.g. all combinations of role, custom permissions, expectedResult) and consuming it with the MemberData attribute in a single test.

We have a wrapper around it called BitMemberAutoDataAttribute which may provide some examples.

Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,137 @@ public async Task HandleRequirementAsync_NoSpecifiedOrgId_Failure(
Assert.False(context.HasSucceeded);
Assert.True(context.HasFailed);
}

[Theory]
[BitAutoData(OrganizationUserType.Admin)]
[BitAutoData(OrganizationUserType.Owner)]
public async Task HandleRequirementsAsync_GivenViewDetailsOperation_WhenUserIsAdminOwner_ThenShouldSucceed(OrganizationUserType userType,
Guid userId, CurrentContextOrganization organization, SutProvider<GroupAuthorizationHandler> sutProvider)
{
organization.Type = userType;
organization.Permissions = new Permissions();

var context = new AuthorizationHandlerContext(
[GroupOperations.ReadDetails(organization.Id)],
new ClaimsPrincipal(),
null
);

sutProvider.GetDependency<ICurrentContext>().UserId.Returns(userId);
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 HandleRequirementsAsync_GivenViewDetailsOperation_WhenUserIsNotOwnerOrAdmin_ThenShouldFail(OrganizationUserType userType,
Guid userId, CurrentContextOrganization organization, SutProvider<GroupAuthorizationHandler> sutProvider)
{
organization.Type = userType;
organization.Permissions = new Permissions();

var context = new AuthorizationHandlerContext(
[GroupOperations.ReadDetails(organization.Id)],
new ClaimsPrincipal(),
null
);

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

await sutProvider.Sut.HandleAsync(context);
Assert.False(context.HasSucceeded);
}

[Theory, BitAutoData]
public async Task HandleRequirementsAsync_GivenViewDetailsOperation_WhenUserHasManageGroupPermission_ThenShouldSucceed(
Guid userId, CurrentContextOrganization organization, SutProvider<GroupAuthorizationHandler> sutProvider)
{
organization.Type = OrganizationUserType.Custom;
organization.Permissions = new Permissions
{
ManageGroups = true
};

var context = new AuthorizationHandlerContext(
[GroupOperations.ReadDetails(organization.Id)],
new ClaimsPrincipal(),
null
);

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

await sutProvider.Sut.HandleAsync(context);
Assert.True(context.HasSucceeded);
}

[Theory, BitAutoData]
public async Task HandleRequirementsAsync_GivenViewDetailsOperation_WhenUserHasManageUserPermission_ThenShouldSucceed(
Guid userId, CurrentContextOrganization organization, SutProvider<GroupAuthorizationHandler> sutProvider)
{
organization.Type = OrganizationUserType.Custom;
organization.Permissions = new Permissions
{
ManageUsers = true
};

var context = new AuthorizationHandlerContext(
[GroupOperations.ReadDetails(organization.Id)],
new ClaimsPrincipal(),
null
);

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

await sutProvider.Sut.HandleAsync(context);
Assert.True(context.HasSucceeded);
}

[Theory, BitAutoData]
public async Task HandleRequirementsAsync_GivenViewDetailsOperation_WhenUserIsStandardUserTypeWithoutElevatedPermissions_ThenShouldFail(
Guid userId, CurrentContextOrganization organization, SutProvider<GroupAuthorizationHandler> sutProvider)
{
organization.Type = OrganizationUserType.User;
organization.Permissions = new Permissions();

var context = new AuthorizationHandlerContext(
[GroupOperations.ReadDetails(organization.Id)],
new ClaimsPrincipal(),
null
);

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

await sutProvider.Sut.HandleAsync(context);
Assert.False(context.HasSucceeded);
}

[Theory, BitAutoData]
public async Task HandleRequirementsAsync_GivenViewDetailsOperation_WhenIsProviderUser_ThenShouldSucceed(
Guid userId,
SutProvider<GroupAuthorizationHandler> sutProvider, CurrentContextOrganization organization)
{
organization.Type = OrganizationUserType.User;
organization.Permissions = new Permissions();

var context = new AuthorizationHandlerContext(
new[] { GroupOperations.ReadAll(organization.Id) },
new ClaimsPrincipal(),
null);

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

await sutProvider.Sut.HandleAsync(context);

Assert.True(context.HasSucceeded);
}
}
Loading