ParamConverter to inject the current user #327

adamquaile · 2014-09-20T19:20:19Z

When creating testable controllers, it's often desirable to not use the getUser function in the base controller, as this relies on the whole service container.

In this case, we might add a dependency on the SecurityContext to achieve the same result, replicating this code:

public function getUser()
{
    if (!$this->container->has('security.context')) {
        throw new \LogicException('The SecurityBundle is not registered in your application.');
    }

    if (null === $token = $this->container->get('security.context')->getToken()) {
        return;
    }

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

    return $user;
}

This parameter converter streamlines the process and injects the current user into the controller method instead using the following format:

/**
 * @ParamConverter(converter="current_user", name="parameterName")
 */
public function viewInboxAction(User $currentUser)

fabpot · 2014-09-21T07:26:00Z

Request/ParamConverter/CurrentUserParamConverter.php

+ * Does not require controllers to depend upon the entire
+ * SecurityContext object.
+ *
+ * @author Benjamin Eberlei <kontakt@beberlei.de>


Not sure if this is correct.

pyrech · 2014-09-23T18:24:10Z

Request/ParamConverter/CurrentUserParamConverter.php

+    /**
+     * {@inheritdoc}
+     *
+     * @throws NotFoundHttpException When invalid date given


It seems wrong :)

Good point. I used the DateTime converter as reference. Updated now, thanks.

linaori · 2014-10-09T15:50:39Z

Request/ParamConverter/CurrentUserParamConverter.php

+    {
+        if (null === $this->security) {
+            throw new \LogicException('The SecurityBundle is required for the current_user ParamConverter annotation');
+        }


I see 2 better alternatives here:

A) Instead of making it optional and throwing an exception if missing, add this service/converter only if the security component is available
B) Move this to the supports() and just return false if non-existent.

I'm in favor of A.

@iltar This is actually intentional. If this converter is ignored because the SecurityBundle is not enabled, either the type-hinted $user object will not be injected, or it might be handled by another converter which wasn't the developer's intention.

This way forces a meaningful error message to the user, and tells them how to fix the problem.

You have a dependency on the SecurityContext(Interface). That means this converter should not be registered at the moment your application doesn't have it. This can be gracefully done by removing it when the security.context is not available. What if I register a second param converter that listens to that interface with a lower priority that doesn't need the SecurityContext for this?

In my opinion it's not up to the param converter to determine what should happen when the SecurityContext is not available, but the configuration.

I see your point, although there can always be conflicts between two converters, as I found when doctrine was trying to handle my User objects (when not using the UserInterface) for this converter.

I'll try it this way too, only defining the service if the SecurityBundle is enabled.

hhamon · 2014-10-09T15:58:53Z

Just nit picking but I don't really like the name CurrentUserParamConverter. Why not using a more meaningful name like AuthenticatedUserParamConverter?

adamquaile · 2014-10-09T16:05:40Z

@hhamon Completely open to better names, although isn't it possible that the current user isn't authenticated?

xabbuh · 2014-10-10T07:24:19Z

Resources/doc/annotations/converters.rst

+If you type-hint against the UserInterface, or an interface which extends UserInterface,
+you may omit this annotation entirely, like this:
+
+     public function myAccountAction(\Symfony\Component\Security\Core\User\UserInterface $currentUser)


I would add a use statement to make this more readable:

use Symfony\Component\Security\Core\User\UserInterface; // ... public function myAccountAction(UserInterface $currentUser) { // ... }

@xabbuh Updated. Thanks.

fabpot · 2015-01-07T07:20:30Z

Resources/doc/annotations/converters.rst

+
+    // ...
+
+    public function myAccountAction(\Symfony\Component\Security\Core\User\UserInterface $currentUser)


This can be simplified to public function myAccountAction(UserInterface $currentUser) as you have a use statement above.

Done. 944ec39

adamquaile · 2015-01-07T09:24:20Z

@fabpot Job failed on travis for transient composer network issue. Restarted and it's good again.

adamquaile · 2015-04-13T13:55:04Z

@fabpot Updated from most recent master. Any other changes I should make?

mickaelandrieu · 2015-04-21T18:47:48Z

nice addition :)

MacDada · 2015-08-14T14:24:51Z

Request/ParamConverter/CurrentUserParamConverter.php

+     */
+    public function apply(Request $request, ParamConverter $configuration)
+    {
+        $param = $configuration->getName();


@adamquaile $param is used after if+throw so the value could be fetched later (after if+throw)

@MacDada Yes, you're right. I wouldn't mind making this change, if this PR is still wanted - it was opened a year ago!

Any maintainers know if there is a plan to merge this, or should we close it? @fabpot @jakzal

@adamquaile Well, typehinting UserInterface / EntityWithUserInterface is exactly what I'm looking for, so I hope it's gonna be resolved soon :)

linaori · 2016-04-08T07:58:53Z

@adamquaile I think this PR is no longer required. This functionality can be build either inside the SecurityBundle in symfony or in your app manually. This PR is actually the inspiration I've used to add a doc example: https://github.com/symfony/symfony-docs/pull/6438/files#diff-378394f006769e0d975a8e861369dc33R99

fabpot · 2016-04-10T17:08:02Z

agreed

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

adamquaile added 7 commits September 20, 2014 19:36

Added CurrentUserParamConverter

be48fcf

Integrate CurrentUserParamConverter into bundle

48008b3

Documentation for CurrentUserParamConverter

c5a00a7

Changed exception type for CurrentUserParamConverter

b468684

Fixed converter name

4ffec9b

Add friendlier message when SecurityBundle is not enabled

5b0fe9b

Fix coding standards on CurrentUserParamConverter

61dffd4

fabpot reviewed Sep 21, 2014
View reviewed changes

adamquaile added 4 commits September 21, 2014 13:16

CurrentUser CS suggestions from @fabpot

9182c58

Added further test cases for supports

ac1d271

Added comment about being able to ommit annotation for current user

995702d

Applied CS patch from fabbot

be58513

pyrech reviewed Sep 23, 2014
View reviewed changes

adamquaile added 2 commits September 23, 2014 19:26

Updated CurrentUserParamConverter doc-comment

1815f77

Updated CS, for fabbot

7bd2734

linaori reviewed Oct 9, 2014
View reviewed changes

xabbuh reviewed Oct 10, 2014
View reviewed changes

adamquaile added 4 commits October 10, 2014 09:50

Added use statement to CurrentUserParamConverter docs

71fbde4

Removed .class parameter in service def for CurrentUserParamConverter

5e76d8e

Do not throw exception when SecurityBundle is not enabled

724952f

Updated documentation to match change in exception logic

c42018e

fabpot reviewed Jan 7, 2015
View reviewed changes

Code style in current-user paramconverter

944ec39

adamquaile added 2 commits April 13, 2015 14:30

Merge branch 'master' into feature/current-user-paramconverter

95c0cd8

Change for updated code-style rules

65c8a19

linaori mentioned this pull request Apr 19, 2015

[DX] Added CurrentUserProvider service symfony/symfony#14407

Closed

MacDada reviewed Aug 14, 2015
View reviewed changes

linaori mentioned this pull request Mar 28, 2016

Added an ArgumentResolver with clean extension point symfony/symfony#18308

Merged

fabpot closed this Apr 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ParamConverter to inject the current user #327

ParamConverter to inject the current user #327

adamquaile commented Sep 20, 2014

fabpot Sep 21, 2014

pyrech Sep 23, 2014

adamquaile Sep 23, 2014

linaori Oct 9, 2014

adamquaile Oct 9, 2014

linaori Oct 9, 2014

adamquaile Oct 10, 2014

hhamon commented Oct 9, 2014

adamquaile commented Oct 9, 2014

xabbuh Oct 10, 2014

adamquaile Oct 10, 2014

fabpot Jan 7, 2015

adamquaile Jan 7, 2015

adamquaile commented Jan 7, 2015

adamquaile commented Apr 13, 2015

mickaelandrieu commented Apr 21, 2015

MacDada Aug 14, 2015

adamquaile Aug 14, 2015

MacDada Aug 14, 2015

linaori commented Apr 8, 2016

fabpot commented Apr 10, 2016


		// ...

		public function myAccountAction(\Symfony\Component\Security\Core\User\UserInterface $currentUser)

ParamConverter to inject the current user #327

ParamConverter to inject the current user #327

Conversation

adamquaile commented Sep 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhamon commented Oct 9, 2014

adamquaile commented Oct 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamquaile commented Jan 7, 2015

adamquaile commented Apr 13, 2015

mickaelandrieu commented Apr 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Apr 8, 2016

fabpot commented Apr 10, 2016