Merge pull request #56847 from nextcloud/backport/56839/stable28

[stable28] refactor(workflowengine): Check if class is correct

apps/workflowengine/lib/Manager.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * @copyright Copyright (c) 2016 Morris Jobke <hey@morrisjobke.de>
  *
@@ -440,6 +441,13 @@ class Manager implements IManager {
 	}
 
 	protected function validateEvents(string $entity, array $events, IOperation $operation) {
+		/** @psalm-suppress TaintedCallable newInstance is not called */
+		$reflection = new \ReflectionClass($entity);
+
+		if ($entity !== IEntity::class && !in_array(IEntity::class, $reflection->getInterfaceNames())) {
+			throw new \UnexpectedValueException($this->l->t('Entity %s is invalid', [$entity]));
+		}
+
 		try {
 			/** @var IEntity $instance */
 			$instance = $this->container->query($entity);
@@ -447,10 +455,6 @@ class Manager implements IManager {
 			throw new \UnexpectedValueException($this->l->t('Entity %s does not exist', [$entity]));
 		}
 
-		if (!$instance instanceof IEntity) {
-			throw new \UnexpectedValueException($this->l->t('Entity %s is invalid', [$entity]));
-		}
-
 		if (empty($events)) {
 			if (!$operation instanceof IComplexOperation) {
 				throw new \UnexpectedValueException($this->l->t('No events are chosen.'));
@@ -481,6 +485,16 @@ class Manager implements IManager {
 	 * @throws \UnexpectedValueException
 	 */
 	public function validateOperation($class, $name, array $checks, $operation, ScopeContext $scope, string $entity, array $events) {
+		if (strlen($operation) > IManager::MAX_OPERATION_VALUE_BYTES) {
+			throw new \UnexpectedValueException($this->l->t('The provided operation data is too long'));
+		}
+
+		/** @psalm-suppress TaintedCallable newInstance is not called */
+		$reflection = new \ReflectionClass($class);
+		if ($class !== IOperation::class && !in_array(IOperation::class, $reflection->getInterfaceNames())) {
+			throw new \UnexpectedValueException($this->l->t('Operation %s is invalid', [$class]) . join(', ', $reflection->getInterfaceNames()));
+		}
+
 		try {
 			/** @var IOperation $instance */
 			$instance = $this->container->query($class);
@@ -488,10 +502,6 @@ class Manager implements IManager {
 			throw new \UnexpectedValueException($this->l->t('Operation %s does not exist', [$class]));
 		}
 
-		if (!($instance instanceof IOperation)) {
-			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]));
 		}
@@ -513,6 +523,15 @@ class Manager implements IManager {
 				throw new \UnexpectedValueException($this->l->t('Invalid check provided'));
 			}
 
+			if (strlen((string)$check['value']) > IManager::MAX_CHECK_VALUE_BYTES) {
+				throw new \UnexpectedValueException($this->l->t('The provided check value is too long'));
+			}
+
+			$reflection = new \ReflectionClass($check['class']);
+			if ($check['class'] !== ICheck::class && !in_array(ICheck::class, $reflection->getInterfaceNames())) {
+				throw new \UnexpectedValueException($this->l->t('Check %s is invalid', [$class]));
+			}
+
 			try {
 				/** @var ICheck $instance */
 				$instance = $this->container->query($check['class']);
@@ -520,20 +539,12 @@ class Manager implements IManager {
 				throw new \UnexpectedValueException($this->l->t('Check %s does not exist', [$class]));
 			}
 
-			if (!($instance instanceof ICheck)) {
-				throw new \UnexpectedValueException($this->l->t('Check %s is invalid', [$class]));
-			}
-
 			if (!empty($instance->supportedEntities())
 				&& !in_array($entity, $instance->supportedEntities())
 			) {
 				throw new \UnexpectedValueException($this->l->t('Check %s is not allowed with this entity', [$class]));
 			}
 
-			if (strlen((string)$check['value']) > IManager::MAX_CHECK_VALUE_BYTES) {
-				throw new \UnexpectedValueException($this->l->t('The provided check value is too long'));
-			}
-
 			$instance->validateCheck($check['operator'], $check['value']);
 		}
 	}

apps/workflowengine/tests/ManagerTest.php

@@ -32,6 +32,7 @@ use OCA\WorkflowEngine\Entity\File;
 use OCA\WorkflowEngine\Helper\ScopeContext;
 use OCA\WorkflowEngine\Manager;
 use OCP\AppFramework\QueryException;
+use OCP\EventDispatcher\Event;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Events\Node\NodeCreatedEvent;
 use OCP\Files\IRootFolder;
@@ -52,10 +53,41 @@ use OCP\WorkflowEngine\IEntity;
 use OCP\WorkflowEngine\IEntityEvent;
 use OCP\WorkflowEngine\IManager;
 use OCP\WorkflowEngine\IOperation;
+use OCP\WorkflowEngine\IRuleMatcher;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
+class TestAdminOp implements IOperation {
+	public function getDisplayName(): string {
+		return 'Admin';
+	}
+
+	public function getDescription(): string {
+		return '';
+	}
+
+	public function getIcon(): string {
+		return '';
+	}
+
+	public function isAvailableForScope(int $scope): bool {
+		return true;
+	}
+
+	public function validateOperation(string $name, array $checks, string $operation): void {
+	}
+
+	public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatcher): void {
+	}
+}
+
+class TestUserOp extends TestAdminOp {
+	public function getDisplayName(): string {
+		return 'User';
+	}
+}
+
 /**
  * Class ManagerTest
  *
@@ -420,19 +452,19 @@ class ManagerTest extends TestCase {
 		$opId1 = $this->invokePrivate(
 			$this->manager,
 			'insertOperation',
-			['OCA\WFE\TestAdminOp', 'Test01', [11, 22], 'foo', $entity, []]
+			[TestAdminOp::class, 'Test01', [11, 22], 'foo', $entity, []]
 		);
 		$this->invokePrivate($this->manager, 'addScope', [$opId1, $adminScope]);
 
 		$opId2 = $this->invokePrivate(
 			$this->manager,
 			'insertOperation',
-			['OCA\WFE\TestUserOp', 'Test02', [33, 22], 'bar', $entity, []]
+			[TestUserOp::class, 'Test02', [33, 22], 'bar', $entity, []]
 		);
 		$this->invokePrivate($this->manager, 'addScope', [$opId2, $userScope]);
 
-		$check1 = ['class' => 'OCA\WFE\C22', 'operator' => 'eq', 'value' => 'asdf'];
-		$check2 = ['class' => 'OCA\WFE\C33', 'operator' => 'eq', 'value' => 23456];
+		$check1 = ['class' => ICheck::class, 'operator' => 'eq', 'value' => 'asdf'];
+		$check2 = ['class' => ICheck::class, 'operator' => 'eq', 'value' => 23456];
 
 		/** @noinspection PhpUnhandledExceptionInspection */
 		$op = $this->manager->updateOperation($opId1, 'Test01a', [$check1, $check2], 'foohur', $adminScope, $entity, ['\OCP\Files::postDelete']);
@@ -695,11 +727,6 @@ class ManagerTest extends TestCase {
 			->method('getScope')
 			->willReturn(IManager::SCOPE_ADMIN);
 
-		$operationMock->expects($this->once())
-			->method('isAvailableForScope')
-			->with(IManager::SCOPE_ADMIN)
-			->willReturn(true);
-
 		$operationMock->expects($this->never())
 			->method('validateOperation');
 
@@ -747,7 +774,7 @@ class ManagerTest extends TestCase {
 			'operator' => 'is',
 			'value' => 'barfoo',
 		];
-		$operationData = str_pad('', IManager::MAX_OPERATION_VALUE_BYTES + 1, 'FooBar');
+		$operationData = str_pad('', IManager::MAX_OPERATION_VALUE_BYTES - 1, 'FooBar');
 
 		$operationMock = $this->createMock(IOperation::class);
 		$entityMock = $this->createMock(IEntity::class);