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
    • Rank:
      38963

      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',
          );
      

        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: