From 9860bb7a3c3f6b9caf70499bf09d1546d56d80b4 Mon Sep 17 00:00:00 2001 From: Alastair Porter Date: Mon, 13 Jul 2026 21:13:29 +0200 Subject: [PATCH 1/2] Open downloaders modal when clicking collection download count This link should open the modal, not trigger a download --- templates/collections/display_collection.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/collections/display_collection.html b/templates/collections/display_collection.html index d96bbf2fe..c4dc4aacc 100644 --- a/templates/collections/display_collection.html +++ b/templates/collections/display_collection.html @@ -34,7 +34,7 @@
{% if collection.num_downloads %}
- + {{ collection.num_downloads|formatnumber }}
From 8074fe922d149ba7bddc807df09efede1d07d062 Mon Sep 17 00:00:00 2001 From: Alastair Porter Date: Mon, 13 Jul 2026 11:02:42 +0200 Subject: [PATCH 2/2] enforce a daily download limit Count the number of downloads that a user makes over a day. Align all users to UTC for simplicity. Use Redis to store the count. Count Sounds, Packs, Collections, and Bookmark Category downloads as 1 download each time. Once a user goes over the limit, don't show a download link, and prevent the direct download link from providing the file (HTTP 429). Set an "in progress" download key when the user starts a download, and don't count again if they access the URL when the download sentinel is active - this prevents download managers that request different ranges from counting multiple times. Add the same guard to Collection downloads (it was missing) --- .github/workflows/unit-tests.yml | 2 +- bookmarks/views.py | 12 +- docker-compose.test.yml | 4 + freesound/settings.py | 9 + freesound/test_settings.py | 4 + freesound/urls.py | 1 + fscollections/views.py | 25 +- pyproject.toml | 1 + sounds/tests/test_download_limit_views.py | 260 ++++++++++++++++++ sounds/tests/test_sound.py | 15 +- sounds/views.py | 62 +++-- templates/bookmarks/bookmarks.html | 4 + templates/collections/collection.html | 4 + templates/molecules/modal_download_limit.html | 14 + templates/sounds/download_limit_reached.html | 8 + templates/sounds/pack.html | 4 + templates/sounds/sound.html | 4 + utils/download_limit.py | 123 +++++++++ utils/tests/test_download_limit.py | 61 ++++ 19 files changed, 573 insertions(+), 44 deletions(-) create mode 100644 sounds/tests/test_download_limit_views.py create mode 100644 templates/molecules/modal_download_limit.html create mode 100644 templates/sounds/download_limit_reached.html create mode 100644 utils/download_limit.py create mode 100644 utils/tests/test_download_limit.py diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 11446eefd..df1fceb19 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -28,7 +28,7 @@ jobs: continue-on-error: true - name: Pull docker images - run: docker compose -f docker-compose.test.yml pull db + run: docker compose -f docker-compose.test.yml pull db redis - name: Build test_runner image uses: docker/build-push-action@v5 diff --git a/bookmarks/views.py b/bookmarks/views.py index 9724e164a..571402e0f 100755 --- a/bookmarks/views.py +++ b/bookmarks/views.py @@ -30,6 +30,13 @@ from bookmarks.forms import BookmarkCategoryForm, BookmarkForm from bookmarks.models import Bookmark, BookmarkCategory from sounds.models import Sound +from utils.download_limit import ( + DownloadType, + count_download_and_set_sentinel, + download_limit_reached_response, + new_download_blocked, + user_download_limit_reached, +) from utils.downloads import download_sounds from utils.pagination import paginate from utils.username import get_parameter_user_or_404, raise_404_if_user_is_deleted, redirect_if_old_username @@ -59,6 +66,7 @@ def bookmarks(request, category_id=None): page_sounds = Sound.objects.ordered_ids([bookmark.sound_id for bookmark in paginator["page"].object_list]) tvars.update(paginator) tvars["page_bookmarks_and_sound_objects"] = zip(paginator["page"].object_list, page_sounds) + tvars["download_limit_reached"] = user_download_limit_reached(request) return render(request, "bookmarks/bookmarks.html", tvars) @@ -108,9 +116,11 @@ def download_bookmark_category(request, category_id): sounds_list = Sound.objects.filter( id__in=bookmarked_sounds, processing_state="OK", moderation_state="OK" ).select_related("user", "license") + if new_download_blocked(request, DownloadType.BOOKMARK_CATEGORY, category.id): + return download_limit_reached_response(request) + count_download_and_set_sentinel(request, DownloadType.BOOKMARK_CATEGORY, category.id) licenses_url = reverse("category-licenses", args=[category_id]) licenses_content = category.get_attribution(sound_qs=sounds_list) - # NOTE: unlike pack downloads, here we are not doing any cache check to avoid consecutive downloads return download_sounds(licenses_url, licenses_content, sounds_list, category.download_filename) diff --git a/docker-compose.test.yml b/docker-compose.test.yml index 35d2eb1ac..acc48ceba 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -3,6 +3,9 @@ volumes: solr9data: services: + redis: + image: redis:8.4.2 + db: image: postgres:18.3 env_file: @@ -38,6 +41,7 @@ services: - FS_USER_ID depends_on: - db + - redis test_runner_search: image: freesound-test:ci diff --git a/freesound/settings.py b/freesound/settings.py index 8d06892fc..4addf94ed 100644 --- a/freesound/settings.py +++ b/freesound/settings.py @@ -120,6 +120,7 @@ CLUSTERING_CACHE_REDIS_STORE_ID = 1 AUDIO_FEATURES_REDIS_STORE_ID = 2 CELERY_BROKER_REDIS_STORE_ID = 3 +ABUSE_REDIS_STORE_ID = 4 CACHES = { "default": { "BACKEND": "django.core.cache.backends.locmem.LocMemCache", @@ -138,6 +139,10 @@ "BACKEND": "django.core.cache.backends.redis.RedisCache", "LOCATION": f"redis://{REDIS_HOST}:{REDIS_PORT}/{CLUSTERING_CACHE_REDIS_STORE_ID}", }, + "abuse": { + "BACKEND": "django.core.cache.backends.redis.RedisCache", + "LOCATION": f"redis://{REDIS_HOST}:{REDIS_PORT}/{ABUSE_REDIS_STORE_ID}", + }, } CACHE_MIDDLEWARE_SECONDS = 300 @@ -334,6 +339,10 @@ USERFLAG_THRESHOLD_FOR_NOTIFICATION = 3 USERFLAG_THRESHOLD_FOR_AUTOMATIC_BLOCKING = 6 +# Daily download limit +MAX_DOWNLOADS_PER_DAY = 200 +DOWNLOAD_LIMIT_MESSAGE = "You have reached the download limit. Come back tomorrow to download more sounds." + # Supported audio formats # When adding support for a new audio format you have to change the variables below and check: # - mime types in accounts.forms.validate_file_extension diff --git a/freesound/test_settings.py b/freesound/test_settings.py index 6d3eab933..957f1dcc9 100644 --- a/freesound/test_settings.py +++ b/freesound/test_settings.py @@ -27,6 +27,10 @@ "clustering": { "BACKEND": "django.core.cache.backends.locmem.LocMemCache", }, + "abuse": { + "BACKEND": "django.core.cache.backends.redis.RedisCache", + "LOCATION": f"redis://{REDIS_HOST}:{REDIS_PORT}/15", + }, } # django_ratelimit off in tests. diff --git a/freesound/urls.py b/freesound/urls.py index 2780639fb..024aa14f5 100644 --- a/freesound/urls.py +++ b/freesound/urls.py @@ -118,6 +118,7 @@ path("embed/geotags_box/iframe/", geotags.views.embed_iframe, name="embed-geotags"), path("oembed/", sounds.views.oembed, name="oembed-sound"), path("after-download-modal/", sounds.views.after_download_modal, name="after-download-modal"), + path("download-limit-modal/", sounds.views.download_limit_modal, name="download-limit-modal"), path("browse/", sounds.views.sounds, name="sounds"), path("browse/tags/", tags.views.tags, name="tags"), path("browse/tags//", tags.views.multiple_tags_lookup, name="tags"), diff --git a/fscollections/views.py b/fscollections/views.py index 33230e76e..a650d8e26 100644 --- a/fscollections/views.py +++ b/fscollections/views.py @@ -26,6 +26,7 @@ from django.contrib.auth.decorators import login_required from django.contrib.auth.models import User from django.core.paginator import Paginator +from django.db import transaction from django.db.models import Case, IntegerField, Q, Value, When from django.http import Http404, HttpResponse, HttpResponseRedirect, JsonResponse from django.shortcuts import get_object_or_404, render @@ -42,6 +43,13 @@ from fscollections.models import Collection, CollectionDownload, CollectionDownloadSound, CollectionSound from sounds.models import Sound from sounds.views import add_sounds_modal_helper +from utils.download_limit import ( + DownloadType, + count_download_and_set_sentinel, + download_limit_reached_response, + new_download_blocked, + user_download_limit_reached, +) from utils.downloads import download_sounds from utils.pagination import build_paginator_template_context, paginate, read_page @@ -121,6 +129,7 @@ def collection(request, collection): "current_search": search, } tvars.update(pagination) + tvars["download_limit_reached"] = user_download_limit_reached(request) return render(request, "collections/collection.html", tvars) @@ -129,7 +138,10 @@ def collections_for_user(request): user = request.user user_collections = Collection.objects.filter(user=user).order_by("-modified") maintainer_collections = Collection.objects.filter(maintainers__id=user.id).order_by("-modified") - tvars = {"user_collections": user_collections, "maintainer_collections": maintainer_collections} + tvars = { + "user_collections": user_collections, + "maintainer_collections": maintainer_collections, + } # one URL needed to display all collections and one URL to display ONE collection at a time # the collections_for_user can be reused to display ONE collection so give it a thought on full collections display return render(request, "collections/your_collections.html", tvars) @@ -368,17 +380,14 @@ def edit_collection(request, collection): @login_required +@transaction.atomic() @resolve_collection_from_url def download_collection(request, collection): sounds_list = Sound.objects.bulk_sounds_for_collection(collection.id) - if "range" not in request.headers: - """ - Download managers and some browsers use the range header to download files in multiple parts. We have observed - that all clients first make a GET with no range header (to get the file length) and then make multiple other - requests. We ignore all requests that have range header because we assume that a first query has already been - made. Unlike in pack downloads, here we do not guard against multiple consecutive downloads. - """ + if new_download_blocked(request, DownloadType.COLLECTION, collection.id): + return download_limit_reached_response(request) + if count_download_and_set_sentinel(request, DownloadType.COLLECTION, collection.id): cd = CollectionDownload.objects.create(user=request.user, collection=collection) cds = [] for sound in sounds_list: diff --git a/pyproject.toml b/pyproject.toml index af101f9bf..ca8255b10 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -126,6 +126,7 @@ markers = [ "search_engine: solr tests", "forum: search engine forum tests", "sounds: search engine search tests", + "redis: tests that require a real Redis server", ] [tool.ty.src] diff --git a/sounds/tests/test_download_limit_views.py b/sounds/tests/test_download_limit_views.py new file mode 100644 index 000000000..e25df83be --- /dev/null +++ b/sounds/tests/test_download_limit_views.py @@ -0,0 +1,260 @@ +from unittest import mock + +import pytest +from django.contrib.auth.models import Group, User +from django.core.cache import cache, caches +from django.core.management import call_command +from django.http import Http404, HttpResponse +from django.urls import reverse +from django.utils.text import slugify +from pytest_django.asserts import assertContains, assertNotContains + +from bookmarks.models import Bookmark, BookmarkCategory +from fscollections.models import Collection, CollectionDownload, CollectionSound +from sounds.models import Download, PackDownload +from utils.download_limit import get_daily_download_count, increment_daily_download_count +from utils.test_helpers import create_user_and_sounds + +pytestmark = pytest.mark.redis + + +@pytest.fixture +def limit_test_data(db): + """Load users and clear abuse db on each call""" + caches["abuse"]._cache.get_client(write=True).flushdb() + cache.clear() + call_command("loaddata", "licenses", "users") + + +@pytest.fixture +def uploader(limit_test_data): + return User.objects.get(username="User2") + + +@pytest.fixture +def downloader(limit_test_data, client): + user = User.objects.get(username="User1") + client.force_login(user) + return user + + +@pytest.fixture +def moderators_group(db): + Group.objects.get_or_create(name="moderators") + + +@pytest.fixture +def download_sound(uploader): + _, _, sounds = create_user_and_sounds(num_sounds=1, processing_state="OK", moderation_state="OK", user=uploader) + return sounds[0] + + +@pytest.fixture +def download_pack(uploader): + _, packs, _ = create_user_and_sounds( + num_sounds=1, num_packs=1, processing_state="OK", moderation_state="OK", user=uploader + ) + pack = packs[0] + pack.process() + return pack + + +@pytest.fixture +def sound_download_url(download_sound): + return reverse("sound-download", args=[download_sound.user.username, download_sound.id]) + + +@pytest.fixture +def download_collection(downloader, download_sound): + collection = Collection.objects.create(user=downloader, name="limit collection") + CollectionSound.objects.create(user=downloader, sound=download_sound, collection=collection, status="OK") + return collection + + +@pytest.fixture +def bookmark_category(downloader, download_sound): + category = BookmarkCategory.objects.create(user=downloader, name="limit bookmarks") + Bookmark.objects.create(user=downloader, category=category, sound=download_sound) + return category + + +@pytest.fixture +def public_collection(downloader, download_sound): + collection = Collection.objects.create(user=downloader, name="render test collection", public=True) + CollectionSound.objects.create(user=downloader, sound=download_sound, collection=collection, status="OK") + return collection + + +def test_sound_download_under_limit_records_and_increments( + client, settings, download_sound, downloader, sound_download_url, django_capture_on_commit_callbacks +): + settings.MAX_DOWNLOADS_PER_DAY = 1 + with mock.patch("sounds.views.sendfile", return_value=HttpResponse()): + with django_capture_on_commit_callbacks(execute=True): + response = client.get(sound_download_url) + assert response.status_code == 200 + assert Download.objects.filter(user=downloader).count() == 1 + assert get_daily_download_count(downloader.id) == 1 + + +def test_sound_download_over_limit_returns_429_and_no_row( + client, settings, download_sound, downloader, sound_download_url +): + settings.MAX_DOWNLOADS_PER_DAY = 1 + increment_daily_download_count(downloader.id) + response = client.get(sound_download_url) + assert response.status_code == 429 + assert Download.objects.filter(user=downloader).count() == 0 + assert get_daily_download_count(downloader.id) == 1 + + +def test_sound_download_error_does_not_increment( + client, settings, download_sound, downloader, sound_download_url, django_capture_on_commit_callbacks +): + # Simulate a download error - no daily download count and now Download row. + settings.MAX_DOWNLOADS_PER_DAY = 200 + with mock.patch("sounds.views.sendfile", side_effect=Http404): + with django_capture_on_commit_callbacks(execute=True): + response = client.get(sound_download_url) + assert response.status_code == 404 + assert Download.objects.filter(user=downloader).count() == 0 + assert get_daily_download_count(downloader.id) == 0 + assert cache.get("sdwn_%s_%d" % (download_sound.id, downloader.id), None) is None + + +def test_sound_download_over_limit_range_continuation_is_served( + client, settings, download_sound, downloader, sound_download_url +): + # User is over the limit but downloading a sound that has already been started. + settings.MAX_DOWNLOADS_PER_DAY = 1 + increment_daily_download_count(downloader.id) + cache.set("sdwn_%s_%d" % (download_sound.id, downloader.id), True, 60 * 5) + with mock.patch("sounds.views.sendfile", return_value=HttpResponse()): + response = client.get(sound_download_url, HTTP_RANGE="bytes=0-") + assert response.status_code == 200 + + +def test_sound_download_continuation_refreshes_sentinel_without_recounting( + client, settings, download_sound, downloader, sound_download_url, django_capture_on_commit_callbacks +): + # An in-progress download (sentinel present) refreshes its marker on every request and + # does not re-count. + settings.MAX_DOWNLOADS_PER_DAY = 200 + cache.set("sdwn_%s_%d" % (download_sound.id, downloader.id), True, 60 * 5) + with mock.patch("sounds.views.sendfile", return_value=HttpResponse()): + with django_capture_on_commit_callbacks(execute=True) as callbacks: + client.get(sound_download_url, HTTP_RANGE="bytes=100-") + assert len(callbacks) == 1 + assert Download.objects.filter(user=downloader).count() == 0 + assert get_daily_download_count(downloader.id) == 0 + + +def test_pack_download_over_limit_returns_429_and_no_row(client, settings, download_pack, downloader): + settings.MAX_DOWNLOADS_PER_DAY = 1 + increment_daily_download_count(downloader.id) + response = client.get(reverse("pack-download", args=[download_pack.user.username, download_pack.id])) + assert response.status_code == 429 + assert PackDownload.objects.filter(user=downloader).count() == 0 + + +def test_collection_download_over_limit_returns_429_and_no_row(client, settings, download_collection, downloader): + settings.MAX_DOWNLOADS_PER_DAY = 1 + increment_daily_download_count(downloader.id) + url = reverse("download-collection", args=[download_collection.id, slugify(download_collection.name)]) + response = client.get(url) + assert response.status_code == 429 + assert CollectionDownload.objects.filter(user=downloader).count() == 0 + + +def test_repeated_collection_download_records_one_row( + client, settings, download_collection, downloader, django_capture_on_commit_callbacks +): + # Repeated downloads within the sentinel window record a single CollectionDownload row + settings.MAX_DOWNLOADS_PER_DAY = 200 + url = reverse("download-collection", args=[download_collection.id, slugify(download_collection.name)]) + with mock.patch("fscollections.views.download_sounds", return_value=HttpResponse()): + for _ in range(3): + with django_capture_on_commit_callbacks(execute=True): + client.get(url) + assert CollectionDownload.objects.filter(user=downloader).count() == 1 + + +def test_bookmark_category_download_over_limit_returns_429_and_does_not_increment( + client, settings, bookmark_category, downloader +): + settings.MAX_DOWNLOADS_PER_DAY = 1 + increment_daily_download_count(downloader.id) + response = client.get(reverse("download-bookmark-category", args=[bookmark_category.id])) + assert response.status_code == 429 + # Bookmark-category downloads do not have a download-row model, so assert the + # failed over-limit request did not advance the counter. + assert get_daily_download_count(downloader.id) == 1 + + +def test_sound_page_hides_download_href_when_over_limit( + client, settings, download_sound, downloader, sound_download_url, moderators_group +): + settings.MAX_DOWNLOADS_PER_DAY = 1 + url = reverse("sound", args=[download_sound.user.username, download_sound.id]) + + response = client.get(url) + assert not response.context["download_limit_reached"] + assertContains(response, 'href="%s' % sound_download_url) + + increment_daily_download_count(downloader.id) + response = client.get(url) + assert response.context["download_limit_reached"] + assertNotContains(response, 'href="%s' % sound_download_url) + assertContains(response, "download-limit-modal") + + +def test_pack_page_hides_download_href_when_over_limit(client, settings, download_pack, downloader, moderators_group): + settings.MAX_DOWNLOADS_PER_DAY = 1 + url = reverse("pack", args=[download_pack.user.username, download_pack.id]) + download_path = reverse("pack-download", args=[download_pack.user.username, download_pack.id]) + + response = client.get(url) + assert not response.context["download_limit_reached"] + assertContains(response, 'href="%s' % download_path) + + increment_daily_download_count(downloader.id) + response = client.get(url) + assert response.context["download_limit_reached"] + assertNotContains(response, 'href="%s' % download_path) + assertContains(response, "download-limit-modal") + + +def test_collection_page_hides_download_href_when_over_limit(client, settings, public_collection, downloader): + settings.MAX_DOWNLOADS_PER_DAY = 1 + url = reverse("collection", args=[public_collection.id, slugify(public_collection.name)]) + + response = client.get(url) + assert not response.context["download_limit_reached"] + assertContains(response, 'href="%s' % public_collection.download_url) + + increment_daily_download_count(downloader.id) + response = client.get(url) + assert response.context["download_limit_reached"] + assertNotContains(response, 'href="%s' % public_collection.download_url) + assertContains(response, "download-limit-modal") + + +def test_bookmarks_page_hides_download_href_when_over_limit(client, settings, bookmark_category, downloader): + settings.MAX_DOWNLOADS_PER_DAY = 1 + download_path = reverse("download-bookmark-category", args=[bookmark_category.id]) + + response = client.get(reverse("bookmarks")) + assert not response.context["download_limit_reached"] + assertContains(response, 'href="%s' % download_path) + + increment_daily_download_count(downloader.id) + response = client.get(reverse("bookmarks")) + assert response.context["download_limit_reached"] + assertNotContains(response, 'href="%s' % download_path) + assertContains(response, "download-limit-modal") + + +def test_modal_view_returns_limit_content(client, downloader): + response = client.get(reverse("download-limit-modal") + "?ajax=1") + assert response.status_code == 200 + assert b"download limit" in response.content.lower() diff --git a/sounds/tests/test_sound.py b/sounds/tests/test_sound.py index f2e404f94..416facab8 100644 --- a/sounds/tests/test_sound.py +++ b/sounds/tests/test_sound.py @@ -385,14 +385,16 @@ def test_download_sound(self): # Check download works successfully if user logged in self.client.force_login(self.user) - resp = self.client.get(reverse("sound-download", args=[self.sound.user.username, self.sound.id])) + with self.captureOnCommitCallbacks(execute=True): + resp = self.client.get(reverse("sound-download", args=[self.sound.user.username, self.sound.id])) self.assertEqual(resp.status_code, 200) # Check n download objects is 1 self.assertEqual(Download.objects.filter(user=self.user, sound=self.sound).count(), 1) # Download again and check n download objects is still 1 - self.client.get(reverse("sound-download", args=[self.sound.user.username, self.sound.id])) + with self.captureOnCommitCallbacks(execute=True): + self.client.get(reverse("sound-download", args=[self.sound.user.username, self.sound.id])) self.assertEqual(Download.objects.filter(user=self.user, sound=self.sound).count(), 1) # Check num_download attribute of Sound is 1 @@ -475,7 +477,8 @@ def test_download_pack(self): # Check download works successfully if user logged in self.client.force_login(self.user) - resp = self.client.get(reverse("pack-download", args=[self.sound.user.username, self.pack.id])) + with self.captureOnCommitCallbacks(execute=True): + resp = self.client.get(reverse("pack-download", args=[self.sound.user.username, self.pack.id])) self.assertEqual(resp.status_code, 200) # Check n download objects is 1 @@ -487,8 +490,10 @@ def test_download_pack(self): 1, ) - # Download again and check n download objects is still 1 - self.client.get(reverse("pack-download", args=[self.sound.user.username, self.pack.id])) + # Download again and check n download objects is still 1 (deduplicated via the + # in-progress sentinel, which is written on transaction commit) + with self.captureOnCommitCallbacks(execute=True): + self.client.get(reverse("pack-download", args=[self.sound.user.username, self.pack.id])) self.assertEqual(PackDownload.objects.filter(user=self.user, pack=self.pack).count(), 1) # Check num_download attribute of Sound is 1 diff --git a/sounds/views.py b/sounds/views.py index e251cb492..562e57338 100644 --- a/sounds/views.py +++ b/sounds/views.py @@ -63,6 +63,13 @@ from tickets.models import Ticket, TicketComment from utils.cache import invalidate_user_template_caches from utils.cdn import generate_cdn_download_url +from utils.download_limit import ( + DownloadType, + count_download_and_set_sentinel, + download_limit_reached_response, + new_download_blocked, + user_download_limit_reached, +) from utils.downloads import download_sounds, should_suggest_donation from utils.mail import send_mail_template, send_mail_template_to_support from utils.nginxsendfile import prepare_sendfile_arguments_for_sound_download, sendfile @@ -294,6 +301,7 @@ def sound(request, username, sound_id): "is_explicit": is_explicit, # if the sound should be shown blurred, already checks for adult profile "sizes": settings.IFRAME_PLAYER_SIZE, "min_num_ratings": settings.MIN_NUMBER_RATINGS, + "download_limit_reached": user_download_limit_reached(request), } tvars.update(paginate(request, qs, settings.SOUND_COMMENTS_PER_PAGE)) return render(request, "sounds/sound.html", tvars) @@ -338,6 +346,12 @@ def modal_shown_timestamps_cache_key(user): return HttpResponse() +@login_required +def download_limit_modal(request): + """modal shown when a user reaches the daily download limit.""" + return render(request, "molecules/modal_download_limit.html", {"message": settings.DOWNLOAD_LIMIT_MESSAGE}) + + @redirect_if_old_username @transaction.atomic() def sound_download(request, username, sound_id): @@ -347,19 +361,14 @@ def sound_download(request, username, sound_id): if sound.user.username.lower() != username.lower(): raise Http404 - if "range" not in request.headers: - """ - Download managers and some browsers use the range header to download files in multiple parts. We have observed - that all clients first make a GET with no range header (to get the file length) and then make multiple other - requests. We ignore all requests that have range header because we assume that a first query has already been - made. We additionally guard against users clicking on download multiple times by storing a sentinel in the - cache for 5 minutes. - """ - cache_key = "sdwn_%s_%d" % (sound_id, request.user.id) - if cache.get(cache_key, None) is None: - Download.objects.create(user=request.user, sound=sound, license_id=sound.license_id) - sound.invalidate_template_caches() - cache.set(cache_key, True, 60 * 5) # Don't save downloads for the same user/sound in 5 minutes + if new_download_blocked(request, DownloadType.SOUND, sound_id): + return download_limit_reached_response(request) + if count_download_and_set_sentinel(request, DownloadType.SOUND, sound_id): + # True only for a new download, not a continuation of one already started + # (e.g. a multi-part Range request, or a repeated click within the window), + # so we record only one Download row per download. + Download.objects.create(user=request.user, sound=sound, license_id=sound.license_id) + sound.invalidate_template_caches() if settings.USE_CDN_FOR_DOWNLOADS: cdn_url = generate_cdn_download_url(sound) @@ -378,22 +387,16 @@ def pack_download(request, username, pack_id): if pack.user.username.lower() != username.lower(): raise Http404 - if "range" not in request.headers: - """ - Download managers and some browsers use the range header to download files in multiple parts. We have observed - that all clients first make a GET with no range header (to get the file length) and then make multiple other - requests. We ignore all requests that have range header because we assume that a first query has already been - made. We additionally guard against users clicking on download multiple times by storing a sentinel in the - cache for 5 minutes. - """ - cache_key = "pdwn_%s_%d" % (pack_id, request.user.id) - if cache.get(cache_key, None) is None: - pd = PackDownload.objects.create(user=request.user, pack=pack) - pds = [] - for sound in pack.sounds.all(): - pds.append(PackDownloadSound(sound=sound, license_id=sound.license_id, pack_download=pd)) - PackDownloadSound.objects.bulk_create(pds) - cache.set(cache_key, True, 60 * 5) # Don't save downloads for the same user/pack in the next 5 minutes + if new_download_blocked(request, DownloadType.PACK, pack_id): + return download_limit_reached_response(request) + if count_download_and_set_sentinel(request, DownloadType.PACK, pack_id): + # Create a PackDownload only if it's a new download, not if it's a continuation + # of an existing download (Range request, or double-click) + pd = PackDownload.objects.create(user=request.user, pack=pack) + pds = [] + for sound in pack.sounds.all(): + pds.append(PackDownloadSound(sound=sound, license_id=sound.license_id, pack_download=pd)) + PackDownloadSound.objects.bulk_create(pds) sounds_list = pack.sounds.filter(processing_state="OK", moderation_state="OK").select_related("user", "license") licenses_url = reverse("pack-licenses", args=[username, pack_id]) @@ -1001,6 +1004,7 @@ def pack(request, username, pack_id): "pack_sounds": pack_sounds, "is_following": is_following, "geotags_in_pack_serialized": geotags_in_pack_serialized, + "download_limit_reached": user_download_limit_reached(request), } return render(request, "sounds/pack.html", tvars) diff --git a/templates/bookmarks/bookmarks.html b/templates/bookmarks/bookmarks.html index 188a42964..4068c957f 100644 --- a/templates/bookmarks/bookmarks.html +++ b/templates/bookmarks/bookmarks.html @@ -21,7 +21,11 @@

