diff --git a/docs/en/02_Developer_Guides/00_Model/10_Versioning.md b/docs/en/02_Developer_Guides/00_Model/10_Versioning.md index ac824e28c..12f46464e 100644 --- a/docs/en/02_Developer_Guides/00_Model/10_Versioning.md +++ b/docs/en/02_Developer_Guides/00_Model/10_Versioning.md @@ -552,12 +552,99 @@ public function init() } ``` -### Controllers +### Controllers {#controllers} -The current stage for each request is determined by `VersionedRequestFilter` before any controllers initialize, through -`Versioned::choose_site_stage()`. It checks for a `Stage` GET parameter, so you can force a draft stage by appending -`?stage=Stage` to your request. The setting is "sticky" in the PHP session, so any subsequent requests will also be in -draft stage. +The current stage for each request is determined by `VersionedHTTPMiddleware` before any controllers initialize, through +`Versioned::choose_site_stage()`. It checks for a `stage` GET parameter, so you can force a draft stage by appending +`?stage=Stage` to your request. + +Since SilverStripe 4.2, the current stage setting is no longer "sticky" in the session. +Any links presented on the view produced with `?stage=Stage` need to have the same GET parameters in order +to retain the stage. If you are using the `SiteTree->Link()` and `Controller->Link()` methods, +this is automatically the case for `DataObject` links, controller links and form actions. +Note that this behaviour applies for unversioned objects as well, since the views +these are presented in might still contain dependant objects that are versioned. + +You can opt for a session base stage setting through the `Versioned.use_session` setting. +Warning: This can lead to leaking of unpublished information, if a live URL is viewed in draft mode, +and the result is cached due to agressive cache settings (not varying on cookie values). + +*mysite/src/MyObject.php* + +```php +use SilverStripe\Core\Injector\Injector; +use SilverStripe\ORM\DataObject; +use SilverStripe\Versioned\Versioned; +use SilverStripe\Control\Controller; + +class MyObject extends DataObject { + + private static $extensions = [ + Versioned::class + ]; + + public function Link() + { + return Injector::inst()->get(MyObjectController::class)->Link($this->ID); + } + + public function CustomLink() + { + $link = Controller::join_links('custom-route', $this->ID, '?rand=' . rand()); + $this->extend('updateLink', $link); // updates $link by reference + return $link; + } + + public function LiveLink() + { + // Force live link even when current view is in draft mode + return Controller::join_links(Injector::inst()->get(MyObjectController::class)->Link($this->ID), '?stage=Live'); + } +} +``` + +*mysite/src/MyObjectController.php* + +```php +use SilverStripe\Control\Controller; +use SilverStripe\Control\HTTPRequest; + +class MyObjectController extends Controller +{ + public function index(HTTPRequest $request) + { + $obj = MyObject::get()->byID($request->param('ID')); + if (!$obj) { + return $this->httpError(404); + } + + // Construct view + $html = sprintf('<a href="%s">%s</a>', $obj->Link(), $obj->ID); + + return $html; + } + + public function Link($action = null) + { + // Construct link with graceful handling of GET parameters + $link = Controller::join_links('my-objects', $action); + + // Allow Versioned and other extension to update $link by reference. + // Calls VersionedStateExtension->updateLink(). + $this->extend('updateLink', $link, $action); + + return $link; + } +} +``` + +*mysite/_config/routes.yml* + +```yml +SilverStripe\Control\Director: + rules: + 'my-objects/$ID': 'MyObjectController' +``` <div class="alert" markdown="1"> The `choose_site_stage()` call only deals with setting the default stage, and doesn't check if the user is diff --git a/docs/en/02_Developer_Guides/02_Controllers/01_Introduction.md b/docs/en/02_Developer_Guides/02_Controllers/01_Introduction.md index 44c6810b4..c44f532dc 100644 --- a/docs/en/02_Developer_Guides/02_Controllers/01_Introduction.md +++ b/docs/en/02_Developer_Guides/02_Controllers/01_Introduction.md @@ -159,20 +159,23 @@ For more information about templates, inheritance and how to render into views, ## Link -Each controller should define a `Link()` method. This should be used to avoid hard coding your routing in views etc. +Each controller should define a `Link()` method. This should be used to avoid hard coding your routing in views, +as well as give other features in SilverStripe the ability to influence link behaviour. **mysite/code/controllers/TeamController.php** ```php public function Link($action = null) { - return Controller::join_links('teams', $action); + // Construct link with graceful handling of GET parameters + $link = Controller::join_links('teams', $ction); + + // Allow Versioned and other extension to update $link by reference. + $this->extend('updateLink', $link, $action); + + return $link; } -``` - -<div class="info" markdown="1"> -The [Controller::join_links()](api:SilverStripe\Control\Controller::join_links()) is optional, but makes `Link()` more flexible by allowing an `$action` argument, and concatenates the path segments with slashes. The action should map to a method on your controller. -</div> +``` ## Related Lessons * [Controller actions/DataObjects as pages](https://www.silverstripe.org/learn/lessons/v4/controller-actions-dataobjects-as-pages-1) diff --git a/docs/en/04_Changelogs/4.2.0.md b/docs/en/04_Changelogs/4.2.0.md new file mode 100644 index 000000000..dddf7738b --- /dev/null +++ b/docs/en/04_Changelogs/4.2.0.md @@ -0,0 +1,62 @@ +# 4.2.0 + +## Overview {#overview} + + * Disable session-based stage setting in `Versioned` (see [#1578](https://github.com/silverstripe/silverstripe-cms/issues/1578)) + * Deprecated `FunctionalTest::useDraftSite()`. You should use querystring args instead for setting stage. + +## Upgrading {#upgrading} + +### Disable session-based stage setting + +When viewing a versioned record (usually pages) in "draft" mode, +SilverStripe used to record this mode in the session for further requests. +This has the advantage of transparently working on XHR and API requests, +as well as authenticated users navigating through other views. + +These subsequent requests no longer carried an explicit `stage` query parameter, +which meant the same URL might show draft or live content depending on your session state. +While most HTTP caching layers deal gracefully with this variation by disabling +any caching when a session cookie is present, there is a small chance +that draft content is exposed to unauthenticated users for the lifetime of the cache. + +Due to this potential risk for information leakage, +we have decided to only rely on the `stage` query parameter. +If you are consistently using the built-in `SiteTree->Link()` +and `Controller->Link()` methods to get URLs, this change likely won't affect you. + +If you are manually concatenating URLs to SilverStripe controllers +rather than through their `Link()` methods (in custom PHP or JavaScript), +or have implemented your own `Link()` methods on controllers exposing +versioned objects, you'll need to check your business logic. + +Alternatively, you can opt-out of this security feature via YAML configuration: + +```yml +SilverStripe\Versioned\Versioned: + use_session: true +``` + +Check our [versioning docs](/developer_guides/model/versioning#controllers) +for more details. + +### New Versioned API + +The following methods have been added to [api:SilverStripe\Versioned\Versioned] class: + + * `withVersionedMode()` Allows users to execute a closure which may internally modify + the current stage, but will guarantee these changes are reverted safely on return. + Helpful when temporarily performing a task in another stage or view mode. + * `get_draft_site_secured()` / `set_draft_site_secured()` Enables the explicit toggle + of draft site security. By setting this to false, you can expose a draft mode to + unauthenticated users. Replaces `unsecuredDraftSite` session var. + * `get_default_reading_mode()` / `set_default_reading_mode()` The default reading + mode is now configurable. Any non-default reading mode must have querystring args + to be visible. This will be the mode choosen for requests that do not have these args. + Note that the default mode for CMS is now draft, but is live on the frontend. + +A new class [api:SilverStripe\Versioned\ReadingMode] has also been added to assist with +conversion of the reading mode between: + - Reading mode string + - DataQuery parameters + - Querystring parameters diff --git a/src/Control/RequestHandler.php b/src/Control/RequestHandler.php index a8f1cfa69..fc880def7 100644 --- a/src/Control/RequestHandler.php +++ b/src/Control/RequestHandler.php @@ -560,7 +560,11 @@ class RequestHandler extends ViewableData // Check configured url_segment $url = $this->config()->get('url_segment'); if ($url) { - return Controller::join_links($url, $action, '/'); + $link = Controller::join_links($url, $action, '/'); + + // Give extensions the chance to modify by reference + $this->extend('updateLink', $link, $action); + return $link; } // no link defined by default diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php index da338e204..e7e9597aa 100644 --- a/src/Core/Extensible.php +++ b/src/Core/Extensible.php @@ -48,7 +48,6 @@ trait Extensible */ private static $unextendable_classes = array( ViewableData::class, - RequestHandler::class, ); /** @@ -212,13 +211,6 @@ trait Extensible )); Injector::inst()->unregisterNamedObject($class); - - // load statics now for DataObject classes - if (is_subclass_of($class, DataObject::class)) { - if (!is_subclass_of($extensionClass, DataExtension::class)) { - user_error("$extensionClass cannot be applied to $class without being a DataExtension", E_USER_ERROR); - } - } return true; } diff --git a/src/Dev/FunctionalTest.php b/src/Dev/FunctionalTest.php index 7e14e944c..29f787ebc 100644 --- a/src/Dev/FunctionalTest.php +++ b/src/Dev/FunctionalTest.php @@ -2,18 +2,14 @@ namespace SilverStripe\Dev; -use SilverStripe\Control\Controller; +use PHPUnit_Framework_AssertionFailedError; use SilverStripe\Control\Director; -use SilverStripe\Control\Session; use SilverStripe\Control\HTTPResponse; +use SilverStripe\Control\Session; use SilverStripe\Core\Config\Config; -use SilverStripe\ORM\DataObject; use SilverStripe\Security\BasicAuth; -use SilverStripe\Security\Member; -use SilverStripe\Security\Security; use SilverStripe\Security\SecurityToken; use SilverStripe\View\SSViewer; -use PHPUnit_Framework_AssertionFailedError; use SimpleXMLElement; /** @@ -49,6 +45,7 @@ class FunctionalTest extends SapphireTest implements TestOnly /** * Set this to true on your sub-class to use the draft site by default for every test in this class. * + * @deprecated 4.2..5.0 Use ?stage=Stage in your ->get() querystring requests instead * @var bool */ protected static $use_draft_site = false; @@ -104,6 +101,7 @@ class FunctionalTest extends SapphireTest implements TestOnly $this->logOut(); // Switch to draft site, if necessary + // If you rely on this you should be crafting stage-specific urls instead though. if (static::get_use_draft_site()) { $this->useDraftSite(); } @@ -406,16 +404,18 @@ class FunctionalTest extends SapphireTest implements TestOnly * This is helpful if you're not testing publication functionality and don't want "stage management" cluttering * your test. * + * @deprecated 4.2..5.0 Use ?stage=Stage querystring arguments instead of useDraftSite * @param bool $enabled toggle the use of the draft site */ public function useDraftSite($enabled = true) { + Deprecation::notice('5.0', 'Use ?stage=Stage querystring arguments instead of useDraftSite'); if ($enabled) { $this->session()->set('readingMode', 'Stage.Stage'); $this->session()->set('unsecuredDraftSite', true); } else { - $this->session()->set('readingMode', 'Stage.Live'); - $this->session()->set('unsecuredDraftSite', false); + $this->session()->clear('readingMode'); + $this->session()->clear('unsecuredDraftSite'); } } @@ -428,6 +428,7 @@ class FunctionalTest extends SapphireTest implements TestOnly } /** + * @deprecated 4.2..5.0 Use ?stage=Stage in your querystring arguments instead * @return bool */ public static function get_use_draft_site() diff --git a/src/Dev/State/SapphireTestState.php b/src/Dev/State/SapphireTestState.php index ba2bd2556..3571a8006 100644 --- a/src/Dev/State/SapphireTestState.php +++ b/src/Dev/State/SapphireTestState.php @@ -3,7 +3,6 @@ namespace SilverStripe\Dev\State; use SilverStripe\Core\Injector\Injectable; -use SilverStripe\Dev\Debug; use SilverStripe\Dev\SapphireTest; class SapphireTestState implements TestState diff --git a/src/Dev/TestSession.php b/src/Dev/TestSession.php index 171ec509f..a27142632 100644 --- a/src/Dev/TestSession.php +++ b/src/Dev/TestSession.php @@ -2,15 +2,16 @@ namespace SilverStripe\Dev; -use SilverStripe\Control\Cookie_Backend; -use SilverStripe\Control\HTTPRequest; -use SilverStripe\Control\Session; +use Exception; use SilverStripe\Control\Controller; +use SilverStripe\Control\Cookie_Backend; use SilverStripe\Control\Director; +use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; +use SilverStripe\Control\Session; +use SilverStripe\Core\Extensible; use SilverStripe\Core\Injector\Injector; use SimpleByName; -use Exception; use SimplePage; use SimplePageBuilder; @@ -20,6 +21,7 @@ use SimplePageBuilder; */ class TestSession { + use Extensible; /** * @var Session @@ -89,6 +91,7 @@ class TestSession */ public function get($url, $session = null, $headers = null, $cookies = null) { + $this->extend('updateGetURL', $url, $session, $headers, $cookies); $headers = (array) $headers; if ($this->lastUrl && !isset($headers['Referer'])) { $headers['Referer'] = $this->lastUrl; @@ -123,6 +126,7 @@ class TestSession */ public function post($url, $data, $headers = null, $session = null, $body = null, $cookies = null) { + $this->extend('updatePostURL', $url, $data, $headers, $session, $body, $cookies); $headers = (array) $headers; if ($this->lastUrl && !isset($headers['Referer'])) { $headers['Referer'] = $this->lastUrl; diff --git a/src/Forms/FormField.php b/src/Forms/FormField.php index 0c9108fcd..8b6c3a571 100644 --- a/src/Forms/FormField.php +++ b/src/Forms/FormField.php @@ -363,7 +363,9 @@ class FormField extends RequestHandler */ public function Link($action = null) { - return Controller::join_links($this->form->FormAction(), 'field/' . $this->name, $action); + $link = Controller::join_links($this->form->FormAction(), 'field/' . $this->name, $action); + $this->extend('updateLink', $link, $action); + return $link; } /** diff --git a/src/Forms/FormRequestHandler.php b/src/Forms/FormRequestHandler.php index 0244a11f5..408f57d77 100644 --- a/src/Forms/FormRequestHandler.php +++ b/src/Forms/FormRequestHandler.php @@ -85,15 +85,15 @@ class FormRequestHandler extends RequestHandler // Respect FormObjectLink() method if ($controller->hasMethod("FormObjectLink")) { - return Controller::join_links( - $controller->FormObjectLink($this->form->getName()), - $action, - '/' - ); + $base = $controller->FormObjectLink($this->form->getName()); + } else { + $base = Controller::join_links($controller->Link(), $this->form->getName()); } - // Default form link - return Controller::join_links($controller->Link(), $this->form->getName(), $action, '/'); + // Join with action and decorate + $link = Controller::join_links($base, $action, '/'); + $this->extend('updateLink', $link, $action); + return $link; } /** diff --git a/src/Security/MemberAuthenticator/ChangePasswordHandler.php b/src/Security/MemberAuthenticator/ChangePasswordHandler.php index 3149f3404..74cc528fe 100644 --- a/src/Security/MemberAuthenticator/ChangePasswordHandler.php +++ b/src/Security/MemberAuthenticator/ChangePasswordHandler.php @@ -7,7 +7,6 @@ use Psr\Container\NotFoundExceptionInterface; use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\RequestHandler; -use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injector; use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\FieldType\DBField; @@ -25,9 +24,11 @@ class ChangePasswordHandler extends RequestHandler protected $authenticator; /** + * Link to this handler + * * @var string */ - protected $link; + protected $link = null; /** * @var array Allowed Actions @@ -123,8 +124,8 @@ class ChangePasswordHandler extends RequestHandler . '<p>You can request a new one <a href="{link1}">here</a> or change your password after' . ' you <a href="{link2}">logged in</a>.</p>', [ - 'link1' => $this->link('lostpassword'), - 'link2' => $this->link('login') + 'link1' => $this->Link('lostpassword'), + 'link2' => $this->Link('login') ] ) ); @@ -164,16 +165,15 @@ class ChangePasswordHandler extends RequestHandler /** * Return a link to this request handler. * The link returned is supplied in the constructor - * @param null $action + * + * @param string|null $action * @return string */ - public function link($action = null) + public function Link($action = null) { - if ($action) { - return Controller::join_links($this->link, $action); - } - - return $this->link; + $link = Controller::join_links($this->link, $action); + $this->extend('updateLink', $link, $action); + return $link; } /** diff --git a/src/Security/MemberAuthenticator/LoginHandler.php b/src/Security/MemberAuthenticator/LoginHandler.php index 2058c7864..f97538810 100644 --- a/src/Security/MemberAuthenticator/LoginHandler.php +++ b/src/Security/MemberAuthenticator/LoginHandler.php @@ -41,9 +41,11 @@ class LoginHandler extends RequestHandler ]; /** - * @var string Called link on this handler + * Link to this handler + * + * @var string */ - private $link; + protected $link = null; /** * @param string $link The URL to recreate this request handler @@ -59,16 +61,15 @@ class LoginHandler extends RequestHandler /** * Return a link to this request handler. * The link returned is supplied in the constructor + * * @param null|string $action * @return string */ - public function link($action = null) + public function Link($action = null) { - if ($action) { - return Controller::join_links($this->link, $action); - } - - return $this->link; + $link = Controller::join_links($this->link, $action); + $this->extend('updateLink', $link, $action); + return $link; } /** @@ -152,7 +153,7 @@ class LoginHandler extends RequestHandler public function getReturnReferer() { - return $this->link(); + return $this->Link(); } /** diff --git a/src/Security/MemberAuthenticator/LostPasswordHandler.php b/src/Security/MemberAuthenticator/LostPasswordHandler.php index d5d4b4e22..5086e525c 100644 --- a/src/Security/MemberAuthenticator/LostPasswordHandler.php +++ b/src/Security/MemberAuthenticator/LostPasswordHandler.php @@ -44,7 +44,12 @@ class LostPasswordHandler extends RequestHandler 'passwordsent', ]; - private $link = null; + /** + * Link to this handler + * + * @var string + */ + protected $link = null; /** * @param string $link The URL to recreate this request handler @@ -59,16 +64,14 @@ class LostPasswordHandler extends RequestHandler * Return a link to this request handler. * The link returned is supplied in the constructor * - * @param string $action + * @param string|null $action * @return string */ - public function link($action = null) + public function Link($action = null) { - if ($action) { - return Controller::join_links($this->link, $action); - } - - return $this->link; + $link = Controller::join_links($this->link, $action); + $this->extend('updateLink', $link, $action); + return $link; } /** @@ -261,7 +264,7 @@ class LostPasswordHandler extends RequestHandler protected function redirectToSuccess(array $data) { $link = Controller::join_links( - $this->link('passwordsent'), + $this->Link('passwordsent'), rawurlencode($data['Email']), '/' ); diff --git a/src/Security/Security.php b/src/Security/Security.php index 675ff0ba0..e3952c056 100644 --- a/src/Security/Security.php +++ b/src/Security/Security.php @@ -483,7 +483,9 @@ class Security extends Controller implements TemplateGlobalProvider public function Link($action = null) { /** @skipUpgrade */ - return Controller::join_links(Director::baseURL(), "Security", $action); + $link = Controller::join_links(Director::baseURL(), "Security", $action); + $this->extend('updateLink', $link, $action); + return $link; } /** diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php index b986dadd6..76aed937c 100644 --- a/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php +++ b/tests/php/Forms/HTMLEditor/HTMLEditorFieldTest.php @@ -2,30 +2,25 @@ namespace SilverStripe\Forms\Tests\HTMLEditor; +use Silverstripe\Assets\Dev\TestAssetStore; use SilverStripe\Assets\File; use SilverStripe\Assets\FileNameFilter; use SilverStripe\Assets\Filesystem; use SilverStripe\Assets\Folder; use SilverStripe\Assets\Image; -use Silverstripe\Assets\Dev\TestAssetStore; use SilverStripe\Core\Config\Config; -use SilverStripe\Core\Manifest\ModuleLoader; -use SilverStripe\Core\Manifest\ModuleManifest; -use SilverStripe\Core\Manifest\ModuleResourceLoader; use SilverStripe\Dev\CSSContentParser; use SilverStripe\Dev\FunctionalTest; use SilverStripe\Forms\HTMLEditor\HTMLEditorField; +use SilverStripe\Forms\HTMLEditor\TinyMCEConfig; use SilverStripe\Forms\HTMLReadonlyField; use SilverStripe\Forms\Tests\HTMLEditor\HTMLEditorFieldTest\TestObject; use SilverStripe\ORM\FieldType\DBHTMLText; -use SilverStripe\Forms\HTMLEditor\TinyMCEConfig; class HTMLEditorFieldTest extends FunctionalTest { protected static $fixture_file = 'HTMLEditorFieldTest.yml'; - protected static $use_draft_site = true; - protected static $extra_dataobjects = [ TestObject::class, ];