Warn when [Authorize] is overridden by [AllowAnonymous] from "farther" away

[halter73]

This PR adds analyzer that warns when [Authorize] is overridden by [AllowAnymous] from farther away. A lot of people don't realize that the relative order of [Authorize] and [AllowAnonymous] does not matter, and incorrectly assume that if they put [Authorize] closer to an MVC action than [AllowAnonymous], that it will still force authorization.

At first, I wanted to make this work and change the runtime to ignore [AllowAnonymous] if we observe an [Authorize] attribute that’s closer or at least log a warning. I tried this, but after seeing some of our test cases that started failing, I question if even logging a warning when there’s IAuthorizeData metadata after IAllowAnonymous metadata because it’s just so common to add [Authorize] using something like the app.MapControllers().RequireAuthorization() or options.Conventions.AuthorizeFolder(). Both those examples would result false positives if there any individual endpoints with [AllowAnonymous] because the [Authorize] attribute gets added last programmatically. I think this kind of thing is very common in real-world applications where certain endpoints like a login page never need authorization.

I now think we should stick solely with this analyzer that looks at the attributes on MVC controllers actions and base types in .NET 9. While this might not catch everything a runtime change would, I think the vast majority of the cases the analyzer would miss are false positives because they involve adding an authorization requirement programmatically which is an advanced scenario. Plus, the analyzer can catch some things that cannot be reliably caught at runtime, like [Authorize] coming after [AllowAnonymous] in the same attribute list since the order of attributes using reflection is undefined.

[AllowAnonymous]
public class OAuthControllerAnon : ControllerBase
{
}

[Authorize]
public class OAuthControllerAuthz : ControllerBase
{
}

[Authorize] // BUG
public class OAuthControllerInherited : OAuthControllerAnon
{
}

public class OAuthControllerInherited2 : OAuthControllerAnon
{
    [Authorize] // BUG
    public IActionResult Privacy()
    {
        return null;
    }
}

[AllowAnonymous]
[Authorize] // BUG
public class OAuthControllerMultiple : ControllerBase
{
}

[AllowAnonymous]
public class OAuthControllerInherited3 : ControllerBase
{
    [Authorize] // BUG
    public IActionResult Privacy()
    {
        return null;
    }
}

I’ve considered suggesting a code fix, but I’m leaning against it. The two main possibilities would be to recommend removing either the [AllowAnonymous] attribute or the [Authorize] attribute, but I don’t think we should assume intent. And even if the intent was for the endpoint to allow anonymous access, removing something like [Authorize(AuthenticationSchemes = “Cookies”)] could easily be breaking since the authorization middleware would no longer know which authentication scheme to try to authenticate with. This can matter even if authentication isn’t required to access the endpoint, because the behavior of the endpoint could change given an authenticated user.

API Proposal: #56310

Fixes #43550

[JamesNK]

Looks pretty good. As soon as I saw this, I was ready to point out that you need to look for IAllowAnonymous and not just AllowAnonymous, but you already are.

[JamesNK]

I think the title below is good, but for the message I think people who don't know what is going on will want some more detail.

More information:

• When authorization is run, AllowAnonymous takes precedence over Authorize. If both are present, then authorization doesn't run.
• MVC actions inherit (is that the right word? You should look at routing docs to see what it says) metadata from the controller. And controllers inherit metadata from base controls.
• Investigate your controllers and actions and decide whether to remove the [Authorize] attribute or move where the [AllowAnonymous] attribute is placed.

That detail could be in the message. Or there could be an aka.ms link here. Or it could be in the doc for the analyzer warning on learn.microsoft.com

[javiercn]

When authorization is run, AllowAnonymous takes precedence over Authorize. If both are present, then authorization doesn't run.

Does this need a bit of refinement? It sounds we are saying the filter doesn't run, but that's not truly the case. The filter still runs partially, it just doesn't call policy.AuthorizeAsync. So maybe we can clarify a bit saying that in this case the filter will run the authentication phase but not the authorization phase. This is relevant because the authentication phase can change the principal.

See https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Authorization/AuthorizeFilter.cs#L161-L183 for details.

At least AuthenticateAsync happens, which sets the principal on to the context. See https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authorization/Policy/src/PolicyEvaluator.cs#L56

[javiercn]

Should we really warn in this case? I understand that when AllowAnonymous is on a controller its problematic, but here it should be obvious that the action is not secure in the sense that it doesn't enforce authorization.

[javiercn]

An example of this (and I might be biased since I have experience in this field and I can probably agree it's not the common case) is for example when you are implementing multi-factor auth or social logins, where you have an action that receives the callback and has a custom Authorize(Scheme = "Social") to make sure the cookie is read and the principal set on the request, and that checks within the action if the principal has the right information on it or performs a custom action if not.

[halter73]

This is why I currently don't warn when [AllowAnonymous] is after [Authorize] on the same symbol. That's the way to signal to the analyzer that you know [AllowAnonymous] will win, so don't warn. We could never warn when the attributes are on the same symbol. That would probably be the least risky in terms of false positives. I discuss some other options in my earlier comment.

[javiercn]

Do we want to also cover situations where the signature is hidden?

[Route("[controller]")]
[AllowAnonymous]
public class MyBaseController : Controller
{
    public IActionResult Index()
    {
        return View();
    }
}

public class HomeController : MyBaseController
{
    [Authorize]
    public new IActionResult Index()
    {
        return View();
    }
}

[halter73]

In the new case, the closer attributes always win, right? And that's what we think people expect. So, there should be no relevant "farther away" [AllowAnonymous] attributes to warn about.

[javiercn]

Sorry, my example got botched at some point. I've updated the comment to reflect that. Maybe I put the comment in the wrong place and was misinterpreted as related to overriding, but I was suggesting that we also covered the case with new, even if we "know" it works like other cases to ensure that if we find this case we don't produce the wrong result.

[javiercn]

Looked at the tests so far. Overall, changes look good, with very thorough testing. Left some comments, and I'll take another pass at the implementation later.

[javiercn]

Plus, the analyzer can catch some things that cannot be reliably caught at runtime, like [Authorize] coming after [AllowAnonymous] in the same attribute list since the order of attributes using reflection is undefined.

How does the order impact the behavior in this case? My understanding was that it didn't matter, that the moment AllowAnonymous is present it'll always avoid enforcing the authorization rules.

[halter73]

How does the order impact the behavior in this case?

It doesn't. However, it could be somewhat sensible to have something like [Authorize(AuthenticationSchemes = “Cookies”)] along with [AllowAnonymous] on the same endpoint. Without the [Authorize(AuthenticationSchemes = “Cookies”)], the authorization middleware would no longer know which authentication scheme to try to authenticate with for anonymous endpoints which could have features light up for authenticated users.

The idea was if you put [AllowAnonymous] after [Authorize], [AllowAnonymous] is clearly "closer", so you have no reason to expect [Authorize] to win. That allows you to specify authentication schemes for an anonymous endpoint without having to suppress this warning.

I can think of two ways to improve this.

1. Warn for [AllowAnonymous] even if it's after [Authorize] on the same symbol unless AuthenticationSchemes is actually set. The downside is this might over warn for custom IAuthorizeData implementations that configure AuthenticationSchemes.
2. Warn for all usages of [AllowAnonymous] and [Authorize] on the same symbol regardless of whether AuthenticationSchemes is set, but add new public API to support the scenario. Maybe something like [AuthenticationSchemes("Cookies")] and IAuthenticationData. Then if people in this scenario run into this warning, they can replace both [Authorize(AuthenticationSchemes = “Cookies”)] and [AllowAnonymous] with a single [AuthenticationSchemes("Cookies")].

Or we can leave it as is with the assumption that accidently putting [AllowAnonymous] and [Authorize] on the same symbol is rare.

[javiercn]

The idea was if you put [AllowAnonymous] after [Authorize], [AllowAnonymous] is clearly "closer", so you have no reason to expect [Authorize] to win. That allows you to specify authentication schemes for an anonymous endpoint without having to suppress this warning.

This is fine, thanks for the clarification.

I think we are on the same page here, I just didn't see it :).

[javiercn]

I can think of two ways to improve this.

I think what we have is fine. If you put the two things on the same symbol we shouldn't warn. That's the behavior, isn't it? If we do warn, then we should explain what's going on in the warning and suggest the alternative ordering to avoid the warning.

[javiercn]

Looks good.

My only remaining comment would be that we need to provide an extended error message that explains the situation in more detail.

If we can provide concrete examples based on what happens (for example, AllowAnonymous on controller but Authorize on action) and (Authorize after AllowAnonymous on the same action) that would be better I think.

As a rough example, the error message could be something like:
[AllowAnonymous] was applied to the class <<Controller>>. Unauthorized users will be able to invoke this action without the proper permissions and potentially create a security issue. If your action is explicitly designed to allow anonymous users, you can <> or disable this warning.

But essentially comes down to is explaining what's happening, what are the consequences, and what action they can take to avoid the warning.

[JamesNK]

On [Authorize] vs [AllowAnonymous], the current behavior is fine, but not immediately obvious to someone who is new to ASP.NET Core authorization.

An analyzer like this is an opportunity for us to shift-left knowledge about how they interact together from a runtime bug to a build-time warning (@DamianEdwards am I using shift-left it right?). Having the warning is great, but a few sentences about why it is a warning is my ask, e.g. AllowAnonymous takes priority over Authorize, so using them together is pointless. And attributes can be inherited.

[halter73]

Thanks for the reviews. We can work on improving the warning title and message in the Analyzer proposal at #56310. I'll submit a follow up to address the feedback from the review.