Expire OAuth2AuthorizationRequest when saving to the session

oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepository.java

@@ -16,23 +16,33 @@
 
 package org.springframework.security.oauth2.client.web;
 
+import java.io.Serializable;
+import java.time.Clock;
+import java.time.Duration;
+import java.time.Instant;
 import java.util.HashMap;
 import java.util.Map;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
 
+import org.springframework.security.core.SpringSecurityCoreVersion;
 import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
 import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames;
 import org.springframework.util.Assert;
 
 /**
  * An implementation of an {@link AuthorizationRequestRepository} that stores
  * {@link OAuth2AuthorizationRequest} in the {@code HttpSession}.
+ * <p>
+ * <b>NOTE:</b> {@link OAuth2AuthorizationRequest}s expire after two minutes, the default
+ * duration can be configured via
+ * {@link #setOAuth2AuthorizationRequestExpiresIn(Duration)}.
  *
  * @author Joe Grandja
  * @author Rob Winch
+ * @author Craig Andrews
  * @since 5.0
  * @see AuthorizationRequestRepository
  * @see OAuth2AuthorizationRequest
@@ -45,15 +55,21 @@ public final class HttpSessionOAuth2AuthorizationRequestRepository
 
 	private final String sessionAttributeName = DEFAULT_AUTHORIZATION_REQUEST_ATTR_NAME;
 
+	private Clock clock = Clock.systemUTC();
+
+	private Duration oAuth2AuthorizationRequestExpiresIn = Duration.ofSeconds(120);
+
 	@Override
 	public OAuth2AuthorizationRequest loadAuthorizationRequest(HttpServletRequest request) {
 		Assert.notNull(request, "request cannot be null");
 		String stateParameter = this.getStateParameter(request);
 		if (stateParameter == null) {
 			return null;
 		}
-		Map<String, OAuth2AuthorizationRequest> authorizationRequests = this.getAuthorizationRequests(request);
-		return authorizationRequests.get(stateParameter);
+		Map<String, WrappedWithCreated<OAuth2AuthorizationRequest>> authorizationRequests = this
+				.getAuthorizationRequests(request);
+		WrappedWithCreated<OAuth2AuthorizationRequest> wrappedWithCreated = authorizationRequests.get(stateParameter);
+		return (wrappedWithCreated != null) ? wrappedWithCreated.wrapped : null;
 	}
 
 	@Override
@@ -67,8 +83,9 @@ public void saveAuthorizationRequest(OAuth2AuthorizationRequest authorizationReq
 		}
 		String state = authorizationRequest.getState();
 		Assert.hasText(state, "authorizationRequest.state cannot be empty");
-		Map<String, OAuth2AuthorizationRequest> authorizationRequests = this.getAuthorizationRequests(request);
-		authorizationRequests.put(state, authorizationRequest);
+		Map<String, WrappedWithCreated<OAuth2AuthorizationRequest>> authorizationRequests = this
+				.getAuthorizationRequests(request);
+		authorizationRequests.put(state, new WrappedWithCreated<>(this.clock.instant(), authorizationRequest));
 		request.getSession().setAttribute(this.sessionAttributeName, authorizationRequests);
 	}
 
@@ -79,15 +96,17 @@ public OAuth2AuthorizationRequest removeAuthorizationRequest(HttpServletRequest
 		if (stateParameter == null) {
 			return null;
 		}
-		Map<String, OAuth2AuthorizationRequest> authorizationRequests = this.getAuthorizationRequests(request);
-		OAuth2AuthorizationRequest originalRequest = authorizationRequests.remove(stateParameter);
+		Map<String, WrappedWithCreated<OAuth2AuthorizationRequest>> authorizationRequests = this
+				.getAuthorizationRequests(request);
+		WrappedWithCreated<OAuth2AuthorizationRequest> wrappedWithCreatedOriginalRequest = authorizationRequests
+				.remove(stateParameter);
 		if (!authorizationRequests.isEmpty()) {
 			request.getSession().setAttribute(this.sessionAttributeName, authorizationRequests);
 		}
 		else {
 			request.getSession().removeAttribute(this.sessionAttributeName);
 		}
-		return originalRequest;
+		return (wrappedWithCreatedOriginalRequest != null) ? wrappedWithCreatedOriginalRequest.wrapped : null;
 	}
 
 	@Override
@@ -113,14 +132,62 @@ private String getStateParameter(HttpServletRequest request) {
 	 * @return a non-null and mutable map of {@link OAuth2AuthorizationRequest#getState()}
 	 * to an {@link OAuth2AuthorizationRequest}.
 	 */
