User created via /user/register?_format=json get blocked

See https://www.drupal.org/core/issue-priority about issue priorities.

Required email verification should affect the user password, not the user status.
Patch attached

Hello,

I have reviewed the patch #7 and It worked as designed. Below is my update.

1). I have given permission to "Visitors" users to create an account with an email verification option.

2 I have created a user account using the drupal register form and it was an active user.

3 I have created a user account using Rest API then it was a blocked user.

Rest API:

User:

4 I have created users using Rest API after applying the patch then the user was active.

Rest API:

User:

The patch is working great.

Thanks,
Ren

This change matches what happens on \Drupal\user\AccountForm::form() and \Drupal\user\RegisterForm - nice catch.

+++ b/core/modules/user/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -100,15 +100,19 @@ public static function create(ContainerInterface $container, array $configuratio
+    // Generate password if email verification required.
+    if ($this->userSettings->get('verify_mail')) {
+      $account->setPassword(user_password());
+    }
+

One question is if they've sent a password in the request should we error here? Because if they have email verification on I think we need to generate a new password and whatever they've sent should be ignored.

If we decide to stick with the current approach we should have test coverage of sending a password and ignoring it when we are verifying email.

Ideally @marthinal & @tedbow would also review this — they were the most active participants in #2291055: REST resources for anonymous users: register, which brought \Drupal\user\Plugin\rest\resource\UserRegistrationResource to Drupal core.

#10 Now is not possible to send pass data. This case checked before temporary password generation inside the protected function ensureAccountCanRegister

    if (!$this->userSettings->get('verify_mail')) {
      if (empty($account->getPassword())) {
        // If no e-mail verification then the user must provide a password.
        throw new UnprocessableEntityHttpException('No password provided.');
      }
    }
    else {
      if (!empty($account->getPassword())) {
        // If e-mail verification required then a password cannot provided.
        // The password will be set when the user logs in.
        throw new UnprocessableEntityHttpException('A Password cannot be specified. It will be generated on login.');
      }
    }

Also, \Drupal\Tests\user\Functional\RestRegisterUserTest have related case inside testRegisterUser()

      // Attempt to register with a password when e-mail verification is on.
    $config->set('register', UserInterface::REGISTER_VISITORS);
    $config->set('verify_mail', 1);
    $config->save();
    $response = $this->registerRequest('Estraven', TRUE);
    $this->assertResourceErrorResponse(422, 'A Password cannot be specified. It will be generated on login.', $response);

Is this the answer to your question?

Hello,
Thank you for this welcome patch.
Silly question: what is the module name needed to apply the patch via Composer? Is it User?

 "drupal/user": {
  "Set REST-created user account as Active instead of Blocked" : "https://www.drupal.org/files/issues/2019-10-24/drupal-3055807-user_registered-via-rest-get-blocked-7.patch"
}

Thank you!

@kloport this is drupal/core

I am currently using 8.9.7 This still not actually resolved.
When ever account is created using rest user still showing blocked
REST JSON
{
'name' : [{"value": 'abc' }],
'mail' : [{"value": 'pr'}],
}

Some of the other forum saying I should pass additional key as "status": [{"value":"1"}] but then its giving "no authentication credentials provided" and if I don't have status and user got registered with status is "Blocked"

It would be great if this could get a review as it breaks registration for headless sites.

Apply 7.patch and working fine now sharing the screenshot

Applying 7. Patch works for me.

Ran into this problem as well. Applying the patch from #7 allowed the account to be created with a status of Active. Note that you do not need to actually pass 'status' in the request when creating an account, you will still get the access denied error.

We still require users to click the registration link in their email, but now they actually can do that without hitting a 403.

Re-rolling the patch in #7 to apply to Drupal 9.3.2.

I tested #7 patch as well and confirm it's working properly. Same steps as #9:

1. I created a user with Drupal UI and “Require email verification when a visitor creates an account” flag : status active
2. I created a user via REST : status blocked
3. I applied the patch and created a user via REST: status active

Last, it seems that @Vladimir #12 comment answers @Alexpott, so should this be reviewed yet another time or could it be committed ? It's a pretty annoying issue in decoupled context. I'll mark it as RTBC for now.

Hey @Kojo, thanks for testing and moving this issue to RTBC, looking at #25, the patch looks good, I just have one minor suggestion:

+++ b/core/modules/user/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -100,15 +100,19 @@ public static function create(ContainerInterface $container, array $configuratio
+      $account->setPassword(\Drupal::service('password_generator')->generate());

Don't you think password generator should be injected? I mean, I know it's just one call, but this way I think we keep things consistent.

The following patch has the suggestion implemented, I'll move this back to needs review to see what you think.

Thanks

My bad, here's a new patch with the tests fixed.

@murilohp Yes, it's better to inject the class. Thanks for redoing the patch. Setting back to RTBC.

+++ b/core/modules/user/src/Plugin/rest/resource/UserRegistrationResource.php
@@ -63,11 +71,14 @@ class UserRegistrationResource extends ResourceBase {
+   * @param \Drupal\Core\Password\PasswordGeneratorInterface $password_generator
+   *   The password generator.
    */
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, array $serializer_formats, LoggerInterface $logger, ImmutableConfig $user_settings, AccountInterface $current_user) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, array $serializer_formats, LoggerInterface $logger, ImmutableConfig $user_settings, AccountInterface $current_user, PasswordGeneratorInterface $password_generator) {
     parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
     $this->userSettings = $user_settings;
     $this->currentUser = $current_user;
+    $this->passwordGenerator = $password_generator;

We need to do the deprecation dance here - in case someone has extended this class.

Here's an example from \Drupal\jsonapi\Context\FieldResolver::__construct

   * @param \Drupal\Core\Session\AccountInterface|null $current_user
   *   The current user account.
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $field_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info, ResourceTypeRepositoryInterface $resource_type_repository, ModuleHandlerInterface $module_handler, AccountInterface $current_user = NULL) {
    if (is_null($current_user)) {
      @trigger_error('Calling ' . __METHOD__ . '() without the $current_user argument is deprecated in drupal:9.3.0 and will be required in drupal:10.0.0.', E_USER_DEPRECATED);
      $current_user = \Drupal::currentUser();
    }

Here is an updated patch from #29 addressing the changes suggested in #31. Thanks for the reference @alexpott.

Thanks for addressing #31 @ankithashetty!. Since it's a deprecation message, we should have tests for this, see core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php as example.

Hey, I made a new patch with the test, I think now we can move it to needs review.

I have applied this patch and tested the REST API vs Drupal 9.3.6 and successfully created a user with active status. Thank you all for your work on this!

Yeah, I've been using this patch in production for a couple months with no issues. Moving to RTBC.

The patch didn't apply in January. How can this be RTBC?

I'm sorry, I forgot to re-queue the tests here before moving to RTBC. It did pass the test in January on 9.3, it just seems to be failing on 9.4.

Reroll for 9.4.x & added reroll diff as well.

Thanks for the reroll @yogeshmpawar, the testbot is happy and it looks good to me, so moving back to RTBC.

We have an interesting problem the hal module is removed from 10.0.x but what this means is that there is no real test coverage of the \Drupal\user\Plugin\rest\resource\UserRegistrationResource class in Drupal 10. We only have unit coverage and no functional coverage. Ideally we need to re-instate the coverage and not rely on hal (because it does not exist in D10).

This is blocked on #3268105: Bring back RestRegisterUserTest into user module (without HAL)

Setting to Postponed based on #42.

#3268105: Bring back RestRegisterUserTest into user module (without HAL) landed! So we can move this on. I've just updated #39 and created a new patch for D10 only.

The last patch uploaded here is a 9.x with deprecation dance and a 10.x withouth, I think this is too short to 10.x being release, so it should be 11.x without and 10.x with the deprecations? Means we also have to update the version numbers in the message.

In the higher version patch we also remove the deprecation dance without actually remove the = null from the constructor, we should change this also.

Added Drupal 9.5.x patch of comment #44 on Drupal 10.1.x, with changes of #46 as suggested, please review.

Patch #25 works on Core version 9.4.5

Patch #47 applies and works perfectly with 9.4.8.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Thank you for the patch @ravi.shankar please include an interdiff also so we can easily see what was changed. It does look like the deprecated calls were put in. But believe we missed the 10.0 window so those probably should be updated for deprecated in 10.1

Tagging for IS update as it doesn't clearly say what is being accomplished in the patch.

Thanks.

I have re-rolled patch #47 as an MR, updated the deprecation messages to 10.3/11, and rewrote the issue summary. Setting back to needs review.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

I created a draft change record and updated the MR to link to it.

I've never written a change record before so someone should take a careful look at it.

Seems to have a failing test.

I fixed the test by using the correct deprecation message.

Sorry to do this but we will need 2 MRs now

1 with the deprecations (current MR) for 10.3.x
1 without the deprecations, can probably add promotion too for 11.x

@smustgrave Ok, I split the MR in two, one for 10.x with deprecations, one for 11.x without.

1 without the deprecations, can probably add promotion too for 11.x

What did you mean by "promotion"? I'm happy to keep working on this but I don't know what that is.

Hard to explain so pushed what I meant to the 11.x branch. Reduces code by a ton. Also updated MR to not include default NULL as the deprecation makes seem like it's required.

But both MRs appear to be good and address the issue at hand.

Committed d349f1d and pushed to 11.x. Thanks!
Committed e6fae86 and pushed to 10.4.x. Thanks!
Committed 68715a6 and pushed to 10.3.x. Thanks!

Backported to 10.3.x as a major bug fix.