Merge branch 'feature/15' into 'develop'

Fixed #15: Ensure we check for authorization for serving audio files, meaning we…

Closes #15

See merge request !12

api/compose/nginx/Dockerfile

@@ -1,2 +0,0 @@
-FROM nginx:latest
-ADD nginx.conf /etc/nginx/nginx.conf

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')

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')

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')

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'

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)
+        )

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

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/;
     }
 }

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"

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 /_protected/media {
+            internal;
+            alias   /protected/media;
+        }
         location / {
-            # checks for static file, if not found proxy to app
-            try_files $uri @proxy_to_app;
-        }
-
-        location @proxy_to_app {
+            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/;
         }
-
     }
 }

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
 -------