-	private Map<String, OAuth2AuthorizationRequest> getAuthorizationRequests(HttpServletRequest request) {
+	private Map<String, WrappedWithCreated<OAuth2AuthorizationRequest>> getAuthorizationRequests(
+			HttpServletRequest request) {
 		HttpSession session = request.getSession(false);
-		Map<String, OAuth2AuthorizationRequest> authorizationRequests = (session != null)
-				? (Map<String, OAuth2AuthorizationRequest>) session.getAttribute(this.sessionAttributeName) : null;
+		Map<String, WrappedWithCreated<OAuth2AuthorizationRequest>> authorizationRequests = (session != null)
+				? (Map<String, WrappedWithCreated<OAuth2AuthorizationRequest>>) session
+						.getAttribute(this.sessionAttributeName)
+				: null;
 		if (authorizationRequests == null) {
 			return new HashMap<>();
 		}
+		// remove expired entries
+		authorizationRequests.entrySet().removeIf((entry) -> entry.getValue().created
+				.isBefore(this.clock.instant().minus(this.oAuth2AuthorizationRequestExpiresIn)));
 		return authorizationRequests;
 	}
 
+	/**
+	 * Sets the {@link Clock} used in {@link Instant#now(Clock)} when setting the instant
+	 * created for {@link OAuth2AuthorizationRequest}.
+	 * @param clock the clock
+	 * @since 5.5
+	 */
+	public void setClock(Clock clock) {
+		Assert.notNull(clock, "clock cannot be null");
+		this.clock = clock;
+	}
+
+	/**
+	 * Sets the {@link Duration} for which {@link OAuth2AuthorizationRequest} should
+	 * expire.
+	 * @param oAuth2AuthorizationRequestExpiresIn the {@link Duration} a
+	 * {@link OAuth2AuthorizationRequest} is considered not expired. Must not be negative.
+	 * @since 5.5
+	 */
+	public void setOAuth2AuthorizationRequestExpiresIn(Duration oAuth2AuthorizationRequestExpiresIn) {
+		Assert.notNull(oAuth2AuthorizationRequestExpiresIn, "oAuth2AuthorizationRequestExpiresIn cannot be null");
+		Assert.state(!oAuth2AuthorizationRequestExpiresIn.isNegative(),
+				"oAuth2AuthorizationRequestExpiresIn cannot be negative");
+		this.oAuth2AuthorizationRequestExpiresIn = oAuth2AuthorizationRequestExpiresIn;
+	}
+
+	private static final class WrappedWithCreated<T extends Serializable> implements Serializable {
+
+		private static final long serialVersionUID = SpringSecurityCoreVersion.SERIAL_VERSION_UID;
+
+		private final Instant created;
+
+		private final T wrapped;
+
+		private WrappedWithCreated(Instant created, T wrapped) {
+			super();
+			this.created = created;
+			Assert.notNull(wrapped, "wrapped cannot be null");
+			this.wrapped = wrapped;
+		}
+
+	}
+
 }

oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepositoryTests.java

@@ -16,6 +16,10 @@
 
 package org.springframework.security.oauth2.client.web;
 
+import java.time.Clock;
+import java.time.Duration;
+import java.time.Instant;
+import java.time.ZoneId;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -36,6 +40,7 @@
  * Tests for {@link HttpSessionOAuth2AuthorizationRequestRepository}.
  *
  * @author Joe Grandja
+ * @author Craig Andrews
  */
 @RunWith(MockitoJUnitRunner.class)
 public class HttpSessionOAuth2AuthorizationRequestRepositoryTests {
@@ -237,6 +242,32 @@ public void removeAuthorizationRequestWhenNotSavedThenNotRemoved() {
 		assertThat(removedAuthorizationRequest).isNull();
 	}
 
+	@Test
+	public void testExpiredAuthorizationRequestsRemoved() {
+		final Duration expiresIn = Duration.ofMinutes(2);
+		this.authorizationRequestRepository.setOAuth2AuthorizationRequestExpiresIn(expiresIn);
+		this.authorizationRequestRepository.setClock(Clock.fixed(Instant.ofEpochMilli(0), ZoneId.systemDefault()));
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		MockHttpServletResponse response = new MockHttpServletResponse();
+		String state1 = "state-1122";
+		OAuth2AuthorizationRequest authorizationRequest1 = createAuthorizationRequest().state(state1).build();
+		this.authorizationRequestRepository.saveAuthorizationRequest(authorizationRequest1, request, response);
+		String state2 = "state-3344";
+		this.authorizationRequestRepository
+				.setClock(Clock.fixed(Instant.ofEpochMilli(1).plus(expiresIn), ZoneId.systemDefault()));
+		OAuth2AuthorizationRequest authorizationRequest2 = createAuthorizationRequest().state(state2).build();
+		this.authorizationRequestRepository.saveAuthorizationRequest(authorizationRequest2, request, response);
+		request.addParameter(OAuth2ParameterNames.STATE, state1);
+		OAuth2AuthorizationRequest loadedAuthorizationRequest1 = this.authorizationRequestRepository
+				.loadAuthorizationRequest(request);
+		assertThat(loadedAuthorizationRequest1).isNull();
+		request.removeParameter(OAuth2ParameterNames.STATE);
+		request.addParameter(OAuth2ParameterNames.STATE, state2);
+		OAuth2AuthorizationRequest loadedAuthorizationRequest2 = this.authorizationRequestRepository
+				.loadAuthorizationRequest(request);
+		assertThat(loadedAuthorizationRequest2).isEqualTo(authorizationRequest2);
+	}
+
 	private OAuth2AuthorizationRequest.Builder createAuthorizationRequest() {
 		return OAuth2AuthorizationRequest.authorizationCode().authorizationUri("https://example.com/oauth2/authorize")
 				.clientId("client-id-1234").state("state-1234");