Allow more failure details for AuthZ handlers

[HaoK]

Today there's no way to signal anything other than which requirements have failed or an explicit Fail https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authorization/Core/src/AuthorizationHandlerContext.cs#L82

This makes it hard for a handler to flow more specific error information/details to the IAuthorizationMiddlewareResultHandler

[Tratcher]

New Overload Fail on AuthorizationHandlerContext to pass failure reason. New AuthorizationFailureReason base class has a string message and the handler which called failed, developers can inherit and pass their own derived failure reasons which can contain typed state as needed.

aspnetcore/src/Security/Authorization/Core/src/AuthorizationHandlerContext.cs

Line 82 in 302ed3c

public virtual void Fail()

+public class AuthorizationFailureReason
{
+ Encapsulates a reason why authorization failed
+ public AuthorizationFailureReason(IAuthorizationHandler handler, string message)
+ public string Message { get; set; }
+ public IAuthorizationHandler Handler { get; set; }

public class AuthorizationHandlerContext
{
+ // Calls Fail() and stores the failure state for future reference. Can be called multiple times.
+ public virtual void Fail(AuthorizationFailureReason);
+ public IEnumerable<AuthorizationFailureReason> FailureReasons { get; }

The IAuthorizationEvaluator checks for failure and bundles it into an AuthorizationFailure

aspnetcore/src/Security/Authorization/Core/src/DefaultAuthorizationEvaluator.cs

Lines 19 to 20 in 8b30d86

	: AuthorizationResult.Failed(context.HasFailed
	? AuthorizationFailure.ExplicitFail()

aspnetcore/src/Security/Authorization/Core/src/AuthorizationFailure.cs

Line 31 in 8b30d86

public static AuthorizationFailure ExplicitFail()

public class AuthorizationFailure
{
+ // Calls Fail and stores the failure reasons for future reference. Can be called multiple times.
+ public static void ExplicitFail(IEnumerable<AuthorizationFailureReason> reasons);
+ public IEnumerable<AuthorizationFailureReason> FailureStates { get; }

Which ends up on the AuthorizationResult:

aspnetcore/src/Security/Authorization/Core/src/AuthorizationResult.cs

Line 24 in 8b30d86

public AuthorizationFailure? Failure { get; private set; }

Which is then passed to the IAuthorizationMiddlewareResultHandler

aspnetcore/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs

Lines 105 to 107 in 302ed3c

	var authorizeResult = await policyEvaluator.AuthorizeAsync(policy, authenticateResult!, context, resource);
	var authorizationMiddlewareResultHandler = context.RequestServices.GetRequiredService<IAuthorizationMiddlewareResultHandler>();
	await authorizationMiddlewareResultHandler.HandleAsync(_next, context, policy, authorizeResult);

aspnetcore/src/Security/Authorization/Policy/src/IAuthorizationMiddlewareResultHandler.cs

Line 24 in 302ed3c

Task HandleAsync(RequestDelegate next, HttpContext context, AuthorizationPolicy policy, PolicyAuthorizationResult authorizeResult);

Sample consumption: https://github.com/dotnet/aspnetcore/blob/main/src/Security/samples/CustomAuthorizationFailureResponse/Authorization/SampleAuthorizationMiddlewareResultHandler.cs#L36

Full PR: #35425

[HaoK]

@Tratcher @blowdart how do you feel about ExplicitFail/States => FailureReason(s)?

[HaoK]

I don't think it necessarily makes sense to force correlation of requirements/reasons, but we could do Handler -> reasons, since that is really the split, but the failure state should be the mechanism to communicate information, if the handler wants to enumerate the requirements that failed, they are free to put that in the failure reason explicitly, but that mapping is completely up to the app/handlers

[blowdart]

State has multiple meanings, I'd prefer reasons. But object feels, odd to me. I'd suggest an interface here with, at least, a message property, so folks don't have to cast around if they don't want to?

As an aside aren't there old issues about this? We should link them.

[HaoK]

Isn't the argument against interfaces future breaking change concerns? We could do a base FailureReason class which has a message property?

[davidfowl]

Did this get API reviewed?

[HaoK]

Oops, no I'll add the labels

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[pranavkm]

• Remove setters from properties.

public class AuthorizationFailureReason
{
+   public AuthorizationFailureReason(IAuthorizationHandler handler, string message)
+   public string Message { get; }
+   public IAuthorizationHandler Handler { get; }
}

API consideration: Can this type be sealed and use a `IDictionary` to pass additional data instead e.g.

```diff
+public sealed class AuthorizationFailureReason
{
+   public AuthorizationFailureReason(IAuthorizationHandler handler, string message)
+   public string Message { get; }
+   public IAuthorizationHandler Handler { get; }
+   public IDictionary<object, object> Properties { get; } 
}

AuthorizationFailure

• Update property name to FailureReasons for consistency:

public class AuthorizationFailure
{
+ // Calls Fail and stores the failure reasons for future reference. Can be called multiple times.
+ public static void ExplicitFail(IEnumerable<AuthorizationFailureReason> reasons);
+ public IEnumerable<AuthorizationFailureReason> FailureReasons { get; }

[pranavkm]

@blowdart / @adityamandaleeka is someone on point to update the PR based on the API feedback?

[blowdart]

If hao isn't here @Tratcher i'd guess

[Kahbazi]

@pranavkm I'm interested in doing this. May I send the PR?

[pranavkm]

We'd appreciate that @Kahbazi. Thanks!

[Tratcher]

A) There's an RC1 PR here that needs updating: #35529. @Kahbazi you'll need to send a PR to that branch.
B) Main also needs updating.
C) There's an outstanding point to resolve first:

API consideration: Can this type be sealed and use a `IDictionary` to pass additional data instead e.g.

@blowdart? When designing this we specifically discussed an unsealed type that could be derived from to add strongly typed data. Using a loosely typed dictionary didn't come up. Either way requires downcasting, though with the derived type it would only be a single downcast instead of per property.

[blowdart]

A single cast seems the better approach, less error prone and faster.

[Tratcher]

+ public static void ExplicitFail(IEnumerable<AuthorizationFailureReason> reasons);

This is outdated, the name used in the PR is:

+ public static void Fail(IEnumerable<AuthorizationFailureReason> reasons);

[davidfowl]

Are we sure we want IEnumerable<T> , allocations...