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.

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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 41 additions & 20 deletions src/Api/AdminConsole/Controllers/GroupsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,22 @@
using Bit.Api.AdminConsole.Models.Response;
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.Authorization;
using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.AdminConsole.Services;
using Bit.Core.Context;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Utilities;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;

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 @@
_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 @@
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,12 +89,32 @@
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;
if (!authorized)
var authResult = await _authorizationService.AuthorizeAsync(User, new OrganizationScope(orgId), GroupOperations.ReadAll);

Check warning on line 95 in src/Api/AdminConsole/Controllers/GroupsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/GroupsController.cs#L95

Added line #L95 was not covered by tests
if (!authResult.Succeeded)
{
throw new NotFoundException();

Check warning on line 98 in src/Api/AdminConsole/Controllers/GroupsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/GroupsController.cs#L97-L98

Added lines #L97 - L98 were not covered by tests
}

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

Check warning on line 105 in src/Api/AdminConsole/Controllers/GroupsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/GroupsController.cs#L102-L105

Added lines #L102 - L105 were not covered by tests
}

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

Check warning on line 111 in src/Api/AdminConsole/Controllers/GroupsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/GroupsController.cs#L108-L111

Added lines #L108 - L111 were not covered by tests

[HttpGet("group-details")]
public async Task<ListResponseModel<GroupDetailsResponseModel>> GetOrganizationGroupDetails(Guid orgId)
{
var authResult = await _authorizationService.AuthorizeAsync(User, new OrganizationScope(orgId), GroupOperations.ReadAllDetails);

Check warning on line 116 in src/Api/AdminConsole/Controllers/GroupsController.cs

View check run for this annotation

Codecov / codecov/patch

src/Api/AdminConsole/Controllers/GroupsController.cs#L115-L116

Added lines #L115 - L116 were not covered by tests
if (!authResult.Succeeded)
{
throw new NotFoundException();
}
Expand All @@ -103,7 +124,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Bit.Core.AdminConsole.Models.Data.Organizations.Policies;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization;
using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces;
using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.Auth.Enums;
using Bit.Core.Auth.Repositories;
Expand Down
2 changes: 1 addition & 1 deletion src/Api/Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,5 @@
<PackageReference Include="Azure.Messaging.EventGrid" Version="4.25.0" />
<PackageReference Include="Swashbuckle.AspNetCore" Version="6.8.1" />
</ItemGroup>

</Project>
2 changes: 1 addition & 1 deletion src/Api/Utilities/ServiceCollectionExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
๏ปฟusing Bit.Api.Vault.AuthorizationHandlers.Collections;
using Bit.Api.Vault.AuthorizationHandlers.Groups;
using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Authorization;
using Bit.Core.IdentityServer;
using Bit.Core.Settings;
using Bit.Core.Utilities;
Expand Down

This file was deleted.

22 changes: 0 additions & 22 deletions src/Api/Vault/AuthorizationHandlers/Groups/GroupOperations.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
๏ปฟ#nullable enable
using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization;
using Bit.Core.Context;
using Bit.Core.Enums;
using Microsoft.AspNetCore.Authorization;

namespace Bit.Core.AdminConsole.OrganizationFeatures.Groups.Authorization;

/// <summary>
/// Handles authorization logic for Group operations.
/// This uses new logic implemented in the Flexible Collections initiative.
/// </summary>
public class GroupAuthorizationHandler(ICurrentContext currentContext)
: AuthorizationHandler<GroupOperationRequirement, OrganizationScope>
{
protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context,
GroupOperationRequirement requirement, OrganizationScope organizationScope)
{
var authorized = requirement switch
{
not null when requirement.Name == nameof(GroupOperations.ReadAll) =>
await CanReadAllAsync(organizationScope),
not null when requirement.Name == nameof(GroupOperations.ReadAllDetails) =>
await CanViewGroupDetailsAsync(organizationScope),
_ => false

Check warning on line 25 in src/Core/AdminConsole/OrganizationFeatures/Groups/Authorization/GroupAuthorizationHandler.cs

View check run for this annotation

Codecov / codecov/patch

src/Core/AdminConsole/OrganizationFeatures/Groups/Authorization/GroupAuthorizationHandler.cs#L25

Added line #L25 was not covered by tests
};

if (requirement is not null && authorized)
{
context.Succeed(requirement);
}
}

private async Task<bool> CanReadAllAsync(OrganizationScope organizationScope) =>
currentContext.GetOrganization(organizationScope) is not null
|| await currentContext.ProviderUserForOrgAsync(organizationScope);

private async Task<bool> CanViewGroupDetailsAsync(OrganizationScope organizationScope) =>
currentContext.GetOrganization(organizationScope) is { Type: OrganizationUserType.Owner } or { Type: OrganizationUserType.Admin }
or { Permissions: { ManageGroups: true } or { ManageUsers: true } }
|| await currentContext.ProviderUserForOrgAsync(organizationScope);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
๏ปฟusing Microsoft.AspNetCore.Authorization.Infrastructure;

namespace Bit.Core.AdminConsole.OrganizationFeatures.Groups.Authorization;

public class GroupOperationRequirement : OperationAuthorizationRequirement
{
public GroupOperationRequirement(string name)
{
Name = name;
}
}

public static class GroupOperations
{
public static readonly GroupOperationRequirement ReadAll = new(nameof(ReadAll));
public static readonly GroupOperationRequirement ReadAllDetails = new(nameof(ReadAllDetails));
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
๏ปฟ#nullable enable
using Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization;
using Bit.Core.Context;
using Bit.Core.Enums;
using Microsoft.AspNetCore.Authorization;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,16 @@
๏ปฟusing Bit.Core.Context;
using Bit.Core.Services;
๏ปฟusing Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization;
using Bit.Core.Context;
using Microsoft.AspNetCore.Authorization;

namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization;

public class OrganizationUserUserMiniDetailsAuthorizationHandler :
AuthorizationHandler<OrganizationUserUserMiniDetailsOperationRequirement, OrganizationScope>
{
private readonly IApplicationCacheService _applicationCacheService;
private readonly ICurrentContext _currentContext;

public OrganizationUserUserMiniDetailsAuthorizationHandler(
IApplicationCacheService applicationCacheService,
ICurrentContext currentContext)
public OrganizationUserUserMiniDetailsAuthorizationHandler(ICurrentContext currentContext)
{
_applicationCacheService = applicationCacheService;
_currentContext = currentContext;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
๏ปฟ#nullable enable

namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Authorization;
namespace Bit.Core.AdminConsole.OrganizationFeatures.Shared.Authorization;

/// <summary>
/// A typed wrapper for an organization Guid. This is used for authorization checks
Expand Down
1 change: 1 addition & 0 deletions src/Core/Constants.cs
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
Loading
Loading