tonyair
/
silverstripe-framework
mirror of https://github.com/silverstripe/silverstripe-framework
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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().
This commit is contained in:
Ingo Schommer 2018-07-18 14:24:27 +12:00
parent b83281fa5e
commit 93b0884e19
2 changed files with 59 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

77
src/Control/Session.php
View File

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

9
src/Security/MemberAuthenticator/SessionAuthenticationHandler.php
View File

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