Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Workflow] add guard is_valid() method support #23499

Merged
merged 2 commits into from Oct 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -17,6 +17,7 @@

/**
* @author Christian Flothmann <christian.flothmann@sensiolabs.de>
* @author Grégoire Pineau <lyrixx@lyrixx.info>
*/
class WorkflowGuardListenerPass implements CompilerPassInterface
{
Expand All @@ -31,20 +32,17 @@ public function process(ContainerBuilder $container)

$container->getParameterBag()->remove('workflow.has_guard_listeners');

if (!$container->has('security.token_storage')) {
throw new LogicException('The "security.token_storage" service is needed to be able to use the workflow guard listener.');
}

if (!$container->has('security.authorization_checker')) {
throw new LogicException('The "security.authorization_checker" service is needed to be able to use the workflow guard listener.');
}

if (!$container->has('security.authentication.trust_resolver')) {
throw new LogicException('The "security.authentication.trust_resolver" service is needed to be able to use the workflow guard listener.');
}

if (!$container->has('security.role_hierarchy')) {
throw new LogicException('The "security.role_hierarchy" service is needed to be able to use the workflow guard listener.');
$servicesNeeded = array(
'security.token_storage',
'security.authorization_checker',
'security.authentication.trust_resolver',
'security.role_hierarchy',
);

foreach ($servicesNeeded as $service) {
if (!$container->has($service)) {
throw new LogicException(sprintf('The "%s" service is needed to be able to use the workflow guard listener.', $service));
}
}
}
}
Expand Up @@ -85,6 +85,7 @@
use Symfony\Component\Translation\Translator;
use Symfony\Component\Validator\ConstraintValidatorInterface;
use Symfony\Component\Validator\ObjectInitializerInterface;
use Symfony\Component\Validator\Validator\ValidatorInterface;
use Symfony\Component\WebLink\HttpHeaderSerializer;
use Symfony\Component\Workflow;
use Symfony\Component\Yaml\Command\LintCommand as BaseYamlLintCommand;
Expand Down Expand Up @@ -752,6 +753,7 @@ private function registerWorkflowConfiguration(array $config, ContainerBuilder $
new Reference('security.authorization_checker'),
new Reference('security.authentication.trust_resolver'),
new Reference('security.role_hierarchy'),
new Reference('validator', ContainerInterface::NULL_ON_INVALID_REFERENCE),
));

$container->setDefinition(sprintf('%s.listener.guard', $workflowId), $guard);
Expand Down
Expand Up @@ -14,12 +14,11 @@
use PHPUnit\Framework\TestCase;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\WorkflowGuardListenerPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
use Symfony\Component\Security\Core\Role\RoleHierarchy;
use Symfony\Component\Workflow\EventListener\GuardListener;
use Symfony\Component\Validator\Validator\ValidatorInterface;

class WorkflowGuardListenerPassTest extends TestCase
{
Expand All @@ -29,53 +28,37 @@ class WorkflowGuardListenerPassTest extends TestCase
protected function setUp()
{
$this->container = new ContainerBuilder();
$this->container->register('foo.listener.guard', GuardListener::class);
$this->container->register('bar.listener.guard', GuardListener::class);
$this->compilerPass = new WorkflowGuardListenerPass();
}

public function testListenersAreNotRemovedIfParameterIsNotSet()
public function testNoExeptionIfParameterIsNotSet()
{
$this->compilerPass->process($this->container);

$this->assertTrue($this->container->hasDefinition('foo.listener.guard'));
$this->assertTrue($this->container->hasDefinition('bar.listener.guard'));
}

public function testParameterIsRemovedWhenThePassIsProcessed()
{
$this->container->setParameter('workflow.has_guard_listeners', array('foo.listener.guard', 'bar.listener.guard'));

try {
$this->compilerPass->process($this->container);
} catch (LogicException $e) {
// Here, we are not interested in the exception handling. This is tested further down.
}

$this->assertFalse($this->container->hasParameter('workflow.has_guard_listeners'));
}

public function testListenersAreNotRemovedIfAllDependenciesArePresent()
public function testNoExeptionIfAllDependenciesArePresent()
{
$this->container->setParameter('workflow.has_guard_listeners', array('foo.listener.guard', 'bar.listener.guard'));
$this->container->setParameter('workflow.has_guard_listeners', true);
$this->container->register('security.token_storage', TokenStorageInterface::class);
$this->container->register('security.authorization_checker', AuthorizationCheckerInterface::class);
$this->container->register('security.authentication.trust_resolver', AuthenticationTrustResolverInterface::class);
$this->container->register('security.role_hierarchy', RoleHierarchy::class);
$this->container->register('validator', ValidatorInterface::class);

$this->compilerPass->process($this->container);

$this->assertTrue($this->container->hasDefinition('foo.listener.guard'));
$this->assertTrue($this->container->hasDefinition('bar.listener.guard'));
$this->assertFalse($this->container->hasParameter('workflow.has_guard_listeners'));
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\LogicException
* @expectedExceptionMessage The "security.token_storage" service is needed to be able to use the workflow guard listener.
*/
public function testListenersAreRemovedIfTheTokenStorageServiceIsNotPresent()
public function testExceptionIfTheTokenStorageServiceIsNotPresent()
{
$this->container->setParameter('workflow.has_guard_listeners', array('foo.listener.guard', 'bar.listener.guard'));
$this->container->setParameter('workflow.has_guard_listeners', true);
$this->container->register('security.authorization_checker', AuthorizationCheckerInterface::class);
$this->container->register('security.authentication.trust_resolver', AuthenticationTrustResolverInterface::class);
$this->container->register('security.role_hierarchy', RoleHierarchy::class);
Expand All @@ -87,9 +70,9 @@ public function testListenersAreRemovedIfTheTokenStorageServiceIsNotPresent()
* @expectedException \Symfony\Component\DependencyInjection\Exception\LogicException
* @expectedExceptionMessage The "security.authorization_checker" service is needed to be able to use the workflow guard listener.
*/
public function testListenersAreRemovedIfTheAuthorizationCheckerServiceIsNotPresent()
public function testExceptionIfTheAuthorizationCheckerServiceIsNotPresent()
{
$this->container->setParameter('workflow.has_guard_listeners', array('foo.listener.guard', 'bar.listener.guard'));
$this->container->setParameter('workflow.has_guard_listeners', true);
$this->container->register('security.token_storage', TokenStorageInterface::class);
$this->container->register('security.authentication.trust_resolver', AuthenticationTrustResolverInterface::class);
$this->container->register('security.role_hierarchy', RoleHierarchy::class);
Expand All @@ -101,9 +84,9 @@ public function testListenersAreRemovedIfTheAuthorizationCheckerServiceIsNotPres
* @expectedException \Symfony\Component\DependencyInjection\Exception\LogicException
* @expectedExceptionMessage The "security.authentication.trust_resolver" service is needed to be able to use the workflow guard listener.
*/
public function testListenersAreRemovedIfTheAuthenticationTrustResolverServiceIsNotPresent()
public function testExceptionIfTheAuthenticationTrustResolverServiceIsNotPresent()
{
$this->container->setParameter('workflow.has_guard_listeners', array('foo.listener.guard', 'bar.listener.guard'));
$this->container->setParameter('workflow.has_guard_listeners', true);
$this->container->register('security.token_storage', TokenStorageInterface::class);
$this->container->register('security.authorization_checker', AuthorizationCheckerInterface::class);
$this->container->register('security.role_hierarchy', RoleHierarchy::class);
Expand All @@ -115,9 +98,9 @@ public function testListenersAreRemovedIfTheAuthenticationTrustResolverServiceIs
* @expectedException \Symfony\Component\DependencyInjection\Exception\LogicException
* @expectedExceptionMessage The "security.role_hierarchy" service is needed to be able to use the workflow guard listener.
*/
public function testListenersAreRemovedIfTheRoleHierarchyServiceIsNotPresent()
public function testExceptionIfTheRoleHierarchyServiceIsNotPresent()
{
$this->container->setParameter('workflow.has_guard_listeners', array('foo.listener.guard', 'bar.listener.guard'));
$this->container->setParameter('workflow.has_guard_listeners', true);
$this->container->register('security.token_storage', TokenStorageInterface::class);
$this->container->register('security.authorization_checker', AuthorizationCheckerInterface::class);
$this->container->register('security.authentication.trust_resolver', AuthenticationTrustResolverInterface::class);
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Workflow/CHANGELOG.md
Expand Up @@ -4,6 +4,7 @@ CHANGELOG
3.4.0
-----

* Added guard `is_valid()` method support.
* Added support for `Event::getWorkflowName()` for "announce" events.
* Added `workflow.completed` events which are fired after a transition is completed.

Expand Down
Expand Up @@ -12,6 +12,8 @@
namespace Symfony\Component\Workflow\EventListener;

use Symfony\Component\Security\Core\Authorization\ExpressionLanguage as BaseExpressionLanguage;
use Symfony\Component\Validator\Validator\ValidatorInterface;
use Symfony\Component\Workflow\Exception\RuntimeException;

/**
* Adds some function to the default Symfony Security ExpressionLanguage.
Expand All @@ -29,5 +31,17 @@ protected function registerFunctions()
}, function (array $variables, $attributes, $object = null) {
return $variables['auth_checker']->isGranted($attributes, $object);
});

$this->register('is_valid', function ($object = 'null', $groups = 'null') {
return sprintf('0 === count($validator->validate(%s, null, %s))', $object, $groups);
}, function (array $variables, $object = null, $groups = null) {
if (!$variables['validator'] instanceof ValidatorInterface) {
throw new RuntimeException('"is_valid" cannot be used as the Validator component is not installed.');
}

$errors = $variables['validator']->validate($object, null, $groups);

return 0 === count($errors);
});
}
}
Expand Up @@ -15,6 +15,7 @@
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
use Symfony\Component\Security\Core\Role\RoleHierarchyInterface;
use Symfony\Component\Validator\Validator\ValidatorInterface;
use Symfony\Component\Workflow\Event\GuardEvent;

/**
Expand All @@ -28,15 +29,17 @@ class GuardListener
private $authenticationChecker;
private $trustResolver;
private $roleHierarchy;
private $validator;

public function __construct($configuration, ExpressionLanguage $expressionLanguage, TokenStorageInterface $tokenStorage, AuthorizationCheckerInterface $authenticationChecker, AuthenticationTrustResolverInterface $trustResolver, RoleHierarchyInterface $roleHierarchy = null)
public function __construct($configuration, ExpressionLanguage $expressionLanguage, TokenStorageInterface $tokenStorage, AuthorizationCheckerInterface $authenticationChecker, AuthenticationTrustResolverInterface $trustResolver, RoleHierarchyInterface $roleHierarchy = null, ValidatorInterface $validator = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we shouldn't register a new listener with the validator as this part is not a security related listener

Copy link
Contributor Author

@halundraN halundraN Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong, but I thought that the "guard" option was designed to protect the execution of the transition and not only from a security point of view.

In this case, this does seem to me problematic that the expresion is_valid is in the guard listener ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fear is that the GuardListener could potentially become very big in the future if add support for other things here. Maybe splitting it into a SecurityGuardListener (as a replacement for the existing one) and a ValidatorGuardListener could be an idea.

Copy link
Contributor Author

@halundraN halundraN Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how many types of conditions can bloc a transistion but yes we can imagine a lot of thing.

Then I could split GuardListener into SecurityGuardListener (the same as GuardListener before my PR) and a ValidatorGuardListener with the new behavior.

In FrameworkExtension, I need to create a new definition for ValidatorGuardListener.
For the definition id are you ok with :

sprintf('%s.listener.guard.security', $workflowId)
and
sprintf('%s.listener.guard.validator', $workflowId)

And I plug it on the same event ?

sprintf('workflow.%s.guard.%s', $name, $transitionName)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks okay to me, but let's wait for @lyrixx and the other @symfony/deciders to share their opinion before spending time on this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a new listener event is a bad idea, you should only use the guard event.

Then, the GuardListener name is not very well chosen indeed. I should have name it ExpressionGuardListener

Finally, splitting it in 2 parts will be hard because you will need to duplicate everything (config in the .yml, .xml etc etc). More over the end user will not be able to mix security stuff and validation stuff in the same expression.

So I'm 👍 with the current implement (except you need to fix issue spotted by @xabbuh)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I made a code review commit but I don't understand why github don't close "is_valid() should be enclosed by backticks" discussion because it's done (you can see it in the files changed).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry I see it in the diff.
I guess it's because you have an anchor in your URL, so github re-expand it

{
$this->configuration = $configuration;
$this->expressionLanguage = $expressionLanguage;
$this->tokenStorage = $tokenStorage;
$this->authenticationChecker = $authenticationChecker;
$this->trustResolver = $trustResolver;
$this->roleHierarchy = $roleHierarchy;
$this->validator = $validator;
}

public function onTransition(GuardEvent $event, $eventName)
Expand Down Expand Up @@ -72,6 +75,8 @@ private function getVariables(GuardEvent $event)
'auth_checker' => $this->authenticationChecker,
// needed for the is_* expression function
'trust_resolver' => $this->trustResolver,
// needed for the is_valid expression function
'validator' => $this->validator,
);

return $variables;
Expand Down
21 changes: 21 additions & 0 deletions src/Symfony/Component/Workflow/Exception/RuntimeException.php
@@ -0,0 +1,21 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Workflow\Exception;

/**
* Base RuntimeException for the Workflow component.
*
* @author Alain Flaus <alain.flaus@gmail.com>
*/
class RuntimeException extends \RuntimeException implements ExceptionInterface
{
}