feat: add `security_post_denormalize` in `ApiProperty` #4184

DavidBennettUK · 2021-04-01T16:07:01Z

Q	A
Branch?	main
Bug fix?	yes
New feature?	yes
Deprecations?	no
Tickets	N/A
License	MIT
Doc PR	N/A

While implementing #4143 I noticed that it's not possible to use object in the security expression of ApiProperty - not only for GraphQL, but for all.

It was already possible to use object for read operations - but doing so would cause unexpected exceptions to be thrown on updates/creates, as the object is actually a class string. For example, the security expression object.getOwner() == user would throw an exception when performing an update as object would be a class string - which the developer wouldn't expect. This exception would happen even if you weren't updating that specific field, as the Symfony AbstractObjectNormalizer::denormalize() calls getAllowedAttributes - which runs security on every single field, due to canAccessAttribute being overridden in AbstractItemNormalizer.

There didn't seem to be a way to work around this in the expressions, as there isn't an obvious way to check if a variable is an object or string in the expression language.

This PR updates this so that object will always be the object or null - which can easily be checked for in the expression.

Also added is a security_post_denormalize which works similarly to those on ApiResource. This lets you be guaranteed that object isn't null and will also give you access to previous_object for more advanced checks.

This should hopefully not be a BC break unless developer workarounds were put in place possibly. For safety, and because it depends on #4143, it is targeting main.

alanpoulain · 2021-04-01T16:19:51Z

Related: #3966
@Toflar, since you are the one who has created the PR, could you see if it's OK for you?

@DavidBennettUK I think @Toflar is able to distinguish between an object and a class string by using a custom expression language function (user_has_role_for_organisation).

alanpoulain · 2021-04-01T16:24:24Z

src/Serializer/AbstractItemNormalizer.php

+
+        // Filter out input attributes that aren't allowed for updates
+        if ($isUpdate) {
+            $data = array_filter($data, function (string $attribute) use ($context, $objectToPopulate) {


Why filtering the input? Was it possible to modify a property of a resource without the right to do so before?

You're right that there was already a security pass - AbstractObjectNormalizer::denormalize() calls AbstractItemNormalizer::getAllowedAttributes() but only with the class string, so object can't be used. This addition does another pass over, calling the security expression with a populated object variable. Without this, security will always be called with a null object for updates, which may be unexpected. We could take this out and force use of security_post_denormalize if the developer wants an object, but I think an object would be expected for updates as it already exists.

I'd be in favor of reducing the complexity in our code, enforcing the use of security_post_denormalize

src/Serializer/AbstractItemNormalizer.php

Toflar · 2021-04-01T17:13:29Z

In my case, I cannot check on the roles of the user directly because the same user has different roles for different organisations. So what I did is this:

    /**
     * @ApiProperty(security="user_has_role_for_organisation('ROLE_OWNER', object)")
     */
    private BillingInfo $billingInfo;

And then a simple

class AccessExpressionProvider implements ExpressionFunctionProviderInterface
{
    public function getFunctions(): array
    {
        return [
            new ExpressionFunction('user_has_role_for_organisation', function (string $role, Organisation $organisation) {
                return sprintf('$user->hasRoleForOrganisation(%s, %s)', $role, $organisation);
            }, function (array $variables, string $role, Organisation $organisation) {
                return $variables['user']->hasRoleForOrganisation($role, $organisation);
            }),
        ];
    }
}

This should still work as far as I can see.

alanpoulain · 2021-04-01T17:18:20Z

That's weird. If I understand what @DavidBennettUK is saying, your function should fail when doing an update / create, because $organisation is then a string and not an Organisation object.

Toflar · 2021-04-01T17:24:10Z

Everywhere I use this, it's only GET ;)

alanpoulain · 2021-04-01T17:28:08Z

It makes sense then! It should fail if you try an update.

Toflar · 2021-04-01T17:29:55Z

Might very well be but as this is not a requirement for me, I have no tests for that :)

alanpoulain · 2021-04-01T17:36:47Z

No problem 🙂

src/Annotation/ApiProperty.php

soyuka · 2021-04-07T12:12:50Z

src/Serializer/AbstractItemNormalizer.php

+
+        // Filter out input attributes that aren't allowed for updates
+        if ($isUpdate) {
+            $data = array_filter($data, function (string $attribute) use ($context, $objectToPopulate) {


I'd be in favor of reducing the complexity in our code, enforcing the use of security_post_denormalize

DavidBennettUK · 2021-04-09T16:19:47Z

@soyuka thanks for the feedback, I've made the suggested changes 👍

alanpoulain · 2021-04-12T12:18:09Z

src/Serializer/AbstractItemNormalizer.php

+        $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) {


Was it possible to modify a property of a resource without the right to do so before?
If so, isn't it a bugfix for 2.6?

Not that I'm aware of. This is only reverting the properties that fail the new security_post_denormalize check. Properties that fail the security check are still filtered from denormalization via AbstractItemNormalizer::getAllowedAttributes()

alanpoulain · 2021-04-13T09:21:12Z

tests/Fixtures/TestBundle/Document/SecuredDummy.php

+     *
+     * @ODM\Field
+     * @ApiProperty(
+     *     security="object == null or object.getOwner() == user",


Is it still useful?

alanpoulain · 2021-04-13T09:21:26Z

tests/Fixtures/TestBundle/Entity/SecuredDummy.php

+     *
+     * @ORM\Column
+     * @ApiProperty(
+     *     security="object == null or object.getOwner() == user",


Is it still useful?

As the object isn't available in creates/updates, in this scenario you'd need to pass the security check if the objectis null, as otherwise it would fail this check (object.getOwner() would throw a null pointer exception) and never run security_post_denormalize.

But if there is already a securityPostDenormalize, why a security is needed?

Found the answer myself: it's because it is used for the get operation too.

alanpoulain · 2021-04-13T09:26:11Z

LGTM. Don't forget the documentation entry.

DavidBennettUK · 2021-04-14T17:16:05Z

Documentation PR api-platform/docs#1349

…ct` 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).

…tadata instead of reflection directly.

… reduce code complexity. `security_post_normalize` should be used if an object is required.

alanpoulain · 2021-05-06T16:10:07Z

Thank you very much @DavidBennettUK!

DavidBennettUK mentioned this pull request Apr 1, 2021

GraphQL association field security fixes + @ApiProperty security support #4143

Merged

DavidBennettUK force-pushed the api-property-security-objects branch from c9643ea to 5436060 Compare April 1, 2021 16:12

alanpoulain reviewed Apr 1, 2021

View reviewed changes

src/Serializer/AbstractItemNormalizer.php Outdated Show resolved Hide resolved

soyuka reviewed Apr 7, 2021

View reviewed changes

alanpoulain reviewed Apr 12, 2021

View reviewed changes

alanpoulain reviewed Apr 13, 2021

View reviewed changes

DavidBennettUK mentioned this pull request Apr 14, 2021

Add notes on usage of ApiProperty security_post_denormalize. api-platform/docs#1349

Merged

alanpoulain mentioned this pull request Apr 27, 2021

Update security ApiProperty variables api-platform/docs#1351

Merged

alanpoulain force-pushed the api-property-security-objects branch from f4a0e41 to 4e5ded5 Compare May 6, 2021 15:02

DavidBennettUK added 8 commits May 6, 2021 17:49

Update change log with PR #.

6f6ee14

Fix ApiPropertyTest.

a209247

Update AbstractItemNormalizer changes to use property accessor + me…

4de8bb9

…tadata instead of reflection directly.

Add more AbstractItemNormalizerTest tests for coverage.

7de1dff

Fix ApiPropertyTest.

9fc673a

Move new ApiProperty attribute parameter to last, to ensure no BC.

699f8e0

Remove the additional pre-denormalize ApiProperty security check to…

c178a85

… reduce code complexity. `security_post_normalize` should be used if an object is required.

alanpoulain force-pushed the api-property-security-objects branch from 4e5ded5 to c178a85 Compare May 6, 2021 15:49

alanpoulain changed the title ~~Fix ApiProperty security expressions not supporting objects in updates/creates~~ feat: add security_post_denormalize in ApiProperty May 6, 2021

alanpoulain merged commit de5988d into api-platform:main May 6, 2021

mbrodala mentioned this pull request Feb 21, 2022

Problem with new ApiProperty security api-platform/api-platform#1795

Open

LoicBoursin mentioned this pull request Nov 12, 2022

fix: securityPostDenormalize not working because clone is made after denormalization #5182

Merged

colinodell mentioned this pull request Apr 21, 2023

Error denormalizing to non-cloneable objects #5561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `security_post_denormalize` in `ApiProperty` #4184

feat: add `security_post_denormalize` in `ApiProperty` #4184

DavidBennettUK commented Apr 1, 2021

alanpoulain commented Apr 1, 2021 •

edited

Loading

alanpoulain Apr 1, 2021

DavidBennettUK Apr 6, 2021

soyuka Apr 7, 2021

Toflar commented Apr 1, 2021

alanpoulain commented Apr 1, 2021

Toflar commented Apr 1, 2021

alanpoulain commented Apr 1, 2021

Toflar commented Apr 1, 2021

alanpoulain commented Apr 1, 2021

soyuka Apr 7, 2021

DavidBennettUK commented Apr 9, 2021

alanpoulain Apr 12, 2021

DavidBennettUK Apr 12, 2021

alanpoulain Apr 13, 2021

alanpoulain Apr 13, 2021

DavidBennettUK Apr 14, 2021

alanpoulain Apr 27, 2021

alanpoulain May 6, 2021

alanpoulain commented Apr 13, 2021

DavidBennettUK commented Apr 14, 2021

alanpoulain commented May 6, 2021

feat: add security_post_denormalize in ApiProperty #4184

feat: add security_post_denormalize in ApiProperty #4184

Conversation

DavidBennettUK commented Apr 1, 2021

alanpoulain commented Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Apr 1, 2021

alanpoulain commented Apr 1, 2021

Toflar commented Apr 1, 2021

alanpoulain commented Apr 1, 2021

Toflar commented Apr 1, 2021

alanpoulain commented Apr 1, 2021

Choose a reason for hiding this comment

DavidBennettUK commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain commented Apr 13, 2021

DavidBennettUK commented Apr 14, 2021

alanpoulain commented May 6, 2021

feat: add `security_post_denormalize` in `ApiProperty` #4184

feat: add `security_post_denormalize` in `ApiProperty` #4184

alanpoulain commented Apr 1, 2021 •

edited

Loading