Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

feat: refactor directives, add auth #8

Merged
merged 5 commits into from Jul 5, 2019
Merged

Conversation

darahayes
Copy link
Contributor

This PR does the following.

  • Moves the complex logic from the hasRole SchemaDirectiveVisitor into a more simplified directiveResolvers map that follows a middleware style approach. This will ultimately make our directives compatible with graphql-modules
  • Adds an @auth directive. This will support cases where the entire /graphql endpoint is not protected and users want to add auth to individual fields/mutations/queries
  • tests for the auth directive
  • moves tests all into one folder.

Next steps (in another PR) will be:

  • Completely remove the SecurityService interface
  • Expose directives and context provider directly
  • examples and tooling to help users see the various use cases in action

@darahayes darahayes requested a review from wtrocki July 5, 2019 15:27
@darahayes
Copy link
Contributor Author

@wtrocki code coverage is being reported as a lower percentage because the originally the very large test files were being included in the percentage which drove it up. This number is the accurate number.

@wtrocki
Copy link
Contributor

wtrocki commented Jul 5, 2019

@darahayes I was just having a laugh :)

Copy link
Contributor

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

Love the idea and future plans. Did not verified

@darahayes darahayes merged commit 06e0034 into master Jul 5, 2019
@darahayes darahayes deleted the directive-refactor branch July 5, 2019 17:20

let foundRole = null // this will be the role the user was successfully authorized on

foundRole = roles.find((role: string) => {
Copy link
Contributor

@wtrocki wtrocki Jul 6, 2019

Choose a reason for hiding this comment

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

Actually this will be vulnerable to code swapping. Using classical for loop may be more secure as Array.find is global.
Request or context aren't so easy to swap.

@renovate renovate bot mentioned this pull request Jun 15, 2020
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants