Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EVALSYS-1604 add RoleView check to isUserAllowedInEvalGroup #163

Merged
merged 2 commits into from
Dec 12, 2024
Merged

EVALSYS-1604 add RoleView check to isUserAllowedInEvalGroup #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ba6978
EVALSYS-1604 add RoleView check to isUserAllowedInEvalGroup
ottenhoff Dec 10, 2024
6465419
More updates
ottenhoff Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions sakai-evaluation-impl/src/java/org/sakaiproject/evaluation/dao/EvaluationDaoImpl.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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