Analyzer to warn when [Authorize] is overridden by [AllowAnymous] from "farther" away

[halter73]

Background and Motivation

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. The following code shows examples of where a closer [Authorize] attribute gets overridden by an [AllowAnonymous] attribute that is further away.

[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() => null;
}

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

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

Proposed Analyzer

Title

[Authorize] overridden by [AllowAnonymous] from farther away

Message

This [Authorize] attribute is overridden by an [AllowAnonymous] attribute from farther away on '{0}'. See https://aka.ms/aspnetcore-warnings/ASP0026 for more details.

And then https://aka.ms/aspnetcore-warnings/ASP0026 would point to documentation explaining.

• When authorization is run, [AllowAnonymous] takes precedence over [Authorize]. If both are present, then authorization doesn't run.
• MVC actions inherit 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.

Category

• [ ] Design
• [ ] Documentation
• [ ] Globalization
• [ ] Interoperability
• [ ] Maintainability
• [ ] Naming
• [ ] Performance
• [ ] Reliability
• [x] Security
• [ ] Style
• [ ] Usage

Severity Level

• [ ] Error
• [x] Warning
• [ ] Info
• [ ] Hidden

Risks

This could have some false positives where [Authorize] and [AllowAnonymous] were intended to be used together like when specifying an authentication scheme (e.g. [Authorize(AuthenticationSchemes = “Cookies”)]). No analyzer can catch every accidental application of [AllowAnonymous], but false positives and negatives can be mitigated by making the analyzer more or less conservative as need be.

[amcasey]

• Can we enrich the diagnostic by including related locations or Symbol objects?
• https://aka.ms/aspnetcore-warnings is an existing pattern
• Why say "farther away"?
  • Because it's not surprising when it's closer
• There are valid scenarios for doing the thing the analyzer warns against
  • Either suppress or repeat the AllowAnonymous in the same place as the Authorize to be explicit
• If it's not intentional, you should just delete the AllowAnonymous
• "Inherited" locations:
  • Base type
  • Current type
  • Overridden method
• Scoped to MVC for now
• We think the title is used when enabling/suppressing the analyzer or its diagnostics
• Mild preference for "further", but we genuinely don't know
• Alternative phrasing "distant location"
  • This is more or less synonymous and doesn't address the conceptual vagueness
• On the explanatory page, we may want to be explicit about the pitfalls not covered by this analyzer

Analyzer approved!

[halter73]

The analyzer was added by #56244 and https://aka.ms/aspnetcore-warnings/ASP0026 now points to https://learn.microsoft.com/aspnet/core/diagnostics/ASP0026.