From 3f58fa24004795148d0f01fd25fb05bdf576b212 Mon Sep 17 00:00:00 2001 From: damontgomery Date: Wed, 22 Jul 2020 13:11:22 -0400 Subject: [PATCH 1/7] Issue #2982550 by andy_w, damontgomery: Allow specific schemes to be applied to certain roles --- config/install/workbench_access.settings.yml | 1 + config/schema/workbench_access.schema.yml | 3 ++ src/Form/WorkbenchAccessConfigForm.php | 7 ++++ tests/src/Functional/UpdatePathTest.php | 1 + workbench_access.install | 14 +++++++ workbench_access.module | 42 +++++++++++++++++--- 6 files changed, 62 insertions(+), 6 deletions(-) diff --git a/config/install/workbench_access.settings.yml b/config/install/workbench_access.settings.yml index a1b2f718..4d43d4ab 100644 --- a/config/install/workbench_access.settings.yml +++ b/config/install/workbench_access.settings.yml @@ -1 +1,2 @@ deny_on_empty: false +deny_strict: false diff --git a/config/schema/workbench_access.schema.yml b/config/schema/workbench_access.schema.yml index bb79dd65..f2372968 100644 --- a/config/schema/workbench_access.schema.yml +++ b/config/schema/workbench_access.schema.yml @@ -5,6 +5,9 @@ workbench_access.settings: deny_on_empty: type: boolean label: 'Deny access to unassigned content' + deny_strict: + type: boolean + label: 'When multiple schemes apply, deny access if any scheme does not provide access' entity_reference_selection.workbench_access: diff --git a/src/Form/WorkbenchAccessConfigForm.php b/src/Form/WorkbenchAccessConfigForm.php index d4117cca..b9759e59 100644 --- a/src/Form/WorkbenchAccessConfigForm.php +++ b/src/Form/WorkbenchAccessConfigForm.php @@ -35,6 +35,12 @@ public function buildForm(array $form, FormStateInterface $form_state) { '#default_value' => $config->get('deny_on_empty'), '#description' => $this->t('For content under access control, deny access for any content not assigned to a section. This setting is off by default so that installing the module does not break existing site behavior.'), ]; + $form['deny_strict'] = [ + '#type' => 'checkbox', + '#title' => $this->t('Deny access if any scheme does not provide access'), + '#default_value' => $config->get('deny_strict'), + '#description' => $this->t('When multiple schemes apply, deny access if any scheme does not provide access. Without this checked, access is only denied if all schemes do not provide access.'), + ]; return parent::buildForm($form, $form_state); } @@ -45,6 +51,7 @@ public function buildForm(array $form, FormStateInterface $form_state) { public function submitForm(array &$form, FormStateInterface $form_state) { $config = $this->config('workbench_access.settings'); $config->set('deny_on_empty', $form_state->getValue('deny_on_empty'))->save(); + $config->set('deny_strict', $form_state->getValue('deny_strict'))->save(); parent::submitForm($form, $form_state); } diff --git a/tests/src/Functional/UpdatePathTest.php b/tests/src/Functional/UpdatePathTest.php index 6387e6d5..75fa4ac0 100644 --- a/tests/src/Functional/UpdatePathTest.php +++ b/tests/src/Functional/UpdatePathTest.php @@ -49,6 +49,7 @@ protected function setDatabaseDumpFiles() { public function testUpdatePath() { $expected_new_config = [ 'deny_on_empty' => \Drupal::config('workbench_access.settings')->get('deny_on_empty'), + 'deny_strict' => TRUE, '_core' => \Drupal::config('workbench_access.settings')->get('_core'), ]; $block = Block::load('workbench_access_block'); diff --git a/workbench_access.install b/workbench_access.install index bac81f63..c920f134 100644 --- a/workbench_access.install +++ b/workbench_access.install @@ -54,3 +54,17 @@ function workbench_access_update_8004() { $type = $entity_manager->getDefinition('section_association'); $update_manager->installEntityType($type); } + +/** + * Update general configuration with new multi-scheme variable. + */ +function workbench_access_update_8005() { + /* @var Drupal\Core\Config\ConfigFactoryInterface $config_factory */ + $config_factory = \Drupal::service('config.factory'); + + $config = $config_factory->getEditable('workbench_access.settings'); + // This is not the new default value, but we don't want to change behavior + // for existing sites. + $config->set('deny_strict', TRUE); + $config->save(); +} diff --git a/workbench_access.module b/workbench_access.module index c0762ee5..2904e06e 100644 --- a/workbench_access.module +++ b/workbench_access.module @@ -53,17 +53,47 @@ function workbench_access_form_alter(&$form, FormStateInterface $form_state) { * Implements hook_entity_access(). */ function workbench_access_entity_access(EntityInterface $entity, $op, AccountInterface $account) { - // Return net result of all enabled access schemes. If one scheme allows - // access, then it is granted. // We don't care about the View operation right now. if ($op === 'view' || $op === 'view label' || $account->hasPermission('bypass workbench access')) { // Return early. return AccessResult::neutral(); } - return array_reduce(\Drupal::entityTypeManager()->getStorage('access_scheme')->loadMultiple(), function (AccessResult $carry, AccessSchemeInterface $scheme) use ($entity, $op, $account) { - $carry->addCacheableDependency($scheme)->cachePerPermissions()->addCacheableDependency($entity); - return $carry->orIf($scheme->getAccessScheme()->checkEntityAccess($scheme, $entity, $op, $account)); - }, AccessResult::neutral()); + + // Return net result of all enabled access schemes. + $aggregate_access_result = AccessResult::neutral(); + + $schemes = \Drupal::entityTypeManager()->getStorage('access_scheme')->loadMultiple(); + + if (count($schemes) > 0) { + /* @var Drupal\Core\Config\ConfigFactoryInterface $config_factory */ + $config_factory = \Drupal::service('config.factory'); + $config = $config_factory->get('workbench_access.settings'); + + if ($config->get('deny_strict') === TRUE) { + foreach ($schemes as $scheme) { + if ($scheme->getAccessScheme()->checkEntityAccess($scheme, $entity, $op, $account)->isForbidden()) { + $aggregate_access_result = AccessResult::forbidden(); + } + } + } + else { + $aggregate_access_result = AccessResult::forbidden(); + + foreach ($schemes as $scheme) { + if ($scheme->getAccessScheme()->checkEntityAccess($scheme, $entity, $op, $account)->isNeutral()) { + $aggregate_access_result = AccessResult::neutral(); + } + } + } + } + + $aggregate_access_result->addCacheableDependency($entity); + $aggregate_access_result->cachePerPermissions(); + foreach ($schemes as $scheme) { + $aggregate_access_result->addCacheableDependency($scheme); + } + + return $aggregate_access_result; } /** From af5a6d16d74f86188f2b5592ce8b3c0ee0c083a3 Mon Sep 17 00:00:00 2001 From: agentrickard Date: Wed, 22 Jul 2020 14:14:33 -0400 Subject: [PATCH 2/7] Working through tests. --- .../Functional/MultipleSchemeAccessTest.php | 179 ++++++++++++++++++ workbench_access.module | 45 +++-- 2 files changed, 212 insertions(+), 12 deletions(-) create mode 100644 tests/src/Functional/MultipleSchemeAccessTest.php diff --git a/tests/src/Functional/MultipleSchemeAccessTest.php b/tests/src/Functional/MultipleSchemeAccessTest.php new file mode 100644 index 00000000..8a37e8f8 --- /dev/null +++ b/tests/src/Functional/MultipleSchemeAccessTest.php @@ -0,0 +1,179 @@ + 'page', + 'third_party_settings' => [ + 'menu_ui' => [ + 'available_menus' => [ + 'main', + 'account' + ] + ] + ] + ]; + + $node_type = $this->createContentType($node_type_values); + $menu_scheme = $this->setUpMenuScheme(['page'], ['main', 'account'], 'menu_scheme'); + + $vocab = $this->setUpVocabulary(); + $field = $this->setUpTaxonomyFieldForEntityType('node', $node_type->id(), $vocab->id()); + $this->assertEqual($field->getDefaultValueLiteral(), []); + $taxonomy_scheme = $this->setUpTaxonomyScheme($node_type, $vocab); + $user_storage = \Drupal::service('workbench_access.user_section_storage'); + $role_storage = \Drupal::service('workbench_access.role_section_storage'); + + // Set up an editor and log in as them. + $editor = $this->setUpEditorUser(); + $this->drupalLogin($editor); + + // Set up some roles and terms for this test. + // Create terms and roles. + $staff_term = Term::create([ + 'vid' => $vocab->id(), + 'name' => 'Staff', + ]); + $staff_term->save(); + $super_staff_term = Term::create([ + 'vid' => $vocab->id(), + 'name' => 'Super staff', + ]); + $super_staff_term->save(); + $base_term = Term::create([ + 'vid' => $vocab->id(), + 'name' => 'Editor', + ]); + $base_term->save(); + + // Set up some roles and menu links for this test. + $staff_link = MenuLinkContent::create([ + 'title' => 'Link 1', + 'link' => [['uri' => 'route:']], + 'menu_name' => 'main', + ]); + $staff_link->save(); + $super_staff_link = MenuLinkContent::create([ + 'title' => 'Link 2', + 'link' => [['uri' => 'route:']], + 'menu_name' => 'main', + ]); + $super_staff_link->save(); + $base_link = MenuLinkContent::create([ + 'title' => 'Link 3', + 'link' => [['uri' => 'route:']], + 'menu_name' => 'main', + ]); + $base_link->save(); + + // Add the user to the base section. + $user_storage->addUser($taxonomy_scheme, $editor, [$staff_term->id()]); + $expected = [$editor->id()]; + $existing_users = $user_storage->getEditors($taxonomy_scheme, $staff_term->id()); + $this->assertEquals($expected, array_keys($existing_users)); + + $this->drupalGet('node/add/page'); + $this->assertResponse(200); + + $config = $this->config('workbench_access.settings'); + $config->set('deny_strict', TRUE)->save(); + + $this->drupalGet('node/add/page'); + $this->assertResponse(403); + + // Create a node and try to edit it. + $node_values = [ + 'type' => 'page', + 'title' => 'foo', + WorkbenchAccessManagerInterface::FIELD_NAME => [$super_staff_term->id(), $staff_term->id()], + ]; + $node = $this->createNode($node_values); + + $this->drupalGet('node/' . $node->id() . '/edit'); + $this->assertResponse(403); + + $config = $this->config('workbench_access.settings'); + $config->set('deny_strict', FALSE)->save(); + + $this->drupalGet('node/' . $node->id() . '/edit'); + $this->assertResponse(200); + + // Create a node and try to edit it. + $node_values = [ + 'type' => 'page', + 'title' => 'foo', + WorkbenchAccessManagerInterface::FIELD_NAME => [$super_staff_term->id(), $staff_term->id()], + ]; + $node2 = $this->createNode($node_values); + _menu_ui_node_save($node2, [ + 'title' => 'bar', + 'menu_name' => 'main', + 'description' => 'view bar', + 'parent' => $base_link->getPluginId(), + ]); + + $this->drupalGet('node/' . $node2->id() . '/edit'); + $this->assertResponse(200); + + $config = $this->config('workbench_access.settings'); + $config->set('deny_strict', TRUE)->save(); + + $this->drupalGet('node/' . $node2->id() . '/edit'); + $this->assertResponse(403); + + // Add the user to the base menu section. + $user_storage->addUser($menu_scheme, $editor, [$base_link->getPluginId()]); + + $this->drupalGet('node/' . $node2->id() . '/edit'); + $this->assertResponse(200); + + } + +} diff --git a/workbench_access.module b/workbench_access.module index 2904e06e..c6989589 100644 --- a/workbench_access.module +++ b/workbench_access.module @@ -78,7 +78,6 @@ function workbench_access_entity_access(EntityInterface $entity, $op, AccountInt } else { $aggregate_access_result = AccessResult::forbidden(); - foreach ($schemes as $scheme) { if ($scheme->getAccessScheme()->checkEntityAccess($scheme, $entity, $op, $account)->isNeutral()) { $aggregate_access_result = AccessResult::neutral(); @@ -88,6 +87,7 @@ function workbench_access_entity_access(EntityInterface $entity, $op, AccountInt } $aggregate_access_result->addCacheableDependency($entity); + $aggregate_access_result->addCacheableDependency($account); $aggregate_access_result->cachePerPermissions(); foreach ($schemes as $scheme) { $aggregate_access_result->addCacheableDependency($scheme); @@ -140,19 +140,40 @@ function workbench_access_entity_create_access(AccountInterface $account, $conte // Check that the user is able to assign content to a section. $user_section_storage = \Drupal::service('workbench_access.user_section_storage'); - $forbidden = AccessResult::forbidden(); - $invalid_schemes = array_reduce($schemes, function ($carry, AccessSchemeInterface $scheme) use ($user_section_storage, $account, $forbidden) { - $sections = $user_section_storage->getUserSections($scheme, $account); - if (!$sections) { - $carry[] = $scheme->label(); + /* @var Drupal\Core\Config\ConfigFactoryInterface $config_factory */ + $config_factory = \Drupal::service('config.factory'); + $config = $config_factory->get('workbench_access.settings'); + + // Return net result of all enabled access schemes. + if ($config->get('deny_strict') === TRUE) { + // Allow but deny if any say no. + $aggregate_access_result = AccessResult::neutral(); + foreach ($schemes as $scheme) { + $sections = $user_section_storage->getUserSections($scheme, $account); + if (!$sections) { + $aggregate_access_result = AccessResult::forbidden(); + } + } + } + else { + // Deny but allow if any say yes. + $aggregate_access_result = AccessResult::forbidden(); + foreach ($schemes as $scheme) { + $sections = $user_section_storage->getUserSections($scheme, $account); + if ($sections) { + $aggregate_access_result = AccessResult::neutral(); + } } - $forbidden->addCacheableDependency($scheme); - return $carry; - }, []); - if ($invalid_schemes) { - return $forbidden->setReason(sprintf('User has no active sections for the following access scheme(s): %s', implode(', ', $invalid_schemes))); } - return $return; + + $aggregate_access_result->addCacheableDependency($entity_bundle); + $aggregate_access_result->addCacheableDependency($account); + $aggregate_access_result->cachePerPermissions(); + foreach ($schemes as $scheme) { + $aggregate_access_result->addCacheableDependency($scheme); + } + + return $aggregate_access_result; } /** From 2bc768c8802a0e21a0ca398922d2d82892351ae6 Mon Sep 17 00:00:00 2001 From: agentrickard Date: Thu, 23 Jul 2020 10:20:39 -0400 Subject: [PATCH 3/7] Fix for failing test. --- tests/src/Functional/MultipleSchemeAccessTest.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/src/Functional/MultipleSchemeAccessTest.php b/tests/src/Functional/MultipleSchemeAccessTest.php index 8a37e8f8..4df87da4 100644 --- a/tests/src/Functional/MultipleSchemeAccessTest.php +++ b/tests/src/Functional/MultipleSchemeAccessTest.php @@ -132,9 +132,15 @@ public function testNodeForm() { $node_values = [ 'type' => 'page', 'title' => 'foo', - WorkbenchAccessManagerInterface::FIELD_NAME => [$super_staff_term->id(), $staff_term->id()], + WorkbenchAccessManagerInterface::FIELD_NAME => $staff_term->id(), ]; $node = $this->createNode($node_values); + _menu_ui_node_save($node, [ + 'title' => 'baz', + 'menu_name' => 'main', + 'description' => 'view baz', + 'parent' => $staff_link->getPluginId(), + ]); $this->drupalGet('node/' . $node->id() . '/edit'); $this->assertResponse(403); @@ -149,7 +155,7 @@ public function testNodeForm() { $node_values = [ 'type' => 'page', 'title' => 'foo', - WorkbenchAccessManagerInterface::FIELD_NAME => [$super_staff_term->id(), $staff_term->id()], + WorkbenchAccessManagerInterface::FIELD_NAME => $staff_term->id(), ]; $node2 = $this->createNode($node_values); _menu_ui_node_save($node2, [ @@ -170,6 +176,9 @@ public function testNodeForm() { // Add the user to the base menu section. $user_storage->addUser($menu_scheme, $editor, [$base_link->getPluginId()]); + $expected = [$editor->id()]; + $existing_users = $user_storage->getEditors($menu_scheme, $base_link->getPluginId()); + $this->assertEquals($expected, array_keys($existing_users)); $this->drupalGet('node/' . $node2->id() . '/edit'); $this->assertResponse(200); From d7aa649e8e58dcea4e0ef7eb76fd150714cc0d1c Mon Sep 17 00:00:00 2001 From: agentrickard Date: Thu, 23 Jul 2020 10:21:29 -0400 Subject: [PATCH 4/7] Fixes failing test. --- tests/src/Functional/MultipleSchemeAccessTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/src/Functional/MultipleSchemeAccessTest.php b/tests/src/Functional/MultipleSchemeAccessTest.php index 4df87da4..b75e13a3 100644 --- a/tests/src/Functional/MultipleSchemeAccessTest.php +++ b/tests/src/Functional/MultipleSchemeAccessTest.php @@ -69,7 +69,6 @@ public function testNodeForm() { $this->assertEqual($field->getDefaultValueLiteral(), []); $taxonomy_scheme = $this->setUpTaxonomyScheme($node_type, $vocab); $user_storage = \Drupal::service('workbench_access.user_section_storage'); - $role_storage = \Drupal::service('workbench_access.role_section_storage'); // Set up an editor and log in as them. $editor = $this->setUpEditorUser(); From a90927e2b317d6220c7d4c87d0ecad5fe2770bde Mon Sep 17 00:00:00 2001 From: agentrickard Date: Thu, 23 Jul 2020 10:34:16 -0400 Subject: [PATCH 5/7] Fixes creation logic to be either/or in all cases. --- .../Functional/MultipleSchemeAccessTest.php | 3 +- workbench_access.module | 46 ++++++------------- 2 files changed, 17 insertions(+), 32 deletions(-) diff --git a/tests/src/Functional/MultipleSchemeAccessTest.php b/tests/src/Functional/MultipleSchemeAccessTest.php index b75e13a3..b8d30931 100644 --- a/tests/src/Functional/MultipleSchemeAccessTest.php +++ b/tests/src/Functional/MultipleSchemeAccessTest.php @@ -121,11 +121,12 @@ public function testNodeForm() { $this->drupalGet('node/add/page'); $this->assertResponse(200); + // Strict checking does not affect creation. $config = $this->config('workbench_access.settings'); $config->set('deny_strict', TRUE)->save(); $this->drupalGet('node/add/page'); - $this->assertResponse(403); + $this->assertResponse(200); // Create a node and try to edit it. $node_values = [ diff --git a/workbench_access.module b/workbench_access.module index c6989589..ae9ab345 100644 --- a/workbench_access.module +++ b/workbench_access.module @@ -139,41 +139,25 @@ function workbench_access_entity_create_access(AccountInterface $account, $conte } // Check that the user is able to assign content to a section. + // Note that 'strict' access checking for multiple schemes does not apply + // when creating content. $user_section_storage = \Drupal::service('workbench_access.user_section_storage'); - /* @var Drupal\Core\Config\ConfigFactoryInterface $config_factory */ - $config_factory = \Drupal::service('config.factory'); - $config = $config_factory->get('workbench_access.settings'); - - // Return net result of all enabled access schemes. - if ($config->get('deny_strict') === TRUE) { - // Allow but deny if any say no. - $aggregate_access_result = AccessResult::neutral(); - foreach ($schemes as $scheme) { - $sections = $user_section_storage->getUserSections($scheme, $account); - if (!$sections) { - $aggregate_access_result = AccessResult::forbidden(); - } + $forbidden = AccessResult::forbidden(); + $valid_schemes = array_reduce($schemes, function ($carry, AccessSchemeInterface $scheme) use ($user_section_storage, $account, $forbidden, $return) { + $sections = $user_section_storage->getUserSections($scheme, $account); + if ($sections) { + $carry[] = $scheme->label(); + $return->addCacheableDependency($scheme); } - } - else { - // Deny but allow if any say yes. - $aggregate_access_result = AccessResult::forbidden(); - foreach ($schemes as $scheme) { - $sections = $user_section_storage->getUserSections($scheme, $account); - if ($sections) { - $aggregate_access_result = AccessResult::neutral(); - } + else { + $forbidden->addCacheableDependency($scheme); } + return $carry; + }, []); + if (empty($valid_schemes)) { + return $forbidden->setReason(sprintf('User has no active sections for the following access scheme(s): %s', implode(', ', $invalid_schemes))); } - - $aggregate_access_result->addCacheableDependency($entity_bundle); - $aggregate_access_result->addCacheableDependency($account); - $aggregate_access_result->cachePerPermissions(); - foreach ($schemes as $scheme) { - $aggregate_access_result->addCacheableDependency($scheme); - } - - return $aggregate_access_result; + return $return; } /** From 1a087474af77cfaeece56ba76e5f30b1967f5ac7 Mon Sep 17 00:00:00 2001 From: agentrickard Date: Thu, 23 Jul 2020 10:38:47 -0400 Subject: [PATCH 6/7] Tests for the deny on empty setting. --- .../Functional/MultipleSchemeAccessTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/src/Functional/MultipleSchemeAccessTest.php b/tests/src/Functional/MultipleSchemeAccessTest.php index b8d30931..4e70be02 100644 --- a/tests/src/Functional/MultipleSchemeAccessTest.php +++ b/tests/src/Functional/MultipleSchemeAccessTest.php @@ -183,6 +183,24 @@ public function testNodeForm() { $this->drupalGet('node/' . $node2->id() . '/edit'); $this->assertResponse(200); + // In cases where one scheme is empty, access should be allowed unless + // deny on empty is enforced. + // Create a node and try to edit it. + $node_values = [ + 'type' => 'page', + 'title' => 'foo', + WorkbenchAccessManagerInterface::FIELD_NAME => $staff_term->id(), + ]; + $node3 = $this->createNode($node_values); + $this->drupalGet('node/' . $node3->id() . '/edit'); + $this->assertResponse(200); + + $config = $this->config('workbench_access.settings'); + $config->set('deny_on_empty', TRUE)->save(); + + $this->drupalGet('node/' . $node3->id() . '/edit'); + $this->assertResponse(403); + } } From 341f0ae187f7839c5b6caefa42964bd694fa7606 Mon Sep 17 00:00:00 2001 From: agentrickard Date: Thu, 23 Jul 2020 10:57:45 -0400 Subject: [PATCH 7/7] Fix missing variable. --- workbench_access.module | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workbench_access.module b/workbench_access.module index ae9ab345..1b851d6d 100644 --- a/workbench_access.module +++ b/workbench_access.module @@ -155,7 +155,7 @@ function workbench_access_entity_create_access(AccountInterface $account, $conte return $carry; }, []); if (empty($valid_schemes)) { - return $forbidden->setReason(sprintf('User has no active sections for the following access scheme(s): %s', implode(', ', $invalid_schemes))); + return $forbidden->setReason(sprintf('User has no active sections.')); } return $return; }