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

@@ -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(
+        if ($this->groupNicknameExists($this->nickname)) {
-            $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)) {
             $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(
+            if (!Nickname::isValid($alias)) {
-                $alias, array(
-                    'min_length' => 1,
-                    'max_length' => 64,
-                    'format' => NICKNAME_FMT
-                )
-            );
-
-            if (!$valid) {
                 $this->clientError(
                     // TRANS: Client error shown when providing an invalid alias during group creation.
                     // TRANS: %s is the invalid alias.

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,
+        if ($this->nicknameExists($nickname)) {
-                                               '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)) {
             // 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,
+            if (!Nickname::isValid($alias)) {
-                                                'max_length' => 64,
-                                                'format' => NICKNAME_FMT))) {
                 // TRANS: Group edit form validation error.
                 $this->showForm(sprintf(_('Invalid alias: "%s"'), $alias));
                 return;

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,
+        if ($this->nicknameExists($nickname)) {
-                                               '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)) {
             // 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,
+            if (!Nickname::isValid($alias)) {
-                                                'max_length' => 64,
-                                                'format' => NICKNAME_FMT))) {
                 // TRANS: Group create form validation error.
                 $this->showForm(sprintf(_('Invalid alias: "%s"'), $alias));
                 return;

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,
+            if (!User::allowed_nickname($nickname)) {
-                                                   '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)) {
                 // TRANS: Validation error in form for profile settings.
                 $this->showForm(_('Not a valid nickname.'));
                 return;

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)) {

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';

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 Nickname::normalize($nickname);
-    return strtolower($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 <type> $sender_id
+ * @param <type> $nickname
+ * @return <type>
+ * @access private
+ */
 function common_group_link($sender_id, $nickname)
 {
     $sender = Profile::staticGet($sender_id);

plugins/Facebook/FBConnectAuth.php

@@ -257,13 +257,10 @@ class FBConnectauthAction extends Action
             }
         }
 
-        $nickname = $this->trimmed('newname');
+        try {
-
+            $nickname = Nickname::normalize($this->trimmed('newname'));
-        if (!Validate::string($nickname, array('min_length' => 1,
+        } catch (NicknameException $e) {
-                                               'max_length' => 64,
+            $this->showForm($e->getMessage());
-                                               'format' => NICKNAME_FMT))) {
-            $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.'));
-            return;
         }
 
         if (!User::allowed_nickname($nickname)) {
@@ -447,9 +444,7 @@ class FBConnectauthAction extends Action
 
     function isNewNickname($str)
     {
-        if (!Validate::string($str, array('min_length' => 1,
+        if (!Nickname::isValid($str)) {
-                                          'max_length' => 64,
-                                          'format' => NICKNAME_FMT))) {
             return false;
         }
         if (!User::allowed_nickname($str)) {

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;
     }
 

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
 

plugins/OpenID/finishopenidlogin.php

@@ -272,13 +272,10 @@ class FinishopenidloginAction extends Action
             }
         }
 
-        $nickname = $this->trimmed('newname');
+        try {
-
+            $nickname = Nickname::validate($this->trimmed('newname'));
-        if (!Validate::string($nickname, array('min_length' => 1,
+        } catch (NicknameException $e) {
-                                               'max_length' => 64,
+            $this->showForm($e->getMessage());
-                                               '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.'));
             return;
         }
 
@@ -463,9 +460,7 @@ class FinishopenidloginAction extends Action
 
     function isNewNickname($str)
     {
-        if (!Validate::string($str, array('min_length' => 1,
+        if (!Nickname::isValid($str)) {
-                                          'max_length' => 64,
-                                          'format' => NICKNAME_FMT))) {
             return false;
         }
         if (!User::allowed_nickname($str)) {

plugins/TwitterBridge/twitterauthorization.php

@@ -441,12 +441,10 @@ class TwitterauthorizationAction extends Action
             }
         }
 
-        $nickname = $this->trimmed('newname');
+        try {
-
+            $nickname = Nickname::normalize($this->trimmed('newname'));
-        if (!Validate::string($nickname, array('min_length' => 1,
+        } catch (NicknameException $e) {
-                                               'max_length' => 64,
+            $this->showForm($e->getMessage());
-                                               'format' => NICKNAME_FMT))) {
-            $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.'));
             return;
         }
 
@@ -619,9 +617,7 @@ class TwitterauthorizationAction extends Action
 
     function isNewNickname($str)
     {
-        if (!Validate::string($str, array('min_length' => 1,
+        if (!Nickname::isValid($str)) {
-                                          'max_length' => 64,
-                                          'format' => NICKNAME_FMT))) {
             return false;
         }
         if (!User::allowed_nickname($str)) {

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();
+        $exception = null;
-        // First problem: this is all manual, wtf!
+        $normalized = false;
-        if (preg_match('/^([' . NICKNAME_FMT . ']{1,64})$/', $input, $matches)) {
+        try {
-            $norm = common_canonical_nickname($matches[1]);
+            $normalized = Nickname::normalize($normalized);
-            $this->assertEquals($expected, $norm, "normalized input nickname: $input -> $norm");
+        } 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('E__V_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('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
                      );
     }
 }