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

Expired token raising wrong exception #14

Closed
felnne opened this issue Nov 27, 2014 · 9 comments
Closed

Expired token raising wrong exception #14

felnne opened this issue Nov 27, 2014 · 9 comments

Comments

@felnne
Copy link

felnne commented Nov 27, 2014

Hi,

I'm trying to combine jwt-auth with regular session based auth such that if either are present/valid the user is allowed through.

I've created a custom filter to do this since I don't think this is something I could do out of the box (but please correct me if i'm wrong, I would love not to have to do this ;) ).

Everything seems to working fine, I can check if either a session is authenticated or a token is valid but when a token isn't valid, but is supplied, i'm having trouble determining why it failed.

Specifically i'm having trouble figuring out when a TokenExpiredException is ever thrown.

For reference this is how i'm checking, its essentially the same as your JWTAuthFilter but i'm suppressing the exceptions in case another source of authentication proves correct. The check to see if a user is authenticated using a token is this:

https://gist.github.com/felnne/67851ee22a7681e43352
(I assumed the gist would be auto-embedded somehow but I guess not! Sorry for the extra click).

Note: I haven't added the extra checks for checking the user etc. that's to come.

If I try to go to a route using my filter, but without the switch statement for the JWTEception catch, I will get an error that the token is invalid. If I dd() the exception it says the token has expired.

I would assume in such cases the TokenExpiredException would be thrown instead? If not when is that used? I had a quick look through the code and found a call to that exception in a validator class, but i'm afraid I gave up looking after going up a couple of levels.

I copied the try/catch structure from your filter, so I assume that's not the problem. My workaround works and can tell the difference between an expired token and an invalid token, but i'm curious why the 'wrong' exception is thrown at all.

I tried using your filter but I get the same error, "invalid token" rather than anything about validity.

Could you help clarify this for me?

Thanks,

Felix

@tymondesigns
Copy link
Owner

Hi Felix,

This is indeed an issue with the package. The reason your seeing this behaviour is because the firebase provider, which does the heavy lifting of encoding and decoding the tokens, was throwing an UnexpectedValueException (because the token had expired) which was being caught before a TokenExpiredException could be thrown. As a result a TokenInvalidException is actually thrown with the message "Expired Token", (as you experienced).

I believe this is now fixed and I have released version 0.3.9 to address this. Please let me know if this fixes your issue.

Thanks!

@felnne
Copy link
Author

felnne commented Nov 28, 2014

Hi,

I've updated and now getting an error that $payload isn't defined when using an expired token.

Looking at the firebase provider its correctly skipping the general JWTException but there's then no $payload defined. I tried just repeating the $payload = (array) Firebase::decode() from the 'try' after the if statement but got an unexpected value error so not sure what to do to fix that.

@migaber
Copy link

migaber commented Nov 30, 2014

I've The same issue, I've tried to bypass the $payload but I get the exception from the toUser() method, it seems that the Expire Exception not fired,

my code

$user = \JWTAuth::parseToken()->toUser();

the error report

file: "/home/migaber/public_html/dandin-system/api-laravel/vendor/tymon/jwt-auth/src/Tymon/JWTAuth/Providers/AbstractProvider.php"
line: 105
message: "A token is required"
type: "Tymon\JWTAuth\Exceptions\JWTException"

my Edit on decode method

public function decode($token){
    $this->createToken($token);
    try {
        $payload = (array) Firebase::decode($this->token, $this->secret);
    } catch (Exception $e) {
        // ignore firebase's expired exception because we will throw our own later
        if ($e->getMessage() !== 'Expired Token') {
            throw new JWTException('Could not decode token: ' . $e->getMessage());
        } else {
            return false;
       }
   }
       return $this->createPayload($payload);
   }

@tymondesigns
Copy link
Owner

Sorry about that guys.. should be fixed now. (0.3.10)

The firebase provider obviously doesn't return the payload if an expired exception is thrown, and that's where it went wrong. doh!

I will try and come up with a better solution to this, as I complete the refactor over on develop

@felnne
Copy link
Author

felnne commented Dec 1, 2014

Hi,

That (almost) works for me. The correct exception is thrown but I had to add a use statement for theTokenExpiredException exception in FirebaseProvider.php (well my IDE told me to at any rate),

i.e.

use Tymon\JWTAuth\Exceptions\TokenExpiredException;

As an aside i'm told these two references aren't needed:

use Tymon\JWTAuth\Providers\AbstractProvider;
use Tymon\JWTAuth\Providers\ProviderInterface;

@tymondesigns
Copy link
Owner

yes, you are right, don't know what was going on in my head this weekend! 😣

I think I'll be using php storm from now on!

0.3.11 tagged

@migaber
Copy link

migaber commented Dec 1, 2014

Solved from my side too,
but I think you missed the Event Listener Event::listen('tymon.jwt.expired') ??

@tymondesigns
Copy link
Owner

@migaber good point, i'll edit the release a little later for that.

Tbh I'm not overly fond of having those two places that handle token expiry.. I will have a think on a better solution for this

@felnne
Copy link
Author

felnne commented Dec 1, 2014

0.3.11 works for me (I don't use the events so I didn't think about that).

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

3 participants