-
-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auth engine rework #128
base: master
Are you sure you want to change the base?
Auth engine rework #128
Conversation
I'll need a little time to look at it. Hopefully I can do so tonight. |
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
===========================================
+ Coverage 80.84% 93.12% +12.28%
===========================================
Files 10 17 +7
Lines 214 291 +77
Branches 32 42 +10
===========================================
+ Hits 173 271 +98
+ Misses 32 13 -19
+ Partials 9 7 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I write all my own authentication code, both for ASP.Net code and GraphQL, so I don't have much input here. I did look through all the files, but it's a bit much to take in.
Co-authored-by: Shane Krueger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super involved these days, but I do have some design concerns with these changes.
.Evaluate(userContext?.User, context.UserContext, context.Inputs, provider.GetPolicies()) | ||
.GetAwaiter() | ||
.GetResult(); | ||
if (policyNames?.Count == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like all of these checks are in the wrong class. This was previously the responsibility of the evaluator - to evaluate the success of the policies. Now that code has moved to this class. I think this code belongs in the Authorization Service, not this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What checks? if (policyNames?.Count == 1)
? It's just a small optimization. Authorization Service authorizes a single policy. I see no code there that can be moved inside Authorization Service.
This was previously the responsibility of the evaluator - to evaluate the success of the policies.
I know. In case of the custom evaluator, the client had to write more code to create and fill AuthorizationContext
, iterating through all tasks, etc. I changed the architecture so that it was more flexible. In many ways, it corresponds to how this is done in ASP.NET Core. I understand that the initial design was taken from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is looping over each of the policies and building up the validation result for each policy, which was previously the responsibility of the the Authorization framework. That responsibility was moved to this GraphQL class instead. This GraphQL class should call into the Authorization framework, it should not BE the framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So AuthorizationValidationRule
is not a framework class in your opinion. Right? Does this suit you if I will simply move all three dependences from rule ctor in one new dependency (evaluator) as it was before? Of cource I'll add then DafaultAuthorizationEvaluator
with these three dependencies in its ctor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This GraphQL class should call into the Authorization framework, it should not BE the framework.
And just a note - I thought that since the AuthorizationValidationRule
class is located in the GraphQL.Authorization
project among other classes, then it is an integral part of this framework. I do not understand what rule you use to decide which classes are included in the framework and what is not.
/// </summary> | ||
/// <param name="context">GraphQL validation context.</param> | ||
/// <param name="policyName">Name of checked policy for the current authorization processing.</param> | ||
protected virtual IAuthorizationContext CreateAuthorizationContext(ValidationContext context, string policyName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code can also be moved to the Authorization Service. That would remove both the IClaimsPrincipalAccessor
and the IAuthorizationPolicyProvider
out of this validation rule class, which was intended to be a simple wrapper around the authorization framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authorization Service uses provided context and does not create it. This is the whole point. Otherwise, it will solve several tasks.
which was intended to be a simple wrapper around the authorization framework.
Now it not much more difficult:
new AuthorizationValidationRule(new DefaultAuthorizationService(), new DefaultClaimsPrincipalAccessor(), new DefaultAuthorizationPolicyProvider(Settings));
I agree that dependencies appeared but it rather says that they were before, only in implicit form and they could not be replaced independent of each other. Now it can. Authorization evaluator tasks before these changes:
- Create auth context.
- Pick policy by name from settings.
- Authorize policy.
- Iterate over all policies.
- Create AuthorizationResult.
Too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was the entire purpose of the Evaluator class, to handle all of those things so the Validation Rule, which is a GraphQL Concept, does not have to. The previous design you could use this Authorization framework in any application, it had no direct GraphQL dependencies besides the Validation Rule. The Validation Rule was just the entry point to the Authorization framework. This moves a lot of the actual Authorization logic into the GraphQL class, which tightly couples it to the GraphQL framework. You can no longer use a single class outside of GraphQL to get the full Authorization Result, it is now dependent on the Validation Rule to handle evaluating policies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous design you could use this Authorization framework in any application
But isn't it still possible to use Authorization framework in any application?
tightly couples it to the GraphQL framework
I look at it on the other side. Tightly coupling factor did not increase. It already has already been all this time inside the code. My changes made some dependencies explicit. Before that, they were hidden.
… auth # Conflicts: # README.md # src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.approved.txt # src/GraphQL.Authorization/GraphQLAuthorizeAttribute.cs
This PR adds many new classes and interfaces to unify authorization architecture with such from the server project. Also adds APIs from graphql-dotnet/server#480 to build custom errors and messages.
Many classes are similar to those from ASP.NET Core (but not all). Of course, the project still does not have any dependencies from ASP.NET Core bits.
fixes #127
fixes #121