From fc48e16e6519184596cffe79b597181e2d2666d3 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 May 2018 18:45:31 +0200 Subject: [PATCH 01/57] Fix #218: Ensure inactive users cannot get auth tokens --- api/tests/subsonic/test_authentication.py | 19 +++++++++ api/tests/users/test_views.py | 48 ++++++++++++++--------- changes/changelog.d/218.bugfix | 2 + 3 files changed, 50 insertions(+), 19 deletions(-) create mode 100644 changes/changelog.d/218.bugfix diff --git a/api/tests/subsonic/test_authentication.py b/api/tests/subsonic/test_authentication.py index 724513523..656f8c44d 100644 --- a/api/tests/subsonic/test_authentication.py +++ b/api/tests/subsonic/test_authentication.py @@ -1,4 +1,7 @@ import binascii +import pytest + +from rest_framework import exceptions from funkwhale_api.subsonic import authentication @@ -54,3 +57,19 @@ def test_auth_with_password_cleartext(api_request, factories): u, _ = authenticator.authenticate(request) assert user == u + + +def test_auth_with_inactive_users(api_request, factories): + salt = 'salt' + user = factories['users.User'](is_active=False) + user.subsonic_api_token = 'password' + user.save() + token = authentication.get_token(salt, 'password') + request = api_request.get('/', { + 'u': user.username, + 'p': 'password', + }) + + authenticator = authentication.SubsonicAuthentication() + with pytest.raises(exceptions.AuthenticationFailed): + authenticator.authenticate(request) diff --git a/api/tests/users/test_views.py b/api/tests/users/test_views.py index 1bbf8b9a2..6418889ce 100644 --- a/api/tests/users/test_views.py +++ b/api/tests/users/test_views.py @@ -7,7 +7,7 @@ from django.urls import reverse from funkwhale_api.users.models import User -def test_can_create_user_via_api(preferences, client, db): +def test_can_create_user_via_api(preferences, api_client, db): url = reverse('rest_register') data = { 'username': 'test1', @@ -16,14 +16,14 @@ def test_can_create_user_via_api(preferences, client, db): 'password2': 'testtest', } preferences['users__registration_enabled'] = True - response = client.post(url, data) + response = api_client.post(url, data) assert response.status_code == 201 u = User.objects.get(email='test1@test.com') assert u.username == 'test1' -def test_can_restrict_usernames(settings, preferences, db, client): +def test_can_restrict_usernames(settings, preferences, db, api_client): url = reverse('rest_register') preferences['users__registration_enabled'] = True settings.USERNAME_BLACKLIST = ['funkwhale'] @@ -34,13 +34,13 @@ def test_can_restrict_usernames(settings, preferences, db, client): 'password2': 'testtest', } - response = client.post(url, data) + response = api_client.post(url, data) assert response.status_code == 400 assert 'username' in response.data -def test_can_disable_registration_view(preferences, client, db): +def test_can_disable_registration_view(preferences, api_client, db): url = reverse('rest_register') data = { 'username': 'test1', @@ -49,7 +49,7 @@ def test_can_disable_registration_view(preferences, client, db): 'password2': 'testtest', } preferences['users__registration_enabled'] = False - response = client.post(url, data) + response = api_client.post(url, data) assert response.status_code == 403 @@ -73,7 +73,7 @@ def test_can_fetch_data_from_api(api_client, factories): assert response.data['permissions'] == user.get_permissions() -def test_can_get_token_via_api(client, factories): +def test_can_get_token_via_api(api_client, factories): user = factories['users.User']() url = reverse('api:v1:token') payload = { @@ -81,12 +81,24 @@ def test_can_get_token_via_api(client, factories): 'password': 'test' } - response = client.post(url, payload) + response = api_client.post(url, payload) assert response.status_code == 200 - assert '"token":' in response.content.decode('utf-8') + assert 'token' in response.data -def test_can_refresh_token_via_api(client, factories): +def test_can_get_token_via_api_inactive(api_client, factories): + user = factories['users.User'](is_active=False) + url = reverse('api:v1:token') + payload = { + 'username': user.username, + 'password': 'test' + } + + response = api_client.post(url, payload) + assert response.status_code == 400 + + +def test_can_refresh_token_via_api(api_client, factories, mocker): # first, we get a token user = factories['users.User']() url = reverse('api:v1:token') @@ -95,21 +107,19 @@ def test_can_refresh_token_via_api(client, factories): 'password': 'test' } - response = client.post(url, payload) + response = api_client.post(url, payload) assert response.status_code == 200 - token = json.loads(response.content.decode('utf-8'))['token'] + token = response.data['token'] url = reverse('api:v1:token_refresh') - response = client.post(url,{'token': token}) + response = api_client.post(url, {'token': token}) assert response.status_code == 200 - assert '"token":' in response.content.decode('utf-8') - # a different token should be returned - assert token in response.content.decode('utf-8') + assert 'token' in response.data -def test_changing_password_updates_secret_key(logged_in_client): - user = logged_in_client.user +def test_changing_password_updates_secret_key(logged_in_api_client): + user = logged_in_api_client.user password = user.password secret_key = user.secret_key payload = { @@ -119,7 +129,7 @@ def test_changing_password_updates_secret_key(logged_in_client): } url = reverse('change_password') - response = logged_in_client.post(url, payload) + response = logged_in_api_client.post(url, payload) user.refresh_from_db() diff --git a/changes/changelog.d/218.bugfix b/changes/changelog.d/218.bugfix new file mode 100644 index 000000000..f754d7ca4 --- /dev/null +++ b/changes/changelog.d/218.bugfix @@ -0,0 +1,2 @@ +Ensure inactive users cannot get auth tokens (#218) +This was already the case bug we missed some checks From f91ec0c8104bfabc77ed9eeec3744680fb52db6f Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 May 2018 18:46:39 +0200 Subject: [PATCH 02/57] Upgraded to django-allauth 0.36 --- api/requirements/base.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/requirements/base.txt b/api/requirements/base.txt index ac0586566..d88483de4 100644 --- a/api/requirements/base.txt +++ b/api/requirements/base.txt @@ -10,7 +10,7 @@ Pillow>=4.3,<4.4 # For user registration, either via email or social # Well-built with regular release cycles! -django-allauth>=0.34,<0.35 +django-allauth>=0.36,<0.37 # Python-PostgreSQL Database Adapter From ae00cccf1462b3a00befdb05d3aeb10eb68fb434 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 May 2018 19:04:28 +0200 Subject: [PATCH 03/57] Fix #207: Consistent constraints/checks for URL size --- .../migrations/0006_auto_20180521_1702.py | 28 ++++++++ api/funkwhale_api/federation/models.py | 6 +- api/funkwhale_api/federation/serializers.py | 70 +++++++++---------- changes/changelog.d/207.bugfix | 1 + 4 files changed, 67 insertions(+), 38 deletions(-) create mode 100644 api/funkwhale_api/federation/migrations/0006_auto_20180521_1702.py create mode 100644 changes/changelog.d/207.bugfix diff --git a/api/funkwhale_api/federation/migrations/0006_auto_20180521_1702.py b/api/funkwhale_api/federation/migrations/0006_auto_20180521_1702.py new file mode 100644 index 000000000..7dcf85670 --- /dev/null +++ b/api/funkwhale_api/federation/migrations/0006_auto_20180521_1702.py @@ -0,0 +1,28 @@ +# Generated by Django 2.0.4 on 2018-05-21 17:02 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('federation', '0005_auto_20180413_1723'), + ] + + operations = [ + migrations.AlterField( + model_name='library', + name='url', + field=models.URLField(max_length=500), + ), + migrations.AlterField( + model_name='librarytrack', + name='audio_url', + field=models.URLField(max_length=500), + ), + migrations.AlterField( + model_name='librarytrack', + name='url', + field=models.URLField(max_length=500, unique=True), + ), + ] diff --git a/api/funkwhale_api/federation/models.py b/api/funkwhale_api/federation/models.py index 69d0ea925..8b4f28475 100644 --- a/api/funkwhale_api/federation/models.py +++ b/api/funkwhale_api/federation/models.py @@ -139,7 +139,7 @@ class Library(models.Model): on_delete=models.CASCADE, related_name='library') uuid = models.UUIDField(default=uuid.uuid4) - url = models.URLField() + url = models.URLField(max_length=500) # use this flag to disable federation with a library federation_enabled = models.BooleanField() @@ -166,8 +166,8 @@ def get_file_path(instance, filename): class LibraryTrack(models.Model): - url = models.URLField(unique=True) - audio_url = models.URLField() + url = models.URLField(unique=True, max_length=500) + audio_url = models.URLField(max_length=500) audio_mimetype = models.CharField(max_length=200) audio_file = models.FileField( upload_to=get_file_path, diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 8d3dd6379..51561e222 100644 --- a/api/funkwhale_api/federation/serializers.py +++ b/api/funkwhale_api/federation/serializers.py @@ -26,16 +26,16 @@ logger = logging.getLogger(__name__) class ActorSerializer(serializers.Serializer): - id = serializers.URLField() - outbox = serializers.URLField() - inbox = serializers.URLField() + id = serializers.URLField(max_length=500) + outbox = serializers.URLField(max_length=500) + inbox = serializers.URLField(max_length=500) type = serializers.ChoiceField(choices=models.TYPE_CHOICES) preferredUsername = serializers.CharField() manuallyApprovesFollowers = serializers.NullBooleanField(required=False) name = serializers.CharField(required=False, max_length=200) summary = serializers.CharField(max_length=None, required=False) - followers = serializers.URLField(required=False, allow_null=True) - following = serializers.URLField(required=False, allow_null=True) + followers = serializers.URLField(max_length=500, required=False, allow_null=True) + following = serializers.URLField(max_length=500, required=False, allow_null=True) publicKey = serializers.JSONField(required=False) def to_representation(self, instance): @@ -224,7 +224,7 @@ class APILibraryFollowUpdateSerializer(serializers.Serializer): class APILibraryCreateSerializer(serializers.ModelSerializer): - actor = serializers.URLField() + actor = serializers.URLField(max_length=500) federation_enabled = serializers.BooleanField() uuid = serializers.UUIDField(read_only=True) @@ -315,9 +315,9 @@ class APILibraryTrackSerializer(serializers.ModelSerializer): class FollowSerializer(serializers.Serializer): - id = serializers.URLField() - object = serializers.URLField() - actor = serializers.URLField() + id = serializers.URLField(max_length=500) + object = serializers.URLField(max_length=500) + actor = serializers.URLField(max_length=500) type = serializers.ChoiceField(choices=['Follow']) def validate_object(self, v): @@ -374,8 +374,8 @@ class APIFollowSerializer(serializers.ModelSerializer): class AcceptFollowSerializer(serializers.Serializer): - id = serializers.URLField() - actor = serializers.URLField() + id = serializers.URLField(max_length=500) + actor = serializers.URLField(max_length=500) object = FollowSerializer() type = serializers.ChoiceField(choices=['Accept']) @@ -417,8 +417,8 @@ class AcceptFollowSerializer(serializers.Serializer): class UndoFollowSerializer(serializers.Serializer): - id = serializers.URLField() - actor = serializers.URLField() + id = serializers.URLField(max_length=500) + actor = serializers.URLField(max_length=500) object = FollowSerializer() type = serializers.ChoiceField(choices=['Undo']) @@ -459,9 +459,9 @@ class UndoFollowSerializer(serializers.Serializer): class ActorWebfingerSerializer(serializers.Serializer): subject = serializers.CharField() - aliases = serializers.ListField(child=serializers.URLField()) + aliases = serializers.ListField(child=serializers.URLField(max_length=500)) links = serializers.ListField() - actor_url = serializers.URLField(required=False) + actor_url = serializers.URLField(max_length=500, required=False) def validate(self, validated_data): validated_data['actor_url'] = None @@ -496,8 +496,8 @@ class ActorWebfingerSerializer(serializers.Serializer): class ActivitySerializer(serializers.Serializer): - actor = serializers.URLField() - id = serializers.URLField(required=False) + actor = serializers.URLField(max_length=500) + id = serializers.URLField(max_length=500, required=False) type = serializers.ChoiceField( choices=[(c, c) for c in activity.ACTIVITY_TYPES]) object = serializers.JSONField() @@ -539,8 +539,8 @@ class ActivitySerializer(serializers.Serializer): class ObjectSerializer(serializers.Serializer): - id = serializers.URLField() - url = serializers.URLField(required=False, allow_null=True) + id = serializers.URLField(max_length=500) + url = serializers.URLField(max_length=500, required=False, allow_null=True) type = serializers.ChoiceField( choices=[(c, c) for c in activity.OBJECT_TYPES]) content = serializers.CharField( @@ -554,16 +554,16 @@ class ObjectSerializer(serializers.Serializer): updated = serializers.DateTimeField( required=False, allow_null=True) to = serializers.ListField( - child=serializers.URLField(), + child=serializers.URLField(max_length=500), required=False, allow_null=True) cc = serializers.ListField( - child=serializers.URLField(), + child=serializers.URLField(max_length=500), required=False, allow_null=True) bto = serializers.ListField( - child=serializers.URLField(), + child=serializers.URLField(max_length=500), required=False, allow_null=True) bcc = serializers.ListField( - child=serializers.URLField(), + child=serializers.URLField(max_length=500), required=False, allow_null=True) OBJECT_SERIALIZERS = { @@ -575,10 +575,10 @@ OBJECT_SERIALIZERS = { class PaginatedCollectionSerializer(serializers.Serializer): type = serializers.ChoiceField(choices=['Collection']) totalItems = serializers.IntegerField(min_value=0) - actor = serializers.URLField() - id = serializers.URLField() - first = serializers.URLField() - last = serializers.URLField() + actor = serializers.URLField(max_length=500) + id = serializers.URLField(max_length=500) + first = serializers.URLField(max_length=500) + last = serializers.URLField(max_length=500) def to_representation(self, conf): paginator = Paginator( @@ -607,13 +607,13 @@ class CollectionPageSerializer(serializers.Serializer): type = serializers.ChoiceField(choices=['CollectionPage']) totalItems = serializers.IntegerField(min_value=0) items = serializers.ListField() - actor = serializers.URLField() - id = serializers.URLField() - first = serializers.URLField() - last = serializers.URLField() - next = serializers.URLField(required=False) - prev = serializers.URLField(required=False) - partOf = serializers.URLField() + actor = serializers.URLField(max_length=500) + id = serializers.URLField(max_length=500) + first = serializers.URLField(max_length=500) + last = serializers.URLField(max_length=500) + next = serializers.URLField(max_length=500, required=False) + prev = serializers.URLField(max_length=500, required=False) + partOf = serializers.URLField(max_length=500) def validate_items(self, v): item_serializer = self.context.get('item_serializer') @@ -698,7 +698,7 @@ class AudioMetadataSerializer(serializers.Serializer): class AudioSerializer(serializers.Serializer): type = serializers.CharField() - id = serializers.URLField() + id = serializers.URLField(max_length=500) url = serializers.JSONField() published = serializers.DateTimeField() updated = serializers.DateTimeField(required=False) diff --git a/changes/changelog.d/207.bugfix b/changes/changelog.d/207.bugfix new file mode 100644 index 000000000..edab45cdf --- /dev/null +++ b/changes/changelog.d/207.bugfix @@ -0,0 +1 @@ +Consistent constraints/checks for URL size (#207) From 7b71463ef8e9e88c8c8ee26277d8c3eb13cea02a Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 May 2018 19:55:06 +0200 Subject: [PATCH 04/57] Removed acoustid support, as the integration was buggy and error-prone (#106) --- api/funkwhale_api/music/tasks.py | 15 +++--- .../management/commands/import_files.py | 10 +--- api/tests/music/test_tasks.py | 48 +++++-------------- api/tests/test_import_audio_file.py | 24 ---------- changes/changelog.d/acoustid.misc | 1 + docs/importing-music.rst | 10 ++-- front/src/views/admin/Settings.vue | 3 +- 7 files changed, 27 insertions(+), 84 deletions(-) create mode 100644 changes/changelog.d/acoustid.misc diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index 34345e47b..e5426904a 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -9,7 +9,7 @@ from funkwhale_api.federation import models as federation_models from funkwhale_api.federation import serializers as federation_serializers from funkwhale_api.taskapp import celery from funkwhale_api.providers.acoustid import get_acoustid_client -from funkwhale_api.providers.audiofile.tasks import import_track_data_from_path +from funkwhale_api.providers.audiofile import tasks as audiofile_tasks from django.conf import settings from . import models @@ -73,13 +73,15 @@ def import_track_from_remote(library_track): library_track.title, artist=artist, album=album)[0] -def _do_import(import_job, replace=False, use_acoustid=True): +def _do_import(import_job, replace=False, use_acoustid=False): from_file = bool(import_job.audio_file) mbid = import_job.mbid acoustid_track_id = None duration = None track = None - use_acoustid = use_acoustid and preferences.get('providers_acoustid__api_key') + # use_acoustid = use_acoustid and preferences.get('providers_acoustid__api_key') + # Acoustid is not reliable, we disable it for now. + use_acoustid = False if not mbid and use_acoustid and from_file: # we try to deduce mbid from acoustid client = get_acoustid_client() @@ -91,11 +93,12 @@ def _do_import(import_job, replace=False, use_acoustid=True): if mbid: track, _ = models.Track.get_or_create_from_api(mbid=mbid) elif import_job.audio_file: - track = import_track_data_from_path(import_job.audio_file.path) + track = audiofile_tasks.import_track_data_from_path( + import_job.audio_file.path) elif import_job.library_track: track = import_track_from_remote(import_job.library_track) elif import_job.source.startswith('file://'): - track = import_track_data_from_path( + track = audiofile_tasks.import_track_data_from_path( import_job.source.replace('file://', '', 1)) else: raise ValueError( @@ -151,7 +154,7 @@ def _do_import(import_job, replace=False, use_acoustid=True): models.ImportJob.objects.filter( status__in=['pending', 'errored']), 'import_job') -def import_job_run(self, import_job, replace=False, use_acoustid=True): +def import_job_run(self, import_job, replace=False, use_acoustid=False): def mark_errored(): import_job.status = 'errored' import_job.save(update_fields=['status']) diff --git a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py index a2757c692..70ff90ffa 100644 --- a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py +++ b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py @@ -54,13 +54,6 @@ class Command(BaseCommand): 'import and not much disk space available.' ) ) - parser.add_argument( - '--no-acoustid', - action='store_true', - dest='no_acoustid', - default=False, - help='Use this flag to completely bypass acoustid completely', - ) parser.add_argument( '--noinput', '--no-input', action='store_false', dest='interactive', help="Do NOT prompt the user for input of any kind.", @@ -118,7 +111,6 @@ class Command(BaseCommand): len(filtered['new']))) self.stdout.write('Selected options: {}'.format(', '.join([ - 'no acoustid' if options['no_acoustid'] else 'use acoustid', 'in place' if options['in_place'] else 'copy music files', ]))) if len(filtered['new']) == 0: @@ -201,4 +193,4 @@ class Command(BaseCommand): job.save() import_handler( import_job_id=job.pk, - use_acoustid=not options['no_acoustid']) + use_acoustid=False) diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index c5839432b..26cb9453e 100644 --- a/api/tests/music/test_tasks.py +++ b/api/tests/music/test_tasks.py @@ -105,7 +105,7 @@ def test_run_import_skipping_accoustid(factories, mocker): def test__do_import_skipping_accoustid(factories, mocker): t = factories['music.Track']() m = mocker.patch( - 'funkwhale_api.music.tasks.import_track_data_from_path', + 'funkwhale_api.providers.audiofile.tasks.import_track_data_from_path', return_value=t) path = os.path.join(DATA_DIR, 'test.ogg') job = factories['music.FileImportJob']( @@ -121,7 +121,7 @@ def test__do_import_skipping_accoustid_if_no_key( preferences['providers_acoustid__api_key'] = '' t = factories['music.Track']() m = mocker.patch( - 'funkwhale_api.music.tasks.import_track_data_from_path', + 'funkwhale_api.providers.audiofile.tasks.import_track_data_from_path', return_value=t) path = os.path.join(DATA_DIR, 'test.ogg') job = factories['music.FileImportJob']( @@ -132,32 +132,14 @@ def test__do_import_skipping_accoustid_if_no_key( m.assert_called_once_with(p) -def test_import_job_can_be_skipped( - artists, albums, tracks, factories, mocker, preferences): - preferences['providers_acoustid__api_key'] = 'test' +def test_import_job_skip_if_already_exists( + artists, albums, tracks, factories, mocker): path = os.path.join(DATA_DIR, 'test.ogg') mbid = '9968a9d6-8d92-4051-8f76-674e157b6eed' track_file = factories['music.TrackFile'](track__mbid=mbid) - acoustid_payload = { - 'results': [ - {'id': 'e475bf79-c1ce-4441-bed7-1e33f226c0a2', - 'recordings': [ - { - 'duration': 268, - 'id': mbid}], - 'score': 0.860825}], - 'status': 'ok' - } mocker.patch( - 'funkwhale_api.musicbrainz.api.artists.get', - return_value=artists['get']['adhesive_wombat']) - mocker.patch( - 'funkwhale_api.musicbrainz.api.releases.get', - return_value=albums['get']['marsupial']) - mocker.patch( - 'funkwhale_api.musicbrainz.api.recordings.search', - return_value=tracks['search']['8bitadventures']) - mocker.patch('acoustid.match', return_value=acoustid_payload) + 'funkwhale_api.providers.audiofile.tasks.import_track_data_from_path', + return_value=track_file.track) job = factories['music.FileImportJob'](audio_file__path=path) f = job.audio_file @@ -171,29 +153,23 @@ def test_import_job_can_be_skipped( def test_import_job_can_be_errored(factories, mocker, preferences): - preferences['providers_acoustid__api_key'] = 'test' path = os.path.join(DATA_DIR, 'test.ogg') mbid = '9968a9d6-8d92-4051-8f76-674e157b6eed' track_file = factories['music.TrackFile'](track__mbid=mbid) - acoustid_payload = { - 'results': [ - {'id': 'e475bf79-c1ce-4441-bed7-1e33f226c0a2', - 'recordings': [ - { - 'duration': 268, - 'id': mbid}], - 'score': 0.860825}], - 'status': 'ok' - } + class MyException(Exception): pass - mocker.patch('acoustid.match', side_effect=MyException()) + + mocker.patch( + 'funkwhale_api.music.tasks._do_import', + side_effect=MyException()) job = factories['music.FileImportJob']( audio_file__path=path, track_file=None) with pytest.raises(MyException): tasks.import_job_run(import_job_id=job.pk) + job.refresh_from_db() assert job.track_file is None diff --git a/api/tests/test_import_audio_file.py b/api/tests/test_import_audio_file.py index 8217ffa0b..de8186075 100644 --- a/api/tests/test_import_audio_file.py +++ b/api/tests/test_import_audio_file.py @@ -1,5 +1,4 @@ import pytest -import acoustid import datetime import os import uuid @@ -17,8 +16,6 @@ DATA_DIR = os.path.join( def test_can_create_track_from_file_metadata(db, mocker): - mocker.patch( - 'acoustid.match', side_effect=acoustid.WebServiceError('test')) metadata = { 'artist': ['Test artist'], 'album': ['Test album'], @@ -94,24 +91,6 @@ def test_import_files_creates_a_batch_and_job(factories, mocker): assert job.audio_file.read() == f.read() assert job.source == 'file://' + path - m.assert_called_once_with( - import_job_id=job.pk, - use_acoustid=True) - - -def test_import_files_skip_acoustid(factories, mocker): - m = mocker.patch('funkwhale_api.music.tasks.import_job_run') - user = factories['users.User'](username='me') - path = os.path.join(DATA_DIR, 'dummy_file.ogg') - call_command( - 'import_files', - path, - username='me', - async=False, - no_acoustid=True, - interactive=False) - batch = user.imports.latest('id') - job = batch.jobs.first() m.assert_called_once_with( import_job_id=job.pk, use_acoustid=False) @@ -128,7 +107,6 @@ def test_import_files_skip_if_path_already_imported(factories, mocker): path, username='me', async=False, - no_acoustid=True, interactive=False) assert user.imports.count() == 0 @@ -142,7 +120,6 @@ def test_import_files_works_with_utf8_file_name(factories, mocker): path, username='me', async=False, - no_acoustid=True, interactive=False) batch = user.imports.latest('id') job = batch.jobs.first() @@ -162,7 +139,6 @@ def test_import_files_in_place(factories, mocker, settings): username='me', async=False, in_place=True, - no_acoustid=True, interactive=False) batch = user.imports.latest('id') job = batch.jobs.first() diff --git a/changes/changelog.d/acoustid.misc b/changes/changelog.d/acoustid.misc new file mode 100644 index 000000000..7fc34febe --- /dev/null +++ b/changes/changelog.d/acoustid.misc @@ -0,0 +1 @@ +Removed acoustid support, as the integration was buggy and error-prone (#106) diff --git a/docs/importing-music.rst b/docs/importing-music.rst index 97dd13854..4d256604b 100644 --- a/docs/importing-music.rst +++ b/docs/importing-music.rst @@ -6,7 +6,8 @@ From music directory on the server You can import music files in funkwhale assuming they are located on the server and readable by the funkwhale application. Your music files should contain at -least an ``artist``, ``album`` and ``title`` tags. +least an ``artist``, ``album`` and ``title`` tags, but we recommend you tag +it extensively using a proper tool, such as Beets or Musicbrainz Picard. You can import those tracks as follows, assuming they are located in ``/srv/funkwhale/data/music``: @@ -32,11 +33,6 @@ get details:: For the best results, we recommand tagging your music collection through `Picard `_ in order to have the best quality metadata. -.. note:: - - Autotagging using acoustid is experimental now and can yield unexpected - result. You can disable acoustid by passing the --no-acoustid flag. - .. note:: This command is idempotent, meaning you can run it multiple times on the same @@ -44,7 +40,7 @@ get details:: .. note:: - At the moment, only OGG/Vorbis and MP3 files with ID3 tags are supported + At the moment, only Flac, OGG/Vorbis and MP3 files with ID3 tags are supported .. _in-place-import: diff --git a/front/src/views/admin/Settings.vue b/front/src/views/admin/Settings.vue index 7174ab516..81eb97aa6 100644 --- a/front/src/views/admin/Settings.vue +++ b/front/src/views/admin/Settings.vue @@ -93,8 +93,7 @@ export default { label: this.$t('Imports'), id: 'imports', settings: [ - 'providers_youtube__api_key', - 'providers_acoustid__api_key' + 'providers_youtube__api_key' ] }, { From ca104d64504c07614e72415b9cffb016378d6431 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 May 2018 20:34:03 +0200 Subject: [PATCH 05/57] Increased max body size in dev --- docker/nginx/conf.dev | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/nginx/conf.dev b/docker/nginx/conf.dev index ab6714e60..673edd1a4 100644 --- a/docker/nginx/conf.dev +++ b/docker/nginx/conf.dev @@ -36,7 +36,7 @@ http { server { listen 6001; charset utf-8; - client_max_body_size 20M; + client_max_body_size 30M; include /etc/nginx/funkwhale_proxy.conf; location /_protected/media { internal; From 0c1a2b76c16be40171a65d57f9712393b7052275 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Mon, 21 May 2018 20:38:49 +0200 Subject: [PATCH 06/57] Fix #106 and #213: better web uploader, that supports Flac files --- changes/changelog.d/106.bugfix | 1 + changes/changelog.d/213.bugfix | 9 +++++++ .../components/library/import/FileUpload.vue | 14 +++++++--- front/src/components/library/import/Main.vue | 26 +++++++++++++++++-- 4 files changed, 45 insertions(+), 5 deletions(-) create mode 100644 changes/changelog.d/106.bugfix create mode 100644 changes/changelog.d/213.bugfix diff --git a/changes/changelog.d/106.bugfix b/changes/changelog.d/106.bugfix new file mode 100644 index 000000000..ff0f61609 --- /dev/null +++ b/changes/changelog.d/106.bugfix @@ -0,0 +1 @@ +File-upload importer should now work properly, assuming files are tagged (#106) diff --git a/changes/changelog.d/213.bugfix b/changes/changelog.d/213.bugfix new file mode 100644 index 000000000..d6ff593b9 --- /dev/null +++ b/changes/changelog.d/213.bugfix @@ -0,0 +1,9 @@ +File-upload import now supports Flac files (#213) + +Flac files imports via upload +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +You have nothing to do to benefit from this, however, since Flac files +tend to be a lot bigger than other files, you may want to increase the +``client_max_body_size`` value in your Nginx configuration if you plan +to upload flac files. diff --git a/front/src/components/library/import/FileUpload.vue b/front/src/components/library/import/FileUpload.vue index 9a4b820e3..7aa8adac0 100644 --- a/front/src/components/library/import/FileUpload.vue +++ b/front/src/components/library/import/FileUpload.vue @@ -1,6 +1,10 @@ @@ -107,6 +81,7 @@ import axios from 'axios' import _ from 'lodash' import Pagination from '@/components/Pagination' +import ActionTable from '@/components/common/ActionTable' export default { props: { @@ -114,7 +89,8 @@ export default { showLibrary: {type: Boolean, default: false} }, components: { - Pagination + Pagination, + ActionTable }, data () { return { @@ -123,9 +99,6 @@ export default { page: 1, paginateBy: 25, search: '', - checked: {}, - isImporting: false, - importBatch: null, importedFilter: null } }, @@ -153,47 +126,26 @@ export default { self.errors = error.backendErrors }) }, - launchImport () { - let self = this - self.isImporting = true - let payload = { - objects: this.checked, - action: 'import' - } - axios.post('/federation/library-tracks/action/', payload).then((response) => { - self.importBatch = response.data.result.batch - self.isImporting = false - self.fetchData() - }, error => { - self.isImporting = false - self.errors = error.backendErrors - }) - }, - toggleCheckAll () { - if (this.checked.length === this.result.results.length) { - // we uncheck - this.checked = [] - } else { - this.checked = this.result.results.filter(t => { - return t.local_track_file === null - }).map(t => { return t.id }) - } - }, - toggleCheck (id) { - if (this.checked.indexOf(id) > -1) { - // we uncheck - this.checked.splice(this.checked.indexOf(id), 1) - } else { - this.checked.push(id) - } - }, selectPage: function (page) { this.page = page } }, + computed: { + actionFilters () { + var currentFilters = { + q: this.search + } + if (this.filters) { + return _.merge(currentFilters, this.filters) + } else { + return currentFilters + } + } + }, watch: { search (newValue) { if (newValue.length > 0) { + this.page = 1 this.fetchData() } }, @@ -201,6 +153,7 @@ export default { this.fetchData() }, importedFilter () { + this.page = 1 this.fetchData() } } From 4f8db661fa80c3b1c38fdbbcbc84f64b0fe644c6 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Thu, 24 May 2018 18:33:26 +0200 Subject: [PATCH 22/57] See #228: now expose library track status in API --- api/funkwhale_api/federation/filters.py | 16 ++++++++------ api/funkwhale_api/federation/serializers.py | 13 ++++++++++++ api/funkwhale_api/federation/views.py | 2 +- api/tests/federation/test_serializers.py | 23 +++++++++++++++++++++ 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/api/funkwhale_api/federation/filters.py b/api/funkwhale_api/federation/filters.py index 7a388ff12..1d93f68b9 100644 --- a/api/funkwhale_api/federation/filters.py +++ b/api/funkwhale_api/federation/filters.py @@ -24,7 +24,7 @@ class LibraryFilter(django_filters.FilterSet): class LibraryTrackFilter(django_filters.FilterSet): library = django_filters.CharFilter('library__uuid') - imported = django_filters.CharFilter(method='filter_imported') + status = django_filters.CharFilter(method='filter_status') q = fields.SearchFilter(search_fields=[ 'artist_name', 'title', @@ -32,11 +32,15 @@ class LibraryTrackFilter(django_filters.FilterSet): 'library__actor__domain', ]) - def filter_imported(self, queryset, field_name, value): - if value.lower() in ['true', '1', 'yes']: - queryset = queryset.filter(local_track_file__isnull=False) - elif value.lower() in ['false', '0', 'no']: - queryset = queryset.filter(local_track_file__isnull=True) + def filter_status(self, queryset, field_name, value): + if value == 'imported': + return queryset.filter(local_track_file__isnull=False) + elif value == 'not_imported': + return queryset.filter( + local_track_file__isnull=True + ).exclude(import_jobs__status='pending') + elif value == 'import_pending': + return queryset.filter(import_jobs__status='pending') return queryset class Meta: diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 77cb25096..59e93a9b1 100644 --- a/api/funkwhale_api/federation/serializers.py +++ b/api/funkwhale_api/federation/serializers.py @@ -296,6 +296,7 @@ class APILibraryCreateSerializer(serializers.ModelSerializer): class APILibraryTrackSerializer(serializers.ModelSerializer): library = APILibrarySerializer() + status = serializers.SerializerMethodField() class Meta: model = models.LibraryTrack @@ -314,8 +315,20 @@ class APILibraryTrackSerializer(serializers.ModelSerializer): 'title', 'library', 'local_track_file', + 'status', ] + def get_status(self, o): + try: + if o.local_track_file is not None: + return 'imported' + except music_models.TrackFile.DoesNotExist: + pass + for job in o.import_jobs.all(): + if job.status == 'pending': + return 'import_pending' + return 'not_imported' + class FollowSerializer(serializers.Serializer): id = serializers.URLField(max_length=500) diff --git a/api/funkwhale_api/federation/views.py b/api/funkwhale_api/federation/views.py index 01bc02fdf..1350ec731 100644 --- a/api/funkwhale_api/federation/views.py +++ b/api/funkwhale_api/federation/views.py @@ -296,7 +296,7 @@ class LibraryTrackViewSet( 'library__actor', 'library__follow', 'local_track_file', - ) + ).prefetch_related('import_jobs') filter_class = filters.LibraryTrackFilter serializer_class = serializers.APILibraryTrackSerializer ordering_fields = ( diff --git a/api/tests/federation/test_serializers.py b/api/tests/federation/test_serializers.py index f298c61f5..fcf2ba1b6 100644 --- a/api/tests/federation/test_serializers.py +++ b/api/tests/federation/test_serializers.py @@ -699,3 +699,26 @@ def test_api_library_create_serializer_save(factories, r_mock): assert library.tracks_count == 10 assert library.actor == actor assert library.follow == follow + + +def test_tapi_library_track_serializer_not_imported(factories): + lt = factories['federation.LibraryTrack']() + serializer = serializers.APILibraryTrackSerializer(lt) + + assert serializer.get_status(lt) == 'not_imported' + + +def test_tapi_library_track_serializer_imported(factories): + tf = factories['music.TrackFile'](federation=True) + lt = tf.library_track + serializer = serializers.APILibraryTrackSerializer(lt) + + assert serializer.get_status(lt) == 'imported' + + +def test_tapi_library_track_serializer_import_pending(factories): + job = factories['music.ImportJob'](federation=True, status='pending') + lt = job.library_track + serializer = serializers.APILibraryTrackSerializer(lt) + + assert serializer.get_status(lt) == 'import_pending' From eded32c2e8271cfffb9575a123f0caabdcc851f9 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Thu, 24 May 2018 18:53:12 +0200 Subject: [PATCH 23/57] See #228: more performante federation import launch via API --- api/funkwhale_api/federation/serializers.py | 12 ++++++------ api/funkwhale_api/music/tasks.py | 7 +++++++ api/tests/federation/test_views.py | 4 +++- api/tests/music/test_tasks.py | 9 +++++++++ 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 59e93a9b1..6ffffaa9a 100644 --- a/api/funkwhale_api/federation/serializers.py +++ b/api/funkwhale_api/federation/serializers.py @@ -834,17 +834,17 @@ class LibraryTrackActionSerializer(common_serializers.ActionSerializer): source='federation', submitted_by=self.context['submitted_by'] ) + jobs = [] for lt in objects: - job = music_models.ImportJob.objects.create( + job = music_models.ImportJob( batch=batch, library_track=lt, mbid=lt.mbid, source=lt.url, ) - funkwhale_utils.on_commit( - music_tasks.import_job_run.delay, - import_job_id=job.pk, - use_acoustid=False, - ) + jobs.append(job) + + music_models.ImportJob.objects.bulk_create(jobs) + music_tasks.import_batch_run.delay(import_batch_id=batch.pk) return {'batch': {'id': batch.pk}} diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index e5426904a..993456c27 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -173,6 +173,13 @@ def import_job_run(self, import_job, replace=False, use_acoustid=False): raise +@celery.app.task(name='ImportBatch.run') +@celery.require_instance(models.ImportBatch, 'import_batch') +def import_batch_run(import_batch): + for job_id in import_batch.jobs.order_by('id').values_list('id', flat=True): + import_job_run.delay(import_job_id=job_id) + + @celery.app.task(name='Lyrics.fetch_content') @celery.require_instance(models.Lyrics, 'lyrics') def fetch_content(lyrics): diff --git a/api/tests/federation/test_views.py b/api/tests/federation/test_views.py index 1b0de5754..04a419aed 100644 --- a/api/tests/federation/test_views.py +++ b/api/tests/federation/test_views.py @@ -426,7 +426,8 @@ def test_library_track_action_import( lt2 = factories['federation.LibraryTrack'](library=lt1.library) lt3 = factories['federation.LibraryTrack']() lt4 = factories['federation.LibraryTrack'](library=lt3.library) - mocker.patch('funkwhale_api.music.tasks.import_job_run') + mocked_run = mocker.patch( + 'funkwhale_api.music.tasks.import_batch_run.delay') payload = { 'objects': 'all', @@ -452,3 +453,4 @@ def test_library_track_action_import( assert batch.jobs.count() == 2 for i, job in enumerate(batch.jobs.all()): assert job.library_track == imported_lts[i] + mocked_run.assert_called_once_with(import_batch_id=batch.pk) diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 26cb9453e..dfe649be0 100644 --- a/api/tests/music/test_tasks.py +++ b/api/tests/music/test_tasks.py @@ -47,6 +47,15 @@ def test_set_acoustid_on_track_file_required_high_score(factories, mocker): assert track_file.acoustid_track_id is None +def test_import_batch_run(factories, mocker): + job = factories['music.ImportJob']() + mocked_job_run = mocker.patch( + 'funkwhale_api.music.tasks.import_job_run.delay') + tasks.import_batch_run(import_batch_id=job.batch.pk) + + mocked_job_run.assert_called_once_with(import_job_id=job.pk) + + def test_import_job_can_run_with_file_and_acoustid( artists, albums, tracks, preferences, factories, mocker): preferences['providers_acoustid__api_key'] = 'test' From 6586b2b73d6d094142a0d99edc6e750e4b5d925f Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Thu, 24 May 2018 20:07:14 +0200 Subject: [PATCH 24/57] See #228: smarter action table with shift-click select --- front/src/components/common/ActionTable.vue | 101 ++++++++++++--- .../src/components/common/DangerousButton.vue | 13 +- .../federation/LibraryTrackTable.vue | 115 ++++++++++-------- 3 files changed, 159 insertions(+), 70 deletions(-) diff --git a/front/src/components/common/ActionTable.vue b/front/src/components/common/ActionTable.vue index b9822ca4d..718e57b19 100644 --- a/front/src/components/common/ActionTable.vue +++ b/front/src/components/common/ActionTable.vue @@ -1,29 +1,42 @@