Add extra test for user suspension special cases, and document and si…

…mplify suspension logic (closes: #1293)

Main change is that users only get suspended/deleted at the configured number of days after their last warning, and are eligible for deletion at the configured number of days after suspension, even if the allowed_inactive_period_days config var changes after the warning is sent.
This allow us to steadily decrease allowed_inactive_period_days from the current (large) value to the agreed value of 1 year, without all users getting deleted before they can act on their warnings, or before the suspension grace period has passed.
Closes: #1293

server/cron/user_suspending.py

@@ -68,19 +68,24 @@ def _do_suspend_users(app):
         logger.info("Start running suspend_users job")
 
         current_time = dt_now()
+        # users who have bene inactive since this date will be suspended
         suspension_date = current_time - datetime.timedelta(days=retention.allowed_inactive_period_days)
+        # users who have been inactive since this date will get a first suspension warning
         warning_date = suspension_date + datetime.timedelta(days=retention.reminder_suspend_period_days)
+        # suspension warnings that have been sent before this date are considered "old" and can be acted upon
         warning_timeout = current_time - datetime.timedelta(days=retention.reminder_suspend_period_days)
 
         results = _result_container()
 
         # note: we handle the following progression here:
-        #  - at allowed_inactive_period_days-reminder_suspend_period_days after the last login,
+        #  - at warning_date = allowed_inactive_period_days-reminder_suspend_period_days after the last login,
         #    a user is warned about pending suspension
-        #  - at allowed_inactive_period_days after the last login, a user's account is suspended
+        #  - at reminder_suspend_period_days after the warning _and_ allowed_inactive_period_days after the last login,
+        #    a user's account is suspended
         #  - at remove_suspended_users_period_days-reminder_expiry_period_days after suspension, a
         #    user is warned about pending deletion
-        #  - at remove_suspended_users_period_days after suspension, a user is deleted
+        #  - at reminder_expiry_period_days after the reminder _and_ remove_suspended_users_period_days after suspension,
+        #    a user is deleted
 
         # first, we handle suspension and warning about suspension
         # concretely:
@@ -105,7 +110,7 @@ def _do_suspend_users(app):
                 # no recent reminder, sent one first
                 create_suspend_notification(user, retention, app, True, True)
                 results["warning_suspend_notifications"].append(user.email)
-            elif user.last_login_date < suspension_date and last_suspend_notification.sent_at < warning_timeout:
+            elif user.last_login_date < suspension_date < last_suspend_notification.sent_at < warning_timeout:
                 # user has gotten reminder, and we've waited long enough
                 logger.info(f"Suspending user {user.email} because of inactivity")
                 user.suspended = True
@@ -122,17 +127,21 @@ def _do_suspend_users(app):
         # - if deletion reminder was reminder_expiry_period_days ago, and last login was
         #   (allowed_inactive_period_days+remove_suspended_users_period_days) days ago, delete user
 
+        # users who have been inactive since this date can be deleted
         deletion_date = (
             current_time
             - datetime.timedelta(days=retention.allowed_inactive_period_days)
             - datetime.timedelta(days=retention.remove_suspended_users_period_days)
         )
+        # users who have been inactive since this date will get a first deletion warning
         warning_date = deletion_date + datetime.timedelta(days=retention.reminder_expiry_period_days)
+        # users who have been suspended since this date can be warned about deletion
         suspension_timeout = (
             current_time
             - datetime.timedelta(days=retention.remove_suspended_users_period_days)
             + datetime.timedelta(days=retention.reminder_expiry_period_days)
         )
+        # deletion warnings that have been sent before this date are considered "old" and can be acted upon
         warning_timeout = current_time - datetime.timedelta(days=retention.reminder_expiry_period_days)
         suspended_users = User.query \
             .filter(User.last_login_date < warning_date, User.suspended == True) \
@@ -151,13 +160,17 @@ def _do_suspend_users(app):
                 SuspendNotification.is_suspension.is_(False)
             ).order_by(desc(SuspendNotification.sent_at)).first()  # noqa: E712
 
-            if last_suspend_notification is not None and \
-               last_suspend_notification.sent_at < suspension_timeout and \
-               (last_delete_warning is None or last_delete_warning.sent_at < suspension_date):
+            if last_suspend_notification is None:
+                raise Exception(f"User {user.uid} ({user.uid}) is suspended but has no suspension notification."
+                                "This should not happen.")
+            if last_suspend_notification.sent_at > suspension_timeout:
+                continue  # user suspended too recently, don't delete yet
+
+            if last_delete_warning is None or last_delete_warning.sent_at < suspension_date:
+                # no recent deletion warning, sent one first
                 create_suspend_notification(user, retention, app, True, False)
                 results["warning_deleted_notifications"].append(user.email)
