Skip to content

Commit

Permalink
use new onlyAllow() method in baked code, to ensure 405 responses hav…
Browse files Browse the repository at this point in the history 
…e required Allow header included
  • Loading branch information
@ceeram
ceeram committed Aug 25, 2012
1 parent 17ba713 commit 27d83ee
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 7 deletions.
12 changes: 7 additions & 5 deletions lib/Cake/Console/Templates/default/actions/controller_actions.ctp
View file
Expand Up @@ -47,10 +47,12 @@
/**
* <?php echo $admin ?>add method
*
* @throws MethodNotAllowedException
* @return void
*/
public function <?php echo $admin ?>add() {
if ($this->request->is('post')) {
if ($this->request->data) {
$this->request->onlyAllow('post');
$this-><?php echo $currentModelName; ?>->create();
if ($this-><?php echo $currentModelName; ?>->save($this->request->data)) {
<?php if ($wannaUseSession): ?>
Expand Down Expand Up @@ -86,6 +88,7 @@
/**
* <?php echo $admin ?>edit method
*
* @throws MethodNotAllowedException
* @throws NotFoundException
* @param string $id
* @return void
Expand All @@ -95,7 +98,8 @@
if (!$this-><?php echo $currentModelName; ?>->exists()) {
throw new NotFoundException(__('Invalid <?php echo strtolower($singularHumanName); ?>'));
}
if ($this->request->is('post') || $this->request->is('put')) {
if ($this->request->data) {
$this->request->onlyAllow('post', 'put');
if ($this-><?php echo $currentModelName; ?>->save($this->request->data)) {
<?php if ($wannaUseSession): ?>
$this->Session->setFlash(__('The <?php echo strtolower($singularHumanName); ?> has been saved'));
Expand Down Expand Up @@ -137,9 +141,7 @@
* @return void
*/
public function <?php echo $admin; ?>delete($id = null) {
if (!$this->request->is('post')) {
throw new MethodNotAllowedException();
}
$this->request->onlyAllow('post', 'delete');
$this-><?php echo $currentModelName; ?>->id = $id;
if (!$this-><?php echo $currentModelName; ?>->exists()) {
throw new NotFoundException(__('Invalid <?php echo strtolower($singularHumanName); ?>'));
Expand Down
6 changes: 4 additions & 2 deletions lib/Cake/Test/Case/Console/Command/Task/ControllerTaskTest.php
View file
Expand Up @@ -353,7 +353,8 @@ public function testBakeActionsUsingSessions() {
$this->assertContains("\$this->set('bakeArticle', \$this->BakeArticle->read(null, \$id)", $result);

$this->assertContains('function add()', $result);
$this->assertContains("if (\$this->request->is('post'))", $result);
$this->assertContains("if (\$this->request->data)", $result);
$this->assertContains("\$this->request->onlyAllow('post')", $result);
$this->assertContains('if ($this->BakeArticle->save($this->request->data))', $result);
$this->assertContains("\$this->Session->setFlash(__('The bake article has been saved'));", $result);

Expand Down Expand Up @@ -392,7 +393,8 @@ public function testBakeActionsWithNoSessions() {
$this->assertContains("\$this->set('bakeArticle', \$this->BakeArticle->read(null, \$id)", $result);

$this->assertContains('function add()', $result);
$this->assertContains("if (\$this->request->is('post'))", $result);
$this->assertContains("if (\$this->request->data)", $result);
$this->assertContains("\$this->request->onlyAllow('post')", $result);
$this->assertContains('if ($this->BakeArticle->save($this->request->data))', $result);

$this->assertContains("\$this->flash(__('The bake article has been saved.'), array('action' => 'index'))", $result);
Expand Down

8 comments on commit 27d83ee

@lorenzo
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a good change. data can be empty but still be a POST request

@lorenzo
Copy link
Member

Choose a reason for hiding this comment

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

Moreover, those methods also accept GET so it would be inaccurate to respond to browser that method only accepts POST

@dereuromark
Copy link
Member

Choose a reason for hiding this comment

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

yeah, the delete method makes sense, but the rest is debatable.

@ceeram
Copy link
Contributor Author

@ceeram ceeram commented on 27d83ee Aug 25, 2012

Choose a reason for hiding this comment

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

propose to partial remove and keep in delete: cc92717

@dereuromark
Copy link
Member

Choose a reason for hiding this comment

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

@ceeram: i guess you could first check on valid post/delete before actually checking for exists() in the db on delete. so the initial order was good for me.

@ceeram
Copy link
Contributor Author

@ceeram ceeram commented on 27d83ee Aug 25, 2012

Choose a reason for hiding this comment

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

I dont agree, if you get methodnotallowed then change method to same uri suddenly you could get 404
if the resource does not exist, your should always get 404, and only get 405 when it exists but wrong method

@markstory
Copy link
Member

Choose a reason for hiding this comment

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

@ceeram I think the change in cc92717 is a good compromise that better communicates how delete() methods should be used, and doesn't tell half truths for add() and edit().

@ceeram
Copy link
Contributor Author

@ceeram ceeram commented on 27d83ee Aug 26, 2012

Choose a reason for hiding this comment

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

cherry-picked the commit to 2.3: abe74ad

Please sign in to comment.