#Security improvements

Just to check my understanding of the problem, the problem is that an Entity's permissions could be more restrictive, and modules should be able to loosen those restrictions. Does that sound correct?

Here are two proposals to solve that problem without haivng to know anything about the intent of the query.

Proposal A

Node adds a view any unpblished content permission and Group adds a view ungrouped unpublished content permission.

In this way, the permissions always flow downwards and Group doesn't have to undo anything. It puts a small amount of burdon on site builders because permissions from the entity's module would always take precidence over any other module. This makes sense to me from a security standpoint becausae it seems really odd (and somewhat dangourous?) that you could install a module and all of the sudden the permissions are less restrictive rather than more restrictive.

Proposal B

If we really want to allow modules to loosen permissions (which I don't think is a good idea, but fine), then what if we didn't allow folks to inspect/modify the query at all? What if we created an AccessResult that added a ConditionInterface? It seems like there isn't actually a need to inspect or mutate the query, just a need to remove access results? Does that sound right?

For instance, it could look something like this:

public function queryAccess(?AccountInterface $account = NULL): AccessQueryResultInterface

and the hook:

function hook_entity_query_access(string $entity_type_id, ?AccountInterface $account = NULL): AccessQueryResultInterface

and the AccessQueryResultInterface would be something like this:

interface AccessQueryResultInterface extends AccessResultInterface {
  public function condition(): Condition;
}

and you'd create one like this:

   AccessQueryResult::allowedWithCondotion($condition);

Then we could just merge all the access results together, or we could provide a hook to alter them (or the list of them).

Would either of those solve the problem?

catch → closed merge request !10482

Committed/pushed to 11.x, thanks!

I think there's enough changes in here that we should probably keep it just in 11.2.

Thanks for reporting. Fixed.

@ahmad-abbad Many thanks! The patch in #80 📌 Review/update $adminTags variable for new html elements to be whitelisted Needs work works for me (d10.4.4).

Also, the default conjunction on entity queries is AND, which screws Group over if modules start to implement entity query access without a way to show intent. Right now, a View that filters by status has the following query after it's altered by Group:

status=1
AND
(node_is_not_grouped OR (node_is_grouped AND group_grants_node_access))

So if the node isn't grouped, you will see it as long as it is published. But if it is grouped and you do not have Group access, it will not show up in the list. Which is what Group promises: To protect your private content.

Now imagine if the above query came from the fact that entities finally have generic query access and the status check was an access check. Now we run into the intent problem from above. If we were able to determine intent, we could change the query to:

(node_is_not_grouped AND status=1)
 OR
(node_is_grouped AND group_grants_node_access))

But without intent, this would be impossible. Which is exactly why Group doesn't play nice with node grants right now.

I'm trying to understand why you would need to undo a condition from another module? Wouldn't that imply that you're trying to override an access restriction put in place by another module?

Group is a perfect example of this.

Say Node adds a condition for "status=1" to the query because the current user does not have any permission to view unpublished nodes. However, in one of the current user's groups they do have access to view any unpublished node. If Group were not able to undo Node's access check, how would it be able to also have unpublished nodes from the user's group show up in the results?

It doesn't seem like it's a problem that two modules could add conditions on the same field? It seems like that would only compile to a single JOIN?

I'm talking about manual joins. Access modules tend to want to join their "records" table to cross-reference.

I'm really trying to understand how this isn't exactly what I'm proposing.

There are many issues I've encountered in the past with a similar approach, but I'll give you a big one: Your suggestion lacks the expression of intent.

Assume the following query condition: bypass=1, which comes from a module that allows you to bypass any entity access if you have a particular permission.

Now the query also specifies status=1, but we don't know where this was from. Was it from a View that wishes to filter the results and so the query result should definitely not contain any unpublished entities even if you have access to them. Or was it from an access check because the current user does not have access to unpublished entities?

If we don't know intent, we don't know whether we should use status=1 AND bypass=1 (Views filter) or status=1 OR bypass=1 (access check).

See how this is a problem if all we have to go on is the query? And why we tried to go with a value object based solution before (because then you have intent)?

Good luck digging through all of the joins and conditions finding the thing you need to undo and adjust.

I'm trying to understand why you would need to undo a condition from another module? Wouldn't that imply that you're trying to override an access restriction put in place by another module? If you're trying to add a restriction, simply adding the condition seems totally fine from what I can tell.

we need to make some, we might end up joining the same table 5 times because everyone and their dog wanted to get some info from it.

