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 Bug
    • Status: Closed
    • Priority: Blocker 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
    • Rank:
      33431

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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.
          Hide
          Martin Dougiamas added a comment -

          Petr can you peer-review this please?

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

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

          Show
          Martin Dougiamas added a comment - I've run that SQL on moodle.org and it's fixed the issue. Thanks, Sam!
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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.
          Hide
          Petr Škoda added a comment -

          thanks a lot for the report and proposed patch!

          Show
          Petr Škoda added a comment - thanks a lot for the report and proposed patch!
          Hide
          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
          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
          Sam Hemelryk added a comment -

          Thanks Petr - indeed that was the other route

          Thanks for the fix it is has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Petr - indeed that was the other route Thanks for the fix it is has been integrated now
          Hide
          Martin Dougiamas added a comment -

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

          Show
          Martin Dougiamas added a comment - Yes, I thought NULL was the standard way to have "nothing" now.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: