Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Added a SecurityUserValueResolver for controllers#18510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Added a SecurityUserValueResolver for controllers#18510
Uh oh!
There was an error while loading.Please reload this page.
Conversation
linaori commentedApr 12, 2016
Build failures unrelated to this change. For 5.5 see#18512 and regarding HHVM, I have no idea what it's complaining about. |
javiereguiluz commentedApr 12, 2016
Yes, I think
|
wouterj commentedApr 12, 2016
|
linaori commentedApr 12, 2016
@wouterj in theory it can be used for anything yes, you simply give it a callable and it will resolve. It's just called
|
| {%blockbody %} | ||
| Hello {{app.user.username }}!<br /><br /> | ||
| Hello {{user.username }}!<br /><br /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think this should be changed,app.user is still a valid way to access the user in a template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm I was in the assumption there was another location where this was tested, I'll add a second test to verify this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The CsrfFormLoginBundle also has after_login.html.twig test and still doesHello {{ app.user.username }}!
DavidBadura commentedApr 12, 2016
Isn't it a conflict with Doctrine ParamConverter? What happens if i have an User Entity that implements the Security UserInterface? /** * @Route("/user/{user}") */publicfunctionfooAction(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 |
jvasseur commentedApr 12, 2016
@DavidBadura this resolver have a lower priority that the |
linaori commentedApr 12, 2016
@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 |
| * file that was distributed with this source code. | ||
| */ | ||
| namespace Symfony\Bundle\SecurityBundle\ValueResolver; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What aboutSymfony\Bundle\SecurityBundle\ArgumentValueResolver ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Got my vote but I don't know if it's a good idea to put things directly in the bundle root
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If there is no other files requiring aHttpKernel orController (sub)namespace there is no reason to keep a complex tree in a bundle IMHO.
stof commentedApr 12, 2016
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 commentedApr 12, 2016
@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 imonever be done via the url. |
stof commentedApr 12, 2016
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).
When the security user object is your entity, wanting a User object can mean 2 different things:
I'm not talking about getting the current user based on a parameter in the URL. I'm talking about gettingany User based on this parameter. |
linaori commentedApr 12, 2016
@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 ( If someone uses the |
wouterj commentedApr 12, 2016
What about injecting the current user only when typehinting for |
linaori commentedApr 13, 2016
@wouterj that could work, as that interface defines your security user. In that case should I un-deprecate the |
wouterj commentedApr 13, 2016
@iltar why, this is still a replacement for the getUser getter afaics? |
linaori commentedApr 13, 2016
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 |
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 commentedApr 15, 2016
Tag name is updated to |
| <serviceid="security.token_storage"class="Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage" /> | ||
| <serviceid="security.user_value_resolver"class="Symfony\Bundle\SecurityBundle\SecurityUserValueResolver"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should beprivate
linaori commentedApr 18, 2016
fabpot commentedJun 15, 2016
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 commentedJun 17, 2016
PR is updated and passing |
fabpot commentedJun 21, 2016
ping@stof |
| */ | ||
| protectedfunctiongetUser() | ||
| { | ||
| @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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Missing parentheses after the first%s.
fabpot commentedJul 1, 2016
linaori commentedJul 1, 2016
@fabpot done |
fabpot commentedJul 1, 2016
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 firewallpublic function fooAction(UserInterface $user)// when a User is optional, e.g. where it can be anonymouspublic 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 commentedJul 19, 2016
@iltar : just a question, can i do like this use this feature? |
linaori commentedJul 19, 2016
@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 commentedJul 19, 2016
@iltar : 👍 thanks for confirmation |
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/aHi 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 URL2) 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 argsAdded notes about the `UserInterface` added insymfony/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:```phppublic 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)-#18510Commits-------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:```phppublic 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#18510Commits-------48499b99c4 Set `UserValueResolver`'s priority higher than `EntityValueResolver`
This PR uses the new
ArgumentResolverto inject a security user when the signature implies so. This is based on thedocs code example andexisting pr on the SFEB.With the new example you can do the following:
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_resolvertag name wasn't too long. If decided this tag should change before 3.1 is released, I will update it in here.RequestValueResolvercontains a small codestyle consistency fix.