From eb1a4776358a8e0df1d215b77a4f922f0672887c Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Thu, 18 Oct 2018 15:24:46 +0800 Subject: [PATCH] MDL-62922 core_user: Check auth mechanism before validating password * External authentication mechanisms (e.g. via oauth2, etc.) don't store password in the user table, so we shouldn't be requiring password in such case when creating users via the core_user_create_users WS function. --- user/externallib.php | 16 ++++--- user/tests/externallib_test.php | 85 ++++++++++++++++++++++++--------- 2 files changed, 72 insertions(+), 29 deletions(-) diff --git a/user/externallib.php b/user/externallib.php index 2023cd805e8..84adf0f06f6 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -161,7 +161,6 @@ class core_user_external extends external_api { $transaction = $DB->start_delegated_transaction(); $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) { @@ -194,7 +193,8 @@ class core_user_external extends external_api { } // Make sure we have a password or have to create one. - if (empty($user['password']) && empty($user['createpassword'])) { + $authplugin = get_auth_plugin($user['auth']); + if ($authplugin->is_internal() && empty($user['password']) && empty($user['createpassword'])) { throw new invalid_parameter_exception('Invalid password: you must provide a password, or set createpassword.'); } @@ -213,11 +213,15 @@ class core_user_external extends external_api { $createpassword = !empty($user['createpassword']); unset($user['createpassword']); - if ($createpassword) { - $user['password'] = ''; - $updatepassword = false; + $updatepassword = false; + if ($authplugin->is_internal()) { + if ($createpassword) { + $user['password'] = ''; + } else { + $updatepassword = true; + } } else { - $updatepassword = true; + $user['password'] = AUTH_PASSWORD_NOT_CACHED; } // Create the user data now! diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index 36fa7b5d9ba..02891dfdb73 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -484,7 +484,7 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { * Test create_users */ public function test_create_users() { - global $USER, $CFG, $DB; + global $DB; $this->resetAfterTest(true); @@ -517,46 +517,85 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { 'interests' => 'badminton, basketball, cooking, ' ); + // User with an authentication method done externally. + $user2 = array( + 'username' => 'usernametest2', + 'firstname' => 'First Name User Test 2', + 'lastname' => 'Last Name User Test 2', + 'email' => 'usertest2@example.com', + 'auth' => 'oauth2' + ); + $context = context_system::instance(); $roleid = $this->assignUserCapability('moodle/user:create', $context->id); $this->assignUserCapability('moodle/user:editprofile', $context->id, $roleid); // Call the external function. - $createdusers = core_user_external::create_users(array($user1)); + $createdusers = core_user_external::create_users(array($user1, $user2)); // We need to execute the return values cleaning process to simulate the web service server. $createdusers = external_api::clean_returnvalue(core_user_external::create_users_returns(), $createdusers); // Check we retrieve the good total number of created users + no error on capability. - $this->assertEquals(1, count($createdusers)); + $this->assertCount(2, $createdusers); foreach($createdusers as $createduser) { $dbuser = $DB->get_record('user', array('id' => $createduser['id'])); - $this->assertEquals($dbuser->username, $user1['username']); - $this->assertEquals($dbuser->idnumber, $user1['idnumber']); - $this->assertEquals($dbuser->firstname, $user1['firstname']); - $this->assertEquals($dbuser->lastname, $user1['lastname']); - $this->assertEquals($dbuser->email, $user1['email']); - $this->assertEquals($dbuser->description, $user1['description']); - $this->assertEquals($dbuser->city, $user1['city']); - $this->assertEquals($dbuser->country, $user1['country']); - $this->assertEquals($dbuser->department, $user1['department']); - $this->assertEquals($dbuser->institution, $user1['institution']); - $this->assertEquals($dbuser->phone1, $user1['phone1']); - $this->assertEquals($dbuser->maildisplay, $user1['maildisplay']); - $this->assertEquals('atto', get_user_preferences('htmleditor', null, $dbuser)); - $this->assertEquals(null, get_user_preferences('invalidpreference', null, $dbuser)); - // Confirm user interests have been saved. - $interests = core_tag_tag::get_item_tags_array('core', 'user', $createduser['id'], core_tag_tag::BOTH_STANDARD_AND_NOT, - 0, false); - // There should be 3 user interests. - $this->assertCount(3, $interests); + + if ($createduser['username'] === $user1['username']) { + $usertotest = $user1; + $this->assertEquals('atto', get_user_preferences('htmleditor', null, $dbuser)); + $this->assertEquals(null, get_user_preferences('invalidpreference', null, $dbuser)); + // Confirm user interests have been saved. + $interests = core_tag_tag::get_item_tags_array('core', 'user', $createduser['id'], + core_tag_tag::BOTH_STANDARD_AND_NOT, 0, false); + // There should be 3 user interests. + $this->assertCount(3, $interests); + + } else if ($createduser['username'] === $user2['username']) { + $usertotest = $user2; + } + + foreach ($dbuser as $property => $value) { + if ($property === 'password') { + if ($usertotest === $user2) { + // External auth mechanisms don't store password in the user table. + $this->assertEquals(AUTH_PASSWORD_NOT_CACHED, $value); + } else { + // Skip hashed passwords. + continue; + } + } + // Confirm that the values match. + if (isset($usertotest[$property])) { + $this->assertEquals($usertotest[$property], $value); + } + } } // Call without required capability $this->unassignUserCapability('moodle/user:create', $context->id, $roleid); $this->expectException('required_capability_exception'); - $createdusers = core_user_external::create_users(array($user1)); + core_user_external::create_users(array($user1)); + } + + /** + * Test create_users with password and createpassword parameter not set. + */ + public function test_create_users_empty_password() { + $this->resetAfterTest(); + $this->setAdminUser(); + + $user = [ + 'username' => 'usernametest1', + 'firstname' => 'First Name User Test 1', + 'lastname' => 'Last Name User Test 1', + 'email' => 'usertest1@example.com', + ]; + + // This should throw an exception because either password or createpassword param must be passed for auth_manual. + $this->expectException(invalid_parameter_exception::class); + core_user_external::create_users([$user]); } /**