merged branch meandmymonkey/switchuser-noexception (PR #3580)
Commits -------0e4f789
changed test configa98d554
[SecurityBundle] Allow switching to the user that is already impersonated (fix #2554) Discussion ---------- [Security] Disabled exception when switching to the user that is already impersonated Bug fix: yes-ish Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #2554 Todo: - --------------------------------------------------------------------------- by vicb at 2012-03-13T14:31:45Z @meandmymonkey thank you for your work on this issue. Would you have time to add functional tests ? --------------------------------------------------------------------------- by meandmymonkey at 2012-03-13T14:49:52Z Probably not today, but during the next few days, yes, of course. --------------------------------------------------------------------------- by meandmymonkey at 2012-03-14T18:05:19Z @vicb @schmittjoh Writing the tests I noticed switching to an non-existent user will not raise an exception. While it's not a security issue, it should raise an error for completeness sake, shouldn't it? --------------------------------------------------------------------------- by vicb at 2012-03-14T20:28:52Z I think it should (throw an `AuthenticationCredentialsNotFoundException`). _btw there is an extra `sprintf` in the original code that could be remove when attempting to exit_ --------------------------------------------------------------------------- by meandmymonkey at 2012-03-14T21:13:16Z The problem with throwing an `AuthenticationCredentialsNotFoundException` (or any other security exception for that matter) is that it derives from `AuthenticationException`, which means it gets caught by the framework and redirects to the login form, which is not what we want in this case. We need to throw something 500-ish at [L89](d40b3376ec/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php (L89)
), either a generic or a (new) custom Exception. --------------------------------------------------------------------------- by meandmymonkey at 2012-03-14T21:43:57Z IMHO a `LogicException`would be fine, like the one used at [L117](d40b3376ec/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php (L117)
), as the error is not really about a failed authentication. --------------------------------------------------------------------------- by vicb at 2012-03-14T21:49:04Z I agree and btw very good job on the tests ! --------------------------------------------------------------------------- by meandmymonkey at 2012-03-14T22:12:43Z Thanks :) --------------------------------------------------------------------------- by vicb at 2012-03-15T08:01:13Z Could you squash the commits, prefix the commit message with `[SecurityBundle]` and add `(fix #2554)` at the end ? --------------------------------------------------------------------------- by meandmymonkey at 2012-03-15T08:53:12Z Done. --------------------------------------------------------------------------- by vicb at 2012-03-15T09:19:09Z @fabpot this PR looks good to me. --------------------------------------------------------------------------- by fabpot at 2012-03-15T12:50:50Z Tests do not pass when you run them all. --------------------------------------------------------------------------- by meandmymonkey at 2012-03-15T13:41:45Z @fabpot @vicb With this config change, they pass when run together. What is weird though is that the reason seems to be that the config for the profiler gets overwritten when running all tests together, while being used correctly when run alone. Any idea what can cause this? They should be isolated from each other. The new config from0e4f789
works, but enables the profiler for all SecurityBundle Tests... which is not strictly necessary.
This commit is contained in:
commit
c4df57212b
@ -0,0 +1,91 @@
|
||||
<?php
|
||||
|
||||
/*
|
||||
* This file is part of the Symfony framework.
|
||||
*
|
||||
* (c) Fabien Potencier <fabien@symfony.com>
|
||||
*
|
||||
* This source file is subject to the MIT license that is bundled
|
||||
* with this source code in the file LICENSE.
|
||||
*/
|
||||
|
||||
namespace Symfony\Bundle\SecurityBundle\Tests\Functional;
|
||||
|
||||
/**
|
||||
* @group functional
|
||||
*/
|
||||
class SwitchUserTest extends WebTestCase
|
||||
{
|
||||
/**
|
||||
* @dataProvider getTestParameters
|
||||
*/
|
||||
public function testSwitchUser($originalUser, $targetUser, $expectedUser, $expectedStatus)
|
||||
{
|
||||
$client = $this->createAuthenticatedClient($originalUser);
|
||||
|
||||
$client->request('GET', '/profile?_switch_user=' . $targetUser);
|
||||
|
||||
$this->assertEquals($expectedStatus, $client->getResponse()->getStatusCode());
|
||||
$this->assertEquals($expectedUser, $client->getProfile()->getCollector('security')->getUser());
|
||||
}
|
||||
|
||||
public function testSwitchedUserCannotSwitchToOther()
|
||||
{
|
||||
$client = $this->createAuthenticatedClient('user_can_switch');
|
||||
|
||||
$client->request('GET', '/profile?_switch_user=user_cannot_switch_1');
|
||||
$client->request('GET', '/profile?_switch_user=user_cannot_switch_2');
|
||||
|
||||
$this->assertEquals(500, $client->getResponse()->getStatusCode());
|
||||
$this->assertEquals('user_cannot_switch_1', $client->getProfile()->getCollector('security')->getUser());
|
||||
}
|
||||
|
||||
public function testSwitchedUserExit()
|
||||
{
|
||||
$client = $this->createAuthenticatedClient('user_can_switch');
|
||||
|
||||
$client->request('GET', '/profile?_switch_user=user_cannot_switch_1');
|
||||
$client->request('GET', '/profile?_switch_user=_exit');
|
||||
|
||||
$this->assertEquals(200, $client->getResponse()->getStatusCode());
|
||||
$this->assertEquals('user_can_switch', $client->getProfile()->getCollector('security')->getUser());
|
||||
}
|
||||
|
||||
public function getTestParameters()
|
||||
{
|
||||
return array(
|
||||
'unauthorized_user_cannot_switch' => array('user_cannot_switch_1', 'user_cannot_switch_1', 'user_cannot_switch_1', 403),
|
||||
'authorized_user_can_switch' => array('user_can_switch', 'user_cannot_switch_1', 'user_cannot_switch_1', 200),
|
||||
'authorized_user_cannot_switch_to_non_existent' => array('user_can_switch', 'user_does_not_exist', 'user_can_switch', 500),
|
||||
'authorized_user_can_switch_to_himself' => array('user_can_switch', 'user_can_switch', 'user_can_switch', 200),
|
||||
);
|
||||
}
|
||||
|
||||
protected function createAuthenticatedClient($username)
|
||||
{
|
||||
$client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => 'switchuser.yml'));
|
||||
$client->followRedirects(true);
|
||||
$client->insulate();
|
||||
|
||||
$form = $client->request('GET', '/login')->selectButton('login')->form();
|
||||
$form['_username'] = $username;
|
||||
$form['_password'] = 'test';
|
||||
$client->submit($form);
|
||||
|
||||
return $client;
|
||||
}
|
||||
|
||||
protected function setUp()
|
||||
{
|
||||
parent::setUp();
|
||||
|
||||
$this->deleteTmpDir('StandardFormLogin');
|
||||
}
|
||||
|
||||
protected function tearDown()
|
||||
{
|
||||
parent::tearDown();
|
||||
|
||||
$this->deleteTmpDir('StandardFormLogin');
|
||||
}
|
||||
}
|
@ -0,0 +1,14 @@
|
||||
imports:
|
||||
- { resource: ./config.yml }
|
||||
|
||||
security:
|
||||
providers:
|
||||
in_memory:
|
||||
memory:
|
||||
users:
|
||||
user_can_switch: { password: test, roles: [ROLE_USER, ROLE_ALLOWED_TO_SWITCH] }
|
||||
user_cannot_switch_1: { password: test, roles: [ROLE_USER] }
|
||||
user_cannot_switch_2: { password: test, roles: [ROLE_USER] }
|
||||
firewalls:
|
||||
default:
|
||||
switch_user: true
|
@ -11,6 +11,7 @@ framework:
|
||||
session:
|
||||
auto_start: true
|
||||
storage_id: session.storage.mock_file
|
||||
profiler: { only_exceptions: false }
|
||||
|
||||
services:
|
||||
logger: { class: Symfony\Component\HttpKernel\Log\NullLogger }
|
||||
|
@ -86,9 +86,7 @@ class SwitchUserListener implements ListenerInterface
|
||||
try {
|
||||
$this->securityContext->setToken($this->attemptSwitchUser($request));
|
||||
} catch (AuthenticationException $e) {
|
||||
if (null !== $this->logger) {
|
||||
$this->logger->warn(sprintf('Switch User failed: "%s"', $e->getMessage()));
|
||||
}
|
||||
throw new \LogicException(sprintf('Switch User failed: "%s"', $e->getMessage()));
|
||||
}
|
||||
}
|
||||
|
||||
@ -108,8 +106,14 @@ class SwitchUserListener implements ListenerInterface
|
||||
private function attemptSwitchUser(Request $request)
|
||||
{
|
||||
$token = $this->securityContext->getToken();
|
||||
if (false !== $this->getOriginalToken($token)) {
|
||||
throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
|
||||
$originalToken = $this->getOriginalToken($token);
|
||||
|
||||
if (false !== $originalToken) {
|
||||
if ($token->getUsername() === $request->get($this->usernameParameter)) {
|
||||
return $token;
|
||||
} else {
|
||||
throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
|
||||
}
|
||||
}
|
||||
|
||||
if (false === $this->accessDecisionManager->decide($token, array($this->role))) {
|
||||
@ -148,7 +152,7 @@ class SwitchUserListener implements ListenerInterface
|
||||
private function attemptExitUser(Request $request)
|
||||
{
|
||||
if (false === $original = $this->getOriginalToken($this->securityContext->getToken())) {
|
||||
throw new AuthenticationCredentialsNotFoundException(sprintf('Could not find original Token object.'));
|
||||
throw new AuthenticationCredentialsNotFoundException('Could not find original Token object.');
|
||||
}
|
||||
|
||||
if (null !== $this->dispatcher) {
|
||||
|
Reference in New Issue
Block a user