Lighthouse: Validation with @rules executes before field middleware

[lucacri]

Hello,
I am trying to have an authorized mutation check for a custom validation rule, in which I check for auth()->user() to see if the user can create the entity related to that specific model.
To make it simple, I am creating a Todo for a Client, but I need to check that the Client is part of the Team of the current User.

My mutation looks like this:

extend type Mutation @middleware(checks: ["auth:api"]) {
    CreateTodo(client_id: ID! @rules(apply: ["App\\Rules\\ClientBelongsToCurrentTeam"]),
        message: String @rules(apply: [ "required"])) : Todo

The ClientBelongsToCurrentTeam is pretty simple:

    public function passes($attribute, $value) {

            return Client::byTeam(auth()->user()->currentTeam)
                         ->where('id', $value)
                         ->exists();
    }

And I'm using Passport with the joselfonseca/lighthouse-graphql-passport-auth package, but this happens even without it (using the CSRF token).

From what I could gather, the validation directives happen _before_ the middleware, so the user is not logged in yet.

I tested this same validation rule on a normal Laravel validation + auth page, and it worked correctly

Expected behavior/Solution

We should follow the standard Laravel way, and middleware/auth should happen before validation.

Steps to reproduce

1. Create a custom validation that uses auth()->user()
2. Create a mutation with @middleware(checks: ["auth:api"]) and using the custom validation created before
3. Try to run it

Output/Logs

N/A

Environment

Lighthouse Version: v3.7.0
Laravel Version: v5.8.27

[spawnia]

Thanks for your detailed write-up.

What you are saying makes sense, i am investigating the implications of switching the resolver order around right now.

Edit:

I stumbled upon one particular issue that lies within the interaction between @spread and @can. As of now, all ArgumentDirectives are evaluated before the Field Directives. That means that inputs are flattened out before they reach the next middleware. If i change that order around, @can would break as it would no longer receive a flattened input.

Still pondering easier solutions. As of now, i am afraid that currently you would have to work around the issue by doing validation in a custom FieldMiddleware (something that will get better support with https://github.com/nuwave/lighthouse/pull/846)

[spawnia]

To accomodate all possible use cases, there has to be a way to define the order in which operations such as validation, input transformation etc. should be carried out.

This works quite well when dealing with Field directives, as they are applied in order of definition. The same holds true for Argument directives, at least to some extent.

The fundemental problem i see right now is that that there is currently no way to specify the order between those two types of directives. Lighthouse is trying to do something reasonable by default, but as this issue shows, it might not always work.

[lucacri]

Thank you for the reply, what you are saying is totally valid. What I'd do is process everything with the order of:

• middleware first
• authorization ( can, etc )
• validation of fields

The thing is that we should assume that if a person is asking for a mutation/query to be under the middleware auth (for example), the authorization should already be done before we proceed to the next steps. This is also how Laravel does it, since the authorization is handled in the kernel of the Http section, before we deal with seeing fi the user passed a valid Post $id in the /posts/:id route (for example).

[spawnia]

https://github.com/nuwave/lighthouse/pull/856 should resolve the conflict between @can and @spread, so we can try changing the execution order after that.

Middleware directives are not the same as Laravel middleware, albeit conceptually similar. They do reach further into the actual execution/resolving of results. Generally, we can not know what a user does within such middleware. They might be validating, authenticating, transforming inputs or output - anything goes.

I thought about adding special directives that trigger the execution of ArgDirectives at a certain point, e.g.:

type Query {
  foo(
    # Argument directives by themselves do nothing
    bar: String @rules(apply: ["alpha"]) @trim
  ): Foo
    @middleware(checks: ["auth:api"])
    @validate # Considers @rules and runs validation
    @can(ability: "view")
    @transform # Runs arg transformers such as @trim
}

This might be overly complex...

[lucacri]

Yes that’d be complex, and also it would break the conformity with the Laravel nomenclature.

I’d say we should consider middlewares in the same way Laravel considers them: things that happen (in order) before the request is processed. Changing the concept of “middleware” just for this package will lead to a lot of confusion for new users.

Validation and authorization should happen with @can and @validate, after @middlewares are applied

[olivernybroe]

In Laravel you have the $middlewarePriority, maybe we can incorporate something like that?

/**
 * The priority-sorted list of middleware.
 *
 * This forces non-global middleware to always be in the given order.
 *
 * @var array
 */
protected $middlewarePriority = [
    \Illuminate\Session\Middleware\StartSession::class,
    \Illuminate\View\Middleware\ShareErrorsFromSession::class,
    \App\Http\Middleware\Authenticate::class,
    \Illuminate\Session\Middleware\AuthenticateSession::class,
    \Illuminate\Routing\Middleware\SubstituteBindings::class,
    \Illuminate\Auth\Middleware\Authorize::class,
];

[markwalet]

What is the state of this issue?

[spawnia]

What is the state of this issue?

There are some prerequisites that i think we will have to meet first in order to implement proper ordering in a generalized way.

Firstly, @middleware is deprecated and will be removed in v5 in favor of @guard. That makes it explicit that it is used for authentication purposes.

Next, we will need to put it in the correct order. As of now, @guard is implemented just like any other field middleware. We have an ongoing discussion to allow ordering the execution of field middleware: https://github.com/nuwave/lighthouse/issues/1136

Finally, we will have to put validation completely into a single field middleware. Right now, we do it all before any field middleware executes. The reason for that is that we interleave validation with argument transformers, a way to modify args through directives. I have thought of a way how we can solve that though, we need to add two new field middleware: One each for running arg transformers before and after validation.

[spawnia]

This issue will be resolved through https://github.com/nuwave/lighthouse/issues/1136 in an upcoming v5.