mirror of
https://github.com/bitwarden/server.git
synced 2026-04-20 11:42:29 -05:00
[PM-33819] Enforce use of authorize attributes (#7242)
Add tests to ensure we are using authorize attributes Also clean up non-compliant and deprecated methods on PoliciesController.
This commit is contained in:
@@ -14,10 +14,8 @@ using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyUpdateEvents.Int
|
||||
using Bit.Core.AdminConsole.Repositories;
|
||||
using Bit.Core.Auth.Models.Business.Tokenables;
|
||||
using Bit.Core.Context;
|
||||
using Bit.Core.Enums;
|
||||
using Bit.Core.Exceptions;
|
||||
using Bit.Core.Repositories;
|
||||
using Bit.Core.Services;
|
||||
using Bit.Core.Tokens;
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
@@ -34,42 +32,32 @@ public class PoliciesController : Controller
|
||||
private readonly IOrganizationUserRepository _organizationUserRepository;
|
||||
private readonly IDataProtectorTokenFactory<OrgUserInviteTokenable> _orgUserInviteTokenDataFactory;
|
||||
private readonly IPolicyRepository _policyRepository;
|
||||
private readonly IUserService _userService;
|
||||
private readonly ISavePolicyCommand _savePolicyCommand;
|
||||
private readonly IVNextSavePolicyCommand _vNextSavePolicyCommand;
|
||||
private readonly IPolicyQuery _policyQuery;
|
||||
|
||||
public PoliciesController(IPolicyRepository policyRepository,
|
||||
IOrganizationUserRepository organizationUserRepository,
|
||||
IUserService userService,
|
||||
ICurrentContext currentContext,
|
||||
IDataProtectorTokenFactory<OrgUserInviteTokenable> orgUserInviteTokenDataFactory,
|
||||
IOrganizationHasVerifiedDomainsQuery organizationHasVerifiedDomainsQuery,
|
||||
IOrganizationRepository organizationRepository,
|
||||
ISavePolicyCommand savePolicyCommand,
|
||||
IVNextSavePolicyCommand vNextSavePolicyCommand,
|
||||
IPolicyQuery policyQuery)
|
||||
{
|
||||
_policyRepository = policyRepository;
|
||||
_organizationUserRepository = organizationUserRepository;
|
||||
_userService = userService;
|
||||
_currentContext = currentContext;
|
||||
_organizationRepository = organizationRepository;
|
||||
_orgUserInviteTokenDataFactory = orgUserInviteTokenDataFactory;
|
||||
_organizationHasVerifiedDomainsQuery = organizationHasVerifiedDomainsQuery;
|
||||
_savePolicyCommand = savePolicyCommand;
|
||||
_vNextSavePolicyCommand = vNextSavePolicyCommand;
|
||||
_policyQuery = policyQuery;
|
||||
}
|
||||
|
||||
[HttpGet("{type}")]
|
||||
[Authorize<ManagePoliciesRequirement>]
|
||||
public async Task<PolicyStatusResponseModel> Get(Guid orgId, PolicyType type)
|
||||
{
|
||||
if (!await _currentContext.ManagePolicies(orgId))
|
||||
{
|
||||
throw new NotFoundException();
|
||||
}
|
||||
|
||||
var policy = await _policyQuery.RunAsync(orgId, type);
|
||||
if (policy.Type is PolicyType.SingleOrg)
|
||||
{
|
||||
@@ -80,15 +68,10 @@ public class PoliciesController : Controller
|
||||
}
|
||||
|
||||
[HttpGet("")]
|
||||
public async Task<ListResponseModel<PolicyResponseModel>> GetAll(string orgId)
|
||||
[Authorize<ManagePoliciesRequirement>]
|
||||
public async Task<ListResponseModel<PolicyResponseModel>> GetAll(Guid orgId)
|
||||
{
|
||||
var orgIdGuid = new Guid(orgId);
|
||||
if (!await _currentContext.ManagePolicies(orgIdGuid))
|
||||
{
|
||||
throw new NotFoundException();
|
||||
}
|
||||
|
||||
var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgIdGuid);
|
||||
var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgId);
|
||||
|
||||
return new ListResponseModel<PolicyResponseModel>(policies.Select(p => new PolicyResponseModel(p)));
|
||||
}
|
||||
@@ -124,34 +107,8 @@ public class PoliciesController : Controller
|
||||
return new ListResponseModel<PolicyResponseModel>(responses);
|
||||
}
|
||||
|
||||
// TODO: PM-4097 - remove GetByInvitedUser once all clients are updated to use the GetMasterPasswordPolicy endpoint below
|
||||
[Obsolete("Deprecated API", false)]
|
||||
[AllowAnonymous]
|
||||
[HttpGet("invited-user")]
|
||||
public async Task<ListResponseModel<PolicyResponseModel>> GetByInvitedUser(Guid orgId, [FromQuery] Guid userId)
|
||||
{
|
||||
var user = await _userService.GetUserByIdAsync(userId);
|
||||
if (user == null)
|
||||
{
|
||||
throw new UnauthorizedAccessException();
|
||||
}
|
||||
var orgUsersByUserId = await _organizationUserRepository.GetManyByUserAsync(user.Id);
|
||||
var orgUser = orgUsersByUserId.SingleOrDefault(u => u.OrganizationId == orgId);
|
||||
if (orgUser == null)
|
||||
{
|
||||
throw new NotFoundException();
|
||||
}
|
||||
if (orgUser.Status != OrganizationUserStatusType.Invited)
|
||||
{
|
||||
throw new UnauthorizedAccessException();
|
||||
}
|
||||
|
||||
var policies = await _policyRepository.GetManyByOrganizationIdAsync(orgId);
|
||||
var responses = policies.Where(p => p.Enabled).Select(p => new PolicyResponseModel(p));
|
||||
return new ListResponseModel<PolicyResponseModel>(responses);
|
||||
}
|
||||
|
||||
[HttpGet("master-password")]
|
||||
[Authorize<MemberRequirement>]
|
||||
public async Task<PolicyResponseModel> GetMasterPasswordPolicy(Guid orgId)
|
||||
{
|
||||
var organization = await _organizationRepository.GetByIdAsync(orgId);
|
||||
@@ -161,15 +118,6 @@ public class PoliciesController : Controller
|
||||
throw new NotFoundException();
|
||||
}
|
||||
|
||||
var userId = _userService.GetProperUserId(User).Value;
|
||||
|
||||
var orgUser = await _organizationUserRepository.GetByOrganizationAsync(orgId, userId);
|
||||
|
||||
if (orgUser == null)
|
||||
{
|
||||
throw new NotFoundException();
|
||||
}
|
||||
|
||||
var policy = await _policyRepository.GetByOrganizationIdTypeAsync(orgId, PolicyType.MasterPassword);
|
||||
|
||||
if (policy == null || !policy.Enabled)
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
using Bit.Api.AdminConsole.Controllers;
|
||||
using Bit.Api.Test.Utilities;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Xunit;
|
||||
|
||||
namespace Bit.Api.Test.AdminConsole.Controllers;
|
||||
|
||||
public class AdminConsoleControllersAuthorizationTests
|
||||
{
|
||||
/// <summary>
|
||||
/// Controllers that have not yet been migrated to use method-level authorization attributes.
|
||||
/// TODO: Remove controllers from this list as they are migrated to use [Authorize] or [AllowAnonymous] on all methods.
|
||||
/// </summary>
|
||||
private static readonly HashSet<Type> _controllersNotYetMigrated =
|
||||
[
|
||||
typeof(GroupsController),
|
||||
typeof(OrganizationAuthRequestsController),
|
||||
typeof(OrganizationConnectionsController),
|
||||
typeof(OrganizationDomainController),
|
||||
typeof(OrganizationsController),
|
||||
typeof(OrganizationUsersController),
|
||||
typeof(ProviderClientsController),
|
||||
typeof(ProviderOrganizationsController),
|
||||
typeof(ProvidersController),
|
||||
typeof(ProviderUsersController)
|
||||
];
|
||||
|
||||
public static IEnumerable<object[]> GetAllAdminConsoleControllers()
|
||||
{
|
||||
// This is just a convenient way to get the assembly reference - it does
|
||||
// not actually require that all controllers extend this base class
|
||||
var assembly = typeof(BaseAdminConsoleController).Assembly;
|
||||
return assembly.GetTypes()
|
||||
.Where(t => t.IsClass
|
||||
&& !t.IsAbstract
|
||||
&& typeof(ControllerBase).IsAssignableFrom(t)
|
||||
&& t.Namespace == "Bit.Api.AdminConsole.Controllers")
|
||||
.Except(_controllersNotYetMigrated)
|
||||
.Select(t => new object[] { t });
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Automatically finds all controllers in the Bit.Api.AdminConsole.Controllers namespace
|
||||
/// and ensures that they have [Authorize] or [AllowAnonymous] attributes on all methods.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// See <see cref="_controllersNotYetMigrated"/> for an exemption list of existing controllers
|
||||
/// that aren't using these attributes yet (but should be).
|
||||
/// See <see cref="ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization"/>
|
||||
/// for more information about what this test requires to pass.
|
||||
/// </remarks>
|
||||
[Theory]
|
||||
[MemberData(nameof(GetAllAdminConsoleControllers))]
|
||||
public void AllControllers_HaveAuthorizationOnAllMethods(Type controllerType)
|
||||
{
|
||||
ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(controllerType);
|
||||
}
|
||||
}
|
||||
@@ -1,8 +1,8 @@
|
||||
using System.Security.Claims;
|
||||
using System.Text.Json;
|
||||
using System.Text.Json;
|
||||
using Bit.Api.AdminConsole.Controllers;
|
||||
using Bit.Api.AdminConsole.Models.Request;
|
||||
using Bit.Api.AdminConsole.Models.Response.Organizations;
|
||||
using Bit.Api.Test.Utilities;
|
||||
using Bit.Core.AdminConsole.Entities;
|
||||
using Bit.Core.AdminConsole.Enums;
|
||||
using Bit.Core.AdminConsole.Models.Data.Organizations.Policies;
|
||||
@@ -15,14 +15,13 @@ using Bit.Core.Context;
|
||||
using Bit.Core.Entities;
|
||||
using Bit.Core.Exceptions;
|
||||
using Bit.Core.Repositories;
|
||||
using Bit.Core.Services;
|
||||
using Bit.Core.Tokens;
|
||||
using Bit.Test.Common.AutoFixture;
|
||||
using Bit.Test.Common.AutoFixture.Attributes;
|
||||
using NSubstitute;
|
||||
using Xunit;
|
||||
|
||||
namespace Bit.Api.Test.Controllers;
|
||||
namespace Bit.Api.Test.AdminConsole.Controllers;
|
||||
|
||||
|
||||
// Note: test names follow MethodName_StateUnderTest_ExpectedBehavior pattern.
|
||||
@@ -30,13 +29,18 @@ namespace Bit.Api.Test.Controllers;
|
||||
[SutProviderCustomize]
|
||||
public class PoliciesControllerTests
|
||||
{
|
||||
[Fact]
|
||||
public void AllActionMethodsHaveAuthorization()
|
||||
{
|
||||
ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(
|
||||
typeof(PoliciesController));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[BitAutoData]
|
||||
public async Task GetMasterPasswordPolicy_WhenCalled_ReturnsMasterPasswordPolicy(
|
||||
SutProvider<PoliciesController> sutProvider,
|
||||
Guid orgId, Guid userId,
|
||||
OrganizationUser orgUser,
|
||||
Guid orgId,
|
||||
Policy policy,
|
||||
MasterPasswordPolicyData mpPolicyData,
|
||||
Organization organization)
|
||||
@@ -47,15 +51,6 @@ public class PoliciesControllerTests
|
||||
var organizationRepository = sutProvider.GetDependency<IOrganizationRepository>();
|
||||
organizationRepository.GetByIdAsync(orgId).Returns(organization);
|
||||
|
||||
sutProvider.GetDependency<IUserService>()
|
||||
.GetProperUserId(Arg.Any<ClaimsPrincipal>())
|
||||
.Returns(userId);
|
||||
|
||||
sutProvider.GetDependency<IOrganizationUserRepository>()
|
||||
.GetByOrganizationAsync(orgId, userId)
|
||||
.Returns(orgUser);
|
||||
|
||||
|
||||
policy.Type = PolicyType.MasterPassword;
|
||||
policy.Enabled = true;
|
||||
// data should be a JSON serialized version of the mpPolicyData object
|
||||
@@ -90,35 +85,17 @@ public class PoliciesControllerTests
|
||||
[Theory]
|
||||
[BitAutoData]
|
||||
public async Task GetMasterPasswordPolicy_OrgUserIsNull_ThrowsNotFoundException(
|
||||
SutProvider<PoliciesController> sutProvider, Guid orgId, Guid userId)
|
||||
SutProvider<PoliciesController> sutProvider, Guid orgId)
|
||||
{
|
||||
// Arrange
|
||||
sutProvider.GetDependency<IUserService>()
|
||||
.GetProperUserId(Arg.Any<ClaimsPrincipal>())
|
||||
.Returns(userId);
|
||||
|
||||
sutProvider.GetDependency<IOrganizationUserRepository>()
|
||||
.GetByOrganizationAsync(orgId, userId)
|
||||
.Returns((OrganizationUser)null);
|
||||
|
||||
// Act & Assert
|
||||
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.GetMasterPasswordPolicy(orgId));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[BitAutoData]
|
||||
public async Task GetMasterPasswordPolicy_PolicyIsNull_ThrowsNotFoundException(
|
||||
SutProvider<PoliciesController> sutProvider, Guid orgId, Guid userId, OrganizationUser orgUser)
|
||||
SutProvider<PoliciesController> sutProvider, Guid orgId)
|
||||
{
|
||||
// Arrange
|
||||
sutProvider.GetDependency<IUserService>()
|
||||
.GetProperUserId(Arg.Any<ClaimsPrincipal>())
|
||||
.Returns(userId);
|
||||
|
||||
sutProvider.GetDependency<IOrganizationUserRepository>()
|
||||
.GetByOrganizationAsync(orgId, userId)
|
||||
.Returns(orgUser);
|
||||
|
||||
sutProvider.GetDependency<IPolicyRepository>()
|
||||
.GetByOrganizationIdTypeAsync(orgId, PolicyType.MasterPassword)
|
||||
.Returns((Policy)null);
|
||||
@@ -130,17 +107,9 @@ public class PoliciesControllerTests
|
||||
[Theory]
|
||||
[BitAutoData]
|
||||
public async Task GetMasterPasswordPolicy_PolicyNotEnabled_ThrowsNotFoundException(
|
||||
SutProvider<PoliciesController> sutProvider, Guid orgId, Guid userId, OrganizationUser orgUser, Policy policy)
|
||||
SutProvider<PoliciesController> sutProvider, Guid orgId, Policy policy)
|
||||
{
|
||||
// Arrange
|
||||
sutProvider.GetDependency<IUserService>()
|
||||
.GetProperUserId(Arg.Any<ClaimsPrincipal>())
|
||||
.Returns(userId);
|
||||
|
||||
sutProvider.GetDependency<IOrganizationUserRepository>()
|
||||
.GetByOrganizationAsync(orgId, userId)
|
||||
.Returns(orgUser);
|
||||
|
||||
policy.Enabled = false; // Ensuring the policy is not enabled
|
||||
sutProvider.GetDependency<IPolicyRepository>()
|
||||
.GetByOrganizationIdTypeAsync(orgId, PolicyType.MasterPassword)
|
||||
@@ -160,7 +129,6 @@ public class PoliciesControllerTests
|
||||
var organizationRepository = sutProvider.GetDependency<IOrganizationRepository>();
|
||||
organizationRepository.GetByIdAsync(orgId).Returns((Organization)null);
|
||||
|
||||
|
||||
// Act & Assert
|
||||
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.GetMasterPasswordPolicy(orgId));
|
||||
}
|
||||
@@ -178,7 +146,6 @@ public class PoliciesControllerTests
|
||||
var organizationRepository = sutProvider.GetDependency<IOrganizationRepository>();
|
||||
organizationRepository.GetByIdAsync(orgId).Returns(organization);
|
||||
|
||||
|
||||
// Act & Assert
|
||||
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.GetMasterPasswordPolicy(orgId));
|
||||
}
|
||||
@@ -211,20 +178,6 @@ public class PoliciesControllerTests
|
||||
Assert.Equal(policy.OrganizationId, result.OrganizationId);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[BitAutoData]
|
||||
public async Task Get_WhenUserCannotManagePolicies_ThrowsNotFoundException(
|
||||
SutProvider<PoliciesController> sutProvider, Guid orgId, PolicyType type)
|
||||
{
|
||||
// Arrange
|
||||
sutProvider.GetDependency<ICurrentContext>()
|
||||
.ManagePolicies(orgId)
|
||||
.Returns(false);
|
||||
|
||||
// Act & Assert
|
||||
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.Get(orgId, type));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[BitAutoData]
|
||||
public async Task GetByToken_WhenOrganizationUseUsePoliciesIsFalse_ThrowsNotFoundException(
|
||||
@@ -473,10 +426,6 @@ public class PoliciesControllerTests
|
||||
m.PerformedBy.UserId == userId &&
|
||||
m.PerformedBy.IsOrganizationOwnerOrProvider == true));
|
||||
|
||||
await sutProvider.GetDependency<ISavePolicyCommand>()
|
||||
.DidNotReceiveWithAnyArgs()
|
||||
.VNextSaveAsync(default);
|
||||
|
||||
Assert.NotNull(result);
|
||||
Assert.Equal(policy.Id, result.Id);
|
||||
Assert.Equal(policy.Type, result.Type);
|
||||
@@ -515,10 +464,6 @@ public class PoliciesControllerTests
|
||||
m.PerformedBy.UserId == userId &&
|
||||
m.PerformedBy.IsOrganizationOwnerOrProvider == true));
|
||||
|
||||
await sutProvider.GetDependency<ISavePolicyCommand>()
|
||||
.DidNotReceiveWithAnyArgs()
|
||||
.VNextSaveAsync(default);
|
||||
|
||||
Assert.NotNull(result);
|
||||
Assert.Equal(policy.Id, result.Id);
|
||||
Assert.Equal(policy.Type, result.Type);
|
||||
159
test/Api.Test/Utilities/ControllerAuthorizationTestHelpers.cs
Normal file
159
test/Api.Test/Utilities/ControllerAuthorizationTestHelpers.cs
Normal file
@@ -0,0 +1,159 @@
|
||||
using System.Reflection;
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Xunit;
|
||||
|
||||
namespace Bit.Api.Test.Utilities;
|
||||
|
||||
public static class ControllerAuthorizationTestHelpers
|
||||
{
|
||||
private static readonly Type[] _httpMethodAttributes =
|
||||
[
|
||||
typeof(HttpGetAttribute),
|
||||
typeof(HttpPostAttribute),
|
||||
typeof(HttpPutAttribute),
|
||||
typeof(HttpDeleteAttribute),
|
||||
typeof(HttpPatchAttribute),
|
||||
typeof(HttpHeadAttribute),
|
||||
typeof(HttpOptionsAttribute)
|
||||
];
|
||||
|
||||
/// <summary>
|
||||
/// Asserts that a controller follows the two-layer authorization pattern required by Bitwarden.
|
||||
/// </summary>
|
||||
/// <param name="controllerType">The controller type to validate.</param>
|
||||
/// <remarks>
|
||||
/// This enforces two requirements:
|
||||
/// <list type="number">
|
||||
/// <item>
|
||||
/// <description>
|
||||
/// Class-level requirement: The controller MUST have a class-level <c>[Authorize]</c> attribute.
|
||||
/// </description>
|
||||
/// </item>
|
||||
/// <item>
|
||||
/// <description>
|
||||
/// Method-level requirement: Every HTTP action method MUST have either:
|
||||
/// <list type="bullet">
|
||||
/// <item><description>Any custom <c>AuthorizeAttribute</c> implementation (e.g., <c>[Authorize<TRequirement>]</c>) for protected endpoints, OR</description></item>
|
||||
/// <item><description><c>[AllowAnonymous]</c> for intentionally public endpoints</description></item>
|
||||
/// </list>
|
||||
/// </description>
|
||||
/// </item>
|
||||
/// </list>
|
||||
/// <para>
|
||||
/// This ensures that every route is explicitly decorated with authorization, preventing accidental
|
||||
/// exposure of endpoints. The class-level <c>[Authorize]</c> alone is necessary but not sufficient.
|
||||
/// Note that the base <c>[Authorize]</c> attribute is not accepted at the method level.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <exception cref="Xunit.Sdk.FailException">
|
||||
/// Thrown when the controller is missing class-level authorization, has no HTTP methods,
|
||||
/// or has HTTP methods without explicit method-level authorization.
|
||||
/// </exception>
|
||||
/// <example>
|
||||
/// <code>
|
||||
/// [Fact]
|
||||
/// public void AllActionMethodsHaveAuthorization()
|
||||
/// {
|
||||
/// ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(
|
||||
/// typeof(MyController));
|
||||
/// }
|
||||
/// </code>
|
||||
/// </example>
|
||||
public static void AssertAllHttpMethodsHaveAuthorization(Type controllerType)
|
||||
{
|
||||
var methods = controllerType.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly);
|
||||
|
||||
var httpActionMethods = methods
|
||||
.Where(HasHttpMethodAttribute)
|
||||
.ToList();
|
||||
|
||||
if (httpActionMethods.Count == 0)
|
||||
{
|
||||
Assert.Fail($"Controller {controllerType.Name} has no HTTP action methods.");
|
||||
}
|
||||
|
||||
// REQUIRE class-level [Authorize]
|
||||
var classHasAuthorization = HasAuthorizeAttribute(controllerType);
|
||||
if (!classHasAuthorization)
|
||||
{
|
||||
Assert.Fail(
|
||||
$"Controller {controllerType.Name} is missing required class-level [Authorize] attribute.\n" +
|
||||
$"All controllers must have [Authorize] at the class level as a baseline security measure.");
|
||||
}
|
||||
|
||||
// REQUIRE each method to have explicit authorization
|
||||
var unauthorizedMethods = new List<string>();
|
||||
|
||||
foreach (var method in httpActionMethods)
|
||||
{
|
||||
// Only check for custom [Authorize<T>] (not base [Authorize])
|
||||
var methodHasCustomAuthorize = HasCustomAuthorizeAttribute(method);
|
||||
var methodHasAllowAnonymous = HasAllowAnonymousAttribute(method);
|
||||
|
||||
// Method must have EITHER [Authorize<T>] OR [AllowAnonymous]
|
||||
var hasAuthorizationAttribute = methodHasCustomAuthorize || methodHasAllowAnonymous;
|
||||
|
||||
if (!hasAuthorizationAttribute)
|
||||
{
|
||||
var httpAttributes = string.Join(", ",
|
||||
method.GetCustomAttributes()
|
||||
.Where(a => _httpMethodAttributes.Contains(a.GetType()))
|
||||
.Select(a => $"[{a.GetType().Name.Replace("Attribute", "")}]"));
|
||||
|
||||
unauthorizedMethods.Add($"{method.Name} ({httpAttributes})");
|
||||
}
|
||||
}
|
||||
|
||||
if (unauthorizedMethods.Count != 0)
|
||||
{
|
||||
var methodList = string.Join("\n - ", unauthorizedMethods);
|
||||
Assert.Fail(
|
||||
$"Controller {controllerType.Name} has {unauthorizedMethods.Count} HTTP action method(s) without method-level authorization:\n" +
|
||||
$" - {methodList}\n\n" +
|
||||
$"Each HTTP action method must be explicitly decorated with:\n" +
|
||||
$" - [Authorize<TRequirement>] for protected endpoints, OR\n" +
|
||||
$" - [AllowAnonymous] for intentionally public endpoints\n\n" +
|
||||
$"Note: Class-level [Authorize] is required but not sufficient. Every route must be explicitly decorated.");
|
||||
}
|
||||
}
|
||||
|
||||
private static bool HasHttpMethodAttribute(MethodInfo method)
|
||||
{
|
||||
return method.GetCustomAttributes()
|
||||
.Any(attr => _httpMethodAttributes.Contains(attr.GetType()));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Checks if a type or method has any [Authorize] attribute (including subclasses).
|
||||
/// Used for class-level checks.
|
||||
/// </summary>
|
||||
private static bool HasAuthorizeAttribute(MemberInfo member)
|
||||
{
|
||||
return member.GetCustomAttributes()
|
||||
.Any(attr => attr.GetType().IsAssignableTo(typeof(AuthorizeAttribute)));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Checks if a method has a custom (subclassed) [Authorize] attribute.
|
||||
/// Does NOT match the base [Authorize] attribute.
|
||||
/// Used for method-level checks.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// We don't match the base [Authorize] attribute because we don't currently use this
|
||||
/// for role-based checks on methods, so it is unlikely to indicate a
|
||||
/// proper authorization check. This is based on current practice only and could be
|
||||
/// changed in the future if our practice changes.
|
||||
/// </remarks>
|
||||
private static bool HasCustomAuthorizeAttribute(MethodInfo method)
|
||||
{
|
||||
return method.GetCustomAttributes()
|
||||
.Select(attr => attr.GetType())
|
||||
.Any(attrType => attrType.IsSubclassOf(typeof(AuthorizeAttribute)));
|
||||
}
|
||||
|
||||
private static bool HasAllowAnonymousAttribute(MethodInfo method)
|
||||
{
|
||||
return method.GetCustomAttribute<AllowAnonymousAttribute>() != null;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,133 @@
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Xunit;
|
||||
|
||||
namespace Bit.Api.Test.Utilities;
|
||||
|
||||
public class ControllerAuthorizationTestHelpersTests
|
||||
{
|
||||
[Fact]
|
||||
public void AssertAllHttpMethodsHaveAuthorization_ControllerMissingClassLevelAuthorize_Throws()
|
||||
{
|
||||
var exception = Assert.Throws<Xunit.Sdk.FailException>(() =>
|
||||
ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(
|
||||
typeof(ControllerWithoutClassAuthorize)));
|
||||
|
||||
Assert.Contains("missing required class-level [Authorize] attribute", exception.Message);
|
||||
Assert.Contains("ControllerWithoutClassAuthorize", exception.Message);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AssertAllHttpMethodsHaveAuthorization_MethodMissingAuthorization_Throws()
|
||||
{
|
||||
var exception = Assert.Throws<Xunit.Sdk.FailException>(() =>
|
||||
ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(
|
||||
typeof(ControllerWithUnauthorizedMethod)));
|
||||
|
||||
Assert.Contains("3 HTTP action method(s) without method-level authorization", exception.Message);
|
||||
Assert.Contains("GetUnauthorized ([HttpGet])", exception.Message);
|
||||
Assert.Contains("PostUnauthorized ([HttpPost])", exception.Message);
|
||||
Assert.Contains("PutUnauthorized ([HttpPut])", exception.Message);
|
||||
Assert.Contains("ControllerWithUnauthorizedMethod", exception.Message);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AssertAllHttpMethodsHaveAuthorization_AllMethodsProperlyAuthorized_DoesNotThrow()
|
||||
{
|
||||
ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(
|
||||
typeof(ControllerWithProperAuthorization));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AssertAllHttpMethodsHaveAuthorization_MethodWithAllowAnonymous_DoesNotThrow()
|
||||
{
|
||||
ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(
|
||||
typeof(ControllerWithAllowAnonymous));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AssertAllHttpMethodsHaveAuthorization_ControllerWithNoHttpMethods_Throws()
|
||||
{
|
||||
var exception = Assert.Throws<Xunit.Sdk.FailException>(() =>
|
||||
ControllerAuthorizationTestHelpers.AssertAllHttpMethodsHaveAuthorization(
|
||||
typeof(ControllerWithNoHttpMethods)));
|
||||
|
||||
Assert.Contains("has no HTTP action methods", exception.Message);
|
||||
}
|
||||
|
||||
// Controller missing class-level [Authorize]
|
||||
private class ControllerWithoutClassAuthorize : ControllerBase
|
||||
{
|
||||
[HttpGet]
|
||||
[CustomAuthorize]
|
||||
public IActionResult Get() => Ok();
|
||||
}
|
||||
|
||||
// Controller with class-level [Authorize] but methods missing method-level authorization
|
||||
[Authorize]
|
||||
private class ControllerWithUnauthorizedMethod : ControllerBase
|
||||
{
|
||||
[HttpGet("authorized")]
|
||||
[CustomAuthorize]
|
||||
public IActionResult GetAuthorized() => Ok();
|
||||
|
||||
[HttpGet("unauthorized")]
|
||||
public IActionResult GetUnauthorized() => Ok();
|
||||
|
||||
[HttpPost("unauthorized")]
|
||||
public IActionResult PostUnauthorized() => Ok();
|
||||
|
||||
[HttpPut("unauthorized")]
|
||||
public IActionResult PutUnauthorized() => Ok();
|
||||
|
||||
// Non-HTTP method should be ignored
|
||||
public IActionResult NonHttpMethod() => Ok();
|
||||
}
|
||||
|
||||
// Controller with proper authorization on all methods
|
||||
[Authorize]
|
||||
private class ControllerWithProperAuthorization : ControllerBase
|
||||
{
|
||||
[HttpGet("custom")]
|
||||
[CustomAuthorize]
|
||||
public IActionResult GetWithCustom() => Ok();
|
||||
|
||||
[HttpPost("custom")]
|
||||
[CustomAuthorize]
|
||||
public IActionResult PostWithCustom() => Ok();
|
||||
|
||||
[HttpDelete("custom")]
|
||||
[CustomAuthorize]
|
||||
public IActionResult DeleteWithCustom() => Ok();
|
||||
}
|
||||
|
||||
// Controller with AllowAnonymous (which is valid method-level authorization)
|
||||
[Authorize]
|
||||
private class ControllerWithAllowAnonymous : ControllerBase
|
||||
{
|
||||
[HttpGet("anonymous")]
|
||||
[AllowAnonymous]
|
||||
public IActionResult GetAnonymous() => Ok();
|
||||
|
||||
[HttpPost("protected")]
|
||||
[CustomAuthorize]
|
||||
public IActionResult PostProtected() => Ok();
|
||||
|
||||
[HttpGet("mixed")]
|
||||
[AllowAnonymous]
|
||||
public IActionResult GetMixed() => Ok();
|
||||
}
|
||||
|
||||
// Controller with no HTTP methods
|
||||
[Authorize]
|
||||
private class ControllerWithNoHttpMethods : ControllerBase
|
||||
{
|
||||
public IActionResult NotAnHttpMethod() => Ok();
|
||||
public void AnotherNonHttpMethod() { }
|
||||
}
|
||||
|
||||
// Custom authorize attribute for testing (mimics [Authorize<T>])
|
||||
private class CustomAuthorizeAttribute : AuthorizeAttribute
|
||||
{
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user