merged branch canni/user_comparable_interface2 (PR #2927)
Commits -------e23d452
Add info about BC Break to CHANGELOG-2.1d7ffeb5
Add some more tests, and enforce boolean return value of interface implementations.9d3a49f
When method name is `hasUserChanged` the return boolean should be true (to match question semantics) and false when user has not changed, this commits inverts return statements.c57b528
Add note about `AdvancedUserInterface`.3682f62
Refactor `isUserChanged` to `hasUserChanged`56db4a1
Change names to Equatable680b108
Suggested fixes ;)9386583
[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. Discussion ---------- [BC Break][Security][Option2] 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: decide about naming [![Build Status](https://secure.travis-ci.org/canni/symfony.png)](http://travis-ci.org/canni/symfony) --------------------------------------------------------------------------- by schmittjoh at 2011-12-19T19:33:24Z This looks much better than the previous PR. Thanks! One thing, we also discussed this on Doctrine, the name "comparable" is used in most programming languages to perform a real compare operation that is ">", "<", or "=". In this case though, we are specifically interested in equality of two objects (we cannot establish a natural order between these objects). Java has no such interface as all objects naturally have an equals() method, .NET uses "Equatable" which looks a bit odd. Not sure if there are better names. --------------------------------------------------------------------------- by canni at 2011-12-19T19:34:52Z I think this is best of "both worlds" we have nice full-featured implementation suitable for most, and if someone needs advanced compare logic just implements interface. @stof @schmittjoh, what do you think? --------------------------------------------------------------------------- by stof at 2011-12-19T19:36:55Z @canni I already commented on the code, and I agree with @schmittjoh that the naming can be confusing --------------------------------------------------------------------------- by jmikola at 2011-12-20T17:33:22Z I don't mean to bikeshed, but I strongly agree with @schmittjoh about implications of "compare". I'm not concerned with the interface name so much as I am with `compareUser()`. Given that this method returns a boolean, I think it's best to prefix it with `is` (e.g. `isSameUser`, `isUserEqualTo`) or `equals` (e.g. `equalsUser`). In this PR, the Token class is implementing the interface, so I think having "User" in the method name is a good idea. Naturally, if the interface was intended for User classes, we could do without it. --------------------------------------------------------------------------- by canni at 2011-12-20T19:00:00Z @jmikola in this PR Token class does not implement any additional interface, and `compareUser` is `private` and used internally. I don't stand still after this names, I'll update PR as soon as some decision about naming will be done. --------------------------------------------------------------------------- by jmikola at 2011-12-21T02:29:59Z @canni: My mistake, I got confused between the Token method and interface method, which you've since renamed in canni/symfony@fcfcd1087b. --------------------------------------------------------------------------- by mvrhov at 2011-12-21T06:09:45Z hm. Now I'm going to bike shed. Wouldn't the proper function name be hasUserChanged? --------------------------------------------------------------------------- by stof at 2011-12-21T10:58:38Z it would probably be bettter. The meaning of ``true`` and ``false`` would then be the opposite of the current ones but this is not an issue IMO as it is a different method --------------------------------------------------------------------------- by jstout24 at 2011-12-27T18:08:49Z @canni nice job --------------------------------------------------------------------------- by fabpot at 2011-12-30T14:59:11Z The method `isUserChanged()` must be rename. What about `hasUserChanged()` as @mvrhov suggested or `isUserDifferent()`? --------------------------------------------------------------------------- by canni at 2012-01-02T11:44:05Z @fabpot done. --------------------------------------------------------------------------- by fabpot at 2012-01-02T18:13:40Z The only missing thing I can think of is adding some unit tests. --------------------------------------------------------------------------- by canni at 2012-01-10T20:16:25Z @fabpot is there anything more you think that should done in this PR? --------------------------------------------------------------------------- by stof at 2012-01-10T20:38:46Z @canni can you rebase your branch ? it conflicts with the current master according to github --------------------------------------------------------------------------- by canni at 2012-01-10T20:56:55Z @stof done. --------------------------------------------------------------------------- by fabpot at 2012-01-12T18:06:00Z @canni: Can you just add some information in the CHANGELOG and in the UPGRADE file? That's all I need to merge this PR now. Thanks a lot. --------------------------------------------------------------------------- by canni at 2012-01-12T18:16:32Z @fabpot done, and no problem :)
This commit is contained in:
commit
741859dc47
@ -79,6 +79,10 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
|
|||||||
toto: { password: foobar, roles: [ROLE_USER] }
|
toto: { password: foobar, roles: [ROLE_USER] }
|
||||||
foo: { password: bar, roles: [ROLE_USER, ROLE_ADMIN] }
|
foo: { password: bar, roles: [ROLE_USER, ROLE_ADMIN] }
|
||||||
|
|
||||||
|
* [BC BREAK] Method `equals` was removed from `UserInterface` to its own new
|
||||||
|
`EquatableInterface`, now user class can implement this interface to override
|
||||||
|
the default implementation of users equality test.
|
||||||
|
|
||||||
* added a validator for the user password
|
* added a validator for the user password
|
||||||
* added 'erase_credentials' as a configuration key (true by default)
|
* added 'erase_credentials' as a configuration key (true by default)
|
||||||
* added new events: `security.authentication.success` and `security.authentication.failure`
|
* added new events: `security.authentication.success` and `security.authentication.failure`
|
||||||
|
@ -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\EquatableInterface;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* 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->hasUserChanged($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 hasUserChanged(UserInterface $user)
|
||||||
|
{
|
||||||
|
if (!($this->user instanceof UserInterface)) {
|
||||||
|
throw new \BadMethodCallException('Method "hasUserChanged" should be called when current user class is instance of "UserInterface".');
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user instanceof EquatableInterface) {
|
||||||
|
return ! (Boolean) $this->user->isEqualTo($user);
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->getPassword() !== $user->getPassword()) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->getSalt() !== $user->getSalt()) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->getUsername() !== $user->getUsername()) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user instanceof AdvancedUserInterface && $user instanceof AdvancedUserInterface) {
|
||||||
|
if ($this->user->isAccountNonExpired() !== $user->isAccountNonExpired()) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->isAccountNonLocked() !== $user->isAccountNonLocked()) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->isCredentialsNonExpired() !== $user->isCredentialsNonExpired()) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($this->user->isEnabled() !== $user->isEnabled()) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
} elseif ($this->user instanceof AdvancedUserInterface xor $user instanceof AdvancedUserInterface) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
return false;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
@ -0,0 +1,37 @@
|
|||||||
|
<?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;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* EquatableInterface used to test if two objects are equal in security
|
||||||
|
* and re-authentication context.
|
||||||
|
*
|
||||||
|
* @author Dariusz Górecki <darek.krk@gmail.com>
|
||||||
|
*/
|
||||||
|
interface EquatableInterface
|
||||||
|
{
|
||||||
|
/**
|
||||||
|
* 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.
|
||||||
|
*
|
||||||
|
* Also implementation should consider that $user instance may implement
|
||||||
|
* the extended user interface `AdvancedUserInterface`.
|
||||||
|
*
|
||||||
|
* @param UserInterface $user
|
||||||
|
*
|
||||||
|
* @return Boolean
|
||||||
|
*/
|
||||||
|
function isEqualTo(UserInterface $user);
|
||||||
|
}
|
@ -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,13 +144,10 @@ 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
|
$advancedUser = $this->getMock('Symfony\Component\Security\Core\User\AdvancedUserInterface');
|
||||||
->expects($this->any())
|
|
||||||
->method('equals')
|
|
||||||
->will($this->returnValue(true))
|
|
||||||
;
|
|
||||||
|
|
||||||
return array(
|
return array(
|
||||||
|
array($advancedUser),
|
||||||
array($user),
|
array($user),
|
||||||
array(new TestUser('foo')),
|
array(new TestUser('foo')),
|
||||||
array('foo'),
|
array('foo'),
|
||||||
@ -176,11 +173,7 @@ 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
|
$advancedUser = $this->getMock('Symfony\Component\Security\Core\User\AdvancedUserInterface');
|
||||||
->expects($this->any())
|
|
||||||
->method('equals')
|
|
||||||
->will($this->returnValue(false))
|
|
||||||
;
|
|
||||||
|
|
||||||
return array(
|
return array(
|
||||||
array(
|
array(
|
||||||
@ -193,14 +186,20 @@ class AbstractTokenTest extends \PHPUnit_Framework_TestCase
|
|||||||
'foo', $user,
|
'foo', $user,
|
||||||
),
|
),
|
||||||
array(
|
array(
|
||||||
$user, $user,
|
'foo', $advancedUser
|
||||||
),
|
),
|
||||||
array(
|
array(
|
||||||
$user, 'foo'
|
$user, 'foo'
|
||||||
),
|
),
|
||||||
|
array(
|
||||||
|
$advancedUser, 'foo'
|
||||||
|
),
|
||||||
array(
|
array(
|
||||||
$user, new TestUser('foo'),
|
$user, new TestUser('foo'),
|
||||||
),
|
),
|
||||||
|
array(
|
||||||
|
$advancedUser, new TestUser('foo'),
|
||||||
|
),
|
||||||
array(
|
array(
|
||||||
new TestUser('foo'), new TestUser('bar'),
|
new TestUser('foo'), new TestUser('bar'),
|
||||||
),
|
),
|
||||||
@ -210,6 +209,15 @@ class AbstractTokenTest extends \PHPUnit_Framework_TestCase
|
|||||||
array(
|
array(
|
||||||
new TestUser('foo'), $user,
|
new TestUser('foo'), $user,
|
||||||
),
|
),
|
||||||
|
array(
|
||||||
|
new TestUser('foo'), $advancedUser,
|
||||||
|
),
|
||||||
|
array(
|
||||||
|
$user, $advancedUser
|
||||||
|
),
|
||||||
|
array(
|
||||||
|
$advancedUser, $user
|
||||||
|
),
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -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