Adding Auth to endpoints #241

BrennanConroy · 2017-02-26T20:58:34Z

This PR is mainly to see what the current Auth progress is and if it is heading in the right direction and also to get input on design. Built on top of #240. Try to ignore most of the weird stuff in the samples.

@moozzyk @davidfowl @anurse @mikaelm12

dnfclas · 2017-02-26T20:58:38Z

@BrennanConroy,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

mikaelm12 · 2017-02-27T22:32:40Z

samples/ChatSample/Hubs/Chat.cs

-            {
-                Context.Connection.Dispose();
-            }
+            //if (!Context.User.Identity.IsAuthenticated)


mikaelm12 · 2017-02-27T22:33:19Z

samples/ChatSample/Startup.cs

 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;

 namespace ChatSample
 {
+    public class req : IAuthorizationRequirement


Change this class name

mikaelm12 · 2017-02-27T22:33:30Z

samples/ChatSample/Startup.cs

+    public class req : IAuthorizationRequirement
+    {
+    }
+    public class reqHandler : AuthorizationHandler<req>


This one too

mikaelm12 · 2017-02-27T22:38:07Z

samples/ChatSample/Startup.cs

@@ -54,8 +71,13 @@ public void ConfigureServices(IServiceCollection services)
            services.AddTransient<ISmsSender, AuthMessageSender>();

            services.AddSignalR();
+            services.AddEndPoint<HubEndPoint<Chat>>(options =>


new line here?

mikaelm12 · 2017-02-27T22:40:57Z

src/Microsoft.AspNetCore.Sockets/HttpConnectionDispatcher.cs

 using Microsoft.Extensions.Primitives;
+using Microsoft.Extensions.Internal;


nit: Sort this last using

analogrelay

Looking good so far. Bring @HaoK in to look at the authentication process. We're trying to mimic MVC here, but with EndPoints instead of Controllers/Actions as the authorized resource.

analogrelay · 2017-02-28T18:57:46Z

src/Microsoft.AspNetCore.Sockets/EndPointOptions.cs

+{
+    public class EndPointOptions<TEndPoint> where TEndPoint : EndPoint
+    {
+        public AuthorizationPolicy Policy { get; set; }


Do we want to take the actual policy or a name to look up? I know AuthorizeAttribute uses a name, but then again it has to because it's an Attribute.

You probably want to take the name, as that would enable scenarios where the policy is resolved dynamically by name which would be blocked if you took the policy instance always.

HaoK · 2017-02-28T19:43:33Z

src/Microsoft.AspNetCore.Sockets/HttpConnectionDispatcher.cs

@@ -48,6 +58,59 @@ public HttpConnectionDispatcher(ConnectionManager manager, ILoggerFactory logger
            }
        }

+        private async Task<bool> AuthorizeAsync<TEndPoint>(HttpContext context) where TEndPoint : EndPoint


Is this pretty much an exact copy of the AuthorizeFilter logic? As apart of Auth 2.0 I'll try to consolidate this logic somewhere that MVC + SignalR can consume rather than duplicate...

Yeah its from AuthorizeFilter, a common spot for this would be great!

/cc @javiercn

Agree. We should have live somewhere where both MVC and Signal R can consume

So currently AuthZ doesn't depend on HttpAbstractions, assuming we want to keep that clean, we will need to add a new package for this helper in Security... maybe Microsoft.AspNetCore.Authorization.Http?

Or is this more appropriate as a shared common code i.e. AuthorizationHelper similar to SecurityHelper (or we could just add this as a new method in SecurityHelper too)...

I would make a package like Microsoft.AspNetCore.Authentication.Policy and I would offer a service
IPolicyAuthenticationProvider {
Task AuthenticateAsync(HttpContext context, string policyName);
}

It's implementation will do what the AuthorizeFilter does before setting the principal into the context. This has two benefits:

You can reuse this logic across implementations (MVC, SignalR, any other middleware).

You can get access to a principal that matches exactly what that policy defines at any point where you have access to the HttpContext.

Okay I'll start a prototype/design PR for the rough shape of something this week

Something like aspnet/Security#1148 look reasonable? It basically splits things into the authentication method, and authorization method (eventually this might have logic to distinguish between challenge/forbidden)

davidfowl · 2017-03-12T09:04:36Z

@BrennanConroy What's the deal with this PR?

davidfowl · 2017-03-20T19:23:15Z

@BrennanConroy rebsae

davidfowl · 2017-03-20T19:23:36Z

src/Microsoft.AspNetCore.Sockets/HttpConnectionDispatcher.cs

@@ -51,6 +60,58 @@ public HttpConnectionDispatcher(ConnectionManager manager, ILoggerFactory logger
            }
        }

+        private async Task<bool> AuthorizeAsync<TEndPoint>(HttpContext context) where TEndPoint : EndPoint


Move this code to a helper

BrennanConroy · 2017-03-21T04:18:32Z

🆙 📅

davidfowl · 2017-03-21T04:23:52Z

src/Microsoft.AspNetCore.Sockets/HttpConnectionDispatcher.cs

+        private Task<bool> AuthorizeAsync<TEndPoint>(HttpContext context) where TEndPoint : EndPoint
+        {
+            // TODO: Authorize attribute on EndPoint
+            var options = context.RequestServices.GetRequiredService<IOptions<EndPointOptions<TEndPoint>>>();


Why are you resolving options twice?

analogrelay

Small notes and approval is subject to build passing

analogrelay · 2017-03-21T16:47:10Z

src/Microsoft.AspNetCore.Sockets/HttpConnectionDispatcher.cs

            var options = context.RequestServices.GetRequiredService<IOptions<EndPointOptions<TEndPoint>>>().Value;
+            // TODO: Authorize attribute on EndPoint


Is there a bug for this?

analogrelay · 2017-03-21T16:50:15Z

src/Microsoft.AspNetCore.Sockets/Internal/AuthorizeHelper.cs

+{
+    public static class AuthorizeHelper
+    {
+        public static async Task<bool> AuthorizeAsync(HttpContext context, AuthorizationPolicy policy)


Given that we complete synchronously if policy is null, perhaps this is a place for ValueTask<bool>? C# 7 lets you drop it in place and continue using async. It's not a super high-performance code path, but it seems useful for the relatively common case of not having an auth policy.

@davidfowl thoughts on more ValueTasks? Premature optimization? Other unknown costs?

Do it in a follow up commit.

If there's going to be a new commit to remove the jasmine crap, may as well throw it in here, it's a small change (unless ValueTask isn't available in our current package set, then I agree, just get it in).

Never mind, Task<bool> is cached: https://referencesource.microsoft.com/#mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs,a431f9d7dd423518

analogrelay · 2017-03-21T16:55:52Z

test/Microsoft.AspNetCore.SignalR.Test.Server/wwwroot/lib/jasmine/boot.js

@@ -0,0 +1,152 @@
+/*


Is this supposed to be checked in? If not, can we gitignore it?

It totally confused me.

dnfclas added the cla-already-signed label Feb 26, 2017

mikaelm12 reviewed Feb 28, 2017

View reviewed changes

analogrelay reviewed Feb 28, 2017

View reviewed changes

HaoK reviewed Feb 28, 2017

View reviewed changes

BrennanConroy force-pushed the brecon/endpoint_auth branch 3 times, most recently from faa3930 to c1f3845 Compare March 1, 2017 17:21

BrennanConroy force-pushed the brecon/endpoint_auth branch 2 times, most recently from f6bea88 to c86a3b4 Compare March 20, 2017 16:14

BrennanConroy changed the title ~~[WIP] Adding Auth to endpoints~~ Adding Auth to endpoints Mar 20, 2017

davidfowl reviewed Mar 20, 2017

View reviewed changes

BrennanConroy force-pushed the brecon/endpoint_auth branch from c86a3b4 to 842662e Compare March 20, 2017 22:10

davidfowl reviewed Mar 21, 2017

View reviewed changes

BrennanConroy force-pushed the brecon/endpoint_auth branch from f28baa3 to 23ca2a2 Compare March 21, 2017 16:45

analogrelay approved these changes Mar 21, 2017

View reviewed changes

davidfowl approved these changes Mar 21, 2017

View reviewed changes

Adding Auth to endpoints

8f9f6bc

BrennanConroy force-pushed the brecon/endpoint_auth branch from 4f9c8e5 to 8f9f6bc Compare March 21, 2017 17:24

BrennanConroy merged commit 8f9f6bc into dev Mar 21, 2017

BrennanConroy deleted the brecon/endpoint_auth branch March 21, 2017 17:25

		using Microsoft.Extensions.Primitives;
		using Microsoft.Extensions.Internal;

		var options = context.RequestServices.GetRequiredService<IOptions<EndPointOptions<TEndPoint>>>().Value;
		// TODO: Authorize attribute on EndPoint

Adding Auth to endpoints #241

Adding Auth to endpoints #241

Uh oh!

Conversation

BrennanConroy commented Feb 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Feb 26, 2017

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!

analogrelay left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Mar 12, 2017

Uh oh!

davidfowl commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay left a comment

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!

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!

BrennanConroy commented Feb 26, 2017 •

edited

Loading