SAK-23694 Fail gracefully when role isn’t in group. (sakaiproject#5130)

When adding a member to a group that belongs to a site, if the role doesn’t exist in the group copy it out of the site and re-attempt the add of the member.

This follows the behaviour of creating the group (KNL-313) where the roles from the site are copied into the group even if they aren’t defined on the group template.

When failing to add a member to a AuthzGroup we give a better error message about what was being attempted.

This also handles the case where the user who is being added isn’t a member of the site any more.

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

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

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:

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