Move token seed in SessionManager in isSessionObsolete() and regenerate() to the MetadataBag

#2265789: Add an interface for CsrfTokenGenerator should help with the installer.

Instead of mocking CsrfTokenGenerator (the initial goal of the issue referenced in#4), it also would be possible to fix the installer/update script with #2272987: Do not persist session manager.

Active again after #2272987: Do not persist session manager. Now that CsrfTokenGenerator stops accessing $_SESSION directly, code trying to use tokens for anonymous users really needs to make sure that there is a session before attempting to generate a token.

Reroll.

RTBC except nits

1. +++ b/core/lib/Drupal/Core/Batch/BatchStorage.php
   @@ -8,25 +8,57 @@
   +   * The Database connection.
   

   s/Database/database

2. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -26,4 +28,33 @@ public function __construct(Settings $settings) {
   +   * Get the CSRF token seed.
   +   *
   +   * @return string
   +   *   The per-session CSRF token seed.
   +   */
   +  public function getCsrfTokenSeed() {
   +    if (isset($this->meta[self::CSRF_TOKEN_SEED])) {
   +      return $this->meta[self::CSRF_TOKEN_SEED];
   +    }
   +  }
   

   string|null
   when no seed stored

Thanks.

nice

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
@@ -35,20 +36,26 @@ class CsrfTokenGeneratorTest extends UnitTestCase {
+   * The mock session metadata bag.
+   *
+   * @var \Drupal\Core\Session\MetadataBag|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $sessionMetadata;
+
+  /**
...
+    $this->sessionMetadata = $this->getMockBuilder('Drupal\Core\Session\MetadataBag')
+      ->disableOriginalConstructor()
+      ->getMock();

@@ -56,27 +63,71 @@ protected function setUp() {
+  /**
+   * 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));

Why are we going through all this trouble to mock the object? Can't we just use an instantiated version of the object?

NM, its because settings isn't test friendly. Opened #2329817: Add a Settings storage object

I guess the retest passed? Going to move this back to RTBC.

patch applies +!

1. +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
   @@ -27,13 +27,21 @@ class CsrfTokenGenerator {
   +  public function __construct(PrivateKey $private_key, MetadataBag $session_metadata) {
   

   Please update Docblock

2. +++ b/core/lib/Drupal/Core/Batch/BatchStorage.php
   @@ -66,14 +98,17 @@ public function cleanup() {
   +
   

   Extra needless whitespace

3. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -26,4 +28,33 @@ public function __construct(Settings $settings) {
   +  /**
   +   * Set the CSRF token seed.
   +   *
   +   * @param string $csrf_token_seed
   +   *   The per-session CSRF token seed.
   +   */
   +  public function setCsrfTokenSeed($csrf_token_seed) {
   +    $this->meta[self::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[self::CSRF_TOKEN_SEED])) {
   +      return $this->meta[self::CSRF_TOKEN_SEED];
   +    }
   +  }
   +
   +  /**
   +   * Clear the CSRF token seed.
   +   */
   +  public function clearCsrfTokenSeed() {
   +    unset($this->meta[self::CSRF_TOKEN_SEED]);
   +  }
   +
   

   if this proves to be important we should consider contributing this work back to Symfony

1. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -15,6 +15,8 @@
   +  const CSRF_TOKEN_SEED = 's';
   

   Er. How is a hard-coded token seed that cannot vary site to site in any way secure?

2. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -26,4 +28,33 @@ public function __construct(Settings $settings) {
   +    if (isset($this->meta[self::CSRF_TOKEN_SEED])) {
   

   Assuming the constant is kept, there's extremely few reasons to use self:: anymore. Use static:: instead, by default.

#16.1: done, thanks, #16.2: This is by intention. It is the single newline separating the last method from the closing brace of the class.

#16.3 Symfony has Symfony\Component\Security\Csrf which provides anything anyone ever could wish including a SessionStorage class. Because we do not have the Session object yet in Drupal we cannot just switch to those classes. We might want to revisit that later but at the moment the top priority for me is to advance the conversion of the session component. This patch is important especially because of the changes in the batch-system.

@Crell

17.1: Look closer, const CSRF_TOKEN_SEED isn't the seed, it's the array key. The value is created elsewhere and presumeably is random enough. Do we need the array key to be randomized as well?

Here's the change from self:: to static::

I spent a good bit of time looking into this code too. It confused me at first but cosmicdreams nailed it.

Other concerns had and my internal resolution.

1. 's' doesn't really have much to do with csrf and 'c' would be better.
   • 'c' is used for cookie. there really isn't a good character so 's' is as good as any other.
2. We don't really use single characters for constant strings like this, CSRF would be better.
   • This isn't done for code effeciency but because this metadata has to be stored in session storage. Smaller concise serialized objects in storage is at least a better reason then key length in code execution.
   • The internal implementation is not exposed other then through the serialized structure so it doesn't really matter.

@neclimdul so RTBC? (I can no longer mark it as RTBC myself)

This issue is a good step forward.

1. +++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
   @@ -56,11 +66,13 @@ public function __construct(PrivateKey $private_key) {
   +    $seed = $this->sessionMetadata->getCsrfTokenSeed();
   +    if (empty($seed)) {
   +      $seed = Crypt::randomBytesBase64();
   +      $this->sessionMetadata->setCsrfTokenSeed($seed);
        }
    
   

   Can't we put that logic into the getCsrfTokenSeed method? So generate it on the fly? But yeah I see the point that the csrf token generator should handle the creation of csrf token seeds

2. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -15,6 +15,8 @@
    
   +  const CSRF_TOKEN_SEED = 's';
   +
   

   Maybe we could document this and tell us, why we use 's' and not 'c' for exmaple

#22.1: We cannot generate the seed in MetadataBag::getCsrfTokenSeed(). Otherwise we'd forcibly open a session even when only calling CsrfTokenGenerator::validate() for anonymous users.

#22.2: Added documentation for the token-seed. The reason we cannot use 'c' is that this is already taken by the parent Symfony MetadataBag for the creation time.

Yep that looks good to go.

This looks really good. One small nit:

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -83,14 +82,13 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
-   * @param \Symfony\Component\HttpFoundation\Session\Storage\MetadataBag $metadata_bag
+   * @param \Drupal\Core\Session\MetadataBag $metadata_bag
...
-  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) {

Let's typehint on the interface SessionBagInterface

That would be incorrect.

The only reference in the constructor is

    parent::__construct($options, $write_check_handler, $metadata_bag);

and the parent sig is:

public function __construct(array $options = array(), $handler = null, MetadataBag $metaBag = null)

More information, those signatures a misleading as they're different bags. Our sig is our MetadataBag and the parent is Symfony's bag. We internally require methods specifically on our MetadataBag though. Specifically the CsrfToken Methods currently. Only matching the interface would not require this.

Discussed with @neclimdul on IRC - I pointed out our metadata bag is service and therefore swappable which means that we perhaps we should be using an interface here - but we agreed that in reality you could just extend from \Drupal\Core\Session\MetadataBag

Committed 0a12d85 and pushed to 8.0.x. Thanks!