Move domain access rules?

The Nodeaccess module does something similar, I believe.

Here's the problem in practice. With OG enabled, the following node_access query is run:

SELECT COUNT(*) FROM node_access WHERE (nid = 0 OR nid = 22) AND ((gid = 0 AND realm = 'all') OR (gid = 0 AND realm = 'domain_site') OR (gid = 0 AND realm = 'domain_id') OR (gid = 0 AND realm = 'og_public')) AND grant_view >= 1

The domain module queries really need to be AND queries here, since the node should be viewable only if both OG and Domain Access approve the request.

The only exception to this rule is the domain_editor realm, which functions correctly:

SELECT COUNT(*) FROM node_access WHERE (nid = 0 OR nid = 22) AND ((gid = 0 AND realm = 'all') OR (gid = 0 AND realm = 'domain_site') OR (gid = 0 AND realm = 'domain_id')) AND grant_update >= 1

This grant should be an OR grant, as any editor role that gives you access should be respected.

To make this happen, we would need to hijack the node_access function a little.

http://api.drupal.org/api/function/node_access/5

node_access does not use db_rewrite_sql(), and the grant OR logic is hard coded in.

However, this clause would let us rewrite the rules.

  $access = module_invoke($module, 'access', $op, $node);
  if (!is_null($access)) {
    return $access;
  }

In domain_access($op, $node), we would replicate most of the lower portion of node_access, but rewrite the queries so that, if domain realms were involved, they became AND.

  if ($op != 'create' && $node->nid && $node->status) {
    $grants = array();
    foreach (node_access_grants($op) as $realm => $gids) {
      foreach ($gids as $gid) {
        $grants[] = "(gid = $gid AND realm = '$realm')";
      }
    }

    $grants_sql = '';
    if (count($grants)) {
      // THIS SECTION WOULD BE REWRITTEN IF GRANTS OTHER THAN domain_ were present.
      $grants_sql = 'AND ('. implode(' OR ', $grants) .')';
      //
    }
    // THIS WOULD BE REWRITTEN, IF NEEDED, WITH A JOIN TO THE {domain_access} table.
    $sql = "SELECT COUNT(*) FROM {node_access} WHERE (nid = 0 OR nid = %d) $grants_sql AND grant_$op >= 1";
    $result = db_query($sql, $node->nid);
    return (db_result($result));
  }

Alternately, we could choose to implement hook_access() just to check the DA rules. If DA returns TRUE, we might actually return NULL, which would let normal node access rules apply. If DA returned FALSE, then we would return FALSE and end the request.

I think we have to move in this direction. See http://drupal.org/node/191587 for example.

And this gives us the option of configuring DA as an "AND" or "OR" check for working with other access control modules.

Note: on certain paths, the view check is always TRUE -- e.g. search, mysite/UID/view, user/UID/track -- see http://drupal.org/node/189797#comment-626343. Though this would also be configurable.

More notes:

domain_access($op, $node) {
  switch ($op) {
    case 'create':
    // use domain_goto() to ensure proper domain? or ignore.

    case 'view':
    // IF FALSE, return FALSE.
    // IF TRUE, return NULL and let {node_access} check the node.

    case 'update'
    // return FALSE or TRUE 

    case 'delete'
    // return FALSE or TRUE
  }
}

In all cases check if the rule is AND or OR?

^%$@!

This clause in node_access ONLY works for node type modules. The full clause is:

  $module = node_get_types('module', $node);
  if ($module == 'node') {
    $module = 'node_content'; // Avoid function name collisions.
  }
  $access = module_invoke($module, 'access', $op, $node);
  if (!is_null($access)) {
    return $access;
  }

We're going to need to patch this function:

  if (!user_access('access content')) {
    return FALSE;
  }

  $domain_access = module_invoke('domain', 'access', $op, $node);
  if (!is_null($domain_access)) {
    return $domain_access;
  }

  // Can't use node_invoke(), because the access hook takes the $op parameter
  // before the $node parameter.
  $module = node_get_types('module', $node);
  if ($module == 'node') {
    $module = 'node_content'; // Avoid function name collisions.
  }
  $access = module_invoke($module, 'access', $op, $node);
  if (!is_null($access)) {
    return $access;
  }

I don't like it, but there it is.

After thinking about this and studying OG, I think this is the wrong direction.

The correct path is to patch the node access behavior in the function, rewriting this part of the node_access function:

  // If the module did not override the access rights, use those set in the
  // node_access table.
  if ($op != 'create' && $node->nid && $node->status) {
    $grants = array();
    foreach (node_access_grants($op) as $realm => $gids) {
      foreach ($gids as $gid) {
        $grants[] = "(gid = $gid AND realm = '$realm')";
      }
    }

    $grants_sql = '';
    if (count($grants)) {
      $grants_sql = 'AND ('. implode(' OR ', $grants) .')';
    }

    $sql = "SELECT COUNT(*) FROM {node_access} WHERE (nid = 0 OR nid = %d) $grants_sql AND grant_$op >= 1";
    $result = db_query($sql, $node->nid);
    return (db_result($result));
  }

Introduce a hook_access_logic that allows the AND and OR logic here to be specified on a per-module basis.

function hook_access_logic($op) {
  $logic = array();
   switch ($op) {
    // Written out in longhand to show the available options.
     case 'view':
       $logic['internal'] = 'OR';
       $logic['external'] = 'AND';
       break;
     case 'update':
       $logic['internal'] = 'OR';
       $logic['external'] = 'OR';
       break;
     case 'delete':
       $logic['internal'] = 'OR';
       $logic['external'] = 'OR';
       break;
   }
  return $logic;
}

Then implement this hook in Domain Access and submit as a patch for Drupal 7.

Of course we still have to deal with the $node_status issue mentioned in http://drupal.org/node/191587.

  // If the module did not override the access rights, use those set in the
  // node_access table.
  if ($op != 'create' && $node->nid && $node->status) {

So I suspect we have to do two things:

- Insert a statement that allows non-node modules to rewrite access rules.
- Make the node access table query more flexible.

That's the plan.

hook_access_logic() will make more sense when I write the handler inside of node_access() which processes the hook.

I'll write it up and submit against 7.x

Well, it turns out that this can be accomplished.....

But it requires SUBSELECT statements -- which requires MySQL 4.1+ or PgSQL 7.x+ -- and it may not work for more than two access control modules at a time....

OK. I found the answer -- and the reason that Drupal < 6 doesn't work this way.

You must use subqueries to properly AND the logic together.

Attached is an experimental patch for Drupal 5. It changes the node_access logic to use AND syntax. It should work for multiple node access modules.

Some notes:

- WILL NOT WORK on MySQL < 4.1
- Should be fine on all pgSQL.
- Introduces a change to hook_node_access_grants. In addition to passing an array of grant ids, three new optional items may be included in the array:

- $grants['group'] = 'NAME_OF_GROUP';
- $grants['required'] = TRUE / FALSE;
- $grants['check'] = TRUE / FALSE; -- Allows node access checks to be run for unpublished nodes. This is a use-case specific to the Domain Access workflow.

The behavior of this patch is as follows:

IF

- No 'group' elements or 'required' elements are set, all grants are OR linked together. This is the current node access behavior.
- For each unique 'group' element present, a new subquery is AND linked to the main query, with each grant in the group linked together as an OR clause.
- For each 'required' element present, a new subquery is AND linked to the main query.

Sample queries -- with OG and Domain Access --

SELECT COUNT(*) FROM node_access WHERE nid = 0 
AND ((realm = 'all' AND gid = 0) OR (realm = 'og_public' AND gid = 0)) 
AND nid IN (SELECT nid FROM node_access WHERE ((realm = 'domain_site' AND gid = 0) OR (realm = 'domain_id' AND gid = 0))) 
AND grant_view >= 1

SELECT COUNT(*) FROM node n INNER JOIN node_access na ON na.nid = n.nid WHERE 
(na.grant_view >= 1 
AND ((na.realm = 'all' AND na.gid = 0) OR (na.realm = 'og_public' AND na.gid = 0)) 
AND na.nid IN (SELECT na.nid FROM node_access na WHERE ((na.realm = 'domain_site' AND na.gid = 0) OR (na.realm = 'domain_id' AND na.gid = 0))) ) 
AND ( n.promote = 1 AND n.status = 1 )

SELECT DISTINCT(n.nid), n.sticky, n.created FROM node n INNER JOIN node_access na ON na.nid = n.nid WHERE (na.grant_view >= 1
AND ((na.realm = 'all' AND na.gid = 0) OR (na.realm = 'og_public' AND na.gid = 0)) 
AND na.nid IN (SELECT na.nid FROM node_access na WHERE ((na.realm = 'domain_site' AND na.gid = 0) OR (na.realm = 'domain_id' AND na.gid = 0))) ) 
AND ( n.promote = 1 AND n.status = 1 ) 
ORDER BY n.sticky DESC, n.created DESC LIMIT 0, 10

Next steps:

-- Port the patch to Drupal 6 and file as feature for Drupal 7.
-- Document the API changes.
-- Testing!

Moshe-

No official benchmark tests yet, but Devel module does not flag any of these queries as 'slow' (i.e. > 5 ms).

I think I'm going to pull the 'required' element out, since I think it is redundant. If developers want to make something required, it should be its own group.

Note, btw, that this can be implemented without any changes to OG.

Required has been removed as a flag. The change now is the addition of 'group' and 'check' to the hook_node_grants() array.

This version of the patch has been committed to HEAD.

moshe-

The subqueries posted above have been tested on pgSQL -- but only as isolated queries, not in a Drupal context.

The other question is: how does this approach compare to http://drupal.org/node/122173

I actually think we may need both. I can see additional use-cases for nodeapi('access') and started down that path before settling on this (as the above record shows).

But I think sub-selects and revamping hook_node_grants() is the best path.

This has been submitted as a core patch here: http://drupal.org/node/196922

In porting this for Drupal 7.x, I found this bug

Bugfix committed to HEAD.