From ecde29a78cb532029c628e16e3f07cc454764cf6 Mon Sep 17 00:00:00 2001 From: Peter Drier Date: Wed, 15 Apr 2026 16:34:44 +0200 Subject: [PATCH 1/4] Rewrite design-rules.md for repository/store/decorator target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/architecture/design-rules.md | 374 +++++++++++++++++++++--------- 1 file changed, 270 insertions(+), 104 deletions(-) diff --git a/docs/architecture/design-rules.md b/docs/architecture/design-rules.md index b43bd98e..83b623d5 100644 --- a/docs/architecture/design-rules.md +++ b/docs/architecture/design-rules.md @@ -1,46 +1,230 @@ # Design Rules -Architectural rules governing how components interact. These are target-state rules — new code must follow them, existing code should be migrated incrementally (see [Migration Strategy](#migration-strategy)). +Architectural rules governing how Web, Application, Infrastructure, and Domain interact. **These are target-state rules.** New code must follow them; existing code is migrated incrementally per [Migration Strategy](#15-migration-strategy). ## 1. Layer Responsibilities -Clean Architecture with strict dependency direction: Web → Application → Domain. Infrastructure implements Application interfaces. +Clean Architecture with strict dependency direction. Application depends only on Domain. Infrastructure and Web both depend inward toward Application and Domain. Nothing depends on Web or Infrastructure. -| Layer | Allowed | Not Allowed | -|-------|---------|-------------| -| **Domain** | Entities, enums, value objects. No dependencies on other layers. | Services, interfaces, framework references | -| **Application** | Interfaces, DTOs, use cases. Depends only on Domain. | DbContext, EF Core, HTTP, external SDKs | -| **Infrastructure** | Implements Application interfaces. EF Core, external API clients, background jobs. | Controller logic, Razor, HTTP request/response | -| **Web** | Controllers, views, view models, API endpoints. Depends on Application interfaces only. | DbContext, direct EF queries, direct cache access | +``` +Domain ← Application ← Infrastructure + ← Web +``` + +| Layer | Contains | Forbidden | +|---|---|---| +| **Domain** | Entities, enums, value objects. No external dependencies. | Services, interfaces, framework references, EF types, DTOs | +| **Application** | Service **interfaces** and **implementations** (business logic), repository and store **interfaces**, DTOs, use cases, authorization handlers | `DbContext`, `Microsoft.EntityFrameworkCore.*`, HTTP types, external SDKs, direct I/O | +| **Infrastructure** | Repository implementations, store implementations, caching decorators, `HumansDbContext`, migrations, external API clients (Google, Stripe, SMTP), background jobs | Controller logic, Razor, HTTP request/response, business rules | +| **Web** | Controllers, views, view models, API endpoints, DI wiring | `DbContext`, direct EF queries, direct cache access for domain data, raw SQL | + +The project reference graph (`Humans.Application.csproj` references only `Humans.Domain.csproj`) **structurally enforces** that services in Application cannot import `Microsoft.EntityFrameworkCore`. EF pollution in business logic is a compile error, not a code-review finding. + +**Key change from prior rules:** Services now live in `Humans.Application`, not `Humans.Infrastructure`. The old rule ("services own their data access") meant "services inject `DbContext` directly," which conflated business logic with persistence and made "no cross-domain joins" impossible to enforce structurally. The new rule is "services go through their owning repository." ## 2. Service Ownership — The Core Rule -**Each service is the exclusive gateway to its data.** No other component — controller, service, job, or view component — may bypass the owning service to access its tables or cache. +Each service is the exclusive gateway to its data. No component — controller, other service, job, or view component — may bypass the owning service to reach its tables, its cache, or its store. ### 2a. Controllers Cannot Talk to the Database -Controllers call services. Controllers never inject `DbContext`, never write EF queries, never access `IMemoryCache` for domain data. Their job is: receive HTTP request → call service(s) → return response. +Controllers call services. Controllers never inject `DbContext`, never write EF queries, never instantiate repositories or stores directly, never access `IMemoryCache` for domain data. Their job is: receive HTTP request → authorize → call service(s) → return response. + +**Exception:** `UserManager` / `SignInManager` for ASP.NET Identity operations (login, password, claims) are allowed in controllers since Identity is a framework concern, not a domain service. -**Exceptions:** `UserManager` for ASP.NET Identity operations (login, password, claims) is allowed in controllers since Identity is a framework concern, not a domain service. +### 2b. Services Live in Application, Not Infrastructure -### 2b. Only Services Talk to the Database +Business services (`ProfileService`, `TeamService`, `BudgetService`, etc.) live in `Humans.Application`. They contain business rules, workflow logic, validation, and orchestration. They **never** import EF types. When they need to load or persist entities, they call their owning repository interface; when they need cached data, they go through their owning store. -All EF Core queries live in Infrastructure service implementations. If data needs to come from the database, it goes through a service method. +Repository **implementations** (the classes that talk to `DbContext`) live in `Humans.Infrastructure`. That is the only project that may touch EF Core. ### 2c. Table Ownership Is Strict and Sectional -Each service owns specific database tables. **No other service may query, insert, update, or delete rows in tables it does not own.** If `CampService` needs a profile, it calls `IProfileService` — it does not query the `profiles` table. +Each domain's tables are owned by exactly one service (and that service's repository). **No other service may query, insert, update, or delete rows in tables it does not own.** If `CampService` needs a profile, it calls `IProfileService` — it does not query the `profiles` table, does not instantiate `IProfileRepository`, does not read from `IProfileStore`. ### 2d. Cache Ownership Follows Data Ownership -The service that owns the data owns the cache for that data. Caching is an internal implementation detail of the owning service. Callers don't know whether data came from cache or DB — they call the service method and get the result. +Caching is an internal concern of the owning service. Callers don't know whether data came from memory, the store, or the database — they call the service method and get the result. The mechanism for caching is the **store pattern** (§4) and the **caching decorator** (§5), not raw `IMemoryCache` calls inlined in service methods. + +## 3. Repository Layer + +Every domain has a narrow, entity-shaped **repository interface** in `Humans.Application/Interfaces/Repositories/` and an EF-backed **implementation** in `Humans.Infrastructure/Repositories/`. The repository is the single point of EF access for its tables. + +### 3a. Repository Rules + +1. **Entities in, entities out.** Return types are `Profile`, `IReadOnlyList`, `IReadOnlyDictionary`, or scalar / id values. Never `IQueryable`, never EF types, never DTOs. +2. **No cross-domain method signatures.** A repository for the Profile domain never takes a `Team`, returns a `User`, or accepts a filter that requires joining another domain's table. If a caller needs a compound shape, a composer at the service layer stitches it from multiple repositories. +3. **Bulk-by-ids is first class.** Every repository exposes a `GetByIdsAsync(IReadOnlyCollection)` returning a dictionary. This is what makes in-memory joins (§6) cheap. +4. **`GetAllAsync` exists for store warmup.** At ~500 users it is trivial. Larger datasets would replace it with a streaming shape; at our scale it is strictly cheaper than lazy loading. +5. **No cross-domain navigation properties in return shapes.** `Profile.User` is a cross-domain nav — callers get the FK (`Profile.UserId`) and resolve via `IUserRepository` if they need the User. Aggregate-local navs (`Profile.Languages`) are fine. +6. **No logging of domain events, no audit, no `IClock`, no caching.** Just persistence. Side effects belong to the service. + +### 3b. Canonical Repository Shape + +```csharp +// Humans.Application/Interfaces/Repositories/IProfileRepository.cs +public interface IProfileRepository +{ + Task GetByIdAsync(Guid profileId, CancellationToken ct = default); + Task GetByUserIdAsync(Guid userId, CancellationToken ct = default); + Task> GetByUserIdsAsync( + IReadOnlyCollection userIds, CancellationToken ct = default); + + Task> GetAllAsync(CancellationToken ct = default); + + Task CountByTierAsync(MembershipTier tier, CancellationToken ct = default); + Task> GetUserIdsByBirthdayMonthAsync(int month, CancellationToken ct = default); + + Task AddAsync(Profile profile, CancellationToken ct = default); + Task UpdateAsync(Profile profile, CancellationToken ct = default); + Task DeleteAsync(Guid profileId, CancellationToken ct = default); +} +``` + +## 4. Store Pattern (In-Memory Entity Cache) + +Every cached domain has a **store** — a dedicated class that owns an in-memory canonical copy of its entities. The store is the *data shape* of the cache; it is separate from the decorator that makes reads transparent. + +### 4a. Store Rules + +1. **One store per domain.** `IProfileStore` holds the Profile world. `ITeamStore` holds the Team world. Stores do not share state. +2. **Canonical storage is a dictionary keyed by primary id** (`Dictionary`). Secondary indexes (e.g., `Dictionary` for email → id) are allowed when a specific lookup pattern justifies them; the store keeps them consistent because only the store writes. +3. **Single writer.** Only the owning service writes to the store, and only as part of a successful DB write. The store interface exposes `Upsert(entity)` and `Remove(id)`; the owning service calls these immediately after its repository write returns successfully. +4. **Startup warmup.** Each store loads its full domain on startup via `IProfileRepository.GetAllAsync()`. At ~500 users this is trivial memory and query cost; it eliminates cache-miss reasoning entirely. +5. **Stores are Infrastructure.** The interface lives in `Humans.Application/Interfaces/Stores/`, the implementation lives in `Humans.Infrastructure/Stores/`. + +### 4b. Why a Store, Not Inline `IMemoryCache.GetOrCreateAsync` + +The old pattern (`_cache.GetOrCreateAsync($"profile:{id}", ...)` inside a service method) caches *query results*, not entities. `GetById`, `GetByEmail`, and `GetByIds` become three independent cache entries for overlapping data, with three independent invalidation paths and three opportunities for staleness. Under the store pattern, all three are dict lookups over the same canonical `Profile` object, and invalidation is a single `Upsert` call in one place: the owning service's write method. + +## 5. Decorator Caching + +Services are cached by **wrapping them in a decorator**, not by inlining `IMemoryCache` calls. The decorator is registered via `services.Decorate()` (Scrutor). Callers inject `IProfileService` and get the cached version transparently. + +### 5a. Decorator Rules + +1. **One decorator per service.** `CachingProfileService : IProfileService` wraps the real `ProfileService`. +2. **Reads go through the store.** The decorator asks `IProfileStore` first. With startup warmup, every read is a hit at our scale. +3. **Writes pass through to the inner service.** The inner service writes to the repository and then updates the store. The decorator does not update the store itself — the service does, because only the service knows what the final entity state is after business rules run. +4. **Decorators contain zero business logic.** If the decorator needs to decide anything beyond "is it in the store?", that decision belongs in the service, not the wrapper. + +### 5b. The Full Stack + +``` +Controllers / other services + ↓ IProfileService +CachingProfileService (decorator) [Infrastructure] + ↓ IProfileService +ProfileService (business logic) [Application] + ↓ IProfileRepository, IProfileStore +ProfileRepository, ProfileStore [Infrastructure] + ↓ DbContext +HumansDbContext [Infrastructure] +``` + +Three roles, cleanly separated: +- **Repository** talks to EF, nothing else +- **Service** runs business rules and coordinates repository + store writes +- **Decorator** makes caching invisible to callers + +## 6. Cross-Domain Joins Are Forbidden + +**No EF query may `.Include()` or `.Join()` across a domain boundary.** A Profile query cannot navigate into User, Team, or Campaign. A Team query cannot navigate into Profile or User. A Campaign query cannot navigate into Team members. And so on. + +### 6a. Why + +Cross-domain joins couple caching and invalidation to the database because no single service owns the joined shape. Nothing upstream can safely cache a Team+Profile join; nothing upstream can safely invalidate it when either side changes. These joins are the single biggest structural barrier to the caching model in §4–§5, and they silently break the table-ownership rule in §2c because the joining service ends up reading columns it does not own. + +### 6b. In-Memory Joins Are the Replacement + +When a caller needs Team + Profile + User together, the caller (controller, page service, or composer service) asks each owning service for its slice and stitches in memory: + +```csharp +// In a controller or composer +var team = await _teamService.GetByIdAsync(teamId, ct); +var userIds = team.Members.Select(m => m.UserId).ToList(); +var profiles = await _profileService.GetByUserIdsAsync(userIds, ct); +var users = await _userService.GetByIdsAsync(userIds, ct); + +var rows = team.Members.Select(m => new TeamMemberRow( + UserId: m.UserId, + DisplayName: users[m.UserId].DisplayName, + BurnerName: profiles[m.UserId].BurnerName, + Role: m.Role)); +``` + +Three store reads, no SQL joins, cache ownership intact, each service cachable independently. -- Only the owning service may read/write/invalidate cache entries for its data. -- Other services and controllers must not access `IMemoryCache` with another service's cache keys. +### 6c. Cross-Domain Nav Properties -## 3. Table Ownership Map +Strip cross-domain navigation properties at the repository and entity boundary: -Each section's service owns these tables. Cross-service access goes through the service interface, never direct DB queries. +- ❌ `Profile.User` (nav to User entity in another domain) +- ✅ `Profile.UserId` (FK only) +- ❌ `TeamMember.User` (nav to User) +- ✅ `TeamMember.UserId` (FK only) +- ❌ `CampLead.User`, `ApplicationVote.BoardMember`, etc. +- ✅ The corresponding FKs +- ✅ `Profile.Languages` (aggregate-local collection, fine — same domain) + +### 6d. What You Give Up + +- **Server-side filter or sort on joined columns** (e.g., "teams ordered by coordinator's city"). At 500 users you filter and sort in memory — cheap. +- **Some EF LINQ elegance.** You write more `Dictionary` lookups and fewer `Include / ThenInclude` chains. + +### 6e. What You Gain + +- Cache ownership becomes tractable. Every domain owns its own store and its own invalidation. +- Every table has exactly one writer (its repository) and one cache (its store). +- Missing-`Include` bugs (lazy-load exceptions, over-fetching graphs) stop happening because there are no cross-domain navs to forget. +- The table-ownership rule finally has teeth at query time, not just at write time. + +## 7. Decorators vs In-Service Crosscuts + +Not every crosscut belongs in a decorator. The decorator pattern works only for concerns that are **mechanical and context-free** — where the wrapper does not need to know *who* is calling or *why*. + +| Concern | Pattern | Why | +|---|---|---| +| Caching | Decorator ✅ | Mechanical, context-free | +| Metrics / timing | Decorator ✅ | Mechanical, context-free | +| Retry / circuit breaker (external calls) | Decorator ✅ | Mechanical, context-free | +| Access logging (GDPR "who viewed what") | Decorator ✅ | Mechanical, context-free | +| **Domain audit** (suspended, approved, tier changed) | **In-service** | Needs actor, before/after state, semantic intent, same transaction as the write | +| **Authorization** | **In-controller** (resource-based handlers, §11) | Needs HTTP identity + resource context | +| **Transactions / unit of work** | **In-service** | Must wrap the whole business operation | + +### 7a. Audit Is In-Service + +Domain audit events — "user X suspended user Y for reason Z" — need the actor, the before/after state, the semantic intent, and must commit in the same unit of work as the mutation. A decorator wrapping `SuspendAsync(userId)` has none of that context: it does not know the actor (unless plumbed in), it does not know the old state (unless it re-reads, which is wasteful), and it cannot distinguish a name edit from a suspension from a tier change. + +Services call `IAuditLogService.LogAsync(...)` explicitly inside the business method: + +```csharp +public async Task SuspendAsync(Guid userId, Guid actorId, string reason, CancellationToken ct) +{ + var profile = await _repo.GetByUserIdAsync(userId, ct); + if (profile is null) return; + + var wasAlreadySuspended = profile.IsSuspended; + profile.IsSuspended = true; + await _repo.UpdateAsync(profile, ct); + _store.Upsert(profile); + + await _auditLog.LogAsync(new AuditEntry( + Actor: actorId, + Subject: userId, + Action: "ProfileSuspended", + Before: wasAlreadySuspended, + After: true, + Reason: reason), ct); +} +``` + +If audit calls become noisy across many methods inside one service, the next evolution is **domain events** raised from the entity and handled in Infrastructure — not a decorator. + +## 8. Table Ownership Map + +Each section's service owns these tables. Cross-service access goes through the service interface, never through direct DB queries, never through another domain's repository or store. | Section | Service(s) | Owned Tables | |---------|-----------|--------------| @@ -68,43 +252,51 @@ Each section's service owns these tables. Cross-service access goes through the See [`docs/architecture/dependency-graph.md`](dependency-graph.md) for the full directed dependency graph with current vs target edges and circular dependency analysis. -## 4. Cross-Service Communication +## 9. Cross-Service Communication -When a service needs data from another section, it calls that section's service interface via constructor injection. +When a service needs data from another section, it calls that section's public service interface via constructor injection. Repositories and stores are never crossed — only the public `I{Section}Service` interface. ```csharp -// CORRECT — CampService needs a profile, asks ProfileService -public class CampService : ICampService +// CORRECT — CampService needs profiles, asks ProfileService +public class CampService( + ICampRepository campRepository, + ICampStore campStore, + IProfileService profileService) : ICampService { - private readonly IProfileService _profileService; - - public async Task GetCampDetailAsync(Guid campId) + public async Task GetCampDetailAsync(Guid campId, CancellationToken ct) { - var camp = await _context.Camps.FindAsync(campId); - var leadProfiles = await _profileService.GetProfilesByIdsAsync(camp.LeadUserIds); - // ... - } -} + var camp = await campRepository.GetByIdAsync(campId, ct); + if (camp is null) return null; -// WRONG — CampService queries profiles table directly -public class CampService : ICampService -{ - public async Task GetCampDetailAsync(Guid campId) - { - var camp = await _context.Camps.FindAsync(campId); - var leads = await _context.Profiles.Where(p => leadIds.Contains(p.UserId)).ToListAsync(); - // ... + var leadProfiles = await profileService.GetByUserIdsAsync(camp.LeadUserIds, ct); + return BuildDto(camp, leadProfiles); } } ``` -### Cross-Service Contract Rules +Wrong patterns — each violates an invariant somewhere: -- Cross-service calls are by **ID or small parameter set** — `GetProfileAsync(Guid userId)`, not raw queries. -- Services return **DTOs or domain entities** — never `IQueryable` or EF query fragments. -- Circular dependencies are resolved by extracting a shared interface or using an orchestrating service (like `OnboardingService` orchestrating Profiles + Legal + Teams). +```csharp +// WRONG — reaches into another domain's repository +public class CampService(ICampRepository repo, IProfileRepository profileRepo) : ICampService { ... } + +// WRONG — reaches into another domain's store +public class CampService(ICampRepository repo, IProfileStore profileStore) : ICampService { ... } + +// WRONG — direct DbContext access (impossible by project graph once migrated) +public class CampService(HumansDbContext db) : ICampService { ... } -## 5. Cross-Cutting Services +// WRONG — cross-domain .Include +var camp = await db.Camps.Include(c => c.Leads).ThenInclude(l => l.Profile).FirstAsync(...); +``` + +### Rules + +- Cross-service calls are **by id or small parameter set** — `GetByUserIdAsync(Guid)`, `GetByIdsAsync(IReadOnlyCollection)`. Never a raw predicate that pushes another domain's schema knowledge into the caller. +- Services return **DTOs or domain entities** — never `IQueryable`, never cross-domain entity graphs. +- Circular dependencies are resolved by extracting a shared interface or using an orchestrating service (e.g., `OnboardingService` orchestrates Profiles + Legal + Teams). + +## 10. Cross-Cutting Services Some services are used across all sections. They own their own tables but are injected everywhere. @@ -115,9 +307,9 @@ Some services are used across all sections. They own their own tables but are in | `EmailOutboxService` | Queue and track transactional emails | `email_outbox_messages` | | `NotificationService` | In-app notifications | *(transient)* | -These are standalone services, not embedded in section services. Any service or controller can call `IAuditLogService.LogAsync(...)` to record an action, or `IRoleAssignmentService.HasActiveRoleAsync(...)` to check a role. +These are standalone services, not embedded in section services. Any service or controller can call `IAuditLogService.LogAsync(...)` to record an action, or `IRoleAssignmentService.HasActiveRoleAsync(...)` to check a role. They follow the same repository + store + decorator pattern as any other service. -## 6. Authorization Pattern +## 11. Authorization Pattern Authorization uses **ASP.NET Core resource-based authorization** — one pattern, everywhere. @@ -150,7 +342,7 @@ await _budgetService.DeleteLineItemAsync(id); - **Services are auth-free.** They don't check roles, don't inject `IHttpContextAccessor`, don't receive boolean privilege flags. Authorization happens before the service is called. - **New sections need a handler.** When adding a new section with resource-scoped auth, add a `*OperationRequirement` + `*AuthorizationHandler` pair. Don't invent a new pattern. -## 7. Immutable Entity Rules +## 12. Immutable Entity Rules Some entities are append-only. They have database triggers or application-level enforcement preventing UPDATE and DELETE. @@ -163,9 +355,9 @@ Some entities are append-only. They have database triggers or application-level | `ApplicationStateHistory` | `application_state_histories` | Append-only by convention | | `TeamJoinRequestStateHistory` | `team_join_request_state_histories` | Append-only by convention | -**Rule:** Never add UPDATE or DELETE logic for append-only entities. New state = new row. +**Rule:** Never add UPDATE or DELETE logic for append-only entities. New state = new row. Repository interfaces for these domains expose `AddAsync` and `GetX` methods but no `UpdateAsync` or `DeleteAsync`. -## 8. Google Resource Ownership +## 13. Google Resource Ownership All Google Drive resources are on **Shared Drives** (never My Drive). Google integration is managed by dedicated services: @@ -174,69 +366,43 @@ All Google Drive resources are on **Shared Drives** (never My Drive). Google int - `GoogleWorkspaceUserService` — user provisioning - `SyncSettingsService` — per-service sync mode (None/AddOnly/AddAndRemove) -**No other service queries Google resources directly.** If a section needs to know about a team's Google resources, it asks `ITeamResourceService`. - -## 9. DTO and ViewModel Boundary +**No other service queries Google resources directly.** If a section needs to know about a team's Google resources, it asks `ITeamResourceService`. The guardrail script `scripts/check-google-resource-ownership.sh` enforces this at CI time. -- **Services** return DTOs or domain entities to callers. -- **Controllers** map service results to ViewModels for Razor views. -- **Domain entities** should not leak into Razor views when a ViewModel would provide better separation. For simple cases where the entity maps 1:1 to the view, passing the entity is acceptable. -- **View Components** may inject services directly (they are part of the Web layer) but still follow the service gateway rule — they call services, not DbContext. +## 14. DTO and ViewModel Boundary -## 10. Cache Invalidation Responsibility +- **Domain entities** live in `Humans.Domain`. They are mutable, have identity, and carry invariants. Entities never reference EF types. +- **DTOs** live in `Humans.Application`. They are read-optimized shapes for specific use cases (admin tables, API responses, view data). Services return DTOs when the shape is call-specific and the entity does not match; they return entities when the caller needs the full aggregate. +- **ViewModels** live in `Humans.Web` (or are inlined in controllers). Controllers map DTOs or entities to view models for Razor. +- **Domain entities should not leak into Razor views** when a DTO would provide better separation. Simple 1:1 cases are acceptable; anything that would have required `.Include` for navigation in the old model is not. +- **View components** are part of Web. They call services, not repositories or stores. -The owning service decides when to invalidate its cache. Common patterns: - -```csharp -// ProfileService owns profile caching -public class ProfileService : IProfileService -{ - public async Task GetProfileAsync(Guid userId) - { - return await _cache.GetOrCreateAsync($"profile:{userId}", async entry => - { - entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(10); - return await LoadFromDb(userId); - }); - } - - public async Task UpdateProfileAsync(Guid userId, UpdateProfileDto dto) - { - // ... save to DB ... - _cache.Remove($"profile:{userId}"); // Owning service invalidates - } -} +## 15. Migration Strategy -// WRONG — CampService invalidating ProfileService's cache -public class CampService : ICampService -{ - private readonly IMemoryCache _cache; - - public async Task UpdateCampLeadAsync(...) - { - // ... - _cache.Remove($"profile:{leadUserId}"); // NOT YOUR CACHE - } -} -``` +This is the target. Existing code violates most of it. Migration is **per-domain, one at a time** — no big-bang rewrite. -If a cross-service operation requires cache invalidation, the owning service should expose a method for it (e.g., `IProfileService.InvalidateCacheAsync(Guid userId)`), or the invalidation should happen naturally when the owning service's write method is called. +### 15a. Migration Phases -## Migration Strategy +1. **Step 0 — Spike on Profile.** Land the full pattern on one domain to validate the shape: create `IProfileRepository` + `ProfileRepository`, create `IProfileStore` + `ProfileStore`, move `ProfileService` from `Humans.Infrastructure` to `Humans.Application`, create `CachingProfileService` decorator, wire DI. Verify build and smoke test. This is the architectural proof — if it feels wrong, bail before touching more domains. +2. **Step 1 — Quarantine direct access to the spiked domain.** Replace every `_dbContext.Profiles.*` call in other services with `IProfileService` calls. Delete every `.Include(x => x.Profile)` / `.Include(x => x.User)` in other domains; replace with in-memory stitching per §6b. At the end of this step, `DbContext.Profiles` is referenced in exactly one file: `ProfileRepository.cs`. +3. **Step 2 — Repeat per domain, highest-blast-radius first.** Priority order (driven by cross-domain `.Include` count and fan-in, not alphabet): User, Team, RoleAssignment, then Campaign, Application, Consent, then the long tail. +4. **Step 3 — Tail domains.** Audit log, outbox, sync settings, and other low-traffic domains get the full pattern for consistency. These are mechanical and low-risk. +5. **Step 4 — Structural enforcement.** Architecture test or CI grep that fails if (a) any service lives in `Humans.Infrastructure/Services/`, or (b) any file outside a `*Repository.cs` references `DbContext.`, or (c) any `.Include()` navigates across domain boundaries. -This is a target architecture. Existing code has violations. The migration approach: +### 15b. Migration Rules During the Transition -1. **New code must comply.** All new features, services, and controllers follow these rules from day one. -2. **Touch-and-clean.** When modifying an existing controller or service for any reason, clean up violations in the code you're touching. Don't scope-creep into unrelated files. -3. **Tech debt cleanup.** Systematic violation cleanup is tracked as tech debt. Section-by-section migration can be done as dedicated work. -4. **Don't break working code for purity.** If a refactor to comply would require significant changes across multiple files, create a GitHub issue instead of a risky inline fix. +1. **New code must comply.** New features use the target pattern from day one, even in domains that have not been migrated yet. That means creating the repository + store + decorator for a new domain the same day you create its service. +2. **Touch-and-clean within scope.** When modifying an existing service for unrelated reasons, don't scope-creep into a full repository migration. Fix the immediate bug; migrate the domain in a dedicated session. +3. **Don't half-migrate a domain.** If you start extracting `IProfileRepository`, finish the full stack (repo + store + decorator + caller updates) in one session. A half-migrated domain where some callers use the new service and others still `.Include()` directly is worse than either extreme. +4. **EF migration review still applies.** Schema changes still go through the EF migration reviewer agent — the repository layer does not change what migrations look like, just who calls them. -### Known Current Violations +### 15c. Known Current Violations (as of 2026-04-15) -Controllers with direct DbContext access (to be migrated): -- `AdminController` — queries DB directly for admin operations -- `ProfileController` — some direct DB queries -- `GoogleController` — direct DB access for sync operations -- `DevLoginController` — dev-only, low priority +- **43 services** live in `Humans.Infrastructure/Services/` and inject `HumansDbContext` directly. Target: 0 (all business services move to `Humans.Application`). +- **48 cross-domain `.Include()` calls** across **20 services**. Biggest offenders: `OnboardingService` (9), `GoogleWorkspaceSyncService` (4), `ApplicationDecisionService` (4), `FeedbackService` (4). Target: 0. +- **0 repositories** exist today. Target: one per domain (~20 total). +- **0 stores** exist today. Target: one per cached domain (~15–20). +- **Inline `IMemoryCache.GetOrCreateAsync`** scattered across services. Target: replaced by decorator + store pattern. +- **Cross-domain navigation properties** (`Profile.User`, `TeamMember.User`, `CampLead.User`, etc.) are used freely today. Target: stripped at the entity boundary, FK-only. -Cross-service table access violations are documented per-section in `docs/sections/`. +Controllers with direct DbContext access (violation of §2a, tracked separately): +- `AdminController`, `ProfileController`, `GoogleController`, `DevLoginController` (dev-only, low priority). From 2ebd14d1e7fbce80bbd97dc0af13bb828c28dc96 Mon Sep 17 00:00:00 2001 From: Peter Drier Date: Wed, 15 Apr 2026 17:05:30 +0200 Subject: [PATCH 2/4] Move architecture.md into docs/architecture/ as conventions.md 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) --- docs/{architecture.md => architecture/conventions.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/{architecture.md => architecture/conventions.md} (100%) diff --git a/docs/architecture.md b/docs/architecture/conventions.md similarity index 100% rename from docs/architecture.md rename to docs/architecture/conventions.md From 6221e43b77944e08395455fb74f963e2fbe0e1b5 Mon Sep 17 00:00:00 2001 From: Peter Drier Date: Wed, 15 Apr 2026 17:06:55 +0200 Subject: [PATCH 3/4] Trim conventions.md to cross-cutting rules; expand docs/README.md index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/README.md | 18 +- docs/architecture/conventions.md | 452 ++++++------------------------- 2 files changed, 104 insertions(+), 366 deletions(-) diff --git a/docs/README.md b/docs/README.md index f23f71fe..162ac1fc 100644 --- a/docs/README.md +++ b/docs/README.md @@ -69,11 +69,21 @@ Clean Architecture with four layers: ``` Web Controllers, Views, ViewModels -Application Interfaces, DTOs, Use Cases -Infrastructure EF Core, Services, Jobs +Application Interfaces, DTOs, Services (business logic), Use Cases +Infrastructure EF Core, Repositories, Stores, Caching Decorators, Jobs, Integrations Domain Entities, Enums, Value Objects ``` -Primary macro-level guidance lives in [architecture.md](architecture.md). +| Document | Description | +|----------|-------------| +| [Design Rules](architecture/design-rules.md) | Persistence, service ownership, repository / store / decorator pattern, cross-domain join ban, authorization, migration strategy | +| [Conventions](architecture/conventions.md) | Domain invariants, transactions, integration, time/config, rendering (Razor vs fetch), testing, exception rule, smell checklist | +| [Dependency Graph](architecture/dependency-graph.md) | Service-to-service dependency graph, current vs target edges, circular dependency analysis | +| [Data Model](architecture/data-model.md) | Entities, relationships, serialization notes | +| [Coding Rules](architecture/coding-rules.md) | NodaTime, JSON, enums, string comparisons, nav, magic strings | +| [Code Review Rules](architecture/code-review-rules.md) | Hard-reject rules for code review | +| [Service / Data Access Map](architecture/service-data-access-map.md) | Per-service table access inventory | +| [Code Analysis](architecture/code-analysis.md) | Analyzers, ReSharper configuration | +| [Maintenance Log](architecture/maintenance-log.md) | Recurring maintenance tasks and last-run dates | -See the [root CLAUDE.md](../CLAUDE.md) for build commands, coding rules, and project conventions. +See the [root CLAUDE.md](../CLAUDE.md) for build commands and project overview. diff --git a/docs/architecture/conventions.md b/docs/architecture/conventions.md index 7e22041d..94208cde 100644 --- a/docs/architecture/conventions.md +++ b/docs/architecture/conventions.md @@ -1,125 +1,12 @@ -# Humans Architecture +# App Conventions -Normative macro-level architecture guide for the application. +Cross-cutting conventions for how we build this app. -This document is intentionally opinionated. It describes the architecture that exists now, the direction we are steering it, and the default rules new code must follow. +Persistence, service boundaries, caching, repositories, layering, and authorization rules live in [`design-rules.md`](design-rules.md). This document covers everything else — domain invariants, transactions, integration, time and configuration, rendering, testing, exceptions, and the smell checklist used during code review. -Code that does not follow these rules should justify the exception in the PR or commit notes. +> **History:** This file was originally `docs/architecture.md`, a broader architecture position paper written 2026-04-03. As of 2026-04-15 the persistence / service-layer content was migrated into `design-rules.md` (which now holds the repository, store, and decorator doctrine), and this file was trimmed to the cross-cutting conventions that remain. See git history for the original. -## Current Architectural Shape - -Humans is a layered monolith: - -```text -Web Controllers, Razor views, view models, HTTP concerns -Application Interfaces, DTOs, request/result contracts -Infrastructure EF Core, service implementations, jobs, integrations -Domain Entities, enums, value objects, local invariants -``` - -That shape is correct for this project and should be preserved. - -We are not building: - -- microservices -- a generic platform -- a framework for future hypothetical apps -- distributed coordination machinery - -We are building one clear, maintainable product. - -## Architectural Direction - -The main existing smell is boundary leakage, not the top-level structure. - -The recurring problem today is that some controllers still do too much: - -- direct `HumansDbContext` usage -- direct `SaveChangesAsync()` -- query composition -- workflow orchestration -- business-rule enforcement - -Going forward, new code should move in the opposite direction: - -- thinner controllers -- clearer service boundaries -- business rules concentrated in services and domain methods -- persistence and integration work kept out of the web layer - -## Core Decisions - -### 1. Keep the layered monolith - -Do not introduce service boundaries, event-driven architectures, or mediator-heavy patterns without a concrete problem that the current layered model cannot handle cleanly. - -### 2. Use EF Core directly in infrastructure - -`HumansDbContext` is already the data access abstraction for this app. - -Default rule: - -- use EF Core directly inside infrastructure services - -Do not add by default: - -- generic repositories -- generic unit-of-work wrappers -- CQRS/MediatR plumbing -- command/query class hierarchies for ordinary app work - -Those patterns add indirection here more often than they add clarity. - -### 3. Controllers are adapters only - -Controllers own: - -- routes -- auth attributes -- model binding -- input shape validation tied to HTTP -- choosing view vs redirect vs JSON response -- mapping service results into view models -- user-facing success/error messages - -Controllers do not own: - -- persistence orchestration -- business workflows -- integration calls that change system state -- cross-entity invariants -- caching - -Strict default: - -- new controllers should not inject `HumansDbContext` -- new controllers should not call `SaveChangesAsync()` -- new controllers should not contain the main query for a screen beyond trivial glue logic -- new controllers should not read from or write to cache directly - -If a controller needs any of those, assume the code belongs in a service unless there is a very specific reason otherwise. - -### 4. Services own use cases - -A service method should correspond to a real business capability or a coherent read model. - -Good examples: - -- save a profile -- approve an application -- build a team page -- provision a workspace account -- sync ticket data - -Bad examples: - -- generic CRUD wrappers with no policy -- catch-all managers -- services that merely bounce through to EF without shaping behavior or a meaningful query contract - -When a controller action becomes more than thin request/response glue, move the work into a service. - -### 5. Domain owns local invariants +## Domain Owns Local Invariants If a rule is inherent to an entity state transition, it belongs on the entity or a domain-adjacent type. @@ -133,150 +20,11 @@ Continue that approach. Do not leave important workflow transitions as ad hoc property mutation if the entity itself can protect the invariant. -### 6. Application defines contracts, not mechanics - -`Application` should contain: - -- interfaces -- DTOs -- request/result records -- cross-layer contracts - -`Application` should not contain: - -- MVC concerns -- EF Core details -- `HttpContext` -- Razor view models -- provider SDK types leaking through the contract - -### 7. Infrastructure implements persistence and integration - -`Infrastructure` owns: - -- EF Core queries and writes -- service implementations -- jobs -- integration clients -- cache implementation -- metrics/logging hooks - -`Infrastructure` should be organized by business capability first, technical detail second. - -## Layer Rules - -### Domain - -Owns: - -- entities -- enums -- value objects -- local calculations -- state transition rules - -Must not own: - -- EF Core query logic -- HTTP concerns -- configuration access -- logging orchestration -- third-party SDK calls - -### Application - -Owns: - -- use-case contracts -- read/write request shapes -- result shapes crossing boundaries - -Must not own: - -- persistence mechanics -- view concerns -- startup wiring - -### Infrastructure - -Owns: - -- query composition -- data loading -- writes and `SaveChangesAsync()` -- outbox/integration behavior -- scheduled job execution -- cache population and invalidation - -Must not own: - -- Razor view models -- route decisions -- page-local UI branching - -### Web - -Owns: - -- controller entry points -- view models -- views -- API formatting -- redirects, status codes, temp-data messaging - -Must not own: - -- the source of truth for business rules -- database transaction boundaries -- cache ownership - -## Read Rules - -Default rule: - -- read queries belong in services, not controllers - -Use: - -- `AsNoTracking()` for read-only queries -- projection to DTOs/summary records for list/detail screens -- explicit ordering and filtering - -Avoid: - -- returning large tracked graphs to drive UI screens -- shaping major read models directly in controllers -- sprinkling ad hoc includes across the web layer - -Returning domain entities from a service is acceptable for narrow internal workflows. For page data and reports, prefer shaped results. - -## Write Rules - -Default rule: - -- the service that owns the use case owns persistence - -Use services to: - -- load required entities -- enforce rules -- mutate state -- write audit/outbox side effects -- call `SaveChangesAsync()` - -Avoid: - -- controllers mutating entities and flushing directly -- write workflows split between controller and service -- hidden side effects that occur outside the main use-case boundary - -If a workflow requires multiple persistence stages, that should be explicit and commented. It is an exception, not the default. - -## Transaction Rule +## Transaction Boundary The default transaction boundary is the service method handling the use case. -A controller action should usually call one primary mutation method, and that method should own the write boundary. +A controller action should usually call one primary mutation method, and that method should own the write boundary — including repository writes, store updates, audit log calls, and any outbox entries that must commit atomically with the primary mutation. Do not make the controller the coordinator of: @@ -288,41 +36,31 @@ Do not make the controller the coordinator of: That is service work. -## Caching Rules - -Caching is allowed. Controller-owned caching is not. - -Default rule: +## Caching -- caching belongs in infrastructure or service-level read paths +See [`design-rules.md`](design-rules.md) §4–§5 for the structured caching pattern (store + decorator). -Controllers must not: +Two controller-level rules still apply at this layer: -- populate cache entries -- invalidate cache entries -- decide cache lifetimes -- contain fallback logic like "read cache, else query db" +- Controllers must not populate, invalidate, or read cache entries for domain data. +- Controllers must not contain fallback logic like "read cache, else query db." That belongs in a service or its caching decorator. -Allowed caching patterns: +Do not add speculative caching because something "might be slow." At this project scale, clarity beats premature cache spread. -- stable read models -- lookup/reference datasets -- expensive aggregate summaries used by multiple entry points +## Authorization -Requirements for any cache: +See [`design-rules.md`](design-rules.md) §11 for the resource-based authorization pattern and handler inventory. -- clear ownership -- explicit invalidation path -- narrow, named purpose -- correctness if the cache is cold or empty +Two general rules apply at this layer: -If a cache can become stale, the write path that makes it stale must own invalidation. +1. Web boundary protection for routes/pages/API endpoints via `[Authorize]` attributes or resource-based authorization handlers. +2. Service/domain enforcement when violating the rule would create invalid state or bypass workflow policy — but service methods are otherwise auth-free; they trust the caller. -Do not add speculative caching because something "might be slow." At this project scale, clarity beats premature cache spread. +Do not rely on hidden buttons or view-only checks for anything important. -## Integration Rules +## Integration -External systems stay behind `Application` interfaces and `Infrastructure` implementations. +External systems stay behind `Humans.Application` interfaces and `Humans.Infrastructure` implementations. Do not leak raw provider concerns through multiple layers. @@ -330,21 +68,12 @@ Controller code should talk in product language, not vendor API language. Non-production stub implementations are preferred over scattered environment checks in business logic. -## Authorization Rules - -Authorization belongs in two places: - -1. Web boundary protection for routes/pages/API endpoints. -2. Service/domain enforcement when violating the rule would create invalid state or bypass workflow policy. - -Do not rely on hidden buttons or view-only checks for anything important. - -## Time and Configuration Rules +## Time and Configuration For time: - use `IClock` -- use NodaTime types +- use NodaTime types (`Instant`, `LocalDate`, etc.) - do not introduce new workflow logic based on `DateTime.UtcNow` For configuration: @@ -353,7 +82,56 @@ For configuration: - keep configuration access centralized - do not scatter raw environment-variable reads through feature code unless the existing pattern already requires it at composition time -## Testing Rules +## Rendering + +Server-rendered Razor is the default rendering approach for all pages. + +Default rule: + +- page content is rendered server-side using Razor views, tag helpers, and view components +- slow data loads use the partial-via-AJAX pattern: render the page frame server-side, load the slow section by fetching a Razor partial from an AJAX call + +Razor provides: + +- compile-time type safety +- tag helpers and `asp-*` route generation +- automatic HTML encoding (no manual `escapeHtml`) +- localization via `IStringLocalizer` +- view components for reusable data-fetching UI +- authorization tag helpers for role-based visibility + +Do not use client-side `fetch()` + JavaScript DOM construction to build page content when Razor can render the same output. That pattern requires manual HTML escaping, duplicated rendering logic, projection DTOs solely for JSON serialization, and string-based URL construction that breaks on route constraint changes. + +### Valid exceptions + +Client-side JavaScript with `fetch()` is appropriate for: + +- **Autocomplete/search inputs** that need instant feedback on keystrokes (profile search, member search, volunteer search, shift volunteer search) +- **Dynamic form field population** that responds to parent field changes (team Google resource dropdown) +- **Progressive enhancement** for inline actions that avoid full page reloads (notification dismiss/mark-read, feedback detail panel loading) +- **Utility behaviors** that are not page content (timezone detection, notification popup, profile popover on hover) + +These patterns use `fetch()` to enhance an already server-rendered page, not to replace server rendering entirely. + +### Current exceptions list + +All pages are server-rendered with Razor. The following use `fetch()` for the specific justified purposes listed above: + +| File | Purpose | Exception type | +|------|---------|----------------| +| `_HumanSearchInput.cshtml` | Profile autocomplete | Search input | +| `_MemberSearchScript.cshtml` | Member search autocomplete | Search input | +| `_VolunteerSearchScript.cshtml` | Volunteer search autocomplete | Search input | +| `_TeamGoogleAndParentFields.cshtml` | Google resource dropdown on team change | Dynamic form field | +| `ShiftAdmin/Index.cshtml` | Shift volunteer search + tag creation | Search input + inline action | +| `Notification/Index.cshtml` | Dismiss/mark-read without reload | Progressive enhancement | +| `Feedback/Index.cshtml` | Master-detail panel loading | Progressive enhancement | +| `Google/Sync.cshtml` | Tab content loaded via Razor partial (slow Google API) | Partial-via-AJAX | +| `site.js` | Timezone, notification popup, profile popover | Utility | + +When adding a new page that needs client-side data loading, add it to this list with justification. If a page has no entry here, it must be server-rendered. + +## Testing This project should test behavior primarily at the service boundary. @@ -378,7 +156,7 @@ Do not default to e2e when a service test would cover the rule more directly. ## Exception Rule -Exceptions are allowed, but the burden is on the exception. +Exceptions to any rule in this doc or in `design-rules.md` are allowed, but the burden is on the exception. An exception should state: @@ -391,6 +169,7 @@ Weak reasons: - "it was faster" - "the controller already had the db context" - "making a service felt heavy" +- "adding a repository felt like over-engineering" Stronger reasons: @@ -402,76 +181,25 @@ Stronger reasons: Stop and reconsider when a change introduces any of these: +**Web layer smells:** - controller injects `HumansDbContext` - controller calls `SaveChangesAsync()` - controller owns cache logic - controller contains the only enforcement of a business rule - query logic for a major screen lives in the web layer -- a new abstraction exists only to wrap EF mechanically -- a provider SDK type leaks across multiple layers -- a cache is added without a clear invalidation owner -- a job re-implements a workflow that should be in a service - -## Rendering Rules -Server-rendered Razor is the default rendering approach for all pages. - -Default rule: - -- page content is rendered server-side using Razor views, tag helpers, and view components -- slow data loads use the partial-via-AJAX pattern: render the page frame server-side, load the slow section by fetching a Razor partial from an AJAX call - -Razor provides: - -- compile-time type safety -- tag helpers and `asp-*` route generation -- automatic HTML encoding (no manual `escapeHtml`) -- localization via `IStringLocalizer` -- view components for reusable data-fetching UI -- authorization tag helpers for role-based visibility - -Do not use client-side `fetch()` + JavaScript DOM construction to build page content when Razor can render the same output. That pattern requires manual HTML escaping, duplicated rendering logic, projection DTOs solely for JSON serialization, and string-based URL construction that breaks on route constraint changes. - -### Valid exceptions - -Client-side JavaScript with `fetch()` is appropriate for: - -- **Autocomplete/search inputs** that need instant feedback on keystrokes (profile search, member search, volunteer search, shift volunteer search) -- **Dynamic form field population** that responds to parent field changes (team Google resource dropdown) -- **Progressive enhancement** for inline actions that avoid full page reloads (notification dismiss/mark-read, feedback detail panel loading) -- **Utility behaviors** that are not page content (timezone detection, notification popup, profile popover on hover) - -These patterns use `fetch()` to enhance an already server-rendered page, not to replace server rendering entirely. - -### Current exceptions list - -All pages are server-rendered with Razor. The following use `fetch()` for the specific justified purposes listed above: - -| File | Purpose | Exception type | -|------|---------|----------------| -| `_HumanSearchInput.cshtml` | Profile autocomplete | Search input | -| `_MemberSearchScript.cshtml` | Member search autocomplete | Search input | -| `_VolunteerSearchScript.cshtml` | Volunteer search autocomplete | Search input | -| `_TeamGoogleAndParentFields.cshtml` | Google resource dropdown on team change | Dynamic form field | -| `ShiftAdmin/Index.cshtml` | Shift volunteer search + tag creation | Search input + inline action | -| `Notification/Index.cshtml` | Dismiss/mark-read without reload | Progressive enhancement | -| `Feedback/Index.cshtml` | Master-detail panel loading | Progressive enhancement | -| `Google/Sync.cshtml` | Tab content loaded via Razor partial (slow Google API) | Partial-via-AJAX | -| `site.js` | Timezone, notification popup, profile popover | Utility | - -When adding a new page that needs client-side data loading, add it to this list with justification. If a page has no entry here, it must be server-rendered. - -## Direction of Travel - -We do not need a rewrite. - -We do need steady pressure toward: - -- no new db-heavy controllers -- no new controller-owned caching -- fewer mixed controller/service workflows -- richer domain methods for workflow-heavy entities -- clearer service-owned read models -- tests that move with behavior changes +**Service / persistence smells:** +- a new service placed in `Humans.Infrastructure/Services/` instead of `Humans.Application/Services/` +- a service that injects `HumansDbContext` directly instead of going through its owning repository +- a service that injects another domain's repository or store (should call the other domain's `I{Section}Service` interface instead) +- a `.Include()` that navigates across a domain boundary (Profile → User, Team → Profile, Camp → Profile, etc.) +- a repository method that takes or returns another domain's type +- a repository method that returns `IQueryable` +- inline `IMemoryCache.GetOrCreateAsync` inside a service method instead of a store + decorator +- a cache is added without a clear invalidation owner +- a cross-domain nav property being added to an entity (e.g., `TeamMember.User`) -If a change makes the business rules more local, the web layer thinner, and the write/read ownership clearer, it is moving in the right direction. +**Cross-cutting smells:** +- a provider SDK type leaks across multiple layers +- a job re-implements a workflow that should be in a service +- audit logging implemented as a decorator instead of an in-service call (audit needs actor + before/after + same transaction) From e5da8f036853bbd24238d5314ff2e73cda9aafe9 Mon Sep 17 00:00:00 2001 From: Peter Drier Date: Wed, 15 Apr 2026 17:32:40 +0200 Subject: [PATCH 4/4] Unwind Phase 3a service-layer auth, tombstone Phase 3 of auth plan 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 (#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 (#419, Google sync) was merged (1626098) and reverted (bbbe508) for the same reason. - Phase 3c (#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) --- ...03-first-class-authorization-transition.md | 49 +++++++---- .../RoleAssignmentAuthorizationHandler.cs | 14 +--- .../Authorization/SystemPrincipal.cs | 44 ---------- .../Interfaces/IRoleAssignmentService.cs | 12 +-- .../Services/RoleAssignmentService.cs | 36 +------- .../Requirements/TeamAuthorizationHandler.cs | 20 ++--- .../RoleAssignmentClaimsTransformation.cs | 13 --- .../Controllers/ProfileController.cs | 41 +++++++--- ...RoleAssignmentAuthorizationHandlerTests.cs | 12 --- .../TeamAuthorizationHandlerTests.cs | 6 +- .../Services/RoleAssignmentServiceTests.cs | 82 +------------------ .../Services/TeamRoleServiceTests.cs | 2 - .../Services/TeamServiceTests.cs | 2 - 13 files changed, 73 insertions(+), 260 deletions(-) delete mode 100644 src/Humans.Application/Authorization/SystemPrincipal.cs diff --git a/docs/plans/2026-04-03-first-class-authorization-transition.md b/docs/plans/2026-04-03-first-class-authorization-transition.md index aaa1199d..e6c57634 100644 --- a/docs/plans/2026-04-03-first-class-authorization-transition.md +++ b/docs/plans/2026-04-03-first-class-authorization-transition.md @@ -113,27 +113,40 @@ This is a mechanical refactor done section by section. Each section is one PR. --- -## Phase 3: Service-Layer Enforcement +## Phase 3: Service-Layer Enforcement — **CANCELLED** -**Objective:** Push authorization checks down to service boundaries for sensitive mutations, so they're protected regardless of call path (controller, background job, future API). +> **Status: tombstoned.** Do not reopen without reading this section. The superseding rule is `docs/architecture/design-rules.md` §11: services are auth-free. -**Do this incrementally** — as new mutation paths are added or existing ones are refactored. +**Original objective:** Push authorization checks down to service boundaries for sensitive mutations, so they're protected regardless of call path (controller, background job, future API). -**First candidates:** -- Role assignment / removal -- Google sync actions with external side effects -- Budget mutations -- Privileged onboarding/review actions +**Why it was cancelled:** -**What changes:** +1. **Phase 3a / #418 (role assignment, PR #210)** shipped and broke QA within two days. The cycle `TeamService → RoleAssignmentService → IAuthorizationService → TeamAuthorizationHandler → ITeamService` crashed DI validation on startup. Fixed in commit `225ac14` by making `TeamAuthorizationHandler` lazily resolve `ITeamService` via `IServiceProvider` — i.e., a service-locator escape hatch that hides the cycle from the validator rather than removing it. Hazard preserved, lesson ignored. +2. **Phase 3b / #419 (Google sync)** was merged as `1626098` and reverted in `bbbe508` for the same reason. +3. **Phase 3c / #420 (budget mutations)** was drafted on branch `sprint/20260415/batch-4` but never merged. Closed as *won't do* on 2026-04-15. -- Service methods accept the current `ClaimsPrincipal` (or an authorization context) and call `IAuthorizationService` before executing -- Controllers become thin — they authorize via attribute and delegate to the service -- Services that are also called from background jobs use a system-level authorization context +Two out of three attempts ended in crash or revert. The pattern has zero clean wins on this codebase. -**Exit criteria:** -- Sensitive workflows are protected even if later reused from jobs, APIs, or alternate UI surfaces -- Controllers don't duplicate authorization logic that services already enforce +**Why service-layer enforcement is unnecessary at our scale:** + +The defence-in-depth argument for Phase 3 was "protect mutations regardless of call path — controllers, background jobs, future APIs." In practice on Humans: + +- We have **one UI** and no public API. "Future API" is speculative; service-layer enforcement cannot be justified by a caller that does not exist. +- Background jobs that mutate state (`TicketingBudgetSyncJob`, `SystemTeamSyncJob`) are trusted server-side code and do not need to authenticate against their own domain's auth handlers. +- Controllers are the only human-facing mutation path. Enforcing auth there (via `[Authorize]` + resource-based `IAuthorizationService.AuthorizeAsync`) covers 100% of the real threat surface. + +**The superseding pattern (from `design-rules.md` §11):** + +```csharp +// Controller — authorize, then call service +var authResult = await _authorizationService.AuthorizeAsync(User, category, BudgetOperationRequirement.Edit); +if (!authResult.Succeeded) return Forbid(); +await _budgetService.DeleteLineItemAsync(id); +``` + +Resource-based handlers (`BudgetAuthorizationHandler`, `RoleAssignmentAuthorizationHandler`, etc.) still exist and are still the right shape — they just get invoked from controllers, not from inside services. Services remain auth-free, do not inject `IAuthorizationService`, do not accept `ClaimsPrincipal` parameters, do not use `SystemPrincipal`. + +**Unwind PR:** Phase 3a's in-service auth was reverted in a follow-up PR that removed the `ClaimsPrincipal` parameter from `IRoleAssignmentService`, moved the `AuthorizeAsync` call to `ProfileController`, deleted `SystemPrincipal`, and removed the `IServiceProvider` hack from `TeamAuthorizationHandler`. The `RoleAssignmentAuthorizationHandler` and `RoleAssignmentOperationRequirement` remain — they are now called from the controller, which is the correct pattern. --- @@ -144,11 +157,11 @@ This is a mechanical refactor done section by section. Each section is one PR. | Phase 0 | PR #116 merged | 1 PR — inventory doc | | Phase 1 | Phase 0 complete | ~8 section PRs, mechanical | | Phase 2 | Feature needs object-relative auth | 1 PR for the vertical slice | -| Phase 3 | Ongoing as mutations are added/refactored | Incremental | +| ~~Phase 3~~ | **Cancelled — see Phase 3 section above** | — | Phase 0 is the first issue created. Phase 1 is done section-by-section across sprint batches — not all at once. Each section PR is small and low-risk. -Phases 2 and 3 are driven by real feature work, not scheduled proactively. +Phase 2 is driven by real feature work, not scheduled proactively. Phase 3 is cancelled; any future proposal to push auth into services must update `design-rules.md` §11 first. --- @@ -171,4 +184,4 @@ Each phase step creates one GitHub issue. The exit gate of each issue includes c **Phase 2 issue:** created when a feature needs resource-based auth -**Phase 3 issues:** created per-service as mutations are added +**~~Phase 3 issues~~:** cancelled. #418 shipped and required hazard-hiding hotfix `225ac14`. #419 merged and reverted (`bbbe508`). #420 closed *won't do* on 2026-04-15. See the Phase 3 tombstone above for rationale. diff --git a/src/Humans.Application/Authorization/RoleAssignmentAuthorizationHandler.cs b/src/Humans.Application/Authorization/RoleAssignmentAuthorizationHandler.cs index b91a816a..4cb1093e 100644 --- a/src/Humans.Application/Authorization/RoleAssignmentAuthorizationHandler.cs +++ b/src/Humans.Application/Authorization/RoleAssignmentAuthorizationHandler.cs @@ -5,12 +5,8 @@ namespace Humans.Application.Authorization; /// /// Resource-based authorization handler for role assignment operations. -/// Evaluates whether a user can assign or end a specific role. -/// -/// Authorization logic: -/// - Admin: can manage any role -/// - Board or HumanAdmin: can manage roles in RoleNames.BoardManageableRoles -/// - System principal: can manage any role (for background jobs) +/// - Admin: manage any role +/// - Board or HumanAdmin: manage roles in RoleNames.BoardManageableRoles /// - Everyone else: deny /// public class RoleAssignmentAuthorizationHandler : AuthorizationHandler @@ -28,12 +24,6 @@ protected override Task HandleRequirementAsync( return Task.CompletedTask; } - if (SystemPrincipal.IsSystem(context.User)) - { - context.Succeed(requirement); - return Task.CompletedTask; - } - if (context.User.IsInRole(RoleNames.Board) || context.User.IsInRole(RoleNames.HumanAdmin)) { if (BoardManageableRoles.Contains(roleName)) diff --git a/src/Humans.Application/Authorization/SystemPrincipal.cs b/src/Humans.Application/Authorization/SystemPrincipal.cs deleted file mode 100644 index 20adf175..00000000 --- a/src/Humans.Application/Authorization/SystemPrincipal.cs +++ /dev/null @@ -1,44 +0,0 @@ -using System.Security.Claims; - -namespace Humans.Application.Authorization; - -/// -/// Provides a system-level ClaimsPrincipal for background jobs and automated processes -/// that need to call service methods requiring authorization context. -/// The system principal has a special "System" claim that authorization handlers -/// can check to bypass role-based checks. -/// -public static class SystemPrincipal -{ - /// - /// Claim type used to identify the system principal. - /// - public const string SystemClaimType = "Humans.System"; - - /// - /// A singleton system principal for use by background jobs and automated processes. - /// - public static ClaimsPrincipal Instance { get; } = CreateSystemPrincipal(); - - /// - /// Checks whether the given principal is the system principal. - /// - public static bool IsSystem(ClaimsPrincipal principal) - { - return principal.HasClaim(c => - string.Equals(c.Type, SystemClaimType, StringComparison.Ordinal) && - string.Equals(c.Value, "true", StringComparison.Ordinal)); - } - - private static ClaimsPrincipal CreateSystemPrincipal() - { - var claims = new[] - { - new Claim(ClaimTypes.Name, "System"), - new Claim(SystemClaimType, "true") - }; - - var identity = new ClaimsIdentity(claims, "SystemAuth"); - return new ClaimsPrincipal(identity); - } -} diff --git a/src/Humans.Application/Interfaces/IRoleAssignmentService.cs b/src/Humans.Application/Interfaces/IRoleAssignmentService.cs index a9b405d4..4d54107c 100644 --- a/src/Humans.Application/Interfaces/IRoleAssignmentService.cs +++ b/src/Humans.Application/Interfaces/IRoleAssignmentService.cs @@ -1,18 +1,10 @@ -using System.Security.Claims; using Humans.Domain.Entities; using NodaTime; namespace Humans.Application.Interfaces; -/// -/// Service for role assignment queries and mutations. -/// public interface IRoleAssignmentService { - /// - /// Checks whether the proposed role window overlaps any existing window - /// for the same user and role. - /// Task HasOverlappingAssignmentAsync( Guid userId, string roleName, @@ -30,12 +22,12 @@ Task HasOverlappingAssignmentAsync( Task AssignRoleAsync( Guid userId, string roleName, Guid assignerId, - string? notes, ClaimsPrincipal principal, + string? notes, CancellationToken ct = default); Task EndRoleAsync( Guid assignmentId, Guid enderId, - string? notes, ClaimsPrincipal principal, + string? notes, CancellationToken ct = default); /// diff --git a/src/Humans.Infrastructure/Services/RoleAssignmentService.cs b/src/Humans.Infrastructure/Services/RoleAssignmentService.cs index 6da0eb4d..dfc93a00 100644 --- a/src/Humans.Infrastructure/Services/RoleAssignmentService.cs +++ b/src/Humans.Infrastructure/Services/RoleAssignmentService.cs @@ -1,10 +1,7 @@ -using System.Security.Claims; -using Microsoft.AspNetCore.Authorization; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Logging; using NodaTime; -using Humans.Application.Authorization; using Humans.Application.Extensions; using Humans.Application.Interfaces; using Humans.Domain.Constants; @@ -14,17 +11,12 @@ namespace Humans.Infrastructure.Services; -/// -/// Role assignment validation/query service. -/// Mutations enforce authorization at the service boundary via IAuthorizationService. -/// public class RoleAssignmentService : IRoleAssignmentService { private readonly HumansDbContext _dbContext; private readonly IAuditLogService _auditLogService; private readonly INotificationService _notificationService; private readonly ISystemTeamSync _systemTeamSyncJob; - private readonly IAuthorizationService _authorizationService; private readonly IClock _clock; private readonly IMemoryCache _cache; private readonly ILogger _logger; @@ -34,7 +26,6 @@ public RoleAssignmentService( IAuditLogService auditLogService, INotificationService notificationService, ISystemTeamSync systemTeamSyncJob, - IAuthorizationService authorizationService, IClock clock, IMemoryCache cache, ILogger logger) @@ -43,7 +34,6 @@ public RoleAssignmentService( _auditLogService = auditLogService; _notificationService = notificationService; _systemTeamSyncJob = systemTeamSyncJob; - _authorizationService = authorizationService; _clock = clock; _cache = cache; _logger = logger; @@ -129,20 +119,9 @@ public async Task> GetByUserIdAsync(Guid userId, C public async Task AssignRoleAsync( Guid userId, string roleName, Guid assignerId, - string? notes, ClaimsPrincipal principal, + string? notes, CancellationToken ct = default) { - var authResult = await _authorizationService.AuthorizeAsync( - principal, roleName, RoleAssignmentOperationRequirement.Manage); - - if (!authResult.Succeeded) - { - _logger.LogWarning( - "Authorization denied for role assignment: principal {Principal} attempted to assign role {Role} to user {UserId}", - principal.Identity?.Name, roleName, userId); - return new OnboardingResult(false, "Unauthorized"); - } - var now = _clock.GetCurrentInstant(); var hasOverlap = await HasOverlappingAssignmentAsync(userId, roleName, now, cancellationToken: ct); @@ -201,7 +180,7 @@ await _notificationService.SendAsync( public async Task EndRoleAsync( Guid assignmentId, Guid enderId, - string? notes, ClaimsPrincipal principal, + string? notes, CancellationToken ct = default) { var roleAssignment = await _dbContext.RoleAssignments @@ -213,17 +192,6 @@ public async Task EndRoleAsync( return new OnboardingResult(false, "NotFound"); } - var authResult = await _authorizationService.AuthorizeAsync( - principal, roleAssignment.RoleName, RoleAssignmentOperationRequirement.Manage); - - if (!authResult.Succeeded) - { - _logger.LogWarning( - "Authorization denied for ending role: principal {Principal} attempted to end role {Role} for user {UserId}", - principal.Identity?.Name, roleAssignment.RoleName, roleAssignment.UserId); - return new OnboardingResult(false, "Unauthorized"); - } - var now = _clock.GetCurrentInstant(); if (!roleAssignment.IsActive(now)) diff --git a/src/Humans.Web/Authorization/Requirements/TeamAuthorizationHandler.cs b/src/Humans.Web/Authorization/Requirements/TeamAuthorizationHandler.cs index 77027f22..05ba6949 100644 --- a/src/Humans.Web/Authorization/Requirements/TeamAuthorizationHandler.cs +++ b/src/Humans.Web/Authorization/Requirements/TeamAuthorizationHandler.cs @@ -7,24 +7,17 @@ namespace Humans.Web.Authorization.Requirements; /// /// Resource-based authorization handler for team management operations. -/// Evaluates whether a user can perform coordinator/management operations on a specific Team. -/// -/// Authorization logic: -/// - Admin: allow any team -/// - TeamsAdmin: allow any team +/// - Admin / TeamsAdmin: allow any team /// - Team coordinator (or parent department coordinator): allow only their team /// - Everyone else: deny -/// -/// Uses IServiceProvider to lazily resolve ITeamService, breaking the DI cycle: -/// TeamService → RoleAssignmentService → IAuthorizationService → TeamAuthorizationHandler → ITeamService /// public class TeamAuthorizationHandler : AuthorizationHandler { - private readonly IServiceProvider _serviceProvider; + private readonly ITeamService _teamService; - public TeamAuthorizationHandler(IServiceProvider serviceProvider) + public TeamAuthorizationHandler(ITeamService teamService) { - _serviceProvider = serviceProvider; + _teamService = teamService; } protected override async Task HandleRequirementAsync( @@ -32,20 +25,17 @@ protected override async Task HandleRequirementAsync( TeamOperationRequirement requirement, Team resource) { - // Admin and TeamsAdmin can manage any team if (RoleChecks.IsTeamsAdminBoardOrAdmin(context.User)) { context.Succeed(requirement); return; } - // Check if user is a coordinator for this specific team (or its parent department) var userIdClaim = context.User.FindFirst(ClaimTypes.NameIdentifier); if (userIdClaim is null || !Guid.TryParse(userIdClaim.Value, out var userId)) return; - var teamService = _serviceProvider.GetRequiredService(); - if (await teamService.IsUserCoordinatorOfTeamAsync(resource.Id, userId)) + if (await _teamService.IsUserCoordinatorOfTeamAsync(resource.Id, userId)) { context.Succeed(requirement); } diff --git a/src/Humans.Web/Authorization/RoleAssignmentClaimsTransformation.cs b/src/Humans.Web/Authorization/RoleAssignmentClaimsTransformation.cs index 564ed67b..ae5c28e2 100644 --- a/src/Humans.Web/Authorization/RoleAssignmentClaimsTransformation.cs +++ b/src/Humans.Web/Authorization/RoleAssignmentClaimsTransformation.cs @@ -4,7 +4,6 @@ using Microsoft.Extensions.Caching.Memory; using NodaTime; using Humans.Application; -using Humans.Application.Authorization; using Humans.Domain.Constants; using Humans.Infrastructure.Data; @@ -51,18 +50,6 @@ public async Task TransformAsync(ClaimsPrincipal principal) return principal; } - // Defensive: the SystemPrincipal claim grants background-job authorization bypass in - // RoleAssignmentAuthorizationHandler. It must never appear on a real user's identity, - // even if an external IdP somehow emits it. Strip any leaked instances here. - foreach (var userIdentity in principal.Identities) - { - var systemClaims = userIdentity.FindAll(SystemPrincipal.SystemClaimType).ToList(); - foreach (var systemClaim in systemClaims) - { - userIdentity.RemoveClaim(systemClaim); - } - } - var userIdClaim = principal.FindFirst(ClaimTypes.NameIdentifier); if (userIdClaim is null || !Guid.TryParse(userIdClaim.Value, out var userId)) { diff --git a/src/Humans.Web/Controllers/ProfileController.cs b/src/Humans.Web/Controllers/ProfileController.cs index b6434120..73d224c5 100644 --- a/src/Humans.Web/Controllers/ProfileController.cs +++ b/src/Humans.Web/Controllers/ProfileController.cs @@ -3,6 +3,7 @@ using System.ComponentModel.DataAnnotations; using System.Globalization; using System.Web; +using Humans.Application.Authorization; using Humans.Application.Configuration; using Humans.Application.Extensions; using Microsoft.AspNetCore.Authorization; @@ -48,6 +49,7 @@ public class ProfileController : HumansControllerBase private readonly ITicketQueryService _ticketQueryService; private readonly IMemoryCache _cache; private readonly IClock _clock; + private readonly IAuthorizationService _authorizationService; private const int MaxProfilePictureUploadBytes = 20 * 1024 * 1024; // 20MB upload limit private static readonly HashSet AllowedImageContentTypes = new(StringComparer.OrdinalIgnoreCase) @@ -92,7 +94,8 @@ public ProfileController( HumansDbContext dbContext, ITicketQueryService ticketQueryService, IMemoryCache cache, - IClock clock) + IClock clock, + IAuthorizationService authorizationService) : base(userManager) { _userManager = userManager; @@ -115,6 +118,7 @@ public ProfileController( _ticketQueryService = ticketQueryService; _cache = cache; _clock = clock; + _authorizationService = authorizationService; } // ─── Own Profile (Me) ──────────────────────────────────────────── @@ -1501,16 +1505,21 @@ public async Task AddRole(Guid id, CreateRoleAssignmentViewModel return Unauthorized(); } + var authResult = await _authorizationService.AuthorizeAsync( + User, model.RoleName, RoleAssignmentOperationRequirement.Manage); + if (!authResult.Succeeded) + { + _logger.LogWarning( + "Authorization denied for role assignment: principal {Principal} attempted to assign role {Role} to user {UserId}", + User.Identity?.Name, model.RoleName, id); + return Forbid(); + } + var result = await _roleAssignmentService.AssignRoleAsync( - id, model.RoleName, currentUser.Id, model.Notes, User); + id, model.RoleName, currentUser.Id, model.Notes); if (!result.Success) { - if (string.Equals(result.ErrorKey, "Unauthorized", StringComparison.Ordinal)) - { - return Forbid(); - } - SetError(string.Format(_localizer["Admin_RoleAlreadyActive"].Value, model.RoleName)); return RedirectToAction(nameof(AdminDetail), new { id }); } @@ -1528,6 +1537,7 @@ public async Task EndRole(Guid id, Guid roleId, string? notes) if (roleAssignment is null) { + // Return NotFound rather than Unauthorized to prevent role-assignment enumeration. return NotFound(); } @@ -1537,16 +1547,21 @@ public async Task EndRole(Guid id, Guid roleId, string? notes) return Unauthorized(); } + var authResult = await _authorizationService.AuthorizeAsync( + User, roleAssignment.RoleName, RoleAssignmentOperationRequirement.Manage); + if (!authResult.Succeeded) + { + _logger.LogWarning( + "Authorization denied for ending role: principal {Principal} attempted to end role {Role} for user {UserId}", + User.Identity?.Name, roleAssignment.RoleName, roleAssignment.UserId); + return NotFound(); + } + var result = await _roleAssignmentService.EndRoleAsync( - roleId, currentUser.Id, notes, User); + roleId, currentUser.Id, notes); if (!result.Success) { - if (string.Equals(result.ErrorKey, "Unauthorized", StringComparison.Ordinal)) - { - return Forbid(); - } - SetError(_localizer["Admin_RoleNotActive"].Value); return RedirectToAction(nameof(AdminDetail), new { id = roleAssignment.UserId }); } diff --git a/tests/Humans.Application.Tests/Authorization/RoleAssignmentAuthorizationHandlerTests.cs b/tests/Humans.Application.Tests/Authorization/RoleAssignmentAuthorizationHandlerTests.cs index 6f53f562..03f3db49 100644 --- a/tests/Humans.Application.Tests/Authorization/RoleAssignmentAuthorizationHandlerTests.cs +++ b/tests/Humans.Application.Tests/Authorization/RoleAssignmentAuthorizationHandlerTests.cs @@ -38,18 +38,6 @@ public async Task Admin_CanManageAnyRole(string roleName) result.Should().BeTrue(); } - // --- System principal override --- - - [Theory] - [InlineData(RoleNames.Admin)] - [InlineData(RoleNames.Board)] - [InlineData(RoleNames.TeamsAdmin)] - public async Task SystemPrincipal_CanManageAnyRole(string roleName) - { - var result = await EvaluateAsync(SystemPrincipal.Instance, roleName); - result.Should().BeTrue(); - } - // --- Board access --- [Theory] diff --git a/tests/Humans.Application.Tests/Authorization/TeamAuthorizationHandlerTests.cs b/tests/Humans.Application.Tests/Authorization/TeamAuthorizationHandlerTests.cs index d7861a6b..23b431e2 100644 --- a/tests/Humans.Application.Tests/Authorization/TeamAuthorizationHandlerTests.cs +++ b/tests/Humans.Application.Tests/Authorization/TeamAuthorizationHandlerTests.cs @@ -5,7 +5,6 @@ using Humans.Domain.Entities; using Humans.Web.Authorization.Requirements; using Microsoft.AspNetCore.Authorization; -using Microsoft.Extensions.DependencyInjection; using NSubstitute; using Xunit; @@ -27,10 +26,7 @@ public sealed class TeamAuthorizationHandlerTests public TeamAuthorizationHandlerTests() { - var services = new ServiceCollection(); - services.AddSingleton(_teamService); - var serviceProvider = services.BuildServiceProvider(); - _handler = new TeamAuthorizationHandler(serviceProvider); + _handler = new TeamAuthorizationHandler(_teamService); _teamService.IsUserCoordinatorOfTeamAsync(CoordinatorTeamId, UserId) .Returns(true); diff --git a/tests/Humans.Application.Tests/Services/RoleAssignmentServiceTests.cs b/tests/Humans.Application.Tests/Services/RoleAssignmentServiceTests.cs index 6d2badfc..30f47c57 100644 --- a/tests/Humans.Application.Tests/Services/RoleAssignmentServiceTests.cs +++ b/tests/Humans.Application.Tests/Services/RoleAssignmentServiceTests.cs @@ -1,13 +1,10 @@ -using System.Security.Claims; using AwesomeAssertions; -using Microsoft.AspNetCore.Authorization; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Logging.Abstractions; using NodaTime; using NodaTime.Testing; using Humans.Application; -using Humans.Application.Authorization; using Humans.Infrastructure.Data; using Humans.Infrastructure.Services; using Humans.Domain.Entities; @@ -23,7 +20,6 @@ public class RoleAssignmentServiceTests : IDisposable private readonly FakeClock _clock; private readonly RoleAssignmentService _service; private readonly IMemoryCache _cache = new MemoryCache(new MemoryCacheOptions()); - private readonly IAuthorizationService _authorizationService = Substitute.For(); public RoleAssignmentServiceTests() { @@ -34,18 +30,11 @@ public RoleAssignmentServiceTests() _dbContext = new HumansDbContext(options); _clock = new FakeClock(Instant.FromUtc(2026, 2, 15, 15, 30)); - // Default: authorize succeeds (tests for authorization denial are separate) - _authorizationService.AuthorizeAsync( - Arg.Any(), Arg.Any(), - Arg.Any>()) - .Returns(AuthorizationResult.Success()); - _service = new RoleAssignmentService( _dbContext, Substitute.For(), Substitute.For(), Substitute.For(), - _authorizationService, _clock, _cache, NullLogger.Instance); @@ -138,7 +127,7 @@ public async Task AssignRoleAsync_InvalidatesCachedClaimsForUser() _cache.Set(CacheKeys.RoleAssignmentClaims(userId), new[] { "stale-claim" }); var result = await _service.AssignRoleAsync( - userId, RoleNames.Board, assignerId, null, SystemPrincipal.Instance); + userId, RoleNames.Board, assignerId, null); result.Success.Should().BeTrue(); _cache.TryGetValue(CacheKeys.RoleAssignmentClaims(userId), out _).Should().BeFalse(); @@ -159,79 +148,12 @@ public async Task EndRoleAsync_InvalidatesCachedClaimsForUser() _cache.Set(CacheKeys.RoleAssignmentClaims(userId), new[] { "stale-claim" }); var result = await _service.EndRoleAsync( - assignment.Id, enderId, null, SystemPrincipal.Instance); + assignment.Id, enderId, null); result.Success.Should().BeTrue(); _cache.TryGetValue(CacheKeys.RoleAssignmentClaims(userId), out _).Should().BeFalse(); } - [Fact] - public async Task AssignRoleAsync_DeniedWhenAuthorizationFails() - { - var userId = Guid.NewGuid(); - var assignerId = Guid.NewGuid(); - await SeedUserAsync(userId, "Target User"); - await SeedUserAsync(assignerId, "Regular User"); - - _authorizationService.AuthorizeAsync( - Arg.Any(), Arg.Any(), - Arg.Any>()) - .Returns(AuthorizationResult.Failed()); - - var principal = new ClaimsPrincipal(new ClaimsIdentity()); - - var result = await _service.AssignRoleAsync( - userId, RoleNames.Admin, assignerId, null, principal); - - result.Success.Should().BeFalse(); - result.ErrorKey.Should().Be("Unauthorized"); - } - - [Fact] - public async Task EndRoleAsync_DeniedWhenAuthorizationFails() - { - var userId = Guid.NewGuid(); - var enderId = Guid.NewGuid(); - await SeedUserAsync(userId, "Target User"); - await SeedUserAsync(enderId, "Regular User"); - var assignment = await AddAssignmentAsync( - userId, - RoleNames.Admin, - _clock.GetCurrentInstant() - Duration.FromDays(1), - null); - - _authorizationService.AuthorizeAsync( - Arg.Any(), Arg.Any(), - Arg.Any>()) - .Returns(AuthorizationResult.Failed()); - - var principal = new ClaimsPrincipal(new ClaimsIdentity()); - - var result = await _service.EndRoleAsync( - assignment.Id, enderId, null, principal); - - result.Success.Should().BeFalse(); - result.ErrorKey.Should().Be("Unauthorized"); - } - - [Fact] - public async Task AssignRoleAsync_SystemPrincipalPassedToAuthorizationService() - { - var userId = Guid.NewGuid(); - var assignerId = Guid.NewGuid(); - await SeedUserAsync(userId, "Target User"); - await SeedUserAsync(assignerId, "System"); - - var result = await _service.AssignRoleAsync( - userId, RoleNames.Board, assignerId, null, SystemPrincipal.Instance); - - result.Success.Should().BeTrue(); - await _authorizationService.Received(1).AuthorizeAsync( - SystemPrincipal.Instance, - RoleNames.Board, - Arg.Any>()); - } - private async Task SeedUserAsync(Guid userId, string displayName) { var user = new User diff --git a/tests/Humans.Application.Tests/Services/TeamRoleServiceTests.cs b/tests/Humans.Application.Tests/Services/TeamRoleServiceTests.cs index db54148d..0b4eef65 100644 --- a/tests/Humans.Application.Tests/Services/TeamRoleServiceTests.cs +++ b/tests/Humans.Application.Tests/Services/TeamRoleServiceTests.cs @@ -1,5 +1,4 @@ using AwesomeAssertions; -using Microsoft.AspNetCore.Authorization; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; @@ -35,7 +34,6 @@ public TeamRoleServiceTests() Substitute.For(), Substitute.For(), Substitute.For(), - Substitute.For(), _clock, cache, NullLogger.Instance); diff --git a/tests/Humans.Application.Tests/Services/TeamServiceTests.cs b/tests/Humans.Application.Tests/Services/TeamServiceTests.cs index 7b01c64c..1c6b0906 100644 --- a/tests/Humans.Application.Tests/Services/TeamServiceTests.cs +++ b/tests/Humans.Application.Tests/Services/TeamServiceTests.cs @@ -1,5 +1,4 @@ using AwesomeAssertions; -using Microsoft.AspNetCore.Authorization; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.DependencyInjection; @@ -38,7 +37,6 @@ public TeamServiceTests() Substitute.For(), Substitute.For(), Substitute.For(), - Substitute.For(), _clock, _cache, NullLogger.Instance);