Graphql-laravel: 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 🌴

[fvdung]

@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.

[fvdung]

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.

[fvdung]

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)

[mfn]

@fvdung looks interesting but you can elaborate what / how this project can benefit from the trait approach, specifically? I checked it out but it's not obvious to me; thank you!

[fvdung]

@fvdung looks interesting but you can elaborate what / how this project can benefit from the trait approach, specifically? I checked it out but it's not obvious to me; thank you!

The use of traits as helper makes the code more flexible. The authorize method can be called anywhere in a hook of GraphQL.
Moreover, it is possible to customize message from authorization (multi language ...)

[mfn]

@fvdung I must be missing something and I confess I still didn't get it :| Can you give a more practical example how a/the trait would improve things?

[fvdung]

You can refer to this document in the core of laravel.
authorization#via-controller-helpers
My idea is something like that.

[mfn]

I think everything which came up here was tackled:

• explained why the current form of authentication (middleware) -> validation -> authorization -> resolver stays
• arguments for authorize where changed: https://github.com/rebing/graphql-laravel/pull/489
  as in: they now take the exact same arguments as resolve(), there's no difference except authorize will throw a certain kind of exception it the return value is "falsy"

[chrisnharvey]

Sorry to drag up an old thread, but I just wanted to add some thoughts on this.

I think we should rethink the order of authorization and validation. In my opinion, authorize should always come first for a couple of reasons.

1. Validating first can expose intricacies of your API to unauthenticated users. For example, if you have validation rules for user registration, anyone will be able to query the API to find out if an email address is registered if there is a validation rule that checks that the supplied email address is unique.

2. Since the validation rules are ran before the authorization middleware, there is no access to the currently logged in user from the Request object since the auth middleware hasn't ran at that point. If we had a query that was allowing the user to update their email address, we wouldn't be able to validate if that email is unique but not equal to their current email address. See below.

public function rules(array $args = []): array
{
    $user = request()->user();

    return [
        'name' => ['string', 'max:255'],
        'email' => ['string', 'email', 'max:255', "unique:users,email,{$user->id}"],
        'password' => ['string', 'min:8'],
        'billingAddress1' => ['string', 'max:255'],
        'billingAddress2' => ['nullable', 'string', 'max:255'],
        'billingCity' => ['string', 'max:255'],
        'billingCounty' => ['string', 'max:255'],
        'billingPostcode' => ['string', 'max:255'],
        'billingCountry' => ['string', 'max:255'],
    ];
}

I understand the pros/cons for both approaches, but I think the pros of running authorization first outweigh the cons.

[mfn]

That's some good arguments and TBH I'm also not so confident about my "TL;DR" from https://github.com/rebing/graphql-laravel/issues/481#issuecomment-535042684 anymore.

From where I currently stand, _authorization before validation_ indeed makes a lot of sense.

[mfn]

So, re-opened this to gather more feedback / arguments how we should do it, pro-cons, what options are we having for those we "need it the other way around", security considerations in mind, backward compatibility breaks, etc.

I think this should be clarified before jumping into code (though I don't mind PRs + tests for discussion)

[chrisnharvey]

Yeah, would be good to get feedback from other users.

I think in terms of implementation, it should just be a case of moving this below this.

Happy to submit a PR (with tests, if applicable) so we can start a discussion around this.

[rebing]

These are valid points indeed. I see that the Laravel framework also follows the logic of:

1. Prepare data for validation
2. Authorize request
3. Validate data

Especially from a security standpoint we should definitely consider this change, but need to be careful here, since it's a breaking change. You have my +1 though.

[mfn]

Great, thank you @rebing

@chrisnharvey yes, starting a PR now would be good for design/discussion, thank you!

[illambo]

Hi all,
in my opinion I prefer authorization before validation mainly for two reason :

• security
• efficiency (in terms of avoid consuming machine time and its cost)

But wait, I _“try”_ to see beyond my horizon,
if we look closely there is currently another validation layer, the schema validation.

Currently: schema validation -> data validation (business logic) -> authorization

Is it correct to think so?

I want to deepen as soon as I can in this, schema / business logic / graphql philosophy, maybe i can find something in these resources:
https://www.graphql-tools.com/docs/introduction#the-graphql-first-philosophy
https://www.apollographql.com/blog/graphql-at-facebook-by-dan-schafer-38d65ef075af/#.jduhdwudr

Package like graphql-constraint-directive (for Apollo Server) works like middleware.

So currently I think the best approach is in this pr #594 (middleware pattern) with some ability in configs for manage this (orders, add, remove, ..)

[illambo]

Currently the GraphQL guidelines provide for the authorization and validation phase to be carried out by the business logic layer of the application (see https://graphql.org/learn/thinking-in-graphs/#business-logic-layer and https://graphql.org/learn/authorization).
I remain of the opinion that PR above is the most valid way, I eagerly await :smirk:

[illambo]

I am happy to see that PR #594 has been merged :rocket:, so at the moment I think we can close the discussion (also because I don't see other ideas).