Add analyzer/codefixer to use AddAuthorizationBuilder by david-acker · Pull Request #47428 · dotnet/aspnetcore

david-acker · 2023-03-26T23:44:12Z

Add `UseAuthorizationBuilderAnalyzer`, `AddAuthorizationBuilderFixer`

Adds an analyzer/codefixer to recommend that AddAuthorization be converted to AddAuthorizationBuilder.

Description

The code fix converts any usage of the setters for the DefaultPolicy, FaillbackPolicy, and InvokeHandlersAfterFailure properties on AuthorizationOptions to an equivalent method call (SetDefaultPolicy, SetFallbackPolicy, and SetInvokeHandlersAfterFailure) on AuthorizationBuilder.

No diagnostic is reported when the configure action passed to AddAuthorization uses the GetPolicy(String) method, the DefaultPolicy getter, the FallbackPolicy getter, or the InvokeHandlersAfterFailure getter on AuthorizationOptions as these do not exist on AuthorizationBuilder and thus cannot be converted.

No diagnostic is reported if the configure action passed to AddAuthorization contains operations unrelated to AuthorizationOptions, to avoid unintentionally deleting code when applying the code fix, as it would not be easy, or possible, to automatically map this to AddAuthorizationBuilder's fluent API.

Fixes #45219

…cy, AddFallbackPolicy invocations

…erFailure invocation

…re action

ghost · 2023-03-26T23:44:19Z

Thanks for your PR, @david-acker. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

mkArtakMSFT · 2023-03-28T18:03:46Z

@halter73 can you please review this? Thanks!

halter73

LGTM! Is there any particular reason this is still a draft? @captainsafia Can you take a look too?

halter73 · 2023-03-28T19:57:46Z

+        }
+    }
+
+    private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, SyntaxNode root, [NotNullWhen(true)] out InvocationExpressionSyntax? invocation)


When would this return false after a diagnostic is emitted? If it's possible, should we avoid raising the diagnostic if we cannot do the fix?

Offhand, I don't believe there should be any situations where a diagnostic would be emitted and this would return false. The analyzer should account for all of the scenarios where we wouldn't be able to apply the code fix and not emit a diagnostic in those situations. But I'll take a closer look to make sure the analyzer isn't missing anything.

I'd just add an assert to document this if you're confident.

halter73 · 2023-03-28T20:26:56Z

+                    SyntaxFactory.Space,
+                    SyntaxFactory.Space,
+                    SyntaxFactory.Space,
+                    SyntaxFactory.Space),


This part seems to be the sketchiest to me. I guess our tests expect SyntaxFactory.EndOfLine("\r\n"). Is this indentation relative to the previous line? Can we add tests for a more deeply nested call into AddAuthorization(...) like in HostBuilder.ConfigureServices?

I agree. This part still felt pretty hacky to me, which is why I left the PR in draft. I haven't confirmed it yet, but I'm assuming this may also related to the test that's failing due to some line ending issues.

I added this in an attempt to improve the formatting of the code generated by the code fix so that it would match the example "after" code example in the issue, otherwise there wouldn't be any indentation relative to the previous line.

Should we be concerned with the fixed code being "pretty" (within reason) or should we leave this up to the user to handle? If we do want to handle formatting as part of the code fix, I can go ahead and add tests for more deeply nested scenarios. Otherwise, I'll remove this and update the fixedSource values in the tests.

Fixed example: without the added indentation and newline (from MultipleAddPolicyCalls_UsingExpressionBody_FixedWithAddAuthorizationBuilder)

builder.Services.AddAuthorizationBuilder().AddPolicy("AtLeast18", policy => policy.Requirements.Add(new MinimumAgeRequirement(18))).AddPolicy("AtLeast21", policy => policy.Requirements.Add(new MinimumAgeRequirement(21)));

I think we want it to look reasonably pretty if we're going to do this. The example without the added indentation and newline does look bad.

How helpful is the SyntaxNode.NormalizeWhitespace API here for formatting the InvocationExpression? I know it sometimes doesn't play too nicely with deeply-nested calls but wondering if it might help here.

NormalizeWhitespace seemed to be removing new lines when I tried using it initially, which was causing some issues, but I'll do more testing to see if I can get it to work. Maybe calling NormalizeWhitespace and then manually adding a new line for each chained call would work?

Following up on this. Calling NormalizeWhitespace on the final InvocationExpression actually seems to format the block body examples OK, although the expression body examples ended up being pretty long since NormalizeWhitespace doesn't add new lines after the chained calls. So it looks like we'll need to add the EndOfLineTrivia ourselves if we want go this route.

Example: MultipleAddPolicyCalls_UsingBlockBody_FixedWithAddAuthorizationBuilder

builder.Services.AddAuthorizationBuilder().AddPolicy("AtLeast18", policy => { policy.Requirements.Add(new MinimumAgeRequirement(18)); }).AddPolicy("AtLeast21", policy => { policy.Requirements.Add(new MinimumAgeRequirement(21)); });

Example: MultipleAddPolicyCalls_UsingExpressionBody_FixedWithAddAuthorizationBuilder

builder.Services.AddAuthorizationBuilder().AddPolicy("AtLeast18", policy => policy.Requirements.Add(new MinimumAgeRequirement(18))).AddPolicy("AtLeast21", policy => policy.Requirements.Add(new MinimumAgeRequirement(21)));

I also added two tests for the nested AddAuthorization scenarios and made some adjustments to have leading trivia is handled in AddAuthorizationBuilderFixer to ensure that indentation is correct.

…alse

…ing trivia

david-acker · 2023-04-21T18:49:00Z

@halter73 @captainsafia How should we handle the line-ending issues with the Linux and MacOS test failures? Should we just add OSSkipCondition to these tests like we did for the line-ending issue with Add analyzer and code fix to recommend against IHeaderDictionary.Add #44463?

halter73 · 2023-04-21T23:21:13Z

/azp run

azure-pipelines · 2023-04-21T23:21:29Z

Azure Pipelines successfully started running 3 pipeline(s).

halter73 · 2023-04-21T23:23:31Z

I didn't realize we're skipping tests due to line-ending issues. Ideally, we should split or normalize the line endings before asserting so developers can run the tests on any platform even if we don't expect platform-specific failures. We shouldn't assume everyone is developing on Windows.

We do this for our RequestDelegateGenerator tests.

aspnetcore/src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTestBase.cs

Lines 319 to 325 in b4811e3

    
           var baseline = await File.ReadAllTextAsync(baselineFilePath); 
        
           var expectedLines = baseline 
        
               .TrimEnd() // Trim newlines added by autoformat 
        
               .Replace("%GENERATEDCODEATTRIBUTE%", RequestDelegateGeneratorSources.GeneratedCodeAttribute) 
        
               .Split(Environment.NewLine); 
        
           Assert.True(CompareLines(expectedLines, generatedCode, out var errorMessage), errorMessage);

david-acker · 2023-04-24T00:55:15Z

How would you recommend hooking this into the current verification logic that's being used for these analyzer/code fix tests (from Microsoft.CodeAnalysis.Testing)? At first glance, it looks like it may be possible to add in the line-ending normalization logic if we were to override CodeFixTest.VerifyFixAsync, although the source text verifications take place down in some private methods so I'll have to do some more digging.

Also, as an aside, these tests did actually pass when I ran them locally on MacOS with a clean clone of this branch, oddly enough. I'm curious why this would be the case, since they seem to be failing consistently on CI.

captainsafia · 2023-04-24T18:26:43Z

@david-acker This issue with line-endings originally came up in #45642. At the time, we implemented the ReplaceLineEndings workaround in a single test in anticipation that we could move it lower down into the verifier code if it was encountered in more tests (like you have here). See my comment here for where I think we should fix this.

captainsafia

Looking good! Thanks for the ReplaceLineEndings fixup. Left some comments and questions inline.

captainsafia · 2023-05-02T20:54:46Z

+        await VerifyCodeFix(source, new[] { diagnostic }, fixedSource);
+    }
+
+    private static async Task VerifyCodeFix(string source, DiagnosticResult[] diagnostics, string fixedSource)


Can we add a test for the scenario where the AddAuthorization call is assigned to a variable?

var services = builder.Services.AddAuthorization(options => ...);

I believe we will get the desired behavior here (not emitting a diagnostic) based on the IsLastCallInChain check but good to encode that behavior in tests.

Added AddAuthorization_CallAssignedToVariable_NoDiagnostic.

captainsafia · 2023-05-02T20:58:40Z

+
+    private static bool IsCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, AuthorizationOptionsTypes authorizationOptionsTypes)
+    {
+        if (invocation is { Arguments: { Length: 2 } invocationArguments }


Nit: the pattern checks here are very dense. Maybe factor these out into separate methods so it is clearer what we are asserting with each check?

I've refactored each of these pattern checks into a separate private method to make it clearer what each one does. Right now I've replaced each check with it's respective method to keep things as "flat" as possible, although this does require these methods to effectively be chained with their out parameters.

Let me know if there are any additional improvements or changes you would suggest here!

captainsafia · 2023-05-02T23:00:46Z

+    {
+        if (operation is IExpressionStatementOperation { Operation: { } expressionStatementOperation })
+        {
+            if (expressionStatementOperation is ISimpleAssignmentOperation { Target: IPropertyReferenceOperation { Property.ContainingType: { } propertyReferenceContainingType } }


Do we need add tests that what these code segments are checking (assuming I understood the code correctly) like:

builder.Services.AddAuthorization(options => ConfigureOptions(options));

I've added some new tests and updated a few of the existing ones to ensure that all of the branches in these private methods are covered by tests. I also removed the MethodReferenceOperation-related branch as this was unreachable.

captainsafia · 2023-05-02T23:01:18Z

+
+        foreach (var diagnostic in context.Diagnostics)
+        {
+            if (CanReplaceWithAddAuthorizationBuilder(diagnostic, root, out var invocation))


Yay! Thanks for doing the feasibility check before registering the codefix. This was an issue in the first few codefixers that we did...

…call

captainsafia

LGTM!

david-acker added 11 commits March 25, 2023 10:53

Add diagnostic

9d61804

Add AddAuthorizationBuilder analyzer and code fix

68c5b11

Add private AuthorizationOptionsTypes class

3c25e58

Replace DefaultPolicy, FallbackPolicy assignments with SetDefaultPoli…

5f960b4

…cy, AddFallbackPolicy invocations

Replace InvokeHandlersAfterFailure assignment to SetInvokeHandlersAft…

41cb8ad

…erFailure invocation

Ensure that AddAuthorization is last call in chain

9025b8a

Refactor

b7e9aa7

Handle AddAuthorization configure actions that use expression body

16b6ff4

Refactor, clean up test method names

540fbcd

Report no diagnostic when unrelated operations are present in configu…

53b0775

…re action

Refactor

3841e7f

ghost added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer community-contribution Indicates that the PR has been added by a community member labels Mar 26, 2023

mkArtakMSFT assigned halter73 Mar 28, 2023

mkArtakMSFT requested a review from halter73 March 28, 2023 18:03

halter73 reviewed Mar 28, 2023

View reviewed changes

david-acker added 3 commits March 29, 2023 18:26

Add Debug.Assert when CanReplaceWithAddAuthorizationBuilder returns f…

6ac3406

…alse

Check ContainingType instead of ContainingSymbol

065aff2

Add tests for nested AddAuthorization calls, improve handling of lead…

2fb59a3

…ing trivia

Call ReplaceLineEndings on source and fixedSource

ba61d75

david-acker marked this pull request as ready for review April 30, 2023 20:19

david-acker added 2 commits April 30, 2023 17:47

Merge branch 'main' into use-add-authorization-builder-analyzer

8c87892

Remove unnecessary assignment

25fdcd9

captainsafia reviewed May 2, 2023

View reviewed changes

captainsafia added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label May 2, 2023

david-acker added 12 commits May 2, 2023 20:01

Merge branch 'main' into use-add-authorization-builder-analyzer

a5a38b0

Move AuthorizationOptionsTypes to separate file

d889120

Make configure a named parameter

e82c10b

Use DiagnosticDescriptor Id as code fix equivalence key

404286e

Use SyntaxFactory.Tab

57e5ee2

Add test for assigning AddAuthorization call to variable

f53ec34

Add test for scenario where AuthorizationOptions is passed to method …

3783f77

…call

Fix GetPolicy reference, invocation tests

2451fa2

Add, update tests for getters and setters, guard against self assignment

a87a7a3

Remove unreachable MethodReferenceOperation check

ef08302

Merge branch 'main' into use-add-authorization-builder-analyzer

cab649a

Refactor pattern checks into separate methods

f3af8dc

captainsafia approved these changes May 9, 2023

View reviewed changes

captainsafia merged commit 5d96254 into dotnet:main May 9, 2023

ghost added this to the 8.0-preview5 milestone May 9, 2023

david-acker deleted the use-add-authorization-builder-analyzer branch May 9, 2023 22:03

david-acker mentioned this pull request May 10, 2023

Update analyzer docs to include ASP0025 "Use AddAuthorizationBuilder" dotnet/AspNetCore.Docs#29229

Closed

Conversation

david-acker commented Mar 26, 2023

Add UseAuthorizationBuilderAnalyzer, AddAuthorizationBuilderFixer

Description

Uh oh!

ghost commented Mar 26, 2023

Uh oh!

mkArtakMSFT commented Mar 28, 2023

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-acker Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-acker commented Apr 21, 2023

Uh oh!

halter73 commented Apr 21, 2023

Uh oh!

azure-pipelines Bot commented Apr 21, 2023

Uh oh!

halter73 commented Apr 21, 2023

Uh oh!

david-acker commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captainsafia commented Apr 24, 2023

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add `UseAuthorizationBuilderAnalyzer`, `AddAuthorizationBuilderFixer`

david-acker Mar 28, 2023 •

edited

Loading

david-acker commented Apr 24, 2023 •

edited

Loading