The time has come for a final discussion of a node-level access control API for Drupal. There have been numerous discussions of this topic on the mailing list, forums, IRC, people's blogs, and other issues, but no existing issue seemed the right place to put this.

What is proposed is an API to be included into Drupal's core to allow modules to implement node-level access control without patching the core. Even if by some chance none of these modules make core themselves, the existence of a standard API will provide a stable upgrade path for people who choose to implement a third-party solution. The attached patch is what is being proposed for core. Examples of modules that could use this system are in my sandbox.

The following is from the README accompanying the files.

***********************************************************

This patch adds the following new functions:
function node_perm_join_sql($node_alias = 'n', $node_perm_alias = 'np')
function node_perm_where_sql($op = 'view', $node_perm_alias = 'np')
function node_perm_op_allowed($nid, $op, $uid = NULL)
function node_perm_user_grants($op, $uid = NULL)

These functions are used to construct node listing SQL queries and to inquire
about permissions for a given node. The permissions are stored in the new
{node_permissions} table; this is a grants table, which assumes "access denied"
by default. The table gives grants based on a combination of node ID and permission
ID. Permission IDs may be any identifier as determined by the active permissions
module. Common values may be role IDs, term IDs, or group IDs. Since it may be
required to have both user-level and role-level permissions, for example, the
"realm" column of the table is available to distinguish between different ID
types. A value of 0 in the nid column indicates that all nodes receive the grant;
similarly, a value of 0 in the perm_id column indicates that all users receive
the grant.

The current behavior of the "administer nodes" permission is retained; users
with this permission may view and edit any node.

The patch by itself should not change the behavior of Drupal, since the default
database data grants all operations on all nodes to all users. A permissions
module must be installed and implement hook_node_grants() in order to restrict
permissions.

In determining access rights for a node, node_access() first checks whether the
user has the "administer nodes" permission. Such users have unrestricted access
to all nodes. Then the node module's hook_access() is called, and a TRUE or FALSE
return value will grant or deny access. If the hook returns NULL, then the
node_permissions table is used to determine access. This allows, for example,
the blog module to always grant access to the blog author, and for the book
module to always deny editing access to PHP pages. Node modules are no longer
responsible for checking $node->status in their hook_access() implementations.

Multiple node permissions modules may be installed at the same time if they
operate on different realms. If any module grants the user a permission for a
node, that user is privileged regardless of what grants are provided by other
installed modules.

Four example modules are provided. One is a general example intended for
/docs/developer/examples that grants access based on a public/private toggle
and associated permission. One grants access based on user IDs. The third grants
access based on user role. The fourth grants access based on a taxonomy term. These may coexist peacefully.

CommentFileSizeAuthor
#7 node_permissions.mysql521 bytesjonbob
#6 node_permissions_0.patch44.95 KBjonbob
#1 nodeperm_taxonomy.mysql264 bytesjonbob
node_permissions.patch43.26 KBjonbob

Comments

jonbob’s picture

jonbob commented
StatusFileSize
new nodeperm_taxonomy.mysql264 bytes

Here is the associated addition to the database required for this to work. Note that the table is pre-filled with an entry allowing read access for all nodes, to make things work like they do now. Before enabling a node permissions module, you'd need to remove this entry or else people will be granted access to every node anyway.

moshe weitzman’s picture

moshe weitzman commented

Some time ago, I posted performance benchmarks before and after this patch. The patch slowed down drupal only 2%, which is remarkably low considering the impressive functionality it introduces. Nice work!

dries’s picture

dries commented

Here are my two cents:

  1. With this patch, it is easy to make posts private/public which is probably the number one feature request with regard to permissions. However, another important feature request is forum moderation (incl. comment editing). Would we be able to implememt this, or do we need an alternative solution/API for that? We don't have to solve this right now but it doesn't hurt to think ahead a bit. Can we easily extend the comment module such that user X can edit comments when he can edit the comment's node? Would that make sense?
  2. I find the names node_perm_op_allowed() and node_perm_user_grants() somewhat puzzling; it is not clear whether they set or get data.
jonbob’s picture

jonbob commented

1. It would be easy to modify comment.module to allow comment editing if the user has rights to edit the node. Whether this is a good idea is another matter. Note that the current implementation limits the access-controlled operations to "view", "edit", and "delete" only because those are the operations that already go through node_access(). Also note that only "view" is the hard one, because of all the node listings we gather. Any other operations are trivial to add, but we need to either decide on a finite set in advance, or change the table scheme to have a general "operation" field and one row per operation/node/permission ID combination (which multiplies the length of the table rapidly). This is the biggest bone of contention I foresee people having with the patch.

2. I will work on better naming conventions. It just occurred to me that all checks for permissions related to nodes go through node_access() now, so node_access_ might be a better prefix than node_perm_ to use for these functions anyway. Also, node_perm_op_allowed() might be able to be removed entirely and the code placed right inside node_access(), which I think is the only place it is (or should be) called anyway.

jonbob’s picture

jonbob commented

Some more issues to reference for related requests:
Permission for promote to homepage and static on homepage
Node author and comments
Add permissions to forums
The first two might require more granular operation types. Necessary?

jonbob’s picture

jonbob commented
StatusFileSize
new node_permissions_0.patch44.95 KB

Okay, here's a new version with updated (hopefully clearer) terminology. I wanted to clarify the names of the functions Dries had questions about, and also avoid conflicts with some existing terms. The term "permission" is gone to be replaced with "access" and "grant" where appropriate.

So here's a quick glossary of terms:
- "node_access": The function to call when asking whether the user has access to a node. Also the name of the database table that stores this information, and the prefix to the associated functions.
- "grant": A certification that a user or class of users can perform a particular operation on a specified node.
- "grant ID": A unique identifier for the class of users being given a grant; this may be a role ID or term ID, for example.
- "realm": An indicator of the domain that the grant ID is defined in. For example, this would be "role" for a grant ID that was a role ID.

The functions in the patch are copiously commented if you have more questions.

jonbob’s picture

jonbob commented
StatusFileSize
new node_permissions.mysql521 bytes

The earlier MySQL file was the wrong one, but it's been changed by the terminology change anyway. Here's the new one.

moshe weitzman’s picture

moshe weitzman commented

per Dries post #1 - I don't think it makes sense for a topic starter to be able t0 edit comments on his own threads. well, it makes some sense but its a pretty uncommon configuration in forum land.

i think that comment access control is beyond the scope of this patch. just my .02

as for JonBoB post #5. I closed oth issues 1,2 because they deserved it. #3 is almost fully addressed by this patch.

dries’s picture

dries commented

Moshe, a topic author would not be able to edit the comments unless he can edit his or her topic. You're mixing 'create privileges' and 'edit privileges': create privileges does not let you edit topics/comments, edit privileges do.

Moshe, how do JonBob's patches hold up against your taxonomy_access.module approach? Do you (still) prefer yours? I'd like to know.

dries’s picture

dries commented

JonBob: is it possible/easy to add a check to the database abstraction layer to check whether SQL queries against the node table perform a join against the permission table? A regex that looks for 'INNER JOIN {node_access}' would help catch many obvious security bugs.

JonBob: when I apply this patch, would - by default - unpublished nodes become published because of the fact that you modified the xxx_access() function of node module xxx?

(Kjartan: threaded comments for issues might be handy at one point. ;-))

moshe weitzman’s picture

moshe weitzman commented

I much prefer JonBob's approach to mine because it supports access control based on individual node, on group membership, on taxonomy, and other stuff we haven't dreamed of yet. Mine only supported taxonomy based access control.

jonbob’s picture

jonbob commented

Moshe said:

"as for JonBoB post #5. I closed oth issues 1,2 because they deserved it."

The first issue, while not worded well, does bring up the issue of how "sticky" nodes are handled. Currently a user must have the "administer nodes" permission to mark a post sticky. In the forum moderator scenario, the moderator should be able to do this. But giving the moderators admin access is not feasible because it also would allow them to, say, promote the node to the front page. This would be an argument to add further operation types (but maybe that's for a later phase).

Dries said:

"JonBob: is it possible/easy to add a check to the database abstraction layer to check whether SQL queries against the node table perform a join against the permission table? A regex that looks for 'INNER JOIN {node_access}' would help catch many obvious security bugs."

It isn't simple, because there are plenty of times the node table is used where the access check isn't necessary. The access check only needs to be part of the SQL for queries that retrieve node listings. Other manipulations of the node table can be wrapped in a node_access() call instead. I will look into this more, but I personally think this may be a better candidate for a devel.module extension than an extra regexp on every query in the Drupal core.

Dries said:

"JonBob: when I apply this patch, would - by default - unpublished nodes become published because of the fact that you modified the xxx_access() function of node module xxx?"

Nope. $node->status is now checked inside node_access() where it should have been all along.

dries’s picture

dries commented

Committed to HEAD. I'm not going to commit any of the nodeperm_xxx.modules at this point; they work but are often not intuitive (yet) as they are just proof-of-concept modules. In future, I might commit one or more popular access modules, but first, I want to evaluate how this is being used.

We need to extend the upgrade information in the Drupal handbook: it would be nice if modules like the image and project module could be updated shortly, so we can consider upgrading drupal.org. Note that this might affect caching of blocks with content in them.

Anonymous’s picture

(not verified) commented
pyromanfo’s picture

pyromanfo commented

Here's something I found while implementing [url=http://drupal.org/node/view/10047]Taxonomy Access Control using node_access[/url]. I'll repost here in hopes of getting JonBob's attention.

Oh yeah, this keeps all entries in node_access with full permissions, because node_access doesn't keep track of user roles like term_access. Therefore there could be several rules for different roles against the same taxonomy and in the node_access table the permissions of the last role updated would be the only one to exist, so I just bypass the node_access table entirely. Doesn't it sorta make it redundant to check the node_access table when hook_node_grants is already doing it's own checking? That is the point of hook_node_grants, right?

pyromanfo’s picture

pyromanfo commented

One more try
Taxonomy Access Control using node_access