[Security] $attributes can be anything, but RoleVoter assumes strings #19725

maennchen · 2016-08-24T15:50:58Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#18042
License	MIT
Doc PR	reference to the documentation PR, if any

linaori · 2016-08-24T16:04:33Z

Considering this has most likely never worked, I think attributes should be hinted as string instead.

maennchen · 2016-08-24T16:08:07Z

@iltar I don't think that this is a good idea.

I have a use case where i want to check if some operations (attributes) can be applied on an object (subject).

For sure those operations could be flattened into a string, but one would have to parse them in the voter again which is total nonsense.

Type Hinting for string would be a BC break since one can supply objects as attributes already, but only if the objects implements a __toString() method.

linaori · 2016-08-24T17:31:35Z

I guess implementing __toString() is already a debatable feature on its own. For the other cases, you have examples of how this would be used? I can't imagine because it's not how it's documented.

Gladhon · 2016-08-25T07:26:11Z

@iltar i think its basically a good idea to allow objects there, cause in that case you can easy check if your voter is responsible or not. Like ExpressionVoter: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/Voter/ExpressionVoter.php#L60

Of course you also can do that with strings, and prefix strings, and hope that there are no conflicts ... but who want this?

maennchen · 2016-08-25T09:13:15Z

@iltar I only implemented the __toString to maintain backwards compatibility. In my opinion I'd rather remove that.

But there are already examples in Symfony itself using non string values. (See @Gladhon's comment) The ExpressionVoter only works because the Expression implements __toString.

hhamon · 2016-08-26T11:45:27Z

I think we should not rely on the __toString() method because you usually want to use it to return a "display" value. For instance, returning Administrators label for an associated ROLE_ADMIN role. The security system requires that any objects to be used as a role must implement the RoleInterface. So the RoleVoter should probably be upgraded to check it the $attribute is an instance of RoleInterface instance, so that you can convert it to a string with its getRole() method.

hhamon · 2016-08-26T11:46:00Z

Otherwise you must implement your custom voter that support your custom type of attribute.

hhamon · 2016-08-26T11:48:15Z

Btw, the border between bug and new feature is really close in this case. I think it's better to consider it a new feature and submit the PR against the master branch.

maennchen · 2016-08-26T11:49:00Z

@hhamon I'd like your proposed solution. I implemented the __toString() thing to avoid BC breaks.

maennchen · 2016-08-26T11:57:30Z

I think that it's definitely a bug since the interface clearly says that mixed is accepted. (Which is currently only if you implement __toString.
Could we implement this as a bug and introduce a new feature for master which cleans the behaviour up?

sstok · 2016-08-26T12:00:58Z

supportsAttribute for RoleVoter should only allow a string or Role(Interface) (Role is actually the only one that is officially supported, because of the string promise).

But when the attributes of a not supported type, the de decision should be abstained?
When passing an unsupported attribute it would have failed with an php error, no?

maennchen · 2016-08-26T12:04:21Z

@sstok Two times yes.

maennchen · 2016-08-26T12:05:18Z

Also supportsAttribute does not exist anymore in >= 3.0 I think.

hhamon · 2016-08-26T12:54:13Z

Yes supportAttributes as a "public" method was removed from the VoterInterface in Symfony 3.0 because this method was never called by the Security component in Symfony 2. But its piece of logic still resides in the RoleVoter class.

maennchen · 2016-09-19T09:18:43Z

Should I change to check if the attribute is instance of RoleInterface instead of __toString? This will be a small BC break.

ro0NL · 2016-09-20T16:51:41Z

@maennchen why exactly do you want to remove __toString support? Imo we should first check for RoleInterface then string/string-object.

That would still be a BC break, but personally i consider it a bugfix.

ro0NL · 2016-09-20T16:53:59Z

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

+        if (is_object($attribute) && !method_exists($attribute, '__toString')) {
+            return false;
+        }
+
        return 0 === strpos($attribute, $this->prefix);


Could be simply (string) $attribtute as well. No need for the above statements... it should crash for invalid values by design.

An attribute is mixed by design. It shouldn't crash for non string values. It should abstain. Why should it be a convention that every object passed as an attribute can be converted to a string.

I implemented a Voter, that was designed to vote for Operations on a subject when I encountered the error. Why should those operations which are not by any means strings have a toString method? That makes no sense for something that is by design mixed.

(By the way: strpos will automatically do a string casting. No string cast needed for that.)

I'd change the code to check if $attribute instanceof RoleInterface, if yes do a getRole() on it. If it is no string or RoleInterface, I'd continue / ABSTAIN.

It shouldn't crash for non string values. It should abstain

Sorry, you're right. However i think for 2.7 supportAttribute should also consider RoleInterface due mixed attribute support.

…mes strings

maennchen · 2016-09-21T07:54:28Z

I refactored it with specific checks for RoleInterface.

I personally don't really care if there is support for objects having a __toString() method. In my opinion it's cleaner just to handle strings & RoleInterface, but with __toString() support there is no BC break.

fabpot · 2016-10-06T01:40:23Z

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

@@ -57,6 +58,10 @@ public function vote(TokenInterface $token, $object, array $attributes)
        $roles = $this->extractRoles($token);

        foreach ($attributes as $attribute) {
+            if ($attribute instanceof RoleInterface) {
+                $attribute = $attribute->getRole();


We should not call supportAttribute if the return value of getRole() is null as it means that there is no good representation of the role as a string.

nevermind, that's already covered in supportsAttribute() via the is_string() check.

fabpot · 2016-10-06T01:42:50Z

Thank you @maennchen.

…mes strings (Jonatan Männchen) This PR was merged into the 2.7 branch. Discussion ---------- [Security] $attributes can be anything, but RoleVoter assumes strings | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18042 | License | MIT | Doc PR | reference to the documentation PR, if any Commits ------- ad3ac95 bug #18042 [Security] $attributes can be anything, but RoleVoter assumes strings

carsonbot added Status: Needs Review Security Bug labels Aug 24, 2016

maennchen mentioned this pull request Aug 24, 2016

[Security] $attributes can be anything, but RoleVoter assumes strings #19726

Closed

ro0NL reviewed Sep 20, 2016

View reviewed changes

backbone87 mentioned this pull request Sep 21, 2016

[Security] Make sure RoleVoter only votes for RoleInterface objects and strings #19965

Closed

bug #18042 [Security] $attributes can be anything, but RoleVoter assu…

ad3ac95

…mes strings

fabpot reviewed Oct 6, 2016

View reviewed changes

fabpot merged commit ad3ac95 into symfony:2.7 Oct 6, 2016

maennchen deleted the hotfix/18042_support_mixed_types branch October 6, 2016 07:59

fabpot mentioned this pull request Oct 27, 2016

Release v2.7.20 #20314

Merged

This was referenced Oct 27, 2016

Release v2.8.13 #20315

Merged

Release v3.1.6 #20316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] $attributes can be anything, but RoleVoter assumes strings #19725

[Security] $attributes can be anything, but RoleVoter assumes strings #19725

maennchen commented Aug 24, 2016 •

edited

Loading

linaori commented Aug 24, 2016

maennchen commented Aug 24, 2016 •

edited

Loading

linaori commented Aug 24, 2016

Gladhon commented Aug 25, 2016

maennchen commented Aug 25, 2016

hhamon commented Aug 26, 2016 •

edited

Loading

hhamon commented Aug 26, 2016

hhamon commented Aug 26, 2016

maennchen commented Aug 26, 2016

maennchen commented Aug 26, 2016

sstok commented Aug 26, 2016

maennchen commented Aug 26, 2016

maennchen commented Aug 26, 2016

hhamon commented Aug 26, 2016 •

edited

Loading

maennchen commented Sep 19, 2016

ro0NL commented Sep 20, 2016

ro0NL Sep 20, 2016

maennchen Sep 21, 2016

maennchen Sep 21, 2016

ro0NL Sep 21, 2016

maennchen commented Sep 21, 2016

fabpot Oct 6, 2016

fabpot Oct 6, 2016

fabpot commented Oct 6, 2016

[Security] $attributes can be anything, but RoleVoter assumes strings #19725

[Security] $attributes can be anything, but RoleVoter assumes strings #19725

Conversation

maennchen commented Aug 24, 2016 • edited Loading

linaori commented Aug 24, 2016

maennchen commented Aug 24, 2016 • edited Loading

linaori commented Aug 24, 2016

Gladhon commented Aug 25, 2016

maennchen commented Aug 25, 2016

hhamon commented Aug 26, 2016 • edited Loading

hhamon commented Aug 26, 2016

hhamon commented Aug 26, 2016

maennchen commented Aug 26, 2016

maennchen commented Aug 26, 2016

sstok commented Aug 26, 2016

maennchen commented Aug 26, 2016

maennchen commented Aug 26, 2016

hhamon commented Aug 26, 2016 • edited Loading

maennchen commented Sep 19, 2016

ro0NL commented Sep 20, 2016

ro0NL Sep 20, 2016

Choose a reason for hiding this comment

maennchen Sep 21, 2016

Choose a reason for hiding this comment

maennchen Sep 21, 2016

Choose a reason for hiding this comment

ro0NL Sep 21, 2016

Choose a reason for hiding this comment

maennchen commented Sep 21, 2016

fabpot Oct 6, 2016

Choose a reason for hiding this comment

fabpot Oct 6, 2016

Choose a reason for hiding this comment

fabpot commented Oct 6, 2016

maennchen commented Aug 24, 2016 •

edited

Loading

maennchen commented Aug 24, 2016 •

edited

Loading

hhamon commented Aug 26, 2016 •

edited

Loading

hhamon commented Aug 26, 2016 •

edited

Loading