The streams should enforce node access restrictions

Each of the "entity groups" where this could be an issue has specific code to address this. For example, the code that finds users' relationships looks like this:

/**
 * Returns the entity IDs for the Acting user's %type relationships group.
 */
function activity_log_eg_acting_user_rels_type($event, $settings, $acting_uid, $ur_type) {
  // If UR is enabled and the acting user has relationships
  if (module_exists('user_relationships_api') && $acting_uid && $rels = _activity_log_get_user_relationships($acting_uid, $ur_type->rtid)) {
    // If OG is enabled and the action was in a group
    if (module_exists('og') && $node = $event->og) {
      // If the group is private
      if ($node->og_private) {
        // Reduce the list of relationships to people who are also group members
        $members = _activity_log_get_active_group_members($event);
        $rels = array_intersect($rels, $members);
      }
    }
    if ($rels) {
      return array('user' => $rels);
    }
  }
}

Additionally, 0 (zero) is a valid stream owner ID, and there could potentially be non-numeric values or negative values added in the future as well (which we already do for viewers). So even if we didn't catch the private group cases by the time they get to $owners = $results['stream_owner']; the handling your patch adds isn't quite enough.

So the real question is -- under what circumstances are people who are not group members able to see private group posts? What specific case has been missed? Specifically, what entity groups and for what event are relevant here?

Well there aren't any more hooks that would have been run by the time the system runs your code. But your patch uses og_public and right now all the entity group callbacks use og_private. Maybe switching to og_public in the entity group callbacks would work.

Hmm. Sounds like I'm going to have to dig into this one. What version of OG are you using?

We have the core node access system/db_rewrite_sql() to enforce node access so that we don't have to write our own system. Changing title to reflect that the stream should enforce any node access restriction that uses the core node access system, not just OG access restrictions.

Here's a patch that implements hook_views_pre_render() to remove activity log messages from the view result set if the user doesn't have view access on the node that's discussed in the activity log message.

Note, this assumes that an activity log message about a node will always be of the target_type 'node'.

I tested this code by

a) Creating a private group
b) Adding a private discussion to a public group

and verified that those status messages were not displayed to an anonymous user but were displayed to a privileged user, while the anonymous user was still able to see messages that she should be able to see.

It would be great to get some more thorough testing here.

@IceCreamYou, it would be great if you could confirm my assumption about target_type :).

Raising priority to "major" since, as a security issue, this would prevent a stable release.

Thanks!

With an empty() check on $view->result.

If you are looking for a simple non-OG module to confirm this thing works, I suggest http://drupal.org/project/private

(also subscribe).

Adding Commons 2.0 Release tag.

Thanks for the patch, Ezra. A couple of things:

• It kind of bothers me that this approach reduces the final result set rather than changing the query before it's executed because we end up displaying possibly fewer results than expected. However, as far as I can tell, if it is possible to avoid this problem it would require some re-architecting,⁽¹⁾ so I can live with it.
• The patch doesn't remove existing private group access controls implemented in the callbacks in activity_log.entity_groups.inc. Not a show-stopper, but it would be good to get rid of those as long as we're getting this in.
• The patch assumes that the target_type and target_id columns have been added to the results. This is not *necessarily* the case, although it would be pretty silly if they weren't. But we should make sure they're there in a Views hook that runs before the query is executed.
• Isn't there a better way to get the fields than to explicitly assume we know what the alias is (e.g. $result->activity_log_messages_target_type)? I'm pretty sure the alias is stored somewhere in the View object... something like $view->query->fields[$field]['alias']?
• With regards to the target entity: when messages are grouped, they are only grouped with other messages that use the same template (activity_log.rules.inc line 863). Since Rules events can't have multiple target types,⁽²⁾ the target_type will always be accurate in {activity_log_messages}. However, the target_id won't be useful when a message is grouped on the acting user or the action (as opposed to grouping on the target entity). Instead you need to look at the relevant events (available in a comma-separated list in the "aids" column) and use the target_id value from the {activity_log_events} table.⁽³⁾
• This might be micro-optimizing, but I think that it should be more efficient to run array_unique() on $nids than to use DISTINCT in the query... NBD, but also the fewer database commands we use, the less I have to go look up when I port the module to Drupal 7. :-P

⁽¹⁾ If we used a node access grant-style system instead of having a "viewer" column to control who can see activity messages, we *might* be able to keep it in sync with the node access control system.

⁽²⁾ Rules events have to define the types of variables they use. Activity Log assumes that the first defined variable is the target entity. Since Rules event definitions are static, the array of which variables each event uses never changes, so the target type will always be consistent.

