Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30623

Guest are continually asked for an enrolment password when trying to enter courses they should have access to

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2.1
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      1/ use DB editor and set some enrol->password fields from guest plugin to NULL
      2/ make sure user can not access
      3/ upgrade and verify guest access is now possible
      4/ try upgrade from 1.9

      Show
      1/ use DB editor and set some enrol->password fields from guest plugin to NULL 2/ make sure user can not access 3/ upgrade and verify guest access is now possible 4/ try upgrade from 1.9
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w50_MDL-30623_m23_guest

      Description

      MDL-30148 added 3 tighter checks on $instance->password in enrol/guest/lib.php

      eg

      -        if (empty($instance->password)) {
      +        if ($instance->password === '') {

      Since the default value of this field in the database is actually NULL this now breaks sites like moodle.org with a lot of guest access by forcing everyone to type a guest password.

        Gliffy Diagrams

          Issue Links

            Activity

            dougiamas Martin Dougiamas created issue -
            dougiamas Martin Dougiamas made changes -
            Field Original Value New Value
            Link This issue is a regression caused by MDL-30148 [ MDL-30148 ]
            dougiamas Martin Dougiamas made changes -
            Priority Minor [ 4 ] Blocker [ 1 ]
            dougiamas Martin Dougiamas made changes -
            Labels triaged
            dougiamas Martin Dougiamas made changes -
            Fix Version/s STABLE Sprint 16 [ 11350 ]
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Just noting behavior in master currently:

            In a new installation with default settings I create a new course with default settings. Upon inspecting the database I can see that the enrollment instance for guest access (currently disabled) has password=null.
            I browse to Course administration > Users > Enrollment methods and use the eye to enable to plugin. Everything stays as it is.
            I browse to Course administration > Edit settings. Without changing things I click save changes. guest enrol password in the DB changes from NULL to ''

            Looks like we need to decide whether the enrol_guest support null password or whether it should default passwords to ''.

            Show
            samhemelryk Sam Hemelryk added a comment - Just noting behavior in master currently: In a new installation with default settings I create a new course with default settings. Upon inspecting the database I can see that the enrollment instance for guest access (currently disabled) has password=null. I browse to Course administration > Users > Enrollment methods and use the eye to enable to plugin. Everything stays as it is. I browse to Course administration > Edit settings. Without changing things I click save changes. guest enrol password in the DB changes from NULL to '' Looks like we need to decide whether the enrol_guest support null password or whether it should default passwords to ''.
            Hide
            samhemelryk Sam Hemelryk added a comment - - edited

            Having looked at the code I think we need to allow the code to support null passwords as well as '' (given the field supports null).
            I also think that default instances should be added with a password of '' as well (makes it clear password is being used).

            I've created a pair of patches for 2.2 and master to do this:
            Git repo: git://github.com/samhemelryk/moodle.git

            2.2 branch: wip-MDL-30623-m22
            2.2 diff: https://github.com/samhemelryk/moodle/compare/MOODLE_22_STABLE...wip-MDL-30623-m22

            master branch: wip-MDL-30623-m23
            master diff: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-30623-m23

            Up for your consideration Petr

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - - edited Having looked at the code I think we need to allow the code to support null passwords as well as '' (given the field supports null). I also think that default instances should be added with a password of '' as well (makes it clear password is being used). I've created a pair of patches for 2.2 and master to do this: Git repo: git://github.com/samhemelryk/moodle.git 2.2 branch: wip- MDL-30623 -m22 2.2 diff: https://github.com/samhemelryk/moodle/compare/MOODLE_22_STABLE...wip-MDL-30623-m22 master branch: wip- MDL-30623 -m23 master diff: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-30623-m23 Up for your consideration Petr Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Also noting that a quick fix SQL statement for moodle.org in the mean time would be:

            UPDATE enrol SET password = '' WHERE password IS NULL AND enrol='guest';

            That should update all guest enrollment plugins with a null password to a '' password which passes the !== and === '' checks.

            Show
            samhemelryk Sam Hemelryk added a comment - Also noting that a quick fix SQL statement for moodle.org in the mean time would be: UPDATE enrol SET password = '' WHERE password IS NULL AND enrol='guest'; That should update all guest enrollment plugins with a null password to a '' password which passes the !== and === '' checks.
            dougiamas Martin Dougiamas made changes -
            Assignee moodle.com [ moodle.com ] Sam Hemelryk [ samhemelryk ]
            Hide
            dougiamas Martin Dougiamas added a comment -

            Petr can you peer-review this please?

            Show
            dougiamas Martin Dougiamas added a comment - Petr can you peer-review this please?
            Hide
            dougiamas Martin Dougiamas added a comment -

            I've run that SQL on moodle.org and it's fixed the issue. Thanks, Sam!

            Show
            dougiamas Martin Dougiamas added a comment - I've run that SQL on moodle.org and it's fixed the issue. Thanks, Sam!
            salvetore Michael de Raadt made changes -
            Labels triaged sprinting triaged
            skodak Petr Skoda made changes -
            Assignee Sam Hemelryk [ samhemelryk ] Petr Škoda (skodak) [ skodak ]
            Hide
            skodak Petr Skoda added a comment -

            Hi, sorry for the trouble. I have created a bit different fix that makes sure that all guest instances have empty password instead of null and added necessary upgrade code.

            Show
            skodak Petr Skoda added a comment - Hi, sorry for the trouble. I have created a bit different fix that makes sure that all guest instances have empty password instead of null and added necessary upgrade code.
            Hide
            skodak Petr Skoda added a comment -

            To integrators: I have intentionally excluded the install.php from master because it is used only during upgrade from 1.9 see MDL-30610 for more details.

            Show
            skodak Petr Skoda added a comment - To integrators: I have intentionally excluded the install.php from master because it is used only during upgrade from 1.9 see MDL-30610 for more details.
            skodak Petr Skoda made changes -
            Status Open [ 1 ] Waiting for integration review [ 10010 ]
            Pull Master Diff URL https://github.com/skodak/moodle/compare/master...w50_MDL-30623_m23_guest
            Pull Master Branch w50_MDL-30623_m23_guest
            Pull from Repository git://github.com/skodak/moodle.git
            Fix Version/s 2.2.1 [ 11456 ]
            Fix Version/s 2.3 [ 10657 ]
            Fix Version/s STABLE Sprint 16 [ 11350 ]
            Testing Instructions 1/ use DB editor and set some enrol->password fields from guest plugin to NULL
            2/ make sure user can not access
            3/ upgrade and verify guest access is now possible
            4/ try upgrade from 1.9
            Pull 2.2 Diff URL https://github.com/skodak/moodle/compare/MOODLE_22_STABLE...w50_MDL-30623_m22_guest
            Pull 2.2 Branch w50_MDL-30623_m22_guest
            Hide
            skodak Petr Skoda added a comment -

            thanks a lot for the report and proposed patch!

            Show
            skodak Petr Skoda added a comment - thanks a lot for the report and proposed patch!
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator samhemelryk
            Currently in integration Yes [ 10041 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            So... now... we are storing '' (empty strings) with meaning "no password". If so, and we aren't accepting nulls there anymore... wouldn't it be better to change the column specs to avoid accepting NULLs anymore?

            Also, being realistic, I think that NULL is a far better value for the "no password" meaning, in fact that seemed to be your 1st idea.

            But well, if we want to have one more "silly" empty-string field, oki, np. But then keep out nulls completely.

            Ciao

            PS: About the missing install.php... oki, np we can consider it ok.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - So... now... we are storing '' (empty strings) with meaning "no password". If so, and we aren't accepting nulls there anymore... wouldn't it be better to change the column specs to avoid accepting NULLs anymore? Also, being realistic, I think that NULL is a far better value for the "no password" meaning, in fact that seemed to be your 1st idea. But well, if we want to have one more "silly" empty-string field, oki, np. But then keep out nulls completely. Ciao PS: About the missing install.php... oki, np we can consider it ok.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Petr - indeed that was the other route

            Thanks for the fix it is has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Petr - indeed that was the other route Thanks for the fix it is has been integrated now
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Fix Version/s 2.3 [ 10657 ]
            Hide
            dougiamas Martin Dougiamas added a comment -

            Yes, I thought NULL was the standard way to have "nothing" now.

            Show
            dougiamas Martin Dougiamas added a comment - Yes, I thought NULL was the standard way to have "nothing" now.
            gerry Gerard Caulfield made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester gerry
            Hide
            skodak Petr Skoda added a comment -

            hmm, you are right, NULLs might be better choice, I did not think about that much, I just tried to make it consistent

            oh, it is already integrated!

            Show
            skodak Petr Skoda added a comment - hmm, you are right, NULLs might be better choice, I did not think about that much, I just tried to make it consistent oh, it is already integrated!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yeah, we commented exactly at the same moment, grrr.

            So there are 2 (perhaps 3) solutions:

            1) or we transform it all to use NULLs
            2) or we agree about to use empties, so we transform the DB to stop accepting nulls and defaulting to empty (same upgrade step would be ok IMO).
            3) or we do nothing

            For your consideration, I'm happy with any of 1) or 2).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yeah, we commented exactly at the same moment, grrr. So there are 2 (perhaps 3) solutions: 1) or we transform it all to use NULLs 2) or we agree about to use empties, so we transform the DB to stop accepting nulls and defaulting to empty (same upgrade step would be ok IMO). 3) or we do nothing For your consideration, I'm happy with any of 1) or 2). Ciao
            Hide
            skodak Petr Skoda added a comment -

            my patch did roughly 2/, the logic should be fine, I could have implemented it as NULLS, but it was already using empties in 2.2

            Show
            skodak Petr Skoda added a comment - my patch did roughly 2/, the logic should be fine, I could have implemented it as NULLS, but it was already using empties in 2.2
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Wow, I must be losing explaining skills,

            I saw your upgrade script and the SQL update on it, obviouly. What I'm saying, is that we should ALTER the table to stop accepting nulls (right now it continues accepting them, you just modified the contents).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Wow, I must be losing explaining skills, I saw your upgrade script and the SQL update on it, obviouly. What I'm saying, is that we should ALTER the table to stop accepting nulls (right now it continues accepting them, you just modified the contents).
            Hide
            gerry Gerard Caulfield added a comment - - edited

            I'm a noob here so I'm going to be quite terse in case I've made a false assumption in my test process. This is how I interpreted the test instructions and performed them:
            1)

            • git checkout -b preGuestFix da3494a8c42a1bf08fa921f4a9cd73fd4c1df443; # The commit just before the fix was applied to moodle22stable integration on git.moodle.org/integration.git
            • created a blank database moodle_int_moodle22stable the with appropriate settings.
            • ran the following command to set everything up:

              php ./admin/cli/install.php --lang=en --wwwroot="http://<mydomain>/moodle/int/moodle22stable" --dataroot="<dataroot>/int/moodle22stable" --dbtype=mysqli \
                          --dbhost="localhost" --dbname="moodle_int_moodle22stable" --dbuser="<mydbuser>" --dbpass="<mydbpassword>" --fullname="Moodle Integration version 2.2" \
                          --shortname="moodleInt22" --adminuser="<adminuser>" --adminpass="<adminpassword>" --non-interactive --agree-license --allow-unstable

            • created a test course with guest access and no course password.
            • Logged out and logged in as guest on the course
            • Logged back into admin and out again, to clear any guest session.
            • Change the mdl_enrol.password field for guest to NULL

            2)

            • Tried to "Login as a Guest" and recieved a password box
            • cat enrol/guest/version.php | grep "plugin->version"; # outputs: $plugin->version = 2011112900; // The current plugin version (Date: YYYYMMDDXX)

            3)

            • git checkout MOODLE_22_STABLE # git rev-parse HEAD returns 4dbdc834ffc4adf09c4ae104b9024f61388a2c53
              Just to prove that the patch is applied:
            • git remote add skodak git://github.com/skodak/moodle.git
            • git fetch skodak
            • git merge skodak/w50_MDL-30623_m22_guest # returns "Already up-to-date."
            • cat enrol/guest/version.php | grep "plugin->version"; # outputs: $plugin->version = 2011112901; // The current plugin version (Date: YYYYMMDDXX)
            • logged into admin, went to home and performed upgrade
            • logged out.
            • Clicked on "Login as a Guest" for the course
            • It worked, WOOT!

            4)

            • Setup 1.9 instance with test course that has guess access (without a key)
            • Login as guest and enter the course as a guest.
            • Login as admin to clear the guest session.
            • git fetch integration
            • git checkout -b MOODLE_22_STABLE integration/MOODLE_22_STABLE
            • Run upgrade
            • Sign out of admin
            • Log into test course as guest

            TEST PASSED.

            Thanks to Rajesh, Jason and Aparup for your help with this.

            Show
            gerry Gerard Caulfield added a comment - - edited I'm a noob here so I'm going to be quite terse in case I've made a false assumption in my test process. This is how I interpreted the test instructions and performed them: 1) git checkout -b preGuestFix da3494a8c42a1bf08fa921f4a9cd73fd4c1df443; # The commit just before the fix was applied to moodle22stable integration on git.moodle.org/integration.git created a blank database moodle_int_moodle22stable the with appropriate settings. ran the following command to set everything up: php ./admin/cli/install.php --lang=en --wwwroot="http://<mydomain>/moodle/int/moodle22stable" --dataroot="<dataroot>/int/moodle22stable" --dbtype=mysqli \ --dbhost="localhost" --dbname="moodle_int_moodle22stable" --dbuser="<mydbuser>" --dbpass="<mydbpassword>" --fullname="Moodle Integration version 2.2" \ --shortname="moodleInt22" --adminuser="<adminuser>" --adminpass="<adminpassword>" --non-interactive --agree-license --allow-unstable created a test course with guest access and no course password. Logged out and logged in as guest on the course Logged back into admin and out again, to clear any guest session. Change the mdl_enrol.password field for guest to NULL 2) Tried to "Login as a Guest" and recieved a password box cat enrol/guest/version.php | grep "plugin->version"; # outputs: $plugin->version = 2011112900; // The current plugin version (Date: YYYYMMDDXX) 3) git checkout MOODLE_22_STABLE # git rev-parse HEAD returns 4dbdc834ffc4adf09c4ae104b9024f61388a2c53 Just to prove that the patch is applied: git remote add skodak git://github.com/skodak/moodle.git git fetch skodak git merge skodak/w50_ MDL-30623 _m22_guest # returns "Already up-to-date." cat enrol/guest/version.php | grep "plugin->version"; # outputs: $plugin->version = 2011112901; // The current plugin version (Date: YYYYMMDDXX) logged into admin, went to home and performed upgrade logged out. Clicked on "Login as a Guest" for the course It worked, WOOT! 4) Setup 1.9 instance with test course that has guess access (without a key) Login as guest and enter the course as a guest. Login as admin to clear the guest session. git fetch integration git checkout -b MOODLE_22_STABLE integration/MOODLE_22_STABLE Run upgrade Sign out of admin Log into test course as guest TEST PASSED. Thanks to Rajesh, Jason and Aparup for your help with this.
            gerry Gerard Caulfield made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            skodak Petr Skoda added a comment -

            Eloy: that table is general, each plugin defines what each filed means - changing to NOT NULL would break BC which is not acceptable for stable, this was one of the reasons why I fixed the trouble with '' because in current 2.2 stable it is expecting empty strings, in older 2.0 and 2.1 we have a mixture there and use sloppy empty(). And no, it is not your fault, I am thinking very slowly these days...

            Show
            skodak Petr Skoda added a comment - Eloy: that table is general, each plugin defines what each filed means - changing to NOT NULL would break BC which is not acceptable for stable, this was one of the reasons why I fixed the trouble with '' because in current 2.2 stable it is expecting empty strings, in older 2.0 and 2.1 we have a mixture there and use sloppy empty(). And no, it is not your fault, I am thinking very slowly these days...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, you did it!

            Now your code is part of the best weeklies released ever, many thanks!

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, you did it! Now your code is part of the best weeklies released ever, many thanks! Closing, ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 09/Dec/11

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Jan/12