HistoricalRecords: add additional fields (ip and browser) (#8490)

Author: stsewd

readthedocs/core/history.py

@@ -29,7 +29,8 @@ class ExtraFieldsHistoricalModel(models.Model):
 
     Extra data includes:
 
-    - Users after they have been deleted.
+    - User information to retain after they have been deleted
+    - IP & browser
     """
 
     extra_history_user_id = models.IntegerField(
@@ -44,6 +45,18 @@ class ExtraFieldsHistoricalModel(models.Model):
         null=True,
         db_index=True,
     )
+    extra_history_ip = models.CharField(
+        _('IP address'),
+        blank=True,
+        null=True,
+        max_length=250,
+    )
+    extra_history_browser = models.CharField(
+        _('Browser user-agent'),
+        max_length=250,
+        blank=True,
+        null=True,
+    )
 
     class Meta:
         abstract = True

readthedocs/core/migrations/0008_add_extra_history_fields.py

@@ -0,0 +1,23 @@
+# Generated by Django 2.2.24 on 2021-09-29 19:27
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0007_historicaluser'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='historicaluser',
+            name='extra_history_browser',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='Browser user-agent'),
+        ),
+        migrations.AddField(
+            model_name='historicaluser',
+            name='extra_history_ip',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='IP address'),
+        ),
+    ]

readthedocs/core/signals.py

@@ -8,8 +8,10 @@
 from django.db.models.signals import pre_delete
 from django.dispatch import Signal, receiver
 from rest_framework.permissions import SAFE_METHODS
+from simple_history.models import HistoricalRecords
 from simple_history.signals import pre_create_historical_record
 
+from readthedocs.analytics.utils import get_client_ip
 from readthedocs.builds.models import Version
 from readthedocs.core.unresolver import unresolve
 from readthedocs.projects.models import Project
@@ -122,10 +124,18 @@ def delete_projects(sender, instance, *args, **kwargs):
 @receiver(pre_create_historical_record)
 def add_extra_historical_fields(sender, **kwargs):
     history_instance = kwargs['history_instance']
+    if not history_instance:
+        return
+
     history_user = kwargs['history_user']
-    if history_instance and history_user:
+    if history_user:
         history_instance.extra_history_user_id = history_user.id
         history_instance.extra_history_user_username = history_user.username
 
+    request = getattr(HistoricalRecords.context, 'request', None)
+    if request:
+        history_instance.extra_history_ip = get_client_ip(request)
+        history_instance.extra_history_browser = request.headers.get('User-Agent')
+
 
 signals.check_request_enabled.connect(decide_if_cors)

readthedocs/core/tests/test_history.py

@@ -0,0 +1,49 @@
+from django.contrib.auth.models import User
+from django.test import TestCase
+from django.urls import reverse
+from django_dynamic_fixture import get
+
+from readthedocs.projects.models import Project
+
+
+class TestHistoricalModels(TestCase):
+
+    def setUp(self):
+        self.user = get(User)
+        self.project = get(Project, users=[self.user])
+        self.client.force_login(self.user)
+
+    def test_extra_historical_fields_with_request(self):
+        self.assertEqual(self.project.history.count(), 1)
+        r = self.client.post(
+            reverse('projects_edit', args=[self.project.slug]),
+            data={
+                'name': 'Changed!',
+                'repo': 'https://github.com/readthedocs/readthedocs',
+                'repo_type': self.project.repo_type,
+                'language': self.project.language,
+            },
+            HTTP_USER_AGENT='Firefox',
+        )
+        self.assertEqual(r.status_code, 302)
+        self.assertEqual(self.project.history.count(), 2)
+        history = self.project.history.first()
+        self.assertEqual(history.name, 'Changed!')
+        self.assertEqual(history.history_user, self.user)
+        self.assertEqual(history.extra_history_user_id, self.user.id)
+        self.assertEqual(history.extra_history_user_username, self.user.username)
+        self.assertEqual(history.extra_history_ip, '127.0.0.1')
+        self.assertEqual(history.extra_history_browser, 'Firefox')
+
+    def test_extra_historical_fields_outside_request(self):
+        self.assertEqual(self.project.history.count(), 1)
+        self.project.name = 'Changed!'
+        self.project.save()
+        self.assertEqual(self.project.history.count(), 2)
+        history = self.project.history.first()
+        self.assertEqual(history.name, 'Changed!')
+        self.assertIsNone(history.history_user)
+        self.assertIsNone(history.extra_history_user_id)
+        self.assertIsNone(history.extra_history_user_username)
+        self.assertIsNone(history.extra_history_ip)
+        self.assertIsNone(history.extra_history_browser)

readthedocs/organizations/migrations/0007_add_extra_history_fields.py

@@ -0,0 +1,33 @@
+# Generated by Django 2.2.24 on 2021-09-29 19:27
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('organizations', '0006_add_assets_cleaned'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='historicalorganization',
+            name='extra_history_browser',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='Browser user-agent'),
+        ),
+        migrations.AddField(
+            model_name='historicalorganization',
+            name='extra_history_ip',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='IP address'),
+        ),
+        migrations.AddField(
+            model_name='historicalteam',
+            name='extra_history_browser',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='Browser user-agent'),
+        ),
+        migrations.AddField(
+            model_name='historicalteam',
+            name='extra_history_ip',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='IP address'),
+        ),
+    ]

readthedocs/projects/migrations/0082_add_extra_history_fields.py

@@ -0,0 +1,23 @@
+# Generated by Django 2.2.24 on 2021-09-29 19:27
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('projects', '0081_add_another_header'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='historicalproject',
+            name='extra_history_browser',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='Browser user-agent'),
+        ),
+        migrations.AddField(
+            model_name='historicalproject',
+            name='extra_history_ip',
+            field=models.CharField(blank=True, max_length=250, null=True, verbose_name='IP address'),
+        ),
+    ]