Service-layer auth enforcement for budget mutations (#420)#230
Open
peterdrier wants to merge 2 commits intomainfrom
Open
Service-layer auth enforcement for budget mutations (#420)#230peterdrier wants to merge 2 commits intomainfrom
peterdrier wants to merge 2 commits intomainfrom
Conversation
Promote QA batch: service ownership, dashboard cluster, rota partials
…ive#420) Phase 3c (final slice) of the first-class authorization transition plan. Budget mutations are now protected at the service boundary regardless of call path (controllers, background jobs, future API surfaces), mirroring the pattern established by #210 (role assignment). - BudgetOperationRequirement moves to Humans.Application.Authorization and gains a resource-free Manage static alongside the existing Edit. - New BudgetManageAuthorizationHandler succeeds for Admin / FinanceAdmin / SystemPrincipal on the Manage requirement (used for budget years, groups, categories, ticketing projection parameters, and ticketing sync jobs). - Existing BudgetAuthorizationHandler still handles Edit against a BudgetCategory resource (department-coordinator scoping) and now succeeds for SystemPrincipal. Uses IServiceProvider to lazily resolve IBudgetService, matching TeamAuthorizationHandler so the new BudgetService -> IAuthorizationService dependency does not introduce a DI cycle. - Every IBudgetService mutation method now accepts a ClaimsPrincipal and calls IAuthorizationService.AuthorizeAsync before touching the database. Unauthorized callers raise UnauthorizedAccessException. Line-item create/update/delete authorize against the loaded BudgetCategory (Edit); year/group/category/projection/sync mutations authorize against null (Manage). - BudgetController and FinanceController forward User to the service; the controller-level [Authorize(Policy = FinanceAdminOrAdmin)] and resource-based edit check stay as defense in depth. - TicketingBudgetService and ITicketingBudgetService forward ClaimsPrincipal through to BudgetService. TicketingBudgetSyncJob and DevelopmentBudgetSeeder pass SystemPrincipal.Instance. - RoleChecks is now an internal helper — part of the Phase 1 cleanup called out in docs/plans/2026-04-03-first-class-authorization-transition.md. ViewPolicies was already removed in earlier phases (no references remain outside that plan doc). Tests: - BudgetServiceTests updated for the new ctor, plus new cases covering unauthorized CreateLineItem / UpdateLineItem / DeleteLineItem / CreateYear / UpdateYearStatus / CreateGroup / CreateCategory paths (UnauthorizedAccessException thrown, no DB mutation) and an authorized FinanceAdmin path. - BudgetAuthorizationHandlerTests updated for the IServiceProvider constructor and exercises the new SystemPrincipal override plus the "Manage requirement not handled" guard. - New BudgetManageAuthorizationHandlerTests covers Admin, FinanceAdmin, SystemPrincipal, Board (denied), regular user (denied), unauthenticated user (denied), and the "Edit requirement not handled" guard. - docs/sections/Budget.md gains an Authorization note describing the two-layer defense. Closes nobodies-collective#420 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The preview deployment for humans-qa is ready. 🟢 Open Preview | Open Build Logs | Open Application Logs Last updated at: 2026-04-15 02:32:33 CET |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 3c (final slice) of the first-class authorization transition plan. Budget mutations are now enforced at the service boundary in addition to the existing controller checks, protecting budget writes regardless of call path (controllers, background jobs, future APIs). Follows the pattern established by #210 (role assignment) and the TeamAuthorizationHandler DI-cycle fix from 225ac14.
BudgetOperationRequirementmoved toHumans.Application.Authorizationwith a newManagesingleton alongside the existingEdit.BudgetManageAuthorizationHandlersucceeds for Admin / FinanceAdmin /SystemPrincipalon resource-freeManagechecks. ExistingBudgetAuthorizationHandlerstill handlesEditagainst aBudgetCategoryresource (department-coordinator scoping), now also allowsSystemPrincipal, and lazily resolvesIBudgetServiceviaIServiceProviderto avoid a DI cycle now thatBudgetServiceitself takesIAuthorizationService.IBudgetServicemutation method acceptsClaimsPrincipaland callsIAuthorizationService.AuthorizeAsyncbefore touching the database — unauthorized callers raiseUnauthorizedAccessException. Line-item writes authorize against the loadedBudgetCategory(Edit); year/group/category/projection/ticketing-sync writes authorize against null (Manage).BudgetControllerandFinanceControllerforwardUserto the service; the controller-level[Authorize(Policy = FinanceAdminOrAdmin)]and resource-basedEditcheck stay as defense in depth.TicketingBudgetService/ITicketingBudgetServiceforwardClaimsPrincipaltoBudgetService.TicketingBudgetSyncJobandDevelopmentBudgetSeederpassSystemPrincipal.Instance.RoleChecksis now an internal helper — part of the Phase 1 cleanup called out indocs/plans/2026-04-03-first-class-authorization-transition.md.ViewPolicieswas already removed in earlier phases (no references remain outside that plan doc).Tests
BudgetServiceTestsupdated for the new ctor, plus new cases covering unauthorizedCreateLineItem/UpdateLineItem/DeleteLineItem/CreateYear/UpdateYearStatus/CreateGroup/CreateCategorypaths (UnauthorizedAccessExceptionthrown, no DB mutation) and an authorized FinanceAdmin path.BudgetAuthorizationHandlerTestsupdated for theIServiceProviderconstructor and exercises the newSystemPrincipaloverride plus the "Managerequirement not handled here" guard.BudgetManageAuthorizationHandlerTestscovers Admin / FinanceAdmin /SystemPrincipal/ Board (denied) / regular user (denied) / unauthenticated user (denied) / "Editrequirement not handled here" guard.docs/sections/Budget.mdgains an Authorization note describing the two-layer defense.Test plan
dotnet build Humans.slnx— cleandotnet test Humans.Application.Tests— 965 passed, 0 faileddotnet format Humans.slnx --verify-no-changes— cleanHumans.Integration.Tests) — pre-existing Docker/testcontainers unavailability on this host, unrelated to this change/Financemutations in QA as FinanceAdmin (create/edit/delete year, group, category, line item)/Budgetdepartment coordinator editing of own-team line itemsTicketingBudgetSyncJobruns after deploy (budget audit log should show system actor)Closes nobodies-collective#420
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com