summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlex Pott2014-09-09 19:03:13 (GMT)
committerAlex Pott2014-09-09 19:03:13 (GMT)
commit0a12d85e1c2d46ae6d4f9f1048b4f52faa566b40 (patch)
tree3c54f3eda0a81734c2673e7b3977cb6004c51955
parente32a11e76bbe8be639987e331fb8c7e647e0a5e7 (diff)
Issue #2256257 by znerol, cosmicdreams | YesCT: Move token seed in SessionManager in isSessionObsolete() and regenerate() to the MetadataBag.
-rw-r--r--core/core.services.yml4
-rw-r--r--core/lib/Drupal/Core/Access/CsrfTokenGenerator.php27
-rw-r--r--core/lib/Drupal/Core/Batch/BatchStorage.php43
-rw-r--r--core/lib/Drupal/Core/Session/MetadataBag.php34
-rw-r--r--core/lib/Drupal/Core/Session/SessionManager.php25
-rw-r--r--core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php74
6 files changed, 161 insertions, 46 deletions
diff --git a/core/core.services.yml b/core/core.services.yml
index e5d19b1..f8c428f 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -659,7 +659,7 @@ services:
arguments: ['@state']
csrf_token:
class: Drupal\Core\Access\CsrfTokenGenerator
- arguments: ['@private_key']
+ arguments: ['@private_key', '@session_manager.metadata_bag']
access_arguments_resolver:
class: Drupal\Core\Access\AccessArgumentsResolver
access_manager:
@@ -823,7 +823,7 @@ services:
arguments: ['@module_handler', '@cache.discovery', '@language_manager']
batch.storage:
class: Drupal\Core\Batch\BatchStorage
- arguments: ['@database']
+ arguments: ['@database', '@session_manager', '@csrf_token']
tags:
- { name: backend_overridable }
replica_database_ignore__subscriber:
diff --git a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
index 7d38ba0..083dd6c 100644
--- a/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -9,7 +9,7 @@ namespace Drupal\Core\Access;
use Drupal\Component\Utility\Crypt;
use Drupal\Core\PrivateKey;
-use Drupal\Core\Session\AccountInterface;
+use Drupal\Core\Session\MetadataBag;
use Drupal\Core\Site\Settings;
/**
@@ -27,13 +27,23 @@ class CsrfTokenGenerator {
protected $privateKey;
/**
+ * The session metadata bag.
+ *
+ * @var \Drupal\Core\Session\MetadataBag
+ */
+ protected $sessionMetadata;
+
+ /**
* Constructs the token generator.
*
* @param \Drupal\Core\PrivateKey $private_key
* The private key service.
+ * @param \Drupal\Core\Session\MetadataBag $session_metadata
+ * The session metadata bag.
*/
- public function __construct(PrivateKey $private_key) {
+ public function __construct(PrivateKey $private_key, MetadataBag $session_metadata) {
$this->privateKey = $private_key;
+ $this->sessionMetadata = $session_metadata;
}
/**
@@ -56,11 +66,13 @@ class CsrfTokenGenerator {
* @see \Drupal\Core\Session\SessionManager::start()
*/
public function get($value = '') {
- if (empty($_SESSION['csrf_token_seed'])) {
- $_SESSION['csrf_token_seed'] = Crypt::randomBytesBase64();
+ $seed = $this->sessionMetadata->getCsrfTokenSeed();
+ if (empty($seed)) {
+ $seed = Crypt::randomBytesBase64();
+ $this->sessionMetadata->setCsrfTokenSeed($seed);
}
- return $this->computeToken($_SESSION['csrf_token_seed'], $value);
+ return $this->computeToken($seed, $value);
}
/**
@@ -75,11 +87,12 @@ class CsrfTokenGenerator {
* TRUE for a valid token, FALSE for an invalid token.
*/
public function validate($token, $value = '') {
- if (empty($_SESSION['csrf_token_seed'])) {
+ $seed = $this->sessionMetadata->getCsrfTokenSeed();
+ if (empty($seed)) {
return FALSE;
}
- return $token === $this->computeToken($_SESSION['csrf_token_seed'], $value);
+ return $token === $this->computeToken($seed, $value);
}
/**
diff --git a/core/lib/Drupal/Core/Batch/BatchStorage.php b/core/lib/Drupal/Core/Batch/BatchStorage.php
index 851fe00..da0fe94 100644
--- a/core/lib/Drupal/Core/Batch/BatchStorage.php
+++ b/core/lib/Drupal/Core/Batch/BatchStorage.php
@@ -8,25 +8,57 @@
namespace Drupal\Core\Batch;
use Drupal\Core\Database\Connection;
+use Drupal\Core\Session\SessionManager;
+use Drupal\Core\Access\CsrfTokenGenerator;
class BatchStorage implements BatchStorageInterface {
/**
+ * The database connection.
+ *
* @var \Drupal\Core\Database\Connection
*/
protected $connection;
- public function __construct(Connection $connection) {
+ /**
+ * The session manager.
+ *
+ * @var \Drupal\Core\Session\SessionManager
+ */
+ protected $sessionManager;
+
+ /**
+ * The CSRF token generator.
+ *
+ * @var \Drupal\Core\Access\CsrfTokenGenerator
+ */
+ protected $csrfToken;
+
+ /**
+ * Constructs the database batch storage service.
+ *
+ * @param \Drupal\Core\Database\Connection $connection
+ * The database connection.
+ * @param \Drupal\Core\Session\SessionManager $session_manager
+ * The session manager.
+ * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token
+ * The CSRF token generator.
+ */
+ public function __construct(Connection $connection, SessionManager $session_manager, CsrfTokenGenerator $csrf_token) {
$this->connection = $connection;
+ $this->sessionManager = $session_manager;
+ $this->csrfToken = $csrf_token;
}
/**
* {@inheritdoc}
*/
public function load($id) {
+ // Ensure that a session is started before using the CSRF token generator.
+ $this->sessionManager->start();
$batch = $this->connection->query("SELECT batch FROM {batch} WHERE bid = :bid AND token = :token", array(
':bid' => $id,
- ':token' => \Drupal::csrfToken()->get($id),
+ ':token' => $this->csrfToken->get($id),
))->fetchField();
if ($batch) {
return unserialize($batch);
@@ -66,14 +98,17 @@ class BatchStorage implements BatchStorageInterface {
/**
* {@inheritdoc}
*/
- function create(array $batch) {
+ public function create(array $batch) {
+ // Ensure that a session is started before using the CSRF token generator.
+ $this->sessionManager->start();
$this->connection->insert('batch')
->fields(array(
'bid' => $batch['id'],
'timestamp' => REQUEST_TIME,
- 'token' => \Drupal::csrfToken()->get($batch['id']),
+ 'token' => $this->csrfToken->get($batch['id']),
'batch' => serialize($batch),
))
->execute();
}
+
}
diff --git a/core/lib/Drupal/Core/Session/MetadataBag.php b/core/lib/Drupal/Core/Session/MetadataBag.php
index 164062b..4b44739 100644
--- a/core/lib/Drupal/Core/Session/MetadataBag.php
+++ b/core/lib/Drupal/Core/Session/MetadataBag.php
@@ -16,6 +16,11 @@ use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag as SymfonyMetad
class MetadataBag extends SymfonyMetadataBag {
/**
+ * The key used to store the CSRF token seed in the session.
+ */
+ const CSRF_TOKEN_SEED = 's';
+
+ /**
* Constructs a new metadata bag instance.
*
* @param \Drupal\Core\Site\Settings $settings
@@ -26,4 +31,33 @@ class MetadataBag extends SymfonyMetadataBag {
parent::__construct('_sf2_meta', $update_threshold);
}
+ /**
+ * Set the CSRF token seed.
+ *
+ * @param string $csrf_token_seed
+ * The per-session CSRF token seed.
+ */
+ public function setCsrfTokenSeed($csrf_token_seed) {
+ $this->meta[static::CSRF_TOKEN_SEED] = $csrf_token_seed;
+ }
+
+ /**
+ * Get the CSRF token seed.
+ *
+ * @return string|null
+ * The per-session CSRF token seed or null when no value is set.
+ */
+ public function getCsrfTokenSeed() {
+ if (isset($this->meta[static::CSRF_TOKEN_SEED])) {
+ return $this->meta[static::CSRF_TOKEN_SEED];
+ }
+ }
+
+ /**
+ * Clear the CSRF token seed.
+ */
+ public function clearCsrfTokenSeed() {
+ unset($this->meta[static::CSRF_TOKEN_SEED]);
+ }
+
}
diff --git a/core/lib/Drupal/Core/Session/SessionManager.php b/core/lib/Drupal/Core/Session/SessionManager.php
index df6cdee..31989e6 100644
--- a/core/lib/Drupal/Core/Session/SessionManager.php
+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -14,7 +14,6 @@ use Drupal\Core\Session\SessionHandler;
use Drupal\Core\Site\Settings;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler;
-use Symfony\Component\HttpFoundation\Session\Storage\MetadataBag as SymfonyMetadataBag;
use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
/**
@@ -83,14 +82,13 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
* The request stack.
* @param \Drupal\Core\Database\Connection $connection
* The database connection.
- * @param \Symfony\Component\HttpFoundation\Session\Storage\MetadataBag $metadata_bag
+ * @param \Drupal\Core\Session\MetadataBag $metadata_bag
* The session metadata bag.
* @param \Drupal\Core\Site\Settings $settings
* The settings instance.
*/
- public function __construct(RequestStack $request_stack, Connection $connection, SymfonyMetadataBag $metadata_bag, Settings $settings) {
+ public function __construct(RequestStack $request_stack, Connection $connection, MetadataBag $metadata_bag, Settings $settings) {
$options = array();
-
$this->requestStack = $request_stack;
$this->connection = $connection;
@@ -261,12 +259,7 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
}
session_id(Crypt::randomBytesBase64());
- // @todo The token seed can be moved onto \Drupal\Core\Session\MetadataBag.
- // The session manager then needs to notify the metadata bag when the
- // token should be regenerated. https://drupal.org/node/2256257
- if (!empty($_SESSION)) {
- unset($_SESSION['csrf_token_seed']);
- }
+ $this->getMetadataBag()->clearCsrfTokenSeed();
if (isset($old_session_id)) {
$params = session_get_cookie_params();
@@ -405,18 +398,6 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
// Ignore the metadata bag, it does not contain any user data.
$mask[$this->metadataBag->getStorageKey()] = FALSE;
- // Ignore the CSRF token seed.
- //
- // @todo Anonymous users should not get a CSRF token at any time, or if they
- // do, then the originating code is responsible for cleaning up the
- // session once obsolete. Since that is not guaranteed to be the case,
- // this check force-ignores the CSRF token, so as to avoid performance
- // regressions.
- // The token seed can be moved onto \Drupal\Core\Session\MetadataBag. This
- // will result in the CSRF token being ignored automatically.
- // https://drupal.org/node/2256257
- $mask['csrf_token_seed'] = FALSE;
-
// Ignore attribute bags when they do not contain any data.
foreach ($this->bags as $bag) {
$key = $bag->getStorageKey();
diff --git a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
index 1f3a42e..ec2abec 100644
--- a/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
@@ -17,6 +17,7 @@ use Symfony\Component\HttpFoundation\Request;
* Tests the CsrfTokenGenerator class.
*
* @group Access
+ * @coversDefaultClass \Drupal\Core\Access\CsrfTokenGenerator
*/
class CsrfTokenGeneratorTest extends UnitTestCase {
@@ -35,20 +36,26 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
protected $privateKey;
/**
+ * The mock session metadata bag.
+ *
+ * @var \Drupal\Core\Session\MetadataBag|\PHPUnit_Framework_MockObject_MockObject
+ */
+ protected $sessionMetadata;
+
+ /**
* {@inheritdoc}
*/
protected function setUp() {
parent::setUp();
- $this->key = Crypt::randomBytesBase64(55);
$this->privateKey = $this->getMockBuilder('Drupal\Core\PrivateKey')
->disableOriginalConstructor()
->setMethods(array('get'))
->getMock();
- $this->privateKey->expects($this->any())
- ->method('get')
- ->will($this->returnValue($this->key));
+ $this->sessionMetadata = $this->getMockBuilder('Drupal\Core\Session\MetadataBag')
+ ->disableOriginalConstructor()
+ ->getMock();
$settings = array(
'hash_salt' => $this->randomMachineName(),
@@ -56,27 +63,71 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
new Settings($settings);
- $this->generator = new CsrfTokenGenerator($this->privateKey);
+ $this->generator = new CsrfTokenGenerator($this->privateKey, $this->sessionMetadata);
+ }
+
+ /**
+ * Set up default expectations on the mocks.
+ */
+ protected function setupDefaultExpectations() {
+ $key = Crypt::randomBytesBase64();
+ $this->privateKey->expects($this->any())
+ ->method('get')
+ ->will($this->returnValue($key));
+
+ $seed = Crypt::randomBytesBase64();
+ $this->sessionMetadata->expects($this->any())
+ ->method('getCsrfTokenSeed')
+ ->will($this->returnValue($seed));
}
/**
* Tests CsrfTokenGenerator::get().
+ *
+ * @covers ::get
*/
public function testGet() {
+ $this->setupDefaultExpectations();
+
$this->assertInternalType('string', $this->generator->get());
$this->assertNotSame($this->generator->get(), $this->generator->get($this->randomMachineName()));
$this->assertNotSame($this->generator->get($this->randomMachineName()), $this->generator->get($this->randomMachineName()));
}
/**
+ * Tests that a new token seed is generated upon first use.
+ *
+ * @covers ::get
+ */
+ public function testGenerateSeedOnGet() {
+ $key = Crypt::randomBytesBase64();
+ $this->privateKey->expects($this->any())
+ ->method('get')
+ ->will($this->returnValue($key));
+
+ $this->sessionMetadata->expects($this->once())
+ ->method('getCsrfTokenSeed')
+ ->will($this->returnValue(NULL));
+
+ $this->sessionMetadata->expects($this->once())
+ ->method('setCsrfTokenSeed')
+ ->with($this->isType('string'));
+
+ $this->assertInternalType('string', $this->generator->get());
+ }
+
+ /**
* Tests CsrfTokenGenerator::validate().
+ *
+ * @covers ::validate
*/
public function testValidate() {
+ $this->setupDefaultExpectations();
+
$token = $this->generator->get();
$this->assertTrue($this->generator->validate($token));
$this->assertFalse($this->generator->validate($token, 'foo'));
-
$token = $this->generator->get('bar');
$this->assertTrue($this->generator->validate($token, 'bar'));
}
@@ -89,11 +140,11 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
* @param mixed $value
* (optional) An additional value to base the token on.
*
+ * @covers ::validate
* @dataProvider providerTestValidateParameterTypes
*/
public function testValidateParameterTypes($token, $value) {
- // Ensure that there is a valid token seed on the session.
- $this->generator->get();
+ $this->setupDefaultExpectations();
// The following check might throw PHP fatals and notices, so we disable
// error assertions.
@@ -124,12 +175,12 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
* @param mixed $value
* (optional) An additional value to base the token on.
*
+ * @covers ::validate
* @dataProvider providerTestInvalidParameterTypes
* @expectedException InvalidArgumentException
*/
public function testInvalidParameterTypes($token, $value = '') {
- // Ensure that there is a valid token seed on the session.
- $this->generator->get();
+ $this->setupDefaultExpectations();
$this->generator->validate($token, $value);
}
@@ -152,12 +203,13 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
/**
* Tests the exception thrown when no 'hash_salt' is provided in settings.
*
+ * @covers ::get
* @expectedException \RuntimeException
*/
public function testGetWithNoHashSalt() {
// Update settings with no hash salt.
new Settings(array());
- $generator = new CsrfTokenGenerator($this->privateKey);
+ $generator = new CsrfTokenGenerator($this->privateKey, $this->sessionMetadata);
$generator->get();
}