Grant equal time to nodeapi for access control responsibilities

Ooh yes. +1 on this one.
It's one of the remaining shortcomings of CCK-defined content-types : node.module 'owns' the nodes, and thus is currently the only one who can grant access rights.

i'm not sure how i feel about it happening before the module gets a chance to weigh in. module can make rather complex decisions on the permissions and this would throw a wrench in it... but i guess you could say that about every hook in drupal.

in principle, this seems like a good move. my main concern is that people already getting confused by hook_access() all the time, which leads to security bugs. adding yet another hook related to this seems like it's just going to add to that confusion. i'm not really sure what to do about that, since this new hook obviously has some good use-cases. but, it'd be nice to at least consider the developer-friendliness while we're at it.

my other concern is regarding the dreaded FALSE vs. TRUE vs. NULL return value from hook_access. if *any* module implements hook_nodeapi('access'), then the only possible returns are TRUE (definitely grant access, regardless of any other possible ACLs) and FALSE. this scares me, since that's the cause of most of the security related bugs. it's not obvious how this code handles this case. i guess the idea is that node_invoke_nodeapi('access') is going to return an empty array if all of the hooks returned NULL. in that case, $access will in fact by empty, and we'll fall through and do the rest of the checks. if any of the nodeapi('access') calls returned a value, then we get the array_filter() squashing behavior as commented. if that's the intention, i think the comment should make that clear.

we also definitely need documentation about the new op for hook_nodeapi(), though i guess that's a patch against contributions/docs/developer/hooks/core.php, not drupal/modules/node/node.module.

setting to CNW for these documentation tasks, since i think seeing the docs up front will help people grok this patch.

love it, new comment works for me. i'd still like to see the docs for the new op, but that doesn't require a core patch, so no sense holding this up unnecessarily. i tested merlin's na_test.module and the patch, and everything works great. i still think people will screw this up, but that's not a good reason not to add it.

speaking of which, merlin's example code in na_test.module is a classic case of the bug everyone's going to hit:

 case 'create':
        // randomly disallow creation of 'story' pages. Period.
        // during $a3 == 'create' $node isn't actually a node (we don't
        // have one, so it's the node type to be created instead).
        return $node != 'story';

that's not actually what that code does. that code actually translates into: "prevent creation of story nodes, and *allow* everything else"(!). to do what the comment says, you'd need to do this:

  if ($node == 'story') {
    return FALSE;
  }
  break;

see what i mean? ;) also, beware that if you return TRUE in here, you can override the dedicated permissions for creating or editing node types. that's by design, but potentially scary. again, we just need good docs about this.

anyway, in spite of the likelihood of bugs and potential confusion, i still think this is RTBC. it'll certainly make life vastly better for certain (important) cases, and that outweighs my concerns. for example, just because people are likely to screw it up and hurt themselves badly with the php filter doesn't mean we shouldn't provide it. ;)

I always wanted to have an opportunity to allow nodeapi to do something with create access. So, lovely. Still. I would like to see Heine's opinion on this patch.

Subscribing.

I am one who dreads the node_access table too. I needed to implement this, and read the documentation, but was still stumped, mainly because of the grant ID and realm being confusing and ambiguous. I even had to email Earl asking questions before I got it going.

So, the node_access system needs to either become easier or replaced by some layer on top of it that would be easier. What we have now is a medley of modules (taxonomy access, node access by role, simple access, ...etc.). What is needed is either to replace node_access and its API, or some layer between those modules (access control applications) and the few functions that manipulate node_access that is easier to grok.

Back to this patch, I think when we had node types owned by modules, things were easier (e.g. job search module in 4.6). Once CCK entered the picture, we could not do things like "view own resume", and I had to jump through hoops to do that with node_access.

So, I am for anything that makes access control easier. Would this cause some people to shoot themselves in the foot? Great! More fun! I mean these same people would be making other mistakes that are as deadly anyway.

Can't we at least remove hook_access in favour of nodeapi(access)?

Can't we at least remove hook_access in favour of nodeapi(access)?

I wonder what this means for the future of the node_access table/system? I'll have to think about that some.

Rerolling to remove fuzz.

I agree with those who say this is a good thing. Read access is tricky because of performance, but for CUD operations this offers far more flexibility with a far easier to understand API than the node_access table. As for the future of that table, on a project I'm doing now in D5 I'm doing all sorts of unholy things to the menu system in order to avoid having to touch node_access, especially when I already have OG installed.

This opens up all sorts of interesting and not-mutually-exclusive possibilities for richer permission control. There is certainly interest in such power.

The future of node_access would likely be that for non-read operations it's vestigial, IMO. I'm OK with that. If that happens and access modules switch to this mechanism, we can remove/reduce/refactor it in D7.

A big +1 for this. Working with the node access system is one of the difficulties in implementing more complex moderator behavior for forum systems, etc...

Too me, this looks like a bad move. The old system is still there; we just added an extra layer to work around a problem. The solution is to fix the problem rather than to work around it. Working around it, will only make things worse in the long run. We can't take this lightly; we're talking about security, remember ...

Then the fix to the problem would be this patch plus removing the node_access table entirely. Right now, it simply not scalable, especially with hook_access() effectively vestigial already. That's a step backward if something (like this patch unless someone has a better suggestion) isn't done to compensate.

i oppose classifying this as "needs work" without any clear suggestion of what alternative to investigate, instead. there's nothing wrong with this patch, except that it exposes just how bad another part of the system (the node_access table) is. i thought we were all in favor of smaller, self-contained patches whenever appropriate, to make them easier to read, understand, test, and commit. so, let's start here: make a new way to handle access that's not as bad as the node_access table. in another patch, we can consider removing node_access all together, if that's really a good idea (i don't yet know enough to say for sure). people who know far more than me are pretty clear we can't yet completely be rid of it, and the feature/API freeze is approaching fast. perhaps we'll have to say D6 is in a transition regarding node access, and we'll have a couple of different, flexible ways to handle it. we'll use the lifespan of D6 to get more experience with these issues and how we want to clean it all up and unify it in D7. FAPI has gone through a number of iterations. i'm sure the schema API is going to change in the future, too. just because it's not perfect doesn't mean we shouldn't start.

that said, i totally sympathize with the potential for error this new hook will create, and that means more work for the security team. i'm accutely aware of this, being both on the team myself, and having found and fixed node access bugs in project* in the recent past. however, in the wrong hands, print() or db_query() are just as likely an API to write insecure code with as this hook. :( just because there's potential for people to screw up doesn't mean we shouldn't provide this tool, it means we should re-double our efforts to continue educating the drupal development community on all issues related to security, recruit more forces for the security team, provide better infrastructure for handling security issues, etc. merlin and i have been working hard on the infrastructure, especial via the update_status.module, the new "Release type" vocabulary for release nodes, and so on. perhaps we can convince merlin to join the security team, too, when this patch lands. ;)

cheers,
-derek

I agree that this patch should be incorporated. I started using a similar patch a few months ago: http://drupal.org/node/122173. This allowed me to get both OG and Taxonomy Access Control working together: http://drupal.org/node/122712.

The result is very secure, multi-level access control that allows you to truly create communities within communities. For example, we don't need to create 10 different forums because users can determine who can see their posts by the vocabulary terms they select.

Now, I know what you're going to say: "But, you can do that now in Drupal!". Well, you can create content in a forum in a group and assign it a vocabulary. However, right now, anyone who is either in the forum, in the group, or has access to the vocabulary term can see the content. Once you can control your access, using a mechanism such as the nodeapi access patch, they you can create access control that requires that someone belong to the forum AND the group AND have access to the vocabulary term before he can see the content. See my notes on doing this here: http://groups.drupal.org/node/3700.

So, bottom line: Although I'm using a slightly different patch, I would truly like to see this patch included in the core to give folks like me the chance to create the systems with the content security we need.

Dries: This is an important feature. Right now, Drupal's access control system has regressed in power due to node module owning most node types. That's made hook_access() effectively less powerful. We need some way to make the access control system more flexible.

What would need to be done for you to accept this patch for D6? I really don't want to go another major release with a reduced-capability access control system. Or is there some other mechanism that you think would work better?

re-rolled without fuzz. still works as nicely as it did originally. there's obviously need for something like this, as SomebodySysop and Crell eloquently explained above.

+100 for this patch.

Not to long ago there was a drupal dojo lesson that covered how to do what this patch allows in a rather hackish manner. But, for the sites I have worked on this patch provides a much better way to handle the access controls. By better I mean faster, less code, smaller db, less logic, and less confusion in my mind. I know I'm not the only one that thinks like this.

Personally, I see how both methods can be valid. There are some cases where I'd use this method. There are some cases where I would use the existing system. Is what we need a better way to make the two methods play with each other?

I've already dreamed up the first module I would write using this hook so I hope it gets into D6.

