From c37c6c7d657b7a97845c15128b1e0169cd612146 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Tue, 10 Sep 2024 15:38:23 -0700 Subject: [PATCH] [PM-10996] Remove restrict-provider-access feature flag and usage --- .../Vault/Controllers/CiphersController.cs | 45 +------------------ src/Core/Constants.cs | 1 - .../Controllers/CiphersControllerTests.cs | 32 ++++--------- 3 files changed, 10 insertions(+), 68 deletions(-) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 769ba34a16..796997d178 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -37,11 +37,9 @@ public class CiphersController : Controller private readonly ICipherService _cipherService; private readonly IUserService _userService; private readonly IAttachmentStorageService _attachmentStorageService; - private readonly IProviderService _providerService; private readonly ICurrentContext _currentContext; private readonly ILogger _logger; private readonly GlobalSettings _globalSettings; - private readonly IFeatureService _featureService; private readonly IOrganizationCiphersQuery _organizationCiphersQuery; private readonly IApplicationCacheService _applicationCacheService; private readonly ICollectionRepository _collectionRepository; @@ -52,11 +50,9 @@ public class CiphersController : Controller ICipherService cipherService, IUserService userService, IAttachmentStorageService attachmentStorageService, - IProviderService providerService, ICurrentContext currentContext, ILogger logger, GlobalSettings globalSettings, - IFeatureService featureService, IOrganizationCiphersQuery organizationCiphersQuery, IApplicationCacheService applicationCacheService, ICollectionRepository collectionRepository) @@ -66,11 +62,9 @@ public class CiphersController : Controller _cipherService = cipherService; _userService = userService; _attachmentStorageService = attachmentStorageService; - _providerService = providerService; _currentContext = currentContext; _logger = logger; _globalSettings = globalSettings; - _featureService = featureService; _organizationCiphersQuery = organizationCiphersQuery; _applicationCacheService = applicationCacheService; _collectionRepository = collectionRepository; @@ -282,8 +276,7 @@ public class CiphersController : Controller /// /// 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). - /// Falls back to original EditAnyCollection permission check for when V1 flag is disabled. + /// Allowed for custom users with EditAnyCollection, unrestricted owners and admins (allowAdminAccess setting is ON). /// TODO: Move this to its own authorization handler or equivalent service - AC-2062 /// private async Task CanEditCipherAsAdminAsync(Guid organizationId, IEnumerable cipherIds) @@ -293,23 +286,7 @@ public class CiphersController : Controller // If we're not an "admin", we don't need to check the ciphers if (org is not ({ Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or { Permissions.EditAnyCollection: true })) { - // Are we a provider user? If so, we need to be sure we're not restricted - // Once the feature flag is removed, this check can be combined with the above - if (await _currentContext.ProviderUserForOrgAsync(organizationId)) - { - // Provider is restricted from editing ciphers, so we're not an "admin" - if (_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess)) - { - return false; - } - - // Provider is unrestricted, so we're an "admin", don't return early - } - else - { - // Not a provider or admin - return false; - } + return false; } // We know we're an "admin", now check the ciphers explicitly (in case admins are restricted) @@ -366,12 +343,6 @@ public class CiphersController : Controller return true; } - // Provider users can edit all ciphers if RestrictProviderAccess is disabled - if (await _currentContext.ProviderUserForOrgAsync(organizationId)) - { - return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); - } - return false; } @@ -389,12 +360,6 @@ public class CiphersController : Controller return true; } - // Provider users can only access organization ciphers if RestrictProviderAccess is disabled - if (await _currentContext.ProviderUserForOrgAsync(organizationId)) - { - return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); - } - return false; } @@ -412,12 +377,6 @@ public class CiphersController : Controller return true; } - // Provider users can only access all ciphers if RestrictProviderAccess is disabled - if (await _currentContext.ProviderUserForOrgAsync(organizationId)) - { - return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); - } - return false; } diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index 2664664279..c97aa95d72 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -116,7 +116,6 @@ public static class FeatureFlagKeys public const string EmailVerificationDisableTimingDelays = "email-verification-disable-timing-delays"; public const string AnhFcmv1Migration = "anh-fcmv1-migration"; public const string ExtensionRefresh = "extension-refresh"; - public const string RestrictProviderAccess = "restrict-provider-access"; public const string PM4154BulkEncryptionService = "PM-4154-bulk-encryption-service"; public const string VaultBulkManagementAction = "vault-bulk-management-action"; public const string BulkDeviceApproval = "bulk-device-approval"; diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index e7c5cd9ef5..887d725306 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -186,18 +186,14 @@ public class CiphersControllerTests } [Theory] - [BitAutoData(false)] - [BitAutoData(false)] - [BitAutoData(true)] - public async Task CanEditCiphersAsAdminAsync_Providers( - bool restrictProviders, Cipher cipher, CurrentContextOrganization organization, Guid userId, SutProvider sutProvider + [BitAutoData] + public async Task CannotEditCiphersAsAdminAsync_Providers(Cipher cipher, CurrentContextOrganization organization, Guid userId, SutProvider sutProvider ) { cipher.OrganizationId = organization.Id; - // Simulate that the user is a provider for the organization - sutProvider.GetDependency().EditAnyCollection(organization.Id).Returns(true); - sutProvider.GetDependency().ProviderUserForOrgAsync(organization.Id).Returns(true); + // Simulate that the user is a provider for the organization (i.e. not a member, so there is no organization available) + sutProvider.GetDependency().GetOrganization(organization.Id).Returns((CurrentContextOrganization)null); sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); @@ -209,22 +205,10 @@ public class CiphersControllerTests Id = organization.Id, AllowAdminAccessToAllCollectionItems = false }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(restrictProviders); - // Non restricted providers should succeed - if (!restrictProviders) - { - await sutProvider.Sut.DeleteAdmin(cipher.Id.ToString()); - await sutProvider.GetDependency().ReceivedWithAnyArgs() - .DeleteAsync(default, default); - } - else // Otherwise, they should fail - { - await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteAdmin(cipher.Id.ToString())); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() - .DeleteAsync(default, default); - } - - await sutProvider.GetDependency().Received().ProviderUserForOrgAsync(organization.Id); + // Providers should not be allowed to edit ciphers as Admin + await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteAdmin(cipher.Id.ToString())); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .DeleteAsync(default, default); } }