Advertisement doesn't respect node access?

The problem with this is that the ad module makes no check when retrieving ads to whether or not the user has permission to view them. In addition, the ad_access function only checks to see if the user has permission to see ads or if they are an administrator. The fix would be 1) when gathering ads, make sure to check the node access, and 2) update ad_access for the $op = view to only return false if they don't have "show advertisements" permission - letting the node access module (which the domain access module integrates with) take care of the basic view permissions.

My setup
Drupal 6.13 - Ad 6.x-2.1 - Domain 6.x-2.0-rc9

I've attached patches for the files, but here's the code change for reference as well:

ad.module - line 561

function ad_access($op, $node, $account) {
  switch ($op) {
    case 'create':
      return (user_access('create advertisements', $account) || user_access('administer advertisements'));
    case 'update':
      return (user_access('edit any advertisement', $account) || (user_access('edit own advertisements', $account) && is_ad_owner($node->nid)) || user_access('administer advertisements', $account));
    case 'delete':
      return (user_access('delete any advertisement', $account) || (user_access('delete own advertisements', $account) && is_ad_owner($node->nid)) || user_access('administer advertisements', $account));
    case 'view':
      // NSciacca: update ad_access only to return false if the user doesn't have permissions, but not return true so the domain/node access checks will happen
      // return (user_access('show advertisements', $account) || user_access('administer advertisements', $account));
      if (user_access('show advertisements', $account) !== TRUE) return false;
  }
}

adcache.inc - line 215

/**
 * Default function for retrieving list of ids.
 */
function adserve_cache_id($type, $id) {
  switch ($type) {
    case 'nids':
      $result = db_query("SELECT aid FROM {ads} WHERE adstatus = 'active' AND aid IN(%s)", $id);
      break;
    case 'tids':
      $result = db_query("SELECT a.aid FROM {ads} a INNER JOIN {term_node} n ON a.aid = n.nid WHERE a.adstatus = 'active' AND n.tid IN(%s)", $id);
      break;
    case 'default':
      $result = db_query("SELECT a.aid FROM {ads} a LEFT JOIN {term_node} n ON a.aid = n.nid WHERE a.adstatus = 'active' AND n.tid IS NULL");
      break;
    default:
      _debug_echo("Unsupported type '$type'.");
  }

  $ids = array();
  if (isset($result)) {
    while ($ad = db_fetch_object($result)) {
	
      // NSciacca: do a node access check before adding this ad to the list of available ads
      //$ids[$ad->aid] = $ad->aid;
      $ad_node = node_load($ad->aid);
      if (node_access('view',$ad_node) == TRUE) {  
	$ids[$ad->aid] = $ad->aid;
      }	
    }
  }
  return $ids;
}

I haven't tested this but I will 2nd the need for this....

Updating ticket status to reflect that there is a patch attached which needs review.

I put this in our test environment with multiple domains and it appears the ad node respects the node access rules (at least the domain node access rules) with this patch in place.
+1

Sadly this fix will only work if no ad caching is enabled, which is pretty much required if you want good performance. When ads are displayed and an ad cache is used, Drupal is not bootstrapped and thus we're unable to invoke the _access hook.

I've gone ahead and cleaned up your patch for coding style and committed here, but I'll leave this issue open to continue thinking about a complete solution:
http://drupal.org/cvs?commit=292770

Domain Blocks module might offer a better (cacheable) solution, but then you have to create an ad block per domain.

The posted solution in #3 is very bad and I recommend you not to use this solution.

In general, domain access and other node access modules work with SQL rewriting of Drupal queries. Because the ad module does db_query() calls without db_rewrite_sql(), the SQL statement cannot be rewritten and node access rules aren't respected.

The best solution adds the db_rewrite_sql() as proposed in #2 to all db_query() that are fetching nodes. Unfortunately this does not work, because the ad module stores all the nids in a separate table and queries the ads table without doing a join with the base table node. SQL rewriting does only work if the base table is node.

Example: aid = node ids
adcache.inc Line 219

$result = db_query("SELECT aid FROM {ads} WHERE adstatus = 'active' AND aid IN(%s)", $id);

No rewrite will happen if you change the statement like this:

$result = db_query(db_rewrite_sql("SELECT aid FROM {ads} WHERE adstatus = 'active' AND aid IN(%s)", $id));

Working solution
Base table is node, then make a JION to the ads table and the db_rewrite_sql() will work perfectly

$result = db_query(db_rewrite_sql("SELECT n.nid FROM {node} n INNER JOIN {ads} a ON a.aid = n.nid WHERE adstatus = 'active' AND aid IN(%s)"), $id);

Later in the code you have to change the line from aid to nid:
$ids[$ad->nid] = $ad->nid;

Attached you will find a patch against the latest version 2.2 that reverts the committed changes and implements a solution in the real Drupal way without a node_load() to check the permissions.

The best thing is, that it works with cache mode on, because it does not rely on _access hook.

I use this path on my live site. Please test it and commit it as soon as possible.

The patch above has one important typo:
$ids[$ad->aid] = $ad->nid; should be $ids[$ad->nid] = $ad->nid;

Attached you fill find a corrected patch.

Can someone review this to commit this patch?

Anyone?

Well, the first change is missing the $result =, so this would fail.

I don't use this module; I just maintain a node access module.

Is this fix, or one like it, planned for the next release of the ad module?

Ad module do not know is_ad_owner if ad_owners is not enabled, thus you patch is making ad_owners a prerequisite for ad.module, which is fun, because ad_owners depends on ad.module, thus it's a prerequisite loop.

The use of is_ad_owner in ad.module itself makes ad_owners a required module instead of optional.
Not enabling ad_owners causes the following Fatal error: Call to undefined function is_ad_owner() in ...\sites\all\modules\ad\ad.module on line 570

I've just discovered this problem on my site, and I'm surprised to see that it's been almost a year since there was any feedback on this issue. What's the status of the latest patch?