-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add analyzer/codefixer to use AddAuthorizationBuilder #47428
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
captainsafia
merged 29 commits into
dotnet:main
from
david-acker:use-add-authorization-builder-analyzer
May 9, 2023
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
9d61804
Add diagnostic
david-acker 68c5b11
Add AddAuthorizationBuilder analyzer and code fix
david-acker 3c25e58
Add private AuthorizationOptionsTypes class
david-acker 5f960b4
Replace DefaultPolicy, FallbackPolicy assignments with SetDefaultPoli…
david-acker 41cb8ad
Replace InvokeHandlersAfterFailure assignment to SetInvokeHandlersAft…
david-acker 9025b8a
Ensure that AddAuthorization is last call in chain
david-acker b7e9aa7
Refactor
david-acker 16b6ff4
Handle AddAuthorization configure actions that use expression body
david-acker 540fbcd
Refactor, clean up test method names
david-acker 53b0775
Report no diagnostic when unrelated operations are present in configu…
david-acker 3841e7f
Refactor
david-acker 6ac3406
Add Debug.Assert when CanReplaceWithAddAuthorizationBuilder returns f…
david-acker 065aff2
Check ContainingType instead of ContainingSymbol
david-acker 2fb59a3
Add tests for nested AddAuthorization calls, improve handling of lead…
david-acker ba61d75
Call ReplaceLineEndings on source and fixedSource
david-acker 8c87892
Merge branch 'main' into use-add-authorization-builder-analyzer
david-acker 25fdcd9
Remove unnecessary assignment
david-acker a5a38b0
Merge branch 'main' into use-add-authorization-builder-analyzer
david-acker d889120
Move AuthorizationOptionsTypes to separate file
david-acker e82c10b
Make configure a named parameter
david-acker 404286e
Use DiagnosticDescriptor Id as code fix equivalence key
david-acker 57e5ee2
Use SyntaxFactory.Tab
david-acker f53ec34
Add test for assigning AddAuthorization call to variable
david-acker 3783f77
Add test for scenario where AuthorizationOptions is passed to method …
david-acker 2451fa2
Fix GetPolicy reference, invocation tests
david-acker a87a7a3
Add, update tests for getters and setters, guard against self assignment
david-acker ef08302
Remove unreachable MethodReferenceOperation check
david-acker cab649a
Merge branch 'main' into use-add-authorization-builder-analyzer
david-acker f3af8dc
Refactor pattern checks into separate methods
david-acker File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
234 changes: 234 additions & 0 deletions
234
...mework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Linq; | ||
| using Microsoft.AspNetCore.App.Analyzers.Infrastructure; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
|
|
||
| namespace Microsoft.AspNetCore.Analyzers.Authorization; | ||
|
|
||
| using WellKnownType = WellKnownTypeData.WellKnownType; | ||
|
|
||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public sealed class AddAuthorizationBuilderAnalyzer : DiagnosticAnalyzer | ||
| { | ||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(DiagnosticDescriptors.UseAddAuthorizationBuilder); | ||
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
| context.EnableConcurrentExecution(); | ||
| context.RegisterCompilationStartAction(OnCompilationStart); | ||
| } | ||
|
|
||
| private static void OnCompilationStart(CompilationStartAnalysisContext context) | ||
| { | ||
| var wellKnownTypes = WellKnownTypes.GetOrCreate(context.Compilation); | ||
|
|
||
| var authorizationOptionsTypes = new AuthorizationOptionsTypes(wellKnownTypes); | ||
| if (!authorizationOptionsTypes.HasRequiredTypes) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var policyServiceCollectionExtensions = wellKnownTypes.Get(WellKnownType.Microsoft_Extensions_DependencyInjection_PolicyServiceCollectionExtensions); | ||
| if (policyServiceCollectionExtensions is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var addAuthorizationMethod = policyServiceCollectionExtensions.GetMembers() | ||
| .OfType<IMethodSymbol>() | ||
| .FirstOrDefault(member => member is { Name: "AddAuthorization", Parameters.Length: 2 }); | ||
|
|
||
| if (addAuthorizationMethod is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| context.RegisterOperationAction(context => | ||
| { | ||
| var invocation = (IInvocationOperation)context.Operation; | ||
|
|
||
| if (SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) | ||
| && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, policyServiceCollectionExtensions) | ||
| && IsLastCallInChain(invocation) | ||
| && IsCompatibleWithAuthorizationBuilder(invocation, authorizationOptionsTypes)) | ||
| { | ||
| AddDiagnosticInformation(context, invocation.Syntax.GetLocation()); | ||
| } | ||
|
|
||
| }, OperationKind.Invocation); | ||
| } | ||
|
|
||
| private static bool IsCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, AuthorizationOptionsTypes authorizationOptionsTypes) | ||
| { | ||
| if (TryGetConfigureArgumentOperation(invocation, out var configureArgumentOperation) | ||
| && TryGetConfigureDelegateCreationOperation(configureArgumentOperation, out var configureDelegateCreationOperation) | ||
| && TryGetConfigureAnonymousFunctionOperation(configureDelegateCreationOperation, out var configureAnonymousFunctionOperation) | ||
| && TryGetConfigureBlockOperation(configureAnonymousFunctionOperation, out var configureBlockOperation)) | ||
| { | ||
| // Ensure that the child operations of the configuration action passed to AddAuthorization are all related to AuthorizationOptions. | ||
| var allOperationsInvolveAuthorizationOptions = configureBlockOperation.ChildOperations | ||
| .Where(operation => operation is not IReturnOperation { IsImplicit: true }) | ||
| .All(operation => DoesOperationInvolveAuthorizationOptions(operation, authorizationOptionsTypes)); | ||
|
|
||
| return allOperationsInvolveAuthorizationOptions | ||
| // Ensure that the configuration action passed to AddAuthorization does not use any AuthorizationOptions-specific APIs. | ||
| && IsConfigureActionCompatibleWithAuthorizationBuilder(configureBlockOperation, authorizationOptionsTypes); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool TryGetConfigureArgumentOperation(IInvocationOperation invocation, [NotNullWhen(true)] out IArgumentOperation? configureArgumentOperation) | ||
| { | ||
| configureArgumentOperation = null; | ||
|
|
||
| if (invocation is { Arguments: { Length: 2 } invocationArguments }) | ||
| { | ||
| configureArgumentOperation = invocationArguments[1]; | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool TryGetConfigureDelegateCreationOperation(IArgumentOperation configureArgumentOperation, [NotNullWhen(true)] out IDelegateCreationOperation? configureDelegateCreationOperation) | ||
| { | ||
| configureDelegateCreationOperation = null; | ||
|
|
||
| if (configureArgumentOperation is { ChildOperations: { Count: 1 } argumentChildOperations } | ||
| && argumentChildOperations.First() is IDelegateCreationOperation delegateCreationOperation) | ||
| { | ||
| configureDelegateCreationOperation = delegateCreationOperation; | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool TryGetConfigureAnonymousFunctionOperation(IDelegateCreationOperation configureDelegateCreationOperation, [NotNullWhen(true)] out IAnonymousFunctionOperation? configureAnonymousFunctionOperation) | ||
| { | ||
| configureAnonymousFunctionOperation = null; | ||
|
|
||
| if (configureDelegateCreationOperation is { ChildOperations: { Count: 1 } delegateCreationChildOperations } | ||
| && delegateCreationChildOperations.First() is IAnonymousFunctionOperation anonymousFunctionOperation) | ||
| { | ||
| configureAnonymousFunctionOperation = anonymousFunctionOperation; | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool TryGetConfigureBlockOperation(IAnonymousFunctionOperation configureAnonymousFunctionOperation, [NotNullWhen(true)] out IBlockOperation? configureBlockOperation) | ||
| { | ||
| configureBlockOperation = null; | ||
|
|
||
| if (configureAnonymousFunctionOperation is { ChildOperations: { Count: 1 } anonymousFunctionChildOperations } | ||
| && anonymousFunctionChildOperations.First() is IBlockOperation blockOperation) | ||
| { | ||
| configureBlockOperation = blockOperation; | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool DoesOperationInvolveAuthorizationOptions(IOperation operation, AuthorizationOptionsTypes authorizationOptionsTypes) | ||
| { | ||
| if (operation is IExpressionStatementOperation { Operation: { } expressionStatementOperation }) | ||
| { | ||
| if (expressionStatementOperation is ISimpleAssignmentOperation { Target: IPropertyReferenceOperation { Property.ContainingType: { } propertyReferenceContainingType } } | ||
| && SymbolEqualityComparer.Default.Equals(propertyReferenceContainingType, authorizationOptionsTypes.AuthorizationOptions)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (expressionStatementOperation is IInvocationOperation { TargetMethod.ContainingType: { } invokedMethodContainingType } | ||
| && SymbolEqualityComparer.Default.Equals(invokedMethodContainingType, authorizationOptionsTypes.AuthorizationOptions)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IBlockOperation configureAction, AuthorizationOptionsTypes authorizationOptionsTypes) | ||
| { | ||
| var usesAuthorizationOptionsSpecificAPIs = configureAction.Descendants() | ||
| .Any(operation => UsesAuthorizationOptionsSpecificGetters(operation, authorizationOptionsTypes) | ||
| || UsesAuthorizationOptionsGetPolicy(operation, authorizationOptionsTypes)); | ||
|
|
||
| return !usesAuthorizationOptionsSpecificAPIs; | ||
| } | ||
|
|
||
| private static bool UsesAuthorizationOptionsSpecificGetters(IOperation operation, AuthorizationOptionsTypes authorizationOptionsTypes) | ||
| { | ||
| if (operation is IPropertyReferenceOperation propertyReferenceOperation) | ||
| { | ||
| var property = propertyReferenceOperation.Property; | ||
|
|
||
| // Check that the referenced property is not being set. | ||
| if (propertyReferenceOperation.Parent is IAssignmentOperation { Target: IPropertyReferenceOperation targetProperty } | ||
| && SymbolEqualityComparer.Default.Equals(property, targetProperty.Property)) | ||
| { | ||
| // Ensure the referenced property isn't being assigned to itself | ||
| // (i.e. options.DefaultPolicy = options.DefaultPolicy;) | ||
| if (propertyReferenceOperation.Parent is IAssignmentOperation { Value: IPropertyReferenceOperation valueProperty } | ||
| && SymbolEqualityComparer.Default.Equals(property, valueProperty.Property)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| if (SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.DefaultPolicy) | ||
| || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.FallbackPolicy) | ||
| || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.InvokeHandlersAfterFailure)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool UsesAuthorizationOptionsGetPolicy(IOperation operation, AuthorizationOptionsTypes authorizationOptionsTypes) | ||
| { | ||
| if (operation is IMethodReferenceOperation methodReferenceOperation | ||
| && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, authorizationOptionsTypes.GetPolicy) | ||
| && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingType, authorizationOptionsTypes.AuthorizationOptions)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (operation is IInvocationOperation invocationOperation | ||
| && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy) | ||
| && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingType, authorizationOptionsTypes.AuthorizationOptions)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool IsLastCallInChain(IInvocationOperation invocation) | ||
| { | ||
| return invocation.Parent is IExpressionStatementOperation; | ||
| } | ||
|
|
||
| private static void AddDiagnosticInformation(OperationAnalysisContext context, Location location) | ||
| { | ||
| context.ReportDiagnostic(Diagnostic.Create( | ||
| DiagnosticDescriptors.UseAddAuthorizationBuilder, | ||
| location)); | ||
| } | ||
| } | ||
47 changes: 47 additions & 0 deletions
47
src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AuthorizationOptionsTypes.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Linq; | ||
| using Microsoft.AspNetCore.App.Analyzers.Infrastructure; | ||
| using Microsoft.CodeAnalysis; | ||
|
|
||
| namespace Microsoft.AspNetCore.Analyzers.Authorization; | ||
|
|
||
| using WellKnownType = WellKnownTypeData.WellKnownType; | ||
|
|
||
| internal sealed class AuthorizationOptionsTypes | ||
| { | ||
| public AuthorizationOptionsTypes(WellKnownTypes wellKnownTypes) | ||
| { | ||
| AuthorizationOptions = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Authorization_AuthorizationOptions); | ||
|
|
||
| if (AuthorizationOptions is not null) | ||
| { | ||
| var authorizationOptionsMembers = AuthorizationOptions.GetMembers(); | ||
|
|
||
| var authorizationOptionsProperties = authorizationOptionsMembers.OfType<IPropertySymbol>(); | ||
|
|
||
| DefaultPolicy = authorizationOptionsProperties | ||
| .FirstOrDefault(member => member.Name == "DefaultPolicy"); | ||
| FallbackPolicy = authorizationOptionsProperties | ||
| .FirstOrDefault(member => member.Name == "FallbackPolicy"); | ||
| InvokeHandlersAfterFailure = authorizationOptionsProperties | ||
| .FirstOrDefault(member => member.Name == "InvokeHandlersAfterFailure"); | ||
|
|
||
| GetPolicy = authorizationOptionsMembers.OfType<IMethodSymbol>() | ||
| .FirstOrDefault(member => member.Name == "GetPolicy"); | ||
| } | ||
| } | ||
|
|
||
| public INamedTypeSymbol? AuthorizationOptions { get; } | ||
| public IPropertySymbol? DefaultPolicy { get; } | ||
| public IPropertySymbol? FallbackPolicy { get; } | ||
| public IPropertySymbol? InvokeHandlersAfterFailure { get; } | ||
| public IMethodSymbol? GetPolicy { get; } | ||
|
|
||
| public bool HasRequiredTypes => AuthorizationOptions is not null | ||
| && DefaultPolicy is not null | ||
| && FallbackPolicy is not null | ||
| && InvokeHandlersAfterFailure is not null | ||
| && GetPolicy is not null; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need add tests that what these code segments are checking (assuming I understood the code correctly) like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.