Multiple Node Access logic patch

For the background to this approach, see http://drupal.org/node/191375.

I finally decided to go this route because a) I didn't want to store my rules in a separate db table; and b) I think this is the most flexible solution for other developers.

subscribe. this is a great step forward. put another way, this lets a site require that a user have *all* applicable grants in order to se e a node. today, only one applicable grant automatically grants access. so you cannot implement a true 'deny' grant. with this patch, you could.

Would be good to get a couple of test modules in this issue so we can test the new code. Ideally we'd do some benchmarks but I'm not sure thats needed. This is new functionality, and could only be slow for those who implement it. All existing node access modules are unaffected.

Working on some benchmarks now. Will do the following:

-- AB test with nodes and no node access modules
-- AB test with nodes and one access control module (OR logic)
-- AB test with nodes and two access control modules (AND logic)
-- AB test with nodes and two access control modules (OR logic)

Following the standards at http://drupal.org/node/79237

Well, crap. I get this error:

Apparently, this is a buffer issue in AB.

I am _not_ qualified to upgrade my Apache, so someone else will have to benchmark this code.

I will write a quick 'node_access' test module for Devel, though.

Testers may want to try http://drupal.org/node/197126, the Devel Node Access Test module (DNAT). It will auto-generate node access rules for your nodes, and it safely incorporates the proposed API change.

Do we need some UI here? Won't some sites require AND and others OR?

Subscribing. This sounds awesome.

I'm sceptical about deny grants. Right now, there's a system in place, and well-behaved modules obey the system. The system is "don't grant access unless you KNOW the user is allowed access". With this patch, you introduce a new system, and the two compete. How do you explain to the user that the access system is based on a GRANT, unless you have a certain patch/module installed in which case it's based on DENY.

The only reason I can think of to change the system is to make the node_access table more concise. That is, if 100,000 of my nodes are for the public, and 10 are private, it would be cool if node_access had closer to 10 entries rather than 100,010, if you know what I mean. But if you want to do that, I'd suggest you either a) make a third-party module that does access control better than core, or b) change core to use only one system. The fine line between grants and denies is begging for trouble in my opinion.

The one thing about this patch I am solidly in favor of is checking node_access on unpublished nodes. Right now, to show a user an unpublished node requires either giving them administer node access, or defining the node type yourself and implementing hook_access. However, even for this, I would take a different approach. I'd replace the 'grant_view', 'grant_update' and 'grant_delete' columns with a single 'grant' text column. The values found there would be 'view', 'update', 'delete', and any other grants a module might want to introduce i.e. 'purchase', 'play_media', and more to the point 'view_unpublished', 'update_unpublished', etc...

Sorry to be a nay-sayer, but -1 from me.

Dave-

I think your argument ignores vital use cases. OG may 'know' to grant access, but DA may not. In the current system, OG wins out, which is not always the desired behavior.

In order for multiple node access modules to correctly grant access, having the AND/OR logic is absolutely necessary.

It may change your mind to know that Domain Access (in HEAD at least) has a setting to control this behavior. And I think it is up to module authors -- since Node Access modules are always contrib -- to make this available.

In Domain Access, the default OR logic can be kept and is the default -- in fact, it must be kept if this is the only module that implements hook_node_grants() -- or it can be toggled to use AND. (See attached.)

And, I would also argue that Node Access is an advanced Drupal feature. Running multiple Node Access modules is very advanced behavior. For people who want this type of granular access control, this patch is absolutely required. (In fact, Domain Access comes with this patch for D5 in the download).

For D7, by the way, I agree that we have time to totally rewrite the node access system. This patch was designed for the system as it stands today, and for users who really need complex functionality.

It is really a patch against D6, but too late for this release cycle.

Attachment	Size
mna.png	14.27 KB

subscribing

+1 on the concept. Haven't wrapped my head around the details yet but it would be good to see this in D6.

^{benjamin, Agaric Design Collective}

It won't get into D6, because it would be a huge API change.

For D7, this would be great though.

@Benjamin: are you taking this one on? If not, set the 'assigned' property back to 'unassign'

I'll be happy to help out writing some tests for this one, and getting it into core after we get D6 out.

@darthsteven

I disagree that it is a huge API change. It is actually an incremental change to the API. Module developers can, in fact, entirely ignore the API change and modules will function exactly as it did in D5. The 'group' and 'check' elements of the $grants array are optional.

I agree, however, that this patch is too late for D6. Which is why I filed it against D7.

there's no way i meant to take this on! yikes, gotta watch my form fields.

but, it is not a huge API change-- everything would go on as before for modules done as they are now, but new modules would be able to take advantage of it. That is why it would be good to see it in D6, so we have a learning curve if greater API changes should be made against D7.

It is just too late in the release cycle to try this for D6, even if this is an incremental change.

I suspect the path will be: Use the patch to test this behavior in D6 (for sites that want to try). Then rethink (as needed) the node_access system for D7.

First of all thanks for the work and elegant patch.

This looked like it would fit an exact need. However, upon trying to implement and work a related patch for workflow_access I ran into a bit of a snag. (workflow_access is part of the workflow module http://drupal.org/project/workflow)

workflow_access generates one access realm for various roles, 'workflow_access', and a realm for content creators, 'workflow_access_owner'. I added the group flag to both realms in hook_node_grants() as follows;

This generates the following SQL;

Which results in a user having to have workflow access via a role and appropriate state AND be the content creator along with an appropriate workflow state. However, workflow_access, at least as coded now, really wants an 'OR' for it's two realms grouped.

The use of 'OR' (as well as a grouped SQL snippet) results in the workflow access module granting access if you should have access via a role and workflow state OR by being the content creator and appropriate workflow state.

Is this a case where the workflow_access logic should be reworked, or is this a case that this patch should be further generalized to allow a module to group 2, or many, SQL queries into one using 'AND' or 'OR'?

Should I have skipped the 'group' tag on the 'workflow_access_owner' realm, or something else?
Is there a way without heavily reworking workflow_access to target the two realms appropriately?

If I get a working patch for workflow I will share it here for further testing of this patch. Thanks for the work!

If you want both elements from workflow_access to be added using AND, but maintain an internal OR logic, they should both be assigned to the same 'group'.

The following should work as you expect:

Here is the code logic:

-- If 'group' is not defined for a grant, it is set to the default 'all' group.
-- If group is defined for a grant, it is set to the specified group.
-- All members of a group are OR chained together.
-- All groups are AND chained together.

So by setting workflow to assign all its grants to a single group, you should get a query like:

WHERE (ALL == TRUE) AND (workflow_access == TRUE || workflow_owner == TRUE)

I don't believe the patch explains how 'group' works, and I see that this is not explained in the original note.

Ideally, contrib module authors make this configurable, so either behavior could be supported.

agentrickard,

thanks, your explanation was exactly what I needed.

I do have one other question though, how does this work with the node access arbitration that takes place in node_access_acquire_grants() in the node module? See http://drupal.org/node/75395 for more info.

I haven't reviewed what's in D7, but from the code in D5 only the node access with the highest priority is written. I think that node_access_acquire_grants() will also need some adjustment. It seems for this patch to work properly all modules that have a 'group' set via hook_node_grants() will need to have an entry in the node_access table otherwise it will be impossible to grant any privilege as the SQL will never return a result. This means inserting each grant in node_access_acquire_grants, and not just the highest priority. Please correct me if I am wrong.

I do have one other question though, how does this work with the node access arbitration that takes place in node_access_acquire_grants() in the node module? See http://drupal.org/node/75395 for more info.

It makes no change to the behavior that exists in 5.x or (as I understand) 6.x.

It seems for this patch to work properly all modules that have a 'group' set via hook_node_grants() will need to have an entry in the node_access table otherwise it will be impossible to grant any privilege as the SQL will never return a result.

I believe this is also not an issue.

Why would modules not have an entry in the node access table if they are trying to check access rules? Because of the discarding of grants due to priority?

When the grants are discarded based on priority -- due to an array_merge in node_access_grants() -- they never reach the SQL building stage. Look at the node_access_grants_sql() logic in the patch.

subscribing

Revised patch for the current state of HEAD.

Attachment	Size
multiple_node_access.patch	6.09 KB

Comment review:
-- great, except for
-- a typo in "+ // Run throught the $grants, if needed and generate the query SQL."
-- two spaces between sentences, rather than one.
-- an instance of a lowercase "sql", which should be capitalized to match other instances of "SQL" in this patch (and core).

subscribe

The patch also needs some extra documentation, I think.

Note also http://drupal.org/node/241189. With the AND logic in place, every node access module would need to assert a grant to all nodes. We run into some issues with OG in cases where OG assumes that OR logic is in place and does not write grants for certain node types.

That fact classifies this patch as an API change which must be addressed by module authors.

In my earlier comment, #10, I mentioned that the node_access table should be kept small. It sounds like this patch is going in the opposite direction, requiring more entries in the node_access table from every access control module. As a maintainer of tac_lite I don't like that idea.

Why not just create a custom access module with the special logic you want?

What if instead of doing OR in groups and AND between groups we did AND in groups and then OR between them?

@darthsteven

I do not understand how that would be beneficial. The problem now is that the system is too permissive when multiple node access modules are enabled. Allowing OR grants inside a group lets a single module (or module family) handle its own grants cleanly, while the AND logic lets other groups act independently.

If you reverse that logic, you force unrelated modules to belong to the same group -- and you still have the issue where a permissive module can upset the whole system.

Right, modules shouldn't enter data into the node_access table if they don't care about the access permissions of that node, but they might still add some grants. Using the current code, this will never work. It will be too restrictive.

I'm just trying to sound out solutions. If AND groups were ORed together we'd make it so that you'd need to specify the group in each node_access module that you wanted ANDed together. I'm just trying to sound out solutions.

Okay, okay, I see where my approach breaks down. I think we need to add the group to the node_access table itself, and check groups too, otherwise we'll always run into problems.

I don't see it that way at all. The 'group' concept does two things:

- Places all elements of a group into an OR logic statement. This is the current behavior. Any grant not assigned to a group is in the default 'all' group.

- Places all distinct groups into an AND relationship with each other.

Thinking about this, however, it might be more efficient to place the 'group' into the {node_access} table. That might make the SQL statements easier. But the only real gain would be if you can eliminate subselects, and I don't know that adding a 'group' column would do that.

Now, adding a second table {node_access_group} might make that possible.

But this patch -- by design -- is an incremental change, so changing the schema was never considered.

Okay, given that we're nice and in the middle of a development cycle we can now have schema and api breaking changes etc.

The fundamental flaw in the approach taken in the patch.:

Suppose we have a node with three grants, A, B, C against it. We'd like it to be that user's would only get access if they had grant A, or grants B and C. That is:

A OR (B AND C)

But that is also:

(A OR B) AND (A OR C)

Which is how the SQL query would need to be written using the current patch's logic. So the module that provided grant A would need to put grant A in every group defined by every other module.

I'm going to have to get my hands dirty with some code to see if AND then OR works instead.

True. But the patch still gives us better control than we have now.

A UI for setting the relationships between node_access modules would also help solve that problem. It would be easy to read hook_node_grants() implementations and then let admins determine how to group various grants. In that scenario, modules would declare defaults that could be overridden.

Considering the above: what is the likelihood of a major revision of node access getting in to D7?

To me, incremental improvements are our best hope, and rejecting the patch because it doesn't solve every possible use case is a recipe for stagnation.

New version of the patch, with corrections from #24 and a great assist from SomebodySysop over at http://drupal.org/node/243731

Attachment	Size
new_multiple.patch	6.12 KB

I'm going to have to get my hands dirty with some code to see if AND then OR works instead.

I think we have to look at this in terms of how this would be used in the real world.

When used with only Domain Access, the multinode_access_patch gives us this logic:

I've actually gotten my hands dirty and been using the multinode_access_patch successfully to get two access control modules working together: Taxonomy Access Control and Organic Groups ( http://drupal.org/node/243731 ).

The logic:

With this logic, I need to explicitly give a grant to every node OG doesn't care about. TAC must also give a grant to every "uncategorized" node. But, it works like a charm. Users who belong to groups must also have taxonomy term permissions to access content (as opposed to being allowed to access content with either group permissions OR term permissions).

But, we won't necessarily want to use this same logic with every other Access Control module we wish to add. For example, if I wanted to add the ACL module, I'd want logic more like this:

Because of the nature of ACL, we really can't include it with "AND" logic. And, if we include it with "OR" logic, we wouldn't need to give every node that it doesn't care about a grant.

Let's add my module: OG User Roles. It only assigns grants based upon a user's OG and TAC status. Trying to write grants for all nodes it doesn't care about, on top of the ones OG and TAC don't care about, would be insanely inefficient. However, this logic would work perfectly:

So, it would seem to me that creating a way to add a module's node grants either as AND or OR would go a long way towards solving the logic problem and standardizing a potentially useful core feature. My two cents worth.

I remain skeptical about this patch. Perhaps because I'm skeptical by nature; or perhaps I don't understand the patch properly. But it seems to me that with this patch...

1. All node_access modules must do slightly more work. They all need to be updated.
2. More records are written to the node_access table, even when only one node_access module is enabled.
3. The view-time queries become more complex and probably slower.

So I'm suggesting an alternative. Attached is a patch to node_access_acquire_grants() (node.module 5.x - I don't have a CVS HEAD install handy, but really its just the idea that matters), which introduces hook_node_access_grants_alter(). This hook would be called when nodes are saved/updated. The hook can change the grants before they are written to the node_access table. So a module implementing this is potentially unlimited in the access schemes that may be configured.

For example, the Domain Access module could implement hook_node_access_grants_alter to remove any grants from og that don't have a corresponding grants from Domain Access (and vice versa), effectively introducing AND logic. Or, a third module could be written to be very configurable, ANDing any two (or more) node access realms together.

I see this option having advantages...

1. Anyone who understands hook_form_alter() will grasp hook_node_access_grants_alter().
2. This places the burden of extra logic at node save time, rather than node view time.
3. This is flexible and may solve other people's access problem we haven't even thought of.

Thoughts?

Attachment	Size
node_access_grants_alter.diff	1.13 KB

I _really_ like using grants_alter() -- both at save and at load time (Domain Access uses this trick). And I would love to see that element of the patch go forward separately.

However, your first two points are not correct.

1. All node_access modules must do slightly more work. They all need to be updated.

This is not the case. The patch does not alter existing behavior. Modules only need to be changed if they wish to interact using AND syntax of their own. However, modules like OG that are selective about their grants may need to write grants for all nodes.

2. More records are written to the node_access table, even when only one node_access module is enabled.

Also not necessarily true. This is true if multiple node access modules are enabled. But it is not true if only one is present. If only one node_access module is present, the current behavior is unchanged.

OK, I've got code working which will organize my query like this:

This is what I have to send from hook_node_grants() in each module:

The modified node_access_grants_sql() is below, but my question is: How do I structure the SQL so that it works with AND and OR logic in the way I'm trying to achieve?

Here is a sample SQL returned by my new code below which attempts this logical structure:

What I really want to happen is for it to require ((all or OG) AND (tac)) OR (ogr). That's not what's happening.

I've grouped it so it's easier to read.

Problem is, it's returning an og_subscriber grant when it shouldn't be returning any. That means that the (all OR og) AND (tac) portion of the SQL statement isn't working. I just can't figure out, logically, how to do it correctly.

Here's the code. Note the stuff I've commented out: I was trying various combinations of parenthesis to try and achieve the correct logic. The code itself works to separate out the AND and ORs correctly. It created the SQL you see above using the input from hook_node_grants() in TAC and OGR. I just don't know how to organize the SQL from there so that it executes in the manner I wish.

Help!

The problem here is that you can't hardcode the AND/OR logic. This would have to be made on a site-by-site basis.

Personally, I think trying to allow (this OR that) AND (foor OR bar) OR (baz) is too much to ask for in the next release.

OK, I think I've got it. Using this SQL format:

I was able to achieve this logic:

All I had to do in my node_access_grants_sql() code above was to put the parenthesis around the entire $grants_sql. I uncommented the following:

And voila! Testing this now, but it appears to be working. To recap, working means:

1. OG access control is respected.
2. TAC access control is respected, even within OG (that is, you must belong to group AND have TAC: Permissions to access content)
3. OGR grants respected. You can see nodes that OGR gives you access to, even if you are not in Group context (i.e., using the "Recent posts" link).

Woo-Hoo!

If this really works, the nice thing about it is that the code modifications to hook_node_grants in the affected modules could be replaced by code in node_access_grants_sql that would bring in the required elements from variables set using a UI. That would mean that, if the multinode_access_patch is released as part of Drupal core, NO code modifications or further patches would be required to use it. Users would simply designate which installed access control modules were to be used, determine the group, logic (AND or OR) and order (Weight) of each, and they'll be good to go.

Will report back on results of my unit testing.

I'm going to have to get my hands dirty with some code to see if AND then OR works instead.

Just wrote some very quick and dirty code, but this module ANDs together realms in a way that works with 5.x and 6.x, though is probably very slow.

Poorly documented, but I'll get around to explaining it later. And there is no user interface for defining what to AND together.

Attachment	Size
test1.install.txt	873 bytes
test1.module.txt	4.47 KB
test1.info_.txt	25 bytes

OK. I've been testing out my code for the past couple weeks and it appears to be holding strong. Have not tried it with DA. Shall I pursue this here?

To reinterate, I wanted to achieve this logic:

And I did so with the modified multinode_access code I've posted here. I would assume that the following would work as well:

What's the next step?

A proper patch that updates where we stand.

Here it is. This patch should be ran against 5.7 node.module. It assumes that you have modified the hook_node_grants() function of each module you intend to AND and/or OR as follows:
For these modules with this logic:

we make these modifications to hook_node_grants()

The 'group' is the tag for the module.
The 'logic' is 'AND' or 'OR' depending on if we want the module locically ANDed or ORed.
The 'weight' is the order in which the module should be ANDed or ORed. Can't use numbers here or else they will be confused as actual grants.

The only other change I think was needed was this one where we need to create grants for anonymous user permissions when integrating with OG: http://drupal.org/node/234087

Would really like to know how this works out with others. So far, it's been working as advertised in my environment.

Attachment	Size
multinode_access.ogr_.patch	8.11 KB

I like the 'logic' and 'weight' concepts, but as written I would reject the implementation in the contrib modules.

You cannot hard code the AND or OR logic -- those choice will vary from site to site. These would have to be variable settings for the various modules or an overall logic/weighting page for configuring interactions. See the implementation in domain_node_grants().

http://therickards.com/api/function/domain_node_grants/Domain

Especially the lines:

These rules trigger the AND/OR logic for the module, but only IF multiple node access modules exist and the admin has enabled the AND logic feature.

What's needed is a user interface to provide this piece of the puzzle in contrib modules:

The larger question is: Does the logic of the core patch deliver the ability to create these grant logic options?:

If so, creating an interface should be pretty easy. What do you think?

I agree with those two points and need to test the revised patch.

A little help, please.

With respect to the UI, I know what needs to be done, just struggling with how to do it. Need a way for users to select:

1. Module
2. Realm
3. Realm Group
4. Realm Logic
5. Realm Weight
6. Realm Check

Then submit this info for as many modules as they wish to add. Can you point me to a module interface which has something like this that I could use as a model?

Secondly, I thought it would be great to not have to modify any contrib modules for this. The key, then, would be to get the additional info in here:

But, how?

Perhaps the new (D6) blocks sorting page.

Thanks. Exactly what I needed. Working on it.

Attachment	Size
multinode_ui.jpg	27.15 KB

Attached is the new patch. It looks for a table "multinode_access". If it doesn't find it, nothing happens. If it finds it and nothings in it, nothing happens. If it's found and it's configured correctly, magic -- multinode access with no contrib module code modifications required!

My idea is that there is a common table so you could write an interface ui for DA (if it works with DA). User's who aren't using DA but are using OGR would use my ui. Module's which don't use either could create their own ui. If someone has both DA and OGR, no matter -- the ui's read and configure the same data in the same way.

Here is the code I use in OGR to to create a settings form used to configure the multinode_access table: