laravel-graphql
laravel-graphql copied to clipboard
Should not check authorization before validation?
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);
}
}
}
@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.
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!
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.