diff --git a/addons/base/views.py b/addons/base/views.py index 3f96d763781..be3342eb4d7 100644 --- a/addons/base/views.py +++ b/addons/base/views.py @@ -213,6 +213,7 @@ def _record_file_download(target, file_node, query_params, auth, version=None): storage_provider=getattr(file_node, 'provider', '') or '', user_guid=getattr(getattr(auth, 'user', None), '_id', None), ip=request.remote_addr, + user_agent=request.headers.get('User-Agent', ''), source_area=query_params.get('source', ''), tz=query_params.get('tz', ''), ) @@ -246,6 +247,7 @@ def _record_zip_download(payload): status_code=action_meta.get('status_code'), user_guid=(payload.get('auth') or {}).get('id'), ip=action_meta.get('ip'), + user_agent=(payload.get('request_meta') or {}).get('user_agent', ''), source_area=action_meta.get('source', ''), tz=action_meta.get('tz', ''), ) diff --git a/admin/templates/download_events/download_events.html b/admin/templates/download_events/download_events.html index 0149733d2a8..10a5c46ad75 100644 --- a/admin/templates/download_events/download_events.html +++ b/admin/templates/download_events/download_events.html @@ -78,6 +78,10 @@

Download telemetry dashboard

Total downloads
{{ download_events_dashboard.summary.total_downloads }}
+
+ Files: {{ download_events_dashboard.split.file.count }} + · Zips: {{ download_events_dashboard.split.zip.count }} +
Total GB
diff --git a/osf/admin.py b/osf/admin.py index ad63d6bd03d..ebf9790300f 100644 --- a/osf/admin.py +++ b/osf/admin.py @@ -6,7 +6,7 @@ from django.template.response import TemplateResponse from django_extensions.admin import ForeignKeyAutocompleteAdmin from django.contrib.auth.models import Group -from django.db.models import Q, Count, Sum, F, Min, Max +from django.db.models import Q, Count, Sum, F, Min, Max, Case, When, Value, IntegerField from django.db.models.functions import Trunc from django.http import HttpResponseRedirect, HttpResponse, JsonResponse from django.utils import timezone @@ -31,7 +31,7 @@ Notification, DownloadEvent ) -from osf.models import AbstractNode +from osf.models import AbstractNode, Preprint, Guid from osf.models.notification_type import get_default_frequency_choices from osf.models.notable_domain import DomainReference @@ -450,7 +450,7 @@ class DownloadEventsView(admin.ModelAdmin): change_list_template = 'download_events/download_events.html' list_display = ( 'resource_guid', - 'user', + 'user_display', 'download_type', 'outcome', 'zip_completed', @@ -462,6 +462,7 @@ class DownloadEventsView(admin.ModelAdmin): 'storage_region', 'ip', 'source_area', + 'user_agent_display', 'created' ) list_filter = ( @@ -487,11 +488,46 @@ class DownloadEventsView(admin.ModelAdmin): 'storage_provider', 'user_region', 'storage_region', - 'source_area' + 'source_area', + 'user_agent' ) - search_help_text = 'Search by username, full name, user or node guid, ip, path, storage provider, user or storage region, source area.' + search_help_text = 'Search by username, full name, user or node guid, ip, path, storage provider, user or storage region, source area, user agent.' + + def get_queryset(self, request): + """Annotate an outcome rank so the computed Outcome column is sortable. + + The rank mirrors :meth:`outcome` exactly. It's just an ordering key — it doesn't + change what rows are returned, so the table and the dashboard aggregates are + unaffected. + """ + return super().get_queryset(request).annotate( + _outcome_rank=Case( + When(zip_completed=True, then=Value(0)), # Completed + When( + zip_completed=False, + status_code__gte=DOWNLOAD_FAILURE_MIN_STATUS, + then=Value(2), # Failed + ), + When(zip_completed=False, then=Value(1)), # Cancelled + default=Value(3), # single files have no outcome ('—') + output_field=IntegerField(), + ) + ) + + @admin.display(description='User', ordering='user__username') + def user_display(self, obj): + """Sort the User column by the username (email) rather than the raw FK id.""" + return obj.user or '—' + + @admin.display(description='User agent', ordering='user_agent') + def user_agent_display(self, obj): + """Truncated in the table so it doesn't dominate the row; the full value is still + searchable and shows on the record's detail view.""" + if not obj.user_agent: + return '—' + return obj.user_agent if len(obj.user_agent) <= 80 else obj.user_agent[:79] + '…' - @admin.display(description='Outcome') + @admin.display(description='Outcome', ordering='_outcome_rank') def outcome(self, obj): """Human-readable end state. Single files have no outcome — they're recorded at the redirect before any bytes move, so they never report completion.""" @@ -760,7 +796,9 @@ def _build_region_breakdown(self, queryset, field_name): breakdown[region_name]['file_count'] += row['file_count'] breakdown[region_name]['zip_count'] += row['zip_count'] - ordered = sorted(breakdown.items(), key=lambda item: item[1]['gb'], reverse=True)[:10] + # gb descending, then name ascending so the order is deterministic when GB ties + # (and never depends on the incoming queryset's row order) + ordered = sorted(breakdown.items(), key=lambda item: (-item[1]['gb'], item[0]))[:10] max_gb = max((data['gb'] for _, data in ordered), default=0) max_downloads = max((data['downloads'] for _, data in ordered), default=0) return [ @@ -788,6 +826,14 @@ def _build_top_resource_breakdown(self, queryset): titles = dict( AbstractNode.objects.filter(guids___id__in=guids).values_list('guids___id', 'title') ) + # preprints aren't nodes, and a preprint guid can be versioned (e.g. abcde_v1), which + # the node query above never matches. Resolve whatever's left through the guid — at + # most ten lookups, since this is a top-ten table. + for guid in guids: + if guid not in titles: + referent, _ = Guid.load_referent(guid) + if isinstance(referent, Preprint) and referent.title: + titles[guid] = referent.title return [ { # a deleted project keeps its title, but fall back to the bare guid so diff --git a/osf/migrations/0050_downloadevent_user_agent.py b/osf/migrations/0050_downloadevent_user_agent.py new file mode 100644 index 00000000000..7ec8653f902 --- /dev/null +++ b/osf/migrations/0050_downloadevent_user_agent.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.26 on 2026-08-12 12:09 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0049_project_enter'), + ] + + operations = [ + migrations.AddField( + model_name='downloadevent', + name='user_agent', + field=models.TextField(blank=True, default=''), + ), + ] diff --git a/osf/models/download_event.py b/osf/models/download_event.py index 6484dd47b76..f06a34c1b4c 100644 --- a/osf/models/download_event.py +++ b/osf/models/download_event.py @@ -44,6 +44,9 @@ class DownloadEvent(models.Model): user_region = models.CharField(max_length=64, blank=True, default='') ip = models.GenericIPAddressField(null=True, blank=True) source_area = models.CharField(max_length=128, blank=True, default='') + # the client's User-Agent, to tell frontend downloads apart from API clients and + # crawlers. Blank when we couldn't read one (and for rows recorded before this field). + user_agent = models.TextField(blank=True, default='') # nullable: anonymous downloads of public files user = models.ForeignKey( diff --git a/osf/utils/download_telemetry.py b/osf/utils/download_telemetry.py index 66dc5a3e9a0..f7fa0079070 100644 --- a/osf/utils/download_telemetry.py +++ b/osf/utils/download_telemetry.py @@ -63,6 +63,7 @@ def write_download_event( status_code=None, user_guid=None, ip=None, + user_agent='', source_area='', tz='', ): @@ -99,6 +100,8 @@ def write_download_event( storage_region=_truncate(storage_region, 64), user_region=_truncate(derive_user_region(tz, user, storage_region), 64), ip=ip or None, + # capped: the User-Agent comes off the request, so it's client-controlled + user_agent=_truncate(user_agent, 512), source_area=_truncate(source_area, 128), user=user, ) diff --git a/tests/test_download_events_dashboard.py b/tests/test_download_events_dashboard.py index 2d54c1f6edb..94224a490d7 100644 --- a/tests/test_download_events_dashboard.py +++ b/tests/test_download_events_dashboard.py @@ -3,11 +3,12 @@ from django.apps import apps as global_apps from django.contrib.admin.sites import AdminSite from django.contrib.auth.models import Group, Permission +from django.test import RequestFactory from django.utils import timezone from osf.admin import DASHBOARD_GROUP_NAME, DownloadEventsView from osf.models import DownloadEvent -from osf_tests.factories import AuthUserFactory, ProjectFactory +from osf_tests.factories import AuthUserFactory, ProjectFactory, PreprintFactory from tests.base import OsfTestCase @@ -265,6 +266,16 @@ def test_top_projects_falls_back_to_the_bare_guid(self): assert data['top_projects'][0]['name'] == 'notaguid' + def test_top_projects_resolves_preprint_title(self): + """Preprints aren't nodes (and their guid can be versioned), but the name should + still resolve — ENG-11849.""" + preprint = PreprintFactory(title='A Preprint About Downloads') + make_event(resource_guid=preprint._id, size_bytes=4 * 1024 ** 3) + + data = self.admin.get_dashboard_data(DownloadEvent.objects.all()) + + assert data['top_projects'][0]['name'] == f'A Preprint About Downloads ({preprint._id})' + def test_time_series_buckets_by_type(self): make_event(size_bytes=1024 ** 3) make_event(size_bytes=3 * 1024 ** 3, download_type=DownloadEvent.FOLDER_ZIP) @@ -332,6 +343,65 @@ def test_unique_users_ignores_anonymous(self): assert data['summary']['unique_users'] == 1 +class TestSortableColumns(OsfTestCase): + """Outcome and User are made sortable (ENG-11863, ENG-11864).""" + + def setUp(self): + super().setUp() + self.admin = DownloadEventsView(DownloadEvent, AdminSite()) + self.request = RequestFactory().get('/admin/osf/downloadevent/') + + def test_outcome_column_declares_a_sort_field(self): + assert self.admin.outcome.admin_order_field == '_outcome_rank' + + def test_outcome_rank_orders_completed_cancelled_failed_then_single(self): + completed = make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=True) + cancelled = make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=200) + failed = make_event(download_type=DownloadEvent.PROJECT, zip_completed=False, status_code=404) + single = make_event(download_type=DownloadEvent.FILE) + + ordered = list( + self.admin.get_queryset(self.request).order_by('_outcome_rank').values_list('id', flat=True) + ) + + assert ordered == [completed.id, cancelled.id, failed.id, single.id] + + def test_user_column_sorts_by_username_not_pk(self): + assert self.admin.user_display.admin_order_field == 'user__username' + + def test_user_display_falls_back_for_anonymous(self): + anon = make_event(user=None) + assert self.admin.user_display(anon) == '—' + + def test_user_agent_column_declares_a_sort_field(self): + assert self.admin.user_agent_display.admin_order_field == 'user_agent' + + def test_user_agent_display_shows_short_agents_in_full(self): + event = make_event(user_agent='curl/8.0') + assert self.admin.user_agent_display(event) == 'curl/8.0' + + def test_user_agent_display_truncates_long_agents(self): + event = make_event(user_agent='Mozilla/5.0 ' + 'x' * 200) + shown = self.admin.user_agent_display(event) + assert len(shown) == 80 and shown.endswith('…') + + def test_user_agent_display_falls_back_when_blank(self): + assert self.admin.user_agent_display(make_event(user_agent='')) == '—' + + def test_outcome_annotation_does_not_change_dashboard_numbers(self): + """Production feeds get_queryset() (annotated with _outcome_rank for sorting) into + get_dashboard_data. The annotation must not alter any aggregate — guards against a + stray GROUP BY. Full equality, since the region sort is now deterministic.""" + make_event(download_type=DownloadEvent.FILE, storage_region='Germany', size_bytes=2 * 1024 ** 3) + make_event(download_type=DownloadEvent.FOLDER_ZIP, storage_region='Germany', zip_completed=True, size_bytes=3 * 1024 ** 3) + make_event(download_type=DownloadEvent.PROJECT, storage_region='United States', zip_completed=False, status_code=404, size_bytes=5 * 1024 ** 3) + + annotated = self.admin.get_dashboard_data(self.admin.get_queryset(self.request)) + plain = self.admin.get_dashboard_data(DownloadEvent.objects.all()) + + assert annotated == plain + + class TestStaffAccessMigration(OsfTestCase): """Django's admin rejects anyone without `is_staff` before our gate runs, so the allow-listed users need it to reach the page at all.""" diff --git a/tests/test_download_telemetry.py b/tests/test_download_telemetry.py index ac71b6afe41..74a4e2b1719 100644 --- a/tests/test_download_telemetry.py +++ b/tests/test_download_telemetry.py @@ -108,6 +108,7 @@ def build_payload(self, materialized='/', action='download_zip', **action_meta): 'provider': 'osfstorage', }, 'action_meta': meta, + 'request_meta': {'user_agent': 'TestZipAgent/1.0'}, } message, signature = signing.default_signer.sign_payload(options) return {'payload': message, 'signature': signature} @@ -215,6 +216,29 @@ def test_storage_provider_comes_from_the_callback(self): assert DownloadEvent.objects.get().storage_provider == 'osfstorage' + def test_user_agent_comes_from_the_callback(self): + self.app.put(self.url, json=self.build_payload()) + + assert DownloadEvent.objects.get().user_agent == 'TestZipAgent/1.0' + + def test_missing_request_meta_leaves_user_agent_blank(self): + """A callback from a WaterButler build that predates request_meta records an empty + user agent rather than failing.""" + options = { + 'auth': {'id': self.user._id}, + 'action': 'download_zip', + 'provider': 'osfstorage', + 'time': time.time() + 1000, + 'metadata': {'nid': self.node._id, 'materialized': '/', 'path': '/', + 'kind': 'folder', 'provider': 'osfstorage'}, + 'action_meta': {}, + } + message, signature = signing.default_signer.sign_payload(options) + res = self.app.put(self.url, json={'payload': message, 'signature': signature}) + + assert res.status_code == 200 + assert DownloadEvent.objects.get().user_agent == '' + def test_callback_still_succeeds_when_recording_fails(self, ): with pytest.MonkeyPatch.context() as patch: patch.setattr( @@ -262,6 +286,23 @@ def test_storage_provider_comes_from_the_file(self): assert DownloadEvent.objects.get().storage_provider == 'osfstorage' + def test_user_agent_comes_from_the_request(self): + self.app.get( + f'/download/{self.guid}/', auth=self.user.auth, + headers={'User-Agent': 'PytestClient/9.9'}, + ) + + assert DownloadEvent.objects.get().user_agent == 'PytestClient/9.9' + + def test_long_user_agent_is_capped(self): + """The User-Agent is client-controlled, so the write caps it to the column width.""" + self.app.get( + f'/download/{self.guid}/', auth=self.user.auth, + headers={'User-Agent': 'x' * 900}, + ) + + assert len(DownloadEvent.objects.get().user_agent) == 512 + def test_zip_completed_is_unset_for_single_files(self): """Only zips stream through WaterButler, so nothing reports completion here.""" self.app.get(f'/download/{self.guid}/', auth=self.user.auth)