Merge pull request #35755 from sberyozkin/oidc_302_if_state_cookie_mi…

…ssing Request reauthentication by default if OIDC state state query param is missing
quarkusio · Sep 6, 2023 · 15445c9 · 15445c9
2 parents 69b0e01 + 29f169e
commit 15445c9
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 14 deletions.
diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
@@ -889,8 +889,8 @@ public enum ResponseMode {
          * It will cause a new authentication redirect to OpenId Connect provider. Please be aware doing so may increase the
          * risk of browser redirect loops.
          */
-        @ConfigItem(defaultValue = "true")
-        public boolean failOnMissingStateParam = true;
+        @ConfigItem(defaultValue = "false")
+        public boolean failOnMissingStateParam = false;
 
         /**
          * If this property is set to 'true' then an OIDC UserInfo endpoint will be called.

diff --git a/...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java b/...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
@@ -270,8 +270,10 @@ private Uni<SecurityIdentity> stateCookieIsNotMatched(OidcTenantConfig oidcTenan
         if (!oidcTenantConfig.authentication.allowMultipleCodeFlows
                 || context.request().path().equals(getRedirectPath(oidcTenantConfig, context))) {
             if (oidcTenantConfig.authentication.failOnMissingStateParam) {
+                removeStateCookies(oidcTenantConfig, context, cookies);
                 return Uni.createFrom().failure(new AuthenticationCompletionException());
-            } else {
+            }
+            if (!oidcTenantConfig.authentication.allowMultipleCodeFlows) {
                 removeStateCookies(oidcTenantConfig, context, cookies);
             }
         }

diff --git a/integration-tests/oidc-code-flow/src/main/resources/application.properties b/integration-tests/oidc-code-flow/src/main/resources/application.properties
@@ -112,7 +112,7 @@ quarkus.oidc.tenant-https.authentication.pkce-required=true
 quarkus.oidc.tenant-https.authentication.nonce-required=true
 quarkus.oidc.tenant-https.authentication.pkce-secret=eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU
 quarkus.oidc.tenant-https.authentication.cookie-same-site=strict
-quarkus.oidc.tenant-https.authentication.fail-on-missing-state-param=false
+quarkus.oidc.tenant-https.authentication.fail-on-missing-state-param=true
 
 quarkus.oidc.tenant-nonce.auth-server-url=${quarkus.oidc.auth-server-url}
 quarkus.oidc.tenant-nonce.client-id=quarkus-app

diff --git a/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java b/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java
@@ -253,31 +253,38 @@ public void testStateCookieIsPresentButStateParamNot() throws Exception {
 
             // This is a redirect from the OIDC server to the endpoint containing the state and code
             String endpointLocation = webResponse.getResponseHeaderValue("location");
-            assertTrue(endpointLocation.startsWith("https"));
-            endpointLocation = "http" + endpointLocation.substring(5);
+            assertTrue(endpointLocation.startsWith("https://localhost:8081/tenant-https"));
+
+            String code = getCode(URI.create(endpointLocation).getQuery());
+
+            endpointLocation = "http://localhost:8081/tenant-https";
 
             // State cookie is present
             Cookie stateCookie = getStateCookie(webClient, "tenant-https_test");
             assertNull(stateCookie.getSameSite());
             verifyCodeVerifierAndNonce(stateCookie, keycloakUrl);
 
             // Make a call without an extra state query param, status is 401
-            webResponse = webClient.loadWebResponse(new WebRequest(URI.create(endpointLocation + "&state=123").toURL()));
+            webResponse = webClient.loadWebResponse(new WebRequest(URI.create(endpointLocation + "?code=" + code).toURL()));
             assertEquals(401, webResponse.getStatusCode());
 
-            // Make a call without the state query param, confirm the old state cookie is removed, status is 302
-            webResponse = webClient.loadWebResponse(new WebRequest(URI.create("http://localhost:8081/tenant-https").toURL()));
-            assertEquals(302, webResponse.getStatusCode());
             // the old state cookie has been removed
             assertNull(webClient.getCookieManager().getCookie(stateCookie.getName()));
-            // new state cookie is created
-            Cookie newStateCookie = getStateCookie(webClient, "tenant-https_test");
-            assertNotEquals(newStateCookie.getName(), stateCookie.getName());
-
             webClient.getCookieManager().clearCookies();
         }
     }
 
+    private String getCode(String query) {
+        final String[] pairs = query.split("&");
+        for (String pair : pairs) {
+            if (pair.startsWith("code")) {
+                return pair.split("=")[1];
+            }
+        }
+        fail("Authorization code is missing");
+        return null;
+    }
+
     @Test
     public void testCodeFlowForceHttpsRedirectUriWithQueryAndPkce() throws Exception {
         try (final WebClient webClient = createWebClient()) {
@@ -825,6 +832,41 @@ public void testIdTokenInjectionJwtMethod() throws IOException, InterruptedExcep
         }
     }
 
+    @Test
+    public void testIdTokenInjectionJwtMethodMissingStateQueryParam() throws IOException, InterruptedException {
+        try (final WebClient webClient = createWebClient()) {
+            webClient.getOptions().setRedirectEnabled(false);
+            WebResponse webResponse = webClient
+                    .loadWebResponse(
+                            new WebRequest(URI.create("http://localhost:8081/web-app/callback-jwt-before-redirect").toURL()));
+            Cookie stateCookie = getNonUniqueStateCookie(webClient, "tenant-jwt");
+
+            assertEquals(stateCookie.getName(), "q_auth_tenant-jwt");
+            assertNotNull(getStateCookieStateParam(stateCookie));
+            assertNull(getStateCookieSavedPath(stateCookie));
+
+            HtmlPage page = webClient.getPage(webResponse.getResponseHeaderValue("location"));
+            assertEquals("Sign in to quarkus", page.getTitleText());
+            HtmlForm loginForm = page.getForms().get(0);
+            loginForm.getInputByName("username").setValueAttribute("alice");
+            loginForm.getInputByName("password").setValueAttribute("alice");
+
+            webClient.getOptions().setThrowExceptionOnFailingStatusCode(false);
+            webResponse = loginForm.getInputByName("login").click().getWebResponse();
+            webClient.getOptions().setThrowExceptionOnFailingStatusCode(true);
+
+            // This is a redirect from the OIDC server to the endpoint
+            String endpointLocation = webResponse.getResponseHeaderValue("location");
+            assertTrue(endpointLocation.startsWith("http://localhost:8081/web-app/callback-jwt-after-redirect"));
+            endpointLocation = "http://localhost:8081/web-app/callback-jwt-after-redirect";
+            webResponse = webClient.loadWebResponse(new WebRequest(URI.create(endpointLocation).toURL()));
+            assertEquals(302, webResponse.getStatusCode());
+            assertNotNull(getNonUniqueStateCookie(webClient, "tenant-jwt"));
+
+            webClient.getCookieManager().clearCookies();
+        }
+    }
+
     @Test
     public void testIdTokenInjectionJwtMethodButPostMethodUsed() throws IOException, InterruptedException {
         try (final WebClient webClient = createWebClient()) {