[Security] Deprecated supportsAttribute and supportsClass methods #15151

wouterj · 2015-06-30T12:48:13Z

These methods aren't used at all in a Symfony application and don't make sense to use in the application. They are only used internally in the voters. This means the voter interface can be made much easier.

I'm not sure how we do these deprecations, should we remove the methods from the interface now already? Also, I don't think it's possible to trigger deprecation notices for the voter methods?

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	one of #11742
License	MIT
Doc PR	-

Abstract Voter

There is one remaining question about the abstract voter. This currently has abstract getSupportedAttributes() and getSupportedClass() methods. One of the reasons to remove the methods for the interface was that these methods are not flexible. Does it make sense to deprecate these methods as well and replace them by an abstract protected vote(array $attributes, $class) method in the AbstractVoter (which is called from AbstractVoter#vote()) ?

dosten · 2015-07-01T02:40:24Z

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

@@ -77,6 +77,8 @@ public function decide(TokenInterface $token, array $attributes, $object = null)
     */
    public function supportsAttribute($attribute)
    {
+        trigger_error('The '.__METHOD__.' is deprecated since version 2.8 and will be removed in version 3.0.');


You should append the trigger_error call with @

wouterj · 2015-07-01T07:40:51Z

Thanks @dosten. Fixed everything you mentioned :)

stof · 2015-07-01T08:04:49Z

@wouterj I suggest you to mark methods as deprecated in the VoterInterface too, and to trigger the deprecation in the AbstractVoter when they are called.
We should not trigger deprecations for people implementing the method: it is required, and it would not hurt to have them on 3.0 anyway.

soullivaneuh · 2015-08-03T08:08:53Z

These methods aren't used at all in a Symfony application and don't make sense to use in the application.

What do you mean? I use those methods on my Voters class, working with it.

Sample:

class DomainVoter extends AbstractVoter
{
    const VIEW      = 'view';
    const EDIT      = 'edit';
    const DELETE    = 'delete';

    /**
     * {@inheritdoc}
     */
    protected function getSupportedClasses()
    {
        return [
            'AppBundle\Entity\Domain',
        ];
    }

    /**
     * {@inheritdoc}
     */
    protected function getSupportedAttributes()
    {
        return [
            self::VIEW,
            self::EDIT,
            self::DELETE,
        ];
    }

    /**
     * @param string               $attribute
     * @param Domain               $object
     * @param UserInterface|string $user
     *
     * @return bool
     */
    protected function isGranted($attribute, $object, $user = null)
    {
        return true; // Stuff
    }
}

What should be done instead of using those deprecated methods?

stof · 2015-08-03T09:19:56Z

@soullivaneuh if you look at the AbstractVoter, it uses these methods internally. But the methods of the interface are never used.
The AbstractVoter could keep using these methods (to hook into the main logic of the vote method), but they should not be part of the VoterInterface

stof · 2015-08-03T09:20:34Z

And btw, the methods you are showing there are not the methods which are meant to be deprecated

fabpot · 2015-08-03T09:22:17Z

@wouterj Can you finish this one?

soullivaneuh · 2015-08-03T09:26:23Z

@stof Indeed, my bad. Wrong reading.

wouterj · 2015-08-09T14:33:53Z

Updated this PR:

I've changed the API of AbstractVoter to have a supports($attribute, $class) method. The reason behind this is to allow some attributes to be accepted for only some classes.
In order to remove too much duplication in AbstractVoter#supports(), I've added an AbstractVoter#isClassInstanceOf() helper method.
I've added some tests for AbstractVoter, in order to make sure the behaviour is kept equal.
I tried to trigger deprecation in as much places as possible.

Fabbot's error is a bug in fabbot.

fabpot · 2015-09-22T12:53:16Z

👍 (should be rebased)

fabpot · 2015-09-24T11:45:54Z

ping @symfony/deciders

Taluu · 2015-09-24T13:07:39Z

src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php

+     */
+    protected function supports($attribute, $class)
+    {
+        @trigger_error('The getSupportedClasses and getSupportedAttributes methods are deprecated since version 2.8 and will be removed in version 3.0. Overwrite supports instead.');


why this trigger here ?

People have to override the supports method to see if the attribute and class are supported. If they didn't, this code will be executed and they will recieve a deprecation notice. If they did, this code is overriden and the deprecation notice isn't triggered.

The triggers in the getSupported* classes are there in case someone is executing this methods directly, to indicate that they have to use supports.

Well, if this method is executed, then wouldn't the other deprecations also be triggered ? So basically here we will have at least 2 (and at most 3) deprecations warnings (one for the call on supports, which triggers this as BC, and one or two for the usage of getSupported* methods)

Yes, but if we remove one of them, we'll loose information or we didn't cover all BC breaks. There are more cases in which you get 2 or more deprecation messages by using 1 deprecated feature (every deprecated setting configurable in yaml/xml/php for example)

Getting the 2 deprecation messages for the deprecated calls to getSupported* are fine IMO, but I think the deprecation warning for this method is not necessary, that was my point.

wouterj · 2015-09-24T14:14:36Z

Rebased, so it can be merged directly if accepted by core

Taluu · 2015-09-24T14:22:29Z

src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php

+     *
+     * @return bool True if the attribute and class is supported, false otherwise
+     */
+    protected function supports($attribute, $class)


I guess this method will turn abstract on 3.0 ?

yes it will. And this is why there is a deprecation warning in the provided implementation.

@wouterj please add a comment in the phpdoc saying it will become abstract in 3.0

xabbuh · 2015-09-24T15:48:22Z

Seems like some tests still test the deprecated features and should be added to the legacy group (:+1: when this is fixed).

wouterj · 2015-09-25T07:34:05Z

So it seems like we now had 2 tests for AbstractVoter (one in Security\Tests\Core and one in Security\Core\Tests). I've now merged both test classes, everything should work now

xabbuh · 2015-09-25T11:32:08Z

👍 (the failures don't seem to be related)

Status: Reviewed

fabpot · 2015-09-25T11:44:15Z

Thank you @wouterj.

weaverryan · 2015-09-26T15:22:10Z

@wouterj All the trigger_error() calls in this PR are missing the E_USER_DEPRECATED flag at the end (so they're actually notices right now).

wouterj · 2015-09-26T15:44:30Z

Whoa, thanks for catching this @weaverryan! Fixed in #15922

@weaverryan

This PR was merged into the 2.8 branch. Discussion ---------- [Security] Fix trigger_error calls in voters Fixes typos found by @weaverryan in #15151 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 69e80be Fix trigger_error calls

This PR was merged into the 2.8 branch. Discussion ---------- [2.8] Document some Security changes | Q | A | --- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#15131, symfony/symfony#16493, symfony/symfony#15151 | Applies to | 2.8+ | Fixed tickets | - Commits ------- 0526ca0 Document deprecation of supports{Attribute,Class}() methods 22026ee Document Security key to secret renamings 4036d26 Use new Simple{Form,Pre}AuthenticatorInterface namespaces

dosten reviewed Jul 1, 2015
View reviewed changes

wouterj force-pushed the cleanup_voter_interface branch from 4da886e to f98a05c Compare July 1, 2015 07:23

xabbuh added the Security label Jul 4, 2015

Koc mentioned this pull request Aug 2, 2015

[Idea] Class auto-completion on SecurityVoter Haehnchen/idea-php-symfony2-plugin#556

Closed

wouterj force-pushed the cleanup_voter_interface branch 2 times, most recently from 5bb4dd4 to cb37f68 Compare August 9, 2015 13:13

Deprecated supportsAttribute and supportsClass methods

869a937

Taluu reviewed Sep 24, 2015
View reviewed changes

Rewrote logic of AbstractVoter

64fd6bd

wouterj force-pushed the cleanup_voter_interface branch from cb37f68 to 64fd6bd Compare September 24, 2015 14:13

Taluu reviewed Sep 24, 2015
View reviewed changes

wouterj added 2 commits September 25, 2015 09:32

Cleanup AbstractVoter tests

3d4743d

Indicate method will become abstract in 3.0

8567966

carsonbot added the Status: Reviewed label Sep 25, 2015

fabpot closed this in 6f7aae9 Sep 25, 2015

fabpot reopened this Sep 25, 2015

fabpot closed this Sep 25, 2015

wouterj deleted the cleanup_voter_interface branch September 25, 2015 12:13

wouterj mentioned this pull request Sep 26, 2015

[Security] Fix trigger_error calls in voters #15922

Merged

weaverryan mentioned this pull request Sep 26, 2015

Update voter documentation for supportsAttribute and supportsClass deprecation symfony/symfony-docs#5728

Closed

wouterj mentioned this pull request Sep 26, 2015

[Security] Readd the correct tests #15932

Closed

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

wouterj mentioned this pull request Nov 28, 2015

[2.8] Document some Security changes symfony/symfony-docs#5921

Merged

Uh oh!

[Security] Deprecated supportsAttribute and supportsClass methods #15151

[Security] Deprecated supportsAttribute and supportsClass methods #15151

Uh oh!

Conversation

wouterj commented Jun 30, 2015

Abstract Voter

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commented Jul 1, 2015

Uh oh!

stof commented Jul 1, 2015

Uh oh!

soullivaneuh commented Aug 3, 2015

Uh oh!

stof commented Aug 3, 2015

Uh oh!

stof commented Aug 3, 2015

Uh oh!

fabpot commented Aug 3, 2015

Uh oh!

soullivaneuh commented Aug 3, 2015

Uh oh!

wouterj commented Aug 9, 2015

Uh oh!

fabpot commented Sep 22, 2015

Uh oh!

fabpot commented Sep 24, 2015

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!

wouterj commented Sep 24, 2015

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!

xabbuh commented Sep 24, 2015

Uh oh!

wouterj commented Sep 25, 2015

Uh oh!

xabbuh commented Sep 25, 2015

Uh oh!

fabpot commented Sep 25, 2015

Uh oh!

weaverryan commented Sep 26, 2015

Uh oh!

wouterj commented Sep 26, 2015

Uh oh!

Uh oh!