From 5453bec1bf2a053c293b02db200984fd1a231f3c Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Thu, 15 Dec 2016 08:08:25 +0100 Subject: [PATCH] KEYCLOAK-4079, KEYCLOAK-4080 Fix for single-valued claims --- .../mappers/OIDCAttributeMapperHelper.java | 4 +-- .../admin/group/GroupMappersTest.java | 9 ++---- .../broker/KcOidcBrokerConfiguration.java | 2 +- .../oauth/OIDCProtocolMappersTest.java | 28 +++++++++++-------- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java index 7b8ba36716..5acde8b1ac 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java @@ -56,7 +56,7 @@ public class OIDCAttributeMapperHelper { if (attributeValue instanceof List) { List valueAsList = (List) attributeValue; - if (valueAsList.size() == 0) return null; + if (valueAsList.isEmpty()) return null; if (isMultivalued(mappingModel)) { List result = new ArrayList<>(); @@ -69,7 +69,7 @@ public class OIDCAttributeMapperHelper { ServicesLogger.LOGGER.multipleValuesForMapper(attributeValue.toString(), mappingModel.getName()); } - attributeValue = valueAsList; + attributeValue = valueAsList.get(0); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupMappersTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupMappersTest.java index a1e6b0ad36..b1338e9e6d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupMappersTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupMappersTest.java @@ -34,9 +34,6 @@ import org.keycloak.representations.idm.UserRepresentation; import java.util.*; -import static org.hamcrest.CoreMatchers.hasItems; -import static org.hamcrest.MatcherAssert.assertThat; - /** * @author Marko Strukelj */ @@ -119,7 +116,7 @@ public class GroupMappersTest extends AbstractGroupTest { Assert.assertNotNull(groups); Assert.assertTrue(groups.size() == 1); Assert.assertEquals("topGroup", groups.get(0)); - Assert.assertEquals(Collections.singletonList("true"), token.getOtherClaims().get("topAttribute")); + Assert.assertEquals("true", token.getOtherClaims().get("topAttribute")); } { UserRepresentation user = realm.users().search("level2GroupUser", -1, -1).get(0); @@ -132,8 +129,8 @@ public class GroupMappersTest extends AbstractGroupTest { Assert.assertNotNull(groups); Assert.assertTrue(groups.size() == 1); Assert.assertEquals("level2group", groups.get(0)); - Assert.assertEquals(Collections.singletonList("true"), token.getOtherClaims().get("topAttribute")); - Assert.assertEquals(Collections.singletonList("true"), token.getOtherClaims().get("level2Attribute")); + Assert.assertEquals("true", token.getOtherClaims().get("topAttribute")); + Assert.assertEquals("true", token.getOtherClaims().get("level2Attribute")); } } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java index 1fdb896275..aa3e56efe3 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java @@ -87,7 +87,7 @@ public class KcOidcBrokerConfiguration implements BrokerConfiguration { userAttrMapperConfig.put(OIDCAttributeMapperHelper.INCLUDE_IN_ACCESS_TOKEN, "true"); userAttrMapperConfig.put(OIDCAttributeMapperHelper.INCLUDE_IN_ID_TOKEN, "true"); userAttrMapperConfig.put(OIDCAttributeMapperHelper.INCLUDE_IN_USERINFO, "true"); -// userAttrMapperConfig.put(ProtocolMapperUtils.MULTIVALUED, "true"); + userAttrMapperConfig.put(ProtocolMapperUtils.MULTIVALUED, "true"); client.setProtocolMappers(Arrays.asList(emailMapper, userAttrMapper)); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java index b896588852..ec2d9f5b79 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java @@ -115,17 +115,18 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { mapper.getConfig().put(AddressMapper.getModelPropertyName(AddressClaimSet.REGION), "region_some"); mapper.getConfig().put(AddressMapper.getModelPropertyName(AddressClaimSet.COUNTRY), "country_some"); mapper.getConfig().remove(AddressMapper.getModelPropertyName(AddressClaimSet.POSTAL_CODE)); // Even if we remove protocolMapper config property, it should still default to postal_code - app.getProtocolMappers().createMapper(mapper); + app.getProtocolMappers().createMapper(mapper).close(); ProtocolMapperRepresentation hard = createHardcodedClaim("hard", "hard", "coded", "String", false, null, true, true); - app.getProtocolMappers().createMapper(hard); - app.getProtocolMappers().createMapper(createHardcodedClaim("hard-nested", "nested.hard", "coded-nested", "String", false, null, true, true)); - app.getProtocolMappers().createMapper(createClaimMapper("custom phone", "phone", "home_phone", "String", true, "", true, true, false)); - app.getProtocolMappers().createMapper(createClaimMapper("nested phone", "phone", "home.phone", "String", true, "", true, true, false)); - app.getProtocolMappers().createMapper(createClaimMapper("departments", "departments", "department", "String", true, "", true, true, true)); - app.getProtocolMappers().createMapper(createHardcodedRole("hard-realm", "hardcoded")); - app.getProtocolMappers().createMapper(createHardcodedRole("hard-app", "app.hardcoded")); - app.getProtocolMappers().createMapper(createRoleNameMapper("rename-app-role", "test-app.customer-user", "realm-user")); + app.getProtocolMappers().createMapper(hard).close(); + app.getProtocolMappers().createMapper(createHardcodedClaim("hard-nested", "nested.hard", "coded-nested", "String", false, null, true, true)).close(); + app.getProtocolMappers().createMapper(createClaimMapper("custom phone", "phone", "home_phone", "String", true, "", true, true, true)).close(); + app.getProtocolMappers().createMapper(createClaimMapper("nested phone", "phone", "home.phone", "String", true, "", true, true, true)).close(); + app.getProtocolMappers().createMapper(createClaimMapper("departments", "departments", "department", "String", true, "", true, true, true)).close(); + app.getProtocolMappers().createMapper(createClaimMapper("firstDepartment", "departments", "firstDepartment", "String", true, "", true, true, false)).close(); + app.getProtocolMappers().createMapper(createHardcodedRole("hard-realm", "hardcoded")).close(); + app.getProtocolMappers().createMapper(createHardcodedRole("hard-app", "app.hardcoded")).close(); + app.getProtocolMappers().createMapper(createRoleNameMapper("rename-app-role", "test-app.customer-user", "realm-user")).close(); } { @@ -147,9 +148,13 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { assertEquals("coded-nested", nested.get("hard")); nested = (Map) idToken.getOtherClaims().get("home"); assertThat((List) nested.get("phone"), hasItems("617-777-6666")); + List departments = (List) idToken.getOtherClaims().get("department"); - assertEquals(2, departments.size()); - assertTrue(departments.contains("finance") && departments.contains("development")); + assertThat(departments, containsInAnyOrder("finance", "development")); + + Object firstDepartment = idToken.getOtherClaims().get("firstDepartment"); + assertThat(firstDepartment, instanceOf(String.class)); + assertThat(firstDepartment, is("finance")); // Has to be the first item AccessToken accessToken = oauth.verifyToken(response.getAccessToken()); assertEquals(accessToken.getName(), "Tom Brady"); @@ -186,6 +191,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { || model.getName().equals("hard-nested") || model.getName().equals("custom phone") || model.getName().equals("departments") + || model.getName().equals("firstDepartment") || model.getName().equals("nested phone") || model.getName().equals("rename-app-role") || model.getName().equals("hard-realm")