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'?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aacraig’s picture

More 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.

RSpliet’s picture

Version: 7.x-dev » 7.10
Priority: Normal » Major

As 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?

RSpliet’s picture

The 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

    foreach($nodes AS $index => $node) {
      if(!node_access('view',$node))
	unset($nodes[$index]);
    }

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.

xjm’s picture

Version: 7.10 » 8.x-dev
catch’s picture

Priority: Major » Normal
Status: Active » Postponed (maintainer needs more info)

node_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.

aacraig’s picture

I 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 :)

catch’s picture

Status: Postponed (maintainer needs more info) » Closed (works as designed)

OK I understand the problem now, but that's by design:

http://api.drupal.org/api/drupal/modules--node--node.module/group/node_a...

In node listings, the process above is followed except that hook_node_access() is not called on each node for performance reasons and for proper functioning of the pager system. When adding a node listing to your module, be sure to use a dynamic query created by db_select() and add a tag of "node_access". This will allow modules dealing with node access to ensure only nodes to which the user has access are retrieved, through the use of hook_query_TAG_alter().

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.

xjm’s picture

Status: Closed (works as designed) » Postponed (maintainer needs more info)

Re: #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.

xjm’s picture

Status: Postponed (maintainer needs more info) » Closed (works as designed)

Oops, xpost.

catch’s picture

Title: hook_node_access passing the wrong operation value in $op » Document that hook_node_access() is not called on node listings
Component: node.module » documentation
Category: bug » task
Status: Closed (works as designed) » Active

Actually we should document this in the doxygen for hook_node_access() itself, with a link to node_access() and etc.

aacraig’s picture

Thanks for the explanation.

jhodgdon’s picture

Issue tags: +Novice

Sounds 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...

chris.leversuch’s picture

Status: Active » Needs review
FileSize
672 bytes

Something like this?

jhodgdon’s picture

Status: Needs review » Needs work

That 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!

chris.leversuch’s picture

Assigned: Unassigned » chris.leversuch
Status: Needs work » Needs review
FileSize
672 bytes

Updated.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. 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?"

jhodgdon’s picture

Maybe 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)?

jhodgdon’s picture

Status: Needs review » Needs work

Actually, 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.

webchick’s picture

It'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.

jhodgdon’s picture

Good point... Let's see. Chris's patch from above says:

+ * Also note that this function isn't called for node listings. See
+ * @link node_access Node access rights @endlink for a full explanation.
+ *

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:

In node listings, the process above is followed except that hook_node_access() is not called on each node for performance reasons and for proper functioning of the pager system. When adding a node listing to your module, be sure to use a dynamic query created by db_select() and add a tag of "node_access". This will allow modules dealing with node access to ensure only nodes to which the user has access are retrieved, through the use of hook_query_TAG_alter().

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. :)

chris.leversuch’s picture

Status: Needs work » Needs review
FileSize
2.36 KB

How about this?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I like it. :)

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Awesome, thanks a lot for the revision!

Committed and pushed to 8.x. This doesn't apply to 7.x for whatever reason.

chris.leversuch’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.3 KB

D7 version.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks like the same patch, aside from d7 context. Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Eric_A’s picture

The 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...

xjm’s picture

@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() and hook_node_grants(), or hook_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!

Automatically closed -- issue fixed for 2 weeks with no activity.