Problem/Motivation

Some EntityListBuilders do not check whether the user has permission for certain operations (such as enable and disabled) on config entities where the routing for the page that it links does. This can cause links to inaccessible pages.

This issue doesn't seem to present itself in core as the permission for the list page seems to be the same as the permissions for any operation on config entities, but it blocks contrib modules that may change the permissions on a config entity, such as a view.

Proposed resolution

Add the access checks.

Remaining tasks

Decide what, if anything, needs to be done for:

  • \Drupal\taxonomy\VocabularyListBuilder::getDefaultOperations() list (have implemented a best guess) and add
  • \Drupal\field_ui\FieldConfigListBuilder::getDefaultOperations() storage-settings (not sure it needs)
  • \Drupal\image\ImageStyleListBuilder::getDefaultOperations() flush (not sure it needs)
  • \Drupal\responsive_image\ResponsiveImageStyleListBuilder::getDefaultOperations() duplicate (not sure it needs)
  • \Drupal\search\SearchPageListBuilder::getDefaultOperations() default (not sure it needs)

User interface changes

Links that could currently go to access denied pages will no longer be shown.

CommentFileSizeAuthor
#27 2650898-27.patch1.79 KBrobertom
#17 2650898-17.patch1.76 KBvacho
#6 2650898-6-check_operation_access.patch2.58 KBandrewbelcher
#3 2650898-3-check_operation_access.patch3.54 KBandrewbelcher
#2 2650898-check_operation_access.patch1.74 KBandrewbelcher
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewbelcher created an issue. See original summary.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented
Status: Active » Needs work
FileSize
2650898-check_operation_access.patch1.74 KB

This patch fixes the operations I've found. I've not actually checked for others, so not marking as needs review yet.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented
Title: ConfigEntityListBuilder/ViewsListBuilder do not check access for operation links » ListBuilders do not check $entity->access() for operation links
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2650898-3-check_operation_access.patch3.54 KB

So I think there is only one other that needs something done:

\Drupal\taxonomy\VocabularyListBuilder::getDefaultOperations() adds list and add which I would guess are view and create respectively?

The following I couldn't see an _entity_access set on the route, so I think don't need anything doing:

\Drupal\field_ui\FieldConfigListBuilder::getDefaultOperations() adds storage-settings. It also alters edit/delete without checking whether they are set. I have fixed the second part, but not sure what to do with the first.

\Drupal\image\ImageStyleListBuilder::getDefaultOperations() adds flush.

\Drupal\responsive_image\ResponsiveImageStyleListBuilder::getDefaultOperations() adds duplicate.

\Drupal\search\SearchPageListBuilder::getDefaultOperations() adds default.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented
Assigned: andrewbelcher » Unassigned
swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Needs work 
+++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
@@ -182,8 +182,12 @@ public function getDefaultOperations(EntityInterface $entity) {
+    if (isset($operations['edit'])) {
+      $operations['edit']['attributes']['title'] = $this->t('Edit field settings.');
+    }
+    if (isset($operations['delete'])) {
+      $operations['delete']['attributes']['title'] = $this->t('Delete field.');
+    }

This doesn't make sense, they will always be there. There's a complete separate discussion on this at #2274433: Do not allow to alter Locked field via UI, so I'd leave this bit out.

Not sure about the others as there are no dedicated permissions for enable/disable and duplicate in the default access check handler, it falls into the default admin permission. Basically, if you have access to the list, then you're fine. Which doesn't mean we could add more granular checks of course, but currently, this patch simply won't fix anything.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Contributed project blocker
FileSize
2650898-6-check_operation_access.patch2.58 KB

swentel: Yes, you are correct - in core this is not an issue. However, core does actually use entity level access checks for the routes, even though those access checks come back to the administer views permission. As it uses entity level checks for the routes, the operations should also follow those checks.

I encountered this as part of working on Views Tag Access which alters the entity level access checks. I've added the contributed project blocker tag and updated the issue summary to make it clearer.

The reason for the code you quoted is because it assumes $operations['edit'] and $operations['delete'] exist. They are created either inside: 

    if ($entity->access('update') && $entity->hasLinkTemplate("{$entity->getTargetEntityTypeId()}-field-edit-form")) {
      $operations['edit'] = array(
        'title' => $this->t('Edit'),
        'weight' => 10,
        'url' => $entity->urlInfo("{$entity->getTargetEntityTypeId()}-field-edit-form"),
      );
    }
    if ($entity->access('delete') && $entity->hasLinkTemplate("{$entity->getTargetEntityTypeId()}-field-delete-form")) {
      $operations['delete'] = array(
        'title' => $this->t('Delete'),
        'weight' => 100,
        'url' => $entity->urlInfo("{$entity->getTargetEntityTypeId()}-field-delete-form"),
      );
    }

or by the call to parent::getDefaultOperations($entity), specifically \Drupal\Core\Entity\EntityListBuilder::getDefaultOperations():

  protected function getDefaultOperations(EntityInterface $entity) {
    $operations = array();
    if ($entity->access('update') && $entity->hasLinkTemplate('edit-form')) {
      $operations['edit'] = array(
        'title' => $this->t('Edit'),
        'weight' => 10,
        'url' => $entity->urlInfo('edit-form'),
      );
    }
    if ($entity->access('delete') && $entity->hasLinkTemplate('delete-form')) {
      $operations['delete'] = array(
        'title' => $this->t('Delete'),
        'weight' => 100,
        'url' => $entity->urlInfo('delete-form'),
      );
    }

    return $operations;
  }

So it is assuming that they exist even though the code that creates them may well not create them. However, you are right - the issue you linked does, co-incidentally, fix the same issue. I've attached an updated version of the patch without those changes.

I'm very happy to break this up/target specific bits (e.g. \Drupal\views_tag_access\ViewListBuilder) if that is preferable?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andrewbelcher’s picture

andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented

There has been no progress at #2274433: Do not allow to alter Locked field via UI. The change made here to the field list builder isn't disruptive and doesn't change any of the work over there (as it replaces the existing code so a simple re-roll will solve it). This doesn't change or break any core behaviour but does fix a bug which limits contrib. Is there anything stopping this from being considered for committal?

Will re-queue the test for 8.1.x to make sure it still applies and passes tests.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.3.x-dev » 8.4.x-dev
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

See also #2200183: Add ConfigEntityAccessController

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs review » Needs work
Related issues: +#2845021: Operation 'add terms' on vocabulary list should respect the access policy 
+++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
@@ -41,11 +41,13 @@ public function getDefaultOperations(EntityInterface $entity) {
     $operations['add'] = array(

We should expand the check also to the "Add terms" operation. I opened an issue for this in #2845021: Operation 'add terms' on vocabulary list should respect the access policy. I didn't knew about this issue. Closing the other as duplicate.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vacho’s picture

vacho CreditAttribution: vacho at Skilld commented
FileSize
2650898-17.patch1.76 KB

Patch updated for branch 8.7.x-dev

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented 
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListBuilder.php
@@ -33,14 +33,14 @@ public function getDefaultOperations(EntityInterface $entity) {
+      if ($entity->access('enable') && !$entity->status() && $entity->hasLinkTemplate('enable')) {
...
+      elseif ($entity->access('disable') && $entity->status() && $entity->hasLinkTemplate('disable')) {

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -161,7 +161,7 @@ public function buildHeader() {
+    if ($entity->access('duplicate') && $entity->hasLinkTemplate('duplicate-form')) {

it looks strange to check access before checking that link template exists

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

robertom’s picture

robertom CreditAttribution: robertom at bmeme commented
Status: Needs work » Needs review
FileSize
2650898-27.patch1.79 KB

patch updated for branch 10.1.x-dev

it looks strange to check access before checking that link template exists

yeah, but it's the same order used in EntityListBuilder::getDefaultOperations

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This will need a test case to show the issue is fixed.

Did not review code.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

MukhtarM’s picture

MukhtarM CreditAttribution: MukhtarM as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented

$entity->access('permission') has been deprecated since D10 and gives the warning(https://www.drupal.org/node/3201242). 

Relying on entity queries to check access by default is deprecated in  
         drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call      
         \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or   
         FALSE to specify whether access should be checked.

Anyway to check explicit access for the entity in getDefaultOperations?

for eg: 

public function getDefaultOperations(EntityInterface $entity) {
    $exists = isset($this->templates[$entity->getGathercontentTemplateId()]);
    $operations = [];
    if ($exists && $entity->access('update') && $entity->hasLinkTemplate('edit-form')) {
      $operations['edit'] = [
        'title' => $entity->hasMapping() ? $this->t('Edit') : $this->t('Create'),
        'weight' => 10,
        'url' => $entity->toUrl('edit-form'),
      ];
    }
    if ($entity->access('delete') && $entity->hasLinkTemplate('delete-form')) {
      $operations['delete'] = [
        'title' => $this->t('Delete'),
        'weight' => 100,
        'url' => $entity->toUrl('delete-form'),
      ];
    }
    return $operations;
  }

here i think its explicitly checking access for 'update' or 'delete'. So the entityQuery->accessCheck(TRUE) would work? as its for the whole access right?