Module Grants does not entirely override node_access
| Project: | Module Grants |
| Version: | 6.x-2.6 |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
Module Grants works by using a menu_alter to override the 'access callback' parameter for nodes so module_grants_node_access is used instead of node_access from node.module when accessing node view, edit, and delete pages.
However, several contributed modules, and indeed core itself, directly call node_access to check for access before taking certain actions. Therefore, in some situations, even though accessing node pages will fail due to module_grants_node_access, other actions will not fail when node_access is called directly.
Not sure there is a solution here, except maybe to put a warning on the module description page? This issue will be resolvable with the new node_access hooks in Drupal 7, but in Drupal 6, it may require hacking node.module to ensure that module_grants_node_access is always called instead of node_access.
#1
Well we can introduce very very simple core patch, something like inserting module_grants_node_access() call at the beginning of the node_access() function. It would be reliable enough, and not hard to maintain.
If I started to work on module like Module Grants, I would start with core patch and it would also then remove the need for overriding access callback.
Yes core patches are generally considered bad, but when you need fix faulty behavior of the core it's worth the hassle.
#2
Yeah, crea, I do think that's the only safe way to go with Drupal 6.
#3
Node access misbehaving is critical.
#4
Should we patch _node_revision_access() too ? Module Grants implements replacement for it too, but it's private function (starting with underscore) and other modules shouldn't call it directly. Do you know any cases of modules directly calling _node_revision_access() ?
I would say, since it's private function, calling it directly would be wrong use so we shouldn't support wrong use (and general Drupal policy is the same).
#5
Also I worry about this comment in node module:
* In node listings, the process above is followed except that* hook_access() is not called on each node for performance reasons and for
* proper functioning of the pager system. When adding a node listing to your
* module, be sure to use db_rewrite_sql() to add
* the appropriate clauses to your query for access checks.
It seems Module Grants doesn't work for example for Views-generated lists, because Views queries are altered using core node access logics (in db_rewrite_sql() ).
So Module Grants is badly broken because of inconsistent behaviour between single node view and node listing view.
#6
Regarding db_rewrite_sql() see http://www.advomatic.com/blogs/marco-carbone/using-multiple-node-access-...
Module Grants is incomplete in this area indeed..
#7
For db_rewrite_sql() I created another issue, cause it's different problem: #601064: Module Grants behavior is inconsistent for node listings
#8
I agree that we don't need to patch _node_revision_access() -- modules shouldn't be calling that directly, and if we find one in contrib, we can file a patch on that.
#9
Agree with all of the above. In particular:
Node access misbehaving is critical.
But more than anything this is critical to core. If core D6 didn't misbehave the way it does, then Module Grants wouldn't have to exist.
Re #1:
"If I started to work on module like Module Grants, I would start with core patch and it would also then remove the need for overriding access callback."
Yes that's the trade-off: make everyone install a core-patch (with possibly far-reaching side-effects), which they may need to re-patch when new versions of core come out, or... keep it clean with a contrib module, which is the path that was chosen with Module Grants.
The latter brings with it the almost impossible task of "fixing the engine (i.e. core) of a car without lifting its bonnet".
As far as "several contributed modules, and indeed core itself, directly call node_access to check for access before taking certain actions" goes, thanks for the suggestion, I will put a warning on the Module Grants project page. Marco can you supply us with some contrib modules and/or core operations that are known to naughtily call node_access() directly?
Rik
#10
I discovered it with the print module. Unfortunately, calling node_access directly isn't really naughty, as core itself does it, and in general it's considered an API function. So I wouldn't put the blame on core for that (except for not making node_access alterable, which is fixed in D7). I think it's Module Grants responsibility to 1) provide a core patch; and 2) give a warning. Users who can't maintain a core patch are very unlikely to be able to fully benefit from Module Grants, so I think it's an unfortunate necessity.
#11
As for which functions call node_access directly in core, see: http://api.drupal.org/api/function/node_access/6
#12
Well, maybe core behavior is wrong, but atleast it is consistent. With some workarounds it's quite usable, and being consistent means it can be trusted.
Module Grants, otoh, is not consistent and getting different node access logics in different places is much much worse.
As for example of node access module...well, take any module. node_access() is public function for checking access. Module developers are expected to use it. Disclaimer won't do much, it should just be consistent, and that means core patch is a must.
update: huh, mcarbone already said everything.