From 1f258e679afe35200fa03e822ca2514941e6359f Mon Sep 17 00:00:00 2001 From: Mihail Geshoski Date: Mon, 5 Nov 2018 16:20:06 +0800 Subject: [PATCH 1/2] MDL-62563 privacy: Add unit tests --- admin/tool/dataprivacy/tests/task_test.php | 189 +++++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 admin/tool/dataprivacy/tests/task_test.php diff --git a/admin/tool/dataprivacy/tests/task_test.php b/admin/tool/dataprivacy/tests/task_test.php new file mode 100644 index 00000000000..e6be1b850a5 --- /dev/null +++ b/admin/tool/dataprivacy/tests/task_test.php @@ -0,0 +1,189 @@ +. + +/** + * Tests for scheduled tasks. + * + * @package tool_dataprivacy + * @copyright 2018 Mihail Geshoski + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); +require_once('data_privacy_testcase.php'); + +use tool_dataprivacy\api; + +/** + * Tests for scheduled tasks. + * + * @package tool_dataprivacy + * @copyright 2018 Mihail Geshoski + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class tool_dataprivacy_task_testcase extends data_privacy_testcase { + + /** + * Test tearDown. + */ + public function tearDown() { + \core_privacy\local\request\writer::reset(); + } + + /** + * Ensure that a delete data request for pre-existing deleted users + * is created when there are not any existing data requests + * for that particular user. + */ + public function test_delete_existing_deleted_users_task_no_previous_requests() { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + // Create a user. + $user = $this->getDataGenerator()->create_user(); + // Mark the user as deleted. + $user->deleted = 1; + $DB->update_record('user', $user); + + // The user should not have a delete data request. + $this->assertCount(0, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_DELETE])); + + $this->execute_task('tool_dataprivacy\task\delete_existing_deleted_users'); + // After running the scheduled task, the deleted user should have a delete data request. + $this->assertCount(1, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_DELETE])); + } + + /** + * Ensure that a delete data request for pre-existing deleted users + * is created when there are existing non-delete data requests + * for that particular user. + */ + public function test_delete_existing_deleted_users_task_existing_export_data_requests() { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + // Create a user. + $user = $this->getDataGenerator()->create_user(); + // Create export data request for the user. + api::create_data_request($user->id, api::DATAREQUEST_TYPE_EXPORT); + // Mark the user as deleted. + $user->deleted = 1; + $DB->update_record('user', $user); + + // The user should have a export data request. + $this->assertCount(1, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_EXPORT])); + // The user should not have a delete data request. + $this->assertCount(0, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_DELETE])); + + $this->execute_task('tool_dataprivacy\task\delete_existing_deleted_users'); + // After running the scheduled task, the deleted user should have a delete data request. + $this->assertCount(1, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_DELETE])); + } + + /** + * Ensure that a delete data request for pre-existing deleted users + * is not created when there are existing ongoing delete data requests + * for that particular user. + */ + public function test_delete_existing_deleted_users_task_existing_ongoing_delete_data_requests() { + $this->resetAfterTest(); + $this->setAdminUser(); + // Create a user. + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + // Create delete data request for the user. + $datarequest = api::create_data_request($user->id, api::DATAREQUEST_TYPE_DELETE); + $requestid = $datarequest->get('id'); + api::update_request_status($requestid, api::DATAREQUEST_STATUS_AWAITING_APPROVAL); + + // The user should have an ongoing delete data request. + $this->assertCount(1, api::get_data_requests($user->id, + [api::DATAREQUEST_STATUS_AWAITING_APPROVAL], [api::DATAREQUEST_TYPE_DELETE])); + + $this->setAdminUser(); + // Delete the user. + delete_user($user); + // The user should still have the existing ongoing delete data request. + $this->assertCount(1, \tool_dataprivacy\api::get_data_requests($user->id, + [api::DATAREQUEST_STATUS_AWAITING_APPROVAL], [api::DATAREQUEST_TYPE_DELETE])); + + $this->execute_task('tool_dataprivacy\task\delete_existing_deleted_users'); + // After running the scheduled task, the user should have only one delete data request. + $this->assertCount(1, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_DELETE])); + // The user should not have a newly created delete data request. + $this->assertCount(0, api::get_data_requests($user->id, + [api::DATAREQUEST_STATUS_PENDING], [api::DATAREQUEST_TYPE_DELETE])); + } + + /** + * Ensure that a delete data request for pre-existing deleted users + * is not created when there are existing finished delete data requests + * for that particular user. + */ + public function test_delete_existing_deleted_users_task_existing_finished_delete_data_requests() { + $this->resetAfterTest(); + $this->setAdminUser(); + // Create a user. + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + // Create delete data request for the user. + $datarequest = api::create_data_request($user->id, api::DATAREQUEST_TYPE_DELETE); + $requestid = $datarequest->get('id'); + api::update_request_status($requestid, api::DATAREQUEST_STATUS_CANCELLED); + + // The user should have a delete data request. + $this->assertCount(1, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_DELETE])); + // The user should not have an ongoing data requests. + $this->assertFalse(api::has_ongoing_request($user->id, api::DATAREQUEST_TYPE_DELETE)); + + $this->setAdminUser(); + // Delete the user. + delete_user($user); + // The user should still have the existing finished delete data request. + $this->assertCount(1, \tool_dataprivacy\api::get_data_requests($user->id, + [api::DATAREQUEST_STATUS_CANCELLED], [api::DATAREQUEST_TYPE_DELETE])); + + $this->execute_task('tool_dataprivacy\task\delete_existing_deleted_users'); + // After running the scheduled task, the user should still have one delete data requests. + $this->assertCount(1, api::get_data_requests($user->id, [], + [api::DATAREQUEST_TYPE_DELETE])); + // The user should still have the existing finished delete data request. + $this->assertCount(1, \tool_dataprivacy\api::get_data_requests($user->id, + [api::DATAREQUEST_STATUS_CANCELLED], [api::DATAREQUEST_TYPE_DELETE])); + } + + /** + * Helper to execute a particular task. + * + * @param string $task The task. + */ + private function execute_task($task) { + // Run the scheduled task. + ob_start(); + $task = \core\task\manager::get_scheduled_task($task); + $task->execute(); + ob_end_clean(); + } +} From 0375eb8488125e2e6d7525b5337353b6018a749f Mon Sep 17 00:00:00 2001 From: Mihail Geshoski Date: Mon, 5 Nov 2018 16:20:37 +0800 Subject: [PATCH 2/2] MDL-62563 privacy: Create delete data request for existing deleted users --- admin/tool/dataprivacy/classes/api.php | 19 ++-- .../tool/dataprivacy/classes/data_request.php | 14 +++ .../task/delete_existing_deleted_users.php | 88 +++++++++++++++++++ .../task/initiate_data_request_task.php | 21 ----- .../task/process_data_request_task.php | 22 ++++- admin/tool/dataprivacy/db/install.xml | 1 + admin/tool/dataprivacy/db/tasks.php | 8 ++ admin/tool/dataprivacy/db/upgrade.php | 12 +++ .../dataprivacy/lang/en/tool_dataprivacy.php | 2 + admin/tool/dataprivacy/tests/api_test.php | 1 - admin/tool/dataprivacy/version.php | 2 +- 11 files changed, 153 insertions(+), 37 deletions(-) create mode 100644 admin/tool/dataprivacy/classes/task/delete_existing_deleted_users.php diff --git a/admin/tool/dataprivacy/classes/api.php b/admin/tool/dataprivacy/classes/api.php index 4c0e1ed4343..ff782a1a4a6 100644 --- a/admin/tool/dataprivacy/classes/api.php +++ b/admin/tool/dataprivacy/classes/api.php @@ -227,25 +227,24 @@ class api { * @param int $foruser The user whom the request is being made for. * @param int $type The request type. * @param string $comments Request comments. + * @param int $creationmethod The creation method of the data request. * @return data_request * @throws invalid_persistent_exception * @throws coding_exception */ - public static function create_data_request($foruser, $type, $comments = '') { - global $USER; + public static function create_data_request($foruser, $type, $comments = '', + $creationmethod = data_request::DATAREQUEST_CREATION_MANUAL) { + global $USER, $ADMIN; $datarequest = new data_request(); // The user the request is being made for. $datarequest->set('userid', $foruser); - $requestinguser = $USER->id; - // Check when the user is making a request on behalf of another. - if ($requestinguser != $foruser) { - if (self::is_site_dpo($requestinguser)) { - // The user making the request is a DPO. Should be fine. - $datarequest->set('dpo', $requestinguser); - } - } + // The cron is considered to be a guest user when it creates a data request. + // NOTE: This should probably be changed. We should leave the default value for $requestinguser if + // the request is not explicitly created by a specific user. + $requestinguser = (isguestuser() && $creationmethod == data_request::DATAREQUEST_CREATION_AUTO) ? + $ADMIN->id : $USER->id; // The user making the request. $datarequest->set('requestedby', $requestinguser); // Set status. diff --git a/admin/tool/dataprivacy/classes/data_request.php b/admin/tool/dataprivacy/classes/data_request.php index 92c8c1ff243..997642261fa 100644 --- a/admin/tool/dataprivacy/classes/data_request.php +++ b/admin/tool/dataprivacy/classes/data_request.php @@ -37,6 +37,12 @@ class data_request extends persistent { /** The table name this persistent object maps to. */ const TABLE = 'tool_dataprivacy_request'; + /** Data request created manually. */ + const DATAREQUEST_CREATION_MANUAL = 0; + + /** Data request created automatically. */ + const DATAREQUEST_CREATION_AUTO = 1; + /** * Return the definition of the properties of this model. * @@ -111,6 +117,14 @@ class data_request extends persistent { 'type' => PARAM_INT, 'default' => FORMAT_PLAIN ], + 'creationmethod' => [ + 'default' => self::DATAREQUEST_CREATION_MANUAL, + 'choices' => [ + self::DATAREQUEST_CREATION_MANUAL, + self::DATAREQUEST_CREATION_AUTO + ], + 'type' => PARAM_INT + ], ]; } diff --git a/admin/tool/dataprivacy/classes/task/delete_existing_deleted_users.php b/admin/tool/dataprivacy/classes/task/delete_existing_deleted_users.php new file mode 100644 index 00000000000..a0787477ea1 --- /dev/null +++ b/admin/tool/dataprivacy/classes/task/delete_existing_deleted_users.php @@ -0,0 +1,88 @@ +. + +/** + * Scheduled task to create delete data request for pre-existing deleted users. + * + * @package tool_dataprivacy + * @copyright 2018 Mihail Geshoski + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace tool_dataprivacy\task; + +use core\task\scheduled_task; +use tool_dataprivacy\api; +use tool_dataprivacy\data_request; + +defined('MOODLE_INTERNAL') || die(); + +require_once($CFG->dirroot . '/' . $CFG->admin . '/tool/dataprivacy/lib.php'); + +/** + * Scheduled task to create delete data request for pre-existing deleted users. + * + * @package tool_dataprivacy + * @copyright 2018 Mihail Geshoski + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class delete_existing_deleted_users extends scheduled_task { + + /** + * Returns the task name. + * + * @return string + */ + public function get_name() { + return get_string('deleteexistingdeleteduserstask', 'tool_dataprivacy'); + } + + /** + * Run the task to delete expired data request files and update request statuses. + * + */ + public function execute() { + global $DB; + + // Select all deleted users that do not have any delete data requests created for them. + $sql = "SELECT DISTINCT(u.id) + FROM {user} u + LEFT JOIN {tool_dataprivacy_request} r + ON u.id = r.userid + WHERE u.deleted = ? + AND (r.id IS NULL + OR r.type != ?)"; + + $params = [ + 1, + api::DATAREQUEST_TYPE_DELETE + ]; + + $deletedusers = $DB->get_records_sql($sql, $params); + $createdrequests = 0; + + foreach ($deletedusers as $user) { + api::create_data_request($user->id, api::DATAREQUEST_TYPE_DELETE, + get_string('datarequestcreatedfromscheduledtask', 'tool_dataprivacy'), + data_request::DATAREQUEST_CREATION_AUTO); + $createdrequests++; + } + + if ($createdrequests > 0) { + mtrace($createdrequests . ' delete data request(s) created for existing deleted users'); + } + } +} diff --git a/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php b/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php index 70402f48f4b..d35f9176ef9 100644 --- a/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php @@ -70,27 +70,6 @@ class initiate_data_request_task extends adhoc_task { return; } - $requestedby = $datarequest->get('requestedby'); - $valid = true; - $comment = ''; - $foruser = $datarequest->get('userid'); - if ($foruser != $requestedby) { - if (!$valid = api::can_create_data_request_for_user($foruser, $requestedby)) { - $params = (object)[ - 'requestedby' => $requestedby, - 'userid' => $foruser - ]; - $comment = get_string('errornocapabilitytorequestforothers', 'tool_dataprivacy', $params); - mtrace($comment); - } - } - // Reject the request outright if it's invalid. - if (!$valid) { - $dpo = $datarequest->get('dpo'); - api::update_request_status($requestid, api::DATAREQUEST_STATUS_REJECTED, $dpo, $comment); - return; - } - // Update the status of this request as pre-processing. mtrace('Generating the contexts containing personal data for the user...'); api::update_request_status($requestid, api::DATAREQUEST_STATUS_PREPROCESSING); 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 50e69df8504..cfac777c209 100644 --- a/admin/tool/dataprivacy/classes/task/process_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/process_data_request_task.php @@ -76,7 +76,6 @@ class process_data_request_task extends adhoc_task { // Get the user details now. We might not be able to retrieve it later if it's a deletion processing. $foruser = core_user::get_user($request->userid); - $usercontext = \context_user::instance($foruser->id); // Update the status of this request as pre-processing. mtrace('Processing request...'); @@ -87,6 +86,14 @@ class process_data_request_task extends adhoc_task { // Run as the user performing the export. cron_setup_user($foruser); + // Get the user context. + $usercontext = \context_user::instance($foruser->id, IGNORE_MISSING); + if (!$usercontext) { + mtrace("Request {$requestid} cannot be processed due to a missing user context instance for the user + with ID {$foruser->id}. Skipping..."); + return; + } + // Get the collection of approved_contextlist objects needed for core_privacy data export. $approvedclcollection = api::get_approved_contextlist_collection_for_request($requestpersistent); @@ -195,12 +202,19 @@ class process_data_request_task extends adhoc_task { // Send message to the user involved. if ($notifyuser) { + $messagesent = false; if ($emailonly) { - email_to_user($foruser, $dpo, $subject, $message->fullmessage, $messagehtml); + // Do not sent an email if the user has been deleted. The user email has been previously deleted. + if (!$foruser->deleted) { + $messagesent = email_to_user($foruser, $dpo, $subject, $message->fullmessage, $messagehtml); + } } else { - message_send($message); + $messagesent = message_send($message); + } + + if ($messagesent) { + mtrace('Message sent to user: ' . $messagetextdata['username']); } - mtrace('Message sent to user: ' . $messagetextdata['username']); } // Send to requester as well in some circumstances. diff --git a/admin/tool/dataprivacy/db/install.xml b/admin/tool/dataprivacy/db/install.xml index 98e852b1b82..92f33bdbeb1 100644 --- a/admin/tool/dataprivacy/db/install.xml +++ b/admin/tool/dataprivacy/db/install.xml @@ -19,6 +19,7 @@ + diff --git a/admin/tool/dataprivacy/db/tasks.php b/admin/tool/dataprivacy/db/tasks.php index 5ee3a19aa17..e3bedd695ab 100644 --- a/admin/tool/dataprivacy/db/tasks.php +++ b/admin/tool/dataprivacy/db/tasks.php @@ -50,5 +50,13 @@ $tasks = array( 'day' => '*', 'dayofweek' => '*', 'month' => '*' + ), array( + 'classname' => 'tool_dataprivacy\task\delete_existing_deleted_users', + 'blocking' => 0, + 'minute' => 'R', + 'hour' => 'R', + 'day' => '*', + 'dayofweek' => '*', + 'month' => '*' ), ); diff --git a/admin/tool/dataprivacy/db/upgrade.php b/admin/tool/dataprivacy/db/upgrade.php index 2e2f3cc7d1b..7af41c6f710 100644 --- a/admin/tool/dataprivacy/db/upgrade.php +++ b/admin/tool/dataprivacy/db/upgrade.php @@ -201,5 +201,17 @@ function xmldb_tool_dataprivacy_upgrade($oldversion) { upgrade_plugin_savepoint(true, 2017051517, 'tool', 'dataprivacy'); } + if ($oldversion < 2017051554) { + // Define field sensitivedatareasons to be added to tool_dataprivacy_purpose. + $table = new xmldb_table('tool_dataprivacy_request'); + $field = new xmldb_field('creationmethod', XMLDB_TYPE_INTEGER, 10, null, XMLDB_NOTNULL, null, 0, 'timemodified'); + // Conditionally launch add field sensitivedatareasons. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + // Dataprivacy savepoint reached. + upgrade_plugin_savepoint(true, 2017051554, 'tool', 'dataprivacy'); + } + return true; } diff --git a/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php b/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php index f9cc79d2e3b..f5e6e4169c3 100644 --- a/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php +++ b/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php @@ -77,6 +77,7 @@ $string['dataprivacy:downloadallrequests'] = 'Download exported data for everyon $string['dataregistry'] = 'Data registry'; $string['dataregistryinfo'] = 'The data registry enables categories (types of data) and purposes (the reasons for processing data) to be set for all content on the site - from users and courses down to activities and blocks. For each purpose, a retention period may be set. When a retention period has expired, the data is flagged and listed for deletion, awaiting admin confirmation.'; $string['datarequestcreatedforuser'] = 'Data request created for {$a}'; +$string['datarequestcreatedfromscheduledtask'] = 'Automatically created from a scheduled task (pre-existing deleted user).'; $string['datarequestemailsubject'] = 'Data request: {$a}'; $string['datarequests'] = 'Data requests'; $string['datecomment'] = '[{$a->date}]: ' . PHP_EOL . ' {$a->comment}'; @@ -89,6 +90,7 @@ $string['deletecategorytext'] = 'Are you sure you want to delete the category \' $string['deletedefaults'] = 'Delete defaults: {$a}'; $string['deletedefaultsconfirmation'] = 'Are you sure you want to delete the default category and purpose for {$a} modules?'; $string['deleteexpiredcontextstask'] = 'Delete expired contexts'; +$string['deleteexistingdeleteduserstask'] = 'Create delete data request for pre-existing deleted users'; $string['deleteexpireddatarequeststask'] = 'Delete files from completed data requests that have expired'; $string['deletemyaccount'] = 'Delete my account'; $string['deletepurpose'] = 'Delete purpose'; diff --git a/admin/tool/dataprivacy/tests/api_test.php b/admin/tool/dataprivacy/tests/api_test.php index 0dd04ad18a8..c291cd2b826 100644 --- a/admin/tool/dataprivacy/tests/api_test.php +++ b/admin/tool/dataprivacy/tests/api_test.php @@ -607,7 +607,6 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $datarequest = api::create_data_request($user->id, api::DATAREQUEST_TYPE_EXPORT, $comment); $this->assertEquals($user->id, $datarequest->get('userid')); $this->assertEquals($USER->id, $datarequest->get('requestedby')); - $this->assertEquals($USER->id, $datarequest->get('dpo')); $this->assertEquals(api::DATAREQUEST_TYPE_EXPORT, $datarequest->get('type')); $this->assertEquals(api::DATAREQUEST_STATUS_PENDING, $datarequest->get('status')); $this->assertEquals($comment, $datarequest->get('comments')); diff --git a/admin/tool/dataprivacy/version.php b/admin/tool/dataprivacy/version.php index da69f2b4a01..2518a2e0ad6 100644 --- a/admin/tool/dataprivacy/version.php +++ b/admin/tool/dataprivacy/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die; -$plugin->version = 2017051553; +$plugin->version = 2017051554; $plugin->requires = 2017051507.00; // Moodle 3.3.7 (Build: 20180517) and upwards. $plugin->component = 'tool_dataprivacy';