Skip to content
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

Migrated SF AdvancedUserInterface to FOS UserInterface #2815

Merged
merged 15 commits into from
Sep 19, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
25 changes: 25 additions & 0 deletions Model/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Symfony\Component\Security\Core\User\UserInterface as BaseUserInterface;

/**
* Storage agnostic user object.
Expand Down Expand Up @@ -554,4 +555,28 @@ public function removeGroup(GroupInterface $group)

return $this;
}

/**
* {@inheritdoc}
*/
public function isEqualTo(BaseUserInterface $user)

Choose a reason for hiding this comment

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

I'm totally missing the point here. What this method suppose to do and where is used/useful?

Copy link
Author

@devtronic devtronic Sep 10, 2018

Choose a reason for hiding this comment

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

The FOS UserInterface extends from EquatableInterface
https://api.symfony.com/master/Symfony/Component/Security/Core/User/EquatableInterface.html
afaik this is required for user switching and re-authentication

Choose a reason for hiding this comment

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

afaik this is required for user switching and re-authentication

I'm not totally sure of this but I really don't know it properly. Just wondering why AdvantageUserInterface didn't extends that interface aswell so that's the reason of my question.

{
if (!$user instanceof self) {
return false;
}

if ($this->password !== $user->getPassword()) {
return false;
}

if ($this->salt !== $user->getSalt()) {
return false;
}

if ($this->username !== $user->getUsername()) {
return false;
}

return true;
}
}
78 changes: 74 additions & 4 deletions Model/UserInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@

namespace FOS\UserBundle\Model;

use Symfony\Component\Security\Core\User\AdvancedUserInterface;
use Symfony\Component\Security\Core\User\EquatableInterface;
use Symfony\Component\Security\Core\User\UserInterface as BaseUserInterface;

/**
* @author Thibault Duplessis <[email protected]>
* @author Johannes M. Schmitt <[email protected]>
* @internal Only for back compatibility Remove / Merge with UserInterface in Nov 2023 (End of support for security fixes SF 4.4)
Copy link
Member

Choose a reason for hiding this comment

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

I would say remove merge when dropping support for Symfony 4 instead

*/
interface UserInterface extends AdvancedUserInterface, \Serializable
interface FosUserInterface extends \Serializable
{
const ROLE_DEFAULT = 'ROLE_USER';

Expand Down Expand Up @@ -227,4 +227,74 @@ public function addRole($role);
* @return static
*/
public function removeRole($role);

/**
* Checks whether the user's account has expired.
*
* Internally, if this method returns false, the authentication system
* will throw an AccountExpiredException and prevent login.
*
* @return bool true if the user's account is non expired, false otherwise
*
* @see AccountExpiredException
*/
public function isAccountNonExpired();
Copy link
Member

Choose a reason for hiding this comment

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

  • 1 for having these methods in the FOSUserBundle interface if we don't do anything useful with them (i.e. we should only have isEnabled)

Copy link
Author

Choose a reason for hiding this comment

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

Same as https://github.com/FriendsOfSymfony/FOSUserBundle/pull/2815/files/3fd0e9789ba08602b5873a164cfe5e61a8ebfd68#diff-fead1d2e8961c005678d0ae0758f1bac

If we remove this method, some people can get in trouble ;-)
This method is used in FOS\UserBundle\Security\UserChecker::checkPreAuth()

Choose a reason for hiding this comment

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

That's true as long as compatibility is granted. When dropping sf4 support, maybe, this should be removed and users should, for example, implement own UserChecker as decorator of this if they want to rely on that field. I know that removing things from interfaces is always seen as a bad thing but here we're introducing something just to avoid breaking the logic of the framework.

Choose a reason for hiding this comment

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

Hi @DonCallisto , I don't understand why introducing account locking in "FOS user" is such a shame? Does this bundle not manage user account? perhaps this is a missing feature from the bundle and it could permit to be fully compliant with next major symfony version and even avoid BC break...

Copy link

@DonCallisto DonCallisto Dec 18, 2018

Choose a reason for hiding this comment

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

@yonisolo see https://symfony.com/blog/new-in-symfony-4-1-deprecated-the-advanceduserinterface

I suppose we provided it only as a a "compatibility feature" for symfony. Don't know if it wise to keep it, instead in a new FOSUserBundle version we should show users how to implement this feature on their own if the need to keep compatibility.

Choose a reason for hiding this comment

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

Yes i saw that deprecation and the link to create a custom user checker but preauth check for account status could be implemented as an interface to provide compatibility and with a basic usage which could be overridden later by the user for its own business logic. Presently, if FOSUserBundle remove this functionality, most user will be lost between the Symfony security component and the FOsUser User management, please don't break this chain...

Choose a reason for hiding this comment

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

@yonisolo No one proposed to remove UserChecker that's basically the "link" you're talking about. As @stof says, isAccountNonExpired (both on UserChecker and UserInterface) should not be part of this bundle as are not FOSUserBundle concept (IIUC) but the UserChecker as well as UserInterface will still be part of this bundle (otherwise it will be pretty useless)

Choose a reason for hiding this comment

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

ah ok, my mistakes, sorry!! I understand better, thank you!!


/**
* Checks whether the user is locked.
*
* Internally, if this method returns false, the authentication system
* will throw a LockedException and prevent login.
*
* @return bool true if the user is not locked, false otherwise
*
* @see LockedException
*/
public function isAccountNonLocked();

/**
* Checks whether the user's credentials (password) has expired.
*
* Internally, if this method returns false, the authentication system
* will throw a CredentialsExpiredException and prevent login.
*
* @return bool true if the user's credentials are non expired, false otherwise
*
* @see CredentialsExpiredException
*/
public function isCredentialsNonExpired();

/**
* Checks whether the user is enabled.
*
* Internally, if this method returns false, the authentication system
* will throw a DisabledException and prevent login.
*
* @return bool true if the user is enabled, false otherwise
*
* @see DisabledException
*/
public function isEnabled();
}

// This is required to support apps that explicitly check if a user is an instance of AdvancedUserInterface
if (interface_exists('\Symfony\Component\Security\Core\User\AdvancedUserInterface')) {
Copy link
Member

@XWB XWB Aug 22, 2018

Choose a reason for hiding this comment

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

This will always return true in Symfony 4.x as the interface was just deprecated and not removed.

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's true, but if we do that way, the user bundle is compatible to SF5.

Do you have an alternative proposal how we can solve this problem?

Copy link

Choose a reason for hiding this comment

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

Solution can be:

use Symfony\Component\HttpKernel\Kernel;

if ( Kernel::VERSION_ID >= 40100 ){
}

Not sure if this is the best we can do, but it's working and nobody receive deprecation notices.

Copy link
Member

Choose a reason for hiding this comment

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

that won't work fine, as it will detect the version of the wrong component

Choose a reason for hiding this comment

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

@XWB probably I'm missing the point but this looks fine to me as long as this version is compliant with sf4 AND sf5.

Copy link
Member

Choose a reason for hiding this comment

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

@DonCallisto Well, my point was that extending the AdvancedUserInterface will still throw a deprecation notice in Symfony 4.1 and above. I wonder how we can avoid that deprecation notice?

Choose a reason for hiding this comment

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

That's impossible unless you create a new major version, still compatible with sf4, and explain that AdvantageUserInterface is now not available anymore (so, basically, you're introducing a BC but keeping the minimum compatibility).

/**
* @author Thibault Duplessis <[email protected]>
* @author Johannes M. Schmitt <[email protected]>
*
* @deprecated since Symfony 4.1. Remove in Nov 2023 (End of support for security fixes SF 4.4)
*/
interface UserInterface extends FosUserInterface, \Symfony\Component\Security\Core\User\AdvancedUserInterface
{
}
} else {
/**
* @author Thibault Duplessis <[email protected]>
* @author Johannes M. Schmitt <[email protected]>
* @author Julian Finkler <[email protected]>
*/
interface UserInterface extends FosUserInterface, BaseUserInterface, EquatableInterface
{
}
}
2 changes: 2 additions & 0 deletions Resources/config/security.xml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
</service>

