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

Deprecated the AdvancedUserInterface #23508

Merged
merged 1 commit into from Feb 4, 2018

Conversation

@iltar
Contributor

iltar commented Jul 14, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #23292
License MIT
Doc PR ~

This PR deprecates the usages of the AdvancedUserInterface.

Show outdated Hide outdated UPGRADE-3.4.md
Security
--------
* Using the AdvancedUserInterface is now deprecated. To use the existing

This comment has been minimized.

@xabbuh

xabbuh Jul 14, 2017

Member

AdvancedUserInterface should be enclosed with backticks

@xabbuh

xabbuh Jul 14, 2017

Member

AdvancedUserInterface should be enclosed with backticks

Show outdated Hide outdated UPGRADE-3.4.md
--------
* Using the AdvancedUserInterface is now deprecated. To use the existing
functionality, create a custom user-checker based off the

This comment has been minimized.

@xabbuh

xabbuh Jul 14, 2017

Member

based on

@xabbuh

xabbuh Jul 14, 2017

Member

based on

Show outdated Hide outdated UPGRADE-3.4.md
removed in Symfony 4.0.
* The in-memory user no longer uses the `AdvancedUserInterface` and has the
functionality inlined.

This comment has been minimized.

@ro0NL

ro0NL Jul 14, 2017

Contributor

isnt this a BC break?

@ro0NL

ro0NL Jul 14, 2017

Contributor

isnt this a BC break?

Show outdated Hide outdated UPGRADE-4.0.md
@@ -475,6 +475,14 @@ Security
* The `AccessDecisionManager::setVoters()` method has been removed. Pass the
voters to the constructor instead.
* Using the AdvancedUserInterface is now deprecated. To use the existing

This comment has been minimized.

@ro0NL

ro0NL Jul 14, 2017

Contributor

in 4.0 the AdvancedUserInterface would be removed right?

@ro0NL

ro0NL Jul 14, 2017

Contributor

in 4.0 the AdvancedUserInterface would be removed right?

Show outdated Hide outdated src/Symfony/Component/Security/Core/User/AdvancedUserInterface.php
@@ -32,6 +32,7 @@
*
* @see UserInterface
* @see AccountStatusException
* @deprecated This interface is deprecated as of 3.4 and will be removed in 4.0.

This comment has been minimized.

@ro0NL

ro0NL Jul 14, 2017

Contributor

@deprecated since 3.4, to be removed in 4.0 something like that i believe. Also missing a runtime deprecation? i.e. trigger_error

@ro0NL

ro0NL Jul 14, 2017

Contributor

@deprecated since 3.4, to be removed in 4.0 something like that i believe. Also missing a runtime deprecation? i.e. trigger_error

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

@deprecated since version 3.4, will be removed in 4.0.
(but no runtime notice because that'd trigger it for existing classes in core)

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

@deprecated since version 3.4, will be removed in 4.0.
(but no runtime notice because that'd trigger it for existing classes in core)

Show outdated Hide outdated src/Symfony/Component/Security/Core/User/UserInterface.php
@@ -47,7 +46,7 @@
* and populated in any number of different ways when the user object
* is created.
*
* @return (Role|string)[] The user roles
* @return Role|string[] The user roles

This comment has been minimized.

@ro0NL

ro0NL Jul 14, 2017

Contributor

Should be reverted, it's intended as PSR-5 doc

@ro0NL

ro0NL Jul 14, 2017

Contributor

Should be reverted, it's intended as PSR-5 doc

This comment has been minimized.

@iltar

iltar Jul 14, 2017

Contributor

Hmm PhpStorm did not recognize this notation and PSR-5 is still not finished

@iltar

iltar Jul 14, 2017

Contributor

Hmm PhpStorm did not recognize this notation and PSR-5 is still not finished

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be Role[]|string[] then I guess?

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be Role[]|string[] then I guess?

This comment has been minimized.

@ro0NL

ro0NL Jul 17, 2017

Contributor

¯_(ツ)_/¯ technically that's an array of either strings or roles, whereas PSR-5 allows a mixed array.

@ro0NL

ro0NL Jul 17, 2017

Contributor

¯_(ツ)_/¯ technically that's an array of either strings or roles, whereas PSR-5 allows a mixed array.

This comment has been minimized.

@jvasseur

jvasseur Jul 27, 2017

Contributor

It should stay (Role|string)[] it's an array that can contains a mix of both Role objects and strings

@jvasseur

jvasseur Jul 27, 2017

Contributor

It should stay (Role|string)[] it's an array that can contains a mix of both Role objects and strings

}
/**
* @group legacy

This comment has been minimized.

@ro0NL

ro0NL Jul 14, 2017

Contributor

missing @expectedDeprecation?

@ro0NL

ro0NL Jul 14, 2017

Contributor

missing @expectedDeprecation?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 15, 2017

Contributor

Btw if we deprecate AdvancedUserInterace, i dont see the need to inline those methods into User (as a side effect); we could deprecate those in turn as well. Probably allows to cleanup further :)

Contributor

ro0NL commented Jul 15, 2017

Btw if we deprecate AdvancedUserInterace, i dont see the need to inline those methods into User (as a side effect); we could deprecate those in turn as well. Probably allows to cleanup further :)

Show outdated Hide outdated src/Symfony/Component/Security/Core/User/User.php
@@ -18,7 +18,7 @@
*
* @author Fabien Potencier <fabien@symfony.com>
*/
final class User implements AdvancedUserInterface
final class User implements UserInterface, EquatableInterface

This comment has been minimized.

@greg0ire

greg0ire Jul 15, 2017

Contributor

Consider this instead;

final class User implements UserInterface, EquatableInterface, AdvancedUserInterface

It should work just fine : https://3v4l.org/eRO4P, and avoids a BC-break for code that does this: $user instanceof AdvancedUserInterface

@greg0ire

greg0ire Jul 15, 2017

Contributor

Consider this instead;

final class User implements UserInterface, EquatableInterface, AdvancedUserInterface

It should work just fine : https://3v4l.org/eRO4P, and avoids a BC-break for code that does this: $user instanceof AdvancedUserInterface

This comment has been minimized.

@iltar

iltar Jul 15, 2017

Contributor

Yes, that's what I've been thinking off as well after a discussion with @ro0NL on slack. On one side it feels like this behavior shouldn't be used too often, but I also know that if something can happen, it will happen.

I just need to find a way to not trigger a deprecation for that.

@iltar

iltar Jul 15, 2017

Contributor

Yes, that's what I've been thinking off as well after a discussion with @ro0NL on slack. On one side it feels like this behavior shouldn't be used too often, but I also know that if something can happen, it will happen.

I just need to find a way to not trigger a deprecation for that.

This comment has been minimized.

@greg0ire

greg0ire Jul 15, 2017

Contributor

I just need to find a way to not trigger a deprecation for that.

In the user checker? I guess you could check the classname before triggering, and if it is the core user class, don't trigger.

@greg0ire

greg0ire Jul 15, 2017

Contributor

I just need to find a way to not trigger a deprecation for that.

In the user checker? I guess you could check the classname before triggering, and if it is the core user class, don't trigger.

This comment has been minimized.

@ro0NL

ro0NL Jul 15, 2017

Contributor

Looking at RoleInterface; i think soft deprecating is fine. Implementing EquatableInterface is a new feature really. Perhaps simply revert this line?

@ro0NL

ro0NL Jul 15, 2017

Contributor

Looking at RoleInterface; i think soft deprecating is fine. Implementing EquatableInterface is a new feature really. Perhaps simply revert this line?

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jul 15, 2017

Contributor

@ro0NL They are already in the User, so it's merely making them a class only set of functions. I was thinking if we could remove those fields and limit it to a disabled/enabled, but this would influence the messages to the user based on the getMessageKey.

Contributor

iltar commented Jul 15, 2017

@ro0NL They are already in the User, so it's merely making them a class only set of functions. I was thinking if we could remove those fields and limit it to a disabled/enabled, but this would influence the messages to the user based on the getMessageKey.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jul 15, 2017

Contributor

Side note, I was thinking of opening an RFC to rename the User to InMemoryUser, as it seems too generic right now. This would also allow cleanup of all those methods.

Contributor

iltar commented Jul 15, 2017

Side note, I was thinking of opening an RFC to rename the User to InMemoryUser, as it seems too generic right now. This would also allow cleanup of all those methods.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 15, 2017

Contributor

I just need to find a way to not trigger a deprecation for that.

rename the User to InMemoryUser

Deprecate User, add InMemoryUser... problem solved?

Contributor

ro0NL commented Jul 15, 2017

I just need to find a way to not trigger a deprecation for that.

rename the User to InMemoryUser

Deprecate User, add InMemoryUser... problem solved?

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jul 15, 2017

Contributor

That could make the deprecations of User complete and simply use a smaller InMemory user anyway, Might solve more issues

Contributor

iltar commented Jul 15, 2017

That could make the deprecations of User complete and simply use a smaller InMemory user anyway, Might solve more issues

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 15, 2017

Member

Please, don't deprecate User by InMemoryUser. It's a really bad name. Remember that Symfony should do the normal simple and the custom possible. The normal is to have a simple user in the database and represent it with a User entity. Thanks!

Member

javiereguiluz commented Jul 15, 2017

Please, don't deprecate User by InMemoryUser. It's a really bad name. Remember that Symfony should do the normal simple and the custom possible. The normal is to have a simple user in the database and represent it with a User entity. Thanks!

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jul 15, 2017

Contributor

@javiereguiluz the User is the InMemoryUser and should only be used for the in memory provider. Sadly, due to the generic name, this is often mistaken as "the" user class.

Contributor

iltar commented Jul 15, 2017

@javiereguiluz the User is the InMemoryUser and should only be used for the in memory provider. Sadly, due to the generic name, this is often mistaken as "the" user class.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 15, 2017

Member

@iltar I'm sorry. I should have checked twice before commenting (I was on the phone). If this is the non-persisted user, then InMemoryUser is a perfect name because it's used in other frameworks/technologies too.

Member

javiereguiluz commented Jul 15, 2017

@iltar I'm sorry. I should have checked twice before commenting (I was on the phone). If this is the non-persisted user, then InMemoryUser is a perfect name because it's used in other frameworks/technologies too.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 17, 2017

@nicolas-grekas

some random comments :)

Show outdated Hide outdated ...Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php
@@ -16,6 +16,8 @@
use Symfony\Component\Security\Core\Role\Role;
use Symfony\Component\Security\Core\Role\SwitchUserRole;
use Symfony\Component\Security\Core\User\User;
use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\User\AdvancedUserInterface;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

alpha order

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

alpha order

Show outdated Hide outdated src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php
@@ -16,6 +16,7 @@
use Symfony\Bridge\Doctrine\Tests\Fixtures\User;
use Symfony\Bridge\Doctrine\Security\User\EntityUserProvider;
use Doctrine\ORM\Tools\SchemaTool;
use Symfony\Component\Security\Core\User\UserInterface;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

local alpha order: should be moved one line up

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

local alpha order: should be moved one line up

Show outdated Hide outdated src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php
@@ -156,7 +157,7 @@ public function testLoadUserByUserNameShouldLoadUserWhenProperInterfaceProvided(
->method('loadUserByUsername')
->with('name')
->willReturn(
$this->getMockBuilder('\Symfony\Component\Security\Core\User\UserInterface')->getMock()
$this->getMockBuilder(UserInterface::class)->getMock()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

please revert all the non required ::class, we favor helping future merges than doing these CS updates

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

please revert all the non required ::class, we favor helping future merges than doing these CS updates

Show outdated Hide outdated src/Symfony/Component/Security/Core/User/UserInterface.php
@@ -47,7 +46,7 @@
* and populated in any number of different ways when the user object
* is created.
*
* @return (Role|string)[] The user roles
* @return Role|string[] The user roles

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be Role[]|string[] then I guess?

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be Role[]|string[] then I guess?

Show outdated Hide outdated src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
@@ -264,6 +264,7 @@ private function hasUserChanged(UserInterface $user)
}
if ($this->user instanceof AdvancedUserInterface && $user instanceof AdvancedUserInterface) {
@trigger_error(sprintf('Checking for the AdvancedUserInterface in %s has been deprecated in 3.4 and will be removed in 4.0. Implement the %s to check if the user has been changed,', __METHOD__, EquatableInterface::class), E_USER_DEPRECATED);

This comment has been minimized.

@iltar

iltar Aug 4, 2017

Contributor

Perhaps I should check for the in memory user class here as well?

@iltar

iltar Aug 4, 2017

Contributor

Perhaps I should check for the in memory user class here as well?

This comment has been minimized.

@ro0NL

ro0NL Aug 15, 2017

Contributor

it's equatable now.. so it wont get here right?

@ro0NL

ro0NL Aug 15, 2017

Contributor

it's equatable now.. so it wont get here right?

This comment has been minimized.

@iltar

iltar Aug 15, 2017

Contributor

The User, yes. The custom implementations of the AdvancedUserInterface, no

@iltar

iltar Aug 15, 2017

Contributor

The User, yes. The custom implementations of the AdvancedUserInterface, no

Show outdated Hide outdated src/Symfony/Component/Security/Core/Tests/User/UserTest.php
public function testIsEqualToWithDifferentUser()
{
$user = new User('username', 'password');
$this->assertFalse($user->isEqualTo($this->createMock(UserInterface::class)));

This comment has been minimized.

@chalasr

chalasr Aug 14, 2017

Member

should be getMockBuilder(UserInterface::class)->getMock() (see travis)

@chalasr

chalasr Aug 14, 2017

Member

should be getMockBuilder(UserInterface::class)->getMock() (see travis)

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Aug 14, 2017

Contributor

@chalasr tests should be green now

Contributor

iltar commented Aug 14, 2017

@chalasr tests should be green now

@@ -28,10 +28,14 @@ class UserChecker implements UserCheckerInterface
*/
public function checkPreAuth(UserInterface $user)
{
if (!$user instanceof AdvancedUserInterface) {
if (!$user instanceof AdvancedUserInterface && !$user instanceof User) {

This comment has been minimized.

@ro0NL

ro0NL Aug 15, 2017

Contributor

why do this? this will break any custom advanced user implem

@ro0NL

ro0NL Aug 15, 2017

Contributor

why do this? this will break any custom advanced user implem

This comment has been minimized.

@iltar

iltar Aug 15, 2017

Contributor

Previously it would return early if it wasn't an AdvancedUserInterface. In the future, this will be only if it's not a User (InMemoryUser). That's what I meant with the confusion about the naming.

@iltar

iltar Aug 15, 2017

Contributor

Previously it would return early if it wasn't an AdvancedUserInterface. In the future, this will be only if it's not a User (InMemoryUser). That's what I meant with the confusion about the naming.

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

This comment has been minimized.

@chalasr

chalasr Aug 23, 2017

Member

We can't assume that this method exists on $user class since it can be any UserInterface implementation which does not provide it

@chalasr

chalasr Aug 23, 2017

Member

We can't assume that this method exists on $user class since it can be any UserInterface implementation which does not provide it

This comment has been minimized.

@chalasr

chalasr Aug 23, 2017

Member

oops, missed the early return

@chalasr

chalasr Aug 23, 2017

Member

oops, missed the early return

@chalasr

chalasr approved these changes Aug 23, 2017 edited

👍 for pushing toward custom user checkers, the community seems to think it's worth dropping this interface.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Sep 25, 2017

Contributor

Squashed and rebased against 3.4 to solve merge conflicts

Contributor

iltar commented Sep 25, 2017

Squashed and rebased against 3.4 to solve merge conflicts

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 26, 2017

Member

@iltar sorry another rebase is needed

Member

nicolas-grekas commented Sep 26, 2017

@iltar sorry another rebase is needed

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Sep 27, 2017

Contributor
Contributor

iltar commented Sep 27, 2017

Show outdated Hide outdated ...Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php
@@ -184,11 +185,9 @@ public function testSetUser($user)
public function getUsers()
{
$user = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserInterface')->getMock();
$advancedUser = $this->getMockBuilder('Symfony\Component\Security\Core\User\AdvancedUserInterface')->getMock();
$user = $this->getMockBuilder(UserInterface::class)->getMock();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

all the non required "string to ::class" CS changes should be reverted to fit our policy and help reduce future merge conflicts as much as possible

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

all the non required "string to ::class" CS changes should be reverted to fit our policy and help reduce future merge conflicts as much as possible

This comment has been minimized.

@iltar

iltar Sep 27, 2017

Contributor

I've changed all usages in my changes to use the strings (in upcoming push). Due to how the diffing works, some of them are not displaying it correctly, as methods have been added/removed. By using the string everywhere, it should be less confusing for git as well.

@iltar

iltar Sep 27, 2017

Contributor

I've changed all usages in my changes to use the strings (in upcoming push). Due to how the diffing works, some of them are not displaying it correctly, as methods have been added/removed. By using the string everywhere, it should be less confusing for git as well.

Show outdated Hide outdated src/Symfony/Component/Security/Core/Tests/User/UserCheckerTest.php
* @group legacy
*
* @expectedDeprecation Calling Symfony\Component\Security\Core\User\UserChecker::checkPostAuth with an AdvancedUserInterface is deprecated as of 3.4 and will be removed in 4.0. Create a custom user checker if you wish to keep this functionality.
*

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

extra blank line (same in other places)

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

extra blank line (same in other places)

Show outdated Hide outdated src/Symfony/Component/Security/Core/User/AdvancedUserInterface.php
@@ -32,6 +32,7 @@
*
* @see UserInterface
* @see AccountStatusException
* @deprecated This interface is deprecated as of 3.4 and will be removed in 4.0.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

@deprecated since version 3.4, will be removed in 4.0.
(but no runtime notice because that'd trigger it for existing classes in core)

@nicolas-grekas

nicolas-grekas Sep 27, 2017

Member

@deprecated since version 3.4, will be removed in 4.0.
(but no runtime notice because that'd trigger it for existing classes in core)

@xabbuh

xabbuh approved these changes Sep 27, 2017

except for my minor comments regarding the changelog

Show outdated Hide outdated UPGRADE-3.4.md
@@ -277,6 +277,11 @@ Security
`DigestAuthenticationListener` and `DigestAuthenticationEntryPoint` will be
removed in 4.0. Use another authentication system like `http_basic` instead.
* Using the `AdvancedUserInterface` is now deprecated. To use the existing
functionality, create a custom user-checker based on the
`Symfony\Component\Security\Core\UserChecker`. This functionality will be

This comment has been minimized.

@xabbuh

xabbuh Sep 27, 2017

Member

Symfony\Component\Security\Core\User\UserChecker

@xabbuh

xabbuh Sep 27, 2017

Member

Symfony\Component\Security\Core\User\UserChecker

This comment has been minimized.

@xabbuh

xabbuh Sep 27, 2017

Member

This interface will [...]

@xabbuh

xabbuh Sep 27, 2017

Member

This interface will [...]

This comment has been minimized.

@ro0NL

ro0NL Sep 27, 2017

Contributor

UserChecker !== UserCheckerInterface

Shouldnt we deprecate UserChecker? as it's tied to AdvancedUserInterface 🤔

@ro0NL

ro0NL Sep 27, 2017

Contributor

UserChecker !== UserCheckerInterface

Shouldnt we deprecate UserChecker? as it's tied to AdvancedUserInterface 🤔

This comment has been minimized.

@iltar

iltar Sep 27, 2017

Contributor

No, but imo it should be renamed to the InMemoryUserChecker (see other comments), just like the InMemoryUser. I wish to do this, but I don't have the time to do so before the feature freeze after this gets merged.

If you have time to do so, I'd gladly help out. I'm already stretching my time with non-project related stuff at the moment, so I can't handle another full PR within a few days

@iltar

iltar Sep 27, 2017

Contributor

No, but imo it should be renamed to the InMemoryUserChecker (see other comments), just like the InMemoryUser. I wish to do this, but I don't have the time to do so before the feature freeze after this gets merged.

If you have time to do so, I'd gladly help out. I'm already stretching my time with non-project related stuff at the moment, so I can't handle another full PR within a few days

This comment has been minimized.

@ro0NL

ro0NL Sep 27, 2017

Contributor

I see. But in changelogs shouldnt we promote "based on UserCheckerInterface" instead?

Ill have a look at the InMemory story soonish :) i like it.

@ro0NL

ro0NL Sep 27, 2017

Contributor

I see. But in changelogs shouldnt we promote "based on UserCheckerInterface" instead?

Ill have a look at the InMemory story soonish :) i like it.

This comment has been minimized.

@iltar

iltar Sep 27, 2017

Contributor

@xabbuh This particular reference (and the one for CHANGELOG.md) are correctly referring to the implementation, how the conditions work based on the advanced user interface.

@iltar

iltar Sep 27, 2017

Contributor

@xabbuh This particular reference (and the one for CHANGELOG.md) are correctly referring to the implementation, how the conditions work based on the advanced user interface.

Show outdated Hide outdated src/Symfony/Component/Security/CHANGELOG.md
@@ -13,6 +13,10 @@ CHANGELOG
the user will always be logged out when the user has changed between
requests.
* deprecated HTTP digest authentication
* Using the AdvancedUserInterface is now deprecated. To use the existing
functionality, create a custom user-checker based on the
`Symfony\Component\Security\Core\UserChecker`. This functionality will be

This comment has been minimized.

@xabbuh

xabbuh Sep 27, 2017

Member

Symfony\Component\Security\Core\User\UserChecker

@xabbuh

xabbuh Sep 27, 2017

Member

Symfony\Component\Security\Core\User\UserChecker

This comment has been minimized.

@xabbuh

xabbuh Sep 27, 2017

Member

This interface will [...]

@xabbuh

xabbuh Sep 27, 2017

Member

This interface will [...]

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
Member

nicolas-grekas commented Oct 2, 2017

ping @iltar

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Oct 2, 2017

Contributor

Merge conflicts resolved and namespaces in the md files fixed as comment by xabbuh

Contributor

iltar commented Oct 2, 2017

Merge conflicts resolved and namespaces in the md files fixed as comment by xabbuh

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 24, 2017

Member

Moving to 4.1 as 3.4 is in beta, thus closed for new feats.

Member

nicolas-grekas commented Oct 24, 2017

Moving to 4.1 as 3.4 is in beta, thus closed for new feats.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Dec 11, 2017

Member

@iltar Can you rebase and update the changelog/update entries to account for 4.1?

Member

xabbuh commented Dec 11, 2017

@iltar Can you rebase and update the changelog/update entries to account for 4.1?

@iltar iltar changed the base branch from 3.4 to master Dec 11, 2017

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Dec 11, 2017

Contributor

@xabbuh should be fixed now

Contributor

iltar commented Dec 11, 2017

@xabbuh should be fixed now

@aschempp aschempp referenced this pull request Dec 20, 2017

Closed

[WIP] Security changes (step 3) #1256

4 of 7 tasks complete
@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jan 18, 2018

Member

@fabpot still sceptical about this change? It would be good to move forward

Member

chalasr commented Jan 18, 2018

@fabpot still sceptical about this change? It would be good to move forward

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 21, 2018

Member

(small rebase needed)

Member

nicolas-grekas commented Jan 21, 2018

(small rebase needed)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 24, 2018

Member

I'm sorry to come late in the game with this question, but what's the purpose of this change? What's the expected benefit? Every new deprecation has a clear downside: it is asking users to, someday, change their code. That's a cost for sure. What's the other side of the balance that justifies this?

To be honest, I really don't know this part of Symfony, so I don't expect to understand your answer :)
But I'd really like that people who understand what's going on here to still have this reasoning, and that we take the decision on this basis.

Any hints?

Member

nicolas-grekas commented Jan 24, 2018

I'm sorry to come late in the game with this question, but what's the purpose of this change? What's the expected benefit? Every new deprecation has a clear downside: it is asking users to, someday, change their code. That's a cost for sure. What's the other side of the balance that justifies this?

To be honest, I really don't know this part of Symfony, so I don't expect to understand your answer :)
But I'd really like that people who understand what's going on here to still have this reasoning, and that we take the decision on this basis.

Any hints?

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jan 24, 2018

Contributor

@nicolas-grekas long story short: this particular part in Symfony is domain logic, rather than infrastructure.

// comes with
public function isAccountNonExpired();
public function isAccountNonLocked();
public function isCredentialsNonExpired();
public function isEnabled();

Those 4 "states" can be reused by an application, but usually does not. In addition to that, the code that everyone uses by default, has all kind of exceptions programmed for the advanced user interface.

  • The UserChecker doesn't do anything unless it's an advanced user
  • The AbstractToken (which pretty much every token extends), contains special branches for the AdvancedUserInterface

This kind of logic does (in my opinion), not belong in the core framework. If you don't use the AdvancedUserInterface, Security will still work, but will still contain the domain logic in all the core framework classes.

Symfony by default comes with 1 user class: Symfony\Component\Security\Core\User\User. By default, this user implements the AdvancedUserInterface. However, this user object, is only used for the InMemoryUserProvider. This particular provider (in theory) doesn't need domain logic, because you manage it from the security.yaml config. In any other case, the documentation tells the developer to implement the UserInterface themselves.

So to sum it up:

  • The AdvancedUserInterface causes the core framework to contain code branches in 2 core classes that is used by pretty much everyone, but never hit
  • It provides a some form of domain logic with the 4 methods listed above, that are pretty much never needed at all (or maybe subset thereof).
  • The method names are very confusing due to being negations

As far as I can tell, the AdvancedUserInterface is based on a subset of methods from the Spring user: https://docs.spring.io/spring-security/site/docs/2.0.7.RELEASE/apidocs/org/springframework/security/userdetails/UserDetails.html (much as other parts, like the SecurityContext were based on). I feel that Symfony does not need this by default.

If this PR comes through:

  • There will be less User related interfaces and branches for a new developer to get confused by
  • It opens the door to make a NullUserChecker which would become the default, the current user checker doesn't do anything without the interface anyway
  • It opens the door to add an InMemoryUserChecker, which could do the current checks, if they are still needed (which seems obsolete imo, due to the config being in security.yaml)
  • It opens the door to rename the current User to InMemoryUser, which is what it's used for

If you have any questions, I'll be happy to answer 👍

Contributor

iltar commented Jan 24, 2018

@nicolas-grekas long story short: this particular part in Symfony is domain logic, rather than infrastructure.

// comes with
public function isAccountNonExpired();
public function isAccountNonLocked();
public function isCredentialsNonExpired();
public function isEnabled();

Those 4 "states" can be reused by an application, but usually does not. In addition to that, the code that everyone uses by default, has all kind of exceptions programmed for the advanced user interface.

  • The UserChecker doesn't do anything unless it's an advanced user
  • The AbstractToken (which pretty much every token extends), contains special branches for the AdvancedUserInterface

This kind of logic does (in my opinion), not belong in the core framework. If you don't use the AdvancedUserInterface, Security will still work, but will still contain the domain logic in all the core framework classes.

Symfony by default comes with 1 user class: Symfony\Component\Security\Core\User\User. By default, this user implements the AdvancedUserInterface. However, this user object, is only used for the InMemoryUserProvider. This particular provider (in theory) doesn't need domain logic, because you manage it from the security.yaml config. In any other case, the documentation tells the developer to implement the UserInterface themselves.

So to sum it up:

  • The AdvancedUserInterface causes the core framework to contain code branches in 2 core classes that is used by pretty much everyone, but never hit
  • It provides a some form of domain logic with the 4 methods listed above, that are pretty much never needed at all (or maybe subset thereof).
  • The method names are very confusing due to being negations

As far as I can tell, the AdvancedUserInterface is based on a subset of methods from the Spring user: https://docs.spring.io/spring-security/site/docs/2.0.7.RELEASE/apidocs/org/springframework/security/userdetails/UserDetails.html (much as other parts, like the SecurityContext were based on). I feel that Symfony does not need this by default.

If this PR comes through:

  • There will be less User related interfaces and branches for a new developer to get confused by
  • It opens the door to make a NullUserChecker which would become the default, the current user checker doesn't do anything without the interface anyway
  • It opens the door to add an InMemoryUserChecker, which could do the current checks, if they are still needed (which seems obsolete imo, due to the config being in security.yaml)
  • It opens the door to rename the current User to InMemoryUser, which is what it's used for

If you have any questions, I'll be happy to answer 👍

@nicolas-grekas

(minor rebase needed)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

Thank you @iltar.

Member

nicolas-grekas commented Feb 4, 2018

Thank you @iltar.

@nicolas-grekas nicolas-grekas merged commit 8456f3b into symfony:master Feb 4, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 4, 2018

feature #23508 Deprecated the AdvancedUserInterface (iltar)
This PR was merged into the 4.1-dev branch.

Discussion
----------

Deprecated the AdvancedUserInterface

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #23292
| License       | MIT
| Doc PR        | ~

This PR deprecates the usages of the `AdvancedUserInterface`.

Commits
-------

8456f3b Deprecated the AdvancedUserInterface
*
* @dataProvider getUserChangesAdvancedUser
*/
public function testSetUserSetsAuthenticatedToFalseWhenUserChangesdvancedUser($firstUser, $secondUser)

This comment has been minimized.

@keichinger

keichinger Mar 1, 2018

Contributor

There's an 'A' missing here to complete the word 'Advanced' :)

@keichinger

keichinger Mar 1, 2018

Contributor

There's an 'A' missing here to complete the word 'Advanced' :)

This comment has been minimized.

@iltar

iltar Mar 1, 2018

Contributor

Heh, you're completely right! If you wish, you could make a PR to fix this (you can just click the edit file). Gives you a nice contributor tag

@iltar

iltar Mar 1, 2018

Contributor

Heh, you're completely right! If you wish, you could make a PR to fix this (you can just click the edit file). Gives you a nice contributor tag

This comment has been minimized.

@keichinger

keichinger Mar 1, 2018

Contributor

Will do =) Thank you.

@keichinger

keichinger Mar 1, 2018

Contributor

Will do =) Thank you.

@@ -212,53 +209,59 @@ public function testSetUserSetsAuthenticatedToFalseWhenUserChanges($firstUser, $
}
public function getUserChanges()
{
$user = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserInterface')->getMock();

This comment has been minimized.

@keichinger

keichinger Mar 1, 2018

Contributor

I'm just wondering since I'm not too familiar with the Symfony testing guidelines: Wouldn't it be easier and more refactoring-friendly to use the ::class constants here instead of hand-writing the FQCN?

@keichinger

keichinger Mar 1, 2018

Contributor

I'm just wondering since I'm not too familiar with the Symfony testing guidelines: Wouldn't it be easier and more refactoring-friendly to use the ::class constants here instead of hand-writing the FQCN?

This comment has been minimized.

@iltar

iltar Mar 1, 2018

Contributor

Consistency in this case. I personally would update everything to use ::class, but that might cause merge conflicts for no reason when merging changes from lower branches upwards.

@iltar

iltar Mar 1, 2018

Contributor

Consistency in this case. I personally would update everything to use ::class, but that might cause merge conflicts for no reason when merging changes from lower branches upwards.

This comment has been minimized.

@keichinger

keichinger Mar 1, 2018

Contributor

Fair enough :) Thanks!

Opened #26349

@keichinger

keichinger Mar 1, 2018

Contributor

Fair enough :) Thanks!

Opened #26349

nicolas-grekas added a commit that referenced this pull request Mar 1, 2018

minor #26349 Fix typo in test method name (keichinger)
This PR was merged into the 4.1-dev branch.

Discussion
----------

Fix typo in test method name

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

This fixes a small typo spotted in #23508 (see https://github.com/symfony/symfony/pull/23508/files/8456f3b32ce6ec394fb27b9fc9a2989ed54862b1#r171488418)

Commits
-------

e5734aa Fix typo in test method name

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@hAz4rd0uS

This comment has been minimized.

Show comment
Hide comment
@hAz4rd0uS

hAz4rd0uS Jun 25, 2018

Hi, what should we use now instead of AdvancedUserInterface ?

Hi, what should we use now instead of AdvancedUserInterface ?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jun 25, 2018

Member

@h4ck3rm1k3 https://symfony.com/doc/current/security/user_checkers.html
Please consider reading the "fixed ticket" section of the PR body, discussion happened there :)

Member

chalasr commented Jun 25, 2018

@h4ck3rm1k3 https://symfony.com/doc/current/security/user_checkers.html
Please consider reading the "fixed ticket" section of the PR body, discussion happened there :)

Devtronic added a commit to Devtronic/FOSUserBundle that referenced this pull request Jul 7, 2018

Migrated SF AdvancedUserInterface to FOS UserInterface
AdvancedUserInterface is deprecated since Symfony 4.1
- symfony/symfony#23508

Issue:
- #2803 Deprecation with Symfony 4.1 - AdvancedUserInterface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment