Problem/Motivation

Unauthenticated users send a POST request on http://mysite.com/entity/user get a 403, and rightfully so because creating users in an administrative task.
That being said registering a user should be allowed if the site permits.

Proposed resolution

Create a user registration resource that allows users to be registered into the system following the settings on admin/config/people/accounts.
This means anonymous users can register an account just as if they could on a regular drupal form.
A workaround would be to login as an admin and create a user the regular way.

Remaining tasks

  1. Land blockers:
    1. #2626298: REST module must cache only responses to GET requests
  2. Get patch to RTBC
  3. Security Review

User interface changes

None

API changes

None

Data model changes

None

Original report by [username]

I have enabled /entity/user REST end point for POST request.
I have give anonymous user permission "Access to POST on user entity"
I have also enable "visitor" to create an account on my Drupal site.

My rest.setting has an entry as shown below

 'entity:user':
    GET:
      supported_formats:
        - hal_json
        - json
      supported_auth:
        - basic_auth
    POST:
      supported_formats:
        - hal_json
        - json
      supported_auth:
        - basic_auth

I tried a POST request on http://mysite.com/entity/user

with headers

Content-Type : application/hal+json

and with body

{
  "_links":{"type":{"href":"http://tntfoss-vivekvpandya.rhcloud.com/rest/type/user/user"}},
  "name":{
    "value":"ronak1"
  },
  "mail":{
    "value":"ronak1.patel@gmail.com"
  },
  "pass":{
    "value":"ronaK123"
  }

}

It give me 403 forbidden.

If I pass credential in Authorization header it works for me.

It should also work for anonymous user because always visitor(anonymous) user will try to create account on the site.

I need review from community is it a bug ?

Files: 
CommentFileSizeAuthor
Capture.PNG41.19 KBvivekvpandya
#2 AdditionalInfo.png90.68 KBvivekvpandya
#10 Screen Shot 2014-12-24 at 10.40.08.png182.16 KBmarthinal
#10 2291055-10.patch760 bytesmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,779 pass(es). View
#11 2291055-11.patch5.68 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,316 pass(es). View
#11 Screen Shot 2014-12-30 at 13.39.18.png973.29 KBmarthinal
#14 interdiff-2291055-11-14.txt4.79 KBmarthinal
#14 2291055-14.patch9.26 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,437 pass(es). View
#16 interdiff-2291055-14-16.txt2.52 KBmarthinal
#16 rest-2291055-16.patch7.87 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,432 pass(es). View
#19 interdiff-2291055-16-19.txt2.03 KBmarthinal
#19 2291055-19.patch8.95 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,142 pass(es). View
#21 creation_of_entity_type-2291055-21.patch8.21 KBclemens.tolboom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,973 pass(es). View
#23 2291055-23.patch7.35 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,295 pass(es). View
#28 interdiff.txt14.55 KBm1r1k
#28 2291055-28.patch15.82 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,265 pass(es). View
#39 2291055-39.patch16.78 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,157 pass(es). View
#39 interdiff-2291055-28-39.txt2.7 KBmarthinal
#42 2291055-42.patch15.84 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#42 interdiff-2291055-28-42.txt7.78 KBmarthinal
#42 roles.png17.61 KBmarthinal
#42 role1.png30.11 KBmarthinal
#42 roles2.png11.53 KBmarthinal
#45 2291055-45.patch16.11 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,140 pass(es). View
#45 interdiff-2291055-42-45.txt4.38 KBmarthinal
#49 2291055-49.patch19.25 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,599 pass(es). View
#49 interdiff-2291055-45-49.txt4.28 KBmarthinal
#53 2291055-53.patch20.94 KBmarthinal
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,931 pass(es). View
#53 interdiff-2291055-49-53.txt17.38 KBmarthinal
#55 restResourceResponseUMLClass.png26.05 KBmarthinal
#55 2291055-55.patch28.09 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,809 pass(es), 25 fail(s), and 0 exception(s). View
#64 interdiff.txt4.92 KBkylebrowning
#64 2291055-64.patch29.02 KBkylebrowning
#66 2291055-66.patch31.57 KBmarthinal
#66 interdiff-2291055-64-66.txt12.23 KBmarthinal
#68 2291055-68.patch31.58 KBmarthinal
#77 2291055-77.patch32.62 KBkylebrowning
#84 2291055-84.patch33.02 KBmarthinal
#84 interdiff-2291055-82-84.txt762 bytesmarthinal
#107 2291055-107-do-not-test.patch20.24 KBmarthinal
#107 interdiff-2291055-84-107.txt12.78 KBmarthinal
#113 user_reg.png9.11 KBvivekvpandya
#119 interdiff-2291055-107-119.txt2.52 KBmarthinal
#119 2291055-119-do-not-test.patch19.99 KBmarthinal
#134 2291055-134.patch19.98 KBmarthinal
#134 interdiff-2291055-119-134.txt1.43 KBmarthinal
#137 2291055-137.patch18.89 KBmarthinal
#137 interdiff-2291055-134-137.txt11.33 KBmarthinal
#139 2291055-139.patch18.44 KBmarthinal
#139 interdiff-2291055-137-139.txt1.72 KBmarthinal
#143 2291055-143.patch23.31 KBmarthinal
#143 interdiff-2291055-139-143.txt15.45 KBmarthinal
#145 interdiff-2291055-143-145.txt11.24 KBmarthinal
#145 2291055-145.patch22.61 KBmarthinal
#148 2291055-148.patch21.85 KBmarthinal
#148 interdiff-2291055-146-148.txt8.17 KBmarthinal
#151 interdiff-2291055-148-151.txt3.68 KBmarthinal
#151 2291055-151.patch21.77 KBmarthinal
#158 interdiff-2291055-151-158.txt1.91 KBmarthinal
#158 2291055-158.patch22.31 KBmarthinal
#163 rest_resources_for-2291055-163.patch22.34 KBjlbellido
#168 2291055-168.patch21.99 KBmarthinal
#168 interdiff-2291055-163-168.txt8.47 KBmarthinal
#170 interdiff-2291055-168-170.txt4.09 KBmarthinal
#170 2291055-170.patch22.16 KBmarthinal
#172 2291055-172.patch22.16 KBmarthinal
#172 interdiff-2291055-170-172.txt1.41 KBmarthinal
#174 2291055-174.patch22.15 KBmarthinal
#174 interdiff-2291055-172-174.txt1.26 KBmarthinal
#178 1.png98.66 KBsnehal.brahmbhatt
#185 2291055-185.patch24.19 KBtedbow
#185 interdiff-2291055-174-185.txt7.54 KBtedbow
#187 2291055-187.patch24.05 KBtedbow
#187 interdiff-2291055-185-187.txt1.29 KBtedbow
#189 2291055-189.patch24.77 KBtedbow
#189 interdiff-2291055-187-189.txt6.13 KBtedbow
#196 interdiff.txt2.56 KBWim Leers
#196 2291055-195.patch25.76 KBWim Leers
#198 2291055-198.patch27.04 KBtedbow
#198 interdiff-22910552291055-196-198.txt7.87 KBtedbow
#204 2291055-204.patch27.02 KBtedbow
#204 interdiff-2291055-198-204.txt1.81 KBtedbow
#213 2291055-213.patch27.59 KBWim Leers
#213 interdiff.txt4.54 KBWim Leers
#216 2291055-216.patch26.98 KBeffulgentsia
#216 interdiff-213-216.txt1.69 KBeffulgentsia
#218 2291055-218.patch27 KBtedbow
#218 interdiff-2291055-216-218.txt8.3 KBtedbow
#219 2291055-219.patch27.16 KBWim Leers
#219 interdiff.txt1.44 KBWim Leers

Comments

klausi’s picture

Version: 8.0-alpha12 » 8.x-dev
Status: Needs review » Active

I think that giving the permission to create user accounts to the anonymous user could be dangerous. Did you debug the access check? I could imagine that it requires "administer users" or similar to create an account. UserAccessController does not implement checkCreateAccess(), so the implementation from EntityAccessController is used. And that facilitates the admin permission as far as I can see, which is "administer users".

So the question is if we should change the permission required to create user accounts. We would need proper field access implemented in #2029855: Missing access control for user base fields, so that people cannot give themselves admin user roles for example.

The second approach would be to create a separate user registration resource, which is only responsible for registering new users as anonymous. So instead of POSTing to /entity/user you would post to something like /user/register. There are a couple things involved here, for example many sites require email verification before user can log in. Having an API that allows you to create accounts without verification is a paradise for spambots that want to create accounts on your site.

vivekvpandya’s picture

FileSize
90.68 KB

A credential set for a authenticated user other than "administrator" is not working. I have checked it.
I suggest to stick with /entity/user POST method.
And for other concerns like email verification before login , it will be good if we can include all settings related to user creation from admin/config/people/accounts
I have attached a sample settings snapshot.

clemens.tolboom’s picture

Title: Creation of entity type user with out credential is not working » Creation of entity type user without credential is not working

@klausi why should there be a different endpoint for creating a user?

I'm in favor of keeping in sync with other REST endpoint so using /entity/user or according to Change Record https://www.drupal.org/node/2199185 /user #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available

Changing the list of entities in \Drupal\rest\test\CreateTest to use user gives a 500 error. (No entity aka user created).

 public function testCreate() {
    $serializer = $this->container->get('serializer');
    $entity_types = array('entity_test', 'node', 'user');
    foreach ($entity_types as $entity_type) {

@vivekvpandya maybe you can work from that test towards a patch?

klausi’s picture

Performing write operations on the user entity resource is usually an administrative task. Only administrators are allowed to create/update/delete arbitrary user accounts.

User registration is a special case where an unprivileged consumer can create their own account (and no other write operation!). To me that sounds like a different use case than user entity manipulation, therefore the idea to make it separate.

clemens.tolboom’s picture

@klausi thanks for the elaboration.

I had two use cases in mind: an App and a LDAP Rest client.

- The web app can easily use user/register as it's endpoint. (role: anonymous)
- The LDAP Rest client needs to set roles and other stuff for it's users too. (role: administer users)

Both do the same thing (creating a user) but have access to different fields.

Can't we solve this on field permission level?

klausi’s picture

We might, although a couple of settings come into play with anonymous user registration (blocking user until enabled by an admin, email confirmation etc.).

vivekvpandya’s picture

klausi is right as we have to include all those settings while creating a user.

vivekvpandya’s picture

but one thing I would like to add here it while creating a user with /entity/user end point if we pass "status":{ "value":"0" } explicitly then created user would be in blocked state . So I am thinking that we can impose certain setting while creating a user by supplying information with in JSON body for request.

clemens.tolboom’s picture

@vivekvpandya please your use case to the issue. It seems you are in need for an App with approval. But how about an Android App without approval? My LDAP sync will never work :-(

Maybe @klausi is right to have registration done by /entity/user/register and my LDAP sync by /entity/user

[Stock response from Dreditor templates and macros.]

Please update the issue summary by applying the template from http://drupal.org/node/1155816.

marthinal’s picture

Status: Active » Needs review
FileSize
182.16 KB
760 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,779 pass(es). View

I'm working to port DrupalGap module. Using the RestClient I have a 403 when I try to create a new user using Basic Auth as admin. I found that the field name is missing. Patch attached.

1) I'm not sure why I can create an entity user from php code but using the same admin user I cannot create a new user from rest. From php code the field name is enough.

2) So, we need to create a new endpoint (entity/user/register) and should be added to Rest module, right?

Thanks!

marthinal’s picture

FileSize
5.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,316 pass(es). View
973.29 KB

This is a first attempt. I'm using the patch from #1964034: Pass entity_type into Serializer via context because it was not possible to denormalize without entity_type into the context.

At this point I can register new users as anonymous.

I'm using this hal+json from FOO/entity/user/register

{
  "langcode": [
    {
      "value": "en"
    }
  ],
  "name": [
    {
      "value": "test127"
    }
  ],
  "mail": [
    {
      "value": "testUser@127.es"
    }
  ],
  "timezone": [
    {
      "value": "UTC"
    }
  ],
  "pass": [
    {
      "value": "test"
    }
  ]
}

I have no errors on my logs but I'm not sure why the rest client never finish the operation but I can verify that the user is created and 201 seems to be received as expected.

Image attached about that.

Missing filter that only mail, name and pass should be saved for this entity.

I hope that this is the good direction. :)

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -0,0 +1,78 @@
+    // @TODO only username, pass and email shoul be added here.
+    $entity->save();

So this looks dangerous. Where do you check field access so that unpriviledged users such as anonymous users cannot assign them arbitrary user roles?

Same for the status field on the user object, shouldn't they be blocked by default?

Please study the usual Drupal core user registration flow with forms and which settings are applied there, we basically need to do the same here.

marthinal’s picture

Assigned: Unassigned » marthinal

@klausi thanks for reviewing.

I continue working on it.

marthinal’s picture

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

It is now more integrated with the user settings form.

IMO the same validation should be added to the entity creation because for the moment if a username exists we receive a 500.

Berdir’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -0,0 +1,168 @@
+    // New username cannot exist.
+    $name_taken = (bool) \Drupal::service('entity.query')->get('user')
+      ->condition('uid', (int) $account->id(), '<>')
+      ->condition('name', $account->getUsername())
+      ->range(0, 1)
+      ->count()
+      ->execute();
+
+    if ($name_taken) {
+      throw new BadRequestHttpException(String::format('The username @name is already taken.', array('@name' => $account->getUsername())));
+    }
+
+    // New email cannot exist.
+    $mail_taken = (bool) \Drupal::service('entity.query')->get('user')
+      ->condition('uid', (int) $account->id(), '<>')
+      ->condition('mail', $account->getEmail())
+      ->range(0, 1)
+      ->count()
+      ->execute();
+
+    if ($mail_taken) {
+      throw new BadRequestHttpException(String::format('The email address @email is already registered', array('@email' => $account->getEmail())));
+    }

We should rely on $user->validate() instead. If this does not cover this at the moment, then that is simply a bug there.

I just checked and we have UserNameUnique and UserMailUnique validation plugins registered on the respective field definitions, so that is supposed to work...

marthinal’s picture

FileSize
2.52 KB
7.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,432 pass(es). View

@Berdir now it works. The problem is when I try to create a new user entity from /entity/user using basic auth (as admin user) because I cannot create "pass" field.

here is the exception:

    foreach ($entity as $field_name => $field) {
      if (!$field->access('create')) {
        throw new AccessDeniedHttpException(String::format('Access denied on creating field @field', array('@field' => $field_name)));
      }
    }

But this is a problem with the entity creation and not for registering new users. Maybe another issue?

Berdir’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -89,7 +89,7 @@ public function post(EntityInterface $entity = NULL) {
     foreach ($entity as $field_name => $field) {
       if (!$field->access('create')) {
-        throw new AccessDeniedHttpException(String::format('Access denied on creating field ', array('@field' => $field_name)));
+        throw new AccessDeniedHttpException(String::format('Access denied on creating field @field', array('@field' => $field_name)));
       }
     }

As discussed in IRC, the usage of 'create' here is IMHO wrong. fields don't use create/update, they only use edit.

marthinal’s picture

marthinal’s picture

FileSize
2.03 KB
8.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,142 pass(es). View

I think that the current resource should extends ResourceBase but we are using exactly the same validate method for EntityResource. Is it correct to duplicate this method here?

marthinal’s picture

Title: Creation of entity type user without credential is not working » Creation of entity type user without credential is not working (Create a Resource for user registration)
clemens.tolboom’s picture

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

Patch needed reroll on #19 failed "core/modules/serialization/src/Normalizer/EntityNormalizer.php"

clemens.tolboom’s picture

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

    No newline at end of file

  2. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -62,8 +62,17 @@ public function handle(RouteMatchInterface $route_match, Request $request, Route
    +        $context = array();
    +        // Get context information for deserialization from the plugin
    +        // definition.
    +        if (!empty($definition['serialization_context'])) {
    +          $context = $definition['serialization_context'];
    +        }
    +        // Always add the resource ID to the deserialization context.
    +        $context['resource_id'] = $plugin;
    +        $context['request_method'] = $method;
    

    This is taken from #1964034: Pass entity_type into Serializer via context? That patch has

    diff --git a/core/modules/rest/src/Plugin/Derivative/EntityDerivative.php b/core/modules/rest/src/Plugin/Derivative/EntityDerivative.php
    index f80694f..ec15fa3 100644
    --- a/core/modules/rest/src/Plugin/Derivative/EntityDerivative.php
    +++ b/core/modules/rest/src/Plugin/Derivative/EntityDerivative.php
    @@ -99,6 +99,9 @@ public function getDerivativeDefinitions($base_plugin_definition) {
               'id' => 'entity:' . $entity_type_id,
               'entity_type' => $entity_type_id,
               'serialization_class' => $entity_type->getClass(),
    +          'serialization_context' => array(
    +            'entity_type' => $entity_type->id(),
    +          ),
               'label' => $entity_type->getLabel(),
             );
     
    diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    index 7b3e49e..3d183cf 100644
    --- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -22,6 +22,9 @@
      *   id = "entity",
      *   label = @Translation("Entity"),
      *   serialization_class = "Drupal\Core\Entity\Entity",
    + *   serialization_context = {
    + *     "entity_type" = "entity"
    + *   },
      *   deriver = "Drupal\rest\Plugin\Derivative\EntityDerivative",
      *   uri_paths = {
      *     "canonical" = "/entity/{entity_type}/{entity}",
    

    Not sure that's not already done somewhere already.

  3. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
    @@ -45,7 +45,7 @@ public function __construct(EntityManagerInterface $entity_manager) {
    +      throw new UnexpectedValueException('Entity type parameter must be included in context.' . print_r($context, TRUE));
    

    Is print_r common practice?

    Does $context contain insecure data?

marthinal’s picture

FileSize
7.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,295 pass(es). View

1) Done

2) Well, this value is necessary. Otherwise we have this problem :
EntityNormalizer::denormalize()

    if (empty($context['entity_type'])) {
      throw new UnexpectedValueException('Entity type parameter must be included in context.');
    }

3) Debugging purposes :)

Thanks!

marthinal’s picture

For the moment we can add users without control using this resource. I've added the flood control for the Login Cookie resource here #2403307: RPC endpoints for user authentication: log in, check login status, log out.

I think we need to try the same here. More info https://www.drupal.org/node/2403307#comment-9624037

Berdir’s picture

I assume we're good, but I wanted to explicitly mention https://www.drupal.org/node/2344389 again, which was a services SA related to creating users, which generated weak passwords. I checked it off in #2419941: Check every Drupal 7 contrib SA that may affect Drupal 8 core modules as not relevant yet, but this issue will add it, so we need to make sure to not have the same problem. Which I assume we don't.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Annotation/RestResource.php
    @@ -43,4 +43,11 @@ class RestResource extends Plugin {
    +  /**
    +   * Options available.
    +   *
    +   * @var array
    +   */
    +  public $serialization_context;
    

    Options available? What options? Did you mean "Context settings that will be passed to the serializer"?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    + * Definition of Drupal\rest\Plugin\rest\resource\UserRegistrationResource.
    

    Should be "Contains ...", see https://www.drupal.org/coding-standards/docs#file

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +  /**
    +   * Responds to entity POST requests and saves the new entity.
    +   *
    

    Can we be a little less generic like "Register a new user account on POST requests."?

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   *   The entity.
    +   *
    

    why $entity? Shouldn't this always be a user entity, thus $user or $account?

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +    if ($entity == NULL) {
    +      throw new BadRequestHttpException('No entity content received.');
    +    }
    

    Should be less generic like "No user account data for registration received."

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +    $definition = $this->getPluginDefinition();
    +    // Verify that the deserialized entity is of the type that we expect to
    +    // prevent security issues.
    +    if ($entity->getEntityTypeId() != $definition['serialization_context']['entity_type']) {
    +      throw new BadRequestHttpException('Invalid entity type');
    +    }
    

    why do we need to check the entity type? I thought this can only be user entities? I think we can just hard-code "entity:user" here and don't need to look at serialization_context?

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +    // POSTed entities must not have an ID set, because we always want to create
    +    // new entities here.
    +    if (!$entity->isNew()) {
    +      throw new BadRequestHttpException('Only new entities can be created');
    +    }
    

    too generic, exception should be "Only new user accounts can be registered." Comment should also be adapted.

  8. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +    $approvalSettings = \Drupal::config('user.settings')->get('register');
    

    don't use \Drupal in classes where possible, the setting should be injected in the constructor. See RestPermissions.php with the create() method for an example.

  9. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +    // Username and email cannot exist.
    +    $this->validate($entity);
    

    what do you mean by cannot exist? Should the comment be "Make sure that user name and email are valid and allowed."?

  10. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,156 @@
    +        throw new BadRequestHttpException(String::format('Anonymous user cannot assign roles when registering a new user account and by default' .
    

    The String class is gone, you need to use SafeMarkup instead.

m1r1k’s picture

Assigned: marthinal » m1r1k

I will jump on it (need it for the project): reroll, phpunit and web test.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
14.55 KB
15.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,265 pass(es). View
vivekvpandya’s picture

I applied the latest patch by @m1r1k and then tried to create user as show in comment #11
But I am getting 406 .
Please some one suggest how to test this.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Annotation/RestResource.php
    @@ -43,4 +43,11 @@ class RestResource extends Plugin {
    +   * Options available.
    

    What options are available? Please be more specific.

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    + *   serialization_class = "Drupal\Core\Entity\Entity",
    + *   serialization_context = {
    + *     "entity_type" = "user"
    + *   }
    

    This should be the User entity class. And is the serialization context necessary? It will always be the user entity type.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    + * @see \Drupal\rest\Plugin\Derivative\EntityDerivative
    

    Why is this @see reference here, there seems to be no relation to that derivative?

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +   * Responds to entity POST requests and saves the new entity.
    

    That comment is too generic, should be something like "Responds to user registration POST request and saves new user account entities."

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +    // Verify that the deserialized entity is of the type that we expect to
    +    // prevent security issues.
    +    if ($account->getEntityTypeId() != 'user') {
    +      throw new BadRequestHttpException('Invalid entity type.');
    +    }
    

    That can be removed, once we only deserialize to the User entity.

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +      throw new BadRequestHttpException('Only new user accounts can be created.');
    

    Should be "... can be registered."

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +    // Cannot add extra roles.
    

    What does that comment mean? We cannot add extra user roles here or the client cannot submit additional user roles?

  8. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +      if ($role != 'authenticated' && $role != 'anonymous') {
    

    Why do you check for the anonymous role here, that role should never be added?

  9. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +    // "Verify email" option disabled and the account as active means that the user can be logged in now.
    

    Comments should wrap at 80 characters.

  10. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +      user_login_finalize($account);
    

    Why do we login the user here? I thought this resource would only register accounts? And this will only work for session cookie authentication, right?

  11. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +  /**
    +   * Implements ResourceInterface::routes().
    +   */
    +  public function routes() {
    

    Comment doc block should be {@inheritdoc}.

    We only have one route, so we don't need the foreach loop and the switch statement in there.

  12. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,199 @@
    +  protected function validate(ContentEntityBase $entity) {
    +    $violations = $entity->validate();
    

    This is missing the $violations->filterByFieldAccess(); same as in EntityResource.

  13. +++ b/core/modules/rest/src/RequestHandler.php
    --- /dev/null
    +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    

    I think we should also have an integration test for this, so that we make sure this works with real HTTP requests.

@vivekvpandya: you can test this with a POST request similar to https://www.drupal.org/node/2098511

droti’s picture

Is this currently working for you guys? I have applied the patch and I have enabled "User Registration" in the REST UI, then checked the permissions for "access POST on User resource" and I still keep getting a 403. I can create a user just fine if I'm actually logged in but I can't do it from an Anonymous user. Am I missing a step here?

vivekvpandya’s picture

@droti Are you making a POST request on /entity/user ? If yes then the behavior you mentioned is the same for me . But I think after applying this patch there should be a separate path for RESTful user registration.

droti’s picture

I am making a POST on /entity/user, I'll have to fish around me and see if I'm missing something and try and figure out if what the other path should be. Thanks.

droti’s picture

Actually I am able to make a post to /entity/user/register and it create a user successfully, but it returns a 500 Internal Server Error
Uncaught PHP Exception LogicException: "The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\rest\ResourceResponse." at drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php line 160

It appears like it is being caused by this line: _user_mail_notify('register_no_approval_required', $entity); In UserRegistrationResource.php

vivekvpandya’s picture

@droti Is 'post to /entity/user/register' working with anonymous user ?

droti’s picture

Yes, I am now able to to post to '/entity/user/register' and create a user. However I had to comment out that line _user_mail_notify('register_no_approval_required', $entity); in UserRegistrationResource.php to stop it from returning a 500 error.

marthinal’s picture

I've created a new issue about this error.

klausi’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -0,0 +1,199 @@
+    return new ResourceResponse(NULL, 201);

That response is missing chachability metadata, same as done in EntityResource post() for example.

marthinal’s picture

Assigned: m1r1k » Unassigned
FileSize
16.78 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,157 pass(es). View
2.7 KB

@klausi many thanks for your help. :)

Adding cacheability metadata.

Missing review #30

marthinal’s picture

Status: Needs work » Needs review
marthinal’s picture

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

Emails are not working as expected. Working on it.

marthinal’s picture

Status: Needs work » Needs review
FileSize
15.84 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
7.78 KB
17.61 KB
30.11 KB
11.53 KB

Working on #30

1. Removed.

2. Using User entity class.

3. Done.

4. Done.

5. Done.

6. Done.

7. Done.

8. Avoid to add roles other than anonymous or authenticated. IMHO this is correct. Anyways anonymous is added by default.



9. This comment was removed when refactoring this part.

10. I agree! Done

11. Done.

12. Done.

13. Missing integration tests.

I've rewritten the part where we send emails to the user and now:

1. If Visitors(no approval required) register a user and email verification is not required then we create the account but the user needs to log in or use basic_auth for example... Also if notifications when activating an account is enabled then the user receives an email predefined by default.

2. If Visitors(no approval required) register a user and email verification is required an email will be sent to the user with the link to activate the account.

3. If Visitors register a user and administrator approval is required then Welcome (awaiting approval) email will be sent to the user.

I've created a module where we send a code to the user email and this code is required when registering the account.

https://github.com/marthinal/registration_code
https://www.drupal.org/sandbox/marthinal/2559393

Status: Needs review » Needs work

The last submitted patch, 42: 2291055-42.patch, failed testing.

The last submitted patch, 42: 2291055-42.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
16.11 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,140 pass(es). View
4.38 KB

Ok let's try again.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
@@ -0,0 +1,212 @@
+ * Tests the REST export view plugin.

Hm, copy and paste error?

And can we have an integration test, meaning something extending WebTestBase like the other tests in REST module?

marthinal’s picture

@klausi Yes, sure. I can continue working on it this weekend during the sprints.

andypost’s picture

marthinal’s picture

Status: Needs work » Needs review
FileSize
19.25 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,599 pass(es). View
4.28 KB

Adding integration test.

Status: Needs review » Needs work

The last submitted patch, 49: 2291055-49.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +   * @param LoggerInterface $loggery
    

    "loggery"? Shouldn't this be called $logger as in the parent class?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +      $container->get('config.factory')
    

    I don't think we need the whole config factory here. We only need user.settings, right? So you should get that in create() and set that as instance variable on the class.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +   * Responds to user registration POST request and saves new user account
    +   * entities.
    

    The method summary should only be one line (shorter than 80 characters).

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +  public function post(User $account = NULL) {
    

    I think we are missing a check here that the current user is anonymous. Only anonymous users are allowed to register accounts, so we should inject the current user and check that.

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +    // If current user can register accounts then let's block the new registered user if admin approval is needed.
    

    Comments should be shorter than 80 characters, see https://www.drupal.org/coding-standards/docs#drupal

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +    $this->validate($account);
    

    Before validation we need to make sure that the user is allowed to submit all the fields that have been submitted. See the field access checks in EntityResource::post(). This is currently a security issue in the patch. When we do the field access checking we also don't need the role checks you are doing, since that is also covered by field access.

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +    $renderer = \Drupal::service('renderer');
    

    Do not call out to the global \Drupal, inject the renderer service instead.

  8. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +          $account->status = 1;
    

    Do not set the status field manuall, call ->activate() instead.

  9. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +    $response = new ResourceResponse(NULL, 201, ['Location' => 'user/' . $account->id()]);
    +    if (!$context->isEmpty()) {
    ...
    +    }
    

    I think this response it not cachable at all, so we should not have to call the addCacheableDependency() method. If we get an exception then we should file a separate issue that fixes responses which didn't specify cachability.

    And the Location should be an absolute URL, see EntityResource.

  10. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +    $route_name = strtr($this->pluginId, ':', '.');
    

    There is no colon in the plugin ID, so this is not necessary.

  11. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +    $collection->add("$route_name.POST", $route);
    

    The POST suffix is not necessary here since we only have one route here.

  12. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,200 @@
    +  protected function validate(ContentEntityBase $entity) {
    

    In this case we have a User entity, so we should use that for type hinting.

  13. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -61,8 +61,13 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    +        // Always add the resource ID to the deserialization context.
    +        $context['resource_id'] = $plugin;
    +        $context['request_method'] = $method;
    

    That should not be necessary for this patch, we are not using the resource_id anywhere?

  14. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +  public static $modules = array('hal');
    

    Let's use the short array syntax in new tests.

  15. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +   * Enables user registration specific REST API configuration and
    +   * authentication.
    

    Can we make this fit on one line?

  16. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +    $settings = array();
    

    short array syntax

  17. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +    // Log in as admin and verify that the new user exists.
    +    $this->drupalLogin($this->rootUser);
    +    $this->httpRequest('user/' . $id, 'GET');
    

    I think that is not necessary. We can make a DB query to ensure that the user was created and is there.

  18. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,212 @@
    +/**
    + * Only administrators can create user accounts.
    + */
    +define('USER_REGISTER_ADMINISTRATORS_ONLY', 'admin_only');
    

    This should only be defined if the constant is not already defined. Can happen if you run the phpunit tests from the UI.

marthinal’s picture

Status: Needs work » Needs review
FileSize
20.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,931 pass(es). View
17.38 KB

1. Done

2. Done

3. Done

4. Done

5. Done

6. Done. Trying to unit test the field access I found #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work.

7. Done.

8. Done

9. @TODO open new issue.

10. Done.

11. Done.

12. Done.

13. Removed. Done!

14. Done.

15. Done.

16. Done

17. Done.

18. So should be added? Otherwise the unit test will fail.

marthinal’s picture

Issue tags: +Barcelona2015
marthinal’s picture

FileSize
26.05 KB
28.09 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,809 pass(es), 25 fail(s), and 0 exception(s). View
9.81 KB

As discussed with @WimLeers and @klausi, we need to cache only GET requests. When sending the email and replacing tokens we get an exception (rendering content too early).

Let's try this patch.

Status: Needs review » Needs work

The last submitted patch, 55: 2291055-55.patch, failed testing.

The last submitted patch, 55: 2291055-55.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
25 KB
30.24 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,341 pass(es). View
6.09 KB

Now should work. I'm using the ResourceResponseTrait but maybe we could avoid the trait like this:

This is not the current structure used for the patch but not sure if the trait is the best option here.

marthinal queued 58: 2291055-58.patch for re-testing.

marthinal’s picture

Rerolled

kylebrowning’s picture

Ive tested this, I don't think there isn't a reason to call this RTBC.

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community
klausi’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -109,8 +110,8 @@ public function post(EntityInterface $entity = NULL) {
    +      //$response->addCacheableDependency($url);
    

    this line be removed?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,221 @@
    +   * @param \Drupal\Core\Config\Config $user_settings
    +   *   A user settings config instance.
    

    should be ImmutableConfig, that is what the config factoy returns.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,221 @@
    +   * @param \Drupal\Core\Render\RendererInterface $renderer
    +   *   The renderer.
    

    so there renderer service is unused and should be removed?

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,221 @@
    +   * @return \Drupal\rest\ResourceResponse
    +   *   The HTTP response object.
    

    but it returns a UncachanleResourceResponse, right?

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,221 @@
    +    $account->save();
    +    // Send emails from a render context to add bubbleable_metadata to the
    +    // response.
    +      $register = $this->userSettings->get('register');
    +      // No email verification is required. Activating the user.
    +    if ($register == 'visitors') {
    

    indentation errors.

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,221 @@
    +        $account->save();
    

    why do we save the account twice? can't we activate the user before the first save with the if() statement?

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,221 @@
    +    $response = new UncacheableResourceResponse(NULL, 201, ['Location' => 'user/' . $account->id()]);
    

    the location should use a generated URL, see EntityResource::post() for an example.

  8. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,221 @@
    +    $route = $this->getBaseRoute('/entity/user/register', 'POST');
    +    $route->setPath('/entity/user/register');
    

    why do we have to set the path again? shouldn't the getBaseRoute() call do it already?

  9. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -61,8 +61,11 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    +        $context = array();
    +        $context['request_method'] = $method;
    +
             try {
    -          $unserialized = $serializer->deserialize($received, $class, $format, array('request_method' => $method));
    +          $unserialized = $serializer->deserialize($received, $class, $format, $context);
    

    looks like this change is not needed?

  10. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -104,7 +107,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
    -    if ($response instanceof ResourceResponse && $data = $response->getResponseData()) {
    +    if ($response instanceof CacheableResourceResponse && $data = $response->getResponseData()) {
    

    but we should serialize CachableResourceResponses as well as UncachableResourceRespones. Those two should have a common interface that we should check here, something like "ResourceResponseInterface".

  11. +++ b/core/modules/rest/src/ResourceResponseTrait.php
    @@ -0,0 +1,29 @@
    +  public function getResponseData() {
    +    return $this->responseData;
    +  }
    +}
    

    missing newline after the last method closer.

  12. +++ b/core/modules/rest/src/ResourceResponseTrait.php
    @@ -0,0 +1,29 @@
    \ No newline at end of file
    

    missing newline at the end of file.

kylebrowning’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
29.02 KB

#1. Fixed
#2. Fixed
#3. the rendered is used, did I miss something?
#4. Fixed.
#5. Fixed.
#6. Looks like the `$account->save` is needed to because of the $account->block() up above.
#7. Fixed.
#8. Not Sure, I left this alone.
#9. It looks like it isn't, removed.
#10. Uhh. Left this alone.
#11. Fixed.
#12. Fixed.

So to recap, #3, #6, #8, and #10 need review.

Status: Needs review » Needs work

The last submitted patch, 64: 2291055-64.patch, failed testing.

marthinal’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
31.57 KB
12.23 KB

As commented on #55

When sending the email and replacing tokens we get an exception (rendering content too early).

I think at least this is a Major. I know we have to different issues here but we are fixing the problem here...

#3 Done (Removed).

#6 Yes. Added comment. We need to save the changes when the account is active.

#8 Yes, we don't need to setPath() again. The constructor of Route class does the same.

#10 Done

Let's take a look at test results! :-)

Status: Needs review » Needs work

The last submitted patch, 66: 2291055-66.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
31.58 KB
693 bytes

Hardcoded Base url . Fixed.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,207 @@
    +  /**
    +   * Constructs a new RestPermissions instance.
    +   *
    

    wrong class name.

  2. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +  /**
    +   * Enables user registration specific REST API configuration and authenticate.
    +   */
    +  protected function enableUserRegistrationConfiguration() {
    

    I think this can be done in the usual test setUp() method?

  3. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +    $resources = [
    +      ['type' => 'rest_user_registration', 'method' => 'POST'],
    +      ['type' => 'entity:user', 'method' => 'GET']
    +    ];
    

    why do we need the entity:user resource here? I think it is not needed, we are only testing user registration?

  4. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +          "value" => "druplicon@superpoweredbydrupal.com"
    

    let's use example.com as test domain.

  5. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +    // Create a JSON version for the user entity we want to create.
    +    $serialized = \Drupal::service('serializer')->serialize($data, 'hal_json');
    

    don't use \Drupal here, use $this->container->get('serializer'); instead.

  6. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,109 @@
    +    $this->assertTrue((bool) \Drupal::entityQuery('user')
    

    same here, don't use \Drupal in classes where possible.

  7. +++ b/core/modules/rest/src/UncacheableResourceResponse.php
    @@ -0,0 +1,30 @@
    \ No newline at end of file
    

    newline at the end of file missing.

  8. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,247 @@
    +/**
    + * Only administrators can create user accounts.
    + */
    +define('USER_REGISTER_ADMINISTRATORS_ONLY', 'admin_only');
    +
    +/**
    + * Visitors can create their own accounts.
    + */
    +define('USER_REGISTER_VISITORS', 'visitors');
    +
    +/**
    + * Visitors can create accounts, but they don't become active without
    + * administrative approval.
    + */
    +define('USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL', 'visitors_admin_approval');
    

    this constants should be defined conditionally, since it might already be defined by user.module.

  9. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,247 @@
    +/**
    + * Tests User Registration REST resource.
    + *
    + * @group rest
    + */
    +class UserRegistrationResourceTest extends UnitTestCase {
    

    should use @coversDefaultClass

  10. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,247 @@
    +  protected $testClass;
    +  protected $reflection;
    +  protected $userSettings;
    +  protected $logger;
    +  protected $currentUser;
    +  protected $renderer;
    

    docs missing for those variables.

  11. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,247 @@
    +    $this->logger = $this->getMock('Psr\Log\LoggerInterface');
    

    use LoggerInterface::class instead of the class string. Also elsewhere.

  12. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,247 @@
    +  public function getProtectedMethod($method) {
    

    docs missing.

  13. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,247 @@
    +  public function testValidate() {
    

    docs missing.

marthinal’s picture

1. Done

2. Done.

3. Yep. I think we wanted to request for the user from REST but we're using the API.

4. Done.

5. Done.

6. Done.

7. Done.

8. Done.

9. Done.

10. Done.

I cannot continue today...

marthinal’s picture

Status: Needs work » Needs review
FileSize
32.64 KB
11.39 KB

And the last three

11. Done

12. Done

13. Done

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community

Tested again, still working.

The last submitted patch, 64: 2291055-64.patch, failed testing.

The last submitted patch, 66: 2291055-66.patch, failed testing.

kylebrowning’s picture

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Barcelona2015

Only code standards and nits.

  1. +++ b/core/modules/rest/src/CacheableResourceResponse.php
    @@ -0,0 +1,35 @@
    +
    +
    

    Remove extra line.

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,207 @@
    +      throw new BadRequestHttpException('Only new user accounts can be registered.');
    

    Maybe this exception message needs to be more clear, explaining that an ID has been passed?

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,207 @@
    +    // Only check 'edit' permissions for fields that were actually
    +    // submitted by the user. Field access makes no difference between 'create'
    +    // and 'update', so the 'edit' operation is used here.
    

    Wrapping inconsistency: "submitted by" can go on the first line. Rearrange next lines.

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,207 @@
    +    // No email verification is required. Activating the user.
    

    We are using "E-mail", not "email".

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,207 @@
    +    // Restrict the incoming HTTP Content-type header to the known
    +    // serialization formats.
    

    Seems that the 1st line can hold one more word?

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,207 @@
    +    $route->addRequirements(array('_content_type_format' => implode('|', $this->serializerFormats)));
    

    Let's use square brackets for array represenatation.

  7. +++ b/core/modules/rest/src/UncacheableResourceResponse.php
    @@ -0,0 +1,30 @@
    +  public function __construct($data = NULL, $status = 200, $headers = array()) {
    

    Let's use [] format for array.

  8. +++ b/core/modules/rest/src/UncacheableResourceResponse.php
    @@ -0,0 +1,30 @@
    +}
    \ No newline at end of file
    

    Add en empty line at the end of the file.

  9. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,308 @@
    +      ->will($this->returnValue(array()));
    

    [] instead of array()

  10. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,308 @@
    +    $method->invokeArgs($this->testClass, array($entity));
    

    [$entity] instead of array($entity)

  11. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,308 @@
    +  /**
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\HttpException
    +   * @expectedException UserRegistrationResourceTest::ERROR_MESSAGE
    +   */
    

    It would be nice to add a description to this docblock.

  12. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,308 @@
    +  /**
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
    +   * @expectedExceptionMessage No user account data for registration received.
    +   */
    ...
    +  /**
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
    +   * @expectedExceptionMessage Only new user accounts can be registered.
    +   */
    ...
    +  /**
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    +   * @expectedExceptionMessage Only administrators can register users.
    +   */
    ...
    +  /**
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    +   * @expectedExceptionMessage Only anonymous users can register users.
    +   */
    ...
    +  /**
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    +   * @expectedExceptionMessage Access denied on creating field 'test_field'.
    +   */
    

    Same, needs few words as description.

kylebrowning’s picture

FileSize
32.62 KB
4.36 KB

1. Done.
2. Done
3. This is actually taken from Entity resource, line 96, but Ive fixed it.
4. Done.
5. This is from line 114 and 121 from ResourceBase.php, but Ive fixed it.
6. This is actually taken from ResourceBase.php as well, lines 116 and 123. But Ive Fixed it.
7. Fixed. Again, taken from other places.
8. Fixed.
9. All over Drupal 8 this line exists, It doesn't look like we've set a standard on array() versus []. I left this one alone unfortunately because of that.
10. Fixed.

I left 11 and 12 alone as I dont have the knowledge on what some of those are doing.

kylebrowning’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 77: 2291055-77.patch, failed testing.

The last submitted patch, 64: 2291055-64.patch, failed testing.

The last submitted patch, 66: 2291055-66.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
33.01 KB
3.28 KB

Fixed problems with test + #76.11 and #76.12

heykarthikwithu’s picture

+   * @param LoggerInterface $logger
+   *   A logger instance.

Should have "@param \Psr\Log\LoggerInterface $logger" as parameter type.

marthinal’s picture

@heykarthikwithu thanks :)

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community

I think all the nits are covered. Marking as RTBC.

kylebrowning’s picture

dawehner’s picture

Can someone update the issue summary for example with the implemented solution?

dawehner’s picture

IMHO its pretty confusing that POST on /entity/user could not be extended to provide the same kind of functionality as our new endpoint does.

kylebrowning’s picture

Creating a user is different than registering one.

dawehner’s picture

Issue tags: +Needs summary

Well yeah this is why I'm asking for an issue summary :)

The last submitted patch, 64: 2291055-64.patch, failed testing.

The last submitted patch, 66: 2291055-66.patch, failed testing.

The last submitted patch, 77: 2291055-77.patch, failed testing.

kylebrowning’s picture

Issue summary: View changes

Okay updated.

kylebrowning’s picture

Issue summary: View changes

Removing leftover example text.

clemens.tolboom’s picture

In #3 I asked the same about /user.

Now the summary says

POST request on http://mysite.com/entity/user get a 403, and rightfully so because creating users...

I still don't get that but OK do we have /user/register but this patch uses /entity/user/register. Why?

The summary could definitely need some steps to test.

kylebrowning’s picture

@clemens.tolboom I believe they were separated because they are two different things.

Admins can create users whether site registration is turned on or off.
Users can register users if site reg is turned on.

How would you propose we incorp that into `/user`

marthinal’s picture

marthinal’s picture

@kylebrowning thanks for updating the summary

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Feature request

I can't see how this is a bug - but I can see how this will be useful. It is a new bc compatible feature, for me, and so can only go in 8.1.x. I have not reviewed the patch yet.

kylebrowning’s picture

Title: Creation of entity type user without credential is not working (Create a Resource for user registration) » Create a Resource for user registration so that anonymous users can register.

Updating title based on new category.

marthinal’s picture

@alexpott Yes, we have the feature + the bug described in #55

alexpott’s picture

@marthinal I'm asking this question without fully understanding the issue - but is there anyway that the bug fix and the feature can be separated? Looking at #55 and the issue summary it is not particular clear what bug is being fixed.

marthinal’s picture

@alexpott I agree that probably we should separate in 2 different issues. At at DCon we decided to fix the bug here...

Basically when creating the user we send an email. Building this email we replace the placeholders and don't remember 100% the place but we are rendering, and then we always receive this error:

Uncaught PHP Exception LogicException: "The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\rest\\ResourceResponse.

I was searching for a solution with @Wim Leers and @klausi and the conclusion was that we don't need to cache POST, DELETE and PATCH requests.

So, if you agree I can create a new issue with a new integration test where for example I render an array from a new REST Resource to reproduce the problem.

Using something like "\Drupal::service('renderer')->render($render_array);". And then remove ResourceResponse and add the new responses(Uncacheable...). Well... remove this part from the current patch :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@marthinal I think we should fix the caching of POST, DELETE and PATCH requests before adding this feature. That way other features or bugs that need the same fix can get it earlier.

Setting to "needs work" to get a new issue created for the bug fix. ONce that is done, this issue should be postponed on the new issue.

marthinal’s picture

marthinal’s picture

Status: Needs work » Postponed
Issue tags: +SprintWeekend2016
FileSize
20.24 KB
12.78 KB

We need to fix #2626298: REST module must cache only responses to GET requests before run the testbot here. Removing UncacheableResource response, etc...

droti’s picture

302 Redirect

Guys I have a wierd problem, I have this workign on my local machine but on my staging server I run into a wierd problem. When I do the post to /entity/user/register I get a 302 redirect and it changes my POST to a GET. You can see the error below:

MLHttpRequest cannot load http://staging.server/entity/user/register. The request was redirected to 'http://staging.server/entity/user/register', which is disallowed for cross-origin requests that require preflight.

Has anyone seen anything like this before? I can't seem to figure out how to get this to stop.

I should note my webapp is on the same domain as the Drupal site, but they are on separate ports (I don't know if that matters).

droti’s picture

Actually I found the problem, I had a double // and it was redirecting to remove it.

droti’s picture

Has anyone tried this with the latest drupal update 8.04? After updating I can no longer register users, I get a 415 error returned to me. "Response for preflight has invalid HTTP status code 415"

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.

cloudbull’s picture

Not working here too.

Tried
POST /user (Access denied)
POST /user/register (Access denied)
POST /entity/user (Access denied)
POST /entity/user/register (Access denied)

While in log message, it shows the user is admin for those access denied report.

In RESTUI, cannot found user registration resource, only USER GET/POST/Delete
this screenshot's user register path not found. https://www.drupal.org/files/issues/Screen%20Shot%202015-01-07%20at%2015...

Using 8.0.5

thanks
Keith

vivekvpandya’s picture

FileSize
9.11 KB

@cloudbull If you are using any REST client I think you should use http basic_auth instead of cookie method for authentication. Also you need to give enough permission to POST on user registration resource, which will be available if patch for this issue is applied correctly.
user_reg permission

cloudbull’s picture

Only local images are allowed.@vivekvpandya the log show current user is admin, so i think the basic auth is applied. Im using postman and applied basic auth.

the admin user is uid1, so i think no other permission need to set ?

Update: I can enable REST user register resource, but that is access denied.

the message is

You are not authorized to access this page. while the return is html instead of json.

cloudbull’s picture

Sorry, one more question.

I checked the patch, it will say

// The current resource only allows anonymous users to register users.
if (!$this->currentUser->isAnonymous()) {
throw new AccessDeniedHttpException('Only anonymous users can register users.');
}

But REST need basic auth to access anyway.

How can i get passed this line of code?

If comment

// The current resource only allows anonymous users to register users.
// if (!$this->currentUser->isAnonymous()) {
// throw new AccessDeniedHttpException('Only anonymous users can register users.');
// }

user can create but 500 error responded. Debug line by line and found the 500 from this

$response = new UncacheableResourceResponse(NULL, 201, ['Location' => $url->getGeneratedUrl()]);

vivekvpandya’s picture

@cloudbull Actually purpose for this REST endpoint is to provide a way for Anonymous user to create their account that is why I suggest you to grant permission to anonymous user and then try with out any credentials.
If you want to create user with admin credentials then you have use POST on user resource. That is the same thing as an admin creates an account for user with the Drupal's Admin panel. if you want error in JSON format you may try Accept: application/json.

cloudbull’s picture

if changing to ResourceResponse, the error is changed. Is that not possible to use ResourceResponse in user register ?

The website encountered an unexpected error. Please try again later.LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\rest\ResourceResponse. in Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (line 159 of core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php).

Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 139)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 62)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 62)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 103)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 55)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 637)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
vivekvpandya’s picture

sorry @cloudbull I can't help you further. You need help from some one who is actually working/ has worked on this issue.

marthinal’s picture

@cloudbull This is a reroll using NonCacheableResourceResponse. So you should apply #2626298: REST module must cache only responses to GET requests and then I can confirm that you can register users.

droti’s picture

@Marthinal, I just updated drupal and applied your latest patch but I get "Type /drupal/rest/type/user/user does not correspond to an entity on this site." in response. Now I wasn't able to apply the latest patch from NonCacheableReoursce because I get this error:

error: patch failed: core/modules/rest/src/Plugin/rest/resource/EntityResource.php:115
error: core/modules/rest/src/Plugin/rest/resource/EntityResource.php: patch does not apply
error: core/modules/rest/tests/src/Kernel/RequestHandlerTest.php: No such file or directory


Edit: Ignore all of that - It looks like the issue was related to be changing the domain (form an ip address), I'm still having problems, but I'm working them out.

Wim Leers’s picture

Title: Create a Resource for user registration so that anonymous users can register. » [PP-2] Create a Resource for user registration so that anonymous users can register.
Issue summary: View changes
Wim Leers’s picture

Title: [PP-2] Create a Resource for user registration so that anonymous users can register. » [PP-3] Create a Resource for user registration so that anonymous users can register.
Issue summary: View changes
Related issues: +#2419825: Make serialization_class optional
Wim Leers’s picture

Issue tags: +Needs change record

This will also need a change record.

Wim Leers’s picture

Better title, that also is in line with #2403307: RPC endpoints for user authentication: log in, check login status, log out's title, so that it's clearer why this one should land after that one: because without the other one: A) being able to register is pointless, B) this has fewer related resources to be in line with.

Wim Leers’s picture

Title: [PP-3] Create a Resource for user registration so that anonymous users can register. » [PP-3] REST resources for anonymous users: register

Here's the actual better title.

clemens.tolboom’s picture

What does PP mean and the numbers PP-1 PP-2 etc?

Wim Leers’s picture

"PP" = PostPoned
"PP-1" = PostPoned on 1 issue.
"PP-2" = PostPoned on 2 issues.

"PP-N" = PostPoned on N issues.

In other words, this allows us to see at a glance when browsing the REST module issue queue (https://www.drupal.org/project/issues/drupal?component=rest.module) which issues are blocked and how many issues must be resolved before we can fix these. This helps us to determine what to work on next, and to hopefully guide multiple people towards the same issues, so those issues actually get fixed, rather than many issues getting worked on small ways that are insufficient to resolve them.

clemens.tolboom’s picture

Thanks for the explanation. Good to know.

dawehner’s picture

Issue summary: View changes

Well, there is already a workaround for logging in and logging out. Its just really unpleasant to encode the login form data. Given that its conceptually something one could work on already

Wim Leers’s picture

#129: but the approach for this patch was in line with the previous approach for #2403307: RPC endpoints for user authentication: log in, check login status, log out. Now that that approach has changed, this one should too. That's why I postponed this issue on that one: to avoid unnecessary work here.

dawehner’s picture

Title: [PP-3] REST resources for anonymous users: register » [PP-1] REST resources for anonymous users: register

Given that /user/register is an actual REST operation, as you post an entire user, with various bits and pieces, and for that, you can actually use other formats etc., I think there is much less of a discussion about it.

Wim Leers’s picture

Title: [PP-1] REST resources for anonymous users: register » REST resources for anonymous users: register
Status: Postponed » Needs work

Ok, cool! Happy to be wrong!

dawehner’s picture

Sorry for my harsh words ... So yeah this is not blocked at all, cool!

marthinal’s picture

Status: Needs work » Needs review
FileSize
19.98 KB
1.43 KB

Replacing NonCacheableResourceResponse with our new ModifiedResourceResponse.

dawehner’s picture

Just a quick review ...

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,202 @@
    +
    +    // The current resource only allows anonymous users to register users.
    +    if (!$this->currentUser->isAnonymous()) {
    +      throw new AccessDeniedHttpException('Only anonymous users can register users.');
    +    }
    

    Does this makes sense conceptually? Admin users can create new users at the moment already

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,202 @@
    +    $url = $account->urlInfo('canonical', ['absolute' => TRUE])->toString(TRUE);
    

    urlInfo is deprecated already, let's use toUrl instead

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,202 @@
    +    $route = $this->getBaseRoute('/entity/user/register', 'POST');
    

    I guess we want to use /user/register now?

  4. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,104 @@
    +
    +
    

    Nitpick: two empty lines

  5. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,104 @@
    +  protected function testRegisterUser() {
    

    Let's use a public function, otherwise we need to convert it to a public once we use a BrowserTestBase

  6. +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,315 @@
    +    $this->userSettings = $this->getMockBuilder(ImmutableConfig::class)
    +      ->setMethods(['get'])
    +      ->disableOriginalConstructor()
    +      ->getMock();
    +
    +    $this->currentUser = $this->getMockBuilder(AccountInterface::class)
    +      ->disableOriginalConstructor()
    +      ->getMock();
    

    I could imagine that using prophecy based mocking might make things much better here in general.

Status: Needs review » Needs work

The last submitted patch, 134: 2291055-134.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
18.89 KB
11.33 KB

Thank you @dawehner

1) Ok I see what you mean but... IMHO the admin user should use the User resource. I mean, we have Content, User, File resources... so probably this message makes sense in that context(an anonymous user wants to register a new user account).

    // The current resource only allows anonymous users to register users.
    if (!$this->currentUser->isAnonymous()) {
      throw new AccessDeniedHttpException('Only anonymous users can register users.');
    }
    $approvalSettings = $this->userSettings->get('register');

    // Verify that the current user can register a user account.
    if ($approvalSettings == USER_REGISTER_ADMINISTRATORS_ONLY) {
      throw new AccessDeniedHttpException('Only administrators can register users.');
    }

Probably it is a little bit confusing I agree. Only administrators can register users could be replaced with You cannot register a new user account or something like that...

2) Done.

3) Done.

4) Done.

5) Done.

6) Done.

Status: Needs review » Needs work

The last submitted patch, 137: 2291055-137.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
18.44 KB
1.72 KB

Adding the uri_paths(Annotation) should be enough.

dawehner’s picture

Just another quick review.

Probably it is a little bit confusing I agree. Only administrators can register users could be replaced with You cannot register a new user account or something like that...

That's a good idea!

  1. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,103 @@
    +  public function setUp() {
    +    parent::setUp();
    +    // Enabling services for the user registration and GET the new user entity
    +    // created.
    +    $config = $this->config('rest.settings');
    +    $settings = [];
    +    $resources = [
    +      ['type' => 'rest_user_registration', 'method' => 'POST'],
    +    ];
    +    $format = 'hal_json';
    +    $auth = $this->defaultAuth;
    +    foreach ($resources as $resource) {
    +      $settings[$resource['type']][$resource['method']]['supported_formats'][] = $format;
    +      $settings[$resource['type']][$resource['method']]['supported_auth'] = $auth;
    +      $config->set('resources', $settings);
    +      $config->save();
    +    }
    +    $this->rebuildCache();
    

    Can we leverage the enableService helper method for that?

  2. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,103 @@
    +    ];
    +
    +    // Create a JSON version for the user entity we want to create.
    +    $serialized = $this->container->get('serializer')->serialize($data, 'hal_json');
    +
    +    // Post to the REST service to register the user.
    +    $this->httpRequest('/user/register', 'POST', $serialized, 'application/hal+json');
    +    $this->assertResponse('201', 'HTTP response code is correct.');
    +
    +    // Obtain the uid from the header.
    +    $url_parts = explode('/', $this->drupalGetHeader('location'));
    +    $id = end($url_parts);
    +    $this->assertHeader('Location', $GLOBALS['base_url'] . '/user/' . $id);
    +
    +    $this->assertTrue((bool) $this->container->get('entity.query')
    +      ->get('user')
    +      ->condition('name', 'Druplicon')
    +      ->range(0, 1)
    +      ->count()
    +      ->execute(), 'The user was created as expected');
    +  }
    

    IMHO it would be nice to also have tests for the access permission test.

Wim Leers’s picture

Status: Needs review » Needs work

This is looking great already :) Lots of nits, and a few small refactor things that will make the code more maintainable:

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    + *   id = "rest_user_registration",
    

    Nit: s/rest_user_registration/user_registration/

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    + *   label = @Translation("User Registration"),
    

    Nit: s/User Registration/User registration/

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +   * @var \Drupal\Core\Config\Config
    

    Should be \Drupal\Core\Config\ImmutableConfig.

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +   * Current user object.
    

    Nit: The current user.

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +   * @throws \Symfony\Component\HttpKernel\Exception\HttpException
    ...
    +      throw new BadRequestHttpException('An ID has been set and only new user accounts can be registered.');
    ...
    +      throw new AccessDeniedHttpException('Only anonymous users can register users.');
    

    Let's list the specific exceptions.

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +    // POSTed user accounts must not have an ID set, because we always
    +    // want to create new entities here.
    

    Nit: 80 cols.

  7. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +    // Only check 'edit' permissions for fields that were actually submitted by
    +    // the user. Field access makes no difference between 'create'and 'update',
    +    // so the 'edit' operation is used here.
    +    foreach ($account->_restSubmittedFields as $key => $field_name) {
    +      if (!$account->get($field_name)->access('edit')) {
    +        throw new AccessDeniedHttpException("Access denied on creating field '$field_name'.");
    +      }
    +    }
    ...
    +  /**
    +   * Verifies that the whole entity does not violate any validation constraints.
    +   *
    +   * @param \Drupal\user\UserInterface $entity
    +   *   The entity object.
    +   *
    +   * @throws \Symfony\Component\HttpKernel\Exception\HttpException
    +   *   If validation errors are found.
    +   */
    +  protected function validate(UserInterface $entity) {
    +    $violations = $entity->validate();
    +
    +    // Remove violations of inaccessible fields as they cannot stem from our
    +    // changes.
    +    $violations->filterByFieldAccess();
    +
    +    if ($violations->count() > 0) {
    +      $message = "Unprocessable Entity: validation failed.\n";
    +      foreach ($violations as $violation) {
    +        $message .= $violation->getPropertyPath() . ': ' . $violation->getMessage() . "\n";
    +      }
    +      // Instead of returning a generic 400 response we use the more specific
    +      // 422 Unprocessable Entity code from RFC 4918. That way clients can
    +      // distinguish between general syntax errors in bad serializations (code
    +      // 400) and semantic errors in well-formed requests (code 422).
    +      throw new HttpException(422, $message);
    +    }
    +  }
    

    This is copy/pasted from EntityResource.

    Rather than duplicating the code, which means we have to keep both up-to-date and in sync, it'd be better to put this in a trait that both resources can use.

  8. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +    // Make sure that the user entity is valid (email and name are valid).
    ...
    +    // No E-mail verification is required. Activating the user.
    

    Nit: inconsistent spelling of "e-mail".

  9. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +    $response = new ModifiedResourceResponse(NULL, 201, ['Location' => $url->getGeneratedUrl()]);
    +
    +    return $response;
    

    These can be combined.

    Also, let's do a 200 response and send the created user object back.

    So:

    return new ModifiedResourceResponse($account, 200);
    
  10. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,103 @@
    +  /**
    +   * Modules to install.
    +   *
    +   * @var array
    +   */
    

    Just use </code>{@inheritdoc}.

  11. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,103 @@
    +          "type" => ["href" => $GLOBALS['base_url'] . "/rest/type/user/user"]
    

    Let's not use globals.

dawehner’s picture

Rather than duplicating the code, which means we have to keep both up-to-date and in sync, it'd be better to put this in a trait that both resources can use.

That's indeed something also jsonapi for example can use.

marthinal’s picture

Status: Needs work » Needs review
FileSize
23.31 KB
15.45 KB

140
Ok! Now the response contains You cannot register a new user account

140.1 Done

140.2 Done

141.1 Done

141.2 Done

141.3 Done

141.4 Done

141.5 Done.

141.6 Done.

141.7 Done.

141.8 Done.

141.9 Done. Ok so probably we should change the EntityResource response new ModifiedResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]); ??

141.10 Done.

141.11 Done.

Thanks!!

Status: Needs review » Needs work

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

marthinal’s picture

Status: Needs work » Needs review
FileSize
11.24 KB
22.61 KB

Fixed unit tests.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -93,17 +96,11 @@ public function post(EntityInterface $entity = NULL) {
         $this->validate($entity);
    +
         try {
    
    @@ -112,8 +109,8 @@ public function post(EntityInterface $entity = NULL) {
    -      return $response;
    +
    +      return new ModifiedResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
    
    @@ -174,6 +171,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
         $this->validate($original_entity);
    +
         try {
    

    Unnecessary whitespace changes.

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,154 @@
    + * Represents user registration as resource.
    

    Nit: s/as resource/as a resource/

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,154 @@
    +   * @param \Drupal\Core\Config\ImmutableConfig $user_settings
    +   *   A user settings config instance.
    +   * @param \Drupal\Core\Session\AccountInterface $current_user
    +   *   A user settings config instance.
    

    The second comment is wrong.

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,154 @@
    +    if ($account == NULL) {
    

    ===

  5. +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,58 @@
    +  protected function validate(EntityInterface $entity) {
    +
    +    $violations = $entity->validate();
    

    Nit: This whitespace didn't exist yet, let's not introduce it here.

  6. +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,58 @@
    +      throw new HttpException(422, $message);
    

    Let's throw \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException instead.

  7. +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,58 @@
    +  protected function fieldsAccess($entity) {
    

    I'd call this checkFieldAccess.

    Also: typehint is missing.

  8. +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,58 @@
    \ No newline at end of file
    

    Nit.

  9. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,99 @@
    + * Tests User Registration.
    

    Tests user registration.

  10. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,99 @@
    +    // Add registration permission to anonymous user.
    ...
    +    // Add registration permission to authenticated user.
    

    Nit: s/Add/Grant/

  11. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,99 @@
    +   * Tests user registration from REST.
    

    Weird sentence.

  12. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,99 @@
    +    // Create a JSON version for the user entity we want to create.
    +    $serialized = $this->container->get('serializer')->serialize($data, 'hal_json');
    

    s/JSON/HAL+JSON/

  13. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,99 @@
    +    // Verify that an authenticated user cannot register a new user using the
    +    // user_registration REST resource.
    +    $user = $this->createUser();
    +    $this->drupalLogin($user);
    +
    +    // Post to the REST service to register the user.
    +    $this->httpRequest('/user/register', 'POST', $serialized, 'application/hal+json');
    +    $this->assertResponse('403', 'Only anonymous users can register users.');
    +
    +    $this->drupalLogout();
    

    I'd keep the first comment, remove the comment in the middle, and remove all blank lines. Because this whole sequence represents one test. And the comment in the middle is not entirely accurate.

    The first comment is also not entirely accurate, this would be better:

    // Verify that an authenticated user cannot register a new user.
    
  14. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,99 @@
    +    // Post to the REST service to register the user.
    

    This comment is also not entirely accurate. Let's make it consistent with the one above:
    // Verify that an anonymous user can register.

  15. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,99 @@
    +    $this->assertResponse('200', 'HTTP response code is correct.');
    +    ¶
    +    $this->assertTrue((bool) $this->container->get('entity.query')
    

    Nit: trailing spaces.

  16. +++ b/core/modules/rest/tests/src/Unit/ResourceValidationTraitTest.php
    @@ -0,0 +1,74 @@
    +  /**
    +   * Tests that the user entity does not violate any validation constraints.
    +   */
    

    Let's just change this to:

    /**
     * @covers ::validate
     */
    
  17. +++ b/core/modules/rest/tests/src/Unit/ResourceValidationTraitTest.php
    @@ -0,0 +1,74 @@
    +  public function testValidate() {
    +
    +    $trait = $this->getMockForTrait('Drupal\rest\ResourceValidationTrait');
    

    Nit: unnecessary whitespace.

  18. +++ b/core/modules/rest/tests/src/Unit/ResourceValidationTraitTest.php
    @@ -0,0 +1,74 @@
    +   * Tests that error validation is thrown as expected.
    

    Again:

    @covers ::validate
    
  19. +++ b/core/modules/rest/tests/src/Unit/ResourceValidationTraitTest.php
    @@ -0,0 +1,74 @@
    \ No newline at end of file
    

    Nit.

  20. +++ b/core/modules/simpletest/src/WebTestBase.php
    @@ -2680,7 +2680,7 @@ protected function prepareRequestForGenerator($clean_urls = TRUE, $override_serv
    -   *   An absolute URL stsring.
    +   *   An absolute URL string.
    

    Hah, great catch :D

Fons Vandamme’s picture

Thank you for your work on this, I will try and test this patch this week. Good job so far!

marthinal’s picture

Status: Needs work » Needs review
FileSize
21.85 KB
8.17 KB

1. Done.

2. Done.

3. Done.

4. Done.

5. Done.

6. Done.

7. Done.

8. Done

9. Done.

10. Done.

11. Done.

12. Done.

13. Done.

14. Done.

15. Done.

16. Done.

17. Done.

18. Done.

19. Done.

20. Yeahh :D

Thank you @Wim!

Status: Needs review » Needs work

The last submitted patch, 148: 2291055-148.patch, failed testing.

Wim Leers’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -93,14 +96,7 @@ public function post(EntityInterface $entity = NULL) {
    +    $this->fieldsAccess($entity);
    

    You forgot to update this, hence the fails :)

  2. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,94 @@
    +    // Grant registration permission to anonymous user.
    ...
    +    // Grant registration permission to authenticated user.
    

    Nit: actually these comments don't seem valuable at all? I think we can remove them.

  3. +++ b/core/modules/rest/src/Tests/RegisterUserTest.php
    @@ -0,0 +1,94 @@
    +    // Verify that an authenticated user cannot register a new user.
    +    $user = $this->createUser();
    +    $this->drupalLogin($user);
    +    $this->httpRequest('/user/register', 'POST', $serialized, 'application/hal+json');
    +    $this->assertResponse('403', 'Only anonymous users can register users.');
    +    $this->drupalLogout();
    

    Much clearer!

    Except now that I think about it, we granted authenticated users the permission to register new users, above in setUp(). So why exactly is this the expected behavior?

marthinal’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
21.77 KB

1. Oops!! Sorry

2. Done.

3. Yes. Because we want to verify that only anonymous users can register new user accounts and... authenticated users could have access to the "registration resource".

    Role::load(RoleInterface::AUTHENTICATED_ID)
      ->grantPermission('restful post user_registration')
      ->save();

thanks!

Wim Leers’s picture

#151.3: Hm I don't fully understand that.

Can we then change

// Verify that an authenticated user cannot register a new user.

to:

// Verify that an authenticated user cannot register a new user, despite being
// granted permission to do so because [EXPLANATION HERE].
yasin-ali’s picture

user error

Wim Leers’s picture

Status: Needs review » Needs work
marthinal’s picture

Status: Needs work » Needs review
    // Verify that an authenticated user cannot register a new user, despite
    // being granted permission to do so because only anonymous users have this
    // possibility.

This resource should be used by anonymous users to register a new account.

For CRUD operations we should use the User resource.

This is my humble opinion.

@Wim I can add this comment... but I prefer to wait for your opinion.

Wim Leers’s picture

Status: Needs review » Needs work

Your explanation makes sense. But it's not clearly reflected in the comment.

I think this is clearer:

    // Verify that an authenticated user cannot register a new user, despite
    // being granted permission to do so because only anonymous users can
    // register themselves, authenticated users with the necessary permissions
    // can POST a new user to the "user" REST resource.

And I now see where the logic determining this behavior lives:

+++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -0,0 +1,154 @@
+    // The current resource only allows anonymous users to register users.
+    if (!$this->currentUser->isAnonymous()) {
+      throw new AccessDeniedHttpException('Only anonymous users can register users.');
+    }

I think the comment there should be changed to:

  // Only allow anonymous users to register, authenticated users with the necessary
  // permissions can POST a new user to the "user" REST resource.
  // @see \Drupal\rest\Plugin\rest\resource\EntityResource

Then both the logic implementing it and the test expectations are clearly documented. I think that will be better for long-term maintenance.


Thanks for your clear explanations and patience!

The last submitted patch, 107: 2291055-107-do-not-test.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
22.31 KB

Sure, makes sense to improve these comments.

Ok, Done.

Thank you for your great reviews and your help!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 158: 2291055-158.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Issue tags: +Novice, +php-novice, +drupaldevdays

This only needs a trivial reroll :)

jlbellido’s picture

Status: Needs work » Needs review
FileSize
22.34 KB

Rerolled #158

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll, -Novice, -php-novice, -drupaldevdays

Thanks!

jjcarrion’s picture

Issue tags: +DevDaysMilan

Restoring the right devdays tag.

Wim Leers’s picture

Oops — sorry!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This is looking good.

  1. +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,57 @@
    +trait ResourceValidationTrait {
    ...
    +  protected function checkFieldAccess($entity) {
    

    The name of the trait and this method are a mismatch - not sure what to do here.

  2. +++ b/core/modules/rest/tests/src/Unit/ResourceValidationTraitTest.php
    @@ -0,0 +1,73 @@
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException
    +   * @expectedException UserRegistrationResourceTest::ERROR_MESSAGE
    
    +++ b/core/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
    @@ -0,0 +1,154 @@
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
    +   * @expectedExceptionMessage No user account data for registration received.
    ...
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
    +   * @expectedExceptionMessage An ID has been set and only new user accounts can be registered.
    ...
    +   *
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    +   * @expectedExceptionMessage You cannot register a new user account.
    ...
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    +   * @expectedExceptionMessage Only anonymous users can register users.
    

    The best practice has recently changed https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

  3. +++ b/core/modules/simpletest/src/WebTestBase.php
    @@ -2680,7 +2680,7 @@ protected function prepareRequestForGenerator($clean_urls = TRUE, $override_serv
    -   *   An absolute URL stsring.
    +   *   An absolute URL string.
    

    Unrelated change.

marthinal’s picture

Status: Needs work » Needs review
Related issues: +#2755667: Fix typo error in WebTestBase
FileSize
21.99 KB
8.47 KB

1. I've created a new trait

2. Done

3. I've created #2755667: Fix typo error in WebTestBase

alexpott’s picture

+++ b/core/modules/rest/src/ResourceAccessTrait.php
@@ -0,0 +1,28 @@
+   * @param \Drupal\Core\Entity\EntityInterface $entity
+   *   The entity object.
+   */
...
+        throw new AccessDeniedHttpException("Access denied on creating field '$field_name'.");

Let's add an @throws here.


FILE: ...tes/drupal8alt.dev/core/modules/rest/src/ResourceAccessTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 27 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: .../drupal8alt.dev/core/modules/rest/src/Tests/RegisterUserTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 49 | ERROR | [x] Expected 1 space after "=>"; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...core/modules/rest/tests/src/Unit/ResourceValidationTraitTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
  5 | WARNING | [x] Unused use statement
 73 | ERROR   | [x] The closing brace for the class must have an
    |         |     empty line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ore/modules/rest/tests/src/Unit/UserRegistrationResourceTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 150 | ERROR | [x] The closing brace for the class must have an empty
     |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 100ms; Memory: 6Mb

Coding standards that need fixing.

marthinal’s picture

1. IMHO the method should be ::checkCreateFieldAccess() instead of ::checkFieldAccess().

throw new AccessDeniedHttpException("Access denied on creating field '$field_name'.");

2. Done

3. Done

4. Done

5. Done

Wim Leers’s picture

Status: Needs review » Needs work

#170.1: disagreed. See \Drupal\Core\Entity\EntityAccessControlHandlerInterface::access() vs \Drupal\Core\Entity\EntityAccessControlHandlerInterface::createAccess(). It's specifically creation that's a special case. But that's exactly not what we have here:

    // Only check 'edit' permissions for fields that were actually
    // submitted by the user. Field access makes no difference between 'create'
    // and 'update', so the 'edit' operation is used here.

So, if anything, it should be checkEditFieldAccess(), not checkCreateFieldAccess().


  1. +++ b/core/modules/rest/src/ResourceAccessTrait.php
    @@ -0,0 +1,31 @@
    +trait ResourceAccessTrait {
    

    I think ResourceFieldEditAccessTrait would be more accurate?

  2. +++ b/core/modules/rest/src/ResourceAccessTrait.php
    @@ -0,0 +1,31 @@
    +   * Performs create access checks for fields.
    ...
    +   *   Throws access denied when the user does not have permissions to create a
    ...
    +        throw new AccessDeniedHttpException("Access denied on creating field '$field_name'.");
    

    Using the term "create" here is wrong.

marthinal’s picture

Status: Needs work » Needs review
FileSize
22.16 KB
1.41 KB

1) +1 ::checkEditFieldAccess()

2) IMHO the class name could be correct. At this moment it is probably not the case but maybe in the future we need to add a new method... I mean if we want to check access in the future but not only for edit permissions. is it ok for you @Wim?

3) throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
Done!

Thanks!

Status: Needs review » Needs work

The last submitted patch, 172: 2291055-172.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
22.15 KB
1.26 KB

Oops :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Great work, and thank you so much!

snehal.brahmbhatt’s picture

One more resource added i.e User Registration but path seems missing, please find the attached screenshot.

Apart from this, there is my configuration for new resource i.e User Registeration. Please find the attached screenshot.

Now, while I am trying to execute the below code using Postman:

URL: http://localhost/d8/user/register
Method: POST
Headers: 
Accept: application/json
Content-Type: application/json
Data:
{
 "_links":{"type":{"href":"http://localhost/d8/rest/type/user/user"}},
 "name":{
   "value":"snehal"
 },
 "mail":{
   "value":"snehal.addweb@gmail.com"
 },
 "pass":{
   "value":"testing"
 }

}

Example CURL Code:

curl --include --request POST --user snehal.addweb:addweb123 --header 'Content-type: application/hal+json' http://localhost/d8/user/regisrmat=hal+json --data-binary '{"_links":{"type":{"href":"http://localhost/d8/rest/type/user/user"}},"name":{"value":"johnnyharpertesting1"},"mail":{"value":"johnnyharpertesting1@gmail.com"},"pass":{"value":"testing"}}'

It shows me the below response:

HTTP/1.1 302 Found
Date: Mon, 04 Jul 2016 08:29:23 GMT
Server: Apache/2.4.18 (Unix) PHP/5.6.19
X-Content-Type-Options: nosniff
X-Powered-By: PHP/5.6.19
Cache-Control: must-revalidate, no-cache, private
Location: http://localhost/d8/user/1/edit
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Length: 368
Content-Type: text/html; charset=UTF-8

<!DOCTYPE html>
<html>
   <head>
       <meta charset="UTF-8" />
       <meta http-equiv="refresh" content="1;url=http://localhost/d8/user/1/edit" />

       <title>Redirecting to http://localhost/d8/user/1/edit</title>
   </head>
   <body>
       Redirecting to <a href="http://localhost/d8/user/1/edit">http://localhost/d8/user/1/edit</a>.
   </body>
</html>

Additionally, Here’s the attached screenshot for my users permission to access RESTful APIs.

Please need some quick help. Also do let me know if anything missing from my end too.

Thanks!

marthinal’s picture

@snehal.brahmbhatt

I'm not 100% sure about the reason.

But... some interesting points to review in your case:

1)

Accept: application/json
Content-Type: application/json

Should be HAL+JSON

2) If you are using RESTUI with D8.2.x then apply this patch #2758563: Handle core 8.2.x changing REST configuration to use configuration entities

3) If you want to register a new user, you don't need to use BASIC AUTH or the cookie session here. This resource is to register a new user. To create a new user you need to have permissions of course.

4) Please check the Integration test for more info (core/modules/rest/src/Tests/RegisterUserTest.php)

I hope it helps.

snehal.brahmbhatt’s picture

FileSize
98.66 KB

Thanks for your prompt response Marthinal.

I have checked with provide points.

For Point 1: HAL + JSON: still gives me same response.
Point 2: Ok
Point 3: This donot allows me to create new register user without BASIC AUTH or Cookie session. PFA screenshot.
Point 4: Ok

marthinal’s picture

Point 3: This donot allows me to create new register user without BASIC AUTH or Cookie session. PFA screenshot.

From RegisterUserTest

     // Verify that an authenticated user cannot register a new user, despite
    // being granted permission to do so because only anonymous users can
    // register themselves, authenticated users with the necessary permissions
    // can POST a new user to the "user" REST resource.
    $user = $this->createUser();
    $this->drupalLogin($user);
    $this->httpRequest('/user/register', 'POST', $serialized, 'application/hal+json');
    $this->assertResponse('403', 'Only anonymous users can register users.');
    $this->drupalLogout();

I mean you don't need to be authenticated. Well RESTUI requires an authentication provider so try with cookie for example...

Wim Leers’s picture

This is not a support issue. The tests prove this works. Let's not let this issue grow even bigger, we're already almost at 200 comments!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 174: 2291055-174.patch, failed testing.

kylebrowning’s picture

Status: Needs work » Reviewed & tested by the community

I think this failed because of something unrelated?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. I don't think we have enough test coverage of what happens when we're in USER_REGISTER_VISITORS vs USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL with different verify email settings. For example, we need to ensure the correct mails are sent. Also there is at least one subtle difference between the UI and rest, when requiring email verification the password is created by system not by the user.
  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,158 @@
    +    $approvalSettings = $this->userSettings->get('register');
    ...
    +    $register = $this->userSettings->get('register');
    

    Doing this twice.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,158 @@
    +    // If current user can register accounts then let's block the new registered
    +    // user if admin approval is needed.
    +    elseif ($approvalSettings == USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL) {
    +      $account->block();
    +    }
    

    This is different from the logic in AccountForm... which is:

    $status = $config->get('register') == USER_REGISTER_VISITORS ? 1 : 0;
    

    I think this means we should block everyone and then only unblock if $config->get('register') == USER_REGISTER_VISITORS - yes core only has the 3 modes but it is possible to add more. So we should keep the behaviour the same.

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,158 @@
    +    if ($register == 'visitors') {
    ...
    +    elseif ($register == 'visitors_admin_approval') {
    

    Should use the constants

  5. Sorry I know this issue is 200 comments long but getting this right is important
marthinal’s picture

Assigned: marthinal » Unassigned

No time to work on it this week. Unassigning the issue so anyone can feel free to work on it.

tedbow’s picture

Status: Needs work » Needs review
FileSize
24.19 KB
7.54 KB

This is patch addresses review in #183

1. Added the test coverage.
2. fixed
3. blocking now if USER_REGISTER_VISITOR
4. fixed
5. no problem! thanks for reviewing!

Also moved the logic to decide whether to block the user based on emails to before the user is saved. This way you don't have to save the user twice.

alexpott’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -0,0 +1,160 @@
+    // All new users should be blocked unless visitors are allowed to register.
+    if ($approvalSettings != USER_REGISTER_VISITORS) {
+      $account->block();
+    }
+    else {
+      if (!$this->userSettings->get('verify_mail')) {
+        // No email verification needed activate account.
+        $account->activate();
+      }
+      else {
+        // Email verification needed.
+        $account->block();
+      }
+    }

This can be written with less logic. Like so...

if ($approvalSettings == USER_REGISTOR_VISITORS && !$this->userSettings->get('verify_mail')) {
  $account->activate();
}
else {
  $account->block();
}
tedbow’s picture

@alexpott whoops. fixed

dawehner’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -163,8 +161,7 @@ public function post(EntityInterface $entity = NULL) {
           // metadata here.
           $url = $entity->urlInfo('canonical', ['absolute' => TRUE])->toString(TRUE);
    -      $response = new ModifiedResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
    -      return $response;
    +      return new ModifiedResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
         }
    

    This small refactoring could be out of scope of this issue, but well ...

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,154 @@
    +    if ($account === NULL) {
    +      throw new BadRequestHttpException('No user account data for registration received.');
    +    }
    +
    +    // POSTed user accounts must not have an ID set, because we always want to
    +    // create new entities here.
    +    if (!$account->isNew()) {
    +      throw new BadRequestHttpException('An ID has been set and only new user accounts can be registered.');
    +    }
    +
    +    // Only allow anonymous users to register, authenticated users with the
    +    // necessary permissions can POST a new user to the "user" REST resource.
    +    // @see \Drupal\rest\Plugin\rest\resource\EntityResource
    +    if (!$this->currentUser->isAnonymous()) {
    +      throw new AccessDeniedHttpException('Only anonymous users can register users.');
    +    }
    +    $approvalSettings = $this->userSettings->get('register');
    +
    +    // Verify that the current user can register a user account.
    +    if ($approvalSettings == USER_REGISTER_ADMINISTRATORS_ONLY) {
    +      throw new AccessDeniedHttpException('You cannot register a new user account.');
    +    }
    

    From a pure refactoring point of view, I would have moved these guards into its own helper method, so the overall code of the post() function becomes easier to read.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,154 @@
    +    $approvalSettings = $this->userSettings->get('register');
    ...
    +    if ($approvalSettings == USER_REGISTER_ADMINISTRATORS_ONLY) {
    

    Do we use camelCase variables now?

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,154 @@
    +
    +    // No e-mail verification is required. Activating the user.
    +    if ($approvalSettings == USER_REGISTER_VISITORS) {
    +      if ($this->userSettings->get('verify_mail')) {
    +        // No administrator approval required.
    +        _user_mail_notify('register_no_approval_required', $account);
    +      }
    +    }
    +    // Administrator approval required.
    +    elseif ($approvalSettings == USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL) {
    +      _user_mail_notify('register_pending_approval', $account);
    +    }
    +
    

    Also moving this into its own method would increase the readability: $this->sendEmailNotifications($account)

  5. +++ b/core/modules/rest/src/ResourceAccessTrait.php
    @@ -0,0 +1,31 @@
    +  protected function checkEditFieldAccess(EntityInterface $entity) {
    ...
    +      if (!$entity->get($field_name)->access('edit')) {
    
    +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,39 @@
    +  protected function validate(EntityInterface $entity) {
    

    Should we typehint for FieldableEntityInterface here, because this is what the function actually needs? both for ->get() and ->validate()

tedbow’s picture

@dawehner thanks for review
1. oh well
2. refactored
3. no we are not ;). fixed
4. refactored
5. yes thats a good idea. changed

post() is much more readable now!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @tedbow, @dawehner and @alexpott! :)

kylebrowning’s picture

So close!

xjm’s picture

Issue tags: +beta deadline

This RTBC issue probably has a beta deadline, so tagging for visibility.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +needs security review

#25 has never been definitively answered on the issue and given that this will allow users to be created I think we need a sec review before committing. Setting back to "needs review" to get that.

dawehner’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,175 @@
    +    return new ModifiedResourceResponse($account, 200);
    

    What about returning a 201 here?

  2. +++ b/core/modules/rest/src/ResourceAccessTrait.php
    @@ -0,0 +1,31 @@
    +
    +trait ResourceAccessTrait {
    
    +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,39 @@
    +
    +trait ResourceValidationTrait {
    

    maybe adding some helpful information would be nice

Wim Leers’s picture

Status: Needs review » Needs work

#194.1++, great nitpick!

#193: excellent, excellent point! I dug into the commit history, and determined that http://cgit.drupalcode.org/services/commit/resources/user_resource.inc?h... contains the fix. The culprit was these two lines (that were removed in the fix):

-    'pass1' => isset($account['pass']) ?: '',
-    'pass2' => isset($account['pass']) ?: '',

In other words, an "account" object was POSTed, and if no password was specified, then the password was set to the empty string. That is of course preposterously unsafe.

Also note that if you look at the /user/register HTML form, you'll see that you can only provide a password if the "require e-mail verification" setting is turned off. Otherwise, the password is e-mailed.

So, let's see what happens if we don't POST a password when registering. It should complain no password was provided when the "require e-mail verification" setting is turned off. In the other case, it should ignore it.

  1. First of all, turns out that one case was not being tested: when admin approval is required, but no e-mail verification.
  2. In case of e-mail verification, the password should be empty (because it will be set later, when the user opens the e-mail verification link), otherwise it should not be empty. Added those assertions.
  3. This means that in the case of no e-mail verification, we expect a validation error (because a password must be provided). Conversely, when a password is sent in the case of e-mail verification, we also expect a validation error, because no password can be provided. Did not yet add those assertions.
  4. Then, modified the test to NEVER send a password.

As you can see, the test fails. So, if no password is provided, then the empty password is set. Just like in https://www.drupal.org/node/2344389. But we don't set the empty password explicitly, it happens automatically.

I suspect we'll need to fix some of the logic deep in User and PasswordItem.

Also: For the HTML form, it's \Drupal\user\RegisterForm::submitForm() that always generates a password in case of a "require e-mail verification" being turned on, so that even violates my assumption above. It seems to be setting a temporary password, that is then overridden when the user has clicked the e-mail verification link.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.56 KB
25.76 KB

Forgot the patch I wanted to attach. This should contain exactly one fail, and that one fail proves that https://www.drupal.org/node/2344389 applies to this also.

Status: Needs review » Needs work

The last submitted patch, 196: 2291055-195.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
27.04 KB
7.87 KB

Ok this patch:

  1. Throws an exception if Email Verification is required and the request has a password. Otherwise the client may expect that password to work which it won't
  2. Throws an exception if Email Verification is NOT required and the request does not have a password.
  3. Adds test coverage for these cases
  4. Adds test coverage for Admin approval required and Verification NOT required

Status: Needs review » Needs work

The last submitted patch, 198: 2291055-198.patch, failed testing.

kylebrowning’s picture

Excellent depiction in #195.

Sorry the commit message is so vague, the security team made me do it!

tedbow’s picture

Status: Needs work » Needs review

Setting to "Needs Review". First time failed was for FieldUI test which I am sure is a test problem. Retested and passed!

kylebrowning’s picture

Status: Needs review » Needs work

All nits.

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +    // Only active new users if visitors are allowed to register and no email
    

    Should read '// Only activate new users if visitors are allowed to register and no email'

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +      throw new AccessDeniedHttpException('Only anonymous users can register users.');
    

    This is only the ability to register a single user, so `Only anonymous users can register a user.`

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +        throw new HttpException(422, 'No password provided');
    

    No period at the end here.

kylebrowning’s picture

For clarification, its ready besides those nits.

Tested locally from an iOS app too, not that the tests don't prove this works. :P

tedbow’s picture

Status: Needs work » Needs review
FileSize
27.02 KB
1.81 KB

@kylebrowning thanks for the review!

This patch addresses nits #202

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Has the security review been fully addressed? I think @Wim Leers and @tedbow and @kylebrowning addressed one issue that was discovered, but I do not see any other comments from the security team on the issue since it was tagged.

kylebrowning’s picture

#25 concerns have been addressed, but have not heard from Security team here.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

I haven't reviewed the full patch yet, but the following jumped out at me so far:

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -156,14 +161,7 @@ public function post(EntityInterface $entity = NULL) {
    -    // Only check 'edit' permissions for fields that were actually
    -    // submitted by the user. Field access makes no difference between 'create'
    -    // and 'update', so the 'edit' operation is used here.
    -    foreach ($entity->_restSubmittedFields as $key => $field_name) {
    -      if (!$entity->get($field_name)->access('edit')) {
    -        throw new AccessDeniedHttpException("Access denied on creating field '$field_name'");
    -      }
    -    }
    +    $this->checkEditFieldAccess($entity);
    +++ b/core/modules/rest/src/ResourceAccessTrait.php
    @@ -0,0 +1,31 @@
    +  protected function checkEditFieldAccess(FieldableEntityInterface $entity) {
    

    Won't this break config entities? Since in HEAD, if _restSubmittedFields is empty, then the loop is skipped, but with the patch, we'd get an error due to the typehint?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -277,39 +274,6 @@ public function delete(EntityInterface $entity) {
    -  protected function validate(EntityInterface $entity) {
    -    // @todo Remove when https://www.drupal.org/node/2164373 is committed.
    -    if (!$entity instanceof FieldableEntityInterface) {
    -      return;
    -    }
    +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,39 @@
    +  protected function validate(FieldableEntityInterface $entity) {
    

    And this too? Since this is changing a no-op return into an error due to the typehint?

effulgentsia’s picture

Some minor stuff, but still not a complete review:

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -156,14 +161,7 @@ public function post(EntityInterface $entity = NULL) {
    -        throw new AccessDeniedHttpException("Access denied on creating field '$field_name'");
    +++ b/core/modules/rest/src/ResourceAccessTrait.php
    @@ -0,0 +1,31 @@
    +        throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
    

    Earlier in the post() method, we require that it only operate for entity creation, so does changing the wording to "updating" make sense? If we don't like "creating field" for some reason, would something like "setting value for field" be more appropriate than either "creating" or "updating"?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -277,39 +274,6 @@ public function delete(EntityInterface $entity) {
    -      throw new HttpException(422, $message);
    +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,39 @@
    +      throw new UnprocessableEntityHttpException($message);
    

    Nice!

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,190 @@
    +        throw new HttpException(422, 'No password provided.');
    ...
    +        throw new HttpException(422, 'A Password cannot be specified. It will be generated on login.');
    

    In which case, why not make these use UnprocessableEntityHttpException as well?

effulgentsia’s picture

#25 concerns have been addressed, but have not heard from Security team here.

I'm not on the security team, other than as a maintainer, but FWIW, I don't see any security problems with the patch.

dawehner’s picture

Status: Needs review » Needs work

Won't this break config entities? Since in HEAD, if _restSubmittedFields is empty, then the loop is skipped, but with the patch, we'd get an error due to the typehint?

Great catch! I think we should have a test in general which is a non fieldable non config entity.

Wim Leers’s picture

We should just revert all of those changes.

The point of those changes was that we move some of those protected helpers on EntityResource to a trait, so that the user registration resource could also use them.

Modifying while moving is unnecessarily complicating things.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
27.59 KB
4.54 KB
  • #208.1: fixed.
  • #208.2: fixed.
  • #209.1: fixed.
  • #209.3: great nit, done.

All of these changes are just reverting to HEAD's behavior/logic. I didn't realize that the "split this off into a separate helper method on a trait so both X (EntityResource) and Y (UserRegistrationResource) can use it" rule had not been respected. Sorry for missing that.

Since this is just moving existing logic to a different place, this should not have to introduce additional test coverage IMHO. It's unfortunate that the REST module has less comprehensive test coverage than we'd like, but we have #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method to solve that. I'll take "comprehensive test coverage" on during the 8.2 beta phase.

Status: Reviewed & tested by the community » Needs work

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

dawehner’s picture

+++ b/core/modules/rest/src/ResourceAccessTrait.php
@@ -10,20 +10,20 @@
     foreach ($entity->_restSubmittedFields as $key => $field_name) {
       if (!$entity->get($field_name)->access('edit')) {
-        throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
+        throw new AccessDeniedHttpException("Access denied on creating field '$field_name'.");
       }

+++ b/core/modules/rest/src/ResourceValidationTrait.php
@@ -10,13 +10,13 @@
-  protected function validate(FieldableEntityInterface $entity) {
+  protected function validate(EntityInterface $entity) {
     $violations = $entity->validate();

This doesn't help. In HEAD we check that there is a fieldable entity passed in ... and now not longer anymore. I think this was the point of @effulgentsia

See #208.2

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
26.98 KB
1.69 KB

Per #215. Also removed some no longer necessary comments, since the class name of the exception is already sufficiently clear.

effulgentsia’s picture

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/UserRegistrationResource.php
    @@ -0,0 +1,191 @@
    +class UserRegistrationResource extends ResourceBase {
    

    Seems to me we should move this to user.module, no?

  2. +++ b/core/modules/rest/src/ResourceAccessTrait.php
    @@ -0,0 +1,31 @@
    +trait ResourceAccessTrait {
    +++ b/core/modules/rest/src/ResourceValidationTrait.php
    @@ -0,0 +1,40 @@
    +trait ResourceValidationTrait {
    

    But these traits don't implement access and validation logic for generic resources, only for entities. So I think we should rename them to EntityResource(Access|Validation)Trait. And perhaps also move them into the Drupal\rest\Plugin\rest\resource namespace?

tedbow’s picture

Ok this patch address #217 renames the 2 traits and moves the registration resource to the user module.

hope I got it right but you know as they say "there are 2 hard things in computer science, renaming things"

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs security review
FileSize
1.44 KB
27.16 KB

#215+#216: sorry for the slight misinterpretation.

#217++ — I can't believe I didn't notice that before!

My only final remark: I think it may be best to mark both of those traits as @internal for now. We can consider making it non-internal in 8.3, once REST is more fully fleshed out, and in particular once we have full Config Entity CRUD support (#2300677: [PP-1] Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST), which would ensure these traits are indeed as universally applicable as their names convey.
So, did that.


#206:

Has the security review been fully addressed? I think @Wim Leers and @tedbow and @kylebrowning addressed one issue that was discovered, but I do not see any other comments from the security team on the issue since it was tagged.

I see four reasons why it's okay for me to remove the needs security review tag:

  1. @kylebrowning in #207: #25 concerns have been addressed, but have not heard from Security team here. — and remember, Kyle Browning is the person who fixed the SA in #25: https://www.drupal.org/node/2344389!
  2. @effulgentsia in #210: I'm not on the security team, other than as a maintainer, but FWIW, I don’t see any security problems with the patch.
  3. @Wim Leers (i.e. I) did a full assessment in #195 of what the security problem in #25 was, and how it applied here. I outlines what the required additional test coverage was for this to be adequately addressed and regression-tested.
  4. @tedbowman then made the necessary fixes and wrote the necessary regression test coverage in #198 to ensure that #25 was indeed fully addressed

Given the beta deadline, and given that everything since #208 was minor to very minor stuff, moving back to RTBC.

effulgentsia’s picture

Adding review credit to @claudiu.cristea.

effulgentsia’s picture

And to @heykarthikwithu.

  • effulgentsia committed 79dfc06 on 8.3.x
    Issue #2291055 by marthinal, tedbow, Wim Leers, kylebrowning, m1r1k,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +needs security review

Pushed to 8.3.x. Thanks!

I discussed this with @alexpott and @xjm about whether to push to 8.2.x (i.e., change this from "beta deadline" to "beta target"). We're not sure yet. We'd feel better about doing that if it got some additional security review, despite the excellent points in #219, so restoring that tag, and setting to "Needs review". If that doesn't happen by beta2, then we can leave this as an 8.3-only feature, in which case, it'll sit around in a non-production branch for 6 more months, which will provide an opportunity for security issues to surface without compromising live sites.

Wim Leers’s picture

Thanks! That sounds like a very reasonable compromise.

/me waves at security team members.

kylebrowning’s picture

Issue summary: View changes

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

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

effulgentsia’s picture

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

Still eligible for 8.2.x if it can be security reviewed in time, per #223.

pwolanin’s picture

I'll try to add a review of this. I did a quick look at the patch and it doesn't seem like we are using the same code path for user registration via form submission and via REST? In general I think having the validation and save code be different for form vs. api (REST, etc) can lead to security bugs if they diverge accidentally in terms of behavior.

pwolanin’s picture

@dawhner - a follow-up plan to normalize those code paths seems very necessary, at a minimum.

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -36,6 +36,9 @@
    +  use EntityResourceValidationTrait;
    +  use EntityResourceAccessTrait;
    

    Are there other places these new traits will be used? Seems like only 1 in this patch.

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
    @@ -0,0 +1,35 @@
    +    foreach ($entity->_restSubmittedFields as $key => $field_name) {
    

    I realize this code is just moved, but it's pretty ugly to have such a property. Should we look at entity API improvements in a follow-up to support generically listing edited fields when processing an entity submission?

kylebrowning’s picture

Im all for followups, It would be really nice if this made it into 8.2, otherwise for 6 months we have to wait for external applications to have a real way to register account on a supported Drupal version

xjm’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs review » Fixed

Following up on #223, @effulgentsia, @alexpott, and I discussed this issue and decided that it is not eligible for the beta at this time (see also: https://www.drupal.org/core/d8-allowed-changes#beta). This issue is already fixed in 8.3.x (hooray!) so moving back to that branch.

kylebrowning’s picture

:(

It would have been nice to have been included in that discussion. This was the final ticket to finish Fully decoupled Drupal #2721489: REST: top priorities for Drupal 8.2.x, im disappointed we now have to wait 6 months to use it.

alexpott’s picture

Wim Leers’s picture

Issue tags: +8.3.0 release notes

Considering which REST issues have been tagged with 8.2.0 release notes, I think this one will definitely qualify for 8.3.0 release notes. So, tagging as such.

Status: Fixed » Closed (fixed)

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

ruloweb’s picture

I was wondering... why this endpoint was created as a RestResource but login/logout are common ControllerBase methods defined in user.routing.yml ?

Wim Leers’s picture

@ruloweb: Because registering is the creation of a new User entity. But logging in and out does not correspond to the reading/creating/updating/deleting of anything… it's just "using" something. This is discussed in detail #2403307: RPC endpoints for user authentication: log in, check login status, log out.

hubobbb’s picture

d8.3.2 here has a good article works for me .

https://areatype.com/blog/register-user-drupal-8-rest-api
data format
{
"_links": {
"type": {
"href": "http://yoursite.com/rest/type/user/user"
}
},
"name": [
{
"value": "test3"
}
],
"mail": [
{
"value": "test+3@test.com"
}
],
"pass": [
{
"value": "test"
}
]
}

or see the doc .
https://www.drupal.org/docs/8/core/modules/rest/javascript-and-drupal-8-restful-web-services

,above two methods work ok.