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

[AC-1713] [Flexible collections] Add feature flags to server #3334

Merged
Merged
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
115 changes: 105 additions & 10 deletions src/Api/Controllers/CollectionsController.cs
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)
r-tome marked this conversation as resolved.
Show resolved Hide resolved
{
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.")]
Copy link
Member Author

@eliykat eliykat Oct 12, 2023

Choose a reason for hiding this comment

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

The rest of the code in this file is largely the existing logic restored from master, although with the DeprecatedPermissionsGuard() to ensure that it's not called accidentally. I also moved DeleteAnyCollection here rather than putting it back in CurrentContext where others might call it.

It's a bit messy to keep it all here - I considered more ambitious refactors, such as making a legacy auth handler. However, this is a low-effort solution, it has fewer changes to existing code (so less regression risk), and we get to leverage the current unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good reasoning, I agree with this approach.

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
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
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
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
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
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
Loading