From 746a35a14a989fea203a8f735ad5bded57e1eff3 Mon Sep 17 00:00:00 2001 From: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com> Date: Thu, 8 Aug 2024 12:26:07 -0500 Subject: [PATCH] [PM-10291] Remove Flexible Collections v1 flag from API (#4578) * chore: remove fc v1 from groups controller, refs PM-10291 * chore: remove fc v1 from organization users controller, refs PM-10291 * chore: remove fc v1 from organizations controller and clean up unsused imports, refs PM-10291 * chore: remove fc v1 from BulkCollectionAuthorizationHandler, refs PM-10291 * chore: remove fc v1 from CiphersCollections, refs PM-10291 * fix: unit tests related to fc v1 flag removal, refs PM-10291 * chore: update AllowAdminAccessToAllCollectionItems to take optional params, increase usage, refs PM-10291 * fix: format files, refs PM-10291 * chore: revert change to helper method, ignore double cache call, refs PM-10291 --- .../Controllers/GroupsController.cs | 30 +---- .../OrganizationUsersController.cs | 34 +---- .../Controllers/OrganizationsController.cs | 8 -- .../BulkCollectionAuthorizationHandler.cs | 18 +-- .../Vault/Controllers/CiphersController.cs | 71 ++--------- .../Controllers/GroupsControllerTests.cs | 27 +--- ...BulkCollectionAuthorizationHandlerTests.cs | 118 ------------------ .../Controllers/CiphersControllerTests.cs | 22 ++-- 8 files changed, 27 insertions(+), 301 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/GroupsController.cs b/src/Api/AdminConsole/Controllers/GroupsController.cs index 1d5d1a931..b314155be 100644 --- a/src/Api/AdminConsole/Controllers/GroupsController.cs +++ b/src/Api/AdminConsole/Controllers/GroupsController.cs @@ -4,7 +4,6 @@ using Bit.Api.Models.Response; using Bit.Api.Utilities; 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; @@ -126,8 +125,8 @@ public class GroupsController : Controller throw new NotFoundException(); } - // Flexible Collections - check the user has permission to grant access to the collections for the new group - if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1) && model.Collections?.Any() == true) + // Check the user has permission to grant access to the collections for the new group + if (model.Collections?.Any() == true) { var collections = await _collectionRepository.GetManyByManyIdsAsync(model.Collections.Select(a => a.Id)); var authorized = @@ -149,31 +148,6 @@ public class GroupsController : Controller [HttpPut("{id}")] [HttpPost("{id}")] public async Task Put(Guid orgId, Guid id, [FromBody] GroupRequestModel model) - { - if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) - { - // Use new Flexible Collections v1 logic - return await Put_vNext(orgId, id, model); - } - - // Pre-Flexible Collections v1 logic follows - var group = await _groupRepository.GetByIdAsync(id); - if (group == null || !await _currentContext.ManageGroups(group.OrganizationId)) - { - throw new NotFoundException(); - } - - var organization = await _organizationRepository.GetByIdAsync(orgId); - - await _updateGroupCommand.UpdateGroupAsync(model.ToGroup(group), organization, - model.Collections.Select(c => c.ToSelectionReadOnly()).ToList(), model.Users); - return new GroupResponseModel(group); - } - - /// - /// Put logic for Flexible Collections v1 - /// - private async Task Put_vNext(Guid orgId, Guid id, [FromBody] GroupRequestModel model) { if (!await _currentContext.ManageGroups(orgId)) { diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index 89e0c6e2f..53f691864 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -5,7 +5,6 @@ using Bit.Api.Models.Response; using Bit.Api.Utilities; using Bit.Api.Vault.AuthorizationHandlers.Collections; using Bit.Api.Vault.AuthorizationHandlers.OrganizationUsers; -using Bit.Core; using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; @@ -229,8 +228,8 @@ public class OrganizationUsersController : Controller throw new NotFoundException(); } - // Flexible Collections - check the user has permission to grant access to the collections for the new user - if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1) && model.Collections?.Any() == true) + // Check the user has permission to grant access to the collections for the new user + if (model.Collections?.Any() == true) { var collections = await _collectionRepository.GetManyByManyIdsAsync(model.Collections.Select(a => a.Id)); var authorized = @@ -366,35 +365,6 @@ public class OrganizationUsersController : Controller [HttpPut("{id}")] [HttpPost("{id}")] public async Task Put(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) - { - if (_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1)) - { - // Use new Flexible Collections v1 logic - await Put_vNext(orgId, id, model); - return; - } - - // Pre-Flexible Collections v1 code follows - if (!await _currentContext.ManageUsers(orgId)) - { - throw new NotFoundException(); - } - - var organizationUser = await _organizationUserRepository.GetByIdAsync(id); - if (organizationUser == null || organizationUser.OrganizationId != orgId) - { - throw new NotFoundException(); - } - - var userId = _userService.GetProperUserId(User); - await _updateOrganizationUserCommand.UpdateUserAsync(model.ToOrganizationUser(organizationUser), userId.Value, - model.Collections.Select(c => c.ToSelectionReadOnly()).ToList(), model.Groups); - } - - /// - /// Put logic for Flexible Collections v1 - /// - private async Task Put_vNext(Guid orgId, Guid id, [FromBody] OrganizationUserUpdateRequestModel model) { if (!await _currentContext.ManageUsers(orgId)) { diff --git a/src/Api/AdminConsole/Controllers/OrganizationsController.cs b/src/Api/AdminConsole/Controllers/OrganizationsController.cs index 0fdc03eed..e3af14e19 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationsController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationsController.cs @@ -539,14 +539,6 @@ public class OrganizationsController : Controller throw new NotFoundException(); } - var v1Enabled = _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1); - - if (!v1Enabled) - { - // V1 is disabled, ensure V1 setting doesn't change - model.AllowAdminAccessToAllCollectionItems = organization.AllowAdminAccessToAllCollectionItems; - } - await _organizationService.UpdateAsync(model.ToOrganization(organization), eventType: EventType.Organization_CollectionManagement_Updated); return new OrganizationResponseModel(organization); } diff --git a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs index 3fe5e7ecf..add5b75a3 100644 --- a/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs +++ b/src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs @@ -1,5 +1,4 @@ #nullable enable -using Bit.Core; using Bit.Core.Context; using Bit.Core.Entities; using Bit.Core.Enums; @@ -191,11 +190,8 @@ public class BulkCollectionAuthorizationHandler : BulkAuthorizationHandler AllowAdminAccessToAllCollectionItems(CurrentContextOrganization? org) { - var organizationAbility = await GetOrganizationAbilityAsync(org); - return !_featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1) || - organizationAbility is { AllowAdminAccessToAllCollectionItems: true }; + return await GetOrganizationAbilityAsync(org) is { AllowAdminAccessToAllCollectionItems: true }; } } diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 30296f0ae..1e608155c 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -239,37 +239,24 @@ public class CiphersController : Controller [HttpGet("organization-details")] public async Task> GetOrganizationCiphers(Guid organizationId) { - // Flexible Collections V1 Logic - if (UseFlexibleCollectionsV1()) + if (!await CanAccessAllCiphersAsync(organizationId)) { - return await GetAllOrganizationCiphersAsync(organizationId); + throw new NotFoundException(); } - // Pre-Flexible Collections V1 Logic - var userId = _userService.GetProperUserId(User).Value; + var allOrganizationCiphers = await _organizationCiphersQuery.GetAllOrganizationCiphers(organizationId); - (IEnumerable orgCiphers, Dictionary> collectionCiphersGroupDict) = await _cipherService.GetOrganizationCiphers(userId, organizationId); + var allOrganizationCipherResponses = + allOrganizationCiphers.Select(c => + new CipherMiniDetailsResponseModel(c, _globalSettings, c.OrganizationUseTotp) + ); - var responses = orgCiphers.Select(c => new CipherMiniDetailsResponseModel(c, _globalSettings, - collectionCiphersGroupDict, c.OrganizationUseTotp)); - - var providerId = await _currentContext.ProviderIdForOrg(organizationId); - if (providerId.HasValue) - { - await _providerService.LogProviderAccessToOrganizationAsync(organizationId); - } - - return new ListResponseModel(responses); + return new ListResponseModel(allOrganizationCipherResponses); } [HttpGet("organization-details/assigned")] public async Task> GetAssignedOrganizationCiphers(Guid organizationId) { - if (!UseFlexibleCollectionsV1()) - { - throw new FeatureUnavailableException(); - } - if (!await CanAccessOrganizationCiphersAsync(organizationId) || !_currentContext.UserId.HasValue) { throw new NotFoundException(); @@ -293,27 +280,6 @@ public class CiphersController : Controller return new ListResponseModel(responses); } - /// - /// Returns all ciphers belonging to the organization if the user has access to All ciphers. - /// - /// - private async Task> GetAllOrganizationCiphersAsync(Guid organizationId) - { - if (!await CanAccessAllCiphersAsync(organizationId)) - { - throw new NotFoundException(); - } - - var allOrganizationCiphers = await _organizationCiphersQuery.GetAllOrganizationCiphers(organizationId); - - var allOrganizationCipherResponses = - allOrganizationCiphers.Select(c => - new CipherMiniDetailsResponseModel(c, _globalSettings, c.OrganizationUseTotp) - ); - - return new ListResponseModel(allOrganizationCipherResponses); - } - /// /// Permission helper to determine if the current user can use the "/admin" variants of the cipher endpoints. /// Allowed for custom users with EditAnyCollection, providers, unrestricted owners and admins (allowAdminAccess setting is ON). @@ -322,12 +288,6 @@ public class CiphersController : Controller /// private async Task CanEditCipherAsAdminAsync(Guid organizationId, IEnumerable cipherIds) { - // Pre-Flexible collections V1 only needs to check EditAnyCollection - if (!UseFlexibleCollectionsV1()) - { - return await _currentContext.EditAnyCollection(organizationId); - } - var org = _currentContext.GetOrganization(organizationId); // If we're not an "admin", we don't need to check the ciphers @@ -390,14 +350,6 @@ public class CiphersController : Controller { var org = _currentContext.GetOrganization(organizationId); - // If not using V1, owners, admins, and users with EditAnyCollection permissions, and providers can always edit all ciphers - if (!UseFlexibleCollectionsV1()) - { - return org is { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or - { Permissions.EditAnyCollection: true } || - await _currentContext.ProviderUserForOrgAsync(organizationId); - } - // Custom users with EditAnyCollection permissions can always edit all ciphers if (org is { Type: OrganizationUserType.Custom, Permissions.EditAnyCollection: true }) { @@ -662,7 +614,7 @@ public class CiphersController : Controller // In V1, we still need to check if the user can edit the collections they're submitting // This should only happen for unassigned ciphers (otherwise restricted admins would use the normal collections endpoint) - if (UseFlexibleCollectionsV1() && !await CanEditItemsInCollections(cipher.OrganizationId.Value, collectionIds)) + if (!await CanEditItemsInCollections(cipher.OrganizationId.Value, collectionIds)) { throw new NotFoundException(); } @@ -1219,9 +1171,4 @@ public class CiphersController : Controller { return await _cipherRepository.GetByIdAsync(cipherId, userId); } - - private bool UseFlexibleCollectionsV1() - { - return _featureService.IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1); - } } diff --git a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs index eb223317f..2885c3318 100644 --- a/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/GroupsControllerTests.cs @@ -2,7 +2,6 @@ using Bit.Api.AdminConsole.Controllers; using Bit.Api.AdminConsole.Models.Request; using Bit.Api.Vault.AuthorizationHandlers.Collections; -using Bit.Core; using Bit.Core.AdminConsole.Entities; using Bit.Core.AdminConsole.OrganizationFeatures.Groups.Interfaces; using Bit.Core.Context; @@ -24,35 +23,14 @@ namespace Bit.Api.Test.AdminConsole.Controllers; [SutProviderCustomize] public class GroupsControllerTests { - [Theory] - [BitAutoData] - public async Task Post_PreFCv1_Success(Organization organization, GroupRequestModel groupRequestModel, SutProvider sutProvider) - { - sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); - sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); - - var response = await sutProvider.Sut.Post(organization.Id, groupRequestModel); - - await sutProvider.GetDependency().Received(1).ManageGroups(organization.Id); - await sutProvider.GetDependency().Received(1).CreateGroupAsync( - Arg.Is(g => - g.OrganizationId == organization.Id && g.Name == groupRequestModel.Name), - organization, - Arg.Any>(), - Arg.Any>()); - Assert.Equal(groupRequestModel.Name, response.Name); - Assert.Equal(organization.Id, response.OrganizationId); - } - [Theory] [BitAutoData] public async Task Post_AuthorizedToGiveAccessToCollections_Success(Organization organization, GroupRequestModel groupRequestModel, SutProvider sutProvider) { - // Enable FC and v1 + // Enable FC sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( new OrganizationAbility { Id = organization.Id, AllowAdminAccessToAllCollectionItems = false }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); sutProvider.GetDependency() .AuthorizeAsync(Arg.Any(), @@ -93,10 +71,9 @@ public class GroupsControllerTests [BitAutoData] public async Task Post_NotAuthorizedToGiveAccessToCollections_Throws(Organization organization, GroupRequestModel groupRequestModel, SutProvider sutProvider) { - // Enable FC and v1 + // Enable FC sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns( new OrganizationAbility { Id = organization.Id, AllowAdminAccessToAllCollectionItems = false }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); sutProvider.GetDependency().GetByIdAsync(organization.Id).Returns(organization); sutProvider.GetDependency().ManageGroups(organization.Id).Returns(true); diff --git a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs index a9f5a16aa..ad3d58b02 100644 --- a/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs +++ b/test/Api.Test/Vault/AuthorizationHandlers/BulkCollectionAuthorizationHandlerTests.cs @@ -534,44 +534,6 @@ public class BulkCollectionAuthorizationHandlerTests Assert.False(context.HasSucceeded); } - [Theory, CollectionCustomization] - [BitAutoData(OrganizationUserType.Admin)] - [BitAutoData(OrganizationUserType.Owner)] - public async Task CanUpdateCollection_WhenAdminOrOwner_WithoutV1Enabled_Success( - OrganizationUserType userType, - Guid userId, SutProvider sutProvider, - ICollection collections, - CurrentContextOrganization organization) - { - organization.Type = userType; - organization.Permissions = new Permissions(); - - var operationsToTest = new[] - { - BulkCollectionOperations.Update, - BulkCollectionOperations.ModifyUserAccess, - BulkCollectionOperations.ModifyGroupAccess, - }; - - foreach (var op in operationsToTest) - { - sutProvider.GetDependency().UserId.Returns(userId); - sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); - - var context = new AuthorizationHandlerContext( - new[] { op }, - new ClaimsPrincipal(), - collections); - - await sutProvider.Sut.HandleAsync(context); - - Assert.True(context.HasSucceeded); - - // Recreate the SUT to reset the mocks/dependencies between tests - sutProvider.Recreate(); - } - } - [Theory, CollectionCustomization] [BitAutoData(OrganizationUserType.Admin)] [BitAutoData(OrganizationUserType.Owner)] @@ -827,32 +789,6 @@ public class BulkCollectionAuthorizationHandlerTests } } - [Theory, BitAutoData, CollectionCustomization] - public async Task CanUpdateUsers_WithManageUsersCustomPermission_V1Disabled_Success( - SutProvider sutProvider, ICollection collections, - CurrentContextOrganization organization, Guid actingUserId) - { - organization.Type = OrganizationUserType.Custom; - organization.Permissions = new Permissions - { - ManageUsers = true - }; - - sutProvider.GetDependency().UserId.Returns(actingUserId); - sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1) - .Returns(false); - - var context = new AuthorizationHandlerContext( - new[] { BulkCollectionOperations.ModifyUserAccess }, - new ClaimsPrincipal(), - collections); - - await sutProvider.Sut.HandleAsync(context); - - Assert.True(context.HasSucceeded); - } - [Theory, BitAutoData, CollectionCustomization] public async Task CanUpdateUsers_WithManageUsersCustomPermission_AllowAdminAccessIsTrue_Success( SutProvider sutProvider, ICollection collections, @@ -909,32 +845,6 @@ public class BulkCollectionAuthorizationHandlerTests Assert.False(context.HasSucceeded); } - [Theory, BitAutoData, CollectionCustomization] - public async Task CanUpdateGroups_WithManageGroupsCustomPermission_V1Disabled_Success( - SutProvider sutProvider, ICollection collections, - CurrentContextOrganization organization, Guid actingUserId) - { - organization.Type = OrganizationUserType.Custom; - organization.Permissions = new Permissions - { - ManageGroups = true - }; - - sutProvider.GetDependency().UserId.Returns(actingUserId); - sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1) - .Returns(false); - - var context = new AuthorizationHandlerContext( - new[] { BulkCollectionOperations.ModifyGroupAccess }, - new ClaimsPrincipal(), - collections); - - await sutProvider.Sut.HandleAsync(context); - - Assert.True(context.HasSucceeded); - } - [Theory, BitAutoData, CollectionCustomization] public async Task CanUpdateGroups_WithManageGroupsCustomPermission_AllowAdminAccessIsTrue_Success( SutProvider sutProvider, ICollection collections, @@ -1047,34 +957,6 @@ public class BulkCollectionAuthorizationHandlerTests Assert.True(context.HasSucceeded); } - [Theory, CollectionCustomization] - [BitAutoData(OrganizationUserType.Admin)] - [BitAutoData(OrganizationUserType.Owner)] - public async Task CanDeleteAsync_WhenAdminOrOwner_V1FlagDisabled_Success( - OrganizationUserType userType, - Guid userId, SutProvider sutProvider, - ICollection collections, - CurrentContextOrganization organization) - { - organization.Type = userType; - organization.Permissions = new Permissions(); - - ArrangeOrganizationAbility(sutProvider, organization, true, false); - - var context = new AuthorizationHandlerContext( - new[] { BulkCollectionOperations.Delete }, - new ClaimsPrincipal(), - collections); - - sutProvider.GetDependency().UserId.Returns(userId); - sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(false); - - await sutProvider.Sut.HandleAsync(context); - - Assert.True(context.HasSucceeded); - } - [Theory, BitAutoData, CollectionCustomization] public async Task CanDeleteAsync_WhenUser_LimitCollectionCreationDeletionFalse_WithCanManagePermission_Success( SutProvider sutProvider, diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index a28c52a68..8c92984ff 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -187,11 +187,11 @@ public class CiphersControllerTests } [Theory] - [BitAutoData(false, false)] - [BitAutoData(true, false)] - [BitAutoData(true, true)] + [BitAutoData(false)] + [BitAutoData(false)] + [BitAutoData(true)] public async Task CanEditCiphersAsAdminAsync_Providers( - bool fcV1Enabled, bool restrictProviders, Cipher cipher, CurrentContextOrganization organization, Guid userId, SutProvider sutProvider + bool restrictProviders, Cipher cipher, CurrentContextOrganization organization, Guid userId, SutProvider sutProvider ) { cipher.OrganizationId = organization.Id; @@ -210,11 +210,10 @@ public class CiphersControllerTests Id = organization.Id, AllowAdminAccessToAllCollectionItems = false }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(fcV1Enabled); sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(restrictProviders); - // Non V1 FC or non restricted providers should succeed - if (!fcV1Enabled || !restrictProviders) + // Non restricted providers should succeed + if (!restrictProviders) { await sutProvider.Sut.DeleteAdmin(cipher.Id.ToString()); await sutProvider.GetDependency().ReceivedWithAnyArgs() @@ -227,13 +226,6 @@ public class CiphersControllerTests .DeleteAsync(default, default); } - if (fcV1Enabled) - { - await sutProvider.GetDependency().Received().ProviderUserForOrgAsync(organization.Id); - } - else - { - await sutProvider.GetDependency().Received().EditAnyCollection(organization.Id); - } + await sutProvider.GetDependency().Received().ProviderUserForOrgAsync(organization.Id); } }