-
Notifications
You must be signed in to change notification settings - Fork 33
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
Changes from 2 commits
991a8d0
6c7aada
65ccec2
e4e1275
bdf22d7
e750115
c7a67ae
5afa058
87fd7f3
45f1fba
4587c0c
93fb382
003d8cd
89bf726
e0d41d8
b516cd7
fc927c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,17 @@ | ||
Changelog | ||
========= | ||
|
||
* **2013-06-20**: [PublishWorkflow] The PublishWorkflowChecker now implements | ||
SecurityContextInterface and the individual checks are moved to voters. | ||
Use the service cmf_core.publish_workflow.checker and call | ||
`isGranted('VIEW', $content)` - or `'VIEW_PUBLISHED'` if you don't want to | ||
see unpublished content even if the current user is allowed to see it. | ||
Configuration was adjusted: The parameter for the role that may see unpublished | ||
content moved from `role` to `publish_workflow.view_non_published_role`. | ||
The security context is also triggered by a core security voter, so that | ||
using the isGranted method of the standard security will check for | ||
publication. | ||
|
||
* **2013-05-16**: [PublishWorkFlowChecker] Removed Request argument | ||
from check method. Class now accepts a DateTime object to | ||
optionally "set" the current time. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
<?php | ||
|
||
namespace Symfony\Cmf\Bundle\CoreBundle\DependencyInjection\Compiler; | ||
|
||
use Symfony\Component\DependencyInjection\Reference; | ||
use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; | ||
|
||
/** | ||
* Adds all configured publish workflow voters to the access decision manager. | ||
* | ||
* This is about the same as Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass | ||
* | ||
* @author David Buchmann <[email protected]> | ||
* @author Johannes M. Schmitt <[email protected]> | ||
*/ | ||
class AddPublishedVotersPass implements CompilerPassInterface | ||
{ | ||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function process(ContainerBuilder $container) | ||
{ | ||
if (!$container->hasDefinition('cmf_core.publish_workflow.access_decision_manager')) { | ||
return; | ||
} | ||
|
||
$voters = new \SplPriorityQueue(); | ||
foreach ($container->findTaggedServiceIds('cmf_published_voter') as $id => $attributes) { | ||
$priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0; | ||
$voters->insert(new Reference($id), $priority); | ||
} | ||
|
||
$voters = iterator_to_array($voters); | ||
ksort($voters); | ||
|
||
$container->getDefinition('cmf_core.publish_workflow.access_decision_manager')->replaceArgument(0, array_values($voters)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,23 +7,26 @@ | |
use Symfony\Component\HttpKernel\Event\GetResponseEvent; | ||
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; | ||
|
||
use Symfony\Cmf\Bundle\CoreBundle\PublishWorkflow\PublishWorkflowChecker; | ||
|
||
use Symfony\Cmf\Bundle\RoutingBundle\Routing\DynamicRouter; | ||
use Symfony\Cmf\Bundle\CoreBundle\PublishWorkflow\PublishWorkflowCheckerInterface; | ||
|
||
/** | ||
* Makes sure only published routes and content can be accessed | ||
* Makes sure only published routes and content can be accessed. | ||
* | ||
* @author David Buchmann <[email protected]> | ||
*/ | ||
class PublishWorkflowListener implements EventSubscriberInterface | ||
{ | ||
/** | ||
* @var PublishWorkflowCheckerInterface | ||
* @var PublishWorkflowChecker | ||
*/ | ||
protected $publishWorkflowChecker; | ||
|
||
/** | ||
* @param PublishWorkflowCheckerInterface $publishWorkflowChecker | ||
* @param PublishWorkflowChecker $publishWorkflowChecker | ||
*/ | ||
public function __construct(PublishWorkflowCheckerInterface $publishWorkflowChecker) | ||
public function __construct(PublishWorkflowChecker $publishWorkflowChecker) | ||
{ | ||
$this->publishWorkflowChecker = $publishWorkflowChecker; | ||
} | ||
|
@@ -38,12 +41,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->publishWorkflowChecker->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->publishWorkflowChecker->isGranted('VIEW', $content)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we always check for VIEW and not VIEW_PUBLISH. i think that makes sense, the VIEW_PUBLISH is mainly interesting in a menu or other lists. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it none the less make sense to make this a constructor parameter, even if we for now do not support configuring it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah why not, there could be use cases (like a prod domain versus editor domain). will do that. btw, i realize i wanted to call it 'VIEW_ANONYMOUS', that is maybe better. or even just 'PUBLISHED'? |
||
throw new NotFoundHttpException('Content not found for: ' . $request->getPathInfo()); | ||
} | ||
} | ||
|
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(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,85 +2,136 @@ | |
|
||
namespace Symfony\Cmf\Bundle\CoreBundle\PublishWorkflow; | ||
|
||
use Symfony\Component\DependencyInjection\ContainerInterface; | ||
use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken; | ||
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; | ||
use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface; | ||
use Symfony\Component\Security\Core\SecurityContextInterface; | ||
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; | ||
use Symfony\Component\HttpFoundation\Request; | ||
|
||
/** | ||
* Implementation of a publish workflow checker. It gives "admins" full access, | ||
* while for other users it checks that both the publish flag is on and the | ||
* publish date isn't reached if one is set. | ||
*/ | ||
class PublishWorkflowChecker implements PublishWorkflowCheckerInterface | ||
* Implementation of a publish workflow checker as a security context. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as we now implement SecurityContextInterface i saw no need to keep a separate PublishWorkflowCheckerInterface |
||
* | ||
* It gives "admins" full access, | ||
* while for other users it runs all cmf_published_voter | ||
* | ||
* @author David Buchmann <[email protected]> | ||
*/ | ||
class PublishWorkflowChecker implements SecurityContextInterface | ||
{ | ||
/** | ||
* @var string the role name for the security check | ||
* This attribute means the user is allowed to see this content, either | ||
* because it is published or because he is granted the bypassingRole. | ||
*/ | ||
protected $requiredRole; | ||
const VIEW_ATTRIBUTE = 'VIEW'; | ||
|
||
/** | ||
* @var SecurityContextInterface | ||
* This attribute means the content is available for viewing by anonymous | ||
* users. This can be used where the role based exception from the | ||
* publication check is not wanted. | ||
* | ||
* The role exception is handled by the workflow checker, the individual | ||
* voters should treat VIEW and VIEW_PUBLISHED the same. | ||
*/ | ||
protected $securityContext; | ||
const VIEW_PUBLISHED_ATTRIBUTE = 'VIEW_PUBLISHED'; | ||
|
||
/** | ||
* @var \DateTime | ||
* @var ContainerInterface | ||
*/ | ||
protected $currentTime; | ||
private $container; | ||
|
||
/** | ||
* @param string $requiredRole the role to check with the securityContext | ||
* (if you pass one), defaults to everybody: IS_AUTHENTICATED_ANONYMOUSLY | ||
* @param \Symfony\Component\Security\Core\SecurityContextInterface|null $securityContext | ||
* the security context to use to check for the role. No security | ||
* check if this is null | ||
* @var bool|string Role allowed to bypass security check or false to never | ||
* bypass | ||
*/ | ||
public function __construct($requiredRole = "IS_AUTHENTICATED_ANONYMOUSLY", SecurityContextInterface $securityContext = null) | ||
{ | ||
$this->requiredRole = $requiredRole; | ||
$this->securityContext = $securityContext; | ||
$this->currentTime = new \DateTime(); | ||
} | ||
private $bypassingRole; | ||
|
||
/** | ||
* Overwrite the current time | ||
* | ||
* @param \DateTime $currentTime | ||
* @var AccessDecisionManagerInterface | ||
*/ | ||
private $accessDecisionManager; | ||
|
||
/** | ||
* @var TokenInterface | ||
*/ | ||
private $token; | ||
|
||
/** | ||
* @param ContainerInterface $container to get the security context from. | ||
* We cannot inject the security context directly as this would lead | ||
* to a circular reference. | ||
* @param AccessDecisionManagerInterface $accessDecisionManager | ||
* @param string $bypassingRole A role that is allowed to bypass the | ||
* publishable check. | ||
*/ | ||
public function setCurrentTime(\DateTime $currentTime) | ||
public function __construct(ContainerInterface $container, AccessDecisionManagerInterface $accessDecisionManager, $bypassingRole = false) | ||
{ | ||
$this->currentTime = $currentTime; | ||
$this->container = $container; | ||
$this->accessDecisionManager = $accessDecisionManager; | ||
$this->bypassingRole = $bypassingRole; | ||
} | ||
|
||
/** | ||
* {inheritDoc} | ||
* {@inheritDoc} | ||
*/ | ||
public function checkIsPublished($document, $ignoreRole = false) | ||
public function getToken() | ||
{ | ||
if (!$document instanceOf PublishWorkflowInterface) { | ||
return true; | ||
} | ||
if (null === $this->token) { | ||
$securityContext = $this->container->get('security.context'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of interest - why lazy load the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dependencies. see the comment on the constructor of this class: we have a publish workflow security voter, so we get into a dependency cycle if we don't do this. |
||
|
||
if ($this->securityContext && $this->securityContext->isGranted($this->requiredRole)) { | ||
if (!$ignoreRole) { | ||
return true; | ||
} | ||
return $securityContext->getToken(); | ||
} | ||
|
||
$startDate = $document->getPublishStartDate(); | ||
$endDate = $document->getPublishEndDate(); | ||
$isPublishable = $document->isPublishable(); | ||
return $this->token; | ||
} | ||
|
||
if (null === $startDate && null === $endDate) { | ||
return $isPublishable !== false; | ||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function setToken(TokenInterface $token = null) | ||
{ | ||
$this->token = $token; | ||
} | ||
|
||
/** | ||
* Checks if the access decision manager supports the given class. | ||
* | ||
* @param string $class A class name | ||
* | ||
* @return boolean true if this decision manager can process the class | ||
*/ | ||
public function supportsClass($class) | ||
{ | ||
return $this->accessDecisionManager->supportsClass($class); | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function isGranted($attributes, $object = null) | ||
{ | ||
if (!is_array($attributes)) { | ||
$attributes = array($attributes); | ||
} | ||
|
||
if ((null === $startDate || $this->currentTime >= $startDate) && | ||
(null === $endDate || $this->currentTime < $endDate) | ||
$securityContext = $this->container->get('security.context'); | ||
if (null !== $securityContext->getToken() | ||
&& (count($attributes) === 1) | ||
&& self::VIEW_ATTRIBUTE === reset($attributes) | ||
&& $securityContext->isGranted($this->bypassingRole) | ||
) { | ||
return $isPublishable !== false; | ||
return true; | ||
} | ||
|
||
$token = $this->getToken(); | ||
if (null === $token) { | ||
// not logged in, surely we can not skip the check. | ||
// create a dummy token to check for publication even if no | ||
// firewall is present. | ||
$token = new AnonymousToken('', ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the voter interface says the token may not be null. published-voters must handle this special token if they care about the token at all (the standard voters do not care) |
||
} | ||
|
||
return false; | ||
return $this->accessDecisionManager->decide($token, $attributes, $object); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for me.
There was a problem hiding this comment.
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