This may be a fun debate with agentrickard. I think our node grant system sucks. Before we try and re-make it work across all entities I'd like to call to question if we even need the grant system anymore. My belief is that any node or entity access module knows how best to handle their own records rather than trying to 'fit' into the limitations of the core grant functionality. Therefore, any node or entity access module should either be using hook_node/entity_access() for individual nodes and using hook_query_node/entity_access_alter() for listings. Having the grant system as the 'last resort' creates more complexity in core where as it should just be handled in the access modules themselves if they can't use the existing two access hook methods.

Comments

Crell’s picture

Crell CreditAttribution: Crell commented

For anything other than list queries, I agree with you. All single-object checks already belong in hook_node_access(). For list operations, I am not yet convinced. I would like to be convinced, however.

One challenge is that node_access is already iffy on non-SQL backends. I am not sure how that even works with EFQ right now, other than saying "other backends, good luck, it's your job."

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

I would counter that core needs a more consistent API for access, and cowboy_node_query_alter() probably doesn't qualify.

It's hard enough to find node access conflicts now, largely because hook_node_access means that any module (or theme!!!) can disrupt expected behavior. Add to that the fact that it is simply not possible to return an accurate list of all content editable by user X, and I would say: Yes, node grants is flawed, but it needs to be replaced by a more robust core API instead of being left to contrib.

Discuss.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Part of the problem is that the node grant hooks do not get *any* context from the actual query being performed aside from 'op' and 'account'. It's impossible to do something like the following query and have the nodes filtered by a specific domain rather than whatever the 'current' context is. This is a recurring problem not just in Domain module.

db_select('node', 'n')
  ->fields('n', array('nid'))
  ->addTag('node_access')
  ->addMetadata('domain_id', 5)
  ->execute();
geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented

so an entity access system must

  • work on sql and non-sql backends
  • have access to metadata about the actual query

this should already work with EntityFieldQuery and hook_entity_query_alter... IF the query carries enough info in tags and metadata.
so then: what conventions about tags and metadate would we need the query caller to obey...?

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

See also the discussion at #1284492: Revision or property based access regarding FieldAPI, revisions and properties wrt access rules.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

@Dave Reid: Why does the grant system suck again?
Can you point me to documentation explaining the architecture the system is based on? I haven't been able to find it.

Crell’s picture

Crell CreditAttribution: Crell commented

That is part of the reason many people don't like it. :-)

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

http://api.drupal.org/api/drupal/modules--node--node.module/group/node_a...

See also chapter 9 of Drupal 7 Module Development. (SHAMELESS PLUG!)

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

Awesome, I have that book. On my reading list.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented

PROPOSAL:
for nodes in the database this already works using
* hook_query_alter() (for node listings) and
* hook_node_access() (for individual nodes).
there is a proof of concept from webchick which i promoted to a full module: http://drupal.org/project/nodetype_access

we might carry this over to entities:
* hook_entity_query_alter() is there for us and seems to work like a charm. i implemented a proof of concept in said module.
* hook_entity_access() should be added - see #1382252: Implement hook_entity_access()

here for convenience some code links:
* nodetype_access.module
* readme
* how to use hook_entity_query_alter()

sinasalek’s picture

sinasalek CreditAttribution: sinasalek commented

I also never liked the node access system, it's very slow.
As axel.rutz mentioned hook_entity_query_alter can take care of that quite easily and the good thing is it's compatible with no-sql backends as well. As you all know using EFQ is quite straightforward so if a particular modules want to add additional access checks it's much easier using EFQ altering comparing to grant system plus there is no need to rebuild the permission which is a very time consuming task for large website.
Regarding conflict, when several modules try to apply their own restrictions there is one problem.
If a module adds or condition all the other restrictions will be bypassed! however since EFQ currently does not support "Or" we don't have to be worried about that right know :) It's kinda similar to the problem we've had with Drupal 6 access control
Can't think of any generic solution for this right now.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

See the more recent debate at #1819726: Move node access code into a pluggable class

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented
Issue summary: View changes

I hope this will finally be done gracefully in #777578-179: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter().

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Version: 8.5.x-dev » 8.6.x-dev

Does that mean it's a duplicate issue?

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Status: Active » Closed (duplicate)
Related issues: +#777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()