From 3903a268962af8cf21aaf7155b11c89d8f991d67 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Fri, 23 Aug 2019 10:46:42 +0100 Subject: [PATCH] MDL-62915 tool_dataprivacy: don't let primary admin delete themselves. --- admin/tool/dataprivacy/classes/api.php | 5 +- .../task/process_data_request_task.php | 16 ++++-- admin/tool/dataprivacy/tests/api_test.php | 57 ++++++++++++++++++- .../tests/behat/datadelete.feature | 6 ++ 4 files changed, 75 insertions(+), 9 deletions(-) diff --git a/admin/tool/dataprivacy/classes/api.php b/admin/tool/dataprivacy/classes/api.php index 80cb7ee172e..33da80e5025 100644 --- a/admin/tool/dataprivacy/classes/api.php +++ b/admin/tool/dataprivacy/classes/api.php @@ -774,7 +774,8 @@ class api { public static function can_create_data_deletion_request_for_self(int $userid = null): bool { global $USER; $userid = $userid ?: $USER->id; - return has_capability('tool/dataprivacy:requestdelete', \context_user::instance($userid), $userid); + return has_capability('tool/dataprivacy:requestdelete', \context_user::instance($userid), $userid) + && !is_primary_admin($userid); } /** @@ -803,7 +804,7 @@ class api { global $USER; $requesterid = $requesterid ?: $USER->id; return has_capability('tool/dataprivacy:makedatadeletionrequestsforchildren', \context_user::instance($userid), - $requesterid); + $requesterid) && !is_primary_admin($userid); } /** diff --git a/admin/tool/dataprivacy/classes/task/process_data_request_task.php b/admin/tool/dataprivacy/classes/task/process_data_request_task.php index 7b9fad33b0d..fe3652f3539 100644 --- a/admin/tool/dataprivacy/classes/task/process_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/process_data_request_task.php @@ -127,13 +127,17 @@ class process_data_request_task extends adhoc_task { $thing = $fs->create_file_from_pathname($filerecord, $exportedcontent); $completestatus = api::DATAREQUEST_STATUS_DOWNLOAD_READY; } else if ($request->type == api::DATAREQUEST_TYPE_DELETE) { - // Delete the data. - $manager = new \core_privacy\manager(); - $manager->set_observer(new \tool_dataprivacy\manager_observer()); + // Delete the data for users other than the primary admin, which is rejected. + if (is_primary_admin($foruser->id)) { + $completestatus = api::DATAREQUEST_STATUS_REJECTED; + } else { + $manager = new \core_privacy\manager(); + $manager->set_observer(new \tool_dataprivacy\manager_observer()); - $manager->delete_data_for_user($approvedclcollection); - $completestatus = api::DATAREQUEST_STATUS_DELETED; - $deleteuser = !$foruser->deleted; + $manager->delete_data_for_user($approvedclcollection); + $completestatus = api::DATAREQUEST_STATUS_DELETED; + $deleteuser = !$foruser->deleted; + } } // When the preparation of the metadata finishes, update the request status to awaiting approval. diff --git a/admin/tool/dataprivacy/tests/api_test.php b/admin/tool/dataprivacy/tests/api_test.php index f95dedda439..a913f3ec1d2 100644 --- a/admin/tool/dataprivacy/tests/api_test.php +++ b/admin/tool/dataprivacy/tests/api_test.php @@ -303,6 +303,29 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $requestid = $datarequest->get('id'); } + /** + * Test that deletion requests for the primary admin are rejected + */ + public function test_reject_data_deletion_request_primary_admin() { + $this->resetAfterTest(); + $this->setAdminUser(); + + $datarequest = api::create_data_request(get_admin()->id, api::DATAREQUEST_TYPE_DELETE); + + // Approve the request and execute the ad-hoc process task. + ob_start(); + api::approve_data_request($datarequest->get('id')); + $this->runAdhocTasks('\tool_dataprivacy\task\process_data_request_task'); + ob_end_clean(); + + $request = api::get_request($datarequest->get('id')); + $this->assertEquals(api::DATAREQUEST_STATUS_REJECTED, $request->get('status')); + + // Confirm they weren't deleted. + $user = core_user::get_user($request->get('userid')); + core_user::require_active_user($user); + } + /** * Test for api::can_contact_dpo() */ @@ -2126,6 +2149,33 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $this->assertFalse(api::can_create_data_deletion_request_for_self()); } + /** + * Test primary admin cannot create data deletion request for themselves + */ + public function test_can_create_data_deletion_request_for_self_primary_admin() { + $this->resetAfterTest(); + $this->setAdminUser(); + $this->assertFalse(api::can_create_data_deletion_request_for_self()); + } + + /** + * Test secondary admin can create data deletion request for themselves + */ + public function test_can_create_data_deletion_request_for_self_secondary_admin() { + $this->resetAfterTest(); + + $admin1 = $this->getDataGenerator()->create_user(); + $admin2 = $this->getDataGenerator()->create_user(); + + // The primary admin is the one listed first in the 'siteadmins' config. + set_config('siteadmins', implode(',', [$admin1->id, $admin2->id])); + + // Set the current user as the second admin (non-primary). + $this->setUser($admin2); + + $this->assertTrue(api::can_create_data_deletion_request_for_self()); + } + /** * Test user can create data deletion request for themselves if they have * "tool/dataprivacy:requestdelete" capability. @@ -2171,7 +2221,8 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { } /** - * Check parents can create data deletion request for their children but not others. + * Check parents can create data deletion request for their children (unless the child is the primary admin), + * but not other users. * * @throws coding_exception * @throws dml_exception @@ -2194,5 +2245,9 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $this->setUser($parent); $this->assertTrue(api::can_create_data_deletion_request_for_children($child->id)); $this->assertFalse(api::can_create_data_deletion_request_for_children($otheruser->id)); + + // Now make child the primary admin, confirm parent can't make deletion request. + set_config('siteadmins', $child->id); + $this->assertFalse(api::can_create_data_deletion_request_for_children($child->id)); } } diff --git a/admin/tool/dataprivacy/tests/behat/datadelete.feature b/admin/tool/dataprivacy/tests/behat/datadelete.feature index 5806102f6cc..0a4da1b114f 100644 --- a/admin/tool/dataprivacy/tests/behat/datadelete.feature +++ b/admin/tool/dataprivacy/tests/behat/datadelete.feature @@ -189,6 +189,12 @@ Feature: Data delete from the privacy API And I follow "Profile" in the user menu Then I should not see "Delete my account" + @javascript + Scenario: As a primary admin, the link to create a data deletion request should not be shown. + Given I log in as "admin" + When I follow "Profile" in the user menu + Then I should not see "Delete my account" + @javascript Scenario: As a Privacy Officer, I cannot Approve to Deny deletion data request without permission. Given the following "permission overrides" exist: