folkloreinc / laravel-graphql

Facebook GraphQL for Laravel 5. It supports Relay, eloquent models, validation and GraphiQL.
1.77k stars 233 forks source link

Should not check authorization before validation? #256

Open ahalimkara opened 6 years ago

ahalimkara commented 6 years ago

Currently validations are running before authorization here, even for unauthorized requests.

Is it something intended? I just thought maybe there is no need to to run validation for unauthorized requests.

If not then this is a quick fix (ShouldValidate.php):

if (call_user_func_array([$this, 'authorize'], $arguments) === true) {
    $rules = call_user_func_array([$this, 'getRules'], $arguments);
    if (sizeof($rules)) {
        $args = array_get($arguments, 1, []);
        $validator = $this->getValidator($args, $rules);
        if ($validator->fails()) {
            throw with(new ValidationError('validation'))->setValidator($validator);
        }
    }
}
hailwood commented 6 years ago

@ahalimkara the thing to consider here is that your authorization logic may well rely on certain fields being present so you can grab the correct model to authorize against, it's unlikely the reverse will happen - you're not likely to need anything from authorization to be able to build your validation rules.

If we were to authorize before validation you'd have a lot of code that would read as

public function authorize($args) {
    try {
        return \Gate::allows('update', ModelClass::findOrFail($args['id']));
    } catch (\Exception) {
        // ... what do we return here? An authorization exception isn't going to tell the
        // user that the provided ID is either not provided, or not valid...
        // but returning true doesn't feel right either
    }
}

So the short of it is, it makes total sense to run validation before authorization.

jpnut commented 6 years ago

Doesn't this process risk leaking some information? For example, if I submit a mutation to some resource which I am not permitted to access or update, I can obtain information about what fields are being stored. This has come up before: #181

I agree with @hailwood that at least some validation must be run before authorisation since we need to know what we are authorising, however it seems to me just as important that we are able to validate after authorisation too.

Not wanting to break backwards compatibility, perhaps a function such as postAuthorisationRules which is essentially identical to the exisiting rules function, but runs only after authorisation? This feels a bit hacky to me.. I'd be grateful to get input from others!

kikoseijo commented 6 years ago

My solution where was to create several schemas, them all open to the introspection query to be able to develop at client level with remote schema, you can disable after development if desired.

And then to protect some parts I was making schema with middleware in the configuration file, I had it working on a lumen project on my repos if you want to see how I implemented.

Hope it helps.