Merge branch '230-upload-permission' into 'develop'

Resolve "Split library management and upload permission"

Closes #230

See merge request funkwhale/funkwhale!212

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

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

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'),
+        ),
+    ]

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

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)

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
 

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', [
+@pytest.mark.parametrize('view,permissions,operator', [
-    (views.ImportBatchViewSet, ['library']),
+    (views.ImportBatchViewSet, ['library', 'upload'], 'or'),
-    (views.ImportJobViewSet, ['library']),
+    (views.ImportJobViewSet, ['library', 'upload'], 'or'),
 ])
-def test_permissions(assert_user_permission, view, permissions):
+def test_permissions(assert_user_permission, view, permissions, operator):
-    assert_user_permission(view, permissions)
+    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

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

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

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.

front/src/components/Sidebar.vue

@@ -68,6 +68,12 @@
                 :title="$t('Pending import requests')">
                 {{ notifications.importRequests }}</div>
             </router-link>
+            <router-link
+              class="item"
+              v-else-if="$store.state.auth.availablePermissions['upload']"
+              to="/library/import/launch">
+              <i class="download icon"></i>{{ $t('Import music') }}
+            </router-link>
             <router-link
               class="item"
               v-if="$store.state.auth.availablePermissions['federation']"
@@ -193,7 +199,8 @@ export default {
     showAdmin () {
       let adminPermissions = [
         this.$store.state.auth.availablePermissions['federation'],
-        this.$store.state.auth.availablePermissions['library']
+        this.$store.state.auth.availablePermissions['library'],
+        this.$store.state.auth.availablePermissions['upload']
       ]
       return adminPermissions.filter(e => {
         return e

front/src/components/library/Library.vue

@@ -13,10 +13,10 @@
           exact>
           <i18next path="Requests"/>
         </router-link>
-        <router-link v-if="$store.state.auth.availablePermissions['library']" class="ui item" to="/library/import/launch" exact>
+        <router-link v-if="showImports" class="ui item" to="/library/import/launch" exact>
           <i18next path="Import"/>
         </router-link>
-        <router-link v-if="$store.state.auth.availablePermissions['library']" class="ui item" to="/library/import/batches">
+        <router-link v-if="showImports" class="ui item" to="/library/import/batches">
           <i18next path="Import batches"/>
         </router-link>
       </div>
@@ -27,7 +27,11 @@
 
 <script>
 export default {
-  name: 'library'
+  computed: {
+    showImports () {
+      return this.$store.state.auth.availablePermissions['upload'] || this.$store.state.auth.availablePermissions['library']
+    }
+  }
 }
 </script>