uaa icon indicating copy to clipboard operation
uaa copied to clipboard

fix: always rotate refresh tokens for public clients

Open mikeroda opened this issue 10 months ago • 4 comments

When refreshing a token, always rotate for public clients, thus not requiring rotation to be enabled for all clients and preventing the possible error condition for public clients.

mikeroda avatar Apr 22 '24 14:04 mikeroda

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187475484

The labels on this github issue will be updated when the story is started.

cf-gitbot avatar Apr 22 '24 14:04 cf-gitbot

@mikeroda this PR has conflicts from the begin, so you need to resolve conflicts / rebase your branch

strehle avatar Apr 24 '24 18:04 strehle

@mikeroda will check the PR and we take it on the list , means if we do not see a problem it should come into dev , end of next week

strehle avatar Apr 26 '24 16:04 strehle

Sonar, ok: https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2846

strehle avatar May 01 '24 16:05 strehle

Hi we have not looked at this PR yet.

peterhaochen47 avatar May 13 '24 19:05 peterhaochen47

Hi we have not looked at this PR yet.

Hi we have not looked at this PR yet.

Ok can you please look and vote if it needs to be reverted

strehle avatar May 13 '24 19:05 strehle

We do have a public client. This change seems like a breaking change?

  1. Previously, whether the refresh token is rotated is controlled by the uaa.jwt.refresh.rotate flag and defaulted to false to maintained backward compatibility. Correct?

This current change makes an exception to the flag and always rotate public clients's refresh token, and potentially breaking the backward compatibility brought by the flag.

  1. Separately, this might also be confusing to the users when they set the uaa.jwt.refresh.rotate flag to false but still see their refresh tokens being rotated?

thus not requiring rotation to be enabled for all clients and preventing the possible error condition for public clients

Could you explain this? If you wish to rotate the refresh tokens, why not just set uaa.jwt.refresh.rotate: true to enable that for all clients? What "possible error condition" would that create for public clients?

peterhaochen47 avatar May 14 '24 21:05 peterhaochen47

Ultimately we want to get to always rotating refresh tokens for public clients. The problem with the flag is it affects all clients. This doesn't seem like an adverse change because if you didn't set the flag and attempted to refresh a token with a public client, you would get "Refresh without client authentication not allowed" error. Now you would get a successful response and newly rotated refresh token. So I don't see how this breaks anything.

mikeroda avatar May 14 '24 22:05 mikeroda

if you didn't set the flag and attempted to refresh a token with a public client, you would get "Refresh without client authentication not allowed" error

I see. Currently, UAA doesn't allow token refresh for public clients when uaa.jwt.refresh.rotate: false? (is this documented somewhere?). Then yes, this is a non-breaking change.

peterhaochen47 avatar May 14 '24 22:05 peterhaochen47