From 562e4be2c96605a7b276ad69a0e070e3c7c2ce56 Mon Sep 17 00:00:00 2001
From: Carrie Ganote <cganote@iu.edu>
Date: Mon, 8 Jul 2019 10:24:34 -0400
Subject: [PATCH 01/14] Refactored the controller vs. manager for the deletion
 and purge of users. Purge now wipes out the username (regardless of gdrp
 setting) and takes out histories. Undelete should now work.

---
 lib/galaxy/managers/users.py                  | 105 +++++++++++++++++-
 lib/galaxy/webapps/galaxy/api/users.py        |  25 +++--
 .../webapps/galaxy/controllers/admin.py       | 105 +++---------------
 3 files changed, 134 insertions(+), 101 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index 1c2cb729a908..7acb33d42c24 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -4,7 +4,9 @@
 import logging
 import random
 import socket
+import time
 from datetime import datetime
+from galaxy.util.hash_util import new_secure_hash
 
 from markupsafe import escape
 from sqlalchemy import and_, desc, exc, func, true
@@ -37,7 +39,6 @@
 can also copy and paste it into your browser.
 """
 
-
 class UserManager(base.ModelManager, deletable.PurgableManagerMixin):
     foreign_key_name = 'user'
 
@@ -109,7 +110,109 @@ def create(self, email=None, username=None, password=None, **kwargs):
         return user
 
     def delete(self, user):
+        if not self.app.config.allow_user_deletion:
+            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
+        # Check first if it is already deleted. If so, say so and do nothing.
+        # Unsure if we should add username to the log.debug or if that breaks the gdrp support
+        if user.deleted:
+            log.debug("User already deleted, but setting delete flag just in case.")
+        if user.purged:
+            log.debug("Cannot delete purged user, but setting delete flag just in case.")
+
         user.deleted = True
+        log.debug("User %s deleted" % user.username)
+        self.session().add(user)
+        self.session().flush()
+
+    def undelete(self, user):
+        log.debug("Requested undelete of user %s" % user.username)
+        if not self.app.config.allow_user_deletion:
+            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
+        if user.purged:
+            raise exceptions.ItemDeletionException('Purged user cannot be undeleted.')
+        if not user.deleted:
+            log.debug('User \'%s\' has not been deleted, so it cannot be undeleted.' % user.email)
+        user.deleted = False
+        self.session().add(user)
+        self.session().flush()
+
+    def purge(self, user):
+        log.debug("Requested purge of user %s" % user)
+        if not self.app.config.allow_user_deletion:
+            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
+        if not user.deleted:
+            raise exceptions.MessageException('User \'%s\' has not been deleted, so it cannot be purged.' % user.email, 'error')
+        else:
+            log.debug("Purging user %s" % user)
+        private_role = self.app.security_agent.get_private_user_role(user)
+        # Delete History
+        for active_history in user.active_histories:
+            self.session().refresh(active_history)
+            for hda in active_history.active_datasets:
+                # Delete HistoryDatasetAssociation
+                hda.deleted = True
+                self.session().add(hda)
+            active_history.deleted = True
+            self.session().add(active_history)
+        # Delete UserGroupAssociations
+        for uga in user.groups:
+            self.session().delete(uga)
+        # Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE
+        for ura in user.roles:
+            if ura.role_id != private_role.id:
+                self.session().delete(ura)
+        # Delete UserAddresses
+        for address in user.addresses:
+            self.session().delete(address)
+        # Purge the user
+        user.purged = True
+        compliance_log = logging.getLogger('COMPLIANCE')
+        compliance_log.info('delete-user-event: %s' % user.username)
+        # Maybe there is some case in the future where an admin needs
+        # to prove that a user was using a server for some reason (e.g.
+        # a court case.) So we make this painfully hard to recover (and
+        # not immediately reversable) in line with GDPR, but still
+        # leave open the possibility to prove someone was part of the
+        # server just in case. By knowing the exact email + approximate
+        # time of deletion, one could run through hashes for every
+        # second of the surrounding days/weeks.
+        pseudorandom_value = str(int(time.time()))
+        # Replace email + username with a (theoretically) unreversable
+        # hash. If provided with the username we can probably re-hash
+        # to identify if it is needed for some reason.
+        #
+        # Deleting multiple times will re-hash the username/email
+        email_hash = new_secure_hash(user.email + pseudorandom_value)
+        uname_hash = new_secure_hash(user.username + pseudorandom_value)
+        # We must also redact username
+        for role in user.all_roles():
+            if self.app.config.redact_username_during_deletion:
+                role.name = role.name.replace(user.username, uname_hash)
+                role.description = role.description.replace(user.username, uname_hash)
+
+            if self.app.config.redact_email_during_deletion:
+                role.name = role.name.replace(user.email, email_hash)
+                role.description = role.description.replace(user.email, email_hash)
+            user.email = email_hash
+            user.username = uname_hash
+        log.debug("User credentials changed. email is now %s and name is %s" % (user.email, user.username))
+        # Redact user addresses as well
+        if self.app.config.redact_user_address_during_deletion:
+            user_addresses = self.session().query(trans.app.model.UserAddress) \
+                .filter(trans.app.model.UserAddress.user_id == user.id).all()
+
+            for addr in user_addresses:
+                addr.desc = new_secure_hash(addr.desc + pseudorandom_value)
+                addr.name = new_secure_hash(addr.name + pseudorandom_value)
+                addr.institution = new_secure_hash(addr.institution + pseudorandom_value)
+                addr.address = new_secure_hash(addr.address + pseudorandom_value)
+                addr.city = new_secure_hash(addr.city + pseudorandom_value)
+                addr.state = new_secure_hash(addr.state + pseudorandom_value)
+                addr.postal_code = new_secure_hash(addr.postal_code + pseudorandom_value)
+                addr.country = new_secure_hash(addr.country + pseudorandom_value)
+                addr.phone = new_secure_hash(addr.phone + pseudorandom_value)
+                self.session().add(addr)
+
         self.session().add(user)
         self.session().flush()
 
diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py
index c394af5824f0..f1951e488b9e 100644
--- a/lib/galaxy/webapps/galaxy/api/users.py
+++ b/lib/galaxy/webapps/galaxy/api/users.py
@@ -35,7 +35,6 @@
     expose_api,
     expose_api_anonymous
 )
-from galaxy.web.form_builder import AddressField
 from galaxy.webapps.base.controller import (
     BaseAPIController,
     BaseUIController,
@@ -43,6 +42,7 @@
     UsesFormDefinitionsMixin,
     UsesTagsMixin
 )
+from galaxy.web.form_builder import AddressField
 
 
 log = logging.getLogger(__name__)
@@ -240,19 +240,28 @@ def delete(self, trans, id, **kwd):
         :param purge: (optional) if True, purge the user
         :type  purge: bool
         """
-        if not trans.app.config.allow_user_deletion:
-            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete users.')
+        user = self.get_user(trans, id)
         purge = util.string_as_bool(kwd.get('purge', False))
         if purge:
-            raise exceptions.NotImplemented('Purge option has not been implemented yet')
-        user = self.get_user(trans, id)
-        self.user_manager.delete(user)
+            log.debug("Purging user from api %s" % user)
+            self.user_manager.purge(user)
+        else:
+            self.user_manager.delete(user)
         return self.user_serializer.serialize_to_view(user, view='detailed')
 
     @expose_api
     @web.require_admin
-    def undelete(self, trans, **kwd):
-        raise exceptions.NotImplemented()
+    def undelete(self, trans, id, **kwd):
+        """
+        UNDELETE /api/users/{id}
+        undelete the user with the given ``id``
+
+        :param id: the encoded id of the user to be undeleted
+        :type  id: str
+        """
+        user = self.get_user(trans, id)
+        self.user_manager.undelete(user)
+        return self.user_serializer.serialize_to_view(user, view='detailed')
 
     # TODO: move to more basal, common resource than this
     def anon_user_api_value(self, trans):
diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py
index 0dc8a1930ab5..0beadd4f8d95 100644
--- a/lib/galaxy/webapps/galaxy/controllers/admin.py
+++ b/lib/galaxy/webapps/galaxy/controllers/admin.py
@@ -37,7 +37,6 @@
 
 
 log = logging.getLogger(__name__)
-compliance_log = logging.getLogger('COMPLIANCE')
 
 
 class UserListGrid(grids.Grid):
@@ -160,14 +159,14 @@ def get_value(self, trans, grid, user):
         grids.GridOperation("Generate New API Key",
                             allow_multiple=False,
                             async_compatible=True)
-
     ]
+
     standard_filters = [
         grids.GridColumnFilter("Active", args=dict(deleted=False)),
         grids.GridColumnFilter("Deleted", args=dict(deleted=True, purged=False)),
         grids.GridColumnFilter("Purged", args=dict(purged=True)),
         grids.GridColumnFilter("All", args=dict(deleted='All'))
-    ]
+        ]
     num_rows_per_page = 50
     use_paging = True
     default_filter = dict(purged="False")
@@ -460,6 +459,7 @@ def get_value(self, trans, grid, quota):
     standard_filters = [
         grids.GridColumnFilter("Active", args=dict(deleted=False)),
         grids.GridColumnFilter("Deleted", args=dict(deleted=True)),
+        grids.GridColumnFilter("Purged", args=dict(purged=True)),
         grids.GridColumnFilter("All", args=dict(deleted='All'))
     ]
     num_rows_per_page = 50
@@ -527,12 +527,13 @@ class AdminGalaxy(controller.JSAppLauncher, AdminActions, UsesQuotaMixin, QuotaP
     group_list_grid = GroupListGrid()
     quota_list_grid = QuotaListGrid()
     tool_version_list_grid = ToolVersionListGrid()
-    delete_operation = grids.GridOperation("Delete", condition=(lambda item: not item.deleted), allow_multiple=True)
+    delete_operation = grids.GridOperation("Delete", condition=(lambda item: not item.deleted and not item.purged), allow_multiple=True)
     undelete_operation = grids.GridOperation("Undelete", condition=(lambda item: item.deleted and not item.purged), allow_multiple=True)
     purge_operation = grids.GridOperation("Purge", condition=(lambda item: item.deleted and not item.purged), allow_multiple=True)
     impersonate_operation = grids.GridOperation(
         "Impersonate",
         url_args=dict(controller="admin", action="impersonate"),
+        condition=(lambda item: not item.deleted and not item.purged),
         allow_multiple=False
     )
 
@@ -1411,61 +1412,9 @@ def _delete_user(self, trans, ids):
         message = 'Deleted %d users: ' % len(ids)
         for user_id in ids:
             user = get_user(trans, user_id)
-            user.deleted = True
-
-            compliance_log.info('delete-user-event: %s' % user_id)
-
-            # Maybe there is some case in the future where an admin needs
-            # to prove that a user was using a server for some reason (e.g.
-            # a court case.) So we make this painfully hard to recover (and
-            # not immediately reversable) in line with GDPR, but still
-            # leave open the possibility to prove someone was part of the
-            # server just in case. By knowing the exact email + approximate
-            # time of deletion, one could run through hashes for every
-            # second of the surrounding days/weeks.
-            pseudorandom_value = str(int(time.time()))
-            # Replace email + username with a (theoretically) unreversable
-            # hash. If provided with the username we can probably re-hash
-            # to identify if it is needed for some reason.
-            #
-            # Deleting multiple times will re-hash the username/email
-            email_hash = new_secure_hash(user.email + pseudorandom_value)
-            uname_hash = new_secure_hash(user.username + pseudorandom_value)
-
-            # We must also redact username
-            for role in user.all_roles():
-                if self.app.config.redact_username_during_deletion:
-                    role.name = role.name.replace(user.username, uname_hash)
-                    role.description = role.description.replace(user.username, uname_hash)
-
-                if self.app.config.redact_email_during_deletion:
-                    role.name = role.name.replace(user.email, email_hash)
-                    role.description = role.description.replace(user.email, email_hash)
-
-            if self.app.config.redact_email_during_deletion:
-                user.email = email_hash
-            if self.app.config.redact_username_during_deletion:
-                user.username = uname_hash
-
-            # Redact user addresses as well
-            if self.app.config.redact_user_address_during_deletion:
-                user_addresses = trans.sa_session.query(trans.app.model.UserAddress) \
-                    .filter(trans.app.model.UserAddress.user_id == user.id).all()
-
-                for addr in user_addresses:
-                    addr.desc = new_secure_hash(addr.desc + pseudorandom_value)
-                    addr.name = new_secure_hash(addr.name + pseudorandom_value)
-                    addr.institution = new_secure_hash(addr.institution + pseudorandom_value)
-                    addr.address = new_secure_hash(addr.address + pseudorandom_value)
-                    addr.city = new_secure_hash(addr.city + pseudorandom_value)
-                    addr.state = new_secure_hash(addr.state + pseudorandom_value)
-                    addr.postal_code = new_secure_hash(addr.postal_code + pseudorandom_value)
-                    addr.country = new_secure_hash(addr.country + pseudorandom_value)
-                    addr.phone = new_secure_hash(addr.phone + pseudorandom_value)
-                    trans.sa_session.add(addr)
-
-            trans.sa_session.add(user)
-            trans.sa_session.flush()
+            # Actually do the delete
+            self.user_manager.delete(user)
+            # Accumulate messages for the return message
             message += ' %s ' % user.email
         return (message, 'done')
 
@@ -1474,12 +1423,9 @@ def _undelete_user(self, trans, ids):
         undeleted_users = ""
         for user_id in ids:
             user = get_user(trans, user_id)
-            if not user.deleted:
-                message = 'User \'%s\' has not been deleted, so it cannot be undeleted.' % user.email
-                return (message, 'error')
-            user.deleted = False
-            trans.sa_session.add(user)
-            trans.sa_session.flush()
+            # Actually do the undelete
+            self.user_manager.undelete(user)
+            # Count and accumulate messages to return to the admin panel
             count += 1
             undeleted_users += ' %s' % user.email
         message = 'Undeleted %d users: %s' % (count, undeleted_users)
@@ -1500,33 +1446,8 @@ def _purge_user(self, trans, ids):
         message = 'Purged %d users: ' % len(ids)
         for user_id in ids:
             user = get_user(trans, user_id)
-            if not user.deleted:
-                return ('User \'%s\' has not been deleted, so it cannot be purged.' % user.email, 'error')
-            private_role = trans.app.security_agent.get_private_user_role(user)
-            # Delete History
-            for h in user.active_histories:
-                trans.sa_session.refresh(h)
-                for hda in h.active_datasets:
-                    # Delete HistoryDatasetAssociation
-                    hda.deleted = True
-                    trans.sa_session.add(hda)
-                h.deleted = True
-                trans.sa_session.add(h)
-            # Delete UserGroupAssociations
-            for uga in user.groups:
-                trans.sa_session.delete(uga)
-            # Delete UserRoleAssociations EXCEPT FOR THE PRIVATE ROLE
-            for ura in user.roles:
-                if ura.role_id != private_role.id:
-                    trans.sa_session.delete(ura)
-            # Delete UserAddresses
-            for address in user.addresses:
-                trans.sa_session.delete(address)
-            # Purge the user
-            user.purged = True
-            trans.sa_session.add(user)
-            trans.sa_session.flush()
-            message += '%s ' % user.email
+            self.user_manager.purge(user)
+            message += '\t%s\n ' % user.email
         return (message, 'done')
 
     def _recalculate_user(self, trans, user_id):

From 8ae5ca3905fe3a40e74d02a345d112db2c1b9154 Mon Sep 17 00:00:00 2001
From: Carrie Ganote <cganote@iu.edu>
Date: Mon, 8 Jul 2019 11:43:39 -0400
Subject: [PATCH 02/14] Fixes, I hope!

---
 lib/galaxy/managers/users.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index 7acb33d42c24..2d3280e0ccdd 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -198,8 +198,8 @@ def purge(self, user):
         log.debug("User credentials changed. email is now %s and name is %s" % (user.email, user.username))
         # Redact user addresses as well
         if self.app.config.redact_user_address_during_deletion:
-            user_addresses = self.session().query(trans.app.model.UserAddress) \
-                .filter(trans.app.model.UserAddress.user_id == user.id).all()
+            user_addresses = self.session().query(self.app.model.UserAddress) \
+                .filter(self.app.model.UserAddress.user_id == user.id).all()
 
             for addr in user_addresses:
                 addr.desc = new_secure_hash(addr.desc + pseudorandom_value)

From 2111321bdcf1c3c55e45172473b5ecc66ccc99b2 Mon Sep 17 00:00:00 2001
From: Martin Cech <marten@bx.psu.edu>
Date: Thu, 11 Jul 2019 15:23:32 -0400
Subject: [PATCH 03/14] style fixes

---
 lib/galaxy/managers/users.py                   | 3 ++-
 lib/galaxy/webapps/galaxy/api/users.py         | 2 +-
 lib/galaxy/webapps/galaxy/controllers/admin.py | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index 2d3280e0ccdd..49c95eb736a2 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -6,7 +6,6 @@
 import socket
 import time
 from datetime import datetime
-from galaxy.util.hash_util import new_secure_hash
 
 from markupsafe import escape
 from sqlalchemy import and_, desc, exc, func, true
@@ -22,6 +21,7 @@
     deletable
 )
 from galaxy.security.validate_user_input import validate_email, validate_password, validate_publicname
+from galaxy.util.hash_util import new_secure_hash
 from galaxy.web import url_for
 
 log = logging.getLogger(__name__)
@@ -39,6 +39,7 @@
 can also copy and paste it into your browser.
 """
 
+
 class UserManager(base.ModelManager, deletable.PurgableManagerMixin):
     foreign_key_name = 'user'
 
diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py
index f1951e488b9e..cf778c9f56a2 100644
--- a/lib/galaxy/webapps/galaxy/api/users.py
+++ b/lib/galaxy/webapps/galaxy/api/users.py
@@ -35,6 +35,7 @@
     expose_api,
     expose_api_anonymous
 )
+from galaxy.web.form_builder import AddressField
 from galaxy.webapps.base.controller import (
     BaseAPIController,
     BaseUIController,
@@ -42,7 +43,6 @@
     UsesFormDefinitionsMixin,
     UsesTagsMixin
 )
-from galaxy.web.form_builder import AddressField
 
 
 log = logging.getLogger(__name__)
diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py
index 0beadd4f8d95..e69f3639673f 100644
--- a/lib/galaxy/webapps/galaxy/controllers/admin.py
+++ b/lib/galaxy/webapps/galaxy/controllers/admin.py
@@ -166,7 +166,7 @@ def get_value(self, trans, grid, user):
         grids.GridColumnFilter("Deleted", args=dict(deleted=True, purged=False)),
         grids.GridColumnFilter("Purged", args=dict(purged=True)),
         grids.GridColumnFilter("All", args=dict(deleted='All'))
-        ]
+    ]
     num_rows_per_page = 50
     use_paging = True
     default_filter = dict(purged="False")

From eae28cd9c0387eca17d34d486c38211aa8584307 Mon Sep 17 00:00:00 2001
From: Martin Cech <marten@bx.psu.edu>
Date: Thu, 11 Jul 2019 17:50:02 -0400
Subject: [PATCH 04/14] remove some extra debugs

---
 lib/galaxy/managers/users.py | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index 49c95eb736a2..c77bd0d0e374 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -114,19 +114,16 @@ def delete(self, user):
         if not self.app.config.allow_user_deletion:
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
         # Check first if it is already deleted. If so, say so and do nothing.
-        # Unsure if we should add username to the log.debug or if that breaks the gdrp support
+        # Unsure if we should add username to the log.debug or if that breaks the GDPR support
         if user.deleted:
             log.debug("User already deleted, but setting delete flag just in case.")
         if user.purged:
             log.debug("Cannot delete purged user, but setting delete flag just in case.")
-
         user.deleted = True
-        log.debug("User %s deleted" % user.username)
         self.session().add(user)
         self.session().flush()
 
     def undelete(self, user):
-        log.debug("Requested undelete of user %s" % user.username)
         if not self.app.config.allow_user_deletion:
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
         if user.purged:
@@ -138,13 +135,10 @@ def undelete(self, user):
         self.session().flush()
 
     def purge(self, user):
-        log.debug("Requested purge of user %s" % user)
         if not self.app.config.allow_user_deletion:
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
         if not user.deleted:
             raise exceptions.MessageException('User \'%s\' has not been deleted, so it cannot be purged.' % user.email, 'error')
-        else:
-            log.debug("Purging user %s" % user)
         private_role = self.app.security_agent.get_private_user_role(user)
         # Delete History
         for active_history in user.active_histories:
@@ -196,7 +190,6 @@ def purge(self, user):
                 role.description = role.description.replace(user.email, email_hash)
             user.email = email_hash
             user.username = uname_hash
-        log.debug("User credentials changed. email is now %s and name is %s" % (user.email, user.username))
         # Redact user addresses as well
         if self.app.config.redact_user_address_during_deletion:
             user_addresses = self.session().query(self.app.model.UserAddress) \

From fca21c472fd989ab3d52fe6ed881b84b95f2f36c Mon Sep 17 00:00:00 2001
From: Martin Cech <marten@bx.psu.edu>
Date: Fri, 12 Jul 2019 10:55:52 -0400
Subject: [PATCH 05/14] minor refactoring

---
 lib/galaxy/managers/users.py           | 11 +++++++----
 lib/galaxy/webapps/galaxy/api/users.py |  6 +++---
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index c77bd0d0e374..5286c0efaa11 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -111,8 +111,9 @@ def create(self, email=None, username=None, password=None, **kwargs):
         return user
 
     def delete(self, user):
+        """Mark the given user deleted."""
         if not self.app.config.allow_user_deletion:
-            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
+            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete users.')
         # Check first if it is already deleted. If so, say so and do nothing.
         # Unsure if we should add username to the log.debug or if that breaks the GDPR support
         if user.deleted:
@@ -124,8 +125,9 @@ def delete(self, user):
         self.session().flush()
 
     def undelete(self, user):
+        """Remove the deleted flag for the given user."""
         if not self.app.config.allow_user_deletion:
-            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
+            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to undelete users.')
         if user.purged:
             raise exceptions.ItemDeletionException('Purged user cannot be undeleted.')
         if not user.deleted:
@@ -135,10 +137,11 @@ def undelete(self, user):
         self.session().flush()
 
     def purge(self, user):
+        """Purge the given user. They must have the deleted flag already."""
         if not self.app.config.allow_user_deletion:
-            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete/undelete users.')
+            raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete or purge users.')
         if not user.deleted:
-            raise exceptions.MessageException('User \'%s\' has not been deleted, so it cannot be purged.' % user.email, 'error')
+            raise exceptions.MessageException('User \'%s\' has not been deleted, so they cannot be purged.' % user.email)
         private_role = self.app.security_agent.get_private_user_role(user)
         # Delete History
         for active_history in user.active_histories:
diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py
index cf778c9f56a2..1c189b042ef8 100644
--- a/lib/galaxy/webapps/galaxy/api/users.py
+++ b/lib/galaxy/webapps/galaxy/api/users.py
@@ -243,7 +243,7 @@ def delete(self, trans, id, **kwd):
         user = self.get_user(trans, id)
         purge = util.string_as_bool(kwd.get('purge', False))
         if purge:
-            log.debug("Purging user from api %s" % user)
+            log.debug("Purging user %s" % user)
             self.user_manager.purge(user)
         else:
             self.user_manager.delete(user)
@@ -253,8 +253,8 @@ def delete(self, trans, id, **kwd):
     @web.require_admin
     def undelete(self, trans, id, **kwd):
         """
-        UNDELETE /api/users/{id}
-        undelete the user with the given ``id``
+        POST /api/users/{id}
+        Undelete the user with the given ``id``
 
         :param id: the encoded id of the user to be undeleted
         :type  id: str

From cd9af5b0e74f74d1dd0e4f1f33354948731c08a9 Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Sun, 7 Jul 2019 12:23:44 +0200
Subject: [PATCH 06/14] Add an API test for deleting users

---
 test/api/test_users.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/api/test_users.py b/test/api/test_users.py
index f1a98a121e5e..860289f7030b 100644
--- a/test/api/test_users.py
+++ b/test/api/test_users.py
@@ -74,6 +74,15 @@ def test_admin_update(self):
         update_json = update_response.json()
         self.assertEqual(update_json['username'], new_name)
 
+    def test_delete_user(self):
+        user = self._setup_user(TEST_USER_EMAIL)
+        response = self._delete("users/%s" % user["id"], admin=True)
+        updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
+        assert updated_user['deleted'] is True, updated_user
+
+    def test_purge_user():
+        pass
+
     def test_information(self):
         user = self._setup_user(TEST_USER_EMAIL)
         url = self.__url("information/inputs", user)

From b76242c5865a1a13ebac4552d4821aa982152616 Mon Sep 17 00:00:00 2001
From: Juleen Graham <jgraham@Juleens-MacBook-Pro.local>
Date: Mon, 8 Jul 2019 12:00:27 +0200
Subject: [PATCH 07/14] add api test

---
 test/api/test_users.py | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/test/api/test_users.py b/test/api/test_users.py
index 860289f7030b..e2b499f1ad5f 100644
--- a/test/api/test_users.py
+++ b/test/api/test_users.py
@@ -10,6 +10,8 @@
 from base.populators import skip_without_tool
 
 TEST_USER_EMAIL = "user_for_users_index_test@bx.psu.edu"
+TEST_USER_EMAIL1 = "user1_for_users_index_test@bx.psu.edu"
+TEST_USER_EMAIL2 = "user2_for_users_index_test@bx.psu.edu"
 
 
 class UsersApiTestCase(api.ApiTestCase):
@@ -75,13 +77,26 @@ def test_admin_update(self):
         self.assertEqual(update_json['username'], new_name)
 
     def test_delete_user(self):
-        user = self._setup_user(TEST_USER_EMAIL)
+        user = self._setup_user(TEST_USER_EMAIL1)
         response = self._delete("users/%s" % user["id"], admin=True)
         updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
         assert updated_user['deleted'] is True, updated_user
 
-    def test_purge_user():
-        pass
+    def test_purge_user(self):
+        user = self._setup_user(TEST_USER_EMAIL1)
+        response = self._delete("users/%s" % user["id"], purge=True, admin=True)
+        updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
+        assert updated_user['deleted'] is True, updated_user
+        assert updated_user['purged'] is True, updated_user
+
+    def test_undelete_user(self):
+        user = self._setup_user(TEST_USER_EMAIL2)
+        response = self._delete("users/%s" % user["id"], admin=True)
+        updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
+        assert updated_user['deleted'] is True, updated_user
+        response = self._put("users/%s" % user["id"], admin=True)
+        updated_user = self._get("users/%s" % user['id'], admin=True).json()
+        assert updated_user['deleted'] is False, updated_user
 
     def test_information(self):
         user = self._setup_user(TEST_USER_EMAIL)

From 4e7371f0e8305823eac3d50df30f726de312f498 Mon Sep 17 00:00:00 2001
From: Juleen Graham <juleengrah@gmail.com>
Date: Mon, 8 Jul 2019 13:01:12 +0200
Subject: [PATCH 08/14] Addressed lint issues

---
 test/api/test_users.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/api/test_users.py b/test/api/test_users.py
index e2b499f1ad5f..4db564858db3 100644
--- a/test/api/test_users.py
+++ b/test/api/test_users.py
@@ -78,23 +78,23 @@ def test_admin_update(self):
 
     def test_delete_user(self):
         user = self._setup_user(TEST_USER_EMAIL1)
-        response = self._delete("users/%s" % user["id"], admin=True)
+        self._delete("users/%s" % user["id"], admin=True)
         updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
         assert updated_user['deleted'] is True, updated_user
 
     def test_purge_user(self):
         user = self._setup_user(TEST_USER_EMAIL1)
-        response = self._delete("users/%s" % user["id"], purge=True, admin=True)
+        self._delete("users/%s" % user["id"], purge=True, admin=True)
         updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
         assert updated_user['deleted'] is True, updated_user
         assert updated_user['purged'] is True, updated_user
 
     def test_undelete_user(self):
         user = self._setup_user(TEST_USER_EMAIL2)
-        response = self._delete("users/%s" % user["id"], admin=True)
+        self._delete("users/%s" % user["id"], admin=True)
         updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
         assert updated_user['deleted'] is True, updated_user
-        response = self._put("users/%s" % user["id"], admin=True)
+        self._put("users/%s" % user["id"], admin=True)
         updated_user = self._get("users/%s" % user['id'], admin=True).json()
         assert updated_user['deleted'] is False, updated_user
 

From 2fa7eba30cb17d3cf3aab3b710dee1d44ce85e39 Mon Sep 17 00:00:00 2001
From: Martin Cech <marten@bx.psu.edu>
Date: Fri, 12 Jul 2019 14:52:04 -0400
Subject: [PATCH 09/14] fix tests

---
 test/api/test_users.py | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/test/api/test_users.py b/test/api/test_users.py
index 4db564858db3..7371a988bf1e 100644
--- a/test/api/test_users.py
+++ b/test/api/test_users.py
@@ -10,8 +10,9 @@
 from base.populators import skip_without_tool
 
 TEST_USER_EMAIL = "user_for_users_index_test@bx.psu.edu"
-TEST_USER_EMAIL1 = "user1_for_users_index_test@bx.psu.edu"
-TEST_USER_EMAIL2 = "user2_for_users_index_test@bx.psu.edu"
+TEST_USER_EMAIL_DELETE = "user_for_delete_test@bx.psu.edu"
+TEST_USER_EMAIL_PURGE = "user_for_purge_test@bx.psu.edu"
+TEST_USER_EMAIL_UNDELETE = "user_for_undelete_test@bx.psu.edu"
 
 
 class UsersApiTestCase(api.ApiTestCase):
@@ -77,26 +78,32 @@ def test_admin_update(self):
         self.assertEqual(update_json['username'], new_name)
 
     def test_delete_user(self):
-        user = self._setup_user(TEST_USER_EMAIL1)
+        user = self._setup_user(TEST_USER_EMAIL_DELETE)
         self._delete("users/%s" % user["id"], admin=True)
         updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
         assert updated_user['deleted'] is True, updated_user
 
     def test_purge_user(self):
-        user = self._setup_user(TEST_USER_EMAIL1)
-        self._delete("users/%s" % user["id"], purge=True, admin=True)
-        updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
-        assert updated_user['deleted'] is True, updated_user
-        assert updated_user['purged'] is True, updated_user
+        """Delete user and then purge them."""
+        user = self._setup_user(TEST_USER_EMAIL_PURGE)
+        self._delete("users/%s" % user["id"], admin=True)
+        data = dict(purge="True")
+        self._delete("users/%s" % user["id"], data=data, admin=True)
+        payload = {'deleted': "True"}
+        purged_user = self._get("users/%s" % user['id'], payload, admin=True).json()
+        assert purged_user['deleted'] is True, purged_user
+        assert purged_user['purged'] is True, purged_user
 
     def test_undelete_user(self):
-        user = self._setup_user(TEST_USER_EMAIL2)
+        """Delete user and then undelete them."""
+        user = self._setup_user(TEST_USER_EMAIL_UNDELETE)
         self._delete("users/%s" % user["id"], admin=True)
