[BC Break][Security] Moved user comparsion logic out of UserInterface
As discussed on IRC meetings and in PR #2669 I came up with implementation. This is option2, I think more elegant. BC break: yes Feature addition: no/feature move Symfony2 test pass: yes Symfony2 test written: yes Todo: feedback needed
This commit is contained in:
parent
009e6d739e
commit
9386583b19
@ -14,6 +14,8 @@ namespace Symfony\Component\Security\Core\Authentication\Token;
|
|||||||
use Symfony\Component\Security\Core\Role\RoleInterface;
|
use Symfony\Component\Security\Core\Role\RoleInterface;
|
||||||
use Symfony\Component\Security\Core\Role\Role;
|
use Symfony\Component\Security\Core\Role\Role;
|
||||||
use Symfony\Component\Security\Core\User\UserInterface;
|
use Symfony\Component\Security\Core\User\UserInterface;
|
||||||
|
use Symfony\Component\Security\Core\User\AdvancedUserInterface;
|
||||||
|
use Symfony\Component\Security\Core\User\ComparableInterface;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Base class for Token instances.
|
* Base class for Token instances.
|
||||||
@ -87,7 +89,7 @@ abstract class AbstractToken implements TokenInterface
|
|||||||
if (!$user instanceof UserInterface) {
|
if (!$user instanceof UserInterface) {
|
||||||
$changed = true;
|
$changed = true;
|
||||||
} else {
|
} else {
|
||||||
$changed = !$this->user->equals($user);
|
$changed = !$this->compareUser($user);
|
||||||
}
|
}
|
||||||
} elseif ($user instanceof UserInterface) {
|
} elseif ($user instanceof UserInterface) {
|
||||||
$changed = true;
|
$changed = true;
|
||||||
@ -220,4 +222,49 @@ abstract class AbstractToken implements TokenInterface
|
|||||||
|
|
||||||
return sprintf('%s(user="%s", authenticated=%s, roles="%s")', $class, $this->getUsername(), json_encode($this->authenticated), implode(', ', $roles));
|
return sprintf('%s(user="%s", authenticated=%s, roles="%s")', $class, $this->getUsername(), json_encode($this->authenticated), implode(', ', $roles));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private function compareUser(UserInterface $user)
|
||||||
|
{
|
||||||
|
if (!($this->user instanceof UserInterface)) {
|
||||||
|
throw new \BadMethodCallException('Method "compareUser" should be called when current user class is instance of "UserInterface".');
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user instanceof ComparableInterface) {
|
||||||
|
return $this->user->compareTo($user);
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->getPassword() !== $user->getPassword()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->getSalt() !== $user->getSalt()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->getUsername() !== $user->getUsername()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user instanceof AdvancedUserInterface && $user instanceof AdvancedUserInterface) {
|
||||||
|
if ($this->user->isAccountNonExpired() !== $user->isAccountNonExpired()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->isAccountNonLocked() !== $user->isAccountNonLocked()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->isCredentialsNonExpired() !== $user->isCredentialsNonExpired()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->isEnabled() !== $user->isEnabled()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
} elseif ($this->user instanceof AdvancedUserInterface xor $user instanceof AdvancedUserInterface) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
return true;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
@ -0,0 +1,32 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
/*
|
||||||
|
* This file is part of the Symfony package.
|
||||||
|
*
|
||||||
|
* (c) Fabien Potencier <fabien@symfony.com>
|
||||||
|
*
|
||||||
|
* For the full copyright and license information, please view the LICENSE
|
||||||
|
* file that was distributed with this source code.
|
||||||
|
*/
|
||||||
|
|
||||||
|
namespace Symfony\Component\Security\Core\User;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* ComparatorInterface used to test if two object are equal in security
|
||||||
|
* and re-authentication context.
|
||||||
|
*
|
||||||
|
* @author Dariusz Górecki <darek.krk@gmail.com>
|
||||||
|
*/
|
||||||
|
interface ComparableInterface
|
||||||
|
{
|
||||||
|
/**
|
||||||
|
* The equality comparison should neither be done by referential equality
|
||||||
|
* nor by comparing identities (i.e. getId() === getId()).
|
||||||
|
*
|
||||||
|
* However, you do not need to compare every attribute, but only those that
|
||||||
|
* are relevant for assessing whether re-authentication is required.
|
||||||
|
*
|
||||||
|
* @return boolean
|
||||||
|
*/
|
||||||
|
public function compareTo($object);
|
||||||
|
}
|
@ -112,44 +112,4 @@ final class User implements AdvancedUserInterface
|
|||||||
public function eraseCredentials()
|
public function eraseCredentials()
|
||||||
{
|
{
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* {@inheritDoc}
|
|
||||||
*/
|
|
||||||
public function equals(UserInterface $user)
|
|
||||||
{
|
|
||||||
if (!$user instanceof User) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if ($this->password !== $user->getPassword()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if ($this->getSalt() !== $user->getSalt()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if ($this->username !== $user->getUsername()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if ($this->accountNonExpired !== $user->isAccountNonExpired()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if ($this->accountNonLocked !== $user->isAccountNonLocked()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if ($this->credentialsNonExpired !== $user->isCredentialsNonExpired()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if ($this->enabled !== $user->isEnabled()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
@ -84,19 +84,4 @@ interface UserInterface
|
|||||||
* @return void
|
* @return void
|
||||||
*/
|
*/
|
||||||
function eraseCredentials();
|
function eraseCredentials();
|
||||||
|
|
||||||
/**
|
|
||||||
* Returns whether or not the given user is equivalent to *this* user.
|
|
||||||
*
|
|
||||||
* The equality comparison should neither be done by referential equality
|
|
||||||
* nor by comparing identities (i.e. getId() === getId()).
|
|
||||||
*
|
|
||||||
* However, you do not need to compare every attribute, but only those that
|
|
||||||
* are relevant for assessing whether re-authentication is required.
|
|
||||||
*
|
|
||||||
* @param UserInterface $user
|
|
||||||
*
|
|
||||||
* @return Boolean
|
|
||||||
*/
|
|
||||||
function equals(UserInterface $user);
|
|
||||||
}
|
}
|
||||||
|
@ -144,11 +144,6 @@ class AbstractTokenTest extends \PHPUnit_Framework_TestCase
|
|||||||
public function getUsers()
|
public function getUsers()
|
||||||
{
|
{
|
||||||
$user = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
|
$user = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
|
||||||
$user
|
|
||||||
->expects($this->any())
|
|
||||||
->method('equals')
|
|
||||||
->will($this->returnValue(true))
|
|
||||||
;
|
|
||||||
|
|
||||||
return array(
|
return array(
|
||||||
array($user),
|
array($user),
|
||||||
@ -176,11 +171,6 @@ class AbstractTokenTest extends \PHPUnit_Framework_TestCase
|
|||||||
public function getUserChanges()
|
public function getUserChanges()
|
||||||
{
|
{
|
||||||
$user = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
|
$user = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
|
||||||
$user
|
|
||||||
->expects($this->any())
|
|
||||||
->method('equals')
|
|
||||||
->will($this->returnValue(false))
|
|
||||||
;
|
|
||||||
|
|
||||||
return array(
|
return array(
|
||||||
array(
|
array(
|
||||||
@ -192,9 +182,6 @@ class AbstractTokenTest extends \PHPUnit_Framework_TestCase
|
|||||||
array(
|
array(
|
||||||
'foo', $user,
|
'foo', $user,
|
||||||
),
|
),
|
||||||
array(
|
|
||||||
$user, $user,
|
|
||||||
),
|
|
||||||
array(
|
array(
|
||||||
$user, 'foo'
|
$user, 'foo'
|
||||||
),
|
),
|
||||||
|
@ -123,22 +123,4 @@ class UserTest extends \PHPUnit_Framework_TestCase
|
|||||||
$user->eraseCredentials();
|
$user->eraseCredentials();
|
||||||
$this->assertEquals('superpass', $user->getPassword());
|
$this->assertEquals('superpass', $user->getPassword());
|
||||||
}
|
}
|
||||||
|
|
||||||
public function testUsersAreEqual()
|
|
||||||
{
|
|
||||||
$user1 = new User('fabien', 'superpass', array('ROLE_USER'));
|
|
||||||
$user2 = clone $user1;
|
|
||||||
|
|
||||||
$this->assertTrue($user1->equals($user2));
|
|
||||||
$this->assertTrue($user2->equals($user1));
|
|
||||||
}
|
|
||||||
|
|
||||||
public function testUsersAreNotEqual()
|
|
||||||
{
|
|
||||||
$user1 = new User('fabien', 'superpass', array('ROLE_USER'));
|
|
||||||
$user2 = new User('fabien', 'superpass', array('ROLE_USER'), false);
|
|
||||||
|
|
||||||
$this->assertFalse($user1->equals($user2));
|
|
||||||
$this->assertFalse($user2->equals($user1));
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
Reference in New Issue
Block a user