Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OPES-1549: Allow to delete users #206

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

OPES-1549: Allow to delete users #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37e21e3
OPES-1549: Add configuration to disable delete user options and permi…
AaronGilMartinez Feb 10, 2025
4462eec
OPES-1549: Revert option and require permission and configuration to …
AaronGilMartinez Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/install/oe_authentication.settings.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ validation_path: 'TicketValidationService'
assurance_level: TOP
ticket_types: SERVICE,PROXY
force_2fa: false
enable_user_delete_options: false
3 changes: 3 additions & 0 deletions config/schema/oe_authentication.schema.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,6 @@ oe_authentication.settings:
force_2fa:
type: boolean
label: 'Force two factor authentication'
enable_user_delete_options:
type: boolean
label: 'Enables the user delete options'
15 changes: 13 additions & 2 deletions oe_authentication.module
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,21 @@ declare(strict_types=1);
/**
* Implements hook_user_cancel_methods_alter().
*
* Remove the option to delete users while keeping the option to block them.
* Conditionally removes the option to delete users.
*/
function oe_authentication_user_cancel_methods_alter(&$methods) {
if (\Drupal::currentUser()->id() == 1) {
$current_user = \Drupal::currentUser();
$enable_user_delete = \Drupal::configFactory()
->get('oe_authentication.settings')
->get('enable_user_delete_options');

if (
$current_user->id() == 1 ||
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AaronGilMartinez AaronGilMartinez Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AaronGilMartinez AaronGilMartinez Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(
$enable_user_delete === TRUE &&
$current_user->hasPermission('access user delete options')
)
) {
return;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$restricted_options = [
Expand Down
3 changes: 3 additions & 0 deletions oe_authentication.permissions.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
administer authentication configuration:
title: 'Administer Authentication configuration'
restrict access: false
access user delete options:
title: 'Use the user delete options in "User cancel form"'
restrict access: true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AaronGilMartinez AaronGilMartinez Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

7 changes: 7 additions & 0 deletions src/Form/AuthenticationSettingsForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ 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['enable_user_delete_options'] = [
'#type' => 'checkbox',
'#title' => $this->t('Enable the user delete options'),
'#description' => $this->t('With this option enabled, users with the "access user delete options" permission will be allowed to delete users.'),
'#default_value' => $this->config(static::CONFIG_NAME)->get('enable_user_delete_options'),
];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AaronGilMartinez AaronGilMartinez Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return parent::buildForm($form, $form_state);
}

Expand All @@ -72,6 +78,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
->set('assurance_level', $form_state->getValue('assurance_level'))
->set('ticket_types', $form_state->getValue('ticket_types'))
->set('force_2fa', (bool) $form_state->getValue('force_2fa'))
->set('enable_user_delete_options', (bool) $form_state->getValue('enable_user_delete_options'))
->save();
parent::submitForm($form, $form_state);
}
Expand Down
40 changes: 40 additions & 0 deletions tests/Behat/AuthenticationContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,22 @@ public function visitOwnUserPage(): void {
$this->visitPath($url->getInternalPath());
}

/**
* Navigates to other user's page.
*
* @Given I visit the :user_name user page
*/
public function visitUserPage(string $user_name): void {
/** @var \Drupal\user\Entity\User $user */
$user = user_load_by_name($user_name);
if ($user === FALSE) {
throw new \Exception(sprintf('User with name %s could not be found.', $user_name));
}
/** @var \Drupal\Core\Url $url */
$url = $user->toUrl();
$this->visitPath($url->getInternalPath());
}

/**
* Configures the the Drupal site so that users are active on creation.
*
Expand Down Expand Up @@ -143,4 +159,28 @@ public function allowExternalUsers(): void {
$this->configContext->setConfig('oe_authentication.settings', 'assurance_level', 'LOW');
}

/**
* Configures oe_authentication module to enable user delete options.
*
* @Given the site is configured to display user delete options
*/
public function setConfigEnableDeleteUsersOptions(): void {
$this->configContext->setConfig('oe_authentication.settings', 'enable_user_delete_options', TRUE);
}

/**
* Logs in as superuser user using default credentials.
*
* @Given I am logged in as the superuser user
*/
public function loginSuperuser(): void {
$user = (object) [
'uid' => 1,
'name' => 'admin',
'pass' => 'admin',
];
// Login.
$this->login($user);
}

}
3 changes: 3 additions & 0 deletions tests/features/configure_authentication.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Feature: Authentication
And the "Application assurance levels" field should contain "TOP"
And the "Application available ticket types" field should contain "SERVICE,PROXY"
And the "Force two factor authentication" checkbox should not be checked
And the "Enable the user delete options" checkbox should not be checked

# Change the configuration values.
When I fill in "Application authentication protocol" with "something"
Expand All @@ -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 check the box "Enable the user delete options"
And I press "Save configuration"
Then I should see the message "The configuration options have been saved."
And the "Application authentication protocol" field should contain "something"
Expand All @@ -33,3 +35,4 @@ Feature: Authentication
And the "Application assurance levels" field should contain "assurance"
And the "Application available ticket types" field should contain "ticket.test"
And the "Force two factor authentication" checkbox should be checked
And the "Enable the user delete options" checkbox should be checked
54 changes: 52 additions & 2 deletions tests/features/user-management.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,63 @@
@api
@DrupalLogin
Feature: Manage users
Because users are managed externally
I should not be able to delete users on the site
Unless I am superuser user
Or module configuration to access the user delete options is enabled and I have
the permission 'access user delete options'

@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."
And I should see "Disable the account and unpublish its content."
And I should not see "Delete the account"

Scenario: As the superuser I can access the delete options
Given users:
| name | mail | roles |
| foo | [email protected] | |
And I am logged in as the superuser user
When I visit the foo 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 see "Disable the account and unpublish its content."
And I should see "Delete the account and its content."
And I should see "Delete the account and make its content belong to the Anonymous user."

Scenario: Users should not be able to delete other users when the configuration
is not enabled but the permission 'access user delete options' has been given.
And I am logged in as a user with the "administer users,access user delete options" 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 see "Disable the account and unpublish its content."
And I should not see "Delete the account"

Scenario: Users should not be able to delete other users when the configuration
is enabled but the permission 'access user delete options' has not been given.
Given the site is configured to display user delete options
And 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 see "Disable the account and unpublish its content."
And I should not see "Delete the account"

Scenario: Users should be able to delete other users when the configuration is
enabled and the permission 'access user delete options' has been given.
Given the site is configured to display user delete options
And I am logged in as a user with the "administer users,access user delete options" 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 see "Disable the account and unpublish its content."
And I should see "Delete the account and its content."
And I should see "Delete the account and make its content belong to the Anonymous user."