Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

After running into the described issue with Views and Custom Entities in combination with custom access modules that determine access based on field values and relations at runtime, I wrote https://www.drupal.org/project/views_entity_access_check → as (more or less hacky) workaround to do an additional access check on each views row: $value->_entity->access('view')

This is surely expensive, but okay as opt-in for simple cases. Eventually I'll create a Core issue for Views to discuss adding something like this as advanced settings. With complex hook_entity_access() conditions on non-node entity types, a solution becomes more and more important, I think.

Happy to receive feedback and improvements in the module's issues. And even happier if we get this fixed in core one day, so this won't be needed anymore! :)

I would expect that with Modern Drupal's caching system we could probably take care of 70% of the performance issues of doing what is proposed in #231

We took the PoC patch in #75 and converted into a module since we needed this functionality.

As such, the overall design is still very similar to the core node access system of records and grants extended to custom content entity types. This is what we needed since we need something that will scale to larger number of entities and where we could easily apply the same kind of logic we are using for our custom OG-based node access records and grants.

Contributions are, of course, welcome (especially tests and review)
https://www.drupal.org/project/raft_entity_access →

Curios about the differences between RAFT Entity Access and Flexible Permissions → .

With regard to 🌱 [Meta, Plan] Pitch-Burgh: Policy based access in core Active is what is proposed in this issue still a good idea? I would rather we don't have multiple access and control systems hanging around. Maybe once 🌱 [Meta, Plan] Pitch-Burgh: Policy based access in core Active is in core the resolution here would be to deprecate the existing node access rights system rather than expand it into entities. Or maybe break node access and entity access into a node sub-module. Having multiple systems that do the same thing is generally bad DX.

If anyone is looking for a solution to this today there is also the Access Policy module → which has implemented many of these concepts. If/When these API changes make it into core I will also integrate those changes into the module.

To learn more about it you can read the documentation → or watch a recent episode of the Talking Drupal podcast where they featured it.

Re #235 and #236 as I explained in the other issue, Flexible Permissions and the work I'm currently doing to get it into core has nothing to do with query access. See #3371246-17: [Meta, Plan] Pitch-Burgh: Policy based access in core →

While implementing a custom content type entity, I looked into access checking related to entity query logic and found this issue. I also created this support request 💬 Does entity query access checking perform any access checking for custom content type entity? Active to confirm my understanding of entity query access checking.

In that support request, I've also proposed a few documentation changes and happy to have these documentation discussions over there. I think be helpful to communicate more clearly for custom content type entity developers.

Changing to the DX special tag defined on Issue tags -- special tags → .

Coming back to this after all these years, I no longer feel like the solution we have in Entity API is sufficient for core. I tried it out with Group and ended up having to copy a bunch of it because of Group's complicated structure.

So what I instead propose is a service collector that gathers the right metadata from either an entity query or a views query (like Entity API and Group do now) and then passes that along to the tagged services for alteration. In a second phase, we could introduce a core tagged service that copies some of the work from Entity API so that people can still define entity type query access handlers that return some of the value objects we saw earlier.

• Phase 1: Service collector, allow modules to alter queries more easily
• Phase 2: Port the entity type handler from Entity API as a tagged service
• Phase 3: Try to provide sane defaults for every entity type in said handler

#240 @kristiaanvandeneynde

This lacks one key feature of entity module approach:
IMHO the entity module approach is the right direction in adding a declarative field condition that can be applied to single-entity AND entity-query access. I think it is NOT expressive enough though.

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?

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.

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.

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?

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?

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.

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.

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.

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.

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.

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.

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?

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.

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.

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)?

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.

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?

Thanks @ and @kristiaanvandeneynde for the good discussion in #242 ff!

Twice 5 cents on this...:

@davidwbarratt, would you like to review the API i pioneered some time ago in Entity Unified Access → , how it resonates with your vision? (Feel free to PM me on this.)

@kristiaanvandeneynde, I have a lot of resonance with the limitations you point out, i have similar experiences. But with all due respect, imho you are wrong that an access alter should be able to take the role of the aggregating AccessControlHander. I know you had to implement group access control that way and all respect for implementing it in the current API. But i think what you point out is the result of a lack of a pluggable access control aggregation api like symfony has.

I took some time to think this over and text it in 📌 Add a a modern EntityAccessAggregation API, and kill 3-State AccessResults Active . The above discussion indicates to me that this issue should be postponed on the other.

I've come to the conclusion that the problem that this issue is aiming to solve is way too broad and is leading to a level of complexity that isn't easily achievable.

For that reason, I've opened 🐛 QueryInterface::accessCheck does nothing Active which more clearly defines the problem that I ran into and I think perhaps has a simpler solution.

would you like to review the API i pioneered some time ago in Entity Unified Access, how it resonates with your vision? (Feel free to PM me on this.)

This feels very comprehensive, but way outside of the scope of the problem I'm looking to solve.

This statement from #256 is going to keep me up at night:

which comes from a published check and a module that allows you to bypass any entity access if you have a particular permission.

Effectively we have an access control system that, allows a user to enable a module, with the express intention of performing an access bypass. I think that is fundamentally the wrong way we should be thinking about access controls. I don't think a user should ever be able to loosen access simply by enabling a module. If we need to have more granular permissions in core to prevent that, then so be it, but that seems incredibly dangerous to me.

Very interesting discussion! At Open Social we've run into similar issues as Kristiaan has expressed. Both within the context of our use of Group, but also in other areas (e.g. around finding user entities based on user profiles and complex platform and per-user settings). I'd have to dig into our codebase to find you exact examples but I'm not sure they would further the discussion here.

There's two observations that I'd like to add:

