From b2f349a4331c0ff01b296432c9ccac20c945089a Mon Sep 17 00:00:00 2001
From: Jonathan Champ <jrchamp@ncsu.edu>
Date: Tue, 5 Jun 2018 15:13:02 -0400
Subject: [PATCH] MDL-54035 accesslib: separate role definition cache clear

Thanks to MDL-49398, we can separate the combined user session and role
definition cache clearing function into two separate functions. At the
same time, we want to identify and remove mark_dirty() calls that were
added for role definition changes but were incorrectly left behind.

Change highlights:
 - Remove unnecessary mark_dirty() calls performed after
   assign_capability(), unassign_capability(), delete_role(),
   deleted contexts, brand new contexts
 - Move role definition cache clear from the user-centric
   accesslib_clear_all_caches() to the newly created,
   role-dedicated accesslib_reset_role_cache()
---
 admin/roles/allow.php                         |  1 -
 .../classes/capability_table_with_risks.php   |  3 --
 admin/roles/manage.php                        |  4 +-
 course/classes/category.php                   |  3 --
 course/lib.php                                |  3 --
 course/tests/courselib_test.php               |  2 -
 grade/tests/report_graderlib_test.php         |  1 -
 lib/accesslib.php                             | 54 +++++++++----------
 lib/behat/classes/util.php                    |  1 +
 lib/moodlelib.php                             |  2 -
 lib/phpunit/classes/util.php                  |  1 +
 lib/tests/accesslib_test.php                  |  7 ++-
 lib/tests/questionlib_test.php                |  7 ---
 message/tests/api_test.php                    |  1 -
 mod/feedback/tests/lib_test.php               |  1 -
 mod/forum/tests/subscriptions_test.php        |  2 -
 tag/tests/external_test.php                   |  1 -
 user/tests/userlib_test.php                   |  3 --
 18 files changed, 31 insertions(+), 66 deletions(-)

diff --git a/admin/roles/allow.php b/admin/roles/allow.php
index 71e9bcb3d0ad7..88609cfe359ff 100644
--- a/admin/roles/allow.php
+++ b/admin/roles/allow.php
@@ -46,7 +46,6 @@
 
 if (optional_param('submit', false, PARAM_BOOL) && data_submitted() && confirm_sesskey()) {
     $controller->process_submission();
-    $syscontext->mark_dirty();
     $event = null;
     // Create event depending on mode.
     switch ($mode) {
diff --git a/admin/roles/classes/capability_table_with_risks.php b/admin/roles/classes/capability_table_with_risks.php
index db7e16bc83f99..4e597f472f4da 100644
--- a/admin/roles/classes/capability_table_with_risks.php
+++ b/admin/roles/classes/capability_table_with_risks.php
@@ -123,9 +123,6 @@ public function save_changes() {
             assign_capability($changedcap, $this->permissions[$changedcap],
                 $this->roleid, $this->context->id, true);
         }
-
-        // Force accessinfo refresh for users visiting this context.
-        $this->context->mark_dirty();
     }
 
     public function display() {
diff --git a/admin/roles/manage.php b/admin/roles/manage.php
index fb390eae1c4d1..d2e703facee25 100644
--- a/admin/roles/manage.php
+++ b/admin/roles/manage.php
@@ -85,12 +85,10 @@
             die;
         }
         if (!delete_role($roleid)) {
-            // The delete failed, but mark the context dirty in case.
-            $systemcontext->mark_dirty();
+            // The delete failed.
             print_error('cannotdeleterolewithid', 'error', $baseurl, $roleid);
         }
         // Deleted a role sitewide...
-        $systemcontext->mark_dirty();
         redirect($baseurl);
         break;
 
diff --git a/course/classes/category.php b/course/classes/category.php
index a0345e604d2d6..bf4bfd6eed572 100644
--- a/course/classes/category.php
+++ b/course/classes/category.php
@@ -445,9 +445,6 @@ public static function create($data, $editoroptions = null) {
         $path = $parent->path . '/' . $newcategory->id;
         $DB->set_field('course_categories', 'path', $path, array('id' => $newcategory->id));
 
-        // We should mark the context as dirty.
-        context_coursecat::instance($newcategory->id)->mark_dirty();
-
         fix_course_sortorder();
 
         // If this is data from form results, save embedded files and update description.
diff --git a/course/lib.php b/course/lib.php
index c4a4ead7100be..2db5dffcb51fe 100644
--- a/course/lib.php
+++ b/course/lib.php
@@ -2450,9 +2450,6 @@ function create_course($data, $editoroptions = NULL) {
     // purge appropriate caches in case fix_course_sortorder() did not change anything
     cache_helper::purge_by_event('changesincourse');
 
-    // new context created - better mark it as dirty
-    $context->mark_dirty();
-
     // Trigger a course created event.
     $event = \core\event\course_created::create(array(
         'objectid' => $course->id,
diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php
index 5434469428d46..083caa71e07b9 100644
--- a/course/tests/courselib_test.php
+++ b/course/tests/courselib_test.php
@@ -985,7 +985,6 @@ public function test_course_can_delete_section() {
         $modulecontext = context_module::instance($assign1->cmid);
         assign_capability('moodle/course:manageactivities', CAP_PROHIBIT, $roleids['editingteacher'],
             $modulecontext);
-        $modulecontext->mark_dirty();
         $this->assertFalse(course_can_delete_section($courseweeks, 1));
         $this->assertTrue(course_can_delete_section($courseweeks, 2));
 
@@ -3131,7 +3130,6 @@ public function test_course_get_user_navigation_options_for_students() {
         $CFG->enableblogs = 0;
         // Disable view participants capability.
         assign_capability('moodle/course:viewparticipants', CAP_PROHIBIT, $roleid, $context);
-        $context->mark_dirty();
 
         $navoptions = course_get_user_navigation_options($context);
         $this->assertFalse($navoptions->blogs);
diff --git a/grade/tests/report_graderlib_test.php b/grade/tests/report_graderlib_test.php
index 75f52c3a8a8dc..152fc3b2ec173 100644
--- a/grade/tests/report_graderlib_test.php
+++ b/grade/tests/report_graderlib_test.php
@@ -287,7 +287,6 @@ public function test_get_right_rows() {
         $context = context_course::instance($course->id);
         $managerroleid = $DB->get_field('role', 'id', array('shortname' => 'manager'));
         assign_capability('moodle/grade:viewhidden', CAP_PROHIBIT, $managerroleid, $context->id, true);
-        $context->mark_dirty();
         $this->assertFalse(has_capability('moodle/grade:viewhidden', $context));
 
         // Recreate the report. Confirm it returns successfully still.
diff --git a/lib/accesslib.php b/lib/accesslib.php
index 8211ffb2b788b..b846108269b95 100644
--- a/lib/accesslib.php
+++ b/lib/accesslib.php
@@ -62,6 +62,8 @@
  * <b>Name conventions</b>
  *
  * "ctx" means context
+ * "ra" means role assignment
+ * "rdef" means role definition
  *
  * <b>accessdata</b>
  *
@@ -72,9 +74,7 @@
  * against userid in $ACCESSLIB_PRIVATE->accessdatabyuser).
  *
  * $accessdata is a multidimensional array, holding
- * role assignments (RAs), role-capabilities-perm sets
- * (role defs) and a list of courses we have loaded
- * data for.
+ * role assignments (RAs), role switches and initialization time.
  *
  * Things are keyed on "contextpaths" (the path field of
  * the context table) for fast walking up/down the tree.
@@ -201,6 +201,7 @@ function accesslib_clear_all_caches_for_unit_testing() {
     }
 
     accesslib_clear_all_caches(true);
+    accesslib_reset_role_cache();
 
     unset($USER->access);
 }
@@ -219,16 +220,32 @@ function accesslib_clear_all_caches($resetcontexts) {
 
     $ACCESSLIB_PRIVATE->dirtycontexts    = null;
     $ACCESSLIB_PRIVATE->accessdatabyuser = array();
-    $ACCESSLIB_PRIVATE->cacheroledefs    = array();
-
-    $cache = cache::make('core', 'roledefs');
-    $cache->purge();
 
     if ($resetcontexts) {
         context_helper::reset_caches();
     }
 }
 
+/**
+ * Full reset of accesslib's private role cache. ONLY TO BE USED FROM THIS LIBRARY FILE!
+ *
+ * This reset does not touch global $USER.
+ *
+ * Note: Only use this when the roles that need a refresh are unknown.
+ *
+ * @see accesslib_clear_role_cache()
+ *
+ * @access private
+ * @return void
+ */
+function accesslib_reset_role_cache() {
+    global $ACCESSLIB_PRIVATE;
+
+    $ACCESSLIB_PRIVATE->cacheroledefs = array();
+    $cache = cache::make('core', 'roledefs');
+    $cache->purge();
+}
+
 /**
  * Clears accesslib's private cache of a specific role or roles. ONLY BE USED FROM THIS LIBRARY FILE!
  *
@@ -1280,8 +1297,6 @@ function delete_role($roleid) {
 /**
  * Function to write context specific overrides, or default capabilities.
  *
- * NOTE: use $context->mark_dirty() after this
- *
  * @param string $capability string name
  * @param int $permission CAP_ constants
  * @param int $roleid role id
@@ -1335,8 +1350,6 @@ function assign_capability($capability, $permission, $roleid, $contextid, $overw
 /**
  * Unassign a capability from a role.
  *
- * NOTE: use $context->mark_dirty() after this
- *
  * @param string $capability the name of the capability
  * @param int $roleid the role id
  * @param int|context $contextid null means all contexts
@@ -2097,9 +2110,6 @@ function reset_role_capabilities($roleid) {
 
     // Reset any cache of this role, including MUC.
     accesslib_clear_role_cache($roleid);
-
-    // Mark the system context dirty.
-    context_system::instance()->mark_dirty();
 }
 
 /**
@@ -2217,7 +2227,7 @@ function update_capabilities($component = 'moodle') {
     capabilities_cleanup($component, $filecaps);
 
     // reset static caches
-    accesslib_clear_all_caches(false);
+    accesslib_reset_role_cache();
 
     // Flush the cached again, as we have changed DB.
     cache::make('core', 'capabilities')->delete('core_capabilities');
@@ -2381,8 +2391,6 @@ function is_inside_frontpage(context $context) {
  * @return stdClass or null if capability not found
  */
 function get_capability_info($capabilityname) {
-    global $ACCESSLIB_PRIVATE, $DB; // one request per page only
-
     $caps = get_all_capabilities();
 
     if (!isset($caps[$capabilityname])) {
@@ -4592,7 +4600,6 @@ function role_change_permission($roleid, $context, $capname, $permission) {
 
     if ($permission == CAP_INHERIT) {
         unassign_capability($capname, $roleid, $context->id);
-        $context->mark_dirty();
         return;
     }
 
@@ -4625,7 +4632,6 @@ function role_change_permission($roleid, $context, $capname, $permission) {
                 // permission already set in parent context or parent - just unset in this context
                 // we do this because we want as few overrides as possible for performance reasons
                 unassign_capability($capname, $roleid, $context->id);
-                $context->mark_dirty();
                 return;
             }
         }
@@ -4639,9 +4645,6 @@ function role_change_permission($roleid, $context, $capname, $permission) {
 
     // assign the needed capability
     assign_capability($capname, $permission, $roleid, $context->id, true);
-
-    // force cap reloading
-    $context->mark_dirty();
 }
 
 
@@ -5066,8 +5069,6 @@ public function update_moved(context $newparent) {
 
         $trans = $DB->start_delegated_transaction();
 
-        $this->mark_dirty();
-
         $setdepth = '';
         if (($newparent->depth +1) != $this->_depth) {
             $diff = $newparent->depth - $this->_depth + 1;
@@ -5189,11 +5190,6 @@ public function delete() {
         $DB->delete_records('context', array('id'=>$this->_id));
         // purge static context cache if entry present
         context::cache_remove($this);
-
-        // do not mark dirty contexts if parents unknown
-        if (!is_null($this->_path) and $this->_depth > 0) {
-            $this->mark_dirty();
-        }
     }
 
     // ====== context level related methods ======
diff --git a/lib/behat/classes/util.php b/lib/behat/classes/util.php
index ab97abb6240eb..a9ab97a7bbbd8 100644
--- a/lib/behat/classes/util.php
+++ b/lib/behat/classes/util.php
@@ -354,6 +354,7 @@ public static function reset_all_data() {
 
         // Reset all static caches.
         accesslib_clear_all_caches(true);
+        accesslib_reset_role_cache();
         // Reset the nasty strings list used during the last test.
         nasty_strings::reset_used_strings();
 
diff --git a/lib/moodlelib.php b/lib/moodlelib.php
index 57a0d0bf0459b..f9886b3580ab7 100644
--- a/lib/moodlelib.php
+++ b/lib/moodlelib.php
@@ -5362,8 +5362,6 @@ function reset_course_userdata($data) {
         foreach ($children as $child) {
             role_unassign_all(array('contextid' => $child->id));
         }
-        // Force refresh for logged in users.
-        $context->mark_dirty();
         $status[] = array('component' => $componentstr, 'item' => get_string('deletelocalroles', 'role'), 'error' => false);
     }
 
diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php
index 965ce0f0584ce..8c594f6497cef 100644
--- a/lib/phpunit/classes/util.php
+++ b/lib/phpunit/classes/util.php
@@ -208,6 +208,7 @@ public static function reset_all_data($detectchanges = false) {
         // reset all static caches
         \core\event\manager::phpunit_reset();
         accesslib_clear_all_caches(true);
+        accesslib_reset_role_cache();
         get_string_manager()->reset_caches(true);
         reset_text_filters_cache(true);
         core_text::reset_caches();
diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php
index 3ff61927e08c0..6027c0a2c6098 100644
--- a/lib/tests/accesslib_test.php
+++ b/lib/tests/accesslib_test.php
@@ -196,7 +196,6 @@ public function test_is_enrolled() {
 
         // Prevent the capability for this user role.
         assign_capability($capability, CAP_PROHIBIT, $role->id, $coursecontext);
-        $coursecontext->mark_dirty();
         $this->assertFalse(has_capability($capability, $coursecontext, $user->id));
 
         // Again, we seed the cache first by checking initial enrolment,
@@ -3212,7 +3211,7 @@ public function test_permission_evaluation() {
         $context = context_course::instance($course->id);
         $this->assertEquals($categorycontext, $context->get_parent_context());
         $dirty = get_cache_flags('accesslib/dirtycontexts', time()-2);
-        $this->assertTrue(isset($dirty[$oldpath]));
+        $this->assertFalse(isset($dirty[$oldpath]));
         $this->assertTrue(isset($dirty[$context->path]));
 
 
@@ -3238,7 +3237,7 @@ public function test_permission_evaluation() {
         $DB->delete_records('cache_flags', array());
         $context->delete(); // Should delete also linked blocks.
         $dirty = get_cache_flags('accesslib/dirtycontexts', time()-2);
-        $this->assertTrue(isset($dirty[$context->path]));
+        $this->assertFalse(isset($dirty[$context->path]));
         $this->assertFalse($DB->record_exists('context', array('id'=>$context->id)));
         $this->assertFalse($DB->record_exists('context', array('id'=>$bicontext->id)));
         $this->assertFalse($DB->record_exists('context', array('contextlevel'=>CONTEXT_MODULE, 'instanceid'=>$testpages[4])));
@@ -3258,7 +3257,7 @@ public function test_permission_evaluation() {
         $DB->delete_records('cache_flags', array());
         context_helper::delete_instance(CONTEXT_COURSE, $lastcourse);
         $dirty = get_cache_flags('accesslib/dirtycontexts', time()-2);
-        $this->assertTrue(isset($dirty[$coursecontext->path]));
+        $this->assertFalse(isset($dirty[$coursecontext->path]));
         $this->assertEquals(0, context_inspection::test_context_cache_size());
         $this->assertFalse($DB->record_exists('context', array('contextlevel'=>CONTEXT_COURSE, 'instanceid'=>$lastcourse)));
         context_course::instance($lastcourse);
diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php
index 41a03bde3e4dc..cab87536c9c1f 100644
--- a/lib/tests/questionlib_test.php
+++ b/lib/tests/questionlib_test.php
@@ -1669,7 +1669,6 @@ public function test_question_has_capability_on_using_stdclass($capabilities, $c
         foreach ($capabilities as $capname => $capvalue) {
             assign_capability($capname, $capvalue, $roleid, $context->id);
         }
-        $context->mark_dirty();
 
         $this->setUser($user);
 
@@ -1719,7 +1718,6 @@ public function test_question_has_capability_on_using_question_definition($capab
         foreach ($capabilities as $capname => $capvalue) {
             assign_capability($capname, $capvalue, $roleid, $context->id);
         }
-        $context->mark_dirty();
 
         // Create the question.
         $qtype = 'truefalse';
@@ -1771,7 +1769,6 @@ public function test_question_has_capability_on_using_question_id($capabilities,
         foreach ($capabilities as $capname => $capvalue) {
             assign_capability($capname, $capvalue, $roleid, $context->id);
         }
-        $context->mark_dirty();
 
         // Create the question.
         $qtype = 'truefalse';
@@ -1823,7 +1820,6 @@ public function test_question_has_capability_on_using_question_string_id($capabi
         foreach ($capabilities as $capname => $capvalue) {
             assign_capability($capname, $capvalue, $roleid, $context->id);
         }
-        $context->mark_dirty();
 
         // Create the question.
         $qtype = 'truefalse';
@@ -1881,8 +1877,6 @@ public function test_question_has_capability_on_using_moved_question($capabiliti
         foreach ($capabilities as $capname => $capvalue) {
             assign_capability($capname, $capvalue, $roleid, $newcontext->id);
         }
-        $context->mark_dirty();
-        $newcontext->mark_dirty();
 
         // Create the question.
         $qtype = 'truefalse';
@@ -1938,7 +1932,6 @@ public function test_question_has_capability_on_using_question($capabilities, $c
         foreach ($capabilities as $capname => $capvalue) {
             assign_capability($capname, $capvalue, $roleid, $context->id);
         }
-        $context->mark_dirty();
 
         // Create the question.
         $question = $questiongenerator->create_question('truefalse', null, [
diff --git a/message/tests/api_test.php b/message/tests/api_test.php
index 60a5bb7eecf19..42f8192c8753b 100644
--- a/message/tests/api_test.php
+++ b/message/tests/api_test.php
@@ -254,7 +254,6 @@ public function test_search_users() {
         // Remove the viewparticipants capability from one of the courses.
         $course5context = context_course::instance($course5->id);
         assign_capability('moodle/course:viewparticipants', CAP_PROHIBIT, $role->id, $course5context->id);
-        $course5context->mark_dirty();
 
         // Perform a search.
         list($contacts, $courses, $noncontacts) = \core_message\api::search_users($user1->id, 'search');
diff --git a/mod/feedback/tests/lib_test.php b/mod/feedback/tests/lib_test.php
index 0990f403be647..42764143eb5bf 100644
--- a/mod/feedback/tests/lib_test.php
+++ b/mod/feedback/tests/lib_test.php
@@ -279,7 +279,6 @@ public function test_feedback_core_calendar_provide_event_action_can_not_submit(
 
         $this->setUser($user);
         assign_capability('mod/feedback:complete', CAP_PROHIBIT, $studentrole->id, $context);
-        $context->mark_dirty();
 
         $factory = new \core_calendar\action_factory();
         $action = mod_feedback_core_calendar_provide_event_action($event, $factory);
diff --git a/mod/forum/tests/subscriptions_test.php b/mod/forum/tests/subscriptions_test.php
index d48bf0e80e948..57035c2ef2505 100644
--- a/mod/forum/tests/subscriptions_test.php
+++ b/mod/forum/tests/subscriptions_test.php
@@ -155,7 +155,6 @@ public function test_unsubscribable_forums() {
         $roleids = $DB->get_records_menu('role', null, '', 'shortname, id');
         $context = \context_course::instance($course->id);
         assign_capability('moodle/course:viewhiddenactivities', CAP_ALLOW, $roleids['student'], $context);
-        $context->mark_dirty();
 
         // All of the unsubscribable forums should now be listed.
         $result = \mod_forum\subscriptions::get_unsubscribable_forums();
@@ -906,7 +905,6 @@ public function test_force_subscribed_to_forum() {
         $cm = get_coursemodule_from_instance('forum', $forum->id);
         $context = \context_module::instance($cm->id);
         assign_capability('mod/forum:allowforcesubscribe', CAP_PROHIBIT, $roleids['student'], $context);
-        $context->mark_dirty();
         $this->assertFalse(has_capability('mod/forum:allowforcesubscribe', $context, $user->id));
 
         // Check that the user is no longer subscribed to the forum.
diff --git a/tag/tests/external_test.php b/tag/tests/external_test.php
index b4fa7cdd7e735..33a6c886fde34 100644
--- a/tag/tests/external_test.php
+++ b/tag/tests/external_test.php
@@ -102,7 +102,6 @@ public function test_update_tags() {
         // User with editing and manage cap can also change the tag name,
         // make it standard and reset flag.
         assign_capability('moodle/tag:manage', CAP_ALLOW, $roleid, $context->id);
-        $context->mark_dirty();
         $this->assertTrue(has_capability('moodle/tag:manage', $context));
         $result = core_tag_external::update_tags(array($updatetag));
         $result = external_api::clean_returnvalue(core_tag_external::update_tags_returns(), $result);
diff --git a/user/tests/userlib_test.php b/user/tests/userlib_test.php
index b7f6d6c456759..e7745f5663df2 100644
--- a/user/tests/userlib_test.php
+++ b/user/tests/userlib_test.php
@@ -587,7 +587,6 @@ public function test_user_can_view_profile() {
 
         // Remove capability moodle/user:viewdetails in course 2.
         assign_capability('moodle/user:viewdetails', CAP_PROHIBIT, $studentrole->id, $coursecontext);
-        $coursecontext->mark_dirty();
         // Set current user to user 1.
         $this->setUser($user1);
         // User 1 can see User 1's profile.
@@ -693,13 +692,11 @@ public function test_user_can_view_profile() {
         $systemcontext = context_system::instance();
         assign_capability('moodle/user:viewdetails', CAP_PREVENT, $managerrole->id, $systemcontext, true);
         assign_capability('moodle/user:viewalldetails', CAP_PREVENT, $managerrole->id, $systemcontext, true);
-        $systemcontext->mark_dirty();
 
         // And override these to 'Allow' in a specific course.
         $course4context = context_course::instance($course4->id);
         assign_capability('moodle/user:viewalldetails', CAP_ALLOW, $managerrole->id, $course4context, true);
         assign_capability('moodle/user:viewdetails', CAP_ALLOW, $managerrole->id, $course4context, true);
-        $course4context->mark_dirty();
 
         // The manager now shouldn't have viewdetails in the system or user context.
         $this->setUser($user9);