Add AuthorizationPolicyCache

[halter73]

Background and Motivation

From @HaoK: We want to add a new metadata class which if applied to an endpoint, the authorization middleware will store the combined policy in this metadata to avoid recomputing the policy on every request (but only if the default policy provider is being used). The benefit for this being public is so anyone can benefit from this behavior by adding it to an endpoints metadata.

Proposed API

namespace Microsoft.AspNetCore.Authorization;

public interface IAuthorizationPolicyProvider
{
+    public bool CanCachePolicies => false;
}

public class AuthorizationMiddleware
{
+    public AuthorizationMiddleware(RequestDelegate next, IAuthorizationPolicyProvider policyProvider, IServiceProvider services);
}

Usage Examples

public class MyAuthorizationPolicyProvider : IAuthorizationPolicyProvider
{
    public bool CanCachePolicies => true;
}

Alternative Designs

Keep this internal for now and only use it from AuthorizationMiddleware.

Risks

We might want to change this class in the future and will be unable to make breaking changes since the metadata is now public.

---

PR for context: #43124
This is a follow up to the previously-approved RequireAuthorization overloads supporting inline policies: #39840

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API review questions?

1. What other than AuthorizationMiddleware and the new RequireAuthorization conventions is going to use this?
2. Does AuthorizationPolicyCache.Policy need to be both publicly readable and settable? Can just one or the other be made public? Neither?
3. On the issue @Kahbazi mentions that YARP might want to add it to its endpoints.
   1. Does YARP actually need to be able to configure or read the cached AuthorizationPolicy on the endpoint? That seems odd if it's just a cache. @Tratcher
   2. If it's an important scenario to add the policy cache to an endpoint but not to use it directly, can things like YARP just call RequireAuthorization on the endpoint to add it?
   3. Do we need an IEndpointConventionBuider extension method that adds AuthorizationPolicyCache and nothing else.
4. Should we seal this?

[halter73]

Also, do we think this a worthy use of publicly mutable metadata? @JamesNK what do you think?

[JamesNK]

I think this is abusing what the endpoint metadata collection is intended for.

[HaoK]

1. Not sure but basically anyone that wants to add authorization metadata, should also add this for perf
2. This probably can just be a marker class, with internal get/set for the middleware to consume.
3. Since there's no good reason (i think?) to ever turn this off for the default policy provider so we could just seal it I think.

I think this is abusing what the endpoint metadata collection is intended for.

This is purely a brainchild of @davidfowl and @DamianEdwards 😆

[Tratcher]

YARP makes endpoints and adds metadata to them:
https://github.com/microsoft/reverse-proxy/blob/e54839e924391e75d37928f21abe5987d4ab5990/src/ReverseProxy/Routing/ProxyEndpointFactory.cs#L100-L111

[halter73]

What about?

namespace Microsoft.AspNetCore.Authorization;

+ public static class AuthorizationCacheEndpointConventionBuilderExtensions
+ {
+     public static TBuilder WithAuthorizationCache<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
+ }

The "correct" namespace is Microsoft.AspNetCore.Builder, but I want to hide this a little, because I expect most people will get this by calling some overload of RequireAuthorization(), but maybe not if they're using attributes.

[davidfowl]

There's one alternative to making it mutable but its a little crazy (good crazy). We do this optimization after we get #43225. We do this caching using Finally. We'd resolve the service in there and cache the "final" metadata if it's the default implementation (we can resolve the policy then and cache this immutable version of the metadata).

[halter73]

I love the Finally idea, but what about MVC? Or someone using auth attributes with minimal route handers? How does auth attach a Finally callback unless RequireAuthorization or the proposed WithAuthorizationCache method gets called?

[davidfowl]

I love the Finally idea, but what about MVC? Or someone using auth attributes with minimal route handers? How does auth attach a Finally callback unless RequireAuthorization or the proposed WithAuthorizationCache method gets called.

Does it need to? This is purely an optimization that happens in the 99% case:

• RequireAuthorization
• MVC controllers
• Razor pages

Other frameworks will work and can call the WithAuthorizationCache or users can call it in the worst case scenario (like on a group!).

For MVC and pages, we can just always call this internally, I assume the call will be smart enough to find the metadata and do the needful, otherwise it'll noop.

[halter73]

Other frameworks will work and can call the WithAuthorizationCache or users can call it in the worst case scenario (like on a group!).

Exactly! Apparently YARP could use WithAuthorizationCache too. So I think we should add this method. Should we put it in with the other authz extension methods? Or hide it in another namespace since most people won't have a reason to call it?

I'm not sure it's quite the 99% case. I think as more people use route handlers, more people are going to use them with attributes. Remember not all route handlers are lambdas.

I'm kinda leaning towards putting it in AuthorizationEndpointConventionBuilderExtensions along with all the RequireAuthorization extension methods right now.

[davidfowl]

I'm not sure it's quite the 99% case. I think as more people use route handlers, more people are going to use them with attributes. Remember not all route handlers are lambdas.

We can make that work as well (implicitly) 😄

I'm kinda leaning towards putting it in AuthorizationEndpointConventionBuilderExtensions along with all the RequireAuthorization extension methods right now.

Sounds good.

[Kahbazi]

YARP or any other framework would only need to add the AuthorizationPolicyCache metadata in order to gain the perf for their endpoints. They would not need to neither set or read the cached value.

The problem with just a method on IEndpointConventionBuilder is that now those frameworks are just using EndpointBuilder and adding their own EndpointDataSource, without IEndpointConventionBuilder must implement IEndpointConventionBuilder as well.

Also be aware that the endpointBuilder here is not IEndpointConventionBuilder
https://github.com/microsoft/reverse-proxy/blob/e54839e924391e75d37928f21abe5987d4ab5990/src/ReverseProxy/Routing/ProxyEndpointFactory.cs#L100-L111
and YARP only has one IEndpointConventionBuilder (https://github.com/microsoft/reverse-proxy/blob/ea063ad3169445f10ac8381ac80c89e1ea558334/src/ReverseProxy/Management/ProxyConfigManager.cs#L91). With the current implementation the easiest way would be to just add authorization cache to all of its endpoint!! 🤔

Overall isn't this leaking of implementation details!? If I'm creating my endpoints by directly creating my own EndpointDataSource and not using RequireAuthorization method. why do I need to know that there's a policy cache?

Could we just have a non-readonly collection on Endpoint, so every middleware use that if needed. I'm writing a rate limiting middleware and I can really use a place like that to store ChainedPartitionedRateLimiter instances that are per endpoints.

[davidfowl]

Could we just have a non-readonly collection on Endpoint, so every middleware use that if needed. I'm writing a rate limiting middleware and I can really use a place like that to store ChainedPartitionedRateLimiter instances that are per endpoints.

Mutable shared state leads to bugs. We should be shifting as much as we can to startup for situations like this.

[halter73]

The problem with just a method on IEndpointConventionBuilder is that now those frameworks are just using EndpointBuilder and adding their own EndpointDataSource, without IEndpointConventionBuilder must implement IEndpointConventionBuilder as well.

I would argue that frameworks that are just using EndpointBuilder ought to exposing an IEndpointConventionBuilder somewhere.

Also be aware that the endpointBuilder here is not IEndpointConventionBuilder microsoft/reverse-proxy@e54839e/src/ReverseProxy/Routing/ProxyEndpointFactory.cs#L100-L111 and YARP only has one IEndpointConventionBuilder (microsoft/reverse-proxy@ea063ad/src/ReverseProxy/Management/ProxyConfigManager.cs#L91). With the current implementation the easiest way would be to just add authorization cache to all of its endpoint!! 🤔

Overall isn't this leaking of implementation details!? If I'm creating my endpoints by directly creating my own EndpointDataSource and not using RequireAuthorization method. why do I need to know that there's a policy cache?

It is annoying that you need to know about the cache in some situations. Of course, everything will continue to work as it does today without it, but I see your point. I was tempted to say we should add an internal property to the AuthorizeAttribute itself, but that wouldn't work for custom attributes implementing IAuthorizeData.

Could we just have a non-readonly collection on Endpoint, so every middleware use that if needed. I'm writing a rate limiting middleware and I can really use a place like that to store ChainedPartitionedRateLimiter instances that are per endpoints.

It is tempting, but I think I agree with @davidfowl that we should at least hide mutable state on the Endpoint. @HaoK Are you okay with the following proposal?

namespace Microsoft.AspNetCore.Builder;
 
public static class AuthorizationEndpointConventionBuilderExtensions
{
+     public static TBuilder WithAuthorizationCache<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
}

[HaoK]

Looks good to me, I like it

[halter73]

API Approved!

[pinkfloydx33]

Re: Default Policy provider. We have our own policy provider that turns various policy names into corresponding policies. For example we have an attribute that takes a permissions enum and a resource type which generates an IAuthorizeData.Policy value such as "SomePrefix__ResourceName:Read|Write" which we then turn into a policy on the fly. We have other such policies that we compute/deconstruct that way. For everything else we fall back to the default provider

This is very useful in both our MVC application, but especially in our YARP gateway where policy names are pulled from external configuration sources rather than attributes, and are usually unique to each proxied endpoint. Defining the individual policies ahead of time in-app just wouldn't be feasible, hence our custom provider which can turn recognized patterns into policies.

We aren't injecting services and/or doing anything more than turning a name (and possibly the current resource/request path) into a policy that remains static for the lifetime of the endpoint.

I realize that by rolling our own we are in an advanced, not-necessarily-supported case. But that shouldn't preclude us from at least opting-in to the combined policy cache. Limiting it to the default provider seems overly restrictive. If we are "advanced" enough to roll our own provider, we should be able to understand the implications of opting into the cache with an improperly implemented provider.

[davidfowl]

We need to restrict it but at the right layer. What we want to happen is that if the user does not override the default policy provider, they get the caching for free. If they do override it, then they need to opt in. I see a couple of options:

• Introduce a marker interface that policy providers can implement to tell the system it's OK to cache policies.
• Only call WithAuthorizationCache internally when the default provider is set.

[HaoK]

Yeah this is basically the same problem I ran into while testing, the fact that its hardcoded to a specific type lacks extensibility, alternatively could we just add a new setting to AuthorizationOptions, alwaysCachePolicies=true , safeToCachePolicies=true or something?

[davidfowl]

I don't think it should be an option. The marker interface lets the implementation opt in to the behavior.

[davidfowl]

The other option is an default interface method:

public interface IAuthorizationPolicyProvider
{
+    public bool CanCachePolicies => false;
}

[HaoK]

I like that idea: IAuthorizationPolicyProvider.CanCachePolicies

[halter73]

@HaoK Can we update the proposed API to what we merged so we can discuss it in todays API review?

[davidfowl]

I updated it.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API Review Notes:

• Why not cache all policies?
  • The policy might look at the HttpContext and vary based on the request.
• Anything that derives from DefaultAuthorizationPolicyProvider also has to opt-in.
  • public virtual bool CanCachePolicy => GetType() == typeof(DefaultAuthorizationPolicyProvider);
  • Which basically means any user defined provider will opt in.
• What do we think about the new AuthorizationMiddleware constructor?
  • We could avoid it, but it would take more work splitting the middleware into an internal a public part.
• Why IServiceProvider constructor param?
  • Because the service we're injecting is internal. 😞
  • We could split it like we did ExceptionHandlerMiddleware to avoid the new constructor.
• What about the CanCachePolicies name?
  • This implies the provider is doing the caching.
  • PolicyCachingAllowed? HasCacheablePolicies? CreatesCacheablePolicies? ProducesCacheablePolicies?
  • Let's go with AllowsCachingPolicies.

API Approved!

namespace Microsoft.AspNetCore.Authorization;

public interface IAuthorizationPolicyProvider
{
+    public bool AllowsCachingPolicies => false;
}

public class AuthorizationMiddleware
{
+    public AuthorizationMiddleware(RequestDelegate next, IAuthorizationPolicyProvider policyProvider, IServiceProvider services);
}