The core node module's comments state:
* 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 node module does not intervene (returns NULL), then the
* node_access table is used to determine access. ...
This is NOT what happens. Here is the code:
if (user_access('administer nodes')) {
return TRUE;
}
if (!user_access('access content')) {
return FALSE;
}
// Can't use node_invoke(), because the access hook takes the $op parameter
// before the $node parameter.
$module = node_get_types('module', $node);
if ($module == 'node') {
$module = 'node_content'; // Avoid function name collisions.
}
$access = module_invoke($module, 'access', $op, $node);
if (!is_null($access)) {
return $access;
}
// If the module did not override the access rights, use those set in the
// node_access table.
As you can see, there is an interim step not metioned in the comments, and unwelcome in practice. The current code checks the "access content" permission BEFORE asking the module's hook_access if it's allowed. So even if the module allows viewing the node, if "access content" is turned off, it never gets called.
The easy fix is to reverse these two checks, which puts the node module more in line with what it's comments say:
// Can't use node_invoke(), because the access hook takes the $op parameter
// before the $node parameter.
$module = node_get_types('module', $node);
if ($module == 'node') {
$module = 'node_content'; // Avoid function name collisions.
}
$access = module_invoke($module, 'access', $op, $node);
if (!is_null($access)) {
return $access;
}
// the module did not override, so use the default access permission
if (!user_access('access content')) {
return FALSE;
}
I have tested this and it works as expected. I'm sorry I didn't create a patch, but I felt that viewing the code here makes more sense.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | node_131950_6.patch | 790 bytes | nancydru |
| #14 | node_131950_5.patch | 770 bytes | nancydru |
| #1 | patch131950.txt | 586 bytes | nancydru |
Comments
Comment #1
nancydruHere it is as a patch.
Comment #2
moshe weitzman commentedwhat is the use case for allowing submission of content without access content? i think i know, but lets hear your need. also, this access content protects against a sloppily coded hook_access(). maybe module authors don't need that protection.
Comment #3
nancydruMy module does that check itself. I want (and in one case, need) to restrict access to my content regardless of whether other access is granted.
The way the node module is currently coded, nodes are not allowed that right - it is overridden before they even get called. By the way, I have responded to at least two posts in the last week from people who are also experiencing this problem. The site on which I discovered this could not properly use an access control module because the owner had to give this privilege.
Besides, whether I code a module sloppily is my problem, isn't it?
Comment #4
moshe weitzman commentedThanks ... but you didn't answer my question. I am lookin for a one or two use cases like "i have a site for bakers. Bakers can submit cakes but can't view them ...."
Comment #5
nancydruI have a site for a group/club that has a newsletter application. Viewing the newsletter needs to be restricted to members only (special role) while the bulk of the site is available to everyone. Because of the way it is implemented (mixture of taxonomy, blocks, and special code), access control modules aren't protecting every possibility. Fixing this bug and using my own "Access Newsletter" permission does what I want.
I maintain another site that is largely a "protect by default" site (even though no access control module currently does that). One content type needs to be available, regardless of the default "access content" setting.
This is all pretty much the same as "access comments" and "access site-wide contact form." If those can be individually restricted or opened up, then specific content types should enjoy the same freedom.
Comment #6
jlinares commented@nancyw: Maybe http://drupal.org/node/139921 helps you. The patch is for HEAD, but I think it applies cleanly to 5.1 (also, I have a 5.1 version).
Comment #7
nancydruThat looks good except that it doesn't seem to help people with modules that create their own content type. This is true for many contrib modules that are already in existence, not just mine. For example, look at the FAQ module, which has its own "view" permission (that presumably doesn't actually work because of this bug).
If this works even in that case (which it may), then it needs to be properly documented that the "official" permission should be worded as "View content-type" rather than "access" as one might assume for the more global permission defaulted by the node module. I can live with that.
It certainly puts this permission in the correct order. Thanks.
Is this an official solution that will show up in 5.2 and 6.x?
Comment #8
jlinares commentedI'm working on a way to apply the same concept to modules that create their own content type. And this is not official at all, just mine. :) If you want to discuss about this solution, I think we'd better go to http://drupal.org/node/139921.
Comment #9
nancydru@jlinares - that patch was rejected (by design).
Comment #10
nancydruStill there
Comment #11
moshe weitzman commentedComment #12
catchno longer applies.
Comment #13
nancydruI just looked at the beta2 code and see the same sequence of code that will produce the same problem.
Comment #14
nancydruUpdated patches for 6.0 and 5.7.
I still don't think it's right to allow "access content" to override the node_access_table.
Comment #15
beholder commentedI have come in very similar situation. We are developing intranet site on Drupal 6. Most pages are closed for all users except special role (which we assign to users after manual checking of their accounts). But some content (for example FAQ-pages) are opened for everyone. So I tried to open access to FAQ and deny access to all other content, but I always get 'Access denied' page. Because 'content access' check goes before FAQ-access.
I think that nancyw solution is very simple and logical. I just can't configure my site's permissions right without this patch.
Comment #16
moshe weitzman commentedSo discuss this for 7
Comment #17
nancydruMoshe, I'm not sure this should be changed to 7. IMHO, this has been a bug since I reported in for 5. The documentation says it works the way we want it, but the code behaves differently - to me that is a bug. Bug fixes shouldn't have to wait for the next version.
Comment #18
moshe weitzman commentedI sympathize with that argument. The problem is that invasive bug fixes are very unlikely to be committed. This "bug fix" changes the behavior in fundamental ways.
Comment #19
nancydruI understand that. I don't know how to check it throughout core, but I seriously doubt that the impact would be noticeable except to those who want it to work right, and we are unlikely to complain.
But if this is to be considered a "fundamental behavior change," why has no one yet changed it to a feature request?
Comment #20
casey commentedWe're in alpha fase already...
Comment #21
nancydru