Moodle
  1. Moodle
  2. MDL-43200

Web service call enrol_manual_enrol_users enrols users to wrong courses if manual enrolments has/have been disabled in the course(s)

    Details

      Description

      This very ugly bug is caused by not resetting $instance before usage in enrol_users() from enrol/manual/externallib.php

      When manual enrolments is disabled in the course where users are being enrolled to, the previous value of $instance is used which means user is then enrolled to a wrong course.

      Here's the fix:

      --- externallib.php.dist	2013-11-29 06:52:44.000000000 +0200
      +++ externallib.php	2013-12-04 15:01:55.651222101 +0200
      @@ -109,6 +109,7 @@
                   }
       
                   // Check manual enrolment plugin instance is enabled/exist.
      +            $instance = null;
                   $enrolinstances = enrol_get_instances($enrolment['courseid'], true);
                   foreach ($enrolinstances as $courseenrolinstance) {
                     if ($courseenrolinstance->enrol == "manual") {
      

        Gliffy Diagrams

        1. moodle-enrol.patch
          0.5 kB
          Jarkko Ala-Louvesniemi

          Activity

          Hide
          Jarkko Ala-Louvesniemi added a comment -

          Here's the fix.

          Show
          Jarkko Ala-Louvesniemi added a comment - Here's the fix.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that and providing a fix.

          Show
          Michael de Raadt added a comment - Thanks for reporting that and providing a fix.
          Hide
          CiBoT added a comment -

          Results for MDL-43200

          • Error: the repository field is empty. Nothing was checked.
          Show
          CiBoT added a comment - Results for MDL-43200 Error: the repository field is empty. Nothing was checked.
          Hide
          Petr Skoda added a comment -

          Nice catch, could you please add branches for 2.5, 2.6 and master?

          Show
          Petr Skoda added a comment - Nice catch, could you please add branches for 2.5, 2.6 and master?
          Hide
          CiBoT added a comment -

          Results for MDL-43200

          • Remote repository: git://github.com/apsdehal/moodle.git
          Show
          CiBoT added a comment - Results for MDL-43200 Remote repository: git://github.com/apsdehal/moodle.git Remote branch MDL-43200 _24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1112 Warning: The MDL-43200 _24 branch at git://github.com/apsdehal/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1112/artifact/work/smurf.html Remote branch MDL-43200 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1113 Error: Unable to fetch information from MDL-43200 _25 branch at git://github.com/apsdehal/moodle.git. Remote branch MDL-43200 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1114 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1114/artifact/work/smurf.html Remote branch MDL-43200 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1115 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1115/artifact/work/smurf.html
          Hide
          Petr Skoda added a comment -

          thanks a lot, submitting for integration

          Show
          Petr Skoda added a comment - thanks a lot, submitting for integration
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Sam Hemelryk added a comment -

          Thanks this has been integrated now.

          Please note that it wasn't backported to 2.4, that branch is only receiving security fixes now.

          Show
          Sam Hemelryk added a comment - Thanks this has been integrated now. Please note that it wasn't backported to 2.4, that branch is only receiving security fixes now.
          Hide
          Jarkko Ala-Louvesniemi added a comment -

          If your web service client is enrolling teachers then write permissions can be given to wrong people which sounds like a security issue...

          Show
          Jarkko Ala-Louvesniemi added a comment - If your web service client is enrolling teachers then write permissions can be given to wrong people which sounds like a security issue...
          Hide
          Andrew Nicols added a comment -

          I'm unable to test this as the instructions don't give information on how to test properly.

          Petr is writing some unit tests for this.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - I'm unable to test this as the instructions don't give information on how to test properly. Petr is writing some unit tests for this. Cheers, Andrew
          Show
          Petr Skoda added a comment - Sorry for the trouble, the patches with new unittests are in: https://github.com/skodak/moodle/commits/int27_MDL-43200 https://github.com/skodak/moodle/commits/int26_MDL-43200 https://github.com/skodak/moodle/commits/int25_MDL-43200
          Hide
          Sam Hemelryk added a comment -

          Tests pulled thanks Petr - back to testing.

          Show
          Sam Hemelryk added a comment - Tests pulled thanks Petr - back to testing.
          Hide
          Andrew Nicols added a comment -

          Unit tests passing as expected.

          Thanks all!

          Show
          Andrew Nicols added a comment - Unit tests passing as expected. Thanks all!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Fetch your remotes, prune them,
          clean your integrated branches and say "Ahem".

          Rebase your ongoing stuff, keep conflicts away
          don't forget to test the code and we'll love you again.

          Thanks, closing!

          Show
          Eloy Lafuente (stronk7) added a comment - Fetch your remotes, prune them, clean your integrated branches and say "Ahem". Rebase your ongoing stuff, keep conflicts away don't forget to test the code and we'll love you again. Thanks, closing!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: