Permission system overhaul

naught101,

Yeah... I was thinking about it. I don't know that much about the GRANT mechanism and it was working the way it was...

Changing it that much would require all the existing users to update their database "properly."

I'm not too sure how much impact it would have, but it could be dramatic.

If you can post a patch, I'll be happy to look into it.

Thank you.
Alexis

naught101,

The to_do pages are nodes and to work properly, which means work with the db_sql_rewrite() function, it must use the GRANT system.

This being said, the poll module, as you mentioned, may be a good starting point. 8-)

Thank you.
Alexis

naught101,

If you are to change the permissions, we'll need an update path too. Which is fine, it's just that we need to somehow make sure our users either go to their permission screen and rearrange the want they want it, or try to fix it by code (I think having an error/warning until you make changes to your permissions would be enough: less work for us and not so much work for the users.)

Now, I think if you're to change the perms, we should also consider doing what you mentioned earlier:

view + create + delete + update

We want all 4 permissions on content so we can have as much granularity as we need.

We want to keep at least one 'administer to do' for the global settings.

'can be assigned to do assignments' sounds good to me, instead of the 'to do list assignment'. The only problem now is that someone who can create or update a To do task should also be considered someone who can be assigned a to do item. Not like the current system work well yet 8-)... On the other hand, there could be one person creating To do entries but never otherwise assigned a to do.

Now, if we are to change all of those, let's make it 100% like the node permissions. In alphabetical order:

'administer to do'
'access to do content'
'can be assigned to do assignments'
'create to do content'
'delete any to do content'
'delete own to do content'
'edit any to do content'
'edit own to do content'

What do you think?

Thank you.
Alexis

Yes, we should keep 'administer to do list'.

I also thought of that problem... being able to click on the buttons if you're assigned a To do list item would make sense to me too. And since this module is supposed to be relatively simple to use, adding too many permissions is not a good solution, I think. So people who have Edit, Update or Assign rights should see the buttons and be able to click on them. And with time we could also add a few more buttons... to go to other statuses (such as Canceled or Wait statuses.)

Thank you.
Alexis

hook_node_access_records() and hook_access() do different things. hook_access() is for defining access permissions across a whole content type, while hook_node_access_records() (combined with hook_node_grants()) are used for defining access grants for individual nodes.

hook_access() is independent of db_rewrite_sql(). db_rewrite_sql() only deals with the grants system.

If I have time over the next few days, I'll try to look at the module and bust out a patch to fix the problem, but I haven't seen the code since Alexis took it over, and I have no idea what it looks like right now. I'm also pretty busy, so I can't guarantee that I will have time, but I'll do what I can.

Hi naught101,

That's a start... 8-)

The grant system is important to avoid showing nodes to users of systems such as views and organic groups. Other systems do not call the hook_access() function when they read the nodes from the database. Instead, they use the db_sql_rewrite() function that uses the grands saved in the database to determine whether the user who sent the request has the right to see that node.

So to test it, you should be able to use a views that shows a complete list of all the TO DO anyone created. You should then only see the TO DO that you created and whatever others were assigned to you (unless you have more rights and can see other's TO DO items.)

I think that we should be able to do that, although if it does not directly work in TO DO itself, someone can make it work with tac_lite, which, by the way, is a very good example on how to use grands. In tac_lite, the author uses taxonomy terms to protect pages, but the functioning is otherwise what we want.

Thank you.
Alexis

I think I originally set up the grants system to make it so that to do list items were only shown and accessible to people who either created the item, or had the item assigned to them. Otherwise, the item was shown on the front page to everyone, and was also accessible to everyone. If there were ever any private to do list items (such as 'Fire naught101 after work on Friday'!), then it would be no good if everyone could see them!

A few points.

1) hook_access is used for permissions for the whole content type, not individual nodes. You have a lot of logic in hook_access() that is checking individual node access, for example the following:

    case "view":
      if ($node->uid == $account->uid || in_array($account->uid, array_keys($node->users)) || user_access('access all to do content', $account)) {
	return TRUE;
      }
      break;
}

