Merge pull request #36816 from nextcloud/backport/36814/stable24

[stable24] Validate the scope when validating operations

2 files changed, 194 insertions, 8 deletions

diff --git a/apps/workflowengine/lib/Manager.php b/apps/workflowengine/lib/Manager.php
index 34dbf507b91..a06b56dc208 100644
--- a/apps/workflowengine/lib/Manager.php
+++ b/apps/workflowengine/lib/Manager.php
@@ -189,6 +189,13 @@ class Manager implements IManager {
 			return $scopesByOperation[$operationClass];
 		}
 
+		try {
+			/** @var IOperation $operation */
+			$operation = $this->container->query($operationClass);
+		} catch (QueryException $e) {
+			return [];
+		}
+
 		$query = $this->connection->getQueryBuilder();
 
 		$query->selectDistinct('s.type')
@@ -203,6 +210,11 @@ class Manager implements IManager {
 		$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;
 		}
 
@@ -232,6 +244,17 @@ class Manager implements IManager {
 
 		$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']] = [];
 			}
@@ -310,7 +333,7 @@ class Manager implements IManager {
 		string $entity,
 		array $events
 	) {
-		$this->validateOperation($class, $name, $checks, $operation, $entity, $events);
+		$this->validateOperation($class, $name, $checks, $operation, $scope, $entity, $events);
 
 		$this->connection->beginTransaction();
 
@@ -382,7 +405,7 @@ class Manager implements IManager {
 			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 {
@@ -482,9 +505,12 @@ class Manager implements IManager {
 	 * @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);
@@ -496,6 +522,10 @@ class Manager implements IManager {
 			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 612495a5b6d..4284ef9c8c4 100644
--- a/apps/workflowengine/tests/ManagerTest.php
+++ b/apps/workflowengine/tests/ManagerTest.php
@@ -30,6 +30,7 @@ use OC\L10N\L10N;
 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\IRootFolder;
 use OCP\IConfig;
@@ -199,6 +200,32 @@ class ManagerTest extends TestCase {
 		$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',
@@ -219,6 +246,13 @@ class ManagerTest extends TestCase {
 		);
 		$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);
 
@@ -269,6 +303,25 @@ class ManagerTest extends TestCase {
 		);
 		$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);
 
@@ -288,11 +341,20 @@ class ManagerTest extends TestCase {
 		$userScope = $this->buildScope('jackie');
 		$entity = File::class;
 
+		$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([
@@ -453,6 +515,16 @@ class ManagerTest extends TestCase {
 		$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')
@@ -489,7 +561,7 @@ class ManagerTest extends TestCase {
 				}
 			});
 
-		$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() {
@@ -503,6 +575,16 @@ class ManagerTest extends TestCase {
 		$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')
@@ -540,7 +622,7 @@ class ManagerTest extends TestCase {
 			});
 
 		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());
 		}
@@ -558,6 +640,16 @@ class ManagerTest extends TestCase {
 		$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');
@@ -594,9 +686,73 @@ class ManagerTest extends TestCase {
 			});
 
 		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());
+		}
+	}
 }