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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for a 'silent' mode #988

Open
jgnieuwhof opened this issue Nov 12, 2020 · 12 comments
Open

Add support for a 'silent' mode #988

jgnieuwhof opened this issue Nov 12, 2020 · 12 comments

Comments

@jgnieuwhof
Copy link

Feature request

Firstly, thanks for the great library and continued support 馃檹.

My current team has encountered a situation where it would be preferable to allow authentication to fail silently, returning null rather than an Error.

It's currently possible to do this by passing { fallbackError: null }, however it's neither TypeScript compliant nor officially supported.

I'm happy to make a PR with the change, if / when a solution is decided upon.

Describe the solution you'd like

Add a new option, { silent: boolean }, informing the middleware resolver to return null if authorization fails.

Describe alternatives you've considered

Allow fallbackError to accept null.

@open-collective-bot
Copy link

Hey @jgnieuwhof 馃憢,

Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider contributing financially.

https://opencollective.com/graphql-shield

PS.: We offer priority support for all financial contributors. Don't forget to add priority label once you start contributing 馃槃

@maticzav
Copy link
Owner

Hey 馃憢 ,

Thank you for your kind words. I think this could be a great addition to the library. I am wondering, though, if clients don't let you ignore errors by default? I think that Apollo Client and the like raise errors by default, but you can change that so that you get the null value back. Do you think that would solve your problem?

@jgnieuwhof
Copy link
Author

馃憢 thanks for the quick response!

Unfortunately the specific issue I've encountered involves Apollo federation, where you have a single graph (gateway) comprised of one or more services. When the gateway encounters a downstream service error the partial data response from that service is ignored. This is a situation where we care far more about the partial data response than the details of the authorization-related downstream error.

Aside from this though - in general I've gained a preference for treating a lack of access as a lack of data. It's inline with how most DB RBAC systems work (queries will silently return only the data that you have access to), and is easy to handle on the front-end (don't render data that isn't there). I suppose there are also situations where it's a security flaw to admit there's data there at all, an example of this might be GitHub's 404 response when trying to visit a repository that you don't have access to.

I guess there's a few thoughts there 馃槄, thanks in advance for taking the time to read 'em.

@codepunkt
Copy link

@jgnieuwhof Sorry for hi-jacking your issue. Just out of interest - how are you using graphql-shield with federation? do you use it on all individual federated services?

@jgnieuwhof
Copy link
Author

@codepunkt - no sweat, yeah using it individually on each schema. There were two things I did to make it play well with federation.

  1. allow all types / fields in the federation schema spec.

  2. My team is using code-first Nexus for our schema definitions. I wrapped graphql-shield's Rule to add a requires directive to the rule configuration object. This allows individual rules to "require" fields if the rule is placed on an extension type, which get merged into a single requires directive on any applicable field when the schema is compiled.

@maticzav
Copy link
Owner

maticzav commented Jan 7, 2021

Thank you for hijacking this! It slipped out of my radar. @jgnieuwhof I have a very similar view on how permissions work - I like this kind of thinking. 馃檶馃徔

Besides that, let's implement the silent option. If you could create a PR with tests and short documentation, I could merge it as soon as tomorrow.

@jgnieuwhof
Copy link
Author

@maticzav - great to hear! Not sure if I'll find time today, but I should have a cycle to work on this within the next week or two.

@stale
Copy link

stale bot commented Jun 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 11, 2021
@labelsync-manager labelsync-manager bot removed the stale label Jun 11, 2021
@esseswann
Copy link

I would recommend also an ability to return custom GraphQL type. There is a certain tendency in the GraphQL developemnet to disregard error responses because they're problematic to add strict typing to, so people prefer schemas like this:

type Query {
  users: Users
}

union Users = Error | PaginatedUsers

type Error {
  kind: ErrorsEnum
  message: String!
}

enum ErrorsEnum {
  UNAUTHORIZED
}

type PaginatedUsers {
  count: Int!
  items: [User]
}

type User {
  id: ID
}

@maticzav maticzav added this to To do in GraphQL Shield v8 via automation Nov 8, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@labelsync-manager labelsync-manager bot removed the stale label Apr 16, 2022
@forrestwilkins
Copy link

@esseswann Have you found any sort of work around in order to use custom error types? I'm attempting to do exactly what you described.

@esseswann
Copy link

@forrestwilkins As of now we had to manually handle the errors and convert them to custom types

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

No branches or pull requests

5 participants