Support multiple refresh tokens per user.

[ryotarai]

fixes #981

This PR introduces enableMultiRefreshTokens option into config to enable multi refresh tokens per user.
In our environment, each user uses multiple id tokens in laptops so this feature is very helpful.

Details

• enableMultiRefreshTokens option defaults to false and in that case this PR does not change any behavior.
• When enableMultiRefreshTokens is true, Dex skips to delete a refresh token in issuing an id token.
• To minimize changes in storage layer, ListRefresh and RevokeRefresh gRPC API use Storage.ListRefreshTokens in enableMultiRefreshTokens mode. This can be heavy but num of refresh tokens not is expected very high (same as num of id tokens).
• storage.OfflineSessions.Refresh contains the latest-issued refresh token.

[tkleczek]

I appreciate your work @ryotarai and I would welcome "multiple refresh token per user" feature, but I have some problems with how this change is designed:

1. Essentially the OfflineSessions.Refresh[clientID] now points to the last created refresh, but not necessarily last updated refresh.
   This can be confusing as I would naturally expect OfflineSessions.Refresh[clientID].LastUsed to give me info when was the last time refresh was performed for this client.

I would also expect OfflineSessions.Refresh[clientID] to store information about all issued refresh tokens. (I know this is much more challenging as would require schema changes and storage plugins update)

2. Also with multiple tokens now the number of refresh tokens could grow indefinitely so it would be good to introduce some (potentially configurable) limit to this. But this raises question: what to do when limit is reached. Probably overwriting least recently used would be a good thing?

3. Another point is this change potentially weakens security. Currently if user logs-in again their refresh token gets overwritten. So even if some old refresh token is leaked - it's most likely invalid at this point. With this feature on, it's more likely that the old refresh token can still be valid. Maybe refresh token expiry should be considered to mitigate this?

[ryotarai]

@tkleczek First of all, thank you for reviewing!

1. Essentially the OfflineSessions.Refresh[clientID] now points to the last created refresh, but not necessarily last updated refresh.
   This can be confusing as I would naturally expect OfflineSessions.Refresh[clientID].LastUsed to give me info when was the last time refresh was performed for this client.

I see. How about making OfflineSessions.Refresh[clientID] point to last updated refresh instead of last created one?

I would also expect OfflineSessions.Refresh[clientID] to store information about all issued refresh tokens. (I know this is much more challenging as would require schema changes and storage plugins update)

Yes, it is more challenging. Is it acceptable to let OfflineSessions.Refresh[clientID] point to only last updated one? If no, I'll think about schema change...

2. Also with multiple tokens now the number of refresh tokens could grow indefinitely so it would be good to introduce some (potentially configurable) limit to this. But this raises question: what to do when limit is reached. Probably overwriting least recently used would be a good thing?

It grows more than multi refresh is off, but I do not expect it to grow too much because refreshing token only updates an existing refresh record. However, I agree it would be good to have limit and overwrite old refreshes on LRU basis.

Another point is this change potentially weakens security. Currently if user logs-in again their refresh token gets overwritten. So even if some old refresh token is leaked - it's most likely invalid at this point. With this feature on, it's more likely that the old refresh token can still be valid. Maybe refresh token expiry should be considered to mitigate this?

I understand. I'll implement refresh token expiry.

[tkleczek]

I see. How about making OfflineSessions.Refresh[clientID] point to last updated refresh instead of last created one?

I'd say it would be more consistent, but I still would like schema change much more.

Yes, it is more challenging. Is it acceptable to let OfflineSessions.Refresh[clientID] point to only last updated one? If no, I'll think about schema change...

I think this would be sth for maintainers to decide.

It grows more than multi refresh is off, but I do not expect it to grow too much because refreshing token only updates an existing refresh record. However, I agree it would be good to have limit and overwrite old refreshes on LRU basis.

Each successful user authentication with "offline_access" scope requested would now create a new refresh token object. They are small, but so far all db objects were either bounded in number (either via short expiry time or limited to user x connector x client).

I understand. I'll implement refresh token expiry.

This could go in a separate PR as it's in fact independent of multiple refresh token support. There were some issues opened around this functionality, e.g. #1685.

[tkleczek]

@ryotarai fiy: refresh token expiration is already being implemented as part of #1846

[tanmaykm]

Thanks @ryotarai! We were looking for something like this in dex, it will be very useful along with #1846. Happy to pitch in if anything needed here!

[vinod-trilio]

@ryotarai We are looking for this functionality in dex. Will be more than happy to help you if needed.

[ryotarai]

Hi, @tanmaykm @vinod-trilio
I'm sorry but I don't have time to implement this, so I'm going to close this PR.
It would be great if you take over this work!