search

exAspArk / graphql-guard

Simple authorization gem for GraphQL :lock:
MIT License
472 stars 36 forks source link

Changing NotAuthorizedError to Be A Subclass of GraphQL::ExecutionError #30

Closed sshaw closed 5 years ago

sshaw commented 5 years ago

Yes, breaking change but I was thinking with NotAuthorizedError as a superclass one gets proper error response formatting for free, as GraphQL Ruby will insert the exception's message into the "errors" property of the response.

And related, how do you feel about changing the default message of NotAuthorizedError from Query.queryName to Not authorized to access: Query.queryName (or similar)?

I just started using this and saw 500 errors with a log message of "Query.queryName". Not a helpful message.

ghost commented 5 years ago

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

exAspArk commented 5 years ago

Hey @sshaw,

Thank you for opening the issue!

with NotAuthorizedError as a superclass one gets proper error response formatting for free, as GraphQL Ruby will insert the exception's message into the "errors" property of the response.

These are 2 different behaviours: either fail the whole request with a Ruby exception (and return a 403 HTTP status, for example) or fail gracefully by returning a response with a GraphQL error. Although both of these use cases are being used, probably returning a GraphQL error is a more popular option (exactly what you suggested). For now, I'd prefer not making this breaking change since there is a way to customize the behaviour.

how do you feel about changing the default message of NotAuthorizedError from Query.queryName to Not authorized to access: Query.queryName (or similar)?

👍 good idea! Feel free to open a PR :)

sshaw commented 5 years ago

PR #32 opened.

These are 2 different behaviours: either fail the whole request with a Ruby exception (and return a > 403 HTTP status, for example) or fail gracefully by returning a response with a GraphQL error.

This is the problem (if I recall): a 403 is not returned. The exception is not handled and an HTTP 500 is returned.

exAspArk commented 5 years ago

Yeah, right. I usually use something like rescue_from from Rails to catch a specific exception (e.g. NotAuthorizedError) and return 403.

I merged #32. Thank you for your contribution! 🙌