From db7ef52d1352fa3fa49ddf85dcd2bb124c00d303 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Wed, 16 Oct 2013 14:58:22 +0200 Subject: [PATCH] Better use of Nickname validation functions Nickname verifications on registration and updates for profiles (not yet groups) have been improved. Minor bugs in RegisterAction were also fixed, where multiple forms would be outputed because the function did not return after showForm(). This will be solved more permanently with throwing exceptions in the future. --- actions/apiaccountregister.php | 27 +------- actions/apichecknickname.php | 4 +- actions/profilesettings.php | 31 +++------ actions/register.php | 32 +-------- lib/installer.php | 2 +- lib/nickname.php | 25 +++---- lib/util.php | 9 ++- .../actions/facebookfinishlogin.php | 69 +++---------------- plugins/OpenID/actions/finishopenidlogin.php | 46 ++----------- .../actions/twitterauthorization.php | 48 ++----------- 10 files changed, 58 insertions(+), 235 deletions(-) diff --git a/actions/apiaccountregister.php b/actions/apiaccountregister.php index 9ce7bcfb68..be66710d80 100644 --- a/actions/apiaccountregister.php +++ b/actions/apiaccountregister.php @@ -114,22 +114,17 @@ class ApiAccountRegisterAction extends ApiAction // Input scrubbing try { - $nickname = Nickname::normalize($nickname); + $nickname = Nickname::normalize($nickname, true); } catch (NicknameException $e) { // clientError handles Api exceptions with various formats and stuff - $this->clientError(_('Not a valid nickname.'), 400); + $this->clientError($e->getMessage(), $e->getCode()); } + $email = common_canonical_email($email); if ($email && !Validate::email($email, common_config('email', 'check_domain'))) { // TRANS: Form validation error displayed when trying to register without a valid e-mail address. $this->clientError(_('Not a valid email address.'), 400); - } else if ($this->nicknameExists($nickname)) { - // TRANS: Form validation error displayed when trying to register with an existing nickname. - $this->clientError(_('Nickname already in use. Try another one.'), 400); - } else if (!User::allowed_nickname($nickname)) { - // TRANS: Form validation error displayed when trying to register with an invalid nickname. - $this->clientError(_('Not a valid nickname.'), 400); } else if ($this->emailExists($email)) { // TRANS: Form validation error displayed when trying to register with an already registered e-mail address. $this->clientError(_('Email address already exists.'), 400); @@ -186,22 +181,6 @@ class ApiAccountRegisterAction extends ApiAction } } } - - - /** - * Does the given nickname already exist? - * - * Checks a canonical nickname against the database. - * - * @param string $nickname nickname to check - * - * @return boolean true if the nickname already exists - */ - function nicknameExists($nickname) - { - $user = User::staticGet('nickname', $nickname); - return is_object($user); - } /** * Does the given email address already exist? diff --git a/actions/apichecknickname.php b/actions/apichecknickname.php index 4f80a7f29e..6867a428cf 100644 --- a/actions/apichecknickname.php +++ b/actions/apichecknickname.php @@ -52,8 +52,8 @@ class ApiCheckNicknameAction extends ApiAction $nickname = $this->trimmed('nickname'); try { - Nickname::normalize($nickname); - $nickname_ok = $this->nicknameExists($nickname) ? 0 : 1; + Nickname::normalize($nickname, true); + $nickname_ok = 1; } catch (NicknameException $e) { $nickname_ok = 0; } diff --git a/actions/profilesettings.php b/actions/profilesettings.php index ef62eb9c8f..b2c9a3c014 100644 --- a/actions/profilesettings.php +++ b/actions/profilesettings.php @@ -239,8 +239,16 @@ class ProfilesettingsAction extends SettingsAction if (Event::handle('StartProfileSaveForm', array($this))) { + $nickname = $this->trimmed('nickname'); try { - $nickname = Nickname::normalize($this->trimmed('nickname')); + $nickname = Nickname::normalize($nickname, true); + } catch (NicknameTakenException $e) { + // Abort only if the nickname is occupied by another user + if ($e->byuser->id != $this->scoped->id) { + $this->showForm($e->getMessage()); + return; + } + $nickname = Nickname::normalize($nickname); // without in-use check this time } catch (NicknameException $e) { $this->showForm($e->getMessage()); return; @@ -258,11 +266,7 @@ class ProfilesettingsAction extends SettingsAction $tagstring = $this->trimmed('tags'); // Some validation - if (!User::allowed_nickname($nickname)) { - // TRANS: Validation error in form for profile settings. - $this->showForm(_('Not a valid nickname.')); - return; - } else if (!is_null($homepage) && (strlen($homepage) > 0) && + if (!is_null($homepage) && (strlen($homepage) > 0) && !common_valid_http_url($homepage)) { // TRANS: Validation error in form for profile settings. $this->showForm(_('Homepage is not a valid URL.')); @@ -288,10 +292,6 @@ class ProfilesettingsAction extends SettingsAction // TRANS: Validation error in form for profile settings. $this->showForm(_('Timezone not selected.')); return; - } else if ($this->nicknameExists($nickname)) { - // TRANS: Validation error in form for profile settings. - $this->showForm(_('Nickname already in use. Try another one.')); - return; } else if (!is_null($language) && strlen($language) > 50) { // TRANS: Validation error in form for profile settings. $this->showForm(_('Language is too long (maximum 50 characters).')); @@ -468,17 +468,6 @@ class ProfilesettingsAction extends SettingsAction } } - function nicknameExists($nickname) - { - $user = common_current_user(); - $other = User::getKV('nickname', $nickname); - if (!$other) { - return false; - } else { - return $other->id != $user->id; - } - } - function showAside() { $user = common_current_user(); diff --git a/actions/register.php b/actions/register.php index 661936d5af..7c372892df 100644 --- a/actions/register.php +++ b/actions/register.php @@ -192,9 +192,10 @@ class RegisterAction extends Action // Input scrubbing try { - $nickname = Nickname::normalize($nickname); + $nickname = Nickname::normalize($nickname, true); } catch (NicknameException $e) { $this->showForm($e->getMessage()); + return; } $email = common_canonical_email($email); @@ -205,12 +206,6 @@ class RegisterAction extends Action } else if ($email && !Validate::email($email, common_config('email', 'check_domain'))) { // TRANS: Form validation error displayed when trying to register without a valid e-mail address. $this->showForm(_('Not a valid email address.')); - } else if ($this->nicknameExists($nickname)) { - // TRANS: Form validation error displayed when trying to register with an existing nickname. - $this->showForm(_('Nickname already in use. Try another one.')); - } else if (!User::allowed_nickname($nickname)) { - // TRANS: Form validation error displayed when trying to register with an invalid nickname. - $this->showForm(_('Not a valid nickname.')); } else if ($this->emailExists($email)) { // TRANS: Form validation error displayed when trying to register with an already registered e-mail address. $this->showForm(_('Email address already exists.')); @@ -218,11 +213,9 @@ class RegisterAction extends Action !common_valid_http_url($homepage)) { // TRANS: Form validation error displayed when trying to register with an invalid homepage URL. $this->showForm(_('Homepage is not a valid URL.')); - return; } else if (!is_null($fullname) && mb_strlen($fullname) > 255) { // TRANS: Form validation error displayed when trying to register with a too long full name. $this->showForm(_('Full name is too long (maximum 255 characters).')); - return; } else if (Profile::bioTooLong($bio)) { // TRANS: Form validation error on registration page when providing too long a bio text. // TRANS: %d is the maximum number of characters for bio; used for plural. @@ -230,15 +223,12 @@ class RegisterAction extends Action 'Bio is too long (maximum %d characters).', Profile::maxBio()), Profile::maxBio())); - return; } else if (!is_null($location) && mb_strlen($location) > 255) { // TRANS: Form validation error displayed when trying to register with a too long location. $this->showForm(_('Location is too long (maximum 255 characters).')); - return; } else if (strlen($password) < 6) { // TRANS: Form validation error displayed when trying to register with too short a password. $this->showForm(_('Password must be 6 or more characters.')); - return; } else if ($password != $confirm) { // TRANS: Form validation error displayed when trying to register with non-matching passwords. $this->showForm(_('Passwords do not match.')); @@ -250,7 +240,7 @@ class RegisterAction extends Action 'bio' => $bio, 'location' => $location, 'code' => $code))) { - if (!$user) { + if (!($user instanceof User)) { // TRANS: Form validation error displayed when trying to register with an invalid username or password. $this->showForm(_('Invalid username or password.')); return; @@ -259,7 +249,6 @@ class RegisterAction extends Action if (!common_set_user($user)) { // TRANS: Server error displayed when saving fails during user registration. $this->serverError(_('Error setting user.')); - return; } // this is a real login common_real_login(true); @@ -281,21 +270,6 @@ class RegisterAction extends Action } } - /** - * Does the given nickname already exist? - * - * Checks a canonical nickname against the database. - * - * @param string $nickname nickname to check - * - * @return boolean true if the nickname already exists - */ - function nicknameExists($nickname) - { - $user = User::getKV('nickname', $nickname); - return is_object($user); - } - /** * Does the given email address already exist? * diff --git a/lib/installer.php b/lib/installer.php index e313576429..bf30d00786 100644 --- a/lib/installer.php +++ b/lib/installer.php @@ -235,7 +235,7 @@ abstract class Installer '" is invalid; should be plain letters and numbers no longer than 64 characters.', true); $fail = true; } - // @fixme hardcoded list; should use User::allowed_nickname() + // @fixme hardcoded list; should use Nickname::isValid() // if/when it's safe to have loaded the infrastructure here $blacklist = array('main', 'panel', 'twitter', 'settings', 'rsd.xml', 'favorited', 'featured', 'favoritedrss', 'featuredrss', 'rss', 'getfile', 'api', 'groups', 'group', 'peopletag', 'tag', 'user', 'message', 'conversation', 'bookmarklet', 'notice', 'attachment', 'search', 'index.php', 'doc', 'opensearch', 'robots.txt', 'xd_receiver.html', 'facebook'); if (in_array($this->adminNick, $blacklist)) { diff --git a/lib/nickname.php b/lib/nickname.php index 2b8249d59a..2792d32fd5 100644 --- a/lib/nickname.php +++ b/lib/nickname.php @@ -110,30 +110,25 @@ class Nickname */ public static function normalize($str, $checkuse=false) { - if (mb_strlen($str) > self::MAX_LEN) { - // Display forms must also fit! - throw new NicknameTooLongException(); - } - + // We should also have UTF-8 normalization (å to a etc.) $str = trim($str); $str = str_replace('_', '', $str); $str = mb_strtolower($str); - if (mb_strlen($str) < 1) { + if (mb_strlen($str) > self::MAX_LEN) { + // Display forms must also fit! + throw new NicknameTooLongException(); + } elseif (mb_strlen($str) < 1) { throw new NicknameEmptyException(); - } - if (!self::isCanonical($str)) { + } elseif (!self::isCanonical($str)) { throw new NicknameInvalidException(); - } - if (self::isBlacklisted($str)) { + } elseif (self::isBlacklisted($str)) { throw new NicknameBlacklistedException(); - } - if (self::isSystemPath($str)) { + } elseif (self::isSystemPath($str)) { throw new NicknamePathCollisionException(); - } - if ($checkuse && $user = self::isTaken($str)) { + } elseif ($checkuse && $user = self::isTaken($str)) { if ($user instanceof User) { - throw new NicknameTakenException(); + throw new NicknameTakenException($user); } } diff --git a/lib/util.php b/lib/util.php index 8c6ff6718a..81643f9a19 100644 --- a/lib/util.php +++ b/lib/util.php @@ -237,7 +237,7 @@ function common_check_user($nickname, $password) if (common_is_email($nickname)) { $user = User::getKV('email', common_canonical_email($nickname)); } else { - $user = User::getKV('nickname', common_canonical_nickname($nickname)); + $user = User::getKV('nickname', Nickname::normalize($nickname)); } if (!empty($user)) { @@ -2283,8 +2283,11 @@ function common_url_to_nickname($url) function common_nicknamize($str) { - $str = preg_replace('/\W/', '', $str); - return strtolower($str); + try { + return Nickname::normalize($str); + } catch (NicknameException $e) { + return null; + } } function common_perf_counter($key, $val=null) diff --git a/plugins/FacebookBridge/actions/facebookfinishlogin.php b/plugins/FacebookBridge/actions/facebookfinishlogin.php index 2abbf9c905..483460baad 100644 --- a/plugins/FacebookBridge/actions/facebookfinishlogin.php +++ b/plugins/FacebookBridge/actions/facebookfinishlogin.php @@ -355,24 +355,12 @@ class FacebookfinishloginAction extends Action } try { - $nickname = Nickname::normalize($this->trimmed('newname')); + $nickname = Nickname::normalize($this->trimmed('newname'), true); } catch (NicknameException $e) { $this->showForm($e->getMessage()); return; } - if (!User::allowed_nickname($nickname)) { - // TRANS: Form validation error displayed when picking a nickname that is not allowed. - $this->showForm(_m('Nickname not allowed.')); - return; - } - - if (User::getKV('nickname', $nickname)) { - // TRANS: Form validation error displayed when picking a nickname that is already in use. - $this->showForm(_m('Nickname already in use. Try another one.')); - return; - } - $args = array( 'nickname' => $nickname, 'fullname' => $this->fbuser->name, @@ -603,58 +591,23 @@ class FacebookfinishloginAction extends Action function bestNewNickname() { - if (!empty($this->fbuser->username)) { - $nickname = $this->nicknamize($this->fbuser->username); - if ($this->isNewNickname($nickname)) { - return $nickname; - } + try { + $nickname = Nickname::normalize($this->fbuser->username, true); + return $nickname; + } catch (NicknameException $e) { + // Failed to normalize nickname, but let's try the full name } - // Try the full name - - $fullname = $this->fbuser->name; - - if (!empty($fullname)) { - $fullname = $this->nicknamize($fullname); - if ($this->isNewNickname($fullname)) { - return $fullname; - } + try { + $nickname = Nickname::normalize($this->fbuser->name, true); + return $nickname; + } catch (NicknameException $e) { + // Any more ideas? Nope. } return null; } - /** - * Given a string, try to make it work as a nickname - */ - function nicknamize($str) - { - $str = preg_replace('/\W/', '', $str); - return strtolower($str); - } - - /* - * Is the desired nickname already taken? - * - * @return boolean result - */ - function isNewNickname($str) - { - if (!Nickname::isValid($str)) { - return false; - } - - if (!User::allowed_nickname($str)) { - return false; - } - - if (User::getKV('nickname', $str)) { - return false; - } - - return true; - } - /* * Do we already have a user record with this email? * (emails have to be unique but they can change) diff --git a/plugins/OpenID/actions/finishopenidlogin.php b/plugins/OpenID/actions/finishopenidlogin.php index 4326f19514..372c1e12b5 100644 --- a/plugins/OpenID/actions/finishopenidlogin.php +++ b/plugins/OpenID/actions/finishopenidlogin.php @@ -345,24 +345,12 @@ class FinishopenidloginAction extends Action } try { - $nickname = Nickname::normalize($this->trimmed('newname')); + $nickname = Nickname::normalize($this->trimmed('newname'), true); } catch (NicknameException $e) { $this->showForm($e->getMessage()); return; } - if (!User::allowed_nickname($nickname)) { - // TRANS: OpenID plugin message. The entered new user name is blacklisted. - $this->showForm(_m('Nickname not allowed.')); - return; - } - - if (User::getKV('nickname', $nickname)) { - // TRANS: OpenID plugin message. The entered new user name is already used. - $this->showForm(_m('Nickname already in use. Try another one.')); - return; - } - list($display, $canonical, $sreg) = $this->getSavedValues(); if (!$display || !$canonical) { @@ -500,8 +488,8 @@ class FinishopenidloginAction extends Action // Try the passed-in nickname if (!empty($sreg['nickname'])) { - $nickname = $this->nicknamize($sreg['nickname']); - if ($this->isNewNickname($nickname)) { + $nickname = common_nicknamize($sreg['nickname']); + if (Nickname::isValid($nickname, true)) { return $nickname; } } @@ -509,8 +497,8 @@ class FinishopenidloginAction extends Action // Try the full name if (!empty($sreg['fullname'])) { - $fullname = $this->nicknamize($sreg['fullname']); - if ($this->isNewNickname($fullname)) { + $fullname = common_nicknamize($sreg['fullname']); + if (Nickname::isValid($fullname, true)) { return $fullname; } } @@ -519,7 +507,7 @@ class FinishopenidloginAction extends Action $from_url = $this->openidToNickname($display); - if ($from_url && $this->isNewNickname($from_url)) { + if ($from_url && Nickname::isValid($from_url, true)) { return $from_url; } @@ -528,20 +516,6 @@ class FinishopenidloginAction extends Action return null; } - function isNewNickname($str) - { - if (!Nickname::isValid($str)) { - return false; - } - if (!User::allowed_nickname($str)) { - return false; - } - if (User::getKV('nickname', $str)) { - return false; - } - return true; - } - function openidToNickname($openid) { if (Auth_Yadis_identifierScheme($openid) == 'XRI') { @@ -570,7 +544,7 @@ class FinishopenidloginAction extends Action // =evan.prodromou // or @gratis*evan.prodromou $parts = explode('*', substr($base, 1)); - return $this->nicknamize(array_pop($parts)); + return common_nicknamize(array_pop($parts)); } } @@ -582,10 +556,4 @@ class FinishopenidloginAction extends Action return $xri; } } - - // Given a string, try to make it work as a nickname - function nicknamize($str) - { - return common_nicknamize($str); - } } diff --git a/plugins/TwitterBridge/actions/twitterauthorization.php b/plugins/TwitterBridge/actions/twitterauthorization.php index 6a86c81cb7..653326e1f8 100644 --- a/plugins/TwitterBridge/actions/twitterauthorization.php +++ b/plugins/TwitterBridge/actions/twitterauthorization.php @@ -519,24 +519,12 @@ class TwitterauthorizationAction extends Action } try { - $nickname = Nickname::normalize($this->trimmed('newname')); + $nickname = Nickname::normalize($this->trimmed('newname'), true); } catch (NicknameException $e) { $this->showForm($e->getMessage()); return; } - if (!User::allowed_nickname($nickname)) { - // TRANS: Client error displayed when trying to create a new user with an invalid username. - $this->showForm(_m('Nickname not allowed.')); - return; - } - - if (User::getKV('nickname', $nickname)) { - // TRANS: Client error displayed when trying to create a new user with a username that is already in use. - $this->showForm(_m('Nickname already in use. Try another one.')); - return; - } - $fullname = trim($this->tw_fields['fullname']); $args = array('nickname' => $nickname, 'fullname' => $fullname); @@ -688,36 +676,10 @@ class TwitterauthorizationAction extends Action function bestNewNickname() { - if (!empty($this->tw_fields['fullname'])) { - $nickname = $this->nicknamize($this->tw_fields['fullname']); - if ($this->isNewNickname($nickname)) { - return $nickname; - } + try { + return Nickname::normalize($this->tw_fields['fullname'], true); + } catch (NicknameException $e) { + return null } - - return null; - } - - // Given a string, try to make it work as a nickname - - function nicknamize($str) - { - $str = preg_replace('/\W/', '', $str); - $str = str_replace(array('-', '_'), '', $str); - return strtolower($str); - } - - function isNewNickname($str) - { - if (!Nickname::isValid($str)) { - return false; - } - if (!User::allowed_nickname($str)) { - return false; - } - if (User::getKV('nickname', $str)) { - return false; - } - return true; } }