From 82701e2443d67464314b3ea234be4fad7c6bb6c6 Mon Sep 17 00:00:00 2001 From: tjhunt Date: Mon, 23 Mar 2009 08:15:21 +0000 Subject: [PATCH] switch roles: MDL-18132 separate database table role_allow_switch instead of re-using role_allow_assign. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This is part 1 that does the back-end: 1. New table role_allow_switch. 2. Upgrade that copies all the allows from role_allow_assign, and then drops the old CFG->allowuserswitchrolestheycantassign. 3. Old function get_assignable_roles_for_switchrole renamed to get_switchable_roles and changed to use the new table. Fixes MDL-18604 in HEAD. 4. Switch callers to use the new function name. 5. Unit tests for this new function. 6. To make those unit tests work, new switch_global_user_id and revert_global_user_id methods in UnitTestCaseUsingDatabase for toggling $USER->id. I still need to do the editing interface under Administration ► Users ► Permissions ► Define roles. That will be done as a second commit. --- course/view.php | 2 +- lib/accesslib.php | 85 +++++++++++++++-------------- lib/db/install.xml | 23 ++++++-- lib/db/upgrade.php | 38 +++++++++++++ lib/simpletest/testaccesslib.php | 94 +++++++++++++++++++++++++++++++- lib/simpletestlib.php | 57 ++++++++++++++++--- lib/weblib.php | 2 +- version.php | 2 +- 8 files changed, 245 insertions(+), 58 deletions(-) diff --git a/course/view.php b/course/view.php index 48e65d0e76..da352c3bdc 100644 --- a/course/view.php +++ b/course/view.php @@ -57,7 +57,7 @@ has_capability('moodle/role:switchroles', $context)) { // is this role assignable in this context? // inquiring minds want to know... - $aroles = get_assignable_roles($context); + $aroles = get_switchable_roles($context); if (is_array($aroles) && isset($aroles[$switchrole])) { role_switch($switchrole, $context); // Double check that this role is allowed here diff --git a/lib/accesslib.php b/lib/accesslib.php index 0e84f05d29..a50d351632 100755 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -4337,52 +4337,57 @@ function get_assignable_roles($context, $rolenamedisplay = ROLENAME_ALIAS, $with } /** - * Gets a list of roles that this user can assign in this context, for the switchrole menu + * Gets a list of roles that this user can switch to in a context, for the switchrole menu. + * This function just process the contents of the role_allow_switch table. You also need to + * test the moodle/role:switchroles to see if the user is allowed to switch in the first place. * - * @param object $context the context. + * @param object $context a context. * @return an array $roleid => $rolename. */ -function get_assignable_roles_for_switchrole($context) { +function get_switchable_roles($context) { global $USER, $DB; - if (!has_capability('moodle/role:assign', $context)) { - return array(); - } - - $parents = get_parent_contexts($context); - $parents[] = $context->id; - $contexts = implode(',' , $parents); - - $raafrom = "{role_allow_assign} raa,"; - $raawhere = "AND raa.roleid = ra.roleid AND r.id = raa.allowassign"; - if (has_capability('moodle/site:doanything', get_context_instance(CONTEXT_SYSTEM))) { - // show all roles allowed in this context to admins - $raafrom = ""; - $raawhere = ""; - } - - if (!$roles = $DB->get_records_sql(" - SELECT ro.* - FROM {role} ro - JOIN (SELECT DISTINCT r.id - FROM {role} r, - {role_assignments} ra, - $raafrom - {role_capabilities} rc - WHERE ra.userid = :userid AND ra.contextid IN ($contexts) - $raawhere - AND r.id = rc.roleid AND rc.capability = :viewcap AND rc.capability <> :anythingcap - ) inline_view ON ro.id = inline_view.id - ORDER BY ro.sortorder ASC", - array('userid'=>$USER->id, 'viewcap'=>'moodle/course:view', - 'anythingcap'=>'moodle/site:doanything'))) { - return array(); - } + $systemcontext = get_context_instance(CONTEXT_SYSTEM); - $rolenames = array(); - foreach ($roles as $role) { - $rolenames[$role->id] = $role->name; - } + $params = array(); + $extrajoins = ''; + $extrawhere = ''; + if (!has_capability('moodle/site:doanything', $systemcontext)) { + // Admins are allowed to switch to any role with 'moodle/course:view' in the + // role definition, and without 'moodle/site:doanything' anywhere. + // Others are subject to the additional constraint that the switch-to role must be allowed by + // 'role_allow_switch' for some role they have assigned in this context or any parent. + $parents = get_parent_contexts($context); + $parents[] = $context->id; + $contexts = implode(',' , $parents); + + $extrajoins = "JOIN {role_allow_switch} ras ON ras.allowswitch = rc.roleid + JOIN {role_assignments} ra ON ra.roleid = ras.roleid"; + $extrawhere = "AND ra.userid = :userid + AND ra.contextid IN ($contexts)"; + $params['userid'] = $USER->id; + } + + $query = " + SELECT r.id, r.name + FROM ( + SELECT DISTINCT rc.roleid + FROM {role_capabilities} rc + $extrajoins + WHERE rc.capability = :viewcap + AND rc.contextid = :syscontextid + $extrawhere + AND NOT EXISTS ( + SELECT 1 FROM {role_capabilities} irc WHERE irc.roleid = rc.roleid AND + irc.capability = :anythingcap) + ) idlist + JOIN {role} r ON r.id = idlist.roleid + ORDER BY r.sortorder"; + $params['syscontextid'] = $systemcontext->id; + $params['viewcap'] = 'moodle/course:view'; + $params['anythingcap'] = 'moodle/site:doanything'; + + $rolenames = $DB->get_records_sql_menu($query, $params); return role_fix_names($rolenames, $context, ROLENAME_ALIAS); } diff --git a/lib/db/install.xml b/lib/db/install.xml index 803f59c072..4f65217bb5 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1,5 +1,5 @@ - @@ -867,7 +867,7 @@ - +
@@ -882,7 +882,22 @@
- +
+ + + + + + + + + + + + + +
+ @@ -2102,7 +2117,7 @@ - + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 18cbdfa561..58899768be 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -1497,6 +1497,44 @@ WHERE gradeitemid IS NOT NULL AND grademax IS NOT NULL"); upgrade_main_savepoint($result, 2009030501); } + /// MDL-18132 replace the use a new Role allow switch settings page, instead of + /// $CFG->allowuserswitchrolestheycantassign + if ($result && $oldversion < 2009032000) { + /// First create the new table. + $table = new xmldb_table('role_allow_switch'); + + /// Adding fields to table role_allow_switch + $table->add_field('id', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, XMLDB_SEQUENCE, null, null, null); + $table->add_field('roleid', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, null, null, null); + $table->add_field('allowswitch', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, null, null, null); + + /// Adding keys to table role_allow_switch + $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); + $table->add_key('roleid', XMLDB_KEY_FOREIGN, array('roleid'), 'role', array('id')); + $table->add_key('allowswitch', XMLDB_KEY_FOREIGN, array('allowswitch'), 'role', array('id')); + + /// Adding indexes to table role_allow_switch + $table->add_index('roleid-allowoverride', XMLDB_INDEX_UNIQUE, array('roleid', 'allowswitch')); + + /// Conditionally launch create table for role_allow_switch + if (!$dbman->table_exists($table)) { + $dbman->create_table($table); + } + + /// Main savepoint reached + upgrade_main_savepoint($result, 2009032000); + } + if ($result && $oldversion < 2009032001) { + /// Copy from role_allow_assign into the new table. + $DB->execute('INSERT INTO {role_allow_switch} SELECT * FROM {role_allow_assign}'); + + /// Unset the config variable used in 1.9. + unset_config('allowuserswitchrolestheycantassign'); + + /// Main savepoint reached + upgrade_main_savepoint($result, 2009032001); + } + return $result; } diff --git a/lib/simpletest/testaccesslib.php b/lib/simpletest/testaccesslib.php index 1387368b77..e80fe20b0d 100644 --- a/lib/simpletest/testaccesslib.php +++ b/lib/simpletest/testaccesslib.php @@ -40,9 +40,6 @@ class accesslib_test extends UnitTestCaseUsingDatabase { } function test_get_users_by_capability() { - global $CFG; - // Warning, this method assumes that the standard roles are set up with - // the standard definitions. $tablenames = array('capabilities' , 'context', 'role', 'role_capabilities', 'role_allow_assign', 'role_allow_override', 'role_assignments', 'role_context_levels', 'user', 'groups_members', 'cache_flags', 'events_handlers', 'user_lastaccess', 'course'); @@ -247,5 +244,96 @@ class accesslib_test extends UnitTestCaseUsingDatabase { $this->drop_test_tables($tablenames); accesslib_clear_all_caches_for_unit_testing(); } + + function test_get_switchable_roles() { + global $USER; + + $tablenames = array('role' , 'role_capabilities', 'role_assignments', 'role_allow_switch', + 'capabilities', 'context', 'role_names'); + $this->create_test_tables($tablenames, 'lib'); + + $this->switch_to_test_db(); + $saveduserid = $USER->id; + + // Ensure SYSCONTEXTID is set. + get_context_instance(CONTEXT_SYSTEM); + + $contexts = $this->load_test_data('context', + array('contextlevel', 'instanceid', 'path', 'depth'), array( + 'sys' => array(CONTEXT_SYSTEM, 0, '/' . SYSCONTEXTID, 1), + 'cat' => array(CONTEXT_COURSECAT, 66, '/' . SYSCONTEXTID . '/' . (SYSCONTEXTID + 1), 2), + 'cou' => array(CONTEXT_COURSE, 666, '/' . SYSCONTEXTID . '/' . (SYSCONTEXTID + 1) . '/' . (SYSCONTEXTID + 2), 3), + 'fp' => array(CONTEXT_COURSE, SITEID, '/' . SYSCONTEXTID . '/' . SITEID, 2))); + $this->testdb->set_field('context', 'id', SYSCONTEXTID, array('id' => $contexts['sys']->id)); + $this->testdb->set_field('context', 'id', SYSCONTEXTID + 1, array('id' => $contexts['cat']->id)); + $this->testdb->set_field('context', 'id', SYSCONTEXTID + 2, array('id' => $contexts['cou']->id)); + $syscontext = $contexts['sys']; + $syscontext->id = SYSCONTEXTID; + $context = $contexts['cou']; + $context->id = SYSCONTEXTID + 2; + + $this->load_test_data('capabilities', + array('name'), array( + array('moodle/site:doanything'), + array('moodle/course:view'))); + + $roles = $this->load_test_data('role', + array( 'name', 'shortname', 'description', 'sortorder'), array( + 'admin' => array('admin', 'admin', 'not null', 1), + 'r1' => array( 'r1', 'r1', 'not null', 2), + 'r2' => array( 'r2', 'r2', 'not null', 3), + 'funny' => array('funny', 'funny', 'not null', 4))); + $adminid = $roles['admin']->id; + $r1id = $roles['r1']->id; + $r2id = $roles['r2']->id; + $funnyid = $roles['funny']->id; // strange role to test that roles with 'moodle/site:doanything' and 'moodle/course:view' are not returned. + + $this->load_test_data('role_capabilities', + array('roleid', 'capability', 'contextid', 'permission'), array( + array($adminid, 'moodle/site:doanything', SYSCONTEXTID, CAP_ALLOW), + array( $r1id, 'moodle/course:view', SYSCONTEXTID + 1, CAP_ALLOW), + array( $r2id, 'moodle/course:view', SYSCONTEXTID, CAP_ALLOW), + array($funnyid, 'moodle/site:doanything', SYSCONTEXTID, CAP_ALLOW), + array($funnyid, 'moodle/course:view', SYSCONTEXTID, CAP_ALLOW))); + + $this->load_test_data('role_assignments', + array('userid', 'contextid', 'roleid'), array( + array( 1, SYSCONTEXTID, $adminid), + array( 2, SYSCONTEXTID + 1 , $r1id), + array( 3, SYSCONTEXTID + 2 , $r2id))); + + $this->load_test_data('role_allow_switch', + array('roleid', 'allowswitch'), array( + array( $r1id , $r2id), + array( $r2id , $r1id), + array( $r2id , $r2id), + array( $r2id , $funnyid))); + + // Admin should be able to switch to any role with 'moodle/course:view' in any context. + $this->switch_global_user_id(1); + accesslib_clear_all_caches_for_unit_testing(); + $this->assert(new ArraysHaveSameValuesExpectation(array($r2id)), array_keys(get_switchable_roles($syscontext))); + $this->assert(new ArraysHaveSameValuesExpectation(array($r2id)), array_keys(get_switchable_roles($context))); + $this->revert_global_user_id(); + + // r1 should be able to switch to r2, but this user only has r1 in $context, not $syscontext. + $this->switch_global_user_id(2); + accesslib_clear_all_caches_for_unit_testing(); + $this->assert(new ArraysHaveSameValuesExpectation(array()), array_keys(get_switchable_roles($syscontext))); + $this->assert(new ArraysHaveSameValuesExpectation(array($r2id)), array_keys(get_switchable_roles($context))); + $this->revert_global_user_id(); + + // The table says r2 should be able to switch to all of r1, r2 and funny, however, only r2 passes the tests on which roles can be returnd.. + $this->switch_global_user_id(3); + accesslib_clear_all_caches_for_unit_testing(); + $this->assert(new ArraysHaveSameValuesExpectation(array()), array_keys(get_switchable_roles($syscontext))); + $this->assert(new ArraysHaveSameValuesExpectation(array($r2id)), array_keys(get_switchable_roles($context))); + $this->revert_global_user_id(); + + // Clean up everything we added. + $this->revert_to_real_db(); + $this->drop_test_tables($tablenames); + accesslib_clear_all_caches_for_unit_testing(); + } } ?> diff --git a/lib/simpletestlib.php b/lib/simpletestlib.php index 6a6680f0a9..2c21e257f3 100644 --- a/lib/simpletestlib.php +++ b/lib/simpletestlib.php @@ -158,23 +158,21 @@ class CheckSpecifiedFieldsExpectation extends SimpleExpectation { class UnitTestCaseUsingDatabase extends UnitTestCase { private $realdb; protected $testdb; + private $realuserid = null; private $tables = array(); - /** - * In the constructor, record the max(id) of each test table into a csv file. - * If this file already exists, it means that a previous run of unit tests - * did not complete, and has left data undeleted in the DB. This data is then - * deleted and the file is retained. Otherwise it is created. - * @throws moodle_exception if CSV file cannot be created - */ public function __construct($label = false) { global $DB, $CFG; + // Complain if we get this far and $CFG->unittestprefix is not set. if (empty($CFG->unittestprefix)) { throw new coding_exception('You cannot use UnitTestCaseUsingDatabase unless you set $CFG->unittestprefix.'); } + + // Only do this after the above text. parent::UnitTestCase($label); + // Create the test DB instance. $this->realdb = $DB; $this->testdb = moodle_database::get_driver_instance($CFG->dbtype, $CFG->dblibrary); $this->testdb->connect($CFG->dbhost, $CFG->dbuser, $CFG->dbpass, $CFG->dbname, $CFG->unittestprefix); @@ -203,12 +201,43 @@ class UnitTestCaseUsingDatabase extends UnitTestCase { $DB = $this->realdb; } + /** + * Switch $USER->id to a test value. + * You must remember to switch back using revert_global_user_id() before the end of the test. + * + * It might be worth making this method do more robuse $USER switching in future, + * however, this is sufficient for my needs at present. + */ + protected function switch_global_user_id($userid) { + global $USER; + if (!is_null($this->realuserid)) { + debugging('switch_global_user_id called when $USER->id was already switched to a different value. This suggest you are doing something wrong and dangerous. Please review your code immediately.', DEBUG_DEVELOPER); + } else { + $this->realuserid = $USER->id; + } + $USER->id = $userid; + } + + /** + * Revert $USER->id to the real value. + */ + protected function revert_global_user_id() { + global $USER; + if (is_null($this->realuserid)) { + debugging('revert_global_user_id called without switch_global_user_id having been called first. This suggest you are doing something wrong and dangerous. Please review your code immediately.', DEBUG_DEVELOPER); + } else { + $USER->id = $this->realuserid; + $this->realuserid = null; + } + } + /** * Check that the user has not forgotten to clean anything up, and if they * have, display a rude message and clean it up for them. */ private function emergency_clean_up() { global $DB; + $cleanmore = false; // Check that they did not forget to drop any test tables. if (!empty($this->tables)) { @@ -221,8 +250,20 @@ class UnitTestCaseUsingDatabase extends UnitTestCase { // Check that they did not forget to switch page to the real DB. if ($DB !== $this->realdb) { - debugging('You did not switch back to the real database in your UnitTestCaseUsingDatabase.', DEBUG_DEVELOPER); + debugging('You did not switch back to the real database using revert_to_real_db in your UnitTestCaseUsingDatabase.', DEBUG_DEVELOPER); $this->revert_to_real_db(); + $cleanmore = true; + } + + // Check for forgetting to call revert_global_user_id. + if (!is_null($this->realuserid)) { + debugging('You did not switch back to the real $USER->id using revert_global_user_id in your UnitTestCaseUsingDatabase.', DEBUG_DEVELOPER); + $this->revert_global_user_id(); + $cleanmore = true; + } + + if ($cleanmore) { + accesslib_clear_all_caches_for_unit_testing(); } } diff --git a/lib/weblib.php b/lib/weblib.php index adff83fef1..602146742f 100644 --- a/lib/weblib.php +++ b/lib/weblib.php @@ -5099,7 +5099,7 @@ function switchroles_form($courseid) { } if (has_capability('moodle/role:switchroles', $context)) { - if (!$roles = get_assignable_roles_for_switchrole($context)) { + if (!$roles = get_switchable_roles($context)) { return ''; // Nothing to show! } // unset default user role - it would not work diff --git a/version.php b/version.php index 9b04bfccb3..4f1d82d485 100644 --- a/version.php +++ b/version.php @@ -6,7 +6,7 @@ // This is compared against the values stored in the database to determine // whether upgrades should be performed (see lib/db/*.php) - $version = 2009030501; // YYYYMMDD = date of the last version bump + $version = 2009032001; // YYYYMMDD = date of the last version bump // XX = daily increments $release = '2.0 dev (Build: 20090323)'; // Human-friendly version name -- 2.39.5