Access control is not applied to config entity queries

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.

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.

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.

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.

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

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.

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.

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. :)

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, ...).

@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.

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.

I just had the requirement to provide access to editors to only one of the menus. This part is easy by registering a new permission and adding it to the routes and implementing some access hooks. However the menu listing is showing all menus even though there is an available operation for only one of them. Unfortunatelly I do not have any other option than overriding the menu list builder right now, so I think that providing the ability to check view access before listing an entity sounds like a reasonably general solution.

This was a bugsmash daily triage target a few days ago, adding tag. It was discussed by lendude and catch, I am just recording the results. This appears to be opened to document [#3330981#24] and catch suggested doing that on accessCheck(). catch also suggested making this a documentation task.

I have updated the Issue Summary and meta data according to that discussion.