MDL-46946 user: Make missing required custom fields trigger profile edit
authorDavid Mudrák <david@moodle.com>
Thu, 8 Sep 2016 20:45:33 +0000 (22:45 +0200)
committerDavid Mudrák <david@moodle.com>
Wed, 21 Sep 2016 15:46:30 +0000 (17:46 +0200)
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).

admin/tool/monitor/classes/task/check_subscriptions.php
auth/ldap/auth.php
auth/mnet/auth.php
auth/shibboleth/index.php
lib/moodlelib.php
login/lib.php
user/edit.php
user/editlib.php
user/profile/lib.php
user/tests/profilelib_test.php

index 8262f10..59125df 100644 (file)
@@ -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];
     }
index 75b0d84..f80d6b3 100644 (file)
@@ -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)) {
index 11ddba2..6f1cf0f 100644 (file)
@@ -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;
         }
index 4e5c92a..a8cbd03 100644 (file)
@@ -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.'&amp;course='.SITEID;
                 // We don't delete $SESSION->wantsurl yet, so we get there later
 
index fc0fc31..6a5b9d0 100644 (file)
@@ -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;
 }
 
 /**
index 07c735f..3e522ff 100644 (file)
@@ -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.
 
index a3341f0..044522a 100644 (file)
@@ -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);
     }
index 67f2f4a..2e7e53e 100644 (file)
@@ -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);
         }
index d38bbe8..f712a97 100644 (file)
@@ -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;
+}
index 6e24e0e..0c4c599 100644 (file)
@@ -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));
+    }
 }