We already dedupe joins in Tables::addField. Indeed the code has this comment:

    // This variable ensures grouping works correctly. For example, given the
    // following conditions:
    // ->condition('tags', 2, '>')
    // ->condition('tags', 20, '<')
    // ->condition('node_reference.nid.entity.tags', 2)
    // The first two should use the same table but the last one needs to be a
    // new table. So for the first two, the table array index will be 'tags'
    // while the third will be 'node_reference.nid.tags'.

It doesn't seem like it's a problem that two modules could add conditions on the same field? It seems like that would only compile to a single JOIN?

Which is why I'd rather have a system where we can first allow modules to specify simple conditions, which get compiled in the end, and then figure out a way for more complex alters to step in before it's too late.

I'm really trying to understand how this isn't exactly what I'm proposing.

The only problem with this that I can see is that it doesn't cover Views which feels like a completely separate beast

You can convert Views just fine, I do so in Group already. So I don't really see a problem there.

Why would you need to do that? The AccessResults should get merged from the entity access handler as well as any hook implementations. Likewise the query gets modified by the hook implementations regardless of final AccessResult.

Say we allow this to live in a handler and a module has a query alter that looks up a particular flag. Now another modules comes in and wants to make it so you need both the original and a custom flag. They now need to extend said handler to make that happen, but by doing so lock out all other contrib modules from making similar changes to the handler.

You cannot easily undo what was done in a queryAccess() because the changes were made directly on the QueryInterface. Good luck digging through all of the joins and conditions finding the thing you need to undo and adjust.

This is why the original proposal in Entity API gathered value objects that were all processed at the very end. So that you could prevent the spaghettification of the query by the time you get your chance at changing it.

Similarly, if we allow anyone to alter the query anywhere we cannot know what joins were made by other handlers. So if we need to make some, we might end up joining the same table 5 times because everyone and their dog wanted to get some info from it.

It will lead to some really bad queries like this. Which is why I'd rather have a system where we can first allow modules to specify simple conditions, which get compiled in the end, and then figure out a way for more complex alters to step in before it's too late. This will not be easy, but we should keep our options open from the get go. If we start simple, ship it and then realize we cannot serve complicated, we're going to get a lot of angry contrib maintainers. Myself included.

I'm having trouble understanding how/why the proposed hook(s) wouldn't work for you?

See above. But I'll try to read through your suggestions again to see if I didn't miss anything. I'm about to sign off for the day, though, so may need to wait a bit.

In #245 I proposed a hook_entity_query_access which could look something like this:

function hook_entity_query_access(\Drupal\Core\Entity\Query\QueryInterface\QueryInterface $query, ?AccountInterface $account = NULL): AccessResultInterface

I'm trying to understand how that wouldn't suit your needs? The only problem with this that I can see is that it doesn't cover Views which feels like a completely separate beast, but perhaps a similar approach could be taken there.

If you put it on an entity storage handler, you imply that it only serves to define access over that entity type's queries. This is not always the case and thus it completely rules out anyone trying to provide entity query access for multiple entity types. You should not put user query logic into a NodeAccessControlHandler, for instance.

And what about any module trying to leverage entity query access across multiple entity types?

It allows you to hook into all EntityQueries (for both content and config) that have QueryInterface::accessCheck set to TRUE.

As we know, entity handlers are a nightmare to extend because only one module can swap out the original (something I also fixed in Group). This is exactly why I abandoned the approach in Entity API in favor of a custom one in Group.

Why would you need to do that? The AccessResults should get merged from the entity access handler as well as any hook implementations. Likewise the query gets modified by the hook implementations regardless of final AccessResult.

We need a system that can take instructions from various places, one of them could be an entity type handler for the default behavior, and compile those into an efficient query.

That is what the hook does? I think there is a separate issue that EntityQuery should better support subqueries, but that's already a problem with node grants.

This again assumes you are dealing with the entity type table.

I guess that is a broader question if we should apply access controls to any SELECT query or only to EntityQueries and Views?

Either way heavy -1 on relying solely on entity handlers to deal with this problem space. They are inflexible and almost non-extensible. You'd make access modules dead in the water from the get go.

I'm having trouble understanding how/why the proposed hook(s) wouldn't work for you?

updated the CR to be a bit easier.

Can you help me understand how the interface I proposed in #245 does not allow you to modify the query more thoroughly?

If you put it on an entity storage handler, you imply that it only serves to define access over that entity type's queries. This is not always the case and thus it completely rules out anyone trying to provide entity query access for multiple entity types. You should not put user query logic into a NodeAccessControlHandler, for instance.

And what about any module trying to leverage entity query access across multiple entity types? As we know, entity handlers are a nightmare to extend because only one module can swap out the original (something I also fixed in Group). This is exactly why I abandoned the approach in Entity API in favor of a custom one in Group.

We need a system that can take instructions from various places, one of them could be an entity type handler for the default behavior, and compile those into an efficient query.

If you take a look at EntityOwnerTrait it exclusively relies on the owner key
This again assumes you are dealing with the entity type table. IIRC core does(/did?) some dirty juggling, looking for a "nid" column because it knows node access doesn't always get added to the node table directly. It could have been Book that did that, but I can't immediately recall.

Either way heavy -1 on relying solely on entity handlers to deal with this problem space. They are inflexible and almost non-extensible. You'd make access modules dead in the water from the get go.

So rather than try to pigeon hole all access logic into conditions, we need a system where you can choose to use simple conditions but at the same time have the freedom to alter the query more thoroughly if necessary.

Can you help me understand how the interface I proposed in #245 does not allow you to modify the query more thoroughly? I would imagine you would also need that in order to handle core's view own unpublished content?

We could perhaps come up with a few custom value objects such as OwnerCheck, but again it needs instructions on where to find that owner. is it on the same table and, if so, under what column? Or is it in a different table and how do we join that one?

If you take a look at EntityOwnerTrait it exclusively relies on the owner key on the entity. You should be able to modify the Entity Query with that key, which is how I would handle view own unpublished content. The table that it is on doesn't matter because EntityQuery deals with entity fields rather than database columns.

I think this task needs to be broken down, first into a "low-level" API that can handle the existing permissions in core that we are currently completely ignoring, and then maybe a higher level API that can replace node grants.

As I stated before, there is a reason I abandoned this approach in Group. We cannot possible translate all access logic into simple conditions, this is also the case for the node grants system.

So rather than try to pigeon hole all access logic into conditions, we need a system where you can choose to use simple conditions but at the same time have the freedom to alter the query more thoroughly if necessary.

We could perhaps come up with a few custom value objects such as OwnerCheck, but again it needs instructions on where to find that owner. is it on the same table and, if so, under what column? Or is it in a different table and how do we join that one?

So, even though this has been on my todo list for a long time and I really wish I'd have more time to work on it, it requires more thought.

We need a system that:

1. Works for simple cases with good DX
2. Is flexible enough for complex cases without breaking item 1
3. Supports any entity type and entity operation and can therefore replace node grants completely

We need to both kill node grants with a flamethrower and come up with something that people can easily switch to. Given a proper budget I could spearhead this effort with the experience I've gained from writing Group's query alters but, even though it's a critical part of core we need to urgently fix, I'm not sure anyone is willing to free up said budget.

I'm already grateful we did get funding for the Access Policy API in 2023, allowing it to get into core in 2024.

Would you prefer to deprecate it instead of removing it immediately?

Still seems to be solving a problem vs just removing code so probably would still vote for a test to show whats needed

#22 should be considered too

It doesn't make sense to me that this issue was filed as a bug report. A quick search for other "dead code" issues in the Core queue shows that they're mostly/all Tasks.

I considered the suggested test:

Possibly add test for D8 to insure that the password is the hashed value after save() if no such test exists.

I even wrote one out, but in the end this is no different from a Functional test where the login form is submitted. That's covered by UserRegistrationTest. If I'm wrong, then tell me. Needing a test for this doesn't make sense to me. The other dead code issues I checked don't require a test for it either. So I'm removing the Needs Tests tag (along with other superfluous tags).

Aside from that, I also wondered why PHPStan didn't pick up on this. This line contains an undeclared class property assignment. I figured that should be setting off a standards error. But it doesn't because Core only lints with PHPStan level 1. Those warnings are activated at level 2. You can cause an error to occur manually with the command vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist -l 2 core/modules/user/src/RegisterForm.php.

More specifically, AccessResult implements RefinableCacheableDependencyInterface so where we handle the query execution we could collect the cacheable metadata from the access result.

Thankyou, now I understand. The proposed queryAccess() handler method allows both modifying the passed in query, and returning an access result with cacheability metadata. Clever, although not a pattern I remember seeing in core before.

The IS should explain the consequences of this bug more clearly. For instance, top-level constraints are not validated with this method:

$violations = ConfigEntityAdapter::createFromEntity($config_entity)->validate();

Also, the CR needs some editing for style. It should explain what is new, and how it has changed. The chatty style isn't appropriate.

I don't understand how your handler proposal helps it.

More specifically, AccessResult implements RefinableCacheableDependencyInterface so where we handle the query execution we could collect the cacheable metadata from the access result.

