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

Initial support for GraphQL path guards #650

Closed
wants to merge 6 commits into from

Conversation

jmpunkt
Copy link
Contributor

@jmpunkt jmpunkt commented May 4, 2020

Overview

Juniper should support authenticated paths in some way. Instead of using external macros, this PR provides trait based guards which are evaluated before the actual field is resolved. If the guard fails, the whole evaluation fails and an FieldError is returned. Besides allowing early aborts, it is also possible to change the context. For example, if the user is in a privileged group, then we can exchange the context. The new context contains additional information and it could be ensured that no re-check of the same permission happens again.

How does the context change work?

Each object defines their context. The guards may require a specific context. If both contexts are compatible, then the guard can be used. The guard also defines how the output context look like, its is still possible to forward the old context. To annotate a field use #[graphql(Guard = "..")]. Notice that guards are only support for object fields, at least for now.

Authentication

With this PR it is possible to define permission systems on top of Juniper. The current testing cases implement a basic role-based authentication system. Take a look at the source to get a feeling how this feature is used.

Related

Progress

  • basic implementation
  • combinators (and_then)
  • sync support?
  • tests
  • documentation for this features
  • documentation for this authentication

@jmpunkt jmpunkt marked this pull request as ready for review May 7, 2020 19:51
@jmpunkt
Copy link
Contributor Author

jmpunkt commented May 7, 2020

Not fully done yet. Tests are missing, docs are not perfect, and the book examples do not compile. However, the basic constructs should be established. Would love to hear some initial feedback about this feature.

The only question which is is not solved now is: how to handle non-async. For now I ignored sync code since it should be removed anytime soon. As a result, the guards only support async resolvers and only provide async interface.

@LegNeato
Copy link
Member

My main concern here (and a lot of the other outstanding PRs) is this is making Juniper more opinionated and less of a library and more of a framework.

@jmpunkt
Copy link
Contributor Author

jmpunkt commented May 16, 2020

Not sure about this. The whole proc macros are kind of a framework which uses the underlying library? This PR modifies the code generation in a way which allows to "intercept" the evaluation. Maybe you can expand what you expect, or if such a feature should be possible or not.

The general problem I see right now, is that it is not possible to have custom attribute macros. The order of macros is not specified, thus we can not ensure that their macro is called before. Furthermore, the user must define two procs, like we do, for impl blocks and structs. As a result, proc macros can not be used as a plugin architecture for fields. In the end, this means the user has to use the #[graphl_object] macro for all structs, if the fields must be must run custom logic before the actual evaluation.

@ccbrown
Copy link
Contributor

ccbrown commented Jul 28, 2020

To quote the Best Practices page on graphql.org for Authorization:

Enforcing this kind of behavior should happen in the business logic layer.

The page goes on to give examples and details that make it pretty clear that defining permissions systems on top of Juniper is not recommended.

That being the viewpoint of the GraphQL Foundation (and of myself), I don't think this sort of functionality should be considered.

@LegNeato
Copy link
Member

Agreed! If Juniper doesn't have the proper hooks, please open an issue on the missing extension / integration point.

@LegNeato LegNeato closed this Jul 28, 2020
@Dispersia
Copy link
Contributor

@ccbrown are you able to name another graphql library that doesn't? Apollo Server does, HotChocolate does, graphql-dotnet does, prisma recommends to use apollo servers auth, etc etc.

Just saying, that spec typically doesn't follow reality, with every company (besides facebook apparently) even versioning their apis: https://developer.github.com/v4/breaking_changes/ .

Would it be happier if this was a sister package? I would argue this is a basics that comes with most other libraries. Just giving my experience from using other libraries, if this just isn't wanted that's completely in your right, but it would definitely lead to a lower adoption.

@ccbrown
Copy link
Contributor

ccbrown commented Aug 10, 2020

The most popular / comparable library for Golang, graphql-go/graphql, does not have any support for this. The only other popular one I've actually used at scale is samsarahq/thunder, which also does not have any support for this. Without looking, my gut says it would not be very hard at all to find more and this probably isn't the kind of situation where lack of the feature is going to be a significant burden to adopters.

This might be reasonable as a sister package, but I would guess the ideal solution here is really to just implement custom directives support (#156). That would make Apollo Server-like authorization via custom directives possible without making Juniper more opinionated (and would enable a plethora of other useful things).

@felipellrocha
Copy link

My main concern here (and a lot of the other outstanding PRs) is this is making Juniper more opinionated

The irony being, that explicitly not supporting auth, Juniper does, indeed, become more opinionated... By definition...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants