From 49db546f784e5e1d715a29d737508604b19f69eb Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 15 Jun 2023 22:46:04 +0200 Subject: [PATCH] fix: include invisible tags for admins Signed-off-by: Arthur Schiwon --- .../lib/Connector/Sabre/FilesReportPlugin.php | 8 +- .../Connector/Sabre/FilesReportPluginTest.php | 104 ++++++++++-------- lib/private/Files/Cache/QuerySearchHelper.php | 60 +++++----- lib/private/Files/Cache/SearchBuilder.php | 17 ++- lib/private/SystemTag/SystemTagManager.php | 9 +- lib/public/SystemTag/ISystemTagManager.php | 5 +- 6 files changed, 106 insertions(+), 97 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index c0cd382d994..4fabe9fdd1c 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -283,8 +283,6 @@ class FilesReportPlugin extends ServerPlugin { * * @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 . '}'; @@ -335,12 +333,8 @@ class FilesReportPlugin extends ServerPlugin { !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 index 5c81757b11a..7cb2d85fe1f 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -44,6 +44,7 @@ use OCP\IUserSession; 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 @@ class FilesReportPluginTest extends \Test\TestCase { ->method('getTagsByIds') ->with(['123', '456', '789']) ->willReturn([$tag123, $tag456, $tag789]); - + $this->userFolder->expects($this->exactly(2)) ->method('searchBySystemTag') ->withConsecutive(['OneTwoThree'], ['FourFiveSix'], ['SevenEightNein']) @@ -758,39 +759,54 @@ class FilesReportPluginTest extends \Test\TestCase { ->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 = [ @@ -798,41 +814,39 @@ class FilesReportPluginTest extends \Test\TestCase { ['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 index 0f9c6eeaa88..964526203b6 100644 --- a/lib/private/Files/Cache/QuerySearchHelper.php +++ b/lib/private/Files/Cache/QuerySearchHelper.php @@ -37,9 +37,9 @@ use OCP\Files\Folder; 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,6 +55,7 @@ class QuerySearchHelper { private $searchBuilder; /** @var QueryOptimizer */ private $queryOptimizer; + private IGroupManager $groupManager; public function __construct( IMimeTypeLoader $mimetypeLoader, @@ -62,7 +63,8 @@ class QuerySearchHelper { SystemConfig $systemConfig, LoggerInterface $logger, SearchBuilder $searchBuilder, - QueryOptimizer $queryOptimizer + QueryOptimizer $queryOptimizer, + IGroupManager $groupManager, ) { $this->mimetypeLoader = $mimetypeLoader; $this->connection = $connection; @@ -70,6 +72,7 @@ class QuerySearchHelper { $this->logger = $logger; $this->searchBuilder = $searchBuilder; $this->queryOptimizer = $queryOptimizer; + $this->groupManager = $groupManager; } protected function getQueryBuilder() { @@ -119,16 +122,16 @@ class QuerySearchHelper { 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 @@ class QuerySearchHelper { $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 @@ class QuerySearchHelper { 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, array} */ diff --git a/lib/private/Files/Cache/SearchBuilder.php b/lib/private/Files/Cache/SearchBuilder.php index c9f35ccd095..b9a70bbd39b 100644 --- a/lib/private/Files/Cache/SearchBuilder.php +++ b/lib/private/Files/Cache/SearchBuilder.php @@ -69,20 +69,17 @@ class SearchBuilder { } /** - * 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 index 79c5adcf450..65046ebf000 100644 --- a/lib/private/SystemTag/SystemTagManager.php +++ b/lib/private/SystemTag/SystemTagManager.php @@ -91,7 +91,7 @@ class SystemTagManager implements ISystemTagManager { /** * {@inheritdoc} */ - public function getTagsByIds($tagIds): array { + public function getTagsByIds($tagIds, ?IUser $user = null): array { if (!\is_array($tagIds)) { $tagIds = [$tagIds]; } @@ -116,7 +116,12 @@ class SystemTagManager implements ISystemTagManager { $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 index 1cf7263b456..0f5c373c49f 100644 --- a/lib/public/SystemTag/ISystemTagManager.php +++ b/lib/public/SystemTag/ISystemTagManager.php @@ -38,6 +38,7 @@ 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 * @@ -45,9 +46,9 @@ interface ISystemTagManager { * @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. -- 2.39.5