In case, when a node belongs to more than one categories,
the "node_access" right is defined only by the LAST TERM (with greatest term id)
instead of having the rights for ANY of the terms used.

(This is a problem when creating, changing nodes, and also when "making housekeeping" on settings page "/?q=admin/settings/taxonomy_access" by disable/enable taxonmy_module settings)

I made a correction to "_nodeapi" hook and to function "_refresh_node_access_table()".

In the uploaded patch, view/update/delete rights of the node are given to the user roles based on the rights of ANY of its categories;
(instead of only the right of the last category - w/ greatest term id - of the node).

--- taxonomy_access.module	Tue Jun 14 04:09:12 2005
+++ taxonomy_access.module	Thu Sep 08 18:04:12 2005
@@ -443,11 +443,24 @@ function taxonomy_access_nodeapi(&$node,
       else {      
 	      // Query the term_access table to get the permissions for the new node's assigned category/categories
 	      // then make appropriate changes to the node_access table.
-	      $result = db_query("SELECT * from {term_access} where tid in ('" . implode("','",array_values($tids)) . "')");
+//	      $result = db_query("SELECT * from {term_access} where tid in ('" . implode("','",array_values($tids)) . "')");
+	      $result = db_query("SELECT * from {term_access} where tid in ('%s')", implode(",",array_values($tids)));
+	      $permissions = array();
 	      while($row = db_fetch_object($result)) {
-	        db_query("DELETE FROM {node_access} WHERE nid=%d AND gid=%d AND realm='term_access'", $node->nid, $row->rid);
-	      	db_query('INSERT INTO {node_access} (nid, gid, realm, grant_view, grant_update, grant_delete) VALUES (%d, %d, \'term_access\', %d, %d, %d)', $node->nid, $row->rid, $row->grant_view, $row->grant_update, $row->grant_delete);
+	  	$permissions[$row->tid][$row->rid] = array('grant_view' => $row->grant_view, 'grant_update' => $row->grant_update, 'grant_delete' => $row->grant_delete, 'grant_create' => $row->grant_create);
 	      }
+		$access = array();
+		foreach ($tids as $tid){
+			foreach ($permissions[$tid] as $rid=>$perm){
+				$access[$rid]['grant_view'] = ($access[$rid]['grant_view'] || $perm['grant_view']) ? 1 : 0;
+				$access[$rid]['grant_update'] = ($access[$rid]['grant_update'] || $perm['grant_update']) ? 1 : 0;
+				$access[$rid]['grant_delete'] = ($access[$rid]['grant_delete'] || $perm['grant_delete']) ? 1 : 0;
+			}
+		}
+		foreach ($access as $rid=>$value){
+			db_query("DELETE FROM {node_access} WHERE nid=%d AND gid=%d AND realm='term_access'", $node->nid, $rid);
+			db_query('INSERT INTO {node_access} (nid, gid, realm, grant_view, grant_update, grant_delete) VALUES (%d, %d, \'term_access\', %d, %d, %d)', $node->nid, $rid, $value['grant_view'], $value['grant_update'], $value['grant_delete']);
+		}
 	    }	    
 	    break;
   }
@@ -788,16 +801,27 @@ function _refresh_node_access_table()
 	// Load term_access table data into mem
 	// for nodes that are in a category.
 	$result = db_query("SELECT tn.nid, tn.tid FROM {term_node} as tn, {term_data} as td WHERE td.tid = tn.tid");
 	$category_nids = array();
