make rest controller more generic to support additional workflows #117
Conversation
There was a problem hiding this comment.
maybe we should also make createphp check http methods case insensitive to avoid confusion with custom actions. or lowercase all custom action http methods.
There was a problem hiding this comment.
we definitely need to validate the methods imho.
There was a problem hiding this comment.
and we should then add a config option to set the allowed methods.
There was a problem hiding this comment.
it is validated here: https://github.com/flack/createphp/blob/master/src/Midgard/CreatePHP/RestService.php#L130
the idea of @uwej711 was to register workflows with a di tag. we could make that tag compiler pass also update the controller with supported methods. would that help? i don't think we can dynamically update the route for this action.
There was a problem hiding this comment.
my concern is making it possible to explicitly disable specific methods in apps. ie. if I want to disable DELETE I do not see how I could prevent users from submitting a DELETE with this PR
There was a problem hiding this comment.
well, for DELETE there is a configuration option to even enable it: symfony-cmf/symfony-cmf-docs#540
when starting to use workflows for other things, you should register an AccessChecker - this has the additional advantage that you can allow different things to different users.
There was a problem hiding this comment.
as long as we have a config option for all possible methods, its fine for me.
There was a problem hiding this comment.
you mean for new methods that we introduce in the bundle?
there is no general configuration for allowed methods. but each workflow we provide indeed should have a configuration to explicitly enable it to prevent unwanted manipulations after updating the bundle.
There was a problem hiding this comment.
with the route definition we open a very large door for people to pass through. while createphp may validate the method, imho its problematic to open a door that is validated at a lower level only. so this is just about security.
second I envision that some people will want to not support specific workflows/methods (doesn't want to allow to update etc) so it would be good to give them config options to control this.
There was a problem hiding this comment.
with the route definition we open a very large door for people to pass
through. while createphp may validate the method, imho its problematic
to open a door that is validated at a lower level only. so this is just
about security.
so do we need to expand on the AccessChecker (i guess it should use
voters like the security checker of symfony) and have a way to configure
the allowed methods?
that just feels very cumbersome when we don't have the workflow tag yet.
also, we just pass the name of that method on to the RestService so i
don't really see such a big problem.
second I envision that some people will want to not support specific
workflows/methods (doesn't want to allow to update etc) so it would be
good to give them config options to control this.
yeah, as i
said
there is AccessChecker for this.
|
For custom workflows we also need to implement the frontend wokflow widget for create.js, that actually triggers the request ... so once we have this I guess we need some useful implementation of a workflow in the sandbox as a reference. |
|
oh, and there was me thinking create.js would just display the name for us :-) but makes sense. i created #119 about further workflow support. so the remaining question for this PR is if the AccessChecker is enough to handle security concerns. |
|
@lsmith77 do you think the AccessChecker is enough for security handling or do we need to take additional steps? right now, we just make it possible that things work when you register a custom workflow. when we introduce the workflow tag, we need to provide an easy solution leveraging the AccessChecker. |
|
sounds like a plan |
make rest controller more generic to support additional workflows
/cc @uwej711
btw for the doc, this is using symfony http handling, meanign X-HTTP-Method-Override is supported as well. when documenting this we should make sure to explain the different ways to configure which workflows should be available (firewall, route, access checker).