From 2be717a5dfbec889f612479eed22944c8e4a606b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 10 Jun 2026 10:34:34 +0200 Subject: [PATCH 1/7] feat: add IPolicy entity-level authorization layered over permissions (#162) - IPolicy, AuthorizationResult, IAuthorizer/Authorizer in SimpleModule.Core.Authorization.Policies: instance-level rules (ownership, tenancy, state) on top of string permissions; deny wins across multiple policies, missing policy fails closed with MissingPolicyException - PolicyAuthorizationOptions.NotFoundActions maps denials to 404 (default: view) to prevent resource enumeration - Declarative .AuthorizeResource(action) endpoint filter backed by IResourceResolver - Source generator discovers IPolicy implementors and emits scoped registrations in AddModules(); new SM0058 diagnostic requires policy resource types to be contracts DTOs - Notifications refactored as the reference implementation: NotificationPolicy (owner-or-admin), MarkReadEndpoint now does load -> authorize -> act via IAuthorizer - Docs: guide/policies.md, permissions cross-link, CONSTITUTION section and diagnostics table Closes #162 --- docs/CONSTITUTION.md | 20 ++ docs/site/.vitepress/config.ts | 1 + docs/site/guide/permissions.md | 2 + docs/site/guide/policies.md | 208 ++++++++++++++++ .../Policies/AuthorizationResult.cs | 28 +++ .../Authorization/Policies/Authorizer.cs | 64 +++++ .../Policies/EndpointPolicyExtensions.cs | 61 +++++ .../Authorization/Policies/IAuthorizer.cs | 35 +++ .../Authorization/Policies/IPolicy.cs | 20 ++ .../Policies/IResourceResolver.cs | 15 ++ .../Policies/MissingPolicyException.cs | 25 ++ .../Authorization/Policies/PolicyActions.cs | 14 ++ .../Policies/PolicyAuthorizationOptions.cs | 16 ++ .../AnalyzerReleases.Unshipped.md | 1 + .../Discovery/CoreSymbols.cs | 4 + .../Discovery/DiscoveryData.cs | 4 + .../Discovery/DiscoveryDataBuilder.cs | 10 + .../Discovery/Finders/PolicyFinder.cs | 87 +++++++ .../Discovery/Records/DataRecords.cs | 8 + .../Discovery/Records/WorkingTypes.cs | 9 + .../Discovery/SymbolDiscovery.cs | 5 + .../Emitters/DiagnosticEmitter.cs | 1 + .../DiagnosticDescriptors.Policy.cs | 15 ++ .../Emitters/Diagnostics/PolicyChecks.cs | 35 +++ .../Emitters/ModuleExtensionsEmitter.cs | 15 ++ .../SimpleModuleHostExtensions.cs | 6 + .../INotificationsContracts.cs | 13 +- .../Notifications/MarkReadEndpoint.cs | 26 +- .../NotificationPolicy.cs | 51 ++++ .../NotificationsModule.cs | 8 + .../Services/NotificationService.cs | 19 +- .../Integration/MarkReadEndpointTests.cs | 110 +++++++++ .../IntegrationTestCollection.cs | 6 + .../Unit/NotificationPolicyTests.cs | 96 ++++++++ .../Unit/NotificationServiceTests.cs | 32 ++- tasks/todo.md | 130 +++++----- .../Authorization/AuthorizerTests.cs | 226 ++++++++++++++++++ .../DiagnosticCatalogTests.cs | 5 + .../Helpers/GeneratorTestHelper.cs | 7 + .../PolicyAutoDiscoveryTests.cs | 183 ++++++++++++++ .../TopologicalSortTests.cs | 3 + 41 files changed, 1524 insertions(+), 100 deletions(-) create mode 100644 docs/site/guide/policies.md create mode 100644 framework/SimpleModule.Core/Authorization/Policies/AuthorizationResult.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/Authorizer.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/EndpointPolicyExtensions.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/IAuthorizer.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/IPolicy.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/IResourceResolver.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/MissingPolicyException.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/PolicyActions.cs create mode 100644 framework/SimpleModule.Core/Authorization/Policies/PolicyAuthorizationOptions.cs create mode 100644 framework/SimpleModule.Generator/Discovery/Finders/PolicyFinder.cs create mode 100644 framework/SimpleModule.Generator/Emitters/Diagnostics/DiagnosticDescriptors.Policy.cs create mode 100644 framework/SimpleModule.Generator/Emitters/Diagnostics/PolicyChecks.cs create mode 100644 modules/Notifications/src/SimpleModule.Notifications/NotificationPolicy.cs create mode 100644 modules/Notifications/tests/SimpleModule.Notifications.Tests/Integration/MarkReadEndpointTests.cs create mode 100644 modules/Notifications/tests/SimpleModule.Notifications.Tests/IntegrationTestCollection.cs create mode 100644 modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationPolicyTests.cs create mode 100644 tests/SimpleModule.Core.Tests/Authorization/AuthorizerTests.cs create mode 100644 tests/SimpleModule.Generator.Tests/PolicyAutoDiscoveryTests.cs diff --git a/docs/CONSTITUTION.md b/docs/CONSTITUTION.md index c22dcc2f..82312264 100644 --- a/docs/CONSTITUTION.md +++ b/docs/CONSTITUTION.md @@ -369,11 +369,25 @@ public sealed class CreateProductRequest : FormRequest - `.AllowAnonymous()` for public endpoints. - Never hardcode permission strings -- always use the permission constants. +### Instance-Level Policies + +String permissions answer "can this user perform this kind of action?". Policies answer "can this user perform this action on *this* resource?" — ownership, tenancy, and state-machine rules. + +- Implement `IPolicy` (in `SimpleModule.Core.Authorization.Policies`) in the module that owns the resource. Policies are auto-discovered by the source generator and registered as scoped services — no manual DI registration. +- The resource type must be a contracts DTO — a `[Dto]` type or a public type in the module's `.Contracts` assembly (SM0058). Policies guard resources that cross module boundaries. +- Policies **complement** permissions, they do not replace them: keep `.RequirePermission()` on the endpoint as the coarse capability gate, then check the instance rule via `IAuthorizer`. +- Endpoints follow load → authorize → act: fetch the resource, call `IAuthorizer.AuthorizeAsync(user, action, resource)`, then perform the operation. Denial throws `ForbiddenException` (403), or `NotFoundException` (404) for actions listed in `PolicyAuthorizationOptions.NotFoundActions` (default: `view`) to prevent resource enumeration. +- Use `PolicyActions` constants for CRUD verbs; declare module-specific actions as `public const string` on the policy class. +- Multiple policies may target the same resource type (e.g. a tenancy policy plus an ownership policy); a single deny wins. +- Collection scoping stays in queries (`WHERE UserId = @me`) — policies are for single-instance checks, not list filtering. +- Reference implementation: `NotificationPolicy` in `modules/Notifications`. + ### Rules - Permissions are owned by the defining module. - Other modules may reference permission constants from Contracts. - Not every module needs permissions. +- Policies are owned by the module that owns the resource — never write a policy for another module's entity. --- @@ -535,6 +549,12 @@ All SM diagnostics are emitted by the Roslyn source generator at compile time. ` | SM0056 | Error | FormRequest class must be sealed | | SM0057 | Error | FormRequest class must extend `FormRequest` | +### Policies + +| Diagnostic | Severity | Rule | +|------------|----------|------| +| SM0058 | Error | Policy resource type must be a contracts DTO | + ### Module Metadata | Diagnostic | Severity | Rule | diff --git a/docs/site/.vitepress/config.ts b/docs/site/.vitepress/config.ts index 6b4e0e26..815de16a 100644 --- a/docs/site/.vitepress/config.ts +++ b/docs/site/.vitepress/config.ts @@ -53,6 +53,7 @@ export default defineConfig({ { text: 'Events', link: '/guide/events' }, { text: 'Broadcasting', link: '/guide/broadcasting' }, { text: 'Permissions', link: '/guide/permissions' }, + { text: 'Policies', link: '/guide/policies' }, { text: 'Menus', link: '/guide/menus' }, { text: 'Settings', link: '/guide/settings' }, { text: 'Inertia.js Integration', link: '/guide/inertia' }, diff --git a/docs/site/guide/permissions.md b/docs/site/guide/permissions.md index daa6b1d6..e2609424 100644 --- a/docs/site/guide/permissions.md +++ b/docs/site/guide/permissions.md @@ -6,6 +6,8 @@ outline: deep SimpleModule includes a claims-based permission system that integrates with ASP.NET Core authorization. Each module defines its own permissions, registers them with the framework, and protects endpoints using attributes or extension methods. +Permissions are coarse capability gates ("can this user update products at all?"). For instance-level rules — ownership, tenancy, state-based access — layer a [policy](/guide/policies) on top. + ## Overview The permission system has three layers: diff --git a/docs/site/guide/policies.md b/docs/site/guide/policies.md new file mode 100644 index 00000000..909fee7a --- /dev/null +++ b/docs/site/guide/policies.md @@ -0,0 +1,208 @@ +--- +outline: deep +--- + +# Policies + +Permissions answer "can this user perform this kind of action?" (`Products.Update`). They cannot express instance-level rules like "users may only edit *their own* orders" or "tenant admins may only manage users *in their tenant*". Policies fill that gap. + +A policy is a class that encapsulates every per-resource authorization rule for one entity, inspired by [Laravel's policy classes](https://laravel.com/docs/authorization#creating-policies). Policies **layer on top of** permissions — the permission stays on the endpoint as the coarse capability gate; the policy adds the instance check. + +## Defining a Policy + +Implement `IPolicy` in the module that owns the resource. The resource type must be a contracts DTO — a `[Dto]` type or a public type in your `.Contracts` assembly (enforced by diagnostic SM0058). + +```csharp +using System.Security.Claims; +using SimpleModule.Core.Authorization; +using SimpleModule.Core.Authorization.Policies; +using SimpleModule.Core.Extensions; +using SimpleModule.Notifications.Contracts; +using SimpleModule.Users.Contracts; + +namespace SimpleModule.Notifications; + +public sealed class NotificationPolicy : IPolicy +{ + // Module-specific action beyond the conventional CRUD verbs + public const string MarkRead = "markRead"; + + public Task AuthorizeAsync( + ClaimsPrincipal user, + string action, + Notification resource, + CancellationToken cancellationToken = default + ) + { + var result = action switch + { + PolicyActions.View or MarkRead => AllowOwnerOrAdmin(user, resource), + _ => AuthorizationResult.Deny($"Unknown notification action '{action}'."), + }; + return Task.FromResult(result); + } + + private static AuthorizationResult AllowOwnerOrAdmin( + ClaimsPrincipal user, + Notification notification + ) + { + if (user.IsInRole(WellKnownRoles.Admin)) + { + return AuthorizationResult.Allow(); + } + + var userId = user.GetUserId(); + return userId is not null && notification.UserId == UserId.From(userId) + ? AuthorizationResult.Allow() + : AuthorizationResult.Deny("You can only access your own notifications."); + } +} +``` + +There is no registration step. The source generator discovers every `IPolicy` implementation in module assemblies and registers it as a scoped service in the generated `AddModules()`: + +```csharp +// generated +services.AddScoped, NotificationPolicy>(); +``` + +Use the `PolicyActions` constants (`view`, `create`, `update`, `delete`) for conventional verbs, and declare module-specific actions as `public const string` on the policy class so endpoints never hardcode action strings. + +## Checking a Policy in an Endpoint + +Inject `IAuthorizer` and follow **load → authorize → act**: + +```csharp +public class MarkReadEndpoint : IEndpoint +{ + public void Map(IEndpointRouteBuilder app) => + app.MapPost( + Route, + async Task ( + Guid id, + HttpContext context, + INotificationsContracts notifications, + IAuthorizer authorizer + ) => + { + var notification = await notifications.FindAsync(NotificationId.From(id)); + if (notification is null) + { + return TypedResults.NotFound(); + } + + // Throws on deny — translated by the global exception handler + await authorizer.AuthorizeAsync( + context.User, + NotificationPolicy.MarkRead, + notification, + context.RequestAborted + ); + + await notifications.MarkReadAsync(notification.Id); + return TypedResults.NoContent(); + } + ) + .RequirePermission(NotificationsPermissions.ViewOwn); // permission gate stays +} +``` + +`AuthorizeAsync` throws on denial, so the happy path stays free of authorization if-statements. To branch instead of throwing, use `CheckAsync`, which returns the `AuthorizationResult`: + +```csharp +var result = await authorizer.CheckAsync(user, PolicyActions.Update, order); +if (!result.IsAllowed) +{ + // result.Reason carries the policy's denial message +} +``` + +## Denial Semantics: 403 vs 404 + +A denied check throws `ForbiddenException` (403) with the policy's denial reason. For the `view` action it throws `NotFoundException` (404) instead, so unauthorized callers cannot probe which resource IDs exist. + +The action set that maps to 404 is configurable via `PolicyAuthorizationOptions`: + +```csharp +// In a module's ConfigureServices — markRead denials also surface as 404 +services.Configure(o => + o.NotFoundActions.Add(NotificationPolicy.MarkRead) +); +``` + +Calling `AuthorizeAsync`/`CheckAsync` for a resource type with **no registered policy throws `MissingPolicyException`** — authorization fails closed and loudly rather than silently allowing. + +## Multiple Policies per Resource + +More than one policy may target the same resource type — for example a tenancy-scoping policy plus an ownership policy, both owned by the resource's module. Policies run in registration order and evaluation stops at the first deny: **a single deny wins**, and its reason is surfaced. Allow requires every policy to allow. + +Denial reasons are returned verbatim in the 403 response detail — write them for the end user and never include internal identifiers. + +## Declarative Form + +When the endpoint does nothing with the resource except authorize it, skip the manual load with `AuthorizeResource`. Register an `IResourceResolver` once per resource type: + +```csharp +// In the module +public sealed class NotificationResolver(INotificationsContracts notifications) + : IResourceResolver +{ + public async ValueTask ResolveAsync( + string routeValue, + CancellationToken cancellationToken = default + ) => + Guid.TryParse(routeValue, out var id) + ? await notifications.FindAsync(NotificationId.From(id)) + : null; +} + +// In ConfigureServices +services.AddScoped, NotificationResolver>(); +``` + +Then the endpoint declares the check instead of performing it: + +```csharp +app.MapPost(Route, handler) + .RequirePermission(NotificationsPermissions.ViewOwn) + .AuthorizeResource(NotificationPolicy.MarkRead); // route param "id" by default +``` + +The filter loads the resource (404 when missing), authorizes the action, and only then invokes the handler. + +## What Policies Are Not For + +- **List filtering** — collection scoping belongs in queries (`WHERE UserId = @me`), not in per-item policy checks. Policies guard single instances. +- **Coarse capabilities** — "can this user use this feature at all" stays a permission string on the endpoint. +- **Other modules' entities** — a policy lives in the module that owns the resource. Never author a policy for an entity you don't own. + +## Rules Summary + +| Rule | Enforced by | +|------|-------------| +| Resource type must be a contracts DTO | SM0058 (build error) | +| Policies auto-registered as scoped services | Source generator | +| Missing policy at check time fails closed | `MissingPolicyException` | +| Deny wins across multiple policies | `IAuthorizer` | +| `view` denial surfaces as 404 (configurable) | `PolicyAuthorizationOptions` | + +## Testing Policies + +Policies are plain classes — unit test them directly without any host: + +```csharp +[Fact] +public async Task NonOwner_IsDenied() +{ + var policy = new NotificationPolicy(); + var user = new ClaimsPrincipal( + new ClaimsIdentity([new Claim("sub", "user-2")], "test") + ); + var notification = new Notification { UserId = UserId.From("user-1") }; + + var result = await policy.AuthorizeAsync(user, PolicyActions.View, notification); + + result.IsAllowed.Should().BeFalse(); +} +``` diff --git a/framework/SimpleModule.Core/Authorization/Policies/AuthorizationResult.cs b/framework/SimpleModule.Core/Authorization/Policies/AuthorizationResult.cs new file mode 100644 index 00000000..ad933044 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/AuthorizationResult.cs @@ -0,0 +1,28 @@ +namespace SimpleModule.Core.Authorization.Policies; + +/// +/// Outcome of a policy check. Use / to construct. +/// +public sealed class AuthorizationResult +{ + private static readonly AuthorizationResult AllowedResult = new(true, null); + + private AuthorizationResult(bool isAllowed, string? reason) + { + IsAllowed = isAllowed; + Reason = reason; + } + + public bool IsAllowed { get; } + + /// + /// Optional human-readable denial reason, surfaced verbatim in the 403 response + /// detail. Write it for the end user — never include internal identifiers or + /// implementation details. + /// + public string? Reason { get; } + + public static AuthorizationResult Allow() => AllowedResult; + + public static AuthorizationResult Deny(string? reason = null) => new(false, reason); +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/Authorizer.cs b/framework/SimpleModule.Core/Authorization/Policies/Authorizer.cs new file mode 100644 index 00000000..3d19d087 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/Authorizer.cs @@ -0,0 +1,64 @@ +using System.Security.Claims; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using SimpleModule.Core.Exceptions; + +namespace SimpleModule.Core.Authorization.Policies; + +public sealed class Authorizer( + IServiceProvider serviceProvider, + IOptions options +) : IAuthorizer +{ + public async Task CheckAsync( + ClaimsPrincipal user, + string action, + TResource resource, + CancellationToken cancellationToken = default + ) + { + var anyPolicy = false; + foreach (var policy in serviceProvider.GetServices>()) + { + anyPolicy = true; + var result = await policy + .AuthorizeAsync(user, action, resource, cancellationToken) + .ConfigureAwait(false); + if (!result.IsAllowed) + { + return result; + } + } + + if (!anyPolicy) + { + throw MissingPolicyException.ForResource(typeof(TResource)); + } + + return AuthorizationResult.Allow(); + } + + public async Task AuthorizeAsync( + ClaimsPrincipal user, + string action, + TResource resource, + CancellationToken cancellationToken = default + ) + { + var result = await CheckAsync(user, action, resource, cancellationToken) + .ConfigureAwait(false); + if (result.IsAllowed) + { + return; + } + + if (options.Value.NotFoundActions.Contains(action)) + { + throw new NotFoundException(); + } + + throw result.Reason is null + ? new ForbiddenException() + : new ForbiddenException(result.Reason); + } +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/EndpointPolicyExtensions.cs b/framework/SimpleModule.Core/Authorization/Policies/EndpointPolicyExtensions.cs new file mode 100644 index 00000000..a2650c16 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/EndpointPolicyExtensions.cs @@ -0,0 +1,61 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +using SimpleModule.Core.Exceptions; + +namespace SimpleModule.Core.Authorization.Policies; + +public static class EndpointPolicyExtensions +{ + /// + /// Declarative policy check: before the handler runs, loads the resource via the + /// module's using the + /// route value, then authorizes + /// against the registered . + /// Missing resource surfaces 404; denial surfaces 403 (or 404 per + /// ). + /// + public static RouteHandlerBuilder AuthorizeResource( + this RouteHandlerBuilder builder, + string action, + string routeParameterName = "id" + ) + { + return builder.AddEndpointFilter(async (context, next) => + { + var httpContext = context.HttpContext; + var routeValue = httpContext.GetRouteValue(routeParameterName)?.ToString(); + if (string.IsNullOrEmpty(routeValue)) + { + throw new NotFoundException(); + } + + var resolver = httpContext.RequestServices.GetService>(); + if (resolver is null) + { + throw new InvalidOperationException( + $"No IResourceResolver<{typeof(TResource).Name}> is registered. " + + "Register one in the owning module's ConfigureServices to use " + + "AuthorizeResource, or perform the check imperatively via IAuthorizer." + ); + } + + var resource = await resolver.ResolveAsync(routeValue, httpContext.RequestAborted); + if (resource is null) + { + throw new NotFoundException(); + } + + var authorizer = httpContext.RequestServices.GetRequiredService(); + await authorizer.AuthorizeAsync( + httpContext.User, + action, + resource, + httpContext.RequestAborted + ); + + return await next(context); + }); + } +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/IAuthorizer.cs b/framework/SimpleModule.Core/Authorization/Policies/IAuthorizer.cs new file mode 100644 index 00000000..e0fa2e23 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/IAuthorizer.cs @@ -0,0 +1,35 @@ +using System.Security.Claims; + +namespace SimpleModule.Core.Authorization.Policies; + +/// +/// Dispatches instance-level authorization checks to the registered +/// implementations for the resource type. +/// +public interface IAuthorizer +{ + /// + /// Runs the registered policies for in registration + /// order, stopping at the first deny. A single deny wins; allow requires every policy + /// to allow. Throws when no policy is registered. + /// + Task CheckAsync( + ClaimsPrincipal user, + string action, + TResource resource, + CancellationToken cancellationToken = default + ); + + /// + /// Like but throws on deny: + /// for actions listed in + /// (anti-enumeration), + /// otherwise . + /// + Task AuthorizeAsync( + ClaimsPrincipal user, + string action, + TResource resource, + CancellationToken cancellationToken = default + ); +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/IPolicy.cs b/framework/SimpleModule.Core/Authorization/Policies/IPolicy.cs new file mode 100644 index 00000000..abda5c80 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/IPolicy.cs @@ -0,0 +1,20 @@ +using System.Security.Claims; + +namespace SimpleModule.Core.Authorization.Policies; + +/// +/// Encapsulates instance-level authorization rules for a resource type — ownership, +/// tenancy, state-machine rules. Layered on top of string permissions: keep +/// .RequirePermission(...) as the coarse capability gate and use a policy for +/// per-resource decisions. Implementations are auto-discovered by the source generator +/// and registered as scoped services; the resource type must be a contracts DTO (SM0058). +/// +public interface IPolicy +{ + Task AuthorizeAsync( + ClaimsPrincipal user, + string action, + TResource resource, + CancellationToken cancellationToken = default + ); +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/IResourceResolver.cs b/framework/SimpleModule.Core/Authorization/Policies/IResourceResolver.cs new file mode 100644 index 00000000..86030b66 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/IResourceResolver.cs @@ -0,0 +1,15 @@ +namespace SimpleModule.Core.Authorization.Policies; + +/// +/// Loads a resource from a route value for the declarative +/// AuthorizeResource<TResource> endpoint filter. Modules that opt into the +/// declarative form register one resolver per resource type in +/// ConfigureServices; return null when the resource does not exist (surfaces 404). +/// +public interface IResourceResolver +{ + ValueTask ResolveAsync( + string routeValue, + CancellationToken cancellationToken = default + ); +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/MissingPolicyException.cs b/framework/SimpleModule.Core/Authorization/Policies/MissingPolicyException.cs new file mode 100644 index 00000000..e504fdc5 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/MissingPolicyException.cs @@ -0,0 +1,25 @@ +namespace SimpleModule.Core.Authorization.Policies; + +/// +/// Thrown when an authorization check runs for a resource type that has no registered +/// . This is a developer error (fail closed, loudly) — +/// add a policy class for the resource or remove the check. +/// +public sealed class MissingPolicyException : InvalidOperationException +{ + public MissingPolicyException() + : base("No policy is registered for the requested resource type.") { } + + public MissingPolicyException(string message) + : base(message) { } + + public MissingPolicyException(string message, Exception innerException) + : base(message, innerException) { } + + public static MissingPolicyException ForResource(Type resourceType) => + new( + $"No IPolicy<{resourceType?.Name}> is registered. " + + "Add a policy class implementing IPolicy in the module that owns the resource, " + + "or remove the authorization check." + ); +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/PolicyActions.cs b/framework/SimpleModule.Core/Authorization/Policies/PolicyActions.cs new file mode 100644 index 00000000..9a5bab3a --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/PolicyActions.cs @@ -0,0 +1,14 @@ +namespace SimpleModule.Core.Authorization.Policies; + +/// +/// Conventional action names for checks. Policies may +/// define additional module-specific actions; these constants just keep the common CRUD +/// verbs consistent across modules. +/// +public static class PolicyActions +{ + public const string View = "view"; + public const string Create = "create"; + public const string Update = "update"; + public const string Delete = "delete"; +} diff --git a/framework/SimpleModule.Core/Authorization/Policies/PolicyAuthorizationOptions.cs b/framework/SimpleModule.Core/Authorization/Policies/PolicyAuthorizationOptions.cs new file mode 100644 index 00000000..0035af81 --- /dev/null +++ b/framework/SimpleModule.Core/Authorization/Policies/PolicyAuthorizationOptions.cs @@ -0,0 +1,16 @@ +namespace SimpleModule.Core.Authorization.Policies; + +/// +/// Options for . By default a denied "view" action surfaces as +/// 404 instead of 403 so unauthorized callers cannot enumerate resource IDs; add or +/// remove actions to tune that behavior per host. +/// +public sealed class PolicyAuthorizationOptions +{ + /// + /// Actions whose denial throws (404) + /// instead of (403). Case-insensitive. + /// + public ISet NotFoundActions { get; } = + new HashSet(StringComparer.OrdinalIgnoreCase) { PolicyActions.View }; +} diff --git a/framework/SimpleModule.Generator/AnalyzerReleases.Unshipped.md b/framework/SimpleModule.Generator/AnalyzerReleases.Unshipped.md index 33d43bae..150d70c1 100644 --- a/framework/SimpleModule.Generator/AnalyzerReleases.Unshipped.md +++ b/framework/SimpleModule.Generator/AnalyzerReleases.Unshipped.md @@ -42,3 +42,4 @@ SM0054 | SimpleModule.Generator | Info | Endpoint missing Route const field SM0055 | SimpleModule.Generator | Error | Entity class must live in a Contracts assembly SM0056 | SimpleModule.Generator | Error | FormRequest class must be sealed SM0057 | SimpleModule.Generator | Error | FormRequest class must extend FormRequest +SM0058 | SimpleModule.Generator | Error | Policy resource type must be a contracts DTO diff --git a/framework/SimpleModule.Generator/Discovery/CoreSymbols.cs b/framework/SimpleModule.Generator/Discovery/CoreSymbols.cs index b3005417..652b5bfc 100644 --- a/framework/SimpleModule.Generator/Discovery/CoreSymbols.cs +++ b/framework/SimpleModule.Generator/Discovery/CoreSymbols.cs @@ -29,6 +29,7 @@ internal readonly record struct CoreSymbols( INamedTypeSymbol? ModuleOptions, INamedTypeSymbol? FormRequestAttribute, INamedTypeSymbol? FormRequestBase, + INamedTypeSymbol? PolicyInterface, bool HasAgentsAssembly, bool HasRagAssembly ) @@ -88,6 +89,9 @@ bool HasRagAssembly FormRequestBase: compilation.GetTypeByMetadataName( "SimpleModule.Core.FormRequests.FormRequest`1" ), + PolicyInterface: compilation.GetTypeByMetadataName( + "SimpleModule.Core.Authorization.Policies.IPolicy`1" + ), HasAgentsAssembly: compilation.GetTypeByMetadataName( "SimpleModule.Agents.SimpleModuleAgentExtensions" ) diff --git a/framework/SimpleModule.Generator/Discovery/DiscoveryData.cs b/framework/SimpleModule.Generator/Discovery/DiscoveryData.cs index cbbc651c..1cd2aed6 100644 --- a/framework/SimpleModule.Generator/Discovery/DiscoveryData.cs +++ b/framework/SimpleModule.Generator/Discovery/DiscoveryData.cs @@ -48,6 +48,7 @@ internal readonly record struct DiscoveryData( ImmutableArray AgentToolProviders, ImmutableArray KnowledgeSources, ImmutableArray FormRequests, + ImmutableArray Policies, ImmutableArray ContractsAssemblyNames, bool HasAgentsAssembly, bool HasRagAssembly, @@ -81,6 +82,7 @@ string HostAssemblyName ImmutableArray.Empty, ImmutableArray.Empty, ImmutableArray.Empty, + ImmutableArray.Empty, ImmutableArray.Empty, false, false, @@ -106,6 +108,7 @@ public bool Equals(DiscoveryData other) && AgentToolProviders.SequenceEqual(other.AgentToolProviders) && KnowledgeSources.SequenceEqual(other.KnowledgeSources) && FormRequests.SequenceEqual(other.FormRequests) + && Policies.SequenceEqual(other.Policies) && ContractsAssemblyNames.SequenceEqual(other.ContractsAssemblyNames) && HasAgentsAssembly == other.HasAgentsAssembly && HasRagAssembly == other.HasRagAssembly @@ -132,6 +135,7 @@ public override int GetHashCode() hash = HashHelper.HashArray(hash, AgentToolProviders); hash = HashHelper.HashArray(hash, KnowledgeSources); hash = HashHelper.HashArray(hash, FormRequests); + hash = HashHelper.HashArray(hash, Policies); hash = HashHelper.HashArray(hash, ContractsAssemblyNames); hash = HashHelper.Combine(hash, HasAgentsAssembly.GetHashCode()); hash = HashHelper.Combine(hash, HasRagAssembly.GetHashCode()); diff --git a/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs b/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs index 1a577891..b78485e1 100644 --- a/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs +++ b/framework/SimpleModule.Generator/Discovery/DiscoveryDataBuilder.cs @@ -30,6 +30,7 @@ internal static DiscoveryData Build( List agentToolProviders, List knowledgeSources, List formRequests, + List policies, Dictionary contractsAssemblyMap, bool hasAgentsAssembly, bool hasRagAssembly, @@ -184,6 +185,15 @@ string hostAssemblyName f.Location )) .ToImmutableArray(), + policies + .Select(p => new PolicyInfoRecord( + p.FullyQualifiedName, + p.ResourceTypeFqn, + p.ModuleName, + p.IsPublic, + p.Location + )) + .ToImmutableArray(), contractsAssemblyMap.Keys.ToImmutableArray(), hasAgentsAssembly, hasRagAssembly, diff --git a/framework/SimpleModule.Generator/Discovery/Finders/PolicyFinder.cs b/framework/SimpleModule.Generator/Discovery/Finders/PolicyFinder.cs new file mode 100644 index 00000000..bae051d8 --- /dev/null +++ b/framework/SimpleModule.Generator/Discovery/Finders/PolicyFinder.cs @@ -0,0 +1,87 @@ +using System.Collections.Generic; +using Microsoft.CodeAnalysis; + +namespace SimpleModule.Generator; + +internal static class PolicyFinder +{ + internal static void FindPolicyTypes( + INamespaceSymbol namespaceSymbol, + INamedTypeSymbol policyInterfaceSymbol, + string moduleName, + List results + ) + { + foreach (var member in namespaceSymbol.GetMembers()) + { + if (member is INamespaceSymbol childNs) + { + FindPolicyTypes(childNs, policyInterfaceSymbol, moduleName, results); + } + else if ( + member is INamedTypeSymbol typeSymbol + && typeSymbol.TypeKind == TypeKind.Class + && !typeSymbol.IsAbstract + && !typeSymbol.IsStatic + ) + { + // A class may implement IPolicy for more than one resource type; + // each closed interface becomes its own DI registration. + foreach (var iface in typeSymbol.AllInterfaces) + { + if ( + !SymbolEqualityComparer.Default.Equals( + iface.OriginalDefinition, + policyInterfaceSymbol + ) + ) + continue; + + results.Add( + new PolicyInfo + { + FullyQualifiedName = typeSymbol.ToDisplayString( + SymbolDisplayFormat.FullyQualifiedFormat + ), + ResourceTypeFqn = iface.TypeArguments[0] + .ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), + ModuleName = moduleName, + IsPublic = + typeSymbol.DeclaredAccessibility == Accessibility.Public, + Location = SymbolHelpers.GetSourceLocation(typeSymbol), + } + ); + } + } + } + } + + /// + /// Scans every module's implementation assembly for IPolicy<T> implementors. + /// No-op when the policy interface isn't resolvable. + /// + internal static void Discover( + List modules, + Dictionary moduleSymbols, + CoreSymbols symbols, + List policies + ) + { + if (symbols.PolicyInterface is not null) + { + SymbolHelpers.ScanModuleAssemblies( + modules, + moduleSymbols, + (assembly, module) => + { + FindPolicyTypes( + assembly.GlobalNamespace, + symbols.PolicyInterface, + module.ModuleName, + policies + ); + } + ); + } + } +} diff --git a/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs b/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs index 7e5bdcc7..9bdfa0bb 100644 --- a/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs +++ b/framework/SimpleModule.Generator/Discovery/Records/DataRecords.cs @@ -199,6 +199,14 @@ public override int GetHashCode() } } +internal readonly record struct PolicyInfoRecord( + string FullyQualifiedName, + string ResourceTypeFqn, + string ModuleName, + bool IsPublic, + SourceLocationRecord? Location +); + internal readonly record struct ModuleOptionsRecord( string FullyQualifiedName, string ModuleName, diff --git a/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs b/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs index f70ca313..9a1ee3be 100644 --- a/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs +++ b/framework/SimpleModule.Generator/Discovery/Records/WorkingTypes.cs @@ -164,6 +164,15 @@ internal sealed class DiscoveredTypeInfo public string ModuleName { get; set; } = ""; } +internal sealed class PolicyInfo +{ + public string FullyQualifiedName { get; set; } = ""; + public string ResourceTypeFqn { get; set; } = ""; + public string ModuleName { get; set; } = ""; + public bool IsPublic { get; set; } + public SourceLocationRecord? Location { get; set; } +} + internal sealed class FormRequestInfo { public string FullyQualifiedName { get; set; } = ""; diff --git a/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs b/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs index 45a42e41..180084d0 100644 --- a/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs +++ b/framework/SimpleModule.Generator/Discovery/SymbolDiscovery.cs @@ -211,6 +211,10 @@ CancellationToken cancellationToken moduleOptionsList ); + // Step 3f': IPolicy implementors (instance-level authorization) + var policies = new List(); + PolicyFinder.Discover(modules, moduleSymbols, s, policies); + // Step 3g: FormRequest types var formRequests = new List(); FormRequestFinder.Discover( @@ -264,6 +268,7 @@ CancellationToken cancellationToken agentToolProviders, knowledgeSources, formRequests, + policies, contractsAssemblyMap, s.HasAgentsAssembly, s.HasRagAssembly, diff --git a/framework/SimpleModule.Generator/Emitters/DiagnosticEmitter.cs b/framework/SimpleModule.Generator/Emitters/DiagnosticEmitter.cs index 8260e8c8..db35dc62 100644 --- a/framework/SimpleModule.Generator/Emitters/DiagnosticEmitter.cs +++ b/framework/SimpleModule.Generator/Emitters/DiagnosticEmitter.cs @@ -13,5 +13,6 @@ public void Emit(SourceProductionContext context, DiscoveryData data) PermissionFeatureChecks.Run(context, data); EndpointChecks.Run(context, data); FormRequestChecks.Run(context, data); + PolicyChecks.Run(context, data); } } diff --git a/framework/SimpleModule.Generator/Emitters/Diagnostics/DiagnosticDescriptors.Policy.cs b/framework/SimpleModule.Generator/Emitters/Diagnostics/DiagnosticDescriptors.Policy.cs new file mode 100644 index 00000000..d7d4a6f3 --- /dev/null +++ b/framework/SimpleModule.Generator/Emitters/Diagnostics/DiagnosticDescriptors.Policy.cs @@ -0,0 +1,15 @@ +using Microsoft.CodeAnalysis; + +namespace SimpleModule.Generator; + +internal static partial class DiagnosticDescriptors +{ + internal static readonly DiagnosticDescriptor PolicyResourceMustBeDto = new( + id: "SM0058", + title: "Policy resource type must be a contracts DTO", + messageFormat: "Policy '{0}' targets resource type '{1}' which is not a contracts DTO. Policies guard resources that cross module boundaries — move the resource type to the module's .Contracts assembly or mark it with [Dto].", + category: "SimpleModule.Generator", + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true + ); +} diff --git a/framework/SimpleModule.Generator/Emitters/Diagnostics/PolicyChecks.cs b/framework/SimpleModule.Generator/Emitters/Diagnostics/PolicyChecks.cs new file mode 100644 index 00000000..57886e6e --- /dev/null +++ b/framework/SimpleModule.Generator/Emitters/Diagnostics/PolicyChecks.cs @@ -0,0 +1,35 @@ +using System.Collections.Generic; +using Microsoft.CodeAnalysis; + +namespace SimpleModule.Generator; + +internal static class PolicyChecks +{ + internal static void Run(SourceProductionContext context, DiscoveryData data) + { + if (data.Policies.Length == 0) + return; + + // DtoTypes already contains both [Dto]-attributed types and convention DTOs + // (public types in .Contracts assemblies), so membership here is the full + // definition of "contracts DTO". + var dtoFqns = new HashSet(); + foreach (var dto in data.DtoTypes) + dtoFqns.Add(dto.FullyQualifiedName); + + foreach (var policy in data.Policies) + { + if (dtoFqns.Contains(policy.ResourceTypeFqn)) + continue; + + context.ReportDiagnostic( + Diagnostic.Create( + DiagnosticDescriptors.PolicyResourceMustBeDto, + LocationHelper.ToLocation(policy.Location), + TypeMappingHelpers.StripGlobalPrefix(policy.FullyQualifiedName), + TypeMappingHelpers.StripGlobalPrefix(policy.ResourceTypeFqn) + ) + ); + } + } +} diff --git a/framework/SimpleModule.Generator/Emitters/ModuleExtensionsEmitter.cs b/framework/SimpleModule.Generator/Emitters/ModuleExtensionsEmitter.cs index c6b98d66..50fc85b8 100644 --- a/framework/SimpleModule.Generator/Emitters/ModuleExtensionsEmitter.cs +++ b/framework/SimpleModule.Generator/Emitters/ModuleExtensionsEmitter.cs @@ -137,6 +137,21 @@ public void Emit(SourceProductionContext context, DiscoveryData data) } } + if (data.Policies.Length > 0) + { + sb.AppendLine(); + sb.AppendLine(" // Auto-discovered resource policies (IPolicy)"); + foreach (var policy in data.Policies) + { + if (!policy.IsPublic) + continue; + + sb.AppendLine( + $" services.AddScoped, {policy.FullyQualifiedName}>();" + ); + } + } + sb.AppendLine(); sb.AppendLine(" var permissionBuilder = new PermissionRegistryBuilder();"); diff --git a/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs b/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs index 6ef608bf..cc00d834 100644 --- a/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs +++ b/framework/SimpleModule.Hosting/SimpleModuleHostExtensions.cs @@ -11,6 +11,7 @@ using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Hosting; using SimpleModule.Core.Authorization; +using SimpleModule.Core.Authorization.Policies; using SimpleModule.Core.Constants; using SimpleModule.Core.Exceptions; using SimpleModule.Core.Health; @@ -108,6 +109,11 @@ public static WebApplicationBuilder AddSimpleModuleInfrastructure( builder.Services.AddHttpContextAccessor(); builder.Services.AddScoped(); + // Instance-level authorization (IPolicy dispatch). Policies themselves are + // auto-discovered by the source generator and registered in AddModules(). + builder.Services.AddOptions(); + builder.Services.AddScoped(); + builder.Services.AddScoped(); builder.Services.AddScoped(); diff --git a/modules/Notifications/src/SimpleModule.Notifications.Contracts/INotificationsContracts.cs b/modules/Notifications/src/SimpleModule.Notifications.Contracts/INotificationsContracts.cs index c732bd03..2f28ac7e 100644 --- a/modules/Notifications/src/SimpleModule.Notifications.Contracts/INotificationsContracts.cs +++ b/modules/Notifications/src/SimpleModule.Notifications.Contracts/INotificationsContracts.cs @@ -8,6 +8,17 @@ public interface INotificationsContracts Task> ListAsync(UserId userId, QueryNotificationsRequest request); Task GetUnreadCountAsync(UserId userId, CancellationToken cancellationToken = default); Task GetByIdAsync(NotificationId id, UserId userId); - Task MarkReadAsync(NotificationId id, UserId userId); + + /// + /// Loads a notification without owner scoping. Callers are responsible for the + /// instance-level check via IAuthorizer + NotificationPolicy. + /// + Task FindAsync(NotificationId id); + + /// + /// Marks a notification read. Authorization happens at the endpoint via + /// NotificationPolicy — this method assumes the caller is allowed. + /// + Task MarkReadAsync(NotificationId id); Task MarkAllReadAsync(UserId userId); } diff --git a/modules/Notifications/src/SimpleModule.Notifications/Endpoints/Notifications/MarkReadEndpoint.cs b/modules/Notifications/src/SimpleModule.Notifications/Endpoints/Notifications/MarkReadEndpoint.cs index 185dd49e..14787fe2 100644 --- a/modules/Notifications/src/SimpleModule.Notifications/Endpoints/Notifications/MarkReadEndpoint.cs +++ b/modules/Notifications/src/SimpleModule.Notifications/Endpoints/Notifications/MarkReadEndpoint.cs @@ -3,9 +3,8 @@ using Microsoft.AspNetCore.Routing; using SimpleModule.Core; using SimpleModule.Core.Authorization; -using SimpleModule.Core.Extensions; +using SimpleModule.Core.Authorization.Policies; using SimpleModule.Notifications.Contracts; -using SimpleModule.Users.Contracts; namespace SimpleModule.Notifications.Endpoints.Notifications; @@ -20,14 +19,27 @@ public void Map(IEndpointRouteBuilder app) => async Task ( Guid id, HttpContext context, - INotificationsContracts notifications + INotificationsContracts notifications, + IAuthorizer authorizer ) => { - var ok = await notifications.MarkReadAsync( - NotificationId.From(id), - UserId.From(context.User.GetUserId()!) + // Load → authorize → act: the permission gate below stays the coarse + // capability check; NotificationPolicy owns the per-instance rule. + var notification = await notifications.FindAsync(NotificationId.From(id)); + if (notification is null) + { + return TypedResults.NotFound(); + } + + await authorizer.AuthorizeAsync( + context.User, + NotificationPolicy.MarkRead, + notification, + context.RequestAborted ); - return ok ? TypedResults.NoContent() : TypedResults.NotFound(); + + await notifications.MarkReadAsync(notification.Id); + return TypedResults.NoContent(); } ) .RequirePermission(NotificationsPermissions.ViewOwn); diff --git a/modules/Notifications/src/SimpleModule.Notifications/NotificationPolicy.cs b/modules/Notifications/src/SimpleModule.Notifications/NotificationPolicy.cs new file mode 100644 index 00000000..e2696c0e --- /dev/null +++ b/modules/Notifications/src/SimpleModule.Notifications/NotificationPolicy.cs @@ -0,0 +1,51 @@ +using System.Security.Claims; +using SimpleModule.Core.Authorization; +using SimpleModule.Core.Authorization.Policies; +using SimpleModule.Core.Extensions; +using SimpleModule.Notifications.Contracts; +using SimpleModule.Users.Contracts; + +namespace SimpleModule.Notifications; + +/// +/// Instance-level rules for notifications: only the recipient (or an admin) may view or +/// mark a notification read. The permission gate (Notifications.ViewOwn) stays on +/// the endpoint; this policy adds the per-resource ownership check. Auto-registered by +/// the source generator. +/// +public sealed class NotificationPolicy : IPolicy +{ + /// Module-specific action beyond the conventional CRUD verbs. + public const string MarkRead = "markRead"; + + public Task AuthorizeAsync( + ClaimsPrincipal user, + string action, + Notification resource, + CancellationToken cancellationToken = default + ) + { + var result = action switch + { + PolicyActions.View or MarkRead => AllowOwnerOrAdmin(user, resource), + _ => AuthorizationResult.Deny($"Unknown notification action '{action}'."), + }; + return Task.FromResult(result); + } + + private static AuthorizationResult AllowOwnerOrAdmin( + ClaimsPrincipal user, + Notification notification + ) + { + if (user.IsInRole(WellKnownRoles.Admin)) + { + return AuthorizationResult.Allow(); + } + + var userId = user.GetUserId(); + return userId is not null && notification.UserId == UserId.From(userId) + ? AuthorizationResult.Allow() + : AuthorizationResult.Deny("You can only access your own notifications."); + } +} diff --git a/modules/Notifications/src/SimpleModule.Notifications/NotificationsModule.cs b/modules/Notifications/src/SimpleModule.Notifications/NotificationsModule.cs index 1c0ea43f..b7317745 100644 --- a/modules/Notifications/src/SimpleModule.Notifications/NotificationsModule.cs +++ b/modules/Notifications/src/SimpleModule.Notifications/NotificationsModule.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.DependencyInjection; using SimpleModule.BackgroundJobs.Contracts; using SimpleModule.Core; +using SimpleModule.Core.Authorization.Policies; using SimpleModule.Core.Settings; using SimpleModule.Database; using SimpleModule.Notifications.Channels; @@ -42,6 +43,13 @@ public void ConfigureServices(IServiceCollection services, IConfiguration config ); services.Configure(configuration.GetSection("Notifications")); + // Denied markRead surfaces as 404 (like "view") so callers can't probe + // other users' notification IDs. NotificationPolicy itself is auto-registered + // by the source generator. + services.Configure(o => + o.NotFoundActions.Add(NotificationPolicy.MarkRead) + ); + services.AddScoped(); services.AddScoped(); diff --git a/modules/Notifications/src/SimpleModule.Notifications/Services/NotificationService.cs b/modules/Notifications/src/SimpleModule.Notifications/Services/NotificationService.cs index a035d2e4..44c62d88 100644 --- a/modules/Notifications/src/SimpleModule.Notifications/Services/NotificationService.cs +++ b/modules/Notifications/src/SimpleModule.Notifications/Services/NotificationService.cs @@ -61,24 +61,19 @@ await db.Notifications.AsNoTracking().FirstOrDefaultAsync(n => n.Id == id && n.UserId == userId ); - public async Task MarkReadAsync(NotificationId id, UserId userId) - { - var notification = await db.Notifications.FirstOrDefaultAsync(n => - n.Id == id && n.UserId == userId - ); - if (notification is null) - { - return false; - } + public async Task FindAsync(NotificationId id) => + await db.Notifications.AsNoTracking().FirstOrDefaultAsync(n => n.Id == id); - if (notification.ReadAt is not null) + public async Task MarkReadAsync(NotificationId id) + { + var notification = await db.Notifications.FirstOrDefaultAsync(n => n.Id == id); + if (notification is null || notification.ReadAt is not null) { - return true; + return; } notification.ReadAt = DateTimeOffset.UtcNow; await db.SaveChangesAsync(); - return true; } public async Task MarkAllReadAsync(UserId userId) diff --git a/modules/Notifications/tests/SimpleModule.Notifications.Tests/Integration/MarkReadEndpointTests.cs b/modules/Notifications/tests/SimpleModule.Notifications.Tests/Integration/MarkReadEndpointTests.cs new file mode 100644 index 00000000..6424c315 --- /dev/null +++ b/modules/Notifications/tests/SimpleModule.Notifications.Tests/Integration/MarkReadEndpointTests.cs @@ -0,0 +1,110 @@ +using System.Net; +using System.Security.Claims; +using FluentAssertions; +using Microsoft.Extensions.DependencyInjection; +using SimpleModule.Notifications.Contracts; +using SimpleModule.Tests.Shared.Fixtures; +using SimpleModule.Users.Contracts; + +namespace SimpleModule.Notifications.Tests.Integration; + +/// +/// End-to-end coverage of the policy-based authorization flow: the MarkRead endpoint +/// loads the notification, dispatches to via +/// IAuthorizer, and denial for non-owners surfaces as 404 (configured through +/// PolicyAuthorizationOptions in the module). +/// +[Collection(TestCollections.Integration)] +public sealed class MarkReadEndpointTests(SimpleModuleWebApplicationFactory factory) +{ + private async Task SeedAsync(string ownerId) + { + using var scope = factory.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + var notification = new Notification + { + Id = NotificationId.From(Guid.CreateVersion7()), + UserId = UserId.From(ownerId), + Type = "test.event", + Channel = NotificationsConstants.Channels.Database, + Title = "Title", + DataJson = "{}", + }; + db.Notifications.Add(notification); + await db.SaveChangesAsync(); + return notification; + } + + private HttpClient CreateClientFor(string userId) => + factory.CreateAuthenticatedClient( + [NotificationsPermissions.ViewOwn], + new Claim("sub", userId) + ); + + [Fact] + public async Task MarkRead_AsOwner_Returns204AndMarksRead() + { + var notification = await SeedAsync("owner-1"); + var client = CreateClientFor("owner-1"); + + var response = await client.PostAsync( + $"/api/notifications/{notification.Id.Value}/read", + null + ); + + response.StatusCode.Should().Be(HttpStatusCode.NoContent); + + using var scope = factory.Services.CreateScope(); + var contracts = scope.ServiceProvider.GetRequiredService(); + var refreshed = await contracts.FindAsync(notification.Id); + refreshed!.ReadAt.Should().NotBeNull(); + } + + [Fact] + public async Task MarkRead_AsNonOwner_Returns404NotForbidden() + { + var notification = await SeedAsync("owner-1"); + var client = CreateClientFor("intruder-2"); + + var response = await client.PostAsync( + $"/api/notifications/{notification.Id.Value}/read", + null + ); + + // NotificationsModule maps denied markRead to 404 via PolicyAuthorizationOptions + // so callers cannot probe other users' notification IDs. + response.StatusCode.Should().Be(HttpStatusCode.NotFound); + + using var scope = factory.Services.CreateScope(); + var contracts = scope.ServiceProvider.GetRequiredService(); + var refreshed = await contracts.FindAsync(notification.Id); + refreshed!.ReadAt.Should().BeNull(); + } + + [Fact] + public async Task MarkRead_MissingNotification_Returns404() + { + var client = CreateClientFor("owner-1"); + + var response = await client.PostAsync( + $"/api/notifications/{Guid.CreateVersion7()}/read", + null + ); + + response.StatusCode.Should().Be(HttpStatusCode.NotFound); + } + + [Fact] + public async Task MarkRead_Unauthenticated_Returns401() + { + var notification = await SeedAsync("owner-1"); + var client = factory.CreateClient(); + + var response = await client.PostAsync( + $"/api/notifications/{notification.Id.Value}/read", + null + ); + + response.StatusCode.Should().Be(HttpStatusCode.Unauthorized); + } +} diff --git a/modules/Notifications/tests/SimpleModule.Notifications.Tests/IntegrationTestCollection.cs b/modules/Notifications/tests/SimpleModule.Notifications.Tests/IntegrationTestCollection.cs new file mode 100644 index 00000000..8c4a61db --- /dev/null +++ b/modules/Notifications/tests/SimpleModule.Notifications.Tests/IntegrationTestCollection.cs @@ -0,0 +1,6 @@ +using SimpleModule.Tests.Shared.Fixtures; +using Xunit; + +[CollectionDefinition(TestCollections.Integration)] +public sealed class IntegrationTestCollection + : ICollectionFixture; diff --git a/modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationPolicyTests.cs b/modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationPolicyTests.cs new file mode 100644 index 00000000..dd7afc8d --- /dev/null +++ b/modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationPolicyTests.cs @@ -0,0 +1,96 @@ +using System.Security.Claims; +using FluentAssertions; +using SimpleModule.Core.Authorization; +using SimpleModule.Core.Authorization.Policies; +using SimpleModule.Notifications.Contracts; +using SimpleModule.Users.Contracts; + +namespace SimpleModule.Notifications.Tests.Unit; + +public sealed class NotificationPolicyTests +{ + private readonly NotificationPolicy _sut = new(); + + private static ClaimsPrincipal CreateUser(string userId, params string[] roles) + { + var claims = new List { new("sub", userId) }; + claims.AddRange(roles.Select(r => new Claim(ClaimTypes.Role, r))); + return new ClaimsPrincipal(new ClaimsIdentity(claims, "test", "name", ClaimTypes.Role)); + } + + private static Notification CreateNotification(string ownerId) => + new() + { + Id = NotificationId.From(Guid.CreateVersion7()), + UserId = UserId.From(ownerId), + Type = "test.event", + Channel = NotificationsConstants.Channels.Database, + }; + + [Theory] + [InlineData(PolicyActions.View)] + [InlineData(NotificationPolicy.MarkRead)] + public async Task Owner_IsAllowed(string action) + { + var result = await _sut.AuthorizeAsync( + CreateUser("user-1"), + action, + CreateNotification("user-1") + ); + + result.IsAllowed.Should().BeTrue(); + } + + [Theory] + [InlineData(PolicyActions.View)] + [InlineData(NotificationPolicy.MarkRead)] + public async Task NonOwner_IsDenied(string action) + { + var result = await _sut.AuthorizeAsync( + CreateUser("user-2"), + action, + CreateNotification("user-1") + ); + + result.IsAllowed.Should().BeFalse(); + result.Reason.Should().NotBeNullOrEmpty(); + } + + [Fact] + public async Task Admin_IsAllowedForOthersNotifications() + { + var result = await _sut.AuthorizeAsync( + CreateUser("admin-1", WellKnownRoles.Admin), + NotificationPolicy.MarkRead, + CreateNotification("user-1") + ); + + result.IsAllowed.Should().BeTrue(); + } + + [Fact] + public async Task UnknownAction_IsDenied() + { + var result = await _sut.AuthorizeAsync( + CreateUser("user-1"), + "transmogrify", + CreateNotification("user-1") + ); + + result.IsAllowed.Should().BeFalse(); + } + + [Fact] + public async Task UserWithoutIdClaim_IsDenied() + { + var anonymous = new ClaimsPrincipal(new ClaimsIdentity()); + + var result = await _sut.AuthorizeAsync( + anonymous, + PolicyActions.View, + CreateNotification("user-1") + ); + + result.IsAllowed.Should().BeFalse(); + } +} diff --git a/modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationServiceTests.cs b/modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationServiceTests.cs index bc744c8d..8a654ec6 100644 --- a/modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationServiceTests.cs +++ b/modules/Notifications/tests/SimpleModule.Notifications.Tests/Unit/NotificationServiceTests.cs @@ -98,21 +98,41 @@ public async Task MarkReadAsync_SetsReadAt() { var n = await SeedAsync(); - var result = await _sut.MarkReadAsync(n.Id, _userId); + await _sut.MarkReadAsync(n.Id); - result.Should().BeTrue(); var refreshed = await _db.Notifications.AsNoTracking().FirstAsync(x => x.Id == n.Id); refreshed.ReadAt.Should().NotBeNull(); } [Fact] - public async Task MarkReadAsync_WithDifferentUser_ReturnsFalse() + public async Task MarkReadAsync_AlreadyRead_KeepsOriginalReadAt() { - var n = await SeedAsync(); + var n = await SeedAsync(readAt: DateTimeOffset.UtcNow.AddDays(-1)); + var stored = await _db.Notifications.AsNoTracking().FirstAsync(x => x.Id == n.Id); + + await _sut.MarkReadAsync(n.Id); - var result = await _sut.MarkReadAsync(n.Id, UserId.From("not-the-owner")); + var refreshed = await _db.Notifications.AsNoTracking().FirstAsync(x => x.Id == n.Id); + refreshed.ReadAt.Should().Be(stored.ReadAt); + } + + [Fact] + public async Task FindAsync_ReturnsNotificationRegardlessOfOwner() + { + var n = await SeedAsync(userId: UserId.From("other-user")); + + var found = await _sut.FindAsync(n.Id); + + found.Should().NotBeNull(); + found!.UserId.Should().Be(UserId.From("other-user")); + } + + [Fact] + public async Task FindAsync_MissingNotification_ReturnsNull() + { + var found = await _sut.FindAsync(NotificationId.From(Guid.CreateVersion7())); - result.Should().BeFalse(); + found.Should().BeNull(); } [Fact] diff --git a/tasks/todo.md b/tasks/todo.md index 9ad3a546..c4591d49 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -1,78 +1,60 @@ -# Task: Design-system consistency pass across all module pages +# Issue #162 — Policy classes for entity-level authorization — DONE -Goal: every page uses the design system consistently → run /qa → open PR with screenshots. - -Scope: 13 tracked modules with `.tsx` source. The 8 untracked dirs (Agents, Chat, Datasets, -Map, Marketplace, Orders, PageBuilder, Products) are stale build artifacts with NO source — left untouched, flagged to user. - -Out of scope (noted, not changed): i18n hardcoded-string gaps; the intentional centered-card -auth-page layout (only token/control bugs inside auth pages are fixed, not forced into PageShell). - -## Fixes (from parallel line-level audit) - -### HIGH — color tokens breaking dark mode / raw controls -- [ ] Tenants/tenantStatus.ts — raw palette → Badge variant map (success/warning/danger) -- [ ] Tenants/Browse.tsx, Manage.tsx — status span → -- [ ] Tenants/Features.tsx — text-green/red-600 → -- [ ] BackgroundJobs/Dashboard.tsx — text-red-500, border-red-200, hover:bg-red-50 → semantic -- [ ] BackgroundJobs/Detail.tsx — text-red-600, border-red-200, bg-red-50/text-red-800 → semantic -- [ ] FeatureFlags/Manage.tsx:264 — raw → -- [ ] RateLimiting/components/RulesTable.tsx — text-muted-foreground (undefined) → text-text-muted -- [ ] Email/History.tsx — text-destructive (undefined) → text-danger -- [ ] OpenIddict/OAuthCallback.tsx — text-muted → text-text-muted -- [ ] Dashboard/Home.tsx — text-white → text-text-inverse -- [ ] Users/Login.tsx, Register.tsx — text-white + inline var → bg-primary text-text-inverse; raw checkbox → Checkbox -- [ ] Users/LoginWith2fa.tsx — raw checkbox → Checkbox - -### MEDIUM — hand-rolled layout → PageShell; custom markup → DS components -- [ ] Settings/UserSettings.tsx — Container+h1 → PageShell; error banner → Alert -- [ ] Settings/AdminSettings.tsx — error banner → Alert -- [ ] Admin/RolesCreate, RolesEdit, UsersCreate, UsersEdit — Container+Breadcrumb+h1 → PageShell -- [ ] Admin/Roles.tsx — raw