dusterio / lumen-passport

Making Laravel Passport work with Lumen
MIT License
654 stars 141 forks source link

Tokens Expired, Access Still Allowed #12

Closed mrforsythexeter closed 7 years ago

mrforsythexeter commented 7 years ago

I think this maybe in the passport layer, however I am not completely sure.

I have set both the ttl of the token and the fresh to 1, and 2 minutes. Then waited 5 minutes. I can still access the routes. This is a password grant, not that this should matter.

DCdeBrabander commented 7 years ago

@mrforsythexeter mmm. that's interesting indeed. I'm using the password grant with custom TTL aswell. @dusterio can you please update us on this?

dusterio commented 7 years ago

@mrforsythexeter can you try to copy/paste your JWT token to https://jwt.io/ and see the expires value? definitely not a correct outcome

mrforsythexeter commented 7 years ago

After some deeper debugging and even though I am setting this

LumenPassport::tokensExpireIn(Carbon::now()->addMinutes(1));

in AuthServiceProvider boot function, it appears to be ignoring this completely and setting it to 1 year in the future (for the expires).

I traced it though passports tokensExpireIn function, and it is setting the static correctly. So I now assume something else is changing the value. I will dig deeper.

The token was created_at/update_at : "2016-12-13 10:49:54" expires_at : "2017-12-13 10:49:54"

The refresh token expires_at : "2016-12-13 10:51:53"

The JWT "exp": 1513162194, ---- 12/13/2017 @ 10:49am (UTC)

Changing the value in the database has no effect. I assume this is because the JWT is trusted?

paolopiccinini commented 7 years ago

Ok so i redit this :) @mrforsythexeter you were setting LumenPassport::tokensExpireIn(Carbon::now()->addMinutes(1)) in the boot an then when ask for a token it resets the expire in Dusterio\LumenPassport\Http\Controllers\AccessTokenController.php

// Overwrite password grant at the last minute to add support for customized TTLs
            $this->server->enableGrantType(
                $this->makePasswordGrant(), LumenPassport::tokensExpireIn(null, $clientId)
            );
mrforsythexeter commented 7 years ago

@paolopiccinini you were right. If I set (with clientid) in boot...

LumenPassport::tokensExpireIn(Carbon::now()->addMinutes(1),2);

Then it takes my time, and the token expire's as expected.

However I was under the impression this was an optional feature of this bridge, but this line doesn't make it optional as the Dusterio\LumenPassport\LumenPassport tokensExpireIn is expecting the clientId? Or is this my mistake because I am using client ids?, following the instructions, 2 clients where created for me, so I used them?

Also should the JWT be trusted? So when the value in the database updates (or is different in the exp), the JWT should no longer be valid?

paolopiccinini commented 7 years ago

you have to watch in Dusterio\LumenPassport\LumenPassport::tokensExpireIn

public static function tokensExpireIn(DateTimeInterface $date = null, $clientId = null)
    {
        if (!$clientId) return Passport::tokensExpireIn($date);

        if (is_null($date)) {
            return isset(static::$tokensExpireAt[$clientId])
                ? Carbon::now()->diff(static::$tokensExpireAt[$clientId])
                : Passport::tokensExpireIn();
        } else {
            static::$tokensExpireAt[$clientId] = $date;
        }

        return new static;
    }

if you don't set the client id in the boot, you update the expire time correctly. But not the expire of that client. In the iusseToken you instead are setting the expire of yourt clientId, whitch is not set ant then get 1 year.

dusterio commented 7 years ago

@mrforsythexeter yes JWT can be trusted - that's the whole idea, they may be 100% verified at the client side that has a public key of the token issuer. That's why 'auth' middleware fully trusts JWT including its expiration date and scopes, so changing them in the database later doesn't help.

paolopiccinini commented 7 years ago

there is something i'm not understanding: 1) if you don't set the clientId in the boot, it seems this have to happen: go in LumenPassport, clientId is null set Passport::tokensExpireIn to your date. Ask for a token, call LumenPassport(null, clientId), now the client id is set, but not it's $tokensExpireAt so we take Passport::tokensExpireIn() we previous setted. Am i wrong?

2) if you set the clientId in the boot, it seems this have to happen: go in LumenPassport clientId != null so this line it's executed: static::$tokensExpireAt[$clientId] = $date; no one now is calling passport. Ask for a token, call LumenPassport(null, clientId), isset(static::$tokensExpireAt[$clientId]) is true, so this line it's executed: Carbon::now()->diff(static::$tokensExpireAt[$clientId]). No one again it's calling Passport.

How is this working. There something i'm missing. Could someone explain?

mrforsythexeter commented 7 years ago

Its because the bridge class is looking for

static::$tokensExpireAt[$clientId]

which is expecting the $clientId to be found in array in $tokensExpireAt.

Since the the this call to function is passing the client_id (as I have in the header request). and if I havn't set it before hand in the boot, then it will use the default P1Y (1 year).

personally I would think that this function should look at the default (or what has been set) in passports static::tokensExpireAt before asumming that 1 year is the right fall back.

However I am not 100% sure how this static is working.

dusterio commented 7 years ago

What's the current status - are you still experiencing this problem?

dusterio commented 7 years ago

FYI, I've added some unit tests that cover this and they seem to pass ok

DCdeBrabander commented 7 years ago

@dusterio @mrforsythexeter @paolopiccinini I would like to know this as well. I am using this package in a production environment, and this would be a important issue!

mrforsythexeter commented 7 years ago

@DCdeBrabander Sorry I am on another project at the moment and most likely wont get back to this one until late tomorrow. When using the client Id for setting the expiry, it appears to be working.

paolopiccinini commented 7 years ago

@DCdeBrabander it works, i've made more tests today and there is no bug. I've tested with or without the client id. I've put theLumenPassport::tokensExpireIn(Carbon::now()->addMinutes(1),2); in AppServiceProvider. @mrforsythexeter this seems really strange. Where have you put that call to function?

mrforsythexeter commented 7 years ago

The call is in app\Providers\AuthServiceProvider.php boot function. Is this the wrong place? I have the app register it in bootstrap\app.php using

$app->register(App\Providers\AuthServiceProvider::class);
paolopiccinini commented 7 years ago

@mrforsythexeter can you try in AppServiceProvider instead of AuthServiceProdider (also deregistering this from app.php)? maybe something is going wrong there

mrforsythexeter commented 7 years ago

ok, I moved it to the app\Providers\AppServiceProvider.php boot function without the client id in the call, it still sets it to expire in 1 years time. I tried to remove the client_id header from the login auth request, but this just throws an error saying its missing (as you would expect from oauth2).

I did leave the code in place where I had it before, but I placed a DIE statement in the top of the function to make sure that it wasn't being called.

I have taken a look at your tests, and I can see they are fine, however I think its missing the point...

line 31 in AccessTokenController.php does this

$this->makePasswordGrant(), LumenPassport::tokensExpireIn(null, $clientId)

the null then inside Dusterio\LumenPassport\LumenPassport::tokensExpireIn causes it to check

static::$tokensExpireAt[$clientId]

I don't think this is an array if you haven't set the expire using the client_id in the first place. so I assume the isset fails and you get the default

Hope that makes sense.

paolopiccinini commented 7 years ago

mmm i'dont think it's correct. When you first call LumenPassport in the boot without the clientId this is setting Passport::tokensExpireIn($date);. When you call the function the second time in the controller as you say isset fails and you get the previous date settted in passport. I'm not facing this problem so i'dont understand whats going on.

mrforsythexeter commented 7 years ago

How do you get the value in Passport?

 return isset(static::$tokensExpireAt[$clientId])
                ? Carbon::now()->diff(static::$tokensExpireAt[$clientId])
                : new DateInterval('P1Y');

If that if statement fails, you get new DateInterval('P1Y') which is one year in the future.. not anything from Passport

paolopiccinini commented 7 years ago

:))) you have an older version. it's time to update. i've this in LumenPassport

if (is_null($date)) {
            return isset(static::$tokensExpireAt[$clientId])
                ? Carbon::now()->diff(static::$tokensExpireAt[$clientId])
                : Passport::tokensExpireIn();
        } else {
            static::$tokensExpireAt[$clientId] = $date;
        }
mrforsythexeter commented 7 years ago

oh, what I an idiot I am. Sorry for wasting your time..

Right onto my next issue.. yay. Have a great day chaps.

DCdeBrabander commented 7 years ago

@mrforsythexeter @paolopiccinini Glad to read it is an old issue :)