Why perform validation method before authorization verification ?

[illambo]

Versions:

• graphql-laravel Version: 2.1
• Laravel Version: 5.8
• PHP Version: 7.1

Question:

About issue resolved in #407 in my opinion it was better to check first authorize method and than perform validation rules, this prevents any superfluous and possibly redundant validation queries.

For example Rule::exists check in db if the input data exists, even if I might not have permission to do this. I think this generates bypass the acl controls.

What do you think ? Where I'm wrong ?
Thanks

[illambo]

Ping for @rebing and @mfn
Thanks

[rebing]

There are pros and cons with both implementations. If we perform authorization before validation then there might be inputs that should not be there.

Therefore, implementing this method will just replace one problem with another.

[illambo]

If I remember correctly in classic laravel http request usually the authorization it is performed before the input validations.
Maybe is it possible implement the "order change" behavior with some configuration option ?

[rebing]

Yes, Laravel authorizes before validation. No, we should not make it more complicated by providing a configuration option.

I'm on the fence about which method to use, so let's wait for @mfn to provide his input. He's on vacay right now 🌴

[dungfv]

@rebing Can $context be added as a second argument to the authorize function?

[rebing]

@fvdung Yes, but it would be a breaking change

[mfn]

If I remember correctly in classic laravel http request usually the authorization it is performed before the input validations.

This is not unambiguously clear.

If we stick to Laravel, what you define in your middleware is the authentication, which we can thus say it's always performed before any validation.

Inside your request, you then usually validate your request data and then decide if the request is authorized. Because it's not unreasonable to expect to have to use the request data to actually figure out if you are allowed to access something.

In other words: for non trivial tasks, you need to consider the input request to decide if a user is allowed to do something ("authorization"), hence validation before authorization makes sense.

Can $context be added as a second argument to the authorize function?

This sounds reasonable to me; I was thinking about this with all the refactoring lately when I touched the code surrounding it but haven't gotten around doing it.

Yes, but it would be a breaking change

Are we sure? I'd love following semver with this package, but adding a new argument which is simply ignored for existing code shouldn't be a problem, or is it? If so, I'd love to get some example code to better understand this, thanks 😄

My TL;DR

• I think validation before authorization is the better default / practice; if you need / want it differently, don't use getRules/authorization but roll your own code in the resolve
• Adding the the context seems reasonable!

[rebing]

@mfn Nice write up, I guess it's decided that we stay with validation before authorization as it is for now.

Regarding add context, I'm not sure yet how we could do it without a breaking change. We have to modify Rebing\GraphQL\Support\Field row 191:

if (call_user_func($authorize, $arguments[1]) != true) {

to

if (call_user_func($authorize, $arguments[1], $arguments[2]) != true) {

where $arguments[2] is context. However this means we also have to modify the authorize(array $args): bool function to authorize(array $args, array $ctx): bool and all classes that inherit this function have to make this change as well.

If we remove the function from Fields altogether then it needs to be added to every Query and Mutation.

Perhaps someone has a better idea?

[mfn]

However this means we also have to modify the authorize(array $args): bool function to authorize(array $args, array $ctx): bool

🤦‍♂️

Absolutely correct, it's a breaking change. Totally forgot the contract.

There was never any talk (AFAIK) about semver until I mentioned it today.

Personally I'd really love do it, which in fact means: the next release would be 3.0

But there was never a discussion if this project should even do it, if we all want it, etc.

Any feelings about this?

FTR: https://semver.org/

[rebing]

I think that we should follow semver guidelines and release 3.0 as the next version. Unless there is a better way of handling the authorization without a breaking change.

[dungfv]

I think adding the $context variable in the authorize() function still has a lot of issues to discuss. This has not yet resolved the actual issue of authorization.
Imagine, sometimes, authorization needs to be done with certain objects in the resolve() function (need to query in the database).
Then, to follow the current pattern, we will have to duplicate the query.

An idea for this is to build an authorization function similar to $this->authorize($ability, $arguments) in Illuminate controller (The essence is the Policy)

[mfn]

I would wager in this case, don't use the authorize method.

It's really nothing more a convenience method to throw a specific exception; I would say anything leaving that "comfort zone" should simply just implement resolve and handle it application specific.

Then, to follow the current pattern, we will have to duplicate the query.

That's exactly my experience and for that reason, in non-trivial projects I can't use it.

Authorization is so much baked into the (my) application, I can't simply extract it into a separate step like suggested by rebing

[illambo]

I think validation before authorization is the better default / practice; if you need / want it differently, don't use getRules/authorization but roll your own code in the resolve.

Ok thanks for the clarifications, I try with the resolve way because in some scenarios
with multi-tenant applications (with single schema) it does not allow a clean privacy of data and controls (db) and performs any unnecessary checks
(currently checks about the possibility of operating on the tentant and the appropriate tenant global scope are applied on the authorization method).

Feel free to close the issue and thank you again.

[dungfv]

That's exactly my experience and for that reason, in non-trivial projects I can't use it.

I am also unable to use the authorization method, instead have to use custom Traits.
So I proposed a solution similar to the current Controller of the Laravel library (Illuminate).
(found be at Illuminate\Foundation\Auth\Access\AuthorizesRequests)