From 9d61804dc937490a47419a7f1aef090b34328cea Mon Sep 17 00:00:00 2001 From: David Acker Date: Sat, 25 Mar 2023 10:53:24 -0400 Subject: [PATCH 01/26] Add diagnostic --- .../src/Analyzers/DiagnosticDescriptors.cs | 9 +++++++++ .../AspNetCoreAnalyzers/src/Analyzers/Resources.resx | 8 +++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs index daba6a423cd7..85bc320937b6 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs @@ -205,4 +205,13 @@ internal static class DiagnosticDescriptors DiagnosticSeverity.Error, isEnabledByDefault: true, helpLinkUri: "https://aka.ms/aspnet/analyzers"); + + internal static readonly DiagnosticDescriptor UseAddAuthorizationBuilder = new( + "ASP0025", + new LocalizableResourceString(nameof(Resources.Analyzer_UseAddAuthorizationBuilder_Title), Resources.ResourceManager, typeof(Resources)), + new LocalizableResourceString(nameof(Resources.Analyzer_UseAddAuthorizationBuilder_Message), Resources.ResourceManager, typeof(Resources)), + "Usage", + DiagnosticSeverity.Info, + isEnabledByDefault: true, + helpLinkUri: "https://aka.ms/aspnet/analyzers"); } diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx index 7f0bd3d358e7..c8eb50c591e4 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx @@ -309,4 +309,10 @@ Route '{0}' conflicts with another action route. An HTTP request that matches multiple routes results in an ambiguous match error. Fix the conflict by changing the route's pattern, HTTP method, or route constraints. - + + Use AddAuthorizationBuilder to register authorization services and construct policies. + + + Use AddAuthorizationBuilder + + \ No newline at end of file From 68c5b1183f65b2d88404963c1862675c50296474 Mon Sep 17 00:00:00 2001 From: David Acker Date: Sat, 25 Mar 2023 15:23:27 -0400 Subject: [PATCH 02/26] Add AddAuthorizationBuilder analyzer and code fix --- .../AddAuthorizationBuilderAnalyzer.cs | 123 +++++++ .../AddAuthorizationBuilderFixer.cs | 112 ++++++ .../AddAuthorizationBuilderTests.cs | 323 ++++++++++++++++++ src/Shared/RoslynUtils/WellKnownTypeData.cs | 8 +- 4 files changed, 564 insertions(+), 2 deletions(-) create mode 100644 src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs create mode 100644 src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs create mode 100644 src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs new file mode 100644 index 000000000000..b5891e75f3ec --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -0,0 +1,123 @@ +// 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.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 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 policyServiceCollectionExtensions = wellKnownTypes.Get(WellKnownType.Microsoft_Extensions_DependencyInjection_PolicyServiceCollectionExtensions); + var addAuthorizationMethod = policyServiceCollectionExtensions.GetMembers() + .OfType() + .Single(member => member.Parameters.Length == 2 && member.Name == "AddAuthorization"); + + context.RegisterOperationAction(context => + { + var invocation = (IInvocationOperation)context.Operation; + + if (invocation.TargetMethod.Parameters.Length == 2 + && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, policyServiceCollectionExtensions) + && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) + && IsConfigureActionCompatibleWithAuthorizationBuilder(invocation, wellKnownTypes)) + { + AddDiagnosticInformation(context, invocation.Syntax.GetLocation()); + } + + }, OperationKind.Invocation); + } + + private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, WellKnownTypes wellKnownTypes) + { + if (invocation.Arguments is not { Length: 2 }) + { + return false; + } + + var configureAction = invocation.Arguments[1]; + + var authorizationOptions = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Authorization_AuthorizationOptions); + var authorizationOptionsMembers = authorizationOptions.GetMembers(); + + var defaultPolicyProperty = authorizationOptionsMembers.First(member => + member.Kind == SymbolKind.Property && member.Name == "DefaultPolicy"); + + var fallbackPolicyProperty = authorizationOptionsMembers.First(member => + member.Kind == SymbolKind.Property && member.Name == "FallbackPolicy"); + + var invokeHandlersAfterFailureProperty = authorizationOptionsMembers.First(member => + member.Kind == SymbolKind.Property && member.Name == "InvokeHandlersAfterFailure"); + + var getPolicyMethod = authorizationOptionsMembers.First(member => + member.Kind == SymbolKind.Method && member.Name == "GetPolicy"); + + return !configureAction.Descendants().Any(operation => + { + if (operation is IPropertyReferenceOperation propertyReferenceOperation) + { + var property = propertyReferenceOperation.Property; + + if (SymbolEqualityComparer.Default.Equals(property, defaultPolicyProperty) + || SymbolEqualityComparer.Default.Equals(property, fallbackPolicyProperty) + || SymbolEqualityComparer.Default.Equals(property, invokeHandlersAfterFailureProperty)) + { + return true; + } + + return false; + } + + if (operation is IMethodReferenceOperation methodReferenceOperation) + { + if (SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, getPolicyMethod) + && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingSymbol, authorizationOptions)) + { + return true; + } + + return false; + } + + if (operation is IInvocationOperation invocationOperation) + { + if (SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingType, authorizationOptions) + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, getPolicyMethod)) + { + return true; + } + + return false; + } + + return false; + }); + } + + private static void AddDiagnosticInformation(OperationAnalysisContext context, Location location) + { + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.UseAddAuthorizationBuilder, + location)); + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs new file mode 100644 index 000000000000..0459d566a8e2 --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -0,0 +1,112 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Immutable; +using System.Composition; +using System.Diagnostics.CodeAnalysis; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace Microsoft.AspNetCore.Analyzers.Authorization.Fixers; + +[ExportCodeFixProvider(LanguageNames.CSharp), Shared] +public sealed class AddAuthorizationBuilderFixer : CodeFixProvider +{ + public override ImmutableArray FixableDiagnosticIds { get; } = ImmutableArray.Create(DiagnosticDescriptors.UseAddAuthorizationBuilder.Id); + + public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root == null) + { + return; + } + + var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + if (semanticModel == null) + { + return; + } + + foreach (var diagnostic in context.Diagnostics) + { + if (CanReplaceWithAddAuthorizationBuilder(diagnostic, root, out var invocation)) + { + const string title = "Use 'AddAuthorizationBuilder'"; + context.RegisterCodeFix( + CodeAction.Create(title, + cancellationToken => ReplaceWithAddAuthorizationBuilder(diagnostic, root, context.Document, invocation), + equivalenceKey: title), + diagnostic); + } + } + } + + private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, SyntaxNode root, [NotNullWhen(true)] out InvocationExpressionSyntax? invocation) + { + invocation = null; + + var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + if (diagnosticTarget is InvocationExpressionSyntax { ArgumentList.Arguments: { } arguments, Expression: MemberAccessExpressionSyntax { Name.Identifier: { } identifierToken } memberAccessExpression } invocationExpression) + { + if (arguments.Count == 1 + && arguments[0].Expression is SimpleLambdaExpressionSyntax lambda) + { + if (lambda.Body is not BlockSyntax lambdaBody) + { + return false; + } + + var addAuthorizationBuilderMethod = + memberAccessExpression.ReplaceToken(identifierToken, SyntaxFactory.Identifier("AddAuthorizationBuilder")); + + invocation = SyntaxFactory.InvocationExpression(addAuthorizationBuilderMethod); + + var lambdaNodes = lambdaBody.DescendantNodes(); + + foreach (var configureAction in lambdaNodes.OfType()) + { + if (configureAction is InvocationExpressionSyntax { ArgumentList.Arguments: { } configureArguments, Expression: MemberAccessExpressionSyntax { Name.Identifier.Text: "AddPolicy" } }) + { + if (configureArguments.Count == 2) + { + invocation = SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + invocation.WithTrailingTrivia( + SyntaxFactory.EndOfLine(Environment.NewLine), + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space), + SyntaxFactory.IdentifierName("AddPolicy")), + SyntaxFactory.ArgumentList( + SyntaxFactory.SeparatedList(configureArguments))); + } + } + } + + return true; + } + } + + return false; + } + + private static Task ReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, SyntaxNode root, Document document, InvocationExpressionSyntax invocation) + { + var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + return Task.FromResult(document.WithSyntaxRoot( + root.ReplaceNode(diagnosticTarget, invocation))); + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs new file mode 100644 index 000000000000..b40a4dcc10aa --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -0,0 +1,323 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.CodeAnalysis.Testing; +using VerifyCS = Microsoft.AspNetCore.Analyzers.Verifiers.CSharpCodeFixVerifier< + Microsoft.AspNetCore.Analyzers.Authorization.AddAuthorizationBuilderAnalyzer, + Microsoft.AspNetCore.Analyzers.Authorization.Fixers.AddAuthorizationBuilderFixer>; + +namespace Microsoft.AspNetCore.Analyzers.Authorization; + +public sealed class AddAuthorizationBuilderTests +{ + // TODO: Additional Test Cases + // - Refactor assignment to AuthorizationOptions.FallbackPolicy to use AuthorizationBuilder.SetFallbackPolicy + // - Refactoring to call InvokeHandlersAfterFailure instead of using setter + // - Other IServiceCollection extension is changed to AddAuthorization call. + // to keep things simple, just check if the parent of the InvocationExpression is a GlobalStatementExpression + + [Fact] + public async Task AddAuthorization_WithSingleAddPolicyCall_UsingExpressionBody_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => +{ + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +})|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + + [Fact] + public async Task AddAuthorization_WithMultipleAddPolicyCalls_UsingExpressionBody_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => +{ + options.AddPolicy(""AtLeast18"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(18))); + + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +})|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .AddPolicy(""AtLeast18"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(18))) + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + + [Fact] + public async Task AddAuthorization_WithSingleAddPolicyCall_UsingBlockBody_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => +{ + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +})|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + + [Fact] + public async Task AddAuthorization_WithMultipleAddPolicyCalls_UsingBlockBody_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => +{ + options.AddPolicy(""AtLeast18"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(18)); + }); + + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +})|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .AddPolicy(""AtLeast18"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(18)); + }) + .AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + + [Fact] + public async Task AddAuthorization_AccessesAuthorizationOptionsDefaultPolicy_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + _ = options.DefaultPolicy; + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +}); +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task AddAuthorization_AccessesAuthorizationOptionsFallbackPolicy_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + _ = options.FallbackPolicy; + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +}); +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task AddAuthorization_AccessesAuthorizationOptionsInvokeHandlesAfterFailure_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + _ = options.InvokeHandlersAfterFailure; + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +}); +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task AddAuthorization_ReferencesAuthorizationOptionsGetPolicy_NoDiagnostic() + { + var source = @" +using System; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + Func _ = options.GetPolicy; + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +}); +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task AddAuthorization_InvokesAuthorizationOptionsGetPolicy_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + _ = options.GetPolicy(""""); + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +}); +"; + + await VerifyNoCodeFix(source); + } + + private static async Task VerifyCodeFix(string source, DiagnosticResult[] diagnostics, string fixedSource) + { + var fullSource = string.Join(Environment.NewLine, source, _testAuthorizationPolicyClassDeclaration); + var fullFixedSource = string.Join(Environment.NewLine, fixedSource, _testAuthorizationPolicyClassDeclaration); + + await VerifyCS.VerifyCodeFixAsync(fullSource, diagnostics, fullFixedSource); + } + + private static async Task VerifyNoCodeFix(string source) + { + var fullSource = string.Join(Environment.NewLine, source, _testAuthorizationPolicyClassDeclaration); + + await VerifyCS.VerifyCodeFixAsync(fullSource, Array.Empty(), fullSource); + } + + private const string _testAuthorizationPolicyClassDeclaration = @" +public class MinimumAgeRequirement: IAuthorizationRequirement +{ + public int Age { get; } + public MinimumAgeRequirement(int age) => Age = age; +} +"; +} diff --git a/src/Shared/RoslynUtils/WellKnownTypeData.cs b/src/Shared/RoslynUtils/WellKnownTypeData.cs index 86f0302d5d9b..2d0def277fdd 100644 --- a/src/Shared/RoslynUtils/WellKnownTypeData.cs +++ b/src/Shared/RoslynUtils/WellKnownTypeData.cs @@ -107,7 +107,9 @@ public enum WellKnownType Microsoft_AspNetCore_Mvc_ValidateAntiForgeryTokenAttribute, Microsoft_AspNetCore_Mvc_ModelBinding_EmptyBodyBehavior, Microsoft_AspNetCore_Authorization_AllowAnonymousAttribute, - Microsoft_AspNetCore_Authorization_AuthorizeAttribute + Microsoft_AspNetCore_Authorization_AuthorizeAttribute, + Microsoft_Extensions_DependencyInjection_PolicyServiceCollectionExtensions, + Microsoft_AspNetCore_Authorization_AuthorizationOptions } public static string[] WellKnownTypeNames = new[] @@ -212,6 +214,8 @@ public enum WellKnownType "Microsoft.AspNetCore.Mvc.ValidateAntiForgeryTokenAttribute", "Microsoft.AspNetCore.Mvc.ModelBinding.EmptyBodyBehavior", "Microsoft.AspNetCore.Authorization.AllowAnonymousAttribute", - "Microsoft.AspNetCore.Authorization.AuthorizeAttribute" + "Microsoft.AspNetCore.Authorization.AuthorizeAttribute", + "Microsoft.Extensions.DependencyInjection.PolicyServiceCollectionExtensions", + "Microsoft.AspNetCore.Authorization.AuthorizationOptions" }; } From 3c25e58a37e5dfefa93764b975ddb0282cb5c90d Mon Sep 17 00:00:00 2001 From: David Acker Date: Sat, 25 Mar 2023 16:15:07 -0400 Subject: [PATCH 03/26] Add private AuthorizationOptionsTypes class --- .../AddAuthorizationBuilderAnalyzer.cs | 75 +++++++++++++------ 1 file changed, 51 insertions(+), 24 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index b5891e75f3ec..43f659d8d6ed 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -28,6 +28,12 @@ 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); var addAuthorizationMethod = policyServiceCollectionExtensions.GetMembers() .OfType() @@ -40,7 +46,7 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) if (invocation.TargetMethod.Parameters.Length == 2 && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, policyServiceCollectionExtensions) && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) - && IsConfigureActionCompatibleWithAuthorizationBuilder(invocation, wellKnownTypes)) + && IsConfigureActionCompatibleWithAuthorizationBuilder(invocation, authorizationOptionsTypes)) { AddDiagnosticInformation(context, invocation.Syntax.GetLocation()); } @@ -48,7 +54,7 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) }, OperationKind.Invocation); } - private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, WellKnownTypes wellKnownTypes) + private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, AuthorizationOptionsTypes authorizationOptionsTypes) { if (invocation.Arguments is not { Length: 2 }) { @@ -57,30 +63,15 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat var configureAction = invocation.Arguments[1]; - var authorizationOptions = wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Authorization_AuthorizationOptions); - var authorizationOptionsMembers = authorizationOptions.GetMembers(); - - var defaultPolicyProperty = authorizationOptionsMembers.First(member => - member.Kind == SymbolKind.Property && member.Name == "DefaultPolicy"); - - var fallbackPolicyProperty = authorizationOptionsMembers.First(member => - member.Kind == SymbolKind.Property && member.Name == "FallbackPolicy"); - - var invokeHandlersAfterFailureProperty = authorizationOptionsMembers.First(member => - member.Kind == SymbolKind.Property && member.Name == "InvokeHandlersAfterFailure"); - - var getPolicyMethod = authorizationOptionsMembers.First(member => - member.Kind == SymbolKind.Method && member.Name == "GetPolicy"); - return !configureAction.Descendants().Any(operation => { if (operation is IPropertyReferenceOperation propertyReferenceOperation) { var property = propertyReferenceOperation.Property; - if (SymbolEqualityComparer.Default.Equals(property, defaultPolicyProperty) - || SymbolEqualityComparer.Default.Equals(property, fallbackPolicyProperty) - || SymbolEqualityComparer.Default.Equals(property, invokeHandlersAfterFailureProperty)) + if (SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.DefaultPolicy) + || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.FallbackPolicy) + || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.InvokeHandlersAfterFailure)) { return true; } @@ -90,8 +81,8 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat if (operation is IMethodReferenceOperation methodReferenceOperation) { - if (SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, getPolicyMethod) - && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingSymbol, authorizationOptions)) + if (SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, authorizationOptionsTypes.GetPolicy) + && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) { return true; } @@ -101,8 +92,8 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat if (operation is IInvocationOperation invocationOperation) { - if (SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingType, authorizationOptions) - && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, getPolicyMethod)) + if (SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingType, authorizationOptionsTypes.AuthorizationOptions) + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy)) { return true; } @@ -120,4 +111,40 @@ private static void AddDiagnosticInformation(OperationAnalysisContext context, L DiagnosticDescriptors.UseAddAuthorizationBuilder, location)); } + private 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(); + + DefaultPolicy = authorizationOptionsProperties + .FirstOrDefault(member => member.Name == "DefaultPolicy"); + FallbackPolicy = authorizationOptionsProperties + .FirstOrDefault(member => member.Name == "FallbackPolicy"); + InvokeHandlersAfterFailure = authorizationOptionsProperties + .FirstOrDefault(member => member.Name == "InvokeHandlersAfterFailure"); + + GetPolicy = authorizationOptionsMembers.OfType() + .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; + } } From 5f960b4284c380fdd9589f00d5be6f96a54f1a3a Mon Sep 17 00:00:00 2001 From: David Acker Date: Sat, 25 Mar 2023 19:23:21 -0400 Subject: [PATCH 04/26] Replace DefaultPolicy, FallbackPolicy assignments with SetDefaultPolicy, AddFallbackPolicy invocations --- .../AddAuthorizationBuilderAnalyzer.cs | 6 ++ .../AddAuthorizationBuilderFixer.cs | 41 ++++++++- .../AddAuthorizationBuilderTests.cs | 87 ++++++++++++++++++- 3 files changed, 131 insertions(+), 3 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index 43f659d8d6ed..9e5151fae1c6 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -73,6 +73,12 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.FallbackPolicy) || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.InvokeHandlersAfterFailure)) { + if (propertyReferenceOperation.Parent is IAssignmentOperation { Target: IPropertyReferenceOperation targetProperty } assignmentOperation + && SymbolEqualityComparer.Default.Equals(property, targetProperty.Property)) + { + return false; + } + return true; } diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index 0459d566a8e2..44a63a98ace7 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -5,7 +5,6 @@ using System.Collections.Immutable; using System.Composition; using System.Diagnostics.CodeAnalysis; -using System.Linq; using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; @@ -73,7 +72,7 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, var lambdaNodes = lambdaBody.DescendantNodes(); - foreach (var configureAction in lambdaNodes.OfType()) + foreach (var configureAction in lambdaNodes) { if (configureAction is InvocationExpressionSyntax { ArgumentList.Arguments: { } configureArguments, Expression: MemberAccessExpressionSyntax { Name.Identifier.Text: "AddPolicy" } }) { @@ -93,6 +92,44 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, SyntaxFactory.SeparatedList(configureArguments))); } } + else if (configureAction is AssignmentExpressionSyntax assignmentSyntax) + { + if (assignmentSyntax.Left is MemberAccessExpressionSyntax { Name.Identifier.Text: { } assignmentTargetName }) + { + if (assignmentTargetName == "DefaultPolicy") + { + invocation = SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + invocation.WithTrailingTrivia( + SyntaxFactory.EndOfLine(Environment.NewLine), + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space), + SyntaxFactory.IdentifierName("SetDefaultPolicy")), + SyntaxFactory.ArgumentList( + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.Argument(assignmentSyntax.Right)))); + } + else if (assignmentTargetName == "FallbackPolicy") + { + invocation = SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + invocation.WithTrailingTrivia( + SyntaxFactory.EndOfLine(Environment.NewLine), + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space), + SyntaxFactory.IdentifierName("SetFallbackPolicy")), + SyntaxFactory.ArgumentList( + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.Argument(assignmentSyntax.Right)))); + } + } + } } return true; diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index b40a4dcc10aa..e7d33d5f30ab 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -11,7 +11,6 @@ namespace Microsoft.AspNetCore.Analyzers.Authorization; public sealed class AddAuthorizationBuilderTests { // TODO: Additional Test Cases - // - Refactor assignment to AuthorizationOptions.FallbackPolicy to use AuthorizationBuilder.SetFallbackPolicy // - Refactoring to call InvokeHandlersAfterFailure instead of using setter // - Other IServiceCollection extension is changed to AddAuthorization call. // to keep things simple, just check if the parent of the InvocationExpression is a GlobalStatementExpression @@ -182,6 +181,92 @@ public async Task AddAuthorization_WithMultipleAddPolicyCalls_UsingBlockBody_Fix await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); } + [Fact] + public async Task AddAuthorization_WithAuthorizationOptionsDefaultPolicyAssignment_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => +{ + options.DefaultPolicy = new AuthorizationPolicyBuilder() + .RequireClaim(""Claim"") + .Build(); + + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +})|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .SetDefaultPolicy(new AuthorizationPolicyBuilder() + .RequireClaim(""Claim"") + .Build()) + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + + [Fact] + public async Task AddAuthorization_WithAuthorizationOptionsFallbackPolicyAssignment_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => +{ + options.FallbackPolicy = new AuthorizationPolicyBuilder() + .RequireClaim(""Claim"") + .Build(); + + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +})|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .SetFallbackPolicy(new AuthorizationPolicyBuilder() + .RequireClaim(""Claim"") + .Build()) + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + [Fact] public async Task AddAuthorization_AccessesAuthorizationOptionsDefaultPolicy_NoDiagnostic() { From 41cb8ad2f2c99291d3750bb3d30e2e7faa6e57b4 Mon Sep 17 00:00:00 2001 From: David Acker Date: Sat, 25 Mar 2023 19:52:14 -0400 Subject: [PATCH 05/26] Replace InvokeHandlersAfterFailure assignment to SetInvokeHandlersAfterFailure invocation --- .../AddAuthorizationBuilderFixer.cs | 16 ++++++++ .../AddAuthorizationBuilderTests.cs | 39 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index 44a63a98ace7..e96092f0958d 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -128,6 +128,22 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, SyntaxFactory.SingletonSeparatedList( SyntaxFactory.Argument(assignmentSyntax.Right)))); } + else if (assignmentTargetName == "InvokeHandlersAfterFailure") + { + invocation = SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + invocation.WithTrailingTrivia( + SyntaxFactory.EndOfLine(Environment.NewLine), + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space), + SyntaxFactory.IdentifierName("SetInvokeHandlersAfterFailure")), + SyntaxFactory.ArgumentList( + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.Argument(assignmentSyntax.Right)))); + } } } } diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index e7d33d5f30ab..10b8177db349 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -267,6 +267,45 @@ public async Task AddAuthorization_WithAuthorizationOptionsFallbackPolicyAssignm await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); } + [Fact] + public async Task AddAuthorization_WithAuthorizationOptionsInvokeHandlersAfterFailureAssignment_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => +{ + options.InvokeHandlersAfterFailure = false; + + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +})|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .SetInvokeHandlersAfterFailure(false) + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + [Fact] public async Task AddAuthorization_AccessesAuthorizationOptionsDefaultPolicy_NoDiagnostic() { From 9025b8a1a6ec64854ebbd3de442a0e9f05cdd3e4 Mon Sep 17 00:00:00 2001 From: David Acker Date: Sat, 25 Mar 2023 20:24:28 -0400 Subject: [PATCH 06/26] Ensure that AddAuthorization is last call in chain --- .../AddAuthorizationBuilderAnalyzer.cs | 15 ++++- .../AddAuthorizationBuilderTests.cs | 64 +++++++++++++++++-- 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index 9e5151fae1c6..b0145114b7e8 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -46,6 +46,7 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) if (invocation.TargetMethod.Parameters.Length == 2 && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, policyServiceCollectionExtensions) && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) + && IsLastCallInChain(invocation) && IsConfigureActionCompatibleWithAuthorizationBuilder(invocation, authorizationOptionsTypes)) { AddDiagnosticInformation(context, invocation.Syntax.GetLocation()); @@ -98,8 +99,8 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat if (operation is IInvocationOperation invocationOperation) { - if (SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingType, authorizationOptionsTypes.AuthorizationOptions) - && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy)) + if (SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy) + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) { return true; } @@ -111,6 +112,16 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat }); } + private static bool IsLastCallInChain(IInvocationOperation invocation) + { + if (invocation.Parent is IExpressionStatementOperation) + { + return true; + } + + return false; + } + private static void AddDiagnosticInformation(OperationAnalysisContext context, Location location) { context.ReportDiagnostic(Diagnostic.Create( diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 10b8177db349..9057079622a3 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -10,11 +10,6 @@ namespace Microsoft.AspNetCore.Analyzers.Authorization; public sealed class AddAuthorizationBuilderTests { - // TODO: Additional Test Cases - // - Refactoring to call InvokeHandlersAfterFailure instead of using setter - // - Other IServiceCollection extension is changed to AddAuthorization call. - // to keep things simple, just check if the parent of the InvocationExpression is a GlobalStatementExpression - [Fact] public async Task AddAuthorization_WithSingleAddPolicyCall_UsingExpressionBody_FixedWithAddAuthorizationBuilder() { @@ -306,6 +301,65 @@ public async Task AddAuthorization_WithAuthorizationOptionsInvokeHandlersAfterFa await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); } + [Fact] + public async Task AddAuthorization_IsTheLastCallInChain_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddRouting() + .AddAuthorization(options => + { + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); + })|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddRouting() + .AddAuthorizationBuilder() + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + + [Fact] + public async Task AddAuthorization_IsNotTheLastCallInChain_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +}) +.AddAuthentication(); +"; + + await VerifyNoCodeFix(source); + } + [Fact] public async Task AddAuthorization_AccessesAuthorizationOptionsDefaultPolicy_NoDiagnostic() { From b7e9aa776ec5e728bb9f117560e33c035ad3910b Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 26 Mar 2023 11:47:14 -0400 Subject: [PATCH 07/26] Refactor --- .../AddAuthorizationBuilderAnalyzer.cs | 31 ++----- .../AddAuthorizationBuilderFixer.cs | 87 ++++++------------- 2 files changed, 37 insertions(+), 81 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index b0145114b7e8..834bdca234fa 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -86,26 +86,18 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat return false; } - if (operation is IMethodReferenceOperation methodReferenceOperation) + if (operation is IMethodReferenceOperation methodReferenceOperation + && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, authorizationOptionsTypes.GetPolicy) + && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) { - if (SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, authorizationOptionsTypes.GetPolicy) - && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) - { - return true; - } - - return false; + return true; } - if (operation is IInvocationOperation invocationOperation) + if (operation is IInvocationOperation invocationOperation + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy) + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) { - if (SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy) - && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) - { - return true; - } - - return false; + return true; } return false; @@ -114,12 +106,7 @@ private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocat private static bool IsLastCallInChain(IInvocationOperation invocation) { - if (invocation.Parent is IExpressionStatementOperation) - { - return true; - } - - return false; + return invocation.Parent is IExpressionStatementOperation; } private static void AddDiagnosticInformation(OperationAnalysisContext context, Location location) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index e96092f0958d..eacd13262fff 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -78,72 +78,23 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, { if (configureArguments.Count == 2) { - invocation = SyntaxFactory.InvocationExpression( - SyntaxFactory.MemberAccessExpression( - SyntaxKind.SimpleMemberAccessExpression, - invocation.WithTrailingTrivia( - SyntaxFactory.EndOfLine(Environment.NewLine), - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space), - SyntaxFactory.IdentifierName("AddPolicy")), + invocation = ChainInvocation( + invocation, + "AddPolicy", SyntaxFactory.ArgumentList( SyntaxFactory.SeparatedList(configureArguments))); } } else if (configureAction is AssignmentExpressionSyntax assignmentSyntax) { - if (assignmentSyntax.Left is MemberAccessExpressionSyntax { Name.Identifier.Text: { } assignmentTargetName }) + if (assignmentSyntax.Left is MemberAccessExpressionSyntax { Name.Identifier.Text: { } assignmentTargetName } + && assignmentTargetName is "DefaultPolicy" or "FallbackPolicy" or "InvokeHandlersAfterFailure") { - if (assignmentTargetName == "DefaultPolicy") - { - invocation = SyntaxFactory.InvocationExpression( - SyntaxFactory.MemberAccessExpression( - SyntaxKind.SimpleMemberAccessExpression, - invocation.WithTrailingTrivia( - SyntaxFactory.EndOfLine(Environment.NewLine), - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space), - SyntaxFactory.IdentifierName("SetDefaultPolicy")), - SyntaxFactory.ArgumentList( - SyntaxFactory.SingletonSeparatedList( - SyntaxFactory.Argument(assignmentSyntax.Right)))); - } - else if (assignmentTargetName == "FallbackPolicy") - { - invocation = SyntaxFactory.InvocationExpression( - SyntaxFactory.MemberAccessExpression( - SyntaxKind.SimpleMemberAccessExpression, - invocation.WithTrailingTrivia( - SyntaxFactory.EndOfLine(Environment.NewLine), - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space), - SyntaxFactory.IdentifierName("SetFallbackPolicy")), - SyntaxFactory.ArgumentList( - SyntaxFactory.SingletonSeparatedList( - SyntaxFactory.Argument(assignmentSyntax.Right)))); - } - else if (assignmentTargetName == "InvokeHandlersAfterFailure") - { - invocation = SyntaxFactory.InvocationExpression( - SyntaxFactory.MemberAccessExpression( - SyntaxKind.SimpleMemberAccessExpression, - invocation.WithTrailingTrivia( - SyntaxFactory.EndOfLine(Environment.NewLine), - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space), - SyntaxFactory.IdentifierName("SetInvokeHandlersAfterFailure")), - SyntaxFactory.ArgumentList( - SyntaxFactory.SingletonSeparatedList( - SyntaxFactory.Argument(assignmentSyntax.Right)))); - } + invocation = ChainInvocation( + invocation, $"Set{assignmentTargetName}", + SyntaxFactory.ArgumentList( + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.Argument(assignmentSyntax.Right)))); } } } @@ -155,6 +106,24 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, return false; } + private static InvocationExpressionSyntax ChainInvocation( + InvocationExpressionSyntax invocation, + string invokedMemberName, + ArgumentListSyntax argumentList) + { + return SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression( + SyntaxKind.SimpleMemberAccessExpression, + invocation.WithTrailingTrivia( + SyntaxFactory.EndOfLine(Environment.NewLine), + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space), + SyntaxFactory.IdentifierName(invokedMemberName)), + argumentList); + } + private static Task ReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, SyntaxNode root, Document document, InvocationExpressionSyntax invocation) { var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); From 16b6ff46ac08d634f432707fa1be05714c020dcd Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 26 Mar 2023 12:00:04 -0400 Subject: [PATCH 08/26] Handle AddAuthorization configure actions that use expression body --- .../AddAuthorizationBuilderFixer.cs | 17 +++++++--- .../AddAuthorizationBuilderTests.cs | 34 +++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index eacd13262fff..d3edcee05175 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; using System.Diagnostics.CodeAnalysis; @@ -60,7 +61,17 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, if (arguments.Count == 1 && arguments[0].Expression is SimpleLambdaExpressionSyntax lambda) { - if (lambda.Body is not BlockSyntax lambdaBody) + IEnumerable nodes; + + if (lambda.Body is BlockSyntax lambdaBlockBody) + { + nodes = lambdaBlockBody.DescendantNodes(); + } + else if (lambda.Body is InvocationExpressionSyntax lambdaExpressionBody) + { + nodes = new[] { lambdaExpressionBody }; + } + else { return false; } @@ -70,9 +81,7 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, invocation = SyntaxFactory.InvocationExpression(addAuthorizationBuilderMethod); - var lambdaNodes = lambdaBody.DescendantNodes(); - - foreach (var configureAction in lambdaNodes) + foreach (var configureAction in nodes) { if (configureAction is InvocationExpressionSyntax { ArgumentList.Arguments: { } configureArguments, Expression: MemberAccessExpressionSyntax { Name.Identifier.Text: "AddPolicy" } }) { diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 9057079622a3..5467d6183fcc 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -10,6 +10,40 @@ namespace Microsoft.AspNetCore.Analyzers.Authorization; public sealed class AddAuthorizationBuilderTests { + [Fact] + public async Task AddAuthorization_UsingExpressionBody_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +{|#0:builder.Services.AddAuthorization(options => + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))))|}; +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorizationBuilder() + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + [Fact] public async Task AddAuthorization_WithSingleAddPolicyCall_UsingExpressionBody_FixedWithAddAuthorizationBuilder() { From 540fbcde85fc40e37f243e53b8aef541997c2a5d Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 26 Mar 2023 14:41:48 -0400 Subject: [PATCH 09/26] Refactor, clean up test method names --- .../AddAuthorizationBuilderAnalyzer.cs | 75 +++++++++-------- .../AddAuthorizationBuilderFixer.cs | 84 +++++++++---------- .../AddAuthorizationBuilderTests.cs | 26 +++--- 3 files changed, 90 insertions(+), 95 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index 834bdca234fa..c09ffeb56f16 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -43,11 +43,11 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) { var invocation = (IInvocationOperation)context.Operation; - if (invocation.TargetMethod.Parameters.Length == 2 + if (invocation.Arguments.Length == 2 && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, policyServiceCollectionExtensions) && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) && IsLastCallInChain(invocation) - && IsConfigureActionCompatibleWithAuthorizationBuilder(invocation, authorizationOptionsTypes)) + && IsConfigureActionCompatibleWithAuthorizationBuilder(invocation.Arguments[1], authorizationOptionsTypes)) { AddDiagnosticInformation(context, invocation.Syntax.GetLocation()); } @@ -55,53 +55,56 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) }, OperationKind.Invocation); } - private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, AuthorizationOptionsTypes authorizationOptionsTypes) + private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IArgumentOperation configureAction, AuthorizationOptionsTypes authorizationOptionsTypes) { - if (invocation.Arguments is not { Length: 2 }) - { - return false; - } + var usesAuthorizationOptionsSpecificAPIs = configureAction.Descendants() + .Any(operation => + UsesAuthorizationOptionsSpecificGetters(operation, authorizationOptionsTypes) + || UsesAuthorizationOptionsGetPolicy(operation, authorizationOptionsTypes)); - var configureAction = invocation.Arguments[1]; + return !usesAuthorizationOptionsSpecificAPIs; + } - return !configureAction.Descendants().Any(operation => + private static bool UsesAuthorizationOptionsSpecificGetters(IOperation operation, AuthorizationOptionsTypes authorizationOptionsTypes) + { + if (operation is IPropertyReferenceOperation propertyReferenceOperation) { - if (operation is IPropertyReferenceOperation propertyReferenceOperation) - { - var property = propertyReferenceOperation.Property; - - if (SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.DefaultPolicy) - || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.FallbackPolicy) - || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.InvokeHandlersAfterFailure)) - { - if (propertyReferenceOperation.Parent is IAssignmentOperation { Target: IPropertyReferenceOperation targetProperty } assignmentOperation - && SymbolEqualityComparer.Default.Equals(property, targetProperty.Property)) - { - return false; - } - - return true; - } + var property = propertyReferenceOperation.Property; + if (propertyReferenceOperation.Parent is IAssignmentOperation { Target: IPropertyReferenceOperation targetProperty } assignmentOperation + && SymbolEqualityComparer.Default.Equals(property, targetProperty.Property)) + { return false; } - if (operation is IMethodReferenceOperation methodReferenceOperation - && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, authorizationOptionsTypes.GetPolicy) - && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) + if (SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.DefaultPolicy) + || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.FallbackPolicy) + || SymbolEqualityComparer.Default.Equals(property, authorizationOptionsTypes.InvokeHandlersAfterFailure)) { return true; } + } - if (operation is IInvocationOperation invocationOperation - && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy) - && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) - { - 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.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) + { + return true; + } + + if (operation is IInvocationOperation invocationOperation + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod, authorizationOptionsTypes.GetPolicy) + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) + { + return true; + } - return false; - }); + return false; } private static bool IsLastCallInChain(IInvocationOperation invocation) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index d3edcee05175..45a5d09dff73 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -56,60 +56,52 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, var diagnosticTarget = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); - if (diagnosticTarget is InvocationExpressionSyntax { ArgumentList.Arguments: { } arguments, Expression: MemberAccessExpressionSyntax { Name.Identifier: { } identifierToken } memberAccessExpression } invocationExpression) + if (diagnosticTarget is InvocationExpressionSyntax { ArgumentList.Arguments: { Count: 1 } arguments, Expression: MemberAccessExpressionSyntax { Name.Identifier: { } identifierToken } memberAccessExpression } + && arguments[0].Expression is SimpleLambdaExpressionSyntax lambda) { - if (arguments.Count == 1 - && arguments[0].Expression is SimpleLambdaExpressionSyntax lambda) - { - IEnumerable nodes; + IEnumerable nodes; - if (lambda.Body is BlockSyntax lambdaBlockBody) - { - nodes = lambdaBlockBody.DescendantNodes(); - } - else if (lambda.Body is InvocationExpressionSyntax lambdaExpressionBody) - { - nodes = new[] { lambdaExpressionBody }; - } - else - { - return false; - } + if (lambda.Body is BlockSyntax lambdaBlockBody) + { + nodes = lambdaBlockBody.DescendantNodes(); + } + else if (lambda.Body is InvocationExpressionSyntax lambdaExpressionBody) + { + nodes = new[] { lambdaExpressionBody }; + } + else + { + return false; + } - var addAuthorizationBuilderMethod = - memberAccessExpression.ReplaceToken(identifierToken, SyntaxFactory.Identifier("AddAuthorizationBuilder")); + var addAuthorizationBuilderMethod = + memberAccessExpression.ReplaceToken(identifierToken, SyntaxFactory.Identifier("AddAuthorizationBuilder")); - invocation = SyntaxFactory.InvocationExpression(addAuthorizationBuilderMethod); + invocation = SyntaxFactory.InvocationExpression(addAuthorizationBuilderMethod); - foreach (var configureAction in nodes) + foreach (var configureAction in nodes) + { + if (configureAction is InvocationExpressionSyntax { ArgumentList.Arguments: { Count: 2 } configureArguments, Expression: MemberAccessExpressionSyntax { Name.Identifier.Text: "AddPolicy" } }) { - if (configureAction is InvocationExpressionSyntax { ArgumentList.Arguments: { } configureArguments, Expression: MemberAccessExpressionSyntax { Name.Identifier.Text: "AddPolicy" } }) - { - if (configureArguments.Count == 2) - { - invocation = ChainInvocation( - invocation, - "AddPolicy", - SyntaxFactory.ArgumentList( - SyntaxFactory.SeparatedList(configureArguments))); - } - } - else if (configureAction is AssignmentExpressionSyntax assignmentSyntax) - { - if (assignmentSyntax.Left is MemberAccessExpressionSyntax { Name.Identifier.Text: { } assignmentTargetName } - && assignmentTargetName is "DefaultPolicy" or "FallbackPolicy" or "InvokeHandlersAfterFailure") - { - invocation = ChainInvocation( - invocation, $"Set{assignmentTargetName}", - SyntaxFactory.ArgumentList( - SyntaxFactory.SingletonSeparatedList( - SyntaxFactory.Argument(assignmentSyntax.Right)))); - } - } + invocation = ChainInvocation( + invocation, + "AddPolicy", + SyntaxFactory.ArgumentList( + SyntaxFactory.SeparatedList(configureArguments))); + } + else if (configureAction is AssignmentExpressionSyntax { Left: MemberAccessExpressionSyntax { Name.Identifier.Text: { } assignmentTargetName }, Right: { } assignmentExpression } + && assignmentTargetName is "DefaultPolicy" or "FallbackPolicy" or "InvokeHandlersAfterFailure") + { + invocation = ChainInvocation( + invocation, + $"Set{assignmentTargetName}", + SyntaxFactory.ArgumentList( + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.Argument(assignmentExpression)))); } - - return true; } + + return true; } return false; diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 5467d6183fcc..15a5da88ead1 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Analyzers.Authorization; public sealed class AddAuthorizationBuilderTests { [Fact] - public async Task AddAuthorization_UsingExpressionBody_FixedWithAddAuthorizationBuilder() + public async Task ConfigureAction_UsingExpressionBody_FixedWithAddAuthorizationBuilder() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -45,7 +45,7 @@ public async Task AddAuthorization_UsingExpressionBody_FixedWithAddAuthorization } [Fact] - public async Task AddAuthorization_WithSingleAddPolicyCall_UsingExpressionBody_FixedWithAddAuthorizationBuilder() + public async Task SingleAddPolicyCall_UsingExpressionBody_FixedWithAddAuthorizationBuilder() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -81,7 +81,7 @@ public async Task AddAuthorization_WithSingleAddPolicyCall_UsingExpressionBody_F } [Fact] - public async Task AddAuthorization_WithMultipleAddPolicyCalls_UsingExpressionBody_FixedWithAddAuthorizationBuilder() + public async Task MultipleAddPolicyCalls_UsingExpressionBody_FixedWithAddAuthorizationBuilder() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -122,7 +122,7 @@ public async Task AddAuthorization_WithMultipleAddPolicyCalls_UsingExpressionBod } [Fact] - public async Task AddAuthorization_WithSingleAddPolicyCall_UsingBlockBody_FixedWithAddAuthorizationBuilder() + public async Task SingleAddPolicyCall_UsingBlockBody_FixedWithAddAuthorizationBuilder() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -162,7 +162,7 @@ public async Task AddAuthorization_WithSingleAddPolicyCall_UsingBlockBody_FixedW } [Fact] - public async Task AddAuthorization_WithMultipleAddPolicyCalls_UsingBlockBody_FixedWithAddAuthorizationBuilder() + public async Task MultipleAddPolicyCalls_UsingBlockBody_FixedWithAddAuthorizationBuilder() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -211,7 +211,7 @@ public async Task AddAuthorization_WithMultipleAddPolicyCalls_UsingBlockBody_Fix } [Fact] - public async Task AddAuthorization_WithAuthorizationOptionsDefaultPolicyAssignment_FixedWithAddAuthorizationBuilder() + public async Task AuthorizationOptions_DefaultPolicyAssignment_ReplacedWithSetDefaultPolicyInvocation() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -254,7 +254,7 @@ public async Task AddAuthorization_WithAuthorizationOptionsDefaultPolicyAssignme } [Fact] - public async Task AddAuthorization_WithAuthorizationOptionsFallbackPolicyAssignment_FixedWithAddAuthorizationBuilder() + public async Task AuthorizationOptions_FallbackPolicyAssignment_ReplacedWithSetFallbackPolicyInvocation() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -297,7 +297,7 @@ public async Task AddAuthorization_WithAuthorizationOptionsFallbackPolicyAssignm } [Fact] - public async Task AddAuthorization_WithAuthorizationOptionsInvokeHandlersAfterFailureAssignment_FixedWithAddAuthorizationBuilder() + public async Task AuthorizationOptions_InvokeHandlersAfterFailureAssignment_ReplacedWithSetInvokeHandlersAfterFailureInvocation() { var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) .WithLocation(0) @@ -395,7 +395,7 @@ public async Task AddAuthorization_IsNotTheLastCallInChain_NoDiagnostic() } [Fact] - public async Task AddAuthorization_AccessesAuthorizationOptionsDefaultPolicy_NoDiagnostic() + public async Task AuthorizationOptions_DefaultPolicyAccess_NoDiagnostic() { var source = @" using Microsoft.AspNetCore.Authorization; @@ -418,7 +418,7 @@ public async Task AddAuthorization_AccessesAuthorizationOptionsDefaultPolicy_NoD } [Fact] - public async Task AddAuthorization_AccessesAuthorizationOptionsFallbackPolicy_NoDiagnostic() + public async Task AuthorizationOptions_FallbackPolicyAccess_NoDiagnostic() { var source = @" using Microsoft.AspNetCore.Authorization; @@ -441,7 +441,7 @@ public async Task AddAuthorization_AccessesAuthorizationOptionsFallbackPolicy_No } [Fact] - public async Task AddAuthorization_AccessesAuthorizationOptionsInvokeHandlesAfterFailure_NoDiagnostic() + public async Task AuthorizationOptions_InvokeHandlesAfterFailureAccess_NoDiagnostic() { var source = @" using Microsoft.AspNetCore.Authorization; @@ -464,7 +464,7 @@ public async Task AddAuthorization_AccessesAuthorizationOptionsInvokeHandlesAfte } [Fact] - public async Task AddAuthorization_ReferencesAuthorizationOptionsGetPolicy_NoDiagnostic() + public async Task AuthorizationOptions_GetPolicyReference_NoDiagnostic() { var source = @" using System; @@ -488,7 +488,7 @@ public async Task AddAuthorization_ReferencesAuthorizationOptionsGetPolicy_NoDia } [Fact] - public async Task AddAuthorization_InvokesAuthorizationOptionsGetPolicy_NoDiagnostic() + public async Task AuthorizationOptions_GetPolicyInvocation_NoDiagnostic() { var source = @" using Microsoft.AspNetCore.Authorization; From 53b0775122db32bf5aa39465918967b1b631d824 Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 26 Mar 2023 16:10:42 -0400 Subject: [PATCH 10/26] Report no diagnostic when unrelated operations are present in configure action --- .../AddAuthorizationBuilderAnalyzer.cs | 45 ++++++++++++++++- .../AddAuthorizationBuilderTests.cs | 50 +++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index c09ffeb56f16..636dc7c184ef 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -47,7 +47,7 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, policyServiceCollectionExtensions) && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) && IsLastCallInChain(invocation) - && IsConfigureActionCompatibleWithAuthorizationBuilder(invocation.Arguments[1], authorizationOptionsTypes)) + && IsCompatibleWithAuthorizationBuilder(invocation, authorizationOptionsTypes)) { AddDiagnosticInformation(context, invocation.Syntax.GetLocation()); } @@ -55,7 +55,48 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) }, OperationKind.Invocation); } - private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IArgumentOperation configureAction, AuthorizationOptionsTypes authorizationOptionsTypes) + private static bool IsCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, AuthorizationOptionsTypes authorizationOptionsTypes) + { + if (invocation.Arguments[1] is IArgumentOperation { ChildOperations: { Count: 1 } argumentOperations } + && argumentOperations.First() is IDelegateCreationOperation { ChildOperations: { Count: 1 } delegateCreationOperations } + && delegateCreationOperations.First() is IAnonymousFunctionOperation { ChildOperations: { Count: 1 } anonymousFunctionOperations } + && anonymousFunctionOperations.First() is IBlockOperation blockOperation) + { + // Ensure that the child operations of the configuration action passed to AddAuthorization are all related to AuthorizationOptions + foreach (var operation in blockOperation.ChildOperations.Where(op => op is not IReturnOperation { IsImplicit: true })) + { + if (operation is IExpressionStatementOperation { Operation: { } expressionStatementOperation }) + { + if (expressionStatementOperation is ISimpleAssignmentOperation { Target: IPropertyReferenceOperation { Property.ContainingType: { } propertyReferenceContainingType } } + && SymbolEqualityComparer.Default.Equals(propertyReferenceContainingType, authorizationOptionsTypes.AuthorizationOptions)) + { + continue; + } + + if (expressionStatementOperation is IMethodReferenceOperation { Method.ContainingType: { } methodReferenceContainingType } + && SymbolEqualityComparer.Default.Equals(methodReferenceContainingType, authorizationOptionsTypes.AuthorizationOptions)) + { + continue; + } + + if (expressionStatementOperation is IInvocationOperation { TargetMethod.ContainingType: { } invokedMethodContainingType } invocationOperation + && SymbolEqualityComparer.Default.Equals(invokedMethodContainingType, authorizationOptionsTypes.AuthorizationOptions)) + { + continue; + } + } + + return false; + } + + // Ensure that the configuration action passed to AddAuthorization does not use any AuthorizationOptions-specific APIs. + return IsConfigureActionCompatibleWithAuthorizationBuilder(blockOperation, authorizationOptionsTypes); + } + + return false; + } + + private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IBlockOperation configureAction, AuthorizationOptionsTypes authorizationOptionsTypes) { var usesAuthorizationOptionsSpecificAPIs = configureAction.Descendants() .Any(operation => diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 15a5da88ead1..b0dc5fd5b66c 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -510,6 +510,56 @@ public async Task AuthorizationOptions_GetPolicyInvocation_NoDiagnostic() await VerifyNoCodeFix(source); } + [Fact] + public async Task ConfigureAction_IsNotAnonymousFunction_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(Helper.ConfigureAuthorization); + +public static class Helper +{ + public static void ConfigureAuthorization(AuthorizationOptions options) + { + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); + } +} +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task ConfigureAction_ContainsOperationsNotRelatedToAuthorizationOptions_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + var value = 1 + 1; + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); +}); +"; + + await VerifyNoCodeFix(source); + } + private static async Task VerifyCodeFix(string source, DiagnosticResult[] diagnostics, string fixedSource) { var fullSource = string.Join(Environment.NewLine, source, _testAuthorizationPolicyClassDeclaration); From 3841e7f1ee6352153d2b225dac18e1b22cce1ee9 Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 26 Mar 2023 19:22:54 -0400 Subject: [PATCH 11/26] Refactor --- .../AddAuthorizationBuilderAnalyzer.cs | 81 +++++++++++-------- .../AddAuthorizationBuilderFixer.cs | 4 +- 2 files changed, 50 insertions(+), 35 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index 636dc7c184ef..c7e8d026441a 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -35,17 +35,26 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) } var policyServiceCollectionExtensions = wellKnownTypes.Get(WellKnownType.Microsoft_Extensions_DependencyInjection_PolicyServiceCollectionExtensions); + if (policyServiceCollectionExtensions is null) + { + return; + } + var addAuthorizationMethod = policyServiceCollectionExtensions.GetMembers() .OfType() - .Single(member => member.Parameters.Length == 2 && member.Name == "AddAuthorization"); + .FirstOrDefault(member => member is { Name: "AddAuthorization", Parameters.Length: 2 }); + + if (addAuthorizationMethod is null) + { + return; + } context.RegisterOperationAction(context => { var invocation = (IInvocationOperation)context.Operation; - if (invocation.Arguments.Length == 2 + if (SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, policyServiceCollectionExtensions) - && SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, addAuthorizationMethod) && IsLastCallInChain(invocation) && IsCompatibleWithAuthorizationBuilder(invocation, authorizationOptionsTypes)) { @@ -57,40 +66,46 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) private static bool IsCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, AuthorizationOptionsTypes authorizationOptionsTypes) { - if (invocation.Arguments[1] is IArgumentOperation { ChildOperations: { Count: 1 } argumentOperations } + if (invocation is { Arguments: { Length: 2 } invocationArguments } + && invocationArguments[1] is IArgumentOperation { ChildOperations: { Count: 1 } argumentOperations } && argumentOperations.First() is IDelegateCreationOperation { ChildOperations: { Count: 1 } delegateCreationOperations } && delegateCreationOperations.First() is IAnonymousFunctionOperation { ChildOperations: { Count: 1 } anonymousFunctionOperations } && anonymousFunctionOperations.First() is IBlockOperation blockOperation) { - // Ensure that the child operations of the configuration action passed to AddAuthorization are all related to AuthorizationOptions - foreach (var operation in blockOperation.ChildOperations.Where(op => op is not IReturnOperation { IsImplicit: true })) + // Ensure that the child operations of the configuration action passed to AddAuthorization are all related to AuthorizationOptions. + var allOperationsInvolveAuthorizationOptions = blockOperation.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(blockOperation, authorizationOptionsTypes); + } + + 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)) { - if (operation is IExpressionStatementOperation { Operation: { } expressionStatementOperation }) - { - if (expressionStatementOperation is ISimpleAssignmentOperation { Target: IPropertyReferenceOperation { Property.ContainingType: { } propertyReferenceContainingType } } - && SymbolEqualityComparer.Default.Equals(propertyReferenceContainingType, authorizationOptionsTypes.AuthorizationOptions)) - { - continue; - } - - if (expressionStatementOperation is IMethodReferenceOperation { Method.ContainingType: { } methodReferenceContainingType } - && SymbolEqualityComparer.Default.Equals(methodReferenceContainingType, authorizationOptionsTypes.AuthorizationOptions)) - { - continue; - } - - if (expressionStatementOperation is IInvocationOperation { TargetMethod.ContainingType: { } invokedMethodContainingType } invocationOperation - && SymbolEqualityComparer.Default.Equals(invokedMethodContainingType, authorizationOptionsTypes.AuthorizationOptions)) - { - continue; - } - } - - return false; + return true; + } + + if (expressionStatementOperation is IMethodReferenceOperation { Method.ContainingType: { } methodReferenceContainingType } + && SymbolEqualityComparer.Default.Equals(methodReferenceContainingType, authorizationOptionsTypes.AuthorizationOptions)) + { + return true; } - // Ensure that the configuration action passed to AddAuthorization does not use any AuthorizationOptions-specific APIs. - return IsConfigureActionCompatibleWithAuthorizationBuilder(blockOperation, authorizationOptionsTypes); + if (expressionStatementOperation is IInvocationOperation { TargetMethod.ContainingType: { } invokedMethodContainingType } invocationOperation + && SymbolEqualityComparer.Default.Equals(invokedMethodContainingType, authorizationOptionsTypes.AuthorizationOptions)) + { + return true; + } } return false; @@ -99,8 +114,7 @@ private static bool IsCompatibleWithAuthorizationBuilder(IInvocationOperation in private static bool IsConfigureActionCompatibleWithAuthorizationBuilder(IBlockOperation configureAction, AuthorizationOptionsTypes authorizationOptionsTypes) { var usesAuthorizationOptionsSpecificAPIs = configureAction.Descendants() - .Any(operation => - UsesAuthorizationOptionsSpecificGetters(operation, authorizationOptionsTypes) + .Any(operation => UsesAuthorizationOptionsSpecificGetters(operation, authorizationOptionsTypes) || UsesAuthorizationOptionsGetPolicy(operation, authorizationOptionsTypes)); return !usesAuthorizationOptionsSpecificAPIs; @@ -112,7 +126,8 @@ private static bool UsesAuthorizationOptionsSpecificGetters(IOperation operation { var property = propertyReferenceOperation.Property; - if (propertyReferenceOperation.Parent is IAssignmentOperation { Target: IPropertyReferenceOperation targetProperty } assignmentOperation + // Check that the referenced property is not being set. + if (propertyReferenceOperation.Parent is IAssignmentOperation { Target: IPropertyReferenceOperation targetProperty } && SymbolEqualityComparer.Default.Equals(property, targetProperty.Property)) { return false; diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index 45a5d09dff73..219a87baeb8c 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -74,8 +74,8 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, return false; } - var addAuthorizationBuilderMethod = - memberAccessExpression.ReplaceToken(identifierToken, SyntaxFactory.Identifier("AddAuthorizationBuilder")); + var addAuthorizationBuilderMethod = memberAccessExpression.ReplaceToken(identifierToken, + SyntaxFactory.Identifier("AddAuthorizationBuilder")); invocation = SyntaxFactory.InvocationExpression(addAuthorizationBuilderMethod); From 6ac3406dda0b89260a63736549328b97c24c3ee8 Mon Sep 17 00:00:00 2001 From: David Acker Date: Wed, 29 Mar 2023 18:26:39 -0400 Subject: [PATCH 12/26] Add Debug.Assert when CanReplaceWithAddAuthorizationBuilder returns false --- .../CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index 219a87baeb8c..47ef7a0e4daa 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Threading.Tasks; using Microsoft.CodeAnalysis; @@ -71,6 +72,7 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, } else { + Debug.Assert(false, "AddAuthorizationBuilderAnalyzer should not have emitted a diagnostic."); return false; } @@ -104,6 +106,7 @@ private static bool CanReplaceWithAddAuthorizationBuilder(Diagnostic diagnostic, return true; } + Debug.Assert(false, "AddAuthorizationBuilderAnalyzer should not have emitted a diagnostic."); return false; } From 065aff215de55e758832a36ac8196075c61e0689 Mon Sep 17 00:00:00 2001 From: David Acker Date: Wed, 29 Mar 2023 18:36:37 -0400 Subject: [PATCH 13/26] Check ContainingType instead of ContainingSymbol --- .../Authorization/AddAuthorizationBuilderAnalyzer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index c7e8d026441a..68469e7435ed 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -148,14 +148,14 @@ private static bool UsesAuthorizationOptionsGetPolicy(IOperation operation, Auth { if (operation is IMethodReferenceOperation methodReferenceOperation && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member, authorizationOptionsTypes.GetPolicy) - && SymbolEqualityComparer.Default.Equals(methodReferenceOperation.Member.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) + && 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.ContainingSymbol, authorizationOptionsTypes.AuthorizationOptions)) + && SymbolEqualityComparer.Default.Equals(invocationOperation.TargetMethod.ContainingType, authorizationOptionsTypes.AuthorizationOptions)) { return true; } From 2fb59a3245ae5d2199886393c0c9cdf2b33840cb Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 9 Apr 2023 10:25:52 -0400 Subject: [PATCH 14/26] Add tests for nested AddAuthorization calls, improve handling of leading trivia --- .../AddAuthorizationBuilderFixer.cs | 19 +++-- .../AddAuthorizationBuilderTests.cs | 83 +++++++++++++++++++ 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index 47ef7a0e4daa..eeb4e5c6e26e 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -7,6 +7,7 @@ using System.Composition; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; @@ -115,15 +116,21 @@ private static InvocationExpressionSyntax ChainInvocation( string invokedMemberName, ArgumentListSyntax argumentList) { + var invocationLeadingTrivia = invocation.GetLeadingTrivia() + .Where(trivia => !trivia.IsKind(SyntaxKind.EndOfLineTrivia)); + var newInvocationTrivia = new SyntaxTriviaList( + SyntaxFactory.EndOfLine( + Environment.NewLine), + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space, + SyntaxFactory.Space) + .AddRange(invocationLeadingTrivia); + return SyntaxFactory.InvocationExpression( SyntaxFactory.MemberAccessExpression( SyntaxKind.SimpleMemberAccessExpression, - invocation.WithTrailingTrivia( - SyntaxFactory.EndOfLine(Environment.NewLine), - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space), + invocation.WithTrailingTrivia(newInvocationTrivia), SyntaxFactory.IdentifierName(invokedMemberName)), argumentList); } diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index b0dc5fd5b66c..48f87f1a3df0 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -544,6 +544,7 @@ public async Task ConfigureAction_ContainsOperationsNotRelatedToAuthorizationOpt using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Builder; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; var builder = WebApplication.CreateBuilder(args); @@ -560,6 +561,88 @@ public async Task ConfigureAction_ContainsOperationsNotRelatedToAuthorizationOpt await VerifyNoCodeFix(source); } + [Fact] + public async Task NestedAddAuthorization_UsingBlockBody_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +var builder = new HostBuilder() + .ConfigureServices((context, services) => + { + {|#0:services.AddAuthorization(options => + { + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); + })|}; + }); +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +var builder = new HostBuilder() + .ConfigureServices((context, services) => + { + services.AddAuthorizationBuilder() + .AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); + }); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + + [Fact] + public async Task NestedAddAuthorization_UsingExpressionBody_FixedWithAddAuthorizationBuilder() + { + var diagnostic = new DiagnosticResult(DiagnosticDescriptors.UseAddAuthorizationBuilder) + .WithLocation(0) + .WithMessage(Resources.Analyzer_UseAddAuthorizationBuilder_Message); + + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +var builder = new HostBuilder() + .ConfigureServices((context, services) => + { + {|#0:services.AddAuthorization(options => + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))))|}; + }); +"; + + var fixedSource = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +var builder = new HostBuilder() + .ConfigureServices((context, services) => + { + services.AddAuthorizationBuilder() + .AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21))); + }); +"; + + await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); + } + private static async Task VerifyCodeFix(string source, DiagnosticResult[] diagnostics, string fixedSource) { var fullSource = string.Join(Environment.NewLine, source, _testAuthorizationPolicyClassDeclaration); From 25fdcd91e47b8bdf8caaeed04c3f37abb623b23b Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 30 Apr 2023 13:56:20 -0400 Subject: [PATCH 15/26] Remove unnecessary assignment --- .../Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index 68469e7435ed..bcdcc2901893 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -101,7 +101,7 @@ private static bool DoesOperationInvolveAuthorizationOptions(IOperation operatio return true; } - if (expressionStatementOperation is IInvocationOperation { TargetMethod.ContainingType: { } invokedMethodContainingType } invocationOperation + if (expressionStatementOperation is IInvocationOperation { TargetMethod.ContainingType: { } invokedMethodContainingType } && SymbolEqualityComparer.Default.Equals(invokedMethodContainingType, authorizationOptionsTypes.AuthorizationOptions)) { return true; From ba61d755ed50feaed8da1a296e85883c4ee1a5be Mon Sep 17 00:00:00 2001 From: David Acker Date: Sun, 30 Apr 2023 14:23:24 -0400 Subject: [PATCH 16/26] Call ReplaceLineEndings on source and fixedSource --- .../test/Verifiers/CSharpAnalyzerVerifier.cs | 2 +- .../test/Verifiers/CSharpCodeFixVerifier.cs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs index db13c60dbe9c..623ad13e3ab1 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs @@ -34,7 +34,7 @@ public static async Task VerifyAnalyzerAsync(string source, params DiagnosticRes { var test = new CSharpAnalyzerTest { - TestCode = source, + TestCode = source.ReplaceLineEndings(), // We need to set the output type to an exe to properly // support top-level programs in the tests. Otherwise, // the test infra will assume we are trying to build a library. diff --git a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs index 124b40083885..d3e39d009851 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpCodeFixVerifier.cs @@ -31,7 +31,7 @@ public static async Task VerifyAnalyzerAsync(string source, params DiagnosticRes { var test = new CSharpCodeFixTest { - TestCode = source, + TestCode = source.ReplaceLineEndings(), // We need to set the output type to an exe to properly // support top-level programs in the tests. Otherwise, // the test infra will assume we are trying to build a library. @@ -59,8 +59,8 @@ public static async Task VerifyCodeFixAsync(string source, DiagnosticResult[] ex // We need to set the output type to an exe to properly // support top-level programs in the tests. Otherwise, // the test infra will assume we are trying to build a library. - TestState = { Sources = { source }, OutputKind = OutputKind.ConsoleApplication }, - FixedState = { Sources = { fixedSource } }, + TestState = { Sources = { source.ReplaceLineEndings() }, OutputKind = OutputKind.ConsoleApplication }, + FixedState = { Sources = { fixedSource.ReplaceLineEndings() } }, ReferenceAssemblies = CSharpAnalyzerVerifier.GetReferenceAssemblies(), NumberOfFixAllIterations = expectedIterations, CodeActionEquivalenceKey = codeActionEquivalenceKey From d889120471f801e6136fc943130bac17cf2db999 Mon Sep 17 00:00:00 2001 From: David Acker Date: Wed, 3 May 2023 06:27:23 -0400 Subject: [PATCH 17/26] Move AuthorizationOptionsTypes to separate file --- .../AddAuthorizationBuilderAnalyzer.cs | 36 -------------- .../AuthorizationOptionsTypes.cs | 47 +++++++++++++++++++ 2 files changed, 47 insertions(+), 36 deletions(-) create mode 100644 src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AuthorizationOptionsTypes.cs diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index bcdcc2901893..e438d994e2bc 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -174,40 +174,4 @@ private static void AddDiagnosticInformation(OperationAnalysisContext context, L DiagnosticDescriptors.UseAddAuthorizationBuilder, location)); } - private 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(); - - DefaultPolicy = authorizationOptionsProperties - .FirstOrDefault(member => member.Name == "DefaultPolicy"); - FallbackPolicy = authorizationOptionsProperties - .FirstOrDefault(member => member.Name == "FallbackPolicy"); - InvokeHandlersAfterFailure = authorizationOptionsProperties - .FirstOrDefault(member => member.Name == "InvokeHandlersAfterFailure"); - - GetPolicy = authorizationOptionsMembers.OfType() - .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; - } } diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AuthorizationOptionsTypes.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AuthorizationOptionsTypes.cs new file mode 100644 index 000000000000..ceada6264288 --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AuthorizationOptionsTypes.cs @@ -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(); + + DefaultPolicy = authorizationOptionsProperties + .FirstOrDefault(member => member.Name == "DefaultPolicy"); + FallbackPolicy = authorizationOptionsProperties + .FirstOrDefault(member => member.Name == "FallbackPolicy"); + InvokeHandlersAfterFailure = authorizationOptionsProperties + .FirstOrDefault(member => member.Name == "InvokeHandlersAfterFailure"); + + GetPolicy = authorizationOptionsMembers.OfType() + .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; +} From e82c10baa059a0ba0160deafc7a0688c52339efc Mon Sep 17 00:00:00 2001 From: David Acker Date: Wed, 3 May 2023 06:28:01 -0400 Subject: [PATCH 18/26] Make configure a named parameter --- .../test/Authorization/AddAuthorizationBuilderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 48f87f1a3df0..7ff81cc6ead3 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -24,7 +24,7 @@ public async Task ConfigureAction_UsingExpressionBody_FixedWithAddAuthorizationB var builder = WebApplication.CreateBuilder(args); -{|#0:builder.Services.AddAuthorization(options => +{|#0:builder.Services.AddAuthorization(configure: options => options.AddPolicy(""AtLeast21"", policy => policy.Requirements.Add(new MinimumAgeRequirement(21))))|}; "; From 404286e09895b0bf0f1bae34bf7a9a3d3ab9becb Mon Sep 17 00:00:00 2001 From: David Acker Date: Wed, 3 May 2023 19:55:38 -0400 Subject: [PATCH 19/26] Use DiagnosticDescriptor Id as code fix equivalence key --- .../src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index eeb4e5c6e26e..d53d64b126d3 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -46,7 +46,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) context.RegisterCodeFix( CodeAction.Create(title, cancellationToken => ReplaceWithAddAuthorizationBuilder(diagnostic, root, context.Document, invocation), - equivalenceKey: title), + equivalenceKey: DiagnosticDescriptors.UseAddAuthorizationBuilder.Id), diagnostic); } } From 57e5ee2cdc20990650b294511b088d0a0375ce09 Mon Sep 17 00:00:00 2001 From: David Acker Date: Wed, 3 May 2023 20:05:14 -0400 Subject: [PATCH 20/26] Use SyntaxFactory.Tab --- .../Authorization/AddAuthorizationBuilderFixer.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs index d53d64b126d3..78b60966bb58 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/CodeFixes/Authorization/AddAuthorizationBuilderFixer.cs @@ -119,12 +119,8 @@ private static InvocationExpressionSyntax ChainInvocation( var invocationLeadingTrivia = invocation.GetLeadingTrivia() .Where(trivia => !trivia.IsKind(SyntaxKind.EndOfLineTrivia)); var newInvocationTrivia = new SyntaxTriviaList( - SyntaxFactory.EndOfLine( - Environment.NewLine), - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space, - SyntaxFactory.Space) + SyntaxFactory.EndOfLine(Environment.NewLine), + SyntaxFactory.Tab) .AddRange(invocationLeadingTrivia); return SyntaxFactory.InvocationExpression( From f53ec3423f2749f39165ed756ed53be163547ef0 Mon Sep 17 00:00:00 2001 From: David Acker Date: Fri, 5 May 2023 11:32:06 -0400 Subject: [PATCH 21/26] Add test for assigning AddAuthorization call to variable --- .../AddAuthorizationBuilderTests.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 7ff81cc6ead3..c2924753588a 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -643,6 +643,24 @@ public async Task NestedAddAuthorization_UsingExpressionBody_FixedWithAddAuthori await VerifyCodeFix(source, new[] { diagnostic }, fixedSource); } + [Fact] + public async Task AddAuthorization_CallAssignedToVariable_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +var services = builder.Services.AddAuthorization(options => + options.AddPolicy(""AtLeast21"", policy => + policy.Requirements.Add(new MinimumAgeRequirement(21)))); +"; + + await VerifyNoCodeFix(source); + } + private static async Task VerifyCodeFix(string source, DiagnosticResult[] diagnostics, string fixedSource) { var fullSource = string.Join(Environment.NewLine, source, _testAuthorizationPolicyClassDeclaration); From 3783f77c41077a284ea8227da6deaee82fa5ec94 Mon Sep 17 00:00:00 2001 From: David Acker Date: Fri, 5 May 2023 11:44:45 -0400 Subject: [PATCH 22/26] Add test for scenario where AuthorizationOptions is passed to method call --- .../AddAuthorizationBuilderTests.cs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index c2924753588a..7f0f02dd6b27 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -522,6 +522,33 @@ public async Task ConfigureAction_IsNotAnonymousFunction_NoDiagnostic() builder.Services.AddAuthorization(Helper.ConfigureAuthorization); +public static class Helper +{ + public static void ConfigureAuthorization(AuthorizationOptions options) + { + options.AddPolicy(""AtLeast21"", policy => + { + policy.Requirements.Add(new MinimumAgeRequirement(21)); + }); + } +} +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task ConfigureAction_AuthorizationOptionsPassedToMethodCall_NoDiagnostic() + { + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => Helper.ConfigureAuthorization(options)); + public static class Helper { public static void ConfigureAuthorization(AuthorizationOptions options) From 2451fa296ae5cf5df04918d0e0f850c34446af73 Mon Sep 17 00:00:00 2001 From: David Acker Date: Fri, 5 May 2023 14:51:03 -0400 Subject: [PATCH 23/26] Fix GetPolicy reference, invocation tests --- .../Authorization/AddAuthorizationBuilderTests.cs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 7f0f02dd6b27..6bfcefe6564d 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -476,11 +476,7 @@ public async Task AuthorizationOptions_GetPolicyReference_NoDiagnostic() builder.Services.AddAuthorization(options => { - Func _ = options.GetPolicy; - options.AddPolicy(""AtLeast21"", policy => - { - policy.Requirements.Add(new MinimumAgeRequirement(21)); - }); + options.AddPolicy(""AtLeast21"", ((Func)options.GetPolicy)(string.Empty)!); }); "; @@ -499,11 +495,7 @@ public async Task AuthorizationOptions_GetPolicyInvocation_NoDiagnostic() builder.Services.AddAuthorization(options => { - _ = options.GetPolicy(""""); - options.AddPolicy(""AtLeast21"", policy => - { - policy.Requirements.Add(new MinimumAgeRequirement(21)); - }); + options.AddPolicy(""AtLeast21"", options.GetPolicy(string.Empty)!); }); "; From a87a7a30e49c538b176ea39d21ee488301a74ce3 Mon Sep 17 00:00:00 2001 From: David Acker Date: Fri, 5 May 2023 15:24:30 -0400 Subject: [PATCH 24/26] Add, update tests for getters and setters, guard against self assignment --- .../AddAuthorizationBuilderAnalyzer.cs | 8 +++ .../AddAuthorizationBuilderTests.cs | 69 +++++++++++++++---- 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index e438d994e2bc..b933bd5a274e 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -130,6 +130,14 @@ private static bool UsesAuthorizationOptionsSpecificGetters(IOperation operation 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; } diff --git a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs index 6bfcefe6564d..6d56ec02991c 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Authorization/AddAuthorizationBuilderTests.cs @@ -406,11 +406,26 @@ public async Task AuthorizationOptions_DefaultPolicyAccess_NoDiagnostic() builder.Services.AddAuthorization(options => { - _ = options.DefaultPolicy; - options.AddPolicy(""AtLeast21"", policy => + options.FallbackPolicy = options.DefaultPolicy; +}); +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task AuthorizationOptions_DefaultPolicyAccess_SelfAssignment_NoDiagnostic() { - policy.Requirements.Add(new MinimumAgeRequirement(21)); - }); + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + options.DefaultPolicy = options.DefaultPolicy; }); "; @@ -429,11 +444,26 @@ public async Task AuthorizationOptions_FallbackPolicyAccess_NoDiagnostic() builder.Services.AddAuthorization(options => { - _ = options.FallbackPolicy; - options.AddPolicy(""AtLeast21"", policy => + options.DefaultPolicy = options.FallbackPolicy; +}); +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task AuthorizationOptions_FallbackPolicyAccess_SelfAssignment_NoDiagnostic() { - policy.Requirements.Add(new MinimumAgeRequirement(21)); - }); + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + options.FallbackPolicy = options.FallbackPolicy; }); "; @@ -452,11 +482,26 @@ public async Task AuthorizationOptions_InvokeHandlesAfterFailureAccess_NoDiagnos builder.Services.AddAuthorization(options => { - _ = options.InvokeHandlersAfterFailure; - options.AddPolicy(""AtLeast21"", policy => + options.InvokeHandlersAfterFailure = !options.InvokeHandlersAfterFailure; +}); +"; + + await VerifyNoCodeFix(source); + } + + [Fact] + public async Task AuthorizationOptions_InvokeHandlesAfterFailureAccess_SelfAssignment_NoDiagnostic() { - policy.Requirements.Add(new MinimumAgeRequirement(21)); - }); + var source = @" +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; + +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddAuthorization(options => +{ + options.InvokeHandlersAfterFailure = options.InvokeHandlersAfterFailure; }); "; From ef0830216fb09944c95234a0c299a6c13d5b3ee1 Mon Sep 17 00:00:00 2001 From: David Acker Date: Fri, 5 May 2023 15:47:08 -0400 Subject: [PATCH 25/26] Remove unreachable MethodReferenceOperation check --- .../Authorization/AddAuthorizationBuilderAnalyzer.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index b933bd5a274e..560410bad6d6 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -95,12 +95,6 @@ private static bool DoesOperationInvolveAuthorizationOptions(IOperation operatio return true; } - if (expressionStatementOperation is IMethodReferenceOperation { Method.ContainingType: { } methodReferenceContainingType } - && SymbolEqualityComparer.Default.Equals(methodReferenceContainingType, authorizationOptionsTypes.AuthorizationOptions)) - { - return true; - } - if (expressionStatementOperation is IInvocationOperation { TargetMethod.ContainingType: { } invokedMethodContainingType } && SymbolEqualityComparer.Default.Equals(invokedMethodContainingType, authorizationOptionsTypes.AuthorizationOptions)) { From f3af8dc59b379661d82b07163579978348532a9f Mon Sep 17 00:00:00 2001 From: David Acker Date: Fri, 5 May 2023 19:10:42 -0400 Subject: [PATCH 26/26] Refactor pattern checks into separate methods --- .../AddAuthorizationBuilderAnalyzer.cs | 69 +++++++++++++++++-- 1 file changed, 62 insertions(+), 7 deletions(-) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs index 560410bad6d6..957d8eb8345f 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Authorization/AddAuthorizationBuilderAnalyzer.cs @@ -2,6 +2,7 @@ // 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; @@ -66,20 +67,74 @@ private static void OnCompilationStart(CompilationStartAnalysisContext context) private static bool IsCompatibleWithAuthorizationBuilder(IInvocationOperation invocation, AuthorizationOptionsTypes authorizationOptionsTypes) { - if (invocation is { Arguments: { Length: 2 } invocationArguments } - && invocationArguments[1] is IArgumentOperation { ChildOperations: { Count: 1 } argumentOperations } - && argumentOperations.First() is IDelegateCreationOperation { ChildOperations: { Count: 1 } delegateCreationOperations } - && delegateCreationOperations.First() is IAnonymousFunctionOperation { ChildOperations: { Count: 1 } anonymousFunctionOperations } - && anonymousFunctionOperations.First() is IBlockOperation blockOperation) + 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 = blockOperation.ChildOperations + 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(blockOperation, authorizationOptionsTypes); + && 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;