Skip to content

Add analyzer for detecting misplaced attributes on delegates #35779

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

Merged
merged 6 commits into from
Sep 1, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Aug 26, 2021

This PR adds an analyzer that detects if attributes are incorrectly placed on a method invoked from an attribute in a lambda.

using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", () => Hello());
[Authorize]
void Hello() { }

The above will produce a diagnostic warning the user about the misplaced AuthorizeAttribute with the following message:

'AuthorizeAttribute' should not be placed on 'Hello'. Place 'AuthorizeAttribute' on the endpoint delegate instead.

Fixes #35638

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 26, 2021
@captainsafia captainsafia added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Aug 26, 2021
@captainsafia captainsafia marked this pull request as ready for review August 26, 2021 22:39
@captainsafia captainsafia requested a review from Pilchie as a code owner August 26, 2021 22:39
@captainsafia captainsafia requested a review from pranavkm August 26, 2021 22:52
Assert.Collection(diagnostics,
diagnostic => {
Assert.Same(DiagnosticDescriptors.DetectMisplacedLambdaAttribute, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we be producing the diagnostic on the attribute (rather than on the delegate)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think it makes more sense to put it on the delegate since that is where the user will take the action to resolve the issue (absent us providing a codefix).

private TestDiagnosticAnalyzerRunner Runner { get; } = new(new DelegateEndpointAnalyzer());

[Fact]
public async Task MinimalAction_WithCorrectlyPlacedAttribute_Works()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test with an invalid lambda or a invalid method reference? e.g.

app.MapGet(""/"", () => DoesNotExist());

One with a MethodReference:

app.MapGet(""/"", GetPersonId);

[Authorize]
public string GetPersonId() => string.Empty;

and with a reference to a different source:

// Source1 
app.MapGet(""/"", () => PersonHandlers.GetPersonId);

Where PersonHandlers.GetPersonId is in a separate source file / SyntaxTree

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good idea. Here's the expected behavior for these:

  1. Invalid reference: No warning from us but user gets a not defined warning.
  2. Method reference: No warning from us since this warning only applies to lambdas.
  3. Reference to a method in a different class: Warning from us about misplaced attribute.

Where PersonHandlers.GetPersonId is in a separate source file / SyntaxTree

For this part, I couldn't find any examples in our codebase of the GetDiagnosticsAsync method processing sources from different source files so I put it in separate files. Is there a way to do that?

using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Builder;
var app = WebApplication.Create();
app.MapGet(""/"", /*MM*/() => { return Hello(); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's interesting - if the code looked like:

(int id) => if (id == 0) { return Results.NotFound(); } else { return Hello(); }

This would produce no diagnostics? Is that weird?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a consequence of not doing the full search through the syntax tree that we were doing before (see #35779 (comment)).

Supporting this scenario invites the question of how "deep" in the syntax tree we want to look for returned invocations and support throwing a warning.

@captainsafia captainsafia requested a review from pranavkm August 30, 2021 17:00
return false;
}

var fullyQualifiedName = attribute.AttributeClass.ContainingNamespace.ToDisplayString(symbolDisplayFormat);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we look up the assembly name or walk up the namespace? Allocating a formatted string on every attribute doesn't sound great for performance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can walk up the namespace, we'll still have to evaluate against the string representation of the NamespaceSymbol though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be fine. It wouldn't allocate a new string every time no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, actually, it uses the same SymbolDisplay logic under the hood.

I ended up rewriting this to avoid interacting with the FQNS of the type at all. We can just traverse up the tree until we find an AspNetCore namespace containing in a Microsoft namespace and treat that as a valid scenario.

@captainsafia captainsafia requested a review from pranavkm August 31, 2021 20:44
return true;
}

return IsInValidNamespace(@namespace.ContainingNamespace);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

…mbdaAttribute.cs

Co-authored-by: Pranav K <prkrishn@hotmail.com>
@captainsafia captainsafia merged commit 7759944 into main Sep 1, 2021
@captainsafia captainsafia deleted the safia/attr-analyzer branch September 1, 2021 01:24
@ghost ghost added this to the 7.0-preview1 milestone Sep 1, 2021
@captainsafia
Copy link
Member Author

/backport release/6.0

@captainsafia
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2021

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1190188412

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detecting when attributes are put on a method called by a lambda instead of the lambda itself
3 participants