diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a214d6953b86dc06cd116bc87a52b2f811822ae0..c7b8c858c1bf4370f219f0c13fe57d6facc6fbac 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -15,7 +15,6 @@ gnupg debian-keyring python3-git - python3-testfixtures libjs-bootstrap4 fonts-fork-awesome libjs-jquery-datatables diff --git a/dsa/housekeeping.py b/dsa/housekeeping.py index d7f5ec7d6aa5dd635b6b1e86692eeda738cfadcd..93b50b54c7103ad04900019b2248b964dff7b099 100644 --- a/dsa/housekeeping.py +++ b/dsa/housekeeping.py @@ -6,6 +6,7 @@ from django.utils.timezone import now from backend.housekeeping import MakeLink, Housekeeper from . import udldap from backend import const +from sitechecks.models import Inconsistency import backend.models as bmodels import process.models as pmodels import backend.ops as bops @@ -140,8 +141,10 @@ class CheckLDAPConsistency(hk.Task): fpr = entry.single("keyFingerPrint") if fpr: - log.warn("%s: %s has fingerprint %s and gid %s in LDAP, but is not in our db", - self.IDENTIFIER, entry.uid, fpr, entry.single("gidNumber")) + Inconsistency.objects.found( + self.IDENTIFIER, + f"{entry.uid} has fingerprint {fpr} and gid {entry.single('gidNumber')} in LDAP," + " but is not in our db") else: ldap_fields_args = { "cn": entry.single("cn"), @@ -199,9 +202,12 @@ class CheckLDAPConsistency(hk.Task): elif not entry.is_dd: # mom040267 has a locked account (see the FD comments on the site) if person.ldap_fields.uid != "mom040267": - log.warn("%s: %s has accountStatus '%s' (comment: %s) but in our db the state is %s", - self.IDENTIFIER, self.hk.link(person), entry.single("accountStatus"), - entry.single("accountComment"), const.ALL_STATUS_DESCS[person.status]) + Inconsistency.objects.found( + self.IDENTIFIER, + f"person has accountStatus '{entry.single('accountStatus')}'" + f" (comment: {entry.single('accountComment')})" + f" but in our db the state is {const.ALL_STATUS_DESCS[person.status]}", + person=person) if entry.is_dd: # if person.status not in (const.STATUS_REMOVED_DD, const.STATUS_EMERITUS_DD): @@ -210,26 +216,36 @@ class CheckLDAPConsistency(hk.Task): if person.status_changed > self.email_forwarding_cutoff: if dsa_status == "retiring" and person.status != const.STATUS_EMERITUS_DD: - log.warn("%s: %s has accountStatus '%s' (comment: %s) but in our db the state is %s [retiring]", - self.IDENTIFIER, self.hk.link(person), entry.single("accountStatus"), - entry.single("accountComment"), const.ALL_STATUS_DESCS[person.status]) + Inconsistency.objects.found( + self.IDENTIFIER, + f"person has accountStatus '{entry.single('accountStatus')}'" + f" (comment: {entry.single('accountComment')})" + f" but in our db the state is {const.ALL_STATUS_DESCS[person.status]} [retiring]", + person=person) if dsa_status == "inactive" and person.status != const.STATUS_REMOVED_DD: - log.warn("%s: %s has accountStatus '%s' (comment: %s) but in our db the state is %s [inactive]", - self.IDENTIFIER, self.hk.link(person), entry.single("accountStatus"), - entry.single("accountComment"), const.ALL_STATUS_DESCS[person.status]) + Inconsistency.objects.found( + self.IDENTIFIER, + f"person has accountStatus '{entry.single('accountStatus')}'" + f" (comment: {entry.single('accountComment')})" + f" but in our db the state is {const.ALL_STATUS_DESCS[person.status]} [inactive]", + person=person) if dsa_status == "locked": parsed = entry.single("accountStatus").split() if len(parsed) == 1: - log.warn("%s: %s has accountStatus '%s' (comment: %s) locked with no date", - self.IDENTIFIER, self.hk.link(person), entry.single("accountStatus"), - entry.single("accountComment")) + Inconsistency.objects.found( + self.IDENTIFIER, + f"person has accountStatus '{entry.single('accountStatus')}'" + f"(comment: {entry.single('accountComment')}) locked with no date", + person=person) else: if dsa_status_date is not None and dsa_status_date < self.email_forwarding_cutoff.date(): - log.warn("%s: %s has accountStatus '%s' (comment: %s) locked for longer than 6 months", - self.IDENTIFIER, self.hk.link(person), entry.single("accountStatus"), - entry.single("accountComment")) + Inconsistency.objects.found( + self.IDENTIFIER, + f"person has accountStatus '{entry.single('accountStatus')}'" + f" (comment: {entry.single('accountComment')}) locked for longer than 6 months", + person=person) def run_main(self, stage): # Prefetch people and index them by uid @@ -253,15 +269,19 @@ class CheckLDAPConsistency(hk.Task): try: dsa_status_date = datetime.datetime.strptime(parsed[1], "%Y-%m-%d").date() except ValueError: - log.warn("%s: %s has accountStatus '%s' (comment: %s) with unparsable date", - self.IDENTIFIER, self.hk.link(person), entry.single("accountStatus"), - entry.single("accountComment")) + Inconsistency.objects.found( + self.IDENTIFIER, + "person has accountStatus '{entry.single('accountStatus')}'" + " (comment: {entry.single('accountComment')}) with unparsable date", + person=person) dsa_status_date = None else: dsa_status_date = None if dsa_status not in ("retiring", "inactive", "memorial", "locked", "renamed"): - log.warn("%s: %s has unknown accountStatus %s", - self.IDENTIFIER, self.hk.link(person), entry.single("accountStatus")) + Inconsistency.objects.found( + self.IDENTIFIER, + "person has unknown accountStatus {entry.single('accountStatus')}", + person=person) else: self.check_account_status(person, entry, dsa_status, dsa_status_date) else: @@ -269,14 +289,18 @@ class CheckLDAPConsistency(hk.Task): # DDs we don't expect if entry.is_dd: if person.status not in (const.STATUS_DD_U, const.STATUS_DD_NU): - log.warn("%s: %s has supplementaryGid 'Debian', but in our db the state is %s", - self.IDENTIFIER, self.hk.link(person), - const.ALL_STATUS_DESCS[person.status]) + Inconsistency.objects.found( + self.IDENTIFIER, + "person has supplementaryGid 'Debian'," + f" but in our db the state is {const.ALL_STATUS_DESCS[person.status]}", + person=person) else: if person.status in (const.STATUS_DD_U, const.STATUS_DD_NU): - log.warn("%s: %s has no supplementaryGid 'Debian', but in our db the state is %s", - self.IDENTIFIER, self.hk.link(person), - const.ALL_STATUS_DESCS[person.status]) + Inconsistency.objects.found( + self.IDENTIFIER, + "person has no supplementaryGid 'Debian'," + f" but in our db the state is {const.ALL_STATUS_DESCS[person.status]}", + person=person) email = entry.single("emailForward") if email != person.ldap_fields.email: diff --git a/dsa/tests/test_housekeeping.py b/dsa/tests/test_housekeeping.py index b591a70fd3404d527eda2dd4bc958d522e1c2dc3..35126f28cccd2bf02efe84c08c401385afe75756 100644 --- a/dsa/tests/test_housekeeping.py +++ b/dsa/tests/test_housekeeping.py @@ -5,8 +5,8 @@ from dsa.housekeeping import CheckLDAPConsistency from backend.models import Person import backend.const as const from process.models import Process +from sitechecks.models import Inconsistency from unittest import mock -from testfixtures import LogCapture class MockHousekeeper: @@ -16,6 +16,12 @@ class MockHousekeeper: def link(self, person): return str(person.ldap_fields.uid or person.pk) + def run(self, cls, stage: str = "main"): + task = cls(self) + task.IDENTIFIER = f"test.{cls.__name__}" + getattr(task, f"run_{stage}")(None) + return task + class MockEntry: def __init__(self, uid, **attrs): @@ -34,16 +40,27 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): super().setUp() self.hk = MockHousekeeper(self.persons.oldam) + def assertInconsistenciesEqual(self, items): + res = list(Inconsistency.objects.all()) + self.assertEqual(len(res), len(items)) + for inc, item in zip(res, items): + if "person" in item: + self.assertEqual(inc.person, item["person"]) + if "text" in item: + self.assertEqual(inc.text, item["text"]) + def test_new_removed(self): with mock.patch("dsa.udldap.list_people") as m: m.return_value = [ MockEntry("newdd", supplementaryGid=["Debian"], cn="tcn", mn="tmn", sn="tsn", emailForward="test@example.org", accountStatus=None) ] - with LogCapture() as lc: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - lc.check(("dsa.housekeeping", "WARNING", "None: newdd: created to mirror a removed DD account from LDAP")) + with self.assertLogs() as log: + self.hk.run(CheckLDAPConsistency) + self.assertEqual(log.output, [ + "WARNING:dsa.housekeeping:test.CheckLDAPConsistency:" + " newdd: created to mirror a removed DD account from LDAP", + ]) p = Person.objects.get(ldap_fields__uid="newdd") audit = ["{}:{}".format(l.author.lookup_key, l.notes) for l in p.audit_log.all()] @@ -64,11 +81,11 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): MockEntry("newguest", supplementaryGid=[], cn="tcn", mn="tmn", sn="tsn", emailForward="test@example.org", accountStatus=None) ] - with LogCapture() as lc: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - lc.check(("dsa.housekeeping", "WARNING", - "None: newguest: created to mirror a removed guest account from LDAP")) + with self.assertLogs() as log: + self.hk.run(CheckLDAPConsistency) + self.assertEqual(log.output, [ + "WARNING:dsa.housekeeping:test.CheckLDAPConsistency:" + " newguest: created to mirror a removed guest account from LDAP"]) p = Person.objects.get(ldap_fields__uid="newguest") audit = ["{}:{}".format(l.author.lookup_key, l.notes) for l in p.audit_log.all()] @@ -90,12 +107,12 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): emailForward="test@example.org", keyFingerPrint="66B4DFB68CB24EBBD8650BC4F4B4B0CC797EBFAB", accountStatus=None) ] - with LogCapture() as lc: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - lc.check(("dsa.housekeeping", "WARNING", - "None: newdd has fingerprint 66B4DFB68CB24EBBD8650BC4F4B4B0CC797EBFAB and gid 800 in LDAP," - " but is not in our db")) + self.hk.run(CheckLDAPConsistency) + self.assertInconsistenciesEqual([ + {"person": None, + "text": "newdd has fingerprint 66B4DFB68CB24EBBD8650BC4F4B4B0CC797EBFAB and gid 800 in LDAP," + " but is not in our db"}, + ]) with self.assertRaises(Person.DoesNotExist): Person.objects.get(ldap_fields__uid="newdd") @@ -106,16 +123,15 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): MockEntry("dd_u", supplementaryGid=["Debian"], cn="tcn", mn="tmn", sn="tsn", emailForward="test@example.org", accountStatus=None) ] - with LogCapture() as lc: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - lc.check( - ("dsa.housekeeping", "INFO", "None: dd_u changing email_ldap from dd_u@example.org to test@example.org" - " (source: LDAP)"), - ('dsa.housekeeping', 'INFO', 'None: dd_u changing cn from Dd_u to tcn (source: LDAP)'), - ('dsa.housekeeping', 'INFO', 'None: dd_u changing mn from to tmn (source: LDAP)'), - ('dsa.housekeeping', 'INFO', 'None: dd_u changing sn from to tsn (source: LDAP)'), - ) + with self.assertLogs() as log: + self.hk.run(CheckLDAPConsistency) + self.assertEqual(log.output, [ + "INFO:dsa.housekeeping:test.CheckLDAPConsistency: dd_u changing email_ldap from dd_u@example.org" + " to test@example.org (source: LDAP)", + 'INFO:dsa.housekeeping:test.CheckLDAPConsistency: dd_u changing cn from Dd_u to tcn (source: LDAP)', + 'INFO:dsa.housekeeping:test.CheckLDAPConsistency: dd_u changing mn from to tmn (source: LDAP)', + 'INFO:dsa.housekeeping:test.CheckLDAPConsistency: dd_u changing sn from to tsn (source: LDAP)', + ]) p = self.persons.dd_u p.refresh_from_db() @@ -137,12 +153,10 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): MockEntry("dm", supplementaryGid=["Debian"], cn="Dm", mn="", sn="", emailForward="dm@example.org", keyFingerPrint="66B4DFB68CB24EBBD8650BC4F4B4B0CC797EBFAB", accountStatus=None) ] - with self.assertLogs() as log: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - self.assertEqual(log.output, [ - "WARNING:dsa.housekeeping:" - "None: dm has supplementaryGid 'Debian', but in our db the state is Debian Maintainer", + self.hk.run(CheckLDAPConsistency) + self.assertInconsistenciesEqual([ + {"person": self.persons.dm, + "text": "person has supplementaryGid 'Debian', but in our db the state is Debian Maintainer"}, ]) p = self.persons.dm @@ -156,13 +170,13 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): emailForward="dd_u@example.org", keyFingerPrint="66B4DFB68CB24EBBD8650BC4F4B4B0CC797EBFAB", accountStatus="inactive 2018-03-20"), ] - with self.assertLogs() as log: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - self.assertEqual(log.output, [ - "WARNING:dsa.housekeeping:" - "None: dd_u has accountStatus 'inactive 2018-03-20' (comment: None)" - " but in our db the state is Debian Developer, uploading [inactive]", + self.hk.run(CheckLDAPConsistency) + + self.assertInconsistenciesEqual([ + {"person": self.persons.dd_u, + "text": + "person has accountStatus 'inactive 2018-03-20' (comment: None)" + " but in our db the state is Debian Developer, uploading [inactive]"} ]) p = self.persons.dd_u @@ -177,12 +191,14 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): keyFingerPrint="66B4DFB68CB24EBBD8650BC4F4B4B0CC797EBFAB", accountStatus="inactive 2018-03-20", accountComment="RT#1234"), ] - with self.assertLogs() as log: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - self.assertEqual(log.output, [ - "WARNING:dsa.housekeeping:None: dd_u has accountStatus 'inactive 2018-03-20' " - '(comment: RT#1234) but in our db the state is Debian Developer, uploading [inactive]']) + + self.hk.run(CheckLDAPConsistency) + self.assertInconsistenciesEqual([ + {"person": self.persons.dd_u, + "text": + "person has accountStatus 'inactive 2018-03-20' " + '(comment: RT#1234) but in our db the state is Debian Developer, uploading [inactive]'} + ]) p = self.persons.dd_u p.refresh_from_db() @@ -196,23 +212,21 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): MockEntry("dd_u", supplementaryGid=["Debian"], cn="Dd_u", mn="", sn="", emailForward="dd_u@example.org", accountStatus="retiring 2018-03-20"), ] - with self.assertLogs() as log: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) - self.assertEqual(log.output, [ - "WARNING:dsa.housekeeping:None: dd_u has accountStatus 'retiring 2018-03-20' " - '(comment: None) but in our db the state is Debian Developer, uploading ' - '[retiring]', + self.hk.run(CheckLDAPConsistency) + self.assertInconsistenciesEqual([ + {"person": self.persons.dd_u, + "text": "person has accountStatus 'retiring 2018-03-20' " + '(comment: None) but in our db the state is Debian Developer, uploading ' + '[retiring]'}, ]) process.approved_by = self.persons.dam process.approved_time = now() process.save() with self.assertLogs() as log: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) + self.hk.run(CheckLDAPConsistency) self.assertEqual(log.output, [ - "INFO:dsa.housekeeping:None: dd_u closed from dsa: retiring 2018-03-20", + "INFO:dsa.housekeeping:test.CheckLDAPConsistency: dd_u closed from dsa: retiring 2018-03-20", ]) p = self.persons.dd_u @@ -234,24 +248,25 @@ class TestCheckLDAPConsistency(ProcessFixtureMixin, TestCase): emailForward="test@example.org", accountStatus="retiring 2018-03-20"), ] with self.assertLogs() as log: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) + self.hk.run(CheckLDAPConsistency) self.assertEqual(log.output, [ - "WARNING:dsa.housekeeping:None: dd_u has accountStatus 'retiring 2018-03-20' " - '(comment: None) but in our db the state is Debian Developer, uploading ' - '[retiring]', - "INFO:dsa.housekeeping:None: dd_u changing email_ldap from dd_u@example.org to test@example.org" - " (source: LDAP)", + "INFO:dsa.housekeeping:test.CheckLDAPConsistency: dd_u changing email_ldap" + " from dd_u@example.org to test@example.org (source: LDAP)", + ]) + self.assertInconsistenciesEqual([ + {"person": self.persons.dd_u, + "text": "person has accountStatus 'retiring 2018-03-20' " + '(comment: None) but in our db the state is Debian Developer, uploading ' + '[retiring]'}, ]) process.approved_by = self.persons.dam process.approved_time = now() process.save() with self.assertLogs() as log: - task = CheckLDAPConsistency(self.hk) - task.run_main(None) + self.hk.run(CheckLDAPConsistency) self.assertEqual(log.output, [ - "INFO:dsa.housekeeping:None: dd_u closed from dsa: retiring 2018-03-20", + "INFO:dsa.housekeeping:test.CheckLDAPConsistency: dd_u closed from dsa: retiring 2018-03-20", ]) p = self.persons.dd_u diff --git a/sitechecks/housekeeping.py b/sitechecks/housekeeping.py index 80dfc52c4193f78ba00fe4ce3d657845381e6b71..33b308c9d085156e4f57949d65f4d16db2b97fe4 100644 --- a/sitechecks/housekeeping.py +++ b/sitechecks/housekeeping.py @@ -1,3 +1,4 @@ +from __future__ import annotations from collections import defaultdict import logging import django_housekeeping as hk diff --git a/sitechecks/models.py b/sitechecks/models.py index ac449c1cdbbb25becd4b12cadb5410752bde3451..9af63a87be405c15007f91b57df9fa3b9b66b724 100644 --- a/sitechecks/models.py +++ b/sitechecks/models.py @@ -1,3 +1,4 @@ +from __future__ import annotations from django.db import models from backend.models import Person from process.models import Process diff --git a/test-instance/deploy.yml b/test-instance/deploy.yml index bef5f9a304f9dc037b5506104b69d7ba5a9769b2..74f5476c0e67df5b2e52045fee60ce7b095f8932 100644 --- a/test-instance/deploy.yml +++ b/test-instance/deploy.yml @@ -48,7 +48,6 @@ - gnupg - debian-keyring - python3-git - - python3-testfixtures - libjs-bootstrap4 - fonts-fork-awesome - libjs-jquery-datatables