From 1b2ec27efc8dabc5709fde493042a8784b48673d Mon Sep 17 00:00:00 2001 From: Dongsheng Cai Date: Tue, 26 Oct 2021 05:37:39 +1100 Subject: [PATCH] MDL-71239 calendar: disallow manager role users edit user events --- calendar/externallib.php | 6 +- calendar/lib.php | 77 +++++++--- calendar/tests/events_test.php | 8 +- calendar/tests/externallib_test.php | 220 +++++++++++++++++++++++++++- calendar/tests/lib_test.php | 49 +++++++ calendar/upgrade.txt | 1 + lang/en/error.php | 1 + 7 files changed, 328 insertions(+), 34 deletions(-) diff --git a/calendar/externallib.php b/calendar/externallib.php index 620c20bad3b9c..2433fe8eb8625 100644 --- a/calendar/externallib.php +++ b/calendar/externallib.php @@ -803,10 +803,10 @@ public static function get_calendar_event_by_id($eventid) { $warnings = array(); $eventvault = event_container::get_event_vault(); - if ($event = $eventvault->get_event_by_id($eventid)) { + if ($event = $eventvault->get_event_by_id($params['eventid'])) { $mapper = event_container::get_event_mapper(); if (!calendar_view_event_allowed($mapper->from_event_to_legacy_event($event))) { - $event = null; + throw new moodle_exception('nopermissiontoviewcalendar', 'error'); } } @@ -814,7 +814,7 @@ public static function get_calendar_event_by_id($eventid) { // We can't return a warning in this case because the event is not optional. // We don't know the context for the event and it's not worth loading it. $syscontext = context_system::instance(); - throw new \required_capability_exception($syscontext, 'moodle/course:view', 'nopermission', ''); + throw new \required_capability_exception($syscontext, 'moodle/course:view', 'nopermissions', 'error'); } $cache = new events_related_objects_cache([$event]); diff --git a/calendar/lib.php b/calendar/lib.php index a5942ad0623e5..f5abca7489176 100644 --- a/calendar/lib.php +++ b/calendar/lib.php @@ -2171,6 +2171,24 @@ function calendar_set_filters(array $courseeventsfrom, $ignorefilters = false, s return array($courses, $group, $userid); } +/** + * Can current user manage a non user event in system context. + * + * @param calendar_event|stdClass $event event object + * @return boolean + */ +function calendar_can_manage_non_user_event_in_system($event) { + $sitecontext = \context_system::instance(); + $isuserevent = $event->eventtype == 'user'; + $canmanageentries = has_capability('moodle/calendar:manageentries', $sitecontext); + // If user has manageentries at site level and it's not user event, return true. + if ($canmanageentries && !$isuserevent) { + return true; + } + + return false; +} + /** * Return the capability for viewing a calendar event. * @@ -2185,10 +2203,7 @@ function calendar_view_event_allowed(calendar_event $event) { return true; } - // If a user can manage events at the site level they can see any event. - $sitecontext = \context_system::instance(); - // If user has manageentries at site level, return true. - if (has_capability('moodle/calendar:manageentries', $sitecontext)) { + if (calendar_can_manage_non_user_event_in_system($event)) { return true; } @@ -2244,11 +2259,7 @@ function calendar_view_event_allowed(calendar_event $event) { return can_access_course(get_course($event->courseid)); } else if ($event->userid) { - if ($event->userid != $USER->id) { - // No-one can ever see another users events. - return false; - } - return true; + return calendar_can_manage_user_event($event); } else { throw new moodle_exception('unknown event type'); } @@ -2321,10 +2332,7 @@ function calendar_edit_event_allowed($event, $manualedit = false) { } } - $sitecontext = \context_system::instance(); - - // If user has manageentries at site level, return true. - if (has_capability('moodle/calendar:manageentries', $sitecontext)) { + if (calendar_can_manage_non_user_event_in_system($event)) { return true; } @@ -2348,7 +2356,38 @@ function calendar_edit_event_allowed($event, $manualedit = false) { // If course is not set, but userid id set, it's a user event. return (has_capability('moodle/calendar:manageownentries', $event->context)); } else if (!empty($event->userid)) { - return (has_capability('moodle/calendar:manageentries', $event->context)); + return calendar_can_manage_user_event($event); + } + + return false; +} + +/** + * Can current user edit/delete/add an user event? + * + * @param calendar_event|stdClass $event event object + * @return bool + */ +function calendar_can_manage_user_event($event): bool { + global $USER; + + if (!($event instanceof \calendar_event)) { + $event = new \calendar_event(clone($event)); + } + + $canmanage = has_capability('moodle/calendar:manageentries', $event->context); + $canmanageown = has_capability('moodle/calendar:manageownentries', $event->context); + $ismyevent = $event->userid == $USER->id; + $isadminevent = is_siteadmin($event->userid); + + if ($canmanageown && $ismyevent) { + return true; + } + + // In site context, user must have login and calendar:manageentries permissions + // ... to manage other user's events except admin users. + if ($canmanage && !$isadminevent) { + return true; } return false; @@ -2660,10 +2699,7 @@ function calendar_add_event_allowed($event) { return false; } - $sitecontext = \context_system::instance(); - - // If user has manageentries at site level, always return true. - if (has_capability('moodle/calendar:manageentries', $sitecontext)) { + if (calendar_can_manage_non_user_event_in_system($event)) { return true; } @@ -2682,10 +2718,7 @@ function calendar_add_event_allowed($event) { (has_capability('moodle/calendar:managegroupentries', $event->context) && groups_is_member($event->groupid))); case 'user': - if ($event->userid == $USER->id) { - return (has_capability('moodle/calendar:manageownentries', $event->context)); - } - // There is intentionally no 'break'. + return calendar_can_manage_user_event($event); case 'site': return has_capability('moodle/calendar:manageentries', $event->context); default: diff --git a/calendar/tests/events_test.php b/calendar/tests/events_test.php index 1d79f1332ec5f..508a675e62f1d 100644 --- a/calendar/tests/events_test.php +++ b/calendar/tests/events_test.php @@ -223,7 +223,8 @@ public function test_calendar_event_updated() { * Tests for calendar_event_updated event. */ public function test_calendar_event_updated_toggle_visibility() { - global $DB, $SITE; + global $DB; + $siteid = 0; $this->resetAfterTest(); @@ -242,10 +243,9 @@ public function test_calendar_event_updated_toggle_visibility() { $event = $events[0]; $this->assertInstanceOf('\core\event\calendar_event_updated', $event); $this->assertEquals('event', $event->objecttable); - $this->assertEquals($SITE->id, $event->courseid); + $this->assertEquals($siteid, $event->courseid); $this->assertEquals($calevent->context, $event->get_context()); - $expectedlog = array($SITE->id, 'calendar', 'edit', 'event.php?action=edit&id=' . $calevent->id , - $calevent->name); + $expectedlog = [$siteid, 'calendar', 'edit', 'event.php?action=edit&id=' . $calevent->id , $calevent->name]; $this->assertEventLegacyLogData($expectedlog, $event); $other = array('repeatid' => 0, 'timestart' => $time, 'name' => 'Some wickedly awesome event yo!'); $this->assertEquals($other, $event->other); diff --git a/calendar/tests/externallib_test.php b/calendar/tests/externallib_test.php index 0d05cd0ad41c9..b69825a502546 100644 --- a/calendar/tests/externallib_test.php +++ b/calendar/tests/externallib_test.php @@ -104,7 +104,14 @@ public static function create_calendar_event($name, $userid = 0, $type = 'user', } } if (!isset($prop->courseid)) { - $prop->courseid = $SITE->id; + // Set a default value of the event's course ID field. + if ($type === 'user') { + // If it's a user event, course ID should be zero. + $prop->courseid = 0; + } else { + // Otherwise, default to the site ID. + $prop->courseid = $SITE->id; + } } // Determine event priority. @@ -202,7 +209,7 @@ public function test_delete_calendar_events() { $record = new stdClass(); $record->courseid = $course->id; $courseevent = $this->create_calendar_event('course', $USER->id, 'course', 3, time(), $record); - $userevent = $this->create_calendar_event('user', $USER->id); + $userevent = $this->create_calendar_event('user', $user->id); $record = new stdClass(); $record->courseid = $course->id; $record->groupid = $group->id; @@ -1806,8 +1813,7 @@ public function test_submit_create_update_form_create_user_event_no_permission() $this->resetAfterTest(true); $this->setUser($user); - $this->expectException('moodle_exception'); - + $this->expectException(moodle_exception::class); external_api::clean_returnvalue( core_calendar_external::submit_create_update_form_returns(), core_calendar_external::submit_create_update_form($querystring) @@ -2609,13 +2615,217 @@ public function test_get_calendar_event_by_id_no_course_permission() { $this->assertEquals($data['event']['id'], $courseevent->id); // User not enrolled in the course cannot load the course event. $this->setUser($user2); - $this->expectException('required_capability_exception'); + $this->expectException(moodle_exception::class); $data = external_api::clean_returnvalue( core_calendar_external::get_calendar_event_by_id_returns(), core_calendar_external::get_calendar_event_by_id($courseevent->id) ); } + /** + * User data for testing reading calendar events. + * + * @return array + */ + public function test_get_calendar_event_by_id_prevent_read_other_users_events_data_provider(): array { + $syscontext = context_system::instance(); + $managerrole = 'manager'; + return [ + [true, false, $syscontext, $managerrole, true], + [false, false, $syscontext, $managerrole, false], + [false, false, null, null, true], + [false, true, null, null, false], + ]; + } + + /** + * Prevent user from reading other user's event. + * + * @covers \core_calendar_external::get_calendar_event_by_id + * @dataProvider test_get_calendar_event_by_id_prevent_read_other_users_events_data_provider + * + * @param bool $isadminevent Is admin's event + * @param bool $isadmin Is current user admin user + * @param null|stdClass $readerrolecontext Reader role context + * @param null|string $readerrolename Role name + * @param bool $expectexception Should the test throw exception + */ + public function test_get_calendar_event_by_id_prevent_read_other_users_events( + bool $isadminevent, bool $isadmin, ?stdClass $readerrolecontext, + ?string $readerrolename, bool $expectexception) { + global $USER, $DB; + + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + + if ($isadminevent) { + $this->setAdminUser(); + } else { + $user = $generator->create_user(); + $this->setUser($user); + } + $userevent = $this->create_calendar_event('user event', $USER->id, 'user', 0, time()); + $results = external_api::clean_returnvalue( + core_calendar_external::get_calendar_event_by_id_returns(), + core_calendar_external::get_calendar_event_by_id($userevent->id) + ); + $event = reset($results); + $this->assertEquals($userevent->id, $event['id']); + + if ($isadmin) { + $this->setAdminUser(); + } else { + $reader = $generator->create_user(); + if ($readerrolename && $readerrolecontext) { + $managerroleid = $DB->get_field('role', 'id', ['shortname' => $readerrolename]); + role_assign($managerroleid, $reader->id, $readerrolecontext->id); + } + $this->setUser($reader); + } + + if ($expectexception) { + // Setup if exception is expected for the test. + $this->expectException(moodle_exception::class); + } + external_api::clean_returnvalue( + core_calendar_external::get_calendar_event_by_id_returns(), + core_calendar_external::get_calendar_event_by_id($userevent->id) + ); + } + + /** + * User data for testing editing or deleting calendar events. + * + * @return array + */ + public function test_edit_or_delete_other_users_events_data_provider(): array { + $syscontext = context_system::instance(); + $managerrole = 'manager'; + return [ + [false, false, $syscontext, $managerrole, false], + [false, true, $syscontext, $managerrole, true], + [false, false, null, null, true], + [true, false, null, null, false], + ]; + } + + /** + * Test the behavior of deleting other users' user events. + * + * @dataProvider test_edit_or_delete_other_users_events_data_provider + * @covers \core_calendar_external::delete_calendar_events + * @param bool $isadmin Whether the current user is admin. + * @param bool $isadminevent Whether it's an admin event or not. + * @param stdClass|null $writerrolecontext The reader role context. + * @param string|null $writerrolename The role name. + * @param bool $expectexception Whether the test should throw an exception or not. + */ + public function test_delete_other_users_events(bool $isadmin, bool $isadminevent, + ?stdClass $writerrolecontext, ?string $writerrolename, bool $expectexception) { + global $DB, $USER; + + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + + if ($isadminevent) { + $this->setAdminUser(); + $user = $USER; + } else { + $user = $generator->create_user(); + $this->setUser($user); + } + $userevent = $this->create_calendar_event('user event', $user->id, 'user', 0, time()); + + if ($isadmin) { + $this->setAdminUser(); + } else { + $writer = $generator->create_user(); + if ($writerrolename && $writerrolecontext) { + $managerroleid = $DB->get_field('role', 'id', ['shortname' => $writerrolename]); + role_assign($managerroleid, $writer->id, $writerrolecontext->id); + } + $this->setUser($writer); + } + + if ($expectexception) { + $this->expectException(moodle_exception::class); + } + $events = [ + ['eventid' => $userevent->id, 'repeat' => 0] + ]; + core_calendar_external::delete_calendar_events($events); + } + + /** + * Test the behavior of editing other users' user events + * + * @dataProvider test_edit_or_delete_other_users_events_data_provider + * @covers \core_calendar_external::submit_create_update_form + * @param bool $isadmin Whether the current user is admin. + * @param bool $isadminevent Whether it's an admin event or not. + * @param stdClass|null $writerrolecontext The reader role context. + * @param string|null $writerrolename The role name. + * @param bool $expectexception Whether the test should throw an exception or not. + */ + public function test_edit_other_users_events(bool $isadmin, bool $isadminevent, + ?stdClass $writerrolecontext, ?string $writerrolename, bool $expectexception) { + global $DB, $USER; + + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + if ($isadminevent) { + $this->setAdminUser(); + $user = $USER; + } else { + $user = $generator->create_user(); + } + + $formdata = [ + 'id' => 0, + 'userid' => $user->id, + 'modulename' => '', + 'instance' => 0, + 'visible' => 1, + 'eventtype' => 'user', + 'name' => 'Test', + 'timestart' => [ + 'day' => 1, + 'month' => 1, + 'year' => 2021, + 'hour' => 1, + 'minute' => 0, + ], + 'description' => [ + 'text' => 'xxxxx', + 'format' => 1, + 'itemid' => 0 + ], + 'location' => 'Test', + 'duration' => 0, + ]; + $formdata = \core_calendar\local\event\forms\create::mock_generate_submit_keys($formdata); + + $querystring = http_build_query($formdata, '', '&'); + + if ($isadmin) { + $this->setAdminUser(); + } else { + $writer = $generator->create_user(); + if ($writerrolename && $writerrolecontext) { + $managerroleid = $DB->get_field('role', 'id', ['shortname' => $writerrolename]); + role_assign($managerroleid, $writer->id, $writerrolecontext->id); + } + $this->setUser($writer); + } + $USER->ignoresesskey = true; + + if ($expectexception) { + $this->expectException(moodle_exception::class); + } + core_calendar_external::submit_create_update_form($querystring); + } + /** * A user should not be able load the calendar events for a category they cannot see. */ diff --git a/calendar/tests/lib_test.php b/calendar/tests/lib_test.php index 6fb459df8b6e4..653177b3183dc 100644 --- a/calendar/tests/lib_test.php +++ b/calendar/tests/lib_test.php @@ -992,4 +992,53 @@ public function test_calendar_get_export_token_for_another_user() { $this->assertEquals($expected, $authtoken); } + + /** + * Test calendar_can_manage_user_event for different users. + * + * @covers ::calendar_can_manage_user_event + */ + public function test_calendar_can_manage_user_event() { + global $DB, $USER; + $generator = $this->getDataGenerator(); + $sitecontext = context_system::instance(); + $this->resetAfterTest(); + $this->setAdminUser(); + $user1 = $generator->create_user(); + $user2 = $generator->create_user(); + $adminevent = create_event([ + 'eventtype' => 'user', + 'userid' => $USER->id, + ]); + + $this->setUser($user1); + $user1event = create_event([ + 'name' => 'user1 event', + 'eventtype' => 'user', + 'userid' => $user1->id, + ]); + $this->setUser($user2); + $user2event = create_event([ + 'name' => 'user2 event', + 'eventtype' => 'user', + 'userid' => $user2->id, + ]); + $this->setUser($user1); + $result = calendar_can_manage_user_event($user1event); + $this->assertEquals(true, $result); + $result = calendar_can_manage_user_event($user2event); + $this->assertEquals(false, $result); + + $sitemanager = $generator->create_user(); + + $managerroleid = $DB->get_field('role', 'id', ['shortname' => 'manager']); + role_assign($managerroleid, $sitemanager->id, $sitecontext->id); + + $this->setUser($sitemanager); + + $result = calendar_can_manage_user_event($user1event); + $this->assertEquals(true, $result); + $result = calendar_can_manage_user_event($adminevent); + $this->assertEquals(false, $result); + } } diff --git a/calendar/upgrade.txt b/calendar/upgrade.txt index 35fda4daa052c..ab05dcfaf97b7 100644 --- a/calendar/upgrade.txt +++ b/calendar/upgrade.txt @@ -4,6 +4,7 @@ information provided here is intended especially for developers. === 3.10 === * The core_calendar\local\event\value_objects\times_interface class now has new method get_usermidnight_time() which returns the user midnight time for a given event. +* Updated calendar_can_manage_user_event() function to check permissions to user events. === 3.9 === * Plugins can now create their own calendar events, both standard and action ones. To do it they need to specify diff --git a/lang/en/error.php b/lang/en/error.php index c46a4bb6b1ffc..12501c21b2cdb 100644 --- a/lang/en/error.php +++ b/lang/en/error.php @@ -455,6 +455,7 @@ $string['nopermissiontoshow'] = 'No permission to see this!'; $string['nopermissiontounlock'] = 'No permission to unlock!'; $string['nopermissiontoupdatecalendar'] = 'Sorry, but you do not have permission to update the calendar event.'; +$string['nopermissiontoviewcalendar'] = 'Sorry, but you do not have permission to view the calendar event.'; $string['nopermissiontoviewgrades'] = 'Cannot view grades.'; $string['nopermissiontoviewletergrade'] = 'Missing permission to view letter grades'; $string['nopermissiontoviewpage'] = 'You are not allowed to look at this page';