From 37e10692892a83f673280a12b217a5ef81ccc666 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 15 May 2018 10:22:23 +0800 Subject: [PATCH 1/2] MDL-62426 enrol_flatfile: make provider a plugin provider The enrol_flatfile table can contain userdata relating to pending enrolments, so this should be exported and deleted as necessary. --- enrol/flatfile/classes/privacy/provider.php | 155 +++++++++++- enrol/flatfile/lang/en/enrol_flatfile.php | 10 +- .../flatfile/tests/privacy_provider_test.php | 239 ++++++++++++++++++ 3 files changed, 396 insertions(+), 8 deletions(-) create mode 100644 enrol/flatfile/tests/privacy_provider_test.php diff --git a/enrol/flatfile/classes/privacy/provider.php b/enrol/flatfile/classes/privacy/provider.php index 5e403cde6bf..e6987da771c 100644 --- a/enrol/flatfile/classes/privacy/provider.php +++ b/enrol/flatfile/classes/privacy/provider.php @@ -21,6 +21,13 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ namespace enrol_flatfile\privacy; +use core_privacy\local\metadata\collection; +use core_privacy\local\request\approved_contextlist; +use core_privacy\local\request\context; +use core_privacy\local\request\contextlist; +use core_privacy\local\request\writer; +use core_privacy\local\request\transform; + defined('MOODLE_INTERNAL') || die(); /** * Privacy Subsystem for enrol_flatfile implementing null_provider. @@ -28,14 +35,148 @@ defined('MOODLE_INTERNAL') || die(); * @copyright 2018 Carlos Escobedo * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class provider implements \core_privacy\local\metadata\null_provider { +class provider implements + \core_privacy\local\metadata\provider, + \core_privacy\local\request\plugin\provider { + /** - * Get the language string identifier with the component's language - * file to explain why this plugin stores no data. + * Returns meta data about this system. * - * @return string + * @param collection $collection The initialised collection to add items to. + * @return collection A listing of user data stored through this system. */ - public static function get_reason() : string { - return 'privacy:metadata'; + public static function get_metadata(collection $collection) : collection { + return $collection->add_database_table('enrol_flatfile', [ + 'action' => 'privacy:metadata:enrol_flatfile:action', + 'roleid' => 'privacy:metadata:enrol_flatfile:roleid', + 'userid' => 'privacy:metadata:enrol_flatfile:userid', + 'courseid' => 'privacy:metadata:enrol_flatfile:courseid', + 'timestart' => 'privacy:metadata:enrol_flatfile:timestart', + 'timeend' => 'privacy:metadata:enrol_flatfile:timeend', + 'timemodified' => 'privacy:metadata:enrol_flatfile:timemodified' + ], 'privacy:metadata:enrol_flatfile'); } -} \ No newline at end of file + + /** + * Get the list of contexts that contain user information for the specified user. + * + * @param int $userid The user to search. + * @return contextlist $contextlist The contextlist containing the list of contexts used in this plugin. + */ + public static function get_contexts_for_userid(int $userid) : contextlist { + $sql = "SELECT c.id + FROM {enrol_flatfile} ef + JOIN {context} c ON c.contextlevel = ? AND c.instanceid = ef.courseid + WHERE ef.userid = ?"; + $params = [CONTEXT_COURSE, $userid]; + + $contextlist = new contextlist(); + $contextlist->set_component('enrol_flatfile'); + return $contextlist->add_from_sql($sql, $params); + } + + /** + * Export all user data for the specified user, in the specified contexts. + * + * @param approved_contextlist $contextlist The approved contexts to export information for. + */ + public static function export_user_data(approved_contextlist $contextlist) { + global $DB; + + // Ensure all contexts are CONTEXT_COURSE. + $contexts = static::validate_contextlist_contexts($contextlist); + if (empty($contexts)) { + return; + } + + // Get the context instance ids from the contexts. These are the course ids.. + $contextinstanceids = array_map(function($context) { + return $context->instanceid; + }, $contexts); + $userid = $contextlist->get_user()->id; + + // Now, we just need to fetch and format all entries corresponding to the contextids provided. + $sql = "SELECT ef.action, r.shortname, ef.courseid, ef.timestart, ef.timeend, ef.timemodified + FROM {enrol_flatfile} ef + JOIN {context} c ON c.contextlevel = :contextlevel AND c.instanceid = ef.courseid + JOIN {role} r ON r.id = ef.roleid + WHERE ef.userid = :userid"; + $params = ['contextlevel' => CONTEXT_COURSE, 'userid' => $userid]; + list($insql, $inparams) = $DB->get_in_or_equal($contextinstanceids, SQL_PARAMS_NAMED); + $sql .= " AND ef.courseid $insql"; + $params = array_merge($params, $inparams); + + $futureenrolments = $DB->get_recordset_sql($sql, $params); + $enrolmentdata = []; + foreach ($futureenrolments as $futureenrolment) { + // It's possible to have more than one future enrolment per course. + $futureenrolment->timestart = transform::datetime($futureenrolment->timestart); + $futureenrolment->timeend = transform::datetime($futureenrolment->timeend); + $futureenrolment->timemodified = transform::datetime($futureenrolment->timemodified); + $enrolmentdata[$futureenrolment->courseid][] = $futureenrolment; + } + $futureenrolments->close(); + + // And finally, write out the data to the relevant course contexts. + $subcontext = [get_string('flatfileenrolments', 'enrol_flatfile')]; + foreach ($enrolmentdata as $courseid => $enrolments) { + $data = (object) [ + 'pendingenrolments' => $enrolments, + ]; + writer::with_context(\context_course::instance($courseid))->export_data($subcontext, $data); + } + } + + /** + * Delete all data for all users in the specified context. + * + * @param \context $context The specific context to delete data for. + */ + public static function delete_data_for_all_users_in_context(\context $context) { + if ($context->contextlevel != CONTEXT_COURSE) { + return; + } + global $DB; + $DB->delete_records('enrol_flatfile', ['courseid' => $context->instanceid]); + } + + /** + * Delete all user data for the specified user, in the specified contexts. + * + * @param approved_contextlist $contextlist The approved contexts and user information to delete information for. + */ + public static function delete_data_for_user(approved_contextlist $contextlist) { + // Only delete data from contexts which are at the COURSE_MODULE contextlevel. + $contexts = self::validate_contextlist_contexts($contextlist); + if (empty($contexts)) { + return; + } + + // Get the course ids based on the provided contexts. + $contextinstanceids = array_map(function($context) { + return $context->instanceid; + }, $contextlist->get_contexts()); + + global $DB; + $user = $contextlist->get_user(); + list($insql, $inparams) = $DB->get_in_or_equal($contextinstanceids, SQL_PARAMS_NAMED); + $params = array_merge(['userid' => $user->id], $inparams); + $sql = "userid = :userid AND courseid $insql"; + $DB->delete_records_select('enrol_flatfile', $sql, $params); + } + + /** + * Simple sanity check on the contextlist contexts, making sure they're of CONTEXT_COURSE contextlevel. + * + * @param approved_contextlist $contextlist + * @return array the array of contexts filtered to only include those of CONTEXT_COURSE contextlevel. + */ + protected static function validate_contextlist_contexts(approved_contextlist $contextlist) { + return array_reduce($contextlist->get_contexts(), function($carry, $context) { + if ($context->contextlevel == CONTEXT_COURSE) { + $carry[] = $context; + } + return $carry; + }, []); + } +} diff --git a/enrol/flatfile/lang/en/enrol_flatfile.php b/enrol/flatfile/lang/en/enrol_flatfile.php index 6f444e3bdf2..e815bf9ceb7 100644 --- a/enrol/flatfile/lang/en/enrol_flatfile.php +++ b/enrol/flatfile/lang/en/enrol_flatfile.php @@ -29,6 +29,7 @@ $string['filelockedmail'] = 'The text file you are using for file-based enrolmen $string['filelockedmailsubject'] = 'Important error: Enrolment file'; $string['flatfile:manage'] = 'Manage user enrolments manually'; $string['flatfile:unenrol'] = 'Unenrol users from the course manually'; +$string['flatfileenrolments'] = 'Flat file (CSV) enrolments'; $string['flatfilesync'] = 'Flat file enrolment sync'; $string['location'] = 'File location'; $string['location_desc'] = 'Specify full path to the enrolment file. The file is automatically deleted after processing.'; @@ -61,4 +62,11 @@ It could look something like this: del, student, 17, CF101 add, student, 21, CF101, 1091115000, 1091215000 '; -$string['privacy:metadata'] = 'The Flat file (CSV) enrolment plugin does not store any personal data.'; +$string['privacy:metadata:enrol_flatfile'] = 'The Flat file (CSV) enrolment plugin may store personal data relating to future enrolments in the enrol_flatfile table.'; +$string['privacy:metadata:enrol_flatfile:action'] = 'The enrolment action expected at the given date.'; +$string['privacy:metadata:enrol_flatfile:courseid'] = 'The courseid to which the enrolment relates.'; +$string['privacy:metadata:enrol_flatfile:roleid'] = 'The id of the role to be assigned or revoked.'; +$string['privacy:metadata:enrol_flatfile:timestart'] = 'The time at which the enrolment change starts.'; +$string['privacy:metadata:enrol_flatfile:timeend'] = 'The time at which the enrolment change ends.'; +$string['privacy:metadata:enrol_flatfile:timemodified'] = 'The modification time of this enrolment change.'; +$string['privacy:metadata:enrol_flatfile:userid'] = 'The id of the user to which the role assignment relates.'; diff --git a/enrol/flatfile/tests/privacy_provider_test.php b/enrol/flatfile/tests/privacy_provider_test.php new file mode 100644 index 00000000000..6f4d188b0c1 --- /dev/null +++ b/enrol/flatfile/tests/privacy_provider_test.php @@ -0,0 +1,239 @@ +. + +/** + * Privacy tests for enrol_flatfile. + * + * @package enrol_flatfile + * @category test + * @copyright 2018 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +use core_privacy\local\metadata\collection; +use core_privacy\tests\provider_testcase; +use core_privacy\local\request\approved_contextlist; +use core_privacy\local\request\writer; +use enrol_flatfile\privacy\provider; + +/** + * Privacy tests for enrol_flatfile. + * + * @copyright 2018 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class enrol_flatfile_privacy_testcase extends provider_testcase { + + /** @var \stdClass $user1 a test user.*/ + protected $user1; + + /** @var \stdClass $user2 a test user.*/ + protected $user2; + + /** @var \context $coursecontext1 a course context.*/ + protected $coursecontext1; + + /** @var \context $coursecontext2 a course context.*/ + protected $coursecontext2; + + /** @var \context $coursecontext3 a course context.*/ + protected $coursecontext3; + + /** + * Called before every test. + */ + public function setUp() { + $this->resetAfterTest(true); + } + + /** + * Verify that get_metadata returns the database table mapping. + */ + public function test_get_metadata() { + $collection = new collection('enrol_flatfile'); + $collection = provider::get_metadata($collection); + $collectiondata = $collection->get_collection(); + $this->assertNotEmpty($collectiondata); + $this->assertInstanceOf(\core_privacy\local\metadata\types\database_table::class, $collectiondata[0]); + } + + /** + * Verify that the relevant course contexts are returned for users with pending enrolment records. + */ + public function test_get_contexts_for_user() { + global $DB; + // Create, via flatfile syncing, the future enrolments entries in the enrol_flatfile table. + $this->create_future_enrolments(); + + $this->assertEquals(3, $DB->count_records('enrol_flatfile')); + + // We expect to see 2 entries for user1, in course1 and course3. + $contextlist = provider::get_contexts_for_userid($this->user1->id); + $this->assertEquals(2, $contextlist->count()); + $contextids = $contextlist->get_contextids(); + $this->assertContains($this->coursecontext1->id, $contextids); + $this->assertContains($this->coursecontext3->id, $contextids); + + // And 1 for user2 on course2. + $contextlist = provider::get_contexts_for_userid($this->user2->id); + $this->assertEquals(1, $contextlist->count()); + $contextids = $contextlist->get_contextids(); + $this->assertContains($this->coursecontext2->id, $contextids); + } + + /** + * Verify the export includes any future enrolment records for the user. + */ + public function test_export_user_data() { + // Create, via flatfile syncing, the future enrolments entries in the enrol_flatfile table. + $this->create_future_enrolments(); + + // Get contexts containing user data. + $contextlist = provider::get_contexts_for_userid($this->user1->id); + $this->assertEquals(2, $contextlist->count()); + + $approvedcontextlist = new approved_contextlist( + $this->user1, + 'enrol_flatfile', + $contextlist->get_contextids() + ); + + // Export for the approved contexts. + provider::export_user_data($approvedcontextlist); + + // Verify we see one future course enrolment in course1, and one in course3. + $writer = writer::with_context($this->coursecontext1); + $this->assertNotEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + + $writer = writer::with_context($this->coursecontext3); + $this->assertNotEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + + // Verify we have nothing in course 2 for this user. + $writer = writer::with_context($this->coursecontext2); + $this->assertEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + } + + /** + * Verify export will limit any future enrolment records to only those contextids provided. + */ + public function test_export_user_data_restricted_context_subset() { + // Create, via flatfile syncing, the future enrolments entries in the enrol_flatfile table. + $this->create_future_enrolments(); + + // Now, limit the export scope to just course1's context and verify only that data is seen in any export. + $subsetapprovedcontextlist = new approved_contextlist( + $this->user1, + 'enrol_flatfile', + [$this->coursecontext1->id] + ); + + // Export for the approved contexts. + provider::export_user_data($subsetapprovedcontextlist); + + // Verify we see one future course enrolment in course1 only. + $writer = writer::with_context($this->coursecontext1); + $this->assertNotEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + + // And nothing in the course3 context. + $writer = writer::with_context($this->coursecontext3); + $this->assertEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + } + + /** + * Verify that records can be deleted by context. + */ + public function test_delete_data_for_all_users_in_context() { + global $DB; + // Create, via flatfile syncing, the future enrolments entries in the enrol_flatfile table. + $this->create_future_enrolments(); + + // Verify we have 1 future enrolments for course 1. + $this->assertEquals(1, $DB->count_records('enrol_flatfile', ['courseid' => $this->coursecontext1->instanceid])); + + // Now, run delete by context and confirm that record is removed. + provider::delete_data_for_all_users_in_context($this->coursecontext1); + $this->assertEquals(0, $DB->count_records('enrol_flatfile', ['courseid' => $this->coursecontext1->instanceid])); + } + + public function test_delete_data_for_user() { + global $DB; + // Create, via flatfile syncing, the future enrolments entries in the enrol_flatfile table. + $this->create_future_enrolments(); + + // Verify we have 2 future enrolments for course 1 and course 3. + $contextlist = provider::get_contexts_for_userid($this->user1->id); + $this->assertEquals(2, $contextlist->count()); + $contextids = $contextlist->get_contextids(); + $this->assertContains($this->coursecontext1->id, $contextids); + $this->assertContains($this->coursecontext3->id, $contextids); + + $approvedcontextlist = new approved_contextlist( + $this->user1, + 'enrol_flatfile', + $contextids + ); + + // Now, run delete for user and confirm that both records are removed. + provider::delete_data_for_user($approvedcontextlist); + $contextlist = provider::get_contexts_for_userid($this->user1->id); + $this->assertEquals(0, $contextlist->count()); + $this->assertEquals(0, $DB->count_records('enrol_flatfile', ['userid' => $this->user1->id])); + } + + /** + * Helper to sync a file and create the enrol_flatfile DB entries, for use with the get, export and delete tests. + */ + protected function create_future_enrolments() { + global $CFG; + $this->user1 = $this->getDataGenerator()->create_user(['idnumber' => 'u1']); + $this->user2 = $this->getDataGenerator()->create_user(['idnumber' => 'u2']); + + $course1 = $this->getDataGenerator()->create_course(['idnumber' => 'c1']); + $course2 = $this->getDataGenerator()->create_course(['idnumber' => 'c2']); + $course3 = $this->getDataGenerator()->create_course(['idnumber' => 'c3']); + $this->coursecontext1 = context_course::instance($course1->id); + $this->coursecontext2 = context_course::instance($course2->id); + $this->coursecontext3 = context_course::instance($course3->id); + + $now = time(); + $future = $now + 60 * 60 * 5; + $farfuture = $now + 60 * 60 * 24 * 5; + + $file = "$CFG->dataroot/enrol.txt"; + $data = "add,student,u1,c1,$future,0 + add,student,u2,c2,$future,0 + add,student,u1,c3,$future,$farfuture"; + file_put_contents($file, $data); + + $trace = new null_progress_trace(); + $this->enable_plugin(); + $flatfileplugin = enrol_get_plugin('flatfile'); + $flatfileplugin->set_config('location', $file); + $flatfileplugin->sync($trace); + } + + /** + * Enables the flatfile plugin for testing. + */ + protected function enable_plugin() { + $enabled = enrol_get_plugins(true); + $enabled['flatfile'] = true; + $enabled = array_keys($enabled); + set_config('enrol_plugins_enabled', implode(',', $enabled)); + } +} From 72580c083283d0e305d336f0faec1a50901e0c01 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 16 May 2018 10:17:26 +0800 Subject: [PATCH 2/2] MDL-62426 core_enrol: control enrolment subcontexts at the provider Added a function to the core_enrol provider allowing enrol plugins to set their subcontext relative to a fixed 'enrolments' parent dir. --- enrol/classes/privacy/provider.php | 12 ++++++++++++ enrol/flatfile/classes/privacy/provider.php | 2 +- enrol/flatfile/tests/privacy_provider_test.php | 14 +++++++++----- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/enrol/classes/privacy/provider.php b/enrol/classes/privacy/provider.php index 46a0c6309ba..0b5533232e5 100644 --- a/enrol/classes/privacy/provider.php +++ b/enrol/classes/privacy/provider.php @@ -230,4 +230,16 @@ class provider implements $DB->delete_records_select('user_enrolments', "id $sql", $params); } + /** + * Get the subcontext for export. + * + * @param array $subcontext Any additional subcontext to use. + * @return array The array containing the full subcontext, i.e. [enrolments, subcontext] + */ + public static function get_subcontext(array $subcontext) { + return array_merge( + [get_string('privacy:metadata:user_enrolments', 'core_enrol')], + $subcontext + ); + } } diff --git a/enrol/flatfile/classes/privacy/provider.php b/enrol/flatfile/classes/privacy/provider.php index e6987da771c..93e5d831791 100644 --- a/enrol/flatfile/classes/privacy/provider.php +++ b/enrol/flatfile/classes/privacy/provider.php @@ -118,7 +118,7 @@ class provider implements $futureenrolments->close(); // And finally, write out the data to the relevant course contexts. - $subcontext = [get_string('flatfileenrolments', 'enrol_flatfile')]; + $subcontext = \core_enrol\privacy\provider::get_subcontext([get_string('pluginname', 'enrol_flatfile')]); foreach ($enrolmentdata as $courseid => $enrolments) { $data = (object) [ 'pendingenrolments' => $enrolments, diff --git a/enrol/flatfile/tests/privacy_provider_test.php b/enrol/flatfile/tests/privacy_provider_test.php index 6f4d188b0c1..5ad5dbbf175 100644 --- a/enrol/flatfile/tests/privacy_provider_test.php +++ b/enrol/flatfile/tests/privacy_provider_test.php @@ -117,15 +117,17 @@ class enrol_flatfile_privacy_testcase extends provider_testcase { provider::export_user_data($approvedcontextlist); // Verify we see one future course enrolment in course1, and one in course3. + $subcontext = \core_enrol\privacy\provider::get_subcontext([get_string('pluginname', 'enrol_flatfile')]); + $writer = writer::with_context($this->coursecontext1); - $this->assertNotEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + $this->assertNotEmpty($writer->get_data($subcontext)); $writer = writer::with_context($this->coursecontext3); - $this->assertNotEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + $this->assertNotEmpty($writer->get_data($subcontext)); // Verify we have nothing in course 2 for this user. $writer = writer::with_context($this->coursecontext2); - $this->assertEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + $this->assertEmpty($writer->get_data($subcontext)); } /** @@ -146,12 +148,14 @@ class enrol_flatfile_privacy_testcase extends provider_testcase { provider::export_user_data($subsetapprovedcontextlist); // Verify we see one future course enrolment in course1 only. + $subcontext = \core_enrol\privacy\provider::get_subcontext([get_string('pluginname', 'enrol_flatfile')]); + $writer = writer::with_context($this->coursecontext1); - $this->assertNotEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + $this->assertNotEmpty($writer->get_data($subcontext)); // And nothing in the course3 context. $writer = writer::with_context($this->coursecontext3); - $this->assertEmpty($writer->get_data([get_string('flatfileenrolments', 'enrol_flatfile')])); + $this->assertEmpty($writer->get_data($subcontext)); } /**