OAuth2AuthorizationRequest not removed from session

[AndreasKl]

When #6215 was fixed only the adding of new OAuth2AuthorizationRequests was fixed, not the removal of those. With a distributed session store we observed an increase in session size for users having long running sessions.

A dump of the keys of the session attributes revealed a huge HashMap of OAuth2AuthorizationRequest. This is due to org.springframework.security.oauth2.client.web.server.WebSessionOAuth2ServerAuthorizationRequestRepository#removeAuthorizationRequest only removing the OAuth2AuthorizationRequest from the HashMap and not updating the session attributes leaving no clue to the session repository that the session was amended.

The expected behaviour would be that the stateToAuthzRequest HashMap should not grow without limit and OAuth2AuthorizationRequest should be removed after it was used to create a new session.

Used version: spring-security-oauth2-client-5.1.6.RELEASE.jar
however the issue exists on master: https://github.com/spring-projects/spring-security/blob/master/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/WebSessionOAuth2ServerAuthorizationRequestRepository.java#L85

[jgrandja]

Thanks for the report @AndreasKl. You are right, WebSessionOAuth2ServerAuthorizationRequestRepository.removeAuthorizationRequest() does not update the sessions attributes with the updated Map of the removal of OAuth2AuthorizationRequest.

Would you be interested in submitting a PR for this fix?

[AndreasKl]

@jgrandja Would be happy to provide the PR.

Do you think it is of any value, if I also provide a PR with an optional facility that allows to limit the amount of OAuth2AuthorizationRequests for a single session. This could be an additional session attribute backed by a size limited LinkedHashMap containing the state values as keys and an empty object as value (this would have the benefit that existing sessions can be de-serialized).

Something like:

  void limitSizeOfAuthorizationRequest(
      OAuth2AuthorizationRequest oAuth2AuthorizationRequest, WebSession webSession) {
    requireNonNull(oAuth2AuthorizationRequest, "oAuth2AuthorizationRequest must not be null");
    requireNonNull(webSession, "webSession must not be null");

    removeOldAuthorizationRequest(
        storeMostRecentAuthorizationRequests(oAuth2AuthorizationRequest, webSession), webSession);
  }

  private Map<String, String> storeMostRecentAuthorizationRequests(
      OAuth2AuthorizationRequest oAuth2AuthorizationRequest, WebSession webSession) {

    SizeLimitedHashMap<String, String> workingSet =
        webSession.getAttribute(DEFAULT_AUTHORIZATION_REQUEST_LIMIT_ATTR_NAME);
    if (workingSet == null) {
      workingSet = new SizeLimitedHashMap<>(limit);
    }

    workingSet.put(oAuth2AuthorizationRequest.getState(), SOME_VALUE);

    if (LOG.isDebugEnabled()) {
      LOG.debug("The current active auth request states are: {}", workingSet.keySet());
    }

    webSession.getAttributes().put(DEFAULT_AUTHORIZATION_REQUEST_LIMIT_ATTR_NAME, workingSet);
    return workingSet;
  }

  private void removeOldAuthorizationRequest(
      Map<String, String> workingSet, WebSession webSession) {
    Map<String, OAuth2AuthorizationRequest> oAuth2AuthorizationRequests =
        webSession.getAttribute(DEFAULT_AUTHORIZATION_REQUEST_ATTR_NAME);

    if (oAuth2AuthorizationRequests == null) {
      return;
    }

    if (LOG.isDebugEnabled()) {
      LOG.debug(
          "Auth request state keys before cleanup: {}", oAuth2AuthorizationRequests.keySet());
    }

    oAuth2AuthorizationRequests.keySet().retainAll(workingSet.keySet());

    if (LOG.isDebugEnabled()) {
      LOG.debug("Auth request state keys after cleanup: {}", oAuth2AuthorizationRequests.keySet());
    }

    webSession
        .getAttributes()
        .put(DEFAULT_AUTHORIZATION_REQUEST_ATTR_NAME, oAuth2AuthorizationRequests);
  }

public class SizeLimitedHashMap<TKey extends Serializable, TValue extends Serializable>
    extends LinkedHashMap<TKey, TValue> implements Serializable {

  private static final long serialVersionUID = -4358903105100110082L;

  private final int limit;

  SizeLimitedHashMap(int limit) {
    this.limit = limit;
  }

  @Override
  protected boolean removeEldestEntry(Entry<TKey, TValue> eldest) {
    return this.size() > limit;
  }
}

[jgrandja]

@AndreasKl

Do you think it is of any value, if I also provide a PR with an optional facility that allows to limit the amount of OAuth2AuthorizationRequests for a single session

The existing issue #5145 tracks this enhancement. We would need this enhancement implemented on both the Servlet and Reactive side. It would be great if you can provide a PR?

[AndreasKl]

@jgrandja Thanks for the hint, wasn't aware there is already an open issue.