Moodle
  1. Moodle
  2. MDL-42884

Can not delete users with invalid emails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6.1
    • Component/s: Other
    • Labels:
    • Story Points:
      5
    • Rank:
      54756
    • Sprint:
      BACKEND Sprint 7

      Description

      The given username contains invalid characters

      More information about this error
      Debug info:
      Error code: invalidusername
      Stack trace:

      line 113 of /user/lib.php: moodle_exception thrown
      line 4213 of /lib/moodlelib.php: call to user_update_user()
      line 84 of /admin/user.php: call to delete_user()

        Issue Links

          Activity

          Hide
          Mark Nelson added a comment -

          When bulk uploading users with an invalid email, and then trying to delete them, it will fail because it concatenates the email to the users name, and in this case it screws everything up during the checking on line 84, which is "if ($user->username !== clean_param($user->username, PARAM_USERNAME)) {".

          Show
          Mark Nelson added a comment - When bulk uploading users with an invalid email, and then trying to delete them, it will fail because it concatenates the email to the users name, and in this case it screws everything up during the checking on line 84, which is "if ($user->username !== clean_param($user->username, PARAM_USERNAME)) {".
          Hide
          Mark Nelson added a comment -

          Sending for peer review, not sure if it should be backported as we can still delete the users in 2.4 and 2.5 without any warning as we do not call the function user_update_user when deleting a user, we simply update the DB record, so the check mentioned in my previous comment is not performed. Still, do we want incorrect names stored in the DB for deleted users? Does it matter if the user is deleted? Thoughts?

          Show
          Mark Nelson added a comment - Sending for peer review, not sure if it should be backported as we can still delete the users in 2.4 and 2.5 without any warning as we do not call the function user_update_user when deleting a user, we simply update the DB record, so the check mentioned in my previous comment is not performed. Still, do we want incorrect names stored in the DB for deleted users? Does it matter if the user is deleted? Thoughts?
          Hide
          Mark Nelson added a comment -

          Petr Škoda your opinion here would be appreciated, thanks.

          Show
          Mark Nelson added a comment - Petr Škoda your opinion here would be appreciated, thanks.
          Hide
          Frédéric Massart added a comment -

          Hi Mark,

          I think your patch will work and seems to be a proper workaround, but I am unsure it should be used like that. The problem definitely comes from the fact that the bulk upload does not have a proper clean up of the parameters. Your CSV should not have spaces before and after the email address.

          While we could keep your patch to prevent unexpected error when we try to delete the users, I think we should fix the bulk user import to clean the email address before we save it. And perhaps also adding an upgrading script to trim the email addresses.

          I'll leave the decision up to you.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Mark, I think your patch will work and seems to be a proper workaround, but I am unsure it should be used like that. The problem definitely comes from the fact that the bulk upload does not have a proper clean up of the parameters. Your CSV should not have spaces before and after the email address. While we could keep your patch to prevent unexpected error when we try to delete the users, I think we should fix the bulk user import to clean the email address before we save it. And perhaps also adding an upgrading script to trim the email addresses. I'll leave the decision up to you. Cheers, Fred
          Hide
          Petr Škoda added a comment -

          This sounds wrong, the PARAM_USERNAME cleaning should be in top levels only, low level API should not use it at all in my opinion.

          Show
          Petr Škoda added a comment - This sounds wrong, the PARAM_USERNAME cleaning should be in top levels only, low level API should not use it at all in my opinion.
          Hide
          Mark Nelson added a comment -

          Fred - MDL-42547 addresses the issue with emails being inserted into the DB with spaces. I am not sure about an upgrade path, seems over complicated. We would have to loop through the whole user table and fix any emails that contains spaces at the start or end of the email. What are you suggestions Petr?

          Show
          Mark Nelson added a comment - Fred - MDL-42547 addresses the issue with emails being inserted into the DB with spaces. I am not sure about an upgrade path, seems over complicated. We would have to loop through the whole user table and fix any emails that contains spaces at the start or end of the email. What are you suggestions Petr?
          Hide
          Mark Nelson added a comment -

          Petr, I am pushing this for integration. How else are we going to verify the conditional check I mentioned in my first comment? Do we chuck in some hack in the user_update_user function so that when the user is deleted the check isn't performed? Or do we simply leave the user stuck in the system? No valid solution was given, so submitting for integration as Fred has OKed it. Would appreciated an integrators opinion.

          Show
          Mark Nelson added a comment - Petr, I am pushing this for integration. How else are we going to verify the conditional check I mentioned in my first comment? Do we chuck in some hack in the user_update_user function so that when the user is deleted the check isn't performed? Or do we simply leave the user stuck in the system? No valid solution was given, so submitting for integration as Fred has OKed it. Would appreciated an integrators opinion.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This sounds wrong, the PARAM_USERNAME cleaning should be in top levels only, low level API should not use it at all in my opinion.

          (lol, sorry could not resist, ignore this - for now). :-D

          Show
          Eloy Lafuente (stronk7) added a comment - This sounds wrong, the PARAM_USERNAME cleaning should be in top levels only, low level API should not use it at all in my opinion. (lol, sorry could not resist, ignore this - for now). :-D
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Petr Škoda added a comment -

          Just recently I saw a funny variable incrementing "feature" in PHP, I suppose we should replace $delname++; with something better. I am goin to look at this first thing in the Monday morning.

          Show
          Petr Škoda added a comment - Just recently I saw a funny variable incrementing "feature" in PHP, I suppose we should replace $delname++; with something better. I am goin to look at this first thing in the Monday morning.
          Hide
          Dan Poltawski added a comment -

          Integrated to master and 26 - thanks Mark

          Show
          Dan Poltawski added a comment - Integrated to master and 26 - thanks Mark
          Hide
          Andrew Nicols added a comment -

          Easy peasy lemon squeezy.

          Show
          Andrew Nicols added a comment - Easy peasy lemon squeezy.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ...
          But still, I thank you, for you made me stronger…

          Stronger as the beast that roar in the wild
          Stronger as the storm across the ocean
          Stronger as the diamond that won’t break
          Stronger enough to take all heart aches….

          I thank you my friend, for you made me stronger…

          ---- Juneah Landicho

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile