From f300edebe4c27171a4fc7c6dfd0a71f219a6d850 Mon Sep 17 00:00:00 2001 From: Johannes Schmitt Date: Tue, 19 Jul 2011 16:21:58 +0200 Subject: [PATCH] fixes several bugs --- UPDATE.md | 3 + .../Controller/LocalizedController.php | 5 ++ .../Resources/config/localized_routing.yml | 13 ++++- .../Functional/LocalizedRoutesAsPathTest.php | 24 ++++++++ .../SecurityRoutingIntegrationTest.php | 31 +++++++--- .../Tests/Functional/WebTestCase.php | 2 +- .../StandardFormLogin/localized_routes.yml | 6 +- .../localized_routes_with_forward.yml | 9 +++ .../HttpFoundation/RequestMatcher.php | 6 +- .../Component/Security/Http/HttpUtils.php | 57 ++++++++++++++----- .../HttpFoundation/RequestMatcherTest.php | 5 +- .../Component/Security/Http/HttpUtilsTest.php | 11 ++++ 12 files changed, 138 insertions(+), 34 deletions(-) create mode 100644 src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes_with_forward.yml diff --git a/UPDATE.md b/UPDATE.md index 407bc43aa1..459156a8cd 100644 --- a/UPDATE.md +++ b/UPDATE.md @@ -77,6 +77,9 @@ RC4 to RC5 Session::getAttributes() -> Session::all() Session::setAttributes() -> Session::replace() +* {_locale} is not supported in paths in the access_control section anymore. You can + rewrite the paths using a regular expression such as "(?:[a-z]{2})". + RC3 to RC4 ---------- diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LocalizedController.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LocalizedController.php index ee4c47469c..7ac6bbdfc8 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LocalizedController.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Controller/LocalizedController.php @@ -34,6 +34,11 @@ class LocalizedController extends ContainerAware throw new \RuntimeException('logoutAction() should never be called.'); } + public function secureAction() + { + throw new \RuntimeException('secureAction() should never be called.'); + } + public function profileAction() { return new Response('Profile'); diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/config/localized_routing.yml b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/config/localized_routing.yml index 2c38fee9be..803558f300 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/config/localized_routing.yml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/config/localized_routing.yml @@ -1,19 +1,30 @@ localized_login_path: pattern: /{_locale}/login defaults: { _controller: FormLoginBundle:Localized:login } + requirements: { _locale: "^[a-z]{2}$" } localized_check_path: pattern: /{_locale}/login_check defaults: { _controller: FormLoginBundle:Localized:loginCheck } + requirements: { _locale: "^[a-z]{2}$" } localized_default_target_path: pattern: /{_locale}/profile defaults: { _controller: FormLoginBundle:Localized:profile } + requirements: { _locale: "^[a-z]{2}$" } localized_logout_path: pattern: /{_locale}/logout defaults: { _controller: FormLoginBundle:Localized:logout } + requirements: { _locale: "^[a-z]{2}$" } localized_logout_target_path: pattern: /{_locale}/ - defaults: { _controller: FormLoginBundle:Localized:homepage } \ No newline at end of file + defaults: { _controller: FormLoginBundle:Localized:homepage } + requirements: { _locale: "^[a-z]{2}$" } + +localized_secure_path: + pattern: /{_locale}/secure/ + defaults: { _controller: FormLoginBundle:Localized:secure } + requirements: { _locale: "^[a-z]{2}$" } + \ No newline at end of file diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php index c4502c60dd..1ad67ac542 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/LocalizedRoutesAsPathTest.php @@ -26,6 +26,30 @@ class LocalizedRoutesAsPathTest extends WebTestCase $this->assertEquals('Homepage', $client->followRedirect()->text()); } + /** + * @dataProvider getLocales + */ + public function testAccessRestrictedResource($locale) + { + $client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => 'localized_routes.yml')); + $client->insulate(); + + $client->request('GET', '/'.$locale.'/secure/'); + $this->assertRedirect($client->getResponse(), '/'.$locale.'/login'); + } + + /** + * @dataProvider getLocales + */ + public function testAccessRestrictedResourceWithForward($locale) + { + $client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => 'localized_routes_with_forward.yml')); + $client->insulate(); + + $crawler = $client->request('GET', '/'.$locale.'/secure/'); + $this->assertEquals(1, count($crawler->selectButton('login')), (string) $client->getResponse()); + } + public function getLocales() { return array(array('en'), array('de')); diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityRoutingIntegrationTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityRoutingIntegrationTest.php index d066f5aea5..d649d4434d 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityRoutingIntegrationTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityRoutingIntegrationTest.php @@ -13,25 +13,37 @@ namespace Symfony\Bundle\SecurityBundle\Tests\Functional; class SecurityRoutingIntegrationTest extends WebTestCase { - public function testRoutingErrorIsNotExposedForProtectedResourceWhenAnonymous() + /** + * @dataProvider getConfigs + */ + public function testRoutingErrorIsNotExposedForProtectedResourceWhenAnonymous($config) { - $client = $this->createClient(array('test_case' => 'StandardFormLogin')); + $client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => $config)); + $client->insulate(); $client->request('GET', '/protected_resource'); $this->assertRedirect($client->getResponse(), '/login'); } - public function testRoutingErrorIsExposedWhenNotProtected() + /** + * @dataProvider getConfigs + */ + public function testRoutingErrorIsExposedWhenNotProtected($config) { - $client = $this->createClient(array('test_case' => 'StandardFormLogin')); + $client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => $config)); + $client->insulate(); $client->request('GET', '/unprotected_resource'); - $this->assertEquals(404, $client->getResponse()->getStatusCode()); + $this->assertEquals(404, $client->getResponse()->getStatusCode(), (string) $client->getResponse()); } - public function testRoutingErrorIsNotExposedForProtectedResourceWhenLoggedInWithInsufficientRights() + /** + * @dataProvider getConfigs + */ + public function testRoutingErrorIsNotExposedForProtectedResourceWhenLoggedInWithInsufficientRights($config) { - $client = $this->createClient(array('test_case' => 'StandardFormLogin')); + $client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => $config)); + $client->insulate(); $form = $client->request('GET', '/login')->selectButton('login')->form(); $form['_username'] = 'johannes'; @@ -43,6 +55,11 @@ class SecurityRoutingIntegrationTest extends WebTestCase $this->assertNotEquals(404, $client->getResponse()->getStatusCode()); } + public function getConfigs() + { + return array(array('config.yml'), array('routes_as_path.yml')); + } + protected function setUp() { parent::setUp(); diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/WebTestCase.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/WebTestCase.php index 953e9f08b9..c5f4f79355 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/WebTestCase.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/WebTestCase.php @@ -18,7 +18,7 @@ class WebTestCase extends BaseWebTestCase { static public function assertRedirect($response, $location) { - self::assertTrue($response->isRedirect()); + self::assertTrue($response->isRedirect(), 'Response is not a redirect, got status code: '.$response->getStatusCode()); self::assertEquals('http://localhost'.$location, $response->headers->get('Location')); } diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes.yml b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes.yml index 7157f29592..291cd2051b 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes.yml +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes.yml @@ -19,4 +19,8 @@ security: logout: path: localized_logout_path target: localized_logout_target_path - anonymous: ~ \ No newline at end of file + anonymous: ~ + + access_control: + - { path: '^/(?:[a-z]{2})/secure/.*', roles: ROLE_USER } + \ No newline at end of file diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes_with_forward.yml b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes_with_forward.yml new file mode 100644 index 0000000000..d57f84e8ef --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/localized_routes_with_forward.yml @@ -0,0 +1,9 @@ +imports: + - { resource: ./localized_routes.yml } + +security: + firewalls: + default: + form_login: + use_forward: true + failure_forward: true diff --git a/src/Symfony/Component/HttpFoundation/RequestMatcher.php b/src/Symfony/Component/HttpFoundation/RequestMatcher.php index dc5a2c34f7..9adf780bcc 100644 --- a/src/Symfony/Component/HttpFoundation/RequestMatcher.php +++ b/src/Symfony/Component/HttpFoundation/RequestMatcher.php @@ -100,11 +100,7 @@ class RequestMatcher implements RequestMatcherInterface } if (null !== $this->path) { - if (null !== $session = $request->getSession()) { - $path = strtr($this->path, array('{_locale}' => $session->getLocale(), '#' => '\\#')); - } else { - $path = str_replace('#', '\\#', $this->path); - } + $path = str_replace('#', '\\#', $this->path); if (!preg_match('#'.$path.'#', $request->getPathInfo())) { return false; diff --git a/src/Symfony/Component/Security/Http/HttpUtils.php b/src/Symfony/Component/Security/Http/HttpUtils.php index 51168cc1e3..3eccb4188b 100644 --- a/src/Symfony/Component/Security/Http/HttpUtils.php +++ b/src/Symfony/Component/Security/Http/HttpUtils.php @@ -11,6 +11,8 @@ namespace Symfony\Component\Security\Http; +use Symfony\Component\Security\Core\SecurityContextInterface; + use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\Routing\RouterInterface; @@ -45,22 +47,10 @@ class HttpUtils */ public function createRedirectResponse(Request $request, $path, $status = 302) { - if (0 === strpos($path, '/')) { + if ('/' === $path[0]) { $path = $request->getUriForPath($path); } elseif (0 !== strpos($path, 'http')) { - // hack (don't have a better solution for now) - $context = $this->router->getContext(); - try { - $parameters = $this->router->match($request->getPathInfo()); - } catch (\Exception $e) { - } - - if (isset($parameters['_locale'])) { - $context->setParameter('_locale', $parameters['_locale']); - } elseif ($session = $request->getSession()) { - $context->setParameter('_locale', $session->getLocale()); - } - + $this->resetLocale($request); $path = $this->generateUrl($path, true); } @@ -78,10 +68,26 @@ class HttpUtils public function createRequest(Request $request, $path) { if ($path && '/' !== $path[0] && 0 !== strpos($path, 'http')) { + $this->resetLocale($request); $path = $this->generateUrl($path, true); } - return Request::create($path, 'get', array(), $request->cookies->all(), array(), $request->server->all()); + $newRequest = Request::create($path, 'get', array(), $request->cookies->all(), array(), $request->server->all()); + if ($session = $request->getSession()) { + $newRequest->setSession($session); + } + + if ($request->attributes->has(SecurityContextInterface::AUTHENTICATION_ERROR)) { + $newRequest->attributes->set(SecurityContextInterface::AUTHENTICATION_ERROR, $request->attributes->get(SecurityContextInterface::AUTHENTICATION_ERROR)); + } + if ($request->attributes->has(SecurityContextInterface::ACCESS_DENIED_ERROR)) { + $newRequest->attributes->set(SecurityContextInterface::ACCESS_DENIED_ERROR, $request->attributes->get(SecurityContextInterface::ACCESS_DENIED_ERROR)); + } + if ($request->attributes->has(SecurityContextInterface::LAST_USERNAME)) { + $newRequest->attributes->set(SecurityContextInterface::LAST_USERNAME, $request->attributes->get(SecurityContextInterface::LAST_USERNAME)); + } + + return $newRequest; } /** @@ -107,6 +113,27 @@ class HttpUtils return $path === $request->getPathInfo(); } + // hack (don't have a better solution for now) + private function resetLocale(Request $request) + { + $context = $this->router->getContext(); + if ($context->getParameter('_locale')) { + return; + } + + try { + $parameters = $this->router->match($request->getPathInfo()); + + if (isset($parameters['_locale'])) { + $context->setParameter('_locale', $parameters['_locale']); + } elseif ($session = $request->getSession()) { + $context->setParameter('_locale', $session->getLocale()); + } + } catch (\Exception $e) { + // let's hope user doesn't use the locale in the path + } + } + private function generateUrl($route, $absolute = false) { if (null === $this->router) { diff --git a/tests/Symfony/Tests/Component/HttpFoundation/RequestMatcherTest.php b/tests/Symfony/Tests/Component/HttpFoundation/RequestMatcherTest.php index 677c286fa8..d807d22893 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/RequestMatcherTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/RequestMatcherTest.php @@ -142,7 +142,7 @@ class RequestMatcherTest extends \PHPUnit_Framework_TestCase $this->assertFalse($matcher->matches($request)); } - public function testPathWithLocale() + public function testPathWithLocaleIsNotSupported() { $matcher = new RequestMatcher(); $request = Request::create('/en/login'); @@ -152,9 +152,6 @@ class RequestMatcherTest extends \PHPUnit_Framework_TestCase $request->setSession($session); $matcher->matchPath('^/{_locale}/login$'); - $this->assertTrue($matcher->matches($request)); - - $session->setLocale('de'); $this->assertFalse($matcher->matches($request)); } diff --git a/tests/Symfony/Tests/Component/Security/Http/HttpUtilsTest.php b/tests/Symfony/Tests/Component/Security/Http/HttpUtilsTest.php index cfba0f0094..4bea58e8e2 100644 --- a/tests/Symfony/Tests/Component/Security/Http/HttpUtilsTest.php +++ b/tests/Symfony/Tests/Component/Security/Http/HttpUtilsTest.php @@ -61,6 +61,17 @@ class HttpUtilsTest extends \PHPUnit_Framework_TestCase $this->assertEquals('bar', $subRequest->server->get('Foo')); // route name + $utils = new HttpUtils($router = $this->getMockBuilder('Symfony\Component\Routing\Router')->disableOriginalConstructor()->getMock()); + $router + ->expects($this->once()) + ->method('generate') + ->will($this->returnValue('/foo/bar')) + ; + $router + ->expects($this->any()) + ->method('getContext') + ->will($this->returnValue($this->getMock('Symfony\Component\Routing\RequestContext'))) + ; $subRequest = $utils->createRequest($this->getRequest(), 'foobar'); $this->assertEquals('/foo/bar', $subRequest->getPathInfo());