InputType Factory and Security

[massimilianobraglia]

Hello there,
thank you for this library!

Just a quick question:

I got a controller for which I created an InputType and relative Factory.
In the factory I pass all the mutation data and return the well formed constructed InputType.

class Controller
{
    /**
     * @GQL\Mutation
     * @GQL\Logged
     * @GQL\Security("is_granted('IS_AUTHENTICATED_FULLY')")
     * @GQL\InjectUser(for="$user")
     * @GQL\UseInputType(for="something", inputType="Something!")
     */
    public function createSomething(User $user, Something $something)
    {
        $something->setUser($user);
        // do stuff
    }
}

I would like to move the user injection into directly the factory, so I used @GQL\InjectUser annotation into my factory like this:

class AccountFactory
{
    /**
     * @GQL\InjectUser(for="$user")
     * @GQL\Factory(name="Something", default=true)
     */
    public static function createSomething(
        string $field1,
        string $field2,
        User $user
    ): Something {
        return new Something($field1, $field2);
    }
}

This works well while I got a logged in user. If not, I got a PHP failure because the createSomething method does not accept null for $user parameter.

I tried using @Logged, @Right and @Security annotations but seems not working.

Am I missing something?

[oojacoboo]

Is it a valid situation for this mutation to be called without a User? If yes, maybe it makes more sense to create another mutation for that. If no, why isn't your auth throwing a 401 before this factory is initialized?

[massimilianobraglia]

Is it a valid situation for this mutation to be called without a User? If yes, maybe it makes more sense to create another mutation for that. If no, why isn't your auth throwing a 401 before this factory is initialized?

Why should it be mandatory to have always a user logged in? It can be useful to inject null whether the user is not logged.

In this example in particular, it the mutation have the user param as mandatory. The problem is not my auth that doesn't throw a 401, the problem is that @Logged annotation in factory context does not throw it 😅

[oojacoboo]

I'm not familiar with the internals of the user injection annotation. Personally, I'm not a huge fan of this functionality at all. Regardless, I did notice that your User $user argument isn't nullable. Did you test it with a nullable argument as well?

[massimilianobraglia]

Maybe I missed that I'm using this one with symfony and GraphQLite official bundle. Anyway, I did not try without because it should be thrown a 401 before accessing this method by design. I am not calling the factory method by myself, it's the bundle that is calling it

[oojacoboo]

Well, if you want a null value there, it can't throw a 401 prior. So, it should either support a null value or not. I'm uncertain of what's the case, or the overall intent since I haven't and never would use something like this. IMO, auth should be handled on every request. It's not a flag to be turned on or off at some annotation level. If you wished to have operations that don't require auth, you simply organize them so that auth checks are bypassed for those operations. It's not difficult.

Maybe someone else can chime in on this one. I'd love nothing more than to remove this annotation, personally. Auth shouldn't be a part of this library as I see it.

[massimilianobraglia]

Well, if you want a null value there, it can't throw a 401 prior.

I don't want a null value there. It's GraphQLite that prior to anything tries to set that value (in the case of anon. users) to null.

Auth should be handled on every request.

Auth is handled on every request.

It's not a flag to be turned on or off at some annotation level. If you wished to have operations that don't require auth, you simply organize them so that auth checks are bypassed for those operations. It's not difficult.

Very kind way to support users, thank you. My code is just ok. I'm just asking why this is happening and whether this is the expected behaviour or not.

[oojacoboo]

Very kind way to support users, thank you. My code is just ok. I'm just asking why this is happening and whether this is the expected behaviour or not.

This is simply my opinion. I think if you're relying on @Logged you've designed your application incorrectly. That's the purpose of my comment and maybe that can provide you with a solution to your issue.

As for the @Logged annotation, maybe @moufmouf can chime in on it. I don't know. I opened #196 quite a while back around this annotation. My current opinion is that it should be deprecated.

[oojacoboo]

Closing this issue as there hasn't been any further discussion.