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

8 files changed, 198 insertions, 91 deletions

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 @@
+<?php
+/**
+ * ElggPAM Pluggable Authentication Module
+ *
+ * @package    Elgg.Core
+ * @subpackage Authentication
+ */
+class ElggPAM {
+	/**
+	 * @var string PAM policy type: user, api or plugin-defined policies
+	 */
+	protected $policy;
+
+	/**
+	 * @var array Failure mesages
+	 */
+	protected $messages;
+
+	/**
+	 * ElggPAM constructor
+	 * 
+	 * @param string $policy PAM policy type: user, api, or plugin-defined policies
+	 */
+	public function __construct($policy) {
+		$this->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 @@
+<?php
+/**
+ * Login Exception Stub
+ *
+ * Generic parent class for login exceptions.
+ *
+ * @package    Elgg.Core
+ * @subpackage Exceptions.Stub
+ */
+class LoginException extends Exception {}
\ No newline at end of file
diff --git a/engine/lib/api.php b/engine/lib/api.php
index 7313fb5e9..2c566b479 100644
--- a/engine/lib/api.php
+++ b/engine/lib/api.php
@@ -170,17 +170,19 @@ function authenticate_method($method) {
 
 	// check API authentication if required
 	if ($API_METHODS[$method]["require_api_auth"] == true) {
-		if (pam_authenticate(null, "api") == false) {
+		$api_pam = new ElggPAM('api');
+		if ($api_pam->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
@@ -128,6 +128,26 @@ function elgg_is_admin_user($user_guid) {
 }
 
 /**
+ * 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',