Merge pull request #63896 from nextcloud/backport/63883/stable35

[stable35] Improve user enumeration

Author: AndyScherzinger

apps/files_sharing/lib/Controller/ShareesAPIController.php

@@ -74,6 +74,7 @@ public function __construct(
 		protected IManager $shareManager,
 		protected ISearch $collaboratorSearch,
 		protected FederatedShareProvider $federatedShareProvider,
+		protected IAppManager $appManager,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -161,7 +162,7 @@ public function search(string $search = '', ?string $itemType = null, int $page
 		}
 
 		// FIXME: DI
-		if (Server::get(IAppManager::class)->isEnabledForUser('circles') && class_exists('\OCA\Circles\ShareByCircleProvider')) {
+		if ($this->appManager->isEnabledForUser('circles') && class_exists('\OCA\Circles\ShareByCircleProvider')) {
 			$shareTypes[] = IShare::TYPE_CIRCLE;
 		}
 
@@ -183,25 +184,26 @@ public function search(string $search = '', ?string $itemType = null, int $page
 		$this->result['lookupEnabled'] = Server::get(GlobalScaleIConfig::class)->isGlobalScaleEnabled();
 		// TODO: Reconsider using lookup server for non-global-scale federation
 
-		[$result, $hasMoreResults] = $this->collaboratorSearch->search($search, $shareTypes, $this->result['lookupEnabled'], $this->limit, $this->offset);
+		[$result, $hasMoreResults] = $this->collaboratorSearch->filteredSearch($search, $shareTypes, $this->result['lookupEnabled'], $itemType, null, $this->limit, $this->offset);
 
 		// extra treatment for 'exact' subarray, with a single merge expected keys might be lost
 		if (isset($result['exact'])) {
 			$result['exact'] = array_merge($this->result['exact'], $result['exact']);
 		}
-		$this->result = array_merge($this->result, $result);
-		$response = new DataResponse($this->result);
+		/** @var Files_SharingShareesSearchResult $result */
+		$result = array_merge($this->result, $result);
 
+		$headers = [];
 		if ($hasMoreResults) {
-			$response->setHeaders(['Link' => $this->getPaginationLink($page, [
+			$headers['Link'] = $this->getPaginationLink($page, [
 				'search' => $search,
 				'itemType' => $itemType,
 				'shareType' => $shareTypes,
 				'perPage' => $perPage,
-			])]);
+			]);
 		}
 
-		return $response;
+		return new DataResponse($result, Http::STATUS_OK, $headers);
 	}
 
 	/**

apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php

@@ -11,6 +11,7 @@
 use OCA\FederatedFileSharing\FederatedShareProvider;
 use OCA\Files_Sharing\Controller\ShareesAPIController;
 use OCA\Files_Sharing\Tests\TestCase;
+use OCP\App\IAppManager;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\OCS\OCSBadRequestException;
 use OCP\Collaboration\Collaborators\ISearch;
@@ -30,26 +31,14 @@
  */
 #[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
 class ShareesAPIControllerTest extends TestCase {
-	/** @var ShareesAPIController */
-	protected $sharees;
-
-	/** @var string */
-	protected $uid;
-
-	/** @var IRequest|MockObject */
-	protected $request;
-
-	/** @var IManager|MockObject */
+	protected ShareesAPIController $sharees;
+	protected string $uid;
+	protected IRequest&MockObject $request;
 	protected $shareManager;
-
-	/** @var ISearch|MockObject */
-	protected $collaboratorSearch;
-
-	/** @var IConfig|MockObject */
-	protected $config;
-
-	/** @var FederatedShareProvider|MockObject */
-	protected $federatedShareProvider;
+	protected ISearch&MockObject $collaboratorSearch;
+	protected IConfig&MockObject $config;
+	protected FederatedShareProvider&MockObject $federatedShareProvider;
+	protected IAppManager&MockObject $appManager;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -59,11 +48,11 @@ protected function setUp(): void {
 		$this->shareManager = $this->createMock(IManager::class);
 		$this->config = $this->createMock(IConfig::class);
 
-		/** @var IURLGenerator|MockObject $urlGeneratorMock */
 		$urlGeneratorMock = $this->createMock(IURLGenerator::class);
 
 		$this->collaboratorSearch = $this->createMock(ISearch::class);
 		$this->federatedShareProvider = $this->createMock(FederatedShareProvider::class);
+		$this->appManager = $this->createMock(IAppManager::class);
 
 		$this->sharees = new ShareesAPIController(
 			'files_sharing',
@@ -73,7 +62,8 @@ protected function setUp(): void {
 			$urlGeneratorMock,
 			$this->shareManager,
 			$this->collaboratorSearch,
-			$this->federatedShareProvider
+			$this->federatedShareProvider,
+			$this->appManager,
 		);
 	}
 
@@ -267,7 +257,8 @@ public function testSearch(
 				$urlGenerator,
 				$this->shareManager,
 				$this->collaboratorSearch,
-				$this->federatedShareProvider
+				$this->federatedShareProvider,
+				$this->appManager,
 			])
 			->onlyMethods(['isRemoteSharingAllowed', 'isRemoteGroupSharingAllowed'])
 			->getMock();
@@ -276,8 +267,8 @@ public function testSearch(
 		sort($expectedShareTypes);
 
 		$this->collaboratorSearch->expects($this->once())
-			->method('search')
-			->with($search, $expectedShareTypes, $this->anything(), $perPage, $perPage * ($page - 1))
+			->method('filteredSearch')
+			->with($search, $expectedShareTypes, $this->anything(), $itemType, null, $perPage, $perPage * ($page - 1))
 			->willReturn([[], false]);
 
 		$sharees->expects($this->any())
@@ -364,15 +355,16 @@ public function testSearchInvalid($getData, $message): void {
 				$urlGenerator,
 				$this->shareManager,
 				$this->collaboratorSearch,
-				$this->federatedShareProvider
+				$this->federatedShareProvider,
+				$this->appManager,
 			])
 			->onlyMethods(['isRemoteSharingAllowed'])
 			->getMock();
 		$sharees->expects($this->never())
 			->method('isRemoteSharingAllowed');
 
 		$this->collaboratorSearch->expects($this->never())
-			->method('search');
+			->method('filteredSearch');
 
 		try {
 			$sharees->search('', null, $page, $perPage, null);

apps/user_status/lib/Controller/StatusesController.php

@@ -19,7 +19,9 @@
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
+use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IRequest;
+use OCP\User\Events\UserEnumerationFilterEvent;
 use OCP\UserStatus\IUserStatus;
 
 /**
@@ -38,7 +40,8 @@ class StatusesController extends OCSController {
 	public function __construct(
 		string $appName,
 		IRequest $request,
-		private StatusService $service,
+		private readonly StatusService $service,
+		private readonly IEventDispatcher $eventDispatcher,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -57,6 +60,15 @@ public function __construct(
 	public function findAll(?int $limit = null, ?int $offset = null): DataResponse {
 		$allStatuses = $this->service->findAll($limit, $offset);
 
+		$users = array_map(fn (UserStatus $userStatus): string => $userStatus->getUserId(), $allStatuses);
+		$event = new UserEnumerationFilterEvent($users);
+		$this->eventDispatcher->dispatchTyped($event);
+
+		if ($users !== $event->getUsers()) {
+			$removedUsers = $event->getFilteredOutUsers();
+			$allStatuses = array_filter($allStatuses, fn (UserStatus $userStatus): bool => !in_array($userStatus->getUserId(), $removedUsers, true));
+		}
+
 		return new DataResponse(array_values(array_map(function ($userStatus) {
 			return $this->formatStatus($userStatus);
 		}, $allStatuses)));

apps/user_status/lib/Service/StatusService.php

@@ -82,7 +82,7 @@ public function __construct(
 	/**
 	 * @param int|null $limit
 	 * @param int|null $offset
-	 * @return UserStatus[]
+	 * @return list<UserStatus>
 	 */
 	public function findAll(?int $limit = null, ?int $offset = null): array {
 		// Return empty array if user enumeration is disabled or limited to groups
@@ -92,9 +92,9 @@ public function findAll(?int $limit = null, ?int $offset = null): array {
 			return [];
 		}
 
-		return array_map(function ($status) {
+		return array_values(array_map(function ($status) {
 			return $this->processStatus($status);
-		}, $this->mapper->findAll($limit, $offset));
+		}, $this->mapper->findAll($limit, $offset)));
 	}
 
 	/**

apps/user_status/tests/Unit/Controller/StatusesControllerTest.php

@@ -14,21 +14,24 @@
 use OCA\UserStatus\Service\StatusService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
+use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IRequest;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 class StatusesControllerTest extends TestCase {
 	private StatusService&MockObject $service;
+	private IEventDispatcher&MockObject $eventDispatcher;
 	private StatusesController $controller;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$request = $this->createMock(IRequest::class);
 		$this->service = $this->createMock(StatusService::class);
+		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 
-		$this->controller = new StatusesController('user_status', $request, $this->service);
+		$this->controller = new StatusesController('user_status', $request, $this->service, $this->eventDispatcher);
 	}
 
 	public function testFindAll(): void {

core/Controller/AutoCompleteController.php

@@ -15,10 +15,8 @@
 use OCP\AppFramework\Http\Attribute\NoAdminRequired;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\OCSController;
-use OCP\Collaboration\AutoComplete\AutoCompleteFilterEvent;
 use OCP\Collaboration\AutoComplete\IManager;
 use OCP\Collaboration\Collaborators\ISearch;
-use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IRequest;
 use OCP\Share\IShare;
 
@@ -31,7 +29,6 @@ public function __construct(
 		IRequest $request,
 		private ISearch $collaboratorSearch,
 		private IManager $autoCompleteManager,
-		private IEventDispatcher $dispatcher,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -55,19 +52,7 @@ public function __construct(
 	public function get(string $search, ?string $itemType, ?string $itemId, ?string $sorter = null, array $shareTypes = [IShare::TYPE_USER], int $limit = 10): DataResponse {
 		// if enumeration/user listings are disabled, we'll receive an empty
 		// result from search() – thus nothing else to do here.
-		[$results,] = $this->collaboratorSearch->search($search, $shareTypes, false, $limit, 0);
-
-		$event = new AutoCompleteFilterEvent(
-			$results,
-			$search,
-			$itemType,
-			$itemId,
-			$sorter,
-			$shareTypes,
-			$limit,
-		);
-		$this->dispatcher->dispatchTyped($event);
-		$results = $event->getResults();
+		[$results,] = $this->collaboratorSearch->filteredSearch($search, $shareTypes, false, $itemType, $itemId, $limit, 0);
 
 		$exactMatches = $results['exact'];
 		unset($results['exact']);

lib/composer/composer/autoload_classmap.php

@@ -1145,6 +1145,7 @@
     'OCP\\User\\Events\\UserConfigChangedEvent' => $baseDir . '/lib/public/User/Events/UserConfigChangedEvent.php',
     'OCP\\User\\Events\\UserCreatedEvent' => $baseDir . '/lib/public/User/Events/UserCreatedEvent.php',
     'OCP\\User\\Events\\UserDeletedEvent' => $baseDir . '/lib/public/User/Events/UserDeletedEvent.php',
+    'OCP\\User\\Events\\UserEnumerationFilterEvent' => $baseDir . '/lib/public/User/Events/UserEnumerationFilterEvent.php',
     'OCP\\User\\Events\\UserFirstTimeLoggedInEvent' => $baseDir . '/lib/public/User/Events/UserFirstTimeLoggedInEvent.php',
     'OCP\\User\\Events\\UserIdAssignedEvent' => $baseDir . '/lib/public/User/Events/UserIdAssignedEvent.php',
     'OCP\\User\\Events\\UserIdUnassignedEvent' => $baseDir . '/lib/public/User/Events/UserIdUnassignedEvent.php',

lib/composer/composer/autoload_static.php

@@ -1186,6 +1186,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
         'OCP\\User\\Events\\UserConfigChangedEvent' => __DIR__ . '/../../..' . '/lib/public/User/Events/UserConfigChangedEvent.php',
         'OCP\\User\\Events\\UserCreatedEvent' => __DIR__ . '/../../..' . '/lib/public/User/Events/UserCreatedEvent.php',
         'OCP\\User\\Events\\UserDeletedEvent' => __DIR__ . '/../../..' . '/lib/public/User/Events/UserDeletedEvent.php',
+        'OCP\\User\\Events\\UserEnumerationFilterEvent' => __DIR__ . '/../../..' . '/lib/public/User/Events/UserEnumerationFilterEvent.php',
         'OCP\\User\\Events\\UserFirstTimeLoggedInEvent' => __DIR__ . '/../../..' . '/lib/public/User/Events/UserFirstTimeLoggedInEvent.php',
         'OCP\\User\\Events\\UserIdAssignedEvent' => __DIR__ . '/../../..' . '/lib/public/User/Events/UserIdAssignedEvent.php',
         'OCP\\User\\Events\\UserIdUnassignedEvent' => __DIR__ . '/../../..' . '/lib/public/User/Events/UserIdUnassignedEvent.php',

lib/private/Collaboration/Collaborators/GroupPlugin.php

@@ -42,7 +42,7 @@ public function __construct(
 	}
 
 	#[\Override]
-	public function search($search, $limit, $offset, ISearchResult $searchResult): bool {
+	public function search(string $search, int $limit, int $offset, ISearchResult $searchResult): bool {
 		if ($this->groupSharingDisabled) {
 			return false;
 		}

lib/private/Collaboration/Collaborators/LookupPlugin.php

@@ -34,7 +34,7 @@ public function __construct(
 	}
 
 	#[\Override]
-	public function search($search, $limit, $offset, ISearchResult $searchResult): bool {
+	public function search(string $search, int $limit, int $offset, ISearchResult $searchResult): bool {
 		$isGlobalScaleEnabled = $this->globalScaleConfig->isGlobalScaleEnabled();
 		$isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
 		$hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true);