Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Security] Fixed roles serialization on token from user object#19778
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
69a588f tof5f7021Compare| @@ -46,7 +46,7 @@ public function __construct(array $roles = array()) | |||
| throw new \InvalidArgumentException(sprintf('$roles must be an array of strings, or RoleInterface instances, but got %s.', gettype($role))); | |||
| } | |||
| $this->roles[] = $role; | |||
| $this->roles[] =clone$role; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not do the cloning inserialize() like the user object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL Because in theserialize() method, we have$this->roles which is an array, not an object (cannot be cloned).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Understand.. i also meant: clone the array and each object in it.array_map(function($role) { return clone $role; }, $roles)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL Alright, this is updated according to your comment.
| @@ -220,7 +220,7 @@ public function testAuthenticateWithPreservingRoleSwitchUserRole() | |||
| $this->assertInstanceOf('Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken', $authToken); | |||
| $this->assertSame($user, $authToken->getUser()); | |||
| $this->assertContains(new Role('ROLE_FOO'), $authToken->getRoles(), '', false, false); | |||
| $this->assertContains($switchUserRole, $authToken->getRoles()); | |||
| $this->assertContains($switchUserRole, $authToken->getRoles(), '', false, false); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this related?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL Yes, it is related, test failed without that change because we checked an exact reference. Check has to be changed like the line just before.
| @@ -96,6 +97,19 @@ public function testSerialize() | |||
| $this->assertEquals($token->getAttributes(), $uToken->getAttributes()); | |||
| } | |||
| public function testSerializeWithRoleObjects() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Better updateAbstractTokenTest::testSerialize with a 2nd role as object instead of all these duplicated tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL I agree with you on this comment, I will remove all of these tests and just let one intoAbstractTokenTest to avoid adding a lot of tests. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is done, PR is up-to-date with comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not updatetestSerialize? Ie.L91 =>$token = $this->getToken(array('ROLE_FOO', new Role('ROLE_BAR'));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL I've updated thetestSerialize test with a second role that is from user entity (even if it were already working in this case).
The issue here was when the roles are the same reference as the ones in the user entity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Your change is covered by the test above now.
The issue here was when the roles are the same reference as the ones in the user entity.
Im not sure how that relates to your change... what about adding a$token->setUser($mockedUser) to above test soAbstractToken::serialize() is fully covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL Roles are passed in theAbstractToken::__construct() when coming from the user:https://github.com/eko/symfony/blob/dfa7f5020e1274870ee815bb536d37b0e61d8046/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php#L40
94266ed tof21fb26Comparef21fb26 todfa7f50Compare@ro0NL Thank you for taking time to review this PR, is it ok for you? |
| @@ -87,7 +88,7 @@ public function testEraseCredentials() | |||
| public function testSerialize() | |||
| { | |||
| $token = $this->getToken(array('ROLE_FOO')); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about
$user =newUser('name','password',array('ROLE_FOO',newRole('ROLE_BAR')));$token =$this->getToken(array('ROLE_FOO2',newRole('ROLE_BAR2')));$token->setUser($user);// serialize/unserialize$this->assertEquals($token->getUser(),$uToken->getUser());
Fully coversAbstractToken::(un)serialize and no need for the test below.
| public function testSerializeWithRoleObjects() | ||
| { | ||
| $user = new User('name', 'password', array(new Role('ROLE_FOO'), new Role('ROLE_BAR'))); | ||
| $token = new ConcreteToken($user, $user->getRoles()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Point is; passing roles here is effectively the same as passing in roles via$this->getToken($roles).. like above.
| $roles = $unserialized->getRoles(); | ||
| $this->assertEquals($roles, $user->getRoles()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same test as above, ie.$this->assertEquals($token->getRoles(), $uToken->getRoles());
Still think tests can be combined, but other then that this is good 👍 Status: Reviewed |
Thank you@eko. |
…ject (eko)This PR was merged into the 2.7 branch.Discussion----------[Security] Fixed roles serialization on token from user object| Q | A || --- | --- || Branch? | 2.7 || Bug fix? | yes || New feature? | no || BC breaks? | no || Deprecations? | no || Tests pass? | yes || Fixed tickets |#14274 || License | MIT || Doc PR | - |This PR fixes the serialization of tokens when using `Role` objects provided from the user. Indeed, there were actually a reference issue that can causes fatal errors like the following one:```FatalErrorException in RoleHierarchy.php line 43:Error: Call to a member function getRole() on string```Here is a small code example to reproduce and its output:``` php$user = new Symfony\Component\Security\Core\User\User('name', 'password', [ new Symfony\Component\Security\Core\Role\Role('name')]);$token = new Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken($user, 'password', 'providerKey', $user->getRoles());$serialized = serialize($token);$unserialized = unserialize($serialized);var_dump($unserialized->getRoles());```Before:```array(1) { [0]=> bool(true) }```After:```array(1) { [0]=> object(Symfony\Component\Security\Core\Role\Role)#15 (1) {["role":"Symfony\Component\Security\Core\Role\Role":private]=> string(4) "name" } }```Thank youCommits-------dfa7f50 [Security] Fixed roles serialization on token from user object
Uh oh!
There was an error while loading.Please reload this page.
This PR fixes the serialization of tokens when using
Roleobjects provided from the user. Indeed, there were actually a reference issue that can causes fatal errors like the following one:Here is a small code example to reproduce and its output:
Before:
After:
Thank you