I've performed a major overhaul to the taxonomy_access module. After I encountered several problems with it, I took a look at the code and saw room for improvement. Here are the highlights of the changes:

-Now supports nodes that do not have categories.
-Integrated it better with nodeapi and simplified some of the code
-The problem with users being able to see restricted nodes through "recent posts" doesn't happen
-Inserts default permissions when new taxonomy term is created via _taxonomy hook
-Like node_privacy_byrole module, authors of nodes are automatically given permission to their edit own nodes
-Adds an "enable" feature like node_privacy_byrole module under module "settings" menu item
-Causes no known conflicts with node_privacy_byrole module installed

Notes:
-The new changes are thouroughly commented. See the file for information on the inner workings.
-doxygen style comments need to be added to new functions
-The drawback to this change is that in order to allow uncategorized nodes to be handled, properly, the "default permissions" functionality was removed. I think that feature was slightly buggy and confusing, however. I will be looking to improve that and reinstall that functionality back into the module.
-If node_privacy_byrole module is installed and then gets disabled, it will break this module. To fix it, simply enbale it again under "settings"

I'm attaching the entire module.

CommentFileSizeAuthor
#47 patch.rar576.96 KBKieran Huggins
#20 taxonomy_access_2.patch20.18 KBSteve Dondley
#11 taxonomy_access_1.patch20.18 KBSteve Dondley
#10 taxonomy_access.module_new20.18 KBSteve Dondley
#9 taxonomy_access_0.patch22.85 KBSteve Dondley
taxonomy_access_0.module20.69 KBSteve Dondley

Comments

Steve Dondley’s picture

Steve Dondley commented

Installation:
1) Print out current category permissions (recommended, see step 4)
2) Replace the old taxonomy_access.module with this version.
3) Under "settings" enable the module.
4) Now manually reset your category permissions under "users | configuration | category permissions". Hopefully you printed out your old permissions in Step 1 to make things easy for yourself.

Note: The revised will not cause a conflict with any database entries made by the older version of the module. Those older entries will just be ignored.

Steve Dondley’s picture

Steve Dondley commented

The last note in the original post above was vague about which module to enable. Here's clearer version of that note.

"If node_privacy_byrole module is installed and then gets disabled, it will break this module. To fix it, simply enbale the taxonomy access module again under "settings".

Bèr Kessels’s picture

Bèr Kessels commented

could you upload the file under antoher name please. .htaccess seems to forbid .module files.

Steve Dondley’s picture

Steve Dondley commented

Ber,

I think it's working now.

---Steve

Anonymous’s picture

(not verified) commented
Category: task » bug
Priority: Normal » Critical

Many places in code where table names aren't enclosed in curly braces (i.e. no table prefixing), although my setup hasn't resulted in any errors yet.

Filesize went from 12.8Kb to 21.1Kb. Is this because of extra commenting or much more code (sorry, didn't take the time to go over it in detail). Not being critical ... just curious why the big jump in size?

Anonymous’s picture

(not verified) commented

I see no point to the varaible taxonomy_access_enabled. I can't image why such a thing would be necessary and it's savagely non-intuitive for users. Why isn't it sufficient to just enable/disable the module from the "modules" admin page, like every other Drupal module?

Also, I'd greatly appreciate a patch file for code review purposes. Merely posting the entire module does not allow us to see nearly as readily what exactly was changed.

Steve Dondley’s picture

Steve Dondley commented

Read the comments in the node_privacy_role for a technical discussion on why this feature is necessary at least for the time being. The short answer is because as of now, Drupal has no way to run code when a module is enabled or disabled. Until the day that happens, this must be done manually.

The other option is to have adminstrators make mysql data entries by hand every time they install the module. That's certainly not reasonable.

Steve Dondley’s picture

Steve Dondley commented

Well, the biggest reason for the jump in the file size is simply that the module does more. There are about the same number of functions, perhaps even fewer because some extraneous functions were deleted from the old module. There were two key functions added that accounted for 6193 bytes. Their job is to handle entries to the node_access table.

The largest of the functions, _taxonomy_access_update_db(), 4741 bytes can definitely be pared down some. There are a lot of comments in it and there is a lot of code that can be factored out. The primary duty of this function is to handle the enabling and disabling of the module and do it properly.

Steve Dondley’s picture

Steve Dondley commented
StatusFileSize
new taxonomy_access_0.patch22.85 KB

I can post a patch file but it won't do much good. I tried it already. There was so much that changed and rearranged, it's not that helpful, at least not for me. But here it is.

Careful, this is a patch between my version and 4.5.0, not cvs version.

Steve Dondley’s picture

Steve Dondley commented
StatusFileSize
new taxonomy_access.module_new20.18 KB

Oh, I see what you are talking about now Ber. Here is the module again with a different extension.

Steve Dondley’s picture

Steve Dondley commented
StatusFileSize
new taxonomy_access_1.patch20.18 KB

That didn't work either. Let's try that again with ".patch" for an extension. Note, although it says "patch," this is actually the entire module.

TDobes’s picture

TDobes commented

Thank you for your work on this module. A few responses follow...
"Read the comments in the node_privacy_role for a technical discussion on why this feature is necessary at least for the time being. The short answer is because as of now, Drupal has no way to run code when a module is enabled or disabled. Until the day that happens, this must be done manually."

Could you please provide a link to to this discussion? I do not understand why it is necessary for the module to run code when it is disabled.

"The other option is to have adminstrators make mysql data entries by hand every time they install the module. That's certainly not reasonable."

Well, maybe it is and maybe it isn't (reasonable)... but the fact is that this is common practice among contributed Drupal modules. With only a couple of exceptions, every Drupal module I've ever met has required the admin to manually create the appropriate tables using a script before the module becomes functional. Until we see a well-thought-out, implemented-in-core install infrastructure, I'd strongly prefer that it stay this way.

After a quick look through the patch, it appears that what you meant to say is "there's no way for a module to execute code as it's being disabled/uninstalled." This is true. As a workaround, you could create a simple button on the settings page which users should click before disabling the module. (which will make the appropriate db changes) My suggestion, however, would be to create a mysql script which should be manually executed by the admin should they decide to disable the module, along with a similar script for installation. Hopefully, we'll end up with a installation system for 4.6, but in the meantime, there's no need to unnecessarily complicate this module.

While you're at it making changes, could you change the opening <? to a <?php, as is the standard throughout Drupal? Adding a // $Id$ line would also be much appreciated.

Thank you for submitting a patch rather than the entire module. I too feel it is important to review changes in this format.

Steve Dondley’s picture

Steve Dondley commented

Tdobes,

I refer you to the comments in the node_privacy_role module: http://cvs.drupal.org/viewcvs/contributions/modules/node_privacy_byrole/... Go to about the 5th paragraph down.

The module is not unnecessarily complicated. There is just one extra function that is used to display the "enable" and "disable" buttons on the settings page. When Drupal has the ability to run code when a module is disable/enabled, then that function will disappear.

It would actually be quite impossible for an administrator to do this with a mysql script. Here's why:

-User enables module and configures taxonomy permissions.
-User disables module for two months.
-User adds new categories, moves many nodes to new taxonomies, etc
-user renables module
-the improved module automatically compares old settings to current settings and make appropriate changes, including deleting entries for nodes that no longer exist, keeping the same permissions for old categories but deleting entires for categories that no longer exist, setting the appropriate permissions for new nodes and taxonomies that have been created, etc.

So, you see, there is some important housekeeping chores that need to be run when the module is enabled and all too difficult to do by hand.

thelmer’s picture

thelmer commented

Great work with the new version og taxonomy_access.

I have some term_access tuples like these:

tid rid perm
| 5 | 0 | 1 | 0 | 0 |
| 5 | 1 | 1 | 0 | 0 |
| 5 | 2 | 1 | 0 | 0 |
| 6 | 1 | 0 | 0 | 0 |
| 6 | 2 | 1 | 0 | 0 |
| 6 | 0 | 0 | 0 | 0 |

The difference is that only authorized users (rid=2) can access tid 6.

The taxonomy_access function implements the access check with the following code:

$sql = "SELECT tid from {term_access} where (rid=0 OR rid in ('"
. implode("','",array_keys($user_object->roles)) . "')) and (tid='$tid' OR tid=0) and grant_". $op . "=1";
if (db_result(db_query($sql)))
return TRUE;
else return FALSE;

The query for tid=5 and tid=6 returns:

+-----+
| tid |
+-----+
| 5 |
| 0 |
| 5 |
| 0 |
+-----+

+-----+
| tid |
+-----+
| 0 |
| 0 |
| 6 |
+-----+

.. and hence users with rid=2 is nor granted access as db_result(db_query($sql)) fails because the first row is 0.

I've added a quick fix by adding a "order by tid desc" to the end of the query. It seems to do the trick. There might be something I have missed.

Steve Dondley’s picture

Steve Dondley commented

Good catch. I didn't even look at that function.

I'm not sure if that is the right solution, however. I think it would be much safer to count the rows in $query using Drupal db_num_rows() function. If that returns 0, then you know that it's empty.

Try that out and let me know if it still works, please. It would be too much effort for me to try to duplicate your data and test it myself. Just change the last test to:

if (db_num_rows(db_query($sql)))
return TRUE;
else return FALSE;

Steve Dondley’s picture

Steve Dondley commented

Oh, I forgot to add why I think my way is safer. It might(?) be possible for there to be only one row returned with a single 0 in it.

thelmer’s picture

thelmer commented

Hi, I've tried it out and it works, but differently. With your solution users are granted access to the /taxonomy/term/x page and the category block item is visible regardsless if there is any nodes accessible in this term. With my solution med /taxonomy/term/x page is restricted and the category block is hidden.

Which one to choose could be configurable ? or are mine to far fetched ? I personally prefer that users don't see the menu item if they don't have access anyway.

Steve Dondley’s picture

Steve Dondley commented

I would prefer your way. I have to study that function, too. I don't even quite understand how that function works with the patch to the taxonomy.module. Thanks for letting me know about the bug.

dries’s picture

dries commented

I haven't tried the patch but looking at the code, your code's indentation is a little messed up. Try not to use tabs.

Looks like you guys are making good progress here. Who is maintaining this module nowadays?

Steve Dondley’s picture

Steve Dondley commented
StatusFileSize
new taxonomy_access_2.patch20.18 KB

Dries,

I think pyromanfo is maintaining. I haven't heard from him on this revision, though.

A setting in my text editor to replace tabs with spaces was left unchecked. Here's a fixed version.

Anonymous’s picture

(not verified) commented

I've been without internet access, looks like you've been busy :)

That said, there are several things about this patch that's going to keep it out of the official version until they're fixed. I like some of what you've done, but other parts really need to be changed.

First off, the good stuff
1) I like the you hook into nodeapi to keep the taxonomy_access_node_grants simpler. It takes more time in the content modification but keeps it much simpler when doing access checks. Plus, it actually uses node_access the way it was intended, instead of having it as a stub for checks to term_access. It should be a speed boost. It also takes care of the "recent posts" thing in a more efficient manner, some of the access checks in taxonomy.module can be removed once this is implemented.
2) The removal of the default permissions is probably a good idea because of the way node_access works, since the "default" setting overrides all other settings. I'm not quite as enthusiastic about this one because as you say, it removes functionality. Still, it probably makes alot more sense to the end user. Maybe some javascript to emulate "default" permissions in the admin page would make more sense to everyone.
3) Most of the places you cleaned up the code are very nice and keep it quite a bit simpler.

Now the bad news
1) You really should've started with the CVS version. When you're doing module development, you need to patch against the CVS version to keep track of the development efforts. Alot of the problems you set out to solve had already been solved in CVS, such as "recent posts", "update permissions for the author", enable/disable the module through a proper taxonomy.module patch and some code changes. Some of your updates are redundant. I can't accept a patch to the module if you don't patch against the CVS version, which like I said has been significantly enhanced.
2) Giving node authors update permissions to their own nodes is not only redundant, but possibly dangerous. You see, we're not node_privacy_byrole, we don't have permissions for individual nodes (from the administrators perspective). Therefore someone shouldn't be able to post a node to a given category if they don't already have update permissions to that category. So giving them update permissions for a node in a category when they already have update permissions for the category is redundant. Plus, if you have some sort of input method that doesn't check taxonomy_access and lets them post anyway, you're bypassing taxonomy_access's permissions and letting them update that node when the admin has explicitly said they shouldn't be able to. The CVS version solves this problem by having a proper taxonomy.module patch, which never allows the user to post to categories they don't have update permissions on in the first place. The whole "tax_access_author" bit is redundant from what I can see. Check out the CVS version and let me know what you think.
3) The "enable/disable" stuff was already in CVS. Fortunately it doesn't differ much from what you did, since it looks like we were both copying from node_privacy_byrole. The way yours works the "update_db" function needs some new code, which you already have, however I'd also want the "tax_access_author" stuff taken out for the reasons above.
4) Tiny thing, could you change all the "tax_access" stuff to "term_access"? It's just the way it works now, if I was going to accept this patch to the main version it'd be nice :)

If you could take care of the "bad news" above I'd be happy to update the cvs version with your changes. Basically if you could take the CVS version, add your streamlining changes to the code, the removal of "Default" and your nodeapi simplifications I'd add it to the main version no problem. Once you do that, I can even remove some of the checks in the taxonomy.module patch. I like most of what you've done, if you can check out what I've done in CVS and make your changes to that codebase I think you'll find we'll not only end up with a better module, but you'll have to do less work too :)