-            elif user.last_login_date < deletion_date and \
-                 last_delete_warning is not None and last_delete_warning.sent_at < warning_timeout:
+            elif user.last_login_date < deletion_date < last_delete_warning.sent_at < warning_timeout:
                 # don't send mail to user; they have already received 3 emails, and this one is not actionable.
                 results["deleted_notifications"].append(user.email)
                 deleted_user_uids.append(user.uid)

server/test/cron/test_user_suspending.py

@@ -84,3 +84,42 @@ def test_schedule(self):
                 self.assertListEqual(["[email protected]"], results["warning_deleted_notifications"])
                 self.assertListEqual(["[email protected]"], results["deleted_notifications"])
                 self.assertEqual(3, len(outbox))
+
+    def test_schedule_changed_config(self):
+        mail = self.app.mail
+
+        # run suspend cron job
+        results = suspend_users(self.app)
+        self.assertListEqual(["[email protected]"], results["warning_suspend_notifications"])
+        self.assertListEqual(["[email protected]"], results["suspended_notifications"])
+        self.assertListEqual(["[email protected]"], results["warning_deleted_notifications"])
+        self.assertListEqual(["[email protected]"], results["deleted_notifications"])
+
+        # now we change the config
+        # this causes the last active date of all suspended users to shift past the deletion date
+        self.app.app_config.retention.allowed_inactive_period_days -= (
+                self.app.app_config.retention.remove_suspended_users_period_days
+                + 1)
+        # now run suspend cron job again; nothing should change!
+        with mail.record_messages() as outbox:
+            results = suspend_users(self.app)
+            self.assertListEqual([], results["warning_suspend_notifications"])
+            self.assertListEqual([], results["suspended_notifications"])
+            self.assertListEqual([], results["warning_deleted_notifications"])
+            self.assertListEqual([], results["deleted_notifications"])
+            self.assertListEqual([], outbox)
+
+        # now fast-forward time past the waiting window
+        retention = self.app.app_config.retention
+        newdate = (dt_now()
+                   + timedelta(retention.reminder_suspend_period_days)
+                   + timedelta(retention.remove_suspended_users_period_days))
+        with freeze_time(newdate):
+            with mail.record_messages() as outbox:
+                # now users should be suspended/reminede again because their notifications are older than the threshold
+                results = suspend_users(self.app)
+                self.assertListEqual([], results["warning_suspend_notifications"])
+                self.assertListEqual(["[email protected]"], results["suspended_notifications"])
+                self.assertListEqual(["[email protected]"], results["warning_deleted_notifications"])
+                self.assertListEqual(["[email protected]"], results["deleted_notifications"])
+                self.assertEqual(3, len(outbox))

server/test/seed.py

@@ -237,15 +237,18 @@ def seed(db, app_config, skip_seed=False):
 
     warning_date = dt_now() - datetime.timedelta(days=retention.remove_suspended_users_period_days) \
                    + datetime.timedelta(days=retention.reminder_expiry_period_days - 1)
-    notification_suspension_warning = SuspendNotification(user=user_deletion_warning, sent_at=warning_date,
-                                                          is_suspension=True, is_warning=False)
+    notification_deletion_warning = SuspendNotification(user=user_deletion_warning, sent_at=warning_date,
+                                                        is_suspension=True, is_warning=False)
 
-    deletion_date = current_time - datetime.timedelta(retention.remove_suspended_users_period_days + 1)
-    notification_gets_deleted = SuspendNotification(user=user_gets_deleted, sent_at=deletion_date,
-                                                    is_suspension=False, is_warning=True)
+    suspension_date = current_time - datetime.timedelta(days=retention.remove_suspended_users_period_days + 1)
+    deletion_date = current_time - datetime.timedelta(days=retention.reminder_expiry_period_days + 1)
+    notification_gets_deleted_1 = SuspendNotification(user=user_gets_deleted, sent_at=suspension_date,
+                                                      is_suspension=True, is_warning=False)
+    notification_gets_deleted_2 = SuspendNotification(user=user_gets_deleted, sent_at=deletion_date,
+                                                      is_suspension=False, is_warning=True)
 
     persist_instance(db, notification_gets_suspended_old, notification_gets_suspended,
-                     notification_suspension_warning, notification_gets_deleted)
+                     notification_deletion_warning, notification_gets_deleted_1, notification_gets_deleted_2)
 
     ssh_key_john = SshKey(user=john, ssh_value="ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC/nvjea1zJJNCnyUfT6HLcHD"
                                                "hwCMp7uqr4BzxhDAjBnjWcgW4hZJvtLTqCLspS6mogCq2d0/31DU4DnGb2MO28"