[Security] voters now are going to be obligated to return int values #39248

adeptofvoltron · 2020-11-30T17:45:07Z

Q	A
Branch?	5.x
Bug fix?	yes
New feature?	no
Deprecations?	yes
Tickets	Fix #39205
License	MIT
Doc PR

Everything described with details in the related ticket

derrabus · 2020-11-30T18:00:54Z

We cannot introduce deprecations in 5.2 anymore. This PR should target 5.3.

derrabus

The deprecation should be triggered in prod as well, so TraceableVoter is imho not the right place for this logic. I think we should move it to AccessDecisionManager instead.
in_array() does not perform a type-safe comparison by default! I don't think the current logic would catch the case drafted in the corresponding issue.
I would avoid in_array() altogether because it's inefficient.

adeptofvoltron · 2020-11-30T18:39:52Z

@derrabus I was thinking of usage of is_int however it would not cover all possible scenarios. That why I used the in_array method. However, point taken about the strict comparison. Already repaired.

The fix is in TraceableVoter on purpose. please check the reason for the related issue. In the brief, I wanted to escalate error on dev environments.

PR should target 5.3....actually it is on branch 5.x shall I create a new Upgrade file??

stof · 2020-11-30T18:53:56Z

PR should target 5.3....actually it is on branch 5.x shall I create a new Upgrade file??

yes, as that's the first PR adding a deprecation for 5.3...

src/Symfony/Component/Security/CHANGELOG.md

adeptofvoltron · 2020-11-30T19:00:30Z

@stof thank you. My first PR to Symfony so I am not totally familiar with that community approach. You helped me a lot.
@derrabus if you can now relate to the requested changes. Are they fullfilled?

derrabus · 2020-11-30T19:29:35Z

The fix is in TraceableVoter on purpose. please check the reason for the related issue. In the brief, I wanted to escalate error on dev environments.

Understood. But this would still create different behavior on dev and prod. Right now, a voter can return false or 42 or "1" or null or new DateTime() and AccessDecisionManager would count that as abstain in prod. If we want to be more strict about the return value (and maybe even add the int return type to VoterInterface in 6.0), we should trigger the deprecation even if TraceableVoter is not available.

This is why I think AccessDecisionManager is in fact the better place.

@wouterj, can you join the discussion please? 😃

wouterj · 2020-11-30T19:44:32Z

Hi! Thanks for your 2 PRs, one of them is already merged. Congrats on your first contributions! (and even though this one has some comments, it's really in a good state for a first contribution!)

I agree with @derrabus and I think the confusion here is on the goal of these changes. In my mind, it's not about escalating errors in dev when an incorrect value is returned (in which case it's the TraceableVoter that should do have this check), but it's about being able to simulate the : int return type (we cannot add it, as that would break backwards compatibility IIRC). In order to do that, we should add an \is_int() check in AccessDecisionManager.

In fact, your idea "showing somewhere that an incorrect value is returned by vote()" might be a great idea in just the dev environment (thus in TraceableVoter). But let's not do that in this PR (as it would also require some changes to the security profiler page I think).

adeptofvoltron · 2020-11-30T22:24:18Z

You guys are here maintainers. so it is up to You. I must admit that You both are super patient.

However, adding that deprecation in AccessDecisionManager leads to a situation when in the dev environment you got no error at all (because TraceableVoter will cast the value to integer). And on the production environment, you will get deprecation + app will not work the way developer thought it will. It was my goal at the beginning.

But fine. I will make the changes you desire in 5 minutes.
Thank you @wouterj and @derrabus for help.

wouterj · 2020-11-30T22:26:29Z

Wait, that's an important detail! Now that the : int return type is removed (the changes from 4.4 will be merged up to 5.1, 5.2 & 5.x), the TraceableVoter is no longer type casting it to an int, right?

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

adeptofvoltron · 2020-11-30T22:37:36Z

@wouterj yes you right. I just was not sure when changes from 4.4 will be merged up. I do not know the SF workflow yet.

derrabus · 2020-11-30T22:38:14Z

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

+                @trigger_error(
+                    sprintf('Return value of "%s::vote()" should always be of the type int since Symfony 5.0, %s returned.', \get_class($voter), \gettype($result)),
+                    \E_USER_DEPRECATED
+                );


Suggested change

@trigger_error(

sprintf('Return value of "%s::vote()" should always be of the type int since Symfony 5.0, %s returned.', \get_class($voter), \gettype($result)),

\E_USER_DEPRECATED

);

trigger_deprecation('symfony/security-core', '5.3', 'Returning a value of type "%s" in "%s::vote()" is deprecated, return an int instead.', get_debug_type($result), get_debug_type($voter));

now I am lost. 5.3 is supposed to be for PHP 8.0 only??

We have a Polyfill for get_debug_type(). You can safely use that method on PHP 7 as well.

ah perfect. I allowed myself to write to you on slack. For faster communication

FYI: https://github.com/symfony/polyfill-php80

derrabus · 2020-11-30T22:40:24Z

When triggering the deprecation, we might want to check if we have a TraceableVoter instance and report the decorated class instead of TraceableVoter.

derrabus · 2020-11-30T22:45:26Z

I'm going to merge your change up now, @adeptofvoltron.

derrabus · 2020-11-30T22:52:22Z

There you go. No more return type on 5.x. 😃

jderusse · 2020-11-30T23:29:56Z

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

@@ -119,6 +122,9 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje
        $deny = 0;
        foreach ($this->voters as $voter) {
            $result = $voter->vote($token, $object, $attributes);
+            if (false === \is_int($result)) {


what's about adding a elseif ($result !== VoterInterface::ACCESS_ABSTAIN) below?
It will assert the return value is a valid constant!

The deprecation message would be `return either VoterInterface::ACCESS_GRANTED, VoterInterface::ACCESS_DENIED or VoterInterface::ACCESS_ABSTAIN).

In Symfony 6.0 we will be able to type hint and replace the deprecation by an exception.

adeptofvoltron · 2020-12-05T18:39:11Z

for a week or two, I will have to focus on another project...so I will finish that PR after that.
It is going to be easier for me if I reopen it after that. Do not worry guys I remember all the comments ;)

This PR was merged into the 5.3-dev branch. Discussion ---------- [Security] Assert voter returns valid decision | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | no | Deprecations? | yes | Tickets | Fix #39205 | License | MIT | Doc PR | Replace #39248 Commits ------- ffdfb4f Assert voter returns valid decision

voters now are going to be obligated to return int values

0b3c3f6

adeptofvoltron requested review from chalasr and wouterj as code owners November 30, 2020 17:45

carsonbot added Status: Needs Review Security Bug Deprecation labels Nov 30, 2020

carsonbot changed the title ~~[security] voters now are going to be obligated to return int values~~ [Security] [security] voters now are going to be obligated to return int values Nov 30, 2020

code style

f4e2ab6

adeptofvoltron changed the title ~~[Security] [security] voters now are going to be obligated to return int values~~ [Security] voters now are going to be obligated to return int values Nov 30, 2020

derrabus added this to the 5.x milestone Nov 30, 2020

derrabus requested changes Nov 30, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 30, 2020

type check

d714896

stof reviewed Nov 30, 2020

View reviewed changes

src/Symfony/Component/Security/CHANGELOG.md Outdated Show resolved Hide resolved

moving deprecation to 5.3

27e3166

moving deprecation to 5.3

da6567a

move deprecation to AccessDecisionManager

f3c63c8

nicolas-grekas reviewed Nov 30, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/TraceableVoter.php Outdated Show resolved Hide resolved

Merge branch '5.x' of github.com:symfony/symfony into 5.x

a70aeff

derrabus reviewed Nov 30, 2020

View reviewed changes

adeptofvoltron added 2 commits December 1, 2020 00:00

triggering deprecation

34ace90

Merge branch '5.x' of github.com:symfony/symfony into 5.x

464d7ad

jderusse reviewed Nov 30, 2020

View reviewed changes

adeptofvoltron closed this Dec 5, 2020

jderusse mentioned this pull request Dec 5, 2020

[Security] Assert voter returns valid decision #39340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] voters now are going to be obligated to return int values #39248

[Security] voters now are going to be obligated to return int values #39248

adeptofvoltron commented Nov 30, 2020 •

edited by derrabus

Loading

derrabus commented Nov 30, 2020 •

edited

Loading

derrabus left a comment •

edited

Loading

adeptofvoltron commented Nov 30, 2020

stof commented Nov 30, 2020

adeptofvoltron commented Nov 30, 2020

derrabus commented Nov 30, 2020 •

edited

Loading

wouterj commented Nov 30, 2020

adeptofvoltron commented Nov 30, 2020

wouterj commented Nov 30, 2020

adeptofvoltron commented Nov 30, 2020

derrabus Nov 30, 2020 •

edited

Loading

adeptofvoltron Nov 30, 2020

derrabus Nov 30, 2020

adeptofvoltron Nov 30, 2020 •

edited

Loading

derrabus Nov 30, 2020

derrabus commented Nov 30, 2020

derrabus commented Nov 30, 2020

derrabus commented Nov 30, 2020

jderusse Nov 30, 2020

adeptofvoltron commented Dec 5, 2020

[Security] voters now are going to be obligated to return int values #39248

[Security] voters now are going to be obligated to return int values #39248

Conversation

adeptofvoltron commented Nov 30, 2020 • edited by derrabus Loading

derrabus commented Nov 30, 2020 • edited Loading

derrabus left a comment • edited Loading

Choose a reason for hiding this comment

adeptofvoltron commented Nov 30, 2020

stof commented Nov 30, 2020

adeptofvoltron commented Nov 30, 2020

derrabus commented Nov 30, 2020 • edited Loading

wouterj commented Nov 30, 2020

adeptofvoltron commented Nov 30, 2020

wouterj commented Nov 30, 2020

adeptofvoltron commented Nov 30, 2020

derrabus Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

adeptofvoltron Nov 30, 2020

Choose a reason for hiding this comment

derrabus Nov 30, 2020

Choose a reason for hiding this comment

adeptofvoltron Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

derrabus Nov 30, 2020

Choose a reason for hiding this comment

derrabus commented Nov 30, 2020

derrabus commented Nov 30, 2020

derrabus commented Nov 30, 2020

jderusse Nov 30, 2020

Choose a reason for hiding this comment

adeptofvoltron commented Dec 5, 2020

adeptofvoltron commented Nov 30, 2020 •

edited by derrabus

Loading

derrabus commented Nov 30, 2020 •

edited

Loading

derrabus left a comment •

edited

Loading

derrabus commented Nov 30, 2020 •

edited

Loading

derrabus Nov 30, 2020 •

edited

Loading

adeptofvoltron Nov 30, 2020 •

edited

Loading