Users deleted via JSON:API DELETE don't follow the site-wide cancel_method in the user settings

The code to delete a user account could call user_cancel(). That's what the controller for the email links to cancel an account does in UserController::confirmCancel().

$account_data = $this->userData->get('user', $user->id());

// …

$edit = [
  'user_cancel_notify' => isset($account_data['cancel_notify']) ? $account_data['cancel_notify'] : $this->config('user.settings')         ->get('notify.status_canceled'),
];
user_cancel($edit, $user->id(), $account_data['cancel_method']);

I could understand the objection that user_cancel() is for cancelling an account (which means a user started it from the site user interface), not deleting an account (which could be done without any user starting it).
Do we want to always cancel an account, instead of making a distinction between cancelling an account and deleting an account?

This is on the borderline between major and critical. The case for it being major is that there is a workaround as described in #2, but on the other hand it is unexpected, permanent data loss and one shouldn't need custom code to prevent data loss. Erring on the side of critical. Thank you for reporting this!

8.9.x is in security support only, so moving to 9.2.x which is the lowest branch where we would commit a fix. (Upgrade to Drupal 9 ASAP as there are only 81 days until Drupal 8 is completely end-of-life with no security coverage whatsoever!) :)

Hi @GuyPaddock, first of all thanks for reporting this issue. Users and JSON:API seem to have some history of having some issues working correctly together. This is another example of that.

In my opinion when a user is deleted in the JSON:API it should follow the settings of the Drupal installation. JSON:API should be a API representation of the installation, including that setting.

I remembered someone else mentioning this a while back, but this unfortunately didn't trigger us to investigate more. See the relevant slack thread. In a way the mentioned #2953748: user_delete() should not remove the content created by user could be seen as an related issue, where a simple delete on the User entity results in the deletion of all the related content.

The question then becomes, where should this be fixed.

1. Do we fix this in JSON:API, by calling the method in the user module.
2. Or is this an issue that when an user is deleted it should always respect the cancel_method as defined in the user settings, regardless of which code calls for a deletion.

Guess the easiest way to fix it is to make JSON:API concise with how the backend works. But I think the correct way is not to delete everything of a user but respect the site settings regardless of who calls for deletion.

If we are to fix this in JSON:API while we wait for #2953748: user_delete() should not remove the content created by user the fix is adding an extra condition in the EntityResource::deleteIndividual() method so it respects the site settings.

But i feel there should also be a way to call the different cancel methods so you COULD delete the user if you want. I can image in a decoupled situation you might want to cancel by default but also have the option to anonimize or delete all related content.

Isn't this a bug in User module? 🤔

Well, if you look at the related issue, it is currently marked as a feature request? The thing is, before this, the interface was really the only way to do this, we now supplied a new way to call DELETE and suddenly we are loosing the settings and options from the normal delete form from the backend.

I could argue its a bug there, or it's an oversight here. That why i'm kinda looking for ideas on how to view this issue. I think if we try to get the entity changed that this will be a lot more effort to get committed though.

I agree that the definitive fix is probably in User. In so far as, I don't think json:api module wants to be in the business of encapsulating entity-specific logic (core as it is in Drupal) but rather let the entity's handlers do that.

True that. But jsonapi has multiple places we're special cases for the user entities are spread. So there is precedent, even though its not ideaal at all.

Either user_cancel() and User::delete() are merged, which means there isn't anymore a distinction between cancelling an account or deleted it, or modules need to call user_cancel() or User::delete() depending on which one is considered more correct.

I am not sure an account should always be cancelled instead of deleted. (It would also make an entity class depend on the config.factory service, which doesn't happen for other entity classes.)

After discussion with @win-leers and @e0ipso on this issue we decided to go ahead and fix this in jsonapi.

Added tests that test the different options. 3 out of the 4 should fail since the current implementation is the same as the user_cancel_delete cancel method.

Created the appropiate changes, unfortunately there is some new arguments to service, so I added some deprecation notices.

left a review, I think we need to have a follow up to remove the 'json api knows about the specifics of various entity-types' and introduce a proper entity-type level handler API for JSON:API

Ok, i've looked into the failure. This is quite logical.

When a user is "deleted" it by default is only blocked now. This means the second delete test will not get a 404 response.

I think it's safe to change the settings for testDeleteIndividual to use the user_cancel_delete method, since that is what was tested there before this MR.

The added tests for the different ways you can cancel/delete should cover the other methods.

looking good, left one minor comment for a todo pointing to the followup

I have reviewed the MR, the above discussion and trade-offs, and tested this locally. I believe it is eligible to be RTBC.

My only general note on the code is that we "should" be using DI for invoking the config factory and module handler, however that point is addressed in the MR discussion regarding this being a stopgap to fix a critical bug, and the follow-up for adding jsonapi-specific handlers is a more definitive fix. Changing the constructor or other methods' signatures for what is otherwise a stopgap would be an unnecessary API change. (E.g., the site I am working on now has reason to decorate/extend the entity resource (similar to, but more complex than jsonapi_extras' jsonapi_defaults module) and leaving the methods alone for now is good for those similarly situated.

I think we need to ensure that _user_cancel_session_regenerate() is called too. I.e. if we're using json cookie auth...

I.e we need to do something like:

  // After cancelling account, ensure that user is logged out.
  if ($account->id() == \Drupal::currentUser()->id()) {
    _user_cancel_session_regenerate();
  }

I think we need to ensure that _user_cancel_session_regenerate() is called too. I.e. if we're using json cookie auth...

I think this should be the responsibility of the user storage handler, so e.g. graphql, rest etc can also benefit

@larowlan I agree that there should be some central code for services to call but the implementation added here is not doing the whole thing. I think we should either postpone this on adding something to user storage or we do the whole thing properly for JSON API and then refactor to the new code when it exists.

we do the whole thing properly for JSON API and then refactor to the new code when it exists

Given this is a critical with major data loss, I think that's the best course of action.

Can we get that code added, plus a follow-up and a @todo.

Thanks

I've had a discussion with @larowlan around this issue. Your first comment shouldn't be a problem. The comment on the merge request seems to be a bigger challenge. With help of @larowlan i hope to send in an update this friday, or at least a conclusion.

Ok, so I've gone down a rabbit hole and back.

On one hand, this code in core is ancient and really in need of a refresh, but on the other hand - its not the job of JSON:API to drag up 12 year old plus issues.

In my travels, I came across \Drupal\user\Controller\UserController::confirmCancel which is used to

Confirms cancelling a user account via an email link

and in there is does this:

$edit = [
          'user_cancel_notify' => isset($account_data['cancel_notify']) ? $account_data['cancel_notify'] : $this->config('user.settings')->get('notify.status_canceled'),
        ];
        user_cancel($edit, $user->id(), $account_data['cancel_method']);
        // Since user_cancel() is not invoked via Form API, batch processing
        // needs to be invoked manually and should redirect to the front page
        // after completion.
        return batch_process('<front>');

So I think that's a clue to what we could try here.

Going to try that in the branch.

Pushed a batch process approach that passes tests

That change passed tests, and should respect batch handling from group module *I think*

Tested manually as follows:

1. Fresh drupal installation with jsonapi, groups and basic_auth enabled
2. Applied merge request
3. Changed account settings to "Delete the account and make its content belong to the Anonymous user. Reassign its groups to the super administrator."
4. Created a test user in group administrators (password test)
5. Created 12 groups owned by that user. 12 because >10 triggers the batch API as mentioned in the linked code by @alexpott).
6. Sent an delete to that user on a incognito chrome window:
   

   const response = await fetch('https://drupal-core.dev.swis.nl/jsonapi/user/user/2e28b1f8-ba34-43d5-b1b3-537e0e593168', {
       method: 'DELETE',
       headers: {
           'Accept': 'application/vnd.api+json', 
           'Authorization': 'Basic dGVzdDp0ZXN0'
       }
   });
   console.log(await response.json());

7. Verified all groups were reassigned to root

This is working as expected yay!

Would be nice to get this merged asap :)

Settings to RTBC again.

Couple of questions on the MR.

Wow.

On one hand, this code in core is ancient and really in need of a refresh, but on the other hand - its not the job of JSON:API to drag up 12 year old plus issues.

Wow.

I have strong concerns about calling batch_process() and expecting it to complete within a single request (so, usually within 30 seconds).

Wim I understand your performance issue. But that is currently is already in the code. It will delete ALL content related to the user. This will also break on very large sets. We don't introduce a new problem, we just make sure we aren't loosing data.

In mobile so can't really dive into the MR right now :)

But that is currently is already in the code.

Are you sure? I don't see any mentions of "batch" in core/modules/jsonapi? 🤔

@Wim Leers it uses the delete hook of user. This deletes all related content, not even in a batch at all. So yeah. I'm quite sure :)

Thas logic actually happend here: node_user_predelete and comment_user_predelete. As mentioned in the summary :)

I've pushed 2 more commits to the branch, mostly adding a test and removed some dulpicate code. Did resolve most threads also.

Rebased on 9.2.x

Left a comment on how I think you may be able to access the success/fail

I actually dumped the $batch variable after the batch processing and it was null. I think this happend in batch.inc line 484.

@bbrala pinged me about the above, batch.inc line 484 sets $batch to NULL when the batch is finished, so there doesn't seem to be a way to check if it succeeded or not - putting back to RTBC on that basis, I don't think there's much more we can do here.

FWIW the json api spec does have something to say about long running jobs - see https://jsonapi.org/recommendations/#asynchronous-processing - but in order to move to something like that we need to have queue processing and a queue runner that does not just run on cron... fun.

Committed and pushed 9d8e7da4fc to 9.3.x and 7c0c951f76 to 9.2.x. Thanks!

@bbrala can we get some follow-ups for implementing the queue processing support in JSON:API?

I'll handle opening an issue for generic queue processing outside cron.

#3068764: Refactor cron queue processing into its own class seems the place for generic queue processing (non cron)

Issue for exposing queues/jobs in json:api.

And nice find @larowlan ;)

Wow, pushing #3068764: Refactor cron queue processing into its own class and #1189464: Add an 'instant' queue runner forward is a nice unexpected but very welcome side effect of this issue! 🤩🥳