MDL-59897 Accesslib: get_user_capability_course is slow
authorsam marshall <s.marshall@open.ac.uk>
Tue, 22 Aug 2017 17:09:35 +0000 (18:09 +0100)
committersam marshall <s.marshall@open.ac.uk>
Fri, 6 Oct 2017 12:57:55 +0000 (13:57 +0100)
lib/accesslib.php
lib/classes/access/get_user_capability_course_helper.php [new file with mode: 0644]
lib/tests/accesslib_test.php

index 587a61f..8c7c52a 100644 (file)
@@ -3784,7 +3784,9 @@ function count_role_users($roleid, context $context, $parent = false) {
 
 /**
  * This function gets the list of courses that this user has a particular capability in.
- * It is still not very efficient.
+ *
+ * It is now reasonably efficient, but bear in mind that if there are users who have the capability
+ * everywhere, it may return an array of all courses.
  *
  * @param string $capability Capability in question
  * @param int $userid User ID or null for current user
@@ -3799,15 +3801,51 @@ function count_role_users($roleid, context $context, $parent = false) {
  */
 function get_user_capability_course($capability, $userid = null, $doanything = true, $fieldsexceptid = '', $orderby = '',
         $limit = 0) {
-    global $DB;
+    global $DB, $USER;
+
+    // Default to current user.
+    if (!$userid) {
+        $userid = $USER->id;
+    }
+
+    if ($doanything && is_siteadmin($userid)) {
+        // If the user is a site admin and $doanything is enabled then there is no need to restrict
+        // the list of courses.
+        $contextlimitsql = '';
+        $contextlimitparams = [];
+    } else {
+        // Gets SQL to limit contexts ('x' table) to those where the user has this capability.
+        list ($contextlimitsql, $contextlimitparams) = \core\access\get_user_capability_course_helper::get_sql(
+                $userid, $capability);
+        if (!$contextlimitsql) {
+            // If the does not have this capability in any context, return false without querying.
+            return false;
+        }
+
+        $contextlimitsql = 'WHERE' . $contextlimitsql;
+        unset($root);
+    }
 
     // Convert fields list and ordering
     $fieldlist = '';
     if ($fieldsexceptid) {
         $fields = array_map('trim', explode(',', $fieldsexceptid));
         foreach($fields as $field) {
-            // Context fields have a different alias and are added further down.
-            if (strpos($field, 'ctx') !== 0) {
+            // Context fields have a different alias.
+            if (strpos($field, 'ctx') === 0) {
+                switch($field) {
+                    case 'ctxlevel' :
+                        $realfield = 'contextlevel';
+                        break;
+                    case 'ctxinstance' :
+                        $realfield = 'instanceid';
+                        break;
+                    default:
+                        $realfield = substr($field, 3);
+                        break;
+                }
+                $fieldlist .= ',x.' . $realfield . ' AS ' . $field;
+            } else {
                 $fieldlist .= ',c.'.$field;
             }
         }
@@ -3824,43 +3862,18 @@ function get_user_capability_course($capability, $userid = null, $doanything = t
         $orderby = 'ORDER BY '.$orderby;
     }
 
-    // Obtain a list of everything relevant about all courses including context.
-    // Note the result can be used directly as a context (we are going to), the course
-    // fields are just appended.
-
-    $contextpreload = context_helper::get_preload_record_columns_sql('x');
-
-    $contextlist = ['ctxid', 'ctxpath', 'ctxdepth', 'ctxlevel', 'ctxinstance'];
-    $unsetitems = $contextlist;
-    if ($fieldsexceptid) {
-        $coursefields = array_map('trim', explode(',', $fieldsexceptid));
-        $unsetitems = array_diff($contextlist, $coursefields);
-    }
-
     $courses = array();
-    $rs = $DB->get_recordset_sql("SELECT c.id $fieldlist, $contextpreload
-                                    FROM {course} c
-                                    JOIN {context} x ON (c.id=x.instanceid AND x.contextlevel=".CONTEXT_COURSE.")
-                                $orderby");
-    // Check capability for each course in turn
+    $rs = $DB->get_recordset_sql("
+            SELECT c.id $fieldlist
+              FROM {course} c
+              JOIN {context} x ON c.id = x.instanceid AND x.contextlevel = ?
+            $contextlimitsql
+            $orderby", array_merge([CONTEXT_COURSE], $contextlimitparams));
     foreach ($rs as $course) {
-        // The preload_from_record() unsets the context related fields, but it is possible that our caller may
-        // want them returned too (so they can use them for their own context preloading). For that reason we
-        // pass a clone.
-        context_helper::preload_from_record(clone($course));
-        $context = context_course::instance($course->id);
-        if (has_capability($capability, $context, $userid, $doanything)) {
-            // Unset context fields if they were not asked for.
-            foreach ($unsetitems as $item) {
-                unset($course->$item);
-            }
-            // We've got the capability. Make the record look like a course record
-            // and store it
-            $courses[] = $course;
-            $limit--;
-            if ($limit == 0) {
-                break;
-            }
+        $courses[] = $course;
+        $limit--;
+        if ($limit == 0) {
+            break;
         }
     }
     $rs->close();
diff --git a/lib/classes/access/get_user_capability_course_helper.php b/lib/classes/access/get_user_capability_course_helper.php
new file mode 100644 (file)
index 0000000..80b5b45
--- /dev/null
@@ -0,0 +1,370 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Helper functions to implement the complex get_user_capability_course function.
+ *
+ * @package core
+ * @copyright 2017 The Open University
+ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+namespace core\access;
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * Helper functions to implement the complex get_user_capability_course function.
+ *
+ * @package core
+ * @copyright 2017 The Open University
+ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class get_user_capability_course_helper {
+    /**
+     * Based on the given user's access data (roles) and system role definitions, works out
+     * an array of capability values at each relevant context for the given user and capability.
+     *
+     * This is organised by the effective context path (the one at which the capability takes
+     * effect) and then by role id.
+     *
+     * @param int $userid User id
+     * @param string $capability Capability e.g. 'moodle/course:view'
+     * @return array Array of capability constants, indexed by context path and role id
+     */
+    protected static function get_capability_info_at_each_context($userid, $capability) {
+        // Get access data for user.
+        $accessdata = get_user_accessdata($userid);
+
+        // Get list of roles for user (any location) and information about these roles.
+        $roleids = [];
+        foreach ($accessdata['ra'] as $path => $roles) {
+            foreach ($roles as $roleid) {
+                $roleids[$roleid] = true;
+            }
+        }
+        $rdefs = get_role_definitions(array_keys($roleids));
+
+        // Get data for required capability at each context path where the user has a role that can
+        // affect it.
+        $systemcontext = \context_system::instance();
+        $pathroleperms = [];
+        foreach ($accessdata['ra'] as $userpath => $roles) {
+            foreach ($roles as $roleid) {
+                // Get role definition for that role.
+                foreach ($rdefs[$roleid] as $rolepath => $caps) {
+                    // Ignore if this override/definition doesn't refer to the relevant cap.
+                    if (!array_key_exists($capability, $caps)) {
+                        continue;
+                    }
+
+                    // Check path is /1 or matches a path the user has.
+                    if ($rolepath === '/' . $systemcontext->id) {
+                        // Note /1 is listed first in the array so this entry will be overridden
+                        // if there is an override for the role on this actual level.
+                        $effectivepath = $userpath;
+                    } else if (preg_match('~^' . $userpath . '($|/)~', $rolepath)) {
+                        $effectivepath = $rolepath;
+                    } else {
+                        // Not inside an area where the user has the role, so ignore.
+                        continue;
+                    }
+
+                    if (!array_key_exists($effectivepath, $pathroleperms)) {
+                        $pathroleperms[$effectivepath] = [];
+                    }
+                    $pathroleperms[$effectivepath][$roleid] = $caps[$capability];
+                }
+            }
+        }
+
+        return $pathroleperms;
+    }
+
+    /**
+     * Calculates a permission tree based on an array of information about role permissions.
+     *
+     * The input parameter must be in the format returned by get_capability_info_at_each_context.
+     *
+     * The output is the root of a tree of stdClass objects with the fields 'path' (a context path),
+     * 'allow' (true or false), and 'children' (an array of similar objects).
+     *
+     * @param array $pathroleperms Array of permissions
+     * @return \stdClass Root object of permission tree
+     */
+    protected static function calculate_permission_tree(array $pathroleperms) {
+        // Considering each discovered context path as an inflection point, evaluate the user's
+        // permission (based on all roles) at each point.
+        $pathallows = [];
+        $mindepth = 1000;
+        $maxdepth = 0;
+        foreach ($pathroleperms as $path => $roles) {
+            $evaluatedroleperms = [];
+
+            // Walk up the tree starting from this path.
+            $innerpath = $path;
+            while ($innerpath !== '') {
+                $roles = $pathroleperms[$innerpath];
+
+                // Evaluate roles at this path level.
+                foreach ($roles as $roleid => $perm) {
+                    if (!array_key_exists($roleid, $evaluatedroleperms)) {
+                        $evaluatedroleperms[$roleid] = $perm;
+                    } else {
+                        // The existing one is at a more specific level so it takes precedence
+                        // UNLESS this is a prohibit.
+                        if ($perm == CAP_PROHIBIT) {
+                            $evaluatedroleperms[$roleid] = $perm;
+                        }
+                    }
+                }
+
+                // Go up to next path level (if any).
+                do {
+                    $innerpath = substr($innerpath, 0, strrpos($innerpath, '/'));
+                    if ($innerpath === '') {
+                        // No higher level data.
+                        break;
+                    }
+                } while (!array_key_exists($innerpath, $pathroleperms));
+            }
+
+            // If we have an allow from any role, and no prohibits, then user can access this path,
+            // else not.
+            $allow = false;
+            foreach ($evaluatedroleperms as $perm) {
+                if ($perm == CAP_ALLOW) {
+                    $allow = true;
+                } else if ($perm == CAP_PROHIBIT) {
+                    $allow = false;
+                    break;
+                }
+            }
+
+            // Store the result based on path and depth so that we can process in depth order in
+            // the next step.
+            $depth = strlen(preg_replace('~[^/]~', '', $path));
+            $mindepth = min($depth, $mindepth);
+            $maxdepth = max($depth, $maxdepth);
+            $pathallows[$depth][$path] = $allow;
+        }
+
+        // Organise into a tree structure, processing in depth order so that we have ancestors
+        // set up before we encounter their children.
+        $root = (object)['allow' => false, 'path' => null, 'children' => []];
+        $nodesbypath = [];
+        for ($depth = $mindepth; $depth <= $maxdepth; $depth++) {
+            // Skip any missing depth levels.
+            if (!array_key_exists($depth, $pathallows)) {
+                continue;
+            }
+            foreach ($pathallows[$depth] as $path => $allow) {
+                // Value for new tree node.
+                $leaf = (object)['allow' => $allow, 'path' => $path, 'children' => []];
+
+                // Try to find a place to join it on if there is one.
+                $ancestorpath = $path;
+                $found = false;
+                while ($ancestorpath) {
+                    $ancestorpath = substr($ancestorpath, 0, strrpos($ancestorpath, '/'));
+                    if (array_key_exists($ancestorpath, $nodesbypath)) {
+                        $found = true;
+                        break;
+                    }
+                }
+
+                if ($found) {
+                    $nodesbypath[$ancestorpath]->children[] = $leaf;
+                } else {
+                    $root->children[] = $leaf;
+                }
+                $nodesbypath[$path] = $leaf;
+            }
+        }
+
+        return $root;
+    }
+
+    /**
+     * Given a permission tree (in calculate_permission_tree format), removes any subtrees that
+     * are negative from the root. For example, if a top-level node of the permission tree has
+     * 'false' permission then it is meaningless because the default permission is already false;
+     * this function will remove it. However, if there is a child within that node that is positive,
+     * then that will need to be kept.
+     *
+     * @param \stdClass $root Root object
+     * @return \stdClass Filtered tree root
+     */
+    protected static function remove_negative_subtrees($root) {
+        // If a node 'starts' negative, we don't need it (as negative is the default) - extract only
+        // subtrees that start with a positive value.
+        $positiveroot = (object)['allow' => false, 'path' => null, 'children' => []];
+        $consider = [$root];
+        while ($consider) {
+            $first = array_shift($consider);
+            foreach ($first->children as $node) {
+                if ($node->allow) {
+                    // Add directly to new root.
+                    $positiveroot->children[] = $node;
+                } else {
+                    // Consider its children for adding to root (if there are any positive ones).
+                    $consider[] = $node;
+                }
+            }
+        }
+        return $positiveroot;
+    }
+
+    /**
+     * Removes duplicate nodes of a tree - where a child node has the same permission as its
+     * parent.
+     *
+     * @param \stdClass $parent Tree root node
+     */
+    protected static function remove_duplicate_nodes($parent) {
+        $length = count($parent->children);
+        $index = 0;
+        while ($index < $length) {
+            $child = $parent->children[$index];
+            if ($child->allow === $parent->allow) {
+                // Remove child node, but add its children to this node instead.
+                array_splice($parent->children, $index, 1);
+                $length--;
+                $index--;
+                foreach ($child->children as $grandchild) {
+                    $parent->children[] = $grandchild;
+                    $length++;
+                }
+            } else {
+                // Keep child node, but recurse to remove its unnecessary children.
+                self::remove_duplicate_nodes($child);
+            }
+            $index++;
+        }
+    }
+
+    /**
+     * Gets a permission tree for the given user and capability, representing the value of that
+     * capability at different contexts across the system. The tree will be simplified as far as
+     * possible.
+     *
+     * The output is the root of a tree of stdClass objects with the fields 'path' (a context path),
+     * 'allow' (true or false), and 'children' (an array of similar objects).
+     *
+     * @param int $userid User id
+     * @param string $capability Capability e.g. 'moodle/course:view'
+     * @return \stdClass Root node of tree
+     */
+    protected static function get_tree($userid, $capability) {
+        // Extract raw capability data for this user and capability.
+        $pathroleperms = self::get_capability_info_at_each_context($userid, $capability);
+
+        // Convert the raw data into a permission tree based on context.
+        $root = self::calculate_permission_tree($pathroleperms);
+        unset($pathroleperms);
+
+        // Simplify the permission tree by removing unnecessary nodes.
+        $root = self::remove_negative_subtrees($root);
+        self::remove_duplicate_nodes($root);
+
+        // Return the tree.
+        return $root;
+    }
+
+    /**
+     * Creates SQL suitable for restricting by contexts listed in the given permission tree.
+     *
+     * This function relies on the permission tree being in the format created by get_tree.
+     * Specifically, all the children of the root element must be set to 'allow' permission,
+     * children of those children must be 'not allow', children of those grandchildren 'allow', etc.
+     *
+     * @param \stdClass $parent Root node of permission tree
+     * @return array Two-element array of SQL (containing ? placeholders) and then a params array
+     */
+    protected static function create_sql($parent) {
+        global $DB;
+
+        $sql = '';
+        $params = [];
+        if ($parent->path !== null) {
+            // Except for the root element, create the condition that it applies to the context of
+            // this element (or anything within it).
+            $sql = ' (x.path = ? OR ' . $DB->sql_like('x.path', '?') .')';
+            $params[] = $parent->path;
+            $params[] = $parent->path . '/%';
+            if ($parent->children) {
+                // When there are children, these are assumed to have the opposite sign i.e. if we
+                // are allowing the parent, we are not allowing the children, and vice versa. So
+                // the 'OR' clause for children will be inside this 'AND NOT'.
+                $sql .= ' AND NOT (';
+            }
+        } else if (count($parent->children) > 1) {
+            // Place brackets in the query when it is going to be an OR of multiple conditions.
+            $sql .= ' (';
+        }
+        if ($parent->children) {
+            $first = true;
+            foreach ($parent->children as $child) {
+                if ($first) {
+                    $first = false;
+                } else {
+                    $sql  .= ' OR';
+                }
+
+                // Recuse to get the child requirements - this will be the check that the context
+                // is within the child, plus possibly and 'AND NOT' for any different contexts
+                // within the child.
+                list ($childsql, $childparams) = self::create_sql($child);
+                $sql .= $childsql;
+                $params = array_merge($params, $childparams);
+            }
+            // Close brackets if opened above.
+            if ($parent->path !== null || count($parent->children) > 1) {
+                $sql .= ')';
+            }
+        }
+        return [$sql, $params];
+    }
+
+    /**
+     * Gets SQL to restrict a query to contexts in which the user has a capability.
+     *
+     * This returns an array with two elements (SQL containing ? placeholders, and a params array).
+     * The SQL is intended to be used as part of a WHERE clause. It relies on the prefix 'x' being
+     * used for the Moodle context table.
+     *
+     * If the user does not have the permission anywhere at all (so that there is no point doing
+     * the query) then the two returned values will both be false.
+     *
+     * @param int $userid User id
+     * @param string $capability Capability e.g. 'moodle/course:view'
+     * @return array Two-element array of SQL (containing ? placeholders) and then a params array
+     */
+    public static function get_sql($userid, $capability) {
+        // Get a tree of capability permission at various contexts for current user.
+        $root = self::get_tree($userid, $capability);
+
+        // The root node always has permission false. If there are no child nodes then the user
+        // cannot access anything.
+        if (!$root->children) {
+            return [false, false];
+        }
+
+        // Get SQL to limit contexts based on the permission tree.
+        return self::create_sql($root);
+
+    }
+}
index f192822..9e27b4b 100644 (file)
@@ -1696,6 +1696,165 @@ class core_accesslib_testcase extends advanced_testcase {
         $this->assertFalse(has_all_capabilities($sca, $coursecontext, 0));
     }
 
+    /**
+     * Tests get_user_capability_course() which checks a capability across all courses.
+     */
+    public function test_get_user_capability_course() {
+        global $CFG, $USER;
+
+        $this->resetAfterTest();
+
+        $generator = $this->getDataGenerator();
+        $cap = 'moodle/course:view';
+
+        // Create a role which allows course:view and one that prohibits it, and one neither.
+        $allowroleid = $generator->create_role();
+        $prohibitroleid = $generator->create_role();
+        $emptyroleid = $generator->create_role();
+        $systemcontext = context_system::instance();
+        assign_capability($cap, CAP_ALLOW, $allowroleid, $systemcontext->id);
+        assign_capability($cap, CAP_PROHIBIT, $prohibitroleid, $systemcontext->id);
+
+        // Create two categories (nested).
+        $cat1 = $generator->create_category();
+        $cat2 = $generator->create_category(['parent' => $cat1->id]);
+
+        // Create six courses - two in cat1, two in cat2, and two in default category.
+        $c1 = $generator->create_course(['category' => $cat1->id, 'shortname' => 'Z']);
+        $c2 = $generator->create_course(['category' => $cat1->id, 'shortname' => 'Y']);
+        $c3 = $generator->create_course(['category' => $cat2->id, 'shortname' => 'X']);
+        $c4 = $generator->create_course(['category' => $cat2->id]);
+        $c5 = $generator->create_course();
+        $c6 = $generator->create_course();
+
+        // Category overrides: in cat 1, empty role is allowed; in cat 2, empty role is prevented.
+        assign_capability($cap, CAP_ALLOW, $emptyroleid,
+                context_coursecat::instance($cat1->id)->id);
+        assign_capability($cap, CAP_PREVENT, $emptyroleid,
+                context_coursecat::instance($cat2->id)->id);
+
+        // Course overrides: in C5, allow role is prevented; in C6, empty role is prohibited; in
+        // C3, empty role is allowed.
+        assign_capability($cap, CAP_PREVENT, $allowroleid,
+                context_course::instance($c5->id)->id);
+        assign_capability($cap, CAP_PROHIBIT, $emptyroleid,
+                context_course::instance($c6->id)->id);
+        assign_capability($cap, CAP_ALLOW, $emptyroleid,
+                context_course::instance($c3->id)->id);
+
+        // User 1 has no roles except default user role.
+        $u1 = $generator->create_user();
+
+        // It returns false (annoyingly) if there are no courses.
+        $this->assertFalse(get_user_capability_course($cap, $u1->id, true, '', 'id'));
+
+        // Final override: in C1, default user role is allowed.
+        assign_capability($cap, CAP_ALLOW, $CFG->defaultuserroleid,
+                context_course::instance($c1->id)->id);
+
+        // Should now get C1 only.
+        $courses = get_user_capability_course($cap, $u1->id, true, '', 'id');
+        $this->assert_course_ids([$c1->id], $courses);
+
+        // User 2 has allow role (system wide).
+        $u2 = $generator->create_user();
+        role_assign($allowroleid, $u2->id, $systemcontext->id);
+
+        // Should get everything except C5.
+        $courses = get_user_capability_course($cap, $u2->id, true, '', 'id');
+        $this->assert_course_ids([SITEID, $c1->id, $c2->id, $c3->id, $c4->id, $c6->id], $courses);
+
+        // User 3 has empty role (system wide).
+        $u3 = $generator->create_user();
+        role_assign($emptyroleid, $u3->id, $systemcontext->id);
+
+        // Should get cat 1 courses but not cat2, except C3.
+        $courses = get_user_capability_course($cap, $u3->id, true, '', 'id');
+        $this->assert_course_ids([$c1->id, $c2->id, $c3->id], $courses);
+
+        // User 4 has allow and empty role (system wide).
+        $u4 = $generator->create_user();
+        role_assign($allowroleid, $u4->id, $systemcontext->id);
+        role_assign($emptyroleid, $u4->id, $systemcontext->id);
+
+        // Should get everything except C5 and C6.
+        $courses = get_user_capability_course($cap, $u4->id, true, '', 'id');
+        $this->assert_course_ids([SITEID, $c1->id, $c2->id, $c3->id, $c4->id], $courses);
+
+        // User 5 has allow role in default category only.
+        $u5 = $generator->create_user();
+        role_assign($allowroleid, $u5->id, context_coursecat::instance($c5->category)->id);
+
+        // Should get C1 and the default category courses but not C5.
+        $courses = get_user_capability_course($cap, $u5->id, true, '', 'id');
+        $this->assert_course_ids([$c1->id, $c6->id], $courses);
+
+        // User 6 has a bunch of course roles: prohibit role in C1, empty role in C3, allow role in
+        // C6.
+        $u6 = $generator->create_user();
+        role_assign($prohibitroleid, $u6->id, context_course::instance($c1->id)->id);
+        role_assign($emptyroleid, $u6->id, context_course::instance($c3->id)->id);
+        role_assign($allowroleid, $u6->id, context_course::instance($c5->id)->id);
+
+        // Should get C3 only because the allow role is prevented in C5.
+        $courses = get_user_capability_course($cap, $u6->id, true, '', 'id');
+        $this->assert_course_ids([$c3->id], $courses);
+
+        // Admin user gets everything....
+        $courses = get_user_capability_course($cap, get_admin()->id, true, '', 'id');
+        $this->assert_course_ids([SITEID, $c1->id, $c2->id, $c3->id, $c4->id, $c5->id, $c6->id],
+                $courses);
+
+        // Unless you turn off doanything, when it only has the things a user with no role does.
+        $courses = get_user_capability_course($cap, get_admin()->id, false, '', 'id');
+        $this->assert_course_ids([$c1->id], $courses);
+
+        // Using u3 as an example, test the limit feature.
+        $courses = get_user_capability_course($cap, $u3->id, true, '', 'id', 2);
+        $this->assert_course_ids([$c1->id, $c2->id], $courses);
+
+        // Check sorting.
+        $courses = get_user_capability_course($cap, $u3->id, true, '', 'shortname');
+        $this->assert_course_ids([$c3->id, $c2->id, $c1->id], $courses);
+
+        // Check returned fields - default.
+        $courses = get_user_capability_course($cap, $u3->id, true, '', 'id');
+        $this->assertEquals((object)['id' => $c1->id], $courses[0]);
+
+        // Add a selection of fields, including the context ones with special handling.
+        $courses = get_user_capability_course($cap, $u3->id, true, 'shortname, ctxlevel, ctxdepth, ctxinstance', 'id');
+        $this->assertEquals((object)['id' => $c1->id, 'shortname' => 'Z', 'ctxlevel' => 50,
+                'ctxdepth' => 3, 'ctxinstance' => $c1->id], $courses[0]);
+
+        // Test front page role - user 1 has no roles, but if we change the front page role
+        // definition so that it has our capability, then they should see the front page course.
+        // as well as C1.
+        assign_capability($cap, CAP_ALLOW, $CFG->defaultfrontpageroleid, $systemcontext->id);
+        $courses = get_user_capability_course($cap, $u1->id, true, '', 'id');
+        $this->assert_course_ids([SITEID, $c1->id], $courses);
+
+        // Check that temporary guest access (in this case, given on course 2 for user 1)
+        // also is included, if it has this capability.
+        assign_capability($cap, CAP_ALLOW, $CFG->guestroleid, $systemcontext->id);
+        $this->setUser($u1);
+        load_temp_course_role(context_course::instance($c2->id), $CFG->guestroleid);
+        $courses = get_user_capability_course($cap, $USER->id, true, '', 'id');
+        $this->assert_course_ids([SITEID, $c1->id, $c2->id], $courses);
+    }
+
+    /**
+     * Extracts an array of course ids to make the above test script shorter.
+     *
+     * @param int[] $expected Array of expected course ids
+     * @param stdClass[] $courses Array of course objects
+     */
+    protected function assert_course_ids(array $expected, array $courses) {
+        $courseids = array_map(function($c) {
+            return $c->id;
+        }, $courses);
+        $this->assertEquals($expected, $courseids);
+    }
+
     /**
      * Test if course creator future capability lookup works.
      */