Skip to content

Respect user enumeration settings in user status lists #27879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mejo- wants to merge 1 commit into from
Closed

Respect user enumeration settings in user status lists #27879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions apps/user_status/lib/Db/UserStatusMapper.php
View file
Open in desktop
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 @@ -114,15 +114,56 @@ public function findByUserId(string $userId):UserStatus {

/**
* @param array $userIds
* @param int|null $limit
* @param int|null $offset
* @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)
->orderBy('status_timestamp', 'DESC')
->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
Open in desktop
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;
Copy link
Member

@nickvergessen nickvergessen Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this does not scale. We need to do it in another way. In worst case someone is in hundreds of (automated) groups, you run hundreds of queries to generate the user list to then later know that they don't even have a status set or something. Can't we do it similar to lib/private/Collaboration/Collaborators/UserPlugin.php and first get the recent user statuses and then filter out the results?
Basically get the status of 100 people, then filter out, if less than the requested users remain retry

Copy link
Member Author

@mejo- mejo- Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhhh, you certainly have an argument here 😬

But I'm unsure whether the other way around scales better: It's rather unlikely that the desired 8 users from shared groups with recent status changes are amongh the first 100. So in many cases we'll end up iterating over all users with recent status changes. And for each user we have to check whether they're in a shared group.

So I see two approaches and I'm not sure which one scales better:

  • Iterate over all usergroups and their members, return a list of users. Filter the search for user statuses by this list of users.
  • Iterate over most users with recent status changes. For some/many of them, check whether we're in a shared group.

@nickvergessen you're way more experienced, so I'd trust you on what scales better on common setups/instances.

Besides: I'm not sure what you mean with your reference to lib/private/Collaboration/Collaborators/UserPlugin.php. I mostly copied its implementation to search for users from shared groups. Looking at lib/private/Collaboration/Collaborators/UserPlugin.php#L98-L115, it iterates over all members of all groups that the user is member of just like the implementation in this PR, no? Maybe I miss something :grimaced:

}

/**
* 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
Open in desktop
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