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

Conversation

dbu
Copy link
Member

@dbu dbu commented Jun 22, 2013

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.

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


<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

@rmsint
Copy link

rmsint commented Jun 24, 2013

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
Copy link
Member Author

dbu commented Jun 24, 2013

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.

}
}

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

@dbu
Copy link
Member Author

dbu commented Jun 24, 2013

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.

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)) {
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

</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"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@dbu
Copy link
Member Author

dbu commented Jun 24, 2013

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

@wouterj
Copy link
Member

wouterj commented Jun 25, 2013

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
Copy link
Member Author

dbu commented Jun 26, 2013

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
Copy link

rmsint commented Jun 28, 2013

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
Copy link
Member Author

dbu commented Jun 28, 2013

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
Copy link
Member Author

dbu commented Jul 2, 2013

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

@lsmith77
Copy link
Member

lsmith77 commented Jul 3, 2013

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

@dbu
Copy link
Member Author

dbu commented Jul 3, 2013

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
Copy link
Member

lsmith77 commented Jul 3, 2013

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

@lsmith77
Copy link
Member

fixes #57

@dbu
Copy link
Member Author

dbu commented Jul 12, 2013

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants