Migrated SF AdvancedUserInterface to FOS UserInterface #2815

devtronic · 2018-07-07T10:55:15Z

AdvancedUserInterface is deprecated since Symfony 4.1

Issues:

Deprecation with Symfony 4.1 - AdvancedUserInterface #2803 Deprecation with Symfony 4.1 - AdvancedUserInterface
Deprecation with Symfony 4.1 - AbstractToken::hasUserChanged #2802 Deprecation with Symfony 4.1 - AbstractToken::hasUserChanged

AdvancedUserInterface is deprecated since Symfony 4.1 - symfony/symfony#23508 Issue: - #2803 Deprecation with Symfony 4.1 - AdvancedUserInterface

…ion`

giansalex · 2018-07-09T14:16:55Z

use git commit --allow-empty instead code changes to restart travis

devtronic · 2018-07-09T14:31:38Z

Thanks. Learning never stops. ;-)

…to `User`

XWB · 2018-07-20T10:15:30Z

Would this introduce a BC break, @stof ?

XWB · 2018-08-03T16:02:21Z

Tests/Mailer/MailerTest.php

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


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

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

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

…' into Issue_2803_AdvancedUserInterface

back-2-95 · 2018-08-07T06:50:55Z

What is the status of this? Summer vacations?

devtronic · 2018-08-07T08:09:52Z

Any ideas why this test always fails or how to solve this problem? https://travis-ci.org/FriendsOfSymfony/FOSUserBundle/jobs/412998483

XWB · 2018-08-07T11:29:42Z

@devtronic Should be fixed by d41c5a9

XWB · 2018-08-07T11:31:05Z

@back-2-95 We still need to find a solution for the BC break.

devtronic · 2018-08-07T11:39:29Z

@XWB Which BC break? As far as I can see, there should be no BC break

XWB · 2018-08-07T11:45:31Z

@devtronic Everyone that checks its user model against the AdvancedUserInterface will end up with a broken app.

devtronic · 2018-08-07T12:25:37Z

Maybe something like this? (untested)

<?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\Model;

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

interface FosUserInterface extends \Serializable {
{
    const ROLE_DEFAULT = 'ROLE_USER';

    const ROLE_SUPER_ADMIN = 'ROLE_SUPER_ADMIN';
	
    /**
     * Returns the user unique id.
     *
     * @return mixed
     */
    public function getId();
	
}

if(interface_exists(\Symfony\Component\Security\Core\User\AdvancedUserInterface::class){

	/**
	 * @author Thibault Duplessis <[email protected]>
	 * @author Johannes M. Schmitt <[email protected]>
	 */
	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


		/**
		 * Sets the username.
		 *
		 * @param string $username
		 *
		 * @return static
		 */
		public function setUsername($username);

		/**
		 * Gets the canonical username in search and sort queries.
		 *
		 * @return string
		 */
		public function getUsernameCanonical();

		/**
		 * Sets the canonical username.
		 *
		 * @param string $usernameCanonical
		 *
		 * @return static
		 */
		public function setUsernameCanonical($usernameCanonical);

		/**
		 * @param string|null $salt
		 *
		 * @return static
		 */
		public function setSalt($salt);

		/**
		 * Gets email.
		 *
		 * @return string
		 */
		public function getEmail();

		/**
		 * Sets the email.
		 *
		 * @param string $email
		 *
		 * @return static
		 */
		public function setEmail($email);

		/**
		 * Gets the canonical email in search and sort queries.
		 *
		 * @return string
		 */
		public function getEmailCanonical();

		/**
		 * Sets the canonical email.
		 *
		 * @param string $emailCanonical
		 *
		 * @return static
		 */
		public function setEmailCanonical($emailCanonical);

		/**
		 * Gets the plain password.
		 *
		 * @return string
		 */
		public function getPlainPassword();

		/**
		 * Sets the plain password.
		 *
		 * @param string $password
		 *
		 * @return static
		 */
		public function setPlainPassword($password);

		/**
		 * Sets the hashed password.
		 *
		 * @param string $password
		 *
		 * @return static
		 */
		public function setPassword($password);

		/**
		 * Tells if the the given user has the super admin role.
		 *
		 * @return bool
		 */
		public function isSuperAdmin();

		/**
		 * @param bool $boolean
		 *
		 * @return static
		 */
		public function setEnabled($boolean);

		/**
		 * Sets the super admin status.
		 *
		 * @param bool $boolean
		 *
		 * @return static
		 */
		public function setSuperAdmin($boolean);

		/**
		 * Gets the confirmation token.
		 *
		 * @return string|null
		 */
		public function getConfirmationToken();

		/**
		 * Sets the confirmation token.
		 *
		 * @param string|null $confirmationToken
		 *
		 * @return static
		 */
		public function setConfirmationToken($confirmationToken);

		/**
		 * Sets the timestamp that the user requested a password reset.
		 *
		 * @param null|\DateTime $date
		 *
		 * @return static
		 */
		public function setPasswordRequestedAt(\DateTime $date = null);

		/**
		 * Checks whether the password reset request has expired.
		 *
		 * @param int $ttl Requests older than this many seconds will be considered expired
		 *
		 * @return bool
		 */
		public function isPasswordRequestNonExpired($ttl);

		/**
		 * Sets the last login time.
		 *
		 * @param \DateTime|null $time
		 *
		 * @return static
		 */
		public function setLastLogin(\DateTime $time = null);

		/**
		 * Never use this to check if this user has access to anything!
		 *
		 * Use the AuthorizationChecker, or an implementation of AccessDecisionManager
		 * instead, e.g.
		 *
		 *         $authorizationChecker->isGranted('ROLE_USER');
		 *
		 * @param string $role
		 *
		 * @return bool
		 */
		public function hasRole($role);

		/**
		 * Sets the roles of the user.
		 *
		 * This overwrites any previous roles.
		 *
		 * @param array $roles
		 *
		 * @return static
		 */
		public function setRoles(array $roles);

		/**
		 * Adds a role to the user.
		 *
		 * @param string $role
		 *
		 * @return static
		 */
		public function addRole($role);

		/**
		 * Removes a role to the user.
		 *
		 * @param string $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();

		/**
		 * 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();
	}
}

Move Travis memory limit

…' into Issue_2803_AdvancedUserInterface

devtronic · 2018-08-22T09:12:17Z

@XWB can you check my last changes, please? The changes should be now comaptible to >=SF 4.4 (with AdvancedUserInterface) and >=SF 5

XWB · 2018-08-22T10:00:20Z

Model/UserInterface.php

+}
+
+// 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')) {


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

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?

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.

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

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

@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?

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

stof · 2018-08-22T10:50:46Z

Resources/config/security.xml

@@ -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" />


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

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)

stof · 2018-08-22T10:51:42Z

Security/UserChecker.php

+     */
+    public function checkPreAuth(BaseUserInterface $user)
+    {
+        if (!$user->isAccountNonLocked()) {


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

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

stof · 2018-08-22T10:52:29Z

Model/UserInterface.php


 /**
- * @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)


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

stof · 2018-08-22T10:53:17Z

Model/UserInterface.php

+     *
+     * @see AccountExpiredException
+     */
+    public function isAccountNonExpired();


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)

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

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.

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

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

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

@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)

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

DonCallisto · 2018-09-10T07:38:31Z

Model/User.php

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


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

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

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.

DonCallisto · 2018-09-10T08:08:09Z

Hi, what is missing here to accept the PR?

XWB · 2018-09-19T15:01:13Z

Thanks @devtronic

dimitrilahaye · 2018-09-21T10:02:42Z

Hello, I've upgraded my package to the 2.1.2 but still have my UserInterface interface instead of the waited FosUserInterface. Do you know what could be wrong ?
I specificaly asked for composer require friendsofsymfony/user-bundle:2.1.2 to be sure to have the good version.

devtronic · 2018-09-21T10:28:17Z

The bundle must be backcompatible to old versions. So in <SF5 the bundle uses the UserInterface and from SF5 the FosUserInterface

2.1.2 was released in March: https://github.com/FriendsOfSymfony/FOSUserBundle/releases/tag/v2.1.2

dimitrilahaye · 2018-09-21T11:27:52Z

Ok I understand, thanks for the answer. So, as we are still on 4* in my office, we will still have the Calling "Symfony\Component\Security\Core\User\UserChecker::checkPreAuth()" error on client test ?

jpegram2 · 2018-11-28T12:59:28Z

Just out of curiosity what is SF5 you refer to? There is no Symfony 5 on the roadmap, just 4.2.

DonCallisto · 2018-11-28T13:56:36Z

@jpegram2 as symfony relase policy is that next major version is x.4 without deprecations, if you're compliant with x.4 you'll be also with next major.

jpegram2 · 2018-11-28T14:07:46Z

Got it, thanks. On another note, how do you replace this with a custom user checker. The doc says it should autowire but I still get deprecation notices in the log where it's trying to use the old one.

DonCallisto · 2018-11-28T14:09:30Z

@jpegram2 maybe related to #2815 (comment) ?

Migrated SF AdvancedUserInterface to FOS UserInterface

017a2bb

AdvancedUserInterface is deprecated since Symfony 4.1 - symfony/symfony#23508 Issue: - #2803 Deprecation with Symfony 4.1 - AdvancedUserInterface

devtronic mentioned this pull request Jul 7, 2018

Deprecation with Symfony 4.1 - AdvancedUserInterface #2803

Closed

devtronic and others added 2 commits July 7, 2018 13:16

Code style fixed and using getMockBuilder instead of createMock

f4d3275

Code style fixed and using attributes instead of `$this->expectExcept…

340db13

…ion`

devtronic mentioned this pull request Jul 9, 2018

Forbid Inactive Users (AdvancedUserInterface) #2816

Closed

Change to restart travis

e6fcbfe

devtronic mentioned this pull request Jul 9, 2018

Deprecation with Symfony 4.1 - AbstractToken::hasUserChanged #2802

Closed

devtronic changed the title ~~Migrated SF AdvancedUserInterface to FOS UserInterface~~ [WIP] Migrated SF AdvancedUserInterface to FOS UserInterface Jul 9, 2018

EquatableInterface added to UserInterface and implementation added …

ddc0b08

…to `User`

devtronic changed the title ~~[WIP] Migrated SF AdvancedUserInterface to FOS UserInterface~~ Migrated SF AdvancedUserInterface to FOS UserInterface Jul 9, 2018

devtronic and others added 2 commits August 3, 2018 15:21

Merge branch 'master' into Issue_2803_AdvancedUserInterface

6beb423

Tests after merge of master fixed

41ab05c

XWB requested changes Aug 3, 2018

View reviewed changes

devtronic added 2 commits August 3, 2018 20:38

Tests after merge of master fixed

6512e71

Merge remote-tracking branch 'origin/Issue_2803_AdvancedUserInterface…

e594ae5

…' into Issue_2803_AdvancedUserInterface

Update README.md

450b2d4

devtronic and others added 4 commits August 8, 2018 12:24

Merge pull request #1 from FriendsOfSymfony/master

1f10a7f

Move Travis memory limit

Added compatibility for apps that check against AdvancedUserInterface

815f35c

Merge remote-tracking branch 'origin/Issue_2803_AdvancedUserInterface…

d02f9c7

…' into Issue_2803_AdvancedUserInterface

Code style fixed to pass all travis tests

3fd0e97

XWB reviewed Aug 22, 2018

View reviewed changes

stof reviewed Aug 22, 2018

View reviewed changes

fos_user.user_checker Service marked as non-public

6b8d62b

DonCallisto reviewed Sep 10, 2018

View reviewed changes

XWB approved these changes Sep 19, 2018

View reviewed changes

XWB merged commit b93d732 into FriendsOfSymfony:master Sep 19, 2018

devtronic deleted the Issue_2803_AdvancedUserInterface branch September 21, 2018 10:28

devtronic mentioned this pull request May 28, 2019

[Feature Request] 'non-advanced' FosUser? #2911

Closed

Migrated SF AdvancedUserInterface to FOS UserInterface #2815

Migrated SF AdvancedUserInterface to FOS UserInterface #2815

Conversation

devtronic commented Jul 7, 2018 • edited Loading

giansalex commented Jul 9, 2018 • edited Loading

devtronic commented Jul 9, 2018

XWB commented Jul 20, 2018

Choose a reason for hiding this comment

devtronic Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

back-2-95 commented Aug 7, 2018

devtronic commented Aug 7, 2018

XWB commented Aug 7, 2018

XWB commented Aug 7, 2018

devtronic commented Aug 7, 2018

XWB commented Aug 7, 2018

devtronic commented Aug 7, 2018 • edited Loading

devtronic commented Aug 22, 2018 • edited Loading

XWB Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XWB Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonCallisto Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devtronic Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonCallisto commented Sep 10, 2018

XWB commented Sep 19, 2018

dimitrilahaye commented Sep 21, 2018

devtronic commented Sep 21, 2018

dimitrilahaye commented Sep 21, 2018

jpegram2 commented Nov 28, 2018

DonCallisto commented Nov 28, 2018

jpegram2 commented Nov 28, 2018

DonCallisto commented Nov 28, 2018

devtronic commented Jul 7, 2018 •

edited

Loading

giansalex commented Jul 9, 2018 •

edited

Loading

devtronic Aug 3, 2018 •

edited

Loading

devtronic commented Aug 7, 2018 •

edited

Loading

devtronic commented Aug 22, 2018 •

edited

Loading

XWB Aug 22, 2018 •

edited

Loading

XWB Aug 22, 2018 •

edited

Loading

DonCallisto Dec 18, 2018 •

edited

Loading

devtronic Sep 10, 2018 •

edited

Loading