1. IFF we would solve access handling with a ValueObject/Non-Query-Condition system that Kristiaan is proposing (I'm a fan) then we would actually be solving a broader class of problems which is basically "Being able to filter in the database based on business logic" which can be huge for doing performant things like user-segmenting, without having to pre-calculate that constantly.

Which brings me to my second point which might help steer the discussion:

2. I think (apologies if my assumptions about people's mental model are wrong): there's a framing difference between what Kristiaan (and us at Open Social) are trying to achieve and the model mental model that davidwbarret is working with.

Effectively we have an access control system that, allows a user to enable a module, with the express intention of performing an access bypass. I think that is fundamentally the wrong way we should be thinking about access controls.

The use-case that group has (and that I'd argue exists in multiple places) is not that the module performs an access bypass. Instead it introduces a new "context" in which it is the owner of access checks. Using the group module as example.

For any content not within a group, the context is global and all the default permissions apply. However, for any content within a group, the global permissions may or may not apply and there is a different set of rules that should apply. Within the Access Policy API this exact problem can be seen as well and it was solved by adding even two levels of this (the $scope and $identifier variable) in the API.

I think this is what Kristiaan is trying to frame: there is a difference between which is a matter of user intent (I want to find green apples) and which is a matter of context specific rules (anyone can find green apples on the tree outside, but you can only see the apples in a secret apple club if you're a member of that club).

I would disagree that the club system trying to manage access to the content in its clubs would be an access bypass, because the site builder has specifically chosen to be able to operate in this different context.

To be clear, I'm not arguing that permissions don't depend on some sort of context, I'm saying that part of that context is the user's existing permissions.

In other words, a permission does not infer some sort of right. The context window therefore, ought to get narrower rather than broader.

Let's take your example:

I think this is what Kristiaan is trying to frame: there is a difference between filtering which is a matter of user intent (I want to find green apples) and access control which is a matter of context specific rules (anyone can find green apples on the tree outside, but you can only see the apples in a secret apple club if you're a member of that club).

I'm saying that must have permission to view apples (regardless of the variety) in order to view secret green apples. The access system we have right now says that it doesn't actually matter if I have permission to view apples or not, a member of the secret green apples club has a right to view secret green apples. If that's the case, we aren't talking about permissions, we are talking about rights.

For a more concreate example, if you don't have access to view any unpublished node how could you possibly view an unpublished node (that isn't yours) in a group? A simple way to solve the contextual problem therefore is by removing the notion of a right by allowing modules to remove access rather than grant access. In this example, Group could add a view any ungrouped unpublished nodes in which a user could (or could not) do just that, but it still resides within the view any unpublished node context rather than overriding (or ignoring) that context.

If we really want to allow modules to loosen permissions (which I don't think is a good idea, but fine)

But it is a good idea? Even node grants supported that for viewing unpublished nodes.

@kindutch put the concept behind this all very well. Thanks for that!

It all ties into the access policy API's concept of scopes and identifiers. One scope should not be concerned with another. It's perfectly valid to not have node edit access in the global scope, but to have said access in a specific domain or group scope. It would be an absolute ball ache to achieve per-domain editor access if it meant you first had to grant general node edit access for the entire website.

It would be an absolute ball ache to achieve per-domain editor access if it meant you first had to grant general node edit access for the entire website.

Could you expand on this? Why would it be a ball ache and for whom? A lot of systems have access controls like this so I'm confused why it would be such a problem?

I do admit that it shifts the responsability from the developer to the site builder, but on the flip side it gives them a lot more control?

Effectively it's making each module responsible for it's own context and the access controls that goes along with it rather than spreading that context across multiple modules. The only context that gets shared is the permissions themselves based on the access result.

In your case you would need to provide an additional permission to prevent access to ungrouped content. In other words, you're aware of the gorup context, and therefore can (and should) perform access restrictions based on that context alone, without reaching into the context of the underlying entity.

Would it be useful if you could take over permissions from other modules? For instance, what if you could just adjust which roles have a permission and "disable" permissions? In this case you could effectively grant all users the ability to access unpublish content globally, but then you can have your own "ungrouped" permissions?

That would basically alter the way the access result in the entity (in this case, node) is handled, while still giving you full control over how it should be handled?

Okay so I feel like this thread is seriously getting derailed, even if there was no intention to do so.

So I'll answer one more time, but after that I don't think we should still be discussing the design decisions we made in the past regarding how we should allow people to alter other modules' access. Even with the most recent addition to our access layer (Access Policy API) we allowed modules to both add to and subtract from passed in access logic. The goal is not to change that, so why are we discussing it?

Could you expand on this? Why would it be a ball ache and for whom? A lot of systems have access controls like this so I'm confused why it would be such a problem?

If you want a sports editor to only be able to post articles to the sports section of your news website, currently all you need to do is assign them the "editor" role in the sports section. If we take one of your suggestions, we would have to make them an editor on the entire website and then make sure they can't do anything in fashion, politics, economics, etc. which is a lot of extra work.

It would also mean we break with everything Drupal users have grown to expect. We already have an issue floating around somewhere about the massive mindfuck we have when it comes to neutral access results. Both the route access checks, entity access checks and grants system treats a neutral result differently.

Now on topic: When you build an entity query, we know that the conditions are filters because access conditions should go into the access check (= why we have the accessCheck() method). For Views it should be the same thing.

So if we build an API that takes an existing query and assumes that all conditions are filters, we could very well create a system that gathers the access logic and allows for easy alteration. Only after everything is processed do we add it all to the query. This would solve all of the problems I explained above because it would be crystal clear whether a status check is a filter or an access rule.

I'd like to pursue this route when i have a bit more time on my hands.

I'm confused, are you saying a module can undo a EntityAccessControlHandler? I know about hook_entity_access but if the former returns forbidden can the latter override that decision? It looks like AccessResult::orIf would prevent that?

If that is the case, then why would a query be any different at all?

If we take one of your suggestions, we would have to make them an editor on the entire website and then make sure they can't do anything in fashion, politics, economics, etc. which is a lot of extra work.

I'm still confused on how that any extra work than determining the intent of a query or trying to modify things? It seems like it would be like a single hook alter (to disable the existing permission) implementation and one additional permission conditional? Am I missing something really basic?

This would solve all of the problems I explained above because it would be crystal clear whether a status check is a filter or an access rule.

Providing a mechanism (a hook?) to either enable or disable an existing permission in core would fix the same problem without having to express intent at all, which is why I'm talking about the design decisions.

but if the former returns forbidden can the latter override that decision? It looks like AccessResult::orIf would prevent that?

Returning Forbidden is what I meant by saying "subtract from". The default is neutral and then people can explicitly allow or forbid access, how is that not add to or subtract from?

If that is the case, then why would a query be any different at all?

Queries don't have the luxury of running code on them. So if one bad actor messes up the query, everyone else is stuck with that outcome. A processor in between could prevent a lot of that damage.

Providing a mechanism (a hook?) to either enable or disable an existing permission in core would fix the same problem without having to express intent at all, which is why I'm talking about the design decisions.

You are literally describing the Access Policy API. Which is one piece of this puzzle, but query access runs after permission checks.

@davidwbarratt, I'm not sure where the difference in vision is based on exactly but I'll make another attempt to see if I can clarify it. I'm also not sure how familiar you are with the group module and its usages, which could also explain why we're talking past one another.

For the below example I want to sketch the scenario where we have a Drupal (global) scope and we also have a Group scope. Within the Group scope there is additionally a sub-division of Groups with identifiers A through Z. This matches the terminology that has been implemented for the Access Policy API (where modules are entirely free to determine exactly which permissions exist within their scope and for a specific identifier). Kristiaan did a great video on the reasoning and how that policy worked at DrupalCon Barcelona.

Within the system we have five users:

1. User 1 can view content anywhere (permissions: "view content" in scope Drupal; and "view content' in scope Group)
2. User 2 can view content outside of a group but not in any group (permissions: "view content" in scope Drupal)
3. User 3 can view content when it's in a group but not outside of a group (permissions: "view content" in scope Group)
4. User 4 can view content in Group B only (permissions: "view content" in scope Group with identifier B)
5. User 5 can't view content (permissions: none)

I can see in the current "scoped" system quite easily how I could assign these permissions. The management of these permissions is also entirely delegated to the system that's responsible for the scope (in this case either Drupal core for the global permissions; or the group system for the group scoped permissions).

If I want to promote User 5 from not being able to see anything to being able to view content in a specific group, then I can do that by assigning them the "view content" permission in the group scope for a specific identifier. That's essentially what group does when a user becomes a member of a group.

If I understand your proposal correctly, then in your proposed system of additive permissions, I would have to first grant user 5 the "view content' permission globally and then add the group specific permission . However, now I have a problem because the meaning of "view content" has changed. If this flow would work then that would mean that "view content' itself does not provide global content viewing rights, otherwise I've overshot my goal and assigned User 5 too many permissions. We could say "view content" doesn't do anything without another permission, but now I've broken User 2.

The proposed solution then seems to be to have another permission "view content", "view global content", "view group content". Where the top level is some kind of "enabler" and the other two are there for the specific scope. However, this causes a myriad of problems:

• First and foremost, Drupal is now suddenly aware that it has a global scope and on a site that I don't have any other scopes, I'm still stuck with multiple permissions
• Even with this more complex permission structure, I'm unable to solve my use-case for User 4, because the permissions are only on the level of the scope, whereas I want to distinguish for a combination of scope and identifier (Group B only)

The above exercise is basically the reinvention of the Access Policy API and why a purely additive permission structure does not solve the problem but a more complex system was needed where modules had the freedom to override the entire access check within a specific scope to achieve more complex access scenarios.

One such scenario would be scaling the above example to 100s of groups with 10.000s of users (you now have an Open Social installation). At that point it becomes infeasible to add permissions in multiple places just to give a user access to a single, or multiple groups. The system itself needs to be able to apply certain rules based on data relationships that exist.

The goal of this issue should not be to re-litigate the Access Policy API. I think Kristiaan is right in identifying that's where we're going with the discussion (and while trying to figure out what example I needed above, I came to the same conclusion). At least in my eyes the Access Policy API is a system with a great and proven design.

Two non-group use-cases that I'm aware of to drive that home:

• Within Simple OAuth we currently decorate the User entity to be able to overwrite permission checks based on the token. With the Access Policy API we can put that into an access check so that permissions can be cached. The permissions are either the exact permissions assigned as scopes to a client (for system-to-system communication) or they are the intersection of the application's permissions for scopes and the authenticated user's permissions (for applications on behalf of a user).
• Within Open Social we have the concept of a verified user. Which is basically the authenticated user role but after someone has performed some verification steps (i.e. everyone gets it, not based on role, but based on some user state). This is currently a role juggling nightmare and with the Access Policy API we can implement that state based logic.

The Access Policy API in its implementation has shown that there are complex Drupal use-cases where an additive system is not enough and there needs to be the flexibility to re-arrange or even entirely overwrite, based on business logic (whether that's the scope; or even the time of day or whether another senior is currently working).

The goal of this issue should be to create a system in a similar vein as the Access Policy API, where the outcome is not individual permissions that feed on to other access checks, but where the result is a bunch of conditions that can be combined into a database query (SQL or e.g. MongoDB) so that we can efficiently fetch data we know the user has access to. Within the current system we're often forced to move that kind of logic into an entity access handler where we can actually perform complex logic, which leads to overfetching from the database, filtering out, and then fetching more to make up the result set.

Oh. I think I understand now. Thank you for walking me through that. I didn't get that you could already revoke permissions with the Access Policy API. Now I see why we were talking past each other.

So riddle me this... given that we have this in NodeAccessControlHandler::checkViewAccess

protected function checkViewAccess(NodeInterface $node, AccountInterface $account, CacheableMetadata $cacheability) : ?AccessResultInterface {
    // If the node status changes, so does the outcome of the check below, so
    // we need to add the node as a cacheable dependency.
    $cacheability->addCacheableDependency($node);
    if ($node->isPublished()) {
        return NULL;
    }
    $cacheability->addCacheContexts([
        'user.permissions',
    ]);
    if (!$account->hasPermission('view own unpublished content')) {
        return NULL;
    }
    $cacheability->addCacheContexts([
        'user.roles:authenticated',
    ]);
    // The "view own unpublished content" permission must not be granted
    // to anonymous users for security reasons.
    if (!$account->isAuthenticated()) {
        return NULL;
    }
    $cacheability->addCacheContexts([
        'user',
    ]);
    if ($account->id() != $node->getOwnerId()) {
        return NULL;
    }
    return AccessResult::allowed()->addCacheableDependency($cacheability);
}

Are you saying the Access Policy API could allow you to bypass that bit at the end by revoking the user's view own unpublished content permission?

If that is the case, then....

What if, we had this hypothetical method / implementation for a query?

  protected function queryAccess(callable $conditionFactory, ?AccountInterface $account = NULL): AccessResultInterface {
    $cacheability = new CacheableMetadata();

    $cacheability->addCacheContexts(['user.permissions']);

    if (!$account->hasPermission('view own unpublished content')) {
      return AccessResult::neutral()->addCacheableDependency($cacheability);
    }

    $cacheability->addCacheContexts(['user']);

    $condition = $conditionFactory('OR')
        ->condition('status', 1)
        ->condition(
          $conditionFactory('AND')
            ->condition('status', 0)
            ->condition('uid', $account->id())
        );

    return AccessResult::allowedWithCondition($condition)->addCacheableDependency($cacheability);
  }

I assume you would also be able to prevent the hypothetical allowedWithCondition to be invoked by likewise revoking the user's view own unpublished content?

I realize I'm overly simplifying the example, you would also have other conditions that would/could be added based on the permission.

Are you saying the Access Policy API could allow you to bypass that bit at the end by revoking the user's view own unpublished content permission?

Yes absolutely. You would implement AccessPolicyInterface::alterPermissions.

What if, we had this hypothetical method / implementation for a query?

I think the "use permissions to influence the query" is a solved problem? That's essentially implemented by existing query alters that should already be performing permission checks together with the query that's being altered. So implementing the right Access Policy API would already achieve that.

The problem area is (in my understanding) much more in scenarios where complicated business rules need to be combined together.

In your example there's already 2 business rules being combined which are:

1. ->condition('status', 1) - User can view any published entity
2. ->condition('status', 0)->condition('uid', $account->id()) - User can view unpublished entities they own

Now lets add a new condition that lives in a different system and requires data from different tables:

1. User can view unpublished entities in any group they're a member of a group and on that membership have a role that has the "view unpublished content" permission

This business rule would be based on the configuration of permissions for a type of group. There's not a specific permission for the user we can check, because it not only depends on the scope, but on the identifier of group. There may be 100s or 1000s of groups so we have to encode this logic in the query itself and join into the membership table.

This would already be impossible to write as a simple entity condition in the current entity API because there's no path from the content entity (Node in the example, but could be any entity) to the membership table. That's because for efficiency (to prevent modifying the group or user whenever memberships change) the structure is as follows (<:: and ::> indicate entity references):

[group]  [<:: group_membership ::>] [user]

So in this case the system that wants to add these conditions also needs to indicate that this join is needed to modify the query, so here the information already becomes more complex. That's likely a change we can make in your proposed additive system by providing more information.

Lets complicate it more, we introduce a new business rule:

1. We want to add the additional restriction that Rule 3 only works on entities that have "initial review = TRUE" status. "initial review = FALSE" unpublished entities can only be viewed if the user has the additional "provide initial reviews" permission. This should only happen within a specific set of groups, it should not affect the global scope

In this case the additive system that's linked together with "OR" is no longer sufficient and we instead need to be able to identify the conditions added by Rule 3 so that we can create our changes on that rule.

I think within the current system (and within your proposed example) the biggest failure is probably that the conditions are mostly just put into a pile. There's no identifier for the conditions that allow saying "this should combine with or replace that".

I think the idea of having a separate ConditionFactory is the right direction towards the ValueObjects that Kristiaan was talking about. Basically "declare your goals" (which is very similar to declaring permissions) and then let another system combine those goals efficiently. The reason to stay away from queries for slightly longer is that part of the goals might be "I need data from this other part of the database".

I think an important business rule that's not part of the above scenario but definitely should be considered to design/test the system is a rule that we originally discussed:

1. Rule 1 should only apply to the global scope. Published entities in the scope "Group" should only be seen if the visibility of that group is "public". If the group visibility is not public then the user may only see published content in groups they're a member of

Hey, I've been following this thread for a while now and I'm impressed by the great discussion. It seems we're already quite deep into details and experiences. So I'd like to ask if these details are the right level already or if it might make sense to first look how other large frameworks (maybe from other programming languages) are solving this and how their access API works?

Maybe other general approaches solve things differently, hopefully better, and maybe already solve some of the things we're discussing. Maybe they even offer a better DX?

And with the issue growing fast, I'm unsure if it might make sense to split it?

Thank you all very much, I'm seeing a lot of experience here!

I think the "use permissions to influence the query" is a solved problem? That's essentially implemented by existing query alters that should already be performing permission checks together with the query that's being altered.

heh. that is what brought me here and is better explained by 🐛 QueryInterface::accessCheck does nothing Active . Effectively there isn't a way to do this other than directly editing the SQL itself (which means it only applies to content entity queries that use Sql Storage), but at that point the query will be run (no opportunity to no-op the query), and you no longer get to work with the EntityQuery, you have to work with the SqlQuery. The only work-around I can find is by directly attaching to the access logic into the query when it is being built, which is what core does. Which I guess is the way I'm going to implement for now, which feels gross.

This would already be impossible to write as a simple entity condition in the current entity API because there's no path from the content entity (Node in the example, but could be any entity) to the membership table

You can use the conditions across entity references even with deep references, so as long as they are connected with entity reference, this isn't a problem (as far as I can tell). Of course, it does not allow one to make an arbitrary entity join which is somewhat annoying, but that seems like a separate issue.

Is another way to solve this problem by attaching scopes to the entity query? Wouldn't that express the intent without having to actually know anything about the query itself (other than the entity type and the scope)?

In the Access Policy module → , I added support for very flexible query access control using `hook_query_tag_alter`. It's very complex with many, many joins so obviously not ideal. It can also start to return unpredictable results when you have a lot of OR/AND conditions.

One of the areas that I explored is to introduce a kind of entity field indexing mechanism. This would create a two step process not too dissimilar to `entity_grants`. At a very high level.

1. Define which fields are responsible for access.
2. Index the values in those fields into a single table.
3. Join against that table to control access.

As a simple example, let's say I want to support Private content and content restricted by Department.

1. Define which fields are applicable: uid and field_department
2. Run an indexing process where it stores the uid and department in one row in a table.
3. Join against that table to control access.

This approach will be incredibly fast though it has some limitations:

• Eventual consistency
• How to efficiently handle fields that have multiple values.

Note that I'm using fields as an example but it needn't be restricted to that. Just some set of values about an entity.

Also, speaking more generally, I think having some kind of generic entity indexing mechanism in core could be very useful in a lot of applications, not just access.

Just some food for thought.