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 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

          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: