Moodle

CSV User Import: Enforce validation of username field

Details

  • Type: Improvement Improvement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.9, 2.0
  • Fix Version/s: STABLE backlog
  • Component/s: Administration
  • Labels:
    None
  • Affected Branches:
    MOODLE_19_STABLE, MOODLE_20_STABLE

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

This issue will help resolve:
MDL-16919 Administration: Allow username to contain an underscore Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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.

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.