Skip to content

Commit

Permalink
Merge claims from userinfo and ID Token correctly (elastic#42277)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jkakavas
jkakavas authored and Gurkan Kaymak committed May 27, 2019
1 parent c88814d commit b242390
Show file tree
Hide file tree
Showing 2 changed files with 226 additions and 3 deletions.
88 changes: 86 additions & 2 deletions ...src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 " +
Expand All @@ -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)
Expand Down Expand Up @@ -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();
Expand Down
141 changes: 140 additions & 1 deletion ...est/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -96,7 +99,9 @@ public void setup() {

@After
public void cleanup() {
authenticator.close();
if (authenticator != null) {
authenticator.close();
}
}

private OpenIdConnectAuthenticator buildAuthenticator() throws URISyntaxException {
Expand Down Expand Up @@ -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"),
Expand Down

0 comments on commit b242390

Please sign in to comment.