In principle having this on the handler seems good. But I see it as a small addition to the current system, not a simplification

Perhaps I don't understand the problem being described in the issue summary? From what I understand it is mostly to resolve this note:

Note that this hook is not called for listings (e.g., from entity queries and Views).

and provide a hook that is called for listings, is it not?

I'm interested in the cacheability question you raise, but I don't understand how your handler proposal helps it. Can you say more?

For instance, if you have a query like this:

$query = \Drupal::entityQuery('node')
  ->accessCheck(TRUE);

You would expect to at least have the user.permissions cache context. But what if the user does not have bypass node access OR administer nodes but does have view own unpublished content? In that case, you should get the user cache context, even if no results are returned.

As far as I can tell, we don't properly handle this case with the recommended approach of hook_query_TAG_alter. Then again, in core, we completely ignore the the QueryInterface::accessCheck and return everything anyways! Which is not only a huge security flaw, that at least to this engineer did not expect, but is also a fairly bad performance problem. We are effectively querying for entities that the user has no ability to access in the first place.

In principle having this on the handler seems good. But I see it as a small addition to the current system, not a simplification. And something that could be a separate issue.

I'm interested in the cacheability question you raise, but I don't understand how your handler proposal helps it. Can you say more?

Here's my proposal to add to EntityAccessControlHandlerInterface

use Drupal\Core\Entity\Query\QueryInterface;

/**
 * Checks access to query an entity.
 */
public function queryAccess(QueryInterface $query, ?AccountInterface $account = NULL): AccessResultInterface

I think this should be simple enough to handle simple permissions like view media and access user profiles, but flexible enough to handle view own unpublished content which would require modifying the $query. This also allows appropriate cacheability metadata to be added and would only be executed if QueryInterface::accessCheck is TRUE

It does not expand node grants outside of nodes, but it should allow us to simplify the query entity access system? We can add hooks similar to hook_entity_access, maybe hook_entity_query_access ? Then modules can take further action.

What do you all think?

I was doing some research ✨ Add a getter for Entity\Query\QueryBase::$accessCheck Active on where we use hook_query_TAG_alter in core for ✨ Add a getter for Entity\Query\QueryBase::$accessCheck Active and I was surprised that it's only really used in NodeHooks1::queryNodeAccessAlter, which is probably why this issue is so long because that method is a beast and I think we need to find a better solution for all of the other entities first and then it might be more straightforward how to solve this problem.

For instance, if you run an entity query with accessCheck for a list of users, I would expect that query to return no results unless the requesting user has access user profiles (or one of the administrative privlidges). But right now, from what I can tell, it returns all of the results!

The same can be said for media and the view media permission. There are probably many more examples.

I'm starting to think we should indeed add a method to EntityAccessControlHandlerInterface that forces/allows for altering the EntityQuery and the logic is similar to EntityAccessControlHandlerInterface::access. Might also be nice to add a method to AlterableInterface that forces the query to immediatly return zero results. Lastly, we need some way to handle cacheability metadata that doens't weirdly rely on the rendererer.

All of that feels like a lot and has nothing to do with node grants, but, imho, is needed for a more comprehensive solution. Like if you don't have access to view media do more granualar permissions beyond that even matter? We aren't even handling the permissions we have right now let alone more granualar ones.

I think it would be hard to detect which routes need to be protected against CSRF.
POST requests can also be vulnerable (but only when cookies use SameSite=None which is not recommended).
And modifying config or content are not the only actions that need to be protected, for example a route that triggers an external API call could also be vulnerable.

This does feel a bit heavy handed because the vast majority of GET requests do not require this as far as I can tell, so we're making a lot of busy work.

Can we detect writes to config or content entities via events or hooks, and if they were triggered by an HTTP GET request, ensure that it was protected via CSRF? If there is a case where this can happen where CSRF is not required, maybe we can just ensure the route has the CSRF check flag specified, even if it's set to false?

I added a runtime deprecation and a test.
But now of course a lot of tests trigger the deprecation.

Honestly if we want to be stricter about it, we could add a method to EntityAccessControlHandlerInterface that forces entities to alter the entity query (or pass). Which makes it clearer when implementing a custom entity that you need to handle access controlled queries.

You can kind of do this now with hook_entity_query_ENTITY_TYPE_alter. The problem is that you cannot access the accessCheck property without using reflection → .

A very simple implementation could be to either provide access to that property, or by adding an `alterTag` for `access`.

I think that mostly solves the problem described in the issue? I imagine it could be a more comprehensive API, but it's a good first step?