http://api.drupal.org/api/function/node_access_write_grants says

Modules which utilize node_access can use this function when doing mass updates due to widespread permission changes.

So, apparently, node_access_write_grants() may be called by contrib modules, right?

However, if grants are collected by node_access_acquire_grants() (for example from node_access_rebuild()), then node_access_acquire_grants() calls node_access_write_grants() only for the grants with the highest priority. No other grants are written.

This means, modules that call node_access_write_grants() directly, may write grants that would not be written during a node_access_rebuild(), and we get an inconsistent {node_access} table.

Do we have a problem here, or what am I missing?

CommentFileSizeAuthor
#13 node_access_write_grants-rename.237634.13.patch2.2 KBsalvis
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented

Good question! I couldn't say without diving into it but maybe someone with a bit more Drupal mileage would like to comment?

- Arie

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Priority: Normal » Critical

Actually, that was a rhetorical question. We do have a problem with modules calling node_access_write_grants() directly.

I just don't know what to do about it. Forbid calling that function? Turn it into a NOP (no operation) and risk breaking node access modules that may seem to work correctly in simple scenarios (only one NA module)?

Change node_access_write_grants() so that it ignores the grants that are passed in and does a node_access_acquire_grants() instead?

Who has the authority to decide how to pursue this?

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented

Who has the authority to decide how to pursue this?

I'm going to assume that wasn't rhetorical ;-)

AFAIK anyone can write a patch and present it to the community. The patch is then reviewed and tested by the community.
The final decision to commit a patch to core is the branche maintainer. For 7.x that would be webchick aka Angie Byron for 6.x I think it's Gabor Hojtsy (but I'm not sure about that)

So you could write a patch implementing this part of the API the way you think it should be done. If enough people agree with you it might make it into 7.x and then - if needed - you can back port to 6.x .

Before writing a patch I would suggest you state your case on IRC and/or the developers mailing list. They may have a solution to your problem you haven't thought of (yet)

Good luck!

- Arie

ainigma32’s picture

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

@salvis: any luck on IRC or the mailing list(s) ?

- Arie

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Not yet, but I'm working on it...

The API documentation is ambiguous:

http://api.drupal.org/api/function/node_access_acquire_grants/6
"This function is the only function that should write to the node_access table."

http://api.drupal.org/api/function/node_access_write_grants/6
"Modules which utilize node_access can use this function when doing mass updates due to widespread permission changes."

I think node_access_write_grants() should really be _node_access_write_grants(), i.e. private.

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented

@salvis: Sorry to keep nagging you ;-) Did you manage to make any progress?

- Arie

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Postponed (maintainer needs more info) » Active

@ainigma32: I've updated the D6 versions of my node access modules (Forum Access and Image Gallery Access) to not use node_access_write_grants() anymore. This comes at a price: Updating FA or IGA settings requires iterating over all nodes in the forum or gallery, and this can be done safely only using the batch system.

I don't know whether other NA modules still use node_access_write_grants(). I do know that some use node_access_acquire_grants() without the batch system, and this may time out, resulting in inconsistent node access data...

dpearcefl’s picture

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

Is this still an issue in modern Drupal?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

Yes, certainly.

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented

Could you please state how to recreate the problem? Have you tried to duplicate this problem in Drupal 7 or 8?

Please help me understand how this is a problem. If you can state a chain of events, the expected result and the actual result, maybe we can help.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented

The problem is that the module writing its grants with nawg() will always get them written. However, when rebuilding permissions, core will collect grants from all NA modules via naag() and only the ones with the highest priority win. This may result in different data and thus in unreliable node access. Very very bad!

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented

I agree. However, unless someone can write a patch to fix this problem, likely nothing will happen to this issue.

Have you tried to see if this condition exists in D7?

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Title: node_access_write_grants() vs. node_access_acquire_grants() » Rename node_access_write_grants() to _node_access_write_grants() and discourage its use
Version: 6.x-dev » 8.x-dev
Category: support » bug
Priority: Critical » Major
Status: Postponed (maintainer needs more info) » Needs review
FileSize
node_access_write_grants-rename.237634.13.patch2.2 KB

Ok, here's a patch for D8.

dpearcefl’s picture

dpearcefl CreditAttribution: dpearcefl commented

Thanks for the patch!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Category: bug » task
Status: Needs review » Reviewed & tested by the community
Issue tags: +API change, +Needs backport to D7

Makes lots of sense but not actually a functional bug.

While we can't commit the API change to D7, we could document this as deprecated there so tagging for backport (for documentation only).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs backport to D7 +Needs change record

Yikes, yeah this is spooky. :\

Committed and pushed #13 to 8.x, and the documentation in #13 to 7.x. Thanks!

This change needs to be documented in the 8.x to 7.x module upgrade guide: http://drupal.org/node/1152742

Tor Arne Thune’s picture

Tor Arne Thune CreditAttribution: Tor Arne Thune commented

Made an initial addition to the 7.x to 8.x module upgrade guide, but it probably needs a rewrite from a more technical person. The explanation is basically the one from #11.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Fixed

That looks fine to me, the documentation only really needs to say "we renamed it, don't use it, use this other function you should've been using all the time anyway", and it does that well.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Issue tags: -Needs change record

Thanks! 1V already added the upgrade guide entry.

(Our messages crossed.)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Priority: Major » Normal
Status: Fixed » Needs work

The documentation part that was committed was a bit weird. The documentation now reads (in both D7 and D8):

Modules that utilize node_access can use this function when doing mass updates due to widespread permission changes.

Note: Don't call this function directly from a contributed module. Call node_access_acquire_grants() instead.

It seems like the former sentence should be removed or at least reworded.

Marking normal so it doesn't mess up the major threshold.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

LOL.

Sorry, my bad. I clearly should not commit my own half-baked patches. :P~

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Component: node.module » node system
Issue summary: View changes

(Merging "node system" and "node.module" components for 8.x; disregard.)

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.

  • webchick committed 04ec55f on 8.3.x 
    Issue #237634 by salvis: Rename node_access_write_grants() to...

  • webchick committed 04ec55f on 8.3.x 
    Issue #237634 by salvis: Rename node_access_write_grants() to...

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.

  • webchick committed 04ec55f on 8.4.x 
    Issue #237634 by salvis: Rename node_access_write_grants() to...

  • webchick committed 04ec55f on 8.4.x 
    Issue #237634 by salvis: Rename node_access_write_grants() to...

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.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • webchick committed 04ec55f on 9.1.x 
    Issue #237634 by salvis: Rename node_access_write_grants() to...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.