Skip to content

Commit

Permalink
MDL-62409 tool_dataprivacy: Properly validate data request creation
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@junpataleta
junpataleta authored and David Monllao committed May 13, 2018
1 parent 0f7fb98 commit 7bdb9d8
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 18 deletions.
36 changes: 34 additions & 2 deletions admin/tool/dataprivacy/classes/api.php
View file
Expand Up @@ -187,8 +187,25 @@ public static function create_data_request($foruser, $type, $comments = '') {
$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.
Expand Down Expand Up @@ -304,17 +321,19 @@ public static function is_active($status) {
* @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();
}

Expand Down Expand Up @@ -468,6 +487,19 @@ public static function notify_dpo($dpo, data_request $request) {
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.
*
Expand Down
21 changes: 21 additions & 0 deletions admin/tool/dataprivacy/classes/task/initiate_data_request_task.php
View file
Expand Up @@ -70,6 +70,27 @@ public function execute() {
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);
Expand Down
36 changes: 20 additions & 16 deletions admin/tool/dataprivacy/classes/task/process_data_request_task.php
View file
Expand Up @@ -182,23 +182,27 @@ public function execute() {
}
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) {
Expand Down
1 change: 1 addition & 0 deletions admin/tool/dataprivacy/lang/en/tool_dataprivacy.php
View file
Expand Up @@ -89,6 +89,7 @@
$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.';
Expand Down
72 changes: 72 additions & 0 deletions admin/tool/dataprivacy/tests/api_test.php
View file
Expand Up @@ -57,6 +57,7 @@ public function setUp() {
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);
Expand Down Expand Up @@ -145,6 +146,7 @@ public function test_approve_data_request() {
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');

Expand Down Expand Up @@ -186,6 +188,7 @@ public function test_approve_data_request_not_yet_ready() {
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');

Expand All @@ -203,6 +206,7 @@ public function test_approve_data_request_non_dpo_user() {
$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');
Expand Down Expand Up @@ -286,6 +290,71 @@ public function test_create_data_request() {
$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'));
Expand Down Expand Up @@ -351,8 +420,10 @@ public function test_get_data_requests() {
$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.
Expand Down Expand Up @@ -406,6 +477,7 @@ public function test_has_ongoing_request($status, $expected) {
$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);
Expand Down

0 comments on commit 7bdb9d8

Please sign in to comment.