AuthorizeFilter prevents unauthenticated requests to be handled by IAuthorizationService

[malekpour]

Current implementation of AuthorizeFilter only calls IAuthorizationService.AuthorizeAsync when request is authenticated.

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Internal/AuthorizeFilter.cs#L78

I think DenyAnonymousAuthorizationRequirement should be used instead, otherwise there is no way to use authorization requirements without being authenticated.

[Eilon]

@blowdart @Tratcher thoughts?

[Tratcher]

@HaoK?

[blowdart]

By design. Why go into policies when you don't have an identity to evaluate anyway?

[kevinchalet]

By design.

Bad design then. The authorization service is already responsible of determining whether unauthenticated requests (= no principal/identity) are acceptable or not depending on the specific policy that applies, so there's no reason to have a duplicated logic in MVC.

What's the whole point of DenyAnonymousAuthorizationRequirement and policy.RequiredAuthenticatedUser() if the authorization service is not invoked when the user is not authenticated?

[malekpour]

For example you can have a requirement to grant or deny access by IP address only or you may grant guest users access in restricted hours.

Conceptually, in real world, there are situations that we are authorized to do things without being identified.

[blowdart]

For your IP example you're now attempt to authorize on something that isn't an identity at all. So you either write an authentication middleware which builds an identity from an IP address, or you go the filter route.

[malekpour]

This authentication vs. authorization has always been controversial. I've seen systems using IP, MAC address or computer name to identify and authorize group of users (e.g. Kiosk devices).

Considering the modular nature of AspNetCore, it does not make sense if MVC handles security, since the security project can take care of it very well.

In fact unauthenticated users have their own identity which is Anonymous. Lets have security project take care of them. The following code block functionality in the AuthorizeFilter can be replaced by a new requirement like AllowAnonymousAuthorizationRequirement

// Allow Anonymous skips all authorization
if (context.Filters.Any(item => item is IAllowAnonymousFilter))
{
    return;
}

[kevinchalet]

What's the whole point of DenyAnonymousAuthorizationRequirement and policy.RequiredAuthenticatedUser() if the authorization service is not invoked when the user is not authenticated?

Still no answer to this crucial question, it's a bit worrying IMHO.

[HaoK]

[Authorize] with nothing else specified uses the default policy of requiring any authenticated user. So this requirement and method exists mostly for that scenario

[kevinchalet]

But isn't the authorization filter always rejecting unauthenticated requests without invoking the authorization service no matter if you specify a policy?

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs#L77-L79

[HaoK]

Yeah the authorize filter probably shouldn't do that explicit check and leave that to the Authorization service. This seems like a legit bug to fix.

[Eilon]

@HaoK @blowdart any thoughts on whether this would regress any MVC 5.x behavior? I want to make sure there isn't too big a leap for someone updating their app and getting very different or unexpected behavior.

[Eilon]

@HaoK @blowdart ping for my last question regarding possible behavior changes.

[blowdart]

It would be a regression, yes. However you could address that by making the plain [Authorize] always just check for authenticated users. As we didn't have a policy in previous versions it's arguable we can make the behaviour there what we like.

[kevinchalet]

@blowdart well, AFAIK, that's how it works with the current bits: when [Authorize] doesn't specify a policy, AuthorizationPolicy.Combine returns the default one, which blocks unauthenticated requests by default.

[HaoK]

Empty authorize already requires authenticated users by default I believe.

[blowdart]

Yup, I am just emphasising that can't change.

[malekpour]

I think the default behavior is to require authentication, so [AuthorizeFilter] will do the same as before.

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authorization/AuthorizationOptions.cs#L19

[Eilon]

Right, just plain [Authorize] means "authenticated" and nothing more.

[Eilon]

So is this bug ok to close, then?

[kevinchalet]

Not really, the zealous check in the MVC authorization filter still needs to be removed.

Le 14 avr. 2016 à 19:33, Eilon Lipton [email protected] a écrit :

So is this bug ok to close, then?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

[HaoK]

Yeah my PR removes the preemptive check that blocks the AuthZ service from even being checked since unauthenticated requests never make it out of the filter

[Eilon]

Then how happy are we with #4385? @HaoK do we just need a signoff there (perhaps from @rynowak and/or @blowdart ) and we're good to go?

[HaoK]

Yeah its a pretty straight forward change, so just need a sign off and I can merge

[blowdart]

I am confused by Pinpoint's point on #4385

[kevinchalet]

@blowdart what's confusing... my remark about the fact MVC always uses ChallengeResult and relies on some voodoo magic in AuthenticationHandler to dynamically determine whether 401 or 403 should be returned? 😄

[blowdart]

Oh that's what you were talking about? Hah, no, I actually like that. Devs don't need to think about what the right status code is.

[kevinchalet]

Maybe, but it's both nonsensical and incorrect:

• A challenge is necessarily the consequence of a 401 response.
• Asking the authentication handler if it can extract an authentication ticket from the request to determine the right status code prevents a simple (yet fundamental) scenario: asking the caller to authenticate again (if you return a ChallengeResult, a 403 will be returned if the request was authenticated).

[blowdart]

Nothing stops you from return a 401 or a 403 though.

Is the name the problem?

[kevinchalet]

And BTW, users wouldn't have to think about the right status code if the authorization stack was better designed. IAuthorizationService should tell its caller why authorization failed and the caller should return the appropriate code (in MVC, it should be done by the authorization filter).

[kevinchalet]

Nothing stops you from return a 401 or a 403 though.

Well, try it: calling ChallengeResult/AuthenticationManager.ChallengeAsync will result in a 403 response if a token/cookie was successfully retrieved from the request, even if I really want the caller to re-authenticate (a 403 is not appropriate in this case) 😄

[Tratcher]

That results in infinite auth loops.

[HaoK]

f54a964