I wondering if the order of node_access_acquire_grants() in very important in followed node_save() function, or it can be changed without some other problems:

...
  // Call the node specific callback (if any).
  node_invoke($node, $op);
  node_invoke_nodeapi($node, $op);

  // Update the node access table for this node.
  node_access_acquire_grants($node);
...

to this one:

  // Call the node specific callback (if any).
  node_invoke($node, $op);

  // Update the node access table for this node.
  node_access_acquire_grants($node);
  node_invoke_nodeapi($node, $op);

Because some modules like path_redirect are making redirection during nodeapi hook, so then some access module doesn't have possibility to set correct permission after that.
Any suggestions?

CommentFileSizeAuthor
#6 322636-node-access-grants-order-d7-6.patch850 bytesjaperry

Comments

ainigma32’s picture

ainigma32 commented
Status: Active » Postponed (maintainer needs more info)

Doesn't sound unreasonable but what kind of consequences would this change have?

Bumping this so some more seasoned people can look at this.

- Arie

ainigma32’s picture

ainigma32 commented

@kenorb: Did you ever get some feedback through some other channel (i.e. forums or IRC) ?

- Arie

kenorb’s picture

kenorb commented
Status: Postponed (maintainer needs more info) » Postponed

No;/
But it's good to add manually the function node_access_acquire_grants($node);
before some module is doing redirect or patch the core.

I don't remember which exactly module made those redirects during node api, maybe it was content_profile:

function content_profile_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
...
      drupal_goto('node/'. $nid .'/edit', 'destination=user/'. $node->uid);

If I remember correctly, I've made the call to node_access_acquire_grants($node) manually.
or maybe it was path_redirect, login_destination, customdestination, or some similar module.
Probably I've tested them all, without any good result.

Hopefully somebody will have similar problem to confirm this issue.

Related issue here:
#314797: permission 'view any page content' doesn't work #6

mdupont’s picture

mdupont
Primary language French
Location Switzerland
commented
Category: support » bug
Status: Postponed » Active

I think this is more a bug report, since node_access information may not be correctly updated depending on what happens during the hook. Is this still valid?

Isostar’s picture

Isostar commented
Version: 6.x-dev » 7.x-dev

I ran into this issue when making a custom module that uses a drupal_goto() in hook_node_insert() in D7.
The node did not get the correct access rights (using nodeaccess module).

After adding 'node_access_acquire_grants($node);' before the drupal_goto(). The access was correctly saved in the database.

I don't consider this a real bug, but the developer has to be aware he has to use node_access_acquire_grants before redirecting.
If there are no unwanted effects I would switch order in node_save().

japerry’s picture

japerry
Location KOMK
commented
Status: Active » Needs review
StatusFileSize
new 322636-node-access-grants-order-d7-6.patch850 bytes

For modules that rely on node access during the hook_node_insert phase, this patch is needed so they have the correct access if they call, for example, entity_access.

Related to this message_subscribe issue: #1918666: Entity Access support
Also related to #1828184: Message Subscribe sends emails regardless of context

Since no hooks should be modifying permissions before node_access_acquire_grants runs, I don't think there should be any consequences to moving it earlier. Smoke tests of the patch below seem to work just fine.

Its d7, and would need to be reviewed for d8 and potentially backported. But this patch works for d7 now.

ezra-g’s picture

ezra-g commented
Title: node_access_acquire_grants sometimes doesn't call after nodeapi » node_access_acquire_grants() should run before node save/update

Retitling to make more sense.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I would not expect to ever be able to use node_access() until node_save() has been completed. If you need to do something in hook_node_insert/update that calls node_access() on the same node, you should probably be looking to use drupal_register_shutdown_function() to delay your change. I think this is just a documentation issue.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Closed (won't fix)

I agree with Dave.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

This issue affected the CER module as well (and, by association, the Entity Reference module): https://drupal.org/node/1977710#comment-7962331.

To me, it makes sense that hook_node_insert should be invoked when ALL of the relevant information about the node has been inserted, including access records. Just my two cents.

vacilando’s picture

vacilando commented

FWIW; the patch from #6 does solve the nasty CER issue (see #10).

johnkareoke’s picture

johnkareoke commented
Component: node.module » ajax system
Issue summary: View changes

Just want to add my voice to this. Really don't like patching core, but patch in #6 seems only currently available solution to CER issue referenced in #10. Patch doesn't apply cleanly so patched it manually, and solves my issue.