Implement a query-level entity access API

There's a core issue about completely revamping the node access grants system. Based on what I've seen it seems like a huge task that isn't likely to get done any time soon, but wanted to mention it as related: #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

A lot of work has happened on #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() until a few months ago. Any chance you can continue that work, @bojanz? Or is there a reason to not build on top of that?

P.S.: Also closed #2499645: Start using the entity query access API on orders, products and stores for you :)

First: I'm SUPER duper excited about you working on this! 😀🤘🎉🎉🎉🎉 Bringing your years of experience to this subject area is certainly going to help move things forward!

I read/reviewed all of your commits since June. Most commits are "just" bug fixes. There's many more than the two listed in #7. I liked #2977318: Don't generate the "access overview" permission if the entity type has no collection link template and #2943571: Provide a BundleEntityAccessControlHandler with support for the "view label" operation a lot :)

unfortunately, none of the work in that issue went in the right direction

That's not very constructive. You could absolutely be right, but it'd be great if you could elaborate on why you make that statement. You posted two comments on that issue, both on February 17, 2017. Nearly 16 months ago.

It very well might be obvious to you why that issue went in the wrong direction, but it's not to me. It's important that the community can read the rationale backing up a certain architecture!

Perhaps you could update the IS to explain how and why this is different than #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()?

The IS update in #8 already is awesome BTW!

I started reviewing https://github.com/fago/entity/pull/52/files, but will need some more time to really grok its implications.

I reviewed the PR on GitHub here: https://github.com/fago/entity/pull/52#pullrequestreview-128082590

There, I promised to give a more high-level review on Drupal.org, which is what follows.

Overall, this looks nearly impeccable. I love how elegant this is WRT altering both entity queries and Views queries. FANTASTIC job.

I agree with your decision to forgo the core Condition class, it is sufficiently different from a database condition to ignore the core version. That's the same conclusion that we reached in JSON API, GraphQL and the Entity Access Policies module (built to pilot #777578).

I think any further comments I have will all fundamentally depend on your answer to:

unfortunately, none of the work in that issue went in the right direction

... You could absolutely be right, but it'd be great if you could elaborate on why you make that statement.
...
It very well might be obvious to you why that issue went in the wrong direction, but it's not to me. It's important that the community can read the rationale backing up a certain architecture!

I have to agree with @Wim Leers point here. The differences in approach are very clear to me after reviewing, but I'm not sure which of those (or perhaps all of them) is what "went in the wrong direction". I know a lot has changed since your last comment in #97.

Before I write any more, let me first say that far too much time has passed for me to take a prideful stance on the code written in #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter(), I would gladly burn it all in order to get something added to Drupal core for these kinds of use cases.

Please know that I'm just trying to be sure that we're not "throwing the baby out with the bathwater" in either direction.

Since I'm not sure to what you're referring about "wrong direction", this is what I see that is different, better and less capable between the two approaches:

First, I'm genuinely impressed with the SQL-level work done in this patch that is not implemented in #777578. That said, the work done here could be very easily incorporated there without any fundamental changes—in fact, we share nearly identical classes WRT to conditionsand groups and we both have caching logic associated with them.

I think what you're most likely referring to in #777578 is that its approach decided not to forego runtime access checks per entity. IOW, it is not based on queries alone. Personally, I agree with your decision to ignore them. Those could easily and quickly be removed from #777578 and I would support that move.

If my recollection serves me well, @kristiaanvandeneynde was most strongly in favor of keeping runtime checks. His rationale came from his experience with the Group module... he can probably make that case better than I would be able to.

If it's not runtime checks that went in the wrong direction, then perhaps it's scope?

AFAICT, #777578 implements a complete superset of the functionality offered by this patch. Along with extending query-level access checks to all entity types, we decided that a policy-based system would be a leap forward for Drupal on the whole (the existing permissions-based system can be implemented as one of many other policies). That would permit more finely attenuated access control rules for all kinds of applications. That work was done in YAML and config so that non-developers might find access control a more approachable task and in order to enable a second phase of UI development.

Other than that, while this patch has an open question WRT to supporting operations other than view, #777578 chose to support those.

In my mind, what would be the ideal is to merge the SQL-processing work implemented here with the policy-based approach in #777578. That would include keeping the other operations allowed there, but we would drop the "post-query" phase of access checking.

TL;DR:

• I think either approach would be a leap forward for Drupal :)
• At present, both patches are mutually exclusive. We can't do both without some refactoring of them both.
• We would be probably be best served by this patch in almost its entirety to #777578 and removing all "post-query" operations from #777578 (or reverse the direction but end up with the same result).

