From: tjhunt Date: Mon, 15 Dec 2008 06:22:18 +0000 (+0000) Subject: accesslib: MDL-17647, MDL-17648 and MDL-17649 Bug fix, improvement and unit test. X-Git-Url: http://git.mjollnir.org/gw?a=commitdiff_plain;h=212235d3e23bf06decde2f10b96b57fbad46e593;p=moodle.git accesslib: MDL-17647, MDL-17648 and MDL-17649 Bug fix, improvement and unit test. MDL-17647 was referring to moodle/site:candoanything insstead of moodle/site:doanything MDL-17648 Let get_users_by_capability take an array of capabilities, like has_any_capability MDL-17649 get_users_by_capability must have unit tests (HEAD only). The unit tests were briefly working (apart from the system context, which I had to set up by hand in the test contexts table). Then I made the mistake of trying to upgrade the test tables, and it all went horribly wrong (MDL-17644). --- diff --git a/lib/accesslib.php b/lib/accesslib.php index 47aaf63aed..576e3f60d5 100755 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -675,6 +675,7 @@ function path_inaccessdata($path, $accessdata) { * AND that role has moodle/legacy:guest === 1... * THEN we act as if we hadn't seen it. * + * Note that this function must be kept in synch with has_capability_in_accessdata. * * To Do: * @@ -4579,7 +4580,10 @@ function get_default_course_role($course) { * on some DBs. * * @param $context - object - * @param $capability - string capability + * @param $capability - string capability, or an array of capabilities, in which + * case users having any of those capabilities will be returned. + * For performance reasons, you are advised to put the capability + * that the user is most likely to have first. * @param $fields - fields to be pulled * @param $sort - the sort order * @param $limitfrom - number of records to skip (offset) @@ -4617,7 +4621,11 @@ function get_users_by_capability($context, $capability, $fields='', $sort='', } // What roles/rolecaps are interesting? - $caps = array($capability); + if (is_array($capability)) { + $caps = $capability; + } else { + $caps = array($capability); + } if ($doanything === true) { $caps[] = 'moodle/site:doanything'; $doanything_join=''; @@ -4647,15 +4655,15 @@ function get_users_by_capability($context, $capability, $fields='', $sort='', $negperm = false; // has any negative (<0) permission? $roleids = array(); - list($caps, $params) = $DB->get_in_or_equal($caps, SQL_PARAMS_NAMED, 'cap0'); + list($capstest, $params) = $DB->get_in_or_equal($caps, SQL_PARAMS_NAMED, 'cap0'); $params['capany'] = 'moodle/site:doanything'; $sql = "SELECT rc.id, rc.roleid, rc.permission, rc.capability, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel FROM {role_capabilities} rc JOIN {context} ctx on rc.contextid = ctx.id - $doanything_join - WHERE rc.capability $caps AND ctx.id IN ($ctxids) + $doanything_join + WHERE rc.capability $capstest AND ctx.id IN ($ctxids) $doanything_cond ORDER BY rc.roleid ASC, ctx.depth ASC"; @@ -4911,7 +4919,7 @@ function get_users_by_capability($context, $capability, $fields='', $sort='', // (last rec or last in page), trigger the check-permission idiom // - the check permission idiom will // - add the default enrolment if needed - // - call has_capability_from_rarc(), which based on RAs and RCs will return a bool + // - call has_any_capability_from_rarc(), which based on RAs and RCs will return a bool // (should be fairly tight code ;-) ) // - if the user has permission, all is good, just $c++ (counter) // - ...else, decrease the counter - so pagination is kept straight, @@ -4954,7 +4962,7 @@ function get_users_by_capability($context, $capability, $fields='', $sort='', $ras[] = array( 'roleid' => $CFG->defaultuserroleid, 'depth' => 1 ); } - if (has_capability_from_rarc($ras, $roleperms, $capability, $doanything)) { + if (has_any_capability_from_rarc($ras, $roleperms, $caps)) { $c++; } else { // remove the user from the result set, @@ -4999,7 +5007,7 @@ function get_users_by_capability($context, $capability, $fields='', $sort='', $ras[] = array( 'roleid' => $CFG->defaultuserroleid, 'depth' => 1 ); } - if (!has_capability_from_rarc($ras, $roleperms, $capability, $doanything)) { + if (!has_any_capability_from_rarc($ras, $roleperms, $caps)) { // remove the user from the result set, // only if we are 'in the page' if ($limitfrom === 0 || $c >= $limitfrom) { @@ -5014,33 +5022,28 @@ function get_users_by_capability($context, $capability, $fields='', $sort='', } /** - * Fast (fast!) utility function to resolve if a capability is granted, - * based on Role Assignments and Role Capabilities. + * Fast (fast!) utility function to resolve if any of a list of capabilities is + * granted, based on Role Assignments and Role Capabilities. * * Used (at least) by get_users_by_capability(). * * If PHP had fast built-in memoize functions, we could * add a $contextid parameter and memoize the return values. * + * Note that this function must be kept in synch with has_capability_in_accessdata. + * * @param array $ras - role assignments * @param array $roleperms - role permissions - * @param string $capability - name of the capability - * @param bool $doanything + * @param string $capabilities - array of capability names * @return boolean - * */ -function has_capability_from_rarc($ras, $roleperms, $capability, $doanything) { +function has_any_capability_from_rarc($ras, $roleperms, $caps) { // Mini-state machine, using $hascap // $hascap[ 'moodle/foo:bar' ]->perm = CAP_SOMETHING (numeric constant) // $hascap[ 'moodle/foo:bar' ]->radepth = depth of the role assignment that set it // $hascap[ 'moodle/foo:bar' ]->rcdepth = depth of the rolecap that set it // -- when resolving conflicts, we need to look into radepth first, if unresolved - $caps = array($capability); - if ($doanything) { - $caps[] = 'moodle/site:candoanything'; - } - $hascap = array(); // @@ -5110,10 +5113,10 @@ function has_capability_from_rarc($ras, $roleperms, $capability, $doanything) { $hascap[$cap]->perm += $rp->perm; } } - if ((isset($hascap[$capability]->perm) and $hascap[$capability]->perm > 0) - or ($doanything and isset($hascap['moodle/site:candoanything']) - and $hascap['moodle/site:candoanything']->perm > 0)) { - return true; + foreach ($caps as $capability) { + if (isset($hascap[$capability]) && $hascap[$capability]->perm > 0) { + return true; + } } return false; } diff --git a/lib/simpletest/testaccesslib.php b/lib/simpletest/testaccesslib.php index 7dff8f5282..69c615d086 100644 --- a/lib/simpletest/testaccesslib.php +++ b/lib/simpletest/testaccesslib.php @@ -14,12 +14,6 @@ if (!defined('MOODLE_INTERNAL')) { class accesslib_test extends MoodleUnitTestCase { - function setUp() { - } - - function tearDown() { - } - function test_get_parent_contexts() { $context = get_context_instance(CONTEXT_SYSTEM); $this->assertEqual(get_parent_contexts($context), array()); @@ -45,5 +39,187 @@ class accesslib_test extends MoodleUnitTestCase { $context->path = '/1/123/234/345/456'; $this->assertEqual(get_parent_contextid($context), 345); } + + function test_get_users_by_capability() { + global $DB; + // Warning, this method assumes that the standard roles are set up with + // the standard definitions. + + $systemcontext = get_context_instance(CONTEXT_SYSTEM); + + // Create some nested contexts. instanceid does not matter for this. Just + // ensure we don't violate any unique keys by using an unlikely number. + // We will fix paths in a second. + $contexts = $this->load_test_data('context', + array('contextlevel', 'instanceid', 'path', 'depth'), array( + 1 => array(40, 666, '', 2), + 2 => array(50, 666, '', 3), + 3 => array(70, 666, '', 4), + )); + $contexts[0] = $systemcontext; + $contexts[1]->path = $contexts[0]->path . '/' . $contexts[1]->id; + $DB->set_field('context', 'path', $contexts[1]->path, array('id' => $contexts[1]->id)); + $contexts[2]->path = $contexts[1]->path . '/' . $contexts[2]->id; + $DB->set_field('context', 'path', $contexts[2]->path, array('id' => $contexts[2]->id)); + $contexts[3]->path = $contexts[2]->path . '/' . $contexts[3]->id; + $DB->set_field('context', 'path', $contexts[3]->path, array('id' => $contexts[3]->id)); + + // Now make some test users. + $users = $this->load_test_data('user', + array('username', 'confirmed', 'deleted'), array( + 'a' => array('a', 1, 0), + 'cc' => array('cc', 1, 0), + 't1' => array('t1', 1, 0), + 's1' => array('s1', 1, 0), + 's2' => array('s2', 1, 0), + 'del' => array('del', 1, 1), + 'unc' => array('unc', 0, 0), + )); + + // Get some of the standard roles. + $admin = $DB->get_record('role', array('shortname' => 'admin')); + $creator = $DB->get_record('role', array('shortname' => 'coursecreator')); + $teacher = $DB->get_record('role', array('shortname' => 'editingteacher')); + $student = $DB->get_record('role', array('shortname' => 'student')); + $authuser = $DB->get_record('role', array('shortname' => 'user')); + + // And some role assignments. + $ras = $this->load_test_data('role_assignments', + array('userid', 'roleid', 'contextid'), array( + 'a' => array($users['a']->id, $admin->id, $contexts[0]->id), + 'cc' => array($users['cc']->id, $creator->id, $contexts[1]->id), + 't1' => array($users['t1']->id, $teacher->id, $contexts[2]->id), + 's1' => array($users['s1']->id, $student->id, $contexts[2]->id), + 's2' => array($users['s2']->id, $student->id, $contexts[2]->id), + )); + + // And some group memebership. + $gms = $this->load_test_data('groups_members', + array('userid', 'groupid'), array( + array($users['t1']->id, 666), + array($users['s1']->id, 666), + array($users['s2']->id, 667), + )); + + // Test some simple cases - check that looking in coruse and module contextlevel gives the same answer. + foreach (array(2, 3) as $conindex) { + $results = get_users_by_capability($contexts[$conindex], 'mod/forum:replypost'); + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['t1']->id, $users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + $results)); + // Paging. + $firstuser = reset($results); + $this->assertEqual(array($firstuser->id => $firstuser), get_users_by_capability($contexts[$conindex], 'mod/forum:replypost', '', '', 0, 1)); + $seconduser = next($results); + $this->assertEqual(array($seconduser->id => $seconduser), get_users_by_capability($contexts[$conindex], 'mod/forum:replypost', '', '', 1, 1)); + // $doanything = false + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['t1']->id, $users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[$conindex], 'mod/forum:replypost', '', '', '', '', '', '', false))); + // group + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['t1']->id, $users['s1']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[$conindex], 'mod/forum:replypost', '', '', '', '', 666))); + // exceptions + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[$conindex], 'mod/forum:replypost', '', '', '', '', '', array($users['t1']->id)))); + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['s1']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[$conindex], 'mod/forum:replypost', '', '', '', '', 666, array($users['t1']->id)))); + // $useviewallgroups + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['t1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[$conindex], 'mod/forum:replypost', '', '', '', '', 667, '', false, false, true))); + // More than one capability. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[$conindex], array('mod/quiz:attempt', 'mod/quiz:reviewmyattempts')))); + } + // System context, specifically checking doanythign. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[0], 'moodle/site:doanything'))); + +// For reference: get_users_by_capability argument order: +// $context, $capability, $fields='', $sort='', $limitfrom='', $limitnum='', +// $groups='', $exceptions='', $doanything=true, $view=false, $useviewallgroups=false + + // Now add some role overrides. + $rcs = $this->load_test_data('role_capabilities', + array('capability', 'roleid', 'contextid', 'permission'), array( + array('mod/forum:replypost', $student->id, $contexts[1]->id, CAP_PREVENT), + array('mod/forum:replypost', $student->id, $contexts[3]->id, CAP_ALLOW), + array('mod/quiz:attempt', $student->id, $contexts[2]->id, CAP_PREVENT), + array('mod/forum:startdiscussion', $student->id, $contexts[1]->id, CAP_PROHIBIT), + array('mod/forum:startdiscussion', $student->id, $contexts[3]->id, CAP_ALLOW), + array('mod/forum:viewrating', $authuser->id, $contexts[1]->id, CAP_PROHIBIT), + array('mod/forum:createattachment', $authuser->id, $contexts[3]->id, CAP_PREVENT), +// array('mod/forum:startdiscussion', $student->id, $contexts[1]->id, CAP_PROHIBIT), +// array('mod/forum:startdiscussion', $student->id, $contexts[3]->id, CAP_ALLOW), + )); + + // Now test the overridden cases. + // Students prevented at category level, with and without doanything. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['t1']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[2], 'mod/forum:replypost'))); + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['t1']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[2], 'mod/forum:replypost', '', '', '', '', '', '', false))); + // Students prevented at category level, but re-allowed at module level, with and without doanything. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['t1']->id, $users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[3], 'mod/forum:replypost', '', '', '', '', '', '', false))); + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['t1']->id, $users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[3], 'mod/forum:replypost'))); + // Students prohibited at category level, re-allowed at module level should have no effect. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['t1']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[2], 'mod/forum:startdiscussion'))); + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['t1']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[3], 'mod/forum:startdiscussion'))); + // Prevent on logged-in user should be overridden by student allow. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id, $users['t1']->id, $users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[3], 'mod/forum:createattachment'))); + + // Prohibit on logged-in user should trump student/teacher allow. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['a']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[3], 'mod/forum:viewrating'))); + + // More than one capability, where students have one, but not the other. + $this->assert(new ArraysHaveSameValuesExpectation( + array($users['s1']->id, $users['s2']->id)), + array_map(create_function('$o', 'return $o->id;'), + get_users_by_capability($contexts[3], array('mod/quiz:attempt', 'mod/quiz:reviewmyattempts'), '', '', '', '', '', '', false))); + + // Clean up everything we added. + unset($contexts[0]); + $this->delete_test_data('role_capabilities', $rcs); + $this->delete_test_data('groups_members', $gms); + $this->delete_test_data('role_assignments', $ras); + $this->delete_test_data('user', $users); + $this->delete_test_data('context', $contexts); + } } ?>