diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs b/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs index 356276852d..a355477396 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs @@ -12,6 +12,7 @@ using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.Authorization { @@ -22,6 +23,9 @@ namespace Microsoft.AspNetCore.Mvc.Authorization /// public class AuthorizeFilter : IAsyncAuthorizationFilter, IFilterFactory { + private MvcOptions _mvcOptions; + private AuthorizationPolicy _effectivePolicy; + /// /// Initializes a new instance. /// @@ -104,15 +108,46 @@ namespace Microsoft.AspNetCore.Mvc.Authorization bool IFilterFactory.IsReusable => true; - /// - public virtual async Task OnAuthorizationAsync(AuthorizationFilterContext context) + private async Task GetEffectivePolicyAsync(AuthorizationFilterContext context) { - if (context == null) + if (_effectivePolicy != null) { - throw new ArgumentNullException(nameof(context)); + return _effectivePolicy; } var effectivePolicy = Policy; + + if (_mvcOptions == null) + { + _mvcOptions = context.HttpContext.RequestServices.GetRequiredService>().Value; + } + + if (_mvcOptions.CombineAuthorizeFilters) + { + if (!context.IsEffectivePolicy(this)) + { + return null; + } + + // Combine all authorize filters into single effective policy that's only run on the closest filter + AuthorizationPolicyBuilder builder = null; + for (var i = 0; i < context.Filters.Count; i++) + { + if (ReferenceEquals(this, context.Filters[i])) + { + continue; + } + + if (context.Filters[i] is AuthorizeFilter authorizeFilter) + { + builder = builder ?? new AuthorizationPolicyBuilder(effectivePolicy); + builder.Combine(authorizeFilter.Policy); + } + } + + effectivePolicy = builder?.Build() ?? effectivePolicy; + } + if (effectivePolicy == null) { if (PolicyProvider == null) @@ -126,6 +161,24 @@ namespace Microsoft.AspNetCore.Mvc.Authorization effectivePolicy = await AuthorizationPolicy.CombineAsync(PolicyProvider, AuthorizeData); } + // We can cache the effective policy when there is no custom policy provider + if (PolicyProvider == null) + { + _effectivePolicy = effectivePolicy; + } + + return effectivePolicy; + } + + /// + public virtual async Task OnAuthorizationAsync(AuthorizationFilterContext context) + { + if (context == null) + { + throw new ArgumentNullException(nameof(context)); + } + + var effectivePolicy = await GetEffectivePolicyAsync(context); if (effectivePolicy == null) { return; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs b/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs index 4c164d162f..4e6e0c19df 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs @@ -5,6 +5,7 @@ using System; using System.Collections; using System.Collections.Generic; using Microsoft.AspNetCore.Mvc.ApplicationModels; +using Microsoft.AspNetCore.Mvc.Authorization; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Formatters; using Microsoft.AspNetCore.Mvc.Infrastructure; @@ -242,6 +243,13 @@ namespace Microsoft.AspNetCore.Mvc /// public bool RequireHttpsPermanent { get; set; } + /// + /// Gets or sets a value that determines if policies on instances of + /// will be combined into a single effective policy. This was always to be the intended behavior, + /// but was not the case. + /// + public bool CombineAuthorizeFilters { get; set;} + IEnumerator IEnumerable.GetEnumerator() { return ((IEnumerable)_switches).GetEnumerator(); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs index 7222cb16ab..f989ec77a6 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Authorization/AuthorizeFilterTest.cs @@ -221,6 +221,68 @@ namespace Microsoft.AspNetCore.Mvc.Authorization Assert.Null(authorizationContext.Result); } + [Fact] + public async Task AuthorizationFilterCombinesMultipleFilters() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => true).Build()); + var authorizationContext = GetAuthorizationContext(anonymous: false, registerServices: s => s.Configure(o => o.CombineAuthorizeFilters = true)); + // Effective policy should fail, if both are combined + authorizationContext.Filters.Add(authorizeFilter); + var secondFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => false).Build()); + authorizationContext.Filters.Add(secondFilter); + + // Act + await secondFilter.OnAuthorizationAsync(authorizationContext); + + // Assert + Assert.IsType(authorizationContext.Result); + } + + [Fact] + public async Task AuthorizationFilterIgnoresFirstFilterWhenCombining() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => false).Build()); + var authorizationContext = GetAuthorizationContext(anonymous: false, registerServices: s => s.Configure(o => o.CombineAuthorizeFilters = true)); + // Effective policy should fail, if both are combined + authorizationContext.Filters.Add(authorizeFilter); + var secondFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => false).Build()); + authorizationContext.Filters.Add(secondFilter); + + // Act + await authorizeFilter.OnAuthorizationAsync(authorizationContext); + + // Assert + Assert.Null(authorizationContext.Result); + } + + [Fact] + public async Task AuthorizationFilterCombinesDerivedFilters() + { + // Arrange + var authorizeFilter = new AuthorizeFilter(new AuthorizationPolicyBuilder().RequireAssertion(a => true).Build()); + var authorizationContext = GetAuthorizationContext(anonymous: false, registerServices: s => s.Configure(o => o.CombineAuthorizeFilters = true)); + // Effective policy should fail, if both are combined + authorizationContext.Filters.Add(authorizeFilter); + authorizationContext.Filters.Add(new DerivedAuthorizeFilter()); + authorizationContext.Filters.Add(new DerivedAuthorizeFilter()); + var lastFilter = new DerivedAuthorizeFilter(); + authorizationContext.Filters.Add(lastFilter); + + // Act + await lastFilter.OnAuthorizationAsync(authorizationContext); + + // Assert + Assert.IsType(authorizationContext.Result); + } + + public class DerivedAuthorizeFilter : AuthorizeFilter + { + public DerivedAuthorizeFilter() : base(new AuthorizationPolicyBuilder().RequireAssertion(a => false).Build()) + { } + } + [Fact] public async Task AuthZResourceShouldBeAuthorizationFilterContext() { diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CombineAuthorizeTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CombineAuthorizeTests.cs new file mode 100644 index 0000000000..854306e4f9 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CombineAuthorizeTests.cs @@ -0,0 +1,43 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using SecurityWebSite; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.FunctionalTests +{ + public class CombineAuthorizeTests : IClassFixture> + { + public CombineAuthorizeTests(MvcTestFixture fixture) + { + Client = fixture.Client; + } + + public HttpClient Client { get; } + + [Fact] + public async Task CanAuthorizeWithOnlyCookie2() + { + // Arrange & Act + var response = await Client.PostAsync("http://localhost/Administration/SignInCookie2", null); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.True(response.Headers.Contains("Set-Cookie")); + + var cookie2 = response.Headers.GetValues("Set-Cookie").SingleOrDefault(); + + var request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/Administration/EitherCookie"); + request.Headers.Add("Cookie", cookie2); + + response = await Client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal("Administration.EitherCookie:AuthorizeCount=1", body); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/GlobalAuthorizationFilterTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/GlobalAuthorizationFilterTest.cs index 9eecde77ed..94ea6c7033 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/GlobalAuthorizationFilterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/GlobalAuthorizationFilterTest.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Linq; using System.Net; using System.Net.Http; using System.Threading.Tasks; @@ -42,5 +43,30 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests var body = await response.Content.ReadAsStringAsync(); Assert.Equal("Administration.AllowAnonymousAction", body); } + + [Fact] + public async Task AuthorizationPoliciesDoNotCombineByDefault() + { + // Arrange & Act + var response = await Client.PostAsync("http://localhost/Administration/SignInCookie2", null); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.True(response.Headers.Contains("Set-Cookie")); + + var cookie2 = response.Headers.GetValues("Set-Cookie").SingleOrDefault(); + + var request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/Administration/EitherCookie"); + request.Headers.Add("Cookie", cookie2); + + // Will fail because default cookie is not sent so [Authorize] fails + response = await Client.SendAsync(request); + Assert.Equal(HttpStatusCode.Redirect, response.StatusCode); + Assert.NotNull(response.Headers.Location); + Assert.Equal( + "http://localhost/Home/Login?ReturnUrl=%2FAdministration%2FEitherCookie", + response.Headers.Location.ToString()); + } + } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs index f0b585a6b9..31dcf75a53 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/AuthorizeFilterIntegrationTest.cs @@ -17,6 +17,7 @@ using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Microsoft.Extensions.ObjectPool; using Xunit; namespace Microsoft.AspNetCore.Mvc.IntegrationTests @@ -70,8 +71,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests serviceCollection.AddMvc(); serviceCollection .AddTransient() - .AddTransient, Logger>(); - + .AddTransient, Logger>() + .AddSingleton(); return serviceCollection.BuildServiceProvider(); } diff --git a/test/WebSites/SecurityWebSite/Controllers/AdministrationController.cs b/test/WebSites/SecurityWebSite/Controllers/AdministrationController.cs index ac7be06b5e..89ea310500 100644 --- a/test/WebSites/SecurityWebSite/Controllers/AdministrationController.cs +++ b/test/WebSites/SecurityWebSite/Controllers/AdministrationController.cs @@ -1,8 +1,13 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Security.Claims; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Authorization.Policy; using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.DependencyInjection; namespace SecurityWebSite.Controllers { @@ -15,10 +20,27 @@ namespace SecurityWebSite.Controllers return Content("Administration.Index"); } + // Either cookie should allow access to this action (if CombineAuthorizeFilters is true) + // If CombineAuthorizeFilters is false, the main cookie is required. + [Authorize(AuthenticationSchemes = "Cookie2")] + public IActionResult EitherCookie() + { + var countEvaluator = (CountingPolicyEvaluator)HttpContext.RequestServices.GetRequiredService(); + return Content("Administration.EitherCookie:AuthorizeCount="+countEvaluator.AuthorizeCount); + } + [AllowAnonymous] public IActionResult AllowAnonymousAction() { return Content("Administration.AllowAnonymousAction"); } + + [AllowAnonymous] + public async Task SignInCookie2() + { + await HttpContext.SignInAsync("Cookie2", new ClaimsPrincipal(new ClaimsIdentity("Cookie2"))); + return Content("SignInCookie2"); + } + } } diff --git a/test/WebSites/SecurityWebSite/CountingPolicyEvaluator.cs b/test/WebSites/SecurityWebSite/CountingPolicyEvaluator.cs new file mode 100644 index 0000000000..0b4aaadea2 --- /dev/null +++ b/test/WebSites/SecurityWebSite/CountingPolicyEvaluator.cs @@ -0,0 +1,24 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Authorization.Policy; +using Microsoft.AspNetCore.Http; + +namespace SecurityWebSite +{ + public class CountingPolicyEvaluator : PolicyEvaluator + { + public int AuthorizeCount { get; private set; } + + public CountingPolicyEvaluator(IAuthorizationService authorization) : base(authorization) { } + + public override Task AuthorizeAsync(AuthorizationPolicy policy, AuthenticateResult authenticationResult, HttpContext context, object resource) { + AuthorizeCount++; + return base.AuthorizeAsync(policy, authenticationResult, context, resource); + } + } +} diff --git a/test/WebSites/SecurityWebSite/SecurityWebSite.csproj b/test/WebSites/SecurityWebSite/SecurityWebSite.csproj index bfe6424834..5486e54caf 100644 --- a/test/WebSites/SecurityWebSite/SecurityWebSite.csproj +++ b/test/WebSites/SecurityWebSite/SecurityWebSite.csproj @@ -7,6 +7,7 @@ + diff --git a/test/WebSites/SecurityWebSite/Startup.cs b/test/WebSites/SecurityWebSite/Startup.cs index 9f427a1cf9..7dc96a5c2a 100644 --- a/test/WebSites/SecurityWebSite/Startup.cs +++ b/test/WebSites/SecurityWebSite/Startup.cs @@ -1,8 +1,9 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Authorization.Policy; +using Microsoft.AspNetCore.Builder; using Microsoft.Extensions.DependencyInjection; namespace SecurityWebSite @@ -19,7 +20,9 @@ namespace SecurityWebSite { options.LoginPath = "/Home/Login"; options.LogoutPath = "/Home/Logout"; - }); + }).AddCookie("Cookie2"); + + services.AddScoped(); } // This method gets called by the runtime. Use this method to configure the HTTP request pipeline. diff --git a/test/WebSites/SecurityWebSite/StartupWithGlobalAuthorizeAndCombineAuthorizeFilters.cs b/test/WebSites/SecurityWebSite/StartupWithGlobalAuthorizeAndCombineAuthorizeFilters.cs new file mode 100644 index 0000000000..434260777a --- /dev/null +++ b/test/WebSites/SecurityWebSite/StartupWithGlobalAuthorizeAndCombineAuthorizeFilters.cs @@ -0,0 +1,47 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Authorization.Policy; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Mvc.Authorization; +using Microsoft.Extensions.DependencyInjection; + +namespace SecurityWebSite +{ + public class StartupWithGlobalAuthorizeAndCombineAuthorizeFilters + { + // This method gets called by the runtime. Use this method to add services to the container. + public void ConfigureServices(IServiceCollection services) + { + // Add framework services. + services.AddMvc(o => + { + o.CombineAuthorizeFilters = true; + o.Filters.Add(new AuthorizeFilter()); + }); + + services.AddAntiforgery(); + services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme).AddCookie(options => + { + options.LoginPath = "/Home/Login"; + options.LogoutPath = "/Home/Logout"; + }).AddCookie("Cookie2"); + + services.AddScoped(); + } + + // This method gets called by the runtime. Use this method to configure the HTTP request pipeline. + public void Configure(IApplicationBuilder app) + { + app.UseAuthentication(); + + app.UseMvc(routes => + { + routes.MapRoute( + name: "default", + template: "{controller=Home}/{action=Index}/{id?}"); + }); + } + } +} diff --git a/test/WebSites/SecurityWebSite/StartupWithGlobalDenyAnonymousFilter.cs b/test/WebSites/SecurityWebSite/StartupWithGlobalDenyAnonymousFilter.cs index af49233670..4fa7b0e884 100644 --- a/test/WebSites/SecurityWebSite/StartupWithGlobalDenyAnonymousFilter.cs +++ b/test/WebSites/SecurityWebSite/StartupWithGlobalDenyAnonymousFilter.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Authorization.Policy; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Mvc.Authorization; using Microsoft.Extensions.DependencyInjection; @@ -18,12 +19,14 @@ namespace SecurityWebSite { options.LoginPath = "/Home/Login"; options.LogoutPath = "/Home/Logout"; - }); + }).AddCookie("Cookie2"); services.AddMvc(o => { o.Filters.Add(new AuthorizeFilter()); }); + + services.AddScoped(); } public void Configure(IApplicationBuilder app)