-
Notifications
You must be signed in to change notification settings - Fork 436
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
refresh token rotation should be optional (fixes #1338) #1342
Conversation
Hi @mduriancik firstly thanks for the PR. I'm thinking about this. I think we need an allowUnsafeReUseRefreshToken: boolean configuration to activate this feature. Secondly not sure about having the refresh token saved twice to the session storage. Not sure of a solution at present. Greetings Damien |
Any news on this? |
Hi Fabien, Thank's for your feedback. Yes, good idea, I can add the property allowUnsafeReuseRefreshToken (default true?). Yes it is not quite "DRY" to store the refresh token twice (authResult and refresh_token), but as refresh_token can outlive the immediate authResult it looks to me the simplest solution. Other solution: to modify the new authResult transfering the refresh_token form the previous authResult -> It looks to me less "elegant", but if you prefer this one I can change my PR. Do you see there other possibilities? Milan |
allowUnsafeReuseRefreshToken (default false?). We do not want to encourage this, this should only be used if the IDP is missing the feature. This is not a good idea and I'm not even sure we should support this. Maybe ADFS federation to AAD for public IDP would be better. Greetings Damien |
added allowUnsafeReuseRefreshToken
|
Hi @mduriancik I will add this to the next release , Thanks! Greetings Damien |
What is the status on this? i can see the build didnt go through |
Proposed solution to #1338. The idea is to separate the refresh token from authResult in the storage.
I have tested the solution and it resolves my problem.
Of course more refactoring and more tests could be needed.