Expected Behavior

The implementation does not store ClientRegistration in WebSession but uses ReactiveClientRegistrationRepository to resolve from stored registrationId.

Current Behavior

The current implementation of WebSessionServerOAuth2AuthorizedClientRepository stores the entire OAuth2AuthorizedClient object in the WebSession. This does include ClientRegistration including clientId and clientSecret.

Using the default ServerOAuth2AuthorizedClientRepository implementation (i.e. AuthenticatedPrincipalServerOAuth2AuthorizedClientRepository) relies on a ReactiveOAuth2AuthorizedClientService implementation which might be InMemoryReactiveOAuth2AuthorizedClientService depending on config. This does not seem viable for use with spring-session.

Context

Using it with spring-session will serialize the registration data for every user (again, including clientId and clientSecret). This does have performance and potential security implications. A simple fix would be storing and resolving an intermediary object like this:

    private <T extends OAuth2AuthorizedClient> Mono<T> toAuthorizedClient(final StoredOAuth2AuthorizedClient stored) {
        if (stored == null) {
            return Mono.empty();
        } else {
            // TODO handle unknown registrationId?
            return (Mono<T>) _clientRegistrationRepository.findByRegistrationId(stored.getRegistrationId())
                    .map(stored::toOAuth2AuthorizedClient);
        }
    }

    @Data
    private static class StoredOAuth2AuthorizedClient implements Serializable {

        private String _registrationId;
        private String _principalName;
        private OAuth2AccessToken _accessToken;
        private OAuth2RefreshToken _refreshToken;

        public StoredOAuth2AuthorizedClient(@NonNull final OAuth2AuthorizedClient client) {
            _registrationId = client.getClientRegistration().getRegistrationId();
            _principalName = client.getPrincipalName();
            _accessToken = client.getAccessToken();
            _refreshToken = client.getRefreshToken();
        }

        public OAuth2AuthorizedClient toOAuth2AuthorizedClient(@NonNull final ClientRegistration clientRegistration) {
            return new OAuth2AuthorizedClient(clientRegistration, _principalName, _accessToken, _refreshToken);
        }

    }

Comment From: Annaseron

The memory consumption of a single WebSession is quite significant，approaching 20KB. I would like to introduce a hybrid approach to store this information,keeping clients information in memory since usually there won't be many clients, and storing users' tokens along with the corresponding client id in redis.

Comment From: sjohnr

Thanks for the enhancement suggestion, @sfussenegger!

By default, authenticated users will be stored using the configured OAuth2AuthorizedClientService via AuthenticatedPrincipalOAuth2AuthorizedClientRepository (which is InMemoryOAuth2AuthorizedClientService by default but should usually be configured by the user in production deployments).

Given that WebSessionServerOAuth2AuthorizedClientRepository is not the default implementation, looking to provide this as an enhancement makes sense, and we can see if others are requesting this as well (which can be done through upvotes or comments). Whether we would want to introduce a completely new implementation so as not to break backwards compatibility, or another strategy, is something we can discuss closer to working on the issue.

Comment From: sfussenegger

@sjohnr I've already tested the suggested implementation in a testing environment and it looks good to me. I cloud therefore provide a PR with the changes. It really just depend on how you'd like to handle backward compatibility. Given that sessionAttributeName is private I personally wouldn't consider changing the stored type from Map<String, OAuth2AuthorizedClient> to something else a breaking change as long as loadAuthorizedClient(..) returns the same value. But that's obviously not my call.

Comment From: sjohnr

@sfussenegger,

Given that sessionAttributeName is private I personally wouldn't consider changing the stored type from Map<String, OAuth2AuthorizedClient> to something else a breaking change as long as loadAuthorizedClient(..) returns the same value.

It's not only an issue of returning the same value as before, one needs to consider upgrades in which a value is stored in the session using the old saveAuthorizedClient(). Therefore you would need to support loading both old and new stored values without issues. This is often tricky because serialization is in play, and session storage itself is outside the scope of this framework.

Comment From: sfussenegger

Good point. That's why I'm not making that call :)

The code is fairly simple, it's really just about the implementation. Plenty of options, from template methods to new interface, extending the existing class or a completely new class, extracting a common abstract class, simple boolean flag property, ...

Regarding the upvotes, I'd say this might actually be one that people might not even be aware of. I probably wouldn't have been if I wasn't working on a reactive Infinispan integration for spring-session (really just a proof of concept so far).