Merge branch '182-xsendfile-inplace' into 'develop'

Resolve "X-sendfile not working with in place import"

Closes #182

See merge request funkwhale/funkwhale!163

api/funkwhale_api/music/models.py

@@ -463,26 +463,6 @@ class TrackFile(models.Model):
             self.mimetype = utils.guess_mimetype(self.audio_file)
         return super().save(**kwargs)
 
-    @property
-    def serve_from_source_path(self):
-        if not self.source or not self.source.startswith('file://'):
-            raise ValueError('Cannot serve this file from source')
-        serve_path = settings.MUSIC_DIRECTORY_SERVE_PATH
-        prefix = settings.MUSIC_DIRECTORY_PATH
-        if not serve_path or not prefix:
-            raise ValueError(
-                'You need to specify MUSIC_DIRECTORY_SERVE_PATH and '
-                'MUSIC_DIRECTORY_PATH to serve in-place imported files'
-            )
-        file_path = self.source.replace('file://', '', 1)
-        parts = os.path.split(file_path.replace(prefix, '', 1))
-        if parts[0] == '/':
-            parts = parts[1:]
-        return os.path.join(
-            serve_path,
-            *parts
-        )
-
 
 IMPORT_STATUS_CHOICES = (
     ('pending', 'Pending'),

api/funkwhale_api/music/views.py

@@ -206,6 +206,8 @@ class TrackViewSet(
 
 
 def get_file_path(audio_file):
+    serve_path = settings.MUSIC_DIRECTORY_SERVE_PATH
+    prefix = settings.MUSIC_DIRECTORY_PATH
     t = settings.REVERSE_PROXY_TYPE
     if t == 'nginx':
         # we have to use the internal locations
@@ -213,14 +215,24 @@ def get_file_path(audio_file):
             path = audio_file.url
         except AttributeError:
             # a path was given
-            path = '/music' + audio_file
+            if not serve_path or not prefix:
+                raise ValueError(
+                    'You need to specify MUSIC_DIRECTORY_SERVE_PATH and '
+                    'MUSIC_DIRECTORY_PATH to serve in-place imported files'
+                )
+            path = '/music' + audio_file.replace(prefix, '', 1)
         return settings.PROTECT_FILES_PATH + path
     if t == 'apache2':
         try:
             path = audio_file.path
         except AttributeError:
             # a path was given
-            path = audio_file
+            if not serve_path or not prefix:
+                raise ValueError(
+                    'You need to specify MUSIC_DIRECTORY_SERVE_PATH and '
+                    'MUSIC_DIRECTORY_PATH to serve in-place imported files'
+                )
+            path = audio_file.replace(prefix, serve_path, 1)
         return path
 
 
@@ -267,7 +279,7 @@ class TrackFileViewSet(viewsets.ReadOnlyModelViewSet):
         elif audio_file:
             file_path = get_file_path(audio_file)
         elif f.source and f.source.startswith('file://'):
-            file_path = get_file_path(f.serve_from_source_path)
+            file_path = get_file_path(f.source.replace('file://', '', 1))
         response = Response()
         filename = f.filename
         mapping = {

api/tests/music/test_views.py

@@ -76,29 +76,60 @@ def test_can_serve_track_file_as_remote_library_deny_not_following(
     assert response.status_code == 403
 
 
-def test_serve_file_apache(factories, api_client, settings):
+@pytest.mark.parametrize('proxy,serve_path,expected', [
+    ('apache2', '/host/music', '/host/music/hello/world.mp3'),
+    ('apache2', '/app/music', '/app/music/hello/world.mp3'),
+    ('nginx', '/host/music', '/_protected/music/hello/world.mp3'),
+    ('nginx', '/app/music', '/_protected/music/hello/world.mp3'),
+])
+def test_serve_file_in_place(
+        proxy, serve_path, expected, factories, api_client, settings):
+    headers = {
+        'apache2': 'X-Sendfile',
+        'nginx': 'X-Accel-Redirect',
+    }
     settings.PROTECT_AUDIO_FILES = False
-    settings.REVERSE_PROXY_TYPE = 'apache2'
-    tf = factories['music.TrackFile']()
+    settings.PROTECT_FILE_PATH = '/_protected/music'
+    settings.REVERSE_PROXY_TYPE = proxy
+    settings.MUSIC_DIRECTORY_PATH = '/app/music'
+    settings.MUSIC_DIRECTORY_SERVE_PATH = serve_path
+    tf = factories['music.TrackFile'](
+        in_place=True,
+        source='file:///app/music/hello/world.mp3'
+    )
     response = api_client.get(tf.path)
 
     assert response.status_code == 200
-    assert response['X-Sendfile'] == tf.audio_file.path
+    assert response[headers[proxy]] == expected
 
 
-def test_serve_file_apache_in_place(factories, api_client, settings):
+@pytest.mark.parametrize('proxy,serve_path,expected', [
+    ('apache2', '/host/music', '/host/media/tracks/hello/world.mp3'),
+    # apache with container not supported yet
+    # ('apache2', '/app/music', '/app/music/tracks/hello/world.mp3'),
+    ('nginx', '/host/music', '/_protected/media/tracks/hello/world.mp3'),
+    ('nginx', '/app/music', '/_protected/media/tracks/hello/world.mp3'),
+])
+def test_serve_file_media(
+        proxy, serve_path, expected, factories, api_client, settings):
+    headers = {
+        'apache2': 'X-Sendfile',
+        'nginx': 'X-Accel-Redirect',
+    }
     settings.PROTECT_AUDIO_FILES = False
-    settings.REVERSE_PROXY_TYPE = 'apache2'
-    settings.MUSIC_DIRECTORY_PATH = '/music'
-    settings.MUSIC_DIRECTORY_SERVE_PATH = '/host/music'
-    track_file = factories['music.TrackFile'](
-        in_place=True,
-        source='file:///music/test.ogg')
+    settings.MEDIA_ROOT = '/host/media'
+    settings.PROTECT_FILE_PATH = '/_protected/music'
+    settings.REVERSE_PROXY_TYPE = proxy
+    settings.MUSIC_DIRECTORY_PATH = '/app/music'
+    settings.MUSIC_DIRECTORY_SERVE_PATH = serve_path
 
-    response = api_client.get(track_file.path)
+    tf = factories['music.TrackFile']()
+    tf.__class__.objects.filter(pk=tf.pk).update(
+        audio_file='tracks/hello/world.mp3')
+    response = api_client.get(tf.path)
 
     assert response.status_code == 200
-    assert response['X-Sendfile'] == '/host/music/test.ogg'
+    assert response[headers[proxy]] == expected
 
 
 def test_can_proxy_remote_track(
@@ -118,25 +149,6 @@ def test_can_proxy_remote_track(
     assert library_track.audio_file.read() == b'test'
 
 
-def test_can_serve_in_place_imported_file(
-        factories, settings, api_client, r_mock):
-    settings.PROTECT_AUDIO_FILES = False
-    settings.MUSIC_DIRECTORY_SERVE_PATH = '/host/music'
-    settings.MUSIC_DIRECTORY_PATH = '/music'
-    settings.MUSIC_DIRECTORY_PATH = '/music'
-    track_file = factories['music.TrackFile'](
-        in_place=True,
-        source='file:///music/test.ogg')
-
-    response = api_client.get(track_file.path)
-
-    assert response.status_code == 200
-    assert response['X-Accel-Redirect'] == '{}{}'.format(
-        settings.PROTECT_FILES_PATH,
-        '/music/host/music/test.ogg'
-    )
-
-
 def test_can_create_import_from_federation_tracks(
         factories, superuser_api_client, mocker):
     lts = factories['federation.LibraryTrack'].create_batch(size=5)