From ddc2e3822b0b5f610bb1920599d83f3d6b2ea742 Mon Sep 17 00:00:00 2001
From: Will Rossiter <will.rossiter@gmail.com>
Date: Mon, 18 Nov 2013 16:32:15 +1300
Subject: [PATCH] FIX: ErrorPage generating empty responses for 403/401
 requests

Previously by setting the response status code inside the action, this prevented response bodies from being included due to 403/401 being matched by SS_HTTPResponse::isFinished() (which stops popular

I assume SS_HTTPResponse::isFinished() is valid for the permission error use case (and I would be hesitant to change it) so this simply moves the declaration of the response status code till after the parent has populated the body of the response.
---
 code/model/ErrorPage.php      | 44 +++++++++++++++++++++++------------
 tests/model/ErrorPageTest.php | 13 +++++++++--
 tests/model/ErrorPageTest.yml |  7 +++++-
 3 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/code/model/ErrorPage.php b/code/model/ErrorPage.php
index ac029c7a..e5503ecb 100644
--- a/code/model/ErrorPage.php
+++ b/code/model/ErrorPage.php
@@ -42,7 +42,9 @@ class ErrorPage extends Page {
 	
 	/**
 	 * Get a {@link SS_HTTPResponse} to response to a HTTP error code if an
-	 * {@link ErrorPage} for that code is present.
+	 * {@link ErrorPage} for that code is present. First tries to serve it 
+	 * through the standard SilverStripe request method. Falls back to a static
+	 * file generated when the user hit's save and publish in the CMS
 	 *
 	 * @param int $statusCode
 	 *
@@ -50,11 +52,17 @@ class ErrorPage extends Page {
 	 */
 	public static function response_for($statusCode) {
 		// first attempt to dynamically generate the error page
-		if($errorPage = DataObject::get_one('ErrorPage', "\"ErrorPage\".\"ErrorCode\" = $statusCode")) {
+		$errorPage = ErrorPage::get()->filter(array(
+			"ErrorCode" => $statusCode
+		))->first(); 
+
+		if($errorPage) {
 			Requirements::clear();
 			Requirements::clear_combined_files();
 
-			return ModelAsController::controller_for($errorPage)->handleRequest(new SS_HTTPRequest('GET', ''), DataModel::inst());
+			return ModelAsController::controller_for($errorPage)->handleRequest(
+				new SS_HTTPRequest('GET', ''), DataModel::inst()
+			);
 		}
 		
 		// then fall back on a cached version
@@ -62,13 +70,13 @@ class ErrorPage extends Page {
 			$statusCode, 
 			class_exists('Translatable') ? Translatable::get_current_locale() : null
 		);
-		
+
 		if(file_exists($cachedPath)) {
 			$response = new SS_HTTPResponse();	
 			
 			$response->setStatusCode($statusCode);
 			$response->setBody(file_get_contents($cachedPath));	
-			
+
 			return $response;
 		}
 	}
@@ -129,13 +137,12 @@ class ErrorPage extends Page {
 					}
 				}
 			}
-
 		}
 	}
 
 	/**
-	 * Returns an array of arrays, each of which defines
-	 * properties for a new ErrorPage record.
+	 * Returns an array of arrays, each of which defines properties for a new 
+	 * ErrorPage record.
 	 * 
 	 * @return array
 	 */
@@ -159,6 +166,7 @@ class ErrorPage extends Page {
 				)
 			)
 		);
+
 		$this->extend('getDefaultRecords', $data);
 
 		return $data;
@@ -316,14 +324,20 @@ class ErrorPage extends Page {
  */
 class ErrorPage_Controller extends Page_Controller {
 
-	public function init() {
-		parent::init();
+	/**
+	 * Overload the provided {@link Controller::handleRequest()} to append the
+	 * correct status code post request since otherwise permission related error 
+	 * pages such as 401 and 403 pages won't be rendered due to
+	  * {@link SS_HTTPResponse::isFinished() ignoring the response body.
+	 *
+	 * @param SS_HTTPRequest
+	 * @param DataModel
+	 */
+	public function handleRequest(SS_HTTPRequest $request, DataModel $model = NULL) {
+		$body = parent::handleRequest($request, $model);
+		$this->response->setStatusCode($this->ErrorCode);
 
-		$action = $this->request->param('Action');
-		if(!$action || $action == 'index') {
-			$this->getResponse()->setStatusCode($this->failover->ErrorCode ? $this->failover->ErrorCode : 404);
-		}
-		
+		return $this->response;
 	}
 }
 
diff --git a/tests/model/ErrorPageTest.php b/tests/model/ErrorPageTest.php
index c3c11629..c68d9594 100644
--- a/tests/model/ErrorPageTest.php
+++ b/tests/model/ErrorPageTest.php
@@ -60,5 +60,14 @@ class ErrorPageTest extends FunctionalTest {
 		/* Don't show the error page in the search */
 		$this->assertEquals($page->ShowInSearch, 0, 'Don\'t show the error page in search');
 	}
-	
-}
+
+	public function testBehaviourOf403() {
+		$page = $this->objFromFixture('ErrorPage', '403');
+		$page->publish('Stage', 'Live');
+		
+		$response = $this->get($page->Link());
+		
+		$this->assertEquals($response->getStatusCode(), '403');
+		$this->assertNotNull($response->getBody(), 'We have body text from the error page');
+	}
+}
\ No newline at end of file
diff --git a/tests/model/ErrorPageTest.yml b/tests/model/ErrorPageTest.yml
index 859332ae..e2152199 100644
--- a/tests/model/ErrorPageTest.yml
+++ b/tests/model/ErrorPageTest.yml
@@ -3,4 +3,9 @@ ErrorPage:
       Title: Page Not Found
       URLSegment: page-not-found
       Content: My error page body
-      ErrorCode: 404
\ No newline at end of file
+      ErrorCode: 404
+   403:
+      Title: Permission Failure
+      URLSegment: permission-denied
+      Content: You do not have permission to view this page
+      ErrorCode: 403
\ No newline at end of file