This is what the grants system (hook_node_access_records() and hook_node_grants()) is used for. hook_access() checks the whole content type, the grants system checks individual nodes.

2) case 'view' should never be implemented in hook_access(). The way the grants system works is that if nothing is found in hook_access to give or deny permission for a node, then next it moves onto the grants system. By defining a value in hook_access() for 'view', you never give the grants system a chance to work on the node, and as such any access modules, such as tac_lite or even Organic Groups (which is really just a complex grants system) stop working for viewing the node.

3) hook_access() is not called when rendering teasers. So the rules you have defined for the case of 'view' will be ignored, and even if a person isn't supposed to be able to see a node, they will still be able to see the teaser, though clicking on the link to the node will give them an access denied message. Grants take care of this situation.

4)

//can't we assume it's a to_do? otherwise this hook would not get called.
   if ($node->type == 'to_do') {

No, you can't assume that. Unlike other module based hooks, hook_node_access_records() is called for all nodes, not just the node type defined in the module. This is to allow for the creation of access modules.

I didn't look through the rest of the patch, or check your logic or anything, this is just what I noticed at a quick glance.

Nice contribution though! It looks like you put a lot of work into it.

naught101,

It looks like Jay knows more about this than I... 8-)

The hook_access() 'view' case should return FALSE when access is forbidden and nothing otherwise. That would be my take. Whenever you return TRUE or FALSE you break the loop and that very value is returned.

You have a few tab characters in the patch. Only spaces for Drupal!

You may want to run Coder on your changes before submitting them too. There are a few "mistakes."

> to_do now redirects to user/%/to_do instead of user/%/to_do/all, as it's accessible to more users.

I do agree with this change. It makes sense to send people to their list rather than the website list.

Thank you.
Alexis

We'll have to test, but so far the default was not ALLOW... It was DENY. As we should keep it that way.

Thank you.
Alexis

Actually, 'view' in hook_access() shouldn't even be implemented at all. Returning any value prevents the access system from proceeding onto the grants system, which means that other modules cannot implement their access systems.

This video (and accompanying PDF) where a great learning tool for me when I was trying to figure out the grants system: http://szeged2008.drupalcon.org/program/sessions/using-node-access

At a glace, that's looking much better. But you're still missing a test for the $node->type in hook_node_access_records(). Without this, you are going to attach your grants to all nodes, not just to do listings.

Hi naught101,

I've got a little problem with the following:

       foreach ($node->users as $uid => $ignore) {
+
+        //get user object for uid
+        $user = user_load($uid);
+        $grant_update = (user_access('edit any assigned to do content', $user) || user_access('edit all to do content', $author) || user_access('adminster to do list', $author) );
+        $grant_delete = ( user_access('delete all to do content', $author) || user_access('adminster to do list', $author) );
         if ($uid > 0) {

(1) you use $uid in user_load() before testing whether it is larger than 0.

(2) you called the variable $user which is an overlap of the global variable of the same name (dangerous long term)

(3) you use $author in the user_access() calls...

Other than that, there are some simplifications we could apply here and there, but nothing major.

Btw, shouldn't the hook_access() be changed and use the new permission names? I think that the hook_access() is still important for when you try to create a new to_do node.

Thank you.
Alexis

naught101,

Okay, I still need to test with an install, but I think we should have an update hook to fix some of the permissions since we're renaming some of them it is easy to just copy the current status of those couple. And we may want to duplicate some selection for some of the permissions in some of the new permissions. That way people can expect the system to more or less work the same way as before without having to fix the permissions.

What do you think?

The permission table:

psql> \d permission
                            Table "public.permission"
 Column |     Type     |                        Modifiers                         
--------+--------------+----------------------------------------------------------
 pid    | integer      | not null default nextval('permission_pid_seq'::regclass)
 rid    | int_unsigned | not null default 0
 perm   | text         | 
 tid    | int_unsigned | not null default 0
Indexes:
    "permission_pkey" PRIMARY KEY, btree (pid)
    "permission_rid_idx" btree (rid)

Thank you.
Alexis

Just a quick note, you will also need to rebuild the node_access permissions when the update is run.

Actually we should not rebuild the node_access(). Instead, we should mark that the access is out of date. Slight difference. There is a flag to set to 1 or something like that. There's probably a function you can call in the core to do that. I've seen that somewhere... 8-)

Thank you.
Alexis

Ahh ya, good point.

naught101,

A hook_update_N() function is expected to return an array of errors. There are db_...() calls that accept a $ret as the first parameter which is set to an empty array by default and returned at the end of the function.

In any event, whenever possible, use %s and %d and not "$direct_var" for SQL statements. Even though roles and permissions are quite safe, it is better/safer.

Thank you.
Alexis

naught101,

Man! You should be more careful about your spelling! 8-)

Okay. This is checked in. I noticed that (1) some permissions were not spelled correctly, (2) you did not change the corresponding permissions in the to_do_block.* files, (3) there was still a problem with the 'view all to do listings' option, which required a new grant.

Other than that, it was working pretty much as is.

Thank you for your effort. It is now checked in (available in 12h)
Alexis

P.S. you also have a new Navigation feature and a way to automatically go to another page on Mark Finished (see global settings.)

naught101, Jay,

As I was working on another project, I noticed something that caught my eye. The naming convention we just (tried to) fixed is not a 1 to 1 match to what D7 intends. I think it would be better to fix the problem sooner rather than later.

The node accesses are checked by node_node_access() and they clearly use the node type as the name for the access. In other words, if the node type is "to_do", it will use that and not "to do" (space instead of underscore) for the name of the permissions to be checked.

So, I suggest we fix all the permission names with "to_do". This should be easy although in D6 it does not matter much.

Let me know what you think.

Thank you.
Alexis

function node_node_access($node, $op, $account) {
  $type = is_string($node) ? $node : $node->type;

  if (in_array($type, node_permissions_get_configured_types())) {
    if ($op == 'create' && user_access('create ' . $type . ' content', $account)) {
      return NODE_ACCESS_ALLOW;
    }

    if ($op == 'update') {
      if (user_access('edit any ' . $type . ' content', $account) || (user_access('edit own ' . $type . ' content', $account) && ($account->uid == $node->uid))) {
        return NODE_ACCESS_ALLOW;
      }
    }

    if ($op == 'delete') {
      if (user_access('delete any ' . $type . ' content', $account) || (user_access('delete own ' . $type . ' content', $account) && ($account->uid == $node->uid))) {
        return NODE_ACCESS_ALLOW;
      }
    }
  }

  return NODE_ACCESS_IGNORE;
}

1.4 was released -- but it doesn't look like this made it in? Please advise as to the stability of the patch -- I'd need to have this functionality before considering using this module.. thanks for all the work!

http://drupal.org/node/997562

UPDATE: 10:25 AM EST

I installed 1.4 and it appears that the patches in this issue have been applied, so this issue is now resolved? BTW, when testing the permissions, I noted that Masquerade doesn't quite work with your module. It was giving me permissions to edit a task that I didn't create and I didn't assign when permissions were:
- administer to do list
- can be assigned to dos
- create to do content
- delete own to do content

capellic,

The naming convention used for the permissions is wrong. "to do" should have an underscore to be compatible with 7.x ("to_do") as it is the exact name of the module. The previous patches were in the previous version as far as I recall, wasn't it?

Is Masquerade using the GRANT system? If so, then know that if you grant permission in one module, that's it, the user has the right to edit/delete/create nodes of that type.

Thank you.
Alexis Wilke