diff --git a/admin/tool/monitor/classes/task/check_subscriptions.php b/admin/tool/monitor/classes/task/check_subscriptions.php index 8262f102f53..59125dfeb7f 100644 --- a/admin/tool/monitor/classes/task/check_subscriptions.php +++ b/admin/tool/monitor/classes/task/check_subscriptions.php @@ -199,7 +199,7 @@ class check_subscriptions extends \core\task\scheduled_task { */ protected function is_user_setup($user) { if (!isset($this->userssetupcache[$user->id])) { - $this->userssetupcache[$user->id] = !user_not_fully_set_up($user); + $this->userssetupcache[$user->id] = !user_not_fully_set_up($user, true); } return $this->userssetupcache[$user->id]; } diff --git a/auth/ldap/auth.php b/auth/ldap/auth.php index 75b0d84f236..f80d6b3cdbb 100644 --- a/auth/ldap/auth.php +++ b/auth/ldap/auth.php @@ -1771,7 +1771,7 @@ class auth_plugin_ldap extends auth_plugin_base { unset_cache_flag($this->pluginconfig.'/ntlmsess', $key); // Redirection - if (user_not_fully_set_up($USER)) { + if (user_not_fully_set_up($USER, true)) { $urltogo = $CFG->wwwroot.'/user/edit.php'; // We don't delete $SESSION->wantsurl yet, so we get there later } else if (isset($SESSION->wantsurl) and (strpos($SESSION->wantsurl, $CFG->wwwroot) === 0)) { diff --git a/auth/mnet/auth.php b/auth/mnet/auth.php index 11ddba2ce44..6f1cf0f9c5d 100644 --- a/auth/mnet/auth.php +++ b/auth/mnet/auth.php @@ -263,7 +263,7 @@ class auth_plugin_mnet extends auth_plugin_base { exit; } - if (user_not_fully_set_up($remoteuser)) { + if (user_not_fully_set_up($remoteuser, false)) { print_error('notenoughidpinfo', 'mnet'); exit; } diff --git a/auth/shibboleth/index.php b/auth/shibboleth/index.php index 4e5c92a1d79..a8cbd03c78d 100644 --- a/auth/shibboleth/index.php +++ b/auth/shibboleth/index.php @@ -54,7 +54,7 @@ && $user = authenticate_user_login($frm->username, $frm->password)) { complete_user_login($user); - if (user_not_fully_set_up($USER)) { + if (user_not_fully_set_up($USER, true)) { $urltogo = $CFG->wwwroot.'/user/edit.php?id='.$USER->id.'&course='.SITEID; // We don't delete $SESSION->wantsurl yet, so we get there later diff --git a/lib/moodlelib.php b/lib/moodlelib.php index a54828da087..cb08387dae6 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -2644,8 +2644,17 @@ function require_login($courseorid = null, $autologinguest = true, $cm = null, $ } } - // Check that the user account is properly set up. - if (user_not_fully_set_up($USER)) { + // Check that the user account is properly set up. If we can't redirect to + // edit their profile, perform just the lax check. It will allow them to + // use filepicker on the profile edit page. + + if ($preventredirect) { + $usernotfullysetup = user_not_fully_set_up($USER, false); + } else { + $usernotfullysetup = user_not_fully_set_up($USER, true); + } + + if ($usernotfullysetup) { if ($preventredirect) { throw new require_login_exception('User not fully set-up'); } @@ -3160,14 +3169,39 @@ function update_user_login_times() { /** * Determines if a user has completed setting up their account. * + * The lax mode (with $strict = false) has been introduced for special cases + * only where we want to skip certain checks intentionally. This is valid in + * certain mnet or ajax scenarios when the user cannot / should not be + * redirected to edit their profile. In most cases, you should perform the + * strict check. + * * @param stdClass $user A {@link $USER} object to test for the existence of a valid name and email + * @param bool $strict Be more strict and assert id and custom profile fields set, too * @return bool */ -function user_not_fully_set_up($user) { +function user_not_fully_set_up($user, $strict = true) { + global $CFG; + require_once($CFG->dirroot.'/user/profile/lib.php'); + if (isguestuser($user)) { return false; } - return (empty($user->firstname) or empty($user->lastname) or empty($user->email) or over_bounce_threshold($user)); + + if (empty($user->firstname) or empty($user->lastname) or empty($user->email) or over_bounce_threshold($user)) { + return true; + } + + if ($strict) { + if (empty($user->id)) { + // Strict mode can be used with existing accounts only. + return true; + } + if (!profile_has_required_custom_fields_set($user->id)) { + return true; + } + } + + return false; } /** diff --git a/login/lib.php b/login/lib.php index 07c735fcf5a..3e522ffe87f 100644 --- a/login/lib.php +++ b/login/lib.php @@ -285,7 +285,7 @@ function core_login_generate_password_reset ($user) { function core_login_get_return_url() { global $CFG, $SESSION, $USER; // Prepare redirection. - if (user_not_fully_set_up($USER)) { + if (user_not_fully_set_up($USER, true)) { $urltogo = $CFG->wwwroot.'/user/edit.php'; // We don't delete $SESSION->wantsurl yet, so we get there later. diff --git a/user/edit.php b/user/edit.php index a3341f07045..044522a76ca 100644 --- a/user/edit.php +++ b/user/edit.php @@ -72,9 +72,11 @@ if (isguestuser($user)) { // User interests separated by commas. $user->interests = core_tag_tag::get_item_tags_array('core', 'user', $user->id); -// Remote users cannot be edited. +// Remote users cannot be edited. Note we have to perform the strict user_not_fully_set_up() check. +// Otherwise the remote user could end up in endless loop between user/view.php and here. +// Required custom fields are not supported in MNet environment anyway. if (is_mnet_remote_user($user)) { - if (user_not_fully_set_up($user)) { + if (user_not_fully_set_up($user, true)) { $hostwwwroot = $DB->get_field('mnet_host', 'wwwroot', array('id' => $user->mnethostid)); print_error('usernotfullysetup', 'mnet', '', $hostwwwroot); } diff --git a/user/editlib.php b/user/editlib.php index e09935b52c4..9e0a66b25b9 100644 --- a/user/editlib.php +++ b/user/editlib.php @@ -76,7 +76,7 @@ function useredit_setup_preference_page($userid, $courseid) { // Remote users cannot be edited. if (is_mnet_remote_user($user)) { - if (user_not_fully_set_up($user)) { + if (user_not_fully_set_up($user, false)) { $hostwwwroot = $DB->get_field('mnet_host', 'wwwroot', array('id' => $user->mnethostid)); print_error('usernotfullysetup', 'mnet', '', $hostwwwroot); } diff --git a/user/profile/lib.php b/user/profile/lib.php index d38bbe8174c..f712a97464a 100644 --- a/user/profile/lib.php +++ b/user/profile/lib.php @@ -662,3 +662,30 @@ function profile_view($user, $context, $course = null) { $event->trigger(); } +/** + * Does the user have all required custom fields set? + * + * Internal, to be exclusively used by {@link user_not_fully_set_up()} only. + * + * Note that if users have no way to fill a required field via editing their + * profiles (e.g. the field is not visible or it is locked), we still return true. + * So this is actually checking if we should redirect the user to edit their + * profile, rather than whether there is a value in the database. + * + * @param int $userid + * @return bool + */ +function profile_has_required_custom_fields_set($userid) { + global $DB; + + $sql = "SELECT f.id + FROM {user_info_field} f + LEFT JOIN {user_info_data} d ON (d.fieldid = f.id AND d.userid = ?) + WHERE f.required = 1 AND f.visible > 0 AND f.locked = 0 AND d.id IS NULL"; + + if ($DB->record_exists_sql($sql, [$userid])) { + return false; + } + + return true; +} diff --git a/user/tests/profilelib_test.php b/user/tests/profilelib_test.php index 6e24e0ebdeb..0c4c5993dac 100644 --- a/user/tests/profilelib_test.php +++ b/user/tests/profilelib_test.php @@ -143,4 +143,71 @@ class core_user_profilelib_testcase extends advanced_testcase { } + /** + * Test that {@link user_not_fully_set_up()} takes required custom fields into account. + */ + public function test_profile_has_required_custom_fields_set() { + global $CFG, $DB; + require_once($CFG->dirroot.'/mnet/lib.php'); + + $this->resetAfterTest(); + + // Add a required, visible, unlocked custom field. + $DB->insert_record('user_info_field', ['shortname' => 'house', 'name' => 'House', 'required' => 1, + 'visible' => 1, 'locked' => 0, 'categoryid' => 1, 'datatype' => 'text']); + + // Add an optional, visible, unlocked custom field. + $DB->insert_record('user_info_field', ['shortname' => 'pet', 'name' => 'Pet', 'required' => 0, + 'visible' => 1, 'locked' => 0, 'categoryid' => 1, 'datatype' => 'text']); + + // Add required but invisible custom field. + $DB->insert_record('user_info_field', ['shortname' => 'secretid', 'name' => 'Secret ID', 'required' => 1, + 'visible' => 0, 'locked' => 0, 'categoryid' => 1, 'datatype' => 'text']); + + // Add required but locked custom field. + $DB->insert_record('user_info_field', ['shortname' => 'muggleborn', 'name' => 'Muggle-born', 'required' => 1, + 'visible' => 1, 'locked' => 1, 'categoryid' => 1, 'datatype' => 'checkbox']); + + // Create some student accounts. + $hermione = $this->getDataGenerator()->create_user(); + $harry = $this->getDataGenerator()->create_user(); + $ron = $this->getDataGenerator()->create_user(); + $draco = $this->getDataGenerator()->create_user(); + + // Hermione has all available custom fields filled (of course she has). + profile_save_data((object)['id' => $hermione->id, 'profile_field_house' => 'Gryffindor']); + profile_save_data((object)['id' => $hermione->id, 'profile_field_pet' => 'Crookshanks']); + + // Harry has only the optional field filled. + profile_save_data((object)['id' => $harry->id, 'profile_field_pet' => 'Hedwig']); + + // Draco has only the required field filled. + profile_save_data((object)['id' => $draco->id, 'profile_field_house' => 'Slytherin']); + + // Only students with required fields filled should be considered as fully set up in the default (strict) mode. + $this->assertFalse(user_not_fully_set_up($hermione)); + $this->assertFalse(user_not_fully_set_up($draco)); + $this->assertTrue(user_not_fully_set_up($harry)); + $this->assertTrue(user_not_fully_set_up($ron)); + + // In the lax mode, students do not need to have required fields filled. + $this->assertFalse(user_not_fully_set_up($hermione, false)); + $this->assertFalse(user_not_fully_set_up($draco, false)); + $this->assertFalse(user_not_fully_set_up($harry, false)); + $this->assertFalse(user_not_fully_set_up($ron, false)); + + // Lack of required core field is seen as a problem in either mode. + unset($hermione->email); + $this->assertTrue(user_not_fully_set_up($hermione, true)); + $this->assertTrue(user_not_fully_set_up($hermione, false)); + + // When confirming remote MNet users, we do not have custom fields available. + $roamingharry = mnet_strip_user($harry, ['firstname', 'lastname', 'email']); + $roaminghermione = mnet_strip_user($hermione, ['firstname', 'lastname', 'email']); + + $this->assertTrue(user_not_fully_set_up($roamingharry, true)); + $this->assertFalse(user_not_fully_set_up($roamingharry, false)); + $this->assertTrue(user_not_fully_set_up($roaminghermione, true)); + $this->assertTrue(user_not_fully_set_up($roaminghermione, false)); + } }