We should ensure the OAuth2AuthorizationRequest expires after the configured time limit.

Comment From: jgrandja

@rwinch I've been thinking about assigning this as a first-timer issue. However, as I looked into this further, I'm not sure if we really need to implement this. The current and only implementation for AuthorizationRequestRepository is HttpSession-based, including it's reactive counterpart. So the OAuth2AuthorizationRequest(s) will be removed whenever a session is terminated anyway. Although we can be more proactive in expiring OAuth2AuthorizationRequest(s) before a session is terminated, I'm not sure there is value here. What are your thoughts?

Comment From: rwinch

Can you remind me why we want to expire it? Where is the configured limit coming from?

Comment From: jgrandja

@rwinch You proposed this change to me way back and I logged the issue as I thought it was a good idea at the time as well.

Can you remind me why we want to expire it?

If the user triggers the Authorization Code flow OR the oauth2Login() flow and does not proceed through to the end of the flow, for example, closes the browser at the consent screen, than the OAuth2AuthorizationRequest will essentially be orphaned in the AuthorizationRequestRepository until the session is expired. IMO, I don't see an issue with this? Do you?

Where is the configured limit coming from?

This would be part of the feature if implemented. I'm guessing we would have a setter in the implementation(s) of AuthorizationRequestRepository.

Comment From: rwinch

Thanks for the reminder.

It's probably not a huge deal, but I'd like to leave this open. It is technically a memory leak and sessions can be kept around quite a long time.

Comment From: AndreasKl

@rwinch @jgrandja We recently observed this issues which caused an increase in per call overhead in our gateway, due to near 4k of OAuth2AuthorizationRequest attached to a few sessions.

As I did not want to invalidate existing sessions a companion map was added to the session holding a configurable amount of OAuth2AuthorizationRequest state keys.

If you like I could provide a PR for this change? Imho I would prefer a limit based solution over a time based one as a possible attacker could just create a lot of OAuth2AuthorizationRequest just by coincident.

Comment From: jgrandja

@AndreasKl I believe this solution should have both time-limit and size-limit. The size-limit will prevent resource exhaustion and the time-limit will ensure resources are not wasted.

For the time-limit feature, I believe an OAuth2AuthorizationRequest should expire after about 1-2 minutes as the default (should be configurable). I think this makes sense as a user should proceed past the consent screen after about a minute. Anything longer might mean the user aborted the authorization process and left the OAuth2AuthorizationRequest in an orphaned state.

With the time-limit feature alone, I wonder if the size-limit would be needed?

Comment From: AndreasKl

@jgrandja Ok, started to build a solution for the time based expiration (the prototype can be found in this draft PR)

Now I have a few questions.. * I'm not sure if 60 seconds are enough, considering time divergences between badly configured servers without NTP in a on-premise cloud. Should we go with 120 seconds or should I add a skew?

  • Should the cleanup become a exchangeable component. This component could receive the whole session attributes with the benefit of storing state in the session and could share the same implementation between webflux and servlet. Currently it is only one duplicated line so my opinion is more like the "it's not big enough for it's own class".

  • The expireAt is set to null if it should not expire. This has the benefit that I do not have to break the contract of the builder as I otherwise have no easy way to provide a custom Clock instance to the builder. I'm not sure if this is acceptable from an user experience view?

  • I think it is good enough to clean-up on save, but I'm not really sure. What do you think?

Comment From: jgrandja

@AndreasKl

Should we go with 120 seconds or should I add a skew?

We should definitely add a skew setting and Clock. Maybe 120 secs is better for default.

Should the cleanup become a exchangeable component This component could receive the whole session attributes with the benefit of storing state in the session and could share the same implementation between webflux and servlet.

I would prefer a component that can be used between webflux and servlet. This would be ideal of course but we'll need to ensure we're not blocking on the reactive impl.

I think it is good enough to clean-up on save, but I'm not really sure. What do you think?

I still feel this is a cross-cutting concern so I would prefer to externalize the expiry logic and have it scheduled. I'd rather not embed the logic in load/save/remove

Comment From: rwinch

Closing in favor of gh-9649