fix(security): enforce explicit api authorization
Authentication relied on a fallback policy gated on Auth:Require, which defaults to false. Five user-owned controllers carried no [Authorize] of their own, so a deployment that lost that flag would have served tenant data anonymously: JobApplications, Companies, Correspondence, Rules and JobImport. All five now declare [Authorize(AuthenticationSchemes = "local")] explicitly. This does not affect local development, which already sets Auth:Require=true in appsettings.Development.json — the gap was only ever in a production configuration that omitted the flag. Added a reflection test over every controller in the assembly so a new one cannot ship unprotected by accident. A controller passes if the class requires authorization, or if every action declares its own [Authorize] or [AllowAnonymous] — the shape AuthController and TwoFactorController need, since login and register must stay anonymous while the rest must not. Public endpoints are an explicit allow-list, so making something anonymous is now a deliberate edit rather than an omission. That test found one real gap: AuthController.Logout declared neither attribute. It is now explicitly [AllowAnonymous] — it only clears the caller's own session cookies and leaks nothing, and requiring authentication would leave a user whose token had already expired unable to sign out. Also pinned: admin controllers require the Admin role rather than merely a signed-in user, and PublicCvController stays anonymous so shared CV links keep working. 384 backend tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,126 @@
|
||||
using System.Reflection;
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Xunit;
|
||||
|
||||
namespace JobTrackerApi.Tests;
|
||||
|
||||
// Authorization posture, asserted by reflection over every controller in the assembly.
|
||||
//
|
||||
// The point is that a NEW controller cannot ship unprotected by accident. Authentication used to rely
|
||||
// on the Auth:Require fallback policy, which defaults to false — a deployment that lost that flag
|
||||
// would have served tenant data anonymously. Every controller must now declare its intent.
|
||||
// docs/production-readiness-review.md.
|
||||
public sealed class ApiAuthorizationPostureTests
|
||||
{
|
||||
// Endpoints that are anonymous ON PURPOSE. Adding to this list is a deliberate security decision.
|
||||
private static readonly HashSet<string> IntentionallyAnonymous = new(StringComparer.Ordinal)
|
||||
{
|
||||
// Serves /cv/{slug} for variants the user explicitly published. Noindex by default.
|
||||
"PublicCvController",
|
||||
// Accepts browser error reports. Must work on pages reached before sign-in.
|
||||
"ClientErrorsController",
|
||||
};
|
||||
|
||||
// Property accessors and object overrides are not endpoints.
|
||||
private static bool IsEndpoint(MethodInfo m) => !(
|
||||
m.IsSpecialName || m.DeclaringType == typeof(object));
|
||||
|
||||
private static IEnumerable<Type> Controllers() =>
|
||||
typeof(JobTrackerApi.Controllers.ApplicationWorkspaceController).Assembly
|
||||
.GetTypes()
|
||||
.Where(t => typeof(ControllerBase).IsAssignableFrom(t) && !t.IsAbstract);
|
||||
|
||||
// A controller is covered if the CLASS requires authorization, or if every action declares its own
|
||||
// intent. AuthController and TwoFactorController are necessarily mixed — login and register must be
|
||||
// anonymous while the rest are not — so they mark each method individually.
|
||||
private static bool DeclaresIntent(Type controller)
|
||||
{
|
||||
if (controller.GetCustomAttribute<AuthorizeAttribute>(inherit: true) is not null) return true;
|
||||
|
||||
var actions = controller
|
||||
.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)
|
||||
.Where(IsEndpoint)
|
||||
.ToList();
|
||||
|
||||
return actions.Count > 0 && actions.All(m =>
|
||||
m.GetCustomAttribute<AuthorizeAttribute>(inherit: true) is not null ||
|
||||
m.GetCustomAttribute<AllowAnonymousAttribute>(inherit: true) is not null);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Every_controller_either_requires_authorization_or_is_listed_as_public()
|
||||
{
|
||||
var unprotected = Controllers()
|
||||
.Where(t => !IntentionallyAnonymous.Contains(t.Name))
|
||||
.Where(t => !DeclaresIntent(t))
|
||||
.Select(t => t.Name)
|
||||
.OrderBy(name => name, StringComparer.Ordinal)
|
||||
.ToList();
|
||||
|
||||
Assert.True(
|
||||
unprotected.Count == 0,
|
||||
"These controllers declare no authorization intent — neither on the class nor on every "
|
||||
+ "action — so they would be served anonymously if Auth:Require were ever false: "
|
||||
+ string.Join(", ", unprotected));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Every_action_on_the_mixed_auth_controllers_declares_its_own_intent()
|
||||
{
|
||||
// Sign-in surfaces are the easiest place to add an endpoint and forget to mark it.
|
||||
foreach (var name in new[] { "AuthController", "TwoFactorController" })
|
||||
{
|
||||
var type = Controllers().Single(t => t.Name == name);
|
||||
var undeclared = type
|
||||
.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)
|
||||
.Where(IsEndpoint)
|
||||
.Where(m => m.GetCustomAttribute<AuthorizeAttribute>(inherit: true) is null
|
||||
&& m.GetCustomAttribute<AllowAnonymousAttribute>(inherit: true) is null)
|
||||
.Select(m => m.Name)
|
||||
.ToList();
|
||||
|
||||
Assert.True(undeclared.Count == 0,
|
||||
$"{name} has actions with neither [Authorize] nor [AllowAnonymous]: {string.Join(", ", undeclared)}");
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void The_user_owned_controllers_that_used_to_rely_on_the_fallback_policy_are_now_explicit()
|
||||
{
|
||||
// These five were the actual gap found in the Phase 5.6 audit.
|
||||
foreach (var name in new[]
|
||||
{
|
||||
"JobApplicationsController", "CompaniesController", "CorrespondenceController",
|
||||
"RulesController", "JobImportController",
|
||||
})
|
||||
{
|
||||
var type = Controllers().Single(t => t.Name == name);
|
||||
var attribute = type.GetCustomAttribute<AuthorizeAttribute>(inherit: true);
|
||||
|
||||
Assert.True(attribute is not null, $"{name} must declare [Authorize].");
|
||||
Assert.Equal("local", attribute!.AuthenticationSchemes);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Admin_controllers_require_the_admin_role_not_merely_a_signed_in_user()
|
||||
{
|
||||
foreach (var name in new[] { "AdminAuditController", "AdminSystemController", "UsersController" })
|
||||
{
|
||||
var type = Controllers().Single(t => t.Name == name);
|
||||
var attribute = type.GetCustomAttribute<AuthorizeAttribute>(inherit: true);
|
||||
|
||||
Assert.True(attribute is not null, $"{name} must declare [Authorize].");
|
||||
Assert.Equal("Admin", attribute!.Roles);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void The_public_cv_controller_stays_anonymous()
|
||||
{
|
||||
// Regression guard in the other direction: locking this down would break every shared CV link.
|
||||
var type = Controllers().Single(t => t.Name == "PublicCvController");
|
||||
Assert.Null(type.GetCustomAttribute<AuthorizeAttribute>(inherit: true));
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user