Add service-layer auth enforcement for role assignment

[peterdrier]

Summary

• Push authorization checks into RoleAssignmentService so role assignment/removal is protected regardless of call path (controller, background job, future API)
• Service methods now accept ClaimsPrincipal and call IAuthorizationService with a new RoleAssignmentOperationRequirement before executing mutations
• Add SystemPrincipal helper providing a system-level auth context for background jobs, with RoleAssignmentAuthorizationHandler that grants system principal full access

Issues

• Closes Add service-layer auth enforcement: role assignment and removal nobodies-collective/Humans#418

Test plan

• RoleAssignmentAuthorizationHandlerTests: 18 test cases covering Admin override, Board/HumanAdmin per-role access, system principal bypass, and denial for unauthorized users
• RoleAssignmentServiceTests: New tests for authorization denial on AssignRoleAsync and EndRoleAsync, and system principal forwarding verification
• Existing tests updated and passing (911 application tests, 0 failures)
• Build and format check pass
• Smoke test role assignment/removal in QA after deploy

[peterdrier]

Release Review — Issues Found

1. The new service-layer authorization handler, its doc comment, and its tests still encode a mismatched role matrix. RoleAssignmentAuthorizationHandler says Board/HumanAdmin can manage CampAdmin, TicketAdmin, and NoInfoAdmin (src/Humans.Web/Authorization/Requirements/RoleAssignmentAuthorizationHandler.cs:11-16), but the actual check delegates to RoleChecks.CanManageRole, which omits all three (src/Humans.Web/Authorization/RoleChecks.cs:155-163). The new handler tests also skip those cases entirely (tests/Humans.Application.Tests/Authorization/RoleAssignmentAuthorizationHandlerTests.cs:56-89).

Since this PR is centralizing role-assignment enforcement, I would fix that mismatch and add coverage for the missing roles before releasing it to production.

[peterdrier]

PR Review — 2026-04-10

PR: #210 — Add service-layer auth enforcement for role assignment (link)
Branch: sprint/2026-04-10/batch-3 → main
Scope: 11 files changed, 1 commit

---

Prior Review Status

Reviewer	Finding	Status
peterdrier	Role matrix mismatch: handler doc comment lists CampAdmin, TicketAdmin, NoInfoAdmin as manageable by Board/HumanAdmin, but CanManageRole omits all three	STILL OPEN — confirmed in both spec compliance and code quality reviews. Additionally, BoardAssignableRoles array and section invariants (Governance.md, Admin.md) also describe the broader set, widening the contradiction.

---

Spec Compliance

Issue nobodies-collective#418: Add service-layer auth enforcement: role assignment and removal (OPEN)

Intent: Move authorization enforcement for role assignment/removal from controllers to the service layer, so it applies regardless of call path (controller, background job, future API). Provide a system-level auth context for background jobs.

Verdict: FULLY ADDRESSED

Evidence:

• RoleAssignmentService.AssignRoleAsync and EndRoleAsync now call IAuthorizationService.AuthorizeAsync before mutations
• ProfileController removes controller-level CanManageRole checks, passes User to service, maps unauthorized to Forbid()
• SystemPrincipal.Instance provides a singleton system claim; handler grants full access
• 18 handler tests + authorization denial tests for both assign/end + system principal verification

All 4 acceptance criteria met. GovernanceController and BoardController don't call mutation methods, so only ProfileController needed changes — correct.

Gaps: None against the stated spec.

---

Code Quality

CRITICAL

1. Role matrix contradiction — CanManageRole is narrower than doc comments, BoardAssignableRoles, and section invariants

Four sources say Board/HumanAdmin can manage CampAdmin, TicketAdmin, NoInfoAdmin:

• Handler doc comment (RoleAssignmentAuthorizationHandler.cs:12-14)
• RoleChecks.BoardAssignableRoles array (RoleChecks.cs:23-35)
• docs/sections/Governance.md:29 — "Board and HumanAdmin can assign all roles except Admin"
• docs/sections/Admin.md:19 — same

