core icon indicating copy to clipboard operation
core copied to clipboard

Allow config option to throw 403 when securityPostDenormalize fails for an API property

Open dwgebler opened this issue 7 months ago • 3 comments

Description
At the moment, when implementing securityPostDenormalize at the property level in a POST, PATCH or PUT request, if a field is in the request body for which the user does not meet the security expression, the field is silently reverted to its previous or default value in AbstractItemNormalizer:

        foreach (array_keys($data) as $attribute) {
            $attribute = $this->nameConverter ? $this->nameConverter->denormalize((string) $attribute) : $attribute;
            if (!\in_array($attribute, $propertyNames, true)) {
                continue;
            }

            if (!$this->canAccessAttributePostDenormalize($object, $previousObject, $attribute, $context)) {
                if (null !== $previousObject) {
                    $this->setValue($object, $attribute, $this->propertyAccessor->getValue($previousObject, $attribute));
                } else {
                    $propertyMetadata = $this->propertyMetadataFactory->create($resourceClass, $attribute, $options);
                    $this->setValue($object, $attribute, $propertyMetadata->getDefault());
                }
            }
        }

It might be good if we had a config option in the package api_platform.yaml or in the ApiProperty attribute to instead instruct API Platform to throw an appropriate exception and return a 403 or maybe 422 in these cases, depending on use-case.

dwgebler avatar Jun 09 '25 14:06 dwgebler

Yes directly on the ApiProperty looks like a good option, directly inside canAccessAttributePostDenormalize. Because it's painful to add a new property maybe it'd be enough to just use the extraProperties?

#[ApiProperty(security: xxx, extraProperties: ['throw_on_access_denied' => true])]. 

soyuka avatar Jun 13 '25 09:06 soyuka

@soyuka yeah I think extra_properties would be perfect, actually. Could be set at both the Operation level (and configured via defaults in the YAML if a user wants to) and property level, I guess something like:

        $operation = $context['operation'] ?? null;
        $extraProperties = $operation instanceof Metadata ?
            $operation->getExtraProperties() :
            ($context['extra_properties'] ?? []);
        $parameters = $operation instanceof Metadata ? $operation->getParameters() : [];

        $throwOnAccessDenied = $extraProperties['throw_on_access_denied'] ?? false;
        $securityMessage = $operation->getSecurityMessage() ?? null;

        // Revert attributes that aren't allowed to be changed after a post-denormalize check
        foreach (array_keys($data) as $attribute) {
            $attribute = $this->nameConverter ? $this->nameConverter->denormalize((string) $attribute) : $attribute;
            $attributeMeta = $this->propertyMetadataFactory->create($resourceClass, $attribute, $options);
            $attributeExtraProperties = $attributeMeta->getExtraProperties() ?? [];
            $throwOnAccessDenied = (bool) ($attributeExtraProperties['throw_on_access_denied'] ?? $throwOnAccessDenied);
            if (!\in_array($attribute, $propertyNames, true)) {
                continue;
            }

            if (!$this->canAccessAttributePostDenormalize($object, $previousObject, $attribute, $context)) {
                if ($throwOnAccessDenied) {
                    throw new AccessDeniedException($securityMessage ?? 'Access denied');
                }
                ...

You of course may know a better way of implementing this in API Platform Core than I do, but that's my sort of rough draft of what I'm suggesting.

dwgebler avatar Jun 13 '25 13:06 dwgebler

feel free to open a pr

soyuka avatar Jun 13 '25 14:06 soyuka