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

Remove issued at from persistant claims #200

Closed

Conversation

ashvin27
Copy link

Remove iat(IssuedAt) from persistant claim to resolve refresh token expiration issue

Description

Checklist:

  • I've added tests for my changes or tests are not applicable
  • I've changed documentations or changes are not required
  • I've added my changes to CHANGELOG.md

@mfn
Copy link
Contributor

mfn commented Dec 11, 2022

why?

@jansgescheit
Copy link

Because a token cannot be extended beyond the Refresh TTL. After a refresh, the new token retains the IAT of the very first token. This means that after the expiry of the Refresh TTL of the first generated token, the user still has to log in again.

@@ -181,7 +181,7 @@ protected function buildRefreshClaims(Payload $payload)
$persistentClaims,
[
'sub' => $payload['sub'],
'iat' => $payload['iat'],
// 'iat' => $payload['iat'],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility, iat should be included in the persistent_claims config and marked as Breaking Change accordingly

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jansgescheit one question: can we add the iat in $persistentClaims conditionally if it exists on payload?

In that way, it'll not crash if someone wants to use a custom one.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding, the iat is always included in the payload. Therefore, not much will be achieved here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we cannot just remove it right? On that case should be additionally added with a if condition if for some reason the user doesn't want to have it in persist claims.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense to avoid a breaking change. Perhaps via a configuration switch with a denylist for filtering

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so let's add the if to it for that we can merge it.

Thanks

@jansgescheit
Copy link

Because a token cannot be extended beyond the Refresh TTL. After a refresh, the new token retains the IAT of the very first token. This means that after the expiry of the Refresh TTL of the first generated token, the user still has to log in again.

Okay, I see in the JWT specs that this is how the refresh TTL is supposed to work.

@specialtactics
Copy link

I don't really follow why it would cause the user to have to log in again, we use this package without that problem.

If you wanted a custom set of claims, you can do that too already?

@jansgescheit
Copy link

jansgescheit commented Dec 20, 2022

The problem is, that after the refresh the new token get's the iat from the old token. So if your Refresh TTL is 2 weeks your users have to login again with their credentials. The iat is not rolling with the last refresh but nailed to the very first token.

Example:

TTL = 60min
Refresh TTL = 2 weeks

  • Login -> Token with iat = 2022-12-01 00:00:00
  • Api Call at 2022-12-13 23:00:00 -> 401 Token expired (TTL expired, refreshable)
  • Refresh Call -> new Token with iat from the login token because is persistent (2022-12-01 00:00:00)
  • Api Call at 2022-12-14 00:00:00 -> 401 Token expired (TTL expired)
  • Refresh Call -> "422 Token has expired and can no longer be refreshed", because iat is older than Refresh TTL
  • User has to Login again with their credentials

@jansgescheit
Copy link

Here is another article which also addresses the problem in this package

@Messhias
Copy link
Collaborator

Thanks, everyone for contributing, I've tested here and so far I have got any issues, we're good to merge this update?

Thanks.

@Messhias
Copy link
Collaborator

@eschricker what are your thoughts about that?

@Messhias
Copy link
Collaborator

@ashvin27 can you do the requested changes in the comments?

@Messhias Messhias closed this Jul 19, 2023
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

Successfully merging this pull request may close these issues.

None yet

5 participants