Commit 6faa6861 authored by alexpott's avatar alexpott

Issue #2338727 by znerol, almaudoh: Replace static SessionManager::$enabled...

Issue #2338727 by znerol, almaudoh: Replace static SessionManager::$enabled property with WriteSafeSessionHandler class and resolve hidden circular dependency between SessionManager and SessionHandler
parent 42083a1c
......@@ -19,13 +19,6 @@
*/
class SessionHandler extends AbstractProxy implements \SessionHandlerInterface {
/**
* The session manager.
*
* @var \Drupal\Core\Session\SessionManagerInterface
*/
protected $sessionManager;
/**
* The request stack.
*
......@@ -50,15 +43,12 @@ class SessionHandler extends AbstractProxy implements \SessionHandlerInterface {
/**
* Constructs a new SessionHandler instance.
*
* @param \Drupal\Core\Session\SessionManagerInterface $session_manager
* The session manager.
* @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
* The request stack.
* @param \Drupal\Core\Database\Connection $connection
* The database connection.
*/
public function __construct(SessionManagerInterface $session_manager, RequestStack $request_stack, Connection $connection) {
$this->sessionManager = $session_manager;
public function __construct(RequestStack $request_stack, Connection $connection) {
$this->requestStack = $request_stack;
$this->connection = $connection;
}
......@@ -123,12 +113,6 @@ public function write($sid, $value) {
// The exception handler is not active at this point, so we need to do it
// manually.
try {
if (!$this->sessionManager->isEnabled()) {
// We don't have anything to do if we are not allowed to save the
// session.
return TRUE;
}
$fields = array(
'uid' => $user->id(),
'hostname' => $this->requestStack->getCurrentRequest()->getClientIP(),
......@@ -173,10 +157,6 @@ public function close() {
public function destroy($sid) {
global $user;
// Nothing to do if we are not allowed to change the session.
if (!$this->sessionManager->isEnabled()) {
return TRUE;
}
// Delete session data.
$this->connection->delete('sessions')
->condition('sid', Crypt::hashBase64($sid))
......
......@@ -9,8 +9,6 @@
use Drupal\Component\Utility\Crypt;
use Drupal\Core\Database\Connection;
use Drupal\Core\Session\AnonymousUserSession;
use Drupal\Core\Session\SessionHandler;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler;
use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
......@@ -63,15 +61,16 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
protected $startedLazy;
/**
* Whether session management is enabled or temporarily disabled.
* The write safe session handler.
*
* PHP session ID, session, and cookie handling happens in the global scope.
* This value has to persist, since a potentially wrong or disallowed session
* would be written otherwise.
* @todo: The write safe session handler should be exposed in the
* container and this reference should be removed once all database queries
* are removed from the session manager class.
* @see https://www.drupal.org/node/2372389
*
* @var bool
* @var \Drupal\Core\Session\WriteSafeSessionHandlerInterface
*/
protected static $enabled = TRUE;
protected $writeSafeHandler;
/**
* Constructs a new session manager instance.
......@@ -93,11 +92,11 @@ public function __construct(RequestStack $request_stack, Connection $connection,
// Register the default session handler.
// @todo Extract session storage from session handler into a service.
$save_handler = new SessionHandler($this, $this->requestStack, $this->connection);
$save_handler = new SessionHandler($this->requestStack, $this->connection);
$write_check_handler = new WriteCheckSessionHandler($save_handler);
$this->setSaveHandler($write_check_handler);
$this->writeSafeHandler = new WriteSafeSessionHandler($write_check_handler);
parent::__construct($options, $write_check_handler, $metadata_bag);
parent::__construct($options, $this->writeSafeHandler, $metadata_bag);
// @todo When not using the Symfony Session object, the list of bags in the
// NativeSessionStorage will remain uninitialized. This will lead to
......@@ -165,7 +164,7 @@ public function start() {
* TRUE if the session is started.
*/
protected function startNow() {
if (!$this->isEnabled() || $this->isCli()) {
if ($this->isCli()) {
return FALSE;
}
......@@ -190,7 +189,7 @@ protected function startNow() {
public function save() {
global $user;
if (!$this->isEnabled() || $this->isCli()) {
if ($this->isCli()) {
// We don't have anything to do if we are not allowed to save the session.
return;
}
......@@ -222,7 +221,7 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
global $user;
// Nothing to do if we are not allowed to change the session.
if (!$this->isEnabled() || $this->isCli()) {
if ($this->isCli()) {
return;
}
......@@ -262,7 +261,7 @@ public function regenerate($destroy = FALSE, $lifetime = NULL) {
*/
public function delete($uid) {
// Nothing to do if we are not allowed to change the session.
if (!$this->isEnabled() || $this->isCli()) {
if (!$this->writeSafeHandler->isSessionWritable() || $this->isCli()) {
return;
}
$this->connection->delete('sessions')
......@@ -274,14 +273,14 @@ public function delete($uid) {
* {@inheritdoc}
*/
public function isEnabled() {
return static::$enabled;
return $this->writeSafeHandler->isSessionWritable();
}
/**
* {@inheritdoc}
*/
public function disable() {
static::$enabled = FALSE;
$this->writeSafeHandler->setSessionWritable(FALSE);
return $this;
}
......@@ -289,7 +288,7 @@ public function disable() {
* {@inheritdoc}
*/
public function enable() {
static::$enabled = TRUE;
$this->writeSafeHandler->setSessionWritable(TRUE);
return $this;
}
......
<?php
/**
* @file
* Contains \Drupal\Core\Session\WriteSafeSessionHandler.
*/
namespace Drupal\Core\Session;
/**
* Wraps another SessionHandlerInterface to prevent writes when not allowed.
*/
class WriteSafeSessionHandler implements \SessionHandlerInterface, WriteSafeSessionHandlerInterface {
/**
* @var \SessionHandlerInterface
*/
protected $wrappedSessionHandler;
/**
* Whether or not the session is enabled for writing.
*
* @var bool
*/
protected $sessionWritable;
/**
* Constructs a new write safe session handler.
*
* @param \SessionHandlerInterface $wrapped_session_handler
* The underlying session handler.
* @param bool $session_writable
* Whether or not the session should be initially writable.
*/
public function __construct(\SessionHandlerInterface $wrapped_session_handler, $session_writable = TRUE) {
$this->wrappedSessionHandler = $wrapped_session_handler;
$this->sessionWritable = $session_writable;
}
/**
* {@inheritdoc}
*/
public function close() {
return $this->wrappedSessionHandler->close();
}
/**
* {@inheritdoc}
*/
public function destroy($session_id) {
return $this->wrappedSessionHandler->destroy($session_id);
}
/**
* {@inheritdoc}
*/
public function gc($max_lifetime) {
return $this->wrappedSessionHandler->gc($max_lifetime);
}
/**
* {@inheritdoc}
*/
public function open($save_path, $session_id) {
return $this->wrappedSessionHandler->open($save_path, $session_id);
}
/**
* {@inheritdoc}
*/
public function read($session_id) {
return $this->wrappedSessionHandler->read($session_id);
}
/**
* {@inheritdoc}
*/
public function write($session_id, $session_data) {
if ($this->isSessionWritable()) {
return $this->wrappedSessionHandler->write($session_id, $session_data);
}
else {
return TRUE;
}
}
/**
* {@inheritdoc}
*/
public function setSessionWritable($flag) {
$this->sessionWritable = (bool) $flag;
}
/**
* {@inheritdoc}
*/
public function isSessionWritable() {
return $this->sessionWritable;
}
}
<?php
/**
* @file
* Contains \Drupal\Core\Session\WriteSafeSessionHandlerInterface.
*/
namespace Drupal\Core\Session;
/**
* Provides an interface for session handlers where writing can be disabled.
*/
interface WriteSafeSessionHandlerInterface {
/**
* Sets whether or not a session may be written to storage.
*
* It is not possible to enforce writing of the session data. This method is
* only capable of forcibly disabling that session data is written to storage.
*
* @param bool $flag
* TRUE if the session the session is allowed to be written, FALSE
* otherwise.
*/
public function setSessionWritable($flag);
/**
* Returns whether or not a session may be written to storage.
*
* @return bool
* TRUE if the session the session is allowed to be written, FALSE
* otherwise.
*/
public function isSessionWritable();
}
<?php
/**
* @file
* Contains \Drupal\Tests\Core\Session\WriteSafeSessionHandlerTest.
*/
namespace Drupal\Tests\Core\Session;
use Drupal\Tests\UnitTestCase;
use Drupal\Core\Session\WriteSafeSessionHandler;
/**
* Tests \Drupal\Core\Session\WriteSafeSessionHandler.
*
* @coversDefaultClass \Drupal\Core\Session\WriteSafeSessionHandler
* @group Session
*/
class WriteSafeSessionHandlerTest extends UnitTestCase {
/**
* The wrapped session handler.
*
* @var \SessionHandlerInterface|\PHPUnit_Framework_MockObject_MockObject
*/
protected $wrappedSessionHandler;
/**
* The write safe session handler.
*
* @var \Drupal\Core\Session\WriteSafeSessionHandler
*/
protected $sessionHandler;
public function setUp() {
$this->wrappedSessionHandler = $this->getMock('SessionHandlerInterface');
$this->sessionHandler = new WriteSafeSessionHandler($this->wrappedSessionHandler);
}
/**
* Tests creating a WriteSafeSessionHandler with default arguments.
*
* @covers ::__construct
* @covers ::isSessionWritable
* @covers ::write
*/
public function testConstructWriteSafeSessionHandlerDefaultArgs() {
$session_id = 'some-id';
$session_data = 'serialized-session-data';
$this->assertSame($this->sessionHandler->isSessionWritable(), TRUE);
// Writing should be enabled, return value passed to the caller by default.
$this->wrappedSessionHandler->expects($this->at(0))
->method('write')
->with($session_id, $session_data)
->will($this->returnValue(TRUE));
$this->wrappedSessionHandler->expects($this->at(1))
->method('write')
->with($session_id, $session_data)
->will($this->returnValue(FALSE));
$result = $this->sessionHandler->write($session_id, $session_data);
$this->assertSame($result, TRUE);
$result = $this->sessionHandler->write($session_id, $session_data);
$this->assertSame($result, FALSE);
}
/**
* Tests creating a WriteSafeSessionHandler with session writing disabled.
*
* @covers ::__construct
* @covers ::isSessionWritable
* @covers ::write
*/
public function testConstructWriteSafeSessionHandlerDisableWriting() {
$session_id = 'some-id';
$session_data = 'serialized-session-data';
// Disable writing upon construction.
$this->sessionHandler = new WriteSafeSessionHandler($this->wrappedSessionHandler, FALSE);
$this->assertSame($this->sessionHandler->isSessionWritable(), FALSE);
$result = $this->sessionHandler->write($session_id, $session_data);
$this->assertSame($result, TRUE);
}
/**
* Tests using setSessionWritable to enable/disable session writing.
*
* @covers ::setSessionWritable
* @covers ::write
*/
public function testSetSessionWritable() {
$session_id = 'some-id';
$session_data = 'serialized-session-data';
$this->assertSame($this->sessionHandler->isSessionWritable(), TRUE);
// Disable writing after construction.
$this->sessionHandler->setSessionWritable(FALSE);
$this->assertSame($this->sessionHandler->isSessionWritable(), FALSE);
$this->sessionHandler = new WriteSafeSessionHandler($this->wrappedSessionHandler, FALSE);
$this->assertSame($this->sessionHandler->isSessionWritable(), FALSE);
$result = $this->sessionHandler->write($session_id, $session_data);
$this->assertSame($result, TRUE);
// Enable writing again.
$this->sessionHandler->setSessionWritable(TRUE);
$this->assertSame($this->sessionHandler->isSessionWritable(), TRUE);
// Writing should be enabled, return value passed to the caller by default.
$this->wrappedSessionHandler->expects($this->at(0))
->method('write')
->with($session_id, $session_data)
->will($this->returnValue(TRUE));
$this->wrappedSessionHandler->expects($this->at(1))
->method('write')
->with($session_id, $session_data)
->will($this->returnValue(FALSE));
$result = $this->sessionHandler->write($session_id, $session_data);
$this->assertSame($result, TRUE);
$result = $this->sessionHandler->write($session_id, $session_data);
$this->assertSame($result, FALSE);
}
/**
* Tests that other invocations are passed unmodified to the wrapped handler.
*
* @covers ::setSessionWritable
* @covers ::open
* @covers ::read
* @covers ::close
* @covers ::destroy
* @covers ::gc
* @dataProvider providerTestOtherMethods
*/
public function testOtherMethods($method, $expected_result, $args) {
$invocation = $this->wrappedSessionHandler->expects($this->exactly(2))
->method($method)
->will($this->returnValue($expected_result));
// Set the parameter matcher.
call_user_func_array([$invocation, 'with'], $args);
// Test with writable session.
$this->assertSame($this->sessionHandler->isSessionWritable(), TRUE);
$actual_result = call_user_func_array([$this->sessionHandler, $method], $args);
$this->assertSame($expected_result, $actual_result);
// Test with non-writable session.
$this->sessionHandler->setSessionWritable(FALSE);
$this->assertSame($this->sessionHandler->isSessionWritable(), FALSE);
$actual_result = call_user_func_array([$this->sessionHandler, $method], $args);
$this->assertSame($expected_result, $actual_result);
}
/**
* Provides test data for the other methods test.
*
* @return array
* Test data.
*/
public function providerTestOtherMethods() {
return [
['open', TRUE, ['/some/path', 'some-session-id']],
['read', 'some-session-data', ['a-session-id']],
['close', TRUE, []],
['destroy', TRUE, ['old-session-id']],
['gc', TRUE, [42]],
];
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment