Allow specifying authz policies on endpoints #41153

HaoK · 2022-04-12T18:29:12Z

Continuation of #39892

Per offline discussion, removed requirements from this iteration as those APIs need more baking
Added the two new API review approved RequireAuthorization overloads from Allow direct configuration of authorization policies via endpoint metadata #39840

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

HaoK · 2022-04-12T21:11:42Z

src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs

+        // REVIEW: should we check if there already is any authorize attributes first?
+        builder.Add(endpointBuilder =>
+        {
+            endpointBuilder.Metadata.Add(new AuthorizeAttribute());


Should we check if any IAuthorizeData is already in the metadata so we don't add extra empty [Authorize]s?

Yes, but I'm not sure where. Can we do it in this callback? We need an "only add metadata if not already added"

I just added a Any query inline here to prevent dupes, does that work?

davidfowl · 2022-04-12T22:54:49Z

Can you update the code in SignalR as well?

cc @BrennanConroy

HaoK · 2022-04-13T01:52:39Z

Sure what's the equivalent way to get endpoint metadata in SignalR? Looks like we just need to get the metadata policies and add that to the HubMethodDescriptor to flow it to the authZ logic, somewhere like here after we get the Authorize attributes on the method:

aspnetcore/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs

Line 683 in f283f8b

    
           _methods[methodName] = new HubMethodDescriptor(executor, serviceProviderIsService, authorizeAttributes);

BrennanConroy · 2022-04-13T02:11:22Z

Sounds about right, and then update

aspnetcore/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs

Line 566 in f283f8b

    
           var authorizePolicy = await AuthorizationPolicy.CombineAsync(policyProvider, policies);

to use the policies.

HaoK · 2022-04-13T18:07:15Z

So for SignalR, what's the best way for me to get access to the new endpoint metadata, I see the tests are using EndpointDataSource, should I be doing this as well to find the new auth policy metadata in DefaultHubDispatcher?

BrennanConroy · 2022-04-13T18:17:53Z

Test for Hub method auth is at

aspnetcore/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs

Line 2211 in 130849a

public async Task AuthorizedConnectionCanInvokeHubMethodWithAuthorization()

BrennanConroy · 2022-04-13T21:20:49Z

src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs

@@ -549,21 +551,26 @@ private void InitializeHub(THub hub, HubConnectionContext connection)

    private static Task<bool> IsHubMethodAuthorized(IServiceProvider provider, HubConnectionContext hubConnectionContext, HubMethodDescriptor descriptor, object?[] hubMethodArguments, Hub hub)
    {
+        var endpoint = hubConnectionContext.Features.Get<IEndpointFeature>()?.Endpoint;


Move this to where HubMethodDescriptor gets its AuthorizeData, we don't want the cost of these calls per Hub method invoke since it's static information.

HaoK · 2022-04-13T21:21:58Z

src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs

@@ -549,21 +551,26 @@ private void InitializeHub(THub hub, HubConnectionContext connection)

    private static Task<bool> IsHubMethodAuthorized(IServiceProvider provider, HubConnectionContext hubConnectionContext, HubMethodDescriptor descriptor, object?[] hubMethodArguments, Hub hub)
    {
+        var endpoint = hubConnectionContext.Features.Get<IEndpointFeature>()?.Endpoint;


So here was what I was asking about, how do I get the endpoint metadata out, this doesn't look correct because looking at the current auth tests, I don't think it even exposes anything like an HttpContext in the TestClient that the tests use, or at least other than stuffing in my own IEndpointFeature into the test client, it feels like I'm doing something wrong

Ok, so there is no endpoint for a Hub method. The endpoint is already running at this point in the code and has run AuthN/AuthZ for the endpoint.

We grab the metadata off the Hub method and use that. So if you look at DiscoverHubMethods(...) we grab the AuthorizeAttribute off of the methodInfo and will do the same for policies now.

So how does that work for these new policies, since they only exist as metadata off an Endpoint, there is no authorize attribute equivalent I can look at without the endpoint metadata?

I guess it doesn't apply until we do the [RequireClaims] change?

@davidfowl what do we want to do exactly for signalR here?

Ok confirmed in standup that we don't have to do anything here since it won't work, so I will remove these changes in the dispatcher for now

This reverts commit e860244.

This reverts commit 56f38e1.

davidfowl and others added 4 commits January 30, 2022 21:00

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.

APIs...

045f9f8

Merge branch 'davidfowl/authz-extrametadata' into haok/authz-meta

e1eceef

Remove ability to add requirements

585f30e

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

HaoK added 2 commits April 12, 2022 11:30

Cleanup

9c03db0

New RequireAuthorization overloads

b512ca8

HaoK requested a review from davidfowl April 12, 2022 21:08

HaoK commented Apr 12, 2022

View reviewed changes

Only add authorize if needed

3a65292

Add signalR metadata tests

a528db3

Try to get endpoint metadata

56f38e1

BrennanConroy reviewed Apr 13, 2022

View reviewed changes

HaoK commented Apr 13, 2022

View reviewed changes

HaoK added 3 commits April 13, 2022 14:56

Fix build

e860244

Revert "Fix build"

98ac7b5

This reverts commit e860244.

Revert "Try to get endpoint metadata"

c212e8b

This reverts commit 56f38e1.

HaoK marked this pull request as ready for review April 19, 2022 17:28

HaoK requested review from halter73 and Tratcher as code owners April 19, 2022 17:28

HaoK enabled auto-merge (squash) April 19, 2022 23:10

davidfowl approved these changes Apr 19, 2022

View reviewed changes

HaoK merged commit 4f1c90b into main Apr 19, 2022

HaoK deleted the haok/authz-meta branch April 19, 2022 23:22

ghost added this to the 7.0-preview4 milestone Apr 19, 2022

captainsafia mentioned this pull request Aug 18, 2022

Consider adding an IEndpointConventionBuilder.RequireAuthorization overload that accepts an AuthorizationPolicy #18497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow specifying authz policies on endpoints #41153

Allow specifying authz policies on endpoints #41153

Uh oh!

HaoK commented Apr 12, 2022 •

edited

Loading

Uh oh!

HaoK Apr 12, 2022

Uh oh!

davidfowl Apr 13, 2022

Uh oh!

HaoK Apr 13, 2022

Uh oh!

davidfowl commented Apr 12, 2022

Uh oh!

HaoK commented Apr 13, 2022

Uh oh!

BrennanConroy commented Apr 13, 2022

Uh oh!

HaoK commented Apr 13, 2022

Uh oh!

BrennanConroy commented Apr 13, 2022

Uh oh!

BrennanConroy Apr 13, 2022

Uh oh!

HaoK Apr 13, 2022

Uh oh!

BrennanConroy Apr 13, 2022 •

edited

Loading

Uh oh!

HaoK Apr 13, 2022

Uh oh!

BrennanConroy Apr 13, 2022

Uh oh!

HaoK Apr 14, 2022

Uh oh!

HaoK Apr 15, 2022

Uh oh!

Uh oh!

Allow specifying authz policies on endpoints #41153

Allow specifying authz policies on endpoints #41153

Uh oh!

Conversation

HaoK commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Apr 12, 2022

Uh oh!

HaoK commented Apr 13, 2022

Uh oh!

BrennanConroy commented Apr 13, 2022

Uh oh!

HaoK commented Apr 13, 2022

Uh oh!

BrennanConroy commented Apr 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HaoK commented Apr 12, 2022 •

edited

Loading

BrennanConroy Apr 13, 2022 •

edited

Loading