[L5] Request validation breaks router

[esbenp]

If I make input validation on a request using a request class like so

<?php namespace App\Http\Requests;

use App\Http\Requests\Request;

class LoginRequest extends Request {

    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return true;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
            'email'    => 'required|email' ,
            'password' => 'required'
        ];
    }

}

it breaks the router on invalid input. The request class will issue a RedirectResponse and the Dingo router is expecting a normal Response resulting in the following exception being thrown.

Argument 1 passed to Dingo\\Api\\Http\\Response::makeFromExisting() must be an instance of Illuminate\\Http\\Response, instance of Illuminate\\Http\\RedirectResponse given, called in \/home\/vagrant\/code\/traede-l5\/vendor\/dingo\/api\/src\/Routing\/Router.php on line 551 and defined

I don't know if changing the type hinting to Symfony\Component\HttpFoundation\Response and dropping the getOriginalContent for a getContent is a valid solution. I tried it out and it seemed to work, however I did not thoroughly test it.

Dingo\Api\Http\Response

public static function makeFromExisting(\Symfony\Component\HttpFoundation\Response $old)
    {
        $new = static::create($old->getContent(), $old->getStatusCode());

        $new->headers = $old->headers;

        return $new;
    }

[jasonlewis]

Is it set it stone that it must return a redirect response? An API endpoint shouldn't be redirecting.

[jasonlewis]

So after playing around with this for a bit it seems that it's going to be tough to address this issue in a way that would be best.

The exception thrown by the validator is caught manually by Laravel and the response automatically returned. I would've liked to have been able to catch this exception myself but that doesn't seem possible.

I'll look at other ways to resolve this. Simply type-hinting a different response instance isn't going to cut it as that adds in a whole bunch of other things to worry about. Will keep you all posted.

[esbenp]

I checked it out a little bit the other day. It seems the redirect is not the wanted behaviour for API requests.

Illuminate\Foundation\Http\FormRequest

/**
     * Get the proper failed validation response for the request.
     *
     * @param  array  $errors
     * @return \Symfony\Component\HttpFoundation\Response
     */
    public function response(array $errors)
    {
        if ($this->ajax() || $this->wantsJson()) {
            return new JsonResponse($errors, 422);
        }

        return $this->redirector->to($this->getRedirectUrl())
                                        ->withInput($this->except($this->dontFlash))
                                        ->withErrors($errors, $this->errorBag);
    }

So if the first entry of the Accept header is application/json or if X-Requested-With is equal XMLHttpRequest the redirect is not happening.

If I remember correctly we got the redirect response when debugging around in curl, and I did probably not set the correct accept header. I guess it would be reasonable to expect the client to set the correct headers?

Another solution would be to provide a new Request class, like ApiRequest that extends FormRequest and overrides the behaviour in response() to always return a JsonResponse

[fer-ri]

seems like extends FormRequest would be nice :)

[jasonlewis]

No, redirects shouldn't occur for an API endpoint. Basically, if you were to access an endpoint that had the validator in it from your browser (directly, default headers), and it failed then you'd get a RedirectResponse.

I'm not so sure a custom FormRequest would suffice as there's multiple ways to do validation. You can either use the form requests or you can use the ValidatesRequests trait.

[esbenp]

Ah yes you are right.

What do you think, the solution could be either (1) leave it to the client to add appropriate headers, or (2) extend Laravel to enforce API-like responses on validation errors (i.e. JsonResponse)?

I am all in for adding an ApiRequest class and a ApiValidatesRequest trait (or whatever it would be called) that extend from the base Laravel classes and enforces API friendly behaviour. Are you worried this would fit awkwardly with the core library @jasonlewis?

[bweston92]

Any updates? I don't want to put this logic in my controller makes it so ugly. I'm fine with a ApiRequest that handles this.

[Anahkiasen]

I managed to make FormRequests work with dingo/api by doing this personally:

class AbstractApiRequest extends FormRequest
{
    /**
     * {@inheritdoc}
     */
    protected function failedValidation(Validator $validator)
    {
        throw new ResourceException('Invalid request', $validator->getMessageBag());
    }
}

Kind of generic but at least it works

[fer-ri]

@Anahkiasen could you show how to use that code?

THanks :)

[Anahkiasen]

Same as you'd use any FormRequest, you extend it, define your rules, and inject the extending class into a method

[fer-ri]

Ah got it, Thanks :+1:

[tomschlick]

@Anahkiasen turning that into a trait which could then be added to the base form request might be even better.

That would make it easy to implement and doesn't require extending the base.

something like this

trait ApiRequestTrait
{
    /**
     * {@inheritdoc}
     */
    protected function failedValidation(Validator $validator)
    {
        if($this->is(config('api.prefix') . '/*')) {
            throw new ResourceException('Invalid request', $validator->getMessageBag());
        }

        parent::failedValidation($validator);
    }
}

[Anahkiasen]

A trait might be the best approach indeed if dingo/api were to ship with something

[lightvision]

I personally like the @Anahkiasen solution without a trai. That's the way that FormRequest works for laravel too,by extending a base class (to be able to make customizations)

Well, after a little bit of thinking the trait is definitively the solution in the api base controller. This way a single FormRequest could be used for validation in both api controller and in the controller that self consumes the api.

[jasonlewis]

Hm, I'm torn. I'll try a few more things but from the looks of it a trait/class seems like the best bet here. Bare with my folks I'll update again soon.

[SnehalSatardekar]

Hello,

I am getting issue like this

"Argument 1 passed to Dingo\Api\Routing\Router::prepareResponse() must be an instance of Illuminate\Http\Response, instance of Illuminate\Http\RedirectResponse given"

I have dingo api and laravel project under same version or we can say under same roof.

How can we run both things in same project?

Please let me know as soon as possible

[lightvision]

I managed to use FormRequests for validations but I have a different issue here, with VerifyCsrfToken.

The api endpoind throws me an Illuminate \ Session \ TokenMismatchException.

I've tried to change the session.driver config value from file to array but no avail.

Shouldn't ding/api disable the VerifyCsrfToken middleware for api requests (api routes)? I know that this will ad some level of complexity since the same middleware should be left as it is for the rest of the app.

[jasonlewis]

Okay guys and gals so I'm probably going to add a class for this, it'll be pretty basic and all it'll have are the following methods (overriding the existing ones):

class FormRequest extends IlluminateFormRequest
{
    protected function failedValidation(ValidatorContract $validator)
    {
        throw new ResourceException(null, $validator->errors());
    }

    protected function failedAuthorization()
    {
        throw new HttpException(403);
    }
}

Does that sound okay to all?

@lightvision I don't think it should be up to the package to disable the middleware. You can enable it only for the routes that need it and not for the API routes.

[lightvision]

@jasonlewis Yes, I know and maybe I will do it. My client decided to leave it on an make a get request before posting in order to fetch the csfr token and pass along with the post data.

Off-topic: May be I miss understood the things about cookies and api responses, but the package returns cookies whilst it shouldn't do it.

[jasonlewis]

There's no law against cookies as far as I'm aware. Please correct me if I'm wrong though.

[lightvision]

:) You are right!

[catalinux]

Well actually they do :) http://ec.europa.eu/ipg/basics/legal/cookies/index_en.htm

[jasonlewis]

Haha oh god, not that bloody EU cookie law stuff! Ridiculous.

Oh and @lightvision, that said, I don't encourage you to authenticate a user then save that state. As far as authentication goes an API should be stateless. I do think there are people that would want to disable cookies completely though. So I might add a way for people to do that should they want to.

[lightvision]

@jasonlewis Yes, I know that an API should be stateless so that's why I issue the cookie problem. But it's a little bit tricky here, because I need to completely disable the cookies for the api and leave them ON for my app. I am building an app that internally calls the public api.

So if there is a way that cookies to be disabled on the dingo/api responses then that should be it out of the box (to keep consistent with the stateless api requirements). I could disable them at the api base controller but I think that there is more work involved.

[jasonlewis]

@lightvision, moving this cookie discussion to #529.

[beningreenjam]

@jasonlewis yes, a little class which overrides the Illuminate defaults seems like the better plan.

I assume it'd be a simple case of us changing use Illuminate\Foundation\Http\FormRequest; to something like use Dingo\Api\Http\FormRequest; on our base Request class yes?

[lightvision]

@jasonlewis Short explanation: don't bother with adding a class for this, instead provide a wiki entry to show how could be done.

Why? Because there are many use cases of this package that you could not cover.

Long answer and an use case:

I have an app that expose a public api and on the same time self consumes the api through internal requests. The public api and the app are a must of this project.

Right now I have only one version of the api but several version could exists on a long term. I need to perform validation in both public api and my app.

The api is only in english while the app is multi language. The app could use many versions of the api on the time being.

In order to not duplicate the code too much and to maintain a clean code here is what I've done:

