[AbstractAdmin] isGranted() cache should not rely on spl_object_hash()

[fancyweb]

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.37.0 3.51.0 The missing Symfony Admin Generator
sonata-project/block-bundle              3.12.1 3.15.0 Symfony SonataBlockBundle
sonata-project/cache                     1.1.1  2.0.1  Cache library
sonata-project/cache-bundle              2.4.2  3.1.0  This bundle provides caching services
sonata-project/classification-bundle     3.7.1  3.8.1  Symfony SonataClassificationBundle
sonata-project/core-bundle               3.11.2 3.17.0 Symfony SonataCoreBundle
sonata-project/dashboard-bundle          0.3.0  0.3.0  Provides a Dashboard management through container and block...
sonata-project/datagrid-bundle           2.3.1  3.0.0  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.0.2  1.3.0  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.6.1  3.9.0  Symfony Sonata / Integrate Doctrine ORM into the SonataAdmi...
sonata-project/easy-extends-bundle       2.5.0  2.5.0  Symfony SonataEasyExtendsBundle
sonata-project/exporter                  1.9.1  2.0.1  Lightweight Exporter library
sonata-project/formatter-bundle          3.5.0  4.1.3  Symfony SonataFormatterBundle
sonata-project/intl-bundle               2.5.0  2.6.0  Symfony SonataIntlBundle
sonata-project/media-bundle              3.15.0 3.20.1 Symfony SonataMediaBundle
sonata-project/notification-bundle       3.5.1  3.6.1  Symfony SonataNotificationBundle
sonata-project/page-bundle               3.9.0  3.11.1 This bundle provides a Site and Page management through con...
sonata-project/seo-bundle                2.5.1  2.7.0  Symfony SonataSeoBundle
sonata-project/user-bundle               4.2.3  4.4.0  Symfony SonataUserBundle

Symfony packages

$ composer show --latest 'symfony/*'
symfony/monolog-bundle     v3.3.0  v3.4.0  Symfony MonologBundle
symfony/phpunit-bridge     v3.4.27 v4.3.2  Symfony PHPUnit Bridge
symfony/polyfill-apcu      v1.9.0  v1.11.0 Symfony polyfill backporting apcu_* functions to lower PHP versions
symfony/polyfill-ctype     v1.10.0 v1.11.0 Symfony polyfill for ctype functions
symfony/polyfill-intl-icu  v1.9.0  v1.11.0 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-mbstring  v1.9.0  v1.11.0 Symfony polyfill for the Mbstring extension
symfony/polyfill-php56     v1.9.0  v1.11.0 Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions
symfony/polyfill-php70     v1.9.0  v1.11.0 Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions
symfony/polyfill-php72     v1.9.0  v1.11.0 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-util      v1.9.0  v1.11.0 Symfony utilities for portability of PHP codes
symfony/security-acl       v3.0.1  v3.0.2  Symfony Security Component - ACL (Access Control List)
symfony/swiftmailer-bundle v3.2.2  v3.2.8  Symfony SwiftmailerBundle
symfony/symfony            v3.4.26 v4.3.2  The Symfony PHP framework

PHP version

$ php -v
PHP 7.2.19 (cli) (built: Jun  1 2019 00:59:10) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.2.19, Copyright (c) 1999-2018, by Zend Technologies

Subject

AbstractAdmin::isGranted() uses spl_object_hash() to create its cache keys but two different objects can have the same hash.

Steps to reproduce

function foo()
{
    // $admin is an instance of AbstractAdmin

    $obj = new \stdClass();
    $admin->isGranted('bar', $obj);
}

// On first call, isGranted() is called with $obj and the cache is populated
// (cf https://github.com/sonata-project/SonataAdminBundle/blob/62910857db1aacb6ad101aa502123d7ff01645bc/src/Admin/AbstractAdmin.php#L2139)
// based on the spl_object_hash() of the passed $obj.
foo();

// On the second call, previous $obj has been garbage collected, isGranted()
// is called with a new instance of $obj that will end up having the same
// spl_object_hash() than the one before
// (cf https://www.php.net/manual/en/function.spl-object-hash.php),
// thus hitting the previous cached result while it's not the same instance.
foo();

Expected results

On the second call, the cache is not hit.

Actual results

On the second call, the cache is hit.

[OskarStark]

Thanks for reporting this issue. What do you propose? Would you mind send a PR?

Thank you!

[core23]

We should use the object id/primary key instead

[phansys]

Do we define a interface for this purpose? Primary key is surely present in majority of database based model managers, but we have no warranty on the rest. And I don't know if we are forcing the related subjects to implement getId() method.

[core23]

What about the AbstractAdmin::getIdParameter can we use it instead?

[phansys]

AbstractAdmin::getIdParameter() returns strings like "id", "childId", etc: https://github.com/sonata-project/SonataAdminBundle/blob/bc279a80e701c200251d802c7910ff782c2770e5/src/Admin/AbstractAdmin.php#L1142-L1151

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.