Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Copy link
Copy link
Closed
@linaori

Description

@linaori
Issue body actions
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.

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    FeatureHelp wantedIssues and PRs which are looking for volunteers to complete them.Issues and PRs which are looking for volunteers to complete them.Keep openRFCRFC = Request For Comments (proposals about features that you want to be discussed)RFC = Request For Comments (proposals about features that you want to be discussed)Security

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.