From 8a9917857e027159f97ea873ac88d25cd0e9328f Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Fri, 9 Feb 2018 00:08:01 -0500 Subject: [PATCH] Handle authentication exceptions in Login module - Break down large methods into smaller ones - Add more authentication exception - Add a legacy User::authenticate method --- src/Model/User.php | 128 +++++++++++++++++++++++------------ src/Module/Login.php | 157 +++++++++++++++++++++++++------------------ 2 files changed, 178 insertions(+), 107 deletions(-) diff --git a/src/Model/User.php b/src/Model/User.php index 7cf7fea2e6..f92611cb23 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -94,56 +94,36 @@ class User /** - * @brief Authenticate a user with a clear text password - * - * User info can be any of the following: - * - User DB object - * - User Id - * - User email or username or nickname - * - User array with at least the uid and the hashed password + * Authenticate a user with a clear text password * + * @brief Authenticate a user with a clear text password * @param mixed $user_info * @param string $password - * @return boolean + * @return int|boolean + * @deprecated since version 3.6 + * @see Friendica\Model\User::getIdFromPasswordAuthentication() */ public static function authenticate($user_info, $password) { - if (is_object($user_info)) { - $user = (array) $user_info; - } elseif (is_int($user_info)) { - $user = dba::selectFirst('user', ['uid', 'password', 'legacy_password'], - [ - 'uid' => $user_info, - 'blocked' => 0, - 'account_expired' => 0, - 'account_removed' => 0, - 'verified' => 1 - ] - ); - } elseif (is_string($user_info)) { - $user = dba::fetch_first('SELECT `uid`, `password`, `legacy_password` - FROM `user` - WHERE (`email` = ? OR `username` = ? OR `nickname` = ?) - AND `blocked` = 0 - AND `account_expired` = 0 - AND `account_removed` = 0 - AND `verified` = 1 - LIMIT 1', - $user_info, - $user_info, - $user_info - ); - } else { - $user = $user_info; + try { + return self::getIdFromPasswordAuthentication($user_info, $password); + } catch (Exception $ex) { + return false; } + } - if (!DBM::is_result($user) - || !isset($user['uid']) - || !isset($user['password']) - || !isset($user['legacy_password']) - ) { - throw new Exception('Not enough information to authenticate'); - } + /** + * Returns the user id associated with a successful password authentication + * + * @brief Authenticate a user with a clear text password + * @param mixed $user_info + * @param string $password + * @return int User Id if authentication is successful + * @throws Exception + */ + public static function getIdFromPasswordAuthentication($user_info, $password) + { + $user = self::getAuthenticationInfo($user_info); if ($user['legacy_password']) { if (password_verify(self::hashPasswordLegacy($password), $user['password'])) { @@ -159,7 +139,69 @@ class User return $user['uid']; } - return false; + throw new Exception(L10n::t('Login failed')); + } + + /** + * Returns authentication info from various parameters types + * + * User info can be any of the following: + * - User DB object + * - User Id + * - User email or username or nickname + * - User array with at least the uid and the hashed password + * + * @param mixed $user_info + * @return array + * @throws Exception + */ + private static function getAuthenticationInfo($user_info) + { + if (is_object($user_info) || is_array($user_info)) { + if (is_object($user_info)) { + $user = (array) $user_info; + } else { + $user = $user_info; + } + + if (!isset($user['uid']) + || !isset($user['password']) + || !isset($user['legacy_password']) + ) { + throw new Exception(L10n::t('Not enough information to authenticate')); + } + } elseif (is_int($user_info) || is_string($user_info)) { + if (is_int($user_info)) { + $user = dba::selectFirst('user', ['uid', 'password', 'legacy_password'], + [ + 'uid' => $user_info, + 'blocked' => 0, + 'account_expired' => 0, + 'account_removed' => 0, + 'verified' => 1 + ] + ); + } else { + $user = dba::fetch_first('SELECT `uid`, `password`, `legacy_password` + FROM `user` + WHERE (`email` = ? OR `username` = ? OR `nickname` = ?) + AND `blocked` = 0 + AND `account_expired` = 0 + AND `account_removed` = 0 + AND `verified` = 1 + LIMIT 1', + $user_info, + $user_info, + $user_info + ); + } + + if (!DBM::is_result($user)) { + throw new Exception(L10n::t('User not found')); + } + } + + return $user; } /** diff --git a/src/Module/Login.php b/src/Module/Login.php index b0b5a4947d..fe3305a91b 100644 --- a/src/Module/Login.php +++ b/src/Module/Login.php @@ -51,87 +51,116 @@ class Login extends BaseModule session_unset(); // OpenId Login if ( - !x($_POST, 'password') + empty($_POST['password']) && ( - x($_POST, 'openid_url') - || x($_POST, 'username') + !empty($_POST['openid_url']) + || !empty($_POST['username']) ) ) { - $noid = Config::get('system', 'no_openid'); + $openid_url = trim(defaults($_POST, 'openid_url', $_POST['username'])); - $openid_url = trim($_POST['openid_url'] ? : $_POST['username']); - - // if it's an email address or doesn't resolve to a URL, fail. - if ($noid || strpos($openid_url, '@') || !Network::isUrlValid($openid_url)) { - notice(L10n::t('Login failed.') . EOL); - goaway(self::getApp()->get_baseurl()); - // NOTREACHED - } - - // Otherwise it's probably an openid. - try { - $openid = new LightOpenID; - $openid->identity = $openid_url; - $_SESSION['openid'] = $openid_url; - $_SESSION['remember'] = $_POST['remember']; - $openid->returnUrl = self::getApp()->get_baseurl(true) . '/openid'; - goaway($openid->authUrl()); - } catch (Exception $e) { - notice(L10n::t('We encountered a problem while logging in with the OpenID you provided. Please check the correct spelling of the ID.') . '

' . L10n::t('The error message was:') . ' ' . $e->getMessage()); - } - // NOTREACHED + self::openIdAuthentication($openid_url, !empty($_POST['remember'])); } if (x($_POST, 'auth-params') && $_POST['auth-params'] === 'login') { - $record = null; + self::passwordAuthentication( + trim($_POST['username']), + trim($_POST['password']), + !empty($_POST['remember']) + ); + } + } - $addon_auth = [ - 'username' => trim($_POST['username']), - 'password' => trim($_POST['password']), - 'authenticated' => 0, - 'user_record' => null - ]; + /** + * Attempts to authenticate using OpenId + * + * @param string $openid_url OpenID URL string + * @param bool $remember Whether to set the session remember flag + */ + private static function openIdAuthentication($openid_url, $remember) + { + $noid = Config::get('system', 'no_openid'); - /* - * An addon indicates successful login by setting 'authenticated' to non-zero value and returning a user record - * Addons should never set 'authenticated' except to indicate success - as hooks may be chained - * and later addons should not interfere with an earlier one that succeeded. - */ - Addon::callHooks('authenticate', $addon_auth); + // if it's an email address or doesn't resolve to a URL, fail. + if ($noid || strpos($openid_url, '@') || !Network::isUrlValid($openid_url)) { + notice(L10n::t('Login failed.') . EOL); + goaway(self::getApp()->get_baseurl()); + // NOTREACHED + } - if ($addon_auth['authenticated'] && count($addon_auth['user_record'])) { - $record = $addon_auth['user_record']; - } else { - $user_id = User::authenticate(trim($_POST['username']), trim($_POST['password'])); - if ($user_id) { - $record = dba::selectFirst('user', [], ['uid' => $user_id]); - } - } + // Otherwise it's probably an openid. + try { + $openid = new LightOpenID; + $openid->identity = $openid_url; + $_SESSION['openid'] = $openid_url; + $_SESSION['remember'] = $remember; + $openid->returnUrl = self::getApp()->get_baseurl(true) . '/openid'; + goaway($openid->authUrl()); + } catch (Exception $e) { + notice(L10n::t('We encountered a problem while logging in with the OpenID you provided. Please check the correct spelling of the ID.') . '

' . L10n::t('The error message was:') . ' ' . $e->getMessage()); + } + } - if (!$record || !count($record)) { - logger('authenticate: failed login attempt: ' . notags(trim($_POST['username'])) . ' from IP ' . $_SERVER['REMOTE_ADDR']); - notice(L10n::t('Login failed.') . EOL); - goaway(self::getApp()->get_baseurl()); - } + /** + * Attempts to authenticate using login/password + * + * @param string $username User name + * @param string $password Clear password + * @param bool $remember Whether to set the session remember flag + */ + private static function passwordAuthentication($username, $password, $remember) + { + $record = null; + + $addon_auth = [ + 'username' => $username, + 'password' => $password, + 'authenticated' => 0, + 'user_record' => null + ]; + + /* + * An addon indicates successful login by setting 'authenticated' to non-zero value and returning a user record + * Addons should never set 'authenticated' except to indicate success - as hooks may be chained + * and later addons should not interfere with an earlier one that succeeded. + */ + Addon::callHooks('authenticate', $addon_auth); + + try { + if ($addon_auth['authenticated']) { + $record = $addon_auth['user_record']; - if (!$_POST['remember']) { - new_cookie(0); // 0 means delete on browser exit + if (empty($record)) { + throw new Exception(L10n::t('Login failed.')); + } + } else { + $record = dba::selectFirst('user', [], + ['uid' => User::getIdFromPasswordAuthentication($username, $password)] + ); } + } catch (Exception $e) { + logger('authenticate: failed login attempt: ' . notags($username) . ' from IP ' . $_SERVER['REMOTE_ADDR']); + notice($e->getMessage() . EOL); + goaway(self::getApp()->get_baseurl() . '/login'); + } - // if we haven't failed up this point, log them in. - $_SESSION['remember'] = $_POST['remember']; - $_SESSION['last_login_date'] = DateTimeFormat::utcNow(); - authenticate_success($record, true, true); + if (!$remember) { + new_cookie(0); // 0 means delete on browser exit + } - if (x($_SESSION, 'return_url')) { - $return_url = $_SESSION['return_url']; - unset($_SESSION['return_url']); - } else { - $return_url = ''; - } + // if we haven't failed up this point, log them in. + $_SESSION['remember'] = $remember; + $_SESSION['last_login_date'] = DateTimeFormat::utcNow(); + authenticate_success($record, true, true); - goaway($return_url); + if (x($_SESSION, 'return_url')) { + $return_url = $_SESSION['return_url']; + unset($_SESSION['return_url']); + } else { + $return_url = ''; } + + goaway($return_url); } /** -- 2.39.5