From c2922fab2705952f52af98629945b6ef186cbc0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 8 Jan 2019 19:23:59 +0100 Subject: [PATCH 1/2] MDL-64452 cache: Always use the current session id in the key prefix The current logic in the cache_session::check_tracked_user() is not right. We must always set the current session id. A typical use case is when the cache instance is instantiated for a not logged in user. We can't let the sessionid property null in that case as it forms an important part of the parsed key. Similarly, even if we have the same user currently loaded, we must still set the sessionid to make sure the data will be associated with the current PHP session. Same user (including visitors or guest users) can access the site from different browsers and each must end up with its own key prefix. --- cache/classes/loaders.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index 691d2fb540d..6b1337f050d 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -1712,6 +1712,7 @@ class cache_session extends cache { public function __construct(cache_definition $definition, cache_store $store, $loader = null) { // First up copy the loadeduserid to the current user id. $this->currentuserid = self::$loadeduserid; + $this->set_session_id(); parent::__construct($definition, $store, $loader); // This will trigger check tracked user. If this gets removed a call to that will need to be added here in its place. @@ -1771,8 +1772,6 @@ class cache_session extends cache { // Purge the data we have for the old user. // This way we don't bloat the session. $this->purge(); - // Update the session id just in case! - $this->set_session_id(); } self::$loadeduserid = $new; $this->currentuserid = $new; @@ -1780,8 +1779,6 @@ class cache_session extends cache { // The current user matches the loaded user but not the user last used by this cache. $this->purge_current_user(); $this->currentuserid = $new; - // Update the session id just in case! - $this->set_session_id(); } } From 9fb92df86766b5e5a5e4e716997aa39f85e44041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 8 Jan 2019 20:03:06 +0100 Subject: [PATCH 2/2] MDL-64452 cache: Add tests for key prefix based on session id Unfortunately, we can't simply use session_id() to regenerate the session id in unit tests. Starting from PHP 7.2, it would trigger "session_id(): Cannot change session id when headers already sent", refer to MDL-60978 and PHP bug #75628 for more details. As a workaround, we use a static property allowing us to inject the value that we then use as a session identifier. This is reasonably enough to make sure that the identifier is used as a part of the key prefix. --- cache/tests/cache_test.php | 47 ++++++++++++++++++++++++++++++++++++ cache/tests/fixtures/lib.php | 28 +++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index 6aa268fc12f..9c0f1d93475 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -2327,4 +2327,51 @@ class core_cache_testcase extends advanced_testcase { $this->assertEquals('test data 2', $cache->get('testkey1')); } + /** + * Test that values set in different sessions are stored with different key prefixes. + */ + public function test_session_distinct_storage_key() { + $this->resetAfterTest(); + + // Prepare a dummy session cache configuration. + $config = cache_config_testing::instance(); + $config->phpunit_add_definition('phpunit/test_session_distinct_storage_key', array( + 'mode' => cache_store::MODE_SESSION, + 'component' => 'phpunit', + 'area' => 'test_session_distinct_storage_key' + )); + + // First anonymous user's session cache. + cache_phpunit_session::phpunit_mockup_session_id('foo'); + $this->setUser(0); + $cache1 = cache::make('phpunit', 'test_session_distinct_storage_key'); + + // Reset cache instances to emulate a new request. + cache_factory::instance()->reset_cache_instances(); + + // Another anonymous user's session cache. + cache_phpunit_session::phpunit_mockup_session_id('bar'); + $this->setUser(0); + $cache2 = cache::make('phpunit', 'test_session_distinct_storage_key'); + + cache_factory::instance()->reset_cache_instances(); + + // Guest user's session cache. + cache_phpunit_session::phpunit_mockup_session_id('baz'); + $this->setGuestUser(); + $cache3 = cache::make('phpunit', 'test_session_distinct_storage_key'); + + cache_factory::instance()->reset_cache_instances(); + + // Same guest user's session cache but in another browser window. + cache_phpunit_session::phpunit_mockup_session_id('baz'); + $this->setGuestUser(); + $cache4 = cache::make('phpunit', 'test_session_distinct_storage_key'); + + // Assert that different PHP session implies different key prefix for storing values. + $this->assertNotEquals($cache1->phpunit_get_key_prefix(), $cache2->phpunit_get_key_prefix()); + + // Assert that same PHP session implies same key prefix for storing values. + $this->assertEquals($cache3->phpunit_get_key_prefix(), $cache4->phpunit_get_key_prefix()); + } } diff --git a/cache/tests/fixtures/lib.php b/cache/tests/fixtures/lib.php index 6c42c39aff9..bee66090d1a 100644 --- a/cache/tests/fixtures/lib.php +++ b/cache/tests/fixtures/lib.php @@ -465,6 +465,9 @@ class cache_phpunit_application extends cache_application { */ class cache_phpunit_session extends cache_session { + /** @var Static member used for emulating the behaviour of session_id() during the tests. */ + protected static $sessionidmockup = 'phpunitmockupsessionid'; + /** * Returns the class of the store immediately associated with this cache. * @return string @@ -480,6 +483,31 @@ class cache_phpunit_session extends cache_session { public function phpunit_get_store_implements() { return class_implements($this->get_store()); } + + /** + * Provide access to the {@link cache_session::get_key_prefix()} method. + * + * @return string + */ + public function phpunit_get_key_prefix() { + return $this->get_key_prefix(); + } + + /** + * Allows to inject the session identifier. + * + * @param string $sessionid + */ + public static function phpunit_mockup_session_id($sessionid) { + static::$sessionidmockup = $sessionid; + } + + /** + * Override the parent behaviour so that it does not need the actual session_id() call. + */ + protected function set_session_id() { + $this->sessionid = static::$sessionidmockup; + } } /**