Skip to content

Remove RequiredPolicy #9399

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 6 commits into from
Apr 18, 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
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ 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 bool InvokeHandlersAfterFailure { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
public Microsoft.AspNetCore.Authorization.AuthorizationPolicy RequiredPolicy { [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) { }
public Microsoft.AspNetCore.Authorization.AuthorizationPolicy GetPolicy(string name) { throw null; }
Expand Down Expand Up @@ -127,7 +126,6 @@ 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 virtual System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetPolicyAsync(string policyName) { throw null; }
public System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetRequiredPolicyAsync() { throw null; }
}
public partial class DefaultAuthorizationService : Microsoft.AspNetCore.Authorization.IAuthorizationService
{
Expand Down Expand Up @@ -157,7 +155,6 @@ public partial interface IAuthorizationPolicyProvider
{
System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetDefaultPolicyAsync();
System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetPolicyAsync(string policyName);
System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy> GetRequiredPolicyAsync();
}
public partial interface IAuthorizationRequirement
{
Expand Down
12 changes: 0 additions & 12 deletions src/Security/Authorization/Core/src/AuthorizationOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,6 @@ public class AuthorizationOptions
/// </remarks>
public AuthorizationPolicy DefaultPolicy { get; set; } = new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build();

/// <summary>
/// Gets or sets the required authorization policy. Defaults to null.
/// </summary>
/// <remarks>
/// By default the required policy is null.
///
/// If a required policy has been specified then it is always evaluated, even if there are no
/// <see cref="IAuthorizeData"/> instances for a resource. If a resource has <see cref="IAuthorizeData"/>
/// then they are evaluated together with the required policy.
/// </remarks>
public AuthorizationPolicy RequiredPolicy { get; set; }

/// <summary>
/// Add an authorization policy with the provided name.
/// </summary>
Expand Down
11 changes: 0 additions & 11 deletions src/Security/Authorization/Core/src/AuthorizationPolicy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -176,17 +176,6 @@ public static async Task<AuthorizationPolicy> CombineAsync(IAuthorizationPolicyP
}
}

var requiredPolicy = await policyProvider.GetRequiredPolicyAsync();
if (requiredPolicy != null)
{
if (policyBuilder == null)
{
policyBuilder = new AuthorizationPolicyBuilder();
}

policyBuilder.Combine(requiredPolicy);
}

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

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

/// <summary>
/// Gets the required authorization policy.
/// </summary>
/// <returns>The required authorization policy.</returns>
public Task<AuthorizationPolicy> GetRequiredPolicyAsync()
{
return GetCachedPolicy(ref _cachedRequiredPolicy, _options.RequiredPolicy);
}

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,11 +22,5 @@ public interface IAuthorizationPolicyProvider
/// </summary>
/// <returns>The default authorization policy.</returns>
Task<AuthorizationPolicy> GetDefaultPolicyAsync();

/// <summary>
/// Gets the required authorization policy.
/// </summary>
/// <returns>The required authorization policy.</returns>
Task<AuthorizationPolicy> GetRequiredPolicyAsync();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ public async Task Invoke(HttpContext context)

// IMPORTANT: Changes to authorization logic should be mirrored in MVC's AuthorizeFilter
var authorizeData = endpoint?.Metadata.GetOrderedMetadata<IAuthorizeData>() ?? Array.Empty<IAuthorizeData>();
Copy link
Contributor

Choose a reason for hiding this comment

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

@rynowak \ @JamesNK can we have GetOrderedMetadata return an IReadOnlyList<T>? It internally caches an T[] so there isn't any code change involved with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Eilon, @rynowak pointed out that this would be a breaking change since the API exists in 2.2. However, this seems like a worthwhile change to make - other APIs and users are likely to benefit from this change. How do you feel about this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could just do Count() here. It'll effectively be the same: https://github.com/dotnet/corefx/blob/master/src/System.Linq/src/System/Linq/Count.cs#L12-L22

if (authorizeData.Count() == 0)
{
await _next(context);
return;
}

var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData);
if (policy == null)
{
Expand Down
45 changes: 0 additions & 45 deletions src/Security/Authorization/test/AuthorizationMiddlewareTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,6 @@ public async Task NoEndpoint_AnonymousUser_Allows()
Assert.True(next.Called);
}

[Fact]
public async Task NoEndpointWithRequired_AnonymousUser_Challenges()
{
// Arrange
var policy = new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build();
var policyProvider = new Mock<IAuthorizationPolicyProvider>();
policyProvider.Setup(p => p.GetRequiredPolicyAsync()).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 @@ -79,26 +60,6 @@ public async Task HasEndpointWithoutAuth_AnonymousUser_Allows()
Assert.True(next.Called);
}

[Fact]
public async Task HasEndpointWithRequiredWithoutAuth_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.GetRequiredPolicyAsync()).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 HasEndpointWithAuth_AnonymousUser_Challenges()
{
Expand Down Expand Up @@ -148,29 +109,23 @@ public async Task OnAuthorizationAsync_WillCallPolicyProvider()
var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build();
var policyProvider = new Mock<IAuthorizationPolicyProvider>();
var getPolicyCount = 0;
var getRequiredPolicyCount = 0;
policyProvider.Setup(p => p.GetPolicyAsync(It.IsAny<string>())).ReturnsAsync(policy)
.Callback(() => getPolicyCount++);
policyProvider.Setup(p => p.GetRequiredPolicyAsync()).ReturnsAsync(policy)
.Callback(() => getRequiredPolicyCount++);
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(1, getRequiredPolicyCount);
Assert.Equal(1, next.CalledCount);

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

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ public MinimumAgePolicyProvider(IOptions<AuthorizationOptions> options)

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

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

// 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
// by the more strongly-typed attributes derived from AuthorizeAttribute
Expand All @@ -49,4 +47,4 @@ public Task<AuthorizationPolicy> GetPolicyAsync(string policyName)
return FallbackPolicyProvider.GetPolicyAsync(policyName);
}
}
}
}