Adding a shortcuts for the main security functionality #24337

weaverryan · 2017-09-26T19:29:25Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	Big ol' TODO

I'd like one class that I can inject (especially with autowiring) to get access to the User and isGranted() methods. This is really important... because to get the User currently, you need to type-hint TokenStorageInterface... and there are two! That's really bad DX!

Questions:

A) I hi-jacked the existing Security class... I wanted a simple class called Security
B) I called the service security.helper... for lack of a better id.
C) I did not make Security implement the 2 other interfaces (TokenStorageInterface, AuthorizationCheckerInterface... but I suppose we could?)

Cheers!

afurculita · 2017-09-26T19:54:35Z

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

@@ -4,6 +4,8 @@ CHANGELOG
 3.4.0
 -----

+ * Added new `security.helper` service that is an instance of `Symfony\Component\Security\Core`


Symfony\Component\Security\Core\Security

afurculita · 2017-09-26T19:56:52Z

src/Symfony/Component/Security/Core/Security.php

+    /**
+     * @return UserInterface|null
+     */
+    public function getUser()


Return type instead of phpdoc?

No, this targets 3.4 which supports PHP 5.6.

andersonamuller · 2017-09-26T20:01:43Z

I think I good name for this would be Access

$access->getUser();

$access->getToken();

chalasr · 2017-09-26T20:31:28Z

@weaverryan small conflict to resolve

nicolas-grekas · 2017-09-26T20:49:25Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml

+            <argument type="service">
+                <service class="Symfony\Component\DependencyInjection\ServiceLocator">
+                    <tag name="container.service_locator" />
+                    <argument type="collection">


You should make the Security class implement ServiceSubscriberInterface instead. But maybe you prefer no dep on the DI component?

Yea... that was the basic idea: it felt better to keep the Security class from the component coupled only to psr/container. And actually, that wasn't listed as a dep in composer.json. I'm going to add that now... tests must've passed because another dep of symfony/security already required it?

Haha, or because I don't have a test for that class... fixing that now :)

nicolas-grekas

Worth adding a small functional test?

andersonamuller · 2017-09-26T20:58:23Z

src/Symfony/Component/Security/Core/Security.php

+
+        $user = $token->getUser();
+        if (!is_object($user)) {
+            return;


as of PHP 7.1 we have the void return type (reference) so I suggest making the null explicit:

return null;

linaori · 2017-09-27T07:11:37Z

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.

javiereguiluz · 2017-09-27T07:34:31Z

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

Before

public functio
8000
n 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.

afurculita · 2017-09-27T07:44:35Z

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.

linaori · 2017-09-27T07:48:51Z

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

javiereguiluz · 2017-09-27T08:03:51Z

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

linaori · 2017-09-27T08:12:55Z

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

javiereguiluz · 2017-09-27T08:30:06Z

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

linaori · 2017-09-27T08:37:38Z

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.

javiereguiluz · 2017-09-27T08:53:53Z

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

linaori · 2017-09-27T09:08:39Z

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 👍

ro0NL · 2017-09-27T12:12:05Z

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.

weaverryan · 2017-09-27T14:24:12Z

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.

weaverryan · 2017-09-27T15:03:19Z

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.

nicolas-grekas

LGTM to enhance DX in a quick and pragmatic way.

nicolas-grekas · 2017-09-28T08:53:29Z

src/Symfony/Component/Security/Core/composer.json

@@ -17,6 +17,7 @@
    ],
    "require": {
        "php": "^5.5.9|>=7.0.8",
+        "psr/container": "^1.0",


Shouldn't this be in require-dev? The dep is required only when using new Security() (same below).

fabpot · 2017-09-28T14:49:04Z

src/Symfony/Component/Security/Core/Security.php

 /**
- * This class holds security information.
+ * Helper class for commonly-needed security tasks.
 *
 * @author Johannes M. Schmitt <schmittjoh@gmail.com>


should probably be removed

chalasr · 2017-09-28T15:13:29Z

Thank you Ryan.

… (weaverryan, javiereguiluz) This PR was squashed before being merged into the 3.4 branch (closes #24337). Discussion ---------- Adding a shortcuts for the main security functionality | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | Big ol' TODO I'd like one class that I can inject (especially with autowiring) to get access to the User and `isGranted()` methods. This is *really* important... because to get the User currently, you need to type-hint `TokenStorageInterface`... and there are *two*! That's really bad DX! Questions: A) I hi-jacked the existing `Security` class... I wanted a simple class called Security B) I called the service `security.helper`... for lack of a better id. C) I did not make `Security` implement the 2 other interfaces (`TokenStorageInterface`, `AuthorizationCheckerInterface`... but I suppose we could?) Cheers! Commits ------- 0851189 Adding a shortcuts for the main security functionality

fabpot · 2017-09-28T17:32:40Z

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

weaverryan · 2017-09-28T18:11:04Z

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

Pierstoval · 2017-09-28T18:47:35Z

src/Symfony/Component/Security/Core/Security.php

+        }
+
+        $user = $token->getUser();
+        if (!is_object($user)) {


Can't the user be a simple string in some cases?
I mean, the TokenInterface says this:

/** * Returns a user representation. * * @return mixed Can be a UserInterface instance, an object implementing a __toString method, * or the username as a regular string * * @see AbstractToken::setUser() */ public function getUser();

So instead, this should return mixed instead of UserInterface...

Yep, but I'm purposely not allowing for people who use a string User to make use of this feature. We do that in other places. Allowing it to be a string makes everyone's user experience worse. So, you can do this, but you need to use the lower-level tools :)

wouterj · 2017-09-29T12:30:11Z

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

weaverryan · 2017-09-29T14:09:17Z

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

wouterj · 2017-09-29T14:19:07Z

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.

javiereguiluz · 2017-09-29T14:26:43Z

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

wouterj · 2017-09-29T15:01:03Z

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

weaverryan · 2017-10-01T02:39:07Z

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.

umpirsky · 2018-06-28T12:02:51Z

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?

…ss (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [Security] use final annotation to allow mocking the class | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29946 | License | MIT | Doc PR | When the class was initially marked as `final`, it did only contain constants. Since #24337 the `Security` class also contains useful shortcut methods so allowing developers to mock the class in tests looks reasonable to me. Commits ------- 1da00db use final annotation to allow mocking the class

Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7 Commits ------- 7d20c9b Use the security helper f0981e7 Rewrite the picker providers to use security voters 919f974 Use default value for the access validation 7235615 Catch exception if firewall is not active (e.g. 404 pages) 4d4c603 Fix the coding style

Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7cf6546bac6a776a64826ab7b96fe956d4 Commits ------- 7d20c9b3 Use the security helper f0981e7c Rewrite the picker providers to use security voters 919f9744 Use default value for the access validation 7235615d Catch exception if firewall is not active (e.g. 404 pages) 4d4c6032 Fix the coding style

carsonbot added Status: Needs Review Feature labels Sep 26, 2017

weaverryan added this to the 3.4 milestone Sep 26, 2017

afurculita reviewed Sep 26, 2017

View reviewed changes

chalasr changed the base branch from 10000 master to 3.4 September 26, 2017 20:28

nicolas-grekas reviewed Sep 26, 2017

View reviewed changes

andersonamuller reviewed Sep 26, 2017

View reviewed changes

weaverryan force-pushed the security-simple-class branch 4 times, most recently from 9a7f4f5 to 04c774d Compare September 27, 2017 14:22

nicolas-grekas approved these changes Sep 28, 2017

View reviewed changes

Adding a shortcut for the main security functionality

7f1abe5

fabpot reviewed Sep 28, 2017

View reviewed changes

fixes

466a928

chalasr closed this Sep 28, 2017

xabbuh mentioned this pull request Sep 28, 2017

Document the Security helper methods symfony/symfony-docs#8437

Closed

weaverryan deleted the security-simple-class branch September 28, 2017 18:06

Pierstoval reviewed Sep 28, 2017

View reviewed changes

javiereguiluz mentioned this pull request Oct 3, 2017

Added shortcuts for the main security functionality symfony/symfony-docs#8451

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

TomasVotruba mentioned this pull request Sep 4, 2018

Add simple security pehapkari/pehapkari.cz#6

Closed

xabbuh mentioned this pull request Jan 22, 2019

[Security] use final annotation to allow mocking the class #29955

Merged

aschempp mentioned this pull request Jul 26, 2019

Use the security helper contao/contao#609

Merged

Uh oh!

Adding a shortcuts for the main security functionality #24337

Adding a shortcuts for the main security functionality #24337

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Before

After

Before

After

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!