Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

token cookie not set in POST requests with CreateFreshApiToken middleware #293

Closed
plokko opened this issue Feb 24, 2017 · 14 comments
Closed

Comments

@plokko
Copy link

plokko commented Feb 24, 2017

I'm building a Laravel Vue-driven application that uses some API commands;
i'm using CreateFreshApiToken to extend web auth to API auth but i'm having some problems:
CreateFreshApiToken only works on GET requests (as in CreateFreshApiToken.requestShouldReceiveFreshToken).
I tried to make an AJAX login with an axios that returns the logged user but the cookies are populated ONLY on GET requests even if the auth was successfull

Example code for the bug:

axios.post('/login',{email,password}).then(r=>{ 
	// Login succeeded BUT it's a POST request so passport cookies will NOT be returned! (unlike session and CSRF cookies)//

	alert('Welcome back '+r.data.name+'!');

	//... This will NOT work because passport cookies were NOT set during login request!
	axios.get('/api/user').then(r=>{alert('this will fail!');}).catch(()=>{
	
	//...but if i execute a GET on any existing web page or refresh the current page the passport cookies will be re-populated and auth will also work on that page!
	axios.get('/whatever-page-in-web-group').then(()=>{
	
	//..same code but AFTER a GET request
	axios.get('/api/user').then(r=>{alert('hello again '+r.data.name+'!');});
        });
        });
});

Example of Laravel login i want to achieve:

class LoginController extends Controller
{
//...
    public function authenticated(Request $request,\App\User $user)
    {
        return $request->ajax()?//if it's an AJAX request just return the user,no redirect needed!
                response()->json([
                        'name'=>$user->name,
                        'email'=>$user->email,
                ])
            :
            redirect()->intended($this->redirectPath());//if it's a normal login redirect to page
    }
//...
}

i could simply redirect to a page that returns the user (that would be a GET request so the token would be set) but why creating another page and another http request just for that?
A solution would be to enable passport cookie on all requests type (maybe settable on config) or at least provide a response macro like response()->appendPassportCookies() for AJAX login pages.

@renanwilliam
Copy link

+1

@plokko
Copy link
Author

plokko commented Feb 28, 2017

A similar issue was replied in #59 but i think this is a bug not a "feature" because CreateFreshApiToken should provide a transparent bridge between API and Web sessions, not more hassle for the programmers!
GET request does support cookie creation and all php/laravel session cookies are created no matter what the request type is so i don't understand why it should not be fixed!

@renanwilliam
Copy link

renanwilliam commented Feb 28, 2017

I think that the problem is not just with the GET request type.
Looking at the CreateFreshApiToken class at line 53:

if ($this->shouldReceiveFreshToken($request, $response)) {
   $response->withCookie($this->cookieFactory->make(
       $request->user($this->guard)->getKey(), $request->session()->token()
    ));
}

If you working with just Passport authentication through API (for instance a SPA application), the instruction $request->session() with thrown a 'Session store not set on request.' exception because will not exists a session.

Probably the module isn't designed to work using this approach and I think it can work with a custom middleware extended from CreateFreshApiToken and inserted on 'api' middleware group

@henrikruscon
Copy link

henrikruscon commented Mar 3, 2017

It would definitely be neat to be able to handle it this way. It sucks that that my login can't be part of the SPA in an easy elegant way ✌️

@plokko
Copy link
Author

plokko commented Mar 3, 2017

I actually found two problems:

  1. in requestShouldReceiveFreshToken request method must be of type GET (why?!?)
  2. response for json is handled by class Illuminate\Http\JsonResponse; responseShouldReceiveFreshToken fails due to $response instanceof Response check.

FIX:

  1. Remove requestShouldReceiveFreshToken GET limitation or at least add POST/PUT request.
  2. Replace
use Illuminate\Http\Response;

with this: (should work for every request, it's just the basic request class)

use Symfony\Component\HttpFoundation\Response;

I tested with the same sample code and now it's working!

@renanwilliam i'm using CreateFreshApiToken only for web routes as intended;
i'm using the default Laravel login route just with ajax and without redirecting the request to home.

@plokko plokko closed this as completed Mar 3, 2017
@plokko plokko reopened this Mar 3, 2017
plokko pushed a commit to plokko/passport that referenced this issue Mar 3, 2017
@henrikruscon
Copy link

How secure is this? @plokko

@plokko
Copy link
Author

plokko commented Mar 10, 2017

@henrikdahl What do you mean secure?
I just extended the cookie to POST and PUT requests and all response type instead of just plain HTML;
I did not remove any other security check:
the cookies are set only on a valid session login so security should be a non issue.

@henrikruscon
Copy link

@plokko It just baffles me that it wasn't like this by default. Which in turn made me think there might be some security implication but from looking into it I can't find any ✌️

@plokko
Copy link
Author

plokko commented Mar 10, 2017

@henrikdahl it was a non-issue because default Laravel login redirect to a GET/HTML page (/home) where cookies are set (AJAX request do follow redirects and save cookies even from redirect pages).

@yazfield
Copy link

yazfield commented Apr 7, 2017

Having the same problem, I too thought it was weird that the cookie was only set with GET requests, is there a logic behind this choice?

@nikolaynesov
Copy link

Hey guys,

I had the same issue some time before, so this is what I came up with. Maybe it will help someone.
In my LoginController I overwrote method:

protected function sendLoginResponse(Request $request)
    {

        $request->session()->regenerate();
        
        $this->clearLoginAttempts($request);

        return Response::json(

            $this->userRepo->parserResult($this->guard()->user()),
            200

        )->withCookie(

            $this->cookieFactory->make(

                $this->guard()->user()->getKey(),
                $request->session()->token()

            )

        );

    }

I am using repository pattern but you can do the same with pure Eloquent.
You also need to inject ApiTokenCookieFactory into LoginController constructor like this:

protected $cookieFactory;

public function __construct(ApiTokenCookieFactory $cookieFactory)
    {

        $this->cookieFactory = $cookieFactory;
        $this->middleware('guest', ['except' => ['logout']]);

}

And don't forget to use the right class:

use Laravel\Passport\ApiTokenCookieFactory;

for LoginController.

Response here is just ResponseFactory class, so you can use Facade.

So finally you will get laravel_token set in cookies in login server response.

@themsaid
Copy link
Member

themsaid commented Jul 5, 2017

Closing for lack of activity, hope you got the help you needed :)

@themsaid themsaid closed this as completed Jul 5, 2017
@itsUndefined
Copy link
Contributor

Did the fix for post requests not get passed?

@Firtzberg
Copy link

@nikolaynesov It is enough to override the authenticated method of the AuthenticatesUsers trait.

protected function authenticated(Request $request, $user)
    {
        return Response::json($user, 200)
            ->withCookie($this->cookieFactory->make(
                $user->getKey(), $request->session()->token()
            ));
    }

I also had to do this to get it working:
#47 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants