From ce9110d22d0f8114ebd5902f5e9577724c85fdc0 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 20 May 2015 10:31:58 +0200 Subject: [PATCH 01/14] Revert "Add proper respond http codes to service and host controller" This reverts commit 6df031dc786256e0bd42f8047d6c308e90abedf6. I revert this commit for the following reasons: - MissingParameterException must not be thrown manually because we have UrlParams::getRequired() which was UrlParams::req() before. - The commit introduces the untranslated string 'host or service'. - 4xx are client, not server errors. - Copy and paste code for the stack trace handling in the ErrorController. refs #6281 --- application/controllers/ErrorController.php | 6 +----- .../application/controllers/HostController.php | 13 +------------ .../application/controllers/ServiceController.php | 13 +------------ 3 files changed, 3 insertions(+), 29 deletions(-) diff --git a/application/controllers/ErrorController.php b/application/controllers/ErrorController.php index b1343dc81..a77d36f4f 100644 --- a/application/controllers/ErrorController.php +++ b/application/controllers/ErrorController.php @@ -34,11 +34,7 @@ class ErrorController extends ActionController $path = preg_split('~/~', $path); $path = array_shift($path); $this->getResponse()->setHttpResponseCode(404); - $title = preg_replace('/\r?\n.*$/s', '', $exception->getMessage()); - $this->view->title = 'Server error: ' . $title; - if ($this->getInvokeArg('displayExceptions')) { - $this->view->stackTrace = $exception->getTraceAsString(); - } + $this->view->message = $this->translate('Page not found.'); if ($modules->hasInstalled($path) && ! $modules->hasEnabled($path)) { $this->view->message .= ' ' . sprintf( $this->translate('Enabling the "%s" module might help!'), diff --git a/modules/monitoring/application/controllers/HostController.php b/modules/monitoring/application/controllers/HostController.php index 7693023eb..2fc2bc7f3 100644 --- a/modules/monitoring/application/controllers/HostController.php +++ b/modules/monitoring/application/controllers/HostController.php @@ -1,7 +1,6 @@ params->get('host') === null) { - throw new MissingParameterException( - $this->translate('Required parameter \'%s\' is missing'), - 'host' - ); - } - $host = new Host($this->backend, $this->params->get('host')); $this->applyRestriction('monitoring/hosts/filter', $host); if ($host->fetch() === false) { - throw new Zend_Controller_Action_Exception( - sprintf($this->translate('Host \'%s\' not found'), $this->params->get('host')), - 404 - ); + throw new Zend_Controller_Action_Exception($this->translate('Host not found')); } $this->object = $host; $this->createTabs(); diff --git a/modules/monitoring/application/controllers/ServiceController.php b/modules/monitoring/application/controllers/ServiceController.php index e61c4685d..d6105df4d 100644 --- a/modules/monitoring/application/controllers/ServiceController.php +++ b/modules/monitoring/application/controllers/ServiceController.php @@ -1,7 +1,6 @@ params->get('host') === null || $this->params->get('service') === null) { - throw new MissingParameterException( - $this->translate('One of the required parameters \'%s\' is missing'), - 'host or service' - ); - } - $service = new Service($this->backend, $this->params->get('host'), $this->params->get('service')); $this->applyRestriction('monitoring/services/filter', $service); if ($service->fetch() === false) { - throw new Zend_Controller_Action_Exception( - sprintf($this->translate('Service \'%s\' not found'), $this->params->get('service')), - 404 - ); + throw new Zend_Controller_Action_Exception($this->translate('Service not found')); } $this->object = $service; $this->createTabs(); From e8c704b98d46abb1304e098b744f61f8d2829c2b Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 20 May 2015 10:36:05 +0200 Subject: [PATCH 02/14] monitoring: Fix HTTP response code when showing an invalid service refs #6281 --- .../application/controllers/ServiceController.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/monitoring/application/controllers/ServiceController.php b/modules/monitoring/application/controllers/ServiceController.php index d6105df4d..2886650c9 100644 --- a/modules/monitoring/application/controllers/ServiceController.php +++ b/modules/monitoring/application/controllers/ServiceController.php @@ -21,17 +21,17 @@ class Monitoring_ServiceController extends MonitoredObjectController /** * Fetch the requested service from the monitoring backend - * - * @throws Zend_Controller_Action_Exception If the service was not found */ public function init() { - $service = new Service($this->backend, $this->params->get('host'), $this->params->get('service')); + $service = new Service( + $this->backend, $this->params->getRequired('host'), $this->params->getRequired('service') + ); $this->applyRestriction('monitoring/services/filter', $service); if ($service->fetch() === false) { - throw new Zend_Controller_Action_Exception($this->translate('Service not found')); + $this->httpNotFound($this->translate('Service not found')); } $this->object = $service; $this->createTabs(); From 0b81a1130f9df955801273c0a91c2c2d3e0ef0cb Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Wed, 20 May 2015 10:38:00 +0200 Subject: [PATCH 03/14] monitoring: Fix HTTP response code when showing an invalid host refs #6281 --- .../monitoring/application/controllers/HostController.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/modules/monitoring/application/controllers/HostController.php b/modules/monitoring/application/controllers/HostController.php index 2fc2bc7f3..464da200c 100644 --- a/modules/monitoring/application/controllers/HostController.php +++ b/modules/monitoring/application/controllers/HostController.php @@ -21,17 +21,15 @@ class Monitoring_HostController extends MonitoredObjectController /** * Fetch the requested host from the monitoring backend - * - * @throws Zend_Controller_Action_Exception If the host was not found */ public function init() { - $host = new Host($this->backend, $this->params->get('host')); + $host = new Host($this->backend, $this->params->getRequired('host')); $this->applyRestriction('monitoring/hosts/filter', $host); if ($host->fetch() === false) { - throw new Zend_Controller_Action_Exception($this->translate('Host not found')); + $this->httpNotFound($this->translate('Host not found')); } $this->object = $host; $this->createTabs(); From c4ed49cb1a0a12ca153606cb1949b42ba740011b Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 21 May 2015 16:43:58 +0200 Subject: [PATCH 04/14] lib: Add HttpNotFoundException The HttpNotFoundException should be used for sending a HTTP 404 response w/ a custom message. There's also Zend_Controller_Action_Exception but this exception will always show 'Page not found' because we want to hide messages generated by Zend, like 'Action "foobar" does not exist and was not trapped in __call()'. refs #6281 --- library/Icinga/Exception/HttpNotFoundException.php | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 library/Icinga/Exception/HttpNotFoundException.php diff --git a/library/Icinga/Exception/HttpNotFoundException.php b/library/Icinga/Exception/HttpNotFoundException.php new file mode 100644 index 000000000..09febf0b4 --- /dev/null +++ b/library/Icinga/Exception/HttpNotFoundException.php @@ -0,0 +1,11 @@ + Date: Thu, 21 May 2015 16:54:00 +0200 Subject: [PATCH 05/14] Throw HttpNotFoundException on Controller::httpNotFound() refs #6281 --- library/Icinga/Web/Controller.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/Icinga/Web/Controller.php b/library/Icinga/Web/Controller.php index 0975d4c13..cdc049c87 100644 --- a/library/Icinga/Web/Controller.php +++ b/library/Icinga/Web/Controller.php @@ -3,7 +3,7 @@ namespace Icinga\Web; -use Zend_Controller_Action_Exception; +use Icinga\Exception\HttpNotFoundException; use Icinga\Data\Sortable; use Icinga\Data\QueryInterface; use Icinga\Web\Controller\ModuleActionController; @@ -55,11 +55,11 @@ class Controller extends ModuleActionController * * @param $message * - * @throws Zend_Controller_Action_Exception + * @throws HttpNotFoundException */ public function httpNotFound($message) { - throw new Zend_Controller_Action_Exception($message, 404); + throw new HttpNotFoundException($message); } /** From 03b4de32530e5b7c3822ca66129230d1debcbc5a Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 21 May 2015 16:56:27 +0200 Subject: [PATCH 06/14] Handle the HttpNotFoundException in the ErrorController refs #6281 --- application/controllers/ErrorController.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/application/controllers/ErrorController.php b/application/controllers/ErrorController.php index a77d36f4f..ba9b44266 100644 --- a/application/controllers/ErrorController.php +++ b/application/controllers/ErrorController.php @@ -3,6 +3,7 @@ use Icinga\Application\Icinga; use Icinga\Application\Logger; +use Icinga\Exception\HttpNotFoundException; use Icinga\Exception\MissingParameterException; use Icinga\Security\SecurityException; use Icinga\Web\Controller\ActionController; @@ -45,8 +46,8 @@ class ErrorController extends ActionController break; default: switch (true) { - case $exception instanceof SecurityException: - $this->getResponse()->setHttpResponseCode(403); + case $exception instanceof HttpNotFoundException: + $this->getResponse()->setHttpResponseCode(404); break; case $exception instanceof MissingParameterException: $this->getResponse()->setHttpResponseCode(400); @@ -55,6 +56,9 @@ class ErrorController extends ActionController 'Missing parameter ' . $exception->getParameter() ); break; + case $exception instanceof SecurityException: + $this->getResponse()->setHttpResponseCode(403); + break; default: $this->getResponse()->setHttpResponseCode(500); break; From 5e520e7b59420371e8b882cd3e2e8d5de5ab1064 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 21 May 2015 16:57:43 +0200 Subject: [PATCH 07/14] Don't display a error message as title too refs #6281 --- application/controllers/ErrorController.php | 2 -- application/views/scripts/error/error.phtml | 5 ----- 2 files changed, 7 deletions(-) diff --git a/application/controllers/ErrorController.php b/application/controllers/ErrorController.php index ba9b44266..e5e98ee7e 100644 --- a/application/controllers/ErrorController.php +++ b/application/controllers/ErrorController.php @@ -63,8 +63,6 @@ class ErrorController extends ActionController $this->getResponse()->setHttpResponseCode(500); break; } - $title = preg_replace('/\r?\n.*$/s', '', $exception->getMessage()); - $this->view->title = 'Server error: ' . $title; $this->view->message = $exception->getMessage(); if ($this->getInvokeArg('displayExceptions')) { $this->view->stackTrace = $exception->getTraceAsString(); diff --git a/application/views/scripts/error/error.phtml b/application/views/scripts/error/error.phtml index d9422474c..5b3480922 100644 --- a/application/views/scripts/error/error.phtml +++ b/application/views/scripts/error/error.phtml @@ -1,13 +1,8 @@
tabs->showOnlyCloseButton() ?> -title): ?> -

escape($title) ?>

-
-message): ?>

escape($message)) ?>

- 
escape($stackTrace) ?>
From 8f42d7a1d34c5867738ddf8aa460f679b3b8c873 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 21 May 2015 17:17:25 +0200 Subject: [PATCH 08/14] monitoring: Fix HTTP response code when showing an invalid downtime refs #6281 --- .../application/controllers/DowntimeController.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/modules/monitoring/application/controllers/DowntimeController.php b/modules/monitoring/application/controllers/DowntimeController.php index c06e0311d..706090f62 100644 --- a/modules/monitoring/application/controllers/DowntimeController.php +++ b/modules/monitoring/application/controllers/DowntimeController.php @@ -35,8 +35,8 @@ class Monitoring_DowntimeController extends Controller */ public function init() { - $downtimeId = $this->params->get('downtime_id'); - + $downtimeId = $this->params->getRequired('downtime_id'); + $this->downtime = $this->backend->select()->from('downtime', array( 'id' => 'downtime_internal_id', 'objecttype' => 'downtime_objecttype', @@ -60,17 +60,17 @@ class Monitoring_DowntimeController extends Controller 'host_display_name', 'service_display_name' ))->where('downtime_internal_id', $downtimeId)->getQuery()->fetchRow(); - - if (false === $this->downtime) { - throw new Zend_Controller_Action_Exception($this->translate('Downtime not found')); + + if ($this->downtime === false) { + $this->httpNotFound($this->translate('Downtime not found')); } - + if (isset($this->downtime->service_description)) { $this->isService = true; } else { $this->isService = false; } - + $this->getTabs() ->add( 'downtime', From fcd7aaef876a9826471aa40df30ad48d81b61e87 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 21 May 2015 17:18:29 +0200 Subject: [PATCH 09/14] lib: Add HttpException as base class for HTTP exceptions refs #6281 --- library/Icinga/Exception/Http/HttpException.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 library/Icinga/Exception/Http/HttpException.php diff --git a/library/Icinga/Exception/Http/HttpException.php b/library/Icinga/Exception/Http/HttpException.php new file mode 100644 index 000000000..4f47f5b67 --- /dev/null +++ b/library/Icinga/Exception/Http/HttpException.php @@ -0,0 +1,13 @@ + Date: Thu, 21 May 2015 17:19:07 +0200 Subject: [PATCH 10/14] lib: Add HttpMethodNotAllowedException At the moment we throw a Zend_Controller_Action_Exception when the HTTP method is not allowed. I'll replace this w/ the exception introduced. refs #6281 --- .../Http/HttpMethodNotAllowedException.php | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 library/Icinga/Exception/Http/HttpMethodNotAllowedException.php diff --git a/library/Icinga/Exception/Http/HttpMethodNotAllowedException.php b/library/Icinga/Exception/Http/HttpMethodNotAllowedException.php new file mode 100644 index 000000000..5826e52de --- /dev/null +++ b/library/Icinga/Exception/Http/HttpMethodNotAllowedException.php @@ -0,0 +1,41 @@ +allowedMethods; + } + + /** + * Set the allowed HTTP methods + * + * @param string $allowedMethods + * + * @return $this + */ + public function setAllowedMethods($allowedMethods) + { + $this->allowedMethods = (string) $allowedMethods; + return $this; + } +} From 71a2324cb9c9e3f817abf8519a5e39bb0817b5e7 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Fri, 22 May 2015 09:12:42 +0200 Subject: [PATCH 11/14] lib: Let Controller::assertHttpMethod() throw a HttpMethodNotAllowedException refs #6281 --- library/Icinga/Web/Controller/ActionController.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/Icinga/Web/Controller/ActionController.php b/library/Icinga/Web/Controller/ActionController.php index b78090a7f..a60c82c8e 100644 --- a/library/Icinga/Web/Controller/ActionController.php +++ b/library/Icinga/Web/Controller/ActionController.php @@ -7,6 +7,7 @@ use Exception; use Icinga\Application\Benchmark; use Icinga\Application\Config; use Icinga\Authentication\Manager; +use Icinga\Exception\Http\HttpMethodNotAllowedException; use Icinga\Exception\IcingaException; use Icinga\Exception\ProgrammingError; use Icinga\File\Pdf; @@ -192,16 +193,17 @@ class ActionController extends Zend_Controller_Action /** * Respond with HTTP 405 if the current request's method is not one of the given methods * - * @param string $httpMethod Unlimited number of allowed HTTP methods + * @param string $httpMethod Unlimited number of allowed HTTP methods * - * @throws \Zend_Controller_Action_Exception If the request method is not one of the given methods + * @throws HttpMethodNotAllowedException If the request method is not one of the given methods */ public function assertHttpMethod($httpMethod) { $httpMethods = array_flip(array_map('strtoupper', func_get_args())); if (! isset($httpMethods[$this->getRequest()->getMethod()])) { - $this->getResponse()->setHeader('Allow', implode(', ', array_keys($httpMethods))); - throw new \Zend_Controller_Action_Exception($this->translate('Method Not Allowed'), 405); + $e = new HttpMethodNotAllowedException($this->translate('Method Not Allowed')); + $e->setAllowedMethods(implode(', ', array_keys($httpMethods))); + throw $e; } } From 3f608fda24276ea9942d0b92236a66d432ad1162 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Fri, 22 May 2015 09:15:52 +0200 Subject: [PATCH 12/14] Handle the HttpMethodNotAllowedException in the ErrorController refs #6281 --- application/controllers/ErrorController.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/application/controllers/ErrorController.php b/application/controllers/ErrorController.php index e5e98ee7e..0acc85bdd 100644 --- a/application/controllers/ErrorController.php +++ b/application/controllers/ErrorController.php @@ -3,6 +3,7 @@ use Icinga\Application\Icinga; use Icinga\Application\Logger; +use Icinga\Exception\Http\HttpMethodNotAllowedException; use Icinga\Exception\HttpNotFoundException; use Icinga\Exception\MissingParameterException; use Icinga\Security\SecurityException; @@ -46,6 +47,10 @@ class ErrorController extends ActionController break; default: switch (true) { + case $exception instanceof HttpMethodNotAllowedException: + $this->getResponse()->setHttpResponseCode(405); + $this->getResponse()->setHeader('Allow', $exception->getAllowedMethods()); + break; case $exception instanceof HttpNotFoundException: $this->getResponse()->setHttpResponseCode(404); break; From e137263c4b061bd4f2b26c96406a8aff4d3a7d22 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Fri, 22 May 2015 09:20:27 +0200 Subject: [PATCH 13/14] monitoring: Fix HTTP response code when showing an invalid comment refs #6281 --- .../application/controllers/CommentController.php | 14 ++++++-------- .../application/controllers/DowntimeController.php | 2 -- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/modules/monitoring/application/controllers/CommentController.php b/modules/monitoring/application/controllers/CommentController.php index c3fcd18f8..4237d598b 100644 --- a/modules/monitoring/application/controllers/CommentController.php +++ b/modules/monitoring/application/controllers/CommentController.php @@ -20,13 +20,11 @@ class Monitoring_CommentController extends Controller /** * Fetch the first comment with the given id and add tabs - * - * @throws Zend_Controller_Action_Exception */ public function init() { - $commentId = $this->params->get('comment_id'); - + $commentId = $this->params->getRequired('comment_id'); + $this->comment = $this->backend->select()->from('comment', array( 'id' => 'comment_internal_id', 'objecttype' => 'comment_objecttype', @@ -41,9 +39,9 @@ class Monitoring_CommentController extends Controller 'host_display_name', 'service_display_name' ))->where('comment_internal_id', $commentId)->getQuery()->fetchRow(); - - if (false === $this->comment) { - throw new Zend_Controller_Action_Exception($this->translate('Comment not found')); + + if ($this->comment === false) { + $this->httpNotFound($this->translate('Comment not found')); } $this->getTabs()->add( @@ -88,7 +86,7 @@ class Monitoring_CommentController extends Controller private function createDelCommentForm() { $this->assertPermission('monitoring/command/comment/delete'); - + $delCommentForm = new DeleteCommentCommandForm(); $delCommentForm->setAction( Url::fromPath('monitoring/comment/show') diff --git a/modules/monitoring/application/controllers/DowntimeController.php b/modules/monitoring/application/controllers/DowntimeController.php index 706090f62..67b8b6a33 100644 --- a/modules/monitoring/application/controllers/DowntimeController.php +++ b/modules/monitoring/application/controllers/DowntimeController.php @@ -30,8 +30,6 @@ class Monitoring_DowntimeController extends Controller /** * Fetch the downtime matching the given id and add tabs - * - * @throws Zend_Controller_Action_Exception */ public function init() { From d2bb74a2f96fe5c4e03c8dec3b8e7206faeba2d9 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Fri, 22 May 2015 09:25:49 +0200 Subject: [PATCH 14/14] lib: Move HttpNotFoundException beneath the Http Exception namespace refs #6281 --- application/controllers/ErrorController.php | 2 +- .../Icinga/Exception/Http/HttpMethodNotAllowedException.php | 4 +--- library/Icinga/Exception/{ => Http}/HttpNotFoundException.php | 4 ++-- library/Icinga/Web/Controller.php | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) rename library/Icinga/Exception/{ => Http}/HttpNotFoundException.php (52%) diff --git a/application/controllers/ErrorController.php b/application/controllers/ErrorController.php index 0acc85bdd..0223a66b7 100644 --- a/application/controllers/ErrorController.php +++ b/application/controllers/ErrorController.php @@ -4,7 +4,7 @@ use Icinga\Application\Icinga; use Icinga\Application\Logger; use Icinga\Exception\Http\HttpMethodNotAllowedException; -use Icinga\Exception\HttpNotFoundException; +use Icinga\Exception\Http\HttpNotFoundException; use Icinga\Exception\MissingParameterException; use Icinga\Security\SecurityException; use Icinga\Web\Controller\ActionController; diff --git a/library/Icinga/Exception/Http/HttpMethodNotAllowedException.php b/library/Icinga/Exception/Http/HttpMethodNotAllowedException.php index 5826e52de..57144a883 100644 --- a/library/Icinga/Exception/Http/HttpMethodNotAllowedException.php +++ b/library/Icinga/Exception/Http/HttpMethodNotAllowedException.php @@ -2,12 +2,10 @@ namespace Icinga\Exception\Http; -use Icinga\Exception\IcingaException; - /** * Exception thrown if the HTTP method is not allowed */ -class HttpMethodNotAllowedException extends IcingaException +class HttpMethodNotAllowedException extends HttpException { /** * Allowed HTTP methods diff --git a/library/Icinga/Exception/HttpNotFoundException.php b/library/Icinga/Exception/Http/HttpNotFoundException.php similarity index 52% rename from library/Icinga/Exception/HttpNotFoundException.php rename to library/Icinga/Exception/Http/HttpNotFoundException.php index 09febf0b4..8ec6b7fcb 100644 --- a/library/Icinga/Exception/HttpNotFoundException.php +++ b/library/Icinga/Exception/Http/HttpNotFoundException.php @@ -1,11 +1,11 @@