From 53450e12218dbfa552c78fd09f7852c2513c97c2 Mon Sep 17 00:00:00 2001
From: Andrew Short <andrewjshort@gmail.com>
Date: Sun, 11 Oct 2009 00:07:24 +0000
Subject: [PATCH] MINOR: Updated Controller to return a 404 on actions that
 don't exist, rather than a 403.

From: Andrew Short <andrewjshort@gmail.com>

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@88506 467b73ca-7a2a-4603-9d3b-597d59a354a9
---
 core/control/Controller.php   | 10 +++++-----
 tests/ControllerTest.php      | 15 ++++++++++++++-
 tests/RequestHandlingTest.php |  4 ++--
 3 files changed, 21 insertions(+), 8 deletions(-)
 mode change 100644 => 100755 tests/RequestHandlingTest.php

diff --git a/core/control/Controller.php b/core/control/Controller.php
index 5d8783c82..62289d5ac 100755
--- a/core/control/Controller.php
+++ b/core/control/Controller.php
@@ -183,6 +183,10 @@ class Controller extends RequestHandler {
 		$this->requestParams = $request->requestVars();
 		if(!$this->action) $this->action = 'index';
 		
+		if(!$this->hasAction($this->action)) {
+			$this->httpError(404, "The action '$this->action' does not exist in class $this->class");
+		}
+		
 		// run & init are manually disabled, because they create infinite loops and other dodgy situations 
 		if(!$this->checkAccessAction($this->action) || in_array(strtolower($this->action), array('run', 'init'))) {
 			return $this->httpError(403, "Action '$this->action' isn't allowed on class $this->class");
@@ -198,11 +202,7 @@ class Controller extends RequestHandler {
 				return $result;
 			}
 		} else {
-			if($this->action == 'index' || $this->hasAction($this->action)) {
-				return $this->getViewer($this->action)->process($this);
-			} else {
-				return $this->httpError(404, "The action '$this->action' does not exist in class $this->class");
-			}
+			return $this->getViewer($this->action)->process($this);
 		}
 	}
 
diff --git a/tests/ControllerTest.php b/tests/ControllerTest.php
index b3edca49a..f2e82b5af 100755
--- a/tests/ControllerTest.php
+++ b/tests/ControllerTest.php
@@ -38,7 +38,7 @@ class ControllerTest extends FunctionalTest {
 		$this->assertEquals(200, $response->getStatusCode());
 		
 		$response = $this->get("ControllerTest_SecuredController/stringaction");
-		$this->assertEquals(403, $response->getStatusCode());
+		$this->assertEquals(404, $response->getStatusCode());
 
 		$response = $this->get("ControllerTest_SecuredController/adminonly");
 		$this->assertEquals(403, $response->getStatusCode());
@@ -113,6 +113,13 @@ class ControllerTest extends FunctionalTest {
 		$this->assertFalse($controller->hasAction('undefined'), 'undefined actions do not exist');
 		$this->assertTrue($controller->hasAction('allowed_action'), 'allowed actions are recognised');
 		$this->assertTrue($controller->hasAction('template_action'), 'action-specific templates are recognised');
+		
+		$unsecured = new ControllerTest_HasAction_Unsecured();
+		
+		$this->assertTrue (
+			$unsecured->hasAction('defined_action'),
+			'Without an allowed_actions, any defined methods are recognised as actions'
+		);
 	}
 	
 }
@@ -188,4 +195,10 @@ class ControllerTest_HasAction extends Controller {
 		'template_action' => 'template'
 	);
 	
+}
+
+class ControllerTest_HasAction_Unsecured extends ControllerTest_HasAction {
+	
+	public function defined_action() {  }
+	
 }
\ No newline at end of file
diff --git a/tests/RequestHandlingTest.php b/tests/RequestHandlingTest.php
old mode 100644
new mode 100755
index 1294db729..9d471ca76
--- a/tests/RequestHandlingTest.php
+++ b/tests/RequestHandlingTest.php
@@ -86,7 +86,7 @@ class RequestHandlingTest extends SapphireTest {
 	function testDisallowedExtendedActions() {
 		/* Actions on magic methods are only accessible if explicitly allowed on the controller. */
 		$response = Director::test("testGoodBase1/extendedMethod");
-		$this->assertEquals(403, $response->getStatusCode());
+		$this->assertEquals(404, $response->getStatusCode());
 		
 		/* Actions on an extension are allowed because they specifically provided appropriate allowed_actions items */
 		$response = Director::test("testGoodBase1/otherExtendedMethod");
@@ -94,7 +94,7 @@ class RequestHandlingTest extends SapphireTest {
 
 		/* The failoverMethod action wasn't explicitly listed and so isnt' allowed */
 		$response = Director::test("testGoodBase1/failoverMethod");
-		$this->assertEquals(403, $response->getStatusCode());
+		$this->assertEquals(404, $response->getStatusCode());
 		
 		/* However, on RequestHandlingTest_AllowedController it has been explicitly allowed */
 		$response = Director::test("RequestHandlingTest_AllowedController/failoverMethod");