@schmittjoh can you verify this change?
@fabpot feel free to merge once reviewed.

This come back like boomerang =) i.e.#5909

@stloyd but the problem is that ALL others are closed as "duplicate" of another one, and none is actually opened to be considered. So closing this one means we'll be back to no PR opened related to this.

@stloyd Actually,#5076 is opened, but it doesn't contain any tests, quite different from this one. =)
I'd suggest closing#5076 and accepting this if possible.

According toaf70ac8@schmittjoh considered this change as a security vulnerability.
I don't see how it can be a security vulnerability since both default implementation AND interface based checking stores both information on DB in case of an ORM based solution.
According to default implementation, it stores the Role as a list inside of User (as part of a project Johannes is also part ofFOSUserBundle and any separate implementation can't be different).
In my company's situation we extend a base class and we correctly implement the RoleInterface, which should provide the contract for Role implementations, but instead it checks for Role class which I cannot extend (PHP does not support multiple inheritance) and my entire ACL fails because it always try to compare a string to an Object (instance of my implemented RoleInterface).

So, I can't see how@schmittjoh understands that it is a security vulnerability, but I'd love if he can provide a clear explanation why is it considered like that. Otherwise, it seems that all those 10 PRs that were already highlighted before are valid. Finally, if it is indeed a security vulnerability, there's no need to provide a contract for extensibility (interface) if implementors are unable to correctly consume the API. It just seems non-sense.

Additionally, if there is indeed a security vulnerability here, it would be great to get a test case to cover that situation so that we can ensure that other changes don't trigger similar flaws.

I remember talking with Fabien about this. I think a better approach is to override the SecurityIdentityRetrievalStrategy if you need this.

Regarding the security vulnerability, this does not necessarily introduce something, but depending on the assumptions that you make in your system, especially if you use different role classes, it could. That's why I'd prefer to not make this change.

@schmittjoh Please, could you elaborate more about the security vulnerability? We have 12 tickets about the same subject, one is 2 years old and unfortunately I couldn't find the vulnerability explanation.

A viable option may be to override theSecurityIdentityRetrievalStrategy however IMHO the best approach is to use the interface.

If the decision is to close and do not accept this pull request wemust remove theRoleInterface from the project, since it's leading to confusion and the only place the interface is being used besides on docblocks is onsrc/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php:

foreach ($roles as $role) {    if (is_string($role)) {        $role = new Role($role);    } elseif (!$role instanceof RoleInterface) {        throw new \InvalidArgumentException(sprintf('$roles must be an array of strings, or RoleInterface instances, but got %s.', gettype($role)));    }    $this->roles[] = $role;}

With the actual constructor we know that thisinstanceof can be replaced forinstaceof Role, then we can rename all@param on docblocks toRole since there is not support to custom roles.

The latter is also something that I suggested, and removing the entire
Role/RoleInterface in favor of simple strings is preferable imo.

On Thu, Jun 20, 2013 at 3:19 PM, Danilo Cabellonotifications@github.comwrote:

@schmittjohhttps://github.com/schmittjoh Please, could you elaborate
more about the security vulnerability? We have 12 tickets about the same
subject, one is 2 years old and unfortunately I couldn't find the
vulnerability explanation.

A viable option may be to override the SecurityIdentityRetrievalStrategyhowever IMHO the best approach is to use the interface.

If the decision is to close and do not accept this pull request we _must_remove the
RoleInterface from the project, since it's leading to confusion and the
only place the interface is being used besides on docblocks is on
src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
:

foreach ($roles as $role) {
if (is_string($role)) {
$role = new Role($role);
} elseif (!$role instanceof RoleInterface) {
throw new \InvalidArgumentException(sprintf('$roles must be an array of strings, or RoleInterface instances, but got %s.', gettype($role)));
}

$this->roles[] = $role;

}

With the actual constructor we know that this instanceof can be replaced
for instaceof Role, then we can rename all@param on docblocks to Rolesince there is not support to custom roles.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/8313#issuecomment-19751622
.

@schmittjoh That doesn't mean anything to me. I could extend Role class then and do whatever I want.
As a consumer, you can always make stupid assumptions. I can write aSecurityIdentityRetrievalStrategy thatequals() always return true. Another security vulnerability, according to your concerns. Based on this, then we should also prevent to implement a different strategy.
That's why I feel this enforcement is non-sense. You provide a point to extend, but then you completely prevent the contract concerned about possible implementations.
You always open a "security vulnerability". Either on a custom Strategy implementation or on a Role implementation.

• If you want to force people to implement their own Strategy, then we need to remove the RoleInterface, mark Role class as final, update where RoleInterface is checked and update documentation about implementing custom ACL Roles.
• If you want to prevent people from doing mistakes by accepting all Role comparisons (like return true onequals()), then kill support for Strategy override.

That's a more lengthy explanation why I think this concern is invalid and the patch should be merged.

@fabpot can you provide some feedback on this?
I don't like to maintain a fork of symfony internally for too long.

Any news on this one?

nanananananananana batman!

👍 for merging... A developer can always introduce a security vulnerability, by any means. I recommendeval().

@aderuwe I tried onceeval("rm -Rf /"); as soon as I changed the apache user to be root instead of www-data. It does work as expected. =)

@guilhermeblanco I don't thinkeval() is to blame for you trying that. ;)

All things considered, I think that (as is the case with theeval() documentation) it should be sufficient to clearly indicate the possibility of disaster when mucking with this, but allow the functionality none the less. The number of PRs this has attracted is silly, it's clearly a sought-after feature. It happens to come with possible side-effects, but those can be documented.

it needs some rebasing

@cordoval What do you mean? Are you trying to use our fork (instaclick/symfony) and is it missing the latest changes from symfony?

@cabello currently, it has conflicts when we try to merge this. That means that this branch is not up-to-date with master and that there is some commit editing in the same files as you did, causing conflicts.

What you need to do is fetching an up-to-date branch from this repo and then rebase your branch against it (and it'll always be better if you create a new branch when working on a PR).

Assume this repo is called "origin" and your repo "cabello", you need to execute these commands:

# fetch latest branches from this repo$ git fetch origin# rebase your master$ git rebase origin/master# ... fix conflicts (you need to do that in your editor)# after you fixed the conflicts, run:$ git rebase --continue# when finished, push to your repo (-f is required here)$ git push -f cabello master

Sorry that it took so long but let me explain why this PR was never merged and why it cannot be merged as is. First, you must know that I definitely want to fix this issue for 2.5 but you will see in a minute that it is not that simple.

Everything is actually rather well explained in the phpdocs ofRoleInterface: aRoleInterface represents a role granted to the user. But more interesting is the long description:

"A role must either have a string representation or it needs to be explicitly supported by at least one AccessDecisionManager."

Note theor in the sentence. Basically, a Role doesnot necessarily have a string representation: "When the role cannot be represented with sufficient precision by a string, it should return null."

Switching to the interface as it's done here has several consequences:

• we force all Role classes to have a string representation (that limits the usefulness of the interface);
• we remove the difference between roles with a string representation and other ones. Note that in the interface docs, it is mentioned that non-string roles must beexplicitly supported.

That's basically why this PR cannot be merged as is, and why technically it does not cover all cases and can even introduce security issues. Now, we have several options:

• Acknowledge that nobody ever used the possibility to have a role without a string representation (or a Role class with a special behavior that cannot be represented as a string). In that case, let's deprecated the interface and switch to role as strings everywhere (we would need to find another way to deal with the switch user role but this can probably be managed by the Token itself).
• Keep the current behavior for Roles. Then, find a way to actually store the role class/object in the ACL system.

Personally, I'm more in favor of the first option for several reasons:

• removing features that are barely used is always a good idea (especially when it can simplify code);
• it helps with performance and it's cleaner (storing a string or an object in a DB is very different);
• it also helps with the serialization of the User in the session.

Of course,RoleInterface and all classes implementing this interface would be just deprecated in 2.5 and removed only in 3.0. I haven't had a look at the code to see all consequences, but that should be easily manageable in a BC way (and we have 6 months to get it right ;)).

I'm also in favor of option 1.

This would not prevent using a toMany relation to store roles in a database if someone wants to do it. It simply means that thegetRoles method would have to convert the Doctrine Collection of objects to an array of string (with the way the user wants).

Is there a good way to ask the community how much they rely on this feature, to be sure that we are not just assuming it is barely used based on our own projects ?

you could do a simple doodlehttp://doodle.com/vycrabazihq926yq and tweet it 👶 but of course asking about this feature

@fabpot I'll comment the merits of your alternate options on another comment, but I'll stick to the long description ofRoleInterface to describe why this patch is still valid.

A role must either have a string representation or it needs to be explicitly supported by at least one AccessDecisionManager.

In my specific situation, I have a Role that represents an entity in our DB. It does implement__toString() method, following the requirement of first part ofRoleInterface as described. But the problem is that it isnot enough, becauseRoleSecurityIdentity restrict to have a derived implementation ofRole or itself, and notRoleInterface as long explanation of that class describes.
Right now, you only have 1 implementation ofRole that is used, and you can only have 1 because ofRoleSecurityIdentity restriction.

Of course you could suggest me to extendRole and implement my DB information at the top of it, but you're limiting possibilites of having a Role that already extends another class (my case). This makes your consequence section still ok because you only need to add__toString implementation toRole and enforceRoleInterface contract to have implementors implement toString too.

This means we still need to update a bit the code, but solution is still valid from this perspective. Next comment will bring comments over your suggested alternate solutions.

@fabpot I'll detail each suggested alternate approach and then provide my choice.

Acknowledge that nobody ever used the possibility to have a role without a string representation (or a Role class with a special behavior that cannot be represented as a string). In that case, let's deprecated the interface and switch to role as strings everywhere (we would need to find another way to deal with the switch user role but this can probably be managed by the Token itself).

This would bring some headache and possibly a huge BC break, but it is the best solution, by far. =)
The problem relies on existing implementors of Role (like me), that relies not only on this patch proposed here, but also on Role class as being an many-to-many association on DB. My permissions (acl_classes) are permission strings that are coupled to roles (relying on the existing table). I completely ignored masks because of its limitations too. That means migrating from 2.4 to 2.5 would require small-medium code updates on my side (trivial, but still a couple dozen files).

Keep the current behavior for Roles. Then, find a way to actually store the role class/object in the ACL system.

Taking this solution, my previously explained suggestion would address this issue nicely, unless you are considering efforts around storing the roles in DB too. I wouldn't take this approach as it heavily increases the amount of DB calls (my internal implementation does that) and I had to customize implementation by caching the Roles elsewhere.
I'd stick to the simpler way if we want to reduce the effort from userland to migrate to 2.5 and relax the implementation as I suggested before.

I can't remember correctly why I had to go for Roles stored in a DB, but it was a mid-size limitation that was preventing me to use Roles as-is. If I have to choose, option 1 is the best one, but I am uncertain if my implementation would still work.
I still consider in the meantime this patch valid as I've explained previously, and get it as a bug fix for 2.4 until 2.5 is in the oven. That would also allow me to not keep my 6 months old fork purely because of this change.

Why are you closing this PR?

well, you commented 5 months ago saying it was the wrong approach

That's basically why this PR cannot be merged as is, and why technically it does not cover all cases and can even introduce security issues.

• It won't be merged as is.
• It was never explained the security issues this PR would introduce here nor in any of the twelve tickets mentioned on this PR.
• It's a year old and the first ticket requesting this same change is 3 years old.
• Yesterday Travis bot mentioned this PR is failing now.
• We changed the way we represent roles in our application so we don't have to maintain a fork of Symfony.

We are currently storing our roles in the database, because the end user can put some permissions on the roles, that's why string-only roles are not sufficient for us.

Then I hit the issue being resolved in this PR. For now I could also inherit from theRole-class, but is that a sustainable solution? Will this class be removed in a future version of symfony?