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:

  1. logging in/out is not CRUD, it's not stateless, and it's really RPC, not REST — see #164
  2. 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

Reference: https://www.drupal.org/core/beta-changes
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
Files: 
CommentFileSizeAuthor
#274 2403307-274.patch33.5 KBWim Leers
#268 2403307-268.patch36.46 KBtedbow
#268 interdiff-244-268.txt9.62 KBtedbow
#263 interdiff-2403307-244-263.txt15.36 KBtedbow
#263 2403307-263.patch44.56 KBtedbow
#244 interdiff.txt4.83 KBdawehner
#244 2403307-244.patch33.27 KBdawehner
#241 interdiff.txt14.07 KBdawehner
#241 2403307-241.patch33.09 KBdawehner
#220 interdiff-213-220.txt6.04 KBtedbow
#220 2403307-220.patch33.7 KBtedbow
#213 2403307-213.patch32.57 KBtedbow
#213 interdiff-2403307-209-213.txt2.02 KBtedbow
#209 interdiff-2403307-207-209.txt22.02 KBtedbow
#209 2403307-209.patch32.24 KBtedbow
#207 2403307-207.patch28.27 KBtedbow
#207 interdiff-204-207.txt6.41 KBtedbow
#204 2403307-204.patch28.25 KBtedbow
#204 interdiff-196-204.txt16.29 KBtedbow
#196 2403307-196.patch28.16 KBtedbow
#196 interdiff-2403307-195-196.txt12.68 KBtedbow
#195 2403307-195.patch26.21 KBtedbow
#195 interdiff-2403307-193-195.txt3.45 KBtedbow
#193 2403307-193.patch23.7 KBtedbow
#193 interdiff-2403307-188-193.txt28.57 KBtedbow
#188 interdiff-156-188.txt17.96 KBtedbow
#188 2403307-188.patch16.82 KBtedbow
#156 interdiff.txt3.56 KBdawehner
#156 2403307-156.patch15.61 KBdawehner

Comments

dawehner’s picture

Yeah I totally think we can add quite a bunch of stuff.

marthinal’s picture

Assigned: Unassigned » marthinal
Status: Needs review » Needs work

Ok so I'm going to work on it.

marthinal’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,909 pass(es). View

Creating the system connect resource. Missing session id. This is a first step :) We can receive a hal+json with the current user data.

Status: Needs review » Needs work

The last submitted patch, 3: 2403307-3.patch, failed testing.

Status: Needs work » Needs review

marthinal queued 3: 2403307-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2403307-3.patch, failed testing.

Status: Needs work » Needs review

marthinal queued 3: 2403307-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2403307-3.patch, failed testing.

almaudoh’s picture

Can we have this in 8.0.x?

marthinal’s picture

Version: 8.1.x-dev » 8.0.x-dev

@almaudoh Yes I think so.

marthinal’s picture

Status: Needs work » Needs review

marthinal queued 3: 2403307-3.patch for re-testing.

marthinal’s picture

Category: Task » Bug report
Priority: Normal » Major
Issue summary: View changes
Status: Needs review » Needs work

As discussed with @klausi in irc, we need a User Login Resource instead of a System Resource.

marthinal’s picture

Title: Create a System Resource. » Create a User Login Resource.
marthinal’s picture

Issue summary: View changes
marthinal’s picture

Issue summary: View changes
marthinal’s picture

Issue summary: View changes

As discussed with @klausi a 400 should be the status for a failed attempt. (http://stackoverflow.com/questions/8273757/should-a-failed-login-attempt...)

klausi’s picture

Title: Create a User Login Resource. » Create a User session cookie login resource
Category: Bug report » Feature request

More 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.

marthinal’s picture

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,961 pass(es). View

Using 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...

marthinal’s picture

FileSize
2.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,923 pass(es). View

Ok! Needs work but maybe we can try something like this.

Login op works perfectly using this json "{"op": "login","credentials": {"name":"marthinal","pass":"secret"}}"

Status: Needs review » Needs work

The last submitted patch, 21: 2403307-21.patch, failed testing.

Status: Needs work » Needs review

marthinal queued 21: 2403307-21.patch for re-testing.

marthinal’s picture

Adding 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..

clemens.tolboom’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,88 @@
    \ No newline at end of file
    

    No newline at end of file

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,88 @@
    +   *   The operation and username + pass for the login op.
    

    'username' should be 'name'

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,88 @@
    +   *   The HTTP response object
    

    Maybe a little more specific regarding the message strings?

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,88 @@
    +      return new ResourceResponse('Missing username.', 400, array());
    

    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?

clemens.tolboom’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,88 @@
    +        return $this->userLogout();;
    

    Double semicolon

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,88 @@
    +
    

    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$

marthinal’s picture

You 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.

clemens.tolboom’s picture

Hmmm 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?

marthinal’s picture

I 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.

marthinal’s picture

FileSize
53.58 KB
53.24 KB

Attached a couple of screenshots.

For the logout we don't need to add credentials.

marthinal’s picture

Status: Needs work » Needs review
FileSize
4.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,801 pass(es), 48 fail(s), and 22 exception(s). View

Added 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.

Status: Needs review » Needs work

The last submitted patch, 31: 2403307-31.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
4.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,856 pass(es). View
1.94 KB

Here I'm using the user.flood configuration from user module directly. As I said not sure about the best way... Needs review!

clemens.tolboom’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,109 @@
    +    if ($this->restLoginCookieFloodControl()) {
    +      return new ResourceResponse('Blocked.', 400, array());
    +    }
    +
    

    Why 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

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,109 @@
    +   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    

    You should throw the exception in function similar as \Drupal\contact\Controller\ContactController::contactFloodControl

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,109 @@
    +    if (!\Drupal::flood()->isAllowed('rest', $limit, $interval)) {
    +      return TRUE;
    +    }
    +    return FALSE;
    +  }
    

    This should be a onliner

    return !\Drupal::flood->...

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,109 @@
    \ No newline at end of file
    

    No newline at end of file

clemens.tolboom’s picture

+++ b/core/modules/rest/config/install/rest.settings.yml
@@ -44,8 +44,3 @@ resources:
-
-flood:
-  login_cookie:
-    limit: 2
-    interval: 3600

Why remove this from config? I liked it into the config. And I can image REST flood control is different from user flood control.

marthinal’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,259 pass(es). View
3.07 KB

As 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 :

flood:
  limit: 5
  interval: 3600

user:

uid_only: false
ip_limit: 50
ip_window: 3600
user_limit: 5
user_window: 21600
clemens.tolboom’s picture

Issue summary: View changes
FileSize
4.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,228 pass(es). View
2.15 KB

I can test this having basic auth enabled on the resource.

curl --user admin:admin --header "Content-type: application/json" --header "Accept: application/json" --request POST --data '{"op":"login"}' http://drupal.d8/user_login

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?

clemens.tolboom’s picture

Issue summary: View changes

I forgot to set permissions for anonymous. But that should be the default!

  • Why don't we listen on /user/login and /user/logout?
  • Why are the normal permissions not in place? I should not be able to login when I am already logged in and similar for logout.
  • Why isn't this a bug and critical? I could use Basic Auth I guess sending password on every request but should I?
clemens.tolboom’s picture

Weird 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

andypost’s picture

Version: 8.0.x-dev » 8.1.x-dev

Looks like next release material

klausi’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
    @@ -216,4 +216,18 @@ protected function getBaseRoute($canonical_path, $method) {
    +  /**
    +   * Throws an exception if the current user triggers flood control.
    +   *
    +   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    +   */
    +  protected function restFloodControl($config, $name) {
    

    Nope, that method does not throw an exception?

  2. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
    @@ -216,4 +216,18 @@ protected function getBaseRoute($canonical_path, $method) {
    +    if (!\Drupal::flood()->isAllowed($name, $limit, $interval)) {
    

    The flood service should be injected in plugins that need it, so use $this->flood instead.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,93 @@
    + * Definition of Drupal\rest\Plugin\rest\resource\UserLoginResource.
    

    Should be "Contains ..."

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,93 @@
    +/**
    + * Represents user login as resource.
    + *
    

    Can we be a bit more specific what this does? Like "Allows user logins by setting session cookies."

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,93 @@
    +   * @param array $operation
    +   *   The operation and username + pass for the login op.
    

    This should describe all possible keys in $operation. And the type is most like string[], correct? See https://www.drupal.org/node/1354#types

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,93 @@
    +    if ($uid = \Drupal::service('user.auth')->authenticate($credentials['name'], $credentials['pass'])) {
    

    authentication service should be injected.

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,93 @@
    +    \Drupal::flood()->register('rest.login_cookie', \Drupal::config('user.flood')->get('user_window'));
    

    Flood should be injected.

And we need a test case for this.

clemens.tolboom’s picture

FileSize
3.57 KB
5.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,773 pass(es). View

If fixed all but #2 and #6 from #42

The flood service should be injected
authentication service should be injected.

[edit]removed garbage[/edit]

clemens.tolboom’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review

I 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.

clemens.tolboom’s picture

Issue summary: View changes
FileSize
7.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,779 pass(es), 2 fail(s), and 0 exception(s). View
7.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,779 pass(es), 2 fail(s), and 0 exception(s). View

Attached 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.

curl \
  --header "X-CSRF-Token: `curl http://drupal.d8/rest/session/token`" \
  --verbose \
  --header "Content-type: application/json" --header "Accept: application/json" --request POST \
  --data '{"op":"login","credentials":{"name":"admin", "pass":"admin"}}' \
  http://drupal.d8/user_login \
  --header "PHP_AUTH_USER: admin" --header "PHP_AUTH_PW: admin"
clemens.tolboom’s picture

The last submitted patch, 45: create_a_user_session-2403307-45.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 45: create_a_user_session-2403307-45.patch, failed testing.

klausi’s picture

@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.

clemens.tolboom’s picture

I hoped I missed something :-)

Can you provide / update issue summary with a proper curl example which should work with current patch?

clemens.tolboom’s picture

Issue summary: View changes

To make this work patch is needed from #2419825: Make serialization_class optional
Updated summary.

clemens.tolboom’s picture

Assigned: marthinal » Unassigned
Status: Needs work » Needs review
FileSize
2.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 57,667 pass(es), 15,521 fail(s), and 7,683 exception(s). View

It'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

clemens.tolboom’s picture

FileSize
11.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,432 pass(es), 1 fail(s), and 1 exception(s). View

Wrong patch file.

clemens.tolboom’s picture

We still need #2 and #6 from #42 and

  1. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
    @@ -70,7 +76,9 @@ public static function create(ContainerInterface $container, array $configuratio
    +      // FIX ME
    +      \Drupal::flood()
    

    Fix this.

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,104 @@
    +      default:
    +        return new ResourceResponse('Unsupported op.', 400, array());
    +
    

    We lost the status op.

    This must be added.
    - you are logged in as 'user'.
    - you are not logged in.

  3. +++ b/core/modules/rest/src/Tests/UserTest.php
    @@ -0,0 +1,104 @@
    +    $payload = $this->getPayload('status');
    +    $this->httpRequest('user_login', 'POST', json_encode($payload), $this->defaultMimeType, $basic_auth);
    +    $this->assertResponse('200', 'You are not logged in.');
    

    This needs the two cases: logged in / not logged in.

  4. +++ b/core/modules/rest/src/Tests/UserTest.php
    @@ -0,0 +1,104 @@
    +    $payload = $this->getPayload('garbage');
    +    $this->httpRequest('user_login', 'POST', json_encode($payload), $this->defaultMimeType, $basic_auth);
    +    $this->assertResponse('400', 'Unsupported op.');
    

    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). '

  5. +++ b/core/modules/rest/src/Tests/UserTest.php
    @@ -0,0 +1,104 @@
    +    //$this->httpRequest('user_login', 'POST', json_encode($payload), $this->defaultMimeType);
    

    Uncomment this. My bad :-/

The last submitted patch, 52: pass_entity_type_into-1964034-35.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 53: create_a_user_session-2403307-52-incl-2419825.patch, failed testing.

clemens.tolboom’s picture

FileSize
7.12 KB

Test fail as expected. Needs some eyes.

Attached the interdiff.

clemens.tolboom’s picture

Interdiff remarks

  1. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -59,7 +59,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    -        $class = $definition['serialization_class'];
    +        $class = isset($definition['serialization_class']) ? $definition['serialization_class'] : NULL;
             try {
    

    Not sure why this is needed? If left out it gives undefined index 'serialization_class'.

  2. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -77,7 +77,7 @@ protected function setUp() {
    -  protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
    +  protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL, $request_headers = []) {
    

    Needed extra headers. (To inject basic auth header)

  3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -162,6 +163,9 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
           '<hr />Code: ' . curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE) .
    +      '<hr />Request headers: ' . nl2br(print_r($curl_options[CURLOPT_HTTPHEADER], TRUE)) .
    +      '<hr />Extra headers: ' . nl2br(print_r($request_headers, TRUE)) .
    +      '<hr />Request body: ' . nl2br(print_r($body, TRUE)) .
           '<hr />Response headers: ' . nl2br(print_r($headers, TRUE)) .
    

    Logging was missing a lot of info. These lines did help much.

  4. +++ b/core/modules/rest/src/Tests/UserTest.php
    @@ -27,27 +27,78 @@ class UserTest extends RESTTestBase {
    +    $this->defaultAuth = array('basic_auth');
    

    Why do I need basic auth to make this work?

clemens.tolboom’s picture

Issue summary: View changes

Removed the unnecessary X-CSRF header. And noted the inclusion of #2419825-1: Make serialization_class optional

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Feature request => 8.1.x.

marthinal’s picture

Issue summary: View changes

Added Beta phase evaluation

marthinal’s picture

Version: 8.1.x-dev » 8.0.x-dev
droti’s picture

Is 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?

marthinal’s picture

Assigned: Unassigned » marthinal

@droti Yes, I can confirm that it works.

marthinal’s picture

Status: Needs work » Needs review
FileSize
19.66 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,856 pass(es), 1 fail(s), and 1 exception(s). View
12.65 KB

Adding 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.

Status: Needs review » Needs work

The last submitted patch, 65: 2403307-65.patch, failed testing.

The last submitted patch, 65: 2403307-65.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,257 pass(es). View

Patch needed a re-roll. This will not fix the failing test.

Where is $op === 'status'?

    $payload = $this->getPayload('status');
    $this->httpRequest('user_login', 'POST', json_encode($payload), $this->defaultMimeType, $basic_auth);
    $this->assertResponse('200', 'You are not logged in.');

On local I get same exception

InvalidArgumentException: Placeholders must have a trailing [] if they are to be expanded with an array of values. in Drupal\Core\Database\Connection->expandArguments() (line 701 of /Users/clemens/Sites/drupal/d8/www/core/lib/Drupal/Core/Database/Connection.php).

Status: Needs review » Needs work

The last submitted patch, 68: create_a_user_session-2403307-68.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
21.28 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,591 pass(es), 8 fail(s), and 1 exception(s). View
21.31 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,595 pass(es), 1 fail(s), and 1 exception(s). View

Previous patches had constructs like

  $this->assertResponse('200', 'You are not logged in.');

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.

session_destroy(): Trying to destroy uninitialized session Warning SessionManager.php 260 Drupal\Core\Session\SessionManager->destroy()

I've added two patches of which one skips the current user status.

The last submitted patch, 70: create_a_user_session-2403307-70.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 70: create_a_user_session-2403307-70.patch, failed testing.

clemens.tolboom’s picture

FileSize
21.59 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,606 pass(es), 8 fail(s), and 1 exception(s). View
21.6 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,629 pass(es), 2 fail(s), and 1 exception(s). View
2.77 KB
clemens.tolboom’s picture

Issue summary: View changes

@droti I've updated the curl commands which works sort of as they use the _format=json.

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,201 @@
    +    if ($this->userIsAuthenticated()) {
    +      throw new BadRequestHttpException('You need to logout first.');
    +    }
    +
    ...
    +    return \Drupal::currentUser()->isAuthenticated();
    

    Should be a direct call to

    \Drupal::currentUser()->isAuthenticated()
    

    Still this breaks unit test as service 'current_user' is not available

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,201 @@
    +    if (!\Drupal::currentUser()->isAuthenticated()) {
    +      throw new BadRequestHttpException('You cannot logout as you are not logged in.');
    +    }
    +
    

    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

   if (!in_array($method, array('GET', 'HEAD', 'OPTIONS', 'TRACE'))) {
      // GET the CSRF token first for writing requests.
      $token = $this->drupalGet('rest/session/token');
    }

shouldn't that only be done on cookie based requests?

clemens.tolboom’s picture

Issue summary: View changes
Status: Needs work » Needs review

As 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.

The last submitted patch, 75: create_a_user_session-2403307-75.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 75: create_a_user_session-2403307-75-bad-state-management.patch, failed testing.

klausi’s picture

  1. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
    @@ -8,6 +8,7 @@
    +use Drupal\Core\Flood\FloodInterface;
    

    Why do we add that to ResourceBase? It is only used in the user login resource, so it should be only injected there.

  2. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -60,9 +60,15 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    -        $class = $definition['serialization_class'];
    +        $class = isset($definition['serialization_class']) ? $definition['serialization_class'] : NULL;
             try {
    -          $unserialized = $serializer->deserialize($received, $class, $format, array('request_method' => $method));
    +          if ($class) {
    +            $unserialized = $serializer->deserialize($received, $class, $format, array('request_method' => $method));
    +          }
    +          // Avoid denormalization because we need to instantiate a class.
    +          else {
    +            $unserialized = $serializer->decode($received, $format, array('request_method' => $method));
    +          }
             }
    

    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."

  3. +++ b/core/modules/rest/src/Tests/UserTest.php
    @@ -0,0 +1,116 @@
    +class UserTest extends RESTTestBase {
    

    should be called UserLoginTest

  4. +++ b/core/modules/rest/src/Tests/UserTest.php
    @@ -0,0 +1,116 @@
    +    $this->defaultAuth = array('basic_auth');
    

    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.

The last submitted patch, 75: create_a_user_session-2403307-75.patch, failed testing.

The last submitted patch, 75: create_a_user_session-2403307-75-bad-state-management.patch, failed testing.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
7.38 KB
20.05 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,293 pass(es), 0 fail(s), and 1 exception(s). View

#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)

curl --header "Content-type: application/json" --header "Accept: application/json" --request POST \
  --user admin:admin \

request session for other user

  --data '{"op":"login","credentials":{"name":"other user", "pass":"other password"}}' \
  http://drupal.d8/user_login

core/modules/rest/src/Tests/RESTTestBase.php:66 does

    $this->defaultAuth = array('cookie');

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.

Status: Needs review » Needs work

The last submitted patch, 83: create_a_user_session-2403307-83.patch, failed testing.

The last submitted patch, 83: create_a_user_session-2403307-83.patch, failed testing.

andypost’s picture

clemens.tolboom’s picture

I don't get the Drupal\views_ui\Tests\DisplayTest errors. user/login failed.

Status: Needs work » Needs review
clemens.tolboom’s picture

Issue summary: View changes

@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.

Status: Needs review » Needs work

The last submitted patch, 83: create_a_user_session-2403307-83.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
20.27 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,951 pass(es). View
6.39 KB

#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.

clemens.tolboom’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -15,84 +18,89 @@
    -    $this->httpRequest('user_login', 'POST', json_encode($payload), $this->defaultMimeType, $basic_auth);
    -    $this->assertResponseAndText(400, 'No op found. Use: status, login, logout.');
    +    $this->httpRequest('user_login', 'POST', json_encode($payload), 'application/json');
    +    $this->assertResponse('400', 'No op found. Use: status, login, logout.');
    ...
    -  protected function assertResponseAndText($code, $text) {
    -    $this->assertResponse($code);
    -    $this->assertText($text);
    

    I added assertResponseAndText for a reason. See #70

  2. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -15,84 +18,89 @@
    -  protected function assertResponseAndText($code, $text) {
    -    $this->assertResponse($code);
    -    $this->assertText($text);
       }
     
    

    Please restore

marthinal’s picture

Status: Needs work » Needs review
FileSize
22.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,128 pass(es). View
6.25 KB

@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.

marthinal’s picture

FileSize
25.78 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,307 pass(es), 1 fail(s), and 0 exception(s). View
13.9 KB

Added 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.

marthinal’s picture

You can test using:

{"op": "password_reset", "reset_info": [{"name": "marthinal", "lang": "en"}]}

Status: Needs review » Needs work

The last submitted patch, 94: 2403307-94.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
25.77 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,312 pass(es). View
1.25 KB

Ok Let's try :

{"op": "password_reset", "reset_info": {"name": "marthinal", "lang": "en"}}

The last submitted patch, 94: 2403307-94.patch, failed testing.

neclimdul’s picture

FileSize
25.29 KB

Reroll, just chasing HEAD. No changes, just a use statement conflict in ResourceBase.

neclimdul’s picture

#80.2 #2419825: Make serialization_class optional should probably resolve this... which means this is probably postponed.

tyler.frankenstein’s picture

I'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.

$account = \Drupal::currentUser();
$response = new ResourceResponse(array(
  'currentUser' => array(
    'uid' => $account->id(),
    'roles' => $account->getRoles(),
    'name' => $account->getAccountName()
  ),
  'csrfToken' => 'abcdef' /* @TODO Get the new token here. */
));
return $response;

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.

{
  "uid": 1,
  "uri": "http://localhost/d8/user/1"
}

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.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
Assigned: marthinal » Unassigned

bumping to 8.1.x branch because this is a feature request. Unassigning as this issue hasn't been changed in a while.

Wim Leers’s picture

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marthinal’s picture

IMHO 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!

Status: Needs review » Needs work

The last submitted patch, 106: 2403307-106.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
27.28 KB

From #106

-    if (!$this->flood->isAllowed($name, $limit, $interval)) {
+    if ($this->flood->isAllowed($name, $limit, $interval)) {

This change makes sense because we return TRUE. Changing the boolean value in our unit tests... let's try again

marthinal’s picture

Issue tags: -Needs tests
marthinal’s picture

dawehner’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,294 @@
    +   * @param string[] $operation
    

    Yeah that is not really a list of strings, let's use array instead

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,294 @@
    +   *   [
    +   *     'op' => 'login', 'logout'
    +   *     'credentials' => [
    +   *       'name' => 'your-name',
    +   *       'pass' => 'your-password',
    +   *     ],
    +   *   ]
    

    Code should be wrapped in @code>/<code>@endcode

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,294 @@
    +
    +    if (array_key_exists('op', $operation)) {
    +      switch ($operation['op']) {
    +
    +        case 'login':
    +          if (!array_key_exists('credentials', $operation)) {
    +            $operation['credentials'] = [];
    +          }
    +          return $this->login($operation['credentials']);
    +
    

    Can't we use different URLs for that?

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,294 @@
    +        case 'status':
    +          return $this->status();
    

    This sounds like a GET url to be honest

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,294 @@
    +
    +    if (empty($credentials)) {
    +      throw new BadRequestHttpException('Missing credentials.');
    +    }
    +
    +    // Verify that the username is filled.
    +    if (!array_key_exists('name', $credentials)) {
    +      throw new BadRequestHttpException('Missing credentials.name.');
    +    }
    +    // Verify that the password is filled.
    +    if (!array_key_exists('pass', $credentials)) {
    +      throw new BadRequestHttpException('Missing credentials.pass.');
    +    }
    +
    +    // Flood control.
    +    if (!$this->restFloodControl($this->configFactory->get('user.flood'), 'rest.login_cookie')) {
    +      throw new BadRequestHttpException('Blocked.');
    +    }
    +
    +    // Verify that the user is not blocked.
    +    if ($this->userIsBlocked($credentials['name'])) {
    +      throw new BadRequestHttpException('The user has not been activated or is blocked.');
    +    }
    

    Just a suggestion: We would move all that into a guardInput method or have guardEmptyCredentials(); guardRequiredUsername() guardRequiredPassword() ...

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,294 @@
    +    if ($uid = \Drupal::service('user.auth')->authenticate($credentials['name'], $credentials['pass'])) {
    ...
    +      $user = User::load($uid);
    ...
    +        'csrf_token' => \Drupal::csrfToken()->get('rest'),
    

    Let's inject those services ...

  7. +++ b/core/modules/rest/src/Tests/ResourceTest.php
    @@ -117,8 +117,10 @@ public function testUriPaths() {
         foreach ($manager->getDefinitions() as $resource => $definition) {
    -      foreach ($definition['uri_paths'] as $key => $uri_path) {
    -        $this->assertFalse(strpos($uri_path, '//'), 'The resource URI path does not have duplicate slashes.');
    +      if (isset($definition['uri_paths'])) {
    +        foreach ($definition['uri_paths'] as $key => $uri_path) {
    +          $this->assertFalse(strpos($uri_path, '//'), 'The resource URI path does not have duplicate slashes.');
    +        }
    

    We could skip the isset by using $definition += ['uri_paths' => []]

dawehner’s picture

Working on that now and improve it fundamentally

dawehner’s picture

Here is some not working start to split up the different resources

Status: Needs review » Needs work

The last submitted patch, 113: 2403307-113.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
31.44 KB
9.44 KB

Here 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.

Status: Needs review » Needs work

The last submitted patch, 115: rest_user_session-2403307-115.patch, failed testing.

Crell’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,190 @@
    +      $container->get('logger.factory')->get('rest'),
    

    Is the REST logger not registered as its own service already? If not, it should be. Let's do that and simplify this line.

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,190 @@
    +    // Log in the user.
    +    if ($uid = \Drupal::service('user.auth')->authenticate($credentials['name'], $credentials['pass'])) {
    +      /** @var \Drupal\user\Entity\User $user */
    +      $user = User::load($uid);
    

    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.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,190 @@
    +      user_login_finalize($user);
    

    Why is this global function not given a wrapper method when user_is_blocked() is?

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,190 @@
    +      // Add some basics about the user's account.
    +      $response_data = [
    +        'current_user' => [
    +          'uid' => $user->id(),
    +          'roles' => $user->getRoles(),
    +          'name' => $user->getAccountName(),
    +        ],
    +        'csrf_token' => $this->csrfToken->get('rest'),
    +      ];
    +
    +      return new ResourceResponse($response_data, 200, []);
    

    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.

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,190 @@
    +  protected function userIsBlocked($name) {
    +    return user_is_blocked($name);
    +  }
    

    This makes me cry. :-( (Not the code here, the fact that user_is_blocked() is still a function and not a service method.)

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,190 @@
    +  /**
    +   * Checks for flooding.
    +   *
    +   * @param \Drupal\Core\Config\ImmutableConfig $config
    +   *   The flood control config object.
    +   * @param string $name
    +   *  The name of the event.
    +   *
    +   * @return bool
    +   *   TRUE if the user is allowed to proceed, FALSE otherwise.
    +   */
    +  protected function restFloodControl($config, $name) {
    

    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.

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,80 @@
    + * @RestResource(
    + *   id = "user_login_status",
    + *   label = @Translation("Watchdog database log"),
    + *   uri_paths = {
    + *     "canonical" = "/user/login/status"
    + *   }
    + * )
    + */
    +class UserLoginStatus extends ResourceBase {
    

    Copy-paste error on the label.

  8. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,80 @@
    +  public function get() {
    +    if ($this->currentUser->isAuthenticated()) {
    +      return new ResourceResponse('You are logged in.', 200, []);
    +    }
    +    return new ResourceResponse('You are not logged in.', 200, []);
    +  }
    

    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.

  9. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
    @@ -0,0 +1,31 @@
    +  public function post() {
    +    user_logout();
    +    return new ResourceResponse('You are logged out.', 200, []);
    +  }
    

    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.

  10. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,124 @@
    +    // Verify that the email or username is filled.
    +    if (!$name) {
    +      throw new BadRequestHttpException('Missing Email or Username.');
    +    }
    

    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...)

  11. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,124 @@
    +    if (!$account = $this->loadUserByNameOrEmail($name)) {
    +      // No success, the user does not exist.
    +      throw new BadRequestHttpException("Sorry, $name is not recognized as a user name or an e-mail address.");
    +    }
    

    $name comes from user input, and is thus unsafe to put in a string. This should be translated, at least for escaping purposes.

  12. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * Loads a user by name OR mail address.
    +   *
    +   * @param string $name
    +   *   The username or mail address, that should be loaded.
    +   *
    +   * @return \Drupal\user\UserInterface|null
    +   *   The loaded user or NULL.
    +   */
    +  protected function loadUserByNameOrEmail($name) {
    +    // Try to load by email.
    +    $accounts = $this->userStorage->loadByProperties(['mail' => $name, 'status' => '1']);
    +    if ($accounts) {
    +      return reset($accounts);
    +    }
    +
    +    // No success, try to load by name.
    +    $accounts = $this->userStorage->loadByProperties(['name' => $name, 'status' => '1']);
    +    if ($accounts) {
    +      return reset($accounts);
    +    }
    +  }
    

    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. :-(

  13. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -54,9 +54,15 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    -        $class = $definition['serialization_class'];
             try {
    -          $unserialized = $serializer->deserialize($received, $class, $format, array('request_method' => $method));
    +          if (array_key_exists('serialization_class', $definition)) {
    +            $unserialized = $serializer->deserialize($received, $definition['serialization_class'], $format, array('request_method' => $method));
    +          }
    +          // If the plugin does not specify a serialization class just decode the received data.
    +          // Example: received JSON is decoded into a PHP array.
    +          else {
    +            $unserialized = $serializer->decode($received, $format, array('request_method' => $method));
    +          }
             }
    

    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.

  14. +++ b/core/modules/rest/src/Tests/NodeTest.php
    @@ -175,7 +175,7 @@ public function testInvalidBundle() {
         // Make sure the response is "Bad Request".
         $this->assertResponse(400);
    -    $this->assertResponseBody('{"error":"\"bad_bundle_name\" is not a valid bundle type for denormalization."}');
    +    $this->assertResponseBody(NULL, '{"error":"\"bad_bundle_name\" is not a valid bundle type for denormalization."}');
       }
    

    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.

  15. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -422,7 +430,11 @@ protected function removeNodeFieldsForNonAdminUsers(NodeInterface $node) {
    -  protected function assertResponseBody($expected, $message = '', $group = 'REST Response') {
    +  protected function assertResponseBody($code = NULL, $expected, $message = '', $group = 'REST Response') {
    +    if ($code) {
    +      $this->assertResponse($code);
    +    }
    

    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.

  16. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -0,0 +1,102 @@
    +    $payload = [];
    +    $this->httpRequest('user_login', 'POST', json_encode($payload), 'application/json');
    +    $this->assertResponseBody('400', '{"error":"Missing credentials."}');
    

    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.)

  17. +++ b/core/modules/rest/tests/src/Unit/UserLoginResourceTest.php
    @@ -0,0 +1,239 @@
    +  /**
    +   * A reflection of self::$translation.
    +   *
    +   * @var \ReflectionClass
    +   */
    +  protected $reflection;
    

    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.

  18. +++ b/core/modules/rest/tests/src/Unit/UserLoginResourceTest.php
    @@ -0,0 +1,239 @@
    +    $user_auth_service = $this->getMock('Drupal\user\UserAuthInterface');
    

    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.

  19. +++ b/core/modules/rest/tests/src/Unit/UserLoginResourceTest.php
    @@ -0,0 +1,239 @@
    +
    +    $this->testClass = new UserLoginResource([], 'plugin_id', '', [], $this->logger, $this->config,  $this->flood, $this->userStorage);
    

    Test object. testClass implies it's a string name of a class. This is setting the object (not class) to be tested.

  20. +++ b/core/modules/rest/tests/src/Unit/UserLoginResourceTest.php
    @@ -0,0 +1,239 @@
    +    $this->testClassMock = $this->getMockBuilder('\Drupal\rest\Plugin\rest\resource\UserLoginResource')
    

    Ibid. Also, I don't understand why we need both of these...

  21. +++ b/core/modules/rest/tests/src/Unit/UserLoginResourceTest.php
    @@ -0,0 +1,239 @@
    +  /**
    +   * Gets a protected method from current class using reflection.
    +   *
    +   * @param string $method
    +   *   The requested method.
    +   *
    +   * @return mixed
    +   *   The protected method.
    +   */
    +  public function getProtectedMethod($method) {
    +    $method = $this->reflection->getMethod($method);
    +    $method->setAccessible(TRUE);
    +
    +    return $method;
    +  }
    

    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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
31.5 KB
13.68 KB

Thank 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.

Status: Needs review » Needs work

The last submitted patch, 118: 2403307-118.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +DrupalCampES
tedbow’s picture

Assigned: Unassigned » tedbow

Assigning 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

tedbow’s picture

Assigned: tedbow » Unassigned
FileSize
32.58 KB
15.95 KB

Ok 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.

Status: Needs review » Needs work

The last submitted patch, 122: 2403307-122.patch, failed testing.

Wim Leers’s picture

So that leaves #117.12 through #117.21 to still be addressed.

Here's another review.

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,196 @@
    + * @RestResource(
    + *   id = "user_login",
    + *   label = @Translation("User Login")
    + * )
    

    Why is no URI defined here?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,196 @@
    +   * The flood control mechanism.
    

    Nit: The flood service.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,196 @@
    +   * The Csrf Token Generator.
    

    Nit: CSRF token generator

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,196 @@
    +   * The User Authentication service.
    

    Nit: user authentication service

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,196 @@
    +   *   The flood control mechanism.
    ...
    +   *   The Csrf Token Generator.
    ...
    +   *   The User Authentication service.
    

    Same here

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,89 @@
    + *     "canonical" = "/user/login/status"
    
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
    @@ -0,0 +1,31 @@
    + *     "canonical" = "/rest/user/logout"
    
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,119 @@
    + *     "canonical" = "/rest/user/password/{name}/{langcode}"
    
    1. These URLs are inconsistent.
    2. These URLs are not restful.

    Wouldn't these make more sense?

    /session/login
    /session/status
    /session/logout
    /session/reset_password
    
  7. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -54,9 +54,15 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    +          // If the plugin does not specify a serialization class just decode the received data.
    

    Nit: 80 cols.

dawehner’s picture

FileSize
32.16 KB
21.06 KB

Here are a couple of improvements.

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.

Sure, conceptually for sure, but its a POST request, so this is not necessarily helpful ...

/session/login
/session/status
/session/logout
/session/reset_password

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.

Wim Leers’s picture

. its tricky because using /user/login conflicts with the existing route.

The 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?

Wim Leers’s picture

Discussed 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.)

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.48 KB
5.17 KB

Let's see what happens

Status: Needs review » Needs work

The last submitted patch, 128: 2403307-128.patch, failed testing.

Wim Leers’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
@@ -19,7 +19,10 @@
+ *     "https://www.drupal.org/link-relations/create" = "/user/login",

+++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
@@ -12,7 +12,7 @@
+ *     "https://www.drupal.org/link-relations/create" = "/user/logout",

+++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
@@ -18,8 +18,8 @@
+ *     "https://www.drupal.org/link-relations/create" = "/user/password/{name}/{langcode}",

These all have the same relation type. I wonder if that caused it.

dawehner’s picture

These all have the same relation type. I wonder if that caused it.

To 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:

  public function routes() {
    $collection = new RouteCollection();

    $create_path = isset($definition['uri_paths']['https://www.drupal.org/link-relations/create']) ? $definition['uri_paths']['https://www.drupal.org/link-relations/create'] : '/' . strtr($this->pluginId, ':', '/');
    foreach ($methods as $method) {
      switch ($method) {
        case 'POST':
          $route->setPath($create_path);
Wim Leers’s picture

Oh hm. That makes sense. Though I'm not sure \Drupal\rest\Plugin\ResourceBase::routes() actually makes sense.

dawehner’s picture

Though I'm not sure \Drupal\rest\Plugin\ResourceBase::routes() actually makes sense.

In general or for this usecase? It feels for me that those resources are not really typical REST resources to be honest.

Wim Leers’s picture

Good point.

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.25 KB
8.24 KB

Let's remove the unit tests for now. IMHO we are mostly connecting dots, so the value of those unit tests is quite minimal.

Wim Leers’s picture

Great 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.

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    

    "Resource" as filename suffix: once.

    No suffix: thrice.

    Core has EntityResource and DbLogResource.

    Let's make that consistent.

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    + * Allows user logs in by setting session cookies.
    ...
    +   * Responds to the user login POST requests and logs in a user.
    
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
    @@ -0,0 +1,39 @@
    +   * Logs out the user.
    

    Broken English. Fixed.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    + *   label = @Translation("User Login"),
    
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,89 @@
    + *   label = @Translation("User Login Status"),
    

    We only capitalize the first character. Fixed.

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    +   * @param array $credentials
    

    s/array/string[]/.

    Fixed.

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    +    // Verify that the username is filled.
    ...
    +    // Verify that the password is filled.
    ...
    +    // Flood control.
    ...
    +    // Verify that the user is not blocked.
    ...
    +    // Log in the user.
    

    Useless comments. Fixed.

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    +      /** @var \Drupal\user\Entity\User $user */
    

    Should typehint to interface. Fixed.

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    +      // Add some basics about the user's account.
    

    I think "basic metadata" would be better. Also, it's not "adding", it's sending. Fixed.

  8. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    +      $response = new ResourceResponse($response_data, 200, []);
    
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,89 @@
    +      $response = new ResourceResponse(self::LOGGED_IN, 200, []);
    ...
    +      $response = new ResourceResponse(self::LOGGED_OUT, 200, []);
    

    The second (200) and third ([]) parameter can be dropped. They make this look needlessly complex.

  9. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    +      return $response->addCacheableDependency($user);
    

    Like @dawehner said in #125, this makes conceptual sense, but since it's a POST request, it's rather pointless.

  10. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginResource.php
    @@ -0,0 +1,210 @@
    +    $this->flood->register('rest.login_cookie', $this->configFactory->get('user.flood')->get('user_window'));
    

    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.

  11. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,89 @@
    + * Provides a resource for database watchdog log entries.
    ...
    +   * Constructs a Drupal\rest\Plugin\ResourceBase object.
    
    +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +   * Constructs a Drupal\rest\Plugin\ResourceBase object.
    

    Copy/paste remnant :P Fixed.

  12. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,89 @@
    +  // Logged status constants.
    

    Missing proper documentation. Fixed.

  13. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,89 @@
    +  const LOGGED_IN = 'LOGGED_IN';
    +  const LOGGED_OUT = 'LOGGED_OUT';
    

    I'd argue this should be AUTHENTICATED or ANONYMOUS.

  14. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLoginStatus.php
    @@ -0,0 +1,89 @@
    +   * Response to user login status GET requests.
    

    s/Response/Responds/ Fixed.

  15. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
    @@ -0,0 +1,39 @@
    + *   label = @Translation("Logout"),
    

    Not consistent with the rest. Fixed.

  16. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
    @@ -0,0 +1,39 @@
    +   * Handles POST requests to the user logout path.
    

    Bad documentation string again. (Although documenting these is pretty damn silly indeed.)

    Fixed.

  17. +++ b/core/modules/rest/src/Plugin/rest/resource/UserLogout.php
    @@ -0,0 +1,39 @@
    +  public function post() {
    +    $this->userLogout();
    +    return new ResourceResponse(NULL, 204);
    +  }
    

    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.

  18. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    + * Provides a rest resource for resetting a user password.
    

    English can be improved. Fixed.

  19. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +   * Resets a user password using a POST request.
    

    Inconsistent wording compared to the other resources. Fixed.

  20. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +   *   The username or mail address, that should be resetted.
    

    Broken English. Fixed.

  21. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +   *   The langcode.
    

    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.

  22. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +   * @return \Drupal\rest\ResourceResponse::__construct
    

    Method, not class. Fixed.

  23. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +  public function post($name, $langcode = NULL) {
    

    I expected flood control here… but I see that \Drupal\user\Form\UserPasswordForm also doesn't do that. So, no matter.

  24. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +      // No success, the user does not exist.
    

    Useless comment. Fixed.

  25. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +      throw new BadRequestHttpException(new FormattableMarkup('Sorry, %name is not recognized as a user name or an e-mail address.', ['%name' => $name]));
    

    Let's use the same string as in \Drupal\user\Form\UserPasswordForm::validateForm(). Less translation work. Fixed.

  26. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +      throw new BadRequestHttpException('The email with the instructions was not sent as expected.');
    

    Broken English. Fixed.

  27. +++ b/core/modules/rest/src/Plugin/rest/resource/UserPasswordReset.php
    @@ -0,0 +1,139 @@
    +   * Conditionally create and send a notification email.
    

    Should be 3rd person singular. Fixed.

  28. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -54,9 +54,16 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    -        $class = $definition['serialization_class'];
             try {
    -          $unserialized = $serializer->deserialize($received, $class, $format, array('request_method' => $method));
    +          if (array_key_exists('serialization_class', $definition)) {
    +            $unserialized = $serializer->deserialize($received, $definition['serialization_class'], $format, array('request_method' => $method));
    +          }
    +          // If the plugin does not specify a serialization class just decode
    +          // the received data.
    +          // Example: received JSON is decoded into a PHP array.
    +          else {
    +            $unserialized = $serializer->decode($received, $format, array('request_method' => $method));
    +          }
    

    This is being done in #2419825: Make serialization_class optional. Let's land that one first, then this patch can become smaller.

  29. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -77,7 +77,7 @@ protected function setUp() {
        * @return string
        *   The content returned from the request.
        */
    -  protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
    +  protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL, $request_headers = []) {
    

    Docblock is not updated. NOT yet fixed.

  30. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -0,0 +1,110 @@
    +  /**
    +   * Test user session life cycle.
    +   */
    +  public function testLogin() {
    

    This is testing the common case, not the edge cases.

    We're missing test coverage for:

    1. several of the possible exceptions
    2. flood control
    3. password reset
  31. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -0,0 +1,110 @@
    +    // Enable the service.
    

    We're not enabling a service. NOT yet fixed.

  32. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -0,0 +1,110 @@
    +    $resource = ['type' => 'user_login', 'method' => 'POST'];
    +    $settings[$resource['type']][$resource['method']]['supported_formats'][] = $format;
    +    $settings[$resource['type']][$resource['method']]['supported_auth'] = $auth;
    +
    +    $resource = ['type' => 'user_login_status', 'method' => 'GET'];
    +    $settings[$resource['type']][$resource['method']]['supported_formats'][] = $format;
    +    $settings[$resource['type']][$resource['method']]['supported_auth'] = $auth;
    +
    +    $resource = ['type' => 'user_logout', 'method' => 'POST'];
    +    $settings[$resource['type']][$resource['method']]['supported_formats'][] = $format;
    +    $settings[$resource['type']][$resource['method']]['supported_auth'] = $auth;
    

    Rather than doing this, let's improve \Drupal\rest\Tests\RESTTestBase::enableService(), which would make this 10x easier to read.

  33. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -0,0 +1,110 @@
    +    $payload = [];
    

    I'd call this $request_body. That's also what it's called in RESTTestBase::httpRequest().

  34. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -0,0 +1,110 @@
    +    $url = Url::fromRoute('rest.user_login_status.GET.json');
    +    $url->setRouteParameter('_format', 'json');
    

    Why not use /user/login/status instead, like all the others here?

Wim Leers’s picture

So, in #136, I addressed everything except points:

  • 1
  • 8
  • 9
  • 10
  • 13
  • 16
  • 17
  • 28
  • 29
  • 30
  • 31
  • 32
  • 33
  • 34
Wim Leers’s picture

Title: Create a User session cookie login resource » [PP-1] Create a User session cookie login resource
Status: Needs review » Postponed
Issue tags: +Needs issue summary update, +Needs change record

Per #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.

Wim Leers’s picture

Title: [PP-1] Create a User session cookie login resource » [PP-1] REST resources for authenticated users: log in, check login status, log out, password reset

Better title: the old title didn't match the scope.

dawehner’s picture

FileSize
26.01 KB
10.11 KB

Thank you wim!

"Resource" as filename suffix: once.

No suffix: thrice.

Core has EntityResource and DbLogResource.

Let's make that consistent.

Well, one could come up with metrices, like all plugins, which don't suffix everything.

The second (200) and third ([]) parameter can be dropped. They make this look needlessly complex.

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.

Like @dawehner said in #125, this makes conceptual sense, but since it's a POST request, it's rather pointless.

Well, one could argue that people look out for example and copy it to GETrequests as well.

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.

There is one issue which extract the logic we have for users into a generic service ... if I could just find it.

I'd argue this should be AUTHENTICATED or ANONYMOUS.

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.

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.

Sure, let's put that onto the route level.

This is being done in #2419825: Make serialization_class optional. Let's land that one first, then this patch can become smaller.

Yeah for me though this is not a reason to wait on this issue.

I'd call this $request_body. That's also what it's called in RESTTestBase::httpRequest().

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.

Wim Leers’s picture

Status: Postponed » Needs work

Thanks for pushing this forward! :)

So, looking at the list in #137 and comparing to what @dawehner did in #140:

  • 1: fixed
  • 8: fixed
  • 9: being discussed
  • 10: todo
  • 13: dawehner argued against this, works for me
  • 16: was already fixed by me, I listed that by accident
  • 17: fixed
  • 28: dawehner argued against this, but the thing is that that issue has a test that this issue doesn't have. If we move the test from that issue into this issue's patch, then that's fine.
  • 29: fixed
  • 30: partially fixed, AFAICT flood control is not yet actually being tested
  • 31: fixed
  • 32: fixed
  • 33: fixed
  • 34: fixed

So that leaves just 9, 10, 28 and 30.


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.

Fair. So keep the 200, but remove the empty array?

Well, one could argue that people look out for example

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.

  1. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -73,9 +73,11 @@ protected function setUp() {
    +   * @return string The content returned from the request.
    +   * The content returned from the request.
    

    Nit.

  2. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -63,48 +49,83 @@ public function testLogin() {
    +    $account->block();
    +    $account->save();
    ...
    +    $account->activate();
    +    $account->save();
    

    Nit: can be chained.

  3. +++ b/core/modules/rest/src/Tests/UserLoginTest.php
    @@ -63,48 +49,83 @@ public function testLogin() {
    +    // Flooded.
    +    $request_body = ['name' => $name];
    +    $this->httpRequest('/user/login', 'POST', json_encode($request_body), 'application/json');
    +    $this->assertResponse(400);
    +    $this->assertResponseBody('{"error":"Missing credentials.pass."}');
    

    I don't see how this tests flooding?

    The flood detection looks like this:

    +    if (!$this->isFloodBlocked()) {
    +      throw new BadRequestHttpException('Blocked.');
    

    That error message is what we should be able to assert.

dawehner’s picture

+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -73,9 +73,11 @@ protected function setUp() {
+ * @return string The content returned from the request.
+ * The content returned from the request.
Nit.

One day someone needs to fix phpstorm to no longer do that.

Nit: can be chained.

Good point

I don't see how this tests flooding?

The flood detection looks like this:

+ if (!$this->isFloodBlocked()) {
+ throw new BadRequestHttpException('Blocked.');
That error message is what we should be able to assert.

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 :)

dawehner’s picture

Yeah, 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 controller resolver in RequestHandler to have conistent parameter ordering

Status: Needs review » Needs work

The last submitted patch, 143: 2403307-143.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.94 KB
5.99 KB

There we go

Status: Needs review » Needs work

The last submitted patch, 145: 2403307-145.patch, failed testing.

dawehner’s picture

Added a change record and adapted the issue summary

dawehner’s picture

Here is a short description of the last failure:

The problem

The $flood->register() call on the test process has 127.0.0.1 as IP all the time, see core/scripts/run-tests.sh:433.

When running on a apache vhost configuration, that looks like the following:

<VirtualHost d8.dev:80>
    DocumentRoot "/Users/dawehner/www/d8"
    ServerName d8.dev

and a /etc/hosts file that looks like:

127.0.0.1 d8.dev

apache sets $_SERVER['REMOTE_ADDR'] to 127.0.0.1

When you run the same under another configuration

listen: 80
DocumentRoot "/Users/dawehner/www"
<Directory "/Users/dawehner/www">
 </Directory>

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.78 KB
1.5 KB

Let's use actual HTTP requests ...

dawehner’s picture

Title: [PP-1] REST resources for authenticated users: log in, check login status, log out, password reset » [PP-1] REST resources for authenticated users: log in, check login status, log out

We 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.

dawehner’s picture

Title: [PP-1] REST resources for authenticated users: log in, check login status, log out » REST resources for authenticated users: log in, check login status, log out
FileSize
14.84 KB

Let's try

Status: Needs review » Needs work

The last submitted patch, 151: 2403307-151.patch, failed testing.

Crell’s picture

  1. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,181 @@
    +  public function login($credentials) {
    

    login() and logout() are the controller methods, so they should have docblocks that clarifies that.

  2. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,181 @@
    +  public function loginStatus() {
    

    Docblock here, too.

  3. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,181 @@
    +      $response = new ResourceResponse(self::LOGGED_IN);
    

    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.

  4. +++ b/core/modules/user/user.routing.yml
    @@ -129,6 +129,26 @@ user.login:
    +user.login.json:
    +  path: '/user/login'
    +  defaults:
    +    _controller: \Drupal\user\Controller\UserLoginController::login
    +  requirements:
    +    _user_is_logged_in: 'FALSE'
    +    _format: 'json'
    +  options:
    +    _maintenance_access: TRUE
    +
    +user.logout.json:
    +  path: '/user/logout'
    +  defaults:
    +    _controller: \Drupal\user\Controller\UserLoginController::logout
    +  requirements:
    +    _user_is_logged_in: 'TRUE'
    +    _format: 'json'
    +  options:
    +    _maintenance_access: TRUE
    +
    

    Both of these should be restricted to POST requests.

dawehner’s picture

Status: Needs work » Needs review
FileSize
22.63 KB
16.63 KB

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.

Good point!

Both of these should be restricted to POST requests.

You cannot be more right.

Status: Needs review » Needs work

The last submitted patch, 154: 2403307-154.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.61 KB
3.56 KB

There we go

andypost’s picture

@dawehner please update IS about why that becomes controller and not resource, subject should be updated as well

dawehner’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +security

The 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.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
    @@ -28,6 +28,17 @@ protected static function getPriority() {
    +    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_BAD_REQUEST);
    

    Nit: s/array()/[]/

  2. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,224 @@
    +   * @var string
    +   */
    +  const LOGGED_IN = 1;
    ...
    +   * @var string
    +   */
    +  const LOGGED_OUT = 0;
    

    These aren't strings, but ints.

    Shouldn't they be booleans though?

  3. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,224 @@
    +   * Constructs a new UserLoginResource object.
    

    Nit: outdated comment.

  4. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,224 @@
    +   * Controller to login a user.
    

    Nit: s/login/log in/

  5. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,224 @@
    +   * @return \Symfony\Component\HttpFoundation\Response
    

    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

  6. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,224 @@
    +   *   Returns a response which contains the ID and CSRF token.
    

    s/the ID and/the logged in user's ID, roles and/

dawehner’s picture

… 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

Well, 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 :)

The 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

Yeah maybe this should still be some setting or so. Exposing it by default increases the surface area too much. I agree.

swentel’s picture

+++ b/core/modules/user/src/Controller/UserLoginController.php
@@ -0,0 +1,224 @@
+    $this->flood->register('rest.login_cookie', $this->configFactory->get('user.flood')->get('user_window'));

Since 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?

Yeah maybe this should still be some setting or so. Exposing it by default increases the surface area too much. I agree.

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.

Wim Leers’s picture

Exactly. #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.

Wim Leers’s picture

In other words, I don't understand the sudden change of direction at #150:

We 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.

  1. Who is "we"?
  2. Why doesn't it make sense?
  3. Why change direction while we're just details away from an RTBC patch? What am I missing?
e0ipso’s picture

I may have contributed to this idea to make it an RPC service instead of shoehorning a REST resource during DrupalCon.

Why doesn't it make sense?

My points are:

  • I don't think there is a REST based entity (not talking about Drupal entities). There are no items to act upon.
  • It does not make sense to operate with the HTTP verbs for CRUD operations.
  • It fits really well with a Remote Procedure Call. You are requesting to execute the login/logout operation.
  • It is not stateless. On the contrary, it's very stateful.
  • It feels to me that we are using the REST approach only because: it's a web service related operation and as a route generation.

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.

dawehner’s picture

I'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

Why change direction while we're just details away from an RTBC patch? What am I missing?

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.

Wim Leers’s picture

AFAIK there is nothing that prevents us from having these routes to negotiate the response format/encoding according to the client's request.

While this is true, it does mean duplicating a fair bit of the logic that REST already has.

It is not stateless. On the contrary, it's very stateful.

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 security tag to this issue:

The 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.

Wim Leers’s picture

To 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).

e0ipso’s picture

I 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.

But there is one thing you didn't address, which is the most important one, and the reason I added the security tag to this issue

+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.

Wim Leers’s picture

RE: want to clarify that […] same every time -> you're totally right!
RE: I think we agree on almost everything -> indeed :)

Wim Leers’s picture

Just 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.

dawehner’s picture

Well, we can also just go back to the REST resource and add the field access checking, see

+      if ($user->get('uid')->access('view')) {
+        $response_data['current_user']['uid'] = $user->id();
+      }
+      if ($user->get('roles')->access('view')) {
+        $response_data['current_user']['roles'] = $user->getRoles();
+      }
+      if ($user->get('name')->access('view')) {
+        $response_data['current_user']['name'] = $user->getAccountName();
+      }
I have my own authenticate reource which returns more data (tokens, expire times etc),

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.

Crell’s picture

Making 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.

andypost’s picture

Another interesting point is access to this routes - looks user needs to be anonymous to login

+++ b/core/modules/user/src/Controller/UserLoginController.php
@@ -0,0 +1,224 @@
+  public function loginStatus() {
+    if ($this->currentUser()->isAuthenticated()) {
+      $response = new Response(self::LOGGED_IN);
+    }
+    else {
+      $response = new Response(self::LOGGED_OUT);
+    }

+++ b/core/modules/user/user.routing.yml
@@ -129,6 +129,32 @@ user.login:
+user.login_status.json:
+  path: '/user/login/status'
+  defaults:
+    _controller: \Drupal\user\Controller\UserLoginController::loginStatus
+  methods: [POST]
+  requirements:
+    _access: 'TRUE'

Looks that allows to check cookies for valid auth...

How that fits into other authentication providers?

dawehner’s picture

Good 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.

Wim Leers’s picture

Issue tags: +Needs tests

#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.

Wim Leers’s picture

… and support for other formats, of course.

dawehner’s picture

We already have a test for that:

+    // Flooded.
+    \Drupal::configFactory()->getEditable('user.flood')
+      ->set('user_limit', 3)
+      ->save();
+
+    $result = $this->loginRequest($name, 'wrong-pass');
+    $this->assertEquals(400, $result->getStatusCode());
+    $this->assertEquals('{"message":"Sorry, unrecognized username or password."}', (string) $result->getBody());
+
+    $result = $this->loginRequest($name, 'wrong-pass');
+    $this->assertEquals(400, $result->getStatusCode());
+    $this->assertEquals('{"message":"Sorry, unrecognized username or password."}', (string) $result->getBody());
+
+    $result = $this->loginRequest($name, 'wrong-pass');
+    $this->assertEquals(400, $result->getStatusCode());
+    $this->assertEquals('{"message":"Sorry, unrecognized username or password."}', (string) $result->getBody());
+
+    $result = $this->loginRequest($name, 'wrong-pass');
+    $this->assertEquals(400, $result->getStatusCode());
+    $this->assertEquals('{"message":"Blocked."}', (string) $result->getBody());
+
Wim Leers’s picture

I think that's less comprehensive tests than we have for the login form's flood control:

  1. \Drupal\user\Tests\UserLoginTest::testGlobalLoginFloodControl()
  2. \Drupal\user\Tests\UserLoginTest::testPerUserLoginFloodControl()

Let's duplicate that test for this new form-less login mechanism. And let's add bidirectional @sees.

dawehner’s picture

Fair point.

tedbow’s picture

Assigned: Unassigned » tedbow

Working on multiple formats tests.

tedbow’s picture

Assigned: tedbow » Unassigned

Starting 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?

Wim Leers’s picture

We 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 the serialization module is installed. But that means if ($this->moduleHandler->moduleExists()), which is

In any case, big -1 on explicitly calling JsonEncoder & XmlEncoder. That's still not supporting any formats anyone may add then.

tedbow’s picture

Assigned: Unassigned » tedbow

Ok assigning back to myself after @Wim Leers comment and chat with @e0ipso

dawehner’s picture

We 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.

I'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.

Crell’s picture

I 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.

dawehner’s picture

@crell
Oh that is a really strong point!

Wim Leers’s picture

XML 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
16.82 KB
17.96 KB

Ok here is another patch.

It does a few things

  1. Removes _format from routes because the same route will handle all available formats.
  2. If $container->has('serializer') then it is used for the encoding and decoding. This means any formats available in $container->getParameter('serializer.formats') will work
  3. If not then only xml and json will be supported.
  4. The above 2 are to avoid making the user module dependent on the serialization module
  5. In either case a 406 will be thrown if the sent format is not supported.

The part I was a bit ify about was

  public static function create(ContainerInterface $container) {
    if ($container->hasParameter('serializer.formats') && $container->has('serializer')) {
      $serializer = $container->get('serializer');
      $formats = $container->getParameter('serializer.formats');
    }
    else {
      $formats = ['json', 'xml'];
      $encoders = [new JsonEncoder(), new XmlEncoder()];
      $serializer = new Serializer([], $encoders);
    }

    return new static(
      $container->get('flood'),
      $container->get('entity_type.manager')->getStorage('user'),
      $container->get('csrf_token'),
      $container->get('user.auth'),
      $serializer,
      $formats
    );
  }

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

tedbow’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
    @@ -28,6 +28,17 @@ protected static function getPriority() {
    +   * Handles a 400 error for JSON.
    +   *
    +   * @param \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent $event
    +   *   The event to process.
    +   */
    +  public function on400(GetResponseForExceptionEvent $event) {
    +    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_BAD_REQUEST);
    +    $event->setResponse($response);
    +  }
    +
    

    This is probably not needed anymore I didn't notice this from previous patch.

  2. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +   * @param array $serializer_formats
    +   *   The available serialization formats.
    +   */
    +  public function __construct(FloodInterface $flood, UserStorageInterface $user_storage, CsrfTokenGenerator $csrf_token, UserAuthInterface $user_auth, Serializer $serializer, array $serializer_formats) {
    

    I left out the $serializer @param from phpdoc

  3. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -0,0 +1,201 @@
    +
    +    $formats = ['json'];
    +    foreach ($formats as $format) {
    

    Will need to add cases from both serialization on/ff and xml format to loop through

tedbow’s picture

I now see what ExceptionJsonSubscriber is being used for. It seems now we would need to make a ExceptionHTTPSubscriber that would handle all formats enabled.

Wim Leers’s picture

Great 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.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
    @@ -28,6 +28,17 @@ protected static function getPriority() {
    +  public function on400(GetResponseForExceptionEvent $event) {
    +    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_BAD_REQUEST);
    +    $event->setResponse($response);
    

    This looks … exactly like the on403(), on404(), on405() and on406() 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.

  2. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    + * Provides controllers for login, login status and logout.
    ...
    +class UserLoginController extends ControllerBase implements ContainerInjectionInterface {
    

    The class name suggests this is only for "login", but it's really used for 3 different things.

    What about renaming this UserAuthenticationController or UserSessionController?

  3. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +   * The Serializer.
    
    +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -0,0 +1,201 @@
    +   * The Serializer.
    

    s/Serializer/serializer/

  4. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +   * Constructs a new UserLoginResource object.
    

    Does not match the class name.

  5. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +   * @param \Drupal\user\UserAuthInterface $user_auth
    +   *   The user authentication.
    +   * @param array $serializer_formats
    +   *   The available serialization formats.
    

    Missing $serializer.

  6. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +    $this->userAuth = $user_auth;
    +    $this->serializerFormats = $serializer_formats;
    +    $this->serializer = $serializer;
    

    Nit: does not match parameter order.

  7. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +    else {
    +      $formats = ['json', 'xml'];
    +      $encoders = [new JsonEncoder(), new XmlEncoder()];
    +      $serializer = new Serializer([], $encoders);
    +    }
    

    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.

  8. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +   * Controller to login a user.
    ...
    +   * Controller to logout a user.
    ...
    +   * Controller to show whether a user is logged in or not.
    

    Strange descriptions. "Controller to …" and "login/logout" instead of "log in/log out".

    Look at other controllers for more consistent wording.

  9. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +   *   Returns a response which contains the ID and CSRF token.
    

    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.

  10. +++ b/core/modules/user/src/Controller/UserLoginController.php
    @@ -0,0 +1,277 @@
    +  }
    +
    +
    +  /**
    

    Nit: two newlines, should be one.

  11. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -0,0 +1,201 @@
    +   * @
    +   *
    

    Forgotten to be finished :)

  12. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -0,0 +1,201 @@
    +   * Test user session life cycle.
    

    Nit: s/Test/Tests/

  13. +++ b/core/modules/user/user.routing.yml
    @@ -129,6 +129,30 @@ user.login:
    +user.login.http:
    +  path: '/user/login/http'
    ...
    +user.login_status.http:
    +  path: '/user/login/status'
    ...
    +user.logout.http:
    +  path: '/user/logout/http'
    

    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 the serialization module is installed, specify all of the formats that supports.

    By doing that, these routes have the expected URLs.

tedbow’s picture

Status: Needs review » Needs work

@Wim Leers thanks for the review.

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.

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.

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Needs work » Needs review
FileSize
28.57 KB
23.7 KB

This patch address most of #191 plus so other code cleanup.

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 the serialization module is installed, specify all of the formats that supports.

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.

This looks … exactly like the on403(), on404(), on405() and on406() methods that already exist in this class. This is really ridiculous. But not the fault of this patch.

I changed this so they all call \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber::setEventResponse

tedbow’s picture

+++ b/core/modules/serialization/serialization.services.yml
@@ -64,3 +64,8 @@ services:
diff --git a/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php

diff --git a/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
new file mode 100644

I 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.

tedbow’s picture

added \Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber to alter user routes if serialization module enabled

tedbow’s picture

Title: REST resources for authenticated users: log in, check login status, log out » RPC endpoints for user authenication: log in, check login status, log out
FileSize
12.68 KB
28.16 KB

Ok this is just a change to the test \Drupal\Tests\user\Functional\UserLoginHttpTest

It now tests:

  1. json format without serialization module enabled
  2. json and xml formats with serialization module enabled

Also changing issue title because these are no longer rest resources

tedbow’s picture

Issue summary: View changes

Updating issue summary because string messages from login/logout are not longer return in favor http response codes

Wim Leers’s picture

Status: Needs review » Needs work

#193:

So this should be added to the serialization module?

Yes. The serialization module would enhance these routes in user.module with new capabilities if it is installed.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
    @@ -34,8 +34,7 @@ protected static function getPriority() {
    +    $this->setEventResponse($event, Response::HTTP_BAD_REQUEST);
    
    @@ -45,8 +44,7 @@ public function on400(GetResponseForExceptionEvent $event) {
    +    $this->setEventResponse($event, Response::HTTP_FORBIDDEN);
    
    @@ -56,8 +54,7 @@ public function on403(GetResponseForExceptionEvent $event) {
    +    $this->setEventResponse($event, Response::HTTP_NOT_FOUND);
    
    @@ -67,8 +64,7 @@ public function on404(GetResponseForExceptionEvent $event) {
        *   The event to process.
    ...
    +    $this->setEventResponse($event, Response::HTTP_METHOD_NOT_ALLOWED);
    
    @@ -78,7 +74,19 @@ public function on405(GetResponseForExceptionEvent $event) {
    +  protected function setEventResponse(GetResponseForExceptionEvent $event, $status) {
    

    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.

  2. +++ b/core/modules/serialization/serialization.services.yml
    @@ -64,3 +64,8 @@ services:
    +  serialization.exception.default_http:
    
    +++ b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
    @@ -0,0 +1,124 @@
    +class ExceptionHttpSubscriber extends HttpExceptionSubscriberBase {
    

    See above.


#195:

  1. +++ b/core/modules/serialization/serialization.services.yml
    @@ -69,3 +69,8 @@ services:
    +  serialization.user_last_access_subscriber:
    

    This service name looks bizarre to me. C/p remnant? :)

  2. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -0,0 +1,72 @@
    +  protected $serializerFormats = array();
    

    Nit: s/array()/[]/

  3. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -0,0 +1,72 @@
    +   * Adds supported formats to the user authentication http routes.
    

    Nit: s/http/HTTP/


#196:

Excellent! :)

Just one detail:

+++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
@@ -82,106 +84,118 @@ protected function loginRequest($name, $pass, $format) {
+    $serialization_enabled_options = [FALSE, TRUE];
+    foreach ($serialization_enabled_options as $serialization_enabled_option) {

You can inline this beautiful array for even more elegance :)


So IMO remaining tasks:

  1. more flood test coverage, as described in #178 (hence keeping the Needs tests issue tag)
  2. revert the exception handling changes that were added in #193, those belong in a different issue
clemens.tolboom’s picture

Please update IS regarding the curl commands too :-) ... are they still using json?

Wim Leers’s picture

  1. +++ b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
    @@ -0,0 +1,124 @@
    +class ExceptionHttpSubscriber extends HttpExceptionSubscriberBase {
    ...
    +  protected function getHandledFormats() {
    +    return $this->serializerFormats;
    +  }
    

    tedbow 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.

  2. +++ b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
    @@ -0,0 +1,124 @@
    +  public function on400(GetResponseForExceptionEvent $event) {
    ...
    +  public function on403(GetResponseForExceptionEvent $event) {
    ...
    +  public function on404(GetResponseForExceptionEvent $event) {
    ...
    +    $content = ['message' => $event->getException()->getMessage()];
    

    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:

    1. we don't want/need custom routes for each HTTP status code
    2. we already can (and do!) support messages for particular instances of a 4xx response: the exception contains such a message

    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 gazillion on$SOMETHING() methods, just let it do what the setEventResponse() method in this class is doing.

    That would mean that we support all 4xx responses in one go. Much simpler.

dawehner’s picture

This is looking quite great now!

So given the above, we should try to get rid of this insanity.

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.

  1. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -0,0 +1,72 @@
    +    foreach ($route_names as $route_name) {
    +      $route = $routes->get($route_name);
    

    Ideally we would check whether these routes are available. For example a module might remove them entirely

  2. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -0,0 +1,276 @@
    +    else {
    +      $formats = ['json'];
    +      $encoders = [new JsonEncoder()];
    +      $serializer = new Serializer([], $encoders);
    +    }
    

    +1 for just supporting json

  3. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -0,0 +1,276 @@
    +    $this->flood->register('rest.login_cookie', $this->configFactory->get('user.flood')->get('user_window'));
    ...
    +    return $this->flood->isAllowed('rest.login_cookie', $limit, $interval);
    

    What about using 'user.http_login' as flood name or so? Could be a constant on the class as well.

  4. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -0,0 +1,276 @@
    +    if (!in_array($format, $this->serializerFormats)) {
    +      throw new BadRequestHttpException("Unrecognized format: $format.");
    +    }
    

    Conceptually this is more of a 406 error, so let's leverage \Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException

  5. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -0,0 +1,261 @@
    +      if ($serialization_enabled_option) {
    +        /** @var \Drupal\Core\Extension\ModuleInstaller $module_installer */
    +        $module_installer = \Drupal::service('module_installer');
    +        $module_installer->install(['serialization']);
    +        $formats = ['json', 'xml'];
    +      }
    +      else {
    +        // Without the serialization module only JSON is supported.
    +        $formats = ['json'];
    ...
    +      foreach ($formats as $format) {
    

    Nice!

tedbow’s picture

Conceptually this is more of a 406 error, so let's leverage \Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException

Actually 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?

tstoeckler’s picture

+ // This will fire after the most common HTML handler, since HTML requests
+ // are still more common than HTTP requests.

This 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 to UserAuthenticationController::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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
16.29 KB
28.25 KB

Uploading new patch will comment in few about changes

Wim Leers’s picture

#203:

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.

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:

  1. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -258,4 +245,74 @@ protected function resetFlood() {
    +   * Test the global login flood control.
    ...
    +   * Check a response for status code and body.
    

    Nit: must be 3rd singular.

  2. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -258,4 +245,74 @@ protected function resetFlood() {
    +  protected function checkResponse(ResponseInterface $response, $expected_code, $expected_body) {
    ...
    +  protected function checkResponseWithMessage(ResponseInterface $response, $expected_code, $expected_message, $format) {
    

    s/check*/assert*/, which also means these need return values like other assertion methods.

tedbow’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
    @@ -74,19 +78,7 @@ public function on405(GetResponseForExceptionEvent $event) {
    -  protected function setEventResponse(GetResponseForExceptionEvent $event, $status) {
    

    Removed this function as it was noted it is out of the scope of this issue. Revert on40* functions that were calling it.

  2. +++ b/core/modules/serialization/serialization.services.yml
    @@ -69,7 +69,7 @@ services:
    -  serialization.user_last_access_subscriber:
    +  serialization.user_route_alter_subscriber:
    

    Renamed service

  3. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -25,7 +25,7 @@ class UserRouteAlterSubscriber implements EventSubscriberInterface {
    -  protected $serializerFormats = array();
    +  protected $serializerFormats = [];
    

    fixed nit

  4. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -62,10 +62,11 @@ public function onRoutingRouteAlterAddFormats(RouteBuildEvent $event) {
    -      $route = $routes->get($route_name);
    -      $formats = explode('|', $route->getRequirement('_format'));
    -      $formats = array_unique($formats + $this->serializerFormats);
    -      $route->setRequirement('_format', implode('|', $formats));
    +      if ($route = $routes->get($route_name)) {
    +        $formats = explode('|', $route->getRequirement('_format'));
    +        $formats = array_unique($formats + $this->serializerFormats);
    +        $route->setRequirement('_format', implode('|', $formats));
    +      }
    

    Checked to see if routes actually exist

  5. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -258,4 +245,74 @@ protected function resetFlood() {
    +  public function testGlobalLoginFloodControl() {
    

    Added 1 flood control test and logic in controller. Need to add the other one

  6. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -258,4 +245,74 @@ protected function resetFlood() {
    +  protected function checkResponse(ResponseInterface $response, $expected_code, $expected_body) {
    ...
    +  protected function checkResponseWithMessage(ResponseInterface $response, $expected_code, $expected_message, $format) {
    

    Add 2 new help functions to check responses. I felt it made the test more readable.

tedbow’s picture

@Wim Leers

s/check*/assert*/, which also means these need return values like other assertion methods.

Changed the function name but I am just calling assertEquals in both these functions. assertEquals doesn't return a value.

Nit: must be 3rd singular.

Fixed

Wim Leers’s picture

Status: Needs review » Needs work

#203: the implication of are still the most common requests. 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.

  1. +++ b/core/modules/serialization/serialization.services.yml
    @@ -64,3 +64,13 @@ services:
    +  serialization.exception.default_http:
    +    class: Drupal\serialization\EventSubscriber\ExceptionHttpSubscriber
    

    This compares to:

      exception.default_html:
        class: Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber
    

    So given that, I'd call this service serialization.exception.default and the class DefaultExceptionSubscriber.

  2. +++ b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
    @@ -0,0 +1,124 @@
    + * Default handling for HTTP errors.
    

    Core's default says:

    Exception subscriber for handling core default HTML error pages.
    

    I'd make this in line with that, so:

    Exception subscriber for handling default  error responses in serialization formats.
    
  3. +++ b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
    @@ -0,0 +1,124 @@
    +   * The available serialization formats.
    ...
    +   *   The available serializer formats.
    

    Nit: inconsistent.

  4. +++ b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
    @@ -0,0 +1,124 @@
    +  protected $serializerFormats = array();
    

    Nit: s/array()/[]/

  5. +++ b/core/modules/serialization/src/EventSubscriber/ExceptionHttpSubscriber.php
    @@ -0,0 +1,124 @@
    +  public function on400(GetResponseForExceptionEvent $event) {
    ...
    +  public function on403(GetResponseForExceptionEvent $event) {
    ...
    +  public function on404(GetResponseForExceptionEvent $event) {
    ...
    +  public function on405(GetResponseForExceptionEvent $event) {
    

    See #200.2, that'd make all this go away.

  6. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -0,0 +1,73 @@
    + * Alters user authentication routes to add formats supported by this module.
    

    Not just by this module: other modules can add more formats.

  7. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -0,0 +1,73 @@
    +   * The available serialization formats.
    ...
    +   *   The available serializer formats.
    

    Nit: inconsistent.

  8. +++ b/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php
    @@ -0,0 +1,73 @@
    +  public function onRoutingRouteAlterAddFormats(RouteBuildEvent $event) {
    

    Very elegant, very easy to understand :)

  9. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -0,0 +1,285 @@
    +   *   Returns a response which contains the ID and CSRF token.
    ...
    +   *   Returns TRUE if the user is blocked, otherwise FALSE.
    

    Without the verb.

  10. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -0,0 +1,318 @@
    +   * Get a value for a given key from the response.
    ...
    +   * Reset all flood entries.
    ...
    +   * Test the global login flood control.
    

    Nit: 3rd person singular.

  11. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -0,0 +1,318 @@
    +  public function testGlobalLoginFloodControl() {
    

    Yay! Now just the one other flood control test, and then we're done here!

    Also: let's add @sees to the non-REST variants of these flood control tests.

tedbow’s picture

Status: Needs work » Needs review
FileSize
32.24 KB
22.02 KB

Ok addressed issues in #208

tedbow’s picture

Notes besides what was address from review in #208

  1. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -217,20 +214,6 @@ protected function userLoginFinalize(UserInterface $user) {
    -  protected function isFloodBlocked() {
    

    This function is not need because of the new function floodControl which throws exceptions if request should be blocked.

  2. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -282,4 +265,65 @@ protected function getRequestFormat(Request $request) {
    +  protected function floodControl(Request $request, $username) {
    

    This is new function to enforce flood control. I attempted to implement the logic that is found in UserLoginForm to enforce flood control.

  3. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -282,4 +265,65 @@ protected function getRequestFormat(Request $request) {
    +        if ($flood_config->get('uid_only')) {
    +          $error_message = $this->formatPlural($flood_config->get('user_limit'), 'There has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or request a new password.', 'There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or request a new password.');
    +        }
    +        else {
    +          $error_message = $this->t('Too many failed login attempts from your IP address. This IP address is temporarily blocked.');
    +        }
    +        throw new AccessDeniedHttpException($error_message, NULL, 403);
    
    +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -124,7 +124,7 @@ public function testLogin() {
    -        $this->assertResponseWithMessage($response, 400, 'Blocked.', $format);
    

    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"

  4. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -257,17 +253,17 @@ public function testGlobalLoginFloodControl() {
    -    $response = $this->loginRequest($user->getUsername(), $user->pass_raw);
    -    $this->assertResponseWithMessage($response, '403', 'Access is blocked because of IP based flood prevention.', 'json');
    +    $response = $this->loginRequest($user->getUsername(), $user->passRaw);
    +    $this->assertResponseWithMessage($response, '403', 'Access is blocked because of IP based flood prevention.');
    

    $user->pass_raw was wrong here. Password is actually stored in $user->passRaw

  5. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
    @@ -310,9 +306,90 @@ protected function assertResponse(ResponseInterface $response, $expected_code, $
    +  public function testPerUserLoginFloodControl() {
    +    foreach ([TRUE, FALSE] as $uid_only_setting) {
    

    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.

Status: Needs review » Needs work

The last submitted patch, 209: 2403307-209.patch, failed testing.

tedbow’s picture

Ok I see the failures relate to DefaultExceptionSubscriber::onException.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
32.57 KB

Fixed \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber::onException

It was causing problems in other existing REST tests not the new tests here.

Status: Needs review » Needs work

The last submitted patch, 213: 2403307-213.patch, failed testing.

tedbow’s picture

Ok 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

/**
 * Last-chance handler for exceptions.
 *
 * This handler will catch any exceptions not caught elsewhere and report
 * them as an error page.
 */

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

tedbow’s picture

Looking 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.

dawehner’s picture

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.

I'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?

Wim Leers’s picture

#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:

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.

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 have ExceptionJsonSubscriber and ExceptionHalJsonSubscriber: every module adding a new format is responsible for adding an exception subscriber that implements every possible on$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.

dawehner’s picture

+++ b/core/modules/user/user.routing.yml
@@ -129,6 +129,33 @@ user.login:
+user.login_status.http:
+  path: '/user/login_status'
+  defaults:
+    _controller: \Drupal\user\Controller\UserAuthenticationController::loginStatus
+  methods: [POST]
+  requirements:
+    _access: 'TRUE'
+    _format: 'json'

Does it really make sense for this to be a POST request rather than a GET request?

tedbow’s picture

Status: Needs work » Needs review
FileSize
33.7 KB
6.04 KB

Ok I added back the on40x functions.

Does it really make sense for this to be a POST request rather than a GET request?

Makes sense to me changed user.login_status.http to GET.

Perhaps that even means adding a ExceptionXmlSubscriber, which would be in line with the existing pattern, where we have ExceptionJsonSubscriber and ExceptionHalJsonSubscriber:

I didn't add this because it seems REST is already handling XML exceptions without this so it seems unrelated to this issue.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Hurray! :)

(Also, great catch @dawehner in #219!)

dawehner’s picture

It looks good for me now

tedbow’s picture

Issue summary: View changes
tedbow’s picture

Issue summary: View changes
tedbow’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

IS clarifications.

dawehner’s picture

Adapted the CR as well.

Wim Leers’s picture

Great, thanks Daniel!

fgm’s picture

I'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 ?

  • user logs in as A, successfully
  • user attempts to log in again as B, with proper credentials: what is the expected behaviour ?
    • fail because a logout is needed before re-logging ?
    • succeed because the credentials are correct, but then
      • emit a logout before logging the user, for consistency with UI situations where you need to logout before logging again
      • do not emit a logout because it was not requested ?
dawehner’s picture

@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?

Crell’s picture

I'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.

Wim Leers’s picture

#229: Great question!

fail because a logout is needed before re-logging ?

This is what happens because of those _user_is_logged_in requirements.

+++ b/core/modules/user/user.routing.yml
@@ -129,6 +129,33 @@ user.login:
+user.login.http:
+  path: '/user/login'
+  defaults:
+    _controller: \Drupal\user\Controller\UserAuthenticationController::login
+  methods: [POST]
+  requirements:
+    _user_is_logged_in: 'FALSE'
...
+user.logout.http:
+  path: '/user/logout'
+  defaults:
+    _controller: \Drupal\user\Controller\UserAuthenticationController::logout
+  methods: [POST]
+  requirements:
+    _user_is_logged_in: 'TRUE'
Wim Leers’s picture

Issue tags: -DrupalCampES
Crell’s picture

Oh good, then the code is already doing what we want. :-) Is it worth adding a test for that as fgm suggests?

Wim Leers’s picture

We could, but it's not exactly essential to this issue/patch. Seems fine for a follow-up though.

Wim Leers’s picture

Crell’s picture

I'm still good with the RTBC here, FTR.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 220: 2403307-220.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

This failed due to #2735235: BrowserTestBase standards cleanup being temporarily committed. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 220: 2403307-220.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
33.09 KB
14.07 KB

Let's fix stuff but also improve the test coverage a bit by checking 'roles' and 'uid' as part of the result.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Mostly small nitpick improvements, but also this:

+++ b/core/modules/user/src/Controller/UserAuthenticationController.php
@@ -166,13 +166,13 @@ public function login(Request $request) {
-      if ($user->get('uid')->access('view')) {
+      if ($user->get('uid')->access('view', $user)) {
         $response_data['current_user']['uid'] = $user->id();
       }
-      if ($user->get('roles')->access('view')) {
+      if ($user->get('roles')->access('view', $user)) {
         $response_data['current_user']['roles'] = $user->getRoles();
       }
-      if ($user->get('name')->access('view')) {
+      if ($user->get('name')->access('view', $user)) {

+++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
@@ -98,7 +98,9 @@ public function testLogin() {
+        // Grant the user administer users permissions to they can see the
+        // 'roles' field.
+        $account = $this->drupalCreateUser(['administer users']);

@@ -146,22 +148,24 @@ public function testLogin() {
+        $this->assertEquals($account->id(), $result_data['current_user']['uid']);
+        $this->assertEquals($account->getRoles(), $result_data['current_user']['roles']);

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!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -0,0 +1,329 @@
    +/**
    ...
    + */
    

    Apart from for the regular UI - right? I think this should be improved to make the distinction between this and UserController

  2. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
    @@ -0,0 +1,329 @@
    +          $error_message = $this->formatPlural($flood_config->get('user_limit'), 'There has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or request a new password.', 'There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or request a new password.');
    ...
    +          $error_message = $this->t('Too many failed login attempts from your IP address. This IP address is temporarily blocked.');
    ...
    +        throw new AccessDeniedHttpException($error_message, NULL, 403);
    

    Why is this message translated but none of the others are.

  3. +++ b/core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected static function getPriority() {
    +    // This will fire after the most common HTML handler, since HTML requests
    +    // are still more common than HTTP requests.
    +    return -75;
    +  }
    

    /aside I really dislike priorities - so difficult to know if this is the right number or not.

dawehner’s picture

Status: Needs work » Needs review
FileSize
33.27 KB
4.83 KB
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

/aside I really dislike priorities - so difficult to know if this is the right number or not.

+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 and after: Y relationships.


#244:

+++ b/core/modules/serialization/src/EventSubscriber/DefaultExceptionSubscriber.php
@@ -106,6 +106,16 @@ public function on406(GetResponseForExceptionEvent $event) {
+  public function on429(GetResponseForExceptionEvent $event) {

+++ b/core/modules/user/src/Controller/UserAuthenticationController.php
@@ -276,7 +276,7 @@ protected function getRequestFormat(Request $request) {
-      throw new AccessDeniedHttpException('Access is blocked because of IP based flood prevention.', NULL, 403);
+      throw new AccessDeniedHttpException('Access is blocked because of IP based flood prevention.', NULL, Response::HTTP_TOO_MANY_REQUESTS);

@@ -284,12 +284,12 @@ protected function floodControl(Request $request, $username) {
-        throw new AccessDeniedHttpException($error_message, NULL, 403);
+        throw new AccessDeniedHttpException($error_message, NULL, Response::HTTP_TOO_MANY_REQUESTS);

This makes a ton of sense. Nice additional improvement!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed da16ae5 and pushed to 8.2.x. Thanks!

diff --git a/core/modules/user/src/Controller/UserAuthenticationController.php b/core/modules/user/src/Controller/UserAuthenticationController.php
index c8644f6..e739795 100644
--- a/core/modules/user/src/Controller/UserAuthenticationController.php
+++ b/core/modules/user/src/Controller/UserAuthenticationController.php
@@ -18,7 +18,7 @@
 use Symfony\Component\Serializer\Serializer;
 
 /**
- * Provides controllers for login, login status and logout via HTTP requests.
+ * Provides HTTP RPC endpoints for login, login status and logout.
  */
 class UserAuthenticationController extends ControllerBase implements ContainerInjectionInterface {
 

Fixed on commit - the description in #244 still could be said about UserController

  • alexpott committed da16ae5 on 8.2.x
    Issue #2403307 by dawehner, marthinal, clemens.tolboom, tedbow, Wim...
klausi’s picture

The 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?

dawehner’s picture

@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?

alexpott’s picture

+++ b/core/modules/user/src/Controller/UserAuthenticationController.php
@@ -0,0 +1,328 @@
+      $response_data['csrf_token'] = $this->csrfToken->get('rest');

Well 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?

  • alexpott committed 9764ec4 on 8.2.x
    Revert "Issue #2403307 by dawehner, marthinal, clemens.tolboom, tedbow,...
alexpott’s picture

Status: Fixed » Needs work

Given @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.

Wim Leers’s picture

Oh 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.)

klausi’s picture

Status: Needs work » Fixed

$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?

klausi’s picture

Status: Fixed » Needs work

Sorry, did not mean to change the status.

Wim Leers’s picture

Issue tags: +Needs tests

But I think we support "?_format=json" in the URL query part, so should be easy, right?

Correct.

So, we should have a test proving that the current patch contains a CSRF vulnerability. And then we should fix it.

tedbow’s picture

Assigned: Unassigned » tedbow

Ok, I will work on the test and then the fix

tedbow’s picture

@Wim Leers from IRC

So on the "CSRF for log out" issue: do we want to use the "_csrf_token" route option (which requires ?token=TOKEN), or do we want to use the "_access_rest_csrf" route option (which requires a X-CSRF-Token request header)? I think the latter makes more sense. But it means moving that logic out of REST module, into core, so that both 'rest' module and 'user' module can use it (and all of contrib). And we'd have to make sure we keep BC

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.

  1. Move logic from \Drupal\rest\Access\CSRFAccessCheck to existing \Drupal\Core\Access\CsrfAccessCheck class
  2. Then \Drupal\Core\Access\CsrfAccessCheck would handle both _access_rest_csrf and _csrf_token route requirements requirements

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?

tedbow’s picture

Assigned: tedbow » Unassigned

Un-assigning from myself for feedback on above

Wim Leers’s picture

Why did REST module not use _csrf_token in the first place?

A 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.

Should we cover _access_rest_csrf with backward compatibility but change all rest routes to use _csrf_token?

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 the rest.csrftoken route out of the REST module into Drupal core's system 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).

klausi’s picture

The 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.

Crell’s picture

My 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
44.56 KB
15.36 KB

Here 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.

  1. Both deal with "access" via a "token"
  2. Neither one of these would give you any idea by name which one expects the token to be in the url and which in the header.
dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -42,26 +60,91 @@ public function __construct(CsrfTokenGenerator $csrf_token) {
    +  protected function pathAccess(Route $route, Request $request, RouteMatchInterface $route_match) {
    

    What 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.

  2. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -42,26 +60,91 @@ public function __construct(CsrfTokenGenerator $csrf_token) {
    +    if (!in_array($method, array('GET', 'HEAD', 'OPTIONS', 'TRACE'))
    

    Given that we handle options request before routing, I was wondering whether we maybe should just use !$request->isMethodSafe() here?

  3. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -42,26 +60,91 @@ public function __construct(CsrfTokenGenerator $csrf_token) {
    +    // If no token provided or not a write method then do not allow or forbid.
    +    return AccessResult::neutral();
    

    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?

Wim Leers’s picture

#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:

  1. Great idea! Furthermore, "path" is wrong, "query" is correct.
  2. Sounds good.
  3. I agree. I think a missing token means an immediate, unoverrideable "nope, can't touch that". It's essential. It's not like some module can grant you access regardless, because that would open you up to CSRF attacks.

  1. My main concern is that this approach of having the same route requirement (_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 like Url::fromRoute('user.logout.http')->toString() will result in /user/logout?token=TOKEN. Because RouteProcessorCSRF will not know the difference. We could work around that by letting RouteProcessorCsrf 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: with CsrfAccessCheck, 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 that UserAuthenticationController::login() now receives $response_data['csrf_token'] = $this->csrfToken->get('rest');. But how is that token able to even allow for CsrfAccessCheck to allow access? Oh, I see, because CsrfAccessCheck'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:

    1. the CSRF token is now specific to this session and that specific URL
    2. it won't be cached since it's still a POST request
  2. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -4,15 +4,23 @@
    + * To use this check you can either provide the token via the URL or the http
    ...
    + * To use this token via the header, provide the token the 'X-CSRF-Token' http
    

    s/http/HTTP/

  3. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -4,15 +4,23 @@
    + *  header.
    

    s/ header/request header/

    (Note the extraneous leading space.)

  4. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -42,26 +60,91 @@ public function __construct(CsrfTokenGenerator $csrf_token) {
    +    if (!$header_access->isNeutral()) {
    ...
    +      if (!$path_access->isNeutral()) {
    ...
    +    if ($request->query->has('token')) {
    ...
    +    return AccessResult::neutral();
    ...
    +      if ($request->headers->has('X-CSRF-Token')) {
    

    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.

  5. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -42,26 +60,91 @@ public function __construct(CsrfTokenGenerator $csrf_token) {
    +   * Checks for a valid csrf token via the path.
    

    s/csrf/CSRF/

dawehner’s picture

Status: Needs review » Needs work

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.

I agre with you. Its something we can easily avoid, so let's do that. It also makes the code easier to follow etc.

neclimdul’s picture

This 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
9.62 KB
36.46 KB

Ok. 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

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:

But has UserAuthenticationController::login() return a $response_data['logout_token'] instead of full url.

Maybe it is moot point because:

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.

Which seems like a good idea unless people love this approach I just took.

dawehner’s picture

+++ b/core/modules/user/src/Controller/UserAuthenticationController.php
@@ -177,6 +189,11 @@ public function login(Request $request) {
       }
       $response_data['csrf_token'] = $this->csrfToken->get('rest');
 
+      $logout_route = $this->routeProvider->getRouteByName('user.logout.http');
+      // Trim '/' off path to match \Drupal\Core\Access\CsrfAccessCheck.
+      $logout_path = ltrim($logout_route->getPath(), '/');
+      $response_data['logout_token'] = $this->csrfToken->get($logout_path);

Is 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.

tedbow’s picture

Status: Needs review » Postponed
Wim Leers’s picture

Title: RPC endpoints for user authenication: log in, check login status, log out » RPC endpoints for user authentication: log in, check login status, log out
kylebrowning’s picture

Issue summary: View changes

+1

P.s fixed a typo in description because it bugggggged me :P

Wim Leers’s picture

Wim Leers’s picture

The 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 a on422() method, and this one adds a on429() 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 that on429() method.

I didn't make any changes, I only rebased, to expedite this issue.

Wim Leers’s picture

Issue tags: -Needs tests

I added the Needs tests issue tag in #256, where I wrote:

So, we should have a test proving that the current patch contains a CSRF vulnerability. And then we should fix it.

@tedbow added tests for that in #263.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

The only remaining remark is the one in #269, which is just a nitpick. I think @dawehner is basically saying just hardcode the path, don't bother with that ltrim() business. But I think that it's actually the right thing to do, because \Drupal\Core\Access\CsrfAccessCheck::access() indeed does this:

    $path = ltrim($route->getPath(), '/');
    …

    if ($this->csrfToken->validate($request->query->get('token'), $path)) {

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!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.2.0 release notes

Second time lucky - @klausi's security concern has been addressed.

Committed 18feefc and pushed to 8.2.x. Thanks!

kylebrowning’s picture

Omg yay!

Thanks everyone!

  • alexpott committed 18feefc on 8.2.x
    Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom, Wim...
Wim Leers’s picture

\o/

tstoeckler’s picture

Status: Fixed » Reviewed & tested by the community

Seems #2278383: Create an injectible service for drupal_set_message() was committed together with this one here.

  • alexpott committed 1e770ce on 8.2.x
    Revert "Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom,...
  • alexpott committed 27df05c on 8.2.x
    Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom, Wim...
Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the quick fix!

  • alexpott committed 9764ec4 on 8.3.x
    Revert "Issue #2403307 by dawehner, marthinal, clemens.tolboom, tedbow,...
  • alexpott committed da16ae5 on 8.3.x
    Issue #2403307 by dawehner, marthinal, clemens.tolboom, tedbow, Wim...
  • alexpott committed 18feefc on 8.3.x
    Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom, Wim...
  • alexpott committed 1e770ce on 8.3.x
    Revert "Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom,...
  • alexpott committed 27df05c on 8.3.x
    Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom, Wim...

  • alexpott committed 9764ec4 on 8.3.x
    Revert "Issue #2403307 by dawehner, marthinal, clemens.tolboom, tedbow,...
  • alexpott committed da16ae5 on 8.3.x
    Issue #2403307 by dawehner, marthinal, clemens.tolboom, tedbow, Wim...
  • alexpott committed 18feefc on 8.3.x
    Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom, Wim...
  • alexpott committed 1e770ce on 8.3.x
    Revert "Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom,...
  • alexpott committed 27df05c on 8.3.x
    Issue #2403307 by dawehner, marthinal, tedbow, clemens.tolboom, Wim...
kylebrowning’s picture

Was 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.

kylebrowning’s picture

dawehner’s picture

Great, someone trying out this new feature :)

Was there a reason we chose to return a single string/integer from user/login_status?

What else would you expect to be there?

Also the release notes / CR don't actually depict what you should do.

For example theres no logout token in the logout example.

The great part about CRs is that everyone can fix them :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

ruloweb’s picture

Now 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?

Wim Leers’s picture

#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.

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?

Yes, please create a new feature request issue for that.

damiankloip’s picture

This is under the rest module but not one line of code in the rest module is touched here.

ruloweb’s picture

@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.