pyromanfo’s picture

pyromanfo commented

Whoops, the above was me, pyromanfo.

Steve Dondley’s picture

Steve Dondley commented

Yeah, no problem. I'm here to work with you. I'll get you a revised version by the end of this week or the middle of next week at the latest.

Just a few notes on what you said above:

1) It looks like I had started coding my version of the module just before you tackled the enable feature. I didn't know your changes were in the making.

2) I didn't even stop to think about the problem with the author permission. Like you pointed out, it was something I borrowed from node_privacy without thinking about the obvious security hole it creates.

3) Sorry for not working more directly with you from the start but this was something I needed up and running by last Monday.

4) The reason I didn't use the CVS version is because I wanted to be sure the code would work with version 4.5 for the site I'm using. My next change will be from the CVS.

I'll let you know when I've got something for you to look at.

Steve Dondley’s picture

Steve Dondley commented

pyromanfo,

I'm going to start work on the merge between your version and mine. I will do it piecemeal so we can agree on the changes.

Steve Dondley’s picture

Steve Dondley commented

After I got into it, I realized it will be much harder to do this change piecemeal. I'll do it all at once and get it done. Give me another 2 or 3 days, perhaps less.

pyromanfo’s picture

pyromanfo commented

No problem man, thanks for working on it :)

I'll be without internet till Sunday, just wanted to let you know in case you were wanting some feedback. I'll check when I get back Sunday night.

Steve Dondley’s picture

Steve Dondley commented

No problem. I'm almost done, actually. For some reason I thought it would be more time consuming than it was. But I need to test it now. Talk to you on Sunday.

joshuajabbour’s picture

joshuajabbour commented

I do want to chime in here, and bring up something I mentioned before, and pyromanfo mentioned here as well.

A javascript method to enable/disable columns would be great. When you get the changes merged, maybe I can tackle this, or if you want to, go ahead. :)

I had thoughts for the whole UI being updated...

My thoughts:
Basically, the way it is now is okay, and a good way to display the form if the user has Javascript disabled, but if they have JS, then easier editing would be nice...

1. At the top of each column (view, edit, delete) have an 'all terms' checkbox, that when toggled, will check/uncheck all terms displayed. (Huge timesaver)

2. Also maybe have a dropdown menu with the options: (all vocabs, vocab 1, vocab 2, etc). This way an admin could select which vocab to work on (I have a dozen vocabs in my setup, each with 50+ terms. Makes for a big page. Management is key for me)

Anyway, as I said, I can tackle this, but it'll be a couple of weeks...

Steve Dondley’s picture

Steve Dondley commented

I think the big thing to keep in mind about adding javascript code is that it should be "helper" code and not essential to the operation of the module. Javascript is like a heated car seat: nice to have a warm ass, but not essential for getting your ass from point A to B. It sounds like your javascript feature fits into the heated car seat category, which is good.

Even so, we should assume that the javascript code will not get incorporated into the core. The core developers may not want any javascript at all. So, I'd write the javascript in a way so that it could easily be removed from the module and easily added back in with a simple patch for those who want the javascript functionality.

But you are right, I think the UI could definitely could be improved. I wanted to change some things about it myself. Pyromanfo should be in on this conversation, too.

pyromanfo’s picture

pyromanfo commented

Since I commited all these changes to CVS, I'm marking this fixed.

sulleleven’s picture

sulleleven commented

is this at all related to http://drupal.org/node/10047 regarding point where users can see and post into categories they do not have access to?
If not, then I am still confused as to how to fix that. I tried patching both 4.5 and cvs of taxonomy.module but it's still not working . can you post the taxonomy.module you are using instead of patch?

thanks.

Steve Dondley’s picture

Steve Dondley commented

sulleleven,

Pyromanfo reports that he just uploaded a new CVS version of taxonomy_access tonight that some improvements I made to the module. If you haven't already, try giving the cvs version a shot. It will still work with 4.5.0. If you want to be a good thorough about things, delete your old term_access and node_access tables and then recreate them mysql.

Steve Dondley’s picture

Steve Dondley commented

Damn, sorry, I should have said new taxonomy_access moduel is in 4.5.0, not cvs. I need to go to bed.

nazadus’s picture

nazadus commented

Ok, to get things straight (from what I understand)

The taxonomoy_access was split into two branche becuase some bugs needed to be fixed.

Some changes / patches where recently made to update server bug issues (way too many to place links in here to each bug report and relevent post).

So *now* the latest, greatest, and most stable version of taxonomoy_access is located in the 4.5.0 tree now, so we don't need to patch or anything.. just grab 'n go.

Who is the official maintainer (if any -- not that it really matters, I don't think but I'm nosey and curious)?

Do I have my ducks in a row or am I missing something?

(Just attempting to clarrify... seems a wee bit hectic)

nazadus’s picture

nazadus commented

I should have clarified what I meant when I said split into two branches.
Someone seemed to be gone a while, and in the mean time someone else (nsysus) made some patches and recently (mere moments ago) made it an official 4.5.0 release thusly make two seperate mainters and two branches (this is a perception thing on my part, nothing official... branch = new person = new vision, sorda)

In any case, excellent work!
Most impressive!

Steve Dondley’s picture

Steve Dondley commented

nazadus,

Here's a brief chain of events:

1) taxonomy_access.module exists
2) I wrote a quick rewrite to 4.5.0 code because I needed it fast. I upload entire moduel and patch.
3) pyromanfo (maintainer) says there's some problems and wants it rewritten for cvs
4) I rewrite for cvs and make appropriate fixes about a week later
5) pyromanfo incorporates changes into cvs/4.5.0

Before using the improved version, you may want to wipe out the node_access and term_access tables and then recreate them just to be safe. Also, be sure to disable and then re-enable the module so it will update itself.

Let me know if you encounter any problems/bugs. I'll be happy to offer support.

media girl’s picture

media girl commented

Could someone please point me to the new version. What's on the project site is dated Nov 2. Is there a Nov 30/Dec 1 update for taxonomy_access somewhere else? Or is the available tarball simply mis-marked?

Thanks!

Steve Dondley’s picture

Steve Dondley commented

Download the CVS version here: http://drupal.org/project/releases/cvs

pyromanfo’s picture

pyromanfo commented

Yeah the date beside the tarball is just wrong, it's the updated version.

media girl’s picture

media girl commented

Thanks, I was hoping the date was wrong. The quick answer is much appreciated!! (I also appreciate the pointer to the CVS version, but I'm working from 4.5.1. Still, thanks anyway! And that's for the overhaul!)

Steve Dondley’s picture

Steve Dondley commented

It will work with 4.5.1. Just install it from scratch and don't use your old tables (if you have already installed taxonomy_access before)

Steve Dondley’s picture

Steve Dondley commented

Point of clarification: install the module and its tables from scratch, not all of Drupal

media girl’s picture

media girl commented

Gots it. Muy thanks. :D

media girl’s picture

media girl commented

This may be a really dumb question, but what am I supposed to do with the patch? Am I supposed to cut and paste that code into the module, or what? (Never really got the patch process here. Any clues much appreciated!) Thanks!

pyromanfo’s picture

pyromanfo commented

If you're on Linux or Mac, just use the "patch" program.

I don't know on Windows, try searching the rest of Drupal.

media girl’s picture

media girl commented

The patch command....hmmm.... I'm using shared hosting on Debian Linux. I've read the "Diff and Patch" thread, but I guess I'm missing something. Does the patch patch the SQL tables? Is it a MySQL command? Does it patch the module? Do I run that from my computer at home or on the server directly? There's nothing in the tarball explaining even what it does, or whether it applies to this issue or something else. I guess this is something for the forum. I'm lost.

Thanks anyway.

Kieran Huggins’s picture

Kieran Huggins commented
StatusFileSize
new patch.rar576.96 KB

Patch on windows:

I've attached a .rar of the files you need (patch.exe + 2 dll's)

you should have (at least) these files included with the module:

  • taxonomy_access.module
  • taxonomy.patch

and this file in your modules directory already:

  • taxonomy.module
  1. copy "taxonomy.module" and "taxonomy.patch" to a directory somewhere on your computer.
  2. unrar "patch.rar" attached to this comment to the same directory
  3. open a cmd box and run the following:

patch -l taxonomy.module < taxonomy.patch

this will "patch" your taxonomy.module file. you should now replace your original (on the webserver) with the patched version.

then, upload the taxonomy_access.module as usual, and install the mySQL tables.

cheers,
kieran

Kieran Huggins’s picture

Kieran Huggins commented

I should add that windows users have lots of trouble with patches because often text files from unix have different line break codes.

To get around this, I use Editplus http:editplus.com and convert the file format from Unix to PC under document -> file format.

cheers,
kieran

Anonymous’s picture

(not verified) commented