unused-accounts: do not sent mail to never-logged-in federated users (#75086)

src/authentic2/management/commands/clean-unused-accounts.py

@@ -64,14 +64,7 @@ class Command(BaseCommand):
 
         self.now = timezone.now()
 
-        # exclude user from LDAP directories based on their source name (or realm)
-        realms = [block['realm'] for block in LDAPBackend.get_config() if block.get('realm')]
-        self.user_qs = (
-            get_user_queryset()
-            .exclude(oidc_account__isnull=False)
-            .exclude(userexternalid__source__in=realms)
-            .exclude(email='')
-        )
+        self.user_qs = get_user_queryset().exclude(email='')
 
         translation.activate(settings.LANGUAGE_CODE)
         try:
@@ -81,6 +74,7 @@ class Command(BaseCommand):
 
     def clean_unused_accounts(self):
         count = app_settings.A2_CLEAN_UNUSED_ACCOUNTS_MAX_MAIL_PER_PERIOD
+        realms = [block['realm'] for block in LDAPBackend.get_config() if block.get('realm')]
         for ou in OrganizationalUnit.objects.filter(clean_unused_accounts_alert__isnull=False):
             alert_delay = timedelta(days=ou.clean_unused_accounts_alert)
             deletion_delay = timedelta(days=ou.clean_unused_accounts_deletion)
@@ -101,8 +95,13 @@ class Command(BaseCommand):
                 & (Q(keepalive__isnull=True) | Q(keepalive__lte=self.now - alert_delay))
             )
 
-            # send first alert
-            inactive_users_first_alert = inactive_users.filter(last_account_deletion_alert__isnull=True)
+            # send first alert to users having never received an alert beforehand, skipping
+            # federated users
+            inactive_users_first_alert = inactive_users.filter(
+                Q(last_account_deletion_alert__isnull=True)
+                & Q(oidc_account__isnull=True)
+                & ~Q(userexternalid__source__in=realms)
+            )
             days_to_deletion = ou.clean_unused_accounts_deletion - ou.clean_unused_accounts_alert
             for user in inactive_users_first_alert[:count]:
                 logger.info('%s last login %d days ago, sending alert', user, ou.clean_unused_accounts_alert)
@@ -114,7 +113,12 @@ class Command(BaseCommand):
                     | Q(last_login__isnull=True) & Q(date_joined__lte=self.now - deletion_delay)
                 )
                 # ensure respect of alert delay before deletion
-                & Q(last_account_deletion_alert__lte=self.now - (deletion_delay - alert_delay))
+                # or if user is federated and never logged-in
+                & (
+                    Q(last_account_deletion_alert__lte=self.now - (deletion_delay - alert_delay))
+                    | Q(last_login__isnull=True)
+                    & (Q(oidc_account__isnull=False) | Q(userexternalid__source__in=realms))
+                )
             )
             for user in inactive_users_to_delete[:count]:
                 logger.info(
@@ -122,7 +126,14 @@ class Command(BaseCommand):
                     user,
                     ou.clean_unused_accounts_deletion,
                 )
-                self.delete_user(user, days_of_inactivity=deletion_delay.days)
+                self.delete_user(
+                    user,
+                    days_of_inactivity=deletion_delay.days,
+                    send_mail=user.last_login
+                    or not (
+                        getattr(user, 'oidc_account', None) or getattr(user, 'userexternalid', None) in realms
+                    ),
+                )
 
     def send_alert(self, user, days_to_deletion, days_of_inactivity):
         ctx = {
@@ -150,10 +161,11 @@ class Command(BaseCommand):
 
                 transaction.on_commit(send_mail)
 
-    def delete_user(self, user, days_of_inactivity):
+    def delete_user(self, user, days_of_inactivity, send_mail=True):
         ctx = {'user': user}
         with transaction.atomic():
-            self.send_mail('authentic2/unused_account_delete', user, ctx)
+            if send_mail:
+                self.send_mail('authentic2/unused_account_delete', user, ctx)
             if not self.fake:
                 UserDeletionForInactivity.record(user=user, days_of_inactivity=days_of_inactivity)
                 user.delete()

tests/test_commands.py

@@ -182,6 +182,72 @@ def test_clean_unused_account_never_logged_in(app, db, simple_user, mailoutbox,
     assert deleted_user.old_user_id == simple_user.id
 
 
+def test_clean_unused_federated_account_never_logged_in(app, db, simple_user, mailoutbox, freezer):
+    freezer.move_to('2018-01-01')
+    simple_user.ou.clean_unused_accounts_alert = 2
+    simple_user.ou.clean_unused_accounts_deletion = 3
+    simple_user.ou.save()
+
+    simple_user.last_login = None
+    simple_user.keepalive = None
+    simple_user.date_joined = now() - datetime.timedelta(days=4)
+    simple_user.save()
+
+    provider = OIDCProvider.objects.create(name='Foo', slug='foo', enabled=True)
+    OIDCAccount.objects.create(
+        provider=provider,
+        user=simple_user,
+        sub='abc',
+    )
+
+    call_command('clean-unused-accounts')
+    assert len(mailoutbox) == 0
+
+    freezer.move_to('2018-01-03')
+    call_command('clean-unused-accounts')
+    assert len(mailoutbox) == 0
+    assert (
+        Event.objects.filter(
+            type__name='user.deletion.inactivity', user=simple_user, data__email=simple_user.email
+        ).count()
+        == 1
+    )
+    deleted_user = DeletedUser.objects.get()
+    assert deleted_user.old_user_id == simple_user.id
+
+
+def test_clean_unused_federated_account_logged_in_untouched(app, db, simple_user, mailoutbox, freezer):
+    freezer.move_to('2018-01-01')
+    simple_user.ou.clean_unused_accounts_alert = 2
+    simple_user.ou.clean_unused_accounts_deletion = 3
+    simple_user.ou.save()
+
+    simple_user.last_login = simple_user.date_joined = now() - datetime.timedelta(days=4)
+    simple_user.keepalive = None
+    simple_user.save()
+
+    provider = OIDCProvider.objects.create(name='Foo', slug='foo', enabled=True)
+    OIDCAccount.objects.create(
+        provider=provider,
+        user=simple_user,
+        sub='abc',
+    )
+
+    call_command('clean-unused-accounts')
+    assert len(mailoutbox) == 0
+
+    freezer.move_to('2018-01-03')
+    call_command('clean-unused-accounts')
+    assert len(mailoutbox) == 0
+    assert (
+        Event.objects.filter(
+            type__name='user.deletion.inactivity', user=simple_user, data__email=simple_user.email
+        ).count()
+        == 0
+    )
+    assert not DeletedUser.objects.count()
+
+
 def test_clean_unused_account_keepalive_after_alert(app, db, simple_user, mailoutbox, freezer):
     freezer.move_to('2018-01-01')
     simple_user.ou.clean_unused_accounts_alert = 2