But RoleChecks.CanManageRole (RoleChecks.cs:155-163) omits all three. The handler delegates to CanManageRole, so Board/HumanAdmin are silently denied. Tests skip those roles entirely.

Decision needed: expand CanManageRole to match docs (likely correct), or narrow docs to match code.

IMPORTANT

2. EndRoleAsync queries DB before auth check — RoleAssignmentService.cs:207-224

Fetches the RoleAssignment before running authorization. A caller can distinguish "exists but unauthorized" from "not found" without having permission. Auth check should precede or responses should be uniform.

3. Handler in Web layer; service in Infrastructure — RoleAssignmentAuthorizationHandler.cs

The service (Infrastructure) depends on IAuthorizationService which resolves handlers registered in Web's AddAuthorizationPolicies(). If the service is ever used from integration tests or a console tool that skips that registration, SystemPrincipal bypass won't work. Consider moving the handler to Infrastructure or Application alongside SystemPrincipal.

4. Test file in wrong project — Humans.Application.Tests/Authorization/RoleAssignmentAuthorizationHandlerTests.cs

Tests a Web-layer class but lives in Application.Tests. Should move to match the handler's project.

MINOR

5. Auth-denial logs omit principal identity — RoleAssignmentService.cs:140, 221

Warning logs include role name and target user but not the denied principal's identity. Add principal.Identity?.Name for audit traceability.

6. Missing test cases for CampAdmin/TicketAdmin/NoInfoAdmin — Once finding #1 is resolved, add theory cases for these roles (whether they should pass or fail).

Clean Areas

• Auth-before-mutation pattern well implemented in AssignRoleAsync
• Cache invalidation correct in both mutation paths
• SystemPrincipal implementation is clean (custom claim, singleton, IsSystem() helper)
• RoleAssignmentOperationRequirement follows ASP.NET Core resource-based auth pattern correctly
• Controller correctly removes redundant pre-check and delegates fully

---

Bottom Line

The PR fully addresses issue nobodies-collective#418 — all four acceptance criteria are met and the structural approach is solid. The critical blocker is the role matrix contradiction (finding #1): this PR codifies the narrower CanManageRole behavior in a handler while docs, BoardAssignableRoles, and section invariants all describe the broader set. This should be resolved before merge. The handler placement (finding #3) is a structural concern worth addressing now while the code is fresh.

---

Review by Claude Code /pr-review

[peterdrier]

Fixes Applied — 2026-04-10

Requested: Fix all 6 findings from the PR review.

Changes (commit e3ed627):

#	Finding	Fix	Status
1	CRITICAL: CanManageRole omits CampAdmin, TicketAdmin, NoInfoAdmin	Added all 3 roles to CanManageRole and new Application-layer BoardManageableRoles HashSet	VERIFIED
2	IMPORTANT: EndRoleAsync info disclosure (auth vs not-found distinguishable)	Both paths now return "NotFound" — uniform response prevents enumeration	VERIFIED
3	IMPORTANT: Handler in Web layer, service in Infrastructure	Moved RoleAssignmentAuthorizationHandler to Humans.Application.Authorization with self-contained role logic (no Web dependency)	VERIFIED
4	IMPORTANT: Test file in wrong project	Handler moved to Application, so tests in Humans.Application.Tests are now correctly placed; updated using	VERIFIED
5	MINOR: Auth-denial logs omit principal identity	Added principal.Identity?.Name to both AssignRoleAsync and EndRoleAsync warning logs	VERIFIED
6	MINOR: Missing test cases	Added CampAdmin, TicketAdmin, NoInfoAdmin to Board and HumanAdmin test theories; updated EndRoleAsync denial test to expect "NotFound"	VERIFIED

Verification: Build clean (0 errors, 0 warnings). All 1025 tests pass.

Note: BoardManageableRoles (Application) and CanManageRole (Web) are now intentionally duplicated — comment notes they must stay in sync. No compile-time enforcement.

---

Fix & re-review by Claude Code /pr-review