fix: Move SPRING_REDIRECT_URI cookie invalidation to correct place

Invalidating SPRING_REDIRECT_URI cookie right after the cookie is read.

JIRA: LX-1022
risk: high

Author: jeskepetr

Diff for: gooddata-server-oauth2-autoconfigure/src/main/kotlin/CookieServerRequestCache.kt

@@ -18,14 +18,12 @@
  */
 package com.gooddata.oauth2.server
 
+import java.net.URI
 import org.springframework.http.HttpMethod
-import org.springframework.http.HttpStatus
-import org.springframework.http.server.reactive.ServerHttpRequest
 import org.springframework.security.web.server.savedrequest.ServerRequestCache
 import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatchers
 import org.springframework.web.server.ServerWebExchange
 import reactor.core.publisher.Mono
-import java.net.URI
 
 /**
  * An implementation of [ServerRequestCache] that saves the requested URI in a cookie.
@@ -51,22 +49,7 @@ class CookieServerRequestCache(private val cookieService: ReactiveCookieService)
         Mono.just(exchange)
             .flatMap { cookieService.decodeCookie(it, SPRING_REDIRECT_URI) }
             .map { URI.create(it) }
+            .doOnNext { cookieService.invalidateCookie(exchange, SPRING_REDIRECT_URI) }
 
-    override fun removeMatchingRequest(exchange: ServerWebExchange): Mono<ServerHttpRequest> {
-        return Mono.just(exchange)
-            .flatMap { webExchange ->
-                if (webExchange.response.statusCode == HttpStatus.UNAUTHORIZED) {
-                    // For 401 responses, always preserve the redirect URI
-                    // If there's an existing one, keep it
-                    // If not, the saveRequest() will set it later
-                    Mono.just(webExchange.request)
-                } else {
-                    // For non-401 responses (like successful authentication),
-                    // we can safely invalidate the cookie as we don't need it anymore
-                    Mono.just(webExchange)
-                        .doOnNext { cookieService.invalidateCookie(it, SPRING_REDIRECT_URI) }
-                        .thenReturn(webExchange.request)
-                }
-            }
-    }
+    override fun removeMatchingRequest(exchange: ServerWebExchange) = Mono.just(exchange.request)
 }

Diff for: gooddata-server-oauth2-autoconfigure/src/test/kotlin/CookieServerRequestCacheTest.kt

@@ -23,10 +23,12 @@ import io.mockk.slot
 import io.mockk.spyk
 import io.mockk.verify
 import io.netty.handler.codec.http.cookie.CookieHeaderNames
+import java.net.URI
+import java.time.Duration
+import java.time.Instant
 import org.intellij.lang.annotations.Language
 import org.junit.jupiter.api.Test
 import org.springframework.http.HttpCookie
-import org.springframework.http.HttpStatus
 import org.springframework.mock.http.server.reactive.MockServerHttpRequest
 import org.springframework.mock.web.server.MockServerWebExchange
 import org.springframework.util.CollectionUtils
@@ -35,10 +37,6 @@ import reactor.core.publisher.Mono
 import strikt.api.expectThat
 import strikt.assertions.isEqualTo
 import strikt.assertions.isTrue
-import java.net.URI
-import java.time.Duration
-import java.time.Instant
-import strikt.assertions.isNotNull
 
 internal class CookieServerRequestCacheTest {
 
@@ -68,7 +66,9 @@ internal class CookieServerRequestCacheTest {
     """
 
     private val client: AuthenticationStoreClient = mockk {
-        mockCookieSecurityProperties(this, ORG_ID,
+        mockCookieSecurityProperties(
+            this,
+            ORG_ID,
             CookieSecurityProperties(
                 keySet = CleartextKeysetHandle.read(JsonKeysetReader.withBytes(keyset.toByteArray())),
                 lastRotation = Instant.now(),
@@ -169,11 +169,10 @@ internal class CookieServerRequestCacheTest {
     }
 
     @Test
-    fun `should preserve redirect URI during 401 response`() {
+    fun `should invalidate cookie after reading redirect URI`() {
         val webExchange = mockk<ServerWebExchange> {
             every { request.uri.host } returns "localhost"
             every { attributes[OrganizationWebFilter.ORGANIZATION_CACHE_KEY] } returns Organization(ORG_ID)
-            every { response.statusCode } returns HttpStatus.UNAUTHORIZED
         }
 
         val redirect = "/requestURI?query=true"
@@ -182,36 +181,12 @@ internal class CookieServerRequestCacheTest {
                 HttpCookie(SPRING_REDIRECT_URI, cookieSerializer.encodeCookieBlocking(webExchange, redirect))
             )
         )
-        exchange.response.statusCode = HttpStatus.UNAUTHORIZED
 
-        val request = cache.removeMatchingRequest(exchange).block()
-
-        // Verify the cookie was not invalidated
-        verify(exactly = 0) { cookieService.invalidateCookie(exchange, SPRING_REDIRECT_URI) }
-        expectThat(request).isNotNull()
-    }
-
-    @Test
-    fun `should clear redirect URI for non-401 response`() {
-        val webExchange = mockk<ServerWebExchange> {
-            every { request.uri.host } returns "localhost"
-            every { attributes[OrganizationWebFilter.ORGANIZATION_CACHE_KEY] } returns Organization(ORG_ID)
-            every { response.statusCode } returns HttpStatus.FOUND
-        }
-
-        val redirect = "/requestURI?query=true"
-        val exchange = MockServerWebExchange.from(
-            MockServerHttpRequest.get("http://localhost/").cookie(
-                HttpCookie(SPRING_REDIRECT_URI, cookieSerializer.encodeCookieBlocking(webExchange, redirect))
-            )
-        )
-        exchange.response.statusCode = HttpStatus.FOUND
+        val uri = cache.getRedirectUri(exchange).block()
 
-        val request = cache.removeMatchingRequest(exchange).block()
+        expectThat(uri).isEqualTo(URI.create(redirect))
 
-        // Verify the cookie was invalidated
         verify(exactly = 1) { cookieService.invalidateCookie(exchange, SPRING_REDIRECT_URI) }
-        expectThat(request).isNotNull()
     }
 
     companion object {