From 0d3bdb94bc9ba47be88d3a1025d0d30b06cbf98e Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 14 Apr 2020 17:06:13 +0100 Subject: [PATCH 1/3] MDL-68402 accesslib: fix get_with_capability_join logic In fact, rather than fix the old logic, I noticed that the correct logic was already implemented in get_users_by_capability. So, I refactored to extract the working version into a function, which it turns out can have exactly the same API as get_with_capability_join, which was convenient. --- lib/accesslib.php | 397 ++++++++++++----------------------- lib/classes/dml/sql_join.php | 25 ++- lib/tests/accesslib_test.php | 71 ++++++- 3 files changed, 226 insertions(+), 267 deletions(-) diff --git a/lib/accesslib.php b/lib/accesslib.php index 6c419f2cb3a..774ada6c0d0 100644 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -3427,36 +3427,25 @@ function set_role_contextlevels($roleid, array $contextlevels) { } /** - * Who has this capability in this context? + * Gets sql joins for finding users with capability in the given context. * - * This can be a very expensive call - use sparingly and keep - * the results if you are going to need them again soon. - * - * Note if $fields is empty this function attempts to get u.* - * which can get rather large - and has a serious perf impact - * on some DBs. - * - * @param context $context - * @param string|array $capability - capability name(s) - * @param string $fields - fields to be pulled. The user table is aliased to 'u'. u.id MUST be included. - * @param string $sort - the sort order. Default is lastaccess time. - * @param mixed $limitfrom - number of records to skip (offset) - * @param mixed $limitnum - number of records to fetch - * @param string|array $groups - single group or array of groups - only return - * users who are in one of these group(s). - * @param string|array $exceptions - list of users to exclude, comma separated or array - * @param bool $doanything_ignored not used any more, admin accounts are never returned - * @param bool $view_ignored - use get_enrolled_sql() instead - * @param bool $useviewallgroups if $groups is set the return users who - * have capability both $capability and moodle/site:accessallgroups - * in this context, as well as users who have $capability and who are - * in $groups. - * @return array of user records + * @param context $context Context for the join. + * @param string|array $capability Capability name or array of names. + * If an array is provided then this is the equivalent of a logical 'OR', + * i.e. the user needs to have one of these capabilities. + * @param string $useridcolumn e.g. 'u.id'. + * @return \core\dml\sql_join Contains joins, wheres, params. + * This function will set ->cannotmatchanyrows if applicable. + * This may let you skip doing a DB query. */ -function get_users_by_capability(context $context, $capability, $fields = '', $sort = '', $limitfrom = '', $limitnum = '', - $groups = '', $exceptions = '', $doanything_ignored = null, $view_ignored = null, $useviewallgroups = false) { +function get_with_capability_join(context $context, $capability, $useridcolumn) { global $CFG, $DB; + // Add a unique prefix to param names to ensure they are unique. + static $i = 0; + $i++; + $paramprefix = 'eu' . $i . '_'; + $defaultuserroleid = isset($CFG->defaultuserroleid) ? $CFG->defaultuserroleid : 0; $defaultfrontpageroleid = isset($CFG->defaultfrontpageroleid) ? $CFG->defaultfrontpageroleid : 0; @@ -3464,16 +3453,8 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s $ctxids = str_replace('/', ',', $ctxids); // Context is the frontpage - $iscoursepage = false; // coursepage other than fp - $isfrontpage = false; - if ($context->contextlevel == CONTEXT_COURSE) { - if ($context->instanceid == SITEID) { - $isfrontpage = true; - } else { - $iscoursepage = true; - } - } - $isfrontpage = ($isfrontpage || is_inside_frontpage($context)); + $isfrontpage = $context->contextlevel == CONTEXT_COURSE && $context->instanceid == SITEID; + $isfrontpage = $isfrontpage || is_inside_frontpage($context); $caps = (array)$capability; @@ -3482,8 +3463,8 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s // we need to find out all roles that have these capabilities either in definition or in overrides $defs = array(); - list($incontexts, $params) = $DB->get_in_or_equal($contextids, SQL_PARAMS_NAMED, 'con'); - list($incaps, $params2) = $DB->get_in_or_equal($caps, SQL_PARAMS_NAMED, 'cap'); + list($incontexts, $params) = $DB->get_in_or_equal($contextids, SQL_PARAMS_NAMED, $paramprefix . 'con'); + list($incaps, $params2) = $DB->get_in_or_equal($caps, SQL_PARAMS_NAMED, $paramprefix . 'cap'); // Check whether context locking is enabled. // Filter out any write capability if this is the case. @@ -3558,7 +3539,7 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s if (empty($needed)) { // there can not be anybody if no roles match this request - return array(); + return new \core\dml\sql_join('', '1 = 2', [], true); } if (empty($prohibited)) { @@ -3573,76 +3554,15 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s unset($n); } - // ***** Set up default fields ****** - if (empty($fields)) { - if ($iscoursepage) { - $fields = 'u.*, ul.timeaccess AS lastaccess'; - } else { - $fields = 'u.*'; - } - } else { - if ($CFG->debugdeveloper && strpos($fields, 'u.*') === false && strpos($fields, 'u.id') === false) { - debugging('u.id must be included in the list of fields passed to get_users_by_capability().', DEBUG_DEVELOPER); - } - } - - // Set up default sort - if (empty($sort)) { // default to course lastaccess or just lastaccess - if ($iscoursepage) { - $sort = 'ul.timeaccess'; - } else { - $sort = 'u.lastaccess'; - } - } - // Prepare query clauses $wherecond = array(); $params = array(); $joins = array(); - // User lastaccess JOIN - if ((strpos($sort, 'ul.timeaccess') === false) and (strpos($fields, 'ul.timeaccess') === false)) { - // user_lastaccess is not required MDL-13810 - } else { - if ($iscoursepage) { - $joins[] = "LEFT OUTER JOIN {user_lastaccess} ul ON (ul.userid = u.id AND ul.courseid = {$context->instanceid})"; - } else { - throw new coding_exception('Invalid sort in get_users_by_capability(), ul.timeaccess allowed only for course contexts.'); - } - } - // We never return deleted users or guest account. - $wherecond[] = "u.deleted = 0 AND u.id <> :guestid"; - $params['guestid'] = $CFG->siteguest; - - // Groups - if ($groups) { - $groups = (array)$groups; - list($grouptest, $grpparams) = $DB->get_in_or_equal($groups, SQL_PARAMS_NAMED, 'grp'); - $joins[] = "LEFT OUTER JOIN (SELECT DISTINCT userid - FROM {groups_members} - WHERE groupid $grouptest - ) gm ON gm.userid = u.id"; - - $params = array_merge($params, $grpparams); - - $grouptest = 'gm.userid IS NOT NULL'; - if ($useviewallgroups) { - $viewallgroupsusers = get_users_by_capability($context, 'moodle/site:accessallgroups', 'u.id, u.id', '', '', '', '', $exceptions); - if (!empty($viewallgroupsusers)) { - $grouptest .= ' OR u.id IN (' . implode(',', array_keys($viewallgroupsusers)) . ')'; - } - } - $wherecond[] = "($grouptest)"; - } - - // User exceptions - if (!empty($exceptions)) { - $exceptions = (array)$exceptions; - list($exsql, $exparams) = $DB->get_in_or_equal($exceptions, SQL_PARAMS_NAMED, 'exc', false); - $params = array_merge($params, $exparams); - $wherecond[] = "u.id $exsql"; - } + $deletedusercolumn = substr($useridcolumn, 0, -2) . 'deleted'; // Hack to get the deleted user column without an API change. + $wherecond[] = "$deletedusercolumn = 0 AND $useridcolumn <> :{$paramprefix}guestid"; + $params[$paramprefix . 'guestid'] = $CFG->siteguest; // now add the needed and prohibited roles conditions as joins if (!empty($needed['any'])) { @@ -3654,7 +3574,7 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s FROM {role_assignments} WHERE contextid IN ($ctxids) AND roleid IN (".implode(',', array_keys($needed['any'])) .") - ) ra ON ra.userid = u.id"; + ) ra ON ra.userid = $useridcolumn"; } } else { $unions = array(); @@ -3699,7 +3619,7 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s } if (!$everybody) { if ($unions) { - $joins[] = "JOIN (SELECT DISTINCT userid FROM ( ".implode(' UNION ', $unions)." ) us) ra ON ra.userid = u.id"; + $joins[] = "JOIN (SELECT DISTINCT userid FROM ( ".implode(' UNION ', $unions)." ) us) ra ON ra.userid = $useridcolumn"; } else { // only prohibits found - nobody can be matched $wherecond[] = "1 = 2"; @@ -3707,6 +3627,116 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s } } + return new \core\dml\sql_join(implode("\n", $joins), implode(" AND ", $wherecond), $params); +} + +/** + * Who has this capability in this context? + * + * This can be a very expensive call - use sparingly and keep + * the results if you are going to need them again soon. + * + * Note if $fields is empty this function attempts to get u.* + * which can get rather large - and has a serious perf impact + * on some DBs. + * + * @param context $context + * @param string|array $capability - capability name(s) + * @param string $fields - fields to be pulled. The user table is aliased to 'u'. u.id MUST be included. + * @param string $sort - the sort order. Default is lastaccess time. + * @param mixed $limitfrom - number of records to skip (offset) + * @param mixed $limitnum - number of records to fetch + * @param string|array $groups - single group or array of groups - only return + * users who are in one of these group(s). + * @param string|array $exceptions - list of users to exclude, comma separated or array + * @param bool $doanything_ignored not used any more, admin accounts are never returned + * @param bool $view_ignored - use get_enrolled_sql() instead + * @param bool $useviewallgroups if $groups is set the return users who + * have capability both $capability and moodle/site:accessallgroups + * in this context, as well as users who have $capability and who are + * in $groups. + * @return array of user records + */ +function get_users_by_capability(context $context, $capability, $fields = '', $sort = '', $limitfrom = '', $limitnum = '', + $groups = '', $exceptions = '', $doanything_ignored = null, $view_ignored = null, $useviewallgroups = false) { + global $CFG, $DB; + + // Context is a course page other than the frontpage + $iscoursepage = $context->contextlevel == CONTEXT_COURSE && $context->instanceid != SITEID; + + // Get the bits of SQL relating to capabilities. + $sqljoin = get_with_capability_join($context, $capability, 'u.id'); + if ($sqljoin->cannotmatchanyrows) { + return []; + } + + // ***** Set up default fields ****** + if (empty($fields)) { + if ($iscoursepage) { + $fields = 'u.*, ul.timeaccess AS lastaccess'; + } else { + $fields = 'u.*'; + } + } else { + if ($CFG->debugdeveloper && strpos($fields, 'u.*') === false && strpos($fields, 'u.id') === false) { + debugging('u.id must be included in the list of fields passed to get_users_by_capability().', DEBUG_DEVELOPER); + } + } + + // Set up default sort + if (empty($sort)) { // default to course lastaccess or just lastaccess + if ($iscoursepage) { + $sort = 'ul.timeaccess'; + } else { + $sort = 'u.lastaccess'; + } + } + + // Prepare query clauses + $wherecond = [$sqljoin->wheres]; + $params = $sqljoin->params; + $joins = [$sqljoin->joins]; + + // User lastaccess JOIN + if ((strpos($sort, 'ul.timeaccess') === false) and (strpos($fields, 'ul.timeaccess') === false)) { + // user_lastaccess is not required MDL-13810 + } else { + if ($iscoursepage) { + $joins[] = "LEFT OUTER JOIN {user_lastaccess} ul ON (ul.userid = u.id AND ul.courseid = {$context->instanceid})"; + } else { + throw new coding_exception('Invalid sort in get_users_by_capability(), ul.timeaccess allowed only for course contexts.'); + } + } + + // Groups + if ($groups) { + $groups = (array)$groups; + list($grouptest, $grpparams) = $DB->get_in_or_equal($groups, SQL_PARAMS_NAMED, 'grp'); + $joins[] = "LEFT OUTER JOIN (SELECT DISTINCT userid + FROM {groups_members} + WHERE groupid $grouptest + ) gm ON gm.userid = u.id"; + + $params = array_merge($params, $grpparams); + + $grouptest = 'gm.userid IS NOT NULL'; + if ($useviewallgroups) { + $viewallgroupsusers = get_users_by_capability($context, 'moodle/site:accessallgroups', 'u.id, u.id', '', '', '', '', $exceptions); + if (!empty($viewallgroupsusers)) { + $grouptest .= ' OR u.id IN (' . implode(',', array_keys($viewallgroupsusers)) . ')'; + } + } + $wherecond[] = "($grouptest)"; + } + + // User exceptions + if (!empty($exceptions)) { + $exceptions = (array)$exceptions; + list($exsql, $exparams) = $DB->get_in_or_equal($exceptions, SQL_PARAMS_NAMED, 'exc', false); + $params = array_merge($params, $exparams); + $wherecond[] = "u.id $exsql"; + } + // Collect WHERE conditions and needed joins $where = implode(' AND ', $wherecond); if ($where !== '') { @@ -7586,160 +7616,3 @@ function get_with_capability_sql(context $context, $capability) { return array($sql, $capjoin->params); } - -/** - * Gets sql joins for finding users with capability in the given context - * - * @param context $context Context for the join - * @param string|array $capability Capability name or array of names. - * If an array is provided then this is the equivalent of a logical 'OR', - * i.e. the user needs to have one of these capabilities. - * @param string $useridcolumn e.g. 'u.id' - * @return \core\dml\sql_join Contains joins, wheres, params - */ -function get_with_capability_join(context $context, $capability, $useridcolumn) { - global $DB, $CFG; - - // Use unique prefix just in case somebody makes some SQL magic with the result. - static $i = 0; - $i++; - $prefix = 'eu' . $i . '_'; - - // First find the course context. - $coursecontext = $context->get_course_context(); - - $isfrontpage = ($coursecontext->instanceid == SITEID); - - $joins = array(); - $wheres = array(); - $params = array(); - - list($contextids, $contextpaths) = get_context_info_list($context); - - list($incontexts, $cparams) = $DB->get_in_or_equal($contextids, SQL_PARAMS_NAMED, 'ctx'); - - list($incaps, $capsparams) = $DB->get_in_or_equal($capability, SQL_PARAMS_NAMED, 'cap'); - - // Check whether context locking is enabled. - // Filter out any write capability if this is the case. - $excludelockedcaps = ''; - $excludelockedcapsparams = []; - if (!empty($CFG->contextlocking) && $context->locked) { - $excludelockedcaps = 'AND (cap.captype = :capread OR cap.name = :managelockscap)'; - $excludelockedcapsparams['capread'] = 'read'; - $excludelockedcapsparams['managelockscap'] = 'moodle/site:managecontextlocks'; - } - - $defs = array(); - $sql = "SELECT rc.id, rc.roleid, rc.permission, ctx.path - FROM {role_capabilities} rc - JOIN {capabilities} cap ON rc.capability = cap.name - JOIN {context} ctx on rc.contextid = ctx.id - WHERE rc.contextid $incontexts AND rc.capability $incaps $excludelockedcaps"; - $rcs = $DB->get_records_sql($sql, array_merge($cparams, $capsparams, $excludelockedcapsparams)); - foreach ($rcs as $rc) { - $defs[$rc->path][$rc->roleid] = $rc->permission; - } - - $access = array(); - if (!empty($defs)) { - foreach ($contextpaths as $path) { - if (empty($defs[$path])) { - continue; - } - foreach ($defs[$path] as $roleid => $perm) { - if ($perm == CAP_PROHIBIT) { - $access[$roleid] = CAP_PROHIBIT; - continue; - } - if (!isset($access[$roleid])) { - $access[$roleid] = (int) $perm; - } - } - } - } - - unset($defs); - - // Make lists of roles that are needed and prohibited. - $needed = array(); // One of these is enough. - $prohibited = array(); // Must not have any of these. - foreach ($access as $roleid => $perm) { - if ($perm == CAP_PROHIBIT) { - unset($needed[$roleid]); - $prohibited[$roleid] = true; - } else { - if ($perm == CAP_ALLOW and empty($prohibited[$roleid])) { - $needed[$roleid] = true; - } - } - } - - $defaultuserroleid = isset($CFG->defaultuserroleid) ? $CFG->defaultuserroleid : 0; - $defaultfrontpageroleid = isset($CFG->defaultfrontpageroleid) ? $CFG->defaultfrontpageroleid : 0; - - $nobody = false; - - if ($isfrontpage) { - if (!empty($prohibited[$defaultuserroleid]) or !empty($prohibited[$defaultfrontpageroleid])) { - $nobody = true; - } else { - if (!empty($needed[$defaultuserroleid]) or !empty($needed[$defaultfrontpageroleid])) { - // Everybody not having prohibit has the capability. - $needed = array(); - } else { - if (empty($needed)) { - $nobody = true; - } - } - } - } else { - if (!empty($prohibited[$defaultuserroleid])) { - $nobody = true; - } else { - if (!empty($needed[$defaultuserroleid])) { - // Everybody not having prohibit has the capability. - $needed = array(); - } else { - if (empty($needed)) { - $nobody = true; - } - } - } - } - - if ($nobody) { - // Nobody can match so return some SQL that does not return any results. - $wheres[] = "1 = 2"; - - } else { - - if ($needed) { - $ctxids = implode(',', $contextids); - $roleids = implode(',', array_keys($needed)); - $joins[] = "JOIN {role_assignments} {$prefix}ra3 - ON ({$prefix}ra3.userid = $useridcolumn - AND {$prefix}ra3.roleid IN ($roleids) - AND {$prefix}ra3.contextid IN ($ctxids))"; - } - - if ($prohibited) { - $ctxids = implode(',', $contextids); - $roleids = implode(',', array_keys($prohibited)); - $joins[] = "LEFT JOIN {role_assignments} {$prefix}ra4 - ON ({$prefix}ra4.userid = $useridcolumn - AND {$prefix}ra4.roleid IN ($roleids) - AND {$prefix}ra4.contextid IN ($ctxids))"; - $wheres[] = "{$prefix}ra4.id IS NULL"; - } - - } - - $wheres[] = "$useridcolumn <> :{$prefix}guestid"; - $params["{$prefix}guestid"] = $CFG->siteguest; - - $joins = implode("\n", $joins); - $wheres = "(" . implode(" AND ", $wheres) . ")"; - - return new \core\dml\sql_join($joins, $wheres, $params); -} diff --git a/lib/classes/dml/sql_join.php b/lib/classes/dml/sql_join.php index 1920a3fc7cc..1b2d2292c64 100644 --- a/lib/classes/dml/sql_join.php +++ b/lib/classes/dml/sql_join.php @@ -31,6 +31,14 @@ defined('MOODLE_INTERNAL') || die(); /** * An object that contains sql join fragments. * + * An example of how to use this class in a simple query, where you have got + * a join that is a join to the user table: + * + * $users = $DB->get_records_sql("SELECT u.* + * FROM {user} u + * {$sqljoin->joins} + * WHERE {$sqljoin->wheres}", $sqljoin->params); + * * @since Moodle 3.1 * @package core * @category dml @@ -54,16 +62,31 @@ class sql_join { */ public $params; + /** + * @var bool if true this join is guaranteed to never match any rows. + * In this case, the calling code may be able to completely + * skip doing the database query. + * @since Moodle 3.9/3.8.3/3.7.6. + */ + public $cannotmatchanyrows; + /** * Create an object that contains sql join fragments. * + * Note, even if you set $cannotmatchanyrows to true, it is + * important to also set the other fields because the calling + * code is not required to check it. For example + * new \core\dml\sql_join('', '1 = 2', [], true); + * * @param string $joins The join sql fragment. * @param string $wheres The where sql fragment. * @param array $params Any parameter values. + * @param bool $cannotmatchanyrows If true, this join is guaranteed to match no rows. See comment on the field above. */ - public function __construct($joins = '', $wheres = '', $params = array()) { + public function __construct($joins = '', $wheres = '', $params = array(), $cannotmatchanyrows = false) { $this->joins = $joins; $this->wheres = $wheres; $this->params = $params; + $this->cannotmatchanyrows = $cannotmatchanyrows; } } diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php index 8e6ce40f250..fcaf107df0a 100644 --- a/lib/tests/accesslib_test.php +++ b/lib/tests/accesslib_test.php @@ -3850,10 +3850,6 @@ class core_accesslib_testcase extends advanced_testcase { $this->assertFalse(array_key_exists($guest->id, $users)); } - /** - * Test updating of role capabilities during upgrade - * @return void - */ public function test_get_with_capability_sql() { global $DB; @@ -3905,6 +3901,73 @@ class core_accesslib_testcase extends advanced_testcase { $this->assertFalse(array_key_exists($guest->id, $users)); } + + /** + * Get the test cases for {@link test_get_with_capability_join_when_overrides_present()}. + * + * The particular capabilties used here do not really matter. What is important is + * that they are capabilities which the Student roles has by default, but the + * authenticated suser role does not. + * + * @return array + */ + public function get_get_with_capability_join_override_cases() { + return [ + 'no overrides' => [true, []], + 'one override' => [true, ['moodle/course:viewscales']], + 'both overrides' => [false, ['moodle/course:viewscales', 'moodle/question:flag']], + ]; + } + + /** + * Test get_with_capability_join. + * + * @dataProvider get_get_with_capability_join_override_cases + * + * @param bool $studentshouldbereturned whether, with this combination of capabilities, the student should be in the results. + * @param array $capabilitiestoprevent capabilities to override to prevent in the course context. + */ + public function test_get_with_capability_join_when_overrides_present( + bool $studentshouldbereturned, array $capabilitiestoprevent) { + global $DB; + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + + // Create a course. + $category = $generator->create_category(); + $course = $generator->create_course(['category' => $category->id]); + + // Create a user. + $student = $generator->create_user(); + $studentrole = $DB->get_record('role', ['shortname' => 'student'], '*', MUST_EXIST); + $generator->enrol_user($student->id, $course->id, $studentrole->id); + + // This test assumes that by default the student roles has the two + // capabilities. Check this now in case the role definitions are every changed. + $coursecontext = context_course::instance($course->id); + $this->assertTrue(has_capability('moodle/course:viewscales', $coursecontext, $student)); + $this->assertTrue(has_capability('moodle/question:flag', $coursecontext, $student)); + + // We test cases where there are a varying number of prevent overrides. + foreach ($capabilitiestoprevent as $capability) { + role_change_permission($studentrole->id, $coursecontext, $capability, CAP_PREVENT); + } + + // So now, assemble our query using the method under test, and verify that it returns the student. + $sqljoin = get_with_capability_join($coursecontext, + ['moodle/course:viewscales', 'moodle/question:flag'], 'u.id'); + + $users = $DB->get_records_sql("SELECT u.* + FROM {user} u + {$sqljoin->joins} + WHERE {$sqljoin->wheres}", $sqljoin->params); + if ($studentshouldbereturned) { + $this->assertEquals([$student->id], array_keys($users)); + } else { + $this->assertEmpty($users); + } + } + /** * Test the get_profile_roles() function. */ From 1736c599251403df6ad5002cfccee20ba4a8ad85 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 14 Apr 2020 22:29:17 +0100 Subject: [PATCH 2/3] MDL-68402 accesslib: Modernise coding style in the functions changed --- lib/accesslib.php | 132 +++++++++++++++++++++++++--------------------- 1 file changed, 72 insertions(+), 60 deletions(-) diff --git a/lib/accesslib.php b/lib/accesslib.php index 774ada6c0d0..c2b05a82465 100644 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -3456,13 +3456,13 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) $isfrontpage = $context->contextlevel == CONTEXT_COURSE && $context->instanceid == SITEID; $isfrontpage = $isfrontpage || is_inside_frontpage($context); - $caps = (array)$capability; + $caps = (array) $capability; - // construct list of context paths bottom-->top + // Construct list of context paths bottom --> top. list($contextids, $paths) = get_context_info_list($context); - // we need to find out all roles that have these capabilities either in definition or in overrides - $defs = array(); + // We need to find out all roles that have these capabilities either in definition or in overrides. + $defs = []; list($incontexts, $params) = $DB->get_in_or_equal($contextids, SQL_PARAMS_NAMED, $paramprefix . 'con'); list($incaps, $params2) = $DB->get_in_or_equal($caps, SQL_PARAMS_NAMED, $paramprefix . 'cap'); @@ -3488,9 +3488,9 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) $defs[$rc->capability][$rc->path][$rc->roleid] = $rc->permission; } - // go through the permissions bottom-->top direction to evaluate the current permission, - // first one wins (prohibit is an exception that always wins) - $access = array(); + // Go through the permissions bottom-->top direction to evaluate the current permission, + // first one wins (prohibit is an exception that always wins). + $access = []; foreach ($caps as $cap) { foreach ($paths as $path) { if (empty($defs[$cap][$path])) { @@ -3508,9 +3508,9 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) } } - // make lists of roles that are needed and prohibited in this context - $needed = array(); // one of these is enough - $prohibited = array(); // must not have any of these + // Make lists of roles that are needed and prohibited in this context. + $needed = []; // One of these is enough. + $prohibited = []; // Must not have any of these. foreach ($caps as $cap) { if (empty($access[$cap])) { continue; @@ -3524,11 +3524,11 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) } } if (empty($needed[$cap]) or !empty($prohibited[$cap][$defaultuserroleid])) { - // easy, nobody has the permission + // Easy, nobody has the permission. unset($needed[$cap]); unset($prohibited[$cap]); } else if ($isfrontpage and !empty($prohibited[$cap][$defaultfrontpageroleid])) { - // everybody is disqualified on the frontpage + // Everybody is disqualified on the frontpage. unset($needed[$cap]); unset($prohibited[$cap]); } @@ -3538,50 +3538,54 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) } if (empty($needed)) { - // there can not be anybody if no roles match this request + // There can not be anybody if no roles match this request. return new \core\dml\sql_join('', '1 = 2', [], true); } if (empty($prohibited)) { - // we can compact the needed roles - $n = array(); + // We can compact the needed roles. + $n = []; foreach ($needed as $cap) { - foreach ($cap as $roleid=>$unused) { + foreach ($cap as $roleid => $unused) { $n[$roleid] = true; } } - $needed = array('any'=>$n); + $needed = ['any' => $n]; unset($n); } - // Prepare query clauses - $wherecond = array(); - $params = array(); - $joins = array(); + // Prepare query clauses. + $wherecond = []; + $params = []; + $joins = []; + $cannotmatchanyrows = false; // We never return deleted users or guest account. - $deletedusercolumn = substr($useridcolumn, 0, -2) . 'deleted'; // Hack to get the deleted user column without an API change. + // Use a hack to get the deleted user column without an API change. + $deletedusercolumn = substr($useridcolumn, 0, -2) . 'deleted'; $wherecond[] = "$deletedusercolumn = 0 AND $useridcolumn <> :{$paramprefix}guestid"; $params[$paramprefix . 'guestid'] = $CFG->siteguest; - // now add the needed and prohibited roles conditions as joins + // Now add the needed and prohibited roles conditions as joins. if (!empty($needed['any'])) { - // simple case - there are no prohibits involved - if (!empty($needed['any'][$defaultuserroleid]) or ($isfrontpage and !empty($needed['any'][$defaultfrontpageroleid]))) { - // everybody + // Simple case - there are no prohibits involved. + if (!empty($needed['any'][$defaultuserroleid]) || + ($isfrontpage && !empty($needed['any'][$defaultfrontpageroleid]))) { + // Everybody. } else { $joins[] = "JOIN (SELECT DISTINCT userid FROM {role_assignments} WHERE contextid IN ($ctxids) - AND roleid IN (".implode(',', array_keys($needed['any'])) .") + AND roleid IN (" . implode(',', array_keys($needed['any'])) . ") ) ra ON ra.userid = $useridcolumn"; } } else { - $unions = array(); + $unions = []; $everybody = false; - foreach ($needed as $cap=>$unused) { + foreach ($needed as $cap => $unused) { if (empty($prohibited[$cap])) { - if (!empty($needed[$cap][$defaultuserroleid]) or ($isfrontpage and !empty($needed[$cap][$defaultfrontpageroleid]))) { + if (!empty($needed[$cap][$defaultuserroleid]) || + ($isfrontpage && !empty($needed[$cap][$defaultfrontpageroleid]))) { $everybody = true; break; } else { @@ -3591,43 +3595,51 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) AND roleid IN (".implode(',', array_keys($needed[$cap])) .")"; } } else { - if (!empty($prohibited[$cap][$defaultuserroleid]) or ($isfrontpage and !empty($prohibited[$cap][$defaultfrontpageroleid]))) { - // nobody can have this cap because it is prevented in default roles + if (!empty($prohibited[$cap][$defaultuserroleid]) || + ($isfrontpage && !empty($prohibited[$cap][$defaultfrontpageroleid]))) { + // Nobody can have this cap because it is prohibited in default roles. continue; - } else if (!empty($needed[$cap][$defaultuserroleid]) or ($isfrontpage and !empty($needed[$cap][$defaultfrontpageroleid]))) { - // everybody except the prohibitted - hiding does not matter + } else if (!empty($needed[$cap][$defaultuserroleid]) || + ($isfrontpage && !empty($needed[$cap][$defaultfrontpageroleid]))) { + // Everybody except the prohibited - hiding does not matter. $unions[] = "SELECT id AS userid FROM {user} WHERE id NOT IN (SELECT userid FROM {role_assignments} WHERE contextid IN ($ctxids) - AND roleid IN (".implode(',', array_keys($prohibited[$cap])) ."))"; + AND roleid IN (" . implode(',', array_keys($prohibited[$cap])) . "))"; } else { $unions[] = "SELECT userid FROM {role_assignments} - WHERE contextid IN ($ctxids) AND roleid IN (".implode(',', array_keys($needed[$cap])) .") + WHERE contextid IN ($ctxids) AND roleid IN (" . implode(',', array_keys($needed[$cap])) . ") AND userid NOT IN ( SELECT userid FROM {role_assignments} WHERE contextid IN ($ctxids) - AND roleid IN (" . implode(',', array_keys($prohibited[$cap])) . ") - )"; + AND roleid IN (" . implode(',', array_keys($prohibited[$cap])) . "))"; } } } + if (!$everybody) { if ($unions) { - $joins[] = "JOIN (SELECT DISTINCT userid FROM ( ".implode(' UNION ', $unions)." ) us) ra ON ra.userid = $useridcolumn"; + $joins[] = "JOIN ( + SELECT DISTINCT userid + FROM ( + " . implode("\n UNION \n", $unions) . " + ) us + ) ra ON ra.userid = $useridcolumn"; } else { - // only prohibits found - nobody can be matched + // Only prohibits found - nobody can be matched. $wherecond[] = "1 = 2"; + $cannotmatchanyrows = true; } } } - return new \core\dml\sql_join(implode("\n", $joins), implode(" AND ", $wherecond), $params); + return new \core\dml\sql_join(implode("\n", $joins), implode(" AND ", $wherecond), $params, $cannotmatchanyrows); } /** @@ -3649,8 +3661,8 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) * @param string|array $groups - single group or array of groups - only return * users who are in one of these group(s). * @param string|array $exceptions - list of users to exclude, comma separated or array - * @param bool $doanything_ignored not used any more, admin accounts are never returned - * @param bool $view_ignored - use get_enrolled_sql() instead + * @param bool $notuseddoanything not used any more, admin accounts are never returned + * @param bool $notusedview - use get_enrolled_sql() instead * @param bool $useviewallgroups if $groups is set the return users who * have capability both $capability and moodle/site:accessallgroups * in this context, as well as users who have $capability and who are @@ -3658,19 +3670,13 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) * @return array of user records */ function get_users_by_capability(context $context, $capability, $fields = '', $sort = '', $limitfrom = '', $limitnum = '', - $groups = '', $exceptions = '', $doanything_ignored = null, $view_ignored = null, $useviewallgroups = false) { + $groups = '', $exceptions = '', $notuseddoanything = null, $notusedview = null, $useviewallgroups = false) { global $CFG, $DB; - // Context is a course page other than the frontpage + // Context is a course page other than the frontpage. $iscoursepage = $context->contextlevel == CONTEXT_COURSE && $context->instanceid != SITEID; - // Get the bits of SQL relating to capabilities. - $sqljoin = get_with_capability_join($context, $capability, 'u.id'); - if ($sqljoin->cannotmatchanyrows) { - return []; - } - - // ***** Set up default fields ****** + // Set up default fields list if necessary. if (empty($fields)) { if ($iscoursepage) { $fields = 'u.*, ul.timeaccess AS lastaccess'; @@ -3683,7 +3689,7 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s } } - // Set up default sort + // Set up default sort if necessary. if (empty($sort)) { // default to course lastaccess or just lastaccess if ($iscoursepage) { $sort = 'ul.timeaccess'; @@ -3692,14 +3698,20 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s } } - // Prepare query clauses + // Get the bits of SQL relating to capabilities. + $sqljoin = get_with_capability_join($context, $capability, 'u.id'); + if ($sqljoin->cannotmatchanyrows) { + return []; + } + + // Prepare query clauses. $wherecond = [$sqljoin->wheres]; $params = $sqljoin->params; $joins = [$sqljoin->joins]; - // User lastaccess JOIN + // Add user lastaccess JOIN, if required. if ((strpos($sort, 'ul.timeaccess') === false) and (strpos($fields, 'ul.timeaccess') === false)) { - // user_lastaccess is not required MDL-13810 + // Here user_lastaccess is not required MDL-13810. } else { if ($iscoursepage) { $joins[] = "LEFT OUTER JOIN {user_lastaccess} ul ON (ul.userid = u.id AND ul.courseid = {$context->instanceid})"; @@ -3708,7 +3720,7 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s } } - // Groups + // Groups. if ($groups) { $groups = (array)$groups; list($grouptest, $grpparams) = $DB->get_in_or_equal($groups, SQL_PARAMS_NAMED, 'grp'); @@ -3729,7 +3741,7 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s $wherecond[] = "($grouptest)"; } - // User exceptions + // User exceptions. if (!empty($exceptions)) { $exceptions = (array)$exceptions; list($exsql, $exparams) = $DB->get_in_or_equal($exceptions, SQL_PARAMS_NAMED, 'exc', false); @@ -3737,14 +3749,14 @@ function get_users_by_capability(context $context, $capability, $fields = '', $s $wherecond[] = "u.id $exsql"; } - // Collect WHERE conditions and needed joins + // Collect WHERE conditions and needed joins. $where = implode(' AND ', $wherecond); if ($where !== '') { $where = 'WHERE ' . $where; } $joins = implode("\n", $joins); - // Ok, let's get the users! + // Finally! we have all the bits, run the query. $sql = "SELECT $fields FROM {user} u $joins From 1ed243c2fef999dd23b5c36fe62171ad6392a493 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 6 May 2020 09:14:49 +0100 Subject: [PATCH 3/3] MDL-68402 accesslib: fix coding style of loops --- lib/accesslib.php | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/accesslib.php b/lib/accesslib.php index c2b05a82465..1e56b09d145 100644 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -776,7 +776,7 @@ function has_capability_in_accessdata($capability, context $context, array &$acc // Build $paths as a list of current + all parent "paths" with order bottom-to-top $path = $context->path; $paths = array($path); - while($path = rtrim($path, '0123456789')) { + while ($path = rtrim($path, '0123456789')) { $path = rtrim($path, '/'); if ($path === '') { break; @@ -1106,7 +1106,7 @@ function remove_temp_course_roles(context_course $coursecontext) { $ras = $DB->get_records_sql($sql, array('contextid'=>$coursecontext->id, 'userid'=>$USER->id)); $USER->access['ra'][$coursecontext->path] = array(); - foreach($ras as $r) { + foreach ($ras as $r) { $USER->access['ra'][$coursecontext->path][(int)$r->id] = (int)$r->id; } } @@ -1663,7 +1663,7 @@ function role_unassign_all(array $params, $subcontexts = false, $includemanual = } $ras = $DB->get_records('role_assignments', $params); - foreach($ras as $ra) { + foreach ($ras as $ra) { $DB->delete_records('role_assignments', array('id'=>$ra->id)); if ($context = context::instance_by_id($ra->contextid, IGNORE_MISSING)) { // Role assignments have changed, so mark user as dirty. @@ -1697,10 +1697,10 @@ function role_unassign_all(array $params, $subcontexts = false, $includemanual = if ($context) { $contexts = $context->get_child_contexts(); $mparams = $params; - foreach($contexts as $context) { + foreach ($contexts as $context) { $mparams['contextid'] = $context->id; $ras = $DB->get_records('role_assignments', $mparams); - foreach($ras as $ra) { + foreach ($ras as $ra) { $DB->delete_records('role_assignments', array('id'=>$ra->id)); // Role assignments have changed, so mark user as dirty. mark_user_dirty($ra->userid); @@ -1978,7 +1978,7 @@ function can_access_course(stdClass $course, $user = null, $withcapability = '', // if not enrolled try to gain temporary guest access $instances = $DB->get_records('enrol', array('courseid'=>$course->id, 'status'=>ENROL_INSTANCE_ENABLED), 'sortorder, id ASC'); $enrols = enrol_get_plugins(true); - foreach($instances as $instance) { + foreach ($instances as $instance) { if (!isset($enrols[$instance->enrol])) { continue; } @@ -2067,7 +2067,7 @@ function get_default_capabilities($archetype) { $alldefs = array_merge($alldefs, load_capability_def($cap['component'])); } } - foreach($alldefs as $name=>$def) { + foreach ($alldefs as $name=>$def) { // Use array 'archetypes if available. Only if not specified, use 'legacy'. if (isset($def['archetypes'])) { if (isset($def['archetypes'][$archetype])) { @@ -2188,7 +2188,7 @@ function reset_role_capabilities($roleid) { $DB->delete_records('role_capabilities', array('roleid' => $roleid, 'contextid' => $systemcontext->id)); - foreach($defaultcaps as $cap=>$permission) { + foreach ($defaultcaps as $cap=>$permission) { assign_capability($cap, $permission, $roleid, $systemcontext->id); } @@ -2215,7 +2215,7 @@ function update_capabilities($component = 'moodle') { $storedcaps = array(); $filecaps = load_capability_def($component); - foreach($filecaps as $capname=>$unused) { + foreach ($filecaps as $capname=>$unused) { if (!preg_match('|^[a-z]+/[a-z_0-9]+:[a-z_0-9]+$|', $capname)) { debugging("Coding problem: Invalid capability name '$capname', use 'clonepermissionsfrom' field for migration."); } @@ -2345,7 +2345,7 @@ function capabilities_cleanup($component, $newcapdef = null) { // Delete from roles. if ($roles = get_roles_with_capability($cachedcap->name)) { - foreach($roles as $role) { + foreach ($roles as $role) { if (!unassign_capability($cachedcap->name, $role->id)) { print_error('cannotunassigncap', 'error', '', (object)array('cap'=>$cachedcap->name, 'role'=>$role->name)); } @@ -3496,7 +3496,7 @@ function get_with_capability_join(context $context, $capability, $useridcolumn) if (empty($defs[$cap][$path])) { continue; } - foreach($defs[$cap][$path] as $roleid => $perm) { + foreach ($defs[$cap][$path] as $roleid => $perm) { if ($perm == CAP_PROHIBIT) { $access[$cap][$roleid] = CAP_PROHIBIT; continue; @@ -4105,7 +4105,7 @@ function get_user_capability_course($capability, $userid = null, $doanything = t $fieldlist = ''; if ($fieldsexceptid) { $fields = array_map('trim', explode(',', $fieldsexceptid)); - foreach($fields as $field) { + foreach ($fields as $field) { // Context fields have a different alias. if (strpos($field, 'ctx') === 0) { switch($field) { @@ -4128,7 +4128,7 @@ function get_user_capability_course($capability, $userid = null, $doanything = t if ($orderby) { $fields = explode(',', $orderby); $orderby = ''; - foreach($fields as $field) { + foreach ($fields as $field) { if ($orderby) { $orderby .= ','; } @@ -4618,7 +4618,7 @@ function get_roles_with_cap_in_context($context, $capability) { $forbidden = array(); $needed = array(); - foreach($capdefs as $def) { + foreach ($capdefs as $def) { if (isset($forbidden[$def->roleid])) { continue; } @@ -4638,7 +4638,7 @@ function get_roles_with_cap_in_context($context, $capability) { unset($capdefs); // remove all those roles not allowing - foreach($needed as $key=>$value) { + foreach ($needed as $key=>$value) { if (!$value) { unset($needed[$key]); } else { @@ -4660,7 +4660,7 @@ function get_roles_with_cap_in_context($context, $capability) { function get_roles_with_caps_in_context($context, $capabilities) { $neededarr = array(); $forbiddenarr = array(); - foreach($capabilities as $caprequired) { + foreach ($capabilities as $caprequired) { list($neededarr[], $forbiddenarr[]) = get_roles_with_cap_in_context($context, $caprequired); } @@ -4774,7 +4774,7 @@ function role_change_permission($roleid, $context, $capname, $permission) { ORDER BY ctx.depth DESC"; if ($existing = $DB->get_records_sql($sql, $params)) { - foreach($existing as $e) { + foreach ($existing as $e) { if ($e->permission == CAP_PROHIBIT) { // prohibit can not be overridden, no point in changing anything return; @@ -4931,7 +4931,7 @@ abstract class context extends stdClass implements IteratorAggregate { if (self::$cache_count >= CONTEXT_CACHE_MAX_SIZE) { $i = 0; - foreach(self::$cache_contextsbyid as $ctx) { + foreach (self::$cache_contextsbyid as $ctx) { $i++; if ($i <= 100) { // we want to keep the first contexts to be loaded on this page, hopefully they will be needed again later