Moodle
  1. Moodle
  2. MDL-16919

Administration: Allow username to contain an underscore

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0
    • Fix Version/s: 2.0
    • Component/s: Administration
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32885

      Description

      Currently, the username is not allowed to contain an underscore. The validity check when manually creating a new issue states that the username field "Can only contain alphabetical letters or numbers"; however, periods and commas are allowed. It would make sense to also allow the underscore character. Part of what necessitates this is that the uploading of users was previously not checking for the underscore character so in fact some sites have users with them and others have discovered now that the underscore is being removed in 1.9 uploads. To be consistent, we should determine whether we are going to allow them or not and if so then we will need to fix the upload so that it does not strip out the underscore character and modify the validity check when manually adding a new user so that it allows it. We should also change the warning so that it is more accurate so that it states that the username "can only contain alphabetical letters, numbers, periods, commas, and underscores". In the meantime, in the hopes of preventing confusion, I have added notes about validity checking and requirements for the username field in Moodle docs at the following locations:

      http://docs.moodle.org/en/Update_profile#Adding_a_new_user
      http://docs.moodle.org/en/Upload_users#Upload_file_format
      http://docs.moodle.org/en/Edit_profile#User_name

      When this issue is resolved, we should probably go back in and make sure the docs reflect what we have done.

      Peace - Anthony

      1. 20091117_HEAD_mdl_16919.patch
        4 kB
        Rossiani Wijaya
      2. 20091118_HEAD_mdl_16919.patch
        4 kB
        Rossiani Wijaya
      3. 20091118_moodlelib.php.patch
        1 kB
        Rossiani Wijaya
      4. 20091119_HEAD_moodlelib.php.patch
        1 kB
        Rossiani Wijaya
      5. 20091119_HEAD_testmoodlelib.php.patch
        3 kB
        Rossiani Wijaya
      6. 20091120_HEAD_testmoodlelib.php.patch
        3 kB
        Rossiani Wijaya
      7. 20091124_HEAD_mdl_16919.patch
        7 kB
        Rossiani Wijaya
      8. 20091203_mdl_16919_HEAD.patch
        11 kB
        Rossiani Wijaya
      9. 20091221_mdl_16919.patch
        15 kB
        Rossiani Wijaya
      10. 20091222_mdl_16919_HEAD.patch
        20 kB
        Rossiani Wijaya
      11. uploaduser_20100201_2.patch
        15 kB
        Rossiani Wijaya
      1. upload_validation1.png
        14 kB
      2. upload_validation2.png
        16 kB
      3. useruploadfile.jpg
        42 kB

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          I had forgotten about the extendedusernamechars option under site policies. When this is enabled, it allows the underscore character to be imported and added manually. So I think that we are really only dealing with an issue of firming up when extendedusernamechars is off then I think that is the behavior we should get. No underscore characters allowed in manual or uploads. I would go further and say that if extendedusernamechars is not being used then the period and commas currently permissible should not be allowed without it being set. Peace - Anthony

          Show
          Anthony Borrow added a comment - I had forgotten about the extendedusernamechars option under site policies. When this is enabled, it allows the underscore character to be imported and added manually. So I think that we are really only dealing with an issue of firming up when extendedusernamechars is off then I think that is the behavior we should get. No underscore characters allowed in manual or uploads. I would go further and say that if extendedusernamechars is not being used then the period and commas currently permissible should not be allowed without it being set. Peace - Anthony
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, so, summarizing:

          (A)- With $CFG->extendedusernamechars DISABLED we must allow exclusively these:

          alphanumeric chars (A-Z, a-z, 0-9)
          dot (.)
          hypen
          undescore (_)

          (B)- With $CFG->extendedusernamechars ENABLED, any char is allowed in usernames (current behaviour).

          And we must perform changes in:

          admin/uploaduser.php
          login/index.php
          login/signup_form.php
          user/editadvanced_form.php

          in order to check for the characters specified in (A).

          Sounds as a good plan. Some observations:

          1) I'd move the check to one central function - user_normalise_username() - somewhere and make all the places above to use it.

          2) External auth plugins... should them be rejecting wrong usernames? I guess we don't make any verification there.

          3) I've detected that the currently used regular expression:

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

          seems to be faulty, because, or I'm wrong or it allows parenthesis in usernames!! So the question is... should we continue allowing them (can break users using those chars previously).

          Adding some watchers, assigning to Jerome and addressing to 1.9.4. Please comment about 2 & 3.

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, so, summarizing: (A)- With $CFG->extendedusernamechars DISABLED we must allow exclusively these: alphanumeric chars (A-Z, a-z, 0-9) dot (.) hypen undescore (_) (B)- With $CFG->extendedusernamechars ENABLED, any char is allowed in usernames (current behaviour). And we must perform changes in: admin/uploaduser.php login/index.php login/signup_form.php user/editadvanced_form.php in order to check for the characters specified in (A). Sounds as a good plan. Some observations: 1) I'd move the check to one central function - user_normalise_username() - somewhere and make all the places above to use it. 2) External auth plugins... should them be rejecting wrong usernames? I guess we don't make any verification there. 3) I've detected that the currently used regular expression: $user->username = eregi_replace('[^( \. [:alnum:] )]', '', $user >username); seems to be faulty, because, or I'm wrong or it allows parenthesis in usernames!! So the question is... should we continue allowing them (can break users using those chars previously). Adding some watchers, assigning to Jerome and addressing to 1.9.4. Please comment about 2 & 3.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          BTW the current eregi_replace() function above seems to be double-faulty. Not only parenthesis are allowed, but slashes and backslashes too!

          Show
          Eloy Lafuente (stronk7) added a comment - BTW the current eregi_replace() function above seems to be double-faulty. Not only parenthesis are allowed, but slashes and backslashes too!
          Hide
          Marc Grober added a comment -

          My only concern is the basis for the extended characters in username. Are there technical reasons to exclude dot hyphen and underscore. On the otherhand, as I pointed out to Anthony, is this another area where Moodle might want to provide a gui to create an array of excluded characters. For example, many systems allow the user to use their e-mail address because users are now havng to access more and more systems and are having a hard time remembering all their usernames.... should Moodle be able to afford this option? SHould we provide the option to have extended characters largely because of different auth schemes?

          Show
          Marc Grober added a comment - My only concern is the basis for the extended characters in username. Are there technical reasons to exclude dot hyphen and underscore. On the otherhand, as I pointed out to Anthony, is this another area where Moodle might want to provide a gui to create an array of excluded characters. For example, many systems allow the user to use their e-mail address because users are now havng to access more and more systems and are having a hard time remembering all their usernames.... should Moodle be able to afford this option? SHould we provide the option to have extended characters largely because of different auth schemes?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Marc,

          dots and hypens aren't excluded at all (right now). The thing is about to allow also the underscore (and decide what to do with parenthesis, slashes and backslashes that (right now again) are allowed by mistake.

          And do this consistently in:

          • authentication
          • login
          • user profile
          • upload users

          About the idea of having some configuration setting allowing some more chars... uhm... could be interesting... the only point against that is that those chars need to end in a regular expression and, depending of the chars.... they'll need to be escaped in the regexp or it will break. But can be a interesting idea, yup.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Marc, dots and hypens aren't excluded at all (right now). The thing is about to allow also the underscore (and decide what to do with parenthesis, slashes and backslashes that (right now again) are allowed by mistake. And do this consistently in: authentication login user profile upload users About the idea of having some configuration setting allowing some more chars... uhm... could be interesting... the only point against that is that those chars need to end in a regular expression and, depending of the chars.... they'll need to be escaped in the regexp or it will break. But can be a interesting idea, yup. Ciao
          Hide
          Anthony Borrow added a comment -

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

          Assigning this to me just becuase it involves regular expressions is a bit stupid IMO.

          It is really a policy issue, which I don't want to decide about. Well, if I had to decide, I would say that the code is currently unambiguous and consistent. Everywhere we check, we allow:

          • /\.[:alnum:]/ - that isalphanumeric, hyphen and full stop if $CFG>extendedusernamechars is off.
          • Absolutely anything if $CFG->extendedusernamechars is on
            So that is the current behaviour. If the docs do not say that, we should fix the docs. If any code fails to check, it should be fixed.

          However, we have code like
          if (empty($CFG->extendedusernamechars)) {
          $string = eregi_replace("[^(-\.[:alnum:])]", '', $data['username']);

          copied and pasted all over the place. That is horrible. I suggest we define a new PARAM_USERNAME with code in clean_param like

          case PARAM_USERNAME:
          if (empty($CFG->extendedusernamechars))

          { return eregi_replace('[^(-\.[:alnum:])]', '', $param); }

          else

          { return clean_param($param, PARAM_NOTAGS); }

          (the signup form does PARAM_NOTAGS. I am not sure that is really right.)

          Once all the code is consistently using PARAM_USERNAME, then we can easily implement whatever we decide about this bug by changing the code in just one place.

          Show
          Tim Hunt added a comment - Assigning this to me just becuase it involves regular expressions is a bit stupid IMO. It is really a policy issue, which I don't want to decide about. Well, if I had to decide, I would say that the code is currently unambiguous and consistent. Everywhere we check, we allow: / \. [:alnum:] / - that isalphanumeric, hyphen and full stop if $CFG >extendedusernamechars is off. Absolutely anything if $CFG->extendedusernamechars is on So that is the current behaviour. If the docs do not say that, we should fix the docs. If any code fails to check, it should be fixed. However, we have code like if (empty($CFG->extendedusernamechars)) { $string = eregi_replace("[^(-\. [:alnum:] )]", '', $data ['username'] ); copied and pasted all over the place. That is horrible. I suggest we define a new PARAM_USERNAME with code in clean_param like case PARAM_USERNAME: if (empty($CFG->extendedusernamechars)) { return eregi_replace('[^(-\.[:alnum:])]', '', $param); } else { return clean_param($param, PARAM_NOTAGS); } (the signup form does PARAM_NOTAGS. I am not sure that is really right.) Once all the code is consistently using PARAM_USERNAME, then we can easily implement whatever we decide about this bug by changing the code in just one place.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Well,

          "I would say that the code is currently unambiguous and consistent"

          we can call current code that way, np. But let's call it BUGGY (as I commented some messages above). Current eregi_replace() expression is 100% incorrect (that's the point to fix in this bug). It allows a lot of chars to pass!!

          Code:

          <?php
          $string = '012abc.,:;-_/
          ñÑ[]{}';
          echo 'orig: ' . $string . "\n";
          echo 'ereg: ' . eregi_replace("[^(-\.[:alnum:])]", '', $string) . "\n";
          ?>

          Results:

          orig: 012abc.,:;-_/\ñÑ[]{}
          ereg: 012abc.,:;-/[

          So, dots, commas, colons, semicolons, slashes, backslashes, square brackets are allowed! And they shouldn't.

          I agree about to move check to a central place and apply it everywhere (bulk user load, auth...) But first the check itself must be fixed. We are allowing far more than the recommended / documented chars.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Well, "I would say that the code is currently unambiguous and consistent" we can call current code that way, np. But let's call it BUGGY (as I commented some messages above). Current eregi_replace() expression is 100% incorrect (that's the point to fix in this bug). It allows a lot of chars to pass!! Code: <?php $string = '012abc.,:;-_/ ñÑ[]{}'; echo 'orig: ' . $string . "\n"; echo 'ereg: ' . eregi_replace("[^(-\. [:alnum:] )]", '', $string) . "\n"; ?> Results: orig: 012abc.,:;-_/\ñÑ[]{} ereg: 012abc.,:;-/[ So, dots, commas, colons, semicolons, slashes, backslashes, square brackets are allowed! And they shouldn't. I agree about to move check to a central place and apply it everywhere (bulk user load, auth...) But first the check itself must be fixed. We are allowing far more than the recommended / documented chars. Ciao
          Hide
          Chris Collman added a comment -

          This may not be the right place but I was searching to find out if a space was allowed in a user login name.
          The documentation does not say yes or no. A Moodle site search on username got me here and I check the three links to MoodleDocs Anthony provided.

          Details: I noticed a site login error and the customer service person did something (I suspect changed the password) and then the user could not login again. Everyone is in different time zones, so I added a period that changed the username from "chris collman" to "chris.collman".

          Then I downloaded a list of users in Excel and ran my eye over the names. Much to my surprise, I think I am seeing some other user names that contain spaces. One of the names I recognize as receiving a certificate for one of the courses. Still got a couple of hours before the customer service person comes on line and I will follow up to see exactly what the problem was/is with the login.

          But I am more concerned that our documentation specifically mention "spaces" in the user login name and if they are allowed or not.

          Thanks Chris

          Show
          Chris Collman added a comment - This may not be the right place but I was searching to find out if a space was allowed in a user login name. The documentation does not say yes or no. A Moodle site search on username got me here and I check the three links to MoodleDocs Anthony provided. Details: I noticed a site login error and the customer service person did something (I suspect changed the password) and then the user could not login again. Everyone is in different time zones, so I added a period that changed the username from "chris collman" to "chris.collman". Then I downloaded a list of users in Excel and ran my eye over the names. Much to my surprise, I think I am seeing some other user names that contain spaces. One of the names I recognize as receiving a certificate for one of the courses. Still got a couple of hours before the customer service person comes on line and I will follow up to see exactly what the problem was/is with the login. But I am more concerned that our documentation specifically mention "spaces" in the user login name and if they are allowed or not. Thanks Chris
          Hide
          Rossiani Wijaya added a comment - - edited

          Chris - username should not contain spaces. if you upload new user file, the system will automatically truncate any empty spaces within the user name.

          as per tim suggestion, I defined a new case for PARAM_USERNAME in clean_param (moodlelib.php)
          case PARAM_USERNAME:
          $patterns = array();
          array_push($patterns, '/[*()+]/');
          array_push($patterns, '/[^(-\.[:alnum:])_]/i');

          if ($CFG->extendedusernamechars)

          { $param = preg_replace($patterns, '',trim($param)); return $param; }

          else

          { array_push($patterns, '/_/i'); //truncate underscore $param = preg_replace($patterns, '',trim($param)); return $param; }

          the patch is available only for HEAD. Please let me know if you have any question.

          thanks

          Show
          Rossiani Wijaya added a comment - - edited Chris - username should not contain spaces. if you upload new user file, the system will automatically truncate any empty spaces within the user name. as per tim suggestion, I defined a new case for PARAM_USERNAME in clean_param (moodlelib.php) case PARAM_USERNAME: $patterns = array(); array_push($patterns, '/ [*()+] /'); array_push($patterns, '/[^(-\. [:alnum:] )_]/i'); if ($CFG->extendedusernamechars) { $param = preg_replace($patterns, '',trim($param)); return $param; } else { array_push($patterns, '/_/i'); //truncate underscore $param = preg_replace($patterns, '',trim($param)); return $param; } the patch is available only for HEAD. Please let me know if you have any question. thanks
          Hide
          Rossiani Wijaya added a comment -

          http://docs.moodle.org/en/Site_policies#Allow_extended_characters_in_usernames
          download.moodle.org/docs/en/using_moodle/ch16_server_admin.pdf

          according to the above document, the rule for username with Allow extended characters:

          Disable:
          alphanumeric
          period (.)
          hypen

          enable:
          any character

          empty spaces will be removed from username.

          I'm attaching new patch to apply the above rule (moodlelib.php only).

          Show
          Rossiani Wijaya added a comment - http://docs.moodle.org/en/Site_policies#Allow_extended_characters_in_usernames download.moodle.org/docs/en/using_moodle/ch16_server_admin.pdf according to the above document, the rule for username with Allow extended characters: Disable: alphanumeric period (.) hypen enable: any character empty spaces will be removed from username. I'm attaching new patch to apply the above rule (moodlelib.php only).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rossiani,

          I must confess I don't understand the logic beyond your HEAD patch. Can you add some tests demonstrating what is allowed and what is not?

          • different symbols
          • utf-8 alphanumeric chars...
          • underscore, hypen...

          both with CFG->extendedusernamechars enable and disabled

          Also, the 1.9 patch looks somehow "wrong", with patters array declared in wrong place, not sure though, just a perception.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rossiani, I must confess I don't understand the logic beyond your HEAD patch. Can you add some tests demonstrating what is allowed and what is not? different symbols utf-8 alphanumeric chars... underscore, hypen... both with CFG->extendedusernamechars enable and disabled Also, the 1.9 patch looks somehow "wrong", with patters array declared in wrong place, not sure though, just a perception. Ciao
          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy,

          Both patches should be applied on HEAD. I should mention that on my previous patch. Ah, just realize i uploaded the wrong file for moodlelib.php. Please use the latest patch.

          so, if $allowextendedcharacters == disable, then
          johndoe123 – valid
          john.doe – valid
          john-doe – valid

          john_doe – invalid
          john@doe – invalid

          if $allowextendedcharacters == enable, then
          johndoe123 – valid
          john.doe – valid
          john-doe – valid

          john_doe – valid
          john@doe – valid
          john#$%&() – valid

          Please let me know if you have more question.

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Eloy, Both patches should be applied on HEAD. I should mention that on my previous patch. Ah, just realize i uploaded the wrong file for moodlelib.php. Please use the latest patch. so, if $allowextendedcharacters == disable, then johndoe123 – valid john.doe – valid john-doe – valid john_doe – invalid john@doe – invalid if $allowextendedcharacters == enable, then johndoe123 – valid john.doe – valid john-doe – valid john_doe – valid john@doe – valid john#$%&() – valid Please let me know if you have more question. Thanks Rosie
          Hide
          Eloy Lafuente (stronk7) added a comment -

          can you test these too, plz:

          • john~doe
          • john´doe
          • johndóé

          Also, having all these tests into lib/simpletest/test_moodlelib.php would be perfect IMO.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - can you test these too, plz: john~doe john´doe johndóé Also, having all these tests into lib/simpletest/test_moodlelib.php would be perfect IMO. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Also, http://docs.moodle.org/en/Site_policies#Allow_extended_characters_in_usernames should show final behaviour without ambiguities once this get tested and resolved. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Also, http://docs.moodle.org/en/Site_policies#Allow_extended_characters_in_usernames should show final behaviour without ambiguities once this get tested and resolved. Ciao
          Hide
          Rossiani Wijaya added a comment -

          Eloy,

          I made slightly change on moodlelib.php (line 729)
          from:
          $param = trim($param); (this only removing empty spaces at the beginning and end of string)
          to:
          $param = str_replace(" " , "", $param); (i think this is better, it will eliminate all empty spaces in the string.)

          i also attaching the test cases for clean_param().

          Please take a look and let me know if i need to make any changes to it.

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Eloy, I made slightly change on moodlelib.php (line 729) from: $param = trim($param); (this only removing empty spaces at the beginning and end of string) to: $param = str_replace(" " , "", $param); (i think this is better, it will eliminate all empty spaces in the string.) i also attaching the test cases for clean_param(). Please take a look and let me know if i need to make any changes to it. Thanks Rosie
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rosie, nice tests!

          Anyway I think you can, do domething like:

          global $CFG;
          $currentstatus = $FG->extendedusernamechars;

          $FG->extendedusernamechars = false;
          /// Run tests with extended disabled here

          $FG->extendedusernamechars = true;
          /// Run tests with extended enabled here

          $FG->extendedusernamechars = $currentstatus;

          That way all the tests will be executed in a "correct" environment and we'll have perfect coverage of the PARAM_USERNAME thing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rosie, nice tests! Anyway I think you can, do domething like: global $CFG; $currentstatus = $FG->extendedusernamechars; $FG->extendedusernamechars = false; /// Run tests with extended disabled here $FG->extendedusernamechars = true; /// Run tests with extended enabled here $FG->extendedusernamechars = $currentstatus; That way all the tests will be executed in a "correct" environment and we'll have perfect coverage of the PARAM_USERNAME thing. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And the last point with this is:

          Are we breaking BC compatibility? If so... will that affect newly created users on login or the change only applies to new accounts?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And the last point with this is: Are we breaking BC compatibility? If so... will that affect newly created users on login or the change only applies to new accounts? Ciao
          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy,

          I made changes to the test file (20091120_HEAD_testmoodlelib.php.patch) and the changes only apply to new accounts.

          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Eloy, I made changes to the test file (20091120_HEAD_testmoodlelib.php.patch) and the changes only apply to new accounts. Rosie
          Hide
          Martin Dougiamas added a comment -

          Consensus after a chat in dev chat just now was

          normal: a-z . _ @

          extended: everything

          Can you double check that š??žžý fails in the first case, Rosie?

          Show
          Martin Dougiamas added a comment - Consensus after a chat in dev chat just now was normal: a-z . _ @ extended: everything Can you double check that š??žžý fails in the first case, Rosie?
          Hide
          Petr Škoda added a comment -

          ereg and preg treat [:anum:] differently, in preg ?ýáž?žý??š is valid, in ereg it is not
          in case of "normal" passwords we should imo accept only safe 7-bit chars that are compatible with any encoding (ASCII)

          Show
          Petr Škoda added a comment - ereg and preg treat [:anum:] differently, in preg ?ýáž?žý??š is valid, in ereg it is not in case of "normal" passwords we should imo accept only safe 7-bit chars that are compatible with any encoding (ASCII)
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Tests look perfect Rossi

          just for the record, at HQ chat we talked about:

          • Accept "_" and "@" in nonextended mode.
          • Be careful with [alnum] as far as it accepts sometimes utf-8 chars. It seems that [a-z0-9] is always restricted to ASCII, needs more tests.

          Ciao

          Edited: Correct typo.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Tests look perfect Rossi just for the record, at HQ chat we talked about: Accept "_" and "@" in nonextended mode. Be careful with [alnum] as far as it accepts sometimes utf-8 chars. It seems that [a-z0-9] is always restricted to ASCII, needs more tests. Ciao Edited: Correct typo.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hoho, LOL it seems that everybody updated the bug with today's HQ conclusions!

          Sure Rosie have complete info now, hehe, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hoho, LOL it seems that everybody updated the bug with today's HQ conclusions! Sure Rosie have complete info now, hehe, ciao
          Hide
          Rossiani Wijaya added a comment -

          Created new patch to add "_" and "@" to nonextended mode and also added new testing line to testmoodlelib.

          I tested the following:

          $string = 'john.,:;/|\ñÑ[]A_X,D {} ~!@#$%^&*()+ ?><[] š??žžý ?ýá?ý?doe ';
          echo preg_replace('/[^-\.@_[:alnum:]]/i', '', $string);

          output: john.-_A_X-D@_doe

          FYI: eregi_replace has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 6.0.0.

          thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Created new patch to add "_" and "@" to nonextended mode and also added new testing line to testmoodlelib. I tested the following: $string = 'john.,:; /|\ñÑ[]A_X ,D {} ~!@#$%^&*() + ?><[] š??žžý ?ýá? ý ?doe '; echo preg_replace('/[^-\.@_ [:alnum:] ]/i', '', $string); output: john.-_A_X-D@_doe FYI: eregi_replace has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 6.0.0. thanks Rosie
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry Rossi but in my test environment (MacOS X, full UTF-8 + custom compiled PHP)

          your

          preg_replace('/[^-\.@_[:alnum:]]/i', '', $string)
          

          returns accents and other UTF-8 chars perfectly.

          I think Petr was saying exactly that some comments above: "[:alnum:] supports/matches UTF-8 chars under some environments". So it's not usable in this case at all.

          So, the alternative was to use a-zA-Z0-9. I've tested this:

          preg_replace('/[^-\.@_a-zA-Z0-9]/', '', $string)
          

          And seems to work correctly here. Does it work there too? If so, +1 to use that.

          Also, don't forget to change http://docs.moodle.org/en/Site_policies#Allow_extended_characters_in_usernames

          Ciao

          PS: Silly question, we support uppercase chars in usernames.... if so... are we checking in DB in a case insensitive way to avoid duplicates? I say that because some DBs are casesensitive and others are insensitive... So we should be guaranteeing no case insensitive dupes. Not critical but can affect migration from one DB to other.

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry Rossi but in my test environment (MacOS X, full UTF-8 + custom compiled PHP) your preg_replace('/[^-\.@_[:alnum:]]/i', '', $string) returns accents and other UTF-8 chars perfectly. I think Petr was saying exactly that some comments above: " [:alnum:] supports/matches UTF-8 chars under some environments" . So it's not usable in this case at all. So, the alternative was to use a-zA-Z0-9. I've tested this: preg_replace('/[^-\.@_a-zA-Z0-9]/', '', $string) And seems to work correctly here. Does it work there too? If so, +1 to use that. Also, don't forget to change http://docs.moodle.org/en/Site_policies#Allow_extended_characters_in_usernames Ciao PS: Silly question, we support uppercase chars in usernames.... if so... are we checking in DB in a case insensitive way to avoid duplicates? I say that because some DBs are casesensitive and others are insensitive... So we should be guaranteeing no case insensitive dupes. Not critical but can affect migration from one DB to other.
          Hide
          Petr Škoda added a comment -

          oh, I thought those are case insensitive, hmm I might remember some complaints related to username case and external account sync

          Show
          Petr Škoda added a comment - oh, I thought those are case insensitive, hmm I might remember some complaints related to username case and external account sync
          Hide
          Rossiani Wijaya added a comment -

          Eloy - username does not allow uppercase chars and there's no documentation regarding this rule.

          uploading bulk username, the system will automatically truncate the uppercase character.

          Ps: i did not make new patch yet, just in case we want add new rule regarding uppercase character.

          Show
          Rossiani Wijaya added a comment - Eloy - username does not allow uppercase chars and there's no documentation regarding this rule. uploading bulk username, the system will automatically truncate the uppercase character. Ps: i did not make new patch yet, just in case we want add new rule regarding uppercase character.
          Hide
          Helen Foster added a comment - - edited

          Re. the lang string alphanumerical, as the functionality is changing (_ and @ is to be allowed in Moodle 2.0, whereas before it wasn't allowed) I suggest we have a new lang string validusername:

          $string['invalidusername'] = 'The username can only contain alphanumeric characters, underscore (_), hyphen (-), period (.) or at symbol (@)';

          The existing lang string could be reworded to make things clearer:

          $string['alphanumerical'] = 'Can only contain alphanumeric characters, hyphen (-) or  period (.)';

          (Edited to change string name from alphanumerical2 to invalidusername as suggested by Eloy.)

          Show
          Helen Foster added a comment - - edited Re. the lang string alphanumerical, as the functionality is changing (_ and @ is to be allowed in Moodle 2.0, whereas before it wasn't allowed) I suggest we have a new lang string validusername: $string['invalidusername'] = 'The username can only contain alphanumeric characters, underscore (_), hyphen (-), period (.) or at symbol (@)'; The existing lang string could be reworded to make things clearer: $string['alphanumerical'] = 'Can only contain alphanumeric characters, hyphen (-) or period (.)'; (Edited to change string name from alphanumerical2 to invalidusername as suggested by Eloy.)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 for new string, agree Helen, I just would change it to add the word "lowercase characters".

          I think uppercase chars must be cleaned (detected as invalid) in any case, Rossi. No matter of extended or normal modes. Rigth now the create_user_record() function is doing one strlower always, so there isn't any reason to accept uppercase chars at all.

          So, any point of user creation (bulk upload, manual accounts, signup...) must enforce lowercase chars always. The only exception is the login page where we'll be performing the lowercase behind the scenes just to allow people used to have uppercase chars (but not really having them) to continue login in the same way.

          Does it have sense?

          In any case... note we are discussing latter this week if PARAM_USERNAME and other PARAM functions must be really PARAM functions or, alternatively, must be moved to other functions (out from the PARAM/clean stuff). Just to let you know, Rossi. In any case it will be easily movable if needed so you can continue advancing assuming current PARAM/clean is the way.

          Show
          Eloy Lafuente (stronk7) added a comment - +1 for new string, agree Helen, I just would change it to add the word "lowercase characters". I think uppercase chars must be cleaned (detected as invalid) in any case, Rossi. No matter of extended or normal modes. Rigth now the create_user_record() function is doing one strlower always, so there isn't any reason to accept uppercase chars at all. So, any point of user creation (bulk upload, manual accounts, signup...) must enforce lowercase chars always. The only exception is the login page where we'll be performing the lowercase behind the scenes just to allow people used to have uppercase chars (but not really having them) to continue login in the same way. Does it have sense? In any case... note we are discussing latter this week if PARAM_USERNAME and other PARAM functions must be really PARAM functions or, alternatively, must be moved to other functions (out from the PARAM/clean stuff). Just to let you know, Rossi. In any case it will be easily movable if needed so you can continue advancing assuming current PARAM/clean is the way.
          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy,

          I made changes to the patch according to you suggestion. In addition, i add validation to "upload users preview" page and attached 2images for the possibility to display the error validation.

          Please take a look and let me know if any changes needed.

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Eloy, I made changes to the patch according to you suggestion. In addition, i add validation to "upload users preview" page and attached 2images for the possibility to display the error validation. Please take a look and let me know if any changes needed. Thanks Rosie
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rosie,

          a few things about your last patch:

          1) About the tests, it seems that we are only executing one group or the other ("if" sentence). The idea is to execute all the always, setting $CFG->extendedusernamechars by hand (and resetting it to original status at the end with the value stored in $currentstatus).
          2) There is one harcoded English string in admin/uploaduser.php: "Originally". Must be changed to proper lang string.
          3) Question about admin/uploaduser.php... when one record has an incorrect username... what happens? Is it created with the cleaned username or rejected with error? IMO we should avoid to create automatically cleaned usernames and reject them, to allow the admin to fix it. Not sure if that's current approach, please confirm.

          Finally note it was supposed we were going to discuss about this in the Hackfest but that never happened. So... plz, watchers... it you've something to say, say it here right now. Else we'll go ahead and commit.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rosie, a few things about your last patch: 1) About the tests, it seems that we are only executing one group or the other ("if" sentence). The idea is to execute all the always, setting $CFG->extendedusernamechars by hand (and resetting it to original status at the end with the value stored in $currentstatus). 2) There is one harcoded English string in admin/uploaduser.php: "Originally". Must be changed to proper lang string. 3) Question about admin/uploaduser.php... when one record has an incorrect username... what happens? Is it created with the cleaned username or rejected with error? IMO we should avoid to create automatically cleaned usernames and reject them, to allow the admin to fix it. Not sure if that's current approach, please confirm. Finally note it was supposed we were going to discuss about this in the Hackfest but that never happened. So... plz, watchers... it you've something to say, say it here right now. Else we'll go ahead and commit. Ciao
          Hide
          Rossiani Wijaya added a comment -

          Thanks Eloy for reviewing the patch.

          answering eloy's question:

          1. got it. i will make the changes to the test file.
          2. i was purposely hardcoded the string, because I wasn't sure on which is the best way to display the error. I created 2 screenshots for the possibilities of displaying the error (upload_validation1.png and upload_validation2.png). I prefer displaying it with the second option 'upload_validation2.png). Eloy (or watchers), do you have any preference on how the error should be display?
          3. currently, when the record has an incorrect username, it will created with the cleaned username.

          I will fix the tests file and works more on the uploaduser.php to reject creating user when record has invalid characters. in other words, if the file contains any invalid username or the email is already exist, the system wont be able to create any new user, until the file is error free.

          Show
          Rossiani Wijaya added a comment - Thanks Eloy for reviewing the patch. answering eloy's question: 1. got it. i will make the changes to the test file. 2. i was purposely hardcoded the string, because I wasn't sure on which is the best way to display the error. I created 2 screenshots for the possibilities of displaying the error (upload_validation1.png and upload_validation2.png). I prefer displaying it with the second option 'upload_validation2.png). Eloy (or watchers), do you have any preference on how the error should be display? 3. currently, when the record has an incorrect username, it will created with the cleaned username. I will fix the tests file and works more on the uploaduser.php to reject creating user when record has invalid characters. in other words, if the file contains any invalid username or the email is already exist, the system wont be able to create any new user, until the file is error free.
          Hide
          Sam Hemelryk added a comment -

          Hi Rossi,
          In regards to 2. my +1 for the second option (upload_validation2.png)
          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rossi, In regards to 2. my +1 for the second option (upload_validation2.png) Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          I added username and email address validation for the uploaded file. If username contains invalid characters (extended chars is turn on) and/or email had exist, it will display the error and force user to fix the file and re-upload new file. also, I made a little changes in displaying the error message. if and error exist, it will be displayed in the last column of the row.

          Patch is available to be reviewed.

          Show
          Rossiani Wijaya added a comment - I added username and email address validation for the uploaded file. If username contains invalid characters (extended chars is turn on) and/or email had exist, it will display the error and force user to fix the file and re-upload new file. also, I made a little changes in displaying the error message. if and error exist, it will be displayed in the last column of the row. Patch is available to be reviewed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rosie, it's looking great!

          Here it's one check list, to see if everything is in order:

          1) In login, if user enters uppercase username, it's automatically lowercased (no error at all). The login happens with that lowercased username. This is the only point of Moodle where we "accept" uppercase usernames.
          2) In signup process, uppercase usernames are forbidden, so error is shown. Usernames not fulfilling PARAM_USERNAME also cause error.
          3) In edit-advanced, same behaviour than 2) (both uppercase and not correct PARAM_USERNAME cause error).
          4) In upload user, same behaviour that 2) and 3). Any error causes no-user to be created at all. The entire file is rejected. Error is shown. Must be fixed and uploaded again.

          Are 4 points implemented that way? Yay if yes!

          Then, in your last patch:

          1) lang trings aren't included in the patch.
          2) tests continue being alternative. We must execute all them as commented above.
          3) There are other places where we can be creating users too (mainly authentication plugins... I think we may put at least some TODO notices in those plugins in order to remember that we must user PARAM_USERNAME before inserting into user table. Plz, take a look to any insert_record call into user table in moodle code and add some TODO pointing to this bug. Somethinh like grep -r insert_record * | grep user should return a list of candidates to be examined.

          And that's all. I think we are becoming closer to the end... thanks! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rosie, it's looking great! Here it's one check list, to see if everything is in order: 1) In login, if user enters uppercase username, it's automatically lowercased (no error at all). The login happens with that lowercased username. This is the only point of Moodle where we "accept" uppercase usernames. 2) In signup process, uppercase usernames are forbidden, so error is shown. Usernames not fulfilling PARAM_USERNAME also cause error. 3) In edit-advanced, same behaviour than 2) (both uppercase and not correct PARAM_USERNAME cause error). 4) In upload user, same behaviour that 2) and 3). Any error causes no-user to be created at all. The entire file is rejected. Error is shown. Must be fixed and uploaded again. Are 4 points implemented that way? Yay if yes! Then, in your last patch: 1) lang trings aren't included in the patch. 2) tests continue being alternative. We must execute all them as commented above. 3) There are other places where we can be creating users too (mainly authentication plugins... I think we may put at least some TODO notices in those plugins in order to remember that we must user PARAM_USERNAME before inserting into user table. Plz, take a look to any insert_record call into user table in moodle code and add some TODO pointing to this bug. Somethinh like grep -r insert_record * | grep user should return a list of candidates to be examined. And that's all. I think we are becoming closer to the end... thanks! Ciao
          Hide
          Rossiani Wijaya added a comment - - edited

          Hi Eloy,

          the 4 check list points have been implemented.

          I updated testlib.

          I also attached a new screenshot for the new look for handling invalid file upload (useruploadfile.jpg).

          are you looking for the word "originally" in the language string? if you are, the string has been drop from the new display. please take a look the screenshot.

          here is the list for the new language string that included in the patch
          -$string['alphanumerical'] = 'Can only contain alphabetical letters or numbers';
          +$string['alphanumerical'] = 'Can only contain alphanumeric characters, hyphen or period (.)';
          +$string['invalidusername'] = 'The username can only contain alphanumeric lowercase characters, underscore (_), hyphen , period (.) or at symbol (@)';
          +$string['invalidusernameupload'] = "Invalid username";
          +$string['uploadnewfile'] = 'Upload new file';
          +$string['uploadinvalidpreprocessedcount'] = 'Number of invalid preprocessed records: $a';
          +$string['uploadfilecontainerror'] = 'The uploaded file has not been submitted due to invalid information. Please modify the file and re-upload. ';

          i had prepared a documentation for this update. once this gets approve and checkin, i will update the online documentation.

          please review the latest patch.

          thank you
          Rosie

          Show
          Rossiani Wijaya added a comment - - edited Hi Eloy, the 4 check list points have been implemented. I updated testlib. I also attached a new screenshot for the new look for handling invalid file upload (useruploadfile.jpg). are you looking for the word "originally" in the language string? if you are, the string has been drop from the new display. please take a look the screenshot. here is the list for the new language string that included in the patch -$string ['alphanumerical'] = 'Can only contain alphabetical letters or numbers'; +$string ['alphanumerical'] = 'Can only contain alphanumeric characters, hyphen or period (.)'; +$string ['invalidusername'] = 'The username can only contain alphanumeric lowercase characters, underscore (_), hyphen , period (.) or at symbol (@)'; +$string ['invalidusernameupload'] = "Invalid username"; +$string ['uploadnewfile'] = 'Upload new file'; +$string ['uploadinvalidpreprocessedcount'] = 'Number of invalid preprocessed records: $a'; +$string ['uploadfilecontainerror'] = 'The uploaded file has not been submitted due to invalid information. Please modify the file and re-upload. '; i had prepared a documentation for this update. once this gets approve and checkin, i will update the online documentation. please review the latest patch. thank you Rosie
          Hide
          Helen Foster added a comment -

          Hi Rosie,

          Re. the string uploadfilecontainerror how about:

          'The uploaded file has not been processed due to the error(s) specified. Please amend the file before uploading it again.'

          Show
          Helen Foster added a comment - Hi Rosie, Re. the string uploadfilecontainerror how about: 'The uploaded file has not been processed due to the error(s) specified. Please amend the file before uploading it again.'
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho, sorry by the delay, I had lost this completely!

          +1 for it (see also last lang string suggestion from Helen, I'm not an expert on that at all, hehe).

          As also commented in MDL-21226 , I think we'll end using one core create_user() function or so. We have really tons of insert_record() calls here and there, grrr. But I think it can be handled in the other bug.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, sorry by the delay, I had lost this completely! +1 for it (see also last lang string suggestion from Helen, I'm not an expert on that at all, hehe). As also commented in MDL-21226 , I think we'll end using one core create_user() function or so. We have really tons of insert_record() calls here and there, grrr. But I think it can be handled in the other bug. Thanks and ciao
          Hide
          Petr Škoda added a comment -

          I personally find the PARAM_USERNAME confusing, because it is not actually fixing the submitted string like the other PARAM_, the UPPERCASE letter should be converted to lowercase (using UTF-8 aware function). Looking at the code this causes regressions, sorry, please.

          Also the code actually using this param is not ok imho:

                          if (empty($CFG->extendedusernamechars)) {
                              $string = clean_param($usernew->username, PARAM_USERNAME);
                              if ($usernew->username !== $string) {
                                  $err['username'] = get_string('invalidusername');
                              }
                          }
          

          because it should always do this test no matter what $CFG->extendedusernamechars contains.

          Maybe it would be a good time to introduce option for cases sensitive usernames too at the same time, just an idea

          Show
          Petr Škoda added a comment - I personally find the PARAM_USERNAME confusing, because it is not actually fixing the submitted string like the other PARAM_, the UPPERCASE letter should be converted to lowercase (using UTF-8 aware function). Looking at the code this causes regressions, sorry, please. Also the code actually using this param is not ok imho: if (empty($CFG->extendedusernamechars)) { $string = clean_param($usernew->username, PARAM_USERNAME); if ($usernew->username !== $string) { $err['username'] = get_string('invalidusername'); } } because it should always do this test no matter what $CFG->extendedusernamechars contains. Maybe it would be a good time to introduce option for cases sensitive usernames too at the same time, just an idea
          Hide
          Martin Dougiamas added a comment -

          Thanks Petr, looking at this.

          Show
          Martin Dougiamas added a comment - Thanks Petr, looking at this.
          Hide
          Martin Dougiamas added a comment -

          I think the PARAM is ok (it is actually cleaning the param) so comparing strings afterwards makes sense, but you're right, the calling code to it does not need to check for $CFG->extendedusernamechars.

          Show
          Martin Dougiamas added a comment - I think the PARAM is ok (it is actually cleaning the param) so comparing strings afterwards makes sense, but you're right, the calling code to it does not need to check for $CFG->extendedusernamechars.
          Hide
          Petr Škoda added a comment -

          hmm, up until now we were converting usernames to lowercase - now we just dropping CAPITAL letters, that is a big change imo. For example if we used this new "cleaning" in place where tacher types in username and we look for user, he/she would not know that those letters were removed, instead I would expect it does the same lowercasing like the login page.

          Show
          Petr Škoda added a comment - hmm, up until now we were converting usernames to lowercase - now we just dropping CAPITAL letters, that is a big change imo. For example if we used this new "cleaning" in place where tacher types in username and we look for user, he/she would not know that those letters were removed, instead I would expect it does the same lowercasing like the login page.
          Hide
          Martin Dougiamas added a comment -

          Yep, that makes sense. Rossiani, can you convert uppercase to lowercase in this function as well? Use moodle_strtolower().

          Show
          Martin Dougiamas added a comment - Yep, that makes sense. Rossiani, can you convert uppercase to lowercase in this function as well? Use moodle_strtolower().
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm don't agree about lowercasing PARAM_USERNAME being correct behavior. Uppercase chars are invalid, so they aren't allowed, so they are deleted. That's what all clean_xxx() functions use to do, they don't use to "fix" the param, but to clean all the incorrect bits. That's consistent behaviour IMO.

          If search of users wants to accept uppercase chars, well, then make it to perform the strtolower() before cleaning (as we are doing in login.php, where uppercase chars continue being valid. But don't convert the clean function into one "fix" function.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm don't agree about lowercasing PARAM_USERNAME being correct behavior. Uppercase chars are invalid , so they aren't allowed, so they are deleted. That's what all clean_xxx() functions use to do, they don't use to "fix" the param, but to clean all the incorrect bits. That's consistent behaviour IMO. If search of users wants to accept uppercase chars, well, then make it to perform the strtolower() before cleaning (as we are doing in login.php, where uppercase chars continue being valid. But don't convert the clean function into one "fix" function. Ciao
          Hide
          Petr Škoda added a comment -

          for example PARAM_CLEAN can remove but also add stuff, we are not only removing things; PARAM_BOOL is another example that actually modifies the data. My point was that the actual behaviour of moodle changed after introduction of PARAM_USERNAME, I would personally not add it at all and instead created a special function for username normalisation. I personally do not like putting too much login into the param cleaning.

          Show
          Petr Škoda added a comment - for example PARAM_CLEAN can remove but also add stuff, we are not only removing things; PARAM_BOOL is another example that actually modifies the data. My point was that the actual behaviour of moodle changed after introduction of PARAM_USERNAME, I would personally not add it at all and instead created a special function for username normalisation. I personally do not like putting too much login into the param cleaning.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          > My point was that the actual behaviour of moodle changed after introduction of PARAM_USERNAME

          Sorry, I really don't see what has changed. The only difference is that lowercasing doesn't happen beyond the scenes anymore and now it's properly detected and informed. That's the only difference. Behavior (only accept lowercase usernames) continue being the same.

          > I would personally not add it at all and instead created a special function for username normalization

          Nice (said in an ironic way). I requested comments about that (should be a PARAM_XXX or one function) here, in HQ chat and also added it to the Hackfest Moodle Docs page. And nobody commented at all. So finally we followed the original path (PARAM_XXX). Grrr. Anyway, we are on time to change that if ppl thinks it's better. it's only a matter of decide where all those "normalization" functions should go.

          > I personally do not like putting too much login into the param cleaning.

          It's only one simple regular expression. Other params are, by far, more complex than this, IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - > My point was that the actual behaviour of moodle changed after introduction of PARAM_USERNAME Sorry, I really don't see what has changed. The only difference is that lowercasing doesn't happen beyond the scenes anymore and now it's properly detected and informed. That's the only difference. Behavior (only accept lowercase usernames) continue being the same. > I would personally not add it at all and instead created a special function for username normalization Nice (said in an ironic way). I requested comments about that (should be a PARAM_XXX or one function) here, in HQ chat and also added it to the Hackfest Moodle Docs page. And nobody commented at all. So finally we followed the original path (PARAM_XXX). Grrr. Anyway, we are on time to change that if ppl thinks it's better. it's only a matter of decide where all those "normalization" functions should go. > I personally do not like putting too much login into the param cleaning. It's only one simple regular expression. Other params are, by far, more complex than this, IMO. Ciao
          Hide
          Petr Škoda added a comment -

          old code in upload user:

                  // normalize username
                  $user->username = $textlib->strtolower($user->username);
                  if (empty($CFG->extendedusernamechars)) {
                      $user->username = preg_replace('/[^(-\.[:alnum:])]/i', '', $user->username);
                  }
          

          new code:

          $user->username = clean_param($user->username, PARAM_USERNAME);
          

          the removing of uppercase letters makes a huge difference there, sorry Eloy

          Show
          Petr Škoda added a comment - old code in upload user: // normalize username $user->username = $textlib->strtolower($user->username); if (empty($CFG->extendedusernamechars)) { $user->username = preg_replace('/[^(-\.[:alnum:])]/i', '', $user->username); } new code: $user->username = clean_param($user->username, PARAM_USERNAME); the removing of uppercase letters makes a huge difference there, sorry Eloy
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I love to repeat myself:

          "The only difference is that lowercasing doesn't happen beyond the scenes anymore and now it's properly detected and informed"

          or, in other words. When users are going to be created by upload utility... the file is rejected if usernames are incorrect. Old approach was clearly worse (dark magic). See Rosie screenshots above.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I love to repeat myself: "The only difference is that lowercasing doesn't happen beyond the scenes anymore and now it's properly detected and informed" or, in other words. When users are going to be created by upload utility... the file is rejected if usernames are incorrect. Old approach was clearly worse (dark magic). See Rosie screenshots above. Ciao
          Hide
          Martin Dougiamas added a comment -

          Eloy, I do agree that clean functions shouldn't generally do magic fixing, but I think uppercase-lowercase is a special case of cleaning/fixing because we know exactly what the user "meant".

          Silently converting "McDonald" -> "conald" seems really wrong to me and I can't see any usefulness in it.

          There are two issues here.

          1) What to do with a username coming in as a parameter. We do need to make sure it conforms to the config standard. This will usually be used for searches, login and so on, single parameters coming in from forms. Makes total sense for this to be PARAM_USERNAME alongside similar code, and perfect sense for me to convert uppercase to lowercase so that matches with existing data can work as expected.

          2) What to do with usernames being entered to create new accounts (signup/bulkupload etc). For these cases we need to always flag an error when the username doesn't conform. Performing a string comparison between the original and a cleaned version must be done, and this works equally well whether uppercase is deleted or lowercased. Since deleting it isn't useful (see above) and lowercasing is, let's lowercase it.

          I'll make sure this is all implemented as such so that please, let's move on to real issues.

          Show
          Martin Dougiamas added a comment - Eloy, I do agree that clean functions shouldn't generally do magic fixing, but I think uppercase-lowercase is a special case of cleaning/fixing because we know exactly what the user "meant". Silently converting "McDonald" -> "conald" seems really wrong to me and I can't see any usefulness in it. There are two issues here. 1) What to do with a username coming in as a parameter. We do need to make sure it conforms to the config standard. This will usually be used for searches, login and so on, single parameters coming in from forms. Makes total sense for this to be PARAM_USERNAME alongside similar code, and perfect sense for me to convert uppercase to lowercase so that matches with existing data can work as expected. 2) What to do with usernames being entered to create new accounts (signup/bulkupload etc). For these cases we need to always flag an error when the username doesn't conform. Performing a string comparison between the original and a cleaned version must be done, and this works equally well whether uppercase is deleted or lowercased. Since deleting it isn't useful (see above) and lowercasing is, let's lowercase it. I'll make sure this is all implemented as such so that please, let's move on to real issues.
          Show
          Helen Foster added a comment - Rosie, thanks for your documentation updates - very helpful http://docs.moodle.org/en/Update_profile http://docs.moodle.org/en/Upload_users http://docs.moodle.org/en/Edit_profile http://docs.moodle.org/en/Site_policies
          Hide
          Rossiani Wijaya added a comment -

          add validation to upload user data according to the upload type.

          committed.

          Show
          Rossiani Wijaya added a comment - add validation to upload user data according to the upload type. committed.
          Hide
          Penny Leach added a comment -

          I see there is a TODO in enrol/mnet/enrol.php, and auth/mnet/auth.php that says:

          //TODO - username required to use PARAM_USERNAME before inserting into user table (MDL-16919)

          As per Eloy's suggestion in one of the comments.

          Since I'm looking at this code a lot at the moment it's very unclear to me what the proper solution is here. When syncing external users (both in auth AND in enrol actually) we have a problem if the username policy is inconsistent between Moodle and whatever it's syncing with (be that another Moodle or anything else).

          I guess the best thing in all cases is to change everywhere that accepts a username (this includes the cronjob-syncs as well) and changes it, so that at least inserts and subsequent updates are consistent. However, does this mean we have to do a migration of usernames ? I can't change mnet to do this now since it could well break all current users. You can't migrate all users in a moodle site to adhere to whatever the moodle site username policy is, because then you would end up with potential duplication, eg:

          john'doe
          jonndoe

          would end up conflicting.

          I don't have any solutions, just noticed that there's still a TODO here and this bug seems to have been closed, which seems wrong to me.

          Show
          Penny Leach added a comment - I see there is a TODO in enrol/mnet/enrol.php, and auth/mnet/auth.php that says: //TODO - username required to use PARAM_USERNAME before inserting into user table ( MDL-16919 ) As per Eloy's suggestion in one of the comments. Since I'm looking at this code a lot at the moment it's very unclear to me what the proper solution is here. When syncing external users (both in auth AND in enrol actually) we have a problem if the username policy is inconsistent between Moodle and whatever it's syncing with (be that another Moodle or anything else). I guess the best thing in all cases is to change everywhere that accepts a username (this includes the cronjob-syncs as well) and changes it, so that at least inserts and subsequent updates are consistent. However, does this mean we have to do a migration of usernames ? I can't change mnet to do this now since it could well break all current users. You can't migrate all users in a moodle site to adhere to whatever the moodle site username policy is, because then you would end up with potential duplication, eg: john'doe jonndoe would end up conflicting. I don't have any solutions, just noticed that there's still a TODO here and this bug seems to have been closed, which seems wrong to me.
          Hide
          Petr Škoda added a comment -

          hmm, I thought the only reason to prevent some characters is to prevent user problems when entering the username into the login page, technically we should handle all characters in all auth methods, of course there were critical problems with special characters in some auth plugins causing fatal errors before, but all those problems should be fixed anyway. This means that if the username already exists in other system we should use it "as is", no cleaning or case conversion at all. I was thinking a lot about the lowercasing too, it is kind of similar because we should be able to interface with systems that differentiate case while still keeping the caseless unique usernames - so definitely we can not convert the case before we send the username to external systems.

          These are the places where we should clean the user names imo:
          1/ user signup - spaces and extra chars are hard to replicate later; complain if extra chars there only, no automatic cleaning
          2/ internal user creation - teacher/admin creates a new user account manually, again it would be difficult to deal with special chars
          3/ csv upload to internal accounts only

          The internal accounts are those with authentication plugin {email, manual, db with local password, etc.)

          Show
          Petr Škoda added a comment - hmm, I thought the only reason to prevent some characters is to prevent user problems when entering the username into the login page, technically we should handle all characters in all auth methods, of course there were critical problems with special characters in some auth plugins causing fatal errors before, but all those problems should be fixed anyway. This means that if the username already exists in other system we should use it "as is", no cleaning or case conversion at all. I was thinking a lot about the lowercasing too, it is kind of similar because we should be able to interface with systems that differentiate case while still keeping the caseless unique usernames - so definitely we can not convert the case before we send the username to external systems. These are the places where we should clean the user names imo: 1/ user signup - spaces and extra chars are hard to replicate later; complain if extra chars there only, no automatic cleaning 2/ internal user creation - teacher/admin creates a new user account manually, again it would be difficult to deal with special chars 3/ csv upload to internal accounts only The internal accounts are those with authentication plugin {email, manual, db with local password, etc.)

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: