tac_lite for Drupal 7

Dave:

We might be interested in helping out as we really need TAC or TAC_lite (preferable) for an soon-to-start migration.

I'm hoping you can port TAC or TAC_lite to D7 too - other than Simple Access, I haven't found something that will allow for the type of node access control that these offer and need to be able to regulate individual node/user/role access privileges. Thanks

Subscribing

subscribe

Hey all,

I've got a site coming up that I was debating using this for and decided to use this as a chance to get up to speed with D7. I spent some time over the past two days working on an upgrade patch. So far I've got the basic interface working. Over the weekend I hope to move on to the user edit page interface and possibly the tac_lite_db_rewrite_sql functionality (though I may need some direction on that portion). Hopefully I'll be able to get something up for review in the next few days that will at least get the ball rolling. I'll post it here once I have something.

Here is a first pass at this patch. I focused on a straight port with no feature changes or improvements. There are two main areas I didn't get to yet: tac_lite_db_rewrite_sql and the tac_lite_create module.

I used the coder update live site as a first pass (http://drupal.org/project/coder), but removed most of the format suggestions for simplicity. There are a number of minor formatting changes to account for updated function names or structures, but here are some big changes that had to happen for D7:

• Database queries needed to move to the new database API (http://drupal.org/update/modules/6/7#dbtng)
• The taxonomy system has undergone a major overhaul and many of the API functions are gone (see http://drupal.org/update/modules/6/7#taxonomy_node). Of note: $node->taxonomy is gone and replaced by fields.
• The old way of using hook_user to save user data has been split up into hook_user_categories(), hook_form_alter() and hook_user_presave().
• I found that I had to set defaults in the grants for each permission type. Also, there's a note in the docs for hook_node_grants that calling rebuild will actually remove all of the default "0" grants. I wasn't sure if that was a change from d6 or not.

This obviously needs a lot of testing and review, but so far the basic functionality (besides what is mentioned above) seems to be working and no errors are being thrown. It will probably also need a round of comment and help updates too. It's probably worth mentioning that it doesn't seem need an upgrade hook as none of the data seems to be different (although I didn't specifically run any upgrade tests).

Let me know what you think. The patch was built in Eclipse, so let me know if there are any formatting issues. If you want a deeper walk through, just give a shout.

This is a bit of a quick review, and probably missed alot, but here are a few quick items I noticed. There's also some extra whitespace being added in places that should be checked.

+++ tac_lite_create.info	(working copy)
@@ -1,11 +1,11 @@
 ; Information added by drupal.org packaging script on 2010-11-13
-version = "6.x-1.4"
-core = "6.x"
+version = "7.x-1.4"
+core = "7.x"
 project = "tac_lite"
 datestamp = "1289688676"

This chunk can be removed altogether. Doesn't exist in CVS.

+++ tac_lite.module	(working copy)
@@ -13,6 +13,7 @@
+      $output = '';
       $output .= '<p>' . t('This module allows you to restrict access to site content. It uses a simple scheme based on Taxonomy, Users and Roles. It uses the node_access table and other features built into Drupal to hide content from unauthorized users.') . '</p>';

Is there any reason to add $output = '';? Just make the first line of output use the assignment operator (=) rather than the concatenation-assignment operator (.=).

+++ tac_lite.module	(working copy)
@@ -31,8 +32,13 @@
+      'description' => t('TODO Add a description for \'administer tac_lite\''),

Probably this should get a description rather than leaving the TODO. How about 'Administer access control by taxonomy' or similar?

+++ tac_lite.module	(working copy)
@@ -145,6 +151,10 @@
+/**
+ * @todo Please document this function.
+ * @see http://drupal.org/node/1354
+ */

Let's make sure to document these.

+++ tac_lite.module	(working copy)
@@ -186,7 +196,7 @@
+function tac_lite_admin_scheme_form($form_state, $form_state, $i) {

I think the first parameter should be $form.

+++ tac_lite.module	(working copy)
@@ -394,49 +462,45 @@
+ * See http://drupal.org/update/modules/6/7#taxonomy_node
+ * See http://drupal.org/node/959984

Should be removed, unless the upgrade notes are providing meaningful context to how the function operates. If they are relevant to the upgrade process only, they should be removed.

Powered by Dreditor.

Thanks for the quick review. I'll remove the packaging chunk. Actually, I was building this against my own svn, so it might make sense to pull down the contrib cvs and create the patch against that. Might make the patch a bit cleaner.

Some of those documentation todos are left over from the coder update module. I wasn't sure if it made sense for me to try to document these on the first pass, but I'll give it a shot where it makes sense. I'll remove the upgrade notes also.

You're right on the form_state. Typo.

Thanks!

Here's an updated patch that removes some of the comments and other items listed above. I created it against the 6x.1x-dev branch from CVS so it should be a bit cleaner and easier to apply. I also made a small tweak to use the taxonomy select helper function in a spot where it was not being called. Caveats about tac_lite_db_rewrite_sql and the tac_lite_create module needing work still apply. Needs review.

subscribing

Here is an updated patch that adds in the missing tac_lite_db_rewrite_sql() functionality from the last patch. Two questions I had that need review:

1. I'm using the "term_access" tag to identify queries that should be acted on. That seems to be correct after a quick review of taxonomy.module and given how the documentation suggests that db tags are meant to work.

2. I couldn't find a clean replacement for the $primary_table variable that hook_db_rewrite_sql() had. Instead I had to loop through the tables to find the one that didn't have a join and assume that it was the primary. (See the "HELP" comment in there).

Using this patch, I was able to set a scheme to use "term visibility", create a "secret stuff" term, and confirm that another user without that access could not see it. Giving that same user view access to this term then allowed him to see it. This also affected the list of terms on the node edit form: "secret stuff" didn't show up for this user when it wasn't supposed to.

This still does not touch tac_lite_create. I started down this path, but I think I got a bit lost in the interface. I think I've got my head around it now, however. I'll just need to rework this module to account for the structure change in $node->taxonomy (see #978242: Document the changes to taxonomy.module properly to avoid confusion about missing $node->taxonomy property).

I guess this technically needs work until that bit is done, but if someone knowledgeable is around to answer the two questions above that'd be helpful.

subscribe

Something like this?

function _tac_lite_node_get_terms($node) {
  $terms = array();
  if (module_exists('taxonomy') && variable_get('taxonomy_maintain_index_table', TRUE)
    $query = db_select('taxonomy_index', 'ti');
    $query->innerJoin('taxonomy_term_data', 'td', 'ti.tid = td.tid');
    $query->innerJoin('taxonomy_vocabulary', 'tv', 'td.vid = tv.vid');
    $query->addField('ti', 'tid');
    $query->condition('ti.nid', $node->nid);
    $query->orderBy('tv.weight');
    $query->orderBy('td.weight');
    $query->orderBy('td.name');
    $result = $query->execute();
    foreach ($result as $row) {
      $terms[$row->tid] = taxonomy_term_load($row->tid);
    }
  }
  
  return $terms;
}

Thanks for the commit Dave.

I don't yet really have a feel for what all tac_lite does behind the scenes so it's a bit difficult to know where I can help... I submitted that function to get terms but I don't know how well it works. I took the actual query from a portion of the tokens module aimed at getting a taxonomy term token in.

Maybe I'm even looking at the wrong module. What I want is to be able to tag a user with taxonomy terms, tag a node with taxonomy terms, and if the user/node share the same terms then the user can edit those nodes and only those nodes.

Anyway, I need this pretty soon on D7 so any way I can help I'm game, I just don't know exactly what tasks are needed, how exactly to go about some of the functioanlity, or if it does what I need.

I'm getting several:

Notice: Undefined index: #parameters in tac_lite_create_form_alter() (line 74 of /home/dev/www/sites/all/modules/tac_lite/tac_lite_create.module).

on the scheme pages. Still testing the rest but this is definite progress.

Overall it seems to work. I don't understand some of the "visibility" stuff. These settings are by default turned off, yet the tags are usable/visible on the node?

Just an aside and possibly out of the scope of this module is that it might be nice if the user granted update access to that taxonomy could edit the node, but not the terms attached to the node. I foresee having problems with users accidentally revoking their own access because they changed the term to something else, without realizing what they were messing with, or allowing another user access they shouldn't have by adding another term.

Here is a patch to upgrade the tac_lite_create module. Rather than add another db query into the mix, I wanted to try to use the information that was already in the $form object. I *think* was mostly successful, until I got to the vocabulary reference in the field definition. It's actually the machine name for the vocab, not the vid. I had to make a call to taxonomy_vocabulary_machine_name_load(), but I'm hoping that is cached and still better than adding a new query. All of the jumping around in the arrays field_info_fields array seems a bit fragile, but hopefully it is still a smaller memory footprint. If not, then something more along the lines of what mcaden posted in #14 should do the trick.

The patch was made against the new Git "master". First time creating a patch against the new Git system, so hopefully I got it right. I did a few basic tests and it seems to work, but I'm sure I missed a few of the combinations of how vocabs, permissions and fields might be set up. Needs review.

If this works, I'd humbly suggest we close this issue and start opening new ones for other bugs/features that crop up.

I'm working now on getting the code from git so I can patch and test.

Ok, it looks like it worked pretty well. I did 3 things:

Fixes:

line 82 - index not found - needed field lanaguage before getting default-value
line 112 - index not found - $form doesn't have the #parameters object. I found the information needed at: $form_state['build_info']['args'][0];

"Improvements":

line 59 - it only works on listboxes. Why not radio buttons? changed to:

if ($form[$field_name][$field_language]['#type'] != 'select' && $form[$field_name][$field_language]['#type'] != 'radios'){
   continue;
}

The below patch has these changes in addition to the ones from Sean in #24.

Everything I've been testing looks good. The part that concerns me are the snippets in Sean's code where he's uncertain as to what something does or whether or not something is needed. This leads me to believe there are corner-cases that we're leaving out.

I'd like to be using this on a production site very soon. It'd be nice if we could get this committed and get it tested more.

@Dave - any chance you could get these patches committed and see what's lacking? Any glaring holes?

Thanks Dave

Ah:

&& $form[$field_name][$field_language]['#type'] != 'checkboxes'

Needs to be added where I talked about it above in comment #26

Glad that patch was at least helpful. I'll probably be doing some testing between sessions this week. Give a shout if you'll be in Chicago and want to meet up and chat through any of the questions I had.

@mcaden: You're right. There's no reason checkboxes shouldn't be supported too. Maybe that check should just return if the type is auto complete so as to not break other custom widgets down the road?

Probably.

Should we close this and start opening new issues for d7?

Got rid of some more notices by changing line 348 of tac_lite.module from:

if ($form['#user']->data[$config['realm']]) {

to:

if (isset($form['#user']->data[$config['realm']])) {

(Changed to isset(...))

I'll make a patch when I get home. I don't have git where I am at the moment.

Changed in tac_lite_create.module - at line 58

      // Skip auto complete fields as they are not supported.
      if ($form[$field_name][$field_language]['#type'] === 'textfield'){
        continue;
      }

Changed in tac_lite_create.module - at line 92

- if (count($form[$field_name][$field_language]['#options']) == 0 && $form[$field_name]['#required'] == TRUE) {

+ if (count($form[$field_name][$field_language]['#options']) == 0 && $form[$field_name][$field_language]['#required'] == TRUE) {

Changed in tac_lite_create.module - at line 93

- drupal_set_message(t('You have no permissions to add content to the required %name vocabulary. Please contact the site administrator if you believe you should have permission to add content.', array('%name' => $form[$field_name]['#title'])));

+ drupal_set_message(t('You have no permissions to add content to the required %name vocabulary. Please contact the site administrator if you believe you should have permission to add content.', array('%name' => $form[$field_name][$field_language]['#title'])));

Changed in tac_lite_create.module - at line 99

- if (count($form[$field_name][$field_language]['#options']) <= 1) {

+ if ($form[$field_name][$field_language]['#required'] == FALSE && count($form[$field_name][$field_language]['#options']) <= 1) {

Best regards
the-sandman

It's frustrating that there doesn't seem to be a way to allow people who have access to some terms but not others to have the terms they don't have access to hidden on the form, and not removed when they submit the form.

For example:

User A has access to update term 1 and 3
User B has access to update term 1 and 2.

Node A has terms 2 and 3.

If user A edits Node A, it loses term 2 and user B can no longer edit it.
If user B edits Node A, it loses term 3 and user A can no longer edit it.

If something like #284917: Allow FAPI select, radios, and checkboxes to specify some options as disabled was in core we'd be able to do something about it.

What I've done for now:

            if(!in_array($term_id, $form[$field_name][$field_language]['#default_value'])) {
              unset($form[$field_name][$field_language]['#options'][$term_id]);
            }

So all terms that are set on a node will be visible if the user has access to edit even one of the terms. Unfortunately that means that if user B wants to be a pain in the rump they can simply unclick term 3 from node A and suddenly user A couldn't edit it, but this would have to be intentional.

About the only real solution I can think of would be undo the code I put in this post and instead to hook onto node submit and re-add terms that that user doesn't have access to remove.

EDIT: Not sure how that passed my testing but that code was way wrong, corrected it.

What I've done for now:

            if(!in_array($term_id, $form[$field_name][$field_language]['#default_value'])) {
              unset($form[$field_name][$field_language]['#options'][$term_id]);
            }

So all terms that are set on a node will be visible if the user has access to edit even one of the terms. Unfortunately that means that if user B wants to be a pain in the rump they can simply unclick term 3 from node A and suddenly user A couldn't edit it, but this would have to be intentional.

About this quick fix, where would you put it ?

It was at line 86 on the version I have. The problem is that I was doing a bunch of other editing such that I can't give an accurate dif because I don't remember what else I changed. I've attached the complete version I have.

As you are both suggesting, it seems like this issue has gotten a bit out of control and hard to follow. Since the initial pass at a port was committed, can we close this and start opening new threads?