-
Notifications
You must be signed in to change notification settings - Fork 163
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
Add skip and include support for fragment spreads and inline fragments #837
Conversation
src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedFragments.cs
Fixed
Show resolved
Hide resolved
src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedFragments.cs
Fixed
Show fixed
Hide fixed
@sungam3r I would like to merge this in before release of v7 to complete the authorization rule. If we decide to update |
@sungam3r What's the ETA here? |
I'm going to process email on the weekend. |
Anytime you're ready. #838 is merged and tests have been added here. |
Codecov Report
@@ Coverage Diff @@
## develop #837 +/- ##
===========================================
+ Coverage 85.43% 85.75% +0.31%
===========================================
Files 60 61 +1
Lines 2239 2289 +50
Branches 366 375 +9
===========================================
+ Hits 1913 1963 +50
- Misses 273 274 +1
+ Partials 53 52 -1
Continue to review full report at Codecov.
|
src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedFragments.cs
Outdated
Show resolved
Hide resolved
src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedFragments.cs
Outdated
Show resolved
Hide resolved
src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedFragments.cs
Outdated
Show resolved
Hide resolved
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.
Reviewed. v7 is closer
Fixes the known issue in #803 where skipped fragments are still authenticated.
Because
TypeInfo.GetRecursivelyReferencedFragments
returns all recursively referenced fragments, not just ones that are not skipped,GetRecursivelyReferencedFragments
was re-implemented here. It is mostly a copy of the code within GraphQL.NET, but it does callAuthorizationVisitorBase.SkipNode
to determine if a node should be skipped, the same virtual method called by the rest of the authorization logic to determine if a node should be skipped.Note that to meet spec, I believe that the existing validation rules in GraphQL.NET requires the existing behavior of
GetRecursivelyReferencedFragments
.This PR mirrors Shane32/GraphQL.AspNetCore3#50 which includes tests.
Note: turn off “show whitespace differences” to reduce the diff in GitHub