From eed8a5143d324d5de1e5c50d2215c9a39d365d1d Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Tue, 18 Jul 2017 17:30:43 +0200 Subject: [PATCH 1/7] [VarDumper] Move locale sniffing to dump() time --- src/Symfony/Component/VarDumper/Dumper/AbstractDumper.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/VarDumper/Dumper/AbstractDumper.php b/src/Symfony/Component/VarDumper/Dumper/AbstractDumper.php index bed08bf47e..abbe962954 100644 --- a/src/Symfony/Component/VarDumper/Dumper/AbstractDumper.php +++ b/src/Symfony/Component/VarDumper/Dumper/AbstractDumper.php @@ -39,8 +39,8 @@ abstract class AbstractDumper implements DataDumperInterface, DumperInterface public function __construct($output = null, $charset = null) { $this->setCharset($charset ?: ini_get('php.output_encoding') ?: ini_get('default_charset') ?: 'UTF-8'); - $this->decimalPoint = (string) 0.5; - $this->decimalPoint = $this->decimalPoint[1]; + $this->decimalPoint = localeconv(); + $this->decimalPoint = $this->decimalPoint['decimal_point']; $this->setOutput($output ?: static::$defaultOutput); if (!$output && is_string(static::$defaultOutput)) { static::$defaultOutput = $this->outputStream; @@ -134,6 +134,9 @@ abstract class AbstractDumper implements DataDumperInterface, DumperInterface */ public function dump(Data $data, $output = null) { + $this->decimalPoint = localeconv(); + $this->decimalPoint = $this->decimalPoint['decimal_point']; + $exception = null; if ($output) { $prevOutput = $this->setOutput($output); From 04b7b04b65b7df233b56061b8dd3337317ecb544 Mon Sep 17 00:00:00 2001 From: Shaun Simmons Date: Tue, 18 Jul 2017 17:34:47 -0400 Subject: [PATCH 2/7] Change "this" to "that" to avoid confusion --- .../DependencyInjection/Compiler/PriorityTaggedServiceTrait.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php b/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php index 64c041e93f..04e6cc67a5 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php @@ -26,7 +26,7 @@ trait PriorityTaggedServiceTrait * * The order of additions must be respected for services having the same priority, * and knowing that the \SplPriorityQueue class does not respect the FIFO method, - * we should not use this class. + * we should not use that class. * * @see https://bugs.php.net/bug.php?id=53710 * @see https://bugs.php.net/bug.php?id=60926 From c0556cb20447208e277e26a851680802983e0ab9 Mon Sep 17 00:00:00 2001 From: David Maicher Date: Mon, 17 Jul 2017 22:35:32 +0200 Subject: [PATCH 3/7] [FrameworkBundle] fix ValidatorCacheWarmer: use serializing ArrayAdapter --- .../AbstractPhpFileCacheWarmer.php | 92 +++++++++++++++++++ .../CacheWarmer/AnnotationsCacheWarmer.php | 73 ++++----------- .../CacheWarmer/SerializerCacheWarmer.php | 59 +++--------- .../CacheWarmer/ValidatorCacheWarmer.php | 62 ++++--------- 4 files changed, 142 insertions(+), 144 deletions(-) create mode 100644 src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php new file mode 100644 index 0000000000..25801a7829 --- /dev/null +++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php @@ -0,0 +1,92 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Bundle\FrameworkBundle\CacheWarmer; + +use Psr\Cache\CacheItemPoolInterface; +use Symfony\Component\Cache\Adapter\AdapterInterface; +use Symfony\Component\Cache\Adapter\ArrayAdapter; +use Symfony\Component\Cache\Adapter\PhpArrayAdapter; +use Symfony\Component\Cache\Adapter\ProxyAdapter; +use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface; + +/** + * @internal + */ +abstract class AbstractPhpFileCacheWarmer implements CacheWarmerInterface +{ + private $phpArrayFile; + private $fallbackPool; + + /** + * @param string $phpArrayFile The PHP file where metadata are cached + * @param CacheItemPoolInterface $fallbackPool The pool where runtime-discovered metadata are cached + */ + public function __construct($phpArrayFile, CacheItemPoolInterface $fallbackPool) + { + $this->phpArrayFile = $phpArrayFile; + if (!$fallbackPool instanceof AdapterInterface) { + $fallbackPool = new ProxyAdapter($fallbackPool); + } + $this->fallbackPool = $fallbackPool; + } + + /** + * {@inheritdoc} + */ + public function isOptional() + { + return true; + } + + /** + * {@inheritdoc} + */ + public function warmUp($cacheDir) + { + $arrayAdapter = new ArrayAdapter(); + + spl_autoload_register(array(PhpArrayAdapter::class, 'throwOnRequiredClass')); + try { + if (!$this->doWarmUp($cacheDir, $arrayAdapter)) { + return; + } + } finally { + spl_autoload_unregister(array(PhpArrayAdapter::class, 'throwOnRequiredClass')); + } + + // the ArrayAdapter stores the values serialized + // to avoid mutation of the data after it was written to the cache + // so here we un-serialize the values first + $values = array_map(function ($val) { return null !== $val ? unserialize($val) : null; }, $arrayAdapter->getValues()); + + $this->warmUpPhpArrayAdapter(new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool), $values); + + foreach ($values as $k => $v) { + $item = $this->fallbackPool->getItem($k); + $this->fallbackPool->saveDeferred($item->set($v)); + } + $this->fallbackPool->commit(); + } + + protected function warmUpPhpArrayAdapter(PhpArrayAdapter $phpArrayAdapter, array $values) + { + $phpArrayAdapter->warmUp($values); + } + + /** + * @param string $cacheDir + * @param ArrayAdapter $arrayAdapter + * + * @return bool false if there is nothing to warm-up + */ + abstract protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter); +} diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php index a6fb4ed095..4026b53bd7 100644 --- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php +++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php @@ -15,12 +15,8 @@ use Doctrine\Common\Annotations\AnnotationException; use Doctrine\Common\Annotations\CachedReader; use Doctrine\Common\Annotations\Reader; use Psr\Cache\CacheItemPoolInterface; -use Symfony\Component\Cache\Adapter\AdapterInterface; use Symfony\Component\Cache\Adapter\ArrayAdapter; -use Symfony\Component\Cache\Adapter\PhpArrayAdapter; -use Symfony\Component\Cache\Adapter\ProxyAdapter; use Symfony\Component\Cache\DoctrineProvider; -use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface; /** * Warms up annotation caches for classes found in composer's autoload class map @@ -28,11 +24,9 @@ use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface; * * @author Titouan Galopin */ -class AnnotationsCacheWarmer implements CacheWarmerInterface +class AnnotationsCacheWarmer extends AbstractPhpFileCacheWarmer { private $annotationReader; - private $phpArrayFile; - private $fallbackPool; /** * @param Reader $annotationReader @@ -41,70 +35,41 @@ class AnnotationsCacheWarmer implements CacheWarmerInterface */ public function __construct(Reader $annotationReader, $phpArrayFile, CacheItemPoolInterface $fallbackPool) { + parent::__construct($phpArrayFile, $fallbackPool); $this->annotationReader = $annotationReader; - $this->phpArrayFile = $phpArrayFile; - if (!$fallbackPool instanceof AdapterInterface) { - $fallbackPool = new ProxyAdapter($fallbackPool); - } - $this->fallbackPool = $fallbackPool; } /** * {@inheritdoc} */ - public function warmUp($cacheDir) + protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter) { - $adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool); $annotatedClassPatterns = $cacheDir.'/annotations.map'; if (!is_file($annotatedClassPatterns)) { - $adapter->warmUp(array()); - - return; + return true; } $annotatedClasses = include $annotatedClassPatterns; + $reader = new CachedReader($this->annotationReader, new DoctrineProvider($arrayAdapter)); - $arrayPool = new ArrayAdapter(0, false); - $reader = new CachedReader($this->annotationReader, new DoctrineProvider($arrayPool)); - - spl_autoload_register(array($adapter, 'throwOnRequiredClass')); - try { - foreach ($annotatedClasses as $class) { - try { - $this->readAllComponents($reader, $class); - } catch (\ReflectionException $e) { - // ignore failing reflection - } catch (AnnotationException $e) { - /* - * Ignore any AnnotationException to not break the cache warming process if an Annotation is badly - * configured or could not be found / read / etc. - * - * In particular cases, an Annotation in your code can be used and defined only for a specific - * environment but is always added to the annotations.map file by some Symfony default behaviors, - * and you always end up with a not found Annotation. - */ - } + foreach ($annotatedClasses as $class) { + try { + $this->readAllComponents($reader, $class); + } catch (\ReflectionException $e) { + // ignore failing reflection + } catch (AnnotationException $e) { + /* + * Ignore any AnnotationException to not break the cache warming process if an Annotation is badly + * configured or could not be found / read / etc. + * + * In particular cases, an Annotation in your code can be used and defined only for a specific + * environment but is always added to the annotations.map file by some Symfony default behaviors, + * and you always end up with a not found Annotation. + */ } - } finally { - spl_autoload_unregister(array($adapter, 'throwOnRequiredClass')); } - $values = $arrayPool->getValues(); - $adapter->warmUp($values); - - foreach ($values as $k => $v) { - $item = $this->fallbackPool->getItem($k); - $this->fallbackPool->saveDeferred($item->set($v)); - } - $this->fallbackPool->commit(); - } - - /** - * {@inheritdoc} - */ - public function isOptional() - { return true; } diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php index c017f51268..75cc2bcf9b 100644 --- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php +++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/SerializerCacheWarmer.php @@ -13,11 +13,7 @@ namespace Symfony\Bundle\FrameworkBundle\CacheWarmer; use Doctrine\Common\Annotations\AnnotationException; use Psr\Cache\CacheItemPoolInterface; -use Symfony\Component\Cache\Adapter\AdapterInterface; use Symfony\Component\Cache\Adapter\ArrayAdapter; -use Symfony\Component\Cache\Adapter\PhpArrayAdapter; -use Symfony\Component\Cache\Adapter\ProxyAdapter; -use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface; use Symfony\Component\Serializer\Mapping\Factory\CacheClassMetadataFactory; use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactory; use Symfony\Component\Serializer\Mapping\Loader\LoaderChain; @@ -30,11 +26,9 @@ use Symfony\Component\Serializer\Mapping\Loader\YamlFileLoader; * * @author Titouan Galopin */ -class SerializerCacheWarmer implements CacheWarmerInterface +class SerializerCacheWarmer extends AbstractPhpFileCacheWarmer { private $loaders; - private $phpArrayFile; - private $fallbackPool; /** * @param LoaderInterface[] $loaders The serializer metadata loaders @@ -43,60 +37,33 @@ class SerializerCacheWarmer implements CacheWarmerInterface */ public function __construct(array $loaders, $phpArrayFile, CacheItemPoolInterface $fallbackPool) { + parent::__construct($phpArrayFile, $fallbackPool); $this->loaders = $loaders; - $this->phpArrayFile = $phpArrayFile; - if (!$fallbackPool instanceof AdapterInterface) { - $fallbackPool = new ProxyAdapter($fallbackPool); - } - $this->fallbackPool = $fallbackPool; } /** * {@inheritdoc} */ - public function warmUp($cacheDir) + protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter) { if (!class_exists(CacheClassMetadataFactory::class) || !method_exists(XmlFileLoader::class, 'getMappedClasses') || !method_exists(YamlFileLoader::class, 'getMappedClasses')) { - return; + return false; } - $adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool); - $arrayPool = new ArrayAdapter(0, false); + $metadataFactory = new CacheClassMetadataFactory(new ClassMetadataFactory(new LoaderChain($this->loaders)), $arrayAdapter); - $metadataFactory = new CacheClassMetadataFactory(new ClassMetadataFactory(new LoaderChain($this->loaders)), $arrayPool); - - spl_autoload_register(array($adapter, 'throwOnRequiredClass')); - try { - foreach ($this->extractSupportedLoaders($this->loaders) as $loader) { - foreach ($loader->getMappedClasses() as $mappedClass) { - try { - $metadataFactory->getMetadataFor($mappedClass); - } catch (\ReflectionException $e) { - // ignore failing reflection - } catch (AnnotationException $e) { - // ignore failing annotations - } + foreach ($this->extractSupportedLoaders($this->loaders) as $loader) { + foreach ($loader->getMappedClasses() as $mappedClass) { + try { + $metadataFactory->getMetadataFor($mappedClass); + } catch (\ReflectionException $e) { + // ignore failing reflection + } catch (AnnotationException $e) { + // ignore failing annotations } } - } finally { - spl_autoload_unregister(array($adapter, 'throwOnRequiredClass')); } - $values = $arrayPool->getValues(); - $adapter->warmUp($values); - - foreach ($values as $k => $v) { - $item = $this->fallbackPool->getItem($k); - $this->fallbackPool->saveDeferred($item->set($v)); - } - $this->fallbackPool->commit(); - } - - /** - * {@inheritdoc} - */ - public function isOptional() - { return true; } diff --git a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php index 81291d772f..20f76cb4fd 100644 --- a/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php +++ b/src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php @@ -13,11 +13,8 @@ namespace Symfony\Bundle\FrameworkBundle\CacheWarmer; use Doctrine\Common\Annotations\AnnotationException; use Psr\Cache\CacheItemPoolInterface; -use Symfony\Component\Cache\Adapter\AdapterInterface; use Symfony\Component\Cache\Adapter\ArrayAdapter; use Symfony\Component\Cache\Adapter\PhpArrayAdapter; -use Symfony\Component\Cache\Adapter\ProxyAdapter; -use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface; use Symfony\Component\Validator\Mapping\Cache\Psr6Cache; use Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory; use Symfony\Component\Validator\Mapping\Loader\LoaderChain; @@ -31,11 +28,9 @@ use Symfony\Component\Validator\ValidatorBuilderInterface; * * @author Titouan Galopin */ -class ValidatorCacheWarmer implements CacheWarmerInterface +class ValidatorCacheWarmer extends AbstractPhpFileCacheWarmer { private $validatorBuilder; - private $phpArrayFile; - private $fallbackPool; /** * @param ValidatorBuilderInterface $validatorBuilder @@ -44,64 +39,43 @@ class ValidatorCacheWarmer implements CacheWarmerInterface */ public function __construct(ValidatorBuilderInterface $validatorBuilder, $phpArrayFile, CacheItemPoolInterface $fallbackPool) { + parent::__construct($phpArrayFile, $fallbackPool); $this->validatorBuilder = $validatorBuilder; - $this->phpArrayFile = $phpArrayFile; - if (!$fallbackPool instanceof AdapterInterface) { - $fallbackPool = new ProxyAdapter($fallbackPool); - } - $this->fallbackPool = $fallbackPool; } /** * {@inheritdoc} */ - public function warmUp($cacheDir) + protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter) { if (!method_exists($this->validatorBuilder, 'getLoaders')) { - return; + return false; } - $adapter = new PhpArrayAdapter($this->phpArrayFile, $this->fallbackPool); - $arrayPool = new ArrayAdapter(0, false); - $loaders = $this->validatorBuilder->getLoaders(); - $metadataFactory = new LazyLoadingMetadataFactory(new LoaderChain($loaders), new Psr6Cache($arrayPool)); + $metadataFactory = new LazyLoadingMetadataFactory(new LoaderChain($loaders), new Psr6Cache($arrayAdapter)); - spl_autoload_register(array($adapter, 'throwOnRequiredClass')); - try { - foreach ($this->extractSupportedLoaders($loaders) as $loader) { - foreach ($loader->getMappedClasses() as $mappedClass) { - try { - if ($metadataFactory->hasMetadataFor($mappedClass)) { - $metadataFactory->getMetadataFor($mappedClass); - } - } catch (\ReflectionException $e) { - // ignore failing reflection - } catch (AnnotationException $e) { - // ignore failing annotations + foreach ($this->extractSupportedLoaders($loaders) as $loader) { + foreach ($loader->getMappedClasses() as $mappedClass) { + try { + if ($metadataFactory->hasMetadataFor($mappedClass)) { + $metadataFactory->getMetadataFor($mappedClass); } + } catch (\ReflectionException $e) { + // ignore failing reflection + } catch (AnnotationException $e) { + // ignore failing annotations } } - } finally { - spl_autoload_unregister(array($adapter, 'throwOnRequiredClass')); } - $values = $arrayPool->getValues(); - $adapter->warmUp(array_filter($values)); - - foreach ($values as $k => $v) { - $item = $this->fallbackPool->getItem($k); - $this->fallbackPool->saveDeferred($item->set($v)); - } - $this->fallbackPool->commit(); + return true; } - /** - * {@inheritdoc} - */ - public function isOptional() + protected function warmUpPhpArrayAdapter(PhpArrayAdapter $phpArrayAdapter, array $values) { - return true; + // make sure we don't cache null values + parent::warmUpPhpArrayAdapter($phpArrayAdapter, array_filter($values)); } /** From 338761245100e09dafdce47f7646ef7d4fc14f64 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 19 Jul 2017 07:07:01 +0200 Subject: [PATCH 4/7] [Security] refactored tests --- ...efaultAuthenticationSuccessHandlerTest.php | 80 +++++++------------ 1 file changed, 31 insertions(+), 49 deletions(-) diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php index 2c133014cc..70836170c7 100644 --- a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php @@ -12,31 +12,28 @@ namespace Symfony\Component\Security\Http\Tests\Authentication; use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler; class DefaultAuthenticationSuccessHandlerTest extends TestCase { private $httpUtils = null; - - private $request = null; - private $token = null; protected function setUp() { $this->httpUtils = $this->getMockBuilder('Symfony\Component\Security\Http\HttpUtils')->getMock(); - $this->request = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')->getMock(); - $this->request->headers = $this->getMockBuilder('Symfony\Component\HttpFoundation\HeaderBag')->getMock(); $this->token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock(); } public function testRequestIsRedirected() { - $response = $this->expectRedirectResponse('/'); + $request = Request::create('/'); + $response = $this->expectRedirectResponse($request, '/'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } @@ -48,24 +45,22 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase 'default_target_path' => '/dashboard', ); - $response = $this->expectRedirectResponse('/dashboard'); + $request = Request::create('/'); + $response = $this->expectRedirectResponse($request, '/dashboard'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } public function testTargetPathIsPassedWithRequest() { - $this->request->expects($this->once()) - ->method('get')->with('_target_path') - ->will($this->returnValue('/dashboard')); - - $response = $this->expectRedirectResponse('/dashboard'); + $request = Request::create('/?_target_path=/dashboard'); + $response = $this->expectRedirectResponse($request, '/dashboard'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } @@ -73,15 +68,11 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase public function testTargetPathParameterIsCustomised() { $options = array('target_path_parameter' => '_my_target_path'); - - $this->request->expects($this->once()) - ->method('get')->with('_my_target_path') - ->will($this->returnValue('/dashboard')); - - $response = $this->expectRedirectResponse('/dashboard'); + $request = Request::create('/?_my_target_path=/dashboard'); + $response = $this->expectRedirectResponse($request, '/dashboard'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } @@ -95,16 +86,14 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase $session->expects($this->once()) ->method('remove')->with('_security.admin.target_path'); - $this->request->expects($this->any()) - ->method('getSession') - ->will($this->returnValue($session)); - - $response = $this->expectRedirectResponse('/admin/dashboard'); + $request = Request::create('/?_my_target_path=/dashboard'); + $request->setSession($session); + $response = $this->expectRedirectResponse($request, '/admin/dashboard'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); $handler->setProviderKey('admin'); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } @@ -112,15 +101,12 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase public function testTargetPathIsPassedAsReferer() { $options = array('use_referer' => true); - - $this->request->headers->expects($this->once()) - ->method('get')->with('Referer') - ->will($this->returnValue('/dashboard')); - - $response = $this->expectRedirectResponse('/dashboard'); + $request = Request::create('/'); + $request->headers->set('Referer', '/dashboard'); + $response = $this->expectRedirectResponse($request, '/dashboard'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } @@ -128,41 +114,37 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase public function testRefererHasToBeDifferentThatLoginUrl() { $options = array('use_referer' => true); - - $this->request->headers->expects($this->any()) - ->method('get')->with('Referer') - ->will($this->returnValue('/login')); - + $request = Request::create('/'); + $request->headers->set('Referer', '/login'); $this->httpUtils->expects($this->once()) - ->method('generateUri')->with($this->request, '/login') + ->method('generateUri')->with($request, '/login') ->will($this->returnValue('/login')); - $response = $this->expectRedirectResponse('/'); + $response = $this->expectRedirectResponse($request, '/'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } public function testRefererTargetPathIsIgnoredByDefault() { - $this->request->headers->expects($this->never())->method('get'); - - $response = $this->expectRedirectResponse('/'); + $request = Request::create('/'); + $response = $this->expectRedirectResponse($request, '/'); $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); - $result = $handler->onAuthenticationSuccess($this->request, $this->token); + $result = $handler->onAuthenticationSuccess($request, $this->token); $this->assertSame($response, $result); } - private function expectRedirectResponse($path) + private function expectRedirectResponse(Request $request, $path) { $response = new Response(); $this->httpUtils->expects($this->once()) ->method('createRedirectResponse') - ->with($this->request, $path) + ->with($request, $path) ->will($this->returnValue($response)); return $response; From b1f1ae26b47e6aaf5282584d880724fed66a86df Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 19 Jul 2017 07:42:44 +0200 Subject: [PATCH 5/7] [Security] simplified tests --- ...efaultAuthenticationSuccessHandlerTest.php | 193 +++++++----------- 1 file changed, 69 insertions(+), 124 deletions(-) diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php index 70836170c7..8d39e63cd5 100644 --- a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php @@ -15,138 +15,83 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler; +use Symfony\Component\Security\Http\HttpUtils; class DefaultAuthenticationSuccessHandlerTest extends TestCase { - private $httpUtils = null; - private $token = null; - - protected function setUp() + /** + * @dataProvider getRequestRedirections + */ + public function testRequestRedirections(Request $request, $options, $redirectedUrl) { - $this->httpUtils = $this->getMockBuilder('Symfony\Component\Security\Http\HttpUtils')->getMock(); - $this->token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock(); + $httpUtils = new HttpUtils(); + $token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock(); + $handler = new DefaultAuthenticationSuccessHandler($httpUtils, $options); + if ($request->hasSession()) { + $handler->setProviderKey('admin'); + } + $this->assertSame('http://localhost'.$redirectedUrl, $handler->onAuthenticationSuccess($request, $token)->getTargetUrl()); } - public function testRequestIsRedirected() - { - $request = Request::create('/'); - $response = $this->expectRedirectResponse($request, '/'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - public function testDefaultTargetPathCanBeForced() - { - $options = array( - 'always_use_default_target_path' => true, - 'default_target_path' => '/dashboard', - ); - - $request = Request::create('/'); - $response = $this->expectRedirectResponse($request, '/dashboard'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - public function testTargetPathIsPassedWithRequest() - { - $request = Request::create('/?_target_path=/dashboard'); - $response = $this->expectRedirectResponse($request, '/dashboard'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - public function testTargetPathParameterIsCustomised() - { - $options = array('target_path_parameter' => '_my_target_path'); - $request = Request::create('/?_my_target_path=/dashboard'); - $response = $this->expectRedirectResponse($request, '/dashboard'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - public function testTargetPathIsTakenFromTheSession() + public function getRequestRedirections() { $session = $this->getMockBuilder('Symfony\Component\HttpFoundation\Session\SessionInterface')->getMock(); - $session->expects($this->once()) - ->method('get')->with('_security.admin.target_path') - ->will($this->returnValue('/admin/dashboard')); - $session->expects($this->once()) - ->method('remove')->with('_security.admin.target_path'); + $session->expects($this->once())->method('get')->with('_security.admin.target_path')->will($this->returnValue('/admin/dashboard')); + $session->expects($this->once())->method('remove')->with('_security.admin.target_path'); + $requestWithSession = Request::create('/'); + $requestWithSession->setSession($session); - $request = Request::create('/?_my_target_path=/dashboard'); - $request->setSession($session); - $response = $this->expectRedirectResponse($request, '/admin/dashboard'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); - $handler->setProviderKey('admin'); - - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - public function testTargetPathIsPassedAsReferer() - { - $options = array('use_referer' => true); - $request = Request::create('/'); - $request->headers->set('Referer', '/dashboard'); - $response = $this->expectRedirectResponse($request, '/dashboard'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - public function testRefererHasToBeDifferentThatLoginUrl() - { - $options = array('use_referer' => true); - $request = Request::create('/'); - $request->headers->set('Referer', '/login'); - $this->httpUtils->expects($this->once()) - ->method('generateUri')->with($request, '/login') - ->will($this->returnValue('/login')); - - $response = $this->expectRedirectResponse($request, '/'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options); - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - public function testRefererTargetPathIsIgnoredByDefault() - { - $request = Request::create('/'); - $response = $this->expectRedirectResponse($request, '/'); - - $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array()); - $result = $handler->onAuthenticationSuccess($request, $this->token); - - $this->assertSame($response, $result); - } - - private function expectRedirectResponse(Request $request, $path) - { - $response = new Response(); - $this->httpUtils->expects($this->once()) - ->method('createRedirectResponse') - ->with($request, $path) - ->will($this->returnValue($response)); - - return $response; + return array( + 'default' => array( + Request::create('/'), + array(), + '/', + ), + 'forced target path' => array( + Request::create('/'), + array('always_use_default_target_path' => true, 'default_target_path' => '/dashboard'), + '/dashboard', + ), + 'target path as query string' => array( + Request::create('/?_target_path=/dashboard'), + array(), + '/dashboard', + ), + 'target path name as query string is customized' => array( + Request::create('/?_my_target_path=/dashboard'), + array('target_path_parameter' => '_my_target_path'), + '/dashboard', + ), + 'target path name as query string is customized and nested' => array( + Request::create('/?_target_path[value]=/dashboard'), + array('target_path_parameter' => '_target_path[value]'), + '/dashboard', + ), + 'target path in session' => array( + $requestWithSession, + array(), + '/admin/dashboard', + ), + 'target path as referer' => array( + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')), + array('use_referer' => true), + '/dashboard', + ), + 'target path as referer is ignored if not configured' => array( + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')), + array(), + '/', + ), + 'target path should be different than login URL' => array( + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login')), + array('use_referer' => true, 'login_path' => '/login'), + '/', + ), + 'target path should be different than login URL (query string does not matter)' => array( + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')), + array('use_referer' => true, 'login_path' => '/login'), + '/', + ), + ); } } From 9c7a1406cb81f4d1ccd7e3521347e2d847b7467c Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 19 Jul 2017 07:57:00 +0200 Subject: [PATCH 6/7] [Security] fixed default target path when referer contains a query string --- .../DefaultAuthenticationSuccessHandler.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php index b6a7df5062..7da6e35572 100644 --- a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php +++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php @@ -118,8 +118,14 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle return $targetUrl; } - if ($this->options['use_referer'] && ($targetUrl = $request->headers->get('Referer')) && $targetUrl !== $this->httpUtils->generateUri($request, $this->options['login_path'])) { - return $targetUrl; + if ($this->options['use_referer']) { + $targetUrl = $request->headers->get('Referer'); + if (false !== $pos = strpos($targetUrl, '?')) { + $targetUrl = substr($targetUrl, 0, $pos); + } + if ($targetUrl !== $this->httpUtils->generateUri($request, $this->options['login_path'])) { + return $targetUrl; + } } return $this->options['default_target_path']; From 022ac0be096a303aa4de7be64b84be9ffee3ecd9 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 19 Jul 2017 08:04:16 +0200 Subject: [PATCH 7/7] [Security] added more tests --- ...DefaultAuthenticationSuccessHandlerTest.php | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php index 8d39e63cd5..b42f840358 100644 --- a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php @@ -13,7 +13,6 @@ namespace Symfony\Component\Security\Http\Tests\Authentication; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler; use Symfony\Component\Security\Http\HttpUtils; @@ -24,7 +23,9 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase */ public function testRequestRedirections(Request $request, $options, $redirectedUrl) { - $httpUtils = new HttpUtils(); + $urlGenerator = $this->getMockBuilder('Symfony\Component\Routing\Generator\UrlGeneratorInterface')->getMock(); + $urlGenerator->expects($this->any())->method('generate')->will($this->returnValue('http://localhost/login')); + $httpUtils = new HttpUtils($urlGenerator); $token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock(); $handler = new DefaultAuthenticationSuccessHandler($httpUtils, $options); if ($request->hasSession()) { @@ -73,25 +74,30 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase '/admin/dashboard', ), 'target path as referer' => array( - Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')), + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')), array('use_referer' => true), '/dashboard', ), 'target path as referer is ignored if not configured' => array( - Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')), + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')), array(), '/', ), 'target path should be different than login URL' => array( - Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login')), + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login')), array('use_referer' => true, 'login_path' => '/login'), '/', ), 'target path should be different than login URL (query string does not matter)' => array( - Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')), + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')), array('use_referer' => true, 'login_path' => '/login'), '/', ), + 'target path should be different than login URL (login_path as a route)' => array( + Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')), + array('use_referer' => true, 'login_path' => 'login_route'), + '/', + ), ); } }