From 4a012b73ea692c4703175b4e00876fd44ca461f5 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Fri, 8 Dec 2017 01:19:57 +0100 Subject: [PATCH] KEYCLOAK-4998 Fix NPE in AttributeToRoleMapper --- .../saml/mappers/AttributeToRoleMapper.java | 2 +- .../testsuite/broker/AbstractBrokerTest.java | 37 +++++--- .../testsuite/broker/KcOidcBrokerTest.java | 8 +- .../testsuite/broker/KcSamlBrokerTest.java | 87 +++++++++++++++++-- 4 files changed, 114 insertions(+), 20 deletions(-) diff --git a/services/src/main/java/org/keycloak/broker/saml/mappers/AttributeToRoleMapper.java b/services/src/main/java/org/keycloak/broker/saml/mappers/AttributeToRoleMapper.java index cb1e35121e..091723d55e 100755 --- a/services/src/main/java/org/keycloak/broker/saml/mappers/AttributeToRoleMapper.java +++ b/services/src/main/java/org/keycloak/broker/saml/mappers/AttributeToRoleMapper.java @@ -127,7 +127,7 @@ public class AttributeToRoleMapper extends AbstractIdentityProviderMapper { for (AttributeStatementType.ASTChoiceType choice : statement.getAttributes()) { AttributeType attr = choice.getAttribute(); if (name != null && !name.equals(attr.getName())) continue; - if (friendly != null && !name.equals(attr.getFriendlyName())) continue; + if (friendly != null && !friendly.equals(attr.getFriendlyName())) continue; for (Object val : attr.getAttributeValue()) { if (val.equals(desiredValue)) return true; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java index 6332dd0c78..b6c05b776d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java @@ -20,6 +20,7 @@ import org.openqa.selenium.TimeoutException; import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -34,10 +35,18 @@ import org.jboss.arquillian.graphene.page.Page; import javax.ws.rs.core.Response; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertThat; import static org.keycloak.testsuite.broker.BrokerTestTools.*; public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { + public static final String ROLE_USER = "user"; + public static final String ROLE_MANAGER = "manager"; + public static final String ROLE_FRIENDLY_MANAGER = "friendly-manager"; + protected IdentityProviderResource identityProviderResource; @Before @@ -338,9 +347,12 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { } protected void createRolesForRealm(String realm) { - RoleRepresentation managerRole = new RoleRepresentation("manager",null, false); - RoleRepresentation userRole = new RoleRepresentation("user",null, false); + RoleRepresentation managerRole = new RoleRepresentation(ROLE_MANAGER,null, false); + RoleRepresentation friendlyManagerRole = new RoleRepresentation(ROLE_FRIENDLY_MANAGER,null, false); + RoleRepresentation userRole = new RoleRepresentation(ROLE_USER,null, false); + adminClient.realm(realm).roles().create(managerRole); + adminClient.realm(realm).roles().create(friendlyManagerRole); adminClient.realm(realm).roles().create(userRole); } @@ -367,27 +379,32 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { createRoleMappersForConsumerRealm(); - RoleRepresentation managerRole = adminClient.realm(bc.providerRealmName()).roles().get("manager").toRepresentation(); - RoleRepresentation userRole = adminClient.realm(bc.providerRealmName()).roles().get("user").toRepresentation(); + RoleRepresentation managerRole = adminClient.realm(bc.providerRealmName()).roles().get(ROLE_MANAGER).toRepresentation(); + RoleRepresentation userRole = adminClient.realm(bc.providerRealmName()).roles().get(ROLE_USER).toRepresentation(); UserResource userResource = adminClient.realm(bc.providerRealmName()).users().get(userId); userResource.roles().realmLevel().add(Collections.singletonList(managerRole)); logInAsUserInIDPForFirstTime(); - List currentRoles = userResource.roles().realmLevel().listAll(); - assertEquals("There should be manager role",1, currentRoles.stream().filter(role -> role.getName().equals("manager")).collect(Collectors.toList()).size()); - assertEquals("User shouldn't have user role", 0, currentRoles.stream().filter(role -> role.getName().equals("user")).collect(Collectors.toList()).size()); + Set currentRoles = userResource.roles().realmLevel().listAll().stream() + .map(RoleRepresentation::getName) + .collect(Collectors.toSet()); + + assertThat(currentRoles, hasItems(ROLE_MANAGER)); + assertThat(currentRoles, not(hasItems(ROLE_USER))); logoutFromRealm(bc.consumerRealmName()); + userResource.roles().realmLevel().add(Collections.singletonList(userRole)); logInAsUserInIDP(); - currentRoles = userResource.roles().realmLevel().listAll(); - assertEquals("There should be manager role",1, currentRoles.stream().filter(role -> role.getName().equals("manager")).collect(Collectors.toList()).size()); - assertEquals("There should be user role",1, currentRoles.stream().filter(role -> role.getName().equals("user")).collect(Collectors.toList()).size()); + currentRoles = userResource.roles().realmLevel().listAll().stream() + .map(RoleRepresentation::getName) + .collect(Collectors.toSet()); + assertThat(currentRoles, hasItems(ROLE_MANAGER, ROLE_USER)); logoutFromRealm(bc.providerRealmName()); logoutFromRealm(bc.consumerRealmName()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java index ef740da29a..0fdd82fb03 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java @@ -18,16 +18,16 @@ public class KcOidcBrokerTest extends AbstractBrokerTest { attrMapper1.setName("manager-role-mapper"); attrMapper1.setIdentityProviderMapper(ExternalKeycloakRoleToRoleMapper.PROVIDER_ID); attrMapper1.setConfig(ImmutableMap.builder() - .put("external.role", "manager") - .put("role", "manager") + .put("external.role", ROLE_MANAGER) + .put("role", ROLE_MANAGER) .build()); IdentityProviderMapperRepresentation attrMapper2 = new IdentityProviderMapperRepresentation(); attrMapper2.setName("user-role-mapper"); attrMapper2.setIdentityProviderMapper(ExternalKeycloakRoleToRoleMapper.PROVIDER_ID); attrMapper2.setConfig(ImmutableMap.builder() - .put("external.role", "user") - .put("role", "user") + .put("external.role", ROLE_USER) + .put("role", ROLE_USER) .build()); return Lists.newArrayList(attrMapper1, attrMapper2); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerTest.java index 739224c412..1432924791 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerTest.java @@ -1,15 +1,28 @@ package org.keycloak.testsuite.broker; +import org.keycloak.admin.client.resource.UserResource; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import org.keycloak.broker.saml.mappers.AttributeToRoleMapper; import org.keycloak.broker.saml.mappers.UserAttributeMapper; import org.keycloak.protocol.saml.SamlProtocol; import org.keycloak.representations.idm.IdentityProviderMapperRepresentation; +import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.services.resources.RealmsResource; import java.net.URI; +import java.util.Collections; +import java.util.Set; +import java.util.stream.Collectors; import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriBuilderException; +import org.junit.Test; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertThat; +import static org.keycloak.testsuite.broker.AbstractBrokerTest.ROLE_FRIENDLY_MANAGER; +import static org.keycloak.testsuite.broker.AbstractBrokerTest.ROLE_MANAGER; +import static org.keycloak.testsuite.broker.AbstractBrokerTest.ROLE_USER; public class KcSamlBrokerTest extends AbstractBrokerTest { @@ -25,8 +38,8 @@ public class KcSamlBrokerTest extends AbstractBrokerTest { attrMapper1.setIdentityProviderMapper(AttributeToRoleMapper.PROVIDER_ID); attrMapper1.setConfig(ImmutableMap.builder() .put(UserAttributeMapper.ATTRIBUTE_NAME, "Role") - .put(ATTRIBUTE_VALUE, "manager") - .put("role", "manager") + .put(ATTRIBUTE_VALUE, ROLE_MANAGER) + .put("role", ROLE_MANAGER) .build()); IdentityProviderMapperRepresentation attrMapper2 = new IdentityProviderMapperRepresentation(); @@ -34,11 +47,75 @@ public class KcSamlBrokerTest extends AbstractBrokerTest { attrMapper2.setIdentityProviderMapper(AttributeToRoleMapper.PROVIDER_ID); attrMapper2.setConfig(ImmutableMap.builder() .put(UserAttributeMapper.ATTRIBUTE_NAME, "Role") - .put(ATTRIBUTE_VALUE, "user") - .put("role", "user") + .put(ATTRIBUTE_VALUE, ROLE_USER) + .put("role", ROLE_USER) .build()); - return Lists.newArrayList(attrMapper1, attrMapper2); + IdentityProviderMapperRepresentation attrMapper3 = new IdentityProviderMapperRepresentation(); + attrMapper3.setName("friendly-mapper"); + attrMapper3.setIdentityProviderMapper(AttributeToRoleMapper.PROVIDER_ID); + attrMapper3.setConfig(ImmutableMap.builder() + .put(UserAttributeMapper.ATTRIBUTE_FRIENDLY_NAME, AbstractUserAttributeMapperTest.ATTRIBUTE_TO_MAP_FRIENDLY_NAME) + .put(ATTRIBUTE_VALUE, ROLE_FRIENDLY_MANAGER) + .put("role", ROLE_FRIENDLY_MANAGER) + .build()); + + return Lists.newArrayList(attrMapper1, attrMapper2, attrMapper3); + } + + // KEYCLOAK-3987 + @Test + @Override + public void grantNewRoleFromToken() { + createRolesForRealm(bc.providerRealmName()); + createRolesForRealm(bc.consumerRealmName()); + + createRoleMappersForConsumerRealm(); + + RoleRepresentation managerRole = adminClient.realm(bc.providerRealmName()).roles().get(ROLE_MANAGER).toRepresentation(); + RoleRepresentation friendlyManagerRole = adminClient.realm(bc.providerRealmName()).roles().get(ROLE_FRIENDLY_MANAGER).toRepresentation(); + RoleRepresentation userRole = adminClient.realm(bc.providerRealmName()).roles().get(ROLE_USER).toRepresentation(); + + UserResource userResource = adminClient.realm(bc.providerRealmName()).users().get(userId); + userResource.roles().realmLevel().add(Collections.singletonList(managerRole)); + + logInAsUserInIDPForFirstTime(); + + Set currentRoles = userResource.roles().realmLevel().listAll().stream() + .map(RoleRepresentation::getName) + .collect(Collectors.toSet()); + + assertThat(currentRoles, hasItems(ROLE_MANAGER)); + assertThat(currentRoles, not(hasItems(ROLE_USER, ROLE_FRIENDLY_MANAGER))); + + logoutFromRealm(bc.consumerRealmName()); + + + userResource.roles().realmLevel().add(Collections.singletonList(userRole)); + userResource.roles().realmLevel().add(Collections.singletonList(friendlyManagerRole)); + + logInAsUserInIDP(); + + currentRoles = userResource.roles().realmLevel().listAll().stream() + .map(RoleRepresentation::getName) + .collect(Collectors.toSet()); + assertThat(currentRoles, hasItems(ROLE_MANAGER, ROLE_USER, ROLE_FRIENDLY_MANAGER)); + + logoutFromRealm(bc.consumerRealmName()); + + + userResource.roles().realmLevel().remove(Collections.singletonList(friendlyManagerRole)); + + logInAsUserInIDP(); + + currentRoles = userResource.roles().realmLevel().listAll().stream() + .map(RoleRepresentation::getName) + .collect(Collectors.toSet()); + assertThat(currentRoles, hasItems(ROLE_MANAGER, ROLE_USER)); + assertThat(currentRoles, not(hasItems(ROLE_FRIENDLY_MANAGER))); + + logoutFromRealm(bc.providerRealmName()); + logoutFromRealm(bc.consumerRealmName()); } protected URI getAuthServerSamlEndpoint(String realm) throws IllegalArgumentException, UriBuilderException {