Respect user enumeration settings in user status lists

So far, the functions to find user statuses listed didn't respect user enumeration settings (`shareapi_allow_share_dialog_user_enumeration` and `shareapi_restrict_user_enumeration_to_group` core app settings). Fixes: #27122 Signed-off-by: Jonas Meurer <jonas@freesources.org>
nextcloud · Jul 28, 2021 · 6ef3218 · 6ef3218
1 parent a71294e
commit 6ef3218
Show file tree

Hide file tree

Showing 3 changed files with 162 additions and 5 deletions.
diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php
@@ -54,7 +54,7 @@ public function __construct(IDBConnection $db) {
 	 * @param int|null $offset
 	 * @return UserStatus[]
 	 */
-	public function findAll(?int $limit = null, ?int $offset = null):array {
+	public function findAll(?int $limit = null, ?int $offset = null): array {
 		$qb = $this->db->getQueryBuilder();
 		$qb
 			->select('*')
@@ -116,13 +116,51 @@ public function findByUserId(string $userId):UserStatus {
 	 * @param array $userIds
 	 * @return array
 	 */
-	public function findByUserIds(array $userIds):array {
+	public function findByUserIds(array $userIds, ?int $limit = null, ?int $offset = null):array {
 		$qb = $this->db->getQueryBuilder();
 		$qb
 			->select('*')
 			->from($this->tableName)
 			->where($qb->expr()->in('user_id', $qb->createNamedParameter($userIds, IQueryBuilder::PARAM_STR_ARRAY)));
 
+		if ($limit !== null) {
+			$qb->setMaxResults($limit);
+		}
+		if ($offset !== null) {
+			$qb->setFirstResult($offset);
+		}
+
+		return $this->findEntities($qb);
+	}
+
+
+	/**
+	 * @param array $userIds
+	 * @param int|null $limit
+	 * @param int|null $offset
+	 * @return array
+	 */
+	public function findRecentByUserIds(array $userIds, ?int $limit = null, ?int $offset = null): array {
+		$qb = $this->db->getQueryBuilder();
+
+		$qb
+			->select('*')
+			->from($this->tableName)
+			->orderBy('status_timestamp', 'DESC')
+			->where($qb->expr()->in('user_id', $qb->createNamedParameter($userIds, IQueryBuilder::PARAM_STR_ARRAY)))
+			->andWhere($qb->expr()->orX(
+				$qb->expr()->notIn('status', $qb->createNamedParameter([IUserStatus::ONLINE, IUserStatus::AWAY, IUserStatus::OFFLINE], IQueryBuilder::PARAM_STR_ARRAY)),
+				$qb->expr()->isNotNull('message_id'),
+				$qb->expr()->isNotNull('custom_icon'),
+				$qb->expr()->isNotNull('custom_message')));
+
+		if ($limit !== null) {
+			$qb->setMaxResults($limit);
+		}
+		if ($offset !== null) {
+			$qb->setFirstResult($offset);
+		}
+
 		return $this->findEntities($qb);
 	}
 

diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php
@@ -35,6 +35,9 @@
 use OCA\UserStatus\Exception\StatusMessageTooLongException;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\IConfig;
+use OCP\IGroupManager;
+use OCP\IUserSession;
 use OCP\UserStatus\IUserStatus;
 
 /**
@@ -56,6 +59,18 @@ class StatusService {
 	/** @var EmojiService */
 	private $emojiService;
 
+	/** @var IUserSession */
+	private $userSession;
+
+	/** @var IGroupManager */
+	private $groupManager;
+
+	/** @var bool */
+	private $shareeEnumeration;
+
+	/** @var bool */
+	private $shareeEnumerationInGroupOnly;
+
 	/**
 	 * List of priorities ordered by their priority
 	 */
@@ -88,17 +103,27 @@ class StatusService {
 	 *
 	 * @param UserStatusMapper $mapper
 	 * @param ITimeFactory $timeFactory
-	 * @param PredefinedStatusService $defaultStatusService,
+	 * @param PredefinedStatusService $defaultStatusService
 	 * @param EmojiService $emojiService
+	 * @param IUserSession $userSession
+	 * @param IGroupManager $groupManager
+	 * @param IConfig $config
 	 */
 	public function __construct(UserStatusMapper $mapper,
 								ITimeFactory $timeFactory,
 								PredefinedStatusService $defaultStatusService,
-								EmojiService $emojiService) {
+								EmojiService $emojiService,
+								IUserSession $userSession,
+								IGroupManager $groupManager,
+								IConfig $config) {
 		$this->mapper = $mapper;
 		$this->timeFactory = $timeFactory;
 		$this->predefinedStatusService = $defaultStatusService;
 		$this->emojiService = $emojiService;
+		$this->userSession = $userSession;
+		$this->groupManager = $groupManager;
+		$this->shareeEnumeration = $config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
+		$this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
 	}
 
 	/**
@@ -107,6 +132,19 @@ public function __construct(UserStatusMapper $mapper,
 	 * @return UserStatus[]
 	 */
 	public function findAll(?int $limit = null, ?int $offset = null): array {
+		// Return empty array if user enumeration is disabled
+		if (!$this->shareeEnumeration) {
+			return [];
+		}
+
+		// Return only users from common groups if user enumeration is limited to groups
+		if ($this->shareeEnumerationInGroupOnly) {
+			$userIds = $this->userIdsByGroups();
+			return array_map(function ($status) {
+				return $this->processStatus($status);
+			}, $this->mapper->findByUserIds($userIds, $limit, $offset));
+		}
+
 		return array_map(function ($status) {
 			return $this->processStatus($status);
 		}, $this->mapper->findAll($limit, $offset));
@@ -118,6 +156,19 @@ public function findAll(?int $limit = null, ?int $offset = null): array {
 	 * @return array
 	 */
 	public function findAllRecentStatusChanges(?int $limit = null, ?int $offset = null): array {
+		// Return empty array if user enumeration is disabled
+		if (!$this->shareeEnumeration) {
+			return [];
+		}
+
+		// Return only users from common groups if user enumeration is limited to groups
+		if ($this->shareeEnumerationInGroupOnly) {
+			$userIds = $this->userIdsByGroups();
+			return array_map(function ($status) {
+				return $this->processStatus($status);
+			}, $this->mapper->findRecentByUserIds($userIds, $limit, $offset));
+		}
+
 		return array_map(function ($status) {
 			return $this->processStatus($status);
 		}, $this->mapper->findAllRecent($limit, $offset));
@@ -328,6 +379,26 @@ public function removeUserStatus(string $userId): bool {
 		return true;
 	}
 
+	/**
+	 * @return string[]
+	 */
+	private function userIdsByGroups(): array {
+		$usersByGroups = [];
+
+		$currentUserGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
+		foreach ($currentUserGroups as $userGroupId) {
+			$usersInGroup = $this->groupManager->displayNamesInGroup($userGroupId);
+			foreach ($usersInGroup as $userId => $displayName) {
+				$userId = (string)$userId;
+				if (!in_array($userId, $usersByGroups, true)) {
+					$usersByGroups[] = $userId;
+				}
+			}
+		}
+
+		return $usersByGroups;
+	}
+
 	/**
 	 * Processes a status to check if custom message is still
 	 * up to date and provides translated default status if needed

diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php
@@ -38,6 +38,9 @@
 use OCA\UserStatus\Service\StatusService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\IConfig;
+use OCP\IGroupManager;
+use OCP\IUserSession;
 use OCP\UserStatus\IUserStatus;
 use Test\TestCase;
 
@@ -55,6 +58,9 @@ class StatusServiceTest extends TestCase {
 	/** @var EmojiService|\PHPUnit\Framework\MockObject\MockObject */
 	private $emojiService;
 
+	/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
+	private $config;
+
 	/** @var StatusService */
 	private $service;
 
@@ -65,10 +71,25 @@ protected function setUp(): void {
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->predefinedStatusService = $this->createMock(PredefinedStatusService::class);
 		$this->emojiService = $this->createMock(EmojiService::class);
+
+		$userSession = $this->createMock(IUserSession::class);
+		$groupManager = $this->createMock(IGroupManager::class);
+		$groupManager->method('getUserGroupIds')
+			->willReturn(['group1', 'group2']);
+		$groupManager->method('displayNamesInGroup')
+			->willReturnMap([
+				['group1', ['user1' => 'user1']],
+				['group2', ['user2' => 'user2']],
+			]);
+		$this->config = $this->createMock(IConfig::class);
+
 		$this->service = new StatusService($this->mapper,
 			$this->timeFactory,
 			$this->predefinedStatusService,
-			$this->emojiService);
+			$this->emojiService,
+			$userSession,
+			$groupManager,
+			$this->config);
 	}
 
 	public function testFindAll(): void {
@@ -80,6 +101,11 @@ public function testFindAll(): void {
 			->with(20, 50)
 			->willReturn([$status1, $status2]);
 
+		$this->config->expects($this->once())
+			->method('getAppValue')
+			->with('core', 'shareapi_allow_share_dialog_user_enumeration')
+			->willReturn('yes');
+
 		$this->assertEquals([
 			$status1,
 			$status2,
@@ -95,12 +121,34 @@ public function testFindAllRecentStatusChanges(): void {
 			->with(20, 50)
 			->willReturn([$status1, $status2]);
 
+		$this->config->expects($this->once())
+			->method('getAppValue')
+			->with('core', 'shareapi_allow_share_dialog_user_enumeration')
+			->willReturn('yes');
+
 		$this->assertEquals([
 			$status1,
 			$status2,
 		], $this->service->findAllRecentStatusChanges(20, 50));
 	}
 
+	public function testFindAllRecentStatusChangesNoEnumeration(): void {
+		$status1 = $this->createMock(UserStatus::class);
+		$status2 = $this->createMock(UserStatus::class);
+
+		$this->mapper->expects($this->never())
+			->method('findAllRecent')
+			->with(20, 50)
+			->willReturn([$status1, $status2]);
+
+		$this->config->expects($this->once())
+			->method('getAppValue')
+			->with('core', 'shareapi_allow_share_dialog_user_enumeration')
+			->willReturn('no');
+
+		$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
+	}
+
 	public function testFindByUserId(): void {
 		$status = $this->createMock(UserStatus::class);
 		$this->mapper->expects($this->once())