Add support for endpoint authorization #158

johnrutherford · 2018-08-17T13:40:45Z

Make all IAppBuilders extension methods consistent
Add more tests

Make all IAppBuilders extension methods consistent Add more tests

RehanSaeed · 2018-08-17T14:38:37Z

src/AspNetCore.Authorization/HttpAuthorizationHelper.cs

+        /// <param name="context"></param>
+        /// <param name="policyName"></param>
+        /// <returns>True if the user was authorized and the request should proceed; False if the request should be ended immediately.</returns>
+        public static async Task<bool> AuthorizeAsync(HttpContext context, string policyName)


I would suggest modelling this after the [Authorize] attribute in ASP.NET Core. In that attribute you can pass:

AuthenticationSchemes - (This is a comma delimeted string but only because you can't pass an array to an attribute). I think this one is important to provide an overload for.

RoleName - I would recommend using policies but some people like to shoot themselves in the foot and use role names.

ActiveAuthenticationSchemes - This is obsolete. Do not add this.

Also, is the authorization repo dead? I literally submitted a PR yesterday to use the ASP.NET Core Authorization package.

I think it's best to keep this simple. You can create a policy that specifies the authentication schemes or require role(s). I used a role check for the tests in Transports.AspNetCore.Tests\AuthenticatedTestStartup.cs.

I think the stuff in the authorization repo is more for authorizing individual nodes. I felt there was a separate need to support transport authorization of the endpoint that integrates with ASP.NET Core authentication and will trigger the authentication challenge or forbidden responses.

RehanSaeed · 2018-08-17T14:43:02Z

src/AspNetCore.Authorization/GraphQLBuilderExtensions.cs

+        /// </summary>
+        /// <param name="builder"></param>
+        /// <returns></returns>
+        public static IGraphQLBuilder AddHttpAuthorization(this IGraphQLBuilder builder)


Is the reason this is called AddHttpAuthorization, because subscriptions don't support auth?

It was more to differentiate it from the authorization of the GraphQL nodes.

I also added authorization support for the WebSocket transport. WebSockets still uses HTTP(S) before the connection is upgraded.

RehanSaeed · 2018-08-17T14:52:54Z

src/Transports.AspNetCore/GraphQLHttpMiddleware.cs

            {
                await _next(context);
                return;
            }

+            // Authorize request
+            if (!await HttpAuthorizationHelper.AuthorizeAsync(context, _options.AuthorizationPolicyName))


Ok, I understand this now. So AuthorizationPolicyName is like a default policy for the entire API. When you call Microsofts AddAuthorization function you can already set a default policy there, so I don't think you need this:

services.AddAuthorization(options => options.DefaultPolicy = "FooPolicy");

I think you should take a look at the code from my PR graphql-dotnet/authorization#13 to be able to add auth policies to individual graph fields or graph types.

I think UseAuthorization() actually applies the DefaultPolicy above. So there is already middleware that will achieve this built into ASP.NET Core.

Do you know what package UseAuthorization() is in? I can't find it. I looked for a generic middleware that would do that, but I couldn't find one.

Confirmed it doesn't exist: https://github.com/aspnet/Security/issues/1734.

You're absolutely right, I must have imagined it. Looks like they are planning to add a middleware to do this though. If we can't wait for Microsoft to implement it, perhaps it should be a separate UseDefaultAuthorizationPolicy middleware.

Either way, we should still not add AuthorizationPolicyName to GraphQLHttpMiddlewareOptions. Instead we should use DefaultPolicy in the authorization options.

I thought about using the DefaultPolicy, but what if you want to use the default policy for MVC and a different policy for your GraphQL endpoint? With the AuthorizationPolicyName you're able to specify whichever policy you want to use.

I like the passing of a policy name vs. using DefaultPolicy for the reasons @johnrutherford gave.

You can run multiple apps in a single ASP.NET Core application with separate DI containers, see blog post. Not well known you can do this.

joemcbride · 2018-08-17T18:23:04Z

src/AspNetCore.Authorization/AspNetCore.Authorization.csproj

+    <ProjectReference Include="..\Core\Core.csproj" />
+  </ItemGroup>
+
+</Project>


I'm a bit concerned about adding "yet another project" that will have to be packaged as another nuget. FubuMVC ran into this issue of tons of different nuget packages and eventually consolidated everything. Since this is a shared dependency I think it should just be added to the Core project.

Yeah, I was unsure since it brought in additional dependencies, but I think the consolidation would be better.

joemcbride · 2018-08-17T18:34:53Z

src/Transports.AspNetCore/GraphQLHttpMiddleware.cs

            {
                await _next(context);
                return;
            }

+            // Authorize request
+            if (!await HttpAuthorizationHelper.AuthorizeAsync(context, _options.AuthorizationPolicyName))


I like the passing of a policy name vs. using DefaultPolicy for the reasons @johnrutherford gave.

joemcbride · 2018-08-17T18:41:07Z

src/Ui.Voyager/VoyagerExtensions.cs


-namespace GraphQL.Server.Ui.Voyager
+namespace GraphQL.Server


🎉 Like the namespace cleanup.

joemcbride · 2018-08-17T18:51:11Z

src/Transports.AspNetCore/GraphQLHttpMiddleware.cs

+            // Authorize request
+            if (!await HttpAuthorizationHelper.AuthorizeAsync(context, _options.AuthorizationPolicyName))
+            {
+                return;


Just to clarify here, this doesn't need to call the next Middleware because this auth check failed and it should be calling Forbid or Challenge?

joemcbride · 2018-08-17T18:53:15Z

src/AspNetCore.Authorization/HttpAuthorizationHelper.cs

+                return false;
+            }
+
+            return false;


If it reaches here, it appears that we're missing a authorizeResult case (or it should perhaps never be possible to happen?). Should this throw or set a http status code? Since we're not invoking the next middleware it seems this request will just stop without providing any result.

throw new InvalidOperationException("this should never be reached")

It shouldn't happen. Yeah, I think it would be a good idea to throw an exception there.

joemcbride · 2018-08-17T18:57:52Z

src/Transports.AspNetCore/GraphQLHttpMiddleware.cs

+            if (!HttpMethods.IsGet(httpRequest.Method) && !HttpMethods.IsPost(httpRequest.Method))
+            {
+                context.Response.Headers.Add("Allow", "GET, POST");
+                context.Response.StatusCode = 405; // Method Not Allowed


To be consistent perhaps use HttpStatusCode.MethodNotAllowed here and cast it to int. That should remove the need for the comment.

The recommended approach from MS is now to use Microsoft.AspNetCore.Http.StatusCodes, as it's a static class with const fields therefore implying more codes can come as opposed to the old HttpStatusCode enum. Don't have a source, they explained it better, was in an aspnet repo though.

johnrutherford · 2018-08-18T13:48:33Z

@joemcbride pushed some additional changes based on your feedback.

pekkah · 2018-10-19T14:07:45Z

Is this now obsolete?

johnrutherford · 2018-10-19T14:26:27Z

I think there is still a use for it. It allows you to authorize access to the entire endpoint. If the request is not authorized, none of the GraphQL code will be run. If you are relying only on authorization within GraphQL, there is more of a performance cost with the GraphQL execution and checking authorization on nodes.

This will also return standard HTTP status codes (i.e., 401 or 403) to indicate to clients that the request was unauthorized/forbidden. This makes it easier for the client to know to re-authenticate the user. Otherwise, the client needs to parse errors in the GraphQL response to look for a specific message.

I am currently achieving the same result using a custom AuthorizationMiddleware like this:

app.Map("/graphql", branch =>
{
    branch.UseAuthorization(Policies.GraphQL);
    branch.UseGraphQL<MySchema>("");
});

I would prefer something like this:

app.UseGraphQL<MySchema>(opts => opts.AuthorizationPolicyName = Policies.GraphQL);

sungam3r · 2020-11-11T19:56:42Z

@johnrutherford Is this PR still relevant after #440 ?

sungam3r · 2020-11-11T20:02:34Z

I see that at least part of the changes in this PR does what has already been done. Some of the code has already been deprecated and has been removed. The remaining code, as I understand it, establishes certain authorization rules. Authorization is now encapsulated in a separate project. Will you keep working on this PR or is it not worth it?

sungam3r · 2020-12-06T09:46:49Z

@johnrutherford @RehanSaeed friendly bump

sungam3r · 2020-12-20T13:44:44Z

@johnrutherford If there is no more work on this PR, then I'm going to close it after the merging #477 . Otherwise, you need to resolve conflicts and update this PR to target on the develop branch.

sungam3r · 2021-03-26T21:34:53Z

Closed as stale.

Add support for endpoint authorization

9a81602

Make all IAppBuilders extension methods consistent Add more tests

johnrutherford requested review from joemcbride, pekkah and RehanSaeed August 17, 2018 13:40

Fix build script

9a95d3e

RehanSaeed reviewed Aug 17, 2018

View reviewed changes

joemcbride reviewed Aug 17, 2018

View reviewed changes

johnrutherford mentioned this pull request Aug 18, 2018

Extensibility point is needed to return 401 or 500 based on request execution #159

Closed

Changes from code review feedback

358e8a6

johnrutherford mentioned this pull request Aug 31, 2018

Performance improvements graphql-dotnet/graphql-dotnet#805

Merged

johnrutherford mentioned this pull request Sep 10, 2018

Subscription authorization #156

Closed

sungam3r added the BREAKING Breaking changes in either public API or runtime behavior label Feb 24, 2020

Shane32 changed the base branch from develop to master October 26, 2020 23:31

sungam3r added the needs confirmation The problem is most likely resolved and requires verification by the author label Nov 11, 2020

sungam3r requested review from joemcbride and RehanSaeed December 20, 2020 13:47

sungam3r closed this Mar 26, 2021

sungam3r removed the needs confirmation The problem is most likely resolved and requires verification by the author label Mar 26, 2021

johnrutherford deleted the authorization branch March 29, 2021 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for endpoint authorization #158

Add support for endpoint authorization #158

johnrutherford commented Aug 17, 2018

RehanSaeed Aug 17, 2018 •

edited

Loading

johnrutherford Aug 17, 2018 •

edited

Loading

RehanSaeed Aug 17, 2018

johnrutherford Aug 17, 2018

RehanSaeed Aug 17, 2018 •

edited

Loading

RehanSaeed Aug 17, 2018

johnrutherford Aug 17, 2018

johnrutherford Aug 17, 2018

RehanSaeed Aug 17, 2018

johnrutherford Aug 17, 2018

joemcbride Aug 17, 2018

RehanSaeed Aug 20, 2018

joemcbride Aug 17, 2018

johnrutherford Aug 17, 2018

joemcbride Aug 17, 2018

joemcbride Aug 17, 2018

joemcbride Aug 17, 2018

johnrutherford Aug 17, 2018

joemcbride Aug 17, 2018

johnrutherford Aug 17, 2018

joemcbride Aug 17, 2018

benmccallum Jul 18, 2019

johnrutherford commented Aug 18, 2018

pekkah commented Oct 19, 2018

johnrutherford commented Oct 19, 2018 •

edited

Loading

sungam3r commented Nov 11, 2020

sungam3r commented Nov 11, 2020

sungam3r commented Dec 6, 2020

sungam3r commented Dec 20, 2020 •

edited

Loading

sungam3r commented Mar 26, 2021

Add support for endpoint authorization #158

Add support for endpoint authorization #158

Conversation

johnrutherford commented Aug 17, 2018

RehanSaeed Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

johnrutherford Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RehanSaeed Aug 17, 2018 • 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

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

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

johnrutherford commented Aug 18, 2018

pekkah commented Oct 19, 2018

johnrutherford commented Oct 19, 2018 • edited Loading

sungam3r commented Nov 11, 2020

sungam3r commented Nov 11, 2020

sungam3r commented Dec 6, 2020

sungam3r commented Dec 20, 2020 • edited Loading

sungam3r commented Mar 26, 2021

RehanSaeed Aug 17, 2018 •

edited

Loading

johnrutherford Aug 17, 2018 •

edited

Loading

RehanSaeed Aug 17, 2018 •

edited

Loading

johnrutherford commented Oct 19, 2018 •

edited

Loading

sungam3r commented Dec 20, 2020 •

edited

Loading