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.

Catching up here... just want to mention that I think we are planning to make these changes in a new major 4.0.0 release, in which case making breaking changes (in moderation) should be OK.

Regarding views and permissions:

Adding an access {entity_type} collection collection permission likely isn't a breaking change in itself, but updating views and other interfaces to exclusively use this permission would be a breaking change. Combined with query_access handler, we could change all of our views to only need the access {entity_type} collection permission. Users could navigate to any of these views but results would be restricted according to their view (any/own) (bundle} {entity_type} permissions. Still an improvement, but also creates some bad UX if you can navigate to an entity view page even if your permissions will restrict from ever seeing any of the results.

Solving this UX issue leads me to something that hasn't been mentioned wrt Views and permissions - I think many (if not all) of our entity views should allow access with any one of multiple permissions. For example, the "Animal assets" view should require one of view any asset, view own asset, view any animal asset OR view own animal asset. Our "All assets" view could require either view any asset or view own assetpermissions, but it also seems reasonable to grant access to the all assets view as long as the user has one bundle specific view-operation permission, view any {bundle} asset OR view own {bundle} asset. This complexity is simply not solved by the declarative views config, instead we will need to manually alter access to our view pages as needed (in fact, I believe we already do this for view any {bundle} {entity_type}, just we aren't checking owner permissions?)

So... perhaps deciding if we require an access {entity_type} collection permission on top of these view-operation specific permissions is a smaller issue. IMO we should require the collection permission on views; it's a small addition and the breaking change is negligible, but the main reason being that this permission provides some separation between data model and UX needs. I admit it's not super clear *why* you might want to hide all of our view pages, but this permission would make it easy to do so (and perhaps hide other things that could also depend on the collection permission) without revoking a user's access to actual data.

That said... this more granular access on views is only possible if we add the query_access handler (or something like it)!

Adding the query_access handler to our entity types would certainly be a breaking change. But more than that, I actually think this would be a bug fix.. Right now if a user's role is configured to have the view own animal asset permission, all entity queries (and views, but let's set aside views for now) are not respecting this permission. I would not expect module developers to be considering this edge case and be adding additional checks to their logic - we missed this for many years ourselves! Ultimately, I'm really having a hard time understanding why we shouldn't add the query_access handler.

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.

I think we should only allow turning off query_access if you also allow turning off bundle + owner permissions, otherwise you introduce potential security issues. Adding this ability to turn this behavior off seems quite complex when the alternative is... just don't use the bundle + owner permissions :-)

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.

It sounds like there is some concern about how query_access and/or bundle + owner permissions would affect these more custom views. Is there any reason these views cannot just depend on higher permissions like view any asset when they want these guarantees? Otherwise there would only be an issue here if you are indeed trying to show a user something that their permissions determine they should not be seeing... now, I agree there might be scenarios where this might be valid, but it really seems like an edge case, is it not? As such I agree that the solution should be disabling SQL rewriting (confirmed in entity module code this should work) - OR maybe not use views, and instead use a custom entity query with access checking disabled (personally I would be more comfortable with this since views can get very hacky in complex situations...)

Add a custom Views argument plugin that calls out to the entity module's methods for adding conditions to queries. Then this argument 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.

Again, I don't understand why this shouldn't be the default behavior... are there any examples of views in core or farmOS contrib that would not have this enabled?

Does not call $entity->access(), so any additional or PHP-based access logic does not get applied.

Last I just want to mention... if someone is implementing custom PHP-based access logic, they should be aware that this will not be applied in all situations. It is their responsibility to implement additional entity query and views implementations. As explained in hook_entity-access documentation:

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

Currently, we are choosing to use the entity module permission provider and access handler, but are not using the entity and views query handlers they provide, which is a mistake on our part.

With very custom logic, I agree that option #2 may be the easiest and most realistic (if not only) solution.

Ultimately, I'm really having a hard time understanding why we shouldn't add the query_access handler.

Sorry if this issue hadn't been updated in a while (~1 year!), but I will say that my current thinking is aligned with yours @paul121 (I believe). I think we should enable query_access handler on all the entity types provided by farmOS, as well as on Log entities (via 🐛 Use Entity API query_access handler Needs review ), and we should add an access {entity_type} collection and replace view any {entity_type} permission currently required on our Views with it. This seems to be the best approach all around. And it is a (minor) breaking change, so let's do it in the 4.x branch.

So I think we just need to implement these changes, in the same way that we did in Log module. These are the steps, as I see them (in a 4.x-* branch):

1. Add query_access handler to all farmOS entity types (same as 🐛 Use Entity API query_access handler Needs review ).
2. Merge 🐛 Use Entity API query_access handler Needs review and tag a 3.0.0 release of the Log module.
3. Update the farmOS drupal/log version constraint to ^3.0 (in 4.x branch, not 3.x).
4. Provide access {entity_type} collection permissions for all entity types (including Log*) in farmOS.
5. Update all core Views to use those permissions.

* We could also add this in the Log module if it makes sense. No strong opinion there.

FYI #3413263: Use Entity API query access handler to filter entity queries based on user permissions → has a branch with a start on these changes, but was postponed pending the Log module's issue ( 🐛 Use Entity API query_access handler Needs review ).

I think we can move ahead with the Log module and tag 3.0.0, then change #3413263: Use Entity API query access handler to filter entity queries based on user permissions → to "Needs work". Do you agree @paul121?

Sorry if this issue hadn't been updated in a while (~1 year!), but I will say that my current thinking is aligned with yours @paul121 (I believe).

No worries and sorry if I was extra verbose. I've been thinking this for awhile but wanted to get it written down here, afraid my thoughts were lost in old chats.

I think we can move ahead with the Log module and tag 3.0.0, then change #3413263: Use Entity API query access handler to filter entity queries based on user permissions to "Needs work". Do you agree @paul121?

Yep, makes sense to me!

* We could also add this in the Log module if it makes sense. No strong opinion there.

I suppose it isn't necessary, but don't see why not? I like that Drupal core is providing this and hope it will become more commonly used.

5. Update all core Views to use those permissions.

There might be a little more than just using the collection permissions. I think we need to make sure users still have at least one view operation permission for either any or own entities like I describe above, but this shouldn't be too complex.

I think we need to make sure users still have at least one view operation permission for either any or own entities like I describe above, but this shouldn't be too complex.

Is this already taken care of in our managed roles?

https://github.com/farmOS/farmOS/blob/11e275e336bfcfb621d392e72b86b63c1a...

No, a little different. I'm suggesting that our views should require that users have at least 2 permissions. This isn't technically required, but would improve the UX in scenarios when someone only has a single view own land asset permission:

Combined with query_access handler, we could change all of our views to only need the access {entity_type} collection permission. Users could navigate to any of these views but results would be restricted according to their view (any/own) (bundle} {entity_type} permissions. Still an improvement, but also creates some bad UX if you can navigate to an entity view page even if your permissions will restrict from ever seeing any of the results.

I'm suggesting that our views should require that users have at least 2 permissions. This isn't technically required, but would improve the UX in scenarios when someone only has a single view own land asset permission:

Chatted with @paul121 about this and I have a better sense of the issue now. Basically, if we only require the new collection permission on our farm_asset View's page_type display, then it means someone who has the collection permission but does not have the view own animal asset permission (for example), will still see the "Animals" link displayed in the main menu.

It seems that we'll need to dynamically check user permissions in PHP to resolve this. Or dynamically alter the View's permissions via hook_views_pre_view() like we do currently to add bundle-specific fields/filters... but I'm not sure if display permissions can be dynamically overridden at that step or not. Might need to go deeper, or implement a custom Views access plugin.