From 19558114aebd8409970ca52c3b26bc9432a97a26 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 26 Oct 2021 16:34:20 -0500 Subject: [PATCH 1/5] Security logs: delete old user security logs --- readthedocs/audit/tasks.py | 31 +++++++ readthedocs/audit/tests/test_tasks.py | 116 ++++++++++++++++++++++++++ readthedocs/settings/base.py | 5 ++ 3 files changed, 152 insertions(+) create mode 100644 readthedocs/audit/tasks.py create mode 100644 readthedocs/audit/tests/test_tasks.py diff --git a/readthedocs/audit/tasks.py b/readthedocs/audit/tasks.py new file mode 100644 index 00000000000..cae54250add --- /dev/null +++ b/readthedocs/audit/tasks.py @@ -0,0 +1,31 @@ +"""Celery tasks related to audit logs.""" + +import logging + +from django.conf import settings +from django.utils import timezone + +from readthedocs.audit.models import AuditLog +from readthedocs.worker import app + +log = logging.getLogger(__name__) + + +@app.task(queue='web') +def delete_old_personal_audit_logs(days=None): + """ + Delete personal security logs older than `days`. + + If `days` isn't given, default to ``RTD_AUDITLOGS_DEFAULT_RETENTION_DAYS``. + + We delete logs that aren't related to an organization, + there are tasks com .com to delete those according to their plan. + """ + days = days or settings.RTD_AUDITLOGS_DEFAULT_RETENTION_DAYS + days_ago = timezone.now() - timezone.timedelta(days=days) + audit_logs = AuditLog.objects.filter( + log_organization_id__isnull=True, + created__lt=days_ago, + ) + log.info("Deleting old audit logs. days=%s count=%s", days, audit_logs.count()) + audit_logs.delete() diff --git a/readthedocs/audit/tests/test_tasks.py b/readthedocs/audit/tests/test_tasks.py new file mode 100644 index 00000000000..59a8b5dbf2b --- /dev/null +++ b/readthedocs/audit/tests/test_tasks.py @@ -0,0 +1,116 @@ +from unittest import mock + +from django.contrib.auth.models import User +from django.test import TestCase +from django.utils import timezone +from django_dynamic_fixture import get + +from readthedocs.audit.models import AuditLog +from readthedocs.audit.tasks import delete_old_personal_audit_logs +from readthedocs.organizations.models import Organization +from readthedocs.projects.models import Project + + +class AuditTasksTest(TestCase): + + def setUp(self): + self.user = get(User) + self.project = get( + Project, + slug='project', + ) + self.organization = get( + Organization, + owners=[self.user], + name='testorg', + ) + self.organization.projects.add(self.project) + + self.another_user = get(User) + self.another_project = get( + Project, + slug='another-project', + users=[self.user], + ) + + @mock.patch('django.utils.timezone.now') + def test_delete_old_personal_audit_logs(self, now_mock): + now_mock.return_value = timezone.datetime( + year=2021, + month=5, + day=5, + ) + newer_date = timezone.datetime( + year=2021, + month=4, + day=30, + ) + middle_date = timezone.datetime( + year=2021, + month=4, + day=5, + ) + old_date = timezone.datetime( + year=2021, + month=3, + day=20, + ) + for date in [newer_date, middle_date, old_date]: + for user in [self.user, self.another_user]: + # Log attached to a project and organization. + get( + AuditLog, + user=user, + project=self.project, + created=date, + action=AuditLog.PAGEVIEW, + ) + # Log attached to a project only. + get( + AuditLog, + user=user, + project=self.another_project, + created=date, + action=AuditLog.PAGEVIEW, + ) + + # Log attached to the user only. + get( + AuditLog, + user=user, + created=date, + action=AuditLog.AUTHN, + ) + + # Log without a user. + get( + AuditLog, + created=date, + action=AuditLog.AUTHN_FAILURE, + ) + + # Log with a organization, and without a user. + get( + AuditLog, + project=self.project, + created=date, + action=AuditLog.AUTHN_FAILURE, + ) + + self.assertEqual(AuditLog.objects.all().count(), 24) + + # We don't have logs older than 90 days. + delete_old_personal_audit_logs(days=90) + self.assertEqual(AuditLog.objects.all().count(), 24) + + # Only 5 logs can be deteled. + delete_old_personal_audit_logs(days=30) + self.assertEqual(AuditLog.objects.all().count(), 19) + + # Only 5 logs can be deteled. + delete_old_personal_audit_logs(days=10) + self.assertEqual(AuditLog.objects.all().count(), 14) + + # Only 5 logs can be deteled. + delete_old_personal_audit_logs(days=1) + self.assertEqual(AuditLog.objects.all().count(), 9) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 7fb076179a3..41058c37579 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -427,6 +427,11 @@ def TEMPLATES(self): 'schedule': crontab(minute=0, hour=1), 'options': {'queue': 'web'}, }, + 'every-week-delete-old-personal-audit-logs': { + 'task': 'readthedocs.audit.tasks.delete_old_personal_audit_logs', + 'schedule': crontab(day_of_week='wednesday', hour=7, minute=0), + 'options': {'queue': 'web'}, + }, 'every-day-resync-sso-organization-users': { 'task': 'readthedocs.oauth.tasks.sync_remote_repositories_organizations', 'schedule': crontab(minute=0, hour=4), From d34352325066f42230aab8fc4a4dac025bc8e7f0 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 28 Oct 2021 09:34:36 -0500 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Manuel Kaufmann --- readthedocs/audit/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/audit/tasks.py b/readthedocs/audit/tasks.py index cae54250add..d0c81d83140 100644 --- a/readthedocs/audit/tasks.py +++ b/readthedocs/audit/tasks.py @@ -19,7 +19,7 @@ def delete_old_personal_audit_logs(days=None): If `days` isn't given, default to ``RTD_AUDITLOGS_DEFAULT_RETENTION_DAYS``. We delete logs that aren't related to an organization, - there are tasks com .com to delete those according to their plan. + there are tasks in .com to delete those according to their plan. """ days = days or settings.RTD_AUDITLOGS_DEFAULT_RETENTION_DAYS days_ago = timezone.now() - timezone.timedelta(days=days) From d4ee0cdd459a78d8024e9b21a63df20dd2f498a4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 19 Jan 2022 16:48:35 -0500 Subject: [PATCH 3/5] Match task name from the one on .com --- readthedocs/settings/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 41058c37579..8ba8929607a 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -427,7 +427,7 @@ def TEMPLATES(self): 'schedule': crontab(minute=0, hour=1), 'options': {'queue': 'web'}, }, - 'every-week-delete-old-personal-audit-logs': { + 'weekly-delete-old-personal-audit-logs': { 'task': 'readthedocs.audit.tasks.delete_old_personal_audit_logs', 'schedule': crontab(day_of_week='wednesday', hour=7, minute=0), 'options': {'queue': 'web'}, From b885bd3671d9924860bb353948807ba3f91073fa Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 30 Mar 2022 12:52:49 -0500 Subject: [PATCH 4/5] Format --- readthedocs/audit/tasks.py | 2 +- readthedocs/audit/tests/test_tasks.py | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/readthedocs/audit/tasks.py b/readthedocs/audit/tasks.py index d0c81d83140..549d79bef06 100644 --- a/readthedocs/audit/tasks.py +++ b/readthedocs/audit/tasks.py @@ -11,7 +11,7 @@ log = logging.getLogger(__name__) -@app.task(queue='web') +@app.task(queue="web") def delete_old_personal_audit_logs(days=None): """ Delete personal security logs older than `days`. diff --git a/readthedocs/audit/tests/test_tasks.py b/readthedocs/audit/tests/test_tasks.py index 59a8b5dbf2b..7b3efbc4974 100644 --- a/readthedocs/audit/tests/test_tasks.py +++ b/readthedocs/audit/tests/test_tasks.py @@ -12,28 +12,27 @@ class AuditTasksTest(TestCase): - def setUp(self): self.user = get(User) self.project = get( Project, - slug='project', + slug="project", ) self.organization = get( Organization, owners=[self.user], - name='testorg', + name="testorg", ) self.organization.projects.add(self.project) self.another_user = get(User) self.another_project = get( Project, - slug='another-project', + slug="another-project", users=[self.user], ) - @mock.patch('django.utils.timezone.now') + @mock.patch("django.utils.timezone.now") def test_delete_old_personal_audit_logs(self, now_mock): now_mock.return_value = timezone.datetime( year=2021, From c7a1a695422bc901ab50cff5aa1bd701897e447c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 31 Aug 2022 11:49:57 -0500 Subject: [PATCH 5/5] Use structlog --- readthedocs/audit/tasks.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/readthedocs/audit/tasks.py b/readthedocs/audit/tasks.py index 549d79bef06..9ca65d07c76 100644 --- a/readthedocs/audit/tasks.py +++ b/readthedocs/audit/tasks.py @@ -1,14 +1,13 @@ """Celery tasks related to audit logs.""" -import logging - +import structlog from django.conf import settings from django.utils import timezone from readthedocs.audit.models import AuditLog from readthedocs.worker import app -log = logging.getLogger(__name__) +log = structlog.get_logger(__name__) @app.task(queue="web") @@ -27,5 +26,5 @@ def delete_old_personal_audit_logs(days=None): log_organization_id__isnull=True, created__lt=days_ago, ) - log.info("Deleting old audit logs. days=%s count=%s", days, audit_logs.count()) + log.info("Deleting old audit logs.", days=days, count=audit_logs.count()) audit_logs.delete()