Skip to content

Commit

Permalink
Respect user enumeration settings in user status lists
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@mejo-
mejo- committed Jul 30, 2021
1 parent a90bf7e commit f048df1
Show file tree
Hide file tree
Showing 4 changed files with 177 additions and 7 deletions.
42 changes: 40 additions & 2 deletions apps/user_status/lib/Db/UserStatusMapper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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('*')
Expand Down Expand Up @@ -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);
}

Expand Down
75 changes: 73 additions & 2 deletions apps/user_status/lib/Service/StatusService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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
*/
Expand Down Expand Up @@ -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';
}

/**
Expand All @@ -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));
Expand All @@ -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));
Expand Down Expand Up @@ -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
Expand Down
63 changes: 62 additions & 1 deletion apps/user_status/tests/Unit/Service/StatusServiceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -55,6 +58,15 @@ class StatusServiceTest extends TestCase {
/** @var EmojiService|\PHPUnit\Framework\MockObject\MockObject */
private $emojiService;

/** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */
private $userSession;

/** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */
private $groupManager;

/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
private $config;

/** @var StatusService */
private $service;

Expand All @@ -65,10 +77,31 @@ protected function setUp(): void {
$this->timeFactory = $this->createMock(ITimeFactory::class);
$this->predefinedStatusService = $this->createMock(PredefinedStatusService::class);
$this->emojiService = $this->createMock(EmojiService::class);

$this->userSession = $this->createMock(IUserSession::class);
$this->groupManager = $this->createMock(IGroupManager::class);
$this->groupManager->method('getUserGroupIds')
->willReturn(['group1', 'group2']);
$this->groupManager->method('displayNamesInGroup')
->willReturnMap([
['group1', ['user1' => 'user1']],
['group2', ['user2' => 'user2']],
]);
$this->config = $this->createMock(IConfig::class);

$this->config->method('getAppValue')
->willReturnMap([
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'],
['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no']
]);

$this->service = new StatusService($this->mapper,
$this->timeFactory,
$this->predefinedStatusService,
$this->emojiService);
$this->emojiService,
$this->userSession,
$this->groupManager,
$this->config);
}

public function testFindAll(): void {
Expand Down Expand Up @@ -101,6 +134,34 @@ public function testFindAllRecentStatusChanges(): void {
], $this->service->findAllRecentStatusChanges(20, 50));
}

public function testFindAllRecentStatusChangesNoEnumeration(): void {
$status1 = $this->createMock(UserStatus::class);
$status2 = $this->createMock(UserStatus::class);

$this->mapper->method('findAllRecent')
->with(20, 50)
->willReturn([$status1, $status2]);

// Have to rebuild $this->service for different config values
$this->config = $this->createMock(IConfig::class);

$this->config->method('getAppValue')
->willReturnMap([
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'],
['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no']
]);

$this->service = new StatusService($this->mapper,
$this->timeFactory,
$this->predefinedStatusService,
$this->emojiService,
$this->userSession,
$this->groupManager,
$this->config);

$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
}

public function testFindByUserId(): void {
$status = $this->createMock(UserStatus::class);
$this->mapper->expects($this->once())
Expand Down
4 changes: 2 additions & 2 deletions lib/public/AppFramework/Http/ContentSecurityPolicy.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
*/
class ContentSecurityPolicy extends EmptyContentSecurityPolicy {
/** @var bool Whether inline JS snippets are allowed */
protected $inlineScriptAllowed = false;
protected $inlineScriptAllowed = true;
/** @var bool Whether eval in JS scripts is allowed */
protected $evalScriptAllowed = false;
protected $evalScriptAllowed = true;
/** @var array Domains from which scripts can get loaded */
protected $allowedScriptDomains = [
'\'self\'',
Expand Down

0 comments on commit f048df1

Please sign in to comment.