From 2664f606d1f52bdc4f60dd29700efb528c8fb91b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 28 Jul 2021 18:16:13 -0500 Subject: [PATCH 1/9] Audit: track user events --- readthedocs/audit/__init__.py | 1 + readthedocs/audit/admin.py | 18 +++ readthedocs/audit/apps.py | 12 ++ readthedocs/audit/migrations/0001_initial.py | 40 ++++++ readthedocs/audit/migrations/__init__.py | 0 readthedocs/audit/models.py | 126 +++++++++++++++++++ readthedocs/audit/signals.py | 32 +++++ readthedocs/proxito/views/mixins.py | 18 +++ readthedocs/settings/base.py | 1 + 9 files changed, 248 insertions(+) create mode 100644 readthedocs/audit/__init__.py create mode 100644 readthedocs/audit/admin.py create mode 100644 readthedocs/audit/apps.py create mode 100644 readthedocs/audit/migrations/0001_initial.py create mode 100644 readthedocs/audit/migrations/__init__.py create mode 100644 readthedocs/audit/models.py create mode 100644 readthedocs/audit/signals.py diff --git a/readthedocs/audit/__init__.py b/readthedocs/audit/__init__.py new file mode 100644 index 00000000000..dd4aae82403 --- /dev/null +++ b/readthedocs/audit/__init__.py @@ -0,0 +1 @@ +default_app_config = 'readthedocs.audit.apps.AuditConfig' diff --git a/readthedocs/audit/admin.py b/readthedocs/audit/admin.py new file mode 100644 index 00000000000..773c1597634 --- /dev/null +++ b/readthedocs/audit/admin.py @@ -0,0 +1,18 @@ +from django.contrib import admin + +from readthedocs.audit.models import AuditLog + + +@admin.register(AuditLog) +class AuditLogAdmin(admin.ModelAdmin): + + search_fields = ('log_username', 'browser', 'project__slug') + list_filter = ('log_username', 'ip', 'project', 'action') + list_display = ( + 'action', + 'log_username', + 'project', + 'ip', + 'browser', + 'resource', + ) diff --git a/readthedocs/audit/apps.py b/readthedocs/audit/apps.py new file mode 100644 index 00000000000..98869353c99 --- /dev/null +++ b/readthedocs/audit/apps.py @@ -0,0 +1,12 @@ +import logging + +from django.apps import AppConfig + +log = logging.getLogger(__name__) + +class AuditConfig(AppConfig): + name = 'readthedocs.audit' + + def ready(self): + log.info("Importing all Signals handlers") + import readthedocs.audit.signals # noqa diff --git a/readthedocs/audit/migrations/0001_initial.py b/readthedocs/audit/migrations/0001_initial.py new file mode 100644 index 00000000000..0701f03f3ac --- /dev/null +++ b/readthedocs/audit/migrations/0001_initial.py @@ -0,0 +1,40 @@ +# Generated by Django 2.2.24 on 2021-07-28 23:08 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django_extensions.db.fields + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ('projects', '0080_historicalproject'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='AuditLog', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', django_extensions.db.fields.CreationDateTimeField(auto_now_add=True, verbose_name='created')), + ('modified', django_extensions.db.fields.ModificationDateTimeField(auto_now=True, verbose_name='modified')), + ('log_user_id', models.IntegerField(blank=True, db_index=True, null=True, verbose_name='User ID')), + ('log_username', models.CharField(blank=True, db_index=True, max_length=150, null=True, verbose_name='Username')), + ('action', models.CharField(choices=[('pageview', 'Page view'), ('authentication', 'Authentication'), ('authentication-failure', 'Authentication failure'), ('log-out', 'Log out')], max_length=150, verbose_name='Action')), + ('auth_backend', models.CharField(blank=True, max_length=250, null=True, verbose_name='Auth backend')), + ('ip', models.GenericIPAddressField(blank=True, null=True, verbose_name='IP address')), + ('browser', models.CharField(blank=True, max_length=250, null=True, verbose_name='Browser user-agent')), + ('resource', models.CharField(blank=True, max_length=5500, null=True, verbose_name='Resource')), + ('project', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to='projects.Project', verbose_name='Project')), + ('user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL, verbose_name='User')), + ], + options={ + 'get_latest_by': 'modified', + 'abstract': False, + }, + ), + ] diff --git a/readthedocs/audit/migrations/__init__.py b/readthedocs/audit/migrations/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/readthedocs/audit/models.py b/readthedocs/audit/models.py new file mode 100644 index 00000000000..d6401325bfd --- /dev/null +++ b/readthedocs/audit/models.py @@ -0,0 +1,126 @@ +from django.contrib.auth.models import User +from django.db import models +from django.utils.translation import ugettext_lazy as _ +from django_extensions.db.models import TimeStampedModel + + +class AuditLogManager(models.Manager): + + def new(self, action, user=None, request=None, **kwargs): + """ + Create an audit log for `action`. + + If user or request are given, + other fields will be auto-populated from that information. + """ + + actions_requiring_user = (AuditLog.PAGEVIEW, AuditLog.AUTHN, AuditLog.LOGOUT) + if action in actions_requiring_user and (not user or not request): + raise TypeError(f'A user and a request is required for the {action} action.') + if action == AuditLog.PAGEVIEW and not 'project' in kwargs: + raise TypeError(f'A project is required for the {action} action.') + + if user: + kwargs.setdefault('auth_backend', getattr(user, 'backend', None)) + if user.is_authenticated: + kwargs['log_user_id'] = user.id + kwargs['log_username'] = user.username + else: + user = None + + if request: + kwargs['ip'] = request.META.get('REMOTE_ADDR') + kwargs['browser'] = request.headers.get('User-Agent') + kwargs.setdefault('resource', request.path_info) + + return self.create( + user=user, + action=action, + **kwargs, + ) + + +class AuditLog(TimeStampedModel): + """ + Track user actions for audit purposes. + + A log can be attached to a user and/or project. + Logs attached to a project will be deleted when the project is deleted. + + If the user is deleted the log will be preserved, + and the deleted user can be accessed via ``log_user_id`` and ``log_username``. + """ + + PAGEVIEW = 'pageview' + AUTHN = 'authentication' + AUTHN_FAILURE = 'authentication-failure' + LOGOUT = 'log-out' + + CHOICES = ( + (PAGEVIEW, 'Page view'), + (AUTHN, 'Authentication'), + (AUTHN_FAILURE, 'Authentication failure'), + (LOGOUT, 'Log out'), + ) + + user = models.ForeignKey( + User, + verbose_name=_('User'), + null=True, + on_delete=models.SET_NULL, + db_index=True, + ) + # Extra information in case the user is deleted. + log_user_id = models.IntegerField( + _('User ID'), + blank=True, + null=True, + db_index=True, + ) + log_username = models.CharField( + _('Username'), + max_length=150, + blank=True, + null=True, + db_index=True, + ) + + action = models.CharField( + _('Action'), + max_length=150, + choices=CHOICES, + ) + project = models.ForeignKey( + 'projects.Project', + verbose_name=_('Project'), + null=True, + db_index=True, + on_delete=models.CASCADE, + ) + auth_backend = models.CharField( + _('Auth backend'), + max_length=250, + blank=True, + null=True, + ) + ip = models.GenericIPAddressField( + _('IP address'), + blank=True, + null=True, + ) + browser = models.CharField( + _('Browser user-agent'), + max_length=250, + blank=True, + null=True, + ) + # Resource can be a path, + # set it slightly greater than ``HTMLFile.path``. + resource = models.CharField( + _('Resource'), + max_length=5500, + blank=True, + null=True, + ) + + objects = AuditLogManager() diff --git a/readthedocs/audit/signals.py b/readthedocs/audit/signals.py new file mode 100644 index 00000000000..17c1f1c5a21 --- /dev/null +++ b/readthedocs/audit/signals.py @@ -0,0 +1,32 @@ +from django.contrib.auth.signals import ( + user_logged_in, + user_logged_out, + user_login_failed, +) +from django.dispatch import receiver + +from readthedocs.audit.models import AuditLog + + +@receiver(user_logged_in) +def log_logged_in(sender, request, user, **kwargs): + AuditLog.objects.new( + action=AuditLog.AUTHN, + user=user, + request=request, + ) + +@receiver(user_logged_out) +def log_logged_out(sender, request, user, **kwargs): + AuditLog.objects.new( + action=AuditLog.LOGOUT, + user=user, + request=request, + ) + +@receiver(user_login_failed) +def log_login_failed(sender, credentials, request, **kwargs): + AuditLog.objects.new( + action=AuditLog.AUTHN_FAILURE, + request=request, + ) diff --git a/readthedocs/proxito/views/mixins.py b/readthedocs/proxito/views/mixins.py index 152e397e8cc..14a39f6d79f 100644 --- a/readthedocs/proxito/views/mixins.py +++ b/readthedocs/proxito/views/mixins.py @@ -14,6 +14,7 @@ from django.views.static import serve from slugify import slugify as unicode_slugify +from readthedocs.audit.models import AuditLog from readthedocs.builds.constants import EXTERNAL, INTERNAL from readthedocs.core.resolver import resolve from readthedocs.proxito.constants import ( @@ -53,6 +54,14 @@ def _serve_docs( or "docs-celeryproject-org-kombu-en-stable.pdf") """ + if self._is_audit_enabled(final_project): + AuditLog.objects.new( + action=AuditLog.PAGEVIEW, + user=request.user, + request=request, + project=final_project, + ) + if settings.PYTHON_MEDIA: return self._serve_docs_python( request, @@ -68,6 +77,15 @@ def _serve_docs( download=download, ) + def _is_audit_enabled(self, project): + """ + Check if the project has the audit feature enabled to track individual page views. + + This feature is different from page views analytics, + as it records every page view individually with more metadata like the user, IP, etc. + """ + return True + def _serve_docs_python(self, request, final_project, path): """ Serve docs from Python. diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index bfc79035f8b..8a884d725fc 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -176,6 +176,7 @@ def INSTALLED_APPS(self): # noqa 'readthedocs.oauth', 'readthedocs.redirects', 'readthedocs.sso', + 'readthedocs.audit', 'readthedocs.rtd_tests', 'readthedocs.api.v2', 'readthedocs.api.v3', From d0335160a40603b941d105bcb6acd6be36f51843 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 29 Jul 2021 09:49:24 -0500 Subject: [PATCH 2/9] Track selected files only --- readthedocs/proxito/views/mixins.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/readthedocs/proxito/views/mixins.py b/readthedocs/proxito/views/mixins.py index 14a39f6d79f..a9bbf24ac69 100644 --- a/readthedocs/proxito/views/mixins.py +++ b/readthedocs/proxito/views/mixins.py @@ -54,7 +54,12 @@ def _serve_docs( or "docs-celeryproject-org-kombu-en-stable.pdf") """ - if self._is_audit_enabled(final_project): + track_file = any( + path.endswith(ext) + for ext in ['.html', '.pdf', '.epub', '.zip'] + ) + + if track_file and self._is_audit_enabled(final_project): AuditLog.objects.new( action=AuditLog.PAGEVIEW, user=request.user, From d0bac57b05dd6e3fc237a87909043ef4084fc763 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 29 Jul 2021 10:06:42 -0500 Subject: [PATCH 3/9] Linter --- readthedocs/audit/__init__.py | 2 +- readthedocs/audit/admin.py | 2 ++ readthedocs/audit/apps.py | 3 +++ readthedocs/audit/models.py | 7 ++++++- readthedocs/audit/signals.py | 10 ++++++++++ 5 files changed, 22 insertions(+), 2 deletions(-) diff --git a/readthedocs/audit/__init__.py b/readthedocs/audit/__init__.py index dd4aae82403..382291a45d2 100644 --- a/readthedocs/audit/__init__.py +++ b/readthedocs/audit/__init__.py @@ -1 +1 @@ -default_app_config = 'readthedocs.audit.apps.AuditConfig' +default_app_config = 'readthedocs.audit.apps.AuditConfig' # noqa diff --git a/readthedocs/audit/admin.py b/readthedocs/audit/admin.py index 773c1597634..6253c22bfa1 100644 --- a/readthedocs/audit/admin.py +++ b/readthedocs/audit/admin.py @@ -1,3 +1,5 @@ +"""Audit admin.""" + from django.contrib import admin from readthedocs.audit.models import AuditLog diff --git a/readthedocs/audit/apps.py b/readthedocs/audit/apps.py index 98869353c99..38996bf6569 100644 --- a/readthedocs/audit/apps.py +++ b/readthedocs/audit/apps.py @@ -1,9 +1,12 @@ +"""Audit module.""" + import logging from django.apps import AppConfig log = logging.getLogger(__name__) + class AuditConfig(AppConfig): name = 'readthedocs.audit' diff --git a/readthedocs/audit/models.py b/readthedocs/audit/models.py index d6401325bfd..c484a1b791f 100644 --- a/readthedocs/audit/models.py +++ b/readthedocs/audit/models.py @@ -1,3 +1,5 @@ +"""Audit models.""" + from django.contrib.auth.models import User from django.db import models from django.utils.translation import ugettext_lazy as _ @@ -6,6 +8,8 @@ class AuditLogManager(models.Manager): + """AuditLog manager.""" + def new(self, action, user=None, request=None, **kwargs): """ Create an audit log for `action`. @@ -17,7 +21,7 @@ def new(self, action, user=None, request=None, **kwargs): actions_requiring_user = (AuditLog.PAGEVIEW, AuditLog.AUTHN, AuditLog.LOGOUT) if action in actions_requiring_user and (not user or not request): raise TypeError(f'A user and a request is required for the {action} action.') - if action == AuditLog.PAGEVIEW and not 'project' in kwargs: + if action == AuditLog.PAGEVIEW and 'project' not in kwargs: raise TypeError(f'A project is required for the {action} action.') if user: @@ -41,6 +45,7 @@ def new(self, action, user=None, request=None, **kwargs): class AuditLog(TimeStampedModel): + """ Track user actions for audit purposes. diff --git a/readthedocs/audit/signals.py b/readthedocs/audit/signals.py index 17c1f1c5a21..c61f88f2a1a 100644 --- a/readthedocs/audit/signals.py +++ b/readthedocs/audit/signals.py @@ -1,3 +1,5 @@ +"""Audit signals.""" + from django.contrib.auth.signals import ( user_logged_in, user_logged_out, @@ -10,22 +12,30 @@ @receiver(user_logged_in) def log_logged_in(sender, request, user, **kwargs): + """Log when a user has logged in.""" + # pylint: disable=unused-argument AuditLog.objects.new( action=AuditLog.AUTHN, user=user, request=request, ) + @receiver(user_logged_out) def log_logged_out(sender, request, user, **kwargs): + """Log when a user has logged out.""" + # pylint: disable=unused-argument AuditLog.objects.new( action=AuditLog.LOGOUT, user=user, request=request, ) + @receiver(user_login_failed) def log_login_failed(sender, credentials, request, **kwargs): + """Log when a user has failed to logged in.""" + # pylint: disable=unused-argument AuditLog.objects.new( action=AuditLog.AUTHN_FAILURE, request=request, From fa760efe351159790f25cb7be525165c9577eb99 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 29 Jul 2021 10:21:21 -0500 Subject: [PATCH 4/9] Log username on auth failure --- readthedocs/audit/signals.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/readthedocs/audit/signals.py b/readthedocs/audit/signals.py index c61f88f2a1a..6d5ae1a6f96 100644 --- a/readthedocs/audit/signals.py +++ b/readthedocs/audit/signals.py @@ -1,11 +1,13 @@ """Audit signals.""" +from django.contrib.auth.models import User from django.contrib.auth.signals import ( user_logged_in, user_logged_out, user_login_failed, ) from django.dispatch import receiver +from django.db.models import Q from readthedocs.audit.models import AuditLog @@ -36,7 +38,14 @@ def log_logged_out(sender, request, user, **kwargs): def log_login_failed(sender, credentials, request, **kwargs): """Log when a user has failed to logged in.""" # pylint: disable=unused-argument + username = credentials.get('username') + user = ( + User.objects.filter(Q(username=username) | Q(email=username)) + .first() + ) AuditLog.objects.new( action=AuditLog.AUTHN_FAILURE, + user=user, + log_username=username, request=request, ) From 08db200e13eb38effc3984610aaf18a6936f18a2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 29 Jul 2021 10:48:03 -0500 Subject: [PATCH 5/9] Improvements --- readthedocs/acl/__init__.py | 0 readthedocs/acl/constants.py | 1 + readthedocs/acl/utils.py | 9 ++++ readthedocs/audit/admin.py | 9 ++-- readthedocs/audit/migrations/0001_initial.py | 8 +-- readthedocs/audit/models.py | 57 +++++++++++++------- readthedocs/audit/signals.py | 5 +- readthedocs/proxito/views/mixins.py | 30 ++++++----- 8 files changed, 81 insertions(+), 38 deletions(-) create mode 100644 readthedocs/acl/__init__.py create mode 100644 readthedocs/acl/constants.py create mode 100644 readthedocs/acl/utils.py diff --git a/readthedocs/acl/__init__.py b/readthedocs/acl/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/readthedocs/acl/constants.py b/readthedocs/acl/constants.py new file mode 100644 index 00000000000..48099918e1a --- /dev/null +++ b/readthedocs/acl/constants.py @@ -0,0 +1 @@ +BACKEND_REQUEST_KEY = '_auth_request_key' diff --git a/readthedocs/acl/utils.py b/readthedocs/acl/utils.py new file mode 100644 index 00000000000..019fe0d64cf --- /dev/null +++ b/readthedocs/acl/utils.py @@ -0,0 +1,9 @@ +from django.contrib.auth import BACKEND_SESSION_KEY + +from readthedocs.acl.constants import BACKEND_REQUEST_KEY + + +def get_auth_backend(request): + backend_auth = request.session.get(BACKEND_SESSION_KEY) + backend_perm = getattr(request, BACKEND_REQUEST_KEY, None) + return backend_auth or backend_perm diff --git a/readthedocs/audit/admin.py b/readthedocs/audit/admin.py index 6253c22bfa1..1432e9c079b 100644 --- a/readthedocs/audit/admin.py +++ b/readthedocs/audit/admin.py @@ -8,12 +8,13 @@ @admin.register(AuditLog) class AuditLogAdmin(admin.ModelAdmin): - search_fields = ('log_username', 'browser', 'project__slug') - list_filter = ('log_username', 'ip', 'project', 'action') + raw_id_fields = ('user', 'project') + search_fields = ('log_user_username', 'browser', 'log_project_slug') + list_filter = ('log_user_username', 'ip', 'log_project_slug', 'action') list_display = ( 'action', - 'log_username', - 'project', + 'log_user_username', + 'log_project_slug', 'ip', 'browser', 'resource', diff --git a/readthedocs/audit/migrations/0001_initial.py b/readthedocs/audit/migrations/0001_initial.py index 0701f03f3ac..00e928eef88 100644 --- a/readthedocs/audit/migrations/0001_initial.py +++ b/readthedocs/audit/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 2.2.24 on 2021-07-28 23:08 +# Generated by Django 2.2.24 on 2021-07-29 18:34 from django.conf import settings from django.db import migrations, models @@ -23,13 +23,15 @@ class Migration(migrations.Migration): ('created', django_extensions.db.fields.CreationDateTimeField(auto_now_add=True, verbose_name='created')), ('modified', django_extensions.db.fields.ModificationDateTimeField(auto_now=True, verbose_name='modified')), ('log_user_id', models.IntegerField(blank=True, db_index=True, null=True, verbose_name='User ID')), - ('log_username', models.CharField(blank=True, db_index=True, max_length=150, null=True, verbose_name='Username')), + ('log_user_username', models.CharField(blank=True, db_index=True, max_length=150, null=True, verbose_name='Username')), + ('log_project_id', models.IntegerField(blank=True, db_index=True, null=True, verbose_name='Project ID')), + ('log_project_slug', models.CharField(blank=True, db_index=True, max_length=63, null=True, verbose_name='Project slug')), ('action', models.CharField(choices=[('pageview', 'Page view'), ('authentication', 'Authentication'), ('authentication-failure', 'Authentication failure'), ('log-out', 'Log out')], max_length=150, verbose_name='Action')), ('auth_backend', models.CharField(blank=True, max_length=250, null=True, verbose_name='Auth backend')), ('ip', models.GenericIPAddressField(blank=True, null=True, verbose_name='IP address')), ('browser', models.CharField(blank=True, max_length=250, null=True, verbose_name='Browser user-agent')), ('resource', models.CharField(blank=True, max_length=5500, null=True, verbose_name='Resource')), - ('project', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to='projects.Project', verbose_name='Project')), + ('project', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to='projects.Project', verbose_name='Project')), ('user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL, verbose_name='User')), ], options={ diff --git a/readthedocs/audit/models.py b/readthedocs/audit/models.py index c484a1b791f..f3ba18e0670 100644 --- a/readthedocs/audit/models.py +++ b/readthedocs/audit/models.py @@ -5,6 +5,8 @@ from django.utils.translation import ugettext_lazy as _ from django_extensions.db.models import TimeStampedModel +from readthedocs.acl.utils import get_auth_backend + class AuditLogManager(models.Manager): @@ -24,18 +26,15 @@ def new(self, action, user=None, request=None, **kwargs): if action == AuditLog.PAGEVIEW and 'project' not in kwargs: raise TypeError(f'A project is required for the {action} action.') - if user: - kwargs.setdefault('auth_backend', getattr(user, 'backend', None)) - if user.is_authenticated: - kwargs['log_user_id'] = user.id - kwargs['log_username'] = user.username - else: - user = None + # Don't save anonymous users. + if user and user.is_anonymous: + user = None if request: kwargs['ip'] = request.META.get('REMOTE_ADDR') kwargs['browser'] = request.headers.get('User-Agent') kwargs.setdefault('resource', request.path_info) + kwargs.setdefault('auth_backend', get_auth_backend(request)) return self.create( user=user, @@ -50,10 +49,8 @@ class AuditLog(TimeStampedModel): Track user actions for audit purposes. A log can be attached to a user and/or project. - Logs attached to a project will be deleted when the project is deleted. - - If the user is deleted the log will be preserved, - and the deleted user can be accessed via ``log_user_id`` and ``log_username``. + If the user or project are deleted the log will be preserved, + and the deleted user/project can be accessed via the ``log_*`` attributes. """ PAGEVIEW = 'pageview' @@ -82,7 +79,7 @@ class AuditLog(TimeStampedModel): null=True, db_index=True, ) - log_username = models.CharField( + log_user_username = models.CharField( _('Username'), max_length=150, blank=True, @@ -90,17 +87,32 @@ class AuditLog(TimeStampedModel): db_index=True, ) - action = models.CharField( - _('Action'), - max_length=150, - choices=CHOICES, - ) project = models.ForeignKey( 'projects.Project', verbose_name=_('Project'), null=True, db_index=True, - on_delete=models.CASCADE, + on_delete=models.SET_NULL, + ) + # Extra information in case the project is deleted. + log_project_id = models.IntegerField( + _('Project ID'), + blank=True, + null=True, + db_index=True, + ) + log_project_slug = models.CharField( + _('Project slug'), + max_length=63, + blank=True, + null=True, + db_index=True, + ) + + action = models.CharField( + _('Action'), + max_length=150, + choices=CHOICES, ) auth_backend = models.CharField( _('Auth backend'), @@ -129,3 +141,12 @@ class AuditLog(TimeStampedModel): ) objects = AuditLogManager() + + def save(self, **kwargs): + if self.user: + self.log_user_id = self.user.id + self.log_user_username = self.user.username + if self.project: + self.log_project_id = self.project.id + self.log_project_slug = self.project.slug + super().save(**kwargs) diff --git a/readthedocs/audit/signals.py b/readthedocs/audit/signals.py index 6d5ae1a6f96..dd6b51da0fa 100644 --- a/readthedocs/audit/signals.py +++ b/readthedocs/audit/signals.py @@ -27,6 +27,9 @@ def log_logged_in(sender, request, user, **kwargs): def log_logged_out(sender, request, user, **kwargs): """Log when a user has logged out.""" # pylint: disable=unused-argument + # Only log if there is an user. + if not user: + return AuditLog.objects.new( action=AuditLog.LOGOUT, user=user, @@ -46,6 +49,6 @@ def log_login_failed(sender, credentials, request, **kwargs): AuditLog.objects.new( action=AuditLog.AUTHN_FAILURE, user=user, - log_username=username, + log_user_username=username, request=request, ) diff --git a/readthedocs/proxito/views/mixins.py b/readthedocs/proxito/views/mixins.py index a9bbf24ac69..6fdac9634e6 100644 --- a/readthedocs/proxito/views/mixins.py +++ b/readthedocs/proxito/views/mixins.py @@ -54,18 +54,7 @@ def _serve_docs( or "docs-celeryproject-org-kombu-en-stable.pdf") """ - track_file = any( - path.endswith(ext) - for ext in ['.html', '.pdf', '.epub', '.zip'] - ) - - if track_file and self._is_audit_enabled(final_project): - AuditLog.objects.new( - action=AuditLog.PAGEVIEW, - user=request.user, - request=request, - project=final_project, - ) + self._trak_pageview(final_project, path, request) if settings.PYTHON_MEDIA: return self._serve_docs_python( @@ -82,6 +71,23 @@ def _serve_docs( download=download, ) + def _trak_pageview(self, project, path, request): + """Create an audit log of the page view if audit is enabled.""" + # Remove any query args (like the token access from AWS). + path_only = urlparse(path).path + track_file = any( + path_only.endswith(ext) + for ext in ['.html', '.pdf', '.epub', '.zip'] + ) + + if track_file and self._is_audit_enabled(project): + AuditLog.objects.new( + action=AuditLog.PAGEVIEW, + user=request.user, + request=request, + project=project, + ) + def _is_audit_enabled(self, project): """ Check if the project has the audit feature enabled to track individual page views. From 82cec5145e4898f2182901637b47337e22a847f0 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 29 Jul 2021 15:11:41 -0500 Subject: [PATCH 6/9] Tests --- readthedocs/audit/tests/__init__.py | 0 readthedocs/audit/tests/test_signals.py | 41 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 readthedocs/audit/tests/__init__.py create mode 100644 readthedocs/audit/tests/test_signals.py diff --git a/readthedocs/audit/tests/__init__.py b/readthedocs/audit/tests/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/readthedocs/audit/tests/test_signals.py b/readthedocs/audit/tests/test_signals.py new file mode 100644 index 00000000000..33de521e108 --- /dev/null +++ b/readthedocs/audit/tests/test_signals.py @@ -0,0 +1,41 @@ +from django.contrib.auth.models import User +from django.test import TestCase +from django_dynamic_fixture import get + +from readthedocs.audit.models import AuditLog + + +class TestSignals(TestCase): + + def setUp(self): + self.user = get( + User, + username='test', + ) + self.user.set_password('password') + self.user.save() + + def test_log_logged_in(self): + self.assertEqual(AuditLog.objects.all().count(), 0) + self.assertTrue(self.client.login(username='test', password='password')) + self.assertEqual(AuditLog.objects.all().count(), 1) + log = AuditLog.objects.first() + self.assertEqual(log.user, self.user) + self.assertEqual(log.action, AuditLog.AUTHN) + + def test_log_logged_out(self): + self.assertEqual(AuditLog.objects.all().count(), 0) + self.assertTrue(self.client.login(username='test', password='password')) + self.client.logout() + self.assertEqual(AuditLog.objects.all().count(), 2) + log = AuditLog.objects.last() + self.assertEqual(log.user, self.user) + self.assertEqual(log.action, AuditLog.LOGOUT) + + def test_log_login_failed(self): + self.assertEqual(AuditLog.objects.all().count(), 0) + self.assertFalse(self.client.login(username='test', password='incorrect')) + self.assertEqual(AuditLog.objects.all().count(), 1) + log = AuditLog.objects.first() + self.assertEqual(log.user, self.user) + self.assertEqual(log.action, AuditLog.AUTHN_FAILURE) From 364a037a5736168fc1e7826267d7890f6d5bf7e5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 29 Jul 2021 18:14:34 -0500 Subject: [PATCH 7/9] More tests --- readthedocs/proxito/tests/test_full.py | 40 +++++++++++++++++++++++--- readthedocs/proxito/views/mixins.py | 12 +++----- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/readthedocs/proxito/tests/test_full.py b/readthedocs/proxito/tests/test_full.py index 180aaa45a9b..58b9fc8e185 100644 --- a/readthedocs/proxito/tests/test_full.py +++ b/readthedocs/proxito/tests/test_full.py @@ -1,28 +1,30 @@ # Copied from .org import os +from textwrap import dedent from unittest import mock import django_dynamic_fixture as fixture from django.conf import settings -from textwrap import dedent from django.core.cache import cache from django.http import HttpResponse from django.test.utils import override_settings from django.urls import reverse +from readthedocs.audit.models import AuditLog from readthedocs.builds.constants import EXTERNAL, INTERNAL, LATEST from readthedocs.builds.models import Version from readthedocs.projects import constants from readthedocs.projects.constants import ( MKDOCS, + PRIVATE, + PUBLIC, SPHINX, SPHINX_HTMLDIR, SPHINX_SINGLEHTML, - PUBLIC, - PRIVATE, ) -from readthedocs.projects.models import Project, Domain +from readthedocs.projects.models import Domain, Project +from readthedocs.proxito.views.mixins import ServeDocsMixin from readthedocs.rtd_tests.storage import BuildMediaFileSystemStorageTest from .base import BaseDocServing @@ -303,6 +305,36 @@ def test_project_nginx_serving_unicode_filename(self): '/proxito/media/html/project/latest/%C3%BA%C3%B1%C3%AD%C4%8D%C3%B3d%C3%A9.html', ) + @mock.patch.object(ServeDocsMixin, '_is_audit_enabled') + def test_track_html_files_only(self, is_audit_enabled): + is_audit_enabled.return_value = False + + self.assertEqual(AuditLog.objects.all().count(), 0) + url = '/en/latest/awesome.html' + host = 'project.dev.readthedocs.io' + resp = self.client.get(url, HTTP_HOST=host) + self.assertIn('x-accel-redirect', resp) + self.assertEqual(AuditLog.objects.all().count(), 0) + + is_audit_enabled.return_value = True + url = '/en/latest/awesome.html' + host = 'project.dev.readthedocs.io' + resp = self.client.get(url, HTTP_HOST=host) + self.assertIn('x-accel-redirect', resp) + self.assertEqual(AuditLog.objects.all().count(), 1) + + log = AuditLog.objects.last() + self.assertEqual(log.user, None) + self.assertEqual(log.project, self.project) + self.assertEqual(log.resource, url) + self.assertEqual(log.action, AuditLog.PAGEVIEW) + + resp = self.client.get('/en/latest/awesome.js', HTTP_HOST=host) + self.assertIn('x-accel-redirect', resp) + resp = self.client.get('/en/latest/awesome.css', HTTP_HOST=host) + self.assertIn('x-accel-redirect', resp) + self.assertEqual(AuditLog.objects.all().count(), 1) + @override_settings( PYTHON_MEDIA=False, diff --git a/readthedocs/proxito/views/mixins.py b/readthedocs/proxito/views/mixins.py index 6fdac9634e6..98498cbb0dc 100644 --- a/readthedocs/proxito/views/mixins.py +++ b/readthedocs/proxito/views/mixins.py @@ -54,7 +54,7 @@ def _serve_docs( or "docs-celeryproject-org-kombu-en-stable.pdf") """ - self._trak_pageview(final_project, path, request) + self._track_pageview(final_project, path, request) if settings.PYTHON_MEDIA: return self._serve_docs_python( @@ -71,15 +71,11 @@ def _serve_docs( download=download, ) - def _trak_pageview(self, project, path, request): + def _track_pageview(self, project, path, request): """Create an audit log of the page view if audit is enabled.""" # Remove any query args (like the token access from AWS). path_only = urlparse(path).path - track_file = any( - path_only.endswith(ext) - for ext in ['.html', '.pdf', '.epub', '.zip'] - ) - + track_file = path_only.endswith(('.html', '.pdf', '.epub', '.zip')) if track_file and self._is_audit_enabled(project): AuditLog.objects.new( action=AuditLog.PAGEVIEW, @@ -95,7 +91,7 @@ def _is_audit_enabled(self, project): This feature is different from page views analytics, as it records every page view individually with more metadata like the user, IP, etc. """ - return True + return False def _serve_docs_python(self, request, final_project, path): """ From d5a8f214eb1bf76e7c8fb86150b5531cc62fea58 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 29 Jul 2021 18:20:38 -0500 Subject: [PATCH 8/9] Docstring --- readthedocs/acl/utils.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/readthedocs/acl/utils.py b/readthedocs/acl/utils.py index 019fe0d64cf..9f8084a23b8 100644 --- a/readthedocs/acl/utils.py +++ b/readthedocs/acl/utils.py @@ -4,6 +4,13 @@ def get_auth_backend(request): + """ + Get the current auth_backend used on this request. + + By default the full qualified name of the backend class + is stored on the request session, but our internal + backends set this as an attribute on the request object. + """ backend_auth = request.session.get(BACKEND_SESSION_KEY) backend_perm = getattr(request, BACKEND_REQUEST_KEY, None) return backend_auth or backend_perm From d4272d3a6aec77a79ed30fcff8473355dd1667c4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 2 Aug 2021 08:53:46 -0500 Subject: [PATCH 9/9] str --- readthedocs/audit/models.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/readthedocs/audit/models.py b/readthedocs/audit/models.py index f3ba18e0670..b9a6a7e8702 100644 --- a/readthedocs/audit/models.py +++ b/readthedocs/audit/models.py @@ -150,3 +150,6 @@ def save(self, **kwargs): self.log_project_id = self.project.id self.log_project_slug = self.project.slug super().save(**kwargs) + + def __str__(self): + return self.action