Skip to content
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

Proposal for how we handle more metadata for auth #39892

Closed
wants to merge 2 commits into from
Closed
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
47 changes: 45 additions & 2 deletions src/Security/Authorization/Core/src/AuthorizationPolicy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,27 @@ public static AuthorizationPolicy Combine(IEnumerable<AuthorizationPolicy> polic
/// A new <see cref="AuthorizationPolicy"/> which represents the combination of the
/// authorization policies provided by the specified <paramref name="policyProvider"/>.
/// </returns>
public static async Task<AuthorizationPolicy?> CombineAsync(IAuthorizationPolicyProvider policyProvider, IEnumerable<IAuthorizeData> authorizeData)
public static Task<AuthorizationPolicy?> CombineAsync(IAuthorizationPolicyProvider policyProvider,
IEnumerable<IAuthorizeData> authorizeData) => CombineAsync(policyProvider, authorizeData,
Enumerable.Empty<AuthorizationPolicy>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's better to use Array.Empty here since it would benefit from a slightly faster Enumerable.Any()

Copy link
Member Author

Choose a reason for hiding this comment

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

Only slightly in since Enumerable.Empty<T> is also optimized for the Count check.

Enumerable.Empty<IAuthorizationRequirement>());

/// <summary>
/// Combines the <see cref="AuthorizationPolicy"/> provided by the specified
/// <paramref name="policyProvider"/>.
/// </summary>
/// <param name="policyProvider">A <see cref="IAuthorizationPolicyProvider"/> which provides the policies to combine.</param>
/// <param name="authorizeData">A collection of authorization data used to apply authorization to a resource.</param>
/// <param name="policies">A collection of <see cref="AuthorizationPolicy"/> policies to combine.</param>
/// <param name="requirements">A collection of <see cref="IAuthorizationRequirement"/>s to add to the auth policy.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// <param name="requirements">A collection of <see cref="IAuthorizationRequirement"/>s to add to the auth policy.</param>
/// <param name="requirements">A collection of <see cref="IAuthorizationRequirement"/> instances to add to the policy.</param>

/// <returns>
/// A new <see cref="AuthorizationPolicy"/> which represents the combination of the
/// authorization policies provided by the specified <paramref name="policyProvider"/>.
/// </returns>
public static async Task<AuthorizationPolicy?> CombineAsync(IAuthorizationPolicyProvider policyProvider,
Copy link
Member

Choose a reason for hiding this comment

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

Do we care at all potential duplicate requirements in here? I guess currently we don't try to dedupe anything when we combine

This is also complicated enough that I wonder if this is worth exposing preemptively as some kind of IAuthorizationPolicyCombiner now?

Copy link
Member Author

Choose a reason for hiding this comment

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

IAuthorizationPolicyCombiner - No more abstractions 😄. I think the policy combining logic is actually pretty simple and I think we should de-dupe (actually if you have N authorize attributes you end up with the default policy added N times, but the system dedupes elsewhere).

IEnumerable<IAuthorizeData> authorizeData,
IEnumerable<AuthorizationPolicy> policies,
IEnumerable<IAuthorizationRequirement> requirements)
{
if (policyProvider == null)
{
Expand All @@ -120,6 +140,9 @@ public static AuthorizationPolicy Combine(IEnumerable<AuthorizationPolicy> polic
throw new ArgumentNullException(nameof(authorizeData));
}

var anyPolicies = policies.Any();
var anyRequirements = requirements.Any();

// Avoid allocating enumerator if the data is known to be empty
var skipEnumeratingData = false;
if (authorizeData is IList<IAuthorizeData> dataList)
Expand All @@ -137,7 +160,7 @@ public static AuthorizationPolicy Combine(IEnumerable<AuthorizationPolicy> polic
policyBuilder = new AuthorizationPolicyBuilder();
}

var useDefaultPolicy = true;
var useDefaultPolicy = !(anyPolicies || anyRequirements);
if (!string.IsNullOrWhiteSpace(authorizeDatum.Policy))
{
var policy = await policyProvider.GetPolicyAsync(authorizeDatum.Policy);
Expand Down Expand Up @@ -176,6 +199,26 @@ public static AuthorizationPolicy Combine(IEnumerable<AuthorizationPolicy> polic
}
}

if (anyPolicies)
{
policyBuilder ??= new();

foreach (var policy in policies)
{
policyBuilder.Combine(policy);
}
}

if (anyRequirements)
{
policyBuilder ??= new();

foreach (var requirement in requirements)
{
policyBuilder.Requirements.Add(requirement);
}
}

// If we have no policy by now, use the fallback policy if we have one
if (policyBuilder == null)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
#nullable enable
static Microsoft.AspNetCore.Authorization.AuthorizationPolicy.CombineAsync(Microsoft.AspNetCore.Authorization.IAuthorizationPolicyProvider! policyProvider, System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Authorization.IAuthorizeData!>! authorizeData, System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Authorization.AuthorizationPolicy!>! policies, System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Authorization.IAuthorizationRequirement!>! requirements) -> System.Threading.Tasks.Task<Microsoft.AspNetCore.Authorization.AuthorizationPolicy?>!
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,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>();
var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData);

var policies = endpoint?.Metadata.GetOrderedMetadata<AuthorizationPolicy>() ?? Array.Empty<AuthorizationPolicy>();
var reqirements = endpoint?.Metadata.GetOrderedMetadata<IAuthorizationRequirement>() ?? Array.Empty<IAuthorizationRequirement>();

var policy = await AuthorizationPolicy.CombineAsync(_policyProvider, authorizeData, policies, reqirements);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we cache the policy it on Endpoint as an internal property (assuming _policyProvider is a default one)? This seems like to quite a bit to have to do on a per-request basis.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR but if you figure out a way to store objects per endpoint, this PR (#21675) could probably be revived too.


if (policy == null)
{
await _next(context);
Expand Down
58 changes: 58 additions & 0 deletions src/Security/Authorization/test/AuthorizationMiddlewareTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,64 @@ public async Task OnAuthorizationAsync_WillCallPolicyProvider()
Assert.Equal(3, next.CalledCount);
}

[Fact]
public async Task CanApplyPolicyDirectlyToEndpoint()
{
// Arrange
var calledPolicy = false;
var policy = new AuthorizationPolicyBuilder().RequireAssertion(_ =>
{
calledPolicy = true;
return true;
}).Build();

var policyProvider = new Mock<IAuthorizationPolicyProvider>();
policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build());
var next = new TestRequestDelegate();
var middleware = CreateMiddleware(next.Invoke, policyProvider.Object);
var context = GetHttpContext(anonymous: false, endpoint: CreateEndpoint(new AuthorizeAttribute(), policy));

// Act & Assert
await middleware.Invoke(context);
Assert.True(calledPolicy);
}

[Fact]
public async Task CanApplyAdditonalRequirementsToEndpoint()
{
// Arrange
var policyProvider = new Mock<IAuthorizationPolicyProvider>();
policyProvider.Setup(p => p.GetDefaultPolicyAsync()).ReturnsAsync(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build());
var next = new TestRequestDelegate();
var middleware = CreateMiddleware(next.Invoke, policyProvider.Object);
var context = GetHttpContext(anonymous: false, registerServices: services =>
{
services.AddSingleton<IAuthorizationHandler, CustomAuthHandler>();
},
endpoint: CreateEndpoint(new AuthorizeAttribute(), new CustomRequirement("This")));
Copy link
Member

Choose a reason for hiding this comment

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

So what happens here when you combine what's used to be a default policy situation of an empty authorize attribute and a custom requirement, it kind of looks like we wouldn't use the default policy, and would just use the custom requirement, which I think might be not what we want, since we'd lose the requirement of needing an authenticated user right?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I wanted to discuss. It seems like to don't use the default policy if one is already specified so I mimicked that approach here as well. Maybe that only applies when you have a custom policy but additional requirements don't affect that?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think everyone just assumes any kind of authorize brings along that RequireAuthenticatedUser that gets combined for free today. As long as we don't make them have to explicitly add it in any of these new usage scenarios it should be fine.


// Act & Assert
await middleware.Invoke(context);
}

class CustomAuthHandler : AuthorizationHandler<CustomRequirement>
{
protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, CustomRequirement requirement)
{
return Task.CompletedTask;
}
}

class CustomRequirement : IAuthorizationRequirement
{
public string Data { get; init; }

public CustomRequirement(string data)
{
Data = data;
}
}

[Fact]
public async Task Invoke_ValidClaimShouldNotFail()
{
Expand Down
23 changes: 23 additions & 0 deletions src/Security/Authorization/test/AuthorizationPolicyFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,29 @@ public async Task CanCombineAuthorizeAttributes()
Assert.Single(combined.Requirements.OfType<RolesAuthorizationRequirement>());
}

[Fact]
public async Task CanReplaceDefaultPolicyDirectly()
{
// Arrange
var attributes = new AuthorizeAttribute[] {
new AuthorizeAttribute(),
new AuthorizeAttribute(),
};

var policies = new[] { new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build() };

var options = new AuthorizationOptions();

var provider = new DefaultAuthorizationPolicyProvider(Options.Create(options));

// Act
var combined = await AuthorizationPolicy.CombineAsync(provider, attributes, policies, Enumerable.Empty<IAuthorizationRequirement>());

// Assert
Assert.Equal(1, combined.Requirements.Count);
Assert.Empty(combined.Requirements.OfType<DenyAnonymousAuthorizationRequirement>());
}

[Fact]
public async Task CanReplaceDefaultPolicy()
{
Expand Down