The Drupal\Core\Entity\Query\QueryInterface defines an accessCheck() method, which sets whether an access check should be applied when executing the entity query. The QueryBase defaults this to true.

Most, if not all, content entities in core use the Drupal\Core\Entity\Query\Sql\Query, and this class makes appropriate use of the accessCheck property when crafting the query. Config entities, however, use the Drupal\Core\Config\Entity\Query\Query class and this class doesn't do access checking, regardless of what the property is set to.

One of the adverse side effects is that the default list builder for config entities (EntityListBuilder) will always display every config entity, regardless of whether you actually have access to view it.

Comments

ivanjaros created an issue. See original summary.

dawehner’s picture

I don't really get the point of the issue to be honest. Listing an entity is useful for itself, whether there are operations or not.

ivanjaros’s picture

But if the user should not have access to the entity it should not be visible. Right now to fix this one needs to override the list builder with custom class extending the original one with such entity filtered out.

dawehner’s picture

Well, in that case the entity should not have been returned from the entity query in the first place. Adding access checking there (by leveraging the access query tags) is the right way.

ivanjaros’s picture

Wouldn't that make such entity globally unavailable?
For example if I have menu that should not be editable(ie. the user should not be able to manipulate links in it) it should not mean the menu won't be usable in blocks for rendering, only in administration.

ivanjaros’s picture

The same goes for list of available menus in the menu link form, no access check either.

Berdir’s picture

That's just not how it works. It's a list of all entities that you can see with their operations. Everything that you have access to should be listed, not just if you have operations.

This is not a bug IMHO and I don't think we should change anything.

ivanjaros’s picture

I see what you're getting at... by operation I also meant "view". So if there is nothing you can do with the entity, the entity should not be listed(in my case menus).

Berdir’s picture

Yes, if you don't have view access, we shouldn't list it. For content entities, the definition is that lists do not have to check access because it's the job of query tags/altering to take care of that. Not sure about config entities.

ivanjaros’s picture

For content entities, yes, absolutely. But config entities do have this issue.

kevin.dutra’s picture

Title: EntityListBuilder - entity is visible even if no operations are available » Access control is not applied to config entity queries
Issue summary: View changes

Reworking the IS based on where the discussion has led and my own observations. Please tweak if something isn't clear.

alexpott’s picture

I've discussed this issue at length we @Berdir. Based on those discussions I think that we should document that ConfigEntityListBuilder::load() does not check access and we should document that Drupal\Core\Config\Entity\Query\Query does not do the equivalent access checking that occurs for content entities.

We should also fix overrides of ConfigEntityListBuilder::load() to always call ConfigEntityListBuilder::load() and document that not doing so might be a security issue in the future.

alexpott’s picture

Of course #12 implies that we don't have a compelling reason to make Drupal\Core\Config\Entity\Query\Query check access - As far as I can see such a reason is yet to be documented on this issue.

kevin.dutra’s picture

As an example, the use case that we're working on right now is a module that allows uploading a bundled set of files (e.g. ZIP, TAR) to a destination directory. It's sort of a poor man's SFTP. There are config entities for each physical directory and they define things like whether it goes in the public or private file system, who gets access to upload to that directory, etc.

Users may have access to multiple of these entities so we give them access to a listing page, but since there is currently no access control applied for config entities, they can see every directory. Access control is applied to operations correctly, so there are a few rows they can operate on and then some rows that they can do nothing with, but they can see information they shouldn't be able to.

So as a workaround, we could implement a child of the EntityListBuilder and make sure that access is applied to the list, and that works fine, but now we're left playing a game of whack-a-mole. Anywhere else we may be loading these entities we have to remember to be explicit about that or we may end up with info leaking out somewhere else. (Whereas if this was handled at the lower level, you'd be safe everywhere.)

Also, there's just the fact that it's a little intuitive DX-wise to have something defined on an interface that really only applies to one specific implementation. Not as big a deal, but still makes for some head-scratching. :)

Berdir’s picture

You have to have a EntityListBuilder already and for your specific entity type, you can easily check access there.

Anywhere else we may be loading these entities we have to remember to be explicit about that or we may end up with info leaking out somewhere else

You'd have to do that anyway.

I have no idea what it would take to implement that on the query level for config entities, especially now in a stable release (some entity queries that might expect all of them might suddenly no longer do that, we had issues like that also with content entities in 7.x).

Yes, it's not optimal, but I doubt that doing this in a generic way will be backwards compatible. All we could do is do it by default in the list builder. For something as complex as your case, it would hardly help you much as I guess you'll end up customizing load() anyway (different sorting, maybe hierarchy, ...).

Gábor Hojtsy’s picture

@Berdir told be about this issue, "there are some list builders, like LanguageListBuilder that override the load() method and don't use the override free loading" :/ Agree that looks bad however this looks like a bit broader in scope.

Berdir’s picture

Thinking more about this...

I think what we basically need to document is that the meaning of the view operation depends on the config entity type and can be interpreted by that as it wants. And that it, in many cases, might have no meaning at all.

For example, take block configs. They use the view operation to determine if a block is visible on the page. So if you have a block with a node type visibility condition, it would vanish from the block admin page, for everyone.

Not sure where exactly that should to be documented.

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.

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.