Merge branch 'fix/user_suspend'

server/config/test_config.yml

@@ -114,11 +114,11 @@ feature:
 
 # The retention config determines how long users may be inactive, how long the reminder magic link is valid and when do we resent the magic link
 retention:
-  allowed_inactive_period_days: 90
-  reminder_suspend_period_days: 14
+  allowed_inactive_period_days: 365
+  reminder_suspend_period_days: 7
+  remove_suspended_users_period_days: 90
   reminder_expiry_period_days: 7
   cron_hour_of_day: 7
-  remove_suspended_users_period_days: 540
 
 metadata:
   idp_url: https://metadata.surfconext.nl/idps-metadata.xml

server/cron/user_suspending.py

@@ -2,6 +2,8 @@
 import logging
 import time
 
+from sqlalchemy import desc
+
 from server.cron.shared import obtain_lock
 from server.db.db import db
 from server.db.domain import User, SuspendNotification, UserNameHistory
@@ -23,8 +25,11 @@ def create_suspend_notification(user, retention, app, is_warning, is_suspension)
                 f"user {user.email} because last_login_date is {user.last_login_date}")
     current_time = datetime.datetime.utcnow()
     suspension_date = current_time + datetime.timedelta(days=retention.reminder_suspend_period_days)
-    deletion_days = retention.remove_suspended_users_period_days + retention.reminder_suspend_period_days \
-                    + retention.reminder_expiry_period_days
+    deletion_days = (
+        retention.remove_suspended_users_period_days
+        + retention.reminder_suspend_period_days
+        + retention.reminder_expiry_period_days
+    )
     if is_warning and not is_suspension:
         deletion_days -= retention.reminder_suspend_period_days
         deletion_days -= retention.remove_suspended_users_period_days
@@ -63,45 +68,104 @@ def _do_suspend_users(app):
         current_time = datetime.datetime.utcnow()
         suspension_date = current_time - datetime.timedelta(days=retention.allowed_inactive_period_days)
         warning_date = suspension_date + datetime.timedelta(days=retention.reminder_suspend_period_days)
+        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,
+        #    a user is warned about pending suspension
+        #  - at 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
+
+        # first, we handle suspension and warning about suspension
+        # concretely:
+        # - look at all users who haven't logged in since warning_date
+        # - if the user has not yet gotten a notification (or very long ago), send a warning
+        # - otherwise, suspend the user only if the user hasn't logged in since suspension_date and
+        #   the warning was sent at least reminder_suspend_period_days days ago
         users = User.query \
             .filter(User.last_login_date < warning_date, User.suspended == False).all()  # noqa: E712
         for user in users:
-            suspend_notifications = user.suspend_notifications
-            if len(suspend_notifications) == 0:
+            last_suspend_notification = SuspendNotification.query.filter(
+                SuspendNotification.user == user,
+                SuspendNotification.is_warning.is_(True),
+                SuspendNotification.is_suspension.is_(True)
+            ).order_by(desc(SuspendNotification.sent_at)).first()
+
+            if last_suspend_notification is None or last_suspend_notification.sent_at < warning_date:
+                # 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:
+            elif user.last_login_date < suspension_date and 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
                 create_suspend_notification(user, retention, app, False, True)
                 results["suspended_notifications"].append(user.email)
 
-        deletion_date = current_time - datetime.timedelta(days=retention.remove_suspended_users_period_days)
+        # now handle deletion of suspended users.
+        # concretely:
+        # - look at suspended users
+        # - if suspension was (remove_suspended_users_period_days - reminder_expiry_period_days) days ago,
+        #   and last login was (allowed_inactive_period_days+remove_suspended_users_period_days
+        #   -reminder_expiry_period_days) days ago, and no deletion reminder was sent recently,
+        #   send deletion reminder
+        # - 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
+
+        deletion_date = (
+            current_time
+            - datetime.timedelta(days=retention.allowed_inactive_period_days)
+            - datetime.timedelta(days=retention.remove_suspended_users_period_days)
+        )
         warning_date = deletion_date + datetime.timedelta(days=retention.reminder_expiry_period_days)
+        suspension_timeout = (
+            current_time
+            - datetime.timedelta(days=retention.remove_suspended_users_period_days)
+            + datetime.timedelta(days=retention.reminder_expiry_period_days)
+        )
+        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, ).all()  # noqa: E712
+            .filter(User.last_login_date < warning_date, User.suspended == True).all()  # noqa: E712
+        deleted_user_uids = []
         for user in suspended_users:
