Subscriptions not respecting Subdomains

Do you have an issue reference for Subscriptions?

The likely culprit is a problem with the use of hook_db_rewrite_sql() in Susbscriptions module.

I looked through the code in CVS. IMO, this is a bug in the Subscriptions code.

Not using db_rewrite_sql() has the effect, I believe, of not respecting node access modules such as Organic Groups and Domain Access.

DA relies on http://api.drupal.org/api/function/node_db_rewrite_sql/5 to enforce its access rules.

If Subscriptions never invokes db_rewrite_sql() when preparing node lists, then node access rules will not be invoked at the query level.

You should point the Subscriptions maintainer(s) to http://api.drupal.org/api/group/node_access/5, specifically:

When adding a node listing to your module, be sure to use db_rewrite_sql() to add the appropriate clauses to your query for access checks.

Note that you should be testing Subscriptions 2 beta3 -- other versions are discontinued.

Issue link? I couldn't find it yesterday.

This is a Subscriptions issue. Closing.

We'll look into it -- the documentation is pretty scarce...

@salvis -- which documentation?

@agentrickard: the documentation on db_rewrite_sql() and what it's supposed to do; I'm trying to improve it a bit at http://drupal.org/node/204638...

I've added all those db_rewrite_sql() calls as you suggested, but I don't really believe that it will help, because the OP said the problem was getting notifications that he shouldn't be getting. At that point we don't "list" nodes, but we retrieve one by one, and we call node_access() on each one.

@Daryl: please try BETA7...

@salvis

Got it. Let me know if there is anything specific you would like me to review.

I'm not sure how to interpret your post. Are you now getting too little notifications, i.e. are there users that should get them and don't?

That sounds like all users except the admin are failing node access checks (user 1 and users with 'administer nodes' are not subject to node_access rules).

When Subscriptions generates its content lists, which user does it act as?

@Daryl: can you look inside your subscriptions_queue table whether the notifications are queued (per recipient_uid) or not? You should get 4 rows for every new post, if you have 4 subscribed users.

I'm sorry, I wasn't precise enough...

MySQL returned an empty result set (i.e. zero rows).

This indicates that the table was empty at that point. Now, please create a new post and recheck before cron runs, what's in the table. There must be at least one row, because cron will later on send the administrator's notification.

When there are rows, you should see the values in them (this is now suppressed, because you don't have any).

Thanks, we're making progress. I've opened a new issue for the comments.module dependency.

You're seeing three rows? That's unexpected -- I need to see more details...

Please try the following:

1. Make sure cron has run.
2. Instead of SELECT COUNT( * ) AS `Rows` , `uid` FROM `subscriptions_queue` ... run the following query:
   

   SELECT * FROM subscriptions_queue;
   

   This will show you the entire table. Since cron has run, it should come back with 0 rows.

3. Create another post.
4. Rerun the query above and show me what you get.

Thanks for hanging in there...

Sorry for the delay -- I had pretty much completed my reply 12 hours ago but then had to reboot my computer and lost the message...

What we're seeing is this:

sqid uid name        mail                 language module field value author_uid send_interval digest load_args load_function                   last_sent
86   4   daddy       example@example.com           node   tid   4777  -1         1             0      336       subscriptions_content_node_load 0
85   3   littledaddy example2@example.com          node   tid   4777  -1         1             0      336       subscriptions_content_node_load 0
84   1   Bigdaddy    example3@example.net          node   tid   4777  -1         1             0      336       subscriptions_content_node_load 0

Users 1, 3, and 4 each have a notification queued for node 336, based on a subscription to term 4777.

Please check whether 3 and 4 really can look at node/336! (#1)

The queueing side seems to be working just fine, and I was going to say that node_access('view', (node336)) must be failing for users 3 and 4.

After you've checked #1, then try clicking the [Rebuild permissions] button on admin/content/node-settings and see whether that makes any difference.

There must be some other element in the picture that we haven't identified yet, because sending notifications hasn't been a problem for the last few betas anymore...

OK, this error seems to be the problem.

Is this with or without the Domain Access "multiple node access" patch?

And is there any way you can capture the entire query. If this is the whole query, it is malformed (there is no SELECT).

INNER JOIN node_access na ON na.nid = node.nid 
WHERE (na.grant_view >= 1 AND 
((na.gid = 0 AND na.realm = 'all') OR 
(na.gid = 0 AND na.realm = 'domain_site') OR 
(na.gid = 0 AND na.realm = 'domain_id') OR 
(na.gid = 2 AND na.realm = 'workflow_access') OR 
(na.gid = 3 AND na.realm = 'workflow_access') OR 
(na.gid = 3 AND na.realm = 'workflow_access_owner'))) 
LIMIT 0, 10

This section of the query is odd, as well, as 'node' is typically prefixed as 'n':

na.nid = node.nid 

By the way, I checked the code for Subscriptions 5.x.2 beta 7 and Workflow module. Neither they nor Domain Access is responsible for the 'node.nid' portion of the query.

What other modules do you have installed?

Wow, interesting news! So you have one or more node access module(s) installed that define the following realms:

domain_site
domain_id
workflow_access
workflow_access_owner

Please find out what those modules are (for example search the source code for those strings) and post the links to their project pages.

Find out what action is triggering the error. Make sure you can reproduce it. Then replace BETA7 with BETA5 (don't create or delete any users while you have BETA5 installed) and check if the error goes away. Then return to BETA7. If the error goes away with BETA5, then I need to investigate further. Otherwise it's unlikely that Subscriptions is part of the problem.

@salvis

These all come from either Domain Access or WorkFlow Access.

domain_site
domain_id
workflow_access
workflow_access_owner

http://drupal.org/project/domain
http://drupal.org/project/workflow

However, the error seems to be coming from a db_rewrite_sql() call, not a node access hook.

I am beginning to suspect that something other than Subscriptions is the issue here.

@DarylJames

I think the error you are seeing is being truncated by Drupal. Do you have access to your server error logs? I suspect a more complete error message would be reported there.

The other _best_ way to check is to download and install the Devel module. Then turn on Query Tracking. It will print all queries (and the modules that invoke them) at the bottom of your pages.

http://drupal.org/project/devel

It's not surprising that you didn't get this error as user 1, because user 1 bypasses all access checks. The code that caused the problems is never executed when you're user 1. OTOH, when Subscriptions sends out the notifications, it impersonates the receiving user in order to check whether that user really has access to the given node. So, at that point access checks are run, and if they return FALSE, notifications would not be sent.

Your BETA5 tests and your latest findings confirm that it's not a db_rewrite_sql() issue that we're chasing. To check the other end, here's what you can do: locate this function in subscriptions_mail.module

function _subscriptions_content_access($load_function, $load_args, $node) {
  if (($load_function == 'subscriptions_content_node_load' || ($load_function == 'subscriptions_content_comment_load' && $node->_comments)) &&
    $node->status && node_access('view', $node)) {
    // We vote 'yes'. Other modules might vote 'no' and then that wins.
    return TRUE;
  }
}

and insert three lines (don't touch any of the existing lines!) to make it look like

function _subscriptions_content_access($load_function, $load_args, $node) {
global $user;
  if (($load_function == 'subscriptions_content_node_load' || ($load_function == 'subscriptions_content_comment_load' && $node->_comments)) &&
    $node->status && node_access('view', $node)) {
    // We vote 'yes'. Other modules might vote 'no' and then that wins.
watchdog('subs debug', "_sca returns TRUE for node $node->nid, user $user->uid.");
    return TRUE;
  }
watchdog('subs debug', "_sca: node_access('view', $node->nid) returns ". node_access('view', $node) ." for user $user->uid.");
}

Insert these lines, create a post (as before) and run cron, then I'd expect you to see a TRUE for user 1, and 0 being returned from node_access() for users 3 and 4.

@agentrickard: do you by any chance do any caching of grants without looking at $user, so that Subscriptions' user impersonation would always test the cron user (other than user 1)?

BTW, you might be interested in my proposal at http://drupal.org/node/205317...

@Daryljames: the fact that you're still seeing domain_all and domain_id access entries for your nodes means that at least parts of Domain Access are still running (try Rebuild Permissions just to be sure...).

@salvis

I think you may have found the issue, and I apologize for not seeing it earlier.

do you by any chance do any caching of grants without looking at $user, so that Subscriptions' user impersonation would always test the cron user (other than user 1)?

By design, Domain Access does not set user-specific grants. It sets _domain_ specific grants. That is, every user has the exact same grant, based on the currently active domain.

So the user, in this case, would have permissions for whatever domain that CRON runs from.

For example:

Cron from http://example.com/cron.php, where example.com == domain_id 1, the user grants are:

$grants['domain_id'] = 1;

But if you run cron from http://one.example,com/cron.php. where one.example.com == domain id 4, the grants are:

$grants['domain_id'] = 4;

Make sense?

There is a mechanism in Domain Access that allows these grants to be altered -- or bypassed -- based on the current path. Or we could do so based on the current script (cron.php instead of index.php). But that would simply allow DA to grant access to nodes across all domains.

One solution is to run cron from each domain, but this would create multiple emails, which is not the behavior we want. Another solution is to use hook_domaincron(), an untested function that is designed to allow a cron function to be invoked from each domain.

In this case, we might write a subscriptions_domaincron() that functions instead of (or in addition to) the normal subscriptions_cron function.

The answer may be that the two modules are not compatible.

FWIW, the Domain Strict module does check the $user in order to make its grants. So that might solve the original problem, but it also might create new ones for Daryljames.

Ah, we've finally solved the mystery! That's why there's that strange

0 domain_all 0 1 0 0

entry.

One solution is to run cron from each domain, but this would create multiple emails, which is not the behavior we want.

On the contrary, the first one to run would empty the queue, because queue items that fail the access check are discarded. It has to work that way...

Are users assigned to domains? Can you check against $user->domain when running from cron.php? (Would this also work when running cron manually from the status page?)

Actually, I think domain_all is the solution to this problem, and it was designed to address this kind of issue.

Here's the trick. We do not normally want to check $user->domains in order to grant access. The business rule behind Domain Access is that all users have the permissions granted to them based on the domain they are viewing, not who they are.

Domain Strict changes that, optionally, by checking the $user->domains array. However, that probably isn't the solution.

'domain_all' is a special grant that was designed for just this type of situation. On certain paths, like 'user/UID/track' or 'search' 'mysite/UID' -- cases where we are aggregating user content across all domains, we invoke _only_ the domain_all grant, which is a substitute for the default 'all' grant.

domain_all thus allows all nodes to be searched from all domains. Or MySite content to appear regardless of the active domain. (The paths that this works for is configurable in Domain Access under the 'Special Page Requests' settings option.)

I think the fix is to run a check inside the Domain Access module, something similar to:

if ($_SERVER['script'] == 'cron.php') {
  domain_grant_all();
}

The above is psuedo-code, but it would have the effect of ignoring DA restrictions during cron runs, which is, I believe the behavior we want.

That is, unless the behavior desired is, in fact, to send a subscription notice from _each_ distinct domain. In that case, the current behavior is acceptable, and we just need a script (external to Drupal) that call cron from each active domain.

The domain_all functionality is documented, but I just noticed that I have not documented this cron issue.

What I need to test is a setting that lets the admin enable 'domain_all' when cron.php is running. That should fix this issue. I should have some code later tonight.

I also want to thank both of you for digging in on this issue. This is an edge case that would likely not have been found without luck, or the work you have put in.

My pleasure! You prompted me to add the db_rewrite_sql() calls, so Subscriptions has gotten something out of this, too.

That is, unless the behavior desired is, in fact, to send a subscription notice from _each_ distinct domain. In that case, the current behavior is acceptable, and we just need a script (external to Drupal) that call cron from each active domain.

Again, I don't think this could work, because the first cron that comes along would either send or wipe the entire content of the queue and leave it empty, so no other subdomain would send out any notifications.

For people who do testing, it would be great if it could also work when hook_cron is invoked by clicking the link on admin/logs/status -- otherwise we'll still keep getting support calls about Subscriptions not working...

Good luck with Domain Access! Hope it works out as planned and remember, Daryl wants to go live in 10 days... :-)

P.S. Thanks for the support @ Devel

This ought to do it. I don't have a proper patch because I have some other changes RTBC.

Inside domain_grant_all(), add this logic:

    // Search is the easy case, so we check it first.
    $search = variable_get('domain_search', 0);
    if ($search && arg(0) == 'search') {
      $grant = TRUE;
    }
    else {
      // We check the paths registered by the special pages setting.

Change to:

    // Search is the easy case, so we check it first.
    $search = variable_get('domain_search', 0);
    if ($search && arg(0) == 'search') {
      $grant = TRUE;
    }
    // On cron runs, we normally have to disable Domain Access.  See http://drupal.org/node/197488.
    else if (variable_get('domain_cron_rule', 1)) {
      $script = array_pop(explode('/', request_uri()));
      if ($script == 'cron.php') {
        $grant = TRUE;
      }
    }
    else {
      // We check the paths registered by the special pages setting.
}

And that should solve the issue. With this in place, Domain Access will simply step out of the way with regard to node_access requests on cron runs.

@Daryljames
I missed the #42 comment. You have a couple of options, I think.

You could, in theory, use Domain Prefix to create separate Subscriptions tables for each subdomain. That, coupled with running cron on each domain, would perform as you expect.

The issue, really, is that this is an edge case that the DA module wasn't really designed to handle.

You could, I think, write a small module that uses hook_domaingrants() in order to handle these rules -- which is why I suggested Domain User.

It might be possible to determine an additional grant based on the user's subscriptions, and use those when cron is running, in a manner similar to the code above.

Note that I made the cron handler a settings option, to support both models.

Committed to HEAD.

That may be a problem with db_rewrite_sql(), then. I wonder which user the cron is acting as.

@Daryljames: I seem to remember that you said before that you had Domain Access disabled, but you were still seeing Domain Access grants in Devel's node_access entries table. Can you confirm or deny that? List the grants that you're seeing.

It would be odd if it were true and might shed some light on the problem...

Right, but the grants being in the table are not relevant to the way that the access query is written.

Only modules that implement hook_node_grants() get added to the query, so even legacy records in the database would not be checked. If Domain Access is not active, domain_node_grants() will not fire and add checks to db_rewrite_sql().

Those records should still be removed, however, and can be by rebuilding the node access table at:

Admin > Content > Post settings.

Hit the big 'Rebuild permissons' button.

I think he said he'd done that, and the records still stayed. Daryl?

Are you sure they're not being used? Aren't they active if devel_node_access is showing them? Are we talking db_rewrite_sql() here or node_access()?

Just thinking aloud, no need to answer...

I you have access to your database directly, the {node_access} table should look like this. Totally clean:

0 0 all 1 0 0

From what you describe, I suspect it looks like:

2 0 all 1 0 0

Where 2 is the node id for "Crappy's auto repair." Or similar. This line would mean that all users _only_ have access to node 2, which would explain what you are seeing.

Run these queries:

DELETE FROM node_access;
INSERT INTO node_access VALUES(0, 0, 'all', 1, 0, 0);

If you must, you can drop this into a module file temporarily. In which case they are:

db_query("DELETE FROM {node_access}");
db_query("INSERT INTO {node_access} VALUES(0, 0, 'all', 1, 0, 0)");

OK, that means that you now have _no_ access restrictions on your content. If you had any (the 46), they have been removed.

Now try to "Rebuild permissions" at Admin > Content > Post settings.

@Daryljames: You're right, the squid index shouldn't be there, thanks! I'm removing it from subscriptions.install and you can drop it, but I don't think it makes any functional difference; it's just unnecessarily burning CPU cycles.

@Daryl

Please open a new issue for Domain Access rc3 installation. This one is getting very confusing to follow.

There is also a bug in rc3 that might account for this behavior as well. So if you upgraded to Domain Access rc3, you will also want this patch: http://drupal.org/node/208550

Do _not_ create separate {node_access} tables for each domain. That will totally break the functionality.