From 0a7b4591efe1895867a745e528b7e70d4ce83d7e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Jun 2012 07:09:05 -0700
Subject: [PATCH] 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                |  4 +--
 ...atorOAuthDefaultRegistrationController.php |  2 +-
 .../people/PhabricatorUserEditor.php          |  5 ++++
 .../PhabricatorPeopleEditController.php       |  2 +-
 .../people/storage/PhabricatorUser.php        |  7 ++++-
 .../__tests__/PhabricatorUserTestCase.php     | 29 ++++++++++++++++---
 .../PhabricatorRemarkupRuleMention.php        | 13 +++++----
 7 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/scripts/user/account_admin.php b/scripts/user/account_admin.php
index 7e4f587c6a..52c09f2781 100755
--- a/scripts/user/account_admin.php
+++ b/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 ".
-       "numbers and letters.\n";
+  $valid = PhabricatorUser::describeValidUsername();
+  echo "The username '{$username}' is invalid. {$valid}\n";
   exit(1);
 }
 
diff --git a/src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php b/src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php
index 4d6a89f4eb..9c8c157690 100644
--- a/src/applications/auth/controller/oauthregistration/PhabricatorOAuthDefaultRegistrationController.php
+++ b/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;
       }
diff --git a/src/applications/people/PhabricatorUserEditor.php b/src/applications/people/PhabricatorUserEditor.php
index e92b7b5198..ea34144a10 100644
--- a/src/applications/people/PhabricatorUserEditor.php
+++ b/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);
 
diff --git a/src/applications/people/controller/PhabricatorPeopleEditController.php b/src/applications/people/controller/PhabricatorPeopleEditController.php
index c2e314adf4..3db70ed7ea 100644
--- a/src/applications/people/controller/PhabricatorPeopleEditController.php
+++ b/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;
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index a6055a66a8..98f513aff9 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/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() {
diff --git a/src/applications/people/storage/__tests__/PhabricatorUserTestCase.php b/src/applications/people/storage/__tests__/PhabricatorUserTestCase.php
index 28165bf3ee..09c8437fee 100644
--- a/src/applications/people/storage/__tests__/PhabricatorUserTestCase.php
+++ b/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) {
diff --git a/src/infrastructure/markup/remarkup/markuprule/PhabricatorRemarkupRuleMention.php b/src/infrastructure/markup/remarkup/markuprule/PhabricatorRemarkupRuleMention.php
index 1e833a16ee..ba42697c42 100644
--- a/src/infrastructure/markup/remarkup/markuprule/PhabricatorRemarkupRuleMention.php
+++ b/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
-  // addresses, while allowing constructs like "@tomo, lol". The negative
-  // lookbehind for a word character prevents us from matching "mail@lists"
-  // while allowing "@tomo/@mroch". The negative lookahead prevents us from
-  // matching "@joe.com" while allowing us to match "hey, @joe.".
-  const REGEX = '/(?<!\w)@([a-zA-Z0-9]+)\b(?![.]\w)/';
+  // NOTE: The negative lookbehind prevents matches like "mail@lists", while
+  // allowing constructs like "@tomo/@mroch". Since we now allow periods in
+  // usernames, we can't resonably distinguish that "@company.com" isn't a
+  // username, so we'll incorrectly pick it up, but there's little to be done
+  // about that. We forbid terminal periods so that we can correctly capture
+  // "@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(