From c2e3ca2f6c1fb784941b248708b92ea137e9d144 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Thu, 18 Oct 2018 15:41:26 +0800 Subject: [PATCH] MDL-62922 core_user: Check auth mechanism before validating password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 70 ++++++++++++++++++++++++++------- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/user/externallib.php b/user/externallib.php index 4ba0f6b4278..755cac14fc9 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -155,7 +155,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) { @@ -188,7 +187,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.'); } @@ -207,11 +207,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 00261fa57ce..d3212e1b15d 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); @@ -511,37 +511,79 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { ]] ); + // 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('atto', get_user_preferences('htmleditor', null, $dbuser)); - $this->assertEquals(null, get_user_preferences('invalidpreference', null, $dbuser)); + + 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)); + } 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]); } /**