From 7bdb9d877d12a7e16e6c30a288f91678dde26eec Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Sat, 12 May 2018 10:18:04 +0800 Subject: [PATCH] MDL-62409 tool_dataprivacy: Properly validate data request creation Creating data requests * Add capability check when creating data requests for another user. Ad-hoc task that processes pending data requests * Check if the requesting user has the capability to create the data request for another user. Reject otherwise. Ad-hoc task that processes approved data requests * Validate that the requester can receive the notification about the data request processing results. * Do not send the confirmation link to DPOs/admins --- admin/tool/dataprivacy/classes/api.php | 36 +++++++++- .../task/initiate_data_request_task.php | 21 ++++++ .../task/process_data_request_task.php | 34 +++++---- .../dataprivacy/lang/en/tool_dataprivacy.php | 1 + admin/tool/dataprivacy/tests/api_test.php | 72 +++++++++++++++++++ 5 files changed, 147 insertions(+), 17 deletions(-) diff --git a/admin/tool/dataprivacy/classes/api.php b/admin/tool/dataprivacy/classes/api.php index a6d5ab51d3a..7bffc9f8404 100644 --- a/admin/tool/dataprivacy/classes/api.php +++ b/admin/tool/dataprivacy/classes/api.php @@ -187,8 +187,25 @@ class api { $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); + } else { + // If not a DPO, only users with the capability to make data requests for the user should be allowed. + // (e.g. users with the Parent role, etc). + if (!api::can_create_data_request_for_user($foruser)) { + $forusercontext = \context_user::instance($foruser); + throw new required_capability_exception($forusercontext, + 'tool/dataprivacy:makedatarequestsforchildren', 'nopermissions', ''); + } + } + } // The user making the request. - $datarequest->set('requestedby', $USER->id); + $datarequest->set('requestedby', $requestinguser); // Set status. $datarequest->set('status', self::DATAREQUEST_STATUS_PENDING); // Set request type. @@ -304,17 +321,19 @@ class api { * @param int $requestid The request identifier. * @param int $status The request status. * @param int $dpoid The user ID of the Data Protection Officer + * @param string $comment The comment about the status update. * @return bool * @throws invalid_persistent_exception * @throws coding_exception */ - public static function update_request_status($requestid, $status, $dpoid = 0) { + public static function update_request_status($requestid, $status, $dpoid = 0, $comment = '') { // Update the request. $datarequest = new data_request($requestid); $datarequest->set('status', $status); if ($dpoid) { $datarequest->set('dpo', $dpoid); } + $datarequest->set('dpocomment', $comment); return $datarequest->update(); } @@ -468,6 +487,19 @@ class api { return message_send($message); } + /** + * Checks whether a non-DPO user can make a data request for another user. + * + * @param int $user The user ID of the target user. + * @param int $requester The user ID of the user making the request. + * @return bool + * @throws coding_exception + */ + public static function can_create_data_request_for_user($user, $requester = null) { + $usercontext = \context_user::instance($user); + return has_capability('tool/dataprivacy:makedatarequestsforchildren', $usercontext, $requester); + } + /** * Creates a new data purpose. * 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 4e62b74fa7e..0cebeb0caaf 100644 --- a/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/initiate_data_request_task.php @@ -70,6 +70,27 @@ 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 bacf29aa989..589ef01accd 100644 --- a/admin/tool/dataprivacy/classes/task/process_data_request_task.php +++ b/admin/tool/dataprivacy/classes/task/process_data_request_task.php @@ -182,23 +182,27 @@ class process_data_request_task extends adhoc_task { } mtrace('Message sent to user: ' . $messagetextdata['username']); - // Send to the requester as well. requestedby is 0 if the request was made on behalf of the user by a DPO. - if (!empty($request->requestedby) && $foruser->id != $request->requestedby) { - $requestedby = core_user::get_user($request->requestedby); - $message->userto = $requestedby; - $messagetextdata['username'] = fullname($requestedby); - // Render message email body. - $messagehtml = $output->render_from_template('tool_dataprivacy/data_request_results_email', $messagetextdata); - $message->fullmessage = html_to_text($messagehtml); - $message->fullmessagehtml = $messagehtml; + // Send to requester as well if this request was made on behalf of another user who's not a DPO, + // and has the capability to make data requests for the user (e.g. Parent). + if (!api::is_site_dpo($request->requestedby) && $foruser->id != $request->requestedby) { + // Ensure the requester has the capability to make data requests for this user. + if (api::can_create_data_request_for_user($request->userid, $request->requestedby)) { + $requestedby = core_user::get_user($request->requestedby); + $message->userto = $requestedby; + $messagetextdata['username'] = fullname($requestedby); + // Render message email body. + $messagehtml = $output->render_from_template('tool_dataprivacy/data_request_results_email', $messagetextdata); + $message->fullmessage = html_to_text($messagehtml); + $message->fullmessagehtml = $messagehtml; - // Send message. - if ($emailonly) { - email_to_user($requestedby, $dpo, $subject, $message->fullmessage, $messagehtml); - } else { - message_send($message); + // Send message. + if ($emailonly) { + email_to_user($requestedby, $dpo, $subject, $message->fullmessage, $messagehtml); + } else { + message_send($message); + } + mtrace('Message sent to requester: ' . $messagetextdata['username']); } - mtrace('Message sent to requester: ' . $messagetextdata['username']); } if ($request->type == api::DATAREQUEST_TYPE_DELETE) { diff --git a/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php b/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php index c4461f59888..3d4dcf49402 100644 --- a/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php +++ b/admin/tool/dataprivacy/lang/en/tool_dataprivacy.php @@ -89,6 +89,7 @@ $string['effectiveretentionperioduser'] = '{$a} (since the last time the user ac $string['emailsalutation'] = 'Dear {$a},'; $string['errorinvalidrequeststatus'] = 'Invalid request status!'; $string['errorinvalidrequesttype'] = 'Invalid request type!'; +$string['errornocapabilitytorequestforothers'] = 'User {$a->requestedby} doesn\'t have the capability to make a data request on behalf of user {$a->userid}'; $string['errornoexpiredcontexts'] = 'There are no expired contexts to process'; $string['errorcontexthasunexpiredchildren'] = 'The context "{$a}" still has sub-contexts that have not yet expired. No contexts have been flagged for deletion.'; $string['errorrequestalreadyexists'] = 'You already have an ongoing request.'; diff --git a/admin/tool/dataprivacy/tests/api_test.php b/admin/tool/dataprivacy/tests/api_test.php index 2589607b161..6e0647878f0 100644 --- a/admin/tool/dataprivacy/tests/api_test.php +++ b/admin/tool/dataprivacy/tests/api_test.php @@ -57,6 +57,7 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { public function test_update_request_status() { $generator = new testing_data_generator(); $s1 = $generator->create_user(); + $this->setUser($s1); // Create the sample data request. $datarequest = api::create_data_request($s1->id, api::DATAREQUEST_TYPE_EXPORT); @@ -145,6 +146,7 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { set_config('dporoles', $managerroleid, 'tool_dataprivacy'); // Create the sample data request. + $this->setUser($s1); $datarequest = api::create_data_request($s1->id, api::DATAREQUEST_TYPE_EXPORT); $requestid = $datarequest->get('id'); @@ -186,6 +188,7 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { set_config('dporoles', $managerroleid, 'tool_dataprivacy'); // Create the sample data request. + $this->setUser($s1); $datarequest = api::create_data_request($s1->id, api::DATAREQUEST_TYPE_EXPORT); $requestid = $datarequest->get('id'); @@ -203,6 +206,7 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $teacher = $generator->create_user(); // Create the sample data request. + $this->setUser($student); $datarequest = api::create_data_request($student->id, api::DATAREQUEST_TYPE_EXPORT); $requestid = $datarequest->get('id'); @@ -286,6 +290,71 @@ 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(0, $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')); + + // Test adhoc task creation. + $adhoctasks = manager::get_adhoc_tasks(initiate_data_request_task::class); + $this->assertCount(1, $adhoctasks); + } + + /** + * Test for api::create_data_request() made by DPO. + */ + public function test_create_data_request_by_dpo() { + global $USER; + + $generator = new testing_data_generator(); + $user = $generator->create_user(); + $comment = 'sample comment'; + + // Login as DPO (Admin is DPO by default). + $this->setAdminUser(); + + // Test data request creation. + $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')); + + // Test adhoc task creation. + $adhoctasks = manager::get_adhoc_tasks(initiate_data_request_task::class); + $this->assertCount(1, $adhoctasks); + } + + /** + * Test for api::create_data_request() made by a parent. + */ + public function test_create_data_request_by_parent() { + global $DB; + + $generator = new testing_data_generator(); + $user = $generator->create_user(); + $parent = $generator->create_user(); + $comment = 'sample comment'; + + // Get the teacher role pretend it's the parent roles ;). + $systemcontext = context_system::instance(); + $usercontext = context_user::instance($user->id); + $parentroleid = $DB->get_field('role', 'id', array('shortname' => 'teacher')); + // Give the manager role with the capability to manage data requests. + assign_capability('tool/dataprivacy:makedatarequestsforchildren', CAP_ALLOW, $parentroleid, $systemcontext->id, true); + // Assign the parent to user. + role_assign($parentroleid, $parent->id, $usercontext->id); + + // Login as the user's parent. + $this->setUser($parent); + + // Test data request creation. + $datarequest = api::create_data_request($user->id, api::DATAREQUEST_TYPE_EXPORT, $comment); + $this->assertEquals($user->id, $datarequest->get('userid')); + $this->assertEquals($parent->id, $datarequest->get('requestedby')); + $this->assertEquals(0, $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')); @@ -351,8 +420,10 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $comment = 'sample comment'; // Make a data request as user 1. + $this->setUser($user1); $d1 = api::create_data_request($user1->id, api::DATAREQUEST_TYPE_EXPORT, $comment); // Make a data request as user 2. + $this->setUser($user2); $d2 = api::create_data_request($user2->id, api::DATAREQUEST_TYPE_EXPORT, $comment); // Fetching data requests of specific users. @@ -406,6 +477,7 @@ class tool_dataprivacy_api_testcase extends advanced_testcase { $user1 = $generator->create_user(); // Make a data request as user 1. + $this->setUser($user1); $request = api::create_data_request($user1->id, api::DATAREQUEST_TYPE_EXPORT); // Set the status. api::update_request_status($request->get('id'), $status);