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).

Fix this privacy issue by returning an empty list in case
`shareapi_allow_share_dialog_user_enumeration` is unset or
`shareapi_restrict_user_enumeration_to_group` is set.

In the long run, we might want to return users from common groups if
`shareapi_restrict_user_enumeration_to_group` is set. It's complicated
to implement this in a way that scales, though. See the discussion at
https://github.com/nextcloud/server/pull/27879#pullrequestreview-753655308
for details.

Also, don't register the user_status dashboard widget at all if
`shareapi_allow_share_dialog_user_enumeration` is unset or
`shareapi_restrict_user_enumeration_to_group` is set.

Fixes: #27122

Signed-off-by: Jonas Meurer <jonas@freesources.org>
This commit is contained in:
Jonas Meurer 2021-07-08 18:26:27 +02:00
parent 2083e1ede2
commit 3fe267b772
No known key found for this signature in database
GPG key ID: 5262E7FF491049FE
3 changed files with 101 additions and 7 deletions

View file

@ -36,6 +36,7 @@ use OCP\AppFramework\Bootstrap\IBootContext;
use OCP\AppFramework\Bootstrap\IBootstrap;
use OCP\AppFramework\Bootstrap\IRegistrationContext;
use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent;
use OCP\IConfig;
use OCP\User\Events\UserDeletedEvent;
use OCP\User\Events\UserLiveStatusEvent;
use OCP\UserStatus\IManager;
@ -71,8 +72,15 @@ class Application extends App implements IBootstrap {
$context->registerEventListener(UserLiveStatusEvent::class, UserLiveStatusListener::class);
$context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class);
// Register the Dashboard panel
$context->registerDashboardWidget(UserStatusWidget::class);
$config = $this->getContainer()->query(IConfig::class);
$shareeEnumeration = $config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
$shareeEnumerationInGroupOnly = $shareeEnumeration && $config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$shareeEnumerationPhone = $shareeEnumeration && $config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
// Register the Dashboard panel if user enumeration is enabled and not limited
if ($shareeEnumeration && !$shareeEnumerationInGroupOnly && !$shareeEnumerationPhone) {
$context->registerDashboardWidget(UserStatusWidget::class);
}
}
public function boot(IBootContext $context): void {

View file

@ -35,6 +35,7 @@ use OCA\UserStatus\Exception\InvalidStatusTypeException;
use OCA\UserStatus\Exception\StatusMessageTooLongException;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\IConfig;
use OCP\UserStatus\IUserStatus;
/**
@ -56,6 +57,15 @@ class StatusService {
/** @var EmojiService */
private $emojiService;
/** @var bool */
private $shareeEnumeration;
/** @var bool */
private $shareeEnumerationInGroupOnly;
/** @var bool */
private $shareeEnumerationPhone;
/**
* List of priorities ordered by their priority
*/
@ -88,17 +98,22 @@ class StatusService {
*
* @param UserStatusMapper $mapper
* @param ITimeFactory $timeFactory
* @param PredefinedStatusService $defaultStatusService,
* @param PredefinedStatusService $defaultStatusService
* @param EmojiService $emojiService
* @param IConfig $config
*/
public function __construct(UserStatusMapper $mapper,
ITimeFactory $timeFactory,
PredefinedStatusService $defaultStatusService,
EmojiService $emojiService) {
EmojiService $emojiService,
IConfig $config) {
$this->mapper = $mapper;
$this->timeFactory = $timeFactory;
$this->predefinedStatusService = $defaultStatusService;
$this->emojiService = $emojiService;
$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';
$this->shareeEnumerationPhone = $this->shareeEnumeration && $config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
}
/**
@ -107,6 +122,13 @@ class StatusService {
* @return UserStatus[]
*/
public function findAll(?int $limit = null, ?int $offset = null): array {
// Return empty array if user enumeration is disabled or limited to groups
// TODO: find a solution that scales to get only users from common groups if user enumeration is limited to
// groups. See discussion at https://github.com/nextcloud/server/pull/27879#discussion_r729715936
if (!$this->shareeEnumeration || $this->shareeEnumerationInGroupOnly || $this->shareeEnumerationPhone) {
return [];
}
return array_map(function ($status) {
return $this->processStatus($status);
}, $this->mapper->findAll($limit, $offset));
@ -118,6 +140,13 @@ class StatusService {
* @return array
*/
public function findAllRecentStatusChanges(?int $limit = null, ?int $offset = null): array {
// Return empty array if user enumeration is disabled or limited to groups
// TODO: find a solution that scales to get only users from common groups if user enumeration is limited to
// groups. See discussion at https://github.com/nextcloud/server/pull/27879#discussion_r729715936
if (!$this->shareeEnumeration || $this->shareeEnumerationInGroupOnly || $this->shareeEnumerationPhone) {
return [];
}
return array_map(function ($status) {
return $this->processStatus($status);
}, $this->mapper->findAllRecent($limit, $offset));
@ -226,7 +255,7 @@ class StatusService {
/**
* @param string $userId
* @param string|null $statusIcon
* @param string|null $message
* @param string $message
* @param int|null $clearAt
* @return UserStatus
* @throws InvalidClearAtException
@ -334,7 +363,7 @@ class StatusService {
* up to date and provides translated default status if needed
*
* @param UserStatus $status
* @returns UserStatus
* @return UserStatus
*/
private function processStatus(UserStatus $status): UserStatus {
$clearAt = $status->getClearAt();

View file

@ -38,6 +38,7 @@ use OCA\UserStatus\Service\PredefinedStatusService;
use OCA\UserStatus\Service\StatusService;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\IConfig;
use OCP\UserStatus\IUserStatus;
use Test\TestCase;
@ -55,6 +56,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 +69,20 @@ class StatusServiceTest extends TestCase {
$this->timeFactory = $this->createMock(ITimeFactory::class);
$this->predefinedStatusService = $this->createMock(PredefinedStatusService::class);
$this->emojiService = $this->createMock(EmojiService::class);
$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->config);
}
public function testFindAll(): void {
@ -101,6 +115,49 @@ class StatusServiceTest extends TestCase {
], $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]);
// Rebuild $this->service with user enumeration turned off
$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->config);
$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
// Rebuild $this->service with user enumeration limited to common groups
$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', 'yes']
]);
$this->service = new StatusService($this->mapper,
$this->timeFactory,
$this->predefinedStatusService,
$this->emojiService,
$this->config);
$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
}
public function testFindByUserId(): void {
$status = $this->createMock(UserStatus::class);
$this->mapper->expects($this->once())