[RFC][Security] Simplify the User and UserChecker #26348

linaori · 2018-03-01T08:40:52Z

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	yes
Symfony version	4.1

In #23508 the AdvancedUserInterface was deprecated and the UserChecker was made to support the User specifically. When 5.0 is released, that means that the UserChecker will effectively only work when the User is passed.

symfony/src/Symfony/Component/Security/Core/User/UserChecker.php

Lines 31 to 37 in 97cee3a

    
           if (!$user instanceof AdvancedUserInterface && !$user instanceof User) { 
        
               return; 
        
           } 
        
           if ($user instanceof AdvancedUserInterface && !$user instanceof User) { 
        
               @trigger_error(sprintf('Calling %s with an AdvancedUserInterface is deprecated since Symfony 4.1. Create a custom user checker if you wish to keep this functionality.', __METHOD__), E_USER_DEPRECATED); 
        
           }

Some facts:

The User class is not used anywhere in the core except for the LdapUserProvider and InMemoryUserProvider.
The InMemoryUserProvider only supports the enabled feature of the AdvancedUserInterface.
The LdapUserProvider doesn't use any of the AdvancedUserInterface features.

RFC

In this RFC I would like to propose the following things to reduce the complexity:

Create an LdapUser, exclusively used by the LdapUserProvider. For BC it can extend the User for instanceof checks. Would only receive the currently used fields.
Create an InMemoryUser, exclusively used by the InMemoryUserProvider. For BC it can extend the User for instanceof checks. Would only receive the currently used fields.
Add a NullUserChecker, which features 0 checks. In 5.0 this would become the default UserChecker in the configuration.
Rename the current UserChecker to InMemoryUserChecker (BC: UserChecker extends InMemoryUserChecker). This UserChecker will no longer check anything but the enabled flag. Keep this user-checker as default in 4.x, but throw a deprecation if no user-checker is configured.
Deprecated the current User user class and instead document thoroughly how to create your own user class and checker.
Provide an easy-to-use UserInterface implementation for tests only.

Scenarios

I've been thinking about a few scenarios in which things will change for developers. Generally speaking, the impact should be rather small.

Using the `User` in custom `UserProvider`

Time to fix: roughly 15~30 minutes, requires proper documentation

In this scenario, the develop will have to carefully look at which features should be kept. A custom UserInterface implementation will have to be provided. If any of the AdvancedUserInterface domain-like features were used, these will have to be manually implemented, and a custom UserChecker has to be created and configured.

To do:

Write custom user class and configure it in the provider that would previously create a User
Write custom user checker and configure it in the firewall

Using the `InMemoryUser` and `InMemoryUserProvider`

Time to fix: roughly 2 minutes, proper deprecation should suffice

In this scenario, by default, the UserChecker would be configured by Symfony as default config. In 5.0 this would become the NullUserChecker, which does nothing. The InMemoryUser would be able to define an enabled field, which has to be checked by the InMemoryUserChecker. To fix this, configure the right user checker

To do:

Configure the user-checker option in your firewall to use the InMemoryUserChecker

Using the `User` in tests

Time to fix: roughly 5~30 minutes, depending on how often it's used

In this scenario, which I believe is more common, developers have used the User in their tests. This is also the case within Symfony. I think that it would be easiest to add a TestUser in a Test namespace and expose this (as opposed to tests in the Tests namespace).

To do:

Replace the usages of User by TestUser with the right namespace

If I've missed any scenarios, please let me know and I will add them. The goal of this RFC is to reduce domain logic in Symfony and having no ambiguous purposes for the User and User Checker. Additionally the NullUserChecker will simply do nothing, which is the default case with a custom user class as of 4.1.

The text was updated successfully, but these errors were encountered:

Hanmac · 2018-03-01T16:35:24Z

i am okay with that if the AdvancedUserInterface is removed.

what more changes need to be done for stuff like FOSUser or SonataUser?
they need to implement their wanted methods, and meed their own user checker?

javiereguiluz · 2018-03-01T16:44:12Z

Iltar, what would be the impact for the most common use case? Simple app with no fancy needs that only wants a User class to represent their users which log in via a login form and don't have anything special: just a username + password + a bunch of properties (the username may be the email).

linaori · 2018-03-01T19:54:00Z

The most common use-case would probably be an entity with the UserInterface. If you wish to use the Symfony user, you have to have a customer user provider and custom logic to transform a username into that user. I believe that's actually more work than actually using an entity. However, my PoV is pretty limited as I've never seen the need to have either of the two options.

stof · 2018-03-02T11:06:20Z

@javiereguiluz simple apps with no fancy needs don't need a user checker at all (just like they don't need to use the AdvancedUserInterface features today, and the built-in user checker is a no-op for non-advanced implementations)

javiereguiluz · 2018-03-02T11:10:08Z

@stof yes ... and no. For example I consider the feature to mark a user account locked/expired/enabled (whatever you call it in your app) a very basic feature ... but it was provided in the AdvancedUserInterface.

I just wanted to say: OK if you want to improve the advanced case ... but please, don't make the simple case even harder. Thanks 🙏

linaori · 2018-03-02T12:52:47Z

For example I consider the feature to mark a user account locked/expired/enabled (whatever you call it in your app) a very basic feature ... but it was provided in the AdvancedUserInterface.

@javiereguiluz that case has already been made more "difficult" for custom user objects. The User in Symfony is final and none of the standard implementations have support for this. To use the User, you have to implement a lot more code. It would be easier to just create your own UserChecker and method for this. Writing those these few lines of code is maybe 5 minutes of work, as opposed to roughly 30 to 60 minutes (or more if you don't know how the security component works), just to write your own user provider.

linaori · 2018-04-13T07:14:21Z

Pinging @symfony/deciders whether or not I should work on this for Symfony 4.2+. It's a bit too much to do in a single PR and I don't want to do only half of the proposal 😄

I'm hoping to have covered all the upgrade paths in the initial post, but feel free to spew out more possible problems.

edit: seems like the deciders highlight is not working :(

wouterj · 2018-04-21T21:45:10Z

I'm very much 👍 for most of the things you mention in here. In the end, I think the whole concept of user needs to be removed from the Security component. Doing this seems like a sensible step to make User and UserInterface less relevant.
I'm sure we can remove most of the methods from the UserInterface. However, I'm not so sure on how we can do things, while still perserving BC.

I just wanted to say: OK if you want to improve the advanced case ... but please, don't make the simple case even harder. Thanks pray

The simple case of a login form with just the User class will remain exactly the same. The only change is that users now get an InMemoryUser extending User (for BC) instead of User. So the only impact are the cases that explicitly typehint for User instead of UserInterface, which is never a good idea (and also redundant, as all methods of User are in the UserInterface).

The basic features of locking/enabling/whatever users has never been a simple feature in Symfony (that's why most people use FOSUserBundle). That's however completely unrelated to this RFC.

linaori · 2018-04-22T09:43:56Z

I'm sure we can remove most of the methods from the UserInterface. However, I'm not so sure on how we can do things, while still perserving BC.

Personally I would like to see the UserInterface (or the majority thereof) being used for the internal security process, I just would like to prevent it from being stored in the token, storing the identifier only. But something has to be created to fetch the current domain user (usually an entity), see #26971 (comment).

carsonbot · 2020-12-20T13:02:37Z

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

carsonbot · 2021-01-03T13:20:16Z

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

chalasr · 2021-01-04T22:39:26Z

Create an LdapUser, exclusively used by the LdapUserProvider. For BC it can extend the User for instanceof checks. Would only receive the currently used fields.

Done in #32824.
Help needed to make the remaining changes proposed here.

chalasr · 2021-03-12T11:01:43Z

See #40443

xabbuh added Security Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Mar 1, 2018

linaori mentioned this issue Apr 13, 2018

[Security] Load the user before pre/post auth checks when needed #26788

Merged

carsonbot added the Stalled label Dec 20, 2020

chalasr added the Keep open label Jan 4, 2021

carsonbot removed the Stalled label Jan 4, 2021

chalasr added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Jan 4, 2021

This was referenced Jan 30, 2021

26348 Deprecate User and introduce InMemoryUser #40039

Closed

[Security] Depreccate usage of User in InMemoryUserProvider #40044

Closed

chalasr mentioned this issue Mar 10, 2021

[Security] Rename User to InMemoryUser #40443

Merged

fabpot closed this as completed in db87d72 Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][Security] Simplify the User and UserChecker #26348

[RFC][Security] Simplify the User and UserChecker #26348

linaori commented Mar 1, 2018

Hanmac commented Mar 1, 2018

javiereguiluz commented Mar 1, 2018

linaori commented Mar 1, 2018

stof commented Mar 2, 2018

javiereguiluz commented Mar 2, 2018

linaori commented Mar 2, 2018 •

edited

Loading

linaori commented Apr 13, 2018 •

edited

Loading

wouterj commented Apr 21, 2018

linaori commented Apr 22, 2018

carsonbot commented Dec 20, 2020

carsonbot commented Jan 3, 2021

chalasr commented Jan 4, 2021

chalasr commented Mar 12, 2021

[RFC][Security] Simplify the User and UserChecker #26348

[RFC][Security] Simplify the User and UserChecker #26348

Comments

linaori commented Mar 1, 2018

RFC

Scenarios

Using the User in custom UserProvider

Using the InMemoryUser and InMemoryUserProvider

Using the User in tests

Hanmac commented Mar 1, 2018

javiereguiluz commented Mar 1, 2018

linaori commented Mar 1, 2018

stof commented Mar 2, 2018

javiereguiluz commented Mar 2, 2018

linaori commented Mar 2, 2018 • edited Loading

linaori commented Apr 13, 2018 • edited Loading

wouterj commented Apr 21, 2018

linaori commented Apr 22, 2018

carsonbot commented Dec 20, 2020

carsonbot commented Jan 3, 2021

chalasr commented Jan 4, 2021

chalasr commented Mar 12, 2021

Using the `User` in custom `UserProvider`

Using the `InMemoryUser` and `InMemoryUserProvider`

Using the `User` in tests

linaori commented Mar 2, 2018 •

edited

Loading

linaori commented Apr 13, 2018 •

edited

Loading