diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java index 4ddd142f23..d95abdb7d1 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java @@ -229,7 +229,9 @@ public class LDAPFederationProvider implements UserFederationProvider { List result = new ArrayList<>(); for (String username : usernames) { UserModel kcUser = session.users().getUserByUsername(username, realm); - if (!model.getId().equals(kcUser.getFederationLink())) { + if (kcUser == null) { + logger.warnf("User '%s' referenced by membership wasn't found in LDAP", username); + } else if (!model.getId().equals(kcUser.getFederationLink())) { logger.warnf("Incorrect federation provider of user %s" + kcUser.getUsername()); } else { result.add(kcUser); diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java index cd220b292a..8713de7d19 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java @@ -27,7 +27,6 @@ import java.util.Map; import java.util.Set; import org.jboss.logging.Logger; -import org.keycloak.federation.ldap.LDAPConfig; import org.keycloak.federation.ldap.LDAPFederationProvider; import org.keycloak.federation.ldap.LDAPUtils; import org.keycloak.federation.ldap.idm.model.LDAPDn; @@ -42,11 +41,9 @@ import org.keycloak.federation.ldap.mappers.membership.LDAPGroupMapperMode; import org.keycloak.federation.ldap.mappers.membership.MembershipType; import org.keycloak.federation.ldap.mappers.membership.UserRolesRetrieveStrategy; import org.keycloak.models.GroupModel; -import org.keycloak.models.LDAPConstants; import org.keycloak.models.ModelException; import org.keycloak.models.RealmModel; import org.keycloak.models.UserFederationMapperModel; -import org.keycloak.models.UserFederationProviderModel; import org.keycloak.models.UserFederationSyncResult; import org.keycloak.models.UserModel; import org.keycloak.models.utils.KeycloakModelUtils; diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java index 45f51c3df6..7307d81233 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java @@ -20,11 +20,14 @@ import org.jboss.resteasy.annotations.cache.NoCache; import org.jboss.resteasy.spi.NotFoundException; import org.keycloak.events.admin.OperationType; import org.keycloak.models.ClientModel; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ModelException; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleMapperModel; import org.keycloak.models.RoleModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.services.ErrorResponseException; import org.keycloak.services.ServicesLogger; import javax.ws.rs.Consumes; @@ -34,11 +37,14 @@ import javax.ws.rs.POST; import javax.ws.rs.Path; import javax.ws.rs.Produces; import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; +import java.text.MessageFormat; import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Properties; import java.util.Set; /** @@ -48,6 +54,7 @@ import java.util.Set; public class ClientRoleMappingsResource { protected static final ServicesLogger logger = ServicesLogger.ROOT_LOGGER; + protected KeycloakSession session; protected RealmModel realm; protected RealmAuth auth; protected RoleMapperModel user; @@ -55,8 +62,9 @@ public class ClientRoleMappingsResource { protected AdminEventBuilder adminEvent; private UriInfo uriInfo; - public ClientRoleMappingsResource(UriInfo uriInfo, RealmModel realm, RealmAuth auth, RoleMapperModel user, ClientModel client, AdminEventBuilder adminEvent) { + public ClientRoleMappingsResource(UriInfo uriInfo, KeycloakSession session, RealmModel realm, RealmAuth auth, RoleMapperModel user, ClientModel client, AdminEventBuilder adminEvent) { this.uriInfo = uriInfo; + this.session = session; this.realm = realm; this.auth = auth; this.user = user; @@ -182,7 +190,14 @@ public class ClientRoleMappingsResource { if (roleModel == null || !roleModel.getId().equals(role.getId())) { throw new NotFoundException("Role not found"); } - user.deleteRoleMapping(roleModel); + + try { + user.deleteRoleMapping(roleModel); + } catch (ModelException me) { + Properties messages = AdminRoot.getMessages(session, realm, auth.getAuth().getToken().getLocale()); + throw new ErrorResponseException(me.getMessage(), MessageFormat.format(messages.getProperty(me.getMessage(), me.getMessage()), me.getParameters()), + Response.Status.BAD_REQUEST); + } } } adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).representation(roles).success(); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java index b324b5cdf9..5bd67d1961 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java @@ -22,6 +22,7 @@ import org.keycloak.common.ClientConnection; import org.keycloak.events.admin.OperationType; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ModelException; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleMapperModel; import org.keycloak.models.RoleModel; @@ -29,6 +30,7 @@ import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.ClientMappingsRepresentation; import org.keycloak.representations.idm.MappingsRepresentation; import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.services.ErrorResponseException; import org.keycloak.services.ServicesLogger; import org.keycloak.services.managers.RealmManager; @@ -42,11 +44,15 @@ import javax.ws.rs.Produces; import javax.ws.rs.core.Context; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; + +import java.text.MessageFormat; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Properties; import java.util.Set; /** @@ -238,7 +244,14 @@ public class RoleMapperResource { if (roleModel == null || !roleModel.getId().equals(role.getId())) { throw new NotFoundException("Role not found"); } - roleMapper.deleteRoleMapping(roleModel); + + try { + roleMapper.deleteRoleMapping(roleModel); + } catch (ModelException me) { + Properties messages = AdminRoot.getMessages(session, realm, auth.getAuth().getToken().getLocale()); + throw new ErrorResponseException(me.getMessage(), MessageFormat.format(messages.getProperty(me.getMessage(), me.getMessage()), me.getParameters()), + Response.Status.BAD_REQUEST); + } adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo, role.getId()).representation(roles).success(); } @@ -253,7 +266,7 @@ public class RoleMapperResource { throw new NotFoundException("Client not found"); } - return new ClientRoleMappingsResource(uriInfo, realm, auth, roleMapper, clientModel, adminEvent); + return new ClientRoleMappingsResource(uriInfo, session, realm, auth, roleMapper, clientModel, adminEvent); } } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java index 9b035356ed..ad7feec906 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java @@ -939,7 +939,14 @@ public class UsersResource { if (group == null) { throw new NotFoundException("Group not found"); } - if (user.isMemberOf(group)) user.leaveGroup(group); + + try { + if (user.isMemberOf(group)) user.leaveGroup(group); + } catch (ModelException me) { + Properties messages = AdminRoot.getMessages(session, realm, auth.getAuth().getToken().getLocale()); + throw new ErrorResponseException(me.getMessage(), MessageFormat.format(messages.getProperty(me.getMessage(), me.getMessage()), me.getParameters()), + Response.Status.BAD_REQUEST); + } } @PUT diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPGroupMapperTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPGroupMapperTest.java index 43a9634527..b3f652075c 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPGroupMapperTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/LDAPGroupMapperTest.java @@ -31,6 +31,7 @@ import org.junit.runners.MethodSorters; import org.keycloak.federation.ldap.LDAPFederationProvider; import org.keycloak.federation.ldap.LDAPFederationProviderFactory; import org.keycloak.federation.ldap.LDAPUtils; +import org.keycloak.federation.ldap.idm.model.LDAPDn; import org.keycloak.federation.ldap.idm.model.LDAPObject; import org.keycloak.federation.ldap.mappers.membership.LDAPGroupMapperMode; import org.keycloak.federation.ldap.mappers.membership.MembershipType; @@ -302,6 +303,47 @@ public class LDAPGroupMapperTest { } } + + // KEYCLOAK-2682 + @Test + public void test04_groupReferencingNonExistentMember() { + KeycloakSession session = keycloakRule.startSession(); + try { + RealmModel appRealm = session.realms().getRealmByName("test"); + + UserFederationMapperModel mapperModel = appRealm.getUserFederationMapperByName(ldapModel.getId(), "groupsMapper"); + FederationTestUtils.updateGroupMapperConfigOptions(mapperModel, GroupMapperConfig.MODE, LDAPGroupMapperMode.LDAP_ONLY.toString()); + appRealm.updateUserFederationMapper(mapperModel); + + // 1 - Add some group to LDAP for testing + LDAPFederationProvider ldapProvider = FederationTestUtils.getLdapProvider(session, ldapModel); + GroupLDAPFederationMapper groupMapper = FederationTestUtils.getGroupMapper(mapperModel, ldapProvider, appRealm); + LDAPObject group2 = FederationTestUtils.createLDAPGroup(session, appRealm, ldapModel, "group2", descriptionAttrName, "group2 - description"); + + // 2 - Add one existing user rob to LDAP group + LDAPObject robLdap = ldapProvider.loadLDAPUserByUsername(appRealm, "robkeycloak"); + LDAPUtils.addMember(ldapProvider, MembershipType.DN, LDAPConstants.MEMBER, group2, robLdap, false); + + // 3 - Add non-existing user to LDAP group + LDAPDn nonExistentDn = LDAPDn.fromString(ldapProvider.getLdapIdentityStore().getConfig().getUsersDn()); + nonExistentDn.addFirst(robLdap.getRdnAttributeName(), "nonexistent"); + LDAPObject nonExistentLdapUser = new LDAPObject(); + nonExistentLdapUser.setDn(nonExistentDn); + LDAPUtils.addMember(ldapProvider, MembershipType.DN, LDAPConstants.MEMBER, group2, nonExistentLdapUser, true); + + // 4 - Check group members. Just existing user rob should be present + groupMapper.syncDataFromFederationProviderToKeycloak(); + GroupModel kcGroup2 = KeycloakModelUtils.findGroupByPath(appRealm, "/group2"); + List groupUsers = session.users().getGroupMembers(appRealm, kcGroup2, 0, 5); + Assert.assertEquals(1, groupUsers.size()); + UserModel rob = groupUsers.get(0); + Assert.assertEquals("robkeycloak", rob.getUsername()); + + } finally { + keycloakRule.stopSession(session, false); + } + } + private void deleteGroupMappingsInLDAP(GroupLDAPFederationMapper groupMapper, LDAPObject ldapUser, String groupName) { LDAPObject ldapGroup = groupMapper.loadLDAPGroupByName(groupName); groupMapper.deleteGroupMappingInLDAP(ldapUser, ldapGroup); diff --git a/themes/src/main/resources/theme/base/admin/resources/js/controllers/users.js b/themes/src/main/resources/theme/base/admin/resources/js/controllers/users.js index 4ca5bf8ed3..33a5d2b097 100755 --- a/themes/src/main/resources/theme/base/admin/resources/js/controllers/users.js +++ b/themes/src/main/resources/theme/base/admin/resources/js/controllers/users.js @@ -59,6 +59,12 @@ module.controller('UserRoleMappingCtrl', function($scope, $http, realm, user, cl $scope.selectedClientMappings = []; } Notifications.success("Role mappings updated."); + }).error(function(response) { + if (response && response['error_description']) { + Notifications.error(response['error_description']); + } else { + Notifications.error("Failed to remove role mapping"); + } }); }; @@ -87,6 +93,12 @@ module.controller('UserRoleMappingCtrl', function($scope, $http, realm, user, cl $scope.realmComposite = CompositeRealmRoleMapping.query({realm : realm.realm, userId : user.id}); $scope.realmRoles = AvailableRealmRoleMapping.query({realm : realm.realm, userId : user.id}); Notifications.success("Role mappings updated."); + }).error(function(response) { + if (response && response['error_description']) { + Notifications.error(response['error_description']); + } else { + Notifications.error("Failed to remove role mapping"); + } }); }; @@ -1170,6 +1182,12 @@ module.controller('UserGroupMembershipCtrl', function($scope, $route, realm, gro UserGroupMapping.remove({realm: realm.realm, userId: user.id, groupId: $scope.selectedGroup.id}, function() { Notifications.success('Removed group membership'); $route.reload(); + }, function(response) { + if (response.data && response.data['error_description']) { + Notifications.error(response.data['error_description']); + } else { + Notifications.error("Failed to leave group"); + } }); };