Replace authorization project with new rule #803

Shane32 · 2022-05-21T00:59:01Z

This is a complete rewrite of the authorization rule, with many new features as shown below.

New features:

Supports role-based policy rules (e.g. [Authorize(Roles = "Admin")])
Supports IsAuthenticated authentication rule (e.g. [Authorize])
Supports AllowAnonymous to allow querying an anonymous field of a protected type
Introspection queries are treated as anonymous fields; the schema can have authorization rules applied if introspection queries are also to be protected
Supports @skip and @include directives properly
Properly ignores fragments and operations that are not in use
Returns more helpful 'access denied' messages as it pertains to the field or type that failed the check
Does not reveal the reason that the authentication check failed, as this leaks security information.
Provides virtual methods which can be overridden to provide more detailed messages.
IAuthorizationService is pulled from the scoped DI, just in case it is a scoped service (although it probably isn't).

Removed features:

Does not support authorization of input types. This is because the only practical way to validate input object types is to validate all possible child types that an input object type might contain, due to the fact that input objects are deserialized via ParseDictionary within the input object, not to mention they may contain default values. As such, it is just as effective to protect the field or query argument that references the input type that would trigger the validation check. This could be done as described above, however, in another PR.
Does not support IClaimsPrincipalAccessor. This project is for ASP.NET Core and need not support custom authentication schemes. The GraphQL.Authorization project would be better suited for such a scenario. The HttpContext.User property may be set in application pipeline to change the ClaimsPrincipal that validation takes place against. If we reintroduce support for IClaimsPrincipalAccessor, then it and IHttpContextAccessor needs to be registered for the HTTP middleware, which it is not now. It would also complicate the authentication support for WebSocket connections. We may want to review this decision after the new authentication provisions for WebSocket connections are introduced, as WebSocket connections require special authentication code. Note that inheritance may be used to use any ClaimsPrincipal with the authorization rule.
Does not support IAuthorizationErrorMessageBuilder and does not include code to build error messages based on the exact reason the authentication check failed. This is by design, as this is improperly leaking security rules to the caller. AuthorizationVisitor provides virtual methods if it is desired to override message generation. Further, ErrorInfoProvider can override authentication messages just as it did before (as seen in the sample).

Other changes/notes:

Requires ASP.NET Core's AddAuthorization to be installed separately; installing the validation rule does not add or configure ASP.NET Core's authorization framework.
Works with any DI provider.
Authorization rule is part of the main project. Configured as such it can now share AccessDeniedError with the main project. It was one file, but I split it into 4 files to ease the review process.
The authorization logic is rather complicated to properly implement. Even so, I've tried to minimize heap allocations. The local/private variables could be named better; I'm open to any suggestions.
Comprehensive tests are complete but not included in this PR yet; old authorization tests pass

Known issues:

ValidationContext.GetRecursivelyReferencedFragments does not consider @skip and @include directives, and so authorization checks may be performed on fragments that are not part of the operation due to @skip or @include. This should be fixed in GraphQL.NET.

Shane32 · 2022-06-15T14:59:48Z

@sungam3r This PR should be next to concentrate on.

Shane32 · 2022-06-19T01:04:52Z

Bump @sungam3r - status?

github-advanced-security

Found 38 potential problems in the proposed changes. Check the Files changed tab for more details.

src/Transports.AspNetCore/AuthorizationVisitorBase.cs

+            else if (node is GraphQLArgument)
+            {
+                // ignore arguments of directives
+                if (context.TypeInfo.GetAncestor(2)?.Kind == ASTNodeKind.Field)
+                {
+                    // verify field argument
+                    var arg = context.TypeInfo.GetArgument();
+                    if (arg != null)
+                    {
+                        Validate(arg, node, context);
+                    }
+                }
+            }


src/Transports.AspNetCore/AuthorizationVisitorBase.cs

+        if ((ti.WaitingOnFragments?.Count ?? 0) == 0)
+        {
+            if (_todos != null)
+            {
+                var count = _todos.Count;
+                for (var i = 0; i < count; i++)
+                {
+                    var todo = _todos[i];
+                    if (todo.WaitingOnFragments.Remove(fragmentName))
+                    {
+                        todo.AnyAuthenticated |= ti.AnyAuthenticated;
+                        todo.AnyAnonymous |= ti.AnyAnonymous;
+                        if (todo.WaitingOnFragments.Count == 0)
+                        {
+                            _todos.RemoveAt(i);
+                            count--;
+                            if (todo.AnyAuthenticated || !todo.AnyAnonymous)
+                            {
+                                Validate(todo.ValidationInfo);
+                            }
+                        }
+                    }
+                }
+            }
+        }


src/Transports.AspNetCore/AuthorizationVisitorBase.cs

+                else if (ifArg.Value is GraphQLVariable variable)
+                {
+                    if (context.Operation.Variables != null)
+                    {
+                        var varDef = context.Operation.Variables.FirstOrDefault(x => x.Variable.Name == variable.Name);
+                        if (varDef != null && varDef.Type.Name() == "Boolean")
+                        {
+                            if (context.Variables.TryGetValue(variable.Name.StringValue, out var value))
+                            {
+                                if (value is bool boolValue2)
+                                    return boolValue2;
+                            }
+                            if (varDef.DefaultValue is GraphQLBooleanValue boolValue3)
+                            {
+                                return boolValue3.BoolValue;
+                            }
+                        }
+                    }
+                }


src/Transports.AspNetCore/AuthorizationVisitorBase.cs

+                            if (context.Variables.TryGetValue(variable.Name.StringValue, out var value))
+                            {
+                                if (value is bool boolValue2)
+                                    return boolValue2;
+                            }


sungam3r · 2022-06-19T20:35:58Z

I remember this, I will look tomorrow. Is this the last PR of the large changes in the server?

Shane32 · 2022-06-19T21:33:21Z

I remember this, I will look tomorrow. Is this the last PR of the large changes in the server?

Yes. Just samples, tests and documentation after that, which while being a lot of code/text, is not substantive in terms of functionality.

sungam3r · 2022-06-20T20:07:00Z

Supports @Skip and @include directives properly

Were they used incorrectly before? GetRecursivelyReferencedFragments ?

Works with any DI provider.

As you said This project is for ASP.NET Core so I think it does not matter.

sungam3r · 2022-06-20T20:09:30Z

I doubt a bit about removed IAuthorizationErrorMessageBuilder and IClaimsPrincipalAccessor though I understand your intent.

Shane32 · 2022-06-20T20:24:40Z

Were they used incorrectly before? GetRecursivelyReferencedFragments ?

@skip and @include were ignored previously. Note that those directives can be applied directly to a field, so it does not matter if GetRecursivelyReferencedFragments supports @skip and @include.

Below is a sample of a failing test that should pass if @skip were handled properly.

[Fact]
public void ignores_skipped_fields()
{
    ConfigureAuthorizationOptions(options =>
    {
        options.AddPolicy("ClassPolicy", x => x.RequireClaim("admin"));
        options.AddPolicy("SchemaPolicy", x => x.RequireClaim("some"));
    });

    ShouldPassRule(config =>
    {
        config.Query = @"query { post @skip(if: true) __typename }";
        config.Schema = BasicSchema<BasicQueryWithAttributesAndClassPolicy>().AuthorizeWithPolicy("SchemaPolicy");
        config.User = CreatePrincipal(claims: new Dictionary<string, string>
        {
            { "some", "true" },
        });
    });
}

src/Transports.AspNetCore/AuthorizationValidationRule.cs

sungam3r · 2022-06-22T08:13:51Z

tests/Authorization.AspNetCore.Tests/ValidationTestBase.cs

@@ -92,7 +93,8 @@ private IValidationResult Validate(ValidationTestConfig config)
            Document = document,
            Operation = document.Definitions.OfType<GraphQLOperationDefinition>().First(),
            Rules = config.Rules,
-            Variables = config.Variables
+            Variables = config.Variables,
+            RequestServices = ServiceProvider,


ServiceProvider is root service provider. Maybe create and set service provider from scope ?

Nothing registered is scoped, so it makes little difference. I will have new tests anyway.

src/Transports.AspNetCore/AuthorizationVisitorBase.Validation.cs

sungam3r · 2022-06-22T11:14:27Z

src/Transports.AspNetCore/AuthorizationVisitor.cs

+    protected IAuthorizationService AuthorizationService { get; }
+
+    /// <inheritdoc/>
+    protected override bool Authorize()


Method looks like an action to perform (verb). The same for AuthorizeRole. Do they definitely have such names?

I named them so they all start with Authorize, and so they are similar to the extension methods we have on defined within GraphQL.NET, which are Authorize, AuthorizeWithRoles and AuthorizeWithPolicy. We could name them similar to the members they map to:

bool IsAuthenticated { get; },

bool IsInRole(string role), and

AuthorizationResult Authorize(string policy)

What do you think? I'm open to suggestions and would like to do whatever makes the most sense.

For reference here is the code:

protected override bool Authorize() => ClaimsPrincipal.Identity?.IsAuthenticated ?? false; protected override bool AuthorizeRole(string role) => ClaimsPrincipal.IsInRole(role); protected override AuthorizationResult AuthorizePolicy(string policy) => AuthorizationService.AuthorizeAsync(ClaimsPrincipal, policy).GetAwaiter().GetResult();

It is bad that for all of them you can not use the name IsXXX. The last one method is verb. There is no 100% symmetry. I don't know how to do better.

bool IsAuthenticated { get; }, bool IsInRole(string role), and AuthorizationResult Authorize(string policy)

This variant at least looks reasonable.

Will do. At least it matches the methods they call (by default).

sungam3r

I looked through all changes. What can I say? Well, the previous validation rule takes about 200 lines of code and is quite understandable. The current implementation is greatly complicated and overloaded. Honestly, I almost immediately lost my desire to understand the code. I watched it not in detail and I'm sure that this code will be more difficult to maintain. In addition, I still doubt the correctness of the selected approach with building error messages (removed message builder) though I'm fine with any way to assemble those messages. Howewer, in this situation, I am pleased that despite such complexity, it is limited by only one rule of validation and does not go beyond it. See my other comments about schema coordinates and other and feel free to merge since I can't help here more reviewing/tracking control flow.

sungam3r · 2022-06-22T11:31:03Z

Of course, as a user of this rule, I would need comprehensive documentation.

Co-authored-by: Ivan Maximov <[email protected]>

Shane32 · 2022-06-22T13:33:30Z

Well, the previous validation rule takes about 200 lines of code and is quite understandable. The current implementation is greatly complicated and overloaded.

I agree. The nature of properly applying the AllowAnonymous attribute is complex to implement. I've added many comments but still it is not easy to follow. The only other thing I could do is to break it up into additional methods, but I don't think it will make it much easier to understand.

See my other comments about schema coordinates

I looked at the spec you referenced. I certainly see the value in using the spec here. I added an issue so we can come back to this. As this is a implementation detail (being an error message description) changing it after release is not a breaking change. I will review again when I have time; maybe soon.

feel free to merge

I will await your comments on naming the Authorize methods before merging. We should certainly choose the best names we can.

sungam3r · 2022-06-22T20:08:43Z

I looked at the spec you referenced. I certainly see the value in using the spec here.

Not spec itself, still RFC. Some RFCs are worked on for years without any real progress in spec :(

Shane32 · 2022-06-22T20:11:54Z

With the vast number of changes made in this project as a whole (basically a 100% rewrite), maybe it would be better to retain IClaimsPrincipal and the authorization message builder code as [Obsolete] services. At least it gives everyone a chance to open an issue if they feel there is a need or value in the old code before it is completely removed. Then we can remove in v8 if there are no issues -- or we can remove the [Obsolete] tag if there is value in those interfaces.

tests/Authorization.AspNetCore.Tests/ValidationTestBase.cs

Co-authored-by: Ivan Maximov <[email protected]>

codecov-commenter · 2022-06-22T20:19:10Z

Codecov Report

Merging #803 (46e36dd) into develop (5e6a888) will increase coverage by 0.54%.
The diff coverage is 41.06%.

@@             Coverage Diff             @@
##           develop     #803      +/-   ##
===========================================
+ Coverage    28.78%   29.33%   +0.54%     
===========================================
  Files           51       50       -1     
  Lines         1987     2127     +140     
  Branches       290      359      +69     
===========================================
+ Hits           572      624      +52     
- Misses        1330     1393      +63     
- Partials        85      110      +25

Impacted Files	Coverage Δ
....AspNetCore/Extensions/GraphQLBuilderExtensions.cs	`14.03% <0.00%> (-1.35%)`	⬇️
.../Transports.AspNetCore/AuthorizationVisitorBase.cs	`30.27% <30.27%> (ø)`
src/Transports.AspNetCore/AuthorizationVisitor.cs	`50.00% <50.00%> (ø)`
....AspNetCore/AuthorizationVisitorBase.Validation.cs	`57.25% <57.25%> (ø)`
...ansports.AspNetCore/AuthorizationValidationRule.cs	`68.75% <68.75%> (ø)`
.../Transports.AspNetCore/Errors/AccessDeniedError.cs	`88.88% <0.00%> (+88.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e6a888...46e36dd. Read the comment docs.

Shane32 · 2022-06-22T20:21:31Z

With the vast number of changes made in this project as a whole (basically a 100% rewrite), maybe it would be better to retain IClaimsPrincipal and the authorization message builder code as [Obsolete] services. At least it gives everyone a chance to open an issue if they feel there is a need or value in the old code before it is completely removed. Then we can remove in v8 if there are no issues -- or we can remove the [Obsolete] tag if there is value in those interfaces.

Another option would be to update the old nuget package GraphQL.Server.Authorization.AspNetCore to use the new code here, inheriting all of the new features, but still retaining (within that package) the IClaimsPrincipal and message builder code. The entire package can be marked as deprecated, but for the immediate transition to v7 it will be a drop-in replacement. I anticipate no changes to the code within this GraphQL.Server.Transports.AspNetCore package at all. Note that since GraphQL.NET's INodeVisitor is changing for v7, any old validation rules will not work anymore. So users cannot use GraphQL.Server v6 authorization rule with GraphQL.NET v7.

sungam3r · 2022-06-22T20:23:09Z

Do you mean something like [Obsolete("This type is not used anymore now and will be removed in v8. Instead use XXX. If you think you really need it, then feel free to open an issue to return it back")] ?

Shane32 · 2022-06-22T20:23:33Z

Do you mean something like [Obsolete("This type is not used anymore now and will be removed in v8. Instead use XXX. If you think you really need it, then feel free to open an issue to return it back")] ?

Sure; something like that.

sungam3r · 2022-06-22T20:26:42Z

Another option would be to update the old nuget package GraphQL.Server.Authorization.AspNetCore

I would not

Replace ASP.Net Core validation rule

9e95e01

github-actions bot added the test label May 21, 2022

Updates

f07bc7d

Shane32 changed the base branch from master to develop May 21, 2022 19:33

Shane32 added 5 commits May 21, 2022 18:18

Refactor & code splitting

e173d7c

Update

1a4b050

Add comments

faf9ff9

Add a whole lot of comments

04578d8

Update

bdacff1

Shane32 self-assigned this May 22, 2022

Merge remote-tracking branch 'origin/develop' into authorization

c78bd28

Shane32 added enhancement New feature or request BREAKING Breaking changes in either public API or runtime behavior and removed test labels May 22, 2022

Update

c79b327

Shane32 marked this pull request as ready for review May 22, 2022 06:21

Shane32 requested a review from sungam3r May 22, 2022 06:21

Shane32 added this to the v7 milestone May 22, 2022

Update

a32fb91

Merge from develop

d5807a9

github-advanced-security bot found potential problems Jun 19, 2022

View reviewed changes

Updates

b7e083d

github-advanced-security bot found potential problems Jun 19, 2022

View reviewed changes

Merge branch 'develop' into authorization

3c51a59

sungam3r reviewed Jun 22, 2022

View reviewed changes

src/Transports.AspNetCore/AuthorizationValidationRule.cs Outdated Show resolved Hide resolved

sungam3r reviewed Jun 22, 2022

View reviewed changes

src/Transports.AspNetCore/AuthorizationVisitorBase.Validation.cs Outdated Show resolved Hide resolved

sungam3r reviewed Jun 22, 2022

View reviewed changes

src/Transports.AspNetCore/AuthorizationVisitorBase.Validation.cs Outdated Show resolved Hide resolved

sungam3r reviewed Jun 22, 2022

View reviewed changes

src/Transports.AspNetCore/AuthorizationVisitorBase.Validation.cs Show resolved Hide resolved

sungam3r reviewed Jun 22, 2022

View reviewed changes

Shane32 and others added 2 commits June 22, 2022 08:54

Update src/Transports.AspNetCore/AuthorizationVisitorBase.Validation.cs

6d3bb17

Co-authored-by: Ivan Maximov <[email protected]>

Update src/Transports.AspNetCore/AuthorizationValidationRule.cs

b77a68a

Co-authored-by: Ivan Maximov <[email protected]>

Shane32 mentioned this pull request Jun 22, 2022

Consider using schema coordinates syntax for access denied errors #835

Open

Update xml comments

b7a16ca

Shane32 added 2 commits June 22, 2022 16:13

Rename Authorize methods

6f84b7e

Change method to get prop

22b295a

sungam3r reviewed Jun 22, 2022

View reviewed changes

tests/Authorization.AspNetCore.Tests/ValidationTestBase.cs Outdated Show resolved Hide resolved

Update tests/Authorization.AspNetCore.Tests/ValidationTestBase.cs

46e36dd

Co-authored-by: Ivan Maximov <[email protected]>

sungam3r approved these changes Jun 22, 2022

View reviewed changes

Shane32 merged commit a80ad6a into develop Jun 22, 2022

Shane32 deleted the authorization branch June 22, 2022 22:38

This was referenced Jun 23, 2022

Add skip and include support for fragment spreads and inline fragments #837

Merged

Move builder methods to GraphQL namespace #840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace authorization project with new rule #803

Replace authorization project with new rule #803

Shane32 commented May 21, 2022 •

edited

Loading

Shane32 commented Jun 15, 2022

Shane32 commented Jun 19, 2022

github-advanced-security bot left a comment

sungam3r commented Jun 19, 2022

Shane32 commented Jun 19, 2022

sungam3r commented Jun 20, 2022

sungam3r commented Jun 20, 2022

Shane32 commented Jun 20, 2022

sungam3r Jun 22, 2022

Shane32 Jun 22, 2022

sungam3r Jun 22, 2022

Shane32 Jun 22, 2022

sungam3r Jun 22, 2022

Shane32 Jun 22, 2022

sungam3r left a comment •

edited

Loading

sungam3r commented Jun 22, 2022

Shane32 commented Jun 22, 2022 •

edited

Loading

sungam3r commented Jun 22, 2022

Shane32 commented Jun 22, 2022

codecov-commenter commented Jun 22, 2022

Shane32 commented Jun 22, 2022

sungam3r commented Jun 22, 2022

Shane32 commented Jun 22, 2022

sungam3r commented Jun 22, 2022

Replace authorization project with new rule #803

Replace authorization project with new rule #803

Conversation

Shane32 commented May 21, 2022 • edited Loading

Shane32 commented Jun 15, 2022

Shane32 commented Jun 19, 2022

github-advanced-security bot left a comment

Choose a reason for hiding this comment

sungam3r commented Jun 19, 2022

Shane32 commented Jun 19, 2022

sungam3r commented Jun 20, 2022

sungam3r commented Jun 20, 2022

Shane32 commented Jun 20, 2022

sungam3r Jun 22, 2022

Choose a reason for hiding this comment

Shane32 Jun 22, 2022

Choose a reason for hiding this comment

sungam3r Jun 22, 2022

Choose a reason for hiding this comment

Shane32 Jun 22, 2022

Choose a reason for hiding this comment

sungam3r Jun 22, 2022

Choose a reason for hiding this comment

Shane32 Jun 22, 2022

Choose a reason for hiding this comment

sungam3r left a comment • edited Loading

Choose a reason for hiding this comment

sungam3r commented Jun 22, 2022

Shane32 commented Jun 22, 2022 • edited Loading

sungam3r commented Jun 22, 2022

Shane32 commented Jun 22, 2022

codecov-commenter commented Jun 22, 2022

Codecov Report

Shane32 commented Jun 22, 2022

sungam3r commented Jun 22, 2022

Shane32 commented Jun 22, 2022

sungam3r commented Jun 22, 2022

Shane32 commented May 21, 2022 •

edited

Loading

sungam3r left a comment •

edited

Loading

Shane32 commented Jun 22, 2022 •

edited

Loading