diff --git a/features/filter/filter_validation.feature b/features/filter/filter_validation.feature new file mode 100644 index 00000000000..ab85b45dd5d --- /dev/null +++ b/features/filter/filter_validation.feature @@ -0,0 +1,39 @@ +Feature: Validate filters based upon filter description + + @createSchema + Scenario: Required filter should not throw an error if set + When I am on "/filter_validators?required=foo" + Then the response status code should be 200 + + When I am on "/filter_validators?required=" + Then the response status code should be 200 + + Scenario: Required filter should throw an error if not set + When I am on "/filter_validators" + Then the response status code should be 400 + And the JSON node "detail" should be equal to 'Query parameter "required" is required' + + Scenario: Required filter should not throw an error if set + When I am on "/array_filter_validators?arrayRequired[]=foo&indexedArrayRequired[foo]=foo" + Then the response status code should be 200 + + Scenario: Required filter should throw an error if not set + When I am on "/array_filter_validators" + Then the response status code should be 400 + And the JSON node "detail" should match '/^Query parameter "arrayRequired\[\]" is required\nQuery parameter "indexedArrayRequired\[foo\]" is required$/' + + When I am on "/array_filter_validators?arrayRequired=foo&indexedArrayRequired[foo]=foo" + Then the response status code should be 400 + And the JSON node "detail" should be equal to 'Query parameter "arrayRequired[]" is required' + + When I am on "/array_filter_validators?arrayRequired[foo]=foo" + Then the response status code should be 400 + And the JSON node "detail" should match '/^Query parameter "arrayRequired\[\]" is required\nQuery parameter "indexedArrayRequired\[foo\]" is required$/' + + When I am on "/array_filter_validators?arrayRequired[]=foo" + Then the response status code should be 400 + And the JSON node "detail" should be equal to 'Query parameter "indexedArrayRequired[foo]" is required' + + When I am on "/array_filter_validators?arrayRequired[]=foo&indexedArrayRequired[bar]=bar" + Then the response status code should be 400 + And the JSON node "detail" should be equal to 'Query parameter "indexedArrayRequired[foo]" is required' diff --git a/src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php b/src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php index e426648b13a..b1b349e9bf9 100644 --- a/src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php +++ b/src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php @@ -13,6 +13,7 @@ namespace ApiPlatform\Core\Bridge\Symfony\Bundle\DependencyInjection; +use ApiPlatform\Core\Exception\FilterValidationException; use ApiPlatform\Core\Exception\InvalidArgumentException; use FOS\UserBundle\FOSUserBundle; use GraphQL\GraphQL; @@ -248,6 +249,7 @@ private function addExceptionToStatusSection(ArrayNodeDefinition $rootNode) ->defaultValue([ ExceptionInterface::class => Response::HTTP_BAD_REQUEST, InvalidArgumentException::class => Response::HTTP_BAD_REQUEST, + FilterValidationException::class => Response::HTTP_BAD_REQUEST, ]) ->info('The list of exceptions mapped to their HTTP status code.') ->normalizeKeys(false) diff --git a/src/Bridge/Symfony/Bundle/Resources/config/validator.xml b/src/Bridge/Symfony/Bundle/Resources/config/validator.xml index 8fcace8dc56..d34dcd8cfe6 100644 --- a/src/Bridge/Symfony/Bundle/Resources/config/validator.xml +++ b/src/Bridge/Symfony/Bundle/Resources/config/validator.xml @@ -22,6 +22,13 @@ + + + + + + + diff --git a/src/Exception/FilterValidationException.php b/src/Exception/FilterValidationException.php new file mode 100644 index 00000000000..567f8ec32e9 --- /dev/null +++ b/src/Exception/FilterValidationException.php @@ -0,0 +1,36 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Core\Exception; + +/** + * Filter validation exception. + * + * @author Julien DENIAU + */ +final class FilterValidationException extends \Exception implements ExceptionInterface +{ + private $constraintViolationList; + + public function __construct(array $constraintViolationList, string $message = '', int $code = 0, \Exception $previous = null) + { + $this->constraintViolationList = $constraintViolationList; + + parent::__construct($message ?: $this->__toString(), $code, $previous); + } + + public function __toString(): string + { + return implode("\n", $this->constraintViolationList); + } +} diff --git a/src/Filter/QueryParameterValidateListener.php b/src/Filter/QueryParameterValidateListener.php new file mode 100644 index 00000000000..c0cb227be46 --- /dev/null +++ b/src/Filter/QueryParameterValidateListener.php @@ -0,0 +1,103 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Core\Filter; + +use ApiPlatform\Core\Api\FilterLocatorTrait; +use ApiPlatform\Core\Exception\FilterValidationException; +use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; +use ApiPlatform\Core\Util\RequestAttributesExtractor; +use Psr\Container\ContainerInterface; +use Symfony\Component\HttpKernel\Event\GetResponseEvent; + +/** + * Validates query parameters depending on filter description. + * + * @author Julien Deniau + */ +final class QueryParameterValidateListener +{ + use FilterLocatorTrait; + + private $resourceMetadataFactory; + + public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, ContainerInterface $filterLocator) + { + $this->resourceMetadataFactory = $resourceMetadataFactory; + $this->setFilterLocator($filterLocator); + } + + public function onKernelRequest(GetResponseEvent $event) + { + $request = $event->getRequest(); + if ( + !$request->isMethodSafe(false) + || !($attributes = RequestAttributesExtractor::extractAttributes($request)) + || !isset($attributes['collection_operation_name']) + || 'get' !== ($operationName = $attributes['collection_operation_name']) + ) { + return; + } + + $resourceMetadata = $this->resourceMetadataFactory->create($attributes['resource_class']); + $resourceFilters = $resourceMetadata->getCollectionOperationAttribute($operationName, 'filters', [], true); + + $errorList = []; + foreach ($resourceFilters as $filterId) { + if (!$filter = $this->getFilter($filterId)) { + continue; + } + + foreach ($filter->getDescription($attributes['resource_class']) as $name => $data) { + if (!($data['required'] ?? false)) { // property is not required + continue; + } + + if (!$this->isRequiredFilterValid($name, $request)) { + $errorList[] = sprintf('Query parameter "%s" is required', $name); + } + } + } + + if ($errorList) { + throw new FilterValidationException($errorList); + } + } + + /** + * Test if required filter is valid. It validates array notation too like "required[bar]". + */ + private function isRequiredFilterValid($name, $request): bool + { + $matches = []; + parse_str($name, $matches); + if (!$matches) { + return false; + } + + $rootName = array_keys($matches)[0] ?? ''; + if (!$rootName) { + return false; + } + + if (\is_array($matches[$rootName])) { + $keyName = array_keys($matches[$rootName])[0]; + + $queryParameter = $request->query->get($rootName); + + return \is_array($queryParameter) && isset($queryParameter[$keyName]); + } + + return null !== $request->query->get($rootName); + } +} diff --git a/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php b/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php index e57c7a07604..bc77ba5dcd5 100644 --- a/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php +++ b/tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php @@ -28,6 +28,7 @@ use ApiPlatform\Core\DataProvider\CollectionDataProviderInterface; use ApiPlatform\Core\DataProvider\ItemDataProviderInterface; use ApiPlatform\Core\DataProvider\SubresourceDataProviderInterface; +use ApiPlatform\Core\Exception\FilterValidationException; use ApiPlatform\Core\Exception\InvalidArgumentException; use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface; use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface; @@ -451,7 +452,11 @@ private function getPartialContainerBuilderProphecy($test = false) 'api_platform.description' => 'description', 'api_platform.error_formats' => ['jsonproblem' => ['application/problem+json'], 'jsonld' => ['application/ld+json']], 'api_platform.formats' => ['jsonld' => ['application/ld+json'], 'jsonhal' => ['application/hal+json']], - 'api_platform.exception_to_status' => [ExceptionInterface::class => Response::HTTP_BAD_REQUEST, InvalidArgumentException::class => Response::HTTP_BAD_REQUEST], + 'api_platform.exception_to_status' => [ + ExceptionInterface::class => Response::HTTP_BAD_REQUEST, + InvalidArgumentException::class => Response::HTTP_BAD_REQUEST, + FilterValidationException::class => Response::HTTP_BAD_REQUEST, + ], 'api_platform.title' => 'title', 'api_platform.version' => 'version', 'api_platform.allow_plain_identifiers' => false, @@ -516,6 +521,7 @@ private function getPartialContainerBuilderProphecy($test = false) 'api_platform.listener.view.respond', 'api_platform.listener.view.serialize', 'api_platform.listener.view.validate', + 'api_platform.listener.view.validate_query_parameters', 'api_platform.listener.view.write', 'api_platform.metadata.extractor.xml', 'api_platform.metadata.property.metadata_factory.cached', diff --git a/tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php b/tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php index cde0a614204..eae598820a2 100644 --- a/tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php +++ b/tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php @@ -14,6 +14,7 @@ namespace ApiPlatform\Core\Tests\Bridge\Symfony\Bundle\DependencyInjection; use ApiPlatform\Core\Bridge\Symfony\Bundle\DependencyInjection\Configuration; +use ApiPlatform\Core\Exception\FilterValidationException; use ApiPlatform\Core\Exception\InvalidArgumentException; use PHPUnit\Framework\TestCase; use Symfony\Component\Config\Definition\Builder\TreeBuilder; @@ -67,6 +68,7 @@ public function testDefaultConfig() 'exception_to_status' => [ ExceptionInterface::class => Response::HTTP_BAD_REQUEST, InvalidArgumentException::class => Response::HTTP_BAD_REQUEST, + FilterValidationException::class => Response::HTTP_BAD_REQUEST, ], 'default_operation_path_resolver' => 'api_platform.operation_path_resolver.underscore', 'path_segment_name_generator' => 'api_platform.path_segment_name_generator.underscore', diff --git a/tests/Filter/QueryParameterValidateListenerTest.php b/tests/Filter/QueryParameterValidateListenerTest.php new file mode 100644 index 00000000000..573e8d1ff72 --- /dev/null +++ b/tests/Filter/QueryParameterValidateListenerTest.php @@ -0,0 +1,173 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Core\Tests\Filter; + +use ApiPlatform\Core\Api\FilterInterface; +use ApiPlatform\Core\Exception\FilterValidationException; +use ApiPlatform\Core\Filter\QueryParameterValidateListener; +use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; +use ApiPlatform\Core\Metadata\Resource\ResourceMetadata; +use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Event\GetResponseEvent; + +class QueryParameterValidateListenerTest extends TestCase +{ + private $testedInstance; + private $filterLocatorProphecy; + + /** + * unsafe method should not use filter validations. + */ + public function testOnKernelRequestWithUnsafeMethod() + { + $this->setUpWithFilters(); + + $request = new Request(); + $request->setMethod('POST'); + + $eventProphecy = $this->prophesize(GetResponseEvent::class); + $eventProphecy->getRequest()->willReturn($request)->shouldBeCalled(); + + $this->assertNull( + $this->testedInstance->onKernelRequest($eventProphecy->reveal()) + ); + } + + /** + * If the tested filter is non-existant, then nothing should append. + */ + public function testOnKernelRequestWithWrongFilter() + { + $this->setUpWithFilters(['some_inexistent_filter']); + + $request = new Request([], [], ['_api_resource_class' => Dummy::class, '_api_collection_operation_name' => 'get']); + $request->setMethod('GET'); + + $eventProphecy = $this->prophesize(GetResponseEvent::class); + $eventProphecy->getRequest()->willReturn($request)->shouldBeCalled(); + + $this->filterLocatorProphecy->has('some_inexistent_filter')->shouldBeCalled(); + $this->filterLocatorProphecy->get('some_inexistent_filter')->shouldNotBeCalled(); + + $this->assertNull( + $this->testedInstance->onKernelRequest($eventProphecy->reveal()) + ); + } + + /** + * if the required parameter is not set, throw an FilterValidationException. + */ + public function testOnKernelRequestWithRequiredFilterNotSet() + { + $this->setUpWithFilters(['some_filter']); + + $request = new Request([], [], ['_api_resource_class' => Dummy::class, '_api_collection_operation_name' => 'get']); + $request->setMethod('GET'); + + $eventProphecy = $this->prophesize(GetResponseEvent::class); + $eventProphecy->getRequest()->willReturn($request)->shouldBeCalled(); + + $this->filterLocatorProphecy + ->has('some_filter') + ->shouldBeCalled() + ->willReturn(true) + ; + $filterProphecy = $this->prophesize(FilterInterface::class); + $filterProphecy + ->getDescription(Dummy::class) + ->shouldBeCalled() + ->willReturn([ + 'required' => [ + 'required' => true, + ], + ]) + ; + $this->filterLocatorProphecy + ->get('some_filter') + ->shouldBeCalled() + ->willReturn($filterProphecy->reveal()) + ; + + $this->expectException(FilterValidationException::class); + $this->expectExceptionMessage('Query parameter "required" is required'); + $this->testedInstance->onKernelRequest($eventProphecy->reveal()); + } + + /** + * if the required parameter is set, no exception should be throwned. + */ + public function testOnKernelRequestWithRequiredFilter() + { + $this->setUpWithFilters(['some_filter']); + + $request = new Request( + ['required' => 'foo'], + [], + ['_api_resource_class' => Dummy::class, '_api_collection_operation_name' => 'get'] + ); + $request->setMethod('GET'); + + $eventProphecy = $this->prophesize(GetResponseEvent::class); + $eventProphecy->getRequest()->willReturn($request)->shouldBeCalled(); + + $this->filterLocatorProphecy + ->has('some_filter') + ->shouldBeCalled() + ->willReturn(true) + ; + $filterProphecy = $this->prophesize(FilterInterface::class); + $filterProphecy + ->getDescription(Dummy::class) + ->shouldBeCalled() + ->willReturn([ + 'required' => [ + 'required' => true, + ], + ]) + ; + $this->filterLocatorProphecy + ->get('some_filter') + ->shouldBeCalled() + ->willReturn($filterProphecy->reveal()) + ; + + $this->assertNull( + $this->testedInstance->onKernelRequest($eventProphecy->reveal()) + ); + } + + private function setUpWithFilters(array $filters = []) + { + $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); + $resourceMetadataFactoryProphecy + ->create(Dummy::class) + ->willReturn( + (new ResourceMetadata('dummy')) + ->withAttributes([ + 'filters' => $filters, + ]) + ) + ; + + $this->filterLocatorProphecy = $this->prophesize(ContainerInterface::class); + + $this->testedInstance = new QueryParameterValidateListener( + $resourceMetadataFactoryProphecy->reveal(), + $this->filterLocatorProphecy->reveal() + ); + } +} diff --git a/tests/Fixtures/TestBundle/Entity/ArrayFilterValidator.php b/tests/Fixtures/TestBundle/Entity/ArrayFilterValidator.php new file mode 100644 index 00000000000..f0f705c28e3 --- /dev/null +++ b/tests/Fixtures/TestBundle/Entity/ArrayFilterValidator.php @@ -0,0 +1,66 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity; + +use ApiPlatform\Core\Annotation\ApiProperty; +use ApiPlatform\Core\Annotation\ApiResource; +use ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\ArrayRequiredFilter; +use Doctrine\ORM\Mapping as ORM; + +/** + * Filter Validator entity. + * + * @author Julien Deniau + * + * @ApiResource(attributes={ + * "filters"={ + * ArrayRequiredFilter::class + * } + * }) + * @ORM\Entity + */ +class ArrayFilterValidator +{ + /** + * @var int The id + * + * @ORM\Column(type="integer") + * @ORM\Id + * @ORM\GeneratedValue(strategy="AUTO") + */ + private $id; + + /** + * @var string A name + * + * @ORM\Column + * @ApiProperty(iri="http://schema.org/name") + */ + private $name; + + public function getId() + { + return $this->id; + } + + public function setName($name) + { + $this->name = $name; + } + + public function getName() + { + return $this->name; + } +} diff --git a/tests/Fixtures/TestBundle/Entity/FilterValidator.php b/tests/Fixtures/TestBundle/Entity/FilterValidator.php new file mode 100644 index 00000000000..118050a9b8f --- /dev/null +++ b/tests/Fixtures/TestBundle/Entity/FilterValidator.php @@ -0,0 +1,66 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity; + +use ApiPlatform\Core\Annotation\ApiProperty; +use ApiPlatform\Core\Annotation\ApiResource; +use ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\RequiredFilter; +use Doctrine\ORM\Mapping as ORM; + +/** + * Filter Validator entity. + * + * @author Julien Deniau + * + * @ApiResource(attributes={ + * "filters"={ + * RequiredFilter::class + * } + * }) + * @ORM\Entity + */ +class FilterValidator +{ + /** + * @var int The id + * + * @ORM\Column(type="integer") + * @ORM\Id + * @ORM\GeneratedValue(strategy="AUTO") + */ + private $id; + + /** + * @var string A name + * + * @ORM\Column + * @ApiProperty(iri="http://schema.org/name") + */ + private $name; + + public function getId() + { + return $this->id; + } + + public function setName($name) + { + $this->name = $name; + } + + public function getName() + { + return $this->name; + } +} diff --git a/tests/Fixtures/TestBundle/Filter/ArrayRequiredFilter.php b/tests/Fixtures/TestBundle/Filter/ArrayRequiredFilter.php new file mode 100644 index 00000000000..887d6bdbdaa --- /dev/null +++ b/tests/Fixtures/TestBundle/Filter/ArrayRequiredFilter.php @@ -0,0 +1,42 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter; + +use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\AbstractFilter; +use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface; +use Doctrine\ORM\QueryBuilder; + +final class ArrayRequiredFilter extends AbstractFilter +{ + protected function filterProperty(string $property, $value, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null) + { + } + + // This function is only used to hook in documentation generators (supported by Swagger and Hydra) + public function getDescription(string $resourceClass): array + { + return [ + 'arrayRequired[]' => [ + 'property' => 'arrayRequired', + 'type' => 'string', + 'required' => true, + ], + 'indexedArrayRequired[foo]' => [ + 'property' => 'indexedArrayRequired', + 'type' => 'string', + 'required' => true, + ], + ]; + } +} diff --git a/tests/Fixtures/TestBundle/Filter/RequiredFilter.php b/tests/Fixtures/TestBundle/Filter/RequiredFilter.php new file mode 100644 index 00000000000..83644a711e2 --- /dev/null +++ b/tests/Fixtures/TestBundle/Filter/RequiredFilter.php @@ -0,0 +1,42 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter; + +use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\AbstractFilter; +use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface; +use Doctrine\ORM\QueryBuilder; + +final class RequiredFilter extends AbstractFilter +{ + protected function filterProperty(string $property, $value, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null) + { + } + + // This function is only used to hook in documentation generators (supported by Swagger and Hydra) + public function getDescription(string $resourceClass): array + { + return [ + 'required' => [ + 'property' => 'required', + 'type' => 'string', + 'required' => true, + ], + 'not-required' => [ + 'property' => 'not-required', + 'type' => 'string', + 'required' => false, + ], + ]; + } +} diff --git a/tests/Fixtures/app/config/config_test.yml b/tests/Fixtures/app/config/config_test.yml index 3762fa4c694..0f772a12cea 100644 --- a/tests/Fixtures/app/config/config_test.yml +++ b/tests/Fixtures/app/config/config_test.yml @@ -55,6 +55,7 @@ api_platform: exception_to_status: Symfony\Component\Serializer\Exception\ExceptionInterface: 400 ApiPlatform\Core\Exception\InvalidArgumentException: 400 + ApiPlatform\Core\Exception\FilterValidationException: 400 # Use this syntax with Symfony YAML 3.4+: #ApiPlatform\Core\Exception\InvalidArgumentException: !php/const Symfony\Component\HttpFoundation\Response::HTTP_BAD_REQUEST http_cache: @@ -164,6 +165,14 @@ services: parent: 'api_platform.serializer.property_filter' tags: [ { name: 'api_platform.filter', id: 'my_dummy.property' } ] + ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\RequiredFilter: + arguments: ['@doctrine'] + tags: ['api_platform.filter'] + + ApiPlatform\Core\Tests\Fixtures\TestBundle\Filter\ArrayRequiredFilter: + arguments: ['@doctrine'] + tags: ['api_platform.filter'] + app.config_dummy_resource.action: class: 'ApiPlatform\Core\Tests\Fixtures\TestBundle\Action\ConfigCustom' arguments: ['@api_platform.item_data_provider']