Skip to content

RequiredPolicy reborn and less demanding as FallbackPolicy #9759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
May 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/test/Authorization/AuthorizeFilterTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ public Task<AuthorizationPolicy> GetPolicyAsync(string policyName)
return Task.FromResult(policyName == "true" ? _true : _false);
}

public Task<AuthorizationPolicy> GetRequiredPolicyAsync()
public Task<AuthorizationPolicy> GetFallbackPolicyAsync()
=> Task.FromResult<AuthorizationPolicy>(null);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ public Task<AuthorizationPolicy> GetPolicyAsync(string policyName)
return Task.FromResult(new AuthorizationPolicy(requirements, new string[] { }));
}

public Task<AuthorizationPolicy> GetRequiredPolicyAsync()
public Task<AuthorizationPolicy> GetFallbackPolicyAsync()
{
return Task.FromResult<AuthorizationPolicy>(null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@ protected AuthorizationHandler() { }
public partial class AuthorizationMiddleware
{
public AuthorizationMiddleware(Microsoft.AspNetCore.Http.RequestDelegate next, Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider policyProvider) { }
[System.Diagnostics.DebuggerStepThroughAttribute]
public System.Threading.Tasks.Task Invoke(Microsoft.AspNetCore.Http.HttpContext context) { throw null; }
}
public partial class AuthorizationOptions
{
public AuthorizationOptions() { }
public Microsoft.AspNetCore.Authorization.AuthorizationPolicy DefaultPolicy { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public Microsoft.AspNetCore.Authorization.AuthorizationPolicy FallbackPolicy { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public bool InvokeHandlersAfterFailure { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public void AddPolicy(string name, Microsoft.AspNetCore.Authorization.AuthorizationPolicy policy) { }
public void AddPolicy(string name, System.Action<Microsoft.AspNetCore.Authorization.AuthorizationPolicyBuilder> configurePolicy) { }
Expand Down Expand Up @@ -130,6 +132,7 @@ public partial class DefaultAuthorizationPolicyProvider : Microsoft.AspNetCore.A
{
public DefaultAuthorizationPolicyProvider(Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Authorization.AuthorizationOptions> options) { }
public System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetDefaultPolicyAsync() { throw null; }
public System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetFallbackPolicyAsync() { throw null; }
public virtual System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetPolicyAsync(string policyName) { throw null; }
}
public partial class DefaultAuthorizationService : Microsoft.AspNetCore.Authorization.IAuthorizationService
Expand Down Expand Up @@ -159,6 +162,7 @@ public partial interface IAuthorizationHandlerProvider
public partial interface IAuthorizationPolicyProvider
{
System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetDefaultPolicyAsync();
System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetFallbackPolicyAsync();
System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetPolicyAsync(string policyName);
}
public partial interface IAuthorizationRequirement
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authentication;
Expand All @@ -23,21 +22,11 @@ public class AuthorizationMiddleware

public AuthorizationMiddleware(RequestDelegate next, IAuthorizationPolicyProvider policyProvider)
{
if (next == null)
{
throw new ArgumentNullException(nameof(next));
}

if (policyProvider == null)
{
throw new ArgumentNullException(nameof(policyProvider));
}

_next = next;
_policyProvider = policyProvider;
_next = next ?? throw new ArgumentNullException(nameof(next));
_policyProvider = policyProvider ?? throw new ArgumentNullException(nameof(policyProvider));
}

public Task Invoke(HttpContext context)
public async Task Invoke(HttpContext context)
{
if (context == null)
{
Expand All @@ -49,18 +38,8 @@ public Task Invoke(HttpContext context)
// Flag to indicate to other systems, e.g. MVC, that authorization middleware was run for this request
context.Items[AuthorizationMiddlewareInvokedKey] = AuthorizationMiddlewareInvokedValue;

var authorizeData = endpoint?.Metadata.GetOrderedMetadata<IAuthorizeData>();
if (authorizeData == null || authorizeData.Count() == 0)
{
return _next(context);
}

return EvaluatePolicy(context, endpoint, authorizeData);
}

private async Task EvaluatePolicy(HttpContext context, Endpoint endpoint, IEnumerable<IAuthorizeData> authorizeData)
{
// IMPORTANT: Changes to authorization logic should be mirrored in MVC's AuthorizeFilter
var authorizeData = endpoint?.Metadata.GetOrderedMetadata<IAuthorizeData>() ?? Array.Empty<IAuthorizeData>();
var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData);
if (policy == null)
{
Expand Down
12 changes: 11 additions & 1 deletion src/Security/Authorization/Core/src/AuthorizationOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ public class AuthorizationOptions
/// </remarks>
public AuthorizationPolicy DefaultPolicy { get; set; } = new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build();

/// <summary>
/// Gets or sets the fallback authorization policy used by <see cref="AuthorizationPolicy.CombineAsync(IAuthorizationPolicyProvider, IEnumerable{IAuthorizeData})"/>
/// when no IAuthorizeData have been provided. As a result, the <see cref="AuthorizationMiddleware"/> uses the fallback policy
/// if there are no <see cref="IAuthorizeData"/> instances for a resource. If a resource has any <see cref="IAuthorizeData"/>
/// then they are evaluated instead of the fallback policy. By default the fallback policy is null, and usually will have no
/// effect unless you have the <see cref="AuthorizationMiddleware"/> middleware in your pipeline. It is not used in any way by the
/// default <see cref="IAuthorizationService"/>.
/// </summary>
public AuthorizationPolicy FallbackPolicy { get; set; }

/// <summary>
/// Add an authorization policy with the provided name.
/// </summary>
Expand Down Expand Up @@ -84,4 +94,4 @@ public AuthorizationPolicy GetPolicy(string name)
return PolicyMap.ContainsKey(name) ? PolicyMap[name] : null;
}
}
}
}
12 changes: 11 additions & 1 deletion src/Security/Authorization/Core/src/AuthorizationPolicy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,17 @@ public static async Task<AuthorizationPolicy> CombineAsync(IAuthorizationPolicyP
}
}

// If we have no policy by now, use the fallback policy if we have one
if (policyBuilder == null)
{
var fallbackPolicy = await policyProvider.GetFallbackPolicyAsync();
if (fallbackPolicy != null)
{
return fallbackPolicy;
}
}

return policyBuilder?.Build();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public class DefaultAuthorizationPolicyProvider : IAuthorizationPolicyProvider
{
private readonly AuthorizationOptions _options;
private Task<AuthorizationPolicy> _cachedDefaultPolicy;
private Task<AuthorizationPolicy> _cachedFallbackPolicy;

/// <summary>
/// Creates a new instance of <see cref="DefaultAuthorizationPolicyProvider"/>.
Expand All @@ -39,6 +40,15 @@ public Task<AuthorizationPolicy> GetDefaultPolicyAsync()
return GetCachedPolicy(ref _cachedDefaultPolicy, _options.DefaultPolicy);
}

/// <summary>
/// Gets the fallback authorization policy.
/// </summary>
/// <returns>The fallback authorization policy.</returns>
public Task<AuthorizationPolicy> GetFallbackPolicyAsync()
Copy link
Member

Choose a reason for hiding this comment

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

I kinda want to make new methods ValueTask

{
return GetCachedPolicy(ref _cachedFallbackPolicy, _options.FallbackPolicy);
}

private Task<AuthorizationPolicy> GetCachedPolicy(ref Task<AuthorizationPolicy> cachedPolicy, AuthorizationPolicy currentPolicy)
{
var local = cachedPolicy;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,11 @@ public interface IAuthorizationPolicyProvider
/// </summary>
/// <returns>The default authorization policy.</returns>
Task<AuthorizationPolicy> GetDefaultPolicyAsync();

/// <summary>
/// Gets the fallback authorization policy.
/// </summary>
/// <returns>The fallback authorization policy.</returns>
Task<AuthorizationPolicy> GetFallbackPolicyAsync();
}
}
66 changes: 66 additions & 0 deletions src/Security/Authorization/test/AuthorizationMiddlewareTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,25 @@ public async Task NoEndpoint_AnonymousUser_Allows()
Assert.True(next.Called);
}

[Fact]
public async Task NoEndpointWithFallback_AnonymousUser_Challenges()
{
// Arrange
var policy = new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build();
var policyProvider = new Mock<IAuthorizationPolicyProvider>();
policyProvider.Setup(p => p.GetFallbackPolicyAsync()).ReturnsAsync(policy);
var next = new TestRequestDelegate();

var middleware = CreateMiddleware(next.Invoke, policyProvider.Object);
var context = GetHttpContext(anonymous: true);

// Act
await middleware.Invoke(context);

// Assert
Assert.False(next.Called);
}

[Fact]
public async Task HasEndpointWithoutAuth_AnonymousUser_Allows()
{
Expand All @@ -59,6 +78,47 @@ public async Task HasEndpointWithoutAuth_AnonymousUser_Allows()
Assert.True(next.Called);
}

[Fact]
public async Task HasEndpointWithFallbackWithoutAuth_AnonymousUser_Challenges()
{
// Arrange
var policy = new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build();
var policyProvider = new Mock<IAuthorizationPolicyProvider>();
policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(policy);
policyProvider.Setup(p => p.GetFallbackPolicyAsync()).ReturnsAsync(policy);
var next = new TestRequestDelegate();

var middleware = CreateMiddleware(next.Invoke, policyProvider.Object);
var context = GetHttpContext(anonymous: true, endpoint: CreateEndpoint());

// Act
await middleware.Invoke(context);

// Assert
Assert.False(next.Called);
}

[Fact]
public async Task HasEndpointWithOnlyFallbackAuth_AnonymousUser_Allows()
{
// Arrange
var policy = new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build();
var policyProvider = new Mock<IAuthorizationPolicyProvider>();
policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build());
policyProvider.Setup(p => p.GetFallbackPolicyAsync()).ReturnsAsync(policy);
var next = new TestRequestDelegate();
var authenticationService = new TestAuthenticationService();

var middleware = CreateMiddleware(next.Invoke, policyProvider.Object);
var context = GetHttpContext(anonymous: true, endpoint: CreateEndpoint(new AuthorizeAttribute()), authenticationService: authenticationService);

// Act
await middleware.Invoke(context);

// Assert
Assert.True(next.Called);
}

[Fact]
public async Task HasEndpointWithAuth_AnonymousUser_Challenges()
{
Expand Down Expand Up @@ -108,23 +168,29 @@ public async Task OnAuthorizationAsync_WillCallPolicyProvider()
var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build();
var policyProvider = new Mock<IAuthorizationPolicyProvider>();
var getPolicyCount = 0;
var getFallbackPolicyCount = 0;
policyProvider.Setup(p => p.GetPolicyAsync(It.IsAny<string>())).ReturnsAsync(policy)
.Callback(() => getPolicyCount++);
policyProvider.Setup(p => p.GetFallbackPolicyAsync()).ReturnsAsync(policy)
.Callback(() => getFallbackPolicyCount++);
var next = new TestRequestDelegate();
var middleware = CreateMiddleware(next.Invoke, policyProvider.Object);
var context = GetHttpContext(anonymous: true, endpoint: CreateEndpoint(new AuthorizeAttribute("whatever")));

// Act & Assert
await middleware.Invoke(context);
Assert.Equal(1, getPolicyCount);
Assert.Equal(0, getFallbackPolicyCount);
Assert.Equal(1, next.CalledCount);

await middleware.Invoke(context);
Assert.Equal(2, getPolicyCount);
Assert.Equal(0, getFallbackPolicyCount);
Assert.Equal(2, next.CalledCount);

await middleware.Invoke(context);
Assert.Equal(3, getPolicyCount);
Assert.Equal(0, getFallbackPolicyCount);
Assert.Equal(3, next.CalledCount);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ public Task<AuthorizationPolicy> GetDefaultPolicyAsync()
return Task.FromResult(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build());
}

public Task<AuthorizationPolicy> GetRequiredPolicyAsync()
public Task<AuthorizationPolicy> GetFallbackPolicyAsync()
{
return Task.FromResult<AuthorizationPolicy>(null);
}
Expand Down Expand Up @@ -1064,7 +1064,7 @@ public Task<AuthorizationPolicy> GetDefaultPolicyAsync()
return Task.FromResult(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build());
}

public Task<AuthorizationPolicy> GetRequiredPolicyAsync()
public Task<AuthorizationPolicy> GetFallbackPolicyAsync()
{
return Task.FromResult<AuthorizationPolicy>(null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public MinimumAgePolicyProvider(IOptions<AuthorizationOptions> options)
}

public Task<AuthorizationPolicy> GetDefaultPolicyAsync() => FallbackPolicyProvider.GetDefaultPolicyAsync();

public Task<AuthorizationPolicy> GetFallbackPolicyAsync() => FallbackPolicyProvider.GetFallbackPolicyAsync();

// Policies are looked up by string name, so expect 'parameters' (like age)
// to be embedded in the policy names. This is abstracted away from developers
Expand Down