MDL-60983 get_user_capability_course: fix buggy edge cases

There were basically two problems, which are demostrated by
the new test users u7 and u8 in the unit test.

1. There was a problem if a role was overridden in a context above the
one where it was assigned. E.g. User has teacher role in a course, and
there is a role override in the course-category context. That was being
ignored.

2. Problems with the handling of PROHIBITs. It should be the case that
if there is a PROHIBIT in force, then it cannot be overridden by another
role, or by a role override. However, this was not working in all cases.

Also, I had to add comments to the unit test so that I could understand
it. Hopefully these will be hepful to other developers too.

lib/classes/access/get_user_capability_course_helper.php

@@ -39,7 +39,10 @@ class get_user_capability_course_helper {
      * 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.
+     * effect) and then by role id. Note, however, that the resulting array only has
+     * the information that will be needed later. If there are Prohibits present in some
+     * roles, then they cannot be overridden by other roles or role overrides in lower contexts,
+     * therefore, such information, if any, is absent from the results.
      *
      * @param int $userid User id
      * @param string $capability Capability e.g. 'moodle/course:view'
@@ -58,42 +61,101 @@ class get_user_capability_course_helper {
         }
         $rdefs = get_role_definitions(array_keys($roleids));
 
+        // A prohibit in any relevant role prevents the capability
+        // in that context and all subcontexts. We need to track that.
+        // Here, the array keys are the paths where there is a prohibit the values are the role id.
+        $prohibitpaths = [];
+
         // 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 ($accessdata['ra'] as $rapath => $roles) {
+
             foreach ($roles as $roleid) {
                 // Get role definition for that role.
-                foreach ($rdefs[$roleid] as $rolepath => $caps) {
+                foreach ($rdefs[$roleid] as $rdefpath => $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.
+                    // Check a role definition or override above ra.
-                    if ($rolepath === '/' . $systemcontext->id) {
+                    if (self::path_is_above($rdefpath, $rapath)) {
-                        // Note /1 is listed first in the array so this entry will be overridden
+                        // Note that $rdefs is sorted by path, so if a more specific override
-                        // if there is an override for the role on this actual level.
+                        // exists, it will be processed later and override this one.
-                        $effectivepath = $userpath;
+                        $effectivepath = $rapath;
-                    } else if (preg_match('~^' . $userpath . '($|/)~', $rolepath)) {
+                    } else if (self::path_is_above($rapath, $rdefpath)) {
-                        $effectivepath = $rolepath;
+                        $effectivepath = $rdefpath;
                     } else {
                         // Not inside an area where the user has the role, so ignore.
                         continue;
                     }
 
+                    // Check for already seen prohibits in higher context. Overrides can't change that.
+                    if (self::any_path_is_above($prohibitpaths, $effectivepath)) {
+                        continue;
+                    }
+
+                    // This is a releavant role assignment / permission combination. Save it.
                     if (!array_key_exists($effectivepath, $pathroleperms)) {
                         $pathroleperms[$effectivepath] = [];
                     }
                     $pathroleperms[$effectivepath][$roleid] = $caps[$capability];
+
+                    // Update $prohibitpaths if necessary.
+                    if ($caps[$capability] == CAP_PROHIBIT) {
+                        // First remove any lower-context prohibits that might have come from other roles.
+                        foreach ($prohibitpaths as $otherprohibitpath => $notused) {
+                            if (self::path_is_above($effectivepath, $otherprohibitpath)) {
+                                unset($prohibitpaths[$otherprohibitpath]);
+                            }
+                        }
+                        $prohibitpaths[$effectivepath] = $roleid;
+                    }
                 }
             }
         }
 
+        // Finally, if a later role had a higher-level prohibit that an earlier role,
+        // there may be more bits we can prune - but don't prune the prohibits!
+        foreach ($pathroleperms as $effectivepath => $roleperms) {
+            if ($roleid = self::any_path_is_above($prohibitpaths, $effectivepath)) {
+                unset($pathroleperms[$effectivepath]);
+                $pathroleperms[$effectivepath][$roleid] = CAP_PROHIBIT;
+            }
+        }
+
         return $pathroleperms;
     }
 
+    /**
+     * Test if a context path $otherpath is the same as, or underneath, $parentpath.
+     *
+     * @param string $parentpath the path of the parent context.
+     * @param string $otherpath the path of another context.
+     * @return bool true if $otherpath is underneath (or equal to) $parentpath.
+     */
+    protected static function path_is_above($parentpath, $otherpath) {
+        return preg_match('~^' . $parentpath . '($|/)~', $otherpath);
+    }
+
+    /**
+     * Test if a context path $otherpath is the same as, or underneath, any of $prohibitpaths.
+     *
+     * @param array $prohibitpaths array keys are context paths.
+     * @param string $otherpath the path of another context.
+     * @return int releavant $roleid if $otherpath is underneath (or equal to)
+     *      any of the $prohibitpaths, 0 otherwise (so, can be used as a bool).
+     */
+    protected static function any_path_is_above($prohibitpaths, $otherpath) {
+        foreach ($prohibitpaths as $prohibitpath => $roleid) {
+            if (self::path_is_above($prohibitpath, $otherpath)) {
+                return $roleid;
+            }
+        }
+        return 0;
+    }
+
     /**
      * Calculates a permission tree based on an array of information about role permissions.
      *

lib/tests/accesslib_test.php

@@ -1707,6 +1707,25 @@ class core_accesslib_testcase extends advanced_testcase {
         $generator = $this->getDataGenerator();
         $cap = 'moodle/course:view';
 
+        // The structure being created here is this:
+        //
+        // All tests work with the single capability 'moodle/course:view'.
+        //
+        //             ROLE DEF/OVERRIDE                        ROLE ASSIGNS
+        //    Role:  Allow    Prohib    Empty   Def user      u1  u2  u3  u4   u5  u6  u7  u8
+        // System    ALLOW    PROHIBIT                            A   E   A+E
+        //   cat1                       ALLOW
+        //     C1                               (ALLOW)                            P
+        //     C2             ALLOW                                                    E   P
+        //     cat2                     PREVENT
+        //       C3                     ALLOW                                      E
+        //       C4
+        //   Misc.                                                             A
+        //     C5    PREVENT                                                       A
+        //     C6                       PROHIBIT
+        //
+        // Front-page and guest role stuff from the end of this test not included in the diagram.
+
         // Create a role which allows course:view and one that prohibits it, and one neither.
         $allowroleid = $generator->create_role();
         $prohibitroleid = $generator->create_role();
@@ -1720,6 +1739,7 @@ class core_accesslib_testcase extends advanced_testcase {
         $cat2 = $generator->create_category(['parent' => $cat1->id]);
 
         // Create six courses - two in cat1, two in cat2, and two in default category.
+        // Shortnames are used for a sorting test. Otherwise they are not significant.
         $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']);
@@ -1741,6 +1761,8 @@ class core_accesslib_testcase extends advanced_testcase {
                 context_course::instance($c6->id)->id);
         assign_capability($cap, CAP_ALLOW, $emptyroleid,
                 context_course::instance($c3->id)->id);
+        assign_capability($cap, CAP_ALLOW, $prohibitroleid,
+                context_course::instance($c2->id)->id);
 
         // User 1 has no roles except default user role.
         $u1 = $generator->create_user();
@@ -1800,6 +1822,22 @@ class core_accesslib_testcase extends advanced_testcase {
         $courses = get_user_capability_course($cap, $u6->id, true, '', 'id');
         $this->assert_course_ids([$c3->id], $courses);
 
+        // User 7 has empty role in C2.
+        $u7 = $generator->create_user();
+        role_assign($emptyroleid, $u7->id, context_course::instance($c2->id)->id);
+
+        // Should get C1 by the default user role override, and C2 by the cat1 level override.
+        $courses = get_user_capability_course($cap, $u7->id, true, '', 'id');
+        $this->assert_course_ids([$c1->id, $c2->id], $courses);
+
+        // User 8 has prohibit role as system context, to verify that prohibits can't be overridden.
+        $u8 = $generator->create_user();
+        role_assign($prohibitroleid, $u8->id, context_course::instance($c2->id)->id);
+
+        // Should get C1 by the default user role override, no other courses because the prohibit cannot be overridden.
+        $courses = get_user_capability_course($cap, $u8->id, true, '', 'id');
+        $this->assert_course_ids([$c1->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],