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; 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/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..4a83a1c94 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 @@ -306,41 +308,14 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface } /** - * Probe the backend to test if authentication is possible - * - * Try to bind to the backend and fetch a single user to check if: - * + + * @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) { - try { - $result = $this->select()->fetchRow(); - } catch (LdapException $e) { - throw new AuthenticationException('Connection not possible.', $e); - } - 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' - ); - } - - if (! isset($result->user_name)) { - throw new AuthenticationException( - 'UserNameAttribute "%s" not existing in objectClass "%s"', - $this->userNameAttribute, - $this->userClass - ); - } } /** @@ -377,4 +352,61 @@ class LdapUserBackend extends LdapRepository implements UserBackendInterface ); } } + + /** + * 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: + * + * + * @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 { + try { + $res = $this->select()->fetchRow(); + } catch (LdapException $e) { + throw new AuthenticationException('Connection not possible', $e); + } + $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('Error, no users found in backend'); + } + $result->write('Users found in backend'); + if (! isset($res->user_name)) { + throw new AuthenticationException( + 'UserNameAttribute "%s" not existing in objectClass "%s"', + $this->userNameAttribute, + $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()); + } else { + $result->error($e->getMessage()); + } + } catch (Exception $e) { + $result->error(sprintf('Unable to validate authentication: %s', $e->getMessage())); + } + return $result; + } } diff --git a/library/Icinga/Data/Inspectable.php b/library/Icinga/Data/Inspectable.php index 1e037113a..8b37d9503 100644 --- a/library/Icinga/Data/Inspectable.php +++ b/library/Icinga/Data/Inspectable.php @@ -3,24 +3,18 @@ namespace Icinga\Data; - /** * 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 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 The inspection result */ - 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/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 + ); + } +} diff --git a/library/Icinga/Protocol/Ldap/LdapConnection.php b/library/Icinga/Protocol/Ldap/LdapConnection.php index 319293e2a..38440e029 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 * @@ -711,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)) ); @@ -953,21 +944,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 diagnostic 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 +981,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 +991,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 +1086,55 @@ 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 + * Inspect if this LDAP Connection is 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 + * Check if connection, bind and encryption is working as expected and get additional + * information about the used + * + * @return Inspection Inspection result */ - 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) { + return $insp->error($e->getMessage()); + } + + // 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) { + return $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; } /** 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); + } }