This child issue of #2403307: RPC endpoints for user authentication: log in, check login status, log out
In that issue the http logout route needs CSRF protect but the logic for including the CSRF token in the header is in the REST module so is not usable unless we made the user module dependent on the REST module.
This issue will
Create route requirement _csrf_request_header_token which replace the current rest _access_rest_csrf(which will be deprecated)
Create new CsrfRequestHeaderAccessCheck which replace \Drupal\rest\Access\CSRFAccessCheck
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff-2753681-35-42.txt | 7.13 KB | tedbow |
#42 | 2753681-42.patch | 17.79 KB | tedbow |
#35 | interdiff.txt | 2.19 KB | tedbow |
#35 | 2753681-35.patch | 15.13 KB | tedbow |
#24 | 2753681-24.patch | 14.13 KB | tedbow |
Comments
Comment #2
tedbowOk here is the first patch.
Remaining issues that I can think of:
Comment #3
Wim LeersWe want a
@todo
to remove the second one in Drupal 9.0.0.#2:
system
module is the only fit, sadly.Comment #5
Wim LeersThis blocks #2403307: RPC endpoints for user authentication: log in, check login status, log out.
Comment #6
dawehnerCan we keep using access_check.rest.csrf to be an alias of the core service? Its removes some BC break
Comment #7
tedbowOk here is patch that adds the test module csrf_test.
This has to 2 routes one route uses _csrf_request_header_token and one that uses the deprecated _access_rest_csrf.
It also has \Drupal\tests\system\Functional\CsrfHeaderTest that tests these routes.
It also adds a new route at 'session/token' to retrieve the token.
I deprecated '/rest/session/token' but not sure I did it right. It is now pointing to the same method as 'session/token'
I did this and added a @todo to remove in Drupal 9.0.0
Comment #8
Wim Leers#6++
s/path/route/
s/deprectated use/deprecated, use/
s/CSRF/Csrf/
s/manager/generator/
C/P remnant.
s/array()/[]/
s/CSRF Header Token/CSRF request header token/
s/Core/core/
?
:P
s/header/request header/
CsrfRequestHeaderTest
{@inheritdoc}
s/post/POST/
:D
Comment #9
Wim LeersComment #10
tedbowUploading patch that addresses @Wim Leers' review in #8
The only one I am not changing is #8.12 regarding using {@inheritdoc}. The $modules property is not actually in the parent class like you might think.
See \Drupal\Tests\BrowserTestBase::installDrupal
Not really sure why it is done this way. I assuming I can't use {@inheritdoc} in this case.
Comment #11
tedbowWhoops named interdiff wrong not seeing how to cancel it now
Comment #14
tedbowArgggg git case file name change problem
Ok this is the exact patch as 2753681-10.patch except with case of CSRFTokenController.php correct to CsrfTokenController.php
Comment #15
dawehnerI'm feeling a bit bad about it.
Cool, thank you!
The namespace should be
Drupal\Tests\system\Functional
... aka. this will not run on the testbot. This is kinda of a bummerNitpick of the month: 2 spaces!
Comment #16
tedbowNo need, thanks for review!
Here is patch that fixes the 2 issues
Comment #17
dawehnerLet's not extend the controller base as long we don't need it.
Comment #18
tedbowGood point!
Comment #19
Wim LeersSuperNit: I'd format this array across multiple lines, then you can add the
@todo
comment above the relevant line.+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.
SuperNit: I think this could use an
@see
.Comment #20
tedbow@Wim Leers thanks for review. Here is a patch that addresses those issues.
Comment #21
dawehnerI 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.
Comment #22
Wim LeersComment #23
neclimdulWith the new test, why are we changing this? Shouldn't we leave it the same and deprecate the test aligned with the route?
Comment #24
tedbow@neclimdul nice catch!
I am sure about the policy for deprecating tests but this seems like good idea.
Access to routes is already tested in the new test \Drupal\Tests\system\Functional\CsrfRequestHeaderTest::testRouteAccess
Comment #25
dawehnerRTBC under the assumption that https://www.drupal.org/node/2550249#comment-11344307 is fine.
Comment #26
neclimdulWorks for me, +1 on RTBC. Thanks guys!
Comment #28
neclimdulLooks like bot problems. Been fixing a lot of those on RTBC issues this weekend.
Comment #29
tedbowComment #30
catchFew questions on this one, code itself looks good:
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?
+1.
This was using 'rest' as the key.
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.
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.
:)
Comment #31
Wim LeersIMO the latter. The former would cause that to become two round trips rather than one.
terminating sessions anywayapparently we don't terminate sessions upon updating? Should we add an update hook to REST to terminate all sessions?.Comment #32
catchIf 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).
Comment #33
tedbow@catch good point in #30.4
We could change it something like
That way in any sessions that just got the token using the old method during the update will work. After the update even the 'rest' would still work it would still have to be token generated by the system so that doesn't seem like a security problem.
Performance wise after the update for any valid requests using self::TOKEN_KEY the first part of the && condition will not be true so it won't make the second validate call. So it would only be performance hit for requests for invalid tokens.
We can then safely remove it in 8.3 because 8.1 won't have been support for a while, meaning core doesn't support a 8.1 to 8.3 upgrade(am I correct about that?).
Comment #34
catch#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.
Comment #35
tedbowOk here is patch that does my suggest in #33
If also gets rid of "\Drupal::csrfToken()" by making the token generator an argument to the service. I hope there wasn't a reason we weren't already doing that.
Comment #37
Wim Leers#35 had a random fail, so re-testing, should come back green.
Comment #39
Wim LeersGah again?!
UpdatePathTestBaseFilledTest
Comment #40
alexpottCan this new logic be tested? Probably.
Comment #41
alexpottAlso I think this change could do with a change record.
Comment #42
tedbowGood call!
In \Drupal\Tests\system\Functional\CsrfRequestHeaderTest
added outer loop
This uses \Drupal\csrf_test\Controller\DeprecatedCsrfTokenController
which creates the csrf token using the 'rest' value
Comment #44
Wim LeersRandom fail in
UpdatePathTestBaseFilledTest
.Comment #45
Wim LeersComment #46
catchCommitted 62b7392 and pushed to 8.2.x. Thanks!
Comment #48
Wim LeersOMG 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?
Comment #49
catchSorry 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.
Comment #50
tedbowHere is the draft change record: https://www.drupal.org/node/2772399
Please look over and publish if ok
This handbook page will also probably need to be updated: https://www.drupal.org/node/2122195