Skip to content
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

Updated AuthorizationValidationRule to skip authorization checks when… #30

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

chris-nissen
Copy link

… the field is not included or skipped due to directives.

@chris-nissen
Copy link
Author

If there's a more elegant way to get the argument values from the validation context, let me know.

Copy link
Member

@joemcbride joemcbride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding some tests for each directive? Probably one each of where it should exclude the field that has a policy and it passes. And another where it should include the field and it hits the validation?

src/GraphQL.Authorization/AuthorizationValidationRule.cs Outdated Show resolved Hide resolved
src/GraphQL.Authorization/AuthorizationValidationRule.cs Outdated Show resolved Hide resolved

var operation = !string.IsNullOrWhiteSpace(context.OperationName)
? context.Document.Operations.WithName(context.OperationName)
: context.Document.Operations.FirstOrDefault();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Operation and Variables will be the same for both calls, so could pull that up so it isn't calculated twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted the operations to a variable. I wasn't sure what you meant by the Variables here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExecutionHelper.GetVariableValues is called in the GetDirectiveValue method. That only needs to be calculated once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've made that change.

@chris-nissen
Copy link
Author

I'm not sure if this is enough for tests; I tried doing them so that the directive conditions came from inputs set up on the test config, but when the rule was pulling the argument values they weren't there, so I'm not sure if there's something more I'd need to do in the tests...

@chris-nissen
Copy link
Author

I wonder if I should also be accounting for these directives in the ObjectField handler?

@chris-nissen
Copy link
Author

@joemcbride, sorry, I don't know, should I be notifying you I made changes, somehow? I understand you might just not have had time to get back to this.

@joemcbride
Copy link
Member

Yes should probably be handling this on the ObjectField as well. My apologies, I've gotten slammed with work at my day job.

@chris-nissen
Copy link
Author

As for the ObjectField, I'm not really sure what the difference is between that and a Field. Do ObjectFields have directives? There's no corresponding Directives property on them.

@sungam3r sungam3r self-assigned this Mar 22, 2021
@sungam3r sungam3r added the enhancement New feature or request label Mar 22, 2021
@sungam3r sungam3r added this to the 4.0 milestone Mar 22, 2021
@sungam3r sungam3r modified the milestones: 4.0, 4.1 Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants