From 60987ecfae012bab44aa442173de78da3cae1dbc Mon Sep 17 00:00:00 2001 From: Mihail Geshoski Date: Thu, 8 Nov 2018 16:06:43 +0800 Subject: [PATCH] MDL-63671 tool_cohortroles: Limit data to the system context in provider --- .../cohortroles/classes/privacy/provider.php | 171 +++++++++++---- admin/tool/cohortroles/tests/privacy_test.php | 207 ++++++++++++------ 2 files changed, 267 insertions(+), 111 deletions(-) diff --git a/admin/tool/cohortroles/classes/privacy/provider.php b/admin/tool/cohortroles/classes/privacy/provider.php index 2cdfca6f2ca..3cd324ef7a9 100644 --- a/admin/tool/cohortroles/classes/privacy/provider.php +++ b/admin/tool/cohortroles/classes/privacy/provider.php @@ -80,15 +80,25 @@ class provider implements public static function get_contexts_for_userid(int $userid) : contextlist { $contextlist = new contextlist(); - // Retrieve the User context associated with tool_cohortroles records. - $sql = "SELECT DISTINCT c.id - FROM {context} c - JOIN {tool_cohortroles} cr ON cr.userid = c.instanceid AND c.contextlevel = :contextuser - WHERE cr.userid = :userid"; + // When we process user deletions and expiries, we always delete from the user context. + // As a result the cohort role assignments would be deleted, which has a knock-on effect with courses + // as roles may change and data may be removed earlier than it should be. + + // Retrieve the context associated with tool_cohortroles records. + $sql = "SELECT DISTINCT c.contextid + FROM {tool_cohortroles} tc + JOIN {cohort} c + ON tc.cohortid = c.id + JOIN {context} ctx + ON ctx.id = c.contextid + WHERE tc.userid = :userid + AND (ctx.contextlevel = :contextlevel1 + OR ctx.contextlevel = :contextlevel2)"; $params = [ - 'contextuser' => CONTEXT_USER, - 'userid' => $userid + 'userid' => $userid, + 'contextlevel1' => CONTEXT_SYSTEM, + 'contextlevel2' => CONTEXT_COURSECAT ]; $contextlist->add_from_sql($sql, $params); @@ -104,24 +114,29 @@ class provider implements public static function get_users_in_context(userlist $userlist) { $context = $userlist->get_context(); - // We should process user data from the system context. // When we process user deletions and expiries, we always delete from the user context. // As a result the cohort role assignments would be deleted, which has a knock-on effect with courses // as roles may change and data may be removed earlier than it should be. - if (!$context instanceof \context_system) { + + $allowedcontextlevels = [ + CONTEXT_SYSTEM, + CONTEXT_COURSECAT + ]; + + if (!in_array($context->contextlevel, $allowedcontextlevels)) { return; } - $params = [ - 'contextid' => $context->id - ]; - $sql = "SELECT tc.userid as userid FROM {tool_cohortroles} tc JOIN {cohort} c ON tc.cohortid = c.id WHERE c.contextid = :contextid"; + $params = [ + 'contextid' => $context->id + ]; + $userlist->add_from_sql('userid', $sql, $params); } @@ -133,24 +148,28 @@ class provider implements public static function export_user_data(approved_contextlist $contextlist) { global $DB; - // If the user has tool_cohortroles data, then only the User context should be present so get the first context. - $contexts = $contextlist->get_contexts(); - if (count($contexts) == 0) { - return; - } - $context = reset($contexts); + // Remove contexts different from SYSTEM or COURSECAT. + $contextids = array_reduce($contextlist->get_contexts(), function($carry, $context) { + if ($context->contextlevel == CONTEXT_SYSTEM || $context->contextlevel == CONTEXT_COURSECAT) { + $carry[] = $context->id; + } + return $carry; + }, []); - // Sanity check that context is at the User context level, then get the userid. - if ($context->contextlevel !== CONTEXT_USER) { + if (empty($contextids)) { return; } - $userid = $context->instanceid; + + $userid = $contextlist->get_user()->id; + + list($contextsql, $contextparams) = $DB->get_in_or_equal($contextids, SQL_PARAMS_NAMED); // Retrieve the tool_cohortroles records created for the user. - $sql = 'SELECT cr.id as cohortroleid, + $sql = "SELECT cr.id as cohortroleid, c.name as cohortname, c.idnumber as cohortidnumber, c.description as cohortdescription, + c.contextid as contextid, r.shortname as roleshortname, cr.userid as userid, cr.timecreated as timecreated, @@ -158,13 +177,13 @@ class provider implements FROM {tool_cohortroles} cr JOIN {cohort} c ON c.id = cr.cohortid JOIN {role} r ON r.id = cr.roleid - WHERE cr.userid = :userid'; + WHERE cr.userid = :userid + AND c.contextid {$contextsql}"; - $params = [ - 'userid' => $userid - ]; + $params = ['userid' => $userid] + $contextparams; $cohortroles = $DB->get_records_sql($sql, $params); + foreach ($cohortroles as $cohortrole) { // The tool_cohortroles data export is organised in: // {User Context}/Cohort roles management/{cohort name}/{role shortname}/data.json. @@ -184,6 +203,8 @@ class provider implements 'timemodified' => transform::datetime($cohortrole->timemodified) ]; + $context = \context::instance_by_id($cohortrole->contextid); + writer::with_context($context)->export_data($subcontext, $data); } } @@ -196,14 +217,24 @@ class provider implements public static function delete_data_for_all_users_in_context(\context $context) { global $DB; - // Sanity check that context is at the User context level, then get the userid. - if ($context->contextlevel !== CONTEXT_USER) { + // When we process user deletions and expiries, we always delete from the user context. + // As a result the cohort role assignments would be deleted, which has a knock-on effect with courses + // as roles may change and data may be removed earlier than it should be. + + $allowedcontextlevels = [ + CONTEXT_SYSTEM, + CONTEXT_COURSECAT + ]; + + if (!in_array($context->contextlevel, $allowedcontextlevels)) { return; } - $userid = $context->instanceid; - // Delete the tool_cohortroles records created for the userid. - $DB->delete_records('tool_cohortroles', ['userid' => $userid]); + $cohortids = $DB->get_fieldset_select('cohort', 'id', 'contextid = :contextid', + ['contextid' => $context->id]); + + // Delete the tool_cohortroles records created in the specific context. + $DB->delete_records_list('tool_cohortroles', 'cohortid', $cohortids); } /** @@ -214,15 +245,42 @@ class provider implements public static function delete_data_for_users(approved_userlist $userlist) { global $DB; - $context = $userlist->get_context(); - - // We should process user data from the system context. // When we process user deletions and expiries, we always delete from the user context. // As a result the cohort role assignments would be deleted, which has a knock-on effect with courses // as roles may change and data may be removed earlier than it should be. - if ($context instanceof \context_system) { - $DB->delete_records_list('tool_cohortroles', 'userid', $userlist->get_userids()); + + $userids = $userlist->get_userids(); + + if (empty($userids)) { + return; } + + $context = $userlist->get_context(); + + $allowedcontextlevels = [ + CONTEXT_SYSTEM, + CONTEXT_COURSECAT + ]; + + if (!in_array($context->contextlevel, $allowedcontextlevels)) { + return; + } + + $cohortids = $DB->get_fieldset_select('cohort', 'id', 'contextid = :contextid', + ['contextid' => $context->id]); + + if (empty($cohortids)) { + return; + } + + list($cohortsql, $cohortparams) = $DB->get_in_or_equal($cohortids, SQL_PARAMS_NAMED); + list($usersql, $userparams) = $DB->get_in_or_equal($userids, SQL_PARAMS_NAMED); + + $params = $cohortparams + $userparams; + $select = "cohortid {$cohortsql} AND userid {$usersql}"; + + // Delete the tool_cohortroles records created in the specific context for an approved list of users. + $DB->delete_records_select('tool_cohortroles', $select, $params); } /** @@ -233,21 +291,38 @@ class provider implements public static function delete_data_for_user(approved_contextlist $contextlist) { global $DB; - // If the user has tool_cohortroles data, then only the User context should be present so get the first context. - $contexts = $contextlist->get_contexts(); - if (count($contexts) == 0) { - return; - } - $context = reset($contexts); + // When we process user deletions and expiries, we always delete from the user context. + // As a result the cohort role assignments would be deleted, which has a knock-on effect with courses + // as roles may change and data may be removed earlier than it should be. - // Sanity check that context is at the User context level, then get the userid. - if ($context->contextlevel !== CONTEXT_USER) { + // Remove contexts different from SYSTEM or COURSECAT. + $contextids = array_reduce($contextlist->get_contexts(), function($carry, $context) { + if ($context->contextlevel == CONTEXT_SYSTEM || $context->contextlevel == CONTEXT_COURSECAT) { + $carry[] = $context->id; + } + return $carry; + }, []); + + if (empty($contextids)) { return; } - $userid = $context->instanceid; + + $userid = $contextlist->get_user()->id; + + list($contextsql, $contextparams) = $DB->get_in_or_equal($contextids, SQL_PARAMS_NAMED); + $selectcontext = "contextid {$contextsql}"; + // Get the cohorts in the specified contexts. + $cohortids = $DB->get_fieldset_select('cohort', 'id', $selectcontext, $contextparams); + + if (empty($cohortids)) { + return; + } + + list($cohortsql, $cohortparams) = $DB->get_in_or_equal($cohortids, SQL_PARAMS_NAMED); + $selectcohort = "cohortid {$cohortsql} AND userid = :userid"; + $params = ['userid' => $userid] + $cohortparams; // Delete the tool_cohortroles records created for the userid. - $DB->delete_records('tool_cohortroles', ['userid' => $userid]); + $DB->delete_records_select('tool_cohortroles', $selectcohort, $params); } - } diff --git a/admin/tool/cohortroles/tests/privacy_test.php b/admin/tool/cohortroles/tests/privacy_test.php index 26f99d30bee..6399314cf77 100644 --- a/admin/tool/cohortroles/tests/privacy_test.php +++ b/admin/tool/cohortroles/tests/privacy_test.php @@ -58,22 +58,36 @@ class tool_cohortroles_privacy_testcase extends \core_privacy\tests\provider_tes $this->setUser($user); $this->setAdminUser(); - $nocohortroles = 3; - $this->setup_test_scenario_data($user->id, $nocohortroles); + // Create course category. + $coursecategory = $this->getDataGenerator()->create_category(); + $coursecategoryctx = \context_coursecat::instance($coursecategory->id); + $systemctx = context_system::instance(); + // Create course. + $course = $this->getDataGenerator()->create_course(); + $coursectx = \context_course::instance($course->id); + + $this->setup_test_scenario_data($user->id, $systemctx, 1); + $this->setup_test_scenario_data($user->id, $coursecategoryctx, 1, 'Sausage roll 2', + 'sausageroll2'); + $this->setup_test_scenario_data($user->id, $coursectx, 1, 'Sausage roll 3', + 'sausageroll3'); // Test the User's assigned cohortroles matches 3. $cohortroles = $DB->get_records('tool_cohortroles', ['userid' => $user->id]); - $this->assertCount($nocohortroles, $cohortroles); + $this->assertCount(3, $cohortroles); - // Test the User's retrieved contextlist contains only one context. + // Test the User's retrieved contextlist returns only the system and course category context. $contextlist = provider::get_contexts_for_userid($user->id); $contexts = $contextlist->get_contexts(); - $this->assertCount(1, $contexts); + $this->assertCount(2, $contexts); - // Test the User's contexts equal the User's own context. - $context = reset($contexts); - $this->assertEquals(CONTEXT_USER, $context->contextlevel); - $this->assertEquals($user->id, $context->instanceid); + $contextlevels = array_column($contexts, 'contextlevel'); + $expected = [ + CONTEXT_SYSTEM, + CONTEXT_COURSECAT + ]; + // Test the User's contexts equal the system and course category context. + $this->assertEquals($expected, $contextlevels, '', 0, 10, true); } /** @@ -85,26 +99,45 @@ class tool_cohortroles_privacy_testcase extends \core_privacy\tests\provider_tes $this->setUser($user); $this->setAdminUser(); - $nocohortroles = 3; - $this->setup_test_scenario_data($user->id, $nocohortroles); + // Create course category. + $coursecategory = $this->getDataGenerator()->create_category(); + $coursecategoryctx = \context_coursecat::instance($coursecategory->id); + $systemctx = context_system::instance(); + // Create course. + $course = $this->getDataGenerator()->create_course(); + $coursectx = \context_course::instance($course->id); - // Test the User's retrieved contextlist contains only one context. + $this->setup_test_scenario_data($user->id, $systemctx, 1); + $this->setup_test_scenario_data($user->id, $coursecategoryctx, 1, 'Sausage roll 2', + 'sausageroll2'); + $this->setup_test_scenario_data($user->id, $coursectx, 1, 'Sausage roll 3', + 'sausageroll3'); + + // Test the User's retrieved contextlist contains two contexts. $contextlist = provider::get_contexts_for_userid($user->id); $contexts = $contextlist->get_contexts(); - $this->assertCount(1, $contexts); + $this->assertCount(2, $contexts); - // Test the User's contexts equal the User's own context. - $context = reset($contexts); - $this->assertEquals(CONTEXT_USER, $context->contextlevel); - $this->assertEquals($user->id, $context->instanceid); + // Add a system, course category and course context to the approved context list. + $approvedcontextids = [ + $systemctx->id, + $coursecategoryctx->id, + $coursectx->id + ]; // Retrieve the User's tool_cohortroles data. - $approvedcontextlist = new approved_contextlist($user, 'tool_cohortroles', $contextlist->get_contextids()); + $approvedcontextlist = new approved_contextlist($user, 'tool_cohortroles', $approvedcontextids); provider::export_user_data($approvedcontextlist); - // Test the tool_cohortroles data is exported at the User context level. - $writer = writer::with_context($context); + // Test the tool_cohortroles data is exported at the system context level. + $writer = writer::with_context($systemctx); $this->assertTrue($writer->has_any_data()); + // Test the tool_cohortroles data is exported at the course category context level. + $writer = writer::with_context($coursecategoryctx); + $this->assertTrue($writer->has_any_data()); + // Test the tool_cohortroles data is not exported at the course context level. + $writer = writer::with_context($coursectx); + $this->assertFalse($writer->has_any_data()); } /** @@ -118,29 +151,52 @@ class tool_cohortroles_privacy_testcase extends \core_privacy\tests\provider_tes $this->setUser($user); $this->setAdminUser(); - $nocohortroles = 4; - $this->setup_test_scenario_data($user->id, $nocohortroles); + // Create course category. + $coursecategory = $this->getDataGenerator()->create_category(); + $coursecategoryctx = \context_coursecat::instance($coursecategory->id); + $systemctx = context_system::instance(); - // Test the User's assigned cohortroles matches 4. + $this->setup_test_scenario_data($user->id, $systemctx, 1); + $this->setup_test_scenario_data($user->id, $coursecategoryctx, 1, 'Sausage roll 2', + 'sausageroll2'); + + // Test the User's assigned cohortroles matches 2. $cohortroles = $DB->get_records('tool_cohortroles', ['userid' => $user->id]); - $this->assertCount($nocohortroles, $cohortroles); + $this->assertCount(2, $cohortroles); - // Test the User's retrieved contextlist contains only one context. + // Test the User's retrieved contextlist contains two contexts. + $contextlist = provider::get_contexts_for_userid($user->id); + $contexts = $contextlist->get_contexts(); + $this->assertCount(2, $contexts); + + // Make sure the user data is only being deleted in within the system and course category context. + $usercontext = context_user::instance($user->id); + // Delete all the User's records in mdl_tool_cohortroles table by the user context. + provider::delete_data_for_all_users_in_context($usercontext); + + // Test the cohort roles records in mdl_tool_cohortroles table is still present. + $cohortroles = $DB->get_records('tool_cohortroles', ['userid' => $user->id]); + $this->assertCount(2, $cohortroles); + + // Delete all the User's records in mdl_tool_cohortroles table by the specified system context. + provider::delete_data_for_all_users_in_context($systemctx); + + // The user data in the system context should be deleted. + // Test the User's retrieved contextlist contains one context (course category). $contextlist = provider::get_contexts_for_userid($user->id); $contexts = $contextlist->get_contexts(); $this->assertCount(1, $contexts); - // Test the User's contexts equal the User's own context. - $context = reset($contexts); - $this->assertEquals(CONTEXT_USER, $context->contextlevel); - $this->assertEquals($user->id, $context->instanceid); - - // Delete all the User's records in mdl_tool_cohortroles table by the specified User context. - provider::delete_data_for_all_users_in_context($context); + // Delete all the User's records in mdl_tool_cohortroles table by the specified course category context. + provider::delete_data_for_all_users_in_context($coursecategoryctx); // Test the cohort roles records in mdl_tool_cohortroles table is equals zero. $cohortroles = $DB->get_records('tool_cohortroles', ['userid' => $user->id]); $this->assertCount(0, $cohortroles); + + $contextlist = provider::get_contexts_for_userid($user->id); + $contexts = $contextlist->get_contexts(); + $this->assertCount(0, $contexts); } /** @@ -154,24 +210,35 @@ class tool_cohortroles_privacy_testcase extends \core_privacy\tests\provider_tes $this->setUser($user); $this->setAdminUser(); - $nocohortroles = 4; - $this->setup_test_scenario_data($user->id, $nocohortroles); + // Create course category. + $coursecategory = $this->getDataGenerator()->create_category(); + $coursecategoryctx = \context_coursecat::instance($coursecategory->id); + $systemctx = context_system::instance(); - // Test the User's assigned cohortroles matches 4. + $this->setup_test_scenario_data($user->id, $systemctx, 1); + $this->setup_test_scenario_data($user->id, $coursecategoryctx, 1, 'Sausage roll 2', + 'sausageroll2'); + + // Test the User's assigned cohortroles matches 2. $cohortroles = $DB->get_records('tool_cohortroles', ['userid' => $user->id]); - $this->assertCount($nocohortroles, $cohortroles); + $this->assertCount(2, $cohortroles); - // Test the User's retrieved contextlist contains only one context. + // Test the User's retrieved contextlist contains two contexts. $contextlist = provider::get_contexts_for_userid($user->id); $contexts = $contextlist->get_contexts(); - $this->assertCount(1, $contexts); + $this->assertCount(2, $contexts); - // Test the User's contexts equal the User's own context. - $context = reset($contexts); - $this->assertEquals(CONTEXT_USER, $context->contextlevel); - $this->assertEquals($user->id, $context->instanceid); + // Make sure the user data is only being deleted in within the system and the course category contexts. + $usercontext = context_user::instance($user->id); + // Delete all the User's records in mdl_tool_cohortroles table by the specified approved context list. + $approvedcontextlist = new approved_contextlist($user, 'tool_cohortroles', [$usercontext->id]); + provider::delete_data_for_user($approvedcontextlist); - // Delete all the User's records in mdl_tool_cohortroles table by the specified User approved context list. + // Test the cohort roles records in mdl_tool_cohortroles table are still present. + $cohortroles = $DB->get_records('tool_cohortroles', ['userid' => $user->id]); + $this->assertCount(2, $cohortroles); + + // Delete all the User's records in mdl_tool_cohortroles table by the specified approved context list. $approvedcontextlist = new approved_contextlist($user, 'tool_cohortroles', $contextlist->get_contextids()); provider::delete_data_for_user($approvedcontextlist); @@ -190,22 +257,32 @@ class tool_cohortroles_privacy_testcase extends \core_privacy\tests\provider_tes $user = $this->getDataGenerator()->create_user(); $usercontext = context_user::instance($user->id); - $systemcontext = context_system::instance(); + // Create course category. + $coursecategory = $this->getDataGenerator()->create_category(); + $coursecategoryctx = \context_coursecat::instance($coursecategory->id); + $systemctx = context_system::instance(); $this->setAdminUser(); - $userlist = new \core_privacy\local\request\userlist($systemcontext, $component); + $userlist = new \core_privacy\local\request\userlist($systemctx, $component); provider::get_users_in_context($userlist); $this->assertCount(0, $userlist); - $nocohortroles = 3; - $this->setup_test_scenario_data($user->id, $nocohortroles); + $this->setup_test_scenario_data($user->id, $systemctx, 1); + $this->setup_test_scenario_data($user->id, $coursecategoryctx, 1, 'Sausage roll 2', + 'sausageroll2'); // The list of users within the system context should contain user. provider::get_users_in_context($userlist); $this->assertCount(1, $userlist); $this->assertTrue(in_array($user->id, $userlist->get_userids())); + // The list of users within the course category context should contain user. + $userlist = new \core_privacy\local\request\userlist($coursecategoryctx, $component); + provider::get_users_in_context($userlist); + $this->assertCount(1, $userlist); + $this->assertTrue(in_array($user->id, $userlist->get_userids())); + // The list of users within the user context should be empty. $userlist2 = new \core_privacy\local\request\userlist($usercontext, $component); provider::get_users_in_context($userlist2); @@ -226,43 +303,44 @@ class tool_cohortroles_privacy_testcase extends \core_privacy\tests\provider_tes $user3 = $this->getDataGenerator()->create_user(); $usercontext3 = context_user::instance($user3->id); - $systemcontext = context_system::instance(); + // Create course category. + $coursecategory = $this->getDataGenerator()->create_category(); + $coursecategoryctx = \context_coursecat::instance($coursecategory->id); + $systemctx = context_system::instance(); $this->setAdminUser(); - $nocohortroles = 3; - $this->setup_test_scenario_data($user1->id, $nocohortroles); - $this->setup_test_scenario_data($user2->id, $nocohortroles, 'Sausage roll 2', + $this->setup_test_scenario_data($user1->id, $systemctx, 1); + $this->setup_test_scenario_data($user2->id, $systemctx, 1, 'Sausage roll 2', 'sausageroll2'); - $this->setup_test_scenario_data($user3->id, $nocohortroles, 'Sausage roll 3', + $this->setup_test_scenario_data($user3->id, $coursecategoryctx, 1, 'Sausage roll 3', 'sausageroll3'); - $userlist1 = new \core_privacy\local\request\userlist($systemcontext, $component); + $userlist1 = new \core_privacy\local\request\userlist($systemctx, $component); provider::get_users_in_context($userlist1); - $this->assertCount(3, $userlist1); + $this->assertCount(2, $userlist1); $this->assertTrue(in_array($user1->id, $userlist1->get_userids())); $this->assertTrue(in_array($user2->id, $userlist1->get_userids())); - $this->assertTrue(in_array($user3->id, $userlist1->get_userids())); // Convert $userlist1 into an approved_contextlist. - $approvedlist1 = new approved_userlist($systemcontext, $component, [$user1->id, $user2->id]); + $approvedlist1 = new approved_userlist($systemctx, $component, [$user1->id]); // Delete using delete_data_for_user. provider::delete_data_for_users($approvedlist1); // Re-fetch users in systemcontext. - $userlist1 = new \core_privacy\local\request\userlist($systemcontext, $component); + $userlist1 = new \core_privacy\local\request\userlist($systemctx, $component); provider::get_users_in_context($userlist1); - // The user data of user1 and user2 in systemcontext should be deleted. - // The user data of user3 in systemcontext should be still present. + // The user data of user1in systemcontext should be deleted. + // The user data of user2 in systemcontext should be still present. $this->assertCount(1, $userlist1); - $this->assertTrue(in_array($user3->id, $userlist1->get_userids())); + $this->assertTrue(in_array($user2->id, $userlist1->get_userids())); // Convert $userlist1 into an approved_contextlist in the user context. $approvedlist2 = new approved_userlist($usercontext3, $component, $userlist1->get_userids()); // Delete using delete_data_for_user. provider::delete_data_for_users($approvedlist2); // Re-fetch users in systemcontext. - $userlist1 = new \core_privacy\local\request\userlist($systemcontext, $component); + $userlist1 = new \core_privacy\local\request\userlist($systemctx, $component); provider::get_users_in_context($userlist1); // The user data in systemcontext should not be deleted. $this->assertCount(1, $userlist1); @@ -278,12 +356,15 @@ class tool_cohortroles_privacy_testcase extends \core_privacy\tests\provider_tes * @throws \core_competency\invalid_persistent_exception * @throws coding_exception */ - protected function setup_test_scenario_data($userid, $nocohortroles, $rolename = 'Sausage Roll', + protected function setup_test_scenario_data($userid, $context, $nocohortroles, $rolename = 'Sausage Roll', $roleshortname = 'sausageroll') { $roleid = create_role($rolename, $roleshortname, 'mmmm'); + $result = new \stdClass(); + $result->contextid = $context->id; + for ($c = 0; $c < $nocohortroles; $c++) { - $cohort = $this->getDataGenerator()->create_cohort(); + $cohort = $this->getDataGenerator()->create_cohort($result); $params = (object)array( 'userid' => $userid,