Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 6, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9499bc2
[Workflow] Added guard 'is_valid()' method support
halundraN Jul 13, 2017
06d8198
[Workflow] Added tests for the is_valid() guard expression
lyrixx Aug 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions 26 ...Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/WorkflowGuardListenerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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));
}
}
}
}
2 changes: 2 additions & 0 deletions 2 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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
45 changes: 14 additions & 31 deletions 45 ...ndle/FrameworkBundle/Tests/DependencyInjection/Compiler/WorkflowGuardListenerPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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 1 src/Symfony/Component/Workflow/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
14 changes: 14 additions & 0 deletions 14 src/Symfony/Component/Workflow/EventListener/ExpressionLanguage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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);
});
}
}
7 changes: 6 additions & 1 deletion 7 src/Symfony/Component/Workflow/EventListener/GuardListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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 •
edited
Loading

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 •
edited
Loading

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 21 src/Symfony/Component/Workflow/Exception/RuntimeException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
{
}
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.