OPES-1549: Allow to delete users #206

AaronGilMartinez · 2025-02-10T14:16:02Z

Fixes #184

donquixote · 2025-02-12T13:49:38Z

tests/features/configure_authentication.feature

@@ -25,6 +26,7 @@ Feature: Authentication
    And I fill in "Application assurance levels" with "assurance"
    And I fill in "Application available ticket types" with "ticket.test"
    And I check the box "Force two factor authentication"
+    And I uncheck the box "Disable the user delete options"


Isn't one piece missing here in the section below, like "And the ... checkbox should not be checked"

donquixote · 2025-02-12T15:03:31Z

oe_authentication.permissions.yml

@@ -1,3 +1,6 @@
 administer authentication configuration:
  title: 'Administer Authentication configuration'
  restrict access: false
+delete users:
+  title: 'Allows the user to use delete user options in "User cancel form"'
+  restrict access: true


I feel uneasy about introducing a permission with such a generic name that could easily clash with permissions introduced by other modules or by future Drupal core versions.

If this has been agreed with others, then ok.

Ofc, if any other module or core is coming up with a similar option and/or permission, we do likely have a problem either way, independent of the name clash.

This said, I don't have a good suggestion on namespacing the permission.
Permission machine names are typically not strictly namespaced, but they usually contain some piece that make them less likely to clash.

The name seems a bit generic indeed.
Since the permission is making available the options, maybe something related to what is doing would make more sense and hard to clash, like:

use delete user options

delete user options access

access delete user options

Renamed to: access user delete options
Describes what it does and has a similar name to the configuration which helps to see a relation between each other.
It's more concise to avoid collision with other possible names.

donquixote · 2025-02-12T15:07:49Z

oe_authentication.module

@@ -13,7 +13,16 @@ declare(strict_types=1);
 * Remove the option to delete users while keeping the option to block them.


This line needs to change now.
Also, to follow conventions it would say "Removes..." instead of "Remove...".

I like "permanently delete" to make it more clear. With this, I think we no longer need to mention "while keeping the option to block them". But can be discussed.

Suggested change

* Remove the option to delete users while keeping the option to block them.

* Removes the option to permanently delete users, depending on settings and permissions.

or

Suggested change

* Remove the option to delete users while keeping the option to block them.

* Conditionally removes the option to permanently delete users.

Any of the two is ok for me.
About 'permanently' I'm not sure about it, is not that you can delete temporarily.
The entity is deleted or not, seems more a problem of the options themselves that aren't clear enough.

donquixote · 2025-02-12T15:10:08Z

oe_authentication.module

+
+  if (
+    $disabled_user_delete !== TRUE ||
+    $current_user->id() == 1 ||


Doesn't ->hasPermission() already cover the user 1 case?
I mean it returns true for user 1 for all perms, right?

True, actually this includes admin users.
The way permissions are calculated is: is it admin or has the permission?
Which grants access to the users.
Not sure about it.

Since I want to keep functionality as is, I think is better to check the permission directly.

I discard the direct comparisson, inverting the option seems a better idea.
We have two options to allow to see the options:

When option is enabled or user has the permission.

Problem here is that we are granting access to other users than 1: admin users.

(preferred) When user is superuser or (option is enabled and has the permission).

This requires two conditions for the users to get access to the options, but keeps bc compatibility.

donquixote · 2025-02-12T15:11:19Z

config/schema/oe_authentication.schema.yml

@@ -20,3 +20,6 @@ oe_authentication.settings:
    force_2fa:
      type: boolean
      label: 'Force two factor authentication'
+    disable_user_delete:
+      type: boolean
+      label: 'Disable the user delete options'


More accurate than disable_user_delete would be restrict_user_delete.
Then for the description "Require a dedicated permission to permanently delete users."

Inverted the option so the name has changed totally.

donquixote · 2025-02-12T15:17:10Z

oe_authentication.permissions.yml

@@ -1,3 +1,6 @@
 administer authentication configuration:
  title: 'Administer Authentication configuration'
  restrict access: false
+delete users:
+  title: 'Allows the user to use delete user options in "User cancel form"'


In a permission title we don't say "Allows a user", we just describe the action itself.
(More can be in a description if needed)

Suggested change

title: 'Allows the user to use delete user options in "User cancel form"'

title: 'Use the delete user options in "User cancel form"'

Besides this, I like the phrase "permanently delete" to distinguish from other cancel options.

Suggested change

title: 'Allows the user to use delete user options in "User cancel form"'

title: 'Permanently delete users in the "User cancel form"'

TBD if you want.

Used first one.

donquixote · 2025-02-12T15:18:12Z

oe_authentication.permissions.yml

@@ -1,3 +1,6 @@
 administer authentication configuration:
  title: 'Administer Authentication configuration'
  restrict access: false
+delete users:
+  title: 'Allows the user to use delete user options in "User cancel form"'
+  restrict access: true


Ofc, if any other module or core is coming up with a similar option and/or permission, we do likely have a problem either way, independent of the name clash.

donquixote · 2025-02-12T15:19:53Z

oe_authentication.permissions.yml

@@ -1,3 +1,6 @@
 administer authentication configuration:
  title: 'Administer Authentication configuration'
  restrict access: false
+delete users:
+  title: 'Allows the user to use delete user options in "User cancel form"'
+  restrict access: true


This said, I don't have a good suggestion on namespacing the permission.
Permission machine names are typically not strictly namespaced, but they usually contain some piece that make them less likely to clash.

donquixote · 2025-02-12T15:23:35Z

oe_authentication.post_update.php

+  \Drupal::configFactory()->getEditable('oe_authentication.settings')
+    ->set('disable_user_delete', TRUE)
+    ->save();
+}


Do we 100% rely on the update hook, or do we add ?? TRUE in respective places to protect site that did not run their update hooks?
Just because this is a security-relevant feature.

(Btw we don't need a ?? TRUE if instead the setting is inverted, so it would be "Bypass the delete users permission" rather than "Require the delete users permission".)

The module relied on update_hooks for other configuration, I did not want to add it just to this configuration, feels something should be done for all the configuration keys but is not in the scope.

Yes, I guess that the setting could be inverted, would mean no path update. I'm considering it.

Inverted the config at the end so no update path is needed.

donquixote · 2025-02-12T15:26:35Z

src/Form/AuthenticationSettingsForm.php

@@ -58,6 +58,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
      '#title' => $this->t('Force two factor authentication'),
      '#default_value' => $this->config(static::CONFIG_NAME)->get('force_2fa'),
    ];
+    $form['disable_user_delete'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Disable the user delete options'),


What about

Suggested change

'#title' => $this->t('Disable the user delete options'),

'#title' => $this->t('Restrict the options to permanently delete users'),

'#description' => $this->t('With this option enabled, only users with the *** permission will be allowed to permanently delete users.'),

(but see other comment below about radios)

Title no longer applies, but I like the description.

donquixote · 2025-02-12T15:32:26Z

src/Form/AuthenticationSettingsForm.php

+      '#type' => 'checkbox',
+      '#title' => $this->t('Disable the user delete options'),
+      '#default_value' => $this->config(static::CONFIG_NAME)->get('disable_user_delete'),
+    ];


An alternative would be to use radios for this, instead of single checkbox.
But not 100% sure. If the wording for a single checkbox is good, then we don't need radios.

The internal setting can remain a boolean.

With radios we can properly describe the effect of each option, without saying "if this is checked" or "if this is unchecked".

E.g.

( ) Any user with the permission to cancel accounts can choose to permanently delete that user.

( ) Only users with the *** permission can permanently delete users.

When I think about it more, wouldn't this setting belong into "Account settings" form, next to "When canceling a user account"?

The module uses the checkbox for other boolean fields, like 2fa authentication.
And for the place, since it's something the module it's doing seems the right place with other configuration.

donquixote · 2025-02-12T17:21:59Z

oe_authentication.module

+    $disabled_user_delete !== TRUE ||
+    $current_user->id() == 1 ||
+    $current_user->hasPermission('delete users')
+  ) {
    return;
  }


(side note)
In the code below we unset the respective options.
But I see that the items in function user_cancel_methods() have an optional 'access' key, which could be set to hide options.
However I am not really sure about the impact, so let's leave this part as is for now, and just keep it in mind for later.

I don't like the idea of modifing/refactoring the current code/behaviour just the minimum to allow the deletion.

donquixote

First batch of feedback.
I want to submit this already, but I still want to have a closer look at the new tests.

donquixote · 2025-02-12T18:05:38Z

tests/features/user-management.feature

  Scenario: Users should not be able to delete other users
    Given I am logged in as a user with the "administer users" permissions
    When I visit my user page
    And I click "Edit"
    And I click "Cancel account"
    Then I should see "Disable the account and keep its content."
    And I should not see "Delete the account and its content."
+
+  Scenario: As a user with permission "delete users" I can access the options
+    Given I am logged in as a user with the "administer users, delete users" permissions


We are only testing delete own here, so we should not grant the "administer users" permission.
Instead, give "Cancel own user account", and "Select method for cancelling account".

In fact the pre-existing scenario has the same problem.

Maybe we rely on the fact that with "Cancel own user account", and "Select method for cancelling account" the effect of "delete users" would be the same as with "administer users"?
Normally we try to avoid testing stuff that is not controlled by our module. But here it seems quite the overlap / not clean separation.

I would say yes, they are two paths to get to the same destination.
Indeed

donquixote · 2025-02-12T18:06:25Z

tests/features/user-management.feature

+    And I click "Edit"
+    And I click "Cancel account"
+    Then I should see "Disable the account and keep its content."
+    And I should see "Delete the account and its content."


I think we should cover all 4 methods, whether they are available or not.

Note that in core the 4th method already has a different access behavior than the 3rd method.

In a behat test can we just assert all options? E.g. the options for a field are A, B, C. So that, if option D appears, we get a fail.
I don't like the "I should not see ..." because this can lead to blind spots if we misspell the (not) expected text.

I understand, ideally we would assert that there are only 3 options and that they are the right ones.
No need of checking not available options.
Not sure if behat provides steps for this, I check, if not it will mean custom steps that I prefer to avoid.

Added checks for all the options, I think the best for the negative assertions is to use "Delete the account" as you suggest.

donquixote · 2025-02-12T18:17:03Z

tests/features/user-management.feature


-  @DrupalLogin
  Scenario: Users should not be able to delete other users
    Given I am logged in as a user with the "administer users" permissions
    When I visit my user page
    And I click "Edit"
    And I click "Cancel account"
    Then I should see "Disable the account and keep its content."
    And I should not see "Delete the account and its content."


So, this case has the same problem as below, we only test with "administer users" permission but not with "cancel own.." and "select method for cancelling..".

Also we only cover the absence of "Delete the account and its content." but we miss to also cover the absence of "Delete the account and make its content belong to the %anonymous-name user."

(also see below about the fragility of testing the absence of something)

Btw we can cover two "should not see" in one go if we only specify a partial phrase "Delete the account".

donquixote · 2025-02-12T18:40:11Z

tests/Behat/AuthenticationContext.php

+    /** @var \Drupal\user\Entity\User $user */
+    $user = user_load_by_name($user_name);
+    if ($user === FALSE) {
+      throw new \InvalidArgumentException(sprintf('User with name %s could not be found.', $user_name));


In the behat contexts in Drupal core I don't see "InvalidArgumentException" used, and only very few RuntimeException. Instead I see regular exception or ->assert*() calls.

To me, InvalidArgumentException and RuntimeException should indicate clear programming errors, but here the failure is conditional on the application state and data.

True, I use Exception instead.

…ssion to grant access to the options.

…grant access.

AaronGilMartinez force-pushed the disable-delete-user branch 3 times, most recently from 4b81d7f to 3b80ff9 Compare February 11, 2025 08:41

AaronGilMartinez marked this pull request as ready for review February 11, 2025 08:43

donquixote reviewed Feb 12, 2025

View reviewed changes

donquixote requested changes Feb 12, 2025

View reviewed changes

donquixote reviewed Feb 12, 2025

View reviewed changes

AaronGilMartinez added 2 commits February 13, 2025 19:54

OPES-1549: Add configuration to disable delete user options and permi…

37e21e3

…ssion to grant access to the options.

OPES-1549: Revert option and require permission and configuration to …

4462eec

…grant access.

AaronGilMartinez force-pushed the disable-delete-user branch from e584f0c to 4462eec Compare February 13, 2025 18:54

		@@ -13,7 +13,16 @@ declare(strict_types=1);
		* Remove the option to delete users while keeping the option to block them.

	* Remove the option to delete users while keeping the option to block them.
	* Removes the option to permanently delete users, depending on settings and permissions.

	* Remove the option to delete users while keeping the option to block them.
	* Conditionally removes the option to permanently delete users.

	title: 'Allows the user to use delete user options in "User cancel form"'
	title: 'Use the delete user options in "User cancel form"'

	title: 'Allows the user to use delete user options in "User cancel form"'
	title: 'Permanently delete users in the "User cancel form"'

	'#title' => $this->t('Disable the user delete options'),
	'#title' => $this->t('Restrict the options to permanently delete users'),
	'#description' => $this->t('With this option enabled, only users with the *** permission will be allowed to permanently delete users.'),

OPES-1549: Allow to delete users #206

Are you sure you want to change the base?

OPES-1549: Allow to delete users #206

Conversation

AaronGilMartinez commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez Feb 13, 2025 •

edited

Loading

AaronGilMartinez Feb 13, 2025 •

edited

Loading

AaronGilMartinez Feb 13, 2025 •

edited

Loading

AaronGilMartinez Feb 13, 2025 •

edited

Loading

AaronGilMartinez Feb 13, 2025 •

edited

Loading

AaronGilMartinez Feb 13, 2025 •

edited

Loading

AaronGilMartinez Feb 13, 2025 •

edited

Loading