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

CSV User Import: Enforce validation of username field

    Details

    • Testing Instructions:
      Hide
      1. Login as admin and goto user csv import
      2. create a csv file with following content

        username,password,firstname,lastname,email
        dduc@!#k,test,donald,duck,ddudddk@test.com
        mmous{}/ze,test,mickey,mouse,mmodduse@test.com
        "ba  mbi",test,bambi,bambi,bambi@test.com
        elmo,test,elmo,elmo,elmo@test.com
        

      3. Disable the setting extendedusernamechars
      4. Try importing the csv
      5. make sure you get invalid username error for first three entries
      6. enable the setting extendedusernamechars
      7. try importing again and make sure you get error only for the third entry
      8. switch 'Standardise usernames' to 'No'.
      9. go ahead and finish the import, make sure there are no further errors.
      Show
      Login as admin and goto user csv import create a csv file with following content username,password,firstname,lastname,email dduc@!#k,test,donald,duck,ddudddk@test.com mmous{}/ze,test,mickey,mouse,mmodduse@test.com "ba mbi",test,bambi,bambi,bambi@test.com elmo,test,elmo,elmo,elmo@test.com Disable the setting extendedusernamechars Try importing the csv make sure you get invalid username error for first three entries enable the setting extendedusernamechars try importing again and make sure you get error only for the third entry switch 'Standardise usernames' to 'No'. go ahead and finish the import, make sure there are no further errors.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-16730-master

      Description

      Around lines 123-142 of the /user/editadvanced_form.php file, there are a series of validation checks for the username. I think these should be moved to a function called something like valid_username that returns either true or the error (where the validation failed). In this way, we could easily enforce the validation of usernames when they come in via a CSV file (/admin/uploaduser.php). Currently we verify that the username contains no whitespace, is lowercase, and is alphanumeric allowing also dashes , slashes (/ and ), periods (.) and parentheses (). I'm not sure how much of this is intentional. We may want to improve and limit it more closely to alphanums. I can see application for the dashes and periods but wonder about slashes and parentheses.

      I think this is a matter of consistency. I came across a user who was putting underscores in the usernames via CSV files and this is not allowed when manually adding a user. In general, I would like to see that we are performing the same validation on CSV data as we do for manually added data. Peace - Anthony

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Agree, we should perform the same validations, both in manual and upload user creation.

              Assigning to Mathieu... thanks for the report! Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Agree, we should perform the same validations, both in manual and upload user creation. Assigning to Mathieu... thanks for the report! Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Raising to Major and addressing to 1.9.3

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Raising to Major and addressing to 1.9.3
              Hide
              aborrow Anthony Borrow added a comment -

              In http://moodle.org/mod/forum/discuss.php?d=108438#p476539 Marc Grober is reporting that this is no longer an issue. I just tested with the latest 1.9+ and it seems to be resolved. I thought it might have been related to MDL-15823 but that was fixed before I created this issue so I'm not sure what might have changed but I confirmed what Marc said and it seems that the underscore is now being removed. I'm not sure if this automatic removal and proceeding is necessarily desirable or if it would be more beneficial to have it give a warning or error message that the username in the CSV file contains an invalid character (and possibly even identify that character as '_'. Peace - Anthony

              Show
              aborrow Anthony Borrow added a comment - In http://moodle.org/mod/forum/discuss.php?d=108438#p476539 Marc Grober is reporting that this is no longer an issue. I just tested with the latest 1.9+ and it seems to be resolved. I thought it might have been related to MDL-15823 but that was fixed before I created this issue so I'm not sure what might have changed but I confirmed what Marc said and it seems that the underscore is now being removed. I'm not sure if this automatic removal and proceeding is necessarily desirable or if it would be more beneficial to have it give a warning or error message that the username in the CSV file contains an invalid character (and possibly even identify that character as '_'. Peace - Anthony
              Hide
              net-buoy Marc Grober added a comment -

              The forums discussion is labyrinthine as usual, but I could not locate any explanation as to why the underscore is regarded as an illegal character in the username (i.e. the discussion seemed to conclude that this was not a mysql issue.

              If it is not a mysql issue, why is the underscore a suspect character?
              Was a change made between 1.8.2 and 1.9.2+ to bar using underscore in usernames via bulk addition of users? (the notes above indicate that was not supposed to be fixed until 1.9.3 and does indicate a fix having been implemented)
              If this has not been "fixed" what is q.9.3 then doing (it appears to simply be deleting the underscore and conflating the balance of the username, though the bulk preview indicates no problem and the users appear to be added without problem?
              Will the site http policies switch to allow characters resolve this (i.e. allow underscores) or not?

              Show
              net-buoy Marc Grober added a comment - The forums discussion is labyrinthine as usual, but I could not locate any explanation as to why the underscore is regarded as an illegal character in the username (i.e. the discussion seemed to conclude that this was not a mysql issue. If it is not a mysql issue, why is the underscore a suspect character? Was a change made between 1.8.2 and 1.9.2+ to bar using underscore in usernames via bulk addition of users? (the notes above indicate that was not supposed to be fixed until 1.9.3 and does indicate a fix having been implemented) If this has not been "fixed" what is q.9.3 then doing (it appears to simply be deleting the underscore and conflating the balance of the username, though the bulk preview indicates no problem and the users appear to be added without problem? Will the site http policies switch to allow characters resolve this (i.e. allow underscores) or not?
              Hide
              aborrow Anthony Borrow added a comment -

              Marc - I understand your frustration and the reason I filed this as a bug is to avoid this problem for others. If you manually create a user and attempt to have an underscore in the username it will immediately prompt you that the username "Can only contain alphabetical letters or numbers". I have made an attempt to document this better. This seems to be an old restriction and we may be able to allow the underscore. I consider the fact that the upload interface allowed it to be a bug that we have fixed; however, there is nothing preventing us from requesting an improvement or new feature that allows it. I'll create that issue now and add you as a watcher. Peace - Anthony

              Show
              aborrow Anthony Borrow added a comment - Marc - I understand your frustration and the reason I filed this as a bug is to avoid this problem for others. If you manually create a user and attempt to have an underscore in the username it will immediately prompt you that the username "Can only contain alphabetical letters or numbers". I have made an attempt to document this better. This seems to be an old restriction and we may be able to allow the underscore. I consider the fact that the upload interface allowed it to be a bug that we have fixed; however, there is nothing preventing us from requesting an improvement or new feature that allows it. I'll create that issue now and add you as a watcher. Peace - Anthony
              Hide
              aborrow Anthony Borrow added a comment -

              If we allow underscores in the username, then we need to make sure that the underscore character is not being stripped during the upload process as it currently is. Peace - Anthony

              Show
              aborrow Anthony Borrow added a comment - If we allow underscores in the username, then we need to make sure that the underscore character is not being stripped during the upload process as it currently is. Peace - Anthony
              Hide
              aborrow Anthony Borrow added a comment -

              It seems that the username is being stripped of the underscore character when importing using CSV with the code:

              if (empty($CFG->extendedusernamechars))

              { $user->username = eregi_replace('[^(-\.[:alnum:])]', '', $user->username); }

              I think we should check and see if the username changes, if so then we should notify the administrator attempting to do the import that that username is not valid and skip that record. Otherwise, what gets displayed as being imported is not in fact what is actually the username. Peace - Anthony

              Show
              aborrow Anthony Borrow added a comment - It seems that the username is being stripped of the underscore character when importing using CSV with the code: if (empty($CFG->extendedusernamechars)) { $user->username = eregi_replace('[^(-\.[:alnum:])]', '', $user->username); } I think we should check and see if the username changes, if so then we should notify the administrator attempting to do the import that that username is not valid and skip that record. Otherwise, what gets displayed as being imported is not in fact what is actually the username. Peace - Anthony
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Assigning this also to Jerome, in relation with MDL-16919

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Assigning this also to Jerome, in relation with MDL-16919
              Hide
              aborrow Anthony Borrow added a comment -

              Tim - After chatting with Jerome, it was suggested that these (MDL-16919 and MDL-16730) get assigned to you in particular because they deal with regular expressions which make me want to run and hide. If you don't have time feel free to pass them off to someone else. Peace - Anthony

              Show
              aborrow Anthony Borrow added a comment - Tim - After chatting with Jerome, it was suggested that these ( MDL-16919 and MDL-16730 ) get assigned to you in particular because they deal with regular expressions which make me want to run and hide. If you don't have time feel free to pass them off to someone else. Peace - Anthony
              Hide
              timhunt Tim Hunt added a comment -

              As I said in MDL-16919, I fail to see the logic in assigning this to me.

              Show
              timhunt Tim Hunt added a comment - As I said in MDL-16919 , I fail to see the logic in assigning this to me.
              Hide
              salvetore Michael de Raadt added a comment -

              Ankit has started MDL-36298 to manage the necessary changes needed to achieve this fix. This issue is now a sub-task of a META-issue that combines this work.

              Show
              salvetore Michael de Raadt added a comment - Ankit has started MDL-36298 to manage the necessary changes needed to achieve this fix. This issue is now a sub-task of a META-issue that combines this work.
              Hide
              abgreeve Adrian Greeve added a comment -

              [Y] Syntax
              [Y] Output
              [*] Whitespace
              [-] Language
              [Y] Databases
              [N] Testing
              [-] Security
              [-] Documentation
              [Y] Git
              [*] Sanity check

              Hi Ankit,

              I think this looks fine. Your patch fits in with the rest of the code on the page.
              Some minor things:

              1. admin/tool/uploaduser/index.php line 282 - If I'm reading this correctly shouldn't we have a $userserrors++ and a continue here?
              2. line 294 could be removed (blank line).

              Thanks.

              Show
              abgreeve Adrian Greeve added a comment - [Y] Syntax [Y] Output [*] Whitespace [-] Language [Y] Databases [N] Testing [-] Security [-] Documentation [Y] Git [*] Sanity check Hi Ankit, I think this looks fine. Your patch fits in with the rest of the code on the page. Some minor things: admin/tool/uploaduser/index.php line 282 - If I'm reading this correctly shouldn't we have a $userserrors++ and a continue here? line 294 could be removed (blank line). Thanks.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              I have updated the patch based on discussion in MDL-36299
              Resubmitting for review.
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - I have updated the patch based on discussion in MDL-36299 Resubmitting for review. Thanks
              Hide
              abgreeve Adrian Greeve added a comment -

              [Y] Syntax
              [Y] Output
              [Y] Whitespace
              [Y] Language
              [-] Databases
              [N] Testing
              [-] Security
              [-] Documentation
              [Y] Git
              [Y] Sanity check

              Hi Ankit,

              The code looks good and the new string seems appropriate.
              One small issue. The way the testing instructions stand at the moment, it won't test the code that you have written.

              You have to switch 'Standardise usernames' to 'No'.

              At the moment it goes through the following section of code which cleans the usernames and then your code does a check with clean_param which will always pass.
              admin/tool/uploaduser/index.php line 273

              if ($standardusernames) {
                  $user->username = clean_param($user->username, PARAM_USERNAME);
              }
              

              Just fix that up and you're ready to go.

              Show
              abgreeve Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hi Ankit, The code looks good and the new string seems appropriate. One small issue. The way the testing instructions stand at the moment, it won't test the code that you have written. You have to switch 'Standardise usernames' to 'No'. At the moment it goes through the following section of code which cleans the usernames and then your code does a check with clean_param which will always pass. admin/tool/uploaduser/index.php line 273 if ($standardusernames) { $user->username = clean_param($user->username, PARAM_USERNAME); } Just fix that up and you're ready to go.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Thanks for the review Adrian,
              updated testing instructions, submitting for integration.

              Show
              ankit_frenz Ankit Agarwal added a comment - Thanks for the review Adrian, updated testing instructions, submitting for integration.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Ankit, this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Ankit, this has been integrated now.
              Hide
              rwijaya Rossiani Wijaya added a comment -

              This is working as expected.

              Test passed.

              Show
              rwijaya Rossiani Wijaya added a comment - This is working as expected. Test passed.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              And your fantastic code has met core, hope they become good friends for a long period.

              Closing, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

                People

                • Votes:
                  1 Vote for this issue
                  Watchers:
                  8 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/Jan/13