The node access layer currently performs an OR between any grants that have been proposed by the various node access modules that are installed. Some sites need AND, and some need more complex conditional logic. But there is no good place today to inject custom logic into the API. This patch introduces a drupal_alter() when we save a node so that a custom module can alter the grants that get written to the DB. This is the most efficient place to let modules control their site's destiny.

This patch comes form the node_access working group that met in Szeged.

CommentFileSizeAuthor
#88 node-access-alter-309007.patch2.26 KBwebchick
#83 309007-grants-alter_2.patch13.5 KBagentrickard
Passed: 11430 passes, 0 fails, 0 exceptions View
#74 309007-grants-alter.patch13.89 KBagentrickard
Failed: Failed to apply patch. View
#73 309007-grants-alter-final.patch13.88 KBagentrickard
Failed: Failed to apply patch. View
#72 309007-grants-alter-alt.patch13.26 KBagentrickard
Failed: Failed to apply patch. View
#71 309007-grants-alter.patch13.01 KBagentrickard
Failed: Failed to apply patch. View
#68 309007-grants-alter.patch12.42 KBagentrickard
Failed: Failed to apply patch. View
#60 node_access_records_alter.patch8.64 KBmcarbone
Failed: Failed to apply patch. View
#59 node_access_records_alter.patch8.64 KBmcarbone
Failed: Failed to apply patch. View
#52 node_access_records_alter.patch7.8 KBmcarbone
Unable to apply patch node_access_records_alter_4.patch View
#48 node_access_records_alter.patch8.15 KBmcarbone
Failed: Failed to apply patch. View
#45 node_access_records_alter.patch7.82 KBmcarbone
Failed: Invalid PHP syntax in modules/node/node.api.php. View
#42 node_access_records_alter.patch7.27 KBmcarbone
Failed: 10938 passes, 1 fail, 1 exception View
#33 node_access_records_alter.patch5.84 KBmcarbone
Failed: Invalid PHP syntax in modules/node/node.test. View
#31 node_access_records_alter.patch5.84 KBmcarbone
Failed: Invalid PHP syntax in modules/node/node.api.php. View
#29 test_node_access_records_alter.patch3.76 KBmcarbone
Failed: 10691 passes, 2 fails, 0 exceptions View
#28 test_node_access_records_alter.patch3.75 KBmcarbone
Failed: 10691 passes, 2 fails, 0 exceptions View
#22 hook_node_access_records.patch1.57 KBagentrickard
Failed: Failed to install HEAD. View
#17 drupal_alter.patch710 bytesbcn
Passed: 11412 passes, 0 fails, 0 exceptions View
#17 alter_node_docs.patch1.6 KBbcn
Passed: 11412 passes, 0 fails, 0 exceptions View
#8 alter_node_access_docs.patch1.63 KBagentrickard
Failed: Failed to apply patch. View
#7 alter_node_access_docs.patch1.63 KBagentrickard
Failed: Failed to apply patch. View
mw.patch849 bytesmoshe weitzman
Failed: Failed to install HEAD. View
Tag1 supports the Drupal Project.Tag1 logo

Comments

Dave Cohen’s picture

Status: Needs review » Needs work

I was told this was a one-line patch. But it's two. So -1.

Kidding! But seriously, during that fateful day in Szeged Ken convinced me that this alter calls for a corresponding alter after node access grants are compiled. So let's make this truly a two-liner by adding another drupal_alter, then it gets +1. Or has our thinking changed?

moshe weitzman’s picture

Status: Needs work » Needs review

Thats a different patch, David. It may or may not be needed. One step at a time.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

This is a beautiful patch. Very much wanted because it will allow custom modules to control the interaction between node access modules, allowing for advanced business logic.

Patch is clean. Tests pass. http://testing.drupal.org/node/14433.

Dave Cohen’s picture

Ok, then. +1 and thanks.

agentrickard’s picture

For what it's worth, I think we still need the second patch, since the SQL queries are written based on hook_node_grants(). Unless, of course, we totally revise the current SQL logic.

But this part of the node_access upgrade is ready to go.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Since it would need to be written anyway, can someone please write up the documentation for hook_node_access_records_alter(), including a code example which shows how this hook could actually be used in a practical sense? That'd probably make it easier to grok why this is needed.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
Failed: Failed to apply patch. View

Documentation patch.

The idea here is that some sites need more complex node access rules. But we do not necessarily want more rows in the {node_access} table. This hook would allow a module to read the existing grants and write, in theory, a single record that defines the behavior of multiple node access modules at the same time.

agentrickard’s picture

FileSize
1.63 KB
Failed: Failed to apply patch. View

Revised docs patch, with Typo Correction [tm].

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

doc looks reasonable to me

moshe weitzman’s picture

@webchick/dries - Anything else needed here?

webchick’s picture

Sorry, I still feel like this one probably needs a look from Dries since I do not grok the node access system all that well. I will add it to the "hit list" I send off tonight.

agentrickard’s picture

IMO, this makes Node Access more Drupal-y, since we fire an alter hook before saving. Thinks of this as the Node Access equivalent to hook_menu_alter(). Normally, you won't need it, but when you do, it is a lifesaver.

Dave Cohen’s picture

Another +1 for this patch from me. Here's an example of an issue in my queue which can't be addressed without this patch: http://drupal.org/node/323360.

(On a side note... Since it requires some momentum to get any patch into core, should we include the call to alter the grants in this same patch and push them both into core at once? That this, this patch allows one to alter the records and affects the records written to the node_access table; I'm talking about another alter for the grants which could simplify the SQL when nodes are queried.)

doc2@drupalfr.org’s picture

+1 for that patch (from the author of the above mentionned issue #323360: How to arbitrate priorities between TAC_Lite and Workflow_Access grants?)

From an humble non-Drupal-knight developer point of view, +1 for including the simplified SQL queries.

cburschka’s picture

Both patches still apply. I haven't retested them, though.

Anonymous’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

bcn’s picture

FileSize
1.6 KB
Passed: 11412 passes, 0 fails, 0 exceptions View
710 bytes
Passed: 11412 passes, 0 fails, 0 exceptions View

Both patches re-rolled for latest head.

bcn’s picture

Status: Needs work » Needs review
agentrickard’s picture

Looks good. Will RTBC after the automated test returns.

Dries’s picture

This looks reasonable, but I'd like to see a bit more documentation in the code as well. It doesn't need to include an example, but it is worthy of a few lines.

Dries’s picture

Status: Needs review » Needs work
agentrickard’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
Failed: Failed to install HEAD. View

Adds extra explanation of the alter hook inside the docblock. Fixes a grammar issue with 'Retain...' capitalization.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

The comments are lovely.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

We want a test for this, as well as an update to the API documentation that is now part of core.

agentrickard’s picture

Tests are beyond me -- not even sure what we would test here. Documentation patch is http://drupal.org/node/309007#comment-1175506.

mfer’s picture

subscribe

mcarbone’s picture

Kind of a weird thing to test, right? All this is doing is allowing the $grant array to be altered. Still, I think it would be a shame if this died on the vine due to a lack of a test, so here's a suggestion:

Write a hook_node_access_records function that gives an arbitrary grant for a particular node. (E.g., make up a realm and give it view access.) Then, write a hook_node_access_records_alter that replaces the $grant array entirely with a wholly different grant (different arbitrary realm). If that latter grant is the only thing that exists in the node_access table after node_access_acquire_grants is called, then the test passes.

Does that sound reasonable?

mcarbone’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
Failed: 10691 passes, 2 fails, 0 exceptions View

I decided to go ahead and write my first SimpleTest for this.

mcarbone’s picture

FileSize
3.76 KB
Failed: 10691 passes, 2 fails, 0 exceptions View

Noticed a few typos in the comments. Second version.

Status: Needs review » Needs work

The last submitted patch failed testing.

mcarbone’s picture

Status: Needs work » Needs review
FileSize
5.84 KB
Failed: Invalid PHP syntax in modules/node/node.api.php. View

Er, the test failed because I didn't include the patch, so of course the SimpleTest failed. I re-rolled it with everything: the API documentation, the patch itself, and the SimpleTest.

Status: Needs review » Needs work

The last submitted patch failed testing.

mcarbone’s picture

Status: Needs work » Needs review
FileSize
5.84 KB
Failed: Invalid PHP syntax in modules/node/node.test. View

Fixed typo in variable_get in node.api.php.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the fine test. Code looks good. Tests pass. i think we are good to go. This adds one line of code, and many more lines of docs and tests. RTBC.

In the future, generate your diff with -up options as per http://drupal.org/patch/create

keith.smith’s picture

Status: Reviewed & tested by the community » Needs work

Note the mutliple instances of mutliple.

mcarbone’s picture

Sorry about the patch, I do use -up but I misplaced it and didn't notice its absence.

"Note the mutliple instances of mutliple."

I assume you are referring to the fact that the SimpleTest doesn't involve multiple node access modules? I see no reason why it should -- we are testing the alter itself -- what grants are passed to the alter are sort of irrelevant, whether they are from one or seventeen node access modules. This test verifies the functionality of the alter hook -- whether module programmers are capable of merging multiple grants into something else is irrelevant to core, and should be tested within those modules themselves.

mcarbone’s picture

keith.smith -- any chance you could elaborate on your reason for switching this to "needs work", or respond to my comment above? Otherwise, I'll switch this back to needs review or RTBC.

Dave Cohen’s picture

Status: Needs work » Reviewed & tested by the community

Note the single instance of multiple.

;)

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

keith.smith’s picture

I apologize if my earlier comment wasn't clear.

>  * ways that mutliple node access modules interact. The preferred use of 
>  * this hook is in a module that bridges mutliple node access modules

Note the multiple instances of the typo "mutliple".

Dave Cohen’s picture

Ah... I was trying to parse that. I searched the patch for "multiple" and only found one. I was trying to be funny, and also trying to accelerate this issue.

mcarbone’s picture

Status: Needs work » Needs review
FileSize
7.27 KB
Failed: 10938 passes, 1 fail, 1 exception View

I have no idea why the patch failed testing with a PHP syntax error in node.test, but I've run cvs update and re-rolled the patch with the typos fixed that keith.smith pointed out. We'll see how it goes.

mcarbone’s picture

Status: Needs review » Reviewed & tested by the community

Patch passed all tests and 'mutliple' typos fixed. Otherwise nothing changed since Moshe and Dave RTBC'd this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

mcarbone’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
Failed: Invalid PHP syntax in modules/node/node.api.php. View

Ah, so this is where testing really shines. What happened was that the 'Node RSS Content' test uses the same node_test.module that defines the arbitrary grants for testing hook_node_access_records_alter, but it wasn't passing. So I had to add the proper grants to that module so that other tests using that module don't break.

However, that didn't resolve the issue, because there was a variable typo in node_access_view_all_nodes(). I fixed that typo as well and now both tests run successfully -- this is great, because between these two tests we now have something testing node_access_view_all_nodes().

I've marked this for review since there is now an added fix. Let me know if it's inappropriate to fix the variable typo in this patch -- if so, I'll open another issue for it and mark it as critical. Also let me know if you think there should be yet another test written to test node_access_view_all_nodes() explicitly, rather than between two tests.

Otherwise, hopefully this will pass the testing bot and get marked as RTBC again.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

This is once again at RTBC. Thanks mcarbone.

There is overlap in node_test.module between this and #353847: Tests for node_access. Whichever gets in second will need a reroll. No big deal.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

mcarbone’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.15 KB
Failed: Failed to apply patch. View

Somehow bad PHP snuck into hook_node_build_alter from another commit and didn't get caught by testing. I made the simple fix, and am switching this back to RTBC.

mcarbone’s picture

Status: Reviewed & tested by the community » Needs review

Actually, let's let the test run again.

mcarbone’s picture

Status: Needs review » Reviewed & tested by the community

S'all good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

mcarbone’s picture

Status: Needs work » Needs review
FileSize
7.8 KB
Unable to apply patch node_access_records_alter_4.patch View

My impatience is not a virtue. Someone else fixed the other bug -- I just should've waited it out. Re-rolled.

mcarbone’s picture

Status: Needs review » Reviewed & tested by the community

Passed testing, this is the exact same patch that Moshe last RTBC'd.

Dave Cohen’s picture

Status: Reviewed & tested by the community » Needs work

Shouldn't the hook_node_access_alter be passed the node, in addition to the grants? That way, logic could be based on node type or other factors, in addition to grants provided by other node_access modules.

This idea was prompted by #463130: Restrict node_access by node type (tac_lite).

I hate to change status away from RTBC. If others feel this is not necessary, I can live without it.

Unfortunately drupal_alter takes only one data parameter, so we'd have to build a composite structure or invoke the hook another way.

moshe weitzman’s picture

drupal_alter can take many params - only the first may be "altered"

Dave Cohen’s picture

Cool. So my suggestion is simply to add the $node after $grants. Makes sense, right?

moshe weitzman’s picture

yes, agreed.

mcarbone’s picture

OK, I'll pop that in later today.

mcarbone’s picture

Status: Needs work » Needs review
FileSize
8.64 KB
Failed: Failed to apply patch. View

I've added the $node argument to the drupal_alter, and have also added another test that addresses its presence. Setting issue for review.

Nice suggestion, Dave.

mcarbone’s picture

FileSize
8.64 KB
Failed: Failed to apply patch. View

Oops, fixed a minor typo.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

thx. rtbc again.

meba’s picture

Lovely. Huge +1 from me, this is exactly what I was planning to do and needed.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

So a lot of things have happened since Oct 2008. ;)

1) I'm still no expert in the node access system, but I understand it a lot better than I did back then, including encountering a use case for this very hook.
2) This patch now has lovely documentation.
3) It also has lovely tests!!
4) It's gone through several improvement iterations beyond its initial state, and looks to be pretty solid for real-world use cases now.

Just a couple things (most of them very minor), then I'm cool with committing this:

+function hook_node_access_records_alter(&$grants, $node) {
+  // Check to see if we have enabled complex behavior.
+  if (variable_get('example_alter_node_access', FALSE) && !empty($grants)) {
+    // Our module removes the author grant.
+    foreach ($grants as $gid => $grant) {
+      if ($grant['realm'] == 'example_author') {
+        unset($grants[$gid]);
+      }
+    }
+    sort($grants);
+  }
+}

Remember that this is the example documentation for everyone on earth who wants to assess whether this is the hook they need to do whatever task they have in front of them. Can we please pick a more "real world" use case here, or, if we must be arbitrary, then add more comments to explain why unsetting the gid would be desirable here? Because currently it's very confusing, since that looks like you'd be introducing a bug.

+  function testGrantAlter() {

Needs summary PHPDoc line.

+    $this->assertEqual($records[0]->gid, 1, t('Grant with gid = 1 acquired for node without alteration.'));
+    

Please remove trailing whitespace on the blank line here.

+
+/*
+ * Implementation of hook_node_grants().

Should be /** not /*

+  // Give everyone full grants so we don't break other tests.
+  return array(
+    'article_realm' => array(1),
+    'page_realm' => array(1),
+    'alter_realm' => array(2),
+  );

This could do with some clarification. How does array(1) and array(2) grant full access? Could we get 2-3 more sentences here to explain what's going on?

merlinofchaos’s picture

A more real world use case would be to pick one element of a node such as a taxonomy term or an author and create 'specific' access control based upon that.

IMO the documentation for hook_node_access_records_alter() does not contain documentation to what the $grants array is. It should either document it or provide an @see to where this data structure is documented. Otherwise, a newer user to the node access system will find this to be a highly documented wall, wherein the documentation is insufficient, yet everyone else will go "But see, it's documented!!!"

Otherwise, this patch is fine. I would still really love a non-database method of adding additional node access but at least this can let us do complex changes based upon site needs. It is a start.

I disagree that the 'other' alter hook should be a separate patch. I believe that without being able to alter the grants as well as the records, this patch is far less useful. I think they should be packaged together, because the matching patch is also quite simple.

agentrickard’s picture

Working on API docs.

Dave Cohen’s picture

I disagree that the 'other' alter hook should be a separate patch. I believe that without being able to alter the grants as well as the records, this patch is far less useful. I think they should be packaged together, because the matching patch is also quite simple.

I agree with this.

I see this thread as good work by a lot of people, and an example of how difficult it is to get a change into Drupal - even when everyone is in favor of it. I wouldn't stop this patch because it doesn't have the 'other' alter hook. But if we don't do it now I don't expect we ever will.

webchick’s picture

Yeah, let's do it here. We've got the attention all of the people who understand the node_access system (all ~five of them ;)), plus both core maintainers.

So. What we need:

a) Another well-documented entry in node.api.php for hook_node_access_grants_alter().
b) An implementation of this hook in node_test.module
c) A similar test in node.test to prove that it works.
d) The probably 2 lines of code required in node.module for the actual functionality. ;)

@Dave Cohen: Sorry it's taken so long. It's just that the tweakier and deeper the functionality in Drupal, the more important it is to make absolutely sure it's well-documented and well-tested. The less a subsystem is understood by the wider Drupal community, the more of a chance there are for obscure bugs to break things.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
12.42 KB
Failed: Failed to apply patch. View

Here are all 4 of webchick's requests. The new test case for hook_node_grants_alter() is deliberately simple, merely checking that the altered array is != to the original for each $op. (I also changed the grants of node_test.module to be test_ for clarity.

The API docs may need a bit of a rewrite. The only other piece I hesitated on is this:

We allow $grants to be passed by reference, so it is possible that some bad author could turn $grants into a non-array. This would force a fail on return array_merge(array('all' => array(0)), $grants);.

So should we be checking that $grants is still an array here?

merlinofchaos’s picture

The documentation for hook_node_access_records_alter() should have an @see to hook_node_access_records I think. This is probably true for the _grants_alter as well.

Also when I looked this up I noted that the help for hook_node_access_records is minorly broken (merely formatting errors) -- this could probably be corrected in either this or a subsequent patch depending upon what people feel more comfortable with. Since it's a documentation patch it may not be an issue either way.

webchick’s picture

@merlinofchaos: Could you split that off as a separate issue and tag it "Novice"? Always nice to identify little bite-sized chunks like that that we can give to someone new. :)

agentrickard’s picture

FileSize
13.01 KB
Failed: Failed to apply patch. View

Added the @see (plus a @see drupal_alter() for additional clarity.

I also noticed that we call the second hook 'hook_node_access_grants_alter'. This is more clear, I think, but a violation of the _alter naming convention, since the original function is 'node_grants.'

So we might rename this to hook_node_grants_alter.

agentrickard’s picture

FileSize
13.26 KB
Failed: Failed to apply patch. View

Here is an alternate patch that corrects the name of the _alter hook and forces $grants to be an array, in case someone munges the variable passed by reference.

This patch may be preferred to the one in #71. Let the committer decide!

agentrickard’s picture

FileSize
13.88 KB
Failed: Failed to apply patch. View

We had an extensive discussion and edit of the docblock in #drupal today.

Attached is the final (?) language for the patch. Also removed the $grants array check, since we do not normally do so on drupal_alter() calls.

agentrickard’s picture

FileSize
13.89 KB
Failed: Failed to apply patch. View

tha_sun caught a few errata in the last patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in.

Dave Cohen’s picture

here here! This is awesome.

EclipseGc’s picture

OK, this is absolutely going to be a killer feature for D7!

The docs are looking pretty good, and I think this will open up node access to a whole new generation of module developers. Big ++ all around here.

Eclipse

mcarbone’s picture

Wow, looks like I missed all the weekend fun! Great work, everyone -- the latest patch looks solid.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

agentrickard’s picture

Status: Needs work » Reviewed & tested by the community

I assume that fail came from the broken HEAD over the weekend?

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

agentrickard’s picture

WTF? This sits for 9 days (again) and breaks?

agentrickard’s picture

Status: Needs work » Needs review
FileSize
13.5 KB
Passed: 11430 passes, 0 fails, 0 exceptions View

Ha. Patch was borked by my own docblock patch #472658: Clean up inline comments; check for Capitals and periods..

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Resetting, since only the line numbers changes since last review.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Ok, sorry folks. I needed a good half hour to re-review the docs before committing this patch, and half hours are hard to come by lately! :)

The docs are vastly improved, the examples much more "real world," and I can pretty much follow this the whole way through now. There are still places where the node access system is a bit fuzzy for me, but that's not the job of this patch to fix. It'd be great though if hook_node_grants() and hook_node_access_records() could be given a similar documentation treatment, though. Well done.

Committed to HEAD! WOOHOO! :D Please update the 6.x => 7.x page to inform developers of these new hooks.

agentrickard’s picture

Status: Needs work » Fixed

Thanks.

@webchick, that was more a testbot rant than a comitter rant.

Docs are Node access hooks now have drupal_alter() functions.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

webchick’s picture

Here's a Drupal 6 version of this, in case someone wants it. Trying it to see if it'll let me get Domain Access and User Relationships Node Access to play nicely together.

hefox also mentioned http://github.com/hefox/alter_grants/blob/master/alter_grants.module which attempts to do this without a hack to core.

mcarbone’s picture

There's also the module grants module which ANDs grants instead of ORing them: http://drupal.org/project/module_grants