Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions features/filter/filter_validation.feature
Original file line number Diff line number Diff line change
@@ -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'
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions src/Bridge/Symfony/Bundle/Resources/config/validator.xml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@

<tag name="kernel.event_listener" event="kernel.view" method="onKernelView" priority="64" />
</service>

<service id="api_platform.listener.view.validate_query_parameters" class="ApiPlatform\Core\Filter\QueryParameterValidateListener" public="false">
<argument type="service" id="api_platform.metadata.resource.metadata_factory" />
<argument type="service" id="api_platform.filter_locator" />

<tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" priority="16" />
</service>
</services>

</container>
36 changes: 36 additions & 0 deletions src/Exception/FilterValidationException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* 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 <julien.deniau@gmail.com>
*/
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);
}
}
103 changes: 103 additions & 0 deletions src/Filter/QueryParameterValidateListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move it in the ApiPlatform\EventListener namespace.
What about ValidateQueryParametersListener for the name?

@soyuka soyuka May 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay if it's refactored later as said in #1692 (comment) I guess? Anyway if it's an eventlistener it feels weird to have it in filters.


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 <julien.deniau@gmail.com>
*/
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);

@teohhanhui teohhanhui May 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've mentioned this a long time ago, but we really should stop using bad built-in PHP functions like parse_str. We should seriously think about using a proper URI parsing library instead: https://uri.thephpleague.com/5.0/

https://uri.thephpleague.com/5.0/components/parsers/#queryparserextract

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary for this use case? Maybe can we do that at some point in another PR and replace all calls to parse_str in 1 time.

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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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',
Expand Down
Loading