From afe21f91370b302451184d985e9296c3d2d1a0a8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 22 Mar 2023 10:28:01 +0100 Subject: [PATCH 1/2] fix: Check tag attribute Signed-off-by: Joas Schilling --- lib/Operation.php | 37 +++++++++++++++--- tests/OperationTest.php | 86 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 7 deletions(-) diff --git a/lib/Operation.php b/lib/Operation.php index b0bfe5e9..68c0d2e3 100644 --- a/lib/Operation.php +++ b/lib/Operation.php @@ -32,8 +32,11 @@ use OCP\Files\Node; use OCP\Files\Storage\IStorage; use OCP\IConfig; +use OCP\IGroupManager; use OCP\IL10N; use OCP\IURLGenerator; +use OCP\IUser; +use OCP\IUserSession; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; use OCP\SystemTag\TagNotFoundException; @@ -54,6 +57,10 @@ class Operation implements ISpecificOperation, IComplexOperation { private IMountManager $mountManager; private IRootFolder $rootFolder; private File $fileEntity; + /** @var IUserSession */ + protected $userSession; + /** @var IGroupManager */ + protected $groupManager; public function __construct( ISystemTagObjectMapper $objectMapper, @@ -64,7 +71,9 @@ public function __construct( IURLGenerator $urlGenerator, IMountManager $mountManager, IRootFolder $rootFolder, - File $fileEntity + File $fileEntity, + IUserSession $userSession, + IGroupManager $groupManager ) { $this->objectMapper = $objectMapper; $this->tagManager = $tagManager; @@ -75,6 +84,8 @@ public function __construct( $this->mountManager = $mountManager; $this->rootFolder = $rootFolder; $this->fileEntity = $fileEntity; + $this->userSession = $userSession; + $this->groupManager = $groupManager; } public function checkOperations(IStorage $storage, int $fileId, string $file): void { @@ -100,16 +111,27 @@ public function checkOperations(IStorage $storage, int $fileId, string $file): v */ public function validateOperation(string $name, array $checks, string $operation): void { if ($operation === '') { - throw new UnexpectedValueException($this->l->t('No tags given')); + throw new UnexpectedValueException($this->l->t('No tags given'), 1); } $systemTagIds = explode(',', $operation); try { - $this->tagManager->getTagsByIds($systemTagIds); + $tags = $this->tagManager->getTagsByIds($systemTagIds); + + $user = $this->userSession->getUser(); + $isAdmin = $user instanceof IUser && $this->groupManager->isAdmin($user->getUID()); + + if (!$isAdmin) { + foreach ($tags as $tag) { + if (!$tag->isUserAssignable() || !$tag->isUserVisible()) { + throw new UnexpectedValueException($this->l->t('At least one of the given tags is invalid'), 4); + } + } + } } catch (TagNotFoundException $e) { - throw new UnexpectedValueException($this->l->t('Tag(s) could not be found: %s', implode(', ', $e->getMissingTags()))); + throw new UnexpectedValueException($this->l->t('At least one of the given tags is invalid'), 2); } catch (InvalidArgumentException $e) { - throw new UnexpectedValueException($this->l->t('At least one of the given tags is invalid')); + throw new UnexpectedValueException($this->l->t('At least one of the given tags is invalid'), 3); } } @@ -181,7 +203,10 @@ public function getIcon(): string { } public function isAvailableForScope(int $scope): bool { - return true; + return in_array($scope, [ + IManager::SCOPE_ADMIN, + IManager::SCOPE_USER, + ], true); } public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatcher): void { diff --git a/tests/OperationTest.php b/tests/OperationTest.php index 78a28d3f..c72fba04 100644 --- a/tests/OperationTest.php +++ b/tests/OperationTest.php @@ -31,10 +31,15 @@ use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage\IStorage; use OCP\IConfig; +use OCP\IGroupManager; use OCP\IL10N; use OCP\IURLGenerator; +use OCP\IUser; +use OCP\IUserSession; +use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; +use OCP\SystemTag\TagNotFoundException; use OCP\WorkflowEngine\IManager; use OCP\WorkflowEngine\IRuleMatcher; use PHPUnit\Framework\MockObject\MockObject; @@ -59,6 +64,10 @@ class OperationTest extends TestCase { protected $ruleMatcher; /** @var IMountManager|MockObject */ protected $mountManager; + /** @var IUserSession|MockObject */ + protected $userSession; + /** @var IGroupManager|MockObject */ + protected $groupManager; protected function setUp(): void { parent::setUp(); @@ -85,6 +94,8 @@ protected function setUp(): void { $this->mountManager = $this->createMock(IMountManager::class); $this->rootFolder = $this->createMock(IRootFolder::class); $this->fileEntity = $this->createMock(\OCA\WorkflowEngine\Entity\File::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->groupManager = $this->createMock(IGroupManager::class); $this->operation = new Operation( $this->objectMapper, @@ -95,7 +106,9 @@ protected function setUp(): void { $this->urlGenerator, $this->mountManager, $this->rootFolder, - $this->fileEntity + $this->fileEntity, + $this->userSession, + $this->groupManager ); } @@ -161,6 +174,77 @@ public function testCheckOperations(IStorage $storage, $fileId, $file, array $ma $this->operation->checkOperations($storage, $fileId, $file); } + public function dataValidateOperation() { + $public = $this->createMock(ISystemTag::class); + $public->method('isUserVisible') + ->willReturn(true); + $public->method('isUserAssignable') + ->willReturn(true); + $restricted = $this->createMock(ISystemTag::class); + $restricted->method('isUserVisible') + ->willReturn(true); + $restricted->method('isUserAssignable') + ->willReturn(false); + $invisible = $this->createMock(ISystemTag::class); + $invisible->method('isUserVisible') + ->willReturn(false); + $invisible->method('isUserAssignable') + ->willReturn(false); + + return [ + ['', null, false, 1], + ['1', [$public], false, null], + ['2,1', [$restricted, $public], false, 4], + ['1,3', [$public, $invisible], false, 4], + ['1', [$public], true, null], + ['2,1', [$restricted, $public], true, null], + ['1,3', [$public, $invisible], true, null], + ['4', new TagNotFoundException(), false, 2], + ['5', new \InvalidArgumentException(), false, 3], + ]; + } + + /** + * @dataProvider dataValidateOperation + * + * @param string $operation + * @param ISystemTag[]|\Exception|null $tags + * @param bool $isAdmin + * @param int|null $exceptionCode + */ + public function testValidateOperation(string $operation, $tags, bool $isAdmin, ?int $exceptionCode) { + if ($tags === null) { + $this->tagManager->expects($this->never()) + ->method('getTagsByIds'); + } elseif (is_array($tags)) { + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->willReturn($tags); + } else { + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->willThrowException($tags); + } + + if ($isAdmin) { + $userId = 'admin'; + $user = $this->createMock(IUser::class); + $user->method('getUID') + ->willReturn($userId); + $this->userSession->method('getUser') + ->willReturn($user); + $this->groupManager->method('isAdmin') + ->with($userId) + ->willReturn(true); + } + + if ($exceptionCode !== null) { + $this->expectExceptionCode($exceptionCode); + } + + $this->operation->validateOperation('', [], $operation); + } + public function taggingPathDataProvider() { $homeId = 'home::alice'; $localId = 'local::/mnt/users/alice'; From 10bcfd96d4ae94f651d065a929c469fab2cf5354 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 22 Mar 2023 10:56:56 +0100 Subject: [PATCH 2/2] fix(CI): Show colors for PHPUnit Signed-off-by: Joas Schilling --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 8dc55df3..c2aa43be 100644 --- a/composer.json +++ b/composer.json @@ -3,7 +3,7 @@ "cs:check": "php-cs-fixer fix --dry-run --diff", "cs:fix": "php-cs-fixer fix", "lint": "find . -name \\*.php -not -path './vendor/*' -not -path './build/*' -print0 | xargs -0 -n1 php -l", - "test:unit": "vendor/bin/phpunit -c tests/phpunit.xml" + "test:unit": "vendor/bin/phpunit -c tests/phpunit.xml --color" }, "config": { "optimize-autoloader": true,