From 8192c1942436ff467e87420adb23c64869887f7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jannis=20Mo=C3=9Fhammer?= Date: Mon, 24 Jun 2013 18:46:45 +0200 Subject: [PATCH 1/2] Fix Authentication workflow - The authentication controller now uses the Authentication/Manager class, also there were some issues in the Session creation, this has been removed from the Bootstrap now, as the Controller must decide how to open a session (read-only or read/write). - The tests reflect a few chagnes, as the move from the CSRF token generation to the Formbuilder. - Notificaiton now doesn't use Zend Session refs #4340 --- .../controllers/AuthenticationController.php | 69 +++++++++++++++-- .../layouts/scripts/parts/topbar.phtml | 2 +- .../views/scripts/authentication/login.phtml | 11 ++- config/authentication.ini | 9 ++- library/Icinga/Application/Web.php | 11 --- library/Icinga/Authentication/Manager.php | 13 +++- library/Icinga/Authentication/PhpSession.php | 15 ++-- library/Icinga/Authentication/User.php | 16 ++-- library/Icinga/Protocol/Ldap/Connection.php | 40 +++++----- library/Icinga/Web/ActionController.php | 48 ------------ library/Icinga/Web/Notification.php | 7 +- .../application/views/helpers/QlinkTest.php | 2 +- .../Icinga/Web/ActionControllerTest.php | 74 ------------------- 13 files changed, 134 insertions(+), 183 deletions(-) diff --git a/application/controllers/AuthenticationController.php b/application/controllers/AuthenticationController.php index 593a15f22..315e74eaa 100644 --- a/application/controllers/AuthenticationController.php +++ b/application/controllers/AuthenticationController.php @@ -6,8 +6,9 @@ # namespace Icinga\Application\Controllers; use Icinga\Web\ActionController; -use Icinga\Authentication\Auth; -use Icinga\Web\Notification; +use Icinga\Authentication\Credentials as Credentials; +use Icinga\Authentication\Manager as AuthManager; +use Icinga\Form\Builder as FormBuilder; /** * Class AuthenticationController @@ -25,13 +26,67 @@ class AuthenticationController extends ActionController */ protected $modifiesSession = true; + private function getAuthForm() + { + return array( + 'username' => array( + 'text', + array( + 'label' => t('Username'), + 'required' => true, + ) + ), + 'password' => array( + 'password', + array( + 'label' => t('Password'), + 'required' => true + ) + ), + 'submit' => array( + 'submit', + array( + 'label' => t('Login'), + 'class' => 'pull-right' + ) + ) + ); + } + /** * */ public function loginAction() { $this->replaceLayout = true; - $this->view->form = $this->widget('form', array('name' => 'login')); + $credentials = new Credentials(); + $this->view->form = FormBuilder::fromArray( + $this->getAuthForm(), + array( + "CSRFProtection" => false, // makes no sense here + "model" => &$credentials + ) + ); + try { + $auth = AuthManager::getInstance(null, array( + "writeSession" => true + )); + if ($auth->isAuthenticated()) { + $this->redirectNow('index?_render=body'); + } + if ($this->getRequest()->isPost() && $this->view->form->isSubmitted()) { + $this->view->form->repopulate(); + if ($this->view->form->isValid()) { + if (!$auth->authenticate($credentials)) { + $this->view->form->getElement('password')->addError(t('Please provide a valid username and password')); + } else { + $this->redirectNow('index?_render=body'); + } + } + } + } catch (\Icinga\Exception\ConfigurationError $configError) { + $this->view->errorInfo = $configError->getMessage(); + } } /** @@ -39,11 +94,13 @@ class AuthenticationController extends ActionController */ public function logoutAction() { + $auth = AuthManager::getInstance(null, array( + "writeSession" => true + )); $this->replaceLayout = true; - Auth::getInstance()->forgetAuthentication(); - Notification::success('You have been logged out'); + $auth->removeAuthorization(); $this->_forward('login'); } } -// @codingStandardsIgnoreEnd \ No newline at end of file +// @codingStandardsIgnoreEnd diff --git a/application/layouts/scripts/parts/topbar.phtml b/application/layouts/scripts/parts/topbar.phtml index 961fa1da7..39e25a13a 100755 --- a/application/layouts/scripts/parts/topbar.phtml +++ b/application/layouts/scripts/parts/topbar.phtml @@ -12,7 +12,7 @@
  • escape($this->auth()->getUsername()) + $this->escape($this->auth()->getUser()->getUsername()) ?> diff --git a/application/views/scripts/authentication/login.phtml b/application/views/scripts/authentication/login.phtml index 13b248d7e..01dc6d56f 100644 --- a/application/views/scripts/authentication/login.phtml +++ b/application/views/scripts/authentication/login.phtml @@ -1 +1,10 @@ -form ?> +
    +

    Login

    +form->render() ?> +errorInfo)): ?> +
    + errorInfo ?> +
    + + +
    diff --git a/config/authentication.ini b/config/authentication.ini index 6b56f172e..c01e78b17 100644 --- a/config/authentication.ini +++ b/config/authentication.ini @@ -1,2 +1,9 @@ [users] - +backend=ldap +hostname=localhost +root_dn="ou=people,dc=icinga,dc=org" +bind_dn="cn=admin,cn=config" +bind_pw=admin +user_class=inetOrgPerson +user_name_attribute=uid + diff --git a/library/Icinga/Application/Web.php b/library/Icinga/Application/Web.php index d034d344f..32f611594 100644 --- a/library/Icinga/Application/Web.php +++ b/library/Icinga/Application/Web.php @@ -38,7 +38,6 @@ class Web extends ApplicationBootstrap return $this->loadConfig() ->configureErrorHandling() ->setTimezone() - ->configureSession() ->configureCache() ->prepareZendMvc() ->loadTranslations() @@ -80,16 +79,6 @@ class Web extends ApplicationBootstrap $this->dispatchFrontController(); } - /** - * Configure web session settings - * - * @return self - */ - protected function configureSession() - { - Manager::getInstance(); - return $this; - } protected function loadTranslations() { diff --git a/library/Icinga/Authentication/Manager.php b/library/Icinga/Authentication/Manager.php index 339ff718b..68d20f264 100644 --- a/library/Icinga/Authentication/Manager.php +++ b/library/Icinga/Authentication/Manager.php @@ -6,6 +6,7 @@ namespace Icinga\Authentication; use Icinga\Application\Logger as Logger; use Icinga\Application\Config as Config; +use Icinga\Exception\ConfigurationError as ConfigError; class Manager { @@ -25,7 +26,6 @@ class Manager if ($config === null) { $config = Config::getInstance()->authentication; } - if (isset($options["userBackendClass"])) { $this->userBackend = $options["userBackendClass"]; } elseif ($config->users !== null) { @@ -77,6 +77,15 @@ class Manager public function authenticate(Credentials $credentials, $persist = true) { + if (!$this->userBackend) { + Logger::error("No authentication backend provided, your users will never be able to login."); + throw new ConfigError( + "No authentication backend set - login will never succeed as icinga-web ". + "doesn't know how to determine your user. \n". + "To fix this error, setup your authentication.ini with a valid authentication backend." + ); + return false; + } if (!$this->userBackend->hasUsername($credentials)) { Logger::info("Unknown user %s tried to log in", $credentials->getUsername()); return false; @@ -115,7 +124,7 @@ class Manager public function removeAuthorization() { $this->user = null; - $this->session->delete(); + $this->session->purge(); } public function getUser() diff --git a/library/Icinga/Authentication/PhpSession.php b/library/Icinga/Authentication/PhpSession.php index 8dbc682dc..5da310d73 100644 --- a/library/Icinga/Authentication/PhpSession.php +++ b/library/Icinga/Authentication/PhpSession.php @@ -28,7 +28,6 @@ class PhpSession extends Session private static $DEFAULT_COOKIEOPTIONS = array( 'use_trans_sid' => false, 'use_cookies' => true, - 'use_only_cooies' => true, 'cookie_httponly' => true, 'use_only_cookies' => true, 'hash_function' => true, @@ -51,6 +50,9 @@ class PhpSession extends Session ); } } + if (!is_writable(session_save_path())) { + throw new \Icinga\Exception\ConfigurationError("Can't save session"); + } } private function sessionCanBeChanged() @@ -78,12 +80,7 @@ class PhpSession extends Session } session_name(PhpSession::SESSION_NAME); - - /* - * @todo This is not right - */ - \Zend_Session::start(); - // session_start(); + session_start(); $this->isOpen = true; $this->setAll($_SESSION); return true; @@ -138,16 +135,18 @@ class PhpSession extends Session public function purge() { - if ($this->ensureOpen() && !$this->isFlushed) { + if ($this->ensureOpen()) { $_SESSION = array(); session_destroy(); $this->clearCookies(); + $this->close(); } } private function clearCookies() { if (ini_get("session.use_cookies")) { + Logger::debug("Clearing cookies"); $params = session_get_cookie_params(); setcookie( session_name(), diff --git a/library/Icinga/Authentication/User.php b/library/Icinga/Authentication/User.php index 37c303dfa..e4de7c241 100644 --- a/library/Icinga/Authentication/User.php +++ b/library/Icinga/Authentication/User.php @@ -20,15 +20,15 @@ namespace Icinga\Authentication; */ class User { - private $username = ""; - private $firstname = ""; - private $lastname = ""; - private $email = ""; - private $domain = ""; - private $additionalInformation = array(); + public $username = ""; + public $firstname = ""; + public $lastname = ""; + public $email = ""; + public $domain = ""; + public $additionalInformation = array(); - private $permissions = array(); - private $groups = array(); + public $permissions = array(); + public $groups = array(); public function __construct($username, $firstname = null, $lastname = null, $email = null) { diff --git a/library/Icinga/Protocol/Ldap/Connection.php b/library/Icinga/Protocol/Ldap/Connection.php index 19f466370..403bfd665 100644 --- a/library/Icinga/Protocol/Ldap/Connection.php +++ b/library/Icinga/Protocol/Ldap/Connection.php @@ -4,6 +4,7 @@ namespace Icinga\Protocol\Ldap; +use Icinga\Exception\ConfigurationError as ConfigError; use Icinga\Application\Platform; use Icinga\Application\Config; use Icinga\Application\Logger as Log; @@ -69,6 +70,10 @@ class Connection // '1.3.6.1.1.8' => '8', // Cancel Extended Request ); + protected $use_tls = false; + protected $force_tls = false; + + /** * @var array */ @@ -116,7 +121,8 @@ class Connection $this->bind_dn = $config->bind_dn; $this->bind_pw = $config->bind_pw; $this->root_dn = $config->root_dn; - + $this->use_tls = isset($config->tls) ? $config->tls : false; + $this->force_tls = $this->use_tls; } /** @@ -266,6 +272,7 @@ class Connection // We do not support pagination right now, and there is no chance to // do so for PHP < 5.4. Warnings about "Sizelimit exceeded" will // therefore not be hidden right now. + Log::debug("Query: %s", $query->__toString()); $results = ldap_search( $this->ds, $this->root_dn, @@ -405,7 +412,6 @@ class Connection if (isset($result->supportedExtension)) { foreach ($result->supportedExtension as $oid) { if (array_key_exists($oid, $this->ldap_extension)) { - echo $this->ldap_extension[$oid] . "\n"; // STARTTLS -> läuft mit OpenLDAP } } @@ -429,23 +435,23 @@ class Connection if ($this->ds !== null) { return; } - $use_tls = true; - $force_tls = true; - if ($use_tls) { + if ($this->use_tls) { $this->prepareTlsEnvironment(); } Log::debug("Trying to connect to %s", $this->hostname); $this->ds = ldap_connect($this->hostname, 389); $this->discoverCapabilities(); - Log::debug("Trying ldap_start_tls()"); - if (ldap_start_tls($this->ds)) { - Log::debug("Trying ldap_start_tls() succeeded"); - } else { - Log::warn( - "ldap_start_tls() failed: %s. Does your ldap_ca.conf point to the certificate? ", - ldap_error($this->ds) - ); + if ($this->use_tls) { + Log::debug("Trying ldap_start_tls()"); + if (@ldap_start_tls($this->ds)) { + Log::debug("Trying ldap_start_tls() succeeded"); + } else { + Log::warn( + "ldap_start_tls() failed: %s. Does your ldap_ca.conf point to the certificate? ", + ldap_error($this->ds) + ); + } } @@ -470,12 +476,10 @@ class Connection '***', ldap_error($this->ds) ); - throw new Exception( + throw new ConfigError( sprintf( - 'LDAP connection (%s / %s) failed: %s', - $this->bind_dn, - '***' /* $this->bind_pw */, - ldap_error($this->ds) + 'Could not connect to the authentication server, please '. + 'review your LDAP connection settings.' ) ); } diff --git a/library/Icinga/Web/ActionController.php b/library/Icinga/Web/ActionController.php index 58555bfaf..4d8c02fbe 100755 --- a/library/Icinga/Web/ActionController.php +++ b/library/Icinga/Web/ActionController.php @@ -110,10 +110,6 @@ class ActionController extends ZfController * @todo remove this! */ - $this->allowAccess = true; - $this->init(); - - return null; if ($this->handlesAuthentication() || Manager::getInstance( @@ -338,48 +334,4 @@ class ActionController extends ZfController }*/ } - - /** - * Whether the token parameter is valid - * - * TODO: Could this make use of Icinga\Web\Session once done? - * - * @param int $maxAge Max allowed token age - * @param string $sessionId A specific session id (useful for tests?) - * @return bool - */ - public function hasValidToken($maxAge = 600, $sessionId = null) - { - $sessionId = $sessionId ? $sessionId : session_id(); - $seed = $this->_getParam('seed'); - if (!is_numeric($seed)) { - return false; - } - - // Remove quantitized timestamp portion so maxAge applies - $seed -= (intval(time() / $maxAge) * $maxAge); - $token = $this->_getParam('token'); - return $token === hash('sha256', $sessionId . $seed); - } - - /** - * Get a new seed/token pair - * - * TODO: Could this make use of Icinga\Web\Session once done? - * - * @param int $maxAge Max allowed token age - * @param string $sessionId A specific session id (useful for tests?) - * - * @return array - */ - public function getSeedTokenPair($maxAge = 600, $sessionId = null) - { - $sessionId = $sessionId ? $sessionId : session_id(); - $seed = mt_rand(); - $hash = hash('sha256', $sessionId . $seed); - - // Add quantitized timestamp portion to apply maxAge - $seed += (intval(time() / $maxAge) * $maxAge); - return array($seed, $hash); - } } diff --git a/library/Icinga/Web/Notification.php b/library/Icinga/Web/Notification.php index c858f6011..c99713c7a 100644 --- a/library/Icinga/Web/Notification.php +++ b/library/Icinga/Web/Notification.php @@ -7,7 +7,6 @@ namespace Icinga\Web; use Icinga\Exception\ProgrammingError; use Icinga\Application\Platform; use Icinga\Application\Logger as Log; -use Zend_Session_Namespace as SessionNamespace; /** * Class Notification @@ -149,10 +148,10 @@ class Notification */ final private function __construct() { - $this->session = new SessionNamespace('IcingaNotification'); - if (!is_array($this->session->messages)) { + //$this->session = new SessionNamespace('IcingaNotification'); + //if (!is_array($this->session->messages)) { $this->session->messages = array(); - } + //} if (Platform::isCli()) { $this->cliFlag = true; diff --git a/test/php/application/views/helpers/QlinkTest.php b/test/php/application/views/helpers/QlinkTest.php index 7e8030ee8..881669370 100755 --- a/test/php/application/views/helpers/QlinkTest.php +++ b/test/php/application/views/helpers/QlinkTest.php @@ -4,7 +4,7 @@ require_once('Zend/View/Helper/Abstract.php'); require_once('Zend/View.php'); -require('../../application/views/helpers/Qlink.php'); +require_once('../../application/views/helpers/Qlink.php'); /** diff --git a/test/php/library/Icinga/Web/ActionControllerTest.php b/test/php/library/Icinga/Web/ActionControllerTest.php index 44686595e..642b8939c 100755 --- a/test/php/library/Icinga/Web/ActionControllerTest.php +++ b/test/php/library/Icinga/Web/ActionControllerTest.php @@ -1,80 +1,6 @@ args = $args; - } - - protected function _getParam($paramName, $default = null) { - if(isset($this->args[$paramName])) - return $this->args[$paramName]; - return $default; - } -} - -class ActionTest extends \PHPUnit_Framework_TestCase -{ - public function testSeedGeneration() - { - $action = new ActionTestWrap(); - list($seed1,$token1) = $action->getSeedTokenPair(600,"test"); - list($seed2,$token2) = $action->getSeedTokenPair(600,"test"); - list($seed3,$token3) = $action->getSeedTokenPair(600,"test"); - $this->assertTrue($seed1 != $seed2 && $seed2 != $seed3 && $seed1 != $seed3); - $this->assertTrue($token1 != $token2 && $token2 != $token3 && $token1 != $token3); - } - - public function testSeedValidation() - { - $action = new ActionTestWrap(); - list($seed,$token) = $action->getSeedTokenPair(600,"test"); - $action->setArguments(array( - "seed" => $seed, - "token" => $token - )); - $this->assertTrue($action->hasValidToken(600,"test")); - $this->assertFalse($action->hasValidToken(600,"test2")); - $action->setArguments(array( - "seed" => $seed."ds", - "token" => $token - )); - $this->assertFalse($action->hasValidToken(600,"test")); - $action->setArguments(array( - "seed" => $seed, - "token" => $token."afs" - )); - $this->assertFalse($action->hasValidToken(600,"test")); - } - - public function testMaxAge() - { - $action = new ActionTestWrap(); - list($seed,$token) = $action->getSeedTokenPair(1,"test"); - $action->setArguments(array( - "seed" => $seed, - "token" => $token - )); - $this->assertTrue($action->hasValidToken(1,"test")); - sleep(1); - $this->assertFalse($action->hasValidToken(1,"test")); - } -} From a1327a384cc8b7630f557ac8e8398812b7aadd20 Mon Sep 17 00:00:00 2001 From: Marius Hein Date: Tue, 25 Jun 2013 12:24:52 +0200 Subject: [PATCH 2/2] Fix Authentication workflow Fix ldap backends to use Credentials to work with. Fix some tests to use include right files. refs #4340 --- library/Icinga/Authentication/Backend.php | 3 +++ .../Backend/LdapUserBackend.php | 23 ++++++++----------- library/Icinga/Authentication/UserBackend.php | 15 +++++++++++- .../Icinga/Authentication/PhpSessionTest.php | 1 + 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/library/Icinga/Authentication/Backend.php b/library/Icinga/Authentication/Backend.php index 24fcb6433..2678e396b 100644 --- a/library/Icinga/Authentication/Backend.php +++ b/library/Icinga/Authentication/Backend.php @@ -6,6 +6,9 @@ namespace Icinga\Authentication; class Backend { + /** + * @var UserBackend + */ protected $userBackend; public function __construct($config) diff --git a/library/Icinga/Authentication/Backend/LdapUserBackend.php b/library/Icinga/Authentication/Backend/LdapUserBackend.php index 92484a615..d51a784b6 100644 --- a/library/Icinga/Authentication/Backend/LdapUserBackend.php +++ b/library/Icinga/Authentication/Backend/LdapUserBackend.php @@ -5,6 +5,8 @@ namespace Icinga\Authentication\Backend; use Icinga\Authentication\User as User; +use Icinga\Authentication\UserBackend; +use Icinga\Authentication\Credentials; use Icinga\Protocol\Ldap; class LdapUserBackend implements UserBackend @@ -16,14 +18,11 @@ class LdapUserBackend implements UserBackend $this->connection = new Ldap\Connection($config); } - public function hasUsername($username) + public function hasUsername(Credentials $credential) { - if (!$username) { - return false; - } return $this->connection->fetchOne( - $this->selectUsername($username) - ) === $username; + $this->selectUsername($credential->getUsername()) + ) === $credential->getUsername(); } protected function stripAsterisks($string) @@ -38,19 +37,15 @@ class LdapUserBackend implements UserBackend ->where('sAMAccountName', $this->stripAsterisks($username)); } - public function authenticate($username, $password = null) + public function authenticate(Credentials $credentials) { - if (empty($username) || empty($password)) { - return false; - } - if (!$this->connection->testCredentials( - $this->connection->fetchDN($this->selectUsername($username)), - $password + $this->connection->fetchDN($this->selectUsername($credentials->getUsername())), + $credentials->getPassword() ) ) { return false; } - $user = new User($username); + $user = new User($credentials->getUsername()); return $user; } diff --git a/library/Icinga/Authentication/UserBackend.php b/library/Icinga/Authentication/UserBackend.php index a62b99aa3..104ecafb4 100644 --- a/library/Icinga/Authentication/UserBackend.php +++ b/library/Icinga/Authentication/UserBackend.php @@ -1,15 +1,28 @@