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

@@ -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?

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

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();
 

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?
      *

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

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

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)

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)

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

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;
-            }
-        }
-
-        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;
+        try {
+            return Nickname::normalize($this->tw_fields['fullname'], true);
+        } catch (NicknameException $e) {
+            return null
         }
-        return true;
     }
 }