From 82c0795e54d126415ebc8f8b828a72d1a1894fc6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Jan 2012 07:30:28 -0800 Subject: [PATCH] Unify logic for username validation Summary: Revisit of D1254. Don't require lowercase, just standardize the logic. The current implementation has nonuniform logic -- PeopleEditController forbids uppercase. Test Plan: Ran unit tests, see also D1254. Reviewers: btrahan, jungejason, aran Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D1415 --- scripts/user/account_admin.php | 9 +++- src/__phutil_library_map__.php | 2 + ...atorOAuthDefaultRegistrationController.php | 7 ++- .../transform/PhabricatorImageTransformer.php | 2 +- .../edit/PhabricatorPeopleEditController.php | 2 +- .../people/storage/user/PhabricatorUser.php | 4 ++ .../__tests__/PhabricatorUserTestCase.php | 44 +++++++++++++++++++ .../storage/user/__tests__/__init__.php | 13 ++++++ ...habricatorProjectProfileEditController.php | 2 +- .../PhabricatorProfileHeaderView.php | 2 +- .../AphrontSideNavFilterView.php | 2 +- 11 files changed, 79 insertions(+), 10 deletions(-) create mode 100644 src/applications/people/storage/user/__tests__/PhabricatorUserTestCase.php create mode 100644 src/applications/people/storage/user/__tests__/__init__.php diff --git a/scripts/user/account_admin.php b/scripts/user/account_admin.php index cadab1db62..220ed68c1c 100755 --- a/scripts/user/account_admin.php +++ b/scripts/user/account_admin.php @@ -2,7 +2,7 @@ loadOneWhere( 'username = %s', $username); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 81d4780b5d..0a48289455 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -721,6 +721,7 @@ phutil_register_library_map(array( 'PhabricatorUserSSHKeysSettingsPanelController' => 'applications/people/controller/settings/panels/sshkeys', 'PhabricatorUserSettingsController' => 'applications/people/controller/settings', 'PhabricatorUserSettingsPanelController' => 'applications/people/controller/settings/panels/base', + 'PhabricatorUserTestCase' => 'applications/people/storage/user/__tests__', 'PhabricatorWorker' => 'infrastructure/daemon/workers/worker', 'PhabricatorWorkerDAO' => 'infrastructure/daemon/workers/storage/base', 'PhabricatorWorkerTask' => 'infrastructure/daemon/workers/storage/task', @@ -1363,6 +1364,7 @@ phutil_register_library_map(array( 'PhabricatorUserSSHKeysSettingsPanelController' => 'PhabricatorUserSettingsPanelController', 'PhabricatorUserSettingsController' => 'PhabricatorPeopleController', 'PhabricatorUserSettingsPanelController' => 'PhabricatorPeopleController', + 'PhabricatorUserTestCase' => 'PhabricatorTestCase', 'PhabricatorWorkerDAO' => 'PhabricatorLiskDAO', 'PhabricatorWorkerTask' => 'PhabricatorWorkerDAO', 'PhabricatorWorkerTaskData' => 'PhabricatorWorkerDAO', diff --git a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php index aeea60db19..29fdfca33d 100644 --- a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php +++ b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php @@ -1,7 +1,7 @@ setUsername($request->getStr('username')); $username = $user->getUsername(); - $matches = null; if (!strlen($user->getUsername())) { $e_username = 'Required'; $errors[] = 'Username is required.'; - } else if (!preg_match('/^[a-zA-Z0-9]+$/', $username, $matches)) { + } else if (!PhabricatorUser::validateUsername($username)) { $e_username = 'Invalid'; - $errors[] = 'Username may only contain letters and numbers.'; + $errors[] = 'Username must consist of only numbers and letters.'; } else { $e_username = null; } diff --git a/src/applications/files/transform/PhabricatorImageTransformer.php b/src/applications/files/transform/PhabricatorImageTransformer.php index f6cc418768..8819e95c2e 100644 --- a/src/applications/files/transform/PhabricatorImageTransformer.php +++ b/src/applications/files/transform/PhabricatorImageTransformer.php @@ -1,7 +1,7 @@ getUsername())) { $errors[] = "Username is required."; $e_username = 'Required'; - } else if (!preg_match('/^[a-z0-9]+$/', $user->getUsername())) { + } else if (!PhabricatorUser::validateUsername($user->getUsername())) { $errors[] = "Username must consist of only numbers and letters."; $e_username = 'Invalid'; } else { diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php index a5a7ef9893..12a8429856 100644 --- a/src/applications/people/storage/user/PhabricatorUser.php +++ b/src/applications/people/storage/user/PhabricatorUser.php @@ -504,4 +504,8 @@ EOBODY; ->saveAndSend(); } + public static function validateUsername($username) { + return (bool)preg_match('/^[a-zA-Z0-9]+$/', $username); + } + } diff --git a/src/applications/people/storage/user/__tests__/PhabricatorUserTestCase.php b/src/applications/people/storage/user/__tests__/PhabricatorUserTestCase.php new file mode 100644 index 0000000000..28165bf3ee --- /dev/null +++ b/src/applications/people/storage/user/__tests__/PhabricatorUserTestCase.php @@ -0,0 +1,44 @@ + true, + 'alincoln69' => true, + 'hd3' => true, + '7' => true, // Silly, but permitted. + '0' => true, + 'Alincoln' => true, + + 'alincoln!' => false, + ' alincoln' => false, + '____' => false, + '' => false, + ); + + foreach ($map as $name => $expect) { + $this->assertEqual( + $expect, + PhabricatorUser::validateUsername($name), + "Validity of '{$name}'."); + } + } + +} diff --git a/src/applications/people/storage/user/__tests__/__init__.php b/src/applications/people/storage/user/__tests__/__init__.php new file mode 100644 index 0000000000..7707c16080 --- /dev/null +++ b/src/applications/people/storage/user/__tests__/__init__.php @@ -0,0 +1,13 @@ +