This patch was developed to allow Organic Groups and Domain Access to work in tandem.
The code has been prepared against CVS HEAD (01-Dec-2007).
Unlike http://drupal.org/node/122173 and http://drupal.org/node/143075, this patch does not use hook_nodeapi().
Instead, it introduces a new optional element to the $grants array returned by hook_node_grants().
With this patch, Node Access modules can declare the following additional elements to the $grants array:
-- 'group' -- a string indicating the access group that the grants belongs to.
-- 'check -- a boolean TRUE/FALSE value that allows node_access checks to be run on unpublished nodes.
The behavior of the patch is as follows:
1) If neither 'group' nor 'check' is present in any $grants arrays, the node_access() function behaves as it does in D5 and D6.
2) If 'check' is TRUE, the node_access() function will run an access check for nodes with status == 0.
3) If 'group' is populated, then the node access SQL query is separated into multiple statements, using AND and subselects.
Take this example:
function domain_access_node_grants($op, $account) {
$grants = array();
$grants['domain'][] = 1;
$grants['domain']['group'] = 'domain';
return $grants;
}
The SQL generated will be:
node_access_view_all_nodes()
SELECT COUNT(*) FROM node_access WHERE nid = 0 AND (realm = 'all' AND gid = 0) AND nid IN (SELECT nid FROM node_access WHERE nid = 0 AND (realm = 'domain' AND gid = 1)) AND grant_view >= 1
If modules, like OG, do not declare a 'group' then the query is written using the current OR logic.
function og_sample_node_grants($op, $account) {
$grants = array();
$grants['og'][] = 1;
return $grants;
}
If only OG is present, the query is as follows:
node_access_view_all_nodes()
SELECT COUNT(*) FROM node_access WHERE nid = 0 AND ((realm = 'all' AND gid = 0) OR (realm = 'og' AND gid = 1)) AND grant_view >= 1
If _both_ DA and OG are present, the query is:
node_access_view_all_nodes()
SELECT COUNT(*) FROM node_access WHERE nid = 0 AND ((realm = 'all' AND gid = 0) OR (realm = 'og' AND gid = 1)) AND nid IN (SELECT nid FROM node_access WHERE nid = 0 AND (realm = 'domain' AND gid = 1)) AND grant_view >= 1
The use of subselects require MySQL 4.1 or higher, which is why, I presume, this behavior has not been adopted sooner.
The queries have been tested for acceptability on both MySQL 4.1.13 and pgSQL 8.5.x.
Comment | File | Size | Author |
---|---|---|---|
#117 | multinode_ui_tac_lite.jpg | 64.01 KB | SomebodySysop |
#117 | tac_lite_multinode.tgz | 736 bytes | SomebodySysop |
#112 | multinode_ui_error.jpg | 48.84 KB | SomebodySysop |
#109 | node.module.multinode.nonog_.patch | 13.57 KB | SomebodySysop |
#103 | node.module.multinode.nonog_.patch | 13.55 KB | SomebodySysop |
Comments
Comment #1
agentrickardFor 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.
Comment #2
agentrickardComment #3
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe. 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.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedWould 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.
Comment #5
agentrickardWorking 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
Comment #6
agentrickardWell, 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.
Comment #7
agentrickardTesters 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.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedDo we need some UI here? Won't some sites require AND and others OR?
Comment #9
Steven Jones CreditAttribution: Steven Jones commentedSubscribing. This sounds awesome.
Comment #10
Dave Cohen CreditAttribution: Dave Cohen commentedI'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.
Comment #11
agentrickardDave-
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.
Comment #12
neurojavi CreditAttribution: neurojavi commentedsubscribing
Comment #13
mlncn CreditAttribution: mlncn commented+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
Comment #14
Steven Jones CreditAttribution: Steven Jones commentedIt 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.
Comment #15
agentrickard@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.
Comment #16
mlncn CreditAttribution: mlncn commentedthere'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.
Comment #17
agentrickardIt 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.
Comment #18
jgraham CreditAttribution: jgraham commentedFirst 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!
Comment #19
agentrickardIf 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.
Comment #20
jgraham CreditAttribution: jgraham commentedagentrickard,
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.
Comment #21
agentrickardIt makes no change to the behavior that exists in 5.x or (as I understand) 6.x.
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.
Comment #22
sdboyer CreditAttribution: sdboyer commentedsubscribing
Comment #23
agentrickardRevised patch for the current state of HEAD.
Comment #24
keith.smith CreditAttribution: keith.smith commentedComment 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).
Comment #25
amitaibusubscribe
Comment #26
agentrickardThe patch also needs some extra documentation, I think.
Comment #27
agentrickardNote 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.
Comment #28
Dave Cohen CreditAttribution: Dave Cohen commentedIn 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?
Comment #29
Steven Jones CreditAttribution: Steven Jones commentedWhat if instead of doing OR in groups and AND between groups we did AND in groups and then OR between them?
Comment #30
agentrickard@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.
Comment #31
Steven Jones CreditAttribution: Steven Jones commentedRight, 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.
Comment #32
Steven Jones CreditAttribution: Steven Jones commentedOkay, 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.
Comment #33
agentrickardI 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.
Comment #34
Steven Jones CreditAttribution: Steven Jones commentedOkay, 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.
Comment #35
agentrickardTrue. 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.
Comment #36
agentrickardNew version of the patch, with corrections from #24 and a great assist from SomebodySysop over at http://drupal.org/node/243731
Comment #37
SomebodySysop CreditAttribution: SomebodySysop commentedI 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.
Comment #38
Dave Cohen CreditAttribution: Dave Cohen commentedI 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...
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...
Thoughts?
Comment #39
agentrickardI _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.
Comment #40
SomebodySysop CreditAttribution: SomebodySysop commentedOK, 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!
Comment #41
agentrickardThe 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.
Comment #42
SomebodySysop CreditAttribution: SomebodySysop commentedOK, 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.
Comment #43
Steven Jones CreditAttribution: Steven Jones commentedJust 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.
Comment #44
SomebodySysop CreditAttribution: SomebodySysop commentedOK. 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?
Comment #45
agentrickardA proper patch that updates where we stand.
Comment #46
SomebodySysop CreditAttribution: SomebodySysop commentedHere 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.
Comment #47
agentrickardI 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.
Comment #48
SomebodySysop CreditAttribution: SomebodySysop commentedWhat'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?
Comment #49
agentrickardI agree with those two points and need to test the revised patch.
Comment #50
SomebodySysop CreditAttribution: SomebodySysop commentedA 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?
Comment #51
agentrickardPerhaps the new (D6) blocks sorting page.
Comment #52
SomebodySysop CreditAttribution: SomebodySysop commentedThanks. Exactly what I needed. Working on it.
Comment #53
SomebodySysop CreditAttribution: SomebodySysop commentedAttached 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:
Of course, you'll have to modify your .install file to create the table. Here's update code from my module:
So, now we need some testing beyond what I've done.
Thanks for all the help. Never thought I'd get this far!
Comment #54
agentrickardYou are getting away from the core issue, which is improving the Drupal core node access system.
If we need to install a new table, do so from within system.install. If the settings form is dependent on other modules, then we need to implement proper hooks to let those modules define themselves with regard to the way core behaves.
The implementation shown here is still too specific to certain modules. We need to go more abstract.
Comment #55
Susurrus CreditAttribution: Susurrus commentedI think weight makes little sense being a letter. Is there a better choice of label for this?
Comment #56
SomebodySysop CreditAttribution: SomebodySysop commentedI would love to use a numeric weight, and did so originally. Ran into a hitch in my testing. The problem is that the patch, as originally written, will assume that *any* numeric item entered in the grants array is a permission. 0 is fine, but you run into definate problems with 1 and 2.
In all fairness to the original developer of this patch, it was not written assuming that there would be a need for a weight.
That said, I'm not sure how this plays out with the "check" value, which so far has always been "0". I haven't tested that.
Comment #57
agentrickardThat's not the patch -- that's the current core behavior, which was left unmolested. I'm not sure we need weights, either. What do they provide in this context?
Comment #58
SomebodySysop CreditAttribution: SomebodySysop commentedSo, does the "check" logic http://drupal.org/comment/reply/196922/835374#comment-678899 in the original patch actually work?
My testing indicated that the order of the "AND" and "OR" placements was important.
does not yield the same results as
Comment #59
SomebodySysop CreditAttribution: SomebodySysop commentedTo be honest, I didn't make any specific notes as to why I changed the weight from numeric to alpha. I just remember there was a problem. So, I'm switching back to numeric for now to test. Hopefully, the issue will come back up.
I don't think so. The patch I have submitted will allow you to define relationships between multiple access control systems, do it on the grant level, and do it via a fairly simple user interface. The only reliable tests I am able to do are with the access control systems I know best: OG, OGR and TAC. Could use some help.
OK.
The settings form is independent of all modules. It only requires the "multinode_access" table.
So then, my next question is: Where should the settings form go within the node.module? That is, where should it appear on a site under the "Admin" menu?
Well, Somebody's gotta test it!
Let me know where you think a multinode_access settings form should go in core and I'll roll the patch, along with the system.install update.
Comment #60
agentrickardPutting the table install in a contributed module made be think you were moving in a different direction. We're still onthe same page.
It should probably be a sub-menu item from http://example.com/?q=admin/content/node-settings
That is, Admin > Content > Post Settings
Perhaps the "reset node access" part could become a sub-menu item as well.
My concern, though, is the level of complexity being introduced may scuttle the patch and we wouldn't get any improvements in node_access for D7.
Comment #61
SomebodySysop CreditAttribution: SomebodySysop commentedWhat level of complexity?
The Drupal core node grants system is complex. Creating a multiple node access system to operate within that node grant system is complex. Creating a user interface that doesn't require contributed module modification to configure this multiple node access system is complex.
You think this is tough, try using hook_node_access_grants_alter() to accomplish the same thing. http://drupal.org/node/196922#comment-814752
Here are Dave Cohen's comments on the original patch:
These have been addressed:
1. No contrib modules need to be updated.
2. The new records are no longer written to the node_access table, but introduced during the node_access_grants_sql process
3. The view-time queries will become more complex, that's for sure. But, there is NO way around this given the nature of the core grants system. Some queries might run slower, that's possible. But I don't think by much, given that the multinode_access table will probably never contain more than 1 or 2 records for most users.
I spent a year trying to get TAC and OG working together. What I came up with, http://groups.drupal.org/node/3700, required not only the Extensible Node Access/Authorisation Capability patch: http://drupal.org/node/122173, but some heavy, heavy hook_db_rewrite_sql modifications. By comparison, the multinode_access patch is a walk in the park.
Of all efforts to date on this type of multinode accessibility, who has come up with a user interface that requires no coding (assuming capability is within Drupal core)?
I'm not saying that what I have come up with is the best approach possible. But, it's the beginning of a pretty elegant solution to a complicated problem.
Comment #62
agentrickardI would agree. The complexity has to do with the user interface for configuring the grants -- how do we explain these in a way that make sense to people?
The other problem is, Dries is often critical of adding additional settings to the interface.
I'll dig in to the patch over the next week or so. I think it's great work.
Comment #63
SomebodySysop CreditAttribution: SomebodySysop commentedHow do you explain hook_node_grants to someone in a way that makes sense? I find that the primary reason the existing Drupal api documentation is so useless (for learning how to do things) is because it contains no examples. I would start to explain it by showing examples of what setups should look like for the most common situations.
And, be honest. Tell people straight out: "If you don't understand what this is, don't touch it."
If you could accomplish this without an additional setting, it would have already been done a long time ago. Drupal's biggest problem is not it's capabilities, but it's interface.
Thanks. Could use the help.
It is my intent to release this to my OGR audience in the hope that some will implement it and comment on it and help shake loose any remaining bugs.
Comment #64
agentrickardThat's what I did with the original patch from #1.
Comment #65
SomebodySysop CreditAttribution: SomebodySysop commentedAttached is user interface for Drupal core. Patches required to node.module and system.install.
When I get time, I'm intend to test this with content_access and acl, which I think a lot of people would like to combine with OG, TAC, DA and OGR.
What has been your response with the original multinode_access patch?
Comment #66
agentrickardThese patches are for D5. This issue targets Drupal 7.
Comment #67
Dave Cohen CreditAttribution: Dave Cohen commentedI know you both have worked hard on this patch, with little help from anyone else. I've been critical all along, which has not been particularly fun for any of us. I hate to say it, but I think as more people study this patch there will be more criticism.
A number of developers have issues with the node_access system. And there are ideas to improve it or completely reimplement it. Those who want to reimplement will resist this patch. It adds new rules which would have to be supported in the reimplementation.
Using this approach, even if multinode_access contains only 2 records, the node_access table would have two rows for every node instead of 1. And new logic (AND or OR) would be introduced to every node query. My gut tells me this will slow things down a lot. It would be interesting to see some metrics. Performance alone could prevent this patch from being accepted.
As for a way around it, consider adding complexity when building the grants (node-save time) rather than at node view time.
Comment #68
agentrickardDave-
I would agree there, but it will involve rethinking the node_access system entirely.
I'm a little frustrated because the original patch was an _incremental_ improvement that helps extend the current system. We have now, I agree, gone far enough that the entire system would need a rethink.
Problem is, I doubt we have enough time and eyeballs to rethink the whole system, so I doubt that any improvements are going to get into D7.
Perhaps we should swerve off and push for a hook_grant_alter() patch instead -- something that would let an ACL-module style implementation really work.
Comment #69
SomebodySysop CreditAttribution: SomebodySysop commentedThis is the only Drupal version I'm able to test. The contrib modules I need aren't even available in D6, let along D7.
If someone wants to take the code I've contributed and modify for D7, they have my blessings.
Comment #70
SomebodySysop CreditAttribution: SomebodySysop commentedNo doubt.
My primary goal has been to come up with a better implementation of TAC/OG integration which required no patches to core or contrib modules. My thanks to agentrickard who came up with the original multinode_access patch which has taken me most of the way there.
Honestly, re-thinking the current node grant system is above my pay grade.
I will release the patch I have created (and tested) for my users with the usual "Buyer Beware" warning. Those who really need and want it will use it. Those who don't, won't. The core mechanism remains unaffected.
If someone comes up with something that works, and works better than the multinode_access patch, and doesn't require contrib module patches, and provides some sort of user interface for implementation, I'm all for looking at it.
Otherwise, I'm rolling with what I have.
Comment #71
SomebodySysop CreditAttribution: SomebodySysop commentedAlso, if you decide to drop the multinode_access patch and go with another process, doesn't that really become another issue?
Comment #72
agentrickardYes. But in a sense, you have hijacked this issue, since there is now no patch for D7 to review except for #36.
I suspect we need to close this one down and start over next week.
Comment #73
SomebodySysop CreditAttribution: SomebodySysop commentedIf I did hijack the issue, it was not my intent.
I originally posted my feature request elsewhere: http://drupal.org/node/248521. And I was told me to pursue it in this issue. I was also told:
So, this is what I worked on here.
My hope was to get this patch or something like it into Drupal core. That's why I've worked so mightly hard on it for the past couple weeks. As you might recall, I achieved my initial goal last month: http://drupal.org/node/243731, so the effort here was for the cause. I tried to help demonstrate that it worked in D5 so that it would at least theoretically be accepted as workable in D7.
But, I'm simply not able to roll a D7 patch that I can guarantee works.
Then, there's this quote:
Which I take to mean that the original patch is being abandoned altogether.
Please forgive me if I wasn't clear on the precise objectives here. I thought I was helping. Still willing to help if someone comes up with a better way to accomplish the goal.
Comment #74
SomebodySysop CreditAttribution: SomebodySysop commentedIn case anyone still pursues this, I tested the following:
And, it appears to work. I've attached a screenshot of the configuration. This was done using the multinode access ui only -- no additional coding was required. I also noted in the case of caa (content_access_author) and car (content_access_rid) that these actually don't have to be have included in the multinode_access table. You include them only if you want "OR" logic.
In the logic case above, I want tac and og rules always integrated to respect each other, except when overriden by ogr, caa, car and/or acl rules.
If I remove caa and car realms from the displayed table (uncheck them), then they will be integrated with tac and og -- that is, a user must have
(all or og or caa or car) AND (tac) OR (ogr) OR (acl)
.In testing this, I discovered that there are times when a realm is defined by hook_node_grants but doesn't show up in the node_access_grants() array. To fix this, I'm now also searching the node_access table for realms. The attached updated patch now reflects this.
Comment #75
agentrickardSorry. The "hijack" comment was out of frustration.
I thought we were all working on a patch for Drupal 7. Based on the work you have done so far, it is not accurate to say that contributing to core is above your pay grade.
I think we should distribute compatible D5 and D6 versions of the patch in contrib -- OGUR can supply the UI, Domain Access will use the proper arrays defined there.
The next task is to port those UI and table changes to D7 -- then we can run some benchmarks. It is actually pretty easy to set up fake node access in order to run performance tests.
Comment #76
SomebodySysop CreditAttribution: SomebodySysop commentedNo problems. I worked on maintaining a D6 code version of the D5 code. The D6 code will be tested and implemented as soon as OG D6 is ready. Although, I do want to give D5 users a bit of time to test this to make sure we haven't left anything out.
I will then be ready for a D7 version.
I've implemented the UI in the node.module.multinode.patch itself. The only thing that patch does NOT have are the install parameters. You can get that either by applying the system.install.multinode.patch or adding the code from it in DA.install.
Comment #77
agentrickardIn the D5 and D6 versions, I would put the installer in a contrib module. In the D7 version, I would patch system.install.
Comment #78
agentrickardolet also reports the following error, which results from MySQL miscounting the returned rows from a subselect.
http://drupal.org/node/264092
Comment #79
4drian CreditAttribution: 4drian commentedThis may not be the right place to post this but has there been any development towards wrapping this up into a contrib module for D5/D6 or does the nature of the patch make it impossible? If a contrib module were available, I imagine that maintainers of ACL-related modules would be more inclined to take advantage of the functionality where present. This would then mean any D7 ports of contrib modules would already be set up to utilise the functionality if it were to be put into D7 core.
I'm not a programmer so I may have got the wrong end of the stick here - please feel free to correct me if I have.
Comment #80
SomebodySysop CreditAttribution: SomebodySysop commentedThis functionality is currently available in two contrib modules: Domain Access and OG User Roles.
To get this functionality *without* using either of these modules, you would:
1. Read up on exactly how to use the Multinode Access UI. This issue thread talks about it in depth, but theorectically. I wrote some documentation on how to use it with OG/OGR/TAC/ACL/CA here: http://groups.drupal.org/node/5392. Ignore the OGR part, and it should get you started in the right direction. You should know exactly what realms you wish to configure and how.
2. Download and install the system.install patch http://drupal.org/files/issues/system.install.multinode.patch (discussed in #65, http://drupal.org/comment/reply/196922/870954#comment-836672)
which will create the multinode_access file as a system.module update.
3. Download and install the node.module.multinode.patch http://drupal.org/files/issues/node.module.multinode_0.patch (discussed in #74, http://drupal.org/comment/reply/196922/870954#comment-837900) which will create the Multinode Access User Interface: http://drupal.org/files/issues/OGRConfigureMultinodeUI_ACL.jpg.
Keep in mind that even after you install the patches, nothing changes in your node access until you start checking items in the Multinode Access UI.
Comment #81
agentrickardThe short answer is -- this feature requires modification of Drupal core.
Comment #82
Steven Jones CreditAttribution: Steven Jones commented#43 provides the basis of a contrib only solution, albeit with no UI.
Comment #83
toniw CreditAttribution: toniw commentedI would very much like to get this multinode_access thingie to work with just TAC and Workflow_Access without any OG-stuff (Drupal 5.7). Since all examples and tests above use OG and even the UI of multinode talks about TAC/OG integration I might be on the wrong track here trying to get it to work without OG. Should it at all be possible to get this to work with TAC and Workflow without OG ?
Comment #84
SomebodySysop CreditAttribution: SomebodySysop commentedYes, it should. The reason you see "OG" everywhere is because the patch was initially written to allow DA to work with OG. I further added modifications in using the patch to work with OG and TAC. It is only through our respective projects, Domain Access and OG User Roles, that the patch is distributed and supported. So, any instructions we provide assume that the user is wanting use our module and OG.
However, there is nothing in the patch code itself which restricts it to OG. With just the patches provided here for the node and system modules, you should be able to create node_access logic for any realms you wish.
Theoretically.
Comment #85
agentrickardAnd it would be great to test under those circumstances, too. But the code _should_ not care which modules you use.
If not, it's bad code.
Comment #86
SomebodySysop CreditAttribution: SomebodySysop commentedI'm glad you mentioned this and I re-read it. You're right. It does. And, it shouldn't.
Attached is the mutinode access patch with does not reference OG at all!
Note that you still need the system.install patch in order to create the multinode_access table:
http://drupal.org/files/issues/system.install.multinode.patch
Comment #87
Susurrus CreditAttribution: Susurrus commentedIf this has a chance of making it into core, it'll need to be updated for HEAD. The two separate patches should also be combined into 1. And there is code commented out, so that should be removed.
Comment #88
SomebodySysop CreditAttribution: SomebodySysop commentedThis I can do.
Not sure how I do this. I know the code works for 5.7. Is there a version 7 download available somewhere or do I just focus on making it work for 6.2?
Comment #89
Susurrus CreditAttribution: Susurrus commentedIt needs to work on Drupal 7, which is the current development version.
Comment #90
SomebodySysop CreditAttribution: SomebodySysop commentedUnderstood. Forgive my ignorance, but where do I get the current devlopment version. And where would I find info on code changes? Guess that's what I should have asked.
Comment #91
Susurrus CreditAttribution: Susurrus commentedInformation on changes between Drupal versions is here. Not that much has changed from 6.x to 7.x yet, so getting this in soon, it it will get in, will make things much easier.
You can get the dev version from CVS or http://drupal.org/node/156281.
Comment #92
toniw CreditAttribution: toniw commentedGreat to hear that it ought to work for my situation. Please forgive me to ignore the fact that this is a D7 topic, but I would like to stay with D5.7 for a while. D6 is lacking too much modules that I need to be usable as a production site, let alone D7.
So I have D5.7, TAC, Workflow and Workflow_Access installed on a test-system. I have just a couple of nodes and one taxonomy category with just a few terms. In TAC I have specified Ignore, Deny and Allow for the specific terms. In WA I have specified two states, one of them is set to be inaccessible to the anon user. If I test TAC on its own it works. The node having assigned the term that is set to Deny is inaccessible. If I test WA on its own it also works. The workflow state named secret is hidden. Then I add a tablespoon of magic of multinode-access and I am at a loss. I must say that I do not fully understand how I should configure the ANDs/ORs, weights and checks in the multinode-UI, but I think I pretty much tested every possible combination now. What I would like to happen that access to a node is granted if both TAC and WA allow access. If one of them says nay, that vote should win. What I observe is that the Deny of TAC does not seem to be respected if WA says access is granted.
Help is much appreciated here. I can give you access to my test-system if that would make things clearer.
Comment #93
SomebodySysop CreditAttribution: SomebodySysop commentedI'll be honest and say that I don't know what you should do because I don't know workflow or what you are trying to do that is beyond what normal node_access accomplishes for you.
That said, here is what I do know in terms of what works in a TAC/OG environment:
1. TAC should be configured to allow 'View' access to 'Uncategorized nodes' for all roles.
2. In order for OG access to work, we needed to assign an 'og_public' grant to all nodes that OG didn't care about. This is obviously not necessary with respect to OG in your case, but I don't know if it might not be necessary with respect to Workflow. See the discussion here http://drupal.org/node/234087, with particular attention to how we fixed it.
3. I belive that before any of us went monkeying around with the node_access, we knew exactly what node_access logic we needed and how it would be accomplised. For example, I require:
So, I had an idea of how the multinode access needed to be configured BEFORE I started configuring it. I think you've stated as much, but I put this here for others who might go trekking down this path.
That said, in your case:
This sounds like a simple AND logic:
In this case, all you need to do is check "term_access" realm with "AND" logic.
This definately sounds like whatever you've done, you've configured OR access.
4. You definately need to get the "devel.module" in order to see what node access SQL is being created by your multinode access configurations. That way, when you say it's not working, we have something like this:
to help explain why.
Hope this helps.
Comment #94
toniw CreditAttribution: toniw commentedOk. So I installed Devel and configured TAC to say either 'allow' or 'deny' explicitly to all terms. Sometimes I find myself in a situation in which it seems to work, but tweaking the settings here or there again completely breaks it...
Could someone please help me understand what the 'all' in
IF (all OR workflow) AND (tac)
is supposed to mean?
Looks to me I should get the same results if I wrote
IF (all OR tac) AND (workflow)
Right? But it does appear to be like that.
And what should I do with the 'all'-realm in the multi-nodeaccess UI ?
Since I am not a Drupal coder this is pretty complex stuff to me, but I think what might be biting me here is that Workflow_access could be answering 'don't care' on some access-questions, which is then regarded as 'no access' by the multi-nodeaccess stuff. Something like 'Ignore' in TAC. Could it be something like that?
Comment #95
agentrickardWe should really open a new issue for the D5 patch.
Comment #96
Susurrus CreditAttribution: Susurrus commentedI would open a new issue for the D7 patch, since there's already so much discussion about the D5 stuff in this issue.
Comment #97
toniw CreditAttribution: toniw commentedWo... I think I've got it working. Problem might have been that when enabling workflow_access you get a second realm for free: workflow_access_owner. I had been ignoring that one for simplicity sake, but maybe that was the one biting me. I now have this in multinode_access, and for all the tests I have done so far, it seems to work.
As far as I understand it, a user gets access to a node if both TAC AND WA grant access, or if he is the author (owner) of the node.
I will now try to expand this model to my real production problem and see how far I can get.
(If I should continue posting to another thread, please point me in the right direction)
Comment #98
agentrickard@Sussurus-
And so, as initiator of this patch, I should roll a new release to change my module documentation to point to the new issues because other people can't follow the standards?
No thanks.
Comment #99
SomebodySysop CreditAttribution: SomebodySysop commentedI apologize. I'm partly at fault here. I just felt we needed to encourage folks who actually are trying to use this patch. However, Sussurus is correct. This issue has gone way off track from from the original goal of a 7.x feature request.
Question is: Since this is not an official project of it's own, but a feature in two unrelated projects, where do we tell people to go for issues like the last one? Hopefully toniw in #97 has helped to prove that the patch can stand alone (this we want to encourage). But, where would we send users like this for support if they use neither DA nor OGR?
Comment #100
SomebodySysop CreditAttribution: SomebodySysop commented@toniw:
Based on what you said you wanted to happen and what you have configured, I don't understand why it's working. But hey, if it's working the way you want, that's what's most important. If you run into any further problems, again, I would encourage you to provide the node_access SQL which is provided by the Devel module in addition to the multinode_access table configuraton so we can see what the multinode_access patch is actually doing.
Thanks for sticking with it!
Comment #101
toniw CreditAttribution: toniw commentedAlas, further testing show that in more complicated situations it does not work the way I want it to do.
I will post any detail you want, but as I said before I am not looking for D7 support but D5.7
@agentrickard: what do you wish me to do? Go on posting my issues here, or somewhere else? If somewhere else: where?
Comment #102
toniw CreditAttribution: toniw commented@SomebodySysop:
in #59 above
There's my problem (at least one of them): The numeric weights end up being used as gids when building the sql. That's why I got this suspicious node_access sql:
The
(realm = 'workflow_access_owner' AND gid = 3)
is generated because the weightvalue of the workflow_access_owner rule is '3'. But the anonymous user who ran this query is in no way entitled to the gid=3.In #56 above you yourself also mentioned this behaviour. I'm suprised this code actually works for anybody (when using weights)...
@agenttrickard in #57 above:
If you were referring to the usage of the being-numeric of the elements of the array, that was not in the original core, but added by this patch.
Comment #103
SomebodySysop CreditAttribution: SomebodySysop commentedThanks for find this. I guess I should have followed my first mind. Here's a revised patch that uses letters for weights (like I did in the first place). Maybe I should call it "sort order" or something like that?
Comment #104
toniw CreditAttribution: toniw commentedHmm, reverting back to the alphabetic weights does not entirely fix the problem, as noted before also the check column is numeric and will lead to wrong usage of gids when used. Why not leave weight numeric and fix the problem like this:
Comment #105
SomebodySysop CreditAttribution: SomebodySysop commentedPerhaps I'm missing something, but we've already got:
I don't see how your proposal changes this.
Actually, I know how to fix it, and will get it done shortly. Basically, all we have to do is modify the above line to make sure the realm does NOT include check, group or weight.
Comment #106
toniw CreditAttribution: toniw commentedThat's just about what my code does. It only uses the elements of the hash that have numeric keys, which happen to be the gids that got stuffed into the orignal rules array.
But anyway, since I kept struggling with the logic and reasoning behind the struture of the multinode_access table, and never seemed to get the weight, group and logic right, I decided to try a different approach.
As said before, what I want is that access is granted if both TAC and at least one of the workflow rules say it's ok.
In the syntax that is used before something like (ALL) or (TAC and (WFA or WFO))
But wait! Why use the multinode_access table to specify this when we already have specified it like this? Taking this even a bit further: why using abbreviated group names when we can just use the realm name? So why not write it like such:
and then use that string as the base for generating the sql.
So I rewrote the node_access_grants_sql() function to be like this:
Please note, I am a perl-coder, not a PHP coder, nor a Drupal expert. I have not yet studied the Drupal code guidelines, so forgive me if I've written bad code. But in principle this works. And, yes, I know I have the logic hardcoded. Maybe a UI could store this string in a system variable or something like that.
I just think the general idea of specifying the logic of what I want the AND/OR and grouping to be is much more natural like this than using the multinode_access table/UI.
Comment #107
SomebodySysop CreditAttribution: SomebodySysop commentedThis may definately be true in your case. But, keep in mind that the multinode_access table/UI was designed to allow users to be able to do this with a UI that does not involve writing code. That's the goal in this issue queue.
I'm sorry. Perhaps it does. But, without additional context, I can't figure out what your code is doing. Maybe agentrickard can sort it out.
Comment #108
SomebodySysop CreditAttribution: SomebodySysop commentedOK, finally got it.
For the rest of us, the attached patch includes the additional logic which prevents numeric values from "weight" and "check" from being included as gids.
Thank you, toniw!
Comment #109
SomebodySysop CreditAttribution: SomebodySysop commentedOK. I can't get the new patch uploaded. Here it is:
http://www.scbbs.com/system/files/node.module.multinode.nonog.patch
Comment #110
keith.smith CreditAttribution: keith.smith commentedThree notes:
- I can't get the patch to apply. I'm not exactly certain why.
- This patch introduces "node" and stuff like "multinode" into user-facing text. I'm not necessarily opposed to that when it makes sense but it does go against recent precedent.
- There are a number of double spaces between sentences in user-facing text and in particular, code comments.
Comment #111
SomebodySysop CreditAttribution: SomebodySysop commented1. Isn't clearly stated anywhere, but patch is currently for 5.x. I can't speak for anyone else, but that's the only version I've been in a position to test. Coming up on 7.x shortly.
2. If you have other suggestions, would love to hear them.
3. I'll make a note. Thanks
Comment #112
SomebodySysop CreditAttribution: SomebodySysop commentedNeed some help. Slowly working my way to a 7.x version of this patch. Currently trying to get the code working and tested in 6.x. Ran into big problem: I can't seem to figure out how to render the Multinode Access UI form.
When I click on the Multinode Access menu item (in Admin), I get this warning:
I see the realms listed, but the form isn't rendered at all.
Here's the code snippet, with the exception of the hook_menu code, it's more or less the same as in 5.x version of patch:
This is a general depiction of the form I'm trying to generate, modified for 6.x and specific to node module now:
http://drupal.org/files/issues/OGRConfigureMultinodeUI_ACL.jpg
I have attached a copy of what I'm getting.
Can someone who is way more familiar with 6.x than I am tell me what I'm doing wrong here? Thanks.
Comment #113
SomebodySysop CreditAttribution: SomebodySysop commentedNever mind. Got it fixed here: http://drupal.org/node/279254#comment-911340
So far, multinode access code appears to be working without any additional defined logic. That is, default node access appears to be working normally.
I need to test some sort of multinode access. Can't test TAC/OG (which I know how to test) because there is no 6.x version of TAC yet. Don't really know DA.
Any suggestions for some simple node access module that can be used to test the multiple node access logic patch in 6.x.
Once I do some testing here and it looks good, I can move forward to a 7.x patch.
Thanks for any help!
Comment #114
Dave Cohen CreditAttribution: Dave Cohen commentedThe HEAD of tac_lite runs on Drupal 6. There's no automated build yet, but if you check it out of CVS, you could test with it.
Comment #115
SomebodySysop CreditAttribution: SomebodySysop commentedHead version of tac_lite is here: http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/tac_lite/
Installed. Testing integration of OG and tac_lite.
When we integrate OG with DA, OG must create grants for nodes it doesn't care about. This is discussed at length here: http://drupal.org/node/234087
Here's an example of what we create in hook_node_access_records() to create grants for nodes OG doesn't care about (i.e., public nodes).
In TAC, there is an option in "Taxonomy Access Control: Permissions" under each role to create grants for Uncategorized nodes (i.e., nodes that TAC doesn't care about).
This is how we accomplish the AND logic we need to make the two or three node access systems work together. Node access systems that can be OR'd don't need this.
Because I want my node access to respect both OG and tac_lite, I need way to have tac_lite generate grants for "uncategorized" nodes. Any suggestions?
Comment #116
Dave Cohen CreditAttribution: Dave Cohen commentedDo you mean you need to add code like this for each node_access module you want to support?
By using gid=0, is your code allowing everyone to view every node? I don't understand it but I guess you know what you're doing.
I think for tac_lite the code would be something like:
You may have a problem in that tac_lite supports multiple schemes. This means it uses more than one realm. For example it could return records with the realm 'tac_lite', and also 'tac_lite_scheme_2', 'tac_lite_scheme_3' and so on, depending on how it is configured.
Comment #117
SomebodySysop CreditAttribution: SomebodySysop commented@Dave Cohen:
No. It depends upon the node_access module and what we are trying to accomplish. TAC automatically creates access records for "uncategorized" nodes (if option is selected). OG doesn't. In my environment, where I integrate TAC and OG, I don't need OG to create records for nodes it doesn't care about. In the Domain Access / OG environment, however, OG does need to do this.
Because I want to create a node_access logic which says
every node must have a tac_lite grant.
Trust me, I'd like a more efficient way to do it also, but this is how the node access system works.
Yes, I see that. Fortunately, the goal here is to get the multinode access patch working, so I'll have to tackle that one on another day.
I tried your suggestion. Works like a charm (for tac_lite realm). Thanks! Initial tests are looking very good.
I'll create a 6.x multinode access patch and post it here. Hopefully we can get somebody else to do some testing to make sure we're good. Then, it's on to 7.x.
Comment #118
agentrickardGuys --
Before you move any further, we really need to solve http://drupal.org/node/264092
The subselect syntax does not return the expected COUNT(*) value on MySQL, which breaks pagination. If that cannot be fixed, the patch is no good.
Comment #119
SomebodySysop CreditAttribution: SomebodySysop commentedRe: #118:
As far as I can tell, and correct me if I'm wrong, but that is either a core bug or a mysql bug or both.
The patch is working for me, so it's pretty good in that respect. But, not being a Drupal or Mysql expert, I think we need someone who can tell to take a look at it.
Comment #120
toniw CreditAttribution: toniw commentedI have learned a lot from the discussion above and from reviewing the supplied patches. However, this patch is just not right for me, so I took the liberty to create my own, for Drupal 5.7. For anyone interested, please see http://drupal.org/node/286178 for my Cooperating Node Access (CoNAc)-patch.
Comment #121
mantyla CreditAttribution: mantyla commentedSorry to be blunt, but I have to be: I think your approach is fundamentally flawed. I'm sure you could make it work and do something useful, but I'm against making it a part of Drupal core - the cons outweigh the pros.
You say you want this sort of logic. The first flaw is that if module bar needs AND syntax - meaning, that when bar says no it means no - it implies that when module foo says no, it must also mean no. This breaks an important functionality: currently you have two values for access, allow and undecided, defaults to deny. We need the ability that a module can reply with undecided, and using AND logic takes away that.
Worse yet, having module bar require AND logic changes the way denies from module foo are interpreted. This is a prime example of one module breaking another. Even if this functionality is made part of core, meaning it is technically core that breaks module foo, it is still bad. Module foo needs to know how its denies will be interpreted at the time it writes them into node_access.
Also, as you've noticed, this approach requires foo to have a record in node_access for every node. That table is already bloated, and this makes the problem worse. And having module foo write grants for nodes it doesn't care about is not a solution: since its denies will be interpreted as binding, it would need to allow everything for nodes it doesn't care about when another module uses AND logic.
For these reasons, I'm against this. I don't think these logical problems can be solved in any satisfactory way.
Oh - feel free to poke critique at my own proposal, and my inability to format code segments here:
http://drupal.org/node/286692.
Comment #122
SomebodySysop CreditAttribution: SomebodySysop commentedYou may be right. But, it is useful and I do have it working in a 5.x environment: http://groups.drupal.org/node/4026
Everything you say is bascially true. My only response is: This is the only way I've figured out how to do it (custom multiple module node access) and make it relatively simple to do. Remember that this patch is a re-write of the original patch that required you to manually modify the hook_access_node_grants() section of every module you wanted to particpate. My aim was to create a User Interface that didn't require any module modifications beyond the patch to the node.module.
My initial reaction is that this appears to be another variation on the Extensible Node Access/Authorisation Capability patch: http://drupal.org/node/122173. This was the approach I used for nearly a year, and had to abandon for the reasons that Dave Cohen suggested in his response to you. For the list operations, I ended up having to write some of the wildest hook_db_rewrite_sql code you've ever seen. Not good.
I haven't tried it, but this looks like a simpler variation on what we are trying to do here (let user's provide their own multiple node access logic). If it works and is easier to implement and maintain, more power to it.
Let me say that I come to multiple module node access having implemented my own version of it nearly 2 years ago: http://drupal.org/node/122712. I realized from this experience that the only way, in Drupal, to address this issue is from the node access table itself. That's when I began to pay attention to agentrickard's approach to the problem: http://drupal.org/node/191375.
I remember reading agentrickard and other's say that what really needs to change is the way Drupal core handles node access. Until that happens, we have to work within the framework we have. And, if we want mutiple modules to respect each others node access within all operations, i.e., "view", "create", "delete", "list", then it can only be done efficiently from the node access table.
Comment #123
drewish CreditAttribution: drewish commentedsubscribe
Comment #124
agentrickardWe going to split this issue based on the Node Access BoF at DrupalCON Szeged. This patch will be refactored due to the Database TNG patch.
Once new issues are created, this issue will be closed.
Comment #125
gravit CreditAttribution: gravit commentedCan anyone out there help me configure this patch to get things working with simple_access and og_access? It seems the way og_access returns its grants might be an issue as it doesn't return anything to the node_grants hook, unless the user has a subscription to a group... And then this patch is generating a query for all the grants in the node_grants array, but the og_access grants are not in that array, so the patch ignores og_access in the case that the user isn't a member of the group... Ugh.
So is it possible to use this patch without using og user roles at all?
Comment #126
agentrickardSee the domain_og module in http://drupal.org/node/234087
OGUR is not required.
Comment #127
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedI created a list of content permission modules that I have encountered with their weights on my system.
1. Can we create a comprehensive list?
2. How can they work together or should they. From your experience, what has been the worst and best combinations ?
3. what effect does module weight have on implementation of access? is it true that if access is already granted, it will not be restricted by another module that comes into play later?
Weight
0 Content Permissions 6.x-2.x-dev Set field-level permissions for CCK fields.
(admin determines)
0 Coherent Access 6.x-1.x-dev Provides user level node access for viewing and editing.
(node authors can define access to node content)
0 Content Access 6.x-1.0 Provides flexible content access control
(administrators have finer grained controll over access settings per content type and node)
0 UR-Node Access 6.x-1.0-beta9 Provides per node access control based on relationship to author
0 UR - Field Permissions Set field-level permissions for CCK fields of content_profile nodes.
0 Workflow access 6.x-1.1 Content access control based on workflows and roles.
0 ACL 6.x-1.0-beta4 Access control list API. Has no features on its own.
1 Organic groups access control HEAD Enable access control for private posts and private groups
(private or public groups and their posts would have the corresponding permissions)
3 OG Content Type Admin 6.x-1.0 Allows restriction of content type use based on group. (restrict content creation to a particular group)
10 Taxonomy Access Control Lite 6.x-1.x-dev Simple access control based on categories. (restrict who can click on a vocab term and see the results)
19 Content Field Privacy 6.x-1.0 Allow users to control per-field privacy settings. (on input forms and display)
There is Domain Access and Ubercart node access also
Comment #128
agentrickardThis patch is not going in to Drupal 7, as the new database layer and hook_query_alter() make it unnecessary.
Officially closing this issue.
@activelyOut
Why don't you repost those questions to the Access Control group: http://groups.drupal.org/access-control
Comment #129
netw3rker CreditAttribution: netw3rker commentedIs there going to be a patch for 6.16? the current one doesn't apply correctly.
Comment #130
agentrickard6.16??
#579696: multiple_node_access.patch doesn't need update for Drupal 6.16 ?
This issue is closed. Patch issues for multiple_node_access get filed against Domain Access.
Comment #131
excaliburst CreditAttribution: excaliburst commentedSubscribing