On a fresh D7.4 installation, my implementation of hook_node_access shows the $op parameter as 'create' on the home page when generating teasers for the front page content.
Reproduce:
Create a custom module and implement hook_node_access()
function example_node_access($node, $op, $account) {
dpm($node, 'node');
dpm($op, 'op');
}
Create some nodes and publish to the front page.
Load the front page.
Shouldn't the op value be 'view'?
Comments
Comment #1
aacraig CreditAttribution: aacraig commentedMore information:
On the full node page (ie. node/XX), the hook is called for both the view and the create operation, so I assume that the create operation gets checked as part of loading a node for display.
However, on the front page (and perhaps other listings pages?) the view op never gets passed to the hook, which seems strange to me.
Comment #2
RSpliet CreditAttribution: RSpliet commentedAs of today I can still confirm this incorrect behaviour in Drupal 7.10. As this prevents implementing a safe node access system, I feel a higher priority is justified. Is there a logical explanation for this behaviour, or (more likely) should we consider this as a bug in core?
Comment #3
RSpliet CreditAttribution: RSpliet commentedThe main problem is that _node_query_node_access_alter alters the query for the main page to reflect the grants. It however fails to make up for the hook_node_access calls. An acceptable work-around (for me) would be the following.
node.module:node_page_default(), after line 2558 ( $nodes = node_load_multiple($nids); ) , add
However I am unsure if this can be considered a fix, as it still first tries to access the node with 'create' rights. For now I can ignore 'create' in my module as I already did.
Comment #4
xjmComment #5
catchnode_access() with the create $op is called by the menu system to check access for node/add and node/add/%node_type, that's by design. If you can find it being called from a view op in a backtrace that'd be a bug, but no such backtrace has been provided so this needs more info.
Comment #6
aacraig CreditAttribution: aacraig commentedI think we should be careful not to muddy the waters with the issue regarding the 'create' call. It is appropriate that the 'create' op get called for view ops, so that the system can determine whether or not to offer links to manage the content, something I hadn't thought about when I first posted this issue.
The real problem, however, is that the view op is NOT getting called for teasers on the front page. That should happen regardless of what's going on with the create op.
For this reason, I'm not sure the status should be moved to 'postponed', as I don't believe there's any other information to be had. In order to correctly implement node_access for teasers on the home page, hook_node_access() should be called with $op == 'view', and it's not.
While I'm at it, I'll point out that this is potentially quite problematic for D7 sites, so I'm not sure about moving this to D8, either :)
Comment #7
catchOK I understand the problem now, but that's by design:
http://api.drupal.org/api/drupal/modules--node--node.module/group/node_a...
You need to use hook_node_grants() / hook_node_access_records() for node listings.
On moving the issue to 8.x, see the backport policy.
Comment #8
xjmRe: #6: Filing the issue against D8 is done to prevent regressions. (For more information, see the backport policy.) Almost all bug fixes get backported to Drupal 7 within a few weeks of the D8 fix. There are rare exceptions when a fix includes a UI or API change that would in turn break existing sites.
Comment #9
xjmOops, xpost.
Comment #10
catchActually we should document this in the doxygen for hook_node_access() itself, with a link to node_access() and etc.
Comment #11
aacraig CreditAttribution: aacraig commentedThanks for the explanation.
Comment #12
jhodgdonSounds like a good idea. Please just say it isn't used in node listings, and then link to the Node Access topic page, which has the full explanation and links to all of the relevant hooks and functions.
Probably a good novice project...
Comment #13
chris.leversuch CreditAttribution: chris.leversuch commentedSomething like this?
Comment #14
jhodgdonThat looks good! The only detail is the line wrapping -- doc paragraph lines should wrap as close to 80 characters as possible without going over (and without splitting an @link/@endlink up into two lines). So the word "See" should move up to the previous line. Thanks!
Comment #15
chris.leversuch CreditAttribution: chris.leversuch commentedUpdated.
Comment #16
jhodgdonThanks!
Comment #17
webchickHm. Will a developer new to the node access system know what "node listings" means? Should we cite an example or two, like ", such as the default front page and recent content block?"
Comment #18
jhodgdonMaybe that text should be added to
http://api.drupal.org/api/drupal/modules--node--node.module/group/node_a...
instead (which is where the reader of hook_node_access() is pointed to for more information)?
Comment #19
jhodgdonActually, how about if we change it from "node listing pages" to something more descriptive, in both places... maybe:
- pages that list nodes
- listing pages generated from node queries
hm... Somehow we want to get across the point that if you're viewing a single-node page, this hook gets called on the one node on that page, but if you're viewing a page composed of a SELECT query on nodes, the hook doesn't get called on each node.
Comment #20
webchickIt's not just pages, though. The same is true of any node listing (e.g. node queues, blocks, views, etc.) done with SELECT queries rather than running through node_view(). It's /really/ important that people understand this, IMO.
Clarifying it centrally sounds like a reasonable compromise.
Comment #21
jhodgdonGood point... Let's see. Chris's patch from above says:
That seems like a reasonable thing for this function, especially since it says "for a full explanation", and it doesn't call them "node listing pages", just "node listings". But maybe it could say: "node listings (e.g., RSS feeds, the default home page at path 'node', a recent content block, etc.)"?
On the Node access rights topic... it currently says:
So maybe here, we should add another sentence (after the first one? as a parenthetical remark right near the beginning?) explaining more about what node listings are. Maybe something like:
(lists of nodes generated from a select query, such as the default home page at path 'node', an RSS feed, a recent content block, etc.)
Wording is open for improvement and suggestions. :)
Comment #22
chris.leversuch CreditAttribution: chris.leversuch commentedHow about this?
Comment #23
jhodgdonI like it. :)
Comment #24
webchickAwesome, thanks a lot for the revision!
Committed and pushed to 8.x. This doesn't apply to 7.x for whatever reason.
Comment #25
chris.leversuch CreditAttribution: chris.leversuch commentedD7 version.
Comment #26
jhodgdonLooks like the same patch, aside from d7 context. Thanks!
Comment #27
webchickCommitted and pushed to 7.x. Thanks!
Comment #28
Eric_A CreditAttribution: Eric_A commentedThe query tag recommendation confused me for a minute, coming straight from taking a look at node_admin_nodes() which builds a query... without any 'node_access'-tag. (My use case: have it show unpublished content for users without "bypass node access".)
I can't find an existing issue, surely there must be one...
Comment #29
xjm@Eric_A -- Since
node_admin_nodes()
is on an administrative page, it woud make sense that its query would not have the node access tag, I think?Granting access to unpublished nodes can be accomplished by implementing
hook_node_access_records()
andhook_node_grants()
, orhook_node_access_records_alter()
if you want to grant additional access in conjunction with another node access module. See also http://drupal.org/project/view_unpublished. I'd suggest asking in IRC if you have further questions (or filing an against whatever node access module you might be using) since this is outside the scope of this issue. Thanks!