From 25530a3caf7ea6714f22830a4792756507a722b0 Mon Sep 17 00:00:00 2001 From: cash Date: Sun, 14 Nov 2010 23:22:13 +0000 Subject: Refs #1417 Elgg core now passes back useful messages to user when log in fails - uservalidationbyemail is next git-svn-id: http://code.elgg.org/elgg/trunk@7317 36083f99-b078-4883-b0ff-0f9b5a30f544 --- actions/login.php | 64 ++++++++++++--------------- engine/classes/ElggPAM.php | 93 +++++++++++++++++++++++++++++++++++++++ engine/classes/LoginException.php | 10 +++++ engine/lib/api.php | 8 ++-- engine/lib/pam.php | 22 ++------- engine/lib/sessions.php | 78 ++++++++++++++++++++------------ engine/tests/services/api.php | 7 ++- languages/en.php | 7 +++ 8 files changed, 198 insertions(+), 91 deletions(-) create mode 100644 engine/classes/ElggPAM.php create mode 100644 engine/classes/LoginException.php diff --git a/actions/login.php b/actions/login.php index 936d0a7d9..1b4fbe1fd 100644 --- a/actions/login.php +++ b/actions/login.php @@ -12,53 +12,45 @@ $persistent = get_input("persistent", FALSE); $result = FALSE; if (empty($username) || empty($password)) { - register_error(elgg_echo('loginerror')); + register_error(elgg_echo('login:empty')); forward(); } -// check first if logging in with email address +// check if logging in with email address +// @todo Are usernames with @ not allowed? if (strpos($username, '@') !== FALSE && ($users = get_user_by_email($username))) { $username = $users[0]->username; } -if ($user = authenticate($username, $password)) { - $result = login($user, $persistent); +$result = elgg_authenticate($username, $password); +if ($result !== true) { + register_error($result); + forward(REFERER); } -// forward to correct page -if ($result) { - system_message(elgg_echo('loginok')); +$user = get_user_by_username($username); +if (!$user) { + register_error(elgg_echo('login:baduser')); + forward(REFERER); +} - if (isset($_SESSION['last_forward_from']) && $_SESSION['last_forward_from']) { - $forward_url = $_SESSION['last_forward_from']; - unset($_SESSION['last_forward_from']); +try { + login($user, $persistent); +} catch (LoginException $e) { + register_error($e->getMessage()); + forward(REFERER); +} - forward($forward_url); - } else { - if (get_input('returntoreferer')) { - forward(REFERER); - } else { - // forward to index for front page overrides. - // index will forward to dashboard if appropriate. - forward('index.php'); - } - } +// forward to correct page +if (isset($_SESSION['last_forward_from']) && $_SESSION['last_forward_from']) { + $forward_url = $_SESSION['last_forward_from']; + unset($_SESSION['last_forward_from']); +} elseif (get_input('returntoreferer')) { + $forward_url = REFERER; } else { - register_error(elgg_echo('loginerror')); - // // let a plugin hook say why login failed or react to it. - // $params = array( - // 'username' => $username, - // 'password' => $password, - // 'persistent' => $persistent, - // 'user' => $user - // ); - // - // // Returning FALSE to this function will generate a standard - // // "Could not log you in" message. - // // Plugins should use this hook to provide details, and then return TRUE. - // if (!elgg_trigger_plugin_hook('failed_login', 'user', $params, FALSE)) { - // register_error(elgg_echo('loginerror')); - // } + // forward to main index page + $forward_url = ''; } -forward(REFERRER); +system_message(elgg_echo('loginok')); +forward($forward_url); diff --git a/engine/classes/ElggPAM.php b/engine/classes/ElggPAM.php new file mode 100644 index 000000000..a3e4f9a77 --- /dev/null +++ b/engine/classes/ElggPAM.php @@ -0,0 +1,93 @@ +policy = $policy; + $this->messages = array('sufficient' => array(), 'required' => array()); + } + + /** + * Authenticate a set of credentials against a policy + * This function will process all registered PAM handlers or stop when the first + * handler fails. A handler fails by either returning false or throwing an + * exception. The advantage of throwing an exception is that it returns a message + * that can be passed to the user. The processing order of the handlers is + * determined by the order that they were registered. + * + * If $credentials are provided, the PAM handler should authenticate using the + * provided credentials. If not, then credentials should be prompted for or + * otherwise retrieved (eg from the HTTP header or $_SESSION). + * + * @param array $credentials Credentials array dependant on policy type + * @return bool + */ + public function authenticate($credentials) { + global $_PAM_HANDLERS; + + $authenticated = false; + + foreach ($_PAM_HANDLERS[$this->policy] as $k => $v) { + $handler = $v->handler; + $importance = $v->importance; + + try { + // Execute the handler + if ($handler($credentials)) { + $authenticated = true; + } else { + if ($importance == 'required') { + $this->messages['required'][] = "$handler:failed"; + return false; + } else { + $this->messages['sufficient'][] = "$handler:failed"; + } + } + } catch (Exception $e) { + if ($importance == 'required') { + $this->messages['required'][] = $e->getMessage(); + return false; + } else { + $this->messages['sufficient'][] = $e->getMessage(); + } + } + } + + return $authenticated; + } + + /** + * Get a failure message to display to user + * + * @return string + */ + public function getFailureMessage() { + $message = elgg_echo('auth:nopams'); + if (!empty($this->messages['required'])) { + $message = $this->messages['required'][0]; + } elseif (!empty($this->messages['sufficient'])) { + $message = $this->messages['sufficient'][0]; + } + + return elgg_trigger_plugin_hook('fail', 'auth', $this->messages, $message); + } +} diff --git a/engine/classes/LoginException.php b/engine/classes/LoginException.php new file mode 100644 index 000000000..9189a6afd --- /dev/null +++ b/engine/classes/LoginException.php @@ -0,0 +1,10 @@ +authenticate() !== true) { throw new APIException(elgg_echo('APIException:APIAuthenticationFailed')); } } - $user_auth_result = pam_authenticate(); + $user_pam = new ElggPAM('user'); + $user_auth_result = $user_pam->authenticate(); // check if user authentication is required if ($API_METHODS[$method]["require_user_auth"] == true) { if ($user_auth_result == false) { - throw new APIException(elgg_echo('APIException:UserAuthenticationFailed')); + throw new APIException($user_pam->getFailureMessage()); } } diff --git a/engine/lib/pam.php b/engine/lib/pam.php index 21cfdbbb9..f1df3feba 100644 --- a/engine/lib/pam.php +++ b/engine/lib/pam.php @@ -14,12 +14,13 @@ * For more information on PAMs see: * http://www.freebsd.org/doc/en/articles/pam/index.html * + * @see ElggPAM + * * @package Elgg.Core * @subpackage Authentication.PAM */ $_PAM_HANDLERS = array(); -$_PAM_HANDLERS_MSG = array(); /** * Register a PAM handler. @@ -66,25 +67,8 @@ function unregister_pam_handler($handler, $policy = "user") { unset($_PAM_HANDLERS[$policy][$handler]); } -/** - * Attempt to authenticate. - * This function will process all registered PAM handlers or stop when the first - * handler fails. A handler fails by either returning false or throwing an - * exception. The advantage of throwing an exception is that it returns a message - * through the global $_PAM_HANDLERS_MSG which can be used in communication with - * a user. The order that handlers are processed is determined by the order that - * they were registered. - * - * If $credentials are provided the PAM handler should authenticate using the - * provided credentials, if not then credentials should be prompted for or - * otherwise retrieved (eg from the HTTP header or $_SESSION). - * - * @param mixed $credentials Mixed PAM handler specific credentials (e.g. username, password) - * @param string $policy The policy type, default is "user" - * - * @return bool true if authenticated, false if not. - */ function pam_authenticate($credentials = NULL, $policy = "user") { + elgg_deprecated_notice('pam_authenticate has been deprecated for ElggPAM', 1.8); global $_PAM_HANDLERS, $_PAM_HANDLERS_MSG; $_PAM_HANDLERS_MSG = array(); diff --git a/engine/lib/sessions.php b/engine/lib/sessions.php index b4722d38c..c42af2ed3 100644 --- a/engine/lib/sessions.php +++ b/engine/lib/sessions.php @@ -127,6 +127,26 @@ function elgg_is_admin_user($user_guid) { return FALSE; } +/** + * Perform user authentication with a given username and password. + * + * @see login + * + * @param string $username The username + * @param string $password The password + * + * @return true|string True or an error message on failure + */ +function elgg_authenticate($username, $password) { + $pam = new ElggPAM('user'); + $credentials = array('username' => $username, 'password' => $password); + $result = $pam->authenticate($credentials); + if (!$result) { + return $pam->getFailureMessage(); + } + return true; +} + /** * Perform standard authentication with a given username and password. * Returns an ElggUser object for use with login. @@ -138,12 +158,14 @@ function elgg_is_admin_user($user_guid) { * * @return ElggUser|false The authenticated user object, or false on failure. */ - function authenticate($username, $password) { - if (pam_authenticate(array('username' => $username, 'password' => $password))) { + elgg_deprecated_notice('authenticate() has been deprecated for elgg_authenticate()', 1.8); + $pam = new ElggPAM('user'); + $credentials = array('username' => $username, 'password' => $password); + $result = $pam->authenticate($credentials); + if ($result) { return get_user_by_username($username); } - return false; } @@ -152,31 +174,33 @@ function authenticate($username, $password) { * it against a known user. * * @param array $credentials Associated array of credentials passed to - * pam_authenticate. This function expects + * Elgg's PAM system. This function expects * 'username' and 'password' (cleartext). * * @return bool + * @throws LoginException */ function pam_auth_userpass($credentials = NULL) { - if (is_array($credentials) && ($credentials['username']) && ($credentials['password'])) { - if ($user = get_user_by_username($credentials['username'])) { - // User has been banned, so prevent from logging in - if ($user->isBanned()) { - return FALSE; - } + if (!is_array($credentials) && (!$credentials['username']) && (!$credentials['password'])) { + return false; + } - if ($user->password == generate_user_password($user, $credentials['password'])) { - return TRUE; - } else { - // Password failed, log. - log_login_failure($user->guid); - } + $user = get_user_by_username($credentials['username']); + if (!$user) { + throw new LoginException(elgg_echo('LoginException:UsernameFailure')); + } - } + if (check_rate_limit_exceeded($user->guid)) { + throw new LoginException(elgg_echo('LoginException:AccountLocked')); } - return FALSE; + if ($user->password !== generate_user_password($user, $credentials['password'])) { + log_login_failure($user->guid); + throw new LoginException(elgg_echo('LoginException:PasswordFailure')); + } + + return true; } /** @@ -207,7 +231,7 @@ function log_login_failure($user_guid) { * * @param int $user_guid User GUID * - * @return bool on success (success = user has no logged failed attempts) + * @return bool true on success (success = user has no logged failed attempts) */ function reset_login_failure_count($user_guid) { $user_guid = (int)$user_guid; @@ -270,26 +294,22 @@ function check_rate_limit_exceeded($user_guid) { /** * Logs in a specified ElggUser. For standard registration, use in conjunction - * with authenticate. + * with elgg_authenticate. * - * @see authenticate + * @see elgg_authenticate * * @param ElggUser $user A valid Elgg user object * @param boolean $persistent Should this be a persistent login? * - * @return bool Whether login was successful + * @return true or throws exception + * @throws LoginException */ function login(ElggUser $user, $persistent = false) { global $CONFIG; // User is banned, return false. if ($user->isBanned()) { - return false; - } - - // Check rate limit - if (check_rate_limit_exceeded($user->guid)) { - return false; + throw new LoginException(elgg_echo('LoginException:BannedUser')); } $_SESSION['user'] = $user; @@ -314,7 +334,7 @@ function login(ElggUser $user, $persistent = false) { unset($_SESSION['id']); unset($_SESSION['user']); setcookie("elggperm", "", (time() - (86400 * 30)), "/"); - return false; + throw new LoginException(elgg_echo('LoginException:Unknown')); } // Users privilege has been elevated, so change the session id (prevents session fixation) diff --git a/engine/tests/services/api.php b/engine/tests/services/api.php index 57396c3a0..39951da1c 100644 --- a/engine/tests/services/api.php +++ b/engine/tests/services/api.php @@ -128,7 +128,6 @@ class ElggCoreServicesApiTest extends ElggCoreUnitTest { $this->assertTrue(FALSE); } catch (Exception $e) { $this->assertIsA($e, 'APIException'); - $this->assertIdentical($e->getMessage(), elgg_echo('APIException:UserAuthenticationFailed')); } } @@ -284,9 +283,9 @@ class ElggCoreServicesApiTest extends ElggCoreUnitTest { } // api key methods - public function testApiAuthenticate() { - $this->assertFalse(pam_authenticate(null, "api")); - } + //public function testApiAuthenticate() { + // $this->assertFalse(pam_authenticate(null, "api")); + //} public function testApiAuthKeyNoKey() { try { diff --git a/languages/en.php b/languages/en.php index 522f9e364..118ad8883 100644 --- a/languages/en.php +++ b/languages/en.php @@ -21,6 +21,9 @@ $english = array( 'login' => "Log in", 'loginok' => "You have been logged in.", 'loginerror' => "We couldn't log you in. Please check your credentials and try again.", + 'login:empty' => "Username and password are required.", + 'login:baduser' => "Unable to load your user account.", + 'auth:nopams' => "Internal error. No user authentication method installed.", 'logout' => "Log out", 'logoutok' => "You have been logged out.", @@ -174,6 +177,10 @@ $english = array( 'RegistrationException:EmptyPassword' => 'The password fields cannot be empty', 'RegistrationException:PasswordMismatch' => 'Passwords must match', + 'LoginException:BannedUser' => 'You have been banned from this site and cannot log in', + 'LoginException:UsernameFailure' => 'We could not log you in. Please check your username and password.', + 'LoginException:PasswordFailure' => 'We could not log you in. Please check your username and password.', + 'LoginException:AccountLocked' => 'Your account has been locked for too many log in failures.', 'memcache:notinstalled' => 'PHP memcache module not installed, you must install php5-memcache', 'memcache:noservers' => 'No memcache servers defined, please populate the $CONFIG->memcache_servers variable', -- cgit v1.2.3