[META] Allow more granular access on default farmOS Views

Oops, @paul121 already opened an issue for this: #3413263: Allow more general access to farm record views →

I'll close this one as a duplicate.

Actually, I think I'll reopen this as a [META] issue, and make #3413263: Allow more general access to farm record views → a child issue, since it proposes a specific solution. There are other potential solutions, so we can use this issue to document/discuss the advantages/disadvantages.

@paul121 @pcambra and I discussed various approaches in chat recently. I'll try to document some of the main points here.

As I understand it, there are two ways to approach the problem currently, each with advantages and disadvantages, as well as a potential third option that Drupal core may provide in the near future.

The two options we have currently are:

1. Alter entity SQL queries to add conditions that check user permissions.
2. Run $entity->access() on each entity returned by the query and filter out results before render.

#1 can be accomplished with the Entity API module's query access handler. @paul121 began experimenting with this in #3413263: Use Entity API query access handler to filter entity queries based on user permissions → .

#2 can be accomplished with the Views entity_access check → module.

Advantages of #1:

• No new module dependencies. farmOS already includes Entity API.
• Simple one-line change to farmOS entity types + an override to enable it on Log entities.
• Smaller performance impact.

Disadvantages of #1:

• Only permission based. Does not call $entity->access(), so any additional or PHP-based access logic does not get applied.
• Adds conditions to queries everywhere, which could disrupt other code/customizations.

Advantages of #2:

• Performs "proper" entity access checking via $entity->access().
• Can be targeted to specific Views, so it doesn't alter queries globally.

Disadvantages of #2:

• Adds an additional module dependency to farmOS.
• Potentially a big performance impact. This uses PHP after the SQL query is run to load every entity, run $entity->access(), and remove them from the results before they are rendered.

Tl;dr: #1 is fast and easy, but more invasive and doesn't fully check all access logic (just permissions). #2 uses proper access checking, can be more targeted, but could be a big performance hit.

I think the performance consideration is one of the most important for us to consider. We should run some benchmarks of farmOS Views that contain lots of entities right now, and again with #1 or #2 applied, to see what the differences are.

I'm also curious about how #2 affects pagination, since it filters entities out of results after the SQL query is run. My understanding is the SQL query uses LIMIT to only return one page's worth of results at a time. If #2 is filtering out results after the query, wouldn't that means you could end up with pages that have less results than they should (or no results)?

Lastly, @pcambra pointed us to a new initiative that is underway in Drupal core:

• https://www.drupal.org/docs/develop/drupal-apis/access-policy-api →
• https://www.drupal.org/node/3385551 →
• 🌱 [Meta, Plan] Pitch-Burgh: Policy based access in core Active
• 📌 Implement the new access policy API Needs work

I'm curious if this may provide a solution to our problem, but more reading is necessary to understand how it works...

@pcambra @paul121 if I misrepresented anything here please feel free to correct/amend!

One point that came up was that #1 could be considered a "pre-filter", even if it is not a full solution. It serves to perform a first pass at the query level to remove entities that the user does not have access to according to their permissions. This may not work in all situations, but it does seem like it would solve the problem for a majority of use-cases.

The counterpoint that was made to this is: it could cause issues in situations where more bespoke/custom access logic is being applied to farmOS entities.

So one idea might be to provide #1 as an option (and perhaps it is enabled by default), but allow it to be turned off. There are various ways to accomplish this. We could have a farmOS core module that alters the entity definitions that we want to apply the query access handler to, and turn that module on by default. But it could be turned off in contexts where it is detrimental. Or, if custom Views are being used, they could check the "Disable SQL rewriting" box in the View configuration. I *think* that would allow disabling the query alterations on a View-by-View basis.

There's an additional compromise which is adding whatever we were going to do on #1 to each relevant view as a filter. It is declarative and can be detected easily, without a tag query alter.
Also, #1 doesn't technically need to alter the log module, as we could alter the entity class handler definition from farm.

I agree that #2 might be too much of a risk to add at the distro level.

Another aspect to this is the fact that the current default farmOS Views use the view any {entity_type} permissions. @paul121 proposed changing this to use the new access {entity_type} collection permission (along with query access handler) in #3413263: Use Entity API query access handler to filter entity queries based on user permissions → .

In discussing this further, we realized that changing the permission on Views may actually be a breaking change, though. If any downstream users have created roles that grant the view any {entity_type} permission in order to see the default Views, and we start requiring a different permission (access {entity_type} collection), users with those roles won't be able to see the default Views.

To address this, we could consider adding an update hook that automatically adds the access {entity_type} collection permission(s) to any roles that have the corresponding view any {entity_type} permission(s). I *think* that would be safe, but we should give that some dedicated thought.

There's an additional compromise which is adding whatever we were going to do on #1 to each relevant view as a filter. It is declarative and can be detected easily, without a tag query alter.

That's an interesting idea. I wonder how to accomplish that.

#3413263: Use Entity API query access handler to filter entity queries based on user permissions → proposes simply adding this to our entity definitions:

"query_access" = "\Drupal\entity\QueryAccess\UncacheableQueryAccessHandler"

... which affects all queries.

Perhaps we could provide our own query access handler that extends that one, but allows more targeting of specific Views?

Also, #1 doesn't technically need to alter the log module, as we could alter the entity class handler definition from farm.

Agree 100%. I think we can close the Log module issue ( 🐛 Use Entity API query_access handler Needs review ) and focus on implementations in farmOS alone.

Perhaps we could provide our own query access handler that extends that one, but allows more targeting of specific Views?

Another idea that is coming out of chat w/ @pcambra this morning:

Add a custom Views filter plugin that calls out to the entity module's methods for adding conditions to queries. Then this filter can be added to individual Views as needed. This would keep things more declarative in the Views YML.

Or, do the same thing in a hook_views_tag_query_alter() and add a specific tag to the Views that need it.

These would both have the same effect, but it might be nicer to have an explicit filter in the View.

Add a custom Views argument plugin that calls out to the entity module's methods for adding conditions to queries.

I took a quick pass at this: https://github.com/mstenta/farmOS/tree/3.x-views-access-filter

Seem to be working. Probably needs further review/discussion/testing, and I did not replicate the applyCacheability() method.