api-platform
"object" null in security attribute for canAccessAttribute() on PUT/PATCH
API Platform version(s) affected: x3.1
Description
Imagine this property on an ApiResource:
class Cake
{
// ...
#[ApiProperty(security: 'is_granted("FLAVOR", object)')]
private ?string $flavor = null;
}
During a GET operation, the object variable is the Cake object. Expected! 🥇
However, during a PATCH operation, the object variable is null during deserialization.
I believe this is a bug, as there IS an object available during deserialization for PUT/PATCH. The current behavior makes it impossible to make flavor READable using the security expression without preventing it from always failing to be WRITEable because the object is null (and your voter probably needs the object).
The workaround is bizarre:
class Cake
{
// ...
#[ApiProperty(security: 'object === null or is_granted("FLAVOR", object)')]
private ?string $flavor = null;
}
You have to allow the security to pass if the object is null so that deserialization works for PUT/PATCH. Then, during serialization, object will be populated so the voter will always run.
How to reproduce
Small reproducer! https://github.com/weaverryan/api_platform_null_object_security_reproducer
Possible Solution
Somewhere around https://github.com/api-platform/core/blob/60075501a6d15d03d4db26f00715a52cac068977/src/Serializer/AbstractItemNormalizer.php#L443, if $object is null, we look also for $context['object_to_populate'].
In the security rule you should have access to the existing value in DB of the object, but not to the data that is currently being deserialized (to do so postDenormalizationSecurity must be used, as pointed out by @mrossard). Is the initial value not available?
The goal is to make the field only readable if the security rule passes. That works well hence using “security”.
The problem is that this has a side effect: the field is now never writable.
I can’t find a way to make the field readable only if the security expression passes without it becoming never writable. I actually don’t want any “write” security on this field (it’s protected on the operation level). I had assumed that, even though I don’t care, the security expression would work the same during read and PATCH.
another solution is have a way to have read security that is different than write security.
The goal is to make the field only readable if the security rule passes. That works well hence using “security”.
The problem is that this has a side effect: the field is now never writable.
I can’t find a way to make the field readable only if the security expression passes without it becoming never writable. I actually don’t want any “write” security on this field (it’s protected on the operation level). I had assumed that, even though I don’t care, the security expression would work the same during read and PATCH.
another solution is have a way to have read security that is different than write security.
I think i remember having a similar problem, trying to find the right combination of security / securityPostDenormalize to get it working as I wanted. If i remember well i ended up giving up since it became overly complicated and it wasn"t a big deal in that app... You can still achieve a similar effect with a custom provider by not setting the field if your condition is not fulfilled, can't you?
You can still achieve a similar effect with a custom provider by not setting the field if your condition is not fulfilled, can't you?
Hmm, yea,. I think that would work - good point!
Still, it feels that the fix in #5756 feels legit. Kévin mentioned:
In the security rule you should have access to the existing value in DB of the object, but not to the data that is currently being deserialized
But in this case of a PATCH, there IS an existing value in the database, and the data provider has already fetched it. Now, it's denormalizing the JSON data onto that existing object. So it seems consistent that object would be set to OBJECT_TO_POPULATE (i.e. this existing value from the database / from the state provider).
We ran into this ourselves. We ended up having to use "previous_data" instead of object. We were lucky that we were doing it as an entire class security expression on the operation itself.
I fully agree that object should be populated for a PATCH as there is an existing object available from a provider.
I do have a patch on #5756 - but not a test case - I'd be happy if someone smarter than me could find a way to add one