-            suspend_notifications = user.suspend_notifications
-            if not any([not sp.is_suspension and sp.is_warning for sp in suspend_notifications]):
+            last_suspend_notification = SuspendNotification.query.filter(
+                SuspendNotification.user == user,
+                SuspendNotification.is_warning.is_(False),
+                SuspendNotification.is_suspension.is_(True)
+            ).order_by(desc(SuspendNotification.sent_at)).first()
+            last_delete_warning = SuspendNotification.query.filter(
+                SuspendNotification.user == user,
+                SuspendNotification.is_warning.is_(True),
+                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):
                 create_suspend_notification(user, retention, app, True, False)
                 results["warning_deleted_notifications"].append(user.email)
-            elif user.last_login_date < deletion_date:
-                create_suspend_notification(user, retention, app, False, False)
+            elif user.last_login_date < deletion_date and \
+                 last_delete_warning is not None and 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)
                 if user.username:
                     history_not_exists = UserNameHistory.query.filter(
                         UserNameHistory.username == user.username).count() == 0
                     if history_not_exists:
                         user_name_history = UserNameHistory(username=user.username)
                         db.session.merge(user_name_history)
-                mail_suspended_account_deletion(user)
                 db.session.delete(user)
 
         db.session.commit()
 
+        if deleted_user_uids:
+            mail_suspended_account_deletion(deleted_user_uids)
+
         end = int(time.time() * 1000.0)
         logger.info(f"Finished running suspend_users job in {end - start} ms")
 

server/mail.py

@@ -287,10 +287,14 @@ def mail_accepted_declined_service_connection_request(context, service_name, col
 
 def mail_suspend_notification(context, recipients, is_warning, is_suspension):
     _store_mail(context["user"], SUSPEND_NOTIFICATION_MAIL, recipients)
-    if is_warning:
-        template = "suspend_suspend_warning_notification" if is_suspension else "suspend_delete_warning_notification"
+    if is_suspension and is_warning:
+        template = "suspend_suspend_warning_notification"
+    elif is_suspension and not is_warning:
+        template = "suspend_suspend_notification"
+    elif not is_suspension and is_warning:
+        template = "suspend_delete_warning_notification"
     else:
-        template = "suspend_suspend_notification" if is_suspension else "suspend_delete_notification"
+        raise Exception("We don't send mails on account deletion")
     return _do_send_mail(
         subject="SURF SRAM: suspend notification",
         recipients=recipients,
@@ -374,19 +378,18 @@ def mail_account_deletion(user):
     )
 
 
-def mail_suspended_account_deletion(user):
+def mail_suspended_account_deletion(uids: list[str]):
     mail_cfg = current_app.app_config.mail
     recipients = [mail_cfg.eduteams_email]
     if mail_cfg.account_deletion_notifications_enabled:
         recipients.append(mail_cfg.beheer_email)
     _do_send_mail(
-        subject=f"User {user.email} suspended account is deleted in environment {mail_cfg.environment}",
+        subject=f"User accounts deleted in environment {mail_cfg.environment}",
         recipients=recipients,
         template="admin_suspended_user_account_deleted",
         context={"environment": mail_cfg.environment,
                  "date": datetime.datetime.now(),
-                 "attributes": _user_attributes(user),
-                 "user": user},
+                 "uids": uids},
         preview=False,
         working_outside_of_request_context=True
     )

server/requirements/test.txt

@@ -7,4 +7,5 @@ hbmqtt2==0.9.7
 pytest-cov==4.0.0
 pytest==7.2.0
 responses==0.22.0
-alembic==1.9.4
+alembic==1.9.4
+freezegun==1.2.2

server/templates/admin_suspended_user_account_deleted.html

@@ -1,18 +1,20 @@
 {% extends "mail_layout.html" %}
-{% block title %}SURF platform account deletion notification {% endblock %}
+{% block title %}SRAM account deletion notification for {{ environment }}{% endblock %}
 {% block content %}
 
     <p>Hi,</p>
-    <p>User {{ user.name }} ({{ user.uid }}) is deleted on environment {{ environment }}
-    at {{ date }}, because of inactivity by the suspended users cron job</p>
+    <p>The following users have been removed from SRAM in environment {{ environment }}
+        because of inactivity by the suspended users cron job.</p>
 
-    <p>Please remove this user from Perun.</p>
+    <p>Please delete them from the MMS.</p>
 
-    {% for key, value in attributes.items() %}
-        <p>{{ key }}: {{ value }}</p>
+    <ul>
+    {% for uid in uids  %}
+        <li>{{ uid }}</li>
     {% endfor %}
+    </ul>
 
-    <p>This email is automatically sent and can be suppressed by changing the <i>account_deletion_notifications_enabled</i> section
-        of the configuration for environment {{ environment }}</p>
+
+    <p>This email is automatically sent for environment {{ environment }}.</p>
 
 {% endblock %}

server/templates/admin_suspended_user_account_deleted.txt

@@ -1,9 +1,17 @@
 {% extends "mail_layout.txt" %}
-{% block title %}SURF inactive notification{% endblock %}
+{% block title %}SRAM account deletion notification for {{ environment }}{% endblock %}
 {% block content %}
 
 Hi,
-User {{ user.name }} ({{ user.uid }}) is deleted on environment {{ environment }}
-at {{ date }}, because of inactivity by the suspended users cron job.
+The following users have been removed from SRAM in environment {{ environment }}
+because of inactivity by the suspended users cron job.
+
+Please delete them from the MMS.
+
+    {% for uid in uids %}
+        {{ uid }}
+    {% endfor %}
+
+This email is automatically sent for environment {{ environment }}.
 
 {% endblock %}

server/test/cron/test_user_suspending.py

@@ -1,10 +1,14 @@
+from datetime import datetime, timedelta
+
 from sqlalchemy import text
 
 from server.cron.user_suspending import suspend_users, suspend_users_lock_name
 from server.db.db import db
 from server.db.domain import User, UserNameHistory
 from server.test.abstract_test import AbstractTest
 
+from freezegun import freeze_time
+
 
 class TestUserSuspending(AbstractTest):
 
@@ -27,12 +31,13 @@ def test_schedule(self):
             self.assertListEqual(["user_gets_suspended@example.org"], results["suspended_notifications"])
             self.assertListEqual(["user_deletion_warning@example.org"], results["warning_deleted_notifications"])
             self.assertListEqual(["user_gets_deleted@example.org"], results["deleted_notifications"])
-            self.assertEqual(5, len(outbox))
+            self.assertEqual(4, len(outbox))
 
         user_suspend_warning = self.find_entity_by_name(User, "user_suspend_warning")
         self.assertEqual(False, user_suspend_warning.suspended)
         notifications = user_suspend_warning.suspend_notifications
-        self.assertEqual(1, len(notifications))
+        self.assertEqual(2, len(notifications))
+        notifications.sort(key=lambda n: n.sent_at, reverse=True)
         self.assertTrue(notifications[0].is_warning)
         self.assertTrue(notifications[0].is_suspension)
 
@@ -45,8 +50,10 @@ def test_schedule(self):
         self.assertTrue(notifications[0].is_suspension)
 
         user_deletion_warning = self.find_entity_by_name(User, "user_deletion_warning")
+        self.assertEqual(True, user_deletion_warning.suspended)
         notifications = user_deletion_warning.suspend_notifications
-        self.assertEqual(1, len(notifications))
+        self.assertEqual(2, len(notifications))
+        notifications.sort(key=lambda n: n.sent_at, reverse=True)
         self.assertTrue(notifications[0].is_warning)
         self.assertFalse(notifications[0].is_suspension)
 
@@ -56,3 +63,28 @@ def test_schedule(self):
         user_names_history = UserNameHistory.query.all()
         self.assertEqual(1, len(user_names_history))
         self.assertEqual("user_gets_deleted", user_names_history[0].username)
+
+        # 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
+        retention = self.app.app_config.retention
+        newdate = (
+            datetime.utcnow()
+            + timedelta(retention.reminder_suspend_period_days)
+            + timedelta(retention.remove_suspended_users_period_days)
+        )
+        with freeze_time(newdate):
+            with mail.record_messages() as outbox:
+                results = suspend_users(self.app)
+                self.assertListEqual([], results["warning_suspend_notifications"])
+                self.assertListEqual(["user_suspend_warning@example.org"], results["suspended_notifications"])
+                self.assertListEqual(["user_gets_suspended@example.org"], results["warning_deleted_notifications"])
+                self.assertListEqual(["user_deletion_warning@example.org"], results["deleted_notifications"])
+                self.assertEqual(3, len(outbox))