The handler for the edit and delete links fields runs a node access check using a simplified version of the node object. This works so long as one is using only core node access modules. Adding another module such as og means that the node_access check can be incorrect without providing it with the fully loaded node.

To reproduce:
Enable og and set so that group administrators can edit group nodes which their role cannot otherwise edit.
Make a view with an edit or delete link field.
Log in as a group administrator and look at the view.
The edit / delete links do not show up despite that the user has access to edit / delete the nodes in their group.

CommentFileSizeAuthor
#11 mw.patch3.09 KBmoshe weitzman
#6 mw.patch2.63 KBmoshe weitzman
views-node-access.patch2.38 KBjody lynn

Comments

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Needs work

I am really, really unwilling to do a full node_load here. The weight of node_load is really, really heavy, and loading it just to do an access check on an 'edit' link is a major cost in performance.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented

Agreed that it's bad for performance, but if node_access expects a loaded node I'm not sure how we could ensure an accurate result without passing it one.

Shai’s picture

Shai commented

This is a serious issue.

Maybe it can be solved outside of the Views module. Does Views respect Drupal core roles system with regard to the edit/delete links better than it does OG's pseudo role of "admin"? If that is the case, then someone who cares about this issue might solve the problems with og user roles.

http://drupal.org/project/og_user_roles

I haven't used it, but one of its functions as described on the project page is to automatically assign a Drupal role for group admins.

Would this work?

Shai

merlinofchaos’s picture

merlinofchaos commented

Yes, the pseudo node that I created works for the basic access setup. It's possible that only a little more data needs to be added to get og to recognize it as well, I'm not actually sure what needs to happen there. Maybe if Moshe sees this issue he can chime in. If we can get away with adding one more field that should've been there, I'll be happier.

Shai’s picture

Shai commented

Thanks Earl.

I've written to Moshe inviting him to visit this issue.

Shai

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Needs review
StatusFileSize
new mw.patch2.63 KB

I took a look at node_access(). The pseudonode that we setup should be adequate for most circumstances, including OG. When it isn't, beware!

I did notice that $node->format should definitely be in the pseudonode but currently isn't. Would be great if Jody could try the attached patch and see if it fixes your problem. If you have group nodes authored in a non default input format, this patch should help.

Views2 already has $format so no patch needed there.

merlinofchaos’s picture

merlinofchaos commented

Oh!! Nuts, I totally didn't even see that this was for 1.x -- yea, the 'format' turned out to be a key piece missing from the pseudonode. That definitely needs to get put into the next version here.

sun: +1 to this patch.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Needs review » Needs work

No unfortunately that patch didn't solve the problem. I can try to dig further into node_access to try to find out what exactly it needs from the node to work.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Needs work » Closed (duplicate)

Ok, I tracked it down and found the same solution that was committed by this issue/patch #166608: Edit links do not properly respect access modules in view commited to DRUPAL-5 (it was the lack of $node->status preventing node_access from working). We are using the latest stable (5-1.6) where the problem still exists. Hopefully a newer stable will be released soon.

Thanks for your help.

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Status: Closed (duplicate) » Active

Only thing is that the solution in #166608: Edit links do not properly respect access modules in view might be better if it was more like Moshe's patch above, adding 'status' to the additional fields.
I think from what I'm seeing in node_access I disagree that it's safe to just set $node->status to 1. I think that node_access won't ignore unpublished nodes unless it specifically knows they don't have a status of 1.

from node_status:

if ($op != 'create' && $node->nid && $node->status) {
    $grants = array();
    foreach (node_access_grants($op) as $realm => $gids) {
      foreach ($gids as $gid) {
        $grants[] = "(gid = $gid AND realm = '$realm')";
      }
    }
moshe weitzman’s picture

moshe weitzman commented
Status: Active » Needs review
StatusFileSize
new mw.patch3.09 KB

You are correct, Jody. Here is a patch which fixes that errant commit. Please test it out and confirm. I didn't actually test it but I think it will work fine.

esmerel’s picture

esmerel commented
Status: Needs review » Closed (won't fix)

At this time, only security fixes will be made to the 5.x version of Views.