From d079d0c99e5ca09da4f2f3bf7c4d67bda73f6132 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Thu, 1 Apr 2021 12:03:06 +0100 Subject: [PATCH 1/8] Fix `ApiProperty` `security` unexpectedly having a class string `object` passed to the expression during denormalization when trying to update or create a resource. This meant that only simple role checks etc. could be used. Now the object can be used to grant access directly, passed to voters, etc. If the object isn't available, then null is passed as the `object` instead, making it easier to check for this condition in the expression. Also implements a `security_post_denormalization` attribute on `ApiProperty` to perform another check after denormalisation (for create/update) and be guaranteed that there is an `object` (as well as `previous_object` for updates). --- CHANGELOG.md | 2 + features/authorization/deny.feature | 30 +++++ features/graphql/authorization.feature | 123 ++++++++++++++++++ src/Annotation/ApiProperty.php | 3 + src/Serializer/AbstractItemNormalizer.php | 62 ++++++++- .../TestBundle/Document/SecuredDummy.php | 21 +++ .../TestBundle/Entity/SecuredDummy.php | 21 +++ .../Serializer/AbstractItemNormalizerTest.php | 59 +++++++++ tests/Serializer/ItemNormalizerTest.php | 1 + 9 files changed, 320 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b81907cea9b..f980afc2927 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## 2.7.0 +* Security: **BC** Fix `ApiProperty` `security` attribute expression passed a class string for `object` variable for updates/creates - null is now passed instead if the object is not available (#4184) +* Security: `ApiProperty` now supports a `security_post_denormalize` attribute, which provides access to the `object` variable for the object being updated/created and `previous_object` for the object before it was updated (#4184) * JSON Schema: Add support for generating property schema with numeric constraint restrictions (#4225) * JSON Schema: Add support for generating property schema with Collection restriction (#4182) * JSON Schema: Add support for generating property schema format for Url and Hostname (#4185) diff --git a/features/authorization/deny.feature b/features/authorization/deny.feature index 54d63181f1f..d6c4e540a20 100644 --- a/features/authorization/deny.feature +++ b/features/authorization/deny.feature @@ -78,6 +78,22 @@ Feature: Authorization checking And I send a "GET" request to "/secured_dummies/2" Then the response status code should be 200 + Scenario: A user can see a secured owner-only property on an object they own + When I add "Accept" header equal to "application/ld+json" + And I add "Content-Type" header equal to "application/ld+json" + And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" + And I send a "GET" request to "/secured_dummies/2" + Then the response status code should be 200 + And the JSON node "ownerOnlyProperty" should exist + And the JSON node "ownerOnlyProperty" should not be null + + Scenario: An admin can't see a secured owner-only property on objects they don't own + When I add "Accept" header equal to "application/ld+json" + And I add "Authorization" header equal to "Basic YWRtaW46a2l0dGVu" + And I send a "GET" request to "/secured_dummies" + Then the response status code should be 200 + And the response should not contain "ownerOnlyProperty" + Scenario: A user can't assign to themself an item they doesn't own When I add "Accept" header equal to "application/ld+json" And I add "Content-Type" header equal to "application/ld+json" @@ -151,3 +167,17 @@ Feature: Authorization checking Then the response status code should be 200 And the response should contain "adminOnlyProperty" And the JSON node "hydra:member[2].adminOnlyProperty" should be equal to the string "Is it safe?" + + Scenario: An user can update an owner-only secured property on an object they own + When I add "Accept" header equal to "application/ld+json" + And I add "Content-Type" header equal to "application/ld+json" + And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" + And I send a "PUT" request to "/secured_dummies/3" with body: + """ + { + "ownerOnlyProperty": "updated" + } + """ + Then the response status code should be 200 + And the response should contain "ownerOnlyProperty" + And the JSON node "ownerOnlyProperty" should be equal to the string "updated" diff --git a/features/graphql/authorization.feature b/features/graphql/authorization.feature index 3844387d884..a9c16a7131b 100644 --- a/features/graphql/authorization.feature +++ b/features/graphql/authorization.feature @@ -349,6 +349,69 @@ Feature: Authorization checking And the header "Content-Type" should be equal to "application/json" And the JSON node "data.createSecuredDummy.securedDummy.owner" should be equal to "dunglas" + Scenario: An admin can create a secured resource with an owner-only property if they will be the owner + When I add "Authorization" header equal to "Basic YWRtaW46a2l0dGVu" + And I send the following GraphQL request: + """ + mutation { + createSecuredDummy(input: {owner: "admin", title: "Hi", description: "Desc", adminOnlyProperty: "secret", ownerOnlyProperty: "it works"}) { + securedDummy { + ownerOnlyProperty + } + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "data.createSecuredDummy.securedDummy.ownerOnlyProperty" should be equal to the string "it works" + And I add "Authorization" header equal to "Basic YWRtaW46a2l0dGVu" + And I send the following GraphQL request: + """ + { + securedDummies { + edges { + node { + ownerOnlyProperty + } + } + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the JSON node "data.securedDummies.edges[2].node.ownerOnlyProperty" should be equal to "it works" + + Scenario: An admin can't create a secured resource with an owner-only property if they won't be the owner + When I add "Authorization" header equal to "Basic YWRtaW46a2l0dGVu" + And I send the following GraphQL request: + """ + mutation { + createSecuredDummy(input: {owner: "dunglas", title: "Hi", description: "Desc", adminOnlyProperty: "secret", ownerOnlyProperty: "should not be set"}) { + securedDummy { + ownerOnlyProperty + } + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "data.createSecuredDummy.securedDummy.ownerOnlyProperty" should exist + And the JSON node "data.createSecuredDummy.securedDummy.ownerOnlyProperty" should be null + And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" + And I send the following GraphQL request: + """ + { + securedDummy(id: "/secured_dummies/4") { + ownerOnlyProperty + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the JSON node "data.securedDummy.ownerOnlyProperty" should be equal to the string "" + Scenario: A user cannot retrieve an item they doesn't own When I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" And I send the following GraphQL request: @@ -419,6 +482,66 @@ Feature: Authorization checking And the header "Content-Type" should be equal to "application/json" And the JSON node "data.securedDummy.adminOnlyProperty" should be null + Scenario: A user can see a secured owner-only property on an object they own + When I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" + And I send the following GraphQL request: + """ + { + securedDummy(id: "/secured_dummies/2") { + ownerOnlyProperty + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "data.securedDummy.ownerOnlyProperty" should exist + And the JSON node "data.securedDummy.ownerOnlyProperty" should not be null + + Scenario: A user can update a secured owner-only property on an object they own + When I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" + And I send the following GraphQL request: + """ + mutation { + updateSecuredDummy(input: {id: "/secured_dummies/2", ownerOnlyProperty: "updated"}) { + securedDummy { + ownerOnlyProperty + } + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "data.updateSecuredDummy.securedDummy.ownerOnlyProperty" should be equal to the string "updated" + And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" + And I send the following GraphQL request: + """ + { + securedDummy(id: "/secured_dummies/2") { + ownerOnlyProperty + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the JSON node "data.securedDummy.ownerOnlyProperty" should be equal to the string "updated" + + Scenario: An admin can't see a secured owner-only property on an object they don't own + When I add "Authorization" header equal to "Basic YWRtaW46a2l0dGVu" + And I send the following GraphQL request: + """ + { + securedDummy(id: "/secured_dummies/2") { + ownerOnlyProperty + } + } + """ + Then the response status code should be 200 + And the response should be in JSON + And the header "Content-Type" should be equal to "application/json" + And the JSON node "data.securedDummy.ownerOnlyProperty" should be null + Scenario: A user can't assign to themself an item they doesn't own When I add "Authorization" header equal to "Basic YWRtaW46a2l0dGVu" And I send the following GraphQL request: diff --git a/src/Annotation/ApiProperty.php b/src/Annotation/ApiProperty.php index 3140eae218f..e56cd82a274 100644 --- a/src/Annotation/ApiProperty.php +++ b/src/Annotation/ApiProperty.php @@ -30,6 +30,7 @@ * @Attribute("jsonldContext", type="array"), * @Attribute("push", type="bool"), * @Attribute("security", type="string"), + * @Attribute("securityPostDenormalize", type="string"), * @Attribute("swaggerContext", type="array") * ) */ @@ -111,6 +112,7 @@ final class ApiProperty * @param array $openapiContext * @param bool $push * @param string $security + * @param string $securityPostDenormalize * @param array $swaggerContext * * @throws InvalidArgumentException @@ -136,6 +138,7 @@ public function __construct( ?array $openapiContext = null, ?bool $push = null, ?string $security = null, + ?string $securityPostDenormalize = null, ?array $swaggerContext = null ) { if (!\is_array($description)) { // @phpstan-ignore-line Doctrine annotations support diff --git a/src/Serializer/AbstractItemNormalizer.php b/src/Serializer/AbstractItemNormalizer.php index 8ab7b0e116a..779655261f2 100644 --- a/src/Serializer/AbstractItemNormalizer.php +++ b/src/Serializer/AbstractItemNormalizer.php @@ -254,7 +254,34 @@ public function denormalize($data, $class, $format = null, array $context = []) return $item; } - return parent::denormalize($data, $resourceClass, $format, $context); + $objectToPopulate = $context[self::OBJECT_TO_POPULATE] ?? null; + $isUpdate = null !== $objectToPopulate; + + // Filter out input attributes that aren't allowed for updates + if ($isUpdate) { + $data = array_filter($data, function (string $attribute) use ($context, $objectToPopulate) { + return $this->canAccessAttribute($objectToPopulate, $attribute, $context); + }, \ARRAY_FILTER_USE_KEY); + } + + $previousObject = $isUpdate ? clone $objectToPopulate : null; + $object = parent::denormalize($data, $resourceClass, $format, $context); + + // Revert attributes that aren't allowed to be changed after a post-denormalize check + foreach (array_keys($data) as $attribute) { + if (!$this->canAccessAttributePostDenormalize($object, $previousObject, $attribute, $context)) { + if ($isUpdate) { + $object[$attribute] = $previousObject[$attribute]; + } else { + $reflection = (new \ReflectionClass($object)); + $property = $reflection->getProperty($attribute); + $property->setAccessible(true); + $property->setValue($object, $reflection->getDefaultProperties()[$attribute] ?? null); + } + } + } + + return $object; } /** @@ -390,12 +417,43 @@ protected function isAllowedAttribute($classOrObject, $attribute, $format = null return false; } + return $this->canAccessAttribute(\is_object($classOrObject) ? $classOrObject : null, $attribute, $context); + } + + /** + * Check if access to the attribute is granted. + * + * @param object $object + */ + protected function canAccessAttribute($object, string $attribute, array $context = []): bool + { $options = $this->getFactoryOptions($context); $propertyMetadata = $this->propertyMetadataFactory->create($context['resource_class'], $attribute, $options); $security = $propertyMetadata->getAttribute('security'); if ($this->resourceAccessChecker && $security) { return $this->resourceAccessChecker->isGranted($context['resource_class'], $security, [ - 'object' => $classOrObject, + 'object' => $object, + ]); + } + + return true; + } + + /** + * Check if access to the attribute is granted. + * + * @param object $object + * @param object|null $previousObject + */ + protected function canAccessAttributePostDenormalize($object, $previousObject, string $attribute, array $context = []): bool + { + $options = $this->getFactoryOptions($context); + $propertyMetadata = $this->propertyMetadataFactory->create($context['resource_class'], $attribute, $options); + $security = $propertyMetadata->getAttribute('security_post_denormalize'); + if ($this->resourceAccessChecker && $security) { + return $this->resourceAccessChecker->isGranted($context['resource_class'], $security, [ + 'object' => $object, + 'previous_object' => $previousObject, ]); } diff --git a/tests/Fixtures/TestBundle/Document/SecuredDummy.php b/tests/Fixtures/TestBundle/Document/SecuredDummy.php index 277b1220066..1356529c0ec 100644 --- a/tests/Fixtures/TestBundle/Document/SecuredDummy.php +++ b/tests/Fixtures/TestBundle/Document/SecuredDummy.php @@ -83,6 +83,17 @@ class SecuredDummy */ private $adminOnlyProperty = ''; + /** + * @var string Secret property, only readable/writable by owners + * + * @ODM\Field + * @ApiProperty( + * security="object == null or object.getOwner() == user", + * securityPostDenormalize="object.getOwner() == user", + * ) + */ + private $ownerOnlyProperty = ''; + /** * @var string The owner * @@ -187,6 +198,16 @@ public function setAdminOnlyProperty(?string $adminOnlyProperty) $this->adminOnlyProperty = $adminOnlyProperty; } + public function getOwnerOnlyProperty(): ?string + { + return $this->ownerOnlyProperty; + } + + public function setOwnerOnlyProperty(?string $ownerOnlyProperty) + { + $this->ownerOnlyProperty = $ownerOnlyProperty; + } + public function getOwner(): string { return $this->owner; diff --git a/tests/Fixtures/TestBundle/Entity/SecuredDummy.php b/tests/Fixtures/TestBundle/Entity/SecuredDummy.php index 3a35ccf6ada..3af569999dc 100644 --- a/tests/Fixtures/TestBundle/Entity/SecuredDummy.php +++ b/tests/Fixtures/TestBundle/Entity/SecuredDummy.php @@ -84,6 +84,17 @@ class SecuredDummy */ private $adminOnlyProperty = ''; + /** + * @var string Secret property, only readable/writable by owners + * + * @ORM\Column + * @ApiProperty( + * security="object == null or object.getOwner() == user", + * securityPostDenormalize="object.getOwner() == user", + * ) + */ + private $ownerOnlyProperty = ''; + /** * @var string The owner * @@ -198,6 +209,16 @@ public function setAdminOnlyProperty(?string $adminOnlyProperty) $this->adminOnlyProperty = $adminOnlyProperty; } + public function getOwnerOnlyProperty(): ?string + { + return $this->ownerOnlyProperty; + } + + public function setOwnerOnlyProperty(?string $ownerOnlyProperty) + { + $this->ownerOnlyProperty = $ownerOnlyProperty; + } + public function getOwner(): string { return $this->owner; diff --git a/tests/Serializer/AbstractItemNormalizerTest.php b/tests/Serializer/AbstractItemNormalizerTest.php index 09592c80316..fa94d2d52a3 100644 --- a/tests/Serializer/AbstractItemNormalizerTest.php +++ b/tests/Serializer/AbstractItemNormalizerTest.php @@ -266,6 +266,63 @@ public function testNormalizeWithSecuredProperty() ])); } + public function testDenormalizeWithSecuredProperty() + { + $data = [ + 'title' => 'foo', + 'adminOnlyProperty' => 'secret', + ]; + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title'])); + $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title', 'adminOnlyProperty'])); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'title', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'adminOnlyProperty', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', true, null, null, null, null, null, null, null, ['security' => 'is_granted(\'ROLE_ADMIN\')'])); + + $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); + + $propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class); + + $resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class); + $resourceClassResolverProphecy->getResourceClass(null, SecuredDummy::class)->willReturn(SecuredDummy::class); + + $serializerProphecy = $this->prophesize(SerializerInterface::class); + $serializerProphecy->willImplement(NormalizerInterface::class); + + $resourceAccessChecker = $this->prophesize(ResourceAccessCheckerInterface::class); + $resourceAccessChecker->isGranted( + SecuredDummy::class, + 'is_granted(\'ROLE_ADMIN\')', + ['object' => null] + )->willReturn(false); + + $normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [ + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $iriConverterProphecy->reveal(), + $resourceClassResolverProphecy->reveal(), + $propertyAccessorProphecy->reveal(), + null, + null, + null, + false, + [], + [], + null, + $resourceAccessChecker->reveal(), + ]); + $normalizer->setSerializer($serializerProphecy->reveal()); + + $actual = $normalizer->denormalize($data, SecuredDummy::class); + + $this->assertInstanceOf(SecuredDummy::class, $actual); + + $propertyAccessorProphecy->setValue($actual, 'title', 'foo')->shouldHaveBeenCalled(); + $propertyAccessorProphecy->setValue($actual, 'adminOnlyProperty', 'secret')->shouldNotHaveBeenCalled(); + } + public function testNormalizeReadableLinks() { $relatedDummy = new RelatedDummy(); @@ -787,6 +844,8 @@ public function testDenormalizeBadKeyType() $propertyNameCollectionFactoryProphecy->create(Dummy::class, [])->willReturn(new PropertyNameCollection(['relatedDummies'])); $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'name', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummy', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_OBJECT, false, RelatedDummy::class), '', false, true, false, false)); $propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummies', [])->willReturn( new PropertyMetadata( new Type( diff --git a/tests/Serializer/ItemNormalizerTest.php b/tests/Serializer/ItemNormalizerTest.php index 7a097aae786..9a63baea547 100644 --- a/tests/Serializer/ItemNormalizerTest.php +++ b/tests/Serializer/ItemNormalizerTest.php @@ -164,6 +164,7 @@ public function testDenormalizeWithIri() $propertyMetadata = new PropertyMetadata(null, null, true, true); $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'id', [])->willReturn($propertyMetadata)->shouldBeCalled(); $propertyMetadataFactoryProphecy->create(Dummy::class, 'name', [])->willReturn($propertyMetadata)->shouldBeCalled(); $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); From 6f6ee14a3ac5b85446a386148d5a8b7b64d2d187 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Tue, 6 Apr 2021 13:14:19 +0100 Subject: [PATCH 2/8] Update change log with PR #. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f980afc2927..ef9fddde27b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## 2.7.0 -* Security: **BC** Fix `ApiProperty` `security` attribute expression passed a class string for `object` variable for updates/creates - null is now passed instead if the object is not available (#4184) +* Security: **BC** Fix `ApiProperty` `security` attribute expression being passed a class string for the `object` variable on updates/creates - null is now passed instead if the object is not available (#4184) * Security: `ApiProperty` now supports a `security_post_denormalize` attribute, which provides access to the `object` variable for the object being updated/created and `previous_object` for the object before it was updated (#4184) * JSON Schema: Add support for generating property schema with numeric constraint restrictions (#4225) * JSON Schema: Add support for generating property schema with Collection restriction (#4182) From a2092479c8785d9821f91a216a8f676e61dbef8b Mon Sep 17 00:00:00 2001 From: David Bennett Date: Tue, 6 Apr 2021 13:14:50 +0100 Subject: [PATCH 3/8] Fix `ApiPropertyTest`. --- tests/Annotation/ApiPropertyTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/Annotation/ApiPropertyTest.php b/tests/Annotation/ApiPropertyTest.php index f4c1ec790c0..133b206ee2b 100644 --- a/tests/Annotation/ApiPropertyTest.php +++ b/tests/Annotation/ApiPropertyTest.php @@ -53,6 +53,7 @@ public function testConstruct() 'fetchEager' => false, 'jsonldContext' => ['foo' => 'bar'], 'security' => 'is_granted(\'ROLE_ADMIN\')', + 'security_post_denormalize' => 'is_granted(\'VIEW\', object)', 'swaggerContext' => ['foo' => 'baz'], 'openapiContext' => ['foo' => 'baz'], 'push' => true, @@ -64,6 +65,7 @@ public function testConstruct() 'fetch_eager' => false, 'jsonld_context' => ['foo' => 'bar'], 'security' => 'is_granted(\'ROLE_ADMIN\')', + 'security_post_denormalize' => 'is_granted(\'VIEW\', object)', 'swagger_context' => ['foo' => 'baz'], 'openapi_context' => ['foo' => 'baz'], 'push' => true, @@ -83,6 +85,7 @@ public function testConstructAttribute() fetchEager: false, jsonldContext: ['foo' => 'bar'], security: 'is_granted(\'ROLE_ADMIN\')', + securityPostDenormalize: 'is_granted(\'VIEW\', object)', swaggerContext: ['foo' => 'baz'], openapiContext: ['foo' => 'baz'], push: true, @@ -97,6 +100,7 @@ public function testConstructAttribute() 'fetch_eager' => false, 'jsonld_context' => ['foo' => 'bar'], 'security' => 'is_granted(\'ROLE_ADMIN\')', + 'security_post_denormalize' => 'is_granted(\'VIEW\', object)', 'swagger_context' => ['foo' => 'baz'], 'openapi_context' => ['foo' => 'baz'], 'push' => true, From 4de8bb96e4d4b3824172800972792c9df35647dd Mon Sep 17 00:00:00 2001 From: David Bennett Date: Tue, 6 Apr 2021 13:16:39 +0100 Subject: [PATCH 4/8] Update `AbstractItemNormalizer` changes to use property accessor + metadata instead of reflection directly. --- src/Serializer/AbstractItemNormalizer.php | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/Serializer/AbstractItemNormalizer.php b/src/Serializer/AbstractItemNormalizer.php index 779655261f2..db3fca63a31 100644 --- a/src/Serializer/AbstractItemNormalizer.php +++ b/src/Serializer/AbstractItemNormalizer.php @@ -254,29 +254,24 @@ public function denormalize($data, $class, $format = null, array $context = []) return $item; } - $objectToPopulate = $context[self::OBJECT_TO_POPULATE] ?? null; - $isUpdate = null !== $objectToPopulate; - // Filter out input attributes that aren't allowed for updates - if ($isUpdate) { + if (null !== $objectToPopulate) { $data = array_filter($data, function (string $attribute) use ($context, $objectToPopulate) { return $this->canAccessAttribute($objectToPopulate, $attribute, $context); }, \ARRAY_FILTER_USE_KEY); } - $previousObject = $isUpdate ? clone $objectToPopulate : null; + $previousObject = null !== $objectToPopulate ? clone $objectToPopulate : null; $object = parent::denormalize($data, $resourceClass, $format, $context); // Revert attributes that aren't allowed to be changed after a post-denormalize check foreach (array_keys($data) as $attribute) { if (!$this->canAccessAttributePostDenormalize($object, $previousObject, $attribute, $context)) { - if ($isUpdate) { - $object[$attribute] = $previousObject[$attribute]; + if (null !== $previousObject) { + $this->setValue($object, $attribute, $this->propertyAccessor->getValue($previousObject, $attribute)); } else { - $reflection = (new \ReflectionClass($object)); - $property = $reflection->getProperty($attribute); - $property->setAccessible(true); - $property->setValue($object, $reflection->getDefaultProperties()[$attribute] ?? null); + $propertyMetaData = $this->propertyMetadataFactory->create($resourceClass, $attribute, $this->getFactoryOptions($context)); + $this->setValue($object, $attribute, $propertyMetaData->getDefault()); } } } From 7de1dffb2cb1381edcfb26544788207fc50ee418 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Tue, 6 Apr 2021 13:17:23 +0100 Subject: [PATCH 5/8] Add more `AbstractItemNormalizerTest` tests for coverage. --- .../Serializer/AbstractItemNormalizerTest.php | 251 +++++++++++++++++- 1 file changed, 249 insertions(+), 2 deletions(-) diff --git a/tests/Serializer/AbstractItemNormalizerTest.php b/tests/Serializer/AbstractItemNormalizerTest.php index fa94d2d52a3..9d259dd4808 100644 --- a/tests/Serializer/AbstractItemNormalizerTest.php +++ b/tests/Serializer/AbstractItemNormalizerTest.php @@ -274,12 +274,11 @@ public function testDenormalizeWithSecuredProperty() ]; $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); - $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title'])); $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title', 'adminOnlyProperty'])); $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'title', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)); - $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'adminOnlyProperty', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', true, null, null, null, null, null, null, null, ['security' => 'is_granted(\'ROLE_ADMIN\')'])); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'adminOnlyProperty', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', true, true, null, null, null, null, null, null, ['security' => 'is_granted(\'ROLE_ADMIN\')'])); $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); @@ -323,6 +322,254 @@ public function testDenormalizeWithSecuredProperty() $propertyAccessorProphecy->setValue($actual, 'adminOnlyProperty', 'secret')->shouldNotHaveBeenCalled(); } + public function testDenormalizeCreateWithDeniedPostDenormalizeSecuredProperty() + { + $data = [ + 'title' => 'foo', + 'ownerOnlyProperty' => 'should not be set', + ]; + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title', 'ownerOnlyProperty'])); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'title', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'ownerOnlyProperty', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', true, true, null, null, null, null, null, null, ['security_post_denormalize' => 'false'], null, null, '')); + + $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); + + $propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class); + + $resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class); + $resourceClassResolverProphecy->getResourceClass(null, SecuredDummy::class)->willReturn(SecuredDummy::class); + + $serializerProphecy = $this->prophesize(SerializerInterface::class); + $serializerProphecy->willImplement(NormalizerInterface::class); + + $resourceAccessChecker = $this->prophesize(ResourceAccessCheckerInterface::class); + $resourceAccessChecker->isGranted( + SecuredDummy::class, + 'false', + Argument::any() + )->willReturn(false); + + $normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [ + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $iriConverterProphecy->reveal(), + $resourceClassResolverProphecy->reveal(), + $propertyAccessorProphecy->reveal(), + null, + null, + null, + false, + [], + [], + null, + $resourceAccessChecker->reveal(), + ]); + $normalizer->setSerializer($serializerProphecy->reveal()); + + $actual = $normalizer->denormalize($data, SecuredDummy::class); + + $this->assertInstanceOf(SecuredDummy::class, $actual); + + $propertyAccessorProphecy->setValue($actual, 'title', 'foo')->shouldHaveBeenCalled(); + $propertyAccessorProphecy->setValue($actual, 'ownerOnlyProperty', 'should not be set')->shouldHaveBeenCalled(); + $propertyAccessorProphecy->setValue($actual, 'ownerOnlyProperty', '')->shouldHaveBeenCalled(); + } + + public function testDenormalizeUpdateWithSecuredProperty() + { + $dummy = new SecuredDummy(); + + $data = [ + 'title' => 'foo', + 'ownerOnlyProperty' => 'secret', + ]; + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title', 'ownerOnlyProperty'])); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'title', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'ownerOnlyProperty', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', true, true, null, null, null, null, null, null, ['security' => 'true'])); + + $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); + + $propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class); + + $resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class); + $resourceClassResolverProphecy->getResourceClass($dummy, SecuredDummy::class)->willReturn(SecuredDummy::class); + + $serializerProphecy = $this->prophesize(SerializerInterface::class); + $serializerProphecy->willImplement(NormalizerInterface::class); + + $resourceAccessChecker = $this->prophesize(ResourceAccessCheckerInterface::class); + $resourceAccessChecker->isGranted( + SecuredDummy::class, + 'true', + ['object' => null] + )->willReturn(true); + $resourceAccessChecker->isGranted( + SecuredDummy::class, + 'true', + ['object' => $dummy] + )->willReturn(true); + + $normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [ + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $iriConverterProphecy->reveal(), + $resourceClassResolverProphecy->reveal(), + $propertyAccessorProphecy->reveal(), + null, + null, + null, + false, + [], + [], + null, + $resourceAccessChecker->reveal(), + ]); + $normalizer->setSerializer($serializerProphecy->reveal()); + + $context = [AbstractItemNormalizer::OBJECT_TO_POPULATE => $dummy]; + $actual = $normalizer->denormalize($data, SecuredDummy::class, null, $context); + + $this->assertInstanceOf(SecuredDummy::class, $actual); + + $propertyAccessorProphecy->setValue($actual, 'title', 'foo')->shouldHaveBeenCalled(); + $propertyAccessorProphecy->setValue($actual, 'ownerOnlyProperty', 'secret')->shouldHaveBeenCalled(); + } + + public function testDenormalizeUpdateWithDeniedSecuredProperty() + { + $dummy = new SecuredDummy(); + $dummy->setOwnerOnlyProperty('secret'); + + $data = [ + 'title' => 'foo', + 'ownerOnlyProperty' => 'should not be set', + ]; + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title', 'ownerOnlyProperty'])); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'title', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'ownerOnlyProperty', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', true, true, null, null, null, null, null, null, ['security' => 'false'])); + + $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); + + $propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class); + + $resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class); + $resourceClassResolverProphecy->getResourceClass($dummy, SecuredDummy::class)->willReturn(SecuredDummy::class); + + $serializerProphecy = $this->prophesize(SerializerInterface::class); + $serializerProphecy->willImplement(NormalizerInterface::class); + + $resourceAccessChecker = $this->prophesize(ResourceAccessCheckerInterface::class); + $resourceAccessChecker->isGranted( + SecuredDummy::class, + 'false', + ['object' => null] + )->willReturn(false); + $resourceAccessChecker->isGranted( + SecuredDummy::class, + 'false', + ['object' => $dummy] + )->willReturn(false); + + $normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [ + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $iriConverterProphecy->reveal(), + $resourceClassResolverProphecy->reveal(), + $propertyAccessorProphecy->reveal(), + null, + null, + null, + false, + [], + [], + null, + $resourceAccessChecker->reveal(), + ]); + $normalizer->setSerializer($serializerProphecy->reveal()); + + $context = [AbstractItemNormalizer::OBJECT_TO_POPULATE => $dummy]; + $actual = $normalizer->denormalize($data, SecuredDummy::class, null, $context); + + $this->assertInstanceOf(SecuredDummy::class, $actual); + + $propertyAccessorProphecy->setValue($actual, 'title', 'foo')->shouldHaveBeenCalled(); + $propertyAccessorProphecy->setValue($actual, 'ownerOnlyProperty', 'should not be set')->shouldNotHaveBeenCalled(); + } + + public function testDenormalizeUpdateWithDeniedPostDenormalizeSecuredProperty() + { + $dummy = new SecuredDummy(); + $dummy->setOwnerOnlyProperty('secret'); + + $data = [ + 'title' => 'foo', + 'ownerOnlyProperty' => 'should not be set', + ]; + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(SecuredDummy::class, [])->willReturn(new PropertyNameCollection(['title', 'ownerOnlyProperty'])); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'title', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', false, true)); + $propertyMetadataFactoryProphecy->create(SecuredDummy::class, 'ownerOnlyProperty', [])->willReturn(new PropertyMetadata(new Type(Type::BUILTIN_TYPE_STRING), '', true, true, null, null, null, null, null, null, ['security_post_denormalize' => 'false'])); + + $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); + + $propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class); + $propertyAccessorProphecy->getValue($dummy, 'ownerOnlyProperty')->willReturn('secret'); + + $resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class); + $resourceClassResolverProphecy->getResourceClass($dummy, SecuredDummy::class)->willReturn(SecuredDummy::class); + + $serializerProphecy = $this->prophesize(SerializerInterface::class); + $serializerProphecy->willImplement(NormalizerInterface::class); + + $resourceAccessChecker = $this->prophesize(ResourceAccessCheckerInterface::class); + $resourceAccessChecker->isGranted( + SecuredDummy::class, + 'false', + ['object' => $dummy, 'previous_object' => $dummy] + )->willReturn(false); + + $normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [ + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $iriConverterProphecy->reveal(), + $resourceClassResolverProphecy->reveal(), + $propertyAccessorProphecy->reveal(), + null, + null, + null, + false, + [], + [], + null, + $resourceAccessChecker->reveal(), + ]); + $normalizer->setSerializer($serializerProphecy->reveal()); + + $context = [AbstractItemNormalizer::OBJECT_TO_POPULATE => $dummy]; + $actual = $normalizer->denormalize($data, SecuredDummy::class, null, $context); + + $this->assertInstanceOf(SecuredDummy::class, $actual); + + $propertyAccessorProphecy->setValue($actual, 'title', 'foo')->shouldHaveBeenCalled(); + $propertyAccessorProphecy->setValue($actual, 'ownerOnlyProperty', 'should not be set')->shouldHaveBeenCalled(); + $propertyAccessorProphecy->setValue($actual, 'ownerOnlyProperty', 'secret')->shouldHaveBeenCalled(); + } + public function testNormalizeReadableLinks() { $relatedDummy = new RelatedDummy(); From 9fc673ae823c5551e0ce9fe0f5faf2cd32faea96 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Tue, 6 Apr 2021 13:47:57 +0100 Subject: [PATCH 6/8] Fix `ApiPropertyTest`. --- tests/Annotation/ApiPropertyTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Annotation/ApiPropertyTest.php b/tests/Annotation/ApiPropertyTest.php index 133b206ee2b..9946e49ed73 100644 --- a/tests/Annotation/ApiPropertyTest.php +++ b/tests/Annotation/ApiPropertyTest.php @@ -53,7 +53,7 @@ public function testConstruct() 'fetchEager' => false, 'jsonldContext' => ['foo' => 'bar'], 'security' => 'is_granted(\'ROLE_ADMIN\')', - 'security_post_denormalize' => 'is_granted(\'VIEW\', object)', + 'securityPostDenormalize' => 'is_granted(\'VIEW\', object)', 'swaggerContext' => ['foo' => 'baz'], 'openapiContext' => ['foo' => 'baz'], 'push' => true, From 699f8e0729f436a72b1ecc1685338b7da42fd3a5 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Fri, 9 Apr 2021 17:11:30 +0100 Subject: [PATCH 7/8] Move new `ApiProperty` attribute parameter to last, to ensure no BC. --- src/Annotation/ApiProperty.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Annotation/ApiProperty.php b/src/Annotation/ApiProperty.php index e56cd82a274..a61629624f4 100644 --- a/src/Annotation/ApiProperty.php +++ b/src/Annotation/ApiProperty.php @@ -112,8 +112,8 @@ final class ApiProperty * @param array $openapiContext * @param bool $push * @param string $security - * @param string $securityPostDenormalize * @param array $swaggerContext + * @param string $securityPostDenormalize * * @throws InvalidArgumentException */ @@ -138,8 +138,8 @@ public function __construct( ?array $openapiContext = null, ?bool $push = null, ?string $security = null, - ?string $securityPostDenormalize = null, - ?array $swaggerContext = null + ?array $swaggerContext = null, + ?string $securityPostDenormalize = null ) { if (!\is_array($description)) { // @phpstan-ignore-line Doctrine annotations support [$publicProperties, $configurableAttributes] = self::getConfigMetadata(); From c178a854aa6c1a8da6a8f25b2d3bd52c47abc7e4 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Fri, 9 Apr 2021 17:14:35 +0100 Subject: [PATCH 8/8] Remove the additional pre-denormalize `ApiProperty` security check to reduce code complexity. `security_post_normalize` should be used if an object is required. --- src/Serializer/AbstractItemNormalizer.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Serializer/AbstractItemNormalizer.php b/src/Serializer/AbstractItemNormalizer.php index db3fca63a31..be441f9ca53 100644 --- a/src/Serializer/AbstractItemNormalizer.php +++ b/src/Serializer/AbstractItemNormalizer.php @@ -254,13 +254,6 @@ public function denormalize($data, $class, $format = null, array $context = []) return $item; } - // Filter out input attributes that aren't allowed for updates - if (null !== $objectToPopulate) { - $data = array_filter($data, function (string $attribute) use ($context, $objectToPopulate) { - return $this->canAccessAttribute($objectToPopulate, $attribute, $context); - }, \ARRAY_FILTER_USE_KEY); - } - $previousObject = null !== $objectToPopulate ? clone $objectToPopulate : null; $object = parent::denormalize($data, $resourceClass, $format, $context);