From dc350b5463e7d64a46d7f90143c2d001be99e280 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 14:15:25 -0800 Subject: [PATCH] Work in progress on nickname validation changes. lib/nickname.php appears to have been destroyed by NetBeans and will be rewritten shortly. Sigh. --- actions/apigroupcreate.php | 33 +-------- actions/editgroup.php | 15 +--- actions/newgroup.php | 19 ++---- actions/profilesettings.php | 16 ++--- actions/register.php | 11 ++- lib/common.php | 11 +++ lib/util.php | 44 ++++++++---- plugins/Facebook/FBConnectAuth.php | 15 ++-- plugins/Mapstraction/MapstractionPlugin.php | 4 +- plugins/Mapstraction/map.php | 2 +- plugins/OpenID/finishopenidlogin.php | 15 ++-- .../TwitterBridge/twitterauthorization.php | 14 ++-- tests/NicknameTest.php | 68 +++++++++++++++---- 13 files changed, 139 insertions(+), 128 deletions(-) diff --git a/actions/apigroupcreate.php b/actions/apigroupcreate.php index 54875a7188..d01504bc80 100644 --- a/actions/apigroupcreate.php +++ b/actions/apigroupcreate.php @@ -73,7 +73,7 @@ class ApiGroupCreateAction extends ApiAuthAction $this->user = $this->auth_user; - $this->nickname = $this->arg('nickname'); + $this->nickname = Nickname::normalize($this->arg('nickname')); $this->fullname = $this->arg('full_name'); $this->homepage = $this->arg('homepage'); $this->description = $this->arg('description'); @@ -150,26 +150,7 @@ class ApiGroupCreateAction extends ApiAuthAction */ function validateParams() { - $valid = Validate::string( - $this->nickname, array( - 'min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT - ) - ); - - if (!$valid) { - $this->clientError( - // TRANS: Validation error in form for group creation. - _( - 'Nickname must have only lowercase letters ' . - 'and numbers and no spaces.' - ), - 403, - $this->format - ); - return false; - } elseif ($this->groupNicknameExists($this->nickname)) { + if ($this->groupNicknameExists($this->nickname)) { $this->clientError( // TRANS: Client error trying to create a group with a nickname this is already in use. _('Nickname already in use. Try another one.'), @@ -265,15 +246,7 @@ class ApiGroupCreateAction extends ApiAuthAction foreach ($this->aliases as $alias) { - $valid = Validate::string( - $alias, array( - 'min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT - ) - ); - - if (!$valid) { + if (!Nickname::isValid($alias)) { $this->clientError( // TRANS: Client error shown when providing an invalid alias during group creation. // TRANS: %s is the invalid alias. diff --git a/actions/editgroup.php b/actions/editgroup.php index 4d3af34c7b..ab4dbb2836 100644 --- a/actions/editgroup.php +++ b/actions/editgroup.php @@ -177,21 +177,14 @@ class EditgroupAction extends GroupDesignAction return; } - $nickname = common_canonical_nickname($this->trimmed('nickname')); + $nickname = Nickname::normalize($this->trimmed('nickname')); $fullname = $this->trimmed('fullname'); $homepage = $this->trimmed('homepage'); $description = $this->trimmed('description'); $location = $this->trimmed('location'); $aliasstring = $this->trimmed('aliases'); - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: Group edit form validation error. - $this->showForm(_('Nickname must have only lowercase letters '. - 'and numbers and no spaces.')); - return; - } else if ($this->nicknameExists($nickname)) { + if ($this->nicknameExists($nickname)) { // TRANS: Group edit form validation error. $this->showForm(_('Nickname already in use. Try another one.')); return; @@ -241,9 +234,7 @@ class EditgroupAction extends GroupDesignAction } foreach ($aliases as $alias) { - if (!Validate::string($alias, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($alias)) { // TRANS: Group edit form validation error. $this->showForm(sprintf(_('Invalid alias: "%s"'), $alias)); return; diff --git a/actions/newgroup.php b/actions/newgroup.php index e0e7978c32..95af6415e5 100644 --- a/actions/newgroup.php +++ b/actions/newgroup.php @@ -113,21 +113,18 @@ class NewgroupAction extends Action function trySave() { - $nickname = $this->trimmed('nickname'); + try { + $nickname = Nickname::normalize($this->trimmed('nickname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); + } $fullname = $this->trimmed('fullname'); $homepage = $this->trimmed('homepage'); $description = $this->trimmed('description'); $location = $this->trimmed('location'); $aliasstring = $this->trimmed('aliases'); - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: Group create form validation error. - $this->showForm(_('Nickname must have only lowercase letters '. - 'and numbers and no spaces.')); - return; - } else if ($this->nicknameExists($nickname)) { + if ($this->nicknameExists($nickname)) { // TRANS: Group create form validation error. $this->showForm(_('Nickname already in use. Try another one.')); return; @@ -177,9 +174,7 @@ class NewgroupAction extends Action } foreach ($aliases as $alias) { - if (!Validate::string($alias, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($alias)) { // TRANS: Group create form validation error. $this->showForm(sprintf(_('Invalid alias: "%s"'), $alias)); return; diff --git a/actions/profilesettings.php b/actions/profilesettings.php index e1a0f8b6d0..28b1d20f34 100644 --- a/actions/profilesettings.php +++ b/actions/profilesettings.php @@ -225,7 +225,13 @@ class ProfilesettingsAction extends AccountSettingsAction if (Event::handle('StartProfileSaveForm', array($this))) { - $nickname = $this->trimmed('nickname'); + try { + $nickname = Nickname::normalize($this->trimmed('nickname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); + return; + } + $fullname = $this->trimmed('fullname'); $homepage = $this->trimmed('homepage'); $bio = $this->trimmed('bio'); @@ -236,13 +242,7 @@ class ProfilesettingsAction extends AccountSettingsAction $tagstring = $this->trimmed('tags'); // Some validation - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: Validation error in form for profile settings. - $this->showForm(_('Nickname must have only lowercase letters and numbers and no spaces.')); - return; - } else if (!User::allowed_nickname($nickname)) { + if (!User::allowed_nickname($nickname)) { // TRANS: Validation error in form for profile settings. $this->showForm(_('Not a valid nickname.')); return; diff --git a/actions/register.php b/actions/register.php index 3ae3f56f60..5d91aef70e 100644 --- a/actions/register.php +++ b/actions/register.php @@ -198,7 +198,11 @@ class RegisterAction extends Action } // Input scrubbing - $nickname = common_canonical_nickname($nickname); + try { + $nickname = Nickname::normalize($nickname); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); + } $email = common_canonical_email($email); if (!$this->boolean('license')) { @@ -206,11 +210,6 @@ class RegisterAction extends Action 'agree to the license.')); } else if ($email && !Validate::email($email, common_config('email', 'check_domain'))) { $this->showForm(_('Not a valid email address.')); - } else if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - $this->showForm(_('Nickname must have only lowercase letters '. - 'and numbers and no spaces.')); } else if ($this->nicknameExists($nickname)) { $this->showForm(_('Nickname already in use. Try another one.')); } else if (!User::allowed_nickname($nickname)) { diff --git a/lib/common.php b/lib/common.php index cd4fbfb15a..d891807185 100644 --- a/lib/common.php +++ b/lib/common.php @@ -117,6 +117,17 @@ require_once 'markdown.php'; // XXX: other formats here +/** + * Avoid the NICKNAME_FMT constant; use the Nickname class instead. + * + * Nickname::DISPLAY_FMT is more suitable for inserting into regexes; + * note that it includes the [] and repeating bits, so should be wrapped + * directly in a capture paren usually. + * + * For validation, use Nickname::validate() etc. + * + * @deprecated + */ define('NICKNAME_FMT', VALIDATE_NUM.VALIDATE_ALPHA_LOWER); require_once INSTALLDIR.'/lib/util.php'; diff --git a/lib/util.php b/lib/util.php index f5a4dc87b8..c170c8380f 100644 --- a/lib/util.php +++ b/lib/util.php @@ -520,18 +520,15 @@ function common_user_cache_hash($user=false) /** * get canonical version of nickname for comparison * - * Currently this just runs strtolower(); more is needed. - * - * @fixme normalize punctuation chars where applicable - * @fixme reject invalid input - * * @param string $nickname * @return string + * + * @throws NicknameException on invalid input + * @deprecated call Nickname::normalize() directly. */ function common_canonical_nickname($nickname) { - // XXX: UTF-8 canonicalization (like combining chars) - return strtolower($nickname); + return Nickname::normalize($nickname); } /** @@ -553,8 +550,6 @@ function common_canonical_email($email) /** * Partial notice markup rendering step: build links to !group references. * - * @fixme use abstracted group nickname regex - * * @param string $text partially rendered HTML * @param Notice $notice in whose context we're working * @return string partially rendered HTML @@ -564,7 +559,8 @@ function common_render_content($text, $notice) $r = common_render_text($text); $id = $notice->profile_id; $r = common_linkify_mentions($r, $notice); - $r = preg_replace('/(^|[\s\.\,\:\;]+)!([A-Za-z0-9]{1,64})/e', "'\\1!'.common_group_link($id, '\\2')", $r); + $r = preg_replace('/(^|[\s\.\,\:\;]+)!(' . Nickname::DISPLAY_FMT . ')/e', + "'\\1!'.common_group_link($id, '\\2')", $r); return $r; } @@ -625,11 +621,19 @@ function common_linkify_mention($mention) } /** - * @fixme use NICKNAME_FMT more consistently + * Find @-mentions in the given text, using the given notice object as context. + * References will be resolved with common_relative_profile() against the user + * who posted the notice. + * + * Note the return data format is internal, to be used for building links and + * such. Should not be used directly; rather, call common_linkify_mentions(). * * @param string $text * @param Notice $notice notice in whose context we're building links + * * @return array + * + * @access private */ function common_find_mentions($text, $notice) { @@ -665,12 +669,12 @@ function common_find_mentions($text, $notice) } } - preg_match_all('/^T ([A-Z0-9]{1,64}) /', + preg_match_all('/^T (' . Nickname::DISPLAY_FMT . ') /', $text, $tmatches, PREG_OFFSET_CAPTURE); - preg_match_all('/(?:^|\s+)@(['.NICKNAME_FMT.']{1,64})/', + preg_match_all('/(?:^|\s+)@(' . Nickname::DISPLAY_FMT . ')\b/', $text, $atmatches, PREG_OFFSET_CAPTURE); @@ -678,7 +682,12 @@ function common_find_mentions($text, $notice) $matches = array_merge($tmatches[1], $atmatches[1]); foreach ($matches as $match) { - $nickname = common_canonical_nickname($match[0]); + try { + $nickname = Nickname::normalize($match[0]); + } catch (NicknameException $e) { + // Bogus match? Drop it. + continue; + } // Try to get a profile for this nickname. // Start with conversation context, then go to @@ -1038,6 +1047,13 @@ function common_valid_profile_tag($str) return preg_match('/^[A-Za-z0-9_\-\.]{1,64}$/', $str); } +/** + * + * @param $sender_id + * @param $nickname + * @return + * @access private + */ function common_group_link($sender_id, $nickname) { $sender = Profile::staticGet($sender_id); diff --git a/plugins/Facebook/FBConnectAuth.php b/plugins/Facebook/FBConnectAuth.php index d6d3786261..84d51578f1 100644 --- a/plugins/Facebook/FBConnectAuth.php +++ b/plugins/Facebook/FBConnectAuth.php @@ -257,13 +257,10 @@ class FBConnectauthAction extends Action } } - $nickname = $this->trimmed('newname'); - - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.')); - return; + try { + $nickname = Nickname::normalize($this->trimmed('newname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); } if (!User::allowed_nickname($nickname)) { @@ -447,9 +444,7 @@ class FBConnectauthAction extends Action function isNewNickname($str) { - if (!Validate::string($str, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($str)) { return false; } if (!User::allowed_nickname($str)) { diff --git a/plugins/Mapstraction/MapstractionPlugin.php b/plugins/Mapstraction/MapstractionPlugin.php index d5261d8bc7..5ad25763e7 100644 --- a/plugins/Mapstraction/MapstractionPlugin.php +++ b/plugins/Mapstraction/MapstractionPlugin.php @@ -67,10 +67,10 @@ class MapstractionPlugin extends Plugin { $m->connect(':nickname/all/map', array('action' => 'allmap'), - array('nickname' => '['.NICKNAME_FMT.']{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect(':nickname/map', array('action' => 'usermap'), - array('nickname' => '['.NICKNAME_FMT.']{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); return true; } diff --git a/plugins/Mapstraction/map.php b/plugins/Mapstraction/map.php index 50ff82b67e..dbba4edd0c 100644 --- a/plugins/Mapstraction/map.php +++ b/plugins/Mapstraction/map.php @@ -53,7 +53,7 @@ class MapAction extends OwnerDesignAction parent::prepare($args); $nickname_arg = $this->arg('nickname'); - $nickname = common_canonical_nickname($nickname_arg); + $nickname = Nickname::normalize($nickname_arg); // Permanent redirect on non-canonical nickname diff --git a/plugins/OpenID/finishopenidlogin.php b/plugins/OpenID/finishopenidlogin.php index 01dd61edb1..86dd1c669b 100644 --- a/plugins/OpenID/finishopenidlogin.php +++ b/plugins/OpenID/finishopenidlogin.php @@ -272,13 +272,10 @@ class FinishopenidloginAction extends Action } } - $nickname = $this->trimmed('newname'); - - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: OpenID plugin message. The entered new user name did not conform to the requirements. - $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.')); + try { + $nickname = Nickname::validate($this->trimmed('newname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); return; } @@ -463,9 +460,7 @@ class FinishopenidloginAction extends Action function isNewNickname($str) { - if (!Validate::string($str, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($str)) { return false; } if (!User::allowed_nickname($str)) { diff --git a/plugins/TwitterBridge/twitterauthorization.php b/plugins/TwitterBridge/twitterauthorization.php index 931a037230..bbe41bd438 100644 --- a/plugins/TwitterBridge/twitterauthorization.php +++ b/plugins/TwitterBridge/twitterauthorization.php @@ -441,12 +441,10 @@ class TwitterauthorizationAction extends Action } } - $nickname = $this->trimmed('newname'); - - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.')); + try { + $nickname = Nickname::normalize($this->trimmed('newname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); return; } @@ -619,9 +617,7 @@ class TwitterauthorizationAction extends Action function isNewNickname($str) { - if (!Validate::string($str, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($str)) { return false; } if (!User::allowed_nickname($str)) { diff --git a/tests/NicknameTest.php b/tests/NicknameTest.php index 95af94098d..f1d9808228 100644 --- a/tests/NicknameTest.php +++ b/tests/NicknameTest.php @@ -17,18 +17,37 @@ require_once INSTALLDIR . '/lib/common.php'; class NicknameTest extends PHPUnit_Framework_TestCase { /** - * @dataProvider provider + * Basic test using Nickname::normalize() * + * @dataProvider provider */ - public function testBasic($input, $expected) + public function testBasic($input, $expected, $expectedException=null) { - $matches = array(); - // First problem: this is all manual, wtf! - if (preg_match('/^([' . NICKNAME_FMT . ']{1,64})$/', $input, $matches)) { - $norm = common_canonical_nickname($matches[1]); - $this->assertEquals($expected, $norm, "normalized input nickname: $input -> $norm"); + $exception = null; + $normalized = false; + try { + $normalized = Nickname::normalize($normalized); + } catch (NicknameException $e) { + $exception = $e; + } + + if ($expected === false) { + if ($expectedException) { + $this->assert($exception && $exception instanceof $expectedException, + "invalid input '$input' expected to fail with $expectedException, " . + "got " . get_class($exception) . ': ' . $exception->getMessage()); + } else { + $this->assert($normalized == false, + "invalid input '$input' expected to fail"); + } } else { - $this->assertEquals($expected, false, "invalid input nickname: $input"); + $msg = "normalized input nickname '$input' expected to normalize to '$expected', got "; + if ($exception) { + $msg .= get_class($exception) . ': ' . $exception->getMessage(); + } else { + $msg .= "'$normalized'"; + } + $this->assertEquals($expected, $norm, $msg); } } @@ -36,17 +55,38 @@ class NicknameTest extends PHPUnit_Framework_TestCase { return array( array('evan', 'evan'), + + // Case and underscore variants array('Evan', 'evan'), array('EVAN', 'evan'), array('ev_an', 'evan'), - array('ev.an', 'evan'), - array('ev/an', false), - array('ev an', false), - array('ev-an', false), - array('évan', false), // so far... - array('Évan', false), // so far... + array('E__V_an', 'evan'), array('evan1', 'evan1'), array('evan_1', 'evan1'), + array('0x20', '0x20'), + array('1234', '1234'), // should this be allowed though? :) + array('12__34', '1234'), + + // Some (currently) invalid chars... + array('^#@&^#@', false, 'NicknameInvalidException'), // all invalid :D + array('ev.an', false, 'NicknameInvalidException'), + array('ev/an', false, 'NicknameInvalidException'), + array('ev an', false, 'NicknameInvalidException'), + array('ev-an', false, 'NicknameInvalidException'), + + // Non-ASCII letters; currently not allowed, in future + // we'll add them at least with conversion to ASCII. + // Not much use until we have storage of display names, + // though. + array('évan', false, 'NicknameInvalidException'), // so far... + array('Évan', false, 'NicknameInvalidException'), // so far... + + // Length checks + array('', false, 'NicknameEmptyException'), + array('___', false, 'NicknameEmptyException'), + array('eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee'), // 64 chars + array('eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_', 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee'), // the _ will be trimmed off, remaining valid + array('eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', false, 'NicknameTooLongException'), // 65 chars -- too long ); } }