diff --git a/application/Espo/Core/Select/Where/Checker.php b/application/Espo/Core/Select/Where/Checker.php index e69f88ee83..9a0534449b 100644 --- a/application/Espo/Core/Select/Where/Checker.php +++ b/application/Espo/Core/Select/Where/Checker.php @@ -33,8 +33,9 @@ use Espo\Core\Exceptions\Forbidden; use Espo\Core\Exceptions\BadRequest; use Espo\Core\Acl; use Espo\Core\Select\Where\Item\Type; +use Espo\Entities\Team; use Espo\ORM\Defs\Params\RelationParam; -use Espo\ORM\QueryComposer\BaseQueryComposer as QueryComposer; +use Espo\ORM\QueryComposer\Util; use Espo\ORM\QueryComposer\Util as QueryUtil; use Espo\ORM\EntityManager; use Espo\ORM\Entity; @@ -42,6 +43,8 @@ use Espo\ORM\BaseEntity; /** * Checks Where parameters. Throws an exception if anything not allowed is met. + * + * @todo Check read access to foreign entity for belongs-to, belongs-to-parent, has-one. */ class Checker { @@ -70,8 +73,17 @@ class Checker private $linkTypeList = [ self::TYPE_IN_CATEGORY, self::TYPE_IS_USER_FROM_TEAMS, + Type::IS_LINKED_WITH, + Type::IS_NOT_LINKED_WITH, + Type::IS_LINKED_WITH_ALL, Type::IS_LINKED_WITH_ANY, Type::IS_LINKED_WITH_NONE, + ]; + + /** @var string[] */ + private $linkWithIdsTypeList = [ + self::TYPE_IN_CATEGORY, + self::TYPE_IS_USER_FROM_TEAMS, Type::IS_LINKED_WITH, Type::IS_NOT_LINKED_WITH, Type::IS_LINKED_WITH_ALL, @@ -80,7 +92,7 @@ class Checker public function __construct( private string $entityType, private EntityManager $entityManager, - private Acl $acl + private Acl $acl, ) {} /** @@ -120,13 +132,13 @@ class Checker } if ($attribute) { - $argumentList = QueryComposer::getAllAttributesFromComplexExpression($attribute); + $argumentList = Util::getAllAttributesFromComplexExpression($attribute); foreach ($argumentList as $argument) { $this->checkAttributeExistence($argument, $type); if ($checkWherePermission) { - $this->checkAttributePermission($argument, $type); + $this->checkAttributePermission($argument, $type, $value); } } } @@ -163,8 +175,9 @@ class Checker /** * @throws Forbidden + * @throws BadRequest */ - private function checkAttributePermission(string $attribute, string $type): void + private function checkAttributePermission(string $attribute, string $type, mixed $value): void { $entityType = $this->entityType; @@ -176,7 +189,7 @@ class Checker throw new Forbidden("Bad relation '$link' in where."); } - $foreignEntityType = $this->getRelationParam($this->getSeed(), $link, RelationParam::ENTITY); + $foreignEntityType = $this->getRelationEntityType($this->getSeed(), $link); if (!$foreignEntityType) { throw new Forbidden("Bad relation '$link' in where."); @@ -190,32 +203,14 @@ class Checker } if (in_array($attribute, $this->acl->getScopeForbiddenAttributeList($foreignEntityType))) { - throw new Forbidden("Forbidden attribute '$link.{$attribute}' in where."); + throw new Forbidden("Forbidden attribute '$link.$attribute' in where."); } return; } if (in_array($type, $this->linkTypeList)) { - $link = $attribute; - - if (!$this->getSeed()->hasRelation($link)) { - throw new Forbidden("Bad relation '$link' in where."); - } - - $foreignEntityType = $this->getRelationParam($this->getSeed(), $link, RelationParam::ENTITY); - - if (!$foreignEntityType) { - throw new Forbidden("Bad relation '$link' in where."); - } - - if ( - in_array($link, $this->acl->getScopeForbiddenFieldList($entityType)) || - !$this->acl->checkScope($foreignEntityType) || - in_array($link, $this->acl->getScopeForbiddenLinkList($entityType)) - ) { - throw new Forbidden("Forbidden relation '$link' in where."); - } + $this->checkLink($type, $entityType, $attribute, $value); return; } @@ -232,10 +227,10 @@ class Checker return $this->seed; } - private function getRelationParam(Entity $entity, string $relation, string $param): mixed + private function getRelationEntityType(Entity $entity, string $relation): mixed { if ($entity instanceof BaseEntity) { - return $entity->getRelationParam($relation, $param); + return $entity->getRelationParam($relation, RelationParam::ENTITY); } $entityDefs = $this->entityManager @@ -246,6 +241,67 @@ class Checker return null; } - return $entityDefs->getRelation($relation)->getParam($param); + return $entityDefs->getRelation($relation)->getParam(RelationParam::ENTITY); + } + + /** + * @throws BadRequest + * @throws Forbidden + */ + private function checkLink(string $type, string $entityType, string $link, mixed $value): void + { + if (!$this->getSeed()->hasRelation($link)) { + throw new Forbidden("Bad relation '$link' in where."); + } + + $foreignEntityType = $this->getRelationEntityType($this->getSeed(), $link); + + if (!$foreignEntityType) { + throw new Forbidden("Bad relation '$link' in where."); + } + + if ($type === self::TYPE_IS_USER_FROM_TEAMS) { + $foreignEntityType = Team::ENTITY_TYPE; + } + + if ( + in_array($link, $this->acl->getScopeForbiddenFieldList($entityType)) || + !$this->acl->checkScope($foreignEntityType) || + in_array($link, $this->acl->getScopeForbiddenLinkList($entityType)) + ) { + throw new Forbidden("Forbidden relation '$link' in where."); + } + + if (!in_array($type, $this->linkWithIdsTypeList)) { + return; + } + + if ($value === null) { + return; + } + + if (!is_array($value)) { + $value = [$value]; + } + + foreach ($value as $it) { + if (!is_string($it)) { + throw new BadRequest("Bad where item. Non-string ID."); + } + } + + // @todo Use the Select Builder instead. Check the result count equal the input IDs count. + + foreach ($value as $id) { + $entity = $this->entityManager->getEntityById($foreignEntityType, $id); + + if (!$entity) { + throw new Forbidden("Record '$foreignEntityType' `$id` not found."); + } + + if (!$this->acl->checkEntityRead($entity)) { + throw new Forbidden("No access to '$foreignEntityType' `$id`."); + } + } } } diff --git a/application/Espo/Core/Select/Where/ItemConverters/InCategory.php b/application/Espo/Core/Select/Where/ItemConverters/InCategory.php index 1ba6ce5d50..4651da65e2 100644 --- a/application/Espo/Core/Select/Where/ItemConverters/InCategory.php +++ b/application/Espo/Core/Select/Where/ItemConverters/InCategory.php @@ -63,6 +63,16 @@ class InCategory implements ItemConverter return WhereClause::create(); } + if (!is_array($value)) { + $value = [$value]; + } + + foreach ($value as $it) { + if (!is_string($it) && !is_int($it)) { + throw new BadRequest("Bad where item. Bad array item."); + } + } + $entityDefs = $this->ormDefs->getEntity($this->entityType); if (!$entityDefs->hasRelation($link)) { diff --git a/application/Espo/Core/Select/Where/ItemConverters/IsUserFromTeams.php b/application/Espo/Core/Select/Where/ItemConverters/IsUserFromTeams.php index da5431ce91..b7e5eaf661 100644 --- a/application/Espo/Core/Select/Where/ItemConverters/IsUserFromTeams.php +++ b/application/Espo/Core/Select/Where/ItemConverters/IsUserFromTeams.php @@ -66,8 +66,14 @@ class IsUserFromTeams implements ItemConverter return WhereClause::create(); } - if (is_array($value) && count($value) == 1) { - $value = $value[0]; + if (!is_array($value)) { + $value = [$value]; + } + + foreach ($value as $it) { + if (!is_string($it) && !is_int($it)) { + throw new BadRequest("Bad where item. Bad array item."); + } } $entityDefs = $this->ormDefs->getEntity($this->entityType); diff --git a/application/Espo/Core/Select/Where/ItemGeneralConverter.php b/application/Espo/Core/Select/Where/ItemGeneralConverter.php index 4ac7af8c46..2a20d79e91 100644 --- a/application/Espo/Core/Select/Where/ItemGeneralConverter.php +++ b/application/Espo/Core/Select/Where/ItemGeneralConverter.php @@ -1460,13 +1460,17 @@ class ItemGeneralConverter implements ItemConverter $defs = $this->ormDefs->getEntity($this->entityType)->getRelation($link); - $alias = $link . 'LinkedWithFilter' . $this->randomStringGenerator->generate(); + $alias = $link . 'LinkedWithFilter' . $this->randomStringGenerator->generate(); - if (!$value && !is_array($value)) { - throw new BadRequest("Bad where item. Empty value."); + if (!is_array($value)) { + $value = [$value]; } - // @todo Add check for foreign record existence. + foreach ($value as $it) { + if (!is_string($it) && !is_int($it)) { + throw new BadRequest("Bad where item. Bad array item."); + } + } $relationType = $defs->getType(); @@ -1536,10 +1540,16 @@ class ItemGeneralConverter implements ItemConverter $defs = $this->ormDefs->getEntity($this->entityType)->getRelation($link); - $alias = $link . 'NotLinkedWithFilter' . $this->randomStringGenerator->generate(); + $alias = $link . 'NotLinkedWithFilter' . $this->randomStringGenerator->generate(); - if (is_null($value)) { - throw new BadRequest("Bad where item. Empty value."); + if (!is_array($value)) { + $value = [$value]; + } + + foreach ($value as $it) { + if (!is_string($it) && !is_int($it)) { + throw new BadRequest("Bad where item. Bad array item."); + } } $relationType = $defs->getType(); @@ -1615,6 +1625,12 @@ class ItemGeneralConverter implements ItemConverter $value = [$value]; } + foreach ($value as $it) { + if (!is_string($it) && !is_int($it)) { + throw new BadRequest("Bad where item. Bad array item."); + } + } + $defs = $this->ormDefs->getEntity($this->entityType)->getRelation($link); $relationType = $defs->getType(); diff --git a/tests/unit/Espo/Core/Select/Where/CheckerTest.php b/tests/unit/Espo/Core/Select/Where/CheckerTest.php index 9b418b1b2f..193ffbe1e0 100644 --- a/tests/unit/Espo/Core/Select/Where/CheckerTest.php +++ b/tests/unit/Espo/Core/Select/Where/CheckerTest.php @@ -29,53 +29,57 @@ namespace tests\unit\Espo\Core\Select\Where; -use Espo\{ - Core\Exceptions\Forbidden, - Core\Exceptions\BadRequest, - Core\Select\Where\Checker, - Core\Select\Where\Item, - Core\Select\Where\Params, - Core\Acl, - ORM\EntityManager, - ORM\BaseEntity as Entity, -}; +use Espo\Core\Acl; +use Espo\Core\Acl\Table; +use Espo\Core\Exceptions\BadRequest; +use Espo\Core\Exceptions\Forbidden; +use Espo\Core\Select\Where\Checker; +use Espo\Core\Select\Where\Item; +use Espo\Core\Select\Where\Params; +use Espo\ORM\BaseEntity as Entity; +use Espo\ORM\EntityManager; +use PHPUnit\Framework\TestCase; -class CheckerTest extends \PHPUnit\Framework\TestCase +class CheckerTest extends TestCase { + /** @var Checker|null */ + protected $checker = null; + /** @var EntityManager|null */ + protected $entityManager = null; + /** @var Acl|null */ + protected $acl = null; + + protected ?string $entityType = null; + protected ?string $foreignEntityType = null; + protected function setUp() : void { $this->entityManager = $this->createMock(EntityManager::class); $this->acl = $this->createMock(Acl::class); $this->entityType = 'Test'; - - $this->foreignEntityType = 'Test'; + $this->foreignEntityType = 'TestForeign'; $this->checker = new Checker( $this->entityType, $this->entityManager, - $this->acl + $this->acl, ); $this->params = $this->createMock(Params::class); - $this->entity = $this->createMock(Entity::class); - $this->foreignEntity = $this->createMock(Entity::class); $this->entityManager ->expects($this->any()) ->method('getNewEntity') - ->with($this->entityType) - ->willReturn($this->entity); - - $this->entityManager - ->expects($this->any()) - ->method('getNewEntity') - ->with($this->foreignEntityType) - ->willReturn($this->foreignEntity); + ->willReturnMap([ + [$this->entityType, $this->entity], + [$this->foreignEntityType, $this->foreignEntity], + ]); } + /** @noinspection PhpUnhandledExceptionInspection */ public function testAttributeExistence1() { $this->params @@ -129,6 +133,7 @@ class CheckerTest extends \PHPUnit\Framework\TestCase $this->checker->check($item, $this->params); } + /** @noinspection PhpUnhandledExceptionInspection */ public function testAttributeExistence2() { $this->params @@ -166,6 +171,7 @@ class CheckerTest extends \PHPUnit\Framework\TestCase $this->checker->check($item, $this->params); } + /** @noinspection PhpUnhandledExceptionInspection */ public function testAttributeExistence3() { $this->params @@ -200,7 +206,8 @@ class CheckerTest extends \PHPUnit\Framework\TestCase $this->checker->check($item, $this->params); } - public function testPrermissions1() + /** @noinspection PhpUnhandledExceptionInspection */ + public function testPermissions1() { $this->params ->expects($this->any()) @@ -250,17 +257,13 @@ class CheckerTest extends \PHPUnit\Framework\TestCase ->with('test3', 'entity') ->willReturn($this->foreignEntityType); - $this->acl - ->expects($this->any()) - ->method('checkScope') - ->with($this->foreignEntityType) - ->willReturn(true); - $this->acl ->expects($this->any()) ->method('getScopeForbiddenAttributeList') - ->with($this->entityType) - ->willReturn([]); + ->willReturnMap([ + [$this->entityType, Table::ACTION_READ, Table::LEVEL_NO, []], + [$this->foreignEntityType, Table::ACTION_READ, Table::LEVEL_NO, []], + ]); $this->acl ->expects($this->any()) @@ -277,15 +280,31 @@ class CheckerTest extends \PHPUnit\Framework\TestCase $this->acl ->expects($this->any()) ->method('checkScope') - ->with($this->entityType) + ->with($this->foreignEntityType) ->willReturn(true); + $foreign = $this->createMock(Entity::class); + + $this->entityManager + ->expects($this->any()) + ->method('getEntityById') + ->willReturnMap([ + [$this->foreignEntityType, 'value3', $foreign] + ]); + + $this->acl + ->expects($this->any()) + ->method('checkEntityRead') + ->willReturnMap([ + [$foreign, true] + ]); + $this->checker->check($item, $this->params); $this->assertTrue(true); } - public function testPrermissions2() + public function testPermissions2() { $this->params ->expects($this->any()) @@ -327,10 +346,12 @@ class CheckerTest extends \PHPUnit\Framework\TestCase $this->expectException(Forbidden::class); + /** @noinspection PhpUnhandledExceptionInspection */ $this->checker->check($item, $this->params); } - public function testPrermissions3() + /** @noinspection PhpUnhandledExceptionInspection */ + public function testPermissions3() { $this->params ->expects($this->any()) @@ -381,7 +402,8 @@ class CheckerTest extends \PHPUnit\Framework\TestCase $this->checker->check($item, $this->params); } - public function testPrermissions4() + /** @noinspection PhpUnhandledExceptionInspection */ + public function testPermissions4() { $this->params ->expects($this->any()) @@ -432,6 +454,7 @@ class CheckerTest extends \PHPUnit\Framework\TestCase $this->checker->check($item, $this->params); } + /** @noinspection PhpUnhandledExceptionInspection */ public function testComplexExpressions1() { $this->params