Expire OAuth2AuthorizationRequest when saving to the session

[candrews]

Closes gh-5145

This straightforward approach cleans up expired OAuth2AuthorizationRequests as they're loaded (and subsequently saved) to the session. It's basically the same approach as #7381 but storing creation instead of expiration and using a wrapper class instead of modifying OAuth2AuthorizationRequest.

This approach introduces no new dependencies or requirements (such as Spring Cache abstraction).

From #7381 (review)

The expiring capability is really a cross-cutting concern. Can you try the following solution?

* Intercept calls to `loadAuthorizationRequest()` and store the information necessary

* At a scheduled time (configurable with skew), the component will attempt to `loadAuthorizationRequest()` and if it returns than determine if time has expired and than `removeAuthorizationRequest`

Please take a look at the Cache abstraction to see if it would meet our needs.

For HttpSessionOAuth2AuthorizationRequestRepository, I don't believe that could work because the session can only be loaded and saved while the request for it is being processed. In other words, the task that runs at the scheduled itme would not be able to call loadAuthorizationRequest() and get the data back, nor would it be able to call removeAuthorizationRequest to write it. Even if it could call those methods, they wouldn't work right; session persistence is handed by servlet filters (see Spring Session) and those filters wouldn't be intercepting session use to handle persistence outside of a request.

It make sense to create a new implementation of the AuthorizationRequestRepository<OAuth2AuthorizationRequest> interface at some point which uses a database or Spring Cache abstraction for persistence, as that would allow for expiration to run as a scheduled job. However, that's not of interest to me at the moment :)

[candrews]

@jgrandja can you please take a look at this PR?

[jgrandja]

Thanks for the PR @candrews ! Please see review comments.

[candrews]

Thanks for the PR @candrews ! Please see review comments.

I've addressed feedback in a fixup commit I added. I'm assuming that the commits will be squashed when the PR is merged.

Thank you for your review!

[candrews]

@jgrandja Is there anything else you'd like me to do?

Thanks again

[candrews]

I've updated this PR to also set a maximum number of OAuth2AuthorizationRequest per session, which is important based on my own testing and was suggested at #5145 (comment)

@jgrandja can you please take a peek?

[jgrandja]

@candrews Thanks. I will look at this shortly.

[jgrandja]

Thanks for the updates @candrews. Please see review comments.

[jgrandja]

Please remove this as it's not needed. The clock will always be in sync since this implementation operates independently within it's own application instance. Clock and clock skew is needed when components are interacting in a distributed way.

[candrews]

This method is package private, so nothing other than Spring itself should be using it.

Being able to set the clock is important for testing. See https://github.com/candrews/spring-security/blob/9b1a85d992f3ac0d725f67ac03ee6d9f44659b21/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepositoryTests.java#L249

Is there another approach that should be taken to make the clock available to be set by tests?

[jgrandja]

Can you explain the specific use case why this is needed?

[candrews]

It's possible to easily put a lot of OAuth2AuthorizationRequests in this map, resulting in Bad Things ™️ happening. This limit prevents that from being possible.

I contacted security@pivotal.io with the details. Let me know if I should post them here.

[candrews]

How's this looking now?

[jgrandja]

@candrews I've been giving this enhancement some further thought and I would prefer to keep this a patch change.

Something about setAuthorizationRequestTimeToLive(), setMaxActiveAuthorizationRequestsPerSession(), and setClock() doesn't feel right from a design standpoint.

Furthermore, I'm not sure how much these configuration-related settings will be used given that gh-5145 did not receive any upvotes and the issue is now over 3 years old.

HttpSessionOAuth2AuthorizationRequestRepository is designed to be a simplified (default) version of AuthorizationRequestRepository. However, if an application requires more advanced capabilities than it can configure a custom AuthorizationRequestRepository via oauth2Client() or oauth2Login().

Can you provide me more details on your use case and why you need this enhancement?

[candrews]

Something about setAuthorizationRequestTimeToLive(), setMaxActiveAuthorizationRequestsPerSession(), and setClock() doesn't feel right from a design standpoint.

Furthermore, I'm not sure how much these configuration-related settings will be used given that gh-5145 did not receive any upvotes and the issue is now over 3 years old.

I'm happy to change those methods to be package visible (so they can be used by tests, unless you have a better approach for changing them to facilitating testing) instead of public. I don't really care if they're configurable, and as you said, I doubt anyone else would either.

Can you provide me more details on your use case and why you need this enhancement?

I don't care about the configurability of the expiration or size parameters, but I do think having these limits is very important.

As it stands now, Spring Security sets no limits, allowing the map to grow boundlessly. As @rwinch says:

It is technically a memory leak and sessions can be kept around quite a long time.

-- #5145 (comment)

I'm trying to not give away too much; having this map grow boundlessly is a real significant problem. If you'd like me to give the specifics, let me know and I will.

[jgrandja]

@candrews

If you'd like me to give the specifics, let me know and I will.

No, I'm good. I got the message and much appreciated.

I have another idea brewing for this patch. Let me think it over a bit. I'll get back to you by end of day tomorrow the latest.

[jgrandja]

@candrews Here is a template for the solution.

I've added implementation notes but let me know if you have any questions.

/*
 * 1) OAuth2AuthorizationRequest's are stored as Map<String, OAuth2AuthorizationRequestWrapper>, keyed by getState()
 * 2) When saveAuthorizationRequest(), constrain limit by grouping by OAuth2AuthorizationRequestWrapper.getRegistrationId().
 * 		For example, if there are 3 existing `google-registration-id` then reject the save by throwing IllegalStateException.
 * 		I think constraining to 3 for the limit-based strategy is ok?
 * 3) When loadAuthorizationRequest(), remove all isExpired() before returning the Map
 */
private static class OAuth2AuthorizationRequestWrapper implements Serializable {
	private static final long serialVersionUID = SpringSecurityCoreVersion.SERIAL_VERSION_UID;

	private static final Duration DEFAULT_TIME_TO_LIVE = Duration.ofMinutes(5);

	private final OAuth2AuthorizationRequest authorizationRequest;

	private final Instant createdAt;

	private final Instant expiresAt;

	private OAuth2AuthorizationRequestWrapper(OAuth2AuthorizationRequest authorizationRequest) {
		this.authorizationRequest = authorizationRequest;
		this.createdAt = Instant.now();
		// I think 5 minutes is long enough to allow for the authorization (consent) flow to complete?
		this.expiresAt = this.createdAt.plus(DEFAULT_TIME_TO_LIVE);
	}

	private OAuth2AuthorizationRequest getAuthorizationRequest() {
		return this.authorizationRequest;
	}

	private String getRegistrationId() {
		return this.authorizationRequest.getAttribute(OAuth2ParameterNames.REGISTRATION_ID);
	}

	private String getState() {
		return this.authorizationRequest.getState();
	}

	private boolean isExpired() {
		// NOTE:
		// Given that Instant.now() uses the system clock and
		// createdAt and expiresAt is instantiated by the same system clock,
		// there is no need for a configurable Clock or any clock skew adjustment
		return Instant.now().isAfter(this.expiresAt);
	}
}

[candrews]

1. OAuth2AuthorizationRequest's are stored as Map<String, OAuth2AuthorizationRequestWrapper>, keyed by getState()

That's already in place. Based on earlier feedback, the class is named OAuth2AuthorizationRequestReference, not OAuth2AuthorizationRequestWrapper, though.

2. When saveAuthorizationRequest(), constrain limit by grouping by OAuth2AuthorizationRequestWrapper.getRegistrationId().
   For example, if there are 3 existing google-registration-id then reject the save by throwing IllegalStateException.
   I think constraining to 3 for the limit-based strategy is ok?

If we throw IllegalStateException, then the user won't be able to authorize using that registration id for the remainder of their session or until the 2 minute expiration occurs clearing that OAuth2AuthorizationRequestWrapper from the map.

Perhaps instead of throwing IllegalStateException, we should remove the oldest OAuth2AuthorizationRequestWrapper for that registration id and continue?

I've just made that change and pushed the commit for it.

3. When loadAuthorizationRequest(), remove all isExpired() before returning the Map

Already done.

I kept setClock though as a package private method, as it facilitates testing. If you have a better approach, please let me know.

Also, the commit history is getting excessive. Would you like me to squash all the commits and force push update this PR?

[candrews]

@jgrandja here's an alternative, very simple approach that also addresses the root issue:
#9649

[jgrandja]

@candrews I'll close this one in favour of gh-9649