Upstream/bitwarden-server
1
0
Fork 0
mirror of https://github.com/bitwarden/server.git synced 2024-11-21 12:05:42 +01:00
Code Issues Projects Releases Wiki Activity

[AC-1707] Restrict provider access to items (#3881)

Browse Source 
* [AC-2274] Introduce CanEditAnyCiphersAsAdminAsync helper to replace EditAnyCollection usage

* [AC-2274] Add unit tests for CanEditAnyCiphersAsAdmin helper

* [AC-2274] Add Jira ticket

* [AC-1707] Add feature flag

* [AC-1707] Update CanEditAnyCiphersAsAdmin to fail for providers when the feature flag is enabled

* [AC-2274] Undo change to purge endpoint

* [AC-2274] Update admin checks to account for unassigned ciphers

* [AC-1707] Fix provider auth checks after merge with main

* [AC-1707] Fix tests after merge

* [AC-1707] Adjust CanEditCipherAsAdmin method to properly account for admin user types

- Fix associated unit tests

* [AC-1707] Formatting
This commit is contained in:
Shane Melton 2024-05-07 12:30:48 -07:00 committed by GitHub
parent 1ede40d5e1
commit 45be4d5069
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 52 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
src/Api/Vault/Controllers/CiphersController.cs
View File

@ -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);
}
/// <summary>
@ -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;

1
src/Core/Constants.cs
View File

@ -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<string> GetAllKeys()
{

47
test/Api.Test/Vault/Controllers/CiphersControllerTests.cs
View File

@ -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<CiphersController> sutProvider
CurrentContextOrganization organization, Guid userId, Cipher cipher, SutProvider<CiphersController> sutProvider
)
{
cipher.OrganizationId = organization.Id;
organization.Type = userType;
if (userType == OrganizationUserType.Custom)
{
@ -74,6 +73,9 @@ public class CiphersControllerTests
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
sutProvider.GetDependency<IUserService>().GetProperUserId(default).ReturnsForAnyArgs(userId);
sutProvider.GetDependency<ICipherRepository>().GetByIdAsync(cipher.Id).Returns(cipher);
sutProvider.GetDependency<ICipherRepository>().GetManyByOrganizationIdAsync(organization.Id).Returns(new List<Cipher> { cipher });
sutProvider.GetDependency<IApplicationCacheService>().GetOrganizationAbilityAsync(organization.Id).Returns(new OrganizationAbility
{
Id = organization.Id,
@ -82,23 +84,17 @@ public class CiphersControllerTests
});
sutProvider.GetDependency<IFeatureService>().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<Guid>()
};
if (shouldSucceed)
{
await sutProvider.Sut.PostAdmin(requestModel);
await sutProvider.Sut.DeleteAdmin(cipher.Id.ToString());
await sutProvider.GetDependency<ICipherService>().ReceivedWithAnyArgs()
.SaveAsync(default, default, default);
.DeleteAsync(default, default);
}
else
{
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.PostAdmin(requestModel));
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.DeleteAdmin(cipher.Id.ToString()));
await sutProvider.GetDependency<ICipherService>().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<CiphersController> sutProvider
bool fcV1Enabled, bool restrictProviders, Cipher cipher, CurrentContextOrganization organization, Guid userId, SutProvider<CiphersController> sutProvider
)
{
cipher.OrganizationId = organization.Id;
if (fcV1Enabled)
{
sutProvider.GetDependency<ICurrentContext>().ProviderUserForOrgAsync(organization.Id).Returns(shouldSucceed);
}
else
{
sutProvider.GetDependency<ICurrentContext>().EditAnyCollection(organization.Id).Returns(shouldSucceed);
}
sutProvider.GetDependency<ICurrentContext>().GetOrganization(organization.Id).Returns(organization);
// Simulate that the user is a provider for the organization
sutProvider.GetDependency<ICurrentContext>().EditAnyCollection(organization.Id).Returns(true);
sutProvider.GetDependency<ICurrentContext>().ProviderUserForOrgAsync(organization.Id).Returns(true);
sutProvider.GetDependency<IUserService>().GetProperUserId(default).ReturnsForAnyArgs(userId);
sutProvider.GetDependency<ICipherRepository>().GetByIdAsync(cipher.Id).Returns(cipher);
@ -174,14 +165,16 @@ public class CiphersControllerTests
AllowAdminAccessToAllCollectionItems = false
});
sutProvider.GetDependency<IFeatureService>().IsEnabled(FeatureFlagKeys.FlexibleCollectionsV1).Returns(fcV1Enabled);
sutProvider.GetDependency<IFeatureService>().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<ICipherService>().ReceivedWithAnyArgs()
.DeleteAsync(default, default);
}
else
else // Otherwise, they should fail
{
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.DeleteAdmin(cipher.Id.ToString()));
await sutProvider.GetDependency<ICipherService>().DidNotReceiveWithAnyArgs()