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

Tokens Expired, Access Still Allowed #12

Closed
mrforsythexeter opened this issue Dec 13, 2016 · 22 comments
Closed

Tokens Expired, Access Still Allowed #12

mrforsythexeter opened this issue Dec 13, 2016 · 22 comments

Comments

@mrforsythexeter
Copy link

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
Copy link

@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
Copy link
Owner

@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
Copy link
Author

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
Copy link

paolopiccinini commented Dec 13, 2016

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
Copy link
Author

@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
Copy link

paolopiccinini commented Dec 13, 2016

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
Copy link
Owner

@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
Copy link

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
Copy link
Author

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
Copy link
Owner

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

@dusterio
Copy link
Owner

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

@DCdeBrabander
Copy link

@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
Copy link
Author

@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
Copy link

paolopiccinini commented Dec 14, 2016

@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 the LumenPassport::tokensExpireIn(Carbon::now()->addMinutes(1),2); in AppServiceProvider.
@mrforsythexeter this seems really strange. Where have you put that call to function?

@mrforsythexeter
Copy link
Author

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
Copy link

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

@mrforsythexeter
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link

:))) 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
Copy link
Author

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

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

@DCdeBrabander
Copy link

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

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

4 participants