Skip to content

Commit f5bc265

Browse files
committed
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 #27879 (review) 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 <[email protected]>
1 parent f7a4ff4 commit f5bc265

3 files changed

Lines changed: 102 additions & 7 deletions

File tree

apps/user_status/lib/AppInfo/Application.php

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
use OCP\AppFramework\Bootstrap\IBootstrap;
3737
use OCP\AppFramework\Bootstrap\IRegistrationContext;
3838
use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent;
39+
use OCP\IConfig;
3940
use OCP\User\Events\UserDeletedEvent;
4041
use OCP\User\Events\UserLiveStatusEvent;
4142
use OCP\UserStatus\IManager;
@@ -50,6 +51,12 @@ class Application extends App implements IBootstrap {
5051
/** @var string */
5152
public const APP_ID = 'user_status';
5253

54+
/** @var bool */
55+
private $shareeEnumeration;
56+
57+
/** @var bool */
58+
private $shareeEnumerationInGroupOnly;
59+
5360
/**
5461
* Application constructor.
5562
*
@@ -71,8 +78,14 @@ public function register(IRegistrationContext $context): void {
7178
$context->registerEventListener(UserLiveStatusEvent::class, UserLiveStatusListener::class);
7279
$context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class);
7380

74-
// Register the Dashboard panel
75-
$context->registerDashboardWidget(UserStatusWidget::class);
81+
$config = $this->getContainer()->query(IConfig::class);
82+
$this->shareeEnumeration = $config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
83+
$this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
84+
85+
// Register the Dashboard panel if user enumeration is enabled and not limited
86+
if ($this->shareeEnumeration && !$this->shareeEnumerationInGroupOnly) {
87+
$context->registerDashboardWidget(UserStatusWidget::class);
88+
}
7689
}
7790

7891
public function boot(IBootContext $context): void {

apps/user_status/lib/Service/StatusService.php

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
use OCA\UserStatus\Exception\StatusMessageTooLongException;
3636
use OCP\AppFramework\Db\DoesNotExistException;
3737
use OCP\AppFramework\Utility\ITimeFactory;
38+
use OCP\IConfig;
3839
use OCP\UserStatus\IUserStatus;
3940

4041
/**
@@ -56,6 +57,12 @@ class StatusService {
5657
/** @var EmojiService */
5758
private $emojiService;
5859

60+
/** @var bool */
61+
private $shareeEnumeration;
62+
63+
/** @var bool */
64+
private $shareeEnumerationInGroupOnly;
65+
5966
/**
6067
* List of priorities ordered by their priority
6168
*/
@@ -88,17 +95,21 @@ class StatusService {
8895
*
8996
* @param UserStatusMapper $mapper
9097
* @param ITimeFactory $timeFactory
91-
* @param PredefinedStatusService $defaultStatusService,
98+
* @param PredefinedStatusService $defaultStatusService
9299
* @param EmojiService $emojiService
100+
* @param IConfig $config
93101
*/
94102
public function __construct(UserStatusMapper $mapper,
95103
ITimeFactory $timeFactory,
96104
PredefinedStatusService $defaultStatusService,
97-
EmojiService $emojiService) {
105+
EmojiService $emojiService,
106+
IConfig $config) {
98107
$this->mapper = $mapper;
99108
$this->timeFactory = $timeFactory;
100109
$this->predefinedStatusService = $defaultStatusService;
101110
$this->emojiService = $emojiService;
111+
$this->shareeEnumeration = $config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
112+
$this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
102113
}
103114

104115
/**
@@ -107,6 +118,13 @@ public function __construct(UserStatusMapper $mapper,
107118
* @return UserStatus[]
108119
*/
109120
public function findAll(?int $limit = null, ?int $offset = null): array {
121+
// Return empty array if user enumeration is disabled or limited to groups
122+
// TODO: find a solution that scales to get only users from common groups if user enumeration is limited to
123+
// groups. See discussion at https://github.com/nextcloud/server/pull/27879#discussion_r729715936
124+
if (!$this->shareeEnumeration || $this->shareeEnumerationInGroupOnly) {
125+
return [];
126+
}
127+
110128
return array_map(function ($status) {
111129
return $this->processStatus($status);
112130
}, $this->mapper->findAll($limit, $offset));
@@ -118,6 +136,13 @@ public function findAll(?int $limit = null, ?int $offset = null): array {
118136
* @return array
119137
*/
120138
public function findAllRecentStatusChanges(?int $limit = null, ?int $offset = null): array {
139+
// Return empty array if user enumeration is disabled or limited to groups
140+
// TODO: find a solution that scales to get only users from common groups if user enumeration is limited to
141+
// groups. See discussion at https://github.com/nextcloud/server/pull/27879#discussion_r729715936
142+
if (!$this->shareeEnumeration || $this->shareeEnumerationInGroupOnly) {
143+
return [];
144+
}
145+
121146
return array_map(function ($status) {
122147
return $this->processStatus($status);
123148
}, $this->mapper->findAllRecent($limit, $offset));
@@ -225,7 +250,7 @@ public function setPredefinedMessage(string $userId,
225250
/**
226251
* @param string $userId
227252
* @param string|null $statusIcon
228-
* @param string|null $message
253+
* @param string $message
229254
* @param int|null $clearAt
230255
* @return UserStatus
231256
* @throws InvalidClearAtException
@@ -333,7 +358,7 @@ public function removeUserStatus(string $userId): bool {
333358
* up to date and provides translated default status if needed
334359
*
335360
* @param UserStatus $status
336-
* @returns UserStatus
361+
* @return UserStatus
337362
*/
338363
private function processStatus(UserStatus $status): UserStatus {
339364
$clearAt = $status->getClearAt();

apps/user_status/tests/Unit/Service/StatusServiceTest.php

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
use OCA\UserStatus\Service\StatusService;
3939
use OCP\AppFramework\Db\DoesNotExistException;
4040
use OCP\AppFramework\Utility\ITimeFactory;
41+
use OCP\IConfig;
4142
use OCP\UserStatus\IUserStatus;
4243
use Test\TestCase;
4344

@@ -55,6 +56,9 @@ class StatusServiceTest extends TestCase {
5556
/** @var EmojiService|\PHPUnit\Framework\MockObject\MockObject */
5657
private $emojiService;
5758

59+
/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
60+
private $config;
61+
5862
/** @var StatusService */
5963
private $service;
6064

@@ -65,10 +69,20 @@ protected function setUp(): void {
6569
$this->timeFactory = $this->createMock(ITimeFactory::class);
6670
$this->predefinedStatusService = $this->createMock(PredefinedStatusService::class);
6771
$this->emojiService = $this->createMock(EmojiService::class);
72+
73+
$this->config = $this->createMock(IConfig::class);
74+
75+
$this->config->method('getAppValue')
76+
->willReturnMap([
77+
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'],
78+
['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no']
79+
]);
80+
6881
$this->service = new StatusService($this->mapper,
6982
$this->timeFactory,
7083
$this->predefinedStatusService,
71-
$this->emojiService);
84+
$this->emojiService,
85+
$this->config);
7286
}
7387

7488
public function testFindAll(): void {
@@ -101,6 +115,49 @@ public function testFindAllRecentStatusChanges(): void {
101115
], $this->service->findAllRecentStatusChanges(20, 50));
102116
}
103117

118+
public function testFindAllRecentStatusChangesNoEnumeration(): void {
119+
$status1 = $this->createMock(UserStatus::class);
120+
$status2 = $this->createMock(UserStatus::class);
121+
122+
$this->mapper->method('findAllRecent')
123+
->with(20, 50)
124+
->willReturn([$status1, $status2]);
125+
126+
// Rebuild $this->service with user enumeration turned off
127+
$this->config = $this->createMock(IConfig::class);
128+
129+
$this->config->method('getAppValue')
130+
->willReturnMap([
131+
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'],
132+
['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no']
133+
]);
134+
135+
$this->service = new StatusService($this->mapper,
136+
$this->timeFactory,
137+
$this->predefinedStatusService,
138+
$this->emojiService,
139+
$this->config);
140+
141+
$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
142+
143+
// Rebuild $this->service with user enumeration limited to common groups
144+
$this->config = $this->createMock(IConfig::class);
145+
146+
$this->config->method('getAppValue')
147+
->willReturnMap([
148+
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'],
149+
['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'yes']
150+
]);
151+
152+
$this->service = new StatusService($this->mapper,
153+
$this->timeFactory,
154+
$this->predefinedStatusService,
155+
$this->emojiService,
156+
$this->config);
157+
158+
$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
159+
}
160+
104161
public function testFindByUserId(): void {
105162
$status = $this->createMock(UserStatus::class);
106163
$this->mapper->expects($this->once())

0 commit comments

Comments
 (0)