From 6fc4275b681ac65e1ebd289b55e99856408af12f Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 18 May 2018 18:48:46 +0200 Subject: [PATCH] See #152: use new user permissions on relevant viewsets --- api/funkwhale_api/common/permissions.py | 13 +--------- api/funkwhale_api/federation/views.py | 12 ++++----- api/funkwhale_api/instance/views.py | 4 ++- api/funkwhale_api/music/views.py | 18 ++++++-------- api/funkwhale_api/users/serializers.py | 9 ++----- api/tests/conftest.py | 9 +++++++ api/tests/federation/test_views.py | 9 +++++++ api/tests/instance/test_views.py | 14 ++++++++++- api/tests/music/test_views.py | 8 ++++++ api/tests/users/test_views.py | 33 ++++++++++--------------- 10 files changed, 71 insertions(+), 58 deletions(-) diff --git a/api/funkwhale_api/common/permissions.py b/api/funkwhale_api/common/permissions.py index cab4b699d..e9e8b8819 100644 --- a/api/funkwhale_api/common/permissions.py +++ b/api/funkwhale_api/common/permissions.py @@ -3,7 +3,7 @@ import operator from django.conf import settings from django.http import Http404 -from rest_framework.permissions import BasePermission, DjangoModelPermissions +from rest_framework.permissions import BasePermission from funkwhale_api.common import preferences @@ -16,17 +16,6 @@ class ConditionalAuthentication(BasePermission): return True -class HasModelPermission(DjangoModelPermissions): - """ - Same as DjangoModelPermissions, but we pin the model: - - class MyModelPermission(HasModelPermission): - model = User - """ - def get_required_permissions(self, method, model_cls): - return super().get_required_permissions(method, self.model) - - class OwnerPermission(BasePermission): """ Ensure the request user is the owner of the object. diff --git a/api/funkwhale_api/federation/views.py b/api/funkwhale_api/federation/views.py index 37ad9ebfd..06a2cd040 100644 --- a/api/funkwhale_api/federation/views.py +++ b/api/funkwhale_api/federation/views.py @@ -15,8 +15,8 @@ from rest_framework.serializers import ValidationError from funkwhale_api.common import preferences from funkwhale_api.common import utils as funkwhale_utils -from funkwhale_api.common.permissions import HasModelPermission from funkwhale_api.music.models import TrackFile +from funkwhale_api.users.permissions import HasUserPermission from . import activity from . import actors @@ -187,16 +187,13 @@ class MusicFilesViewSet(FederationMixin, viewsets.GenericViewSet): return response.Response(data) -class LibraryPermission(HasModelPermission): - model = models.Library - - class LibraryViewSet( mixins.RetrieveModelMixin, mixins.UpdateModelMixin, mixins.ListModelMixin, viewsets.GenericViewSet): - permission_classes = [LibraryPermission] + permission_classes = (HasUserPermission,) + required_permissions = ['federation'] queryset = models.Library.objects.all().select_related( 'actor', 'follow', @@ -291,7 +288,8 @@ class LibraryViewSet( class LibraryTrackViewSet( mixins.ListModelMixin, viewsets.GenericViewSet): - permission_classes = [LibraryPermission] + permission_classes = (HasUserPermission,) + required_permissions = ['federation'] queryset = models.LibraryTrack.objects.all().select_related( 'library__actor', 'library__follow', diff --git a/api/funkwhale_api/instance/views.py b/api/funkwhale_api/instance/views.py index e6725e248..b905acd3e 100644 --- a/api/funkwhale_api/instance/views.py +++ b/api/funkwhale_api/instance/views.py @@ -6,6 +6,7 @@ from dynamic_preferences.api import viewsets as preferences_viewsets from dynamic_preferences.registries import global_preferences_registry from funkwhale_api.common import preferences +from funkwhale_api.users.permissions import HasUserPermission from . import nodeinfo from . import stats @@ -18,7 +19,8 @@ NODEINFO_2_CONTENT_TYPE = ( class AdminSettings(preferences_viewsets.GlobalPreferencesViewSet): pagination_class = None - + permission_classes = (HasUserPermission,) + required_permissions = ['settings'] class InstanceSettings(views.APIView): permission_classes = [] diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py index f2ab72c5a..e71d3555e 100644 --- a/api/funkwhale_api/music/views.py +++ b/api/funkwhale_api/music/views.py @@ -25,8 +25,8 @@ from rest_framework import permissions from musicbrainzngs import ResponseError from funkwhale_api.common import utils as funkwhale_utils -from funkwhale_api.common.permissions import ( - ConditionalAuthentication, HasModelPermission) +from funkwhale_api.common.permissions import ConditionalAuthentication +from funkwhale_api.users.permissions import HasUserPermission from taggit.models import Tag from funkwhale_api.federation import actors from funkwhale_api.federation.authentication import SignatureAuthentication @@ -107,25 +107,22 @@ class ImportBatchViewSet( .annotate(job_count=Count('jobs')) ) serializer_class = serializers.ImportBatchSerializer - permission_classes = (permissions.DjangoModelPermissions, ) + permission_classes = (HasUserPermission,) + required_permissions = ['library'] filter_class = filters.ImportBatchFilter def perform_create(self, serializer): serializer.save(submitted_by=self.request.user) -class ImportJobPermission(HasModelPermission): - # not a typo, perms on import job is proxied to import batch - model = models.ImportBatch - - class ImportJobViewSet( mixins.CreateModelMixin, mixins.ListModelMixin, viewsets.GenericViewSet): queryset = (models.ImportJob.objects.all().select_related()) serializer_class = serializers.ImportJobSerializer - permission_classes = (ImportJobPermission, ) + permission_classes = (HasUserPermission,) + required_permissions = ['library'] filter_class = filters.ImportJobFilter @list_route(methods=['get']) @@ -442,7 +439,8 @@ class Search(views.APIView): class SubmitViewSet(viewsets.ViewSet): queryset = models.ImportBatch.objects.none() - permission_classes = (permissions.DjangoModelPermissions, ) + permission_classes = (HasUserPermission,) + required_permissions = ['library'] @list_route(methods=['post']) @transaction.non_atomic_requests diff --git a/api/funkwhale_api/users/serializers.py b/api/funkwhale_api/users/serializers.py index eadce6154..3a095e78a 100644 --- a/api/funkwhale_api/users/serializers.py +++ b/api/funkwhale_api/users/serializers.py @@ -55,16 +55,11 @@ class UserReadSerializer(serializers.ModelSerializer): 'is_superuser', 'permissions', 'date_joined', - 'privacy_level' + 'privacy_level', ] def get_permissions(self, o): - perms = {} - for internal_codename, conf in o.relevant_permissions.items(): - perms[conf['external_codename']] = { - 'status': o.has_perm(internal_codename) - } - return perms + return o.get_permissions() class PasswordResetSerializer(PRS): diff --git a/api/tests/conftest.py b/api/tests/conftest.py index dda537801..b7a7d071a 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -14,6 +14,7 @@ from rest_framework.test import APIClient from rest_framework.test import APIRequestFactory from funkwhale_api.activity import record +from funkwhale_api.users.permissions import HasUserPermission from funkwhale_api.taskapp import celery @@ -224,3 +225,11 @@ def authenticated_actor(factories, mocker): 'funkwhale_api.federation.authentication.SignatureAuthentication.authenticate_actor', return_value=actor) yield actor + + +@pytest.fixture +def assert_user_permission(): + def inner(view, permissions): + assert HasUserPermission in view.permission_classes + assert set(view.required_permissions) == set(permissions) + return inner diff --git a/api/tests/federation/test_views.py b/api/tests/federation/test_views.py index fd6ac2eb2..10237ed9f 100644 --- a/api/tests/federation/test_views.py +++ b/api/tests/federation/test_views.py @@ -9,9 +9,18 @@ from funkwhale_api.federation import activity from funkwhale_api.federation import models from funkwhale_api.federation import serializers from funkwhale_api.federation import utils +from funkwhale_api.federation import views from funkwhale_api.federation import webfinger +@pytest.mark.parametrize('view,permissions', [ + (views.LibraryViewSet, ['federation']), + (views.LibraryTrackViewSet, ['federation']), +]) +def test_permissions(assert_user_permission, view, permissions): + assert_user_permission(view, permissions) + + @pytest.mark.parametrize('system_actor', actors.SYSTEM_ACTORS.keys()) def test_instance_actors(system_actor, db, api_client): actor = actors.SYSTEM_ACTORS[system_actor].get_actor_instance() diff --git a/api/tests/instance/test_views.py b/api/tests/instance/test_views.py index 6d8dcac3e..daf54db51 100644 --- a/api/tests/instance/test_views.py +++ b/api/tests/instance/test_views.py @@ -1,5 +1,16 @@ +import pytest + from django.urls import reverse +from funkwhale_api.instance import views + + +@pytest.mark.parametrize('view,permissions', [ + (views.AdminSettings, ['settings']), +]) +def test_permissions(assert_user_permission, view, permissions): + assert_user_permission(view, permissions) + def test_nodeinfo_endpoint(db, api_client, mocker): payload = { @@ -43,7 +54,8 @@ def test_admin_settings_restrict_access(db, logged_in_api_client, preferences): def test_admin_settings_correct_permission( db, logged_in_api_client, preferences): user = logged_in_api_client.user - user.add_permission('change_globalpreferencemodel') + user.permission_settings = True + user.save() url = reverse('api:v1:instance:admin-settings-list') response = logged_in_api_client.get(url) diff --git a/api/tests/music/test_views.py b/api/tests/music/test_views.py index e641b45d5..030fc3a73 100644 --- a/api/tests/music/test_views.py +++ b/api/tests/music/test_views.py @@ -8,6 +8,14 @@ from funkwhale_api.music import views from funkwhale_api.federation import actors +@pytest.mark.parametrize('view,permissions', [ + (views.ImportBatchViewSet, ['library']), + (views.ImportJobViewSet, ['library']), +]) +def test_permissions(assert_user_permission, view, permissions): + assert_user_permission(view, permissions) + + @pytest.mark.parametrize('param,expected', [ ('true', 'full'), ('false', 'empty'), diff --git a/api/tests/users/test_views.py b/api/tests/users/test_views.py index fffc762fd..1bbf8b9a2 100644 --- a/api/tests/users/test_views.py +++ b/api/tests/users/test_views.py @@ -53,33 +53,24 @@ def test_can_disable_registration_view(preferences, client, db): assert response.status_code == 403 -def test_can_fetch_data_from_api(client, factories): +def test_can_fetch_data_from_api(api_client, factories): url = reverse('api:v1:users:users-me') - response = client.get(url) + response = api_client.get(url) # login required assert response.status_code == 401 user = factories['users.User']( - is_staff=True, - perms=[ - 'music.add_importbatch', - 'dynamic_preferences.change_globalpreferencemodel', - ] + permission_library=True ) - assert user.has_perm('music.add_importbatch') - client.login(username=user.username, password='test') - response = client.get(url) + api_client.login(username=user.username, password='test') + response = api_client.get(url) assert response.status_code == 200 - - payload = json.loads(response.content.decode('utf-8')) - - assert payload['username'] == user.username - assert payload['is_staff'] == user.is_staff - assert payload['is_superuser'] == user.is_superuser - assert payload['email'] == user.email - assert payload['name'] == user.name - assert payload['permissions']['import.launch']['status'] - assert payload['permissions']['settings.change']['status'] + assert response.data['username'] == user.username + assert response.data['is_staff'] == user.is_staff + assert response.data['is_superuser'] == user.is_superuser + assert response.data['email'] == user.email + assert response.data['name'] == user.name + assert response.data['permissions'] == user.get_permissions() def test_can_get_token_via_api(client, factories): @@ -202,6 +193,8 @@ def test_user_can_get_new_subsonic_token(logged_in_api_client): assert response.data == { 'subsonic_api_token': 'test' } + + def test_user_can_request_new_subsonic_token(logged_in_api_client): user = logged_in_api_client.user user.subsonic_api_token = 'test'