From 9e829c1607b72efd75f2f942631c8a48c324ee7f Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 5 Jul 2018 22:50:35 +0100 Subject: [PATCH 01/14] FIX Allow cache control changes to affect default state --- src/Control/Middleware/HTTPCacheControlMiddleware.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Control/Middleware/HTTPCacheControlMiddleware.php b/src/Control/Middleware/HTTPCacheControlMiddleware.php index 572e6fb2b..18801f204 100644 --- a/src/Control/Middleware/HTTPCacheControlMiddleware.php +++ b/src/Control/Middleware/HTTPCacheControlMiddleware.php @@ -513,7 +513,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable public function setMaxAge($age) { // Affect all non-disabled states - $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC]; + $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; $this->setStateDirective($applyTo, 'max-age', $age); return $this; } @@ -529,7 +529,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable public function setSharedMaxAge($age) { // Affect all non-disabled states - $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC]; + $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; $this->setStateDirective($applyTo, 's-maxage', $age); return $this; } @@ -543,7 +543,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable */ public function setMustRevalidate($mustRevalidate = true) { - $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC]; + $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; $this->setStateDirective($applyTo, 'must-revalidate', $mustRevalidate); return $this; } From 18b7dc235a8dfbf03fa450ef444bd520f0e05e51 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 5 Jul 2018 22:51:03 +0100 Subject: [PATCH 02/14] FIX Add must-revalidate to default state so its common on all our core states --- src/Control/Middleware/HTTPCacheControlMiddleware.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Control/Middleware/HTTPCacheControlMiddleware.php b/src/Control/Middleware/HTTPCacheControlMiddleware.php index 18801f204..940a4a757 100644 --- a/src/Control/Middleware/HTTPCacheControlMiddleware.php +++ b/src/Control/Middleware/HTTPCacheControlMiddleware.php @@ -94,6 +94,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable ], self::STATE_DEFAULT => [ 'no-cache' => true, + 'must-revalidate' => true, ], ]; From 92f5ef31d89b792ab29d84371a6be47869ccab73 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 5 Jul 2018 22:51:39 +0100 Subject: [PATCH 03/14] FIX Allow setNoCache(false) to remove no-cache directive --- src/Control/Middleware/HTTPCacheControlMiddleware.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Control/Middleware/HTTPCacheControlMiddleware.php b/src/Control/Middleware/HTTPCacheControlMiddleware.php index 940a4a757..6ea8a2662 100644 --- a/src/Control/Middleware/HTTPCacheControlMiddleware.php +++ b/src/Control/Middleware/HTTPCacheControlMiddleware.php @@ -499,7 +499,13 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable { // Affect all non-disabled states $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC]; - $this->setStateDirective($applyTo, 'no-cache', $noCache); + if ($noCache) { + $this->setStateDirective($applyTo, 'no-cache'); + $this->removeStateDirective($applyTo, 'max-age'); + $this->removeStateDirective($applyTo, 's-maxage'); + } else { + $this->removeStateDirective($applyTo, 'no-cache'); + } return $this; } From 399ebd003112c850c44e15d7d6ac5ec973eae226 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 5 Jul 2018 22:52:04 +0100 Subject: [PATCH 04/14] FIX If theres a max-age set remove no-cache and no-store --- src/Control/Middleware/HTTPCacheControlMiddleware.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Control/Middleware/HTTPCacheControlMiddleware.php b/src/Control/Middleware/HTTPCacheControlMiddleware.php index 6ea8a2662..3bdf2d02b 100644 --- a/src/Control/Middleware/HTTPCacheControlMiddleware.php +++ b/src/Control/Middleware/HTTPCacheControlMiddleware.php @@ -522,6 +522,10 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable // Affect all non-disabled states $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; $this->setStateDirective($applyTo, 'max-age', $age); + if ($age) { + $this->removeStateDirective($applyTo, 'no-cache'); + $this->removeStateDirective($applyTo, 'no-store'); + } return $this; } @@ -538,6 +542,10 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable // Affect all non-disabled states $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; $this->setStateDirective($applyTo, 's-maxage', $age); + if ($age) { + $this->removeStateDirective($applyTo, 'no-cache'); + $this->removeStateDirective($applyTo, 'no-store'); + } return $this; } From 601bb4d7681169d20435dae744d5731db1349b23 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Thu, 5 Jul 2018 23:12:55 +0100 Subject: [PATCH 05/14] Make augmentState method more efficient --- .../Middleware/HTTPCacheControlMiddleware.php | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Control/Middleware/HTTPCacheControlMiddleware.php b/src/Control/Middleware/HTTPCacheControlMiddleware.php index 3bdf2d02b..9d710bf94 100644 --- a/src/Control/Middleware/HTTPCacheControlMiddleware.php +++ b/src/Control/Middleware/HTTPCacheControlMiddleware.php @@ -778,17 +778,16 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable */ protected function augmentState(HTTPRequest $request, HTTPResponse $response) { - // If sessions exist we assume that the responses should not be cached by CDNs / proxies as we are - // likely to be supplying information relevant to the current user only - if ($request->getSession()->getAll()) { - // Don't force in case user code chooses to opt in to public caching - $this->privateCache(); - } - // Errors disable cache (unless some errors are cached intentionally by usercode) if ($response->isError() || $response->isRedirect()) { // Even if publicCache(true) is specified, errors will be uncacheable $this->disableCache(true); + } elseif ($request->getSession()->getAll()) { + // If sessions exist we assume that the responses should not be cached by CDNs / proxies as we are + // likely to be supplying information relevant to the current user only + + // Don't force in case user code chooses to opt in to public caching + $this->privateCache(); } } } From fd8448889c5170ad022c069856855b11fb6f57b6 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Fri, 6 Jul 2018 11:48:06 +0100 Subject: [PATCH 06/14] State default should be state enabled (no-cache is an enabled state) --- .../Middleware/HTTPCacheControlMiddleware.php | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/Control/Middleware/HTTPCacheControlMiddleware.php b/src/Control/Middleware/HTTPCacheControlMiddleware.php index 9d710bf94..b59b4e705 100644 --- a/src/Control/Middleware/HTTPCacheControlMiddleware.php +++ b/src/Control/Middleware/HTTPCacheControlMiddleware.php @@ -26,8 +26,6 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable const STATE_DISABLED = 'disabled'; - const STATE_DEFAULT = 'default'; - /** * Generate response for the given request * @@ -90,12 +88,9 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable 'must-revalidate' => true, ], self::STATE_ENABLED => [ - 'must-revalidate' => true, - ], - self::STATE_DEFAULT => [ 'no-cache' => true, 'must-revalidate' => true, - ], + ] ]; /** @@ -104,7 +99,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable * @config * @var string */ - private static $defaultState = self::STATE_DEFAULT; + private static $defaultState = self::STATE_ENABLED; /** * Current state @@ -520,7 +515,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable public function setMaxAge($age) { // Affect all non-disabled states - $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; + $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC]; $this->setStateDirective($applyTo, 'max-age', $age); if ($age) { $this->removeStateDirective($applyTo, 'no-cache'); @@ -540,7 +535,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable public function setSharedMaxAge($age) { // Affect all non-disabled states - $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; + $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC]; $this->setStateDirective($applyTo, 's-maxage', $age); if ($age) { $this->removeStateDirective($applyTo, 'no-cache'); @@ -558,7 +553,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable */ public function setMustRevalidate($mustRevalidate = true) { - $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC, self::STATE_DEFAULT]; + $applyTo = [self::STATE_ENABLED, self::STATE_PRIVATE, self::STATE_PUBLIC]; $this->setStateDirective($applyTo, 'must-revalidate', $mustRevalidate); return $this; } From 28f2ceac6f799b9961603f5d7e12ffd2ad00680b Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Fri, 6 Jul 2018 11:48:30 +0100 Subject: [PATCH 07/14] TEST Add tests for (im)mutablity of states --- .../HTTPCacheControlMiddlewareTest.php | 169 +++++++++++++++++- 1 file changed, 166 insertions(+), 3 deletions(-) diff --git a/tests/php/Control/Middleware/HTTPCacheControlMiddlewareTest.php b/tests/php/Control/Middleware/HTTPCacheControlMiddlewareTest.php index 3eeff44f0..52bbab6a8 100644 --- a/tests/php/Control/Middleware/HTTPCacheControlMiddlewareTest.php +++ b/tests/php/Control/Middleware/HTTPCacheControlMiddlewareTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\Control\Tests\Middleware; +use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\Middleware\HTTPCacheControlMiddleware; use SilverStripe\Dev\SapphireTest; @@ -12,15 +13,177 @@ class HTTPCacheControlMiddlewareTest extends SapphireTest parent::setUp(); // Set to disabled at null forcing level HTTPCacheControlMiddleware::config() - ->set('defaultState', 'disabled') + ->set('defaultState', HTTPCacheControlMiddleware::STATE_ENABLED) ->set('defaultForcingLevel', 0); HTTPCacheControlMiddleware::reset(); } + public function provideCacheStates() + { + return [ + ['enableCache', false], + ['publicCache', false], + ['privateCache', false], + ['disableCache', true], + ]; + } + + /** + * @dataProvider provideCacheStates + */ + public function testCheckDefaultStates($state, $immutable) + { + $cc = HTTPCacheControlMiddleware::singleton(); + $cc->{$state}(); + + $response = new HTTPResponse(); + $cc->applyToResponse($response); + + $this->assertContains('must-revalidate', $response->getHeader('cache-control')); + } + + /** + * @dataProvider provideCacheStates + */ + public function testSetMaxAge($state, $immutable) + { + $cc = HTTPCacheControlMiddleware::singleton(); + $cc->{$state}(); + + $originalResponse = new HTTPResponse(); + $cc->applyToResponse($originalResponse); + + $cc->setMaxAge('300'); + + $response = new HTTPResponse(); + + $cc->applyToResponse($response); + + if ($immutable) { + $this->assertEquals($originalResponse->getHeader('cache-control'), $response->getHeader('cache-control')); + } else { + $this->assertContains('max-age=300', $response->getHeader('cache-control')); + $this->assertNotContains('no-cache', $response->getHeader('cache-control')); + $this->assertNotContains('no-store', $response->getHeader('cache-control')); + } + } + + /** + * @dataProvider provideCacheStates + */ + public function testSetNoStore($state, $immutable) + { + $cc = HTTPCacheControlMiddleware::singleton(); + $cc->setMaxAge('300'); + $cc->setSharedMaxAge('300'); + + $cc->{$state}(); + + $originalResponse = new HTTPResponse(); + $cc->applyToResponse($originalResponse); + + $cc->setNoStore(); + + $response = new HTTPResponse(); + + $cc->applyToResponse($response); + + if ($immutable) { + $this->assertEquals($originalResponse->getHeader('cache-control'), $response->getHeader('cache-control')); + } else { + $this->assertContains('no-store', $response->getHeader('cache-control')); + $this->assertNotContains('max-age', $response->getHeader('cache-control')); + $this->assertNotContains('s-maxage', $response->getHeader('cache-control')); + } + } + + /** + * @dataProvider provideCacheStates + */ + public function testSetNoCache($state, $immutable) + { + $cc = HTTPCacheControlMiddleware::singleton(); + $cc->setMaxAge('300'); + $cc->setSharedMaxAge('300'); + + $cc->{$state}(); + + $originalResponse = new HTTPResponse(); + $cc->applyToResponse($originalResponse); + + $cc->setNoCache(); + + $response = new HTTPResponse(); + + $cc->applyToResponse($response); + + if ($immutable) { + $this->assertEquals($originalResponse->getHeader('cache-control'), $response->getHeader('cache-control')); + } else { + $this->assertContains('no-cache', $response->getHeader('cache-control')); + $this->assertNotContains('max-age', $response->getHeader('cache-control')); + $this->assertNotContains('s-maxage', $response->getHeader('cache-control')); + } + } + + /** + * @dataProvider provideCacheStates + */ + public function testSetSharedMaxAge($state, $immutable) + { + $cc = HTTPCacheControlMiddleware::singleton(); + + $cc->{$state}(); + + $originalResponse = new HTTPResponse(); + $cc->applyToResponse($originalResponse); + + $cc->setSharedMaxAge('300'); + + $response = new HTTPResponse(); + + $cc->applyToResponse($response); + + if ($immutable) { + $this->assertEquals($originalResponse->getHeader('cache-control'), $response->getHeader('cache-control')); + } else { + $this->assertContains('s-maxage=300', $response->getHeader('cache-control')); + $this->assertNotContains('no-cache', $response->getHeader('cache-control')); + $this->assertNotContains('no-store', $response->getHeader('cache-control')); + } + } + + /** + * @dataProvider provideCacheStates + */ + public function testSetMustRevalidate($state, $immutable) + { + $cc = HTTPCacheControlMiddleware::singleton(); + + $cc->{$state}(); + + $originalResponse = new HTTPResponse(); + $cc->applyToResponse($originalResponse); + + $cc->setMustRevalidate(); + + $response = new HTTPResponse(); + + $cc->applyToResponse($response); + + if ($immutable) { + $this->assertEquals($originalResponse->getHeader('cache-control'), $response->getHeader('cache-control')); + } else { + $this->assertContains('must-revalidate', $response->getHeader('cache-control')); + $this->assertNotContains('max-age', $response->getHeader('cache-control')); + $this->assertNotContains('s-maxage', $response->getHeader('cache-control')); + } + } + public function testCachingPriorities() { $hcc = new HTTPCacheControlMiddleware(); - $this->assertTrue($this->isDisabled($hcc), 'caching starts as disabled'); + $this->assertFalse($this->isDisabled($hcc), 'caching starts as disabled'); $hcc->enableCache(); $this->assertFalse($this->isDisabled($hcc)); @@ -74,6 +237,6 @@ class HTTPCacheControlMiddlewareTest extends SapphireTest protected function isDisabled(HTTPCacheControlMiddleware $hcc) { - return $hcc->hasDirective('no-cache') && !$hcc->hasDirective('private') && !$hcc->hasDirective('public'); + return $hcc->hasDirective('no-store') && !$hcc->hasDirective('private') && !$hcc->hasDirective('public'); } } From 8703839eb142ba0414f4d84f885ff898c39d6786 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Sat, 14 Jul 2018 19:30:29 +0100 Subject: [PATCH 08/14] FIX updateValidatePassword calls need to be masked from backtraces --- src/Dev/Backtrace.php | 6 ++++- tests/php/Dev/BacktraceTest.php | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/Dev/Backtrace.php b/src/Dev/Backtrace.php index 756945c52..daceca751 100644 --- a/src/Dev/Backtrace.php +++ b/src/Dev/Backtrace.php @@ -45,6 +45,7 @@ class Backtrace array('SilverStripe\\Security\\PasswordEncryptor_MySQLOldPassword', 'salt'), array('SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'encrypt'), array('SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'salt'), + array('*', 'updateValidatePassword'), ); /** @@ -106,7 +107,10 @@ class Backtrace $match = false; if (!empty($bt[$i]['class'])) { foreach ($ignoredArgs as $fnSpec) { - if (is_array($fnSpec) && $bt[$i]['class'] == $fnSpec[0] && $bt[$i]['function'] == $fnSpec[1]) { + if (is_array($fnSpec) && + ('*' == $fnSpec[0] || $bt[$i]['class'] == $fnSpec[0]) && + $bt[$i]['function'] == $fnSpec[1] + ) { $match = true; } } diff --git a/tests/php/Dev/BacktraceTest.php b/tests/php/Dev/BacktraceTest.php index 8a7f476f8..c10e47e79 100644 --- a/tests/php/Dev/BacktraceTest.php +++ b/tests/php/Dev/BacktraceTest.php @@ -68,4 +68,45 @@ class BacktraceTest extends SapphireTest $this->assertEquals('', $filtered[1]['args']['password'], 'Filters class functions'); $this->assertEquals('myval', $filtered[2]['args']['myarg'], 'Doesnt filter other functions'); } + + public function testFilteredWildCard() + { + $bt = array( + array( + 'type' => '->', + 'file' => 'MyFile.php', + 'line' => 99, + 'function' => 'myIgnoredGlobalFunction', + 'args' => array('password' => 'secred',) + ), + array( + 'class' => 'MyClass', + 'type' => '->', + 'file' => 'MyFile.php', + 'line' => 99, + 'function' => 'myIgnoredClassFunction', + 'args' => array('password' => 'secred',) + ), + array( + 'class' => 'MyClass', + 'type' => '->', + 'file' => 'MyFile.php', + 'line' => 99, + 'function' => 'myFunction', + 'args' => array('myarg' => 'myval') + ) + ); + Backtrace::config()->update( + 'ignore_function_args', + array( + array('*', 'myIgnoredClassFunction'), + ) + ); + + $filtered = Backtrace::filter_backtrace($bt); + + $this->assertEquals('secred', $filtered[0]['args']['password']); + $this->assertEquals('', $filtered[1]['args']['password']); + $this->assertEquals('myval', $filtered[2]['args']['myarg']); + } } From d1229956523d69f63c9e725b261c0142d5ee1de3 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 13 Jul 2018 10:33:12 +1200 Subject: [PATCH 09/14] FIX Duplicate config values for cascade_duplicates no longer duplicate their duplicates Previously you could define identical values for this config prop via a DataExtension and on the base class, resulting in double duplication --- src/ORM/DataObject.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index c6356fe43..397e0e6b1 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -428,6 +428,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // Get duplicates if ($relations === null) { $relations = $this->config()->get('cascade_duplicates'); + // Remove any duplicate entries before duplicating them + if (is_array($relations)) { + $relations = array_unique($relations); + } } // Create unsaved raw duplicate From b93e94c0c38b954e16cf775f7f1d1cd60cbfd703 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 25 Jun 2018 11:06:43 +1200 Subject: [PATCH 10/14] FIX FormField::Link now throws a LogicException if no form is set yet --- src/Forms/FormField.php | 11 +++++++++-- tests/php/Forms/FormFieldTest.php | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/Forms/FormField.php b/src/Forms/FormField.php index 8b6c3a571..502163b0b 100644 --- a/src/Forms/FormField.php +++ b/src/Forms/FormField.php @@ -2,6 +2,7 @@ namespace SilverStripe\Forms; +use LogicException; use ReflectionClass; use SilverStripe\Control\Controller; use SilverStripe\Control\RequestHandler; @@ -355,14 +356,20 @@ class FormField extends RequestHandler } /** - * Return a link to this field. + * Return a link to this field * * @param string $action - * * @return string + * @throws LogicException If no form is set yet */ public function Link($action = null) { + if (!$this->form) { + throw new LogicException( + 'Field must be associated with a form to call Link(). Please use $field->setForm($form);' + ); + } + $link = Controller::join_links($this->form->FormAction(), 'field/' . $this->name, $action); $this->extend('updateLink', $link, $action); return $link; diff --git a/tests/php/Forms/FormFieldTest.php b/tests/php/Forms/FormFieldTest.php index 07c5d618e..b63cbec76 100644 --- a/tests/php/Forms/FormFieldTest.php +++ b/tests/php/Forms/FormFieldTest.php @@ -384,4 +384,22 @@ class FormFieldTest extends SapphireTest $this->assertFalse($field->hasClass('banana')); $this->assertTrue($field->hasClass('cool-BAnana')); } + + public function testLinkWithForm() + { + $field = new FormField('Test'); + $form = new Form(null, 'Test', new FieldList, new FieldList); + $form->setFormAction('foo'); + $field->setForm($form); + $this->assertSame('foo/field/Test/bar', $field->Link('bar')); + } + + /** + * @expectedException \LogicException + */ + public function testLinkWithoutForm() + { + $field = new FormField('Test'); + $field->Link('bar'); + } } From df5395b10191de986aafbf1c0aff8b82204b82bc Mon Sep 17 00:00:00 2001 From: Gerald Baumeister Date: Mon, 16 Jul 2018 18:08:25 +0200 Subject: [PATCH 11/14] Added check for php-intl requirement --- src/Dev/Install/InstallRequirements.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Dev/Install/InstallRequirements.php b/src/Dev/Install/InstallRequirements.php index ed2529278..d83d61215 100644 --- a/src/Dev/Install/InstallRequirements.php +++ b/src/Dev/Install/InstallRequirements.php @@ -384,6 +384,13 @@ class InstallRequirements 'SimpleXML support not included in PHP.' )); + // Check for INTL support + $this->requireClass('IntlTimeZone', array( + 'PHP Configuration', + 'Intl support', + 'Internationalization (php-intl) support not included in PHP.' + )); + // Check for token_get_all $this->requireFunction('token_get_all', array( "PHP Configuration", From 93b0884e196fefe79ef7fdee9b9e3e6cfdf46cd4 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 18 Jul 2018 14:24:27 +1200 Subject: [PATCH 12/14] BUG Lazy session state (fixes #8267) Fixes regression from 3.x, where sessions where lazy started as required: Either because an existing session identifier was sent through with the request, or because new session data needed to be persisted as part of the request execution. Without this lazy starting, *every* request will get a session, which makes all those responses uncacheable by HTTP layers. Note that 4.x also changed the $data vs. $changedData payloads: In 3.x, they both contained key/value pairs. In 4.x, $data contains key/value, while $changedData contains key/boolean to declare isChanged. While this reduces duplication in the class, it also surfaced a bug which was latent in 3.x: When an existing session is lazily resumed via start(), $data is set back to an empty array. In 3.x, any changed data before this point was *also* retained in $changedData, ensuring it gets merged into existing $_SESSION data. In 4.x, this clears out data - hence the need for a more complex merge logic. Since isset($this->data) is no longer an accurate indicator of a started session, we introduce a separate $this->started flag. Note that I've chosen not to make lazy an opt-in (e.g. via start($request, $lazy=false)). We already have a distinction between lazy starting via init(), and force starting via start(). --- src/Control/Session.php | 77 ++++++++++++------- .../SessionAuthenticationHandler.php | 9 ++- 2 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/Control/Session.php b/src/Control/Session.php index d0e68f35e..17a2c295a 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -49,6 +49,9 @@ use SilverStripe\Dev\Deprecation; * * Once you have saved a value to the Session you can access it by using the {@link Session::get()} function. * Like the {@link Session::set()} function you can use this anywhere in your PHP files. + * Note that session data isn't persisted in PHP's own session store (via $_SESSION) + * until {@link Session::save()} is called, which happens automatically at the end of a standard request + * through {@link SilverStripe\Control\Middleware\SessionMiddleware}. * * The values in the comments are the values stored from the previous example. * @@ -84,7 +87,6 @@ use SilverStripe\Dev\Deprecation; * * * @see Cookie - * @todo This class is currently really basic and could do with a more well-thought-out implementation. */ class Session { @@ -128,6 +130,12 @@ class Session */ private static $cookie_secure = false; + /** + * @config + * @var string + */ + private static $cookie_name_secure = 'SECSESSID'; + /** * Name of session cache limiter to use. * Defaults to '' to disable cache limiter entirely. @@ -145,6 +153,11 @@ class Session */ protected $data = null; + /** + * @var bool + */ + protected $started = false; + /** * List of keys changed. This is a nested array which represents the * keys modified in $this->data. The value of each item is either "true" @@ -192,16 +205,21 @@ class Session } $this->data = $data; + $this->started = isset($data); } /** - * Init this session instance before usage + * Init this session instance before usage, + * if a session identifier is part of the passed in request. + * Otherwise, a session might be started in {@link save()} + * if session data needs to be written with a new session identifier. * * @param HTTPRequest $request */ public function init(HTTPRequest $request) { - if (!$this->isStarted()) { + + if (!$this->isStarted() && $this->requestContainsSessionId($request)) { $this->start($request); } @@ -210,6 +228,7 @@ class Session if ($this->data['HTTP_USER_AGENT'] !== $this->userAgent($request)) { $this->clearAll(); $this->destroy(); + $this->started = false; $this->start($request); } } @@ -233,11 +252,24 @@ class Session */ public function isStarted() { - return isset($this->data); + return $this->started; } /** - * Begin session + * @param HTTPRequest $request + * @return bool + */ + public function requestContainsSessionId(HTTPRequest $request) + { + $secure = Director::is_https($request) && $this->config()->get('cookie_secure'); + $name = $secure ? $this->config()->get('cookie_name_secure') : session_name(); + return (bool)Cookie::get($name); + } + + /** + * Begin session, regardless if a session identifier is present in the request, + * or whether any session data needs to be written. + * See {@link init()} if you want to "lazy start" a session. * * @param HTTPRequest $request The request for which to start a session */ @@ -281,7 +313,7 @@ class Session // If we want a secure cookie for HTTPS, use a seperate session name. This lets us have a // seperate (less secure) session for non-HTTPS requests if ($secure) { - session_name('SECSESSID'); + session_name($this->config()->get('cookie_name_secure')); } $limiter = $this->config()->get('sessionCacheLimiter'); @@ -291,7 +323,16 @@ class Session session_start(); - $this->data = isset($_SESSION) ? $_SESSION : array(); + if (isset($_SESSION)) { + // Initialise data from session store if present + $data = $_SESSION; + // Merge in existing in-memory data, taking priority over session store data + $this->recursivelyApply((array)$this->data, $data); + } else { + // Use in-memory data if the session is lazy started + $data = isset($this->data) ? $this->data : []; + } + $this->data = $data; } else { $this->data = []; } @@ -302,6 +343,8 @@ class Session Cookie::set(session_name(), session_id(), $timeout/86400, $path, $domain ? $domain : null, $secure, true); } + + $this->started = true; } /** @@ -335,9 +378,6 @@ class Session */ public function set($name, $val) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } $var = &$this->nestedValueRef($name, $this->data); // Mark changed @@ -380,10 +420,6 @@ class Session */ public function addToArray($name, $val) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } - $names = explode('.', $name); // We still want to do this even if we have strict path checking for legacy code @@ -407,9 +443,6 @@ class Session */ public function get($name) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be accessed until it's started"); - } return $this->nestedValue($name, $this->data); } @@ -421,10 +454,6 @@ class Session */ public function clear($name) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } - // Get var by path $var = $this->nestedValue($name, $this->data); @@ -449,10 +478,6 @@ class Session */ public function clearAll() { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } - if ($this->data && is_array($this->data)) { foreach (array_keys($this->data) as $key) { $this->clear($key); @@ -495,7 +520,7 @@ class Session $this->start($request); } - // Apply all changes recursively + // Apply all changes recursively, implicitly writing them to the actual PHP session store. $this->recursivelyApplyChanges($this->changedData, $this->data, $_SESSION); } } diff --git a/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php b/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php index a5c9ed240..1aafc020c 100644 --- a/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php +++ b/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php @@ -45,9 +45,16 @@ class SessionAuthenticationHandler implements AuthenticationHandler */ public function authenticateRequest(HTTPRequest $request) { + $session = $request->getSession(); + + // Sessions are only started when a session cookie is detected + if (!$session->isStarted()) { + return null; + } + // If ID is a bad ID it will be treated as if the user is not logged in, rather than throwing a // ValidationException - $id = $request->getSession()->get($this->getSessionVariable()); + $id = $session->get($this->getSessionVariable()); if (!$id) { return null; } From e415bcb44a32e0f64f6c0a18b0afc97f0eb9edde Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 18 Jul 2018 21:15:56 +1200 Subject: [PATCH 13/14] Fix tests on unset session data Thanks Robbie! --- src/Control/Director.php | 2 +- src/Control/Session.php | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index 79622d213..6e7aef593 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -206,7 +206,7 @@ class Director implements TemplateGlobalProvider if ($session instanceof Session) { // Note: If passing $session as object, ensure that changes are written back // This is important for classes such as FunctionalTest which emulate cross-request persistence - $newVars['_SESSION'] = $sessionArray = $session->getAll(); + $newVars['_SESSION'] = $sessionArray = $session->getAll() ?: []; $finally[] = function () use ($session, $sessionArray) { if (isset($_SESSION)) { // Set new / updated keys diff --git a/src/Control/Session.php b/src/Control/Session.php index 17a2c295a..f9f95d1cf 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -262,8 +262,8 @@ class Session public function requestContainsSessionId(HTTPRequest $request) { $secure = Director::is_https($request) && $this->config()->get('cookie_secure'); - $name = $secure ? $this->config()->get('cookie_name_secure') : session_name(); - return (bool)Cookie::get($name); + $name = $secure ? $this->config()->get('cookie_name_secure') : session_name(); + return (bool)Cookie::get($name); } /** @@ -330,9 +330,9 @@ class Session $this->recursivelyApply((array)$this->data, $data); } else { // Use in-memory data if the session is lazy started - $data = isset($this->data) ? $this->data : []; + $data = $this->data; } - $this->data = $data; + $this->data = $data ?: []; } else { $this->data = []; } @@ -615,6 +615,7 @@ class Session */ protected function recursivelyApplyChanges($changes, $source, &$destination) { + $source = $source ?: []; foreach ($changes as $key => $changed) { if ($changed === true) { // Determine if replacement or removal From c541283093460faa6b3db60258b1bf5f36ae42cb Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 18 Jul 2018 21:16:06 +1200 Subject: [PATCH 14/14] Test coverage for session data change --- tests/php/Control/SessionTest.php | 142 ++++++++++++++++++ .../SessionAuthenticationHandlerTest.php | 61 ++++++++ 2 files changed, 203 insertions(+) create mode 100644 tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php diff --git a/tests/php/Control/SessionTest.php b/tests/php/Control/SessionTest.php index f1b5a5430..724e778f5 100644 --- a/tests/php/Control/SessionTest.php +++ b/tests/php/Control/SessionTest.php @@ -2,6 +2,8 @@ namespace SilverStripe\Control\Tests; +use http\Exception\BadMessageException; +use SilverStripe\Control\Cookie; use SilverStripe\Control\Session; use SilverStripe\Dev\SapphireTest; use SilverStripe\Control\HTTPRequest; @@ -22,6 +24,127 @@ class SessionTest extends SapphireTest return parent::setUp(); } + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testInitDoesNotStartSessionWithoutIdentifier() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $session->init($req); + $this->assertFalse($session->isStarted()); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testInitStartsSessionWithIdentifier() + { + $req = new HTTPRequest('GET', '/'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->init($req); + $this->assertTrue($session->isStarted()); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testInitStartsSessionWithData() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session([]); + $session->init($req); + $this->assertTrue($session->isStarted()); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartUsesDefaultCookieNameWithHttp() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('http'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->start($req); + $this->assertNotEquals(session_name(), $session->config()->get('cookie_name_secure')); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartUsesDefaultCookieNameWithHttpsAndCookieSecureOff() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('https'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->start($req); + $this->assertNotEquals(session_name(), $session->config()->get('cookie_name_secure')); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartUsesSecureCookieNameWithHttpsAndCookieSecureOn() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('https'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->config()->update('cookie_secure', true); + $session->start($req); + $this->assertEquals(session_name(), $session->config()->get('cookie_name_secure')); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + * @expectedException BadMethodCallException + * @expectedExceptionMessage Session has already started + */ + public function testStartErrorsWhenStartingTwice() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $session->start($req); + $session->start($req); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartRetainsInMemoryData() + { + $this->markTestIncomplete('Test'); + // TODO Figure out how to simulate session vars without a session_start() resetting them + // $_SESSION['existing'] = true; + // $_SESSION['merge'] = 1; + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $session->set('new', true); + $session->set('merge', 2); + $session->start($req); // simulate lazy start + $this->assertEquals( + [ + // 'existing' => true, + 'new' => true, + 'merge' => 2 + ], + $session->getAll() + ); + + unset($_SESSION); + } + public function testGetSetBasics() { $this->session->set('Test', 'Test'); @@ -124,6 +247,25 @@ class SessionTest extends SapphireTest ); } + public function testRequestContainsSessionId() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $this->assertFalse($session->requestContainsSessionId($req)); + Cookie::set(session_name(), '1234'); + $this->assertTrue($session->requestContainsSessionId($req)); + } + + public function testRequestContainsSessionIdRespectsCookieNameSecure() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('https'); + $session = new Session(null); // unstarted session + Cookie::set($session->config()->get('cookie_name_secure'), '1234'); + $session->config()->update('cookie_secure', true); + $this->assertTrue($session->requestContainsSessionId($req)); + } + public function testUserAgentLockout() { // Set a user agent diff --git a/tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php b/tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php new file mode 100644 index 000000000..800e49563 --- /dev/null +++ b/tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php @@ -0,0 +1,61 @@ + 'test@example.com']); + $member->write(); + + $handler = new SessionAuthenticationHandler(); + + $session = new Session(null); // unstarted, simulates lack of session cookie + $session->set($handler->getSessionVariable(), $member->ID); + + $req = new HTTPRequest('GET', '/'); + $req->setSession($session); + + $matchedMember = $handler->authenticateRequest($req); + $this->assertNull($matchedMember); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testAuthenticateRequestStartsSessionWithSessionIdentifier() + { + $member = new Member(['Email' => 'test@example.com']); + $member->write(); + + $handler = new SessionAuthenticationHandler(); + + $session = new Session(null); // unstarted + $session->set($handler->getSessionVariable(), $member->ID); + + $req = new HTTPRequest('GET', '/'); + $req->setSession($session); + + Cookie::set(session_name(), '1234'); + $session->start($req); // simulate detection of session cookie + + $matchedMember = $handler->authenticateRequest($req); + $this->assertNotNull($matchedMember); + $this->assertEquals($matchedMember->Email, $member->Email); + } +}