The DRUPAL-5--2 branch is result of backporting DRUPAL-6--1 in #557152: Backporting last changes to 5.x.

Recently I found that the node access feature is not working as it should be.

This is due to the fact that in D6, node_access() takes also the $account as argument while in D5 version it doesn't.

I found too that there's no easy way to emulate that in D5. Can somebody help find out how to emulate this line in 5?

  if (!node_access('view', $node, $account)) {

...

from apachesolr_nodeaccess.module?

Thanks!

CommentFileSizeAuthor
#12 apachesolr_nodeaccess-614048-4-D5.patch3.06 KBclaudiu.cristea
#11 apachesolr_nodeaccess-614048-3-D5.patch2.3 KBclaudiu.cristea
#9 apachesolr_nodeaccess-614048-2-D5.patch2.3 KBclaudiu.cristea
#8 apachesolr_nodeaccess-614048-1-D5.patch2.14 KBclaudiu.cristea
#7 apachesolr_nodeaccess-614048-D5.patch2.58 KBclaudiu.cristea

Comments

Scott Reynolds’s picture

Scott Reynolds commented

Well I believe this is on cron yes? Or when the node is being reindexed.

So this shouldn't be a problem on cron run because the cron user is anon, just remove the $account param.

Perhaps you will have to query for the node_access records as well as check user_access('view $type') by hand?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@Scott Reynolds,

Usually this runs on cron... But what's happen when cron is ran manually by admin? Maybe adding a new check for UID?

  if ($GLOBAL['user']->uid == 0) {
    if (!node_access('view', $node)) {
      ...
    }
    else {
      ...
    }
  }

This isn't very clean, but it assures that we will not index wrong values in Solr.

Perhaps you will have to query for the node_access records as well as check user_access('view $type') by hand?

Not sure what you mean...

Scott Reynolds’s picture

Scott Reynolds commented
Priority: Critical » Normal

So then this only happens when an admin runs cron. good then its not critical

http://drupal.org/node/218104

That has examples on how to securely change user

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Component: Node access » Code

Wrong post.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Component: Code » Node access

@Scott Reynolds,

Is http://drupal.org/node/218104 tested? If so, I can easily fix the issue.

Thanks for your help!

Scott Reynolds’s picture

Scott Reynolds commented

Yes its written by the security team. its the right way

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Component: Code » Node access
StatusFileSize
new apachesolr_nodeaccess-614048-D5.patch2.58 KB

Here's a patch for testing & review.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review
StatusFileSize
new apachesolr_nodeaccess-614048-1-D5.patch2.14 KB

In addition to that fix I found another issue. In function _apachesolr_nodeaccess_build_subquery(), we try to obtain access grants in D6 style:

    // Get node access grants.
    $grants = node_access_grants('view', $account);

but we have to use Drupal 5 form:

    // Get node access grants.
    $grants = node_access_grants('view', $account->uid);

Note that in D5 we pass the UID not the entire user object.

The patch was rebuilt.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new apachesolr_nodeaccess-614048-2-D5.patch2.3 KB

Add back micro-caching to anonymous account.

Scott Reynolds’s picture

Scott Reynolds commented

Only issue I would point out is

if ($original_user->uid) {

is going to cause php warnings. The original_user isn't always set. 

if (isset($original_user)) {

Would work just fine as the only time you 'set' $original_user is when the $GLOBALS['user'] isn't uid 0.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new apachesolr_nodeaccess-614048-3-D5.patch2.3 KB

Thanks @Scott Reynolds for the "PHP warning" comment...

I fixed that...

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new apachesolr_nodeaccess-614048-4-D5.patch3.06 KB

Added also the CHANGELOG.txt entry.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Fixed

Fixed in #284502.

Status: Fixed » Closed (fixed)

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