Add refresh token rotation feature

[formatCvt]

Added refresh token rotation feature (disabled by default). More info about this:
https://auth0.com/blog/refresh-tokens-what-are-they-and-when-to-use-them/#Refresh-Token-Rotation
https://dev.to/jobber/refresh-token-rotation-what-why-and-how-2eh

[sesposito]

Hello @formatCvt, thank you for the PR. Left a minor comment, if you could address it we'll accept the changes 👍

[sesposito]

We think it'd be best if this defaults to true.

[formatCvt]

When we enable this feature on server side we should disable session auto-refresh on client side. Because auto-refresh on client side can leads to token refresh race condition (for example client calling two RPC on same time - two simultaneous calls to refresh token as result)

[formatCvt]

if you feel ok with this, i will set defaults to true

[redbaron]

previously refresh token was immutable once created, so clients could persist it on login once and then ignore returned refresh token from the refresh session api call. if we turn it on by default this change should be listed prominently in release notes as potential breaking change.

[sesposito]

@redbaron in the end we decided not to turn it on by default as this requires some coordination with our SDKs release and as you mention is a breaking change.

[formatCvt]

Hello @sesposito, what do you think about my comment?

[sesposito]

We've decided to approve this as is, we'll default it to true in a future release after the SDKs have been updated to correctly handle the possible race condition. Thank you for your contribution 👍

[redbaron]

If the goal is to protect from replay attacks, then tracking refresh token reuse is key component of refresh token rotation and it is not implemented here. Any reuse of rotated refresh token must invalidate all subsequent refresh tokens with it.

[formatCvt]

My main goal is refresh token with short time to live (for example if player not playing for 5 min then should re-auth, but if they continue playing can play hours and hours without re-auth)

Protect from replay attacks looks like additional feature for me. Because for games with million players store and validate all refresh tokens for the player looks like overhead and there are should be a way to turn it off but keep refresh token rotation technic

@sesposito what do you think about this?

[redbaron]

My main goal is refresh token with short time to live (for example if player not playing for 5 min then should re-auth, but if they continue playing can play hours and hours without re-auth)

Does it require refresh token rotation? refresh_token_expiry_sec = 5 mins will require users to re-auth unless game makes SessionRefresh call in that window, which it can do while player is playing

[formatCvt]

Does it require refresh token rotation? refresh_token_expiry_sec = 5 mins will require users to re-auth unless game makes SessionRefresh call in that window, which it can do while player is playing

Yes, without refresh token rotation user should re-auth every 5 min because there is no way to get new access token with expired refresh token

[sesposito]

@redbaron raised some pertinent questions, we want to include refresh token support but would like to take the chance to also resolve some of the potential security implications and also make some additional improvements. This requires some more extensive changes so we'll come back to this soon.

Requires some more extensive changes

[formatCvt]

What we should do next? How can i help with this? There are two ways i think:

1. Use nested maps instead of map to store refresh token session expiry and token status (valid/revoked)
2. Add one more map to store revoked tokens

Add some logic to work with token revoked status to SessionCache implementation (IsValidRefresh, Remove, etc. functions) and disconnect all user sessions via sessionRegistry.Disconnect when we found revoked token usage.

@sesposito @redbaron

[sesposito]

@formatCvt we'll make sure to have this included in the next release, there's a few things we need to consider and discuss internally.

[zyro]

Based on this discussion, a slightly tweaked version has landed with fd38186. Many thanks @formatCvt for bringing this up! 🙇