From 0e99d58c35953026bcc6d3890b6d0dec49cd3d06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 17 Aug 2018 12:33:25 +0200 Subject: [PATCH 1/2] MDL-63174 user: core_user_create_users to throw exception on empty names To be consistent with the web administration UI, we should not allow to create invalid user records with empty username, lastname or firstname via the web services. --- user/externallib.php | 7 +++ user/tests/externallib_test.php | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/user/externallib.php b/user/externallib.php index 83da6dcd000..4ba0f6b4278 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -157,6 +157,13 @@ class core_user_external extends external_api { $userids = array(); $createpassword = false; foreach ($params['users'] as $user) { + // Make sure that the username, firstname and lastname are not blank. + foreach (array('username', 'firstname', 'lastname') as $fieldname) { + if (trim($user[$fieldname]) === '') { + throw new invalid_parameter_exception('The field '.$fieldname.' cannot be blank'); + } + } + // Make sure that the username doesn't already exist. if ($DB->record_exists('user', array('username' => $user['username'], 'mnethostid' => $CFG->mnet_localhost_id))) { throw new invalid_parameter_exception('Username already exists: '.$user['username']); diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index 3afffd85ba1..00261fa57ce 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -544,6 +544,84 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $createdusers = core_user_external::create_users(array($user1)); } + /** + * Test create_users with invalid parameters + * + * @dataProvider data_create_users_invalid_parameter + * @param array $data User data to attempt to register. + * @param string $expectmessage Expected exception message. + */ + public function test_create_users_invalid_parameter(array $data, $expectmessage) { + global $USER, $CFG, $DB; + + $this->resetAfterTest(true); + $this->assignUserCapability('moodle/user:create', SYSCONTEXTID); + + $this->expectException('invalid_parameter_exception'); + $this->expectExceptionMessage($expectmessage); + + core_user_external::create_users(array($data)); + } + + /** + * Data provider for {@link self::test_create_users_invalid_parameter()}. + * + * @return array + */ + public function data_create_users_invalid_parameter() { + return [ + 'blank_username' => [ + 'data' => [ + 'username' => '', + 'firstname' => 'Foo', + 'lastname' => 'Bar', + 'email' => 'foobar@example.com', + 'createpassword' => 1, + ], + 'expectmessage' => 'The field username cannot be blank', + ], + 'blank_firtname' => [ + 'data' => [ + 'username' => 'foobar', + 'firstname' => "\t \n", + 'lastname' => 'Bar', + 'email' => 'foobar@example.com', + 'createpassword' => 1, + ], + 'expectmessage' => 'The field firstname cannot be blank', + ], + 'blank_lastname' => [ + 'data' => [ + 'username' => 'foobar', + 'firstname' => '0', + 'lastname' => ' ', + 'email' => 'foobar@example.com', + 'createpassword' => 1, + ], + 'expectmessage' => 'The field lastname cannot be blank', + ], + 'invalid_email' => [ + 'data' => [ + 'username' => 'foobar', + 'firstname' => 'Foo', + 'lastname' => 'Bar', + 'email' => '@foobar', + 'createpassword' => 1, + ], + 'expectmessage' => 'Email address is invalid', + ], + 'missing_password' => [ + 'data' => [ + 'username' => 'foobar', + 'firstname' => 'Foo', + 'lastname' => 'Bar', + 'email' => 'foobar@example.com', + ], + 'expectmessage' => 'Invalid password: you must provide a password, or set createpassword', + ], + ]; + } + /** * Test delete_users */ From 784883e1ee1b37a5277b8e374eb3b43a5e156ecb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 17 Aug 2018 12:57:35 +0200 Subject: [PATCH 2/2] MDL-63174 user: user_create_user to throw exception on empty username The core API function user_create_user() did not check the case when the given username was empty. Also adding a missing string 'usernamelowercase' for the existing lower case check and unit tests. --- lang/en/error.php | 2 ++ user/lib.php | 12 ++++++--- user/tests/userlib_test.php | 49 +++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/lang/en/error.php b/lang/en/error.php index 1c7de748706..f15f401e74d 100644 --- a/lang/en/error.php +++ b/lang/en/error.php @@ -364,6 +364,7 @@ $string['invaliduserid'] = 'Invalid user id'; $string['invaliduserfield'] = 'Invalid user field: {$a}'; $string['invaliduserdata'] = 'Invalid user data: {$a}'; $string['invalidusername'] = 'The given username contains invalid characters'; +$string['invalidusernameblank'] = 'The username cannot be blank'; $string['invalidxmlfile'] = '"{$a}" is not a valid XML file'; $string['iplookupfailed'] = 'Cannot find geo information about this IP address {$a}'; $string['iplookupprivate'] = 'Cannot display lookup of private IP address'; @@ -565,6 +566,7 @@ $string['userautherror'] = 'Unknown auth plugin'; $string['userauthunsupported'] = 'Auth plugin not supported here'; $string['useremailduplicate'] = 'Duplicate address'; $string['usermustbemnet'] = 'Users in the MNET access control list must be remote MNET users'; +$string['usernamelowercase'] = 'The username must be in lower case'; $string['usernotaddederror'] = 'User not added - error'; $string['usernotaddedregistered'] = 'User not added - already registered'; $string['usernotavailable'] = 'The details of this user are not available to you'; diff --git a/user/lib.php b/user/lib.php index 985c402daf6..f7008b5fe60 100644 --- a/user/lib.php +++ b/user/lib.php @@ -48,12 +48,16 @@ function user_create_user($user, $updatepassword = true, $triggerevent = true) { } // Check username. + if (trim($user->username) === '') { + throw new moodle_exception('invalidusernameblank'); + } + if ($user->username !== core_text::strtolower($user->username)) { throw new moodle_exception('usernamelowercase'); - } else { - if ($user->username !== core_user::clean_field($user->username, 'username')) { - throw new moodle_exception('invalidusername'); - } + } + + if ($user->username !== core_user::clean_field($user->username, 'username')) { + throw new moodle_exception('invalidusername'); } // Save the password in a temp value for later. diff --git a/user/tests/userlib_test.php b/user/tests/userlib_test.php index bc5347c5ef3..b7f6d6c4567 100644 --- a/user/tests/userlib_test.php +++ b/user/tests/userlib_test.php @@ -241,6 +241,55 @@ class core_userliblib_testcase extends advanced_testcase { $this->assertDebuggingNotCalled(); } + /** + * Test that {@link user_create_user()} throws exception when invalid username is provided. + * + * @dataProvider data_create_user_invalid_username + * @param string $username Invalid username + * @param string $expectmessage Expected exception message + */ + public function test_create_user_invalid_username($username, $expectmessage) { + global $CFG; + + $this->resetAfterTest(); + $CFG->extendedusernamechars = false; + + $user = [ + 'username' => $username, + ]; + + $this->expectException('moodle_exception'); + $this->expectExceptionMessage($expectmessage); + + user_create_user($user); + } + + /** + * Data provider for {@link self::test_create_user_invalid_username()}. + * + * @return array + */ + public function data_create_user_invalid_username() { + return [ + 'empty_string' => [ + '', + 'The username cannot be blank', + ], + 'only_whitespace' => [ + "\t\t \t\n ", + 'The username cannot be blank', + ], + 'lower_case' => [ + 'Mudrd8mz', + 'The username must be in lower case', + ], + 'extended_chars' => [ + 'dmudrák', + 'The given username contains invalid characters', + ], + ]; + } + /** * Test function user_count_login_failures(). */