From 0ac0f71fef739cd428d9ef5a8d911bcc862b03e7 Mon Sep 17 00:00:00 2001
From: Adarnof <adarnof@gmail.com>
Date: Fri, 8 Sep 2017 13:42:35 -0400
Subject: [PATCH] Correct SeAT API logic (#860)

* Do not attempt to change user email on SeAT if unchanged.
This prevents HTTP422 from being raised on password resets.

*Delete users on deactivation.
The existing disable user logic does nothing and results in a HTTP500.
It's safe to delete users entirely - the API keys are retained.

Fixes #844
---
 services/modules/seat/manager.py | 37 +++++++++++---------------------
 services/modules/seat/tasks.py   |  2 +-
 services/modules/seat/tests.py   |  8 +++----
 3 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/services/modules/seat/manager.py b/services/modules/seat/manager.py
index f979612b..4afe4dfd 100644
--- a/services/modules/seat/manager.py
+++ b/services/modules/seat/manager.py
@@ -75,25 +75,6 @@ class SeatManager:
             return username
         return None
 
-    @classmethod
-    def disable_user(cls, username):
-        """ Disable user """
-        ret = cls.exec_request('user/{}'.format(username), 'put', active=0)
-        logger.debug(ret)
-        ret = cls.exec_request('user/{}'.format(username), 'put', email="")
-        logger.debug(ret)
-        if cls._response_ok(ret):
-            try:
-                cls.update_roles(username, [])
-                logger.info("Disabled SeAT user with username %s" % username)
-                return username
-            except KeyError:
-                # if something goes wrong, delete user from seat instead of disabling
-                if cls.delete_user(username):
-                    return username
-        logger.info("Failed to disabled SeAT user with username %s" % username)
-        return None
-
     @classmethod
     def enable_user(cls, username):
         """ Enable user """
@@ -105,14 +86,22 @@ class SeatManager:
         logger.info("Failed to enabled SeAT user with username %s" % username)
         return None
 
+    @classmethod
+    def _check_email_changed(cls, username, email):
+        """Compares email to one set on SeAT"""
+        ret = cls.exec_request('user/{}'.format(username), 'get', raise_for_status=True)
+        return ret['email'] != email
+
     @classmethod
     def update_user(cls, username, email, password):
         """ Edit user info """
-        logger.debug("Updating SeAT username %s with email %s and password" % (username, email))
-        ret = cls.exec_request('user/{}'.format(username), 'put', email=email)
-        logger.debug(ret)
-        if not cls._response_ok(ret):
-            logger.warn("Failed to update email for username {}".format(username))
+        if cls._check_email_changed(username, email):
+            # if we try to set the email to whatever it is already on SeAT, we get a HTTP422 error
+            logger.debug("Updating SeAT username %s with email %s and password" % (username, email))
+            ret = cls.exec_request('user/{}'.format(username), 'put', email=email)
+            logger.debug(ret)
+            if not cls._response_ok(ret):
+                logger.warn("Failed to update email for username {}".format(username))
         ret = cls.exec_request('user/{}'.format(username), 'put', password=password)
         logger.debug(ret)
         if not cls._response_ok(ret):
diff --git a/services/modules/seat/tasks.py b/services/modules/seat/tasks.py
index d8d14eab..8781d939 100644
--- a/services/modules/seat/tasks.py
+++ b/services/modules/seat/tasks.py
@@ -28,7 +28,7 @@ class SeatTasks:
 
     @classmethod
     def delete_user(cls, user, notify_user=False):
-        if cls.has_account(user) and SeatManager.disable_user(user.seat.username):
+        if cls.has_account(user) and SeatManager.delete_user(user.seat.username):
             user.seat.delete()
             logger.info("Successfully deactivated SeAT for user %s" % user)
             if notify_user:
diff --git a/services/modules/seat/tests.py b/services/modules/seat/tests.py
index 75e8569d..190a26cb 100644
--- a/services/modules/seat/tests.py
+++ b/services/modules/seat/tests.py
@@ -100,10 +100,10 @@ class SeatHooksTestCase(TestCase):
 
         # Test none user is deleted
         none_user = User.objects.get(username=self.none_user)
-        manager.disable_user.return_value = 'abc123'
+        manager.delete_user.return_value = 'abc123'
         SeatUser.objects.create(user=none_user, username='abc123')
         service.validate_user(none_user)
-        self.assertTrue(manager.disable_user.called)
+        self.assertTrue(manager.delete_user.called)
         with self.assertRaises(ObjectDoesNotExist):
             none_seat = User.objects.get(username=self.none_user).seat
 
@@ -115,7 +115,7 @@ class SeatHooksTestCase(TestCase):
         result = service.delete_user(member)
 
         self.assertTrue(result)
-        self.assertTrue(manager.disable_user.called)
+        self.assertTrue(manager.delete_user.called)
         with self.assertRaises(ObjectDoesNotExist):
             seat_user = User.objects.get(username=self.member).seat
 
@@ -177,7 +177,7 @@ class SeatViewsTestCase(TestCase):
 
         response = self.client.get(urls.reverse('auth_deactivate_seat'))
 
-        self.assertTrue(manager.disable_user.called)
+        self.assertTrue(manager.delete_user.called)
         self.assertRedirects(response, expected_url=urls.reverse('auth_services'), target_status_code=200)
         with self.assertRaises(ObjectDoesNotExist):
             seat_user = User.objects.get(pk=self.member.pk).seat