Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

[Authorize(AuthenticationSchemes = Custom)] on Controller takes precedence over [AllowAnonymous] on Action #1577

Closed
ghost opened this issue Dec 20, 2017 · 12 comments

Comments

@ghost
Copy link

ghost commented Dec 20, 2017

I have created a new custom AuthenticationHandler. On the controller I have put the attribute Authorize with this new Authentication Scheme. On the action I have put AllowAnonymous. When I call the action, the AuthenticationHandler.HandleAuthenticateAsync gets called first, it will fail with a AuthenticateResult.Fail result, but then the action will get called. I would expect the AllowAnonymous to override the Authorize of the controller.

[Authorize(AuthenticationSchemes = TokenAuthenticationDefaults.AuthenticationScheme)]    
public class ApiController : Controller
{

[AllowAnonymous]
[Route("api/login")]
[HttpGet]
public async Task<IActionResult> Login(LoginModel model)       
{}

}      
@ghost
Copy link
Author

ghost commented Dec 22, 2017

I just checked the source (Microsoft.AspNetCore.Mvc.Authorization,AuthorizeFilter) and it seems this is the desired behaviour.

 var policyEvaluator = context.HttpContext.RequestServices.GetRequiredService<IPolicyEvaluator>();

    var authenticateResult = await policyEvaluator.AuthenticateAsync(effectivePolicy, context.HttpContext);

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

So the authentication happens whenever there is a Authorize attribute, and it happens before a check for AllowAnonymous. It would be nice if there was a attribute "NoAuthentication"

@ghost ghost closed this as completed Dec 22, 2017
@swlasse
Copy link

swlasse commented Oct 15, 2018

@bogdan-s I stumbled upon this the other day, and was a bit surprised to see the same behavior as you. Do you know why this has been closed? It sure looks like a behavior that is worth reconsidering - using the AllowAnonymous attribute as a short-circuit and ignore all other authorization rules seems fair, but I might be missing something?

@ghost
Copy link
Author

ghost commented Oct 15, 2018

@swlasse I will reopen it for you. I didn't want to spend too much time on this, since they've designed it like this and it was not blocking me.

@ghost ghost reopened this Oct 15, 2018
@swlasse
Copy link

swlasse commented Oct 15, 2018

Thanks @bogdan-s. It might very well be that it is designed like this on purpose, but in any case, I think it is good observation which deserves some attention. Would be great to understand why it is like this.

@muratg
Copy link
Contributor

muratg commented Nov 1, 2018

@swlasse Yes, this is by design.

@muratg muratg closed this as completed Nov 1, 2018
@swlasse
Copy link

swlasse commented Nov 4, 2018

@muratg Thanks for confirming this. It would be great to understand why it is like this though. Why not short-circuit immediately without invoking any AuthenticationHandlers?

@Tratcher
Copy link
Member

Tratcher commented Nov 4, 2018

@HaoK the description here is the opposite of what we discussed, AllowAnonymous isn't overriding Authorize like it's supposed to. More specifically it's not overriding it soon enough. Why isn't the anonymous check first?

@Tratcher Tratcher reopened this Nov 4, 2018
@muratg
Copy link
Contributor

muratg commented Nov 4, 2018

@Tratcher isn't this what we want?

You don't want to accidentally allow anonymous access to a random API method if you have Authorize at the class level IMO.

Opposite sounds fine to me... i.e. class is anonymous, and you add additional Authorize requirements at the method level.

@Tratcher
Copy link
Member

Tratcher commented Nov 4, 2018

AllowAnonymous has always trumped, but what they bring up here is that Authorize runs anyways and then gets discarded. Why does Authorize run at all?

@HaoK
Copy link
Member

HaoK commented Nov 4, 2018

Oh this is the bug where multiple filters were running, that's why we added the new flag that was turned on by default in 2.1, CombineAuthorizeFilters

aspnet/Mvc#7129

@Eilon
Copy link
Member

Eilon commented Nov 8, 2018

This issue should be addressed in 2.1 based on the previous comment.

@Eilon Eilon closed this as completed Nov 8, 2018
@swlasse
Copy link

swlasse commented Nov 13, 2018

I might be missing something here, but I am curious to know what is the answer to what @Tratcher points out?

AllowAnonymous has always trumped, but what they bring up here is that Authorize runs anyways and then gets discarded. Why does Authorize run at all?

I see there is pull request referenced, but I am not entirely sure how that relates to this?

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

No branches or pull requests

5 participants