diff --git a/core/lib/Drupal/Core/Action/ActionBase.php b/core/lib/Drupal/Core/Action/ActionBase.php index 9278deb..57023a8 100644 --- a/core/lib/Drupal/Core/Action/ActionBase.php +++ b/core/lib/Drupal/Core/Action/ActionBase.php @@ -28,4 +28,11 @@ public function executeMultiple(array $entities) { } } + /** + * {@inheritdoc} + */ + public function getLabelArguments() { + return []; + } + } diff --git a/core/lib/Drupal/Core/Action/ActionInterface.php b/core/lib/Drupal/Core/Action/ActionInterface.php index ab9514c..9f0ff1c 100644 --- a/core/lib/Drupal/Core/Action/ActionInterface.php +++ b/core/lib/Drupal/Core/Action/ActionInterface.php @@ -63,4 +63,13 @@ public function executeMultiple(array $objects); */ public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE); + /** + * Gets FormattableMarkup arguments for the label. + * + * @see \Drupal\Component\Render\FormattableMarkup + * + * @return string[] + */ + public function getLabelArguments(); + } diff --git a/core/modules/action/src/ActionFormBase.php b/core/modules/action/src/ActionFormBase.php index 0b972d4..7fb3df6 100644 --- a/core/modules/action/src/ActionFormBase.php +++ b/core/modules/action/src/ActionFormBase.php @@ -91,7 +91,7 @@ public function form(array $form, FormStateInterface $form_state) { ); if ($this->plugin instanceof PluginFormInterface) { - $form += $this->plugin->buildConfigurationForm($form, $form_state); + $form = $this->plugin->buildConfigurationForm($form, $form_state); } return parent::form($form, $form_state); diff --git a/core/modules/system/src/Entity/Action.php b/core/modules/system/src/Entity/Action.php index b5a0f19..6f9157c 100644 --- a/core/modules/system/src/Entity/Action.php +++ b/core/modules/system/src/Entity/Action.php @@ -7,8 +7,11 @@ namespace Drupal\system\Entity; +use Drupal\Component\Render\FormattableMarkup; +use Drupal\Component\Render\PlainTextOutput; use Drupal\Core\Config\Entity\ConfigEntityBase; use Drupal\Core\Config\Entity\ConfigEntityInterface; +use Drupal\Core\Entity\EntityStorageInterface; use Drupal\Core\Entity\EntityWithPluginCollectionInterface; use Drupal\system\ActionConfigEntityInterface; use Drupal\Core\Action\ActionPluginCollection; @@ -86,6 +89,10 @@ class Action extends ConfigEntityBase implements ActionConfigEntityInterface, En */ protected function getPluginCollection() { if (!$this->pluginCollection) { + // Store the original label before argument substition in the plugin + // configuration so that forms can present the correct label to the user + // to edit. + $this->configuration['original_label'] = $this->label; $this->pluginCollection = new ActionPluginCollection(\Drupal::service('plugin.manager.action'), $this->plugin, $this->configuration); } return $this->pluginCollection; @@ -155,4 +162,28 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b) return parent::sort($a, $b); } + /** + * @inheritDoc + */ + public function label() { + $label_arguments = $this->getPlugin()->getLabelArguments(); + $label = $this->label; + if (!empty($label_arguments)) { + $label = new FormattableMarkup($label, $label_arguments); + // Remove the safeness from the label as the label can contain user input. + $label = PlainTextOutput::renderFromHtml($label); + } + return $label; + } + + /** + * {@inheritdoc} + */ + public function preSave(EntityStorageInterface $storage) { + parent::preSave($storage); + // Remove original label when saving so as this does not need to be stored + // in configuration. + unset($this->configuration['original_label']); + } + } diff --git a/core/modules/system/src/Plugin/views/field/BulkForm.php b/core/modules/system/src/Plugin/views/field/BulkForm.php index 3a3b65c..1ae8236 100644 --- a/core/modules/system/src/Plugin/views/field/BulkForm.php +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php @@ -292,7 +292,9 @@ public function viewsForm(&$form, FormStateInterface $form_state) { $form['header'][$this->options['id']]['action'] = array( '#type' => 'select', '#title' => $this->options['action_title'], - '#options' => $this->getBulkOptions(), + // This is a select list therefore apply the plain text formatter to the + // the options which are escaped for display in HTML. + '#options' => array_map('\Drupal\Component\Render\PlainTextOutput::renderFromHtml', $this->getBulkOptions()), ); // Duplicate the form actions into the action container in the header. diff --git a/core/modules/user/src/Plugin/Action/ChangeUserRoleBase.php b/core/modules/user/src/Plugin/Action/ChangeUserRoleBase.php index 12353b3..3f17fb9 100644 --- a/core/modules/user/src/Plugin/Action/ChangeUserRoleBase.php +++ b/core/modules/user/src/Plugin/Action/ChangeUserRoleBase.php @@ -14,6 +14,7 @@ use Drupal\Core\Plugin\ContainerFactoryPluginInterface; use Drupal\Core\Session\AccountInterface; use Drupal\user\RoleInterface; +use Drupal\user\RoleStorageInterface; use Symfony\Component\DependencyInjection\ContainerInterface; /** @@ -31,11 +32,19 @@ protected $entityType; /** + * The user role storage. + * + * @var \Drupal\Core\Entity\EntityTypeInterface + */ + protected $roleStorage; + + /** * {@inheritdoc} */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeInterface $entity_type) { + public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeInterface $entity_type, RoleStorageInterface $role_storage) { parent::__construct($configuration, $plugin_id, $plugin_definition); $this->entityType = $entity_type; + $this->roleStorage = $role_storage; } /** @@ -46,7 +55,8 @@ public static function create(ContainerInterface $container, array $configuratio $configuration, $plugin_id, $plugin_definition, - $container->get('entity.manager')->getDefinition('user_role') + $container->get('entity.manager')->getDefinition('user_role'), + $container->get('entity.manager')->getStorage('user_role') ); } @@ -63,6 +73,14 @@ public function defaultConfiguration() { * {@inheritdoc} */ public function buildConfigurationForm(array $form, FormStateInterface $form_state) { + // The label supports replacing @label with the role's label. + $form['label'] = array( + '#type' => 'textfield', + '#title' => $this->t('Label'), + '#default_value' => $this->configuration['original_label'], + '#maxlength' => '255', + '#description' => $this->t('A unique label for this advanced action. This label will be displayed in the interface of modules that integrate with actions. @role_label is replaced with the role\'s label.'), + ); $roles = user_role_names(TRUE); unset($roles[RoleInterface::AUTHENTICATED_ID]); $form['rid'] = array( @@ -104,4 +122,15 @@ public function access($object, AccountInterface $account = NULL, $return_as_obj return $return_as_object ? $access : $access->isAllowed(); } + /** + * {@inheritdoc} + */ + public function getLabelArguments() { + if (!empty($this->configuration['rid'])) { + $role = $this->roleStorage->load($this->configuration['rid']); + return ['@role_label' => $role->label()]; + } + return []; + } + } diff --git a/core/modules/user/src/Tests/UserAdminTest.php b/core/modules/user/src/Tests/UserAdminTest.php index cf942f9..b028b5c 100644 --- a/core/modules/user/src/Tests/UserAdminTest.php +++ b/core/modules/user/src/Tests/UserAdminTest.php @@ -42,7 +42,7 @@ function testUserAdmin() { $user_storage = $this->container->get('entity.manager')->getStorage('user'); // Create admin user to delete registered user. - $admin_user = $this->drupalCreateUser(array('administer users')); + $admin_user = $this->drupalCreateUser(array('administer users', 'administer permissions')); // Use a predictable name so that we can reliably order the user admin page // by name. $admin_user->name = 'Admin user'; @@ -95,6 +95,16 @@ function testUserAdmin() { $this->assertNoText($user_b->getUsername(), 'User B not on filtered by role on admin users page'); $this->assertText($user_c->getUsername(), 'User C on filtered by role on admin users page'); + // Check that a role is correctly escaped. + $role_name = "123 & more"; + $edit = array('label' => $role_name, 'id' => '123'); + $this->drupalPostForm('admin/people/roles/add', $edit, t('Save')); + $this->drupalGet('admin/people'); + // Assert escaped and XSS filtered correctly. + $this->assertEscaped("123 & alert('xss');more"); + $this->assertNoRaw(""); + $this->assertNoEscaped('&'); + // Test blocking of a user. $account = $user_storage->load($user_c->id()); $this->assertTrue($account->isActive(), 'User C not blocked'); diff --git a/core/modules/user/src/Tests/UserRoleAdminTest.php b/core/modules/user/src/Tests/UserRoleAdminTest.php index a01c01e..91ca773 100644 --- a/core/modules/user/src/Tests/UserRoleAdminTest.php +++ b/core/modules/user/src/Tests/UserRoleAdminTest.php @@ -8,6 +8,7 @@ namespace Drupal\user\Tests; use Drupal\simpletest\WebTestBase; +use Drupal\system\Entity\Action; use Drupal\user\Entity\Role; use Drupal\user\RoleInterface; @@ -58,12 +59,21 @@ function testRoleAdministration() { // Test adding a role. (In doing so, we use a role name that happens to // correspond to an integer, to test that the role administration pages // correctly distinguish between role names and IDs.) - $role_name = '123'; - $edit = array('label' => $role_name, 'id' => $role_name); + $role_name = '123 & more'; + $edit = array('label' => $role_name, 'id' => '123'); $this->drupalPostForm('admin/people/roles/add', $edit, t('Save')); - $this->assertRaw(t('Role %label has been added.', array('%label' => 123))); - $role = Role::load($role_name); + $this->assertRaw(t('Role %label has been added.', array('%label' => $role_name))); + $role = Role::load('123'); $this->assertTrue(is_object($role), 'The role was successfully retrieved from the database.'); + // Assert escaped correctly. + $this->assertEscaped('123 & more'); + + // Test that the corresponding actions have been created and have the + // expected label. + $action = Action::load('user_add_role_action.' . $role->id()); + $this->assertIdentical('Add the 123 & more role to the selected users', $action->label()); + $action = Action::load('user_remove_role_action.' . $role->id()); + $this->assertIdentical('Remove the 123 & more role from the selected users', $action->label()); // Check that the role was created in site default language. $this->assertEqual($role->language()->getId(), $default_langcode); @@ -80,6 +90,12 @@ function testRoleAdministration() { \Drupal::entityManager()->getStorage('user_role')->resetCache(array($role->id())); $new_role = Role::load($role->id()); $this->assertEqual($new_role->label(), $role_name, 'The role name has been successfully changed.'); + // Test that the corresponding actions still exist and have the + // expected label. + $action = Action::load('user_add_role_action.' . $role->id()); + $this->assertIdentical("Add the 456 role to the selected users", $action->label()); + $action = Action::load('user_remove_role_action.' . $role->id()); + $this->assertIdentical("Remove the 456 role from the selected users", $action->label()); // Test deleting a role. $this->drupalGet("admin/people/roles/manage/{$role->id()}"); @@ -89,6 +105,12 @@ function testRoleAdministration() { $this->assertNoLinkByHref("admin/people/roles/manage/{$role->id()}", 'Role edit link removed.'); \Drupal::entityManager()->getStorage('user_role')->resetCache(array($role->id())); $this->assertFalse(Role::load($role->id()), 'A deleted role can no longer be loaded.'); + // Test that the corresponding actions still exist and have the + // expected label. + $action = Action::load('user_add_role_action.' . $role->id()); + $this->assertNull($action, 'Add role action deleted.'); + $action = Action::load('user_remove_role_action.' . $role->id()); + $this->assertNull($action, 'Remove role action deleted.'); // Make sure that the system-defined roles can be edited via the user // interface. diff --git a/core/modules/user/tests/src/Unit/Plugin/Action/AddRoleUserTest.php b/core/modules/user/tests/src/Unit/Plugin/Action/AddRoleUserTest.php index a2f9e9c..c408ba4 100644 --- a/core/modules/user/tests/src/Unit/Plugin/Action/AddRoleUserTest.php +++ b/core/modules/user/tests/src/Unit/Plugin/Action/AddRoleUserTest.php @@ -28,7 +28,7 @@ public function testExecuteAddExistingRole() { ->will($this->returnValue(TRUE)); $config = array('rid' => 'test_role_1'); - $remove_role_plugin = new AddRoleUser($config, 'user_add_role_action', array('type' => 'user'), $this->userRoleEntityType); + $remove_role_plugin = new AddRoleUser($config, 'user_add_role_action', array('type' => 'user'), $this->userRoleEntityType, $this->roleStorage); $remove_role_plugin->execute($this->account); } @@ -46,7 +46,7 @@ public function testExecuteAddNonExistingRole() { ->will($this->returnValue(FALSE)); $config = array('rid' => 'test_role_1'); - $remove_role_plugin = new AddRoleUser($config, 'user_remove_role_action', array('type' => 'user'), $this->userRoleEntityType); + $remove_role_plugin = new AddRoleUser($config, 'user_remove_role_action', array('type' => 'user'), $this->userRoleEntityType, $this->roleStorage); $remove_role_plugin->execute($this->account); } diff --git a/core/modules/user/tests/src/Unit/Plugin/Action/RemoveRoleUserTest.php b/core/modules/user/tests/src/Unit/Plugin/Action/RemoveRoleUserTest.php index e294398..2d2419d 100644 --- a/core/modules/user/tests/src/Unit/Plugin/Action/RemoveRoleUserTest.php +++ b/core/modules/user/tests/src/Unit/Plugin/Action/RemoveRoleUserTest.php @@ -28,7 +28,7 @@ public function testExecuteRemoveExistingRole() { ->will($this->returnValue(TRUE)); $config = array('rid' => 'test_role_1'); - $remove_role_plugin = new RemoveRoleUser($config, 'user_remove_role_action', array('type' => 'user'), $this->userRoleEntityType); + $remove_role_plugin = new RemoveRoleUser($config, 'user_remove_role_action', array('type' => 'user'), $this->userRoleEntityType, $this->roleStorage); $remove_role_plugin->execute($this->account); } @@ -46,7 +46,7 @@ public function testExecuteRemoveNonExistingRole() { ->will($this->returnValue(FALSE)); $config = array('rid' => 'test_role_1'); - $remove_role_plugin = new RemoveRoleUser($config, 'user_remove_role_action', array('type' => 'user'), $this->userRoleEntityType); + $remove_role_plugin = new RemoveRoleUser($config, 'user_remove_role_action', array('type' => 'user'), $this->userRoleEntityType, $this->roleStorage); $remove_role_plugin->execute($this->account); } diff --git a/core/modules/user/tests/src/Unit/Plugin/Action/RoleUserTestBase.php b/core/modules/user/tests/src/Unit/Plugin/Action/RoleUserTestBase.php index 5f7ff13..104f1b6 100644 --- a/core/modules/user/tests/src/Unit/Plugin/Action/RoleUserTestBase.php +++ b/core/modules/user/tests/src/Unit/Plugin/Action/RoleUserTestBase.php @@ -8,6 +8,7 @@ namespace Drupal\Tests\user\Unit\Plugin\Action; use Drupal\Tests\UnitTestCase; +use Drupal\user\RoleStorageInterface; /** * Provides a base class for user role action tests. @@ -29,6 +30,13 @@ protected $userRoleEntityType; /** + * The role storage. + * + * @var \Drupal\user\RoleStorageInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $roleStorage; + + /** * {@inheritdoc} */ protected function setUp() { @@ -39,6 +47,7 @@ protected function setUp() { ->disableOriginalConstructor() ->getMock(); $this->userRoleEntityType = $this->getMock('Drupal\Core\Entity\EntityTypeInterface'); + $this->roleStorage = $this->getMock(RoleStorageInterface::class); } } diff --git a/core/modules/user/user.module b/core/modules/user/user.module index 3d76074..cc51baf 100644 --- a/core/modules/user/user.module +++ b/core/modules/user/user.module @@ -988,7 +988,9 @@ function user_user_role_insert(RoleInterface $role) { $action = entity_create('action', array( 'id' => $add_id, 'type' => 'user', - 'label' => t('Add the @label role to the selected users', array('@label' => $role->label())), + // No arguments are supplied so that the substitution can done at runtime. + // @see \Drupal\system\Entity\Action::label() + 'label' => t('Add the @role_label role to the selected users'), 'configuration' => array( 'rid' => $role->id(), ), @@ -1001,7 +1003,9 @@ function user_user_role_insert(RoleInterface $role) { $action = entity_create('action', array( 'id' => $remove_id, 'type' => 'user', - 'label' => t('Remove the @label role from the selected users', array('@label' => $role->label())), + // No arguments are supplied so that the substitution can done at runtime. + // @see \Drupal\system\Entity\Action::label() + 'label' => t('Remove the @role_label role from the selected users'), 'configuration' => array( 'rid' => $role->id(), ),