+	$node_tids = array (); //array for storing term id-s to each node "$node_tids[$nid][]"
 	while($row = db_fetch_object($result)) {
- 		$nid = $row->nid;
- 		$tid = $row->tid;
  		$category_nids[] = $nid; // created now for use later on in code handling nodes without categories
- 		if(array_key_exists($tid, $permissions)) {
- 			foreach($permissions[$tid] as $key => $value) {
-			  db_query("DELETE FROM {node_access} WHERE nid=%d AND gid=%d AND realm='term_access'", $nid, $key);
-			  db_query('INSERT INTO {node_access} (nid, gid, realm, grant_view, grant_update, grant_delete) VALUES (%d, %d, \'term_access\', %d, %d, %d)', $nid, $key, $value['grant_view'], $value['grant_update'], $value['grant_delete']);
- 			}
+ 		if(array_key_exists($row->tid, $permissions)) {
+			$node_tids [$row->nid][] = $row->tid;
  		}
+	}
+	
+	foreach ($node_tids as $nid => $terms){
+		$access = array();
+		foreach ($terms as $tid){
+			foreach ($permissions[$tid] as $rid=>$perm){
+				$access[$rid]['grant_view'] = ($access[$rid]['grant_view'] || $perm['grant_view']) ? 1 : 0;
+				$access[$rid]['grant_update'] = ($access[$rid]['grant_update'] || $perm['grant_update']) ? 1 : 0;
+				$access[$rid]['grant_delete'] = ($access[$rid]['grant_delete'] || $perm['grant_delete']) ? 1 : 0;
+			}
+		}
+		foreach ($access as $rid=>$value){
+			db_query("DELETE FROM {node_access} WHERE nid=%d AND gid=%d AND realm='term_access'", $nid, $rid);
+			db_query('INSERT INTO {node_access} (nid, gid, realm, grant_view, grant_update, grant_delete) VALUES (%d, %d, \'term_access\', %d, %d, %d)', $nid, $rid, $value['grant_view'], $value['grant_update'], $value['grant_delete']);
+		}
 	}
 	
 	// Determine which nodes don't belong to a category
CommentFileSizeAuthor
#8 taxonomy_access_14.patch6.2 KBkeve
#5 taxonomy_access_13.patch6.2 KBkeve
#3 multicategory.patch3.68 KBmerlinofchaos
taxonomy_access_12.patch4.11 KBkeve

Comments

merlinofchaos’s picture

merlinofchaos commented

I wonder how many nodes you'd have to have in the system before this patch causes it to hit PHPs memory limit; keeping all node tids in memory before writing is suboptimal.

Also you can optimize the query during the _nodeapi insert by using group by and bit_or(grant_view).

(I found this bug on my system today and fixed it before bothering to look here to see if someone else already had)

keve’s picture

keve commented

Dear merlinofchaos,

I am not a professional programmer, but i will consider your suggestions :).

Why do not you share with us your bugfix?

merlinofchaos’s picture

merlinofchaos commented
StatusFileSize
new multicategory.patch3.68 KB

At the time I wrote it, I hadn't fully tested my fix and was not satisfied with it. I'm doing something kind of weird in order to avoid code duplication but I'm not sure it's a good idea.

I'm also not sure but I think they may have fixed this a completely different way in HEAD, giving a unique region name for every TID. I haven't looked at it closely enough to see if that's what's going on or if I'm just seeing things. Personally I don't like that fix, though, if that's what it is.

Finally, I don't think this is a complete solution. The problem here is that we do a basic or -- if anything gives access, access is granted. But that may not be entirely desirable. Though setting precedence and using ANDs would be even whackier.

Here is my patch. I'm not 100% sure this works in all cases, but it seems to work in all of my cases now, but I had a couple of bugs that made me leery.

shouchen’s picture

shouchen commented

Which version of taxonomy_access.module is this patch for? I tried patching

33494 Jun 13 22:09 taxonomy_access.module (from a recently-downloaded version of taxonomy_access-4.6.0.tar.gz)

and got this result:

File to patch: taxonomy_access.module
patching file taxonomy_access.module
Hunk #1 FAILED at 446.
Hunk #2 FAILED at 790.
2 out of 2 hunks FAILED -- saving rejects to file taxonomy_access.module.rej

Thanks,
Steve

keve’s picture

keve commented
StatusFileSize
new taxonomy_access_13.patch6.2 KB

Based on the suggestion and patch of "merlinofchaos", i rewritten my taxonomy_access_12.patch.

I optimized the sql query during the _nodeapi insert and during _refresh_node_access_table by using group by and bit_or(grant_view).

(I used the HEAD branch, but it is the same as 4.6.)

I await your comments.

shouchen’s picture

shouchen commented

What am I doing wrong? I just downloaded version 1.44 of taxonomy_access.module from CVS (HEAD) and tried to patch it...

-rw-r--r-- 1 root root 6635 Sep 27 20:51 taxonomy_access_13.patch
-rw-r--r-- 1 root root 34321 Sep 27 20:54 taxonomy_access.module
[root@fusion modules]# patch -p0 < taxonomy_access_13.patch
patching file taxonomy_access.module
Hunk #2 FAILED at 443.
Hunk #4 FAILED at 772.
2 out of 4 hunks FAILED -- saving rejects to file taxonomy_access.module.rej

Could you please help me figure out what I'm doing wrong... or post the patched module somewhere?

Thanks,
Steve

jpuster’s picture

jpuster commented

I've discovered a potential issue in this patch. Some tids are not being returned properly because the query input checker is escaping some of the quotes in the query. On my system, the SELECT query on line 466 produced output like this:

... where tid in ('4\',\'36\',\'49') group by rid

instead of this:

... where tid in ('4','36','49') group by rid

I was able to fix the issue by putting the implode statement inline instead of using the %s string replacement, so line 466 looks like this:

              $result = db_query("SELECT rid, bit_or(grant_view) as grant_view, bit_or(grant_update) as grant_update, bit_or(grant_delete) as grant_delete from {term_access} where tid in ('".implode("','",array_values($tids))."') group by rid");
keve’s picture

keve commented
StatusFileSize
new taxonomy_access_14.patch6.2 KB

Thanks. You are right.
Here is updated patch.

kurkuma’s picture

kurkuma commented

I think a confusing issue with the Taxonomy access module is that the permissons are calculated with OR instead of with AND policies. Let me explain: to determine if an user role has access to a node is necessary that the role has access to ANY terms affecting that node. Wouldn't it be more logical granting the access when the role has access to ALL the terms related to a certain node?
Otherwise the management of the permissons is very confusing.

kurkuma’s picture

kurkuma commented

After all the "problem" might be in the node access design...

keve’s picture

keve commented

Until now, nobody had a request for the AND policy.
You are right, this is also how node_access work.

With OR policy, you can have a vocab that contorlls permissions. (eg. terms: public, closed, etc.).
I think most people use it this way.

You can maybe hack the modul to put BIT_AND instead of BIT_OR.
Regards,
Keve.

rabello’s picture

rabello commented

Could you describe how you envisage the use of this module in the following situation:

1. One vocab controls access
2. One or more vocab's to control the content types

The problem is that I cannot say that a certain vocabulary should have access control or not. Would that be possible, I see no restrictions with the BIT_OR approach. But as long as all vocabularies have access control permissions and I'm obliged to set them (to either enable/disable), either you take the approach of the ternary possibility ("enable"/"disable"/"don't care") or some other flag has to be set, explaining to ignore the whole vocabulary. Would you agree?

waldirlieb’s picture

waldirlieb commented

I have updated the taxonomy_access.module file with taxonomy_access_14.patch (I actually updated it with taxonomy_access_3.module from the thread http://drupal.org/node/32778 that I suppose has the taxonomy_access_14.patch applied, right?) but it is still getting only the last term for the access control.
Any help? Anyone else has tested the taxonomy_access_14.patch ?
Tks.

keve’s picture

keve commented
Status: Needs review » Fixed

I commited this patch to Drupal 4.6

davidgibbens’s picture

davidgibbens commented
Title: Access is set by LAST term only! » Help with patch please

Apols for ignorance here.
I only installed taxonomy_access at the weekend. It's critical to how the site is structured (this private/public thing). Although I've installed a number of modules I'm seriously newbie here and have never applied a patch before. (Don't have command line powers to do so either - not yet sure whether I can get that). None of the modules I've ever installed have asked for a patch to be applied on a new install.
The install.txt on this module does say you need to apply a patch. But when I looked at the readme text it said the patch was for 4.5 version of the module. So I didn't bother to apply it (possibly ostrich behaviour there!) on the original install.
Now there's this newer version (two successive posts from Keve indicating bug-fixes committed to 4.6.0 version).
If the module has been updated to fix a bug, why can't I just install the latest version of the module, replacing the old with the new? Do I really need to "patch" rather than just "replace"? Can someone clarify please. It doesn't make sense to me but obviously there must be a reason. I've had a look at threads on drupal.org but they didn't addres this.
Thanks.

keve’s picture

keve commented

Sorry, I just took over the maintanance of the module. I did not corrected the install.txt.

You MUST patch the taxonomy.module for 4.6.3!

The patch provided in the tar.gz file is for 4.6.3
You can use command: (in the same directory where taxonomy.module is found)
patch < taxonomy.patch

Regards,
Keve.

davidgibbens’s picture

davidgibbens commented

Keve
Thanks. I can't patch myself but my webhosting firm are going to do it.
But could someone explain to me why you have to apply a patch rather than just having a patched module available? I'd like to deepen my understanding.

PS - you guys to a great job and Drupal is a great resource.

shouchen’s picture

shouchen commented

See http://drupal.org/node/37061 for the patched taxonomy.module file (as of today).

shouchen’s picture

shouchen commented
Title: Help with patch please » Access is set by LAST term only!
Status: Fixed » Closed (fixed)