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

refactor publish workflow to use security, cleanup configuration #59

Merged
merged 17 commits into from
Jul 14, 2013
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
Changelog
=========

* **2013-06-20**: [PublishWorkflow] Moved the access checks to security voter
and using isGranted 'VIEW' instead.
Removed twig function cmf_is_published, just use is_granted('VIEW', content)
instead.
Configuration was adjusted: The parameter for the role that may see unpublished
content moved from `role` to `publish_workflow.view_non_published_role`. The
publish_workflow_listener moved to `publish_workflow.request_listener`.

* **2013-05-16**: [PublishWorkFlowChecker] Removed Request argument
from check method. Class now accepts a DateTime object to
optionally "set" the current time.
20 changes: 15 additions & 5 deletions DependencyInjection/CmfCoreExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,23 @@ public function load(array $configs, ContainerBuilder $container)
$loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config'));
$loader->load('services.xml');

$container->setParameter($this->getAlias().'.role', $config['role']);
$container->setParameter($this->getAlias() . '.document_manager_name', $config['document_manager_name']);

if (!$config['publish_workflow_listener']) {
$container->removeDefinition($this->getAlias() . '.publish_workflow_listener');
} elseif (!class_exists('Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter')) {
throw new InvalidConfigurationException("The 'publish_workflow_listener' may not be enabled unless 'Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter' is available.");
if (isset($config['publish_workflow'])) {
$this->loadPublishWorkflow($config['publish_workflow'], $loader, $container);
}
}

public function loadPublishWorkflow($config, XmlFileLoader $loader, ContainerBuilder $container) {
Copy link
Member

Choose a reason for hiding this comment

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

curly brackets needs to be on the next line

$container->setParameter($this->getAlias().'.publish_workflow.view_non_published_role', $config['view_non_published_role']);
$loader->load('publish_workflow.xml');

if ($config['request_listener']) {
Copy link
Member

Choose a reason for hiding this comment

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

why the nested if?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is possible to load the publish workflow but disable the request listener. or do i misunderstand your question?

Copy link
Member

Choose a reason for hiding this comment

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

you could handle this entire thing with a single if() and no else.

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't see it. if we have request_listener but no dynamic router, we need to explode. if we do not have request_listener at all, we need to remove the service definition.

if you think its better, i can change this to

    if (!$config['request_listener']) {
        $container->removeDefinition($this->getAlias() . '.publish_workflow.request_listener');
    } elseif (!class_exists('Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter')) {
        throw new InvalidConfigurationException('The "publish_workflow.request_listener" may not be enabled unless "Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter" is available.');
    }

if (!class_exists('Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter')) {
throw new InvalidConfigurationException('The "publish_workflow.request_listener" may not be enabled unless "Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter" is available.');
}
} else {
$container->removeDefinition($this->getAlias() . '.publish_workflow.request_listener');
}
}

Expand Down
8 changes: 6 additions & 2 deletions DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ public function getConfigTreeBuilder()
$rootNode
->children()
->scalarNode('document_manager_name')->defaultValue('default')->end()
->scalarNode('role')->defaultValue('IS_AUTHENTICATED_ANONYMOUSLY')->end()
->booleanNode('publish_workflow_listener')->defaultFalse()->end()
->arrayNode('publish_workflow')
Copy link
Member Author

Choose a reason for hiding this comment

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

i wonder if we should not enable the pwf things by default. the model and admin classes do expose the information, so as a clueless user, i would expect it to just work. this is about security, so i would prefer security over performance for the default value.

Copy link
Member

Choose a reason for hiding this comment

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

ok for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

added addDefaultsIfNotSet and an enabled parameter

->children()
->scalarNode('view_non_published_role')->defaultValue('CAN_VIEW_NON_PUBLISHED')->end()
->booleanNode('request_listener')->defaultTrue()->end()
->end()
->end()
->end()
;

Expand Down
16 changes: 8 additions & 8 deletions EventListener/PublishWorkflowListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,24 @@
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

use Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter;
use Symfony\Cmf\Bundle\CoreBundle\PublishWorkflow\PublishWorkflowCheckerInterface;
use Symfony\Component\Security\Core\SecurityContext;

/**
* Makes sure only published routes and content can be accessed
*/
class PublishWorkflowListener implements EventSubscriberInterface
{
/**
* @var PublishWorkflowCheckerInterface
* @var SecurityContext
*/
protected $publishWorkflowChecker;
protected $context;

/**
* @param PublishWorkflowCheckerInterface $publishWorkflowChecker
* @param SecurityContext $context
*/
public function __construct(PublishWorkflowCheckerInterface $publishWorkflowChecker)
public function __construct(SecurityContext $context)
{
$this->publishWorkflowChecker = $publishWorkflowChecker;
$this->context = $context;
}

/**
Expand All @@ -38,12 +38,12 @@ public function onKernelRequest(GetResponseEvent $event)
$request = $event->getRequest();

$route = $request->attributes->get(DynamicRouter::ROUTE_KEY);
if ($route && !$this->publishWorkflowChecker->checkIsPublished($route, false, $request)) {
if ($route && !$this->context->isGranted('VIEW', $route)) {
throw new NotFoundHttpException('Route not found at: ' . $request->getPathInfo());
}

$content = $request->attributes->get(DynamicRouter::CONTENT_KEY);
if ($content && !$this->publishWorkflowChecker->checkIsPublished($content, false, $request)) {
if ($content && !$this->context->isGranted('VIEW', $content)) {
throw new NotFoundHttpException('Content not found for: ' . $request->getPathInfo());
}
}
Expand Down
30 changes: 30 additions & 0 deletions PublishWorkflow/PublishTimePeriodInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

namespace Symfony\Cmf\Bundle\CoreBundle\PublishWorkflow;

/**
* Interface models can implement if they want to support time based publish
* checking.
*/
interface PublishTimePeriodInterface
{
/**
* Return the date from which the content should be published.
*
* A NULL value is interpreted as a date in the past, meaning the content
* is publishable unless publish end date is set and in the past.
*
* @return \DateTime|null
*/
public function getPublishStartDate();

/**
* Return the date at which the content should stop being published.
*
* A NULL value is interpreted as saying that the document will
* never end being publishable.
*
* @return \DateTime|null
*/
public function getPublishEndDate();
}
86 changes: 0 additions & 86 deletions PublishWorkflow/PublishWorkflowChecker.php

This file was deleted.

18 changes: 0 additions & 18 deletions PublishWorkflow/PublishWorkflowCheckerInterface.php

This file was deleted.

38 changes: 5 additions & 33 deletions PublishWorkflow/PublishWorkflowInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,43 +3,21 @@
namespace Symfony\Cmf\Bundle\CoreBundle\PublishWorkflow;

/**
* Interface models can implement if they want to support publish workflow checking
* Interface models can implement to expose editable publishing settings.
*/
interface PublishWorkflowInterface
interface PublishWorkflowInterface extends PublishableInterface, PublishTimePeriodInterface
{
/**
* Return the date from which the content should
* be considered publishable.
*
* A NULL value should be interpreted as saying that
* the content has always been publishable.
*
* @return \DateTime|null
*/
public function getPublishStartDate();

/**
* Set the date from which the content should
* be considered publishable.
*
* Setting a NULL value asserts that the content
* has always been publishable.
* has always been publishable.
*
* @param \DateTime|null $publishDate
*/
public function setPublishStartDate(\DateTime $publishDate = null);

/**
* Return the date at which the content should
* stop being published.
*
* A NULL value should be interpreted as saying that
* the document will always be publishable.
*
* @return \DateTime|null
*/
public function getPublishEndDate();

/**
* Set the date at which the content should
* stop being published.
Expand All @@ -52,15 +30,9 @@ public function getPublishEndDate();
public function setPublishEndDate(\DateTime $publishDate = null);

/**
* Return the base publishable state of the content.
*
* A false value indicates that the content should, under
* no circumstances, be published. A true value indicates
* that the content is publishable - but is subject to
* the $publishStartDate and $publishEndDate if they are
* set.
* Set the boolean flag if this content should be published or not.
*
* @return boolean
*/
public function isPublishable();
public function setPublishable($publishable);
}
24 changes: 24 additions & 0 deletions PublishWorkflow/PublishableInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

namespace Symfony\Cmf\Bundle\CoreBundle\PublishWorkflow;

/**
* Interface models can implement if they want to support publish checking with
* a binary flag.
*
* Several publish interfaces can be combined. Publish voters will return DENY
* if the condition is not met and ABSTAIN if it is met, to allow other voters
* to influence the decision as well.
*/
interface PublishableInterface
{
/**
* Whether this content is publishable at all.
*
* A false value indicates that the content is not published. True means it
* is allowed to show this content.
*
* @return boolean
*/
public function isPublishable();
}
40 changes: 40 additions & 0 deletions Resources/config/publish_workflow.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?xml version="1.0" ?>

<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

<parameters>
<parameter key="cmf_core.twig_extension_class">Symfony\Cmf\Bundle\CoreBundle\Twig\TwigExtension</parameter>
<parameter key="cmf_core.publish_workflow_listener_class">Symfony\Cmf\Bundle\CoreBundle\EventListener\PublishWorkflowListener</parameter>
<parameter key="cmf_core.security.publishable_voter_class">Symfony\Cmf\Bundle\CoreBundle\Security\Authorization\Voter\PublishableVoter</parameter>
<parameter key="cmf_core.security.publish_time_period_voter_class">Symfony\Cmf\Bundle\CoreBundle\Security\Authorization\Voter\PublishTimePeriodVoter</parameter>
<parameter key="cmf_core.listener.request_aware_class">Symfony\Cmf\Bundle\CoreBundle\EventListener\RequestAwareListener</parameter>
<parameter key="cmf_core.admin_extension.publish_workflow_class">Symfony\Cmf\Bundle\CoreBundle\Admin\Extension\PublishWorkflowExtension</parameter>
</parameters>

<services>

<service id="cmf_core.security.publishable_voter" class="%cmf_core.security.publishable_voter_class%">
<argument type="service" id="service_container" on-invalid="ignore"/>
<argument>%cmf_core.publish_workflow.view_non_published_role%</argument>
<tag name="security.voter"/>
</service>

<service id="cmf_core.security.publish_time_period_voter" class="%cmf_core.security.publish_time_period_voter_class%">
<argument type="service" id="service_container" on-invalid="ignore"/>
<argument>%cmf_core.publish_workflow.view_non_published_role%</argument>
<tag name="security.voter"/>
</service>

<service id="cmf_core.publish_workflow.request_listener" class="%cmf_core.publish_workflow_listener_class%">
<tag name="kernel.event_subscriber"/>
<argument type="service" id="security.context"/>
</service>

<service id="cmf_core.admin_extension.publish_workflow" class="%cmf_core.admin_extension.publish_workflow_class%">
<tag name="sonata.admin.extension"/>
</service>
Copy link
Member Author

Choose a reason for hiding this comment

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

do we need to remove this definition when there is no sonata available? see #60

Copy link
Member Author

Choose a reason for hiding this comment

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

will just remove it for good measure


</services>
</container>
Loading