security.context should not be required for publish workflow
rmsint opened this issue · 6 comments
The idea is that the publish workflow works with or without security enabled.
However for the cmf.symfony.com website we discovered that an error is thrown if the service security.context is not available. It happens when Symfony\Bundle\SecurityBundle\SecurityBundle() is not added to AppKernel and/or security.yml is empty.
The security.context is loaded in the security.xml of the SecurityBundle. In the extension I found that if the config for security is empty the security.xml file is not loaded: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php#L54
On the other hand in the PublishWorkflowChecker the security.context is asked from the container without knowing if it exists: https://github.com/symfony-cmf/CoreBundle/blob/master/PublishWorkflow/PublishWorkflowChecker.php#L101
I am not sure how to fix this, maybe it is just adding a check to see if the security.context is in the container and otherwise generate a token differently. Or add validation in the CoreBundleExtension and if needed throw an error that explains what to do.
hm, interesting. what if you use VIEW_ANONYMOUS privilege? that should not trigger trying to load the token, right?
apart from that i see that there is a method to explicitly set the token, which would prevent the security context from being accessed, but we do not use that at all except in tests. it would be of dubious value to do that, as you could trick yourself into believing you test something different than you actually do. if the anonymous thing works, we should check the container for existence of the security and do a more explanative exception in case things go wrong, saying you either need security or should check against VIEW_ANONYMOUS
Doesn't help. The PublishWorkflowListener first does a check, its default is VIEW, tested it also with VIEW_ANONYMOUS. Anyhow, then the PublishWorkflowChecker::isGranted is called. And there the security.context is always requested: https://github.com/symfony-cmf/CoreBundle/blob/master/PublishWorkflow/PublishWorkflowChecker.php#L138
hm. could we do better than that? like first check what attributes we
are looking for and if its only the anonymous one never try to play with
the security context?
Why not add a if ($container->has('security.context')) { clause? And fallback to the user-set token or generating a new Anonymous Token?
i think when there is no security context we simply should not test for the bypassing role at all. also, we have the getToken method, so i think what would be a lot more logic would be to use getToken also for the bypass check, and fix getToken to work even if there is no context at all. we afterwards expect that the token might be null, so actually getToken could do that and if there is no explicit token and no security context or the security context gave a null token provide the anonymous token.
something like
$token = $this->token;
if (null === $token) {
if ($this->container->has('security.context')) {
$securityContext = $this->container->get('security.context');
$token = $securityContext->getToken();
}
if (null === $token) {
$token = new Anonymous...
}
and then
if ($securityContext
&& (count($attributes) === 1)
&& self::VIEW_ATTRIBUTE === reset($attributes)
&& $securityContext->isGranted($this->bypassingRole)
) {
return true;
}
}