Skip to content

Commit

Permalink
fix(graphql): add cache_key in item normalizer (#5686)
Browse files Browse the repository at this point in the history 
Co-authored-by: Xavier Leune <xleune@ccmbenchmark.com>
  • Loading branch information
@xavierleune
xavierleune and Xavier Leune committed Aug 28, 2023
1 parent c2b3514 commit c14b6f4
Show file tree
Hide file tree
Showing 2 changed files with 128 additions and 1 deletion.
38 changes: 37 additions & 1 deletion src/GraphQl/Serializer/ItemNormalizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use ApiPlatform\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
use ApiPlatform\Metadata\Util\ClassInfoTrait;
use ApiPlatform\Serializer\CacheKeyTrait;
use ApiPlatform\Serializer\ItemNormalizer as BaseItemNormalizer;
use ApiPlatform\Symfony\Security\ResourceAccessCheckerInterface;
use Psr\Log\LoggerInterface;
Expand All @@ -37,12 +38,15 @@
*/
final class ItemNormalizer extends BaseItemNormalizer
{
use CacheKeyTrait;
use ClassInfoTrait;

public const FORMAT = 'graphql';
public const ITEM_RESOURCE_CLASS_KEY = '#itemResourceClass';
public const ITEM_IDENTIFIERS_KEY = '#itemIdentifiers';

private array $safeCacheKeysCache = [];

public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, IriConverterInterface $iriConverter, private readonly IdentifiersExtractorInterface $identifiersExtractor, ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, LoggerInterface $logger = null, ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory = null, ResourceAccessCheckerInterface $resourceAccessChecker = null)
{
parent::__construct($propertyNameCollectionFactory, $propertyMetadataFactory, $iriConverter, $resourceClassResolver, $propertyAccessor, $nameConverter, $classMetadataFactory, $logger ?: new NullLogger(), $resourceMetadataCollectionFactory, $resourceAccessChecker);
Expand Down Expand Up @@ -81,7 +85,11 @@ public function normalize(mixed $object, string $format = null, array $context =
return parent::normalize($object, $format, $context);
}

unset($context['operation_name'], $context['operation']);
if ($this->isCacheKeySafe($context)) {
$context['cache_key'] = $this->getCacheKey($format, $context);
}

unset($context['operation_name'], $context['operation']); // Remove operation and operation_name only when cache key has been created
$data = parent::normalize($object, $format, $context);
if (!\is_array($data)) {
throw new UnexpectedValueException('Expected data to be an array.');
Expand Down Expand Up @@ -140,4 +148,32 @@ protected function setAttributeValue($object, $attribute, $value, $format = null

parent::setAttributeValue($object, $attribute, $value, $format, $context);
}

/**
* Check if any property contains a security grants, which makes the cache key not safe,
* as allowed_properties can differ for 2 instances of the same object.
*/
private function isCacheKeySafe(array $context): bool
{
if (!isset($context['resource_class']) || !$this->resourceClassResolver->isResourceClass($context['resource_class'])) {
return false;
}
$resourceClass = $this->resourceClassResolver->getResourceClass(null, $context['resource_class']);
if (isset($this->safeCacheKeysCache[$resourceClass])) {
return $this->safeCacheKeysCache[$resourceClass];
}
$options = $this->getFactoryOptions($context);
$propertyNames = $this->propertyNameCollectionFactory->create($resourceClass, $options);

$this->safeCacheKeysCache[$resourceClass] = true;
foreach ($propertyNames as $propertyName) {
$propertyMetadata = $this->propertyMetadataFactory->create($resourceClass, $propertyName, $options);
if (null !== $propertyMetadata->getSecurity()) {
$this->safeCacheKeysCache[$resourceClass] = false;
break;
}
}

return $this->safeCacheKeysCache[$resourceClass];
}
}
91 changes: 91 additions & 0 deletions tests/GraphQl/Serializer/ItemNormalizerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
use ApiPlatform\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
use ApiPlatform\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use ApiPlatform\Metadata\Property\PropertyNameCollection;
use ApiPlatform\Symfony\Security\ResourceAccessCheckerInterface;
use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Dummy;
use ApiPlatform\Tests\Fixtures\TestBundle\Entity\SecuredDummy;
use PHPUnit\Framework\TestCase;
use Prophecy\Argument;
use Prophecy\PhpUnit\ProphecyTrait;
Expand Down Expand Up @@ -115,6 +117,95 @@ public function testNormalize(): void
];
$this->assertEquals($expected, $normalizer->normalize($dummy, ItemNormalizer::FORMAT, [
'resources' => [],
'resource_class' => Dummy::class,
]));
}

public function testNormalizeWithUnsafeCacheProperty(): void
{
$securedDummyWithOwnerOnlyPropertyAllowed = new SecuredDummy();
$securedDummyWithOwnerOnlyPropertyAllowed->setTitle('hello');
$securedDummyWithOwnerOnlyPropertyAllowed->setOwnerOnlyProperty('ownerOnly');
$securedDummyWithoutOwnerOnlyPropertyAllowed = clone $securedDummyWithOwnerOnlyPropertyAllowed;
$securedDummyWithoutOwnerOnlyPropertyAllowed->setTitle('hello from secured dummy');

$propertyNameCollection = new PropertyNameCollection(['title', 'ownerOnlyProperty']);
$propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class);
$propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn($propertyNameCollection);

$unsecuredPropertyMetadata = (new ApiProperty())->withReadable(true);
$securedPropertyMetadata = (new ApiProperty())->withReadable(true)->withSecurity('object == null or object.getOwner() == user');
$propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class);
$propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'title', [])->willReturn($unsecuredPropertyMetadata);
$propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'ownerOnlyProperty', [])->willReturn($securedPropertyMetadata);

$iriConverterProphecy = $this->prophesize(IriConverterInterface::class);
$iriConverterProphecy->getIriFromResource($securedDummyWithOwnerOnlyPropertyAllowed, UrlGeneratorInterface::ABS_URL, Argument::any(), Argument::type('array'))->willReturn('/dummies/1');
$iriConverterProphecy->getIriFromResource($securedDummyWithoutOwnerOnlyPropertyAllowed, UrlGeneratorInterface::ABS_URL, Argument::any(), Argument::type('array'))->willReturn('/dummies/2');

$identifiersExtractorProphecy = $this->prophesize(IdentifiersExtractorInterface::class);
$identifiersExtractorProphecy->getIdentifiersFromItem($securedDummyWithOwnerOnlyPropertyAllowed, Argument::any())->willReturn(['id' => 1])->shouldBeCalled();
$identifiersExtractorProphecy->getIdentifiersFromItem($securedDummyWithoutOwnerOnlyPropertyAllowed, Argument::any())->willReturn(['id' => 2])->shouldBeCalled();

$resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class);
$resourceClassResolverProphecy->getResourceClass($securedDummyWithOwnerOnlyPropertyAllowed, null)->willReturn(SecuredDummy::class);
$resourceClassResolverProphecy->getResourceClass($securedDummyWithoutOwnerOnlyPropertyAllowed, null)->willReturn(SecuredDummy::class);
$resourceClassResolverProphecy->getResourceClass(null, SecuredDummy::class)->willReturn(SecuredDummy::class);
$resourceClassResolverProphecy->isResourceClass(SecuredDummy::class)->willReturn(true);

$serializerProphecy = $this->prophesize(SerializerInterface::class);
$serializerProphecy->willImplement(NormalizerInterface::class);
$serializerProphecy->normalize('hello', ItemNormalizer::FORMAT, Argument::type('array'))->willReturn('hello');
$serializerProphecy->normalize('hello from secured dummy', ItemNormalizer::FORMAT, Argument::type('array'))->willReturn('hello from secured dummy');
$serializerProphecy->normalize('ownerOnly', ItemNormalizer::FORMAT, Argument::type('array'))->willReturn('ownerOnly');

$resourceAccessCheckerProphecy = $this->prophesize(ResourceAccessCheckerInterface::class);
$resourceAccessCheckerProphecy->isGranted(
SecuredDummy::class,
'object == null or object.getOwner() == user',
Argument::type('array')
)->will(function (array $args) {
return 'hello' === $args[2]['object']->getTitle(); // Allow access only for securedDummyWithOwnerOnlyPropertyAllowed
});

$normalizer = new ItemNormalizer(
$propertyNameCollectionFactoryProphecy->reveal(),
$propertyMetadataFactoryProphecy->reveal(),
$iriConverterProphecy->reveal(),
$identifiersExtractorProphecy->reveal(),
$resourceClassResolverProphecy->reveal(),
null,
null,
null,
null,
null,
$resourceAccessCheckerProphecy->reveal()
);
$normalizer->setSerializer($serializerProphecy->reveal());

$expected = [
'title' => 'hello',
'ownerOnlyProperty' => 'ownerOnly',
ItemNormalizer::ITEM_RESOURCE_CLASS_KEY => SecuredDummy::class,
ItemNormalizer::ITEM_IDENTIFIERS_KEY => [
'id' => 1,
],
];
$this->assertEquals($expected, $normalizer->normalize($securedDummyWithOwnerOnlyPropertyAllowed, ItemNormalizer::FORMAT, [
'resources' => [],
'resource_class' => SecuredDummy::class,
]));

$expected = [
'title' => 'hello from secured dummy',
ItemNormalizer::ITEM_RESOURCE_CLASS_KEY => SecuredDummy::class,
ItemNormalizer::ITEM_IDENTIFIERS_KEY => [
'id' => 2,
],
];
$this->assertEquals($expected, $normalizer->normalize($securedDummyWithoutOwnerOnlyPropertyAllowed, ItemNormalizer::FORMAT, [
'resources' => [],
'resource_class' => SecuredDummy::class,
]));
}

Expand Down

0 comments on commit c14b6f4

Please sign in to comment.