REST resources for anonymous users: register

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.

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

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.

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

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

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!

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

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

@klausi thanks for reviewing.

I continue working on it.

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.

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

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

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

Created new issue about user entities creation(#2405091: Cannot create user entities - {"error":"Access denied on creating field pass"} ). The current issue is about user registration.

For restui module we need a path too when no canonical path exists. (https://www.drupal.org/node/2359245#comment-9490089)

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?

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

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?

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!

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

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.

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.

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

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

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?

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.

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

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.

I've created a new issue about this error.

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

@klausi many thanks for your help. :)

Adding cacheability metadata.

Missing review #30

Emails are not working as expected. Working on it.

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

Ok let's try again.

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

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

Adding integration test.

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.

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.

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.

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.

Rerolled

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

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.

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

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

Hardcoded Base url . Fixed.

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.

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

And the last three

11. Done

12. Done

13. Done

Tested again, still working.

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.

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.

Fixed problems with test + #76.11 and #76.12

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

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

@heykarthikwithu thanks :)

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

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

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.

Creating a user is different than registering one.

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

Okay updated.

Removing leftover example text.

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.

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

@kylebrowning thanks for updating the summary

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.

Updating title based on new category.

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

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

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

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

I've created the new issue #2626298: REST module must cache only responses to GET requests

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

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

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

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"

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

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()]);

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)

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

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

This is blocked on both

1. #2403307: RPC endpoints for user authentication: log in, check login status, log out
2. #2626298: REST module must cache only responses to GET requests

Updating issue title & summary.

Turns out #2403307: RPC endpoints for user authentication: log in, check login status, log out is blocked on #2419825: Make serialization_class optional.

This will also need a change record.

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.

Here's the actual better title.

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

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

Thanks for the explanation. Good to know.

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

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

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.

Ok, cool! Happy to be wrong!

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

Replacing NonCacheableResourceResponse with our new ModifiedResourceResponse.

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.

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.

Adding the uri_paths(Annotation) should be enough.

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.

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.

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.

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

Fixed unit tests.

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

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

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!

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?

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!

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

user error

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

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!

Sure, makes sense to improve these comments.

Ok, Done.

Thank you for your great reviews and your help!

This needs a reroll because #2308745: Remove rest.settings.yml, use rest_resource config entities landed.

I created a change record: https://www.drupal.org/node/2752071.

This only needs a trivial reroll :)

Rerolled #158