8000 [Component] [Security] [Acl] [Domain] Role security identity fix by henneboele · Pull Request #7791 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Component] [Security] [Acl] [Domain] Role security identity fix #7791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

henneboele
Copy link
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Casting the role property in the constructor is necessary to let the equals function binary comparison work properly while using a custom role entity.

The security identity retrieval strategy getSecurityIdentities function instantiates the role security identity class passing an object. The constructor will assign this object to $this->role in case it's not an instance of Role. Binary safe comparison will fail even though gettype returns "string". The custom role entity requires a __toString magic method in this case.

Casting the role property in the constructor is necessary to
let the equals function binary comparison work properly while
using a custom role entity.

The security identity retrieval strategy getSecurityIdentities
function instantiates the role security identity class passing an
object. The constructor will assign this object to $this->role in case it's not an instance of Role. Binary safe comparison will fail even though gettype returns "string". The custom role entity requires a __toString magic method in this case.
The role security identity was only working when using the origin Role class. The construct method is changed to enable support for custom role entities.
@guilhermeblanco
Copy link
Contributor

This is exactly the same as #8313
OMG... when are we gonna just merge that PR? I'm counting around 15 PRs fixing the same thing over and over for a "claimed to exist" security issue that got never proven. It's not a security issue and I already probed that.

@guilhermeblanco
Copy link
Contributor

@henneboele can you please close your PR in favor of #8313 ?

Also, don't forget to do a +1 there. =)

@stloyd
Copy link
Contributor
stloyd commented Dec 3, 2013

@guilhermeblanco I remember the first one about this =) Yet almost 2,5 year after that one we still don't have real anwser why this can't be merged... =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0