Merge pull request #7399 from dhensby/pulls/4/reduce-dependence-on-session-for-reading-mode

Reduce dependence on session state for accessing draft stages

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 

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)

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

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

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;
     }
 

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()

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

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;

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;
     }
 
     /**

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;
     }
 
     /**

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;
     }
 
     /**

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();
     }
 
     /**

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']),
             '/'
         );

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;
     }
 
     /**

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,
     ];