From 6e7d77d11c6af035758a1c34249cdbcebe8dda31 Mon Sep 17 00:00:00 2001 From: Vitaly Potenko Date: Mon, 17 Aug 2020 15:45:53 +0300 Subject: [PATCH 1/2] MDL-69194 user: make core_user_update_users return warnings The external function 'core_user_update_users()' always returned 'null' no matter if a user or users were successfully updated or there were some failures. So, there was no way for the caller to know which users were updated and which were not. After the commit changes the function returns an 'external_warnings' instance. The function uses a delegated transaction for each user to update within a loop. This enables the function to update as many users as possible. This differs from the previous behavior of the function when it used a delegate transaction outside of the loop where the users were updated. This resulted in a rollback of the whole users updating in case any of the users had some invalid data. For each user within a loop a 'try-catch' block is used to throw exceptions which are actually returned as warnings by the function when they are caught. --- lang/en/error.php | 2 +- user/externallib.php | 182 +++++++++++++++++++++++++------------------ 2 files changed, 107 insertions(+), 77 deletions(-) diff --git a/lang/en/error.php b/lang/en/error.php index f0b6b89212f..2ef1fc7f36b 100644 --- a/lang/en/error.php +++ b/lang/en/error.php @@ -626,4 +626,4 @@ $string['wwwrootslash'] = 'Detected incorrect $CFG->wwwroot in config.php, it mu $string['xmldberror'] = 'XMLDB error!'; $string['alreadyloggedin'] = 'You are already logged in as {$a}, you need to log out before logging in as different user.'; $string['youcannotdeletecategory'] = 'You cannot delete category \'{$a}\' because you can neither delete the contents, nor move them elsewhere.'; -$string['protected_cc_not_supported'] = 'Protected cartridges not supported.'; +$string['protected_cc_not_supported'] = 'Protected cartridges not supported.'; \ No newline at end of file diff --git a/user/externallib.php b/user/externallib.php index 86381b25fea..8836e059402 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -564,106 +564,136 @@ class core_user_external extends external_api { 'maxfiles' => 1, 'accepted_types' => 'optimised_image'); - $transaction = $DB->start_delegated_transaction(); - + $warnings = array(); foreach ($params['users'] as $user) { - // First check the user exists. - if (!$existinguser = core_user::get_user($user['id'])) { - continue; - } - // Check if we are trying to update an admin. - if ($existinguser->id != $USER->id and is_siteadmin($existinguser) and !is_siteadmin($USER)) { - continue; - } - // Other checks (deleted, remote or guest users). - if ($existinguser->deleted or is_mnet_remote_user($existinguser) or isguestuser($existinguser->id)) { - continue; - } - // Check duplicated emails. - if (isset($user['email']) && $user['email'] !== $existinguser->email) { - if (!validate_email($user['email'])) { - continue; - } else if (empty($CFG->allowaccountssameemail)) { - // Make a case-insensitive query for the given email address and make sure to exclude the user being updated. - $select = $DB->sql_equal('email', ':email', false) . ' AND mnethostid = :mnethostid AND id <> :userid'; - $params = array( - 'email' => $user['email'], - 'mnethostid' => $CFG->mnet_localhost_id, - 'userid' => $user['id'] - ); - // Skip if there are other user(s) that already have the same email. - if ($DB->record_exists_select('user', $select, $params)) { - continue; + // Catch any exception while updating a user and return it as a warning. + try { + $transaction = $DB->start_delegated_transaction(); + + // First check the user exists. + if (!$existinguser = core_user::get_user($user['id'])) { + throw new moodle_exception('invaliduserid'); + } + // Check if we are trying to update an admin. + if ($existinguser->id != $USER->id and is_siteadmin($existinguser) and !is_siteadmin($USER)) { + throw new moodle_exception('usernotupdatedadmin'); + } + // Other checks (deleted, remote or guest users). + if ($existinguser->deleted) { + throw new moodle_exception('usernotupdateddeleted'); + } + if (is_mnet_remote_user($existinguser)) { + throw new moodle_exception('usernotupdatedremote'); + } + if (isguestuser($existinguser->id)) { + throw new moodle_exception('usernotupdatedguest'); + } + // Check duplicated emails. + if (isset($user['email']) && $user['email'] !== $existinguser->email) { + if (!validate_email($user['email'])) { + throw new moodle_exception('useremailinvalid'); + } else if (empty($CFG->allowaccountssameemail)) { + // Make a case-insensitive query for the given email address + // and make sure to exclude the user being updated. + $select = $DB->sql_equal('email', ':email', false) . ' AND mnethostid = :mnethostid AND id <> :userid'; + $params = array( + 'email' => $user['email'], + 'mnethostid' => $CFG->mnet_localhost_id, + 'userid' => $user['id'] + ); + // Skip if there are other user(s) that already have the same email. + if ($DB->record_exists_select('user', $select, $params)) { + throw new moodle_exception('useremailduplicate'); + } } } - } - user_update_user($user, true, false); + user_update_user($user, true, false); - $userobject = (object)$user; + $userobject = (object)$user; - // Update user picture if it was specified for this user. - if (empty($CFG->disableuserimages) && isset($user['userpicture'])) { - $userobject->deletepicture = null; + // Update user picture if it was specified for this user. + if (empty($CFG->disableuserimages) && isset($user['userpicture'])) { + $userobject->deletepicture = null; - if ($user['userpicture'] == 0) { - $userobject->deletepicture = true; - } else { - $userobject->imagefile = $user['userpicture']; + if ($user['userpicture'] == 0) { + $userobject->deletepicture = true; + } else { + $userobject->imagefile = $user['userpicture']; + } + + core_user::update_picture($userobject, $filemanageroptions); } - core_user::update_picture($userobject, $filemanageroptions); - } - - // Update user interests. - if (!empty($user['interests'])) { - $trimmedinterests = array_map('trim', explode(',', $user['interests'])); - $interests = array_filter($trimmedinterests, function($value) { - return !empty($value); - }); - useredit_update_interests($userobject, $interests); - } - - // Update user custom fields. - if (!empty($user['customfields'])) { - - foreach ($user['customfields'] as $customfield) { - // Profile_save_data() saves profile file it's expecting a user with the correct id, - // and custom field to be named profile_field_"shortname". - $user["profile_field_".$customfield['type']] = $customfield['value']; + // Update user interests. + if (!empty($user['interests'])) { + $trimmedinterests = array_map('trim', explode(',', $user['interests'])); + $interests = array_filter($trimmedinterests, function($value) { + return !empty($value); + }); + useredit_update_interests($userobject, $interests); } - profile_save_data((object) $user); - } - // Trigger event. - \core\event\user_updated::create_from_userid($user['id'])->trigger(); + // Update user custom fields. + if (!empty($user['customfields'])) { - // Preferences. - if (!empty($user['preferences'])) { - $userpref = clone($existinguser); - foreach ($user['preferences'] as $preference) { - $userpref->{'preference_'.$preference['type']} = $preference['value']; + foreach ($user['customfields'] as $customfield) { + // Profile_save_data() saves profile file it's expecting a user with the correct id, + // and custom field to be named profile_field_"shortname". + $user["profile_field_".$customfield['type']] = $customfield['value']; + } + profile_save_data((object) $user); + } + + // Trigger event. + \core\event\user_updated::create_from_userid($user['id'])->trigger(); + + // Preferences. + if (!empty($user['preferences'])) { + $userpref = clone($existinguser); + foreach ($user['preferences'] as $preference) { + $userpref->{'preference_'.$preference['type']} = $preference['value']; + } + useredit_update_user_preference($userpref); + } + if (isset($user['suspended']) and $user['suspended']) { + \core\session\manager::kill_user_sessions($user['id']); + } + + $transaction->allow_commit(); + } catch (Exception $e) { + try { + $transaction->rollback($e); + } catch (Exception $e) { + $warning = []; + $warning['item'] = 'user'; + $warning['itemid'] = $user['id']; + if ($e instanceof moodle_exception) { + $warning['warningcode'] = $e->errorcode; + } else { + $warning['warningcode'] = $e->getCode(); + } + $warning['message'] = $e->getMessage(); + $warnings[] = $warning; } - useredit_update_user_preference($userpref); - } - if (isset($user['suspended']) and $user['suspended']) { - \core\session\manager::kill_user_sessions($user['id']); } } - $transaction->allow_commit(); - - return null; + return ['warnings' => $warnings]; } /** * Returns description of method result value * - * @return null + * @return external_description * @since Moodle 2.2 */ public static function update_users_returns() { - return null; + return new external_single_structure( + array( + 'warnings' => new external_warnings() + ) + ); } /** From 8281261b5aee333f7e45c6a79a1bcc9392b77e8d Mon Sep 17 00:00:00 2001 From: Matt Porritt Date: Thu, 8 Apr 2021 00:44:17 +0000 Subject: [PATCH 2/2] MDL-69194 user: make core_user_update_users return warnings Add unit test coverage for the new warning scenarios. --- lang/en/error.php | 2 +- user/externallib.php | 21 ++++++++++----- user/tests/externallib_test.php | 48 ++++++++++++++++++++++++++++++++- user/upgrade.txt | 7 +++++ 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/lang/en/error.php b/lang/en/error.php index 2ef1fc7f36b..f0b6b89212f 100644 --- a/lang/en/error.php +++ b/lang/en/error.php @@ -626,4 +626,4 @@ $string['wwwrootslash'] = 'Detected incorrect $CFG->wwwroot in config.php, it mu $string['xmldberror'] = 'XMLDB error!'; $string['alreadyloggedin'] = 'You are already logged in as {$a}, you need to log out before logging in as different user.'; $string['youcannotdeletecategory'] = 'You cannot delete category \'{$a}\' because you can neither delete the contents, nor move them elsewhere.'; -$string['protected_cc_not_supported'] = 'Protected cartridges not supported.'; \ No newline at end of file +$string['protected_cc_not_supported'] = 'Protected cartridges not supported.'; diff --git a/user/externallib.php b/user/externallib.php index 8836e059402..0f402dfa9a0 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -572,26 +572,32 @@ class core_user_external extends external_api { // First check the user exists. if (!$existinguser = core_user::get_user($user['id'])) { - throw new moodle_exception('invaliduserid'); + throw new moodle_exception('invaliduserid', '', '', null, + 'Invalid user ID'); } // Check if we are trying to update an admin. if ($existinguser->id != $USER->id and is_siteadmin($existinguser) and !is_siteadmin($USER)) { - throw new moodle_exception('usernotupdatedadmin'); + throw new moodle_exception('usernotupdatedadmin', '', '', null, + 'Cannot update admin accounts'); } // Other checks (deleted, remote or guest users). if ($existinguser->deleted) { - throw new moodle_exception('usernotupdateddeleted'); + throw new moodle_exception('usernotupdateddeleted', '', '', null, + 'User is a deleted user'); } if (is_mnet_remote_user($existinguser)) { - throw new moodle_exception('usernotupdatedremote'); + throw new moodle_exception('usernotupdatedremote', '', '', null, + 'User is a remote user'); } if (isguestuser($existinguser->id)) { - throw new moodle_exception('usernotupdatedguest'); + throw new moodle_exception('usernotupdatedguest', '', '', null, + 'Cannot update guest account'); } // Check duplicated emails. if (isset($user['email']) && $user['email'] !== $existinguser->email) { if (!validate_email($user['email'])) { - throw new moodle_exception('useremailinvalid'); + throw new moodle_exception('useremailinvalid', '', '', null, + 'Invalid email address'); } else if (empty($CFG->allowaccountssameemail)) { // Make a case-insensitive query for the given email address // and make sure to exclude the user being updated. @@ -603,7 +609,8 @@ class core_user_external extends external_api { ); // Skip if there are other user(s) that already have the same email. if ($DB->record_exists_select('user', $select, $params)) { - throw new moodle_exception('useremailduplicate'); + throw new moodle_exception('useremailduplicate', '', '', null, + 'Duplicate email address'); } } } diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index f2a6176745c..fd9f5cb1693 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -715,6 +715,7 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { global $USER, $CFG, $DB; $this->resetAfterTest(true); + $this->preventResetByRollback(); $wsuser = self::getDataGenerator()->create_user(); self::setUser($wsuser); @@ -780,8 +781,20 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $user4['id'] = $userdeleted->id; user_delete_user($userdeleted); + $user5 = self::getDataGenerator()->create_user(); + $user5 = array('id' => $user5->id, 'email' => $user5->email); + // Call the external function. - core_user_external::update_users(array($user1, $user2, $user3, $user4)); + $returnvalue = core_user_external::update_users(array($user1, $user2, $user3, $user4)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + + // Check warnings. + $this->assertEquals($user2['id'], $returnvalue['warnings'][0]['itemid']); // Guest user. + $this->assertEquals('usernotupdatedguest', $returnvalue['warnings'][0]['warningcode']); + $this->assertEquals($user3['id'], $returnvalue['warnings'][1]['itemid']); // Admin user. + $this->assertEquals('usernotupdatedadmin', $returnvalue['warnings'][1]['warningcode']); + $this->assertEquals($user4['id'], $returnvalue['warnings'][2]['itemid']); // Deleted user. + $this->assertEquals('usernotupdateddeleted', $returnvalue['warnings'][2]['warningcode']); $dbuser2 = $DB->get_record('user', array('id' => $user2['id'])); $this->assertNotEquals($dbuser2->username, $user2['username']); @@ -824,6 +837,39 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $dbuserdelpic = $DB->get_record('user', array('id' => $user1['id'])); $this->assertEquals(0, $dbuserdelpic->picture, 'Picture must be deleted when sent as 0.'); + // Updating user with an invalid email. + $user5['email'] = 'bogus'; + $returnvalue = core_user_external::update_users(array($user5)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('useremailinvalid', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('Invalid email address', + $returnvalue['warnings'][0]['message']); + + // Updating user with a duplicate email. + $user5['email'] = $user1['email']; + $returnvalue = core_user_external::update_users(array($user1, $user5)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('useremailduplicate', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('Duplicate email address', + $returnvalue['warnings'][0]['message']); + + // Updating a user that does not exist. + $user5['id'] = -1; + $returnvalue = core_user_external::update_users(array($user5)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('invaliduserid', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('Invalid user ID', + $returnvalue['warnings'][0]['message']); + + // Updating a remote user. + $user1['mnethostid'] = 5; + user_update_user($user1); // Update user not using webservice. + unset($user1['mnethostid']); // The mnet host ID field is not in the allowed field list for the webservice. + $returnvalue = core_user_external::update_users(array($user1)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('usernotupdatedremote', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('User is a remote user', + $returnvalue['warnings'][0]['message']); // Call without required capability. $this->unassignUserCapability('moodle/user:update', $context->id, $roleid); diff --git a/user/upgrade.txt b/user/upgrade.txt index d20af1afffd..51d0ee8f5de 100644 --- a/user/upgrade.txt +++ b/user/upgrade.txt @@ -1,5 +1,12 @@ This files describes API changes for code that uses the user API. +=== 4.0 === + +* External function core_user_external::update_users() will now fail on a per user basis. Previously if one user + update failed all users in the operation would fail. +* External function core_user_external::update_users() now returns an error code and message to why a user update + action failed. + === 3.11 === * Added new core_user/form_user_selector JS module that can be used as the 'ajax' handler for the autocomplete form