Bookmark categories

{% if is_owner %} {% bw_icon 'trash' %} {% bw_icon 'edit' %} + {% if download_limit_reached %} + {% bw_icon 'download' %} + {% else %} {% bw_icon 'download' %} + {% endif %} {% endif %} {% endfor %} diff --git a/templates/collections/collection.html b/templates/collections/collection.html index 9f2a9e814..6eafff7c7 100644 --- a/templates/collections/collection.html +++ b/templates/collections/collection.html @@ -164,7 +164,11 @@
No sounds... 😟
{% if collection.num_sounds > 0 %}
+ {% if download_limit_reached %} + Download collection + {% else %} Download collection + {% endif %}
{% endif %} {% if is_owner or is_maintainer %} diff --git a/templates/molecules/modal_download_limit.html b/templates/molecules/modal_download_limit.html new file mode 100644 index 000000000..fa8ebbc5c --- /dev/null +++ b/templates/molecules/modal_download_limit.html @@ -0,0 +1,14 @@ +{% extends "molecules/modal_base.html" %} + +{% block id %}downloadLimitModal{% endblock %} +{% block extra-class %}modal-width-60{% endblock %} +{% block aria-label %}Download limit reached{% endblock %} + +{% block body %} +
+

Download limit reached

+
+ {{ message }} +
+
+{% endblock %} diff --git a/templates/sounds/download_limit_reached.html b/templates/sounds/download_limit_reached.html new file mode 100644 index 000000000..5ec59fc93 --- /dev/null +++ b/templates/sounds/download_limit_reached.html @@ -0,0 +1,8 @@ +{% extends "simple_page.html" %} + +{% block title %}Download limit reached{% endblock %} +{% block page-title %}Download limit reached{% endblock %} + +{% block page-content %} +

