MDL-62409 tool_dataprivacy: Properly validate data request creation
authorJun Pataleta <jun@moodle.com>
Sat, 12 May 2018 02:18:04 +0000 (10:18 +0800)
committerDavid Monllao <davidm@moodle.com>
Sun, 13 May 2018 14:03:21 +0000 (16:03 +0200)
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
admin/tool/dataprivacy/classes/task/initiate_data_request_task.php
admin/tool/dataprivacy/classes/task/process_data_request_task.php
admin/tool/dataprivacy/lang/en/tool_dataprivacy.php
admin/tool/dataprivacy/tests/api_test.php

index a6d5ab5..7bffc9f 100644 (file)
@@ -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.
      *
index 4e62b74..0cebeb0 100644 (file)
@@ -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);
index bacf29a..589ef01 100644 (file)
@@ -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 message.
-            if ($emailonly) {
-                email_to_user($requestedby, $dpo, $subject, $message->fullmessage, $messagehtml);
-            } else {
-                message_send($message);
+        // 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);
+                }
+                mtrace('Message sent to requester: ' . $messagetextdata['username']);
             }
-            mtrace('Message sent to requester: ' . $messagetextdata['username']);
         }
 
         if ($request->type == api::DATAREQUEST_TYPE_DELETE) {
index c4461f5..3d4dcf4 100644 (file)
@@ -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.';
index 2589607..6e06478 100644 (file)
@@ -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);