From 0a2d6b3b3e4f87c763b4d3274c93a76d99a42d48 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Mon, 2 Oct 2017 15:41:13 -0400 Subject: [PATCH] Changing how IsGranted works so that it can use all controller args as subject --- EventListener/IsGrantedListener.php | 41 +++++++++--- Resources/config/security.xml | 1 + Tests/EventListener/IsGrantedListenerTest.php | 67 ++++++++++++++----- .../Controller/IsGrantedController.php | 47 +++++++++++++ .../FooBundle/Security/IsGrantedVoter.php | 41 ++++++++++++ Tests/Fixtures/TestKernel.php | 1 + Tests/Fixtures/config/config.yml | 28 ++++++++ Tests/Functional/IsGrantedTest.php | 40 +++++++++++ 8 files changed, 239 insertions(+), 27 deletions(-) create mode 100644 Tests/Fixtures/FooBundle/Controller/IsGrantedController.php create mode 100644 Tests/Fixtures/FooBundle/Security/IsGrantedVoter.php create mode 100644 Tests/Functional/IsGrantedTest.php diff --git a/EventListener/IsGrantedListener.php b/EventListener/IsGrantedListener.php index 09e6f75c..d11a2eab 100644 --- a/EventListener/IsGrantedListener.php +++ b/EventListener/IsGrantedListener.php @@ -13,7 +13,8 @@ use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; use Symfony\Component\EventDispatcher\EventSubscriberInterface; -use Symfony\Component\HttpKernel\Event\FilterControllerEvent; +use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactoryInterface; +use Symfony\Component\HttpKernel\Event\FilterControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; @@ -26,14 +27,16 @@ */ class IsGrantedListener implements EventSubscriberInterface { + private $argumentMetadataFactory; private $authChecker; - public function __construct(AuthorizationCheckerInterface $authChecker = null) + public function __construct(ArgumentMetadataFactoryInterface $argumentMetadataFactory, AuthorizationCheckerInterface $authChecker = null) { + $this->argumentMetadataFactory = $argumentMetadataFactory; $this->authChecker = $authChecker; } - public function onKernelController(FilterControllerEvent $event) + public function onKernelControllerArguments(FilterControllerArgumentsEvent $event) { $request = $event->getRequest(); /** @var $configurations IsGranted[] */ @@ -42,20 +45,19 @@ public function onKernelController(FilterControllerEvent $event) } if (null === $this->authChecker) { - throw new \LogicException('To use the @IsGranted tag, you need to install symfony/security-bundle.'); + throw new \LogicException('To use the @IsGranted tag, you need to install symfony/security-bundle and configure your security system.'); } + $arguments = $this->getArguments($event); + foreach ($configurations as $configuration) { $subject = null; if ($configuration->getSubject()) { - if (!$request->attributes->has($configuration->getSubject())) { - $allAttributes = $request->attributes->all(); - // remove one attribute we know is noise in the error message - unset($allAttributes['_is_granted']); - throw new \RuntimeException(sprintf('Could not find the subject "%s" for the @IsGranted annotation. Available attributes are: %s', $configuration->getSubject(), implode(', ', array_keys($allAttributes)))); + if (!isset($arguments[$configuration->getSubject()])) { + throw new \RuntimeException(sprintf('Could not find the subject "%s" for the @IsGranted annotation. Try adding a "$%s" argument to your controller method.', $configuration->getSubject(), $configuration->getSubject())); } - $subject = $request->attributes->get($configuration->getSubject()); + $subject = $arguments[$configuration->getSubject()]; } if (!$this->authChecker->isGranted($configuration->getAttributes(), $subject)) { @@ -72,6 +74,23 @@ public function onKernelController(FilterControllerEvent $event) } } + private function getArguments(FilterControllerArgumentsEvent $event) + { + $namedArguments = $event->getRequest()->attributes->all(); + $argumentMetadatas = $this->argumentMetadataFactory->createArgumentMetadata($event->getController()); + + // loop over each argument value and its name from the metadata + foreach ($event->getArguments() as $index => $argument) { + if (!isset($argumentMetadatas[$index])) { + throw new \LogicException(sprintf('Could not find any argument metadata for argument %d of the controller.', $index)); + } + + $namedArguments[$argumentMetadatas[$index]->getName()] = $argument; + } + + return $namedArguments; + } + private function getIsGrantedString(IsGranted $isGranted) { $attributes = array_map(function ($attribute) { @@ -95,6 +114,6 @@ private function getIsGrantedString(IsGranted $isGranted) */ public static function getSubscribedEvents() { - return array(KernelEvents::CONTROLLER => 'onKernelController'); + return array(KernelEvents::CONTROLLER_ARGUMENTS => 'onKernelControllerArguments'); } } diff --git a/Resources/config/security.xml b/Resources/config/security.xml index e1c77cfb..4c090048 100644 --- a/Resources/config/security.xml +++ b/Resources/config/security.xml @@ -17,6 +17,7 @@ + diff --git a/Tests/EventListener/IsGrantedListenerTest.php b/Tests/EventListener/IsGrantedListenerTest.php index 2bf66a01..35206461 100644 --- a/Tests/EventListener/IsGrantedListenerTest.php +++ b/Tests/EventListener/IsGrantedListenerTest.php @@ -15,7 +15,9 @@ use Sensio\Bundle\FrameworkExtraBundle\EventListener\IsGrantedListener; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; -use Symfony\Component\HttpKernel\Event\FilterControllerEvent; +use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata; +use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactoryInterface; +use Symfony\Component\HttpKernel\Event\FilterControllerArgumentsEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Security\Core\Exception\AccessDeniedException; @@ -27,9 +29,9 @@ class IsGrantedListenerTest extends \PHPUnit_Framework_TestCase */ public function testExceptionIfSecurityNotInstalled() { - $listener = new IsGrantedListener(); + $listener = new IsGrantedListener($this->createArgumentMetadataFactory(array())); $request = $this->createRequest(new IsGranted(array())); - $listener->onKernelController($this->createFilterControllerEvent($request)); + $listener->onKernelControllerArguments($this->createFilterControllerEvent($request, array())); } public function testNothingHappensWithNoConfig() @@ -38,9 +40,9 @@ public function testNothingHappensWithNoConfig() $authChecker->expects($this->never()) ->method('isGranted'); - $listener = new IsGrantedListener($authChecker); + $listener = new IsGrantedListener($this->createArgumentMetadataFactory(array()), $authChecker); $request = $this->createRequest(); - $listener->onKernelController($this->createFilterControllerEvent($request)); + $listener->onKernelControllerArguments($this->createFilterControllerEvent($request, array())); } public function testIsGrantedCalledCorrectly() @@ -52,11 +54,33 @@ public function testIsGrantedCalledCorrectly() ->with('ROLE_ADMIN', 'bar') ->will($this->returnValue(true)); - $listener = new IsGrantedListener($authChecker); + $listener = new IsGrantedListener($this->createArgumentMetadataFactory(array()), $authChecker); $isGranted = new IsGranted(array('attributes' => 'ROLE_ADMIN', 'subject' => 'foo')); $request = $this->createRequest($isGranted); $request->attributes->set('foo', 'bar'); - $listener->onKernelController($this->createFilterControllerEvent($request)); + $listener->onKernelControllerArguments($this->createFilterControllerEvent($request, array())); + } + + public function testIsGrantedSubjectFromArguments() + { + $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); + // createRequest() puts 2 IsGranted annotations into the config + $authChecker->expects($this->exactly(2)) + ->method('isGranted') + // the subject => arg2name will eventually resolve to the 2nd argument, which has this value + ->with('ROLE_ADMIN', 'arg2Value') + ->will($this->returnValue(true)); + + // create metadata for 2 named args for the controller + $arg1Metadata = new ArgumentMetadata('arg1Name', 'string', false, false, null); + $arg2Metadata = new ArgumentMetadata('arg2Name', 'string', false, false, null); + $listener = new IsGrantedListener($this->createArgumentMetadataFactory(array($arg1Metadata, $arg2Metadata)), $authChecker); + $isGranted = new IsGranted(array('attributes' => 'ROLE_ADMIN', 'subject' => 'arg2Name')); + $request = $this->createRequest($isGranted); + + // the 2 resolved arguments to the controller + $arguments = array('arg1Value', 'arg2Value'); + $listener->onKernelControllerArguments($this->createFilterControllerEvent($request, $arguments)); } /** @@ -66,10 +90,10 @@ public function testExceptionWhenMissingSubjectAttribute() { $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); - $listener = new IsGrantedListener($authChecker); + $listener = new IsGrantedListener($this->createArgumentMetadataFactory(array()), $authChecker); $isGranted = new IsGranted(array('attributes' => 'ROLE_ADMIN', 'subject' => 'non_existent')); $request = $this->createRequest($isGranted); - $listener->onKernelController($this->createFilterControllerEvent($request)); + $listener->onKernelControllerArguments($this->createFilterControllerEvent($request, array())); } /** @@ -82,7 +106,7 @@ public function testAccessDeniedMessages(array $attributes, $subject, $expectedM ->method('isGranted') ->will($this->returnValue(false)); - $listener = new IsGrantedListener($authChecker); + $listener = new IsGrantedListener($this->createArgumentMetadataFactory(array()), $authChecker); $isGranted = new IsGranted(array('attributes' => $attributes, 'subject' => $subject)); $request = $this->createRequest($isGranted); @@ -93,7 +117,7 @@ public function testAccessDeniedMessages(array $attributes, $subject, $expectedM $this->setExpectedException(AccessDeniedException::class, $expectedMessage); - $listener->onKernelController($this->createFilterControllerEvent($request)); + $listener->onKernelControllerArguments($this->createFilterControllerEvent($request, array())); } public function getAccessDeniedMessageTests() @@ -110,15 +134,15 @@ public function getAccessDeniedMessageTests() public function testNotFoundHttpException() { $request = $this->createRequest(new IsGranted(array('attributes' => 'ROLE_ADMIN', 'statusCode' => 404, 'message' => 'Not found'))); - $event = $this->createFilterControllerEvent($request); + $event = $this->createFilterControllerEvent($request, array()); $authChecker = $this->getMockBuilder(AuthorizationCheckerInterface::class)->getMock(); $authChecker->expects($this->any()) ->method('isGranted') ->will($this->returnValue(false)); - $listener = new IsGrantedListener($authChecker); - $listener->onKernelController($event); + $listener = new IsGrantedListener($this->createArgumentMetadataFactory(array()), $authChecker); + $listener->onKernelControllerArguments($event); } private function createRequest(IsGranted $isGranted = null) @@ -131,8 +155,19 @@ private function createRequest(IsGranted $isGranted = null) )); } - private function createFilterControllerEvent(Request $request) + private function createFilterControllerEvent(Request $request, array $arguments) + { + return new FilterControllerArgumentsEvent($this->getMockBuilder(HttpKernelInterface::class)->getMock(), function () { return new Response(); }, $arguments, $request, null); + } + + private function createArgumentMetadataFactory(array $argumentMetadatas) { - return new FilterControllerEvent($this->getMockBuilder(HttpKernelInterface::class)->getMock(), function () { return new Response(); }, $request, null); + $factory = $this->getMockBuilder(ArgumentMetadataFactoryInterface::class)->getMock(); + + $factory->expects($this->any()) + ->method('createArgumentMetadata') + ->will($this->returnValue($argumentMetadatas)); + + return $factory; } } diff --git a/Tests/Fixtures/FooBundle/Controller/IsGrantedController.php b/Tests/Fixtures/FooBundle/Controller/IsGrantedController.php new file mode 100644 index 00000000..afb8893c --- /dev/null +++ b/Tests/Fixtures/FooBundle/Controller/IsGrantedController.php @@ -0,0 +1,47 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Tests\Fixtures\FooBundle\Controller; + +use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; +use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; + +class IsGrantedController +{ + /** + * @Route("/is_granted/anonymous") + * @IsGranted("IS_AUTHENTICATED_ANONYMOUSLY") + */ + public function someAction() + { + return new Response('yay1'); + } + + /** + * @Route("/is_granted/request/attribute/args/{a}") + * @IsGranted("ISGRANTED_VOTER", subject="a") + */ + public function some2Action($a) + { + return new Response('yay2'); + } + + /** + * @Route("/is_granted/resolved/args") + * @IsGranted("ISGRANTED_VOTER", subject="foo") + */ + public function some3Action(Request $foo) + { + return new Response('yay3'); + } +} diff --git a/Tests/Fixtures/FooBundle/Security/IsGrantedVoter.php b/Tests/Fixtures/FooBundle/Security/IsGrantedVoter.php new file mode 100644 index 00000000..b3b0cdff --- /dev/null +++ b/Tests/Fixtures/FooBundle/Security/IsGrantedVoter.php @@ -0,0 +1,41 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Tests\Fixtures\FooBundle\Security; + +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; + +/** + * Used in the function test IsGrantedTest. + */ +class IsGrantedVoter extends Voter +{ + protected function supports($attribute, $subject) + { + return 'ISGRANTED_VOTER' === $attribute; + } + + protected function voteOnAttribute($attribute, $subject, TokenInterface $token) + { + // we use these specific conditions in the test + if ('allow_access' === $subject) { + return true; + } + + if ($subject instanceof Request) { + return true; + } + + return false; + } +} diff --git a/Tests/Fixtures/TestKernel.php b/Tests/Fixtures/TestKernel.php index 6a201d61..1892df47 100644 --- a/Tests/Fixtures/TestKernel.php +++ b/Tests/Fixtures/TestKernel.php @@ -26,6 +26,7 @@ public function registerBundles() new \Sensio\Bundle\FrameworkExtraBundle\SensioFrameworkExtraBundle(), new \Symfony\Bundle\TwigBundle\TwigBundle(), new \Doctrine\Bundle\DoctrineBundle\DoctrineBundle(), + new \Symfony\Bundle\SecurityBundle\SecurityBundle(), new \Tests\Fixtures\FooBundle\FooBundle(), new \Tests\Fixtures\ActionArgumentsBundle\ActionArgumentsBundle(), ); diff --git a/Tests/Fixtures/config/config.yml b/Tests/Fixtures/config/config.yml index 981d6bbd..3b2675bb 100644 --- a/Tests/Fixtures/config/config.yml +++ b/Tests/Fixtures/config/config.yml @@ -27,3 +27,31 @@ services: test.action_arguments: class: Tests\Fixtures\ActionArgumentsBundle\Controller\ActionArgumentsController public: true + + test.is_granted_voter: + class: Tests\Fixtures\FooBundle\Security\IsGrantedVoter + public: false + tags: + - { name: security.voter } + +security: + encoders: + Symfony\Component\Security\Core\User\User: plaintext + + providers: + in_memory: + memory: + users: + johannes: { password: test, roles: [ROLE_USER] } + + firewalls: + # This firewall doesn't make sense in combination with the rest of the + # configuration file, but it's here for testing purposes (do not use + # this file in a real world scenario though) + login_form: + pattern: ^/login$ + security: false + + default: + form_login: ~ + anonymous: ~ diff --git a/Tests/Functional/IsGrantedTest.php b/Tests/Functional/IsGrantedTest.php new file mode 100644 index 00000000..079eb014 --- /dev/null +++ b/Tests/Functional/IsGrantedTest.php @@ -0,0 +1,40 @@ +request('GET', '/is_granted/anonymous'); + + $this->assertSame(200, $client->getResponse()->getStatusCode()); + } + + public function testIsGrantedSubjectFromAttributes() + { + $client = self::createClient(); + // allow_access is a special string allowed by the voter + $client->request('GET', '/is_granted/request/attribute/args/allow_access'); + + $this->assertSame(200, $client->getResponse()->getStatusCode()); + } + + public function testIsGrantedSubjectFromAttributesDenied() + { + $client = self::createClient(); + $client->request('GET', '/is_granted/request/attribute/args/no_access'); + + // a redirect + $this->assertSame(302, $client->getResponse()->getStatusCode()); + } + + public function testIsGrantedSubjectFromArguments() + { + $client = self::createClient(); + $client->request('GET', '/is_granted/resolved/args'); + + $this->assertSame(200, $client->getResponse()->getStatusCode()); + } +}