Moodle
  1. Moodle
  2. MDL-16730

CSV User Import: Enforce validation of username field

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0
    • Fix Version/s: None
    • Component/s: Administration
    • Labels:
    • 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
    • Pull 2.4 Branch:
      MDL-16730-m24
    • Pull Master Branch:
      MDL-16730-master
    • Rank:
      1005

      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

        Issue Links

          Activity

          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Raising to Major and addressing to 1.9.3

          Show
          Eloy Lafuente (stronk7) added a comment - Raising to Major and addressing to 1.9.3
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Assigning this also to Jerome, in relation with MDL-16919
          Hide
          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
          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
          Tim Hunt added a comment -

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

          Show
          Tim Hunt added a comment - As I said in MDL-16919 , I fail to see the logic in assigning this to me.
          Hide
          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
          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
          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
          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 Agarwal added a comment -

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

          Show
          Ankit Agarwal added a comment - I have updated the patch based on discussion in MDL-36299 Resubmitting for review. Thanks
          Hide
          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
          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 Agarwal added a comment -

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

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

          Thanks Ankit, this has been integrated now.

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

          This is working as expected.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Test passed.
          Hide
          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
          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: