SAK-44969 joinable sets (groups) should be using saveGroupMembership …

…not SiteService.save() which rebuilds the entire site (sakaiproject#8997)
Chitova263 · Feb 3, 2021 · ef2fadd · ef2fadd
1 parent c2abb6e
commit ef2fadd
Showing 1 changed file with 49 additions and 31 deletions.
diff --git a/site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java b/site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java
@@ -77,6 +77,7 @@
 import org.sakaiproject.authz.api.AuthzGroup;
 import org.sakaiproject.authz.api.AuthzGroupService;
 import org.sakaiproject.authz.api.AuthzPermissionException;
+import org.sakaiproject.authz.api.AuthzRealmLockException;
 import org.sakaiproject.authz.api.GroupNotDefinedException;
 import org.sakaiproject.authz.api.Member;
 import org.sakaiproject.authz.api.PermissionsHelper;
@@ -209,7 +210,10 @@ public class SiteAction extends PagedResourceActionII {
 	private static ResourceLoader cfgRb = new ResourceLoader("multipletools");
 
 	private Locale comparator_locale = rb.getLocale();	
-
+
+	private org.sakaiproject.authz.api.SecurityService securityService = (org.sakaiproject.authz.api.SecurityService) ComponentManager.get(
+			org.sakaiproject.authz.api.SecurityService.class);
+
 	private org.sakaiproject.user.api.UserDirectoryService userDirectoryService = (org.sakaiproject.user.api.UserDirectoryService) ComponentManager.get(
 			org.sakaiproject.user.api.UserDirectoryService.class );
 
@@ -8809,32 +8813,40 @@ public void doJoinableSet(RunData data){
 								// add current user as the maintainer
 								Member member = currentSite.getMember(userId);
 								if(member != null){
-									try{
-										siteGroup.insertMember(userId, member.getRole().getId(), true, false);
-										SecurityAdvisor yesMan = new SecurityAdvisor() {
-											public SecurityAdvice isAllowed(String userId, String function, String reference) {
+									SecurityAdvisor yesMan = new SecurityAdvisor() {
+										public SecurityAdvice isAllowed(String userId, String function, String reference) {
+											if (StringUtils.equalsIgnoreCase(function, SiteService.SECURE_UPDATE_SITE)) {
 												return SecurityAdvice.ALLOWED;
+											} else {
+												return SecurityAdvice.PASS;
 											}
-										};
-										SecurityService.pushAdvisor(yesMan);
-										commitSite(currentSite);
-									} catch (IllegalStateException e) {
+										}
+									};
+
+									try{
+										siteGroup.insertMember(userId, member.getRole().getId(), true, false);
+
+										securityService.pushAdvisor(yesMan);
+										SiteService.saveGroupMembership(currentSite);
+									} catch (AuthzRealmLockException e) {
 										log.error(".doJoinableSet: User with id {} cannot be inserted in group with id {} because the group is locked", userId, siteGroup.getId());
-									} catch (Exception e) {
-										log.debug(e.getMessage());
-									}finally{
-										SecurityService.popAdvisor();
+									} catch (IdUnusedException e) {
+										log.error("IdUnusedException while joining site, userId={}, siteId={}, groupId={}", userId, currentSite.getId(), siteGroup.getId());
+									} catch (PermissionException e) {
+										log.error("doJoinableSet could not save new membership because of permissions", e);
+									} finally {
+										securityService.popAdvisor(yesMan);
 									}
 								}
 							}
 						}	
-					}catch (Exception e) {
-						log.debug("Error adding user to group: " + groupRef + ", " + e.getMessage(), e);
+					} catch (GroupNotDefinedException e) {
+						log.error("Error adding user to group because group does not exist: {}", groupRef, e);
 					}
 				}
 			}
 		} catch (IdUnusedException e) {
-			log.debug("Error adding user to group: " + groupRef + ", " + e.getMessage(), e);
+			log.error("IdUnusedException while adding user to group: {}", groupRef, e);
 		}
 	}
 
@@ -8869,31 +8881,37 @@ public void doUnjoinableSet(RunData data){
 							// remove current user as the maintainer
 							Member member = currentSite.getMember(userId);
 							if(member != null){
-								try{
-									siteGroup.deleteMember(userId);
-									SecurityAdvisor yesMan = new SecurityAdvisor() {
-										public SecurityAdvice isAllowed(String userId, String function, String reference) {
+								SecurityAdvisor yesMan = new SecurityAdvisor() {
+									public SecurityAdvice isAllowed(String userId, String function, String reference) {
+										if (StringUtils.equalsIgnoreCase(function, SiteService.SECURE_UPDATE_SITE)) {
 											return SecurityAdvice.ALLOWED;
+										} else {
+											return SecurityAdvice.PASS;
 										}
-									};
-									SecurityService.pushAdvisor(yesMan);
-									commitSite(currentSite);
-								}catch (IllegalStateException e) {
+									}
+								};
+
+								try{
+									siteGroup.deleteMember(userId);
+
+									securityService.pushAdvisor(yesMan);
+									SiteService.saveGroupMembership(currentSite);
+								} catch (AuthzRealmLockException e) {
 									log.error(".doUnjoinableSet: User with id {} cannot be deleted from group with id {} because the group is locked", userId, siteGroup.getId());
-								}catch (Exception e) {
-									log.debug(e.getMessage());
-								}finally{
-									SecurityService.popAdvisor();
+								} catch (PermissionException e) {
+									log.error("doUnjoinableSet: permission exception as userId={}", userId, e);
+								} finally {
+									securityService.popAdvisor(yesMan);
 								}
 							}
 						}
-					}catch (Exception e) {
-						log.debug("Error removing user to group: {}, {}", groupRef, e.getMessage(), e);
+					} catch (GroupNotDefinedException e) {
+						log.error("Error removing user from group: {}", groupRef, e);
 					}
 				}
 			}
 		} catch (IdUnusedException e) {
-			log.debug("Error removing user to group: {}, {}", groupRef, e.getMessage(), e);
+			log.error("IdUnusedException while removing user to group: {}", groupRef, e);
 		}
 	}