The notify module fails to find nodes/comments for which the anonymous user (cron) does not have permission. This is a result of what I believe is a code error in _notify_send() in notify.inc, lines 101 and 108.

The way I replicated this bug was by using taxonomy_access and creating a term which was accessible only to the role "private". A user with access to "private" should in fact be notified of new content created there. But no notifications are sent to anyone because the anonymous user (cron) doesn't have access and is denied by the rewrite in db_rewrite_sql().

Instead of

$nresult = db_query(db_rewrite_sql('SELECT n.nid, n.body, n.type, n.title, n.promote, n.moderate, n.teaser, n.created, n.changed, u.name FROM {node} n INNER JOIN {users} u ON n.uid = u.uid WHERE n.status = 1 AND n.created > %d ORDER BY n.created'), $period);

which uses db_rewrite_sql, which then denies permissions to all nodes not accessible to the anonymous user,

$nresult = db_query('SELECT n.nid, n.body, n.type, n.title, n.promote, n.moderate, n.teaser, n.created, n.changed, u.name FROM {node} n INNER JOIN {users} u ON n.uid = u.uid WHERE n.status = 1 AND n.created > %d ORDER BY n.created', $period);

works very well.

Similarly, at line 108,

$cresult = db_query(db_rewrite_sql('SELECT c.nid, c.cid, c.subject, c.pid, u.name FROM {comments} c INNER JOIN {users} u ON u.uid = c.uid WHERE c.status = 0 AND c.timestamp > %d ORDER BY c.nid, c.timestamp', 'c'), $period);

should be replaced with:

$cresult = db_query('SELECT c.nid, c.cid, c.subject, c.pid, u.name FROM {comments} c INNER JOIN {users} u ON u.uid = c.uid WHERE c.status = 0 AND c.timestamp > %d ORDER BY c.nid, c.timestamp', $period);

Of course, there's the issue of people without permissions on a private node getting notified about content they should not have access to, so this query should be improved just a bit to do the "where" join that db_rewrite_sql would have done in _node_access_where_sql().

CommentFileSizeAuthor
#5 notify_5.patch1.37 KBPéter Cseke

Comments

merlinofchaos’s picture

merlinofchaos commented

I have an easier fix, though I'm not sure it's technically proper to do this:

Before the first query in _notify_send()

  global $user;
  $restore_user = $user; // save for later restoration.

And after the loop:

  $user = $restore_user; // put it back

This causes the query to run as the user, meaning permissions should be properly obeyed.

killes@www.drop.org’s picture

killes@www.drop.org commented

I've copied the solution used in mailhandler for the 4.7 upgrade. Anybody wants to do a backport?

Péter Cseke’s picture

Péter Cseke commented

It seems that the #1 fix is not enough when node_privacy_byrole is installed, as some users don't receive their notifications. I'm not yet sure if this is a node_privacy_byrole bug, or a bug from another module, however I just wanted to raise that even with the fix the notifications are not always working properly.

Péter Cseke’s picture

Péter Cseke commented

Along with the #1 patch, this must also be inserted at the start of the loop:

    $user->roles = array();
    $result = db_query('SELECT r.rid, r.name FROM {role} r INNER JOIN {users_roles} ur ON ur.rid = r.rid WHERE ur.uid = %d', $user->uid);
    while ($role = db_fetch_object($result)) {
      $user->roles[$role->rid] = $role->name;
    }

This way the notify will work even with node_privacy_byrole installed (see #3).

Péter Cseke’s picture

Péter Cseke commented
StatusFileSize
new notify_5.patch1.37 KB
killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Active » Fixed

I've backported the mailhandler solution from HEAD to 4.6. This should be in the tarball by tomorrow.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)