Problem/Motivation
Currently there is no way to log in except via the user login form. This not ideal for non-browser/head-less clients.
If we use the user login form we always receive a 200. For failed attempts we need to receive appropriate 40x responses based on the type of error. We want to verify the logout and the login status of the current user.
Proposed resolution
- Create a controller and corresponding routes for logging in, login status, and logging out that return meaningful HTTP status codes and messages.
- Support JSON by default. Upon installing the
serialization
module, all serialization formats become available. - The same flood control protection as the current login form (and equal or better test coverage), to prevent security problems.
We are intentionally not implementing this as a set of @RestResource
plugins, because:
- logging in/out is not CRUD, it's not stateless, and it's really RPC, not REST — see #164
- to avoid having to enable the REST module just for logging in, for example when using the GraphQL or Services contrib modules — see #172
Beta phase evaluation
Issue category | Feature because actually there's no way to login from a headless app, only using the "User login form" you can login if you need a cookie session but not sure if this is the correct way or, at least, the best way. |
---|---|
Issue priority | Major because the cookie session is important in some contexts, for example because Basic auth is not possible with Views (see https://www.drupal.org/node/2076725).So, if you want to create a headless app probably you need, in some cases, a cookie session. See https://groups.drupal.org/node/473598 |
Comment | File | Size | Author |
---|---|---|---|
#274 | 2403307-274.patch | 33.5 KB | Wim Leers |
#268 | 2403307-268.patch | 36.46 KB | tedbow |
#268 | interdiff-244-268.txt | 9.62 KB | tedbow |
#263 | interdiff-2403307-244-263.txt | 15.36 KB | tedbow |
#263 | 2403307-263.patch | 44.56 KB | tedbow |
Comments
Comment #1
dawehnerYeah I totally think we can add quite a bunch of stuff.
Comment #2
marthinal CreditAttribution: marthinal commentedOk so I'm going to work on it.
Comment #3
marthinal CreditAttribution: marthinal commentedCreating the system connect resource. Missing session id. This is a first step :) We can receive a hal+json with the current user data.
Comment #9
almaudoh CreditAttribution: almaudoh commentedCan we have this in 8.0.x?
Comment #10
marthinal CreditAttribution: marthinal commented@almaudoh Yes I think so.
Comment #11
marthinal CreditAttribution: marthinal commentedFound the problem... #2316153: [typo follow-up] PHP 5.4.30/5.5.15+ changed json_encode() of DateTime objects to include microseconds, breaks tests. So now it should work.
Comment #13
marthinal CreditAttribution: marthinal commentedAs discussed with @klausi in irc, we need a User Login Resource instead of a System Resource.
Comment #14
marthinal CreditAttribution: marthinal commentedComment #15
marthinal CreditAttribution: marthinal commentedComment #16
marthinal CreditAttribution: marthinal commentedComment #17
marthinal CreditAttribution: marthinal commentedAs discussed with @klausi a 400 should be the status for a failed attempt. (http://stackoverflow.com/questions/8273757/should-a-failed-login-attempt...)
Comment #18
klausiMore like a feature request, since you can also do the hacky cookie login by submitting the user login form, which is of course not ideal.
Comment #19
marthinal CreditAttribution: marthinal commentedComment #20
marthinal CreditAttribution: marthinal commentedUsing the patch from #2419825: Make serialization_class optional it works. Now I can log in from rest receiving a 200 when everything is ok and a 400 for a failed attempt.
Not sure how to add the logout for the same resource. I need to study a little bit more the canonical path...
Comment #21
marthinal CreditAttribution: marthinal commentedOk! Needs work but maybe we can try something like this.
Login op works perfectly using this json "{"op": "login","credentials": {"name":"marthinal","pass":"secret"}}"
Comment #24
marthinal CreditAttribution: marthinal commentedAdding the "X-CSRF-Token" Header the logout works as expected. Not sure if this is the best way. I Can log in without permissions (Access POST on User Login resource) and then I found this problem #2420559: REST permissions are not working as expected..
Comment #25
clemens.tolboomNo newline at end of file
'username' should be 'name'
Maybe a little more specific regarding the message strings?
The field 'name' does not match on 'Missing username'
I'm not sure how to test. Do I get a new cookie for authenticated user?
Comment #26
clemens.tolboomDouble semicolon
It should return a default response too.
How should I test this?
My command
curl --header "Content-type: application/json" --header "Accept: application/json" --request POST --data '{\"op\":\"login\"}' http://drupal.d8/user/login
fails with
{"message":"Not Acceptable.","supported_mime_types":["text\/html","application\/vnd.drupal-ajax","application\/vnd.drupal-dialog","application\/vnd.drupal-modal"]}06:32:54 {feature/2403307-user-session *+$%} ~/Sites/drupal/d8/www$
Comment #27
marthinal CreditAttribution: marthinal commentedYou need to apply this patch #2419825: Make serialization_class optional. A json could be "{"op": "login","credentials": {"name":"marthinal","pass":"secret"}}" for login and this one "{"op": "logout"}" for logout.
Comment #28
clemens.tolboomHmmm my Dreditor does not add the line numbers in #25 and #26. Then it's hard to tell where what belongs to :-/ Try to fix those tomorrow.
The summary proposes three ops of which I miss 'status'. Is that still planned?
Comment #29
marthinal CreditAttribution: marthinal commentedI think "status" could be like "system connect" for Services (D7) that returns the details of currently logged in user.
Yes the current patch is a quick and dirty and needs work.
Comment #30
marthinal CreditAttribution: marthinal commentedAttached a couple of screenshots.
For the logout we don't need to add credentials.
Comment #31
marthinal CreditAttribution: marthinal commentedAdded flood control. I think we can do exactly the same for other resources. For example contact messages... I would like to add the same for the user registration resource too because we can create many users without control for the moment. More info about user registration here #2291055: REST resources for anonymous users: register. This is a first attempt, the limit is 2 and it works but should be 5 or more.
Maybe we could have a method on ResourceBase to control it ... not sure about the best way.
Comment #33
marthinal CreditAttribution: marthinal commentedHere I'm using the user.flood configuration from user module directly. As I said not sure about the best way... Needs review!
Comment #34
clemens.tolboomWhy not throw exception in restLoginCookieFloodControl similar as done in \Drupal\contact\Controller\ContactController::contactFloodControl
This should be the first test too. Otherwise I can still flood with missing username or password
You should throw the exception in function similar as \Drupal\contact\Controller\ContactController::contactFloodControl
This should be a onliner
return !\Drupal::flood->...
No newline at end of file
Comment #35
clemens.tolboomWhy remove this from config? I liked it into the config. And I can image REST flood control is different from user flood control.
Comment #36
marthinal CreditAttribution: marthinal commentedAs discussed with @Berdir last weekend through IRC, would be perfect if we have a method in the ResourceBase class to control the flood.
This patch works for the login(5 attempts and then blocking) but I see problems here:
1) We need to know when the entity has a flood control. But here we can check the configuration.
2) flood should be detected from a config file with the same pattern for the name(we have contact.settings for contact module and user.flood for user module). This could be a problem if we want a standard way to detect the flood.
3) flood yml structure should always be the same. Actually flood for contact module and user module are different.
contact :
user:
Comment #37
clemens.tolboomI can test this having basic auth enabled on the resource.
Patch had some tiny updates: whitespace, mismatch var, unhelpful error feedback.
Open issues:
- the path user_login which should be user-login
- Rest UI does not get
$resource['uri_paths']['canonical']
from this resource. I tried to fill this into the annotation but failed.- Why not report missing op and possible values?
Comment #38
clemens.tolboomI forgot to set permissions for anonymous. But that should be the default!
Comment #39
clemens.tolboomWeird last remark. I get 403 Forbidden using cookie (+XSRF token) when trying to logout after login. I can only logout when first logout through the Web UI. Is user-1 not allowed for something.
You can test this with https://github.com/clemens-tolboom/drupal-8-rest-angularjs/tree/develop
Comment #40
Berdir#2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController
Comment #41
andypostLooks like next release material
Comment #42
klausiNope, that method does not throw an exception?
The flood service should be injected in plugins that need it, so use $this->flood instead.
Should be "Contains ..."
Can we be a bit more specific what this does? Like "Allows user logins by setting session cookies."
This should describe all possible keys in $operation. And the type is most like string[], correct? See https://www.drupal.org/node/1354#types
authentication service should be injected.
Flood should be injected.
And we need a test case for this.
Comment #43
clemens.tolboomIf fixed all but #2 and #6 from #42
[edit]removed garbage[/edit]
Comment #44
clemens.tolboomI forgot to tell I use Rest UI to enable the resource.
@andypost: I don't think this is 8.1.x stuff as in current core rest clients have to use BASIC_AUTH sending username/password on every request which is bad. If disagree please motivate.
Comment #45
clemens.tolboomAttached a patch with a failing test. Hope some sees the failure.
Each request needs the token. So updated the summary.
Remove the basic auth headers to see the code below fail.
Comment #46
clemens.tolboomComment #49
klausi@clemens.tolboom: if you use basic_auth then you must send the user/password on every request. That is simply how basic authentication works.
If you don't want to send that on every request then you should use cookie authentication, right?
PHP_AUTH_USER is not a real HTTP header. See http://en.wikipedia.org/wiki/Basic_access_authentication and http://curl.haxx.se/docs/httpscripting.html#Basic_Authentication for how to use it with curl.
Comment #50
clemens.tolboomI hoped I missed something :-)
Can you provide / update issue summary with a proper curl example which should work with current patch?
Comment #51
clemens.tolboomTo make this work patch is needed from #2419825: Make serialization_class optional
Updated summary.
Comment #52
clemens.tolboomIt's of no use to try to write tests which will fail due to #2419825: Make serialization_class optional. So I merged that issue into here and extended the tests.
Unassigning @marthinal
Comment #53
clemens.tolboomWrong patch file.
Comment #54
clemens.tolboomWe still need #2 and #6 from #42 and
Fix this.
We lost the status op.
This must be added.
- you are logged in as 'user'.
- you are not logged in.
This needs the two cases: logged in / not logged in.
This gives a 'nvalidArgumentException: Placeholders must have a trailing [] if they are to be expanded with an array of values. in Drupal\Core\Database\Connection->expandArguments() (line 645 of /Users/clemens/Sites/drupal/d8/www/core/lib/Drupal/Core/Database/Connection.php). '
Uncomment this. My bad :-/
Comment #57
clemens.tolboomTest fail as expected. Needs some eyes.
Attached the interdiff.
Comment #58
clemens.tolboomInterdiff remarks
Not sure why this is needed? If left out it gives undefined index 'serialization_class'.
Needed extra headers. (To inject basic auth header)
Logging was missing a lot of info. These lines did help much.
Why do I need basic auth to make this work?
Comment #59
clemens.tolboomRemoved the unnecessary X-CSRF header. And noted the inclusion of #2419825-1: Make serialization_class optional
Comment #60
jhedstromFeature request => 8.1.x.
Comment #61
marthinal CreditAttribution: marthinal commentedAdded Beta phase evaluation
Comment #62
marthinal CreditAttribution: marthinal commentedComment #63
droti CreditAttribution: droti commentedIs that patch working for everyone? I have been trying over and over and feel like I must be missing something but I can't get a proper response from the login form. I"ve just updated core and had to apply the patch manually, but I'm still not getting anywhere. I'm about to give up and just use basic auth and wait for this to be cleared up in core. Is this working for everyone else with the latest core updates?
Comment #64
marthinal CreditAttribution: marthinal commented@droti Yes, I can confirm that it works.
Comment #65
marthinal CreditAttribution: marthinal commentedAdding Unit tests, refactoring a little bit and detecting if the account is blocked before authentication.
Missing review #53 and improve flood control after #2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController has landed.
Comment #68
clemens.tolboomPatch needed a re-roll. This will not fix the failing test.
Where is $op === 'status'?
On local I get same exception
Comment #70
clemens.tolboomPrevious patches had constructs like
which tests for the status code but not for the text. So most tests gave wrong result. I added new assertResponseAndText to test both values.
Furthermore there was no pre check for user status login / logout. Adding this now gives warning on logout. And we cannot login either.
I've added two patches of which one skips the current user status.
Comment #75
clemens.tolboomComment #76
clemens.tolboom@droti I've updated the curl commands which works sort of as they use the _format=json.
Should be a direct call to
Still this breaks unit test as service 'current_user' is not available
We must safe guard the user here is logged-in to help app developers manage state consistently.
Unfortunately the isAuthenticated() stays === TRUE no matter what.
Regarding core/modules/rest/src/Tests/RESTTestBase.php:92
shouldn't that only be done on cookie based requests?
Comment #77
clemens.tolboomAs we do basic_auth which is not a 'real user login' iirc that could explain the isAuthenticated() problem for having no real session?
The issue summary could use some update on what the steps are. I guess using the create_a_user_session-2403307-75-bad-state-management.patch gives a COOKIE so next we have to ask for a token.
Comment #80
klausiWhy do we add that to ResourceBase? It is only used in the user login resource, so it should be only injected there.
Instead of checking $defintion and then $class again we can just check empty($definition[..]).
And the comment should say "If the plugin does not specify a serialization class just decode the received data. Example: received JSON is decoded into a PHP array."
should be called UserLoginTest
Why do you need basic authentication in this test? A client should not need any authentication to perform a cookie login, right? Instead, you should allow access to the resource for anonymous users with permissions.
Comment #83
clemens.tolboom#80-1 Done but don't we need flood control on other content? As that is in the future I moved it to UserLoginResource
#8-2 done
#80-3 done
#80-4 Does that implies we do not have to authenticate the requesting user? Like user admin (basic -auth)
request session for other user
core/modules/rest/src/Tests/RESTTestBase.php:66 does
I'm not sure what that does/means. Will chew on this.
Anyway I removed my isAuthenticated() code for login/logout. But will 'status' ever work? I guess not from a basic_auth call.
Comment #86
andypostComment #87
clemens.tolboomI don't get the Drupal\views_ui\Tests\DisplayTest errors. user/login failed.
Comment #89
clemens.tolboom@andypost thanks for the XREF. Maybe #2472535: Remove SessionManager::delete in favor of a portable mechanism to invalid sessions of authenticated users is related too.
I've removed --user admin:admin aka basic_auth from the issue summary examples. Added a 'status' call but that needs a cookie to work.
Comment #91
marthinal CreditAttribution: marthinal commented#80.1 Should we use flood control for content creation?
#80.4 We don't need basic_auth for the login test because we are testing a cookie session. Removed from tests.
We could add the possibility to reset the password from this resource adding a new $op.
Comment #92
clemens.tolboomI added assertResponseAndText for a reason. See #70
Please restore
Comment #93
marthinal CreditAttribution: marthinal commented@clemens.tolboom Oops Sorry :)
I think this assertion should be added to RESTTestBase because it is really useful. What if we have assertResponseBody(), adding the possibility to check the code as well?
Something like this.
Comment #94
marthinal CreditAttribution: marthinal commentedAdded the possibility to reset the password. The email is sent as expected but you will receive a 500. This is because we are rendering a link when replacing tokens for the email and we need to create a new cache Context. This is fixed by the last patch #58 here #2291055: REST resources for anonymous users: register.
Basically with this patch we use an uncacheable response.
Interdiff from 91 because I would like to know if we want to change the assertion on this way.
Comment #95
marthinal CreditAttribution: marthinal commentedYou can test using:
{"op": "password_reset", "reset_info": [{"name": "marthinal", "lang": "en"}]}
Comment #97
marthinal CreditAttribution: marthinal commentedOk Let's try :
{"op": "password_reset", "reset_info": {"name": "marthinal", "lang": "en"}}
Comment #99
neclimdulReroll, just chasing HEAD. No changes, just a use statement conflict in ResourceBase.
Comment #100
neclimdul#80.2 #2419825: Make serialization_class optional should probably resolve this... which means this is probably postponed.
Comment #101
tyler.frankenstein CreditAttribution: tyler.frankenstein commentedI'd like to see this resource return more than just a successful login message upon 200. How about we return some basics about the user's account, and their new X-CSRF-Token? This would eliminate the need for 2 extra calls to the server that will be commonly needed when building apps.
Although when reading about POST for any REST environment, it sounds like we're not supposed to return much of any JSON, and if we do I guess it's supposed to contain a URI to the newly posted "thing". So as an example, if uid 1 logs in.
I'm curious what others think here, mainly because I think it'd be silly to have to do 2 extra calls to Drupal to get this much needed data after a successful login attempt.
Comment #102
joelpittetbumping to 8.1.x branch because this is a feature request. Unassigning as this issue hasn't been changed in a while.
Comment #103
Wim LeersMarked #46377: Web Service for User Login as a duplicate.
Comment #104
Wim LeersAnother duplicate: #2504489: log in and out via REST.
Comment #106
marthinal CreditAttribution: marthinal commentedIMHO this resource does not need canonical URI path, we only have access to POST method and probably we have the same situation when registering new users. So ResourceTest::testUriPaths() should detect if the resource has no uri path definitions.
@tyler.frankenstein
I agree, I think we should return the csrf_token and basic info about the current user. Added to the resource and verifying the username from the integration test.
Thanks!
Comment #108
marthinal CreditAttribution: marthinal commentedFrom #106
This change makes sense because we return TRUE. Changing the boolean value in our unit tests... let's try again
Comment #109
marthinal CreditAttribution: marthinal commentedComment #110
marthinal CreditAttribution: marthinal commentedComment #111
dawehnerYeah that is not really a list of strings, let's use array instead
Code should be wrapped in
@code>/<code>@endcode
Can't we use different URLs for that?
This sounds like a GET url to be honest
Just a suggestion: We would move all that into a guardInput method or have guardEmptyCredentials(); guardRequiredUsername() guardRequiredPassword() ...
Let's inject those services ...
We could skip the isset by using
$definition += ['uri_paths' => []]
Comment #112
dawehnerWorking on that now and improve it fundamentally
Comment #113
dawehnerHere is some not working start to split up the different resources
Comment #115
tedbowHere is updated patch that changes \Drupal\rest\Tests\UserLoginTest to take into account that these are separate resources now because of @dawehner's patch on #113.
So there no longer needs to be 'op' in the payload since each resource is a different 'op'
The check after login to user_login_status still fails. I am assuming we have to pass the token. I had tried to do this but it wasn't working so I left that out of the patch for now.
Comment #117
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedIs the REST logger not registered as its own service already? If not, it should be. Let's do that and simplify this line.
Thou shalt not use the global service locator within an object. This object is already injecting its other dependencies. Inject this, too.
That does double for User::load(). Inject the entity type manager and call getStorage() on it.
Both of these make the class needlessly untestable. Don't do that.
Why is this global function not given a wrapper method when user_is_blocked() is?
The ResourceResponse needs user cache metadata associated with it. I think the following will work:
return (new ResourceResponse(...))->addCacheableDependency($user);
(Untested.)
That may be true elsewhere in the patch as well, I didn't check.
This makes me cry. :-( (Not the code here, the fact that user_is_blocked() is still a function and not a service method.)
If we know the class is ImmutableConfig, type that in the method signature, too.
Also, the method name is not self-descriptive. I'd suggest isFloodBlocked(), as it's a boolean return (is) and it's checking if the user is blocked for flood reasons.
Copy-paste error on the label.
This seems wrong. We should be returning a machine-friendly response to determine if the account is logged in or not. Either a different marker in the body (not a human string) or a different status code.
I am tempted to say it should be 204 No Content for logged in, and 403 for not logged in. That doesn't feel quite right either though.
Why not just send a 204 here? The string body is redundant, and not translated, and a user agent will probably want to use its own messaging anyway.
This should already be verified by the fact we got this far, no? If not, it would have been rejected by the routing system? Or does REST mask that somehow? (It shouldn't...)
$name comes from user input, and is thus unsafe to put in a string. This should be translated, at least for escaping purposes.
I'd prefer to see this split out into separate methods, and the try-both moved to post(). doMultipleThings() methods are generally a code smell.
Side note: This would be a great place to use PHP 7's ?? operator, if we could use PHP 7. :-(
This now allows unclassed request bodies. I am not sure I like this. In fact, I'm reasonably sure I don't. One of the biggest traps I've seen of REST systems (of which I admit I've only dug into a few PHP ones) is not having a formal domain object for the resource data structure. That leads to giant arrays of doom, and thus a lot of fugly error handling in a lot of places.
I'd much rather see the new resources that allow POST requests to just define a serialization class, even if it's a small one, so that we can be dealing with formal data definitions.
This change doesn't make sense to me. Is it because we expect the body of the response to be different now? If so, the second parameter is the message and so shouldn't just be the former expected body. If not, then I don't understand what this is doing.
Oh, that's why. I disagree with this change. The response body is something you assert. The response code is something you assert. Those should be two separate assertions. Turning one into a wrapper of the other is bad news, because now I cannot verify the body without also verifying the code. Having an optional parameter before a required parameter is extremely bad practice in PHP. This change should be reverted. Just add an assertResponse() call where appropriate in the test. That makes the test much more self-documenting.
In the past, I've found it more useful to json_decode() the response and then check the array keys, rather than doing a string match on it. We shouldn't care about whitespace or key order in the response body, just its logical semantic meaning. Let's do that here as well.
(And in the rest of this and other tests.)
This already has me worried, but if nothing else the variable name is wrong. It gives no indication that we're talking about self::$translation (which is what, exactly...?), which is confusing later on. I saw $this->reflection and though it was a reflection of the current class, which would make no sense at all.
For all of these mock calls, don't use the string name of the class. Instead, include a use declaration and do UserAuthInterface::class (no quotes). (Viable as of PHP 5.5.) That makes the code easier to read, and makes the use of that class exposed to the language syntax, and therefore your IDE and other tooling.
Test object. testClass implies it's a string name of a class. This is setting the object (not class) to be tested.
Ibid. Also, I don't understand why we need both of these...
I have to argue that every use of this method means a bad test. You're testing the unit, the class. That doesn't mean testing every internal method out of context. That couples the test far too closely to implementation details that it, by design, shouldn't be dealing with. It's just testing that the implementation gets the right result. Digging into a protected or private method makes the class unmodifiable without changing the test, which means your tests are too coupled to implementation.
Comment #118
dawehnerThank you @crell for that great review, but I haven't yet got time to address it.
Just posting some work on the test so it passes more ...
Anyone, feel free to address the points of Crell.
Comment #120
dawehnerComment #121
tedbowAssigning to myself. I will try address @Crell's review.
If someone at DrupalCampES or elsewhere is already working on this ping me on irc and let me know
Comment #122
tedbowOk here is patch that starts to addresses @Crell review in #117
Specifically it address his point 1-11
Points 3, 5 were just comments about unrelated code
Just a couple notes about changes
#1. The rest logger was exposed as service just used everywhere
#8. Return values for loginstatus: I went with machine-friendly string constants. I looked at http codes and didn't find good codes to return. I also looked ReST APIs for Facebook, Yammer, and couple others. They aren't changing http codes here. Doesn't seem to be a standard practice for doing that but didn't spend too much time looking.
Unassigning myself because I don't know if I will back to this for at least 5 days.
Comment #124
Wim LeersSo that leaves #117.12 through #117.21 to still be addressed.
Here's another review.
Why is no URI defined here?
Nit:
Nit: CSRF token generator
Nit: user authentication service
Same here
Wouldn't these make more sense?
Nit: 80 cols.
Comment #125
dawehnerHere are a couple of improvements.
Sure, conceptually for sure, but its a
POST
request, so this is not necessarily helpful ...On the first sight I thought, this is a wonderful idea. On the second thought I realized that the idea to use passwords for authentication is a user related concept, mh ... its tricky because using /user/login conflicts with the existing route.
Comment #126
Wim LeersThe existing route is for HTML. This is everything except HTML. That's the only thing that the routing system can do that the menu system couldn't: multiple controllers for the same route, each dealing with different formats. So… why wouldn't that be possible?
Comment #127
Wim LeersDiscussed with @dawehner in IRC.
I misrepresented things in #126. What we want here is not route matching based on request format (
_format
route option) negotiation, which determines the format of the output.What we want is route matching based on input format (
_content_type_format
option), which defines the required/accepted formats to send data to the server.(We have
\Drupal\Core\Routing\ContentTypeHeaderMatcher
to do route matching based on incoming content format, and\Drupal\Core\Routing\RequestFormatRouteFilter
to do route matching based on outgoing (requested) content format.)Comment #128
dawehnerLet's see what happens
Comment #130
Wim LeersThese all have the same relation type. I wonder if that caused it.
Comment #131
dawehnerTo be clear, I am not what clear how these link relations are meant to work.
The reason why I went this way is that all of those 3 resources uses a POST request, as they change the state of the system. The reason why I went with this, is the following code:
Comment #132
Wim LeersOh hm. That makes sense. Though I'm not sure
\Drupal\rest\Plugin\ResourceBase::routes()
actually makes sense.Comment #133
dawehnerIn general or for this usecase? It feels for me that those resources are not really typical REST resources to be honest.
Comment #134
Wim LeersGood point.
Comment #135
dawehnerLet's remove the unit tests for now. IMHO we are mostly connecting dots, so the value of those unit tests is quite minimal.
Comment #136
Wim LeersGreat progress here! :)
I tried to do a comprehensive review. I fixed 99% of my comments/docs nitpicks myself, to make this review less annoying/painful.
"Resource" as filename suffix: once.
No suffix: thrice.
Core has
EntityResource
andDbLogResource
.Let's make that consistent.
Broken English. Fixed.
We only capitalize the first character. Fixed.
s/array/string[]/.
Fixed.
Useless comments. Fixed.
Should typehint to interface. Fixed.
I think "basic metadata" would be better. Also, it's not "adding", it's sending. Fixed.
The second (
200
) and third ([]
) parameter can be dropped. They make this look needlessly complex.Like @dawehner said in #125, this makes conceptual sense, but since it's a POST request, it's rather pointless.
This specifies values for the first two parameters of
\Drupal\Core\Flood\FloodInterface::register()
. The third parameter is indeed optional.That means this is an IP-based failed login event.
Shouldn't we also do a per-user failed login event?
See
\Drupal\basic_auth\Authentication\Provider\BasicAuth::authenticate()
for an example that looks more solid/complete than this one.Copy/paste remnant :P Fixed.
Missing proper documentation. Fixed.
I'd argue this should be
AUTHENTICATED
orANONYMOUS
.s/Response/Responds/ Fixed.
Not consistent with the rest. Fixed.
Bad documentation string again. (Although documenting these is pretty damn silly indeed.)
Fixed.
Doesn't this need separate handling for the case where there is no currently authenticated user? Seems like this should throw a 403 if the anonymous user does this.
English can be improved. Fixed.
Inconsistent wording compared to the other resources. Fixed.
Broken English. Fixed.
Looks kinda mysterious why you'd need a langcode here at first sight. When you read the code, it makes sense (for the notification e-mails). Fixed.
Method, not class. Fixed.
I expected flood control here… but I see that
\Drupal\user\Form\UserPasswordForm
also doesn't do that. So, no matter.Useless comment. Fixed.
Let's use the same string as in
\Drupal\user\Form\UserPasswordForm::validateForm()
. Less translation work. Fixed.Broken English. Fixed.
Should be 3rd person singular. Fixed.
This is being done in #2419825: Make serialization_class optional. Let's land that one first, then this patch can become smaller.
Docblock is not updated. NOT yet fixed.
This is testing the common case, not the edge cases.
We're missing test coverage for:
We're not enabling a service. NOT yet fixed.
Rather than doing this, let's improve
\Drupal\rest\Tests\RESTTestBase::enableService()
, which would make this 10x easier to read.I'd call this
$request_body
. That's also what it's called inRESTTestBase::httpRequest()
.Why not use
/user/login/status
instead, like all the others here?Comment #137
Wim LeersSo, in #136, I addressed everything except points:
Comment #138
Wim LeersPer #136.28, this is blocked on #2419825: Make serialization_class optional. Once that lands, this patch becomes a bit smaller.
The IS also needs to be clarified. And this will need a change record.
Comment #139
Wim LeersBetter title: the old title didn't match the scope.
Comment #140
dawehnerThank you wim!
Well, one could come up with metrices, like all plugins, which don't suffix everything.
In a custom project I would have kept the 200 to make it clear that its a 200 response, which well, doesn't make sense for ordinary responses, but in the context of REST its always helpful to think about the status codes as well.
Well, one could argue that people look out for example and copy it to
GET
requests as well.There is one issue which extract the logic we have for users into a generic service ... if I could just find it.
Mh, I mean I don't care about the name of the constant. This is the response to a user login, so it should be clear whether logging in / logging out happened.
Sure, let's put that onto the route level.
Yeah for me though this is not a reason to wait on this issue.
To be honest payload is maybe a better term, given that its the data before we convert it to JSON, which is then the actual request body.
Comment #141
Wim LeersThanks for pushing this forward! :)
So, looking at the list in #137 and comparing to what @dawehner did in #140:
So that leaves just 9, 10, 28 and 30.
Fair. So keep the 200, but remove the empty array?
True. But if we keep it, it needs to be entirely correct, and have test coverage. We'd also need the
user.flood
config object as a cacheable dependency.Nit.
Nit: can be chained.
I don't see how this tests flooding?
The flood detection looks like this:
That error message is what we should be able to assert.
Comment #142
dawehnerOne day someone needs to fix phpstorm to no longer do that.
Good point
OH yeah I just didn't really know how to do that properly, so I applied the pattern I learned in one of my computer science cources, which was about already write down what you would have to do anyway :)
Comment #143
dawehnerYeah, so here is a bit more test coverage, this time even executed locally.
Note: there is one failure related with rendering the email. Not sure yet about the underlying problem.
Added a follow up: #2720233: Use arguments resolver in RequestHandler to have consistent parameter ordering
Comment #145
dawehnerThere we go
Comment #147
dawehnerAdded a change record and adapted the issue summary
Comment #148
dawehnerHere is a short description of the last failure:
The problem
The
$flood->register()
call on the test process has127.0.0.1
as IP all the time, seecore/scripts/run-tests.sh:433
.When running on a apache vhost configuration, that looks like the following:
and a
/etc/hosts
file that looks like:apache sets
$_SERVER['REMOTE_ADDR']
to127.0.0.1
When you run the same under another configuration
it will use
::1
(localhost under IPv6) on the actual request (the one in which the login resource is called), so we see different IPs and the flood registration is "ignored".Solution
Not sure yet
Comment #149
dawehnerLet's use actual HTTP requests ...
Comment #150
dawehnerWe talked about that yesterday at Drupalcon and agreed it doesn't make sense for those things to be rest resources. We will use simple routes + controllers and skip the password reset from this patch.
Comment #151
dawehnerLet's try
Comment #153
Crell CreditAttribution: Crell for Acquia commentedlogin() and logout() are the controller methods, so they should have docblocks that clarifies that.
Docblock here, too.
These constants make no sense to me. Why? I'd almost expect a raw integer back, since it's a boolean state.
Also, Symfony responses have a text/html content type by default. If we're sending back a bare string (or int), we should explicitly set it to text/plain.
Both of these should be restricted to POST requests.
Comment #154
dawehnerGood point!
You cannot be more right.
Comment #156
dawehnerThere we go
Comment #157
andypost@dawehner please update IS about why that becomes controller and not resource, subject should be updated as well
Comment #158
dawehnerComment #159
Wim LeersThe biggest problem I see with this new approach: it is enabled by default, even when REST is not enabled. Which means we need to be absolutely certain about this approach, because it doubles the number of ways you can log in to a Drupal 8 site. Which means we need solid, comprehensive test coverage proving that flood control is working, just like we have that for our regular user login form.
Nit: s/array()/[]/
These aren't strings, but ints.
Shouldn't they be booleans though?
Nit: outdated comment.
Nit: s/login/log in/
JsonResponse
… but isn't it a problem that we are hardcoding this to JSON? Here, but also in the routing YAML. (So at least this is clearly the intent.) Why not allow to use
s/the ID and/the logged in user's ID, roles and/
Comment #160
dawehnerWell, I don't see much use in using alternative formats for data which is really limited. There will never be more than username and password and the result will always be a boolean no matter what. I think every programming language and environment can deal with that :)
Yeah maybe this should still be some setting or so. Exposing it by default increases the surface area too much. I agree.
Comment #161
swentel CreditAttribution: swentel commentedSince it's not a rest resource anymore, just user.login_cookie ?
However, my 2 cents.
I don't know the full details of the talk at drupalcon, but I'm more at Wim's side that I also think this still makes more sense as a rest resource. I'm in the middle of a project where any user which has a consumer token can use the api then and they can either use json, xml or jsonp as response formats. I have my own authenticate reource which returns more data (tokens, expire times etc), but I don't have to care about the format. Developers who want to consume the api and want to use their favorite format don't have to deal with a different format suddenly. I know I have a more advance use case (oauth, consumers etc), but why using normal controllers because we only return minimal data?
Which is then ironic because we have such a system in place when it's a resource plugin, no ? :)
Looking how the new routes basically mimic a rest configuration .. it feels really redundant to me. The system's there, let's use it and be consistent.
Comment #162
Wim LeersExactly. #157 asked to document the reasoning for going in this direction, and #158 updated the IS to document it, but doesn't actually document the reasoning.
Comment #163
Wim LeersIn other words, I don't understand the sudden change of direction at #150:
Comment #164
e0ipsoI may have contributed to this idea to make it an RPC service instead of shoehorning a REST resource during DrupalCon.
My points are:
Additionally, AFAIK there is nothing that prevents us from having these routes to negotiate the response format/encoding according to the client's request.
I understand that there is a contradicting point of view saying that we're not shoehorning but reusing code. I'm by no means a purist of REST, but it feels that it contradicts just enough of the principles to go through the route approach.
Comment #165
dawehnerI'm sorry wim, I should have described in more details what we discussed, who we was and why we thought it was a good idea.
As far as I remember the following people have been part of the discussion: Edward Faulkner, crell, e0ipso, fubhy, tedbow
What was discussed:
See previous comment
Well, just changing something for the sake of it is obviously not the right thing to do. On the other hand we had good reasons to do so, much like I changed the direction of the implementation completely a month ago.
Comment #166
Wim LeersWhile this is true, it does mean duplicating a fair bit of the logic that REST already has.
Well, by that same reasoning,
HTTP GET /node/1?_format=json
is also stateful, because that also includes a session cookie, which requires server-side stored state, which is the statefulness you refer to. This does not make that worse. It doesn't even change it in any way. The only way to "fix" this, would be to bring something like OAuth to core. We'll still need the ability to trigger a session through JS though, for decoupled sites.All of your points make sense :) But there is one thing you didn't address, which is the most important one, and the reason I added the
tag to this issue:Comment #167
Wim LeersTo be clear: I'm fine with this approach, but then it'll need to address the security concern, and it will need to support formats other than JSON (@swentel cited a real-world need for it in #161).
Comment #168
e0ipsoI think we agree on almost everything, although I want to clarify that when you request
HTTP GET /node/1?_format=json
with the auth details (maybe cookie) for user/X you'll get the same every time (edge cases provided). Not sure you can say the same about the login item in the this new resource.+1. I believe that this is cool new feature (decoupling the login system from the Drupal form), but a new feature after all. With all the good and bad things that that implies.
Comment #169
Wim LeersRE: want to clarify that […] same every time -> you're totally right!
RE: I think we agree on almost everything -> indeed :)
Comment #170
Wim LeersJust one more thing: I think both approaches can work, but the downside of this new direction is precisely the upside of the previous direction: the ability to log in using something else than a form had to be specifically enabled, which means that the previous approach was far less security-sensitive.
That is why I still somewhat prefer the previous approach. The "proper" way to do things will always be something like OAuth2, so it's fine to have this guaranteed imperfectness in an imperfect vessel in my humble opinion: as a
@RestResource
plugin.Comment #171
dawehnerWell, we can also just go back to the REST resource and add the field access checking, see
When you have your own authentication resource, and when you have your own authentication mechanism you don't need this particular controller. It is a workaround for people not using something like oauth etc. in the first place.
Comment #172
Crell CreditAttribution: Crell at Platform.sh commentedMaking it a rest.module resource creates a hard dependency on rest.module for anyone who wants to get a session cookie through non-form. We saw no value in that dependency, when modules like Restful, Relaxed, and Services do not (currently) build off of rest.module. Any random client-side JS that needs to make a call, too, may need to get a cokie but may not necessarily be using rest.module.
We also included, if I recall correctly, Alex Pott and Peter Wolanin (from the sec team). They didn't have an issue with the extra resource exposed directly from user.module: It needs flood control, but login/logout can't be CSRFed anyway. It doesn't appreciably increase the attack surface area as long as we have flood control.
It's also simply fewer moving parts, which all else equal is a win.
Comment #173
andypostAnother interesting point is access to this routes - looks user needs to be anonymous to login
Looks that allows to check cookies for valid auth...
How that fits into other authentication providers?
Comment #174
dawehnerGood question! On the other hand other authentication providers require some form of passed in parameter, so the caller is responsible for that. I think its a limit which is okay to accept. Another alternative could be to explicit call the cookie authentication provider again. On the other hand it might be useful as a tool for itself to know whether your passed authentication mechanism worked, no matter whether its cookie or base auth or whatever.
Given the last point, I think keep it is fine.
Comment #175
Wim Leers#172: Thanks, that addresses my security concerns!
All that remains then, is test coverage similar to the one we have for the login form, to prove flood control is working adequately.
Comment #176
Wim Leers… and support for other formats, of course.
Comment #177
dawehnerWe already have a test for that:
Comment #178
Wim LeersI think that's less comprehensive tests than we have for the login form's flood control:
\Drupal\user\Tests\UserLoginTest::testGlobalLoginFloodControl()
\Drupal\user\Tests\UserLoginTest::testPerUserLoginFloodControl()
Let's duplicate that test for this new form-less login mechanism. And let's add bidirectional
@see
s.Comment #179
dawehnerFair point.
Comment #180
tedbowWorking on multiple formats tests.
Comment #181
tedbowStarting working on extra formats. Then realized we would probably want the Serialization module if we are going to handle other formats such as XML. Is that right?
If so would it make sense only to support JSON as a starting point so we didn't have to make the User module depend on Serialization. Seems like we would want to had a new dependency for every single Drupal. We could always add more formats later.
Or should be we just through a 406 format unsupported error if Serialization is not enabled? Or should there a route provided that make route dynamically if serialization is enabled?
Comment #182
Wim LeersWe could let it default to JSON, but upon enabling the serialization module, we could have the responses actually use the
@serializer
service. That'd make it automagically support all available formats. Alternatively, we could let the user module only add those routes if theserialization
module is installed. But that meansif ($this->moduleHandler->moduleExists())
, which isIn any case, big -1 on explicitly calling
JsonEncoder
&XmlEncoder
. That's still not supporting any formats anyone may add then.Comment #183
tedbowOk assigning back to myself after @Wim Leers comment and chat with @e0ipso
Comment #184
dawehnerI'm actually also in favour of this really simple idea.
But in general, let's not overdesign the system. There aren't many usecases we neglect when we don't support XML.
Comment #185
Crell CreditAttribution: Crell as a volunteer commentedI agree, XML seems an edge case here. Most browser-based clients are going to want JSON, and they're the ones that will want a cookie. XML-based clients are more likely, I think, to be using HTTP Basic or OAuth anyway.
Comment #186
dawehner@crell
Oh that is a really strong point!
Comment #187
Wim LeersXML is just an example. I don't care about XML in particular. All I care about is that we don't hardcode JSON, that we are not tied to JSON in a frustrating way.
Comment #188
tedbowOk here is another patch.
It does a few things
The part I was a bit ify about was
This is allows the constructor to be the same because in either case it gets an instance of \Symfony\Component\Serializer\Serializer and array of acceptable formats
Comment #189
tedbowThis is probably not needed anymore I didn't notice this from previous patch.
I left out the $serializer @param from phpdoc
Will need to add cases from both serialization on/ff and xml format to loop through
Comment #190
tedbowI now see what ExceptionJsonSubscriber is being used for. It seems now we would need to make a ExceptionHTTPSubscriber that would handle all formats enabled.
Comment #191
Wim LeersGreat step forward! :) Half of my review is nits, half of it is proposals to make it simpler both code-wise and for the developer interacting with it.
This looks … exactly like the
on403()
,on404()
,on405()
andon406()
methods that already exist in this class. This is really ridiculous. But not the fault of this patch.I wanted to open a new issue, but the root cause here is not Drupal, but Symfony:
\Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase
.The class name suggests this is only for "login", but it's really used for 3 different things.
What about renaming this
UserAuthenticationController
orUserSessionController
?s/Serializer/serializer/
Does not match the class name.
Missing
$serializer
.Nit: does not match parameter order.
I think we can choose to only support XML if the
serialization
module is enabled. In which case we can choose to only support JSON here: JSON would then be the only format supported by default.Strange descriptions. "Controller to …" and "login/logout" instead of "log in/log out".
Look at other controllers for more consistent wording.
That's the things it always returns, but it may return more. We should list what else this may return, if the user has access to it.
Nit: two newlines, should be one.
Forgotten to be finished :)
Nit: s/Test/Tests/
These are inconsistent.
Also, why the mention of
http
? I think we could actually make these use the existing paths (/user/login
and/user/logout
), plus add/user/login_status
. That actually works fine if we specify_format
. We can then use a\Drupal\Core\Routing\RoutingEvents::ALTER
subscriber to specify the supported formats for these routes: a hardcoded set by default, and if theserialization
module is installed, specify all of the formats that supports.By doing that, these routes have the expected URLs.
Comment #192
tedbow@Wim Leers thanks for the review.
I agree with that! I started to do more work support XML and that looked like it was going be a bunch more work like creating ExceptionXmlSubscriber.
I will abandon that and work on only getting JSON to work without serialization module.
Will work on other issues.
Comment #193
tedbowThis patch address most of #191 plus so other code cleanup.
This is not included in the patch.
I started to make user/src/EventSubscriber/UserRouteAlterSubscriber.php but then realized it won't work in because we be adding that to user.services.yml and asking for @serializer which might not be there.
So this should be added to the serialization module? I was going to start doing that but I am quitting for the day. So thought I would ask for feedback before I added it.
I did hard-code _format: 'json' in user.routing.yml. So for now only json will work until above is handled.
I changed this so they all call \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber::setEventResponse
Comment #194
tedbowI forgot to mention that this new file was introduced. It is used to encode response for exceptions when using different formats.
It won't be called until the route alter is implemented.
Comment #195
tedbowadded \Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber to alter user routes if serialization module enabled
Comment #196
tedbowOk this is just a change to the test \Drupal\Tests\user\Functional\UserLoginHttpTest
It now tests:
Also changing issue title because these are no longer rest resources
Comment #197
tedbowUpdating issue summary because string messages from login/logout are not longer return in favor http response codes
Comment #198
Wim Leers#193:
Yes. The
serialization
module would enhance these routes inuser.module
with new capabilities if it is installed.How much does this really help?
In the end, we could just as well do
$event->getStatusCode()
.I think all this is out of scope to change here. It's too distracting. Let's create a separate issue for that and revert back to just adding the
on400()
method like we did before.See above.
#195:
This service name looks bizarre to me. C/p remnant? :)
Nit: s/array()/[]/
Nit: s/http/HTTP/
#196:
Excellent! :)
Just one detail:
You can inline this beautiful array for even more elegance :)
So IMO remaining tasks:
Comment #199
clemens.tolboomPlease update IS regarding the curl commands too :-) ... are they still using json?
Comment #200
Wim Leerstedbow pointed out on IRC we need this class for the formats that Drupal core does not yet support already (it currently only does HTML and JSON).
When the
serialization
module is installed, we for example also need to support XML.So, then this makes sense, because we do actually test that you can authenticate users using XML, which means we have XML HTTP 400 responses, which means we definitely do need this.
So given the above, we should try to get rid of this insanity.
This only makes sense for HTTP exceptions that need HTML responses. Because it makes sense for
\Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber
+\Drupal\Core\EventSubscriber\CustomPageExceptionHtmlSubscriber
.It does not make sense for XML/JSON/… because:
Therefore I think that it would make sense to override
\Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase::onException()
here, and duplicate some of its code, and then instead of letting it call one of a gazillionon$SOMETHING()
methods, just let it do what thesetEventResponse()
method in this class is doing.That would mean that we support all 4xx responses in one go. Much simpler.
Comment #201
dawehnerThis is looking quite great now!
For some sane level ideally we should have a on40x as method name which you can override if you want as a fallback for a more specific error. For example a 406 error could totally return value information about available formats.
Ideally we would check whether these routes are available. For example a module might remove them entirely
+1 for just supporting json
What about using
'user.http_login'
as flood name or so? Could be a constant on the class as well.Conceptually this is more of a 406 error, so let's leverage
\Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException
Nice!
Comment #202
tedbowActually I was wondering if we could just take this out because now that we are altering the routes to only allow formats supported by serialization.formats parameter then the routing should system should make sure it never gets this far with an unsupported format. Correct?
Comment #203
tstoecklerThis comment is weird. I get the point it's trying to make, but HTML requests are still transferred by HTTP. I still we can just say "are still the most common requests." (is the "still" really necessary, BTW?)
But huge props for adding a comment, this is one of those things that not many people do, but it really helps in making the code more self-documenting.
UserAuthenticationController::create()
is weird (as documented above) but it's great that we can encapsulate the weirdness in the factory instead of "actual" code (whatever that means). Great job! I was originally going to suggest that - in order to avoid this weirdness - we add those routes only if serialization module is enabled, but then we have a weird situation that we have a controller lying around in user module that is only usable when serialization is on, which is also weird and in comparison toUserAuthenticationController::create()
a hidden weirdness. So I think I am in favor of a visible weirdness. Another solution would be to add this route+controller to rest.module. We discussed above that this shouldn't be a resource, and I can see the arguments for that. But the route itself feels pretty rest-y. It's a one-off either way, it's a one-off in user.module now, so why not have a one-off in rest.module. And then we would avoid that dependency problem. Not sure though, also don't want to paint a bikeshed with this, I can totally live the current solution.Comment #204
tedbowUploading new patch will comment in few about changes
Comment #205
Wim Leers#203:
The argument is that if you want machines talking to your Drupal site using the Services, GraphQL, JSONAPI or whatever module, they can still use this… without also having to enable the
rest
module.#204:
Nit: must be 3rd singular.
s/check*/assert*/, which also means these need return values like other assertion methods.
Comment #206
tedbowRemoved this function as it was noted it is out of the scope of this issue. Revert on40* functions that were calling it.
Renamed service
fixed nit
Checked to see if routes actually exist
Added 1 flood control test and logic in controller. Need to add the other one
Add 2 new help functions to check responses. I felt it made the test more readable.
Comment #207
tedbow@Wim Leers
Changed the function name but I am just calling assertEquals in both these functions. assertEquals doesn't return a value.
Fixed
Comment #208
Wim Leers#203: the implication of is that we need to execute less code if we ensure that the HTML exception subscriber runs first, followed by such subscribers for other formats.
Yay! The patch is now so close! Many more nits now, and just a few substantial remarks.
This compares to:
So given that, I'd call this service
serialization.exception.default
and the classDefaultExceptionSubscriber
.Core's default says:
I'd make this in line with that, so:
Nit: inconsistent.
Nit: s/array()/[]/
See #200.2, that'd make all this go away.
Not just by this module: other modules can add more formats.
Nit: inconsistent.
Very elegant, very easy to understand :)
Without the verb.
Nit: 3rd person singular.
Yay! Now just the one other flood control test, and then we're done here!
Also: let's add
@see
s to the non-REST variants of these flood control tests.Comment #209
tedbowOk addressed issues in #208
Comment #210
tedbowNotes besides what was address from review in #208
This function is not need because of the new function floodControl which throws exceptions if request should be blocked.
This is new function to enforce flood control. I attempted to implement the logic that is found in UserLoginForm to enforce flood control.
I wasn't sure what to put in the error message here. This were basically copied from UserLoginForm but I removed the links.
Before this patch the message was just "Blocked"
$user->pass_raw was wrong here. Password is actually stored in $user->passRaw
New function here. The other testPerUserLoginFloodControl functions don't actually check for different values for uid_only but it seems it makes sense.
It is only really testing different message. I am not sure there is a way to fake request ip to actually test different behavior if uid_only is set to true.
but that is probably a separate issue since basic_auth and user login form don't test it either.
Comment #212
tedbowOk I see the failures relate to DefaultExceptionSubscriber::onException.
Comment #213
tedbowFixed \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber::onException
It was causing problems in other existing REST tests not the new tests here.
Comment #215
tedbowOk so the problem is the new exception handler \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber is handling exceptions that use to be handled by \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber
Which says
So other tests are expected it's behavoir. Not sure how to handle this. We don't want to set the priority of the new exception handler lower than the "Last-chance handler" but we do want it handle our login requests
Comment #216
tedbowLooking into this further I started to think about the idea that with this patch our new exception handler \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber
is going to be handling all exceptions for rest requests. That seems like a big change for this patch.
We could add back the on4xx functions and remove onException as it was in #207. That would stop the tests from failing but it still feels pretty fragile.
From my understanding that would mean that any 4xx errors that we have on4xx functions that happen via rest calls would be handled by our new handler. It is just that it behaves so similarly to the existing \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber that it doesn't cause any tests to fail. But it could be presumably be sending back slightly different output that the tests just aren't checking for.
The other option would be to check the route and only handle routes that are altered in \Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber::onRoutingAlterAddFormats
But I don't see any other examples of having exception handlers only handle certain routes. So I am not sure that is a great idea.
Comment #217
dawehnerI'm wondering the same. Does it really make sense to have a one of implementation of the exception handling, when you could just do the same handing inside the route itself. So what about returning dedicated responses for different kind of exceptions?
Comment #218
Wim Leers#209: Great! Can you do this with the
-M
flag next time, so that renames/moves are clearer?Also, YAY, this means this is now complete in terms of test coverage!
#216:
I agree it's pretty fragile, but it's how this was designed back in #2323759: Modularize kernel exception handling. I was hopeful we could make it both simpler and more robust in this issue for this exception subscriber, but apparently we can't. So, let's indeed add back the
on4xx
functions as it was before. It's not up to this issue to improve exception handling.Perhaps that even means adding a
ExceptionXmlSubscriber
, which would be in line with the existing pattern, where we haveExceptionJsonSubscriber
andExceptionHalJsonSubscriber
: every module adding a new format is responsible for adding an exception subscriber that implements every possibleon$SOMETHING()
method. This is brittle, but it's existing brittleness, so not up to this issue to solve.IMO this patch is RTBC once this last test failure is resolved. It looks great now.
Comment #219
dawehnerDoes it really make sense for this to be a POST request rather than a GET request?
Comment #220
tedbowOk I added back the on40x functions.
Makes sense to me changed user.login_status.http to GET.
I didn't add this because it seems REST is already handling XML exceptions without this so it seems unrelated to this issue.
Comment #221
Wim LeersHurray! :)
(Also, great catch @dawehner in #219!)
Comment #222
dawehnerIt looks good for me now
Comment #223
tedbowComment #224
tedbowComment #225
tedbowComment #226
Wim LeersIS clarifications.
Comment #227
dawehnerAdapted the CR as well.
Comment #228
Wim LeersGreat, thanks Daniel!
Comment #229
fgmI'm currently working on an APIfied login system (for Meteor) with similar issues, and there is a use case for which I did not found the expected results in the test suite of this patch. Are we in agreement about what should happen in this case ?
Comment #230
dawehner@fgm
Well given that code like Meteor can actually control the cookies of the HTTP requests I think, they could just switch the logged in user by switching between the used cookies, don't they?
Comment #231
Crell CreditAttribution: Crell at Platform.sh commentedI'd say trying to login with a request that carries a session cookie should be an error. Since the client can decide to send that cookie or not, as dawehner points out, there's no reason to actively that client out. That's more stateful than we have any right to be. An anon request for a login session works, an auth request for a login session is 400, everything else just works normally.
Comment #232
Wim Leers#229: Great question!
This is what happens because of those
_user_is_logged_in
requirements.Comment #233
Wim LeersComment #234
Crell CreditAttribution: Crell at Platform.sh commentedOh good, then the code is already doing what we want. :-) Is it worth adding a test for that as fgm suggests?
Comment #235
Wim LeersWe could, but it's not exactly essential to this issue/patch. Seems fine for a follow-up though.
Comment #236
Wim LeersNote: this blocks #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources, see #61 there.
Comment #237
Crell CreditAttribution: Crell at Platform.sh commentedI'm still good with the RTBC here, FTR.
Comment #239
Wim LeersThis failed due to #2735235: BrowserTestBase standards cleanup being temporarily committed. Back to RTBC.
Comment #241
dawehnerLet's fix stuff but also improve the test coverage a bit by checking 'roles' and 'uid' as part of the result.
Comment #242
Wim LeersMostly small nitpick improvements, but also this:
Thanks to granting extra permission, you can now test the returned values for those fields that not every user will get to see, which uncovered that bug. Nice work!
Comment #243
alexpottApart from for the regular UI - right? I think this should be improved to make the distinction between this and
UserController
Why is this message translated but none of the others are.
/aside I really dislike priorities - so difficult to know if this is the right number or not.
Comment #244
dawehnerComment #245
Wim Leers+100000000000
We already made this mistake with module weights and asset (library) weights. Now we imported Symfony's mistake. What we need is
before: X
andafter: Y
relationships.#244:
This makes a ton of sense. Nice additional improvement!
Comment #246
alexpottCommitted da16ae5 and pushed to 8.2.x. Thanks!
Fixed on commit - the description in #244 still could be said about UserController
Comment #248
klausiThe user.logout.http route looks a bit vulnerable to CSRF exploits. Yes, the standard logout route is vulnerable too (see #144538: User logout is vulnerable to CSRF), but we should not add more CSRF vulnerabilities to Drupal core while we are at it :)
I guess we can leave this as fixed and just add a task to the other issue to fix both vulnerabilities?
Comment #249
dawehner@klausi
Does this mean we need a dedicated route in core/user itself to fetch a valid CSRF token, much like rest module provides one?
Comment #250
alexpottWell the login returns a crsf token... hmmm and actually I should have picked this up in review - it uses an identifier of 'rest' which seems odd. Perhaps we should revert - change this to user and add crsf protected to the user.logout.http route?
Comment #252
alexpottGiven @klausi's security concern in #248 and the 'rest' pollution in the user module I've reverted this so we can have further discussion and come up with a solution.
Comment #253
Wim LeersOh wow. Thanks klausi!
(Of course, it's easy to miss this if we look at the existing log in/out flow as The Gold Standard. I don't understand why the other issue is not major though.)
Comment #254
klausi$this->csrfToken->get('rest');
is important because then clients have the REST CSRF token immediately at hand and don't need to call the server again for the token.The CSRF vulnerability for this particular route is mitigated by the fact that an attack needs to be able to set the request format to JSON (e.g. with HTTP Accept header). But I think we support "?_format=json" in the URL query part, so should be easy, right?
Comment #255
klausiSorry, did not mean to change the status.
Comment #256
Wim LeersCorrect.
So, we should have a test proving that the current patch contains a CSRF vulnerability. And then we should fix it.
Comment #257
tedbowOk, I will work on the test and then the fix
Comment #258
tedbow@Wim Leers from IRC
We wanted to get @klausi and @dawehner feedback on this.
So I am working on patch but thought would just write out the plan if we take Wim's suggestion above.
From my understanding the reason to pick "X-CSRF-Token request header" is that the session RPC routes should behave like REST because clients are going to be using them with REST and to have to provide the csrf in different manners in say login vs GET a node would be bad DX.
Questions I have:
Why did REST module not use _csrf_token in the first place?
Should we cover _access_rest_csrf with backward compatibility but change all rest routes to use _csrf_token?
Should we have _csrf_token provide access whether the token is provide via "?token=TOKEN" or " X-CSRF-Token"?
Should we deprecate providing the token in 1 of the ways above and use the other globally?
It just seems like if we are going to move logic in \Drupal\rest\Access\CSRFAccessCheck out of the REST module then it would be weird DX to have 2 ways of specify a route needs to have a csrf access control and then 2 ways to actually provide the token.
Thoughts?
Comment #259
tedbowUn-assigning from myself for feedback on above
Comment #260
Wim LeersA quick archeology round says that #1891052: Protect write operations from CRSF by requiring an token (when using cookie/session-based authentication) introduced REST's
_access_rest_csrf
, and it didn't once mention_csrf_token
despite that being introduced about 10 months earlier, in #1798296: Integrate CSRF link token directly into routing system. Only Crell in #1891052-6: Protect write operations from CRSF by requiring an token (when using cookie/session-based authentication) linked to the latter, but didn't say why it should be different.IMO we can't change all REST routes to use
_csrf_token
. That would also trigger\Drupal\Core\Access\RouteProcessorCsrf::processOutbound()
, which means it'd cause?token=CSRF_TOKEN_HERE
query arguments on REST resource POST/PATCH/DELETE URLs.I think we need to move REST's
_access_csrf_rest
to core and call it something like_access_csrf_request_header
. OTOH, that means we'd need to also move therest.csrftoken
route out of the REST module into Drupal core'ssystem
module, otherwise you wouldn't be able to even fetch the necessary CSRF token!It'd be great if @klausi and @Crell can provide insight into their reasoning, because the two of them landed #1891052: Protect write operations from CRSF by requiring an token (when using cookie/session-based authentication).
Comment #261
klausiThe reasoning for the X-CSRF-Token header instead of the query string was that Django already did the same: https://docs.djangoproject.com/en/1.9/ref/csrf/#ajax
"... you have to remember to pass the CSRF token in as POST data with every POST request. For this reason, there is an alternative method: on each XMLHttpRequest, set a custom X-CSRFToken header to the value of the CSRF token. This is often easier, because many JavaScript frameworks provide hooks that allow headers to be set on every request."
So I think it is fine to continue with this approach and move the CSRF checker out of rest module to user module or core itself.
Comment #262
Crell CreditAttribution: Crell at Platform.sh commentedMy own mental archaeology (warning: "soft science") is that it also kept the token out of the URL, which allowed it to be cached better. I've generally favored avoiding user-identifying tokens in the URL for security and caching reasons. That other frameworks were already doing so, as klausi notes, made it an easier argument.
As an aside, I believe that our current use of tokens on GET-that-does-an-action is fundamentally broken to begin with, since we're using a GET query to perform a state change. (Eg, flag/unflag an entity.) Those should instead be built confirm-form-first, so that the link by default goes to a non-JS-using confirmation form. Then they can be marked with a data attribute, class, or similar to be enhanced to a POST request by Javascript, which makes use of a proper token sent via the HEAD or a Header or similar. Fixing that is out of scope of this issue, certainly, but we shouldn't move more things to that broken approach.
I'm completely on board with moving that functionality from rest.module to core somewhere.
Comment #263
tedbowHere is patch that changes \Drupal\Core\Access\CsrfAccessCheck which currently applies to routes that have _csrf_token: 'TRUE' and expect the token to be in "?token="
with this patch it will handle the token either in "?token=" or in the X-CSRF-Token header.
It also applies the _csrf_token to the http user logout route and adds \Drupal\Tests\user\Functional\UserLoginHttpTest::testLogoutCsrfProtection to test to make sure the protection works.
It took the logic from \Drupal\rest\Access\CSRFAccessCheck but left REST routes using _access_rest_csrf which use REST's version of CSRFAccessCheck
It didn't just move _access_rest_csrf out of rest and into core because I don't think it makes sense to have 2 ways to specify a route needs a csrf token.
I would have removed _access_rest_csrf altogether but that would probably break a bunch of stuff I am not really knowledgeable on.
I don't think it makes sense to use both _csrf_token and _access_rest_csrf(even if renamed to _access_csrf) outside of REST because from a DX perspective knowing when to use which would be very confusing.
Comment #264
dawehnerWhat about making it even more explicit and call the function something like queryParameterAccess or so, to clearly document where the token is? I think strictly speaking the path doesn't contain the query parameters.
Given that we handle options request before routing, I was wondering whether we maybe should just use
!$request->isMethodSafe()
here?Before we returned AccessResult::forbidden in the case no query parameter was given. Should we not return the same when no header/query parameter have been set?
Comment #265
Wim Leers#261: thanks!
#262: thanks! (BTW, anything that uses a CSRF token is not cacheable anyway, so the caching argument does not hold, but otherwise agreed.)
#264:
_csrf_token
) support both URL query argument and request header is that even those URLs where you specifically do NOT want to have the token in the URL, as Crell has explained in #262, if you now do something likeUrl::fromRoute('user.logout.http')->toString()
will result in/user/logout?token=TOKEN
. BecauseRouteProcessorCSRF
will not know the difference. We could work around that by lettingRouteProcessorCsrf
only set the query argument in case the route allows the GET method.But even then does this cause a significant change. How is the REST client supposed to generate this CSRF token? Until now, he was supposed to request a CSRF token from
/rest/session/token
, and that token he could use for everything. But in this case, that doesn't hold: withCsrfAccessCheck
, the CSRF token is based on the URL's path:$this->csrfToken->validate($request->query->get('token'), $path)
. How is a REST client supposed to do that? I see thatUserAuthenticationController::login()
now receives$response_data['csrf_token'] = $this->csrfToken->get('rest');
. But how is that token able to even allow forCsrfAccessCheck
to allow access? Oh, I see, becauseCsrfAccessCheck
's request header option is weaker CSRF protection than than the query argument option: instead of the CSRF token only being valid for that path, it's valid for ALL requests!Not to mention then that this CSRF token that you get when logging in is actually the same as the one you'd retrieve via
/rest/session/token
. Which is fine, but then we need test coverage to prove they're the same, so that they remain the same, so that we don't regress in that way in the future.Because of these vastly different methods of operating and levels of security, I would very much like to see them be separate PHP classes/services, to avoid confusion. At which point we may as well just have
_csrf_token
as it is today and then a new_csrf_request_token
.Or, much simpler than all of this, and perfectly in line with what #144538: User logout is vulnerable to CSRF is doing: just have
UserAuthenticationController::login()
return a$response_data['logout_url']
. That logout URL would use_csrf_token
(just like #144538) and look like/user/logout?token=TOKEN
. That makes the logout URL actually more secure and is perhaps even better because:s/http/HTTP/
s/ header/request header/
(Note the extraneous leading space.)
This logic seems off. I think we need to have these if-tests outside of these "validate CSRF token" methods, so we don't have to use
AccessResult::neutral()
as the "oh, oops, that didn't apply here" solution.s/csrf/CSRF/
Comment #266
dawehnerI agre with you. Its something we can easily avoid, so let's do that. It also makes the code easier to follow etc.
Comment #267
neclimdulThis came up a couple of times in IRC this morning so posting it here. For clarity of discussion and because this thread is a monster I think it makes sense to fork the CSRF move/changes into a quick blocking issue. At least I hope its quick :). Either way it will make the discussion easier and reviews simpler.
Comment #268
tedbowOk. I had almost finished this patch when I saw discussion in IRC and @neclimdul's comment here. So I am uploading anyways so people can see what this code change would look like.
This patch does pretty much what Wim suggested here
But has UserAuthenticationController::login() return a $response_data['logout_token'] instead of full url.
Maybe it is moot point because:
Which seems like a good idea unless people love this approach I just took.
Comment #269
dawehnerIs there a reason we cannot call out to csrfToken->get instead as well? At least we should should drop this pointless command and rather explain why we are doing this here.
Comment #270
tedbowCreated child issue: #2753681: Move CSRF header token out of REST module so that user module can use it, as well as any contrib module
Postponing this issue till that 1 is resolved.
Comment #271
Wim LeersComment #272
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commented+1
P.s fixed a typo in description because it bugggggged me :P
Comment #273
Wim Leers#2753681: Move CSRF header token out of REST module so that user module can use it, as well as any contrib module landed, so this is now unblocked!
Comment #274
Wim LeersThe last patch here is at #268, which dates back to June 16. On June 19, #2308745: Remove rest.settings.yml, use rest_resource config entities landed, which also introduced a
\Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber
class. The only differences are in comments (very minor differences) and the fact that that one added aon422()
method, and this one adds aon429()
method.As such, this patch is identical to that in #268, except that we don't need to create
\Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber
from scratch anymore, we just need to add a few lines for thaton429()
method.I didn't make any changes, I only rebased, to expedite this issue.
Comment #275
Wim LeersI added the #256, where I wrote:
issue tag in@tedbow added tests for that in #263.
Comment #276
Wim LeersThe only remaining remark is the one in #269, which is just a nitpick. I think @dawehner is basically saying . But I think that it's actually the right thing to do, because
\Drupal\Core\Access\CsrfAccessCheck::access()
indeed does this:Note that this already was committed once, back on June 14, in #246. It was reverted because this RPC "log out" route had the same CSRF vulnerability as the "HTML log out" route, for which we have #144538: User logout is vulnerable to CSRF. This one now is superior to the one we've all been using for years, because it has CSRF protection. Everything else is still the same, it's just the CSRF-related changes that are new since then.
So, with that, back to RTBC!
Comment #277
alexpottSecond time lucky - @klausi's security concern has been addressed.
Committed 18feefc and pushed to 8.2.x. Thanks!
Comment #278
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commentedOmg yay!
Thanks everyone!
Comment #280
Wim Leers\o/
Comment #281
tstoecklerSeems #2278383: Create an injectible service for drupal_set_message() was committed together with this one here.
Comment #283
Wim LeersThanks for the quick fix!
Comment #286
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commentedWas there a reason we chose to return a single string/integer from user/login_status?
Also the release notes / CR don't actually depict what you should do.
For example theres no logout token in the logout example.
Comment #287
kylebrowning CreditAttribution: kylebrowning as a volunteer and at Acquia commentedReferring to this.
https://www.drupal.org/node/2720655
Comment #288
dawehnerGreat, someone trying out this new feature :)
What else would you expect to be there?
The great part about CRs is that everyone can fix them :)
Comment #290
Wim LeersI discovered a small bug in the code: #2820888: Cookie authentication: the user.login.http route never supports the 'hal_json' format or some other format, depending on module order.
Comment #291
ruloweb CreditAttribution: ruloweb at Media.Monks commentedNow that #2753681: Move CSRF header token out of REST module so that user module can use it, as well as any contrib module is solved, will _csrf_request_header_token be applied for logout.http in 8.2.x or 8.3.x ?
We also need to add a few endpoints for reset user password, at least one for start the process, which will send the reset link by email, and another one that accept that token, logs the user in, and allow him to edit his password without the current password field. I think this is a big one so maybe we could create a new issue?
Comment #292
Wim Leers#291: Why would we need to change that? It already has
_csrf_token: 'TRUE'
, which requires the CSRF token to be sent as a URL query argument. That's fine too.Yes, please create a new feature request issue for that.
Comment #293
damiankloip CreditAttribution: damiankloip commentedThis is under the rest module but not one line of code in the rest module is touched here.
Comment #294
ruloweb CreditAttribution: ruloweb at Media.Monks commented@Wim Leers it is fine but I think is not the best approach, maybe is better to send all tokens in the same way, as HTTP Header. Anyway I understand that if it is changed, it will be BC update.
Comment #295
Wim LeersA follow-up to this was filed: #2904451: Allow to change password through RPC endpoints through the reset password workflow.