diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 19858ae18..96de4317f 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -334,12 +334,32 @@ public class CiphersController : Controller return await _currentContext.EditAnyCollection(organizationId); } - if (await CanEditCiphersAsync(organizationId, cipherIds)) + var org = _currentContext.GetOrganization(organizationId); + + // 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 })) { - return 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) + return await CanEditCiphersAsync(organizationId, cipherIds); } /// @@ -360,10 +380,10 @@ public class CiphersController : Controller return true; } - // Provider users can access all ciphers in V1 (to change later) + // Provider users can only access all ciphers if RestrictProviderAccess is disabled if (await _currentContext.ProviderUserForOrgAsync(organizationId)) { - return true; + return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); } return false; @@ -399,10 +419,10 @@ public class CiphersController : Controller return true; } - // Provider users can edit all ciphers in V1 (to change later) + // Provider users can edit all ciphers if RestrictProviderAccess is disabled if (await _currentContext.ProviderUserForOrgAsync(organizationId)) { - return true; + return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); } return false; @@ -422,10 +442,10 @@ public class CiphersController : Controller return true; } - // Provider users can still access organization ciphers in V1 (to change later) + // Provider users can only access organization ciphers if RestrictProviderAccess is disabled if (await _currentContext.ProviderUserForOrgAsync(organizationId)) { - return true; + return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); } return false; @@ -445,10 +465,10 @@ public class CiphersController : Controller return true; } - // Provider users can access all ciphers in V1 (to change later) + // Provider users can only access all ciphers if RestrictProviderAccess is disabled if (await _currentContext.ProviderUserForOrgAsync(organizationId)) { - return true; + return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); } return false; diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index de86170ac..fc3faa9d0 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -139,6 +139,7 @@ public static class FeatureFlagKeys public const string EmailVerification = "email-verification"; public const string AnhFcmv1Migration = "anh-fcmv1-migration"; public const string ExtensionRefresh = "extension-refresh"; + public const string RestrictProviderAccess = "restrict-provider-access"; public static List GetAllKeys() { diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index 675bde5da..14f42db72 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -1,6 +1,5 @@ using System.Security.Claims; using Bit.Api.Vault.Controllers; -using Bit.Api.Vault.Models; using Bit.Api.Vault.Models.Request; using Bit.Core; using Bit.Core.Context; @@ -9,7 +8,6 @@ using Bit.Core.Exceptions; using Bit.Core.Models.Data.Organizations; using Bit.Core.Services; using Bit.Core.Vault.Entities; -using Bit.Core.Vault.Enums; using Bit.Core.Vault.Models.Data; using Bit.Core.Vault.Repositories; using Bit.Core.Vault.Services; @@ -62,9 +60,10 @@ public class CiphersControllerTests [BitAutoData(OrganizationUserType.Custom, false, false)] public async Task CanEditCiphersAsAdminAsync_FlexibleCollections_Success( OrganizationUserType userType, bool allowAdminsAccessToAllItems, bool shouldSucceed, - CurrentContextOrganization organization, Guid userId, SutProvider sutProvider + CurrentContextOrganization organization, Guid userId, Cipher cipher, SutProvider sutProvider ) { + cipher.OrganizationId = organization.Id; organization.Type = userType; if (userType == OrganizationUserType.Custom) { @@ -74,6 +73,9 @@ public class CiphersControllerTests sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); + sutProvider.GetDependency().GetByIdAsync(cipher.Id).Returns(cipher); + sutProvider.GetDependency().GetManyByOrganizationIdAsync(organization.Id).Returns(new List { cipher }); + sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns(new OrganizationAbility { Id = organization.Id, @@ -82,23 +84,17 @@ public class CiphersControllerTests }); sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(true); - var requestModel = new CipherCreateRequestModel - { - Cipher = new CipherRequestModel { OrganizationId = organization.Id.ToString(), Type = CipherType.Login, Login = new CipherLoginModel() }, - CollectionIds = new List() - }; - if (shouldSucceed) { - await sutProvider.Sut.PostAdmin(requestModel); + await sutProvider.Sut.DeleteAdmin(cipher.Id.ToString()); await sutProvider.GetDependency().ReceivedWithAnyArgs() - .SaveAsync(default, default, default); + .DeleteAsync(default, default); } else { - await Assert.ThrowsAsync(() => sutProvider.Sut.PostAdmin(requestModel)); + await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteAdmin(cipher.Id.ToString())); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() - .SaveAsync(default, default, default); + .DeleteAsync(default, default); } } @@ -144,24 +140,19 @@ public class CiphersControllerTests } [Theory] - [BitAutoData(false, true)] - [BitAutoData(true, true)] [BitAutoData(false, false)] [BitAutoData(true, false)] + [BitAutoData(true, true)] public async Task CanEditCiphersAsAdminAsync_Providers( - bool fcV1Enabled, bool shouldSucceed, Cipher cipher, CurrentContextOrganization organization, Guid userId, SutProvider sutProvider + bool fcV1Enabled, bool restrictProviders, Cipher cipher, CurrentContextOrganization organization, Guid userId, SutProvider sutProvider ) { cipher.OrganizationId = organization.Id; - if (fcV1Enabled) - { - sutProvider.GetDependency().ProviderUserForOrgAsync(organization.Id).Returns(shouldSucceed); - } - else - { - sutProvider.GetDependency().EditAnyCollection(organization.Id).Returns(shouldSucceed); - } - sutProvider.GetDependency().GetOrganization(organization.Id).Returns(organization); + + // Simulate that the user is a provider for the organization + sutProvider.GetDependency().EditAnyCollection(organization.Id).Returns(true); + sutProvider.GetDependency().ProviderUserForOrgAsync(organization.Id).Returns(true); + sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); sutProvider.GetDependency().GetByIdAsync(cipher.Id).Returns(cipher); @@ -174,14 +165,16 @@ public class CiphersControllerTests AllowAdminAccessToAllCollectionItems = false }); sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(fcV1Enabled); + sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(restrictProviders); - if (shouldSucceed) + // Non V1 FC or non restricted providers should succeed + if (!fcV1Enabled || !restrictProviders) { await sutProvider.Sut.DeleteAdmin(cipher.Id.ToString()); await sutProvider.GetDependency().ReceivedWithAnyArgs() .DeleteAsync(default, default); } - else + else // Otherwise, they should fail { await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteAdmin(cipher.Id.ToString())); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs()