Added a SecurityUserValueResolver for controllers #18510

linaori · 2016-04-12T07:02:08Z

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

This PR uses the new ArgumentResolver to inject a security user when the signature implies so. This is based on the docs code example and existing pr on the SFEB.

With the new example you can do the following:

// when a User is mandatory, e.g. behind firewall
public function fooAction(UserInterface $user)

// when a User is optional, e.g. where it can be anonymous
public function barAction(UserInterface $user = null)

This deprecates the Controller::getUser() method.

I have added it on a priority of 40 so it falls just under the RequestValueResolver. This is because it's already used and the initial performance is less of an impact.

There was a comment asking if the controller_argument.value_resolver tag name wasn't too long. If decided this tag should change before 3.1 is released, I will update it in here.

RequestValueResolver contains a small codestyle consistency fix.

linaori · 2016-04-12T08:16:56Z

Build failures unrelated to this change. For 5.5 see #18512 and regarding HHVM, I have no idea what it's complaining about.

javiereguiluz · 2016-04-12T08:25:10Z

Yes, I think controller_argument.value_resolver is too long. What about using any of the alternatives @iltar proposed in symfony/symfony-docs#6422 (comment)?

controller.value_resolver
controller.argument_value_resolver
controller.argument_value

wouterj · 2016-04-12T08:35:54Z

controller.argument_value_resolver at least sounds better than controller_argument.value_resolver (it isn't shorter though). If people think it's too long, I think argument.value_resolver or controller.argument_resolver should be used (unless I'm wrong, Argumentresolver can be used for all classes and not just controllers, right?)

linaori · 2016-04-12T08:39:37Z

@wouterj in theory it can be used for anything yes, you simply give it a callable and it will resolve. It's just called $controller and is Controller oriented (same for exception messages).

argument.value_resolver seems like the best choice in that PoV.

jvasseur · 2016-04-12T08:42:36Z

...tyBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/after_login.html.twig

@@ -1,7 +1,7 @@
 {% extends "::base.html.twig" %}

 {% block body %}
-    Hello {{ app.user.username }}!<br /><br />
+    Hello {{ user.username }}!<br /><br />


I don't think this should be changed, app.user is still a valid way to access the user in a template.

Hmm I was in the assumption there was another location where this was tested, I'll add a second test to verify this

The CsrfFormLoginBundle also has after_login.html.twig test and still does Hello {{ app.user.username }}!

DavidBadura · 2016-04-12T09:06:56Z

Isn't it a conflict with Doctrine ParamConverter? What happens if i have an User Entity that implements the Security UserInterface?

/**
 * @Route("/user/{user}")
 */
public function fooAction(User $user) {}

In Symfony <= 3.0 you get the User Entity based on the "user" Argument. But what i get with this change? The logged user? In this case we have a BC break.

Btw. why you deprecate Controller::getUser() and force to use the typehint method? Like ParamConverter it is nice to have, but shouldn't be necessary. The base concept of a controller is to transform a request into a response.

jvasseur · 2016-04-12T09:09:29Z

@DavidBadura this resolver have a lower priority that the RequestValueResolver that would fill the $user argument with the value resolved by the Doctrine ParamConvertor, so no BC break here.

linaori · 2016-04-12T09:13:29Z

@DavidBadura @jvasseur correct: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml#L28

I've done this on purpose for 100% BC reasons and the fact that this way you can define the values through attributes if you already have them. This is how the ControllerResolver used to do it as well.

I've deprecated the controller method because it does exactly the same as this. The request variant was also deprecated and removed in 3.0. Imo this is a cleaner way of getting the User than calling a method and checking what the return was. Another issue I sometimes see coming by, is that people with anon. can't figure out why they get no user back. This way you can see that it's always an object and you can easily typehint which user object. This also gives proper auto-completion in IDEs.

HeahDude · 2016-04-12T11:37:04Z

src/Symfony/Bundle/SecurityBundle/ValueResolver/SecurityUserValueResolver.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\ValueResolver;


What about Symfony\Bundle\SecurityBundle\ArgumentValueResolver ?

Got my vote but I don't know if it's a good idea to put things directly in the bundle root

If there is no other files requiring a HttpKernel or Controller (sub)namespace there is no reason to keep a complex tree in a bundle IMHO.

stof · 2016-04-12T14:53:26Z

Typehinting only the User class to get the current user is indeed confusing, as you might need to get a User based on the URL or the current logged-in user depending on the cases (and maybe even both in the same controller action)

linaori · 2016-04-12T17:00:14Z

@stof, that's only the case if you happen to use your Entity as security user object, which imo is a bad practice and is causing the confusion. Retrieving a security user (important to the security system) should imo never be done via the url.

stof · 2016-04-12T18:13:14Z

that's only the case if you happen to use your Entity as security user object, which imo is a bad practice and is causing the confusion

But this is the case in most almost all Symfony projects I know of (the other ones using some in memory providers for very basic setup), and probably in the vast majority of Symfony projects out there that I haven't reviewed as almost any tutorial does it too (and most people asking for support on the security system describe such setup too).
So introducing a feature which would break the parameter resolution for at least 90% of projects out there is bad. You cannot just say "this should not be done this way".

Retrieving a security user (important to the security system) should imo never be done via the url.

When the security user object is your entity, wanting a User object can mean 2 different things:

wanting the current user (which should come from the security context indeed, not from the URL)
wanting the user object based on the URL to display info about this user (and then you almost always need a security check to be sure that the current user can view the info about this user)

I'm not talking about getting the current user based on a parameter in the URL. I'm talking about getting any User based on this parameter.
For instance, on Github, you can look at the profile page of any user, not only about yourselves.

linaori · 2016-04-12T19:07:16Z

@stof so what are you recommending? Would this be a useless feature? I know I won't use it myself because the Token is enough for me (getUsername() > fetch actual user/client object and inject that). It seems like there's still animo for this feature though.

If someone uses the ParamConverter, pretty much everything can collide with that, especially given the silent activation of the listener. The only "broken" thing would be that if it does collide, the ParamConverter would get precedence over this feature.

wouterj · 2016-04-12T19:15:12Z

What about injecting the current user only when typehinting for UserInterface?

linaori · 2016-04-13T05:58:27Z

@wouterj that could work, as that interface defines your security user. In that case should I un-deprecate the getUser() method?

wouterj · 2016-04-13T07:15:30Z

@iltar why, this is still a replacement for the getUser getter afaics?

linaori · 2016-04-13T07:29:05Z

In the end, yes. Conceptually it's a bit further away; Getting "my user which I know it is" or getting "a UserInterface". If it would be @return UserInterface|null, I'd say it matches 100%, but it has @return mixed at the moment. In the end there's no difference and the object will be the same.

This PR was merged into the 3.1-dev branch. Discussion ---------- [HttpKernel] Renamed the argument resolver tag | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | not if merged before 3.1 | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Changed as discussed several times: #18510 (comment), symfony/symfony-docs#6422 (comment). Commits ------- cd10057 Renamed argument resolver tag, added test

This PR was merged into the 3.1-dev branch. Discussion ---------- [HttpKernel] Renamed the argument resolver tag | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | not if merged before 3.1 | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Changed as discussed several times: symfony/symfony#18510 (comment), symfony/symfony-docs#6422 (comment). Commits ------- cd10057 Renamed argument resolver tag, added test

linaori · 2016-04-15T09:57:23Z

Tag name is updated to controller.argument_value_resolver. Failure in appveyor is unreleated.

xabbuh · 2016-04-15T16:17:28Z

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

@@ -20,6 +20,11 @@

        <service id="security.token_storage" class="Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage" />

+        <service id="security.user_value_resolver" class="Symfony\Bundle\SecurityBundle\SecurityUserValueResolver">


should be private

linaori · 2016-04-18T06:42:32Z

@xabbuh I've updated all deprecation notices to state the UserInterface instead, should be clear now what to do instead of getUser().

@fabpot was this feature still in time for 3.1?

fabpot · 2016-06-15T07:31:26Z

It can be in 3.2 now (the changelogs should be updated).

@stof Is it ok as is or do you still have concerns about this feature? (I must admit that I haven't re-read the whole thread)

linaori · 2016-06-17T06:48:10Z

PR is updated and passing

fabpot · 2016-06-21T14:31:57Z

ping @stof

xabbuh · 2016-06-22T06:47:31Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

     * @throws \LogicException If SecurityBundle is not available
     *
     * @see TokenInterface::getUser()
     */
    protected function getUser()
    {
+        @trigger_error(sprintf('%s is deprecated as of 3.2 and will be removed in 4.0. You can typehint your method argument with %s instead.', __METHOD__, UserInterface::class), E_USER_DEPRECATED);


Missing parentheses after the first %s.

fabpot · 2016-07-01T05:36:40Z

@iltar can you rebase this one? If @stof does not have any other concern, I'd like to merge it soon. Thanks.

linaori · 2016-07-01T06:39:33Z

@fabpot done

fabpot · 2016-07-01T08:03:18Z

Thank you @iltar.

This PR was merged into the 3.2-dev branch. Discussion ---------- Added a SecurityUserValueResolver for controllers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ This PR uses the new `ArgumentResolver` to inject a security user when the signature implies so. This is based on the [docs code example](symfony/symfony-docs#6438 (comment)) and [existing pr on the SFEB](sensiolabs/SensioFrameworkExtraBundle#327). With the new example you can do the following: ```php // when a User is mandatory, e.g. behind firewall public function fooAction(UserInterface $user) // when a User is optional, e.g. where it can be anonymous public function barAction(UserInterface $user = null) ``` This deprecates the `Controller::getUser()` method. I have added it on a priority of 40 so it falls just under the `RequestValueResolver`. This is because it's already used and the initial performance is less of an impact. There was a comment asking if the `controller_argument.value_resolver` tag name wasn't too long. If decided this tag should change before 3.1 is released, I will update it in here. *`RequestValueResolver` contains a small codestyle consistency fix.* Commits ------- d341889 Added a SecurityUserValueResolver for controllers

ad3n · 2016-07-19T02:24:46Z

@iltar : just a question, can i do like this use this feature?

indexAction(UserInterface $user, Request $request) {}

linaori · 2016-07-19T06:04:10Z

@ad3n yes, that should be the way to use it. The sequence of arguments doesn't matter. I still have to write the documentation but that will be done before 3.2 is released (I hope).

ad3n · 2016-07-19T07:46:53Z

@iltar : 👍 thanks for confirmation

@iltar

This PR was merged into the 3.2-dev branch. Discussion ---------- Remove the new SecurityUserValueResolver | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no (the feature hasn't been released yet) | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Hi guys! This is a revert for #18510 (ping @iltar), which is a nice idea, but will have some big practical implications: 1) You are only allowed to type-hint the argument with `UserInterface` exactly. For the 90% of Symfony project's that user a User entity for their User, this will be weird: I'll receive a `UserInterface`, that immediately call methods on it that aren't in the interface (and also, your IDE won't give you auto-completion). And as #18510 mentions, we can't allow people to type-hint their concrete `User` class, because this will conflict with SensioFWExtraBundle ParamConverter if there is a user id in the URL 2) Deprecating and removing `$this->getUser()` in a controller is a step back. Where we can, let's make controllers and services act *more* like each other. You can't call `$this->getUser()` in a service, but at least if you look at this method in `Controller`, you can see that it uses `security.token_storage` - which is how you will get the User object if you need it from within services. Sorry for being late on this original issue! It looked good to me at first :). Cheers! Commits ------- da7daee Removing the Controller::getUser() deprecation

This PR was merged into the master branch. Discussion ---------- Added docs mentioning UserInterface in action args Added notes about the `UserInterface` added in symfony/symfony#18510, was waiting for the in-deprecation of the `getUser()` method: symfony/symfony#19452. Commits ------- 7849fa2 Added docs mentioning UserInterface in action args

… than `EntityValueResolver` (kbond) This PR was merged into the 6.2 branch. Discussion ---------- [SecurityBundle] Set `UserValueResolver`'s priority higher than `EntityValueResolver` | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a `UserValueResolver`'s priority is currently `40` and `EntityValueResolver`'s priority is `110` (configured in doctrine-bundle). Currently, to use the `CurrentUser` attribute and `MapEntity` (when `auto_mapping` is enabled), you need to do the following to have it work: ```php public function postAction( #[CurrentUser] #[MapEntity(disabled: true)] User $user, Post $post ) ``` This removes this need for `#[MapEntity(disabled: true)]` but I'm not sure the larger impact of increasing the priority of `UserValueResolver`. Here is some context as to why the priorities are they way they are: - doctrine/DoctrineBundle#1554 (comment) - #18510 Commits ------- 48499b9 Set `UserValueResolver`'s priority higher than `EntityValueResolver`

… than `EntityValueResolver` (kbond) This PR was merged into the 6.2 branch. Discussion ---------- [SecurityBundle] Set `UserValueResolver`'s priority higher than `EntityValueResolver` | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a `UserValueResolver`'s priority is currently `40` and `EntityValueResolver`'s priority is `110` (configured in doctrine-bundle). Currently, to use the `CurrentUser` attribute and `MapEntity` (when `auto_mapping` is enabled), you need to do the following to have it work: ```php public function postAction( #[CurrentUser] #[MapEntity(disabled: true)] User $user, Post $post ) ``` This removes this need for `#[MapEntity(disabled: true)]` but I'm not sure the larger impact of increasing the priority of `UserValueResolver`. Here is some context as to why the priorities are they way they are: - doctrine/DoctrineBundle#1554 (comment) - symfony/symfony#18510 Commits ------- 48499b99c4 Set `UserValueResolver`'s priority higher than `EntityValueResolver`

carsonbot added the Status: Needs Review label Apr 12, 2016

javiereguiluz added Feature SecurityBundle Deprecation labels Apr 12, 2016

jvasseur reviewed Apr 12, 2016
View reviewed changes

HeahDude reviewed Apr 12, 2016
View reviewed changes

linaori mentioned this pull request Apr 13, 2016

[HttpKernel] Renamed the argument resolver tag #18529

Merged

xabbuh reviewed Apr 15, 2016
View reviewed changes

xabbuh reviewed Jun 22, 2016
View reviewed changes

Added a SecurityUserValueResolver for controllers

d341889

fabpot merged commit d341889 into symfony:master Jul 1, 2016

linaori mentioned this pull request Jul 6, 2016

Add docs about the security user value resolver symfony/symfony-docs#6722

Closed

weaverryan mentioned this pull request Jul 27, 2016

Remove the new SecurityUserValueResolver #19452

Merged

linaori mentioned this pull request Oct 15, 2016

Added docs mentioning UserInterface in action args symfony/symfony-docs#7060

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

mickaelandrieu mentioned this pull request Nov 23, 2016

[Refacto] Create a GitHubEvent Resolver PrestaShop/prestonbot#28

Closed

linaori mentioned this pull request Apr 18, 2018

UserValueResolver and SecurityUserValueResolver improvement #26971

Closed

linaori deleted the feature/security-user-value-resolver branch February 8, 2019 13:38

Koc mentioned this pull request Mar 3, 2019

[Security/Http] Make UserValueResolver accept any subtype of UserInterface #30401

Closed

kbond mentioned this pull request Oct 28, 2022

[SecurityBundle] Set UserValueResolver's priority higher than EntityValueResolver #48032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a SecurityUserValueResolver for controllers #18510

Added a SecurityUserValueResolver for controllers #18510

linaori commented Apr 12, 2016

linaori commented Apr 12, 2016

javiereguiluz commented Apr 12, 2016

wouterj commented Apr 12, 2016

linaori commented Apr 12, 2016

jvasseur Apr 12, 2016

linaori Apr 12, 2016

linaori Apr 12, 2016

DavidBadura commented Apr 12, 2016

jvasseur commented Apr 12, 2016

linaori commented Apr 12, 2016

HeahDude Apr 12, 2016

linaori Apr 12, 2016

HeahDude Apr 12, 2016

stof commented Apr 12, 2016

linaori commented Apr 12, 2016

stof commented Apr 12, 2016

linaori commented Apr 12, 2016

wouterj commented Apr 12, 2016

linaori commented Apr 13, 2016

wouterj commented Apr 13, 2016

linaori commented Apr 13, 2016

linaori commented Apr 15, 2016

xabbuh Apr 15, 2016

linaori commented Apr 18, 2016

fabpot commented Jun 15, 2016

linaori commented Jun 17, 2016

fabpot commented Jun 21, 2016

xabbuh Jun 22, 2016

fabpot commented Jul 1, 2016

linaori commented Jul 1, 2016

fabpot commented Jul 1, 2016

ad3n commented Jul 19, 2016

linaori commented Jul 19, 2016

ad3n commented Jul 19, 2016

		@@ -20,6 +20,11 @@

		<service id="security.token_storage" class="Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage" />

		<service id="security.user_value_resolver" class="Symfony\Bundle\SecurityBundle\SecurityUserValueResolver">

Added a SecurityUserValueResolver for controllers #18510

Added a SecurityUserValueResolver for controllers #18510

Conversation

linaori commented Apr 12, 2016

linaori commented Apr 12, 2016

javiereguiluz commented Apr 12, 2016

wouterj commented Apr 12, 2016

linaori commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidBadura commented Apr 12, 2016

jvasseur commented Apr 12, 2016

linaori commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Apr 12, 2016

linaori commented Apr 12, 2016

stof commented Apr 12, 2016

linaori commented Apr 12, 2016

wouterj commented Apr 12, 2016

linaori commented Apr 13, 2016

wouterj commented Apr 13, 2016

linaori commented Apr 13, 2016

linaori commented Apr 15, 2016

Choose a reason for hiding this comment

linaori commented Apr 18, 2016

fabpot commented Jun 15, 2016

linaori commented Jun 17, 2016

fabpot commented Jun 21, 2016

Choose a reason for hiding this comment

fabpot commented Jul 1, 2016

linaori commented Jul 1, 2016

fabpot commented Jul 1, 2016

ad3n commented Jul 19, 2016

linaori commented Jul 19, 2016

ad3n commented Jul 19, 2016