dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
34.59k stars 9.79k forks source link

Explicitly calling `context.Fail(....)` in authorization policy requirement handler results in empty `policyAuthorizationResult.AuthorizationFailure.FailedRequirements` #55673

Closed Xor-el closed 1 week ago

Xor-el commented 3 weeks ago

Is there an existing issue for this?

Describe the bug

Calling context.Fail(....) in our Authorization policy requirement handler causes policyAuthorizationResult.AuthorizationFailure.FailedRequirements to be empty when our Authorization fails.

Expected Behavior

policyAuthorizationResult.AuthorizationFailure.FailedRequirements should not be empty when Authorization fails regardless of whether we call context.Fail(....) in our policy requirement handler or not.

Steps To Reproduce

  1. Clone this repo, build and run.

  2. Initiate the request below using cURL, notice we get a 401 Unauthorized response.

curl --location 'https://localhost:7244/authbugdemo/implicit-fail' \
--header 'Content-Type: application/json' \
--header 'X-API-Key: BadAPIKey' \
--data '{
    "name": "Chucky",
    "description": "Child'\''s Play is an American slasher media franchise created by Don Mancini."
}'
  1. Initiate the request below using cURL, notice we get a 403 Forbidden response.
curl --location 'https://localhost:7244/authbugdemo/explicit-fail' \
--header 'Content-Type: application/json' \
--header 'X-API-Key: BadAPIKey' \
--data '{
    "name": "Chucky",
    "description": "Child'\''s Play is an American slasher media franchise created by Don Mancini."
}'
  1. the difference between the two endpoints are their Authorization policy requirement handlers.

    The implicit-fail endpoint policy requirement handler doesn't call context.Fail if authorization fails. however, the explicit-fail endpoint policy requirement handler calls context.Fail("....") when authorization fails.

  2. I have an IAuthorizationMiddlewareResultHandler implementation that checks if the failed requirement is of a particular type using policyAuthorizationResult.AuthorizationFailure.FailedRequirements and modifies the response status code appropriately.

unfortunately, policyAuthorizationResult.AuthorizationFailure.FailedRequirements is always empty when we use the explicit-fail policy requirement handler that calls context.Fail("....").

I don't see this behaviour documented anywhere so I assume it's a bug. If it is an expected behaviour (this would be strange :confused:), then how do we propagate the failure reason from the various policy requirement handlers to my IAuthorizationMiddlewareResultHandler without calling context.Fail("....")?

Exceptions (if any)

No response

.NET Version

8.0.200

Anything else?

No response

juchom commented 2 weeks ago

I'm just hitting the same issue.

There is nothing in the doc about this behavior : https://learn.microsoft.com/en-us/aspnet/core/security/authorization/customizingauthorizationmiddlewareresponse?view=aspnetcore-8.0

juchom commented 2 weeks ago

Ok, so after digging a bit more I found this :

When we call Fail, the authContext properties are HasSucceeded is false and HasFailed is true When we don't call Fail, the authContext properties are HasSucceeded is false and HasFailed is false

In the first case we go to line 20 In the second case we go to line 21

https://github.com/dotnet/aspnetcore/blob/e47c15abd3ae87aff8e04af5dac44847844460b9/src/Security/Authorization/Core/src/DefaultAuthorizationEvaluator.cs#L16-L21

Line 20 goes to this

https://github.com/dotnet/aspnetcore/blob/e47c15abd3ae87aff8e04af5dac44847844460b9/src/Security/Authorization/Core/src/AuthorizationFailure.cs#L44-L49

Line 21 goes to this

https://github.com/dotnet/aspnetcore/blob/e47c15abd3ae87aff8e04af5dac44847844460b9/src/Security/Authorization/Core/src/AuthorizationFailure.cs#L56-L57

So the question is why in case 1 static AuthorizationFailure.Failed is not taking a second argument with the failed requirements ?

halter73 commented 2 weeks ago

AuthorizationHandlerContext.Success(IAuthorizationRequirement requirement) takes a requirement while AuthorizationHandlerContext.Fail(AuthorizationFailureReason reason) takes a reason (or nothing if you call Fail()).

PolicyAuthorizationResult.AuthorizationFailure.FailedRequirements returns an IEnumerable<IAuthorizationRequirement>, but you did not provide any IAuthorizationRequirement when you called Fail(AuthorizationFailureReason reason), and there's no special stateful logic in AuthorizationHandlerContext to keep track of what requirement is being checked when Fail(AuthorizationFailureReason reason) or Fail() is called. These methods have no idea if any requirement is being checked at all when they get called.

You might think that FailedRequirements should include any requirement that didn't explicitly succeed (i.e. AuthorizationHandlerContext.PendingRequirements) like it does in the case when fail wasn't explicitly called, but this could easily be incorrect. It's very likely that whatever cased fail to be called made it impossible to check whether the pending requirements were actually checked or not. This is particularly true if AuthorizationOptions.InvokeHandlersAfterFailure is set to false, but that's not the only way this could happen.

By calling AuthorizationHandlerContext.Fail without any IAuthorizationRequirement, you're telling the DefaultAuthorizationEvaluator that the authorization failure was unrelated to the requirements. That's why the doc comments tell you that it is "Called to indicate HasSucceeded will never return true, even if all requirements are met." (emphasis mine). So, you're telling the system whether or not the ApiKeyExplicitFailRequirement succeeded is irrelevant to the failure.

You'll notice that we never call AuthorizationHandlerContext.Fail in our conceptual authorization docs. We also don't call it internally anywhere in our framework either because the only reason a built-in AuthorizationHandler can fail is because a requirement wasn't met. It might make sense to call Fail if the reason authorization failed is something more than simply not meeting a requirement like a backend identity provider going down, but that's not a concern for any of the built-in AuthorizationHandler's that only need to verify data already contained within the ClaimsPrincipal meets the expected requirements.

Why do you need to call AuthorizationHandlerContext.Fail? If you need to communicate details to the IAuthorizationMiddlewareResultHandler why authorization failed beyond the requirement simply not being met, why not check PolicyAuthorizationResult.AuthorizationFailure.FailureReasons instead?

The example given of an API key being invalid seems more like an authentication failure than an authorization failure anyway. I'd expect that to be reported by an API key IAuthenticationHandler in AuthenticateAsync() using AuthenticationResult.Fail(string failureMessage).

juchom commented 2 weeks ago

Thanks @halter73,

If I understand it correctly, when we write an AuthorizationHandler we have to follow this rules :

Succeed : means "Ok, this handler has met the required condition"

No call : means "This handler has not met the required condition"

Fail : means "An error has happened that prevent the handler to conclude if it succeeded or not"

halter73 commented 1 week ago

No problem. Your understanding looks correct to me.

Xor-el commented 1 week ago

Thanks @halter73 for the clarification. Apologies for the late response.