Unwind Phase 3a service-layer auth, tombstone Phase 3 of auth plan#231
Merged
peterdrier merged 4 commits intomainfrom Apr 15, 2026
Merged
Unwind Phase 3a service-layer auth, tombstone Phase 3 of auth plan#231peterdrier merged 4 commits intomainfrom
peterdrier merged 4 commits intomainfrom
Conversation
The old rules said "services own DB access" and meant "services inject DbContext directly." That conflated business logic with persistence, made cross-domain joins impossible to forbid structurally, and left caching as an inline IMemoryCache concern scattered through service methods. The rewrite updates the target to: - Services live in Humans.Application (not Infrastructure) so they physically cannot import EntityFrameworkCore — enforced by the project reference graph, not by convention. - Every domain has a narrow IProfileRepository-style interface in Application, with an EF-backed implementation in Infrastructure. Entities in, entities out; no IQueryable, no cross-domain signatures, no Includes that cross domains. - Every cached domain has an IProfileStore — a dict-backed in-memory entity cache with a single writer (the owning service), warmed on startup. Replaces inline IMemoryCache.GetOrCreateAsync calls. - Caching is applied via a Scrutor decorator (CachingProfileService) wrapping the service, not by mixing cache logic into the service itself. One crosscut, one wrapper, zero business logic in the decorator. - Cross-domain EF joins are forbidden. When a caller needs Profile + Team + User together, it asks each owning service and stitches in memory via GetByIdsAsync. 48 existing cross-domain .Include() calls across 20 services are the migration blast radius. - Decorators are only for mechanical, context-free crosscuts (caching, metrics, retry, access logging). Domain audit stays in-service because it needs actor, before/after, semantic intent, and same- unit-of-work guarantees. - Migration is per-domain, starting with a Profile spike, then quarantining each domain one at a time in priority order. Concrete known-violation counts included so progress is measurable. Keep sections unchanged: Table Ownership Map, Cross-Cutting Services, Authorization Pattern, Immutable Entity Rules, Google Resource Ownership. These were already aligned with the target. Follow-ups not in this commit: update dependency-graph.md with the target graph, update coding-rules.md and code-review-rules.md with grep-able enforcement rules, update CLAUDE.md's Critical: Design Rules bullet list. These land in separate commits so the diffs stay readable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pure rename. Content is unchanged in this commit; the trim to remove persistence/service-layer content that conflicts with design-rules.md lands in the next commit so git rename detection preserves history. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove ~270 lines of persistence/service-layer content that conflicted
with design-rules.md (§1 Current Shape, §2-§7 Core Decisions, Layer
Rules, Read Rules, Write Rules, Caching Rules, Direction of Travel).
The persistence/service doctrine now lives exclusively in design-rules.md.
Keep the cross-cutting content that has no better home:
- Domain Owns Local Invariants
- Transaction Boundary
- Caching (controller-level rules only, with pointer to design-rules §4-§5)
- Authorization (web/service rules only, with pointer to design-rules §11)
- Integration (no vendor types leaking, stub impls over env checks)
- Time and Configuration (IClock, NodaTime, centralized config)
- Rendering (Razor default, fetch() exceptions with per-file list)
- Testing (service-boundary first, preferred test order)
- Exception Rule (how to justify breaking a default)
- Smell Checklist (now reorganized and expanded with the new smells:
services in Infrastructure/Services/, direct DbContext in services,
cross-domain .Include, IQueryable-returning repositories, inline
IMemoryCache, cross-domain nav properties, audit-as-decorator)
The Exception Rule gains one weak-reason bullet ("adding a repository
felt like over-engineering") so the old anti-repository position is
explicitly retired rather than left as silent drift.
docs/README.md: update the Architecture section from the outdated
"Infrastructure owns services" layer map to the new layering, and
replace the single link to architecture.md with a full index of the
nine docs now living in docs/architecture/ (design rules, conventions,
dependency graph, data model, coding rules, code review rules, service
data access map, code analysis, maintenance log).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 3 of docs/plans/2026-04-03-first-class-authorization-transition.md proposed pushing authorization into service methods. The track record on this codebase is 0-for-2: - Phase 3a (nobodies-collective#418, PR #210, role assignment) shipped and broke QA within two days with circular DI: TeamService -> RoleAssignmentService -> IAuthorizationService -> TeamAuthorizationHandler -> ITeamService "Fixed" in 225ac14 by making TeamAuthorizationHandler lazily resolve ITeamService via IServiceProvider -- a service-locator escape hatch that hides the cycle from the DI validator instead of removing it. - Phase 3b (nobodies-collective#419, Google sync) was merged (1626098) and reverted (bbbe508) for the same reason. - Phase 3c (nobodies-collective#420, budget mutations) was drafted but never merged. Closed as "won't do" on 2026-04-15. design-rules.md section 11 already says services are auth-free. This PR makes the code match the rule. Changes: - RoleAssignmentService: drop IAuthorizationService dependency and the ClaimsPrincipal parameter on AssignRoleAsync / EndRoleAsync. Service is auth-free again. - IRoleAssignmentService: match signature change. - ProfileController: authorize with IAuthorizationService.AuthorizeAsync and RoleAssignmentOperationRequirement.Manage before calling the service. For EndRole, fetch the assignment, authorize on its role name, return NotFound on deny (prevents enumeration). - RoleAssignmentAuthorizationHandler: remove the SystemPrincipal bypass branch; handler now only checks Admin and Board/HumanAdmin+ BoardManageableRoles. - SystemPrincipal: deleted. No remaining callers. - RoleAssignmentClaimsTransformation: drop the defensive strip of SystemPrincipal claims (the claim no longer grants anything). - TeamAuthorizationHandler: revert 225ac14. Take ITeamService as a normal constructor dependency. The cycle is gone because RoleAssignmentService no longer injects IAuthorizationService, so there is no path back into TeamAuthorizationHandler from a service. - Plan doc: tombstone Phase 3 with full rationale; update sequencing and issue chain sections to reflect cancellation. - Tests: drop auth-denial and SystemPrincipal tests that were testing the removed code path; the handler tests already cover the deny cases at the correct layer. TeamServiceTests and TeamRoleServiceTests updated for the RoleAssignmentService ctor signature change. Verified: dotnet build clean (0 errors), 941 Application tests pass, 108 Domain tests pass. Integration tests not run -- Docker daemon not available locally, unrelated to this change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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
Reverts Phase 3a (nobodies-collective#418) of the first-class authorization transition, tombstones Phase 3 in the plan doc, and removes the service-locator hack that was added to paper over the original crash.
Why
Phase 3 proposed pushing authorization into service methods (services inject
IAuthorizationService, service methods takeClaimsPrincipal, jobs passSystemPrincipal). Track record on Humans: 0-for-2.TeamService → RoleAssignmentService → IAuthorizationService → TeamAuthorizationHandler → ITeamService. "Fixed" in225ac14by makingTeamAuthorizationHandlerlazily resolveITeamServiceviaIServiceProvider— hides the cycle from the validator, doesn't remove it.1626098), reverted (bbbe508) for the same reason.sprint/20260415/batch-4but never merged. Closed as won't do on 2026-04-15.docs/architecture/design-rules.md§11 already says services are auth-free. This PR makes the code match the rule.Changes
RoleAssignmentService: dropIAuthorizationServicedependency and theClaimsPrincipal principalparameter onAssignRoleAsync/EndRoleAsync. Service is auth-free again.IRoleAssignmentService: match signature change.ProfileController: authorize withIAuthorizationService.AuthorizeAsync(User, roleName, RoleAssignmentOperationRequirement.Manage)before calling the service.EndRolefetches the assignment, authorizes against its role name, returnsNotFoundon deny to prevent enumeration (same behavior as before).RoleAssignmentAuthorizationHandler: remove theSystemPrincipalbypass branch. Handler now only checksAdminandBoard/HumanAdmin+BoardManageableRoles.SystemPrincipal: deleted. No remaining callers.RoleAssignmentClaimsTransformation: drop the defensive strip ofSystemPrincipalclaims — the claim no longer grants anything.TeamAuthorizationHandler: revert225ac14. TakeITeamServiceas a normal constructor dependency. The cycle is gone becauseRoleAssignmentServiceno longer injectsIAuthorizationService, so there is no path back intoTeamAuthorizationHandlerfrom a service.docs/plans/2026-04-03-first-class-authorization-transition.md): tombstone Phase 3 with full rationale; update sequencing and issue chain sections to reflect cancellation.SystemPrincipaltests that were testing the removed code path. The deny cases are already covered at the correct layer (RoleAssignmentAuthorizationHandlerTests).TeamServiceTestsandTeamRoleServiceTestsupdated for theRoleAssignmentServicector signature change.Test plan
dotnet build Humans.slnx— clean, 0 errors, 0 warningsRoleAssignmentService,RoleAssignmentAuthorizationHandler,TeamAuthorizationHandler,TeamService,TeamRoleService)/Profile/{id}/Admin/Roles; verify Admin can, Board can for BoardManageableRoles, regular user gets Forbid/NotFoundTeamAuthorizationHandlerDI still resolves cleanly (previously crashed here)🤖 Generated with Claude Code