Move CSRF header token out of REST module so that user module can use it, as well as any contrib module

+++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
@@ -36,8 +34,12 @@ public function __construct(SessionConfigurationInterface $session_configuration
+    $applicable_requirements = ['_csrf_request_header_token', '_access_rest_csrf'];

We want a @todo to remove the second one in Drupal 9.0.0.

#2:

1. Tricky! I think system module is the only fit, sadly.
2. Sounds great!

This blocks #2403307: RPC endpoints for user authentication: log in, check login status, log out.

+++ b/core/modules/rest/rest.services.yml
@@ -8,11 +8,6 @@ services:
-  access_check.rest.csrf:
-    class: Drupal\rest\Access\CSRFAccessCheck
-    arguments: ['@session_configuration']
-    tags:
-      - { name: access_check }

Can we keep using access_check.rest.csrf to be an alias of the core service? Its removes some BC break

#6++

1. +++ b/core/modules/rest/rest.routing.yml
   @@ -1,6 +1,9 @@
   +# @deprecated This path is deprectated use the system.csrftoken route from the
   

   s/path/route/

   s/deprectated use/deprecated, use/

2. +++ b/core/modules/system/src/Controller/CSRFTokenController.php
   @@ -0,0 +1,51 @@
   +class CSRFTokenController extends ControllerBase {
   

   s/CSRF/Csrf/

3. +++ b/core/modules/system/src/Controller/CSRFTokenController.php
   @@ -0,0 +1,51 @@
   +   * The CSRF token manager.
   

   s/manager/generator/

4. +++ b/core/modules/system/src/Controller/CSRFTokenController.php
   @@ -0,0 +1,51 @@
   +   * Constructs a new CsrfTokenGenerator object.
   

   C/P remnant.

5. +++ b/core/modules/system/src/Controller/CSRFTokenController.php
   @@ -0,0 +1,51 @@
   +    return new Response($this->tokenGenerator->get('header'), 200, array('Content-Type' => 'text/plain'));
   

   s/array()/[]/

6. +++ b/core/modules/system/tests/modules/csrf_test/csrf_test.routing.yml
   @@ -0,0 +1,18 @@
   +# Tests CSRF Header Token protection.
   

   s/CSRF Header Token/CSRF request header token/

7. +++ b/core/modules/system/tests/modules/csrf_test/csrf_test.routing.yml
   @@ -0,0 +1,18 @@
   +# This originally was in the REST module but now is supported in Core/lib.
   

   s/Core/core/

8. +++ b/core/modules/system/tests/modules/csrf_test/csrf_test.routing.yml
   @@ -0,0 +1,18 @@
   +  #methods:  [POST]
   

   ?

9. +++ b/core/modules/system/tests/modules/csrf_test/src/Controller/TestController.php
   @@ -0,0 +1,22 @@
   +    return new Response('Sometimes it is hard to think of test content!');
   

   :P

10. +++ b/core/modules/system/tests/src/Functional/CsrfHeaderTest.php
    @@ -0,0 +1,75 @@
    + * Tests protecting routes by requiring CSRF token in the header.
    

    s/header/request header/

11. +++ b/core/modules/system/tests/src/Functional/CsrfHeaderTest.php
    @@ -0,0 +1,75 @@
    +class CsrfHeaderTest extends BrowserTestBase {
    

    CsrfRequestHeaderTest

12. +++ b/core/modules/system/tests/src/Functional/CsrfHeaderTest.php
    @@ -0,0 +1,75 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    

    {@inheritdoc}

13. +++ b/core/modules/system/tests/src/Functional/CsrfHeaderTest.php
    @@ -0,0 +1,75 @@
    +      // Add cookies to post options so that all other requests are for the
    

    s/post/POST/

14. +++ b/core/modules/system/tests/src/Functional/CsrfHeaderTest.php
    @@ -0,0 +1,75 @@
    +      $post_options['headers']['X-CSRF-Token'] = 'this-is-not-the-token-you-are-looking-for';
    

    :D

I'm feeling a bit bad about it.

1. +++ b/core/modules/rest/rest.services.yml
   @@ -8,11 +8,9 @@ services:
      access_check.rest.csrf:
   -    class: Drupal\rest\Access\CSRFAccessCheck
   -    arguments: ['@session_configuration']
   -    tags:
   -      - { name: access_check }
   +    alias: access_check.header.csrf
   

   Cool, thank you!

2. +++ b/core/modules/system/tests/src/Functional/CsrfRequestHeaderTest.php
   @@ -0,0 +1,75 @@
   +
   +namespace Drupal\tests\system\Functional;
   

   The namespace should be Drupal\Tests\system\Functional ... aka. this will not run on the testbot. This is kinda of a bummer

3. +++ b/core/modules/system/tests/src/Functional/CsrfRequestHeaderTest.php
   @@ -0,0 +1,75 @@
   +      $post_options['cookies']  = $cookies;
   

   Nitpick of the month: 2 spaces!

+++ b/core/modules/system/src/Controller/CsrfTokenController.php
@@ -0,0 +1,51 @@
+ * Returns responses for CSRF token routes.
+ */
+class CsrfTokenController extends ControllerBase {
+

Let's not extend the controller base as long we don't need it.

1. +++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
   @@ -36,8 +34,13 @@ public function __construct(SessionConfigurationInterface $session_configuration
   +    // @todo Remove _access_rest_csrf in Drupal 9.0.0.
   +    $applicable_requirements = ['_csrf_request_header_token', '_access_rest_csrf'];
   

   SuperNit: I'd format this array across multiple lines, then you can add the @todo comment above the relevant line.

2. +++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
   @@ -77,7 +80,7 @@ public function access(Request $request, AccountInterface $account) {
   -      if (!\Drupal::csrfToken()->validate($csrf_token, 'rest')) {
   +      if (!\Drupal::csrfToken()->validate($csrf_token, 'header')) {
   

   +1 to not tie this to rest in any way.

   I wonder if 'X-CSRF-Token request header' would be a better value though.

   In any case, this should be moved into a class constant.

3. +++ b/core/modules/system/tests/modules/csrf_test/csrf_test.routing.yml
   @@ -0,0 +1,18 @@
   +# Tests deprecated _access_rest_csrf protection.
   +# This originally was in the REST module but now is supported in core/lib.
   +# @todo Remove this test route in Drupal 9.0.0.
   

   SuperNit: I think this could use an @see.

+++ b/core/core.services.yml
similarity index 78%
rename from core/modules/rest/src/Access/CSRFAccessCheck.php

rename from core/modules/rest/src/Access/CSRFAccessCheck.php
rename to core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php

+++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
@@ -12,7 +10,12 @@
  */
-class CSRFAccessCheck implements AccessCheckInterface {
+class CsrfRequestHeaderAccessCheck implements AccessCheckInterface {

I tried to check the BC policy around those classes and it seems to be not 100% clear at the moment, see https://www.drupal.org/node/2550249#comment-11344307 for a clarification.

+++ b/core/modules/rest/src/Tests/CsrfTest.php
@@ -87,7 +87,7 @@ public function testCookieAuth() {
-    $token = $this->drupalGet('rest/session/token');
+    $token = $this->drupalGet('session/token');

With the new test, why are we changing this? Shouldn't we leave it the same and deprecate the test aligned with the route?

RTBC under the assumption that https://www.drupal.org/node/2550249#comment-11344307 is fine.

Works for me, +1 on RTBC. Thanks guys!

Looks like bot problems. Been fixing a lot of those on RTBC issues this weekend.

Few questions on this one, code itself looks good:

1. +++ b/core/modules/rest/rest.routing.yml
   @@ -1,6 +1,9 @@
   +# @deprecated This route is deprecated, use the system.csrftoken route from the
   

   What does this mean for the actual REST API itself. Should we be adding a 301 here (not necessarily this patch, but in a later minor release?). Or should 9.x keep the path and add the redirect?

2. +++ b/core/modules/rest/rest.services.yml
   @@ -8,11 +8,9 @@ services:
   +    alias: access_check.header.csrf
   

   +1.

3. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -137,16 +137,6 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   -    return new Response(\Drupal::csrfToken()->get('rest'), 200, array('Content-Type' => 'text/plain'));
   

   This was using 'rest' as the key.

4. +++ b/core/modules/system/src/Controller/CsrfTokenController.php
   @@ -0,0 +1,52 @@
   +    return new Response($this->tokenGenerator->get(CsrfRequestHeaderAccessCheck::TOKEN_KEY), 200, ['Content-Type' => 'text/plain']);
   

   Now using the constant, which is not 'rest'. What happens to a rest client if they access the site just as it updates? Won't they fail the CSRF check? If so is there anything we should do about that? Also brings up the question whether the token key should be set as a route option rather than a fixed constant - then REST could still use 'rest' then.

5. +++ b/core/modules/system/system.routing.yml
   @@ -492,3 +492,10 @@ system.entity_autocomplete:
   +system.csrftoken:
   

   Makes me wish we'd split system module up a bit more in 8.x, but for now can't think of anywhere else it could live.

6. +++ b/core/modules/system/tests/modules/csrf_test/src/Controller/TestController.php
   @@ -0,0 +1,22 @@
   +    return new Response('Sometimes it is hard to think of test content!');
   

   :)

1. What does this mean for the actual REST API itself. Should we be adding a 301 here (not necessarily this patch, but in a later minor release?). Or should 9.x keep the path and add the redirect?

   IMO the latter. The former would cause that to become two round trips rather than one.

4. Great point! But AFAICT this is exactly the same as a session having expired? Then your token suddenly is no longer valid either. Furthermore, since this is a minor version bump (8.1.x -> 8.2.x), we'll be terminating sessions anywayapparently we don't terminate sessions upon updating? Should we add an update hook to REST to terminate all sessions?.

If your session has just expired then you'll be logged out, the situation here would be you're logged in but get a CSRF validation error (at least, that's what would happen with a form submission if the token check changed between rendering the form and submitting it, haven't tested it with REST). That's a bit different from getting logged out - you'd be forced to re-authenticate in that case, but not if you just have a wrong token.

Terminating sessions on update would 'fix' this, but not sure if that's acceptable - might be worse than the wrong REST tokens (since that'd affect regular site visitors too).

#33 seems like a good workaround/bc layer, and it's right that'd we'd only need to keep it in for one minor release.

#35 had a random fail, so re-testing, should come back green.

Gah again?! UpdatePathTestBaseFilledTest

+++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
@@ -88,7 +98,10 @@ public function access(Request $request, AccountInterface $account) {
+      // @todo Remove validate call using 'rest' in 8.3.
+      //   Kept here for sessions active during update.
+      if (!$this->csrfToken->validate($csrf_token, self::TOKEN_KEY)
+        && !$this->csrfToken->validate($csrf_token, 'rest')) {

Can this new logic be tested? Probably.

Also I think this change could do with a change record.

Random fail in UpdatePathTestBaseFilledTest.

Committed 62b7392 and pushed to 8.2.x. Thanks!

OMG YES YES YES! This unblocks #2403307: RPC endpoints for user authentication: log in, check login status, log out :)

@tedbow can you still create a change record?

Sorry I saw alexpott's request for a CR but convinced myself it had been added after reviewing the new tests - that'd be good to have indeed for this.