I think I good name for this would be Access

$access->getUser();

$access->getToken();

@weaverryan small conflict to resolve

I'm still 👎 on this for a few important reasons:

• The only reason the user could ever be needed by anything other than the security component internally, is when you use it as entity. While this is makes certain tasks easier, it's not "the right thing to do".
• Same as the above counts for the token, you don't really need it anywhere outside of the security related functionality. The most common locations where you need them, are the voters and you have it available there
• That leaves us with the only useful method: isGranted, which is perfectly fine the way it is right now, though I admit that the naming is a bit unfortunate due to its length.

I feel that making shortcuts like this improves the short-term problem solving of applications, but also makes it easier to entangle your http layer with your domain layer. 3+ years later after making such decisions, I still have to suffer through this Technical Debt.

This proposal looks like a small change ... but it's a "game changer". See the before/after comparison:

Before

public function indexAction(TokenStorageInterface $tokenStorage)
{
    $user = $tokenStorage->getToken()->getUser();
}

After

public function indexAction(Security $security)
{
    $user = $security->getUser();
}

Before

public function indexAction(AuthorizationCheckerInterface $authorizationChecker)
{
    if ($authorizationChecker->isGranted('EDIT', $item)) {
        // ...
    }
}

After

public function indexAction(Security $security)
{
    if ($security->isGranted('EDIT', $item)) {
        // ...
    }
}

The Security part is the biggest problem for Symfony users. We need to make big and immediate changes to improve the situation. This proposal goes in that direction hiding internal details that users shouldn't care about.

Truly, the security component is one of the biggest Symfony problems.

In the current implementation, I would keep only getUser() and isGranted(). As @iltar stated, Token is not needed outside the security internals. I always used it only for getting the user.

@javiereguiluz but what about this?

/** @Security("is_granted('EDIT', item)") */
public function indexAction(UserInterface $user, Item $item)
{
    // ...
}

Granted, this requires the SensioFrameworkExtraBundle, but I rather see a generic solution for this (security annotation) as action requirement being added. If the length of the AuthorizationCheckerInterface is the problem, maybe this can be solved by making an alias?

Edit: don't get me wrong, Security as typehint is a lot nicer than the current solution!

@iltar that's fine ... but as an alternative. Symfony is pushing hard towards autowiring + service injection. That's why we must introduce this Security utility to avoid things like this 🤢 :

public function indexAction(AuthorizationCheckerInterface $authorizationChecker)

Just brainfarting here, but what about:

public function indexAction(IsGranted $isGranted, Item $item)
{
    if ($isGranted('EDIT', $item)) {
        // ...
    }
}

Which would feel similar to:

/**
 * @IsGranted("EDIT", subject="item")
 */

@iltar I don't like the IsGranted proposal for two reasons:

• The name sounds like a boolean variable instead of a class name.
• It forces me to learn more concepts and internal security things.

A single and centralized Security service simplifies everything, including the explanation of the common security things.

The downside is that it promotes another solution to fetch the User object, execute authorization checks and getting the token. I know why you want to add it, I just don't think it's a good idea because it often implies you might be on the wrong track of doing things.

If it's about long names, you could also simply rename the AuthorizationChecker to Authorizer and add an alias to the DIC for autowiring purposes, this prevents duplication of functionality.

• Getting the token in the action is hardly ever needed, if ever at all
• The user can be obtained already as I mentioned above
• That leaves the AuthorizationCheckerInterface, which could become Authorizer?

Side note, my signatures often are (AuthorizationCheckerInterface $authorizer), so it does feel natural to me to name it as such if desired.

@iltar we'll never agree on this ... but that's fine. I still think we need this to solve the most common security needs in a simple way. $security->getUser() and $security->isGranted(...) is something that anybody can easily understand. The alternative is not easy to understand at all (tokens, storages, authentication checkers, access decision managers, etc.)

The idea is to make things simpler, but this change will give them more options to do the same, which seems counter productive to me, that's what's bothering me at the moment. If that can be fixed, I'll be all 👍

Im not sure this helps in terms of teaching people they should decouple their security user from it's source of truth (an entity or so). Which IMHO is the real thing to solve, by teaching better practices.

Today i do it like this SecurityUserFactory + SecurityUser

Thanks to this awesome article; https://stovepipe.systems/post/decoupling-your-security-user

Works perfect today and doesnt require any additional wrapper/god objects etc.

Hey guys!

About decoupling the User class from the User entity, it's a very interesting idea, and solves some real problems. But, more work needs to be done in core before this can become the official way: it must be just as easy and as the current way. This might mean, for example, something similar to the SecurityUserFactory (from @ro0NL / @iltar's blog post) in core, because we still need a way to easily fetch the current User entity object (if you have that kind of setup) in a controller, service or template. This probably won't be a lot of work, but it does not exist in core for 3.4. So, that's the challenge: if you want this to become "the way" for 4.1, let's add a few shortcuts to make that really great experience.

For 3.4, we need this shortcut. And we can always deprecate and remove this if we move forward with a different philosophy in 4.1. In fact, I would love that - the Security component is benefiting from fresh thinking :).

This PR is ready for review again: unit & functional tests added, and psr/container dependency added to the Security component and the sub-component Core.

I believe the failure is false: it's failing (https://travis-ci.org/symfony/symfony/jobs/280454407#L2945) because the new method on Security don't exist on the master branch yet.

Thank you Ryan.

wow, why was it rushed out like this? I'm more than skeptical about these changes.

@fabpot There is one specific problem that we must solve (and this PR is one solution for it). To get the User object in auto-wired way, you need to type-hint TokenStorageInterface. Besides being a bit of an ugly name, the practical problem is that there are two TokenStorageInterface (and both in the Security components). In PhpStorm, you can't even see which is which - the suggestion only shows you the first part of the namespace, and they both match.

Secondarily, this gives an easier way to access the user, versus getToken()->getUser().

Wdyt? Would you like to accomplish that in a different way? Or were your concerns more minor?

There is a UserInterface argument resolver. Why would you want the token storage in your controllers?

Imo, this is moving in completely the opposite direction: The concept of user had to be removed, not emphasized...

See also #21068 (which was closed because "a clearer Security roadmap had to be created before changing anything")

There is a UserInterface argument resolver. Why would you want the token storage in your controllers?

Not in the controllers, it's for when you want the User in a service. In a controller, I would just say $this->getUser() anyways.

I think longer-term, we need to look into bigger changes. But right now, this TokenStorageInterface type-hinting problem with autowiring into services is a major problem. I actually hit this in a KnpU screencast... and I had to fix my use statement because I couldn't tell in the video which is correct... so I just guessed.

We need this as a short-term solution to that one problem. Longer term? Yea, let's investigate some other stuff. But we can't do that other stuff for 3.4 :).

And we can't simply rename TokenStorageInterface to something else as a short term solution?

The problem here imho is that you're not only solving this autowire problem, you're also reintroducing SecurityContext which was removed with very good arguments.

@wouterj I disagree. To me, removing the SecurityContext was a mistake. As an end-user I don't care about the internals: I just need to get the user and protect resources. If Symfony internally separates that and implements classes and interfaces ... that's an internal detail which I don't care about (unless I care about and investigate the internals of Symfony ... but let's not make that mandatory for all users).

As an end-user I don't care about the internals: I just need to get the user and protect resources.

As an end-user, I just need to secure stuff. That's all the security component should do.

The fact that authentication results in a unique identifier, giving you the ability to provide details for that specific client, is just a bonus. And this bonus is used in such a domain specific way, that a framework shouldn't imply anything about it. This is clearly shown, as even the domains used in all the Symfony Documentation examples don't fit the imposed methods of Security's UserInterface.

I know that's hijacking this PR a bit. But my point is: We should remove the I need to get the user idea sooner than later from the Security component. In #21068 , if anything, I proofed that doing this is a real hard game. Adding fancy classes like this make this game even more complex. Until we get at Symfony 4.3, have still no Security roadmap and it's impossible to move away from users as the core team kept working using the I need to get the user-concept for 3 releases.

Now, I understand the problem Ryan is trying to solve (I've faced it as well). However, it can be solved in a much smaller way by just renaming TokenStorageInterface to TokenHolderInterface, TokenPersistentInterface, maybe even Security...

Just a quick note: SecurityContext was split in large part because it caused lazy loading problems between the service for authorization and the token storage. That’s not a problem anymore due to the service locator used here.

But I’m fine if we’d rather rename the interface. But I’d still prefer this (until we have time to dive further into bigger changes) as it also gives you the getUser() shortcut, which just makes sense given how people currently configure their security.

Also, nothing in this PR prevents broader changes in the future. I vote to keep it as is.

Sorry to comment on such old PR, but I'd like to ask a question, maybe I'm missing something.

This class is PITA in tests, it has ContainerInterface as a dependency, and it's final. Which makes it impossible to mock.

This is the code I came up with:

class SecurityMockFactory
{
    public static function create(User $user = null)
    {
        $prophet = new Prophet();

        $token = $prophet->prophesize(TokenInterface::class);
        $token->getUser()->willReturn($user);

        $tokenStorage = $prophet->prophesize(TokenStorageInterface::class);
        $tokenStorage->getToken()->willReturn($token);

        $container = $prophet->prophesize(ContainerInterface::class);
        $container->get('security.token_storage')->willReturn($tokenStorage);

        $authorizationChecker = $prophet->prophesize(AuthorizationCheckerInterface::class);
        $authorizationChecker->isGranted(AuthenticatedVoter::IS_AUTHENTICATED_REMEMBERED, null)->willReturn(null !== $user);

        $container->get('security.authorization_checker')->willReturn($authorizationChecker);

        return new Security($container->reveal());
    }
}

Do you see any easier way to mock this class?