Merge pull request #1121 from velias/KEYCLOAK-1182

KEYCLOAK-1182 - string values for user profile are now sanitized during reading from social providers

broker/oidc/pom.xml

@@ -52,6 +52,11 @@
             <artifactId>jboss-logging</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
 </project>

broker/oidc/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java

@@ -108,11 +108,22 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
     }
 
     protected String extractTokenFromResponse(String response, String tokenName) {
+    	  if(response == null)
+    	  	return null;
+    	  
         if (response.startsWith("{")) {
             try {
-                return mapper.readTree(response).get(tokenName).getTextValue();
+            		JsonNode node = mapper.readTree(response);
+            		if(node.has(tokenName)){
+            			String s = node.get(tokenName).getTextValue();
+            			if(s == null || s.trim().isEmpty())
+            				return null;
+                  return s;
+            		} else {
+            			return null;
+            		}
             } catch (IOException e) {
-                throw new IdentityBrokerException("Could not extract token [" + tokenName + "] from response [" + response + "].", e);
+                throw new IdentityBrokerException("Could not extract token [" + tokenName + "] from response [" + response + "] due: " + e.getMessage(), e);
             }
         } else {
             Matcher matcher = Pattern.compile(tokenName + "=([^&]+)").matcher(response);
@@ -129,7 +140,7 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
         String accessToken = extractTokenFromResponse(response, OAUTH2_PARAMETER_ACCESS_TOKEN);
 
         if (accessToken == null) {
-            throw new IdentityBrokerException("No access token from server.");
+            throw new IdentityBrokerException("No access token available in OAuth server response: " + response);
         }
 
         return doGetFederatedIdentity(accessToken);
@@ -140,7 +151,6 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
         return null;
     }
 
-    ;
 
     protected UriBuilder createAuthorizationUrl(AuthenticationRequest request) {
         return UriBuilder.fromUri(getConfig().getAuthorizationUrl())
@@ -151,9 +161,20 @@ public abstract class AbstractOAuth2IdentityProvider<C extends OAuth2IdentityPro
                 .queryParam(OAUTH2_PARAMETER_REDIRECT_URI, request.getRedirectUri());
     }
 
+    /**
+     * Get JSON property as text. JSON numbers and booleans are converted to text. Empty string is converted to null. 
+     * 
+     * @param jsonNode to get property from
+     * @param name of property to get
+     * @return string value of the property or null.
+     */
     protected String getJsonProperty(JsonNode jsonNode, String name) {
-        if (jsonNode.has(name)) {
+        if (jsonNode.has(name) && !jsonNode.get(name).isNull()) {
-            return jsonNode.get(name).asText();
+        	  String s = jsonNode.get(name).asText();
+        	  if(s != null && !s.isEmpty())
+        	  		return s;
+        	  else
+      	  			return null;
         }
 
         return null;

broker/oidc/src/test/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProviderTest.java

@@ -0,0 +1,138 @@
+/*
+ * JBoss, Home of Professional Open Source
+ * Copyright 2015 Red Hat Inc. and/or its affiliates and other contributors
+ * as indicated by the @authors tag. All rights reserved.
+ */
+package org.keycloak.broker.oidc;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.codehaus.jackson.JsonNode;
+import org.junit.Assert;
+import org.junit.Test;
+import org.keycloak.broker.provider.FederatedIdentity;
+import org.keycloak.broker.provider.IdentityBrokerException;
+import org.keycloak.models.IdentityProviderModel;
+
+/**
+ * Unit test for {@link AbstractOAuth2IdentityProvider}
+ * 
+ * @author Vlastimil Elias (velias at redhat dot com)
+ */
+public class AbstractOAuth2IdentityProviderTest {
+	
+	@Test
+	public void constructor_defaultScopeHandling(){
+		TestProvider tested = getTested();
+		
+		//default scope is set from the provider if not provided in the configuration
+		Assert.assertEquals( tested.getDefaultScopes(), tested.getConfig().getDefaultScope());
+		
+		//default scope is preserved if provided in the configuration
+		IdentityProviderModel model = new IdentityProviderModel();
+		OAuth2IdentityProviderConfig config = new OAuth2IdentityProviderConfig(model);
+		config.setDefaultScope("myscope");
+		tested = new TestProvider(config);
+
+		Assert.assertEquals("myscope", tested.getConfig().getDefaultScope());
+		
+	}
+
+	@Test
+	public void getJsonProperty_asJsonNode() throws IOException {
+		TestProvider tested = getTested();
+
+		JsonNode jsonNode = tested
+				.asJsonNode("{\"nullone\":null, \"emptyone\":\"\", \"blankone\": \" \", \"withvalue\" : \"my value\", \"withbooleanvalue\" : true, \"withnumbervalue\" : 10}");
+		Assert.assertNull(tested.getJsonProperty(jsonNode, "nonexisting"));
+		Assert.assertNull(tested.getJsonProperty(jsonNode, "nullone"));
+		Assert.assertNull(tested.getJsonProperty(jsonNode, "emptyone"));
+		Assert.assertEquals(" ", tested.getJsonProperty(jsonNode, "blankone"));
+		Assert.assertEquals("my value", tested.getJsonProperty(jsonNode, "withvalue"));
+		Assert.assertEquals("true", tested.getJsonProperty(jsonNode, "withbooleanvalue"));
+		Assert.assertEquals("10", tested.getJsonProperty(jsonNode, "withnumbervalue"));
+	}
+
+	@Test(expected = IdentityBrokerException.class)
+	public void getFederatedIdentity_responseUrlLine_tokenNotFound() {
+		TestProvider tested = getTested();
+		Map<String, String> notes = new HashMap<>();
+		tested.getFederatedIdentity(notes, "cosi=sss");
+	}
+
+	@Test(expected = IdentityBrokerException.class)
+	public void getFederatedIdentity_responseJSON_tokenNotFound() {
+		TestProvider tested = getTested();
+		Map<String, String> notes = new HashMap<>();
+		tested.getFederatedIdentity(notes, "{\"cosi\":\"sss\"}");
+	}
+
+	@Test(expected = IdentityBrokerException.class)
+	public void getFederatedIdentity_responseJSON_invalidFormat() {
+		TestProvider tested = getTested();
+		Map<String, String> notes = new HashMap<>();
+		tested.getFederatedIdentity(notes, "{\"cosi\":\"sss\"");
+	}
+
+	@Test(expected = IdentityBrokerException.class)
+	public void getFederatedIdentity_responseJSON_emptyTokenField() {
+		TestProvider tested = getTested();
+		Map<String, String> notes = new HashMap<>();
+		tested.getFederatedIdentity(notes, "{\""
+				+ AbstractOAuth2IdentityProvider.OAUTH2_PARAMETER_ACCESS_TOKEN + "\" : \"\"}");
+	}
+
+	@Test(expected = IdentityBrokerException.class)
+	public void getFederatedIdentity_responseJSON_nullTokenField() {
+		TestProvider tested = getTested();
+		Map<String, String> notes = new HashMap<>();
+		tested.getFederatedIdentity(notes, "{\""
+				+ AbstractOAuth2IdentityProvider.OAUTH2_PARAMETER_ACCESS_TOKEN + "\" : null}");
+	}
+
+	@Test
+	public void getFederatedIdentity_responseJSON() {
+		TestProvider tested = getTested();
+		Map<String, String> notes = new HashMap<>();
+		FederatedIdentity fi = tested.getFederatedIdentity(notes, "{\""
+				+ AbstractOAuth2IdentityProvider.OAUTH2_PARAMETER_ACCESS_TOKEN + "\" : \"458rt\"}");
+		Assert.assertNotNull(fi);
+		Assert.assertEquals("458rt", fi.getId());
+	}
+
+	@Test
+	public void getFederatedIdentity_responseUrlLine() {
+		TestProvider tested = getTested();
+		Map<String, String> notes = new HashMap<>();
+		FederatedIdentity fi = tested.getFederatedIdentity(notes, "cosi=sss&"
+				+ AbstractOAuth2IdentityProvider.OAUTH2_PARAMETER_ACCESS_TOKEN + "=458rtf&kdesi=ss}");
+		Assert.assertNotNull(fi);
+		Assert.assertEquals("458rtf", fi.getId());
+	}
+
+	private TestProvider getTested() {
+		IdentityProviderModel model = new IdentityProviderModel();
+		OAuth2IdentityProviderConfig config = new OAuth2IdentityProviderConfig(model);
+		return new TestProvider(config);
+	}
+
+	private static class TestProvider extends AbstractOAuth2IdentityProvider<OAuth2IdentityProviderConfig> {
+
+		public TestProvider(OAuth2IdentityProviderConfig config) {
+			super(config);
+		}
+
+		@Override
+		protected String getDefaultScopes() {
+			return "default";
+		}
+
+		protected FederatedIdentity doGetFederatedIdentity(String accessToken) {
+			return new FederatedIdentity(accessToken);
+		};
+
+	};
+
+}