Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-17065

No easy way to remove pictures of deleted users

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: 1.8.8, 1.9.4
    • Component/s: Administration
    • Labels:
      None

      Description

      If a user account has been deleted, their picture remains attached to forum posts. However, if the admin decides that they don't like the picture (say because it is offensive) there is now no way to delete or edit it.

      Why do profiles of deleted users simply not remain editable to admins? There could even be a simple (and useful) undelete function!

        Gliffy Diagrams

        1. f1.png
          5 kB
        2. f2.png
          1 kB

          Issue Links

            Activity

            Hide
            howardsmiller Howard Miller added a comment -

            It's worse than that. If you have a link to the picture via pix.php then it's viewable by anyone. Surely, pix.php should check if the picture associated with a user is "deleted" when accessed by guest users?

            Show
            howardsmiller Howard Miller added a comment - It's worse than that. If you have a link to the picture via pix.php then it's viewable by anyone . Surely, pix.php should check if the picture associated with a user is "deleted" when accessed by guest users?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Assigning to Petr. I agree with Howard about pictures of deleted users not being accessible anymore:

            1) at least from guests.
            2) perhaps from everybody.

            Not sure if that implies deleting the picture or just detecting it and sending the "default" image for deleted users.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Assigning to Petr. I agree with Howard about pictures of deleted users not being accessible anymore: 1) at least from guests. 2) perhaps from everybody. Not sure if that implies deleting the picture or just detecting it and sending the "default" image for deleted users. Ciao
            Hide
            aborrow Anthony Borrow added a comment -

            I wonder if instead of actually deleting the picture instead we just do a check and if it is a deleted picture then use the generic smiley face or perhaps even a grayed out smiley face to indicate that it is a deleted user. It could be as simple as using grayscale images (which I'll attach) or overlaying 'Deleted User' or something over the image. I think our general approach of not actually removing data is good as it does leave the door open should we want at some point to implement a restore/undelete user option via the UI. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - I wonder if instead of actually deleting the picture instead we just do a check and if it is a deleted picture then use the generic smiley face or perhaps even a grayed out smiley face to indicate that it is a deleted user. It could be as simple as using grayscale images (which I'll attach) or overlaying 'Deleted User' or something over the image. I think our general approach of not actually removing data is good as it does leave the door open should we want at some point to implement a restore/undelete user option via the UI. Peace - Anthony
            Hide
            aborrow Anthony Borrow added a comment -

            ATW was asking about voting on this issue. I'm guessing that because it is a security issue that normal users cannot vote on it. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - ATW was asking about voting on this issue. I'm guessing that because it is a security issue that normal users cannot vote on it. Peace - Anthony
            Hide
            aborrow Anthony Borrow added a comment -

            here are the grayscaled f1 and f2 files

            Show
            aborrow Anthony Borrow added a comment - here are the grayscaled f1 and f2 files
            Hide
            aborrow Anthony Borrow added a comment -

            Just a quick follow up, my idea would be to make a small modification to the print_user_picture function in /lib/weblib.php to check if the user is deleted and if so to use the deleted user image files.

            Show
            aborrow Anthony Borrow added a comment - Just a quick follow up, my idea would be to make a small modification to the print_user_picture function in /lib/weblib.php to check if the user is deleted and if so to use the deleted user image files.
            Hide
            skodak Petr Skoda added a comment -

            I have added a test into user/pix.php to validate that user is not deleted, if yes default image is used.
            Changing print_user_picture might not be enough because the images are also stored in emails

            There is no "undelete user" feature planned (either manual or automatic) and I would strongly oppose it, if you really want it you can use db editor or some custom script.
            Instead we should have easy to use "user suspend" and proper user delete+purge - we must clean up the old data because it is becoming a real performance problem.

            I am planning to rework the profile image handling in 2.0 - I suppose it will use the same trick as SCORM to work around the caching problems, I agree the gray images is a good idea (+1 for HEAD).

            Show
            skodak Petr Skoda added a comment - I have added a test into user/pix.php to validate that user is not deleted, if yes default image is used. Changing print_user_picture might not be enough because the images are also stored in emails There is no "undelete user" feature planned (either manual or automatic) and I would strongly oppose it, if you really want it you can use db editor or some custom script. Instead we should have easy to use "user suspend" and proper user delete+purge - we must clean up the old data because it is becoming a real performance problem. I am planning to rework the profile image handling in 2.0 - I suppose it will use the same trick as SCORM to work around the caching problems, I agree the gray images is a good idea (+1 for HEAD).
            Hide
            howardsmiller Howard Miller added a comment -

            I agree with you, Petr. However, the underlying problem is that delete was never actually delete at all. It creates an expectation that is not true. Simply renaming it to suspend would have helped.

            Show
            howardsmiller Howard Miller added a comment - I agree with you, Petr. However, the underlying problem is that delete was never actually delete at all. It creates an expectation that is not true. Simply renaming it to suspend would have helped.
            Hide
            skodak Petr Skoda added a comment -

            well, there is a big difference between delete and suspend
            1/ delete should delete stuff that is not needed like assignment submissions (no more user access expected) + purge should remove all traces of users including posts, etc.
            2/ suspend should just prevent logins while keeping ALL user data and preventing creation of account with the same email/username

            I believe that we need enrolment suspend and proper delete too. It is very similar to this problem imho.

            Show
            skodak Petr Skoda added a comment - well, there is a big difference between delete and suspend 1/ delete should delete stuff that is not needed like assignment submissions (no more user access expected) + purge should remove all traces of users including posts, etc. 2/ suspend should just prevent logins while keeping ALL user data and preventing creation of account with the same email/username I believe that we need enrolment suspend and proper delete too. It is very similar to this problem imho.
            Hide
            tsala Helen Foster added a comment -

            Thanks for reporting this issue Howard, thanks for everyone's comments, and thanks for fixing it Petr

            Show
            tsala Helen Foster added a comment - Thanks for reporting this issue Howard, thanks for everyone's comments, and thanks for fixing it Petr
            Hide
            dougiamas Martin Dougiamas added a comment -

            I'm removing security status to open this up.

            Show
            dougiamas Martin Dougiamas added a comment - I'm removing security status to open this up.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  28/Jan/09