8000 [Security] Fixed roles serialization on token from user object by eko · Pull Request #19778 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 22, 2017
Merged

[Security] Fixed roles serialization on token from user object #19778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public function serialize()
array(
is_object($this->user) ? clone $this->user : $this->user,
$this->authenticated,
$this->roles,
array_map(function ($role) { return clone $role; }, $this->roles),
$this->attributes,
)
);
Expand Down
2 changes: 1 addition & 1 deletion .../Component/Security/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related?

Copy link
Contributor Author

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.

$this->assertEquals('foo', $authToken->getCredentials());
$this->assertEquals(array('foo' => 'bar'), $authToken->getAttributes(), '->authenticate() copies token attributes');
}
Expand Down
16 changes: 15 additions & 1 deletion src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Symfony\Component\Security\Core\Authentication\Token\AbstractToken;
use Symfony\Component\Security\Core\Role\Role;
use Symfony\Component\Security\Core\Role\SwitchUserRole;
use Symfony\Component\Security\Core\User\User;

class TestUser
{
Expand Down Expand Up @@ -87,7 +88,7 @@ public function testEraseCredentials()

public function testSerialize()
{
8000 Copy link
Contributor
@ro0NL ro0NL Sep 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about

$user = new User('name', 'password', array('ROLE_FOO', new Role('ROLE_BAR')));
$token = $this->getToken(array('ROLE_FOO2', new Role('ROLE_BAR2')));
$token->setUser($user);

// serialize/unserialize

$this->assertEquals($token->getUser(), $uToken->getUser());

Fully covers AbstractToken::(un)serialize and no need for the test below.

$token = $this->getToken(array('ROLE_FOO'));
$token = $this->getToken(array('ROLE_FOO', new Role('ROLE_BAR')));
$token->setAttributes(array('foo' => 'bar'));

$uToken = unserialize(serialize($token));
Expand All @@ -96,6 +97,19 @@ public function testSerialize()
$this->assertEquals($token->getAttributes(), $uToken->getAttributes());
}

public function testSerializeWithRoleObjects()
Copy link
Contributor
@ro0NL ro0NL Sep 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better update AbstractTokenTest::testSerialize with a 2nd role as object instead of all these duplicated tests.

Copy link
Contributor Author

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 into AbstractTokenTest to avoid adding a lot of tests. Thanks

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not update testSerialize? Ie. L91 => $token = $this->getToken(array('ROLE_FOO', new Role('ROLE_BAR'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ro0NL I've updated the testSerialize 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.

Copy link
Contributor
@ro0NL ro0NL Sep 21, 2016 •
edited
Loading

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 so AbstractToken::serialize() is fully covered.

Copy link
Contributor Author

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 the AbstractToken::__construct() when coming from the user: https://github.com/eko/symfony/blob/dfa7f5020e1274870ee815bb536d37b0e61d8046/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php#L40

{
$user = new User('name', 'password', array(new Role('ROLE_FOO'), new Role('ROLE_BAR')));
$token = new ConcreteToken($user, $user->getRoles());
Copy link
Contributor

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.


$serialized = serialize($token);
$unserialized = unserialize($serialized);

$roles = $unserialized->getRoles();

$this->assertEquals($roles, $user->getRoles());
Copy link
Contributor

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());

}

public function testSerializeParent()
{
$user = new TestUser('fabien');
Expand Down
0