[SecurityBundle] Fatal error on check is granted for class field

[ishakuta]

Initial issue described here symfony/symfony#12875 by @alipek

Here's updated description:

Try to check field: is_granted('VIEW', nameClass, 'fooFieldName')
application throw error:

Fatal Error: Argument 1 passed to Symfony\Component\Security\Acl\Domain\UserSecurityIdentity::equals() must implement interface Symfony\Component\Security\Acl\Model\SecurityIdentityInterface, instance of Symfony\Component\Security\Acl\Domain\FieldEntry given

or, if sid is Role:

Catchable Fatal Error: Argument 1 passed to Symfony\Component\Security\Acl\Domain\RoleSecurityIdentity::equals() must implement interface Symfony\Component\Security\Acl\Model\SecurityIdentityInterface,
instance of Symfony\Component\Security\Acl\Domain\FieldEntry given,
called in ... custom class ("$sid->equals($ace->getSecurityIdentity());") ... and defined in vendor/symfony/symfony/src/Symfony/Component/Security/Acl/Domain/RoleSecurityIdentity.php on line 53

This is problem with serializaton of FieldEntry and Entry types.
serialization working when Acl have only one object of FieldEntry, but when is more that failed

@alipek showed this with test in pull request symfony/symfony#12873, that was closed by @fabpot due to acl component was moved into this repository,
so here's new PR with test to demonstrate bug: #8

Update: Here's serialized data from the example test (difference between two ACEs highlighted):

a:1:{i:0;a:2:{s:8:"fieldOne";a:1:{i:0;C:48:"Symfony\Component\Security\Acl\Domain\FieldEntry":312:{a:2:{i:0;s:3:"foo";i:1;s:279:"a:7:{i:0;i:123456;i:1;i:123;i:2;**O:39:"Mock_SecurityIdentityInterface_54ceb335":2:{s:67:"Mock_SecurityIdentityInterface_54ceb335__phpunit_invocationMocker";N;s:65:"Mock_SecurityIdentityInterface_54ceb335__phpunit_originalObject";N;}**i:3;s:8:"foostrat";i:4;b:0;i:5;b:1;i:6;b:1;}";}}}s:8:"fieldTwo";a:1:{i:0;C:48:"Symfony\Component\Security\Acl\Domain\FieldEntry":112:{a:2:{i:0;s:3:"foo";i:1;s:80:"a:7:{i:0;i:123456;i:1;i:123;i:2;**r:8;**i:3;s:8:"foostrat";i:4;b:0;i:5;b:1;i:6;b:1;}";}}}}}

First FieldEntry has SecurityIdentityInterface mock,
second FieldEntry has "r8", that is somehow restored to first FieldEntry.

PHP 5.5.26
Not exactly the same, but similar serialize/unserialize issue: https://3v4l.org/sSL6F
Another example provided by @alsma https://3v4l.org/viORV

[ishakuta]

Guys, is it something occurred only to me and @alipek and @alsma? Will someone check this? If so - please, let me know your thoughts.
Thank you.
cc: @fabpot @Tobion @stof

[xabbuh]

Not sure I completely got your issue. Would you be able to create a failing test case for your issue?

[ishakuta]

@xabbuh I've prepared PR https://github.com/symfony/security-acl/pull/8/files and there's a test that will fail without provided fix in Domain/Entry.php

[ewgRa]

@ishakuta

second FieldEntry has "r8"

It is expected. It is how PHP mark recursive serialization.

For example:

class A{};
$a = new A();
echo serialize(array($a, $a));

Will output

a:2:{i:0;O:1:"A":0:{}i:1;r:2;}

When you add clone - you just prevent recursive serialization.

Problem started when we start using serialize/unserialize twice, in our case first serialize we have in FieldEntry, and then in Entry. PHP internally (I suggest) have only one "buffer" for recursive object and when we call it twice - we lose recursive objects and unserialize from recursive works not well.

I try to show it in gist https://gist.github.com/ewgRa/721cac08e3c2151366c8
gistfile1.txt - give expected results, gistfile2.txt - show problem.

Most related bug in php is https://bugs.php.net/bug.php?id=66085, maybe there is another.

I think many another classes can have similar problems.

I prepare #15 PR that show possible workaround.

cc: @fabpot @Tobion @stof @xabbuh What is true way? Wait when bug will be fixed in PHP? Change how object serialized everywhere?

[ishakuta]

@ewgRa you're right, when I first saw this recursive serialization - thought it kind of a bug, then realised it's just a recursive link, but my point was that unserialize doesn't work properly in such case.

Indeed, your solutions looks more clear than mine 👍
So waiting for symfony guys @xabbuh @fabpot opinion on this.

[ewgRa]

I grep Symfony source by "parent::unserialize" and it is not so big problem as I think before.

I will prepare clean PR later for this case and than will wait review and maybe instructions how to avoid similar problems later. I think how Symfony implement serialization/unserialization at extended classes must be changed to avoid similar problems in future.

[ewgRa]

PR as I see it done.

[wouterj]

This is now fixed, as the lowest supported PHP version of this package is 7.2 and we recently switched to the better serialization implemention of PHP in #90