⁽³⁾ As an example, Activity Log does this when unpacking a loaded message record in activity_log.module line 151. Note that the aids are stored with a leading and trailing comma, so you need to array_filter() or array_pop()/array_shift() after explode()ing. The aids are stored this way to make SELECT queries easier, so that for example you can do something like SELECT mid FROM {activity_log_messages} WHERE aids LIKE '%%,%d,%%' where %d is an event ID. (There might be a more efficient way to store the relationship between events and messages. However, in theory, one event could map to multiple messages and one message could also map to multiple events. In practice, no event maps to more than one message. The idea was that one event record would correspond to each time a Rules event was triggered, but the way it actually got written was that each event record corresponds to each time a Rules action is triggered. I'm sure there is an opportunity for optimization somewhere in there.)

Obviously this is an issue we'd all like to get resolved ASAP. I *think* I will be mostly available this weekend, and I'll try to remember to get on IRC when I'm near my computer. Ezra, I'd appreciate if you can address the issues above, or if you're not available this weekend I should be able to look into it.

Thanks for the thorough response.

It kind of bothers me that this approach reduces the final result set rather than changing the query before it's executed because we end up displaying possibly fewer results than expected. However, as far as I can tell, if it is possible to avoid this problem it would require some re-architecting,(1) so I can live with it.

Yeah. I think given the timeline (2.0 release of Commons on Monday) we should avoid rearchitecting this complex system and stick with the approach here. We can evaluate whether it's worth re-architecting in the upcoming Commons sprints.

The patch doesn't remove existing private group access controls implemented in the callbacks in activity_log.entity_groups.inc. Not a show-stopper, but it would be good to get rid of those as long as we're getting this in.

Agreed.

However, the target_id won't be useful when a message is grouped on the acting user or the action (as opposed to grouping on the target entity). Instead you need to look at the relevant events (available in a comma-separated list in the "aids" column) and use the target_id value from the {activity_log_events} table.

Gotcha. How can we reproduce a grouped status update?

I can give this some time later today, but given your familiarity here if you wanted to improve the patch here to account for that, it would be awesome.

Isn't there a better way to get the fields than to explicitly assume we know what the alias is (e.g. $result->activity_log_messages_target_type)? I'm pretty sure the alias is stored somewhere in the View object... something like $view->query->fields[$field]['alias']?

That sounds like it's probably true ;), but not as high priority as the other fixes here (though we should fix it eventually).

This might be micro-optimizing, but I think that it should be more efficient to run array_unique() on $nids than to use DISTINCT in the query... NBD, but also the fewer database commands we use, the less I have to go look up when I port the module to Drupal 7. :-P

DISTINCT() is a common MySQL function within Drupal, and I believe that MySQL is generally faster about this kind of data crunching than a built-in PHP function (at least that's the rule of thumb I generally use). So, I'm not sure I follow the "makes upgrade to D7" faster argument :).

We've got a friendly QA tester who will be available on Sunday, so I'd like to wrap this up one way or another today and integrate into a testable version of Commons with some fixes by tonight if possible. I think in worst case scenario, the latest patch with the ability to handle grouping would get us to the release, and we can come back and refactor afterwards.

How can we reproduce a grouped status update?

Probably the best way for this scenario would be to create a new Rule. You can probably clone or change the existing Rule that deals with creating new nodes in groups, and just change the grouping settings for each of its actions.

I committed an iteration of the patch from #9 to dev. I haven't tested it. Leaving this at "needs work" until someone can confirm that it works and the two remaining TODOs (documented in the patch) are addressed.

One stylistic nitpick on the commit in #14 is that I wouldn't do this:

+    /*
     if (module_exists('og') && $node = $event->og) {
       if ($node->og_private) {
         $members = _activity_log_get_active_group_members($event);
         $rels = array_intersect($rels, $members);
       }
     }
+     */

Commented out code makes it harder to see what's really going on. If you want to save that "just in case" then you can always go back in version control to get the version of the module from before the commit.

Here is the tree associated with the parent comment which has the code still present: http://drupalcode.org/project/activity_log.git/tree/84612ff77cc50ae8cfbb...

> I might be missing something, but there's still the problem that the last patch only addresses views, whereas there's also the connection with digests, etc.

That's an important point.

At this point, we could retrofit a similar solution onto the digest functionality. In the long term, it seems ideal to have a more integrated solution.

Probably the best way for this scenario would be to create a new Rule. You can probably clone or change the existing Rule that deals with creating new nodes in groups, and just change the grouping settings for each of its actions.

Currently when I submit one node into 2 groups, I only see one status update about the first group. I'm not sure if this is a configuration issue or a bug where activity_log only records activity for the first group. @IceCreamYou, can you confirm?

Commented out code makes it harder to see what's really going on.

Agreed, I'll get rid of it the next time I take a look at that code (hopefully soon, when I finish the TODOs remaining from this issue).

I might be missing something, but there's still the problem that the last patch only addresses views, whereas there's also the connection with digests, etc.

Committed equivalent code to Digests. Since that part isn't using Views it isn't affected by the issues I mentioned for Activity Log.

Leaving this issue at Needs Work until those issues can be fixed:

+ * TODO:
+ *   - Make sure that the aids and target_type columns have been added to the
+ *     query before running this code.
+ *   - Don't assume we know what the field aliases are;
+ *     use something like $view->query->fields[$field]['alias'] instead.

re #18, I guess that was http://drupalcode.org/project/digests.git/commit/5e00bc7 ?

It's great, in my opinion, to link from commit to issue and from issue to commit to make it easy for folks to review the work and confirm why things were changed.

Thanks for your efforts!

Yes, I meant to do that... there's an equivalent issue in the Commons queue and I posted the commit to the Commons repo there which made me forget about this one. Normally I'd link both ways.

Marking this fixed and moving the remaining work to #1260468: Refer to Views fields the right way when checking node access