fix: include invisible tags for admins

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
nextcloud · Jun 21, 2023 · 9c1daec · 9c1daec
1 parent c400762
commit 9c1daec
Show file tree

Hide file tree

Showing 6 changed files with 114 additions and 97 deletions.
diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php
@@ -106,6 +106,7 @@ class FilesReportPlugin extends ServerPlugin {
 	 * @var IAppManager
 	 */
 	private $appManager;
+	protected ?bool $isSessionUserAdmin = null;
 
 	/**
 	 * @param Tree $tree
@@ -284,8 +285,6 @@ private function getFilesBaseUri(string $uri, string $subPath): string {
 	 *
 	 * @param array $filterRules
 	 * @return array array of unique file id results
-	 *
-	 * @throws TagNotFoundException whenever a tag was not found
 	 */
 	protected function processFilterRulesForFileIDs($filterRules) {
 		$ns = '{' . $this::NS_OWNCLOUD . '}';
@@ -320,6 +319,13 @@ protected function processFilterRulesForFileIDs($filterRules) {
 		return $resultFileIds;
 	}
 
+	protected function isSessionUserAdmin(): bool {
+		if ($this->isSessionUserAdmin === null) {
+			$this->isSessionUserAdmin = $this->groupManager->isAdmin($this->userSession->getUser()->getUID());
+		}
+		return $this->isSessionUserAdmin;
+	}
+
 	protected function processFilterRulesForFileNodes(array $filterRules, ?int $limit, ?int $offset): array {
 		$systemTagIds = [];
 		foreach ($filterRules as $filterRule) {
@@ -336,12 +342,8 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi
 			!empty($systemTagIds)
 			&& (method_exists($this->userFolder, 'searchBySystemTag'))
 		) {
-			$tags = $this->tagManager->getTagsByIds($systemTagIds);
+			$tags = $this->tagManager->getTagsByIds($systemTagIds, $this->userSession->getUser());
 			foreach ($tags as $tag) {
-				if (!$tag->isUserVisible()) {
-					// searchBySystemTag() also has the criteria to include only user visible tags. They can be skipped early nevertheless.
-					continue;
-				}
 				$tagName = $tag->getName();
 				$tmpNodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0);
 				if (count($nodes) === 0) {

diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php
@@ -44,6 +44,7 @@
 use OCP\SystemTag\ISystemTag;
 use OCP\SystemTag\ISystemTagManager;
 use OCP\SystemTag\ISystemTagObjectMapper;
+use OCP\SystemTag\TagNotFoundException;
 use PHPUnit\Framework\MockObject\MockObject;
 use Sabre\DAV\INode;
 use Sabre\DAV\Tree;
@@ -734,7 +735,7 @@ public function testProcessFilterRulesAndConditionWithEmptyMidResult(): void {
 			->method('getTagsByIds')
 			->with(['123', '456', '789'])
 			->willReturn([$tag123, $tag456, $tag789]);
-		
+
 		$this->userFolder->expects($this->exactly(2))
 			->method('searchBySystemTag')
 			->withConsecutive(['OneTwoThree'], ['FourFiveSix'], ['SevenEightNein'])
@@ -758,81 +759,94 @@ public function testProcessFilterRulesInvisibleTagAsAdmin(): void {
 			->method('isAdmin')
 			->willReturn(true);
 
-		$tag1 = $this->getMockBuilder(ISystemTag::class)
-			->disableOriginalConstructor()
-			->getMock();
-		$tag1->expects($this->any())
+		$filesNode1 = $this->createMock(File::class);
+		$filesNode1->expects($this->any())
+			->method('getSize')
+			->willReturn(12);
+		$filesNode1->expects($this->any())
 			->method('getId')
-			->willReturn('123');
-		$tag1->expects($this->any())
+			->willReturn(111);
+		$filesNode2 = $this->createMock(Folder::class);
+		$filesNode2->expects($this->any())
+			->method('getSize')
+			->willReturn(10);
+		$filesNode2->expects($this->any())
+			->method('getId')
+			->willReturn(222);
+		$filesNode3 = $this->createMock(Folder::class);
+		$filesNode3->expects($this->any())
+			->method('getSize')
+			->willReturn(13);
+		$filesNode3->expects($this->any())
+			->method('getId')
+			->willReturn(333);
+
+		$tag123 = $this->createMock(ISystemTag::class);
+		$tag123->expects($this->any())
+			->method('getName')
+			->willReturn('OneTwoThree');
+		$tag123->expects($this->any())
 			->method('isUserVisible')
 			->willReturn(true);
-
-		$tag2 = $this->getMockBuilder(ISystemTag::class)
-			->disableOriginalConstructor()
-			->getMock();
-		$tag2->expects($this->any())
-			->method('getId')
-			->willReturn('123');
-		$tag2->expects($this->any())
+		$tag456 = $this->createMock(ISystemTag::class);
+		$tag456->expects($this->any())
+			->method('getName')
+			->willReturn('FourFiveSix');
+		$tag456->expects($this->any())
 			->method('isUserVisible')
 			->willReturn(false);
 
-		// no need to fetch tags to check permissions
-		$this->tagManager->expects($this->never())
-			->method('getTagsByIds');
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with(['123', '456'])
+			->willReturn([$tag123, $tag456]);
 
-		$this->tagMapper->expects($this->exactly(2))
-			->method('getObjectIdsForTags')
-			->withConsecutive(
-				['123'],
-				['456'],
-			)
+		$this->userFolder->expects($this->exactly(2))
+			->method('searchBySystemTag')
+			->withConsecutive(['OneTwoThree'], ['FourFiveSix'])
 			->willReturnOnConsecutiveCalls(
-				['111', '222'],
-				['222', '333'],
+				[$filesNode1, $filesNode2],
+				[$filesNode2, $filesNode3],
 			);
 
 		$rules = [
 			['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'],
 			['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'],
 		];
 
-		$this->assertEquals(['222'], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null])));
+		$this->assertEquals([$filesNode2], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null])));
 	}
 
 
 	public function testProcessFilterRulesInvisibleTagAsUser(): void {
-		$this->expectException(\OCP\SystemTag\TagNotFoundException::class);
+		$this->expectException(TagNotFoundException::class);
 
 		$this->groupManager->expects($this->any())
 			->method('isAdmin')
 			->willReturn(false);
 
-		$tag1 = $this->getMockBuilder(ISystemTag::class)
-			->disableOriginalConstructor()
-			->getMock();
-		$tag1->expects($this->any())
-			->method('getId')
-			->willReturn('123');
-		$tag1->expects($this->any())
+		$tag123 = $this->createMock(ISystemTag::class);
+		$tag123->expects($this->any())
+			->method('getName')
+			->willReturn('OneTwoThree');
+		$tag123->expects($this->any())
 			->method('isUserVisible')
 			->willReturn(true);
-
-		$tag2 = $this->getMockBuilder(ISystemTag::class)
-			->disableOriginalConstructor()
-			->getMock();
-		$tag2->expects($this->any())
-			->method('getId')
-			->willReturn('123');
-		$tag2->expects($this->any())
+		$tag456 = $this->createMock(ISystemTag::class);
+		$tag456->expects($this->any())
+			->method('getName')
+			->willReturn('FourFiveSix');
+		$tag456->expects($this->any())
 			->method('isUserVisible')
-			->willReturn(false); // invisible
+			->willReturn(false);
 
 		$this->tagManager->expects($this->once())
 			->method('getTagsByIds')
 			->with(['123', '456'])
-			->willReturn([$tag1, $tag2]);
+			->willThrowException(new TagNotFoundException());
+
+		$this->userFolder->expects($this->never())
+			->method('searchBySystemTag');
 
 		$rules = [
 			['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'],

diff --git a/lib/private/Files/Cache/QuerySearchHelper.php b/lib/private/Files/Cache/QuerySearchHelper.php
@@ -37,9 +37,9 @@
 use OCP\Files\IMimeTypeLoader;
 use OCP\Files\Mount\IMountPoint;
 use OCP\Files\Search\ISearchBinaryOperator;
-use OCP\Files\Search\ISearchComparison;
 use OCP\Files\Search\ISearchQuery;
 use OCP\IDBConnection;
+use OCP\IGroupManager;
 use OCP\IUser;
 use Psr\Log\LoggerInterface;
 
@@ -55,21 +55,24 @@ class QuerySearchHelper {
 	private $searchBuilder;
 	/** @var QueryOptimizer */
 	private $queryOptimizer;
+	private IGroupManager $groupManager;
 
 	public function __construct(
 		IMimeTypeLoader $mimetypeLoader,
 		IDBConnection $connection,
 		SystemConfig $systemConfig,
 		LoggerInterface $logger,
 		SearchBuilder $searchBuilder,
-		QueryOptimizer $queryOptimizer
+		QueryOptimizer $queryOptimizer,
+		IGroupManager $groupManager,
 	) {
 		$this->mimetypeLoader = $mimetypeLoader;
 		$this->connection = $connection;
 		$this->systemConfig = $systemConfig;
 		$this->logger = $logger;
 		$this->searchBuilder = $searchBuilder;
 		$this->queryOptimizer = $queryOptimizer;
+		$this->groupManager = $groupManager;
 	}
 
 	protected function getQueryBuilder() {
@@ -119,16 +122,16 @@ public function findUsedTagsInCaches(ISearchQuery $searchQuery, array $caches):
 		return $tags;
 	}
 
-	protected function equipQueryForSystemTags(CacheQueryBuilder $query): void {
-		$query
-			->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $query->expr()->andX(
-				$query->expr()->eq('file.fileid', $query->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)),
-				$query->expr()->eq('systemtagmap.objecttype', $query->createNamedParameter('files'))
-			))
-			->leftJoin('systemtagmap', 'systemtag', 'systemtag', $query->expr()->andX(
-				$query->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'),
-				$query->expr()->eq('systemtag.visibility', $query->createNamedParameter(true))
-			));
+	protected function equipQueryForSystemTags(CacheQueryBuilder $query, IUser $user): void {
+		$query->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $query->expr()->andX(
+			$query->expr()->eq('file.fileid', $query->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)),
+			$query->expr()->eq('systemtagmap.objecttype', $query->createNamedParameter('files'))
+		));
+		$on = $query->expr()->andX($query->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'));
+		if (!$this->groupManager->isAdmin($user->getUID())) {
+			$on->add($query->expr()->eq('systemtag.visibility', $query->createNamedParameter(true)));
+		}
+		$query->leftJoin('systemtagmap', 'systemtag', 'systemtag', $on);
 	}
 
 	protected function equipQueryForDavTags(CacheQueryBuilder $query, IUser $user): void {
@@ -172,25 +175,12 @@ public function searchInCaches(ISearchQuery $searchQuery, array $caches): array
 
 		$query = $builder->selectFileCache('file', false);
 
-		if ($this->searchBuilder->shouldJoinTags($searchQuery->getSearchOperation())) {
-			$user = $searchQuery->getUser();
-			if ($user === null) {
-				throw new \InvalidArgumentException("Searching by tag requires the user to be set in the query");
-			}
-			if ($searchQuery->getSearchOperation() instanceof ISearchComparison) {
-				switch ($searchQuery->getSearchOperation()->getField()) {
-					case 'systemtag':
-						$this->equipQueryForSystemTags($query);
-						break;
-					case 'tagname':
-					case 'favorite':
-						$this->equipQueryForDavTags($query, $user);
-						break;
-				}
-			} elseif ($searchQuery->getSearchOperation() instanceof SearchBinaryOperator) {
-				$this->equipQueryForSystemTags($query);
-				$this->equipQueryForDavTags($query, $user);
-			}
+		$requestedFields = $this->searchBuilder->extractRequestedFields($searchQuery->getSearchOperation());
+		if (in_array('systemtag', $requestedFields)) {
+			$this->equipQueryForSystemTags($query, $this->requireUser($searchQuery));
+		}
+		if (in_array('tagname', $requestedFields) || in_array('favorite', $requestedFields)) {
+			$this->equipQueryForDavTags($query, $this->requireUser($searchQuery));
 		}
 
 		$this->applySearchConstraints($query, $searchQuery, $caches);
@@ -218,6 +208,14 @@ public function searchInCaches(ISearchQuery $searchQuery, array $caches): array
 		return $results;
 	}
 
+	protected function requireUser(ISearchQuery $searchQuery): IUser {
+		$user = $searchQuery->getUser();
+		if ($user === null) {
+			throw new \InvalidArgumentException("This search operation requires the user to be set in the query");
+		}
+		return $user;
+	}
+
 	/**
 	 * @return array{array<string, ICache>, array<string, IMountPoint>}
 	 */

diff --git a/lib/private/Files/Cache/SearchBuilder.php b/lib/private/Files/Cache/SearchBuilder.php
@@ -69,20 +69,17 @@ public function __construct(
 	}
 
 	/**
-	 * Whether or not the tag tables should be joined to complete the search
-	 *
-	 * @param ISearchOperator $operator
-	 * @return boolean
+	 * @return string[]
 	 */
-	public function shouldJoinTags(ISearchOperator $operator) {
+	public function extractRequestedFields(ISearchOperator $operator): array {
 		if ($operator instanceof ISearchBinaryOperator) {
-			return array_reduce($operator->getArguments(), function ($shouldJoin, ISearchOperator $operator) {
-				return $shouldJoin || $this->shouldJoinTags($operator);
-			}, false);
+			return array_reduce($operator->getArguments(), function (array $fields, ISearchOperator $operator) {
+				return array_unique(array_merge($fields, $this->extractRequestedFields($operator)));
+			}, []);
 		} elseif ($operator instanceof ISearchComparison) {
-			return $operator->getField() === 'tagname' || $operator->getField() === 'favorite' || $operator->getField() === 'systemtag';
+			return [$operator->getField()];
 		}
-		return false;
+		return [];
 	}
 
 	/**

diff --git a/lib/private/SystemTag/SystemTagManager.php b/lib/private/SystemTag/SystemTagManager.php
@@ -91,7 +91,7 @@ public function __construct(
 	/**
 	 * {@inheritdoc}
 	 */
-	public function getTagsByIds($tagIds): array {
+	public function getTagsByIds($tagIds, ?IUser $user = null): array {
 		if (!\is_array($tagIds)) {
 			$tagIds = [$tagIds];
 		}
@@ -116,7 +116,12 @@ public function getTagsByIds($tagIds): array {
 
 		$result = $query->execute();
 		while ($row = $result->fetch()) {
-			$tags[$row['id']] = $this->createSystemTagFromRow($row);
+			$tag = $this->createSystemTagFromRow($row);
+			if ($user && !$this->canUserSeeTag($tag, $user)) {
+				// if a user is given, hide invisible tags
+				continue;
+			}
+			$tags[$row['id']] = $tag;
 		}
 
 		$result->closeCursor();

diff --git a/lib/public/SystemTag/ISystemTagManager.php b/lib/public/SystemTag/ISystemTagManager.php
@@ -38,16 +38,17 @@ interface ISystemTagManager {
 	 * Returns the tag objects matching the given tag ids.
 	 *
 	 * @param array|string $tagIds id or array of unique ids of the tag to retrieve
+	 * @param ?IUser $user optional user to run a visibility check against for each tag
 	 *
 	 * @return ISystemTag[] array of system tags with tag id as key
 	 *
 	 * @throws \InvalidArgumentException if at least one given tag ids is invalid (string instead of integer, etc.)
 	 * @throws TagNotFoundException if at least one given tag ids did no exist
 	 * 			The message contains a json_encoded array of the ids that could not be found
 	 *
-	 * @since 9.0.0
+	 * @since 9.0.0, optional parameter $user added in 28.0.0
 	 */
-	public function getTagsByIds($tagIds): array;
+	public function getTagsByIds($tagIds, ?IUser $user = null): array;
 
 	/**
 	 * Returns the tag object matching the given attributes.