diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py index aa07ad52c..d8b173b4f 100644 --- a/api/funkwhale_api/music/views.py +++ b/api/funkwhale_api/music/views.py @@ -91,12 +91,21 @@ class ImportBatchViewSet( ) serializer_class = serializers.ImportBatchSerializer permission_classes = (HasUserPermission,) - required_permissions = ['library'] + required_permissions = ['library', 'upload'] + permission_operator = 'or' filter_class = filters.ImportBatchFilter def perform_create(self, serializer): serializer.save(submitted_by=self.request.user) + def get_queryset(self): + qs = super().get_queryset() + # if user do not have library permission, we limit to their + # own jobs + if not self.request.user.has_permissions('library'): + qs = qs.filter(submitted_by=self.request.user) + return qs + class ImportJobViewSet( mixins.CreateModelMixin, @@ -105,11 +114,22 @@ class ImportJobViewSet( queryset = (models.ImportJob.objects.all().select_related()) serializer_class = serializers.ImportJobSerializer permission_classes = (HasUserPermission,) - required_permissions = ['library'] + required_permissions = ['library', 'upload'] + permission_operator = 'or' filter_class = filters.ImportJobFilter + def get_queryset(self): + qs = super().get_queryset() + # if user do not have library permission, we limit to their + # own jobs + if not self.request.user.has_permissions('library'): + qs = qs.filter(batch__submitted_by=self.request.user) + return qs + @list_route(methods=['get']) def stats(self, request, *args, **kwargs): + if not request.user.has_permissions('library'): + return Response(status=403) qs = models.ImportJob.objects.all() filterset = filters.ImportJobFilter(request.GET, queryset=qs) qs = filterset.qs diff --git a/api/funkwhale_api/users/admin.py b/api/funkwhale_api/users/admin.py index 7e9062a13..cb74abf0e 100644 --- a/api/funkwhale_api/users/admin.py +++ b/api/funkwhale_api/users/admin.py @@ -62,6 +62,7 @@ class UserAdmin(AuthUserAdmin): 'is_active', 'is_staff', 'is_superuser', + 'permission_upload', 'permission_library', 'permission_settings', 'permission_federation')}), diff --git a/api/funkwhale_api/users/migrations/0007_auto_20180524_2009.py b/api/funkwhale_api/users/migrations/0007_auto_20180524_2009.py new file mode 100644 index 000000000..e3d582c53 --- /dev/null +++ b/api/funkwhale_api/users/migrations/0007_auto_20180524_2009.py @@ -0,0 +1,33 @@ +# Generated by Django 2.0.4 on 2018-05-24 20:09 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0006_auto_20180517_2324'), + ] + + operations = [ + migrations.AddField( + model_name='user', + name='permission_upload', + field=models.BooleanField(default=False, verbose_name='Upload new content to the library'), + ), + migrations.AlterField( + model_name='user', + name='permission_federation', + field=models.BooleanField(default=False, help_text='Follow other instances, accept/deny library follow requests...', verbose_name='Manage library federation'), + ), + migrations.AlterField( + model_name='user', + name='permission_library', + field=models.BooleanField(default=False, help_text='Manage library', verbose_name='Manage library'), + ), + migrations.AlterField( + model_name='user', + name='permission_settings', + field=models.BooleanField(default=False, verbose_name='Manage instance-level settings'), + ), + ] diff --git a/api/funkwhale_api/users/models.py b/api/funkwhale_api/users/models.py index c16cd62b3..a739c1e38 100644 --- a/api/funkwhale_api/users/models.py +++ b/api/funkwhale_api/users/models.py @@ -23,6 +23,7 @@ PERMISSIONS = [ 'federation', 'library', 'settings', + 'upload', ] @@ -52,11 +53,14 @@ class User(AbstractUser): default=False) permission_library = models.BooleanField( 'Manage library', - help_text='Import new content, manage existing content', + help_text='Manage library', default=False) permission_settings = models.BooleanField( 'Manage instance-level settings', default=False) + permission_upload = models.BooleanField( + 'Upload new content to the library', + default=False) def __str__(self): return self.username @@ -68,9 +72,12 @@ class User(AbstractUser): perms[p] = v return perms - def has_permissions(self, *perms): + def has_permissions(self, *perms, operator='and'): + if operator not in ['and', 'or']: + raise ValueError('Invalid operator {}'.format(operator)) permissions = self.get_permissions() - return all([permissions[p] for p in perms]) + checker = all if operator == 'and' else any + return checker([permissions[p] for p in perms]) def get_absolute_url(self): return reverse('users:detail', kwargs={'username': self.username}) diff --git a/api/funkwhale_api/users/permissions.py b/api/funkwhale_api/users/permissions.py index 2ff49ff3f..146bc5e1c 100644 --- a/api/funkwhale_api/users/permissions.py +++ b/api/funkwhale_api/users/permissions.py @@ -16,4 +16,6 @@ class HasUserPermission(BasePermission): return False if request.user.is_anonymous: return False - return request.user.has_permissions(*view.required_permissions) + operator = getattr(view, 'permission_operator', 'and') + return request.user.has_permissions( + *view.required_permissions, operator=operator) diff --git a/api/tests/conftest.py b/api/tests/conftest.py index d6b1c0204..7caff2009 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -231,8 +231,9 @@ def authenticated_actor(factories, mocker): @pytest.fixture def assert_user_permission(): - def inner(view, permissions): + def inner(view, permissions, operator='and'): assert HasUserPermission in view.permission_classes + assert getattr(view, 'permission_operator', 'and') == operator assert set(view.required_permissions) == set(permissions) return inner diff --git a/api/tests/music/test_views.py b/api/tests/music/test_views.py index 9328ba329..1fe113832 100644 --- a/api/tests/music/test_views.py +++ b/api/tests/music/test_views.py @@ -9,12 +9,12 @@ from funkwhale_api.music import views from funkwhale_api.federation import actors -@pytest.mark.parametrize('view,permissions', [ - (views.ImportBatchViewSet, ['library']), - (views.ImportJobViewSet, ['library']), +@pytest.mark.parametrize('view,permissions,operator', [ + (views.ImportBatchViewSet, ['library', 'upload'], 'or'), + (views.ImportJobViewSet, ['library', 'upload'], 'or'), ]) -def test_permissions(assert_user_permission, view, permissions): - assert_user_permission(view, permissions) +def test_permissions(assert_user_permission, view, permissions, operator): + assert_user_permission(view, permissions, operator) def test_artist_list_serializer(api_request, factories, logged_in_api_client): @@ -351,3 +351,27 @@ def test_import_batch_and_job_run_via_api( run.assert_any_call(import_job_id=job1.pk) run.assert_any_call(import_job_id=job2.pk) + + +def test_import_job_viewset_get_queryset_upload_filters_user( + factories, logged_in_api_client): + logged_in_api_client.user.permission_upload = True + logged_in_api_client.user.save() + + job = factories['music.ImportJob']() + url = reverse('api:v1:import-jobs-list') + response = logged_in_api_client.get(url) + + assert response.data['count'] == 0 + + +def test_import_batch_viewset_get_queryset_upload_filters_user( + factories, logged_in_api_client): + logged_in_api_client.user.permission_upload = True + logged_in_api_client.user.save() + + job = factories['music.ImportBatch']() + url = reverse('api:v1:import-batches-list') + response = logged_in_api_client.get(url) + + assert response.data['count'] == 0 diff --git a/api/tests/users/test_models.py b/api/tests/users/test_models.py index 49199e0a7..445744802 100644 --- a/api/tests/users/test_models.py +++ b/api/tests/users/test_models.py @@ -47,6 +47,17 @@ def test_get_permissions_regular(factories): ({'permission_library': True}, ['library'], True), ({'permission_library': True}, ['library', 'federation'], False), ]) -def test_has_permissions(args, perms, expected, factories): +def test_has_permissions_and(args, perms, expected, factories): user = factories['users.User'](**args) - assert user.has_permissions(*perms) is expected + assert user.has_permissions(*perms, operator='and') is expected + + +@pytest.mark.parametrize('args,perms,expected', [ + ({'is_superuser': True}, ['federation', 'library'], True), + ({'is_superuser': False}, ['federation'], False), + ({'permission_library': True}, ['library', 'federation'], True), + ({'permission_library': True}, ['federation'], False), +]) +def test_has_permissions_or(args, perms, expected, factories): + user = factories['users.User'](**args) + assert user.has_permissions(*perms, operator='or') is expected diff --git a/api/tests/users/test_permissions.py b/api/tests/users/test_permissions.py index 1564c761d..518ccd1c8 100644 --- a/api/tests/users/test_permissions.py +++ b/api/tests/users/test_permissions.py @@ -39,7 +39,7 @@ def test_has_user_permission_logged_in_single(value, factories, api_request): (False, False, False), (True, True, True), ]) -def test_has_user_permission_logged_in_single( +def test_has_user_permission_logged_in_multiple_and( federation, library, expected, factories, api_request): user = factories['users.User']( permission_federation=federation, @@ -48,9 +48,35 @@ def test_has_user_permission_logged_in_single( class View(APIView): required_permissions = ['federation', 'library'] + permission_operator = 'and' view = View() permission = permissions.HasUserPermission() request = api_request.get('/') setattr(request, 'user', user) result = permission.has_permission(request, view) assert result == user.has_permissions('federation', 'library') == expected + + +@pytest.mark.parametrize('federation,library,expected', [ + (True, False, True), + (False, True, True), + (False, False, False), + (True, True, True), +]) +def test_has_user_permission_logged_in_multiple_or( + federation, library, expected, factories, api_request): + user = factories['users.User']( + permission_federation=federation, + permission_library=library, + ) + + class View(APIView): + required_permissions = ['federation', 'library'] + permission_operator = 'or' + view = View() + permission = permissions.HasUserPermission() + request = api_request.get('/') + setattr(request, 'user', user) + result = permission.has_permission(request, view) + assert result == user.has_permissions( + 'federation', 'library', operator='or') == expected diff --git a/changes/changelog.d/230.enhancement b/changes/changelog.d/230.enhancement new file mode 100644 index 000000000..3d8c4176f --- /dev/null +++ b/changes/changelog.d/230.enhancement @@ -0,0 +1,22 @@ +Added a new "upload" permission that allows user to launch import and view +their own imports (#230) + +New "upload" permission +^^^^^^^^^^^^^^^^^^^^^^^ + +This new permission is helpful if you want to give upload/import rights +to some users, but don't want them to be able to manage the library as a whole: +although there are no controls yet for managing library in the fron-end, +subsequent release will introduce management interfaces for artists, files, +etc. + +Because of that, users with the "library" permission will have much more power, +and will also be able to remove content from the platform. On the other hand, +users with the "upload" permission will only have the ability to add new +content. + +Also, this release also includes a new feature called "default permissions": +those are permissions that are granted to every users on the platform. +On public/open instances, this will play well with the "upload" permission +since everyone will be able to contribute to the instance library without +an admin giving the permission to every single user. diff --git a/front/src/components/Sidebar.vue b/front/src/components/Sidebar.vue index 9f3134c2a..e8f330c38 100644 --- a/front/src/components/Sidebar.vue +++ b/front/src/components/Sidebar.vue @@ -68,6 +68,12 @@ :title="$t('Pending import requests')"> {{ notifications.importRequests }} + + {{ $t('Import music') }} + { return e diff --git a/front/src/components/library/Library.vue b/front/src/components/library/Library.vue index e360ccb1c..50337b229 100644 --- a/front/src/components/library/Library.vue +++ b/front/src/components/library/Library.vue @@ -13,10 +13,10 @@ exact> - + - + @@ -27,7 +27,11 @@