{{ message }}

+{% endblock %} diff --git a/templates/sounds/pack.html b/templates/sounds/pack.html index 5fe7f4e22..b98001a34 100644 --- a/templates/sounds/pack.html +++ b/templates/sounds/pack.html @@ -90,7 +90,11 @@
No sounds... 😟
{% if pack.num_sounds %}
{% if request.user.is_authenticated %} + {% if download_limit_reached %} + Download pack + {% else %} Download pack + {% endif %} {% else %} Login to download {% endif %} diff --git a/templates/sounds/sound.html b/templates/sounds/sound.html index f44540160..1fb5d0b09 100644 --- a/templates/sounds/sound.html +++ b/templates/sounds/sound.html @@ -287,7 +287,11 @@
Comments
{% if sound.moderation_state == 'OK' and sound.processing_state == 'OK' %} {% if request.user.is_authenticated %} + {% if download_limit_reached %} + Download sound + {% else %} Download sound + {% endif %} {% else %} Login to download {% endif %} diff --git a/utils/download_limit.py b/utils/download_limit.py new file mode 100644 index 000000000..18bed2e30 --- /dev/null +++ b/utils/download_limit.py @@ -0,0 +1,123 @@ +import datetime +import enum +import logging + +from django.conf import settings +from django.core.cache import cache, caches +from django.db import transaction +from django.http import HttpRequest, HttpResponse +from django.shortcuts import render + +logger = logging.getLogger("web") + +DOWNLOAD_LIMIT_CACHE_ALIAS = "abuse" +DOWNLOAD_LIMIT_KEY_PREFIX = "downloadlimit" +DOWNLOAD_LIMIT_KEY_TTL = 60 * 60 * 48 # 48 hours + +# TTL of the per-download "in progress" sentinel (see count_download_and_set_sentinel). Long enough to +# span a single (possibly multi-part / slow) download; short enough that it doesn't persist +# between genuinely separate downloads of the same object. +DOWNLOAD_LIMIT_SENTINEL_TTL = 60 * 5 # 5 minutes + + +def _get_redis_client(): + # Native redis-py client from the cache backend. We use the raw client so a single + # pipelined INCR+EXPIRE creates-and-bumps in one round-trip (the backend's incr() + # would raise on a missing key). + return caches[DOWNLOAD_LIMIT_CACHE_ALIAS]._cache.get_client(write=True) + + +def _daily_key(user_id): + today = datetime.datetime.now(datetime.timezone.utc).date() + return f"{DOWNLOAD_LIMIT_KEY_PREFIX}:{user_id}:{today:%Y%m%d}" + + +def get_daily_download_count(user_id): + try: + value = _get_redis_client().get(_daily_key(user_id)) + return int(value) if value is not None else 0 + except Exception: # noqa + # Intentionally fail open on redis error + logger.warning("Could not read daily download count for user %s", user_id, exc_info=True) + return 0 + + +def increment_daily_download_count(user_id): + """Increment the number of downloads for a user. Use the underlying redis client in order + to increment in 1 request (creates and sets to 1 if it doesn't yet exist) and set a 2 day + expiry.""" + try: + client = _get_redis_client() + key = _daily_key(user_id) + pipe = client.pipeline() + pipe.incr(key) + pipe.expire(key, DOWNLOAD_LIMIT_KEY_TTL) + count, _ = pipe.execute() + return int(count) + except Exception: # noqa + # Intentionally fail open on redis error + logger.warning("Could not increment daily download count for user %s", user_id, exc_info=True) + return 0 + + +def download_limit_reached(user_id): + return get_daily_download_count(user_id) >= settings.MAX_DOWNLOADS_PER_DAY + + +def user_download_limit_reached(request): + """Whether the request's user is over the daily download limit.""" + return request.user.is_authenticated and download_limit_reached(request.user.id) + + +class DownloadType(enum.Enum): + """Something that can be downloaded; values are used in the redis download sentinel check.""" + + SOUND = "sdwn" + PACK = "pdwn" + BOOKMARK_CATEGORY = "bdwn" + COLLECTION = "cdwn" + + +def _sentinel_key(download_type: DownloadType, object_id: int, user_id: int) -> str: + return f"{download_type.value}_{object_id}_{user_id}" + + +def download_limit_reached_response(request: HttpRequest) -> HttpResponse: + """The 429 page returned when ``new_download_blocked`` says a download must not start.""" + return render( + request, "sounds/download_limit_reached.html", {"message": settings.DOWNLOAD_LIMIT_MESSAGE}, status=429 + ) + + +def new_download_blocked(request: HttpRequest, download_type: DownloadType, object_id: int) -> bool: + """Check if the user is allowed to download this item. + + The download is blocked if it's a new download and if the user is over their daily limit. + see ``count_download_and_set_sentinel`` for a description of what new download means. + """ + is_new_download = cache.get(_sentinel_key(download_type, object_id, request.user.id), None) is None + return is_new_download and download_limit_reached(request.user.id) + + +def count_download_and_set_sentinel(request: HttpRequest, download_type: DownloadType, object_id: int) -> bool: + """Count this download towards the user's daily limit and mark it as in progress. + + We mark a 5 minute sentinel when a user downloads something for the first time. This guards + us from double-counting downloads if the user's browser or download manager makes multiple + requests to download a single file. + We've seen that sometimes a client makes a request to get the content-length and then makes + another request with a Range header. In this case we don't want to save multiple + Download rows to the database or count this download multiple times in our abuse counter. + + Increments the daily download count if this is a new download and increments the + "existing download" sentinel key every time this is called, new or not. + + Returns True if this is a new download, False otherwise. + """ + user_id = request.user.id + sentinel_key = _sentinel_key(download_type, object_id, user_id) + is_new_download = cache.get(sentinel_key, None) is None + if is_new_download: + transaction.on_commit(lambda: increment_daily_download_count(user_id)) + transaction.on_commit(lambda: cache.set(sentinel_key, True, DOWNLOAD_LIMIT_SENTINEL_TTL)) + return is_new_download diff --git a/utils/tests/test_download_limit.py b/utils/tests/test_download_limit.py new file mode 100644 index 000000000..ca088d8e5 --- /dev/null +++ b/utils/tests/test_download_limit.py @@ -0,0 +1,61 @@ +import pytest +from django.core.cache import caches + +from utils.download_limit import ( + download_limit_reached, + get_daily_download_count, + increment_daily_download_count, +) + +pytestmark = pytest.mark.redis + + +@pytest.fixture +def abuse_redis(): + """The abuse cache's raw redis client, flushed so counters start from a clean slate.""" + client = caches["abuse"]._cache.get_client(write=True) + client.flushdb() + return client + + +def test_count_starts_at_zero(abuse_redis): + assert get_daily_download_count(123) == 0 + + +def test_increment_counts_up_and_is_readable(abuse_redis): + assert increment_daily_download_count(123) == 1 + assert increment_daily_download_count(123) == 2 + assert get_daily_download_count(123) == 2 + + +def test_counts_are_per_user(abuse_redis): + increment_daily_download_count(123) + assert get_daily_download_count(456) == 0 + + +def test_increment_sets_ttl(abuse_redis): + increment_daily_download_count(123) + key = next(iter(abuse_redis.scan_iter("downloadlimit:123:*"))) + assert abuse_redis.ttl(key) > 0 + + +def test_limit_reached_at_threshold(abuse_redis, settings): + settings.MAX_DOWNLOADS_PER_DAY = 2 + increment_daily_download_count(123) + assert not download_limit_reached(123) + increment_daily_download_count(123) + assert download_limit_reached(123) + + +def test_fails_open_when_redis_unavailable(settings): + settings.CACHES = { + **settings.CACHES, + "abuse": { + "BACKEND": "django.core.cache.backends.redis.RedisCache", + # Invalid location, will cause a ConnectionError + "LOCATION": "redis://localhost:1/0", + }, + } + assert get_daily_download_count(123) == 0 + assert increment_daily_download_count(123) == 0 + assert not download_limit_reached(123)