From 50a25748c0f0f636d2ffa0abe656f55fc6ed043f Mon Sep 17 00:00:00 2001 From: Jonas Meurer Date: Thu, 8 Jul 2021 18:26:27 +0200 Subject: [PATCH 1/2] 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 --- apps/user_status/lib/AppInfo/Application.php | 12 +++- .../user_status/lib/Service/StatusService.php | 37 ++++++++++-- .../tests/Unit/Service/StatusServiceTest.php | 59 ++++++++++++++++++- 3 files changed, 101 insertions(+), 7 deletions(-) diff --git a/apps/user_status/lib/AppInfo/Application.php b/apps/user_status/lib/AppInfo/Application.php index 2e511500ea647..3a2d742b26e5e 100644 --- a/apps/user_status/lib/AppInfo/Application.php +++ b/apps/user_status/lib/AppInfo/Application.php @@ -36,6 +36,7 @@ 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 @@ public function register(IRegistrationContext $context): void { $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 { diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 1545e3da4f0aa..1c32b0e8b71c5 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -34,6 +34,7 @@ use OCA\UserStatus\Exception\StatusMessageTooLongException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; use OCP\UserStatus\IUserStatus; /** @@ -55,6 +56,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 */ @@ -87,17 +97,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'; } /** @@ -106,6 +121,13 @@ 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 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)); @@ -117,6 +139,13 @@ 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 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)); @@ -224,7 +253,7 @@ public function setPredefinedMessage(string $userId, /** * @param string $userId * @param string|null $statusIcon - * @param string|null $message + * @param string $message * @param int|null $clearAt * @return UserStatus * @throws InvalidClearAtException @@ -332,7 +361,7 @@ public function removeUserStatus(string $userId): bool { * 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(); diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index 77209b70f4826..167646aac8235 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -37,6 +37,7 @@ 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; @@ -54,6 +55,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; @@ -64,10 +68,20 @@ protected function setUp(): void { $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 { @@ -100,6 +114,49 @@ 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]); + + // 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()) From f8719bce2f57d8dd596897fbcb7f3eb8994589f1 Mon Sep 17 00:00:00 2001 From: Jonas Meurer Date: Tue, 19 Oct 2021 10:56:07 +0200 Subject: [PATCH 2/2] Add integration tests for user_status API Signed-off-by: Jonas Meurer --- .../user_status.feature | 20 +++ .../bootstrap/CollaborationContext.php | 164 ++++++++++++++++++ .../features/bootstrap/Provisioning.php | 1 + 3 files changed, 185 insertions(+) create mode 100644 build/integration/collaboration_features/user_status.feature create mode 100644 build/integration/features/bootstrap/CollaborationContext.php diff --git a/build/integration/collaboration_features/user_status.feature b/build/integration/collaboration_features/user_status.feature new file mode 100644 index 0000000000000..759d6e31795fb --- /dev/null +++ b/build/integration/collaboration_features/user_status.feature @@ -0,0 +1,20 @@ +Feature: user_status + Background: + Given using api version "2" + And user "user0" exists + And user "user0" has status "dnd" + + Scenario: listing recent user statuses with default settings + Then user statuses for "admin" list "user0" with status "dnd" + + Scenario: empty recent user statuses with disabled/limited user enumeration + When parameter "shareapi_allow_share_dialog_user_enumeration" of app "core" is set to "no" + Then user statuses for "admin" are empty + When parameter "shareapi_allow_share_dialog_user_enumeration" of app "core" is set to "yes" + When parameter "shareapi_restrict_user_enumeration_to_group" of app "core" is set to "yes" + Then user statuses for "admin" are empty + When parameter "shareapi_restrict_user_enumeration_to_group" of app "core" is set to "no" + When parameter "shareapi_restrict_user_enumeration_to_phone" of app "core" is set to "yes" + Then user statuses for "admin" are empty + When parameter "shareapi_restrict_user_enumeration_to_phone" of app "core" is set to "no" + Then user statuses for "admin" list "user0" with status "dnd" diff --git a/build/integration/features/bootstrap/CollaborationContext.php b/build/integration/features/bootstrap/CollaborationContext.php new file mode 100644 index 0000000000000..4ac3b6e39717e --- /dev/null +++ b/build/integration/features/bootstrap/CollaborationContext.php @@ -0,0 +1,164 @@ + + * + * @author Joas Schilling + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +use Behat\Behat\Context\Context; +use Behat\Gherkin\Node\TableNode; +use GuzzleHttp\Client; +use PHPUnit\Framework\Assert; + +require __DIR__ . '/../../vendor/autoload.php'; + +class CollaborationContext implements Context { + use Provisioning; + use AppConfiguration; + + /** + * @Then /^get autocomplete for "([^"]*)"$/ + * @param TableNode|null $formData + */ + public function getAutocomplete(string $search, TableNode $formData): void { + $query = $search === 'null' ? null : $search; + + $this->sendRequestForJSON('GET', '/core/autocomplete/get?itemType=files&itemId=123&search=' . $query, [ + 'itemType' => 'files', + 'itemId' => '123', + 'search' => $query, + ]); + $this->theHTTPStatusCodeShouldBe(200); + + $data = json_decode($this->response->getBody()->getContents(), true); + $suggestions = $data['ocs']['data']; + + Assert::assertCount(count($formData->getHash()), $suggestions, 'Suggestion count does not match'); + Assert::assertEquals($formData->getHash(), array_map(static function ($suggestion, $expected) { + $data = []; + if (isset($expected['id'])) { + $data['id'] = $suggestion['id']; + } + if (isset($expected['source'])) { + $data['source'] = $suggestion['source']; + } + return $data; + }, $suggestions, $formData->getHash())); + } + + protected function resetAppConfigs(): void { + $this->deleteServerConfig('core', 'shareapi_allow_share_dialog_user_enumeration'); + $this->deleteServerConfig('core', 'shareapi_restrict_user_enumeration_to_group'); + $this->deleteServerConfig('core', 'shareapi_restrict_user_enumeration_to_phone'); + $this->deleteServerConfig('core', 'shareapi_restrict_user_enumeration_full_match'); + $this->deleteServerConfig('core', 'shareapi_only_share_with_group_members'); + } + + /** + * @Given /^user "([^"]*)" has status "([^"]*)"$/ + * @param string $user + * @param string $status + */ + public function assureUserHasStatus($user, $status) { + $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/apps/user_status/api/v1/user_status/status"; + $client = new Client(); + $options = [ + 'headers' => [ + 'OCS-APIREQUEST' => 'true', + ], + ]; + if ($user === 'admin') { + $options['auth'] = $this->adminUser; + } else { + $options['auth'] = [$user, $this->regularUser]; + } + + $options['form_params'] = [ + 'statusType' => $status + ]; + + $this->response = $client->put($fullUrl, $options); + $this->theHTTPStatusCodeShouldBe(200); + + $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/apps/user_status/api/v1/user_status"; + unset($options['form_params']); + $this->response = $client->get($fullUrl, $options); + $this->theHTTPStatusCodeShouldBe(200); + + $returnedStatus = json_decode(json_encode(simplexml_load_string($this->response->getBody()->getContents())->data), true)['status']; + Assert::assertEquals($status, $returnedStatus); + } + + /** + * @param string $user + * @return null|array + */ + public function getStatusList(string $user): ?array { + $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/apps/user_status/api/v1/statuses"; + $client = new Client(); + $options = [ + 'headers' => [ + 'OCS-APIREQUEST' => 'true', + ], + ]; + if ($user === 'admin') { + $options['auth'] = $this->adminUser; + } else { + $options['auth'] = [$user, $this->regularUser]; + } + + $this->response = $client->get($fullUrl, $options); + $this->theHTTPStatusCodeShouldBe(200); + + $contents = $this->response->getBody()->getContents(); + return json_decode(json_encode(simplexml_load_string($contents)->data), true); + } + + /** + * @Given /^user statuses for "([^"]*)" list "([^"]*)" with status "([^"]*)"$/ + * @param string $user + * @param string $statusUser + * @param string $status + */ + public function assertStatusesList(string $user, string $statusUser, string $status): void { + $statusList = $this->getStatusList($user); + Assert::assertArrayHasKey('element', $statusList, 'Returned status list empty or broken'); + if (array_key_exists('userId', $statusList['element'])) { + // If only one user has a status set, the API returns their status directly + Assert::assertArrayHasKey('status', $statusList['element'], 'Returned status list empty or broken'); + $filteredStatusList = [ $statusList['element']['userId'] => $statusList['element']['status'] ]; + } else { + // If more than one user have their status set, the API returns an array of their statuses + $filteredStatusList = array_column($statusList['element'], 'status', 'userId'); + } + Assert::assertArrayHasKey($statusUser, $filteredStatusList, 'User not listed in statuses: ' . $statusUser); + Assert::assertEquals($status, $filteredStatusList[$statusUser]); + } + + /** + * @Given /^user statuses for "([^"]*)" are empty$/ + * @param string $user + */ + public function assertStatusesEmpty(string $user): void { + $statusList = $this->getStatusList($user); + Assert::assertEmpty($statusList); + } +} diff --git a/build/integration/features/bootstrap/Provisioning.php b/build/integration/features/bootstrap/Provisioning.php index 31331092ae72b..c7f9ead411d0d 100644 --- a/build/integration/features/bootstrap/Provisioning.php +++ b/build/integration/features/bootstrap/Provisioning.php @@ -13,6 +13,7 @@ * @author Sergio Bertolín * @author Thomas Müller * @author Vincent Petry + * @author Jonas Meurer * * @license GNU AGPL version 3 or any later version *