From 702a9c95230c77b758da4a28a9312bfc6f67d97d Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 30 Jun 2015 13:22:54 +0200 Subject: [PATCH 01/25] Form: Show notifications and errors below any descriptions They might be textually related to one or more descriptions. refs #8983 --- library/Icinga/Web/Form.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/Icinga/Web/Form.php b/library/Icinga/Web/Form.php index 679091fde..c450aaef6 100644 --- a/library/Icinga/Web/Form.php +++ b/library/Icinga/Web/Form.php @@ -1043,9 +1043,9 @@ class Form extends Zend_Form ->addDecorator('HtmlTag', array('tag' => 'div', 'class' => 'header')); } - $this->addDecorator('FormErrors', array('onlyCustomFormErrors' => true)) + $this->addDecorator('FormDescriptions') ->addDecorator('FormNotifications') - ->addDecorator('FormDescriptions') + ->addDecorator('FormErrors', array('onlyCustomFormErrors' => true)) ->addDecorator('FormElements') //->addDecorator('HtmlTag', array('tag' => 'dl', 'class' => 'zend_form')) ->addDecorator('Form'); From 147f6be714468c700b3c6987c9dc322c7a0ad9a2 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 30 Jun 2015 14:25:33 +0200 Subject: [PATCH 02/25] Form: Fix notifications * Coding style issues * Notifications were not grouped by type * Notifications of sub-forms were overwriting existing ones --- library/Icinga/Web/Form.php | 127 +++++++++++------- .../Web/Form/Decorator/FormNotifications.php | 50 ++++--- public/css/icinga/forms.less | 27 ++-- 3 files changed, 127 insertions(+), 77 deletions(-) diff --git a/library/Icinga/Web/Form.php b/library/Icinga/Web/Form.php index c450aaef6..24f0e2657 100644 --- a/library/Icinga/Web/Form.php +++ b/library/Icinga/Web/Form.php @@ -39,26 +39,19 @@ class Form extends Zend_Form const DEFAULT_SUFFIX = '_default'; /** - * The type of the notification for the error + * Identifier for notifications of type error */ - const NOTIFICATION_ERROR = 0; + const NOTIFICATION_ERROR = 0; /** - * The type of the notification for the warning + * Identifier for notifications of type warning */ - const NOTIFICATION_WARNING = 2; + const NOTIFICATION_WARNING = 1; /** - * The type of the notification for the info + * Identifier for notifications of type info */ - const NOTIFICATION_INFO = 4; - - /** - * The notifications of the form - * - * @var array - */ - protected $notifications = array(); + const NOTIFICATION_INFO = 2; /** * Whether this form has been created @@ -160,6 +153,13 @@ class Form extends Zend_Form */ protected $descriptions; + /** + * The notifications of this form + * + * @var array + */ + protected $notifications; + /** * Whether the Autosubmit decorator should be applied to this form * @@ -522,6 +522,50 @@ class Form extends Zend_Form return $this->descriptions; } + /** + * Set the notifications for this form + * + * @param array $notifications + * + * @return $this + */ + public function setNotifications(array $notifications) + { + $this->notifications = $notifications; + return $this; + } + + /** + * Add a notification for this form + * + * If $notification is an array the second value should be + * an array as well containing additional HTML properties. + * + * @param string|array $notification + * @param int $type + * + * @return $this + */ + public function addNotification($notification, $type) + { + $this->notifications[$type][] = $notification; + return $this; + } + + /** + * Return the notifications of this form + * + * @return array + */ + public function getNotifications() + { + if ($this->notifications === null) { + return array(); + } + + return $this->notifications; + } + /** * Set whether the Autosubmit decorator should be applied to this form * @@ -1264,55 +1308,48 @@ class Form extends Zend_Form } /** - * Return all form notifications + * Add a error notification and prevent the form from being successfully validated * - * @return array - */ - public function getNotifications() - { - return $this->notifications; - } - - /** - * Add a typed message to the notifications + * @param string|array $message The notfication's message * - * @param string $message The message which would be displayed to the user - * - * @param int $type The type of the message notification - */ - public function addNotification($message, $type = self::NOTIFICATION_ERROR) - { - $this->notifications[$message] = $type; - $this->markAsError(); - } - - /** - * Add a error message to notifications - * - * @param string $message + * @return $this */ public function error($message) { - $this->addNotification($message, $type = self::NOTIFICATION_ERROR); + $this->addNotification($message, self::NOTIFICATION_ERROR); + $this->markAsError(); + return $this; } /** - * Add a warning message to notifications + * Add a warning notification and prevent the form from being successfully validated * - * @param string $message + * @param string|array $message The notfication's message + * + * @return $this */ public function warning($message) { - $this->addNotification($message, $type = self::NOTIFICATION_WARNING); + $this->addNotification($message, self::NOTIFICATION_WARNING); + $this->markAsError(); + return $this; } /** - * Add a info message to notifications + * Add a info notification * - * @param string $message + * @param string|array $message The notfication's message + * @param bool $markAsError Whether to prevent the form from being successfully validated or not + * + * @return $this */ - public function info($message) + public function info($message, $markAsError = true) { - $this->addNotification($message, $type = self::NOTIFICATION_INFO); + $this->addNotification($message, self::NOTIFICATION_INFO); + if ($markAsError) { + $this->markAsError(); + } + + return $this; } } diff --git a/library/Icinga/Web/Form/Decorator/FormNotifications.php b/library/Icinga/Web/Form/Decorator/FormNotifications.php index ed725e625..935c0ce41 100644 --- a/library/Icinga/Web/Form/Decorator/FormNotifications.php +++ b/library/Icinga/Web/Form/Decorator/FormNotifications.php @@ -4,15 +4,16 @@ namespace Icinga\Web\Form\Decorator; use Zend_Form_Decorator_Abstract; -use Icinga\Web\Form as Form; +use Icinga\Exception\ProgrammingError; +use Icinga\Web\Form; /** - * Decorator to add a list of notifications at the top of a form + * Decorator to add a list of notifications at the top or bottom of a form */ class FormNotifications extends Zend_Form_Decorator_Abstract { /** - * Render form descriptions + * Render form notifications * * @param string $content The html rendered so far * @@ -31,16 +32,27 @@ class FormNotifications extends Zend_Form_Decorator_Abstract } $notifications = $this->recurseForm($form); - if (empty($notifications)) { return $content; } $html = '' . $content; + } + } + + /** + * Recurse the given form and return the hints for it and all of its subforms + * + * @param Form $form The form to recurse + * @param mixed $entirelyRequired Set by reference, true means all elements in the hierarchy are + * required, false only a partial subset and null none at all + * @param bool $elementsPassed Whether there were any elements passed during the recursion until now + * + * @return array + */ + protected function recurseForm(Form $form, & $entirelyRequired = null, $elementsPassed = false) + { + $requiredLabels = array(); + if ($form->getRequiredCue() !== null) { + $partiallyRequired = $partiallyOptional = false; + foreach ($form->getElements() as $element) { + if (! in_array($element->getType(), $this->blacklist)) { + if (! $element->isRequired()) { + $partiallyOptional = true; + if ($entirelyRequired) { + $entirelyRequired = false; + } + } else { + $partiallyRequired = true; + if (($label = $element->getDecorator('label')) !== false) { + $requiredLabels[] = $label; + } + } + } + } + + if (! $elementsPassed) { + $elementsPassed = $partiallyRequired || $partiallyOptional; + if ($entirelyRequired === null && $partiallyRequired) { + $entirelyRequired = ! $partiallyOptional; + } + } elseif ($entirelyRequired === null && $partiallyRequired) { + $entirelyRequired = false; + } + } + + $hints = array($form->getHints()); + foreach ($form->getSubForms() as $subForm) { + $hints[] = $this->recurseForm($subForm, $entirelyRequired, $elementsPassed); + } + + if ($entirelyRequired) { + foreach ($requiredLabels as $label) { + $label->setRequiredSuffix(''); + } + } + + return call_user_func_array('array_merge', $hints); + } +} diff --git a/public/css/icinga/forms.less b/public/css/icinga/forms.less index 42f04d7f3..3323520db 100644 --- a/public/css/icinga/forms.less +++ b/public/css/icinga/forms.less @@ -259,3 +259,13 @@ form ul.descriptions { } } } + +form ul.hints { + .non-list-like-list; + padding: 0.5em 0.5em 0 0.5em; + + li { + font-size: 0.8em; + padding-bottom: 0.5em; + } +} \ No newline at end of file From b61c9708bb0d94639b63c1048e09aacafab22b06 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 30 Jun 2015 15:05:57 +0200 Subject: [PATCH 05/25] main-content.less: Use a more friendlier color for info boxes refs #8983 --- public/css/icinga/main-content.less | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/public/css/icinga/main-content.less b/public/css/icinga/main-content.less index 72cffa19b..c51ef8722 100644 --- a/public/css/icinga/main-content.less +++ b/public/css/icinga/main-content.less @@ -104,7 +104,7 @@ table.benchmark { .info-box { padding: 0.5em; border: 1px solid lightgrey; - background-color: #fbfcc5; + background-color: #f2f4fd; } /* Action table */ From 0dc604029ac314e1c9ef921bfd851ec3b32ebedf Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 30 Jun 2015 15:10:17 +0200 Subject: [PATCH 06/25] AdminAccountPage: Do not put an element's description at the top of the form --- .../setup/application/forms/AdminAccountPage.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/modules/setup/application/forms/AdminAccountPage.php b/modules/setup/application/forms/AdminAccountPage.php index 5118ce291..26e274443 100644 --- a/modules/setup/application/forms/AdminAccountPage.php +++ b/modules/setup/application/forms/AdminAccountPage.php @@ -89,9 +89,6 @@ class AdminAccountPage extends Form } if (count($choices) > 1) { - $this->addDescription($this->translate( - 'Below are several options you can choose from for how to define the desired account.' - )); $this->addElement( 'select', 'user_type', @@ -99,6 +96,7 @@ class AdminAccountPage extends Form 'required' => true, 'autosubmit' => true, 'label' => $this->translate('Type Of Definition'), + 'description' => $this->translate('Choose how to define the desired account.'), 'multiOptions' => $choices, 'value' => $choice ) @@ -124,7 +122,7 @@ class AdminAccountPage extends Form 'label' => $this->translate('Username'), 'description' => $this->translate( 'Define the initial administrative account by providing a username that reflects' - . ' a user created later or one that is authenticated using external mechanisms' + . ' a user created later or one that is authenticated using external mechanisms.' ) ) ); @@ -139,7 +137,7 @@ class AdminAccountPage extends Form 'label' => $this->translate('Username'), 'description' => sprintf( $this->translate( - 'Choose a user reported by the %s backend as the initial administrative account', + 'Choose a user reported by the %s backend as the initial administrative account.', 'setup.admin' ), $this->backendConfig['backend'] === 'db' @@ -159,7 +157,7 @@ class AdminAccountPage extends Form 'required' => true, 'label' => $this->translate('Username'), 'description' => $this->translate( - 'Enter the username to be used when creating an initial administrative account' + 'Enter the username to be used when creating an initial administrative account.' ) ) ); @@ -169,7 +167,7 @@ class AdminAccountPage extends Form array( 'required' => true, 'label' => $this->translate('Password'), - 'description' => $this->translate('Enter the password to assign to the newly created account') + 'description' => $this->translate('Enter the password to assign to the newly created account.') ) ); $this->addElement( @@ -179,7 +177,7 @@ class AdminAccountPage extends Form 'required' => true, 'label' => $this->translate('Repeat password'), 'description' => $this->translate( - 'Please repeat the password given above to avoid typing errors' + 'Please repeat the password given above to avoid typing errors.' ), 'validators' => array( array('identical', false, array('new_user_password')) From 774d6ce94aa38c2e6f09c317fd6f4609ab1a9e7b Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Wed, 15 Jul 2015 15:34:11 +0200 Subject: [PATCH 07/25] Fix invalid function call in getCapabilities caused by refactoring --- library/Icinga/Protocol/Ldap/LdapCapabilities.php | 5 +++-- library/Icinga/Protocol/Ldap/LdapConnection.php | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/library/Icinga/Protocol/Ldap/LdapCapabilities.php b/library/Icinga/Protocol/Ldap/LdapCapabilities.php index e98c57818..1ee64eaba 100644 --- a/library/Icinga/Protocol/Ldap/LdapCapabilities.php +++ b/library/Icinga/Protocol/Ldap/LdapCapabilities.php @@ -263,14 +263,15 @@ class LdapCapabilities * Discover the capabilities of the given LDAP server * * @param LdapConnection $connection The ldap connection to use - * @param int $ds The link identifier of the current LDAP connection * * @return LdapCapabilities * * @throws LdapException In case the capability query has failed */ - public static function discoverCapabilities(LdapConnection $connection, $ds) + public static function discoverCapabilities(LdapConnection $connection) { + $ds = $connection->getConnection(); + $fields = array( 'defaultNamingContext', 'namingContexts', diff --git a/library/Icinga/Protocol/Ldap/LdapConnection.php b/library/Icinga/Protocol/Ldap/LdapConnection.php index 099c25006..319293e2a 100644 --- a/library/Icinga/Protocol/Ldap/LdapConnection.php +++ b/library/Icinga/Protocol/Ldap/LdapConnection.php @@ -261,7 +261,7 @@ class LdapConnection implements Selectable, Inspectable { if ($this->capabilities === null) { try { - $this->capabilities = $this->discoverCapabilities($this->getConnection()); + $this->capabilities = LdapCapabilities::discoverCapabilities($this); $this->discoverySuccess = true; } catch (LdapException $e) { Logger::debug($e); @@ -1031,7 +1031,7 @@ class LdapConnection implements Selectable, Inspectable // Try to execute a schema discovery, this may fail if schema discovery is not supported try { - $cap = LdapCapabilities::discoverCapabilities($this, $ds); + $cap = LdapCapabilities::discoverCapabilities($this); $infos []= $cap->getVendor(); $version = $cap->getVersion(); From 11360f36e4aadbf19bf1bbc0a7d52990595020e7 Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Wed, 15 Jul 2015 13:28:01 +0200 Subject: [PATCH 08/25] Fix grammar errors in backend titles --- application/controllers/ConfigController.php | 2 +- application/controllers/UsergroupbackendController.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/application/controllers/ConfigController.php b/application/controllers/ConfigController.php index f077dfe02..206d3c88b 100644 --- a/application/controllers/ConfigController.php +++ b/application/controllers/ConfigController.php @@ -53,7 +53,7 @@ class ConfigController extends Controller )); $tabs->add('usergroupbackend', array( 'title' => $this->translate('Configure how users are associated with groups by Icinga Web 2'), - 'label' => $this->translate('Usergroup Backends'), + 'label' => $this->translate('User Group Backends'), 'url' => 'usergroupbackend/list' )); return $tabs; diff --git a/application/controllers/UsergroupbackendController.php b/application/controllers/UsergroupbackendController.php index 477c1b28d..961c72132 100644 --- a/application/controllers/UsergroupbackendController.php +++ b/application/controllers/UsergroupbackendController.php @@ -160,7 +160,7 @@ class UsergroupbackendController extends Controller )); $tabs->add('usergroupbackend', array( 'title' => $this->translate('Configure how users are associated with groups by Icinga Web 2'), - 'label' => $this->translate('Usergroup Backends'), + 'label' => $this->translate('User Group Backends'), 'url' => 'usergroupbackend/list' )); return $tabs; From 6762ef053e3f49d1896497efcff7b40b9023bcc9 Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Wed, 15 Jul 2015 17:30:39 +0200 Subject: [PATCH 09/25] Upgrade Inspection API Reduce code duplication and stateffullnes by using InspectionException to indicate that an error was thrown, and only using one inspect function. refs #9630 --- library/Icinga/Data/Inspectable.php | 15 +- .../Icinga/Protocol/Ldap/LdapConnection.php | 153 +++++++----------- 2 files changed, 61 insertions(+), 107 deletions(-) diff --git a/library/Icinga/Data/Inspectable.php b/library/Icinga/Data/Inspectable.php index 1e037113a..700f9050b 100644 --- a/library/Icinga/Data/Inspectable.php +++ b/library/Icinga/Data/Inspectable.php @@ -2,7 +2,7 @@ /* Icinga Web 2 | (c) 2013-2015 Icinga Development Team | GPLv2+ */ namespace Icinga\Data; - +use Icinga\Exception\InspectionException; /** * An object for which the user can retrieve status information @@ -12,15 +12,8 @@ interface Inspectable /** * Get information about this objects state * - * @return array An array of strings that describe the state in a human-readable form, each array element - * represents one fact about this object + * @return Inspection + * @throws InspectionException When inspection of the object was not possible */ - public function getInfo(); - - /** - * If this object is working in its current configuration - * - * @return Bool True if the object is working, false if not - */ - public function isHealthy(); + public function inspect(); } diff --git a/library/Icinga/Protocol/Ldap/LdapConnection.php b/library/Icinga/Protocol/Ldap/LdapConnection.php index 319293e2a..7a315c4d8 100644 --- a/library/Icinga/Protocol/Ldap/LdapConnection.php +++ b/library/Icinga/Protocol/Ldap/LdapConnection.php @@ -10,8 +10,10 @@ use Icinga\Application\Logger; use Icinga\Application\Platform; use Icinga\Data\ConfigObject; use Icinga\Data\Inspectable; +use Icinga\Data\Inspection; use Icinga\Data\Selectable; use Icinga\Data\Sortable; +use Icinga\Exception\InspectionException; use Icinga\Exception\ProgrammingError; use Icinga\Protocol\Ldap\LdapException; @@ -162,16 +164,6 @@ class LdapConnection implements Selectable, Inspectable */ protected $encrypted = null; - /** - * @var array - */ - protected $info = null; - - /** - * @var Boolean - */ - protected $healthy = null; - /** * Create a new connection object * @@ -953,21 +945,27 @@ class LdapConnection implements Selectable, Inspectable /** * Prepare and establish a connection with the LDAP server * - * @return resource A LDAP link identifier + * @param Inspection $info Optional inspection to fill with diagnosis info * - * @throws LdapException In case the connection is not possible + * @return resource A LDAP link identifier + * + * @throws LdapException In case the connection is not possible */ - protected function prepareNewConnection() + protected function prepareNewConnection(Inspection $info = null) { + if (! isset($info)) { + $info = new Inspection(''); + } + if ($this->encryption === static::STARTTLS || $this->encryption === static::LDAPS) { $this->prepareTlsEnvironment(); } $hostname = $this->hostname; if ($this->encryption === static::LDAPS) { - $this->logInfo('Connect using LDAPS'); + $info->write('Connect using LDAPS'); if (! $this->validateCertificate) { - $this->logInfo('Skipping certificate validation'); + $info->write('Skip certificate validation'); } $hostname = 'ldaps://' . $hostname; } @@ -984,9 +982,9 @@ class LdapConnection implements Selectable, Inspectable if ($this->encryption === static::STARTTLS) { $this->encrypted = true; - $this->logInfo('Connect using STARTTLS'); + $info->write('Connect using STARTTLS'); if (! $this->validateCertificate) { - $this->logInfo('Skipping certificate validation'); + $info->write('Skip certificate validation'); } if (! ldap_start_tls($ds)) { throw new LdapException('LDAP STARTTLS failed: %s', ldap_error($ds)); @@ -994,60 +992,12 @@ class LdapConnection implements Selectable, Inspectable } elseif ($this->encryption !== static::LDAPS) { $this->encrypted = false; - $this->logInfo('Connect without encryption'); + $info->write('Connect without encryption'); } return $ds; } - /** - * Test if needed aspects of the LDAP connection are working as expected - * - * Extended information about the - * - * @throws \Icinga\Protocol\Ldap\LdapException When a critical aspect of the health test fails - */ - public function testConnectionHealth() - { - $this->healthy = false; - $this->info = array(); - - // Try to connect to the server with the given connection parameters - $ds = $this->prepareNewConnection(); - - // Try a bind-command with the given user credentials, this must not fail - $success = @ldap_bind($ds, $this->bindDn, $this->bindPw); - $msg = sprintf( - 'LDAP bind to %s:%s (%s / %s)', - $this->hostname, - $this->port, - $this->bindDn, - '***' /* $this->bindPw */ - ); - if (! $success) { - throw new LdapException('%s failed: %s', $msg, ldap_error($ds)); - } - $this->logInfo(sprintf($msg . ' successful')); - - // Try to execute a schema discovery, this may fail if schema discovery is not supported - try { - $cap = LdapCapabilities::discoverCapabilities($this); - $infos []= $cap->getVendor(); - - $version = $cap->getVersion(); - if (isset($version)) { - $infos []= $version; - } - $infos []= 'Supports STARTTLS: ' . ($cap->hasStartTls() ? 'True' : 'False'); - $infos []= 'Default naming context: ' . $cap->getDefaultNamingContext(); - $this->info['Discovery Results:'] = $infos; - } catch (Exception $e) { - $this->logInfo('Schema discovery not possible: ', $e->getMessage()); - } - - $this->healthy = true; - } - /** * Set up how to handle StartTLS connections * @@ -1137,43 +1087,54 @@ class LdapConnection implements Selectable, Inspectable return $dir; } - protected function logInfo($message) - { - Logger::debug($message); - if (! isset($this->info)) { - $this->info = array(); - } - $this->info[] = $message; - } - /** - * Get information about this objects state + * Test if needed aspects of the LDAP connection are working as expected * - * @return array An array of strings that describe the state in a human-readable form, each array element - * represents one fact about this object + * @return Inspection Information about the tested connection + * + * @throws InspectionException When inspection failed */ - public function getInfo() + public function inspect() { - if (! isset($this->info)) { - $this->testConnectionHealth(); - } - return $this->info; - } + $insp = new Inspection('Ldap Connection'); - /** - * If this object is working in its current configuration - * - * @return Bool True if the object is working, false if not - */ - public function isHealthy() - { - if (! isset($this->healthy)) { - try { - $this->testConnectionHealth(); - } catch (Exception $e) { + // Try to connect to the server with the given connection parameters + try { + $ds = $this->prepareNewConnection($insp); + } catch (Exception $e) { + throw new InspectionException($e->getMessage(), $insp); + } + + // Try a bind-command with the given user credentials, this must not fail + $success = @ldap_bind($ds, $this->bindDn, $this->bindPw); + $msg = sprintf( + 'LDAP bind to %s:%s (%s / %s)', + $this->hostname, + $this->port, + $this->bindDn, + '***' /* $this->bindPw */ + ); + if (! $success) { + $insp->error(sprintf('%s failed: %s', $msg, ldap_error($ds))); + } + $insp->write(sprintf($msg . ' successful')); + + // Try to execute a schema discovery this may fail if schema discovery is not supported + try { + $cap = LdapCapabilities::discoverCapabilities($this); + $discovery = new Inspection('Discovery Results'); + $discovery->write($cap->getVendor()); + $version = $cap->getVersion(); + if (isset($version)) { + $discovery->write($version); } + $discovery->write('Supports STARTTLS: ' . ($cap->hasStartTls() ? 'True' : 'False')); + $discovery->write('Default naming context: ' . $cap->getDefaultNamingContext()); + $insp->write($discovery); + } catch (Exception $e) { + $insp->write('Schema discovery not possible: ' . $e->getMessage()); } - return $this->healthy; + return $insp; } /** From 276aa43aa26382b973ceab15847d98ab9b81fe93 Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Wed, 15 Jul 2015 18:36:19 +0200 Subject: [PATCH 10/25] Upgrdae Inspection API again Do not use InspectionException any more to reduce complexity of nested inspections, but keep error states in the Inspection object itself. refs #9630 --- library/Icinga/Data/Inspectable.php | 9 +++++---- library/Icinga/Protocol/Ldap/LdapConnection.php | 10 ++++------ 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/library/Icinga/Data/Inspectable.php b/library/Icinga/Data/Inspectable.php index 700f9050b..8b37d9503 100644 --- a/library/Icinga/Data/Inspectable.php +++ b/library/Icinga/Data/Inspectable.php @@ -2,18 +2,19 @@ /* Icinga Web 2 | (c) 2013-2015 Icinga Development Team | GPLv2+ */ namespace Icinga\Data; -use Icinga\Exception\InspectionException; /** * An object for which the user can retrieve status information + * + * This interface is useful for providing summaries or diagnostic information about objects + * to users. */ interface Inspectable { /** - * Get information about this objects state + * Inspect this object to gain extended information about its health * - * @return Inspection - * @throws InspectionException When inspection of the object was not possible + * @return Inspection The inspection result */ public function inspect(); } diff --git a/library/Icinga/Protocol/Ldap/LdapConnection.php b/library/Icinga/Protocol/Ldap/LdapConnection.php index 7a315c4d8..fb20a59ef 100644 --- a/library/Icinga/Protocol/Ldap/LdapConnection.php +++ b/library/Icinga/Protocol/Ldap/LdapConnection.php @@ -1088,11 +1088,9 @@ class LdapConnection implements Selectable, Inspectable } /** - * Test if needed aspects of the LDAP connection are working as expected + * Inspect if this LDAP Connection is working as expected * - * @return Inspection Information about the tested connection - * - * @throws InspectionException When inspection failed + * @return Inspection Inspection result */ public function inspect() { @@ -1102,7 +1100,7 @@ class LdapConnection implements Selectable, Inspectable try { $ds = $this->prepareNewConnection($insp); } catch (Exception $e) { - throw new InspectionException($e->getMessage(), $insp); + return $insp->error($e->getMessage()); } // Try a bind-command with the given user credentials, this must not fail @@ -1115,7 +1113,7 @@ class LdapConnection implements Selectable, Inspectable '***' /* $this->bindPw */ ); if (! $success) { - $insp->error(sprintf('%s failed: %s', $msg, ldap_error($ds))); + return $insp->error(sprintf('%s failed: %s', $msg, ldap_error($ds))); } $insp->write(sprintf($msg . ' successful')); From cf8b760adedc66a4252af7ba6ce84b24a76326ab Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Wed, 15 Jul 2015 19:32:12 +0200 Subject: [PATCH 11/25] Use Inspection API in LdapResourceForm refs #9630 --- .../Config/Resource/LdapResourceForm.php | 22 +++++++++---------- .../Icinga/Protocol/Ldap/LdapConnection.php | 5 ++++- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/application/forms/Config/Resource/LdapResourceForm.php b/application/forms/Config/Resource/LdapResourceForm.php index 604e34199..b385610a8 100644 --- a/application/forms/Config/Resource/LdapResourceForm.php +++ b/application/forms/Config/Resource/LdapResourceForm.php @@ -154,19 +154,17 @@ class LdapResourceForm extends Form */ public static function isValidResource(Form $form) { - try { - $resource = ResourceFactory::createResource(new ConfigObject($form->getValues())); - $resource->bind(); - } catch (Exception $e) { - $msg = $form->translate('Connectivity validation failed, connection to the given resource not possible.'); - if (($error = $e->getMessage())) { - $msg .= ' (' . $error . ')'; - } - - $form->addError($msg); - return false; + $result = ResourceFactory::createResource(new ConfigObject($form->getValues()))->inspect(); + if ($result->hasError()) { + $form->addError(sprintf( + '%s (%s)', + $form->translate('Connectivity validation failed, connection to the given resource not possible.'), + $result->getError() + )); } - return true; + // TODO: display diagnostics in $result->toArray() to the user + + return ! $result->hasError(); } } diff --git a/library/Icinga/Protocol/Ldap/LdapConnection.php b/library/Icinga/Protocol/Ldap/LdapConnection.php index fb20a59ef..2e97b8c9b 100644 --- a/library/Icinga/Protocol/Ldap/LdapConnection.php +++ b/library/Icinga/Protocol/Ldap/LdapConnection.php @@ -945,7 +945,7 @@ class LdapConnection implements Selectable, Inspectable /** * Prepare and establish a connection with the LDAP server * - * @param Inspection $info Optional inspection to fill with diagnosis info + * @param Inspection $info Optional inspection to fill with diagnostic info * * @return resource A LDAP link identifier * @@ -1090,6 +1090,9 @@ class LdapConnection implements Selectable, Inspectable /** * Inspect if this LDAP Connection is working as expected * + * Check if connection, bind and encryption is working as expected and get additional + * information about the used + * * @return Inspection Inspection result */ public function inspect() From 59c4f8d056ad6eb03a6d95655e725ed82f20e42f Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Wed, 15 Jul 2015 19:35:25 +0200 Subject: [PATCH 12/25] Use Inspection API in User Backend Form refs #9630 --- .../Config/UserBackend/LdapBackendForm.php | 26 ++++----- .../Authentication/User/LdapUserBackend.php | 53 ++++++++++++++++--- 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/application/forms/Config/UserBackend/LdapBackendForm.php b/application/forms/Config/UserBackend/LdapBackendForm.php index 9151134c3..6427fbf34 100644 --- a/application/forms/Config/UserBackend/LdapBackendForm.php +++ b/application/forms/Config/UserBackend/LdapBackendForm.php @@ -4,6 +4,8 @@ namespace Icinga\Forms\Config\UserBackend; use Exception; +use Icinga\Authentication\User\LdapUserBackend; +use Icinga\Data\Inspection; use Icinga\Web\Form; use Icinga\Data\ConfigObject; use Icinga\Data\ResourceFactory; @@ -184,22 +186,16 @@ class LdapBackendForm extends Form */ public static function isValidUserBackend(Form $form) { - try { - $ldapUserBackend = UserBackend::create(null, new ConfigObject($form->getValues())); - $ldapUserBackend->assertAuthenticationPossible(); - } catch (AuthenticationException $e) { - if (($previous = $e->getPrevious()) !== null) { - $form->addError($previous->getMessage()); - } else { - $form->addError($e->getMessage()); - } - - return false; - } catch (Exception $e) { - $form->addError(sprintf($form->translate('Unable to validate authentication: %s'), $e->getMessage())); - return false; + /** + * @var $result Inspection + */ + $result = UserBackend::create(null, new ConfigObject($form->getValues()))->inspect(); + if ($result->hasError()) { + $form->addError($result->getError()); } - return true; + // TODO: display diagnostics in $result->toArray() to the user + + return ! $result->hasError(); } } diff --git a/library/Icinga/Authentication/User/LdapUserBackend.php b/library/Icinga/Authentication/User/LdapUserBackend.php index c6efd0673..ecaa08ef4 100644 --- a/library/Icinga/Authentication/User/LdapUserBackend.php +++ b/library/Icinga/Authentication/User/LdapUserBackend.php @@ -5,6 +5,8 @@ namespace Icinga\Authentication\User; use DateTime; use Icinga\Data\ConfigObject; +use Icinga\Data\Inspectable; +use Icinga\Data\Inspection; use Icinga\Exception\AuthenticationException; use Icinga\Exception\ProgrammingError; use Icinga\Repository\LdapRepository; @@ -13,7 +15,7 @@ use Icinga\Protocol\Ldap\LdapException; use Icinga\Protocol\Ldap\Expression; use Icinga\User; -class LdapUserBackend extends LdapRepository implements UserBackendInterface +class LdapUserBackend extends LdapRepository implements UserBackendInterface, Inspectable { /** * The base DN to use for a query @@ -315,24 +317,32 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface *
  • The specified userClass has the property specified by userNameAttribute
    • * * + * @param Inspection $info Optional inspection to fill with diagnostic info + * * @throws AuthenticationException When authentication is not possible */ - public function assertAuthenticationPossible() + public function assertAuthenticationPossible(Inspection $insp = null) { + if (! isset($insp)) { + $insp = new Inspection(''); + } try { $result = $this->select()->fetchRow(); } catch (LdapException $e) { throw new AuthenticationException('Connection not possible.', $e); } + $insp->write('Connection possible.'); + $msg = sprintf( + 'objects with objectClass "%s" in DN "%s" (Filter: %s)', + $this->userClass, + $this->baseDn ?: $this->ds->getDn(), + $this->filter ?: 'None' + ); if ($result === false) { - throw new AuthenticationException( - 'No objects with objectClass "%s" in DN "%s" found. (Filter: %s)', - $this->userClass, - $this->baseDn ?: $this->ds->getDn(), - $this->filter ?: 'None' - ); + throw new AuthenticationException('No ' . $msg . 'found'); } + $insp->write($msg . ' exist'); if (! isset($result->user_name)) { throw new AuthenticationException( @@ -377,4 +387,31 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface ); } } + + /** + * Inspect if this LDAP User Backend is working as expected + * + * @return Inspection Inspection result + */ + public function inspect() + { + $result = new Inspection('Ldap User Backend'); + + // inspect the used connection to get more diagnostic info in case the connection is not working + $result->write($this->ds->inspect()); + + try { + $this->assertAuthenticationPossible($result); + $result->write('User count: ' . $this->select()->count()); + } catch (AuthenticationException $e) { + if (($previous = $e->getPrevious()) !== null) { + $result->error($previous->getMessage()); + } else { + $result->error($e->getMessage()); + } + } catch (Exception $e) { + $result->error(sprintf('Unable to validate authentication: %s', $e->getMessage())); + } + return $result; + } } From 6b8e5da76d0e7a5219ce8b58fd4118d0c93a3bea Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 11:34:15 +0200 Subject: [PATCH 13/25] Move all assertion functions into the inspect functions Reduce code duplication and add class Inspection refs #9630 --- .../Authentication/User/LdapUserBackend.php | 73 +++++----- library/Icinga/Data/Inspection.php | 127 ++++++++++++++++++ 2 files changed, 161 insertions(+), 39 deletions(-) create mode 100644 library/Icinga/Data/Inspection.php diff --git a/library/Icinga/Authentication/User/LdapUserBackend.php b/library/Icinga/Authentication/User/LdapUserBackend.php index ecaa08ef4..e8ae9a579 100644 --- a/library/Icinga/Authentication/User/LdapUserBackend.php +++ b/library/Icinga/Authentication/User/LdapUserBackend.php @@ -308,49 +308,14 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface, In } /** - * Probe the backend to test if authentication is possible - * - * Try to bind to the backend and fetch a single user to check if: - *
      - *
    • Connection credentials are correct and the bind is possible
      • - *
    • At least one user exists
      • - *
    • The specified userClass has the property specified by userNameAttribute
      • - *
    - * + * @param Inspection $info Optional inspection to fill with diagnostic info * * @throws AuthenticationException When authentication is not possible */ public function assertAuthenticationPossible(Inspection $insp = null) { - if (! isset($insp)) { - $insp = new Inspection(''); - } - try { - $result = $this->select()->fetchRow(); - } catch (LdapException $e) { - throw new AuthenticationException('Connection not possible.', $e); - } - $insp->write('Connection possible.'); - $msg = sprintf( - 'objects with objectClass "%s" in DN "%s" (Filter: %s)', - $this->userClass, - $this->baseDn ?: $this->ds->getDn(), - $this->filter ?: 'None' - ); - if ($result === false) { - throw new AuthenticationException('No ' . $msg . 'found'); - } - $insp->write($msg . ' exist'); - - if (! isset($result->user_name)) { - throw new AuthenticationException( - 'UserNameAttribute "%s" not existing in objectClass "%s"', - $this->userNameAttribute, - $this->userClass - ); - } } /** @@ -389,7 +354,15 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface, In } /** - * Inspect if this LDAP User Backend is working as expected + * Inspect if this LDAP User Backend is working as expected by probing the backend + * and testing if thea uthentication is possible + * + * Try to bind to the backend and fetch a single user to check if: + *
      + *
    • Connection credentials are correct and the bind is possible
      • + *
    • At least one user exists
      • + *
    • The specified userClass has the property specified by userNameAttribute
      • + *
    * * @return Inspection Inspection result */ @@ -399,9 +372,31 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface, In // inspect the used connection to get more diagnostic info in case the connection is not working $result->write($this->ds->inspect()); - try { - $this->assertAuthenticationPossible($result); + try { + $res = $this->select()->fetchRow(); + } catch (LdapException $e) { + throw new AuthenticationException('Connection not possible.', $e); + } + $result->write('Connection possible.'); + $msg = sprintf( + 'objects with objectClass "%s" in DN "%s" (Filter: %s)', + $this->userClass, + $this->baseDn ?: $this->ds->getDn(), + $this->filter ?: 'None' + ); + if ($res === false) { + throw new AuthenticationException('No ' . $msg . 'found'); + } + $result->write($msg . ' exist'); + if (! isset($res->user_name)) { + throw new AuthenticationException( + 'UserNameAttribute "%s" not existing in objectClass "%s"', + $this->userNameAttribute, + $this->userClass + ); + } + $result->write('User count: ' . $this->select()->count()); } catch (AuthenticationException $e) { if (($previous = $e->getPrevious()) !== null) { diff --git a/library/Icinga/Data/Inspection.php b/library/Icinga/Data/Inspection.php new file mode 100644 index 000000000..eea91617e --- /dev/null +++ b/library/Icinga/Data/Inspection.php @@ -0,0 +1,127 @@ +description = $description; + } + + /** + * Get the name of this Inspection + * + * @return mixed + */ + public function getDescription() + { + return $this->description; + } + + /** + * Append the given log entry or nested inspection + * + * @throws ProgrammingError When called after erroring + * + * @param $entry string|Inspection A log entry or nested inspection + */ + public function write($entry) + { + if (isset($this->error)) { + throw new ProgrammingError('Inspection object used after error'); + } + if ($entry instanceof Inspection) { + $this->log[$entry->description] = $entry->toArray(); + } else { + $this->log[] = $entry; + } + } + + /** + * Append the given log entry and fail this inspection with the given error + * + * @param $entry string|Inspection A log entry or nested inspection + * + * @throws ProgrammingError When called multiple times + * + * @return this fluent interface + */ + public function error($entry) + { + if (isset($this->error)) { + throw new ProgrammingError('Inspection object used after error'); + } + $this->write($entry); + $this->error = $entry; + return $this; + } + + /** + * If the inspection resulted in an error + * + * @return bool + */ + public function hasError() + { + return isset($this->error); + } + + /** + * The error that caused the inspection to fail + * + * @return Inspection|string + */ + public function getError() + { + return $this->error; + } + + /** + * Convert the inspection to an array + * + * @return array An array of strings that describe the state in a human-readable form, each array element + * represents one log entry about this object. + */ + public function toArray() + { + return $this->log; + } + + /** + * Return a text representation of the inspection log entries + */ + public function __toString() + { + return sprintf( + 'Inspection: description: "%s" error: "%s"', + $this->description, + $this->error + ); + } +} From da5ceb0e73b37fd738e372d732e5c2784629932c Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 12:22:34 +0200 Subject: [PATCH 14/25] Fix broken unit tests Fix unit tests that were broken by API changes to the resource form classes and contemplate life choices. refs #9630 --- .../Config/Resource/LdapResourceFormTest.php | 29 ++++++++++++--- .../UserBackend/LdapBackendFormTest.php | 35 ++++++++++++++++--- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/test/php/application/forms/Config/Resource/LdapResourceFormTest.php b/test/php/application/forms/Config/Resource/LdapResourceFormTest.php index 859288256..bef49edca 100644 --- a/test/php/application/forms/Config/Resource/LdapResourceFormTest.php +++ b/test/php/application/forms/Config/Resource/LdapResourceFormTest.php @@ -26,14 +26,16 @@ class LdapResourceFormTest extends BaseTestCase public function testValidLdapResourceIsValid() { $this->setUpResourceFactoryMock( - Mockery::mock()->shouldReceive('bind')->once()->getMock() + Mockery::mock()->shouldReceive('inspect')->andReturn(self::createInspector(false))->getMock() ); // Passing array(null) is required to make Mockery call the constructor... $form = Mockery::mock('Icinga\Forms\Config\Resource\LdapResourceForm[getView]', array(null)); $form->shouldReceive('getView->escape') ->with(Mockery::type('string')) - ->andReturnUsing(function ($s) { return $s; }); + ->andReturnUsing(function ($s) { + return $s; + }); $form->setTokenDisabled(); $this->assertTrue( @@ -49,14 +51,16 @@ class LdapResourceFormTest extends BaseTestCase public function testInvalidLdapResourceIsNotValid() { $this->setUpResourceFactoryMock( - Mockery::mock()->shouldReceive('bind')->andThrow('\Exception')->getMock() + Mockery::mock()->shouldReceive('inspect')->andReturn(self::createInspector(true))->getMock() ); // Passing array(null) is required to make Mockery call the constructor... $form = Mockery::mock('Icinga\Forms\Config\Resource\LdapResourceForm[getView]', array(null)); $form->shouldReceive('getView->escape') ->with(Mockery::type('string')) - ->andReturnUsing(function ($s) { return $s; }); + ->andReturnUsing(function ($s) { + return $s; + }); $form->setTokenDisabled(); $this->assertFalse( @@ -72,4 +76,21 @@ class LdapResourceFormTest extends BaseTestCase ->with(Mockery::type('Icinga\Data\ConfigObject')) ->andReturn($resourceMock); } + + public static function createInspector($error = false, $log = array('log')) + { + if (! $error) { + $calls = array( + 'hasError' => false, + 'toArray' => $log + ); + } else { + $calls = array( + 'hasError' => true, + 'getError' => 'Error', + 'toArray' => $log + ); + } + return Mockery::mock('Icinga\Data\Inspection', $calls); + } } diff --git a/test/php/application/forms/Config/UserBackend/LdapBackendFormTest.php b/test/php/application/forms/Config/UserBackend/LdapBackendFormTest.php index 6fe63adf4..c5bb8c9af 100644 --- a/test/php/application/forms/Config/UserBackend/LdapBackendFormTest.php +++ b/test/php/application/forms/Config/UserBackend/LdapBackendFormTest.php @@ -12,6 +12,7 @@ use Icinga\Data\ConfigObject; use Icinga\Test\BaseTestCase; use Icinga\Forms\Config\UserBackend\LdapBackendForm; use Icinga\Exception\AuthenticationException; +use Tests\Icinga\Forms\Config\Resource\LdapResourceFormTest; class LdapBackendFormTest extends BaseTestCase { @@ -27,15 +28,18 @@ class LdapBackendFormTest extends BaseTestCase */ public function testValidBackendIsValid() { - $ldapUserBackendMock = Mockery::mock('overload:Icinga\Authentication\User\LdapUserBackend'); - $ldapUserBackendMock->shouldReceive('assertAuthenticationPossible')->andReturnNull(); + $ldapUserBackendMock = Mockery::mock('overload:Icinga\Authentication\User\LdapUserBackend') + ->shouldReceive('inspect') + ->andReturn(self::createInspector(false))->getMock(); $this->setUpUserBackendMock($ldapUserBackendMock); // Passing array(null) is required to make Mockery call the constructor... $form = Mockery::mock('Icinga\Forms\Config\UserBackend\LdapBackendForm[getView]', array(null)); $form->shouldReceive('getView->escape') ->with(Mockery::type('string')) - ->andReturnUsing(function ($s) { return $s; }); + ->andReturnUsing(function ($s) { + return $s; + }); $form->setTokenDisabled(); $form->setResources(array('test_ldap_backend')); $form->populate(array('resource' => 'test_ldap_backend')); @@ -52,7 +56,9 @@ class LdapBackendFormTest extends BaseTestCase */ public function testInvalidBackendIsNotValid() { - $ldapUserBackendMock = Mockery::mock('overload:Icinga\Authentication\User\LdapUserBackend'); + $ldapUserBackendMock = Mockery::mock('overload:Icinga\Authentication\User\LdapUserBackend') + ->shouldReceive('inspect') + ->andReturn(self::createInspector(true))->getMock(); $ldapUserBackendMock->shouldReceive('assertAuthenticationPossible')->andThrow(new AuthenticationException); $this->setUpUserBackendMock($ldapUserBackendMock); @@ -60,7 +66,9 @@ class LdapBackendFormTest extends BaseTestCase $form = Mockery::mock('Icinga\Forms\Config\UserBackend\LdapBackendForm[getView]', array(null)); $form->shouldReceive('getView->escape') ->with(Mockery::type('string')) - ->andReturnUsing(function ($s) { return $s; }); + ->andReturnUsing(function ($s) { + return $s; + }); $form->setTokenDisabled(); $form->setResources(array('test_ldap_backend')); $form->populate(array('resource' => 'test_ldap_backend')); @@ -77,4 +85,21 @@ class LdapBackendFormTest extends BaseTestCase ->shouldReceive('create') ->andReturn($ldapUserBackendMock); } + + public static function createInspector($error = false, $log = array('log')) + { + if (! $error) { + $calls = array( + 'hasError' => false, + 'toArray' => $log + ); + } else { + $calls = array( + 'hasError' => true, + 'getError' => 'Error', + 'toArray' => $log + ); + } + return Mockery::mock('Icinga\Data\Inspection', $calls); + } } From ffe672c252fbd9f6e7cb0dd9f59a85371768655d Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 13:38:35 +0200 Subject: [PATCH 15/25] Improve message texts and scalabillity Always start uppercase and don't use count() function until we've got a more scalable implementation in the LdapConnection. refs #9630 --- .../Authentication/User/LdapUserBackend.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/library/Icinga/Authentication/User/LdapUserBackend.php b/library/Icinga/Authentication/User/LdapUserBackend.php index e8ae9a579..4a83a1c94 100644 --- a/library/Icinga/Authentication/User/LdapUserBackend.php +++ b/library/Icinga/Authentication/User/LdapUserBackend.php @@ -376,19 +376,18 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface, In try { $res = $this->select()->fetchRow(); } catch (LdapException $e) { - throw new AuthenticationException('Connection not possible.', $e); + throw new AuthenticationException('Connection not possible', $e); } - $result->write('Connection possible.'); - $msg = sprintf( - 'objects with objectClass "%s" in DN "%s" (Filter: %s)', + $result->write('Searching for: ' . sprintf( + 'objectClass "%s" in DN "%s" (Filter: %s)', $this->userClass, $this->baseDn ?: $this->ds->getDn(), $this->filter ?: 'None' - ); + )); if ($res === false) { - throw new AuthenticationException('No ' . $msg . 'found'); + throw new AuthenticationException('Error, no users found in backend'); } - $result->write($msg . ' exist'); + $result->write('Users found in backend'); if (! isset($res->user_name)) { throw new AuthenticationException( 'UserNameAttribute "%s" not existing in objectClass "%s"', @@ -397,7 +396,8 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface, In ); } - $result->write('User count: ' . $this->select()->count()); + // (mj) don't do this until we have a more scalable count() implementation + // $result->write('User count: ' . $this->select()->count()); } catch (AuthenticationException $e) { if (($previous = $e->getPrevious()) !== null) { $result->error($previous->getMessage()); From c55ba6dff4541255504adc787ac1b99f0134fed3 Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 13:51:35 +0200 Subject: [PATCH 16/25] fix coding guideline violations --- library/Icinga/Protocol/Ldap/LdapConnection.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/Icinga/Protocol/Ldap/LdapConnection.php b/library/Icinga/Protocol/Ldap/LdapConnection.php index 2e97b8c9b..38440e029 100644 --- a/library/Icinga/Protocol/Ldap/LdapConnection.php +++ b/library/Icinga/Protocol/Ldap/LdapConnection.php @@ -703,8 +703,7 @@ class LdapConnection implements Selectable, Inspectable array_flip($fields) ); } - } while ( - (! $serverSorting || $limit === 0 || $limit !== count($entries)) + } while ((! $serverSorting || $limit === 0 || $limit !== count($entries)) && ($entry = ldap_next_entry($ds, $entry)) ); From f4054d575bc3f1f2ad596218408e0e19d559246c Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 15:29:45 +0200 Subject: [PATCH 17/25] Add Inspection API to db connection refs #9641 --- .../forms/Config/Resource/DbResourceForm.php | 15 +++---- library/Icinga/Data/Db/DbConnection.php | 43 ++++++++++++++++++- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/application/forms/Config/Resource/DbResourceForm.php b/application/forms/Config/Resource/DbResourceForm.php index 0ee64eda3..75d935361 100644 --- a/application/forms/Config/Resource/DbResourceForm.php +++ b/application/forms/Config/Resource/DbResourceForm.php @@ -129,16 +129,13 @@ class DbResourceForm extends Form */ public static function isValidResource(Form $form) { - try { - $resource = ResourceFactory::createResource(new ConfigObject($form->getValues())); - $resource->getConnection()->getConnection(); - } catch (Exception $e) { - $form->addError( - $form->translate('Connectivity validation failed, connection to the given resource not possible.') - ); - return false; + $result = ResourceFactory::createResource(new ConfigObject($form->getValues()))->inspect(); + if ($result->hasError()) { + $form->addError(sprintf($form->translate('Connectivity validation failed: %s'), $result->getError())); } - return true; + // TODO: display diagnostics in $result->toArray() to the user + + return ! $result->hasError(); } } diff --git a/library/Icinga/Data/Db/DbConnection.php b/library/Icinga/Data/Db/DbConnection.php index 95f3e9235..ba7a3c6f0 100644 --- a/library/Icinga/Data/Db/DbConnection.php +++ b/library/Icinga/Data/Db/DbConnection.php @@ -3,6 +3,9 @@ namespace Icinga\Data\Db; +use Exception; +use Icinga\Data\Inspectable; +use Icinga\Data\Inspection; use PDO; use Iterator; use Zend_Db; @@ -23,7 +26,7 @@ use Icinga\Exception\ProgrammingError; /** * Encapsulate database connections and query creation */ -class DbConnection implements Selectable, Extensible, Updatable, Reducible +class DbConnection implements Selectable, Extensible, Updatable, Reducible, Inspectable { /** * Connection config @@ -435,4 +438,42 @@ class DbConnection implements Selectable, Extensible, Updatable, Reducible return $column . ' ' . $sign . ' ' . $this->dbAdapter->quote($value); } } + + public function inspect() + { + $insp = new Inspection('Db Connection'); + try { + $this->getDbAdapter()->getConnection(); + $config = $this->dbAdapter->getConfig(); + $insp->write(sprintf( + 'Connection to %s as %s on %s:%s successful', + $config['dbname'], + $config['username'], + $config['host'], + $config['port'] + )); + switch ($this->dbType) { + case 'mysql': + $rows = $this->dbAdapter->query( + 'SHOW VARIABLES WHERE variable_name ' . + 'IN (\'version\', \'protocol_version\', \'version_compile_os\');' + )->fetchAll(); + $sqlinsp = new Inspection('MySQL'); + foreach ($rows as $row) { + $sqlinsp->write($row->variable_name . ': ' . $row->value); + } + $insp->write($sqlinsp); + break; + case 'pgsql': + $row = $this->dbAdapter->query('SELECT version();')->fetchAll(); + $sqlinsp = new Inspection('PostgreSQL'); + $sqlinsp->write($row[0]->version); + $insp->write($sqlinsp); + break; + } + } catch (Exception $e) { + return $insp->error(sprintf('Connection failed %s', $e->getMessage())); + } + return $insp; + } } From 31618ce2cf815bd3f5ddb916881b28b4255991c1 Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 16:15:27 +0200 Subject: [PATCH 18/25] Fix unit DB Form unit tests broken by inspection refs #9641 --- .../Config/Resource/DbResourceFormTest.php | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/test/php/application/forms/Config/Resource/DbResourceFormTest.php b/test/php/application/forms/Config/Resource/DbResourceFormTest.php index 3cf23a70a..6aa51dbff 100644 --- a/test/php/application/forms/Config/Resource/DbResourceFormTest.php +++ b/test/php/application/forms/Config/Resource/DbResourceFormTest.php @@ -26,7 +26,7 @@ class DbResourceFormTest extends BaseTestCase public function testValidDbResourceIsValid() { $this->setUpResourceFactoryMock( - Mockery::mock()->shouldReceive('getConnection')->atMost()->twice()->andReturn(Mockery::self())->getMock() + Mockery::mock()->shouldReceive('inspect')->andReturn(self::createInspector(false))->getMock() ); $this->assertTrue( @@ -42,7 +42,7 @@ class DbResourceFormTest extends BaseTestCase public function testInvalidDbResourceIsNotValid() { $this->setUpResourceFactoryMock( - Mockery::mock()->shouldReceive('getConnection')->once()->andThrow('\Exception')->getMock() + Mockery::mock()->shouldReceive('inspect')->andReturn(self::createInspector(true))->getMock() ); $this->assertFalse( @@ -58,4 +58,21 @@ class DbResourceFormTest extends BaseTestCase ->with(Mockery::type('Icinga\Data\ConfigObject')) ->andReturn($resourceMock); } + + public static function createInspector($error = false, $log = array('log')) + { + if (! $error) { + $calls = array( + 'hasError' => false, + 'toArray' => $log + ); + } else { + $calls = array( + 'hasError' => true, + 'getError' => 'Error', + 'toArray' => $log + ); + } + return Mockery::mock('Icinga\Data\Inspection', $calls); + } } From e357960d1ed74bb29227377c2804a6024f3d78bf Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 16:16:55 +0200 Subject: [PATCH 19/25] Add Inspection API to DB backend refs #9641 --- .../Config/UserBackend/DbBackendForm.php | 17 +++++------- .../Authentication/User/DbUserBackend.php | 26 ++++++++++++++++++- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/application/forms/Config/UserBackend/DbBackendForm.php b/application/forms/Config/UserBackend/DbBackendForm.php index f096f3e35..c09ec166c 100644 --- a/application/forms/Config/UserBackend/DbBackendForm.php +++ b/application/forms/Config/UserBackend/DbBackendForm.php @@ -105,18 +105,15 @@ class DbBackendForm extends Form */ public static function isValidUserBackend(Form $form) { - try { - $dbUserBackend = new DbUserBackend(ResourceFactory::createResource($form->getResourceConfig())); - if ($dbUserBackend->select()->where('is_active', true)->count() < 1) { - $form->addError($form->translate('No active users found under the specified database backend')); - return false; - } - } catch (Exception $e) { - $form->addError(sprintf($form->translate('Using the specified backend failed: %s'), $e->getMessage())); - return false; + $backend = new DbUserBackend(ResourceFactory::createResource($form->getResourceConfig())); + $result = $backend->inspect(); + if ($result->hasError()) { + $form->addError(sprintf($form->translate('Using the specified backend failed: %s'), $result->getError())); } - return true; + // TODO: display diagnostics in $result->toArray() to the user + + return ! $result->hasError(); } /** diff --git a/library/Icinga/Authentication/User/DbUserBackend.php b/library/Icinga/Authentication/User/DbUserBackend.php index 0e5dad5fa..3c28a2fdd 100644 --- a/library/Icinga/Authentication/User/DbUserBackend.php +++ b/library/Icinga/Authentication/User/DbUserBackend.php @@ -4,13 +4,15 @@ namespace Icinga\Authentication\User; use Exception; +use Icinga\Data\Inspectable; +use Icinga\Data\Inspection; use PDO; use Icinga\Data\Filter\Filter; use Icinga\Exception\AuthenticationException; use Icinga\Repository\DbRepository; use Icinga\User; -class DbUserBackend extends DbRepository implements UserBackendInterface +class DbUserBackend extends DbRepository implements UserBackendInterface, Inspectable { /** * The algorithm to use when hashing passwords @@ -246,4 +248,26 @@ class DbUserBackend extends DbRepository implements UserBackendInterface { return crypt($password, self::HASH_ALGORITHM . ($salt !== null ? $salt : $this->generateSalt())); } + + /** + * Inspect this object to gain extended information about its health + * + * @return Inspection The inspection result + */ + public function inspect() + { + $insp = new Inspection('Db User Backend'); + $insp->write($this->ds->inspect()); + try { + $users = $this->select()->where('is_active', true)->count(); + if ($users > 1) { + $insp->write(sprintf('%s active users', $users)); + } else { + return $insp->error('0 active users', $users); + } + } catch (Exception $e) { + $insp->error(sprintf('Query failed: %s', $e->getMessage())); + } + return $insp; + } } From 6d209ee20316015490b91a84ab60435ba2ca541d Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 16:22:13 +0200 Subject: [PATCH 20/25] Fix unit DB Form unit tests broken by inspection refs #9641 --- .../Config/UserBackend/DbBackendFormTest.php | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php b/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php index d58ff8a33..f1fe7b84d 100644 --- a/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php +++ b/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php @@ -28,8 +28,8 @@ class DbBackendFormTest extends BaseTestCase { $this->setUpResourceFactoryMock(); Mockery::mock('overload:Icinga\Authentication\User\DbUserBackend') - ->shouldReceive('select->where->count') - ->andReturn(2); + ->shouldReceive('inspect') + ->andReturn(self::createInspector(false)); // Passing array(null) is required to make Mockery call the constructor... $form = Mockery::mock('Icinga\Forms\Config\UserBackend\DbBackendForm[getView]', array(null)); @@ -54,8 +54,8 @@ class DbBackendFormTest extends BaseTestCase { $this->setUpResourceFactoryMock(); Mockery::mock('overload:Icinga\Authentication\User\DbUserBackend') - ->shouldReceive('count') - ->andReturn(0); + ->shouldReceive('inspect') + ->andReturn(self::createInspector(true)); // Passing array(null) is required to make Mockery call the constructor... $form = Mockery::mock('Icinga\Forms\Config\UserBackend\DbBackendForm[getView]', array(null)); @@ -80,4 +80,21 @@ class DbBackendFormTest extends BaseTestCase ->shouldReceive('getResourceConfig') ->andReturn(new ConfigObject()); } + + public static function createInspector($error = false, $log = array('log')) + { + if (! $error) { + $calls = array( + 'hasError' => false, + 'toArray' => $log + ); + } else { + $calls = array( + 'hasError' => true, + 'getError' => 'Error', + 'toArray' => $log + ); + } + return Mockery::mock('Icinga\Data\Inspection', $calls); + } } From 9ba4189617fa8bcfa2c38fbd6c20e628ab7c66d4 Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 16:27:28 +0200 Subject: [PATCH 21/25] fix coding guideline violations --- .../forms/Config/UserBackend/DbBackendFormTest.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php b/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php index f1fe7b84d..cc6919c93 100644 --- a/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php +++ b/test/php/application/forms/Config/UserBackend/DbBackendFormTest.php @@ -35,7 +35,9 @@ class DbBackendFormTest extends BaseTestCase $form = Mockery::mock('Icinga\Forms\Config\UserBackend\DbBackendForm[getView]', array(null)); $form->shouldReceive('getView->escape') ->with(Mockery::type('string')) - ->andReturnUsing(function ($s) { return $s; }); + ->andReturnUsing(function ($s) { + return $s; + }); $form->setTokenDisabled(); $form->setResources(array('test_db_backend')); $form->populate(array('resource' => 'test_db_backend')); @@ -61,7 +63,9 @@ class DbBackendFormTest extends BaseTestCase $form = Mockery::mock('Icinga\Forms\Config\UserBackend\DbBackendForm[getView]', array(null)); $form->shouldReceive('getView->escape') ->with(Mockery::type('string')) - ->andReturnUsing(function ($s) { return $s; }); + ->andReturnUsing(function ($s) { + return $s; + }); $form->setTokenDisabled(); $form->setResources(array('test_db_backend')); $form->populate(array('resource' => 'test_db_backend')); From 547802785501ed0e68b63f272460ad6014d7e28a Mon Sep 17 00:00:00 2001 From: Matthias Jentsch Date: Thu, 16 Jul 2015 16:52:56 +0200 Subject: [PATCH 22/25] Bring back user count in ldap backend inspection We already use count later in the wizard anyways. refs #9630 --- library/Icinga/Authentication/User/LdapUserBackend.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/library/Icinga/Authentication/User/LdapUserBackend.php b/library/Icinga/Authentication/User/LdapUserBackend.php index 4a83a1c94..df2c0a8e0 100644 --- a/library/Icinga/Authentication/User/LdapUserBackend.php +++ b/library/Icinga/Authentication/User/LdapUserBackend.php @@ -387,7 +387,7 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface, In if ($res === false) { throw new AuthenticationException('Error, no users found in backend'); } - $result->write('Users found in backend'); + $result->write(sprintf('%d users found in backend', $this->select()->count())); if (! isset($res->user_name)) { throw new AuthenticationException( 'UserNameAttribute "%s" not existing in objectClass "%s"', @@ -395,9 +395,6 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface, In $this->userClass ); } - - // (mj) don't do this until we have a more scalable count() implementation - // $result->write('User count: ' . $this->select()->count()); } catch (AuthenticationException $e) { if (($previous = $e->getPrevious()) !== null) { $result->error($previous->getMessage()); From 163911ffd7e5f93e240a4616a4fb0d0575281f45 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Tue, 21 Jul 2015 16:47:17 +0200 Subject: [PATCH 23/25] Indicate empty icinga_programstatus table as problem fixes #9695 --- .../BackendAvailabilityMenuItemRenderer.php | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/modules/monitoring/library/Monitoring/Web/Menu/BackendAvailabilityMenuItemRenderer.php b/modules/monitoring/library/Monitoring/Web/Menu/BackendAvailabilityMenuItemRenderer.php index 186351a40..4359d5da5 100644 --- a/modules/monitoring/library/Monitoring/Web/Menu/BackendAvailabilityMenuItemRenderer.php +++ b/modules/monitoring/library/Monitoring/Web/Menu/BackendAvailabilityMenuItemRenderer.php @@ -3,41 +3,50 @@ namespace Icinga\Module\Monitoring\Web\Menu; -use Icinga\Web\Menu as Menu; -use Icinga\Module\Monitoring\Backend\MonitoringBackend; +use Icinga\Web\Menu; use Icinga\Web\Menu\MenuItemRenderer; +use Icinga\Module\Monitoring\Backend\MonitoringBackend; class BackendAvailabilityMenuItemRenderer extends MenuItemRenderer { /** - * Checks whether the monitoring backend is running or not + * Get whether or not the monitoring backend is currently running * - * @return mixed + * @return bool */ protected function isCurrentlyRunning() { - return MonitoringBackend::instance()->select()->from( - 'programstatus', - array( - 'is_currently_running' + $programStatus = MonitoringBackend::instance() + ->select() + ->from( + 'programstatus', + array('is_currently_running') ) - )->getQuery()->fetchRow()->is_currently_running; + ->fetchRow(); + return $programStatus !== false ? (bool) $programStatus : false; } /** - * @see MenuItemRenderer::render() + * {@inheritdoc} */ public function render(Menu $menu) { return $this->getBadge() . $this->createLink($menu); } + /** + * Get the problem badge HTML + * + * @return string + */ protected function getBadge() { - if (! (bool)$this->isCurrentlyRunning()) { + if (! $this->isCurrentlyRunning()) { return sprintf( - '
    %s
    ', - mt('monitoring', 'monitoring backend is not running'), + '
    %d
    ', + sprintf( + mt('monitoring', 'Monitoring backend %s is not running'), MonitoringBackend::instance()->getName() + ), 1 ); } @@ -51,10 +60,12 @@ class BackendAvailabilityMenuItemRenderer extends MenuItemRenderer */ public function getSummary() { - if (! (bool)$this->isCurrentlyRunning()) { + if (! $this->isCurrentlyRunning()) { return array( - 'problems' => 1, - 'title' => mt('monitoring', 'monitoring backend is not running') + 'problems' => 1, + 'title' => sprintf( + mt('monitoring', 'Monitoring backend %s is not running'), MonitoringBackend::instance()->getName() + ) ); } return null; From ce2b6862528df32516d68adac7baae81534a5bf0 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Tue, 21 Jul 2015 16:54:23 +0200 Subject: [PATCH 24/25] Add file and line of logged menu item renderer exceptions fixes #9696 --- library/Icinga/Web/MenuRenderer.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/library/Icinga/Web/MenuRenderer.php b/library/Icinga/Web/MenuRenderer.php index 4dec9404e..7b6ad9856 100644 --- a/library/Icinga/Web/MenuRenderer.php +++ b/library/Icinga/Web/MenuRenderer.php @@ -118,7 +118,13 @@ class MenuRenderer extends RecursiveIteratorIterator try { return $child->getRenderer()->render($child); } catch (Exception $e) { - Logger::error('Could not invoke custom renderer. Exception: '. $e->getMessage()); + Logger::error( + 'Could not invoke custom menu renderer. %s in %s:%d with message: %s', + get_class($e), + $e->getFile(), + $e->getLine(), + $e->getMessage() + ); } } From 45ef285e3ded3547898ed52e752c0344d4cf0bdb Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Tue, 21 Jul 2015 17:32:17 +0200 Subject: [PATCH 25/25] RPM: Let php-Icinga require Zend and Zend's MySQL and PostgreSQL adapters This installs both the MySQL and PostgreSQL libs even if the user only wants to use either MySQL or PostgreSQL. But the depencies installed--mysql-libs and postgresql-libs respectively--are so minimal that this is a good trade off for managing our dependencies atm. fixes #9314 --- icingaweb2.spec | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/icingaweb2.spec b/icingaweb2.spec index 7cddee0d3..e3513e979 100644 --- a/icingaweb2.spec +++ b/icingaweb2.spec @@ -46,8 +46,6 @@ Requires: %{name}-vendor-HTMLPurifier Requires: %{name}-vendor-JShrink Requires: %{name}-vendor-lessphp Requires: %{name}-vendor-Parsedown -Requires: %{zend} -Obsoletes: %{name}-vendor-zend %description @@ -84,6 +82,10 @@ Requires: %{php}-gd %{php}-intl %{?fedora:Requires: php-pecl-imagick} %{?rhel:Requires: php-pecl-imagick} %{?suse_version:Requires: %{php}-gettext %{php}-json %{php}-openssl %{php}-posix} +Requires: %{zend} +Obsoletes: %{name}-vendor-zend +Requires: %{zend}-Db-Adapter-Pdo-Mysql +Requires: %{zend}-Db-Adapter-Pdo-Pgsql %description -n php-Icinga Icinga Web 2 PHP library