Menu rewrite (for node menu items)

well, developers already have menu permissions in hook_menu (see the 'access' key). admins have menu administration using menu.module. how would menu permissions interact with these features?

please provide some use cases for this feature and propose a non confusing way to handle the above questions. then reset this to patch.

I think this patch is neccessary since we now have access permissions _and_ menu otf. So people are likely to create menu items for nodes and will get unwelcome surprises such as described in

http://drupal.org/node/57406

With this patch somethign like this could be implemented (untested):

/**
* Implementation of hook_db_rewrite_sql().
*/
function menu_db_rewrite_sql($query, $primary_table, $primary_field) {
if ($primary_field == 'mid' && !node_access_view_all_nodes()) {
db_query_temporary("SELECT mid, SUBSTRING_INDEX(path, '/', 2) AS nid FROM {menu} WHERE path RLIKE '^node/[0-9]+$'", NULL, 'temp_menu_node');
$return['join'] = 'INNER JOIN temp_menu_node tm ON m.mid = tm.mid INNER JOIN {node_access} na ON na.nid = tm.nid';
$return['where'] = _node_access_where_sql();
$return['distinct'] = 1;
return $return;
}
}

applied

What has been comitted to CVS so far does nothing (it simply invokes hook_db_rewrite_sql() - but the implementation of this hook hasn't been committed).

And if you paste chx's menu_db_rewrite_sql() function into menu.module, the result is that ALL menu links are hidden for ALL users, whenever any node access modules (such as OG) are enabled. This function also produces a "temporary table already exists" SQL error, when it's fired more than once.

It will be great when this works - I never imagined that the solution to http://drupal.org/node/57406 would be this simple! But it definitely does NOT work yet.

I have no intention to commit my code to menu.module. A contrib module can do so.

when this works, i imagine that it is only capable of blocking static menu items. IOW, it cannot block (!$may_cache) items. is that right?

yes, this is correct. The main idea was to fix the display of node related menu items and these are static.

Sorry, killes, didn't see that bit about your implementation of the hook being untested. I guess that's why it didn't work when I tried using it. ;-)

I will try improving this snippet, and getting it to work the way that it should. When the working version is ready, I think it belongs in the core node module. Node module defines the node access mechanism, so it should also be responsible for implementing that mechanism on relevant menu items.

I don't think the implementation belongs in a contrib module. The current behaviour is a core bug (I've just updated this issue to indicate as much), and it requires a core patch.

I guess another option would be to encourage all contrib modules that implement node access (i.e. OG, TAC, node_privacy_byrole, etc) to define the hook_db_rewrite_sql() implementation themselves. But this would just result in duplicate code for each node access module. And besides, it will probably lead to conflict further down the track, since it's likely that multiple node access modules will be able to coexist in the future (merlinofchaos and moshe have already made significant progress in this area).

Feel free to get the code core ready. The SQL is not ANSI, for example. It also will only be eligible for 4.8.

This patch takes the code that killes originally posted here, cleans it up, and implements it within node_db_rewrite_sql(). The visibility of node menu items is completely compliant with node access rules when this patch is applied.

Currently only MySQL compatible - Postgres compatible version is on its way.

sounds like a bug. if i have this wrong, please change category back.

