backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Account cancelation methods that specify `#access` are still being offered as a default account self-cancelation method #5945

Open klonos opened 1 year ago

klonos commented 1 year ago

I came across this while working on #3422.

On a fresh installation, head to admin/config/people/settings -> all 4 cancelation methods are available to be selected as the default: image

In user_admin_settings() however, there's this bit of code, along with an inline comment that says:

    // Remove all account cancellation methods that have #access defined, as
    // those cannot be configured as default method.

Now, if you have a look in user_cancel_methods(), you will clearly see that the 'user_cancel_delete' method (the one with 'Delete the account and its content.' as its label) has 'access' => user_access('administer users') defined.

The same is mentioned in hook_user_cancel_methods_alter(): https://github.com/backdrop/backdrop/blob/1.x/core/modules/user/user.api.php#L148

 * - access: (optional) A boolean value indicating whether the user can access
 *   a method. If access is defined, the method cannot be configured as the
 *   default method.

So according to the code and the inline comment, the intention is for that cancelation method to not be available in that set of radio buttons. ...either that, or we should remove the code and the comment from user_admin_settings().

Other thoughts (that can be solve here or in other issues, once we have clarified what the intended functionality is):

  1. Once this is fixed to work as intended, if we keep using ['#access'] = FALSE; as we do currently, then the option will be gone entirely from the form. That can be confusing if an admin tries to cancel a user account, and then sees that 4 options are available ...unless they are a developer and take a look at the code, they will start wondering why that 4th option is not available when setting the defaults. So I would suggest that we instead set the radio as always unticked and locked from user input (by setting '#disabled').
  2. If we do hide (or lock) this option, then what happens to existing sites that have it set as the default? Should we add an update hook to remove it? ...and if so, then what should we replace it with as the default.
  3. The user_cancel_delete option has 'access' => user_access('administer users'), which makes sense, since the action will delete a user account. Then why doesn't user_cancel_reassign option (which also deletes the user account) also have the same 'access' => user_access('administer users') defined?
  4. Another thing with the user_cancel_delete option is that it also says that All of your content will also be deleted. So how come we aren't checking that the user also has access to delete content?

In general, these options and the code are confusing and incomplete/inconsistent. If we cannot check things properly, then should we just remove all '#access'checks? 🤔

klonos commented 1 year ago

FWIW, the same situation applies to D7: image ...which also has the same 'access' => user_access('administer users') in its user_cancel_methods(): https://git.drupalcode.org/project/drupal/-/blob/7.x/modules/user/user.pages.inc#L610 ...and also the same inline comment and code in user_admin_settings(): https://git.drupalcode.org/project/drupal/-/blob/7.x/modules/user/user.admin.inc#L346

klonos commented 1 year ago

...latest D10 seems to have fixed that: image

edit: also checked in latest D9.5.2 and D8.9.20 and it's fixed there as well.

klonos commented 1 year ago

Quick PR that simply restores the intended functionality and brings Backdrop to parity with Drupal: https://github.com/backdrop/backdrop/pull/4317

Now, I need feedback with re to the points outlined in the issue summary (and whether these should be handled as part of this issue/PR or if they should be split into separate issues).

avpaderno commented 1 year ago

The account cancellation method selection is only shown to users who either have the administer users permission or the select account cancellation method permission. When an account cancellation method uses 'administer users' as 'access' is essentially not shown to normal users. I take the idea is that normal users should not be allowed to delete all their content to avoid that comments posted by other users on that content are lost.

user_cancel_delete should not be used as default cancellation method, or users who do not have the select account cancellation method permission would delete their accounts, their content, and any comment posted on their content. It should not be possible to select it as default cancellation method and not allowing to select as default a cancellation method that uses 'access' is a way to avoid that.

Another thing with the user_cancel_delete option is that it also says that All of your content will also be deleted. So how come we aren't checking that the user also has access to delete content?

user_cancel_delete is given to users with the administer users permission. They are already allowed to select that method, so there is no need to check they have the permission to delete content.