backdrop-contrib / reference

Defines a field to allow Backdrop entities to reference each other.
GNU General Public License v2.0
4 stars 6 forks source link

Need to check access when retrieving entities for autocomplete reference field #15

Open herbdool opened 7 years ago

herbdool commented 7 years ago

When using an autocomplete reference field, it should check that the user has access to view the entities before allowing them to be available for selecting.

I'm wondering if this should also be available if a select widget is used or if it's better to use a view for that use case.

mikemccaffrey commented 7 years ago

I'm not sure what level of access control we need to support. To me, it seems like if an admin creates a field to reference a particular entity type (and optionally specify the bundle and whether it is published) that is implicitly stating that anyone that can edit that field should have permission to view the list of applicable entities, or at least their labels.

I know that @quicksketch disagrees with me, and has ideas of how to alter the database query to allow the access of individual entities to be controlled. I know he posted instructions for doing that somewhere, but I don't recall exactly where. Perhaps he can make the case for that course of action again here?

herbdool commented 7 years ago

I believe that backwards compatibility is important for this module, particularly if it's going to end up in core and replace References and Entity Reference. So I also believe that the permissions should work the same, otherwise it could expose private information that people don't want to expose when they use the upgrade path.

I'm not sure if it would get messy since we're dealing with different entities. The basic approach for nodes is to use node grants such as is done in References https://github.com/backdrop-contrib/references/blob/1.x-1.x/node_reference/node_reference.module. But for users, terms, X entities... I'm not sure.

mikemccaffrey commented 6 years ago

@quicksketch Is the node grants approach applicable to other entity types in general? What is a way that we can do permissions checks across all entity types?

mikemccaffrey commented 6 years ago

Okay, Nate explained the query tag concept to me again. He says that entityreference just adds these lines in when creating the query:

    $query->addTag($this->field['settings']['target_type'] . '_access');
    $query->addTag('entityreference');

From this full function: https://gist.github.com/quicksketch/7f607798b0078f5486b06cb6c510efb0

And then any entity that implements access control can alter the query to add their own tables and conditions to the query.

jenlampton commented 3 years ago

We were talking about this today in at BackdropLIVE and decided to use the existing access checks for entities rather than running our own database queries. I think this means the access callback for reference_autocomplete becomes entity_access? Hm, I don't think that will work, we'll need to some reconfiguring.

jenlampton commented 3 years ago

I found some changes on my local copy of references switching to entity_access() but I don't remember how much testing I did, and if it worked as expected. I need to focus on something else right now, so I'm pushing this PR so anyone (including myself!) can pick this up later if they want.

hosef commented 3 years ago

@jenlampton All entities have an access() method, so you can do that with $entity->access($op); as well.