From 97fbf219774c271feb7752b4f42dc1f1574a3aa3 Mon Sep 17 00:00:00 2001 From: Jordan Aasen <166539328+jaasen-livefront@users.noreply.github.com> Date: Thu, 15 May 2025 14:00:48 -0700 Subject: [PATCH] [PM-20543] - remove restrict-provider-access feature flag (#5700) * remove restrict-provider-access feature flag * remove feature flag * re-add flag * remove unnecessary tests * fix bad merge * fix bad merge * remove RestrictProviderAccess key --- .../Vault/Controllers/CiphersController.cs | 56 +---- src/Core/Constants.cs | 1 - .../Controllers/CiphersControllerTests.cs | 209 +----------------- 3 files changed, 18 insertions(+), 248 deletions(-) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 02dace894d..4f105128ea 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -315,26 +315,10 @@ public class CiphersController : Controller { 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 })) + // If we're not an "admin" or if we're not a provider user we don't need to check the ciphers + if (org is not ({ Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or { Permissions.EditAnyCollection: true }) || await _currentContext.ProviderUserForOrgAsync(organizationId)) { - // 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) @@ -350,26 +334,10 @@ public class CiphersController : Controller 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 })) + // If we're not an "admin" or if we're a provider user we don't need to check the ciphers + if (org is not ({ Type: OrganizationUserType.Owner or OrganizationUserType.Admin } or { Permissions.EditAnyCollection: true }) || await _currentContext.ProviderUserForOrgAsync(organizationId)) { - // 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; } // If the user can edit all ciphers for the organization, just check they all belong to the org @@ -462,10 +430,10 @@ public class CiphersController : Controller return true; } - // Provider users can edit all ciphers if RestrictProviderAccess is disabled + // Provider users cannot edit ciphers if (await _currentContext.ProviderUserForOrgAsync(organizationId)) { - return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); + return false; } return false; @@ -485,10 +453,10 @@ public class CiphersController : Controller return true; } - // Provider users can only access organization ciphers if RestrictProviderAccess is disabled + // Provider users cannot access organization ciphers if (await _currentContext.ProviderUserForOrgAsync(organizationId)) { - return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); + return false; } return false; @@ -508,10 +476,10 @@ public class CiphersController : Controller return true; } - // Provider users can only access all ciphers if RestrictProviderAccess is disabled + // Provider users cannot access ciphers if (await _currentContext.ProviderUserForOrgAsync(organizationId)) { - return !_featureService.IsEnabled(FeatureFlagKeys.RestrictProviderAccess); + return false; } return false; diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index abd530b2dd..847214598c 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -193,7 +193,6 @@ public static class FeatureFlagKeys /* Vault Team */ public const string PM8851_BrowserOnboardingNudge = "pm-8851-browser-onboarding-nudge"; public const string PM9111ExtensionPersistAddEditForm = "pm-9111-extension-persist-add-edit-form"; - public const string RestrictProviderAccess = "restrict-provider-access"; public const string SecurityTasks = "security-tasks"; public const string CipherKeyEncryption = "cipher-key-encryption"; public const string DesktopCipherForms = "pm-18520-desktop-cipher-forms"; diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index 0bdc6ab545..e4643f3185 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -193,49 +193,6 @@ public class CiphersControllerTests } } - [Theory] - [BitAutoData(false)] - [BitAutoData(false)] - [BitAutoData(true)] - public async Task CanEditCiphersAsAdminAsync_Providers( - bool restrictProviders, CipherDetails cipherDetails, CurrentContextOrganization organization, Guid userId, SutProvider sutProvider - ) - { - cipherDetails.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); - - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); - - sutProvider.GetDependency().GetByIdAsync(cipherDetails.Id, userId).Returns(cipherDetails); - sutProvider.GetDependency().GetManyByOrganizationIdAsync(organization.Id).Returns(new List { cipherDetails }); - - sutProvider.GetDependency().GetOrganizationAbilityAsync(organization.Id).Returns(new OrganizationAbility - { - Id = organization.Id, - AllowAdminAccessToAllCollectionItems = false - }); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(restrictProviders); - - // Non restricted providers should succeed - if (!restrictProviders) - { - await sutProvider.Sut.DeleteAdmin(cipherDetails.Id); - await sutProvider.GetDependency().ReceivedWithAnyArgs() - .DeleteAsync(default, default); - } - else // Otherwise, they should fail - { - await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteAdmin(cipherDetails.Id)); - await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() - .DeleteAsync(default, default); - } - - await sutProvider.GetDependency().Received().ProviderUserForOrgAsync(organization.Id); - } - [Theory] [BitAutoData(OrganizationUserType.Owner)] [BitAutoData(OrganizationUserType.Admin)] @@ -456,24 +413,7 @@ public class CiphersControllerTests [Theory] [BitAutoData] - public async Task DeleteAdmin_WithProviderUser_DeletesCipher( - CipherDetails cipherDetails, Guid userId, SutProvider sutProvider) - { - cipherDetails.OrganizationId = Guid.NewGuid(); - - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); - sutProvider.GetDependency().ProviderUserForOrgAsync(cipherDetails.OrganizationId.Value).Returns(true); - sutProvider.GetDependency().GetByIdAsync(cipherDetails.Id, userId).Returns(cipherDetails); - sutProvider.GetDependency().GetManyByOrganizationIdAsync(cipherDetails.OrganizationId.Value).Returns(new List { cipherDetails }); - - await sutProvider.Sut.DeleteAdmin(cipherDetails.Id); - - await sutProvider.GetDependency().Received(1).DeleteAsync(cipherDetails, userId, true); - } - - [Theory] - [BitAutoData] - public async Task DeleteAdmin_WithProviderUser_WithRestrictProviderAccessTrue_ThrowsNotFoundException( + public async Task DeleteAdmin_WithProviderUser_ThrowsNotFoundException( Cipher cipher, Guid userId, SutProvider sutProvider) { cipher.OrganizationId = Guid.NewGuid(); @@ -481,7 +421,6 @@ public class CiphersControllerTests sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); sutProvider.GetDependency().ProviderUserForOrgAsync(cipher.OrganizationId.Value).Returns(true); sutProvider.GetDependency().GetByIdAsync(cipher.Id).Returns(cipher); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(true); await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteAdmin(cipher.Id)); } @@ -737,43 +676,13 @@ public class CiphersControllerTests [Theory] [BitAutoData] - public async Task DeleteManyAdmin_WithProviderUser_DeletesCiphers( - CipherBulkDeleteRequestModel model, Guid userId, - List ciphers, SutProvider sutProvider) - { - var organizationId = Guid.NewGuid(); - model.OrganizationId = organizationId.ToString(); - model.Ids = ciphers.Select(c => c.Id.ToString()).ToList(); - - foreach (var cipher in ciphers) - { - cipher.OrganizationId = organizationId; - } - - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); - sutProvider.GetDependency().ProviderUserForOrgAsync(organizationId).Returns(true); - sutProvider.GetDependency().GetManyByOrganizationIdAsync(organizationId).Returns(ciphers); - - await sutProvider.Sut.DeleteManyAdmin(model); - - await sutProvider.GetDependency() - .Received(1) - .DeleteManyAsync( - Arg.Is>(ids => - ids.All(id => model.Ids.Contains(id.ToString())) && ids.Count() == model.Ids.Count()), - userId, organizationId, true); - } - - [Theory] - [BitAutoData] - public async Task DeleteManyAdmin_WithProviderUser_WithRestrictProviderAccessTrue_ThrowsNotFoundException( + public async Task DeleteManyAdmin_WithProviderUser_ThrowsNotFoundException( CipherBulkDeleteRequestModel model, SutProvider sutProvider) { var organizationId = Guid.NewGuid(); model.OrganizationId = organizationId.ToString(); sutProvider.GetDependency().ProviderUserForOrgAsync(organizationId).Returns(true); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(true); await Assert.ThrowsAsync(() => sutProvider.Sut.DeleteManyAdmin(model)); } @@ -1000,24 +909,7 @@ public class CiphersControllerTests [Theory] [BitAutoData] - public async Task PutDeleteAdmin_WithProviderUser_SoftDeletesCipher( - CipherDetails cipherDetails, Guid userId, SutProvider sutProvider) - { - cipherDetails.OrganizationId = Guid.NewGuid(); - - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); - sutProvider.GetDependency().ProviderUserForOrgAsync(cipherDetails.OrganizationId.Value).Returns(true); - sutProvider.GetDependency().GetByIdAsync(cipherDetails.Id, userId).Returns(cipherDetails); - sutProvider.GetDependency().GetManyByOrganizationIdAsync(cipherDetails.OrganizationId.Value).Returns(new List { cipherDetails }); - - await sutProvider.Sut.PutDeleteAdmin(cipherDetails.Id); - - await sutProvider.GetDependency().Received(1).SoftDeleteAsync(cipherDetails, userId, true); - } - - [Theory] - [BitAutoData] - public async Task PutDeleteAdmin_WithProviderUser_WithRestrictProviderAccessTrue_ThrowsNotFoundException( + public async Task PutDeleteAdmin_WithProviderUser_ThrowsNotFoundException( Cipher cipher, Guid userId, SutProvider sutProvider) { cipher.OrganizationId = Guid.NewGuid(); @@ -1025,7 +917,6 @@ public class CiphersControllerTests sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); sutProvider.GetDependency().ProviderUserForOrgAsync(cipher.OrganizationId.Value).Returns(true); sutProvider.GetDependency().GetByIdAsync(cipher.Id).Returns(cipher); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(true); await Assert.ThrowsAsync(() => sutProvider.Sut.PutDeleteAdmin(cipher.Id)); } @@ -1272,43 +1163,13 @@ public class CiphersControllerTests [Theory] [BitAutoData] - public async Task PutDeleteManyAdmin_WithProviderUser_SoftDeletesCiphers( - CipherBulkDeleteRequestModel model, Guid userId, - List ciphers, SutProvider sutProvider) - { - var organizationId = Guid.NewGuid(); - model.OrganizationId = organizationId.ToString(); - model.Ids = ciphers.Select(c => c.Id.ToString()).ToList(); - - foreach (var cipher in ciphers) - { - cipher.OrganizationId = organizationId; - } - - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); - sutProvider.GetDependency().ProviderUserForOrgAsync(organizationId).Returns(true); - sutProvider.GetDependency().GetManyByOrganizationIdAsync(organizationId).Returns(ciphers); - - await sutProvider.Sut.PutDeleteManyAdmin(model); - - await sutProvider.GetDependency() - .Received(1) - .SoftDeleteManyAsync( - Arg.Is>(ids => - ids.All(id => model.Ids.Contains(id.ToString())) && ids.Count() == model.Ids.Count()), - userId, organizationId, true); - } - - [Theory] - [BitAutoData] - public async Task PutDeleteManyAdmin_WithProviderUser_WithRestrictProviderAccessTrue_ThrowsNotFoundException( + public async Task PutDeleteManyAdmin_WithProviderUser_ThrowsNotFoundException( CipherBulkDeleteRequestModel model, SutProvider sutProvider) { var organizationId = Guid.NewGuid(); model.OrganizationId = organizationId.ToString(); sutProvider.GetDependency().ProviderUserForOrgAsync(organizationId).Returns(true); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(true); await Assert.ThrowsAsync(() => sutProvider.Sut.PutDeleteManyAdmin(model)); } @@ -1546,27 +1407,7 @@ public class CiphersControllerTests [Theory] [BitAutoData] - public async Task PutRestoreAdmin_WithProviderUser_RestoresCipher( - CipherDetails cipherDetails, Guid userId, SutProvider sutProvider) - { - cipherDetails.OrganizationId = Guid.NewGuid(); - cipherDetails.Type = CipherType.Login; - cipherDetails.Data = JsonSerializer.Serialize(new CipherLoginData()); - - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); - sutProvider.GetDependency().ProviderUserForOrgAsync(cipherDetails.OrganizationId.Value).Returns(true); - sutProvider.GetDependency().GetByIdAsync(cipherDetails.Id, userId).Returns(cipherDetails); - sutProvider.GetDependency().GetManyByOrganizationIdAsync(cipherDetails.OrganizationId.Value).Returns(new List { cipherDetails }); - - var result = await sutProvider.Sut.PutRestoreAdmin(cipherDetails.Id); - - Assert.IsType(result); - await sutProvider.GetDependency().Received(1).RestoreAsync(cipherDetails, userId, true); - } - - [Theory] - [BitAutoData] - public async Task PutRestoreAdmin_WithProviderUser_WithRestrictProviderAccessTrue_ThrowsNotFoundException( + public async Task PutRestoreAdmin_WithProviderUser_ThrowsNotFoundException( CipherDetails cipherDetails, Guid userId, SutProvider sutProvider) { cipherDetails.OrganizationId = Guid.NewGuid(); @@ -1574,7 +1415,6 @@ public class CiphersControllerTests sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); sutProvider.GetDependency().ProviderUserForOrgAsync(cipherDetails.OrganizationId.Value).Returns(true); sutProvider.GetDependency().GetOrganizationDetailsByIdAsync(cipherDetails.Id).Returns(cipherDetails); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(true); await Assert.ThrowsAsync(() => sutProvider.Sut.PutRestoreAdmin(cipherDetails.Id)); } @@ -1896,49 +1736,12 @@ public class CiphersControllerTests [Theory] [BitAutoData] - public async Task PutRestoreManyAdmin_WithProviderUser_RestoresCiphers( - CipherBulkRestoreRequestModel model, Guid userId, - List ciphers, SutProvider sutProvider) - { - model.OrganizationId = Guid.NewGuid(); - model.Ids = ciphers.Select(c => c.Id.ToString()).ToList(); - - sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs(userId); - sutProvider.GetDependency().ProviderUserForOrgAsync(model.OrganizationId).Returns(true); - sutProvider.GetDependency().GetManyByOrganizationIdAsync(model.OrganizationId).Returns(ciphers); - - var cipherOrgDetails = ciphers.Select(c => new CipherOrganizationDetails - { - Id = c.Id, - OrganizationId = model.OrganizationId - }).ToList(); - - sutProvider.GetDependency() - .RestoreManyAsync( - Arg.Any>(), - userId, model.OrganizationId, true) - .Returns(cipherOrgDetails); - - var result = await sutProvider.Sut.PutRestoreManyAdmin(model); - - Assert.NotNull(result); - await sutProvider.GetDependency() - .Received(1) - .RestoreManyAsync( - Arg.Is>(ids => - ids.All(id => model.Ids.Contains(id.ToString())) && ids.Count == model.Ids.Count()), - userId, model.OrganizationId, true); - } - - [Theory] - [BitAutoData] - public async Task PutRestoreManyAdmin_WithProviderUser_WithRestrictProviderAccessTrue_ThrowsNotFoundException( + public async Task PutRestoreManyAdmin_WithProviderUser_ThrowsNotFoundException( CipherBulkRestoreRequestModel model, SutProvider sutProvider) { model.OrganizationId = Guid.NewGuid(); sutProvider.GetDependency().ProviderUserForOrgAsync(model.OrganizationId).Returns(true); - sutProvider.GetDependency().IsEnabled(FeatureFlagKeys.RestrictProviderAccess).Returns(true); await Assert.ThrowsAsync(() => sutProvider.Sut.PutRestoreManyAdmin(model)); }