From c6700150d53423f64c7608f757d3839b84e3932a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 15 Feb 2023 13:20:20 +0100 Subject: [PATCH 1/2] Validate the scope when validating operations Signed-off-by: Joas Schilling --- apps/workflowengine/lib/Manager.php | 13 ++- apps/workflowengine/tests/ManagerTest.php | 113 +++++++++++++++++++++- 2 files changed, 118 insertions(+), 8 deletions(-) diff --git a/apps/workflowengine/lib/Manager.php b/apps/workflowengine/lib/Manager.php index 417e1a1eba657..7ef913bb1a517 100644 --- a/apps/workflowengine/lib/Manager.php +++ b/apps/workflowengine/lib/Manager.php @@ -323,7 +323,7 @@ public function addOperation( string $entity, array $events ) { - $this->validateOperation($class, $name, $checks, $operation, $entity, $events); + $this->validateOperation($class, $name, $checks, $operation, $scope, $entity, $events); $this->connection->beginTransaction(); @@ -396,7 +396,7 @@ public function updateOperation( throw new \DomainException('Target operation not within scope'); }; $row = $this->getOperation($id); - $this->validateOperation($row['class'], $name, $checks, $operation, $entity, $events); + $this->validateOperation($row['class'], $name, $checks, $operation, $scopeContext, $entity, $events); $checkIds = []; try { @@ -499,9 +499,12 @@ protected function validateEvents(string $entity, array $events, IOperation $ope * @param string $name * @param array[] $checks * @param string $operation + * @param ScopeContext $scope + * @param string $entity + * @param array $events * @throws \UnexpectedValueException */ - public function validateOperation($class, $name, array $checks, $operation, string $entity, array $events) { + public function validateOperation($class, $name, array $checks, $operation, ScopeContext $scope, string $entity, array $events) { try { /** @var IOperation $instance */ $instance = $this->container->query($class); @@ -513,6 +516,10 @@ public function validateOperation($class, $name, array $checks, $operation, stri throw new \UnexpectedValueException($this->l->t('Operation %s is invalid', [$class])); } + if (!$instance->isAvailableForScope($scope->getScope())) { + throw new \UnexpectedValueException($this->l->t('Operation %s is invalid', [$class])); + } + $this->validateEvents($entity, $events, $instance); if (count($checks) === 0) { diff --git a/apps/workflowengine/tests/ManagerTest.php b/apps/workflowengine/tests/ManagerTest.php index d3678b66beeb0..bb77377210e88 100644 --- a/apps/workflowengine/tests/ManagerTest.php +++ b/apps/workflowengine/tests/ManagerTest.php @@ -334,11 +334,20 @@ public function testUpdateOperation() { ->with('events'); $this->cacheFactory->method('createDistributed')->willReturn($cache); + $operationMock = $this->createMock(IOperation::class); + $operationMock->expects($this->any()) + ->method('isAvailableForScope') + ->withConsecutive( + [IManager::SCOPE_ADMIN], + [IManager::SCOPE_USER] + ) + ->willReturn(true); + $this->container->expects($this->any()) ->method('query') - ->willReturnCallback(function ($class) { + ->willReturnCallback(function ($class) use ($operationMock) { if (substr($class, -2) === 'Op') { - return $this->createMock(IOperation::class); + return $operationMock; } elseif ($class === File::class) { return $this->getMockBuilder(File::class) ->setConstructorArgs([ @@ -505,6 +514,16 @@ public function testValidateOperationOK() { $entityMock = $this->createMock(IEntity::class); $eventEntityMock = $this->createMock(IEntityEvent::class); $checkMock = $this->createMock(ICheck::class); + $scopeMock = $this->createMock(ScopeContext::class); + + $scopeMock->expects($this->any()) + ->method('getScope') + ->willReturn(IManager::SCOPE_ADMIN); + + $operationMock->expects($this->once()) + ->method('isAvailableForScope') + ->with(IManager::SCOPE_ADMIN) + ->willReturn(true); $operationMock->expects($this->once()) ->method('validateOperation') @@ -541,7 +560,7 @@ public function testValidateOperationOK() { } }); - $this->manager->validateOperation(IOperation::class, 'test', [$check], 'operationData', IEntity::class, ['MyEvent']); + $this->manager->validateOperation(IOperation::class, 'test', [$check], 'operationData', $scopeMock, IEntity::class, ['MyEvent']); } public function testValidateOperationCheckInputLengthError() { @@ -555,6 +574,16 @@ public function testValidateOperationCheckInputLengthError() { $entityMock = $this->createMock(IEntity::class); $eventEntityMock = $this->createMock(IEntityEvent::class); $checkMock = $this->createMock(ICheck::class); + $scopeMock = $this->createMock(ScopeContext::class); + + $scopeMock->expects($this->any()) + ->method('getScope') + ->willReturn(IManager::SCOPE_ADMIN); + + $operationMock->expects($this->once()) + ->method('isAvailableForScope') + ->with(IManager::SCOPE_ADMIN) + ->willReturn(true); $operationMock->expects($this->once()) ->method('validateOperation') @@ -592,7 +621,7 @@ public function testValidateOperationCheckInputLengthError() { }); try { - $this->manager->validateOperation(IOperation::class, 'test', [$check], 'operationData', IEntity::class, ['MyEvent']); + $this->manager->validateOperation(IOperation::class, 'test', [$check], 'operationData', $scopeMock, IEntity::class, ['MyEvent']); } catch (\UnexpectedValueException $e) { $this->assertSame('The provided check value is too long', $e->getMessage()); } @@ -610,6 +639,16 @@ public function testValidateOperationDataLengthError() { $entityMock = $this->createMock(IEntity::class); $eventEntityMock = $this->createMock(IEntityEvent::class); $checkMock = $this->createMock(ICheck::class); + $scopeMock = $this->createMock(ScopeContext::class); + + $scopeMock->expects($this->any()) + ->method('getScope') + ->willReturn(IManager::SCOPE_ADMIN); + + $operationMock->expects($this->once()) + ->method('isAvailableForScope') + ->with(IManager::SCOPE_ADMIN) + ->willReturn(true); $operationMock->expects($this->never()) ->method('validateOperation'); @@ -646,9 +685,73 @@ public function testValidateOperationDataLengthError() { }); try { - $this->manager->validateOperation(IOperation::class, 'test', [$check], $operationData, IEntity::class, ['MyEvent']); + $this->manager->validateOperation(IOperation::class, 'test', [$check], $operationData, $scopeMock, IEntity::class, ['MyEvent']); } catch (\UnexpectedValueException $e) { $this->assertSame('The provided operation data is too long', $e->getMessage()); } } + + public function testValidateOperationScopeNotAvailable() { + $check = [ + 'class' => ICheck::class, + 'operator' => 'is', + 'value' => 'barfoo', + ]; + $operationData = str_pad('', IManager::MAX_OPERATION_VALUE_BYTES + 1, 'FooBar'); + + $operationMock = $this->createMock(IOperation::class); + $entityMock = $this->createMock(IEntity::class); + $eventEntityMock = $this->createMock(IEntityEvent::class); + $checkMock = $this->createMock(ICheck::class); + $scopeMock = $this->createMock(ScopeContext::class); + + $scopeMock->expects($this->any()) + ->method('getScope') + ->willReturn(IManager::SCOPE_ADMIN); + + $operationMock->expects($this->once()) + ->method('isAvailableForScope') + ->with(IManager::SCOPE_ADMIN) + ->willReturn(false); + + $operationMock->expects($this->never()) + ->method('validateOperation'); + + $entityMock->expects($this->any()) + ->method('getEvents') + ->willReturn([$eventEntityMock]); + + $eventEntityMock->expects($this->any()) + ->method('getEventName') + ->willReturn('MyEvent'); + + $checkMock->expects($this->any()) + ->method('supportedEntities') + ->willReturn([IEntity::class]); + $checkMock->expects($this->never()) + ->method('validateCheck'); + + $this->container->expects($this->any()) + ->method('query') + ->willReturnCallback(function ($className) use ($operationMock, $entityMock, $eventEntityMock, $checkMock) { + switch ($className) { + case IOperation::class: + return $operationMock; + case IEntity::class: + return $entityMock; + case IEntityEvent::class: + return $eventEntityMock; + case ICheck::class: + return $checkMock; + default: + return $this->createMock($className); + } + }); + + try { + $this->manager->validateOperation(IOperation::class, 'test', [$check], $operationData, $scopeMock, IEntity::class, ['MyEvent']); + } catch (\UnexpectedValueException $e) { + $this->assertSame('Operation OCP\WorkflowEngine\IOperation is invalid', $e->getMessage()); + } + } } From 15fc42966c7788aaf372514b3e0a9a8b1847d26a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 15 Feb 2023 15:36:32 +0100 Subject: [PATCH 2/2] Also check the scope when reading operations from the database Signed-off-by: Joas Schilling --- apps/workflowengine/lib/Manager.php | 23 ++++++++++ apps/workflowengine/tests/ManagerTest.php | 53 +++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/apps/workflowengine/lib/Manager.php b/apps/workflowengine/lib/Manager.php index 7ef913bb1a517..674cfc653136a 100644 --- a/apps/workflowengine/lib/Manager.php +++ b/apps/workflowengine/lib/Manager.php @@ -200,6 +200,13 @@ public function getAllConfiguredScopesForOperation(string $operationClass): arra return $scopesByOperation[$operationClass]; } + try { + /** @var IOperation $operation */ + $operation = $this->container->query($operationClass); + } catch (QueryException $e) { + return []; + } + $query = $this->connection->getQueryBuilder(); $query->selectDistinct('s.type') @@ -214,6 +221,11 @@ public function getAllConfiguredScopesForOperation(string $operationClass): arra $scopesByOperation[$operationClass] = []; while ($row = $result->fetch()) { $scope = new ScopeContext($row['type'], $row['value']); + + if (!$operation->isAvailableForScope((int) $row['type'])) { + continue; + } + $scopesByOperation[$operationClass][$scope->getHash()] = $scope; } @@ -243,6 +255,17 @@ public function getAllOperations(ScopeContext $scopeContext): array { $this->operations[$scopeContext->getHash()] = []; while ($row = $result->fetch()) { + try { + /** @var IOperation $operation */ + $operation = $this->container->query($row['class']); + } catch (QueryException $e) { + continue; + } + + if (!$operation->isAvailableForScope((int) $row['scope_type'])) { + continue; + } + if (!isset($this->operations[$scopeContext->getHash()][$row['class']])) { $this->operations[$scopeContext->getHash()][$row['class']] = []; } diff --git a/apps/workflowengine/tests/ManagerTest.php b/apps/workflowengine/tests/ManagerTest.php index bb77377210e88..543b4550ca6aa 100644 --- a/apps/workflowengine/tests/ManagerTest.php +++ b/apps/workflowengine/tests/ManagerTest.php @@ -30,6 +30,7 @@ use OCA\WorkflowEngine\Entity\File; use OCA\WorkflowEngine\Helper\ScopeContext; use OCA\WorkflowEngine\Manager; +use OCP\AppFramework\QueryException; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Events\Node\NodeCreatedEvent; use OCP\Files\IRootFolder; @@ -205,6 +206,32 @@ public function testGetAllOperations() { $userScope = $this->buildScope('jackie'); $entity = File::class; + $adminOperation = $this->createMock(IOperation::class); + $adminOperation->expects($this->any()) + ->method('isAvailableForScope') + ->willReturnMap([ + [IManager::SCOPE_ADMIN, true], + [IManager::SCOPE_USER, false], + ]); + $userOperation = $this->createMock(IOperation::class); + $userOperation->expects($this->any()) + ->method('isAvailableForScope') + ->willReturnMap([ + [IManager::SCOPE_ADMIN, false], + [IManager::SCOPE_USER, true], + ]); + + $this->container->expects($this->any()) + ->method('query') + ->willReturnCallback(function ($className) use ($adminOperation, $userOperation) { + switch ($className) { + case 'OCA\WFE\TestAdminOp': + return $adminOperation; + case 'OCA\WFE\TestUserOp': + return $userOperation; + } + }); + $opId1 = $this->invokePrivate( $this->manager, 'insertOperation', @@ -225,6 +252,13 @@ public function testGetAllOperations() { ); $this->invokePrivate($this->manager, 'addScope', [$opId3, $userScope]); + $opId4 = $this->invokePrivate( + $this->manager, + 'insertOperation', + ['OCA\WFE\TestAdminOp', 'Test04', [41, 10, 4], 'NoBar', $entity, []] + ); + $this->invokePrivate($this->manager, 'addScope', [$opId4, $userScope]); + $adminOps = $this->manager->getAllOperations($adminScope); $userOps = $this->manager->getAllOperations($userScope); @@ -275,6 +309,25 @@ public function testGetOperations() { ); $this->invokePrivate($this->manager, 'addScope', [$opId5, $userScope]); + $operation = $this->createMock(IOperation::class); + $operation->expects($this->any()) + ->method('isAvailableForScope') + ->willReturnMap([ + [IManager::SCOPE_ADMIN, true], + [IManager::SCOPE_USER, true], + ]); + + $this->container->expects($this->any()) + ->method('query') + ->willReturnCallback(function ($className) use ($operation) { + switch ($className) { + case 'OCA\WFE\TestOp': + return $operation; + case 'OCA\WFE\OtherTestOp': + throw new QueryException(); + } + }); + $adminOps = $this->manager->getOperations('OCA\WFE\TestOp', $adminScope); $userOps = $this->manager->getOperations('OCA\WFE\TestOp', $userScope);