Moodle
  1. Moodle
  2. MDL-32204

enrol/mnet creates users with the same auth as on home server

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: MNet
    • Labels:
    • Testing Instructions:
      Hide

      Testing difficulty: HARD (requires MNet environment)

      (I believe testing could be skipped here as the patch is trivial and the reporter obviously tested his solution)

      1. Set up MNet between two Moodle sites and enable remote enrolment via MNet in the site with courses
      2. At the site with users, enrol some local user to the a course on the remote site. Make sure that the user does not have a roaming profile at the remote site yet.
      3. TEST: Check that a new user record was created at the remote site and the auth method is set to 'mnet' for them.

      Show
      Testing difficulty: HARD (requires MNet environment) (I believe testing could be skipped here as the patch is trivial and the reporter obviously tested his solution) 1. Set up MNet between two Moodle sites and enable remote enrolment via MNet in the site with courses 2. At the site with users, enrol some local user to the a course on the remote site. Make sure that the user does not have a roaming profile at the remote site yet. 3. TEST: Check that a new user record was created at the remote site and the auth method is set to 'mnet' for them.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32204-mnet-auth

      Description

      Remote enrolment of users through MNet copies several properties of user account from home server. This includes user name, mailing preferences and among some others the authentication method. Let me repeat: the 'auth' field is copied from original Moodle site to remote Moodle site.
      This has a side-effect for users automatically subscribed to a news forum on a remote site, when they have never visited the site (their accounts are not fully set up). At start their remote profiles have 'auth' field set the same as on their home server, like 'local' or 'ldap'. The next day the cron job runs and tries to update user accounts, and then users which could not be authenticated have their 'auth' field reset to 'nologin'. And then no more news are sent, even if users should be in fact subscribed (since they were remotely enrolled in a course).
      Users that visited the remote site have 'auth' field in their profiles updated when they are fully set up. This field becomes 'mnet' since then, which means forum news are being sent. No visit - no news. Bad Thing (tm).

      The solution may be simpler than the above description sounds. Please consider adding one line to MOODLE/enrol/mnet/enrol.php in function enrol_user():

      /* MOODLE/enrol/mnet/enrol.php */
      ...
          public function enrol_user(array $userdata, $courseid) {
      ...
      /* Around line 143 in Moodle 2.2 */
              if ($user === false) {
                  // here we could check the setting if the enrol_mnet is allowed to auto-register
                  // users {@link http://tracker.moodle.org/browse/MDL-21327}
                  $user = mnet_strip_user((object)$userdata, mnet_fields_to_import($client));
                  $user->mnethostid = $client->id;
      /* ADD! */  $user->auth = 'mnet';  /* THIS ! */
                  try {
                      $user->id = $DB->insert_record('user', $user);
                  } catch (Exception $e) {
                      throw new mnet_server_exception(5011, 'couldnotcreateuser', 'enrol_mnet');
                  }
              }

      This way 'auth' field reflects the fact that a user has been enrolled remotely, and does not contain invalid authentication method (e.g. 'ldap' or other), copied from home site.

      As an additional measure, the 'auth' field might be not transferred at all when enrolling users remotely. In file MOODLE/mnet/lib.php there is a '$forced' array defined, which lists fields that should be transferred. Maybe 'auth' should be removed from '$forced' as well? Especially if $user->auth could be set anyway in enrol_user() as described earlier?

      /* MOODLE/mnet/lib.php */
      ...
      function mnet_profile_field_options() {
      ...
      /* Around line 595 in Moodle 2.2 */
          // these are the ones that user_not_fully_set_up will complain about
          // and also special case ones
          $forced = array(
              'username',
              'email',
              'firstname',
              'lastname',
              'auth',  /* How about removing this one? */
              'wwwroot',
              'session.gc_lifetime',
              '_mnet_userpicture_timemodified',
              '_mnet_userpicture_mimetype',
          );

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting that.

            Show
            Michael de Raadt added a comment - Thanks for reporting that.
            Hide
            David Mudrak added a comment -

            Very well spotted and analysed! You are right, of course. The similar thing is already done in auth_plugin_mnet::confirm_mnet_session() as you noticed. We just forgot to the same step here.

            With regards to not sending the field at all. It makes perfect sense at the first sight. However, I would rather not modify this as it is part of kinda "MNet API" (if something like that actually existed) and chances are that someone relies on that in their custom code. Also, it could break backwards compatibility with older MNet peers that might still expect the field. And finally, Moodle is not the only app that supports MNet so it might have further consequences.

            So I'm submitting your solution for integration. Thanks again Wiktor.

            Show
            David Mudrak added a comment - Very well spotted and analysed! You are right, of course. The similar thing is already done in auth_plugin_mnet::confirm_mnet_session() as you noticed. We just forgot to the same step here. With regards to not sending the field at all. It makes perfect sense at the first sight. However, I would rather not modify this as it is part of kinda "MNet API" (if something like that actually existed) and chances are that someone relies on that in their custom code. Also, it could break backwards compatibility with older MNet peers that might still expect the field. And finally, Moodle is not the only app that supports MNet so it might have further consequences. So I'm submitting your solution for integration. Thanks again Wiktor.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Sam Hemelryk added a comment -

            Thanks guys this has been integrated now. This patch really is straight forward and I agree that it perhaps doesn't need to testing, however I'll leave it in the waiting for testing pile and we'll see if anyone is brave enough

            Show
            Sam Hemelryk added a comment - Thanks guys this has been integrated now. This patch really is straight forward and I agree that it perhaps doesn't need to testing, however I'll leave it in the waiting for testing pile and we'll see if anyone is brave enough
            Hide
            Dan Poltawski added a comment -

            I'll attempt to test this on master, since it seems like a pretty useful exercise anyway.

            Show
            Dan Poltawski added a comment - I'll attempt to test this on master, since it seems like a pretty useful exercise anyway.
            Hide
            Dan Poltawski added a comment -

            Hurray - it works!

            Show
            Dan Poltawski added a comment - Hurray - it works!
            Hide
            Dan Poltawski added a comment -

            (However it was a useful exercise and I found MDL-33182 while testing)

            Show
            Dan Poltawski added a comment - (However it was a useful exercise and I found MDL-33182 while testing)
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!
            Hide
            David Monllaó added a comment -

            Hi,

            Related with this: http://tracker.moodle.org/browse/MDL-31823 with the same scenario (creating users through a remote enrolments) the users are created with mdl_user->confirmed = 0 and cron deletes the no-confirmed users

            http://tracker.moodle.org/browse/MDL-31823 pull branches rebased against latest MOODLE_21_STABLE, MOODLE_22_STABLE and HEAD (including these changes)

            Show
            David Monllaó added a comment - Hi, Related with this: http://tracker.moodle.org/browse/MDL-31823 with the same scenario (creating users through a remote enrolments) the users are created with mdl_user->confirmed = 0 and cron deletes the no-confirmed users http://tracker.moodle.org/browse/MDL-31823 pull branches rebased against latest MOODLE_21_STABLE, MOODLE_22_STABLE and HEAD (including these changes)

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: