Skip to content

Commit

Permalink
SAK-41172 Group Locking Enhancements (#6536)
Browse files Browse the repository at this point in the history 
* SAK-41172 Group Locking Enhancements

* Fixup spacing
  • Loading branch information
@ern
ern committed Nov 4, 2019
1 parent c17058a commit 0b9d28b
Show file tree
Hide file tree
Showing 43 changed files with 929 additions and 548 deletions.
5 changes: 5 additions & 0 deletions admin-tools/src/java/org/sakaiproject/authz/tool/PermissionsHelperAction.java
View file
Expand Up @@ -40,6 +40,7 @@

import org.sakaiproject.authz.api.AuthzGroup;
import org.sakaiproject.authz.api.AuthzPermissionException;
import org.sakaiproject.authz.api.AuthzRealmLockException;
import org.sakaiproject.authz.api.GroupAlreadyDefinedException;
import org.sakaiproject.authz.api.GroupIdInvalidException;
import org.sakaiproject.authz.api.GroupNotDefinedException;
Expand Down Expand Up @@ -626,6 +627,10 @@ public void doSave(RunData data)
{
addAlert(state, rb.getFormattedMessage("alert_permission", new Object[]{edit.getReference()}));
}
catch (AuthzRealmLockException arle)
{
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}
}

// clean up state
Expand Down
9 changes: 9 additions & 0 deletions admin-tools/src/java/org/sakaiproject/authz/tool/RealmsAction.java
View file
Expand Up @@ -35,6 +35,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.FunctionManager;
import org.sakaiproject.authz.api.GroupAlreadyDefinedException;
import org.sakaiproject.authz.api.GroupIdInvalidException;
Expand Down Expand Up @@ -792,6 +793,10 @@ public void doCancel(RunData data, Context context)
{
addAlert(state, rb.getFormattedMessage("realm.notpermis2", new Object[]{realm.getId()}));
}
catch (AuthzRealmLockException arle)
{
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}
}
}

Expand Down Expand Up @@ -848,6 +853,10 @@ public void doRemove_confirmed(RunData data, Context context)
{
addAlert(state, rb.getFormattedMessage("realm.notpermis2", new Object[]{realm.getId()}));
}
catch (AuthzRealmLockException arle)
{
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}

// cleanup
cleanState(state);
Expand Down
9 changes: 5 additions & 4 deletions admin-tools/src/java/org/sakaiproject/site/tool/AdminSitesAction.java
View file
Expand Up @@ -38,6 +38,7 @@
import org.sakaiproject.alias.api.AliasService;
import org.sakaiproject.authz.api.AuthzGroup;
import org.sakaiproject.authz.api.AuthzGroupService;
import org.sakaiproject.authz.api.AuthzRealmLockException;
import org.sakaiproject.authz.api.GroupNotDefinedException;
import org.sakaiproject.authz.api.Role;
import org.sakaiproject.authz.api.SecurityService;
Expand Down Expand Up @@ -1865,8 +1866,8 @@ public void doCancel_group(RunData data, Context context)
{
try {
site.deleteGroup(group);
} catch (IllegalStateException e) {
log.error(".doCancel_group: Group with id {} cannot be removed because is locked", group.getId());
} catch (AuthzRealmLockException arle) {
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}
}

Expand All @@ -1893,8 +1894,8 @@ public void doRemove_group(RunData data, Context context)
// remove the page (no confirm)
try {
site.deleteGroup(group);
} catch (IllegalStateException e) {
log.error(".doRemove_group: Group with id {} cannot be removed because is locked", group.getId());
} catch (AuthzRealmLockException arle) {
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}

