Skip to content

Commit

Permalink
EVALSYS-1604 add RoleView check to isUserAllowedInEvalGroup (#163)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ottenhoff
ottenhoff authored Dec 12, 2024
1 parent f43a1e8 commit c242237
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 132 deletions.
8 changes: 2 additions & 6 deletions sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/dao/EvaluationDaoImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,7 @@ public List<EvalAssignUser> getParticipantsForEval(Long evaluationId, String use
@SuppressWarnings("unchecked")
public List<EvalEvaluation> getEvalsWithoutUserAssignments() {
String hql = "select eval from EvalAssignUser eau right join eau.evaluation eval where eau.id is null";
List<EvalEvaluation> evals = (List<EvalEvaluation>) executeHqlQuery(hql, new Object[] {}, 0, 0);
return evals;
return (List<EvalEvaluation>) executeHqlQuery(hql, new Object[] {}, 0, 0);
}

/**
Expand Down Expand Up @@ -1252,10 +1251,7 @@ public Set<String> getResponseUserIds(Long evaluationId, String[] evalGroupIds,
for (Object object : results) {
responseUsers.add((String) object);
}
if (log.isDebugEnabled()) {
log.debug("ResponseUserIds(eval:"+evaluationId+", groups:"
+ArrayUtils.arrayToString(evalGroupIds)+", completed="+completed+"): users="+responseUsers);
}
log.debug("ResponseUserIds(eval:{}, groups:{}, completed={}): users={}", evaluationId, ArrayUtils.arrayToString(evalGroupIds), completed, responseUsers);
return responseUsers;
}

Expand Down
8 changes: 3 additions & 5 deletions sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalCommonLogicImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ else if( EvalConstants.PERM_TAKE_EVALUATION.equals( permission ) )
|| EvalConstants.PERM_TAKE_EVALUATION.equals(permission)
|| EvalConstants.PERM_ASSISTANT_ROLE.equals( permission ) )
{
log.debug( "Using eval groups provider: evalGroupId: " + evalGroupID + ", permission: " + permission );
log.debug("Using eval groups provider: evalGroupId: {}, permission: {}", evalGroupID, permission);
userIDs.addAll( evalGroupsProvider.getUserIdsForEvalGroups( new String[] { evalGroupID },
EvalExternalLogicImpl.translatePermission(permission)) );
}
Expand Down Expand Up @@ -620,10 +620,8 @@ public boolean isUserAllowedInEvalGroup(String userId, String permission, String
if (EvalConstants.PERM_BE_EVALUATED.equals(permission)
|| EvalConstants.PERM_TAKE_EVALUATION.equals(permission)
|| EvalConstants.PERM_ASSISTANT_ROLE.equals(permission) ) {
log.debug("Using eval groups provider: userId: " + userId + ", permission: " + permission + ", evalGroupId: " + evalGroupId);
if ( evalGroupsProvider.isUserAllowedInGroup(userId, EvalExternalLogicImpl.translatePermission(permission), evalGroupId) ) {
return true;
}
log.debug("Using eval groups provider: userId: {}, permission: {}, evalGroupId: {}", userId, permission, evalGroupId);
return evalGroupsProvider.isUserAllowedInGroup(userId, EvalExternalLogicImpl.translatePermission(permission), evalGroupId);
}
}

Expand Down
106 changes: 11 additions & 95 deletions ...evaluation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,88 +327,8 @@ public List<EvalAssignUser> getParticipantsForEval(Long evaluationId, String use
if (evaluationId == null && (userId == null || "".equals(userId)) ) {
throw new IllegalArgumentException("At least one of the following must be set: evaluationId, userId");
}
/**
// create the search
Search search = new Search();
if (evaluationId != null) {
// getEvaluationOrFail(evaluationId); // took out eval fetch for now
search.addRestriction( new Restriction("evaluation.id", evaluationId) );
}
if (evalStateConstant != null) {
EvalUtils.validateStateConstant(evalStateConstant);
search.addRestriction( new Restriction("evaluation.state", evalStateConstant) );
}
if (evalGroupIds != null && evalGroupIds.length > 0) {
search.addRestriction( new Restriction("evalGroupId", evalGroupIds) );
}
if (assignTypeConstant != null
&& includeConstant == null) {
EvalAssignUser.validateType(assignTypeConstant);
// only set this if the includeConstant is not set
search.addRestriction( new Restriction("type", assignTypeConstant) );
}
if (assignStatusConstant == null) {
search.addRestriction( new Restriction("status", EvalAssignUser.STATUS_REMOVED, Restriction.NOT_EQUALS) );
} else if (STATUS_ANY.equals(assignStatusConstant)) {
// no restriction needed in this case
} else {
EvalAssignUser.validateStatus(assignStatusConstant);
search.addRestriction( new Restriction("status", assignStatusConstant) );
}
if (userId != null && ! "".equals(userId)) {
search.addRestriction( new Restriction("userId", userId) );
}
boolean includeFilterUsers = false;
Set<String> userFilter = null;
if (includeConstant != null) {
EvalUtils.validateEmailIncludeConstant(includeConstant);
String[] groupIds = new String[] {};
if (evalGroupIds != null && evalGroupIds.length > 0) {
groupIds = evalGroupIds;
}
// force the results to only include eval takers
search.addRestriction( new Restriction("type", EvalAssignUser.TYPE_EVALUATOR) );
// now set up the filter
if (EvalConstants.EVAL_INCLUDE_NONTAKERS.equals(includeConstant)) {
// get all users who have NOT responded
userFilter = dao.getResponseUserIds(evaluationId, groupIds);
includeFilterUsers = false;
} else if (EvalConstants.EVAL_INCLUDE_RESPONDENTS.equals(includeConstant)) {
// get all users who have responded
userFilter = dao.getResponseUserIds(evaluationId, groupIds);
includeFilterUsers = true;
} else if (EvalConstants.EVAL_INCLUDE_ALL.equals(includeConstant)) {
// do nothing
} else {
throw new IllegalArgumentException("Unknown includeConstant: " + includeConstant);
}
}
// get the assignments based on the search
List<EvalAssignUser> results = dao.findBySearch(EvalAssignUser.class, search);
List<EvalAssignUser> assignments = new ArrayList<EvalAssignUser>( results );
// This code is potentially expensive but there is not really a better way to handle it -AZ
if (userFilter != null && ! userFilter.isEmpty()) {
// filter the results based on the userFilter
for (Iterator<EvalAssignUser> iterator = assignments.iterator(); iterator.hasNext();) {
EvalAssignUser evalAssignUser = iterator.next();
String uid = evalAssignUser.getUserId();
if (includeFilterUsers) {
// only include users in the filter
if (! userFilter.contains(uid)) {
iterator.remove();
}
} else {
// exclude all users in the filter
if (userFilter.contains(uid)) {
iterator.remove();
}
}
}
}
**/
// this is handled in the DAO now
List<EvalAssignUser> assignments = dao.getParticipantsForEval(evaluationId, userId, evalGroupIds, assignTypeConstant, assignStatusConstant, includeConstant, evalStateConstant);
return assignments;
return dao.getParticipantsForEval(evaluationId, userId, evalGroupIds, assignTypeConstant, assignStatusConstant, includeConstant, evalStateConstant);
}

public int countParticipantsForEval(Long evaluationId, String[] evalGroupIds) {
Expand Down Expand Up @@ -701,17 +621,15 @@ public EvalAssignGroup getAssignGroupById(Long assignGroupId) {
}

public EvalAssignGroup getAssignGroupByEvalAndGroupId(Long evaluationId, String evalGroupId) {
log.debug("evaluationId: " + evaluationId + ", evalGroupId: " + evalGroupId);
if (evaluationId == null
|| evalGroupId == null || "".equals(evalGroupId)) {
log.debug("evaluationId: {}, evalGroupId: {}", evaluationId, evalGroupId);
if (evaluationId == null || evalGroupId == null || evalGroupId.isEmpty()) {
throw new IllegalArgumentException("evaluationId and evalGroupId must not be null");
}
EvalAssignGroup assignGroup = dao.findOneBySearch(EvalAssignGroup.class, new Search(
return dao.findOneBySearch(EvalAssignGroup.class, new Search(
new Restriction[] {
new Restriction("evaluation.id", evaluationId),
new Restriction("evalGroupId", evalGroupId)
}) );
return assignGroup;
}

public List<EvalAssignHierarchy> getAssignHierarchyByEval(Long evaluationId) {
Expand All @@ -732,7 +650,7 @@ public EvalAssignHierarchy getAssignHierarchyById(Long assignHierarchyId) {

public Map<Long, List<EvalAssignGroup>> getAssignGroupsForEvals(Long[] evaluationIds,
boolean includeUnApproved, Boolean includeHierarchyGroups) {
log.debug("evalIds: " + ArrayUtils.arrayToString(evaluationIds) + ", includeUnApproved=" + includeUnApproved);
log.debug("evalIds: {}, includeUnApproved={}", ArrayUtils.arrayToString(evaluationIds), includeUnApproved);
Map<Long, List<EvalAssignGroup>> evals = new TreeMap<>();

if ( evaluationIds != null && evaluationIds.length > 0){
Expand Down Expand Up @@ -765,14 +683,12 @@ public Map<Long, List<EvalAssignGroup>> getAssignGroupsForEvals(Long[] evaluatio
search.addOrder( new Order("evalGroupId") );
List<EvalAssignGroup> l = dao.findBySearch(EvalAssignGroup.class, search );

for (int i=0; i<l.size(); i++) {
EvalAssignGroup eac = l.get(i);

// put stuff in inner list
Long evalId = eac.getEvaluation().getId();
List<EvalAssignGroup> innerList = evals.get(evalId);
innerList.add( eac );
}
for (EvalAssignGroup eac : l) {
// put stuff in inner list
Long evalId = eac.getEvaluation().getId();
List<EvalAssignGroup> innerList = evals.get(evalId);
innerList.add(eac);
}
}
return evals;
}
Expand Down
24 changes: 7 additions & 17 deletions ...ation-impl/src/java/org/sakaiproject/evaluation/logic/EvalEvaluationSetupServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1023,8 +1023,7 @@ public List<Long> synchronizeUserAssignments(Long evaluationId, String evalGroup
* @return the list of {@link EvalAssignUser} ids changed during the synchronization (created, updated, deleted),
* NOTE: deleted {@link EvalAssignUser} will not be able to be retrieved
*/
public List<Long> synchronizeUserAssignmentsForced(EvalEvaluation evaluation,
String evalGroupId, boolean removeAllowed) {
public List<Long> synchronizeUserAssignmentsForced(EvalEvaluation evaluation, String evalGroupId, boolean removeAllowed) {
Long evaluationId = evaluation.getId();
String currentUserId = commonLogic.getCurrentUserId();
if (currentUserId == null) {
Expand Down Expand Up @@ -1099,11 +1098,7 @@ public List<Long> synchronizeUserAssignmentsForced(EvalEvaluation evaluation,
currentTakers.addAll(currentAssistants);
currentTakers.addAll(currentEvaluated);
}

HashSet<String> currentAll = new HashSet<>();
currentAll.addAll(currentEvaluated);
currentAll.addAll(currentAssistants);
currentAll.addAll(currentTakers);

/* Resolve the current permissions against the existing assignments,
* this should only change linked records but should respect unlinked and removed records by not
* adding a record where one already exists for the given user/group combo,
Expand Down Expand Up @@ -1162,12 +1157,9 @@ public List<Long> synchronizeUserAssignmentsForced(EvalEvaluation evaluation,
if (assignUserToRemove.isEmpty() && assignUserToSave.isEmpty()) {
message += ": no changes to the user assignments ("+assignedUsers.size()+")";
} else {
if (removeAllowed
&& ! assignUserToRemove.isEmpty()) {
Long[] assignUserToRemoveArray = assignUserToRemove.toArray(new Long[assignUserToRemove.size()]);
if (log.isDebugEnabled()) {
log.debug("Deleting user eval assignment Ids: "+assignUserToRemove);
}
if (removeAllowed && ! assignUserToRemove.isEmpty()) {
Long[] assignUserToRemoveArray = assignUserToRemove.toArray(new Long[0]);
log.debug("Deleting user eval assignment Ids: {}", assignUserToRemove);
dao.deleteSet(EvalAssignUser.class, assignUserToRemoveArray);
message += ": removed the following assignments: " + assignUserToRemove;
changedUserAssignments.addAll( assignUserToRemove );
Expand All @@ -1178,9 +1170,7 @@ public List<Long> synchronizeUserAssignmentsForced(EvalEvaluation evaluation,
}
// this is meant to force the assigned users set to be re-calculated
assignUserToSave = new HashSet<>(assignUserToSave);
if (log.isDebugEnabled()) {
log.debug("Saving user eval assignments: "+assignUserToSave);
}
log.debug("Saving user eval assignments: {}", assignUserToSave);
dao.saveSet(assignUserToSave);
message += ": created the following assignments: " + assignUserToSave;
for (EvalAssignUser evalAssignUser : assignUserToSave) {
Expand All @@ -1205,7 +1195,7 @@ public List<Long> synchronizeUserAssignmentsForced(EvalEvaluation evaluation,
}
}
if (! orphanedUserAssignments.isEmpty()) {
Long[] orphanedUserAssignmentsArray = orphanedUserAssignments.toArray(new Long[orphanedUserAssignments.size()]);
Long[] orphanedUserAssignmentsArray = orphanedUserAssignments.toArray(new Long[0]);
dao.deleteSet(EvalAssignUser.class, orphanedUserAssignmentsArray);
message += ": removed the following orphaned user assignments: " + orphanedUserAssignments;
changedUserAssignments.addAll( orphanedUserAssignments );
Expand Down
31 changes: 23 additions & 8 deletions ...tion-impl/src/java/org/sakaiproject/evaluation/logic/externals/EvalExternalLogicImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -851,10 +851,7 @@ public Set<String> getUserIdsForEvalGroup( String evalGroupID, String permission
}
azGroups.add( azGroup );
userIDs.addAll( authzGroupService.getUsersIsAllowed( permission, azGroups ) );
if( userIDs.contains( ADMIN_USER_ID ) )
{
userIDs.remove( ADMIN_USER_ID );
}
userIDs.remove( ADMIN_USER_ID );
}

// Otherwise, it's section aware but we only need to run the following if the sectin prefix is present in the evalGroupID
Expand Down Expand Up @@ -900,10 +897,24 @@ else if( groupID.hasSection() )
catch( IdNotFoundException ex ) { log.warn( "Could not find section with ID = " + groupID.getSectionID(), ex ); }
}

// Return the user IDs
// Clean the userIDs of RoleViewType users first
userIDs.removeAll( getRoleViewTypeUserIds(userIDs) );
return userIDs;
}

/**
* Need to help remove the fake users from Sakai 23+ View Site As
*/
protected Set<String> getRoleViewTypeUserIds(Set<String> userIDs) {
Set<String> roleViewTypeUserIds = new HashSet<>();
for (String userId : userIDs) {
if (userDirectoryService.isRoleViewType(userId)) {
roleViewTypeUserIds.add(userId);
}
}
return roleViewTypeUserIds;
}

/* (non-Javadoc)
* @see org.sakaiproject.evaluation.logic.EvalExternalLogic#isUserAllowedInEvalGroup(java.lang.String, java.lang.String, java.lang.String)
*/
Expand All @@ -917,9 +928,13 @@ public boolean isUserAllowedInEvalGroup(String userId, String permission, String
return isUserSakaiAdmin(userId);
}

// try checking Sakai
String reference = evalGroupId;
return securityService.unlock(userId, permission, reference);
// Special RoleView user from View Site As
if (userDirectoryService.isRoleViewType(userId)) {
return false;
}

// try checking Sakai authz permissions last
return securityService.unlock(userId, permission, evalGroupId);
}

/* (non-Javadoc)
Expand Down
2 changes: 1 addition & 1 deletion .../src/java/org/sakaiproject/evaluation/tool/producers/EvaluationAssignConfirmProducer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ private int getEnrollmentCount(HashMap<String, List<EvalAssignUser>> groupIdToEA
if (groupIdToEAUList.get(evalGroupId) == null) {
//enrollmentCount = 0;
// Since no users have been added to the eval YET, get the number of members that WILL be added
// this number is more intuative for the instructor than 0.
// this number is more intuitive for the instructor than 0.
enrollmentCount = commonLogic.countUserIdsForEvalGroup(evalGroupId, EvalConstants.PERM_TAKE_EVALUATION, evaluation.getSectionAwareness());
} else {
enrollmentCount = groupIdToEAUList.get(evalGroupId).size();
Expand Down

0 comments on commit c242237

Please sign in to comment.