Wow, thanks for the very quick, thorough reply :)

Now let's look at how entity_access_policies works:
1) EntityAccessPolicyHandler is called. This class has a Handler suffix but I don't think that the intention is to provide it per entity type, it feels like a single entry point.
2) All "entity_access_policy" config entities are loaded into memory, then filtered by entity type.
3) The field condition plugins of each policy are called to provide conditions.
4) The conditions are applied to the query (not yet implemented).

This is the beginning of an insightful patch review :) It's clear you looked at it pretty thoroughly, would you consider posting more details in the module issue queue?

1. This is correct, but it amounts to a nit about the class name rather than an architectural observation. Unless there's something more fundamental you're trying to point out that I'm missing entirely?
2. Again, this is not architectural, it was something done in a sprint to get to a PoC. I agree that loading logic could/should be improved.
3. Yes.
4. This is a little less than half correct. It is implemented by computing an entity query condition and applying it to an entity query. It is not implemented for Views or other queries. This is where I was saying your work would be most welcome and beneficial.

This approach has the following smaller problems:
1) Complexity. We have an entity type and three plugin types (precondition, field condition, post condition) for the same use case. Significantly more code. No tests yet, though that's temporary.
2) Performance. We are loading all access policies for all entity types. In Commerce word, that means dozens and dozens.
3) Developer experience. As a module maintainer I can't just add a single query_access handler line to my annotation, I need to make sure I provide a config entity in my config/install, and keep it up to date if the logic changes.

1. I agree that it is more complex. Complexity must minimized but also weighed against what it enables. We did have the end goal of enabling a site admin without coding abilities to configure more complex access control rules, this requires storage, which suggests an entity type.
2. Same as 2 above it. See my previous response.
3. This is comparing apples to oranges. You're saying "as a custom entity type creator, I can ensure my entity type has query access checking enabled" is easier than "as a module or site developer, I can add my own rules for evaluating access to any entity type". In this patch, the only way to add or alter the rules for viewing and entity is to swap out the permissions handler or query access handler and implement your custom logic. This also means that only one module can have the ultimate say because handlers don't compose. The developer experience for altering an entity type definition and correctly altering an access control handler is not easier than implementing a plugin and it is certainly not easier than changing a few lines of YAML to reuse an existing condition plugin.

   It's fair to say that the latter is beyond the scope of what your patch does, but it's hard to claim that one is a poorer experience than the other if they are accomplishing entirely different things. #777578 intentionally decouples access handling from the entity type definition and makes it an orthogonal concern.

Query access is completely tied to permissions and the underlying access control handlers. The logic between the two (query access and normal access) must match.

This is what this patch accomplishes, it's not a present fact. Query access is not yet implemented for anything but nodes. It's also tautological, Drupal's current access system is permission based, therefore access is tied to permissions.

But entity_access_policy takes a different approach. Search the module for the word "permission", and you won't find any real mention. There is no attempt to generate conditions based on the available permissions, a config entity needs to be defined by hand, along with related plugins, taking into account all of the complexities. ... Which is why I think that it's wrong to put the weight on the module developer, and not on the API itself. Now that I have the full complexity of normal access checking in my head, I never want another developer to go through the same pains.

There are two condition plugins implemented, one for a handling permissions and one for handling published status. They do not need to be implemented again because they have been implemented once. I think this reveals a misunderstanding you may have about #777578—an entity type developer does not need to implement all the plugins, policies or conditions for their entity type or any at all.

The idea is that custom conditions can be developed for your entity type or any entity type. Thus a module named commerce_business_hours could be developed separately from commerce with a custom condition for "current hour >:begin & <:end & day IN :days" and that module would add a policy of "allow `buy` if business is open" with their own configuration page for business hours. This would not need to be implemented by the module maintainer of the product entity type and the author of commerce_business_hours would not need to swap the query access control handler out with their own.

I completely understand this. But it's also obvious that the scope of a full access policy system, which does query access along the way, is much larger than the scope of an API that just does query access, following existing patterns, introducing no entity types or plugin types.

I suspect that scope is the biggest element of this, which is okay. Don't forget that I said, "I would gladly burn [entity access policies] in order to get something added to Drupal core for these kinds of use cases. Please know that I'm just trying to be sure that we're not 'throwing the baby out with the bathwater' in either direction."

Since there are a few obvious misunderstandings about #777578 in #12, let's just be sure we all know what we're trading off in all regards.

My conclusion is that for entity_access_policies to support #1 properly, it would need to reimplement this PR, by having a single plugin responsible for generating the base conditions. The plugin would look at the entity type to decide whether to apply. Entity types with non-standard permissions (such as node) would provide their own plugin.