Updated to include support for pgsql. I wasn't able to actually test this update with a node access module under pgsql, as the two such modules that I'm familiar with (OG and TAC) do not support pgsql (TAC gives errors, and OG doesn't have an install script for it).

However, when running without a node access module in pgsql, the updated patch seems to work fine.

@moshe: I think this is a bug, and I'm glad someone else does too.

feeling bold, i set this to critical. i don't really see how menu items can be allowed to disrespect node_access. Core offers a menu system and a node access system. It is wrong to expect a contrib module to maintain security between these.

i will try to test this one ASAP. i would appreciate help though.

As promised, this patch removes node menu items when the user has no access. Downgrading to Normal because security is not breached here. Ideally, the new queries here will be benchmarked against existing. I would expect this to get in for 4.7.1

i should have mentioned explicitly said that i tested (mysql only) and code reviewed the patch and is in good order

+ $not_rlike_op = 'NOT RLIKE';
+ if ($GLOBALS['db_type'] == 'pgsql') {
+ $not_rlike_op = '!~';
+ }
+

?

@killes: very sorry about the $not_rlike_op thing, but it's the best I could do to get the code pgsql-compliant. Same goes for the $path_nid code. The only way to do these queries efficiently is with 'advanced' DB functions that handle regex and string splitting, and there are no such functions that are the same across our supported DB systems.

I'd love to hear of a way to do the same thing, in one simple query, and in a cross-DB-compatible way, without using mysql- and pgsql-specific functions. But I doubt that such a way exists.

Small technicality: I mentioned 'DB functions' in previous post - in fact, 'NOT RLIKE' and '!~' are DB operators.

This works perfectly on 4.7rc3 using MySQL. Excellent patch and an absolute neccessity. This should get into core ASAP!!!!

the handling of pgsql ompatibility is simply not up to our standards. The usual way to introduce it is to have compatibility functions in the database.inc files.

Thanks for the pointer, killes. I have followed your advice, and introduced two new database.xxsql.inc functions. The first is db_sql_explode() (splits a DB field according to a delimiter, and returns the SQL for fetching one of those delimited fields), and the second is db_sql_preg_match() (returns SQL for matching a DB field based on a regular expr). I am reluctant to submit a patch with two new database-level functions so close to the final 4.7 release, but I feel that it is a price worth paying in order to get this critical bug fixed for 4.7.

As far as I could tell, this is the first case of the DB abstraction layer having functions that produce 'SQL fragments'. It's also the first case of the abstraction layer supporting non-ANSI-compliant, 'advanced function / operator' support (apart from things like db_next_id()). I don't know what other people will think about this idea, but personally I think it's the best practical solution for this problem, and I think that it sets a reasonable precedent for other problems that will require similar solutions.

I have given the functions the 'db_sql_' prefix, to indicate their special behaviour of returning SQL fragments. Any thoughts on this naming convention? I had also considered 'db_query_', and I'm open to other suggestions. I also named the functions according to their (roughly) equivalent built-in PHP functions, to try and make the names more familiar to my fellow coders.

I am a bit worried that this will lead to the database.xxsql.inc files getting cluttered with heaps of functions, providing support for all sorts of non-standard db-specific functions. That's why I still think my original approach (although hackish) might be more appropriate in this situation, because the DB functions and operators in question are not used very commonly.

Once again, this patch is fully tested on mysql, and is tested on a non-node-access-enabled pgsql install. I haven't tested mysqli, but I understand that it's simply an advanced version of mysql, so I assume it should work fine (mysqli code is identical to mysql code). Full pgsql testing is welcome.

Updated for latest HEAD. Testing and review is still needed for this patch, so please give it a whirl if you have some time.

Works in 4.7.0 with node_privacy_byrole and MySQL

Patch seems to work with Drupal CVS and Taxonomy Access. This is exactly what I need, will be doing some more testing an let you know how it goes.

After some testing I've found one issue so far, but this isn't really a big issue as it is as a result of doing something you wouldn't normally do.

Using Taxonomy I've created a category called Access with vocab Public and Private, and using Taxonomy Access I've allowed only registered users to see anything tagged as Private.

If you then create a menu with the following hierarchy:

• Private Item
  • Public Item

and show it in a block then when an unregistered user views the site, the public item is displayed in the Navigation block and not the block for that menu.

As I said, this is something you wouldn't normally do but just something I tried in the interests of testing.

Does this patch still apply to HEAD? This seems like an important fix to get into the next version.

To answer my own question, the patch still applies cleanly if it's adjusted to take the move of node.module into node/node.module into account.

diff vs. today's HEAD attached for convenience.

This is relevant to tac_lite which I maintain. Looking at the patch, I see that node_db_rewrite_sql handles the case when $primary_field is 'mid'. This makes me think I will not have to modify tac_lite at all, which is great. However two questions...

First, should the code to handle $primary_field=='mid' be in menu.module (i.e. menu_hook_db_rewrite_sql).

Second, I don't see in the patch anywhere where db_rewrite_sql() is called with $primary_field=='mid'. Am I missing something?

Looking forward to this getting checked in.

Hi I have a problem. I can not make it work. Could someone send link to the whole file with applied changes? I am doing something wrong :-(

Is this an issue in 4.6? if so, is someone willing to backport this patch? I'm not familiar enough with node_access in core to know if there is a difference between 4.6 and 4.7. however, it seems like menu items not respect node_access settings might be the root of this issue for taxonomy_access:

http://drupal.org/node/63112

thanks.

So I've applied the head patch, and it works great for it's purpose. I was wondering if anyone knew a way to modify it however to make it only show menus with the edit feature enabled. I want a menu for users who can edit, and view obviously. Otherwise they can view it using another means of access.

Change $op from view to update in this line

function _node_access_menu_where_sql($op = 'update', $menu_alias = 'm', $node_alias = 'n', $node_access_alias = 'na', $uid = NULL) {

I tried to apply the node_acces_2.patch file. I patched the files with cygwin for drupal 4.7.3. patch program said everything went ok. I uploaded the files to my server. the database.inc files and the node.module file. But now I get complete blank pages?
Is there anything I did wrong? It is the first time I tried to apply a patch, maybe i did something wrong. I have privacy by role allready activated and installed.

This is a lot of stuff to be adding to the database API for this patch. I'm not sure that is a good thing.

How does this affect performance. Those don't look like very speedy queries; how often are they executed?

Ok, it seems to work now. But: it works when menu items link to node. I have some menu items that link to taxonomy e.g. to taxonomy/term/13 How can I hide them? Thanks

Did this patch ever make it into 5.1? It currently isn't working for me. I am using TAC, clean URLs. I thought it might be URL Alias, but I tried it with direct access to a node url, and it still show up as a menu item for an anonymous user.

I attempted the last patch on my 5.1 installation and it patched fine. But seems to have zero effect. I wonder if there is any other module in the way.

This patch actually does appear to work for content created after you added a TAC module. And make sure your cache_menu is clear.

What is stopping this patch from making it into a release?

thanks all for this thread!

menu_node_access_HEAD_1.patch is working for me in 5.1 using self-updated nodeaccess-module. (other access modules fundamentally failed)

but the menu does NOT respect invisibility caused by "unpublished nodes" (node.status=0).

to hide unpublished nodes either, i extended pwolanin's menu_node_access_HEAD_1.patch for node.module at approx. line 2876 by

/**
 * Implementation of hook_db_rewrite_sql
 */
function node_db_rewrite_sql($query, $primary_table, $primary_field) {
  if (!node_access_view_all_nodes()) {  //die("dead");    
    if ($primary_field == 'nid') {
      $return['join'] = _node_access_join_sql($primary_table);
      $return['where'] = _node_access_where_sql();
      $return['distinct'] = 1;
      return $return;
    } else if ($primary_field == 'mid') {
      $return['join'] = _node_access_menu_join_sql($primary_table);
      $return['where'] = _node_access_menu_where_sql() ;
      $return['distinct'] = 1;
//<heimstein> 
      if($return['where']) {                        
        $return['where'].= " AND n.status > 0 ";    // just check status, too.
      }                                             
//</heimstein> 
      return $return;
    }
  }
}

and with all due respect guys, please: i don't think, this is a feature-request! inaccessible node MUST NOT appear in menus.

is this mess cleaned be the new menu-system in 6.0? or do i still have to apply a bunch of patchwork in future?

This should be fixed in Drupal 6.x, though there are plenty of menu-related patches that need review or help.

(oops I think what I wrote is unclear). This *is* fixed (or as chx says - is no longer relevant) for Drupal 6.x