Proposal for how we handle more metadata for auth #39892

davidfowl · 2022-01-31T16:06:48Z

The user can specify additional requirements or policies on the endpoint and they will be combined with existing IAuthorizeData.

Contributes to #39840

This is what I was thinking the change could look like in all of the places that currently call into the auth system.

Some open questions:

In this change you need to use the Authorize attribute still. It's not enough to have a policy on the endpoint. This is sheer laziness but it also means we would need to detect policies on the endpoint execution middleware (

aspnetcore/src/Http/Routing/src/EndpointMiddleware.cs

Line 38 in 3de52f5

if (endpoint.Metadata.GetMetadata<IAuthorizeData>() != null &&

).
Same idea for requirements, they can augment the authorize attribute on the endpoint. This one is trickier because if you want to make sure requirements apply to a specific set of auth schemes then you need to use an authorize attribute as well to specify that.

cc @HaoK @pranavkm @BrennanConroy @Tratcher

- The user can specify additional requirements or policies on the endpoint and they will be combined with existing IAuthorizeData.

HaoK · 2022-01-31T16:23:29Z

src/Security/Authorization/Core/src/AuthorizationPolicy.cs

    /// <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, IEnumerable<IAuthorizeData> authorizeData)
+    public static async Task<AuthorizationPolicy?> CombineAsync(IAuthorizationPolicyProvider policyProvider,


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?

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).

davidfowl · 2022-01-31T16:34:59Z

Also side note: It's unfortunate we do all of this work per request. This could be done and cached on the endpoint once.

pranavkm · 2022-01-31T18:07:50Z

src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs

+        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);


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.

#39892 (comment)

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

pranavkm · 2022-01-31T18:09:22Z

src/Security/Authorization/Core/src/AuthorizationPolicy.cs

-    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>(),


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

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

pranavkm · 2022-01-31T18:10:38Z

src/Security/Authorization/Core/src/AuthorizationPolicy.cs

+    /// <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>


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>

davidfowl · 2022-02-02T04:44:27Z

Any other thoughts on this @HaoK @Tratcher @blowdart? I'll expand this to work in other places to get a since for how it would look there.

HaoK · 2022-02-02T08:58:24Z

This looks nice so far. Does this make it any better in any situation now that you can specifying authentication schemes in the Policies this way as well? I guess now you can have pure AuthZ policies that don't specify any authentication schemes, and just put metadata on the endpoint that says whether it takes jwt or cookies, which maybe is a little better than before

HaoK · 2022-02-02T09:04:49Z

src/Security/Authorization/test/AuthorizationMiddlewareTests.cs

+        {
+            services.AddSingleton<IAuthorizationHandler, CustomAuthHandler>();
+        },
+        endpoint: CreateEndpoint(new AuthorizeAttribute(), new CustomRequirement("This")));


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?

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?

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.

davidfowl · 2022-02-02T17:05:46Z

This looks nice so far. Does this make it any better in any situation now that you can specifying authentication schemes in the Policies this way as well? I guess now you can have pure AuthZ policies that don't specify any authentication schemes, and just put metadata on the endpoint that says whether it takes jwt or cookies, which maybe is a little better than before

Right so I guess you can do:

[Authorize], Default Policy
[Authorize(Policy, Schemes)]
[Authorize], Default Policy, Additional requirements from metadata
[Authorize(Schemes)], Default Policy, Additional requirements from metadata
[Authorize], Additional policies from metadata (overrides the default)
[Authorize(Schemes)], Additional policies from metadata (combine everything like multiple auth attributes)

HaoK · 2022-02-03T00:26:31Z

So do we need to give them both the ability to define policies and requirements? Does it simplify anything if we only give them the ability to define additional policies basically we lose 3. and 4. Its only slightly more work to shove your requirements into an empty policy

davidfowl · 2022-02-03T16:10:39Z

The problem is that policies aren't easily defined directly on the endpoint outside of minimal APIs. The only way to declaratively define a policy is the way we do today. I think 3 is a critical feature but it also worries me that you need both IAuthorizeData and the additional requirements. Feels like a recipe for security mistakes since you need both things.

davidfowl · 2022-02-03T17:22:51Z

Thinking more about it, it would be possible to push this responsibility into the frameworks (creating a policy from requirements) but it would always override the default policy....

HaoK · 2022-02-03T18:06:16Z

I mean we could always add a DefaultPolicyRequirement, which they could use to signal ask to get the combination. I think its simpler for the metadata to be limited to asking for more schemes and requirements, leave packaging/combining things to the frameworks, there's no notion of polices in the lowest level of IAuthorizationService anyways, it's always just been a unit of moving metadata around. They can already easily augment with policies today by just adding [Authorize(Policy)} right? So you can get 3,4 behavior just by augmenting via the special DefaultPolicyRequirement, or some other flag

HaoK · 2022-04-01T19:08:02Z

@blowdart any thoughts about this before this starts getting real?

blowdart · 2022-04-01T19:10:16Z

I suggest talking to Jiachen who is doing user research into what users want, before assuming any suggested API changes help.

HaoK · 2022-04-01T19:24:45Z

The motivation behind this directly came out of some user feedback from the team building our docs site who had to do a lot of complex string parsing in their policy providers, which lead to focusing on making it easier to flow metadata from the app (authorize attribute), to the AuthZ handlers, see #39840

Users have been expressing what they want here for several years now, this PR is where we try to design something nicer as we always knew the PolicyProvider that only allowed the name of the policy as the communication was just a first step/workaround

Proposal for how we handle more metadata for auth

cf929b6

- The user can specify additional requirements or policies on the endpoint and they will be combined with existing IAuthorizeData.

ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jan 31, 2022

HaoK reviewed Jan 31, 2022

View reviewed changes

APIs...

045f9f8

pranavkm reviewed Jan 31, 2022

View reviewed changes

HaoK reviewed Feb 2, 2022

View reviewed changes

HaoK mentioned this pull request Apr 1, 2022

Allow direct configuration of authorization policies via endpoint metadata #39840

Closed

HaoK mentioned this pull request Apr 12, 2022

Allow specifying authz policies on endpoints #41153

Merged

davidfowl closed this Apr 19, 2022

davidfowl deleted the davidfowl/authz-extrametadata branch April 24, 2022 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for how we handle more metadata for auth #39892

Proposal for how we handle more metadata for auth #39892

davidfowl commented Jan 31, 2022 •

edited

Loading

HaoK Jan 31, 2022

davidfowl Jan 31, 2022

davidfowl commented Jan 31, 2022

pranavkm Jan 31, 2022

davidfowl Jan 31, 2022

Kahbazi Feb 1, 2022

pranavkm Jan 31, 2022

davidfowl Feb 1, 2022

pranavkm Jan 31, 2022

davidfowl commented Feb 2, 2022 •

edited

Loading

HaoK commented Feb 2, 2022

HaoK Feb 2, 2022

davidfowl Feb 2, 2022

HaoK Feb 3, 2022

davidfowl commented Feb 2, 2022

HaoK commented Feb 3, 2022

davidfowl commented Feb 3, 2022

davidfowl commented Feb 3, 2022

HaoK commented Feb 3, 2022 •

edited

Loading

HaoK commented Apr 1, 2022

blowdart commented Apr 1, 2022

HaoK commented Apr 1, 2022

	/// <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>

Proposal for how we handle more metadata for auth #39892

Proposal for how we handle more metadata for auth #39892

Conversation

davidfowl commented Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Feb 2, 2022 • edited Loading

HaoK commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Feb 2, 2022

HaoK commented Feb 3, 2022

davidfowl commented Feb 3, 2022

davidfowl commented Feb 3, 2022

HaoK commented Feb 3, 2022 • edited Loading

HaoK commented Apr 1, 2022

blowdart commented Apr 1, 2022

HaoK commented Apr 1, 2022

davidfowl commented Jan 31, 2022 •

edited

Loading

davidfowl commented Feb 2, 2022 •

edited

Loading

HaoK commented Feb 3, 2022 •

edited

Loading