Upstream/bitwarden-server
1
0
Fork 0
mirror of https://github.com/bitwarden/server.git synced 2024-11-21 12:05:42 +01:00
Code Issues Projects Releases Wiki Activity

[AC-1981] Fix CollectionsController.Get auth check by just checking collections for the requested orgId (#3575)

Browse Source 
* Fixed auth check by just checking collections for the requested orgId

* [AC-1139] Refactor collection authorization logic to check for manage permission

* [AC-1139] Remove unnecessary authorization check in CollectionsController

* [AC-1139] Remove unused test method

* [AC-1139] Remove unnecessary code for checking read permissions
This commit is contained in:
Rui Tomé 2023-12-20 16:34:09 +00:00 committed by GitHub
parent ca750e226f
commit 72ebb5e66f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 62 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
src/Api/Controllers/CollectionsController.cs
View File

@ -584,11 +584,6 @@ public class CollectionsController : Controller
// Filter the assigned collections to only return those where the user has Manage permission
var manageableOrgCollections = assignedOrgCollections.Where(c => c.Item1.Manage).ToList();
var readAssignedAuthorized = await _authorizationService.AuthorizeAsync(User, manageableOrgCollections.Select(c => c.Item1), BulkCollectionOperations.ReadWithAccess);
if (!readAssignedAuthorized.Succeeded)
{
throw new NotFoundException();
}
return new ListResponseModel<CollectionAccessDetailsResponseModel>(manageableOrgCollections.Select(c =>
new CollectionAccessDetailsResponseModel(c.Item1, c.Item2.Groups, c.Item2.Users)
@ -609,16 +604,8 @@ public class CollectionsController : Controller
}
else
{
var collections = await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId.Value, FlexibleCollectionsIsEnabled);
var readAuthorized = (await _authorizationService.AuthorizeAsync(User, collections, BulkCollectionOperations.Read)).Succeeded;
if (readAuthorized)
{
orgCollections = collections.Where(c => c.OrganizationId == orgId);
}
else
{
throw new NotFoundException();
}
var assignedCollections = await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId.Value, FlexibleCollectionsIsEnabled);
orgCollections = assignedCollections.Where(c => c.OrganizationId == orgId && c.Manage).ToList();
}
var responses = orgCollections.Select(c => new CollectionResponseModel(c));

19
src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs
View File

@ -131,8 +131,8 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
// ensure they have access for the collection being read
if (org is not null)
{
var isAssignedToCollections = await IsAssignedToCollectionsAsync(resources, org, false);
if (isAssignedToCollections)
var canManageCollections = await CanManageCollectionsAsync(resources, org);
if (canManageCollections)
{
context.Succeed(requirement);
return;
@ -164,8 +164,8 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
// ensure they have access with manage permission for the collection being read
if (org is not null)
{
var isAssignedToCollections = await IsAssignedToCollectionsAsync(resources, org, true);
if (isAssignedToCollections)
var canManageCollections = await CanManageCollectionsAsync(resources, org);
if (canManageCollections)
{
context.Succeed(requirement);
return;
@ -199,7 +199,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
// ensure they have manage permission for the collection being managed
if (org is not null)
{
var canManageCollections = await IsAssignedToCollectionsAsync(resources, org, true);
var canManageCollections = await CanManageCollectionsAsync(resources, org);
if (canManageCollections)
{
context.Succeed(requirement);
@ -230,7 +230,7 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
// ensure acting user has manage permissions for all collections being deleted
if (org is { LimitCollectionCreationDeletion: false })
{
var canManageCollections = await IsAssignedToCollectionsAsync(resources, org, true);
var canManageCollections = await CanManageCollectionsAsync(resources, org);
if (canManageCollections)
{
context.Succeed(requirement);
@ -245,17 +245,16 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler<BulkC
}
}
private async Task<bool> IsAssignedToCollectionsAsync(
private async Task<bool> CanManageCollectionsAsync(
ICollection<Collection> targetCollections,
CurrentContextOrganization org,
bool requireManagePermission)
CurrentContextOrganization org)
{
// List of collection Ids the acting user has access to
var assignedCollectionIds =
(await _collectionRepository.GetManyByUserIdAsync(_currentContext.UserId!.Value, useFlexibleCollections: true))
.Where(c =>
// Check Collections with Manage permission
(!requireManagePermission || c.Manage) && c.OrganizationId == org.Id)
c.Manage && c.OrganizationId == org.Id)
.Select(c => c.Id)
.ToHashSet();

50
test/Api.Test/Controllers/CollectionsControllerTests.cs
View File

@ -142,7 +142,7 @@ public class CollectionsControllerTests
}
[Theory, BitAutoData]
public async Task GetOrganizationCollectionsWithGroups_MissingReadPermissions_ThrowsNotFound(Organization organization, Guid userId, SutProvider<CollectionsController> sutProvider)
public async Task GetOrganizationCollections_WithReadAllPermissions_GetsAllCollections(Organization organization, ICollection<Collection> collections, Guid userId, SutProvider<CollectionsController> sutProvider)
{
sutProvider.GetDependency<ICurrentContext>().UserId.Returns(userId);
@ -152,7 +152,37 @@ public class CollectionsControllerTests
Arg.Any<object>(),
Arg.Is<IEnumerable<IAuthorizationRequirement>>(requirements =>
requirements.Cast<CollectionOperationRequirement>().All(operation =>
operation.Name == nameof(CollectionOperations.ReadAllWithAccess)
operation.Name == nameof(CollectionOperations.ReadAll)
&& operation.OrganizationId == organization.Id)))
.Returns(AuthorizationResult.Success());
sutProvider.GetDependency<ICollectionRepository>()
.GetManyByOrganizationIdAsync(organization.Id)
.Returns(collections);
var response = await sutProvider.Sut.Get(organization.Id);
await sutProvider.GetDependency<ICollectionRepository>().Received(1).GetManyByOrganizationIdAsync(organization.Id);
Assert.Equal(collections.Count, response.Data.Count());
}
[Theory, BitAutoData]
public async Task GetOrganizationCollections_MissingReadAllPermissions_GetsManageableCollections(Organization organization, ICollection<CollectionDetails> collections, Guid userId, SutProvider<CollectionsController> sutProvider)
{
collections.First().OrganizationId = organization.Id;
collections.First().Manage = true;
collections.Skip(1).First().OrganizationId = organization.Id;
sutProvider.GetDependency<ICurrentContext>().UserId.Returns(userId);
sutProvider.GetDependency<IAuthorizationService>()
.AuthorizeAsync(
Arg.Any<ClaimsPrincipal>(),
Arg.Any<object>(),
Arg.Is<IEnumerable<IAuthorizationRequirement>>(requirements =>
requirements.Cast<CollectionOperationRequirement>().All(operation =>
operation.Name == nameof(CollectionOperations.ReadAll)
&& operation.OrganizationId == organization.Id)))
.Returns(AuthorizationResult.Failed());
@ -162,10 +192,20 @@ public class CollectionsControllerTests
Arg.Any<object>(),
Arg.Is<IEnumerable<IAuthorizationRequirement>>(requirements =>
requirements.Cast<BulkCollectionOperationRequirement>().All(operation =>
operation.Name == nameof(BulkCollectionOperations.ReadWithAccess))))
.Returns(AuthorizationResult.Failed());
operation.Name == nameof(BulkCollectionOperations.Read))))
.Returns(AuthorizationResult.Success());
_ = await Assert.ThrowsAsync<NotFoundException>(async () => await sutProvider.Sut.GetManyWithDetails(organization.Id));
sutProvider.GetDependency<ICollectionRepository>()
.GetManyByUserIdAsync(userId, true)
.Returns(collections);
var result = await sutProvider.Sut.Get(organization.Id);
await sutProvider.GetDependency<ICollectionRepository>().DidNotReceive().GetManyByOrganizationIdAsync(organization.Id);
await sutProvider.GetDependency<ICollectionRepository>().Received(1).GetManyByUserIdAsync(userId, true);
Assert.Single(result.Data);
Assert.All(result.Data, c => Assert.Equal(organization.Id, c.OrganizationId));
}
[Theory, BitAutoData]

7
test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs
View File

@ -204,13 +204,18 @@ public class BulkCollectionAuthorizationHandlerTests
}
[Theory, BitAutoData, CollectionCustomization]
public async Task CanReadAsync_WhenUserIsAssignedToCollections_Success(
public async Task CanReadAsync_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.LimitCollectionCreationDeletion = false;
organization.Permissions = new Permissions();