I have a node (A), with a node reference to node (B) and I have the nodereference field set to display the full node (B) when displaying node A. This works great when both nodes are published. The problem I am running into is when node B is unpublished. A user (editor) has access to view and edit this node as defined in Drupal's node_access table and can do so when viewing node B. However, when viewing node A, node B is not displayed as I would otherwise expect it to. We get the nid back but not the rendered node as we have previously specified. I believe this is an issue with how nodereference populates the field by checking for the 'administer_nodes' permission instead of paying any mind as to whether the user has rights to view node B as seen here:

from nodereference.module:

    259: if (!user_access('administer nodes')) {
    260:  $where[] = 'n.status = 1';
    261: }
    262: $result = db_query(db_rewrite_sql('SELECT n.nid, n.title, n.status FROM {node} n WHERE '. implode(' AND ', $where)), $missing_ids);
    263: while ($row = db_fetch_array($result)) {
    264:   $sanitized_nodes[$row['nid']] = $row;
    265: }

Does anyone have any insight as to why this is done this way? Is there a work-around? If editor has the ability to view node B, I believe they should have the ability to be able to see it when it is referenced from node A.

CommentFileSizeAuthor
#7 cck-946554-7.patch1.21 KBq0rban
#4 cck-946554-4.patch1.21 KBq0rban

Comments

apotek’s picture

apotek commented

Been wondering about this too, and I think the analysis is correct. The expected behavior should be as described at the bottom of the issue. The 'administer nodes' perm is not flexible (or modular, for that matter, if a node were to wish to override the default response). Ideally, the logic would be left in place, but extensible with an alternate permission check that a module that needed more flexible authorization schemes could extend.

q0rban’s picture

q0rban commented
Title: Nodereference: Administer Nodes vs node_access Table » Sanitize referenced nodes using node_access()
Category: bug » feature

Just to be clear, this is not an issue with a typical setup, this only occurs when you have a system that allows a user without 'administer nodes' permission to be able to view an unpublished node. As such, I'm retitling and changing to a feature request.

Benchmarking would need to be done to see the performance implications of switching to node_access instead of the simple method currently in place.

karens’s picture

karens commented

Some of this code was added as a response to a security issue to ensure that no one could see nodes they don't have access to if they find a way to get to the url used to return autocomplete results. So any attempt to alter this would have to be carefully scrutinized to be sure no new security hole is introduced. That being said, the current setup is certainly hard-wired to the normal Drupal behavior of hiding unpublished nodes from pretty much everyone and might be made more flexible.

q0rban’s picture

q0rban commented
Status: Active » Needs review
StatusFileSize
new cck-946554-4.patch1.21 KB

Attached patch fixes it for us, but needs testing with regards to performance.

q0rban’s picture

q0rban commented
Status: Needs review » Needs work

Actually, a fundamental flaw I just noticed with that patch is that the node's uid is not being passed to node_access. I'll re-roll it tomorrow.

q0rban’s picture

q0rban commented
Status: Needs work » Needs review

Here's some benchmarking. It seems that there isn't much performance hit due to this change on a clean Drupal site (actually Pressflow 6.19.94) with just CCK 3.x, Devel, and Admin modules enabled:

STOCK = stock CCK 3.x
PATCH = with the patch on this comment

Test 1

≈ 1000 nodereferences on a single node, with the noderef display settings set to link

Average Page Execution time:
STOCK: ≈ 625ms
PATCH: ≈ 650ms

Longest Page Execution time:
STOCK: 886ms
PATCH: 744ms

Page Execution time after a drush cc all:
STOCK: 846ms
PATCH: 865ms

Test 2

≈ 20 nodereferences on a single node, with display settings to full node

Average Page Execution time:
STOCK: ≈ 300ms
PATCH: ≈ 300ms

Longest Page Execution time:
STOCK: 325ms
PATCH: 340ms

Page Execution time after a drush cc all
STOCK: 593ms
PATCH: 450ms

q0rban’s picture

q0rban commented
StatusFileSize
new cck-946554-7.patch1.21 KB

Whoops, forgot to set --no-prefix and --relative on that last patch. Here's a re-roll.

markdorison’s picture

markdorison
he/him
commented
Status: Needs review » Reviewed & tested by the community

Tested the patch from #7 and seems to be working as expected.

iratau’s picture

iratau commented

It is worth noting that this patch affects installations that have any modules that implement their own access functions, such as workflow_access or module_grants. There is a comment about this under the module_grants issue queue here:

http://drupal.org/node/948306

q0rban’s picture

q0rban commented

This may need to be re-tested as far as performance, benchmarking with an anonymous as well as authenticated (non-admin) user to see the performance differences between the two.