-        updated_user = self._get("users/deleted/%s" % user['id'], admin=True).json()
-        assert updated_user['deleted'] is True, updated_user
-        self._put("users/%s" % user["id"], admin=True)
-        updated_user = self._get("users/%s" % user['id'], admin=True).json()
-        assert updated_user['deleted'] is False, updated_user
+        payload = {'deleted': "True"}
+        deleted_user = self._get("users/%s" % user['id'], payload, admin=True).json()
+        assert deleted_user['deleted'] is True, deleted_user
+        self._post("users/deleted/%s/undelete" % user["id"], admin=True)
+        undeleted_user = self._get("users/%s" % user['id'], admin=True).json()
+        assert undeleted_user['deleted'] is False, undeleted_user
 
     def test_information(self):
         user = self._setup_user(TEST_USER_EMAIL)

From 2ae4737043f7c9055e89bb93f1d482e76a048b09 Mon Sep 17 00:00:00 2001
From: Martin Cech <marten@bx.psu.edu>
Date: Fri, 12 Jul 2019 14:58:50 -0400
Subject: [PATCH 10/14] bubble up exception without masking, adjust docstring

---
 lib/galaxy/webapps/galaxy/api/users.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py
index 1c189b042ef8..01546f7ef03c 100644
--- a/lib/galaxy/webapps/galaxy/api/users.py
+++ b/lib/galaxy/webapps/galaxy/api/users.py
@@ -168,6 +168,8 @@ def show(self, trans, id, deleted='False', **kwd):
             if not trans.user_is_admin:
                 assert trans.user == user
                 assert not user.deleted
+        except exceptions.ItemDeletionException:
+            raise
         except Exception:
             raise exceptions.RequestParameterInvalidException('Invalid user id specified', id=id)
         return self.user_serializer.serialize_to_view(user, view='detailed')
@@ -253,7 +255,7 @@ def delete(self, trans, id, **kwd):
     @web.require_admin
     def undelete(self, trans, id, **kwd):
         """
-        POST /api/users/{id}
+        POST /api/users/deleted/{id}/undelete
         Undelete the user with the given ``id``
 
         :param id: the encoded id of the user to be undeleted

From 32b1d1ff1a153ba03e3ee170bdec9d6bc7750606 Mon Sep 17 00:00:00 2001
From: Martin Cech <cech.marten@gmail.com>
Date: Thu, 22 Aug 2019 14:55:59 -0400
Subject: [PATCH 11/14] use the superclass for basic operations

suggested by nsoranzo
---
 lib/galaxy/managers/users.py | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index 5286c0efaa11..40e75037a580 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -114,15 +114,7 @@ def delete(self, user):
         """Mark the given user deleted."""
         if not self.app.config.allow_user_deletion:
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete users.')
-        # Check first if it is already deleted. If so, say so and do nothing.
-        # Unsure if we should add username to the log.debug or if that breaks the GDPR support
-        if user.deleted:
-            log.debug("User already deleted, but setting delete flag just in case.")
-        if user.purged:
-            log.debug("Cannot delete purged user, but setting delete flag just in case.")
-        user.deleted = True
-        self.session().add(user)
-        self.session().flush()
+        super(UserManager, self).delete(user)
 
     def undelete(self, user):
         """Remove the deleted flag for the given user."""
@@ -130,11 +122,7 @@ def undelete(self, user):
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to undelete users.')
         if user.purged:
             raise exceptions.ItemDeletionException('Purged user cannot be undeleted.')
-        if not user.deleted:
-            log.debug('User \'%s\' has not been deleted, so it cannot be undeleted.' % user.email)
-        user.deleted = False
-        self.session().add(user)
-        self.session().flush()
+        super(UserManager, self).undelete(user)
 
     def purge(self, user):
         """Purge the given user. They must have the deleted flag already."""
@@ -162,8 +150,6 @@ def purge(self, user):
         # Delete UserAddresses
         for address in user.addresses:
             self.session().delete(address)
-        # Purge the user
-        user.purged = True
         compliance_log = logging.getLogger('COMPLIANCE')
         compliance_log.info('delete-user-event: %s' % user.username)
         # Maybe there is some case in the future where an admin needs
@@ -197,7 +183,6 @@ def purge(self, user):
         if self.app.config.redact_user_address_during_deletion:
             user_addresses = self.session().query(self.app.model.UserAddress) \
                 .filter(self.app.model.UserAddress.user_id == user.id).all()
-
             for addr in user_addresses:
                 addr.desc = new_secure_hash(addr.desc + pseudorandom_value)
                 addr.name = new_secure_hash(addr.name + pseudorandom_value)
@@ -209,9 +194,8 @@ def purge(self, user):
                 addr.country = new_secure_hash(addr.country + pseudorandom_value)
                 addr.phone = new_secure_hash(addr.phone + pseudorandom_value)
                 self.session().add(addr)
-
-        self.session().add(user)
-        self.session().flush()
+        # Purge the user
+        super(UserManager, self).purge(user)
 
     def _error_on_duplicate_email(self, email):
         """

From b218f36ba7c654a299c63cc56f9777a17971fffb Mon Sep 17 00:00:00 2001
From: Martin Cech <cech.marten@gmail.com>
Date: Thu, 22 Aug 2019 15:50:24 -0400
Subject: [PATCH 12/14] remove unused imports

---
 lib/galaxy/webapps/galaxy/controllers/admin.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/galaxy/webapps/galaxy/controllers/admin.py b/lib/galaxy/webapps/galaxy/controllers/admin.py
index e69f3639673f..8dd5ad542420 100644
--- a/lib/galaxy/webapps/galaxy/controllers/admin.py
+++ b/lib/galaxy/webapps/galaxy/controllers/admin.py
@@ -1,7 +1,6 @@
 import imp
 import logging
 import os
-import time
 from collections import OrderedDict
 from datetime import datetime, timedelta
 from string import punctuation as PUNCTUATION
@@ -22,7 +21,6 @@
     sanitize_text,
     url_get
 )
-from galaxy.util.hash_util import new_secure_hash
 from galaxy.web import url_for
 from galaxy.web.framework.helpers import grids, time_ago
 from galaxy.web.params import QuotaParamParser

From addfeffb9e29b55fb5f1b77b63fb1c0067213747 Mon Sep 17 00:00:00 2001
From: Martin Cech <marten@bx.psu.edu>
Date: Mon, 26 Aug 2019 15:27:36 -0400
Subject: [PATCH 13/14] avoid calling superclass method for purging

Because multiple inheritance will result in calling this class 'delete' method
which is not the correct target. This also avoid a database operation
I believe, since the PurgableManagerMixin.purge also calls delete before purging.
---
 lib/galaxy/managers/users.py | 4 +++-
 test/api/test_users.py       | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index 40e75037a580..ec312f7d87db 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -195,7 +195,9 @@ def purge(self, user):
                 addr.phone = new_secure_hash(addr.phone + pseudorandom_value)
                 self.session().add(addr)
         # Purge the user
-        super(UserManager, self).purge(user)
+        user.purged = True
+        self.session().add(user)
+        self.session().flush()
 
     def _error_on_duplicate_email(self, email):
         """
diff --git a/test/api/test_users.py b/test/api/test_users.py
index 7371a988bf1e..0e457d235fc9 100644
--- a/test/api/test_users.py
+++ b/test/api/test_users.py
@@ -86,9 +86,11 @@ def test_delete_user(self):
     def test_purge_user(self):
         """Delete user and then purge them."""
         user = self._setup_user(TEST_USER_EMAIL_PURGE)
-        self._delete("users/%s" % user["id"], admin=True)
+        response = self._delete("users/%s" % user["id"], admin=True)
+        self._assert_status_code_is(response, 200)
         data = dict(purge="True")
-        self._delete("users/%s" % user["id"], data=data, admin=True)
+        response = self._delete("users/%s" % user["id"], data=data, admin=True)
+        self._assert_status_code_is(response, 200)
         payload = {'deleted': "True"}
         purged_user = self._get("users/%s" % user['id'], payload, admin=True).json()
         assert purged_user['deleted'] is True, purged_user

From bada84445f95aefb5118e29ef54eca39793cf1b4 Mon Sep 17 00:00:00 2001
From: Martin Cech <marten@bx.psu.edu>
Date: Tue, 27 Aug 2019 10:56:35 -0400
Subject: [PATCH 14/14] incorporate @nsoranzo's abstraction suggestions

---
 lib/galaxy/managers/users.py | 14 ++++++--------
 test/api/test_users.py       |  4 ++--
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py
index ec312f7d87db..e891d2ec948d 100644
--- a/lib/galaxy/managers/users.py
+++ b/lib/galaxy/managers/users.py
@@ -110,21 +110,21 @@ def create(self, email=None, username=None, password=None, **kwargs):
             self.app.security_agent.user_set_default_permissions(user, default_access_private=permissions)
         return user
 
-    def delete(self, user):
+    def delete(self, user, flush=True):
         """Mark the given user deleted."""
         if not self.app.config.allow_user_deletion:
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete users.')
-        super(UserManager, self).delete(user)
+        super(UserManager, self).delete(user, flush=flush)
 
-    def undelete(self, user):
+    def undelete(self, user, flush=True):
         """Remove the deleted flag for the given user."""
         if not self.app.config.allow_user_deletion:
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to undelete users.')
         if user.purged:
             raise exceptions.ItemDeletionException('Purged user cannot be undeleted.')
-        super(UserManager, self).undelete(user)
+        super(UserManager, self).undelete(user, flush=flush)
 
-    def purge(self, user):
+    def purge(self, user, flush=True):
         """Purge the given user. They must have the deleted flag already."""
         if not self.app.config.allow_user_deletion:
             raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to delete or purge users.')
@@ -195,9 +195,7 @@ def purge(self, user):
                 addr.phone = new_secure_hash(addr.phone + pseudorandom_value)
                 self.session().add(addr)
         # Purge the user
-        user.purged = True
-        self.session().add(user)
-        self.session().flush()
+        super(UserManager, self).purge(user, flush=flush)
 
     def _error_on_duplicate_email(self, email):
         """
diff --git a/test/api/test_users.py b/test/api/test_users.py
index 0e457d235fc9..2d05e0d42192 100644
--- a/test/api/test_users.py
+++ b/test/api/test_users.py
@@ -87,10 +87,10 @@ def test_purge_user(self):
         """Delete user and then purge them."""
         user = self._setup_user(TEST_USER_EMAIL_PURGE)
         response = self._delete("users/%s" % user["id"], admin=True)
-        self._assert_status_code_is(response, 200)
+        self._assert_status_code_is_ok(response)
         data = dict(purge="True")
         response = self._delete("users/%s" % user["id"], data=data, admin=True)
-        self._assert_status_code_is(response, 200)
+        self._assert_status_code_is_ok(response)
         payload = {'deleted': "True"}
         purged_user = self._get("users/%s" % user['id'], payload, admin=True).json()
         assert purged_user['deleted'] is True, purged_user