// done with the page
Expand Down
5 changes: 5 additions & 0 deletions assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java
View file
Expand Up @@ -95,6 +95,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.FunctionManager;
import org.sakaiproject.authz.api.GroupNotDefinedException;
import org.sakaiproject.authz.api.Member;
Expand Down Expand Up @@ -908,6 +909,8 @@ public void deleteAssignment(Assignment assignment) throws PermissionException {
log.debug("successful delete for assignment with id = {}", assignment.getId());
} catch (AuthzPermissionException e) {
log.warn("deleting realm for assignment reference = {}", reference, e);
} catch (AuthzRealmLockException arle) {
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}
}

Expand Down Expand Up @@ -1161,6 +1164,8 @@ public void removeSubmission(AssignmentSubmission submission) throws PermissionE
log.warn("removing realm for : {} : {}", reference, e.getMessage());
} catch (GroupNotDefinedException e) {
log.warn("cannot find group for submission : {} : {}", reference, e.getMessage());
} catch (AuthzRealmLockException arle) {
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}
}
}
Expand Down
9 changes: 5 additions & 4 deletions assignment/tool/src/java/org/sakaiproject/assignment/tool/AssignmentAction.java
View file
Expand Up @@ -67,6 +67,7 @@
import org.sakaiproject.assignment.taggable.tool.DecoratedTaggingProvider.Pager;
import org.sakaiproject.assignment.taggable.tool.DecoratedTaggingProvider.Sort;
import org.sakaiproject.authz.api.*;
import org.sakaiproject.authz.api.AuthzGroup.RealmLockMode;
import org.sakaiproject.calendar.api.Calendar;
import org.sakaiproject.calendar.api.CalendarEvent;
import org.sakaiproject.calendar.api.CalendarEventEdit;
Expand Down Expand Up @@ -1867,7 +1868,7 @@ private String build_student_view_submission_confirmation_context(VelocityPortle

if (currentAssignment.getIsGroup()) {
context.put("submitterNames", getSubmitterFormattedNames(s, "build_student_view_submission_confirmation_context"));

}
}
}
Expand Down Expand Up @@ -7936,7 +7937,7 @@ private void post_save_assignment(RunData data, String postOrSave) {
rubricsService.saveRubricAssociation(RubricsConstants.RBCS_TOOL_ASSIGNMENT, a.getId(), getRubricConfigurationParameters(params));

// Locking and unlocking groups
rangeAndGroups.postSaveAssignmentGroupLocking(state, post, rangeAndGroupSettings, aOldGroups, siteId, a.getId());
rangeAndGroups.postSaveAssignmentGroupLocking(state, post, rangeAndGroupSettings, aOldGroups, siteId, assignmentReference);

if (post) {
// we need to update the submission
Expand Down Expand Up @@ -9701,7 +9702,7 @@ public void doDelete_assignment(RunData data) {
for (String reference : groups) {
Group group = site.getGroup(reference);
if (group != null) {
group.unlockGroup(group.getReference() + "/assignment/" + assignment.getId(), Group.LockMode.ALL);
group.setLockForReference(ref, RealmLockMode.NONE);
siteService.save(group.getContainingSite());
}
}
Expand Down Expand Up @@ -10500,7 +10501,7 @@ public void doAttachments(RunData data) {
} else if (MODE_STUDENT_REVIEW_EDIT.equals(mode)) {
saveReviewGradeForm(data, state, "save");
}

//Set the title and override for anonymous assignment
if (assignment != null) {
assignmentTitle = assignment.getTitle();
Expand Down
63 changes: 32 additions & 31 deletions assignment/tool/src/java/org/sakaiproject/assignment/tool/RangeAndGroupsDelegate.java
View file
Expand Up @@ -15,6 +15,7 @@
import org.sakaiproject.assignment.api.AssignmentService;
import org.sakaiproject.assignment.api.MultiGroupRecord;
import org.sakaiproject.assignment.api.model.Assignment;
import org.sakaiproject.authz.api.AuthzGroup;
import org.sakaiproject.authz.api.SecurityService;
import org.sakaiproject.cheftool.Context;
import org.sakaiproject.cheftool.RunData;
Expand Down Expand Up @@ -47,11 +48,11 @@ class RangeAndGroupsDelegate
private static final String VALUE_ASSIGN_TO_INDIVIDUALS_FROM_GROUPS = "individualsFromGroups";
private static final String VALUE_ASSIGN_TO_GROUPS = "groups";

private final AssignmentService asnServ;
private final AssignmentService assignmentService;
private final ResourceLoader rb;
private final SiteService siteServ;
private final SecurityService secServ;
private final FormattedText fmtTxt;
private final SiteService siteService;
private final SecurityService securityService;
private final FormattedText formattedText;

void buildInstructorNewEditAssignmentContextGroupCheck(Context context, Assignment asn)
{
Expand All @@ -63,17 +64,17 @@ void buildInstructorNewEditAssignmentContextGroupCheck(Context context, Assignme
List<MultiGroupRecord> dupes = defaultMultipleGroupCheck(asn);
if (!dupes.isEmpty())
{
context.put("multipleGroupUsers", fmtTxt.escapeHtml(formatDuplicateMemberships(dupes)));
context.put("multipleGroupUsers", formattedText.escapeHtml(formatDuplicateMemberships(dupes)));
}
}

void setAssignmentFormContext(SessionState state, Context context, String contextString, AssignmentAction asnAct)
{
String range = StringUtils.trimToNull((String) state.getAttribute(NEW_ASSIGNMENT_RANGE));
Collection<Group> groupsAllowAddAssignment = asnServ.getGroupsAllowAddAssignment(contextString);
Collection<Group> groupsAllowAddAssignment = assignmentService.getGroupsAllowAddAssignment(contextString);
if (range == null)
{
if (asnServ.allowAddSiteAssignment(contextString))
if (assignmentService.allowAddSiteAssignment(contextString))
{
range = Assignment.Access.SITE.toString();
}
Expand Down Expand Up @@ -145,7 +146,7 @@ else if (VALUE_ASSIGN_TO_INDIVIDUALS_FROM_GROUPS.equals(assignTo))
if (groupAssignment)
{
List<String> groupIds = Arrays.asList(data.getParameters().getStrings("selectedGroups"));
List<MultiGroupRecord> dupes = asnServ.checkAssignmentForUsersInMultipleGroups(siteId, groupsFromIds(siteId, groupIds));
List<MultiGroupRecord> dupes = assignmentService.checkAssignmentForUsersInMultipleGroups(siteId, groupsFromIds(siteId, groupIds));
alertDuplicateMemberships(dupes, state);
}

Expand Down Expand Up @@ -193,37 +194,38 @@ else if (groupChoice != null)
return new RangeAndGroupSettings(isGroupSubmit, range, groups);
}

void postSaveAssignmentGroupLocking(SessionState state, boolean post, RangeAndGroupSettings settings, Collection<String> aOldGroups, String siteId, String asnId)
void postSaveAssignmentGroupLocking(SessionState state, boolean post, RangeAndGroupSettings settings, Collection<String> aOldGroups, String siteId, String assignmentReference)
{
List<String> lockedGroupsReferences = new ArrayList<>();
if (post && settings.isGroupSubmit && !settings.groups.isEmpty())
{
for (Group group : settings.groups)
{
String groupAssignmentReference = group.getReference() + "/assignment/" + asnId;

log.debug("Getting groups from reference: {}", groupAssignmentReference);
// Prior to SAK-41172 the string concatenation:
// 'group.getReference() + "/assignment/" + a.getId()'
// was used to create the reference for a lock
// this was simplified to the assignment reference
lockedGroupsReferences.add(group.getReference());
log.debug("Adding group: {}", group.getReference());
log.debug("Adding group to lock list: {}", group.getReference());

if (!aOldGroups.contains(group.getReference()) || !group.isLocked(groupAssignmentReference, Group.LockMode.ALL))
if (!aOldGroups.contains(group.getReference()) || group.getLockForReference(assignmentReference) == AuthzGroup.RealmLockMode.NONE)
{
log.debug("locking group: {}", group.getReference());
group.lockGroup(groupAssignmentReference, Group.LockMode.ALL);
group.setLockForReference(assignmentReference, AuthzGroup.RealmLockMode.ALL);
log.debug("locked group: {}", group.getReference());

try
{
siteServ.save(group.getContainingSite());
siteService.save(group.getContainingSite());
}
catch (IdUnusedException e)
{
log.warn(".post_save_assignment: Cannot find site with id {}", siteId);
log.warn("Cannot find site with id {}", siteId);
VelocityPortletPaneledAction.addAlert(state, rb.getFormattedMessage("options_cannotFindSite", siteId));
}
catch (PermissionException e)
{
log.warn(".post_save_assignment: Do not have permission to edit site with id {}", siteId);
log.warn("Do not have permission to edit site with id {}", siteId);
VelocityPortletPaneledAction.addAlert(state, rb.getFormattedMessage("options_cannotEditSite", siteId));
}
}
Expand All @@ -234,7 +236,7 @@ void postSaveAssignmentGroupLocking(SessionState state, boolean post, RangeAndGr
{
try
{
Site site = siteServ.getSite(siteId);
Site site = siteService.getSite(siteId);

for (String reference : aOldGroups)
{
Expand All @@ -244,9 +246,8 @@ void postSaveAssignmentGroupLocking(SessionState state, boolean post, RangeAndGr
Group group = site.getGroup(reference);
if (group != null)
{
String groupReferenceAssignment = group.getReference() + "/assignment/" + asnId;
group.unlockGroup(groupReferenceAssignment, Group.LockMode.ALL);
siteServ.save(group.getContainingSite());
group.setLockForReference(assignmentReference, AuthzGroup.RealmLockMode.NONE);
siteService.save(group.getContainingSite());
}
}
}
Expand Down Expand Up @@ -289,7 +290,7 @@ void initializeAssignment(SessionState state)
*/
List<MultiGroupRecord> defaultMultipleGroupCheck(Assignment asn)
{
return asnServ.checkAssignmentForUsersInMultipleGroups(asn.getContext(), groupsFromRefs(asn.getContext(), asn.getGroups()));
return assignmentService.checkAssignmentForUsersInMultipleGroups(asn.getContext(), groupsFromRefs(asn.getContext(), asn.getGroups()));
}

/**
Expand Down Expand Up @@ -318,13 +319,13 @@ Collection<MultiGroupRecord> checkAssignmentForUsersInMultipleGroups(Assignment
*/
Collection<MultiGroupRecord> checkSubmissionForUsersInMultipleGroups(Assignment asn, Group submissionGroup, SessionState state, boolean showAlert)
{
if (submissionGroup == null || secServ.isSuperUser()) // don't check this for admin users / short circuit if no group given
if (submissionGroup == null || securityService.isSuperUser()) // don't check this for admin users / short circuit if no group given
{
return Collections.emptyList();
}

String siteId = asn.getContext();
List<MultiGroupRecord> dupes = asnServ.checkSubmissionForUsersInMultipleGroups(siteId, submissionGroup, groupsFromRefs(siteId, asn.getGroups()));
List<MultiGroupRecord> dupes = assignmentService.checkSubmissionForUsersInMultipleGroups(siteId, submissionGroup, groupsFromRefs(siteId, asn.getGroups()));
if (showAlert)
{
alertDuplicateNames(dupes, state);
Expand Down Expand Up @@ -367,7 +368,7 @@ void buildStudentViewAssignmentContext(SessionState state, String userId, Assign

void buildInstructorGradeAssignmentContext(SessionState state, Context context, Assignment asn)
{
Collection<String> dupes = checkAssignmentForUsersInMultipleGroups(asn, state).stream().map(mgr -> fmtTxt.escapeHtml(mgr.user.getDisplayName())).collect(Collectors.toList());
Collection<String> dupes = checkAssignmentForUsersInMultipleGroups(asn, state).stream().map(mgr -> formattedText.escapeHtml(mgr.user.getDisplayName())).collect(Collectors.toList());
if (!dupes.isEmpty())
{
context.put("usersinmultiplegroups", dupes);
Expand Down Expand Up @@ -395,7 +396,7 @@ boolean validateUserGroups(SessionState state, String userId, Assignment asn)
}

// user is not in any assignment groups, if they are an instructor this is probably the Student View feature so let them through
return asnServ.allowAddAssignment(site.getId()) || asnServ.allowUpdateAssignmentInContext(site.getId());
return assignmentService.allowAddAssignment(site.getId()) || assignmentService.allowUpdateAssignmentInContext(site.getId());
}

void buildInstructorGradeSubmissionContextGroupCheck(Optional<Assignment> asnOpt, String groupId, SessionState state)
Expand All @@ -419,7 +420,7 @@ private Site getSite(Assignment asn)
{
try
{
return siteServ.getSite(asn.getContext());
return siteService.getSite(asn.getContext());
}
catch (IdUnusedException e)
{
Expand All @@ -433,7 +434,7 @@ private void alertDuplicates(String msg, SessionState state)
if (!msg.isEmpty())
{
String baseMessage = rb.getString("group.user.multiple.warning");
VelocityPortletPaneledAction.addAlert(state, fmtTxt.escapeHtml(baseMessage + " " + msg));
VelocityPortletPaneledAction.addAlert(state, formattedText.escapeHtml(baseMessage + " " + msg));
}
}

Expand Down Expand Up @@ -478,7 +479,7 @@ private List<Group> groupsFromIdsOrRefs(String siteId, Collection<String> groups
{
try
{
return siteServ.getSite(siteId).getGroups().stream().filter(g -> groups.contains(accessor.apply(g))).collect(Collectors.toList());
return siteService.getSite(siteId).getGroups().stream().filter(g -> groups.contains(accessor.apply(g))).collect(Collectors.toList());
}
catch (IdUnusedException e)
{
Expand All @@ -497,7 +498,7 @@ private List<Group> groupsFromIdsOrRefs(String siteId, Collection<String> groups
*/
private List<Group> getGroupsWithUser(String userId, Assignment asn, Site site)
{
boolean isAdmin = secServ.isSuperUser();
boolean isAdmin = securityService.isSuperUser();
return asn.getGroups().stream().map(gref -> site.getGroup(gref)).filter(Objects::nonNull)
.filter(g -> g.getMember(userId) != null || isAdmin) // allow admin to submit on behalf of groups
.collect(Collectors.toList());
Expand Down
14 changes: 12 additions & 2 deletions calendar/calendar-impl/impl/src/java/org/sakaiproject/calendar/impl/BaseCalendarService.java
View file
Expand Up @@ -888,8 +888,13 @@ public void removeCalendar(CalendarEdit calendar) throws PermissionException
{
log.warn("removeCalendar: removing realm for : " + calendar.getReference() + " : " + e);
}
catch (GroupNotDefinedException ignore)
catch (GroupNotDefinedException gnde)
{
log.debug(gnde.getMessage());
}
catch (AuthzRealmLockException arle)
{
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}

} // removeCalendar
Expand Down Expand Up @@ -2917,8 +2922,13 @@ public void removeEvent(CalendarEventEdit edit, int intention) throws Permission
{
log.warn("removeEvent: removing realm for : " + edit.getReference() + " : " + e);
}
catch (GroupNotDefinedException ignore)
catch (GroupNotDefinedException gnde)
{
log.debug(gnde.getMessage());
}
catch (AuthzRealmLockException arle)
{
log.warn("GROUP LOCK REGRESSION: {}", arle.getMessage(), arle);
}

} // removeEvent
Expand Down

0 comments on commit 0b9d28b

Please sign in to comment.