What values to return in hook_node_grants?

The first part of the problem was that I had not removed project_issue_access(), so it was still relying on user permissions, and no permissions had been granted to the authenticated user. I removed project_issue_access, and now node_access is being used for issue nodes, which currently gives read access to everyone.

The next question is how we want to assign permissions for issues. There are a couple options:

• Automatically assign the same permissions to issues for issues that are assigned for the project.
• Have separate permissions for issues

In addition, here are the following defined permissions in the project_issue module by default:

• access own project issues
• access project issues
• assign and be assigned project issues
• create project issues
• edit own project issues
• set issue status active
• set issue status by design
• set issue status closed
• set issue status duplicate
• set issue status fixed
• set issue status needs review
• set issue status needs work
• set issue status postponed
• set issue status ready to commit
• set issue status wont fix

where the first five are hardcoded and then one permission created for each defined issue state. Do we want to leave that part as is?

Re: issue perms. I agree that probably in most cases you just want each issue to inherit the access settings of the project the issue belongs to. However, I can also imagine situations where that's not sufficient, and you want separate settings. For example, you might want to let anyone edit their own projects, but not their own issues, since you don't want anyone editing issues (changes to the issue should be reflected with comments so they're logged and tracked). Or maybe you want to let people delete issues, but never delete projects. Or, maybe there's a wider pool of people who can create issues than can create projects.

Oh, speaking of creation -- how would that work? Seems like project creation is a single site-wide setting. Either you can create them or you can't. But issues could be locked down -- you can only create issues attached to projects you can access. Or, perhaps there are projects you can access where you can't create issues. So, we probably need at least a per-project setting for "create issues for this project". Sorry if I'm getting off topic here.

Also, you might need to override the issue access settings on a per-node basis. For example, imagine scalability wasn't a concern, and we didn't need a separate sec.d.o site -- it'd be nice to just be able to flag certain issues as "confidential" and then they're only visible to the owner of the issue, the project maintainer(s), and the global site admins (e.g. the security team). But, other issues in the same queue could be public issues that anyone could see.

Furthermore, some permissions/actions only make sense for issues, not for the project itself. E.g. the create thing above. You can't have a setting on a project node for "permission to create this project", but you can have settings for "permission to create (issues|releases) attached to this project". Furthermore, this "assign and be assigned issues" setting -- there's no analogy with the project itself that the issues could inherit. Maybe you could argue that if you can edit the project node, you should be able to assign issues. But, it'd be better to decouple those, so that you could have a larger pool of co-maintainers and support helpers who can coordinate in the issue queue, but only a smaller group that can actually edit the project itself.

So, all that leads me to believe we at least need the option of separate issue-related access settings for each project, not just hard-coding that all roles/users always get the same CRUD access on the issues attached to a project as they do for the project itself. I haven't played with the UI yet, so I'm not sure how much this complicates things. Feel free to push back if you think I'm being unreasonable. But, I think we really need the additional flexibility of the issue-related permissions for each project.

Finally, in terms of the per-status permissions -- I think that's basically a hack. That code was in there when I started working with Project* 4 years ago, we've lugged that code around ever since, and I don't remember seeing a single issue where someone was glad they had those permissions to solve a problem. :/ I think it's all a poor substitute for a real solution at #441004: Finite state machine (FSM) for issue status (or something like that). I wish there was an easy way to ask all the Project* users right now, but I suspect no one actually uses any of these -- maybe I'm wrong. At this point, I'd just leave those as site-wide global permissions, and we can consider removing them entirely down the line. I don't think we want per-project per-role per-user knobs for each issue status value people can set. That seems like major overkill, a nasty UI to make sense of, and scope creep. Let's focus on CRUD and issue assignment first. We can always revisit issue status perms later and fold those into this module if we really need to.

Thanks!
-Derek

This is to document my conversation with @dww in IRC tonight:

1. We want separate settings for
   • view this project (already in place)
   • edit this project (already in place)
   • delete this project (already in place)
   • view issues attached to this project
   • edit issues attached to this project
   • delete issues attached to this project
   • create issues attached to this project
   • assign and be assigned issues attached to this project

   View, Edit (update), and Delete are all handled via node_access records. Create and assign will be separate issues to handle in different places in the code (other than hook_node_access records and hook_node_grants).

2. Default global settings
   Two options:
   1. use global defaults
   2. override global defaults with settings for this project
3. Settings issues as confidential
   Each issue will have a "Confidential" checkbox. There will be three different permissions:
   1. view all confidential issues - role setting on permissions page
   2. view own confidential issues (user can view confidential issues he/she created) - role setting on permissions page
   3. view confidential issues for a project - setting per user/role on project node

I've made a bunch of progress implementing the ACL module, but I'm stuck and could use some help. The basic question is, what - if I anything - do I return in hook_node_grants? acl.module has it's own implementation of hook_node_grants, so I'm not sure if I need to add something additional in my own hook.

Second, one of the trickier parts of this is that there is a separate page for issues access, which sets permissions for all issues tied to the project. I have the code working for saving records to the project_access table for all issues, but it looks like (I think) I need to add an acl_user entry for each user/issue combination. Does that sound right?

I'm going to be working on this some more on Sunday and Monday (I know how to have fun on a holiday weekend), so any suggestions are appreciated.

Thanks.

I haven't looked under the hood of ACL module, so I'm not sure if there's a better way to solve this. Seems like it'd be a drag to have to create separate records for every possible user on each issue, but I don't know if there's a good alternative. I know DamZ looked into ACL at one point, and did some work with comment_acl, so I've pinged him to comment here. Hopefully he'll shed some light.

Thanks,
-Derek

ACL mostly does everything itself, but its API is slightly obscure (and by "slightly obscure", I mean "totally undocumented"). I pointed wonder95 to comment_acl, which is probably the most simple implementation of the API one can imagine.

Thanks, Damien. Do we need a task in the acl issue queue requesting proper API docs (acl.api.php)?

There are two already there:

#761462: Get Started Documentation
#256846: API Documentation anywhere?

OK, I've spent some good quality time with acl module, as well as studying comment_acl and content_access, and I think I have a pretty good grip on where I'm at and what needs to be done. Here are some notes more for my own notes than anything else, but feel free to chime in if you think I'm missing something.

I have code working so that I'm implementing acl as much as possible, including using the form provided by acl for adding access at a user level. I also created project_access_acl_save_form, which is a customized copy of acl_save_form that adds records to acl_user for every issue for a project.

I finally have wrapped my head around the question I asked in this issue, which is what is required to be written in project_access_node_access records, and the answer to that is the role access records. acl has it's own implementation of hook_node_access records, but it only writes records for acls that actually have users assigned to them. ACL creates a separate list for each op (view, update, delete) that only handles that op, and ties the uid to the acl_id, which is named by a combination of the op and the nid (i.e. view_2, which gives view access for nid 2). So, even though there are three access lists for node 2, it only returns grants for the ops that actually have users assigned to them. So, if I have view and update assigned to users Test1 and Test2, but delete not assigned to everybody, then acl_node_access records will not return a grant for the delete op for that node, even if a role was granted that permission.

So, the short version of all this is that acl only returns grants for the user level, not the role level (comment_acl only specifies user level access, and content_access adds its own role level permissions to what acl provides). Therefore, in project_access_node_grants, I will have to check the acl tables to see if the user has been granted user access, and only return the role level grant records if user level access has not been specified for that user.

However, just the existence of a record for a node in node_access will grant access to that uid, so if I have a role level records in the project_access realm and user level records in the acl realm that happen to have the same gid (which will be rid in project_access and acl_id in acl), the user would get a combination of the two records. For instance, if a user assigned to acl_id=4 that also has an rid=4 would get the combined rights specified for gid of 4 in the acl realm AND the project_access realm. So, I will also need to check for user level access in project_access_node_access_records and write role access records only if user level access isn't listed in the acl tables.

One final question for @dww (at least in this comment :-)): should I ignore role level access if I find even just one user level permission assigned? For instance, if I see that a user has been granted view access to a node, but no update or delete permission, should I just assume that view is all they get, even if their role gives them greater permissions, effectively assuming that a lack of update and delete permissions is a deny?

Been working on this a little more, and I'm stuck at one point. As I mentioned above, the goal is to return grant records for the role when there are no user level permissions assigned, and I'm doing that. However, I'm getting a conflict that I don't understand. It can be best explained by this screen shot. This can be seen in action at http://project/smgaweb.com/node/2.

In the permissions settings for this node, I have no user level permissions assigned, and I have view and update permissions set for the authenticated user role (which is what Test1 is). However, as seen in the screen shot, devel_node_access.module is giving this error when I hover over the "NO: input format" in the update column:

DNA and Core seem to disagree on this item. This is a bug in either one of them and should be fixed! Try to look at this node as this user and check whether there is still disagreement.

I'm not sure where this is coming from, since I'm only returning the project_access realm (acl isn't returning anything in acl_node_grants), and the only two records in the node_access table for this node are the two from the project_access realm for admin user and authenticated user roles.

Any suggestions?

Got my hook_node_grants issues cleaned up (taking a little time away helped me see things better), and now I have another issue/question. We've established that user level permissions override role level permissions, but does that include an admin user? IIRC, all permissions for project/issues will be ripped out of project.module and project_issue.module, so I'm assuming that would mean that we would want a separate "administer projects" permission (and "administer issues", too) like what currently exists in project.module that would give the user access regardless of what is set for his specific user id/role. Sound correct?

Per dww in IRC today, this is what is needed in relation to confidential issues:

• the person creating the bug to see their own issue
• the project maintainers to see any bugs in their own issue queues
• the sec team to see all issue

I'm going to map out the permissions and grants that I think you need. Some initial notes:

1) I don't see why you would introduce ACL module here. It's overhead. Unless you are trying to integrate with multiple node access mechanisms, it's not needed and just adds clutter, making the code harder to follow and maintain. If you are trying to integrate with multiple node access modules, which ones and why?

2) Many of the permissions we need can be handled _outside_ the node access system, which is really only needed for LIST VIEWS at this point. By adding a very simple drupal_alter() implementation to the Project module's project_project_access() and project_issue_access() functions, we can allow for an extension module (Project Access) to administer fine-grained control over the Create, Edit and Delete operations for projects and issues. In Drupal 7, this happens with the new hook_node_access(), which we can imitate by editing project module. Then we can enforce our most complex rules with a simple hook that can read values from the project or issue node.

3) Having per-issue / per-user access rules in the node access table will potentially skyrocket the number of rows in the {node_access} table. There is no clean way to store per-user records in {node_access}, so expect performance overhead for that. I would strongly recommend simple inheritance of the permissions from the parent project at least for the initial launch. Before trying to engineer a perfect but complex solution, let's try something more basic and see if it works for our use-cases.

4) The 'confidential' status essentially means that you need a separate realm just to handle permission to those issues. I would use simple inheritance from the project for all permissions except those marked confidential. We can write separate records to cover those cases. This will prevent the need to escalate the number of records for every issue, instead isolating just the ones that really need per-user rules.

5) Beware of returning TRUE or FALSE in hook_node_access_records(). This will bite you in D7. Easier to use 0 and 1.

6) Never invoke node_access_write_records() directly. That's an API violation. (I'm looking at project_access_node_access_records()).

Attached is a map of what I think the permissions look like, and how they would be saved. The first document, ProjectAccess.pdf outlines the various permissions and how they could be controlled. The second, ProjectAccessActions.pdf outline specific storage of data and implementations of hooks in order to enable the access rules.

Heh, I knew I needed to get some other eyes on this.

1) It was a decision made after discussions with dww and DamZ so that we were using a (somewhat) standard method of handling user level permissions.

2) Nice. That would eliminate a ton of code I bet.

I'll let dww, greggles, or others more knowledgeable on the s.d.o needs chime in on Ken's suggestions.

Thanks for helping with this, Ken.

Yes, Ken, thanks! I'm very grateful that someone so knowledgable about Drupal's node access systems is getting involved in this effort. Which isn't in any way meant as a slight against all the work wonder95 has already done. wonder95, you've kept this effort alive and moving forward, and done a lot of great work already! So, thanks to both of you. ;)

No time right now to ponder Ken's suggestions carefully. One quick not about ACL -- one of the proposed solutions was to use comment_acl so that as you followup to a sec.d.o issue you can alter the ACLs and add someone (e.g. someone else reports the same bug and we want to just add them to the existing ticket, even if they're not a project maintainer nor the original reporter of the vulnerability). And, DamZ seemed to be way into acl.module, and I haven't had the bandwidth to look closely and have opinions of my own, so I just deferred to his judgement.

I appreciate the concerns about scalability, but keep in mind we're targeting sec.d.o here, not d.o itself. The primary reason that's a whole separate site is b/c we know the performance is an issue once you start doing node access. So, I'm happy to do things faster and smarter, but not at the expense of even more code complexity. Project* is already complicated enough. ;) If faced with a tradeoff of code readability and maintainability vs. server performance, in this case, I'm generally going to vote in favor of saner code and throwing more hardware at the problem... That said, it's entirely possible those aren't the tradeoffs you're talking about, and really you're saying "here are two more or less equivalently complex approaches code-wise, and A is going to perform way better than B". If that's the case, sure, A is the clear winner.

Anyway, I'm very glad to see this being revived and having more eyes on the problem.

Thanks!
-Derek

My position is simply that, since this has been stalled for so long, let's drop some of the complexity -- requiring per-user access to specific issues by default -- and implement something that we can get into the field.

With what I sketched out, we should still be able to mark some issues as 'confidential', which would then trigger some per-user data storage. I just wouldn't do that by default.

The other question, of course, is "what does confidential mean?" Are there a class of users who should have access to all confidential issues? If so, that might change the data model a bit, adding a 'project_confidential' realm with a corresponding grant.

@agentrickard: Makes sense to get something live and improve from there.

Re: "confidential", the use-case is that anytime someone who's not already on the sec team or a project maintainer reports a security problem (by creating a sec.d.o issue about it), it will be "confidential" to them. However, the rule is that project maintainers will be able to see all issues assigned to their sec.d.o issue queue, regardless of if they're "confidential" or not. Plus, anyone on the sec team itself will be able to read all issues, period. Make sense? Does that answer your question?

Thanks again for stepping up to drive this home!

Cheers,
-Derek

@Derek I believe that's how I intepreted 'confidential', and the provided data model should cover that case.