Uploaded image for project: '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

          Attachments

            Activity

            Hide
            jval Jarkko Ala-Louvesniemi added a comment -

            Here's the fix.

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

            Thanks for reporting that and providing a fix.

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

            Results for MDL-43200

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

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

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

            Results for MDL-43200

            • Remote repository: git://github.com/apsdehal/moodle.git
            Show
            cibot 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
            skodak Petr Skoda added a comment -

            thanks a lot, submitting for integration

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

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

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            samhemelryk 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
            samhemelryk 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
            jval 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
            jval 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
            dobedobedoh 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
            dobedobedoh 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
            skodak 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
            samhemelryk Sam Hemelryk added a comment -

            Tests pulled thanks Petr - back to testing.

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

            Unit tests passing as expected.

            Thanks all!

            Show
            dobedobedoh Andrew Nicols added a comment - Unit tests passing as expected. Thanks all!
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  10/Mar/14