Short circuit failure handling in OIDC flow

[n1v0lg]

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

[elasticsearchmachine]

Pinging @elastic/es-security (Team:Security)

[slobodanadamovic]

Let's cover these error cases with unit tests in OpenIdConnectAuthenticatorTests. Also, I think we could potentially get a NullPointerException here when userInfoClaims.getSubject() is not returned at all (null).

Below is a simple test I could think of to test not matching subs. Not sure if there is a test version of action listener implementation which asserts that it was called only once:

    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
                    + "]"
            )
        );

    }

[slobodanadamovic]

Suggested change

	if (userInfoClaims.getSubject().isEmpty()) {
	if (userInfoClaims.getSubject() == null || userInfoClaims.getSubject().isEmpty()) {

[n1v0lg]

thanks catching, I rushed on this 🤦

[slobodanadamovic]

Not related to your changes, but I think we should fix it as well.