<service id="FOS\UserBundle\Controller\SecurityController" alias="fos_user.security.controller" public="true" />

<service id="fos_user.user_checker" class="FOS\UserBundle\Security\UserChecker" public="true" />
Copy link
Member

Choose a reason for hiding this comment

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

no need to make it public, as it is meant to be referenced in the SecurityBundle config, which supports private services

Choose a reason for hiding this comment

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

Moreover I would use FQCN as id in order to avoid aliasing this for bundle users that would like to take advantage of autowire. (i.e.: create a decorator for this user checker as I've suggested above)

</services>

</container>
1 change: 1 addition & 0 deletions Resources/doc/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ in your application:
firewalls:
main:
pattern: ^/
user_checker: fos_user.user_checker
form_login:
provider: fos_userbundle
csrf_token_generator: security.csrf.token_manager
Expand Down
63 changes: 63 additions & 0 deletions Security/UserChecker.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

/*
* This file is part of the FOSUserBundle package.
*
* (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace FOS\UserBundle\Security;

use Symfony\Component\Security\Core\Exception\AccountExpiredException;
use Symfony\Component\Security\Core\Exception\CredentialsExpiredException;
use Symfony\Component\Security\Core\Exception\DisabledException;
use Symfony\Component\Security\Core\Exception\LockedException;
use Symfony\Component\Security\Core\User\UserChecker as BaseUserChecker;
use Symfony\Component\Security\Core\User\UserInterface as BaseUserInterface;

/**
* UserChecker checks the user account flags.
*
* @author Julian Finkler (Devtronic) <[email protected]>
*/
class UserChecker extends BaseUserChecker
{
/**
* {@inheritdoc}
*/
public function checkPreAuth(BaseUserInterface $user)
{
if (!$user->isAccountNonLocked()) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO, the FOSUserBundle checker should check only the methods that belong to FOSUserBundle (account locking is not part of FOSUserBundle)

Copy link
Member

@XWB XWB Aug 22, 2018

Choose a reason for hiding this comment

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

The issue is that some people might be using these methods. Removing it would break their application.

$ex = new LockedException('User account is locked.');
$ex->setUser($user);
throw $ex;
}

if (!$user->isEnabled()) {
$ex = new DisabledException('User account is disabled.');
$ex->setUser($user);
throw $ex;
}

if (!$user->isAccountNonExpired()) {
$ex = new AccountExpiredException('User account has expired.');
$ex->setUser($user);
throw $ex;
}
}

/**
* {@inheritdoc}
*/
public function checkPostAuth(BaseUserInterface $user)
{
if (!$user->isCredentialsNonExpired()) {
$ex = new CredentialsExpiredException('User credentials have expired.');
$ex->setUser($user);
throw $ex;
}
}
}
3 changes: 1 addition & 2 deletions Tests/Mailer/MailerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

use FOS\UserBundle\Mailer\Mailer;
use PHPUnit\Framework\TestCase;
use Swift_Events_EventDispatcher;
use Swift_Mailer;
use Swift_Transport_NullTransport;

Expand Down Expand Up @@ -84,7 +83,7 @@ private function getMailer()
return new Mailer(
new Swift_Mailer(
new Swift_Transport_NullTransport(
$this->getMockBuilder(Swift_Events_EventDispatcher::class)->getMock()
$this->getMockBuilder('\Swift_Events_EventDispatcher')->getMock()
Copy link
Member

Choose a reason for hiding this comment

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

This change is not required. Travis currently fails due to a memory error, not because of this.

Copy link
Author

@devtronic devtronic Aug 3, 2018

Choose a reason for hiding this comment

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

It is required, because this this test fails: https://travis-ci.org/FriendsOfSymfony/FOSUserBundle/jobs/411741460

Only this test fails due to a memory error: https://travis-ci.org/FriendsOfSymfony/FOSUserBundle/jobs/411741470

Copy link
Member

Choose a reason for hiding this comment

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

Which is quite strange because the test already passed on master branch.

)
),
$this->getMockBuilder('Symfony\Component\Routing\Generator\UrlGeneratorInterface')->getMock(),
Expand Down
19 changes: 19 additions & 0 deletions Tests/Model/UserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,25 @@ public function testFalseHasRole()
$this->assertTrue($user->hasRole($newrole));
}

public function testIsEqualTo()
{
$user = $this->getUser();
$this->assertTrue($user->isEqualTo($user));
$this->assertFalse($user->isEqualTo($this->getMockBuilder('FOS\UserBundle\Model\UserInterface')->getMock()));

$user2 = $this->getUser();
$user2->setPassword('secret');
$this->assertFalse($user->isEqualTo($user2));

$user3 = $this->getUser();
$user3->setSalt('pepper');
$this->assertFalse($user->isEqualTo($user3));

$user4 = $this->getUser();
$user4->setUsername('f00b4r');
$this->assertFalse($user->isEqualTo($user4));
}

/**
* @return User
*/
Expand Down
105 changes: 105 additions & 0 deletions Tests/Security/UserCheckerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
<?php

/*
* This file is part of the FOSUserBundle package.
*
* (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace FOS\UserBundle\Tests\Security;

use FOS\UserBundle\Security\UserChecker;
use PHPUnit\Framework\TestCase;

class UserCheckerTest extends TestCase
{
/**
* @expectedException \Symfony\Component\Security\Core\Exception\LockedException
* @expectedExceptionMessage User account is locked.
*/
public function testCheckPreAuthFailsLockedOut()
{
$userMock = $this->getUser(false, false, false, false);
$checker = new UserChecker();
$checker->checkPreAuth($userMock);
}

/**
* @expectedException \Symfony\Component\Security\Core\Exception\DisabledException
* @expectedExceptionMessage User account is disabled.
*/
public function testCheckPreAuthFailsIsEnabled()
{
$userMock = $this->getUser(true, false, false, false);
$checker = new UserChecker();
$checker->checkPreAuth($userMock);
}

/**
* @expectedException \Symfony\Component\Security\Core\Exception\AccountExpiredException
* @expectedExceptionMessage User account has expired.
*/
public function testCheckPreAuthFailsIsAccountNonExpired()
{
$userMock = $this->getUser(true, true, false, false);
$checker = new UserChecker();
$checker->checkPreAuth($userMock);
}

public function testCheckPreAuthSuccess()
{
$userMock = $this->getUser(true, true, true, false);
$checker = new UserChecker();

try {
$this->assertNull($checker->checkPreAuth($userMock));
} catch (\Exception $ex) {
$this->fail();
}
}

/**
* @expectedException \Symfony\Component\Security\Core\Exception\CredentialsExpiredException
* @expectedExceptionMessage User credentials have expired.
*/
public function testCheckPostAuthFailsIsCredentialsNonExpired()
{
$userMock = $this->getUser(true, true, true, false);
$checker = new UserChecker();
$checker->checkPostAuth($userMock);
}

public function testCheckPostAuthSuccess()
{
$userMock = $this->getUser(true, true, true, true);
$checker = new UserChecker();

try {
$this->assertNull($checker->checkPostAuth($userMock));
} catch (\Exception $ex) {
$this->fail();
}
}

private function getUser($isAccountNonLocked, $isEnabled, $isAccountNonExpired, $isCredentialsNonExpired)
{
$userMock = $this->getMockBuilder('FOS\UserBundle\Model\User')->getMock();
$userMock
->method('isAccountNonLocked')
->willReturn($isAccountNonLocked);
$userMock
->method('isEnabled')
->willReturn($isEnabled);
$userMock
->method('isAccountNonExpired')
->willReturn($isAccountNonExpired);
$userMock
->method('isCredentialsNonExpired')
->willReturn($isCredentialsNonExpired);

return $userMock;
}
}