feat: Session token rotation(M2-11012) - #2105
Conversation
adeiji
left a comment
adeiji
left a comment
There was a problem hiding this comment.
Looks good @sricharan-varanasi ! I left a few comments, not blocking, but let me know what you think.
| } | ||
| ) | ||
| # Mark the old refresh token used, and record the replacement for the grace window. | ||
| await AuthenticationService(session).revoke_token( |
There was a problem hiding this comment.
What are your thoughts on this edge case?
So we're handling quick-succession refreshes, but for parallel requests, being that this is a transaction, there could be an instance where parallel requests call multiple inserts and the race loser's insert fails, which would raise a 500 error and then force a logout in the client. This could be an issue since sometimes the frontend may send multiple requests at practically the same time.
Should we catch that and return the winner's replacement pair from the grace record? Because we know it's already "passed" it just now needs to not send the 500 incorrectly.
| access_token = replacement.access_token | ||
| refresh_token = replacement.refresh_token | ||
| refresh_outcome = "grace_redeemed" | ||
| elif await AuthenticationService(session).is_revoked(InternalToken(payload=token_data)): |
There was a problem hiding this comment.
When retrieving the grace record from Redis, if Redis is down we still get back None, so a legit duplicate refresh would be flagged as theft and revoke the whole family. Should we handle Redis being down differently here? What do you think?
2 participants
Curious open source credit page
📝 Description
🔗 Jira Ticket M2-11012
This PR adds refresh-token rotation for web/admin clients so active sessions can be kept alive indefinitely, while limiting how long a leaked/stolen refresh token stays useful.
Changes include:
familyclaim (the login refresh token's ownjti), shared by every token descended from that login. Optional/backward-compatible - existing tokens without it are unaffected.TokenRotationService: stores a short-lived (60s, configurable) Redis "grace record" mapping a just-rotated refresh token to its replacement pair, and manages family-level revocation via the existing token blacklist table.refresh_access_tokennow branches on the token'sclientclaim:delete_access_token/delete_refresh_token) now revokes the whole token family for web/admin, so logging out with a superseded token can't leave a still-live rotated token behind.loggercalls distinguishing refresh outcomes (rotated,grace_redeemed, reuse detected) for observability.AUTHENTICATION__REFRESH_TOKEN__ROTATION_GRACE_SECONDS(default60).No frontend changes are required to ship this - the refresh response shape is unchanged, and both admin and web already persist whatever token pair a refresh call returns.
✏️ Notes
Stacked on top of
session-token-short-lived-web-admin(short-lived web/admin token lifetimes). Frontend keep-alive/idle-timeout work (admin/web tickets) is separate and not a prerequisite for this to ship safely.