Fixes #4789: group_gatekeeper() and river hide closed/invisible group content more reliably

4 files changed, 133 insertions, 39 deletions

diff --git a/engine/classes/ElggGroupItemVisibility.php b/engine/classes/ElggGroupItemVisibility.php
new file mode 100644
index 000000000..2c7e2abb4
--- /dev/null
+++ b/engine/classes/ElggGroupItemVisibility.php
@@ -0,0 +1,93 @@
+<?php
+
+/**
+ * Determines if otherwise visible items should be hidden from a user due to group
+ * policy or visibility.
+ *
+ * @class      ElggGroupItemVisibility
+ * @package    Elgg.Core
+ * @subpackage Groups
+ *
+ * @access private
+ */
+class ElggGroupItemVisibility {
+
+	const REASON_MEMBERSHIP = 'membershiprequired';
+	const REASON_LOGGEDOUT = 'loggedinrequired';
+	const REASON_NOACCESS = 'noaccess';
+
+	/**
+	 * @var bool
+	 */
+	public $shouldHideItems = false;
+
+	/**
+	 * @var string
+	 */
+	public $reasonHidden = '';
+
+	/**
+	 * Determine visibility of items within a container for the current user
+	 *
+	 * @param int $container_guid GUID of a container (may/may not be a group)
+	 *
+	 * @return ElggGroupItemVisibility
+	 *
+	 * @todo Make this faster, considering it must run for every river item.
+	 */
+	static public function factory($container_guid) {
+		// cache because this may be called repeatedly during river display, and
+		// due to need to check group visibility, cache will be disabled for some
+		// get_entity() calls
+		static $cache = array();
+
+		$ret = new ElggGroupItemVisibility();
+
+		if (!$container_guid) {
+			return $ret;
+		}
+
+		$user = elgg_get_logged_in_user_entity();
+		$user_guid = $user ? $user->guid : 0;
+
+		$container_guid = (int) $container_guid;
+
+		$cache_key = "$container_guid|$user_guid";
+		if (empty($cache[$cache_key])) {
+			// compute
+
+			$container = get_entity($container_guid);
+			$is_visible = (bool) $container;
+
+			if (!$is_visible) {
+				// see if it *really* exists...
+				$prev_access = elgg_set_ignore_access();
+				$container = get_entity($container_guid);
+				elgg_set_ignore_access($prev_access);
+			}
+
+			if ($container && $container instanceof ElggGroup) {
+				/* @var ElggGroup $container */
+
+				if ($is_visible) {
+					if (!$container->isPublicMembership()) {
+						if ($user) {
+							if (!$container->isMember($user) && !$user->isAdmin()) {
+								$ret->shouldHideItems = true;
+								$ret->reasonHidden = self::REASON_MEMBERSHIP;
+							}
+						} else {
+							$ret->shouldHideItems = true;
+							$ret->reasonHidden = self::REASON_LOGGEDOUT;
+						}
+					}
+				} else {
+					$ret->shouldHideItems = true;
+					$ret->reasonHidden = self::REASON_NOACCESS;
+				}
+			}
+			$cache[$cache_key] = $ret;
+		}
+		return $cache[$cache_key];
+	}
+}
diff --git a/engine/lib/group.php b/engine/lib/group.php
index feb1f1e7f..b81146e61 100644
--- a/engine/lib/group.php
+++ b/engine/lib/group.php
@@ -247,48 +247,42 @@ function get_users_membership($user_guid) {
 }
 
 /**
- * Checks access to a group.
+ * May the current user access item(s) on this page? If the page owner is a group,
+ * membership, visibility, and logged in status are taken into account.
  *
  * @param boolean $forward If set to true (default), will forward the page;
  *                         if set to false, will return true or false.
  *
- * @return true|false If $forward is set to false.
+ * @return bool If $forward is set to false.
  */
 function group_gatekeeper($forward = true) {
-	$allowed = true;
-	$url = '';
-
-	if ($group = elgg_get_page_owner_entity()) {
-		if ($group instanceof ElggGroup) {
-			$url = $group->getURL();
-			if (!$group->isPublicMembership()) {
-				// closed group so must be member or an admin
-
-				if (!elgg_is_logged_in()) {
-					$allowed = false;
-					if ($forward == true) {
-						$_SESSION['last_forward_from'] = current_page_url();
-						register_error(elgg_echo('loggedinrequired'));
-						forward('', 'login');
-					}
-				} else if (!$group->isMember(elgg_get_logged_in_user_entity())) {
-					$allowed = false;
-				}
 
-				// Admin override
-				if (elgg_is_admin_logged_in()) {
-					$allowed = true;
-				}
-			}
-		}
+	$page_owner_guid = elgg_get_page_owner_guid();
+	if (!$page_owner_guid) {
+		return true;
 	}
+	$visibility = ElggGroupItemVisibility::factory($page_owner_guid);
 
-	if ($forward && $allowed == false) {
-		register_error(elgg_echo('membershiprequired'));
-		forward($url, 'member');
+	if (!$visibility->shouldHideItems) {
+		return true;
 	}
+	if ($forward) {
+		// only forward to group if user can see it
+		$group = get_entity($page_owner_guid);
+		$forward_url = $group ? $group->getURL() : '';
+
+		if ($visibility->reasonHidden !== ElggGroupItemVisibility::REASON_MEMBERSHIP) {
+			$_SESSION['last_forward_from'] = current_page_url();
+			$forward_reason = 'login';
+		} else {
+			$forward_reason = 'member';
+		}
 
-	return $allowed;
+		register_error(elgg_echo($visibility->reasonHidden));
+		forward($forward_url, $forward_reason);
+	}
+
+	return false;
 }
 
 /**
diff --git a/engine/lib/views.php b/engine/lib/views.php
index b00334062..90737c260 100644
--- a/engine/lib/views.php
+++ b/engine/lib/views.php
@@ -1243,7 +1243,7 @@ function elgg_view_module($type, $title, $body, array $vars = array()) {
  * @param ElggRiverItem $item A river item object
  * @param array         $vars An array of variables for the view
  *
- * @return string|false Depending on success
+ * @return string
  */
 function elgg_view_river_item($item, array $vars = array()) {
 	// checking default viewtype since some viewtypes do not have unique views per item (rss)
@@ -1256,6 +1256,12 @@ function elgg_view_river_item($item, array $vars = array()) {
 	if (!$subject || !$object) {
 		// subject is disabled or subject/object deleted
 		return '';
+	} else {
+		// hide based on object's container
+		$visibility = ElggGroupItemVisibility::factory($object->container_guid);
+		if ($visibility->shouldHideItems) {
+			return '';
+		}
 	}
 
 	$vars['item'] = $item;
diff --git a/views/default/page/components/list.php b/views/default/page/components/list.php
index 0cf7d507c..28ed58ddf 100644
--- a/views/default/page/components/list.php
+++ b/views/default/page/components/list.php
@@ -51,14 +51,15 @@ if ($pagination && $count) {
 if (is_array($items) && count($items) > 0) {
 	$html .= "<ul class=\"$list_class\">";
 	foreach ($items as $item) {
-		if (elgg_instanceof($item)) {
-			$id = "elgg-{$item->getType()}-{$item->getGUID()}";
-		} else {
-			$id = "item-{$item->getType()}-{$item->id}";
+		$li = elgg_view_list_item($item, $vars);
+		if ($li) {
+			if (elgg_instanceof($item)) {
+				$id = "elgg-{$item->getType()}-{$item->getGUID()}";
+			} else {
+				$id = "item-{$item->getType()}-{$item->id}";
+			}
+			$html .= "<li id=\"$id\" class=\"$item_class\">$li</li>";
 		}
-		$html .= "<li id=\"$id\" class=\"$item_class\">";
-		$html .= elgg_view_list_item($item, $vars);
-		$html .= '</li>';
 	}
 	$html .= '</ul>';
 }