From 8df850ad6f68f08a9d61f893bc32864b80c64e94 Mon Sep 17 00:00:00 2001 From: =?utf8?q?David=20Mudr=C3=A1k?= Date: Thu, 8 Sep 2016 22:45:33 +0200 Subject: [PATCH] MDL-46946 user: Make missing required custom fields trigger profile edit If there is a required custom field that the user can fill by editing their profile, and that field is missing, the user should be considered as not fully set up. Instead, we want to redirect them to edit their profile first. There are some exceptions when we want to fall back to the previous behaviour and check just the name and email fields. These exceptional cases include checking remote user data in incoming MNet request (no user id, no custom fields supported) and calls to require_login() with redirecting disabled (typically ajax filepicker requests on profile editing page itself). Additional plugins that call the function user_not_fully_set_up() themselves, should perform the strict check in most/typical cases. So the strict mode is enabled by default even if it changes the behaviour slightly. In improbable case of additional plugins relying on the previous behaviour of the function, they can use the $strict parameter and keep performing the lax check. However, I am sure the correct fix in that case will likely be to stop abusing this function. Note that custom fields are not currently transferred during the MNet roaming. So having custom fields configured as required on MNet service provider site (where users can't edit their profiles) is expected to display an error (as the site is considered as misconfigured). --- .../classes/task/check_subscriptions.php | 2 +- auth/ldap/auth.php | 2 +- auth/mnet/auth.php | 2 +- auth/shibboleth/index.php | 2 +- lib/moodlelib.php | 42 ++++++++++-- login/lib.php | 2 +- user/edit.php | 6 +- user/editlib.php | 2 +- user/profile/lib.php | 27 ++++++++ user/tests/profilelib_test.php | 67 +++++++++++++++++++ 10 files changed, 142 insertions(+), 12 deletions(-) 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 fc0fc312a2e..6a5b9d0e6b1 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -2625,8 +2625,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'); } @@ -3141,14 +3150,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 67f2f4a3874..2e7e53e7dd4 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)); + } } -- 2.17.1