What will it take to get this patch committed? A complete change to the existing system? Flowers and candy :-)? Snowflakes in hell?

I think this is an essential patch for making drupal better. The current access system works well but it's hard to understand and is more complicated than is typically needed. I could come up with some use cases for it on an enterprise level but for the sites I have and build it is more complex and has more overhead than I need or desire. I know there are many who agree with this.

Here are a couple use cases for this patch:

1) A very requested feature is to have view control over node types based on role. In 4.7 there was a patch for this. In 5.x there are a couples ways people have done this. This patch would allow for a simple and low overhead method to implement this feature as a module.

2) A forum moderator would be easier to do here. Instead of tying access to nodes they could be tied to taxonomies. I know that taxonomy access does this but it really is a work around and assigns everything on a node level.

I would love to read other use cases for this. I am sure there are directions of access flexibility I have not even thought of.

The current system is really complex. Many developers don't get it so it scares them off. To many others it's more complex than my needs for a project. This would give a simpler method for access control.

Security is an issue. If it's a matter one access control module not playing well with another access control module we already have that problem. In the last month I have had to fix a database because several times due to access control modules not playing nice with each other. That problem already exists.

If it's a matter of putting security in nodeapi is there a security reason not to? Is it a matter of not trusting module developers to have a secure system of their own?

If we can only have one or the other of these access control methods I would be in favor of this patch and pulling out the current system and putting it in a module.

Like Dries, I see this patch as a step in wrong direction. I've written up my brief objection to this patch over at http://drupal.org/node/21613#comment-242137. i think that issue is the way forward.

moshe, I have to disagree. A hook-based solution is far more flexible because it can handle context better. Say I want a given user to be able to created or edit a node of a given type, but only in a given organic group (or similar). With the database method, while in D5 we kinda sorta can mix access modules it's iffy. I would need to store a record for every user/og/node combination. On the site I'm building (this is not a hypothetical example), that's going to be hundreds of thousands of records very quickly and not even handle creation permission. And then I'm still potentially bumping into og's node_access records. Right now I'm doing unholy things with extra dynamic menu items just to avoid that mess, and it keeps me up at night crying. :-)

With a hook-based solution, that becomes a simple context-sensitive function call. No huge database table. I can do anything I need to in it. That opens up the potential for per-user, per-role, per-og or og-like grouping, per-site section, per-something-else, per-node, access controls. Those can all happen in contrib, but I don't see how with the node_access table.

+1 from me- if you can control access with hook_access, you should also be able to do it with hook_nodeapi('access')

i'm coming around on this. i think it would clean up some stuff in the audio module where i added a callback so other modules could weigh in on the access.

http://drupal.org/node/91709 seems like prior art for this.

Maybe what we need, and I know I said it earlier, is a access hook and we make the current access system a module that works through the hook. This would allow those who want to use the current system to continue using it and give access to other form of access control.

how would such a "hook" handle the functionality currently in db_rewrite_sql? The node access system has to be able to deal with filtering lists, as well as determining whether access allowed for a specific node.

I can see the advantages of this patch, however, there are disadvantages that we can't simply ignore.

When it comes to performance vs security, I vote for security. I want one access control system so there is only one system to understand and audit, and so there are no unexpected collisions or interactions.

All the motivations for this patch basically boil down to: the current approach is either slow or hard to understand.
If this patch is vastly superior, let's remove the old approach then? If that's hard, maybe we can simplify the current system and go from there?

I promise to give this issue a closer look as soon time permits!

The current approach is both slow *and* hard to understand, but it mostly gets my +1 from the "hard to understand" angle.

At OSCMS, Earl did a 50 minute talk to explain the workings of the node_access system, complete with real-world examples in the form of a theoretical "book access" module. After that talk (which was very good, btw) many people still found themselves hopelessly confused when they actually sat down to start coding (myself included ;)).

So imagine you are training a new developer:

"Let's take an hour so that we can go over the intricacies, and then let's spend another hour going over node_access_example.module in detail to show how these concepts are implemented. then let's talk about all the ways that you can use to troubleshoot, such as using devel_node_access to view these cryptic nuimbers and talk about what they mean. And then let's talk about all the ways that this fragile system can break, where you'll need to click 'Rebuild node access' which will take 10,000 years."

or!

"You know nodeapi op view? This is exactly the same. Return FALSE in your module if you want to deny access."

I spent some time in #drupal going around and around with some folks on why we need the node_access system at all. I was referred to p. 104 of the Pro Drupal Development book, which has a flowchart of the node access system.

It seems to me that from "Invoke hook_node_grants" on down, this could just be replaced with "Invoke hook_nodeapi op access" => FALSE? => Disallow.

What about node listings? hook_db_rewrite_sql. And you don't need the node_access table to restrict access to viewing listings; modr8 module, for example, implements hook_db_rewrite_sql alone to stop moderated nodes from showing up in listings, and never touches the node_access table.

So can someone please help me out on why we need the old node_access system at all?

@webchick - the node_access table *is* a db_rewrite_sql() based system. your proposal for each module to JOIN against own table does not solve anything. You just decentralized the problem, and probably removed some flexibility for modules to cooperate with grants and denies. how does that help accomplish anything? we will still have 2 systems for determining access.

Let's step back a minute. There are five possible operations that a user could do to a node: Create, Read, Update, Delete, and List. (And then the "own" variants of the last 4, but let's put that aside for now.)

Create: Right now there is no access control for this except for hook_access(). Since node module owns most node types these days, that's not very useful. The node_access table does nothing here.

Read, Update, Delete: These operate on a single node at a time, the latter two on a single node per page load in 99% of cases. Update and Delete are also rare operations (relatively speaking). Therefore, there is no problem with determining access at runtime rather than "compile time" (node_access table).

So for CRUD operations, the node_access table offers us nothing over determining access at runtime via a hook. It's an extra query or two, unless you have a dozen access modules installed, in which case you're in for pain anyway. :-) Cost is n nodes * m access modules, where n is 1.

The only place that there's any performance benefit of pre-compiled access permissions (which are, as we've seen, very brittle and difficult to understand and space hogging and so forth) is on List operations. There, the runtime cost is n*m where n >= 1. Regexing in a couple of joins is better for performance here, but worse for flexibility. Right now, that's what we do.

But what if we let the hook operate on a set of nodes instead of one node at a time? Vis, pass a "list access" hook or nodeapi op an array of nids. Each hook implementation can then do whatever filtering it wants on that array, and then return either an array of those nids it will allow or NULL to indicate that it doesn't care. So the usage would look like (pseudo functions):

$nids = db_query_column("SELECT nid FROM {node} WHERE blah blah");
$nids = module_invoke_all('node_access_list', $nids);
foreach ($nids as $nid) {
  $node = node_load($nid);
  // Do stuff with $node;
}

It's a little more code, but not that far off from what, say, taxonomy module does now. It all gets to run at runtime, though. There's the same number of node_load() operations as we have now. So the total cost becomes n*m+c, where n is again 1 (because it's one array rather than one int).

Thus we eliminate any need for the node_access table and db_rewrite_sql(), push all permission checks to runtime where we have far more flexibility, and the performance hit is reasonable (especially as most sites won't have more than 1-2 access modules installed).

I now wait for someone to tell me why that's a dumb idea. :-)

How would a paired list keep you from knowing the number of nodes you'll have total? I don't follow. You'd just know it in a different place.

$nids = db_query_column("SELECT nid FROM {node} WHERE blah blah");
$nids = module_invoke_all('node_access_list', $nids);
$nids = array_chunk($nids, 10);
foreach ($nids[$page_you're_on] as $nid) {
  $node = node_load($nid);
  // Do stuff with $node;
}

i am fine with earl's proposal to drop update/delete ops from node_access API.

ideally, i'd like to further cleanup and drop hook_nodeapi('access', 'view') and make the view operation db only. then we are left with each operation only living in one place. we would need some db records added to support the 'edit own' permission. maybe there are other gotchas with this - i don't know.

it would be nice if the patch here were a bit more flexible so that a module could grant access even if another module were denying access. basically, modules would return a priority along with a boolean. that could be as simple as an integer like -88 signifying a very str0ng deny, and +5 signifying a mild grant. i'd rather not lose that feature from node_access API.

@Crell

The problem with having no node_access table is that you cannot know the access permissions for a set of nodes without retrieving them individually and doing the new access nodeapi on them.

node_access, as hard as it is to use, offloads the work to the database by doing a join on the realm/grant and returning only the nodes you have permissions to view.

If you have a pager, and have to know how many nodes satisfy your permission criteria, you still have to do the node_invoke_all on ALL nodes from your first SQL.

This is like doing :

SELECT x.id FROM x WHERE x.col = 0;
foreach(x.id in result set)
  SELECT y.blah FROM y WHERE y.id = x.id

(We retrieve in PHP the full list, then filter it by another table).

Instead of:
SELECT y.blah FROM x INNER JOIN y USING(id) WHERE x.col =0
And getting back the result set we need.

That is one (very valid) disadvantage that prevents node_access from being replaced.

That's why I'm saying don't do it one node at a time, but as a set. So you'd end up with m WHERE IN-style SQL queries. No, it would not be as fast as doing it in all one join, but it would be more flexible and would eliminate the last need for {node_access} and db_rewrite_sql(), since Dries indicated he doesn't want to mix access systems.

If Dries is OK with using {node_access} for View and List and a hook for everything else, I'm fine with that too.

I'm a relative newbie to Drupal. I've been programming for 20+ years, programming in Drupal for just under one year.

The node_access nodeapi patch was a Godsend. It provided the mechanism for me to get two access controls, OG and TAC, working in unison. Access control for "view/edit/delete" operations took just a few hours. Using nodeapi('access'), I was able to get TAC and OG working together with less than 6 lines of code (that's in the nodeapi function itself).

Working out the access control for "list" operations took several weeks.

Why the disparity? db_rewrite_sql().

Working out access control for "create" operations in my OG User Roles module took several months (9 to be exact). http://drupal.org/node/87679 (if you don't believe me). And, still no one can tell me why it wouldn't work.

My point? Whatever you do, PLEASE DO NOT require more use of db_rewrite_sql for access control!

mostly just a comment to tag.. I am not completely clear on everything being said here - but an example of issue i was faced with in current project that maybe is (hopefully) somewhat related. (btw - project is still in 4.7)

stumbled upon extremely useful views idea of extending the std /user url by simply adding views menu items with page urls as user/$arg/myview1 user/$arg/myview2 (also did this to extend std search page)

this worked great .. but here's the rub...

site has 2 user types: member and company..

- both have their own profile page at something like user/34 or user/58 - but 34 is a member and 58 is a company.
- each role has their own views to get added
- and each role has their own private tabs on their profile page

it didnt seem like views had the ability to control any of this so i resorted (for lack of knowledge of a better way) to hook_menu code in project's custom module that looked something like this:

// member
$items[] = get_user_tab ("watching",'Watching',"mowner",2);    
$items[] = get_user_tab ("faves",'Favorites',"mowner",1);
$items[] = get_user_tab ("reviews",'Reviews',"member",0,MENU_DEFAULT_LOCAL_TASK); 
$items[] = get_user_tab ("surveys",'Surveys','disable',3);   // disable surveys tab for now
$items[] = get_user_tab ("points",'Points','mowner',4);
       
// company  
$items[] = get_user_tab ("our_reviews",'Reviews',"cowner",1);  
$items[] = get_user_tab ("products","Our Products","company",0,MENU_DEFAULT_LOCAL_TASK); 
$items[] = get_user_tab ("our_surveys","Our Surveys","cowner",6);     
$items[] = get_user_tab ("contact","Contact Info","company",7);

so basically this would create access control on menu items based on 4 different flags: mowner, member, cowner and company which effectively showed certain tabs if the user/$arg was a company or a member and also controlled which tabs were private to the owner.

But even this was not good enough since i also wanted to remove the std "view" tab from this tab set (since it has no place in my site).. this was harder since access control already set before my module runs and i need to then go into _menu array and remove the item manually.

i am sure i have missed the boat here.. and maybe even WAY off topic for this thread.. but can't help thinking there must have been a much easier way to manage all of this.

i guess the majority of this would have simply been a views.module form of access control for "role of ARG" and "owner only" - not sure either of these exist (but dont think so). Although still doesnt help me get rid of std "user/view" tab.

Peter Lindstrom
LiquidCMS - Content Management Solution Experts

Wow, turns out a nearly identical patch was proposed even earlier at http://drupal.org/node/122173.

I just added a comment there, pointing folks to our more in depth discussion here, but we should really continue any further comments in the older thread.

subscribing - i hope interest in this doesn't fall off!

@Harry: see http://drupal.org/node/122173 -- that's where this is going forward (Dries postponed it to D7).

subscribe

When I tried to patch it with D6.10, it says "patch: **** Only garbage was found in the patch input.". So I figure the patch doesn't apply to 6.10 anymore, does that mean I have to manually patch it up?

Anyone has more info?

Thx!

As easy as this makes things, the node access system allows you to do some heavy processing in hook_node_access_records without a performance hit on every page load. I hope people use a feature like this very lightly. I hope it doesn't disuade newbies from learning the node access system.

Moved over to: #537862: Convert hook_access() to hook_node_access() for more flexibility