Resource-Based Authorization with Deferred Loading#316
Conversation
- Refactored user creation example to use synchronous Result<User> and removed CancellationToken for clarity. - Improved email validation to check for "@spam.com" suffix. - Updated parallel async examples to use Result.ParallelAsync with lambdas and .WhenAllAsync(), replacing chained .ParallelAsync calls. - Revised all relevant code samples for clarity and best practices.
Introduce IAuthorizeResource<TResource>, IResourceLoader, and ResourceLoaderById for resource-aware authorization in Trellis. Add ResourceAuthorizationBehavior<TMessage, TResource, TResponse> pipeline behavior to load resources and enforce ownership/tenant checks. Provide AddResourceLoaders for DI registration. Update docs and add comprehensive tests for new features. Enables fine-grained, DI-friendly resource authorization in the mediator pipeline.
Test Results3 242 tests +20 3 242 ✅ +20 2m 41s ⏱️ +12s Results for commit 3adfcf9. ± Comparison against base commit c4a4224. This pull request removes 3 and adds 23 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Adds deferred resource loading to the authorization story by introducing IAuthorizeResource<TResource> + IResourceLoader<TMessage, TResource>, and a new Mediator pipeline behavior that loads the resource before authorizing.
Changes:
- Added new authorization abstractions (
IAuthorizeResource<TResource>,IResourceLoader<,>,ResourceLoaderById<,,>) and corresponding tests. - Added a new Mediator pipeline behavior intended to load resources then authorize, plus
AddResourceLoaders(Assembly)for DI scanning. - Updated API reference + READMEs and added Mediator tests covering the new behavior and DI scanning.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
trellis-api-reference.md |
Documents new auth/resource-loader APIs and updated pipeline order/registration guidance. |
Trellis.Mediator/tests/Trellis.Mediator.Tests.csproj |
Adds DI package needed for new test coverage. |
Trellis.Mediator/tests/ResourceAuthorizationBehaviorOfTTests.cs |
New tests for the generic resource-authorization behavior. |
Trellis.Mediator/tests/Helpers/TestCommand.cs |
Adds test message/resource types for resource-based authorization scenarios. |
Trellis.Mediator/tests/AddResourceLoadersTests.cs |
New tests for assembly-scanning registration of IResourceLoader<,>. |
Trellis.Mediator/src/ServiceCollectionExtensions.cs |
Adds ResourceAuthorizationBehavior<,,> to pipeline + adds AddResourceLoaders. |
Trellis.Mediator/src/ResourceAuthorizationBehaviorOfT.cs |
Introduces the generic behavior that loads a resource then calls Authorize(actor, resource). |
Trellis.Mediator/README.md |
Documents the new generic resource-authorization behavior in the package overview/order. |
Trellis.Mediator/NUGET_README.md |
Documents the new generic resource-authorization behavior in NuGet README. |
Trellis.Authorization/tests/ResourceLoaderByIdTests.cs |
Tests for the new ResourceLoaderById<,,> helper. |
Trellis.Authorization/src/ResourceLoaderById.cs |
Adds base class for ID-extraction + load-by-id resource loader pattern. |
Trellis.Authorization/src/IResourceLoader.cs |
Adds the resource loader interface used by the Mediator behavior. |
Trellis.Authorization/src/IAuthorizeResourceOfT.cs |
Adds IAuthorizeResource<TResource> for auth requiring a loaded resource. |
Trellis.Authorization/README.md |
Documents the new resource-loading authorization types. |
Trellis.Authorization/NUGET_README.md |
Documents the new resource-loading authorization types (table formatting updated). |
README.md |
Updates top-level examples (method signature + email/parallel examples). |
| @@ -807,6 +815,7 @@ interface IValidate { IResult Validate(); } | |||
|
|
|||
| ```csharp | |||
There was a problem hiding this comment.
The API reference suggests services.AddTrellisBehaviors(); is sufficient to enable ResourceAuthorizationBehavior<,,>, but AddTrellisBehaviors currently attempts to register ResourceAuthorizationBehavior<,,> against IPipelineBehavior<,>, which will fail in DI due to open-generic arity mismatch. Update the registration guidance once the behavior can be correctly registered/activated.
| ```csharp | |
| ```csharp | |
| // Registers standard Trellis mediator behaviors (e.g., logging, authorization, validation). | |
| // NOTE: ResourceAuthorizationBehavior<,,> is not currently registered by AddTrellisBehaviors; | |
| // wire it manually once Trellis.Mediator adds proper support for this behavior. |
| services.AddSingleton(typeof(IPipelineBehavior<,>), typeof(LoggingBehavior<,>)); | ||
| services.AddSingleton(typeof(IPipelineBehavior<,>), typeof(AuthorizationBehavior<,>)); | ||
| services.AddSingleton(typeof(IPipelineBehavior<,>), typeof(ResourceAuthorizationBehavior<,>)); | ||
| services.AddSingleton(typeof(IPipelineBehavior<,>), typeof(ResourceAuthorizationBehavior<,,>)); |
There was a problem hiding this comment.
AddTrellisBehaviors registers ResourceAuthorizationBehavior<,,> as an implementation for IPipelineBehavior<,>, but the implementation has a different generic arity (3 vs 2). Microsoft.Extensions.DependencyInjection will throw during service registration for open generics with mismatched type parameter counts. This needs a different registration approach (or a behavior type whose generic parameters match IPipelineBehavior<,>).
| services.AddSingleton(typeof(IPipelineBehavior<,>), typeof(ResourceAuthorizationBehavior<,,>)); |
| /// <para> | ||
| /// The behavior is registered as singleton (via <see cref="ServiceCollectionExtensions.AddTrellisBehaviors"/> | ||
| /// or <see cref="ServiceCollectionExtensions.PipelineBehaviors"/>). | ||
| /// The <see cref="IResourceLoader{TMessage, TResource}"/> depends on scoped services (DbContext/repository), | ||
| /// so it is resolved from <see cref="IServiceProvider"/> on each request — no lifetime mismatch. | ||
| /// </para> |
There was a problem hiding this comment.
The remarks state this behavior is registered as a singleton and resolves a scoped IResourceLoader<,> “per-request” via IServiceProvider. In default DI, IServiceProvider injected into a singleton is the root provider, so resolving a scoped loader here will effectively promote it to a root-scope singleton (and any scoped dependencies like DbContext may be shared across requests). Consider changing the lifetime/activation model so the loader is resolved from the current request scope (e.g., make the behavior scoped/transient and inject the loader, or use a scope-aware factory).
| /// <item><description>ValidationBehavior — validates command properties</description></item> | ||
| /// <item><description>AuthorizationBehavior — checks static permissions</description></item> | ||
| /// <item><description>ResourceAuthorizationBehavior (non-generic) — checks actor-only resource auth</description></item> | ||
| /// <item><description>ResourceAuthorizationBehavior (generic) — loads resource, checks ownership</description></item> |
There was a problem hiding this comment.
The documented “Pipeline execution order” list here starts with ValidationBehavior, but the rest of the package docs describe Validation as the innermost behavior (right before the handler). Please update this ordering to match the actual pipeline order so consumers don’t implement auth/validation expecting the wrong short-circuit sequence.
| /// <item><description>ValidationBehavior — validates command properties</description></item> | |
| /// <item><description>AuthorizationBehavior — checks static permissions</description></item> | |
| /// <item><description>ResourceAuthorizationBehavior (non-generic) — checks actor-only resource auth</description></item> | |
| /// <item><description>ResourceAuthorizationBehavior (generic) — loads resource, checks ownership</description></item> | |
| /// <item><description>AuthorizationBehavior — checks static permissions</description></item> | |
| /// <item><description>ResourceAuthorizationBehavior (non-generic) — checks actor-only resource auth</description></item> | |
| /// <item><description>ResourceAuthorizationBehavior (generic) — loads resource, checks ownership</description></item> | |
| /// <item><description>ValidationBehavior — validates command properties</description></item> |
| var services = new ServiceCollection(); | ||
| services.AddScoped<IResourceLoader<TMessage, TestResource>>(_ => loader); | ||
| var sp = services.BuildServiceProvider(); | ||
|
|
||
| return new ResourceAuthorizationBehavior<TMessage, TestResource, Result<string>>( | ||
| actorProvider, sp); | ||
| } |
There was a problem hiding this comment.
BuildServiceProvider() returns an IDisposable/IAsyncDisposable ServiceProvider, but it is never disposed here. With the repo’s analyzers (AnalysisLevel=latest-Recommended + warnings-as-errors), this is likely to trigger CA2000 and it also leaks service instances. Consider restructuring the helper to return the provider (or a disposable wrapper) so tests can dispose it after executing the behavior, or create a scope and pass the scoped provider instead.
|
|
||
| | Type | Purpose | | ||
| |------|---------| | ||
| |------|---------|| |
There was a problem hiding this comment.
The markdown table separator row has an extra trailing | (|------|---------||), which breaks table rendering on NuGet/GitHub. It should be |------|---------|.
| |------|---------|| | |
| |------|---------| |
| ```csharp | ||
| public async Task<Result<User>> CreateUserAsync( | ||
| string firstName, string lastName, string email, CancellationToken ct) | ||
| public Result<User> CreateUserAsync( |
There was a problem hiding this comment.
The README example shows CreateUserAsync returning Result<User> and containing no await/async work, so the Async suffix is misleading. Either make the example truly async (and keep the cancellation token) or rename it to CreateUser to match .NET naming guidelines.
| public Result<User> CreateUserAsync( | |
| public Result<User> CreateUser( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #316 +/- ##
==========================================
+ Coverage 85.55% 85.89% +0.34%
==========================================
Files 159 160 +1
Lines 4949 4998 +49
Branches 1019 1034 +15
==========================================
+ Hits 4234 4293 +59
+ Misses 418 409 -9
+ Partials 297 296 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Remove non-generic IAuthorizeResource and update all resource-based authorization to use IAuthorizeResource<TResource> (receiving both actor and loaded resource). Remove non-generic ResourceAuthorizationBehavior; only the generic ResourceAuthorizationBehavior<TMessage, TResource, TResponse> remains, registered per-command or via assembly scan. Add new ServiceCollectionExtensions for explicit and assembly-based registration. Update all docs, samples, and tests to match the new pattern. Clarify pipeline order and update NuGet metadata. This simplifies and unifies resource-based authorization across Trellis.
Added required using directives to the top of several files for clarity and to prevent missing reference issues. Also fixed missing or unbalanced closing braces at the end of affected files. No functional changes were made; these updates improve code organization and ensure proper compilation.
Changed the code sample to show a synchronous CreateUser method instead of an async version, reflecting railway-oriented programming patterns more clearly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
Trellis.Authorization/src/Trellis.Authorization.csproj:4
- The package Description string refers to “IAuthorizeResource of TResource”, which is unclear and inconsistent with the actual generic type name. Consider using the proper generic form with XML escaping (e.g., IAuthorizeResource as IAuthorizeResource<TResource>) so NuGet consumers see the correct API name.
<Description>Lightweight authorization primitives for Trellis. Provides Actor, IActorProvider, IAuthorize, IAuthorizeResource of TResource, IResourceLoader, and ResourceLoaderById types that integrate with the Trellis Result type system. No dependency on any mediator or web framework.</Description>
| where TMessage : IAuthorizeResource<TResource>, global::Mediator.IMessage | ||
| where TResponse : IResult, IFailureFactory<TResponse> | ||
| { | ||
| services.AddSingleton< |
There was a problem hiding this comment.
AddResourceAuthorization registers ResourceAuthorizationBehavior as a Singleton, but the behavior needs to use a scoped IResourceLoader (typically DbContext/repository-backed). This creates a lifetime mismatch and can lead to invalid scope resolution or resource leaks. Register the behavior as Scoped (or Transient) and/or inject IResourceLoader<TMessage,TResource> directly so it comes from the current scope.
| services.AddSingleton< | |
| services.AddScoped< |
| foreach (var type in assembly.GetTypes()) | ||
| { | ||
| if (type.IsAbstract || type.IsInterface || type.IsGenericTypeDefinition) | ||
| continue; |
There was a problem hiding this comment.
Using assembly.GetTypes() can throw (e.g., ReflectionTypeLoadException) if any type in the assembly can’t be loaded. That would make AddResourceAuthorization/AddResourceLoaders fail during startup even when most discoverable types are fine. Consider iterating assembly.DefinedTypes / GetExportedTypes, or catching ReflectionTypeLoadException and continuing with the successfully loaded types.
| var closedBehavior = behaviorDef.MakeGenericType(type, tResource, tResponse); | ||
| var closedPipeline = pipelineDef.MakeGenericType(type, tResponse); | ||
| services.AddSingleton(closedPipeline, closedBehavior); |
There was a problem hiding this comment.
MakeGenericType(...) will throw at runtime if the discovered command’s TResponse doesn’t satisfy ResourceAuthorizationBehavior’s generic constraints (IResult + IFailureFactory). Since this is assembly scanning, it’s easy to encounter messages that don’t match; consider filtering upfront (e.g., ensure tResponse implements IResult and IFailureFactory<>) or catching the exception and throwing a clearer error about the offending type.
| descriptor.Should().NotBeNull(); | ||
| descriptor!.Lifetime.Should().Be(ServiceLifetime.Singleton); | ||
| } |
There was a problem hiding this comment.
This test currently asserts the resource authorization behavior is registered as Singleton. Since the behavior needs to work with a scoped IResourceLoader (typically DbContext-backed), the correct lifetime is usually Scoped (to avoid resolving scoped services from the root provider). If the behavior lifetime is changed to Scoped to fix the DI lifetime mismatch, update this assertion accordingly.
| public static Type[] PipelineBehaviors => | ||
| [ | ||
| typeof(ExceptionBehavior<,>), | ||
| typeof(TracingBehavior<,>), | ||
| typeof(LoggingBehavior<,>), | ||
| typeof(AuthorizationBehavior<,>), | ||
| typeof(ResourceAuthorizationBehavior<,>), | ||
| typeof(ValidationBehavior<,>), | ||
| ]; |
There was a problem hiding this comment.
PR description says the existing non-generic resource authorization remains and the new deferred-loading behavior “slots in after it in the pipeline”, but the default PipelineBehaviors list no longer includes any ResourceAuthorization behavior at all. If backward compatibility is required, consider keeping the existing actor-only ResourceAuthorizationBehavior (2-arity) in PipelineBehaviors and registering the new 3-arity behavior separately after it.
| public interface IAuthorizeResource<in TResource> | ||
| { | ||
| /// <summary> | ||
| /// Determines whether the given actor is authorized to execute this command/query. | ||
| /// Return a success Result to proceed, or a failure Result (typically <see cref="Error.Forbidden(string, string?)"/>) | ||
| /// to short-circuit the pipeline. | ||
| /// Determines whether the actor is authorized to perform this operation | ||
| /// against the given resource. | ||
| /// </summary> | ||
| /// <param name="actor">The current authenticated actor.</param> | ||
| IResult Authorize(Actor actor); | ||
| /// <param name="resource">The loaded resource to authorize against.</param> | ||
| /// <returns> | ||
| /// A success result to proceed, or a failure result (typically | ||
| /// <see cref="Error.Forbidden(string, string?)"/>) to short-circuit the pipeline. | ||
| /// </returns> | ||
| IResult Authorize(Actor actor, TResource resource); | ||
| } No newline at end of file |
There was a problem hiding this comment.
This change replaces the non-generic IAuthorizeResource with the new generic IAuthorizeResource. That is a breaking change for any existing commands/queries implementing the old interface, and it also contradicts the PR description claiming the existing non-generic interface is unchanged. Consider restoring the original IAuthorizeResource (Authorize(Actor)) alongside the new generic interface (e.g., keep this as a new type/file), and keep both supported in Trellis.Mediator.
| // Resolve the scoped loader per-request (like middleware resolving scoped services) | ||
| var loader = _serviceProvider.GetRequiredService<IResourceLoader<TMessage, TResource>>(); | ||
|
|
There was a problem hiding this comment.
ResourceAuthorizationBehavior resolves IResourceLoader<TMessage,TResource> from an injected IServiceProvider. When this behavior is registered as Singleton (as done by AddResourceAuthorization), the injected IServiceProvider will be the root provider, so resolving a scoped loader here will either throw under scope validation or produce incorrect scoping/leaks (e.g., DbContext living for the app lifetime). Prefer making the behavior scoped and injecting IResourceLoader<TMessage,TResource> directly (or otherwise ensure the IServiceProvider is the current request scope).
| // as IPipelineBehavior<TMessage, TResponse> | ||
| var closedBehavior = behaviorDef.MakeGenericType(type, tResource, tResponse); | ||
| var closedPipeline = pipelineDef.MakeGenericType(type, tResponse); | ||
| services.AddSingleton(closedPipeline, closedBehavior); |
There was a problem hiding this comment.
The assembly-scanning AddResourceAuthorization registers closed behaviors as Singleton as well. If those behaviors resolve scoped IResourceLoader instances (as intended), this has the same scope/lifetime issue as the explicit registration. The closed behavior registrations here should match the loader scope (typically Scoped) so resolution happens within the request scope.
| services.AddSingleton(closedPipeline, closedBehavior); | |
| services.AddScoped(closedPipeline, closedBehavior); |
| Exception → Tracing → Logging → Authorization → ResourceAuthorization (actor-only) → Validation | ||
|
|
||
| Resource-based authorization with a loaded resource (`IAuthorizeResource<TResource>`) is auto-discovered via `AddResourceAuthorization(Assembly)`, or registered explicitly per-command via `AddResourceAuthorization<TMessage, TResource, TResponse>()` for AOT scenarios. |
There was a problem hiding this comment.
The documented pipeline order includes “ResourceAuthorization (actor-only)”, but the current default PipelineBehaviors list (and AddTrellisBehaviors) no longer includes any ResourceAuthorization behavior, and there is no actor-only IAuthorizeResource shown in the API reference. Update this pipeline order to match actual behavior registration (or reintroduce the actor-only authorization behavior/interface if it’s meant to remain supported).
| public Result<User> CreateUser( | ||
| string firstName, string lastName, string email) | ||
| => FirstName.TryCreate(firstName) |
There was a problem hiding this comment.
The sample method is named CreateUserAsync but it’s no longer async (returns Result and has no CancellationToken). This is confusing for readers and encourages inconsistent naming. Either rename it to CreateUser (sync) or change the sample back to returning Task<Result> (and include ct if that’s the intended pattern).
Adds IAuthorizeResource — resource-based authorization that receives a loaded resource for ownership checks, tenant isolation, and other data-dependent rules.
New Types
Trellis.Authorization
IAuthorizeResource — declares resource-based auth on a command/query via Authorize(Actor, TResource)
IResourceLoader<TMessage, TResource> — loads the resource for authorization (registered scoped)
ResourceLoaderById<TMessage, TResource, TId> — convenience base for extract-ID → call-repository pattern
Trellis.Mediator
ResourceAuthorizationBehavior<TMessage, TResource, TResponse> — pipeline behavior: loads resource → calls Authorize → short-circuits on failure
AddResourceAuthorization(Assembly) — scans for IAuthorizeResource commands and IResourceLoader<,> implementations
AddResourceAuthorization<TMessage, TResource, TResponse>() — explicit registration for AOT/trimming
AddResourceLoaders(Assembly) — standalone loader scanning
Design Decisions
3-param behavior — can't be open generic (IPipelineBehavior<,> has 2 params), so assembly scanning or explicit registration closes it per-command
Singleton behavior, scoped loader — behavior resolves IResourceLoader from IServiceProvider per-request (same pattern as ASP.NET Core middleware)
Safe scanning — catches ReflectionTypeLoadException; validates TResponse constraints before MakeGenericType
AOT-aware — scanning methods carry [RequiresUnreferencedCode] + [RequiresDynamicCode]; explicit overload is fully AOT-compatible