diff --git a/kernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/BaseAuthzGroup.java b/kernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/BaseAuthzGroup.java index aa43b80a3d39..f5acb5aa14ce 100644 --- a/kernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/BaseAuthzGroup.java +++ b/kernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/BaseAuthzGroup.java @@ -1022,7 +1022,8 @@ public void addMember(String user, String roleId, boolean active, boolean provid if (role == null) { log.warn(".addUserRole: role undefined: " + roleId); - throw new IllegalArgumentException("addMember called with null role!"); + throw new IllegalArgumentException("addMember user: "+ user+ "called with roleId: "+ roleId + + " that isn't found on authzGroupId: "+ m_id); } BaseMember grant = (BaseMember) m_userGrants.get(user); diff --git a/kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseGroup.java b/kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseGroup.java index c5f3b6e5768d..7523032f46f4 100644 --- a/kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseGroup.java +++ b/kernel/kernel-impl/src/main/java/org/sakaiproject/site/impl/BaseGroup.java @@ -429,7 +429,31 @@ public void insertMember(String userId, String roleId, boolean active, boolean p throw new IllegalStateException("Error, cannot add " + userId + " with role " + roleId + " into a locked group"); } m_azgChanged = true; - getAzg().addMember(userId, roleId, active, provided); + try + { + getAzg().addMember(userId, roleId, active, provided); + } + catch (IllegalArgumentException iae) + { + // In the same way that we copy across all roles when a group is created, when adding a member + // if the role isn't defined in the group, look in the site and copy it when adding. + Role siteRole = getContainingSite().getRole(roleId); + if (siteRole != null) + { + try + { + getAzg().addRole(roleId, siteRole); + } + catch (RoleAlreadyDefinedException ignore) // Possibly added by another thread. + { + } + getAzg().addMember(userId, roleId, active, provided); + } + else + { + throw iae; + } + } } public Role addRole(String id) throws RoleAlreadyDefinedException diff --git a/site-manage/site-manage-group-section-role-helper/tool/src/bundle/org/sakaiproject/site/tool/managegroupsectionrole/bundle/Messages.properties b/site-manage/site-manage-group-section-role-helper/tool/src/bundle/org/sakaiproject/site/tool/managegroupsectionrole/bundle/Messages.properties index b26c044aa8ff..e4a712911bc5 100644 --- a/site-manage/site-manage-group-section-role-helper/tool/src/bundle/org/sakaiproject/site/tool/managegroupsectionrole/bundle/Messages.properties +++ b/site-manage/site-manage-group-section-role-helper/tool/src/bundle/org/sakaiproject/site/tool/managegroupsectionrole/bundle/Messages.properties @@ -91,6 +91,8 @@ numGroups.zero.alert=Number of groups should be a value greater than 0. maxMembers.zero.alert=Max number of members should be a value greater than 0. maxMembers.empty.alert=Max number of members is required and should be a valid number maxGroups.alert=Max number of groups you can create is 1000 +user.not.member.alert={0} is not a member of the site. +user.unknown=Unknown group.joinable.currentgroups=Current groups: group.joinable.additionalGroups=Generate Additional Groups group.joinable.pendingGroups=Pending groups: diff --git a/site-manage/site-manage-group-section-role-helper/tool/src/java/org/sakaiproject/site/tool/helper/managegroupsectionrole/impl/SiteManageGroupSectionRoleHandler.java b/site-manage/site-manage-group-section-role-helper/tool/src/java/org/sakaiproject/site/tool/helper/managegroupsectionrole/impl/SiteManageGroupSectionRoleHandler.java index 7189d2b7f3fa..62d0bdb1fedb 100644 --- a/site-manage/site-manage-group-section-role-helper/tool/src/java/org/sakaiproject/site/tool/helper/managegroupsectionrole/impl/SiteManageGroupSectionRoleHandler.java +++ b/site-manage/site-manage-group-section-role-helper/tool/src/java/org/sakaiproject/site/tool/helper/managegroupsectionrole/impl/SiteManageGroupSectionRoleHandler.java @@ -775,23 +775,27 @@ else if (siteRoles.contains(memberId)) else { // normal user id - memberId = StringUtils.trimToNull(memberId); - if (memberId != null && group.getUserRole(memberId) == null) { - Role r = site.getUserRole(memberId); - Member m = site.getMember(memberId); - Role memberRole = m != null ? m.getRole() : null; - // for every member added through the "Manage - // Groups" interface, he should be defined as - // non-provided - // get role first from site definition. - // However, if the user is inactive, getUserRole would return null; then use member role instead - String roleString = r != null ? r.getId(): memberRole != null? memberRole.getId() : ""; - boolean active = m != null ? m.isActive() : true; - try { - group.insertMember(memberId, roleString, active,false); - addedGroupMember.add("uid=" + memberId + ";role=" + roleString + ";active=" + active + ";provided=false;groupId=" + group.getId()); - } catch (IllegalStateException e) { - log.error(".processAddGroup: User with id {} cannot be inserted in group with id {} because the group is locked", memberId, group.getId()); + String userId = StringUtils.trimToNull(memberId); + if (userId != null && group.getUserRole(userId) == null) { + Member m = site.getMember(userId); + // User isn't a member of the site, refusing to add them to the group. + if (m != null) { + Role memberRole = m.getRole(); + try { + group.addMember(userId, memberRole.getId(), m.isActive(), false); + addedGroupMember.add("uid=" + userId + ";role=" + memberRole.getId() + ";active=" + m.isActive() + ";provided=false;groupId=" + group.getId()); + } catch (IllegalStateException e) { + log.error(".processAddGroup: User with id {} cannot be inserted in group with id {} because the group is locked", memberId, group.getId()); + return null; + } + } else { + String displayName; + try { + displayName = userDirectoryService.getUser(userId).getDisplayName(); + } catch (UserNotDefinedException e) { + displayName = messageLocator.getMessage("user.unknown"); + } + messages.addMessage(new TargettedMessage("user.not.member.alert", new String[]{displayName}, "groupMembers-selection")); return null; } }