refactor publish workflow to use security, cleanup configuration #59

dbu · 2013-06-22T09:30:03Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	the same as master
Fixed tickets	#45, #49, #58
License	MIT
Doc PR	symfony-cmf/symfony-cmf-docs#171

This is a vast simplification and moving towards standard symfony concepts that people already know. If we do this, we get a lot more flexible.

See https://github.com/symfony-cmf/BlogBundle/pull/25/files for an example of how to use this in other bundles. Once we agree on merging this, i will push an updated sandbox and the other affected bundles.

dbu · 2013-06-22T09:31:32Z

DependencyInjection/Configuration.php

@@ -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')


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.

added addDefaultsIfNotSet and an enabled parameter

dbu · 2013-06-23T12:03:00Z

Resources/config/publish_workflow.xml

+
+        <service id="cmf_core.admin_extension.publish_workflow" class="%cmf_core.admin_extension.publish_workflow_class%">
+            <tag name="sonata.admin.extension"/>
+        </service>


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

will just remove it for good measure

rmsint · 2013-06-24T13:20:44Z

How to configure the firewall? Because I think a security check (using the voter) will always require a user session and for optimizing caching rules I think you want to prevent a user session if it is not needed. Or do you always do the publishing check via a different route, so it could be part of the admin/cms firewall?

dbu · 2013-06-24T15:00:13Z

argh. you are right, if i call isGranted without a firewall, the SecurityContext explodes right into my face!
which is very unfortunate. i guess we have to do something different then. which is extremly sad as it would have been so elegant and simple. we could do our own tag and our own service based on the security context that automatically provides a dummy token when there is no firewall in place.
thanks a LOT roel, that problem would have been annoying to only figure out later.

i would not want to tamper with the security context and inject it some dummy token just to make it happy, no idea of the side effects.

there are some positive sides to this too: we can configure the voting to be "unanimous" and will not interfere with the normal symfony security checks. this would also be a performance gain if some other security voters not interested in the cmf topics are registered. maybe we could then still do a "normal" security voter that ties in our workflow, to integrate with firewalled applications. then we still would need the cmf_is_published (but is_granted would work too) and we would have a publish workflow checker but with the signature of the SecurityContext. this will still keep our own code to a minimum and keep us very close to security.

does that sound reasonable? i can go and refactor this thing, but would love some input if i do the right thing before i dig into it.

lsmith77 · 2013-06-24T16:47:12Z

DependencyInjection/CmfCoreExtension.php

