8000 [FrameworkBundle] Use security.token_storage service in Controller::getUser() by xelaris · Pull Request #13214 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Use security.token_storage service in Controller::getUser() #13214

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[FrameworkBundle] Use security.token_storage service in Controller::getUser() #13214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c5bd20
[FrameworkBundle] Use security.token_storage service in Controller::g…
xelaris Jan 3, 2015
982afbd
[FrameworkBundle] Added unit tests for Controller::getUser()
xelaris Jan 3, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ public function getDoctrine()
}

/**
* Get a user from the Security Context.
* Get a user from the Security Token Storage.
*
* @return mixed
*
Expand All @@ -303,15 +303,16 @@ public function getDoctrine()
*/
public function getUser()
{
if (!$this->container->has('security.context')) {
if (!$this->container->has('security.token_storage')) {
throw new \LogicException('The SecurityBundle is not registered in your application.');
}

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

if (!is_object($user = $token->getUser())) {
// e.g. anonymous authentication
return;
}

Expand Down
95 changes: 94 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,13 @@

use Symfony\Bundle\FrameworkBundle\Tests\TestCase;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
use Symfony\Component\Security\Core\User\User;

class ControllerTest extends TestCase
{
Expand All @@ -37,10 +41,99 @@ public function testForward()
$container->expects($this->at(0))->method('get')->will($this->returnValue($requestStack));
$container->expects($this->at(1))->method('get')->will($this->returnValue($kernel));

$controller = new Controller();
$controller = new TestController();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating a class, where we will add all methods progressively, what about using reflection to access the method?

$m = new \ReflectionMethod('...Controller', 'getUser');
$m->setAccessible(true);
$m->invoke(...);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I see reflection, I usually notice that the code might not be correct. If you can't test it easily, you might have to rewrite so it is.

I think that the best solution would be if all the basic functionality of the Controller would move away to a loose class (or 2). The current Controller could then delegate the methods and you can test the public methods properly on that specific class. This also makes it easier to re-use certain functionality from the controller if you use 'Controller as a Service'.

I think this falls out of the scope of the PR though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I agree with you that "non-easily-testable" code is a smell, in this specific case, this is perfectly valid IMO.

Please, don't overcomplicate things, this class is how we tie everything together, easy and simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be handled with a magic __call() method in the TestController class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used reflection so far, so I am not very familiar with it. @fabpot Your suggestion sounds like it would avoid the need of the TestController class completely. If I am not wrong, it would only avoid the overriding of the methods. Maybe that was your point. In this case it would not simplify the testcode IMO, since I have to change

$this->assertSame($controller->getUser(), $user);

to

 $m = new \ReflectionMethod('Symfony\Bundle\FrameworkBundle\Controller\Controller', 'getUser');
 $m->setAccessible(true);
 $this->assertSame($m->invoke($controller), $user);

where I still need

$controller = new TestController();
$controller->setContainer($this->getContainerWithTokenStorage($token));

By avoiding adding all the tested methods of Controller progressively, the test methods itself would become unlike more verbose. Although it could be reduced with a helper method, for me, the current version is simpler (for readers, writers and runtime).

@xabbuh Using __call is indeed an option, but has the drawback that the IDE complains about calling the protected method then.

class TestController extends Controller
{
    function __call($name, $arguments)
    {
        return call_user_func_array('parent::'.$name, $arguments);
    }
}

Let me know if I should change the PR though to either use reflection or the __call method.

To address @iltar's comment... What about capitalizing the abilities of traits in PHP 5.4 for SF 3.0 by providing traits with the shortcut methods, so developers can use the methods without extending a base Controller?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bad practice to do that because it still requires me to have a container in my controller. As long as I don't see the container and it's abstracted away, I'm ok with it.

$controller->setContainer($container);

$response = $controller->forward('a_controller');
$this->assertEquals('xml--fr', $response->getContent());
}

public function testGetUser()
{
$user = new User('user', 'pass');
$token = new UsernamePasswordToken($user, 'pass', 'default', array('ROLE_USER'));

$controller = new TestController();
$controller->setContainer($this->getContainerWithTokenStorage($token));

$this->assertSame($controller->getUser(), $user);
}

public function testGetUserAnonymousUserConvertedToNull()
{
$token = new AnonymousToken('default', 'anon.');

$controller = new TestController();
$controller->setContainer($this->getContainerWithTokenStorage($token));

$this->assertNull($controller->getUser());
}

public function testGetUserWithEmptyTokenStorage()
{
$controller = new TestController();
$controller->setContainer($this->getContainerWithTokenStorage(null));

$this->assertNull($controller->getUser());
}

/**
* @expectedException \LogicException
* @expectedExceptionMessage The SecurityBundle is not registered in your application.
*/
public function testGetUserWithEmptyContainer()
{
$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
$container
->expects($this->once())
->method('has')
->with('security.token_storage')
->will($this->returnValue(false));

$controller = new TestController();
$controller->setContainer($container);

$controller->getUser();
}

/**
* @param $token
* @return ContainerInterface
*/
private function getContainerWithTokenStorage($token = null)
{
$tokenStorage = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage');
$tokenStorage
->expects($this->once())
->method('getToken')
->will($this->returnValue($token));

$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
$container
->expects($this->once())
->method('has')
->with('security.token_storage')
->will($this->returnValue(true));

$container
->expects($this->once())
->method('get')
->with('security.token_storage')
->will($this->returnValue($tokenStorage));

return $container;
}
}

class TestController extends Controller
{
public function forward($controller, array $path = array(), array $query = array())
{
return parent::forward($controller, $path, $query);
}

public function getUser()
{
return parent::getUser();
}
}
0