[WIP] Deprecate AbstractAdmin::isGranted #4287

fbourigault · 2017-01-23T23:02:39Z

I am targetting this branch, because I'm adding deprecation notices.

Closes #4095.

Changelog

### Deprecated
- deprecated `AbstractAdmin::isGranted`.
### Added
- added `sonata_is_granted` twig function.

To do

Subject

This PR remove a responsibility from the AbstractAdmin class. The AbstractAdmin::isGranted method is now deprecated. SecurityHandlerInterface::isGranted has now to be called directly.

The caching feature has been moved to a new CacheSecurityHandler which decorates the sonata.admin.security.handler service.

A twig function is added to replace admin.is_granted calls.

There is no longer cache if admin security_handler is customized. Should I decorate the SecurityHandler once per admin class?

SecurityHandlerInterface::isGranted third argument was either the object or the admin when called through AbstractAdmin::isGranted. Does using null as SecurityHandlerInterface::isGranted third argument has also to be deprecated?

greg0ire · 2017-01-23T23:08:53Z

Admin/AbstractAdmin.php

@@ -422,8 +422,6 @@
     */
    protected $securityInformation = array();

-    protected $cacheIsGranted = array();


Removing a protected method is a BC-break. Accessing it could be deprecated mith magic methods if deemed worth it. WDYT @sonata-project/contributors ?

Accessing it could be deprecated mith magic methods if deemed worth it.

Dunno, dou you have some samples?

We can start by adding @deprecated annotation for IDE.

Would be https://github.com/symfony/symfony/pull/20769/files.

@soullivaneuh yes, and a very fresh one at that : https://github.com/sonata-project/SonataAdminBundle/pull/4284/files#diff-f06e49acf2d25569dcbac961750bdb33R113 (I like this one better, more mutualization).

@greg0ire the example you showed would not work. Magic methods are only called if property is inaccessible. I can make the property private instead of protected. Moreover, is it a BC break to make the property noop?

Oh right, but neither would your example work then, I guess… BTW, what do you mean by "noop" here?

I mean will always be empty, never read or write by sonata, so it may break classes extending AbstractAdmin, no?

Oh I see… well it might. But then again, every single change you do can be considered a BC-break if you think hard enough. I think in this case it's ok to dismiss this one.

I added the magic methods and deprecated annotations.

greg0ire · 2017-01-23T23:10:50Z

Resources/config/security.xml

@@ -10,6 +10,13 @@
    </parameters>
    <services>
        <service id="sonata.admin.security.handler.noop" class="%sonata.admin.security.handler.noop.class%" public="false"/>
+        <service id="sonata.admin.security.handler.cache"
+            class="Sonata\AdminBundle\Security\Handler\CacheSecurityHandler"
+            decorates="sonata.admin.security.handler"


Nice! Didn't know this feature.

Sadly I can't use this feature introduced in Symfony 2.5. Can I directly set the sonata.admin.security.handler to be the cache one and use the security.handler configuration value as first argument?

Argh. Yes, sure. But please add NEXT_MAJOR instructions explaining how to switch to native decoration

greg0ire · 2017-01-23T23:12:46Z

Security/Handler/CacheSecurityHandler.php

+
+use Sonata\AdminBundle\Admin\AdminInterface;
+
+class CacheSecurityHandler implements SecurityHandlerInterface


Please make this class final.

How about suffixing the class name with Decorator ?

greg0ire · 2017-01-23T23:15:37Z

Security/Handler/CacheSecurityHandler.php

+
+    /**
+     * @param SecurityHandlerInterface $handler
+     */


Personal opinion : I'd remove this phpdoc.

Other constructors have param comments on constructors. IMHO, should be kept for consistency.

We should keep the @param comments, because they could describe the parameters with more information

Yeah but when they don't, what's the point?

We always keept the comments so far. Wouldn't StyleCI fix them automatically?

I don't think it can.

greg0ire · 2017-01-23T23:16:21Z

Twig/Extension/SecurityExtension.php

+
+use Sonata\AdminBundle\Admin\AdminInterface;
+
+class SecurityExtension extends \Twig_Extension


Please make this class final (extending twig extensions is deprecated anyway I think)

soullivaneuh · 2017-01-24T08:49:43Z

Admin/AbstractAdmin.php

@@ -422,8 +422,6 @@
     */
    protected $securityInformation = array();

-    protected $cacheIsGranted = array();


Accessing it could be deprecated mith magic methods if deemed worth it.

Dunno, dou you have some samples?

We can start by adding @deprecated annotation for IDE.

soullivaneuh · 2017-01-24T08:51:23Z

Admin/AbstractAdmin.php

-        }
+        @trigger_error(
+            'The '.__METHOD__.' method is deprecated since version 3.12 and will be removed in 4.0.'.
+            ' Use Sonata\AdminBundle\Security\Handler\SecurityHandlerInterface::isGranted instead.',


This method is also use from twig quite often.

We should tell the twig function replacement on this message too.

greg0ire · 2017-01-24T09:20:36Z

There is no longer cache if admin security_handler is customized. Should I decorate the SecurityHandler once per admin class?

Not sure if this optimization would be worth it (and why there is a cache system in the first place). Not sure I get what you mean by decorating the SecurityHandler once per class either. Do you mean the customization happens by replacing the decorated handler with something else?

SecurityHandlerInterface::isGranted third argument was either the object or the admin when called through AbstractAdmin::isGranted. Does using null as SecurityHandlerInterface::isGranted third argument has also to be deprecated?

Are you should the security handler is not called with null from anywhere? Maybe there is a use-case for this…

fbourigault · 2017-01-24T13:00:44Z

Not sure if this optimization would be worth it (and why there is a cache system in the first place). Not sure I get what you mean by decorating the SecurityHandler once per class either. Do you mean the customization happens by replacing the decorated handler with something else?

Decorating once per class means having one instance of the CacheSecurityHandler per admin class.

fbourigault · 2017-01-24T21:30:23Z

How should I handle the deprecation in TestAdmin::testIsGranted?

About the new twig function, is this really useful or we just let users write admin.security_handler.is_granted(...) and don't provide any function?

greg0ire · 2017-01-24T22:53:36Z

How should I handle the deprecation in TestAdmin::testIsGranted?

You add the @group legacy annotation to the test.

About the new twig function, is this really useful or we just let users write admin.security_handler.is_granted(...) and don't provide any function?

I think sonata_is_granted should know what the current admin is, it would be more useful then. What do you think?

fbourigault · 2017-01-27T12:35:00Z

I think sonata_is_granted should know what the current admin is, it would be more useful then. What do you think?

Have you an idea about how to handle this?

greg0ire · 2017-01-27T12:39:13Z

Maybe there : https://github.com/sonata-project/SonataAdminBundle/blob/3.x/Controller/CRUDController.php#L980 , by getting the service and injecting the admin into it?

fbourigault · 2017-01-27T12:48:30Z

To keep BC, would configuring the twig extension in CRUDController::render be better? Looks like the admin twig property may not be $this->admin: https://github.com/sonata-project/SonataAdminBundle/blob/3.x/Controller/CRUDController.php#L68-L70

greg0ire · 2017-01-27T12:50:18Z

You're right, the latest the better, so just before rendering would be ideal in this case.

greg0ire · 2017-01-27T13:39:25Z

Admin/AbstractAdmin.php

+        @trigger_error(sprintf(
+                'Using the "%s::$%s" property is deprecated since version 3.x and will be removed in 4.0.',
+                __CLASS__,
+                $name),


This is a PSR2 violation, not fixed by php-cs-fixer yet but I made a PR

should be

$name ), E_USER_DEPRECATED);

greg0ire · 2017-01-27T13:47:42Z

I think sonata_is_granted should know what the current admin is, it would be more useful then. What do you think?

People should be able to pass the admin they want anyway, I think sometimes they want to check permissions in the parent admin for instance, it should be possible.

fbourigault · 2017-01-29T21:44:15Z

I updated the sonata_is_granted twig function. The admin is now the latest argument with a null default value and in the CRUDController::render method, the current admin is set into the extension.

OskarStark · 2017-01-30T08:17:59Z

Admin/AbstractAdmin.php

+     * NEXT_MAJOR: remove this property, the __get, __set, __isset, __unset magic methods and the
+     * triggerPropertyDeprecation method.
+     */
+    private $cacheIsGranted = array();


making it private instead of protected is a BC break, too, isn't it?

Magic methods are added to avoid BC break.

realized this afterwards ;-)

By the way, doesn't this magic make the property suddenly "public"?

Technically, yes. But too many protection around this property can create BC breaks when all those stuff is removed. See symfony/symfony#20769 (comment).

Ok 👍 Let's keep it (relatively) simple

OskarStark · 2017-01-30T08:18:15Z

Admin/AbstractAdmin.php

@@ -3326,4 +3380,16 @@ private function buildRoutes()
            $extension->configureRoutes($this, $this->routes);
        }
    }
+
+    /**
+     * @internal


we should add a NEXT_MAJOR here

I added a global NEXT_MAJOR on the $cacheIsGranted property. Should I dispatch the comment here and to magic methods too?

not sure, what do you think @greg0ire ?

global NEXT_MAJOR is good enough, this will be done by a human who can read.

AbstractAdmin::isGranted is now deprecated. Use Sonata\AdminBundle\Security\Handler\SecurityHandlerInterface::isGranted instead. In twig templates, use sonata_is_granted instead of admin.is_granted

fbourigault · 2017-01-30T12:26:15Z

Is everybody ok with code? If so, I will write the docs, changelog and upgrade notes.

About codacy, it fails because I set $cacheIsGranted private. For coveralls, I guess it's because I added magic methods for deprecation layer.

greg0ire · 2017-01-30T12:34:28Z

I'm ok with it!

OskarStark · 2017-01-30T12:42:16Z

Looks good to me 👍

OskarStark · 2017-01-31T07:37:20Z

@fbourigault please add something to the Changelog in your PR header, I prepared that for you!

OskarStark

Changelog and Tests please

fbourigault · 2017-01-31T12:00:59Z

@OskarStark I added the changelog entries. What do you mean by tests? I already wrote SecurityExtension and CacheSecurityHandler tests.

Should I add something in an UPGRADE file?

I'm going to update the documentation.

OskarStark · 2017-01-31T12:10:58Z

Tests are fine 👍

Should I add something in an UPGRADE file?

yes, please!

fbourigault · 2017-01-31T12:16:35Z

When updating documentation I get worried about the new use of checking permissions.

Before we just had to call $this->isGranted('EDIT') but now we have to call $this->getSecurityHandler()->isGranted($this, 'EDIT', $this). To keep the old behavior, developer has to pass $this as third argument when no object is provided.

Does anyone have an idea about how to avoid this extra argument?

greg0ire · 2017-01-31T12:52:02Z

Does anyone have an idea about how to avoid this extra argument?

If the first argument is an instance of AdminInterface, use it also as third argument?

greg0ire · 2017-01-31T12:52:54Z

Wait, it's always supposed to be an AdminInterface

fbourigault · 2017-01-31T13:17:54Z

Wait, it's always supposed to be an AdminInterface

The third argument could be an object if you want to check rights against a specific object.

greg0ire · 2017-01-31T13:18:43Z

I was talking about the first object. But the third is optional anyway, right?

fbourigault · 2017-01-31T13:38:26Z

First argument is type hinted to be and AdminInterface.
The third is optional but the old AbstractAdmin::isGranted code use $this if third argument was null.
So, to keep the behavior, developers have to send the admin as third argument.
Maybe we could add another decorator to keep this behavior?

greg0ire · 2017-01-31T13:49:52Z

Indeed… It means we have even more things to move out of the admin. The first argument is only needed to retrieve admin->getCode() I think.

fbourigault · 2017-02-02T22:53:41Z

After an instructive discussion with @greg0ire on slack, I'm pausing this PR to make an alternative proposal. Stay tuned :)

jordisala1991 · 2017-03-09T20:28:51Z

Why don't we use hasAccess instead of isGranted and do not introduce that twig function @fbourigault ?

core23 · 2017-03-10T13:49:40Z

Twig/Extension/SecurityExtension.php

+    /**
+     * @param AdminInterface $admin
+     */
+    public function setAdmin(AdminInterface $admin)


This looks so wrong. Why would you inject one AdminInterface instance here? Twig extensions are stateless

SonataCI · 2017-03-15T08:30:07Z

Could you please rebase your PR and fix merge conflicts?

core23 · 2017-08-25T20:40:12Z

Any progress here @fbourigault ?

fbourigault · 2017-08-25T21:06:12Z

I didn't get anything that satisfy myself to make a good deprecation of the isGranted method. Moreover, as I have less and less interest in Sonata, I can't find time to work on this PR. Feel free to close it or keep it open to let someone else continue this work.

yceruto · 2017-11-16T15:29:24Z

Security/Handler/CacheSecurityHandler.php

+     */
+    public function isGranted(AdminInterface $admin, $attributes, $object = null)
+    {
+        $key = md5(json_encode($attributes)).'/'.spl_object_hash($object);


spl_object_hash($object) throws an error if it is null.

SonataCI added the review required label Jan 23, 2017

greg0ire requested changes Jan 23, 2017

View reviewed changes

soullivaneuh requested changes Jan 24, 2017

View reviewed changes

greg0ire reviewed Jan 27, 2017

View reviewed changes

OskarStark added the minor label Jan 30, 2017

OskarStark requested changes Jan 30, 2017

View reviewed changes

deprecate AbstractAdmin::isGranted

cdff4d2

AbstractAdmin::isGranted is now deprecated. Use Sonata\AdminBundle\Security\Handler\SecurityHandlerInterface::isGranted instead. In twig templates, use sonata_is_granted instead of admin.is_granted

OskarStark approved these changes Jan 30, 2017

View reviewed changes

greg0ire approved these changes Jan 30, 2017

View reviewed changes

core23 approved these changes Jan 30, 2017

View reviewed changes

OskarStark added Needs: Changelog note Needs: Documentation labels Jan 31, 2017

OskarStark requested changes Jan 31, 2017

View reviewed changes

OskarStark removed the Needs: Changelog note label Jan 31, 2017

OskarStark approved these changes Jan 31, 2017

View reviewed changes

core23 requested changes Mar 10, 2017

View reviewed changes

SonataCI added the pending author label Mar 15, 2017

SonataCI removed the pending author label Aug 25, 2017

SonataCI removed the review required label Sep 20, 2017

yceruto reviewed Nov 16, 2017

View reviewed changes

fbourigault closed this Sep 19, 2018


		use Sonata\AdminBundle\Admin\AdminInterface;

		class CacheSecurityHandler implements SecurityHandlerInterface


		use Sonata\AdminBundle\Admin\AdminInterface;

		class SecurityExtension extends \Twig_Extension

Uh oh!

[WIP] Deprecate AbstractAdmin::isGranted #4287

[WIP] Deprecate AbstractAdmin::isGranted #4287

Uh oh!

Conversation

Uh oh!

Changelog

To do

Subject

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment