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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

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.

agentrickard’s picture

Status: Active » Needs review
moshe weitzman’s picture

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.

moshe weitzman’s picture

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.

agentrickard’s picture

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

agentrickard’s picture

Well, crap. I get this error:

[ ken ] : ab -c1 -n500 http://localhost/drupal-cvs/index.php
This is ApacheBench, Version 1.3d <$Revision: 1.73 $> apache-1.3
Copyright (c) 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Copyright (c) 1998-2002 The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)

Test aborted after 10 failures

: Operation now in progress

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.

agentrickard’s picture

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.

moshe weitzman’s picture

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

Steven Jones’s picture

Subscribing. This sounds awesome.

Dave Cohen’s picture

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.

agentrickard’s picture

FileSize
14.27 KB

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.

neurojavi’s picture

subscribing

mlncn’s picture

Assigned: Unassigned » mlncn

+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

Steven Jones’s picture

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.

agentrickard’s picture

@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.

mlncn’s picture

Assigned: mlncn » Unassigned

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.

agentrickard’s picture

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.

jgraham’s picture

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;

function workflow_access_node_grants($account, $op) {
  $grants = array();
  $grants['workflow_access'] = array_keys($account->roles);
  $grants['workflow_access']['group'] = 'workflow_access';
  $grants['workflow_access_owner'] = array($account->uid);
  $grants['workflow_access_owner']['group'] = 'workflow_access_owner';

  return $grants;
}

This generates the following SQL;

AND nid IN 
(
  SELECT nid 
  FROM {node_access}  
  WHERE (
    (realm = 'workflow_access' AND gid = 2)
    )
) 
AND nid IN 
(
  SELECT nid FROM {node_access}
  WHERE (
    (realm = 'workflow_access_owner' AND gid = 4)
  )
)

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.

AND nid IN 
((
  SELECT nid 
  FROM {node_access}  
  WHERE (
    (realm = 'workflow_access' AND gid = 2)
    )
) 
OR nid IN 
(
  SELECT nid FROM {node_access}
  WHERE (
    (realm = 'workflow_access_owner' AND gid = 4)
  )
))

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!

agentrickard’s picture

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:

function workflow_access_node_grants($account, $op) {
  $grants = array();
  $grants['workflow_access'] = array_keys($account->roles);
  $grants['workflow_access']['group'] = 'workflow_access';
  $grants['workflow_access_owner'] = array($account->uid);
  $grants['workflow_access_owner']['group'] = 'workflow_access';

  return $grants;
}

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.

jgraham’s picture

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.

agentrickard’s picture

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.

sdboyer’s picture

subscribing

agentrickard’s picture

FileSize
6.09 KB

Revised patch for the current state of HEAD.

keith.smith’s picture

Status: Needs review » Needs work

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).

amitaibu’s picture

subscribe

agentrickard’s picture

The patch also needs some extra documentation, I think.

agentrickard’s picture

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.

Dave Cohen’s picture

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?

Steven Jones’s picture

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

agentrickard’s picture

@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.

Steven Jones’s picture

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.

Steven Jones’s picture

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.

agentrickard’s picture

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.

Steven Jones’s picture

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.

agentrickard’s picture

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.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
6.12 KB

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

SomebodySysop’s picture

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:

IF (all OR og) AND (domain_site OR domain_id)

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:

IF (all OR og) AND (tac)

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:

IF ((all OR og) AND (tac)) OR (acl)

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:

IF ((all OR og) AND (tac)) OR (acl) OR (ogr)

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.

Dave Cohen’s picture

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?

agentrickard’s picture

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.

SomebodySysop’s picture

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

if (all or OG) AND (tac) OR (ogr)

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

//// In TAC /////
function taxonomy_access_node_grants($user, $op) {
  $grants['term_access'] = array_keys(is_array($user->roles) ? $user->roles : array(1 => 'anonymous user'));
      $grants['term_access']['group'] = 'tac';
      $grants['term_access']['logic'] = 'AND';
      $grants['term_access']['weight'] = 'A';
  return $grants;
}
//// In OGR /////
function og_user_roles_node_grants($account, $op) {
  $array = array('ogr_access' => array());
  $result = db_query("SELECT ogr_id FROM {og_users_roles} WHERE uid = %d", $account->uid);
  while ($row = db_fetch_object($result)) {
    $array['ogr_access'][] = $row->ogr_id;
  }
    if (!empty($array['ogr_access'])) {
      $array['ogr_access']['group'] = 'ogr';
      $array['ogr_access']['logic'] = 'OR';
      $array['ogr_access']['weight'] = 'B';
   }
  return !empty($array['ogr_access']) ? $array : NULL;
}

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:

if (all or OG) AND (tac) OR (ogr)

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.

SELECT * FROM node_access 

WHERE (nid = 0 OR nid = 11) 

AND ((realm = 'all' AND gid = 0) OR (realm = 'og_public' AND gid = 0) OR (realm = 'og_subscriber' AND gid = 4)) 

AND (nid IN (SELECT nid FROM node_access WHERE ((realm = 'term_access' AND gid = 2) OR (realm = 'term_access' AND gid = 3) OR (realm = 'term_access' AND gid = 4) OR (realm = 'term_access' AND gid = 0))) 

OR nid IN (SELECT nid FROM node_access WHERE ((realm = 'ogr_access' AND gid = 2) OR (realm = 'ogr_access' AND gid = 3))) ) 

AND grant_view >= 1

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.

function node_access_grants_sql($op = 'view', $node_access_alias = NULL, $uid = NULL, $status = TRUE) {
  // Define the grants array.
  $grants = array();
  $template = array();
  // First, acquire all the grants as an array of rules.
  $rules = node_access_grants($op, $uid);

  // Prepare the table alias, if needed.
  if (!empty($node_access_alias)) {
    $alias = $node_access_alias .'.';
  }
  // It may be that only the default rule is active.  In that case, we can skip the
  // process below.
  if (count($rules) == 1 && key($rules) == 'all') {
    $grants_sql .= "AND (". $alias ."realm = 'all' AND ". $alias ."gid = 0)";    
  }
  else {
    // Process the grants into logical groupings.
    foreach ($rules as $realm => $rule) {
      // Set the status flag.
      $status += $rule['check'];  
      // Set the default clause group.  Then check for options.
      $group = 'all';
      if (!empty($rule['group'])) {
        $group = $rule['group'];
        $template[$group]['name'] = $rule['group'];
      }
      if (!empty($rule['logic'])) {
        $template[$group]['logic'] = $rule['logic'];
      }
      if (!empty($rule['weight'])) {
        $template[$group]['weight'] = $rule['weight'];
      }
      foreach ($rule as $gid) {
        // Grants ids must be numeric.
        if (is_numeric($gid)) {
          $grants[$group][$realm][] = $gid;
        }  
      }  
    }
    // In most cases, node status must be set to TRUE.  However, there are use-cases where 
    // we allow access to unpublished nodes.  So we check the status to see if we need to 
    // continue with the logic or end this IF statement.  
    if (!$status) {
      return FALSE;
    }
    
    $grants_sql = '';
	$subquery = array();
    $subqueries = 'no';

    // Run throught the $grants, if needed and generate the query SQL.
    if (count($grants)) {
      foreach ($grants as $group => $grant) {
        $clauses = array();
        foreach ($grant as $key => $value) {
          foreach ($value as $gid) {
            $clauses[] = "(". $alias ."realm = '$key' AND ". $alias ."gid = $gid)";
          }
        }
        if ($group == 'all') {
          $grants_sql .= 'AND ('. implode(' OR ', $clauses) .') ';            
        }
        else {
          // Required elements must go into a subquery.  Will not work prior to MySQL 4.1.x.
          // Set defaults.
          $subqueries = 'yes';
          $weight = 0;
		  $logic = 'AND';

          if ($template[$group]['weight']) {
            $weight = $template[$group]['weight'];
          } 

          if ($template[$group]['logic']) {
		    $logic = $template[$group]['logic'];
		  }

          if ($logic == 'OR') {
            $this_query = "OR ". $alias ."nid IN (SELECT ". $alias ."nid FROM {node_access} ". $node_access_alias ." WHERE ";
		  } else {
            $this_query = "AND ". $alias ."nid IN (SELECT ". $alias ."nid FROM {node_access} ". $node_access_alias ." WHERE ";
          }
          $this_query = $this_query .'('. implode(' OR ', $clauses) .')) ';
          
          $subquery[$this_query] = $weight;
        }
      }  
      if ($subqueries == 'yes') {
        // Get subqueries from subquery table;
        // Sort if first by weight.
		$subquery_phrase = '';
        asort($subquery);
		foreach ($subquery as $query=>$weight) {
          $subquery_phrase .= $query;
		}
        // Need to add parenthesis after first OR or AND and at the end of the query.
//        $pattern = '/^AND /';
//        $replacement = 'AND  (';
//        $subquery_phrase = preg_replace($pattern, $replacement, $subquery_phrase);
//        $pattern = '/^OR /';
//        $replacement = 'OR  (';
//        $subquery_phrase = preg_replace($pattern, $replacement, $subquery_phrase);
//        $subquery_phrase = $subquery_phrase . ')';            
        $grants_sql .= $subquery_phrase;
        // Place everything after "AND" within parenthesis
//        $pattern = '/^AND /';
//        $replacement = 'AND  (';
//        $grants_sql = preg_replace($pattern, $replacement, $grants_sql);
//        $grants_sql = $grants_sql . ')';
	  }
    }
  }  
  return $grants_sql;
}

Help!

agentrickard’s picture

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.

SomebodySysop’s picture

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

    SELECT COUNT(*) FROM node_access

    WHERE (nid = 0 OR nid = 11)

    AND (((realm = 'all' AND gid = 0) OR (realm = 'og_public' AND gid = 0) OR (realm = 'og_subscriber' AND gid = 4))

    AND nid IN (SELECT nid FROM node_access WHERE ((realm = 'term_access' AND gid = 2) OR (realm = 'term_access' AND gid = 4) OR (realm = 'term_access' AND gid = 3)))

    OR nid IN (SELECT nid FROM node_access WHERE ((realm = 'ogr_access' AND gid = 2) OR (realm = 'ogr_access' AND gid = 3)))

    AND grant_view >= 1

I was able to achieve this logic:

if (all OR og) AND (tac) OR (og)

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:

        // Place everything after "AND" within parenthesis
        $pattern = '/^AND /';
        $replacement = 'AND  (';
        $grants_sql = preg_replace($pattern, $replacement, $grants_sql);
        $grants_sql = $grants_sql . ')';

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.

Steven Jones’s picture

FileSize
873 bytes
4.47 KB
25 bytes

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.

SomebodySysop’s picture

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:

if (all OR og) AND (tac) OR (ogr)

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

if (all OR og) AND (tac) AND (DA) OR (ogr)

What's the next step?

agentrickard’s picture

A proper patch that updates where we stand.

SomebodySysop’s picture

FileSize
8.11 KB

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:

if (all OR og) AND (tac) OR (ogr)

we make these modifications to hook_node_grants()

//// In TAC /////
function taxonomy_access_node_grants($user, $op) {
  $grants['term_access'] = array_keys(is_array($user->roles) ? $user->roles : array(1 => 'anonymous user'));
      $grants['term_access']['group'] = 'tac';
      $grants['term_access']['logic'] = 'AND';
      $grants['term_access']['weight'] = 'A';
  return $grants;
}
//// In OGR /////
function og_user_roles_node_grants($account, $op) {
  $array = array('ogr_access' => array());
  $result = db_query("SELECT ogr_id FROM {og_users_roles} WHERE uid = %d", $account->uid);
  while ($row = db_fetch_object($result)) {
    $array['ogr_access'][] = $row->ogr_id;
  }
    if (!empty($array['ogr_access'])) {
      $array['ogr_access']['group'] = 'ogr';
      $array['ogr_access']['logic'] = 'OR';
      $array['ogr_access']['weight'] = 'B';
   }
  return !empty($array['ogr_access']) ? $array : NULL;
}

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.

agentrickard’s picture

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:

  // Do we need to use complex rules?
  $rules = variable_get('domain_access_rules', FALSE); 

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.

SomebodySysop’s picture

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

//// In TAC /////
function taxonomy_access_node_grants($user, $op) {
...
      $grants['term_access']['group'] = 'tac';
      $grants['term_access']['logic'] = 'AND';
      $grants['term_access']['weight'] = 'A';
...

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

if (all OR og) AND (tac) OR (ogr)
if (all OR og) AND (tac) AND (DA)
if (all OR og) AND (tac) AND (DA) or (ogr)

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

agentrickard’s picture

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

SomebodySysop’s picture

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:

function node_access_grants_sql($op = 'view', $node_access_alias = NULL, $uid = NULL, $status = TRUE) {
  // Define the grants array.
  $grants = array();
  $template = array();
  // First, acquire all the grants as an array of rules.
  $rules = node_access_grants($op, $uid);
...

But, how?

agentrickard’s picture

Perhaps the new (D6) blocks sorting page.

SomebodySysop’s picture

FileSize
27.15 KB

Thanks. Exactly what I needed. Working on it.

SomebodySysop’s picture

FileSize
8.73 KB
45.69 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:

/**
 * Generate multinode access UI form.
 */
function og_user_roles_multinode($theme = NULL) {
  global $theme_key, $custom_theme, $user;
  $uid = $user->uid;
     
  // Get whatever is stored in multinode table.
  $existing = array();
  $result = db_query("SELECT * FROM {multinode_access}");
  while ($row = db_fetch_object($result)) {
    $existing[$row->realm] = $row;
  }

  // Get all rules.  User doing this should have grants in all modules affected.
  $op = 'view';
  $rules = node_access_grants($op, $uid);
  ksort($rules);

  $form['#tree'] = TRUE;
  foreach ($rules as $realm => $grants) {
    $form[$realm]['checkbox'] = array('#type' => 'checkbox', '#default_value' => (isset($existing[$realm]->realm) ? 1 : 0)); 
    $form[$realm]['realm'] = array('#type' => 'textfield', '#size' => 25, '#disabled' => TRUE, '#value' => check_plain($realm), '#default_value' => $realm); 
    $form[$realm]['group'] = array('#type' => 'textfield', '#size' => 5, '#default_value' => (isset($existing[$realm]->groupname) ? $existing[$realm]->groupname : '') ); 
    $form[$realm]['logic'] = array('#type' => 'select', '#default_value' => (isset($existing[$realm]->logic) ? $existing[$realm]->logic : 'AND'), '#options' => array('AND' => 'AND', 'OR' => 'OR') ); 
    $form[$realm]['weight'] = array('#type' => 'select', '#default_value' => (isset($existing[$realm]->weight) ? $existing[$realm]->weight : 'A'), '#options' => array('A' => 'A', 'B' => 'B', 'C' => 'C', 'D' => 'D', 'E' => 'E', 'F' => 'F', 'G' => 'G') ); 
    $form[$realm]['check'] = array('#type' => 'select', '#default_value' => (isset($existing[$realm]->checkstatus) ? $existing[$realm]->checkstatus : '0'), '#options' => array('0' => '0', '1' => '1') ); 
  }

  $form['submit'] = array('#type' => 'submit', '#value' => t('Save changes'));

  return $form;

}

/**
 * Theme function to render the table for the og_user_roles_multinode 
 * Multinode access UI
 */
function theme_og_user_roles_multinode($form) {
  $output .= "\n<div id=\"og-roles-multinode-form\">\n";
  $output .= '<div id="desc">'. t('Here you can configure multinode access.  You can only modify values here if TAC/OG Integration is ON. If you turn TAC/OG Integration ON, then later wish to turn it off, you must first UNCHECK all items here and <strong>Save changes</strong> (otherwise, multinode access will continue).') ."</div>\n";
  $header = array(t(''), t('Realm'), t('Group'), t('Logic'), t('Weight'), t('Check'));
  $rows = array();

  foreach (element_children($form) as $i) {
    $block = &$form[$i];
    $rows[] = array(drupal_render($block['checkbox']), drupal_render($block['realm']),
        drupal_render($block['group']), drupal_render($block['logic']),
        drupal_render($block['weight']), drupal_render($block['check']),
      );
  }

  $output .= theme('table', $header, $rows, array('id' => 'og-roles-multinode-table'));

  // This is an OGR restriction: Do not show "submit" button if 
  // TAC/OG Integration not turned on.
  if (variable_get('og_user_roles_tac_og_value', 0) == 1) {
    $output .= drupal_render($form['submit']);
  } else {
    $output .= '<div id="comments">'. t('<strong>TAC/OG Integration is OFF</strong>.') ."</div>\n";
  }

  $output .= "</div>\n";
  $output .= drupal_render($form);
  return $output;
}

/**
 * Process multinode form submission.
 */
function og_user_roles_multinode_submit($form_id, $form_values) {
  // Erase all existing settings
  db_query ("DELETE FROM {multinode_access}");

  // Insert new settings
  foreach ($form_values as $block) {
    if ($block['checkbox'] == 1) {
      db_query("INSERT INTO {multinode_access} (realm, groupname, logic, weight, checkstatus) VALUES ('%s','%s','%s','%s',%d)", $block['realm'], $block['group'], $block['logic'], $block['weight'], $block['check']);
	}
  }
  drupal_set_message(t('The multinode access settings have been updated.'));
  cache_clear_all();
}

Of course, you'll have to modify your .install file to create the table. Here's update code from my module:

/**
 * Update 5291
 * Second database update for Drupal 5.x OGR Release 2.9
 * Create multinode_access table
 */
function og_user_roles_update_5291() {
  $items = array();
  $items[] = update_sql("CREATE TABLE {multinode_access} (
        realm varchar(30) NOT NULL default '',
        groupname varchar(10) NOT NULL default '',
        logic varchar(5) NOT NULL default '',
        weight varchar(5) NOT NULL default '',
        checkstatus int unsigned NOT NULL DEFAULT '0',
        module varchar(60) NOT NULL default '',
        PRIMARY KEY  (realm)
      ) /*!40100 DEFAULT CHARACTER SET utf8 */;");
  return $items;
}

So, now we need some testing beyond what I've done.

Thanks for all the help. Never thought I'd get this far!

agentrickard’s picture

You 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.

Susurrus’s picture

I think weight makes little sense being a letter. Is there a better choice of label for this?

SomebodySysop’s picture

I 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.

agentrickard’s picture

That'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?

SomebodySysop’s picture

That's not the patch -- that's the current core behavior, which was left unmolested.

So, does the "check" logic http://drupal.org/comment/reply/196922/835374#comment-678899 in the original patch actually work?

I'm not sure we need weights, either. What do they provide in this context?

My testing indicated that the order of the "AND" and "OR" placements was important.

(all) AND (tac) OR (ogr)

does not yield the same results as

(all) OR (ogr) AND (tac)
SomebodySysop’s picture

I think weight makes little sense being a letter. Is there a better choice of label for this?

To 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.

You are getting away from the core issue, which is improving the Drupal core node access system.

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.

If we need to install a new table, do so from within system.install.

OK.

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 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?

The implementation shown here is still too specific to certain modules. We need to go more abstract.

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.

agentrickard’s picture

Putting 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.

SomebodySysop’s picture

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.

What 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:

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.

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.

agentrickard’s picture

I 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.

SomebodySysop’s picture

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?

How 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."

The other problem is, Dries is often critical of adding additional settings to the interface.

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.

I'll dig in to the patch over the next week or so.

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.

agentrickard’s picture

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.

That's what I did with the original patch from #1.

SomebodySysop’s picture

Attached 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?

agentrickard’s picture

These patches are for D5. This issue targets Drupal 7.

Dave Cohen’s picture

I 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.

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.

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.

agentrickard’s picture

Dave-

As for a way around it, consider adding complexity when building the grants (node-save time) rather than at node view time.

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.

SomebodySysop’s picture

These patches are for D5. This issue targets Drupal 7.

This 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.

SomebodySysop’s picture

I 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.

No doubt.

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.

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.

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.

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.

SomebodySysop’s picture

Also, if you decide to drop the multinode_access patch and go with another process, doesn't that really become another issue?

agentrickard’s picture

Status: Needs review » Needs work

Yes. 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.

SomebodySysop’s picture

If 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:

This kind of complex logic is possible, but likely needs a UI to establish it, or we have to adjust the $grants array to pass the needed information.

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:

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.

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.

SomebodySysop’s picture

In case anyone still pursues this, I tested the following:

(all or og) AND (tac) OR (ogr) OR (caa) OR (car) OR (acl)

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.

agentrickard’s picture

Sorry. 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.

SomebodySysop’s picture

No 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.

agentrickard’s picture

In the D5 and D6 versions, I would put the installer in a contrib module. In the D7 version, I would patch system.install.

agentrickard’s picture

olet also reports the following error, which results from MySQL miscounting the returned rows from a subselect.

http://drupal.org/node/264092

4drian’s picture

This 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.

SomebodySysop’s picture

This 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.

agentrickard’s picture

The short answer is -- this feature requires modification of Drupal core.

Steven Jones’s picture

#43 provides the basis of a contrib only solution, albeit with no UI.

toniw’s picture

I 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 ?

SomebodySysop’s picture

Yes, 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.

agentrickard’s picture

And 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.

SomebodySysop’s picture

even the UI of multinode talks about TAC/OG integration

I'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

Susurrus’s picture

If 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.

SomebodySysop’s picture

The two separate patches should also be combined into 1. And there is code commented out, so that should be removed.

This I can do.

it'll need to be updated for HEAD

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?

Susurrus’s picture

It needs to work on Drupal 7, which is the current development version.

SomebodySysop’s picture

Understood. 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.

Susurrus’s picture

Information 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.

toniw’s picture

Great 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.

SomebodySysop’s picture

I'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:

IF (all OR og) AND (tac)

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:

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.

This sounds like a simple AND logic:

IF (all OR workflow) AND (tac)

In this case, all you need to do is check "term_access" realm with "AND" logic.

What I observe is that the Deny of TAC does not seem to be respected if WA says access is granted.

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:

SELECT COUNT(*) FROM {node_access} WHERE (nid = 0 OR nid = %d) AND ((realm = 'all' AND gid = 0) OR (realm = 'og_public' AND gid = 0) OR (realm = 'og_subscriber' AND gid = 1112)) AND nid IN (SELECT nid FROM {node_access} WHERE ((realm = 'domain_site' AND gid = 0) OR (realm = 'domain_id' AND gid = 2))) AND grant_view >= 1

to help explain why.

Hope this helps.

toniw’s picture

Ok. 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?

agentrickard’s picture

We should really open a new issue for the D5 patch.

Susurrus’s picture

I would open a new issue for the D7 patch, since there's already so much discussion about the D5 stuff in this issue.

toniw’s picture

Wo... 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.

mysql> select * from multinode_access;
+-----------------------+-----------+-------+--------+-------------+--------+
| realm                 | groupname | logic | weight | checkstatus | module |
+-----------------------+-----------+-------+--------+-------------+--------+
| term_access           | tac       | OR    | 1      |           0 |        |
| workflow_access       | wfa       | AND   | 2      |           0 |        |
| workflow_access_owner | wfo       | OR    | 3      |           0 |        |
+-----------------------+-----------+-------+--------+-------------+--------+

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)

agentrickard’s picture

@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.

SomebodySysop’s picture

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?

I 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?

SomebodySysop’s picture

@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!

toniw’s picture

Alas, 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?

toniw’s picture

@SomebodySysop:
in #59 above

To 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.

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:

SELECT COUNT(*) FROM node_access WHERE (nid = 0 OR nid = 8) AND (((realm = 'all' AND gid = 0)) OR nid IN (SELECT nid FROM node_access WHERE ((realm = 'term_access' AND gid = 1) OR (realm = 'term_access' AND gid = 1) OR (realm = 'term_access' AND gid = 0))) AND nid IN (SELECT nid FROM node_access WHERE ((realm = 'workflow_access' AND gid = 1) OR (realm = 'workflow_access' AND gid = 2) OR (realm = 'workflow_access' AND gid = 0))) OR nid IN (SELECT nid FROM node_access WHERE ((realm = 'workflow_access_owner' AND gid = 0) OR (realm = 'workflow_access_owner' AND gid = 3) OR (realm = 'workflow_access_owner' AND gid = 0))) ) AND grant_view >= 1

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:

That's not the patch -- that's the current core behavior

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.

SomebodySysop’s picture

Thanks 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?

toniw’s picture

Hmm, 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:

      foreach ($rule as $key => $val) {
        // Grants ids must be numeric.
        if (is_numeric($key)) {
          $grants[$group][$realm][] = $val;
        }  
      }  
SomebodySysop’s picture

Perhaps I'm missing something, but we've already got:

      foreach ($rule as $gid) {
        // Grants ids must be numeric.
        if (is_numeric($gid)) {
          $grants[$group][$realm][] = $gid;
        }  
      }  

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.

toniw’s picture

Basically, all we have to do is modify the above line to make sure the realm does NOT include check, group or weight.

That'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:

(all OR (term_access AND (workflow_access OR workflow_access_owner)))

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:

function node_access_grants_sql($op = 'view', $node_access_alias = NULL, $uid = NULL, $status = TRUE) {
  // First, acquire all the grants as an array of rules.
  $rules = node_access_grants($op, $uid);

  // Prepare the table alias, if needed.
  if (!empty($node_access_alias)) {
    $alias = $node_access_alias .'.';
  }

  $logic= 'AND (all OR (term_access AND (workflow_access OR workflow_access_owner)))';

  foreach ($rules as $realm => $gids) {
    $select= "(${alias}nid in (SELECT ${alias}nid FROM {node_access} $node_access_alias WHERE realm='$realm' AND gid in (" . join(',',$gids) . ")))";
    $logic= preg_replace("/\b$realm\b/",$select,$logic);
  }

  return $logic;
}

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.

SomebodySysop’s picture

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.

This 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.

That'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.

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.

SomebodySysop’s picture

OK, 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!

SomebodySysop’s picture

OK. I can't get the new patch uploaded. Here it is:

http://www.scbbs.com/system/files/node.module.multinode.nonog.patch

keith.smith’s picture

Three 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.

SomebodySysop’s picture

- 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.

1. 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

SomebodySysop’s picture

FileSize
48.84 KB

Need 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:

warning: Invalid argument supplied for foreach() in includes/menu.inc on line 258.

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:

//////////////////
/// hook_menu ////
//////////////////
  // multinode_access
  $items['admin/content/multinode'] = array(
    'title' => t('Multinode user interface'),
    'description' => t('Interface for configuring multiple node access.'),
    'page callback' => 'drupal_get_form',
    'page arguments' => array('node_multinode'),
    'access arguments' => user_access('administer nodes'),
    'type' => MENU_NORMAL_ITEM,
   );  

//////////////////////////////////////
////// Multinode Access Form ////////
/////////////////////////////////////

/**
 * Generate multinode access UI form.
 */
function node_multinode($theme = NULL) {
  global $theme_key, $custom_theme, $user;
  if (isset($uid)) {
    $account = user_load(array('uid' => $uid));
  } else {
    $account = $user;
    $uid = $user->uid; 
  }
  
  // Get whatever is stored in multinode table.
  $existing = array();
  $result = db_query("SELECT * FROM {multinode_access}");
  while ($row = db_fetch_object($result)) {
    $existing[$row->realm] = $row;
  }

  // Get all rule realms from node_access table.
  $result = db_query("SELECT DISTINCT realm FROM {node_access}");
   while ($row = db_fetch_object($result)) {
    $rules[] = $row->realm;
   }    

  // Get all rule realms from modules.  User doing this should have grants in all modules affected.
  $op = 'view';
  $mrules = node_access_grants($op, $account);
  foreach ($mrules as $realm => $grants) {
    $rules[] = $realm;
  }
  
  // Get only unique values
  $rules = array_unique($rules);
    
  // Sort the rule realms
  asort($rules);

  $form['#tree'] = TRUE;
  foreach ($rules as $realm) {
    $form[$realm]['checkbox'] = array('#type' => 'checkbox', '#default_value' => (isset($existing[$realm]->realm) ? 1 : 0)); 
    $form[$realm]['realm'] = array('#type' => 'textfield', '#size' => 25, '#disabled' => TRUE, '#value' => check_plain($realm), '#default_value' => $realm); 
    $form[$realm]['group'] = array('#type' => 'textfield', '#size' => 5, '#default_value' => (isset($existing[$realm]->groupname) ? $existing[$realm]->groupname : '') ); 
    $form[$realm]['logic'] = array('#type' => 'select', '#default_value' => (isset($existing[$realm]->logic) ? $existing[$realm]->logic : 'AND'), '#options' => array('AND' => 'AND', 'OR' => 'OR') ); 
    $form[$realm]['weight'] = array('#type' => 'select', '#default_value' => (isset($existing[$realm]->weight) ? $existing[$realm]->weight : '0'), '#options' => array('0' => '0', '1' => '1', '2' => '2', '3' => '3', '4' => '4', '5' => '5', '6' => '6', '7' => '7', '8' => '8', '9' => '9') ); 
    $form[$realm]['check'] = array('#type' => 'select', '#default_value' => (isset($existing[$realm]->checkstatus) ? $existing[$realm]->checkstatus : '0'), '#options' => array('0' => '0', '1' => '1') ); 
  }

  $form['submit'] = array('#type' => 'submit', '#value' => t('Save changes'));

  return $form;

}

/**
 * Theme function to render the table for the node_multinode 
 * Multinode access UI
 */
function theme_node_multinode($form) {
  $output .= "\n<div id=\"node-multinode-form\">\n";
  $output .= '<div id="desc">'. t('Here you can configure multinode access.  You can only modify values here if TAC/OG Integration is ON. If you turn TAC/OG Integration ON, then later wish to turn it off, you must first UNCHECK all items here and <strong>Save changes</strong> (otherwise, multinode access will continue).') ."</div>\n";
  $header = array(t(''), t('Realm'), t('Group'), t('Logic'), t('Weight'), t('Check'));
  $rows = array();

  foreach (element_children($form) as $i) {
    $block = &$form[$i];
    $rows[] = array(drupal_render($block['checkbox']), drupal_render($block['realm']),
        drupal_render($block['group']), drupal_render($block['logic']),
        drupal_render($block['weight']), drupal_render($block['check']),
      );
  }

  $output .= theme('table', $header, $rows, array('id' => 'node-multinode-table'));

  $output .= drupal_render($form['submit']);

  $output .= "</div>\n";
  $output .= drupal_render($form);
  return $output;
}

/**
 * Process multinode form submission.
 */
function node_multinode_submit($form, $form_state) {
  // Erase all existing settings
  db_query ("DELETE FROM {multinode_access}");

  // Insert new settings
  foreach ($form_state['values'] as $block) {
    if ($block['checkbox'] == 1) {
      db_query("INSERT INTO {multinode_access} (realm, groupname, logic, weight, checkstatus) VALUES ('%s','%s','%s','%s',%d)", $block['realm'], $block['group'], $block['logic'], $block['weight'], $block['check']);
	}
  }
  drupal_set_message(t('The multinode access settings have been updated.'));
  cache_clear_all();
}

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.

SomebodySysop’s picture

Never 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!

Dave Cohen’s picture

The 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.

SomebodySysop’s picture

Head 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).

    if (og_is_omitted_type($node->type) || (is_array($node->og_groups) && count($node->og_groups) === 0) || (!is_array($node->og_groups) && !og_is_group_type($node->type))) {
      $grants[] = array (
        'realm' => 'og_public',
        'gid' => 0,
        'grant_view' => 1,
        'grant_update' => 0,
        'grant_delete' => 0 );
      return $grants;
    } else {
      return;
    }

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?

Dave Cohen’s picture

Do 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:

$tac_lite_records =  tac_lite_node_access_records($node);
if (!$tac_lite_records || !count($tac_lite_records)) {
  // Create your fake records here...

}

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.

SomebodySysop’s picture

@Dave Cohen:

Do you mean you need to add code like this for each node_access module you want to support?

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.

By using gid=0, is your code allowing everyone to view every node?

Because I want to create a node_access logic which says

IF (all OR og) AND (tac_lite)

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.

You may have a problem in that tac_lite supports multiple schemes.

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.

agentrickard’s picture

Guys --

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.

SomebodySysop’s picture

Re: #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.

toniw’s picture

I 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.

mantyla’s picture

Sorry 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.

IF (all OR foo) AND bar

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.

SomebodySysop’s picture

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

You 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.

Oh - feel free to poke critique at my own proposal, and my inability to format code segments here:
http://drupal.org/node/286692.

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 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.

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.

drewish’s picture

subscribe

agentrickard’s picture

We 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.

gravit’s picture

Can 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?

agentrickard’s picture

See the domain_og module in http://drupal.org/node/234087

OGUR is not required.

SocialNicheGuru’s picture

I 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

agentrickard’s picture

Status: Needs work » Closed (fixed)

This 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

netw3rker’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Active

Is there going to be a patch for 6.16? the current one doesn't apply correctly.

agentrickard’s picture

Status: Active » Closed (fixed)

6.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.

excaliburst’s picture

Subscribing