Return 404 and delete the session in refreshAccessToken when the login module does not recognize the refresh token #1242
Conversation
…n module does not recognize the refresh token
…"the table ... should remain unchanged", "the table ... should stay unchanged but the row with ... ..." -> "the table ... should remain unchanged, regardless of the row with ... ...", "the table ... should stay unchanged but the row with ... ... should be deleted" -> "the table ... should remain unchanged, except that the row with ... ... should be deleted"
… expired in swagger comments
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1242 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 246 246
Lines 15226 15233 +7
=========================================
+ Hits 15226 15233 +7 ☔ View full report in Codecov by Sentry. |
| logging.SharedLogger.WithContext(ctx). | ||
| Warnf("The refresh token is invalid for user %d", user.GroupID) |
There was a problem hiding this comment.
Actually, is it expected or not? Can you check (in login-module if needed) if the refresh token has an expiration?
Several options
- it has an expiration and we receive the info: we should check this value before trying the use it
- it has an expiration and we don't know it: maybe it should be just a "info" log... there is nothing to "warn"
- it is not supposed to expire: we should investigate why that happens (with Michel) first
There was a problem hiding this comment.
There are several possible errors returned by the login module on refreshing a token (all with 401 error code, but with different hints, cfr https://github.com/thephpleague/oauth2-server/blob/6.0.2/src/Grant/RefreshTokenGrant.php#L95):
- Cannot decrypt the refresh token
- Token is not linked to client
- Token has expired
- Token has been revoked
In #1208 the hint is "Token has been revoked". So, it's probably unexpected.
The login module doesn't provide us with the expiration time of the newly created refresh token on logging in although the expiration time is inside of an encrypted JSON returned as a refresh token (see https://github.com/thephpleague/oauth2-server/blob/6.0.2/src/ResponseTypes/BearerTokenResponse.php#L29)
As the summary, the refresh token has an expiration date, but we cannot get its value. On the other hand, the expiration is not the only cause of 401 error returned by the login module. Happily it returns some hints on the error allowing us to know the real cause.
There was a problem hiding this comment.
The actual ttl of refresh tokens in the login module is 1 year.
There was a problem hiding this comment.
I checked again at an error we got this morning, it was again "Token has been revoked". Could you check in the php code what it corresponds to? A realistic reason I see for this to happen so often would be that the user disconnects from the authenticator in another tab; from the authenticator directly or logging out from another app using the same authenticator. Could you check if that's possible ?
I am pretty sure it is a "normal" case here but we have to check. If it is a valid case, the message should become an "info" and not a warning.
There was a problem hiding this comment.
The only place where the login module revokes refresh tokens is here: https://github.com/thephpleague/oauth2-server/blob/6.0.2/src/Grant/RefreshTokenGrant.php#L74
Also, some cucumber test steps are renamed here.
Fixes #1208