Hmm, funny title I admit, I'll explain the problem (which is demostrated with the taxonomy_access module, TAC).
UserA has all permissions to terms via TAC.
UserB has a limited set of term permissions via TAC.
UserA saves a node and tags it with various terms, some of which are not available to UserB.
Sometime later, UserB edits the node and saves it. Terms that were applied by UserA which UserB did not have access to are deleted (and lost until UserA adds them back again).
Why this happens
The TAC uses hook_db_rewrite_sql() to ensure that, in the example, UserB is only presented with terms that they have access to (see taxonomy_form() -> taxonomy_get_tree). However, despite the fact that all taxonomy API functions honour hook_db_rewrite_sql() for this purpose, taxonomy_nodeapi() -> taxonomy_node_delete() doesn't.
The Patch
The supplied patch ensures that taxonomy_node_delete() also honours hook_db_rewrite_sql() by ensuring only terms returned by taxonomy_node_get_terms() can be deleted by the current user.
Versions
The patch above is for HEAD but following on there will be a patch for 4-7 as I believe this should be backported. That's for others to decide though.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | taxonomy_module_p2_r1.275.2.14 | 873 bytes | AjK |
| #6 | taxonomy_module_p2_r1.322 | 888 bytes | AjK |
| #1 | taxonomy.module_r1.275.2.14 | 970 bytes | AjK |
| taxonomy.module_r1.322 | 985 bytes | AjK |
Comments
Comment #1
AjK commentedand the patch for 4-7
Comment #2
AjK commentedyes, I made a spelling error in teh title, oh the joys of IRC ;)
Comment #3
Zen commentedJust a couple of thoughts:
Taxonomy deletes are called pretty much during every insert, update and delete. These include mass operations (a lot of nodes, terms etc.). With this in mind, the taxonomy_node_get_terms call becomes rather expensive, especially when the primary necessity for this functionality resides in a contrib module (albeit a useful one).
Perhaps an alternative solution could be to only delete associations to terms listed in the taxonomy form (for the insert and update nodeapi calls). Or if the current line of thought is to be pursued, perhaps the query can be split into two: a COUNT and a retrieve query.
Code style issues with the patch: No inline comments please. Capitalise sentences and terminate using a period. The word "drupal" is to always be capitalised. "drupal default" will make no sense to anybody in a year's time :) The $current_terms assignment can also reside on a separate line.
About all I can think of.
Cheers,
-K
Comment #4
Zen commentedAnd another alternative that I forgot to list is to insert any "inaccessible" terms into &$node->taxonomy in hook_nodeapi() (with appropriate weight adjustments).
Comment #5
AjK commented@Zen, the patch you suggest can be found here: http://drupal.org/node/92355
Note, I still think this is an awful workaround. Basically, the API supplied hook_db_rewrite_sql() specifically to allow modules (and core) to curtail returned results (in this case taxonomy terms) but then another part of Core says "well, we won't bother doing that as it's too expensive".
IMHO that's half an API (or broken as I prefer to call it). Yes, it may be expensive but doing one half because it's cheap and not bothering with the other half, is well, half an API. What more can I say.
As the above issue notes "it's a known bug". With taxonomy_access or Core? Well, I believe it's a fault in Core, it's Core that carelessly deletes all term data in the term_node table without consideration as to what it puts back after the delete operation.
But, if it's too expensive, then let it be. TAC (and other restriction/access module) should carry a health warning; Warning, don't do mass update operations with this module installed.
Comment #6
AjK commentedOn the off chance that others want to follow up on this then I have re-rolled teh patches following Zen's style notes (this is HEAD)
Comment #7
AjK commentedand this is for DRUPAL-4-7
Comment #8
neclimdulI tend to agree that this is not the appropriate change. The function description is "Remove associations of a node to its terms." If a node is deleted all taxonomy links should be removed regardless of whether the user has permission to the terms. Permission to the terms is out side of the taxonomy modules concept. Maybe I'm miss understanding the problem but thats the way I see it.
Comment #9
AjK commentedI understand and agree with that. But I'm not reffering to node deletion I'm talking about node updating
Understood. But, just to play devil's advocate, why then do all other taxonomy API functions bother using db_rewrite_sql() in order to offer the oppertunity to provide limitation (permissions style) if this is outside of the taxonomy concept?
I'm probably flogging a dead horse with this argument but it's worth putting it to bed properly imho.
Comment #10
AjK commentedI provided a workaround solution for TAC so I'm cleaning up my own issues and marking this "won't fix" like a well trained puppy