KEYCLOAK-4998 Fix NPE in AttributeToRoleMapper

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;
                 }

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 managerRole = new RoleRepresentation(ROLE_MANAGER,null, false);
-        RoleRepresentation userRole = new RoleRepresentation("user",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 managerRole = adminClient.realm(bc.providerRealmName()).roles().get(ROLE_MANAGER).toRepresentation();
-        RoleRepresentation userRole = adminClient.realm(bc.providerRealmName()).roles().get("user").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<RoleRepresentation> currentRoles = userResource.roles().realmLevel().listAll();
+        Set<String> currentRoles = userResource.roles().realmLevel().listAll().stream()
-        assertEquals("There should be manager role",1, currentRoles.stream().filter(role -> role.getName().equals("manager")).collect(Collectors.toList()).size());
+          .map(RoleRepresentation::getName)
-        assertEquals("User shouldn't have user role", 0, currentRoles.stream().filter(role -> role.getName().equals("user")).collect(Collectors.toList()).size());
+          .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();
+        currentRoles = userResource.roles().realmLevel().listAll().stream()
-        assertEquals("There should be manager role",1, currentRoles.stream().filter(role -> role.getName().equals("manager")).collect(Collectors.toList()).size());
+          .map(RoleRepresentation::getName)
-        assertEquals("There should be user role",1, currentRoles.stream().filter(role -> role.getName().equals("user")).collect(Collectors.toList()).size());
+          .collect(Collectors.toSet());
+        assertThat(currentRoles, hasItems(ROLE_MANAGER, ROLE_USER));
 
         logoutFromRealm(bc.providerRealmName());
         logoutFromRealm(bc.consumerRealmName());

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.<String,String>builder()
-                .put("external.role", "manager")
+                .put("external.role", ROLE_MANAGER)
-                .put("role", "manager")
+                .put("role", ROLE_MANAGER)
                 .build());
 
         IdentityProviderMapperRepresentation attrMapper2 = new IdentityProviderMapperRepresentation();
         attrMapper2.setName("user-role-mapper");
         attrMapper2.setIdentityProviderMapper(ExternalKeycloakRoleToRoleMapper.PROVIDER_ID);
         attrMapper2.setConfig(ImmutableMap.<String,String>builder()
-                .put("external.role", "user")
+                .put("external.role", ROLE_USER)
-                .put("role", "user")
+                .put("role", ROLE_USER)
                 .build());
 
         return Lists.newArrayList(attrMapper1, attrMapper2);

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.<String,String>builder()
                 .put(UserAttributeMapper.ATTRIBUTE_NAME, "Role")
-                .put(ATTRIBUTE_VALUE, "manager")
+                .put(ATTRIBUTE_VALUE, ROLE_MANAGER)
-                .put("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.<String,String>builder()
                 .put(UserAttributeMapper.ATTRIBUTE_NAME, "Role")
-                .put(ATTRIBUTE_VALUE, "user")
+                .put(ATTRIBUTE_VALUE, ROLE_USER)
-                .put("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.<String,String>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<String> 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 {