From 932a9974cbdb25e8eff2cc21caf6186ab9d3e30c Mon Sep 17 00:00:00 2001 From: Santeri Hurnanen Date: Mon, 26 Aug 2024 08:18:17 +0300 Subject: [PATCH 1/6] UHF-10045: Move infofinland_user_cancel to helfi_platform_config https://github.com/City-of-Helsinki/drupal-infofinland/tree/2024-08-21.1/public/modules/custom/infofinland_user_cancel --- modules/helfi_users/helfi_users.info.yml | 5 + modules/helfi_users/helfi_users.module | 92 ++++++++++++++++++ .../helfi_users/helfi_users.permissions.yml | 0 .../Kernel/UserCancelNodeRevisionsTest.php | 97 +++++++++++++++++++ 4 files changed, 194 insertions(+) create mode 100644 modules/helfi_users/helfi_users.info.yml create mode 100644 modules/helfi_users/helfi_users.module rename helfi_platform_config.permissions.yml => modules/helfi_users/helfi_users.permissions.yml (100%) create mode 100644 modules/helfi_users/tests/src/Kernel/UserCancelNodeRevisionsTest.php diff --git a/modules/helfi_users/helfi_users.info.yml b/modules/helfi_users/helfi_users.info.yml new file mode 100644 index 000000000..9d502a1e2 --- /dev/null +++ b/modules/helfi_users/helfi_users.info.yml @@ -0,0 +1,5 @@ +name: HELfi users +description: 'Fixes related to deleting or canceling users.' +package: HELfi +type: module +core_version_requirement: ^10 || ^11 diff --git a/modules/helfi_users/helfi_users.module b/modules/helfi_users/helfi_users.module new file mode 100644 index 000000000..7bf64e337 --- /dev/null +++ b/modules/helfi_users/helfi_users.module @@ -0,0 +1,92 @@ + $group] + $implementations; + } +} + +/** + * Implements hook_user_cancel(). + * + * We have encountered crashes/timeout issues with reassign batch api + * implementation from node module. This Optimizes hook_user_cancel by + * reassigning nodes with direct database query. + * + * This has to run before node module's user_cancel hook. + */ +function helfi_users_user_cancel($edit, UserInterface $account, $method): void { + switch ($method) { + case 'user_cancel_reassign': + // Anonymize all the nodes for this old account. + _helfi_users_reassign_nodes($account, User::load(1)); + break; + } +} + +/** + * Reassigns all node revisions from $source to $target. + * + * Prevents crashes and timeouts when revisions are handled by node_mass_update. + * + * @param AccountInterface $source + * Source user. + * @param AccountInterface $target + * Target user. + */ +function _helfi_users_reassign_nodes(AccountInterface $source, AccountInterface $target): void { + $database = \Drupal::database(); + $tables = [ + 'node_field_data' => 'uid', + 'node_field_revision' => 'uid', + 'node_revision' => 'revision_uid', + ]; + + foreach ($tables as $table => $uid_field) { + $matches = $database->select($table) + ->condition($uid_field, $source->id()) + ->countQuery() + ->execute() + ->fetchField(); + + if ((int) $matches < 1) { + continue; + } + + // Notice: this does not invalidate any caches. This should be fine for + // HELfi, where user information is not rendered on public pages. + $database->update($table) + ->fields([$uid_field => $target->id()]) + ->condition($uid_field, $source->id()) + ->execute(); + + \Drupal::logger('helfi_users')->notice(t('Set @count rows from @table to @target from @source', [ + '@count' => $matches, + '@table' => $table, + '@target' => $target->id(), + '@source' => $source->id(), + ])); + } +} diff --git a/helfi_platform_config.permissions.yml b/modules/helfi_users/helfi_users.permissions.yml similarity index 100% rename from helfi_platform_config.permissions.yml rename to modules/helfi_users/helfi_users.permissions.yml diff --git a/modules/helfi_users/tests/src/Kernel/UserCancelNodeRevisionsTest.php b/modules/helfi_users/tests/src/Kernel/UserCancelNodeRevisionsTest.php new file mode 100644 index 000000000..0f0adac52 --- /dev/null +++ b/modules/helfi_users/tests/src/Kernel/UserCancelNodeRevisionsTest.php @@ -0,0 +1,97 @@ +installEntitySchema($entityTypeId); + } + + $this->installSchema('node', 'node_access'); + + $this->admin = $this->createUser(name: 'admin', values: [ + 'uid' => 1, + ]); + } + + /** + * Tests revision reassign function. + */ + public function testRevisionAnonymization(): void { + $testUser = $this->createUser(name: 'testuser'); + $node = $this->createNode([ + 'uid' => $testUser->id(), + ]); + + // Create few revisions for a total of 4 including the original. + $revision_count = 3; + for ($i = 0; $i < $revision_count; $i++) { + $node->setNewRevision(); + $node + ->setTitle($this->randomMachineName()) + ->save(); + } + + // Run function for test user, assign content to . + _helfi_users_reassign_nodes($testUser, $this->admin); + + // Test that revisions for this user were anonymized correctly. + $connection = Database::getConnection(); + $revision_count = $connection->select('node_revision') + ->condition('revision_uid', $testUser->id()) + ->condition('nid', $node->id()) + ->countQuery() + ->execute() + ->fetchField(); + $this->assertEquals(0, (int) $revision_count, 'Found revisions after running anonymization function.'); + + // Test that the revisions were correctly assigned to target user. + $anon_revision_count = $connection->select('node_revision') + ->condition('revision_uid', $this->admin->id()) + ->condition('nid', $node->id()) + ->countQuery() + ->execute() + ->fetchField(); + $this->assertEquals(4, (int) $anon_revision_count, 'Amount of anonymized revisions does not match'); + } + +} From 1b2885e1a4a28c6763c1e831456c76002abb8fe3 Mon Sep 17 00:00:00 2001 From: Santeri Hurnanen Date: Mon, 26 Aug 2024 08:18:28 +0300 Subject: [PATCH 2/6] UHF-10045: Improve instructions --- README.md | 1 + helfi_platform_config.module | 25 --------- modules/helfi_users/Readme.md | 7 +++ modules/helfi_users/helfi_users.info.yml | 2 + modules/helfi_users/helfi_users.module | 51 +++++++++++++++++-- .../helfi_users/helfi_users.permissions.yml | 4 +- modules/helfi_users/translations/fi.po | 14 +++++ 7 files changed, 72 insertions(+), 32 deletions(-) create mode 100644 modules/helfi_users/Readme.md create mode 100644 modules/helfi_users/translations/fi.po diff --git a/README.md b/README.md index 4649a7c28..c63e5927f 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,7 @@ This repository holds configuration for the Hel.fi platform. - [Update instructions (2.x to 3.x)](documentation/update.md) - [Two-factor authentication/TFA/MFA](/modules/helfi_tfa/README.md) - [JSON:API remote entities](/modules/helfi_etusivu_entities/README.md) +- [Users](/modules/helfi_users/README.md) ## Contact diff --git a/helfi_platform_config.module b/helfi_platform_config.module index 2f9a32dc7..22c7ee45b 100644 --- a/helfi_platform_config.module +++ b/helfi_platform_config.module @@ -496,31 +496,6 @@ function helfi_platform_config_config_ignore_settings_alter(array &$settings) { } } -/** - * Implements hook_user_cancel_methods_alter(). - */ -function helfi_platform_config_user_cancel_methods_alter(array &$methods): void { - /** @var \Drupal\Core\Session\AccountInterface $account */ - $account = \Drupal::currentUser(); - - // Only allow user to disable user accounts if the user doesn't have - // a permission to delete user accounts. - $white_listed_methods = [ - 'user_cancel_block', - 'user_cancel_block_unpublish', - ]; - - // Deny access to all non-whitelisted methods if user doesn't have - // the 'delete user accounts' permission. - if (!$account->hasPermission('delete user accounts')) { - foreach ($methods as $name => &$method) { - if (!in_array($name, $white_listed_methods)) { - $method['access'] = FALSE; - } - } - } -} - /** * Implements hook_config_schema_info_alter(). */ diff --git a/modules/helfi_users/Readme.md b/modules/helfi_users/Readme.md new file mode 100644 index 000000000..4643be34b --- /dev/null +++ b/modules/helfi_users/Readme.md @@ -0,0 +1,7 @@ +# HELfi users + +Fixes related to deleting or banning users. + +Other user account related fixes and features: + - [Expired users](https://github.com/City-of-Helsinki/drupal-module-helfi-api-base/blob/main/documentation/user-expire.md) + - [Infofinland user sanitization](https://github.com/City-of-Helsinki/drupal-infofinland/blob/dev/public/modules/custom/infofinland_user_cancel/README.md) diff --git a/modules/helfi_users/helfi_users.info.yml b/modules/helfi_users/helfi_users.info.yml index 9d502a1e2..79569281f 100644 --- a/modules/helfi_users/helfi_users.info.yml +++ b/modules/helfi_users/helfi_users.info.yml @@ -3,3 +3,5 @@ description: 'Fixes related to deleting or canceling users.' package: HELfi type: module core_version_requirement: ^10 || ^11 +'interface translation project': helfi_users +'interface translation server pattern': modules/contrib/helfi_platform_config/modules/helfi_users/translations/%language.po diff --git a/modules/helfi_users/helfi_users.module b/modules/helfi_users/helfi_users.module index 7bf64e337..1b5390e31 100644 --- a/modules/helfi_users/helfi_users.module +++ b/modules/helfi_users/helfi_users.module @@ -10,7 +10,9 @@ declare(strict_types=1); * and we can reassign module weights / run order if necessary. */ +use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Session\AccountInterface; +use Drupal\Core\StringTranslation\TranslatableMarkup; use Drupal\user\Entity\User; use Drupal\user\UserInterface; @@ -28,6 +30,47 @@ function helfi_users_module_implements_alter(&$implementations, $hook) : void { } } +/** + * Implements hook_form_alter(). + */ +function helfi_users_form_alter(&$form, FormStateInterface $form_state, $form_id): void { + if (!in_array($form_id, ["user_multiple_cancel_confirm", "user_cancel_form"])) { + return; + } + + // Hide email confirmation checkbox. + $form['user_cancel_confirm']['#access'] = FALSE; + $form['user_cancel_method']['#description'] = new TranslatableMarkup( + "Banning accounts prevents them from logging in. If the account is no longer needed, it should be deleted." + ); +} + +/** + * Implements hook_user_cancel_methods_alter(). + */ +function helfi_users_user_cancel_methods_alter(array &$methods): void { + // User can only access allowed methods. User must also have + // 'administer users' permission from core to be able to cancel users. + $allowed_methods = [ + 'user_cancel_block' => new TranslatableMarkup("Ban the account and keep their content."), + 'user_cancel_block_unpublish' => new TranslatableMarkup("Ban the account and unpublish their content."), + 'user_cancel_reassign' => new TranslatableMarkup("Delete the account and make their content belong to %uid1. This action cannot be undone.", [ + '%uid1' => User::load(1)->getAccountName(), + ]), + ]; + + foreach ($allowed_methods as $name => $title) { + $methods[$name]['title'] = $title; + } + + // Without special permission, user is not allowed to access all methods. + if (!\Drupal::currentUser()->hasPermission('allow all user cancel methods')) { + foreach ($methods as $name => &$method) { + $method['access'] = array_key_exists($name, $allowed_methods); + } + } +} + /** * Implements hook_user_cancel(). * @@ -38,11 +81,9 @@ function helfi_users_module_implements_alter(&$implementations, $hook) : void { * This has to run before node module's user_cancel hook. */ function helfi_users_user_cancel($edit, UserInterface $account, $method): void { - switch ($method) { - case 'user_cancel_reassign': - // Anonymize all the nodes for this old account. - _helfi_users_reassign_nodes($account, User::load(1)); - break; + // Reassign nodes for the old account. + if ($method === 'user_cancel_reassign') { + _helfi_users_reassign_nodes($account, User::load(1)); } } diff --git a/modules/helfi_users/helfi_users.permissions.yml b/modules/helfi_users/helfi_users.permissions.yml index 65cb8132e..d605b1dad 100644 --- a/modules/helfi_users/helfi_users.permissions.yml +++ b/modules/helfi_users/helfi_users.permissions.yml @@ -1,2 +1,2 @@ -delete user accounts: - title: Delete user accounts +allow all user cancel methods: + title: All access to all user cancel methods diff --git a/modules/helfi_users/translations/fi.po b/modules/helfi_users/translations/fi.po new file mode 100644 index 000000000..bf1eb9146 --- /dev/null +++ b/modules/helfi_users/translations/fi.po @@ -0,0 +1,14 @@ +msgid "" +msgstr "" + +msgid "Ban the account and keep their content." +msgstr "Estä käyttäjä ja säilytä luotu sisältö." + +msgid "Ban the account and unpublish their content." +msgstr "Estä käyttäjä ja piilota luotu sisältö." + +msgid "Delete the account and make their content belong to %uid1. This action cannot be undone." +msgstr "Poista käyttäjä ja siirrä luotu sisältö käyttäjälle %uid1. Tätä toimintoa ei voi peruuttaa." + +msgid "Banning accounts prevents them from logging in. If the account is no longer needed, it should be deleted." +msgstr "Käyttäjän estäminen estää kirjautumisen käyttäjällä. Käyttäjä tulisi poistaa jos sitä ei enää tarvita." From a50fbcc4727ceef1a4e144ab85893b6406e7f1cb Mon Sep 17 00:00:00 2001 From: Santeri Hurnanen Date: Mon, 26 Aug 2024 08:19:10 +0300 Subject: [PATCH 3/6] UHF-10045: Install helfi_users module --- helfi_platform_config.install | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/helfi_platform_config.install b/helfi_platform_config.install index 72cc3ac42..7a7adeeb1 100644 --- a/helfi_platform_config.install +++ b/helfi_platform_config.install @@ -168,3 +168,16 @@ function helfi_platform_config_update_9311(): void { } } } + +/** + * UHF-9708: Enable helfi_users module. + */ +function helfi_platform_config_update_9312() : void { + helfi_platform_config_remove_permissions_from_all_roles([ + 'delete user accounts', + ]); + + if (!\Drupal::moduleHandler()->moduleExists('helfi_users')) { + \Drupal::service('module_installer')->install(['helfi_users']); + } +} From 999387ab593c37c4f247f7508c9b450797f54ba3 Mon Sep 17 00:00:00 2001 From: Santeri Hurnanen Date: Mon, 26 Aug 2024 08:48:13 +0300 Subject: [PATCH 4/6] UHF-10045: Lock default_content to alpha2 Latest changes break our patch --- composer.json | 1 + 1 file changed, 1 insertion(+) diff --git a/composer.json b/composer.json index 5305be1e3..f2868651e 100644 --- a/composer.json +++ b/composer.json @@ -75,6 +75,7 @@ "drupal/ctools": "<3.11 || ^4.0.1", "drupal/gin_toolbar": ">1.0.0-rc6", "drupal/helfi_media_map": "*", + "drupal/default_content": ">2.0.0-alpha2", "drush/drush": "<12" }, "extra": { From 7f6511cb1483c6cae33f94698c6f219938cdcc7e Mon Sep 17 00:00:00 2001 From: Santeri Hurnanen Date: Mon, 26 Aug 2024 08:54:28 +0300 Subject: [PATCH 5/6] UHF-10045: Fix phpcs --- modules/helfi_users/helfi_users.module | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/helfi_users/helfi_users.module b/modules/helfi_users/helfi_users.module index 1b5390e31..6e40656d6 100644 --- a/modules/helfi_users/helfi_users.module +++ b/modules/helfi_users/helfi_users.module @@ -1,7 +1,5 @@ Date: Mon, 26 Aug 2024 11:15:58 +0300 Subject: [PATCH 6/6] UHF-10045: Fix tests --- .../helfi_users/tests}/src/Functional/UserCancelFormTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename {tests => modules/helfi_users/tests}/src/Functional/UserCancelFormTest.php (95%) diff --git a/tests/src/Functional/UserCancelFormTest.php b/modules/helfi_users/tests/src/Functional/UserCancelFormTest.php similarity index 95% rename from tests/src/Functional/UserCancelFormTest.php rename to modules/helfi_users/tests/src/Functional/UserCancelFormTest.php index 1aad023ac..ebc95b00b 100644 --- a/tests/src/Functional/UserCancelFormTest.php +++ b/modules/helfi_users/tests/src/Functional/UserCancelFormTest.php @@ -18,7 +18,7 @@ class UserCancelFormTest extends BrowserTestBase { */ protected static $modules = [ 'user', - 'helfi_platform_config', + 'helfi_users', ]; /** @@ -60,7 +60,7 @@ public function testUserCancelForm(): void { $this->assertSession()->statusCodeEquals(200); $this->assertSession()->elementExists('xpath', '//input[@value="user_cancel_block"]'); $this->assertSession()->elementExists('xpath', '//input[@value="user_cancel_block_unpublish"]'); - $this->assertSession()->elementNotExists('xpath', '//input[@value="user_cancel_reassign"]'); + $this->assertSession()->elementExists('xpath', '//input[@value="user_cancel_reassign"]'); $this->assertSession()->elementNotExists('xpath', '//input[@value="user_cancel_delete"]'); // Test that the editorUser cannot access the cancel page.