Allow usernames to include ".", "-" and "_"

Summary:
See T1303, which presents a reasonable case for inclusion of these characters in valid usernames.

Also, unify username validity handling.

Test Plan: Created a new user with a valid name. Tried to create a new user with an invalid name. Ran unit tests.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1303

Differential Revision: https://secure.phabricator.com/D2651

scripts/user/account_admin.php

@@ -29,8 +29,8 @@ if (!strlen($username)) {
 }
 
 if (!PhabricatorUser::validateUsername($username)) {
-  echo "The username '{$username}' is invalid. Usernames must consist of only ".
+  $valid = PhabricatorUser::describeValidUsername();
-       "numbers and letters.\n";
+  echo "The username '{$username}' is invalid. {$valid}\n";
   exit(1);
 }
 

src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php

@@ -65,7 +65,7 @@ final class PhabricatorOAuthDefaultRegistrationController
         $errors[] = 'Username is required.';
       } else if (!PhabricatorUser::validateUsername($username)) {
         $e_username = 'Invalid';
-        $errors[] = 'Username must consist of only numbers and letters.';
+        $errors[] = PhabricatorUser::describeValidUsername();
       } else {
         $e_username = null;
       }

src/applications/people/PhabricatorUserEditor.php

@@ -62,6 +62,11 @@ final class PhabricatorUserEditor {
       throw new Exception("Email has already been created!");
     }
 
+    if (!PhabricatorUser::validateUsername($user->getUsername())) {
+      $valid = PhabricatorUser::describeValidUsername();
+      throw new Exception("Username is invalid! {$valid}");
+    }
+
     // Always set a new user's email address to primary.
     $email->setIsPrimary(1);
 

src/applications/people/controller/PhabricatorPeopleEditController.php

@@ -153,7 +153,7 @@ final class PhabricatorPeopleEditController
         $errors[] = "Username is required.";
         $e_username = 'Required';
       } else if (!PhabricatorUser::validateUsername($user->getUsername())) {
-        $errors[] = "Username must consist of only numbers and letters.";
+        $errors[] = PhabricatorUser::describeValidUsername();
         $e_username = 'Invalid';
       } else {
         $e_username = null;

src/applications/people/storage/PhabricatorUser.php

@@ -549,8 +549,13 @@ EOBODY;
       ->saveAndSend();
   }
 
+  public static function describeValidUsername() {
+    return 'Usernames must contain only numbers, letters, period, underscore '.
+           'and hyphen, and can not end with a period.';
+  }
+
   public static function validateUsername($username) {
-    return (bool)preg_match('/^[a-zA-Z0-9]+$/', $username);
+    return (bool)preg_match('/^[a-zA-Z0-9._-]*[a-zA-Z0-9_-]$/', $username);
   }
 
   public static function getDefaultProfileImageURI() {

src/applications/people/storage/__tests__/PhabricatorUserTestCase.php

@@ -23,14 +23,35 @@ final class PhabricatorUserTestCase extends PhabricatorTestCase {
       'alincoln'    => true,
       'alincoln69'  => true,
       'hd3'         => true,
-      '7'           => true,  // Silly, but permitted.
-      '0'           => true,
       'Alincoln'    => true,
+      'a.lincoln'   => true,
 
       'alincoln!'   => false,
-      ' alincoln'   => false,
-      '____'        => false,
       ''            => false,
+
+      // These are silly, but permitted.
+      '7'           => true,
+      '0'           => true,
+      '____'        => true,
+      '-'           => true,
+
+      // These are not permitted because they make capturing @mentions
+      // ambiguous.
+      'joe.'        => false,
+
+      // We can never allow these because they invalidate usernames as tokens
+      // in commit messages ("Reviewers: alincoln, usgrant"), or as parameters
+      // in URIs ("/p/alincoln/", "?user=alincoln"), or make them unsafe in
+      // HTML. Theoretically we escape all the HTML/URI stuff, but these
+      // restrictions make attacks more difficult and are generally reasonable,
+      // since usernames like "<^, ,^>" don't seem very important to support.
+      '<script>'    => false,
+      'a lincoln'   => false,
+      ' alincoln'   => false,
+      'alincoln '   => false,
+      'a,lincoln'   => false,
+      'a&lincoln'   => false,
+      'a/lincoln'   => false,
     );
 
     foreach ($map as $name => $expect) {

src/infrastructure/markup/remarkup/markuprule/PhabricatorRemarkupRuleMention.php

@@ -28,12 +28,13 @@ final class PhabricatorRemarkupRuleMention
   const KEY_MENTIONED = 'phabricator.mentioned-user-phids';
 
 
-  // NOTE: Negative lookahead for period prevents us from picking up email
+  // NOTE: The negative lookbehind prevents matches like "mail@lists", while
-  // addresses, while allowing constructs like "@tomo, lol". The negative
+  // allowing constructs like "@tomo/@mroch". Since we now allow periods in
-  // lookbehind for a word character prevents us from matching "mail@lists"
+  // usernames, we can't resonably distinguish that "@company.com" isn't a
-  // while allowing "@tomo/@mroch". The negative lookahead prevents us from
+  // username, so we'll incorrectly pick it up, but there's little to be done
-  // matching "@joe.com" while allowing us to match "hey, @joe.".
+  // about that. We forbid terminal periods so that we can correctly capture
-  const REGEX = '/(?<!\w)@([a-zA-Z0-9]+)\b(?![.]\w)/';
+  // "@joe" instead of "@joe." in "Hey, @joe.".
+  const REGEX = '/(?<!\w)@([a-zA-Z0-9._-]*[a-zA-Z0-9_-])/';
 
   public function apply($text) {
     return preg_replace_callback(