Skip to content

Unified async IActorProvider, ClaimsActorProvider, SSO sample#356

Merged
xavierjohn merged 11 commits intomainfrom
dev/xavier/rr10
Mar 29, 2026
Merged

Unified async IActorProvider, ClaimsActorProvider, SSO sample#356
xavierjohn merged 11 commits intomainfrom
dev/xavier/rr10

Conversation

@xavierjohn
Copy link
Copy Markdown
Owner

Unifies the actor provider system into a single async interface, adds generic OIDC support via
ClaimsActorProvider, fixes 3 security vulnerabilities found by multi-model review, and ships an SSO sample
app. 40 files changed, +1328/-754.

⚠️ Breaking Change

IActorProvider is now async-only. IAsyncActorProvider is deleted.

// Before (two interfaces):
public interface IActorProvider { Actor GetCurrentActor(); }
public interface IAsyncActorProvider { Task GetCurrentActorAsync(CancellationToken ct); }

// After (single interface):
public interface IActorProvider { Task GetCurrentActorAsync(CancellationToken ct); }

All implementations, pipeline behaviors, and test fakes updated.

✨ New: ClaimsActorProvider — generic OIDC/JWT actor provider

Works with any identity provider (Microsoft Entra, Google, Auth0, Okta, etc.). Configurable via
ClaimsActorOptions:

services.AddClaimsActorProvider(options =>
{
options.ActorIdClaim = "sub";
options.PermissionsClaim = "permissions";
});

EntraActorProvider refactored as subclass with Entra-specific delegate-based mapping (permissions, forbidden
permissions, ABAC attributes). All existing EntraActorOptions functionality preserved.

✨ New: CachingActorProvider — per-request cache decorator

For DB-backed providers that augment claims with database permissions:

services.AddCachingActorProvider();

Caches the Task per scope — concurrent callers share the same in-flight operation.

✨ New: SSO Sample (Examples/SsoExample/)

Minimal ASP.NET Core API demonstrating ClaimsActorProvider with multiple providers:

  • Development profile (:5050) — X-Test-Actor header mode
  • Production profile (:5051) — real JWT required
  • README with setup for Microsoft Entra, Google, Auth0, generic OIDC
  • Token acquisition docs (Azure CLI, Google ID token, Auth0 dashboard)

xavierjohn and others added 2 commits March 29, 2026 09:09
BREAKING: Merged IActorProvider (sync) and IAsyncActorProvider (async) into
single async IActorProvider with Task<Actor> GetCurrentActorAsync().
Deleted IAsyncActorProvider. Simplified AuthorizationBehavior and
ResourceAuthorizationBehavior (no dual-path logic).

New providers:
- ClaimsActorProvider: generic OIDC/JWT provider with ClaimsActorOptions
  (ActorIdClaim, PermissionsClaim). Works with any identity provider.
- CachingActorProvider: per-request cache decorator for DB-backed providers.
  Registered via AddCachingActorProvider<T>().

Refactored:
- EntraActorProvider: extends ClaimsActorProvider with Entra-specific
  delegate-based mapping (permissions, forbidden, attributes).
- DevelopmentActorProvider: returns Task.FromResult, stays separate.
- TestActorProvider: async interface, WithActor scope preserved.

Security fixes (found by 3-model security review — GPT-5.4, Opus, Sonnet):
- S1: Multi-identity claim spoofing — providers now read claims from
  authenticated identity only, not ClaimsPrincipal-wide APIs
- S2: DevelopmentActorProvider production guard — throws unconditionally
  in non-Development environments (was only guarding header path)
- S3: AuthorizationBehavior permission leak — generic 'Insufficient
  permissions' message instead of listing specific permission names

Documentation: copilot instructions, API reference, testing reference
all updated. 27 files changed, 7 new security tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Examples/SsoExample: minimal ASP.NET Core API demonstrating
ClaimsActorProvider with Microsoft Entra, Google, Auth0, and
generic OIDC providers. Configuration-driven — swap providers
by editing appsettings.json.

Features:
- GET /api/me endpoint with [Authorize] returning actor info
- Development profile (port 5050): X-Test-Actor header mode
- Production profile (port 5051): real JWT required
- README with setup docs for Microsoft, Google, Auth0, generic OIDC
- Token acquisition steps (Azure CLI, Google ID token, Auth0 dashboard)

Added to Trellis.slnx solution and Directory.Packages.props
(Microsoft.AspNetCore.Authentication.JwtBearer).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 29, 2026

Test Results

4 145 tests  +11   4 130 ✅ +11   3m 49s ⏱️ -28s
   14 suites ± 0      15 💤 ± 0 
   14 files   ± 0       0 ❌ ± 0 

Results for commit 47c4149. ± Comparison against base commit dac8114.

This pull request removes 45 and adds 56 tests. Note that renamed tests count towards both.
Trellis.Asp.Authorization.Tests.DevelopmentActorProviderTests ‑ GetCurrentActor_Production_WithoutHeader_ReturnsDefaultActor
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_CustomIdClaimType_UsesConfiguredClaim
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_CustomMapAttributes_UsesDelegate
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_CustomMapAttributes_WhenDelegateThrows_WrapsExceptionWithContext
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_CustomMapForbiddenPermissions_UsesDelegate
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_CustomMapForbiddenPermissions_WhenDelegateThrows_WrapsExceptionWithContext
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_CustomMapPermissions_UsesDelegate
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_CustomMapPermissions_WhenDelegateThrows_WrapsExceptionWithContext
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_DefaultMapping_AlsoMapsClaimTypesRoleClaim
Trellis.Asp.Authorization.Tests.EntraActorProviderTests ‑ GetCurrentActor_DefaultMapping_ExtractsAuthContextClassReference
…
Trellis.Asp.Authorization.Tests.CachingActorProviderTests ‑ GetCurrentActorAsync_CachesResultAcrossCalls
Trellis.Asp.Authorization.Tests.CachingActorProviderTests ‑ GetCurrentActorAsync_InnerReceivesRequestAbortedToken
Trellis.Asp.Authorization.Tests.CachingActorProviderTests ‑ GetCurrentActorAsync_NoHttpContext_InnerReceivesNone
Trellis.Asp.Authorization.Tests.CachingActorProviderTests ‑ GetCurrentActorAsync_ReturnsActorFromInnerProvider
Trellis.Asp.Authorization.Tests.ClaimsActorProviderTests ‑ GetCurrentActorAsync_CustomActorIdClaim_UsesConfiguredClaim
Trellis.Asp.Authorization.Tests.ClaimsActorProviderTests ‑ GetCurrentActorAsync_CustomPermissionsClaim_UsesConfiguredClaim
Trellis.Asp.Authorization.Tests.ClaimsActorProviderTests ‑ GetCurrentActorAsync_DefaultOptions_AttributesIsEmpty
Trellis.Asp.Authorization.Tests.ClaimsActorProviderTests ‑ GetCurrentActorAsync_DefaultOptions_ForbiddenPermissionsIsEmpty
Trellis.Asp.Authorization.Tests.ClaimsActorProviderTests ‑ GetCurrentActorAsync_DefaultOptions_MapsPermissionsClaims
Trellis.Asp.Authorization.Tests.ClaimsActorProviderTests ‑ GetCurrentActorAsync_DefaultOptions_NoPermissionsClaims_ReturnsEmptyPermissions
…

♻️ This comment has been updated with latest results.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Unifies the authorization actor-provider model around a single async IActorProvider, adds a generic OIDC/JWT ClaimsActorProvider, introduces a per-scope CachingActorProvider decorator, and updates docs/tests/examples (including a new SSO sample) to match the breaking change.

Changes:

  • Replace sync/async dual interfaces with a single async-only IActorProvider across core, mediator behaviors, fakes, and docs.
  • Add ClaimsActorProvider (generic claim mapping) and CachingActorProvider (per-scope caching) plus DI helpers and tests.
  • Add an SSO example app and update integration guides/API references accordingly.

Reviewed changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
trellis-api-testing-reference.md Updates testing docs for async-only IActorProvider and adds guidance for testing claims/caching providers.
trellis-api-reference.md Updates API reference for unified async actor provider and documents new ASP authorization providers/options.
docs/docfx_project/articles/integration-db-permissions.md Updates DB-permissions guide to use async IActorProvider and the new caching decorator registration.
docs/docfx_project/articles/integration-asp-authorization.md Updates ASP authorization article to reference caching decorator instead of the removed async interface.
Trellis.slnx Adds the new Examples/SsoExample project to the solution.
Trellis.Testing/tests/Fakes/TestActorProviderTests.cs Converts tests to async-only actor provider API.
Trellis.Testing/src/Fakes/TestActorProvider.cs Removes sync API + async interface implementation; TestActorProvider now implements async-only IActorProvider.
Trellis.Testing/README.md Updates README to reference GetCurrentActorAsync() instead of GetCurrentActor().
Trellis.Mediator/tests/ResourceAuthorizationBehaviorTests.cs Updates tests for unified async provider and adds cancellation-token propagation coverage.
Trellis.Mediator/tests/Helpers/FakeAsyncActorProvider.cs Removes fake async-provider helper (obsolete after interface unification).
Trellis.Mediator/tests/Helpers/FakeActorProvider.cs Updates fake provider to implement async-only IActorProvider.
Trellis.Mediator/tests/AuthorizationBehaviorTests.cs Updates authorization tests for unified async provider and adjusted forbidden error detail.
Trellis.Mediator/src/ResourceAuthorizationBehavior.cs Refactors pipeline behavior to use only async IActorProvider and adds null-actor guard.
Trellis.Mediator/src/AuthorizationBehavior.cs Refactors pipeline behavior to use only async IActorProvider and updates forbidden error detail.
Trellis.Mediator/README.md Updates documentation sample to implement async-only IActorProvider.
Trellis.Authorization/src/IAsyncActorProvider.cs Deletes the old async actor provider interface.
Trellis.Authorization/src/IActorProvider.cs Changes IActorProvider to async-only (GetCurrentActorAsync).
Trellis.Authorization/README.md Updates package docs to use async-only IActorProvider.
Trellis.Authorization/NUGET_README.md Updates NuGet README sample to use async-only IActorProvider.
Trellis.Asp.Authorization/tests/ServiceCollectionExtensionsTests.cs Adds coverage for AddClaimsActorProvider and AddCachingActorProvider<T>().
Trellis.Asp.Authorization/tests/EntraActorProviderTests.cs Updates Entra provider tests to async API and adds multi-identity spoofing coverage.
Trellis.Asp.Authorization/tests/DevelopmentActorProviderTests.cs Updates dev provider tests to async API and tighter environment guard behavior.
Trellis.Asp.Authorization/tests/ClaimsActorProviderTests.cs Adds tests for generic claims-based provider including spoofing scenarios and errors.
Trellis.Asp.Authorization/tests/CachingActorProviderTests.cs Adds tests for per-scope caching decorator behavior and token propagation.
Trellis.Asp.Authorization/src/ServiceCollectionExtensions.cs Adds DI registration helpers for ClaimsActorProvider and CachingActorProvider.
Trellis.Asp.Authorization/src/EntraActorProvider.cs Refactors Entra provider as a ClaimsActorProvider subclass with Entra-specific mapping.
Trellis.Asp.Authorization/src/DevelopmentActorProvider.cs Updates dev provider to async-only API and strengthens non-development guard.
Trellis.Asp.Authorization/src/ClaimsActorProvider.cs Introduces generic OIDC/JWT claims-to-Actor provider + options.
Trellis.Asp.Authorization/src/CachingActorProvider.cs Introduces per-scope caching decorator for IActorProvider.
Examples/SsoExample/appsettings.json Adds SSO example configuration for authority/audience and claim mapping.
Examples/SsoExample/appsettings.Development.json Adds development logging overrides for the SSO example.
Examples/SsoExample/SsoExample.csproj New SSO example ASP.NET Core project with JwtBearer dependency.
Examples/SsoExample/README.md Adds setup and usage docs for the SSO example across multiple providers.
Examples/SsoExample/Properties/launchSettings.json Adds launch profiles for dev/prod SSO example modes.
Examples/SsoExample/Program.cs Configures JwtBearer auth and selects dev vs claims actor provider by environment.
Examples/SsoExample/Controllers/MeController.cs Adds an authenticated endpoint to display current actor details.
Examples/AuthorizationExample/MediatorExample.cs Updates example handler to use async IActorProvider.
Examples/AuthorizationExample/Actors.cs Updates in-memory provider to implement async-only IActorProvider.
Directory.Packages.props Adds centralized package version for Microsoft.AspNetCore.Authentication.JwtBearer.
.github/copilot-instructions.md Updates repo instructions/docs to reflect unified async actor provider and caching decorator guidance.

Comment on lines +67 to +83
public virtual Task<Actor> GetCurrentActorAsync(CancellationToken cancellationToken = default)
{
var user = HttpContextAccessor.HttpContext?.User;
var identity = user?.Identities.FirstOrDefault(i => i.IsAuthenticated) as ClaimsIdentity
?? throw new InvalidOperationException(
"No authenticated user. Ensure authentication middleware runs before actor resolution.");

var actorId = identity.FindFirst(Options.ActorIdClaim)?.Value
?? throw new InvalidOperationException(
$"Claim '{Options.ActorIdClaim}' not found in the authenticated user's claims.");

var permissions = identity.FindAll(Options.PermissionsClaim)
.Select(c => c.Value)
.ToFrozenSet();

var actor = Actor.Create(actorId, permissions);
return Task.FromResult(actor);
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCurrentActorAsync accepts a CancellationToken but never observes it. Even though this implementation is currently synchronous, it should still respect cancellation (e.g., throw early if canceled) to align with the unified async contract and avoid analyzers flagging the unused parameter.

Copilot uses AI. Check for mistakes.
/// <inheritdoc />
public virtual Task<Actor> GetCurrentActorAsync(CancellationToken cancellationToken = default)
{
var user = HttpContextAccessor.HttpContext?.User;
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception message for the HttpContext == null case is currently "No authenticated user...", but the actual failure can be "no HttpContext available" (e.g., called outside a request). Consider making the error message distinguish between missing HttpContext vs unauthenticated user to improve diagnosability.

Suggested change
var user = HttpContextAccessor.HttpContext?.User;
var httpContext = HttpContextAccessor.HttpContext
?? throw new InvalidOperationException(
"No HttpContext available. ClaimsActorProvider can only be used within an active HTTP request.");
var user = httpContext.User;

Copilot uses AI. Check for mistakes.
/// builder.Services.AddClaimsActorProvider(opts =>
/// {
/// opts.ActorIdClaim = "sub";
/// opts.PermissionsClaim = "realm_access.roles";
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Keycloak example sets PermissionsClaim = "realm_access.roles", but Keycloak typically emits realm_access as a JSON object claim (with a roles array) rather than a flattened claim type, so ClaimsIdentity.FindAll("realm_access.roles") will not return anything by default. Consider updating the example to a claim that actually appears as a flat claim type (e.g., mapping roles into roles/permissions at the token validation layer) or document that nested JSON claims require custom parsing.

Suggested change
/// opts.PermissionsClaim = "realm_access.roles";
/// // Assumes Keycloak/token validation maps realm_access.roles into a flat "roles" claim
/// opts.PermissionsClaim = "roles";

Copilot uses AI. Check for mistakes.
Comment on lines +49 to +81
public override Task<Actor> GetCurrentActorAsync(CancellationToken cancellationToken = default)
{
var httpContext = httpContextAccessor.HttpContext
var httpContext = HttpContextAccessor.HttpContext
?? throw new InvalidOperationException(
"No HttpContext available. Ensure this is called within an HTTP request scope.");

var user = httpContext.User;

if (user.Identity?.IsAuthenticated != true)
throw new InvalidOperationException(
var identity = user.Identities.FirstOrDefault(i => i.IsAuthenticated) as ClaimsIdentity
?? throw new InvalidOperationException(
"No authenticated user. Ensure authentication middleware runs before actor resolution.");

var config = options.Value;
var claims = user.Claims;
var claims = identity.Claims;

var id = ResolveActorId(user, config)
var id = ResolveActorId(identity, _entraOptions)
?? throw new InvalidOperationException(
$"Claim '{config.IdClaimType}' not found in the authenticated user's claims. " +
$"Claim '{_entraOptions.IdClaimType}' not found in the authenticated user's claims. " +
"Verify the token configuration or set EntraActorOptions.IdClaimType.");

var permissions = InvokeMapping(
"MapPermissions",
() => config.MapPermissions(claims));
() => _entraOptions.MapPermissions(claims));

var forbiddenPermissions = InvokeMapping(
"MapForbiddenPermissions",
() => config.MapForbiddenPermissions(claims));
() => _entraOptions.MapForbiddenPermissions(claims));

var attributes = InvokeMapping(
"MapAttributes",
() => config.MapAttributes(claims, httpContext));
() => _entraOptions.MapAttributes(claims, httpContext));

return new Actor(id, permissions, forbiddenPermissions, attributes);
var actor = new Actor(id, permissions, forbiddenPermissions, attributes);
return Task.FromResult(actor);
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCurrentActorAsync accepts a CancellationToken but does not observe it. Even if this provider is currently synchronous, honoring cancellation (e.g., early ThrowIfCancellationRequested) keeps behavior consistent with the unified async actor-provider contract and avoids unused-parameter analyzer noise.

Copilot uses AI. Check for mistakes.
Comment on lines +121 to +130
### Google (OAuth2 Playground)

1. Go to https://developers.google.com/oauthplayground
2. Select the scopes your app needs
3. Exchange the authorization code for tokens
4. Copy the `access_token` and use it:

```bash
curl http://localhost:5051/api/me -H "Authorization: Bearer {access-token}"
```
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Google section says OAuth2 Playground issues opaque access tokens and recommends using the ID token, but the later curl example instructs copying the access_token and sending it as the Bearer token. Please align these instructions (e.g., use id_token in the example, or clarify when an access token is a JWT vs opaque).

Copilot uses AI. Check for mistakes.
Comment on lines +19 to +23
private Task<Actor>? _cachedTask;

/// <inheritdoc />
public Task<Actor> GetCurrentActorAsync(CancellationToken cancellationToken = default) =>
_cachedTask ??= inner.GetCurrentActorAsync(cancellationToken);
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CachingActorProvider caches the first in-flight Task via _cachedTask ??= ..., which is not concurrency-safe: two concurrent callers in the same scope can both observe _cachedTask as null and invoke the inner provider twice. Also, caching a task created with one caller’s CancellationToken can cause the cached operation to be canceled for other callers (or later callers’ tokens to be ignored). Consider using an atomic initialization (e.g., Interlocked.CompareExchange/LazyInitializer/Lazy<Task<Actor>>) and decouple caller cancellation from the shared resolution (e.g., cache using CancellationToken.None and apply cancellation only to the wait).

Copilot uses AI. Check for mistakes.
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2026

Codecov Report

❌ Patch coverage is 95.55556% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.33%. Comparing base (dac8114) to head (47c4149).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...p.Authorization/src/ServiceCollectionExtensions.cs 76.92% 3 Missing ⚠️
...llis.Asp.Authorization/src/CachingActorProvider.cs 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #356   +/-   ##
=======================================
  Coverage   85.33%   85.33%           
=======================================
  Files         203      205    +2     
  Lines        7144     7174   +30     
  Branches     1391     1398    +7     
=======================================
+ Hits         6096     6122   +26     
- Misses        653      656    +3     
- Partials      395      396    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ClaimsActorProvider, DevelopmentActorProvider, EntraActorProvider now
call cancellationToken.ThrowIfCancellationRequested() at entry point.
Aligns with the unified async IActorProvider contract.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xavierjohn and others added 2 commits March 29, 2026 10:21
ClaimsActorProvider: distinct error for missing HttpContext vs
unauthenticated user (was same message for both).

SSO sample README: Google section now consistently uses id_token
(not access_token). Removed contradictory instructions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use LazyInitializer.EnsureInitialized for atomic initialization
  (concurrent callers share the same in-flight task)
- Pass CancellationToken.None to inner provider so one caller's
  cancellation doesn't cancel the shared resolution
- Caller cancellation applied via Task.WaitAsync on the await only

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 5 comments.

@@ -1,4 +1,4 @@
namespace Trellis.Testing.Tests.Fakes;
namespace Trellis.Testing.Tests.Fakes;
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file appears to have lost its UTF-8 BOM (other .cs files in the repo still show the BOM marker at the start of line 1). The repo’s guidance requires UTF-8 with BOM (see .github/copilot-instructions.md). Please re-save/normalize this file with a BOM to keep encoding consistent.

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,4 @@
namespace Trellis.Asp.Authorization.Tests;
namespace Trellis.Asp.Authorization.Tests;
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file appears to have lost its UTF-8 BOM (other .cs files in the repo still show the BOM marker at the start of line 1). The repo’s guidance requires UTF-8 with BOM (see .github/copilot-instructions.md). Please re-save/normalize this file with a BOM to keep encoding consistent.

Suggested change
namespace Trellis.Asp.Authorization.Tests;
namespace Trellis.Asp.Authorization.Tests;

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,4 @@
namespace Trellis.Asp.Authorization.Tests;
namespace Trellis.Asp.Authorization.Tests;
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file appears to have lost its UTF-8 BOM (other .cs files in the repo still show the BOM marker at the start of line 1). The repo’s guidance requires UTF-8 with BOM (see .github/copilot-instructions.md). Please re-save/normalize this file with a BOM to keep encoding consistent.

Suggested change
namespace Trellis.Asp.Authorization.Tests;
namespace Trellis.Asp.Authorization.Tests;

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +28
// Authentication — configure one or more OIDC providers
builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
.AddJwtBearer(options =>
{
options.Authority = authSection["Authority"];
options.Audience = authSection["Audience"];
});

// Trellis actor provider — reads standard OIDC claims
if (builder.Environment.IsDevelopment())
{
builder.Services.AddDevelopmentActorProvider();
}
else
{
builder.Services.AddClaimsActorProvider(options =>
{
options.ActorIdClaim = authSection["ActorIdClaim"] ?? "sub";
options.PermissionsClaim = authSection["PermissionsClaim"] ?? "permissions";
});
}
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Development, the sample still configures JWT bearer authentication and the controller is marked [Authorize], so requests using only the X-Test-Actor header will be rejected with 401 before DevelopmentActorProvider is ever used (contradicts the README quick start). Consider either (1) using a development-only authentication scheme that always succeeds, (2) conditionally disabling [Authorize]/adding AllowAnonymous for this endpoint in Development, or (3) conditionally skipping JWT auth registration/pipeline in Development and using an explicit dev authorization policy.

Copilot uses AI. Check for mistakes.
Comment on lines +24 to +32
// Use CancellationToken.None for the shared resolution to avoid one caller's
// cancellation affecting others. Caller cancellation is applied to the wait only.
var task = LazyInitializer.EnsureInitialized(
ref _cachedTask,
() => inner.GetCurrentActorAsync(CancellationToken.None));

// If the caller's token is cancelable, wrap so their cancellation doesn't
// cancel the shared task — only their await.
return cancellationToken.CanBeCanceled
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CachingActorProvider always calls the inner provider with CancellationToken.None. This prevents request/operation cancellation from propagating to expensive work (e.g., DB queries) when the request is aborted, which can waste resources under load. A safer pattern is to pass a per-scope token (e.g., the first caller’s token, or a token tied to HttpContext.RequestAborted) into the cached task and only apply per-caller cancellation to the await when tokens differ.

Suggested change
// Use CancellationToken.None for the shared resolution to avoid one caller's
// cancellation affecting others. Caller cancellation is applied to the wait only.
var task = LazyInitializer.EnsureInitialized(
ref _cachedTask,
() => inner.GetCurrentActorAsync(CancellationToken.None));
// If the caller's token is cancelable, wrap so their cancellation doesn't
// cancel the shared task — only their await.
return cancellationToken.CanBeCanceled
// Use the first caller's cancellation token (when available) for the shared
// resolution, so request/operation cancellation can propagate to expensive work.
// Caller-specific cancellation is still applied only to the wait, not the task.
var initializationToken = cancellationToken.CanBeCanceled
? cancellationToken
: CancellationToken.None;
var task = LazyInitializer.EnsureInitialized(
ref _cachedTask,
() => inner.GetCurrentActorAsync(initializationToken));
// If the caller's token is cancelable and differs from the initialization token,
// wrap so their cancellation doesn't cancel the shared task — only their await.
return cancellationToken.CanBeCanceled && !cancellationToken.Equals(initializationToken)

Copilot uses AI. Check for mistakes.
xavierjohn and others added 2 commits March 29, 2026 11:09
JWT bearer authentication is now only registered in Production.
In Development, requests go directly to DevelopmentActorProvider
(X-Test-Actor header) without authentication middleware blocking them.

Replaced [Authorize] on controller with a fallback authorization policy
that requires authenticated users only in Production.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ution

Inner provider now receives HttpContext.RequestAborted instead of
CancellationToken.None, so expensive work (DB queries) cancels when
the HTTP request ends. Individual caller tokens are applied to the
await only via Task.WaitAsync, preventing one caller's cancellation
from affecting others.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.

Comment on lines +206 to +208
var inner = new TestActorProvider("user-1", "orders:read");
var caching = new CachingActorProvider(inner);
var actor = await caching.GetCurrentActorAsync(); // calls inner once, caches
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example constructs new CachingActorProvider(inner) but CachingActorProvider now requires an IHttpContextAccessor in its constructor. Update the snippet to either pass a HttpContextAccessor (with an appropriate HttpContext) or show DI usage via AddCachingActorProvider<T>() so the sample compiles.

Copilot uses AI. Check for mistakes.
Comment on lines +1465 to +1468
## CachingActorProvider (Decorator)

Caching decorator that wraps any `IActorProvider` and caches the result per-scope. Use with database-backed providers to avoid redundant queries within a single request.

Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this document’s DevelopmentActorProvider section (above), the bullets still state it only throws when the X-Test-Actor header is present in Production. The provider now throws unconditionally in any non-Development environment, so that description should be updated to avoid misleading consumers about production behavior.

Copilot uses AI. Check for mistakes.
Comment on lines 237 to 241

public class DatabaseActorProvider(
IHttpContextAccessor httpContextAccessor,
IPermissionRepository permissionRepository,
IMemoryCache cache) : IAsyncActorProvider
IPermissionRepository permissionRepository) : IActorProvider
{
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the DatabaseActorProvider example just above this class declaration, the snippet still imports Microsoft.Extensions.Caching.Memory even though IMemoryCache was removed from the provider. Please remove that unused import from the doc snippet so the guide stays consistent with the updated code.

Copilot uses AI. Check for mistakes.
services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
.AddJwtBearer(options => configuration.Bind("AzureAd", options));
services.AddEntraActorProvider(); // Required IActorProvider for AuthorizationBehavior
services.AddMemoryCache();
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DI snippet still registers services.AddMemoryCache(); even though the updated guidance uses AddCachingActorProvider<T>() (per-request caching) and no longer shows an IMemoryCache dependency. Either remove AddMemoryCache() here or explain what it is still used for to avoid confusing readers.

Suggested change
services.AddMemoryCache();

Copilot uses AI. Check for mistakes.
xavierjohn and others added 2 commits March 29, 2026 11:55
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lopment

API reference still said it only throws when X-Test-Actor header is
present in Production. Updated to reflect the S2 security fix:
throws unconditionally in any non-Development environment.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.


var app = builder.Build();

app.UseAuthentication();
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app.UseAuthentication() is called unconditionally, but in the Development branch the app never calls AddAuthentication(). In ASP.NET Core this typically throws at startup (missing authentication services). Consider either calling builder.Services.AddAuthentication() in Development as well (without registering JwtBearer), or only calling UseAuthentication() in the non-Development branch.

Suggested change
app.UseAuthentication();
if (!app.Environment.IsDevelopment())
app.UseAuthentication();

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,4 @@
namespace Trellis.Asp.Authorization.Tests;
namespace Trellis.Asp.Authorization.Tests;
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file no longer appears to be UTF-8 with BOM (the BOM marker was removed in the diff). The repo’s .editorconfig requires charset = utf-8-bom for [*] (see .editorconfig:6). Please re-save this file with a BOM to match repository encoding conventions.

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,4 @@
namespace Trellis.Asp.Authorization.Tests;
namespace Trellis.Asp.Authorization.Tests;
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file no longer appears to be UTF-8 with BOM (the BOM marker was removed in the diff). The repo’s .editorconfig requires charset = utf-8-bom for [*] (see .editorconfig:6). Please re-save this file with a BOM to match repository encoding conventions.

Copilot uses AI. Check for mistakes.
xavierjohn and others added 2 commits March 29, 2026 12:12
UseAuthentication() requires AddAuthentication() which is only
registered in the Production branch. Calling it unconditionally
throws at startup in Development mode.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All source files in Examples/SsoExample were missing BOM marker
required by .editorconfig (charset = utf-8-bom).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@xavierjohn xavierjohn merged commit 61776a4 into main Mar 29, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants