Moodle
  1. Moodle
  2. MDL-20941

Store hashed username into user->email field for deleted users

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.10, 1.9.6
    • Fix Version/s: 1.8.11, 1.9.7
    • Component/s: Administration, Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      35666

      Description

      Right now, when one user is deleted from Moodle server this happens (delete_user() function):

      • user->username is set to user->email + random string
      • user->email is cleaned (empty string)

      So we lost any original username information forever.

      And that is good in the sense that leaves the username free to be reused by other user later. But it's definitively wrong when we try to import (csv, restore...) users and perform various checks to see if one user already existed. And that can lead to "revived" users that shouldn't be there.

      So, in order to keep the deleted username free to be reused, we are going to start storing the md5 hash of the username into the email field, so utilities creating users will be able to compare that hash in order to detect if the user already existed or no.

      Obviously that md5 only will be there for users deleted in the future and old deletions will continue having empty strings forever, so import utilities only will rely on that hash if not empty (this is one important point).

      So, starting now, we are going to do this on user deletion:

      • user->username is set to user->email + random string (no change here)
      • user->email is set to md5(user->username)

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          adding some watchers to let them know.

          Show
          Eloy Lafuente (stronk7) added a comment - adding some watchers to let them know.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Resolving as fixed.

          Remember we always must check it's not empty if we want' to rely on it for loading/restoring users.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Resolving as fixed. Remember we always must check it's not empty if we want' to rely on it for loading/restoring users. Ciao
          Hide
          Martín Langhoff added a comment -

          Hi Eloy,

          this fix breaks things If you look at the CVS logs, you'll notice we keep playing revert wars on this. I remove the data-munging-on-delete, someone else adds it again :-/

          Here is what happens:

          • Auth plugins delete users if they see the user disappear from the 'backend'. However, the user may 'revive'/'reappear'. The disappearance may have been temporary (bugs in the backend data export that are fixed, students that take a semester off and return). In those cases we must retain the user identity and revive the user, so their old data is re-associated to the same user.

          And for that, we need to retain username and idnumber... and often email.

          • The default email self-registration machinery has no "revive user" mechanism, so when I "fix" auth plugins by keeping data intact upon deletion, users complain because they cannot "re-register". The easy fix is to remove the old data (what your patch does).

          IMHO, the right thing is:

          • keep the original data, don't corrupt it
          • add a "your username / email addr is in our DB!, want to reset your password?" workflow that knows how to revive the user acct.
          Show
          Martín Langhoff added a comment - Hi Eloy, this fix breaks things If you look at the CVS logs, you'll notice we keep playing revert wars on this. I remove the data-munging-on-delete, someone else adds it again :-/ Here is what happens: Auth plugins delete users if they see the user disappear from the 'backend'. However, the user may 'revive'/'reappear'. The disappearance may have been temporary (bugs in the backend data export that are fixed, students that take a semester off and return). In those cases we must retain the user identity and revive the user, so their old data is re-associated to the same user. And for that, we need to retain username and idnumber... and often email. The default email self-registration machinery has no "revive user" mechanism, so when I "fix" auth plugins by keeping data intact upon deletion, users complain because they cannot "re-register". The easy fix is to remove the old data (what your patch does). IMHO, the right thing is: keep the original data, don't corrupt it add a "your username / email addr is in our DB!, want to reset your password?" workflow that knows how to revive the user acct.
          Hide
          Martín Langhoff added a comment -

          Hm... I had mainly read the patch, not the bug. I see what you are trying to do with the hash.

          I still think the right way is to preserve the real data, and get the "revive user acct" thing going, probably hooked w the password reset.

          Show
          Martín Langhoff added a comment - Hm... I had mainly read the patch, not the bug. I see what you are trying to do with the hash. I still think the right way is to preserve the real data, and get the "revive user acct" thing going, probably hooked w the password reset.
          Hide
          Petr Škoda added a comment -

          I disagree, there is no such thing as undeleting/reviving of users because we have to delete a lot of user settings, preferences and data when deleting users, sorry

          it is not possible to cope with cases where users are appearing and disappearing in external systems, instead of deleting those "unreliable" auth plugins should disable the account somehow (I already proposed a new db field for that in 2.0, others did not like it, well I might try to push it through again during the hackfest)

          technically Eloy's latest change does not remove more data imho, instead it helps to identify deleted users - I do not see any problem in that

          I agree this needs to be addressed properly in 2.0, this is also related to enrolments.

          Show
          Petr Škoda added a comment - I disagree, there is no such thing as undeleting/reviving of users because we have to delete a lot of user settings, preferences and data when deleting users, sorry it is not possible to cope with cases where users are appearing and disappearing in external systems, instead of deleting those "unreliable" auth plugins should disable the account somehow (I already proposed a new db field for that in 2.0, others did not like it, well I might try to push it through again during the hackfest) technically Eloy's latest change does not remove more data imho, instead it helps to identify deleted users - I do not see any problem in that I agree this needs to be addressed properly in 2.0, this is also related to enrolments.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, yes, the bug is about to keep more than previously, as far as before the change the original name was completely lost and now, at least, we have its md5().

          And that md5() will allow others (restore, import users, auth plugins...) to detect better deleted users (by md5(username)). It was simple impossible to do that with confidence before.

          About reviving, rekilling, exterminating and friends, I really haven't a clear idea at all (but the basic notion that the record (the id) must survive forever in the DB. Surely, with some clever rework of auth plugins and friends, we'll be able to do those things in an standard way. In fact, having the username hash used for that... anyway that is another story.

          So, if changing email DB contents (of deleted users) from empty() to md5(username) doesn't affect anything, I'd keep that way forever/until some better alternative arrive. I'm going to get immediate benefits matching deleted users on restore (avoiding current "revive everybody" wrong approach).

          Thanks ML!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, yes, the bug is about to keep more than previously, as far as before the change the original name was completely lost and now, at least, we have its md5(). And that md5() will allow others (restore, import users, auth plugins...) to detect better deleted users (by md5(username)). It was simple impossible to do that with confidence before. About reviving, rekilling, exterminating and friends, I really haven't a clear idea at all (but the basic notion that the record (the id) must survive forever in the DB. Surely, with some clever rework of auth plugins and friends, we'll be able to do those things in an standard way. In fact, having the username hash used for that... anyway that is another story. So, if changing email DB contents (of deleted users) from empty() to md5(username) doesn't affect anything, I'd keep that way forever/until some better alternative arrive. I'm going to get immediate benefits matching deleted users on restore (avoiding current "revive everybody" wrong approach). Thanks ML! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          so I'm re-resolving this as fixed, please if have more concerns about that change breaking anything, re-open and comment asap. We are going to release 1.9.7 / 1.8.11 in hours.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - so I'm re-resolving this as fixed, please if have more concerns about that change breaking anything, re-open and comment asap. We are going to release 1.9.7 / 1.8.11 in hours. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: