Skip to content

Commit

Permalink
[AC-1713] [Flexible collections] Add feature flags to server (#3334)
Browse files Browse the repository at this point in the history 
* Add feature flags for FlexibleCollections and BulkCollectionAccess

* Flag new routes and behaviour

---------

Co-authored-by: Rui Tomé <[email protected]>
  • Loading branch information
@eliykat @r-tome
eliykat and r-tome authored Oct 17, 2023
1 parent 6bc38ac commit 3b049a6
Show file tree
Hide file tree
Showing 10 changed files with 479 additions and 20 deletions.
115 changes: 105 additions & 10 deletions src/Api/Controllers/CollectionsController.cs
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
using Bit.Api.Models.Request;
using Bit.Api.Models.Response;
using Bit.Api.Vault.AuthorizationHandlers.Collections;
using Bit.Core;
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Exceptions;
using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Utilities;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;

Expand All @@ -23,6 +25,7 @@ public class CollectionsController : Controller
private readonly IAuthorizationService _authorizationService;
private readonly ICurrentContext _currentContext;
private readonly IBulkAddCollectionAccessCommand _bulkAddCollectionAccessCommand;
private readonly IFeatureService _featureService;

public CollectionsController(
ICollectionRepository collectionRepository,
Expand All @@ -31,7 +34,8 @@ public CollectionsController(
IUserService userService,
IAuthorizationService authorizationService,
ICurrentContext currentContext,
IBulkAddCollectionAccessCommand bulkAddCollectionAccessCommand)
IBulkAddCollectionAccessCommand bulkAddCollectionAccessCommand,
IFeatureService featureService)
{
_collectionRepository = collectionRepository;
_collectionService = collectionService;
Expand All @@ -40,8 +44,11 @@ public CollectionsController(
_authorizationService = authorizationService;
_currentContext = currentContext;
_bulkAddCollectionAccessCommand = bulkAddCollectionAccessCommand;
_featureService = featureService;
}

private bool FlexibleCollectionsIsEnabled => _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollections, _currentContext);

[HttpGet("{id}")]
public async Task<CollectionResponseModel> Get(Guid orgId, Guid id)
{
Expand Down Expand Up @@ -152,8 +159,10 @@ public async Task<CollectionResponseModel> Post(Guid orgId, [FromBody] Collectio
{
var collection = model.ToCollection(orgId);

var result = await _authorizationService.AuthorizeAsync(User, collection, CollectionOperations.Create);
if (!result.Succeeded)
var authorized = FlexibleCollectionsIsEnabled
? (await _authorizationService.AuthorizeAsync(User, collection, CollectionOperations.Create)).Succeeded
: await CanCreateCollection(orgId, collection.Id) || await CanEditCollectionAsync(orgId, collection.Id);
if (!authorized)
{
throw new NotFoundException();
}
Expand Down Expand Up @@ -194,6 +203,10 @@ public async Task PutUsers(Guid orgId, Guid id, [FromBody] IEnumerable<Selection
}

[HttpPost("bulk-access")]
[RequireFeature(FeatureFlagKeys.BulkCollectionAccess)]
// Also gated behind Flexible Collections flag because it only has new authorization logic.
// Could be removed if legacy authorization logic were implemented for many collections.
[RequireFeature(FeatureFlagKeys.FlexibleCollections)]
public async Task PostBulkCollectionAccess([FromBody] BulkCollectionAccessRequestModel model)
{
var collections = await _collectionRepository.GetManyByManyIdsAsync(model.CollectionIds);
Expand Down Expand Up @@ -221,8 +234,11 @@ await _bulkAddCollectionAccessCommand.AddAccessAsync(
public async Task Delete(Guid orgId, Guid id)
{
var collection = await GetCollectionAsync(id, orgId);
var result = await _authorizationService.AuthorizeAsync(User, collection, CollectionOperations.Delete);
if (!result.Succeeded)

var authorized = FlexibleCollectionsIsEnabled
? (await _authorizationService.AuthorizeAsync(User, collection, CollectionOperations.Delete)).Succeeded
: await CanDeleteCollectionAsync(orgId, id);
if (!authorized)
{
throw new NotFoundException();
}
Expand All @@ -232,16 +248,38 @@ public async Task Delete(Guid orgId, Guid id)

[HttpDelete("")]
[HttpPost("delete")]
public async Task DeleteMany([FromBody] CollectionBulkDeleteRequestModel model)
public async Task DeleteMany(Guid orgId, [FromBody] CollectionBulkDeleteRequestModel model)
{
var collections = await _collectionRepository.GetManyByManyIdsAsync(model.Ids);
var result = await _authorizationService.AuthorizeAsync(User, collections, CollectionOperations.Delete);
if (!result.Succeeded)
if (FlexibleCollectionsIsEnabled)
{
// New flexible collections logic
var collections = await _collectionRepository.GetManyByManyIdsAsync(model.Ids);
var result = await _authorizationService.AuthorizeAsync(User, collections, CollectionOperations.Delete);
if (!result.Succeeded)
{
throw new NotFoundException();
}

await _deleteCollectionCommand.DeleteManyAsync(collections);
return;
}

// Old pre-flexible collections logic follows
if (!await _currentContext.DeleteAssignedCollections(orgId) && !await DeleteAnyCollection(orgId))
{
throw new NotFoundException();
}

await _deleteCollectionCommand.DeleteManyAsync(collections);
var userCollections = await _collectionService.GetOrganizationCollectionsAsync(orgId);
var filteredCollections = userCollections
.Where(c => model.Ids.Contains(c.Id) && c.OrganizationId == orgId);

if (!filteredCollections.Any())
{
throw new BadRequestException("No collections found.");
}

await _deleteCollectionCommand.DeleteManyAsync(filteredCollections);
}

[HttpDelete("{id}/user/{orgUserId}")]
Expand Down Expand Up @@ -272,6 +310,28 @@ private async Task<Collection> GetCollectionAsync(Guid id, Guid orgId)
return collection;
}

private void DeprecatedPermissionsGuard()
{
if (FlexibleCollectionsIsEnabled)
{
throw new FeatureUnavailableException("Flexible Collections is ON when it should be OFF.");
}
}

[Obsolete("Pre-Flexible Collections logic. Will be replaced by CollectionsAuthorizationHandler.")]
private async Task<bool> CanCreateCollection(Guid orgId, Guid collectionId)
{
DeprecatedPermissionsGuard();

if (collectionId != default)
{
return false;
}

return await _currentContext.OrganizationManager(orgId) || (_currentContext.Organizations?.Any(o => o.Id == orgId &&
(o.Permissions?.CreateNewCollections ?? false)) ?? false);
}

private async Task<bool> CanEditCollectionAsync(Guid orgId, Guid collectionId)
{
if (collectionId == default)
Expand All @@ -294,6 +354,41 @@ private async Task<bool> CanEditCollectionAsync(Guid orgId, Guid collectionId)
return false;
}

[Obsolete("Pre-Flexible Collections logic. Will be replaced by CollectionsAuthorizationHandler.")]
private async Task<bool> CanDeleteCollectionAsync(Guid orgId, Guid collectionId)
{
DeprecatedPermissionsGuard();

if (collectionId == default)
{
return false;
}

if (await DeleteAnyCollection(orgId))
{
return true;
}

if (await _currentContext.DeleteAssignedCollections(orgId))
{
var collectionDetails =
await _collectionRepository.GetByIdAsync(collectionId, _currentContext.UserId.Value);
return collectionDetails != null;
}

return false;
}

[Obsolete("Pre-Flexible Collections logic. Will be replaced by CollectionsAuthorizationHandler.")]
private async Task<bool> DeleteAnyCollection(Guid orgId)
{
DeprecatedPermissionsGuard();

return await _currentContext.OrganizationAdmin(orgId) ||
(_currentContext.Organizations?.Any(o => o.Id == orgId
&& (o.Permissions?.DeleteAnyCollection ?? false)) ?? false);
}

private async Task<bool> CanViewCollectionAsync(Guid orgId, Guid collectionId)
{
if (collectionId == default)
Expand Down
1 change: 1 addition & 0 deletions src/Api/Controllers/OrganizationsController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,7 @@ public async Task<OrganizationSsoResponseModel> PostSso(Guid id, [FromBody] Orga
}

[HttpPut("{id}/collection-management")]
[RequireFeature(FeatureFlagKeys.FlexibleCollections)]
public async Task<OrganizationResponseModel> PutCollectionManagement(Guid id, [FromBody] OrganizationCollectionManagementUpdateRequestModel model)
{
var organization = await _organizationRepository.GetByIdAsync(id);
Expand Down
19 changes: 17 additions & 2 deletions src/Api/Vault/AuthorizationHandlers/Collections/CollectionAuthorizationHandler.cs
View file
Original file line number Diff line number Diff line change
@@ -1,27 +1,42 @@
using Bit.Core.Context;
using Bit.Core;
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Utilities;
using Microsoft.AspNetCore.Authorization;

namespace Bit.Api.Vault.AuthorizationHandlers.Collections;

/// <summary>
/// Handles authorization logic for Collection objects, including access permissions for users and groups.
/// This uses new logic implemented in the Flexible Collections initiative.
/// </summary>
public class CollectionAuthorizationHandler : BulkAuthorizationHandler<CollectionOperationRequirement, Collection>
{
private readonly ICurrentContext _currentContext;
private readonly ICollectionRepository _collectionRepository;
private readonly IFeatureService _featureService;

public CollectionAuthorizationHandler(ICurrentContext currentContext, ICollectionRepository collectionRepository)
public CollectionAuthorizationHandler(ICurrentContext currentContext, ICollectionRepository collectionRepository,
IFeatureService featureService)
{
_currentContext = currentContext;
_collectionRepository = collectionRepository;
_featureService = featureService;
}

protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context,
CollectionOperationRequirement requirement, ICollection<Collection> resources)
{
if (!_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollections, _currentContext))
{
// Flexible collections is OFF, should not be using this handler
throw new FeatureUnavailableException("Flexible collections is OFF when it should be ON.");
}

// Establish pattern of authorization handler null checking passed resources
if (resources == null || !resources.Any())
{
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Constants.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ public static class FeatureFlagKeys
public const string TrustedDeviceEncryption = "trusted-device-encryption";
public const string AutofillV2 = "autofill-v2";
public const string BrowserFilelessImport = "browser-fileless-import";
public const string FlexibleCollections = "flexible-collections";
public const string BulkCollectionAccess = "bulk-collection-access";

public static List<string> GetAllKeys()
{
Expand Down
20 changes: 14 additions & 6 deletions src/Core/Services/Implementations/CollectionService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public class CollectionService : ICollectionService
private readonly IMailService _mailService;
private readonly IReferenceEventService _referenceEventService;
private readonly ICurrentContext _currentContext;
private readonly IFeatureService _featureService;

public CollectionService(
IEventService eventService,
Expand All @@ -28,7 +29,8 @@ public CollectionService(
IUserRepository userRepository,
IMailService mailService,
IReferenceEventService referenceEventService,
ICurrentContext currentContext)
ICurrentContext currentContext,
IFeatureService featureService)
{
_eventService = eventService;
_organizationRepository = organizationRepository;
Expand All @@ -38,6 +40,7 @@ public CollectionService(
_mailService = mailService;
_referenceEventService = referenceEventService;
_currentContext = currentContext;
_featureService = featureService;
}

public async Task SaveAsync(Collection collection, IEnumerable<CollectionAccessSelection> groups = null,
Expand All @@ -51,12 +54,17 @@ public async Task SaveAsync(Collection collection, IEnumerable<CollectionAccessS

var groupsList = groups?.ToList();
var usersList = users?.ToList();
var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false;
var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false;
if (!groupHasManageAccess && !userHasManageAccess)

// If using Flexible Collections - a collection should always have someone with Can Manage permissions
if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollections, _currentContext))
{
throw new BadRequestException(
"At least one member or group must have can manage permission.");
var groupHasManageAccess = groupsList?.Any(g => g.Manage) ?? false;
var userHasManageAccess = usersList?.Any(u => u.Manage) ?? false;
if (!groupHasManageAccess && !userHasManageAccess)
{
throw new BadRequestException(
"At least one member or group must have can manage permission.");
}
}

if (collection.Id == default(Guid))
Expand Down
7 changes: 5 additions & 2 deletions test/Api.Test/Controllers/CollectionsControllerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
using Bit.Api.Controllers;
using Bit.Api.Models.Request;
using Bit.Api.Vault.AuthorizationHandlers.Collections;
using Bit.Core;
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Exceptions;
using Bit.Core.Models.Data;
using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Test.AutoFixture;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
using Microsoft.AspNetCore.Authorization;
Expand All @@ -19,6 +21,7 @@ namespace Bit.Api.Test.Controllers;

[ControllerCustomize(typeof(CollectionsController))]
[SutProviderCustomize]
[FeatureServiceCustomize(FeatureFlagKeys.FlexibleCollections)]
public class CollectionsControllerTests
{
[Theory, BitAutoData]
Expand Down Expand Up @@ -172,7 +175,7 @@ public async Task DeleteMany_Success(Guid orgId, Collection collection1, Collect
.Returns(AuthorizationResult.Success());

// Act
await sutProvider.Sut.DeleteMany(model);
await sutProvider.Sut.DeleteMany(orgId, model);

// Assert
await sutProvider.GetDependency<IDeleteCollectionCommand>()
Expand Down Expand Up @@ -215,7 +218,7 @@ public async Task DeleteMany_PermissionDenied_ThrowsNotFound(Guid orgId, Collect

// Assert
await Assert.ThrowsAsync<NotFoundException>(() =>
sutProvider.Sut.DeleteMany(model));
sutProvider.Sut.DeleteMany(orgId, model));

await sutProvider.GetDependency<IDeleteCollectionCommand>()
.DidNotReceiveWithAnyArgs()
Expand Down
Loading

0 comments on commit 3b049a6

Please sign in to comment.