Convert CSRF tokens to a service

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+    global $user;

Let's inject the request into the service.

This is the issue I was looking for (based on #2042487: Token checking for Views enable/disable is missing)! This should fix the unit tests. At some point we need to do something with drupal_get_hash_salt() maybe?

Sorry, that was two interdiffs.

At some point we need to do something with drupal_get_hash_salt() maybe?

as a first step #2036259: Move $drupal_hash_salt to settings()

Yes, something pretty much like that :)

+++ b/core/includes/common.incundefined
@@ -3027,13 +3027,12 @@ function drupal_json_decode($var) {
+ * @deprecated

@@ -3048,9 +3047,12 @@ function drupal_get_private_key() {
+ * @deprecated

@@ -3066,10 +3068,12 @@ function drupal_get_token($value = '') {
+ * @deprecated

Afaik this depcreated should be more like "@ deprecated as of 8.0" or something like that.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+ * Contains Drupal\Core\Access\CsrfTokenManager.

Missing "\"

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+   * @return

I guess the private key is always a string?

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+    $key = $key = $this->state->get('system.private_key');

$key equals $key equals $key

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+   * @return \Drupal\Core\Access\CsrfTokenManager

I like the @return self, as it has a slightly better semantic.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+    return Crypt::hmacBase64($value, session_id() . $this->getPrivateKey() . drupal_get_hash_salt());

OT: It feels wrong to hash the database connection, wouldn't the hash change in different enviroments?

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+   * @param $token
+   *   The token to be validated.
+   * @param $value
+   *   An additional value to base the token on.
+   * @param $skip_anonymous
...
+   * @return

needs typehints.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,110 @@
+    global $user;
...
+    return (($skip_anonymous && $user->id() == 0) || ($token == $this->getToken($value)));

global $user can be replaced by the request object now.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,102 @@
+      'name' => '\Drupal\Tests\Core\Access\CsrfTokenManager',

Nice name! I agree but you need a description as well.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,102 @@
+    $this->manager = new CsrfTokenManager($state);

Needs docs.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,102 @@
+  public function testSetPrivateKey() {
+    $this->assertInstanceOf('\Drupal\Core\Access\CsrfTokenManager', $this->manager->setPrivateKey($this->randomName()));
+  }

This should also ensure that the new set private key can be retrieved.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,102 @@
+namespace {
+  if (!function_exists('drupal_get_hash_salt')) {

Let's better add a @todo

Thanks for the review! I have made those changes, I added a helper method and mocked the state methods in each actual test, seems to be more thorough, rather than just using any() for all of them.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -24,23 +25,30 @@ class CsrfTokenManager {
+  function __construct(KeyValueStoreInterface $state, Request $request) {
...
+    $this->request = $request;

There is no need to inject the request object into that class, but we could use the approach of LanguageManager which is some magic provided by symfony 2.3 http://symfony.com/blog/new-in-symfony-2-3-what-else

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -89,10 +100,27 @@ public function testValidateToken() {
+  }
 }

Missing empty line.

How about this?

#15: 2036351-15.patch queued for re-testing.

Related: #1798296: Integrate CSRF link token directly into routing system

Ohh, we need to set the request in a subscriber?!

Patch based on #15 which works at least on the comments.

I didn't know about that. That's much better!

+++ b/core/lib/Drupal.phpundefined
@@ -391,4 +391,13 @@ public static function languageManager() {
+   * Retrieves the CRF token manager service.
+   *
+   * @return \Drupal\Core\Access\CsrfTokenManager
+   */

@return \Drupal\Core\Access\CsrfTokenManager
"The CSRF token manager."

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,129 @@
+<?php
+/**

Missing empty line.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,129 @@
+ * @see \Drupal\Tests\Core\Access\CsrfTokenManagerTest

Since when do we do @see for tests? I quite like that though.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,129 @@
+   *   An additional value to base the token on.

Missing "(Optional)"

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,129 @@
+   *   A 43-character URL-safe token for validation, based on the user session ID,

80 characters.

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,129 @@
+   *   the hash salt provided from drupal_get_hash_salt(), and the

Not sure (not a native speaker) but isn't "provided by" better than "provided from"?

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,129 @@
+   *   An additional value to base the token on.
...
+   *   Set to TRUE to skip token validation for anonymous users.

Missing "(Optional)"

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,129 @@
+    return (($skip_anonymous && $user->id() == 0) || ($token == $this->getToken($value)));

Redundant (()).

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,132 @@
+  /**
+ * Tests \Drupal\Core\Access\CsrfTokenManager.
+ */

Whoops.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,132 @@
+    $this->state = $this->getMockBuilder('Drupal\Core\KeyValueStore\KeyValueStoreInterface')
+      ->disableOriginalConstructor()
+      ->getMock();

Interface mock with "disableOriginalConstructor()"?

Great, thank fubhy! Here are those changes.

#26: 2036351-26.patch queued for re-testing.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,131 @@
+
+  /**
+ * Tests the CSRF token manager..
+ */

Still *whoops* :) (2 periods, leading forward-slash one white-space off).

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,131 @@
+    $this->state = $this->getMockBuilder('Drupal\Core\KeyValueStore\KeyValueStoreInterface')
+      ->getMock();

Why not just "getMock()" directly then? ;)

Looking good otherwise.

Totally right :)

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenManagerTest.phpundefined
@@ -0,0 +1,130 @@
+
+  /**
+ * Tests the CSRF token manager.

Still, the whitespace :/

Should be

+ /**
+ * Tests the CSRF token manager.

and not

+  /**
+ * Tests the CSRF token manager.

BAHHHH.

Thanks

+++ b/core/core.services.ymlundefined
@@ -365,6 +365,11 @@ services:
+      - [setRequest, ['@?request']]

So this will be only fired if the request service exists?

+++ b/core/includes/common.incundefined
@@ -3048,28 +3048,34 @@ function drupal_get_private_key() {
+  return \Drupal::csrfToken()->validateToken($token, $value, $skip_anonymous);

Just validate should be enough, no? The class/getter/servicename is already token...

+++ b/core/lib/Drupal/Core/Access/CsrfTokenManager.phpundefined
@@ -0,0 +1,130 @@
+  public function createPrivateKey() {

This is wrong to be public. This should not be called from the outside. I am not even sure it's worth a method.

Ok, I've changed the methods to get() and validate(). If we're changing from validateToken, we should also change from getToken. I have made createPrivateKey to private.

Yes, the '@?service'syntax is for optional dependencies.

@@ -66,38 +66,31 @@ public function testSetPrivateKey() {
    * Tests CsrfTokenManager::getToken().
...
+  public function testGet() {

You missed to rename it in the docs as well.

Hasn't mark posted his idea to not name it manager but CrsfTokenGenerator instead?

That change sounds reasonable to me.

Great thank you!

+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.phpundefined
@@ -0,0 +1,130 @@
+  /**
+   * Gets the private key.
+   *
+   * @return string
+   *   The private key.
+   */
+  public function getPrivateKey() {
+    if (!$key = $this->state->get('system.private_key')) {
+      $key = $this->createPrivateKey();
+      $this->setPrivateKey($key);
+    }
+
+    return $key;
+  }

The private key is used for other things than CRSF protection - shouldn't it go into its own service? i.e. update module uses it as well.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.phpundefined
@@ -0,0 +1,123 @@
+  /**
+   * Adds mocked get and set methods for the state mock object.
+   */
+  protected function mockAllStateMethods() {
+    $this->state->expects($this->any())
+      ->method('set')
+      ->with('system.private_key')
+      ->will($this->returnValue(TRUE));
+    $this->state->expects($this->any())
+      ->method('get')
+      ->with('system.private_key')
+      ->will($this->returnValue($this->key));
+  }
+
+}
+
+}

Isn't this a syntax error? How did the patch pass?

That screwed me up too... the missing opening brace is up at:

+namespace Drupal\Tests\Core\Access {

Apparently this is a PHPUnit thing.

Its just a php thing :-) otherwise you can't declare anything outside of that namespace.

So...do you both agree with a private key service?

Just tracking some work, added a private key service etc... but haven't converted/split up the tests yet.

With the tests split up too. This actually makes alot more sense to me. Good call.

@catch, what do you think?

Patch looks much better like this, thanks!

The conditionally defined function makes me wince still, we'll need to bump the other issue to major/critical once this is in.

I really like this additional service.

@@ -3028,21 +3028,21 @@ function drupal_json_decode($var) {
+ *
+ * @deprecated as of Drupal 8.0. Use the private key service instead.

Let's call it 'private_key' here as well, so people know the exact name of the service.

@@ -391,4 +391,24 @@ public static function languageManager() {
+   * @return \Drupal\Core\PrivateKey
+   *   The private key service.

I am wondering whether it is really worth to add this to \Drupal, as this potentially is not used a lot of places.

@@ -0,0 +1,92 @@
+  function __construct(PrivateKey $private_key) {

This function needs a visibility added.

@@ -0,0 +1,81 @@
+    $this->assertNotSame($this->generator->get(), $this->generator->get($this->randomName()));

We could just call it with two different random names to make sure what the expected behavior is.

@@ -0,0 +1,81 @@
+ * @todo Remove this when https://drupal.org/node/2036259 is resolved.
...
+  if (!function_exists('drupal_get_hash_salt')) {

We have done this workaround in a lot places already, so let's not freak out everytime we do that.

@@ -0,0 +1,93 @@
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface

It is possible to typehint both the mock object as well as the actual interface, feel free to do that if you reroll.

@@ -0,0 +1,93 @@
+  function setUp() {

Needs visibility.

@@ -0,0 +1,93 @@
+    $this->state->expects($this->any())

Maybe we could/should use $this->once() ?

Thanks Daniel, you're a good man.

I think I've addressed all your points; removed the Drupal::privateKey() method, as you're right I think, this is not all that common, and Drupal::service('private_key') is not too bad for that :)

Perfect!

Thanks!

Committed/pushed to 8.x.

Could use a change notice.

@@ -0,0 +1,92 @@
+  public function validate($token, $value = '', $skip_anonymous = FALSE) {
+    $user = $this->request->attributes->get('account');

Suppose this one should be _account

There we go. So we actually found a missing test coverage in drupal and solved it.

After commit please reset to "Change notice: Convert CSRF tokens to a service" thanks

makes sense, previously it was not broken because $skip_anonymous always was FALSE so $user->id() is not executed

After commit please reset title to "Change notice: Convert CSRF tokens to a service" thanks

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
@@ -69,8 +69,30 @@ public function testValidate() {
+    // Check the skip_anoymous option with both a anonymous user and a real

anoymous.

Just rerolled with that typo change, I think I'm good to RTBC this again. Great work Daniel.

Let's get this in, so it doesn't hold back anything that wants to use it :)

Committed/pushed to 8.x, thanks!

https://drupal.org/node/2068221

Suppose notice should mention about setRequest() that needed for _account

Let's not mention _account - there's a critical issue to sort that out.