Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task done
Xor-el opened this issue May 11, 2024 · 6 comments

Comments

@Xor-el
Copy link

Xor-el commented May 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues

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 😕), 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

@Xor-el Xor-el changed the title Explicitly calling context.Fail in Authorization Policy Requirement Handler Results in empty policyAuthorizationResult.AuthorizationFailure.FailedRequirements Explicitly calling context.Fail(....) in Authorization Policy Requirement Handler Results in empty policyAuthorizationResult.AuthorizationFailure.FailedRequirements May 11, 2024
@Xor-el Xor-el changed the title Explicitly calling context.Fail(....) in Authorization Policy Requirement Handler Results in empty policyAuthorizationResult.AuthorizationFailure.FailedRequirements Explicitly calling context.Fail(....) in authorization policy requirement handler results in empty policyAuthorizationResult.AuthorizationFailure.FailedRequirements May 11, 2024
@juchom
Copy link

juchom commented May 13, 2024

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
Copy link

juchom commented May 15, 2024

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

public AuthorizationResult Evaluate(AuthorizationHandlerContext context)
=> context.HasSucceeded
? AuthorizationResult.Success()
: AuthorizationResult.Failed(context.HasFailed
? AuthorizationFailure.Failed(context.FailureReasons)
: AuthorizationFailure.Failed(context.PendingRequirements));

Line 20 goes to this

public static AuthorizationFailure Failed(IEnumerable<AuthorizationFailureReason> reasons)
=> new AuthorizationFailure
{
FailCalled = true,
FailureReasons = reasons
};

Line 21 goes to this

public static AuthorizationFailure Failed(IEnumerable<IAuthorizationRequirement> failed)
=> new AuthorizationFailure { FailedRequirements = failed };

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

@halter73
Copy link
Member

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
Copy link

juchom commented May 18, 2024

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
Copy link
Member

No problem. Your understanding looks correct to me.

@Xor-el
Copy link
Author

Xor-el commented May 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants