From 0f6a99936b0dd2afd0c4a091f5c42367adcfefe3 Mon Sep 17 00:00:00 2001 From: Robin Chalas Date: Fri, 6 Mar 2020 14:55:51 +0100 Subject: [PATCH 1/2] [Security] Fix access_control behavior with unanimous decision strategy --- .../Authorization/AccessDecisionManager.php | 9 +++- .../Security/Http/Firewall/AccessListener.php | 10 +---- .../Tests/Firewall/AccessListenerTest.php | 41 +++++++++++++++++++ .../Component/Security/Http/composer.json | 2 +- 4 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php index 3b55e7bffaa5..1a6c9cbb81f2 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php @@ -53,11 +53,16 @@ public function __construct(iterable $voters = [], string $strategy = self::STRA } /** + * @param bool $allowMultipleAttributes Whether to allow passing multiple values to the $attributes array + * * {@inheritdoc} */ - public function decide(TokenInterface $token, array $attributes, $object = null) + public function decide(TokenInterface $token, array $attributes, $object = null/*, bool $allowMultipleAttributes = false*/) { - if (\count($attributes) > 1) { + $allowMultipleAttributes = 3 < func_num_args() && func_get_arg(3); + + // Special case for AccessListener, do not remove the right side of the condition before 6.0 + if (\count($attributes) > 1 && !$allowMultipleAttributes) { @trigger_error(sprintf('Passing more than one Security attribute to "%s()" is deprecated since Symfony 4.4. Use multiple "decide()" calls or the expression language (e.g. "is_granted(...) or is_granted(...)") instead.', __METHOD__), E_USER_DEPRECATED); } diff --git a/src/Symfony/Component/Security/Http/Firewall/AccessListener.php b/src/Symfony/Component/Security/Http/Firewall/AccessListener.php index 00673f60aba2..b294456853e8 100644 --- a/src/Symfony/Component/Security/Http/Firewall/AccessListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/AccessListener.php @@ -87,15 +87,7 @@ public function authenticate(RequestEvent $event) $this->tokenStorage->setToken($token); } - $granted = false; - foreach ($attributes as $key => $value) { - if ($this->accessDecisionManager->decide($token, [$key => $value], $request)) { - $granted = true; - break; - } - } - - if (!$granted) { + if (!$this->accessDecisionManager->decide($token, $attributes, $request, true)) { $exception = new AccessDeniedException(); $exception->setAttributes($attributes); $exception->setSubject($request); diff --git a/src/Symfony/Component/Security/Http/Tests/Firewall/AccessListenerTest.php b/src/Symfony/Component/Security/Http/Tests/Firewall/AccessListenerTest.php index 168e25643705..75798d055a38 100644 --- a/src/Symfony/Component/Security/Http/Tests/Firewall/AccessListenerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/Firewall/AccessListenerTest.php @@ -16,6 +16,7 @@ use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface; +use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; use Symfony\Component\Security\Http\AccessMapInterface; @@ -227,4 +228,44 @@ public function testHandleWhenTheSecurityTokenStorageHasNoToken() $listener(new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MASTER_REQUEST)); } + + public function testHandleMWithultipleAttributesShouldBeHandledAsAnd() + { + $request = new Request(); + + $accessMap = $this->getMockBuilder('Symfony\Component\Security\Http\AccessMapInterface')->getMock(); + $accessMap + ->expects($this->any()) + ->method('getPatterns') + ->with($this->equalTo($request)) + ->willReturn([['foo' => 'bar', 'bar' => 'baz'], null]) + ; + + $authenticatedToken = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock(); + $authenticatedToken + ->expects($this->any()) + ->method('isAuthenticated') + ->willReturn(true) + ; + + $tokenStorage = new TokenStorage(); + $tokenStorage->setToken($authenticatedToken); + + $accessDecisionManager = $this->getMockBuilder('Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface')->getMock(); + $accessDecisionManager + ->expects($this->once()) + ->method('decide') + ->with($this->equalTo($authenticatedToken), $this->equalTo(['foo' => 'bar', 'bar' => 'baz']), $this->equalTo($request), true) + ->willReturn(true) + ; + + $listener = new AccessListener( + $tokenStorage, + $accessDecisionManager, + $accessMap, + $this->createMock('Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface') + ); + + $listener(new RequestEvent($this->createMock(HttpKernelInterface::class), $request, HttpKernelInterface::MASTER_REQUEST)); + } } diff --git a/src/Symfony/Component/Security/Http/composer.json b/src/Symfony/Component/Security/Http/composer.json index 7c9cdb484f10..699ffcf7032b 100644 --- a/src/Symfony/Component/Security/Http/composer.json +++ b/src/Symfony/Component/Security/Http/composer.json @@ -17,7 +17,7 @@ ], "require": { "php": "^7.1.3", - "symfony/security-core": "^4.4", + "symfony/security-core": "^4.4.7", "symfony/http-foundation": "^3.4.40|^4.4.7|^5.0.7", "symfony/http-kernel": "^4.4", "symfony/property-access": "^3.4|^4.0|^5.0" From 0050a4dafb18a5a616cc33fe5fc8027b03045158 Mon Sep 17 00:00:00 2001 From: Yonel Ceruto Date: Tue, 11 Feb 2020 09:34:35 -0500 Subject: [PATCH 2/2] [HttpFoundation] Do not set the default Content-Type based on the Accept header --- .../ErrorRenderer/SerializerErrorRenderer.php | 9 ++++++++- src/Symfony/Component/HttpFoundation/Request.php | 4 +++- src/Symfony/Component/HttpFoundation/Response.php | 2 +- .../HttpFoundation/Tests/ResponseTest.php | 15 ++++++++++++++- 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/ErrorHandler/ErrorRenderer/SerializerErrorRenderer.php b/src/Symfony/Component/ErrorHandler/ErrorRenderer/SerializerErrorRenderer.php index d1dd652a6497..e0640850a691 100644 --- a/src/Symfony/Component/ErrorHandler/ErrorRenderer/SerializerErrorRenderer.php +++ b/src/Symfony/Component/ErrorHandler/ErrorRenderer/SerializerErrorRenderer.php @@ -12,6 +12,7 @@ namespace Symfony\Component\ErrorHandler\ErrorRenderer; use Symfony\Component\ErrorHandler\Exception\FlattenException; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Serializer\Exception\NotEncodableValueException; use Symfony\Component\Serializer\SerializerInterface; @@ -30,6 +31,7 @@ class SerializerErrorRenderer implements ErrorRendererInterface /** * @param string|callable(FlattenException) $format The format as a string or a callable that should return it + * formats not supported by Request::getMimeTypes() should be given as mime types * @param bool|callable $debug The debugging mode as a boolean or a callable that should return it */ public function __construct(SerializerInterface $serializer, $format, ErrorRendererInterface $fallbackErrorRenderer = null, $debug = false) @@ -57,11 +59,16 @@ public function render(\Throwable $exception): FlattenException try { $format = \is_string($this->format) ? $this->format : ($this->format)($flattenException); + $headers = [ + 'Content-Type' => Request::getMimeTypes($format)[0] ?? $format, + 'Vary' => 'Accept', + ]; return $flattenException->setAsString($this->serializer->serialize($flattenException, $format, [ 'exception' => $exception, 'debug' => \is_bool($this->debug) ? $this->debug : ($this->debug)($exception), - ])); + ])) + ->setHeaders($flattenException->getHeaders() + $headers); } catch (NotEncodableValueException $e) { return $this->fallbackErrorRenderer->render($exception); } diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index a5e50eb795a5..33f4efb5ed9e 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -1590,7 +1590,9 @@ public function isNoCache() * Gets the preferred format for the response by inspecting, in the following order: * * the request format set using setRequestFormat * * the values of the Accept HTTP header - * * the content type of the body of the request. + * + * Note that if you use this method, you should send the "Vary: Accept" header + * in the response to prevent any issues with intermediary HTTP caches. */ public function getPreferredFormat(?string $default = 'html'): ?string { diff --git a/src/Symfony/Component/HttpFoundation/Response.php b/src/Symfony/Component/HttpFoundation/Response.php index 714109d53669..b9177934a47a 100644 --- a/src/Symfony/Component/HttpFoundation/Response.php +++ b/src/Symfony/Component/HttpFoundation/Response.php @@ -275,7 +275,7 @@ public function prepare(Request $request) } else { // Content-type based on the Request if (!$headers->has('Content-Type')) { - $format = $request->getPreferredFormat(null); + $format = $request->getRequestFormat(null); if (null !== $format && $mimeType = $request->getMimeType($format)) { $headers->set('Content-Type', $mimeType); } diff --git a/src/Symfony/Component/HttpFoundation/Tests/ResponseTest.php b/src/Symfony/Component/HttpFoundation/Tests/ResponseTest.php index d5da59ad7e80..c766f88a294a 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/ResponseTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/ResponseTest.php @@ -497,12 +497,25 @@ public function testPrepareDoesNothingIfRequestFormatIsNotDefined() $this->assertEquals('text/html; charset=UTF-8', $response->headers->get('content-type')); } + /** + * Same URL cannot produce different Content-Type based on the value of the Accept header, + * unless explicitly stated in the response object. + */ + public function testPrepareDoesNotSetContentTypeBasedOnRequestAcceptHeader() + { + $response = new Response('foo'); + $request = Request::create('/'); + $request->headers->set('Accept', 'application/json'); + $response->prepare($request); + + $this->assertSame('text/html; charset=UTF-8', $response->headers->get('content-type')); + } + public function testPrepareSetContentType() { $response = new Response('foo'); $request = Request::create('/'); $request->setRequestFormat('json'); - $request->headers->remove('accept'); $response->prepare($request);