I've created an ApiRequest class which override the failedValidation method. Of course, failedAuthorization could be also overriden. Those methods could be moved on a trait and ApiRequest class is abstract. Nothing fancy here, just how @Anahkiasen said.

For each input validation in api I have to extend the ApiRequest class. Like I should with default Laravel but avoiding redirection. I will provide the code because is easy to follow my idea.

<?php

namespace App\Api\v1\Http\Requests;

use Dingo\Api\Exception\ResourceException;
use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Contracts\Validation\Validator;

/**
 * ApiRequest
 * 
 * This class allows us to use Illuminate\Foundation\Http\FormRequest for validation
 * inside the api controllers.
 * 
 * Api Responses should not redirect!!!
 * 
 * So in order to use FormRequest we must override some defaults methods.
 *
 * @version 1.0.0
 * @author marius ionel <webmaster@grupnet.ro>
 */
abstract class ApiRequest extends FormRequest {

    /**
     * Override the failedValidation method in order to avoid redirection
     * 
     * and return a valid api validation error
     * 
     * @param Validator $validator
     * @throws ResourceException
     */
    protected function failedValidation(Validator $validator) {
        throw new ResourceException('Resource validation failed!', $validator->getMessageBag());
    }

}

Now the actual validation class StoreNewClientRequest:

<?php

namespace RoboticAccounting\Api\v1\Http\Requests\Admin;

use App\Api\v1\Http\Requests\ApiRequest;

/**
 * StoreNewClientRequest
 * 
 * Validation Rules for adding new clients in the admin panel
 *
 * @version 1.0.0
 * @author marius
 */
class StoreNewClientRequest extends ApiRequest {

    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize() {
        // FIXME Check permission before returning true!
        return true;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules() {
        return [
            'first_name' => 'required|min:2',
            'last_name' => 'required|min:2',
            'email'=>'required|email|unique:clients,email',
            'company_name'=>'required|min:2',
        ];
    }
}

In order to perform validation for the api I will inject StoreNewClientRequest in the store() method api controller.

Now on the app I have to exdend the StoreNewClientRequest:

<?php

namespace App\Http\Requests\Admin;

use App\Api\v1\Http\Requests\Admin\StoreNewClientRequest as apiV1StoreNewClientRequest;
use App\Traits\Requests\FormRequestTrait;

/**
 * StoreNewClientRequest
 * 
 * We simply extend the API StoreNewClientRequest
 * and use FormRequestTrait to reset back the failedValidation() method
 * 
 * Rules and authorization are in the parent class.
 *
 * @version 1.0.0
 * @author marius ionel <webmaster@grupnet.ro>
 */
class StoreNewClientRequest extends apiV1StoreNewClientRequest {

    use FormRequestTrait;
    // formating multi language here in attributes()
    // perhaps changing the authorize() method too
}

and in FormRequestTrait I restore the Laravel default behaviour for redirection. In the app controller Iinject the last StoreNewClientRequest which belongs to App\Http\Requests\Admin namespace.

Now let's say you create a dingo/api FormRequest in order to provide an "automatic" response. Which class will be extended by DingoFormRequest? Illuminate Request or Dingo Request? And it doesn't matter because you couldn't use instanceOf to determine which kind of request was, in order to override the default Laravel behavior or to restore it.

So let's go simple since the solution already exists. The solution provided by @tomschlick will work if we are using api.prefix so let's use it.

[jasonlewis]

@lightvision I see your point, although if I were to implement a class it doesn't mean you have to use it. It just means that for people that want that basic usage can easily just implement it. For others who may need a more complex use case they can roll their own.

[lightvision]

Your point is right. But sometimes its just easy to let things as it and stop complains about a feature that has an easy fix - and that's my point.

[jasonlewis]

I understand, but if I'm going to be supplying the code in the docs I might as well supply a simple base API form request in the code. I've gone ahead and added it. It's very simple, but will suit many use cases and will no doubt be fine for the majority of users.

It's located at: Dingo\Api\Http\FormRequest

Docs have been updated.

[baijunyao]

@tomschlick Thank you very much.

[Yangwendaxia]

@tomschlick Thank you very much. The following code works fine:

use Dingo\Api\Exception\ResourceException;
use Illuminate\Contracts\Validation\Validator;

trait ApiRequestTrait
{
    /**
     * {@inheritdoc}
     */
    protected function failedValidation(Validator $validator)
    {
        if ($this->is(config('api.prefix') . '/*')) {
            throw new ResourceException('Invalid request', $validator->getMessageBag());
        }

        parent::failedValidation($validator);
    }
}