Merge claims from userinfo and ID Token correctly

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"),