diff --git a/admin/tool/dataprivacy/classes/api.php b/admin/tool/dataprivacy/classes/api.php index 74281525d54..66e469abd94 100644 --- a/admin/tool/dataprivacy/classes/api.php +++ b/admin/tool/dataprivacy/classes/api.php @@ -105,18 +105,14 @@ class api { } /** - * Check's whether the current user has the capability to manage data requests. + * Checks whether the current user has the capability to manage data requests. * * @param int $userid The user ID. * @return bool - * @throws coding_exception - * @throws dml_exception */ public static function can_manage_data_requests($userid) { - $context = context_system::instance(); - - // A user can manage data requests if he/she has the site DPO role and has the capability to manage data requests. - return self::is_site_dpo($userid) && has_capability('tool/dataprivacy:managedatarequests', $context, $userid); + // Privacy officers can manage data requests. + return self::is_site_dpo($userid); } /** @@ -136,6 +132,31 @@ class api { require_capability('tool/dataprivacy:managedataregistry', $context); } + /** + * Fetches the list of configured privacy officer roles. + * + * Every time this function is called, it checks each role if they have the 'managedatarequests' capability and removes + * any role that doesn't have the required capability anymore. + * + * @return int[] + * @throws dml_exception + */ + public static function get_assigned_privacy_officer_roles() { + $roleids = []; + + // Get roles from config. + $configroleids = explode(',', str_replace(' ', '', get_config('tool_dataprivacy', 'dporoles'))); + if (!empty($configroleids)) { + // Fetch roles that have the capability to manage data requests. + $capableroles = array_keys(get_roles_with_capability('tool/dataprivacy:managedatarequests')); + + // Extract the configured roles that have the capability from the list of capable roles. + $roleids = array_intersect($capableroles, $configroleids); + } + + return $roleids; + } + /** * Fetches the role shortnames of Data Protection Officer roles. * @@ -144,7 +165,7 @@ class api { public static function get_dpo_role_names() : array { global $DB; - $dporoleids = explode(',', str_replace(' ', '', get_config('tool_dataprivacy', 'dporoles'))); + $dporoleids = self::get_assigned_privacy_officer_roles(); $dponames = array(); if (!empty($dporoleids)) { @@ -156,20 +177,15 @@ class api { } /** - * Fetches the list of users with the Data Protection Officer role. - * - * @throws dml_exception + * Fetches the list of users with the Privacy Officer role. */ public static function get_site_dpos() { // Get role(s) that can manage data requests. - $dporoles = explode(',', get_config('tool_dataprivacy', 'dporoles')); + $dporoles = self::get_assigned_privacy_officer_roles(); $dpos = []; $context = context_system::instance(); foreach ($dporoles as $roleid) { - if (empty($roleid)) { - continue; - } $allnames = get_all_user_name_fields(true, 'u'); $fields = 'u.id, u.confirmed, u.username, '. $allnames . ', ' . 'u.maildisplay, u.mailformat, u.maildigest, u.email, u.emailstop, u.city, '. @@ -189,15 +205,14 @@ class api { } /** - * Checks whether a given user is a site DPO. + * Checks whether a given user is a site Privacy Officer. * * @param int $userid The user ID. * @return bool - * @throws dml_exception */ public static function is_site_dpo($userid) { $dpos = self::get_site_dpos(); - return array_key_exists($userid, $dpos); + return array_key_exists($userid, $dpos) || is_siteadmin(); } /** diff --git a/admin/tool/dataprivacy/tests/api_test.php b/admin/tool/dataprivacy/tests/api_test.php index 18915c16a64..820611c70b3 100644 --- a/admin/tool/dataprivacy/tests/api_test.php +++ b/admin/tool/dataprivacy/tests/api_test.php @@ -124,6 +124,47 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $this->assertEquals($u1->id, $dpo->id); } + /** + * Test for \tool_dataprivacy\api::get_assigned_privacy_officer_roles(). + */ + public function test_get_assigned_privacy_officer_roles() { + global $DB; + + // Erroneously set the manager roles as the PO, even if it doesn't have the managedatarequests capability yet. + $managerroleid = $DB->get_field('role', 'id', array('shortname' => 'manager')); + set_config('dporoles', $managerroleid, 'tool_dataprivacy'); + // Get the assigned PO roles when nothing has been set yet. + $roleids = api::get_assigned_privacy_officer_roles(); + // Confirm that the returned list is empty. + $this->assertEmpty($roleids); + + $context = context_system::instance(); + + // Give the manager role with the capability to manage data requests. + assign_capability('tool/dataprivacy:managedatarequests', CAP_ALLOW, $managerroleid, $context->id, true); + + // Give the editing teacher role with the capability to manage data requests. + $editingteacherroleid = $DB->get_field('role', 'id', array('shortname' => 'editingteacher')); + assign_capability('tool/dataprivacy:managedatarequests', CAP_ALLOW, $editingteacherroleid, $context->id, true); + + // Get the non-editing teacher role ID. + $teacherroleid = $DB->get_field('role', 'id', array('shortname' => 'teacher')); + + // Erroneously map the manager and the non-editing teacher roles to the PO role. + $badconfig = $managerroleid . ',' . $teacherroleid; + set_config('dporoles', $badconfig, 'tool_dataprivacy'); + + // Get the assigned PO roles. + $roleids = api::get_assigned_privacy_officer_roles(); + + // There should only be one PO role. + $this->assertCount(1, $roleids); + // Confirm it contains the manager role. + $this->assertContains($managerroleid, $roleids); + // And it does not contain the editing teacher role. + $this->assertNotContains($editingteacherroleid, $roleids); + } + /** * Test for api::approve_data_request(). */