AuthorizeHelper will no-op if endpoint routing is used #10471

BrennanConroy · 2019-05-22T22:07:42Z

If I understand correctly, Auth will run on the Endpoint metadata (which we set the authorize metadata correctly) so internally SignalR doesn't need to run Auth again in the Endpoint code path.

pranavkm

cc @HaoK \ @rynowak

src/SignalR/server/SignalR/test/EndToEndTests.cs

src/SignalR/common/Http.Connections/src/ConnectionEndpointRouteBuilderExtensions.cs

src/SignalR/common/Shared/TaskCache.cs

src/SignalR/common/Http.Connections/src/ConnectionEndpointRouteBuilderExtensions.cs

BrennanConroy · 2019-05-24T21:46:33Z

src/SignalR/common/Http.Connections/src/ConnectionsAppBuilderExtensions.cs

-            app.UseRouter(routes.Build());
+            app.UseRouting();
+            app.UseAuthorization();
+            app.UseEndpoints(endpoints =>


@davidfowl
Have any thoughts on this? Converting app.UseSignalR(r => r.Map...()) to use Endpoints internally. This lets us get rid of our custom auth stuff in HttpConnectionDispatcher.

And side-effect, I believe will allow apps that port to 3.0 but don't change to endpoints to use the service automagically without UseAzureSignalR. (Once I figure out the HubMetadata issue)

This to me seems super spicy. Does it make sense to deprecate this API now that it's literally equivalent to something else? It's goodness if we can converge all of the code that we show in samples/docs.

I would be ok deprecating the old stuff
@anurse @davidfowl ?

I'm OK marking UseSignalR as obsolete if that's what we're talking about. We should have done that already anyway, since we want people to use UseEndpoints.

I would be obsoleting:
UseConnections
ConnectionsRouteBuilder
HubRouteBuilder
UseSignalR

Sounds good.

BrennanConroy · 2019-05-29T15:46:30Z

Ping reviewers, I basically rewrote the change so we no longer do Auth inside HttpConnectionDispatcher and rely on Auth through Endpoint routing.

The one remaining thing is that app.UseSignalR(routes => routes.MapHub<MyHub>("/path")); doesn't set the HubMetadata on the Endpoint produced.

pranavkm

Auth changes kinda makes sense, but would be great if somebody more familiar with SignalR's routing signed off on this.

halter73 · 2019-05-29T18:25:07Z

The one remaining thing is that app.UseSignalR(routes => routes.MapHub("/path")); doesn't set the HubMetadata on the Endpoint produced.

Can you explain why this is the case?

BrennanConroy · 2019-05-29T19:41:47Z

Can you explain why this is the case?

I've just made a commit with a fix, I didn't do it initially because it's a bit ugly, but I don't have a better solution right now.

halter73 · 2019-05-30T17:36:48Z

src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs

-        }
-
-        [Fact]
-        public async Task AuthenticatedUserWithoutPermissionCausesForbidden()


Do we still have end-to-end coverage for cases like this?

We do have some end-to-end tests for auth, for example: https://github.com/aspnet/AspNetCore/pull/10471/files#diff-c65c848cc599ae1724a72656ff60bedaR453

But we don't need to verify it as much because we now rely on the underlying auth system instead of implementing our own version.

Do we still manually verify authorization for hub method invocations to account for method-level auth attributes over a WebSocket for example? Where is that?

Yes we still do. This PR doesn't touch any of that.

Test located here

Yeah, I can't speak to the specific tests that are being removed, but I'd strongly encourage testing a variety of auth scenarios and interactions as a user would write them.

BrennanConroy · 2019-05-30T21:27:01Z

/azp run all

azure-pipelines · 2019-05-30T21:27:06Z

No pipelines are associated with this pull request.

BrennanConroy · 2019-05-30T21:27:24Z

@aspnet/build What's going on?

BrennanConroy · 2019-05-30T23:16:33Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=206259&view=logs

dougbu · 2019-05-30T23:27:47Z

@BrennanConroy looks like AzDO missed your comment or hit a temporary glitch.

Later, the Hello bot ignored your triage request. That bot seems to get confused when the URL doesn't exactly match a known format (see aspnet/AspNetCore-Internal#2425). In this case, &view=logs probably busted it.

rynowak · 2019-05-31T00:02:53Z

src/SignalR/common/Http.Connections/src/Internal/AuthorizeHelper.cs

-
-namespace Microsoft.AspNetCore.Http.Connections.Internal
-{
-    internal static class AuthorizeHelper


It makes me super happy to see this go away. We basically have 3 version of this code rn, and this cuts it down to two.

src/SignalR/common/Http.Connections/test/MapConnectionHandlerTests.cs

rynowak · 2019-05-31T00:07:15Z

src/SignalR/server/SignalR/src/HubRouteBuilder.cs

@@ -25,6 +28,11 @@ public HubRouteBuilder(ConnectionsRouteBuilder routes)
            _routes = routes;
        }

+        internal HubRouteBuilder(IEndpointRouteBuilder endpoints)


Worth a comment explaining why and when this can be null?

rynowak · 2019-05-31T00:07:53Z

src/SignalR/server/SignalR/test/AuthHub.cs

+    [Authorize]
+    class AuthHub : Hub
+    {
+    }


So is the expectation that method-level auth doesn't play with endpoints or with the authZ middleware?

rynowak

I left a few teeny comments, but this looks like what I expected to see, and this seems like valuable simplification.

BrennanConroy · 2019-05-31T00:09:54Z

@dougbu I doubt it: #10425 (comment)

dougbu · 2019-05-31T03:47:59Z

@BrennanConroy that was a whole week ago ❕

More seriously, we don't know why the bot sometimes ignores triage requests.

BrennanConroy · 2019-05-31T16:41:53Z

src/SignalR/common/Http.Connections/test/MapConnectionHandlerTests.cs

+                    endpoint =>
+                    {
+                        Assert.Equal("/path/negotiate", endpoint.DisplayName);
+                        Assert.NotNull(endpoint.Metadata.GetMetadata<IEnableCorsAttribute>());


Look @anurse, I did a thing!

But do you have a test that verifies CORS headers are actually set? :)

BrennanConroy · 2019-05-31T20:37:20Z

/azp run AspNetCore-ci

azure-pipelines · 2019-05-31T20:37:29Z

Azure Pipelines failed to run 1 pipeline(s).

BrennanConroy · 2019-05-31T20:40:17Z

/azp run AspNetCore-ci

azure-pipelines · 2019-05-31T20:40:24Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

BrennanConroy · 2019-05-31T20:43:44Z

/azp run AspNetCore-ci

azure-pipelines · 2019-05-31T20:43:57Z

Azure Pipelines successfully started running 1 pipeline(s).

BrennanConroy · 2019-06-03T05:36:10Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=207741&view=logs

BrennanConroy added the area-signalr Includes: SignalR clients and servers label May 22, 2019

BrennanConroy requested review from davidfowl and pranavkm May 22, 2019 22:07

BrennanConroy requested review from analogrelay, halter73 and mikaelm12 as code owners May 22, 2019 22:07

pranavkm reviewed May 22, 2019

View reviewed changes

src/SignalR/server/SignalR/test/EndToEndTests.cs Show resolved Hide resolved

src/SignalR/common/Http.Connections/src/ConnectionEndpointRouteBuilderExtensions.cs Show resolved Hide resolved

analogrelay added this to the 3.0.0-preview6 milestone May 23, 2019

halter73 reviewed May 23, 2019

View reviewed changes

src/SignalR/common/Shared/TaskCache.cs Show resolved Hide resolved

halter73 reviewed May 23, 2019

View reviewed changes

src/SignalR/common/Http.Connections/src/ConnectionEndpointRouteBuilderExtensions.cs Outdated Show resolved Hide resolved

BrennanConroy commented May 24, 2019

View reviewed changes

BrennanConroy added 7 commits May 29, 2019 09:26

AuthorizeHelper will no-op if endpoint routing is used

4cd8a50

stash

c7975b9

fixes

ffd7c57

stash

f5065ee

wip all the endpoints

ad335be

test

3f615f0

remove authhelper

e42a014

pranavkm reviewed May 29, 2019

View reviewed changes

hubmetadata

c85d231

BrennanConroy force-pushed the brecon/noopAuth branch from 747869b to c85d231 Compare May 29, 2019 19:41

halter73 reviewed May 30, 2019

View reviewed changes

halter73 approved these changes May 30, 2019

View reviewed changes

remove dead code

e951d28

rynowak reviewed May 31, 2019

View reviewed changes

src/SignalR/common/Http.Connections/test/MapConnectionHandlerTests.cs Outdated Show resolved Hide resolved

rynowak reviewed May 31, 2019

View reviewed changes

fb and assert.collection

2c6094b

BrennanConroy commented May 31, 2019

View reviewed changes

obsolete

e4555a5

BrennanConroy added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label May 31, 2019

refs

943e59b

BrennanConroy merged commit f0df10f into master Jun 3, 2019

ghost deleted the brecon/noopAuth branch June 3, 2019 05:35

AuthorizeHelper will no-op if endpoint routing is used #10471

AuthorizeHelper will no-op if endpoint routing is used #10471

Conversation

BrennanConroy commented May 22, 2019

pranavkm left a comment

Choose a reason for hiding this comment

BrennanConroy May 24, 2019 • edited Loading

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

BrennanConroy commented May 29, 2019

pranavkm left a comment

Choose a reason for hiding this comment

halter73 commented May 29, 2019

BrennanConroy commented May 29, 2019

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

BrennanConroy commented May 30, 2019

azure-pipelines bot commented May 30, 2019

BrennanConroy commented May 30, 2019

BrennanConroy commented May 30, 2019

dougbu commented May 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak left a comment

Choose a reason for hiding this comment

BrennanConroy commented May 31, 2019

dougbu commented May 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy commented May 31, 2019

azure-pipelines bot commented May 31, 2019

BrennanConroy commented May 31, 2019

azure-pipelines bot commented May 31, 2019

BrennanConroy commented May 31, 2019

azure-pipelines bot commented May 31, 2019

BrennanConroy commented Jun 3, 2019

BrennanConroy May 24, 2019 •

edited

Loading