+        }
+    }
+
+    public function loadPublishWorkflow($config, XmlFileLoader $loader, ContainerBuilder $container) {


curly brackets needs to be on the next line

dbu · 2013-06-24T20:17:30Z

ok, refactored to be a separate SecurityContext that does not require any login. the changelog should give the gist of the idea. hoping for a bit more feedback, then i will set out to document this thingy.

dbu · 2013-06-24T20:24:31Z

EventListener/PublishWorkflowListener.php

            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)) {


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.

would it none the less make sense to make this a constructor parameter, even if we for now do not support configuring it?

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'?

dbu · 2013-06-24T20:35:33Z

Resources/config/services.xml

    </parameters>

    <services>

        <service id="cmf_core.twig.children_extension" class="%cmf_core.twig_extension_class%">
-            <argument type="service" id="cmf_core.publish_workflow_checker"/>
-            <argument type="service" id="doctrine_phpcr" on-invalid="ignore" />
+            <argument type="service" id="cmf_core.publish_workflow.checker" on-invalid="ignore"/>


when disabled, we pass null and throw exceptions in the twig extension if the user tries to do something that needs the checker

dbu · 2013-06-24T20:37:16Z

also, if its good now, i need to adjust (and add a lot more) tests for this.

wouterj · 2013-06-25T12:10:00Z

Now you changed the config, you must update the Resources/config/schema/core-1.0.xsd file too. It must be in sync with the config as all XML configuration is first validated by that xsd file. When that fails, an urgent exception is thrown and everything breaks.

dbu · 2013-06-26T07:58:23Z

i will try to work on the outstanding issues asap. meanwhile i created the doc PR symfony-cmf/symfony-cmf-docs#171 - i hope explaining the features also helps understanding it and maybe spot issues if there are still some.

rmsint · 2013-06-28T09:02:12Z

Looks like a nice solution too me! Maybe we could add (in documentation or the sandbox) an example how to use it with a typical firewall config? I am thinking about the use case that a user is logged in to the CMS admin, edited a page and requests a preview. In that case the "admin" firewall could be used with the publish workflow and maybe an extra "CAN_PUBLISH" role? Security is a not so easy subject and an example can help here.

dbu · 2013-06-28T13:11:48Z

rmsint: yeah, the sandbox should demo those things. i once started having basic login in the sandbox, but never finished it. thats a prerequisite. created symfony-cmf/cmf-sandbox#184 . for the doc, can you do a more concrete suggestion what we should add in symfony-cmf/symfony-cmf-docs#171 (we should not go too far though, rather build a good example in the sandbox and point to that i think)

for CAN_PUBLISH i don't know if we can do it in 1.0. it would require a field level permission in sonata admin. hm, well i guess just checking the permission and offering to edit the publish info only when you have the right permission could do the trick here... but then the publish fields are now in extension i think, so we would have to refactor that extension... (argh, feature creap!)

Conflicts: Templating/Helper/CmfHelper.php Tests/Functional/Twig/Extension/CmfExtensionHierarchyTest.php Tests/Unit/Templating/Helper/CmfHelperTest.php

dbu · 2013-07-02T20:16:58Z

merged master in and solved conflicts. so do we merge with the failing tests?

lsmith77 · 2013-07-03T06:11:46Z

yes .. the tests that are failing are failing right now already. i will try to get them fixed sometime this week.

dbu · 2013-07-03T16:22:31Z

i will wait merging this until we have tagged the next beta, ok? this is a bit of a BC break for a bunch of bundles and i can't clean up those things this week i fear.

lsmith77 · 2013-07-03T17:33:22Z

hmm OK. I want to fix the issues exposed by the tests. I guess I will do it in top of this branch then to prevent a merge nightmare

totally refactored the handling for depth in the CmfHelper

lsmith77 · 2013-07-11T12:19:22Z

fixes #57

Refactored to inject helper rather than extending it.

Conflicts: Resources/config/services.xml

dbu · 2013-07-12T12:14:50Z

ok, i refactored the interfaces and this should now be ready to merge. glad for some reviewing

refactor publish workflow to use security, cleanup configuration

dantleech · 2013-07-14T16:01:46Z

PublishWorkflow/PublishWorkflowChecker.php

    {
-        $this->currentTime = $currentTime;
+        if (null === $this->token) {
+            $securityContext = $this->container->get('security.context');


Out of interest - why lazy load the security.context? Scope?

security.context is one of the biggest services of Symfony2, I'm not sure it's done because of that.

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.

refactor publish workflow to use security, cleanup configuration

991a8d0

dbu reviewed Jun 22, 2013
View reviewed changes

dbu mentioned this pull request Jun 22, 2013

adjust to workflow moved to security symfony-cmf/blog-bundle#25

Merged

dbu reviewed Jun 23, 2013
View reviewed changes

lsmith77 reviewed Jun 24, 2013
View reviewed changes

dbu reviewed Jun 24, 2013
View reviewed changes

refactoring to have a separate check

6c7aada

dbu reviewed Jun 24, 2013
View reviewed changes

dbu mentioned this pull request Jun 25, 2013

[WIP] Added Templating Helper for PHP templates #62

Merged

dbu mentioned this pull request Jun 26, 2013

documentation for publish workflow symfony-cmf/symfony-cmf-docs#171

Merged

cleanup after feedback

65ccec2

Merge remote-tracking branch 'origin/master' into workflow-security

e750115

Conflicts: Templating/Helper/CmfHelper.php Tests/Functional/Twig/Extension/CmfExtensionHierarchyTest.php Tests/Unit/Templating/Helper/CmfHelperTest.php

lsmith77 and others added 4 commits July 7, 2013 23:10

totally refactored the handling for depth in the CmfHelper

c7a67ae

Merge pull request #67 from symfony-cmf/workflow-security-test-fixes

5afa058

totally refactored the handling for depth in the CmfHelper

Refactored to inject helper rather than extending it.

87fd7f3

Fixed service.xml and added functional test

45f1fba

dantleech and others added 4 commits July 11, 2013 18:45

Removed extra methods by extending Twig_Extension

4587c0c

Merge pull request #68 from dantleech/inject-helper

93fb382

Refactored to inject helper rather than extending it.

split workflow editing interfaces

003d8cd

Merge remote-tracking branch 'origin/master' into workflow-security

89bf726

Conflicts: Resources/config/services.xml

dbu added 3 commits July 13, 2013 11:01

adjust the extension test

e0d41d8

doc cleanups

b516cd7

adjust test to fixed testing component

fc927c9

dbu added a commit that referenced this pull request Jul 14, 2013

Merge pull request #59 from symfony-cmf/workflow-security

1af0e02

refactor publish workflow to use security, cleanup configuration

dbu merged commit 1af0e02 into master Jul 14, 2013

dbu deleted the workflow-security branch July 14, 2013 14:43

dantleech reviewed Jul 14, 2013
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor publish workflow to use security, cleanup configuration #59

refactor publish workflow to use security, cleanup configuration #59

dbu commented Jun 22, 2013

dbu Jun 22, 2013

lsmith77 Jun 24, 2013

dbu Jun 24, 2013

dbu Jun 23, 2013

dbu Jun 24, 2013

rmsint commented Jun 24, 2013

dbu commented Jun 24, 2013

lsmith77 Jun 24, 2013

dbu commented Jun 24, 2013

dbu Jun 24, 2013

lsmith77 Jun 25, 2013

dbu Jun 25, 2013

dbu Jun 24, 2013

dbu commented Jun 24, 2013

wouterj commented Jun 25, 2013

dbu commented Jun 26, 2013

rmsint commented Jun 28, 2013

dbu commented Jun 28, 2013

dbu commented Jul 2, 2013

lsmith77 commented Jul 3, 2013

dbu commented Jul 3, 2013

lsmith77 commented Jul 3, 2013

lsmith77 commented Jul 11, 2013

dbu commented Jul 12, 2013

dantleech Jul 14, 2013

wouterj Jul 14, 2013

dbu Jul 15, 2013

refactor publish workflow to use security, cleanup configuration #59

refactor publish workflow to use security, cleanup configuration #59

Conversation

dbu commented Jun 22, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmsint commented Jun 24, 2013

dbu commented Jun 24, 2013

Choose a reason for hiding this comment

dbu commented Jun 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Jun 24, 2013

wouterj commented Jun 25, 2013

dbu commented Jun 26, 2013

rmsint commented Jun 28, 2013

dbu commented Jun 28, 2013

dbu commented Jul 2, 2013

lsmith77 commented Jul 3, 2013

dbu commented Jul 3, 2013

lsmith77 commented Jul 3, 2013

lsmith77 commented Jul 11, 2013

dbu commented Jul 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment