Merge claims from userinfo and ID Token correctly (#42277)

Enhance the handling of merging the claims sets of the
ID Token and the UserInfo response. JsonObject#merge would throw a
runtime exception when attempting to merge two objects with the
same key and different values. This could happen for an OP that
returns different vales for the same claim in the ID Token and the
UserInfo response ( Google does that for profile claim ).
If a claim is contained in both sets, we attempt to merge the
values if they are objects or arrays, otherwise the ID Token claim
value takes presedence and overwrites the userinfo response.

Author: jkakavas

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

@@ -37,6 +37,7 @@
 import com.nimbusds.openid.connect.sdk.token.OIDCTokens;
 import com.nimbusds.openid.connect.sdk.validators.AccessTokenValidator;
 import com.nimbusds.openid.connect.sdk.validators.IDTokenValidator;
+import net.minidev.json.JSONArray;
 import net.minidev.json.JSONObject;
 import org.apache.commons.codec.Charsets;
 import org.apache.http.Header;
@@ -401,15 +402,16 @@ private void handleUserinfoResponse(HttpResponse httpResponse, JWTClaimsSet veri
             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);
                     if (LOGGER.isTraceEnabled()) {
                         LOGGER.trace("Successfully retrieved user information: [{}]", userInfoClaims.toJSONObject().toJSONString());
                     }
                     final JSONObject combinedClaims = verifiedIdTokenClaims.toJSONObject();
-                    combinedClaims.merge(userInfoClaims.toJSONObject());
+                    mergeObjects(combinedClaims, userInfoClaims.toJSONObject());
                     claimsListener.onResponse(JWTClaimsSet.parse(combinedClaims));
                 } else if (ContentType.parse(contentHeader.getValue()).getMimeType().equals("application/jwt")) {
                     //TODO Handle validating possibly signed responses
-                    claimsListener.onFailure(new IllegalStateException("Unable to parse Userinfo Response. Signed/encryopted JWTs are" +
+                    claimsListener.onFailure(new IllegalStateException("Unable to parse Userinfo Response. Signed/encrypted JWTs are" +
                         "not currently supported"));
                 } else {
                     claimsListener.onFailure(new IllegalStateException("Unable to parse Userinfo Response. Content type was expected to " +
@@ -435,6 +437,19 @@ private void handleUserinfoResponse(HttpResponse httpResponse, JWTClaimsSet veri
         }
     }
 
+    /**
+     * 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 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)
@@ -606,6 +621,75 @@ private void setMetadataFileWatcher(String jwkSetPath) throws IOException {
         watcherService.add(watcher, ResourceWatcherService.Frequency.MEDIUM);
     }
 
+    /**
+     * Merges the JsonObject with the claims of the ID Token with the JsonObject with the claims of the UserInfo response. This is
+     * necessary as some OPs return slightly different values for some claims (i.e. Google for the profile picture) and
+     * {@link JSONObject#merge(Object)} would throw a runtime exception. The merging is performed based on the following rules:
+     * <ul>
+     * <li>If the values for a given claim are primitives (of the the same type), the value from the ID Token is retained</li>
+     * <li>If the values for a given claim are Objects, the values are merged</li>
+     * <li>If the values for a given claim are Arrays, the values are merged without removing duplicates</li>
+     * <li>If the values for a given claim are of different types, an exception is thrown</li>
+     * </ul>
+     *
+     * @param userInfo The JsonObject with the ID Token claims
+     * @param idToken  The JsonObject with the UserInfo Response claims
+     * @return the merged JsonObject
+     */
+    // pkg protected for testing
+    static JSONObject mergeObjects(JSONObject idToken, JSONObject userInfo) {
+        for (Map.Entry<String, Object> entry : idToken.entrySet()) {
+            Object value1 = entry.getValue();
+            Object value2 = userInfo.get(entry.getKey());
+            if (value2 == null) {
+                continue;
+            }
+            if (value1 instanceof JSONArray) {
+                idToken.put(entry.getKey(), mergeArrays((JSONArray) value1, value2));
+            } else if (value1 instanceof JSONObject) {
+                idToken.put(entry.getKey(), mergeObjects((JSONObject) value1, value2));
+            } else if (value1.getClass().equals(value2.getClass()) == false) {
+                throw new IllegalStateException("Error merging ID token and userinfo claim value for claim [" + entry.getKey() + "]. " +
+                    "Cannot merge [" + value1.getClass().getName() + "] with [" + value2.getClass().getName() + "]");
+            }
+        }
+        for (Map.Entry<String, Object> entry : userInfo.entrySet()) {
+            if (idToken.containsKey(entry.getKey()) == false) {
+                idToken.put(entry.getKey(), entry.getValue());
+            }
+        }
+        return idToken;
+    }
+
+    private static JSONObject mergeObjects(JSONObject jsonObject1, Object jsonObject2) {
+        if (jsonObject2 == null) {
+            return jsonObject1;
+        }
+        if (jsonObject2 instanceof JSONObject) {
+            return mergeObjects(jsonObject1, (JSONObject) jsonObject2);
+        }
+        throw new IllegalStateException("Error while merging ID token and userinfo claims. " +
+            "Cannot merge JSONObject with [" + jsonObject2.getClass().getName() + "]");
+    }
+
+    private static JSONArray mergeArrays(JSONArray jsonArray1, Object jsonArray2) {
+        if (jsonArray2 == null) {
+            return jsonArray1;
+        }
+        if (jsonArray2 instanceof JSONArray) {
+            return mergeArrays(jsonArray1, (JSONArray) jsonArray2);
+        }
+        if (jsonArray2 instanceof String) {
+            jsonArray1.add(jsonArray2);
+        }
+        return jsonArray1;
+    }
+
+    private static JSONArray mergeArrays(JSONArray jsonArray1, JSONArray jsonArray2) {
+        jsonArray1.addAll(jsonArray2);
+        return jsonArray1;
+    }
+
     protected void close() {
         try {
             this.httpClient.close();

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

@@ -37,6 +37,8 @@
 import com.nimbusds.openid.connect.sdk.claims.AccessTokenHash;
 import com.nimbusds.openid.connect.sdk.validators.IDTokenValidator;
 import com.nimbusds.openid.connect.sdk.validators.InvalidHashException;
+import net.minidev.json.JSONArray;
+import net.minidev.json.JSONObject;
 import org.elasticsearch.ElasticsearchSecurityException;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.support.PlainActionFuture;
@@ -72,6 +74,7 @@
 import java.util.UUID;
 
 import static java.time.Instant.now;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
@@ -96,7 +99,9 @@ public void setup() {
 
     @After
     public void cleanup() {
-        authenticator.close();
+        if (authenticator != null) {
+            authenticator.close();
+        }
     }
 
     private OpenIdConnectAuthenticator buildAuthenticator() throws URISyntaxException {
@@ -632,6 +637,140 @@ public void testImplicitFlowFailsWithUnsignedJwt() throws Exception {
         assertThat(e.getCause().getMessage(), containsString("Signed ID token expected"));
     }
 
+    public void testJsonObjectMerging() throws Exception {
+        final Nonce nonce = new Nonce();
+        final String subject = "janedoe";
+        final Tuple<Key, JWKSet> keyMaterial = getRandomJwkForType(randomFrom("ES", "RS"));
+        final JWK jwk = keyMaterial.v2().getKeys().get(0);
+        RelyingPartyConfiguration rpConfig = getRpConfig(jwk.getAlgorithm().getName());
+        OpenIdConnectProviderConfiguration opConfig = getOpConfig();
+        JSONObject address = new JWTClaimsSet.Builder()
+            .claim("street_name", "12, Test St.")
+            .claim("locality", "New York")
+            .claim("region", "NY")
+            .claim("country", "USA")
+            .build()
+            .toJSONObject();
+        JSONObject idTokenObject = new JWTClaimsSet.Builder()
+            .jwtID(randomAlphaOfLength(8))
+            .audience(rpConfig.getClientId().getValue())
+            .expirationTime(Date.from(now().plusSeconds(3600)))
+            .issuer(opConfig.getIssuer().getValue())
+            .issueTime(Date.from(now().minusSeconds(200)))
+            .notBeforeTime(Date.from(now().minusSeconds(200)))
+            .claim("nonce", nonce)
+            .claim("given_name", "Jane Doe")
+            .claim("family_name", "Doe")
+            .claim("profile", "https://test-profiles.com/jane.doe")
+            .claim("name", "Jane")
+            .claim("email", "[email protected]")
+            .claim("roles", new JSONArray().appendElement("role1").appendElement("role2").appendElement("role3"))
+            .claim("address", address)
+            .subject(subject)
+            .build()
+            .toJSONObject();
+
+        JSONObject userinfoObject = new JWTClaimsSet.Builder()
+            .claim("given_name", "Jane Doe")
+            .claim("family_name", "Doe")
+            .claim("profile", "https://test-profiles.com/jane.doe")
+            .claim("name", "Jane")
+            .claim("email", "[email protected]")
+            .subject(subject)
+            .build()
+            .toJSONObject();
+
+        OpenIdConnectAuthenticator.mergeObjects(idTokenObject, userinfoObject);
+        assertTrue(idTokenObject.containsKey("given_name"));
+        assertTrue(idTokenObject.containsKey("family_name"));
+        assertTrue(idTokenObject.containsKey("profile"));
+        assertTrue(idTokenObject.containsKey("name"));
+        assertTrue(idTokenObject.containsKey("email"));
+        assertTrue(idTokenObject.containsKey("address"));
+        assertTrue(idTokenObject.containsKey("roles"));
+        assertTrue(idTokenObject.containsKey("nonce"));
+        assertTrue(idTokenObject.containsKey("sub"));
+        assertTrue(idTokenObject.containsKey("jti"));
+        assertTrue(idTokenObject.containsKey("aud"));
+        assertTrue(idTokenObject.containsKey("exp"));
+        assertTrue(idTokenObject.containsKey("iss"));
+        assertTrue(idTokenObject.containsKey("iat"));
+        assertTrue(idTokenObject.containsKey("email"));
+
+        // Claims with different types throw an error
+        JSONObject wrongTypeInfo = new JWTClaimsSet.Builder()
+            .claim("given_name", "Jane Doe")
+            .claim("family_name", 123334434)
+            .claim("profile", "https://test-profiles.com/jane.doe")
+            .claim("name", "Jane")
+            .claim("email", "[email protected]")
+            .subject(subject)
+            .build()
+            .toJSONObject();
+
+        final IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            OpenIdConnectAuthenticator.mergeObjects(idTokenObject, wrongTypeInfo);
+        });
+
+        // Userinfo Claims overwrite ID Token claims
+        JSONObject overwriteUserInfo = new JWTClaimsSet.Builder()
+            .claim("given_name", "Jane Doe")
+            .claim("family_name", "Doe")
+            .claim("profile", "https://test-profiles.com/jane.doe2")
+            .claim("name", "Jane")
+            .claim("email", "[email protected]")
+            .subject(subject)
+            .build()
+            .toJSONObject();
+
+        OpenIdConnectAuthenticator.mergeObjects(idTokenObject, overwriteUserInfo);
+        assertThat(idTokenObject.getAsString("email"), equalTo("[email protected]"));
+        assertThat(idTokenObject.getAsString("profile"), equalTo("https://test-profiles.com/jane.doe"));
+
+        // Merging Arrays
+        JSONObject userInfoWithRoles = new JWTClaimsSet.Builder()
+            .claim("given_name", "Jane Doe")
+            .claim("family_name", "Doe")
+            .claim("profile", "https://test-profiles.com/jane.doe")
+            .claim("name", "Jane")
+            .claim("email", "[email protected]")
+            .claim("roles", new JSONArray().appendElement("role4").appendElement("role5"))
+            .subject(subject)
+            .build()
+            .toJSONObject();
+
+        OpenIdConnectAuthenticator.mergeObjects(idTokenObject, userInfoWithRoles);
+        assertThat((JSONArray) idTokenObject.get("roles"), containsInAnyOrder("role1", "role2", "role3", "role4", "role5"));
+
+        // Merging nested objects
+        JSONObject addressUserInfo = new JWTClaimsSet.Builder()
+            .claim("street_name", "12, Test St.")
+            .claim("locality", "New York")
+            .claim("postal_code", "10024")
+            .build()
+            .toJSONObject();
+        JSONObject userInfoWithAddress = new JWTClaimsSet.Builder()
+            .claim("given_name", "Jane Doe")
+            .claim("family_name", "Doe")
+            .claim("profile", "https://test-profiles.com/jane.doe")
+            .claim("name", "Jane")
+            .claim("email", "[email protected]")
+            .claim("roles", new JSONArray().appendElement("role4").appendElement("role5"))
+            .claim("address", addressUserInfo)
+            .subject(subject)
+            .build()
+            .toJSONObject();
+        OpenIdConnectAuthenticator.mergeObjects(idTokenObject, userInfoWithAddress);
+        assertTrue(idTokenObject.containsKey("address"));
+        JSONObject combinedAddress = (JSONObject) idTokenObject.get("address");
+        assertTrue(combinedAddress.containsKey("street_name"));
+        assertTrue(combinedAddress.containsKey("locality"));
+        assertTrue(combinedAddress.containsKey("street_name"));
+        assertTrue(combinedAddress.containsKey("postal_code"));
+        assertTrue(combinedAddress.containsKey("region"));
+        assertTrue(combinedAddress.containsKey("country"));
+    }
+
     private OpenIdConnectProviderConfiguration getOpConfig() throws URISyntaxException {
         return new OpenIdConnectProviderConfiguration(
             new Issuer("https://op.example.com"),