Skip to content

Commit

Permalink
MDL-67300 Calendar: Inconsistent behaviour of managegroupentries
Browse files Browse the repository at this point in the history 
When calling the calendar_get_allowed_event_types function with
no course id, it is supposed to return true if you have the
relevant permissions in any course.

For users who have the managegroupentries permission, this was not
the case - even though it works correctly if you call the function
with a supplied course id.

This change makes behaviour with and without a supplied course id
consistent.
  • Loading branch information
@sammarshallou
sammarshallou committed Nov 21, 2019
1 parent d547735 commit 584500d
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 33 deletions.
75 changes: 42 additions & 33 deletions calendar/lib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3722,23 +3722,8 @@ function calendar_get_allowed_event_types(int $courseid = null) {

if (!empty($courseid) && $courseid != SITEID) {
$context = \context_course::instance($courseid);
$groups = groups_get_all_groups($courseid);

$types['user'] = has_capability('moodle/calendar:manageownentries', $context);

if (has_capability('moodle/calendar:manageentries', $context)) {
$types['course'] = true;

$types['group'] = (!empty($groups) && has_capability('moodle/site:accessallgroups', $context))
|| array_filter($groups, function($group) use ($USER) {
return groups_is_member($group->id);
});
} else if (has_capability('moodle/calendar:managegroupentries', $context)) {
$types['group'] = (!empty($groups) && has_capability('moodle/site:accessallgroups', $context))
|| array_filter($groups, function($group) use ($USER) {
return groups_is_member($group->id);
});
}
calendar_internal_update_course_and_group_permission($courseid, $context, $types);
}

if (has_capability('moodle/calendar:manageentries', \context_course::instance(SITEID))) {
Expand Down Expand Up @@ -3823,23 +3808,7 @@ function calendar_get_allowed_event_types(int $courseid = null) {
context_helper::preload_from_record($coursewithgroup);
$context = context_course::instance($coursewithgroup->id);

if (has_capability('moodle/calendar:manageentries', $context)) {
// The user has access to manage calendar entries for the whole course.
// This includes groups if they have the accessallgroups capability.
$types['course'] = true;
if (has_capability('moodle/site:accessallgroups', $context)) {
// The user also has access to all groups so they can add calendar entries to any group.
// The manageentries capability overrides the managegroupentries capability.
$types['group'] = true;
break;
}

if (empty($types['group']) && has_capability('moodle/calendar:managegroupentries', $context)) {
// The user has the managegroupentries capability.
// If they have access to _any_ group, then they can create calendar entries within that group.
$types['group'] = !empty(groups_get_all_groups($coursewithgroup->id, $USER->id));
}
}
calendar_internal_update_course_and_group_permission($coursewithgroup->id, $context, $types);

// Okay, course and group event types are allowed, no need to keep the loop iteration.
if ($types['course'] == true && $types['group'] == true) {
Expand Down Expand Up @@ -3873,3 +3842,43 @@ function calendar_get_allowed_event_types(int $courseid = null) {

return $types;
}

/**
* Given a course id, and context, updates the permission types array to add the 'course' or 'group'
* permission if it is relevant for that course.
*
* For efficiency, if they already have 'course' or 'group' then it skips checks.
*
* Do not call this function directly, it is only for use by calendar_get_allowed_event_types().
*
* @param int $courseid Course id
* @param context $context Context for that course
* @param array $types Current permissions
*/
function calendar_internal_update_course_and_group_permission(int $courseid, context $context, array &$types) {
if (!$types['course']) {
// If they have manageentries permission on the course, then they can update this course.
if (has_capability('moodle/calendar:manageentries', $context)) {
$types['course'] = true;
}
}
// To update group events they must have EITHER manageentries OR managegroupentries.
if (!$types['group'] && (has_capability('moodle/calendar:manageentries', $context) ||
has_capability('moodle/calendar:managegroupentries', $context))) {
// And they also need for a group to exist on the course.
$groups = groups_get_all_groups($courseid);
if (!empty($groups)) {
// And either accessallgroups, or belong to one of the groups.
if (has_capability('moodle/site:accessallgroups', $context)) {
$types['group'] = true;
} else {
foreach ($groups as $group) {
if (groups_is_member($group->id)) {
$types['group'] = true;
break;
}
}
}
}
}
}
105 changes: 105 additions & 0 deletions calendar/tests/lib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,10 @@ public function test_calendar_get_allowed_event_types_course() {

$this->setUser($user);

// In general for all courses, they don't have the ability to add course events yet.
$types = calendar_get_allowed_event_types();
$this->assertFalse($types['course']);

assign_capability('moodle/calendar:manageentries', CAP_ALLOW, $roleid, $context1, true);
assign_capability('moodle/calendar:manageentries', CAP_PROHIBIT, $roleid, $context2, true);

Expand All @@ -554,12 +558,20 @@ public function test_calendar_get_allowed_event_types_course() {
$types = calendar_get_allowed_event_types($course1->id);
$this->assertTrue($types['course']);

// If calling function without specified course, there is still a course where they have it.
$types = calendar_get_allowed_event_types();
$this->assertTrue($types['course']);

assign_capability('moodle/calendar:manageentries', CAP_PROHIBIT, $roleid, $context1, true);

// The user only now has the correct capability in both course 1 and 2 so we
// expect both to be in the results.
$types = calendar_get_allowed_event_types($course3->id);
$this->assertFalse($types['course']);

// They now do not have permission in any course.
$types = calendar_get_allowed_event_types();
$this->assertFalse($types['course']);
}

public function test_calendar_get_allowed_event_types_group_no_acces_to_diff_groups() {
Expand All @@ -582,6 +594,11 @@ public function test_calendar_get_allowed_event_types_group_no_acces_to_diff_gro
$types = calendar_get_allowed_event_types($course->id);
$this->assertTrue($types['course']);
$this->assertFalse($types['group']);

// Same result applies when not providing a specific course as they are only on one course.
$types = calendar_get_allowed_event_types();
$this->assertTrue($types['course']);
$this->assertFalse($types['group']);
}

public function test_calendar_get_allowed_event_types_group_no_groups() {
Expand All @@ -598,6 +615,12 @@ public function test_calendar_get_allowed_event_types_group_no_groups() {
// no groups so we shouldn't see a group type.
$types = calendar_get_allowed_event_types($course->id);
$this->assertTrue($types['course']);
$this->assertFalse($types['group']);

// Same result applies when not providing a specific course as they are only on one course.
$types = calendar_get_allowed_event_types();
$this->assertTrue($types['course']);
$this->assertFalse($types['group']);
}

public function test_calendar_get_allowed_event_types_group_access_all_groups() {
Expand All @@ -623,7 +646,12 @@ public function test_calendar_get_allowed_event_types_group_access_all_groups()
// the accessallgroups capability.
$types = calendar_get_allowed_event_types($course1->id);
$this->assertTrue($types['group']);

// Same result applies when not providing a specific course as they are only on one course.
$types = calendar_get_allowed_event_types();
$this->assertTrue($types['group']);
}

public function test_calendar_get_allowed_event_types_group_no_access_all_groups() {
$generator = $this->getDataGenerator();
$user = $generator->create_user();
Expand All @@ -642,10 +670,87 @@ public function test_calendar_get_allowed_event_types_group_no_access_all_groups
// groups that they are not a member of.
$types = calendar_get_allowed_event_types($course->id);
$this->assertFalse($types['group']);

// Same result applies when not providing a specific course as they are only on one course.
$types = calendar_get_allowed_event_types();
$this->assertFalse($types['group']);

assign_capability('moodle/calendar:manageentries', CAP_ALLOW, $roleid, $context, true);
assign_capability('moodle/site:accessallgroups', CAP_ALLOW, $roleid, $context, true);
$types = calendar_get_allowed_event_types($course->id);
$this->assertTrue($types['group']);

// Same result applies when not providing a specific course as they are only on one course.
$types = calendar_get_allowed_event_types();
$this->assertTrue($types['group']);
}

public function test_calendar_get_allowed_event_types_group_cap_no_groups() {
$generator = $this->getDataGenerator();
$user = $generator->create_user();
$course = $generator->create_course();
$context = context_course::instance($course->id);
$roleid = $generator->create_role();
$group = $generator->create_group(['courseid' => $course->id]);
$generator->enrol_user($user->id, $course->id, 'student');
$generator->role_assign($roleid, $user->id, $context->id);
assign_capability('moodle/calendar:managegroupentries', CAP_ALLOW, $roleid, $context, true);

$this->setUser($user);
$types = calendar_get_allowed_event_types($course->id);
$this->assertFalse($types['course']);
$this->assertFalse($types['group']);

// Check without specifying a course (same result as user only has one course).
$types = calendar_get_allowed_event_types();
$this->assertFalse($types['course']);
$this->assertFalse($types['group']);
}

public function test_calendar_get_allowed_event_types_group_cap_has_group() {
$generator = $this->getDataGenerator();
$user = $generator->create_user();
$course = $generator->create_course();
$context = context_course::instance($course->id);
$roleid = $generator->create_role();
$group = $generator->create_group(['courseid' => $course->id]);
$generator->enrol_user($user->id, $course->id, 'student');
$generator->role_assign($roleid, $user->id, $context->id);
groups_add_member($group, $user);
assign_capability('moodle/calendar:managegroupentries', CAP_ALLOW, $roleid, $context, true);

$this->setUser($user);
$types = calendar_get_allowed_event_types($course->id);
$this->assertFalse($types['course']);
$this->assertTrue($types['group']);

// Check without specifying a course (same result as user only has one course).
$types = calendar_get_allowed_event_types();
$this->assertFalse($types['course']);
$this->assertTrue($types['group']);
}

public function test_calendar_get_allowed_event_types_group_cap_access_all_groups() {
$generator = $this->getDataGenerator();
$user = $generator->create_user();
$course = $generator->create_course();
$context = context_course::instance($course->id);
$roleid = $generator->create_role();
$group = $generator->create_group(['courseid' => $course->id]);
$generator->enrol_user($user->id, $course->id, 'student');
$generator->role_assign($roleid, $user->id, $context->id);
assign_capability('moodle/calendar:managegroupentries', CAP_ALLOW, $roleid, $context, true);
assign_capability('moodle/site:accessallgroups', CAP_ALLOW, $roleid, $context, true);

$this->setUser($user);
$types = calendar_get_allowed_event_types($course->id);
$this->assertFalse($types['course']);
$this->assertTrue($types['group']);

// Check without specifying a course (same result as user only has one course).
$types = calendar_get_allowed_event_types();
$this->assertFalse($types['course']);
$this->assertTrue($types['group']);
}

/**
Expand Down

0 comments on commit 584500d

Please sign in to comment.