Skip to content

make rest controller more generic to support additional workflows #117

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 20, 2014
Merged

make rest controller more generic to support additional workflows #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 22 additions & 25 deletions Controller/RestController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,24 @@ protected function getModelBySubject(Request $request, $subject)
}

/**
* Handle document PUT (update)
* Handle arbitrary methods with the RestHandler.
*
* Except for the PUT operation to update a document, operations are
* registered as workflows.
*
* @param Request $request
* @param string $subject URL of the subject, ie: cms/simple/news/news-name
* @param string $subject URL of the subject, ie: /cms/simple/news/news-name
*
* @return Response
*
* @throws AccessDeniedException If the action is not allowed by the access
* checker.
*
* @see RestService::run
*
* @since 1.2
*/
public function putDocumentAction(Request $request, $subject)
public function updateDocumentAction(Request $request, $subject)
{
if (!$this->accessChecker->check($request)) {
throw new AccessDeniedException();
Expand All @@ -105,7 +112,7 @@ public function putDocumentAction(Request $request, $subject)
$model = $this->getModelBySubject($request, $subject);
$type = $this->typeFactory->getTypeByObject($model);

$result = $this->restHandler->run($request->request->all(), $type, null, RestService::HTTP_PUT);
$result = $this->restHandler->run($request->request->all(), $type, null, strtolower($request->getMethod()));
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we should also make createphp check http methods case insensitive to avoid confusion with custom actions. or lowercase all custom action http methods.

Copy link
Member

Choose a reason for hiding this comment

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

we definitely need to validate the methods imho.

Copy link
Member

Choose a reason for hiding this comment

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

and we should then add a config option to set the allowed methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

as long as we have a config option for all possible methods, its fine 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.

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

$view = View::create($result)->setFormat('json');

return $this->viewHandler->handle($view, $request);
Expand Down Expand Up @@ -138,33 +145,23 @@ public function postDocumentAction(Request $request)
return $this->viewHandler->handle($view, $request);
}

return Response::create('The document could not be created', 500);
return Response::create('The document was not created', 500);
}

/**
* Handle document deletion.
*
* @param Request $request
* @param string $subject URL of the subject, ie: cms/simple/news/news-name
*
* @return Response
*
* @throws AccessDeniedException If the action is not allowed by the access
* checker.
* @deprecated Use updateDocumentAction
*/
public function deleteDocumentAction(Request $request, $subject)
public function putDocumentAction(Request $request, $subject)
{
if (!$this->accessChecker->check($request)) {
throw new AccessDeniedException();
}

$model = $this->getModelBySubject($request, $subject);
$type = $this->typeFactory->getTypeByObject($model);

$result = $this->restHandler->run($request->request->all(), $type, $subject, RestService::HTTP_DELETE);
$view = View::create($result)->setFormat('json');
return self::updateDocumentAction($request, $subject);
}

return $this->viewHandler->handle($view, $request);
/**
* @deprecated Use updateDocumentAction
*/
public function deleteDocumentAction(Request $request, $subject)
{
return self::updateDocumentAction($request, $subject);
}

/**
Expand Down
12 changes: 2 additions & 10 deletions Resources/config/routing/rest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,16 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/routing http://symfony.com/schema/routing/routing-1.0.xsd">

<route id="cmf_create_put_document" pattern="/{_locale}/symfony-cmf/create/document/{subject}">
<default key="_controller">cmf_create.rest.controller:putDocumentAction</default>
<default key="_format">json</default>
<requirement key="subject">.+</requirement>
<requirement key="_method">PUT</requirement>
</route>

<route id="cmf_create_post_document" pattern="/{_locale}/symfony-cmf/create/document/{subject}">
<default key="_controller">cmf_create.rest.controller:postDocumentAction</default>
<default key="_format">json</default>
<requirement key="_method">POST</requirement>
</route>

<route id="cmf_create_delete_document" pattern="/{_locale}/symfony-cmf/create/document/{subject}">
<default key="_controller">cmf_create.rest.controller:deleteDocumentAction</default>
<route id="cmf_create_put_document" pattern="/{_locale}/symfony-cmf/create/document/{subject}">
<default key="_controller">cmf_create.rest.controller:updateDocumentAction</default>
<default key="_format">json</default>
<requirement key="subject">.+</requirement>
<requirement key="_method">DELETE</requirement>
</route>

<route id="cmf_create_workflows" pattern="/{_locale}/symfony-cmf/create/workflows/{subject}">
Expand Down