Short circuit failure handling in OIDC flow (elastic#130618) (elastic#132403)

We should return after the `onFailure` call during the claims check.

Author: n1v0lg

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

@@ -477,7 +477,20 @@ static void handleUserinfoResponse(
             if (httpResponse.getStatusLine().getStatusCode() == 200) {
                 if (ContentType.parse(contentHeader.getValue()).getMimeType().equals("application/json")) {
                     final JWTClaimsSet userInfoClaims = JWTClaimsSet.parse(contentAsString);
-                    validateUserInfoResponse(userInfoClaims, verifiedIdTokenClaims.getSubject(), claimsListener);
+                    String expectedSub = verifiedIdTokenClaims.getSubject();
+                    if (userInfoClaims.getSubject() == null || userInfoClaims.getSubject().isEmpty()) {
+                        claimsListener.onFailure(new ElasticsearchSecurityException("Userinfo Response did not contain a sub Claim"));
+                        return;
+                    } else if (userInfoClaims.getSubject().equals(expectedSub) == false) {
+                        claimsListener.onFailure(
+                            new ElasticsearchSecurityException(
+                                "Userinfo Response is not valid as it is for " + "subject [{}] while the ID Token was for subject [{}]",
+                                userInfoClaims.getSubject(),
+                                expectedSub
+                            )
+                        );
+                        return;
+                    }
                     if (LOGGER.isTraceEnabled()) {
                         LOGGER.trace("Successfully retrieved user information: [{}]", userInfoClaims);
                     }
@@ -527,27 +540,6 @@ static void handleUserinfoResponse(
         }
     }
 
-    /**
-     * Validates that the userinfo response contains a sub Claim and that this claim value is the same as the one returned in the ID Token
-     */
-    private static void validateUserInfoResponse(
-        JWTClaimsSet userInfoClaims,
-        String expectedSub,
-        ActionListener<JWTClaimsSet> claimsListener
-    ) {
-        if (userInfoClaims.getSubject().isEmpty()) {
-            claimsListener.onFailure(new ElasticsearchSecurityException("Userinfo Response did not contain a sub Claim"));
-        } else if (userInfoClaims.getSubject().equals(expectedSub) == false) {
-            claimsListener.onFailure(
-                new ElasticsearchSecurityException(
-                    "Userinfo Response is not valid as it is for " + "subject [{}] while the ID Token was for subject [{}]",
-                    userInfoClaims.getSubject(),
-                    expectedSub
-                )
-            );
-        }
-    }
-
     /**
      * Attempts to make a request to the Token Endpoint of the OpenID Connect provider in order to exchange an
      * authorization code for an Id Token (and potentially an Access Token)

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java

@@ -108,6 +108,7 @@
 import java.util.Map;
 import java.util.UUID;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 
 import javax.crypto.SecretKey;
@@ -968,6 +969,53 @@ public void testHandleUserinfoResponseFailure() throws Exception {
         );
     }
 
+    public void testHandleUserinfoValidationFailsOnNotMatchingSubject() throws Exception {
+        final ProtocolVersion httpVersion = randomFrom(HttpVersion.HTTP_0_9, HttpVersion.HTTP_1_0, HttpVersion.HTTP_1_1);
+        final HttpResponse response = new BasicHttpResponse(new BasicStatusLine(httpVersion, RestStatus.OK.getStatus(), "OK"));
+
+        final String sub = randomAlphaOfLengthBetween(4, 36);
+        final String inf = randomAlphaOfLength(12);
+        final JWTClaimsSet infoClaims = new JWTClaimsSet.Builder().subject("it-is-a-different-subject").claim("inf", inf).build();
+        final StringEntity entity = new StringEntity(infoClaims.toString(), ContentType.APPLICATION_JSON);
+        if (randomBoolean()) {
+            entity.setContentEncoding(
+                randomFrom(StandardCharsets.UTF_8.name(), StandardCharsets.UTF_16.name(), StandardCharsets.US_ASCII.name())
+            );
+        }
+        response.setEntity(entity);
+
+        final String idx = randomAlphaOfLength(8);
+        final JWTClaimsSet idClaims = new JWTClaimsSet.Builder().subject(sub).claim("idx", idx).build();
+        final AtomicBoolean listenerCalled = new AtomicBoolean(false);
+        final PlainActionFuture<JWTClaimsSet> future = new PlainActionFuture<>() {
+
+            @Override
+            public void onResponse(JWTClaimsSet result) {
+                assertTrue("listener called more than once", listenerCalled.compareAndSet(false, true));
+                super.onResponse(result);
+            }
+
+            @Override
+            public void onFailure(Exception e) {
+                assertTrue("listener called more than once", listenerCalled.compareAndSet(false, true));
+                super.onFailure(e);
+            }
+        };
+
+        this.authenticator = buildAuthenticator();
+        OpenIdConnectAuthenticator.handleUserinfoResponse(response, idClaims, future);
+        var e = expectThrows(ElasticsearchSecurityException.class, future::actionGet);
+
+        assertThat(
+            e.getMessage(),
+            equalTo(
+                "Userinfo Response is not valid as it is for subject [it-is-a-different-subject] while the ID Token was for subject ["
+                    + sub
+                    + "]"
+            )
+        );
+    }
+
     public void testHandleTokenResponseNullContentType() {
         final HttpResponse response = new BasicHttpResponse(HttpVersion.HTTP_1_1, RestStatus.OK.getStatus(), "");
         final StringEntity entity = new StringEntity("", (ContentType) null);