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).

admin/tool/monitor/classes/task/check_subscriptions.php

@@ -199,7 +199,7 @@ public function execute() {
      */
     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];
     }

auth/ldap/auth.php

@@ -1771,7 +1771,7 @@ function ntlmsso_finish() {
             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)) {

auth/mnet/auth.php

@@ -263,7 +263,7 @@ function confirm_mnet_session($token, $remotepeer) {
             exit;
         }
 
-        if (user_not_fully_set_up($remoteuser)) {
+        if (user_not_fully_set_up($remoteuser, false)) {
             print_error('notenoughidpinfo', 'mnet');
             exit;
         }

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
 

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;
 }
 
 /**

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.
 

user/edit.php

@@ -72,9 +72,11 @@
 // 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);
     }

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);
         }

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;
+}

user/tests/profilelib_test.php

@@ -143,4 +143,71 @@ public function test_profile_view() {
 
     }
 
+    /**
+     * 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));
+    }
 }