From bab3981d253ed58bce8757df23b15a638158d6f9 Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Wed, 28 Jun 2017 23:30:26 +0200
Subject: [PATCH] Fixed #15: Ensure we check for authorization for serving
audio files, meaning we don't leak the absolute URL anymore
---
api/compose/nginx/Dockerfile | 2 -
api/config/api_urls.py | 1 +
api/config/settings/common.py | 12 +++++-
api/funkwhale_api/music/models.py | 7 ++--
api/funkwhale_api/music/tests/factories.py | 39 +++++++++++++++++++
api/funkwhale_api/music/tests/test_api.py | 25 ++++++++++++
api/funkwhale_api/music/views.py | 25 ++++++++++++
deploy/nginx.conf | 10 +++++
dev.yml | 20 +++++-----
.../nginx/nginx.conf => docker/nginx/conf.dev | 22 ++++-------
docs/changelog.rst | 14 +++++++
11 files changed, 147 insertions(+), 30 deletions(-)
delete mode 100644 api/compose/nginx/Dockerfile
create mode 100644 api/funkwhale_api/music/tests/factories.py
rename api/compose/nginx/nginx.conf => docker/nginx/conf.dev (72%)
diff --git a/api/compose/nginx/Dockerfile b/api/compose/nginx/Dockerfile
deleted file mode 100644
index 196395763a..0000000000
--- a/api/compose/nginx/Dockerfile
+++ /dev/null
@@ -1,2 +0,0 @@
-FROM nginx:latest
-ADD nginx.conf /etc/nginx/nginx.conf
\ No newline at end of file
diff --git a/api/config/api_urls.py b/api/config/api_urls.py
index b56944d4ee..9c612c94d9 100644
--- a/api/config/api_urls.py
+++ b/api/config/api_urls.py
@@ -8,6 +8,7 @@ from rest_framework_jwt import views as jwt_views
router = routers.SimpleRouter()
router.register(r'tags', views.TagViewSet, 'tags')
router.register(r'tracks', views.TrackViewSet, 'tracks')
+router.register(r'trackfiles', views.TrackFileViewSet, 'trackfiles')
router.register(r'artists', views.ArtistViewSet, 'artists')
router.register(r'albums', views.AlbumViewSet, 'albums')
router.register(r'import-batches', views.ImportBatchViewSet, 'import-batches')
diff --git a/api/config/settings/common.py b/api/config/settings/common.py
index 93381c4f53..5ba1971453 100644
--- a/api/config/settings/common.py
+++ b/api/config/settings/common.py
@@ -217,7 +217,6 @@ STATICFILES_FINDERS = (
# See: https://docs.djangoproject.com/en/dev/ref/settings/#media-root
MEDIA_ROOT = str(APPS_DIR('media'))
-USE_SAMPLE_TRACK = env.bool("USE_SAMPLE_TRACK", False)
# See: https://docs.djangoproject.com/en/dev/ref/settings/#media-url
@@ -261,7 +260,6 @@ BROKER_URL = env("CELERY_BROKER_URL", default='django://')
# Location of root django.contrib.admin URL, use {% url 'admin:index' %}
ADMIN_URL = r'^admin/'
-SESSION_SAVE_EVERY_REQUEST = True
# Your common stuff: Below this line define 3rd party library settings
CELERY_DEFAULT_RATE_LIMIT = 1
CELERYD_TASK_TIME_LIMIT = 300
@@ -305,3 +303,13 @@ FUNKWHALE_PROVIDERS = {
}
}
ATOMIC_REQUESTS = False
+
+# Wether we should check user permission before serving audio files (meaning
+# return an obfuscated url)
+# This require a special configuration on the reverse proxy side
+# See https://wellfire.co/learn/nginx-django-x-accel-redirects/ for example
+PROTECT_AUDIO_FILES = env.bool('PROTECT_AUDIO_FILES', default=True)
+
+# Which path will be used to process the internal redirection
+# **DO NOT** put a slash at the end
+PROTECT_FILES_PATH = env('PROTECT_FILES_PATH', default='/_protected')
diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py
index 7023084770..6a55dfc00c 100644
--- a/api/funkwhale_api/music/models.py
+++ b/api/funkwhale_api/music/models.py
@@ -8,7 +8,6 @@ import markdown
from django.conf import settings
from django.db import models
-from django.contrib.staticfiles.templatetags.staticfiles import static
from django.core.files.base import ContentFile
from django.core.files import File
from django.core.urlresolvers import reverse
@@ -354,10 +353,12 @@ class TrackFile(models.Model):
@property
def path(self):
- if settings.USE_SAMPLE_TRACK:
- return static('music/sample1.ogg')
+ if settings.PROTECT_AUDIO_FILES:
+ return reverse(
+ 'api:v1:trackfiles-serve', kwargs={'pk': self.pk})
return self.audio_file.url
+
class ImportBatch(models.Model):
creation_date = models.DateTimeField(default=timezone.now)
submitted_by = models.ForeignKey('users.User', related_name='imports')
diff --git a/api/funkwhale_api/music/tests/factories.py b/api/funkwhale_api/music/tests/factories.py
new file mode 100644
index 0000000000..dfa7a75e28
--- /dev/null
+++ b/api/funkwhale_api/music/tests/factories.py
@@ -0,0 +1,39 @@
+import factory
+
+
+class ArtistFactory(factory.django.DjangoModelFactory):
+ name = factory.Sequence(lambda n: 'artist-{0}'.format(n))
+ mbid = factory.Faker('uuid4')
+
+ class Meta:
+ model = 'music.Artist'
+
+
+class AlbumFactory(factory.django.DjangoModelFactory):
+ title = factory.Sequence(lambda n: 'album-{0}'.format(n))
+ mbid = factory.Faker('uuid4')
+ release_date = factory.Faker('date')
+ cover = factory.django.ImageField()
+ artist = factory.SubFactory(ArtistFactory)
+
+ class Meta:
+ model = 'music.Album'
+
+
+class TrackFactory(factory.django.DjangoModelFactory):
+ title = factory.Sequence(lambda n: 'track-{0}'.format(n))
+ mbid = factory.Faker('uuid4')
+ album = factory.SubFactory(AlbumFactory)
+ artist = factory.SelfAttribute('album.artist')
+ position = 1
+
+ class Meta:
+ model = 'music.Track'
+
+
+class TrackFileFactory(factory.django.DjangoModelFactory):
+ track = factory.SubFactory(TrackFactory)
+ audio_file = factory.django.FileField()
+
+ class Meta:
+ model = 'music.TrackFile'
diff --git a/api/funkwhale_api/music/tests/test_api.py b/api/funkwhale_api/music/tests/test_api.py
index d8f56eeb92..21a567084e 100644
--- a/api/funkwhale_api/music/tests/test_api.py
+++ b/api/funkwhale_api/music/tests/test_api.py
@@ -10,6 +10,8 @@ from funkwhale_api.music import serializers
from funkwhale_api.users.models import User
from . import data as api_data
+from . import factories
+
class TestAPI(TMPDirTestCaseMixin, TestCase):
@@ -214,3 +216,26 @@ class TestAPI(TMPDirTestCaseMixin, TestCase):
with self.settings(API_AUTHENTICATION_REQUIRED=False):
response = getattr(self.client, method)(url)
self.assertEqual(response.status_code, 200)
+
+ def test_track_file_url_is_restricted_to_authenticated_users(self):
+ f = factories.TrackFileFactory()
+ self.assertNotEqual(f.audio_file, None)
+ url = f.path
+
+ with self.settings(API_AUTHENTICATION_REQUIRED=True):
+ response = self.client.get(url)
+
+ self.assertEqual(response.status_code, 401)
+
+ user = User.objects.create_superuser(
+ username='test', email='test@test.com', password='test')
+ self.client.login(username=user.username, password='test')
+ with self.settings(API_AUTHENTICATION_REQUIRED=True):
+ response = self.client.get(url)
+
+ self.assertEqual(response.status_code, 200)
+
+ self.assertEqual(
+ response['X-Accel-Redirect'],
+ '/_protected{}'.format(f.audio_file.url)
+ )
diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py
index 772f4173ea..506db12392 100644
--- a/api/funkwhale_api/music/views.py
+++ b/api/funkwhale_api/music/views.py
@@ -3,6 +3,7 @@ import json
from django.core.urlresolvers import reverse
from django.db import models, transaction
from django.db.models.functions import Length
+from django.conf import settings
from rest_framework import viewsets, views
from rest_framework.decorators import detail_route, list_route
from rest_framework.response import Response
@@ -51,6 +52,7 @@ class ArtistViewSet(SearchMixin, viewsets.ReadOnlyModelViewSet):
search_fields = ['name']
ordering_fields = ('creation_date',)
+
class AlbumViewSet(SearchMixin, viewsets.ReadOnlyModelViewSet):
queryset = (
models.Album.objects.all()
@@ -63,6 +65,7 @@ class AlbumViewSet(SearchMixin, viewsets.ReadOnlyModelViewSet):
search_fields = ['title']
ordering_fields = ('creation_date',)
+
class ImportBatchViewSet(viewsets.ReadOnlyModelViewSet):
queryset = models.ImportBatch.objects.all().order_by('-creation_date')
serializer_class = serializers.ImportBatchSerializer
@@ -70,6 +73,7 @@ class ImportBatchViewSet(viewsets.ReadOnlyModelViewSet):
def get_queryset(self):
return super().get_queryset().filter(submitted_by=self.request.user)
+
class TrackViewSet(TagViewSetMixin, SearchMixin, viewsets.ReadOnlyModelViewSet):
"""
A simple ViewSet for viewing and editing accounts.
@@ -120,6 +124,27 @@ class TrackViewSet(TagViewSetMixin, SearchMixin, viewsets.ReadOnlyModelViewSet):
return Response(serializer.data)
+class TrackFileViewSet(viewsets.ReadOnlyModelViewSet):
+ queryset = (models.TrackFile.objects.all().order_by('-id'))
+ serializer_class = serializers.TrackFileSerializer
+ permission_classes = [ConditionalAuthentication]
+
+ @detail_route(methods=['get'])
+ def serve(self, request, *args, **kwargs):
+ try:
+ f = models.TrackFile.objects.get(pk=kwargs['pk'])
+ except models.TrackFile.DoesNotExist:
+ return Response(status=404)
+
+ response = Response()
+ response["Content-Disposition"] = "attachment; filename={0}".format(
+ f.audio_file.name)
+ response['X-Accel-Redirect'] = "{}{}".format(
+ settings.PROTECT_FILES_PATH,
+ f.audio_file.url)
+ return response
+
+
class TagViewSet(viewsets.ReadOnlyModelViewSet):
queryset = Tag.objects.all().order_by('name')
serializer_class = serializers.TagSerializer
diff --git a/deploy/nginx.conf b/deploy/nginx.conf
index 7395e37d90..6a0a9f5093 100644
--- a/deploy/nginx.conf
+++ b/deploy/nginx.conf
@@ -47,7 +47,17 @@ server {
location /media/ {
alias /srv/funkwhale/data/media/;
}
+
+ location /_protected/media {
+ # this is an internal location that is used to serve
+ # audio files once correct permission / authentication
+ # has been checked on API side
+ internal;
+ alias /srv/funkwhale/data/media;
+ }
+
location /staticfiles/ {
+ # django static files
alias /srv/funkwhale/data/static/;
}
}
diff --git a/dev.yml b/dev.yml
index 21b0912e39..7122884920 100644
--- a/dev.yml
+++ b/dev.yml
@@ -53,12 +53,14 @@ services:
- redis
- celeryworker
- # nginx:
- # env_file: .env.dev
- # build: ./api/compose/nginx
- # links:
- # - api
- # volumes:
- # - ./api/funkwhale_api/media:/staticfiles/media
- # ports:
- # - "0.0.0.0:6001:80"
+ nginx:
+ env_file: .env.dev
+ image: nginx
+ links:
+ - api
+ - front
+ volumes:
+ - ./docker/nginx/conf.dev:/etc/nginx/nginx.conf
+ - ./api/funkwhale_api/media:/protected/media
+ ports:
+ - "0.0.0.0:6001:80"
diff --git a/api/compose/nginx/nginx.conf b/docker/nginx/conf.dev
similarity index 72%
rename from api/compose/nginx/nginx.conf
rename to docker/nginx/conf.dev
index 331d8d45f3..6ca395fb1a 100644
--- a/api/compose/nginx/nginx.conf
+++ b/docker/nginx/conf.dev
@@ -27,27 +27,21 @@ http {
#gzip on;
- upstream app {
- server django:12081;
- }
-
server {
listen 80;
charset utf-8;
- root /staticfiles;
- location / {
- # checks for static file, if not found proxy to app
- try_files $uri @proxy_to_app;
+ location /_protected/media {
+ internal;
+ alias /protected/media;
}
-
- location @proxy_to_app {
+ location / {
+ proxy_set_header Host $host;
+ proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
- proxy_set_header Host $http_host;
+ proxy_set_header X-Forwarded-Proto $scheme;
proxy_redirect off;
-
- proxy_pass http://app;
+ proxy_pass http://api:12081/;
}
-
}
}
diff --git a/docs/changelog.rst b/docs/changelog.rst
index c4092dc8b5..ced5c0a7ea 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -1,6 +1,20 @@
Changelog
=========
+next
+-------
+
+* [breaking] we now check for user permission before serving audio files, which requires
+a specific configuration block in your reverse proxy configuration:
+
+.. code-block::
+
+ location /_protected/media {
+ internal;
+ alias /srv/funkwhale/data/media;
+ }
+
+
0.1
-------
--
GitLab