IAuthorizationSkipCondition feature

[sungam3r]

Demonstration of fix for #28. @Shane32 Please review. I would like to merge after #184

[codecov-commenter]

Codecov Report

Merging #186 (8d491c0) into develop (12fb6f6) will decrease coverage by 0.89%. The diff coverage is 78.72%.

@@             Coverage Diff             @@
##           develop     #186      +/-   ##
===========================================
- Coverage    83.81%   82.91%   -0.90%     
===========================================
  Files            9       10       +1     
  Lines          278      322      +44     
  Branches        46       59      +13     
===========================================
+ Hits           233      267      +34     
- Misses          33       37       +4     
- Partials        12       18       +6     

Impacted Files	Coverage Δ
...raphQL.Authorization/IntrospectionSkipCondition.cs	62.96% <62.96%> (ø)
...aphQL.Authorization/AuthorizationValidationRule.cs	95.58% <100.00%> (+0.63%)	:arrow_up:

---

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 12fb6f6...8d491c0. Read the comment docs.

[Shane32]

It looks like this code only serves a single purpose -- to skip authorization checks of the entire AST. I would prefer more robust functionality where one could mark fields as anonymous -- then if only nodes marked as anonymous were selected, the AST would be approved. Introspection fields could [optionally] be implicitly marked as anonymous. In this manner the same feature set could support this requested feature and also the anonymous-fields requested feature.

If you really think we should merge this, okay, but it seems that supporting the other feature would also support this feature.

[Shane32]

I have no comments on the code itself. However, my knowledge of GraphQL.Authorization is limited, as I do not use it in my production libraries.

[Shane32]

It is also possible that the framework in this PR could support the other feature. In this case, let's do so. It's possible that with very little effort this PR could be changed so that in addition to identifying introspection fields, it could identify 'anonymous' fields also. Then, so long as only anonymous or introspection fields were requested, authentication would be skipped.

[Shane32]

Why does this feature target develop? There's nothing specific to version 5, is there? I think this should be added to v4 if it is merely an additional authorization rule.

[sungam3r]

Maybe. It is easy to do so by writing another implementation of IAuthorizationSkipCondition. I specifically did so that there was a space for further work. The introspection does not depend on any anonymous auth stuff. I would not mix concepts.

Why does this feature target develop?

No specific reason when I started. I think I did so because of ValueTask in ValidateAsync.

[Shane32]

But as it is now, if we wrote a separate IAuthorizationSkipCondition which checked if only nodes marked with 'AllowAnonymous` (or similar) were selected, then it would fail if they also selected introspection fields. The apollo graphql client library typically does this, which is a major problem for implementing these 'skip conditions' separately. Rather, any new skip condition would need to reimplement all of the the checks of previous skip conditions.

I really think the design needs to be re-thought here a bit. Perhaps it should be set up so that there is an interface for checking the skip condition of a specific node; then multiple implementations can be registered that will collectively identify if authorization checks for the entire AST should be skipped.

[Shane32]

It would be really neat if the introspection filter could be tied to this code and the authorization library, perhaps as an optional setting, so only authorized fields would be returned in the introspection query. But this may (or may not) require changes to GraphQL.NET. This of course could be implemented at a later time.

[sungam3r]

then it would fail if they also selected introspection fields

Right because these are independent things.

any new skip condition would need to reimplement all of the the checks of previous skip conditions.

Well, not really, but I'll think about it.

only authorized fields would be returned in the introspection query

I would not do that. Otherwize with a big caution. This is a clear intention, but it will lead to difficulties for the client. How does the client find out about the existence of such/any fields? The whole point of receiving the schema throught introspection is to learn some requirements for it. In my applications, authorization requirements are made through directives in the schema. The client is not necessary to be authorized in one way or another in order to obtain information that the systemLog field is available only for admins (@auth(roles: ["admin"]) directive).

[sungam3r]

IAuthorizationSkipCondition/ShouldSkip

OK with naming?

[Shane32]

then it would fail if they also selected introspection fields

Right because these are independent things.

We need a solution here.

only authorized fields would be returned in the introspection query

I would not do that.

Nevertheless, this is a requested feature. I would use this for my own needs as well. You have to understand that the goal is different. In many applications it is possible or likely for any user to have elevated permissions, and the API is intended to be public. But in this case it is specifically to prevent a public client from any knowledge that a private API exists.

For example, an e-commerce site may expose a GraphQL API to allow its SPA access to retrieve its products. The GraphQL API may also be used by its admin site to make changes to the site. However, exposing the schema of all of the many mutations needed to operate an admin site to the public is a security issue, even if it is properly secured. (It could also be seen as a unintended exposure of private intellectual property if the admin site is designed in-house.) Further, GraphiQL and other development tools can send authenticated requests even for introspection queries. So securing introspection requests is not typically a problem for development of the admin site.

Other fields in which this would be important is banking and government.

IAuthorizationSkipCondition/ShouldSkip

OK with naming?

That depends on the solution necessary to solve the above issues. Once we have an acceptable solution, it will be easy to review the naming and determine if it suits the classes/interfaces.

The more I think about this PR, the more this looks like a single-purpose PR that cannot be extended to suit the needs of the user. As such, I think the design needs to be revised before being merged. Or it needs to be revised to support fields marked as anonymous in addition to introspection fields, and it can remain a single-purpose PR that supports those features.

[Shane32]

@sungam3r Have you thought about this any more?

[sungam3r]

Yes, I thought for a while. I came up with something, but I already forgot. I will need to re-read everything I did. Closer to April I will do it.

[Shane32]

The metadata now exists in 5.1.1 to allow skipping authentication of specific parts of a request, be it introspection requests or public fields on a protected graph. We can review my code in GraphQL.AspNetCore3 as a template to use here, if you like, but be aware that my code does not perform any checks on input fields. While writing my implementation, I found that the input-field validation code already present in this repo and the server repo was incomplete, and I would rather have a feature missing than a broken feature. Specifically, a validation check for MyInputGraph when used as an input variable would not check validation on any child input graphs of MyInputGraph. There's no reason why this couldn't be implemented, but I have not yet done so. I plan to write code such that any input variable validation check performs recursive checking of all fields on itself and child graphs. Still not ideal, but better to err in the side of security than insecurity. Literals of course will be easy to check. I do not plan to support AllowAnonymous for input fields.