At that point you'd be right, and entity_access_policies would be a superset of the current work. It is not right now, but has the potential to be. However, I fear that the additional complexity and features that it brings make it very hard for it to land in core. It would be much easier to land #1 and provide an extension point for #2 (custom conditions generated via policies) instead.

Thanks for bearing with me on that discussion. I see where you're coming from, and you're probably right :). I agree with the bold statement then! (emphasis mine)

Let's try to get this to completion and into core ASAP! I think a lot of my review of the PR on GH is still valid.

I get the warm fuzzies when I see how technically deep, constructively critical, mutually respectful the discussion here is! ❤️❤️❤️

@bojanz++
@gabesullice++

I think #11 through #15 may be one of my favorite Drupal/open source consensus building moments ever!

The needs for entity type query access are different from the needs of site-builder-defined access. That's why we've built very different APIs.

This appears to capture succinctly why both issues have unique approaches!

Before I go into detail below, let me first state that I love the discussion going on here and will look at the latest code on Github shortly. I think we can come to a consensus on first reworking the query level and then focusing on the rest. That said, I feel like there are a few items I need to address to avoid a situation where certain modules or use cases are overlooked and left with a system that doesn't work for them.

My key point was "Query access is fundamentally tied to permissions."

I see how you would get to this conclusion but you seem to be overlooking several actual use cases that have modules written for them:

• Accessing entities based on a sub-level of permissions (Group, Organice Groups)
• Accessing entities based on the active domain (Domain Access)
• Accessing entities based on IP address, geographical location, etc.

While some of the above may still be solved by altering a query (e.g. Domain Access), they are far from tied to permissions with the possible exception of a "bypass all of these access checks" permission. So simply building this system on top of the permission layer seems ill-advised. (I'll have a look at the code and see whether this is actually the case.)

Furthermore, there are other modules which would rather not have a query run at all or are too complex to properly capture in a performant query. A business-hours-only module would, for example, prefer to completely cancel any entity query while the office is closed. This is what we tried to solve in EAP by introducing preconditions.

Then there's heavy-lifting modules, such as Group, which could probably function with altering entity queries but would have to resort to some god-awful queries to get the desired result. This is why I initially was vehement about having postconditions, but I have since come up with quite a few drawbacks to such an approach. A major drawback being the death of pagers as we know it. I'll go into more detail on these drawbacks during my session on this very subject at Drupal Europe.

To give you an example: Given a list of 20 nodes that you want to view with the Group module enabled, checking access for each one individually would be rather fast. But if any of them denied access, you'd end up with fewer than 20 (which is the pager drawback I mentioned above).

If, however, you'd tried to only get accessible nodes from the query, the query alter would have to add something like this:
INNER JOIN {group_content} gc WHERE node.nid = gc.entity_id AND (gc.gid IN ***VAR1*** OR (gc.gid NOT IN ***VAR1*** AND gc.type IN ***VAR2***))
Where VAR1 is all of the groups the user is a member in a way that it would grant him access and VAR2 is all of the relation types that would grant access to people who are not part of the owning group. This does not even take authorship into account and neither does it account for some other higher-level permissions. The resulting (huge) query might work and actually perform well, but then again it might do horribly depending on your site set-up (how many groups, group types, etc.)

Please forgive the slightly technical segue above. The goal of it was to show that, sometimes, simply altering queries might not work well at all. I do not expect the result of this issue to actually tackle the problem outlined above, though.

That said, having some form of opt-out to cancel queries altogether (see office hours example) would be a great asset if you ask me.

Yeah I agree.

The only downside I see to QueryAccessEvent is that modules like Domain Access, Group, etc. might need to fit all of their logic in an event handler, rather than the entity type's query access handler. But it still beats hooks and/or swapping out the handlers (which could lead to a fight over which module's handler has the final say).

I see my concern of short-circuiting a query is already implemented:

      if ($conditions->isAlwaysFalse()) {
        $query->where('1 = 0');
      }

I'd rather have the query not run at all, but this will do nicely I suppose. Doesn't SQL optimize this anyway?

Either way, I've reviewed the code on GitHub and it looks really promising. Nice work! I'll make sure to add this to my slides for next week.

I'd like to add in the provider info I talked about on the PR. Should I do that here or on Github?

Sounds good! Just want to make sure we don't shoot ourselves in the foot w.r.t. BC by committing something here that is hard to expand with metadata (such as the provider) later on.

🎉

🎉

This issue flew me by! Very excited to see this land : ) I realise it's not great form to comment on closed issues but I had the exact same thing in mind about the great discussion here as Wim mentioned in #16!

🎉