From ff65a4b93592d7da25a2693042656c89461e6d54 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 18 May 2018 18:47:35 +0200 Subject: [PATCH 1/6] See #152: added permission fields on user model and corresponding API permission --- .../migrations/0006_auto_20180517_2324.py | 28 ++++++++++ api/funkwhale_api/users/models.py | 39 +++++++------ api/funkwhale_api/users/permissions.py | 19 +++++++ api/tests/users/test_models.py | 34 +++++++++++ api/tests/users/test_permissions.py | 56 +++++++++++++++++++ 5 files changed, 159 insertions(+), 17 deletions(-) create mode 100644 api/funkwhale_api/users/migrations/0006_auto_20180517_2324.py create mode 100644 api/funkwhale_api/users/permissions.py create mode 100644 api/tests/users/test_permissions.py diff --git a/api/funkwhale_api/users/migrations/0006_auto_20180517_2324.py b/api/funkwhale_api/users/migrations/0006_auto_20180517_2324.py new file mode 100644 index 000000000..7c9ab0fad --- /dev/null +++ b/api/funkwhale_api/users/migrations/0006_auto_20180517_2324.py @@ -0,0 +1,28 @@ +# Generated by Django 2.0.4 on 2018-05-17 23:24 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0005_user_subsonic_api_token'), + ] + + operations = [ + migrations.AddField( + model_name='user', + name='permission_federation', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='user', + name='permission_library', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='user', + name='permission_settings', + field=models.BooleanField(default=False), + ), + ] diff --git a/api/funkwhale_api/users/models.py b/api/funkwhale_api/users/models.py index 8273507c4..1de23092a 100644 --- a/api/funkwhale_api/users/models.py +++ b/api/funkwhale_api/users/models.py @@ -19,6 +19,13 @@ def get_token(): return binascii.b2a_hex(os.urandom(15)).decode('utf-8') +PERMISSIONS = [ + 'federation', + 'library', + 'settings', +] + + @python_2_unicode_compatible class User(AbstractUser): @@ -28,20 +35,6 @@ class User(AbstractUser): # updated on logout or password change, to invalidate JWT secret_key = models.UUIDField(default=uuid.uuid4, null=True) - # permissions that are used for API access and that worth serializing - relevant_permissions = { - # internal_codename : {external_codename} - 'music.add_importbatch': { - 'external_codename': 'import.launch', - }, - 'dynamic_preferences.change_globalpreferencemodel': { - 'external_codename': 'settings.change', - }, - 'federation.change_library': { - 'external_codename': 'federation.manage', - }, - } - privacy_level = fields.get_privacy_field() # Unfortunately, Subsonic API assumes a MD5/password authentication @@ -52,12 +45,24 @@ class User(AbstractUser): subsonic_api_token = models.CharField( blank=True, null=True, max_length=255) + # permissions + permission_federation = models.BooleanField(default=False) + permission_library = models.BooleanField(default=False) + permission_settings = models.BooleanField(default=False) + def __str__(self): return self.username - def add_permission(self, codename): - p = Permission.objects.get(codename=codename) - self.user_permissions.add(p) + def get_permissions(self): + perms = {} + for p in PERMISSIONS: + v = self.is_superuser or getattr(self, 'permission_{}'.format(p)) + perms[p] = v + return perms + + def has_permissions(self, *perms): + permissions = self.get_permissions() + return all([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 new file mode 100644 index 000000000..2ff49ff3f --- /dev/null +++ b/api/funkwhale_api/users/permissions.py @@ -0,0 +1,19 @@ +from rest_framework.permissions import BasePermission + + +class HasUserPermission(BasePermission): + """ + Ensure the request user has the proper permissions. + + Usage: + + class MyView(APIView): + permission_classes = [HasUserPermission] + required_permissions = ['federation'] + """ + def has_permission(self, request, view): + if not hasattr(request, 'user') or not request.user: + return False + if request.user.is_anonymous: + return False + return request.user.has_permissions(*view.required_permissions) diff --git a/api/tests/users/test_models.py b/api/tests/users/test_models.py index c7cd12e9e..49199e0a7 100644 --- a/api/tests/users/test_models.py +++ b/api/tests/users/test_models.py @@ -1,3 +1,7 @@ +import pytest + +from funkwhale_api.users import models + def test__str__(factories): user = factories['users.User'](username='hello') @@ -16,3 +20,33 @@ def test_changing_password_updates_subsonic_api_token(factories): assert user.subsonic_api_token is not None assert user.subsonic_api_token != 'test' + + +def test_get_permissions_superuser(factories): + user = factories['users.User'](is_superuser=True) + + perms = user.get_permissions() + for p in models.PERMISSIONS: + assert perms[p] is True + + +def test_get_permissions_regular(factories): + user = factories['users.User'](permission_library=True) + + perms = user.get_permissions() + for p in models.PERMISSIONS: + if p == 'library': + assert perms[p] is True + else: + assert perms[p] is False + + +@pytest.mark.parametrize('args,perms,expected', [ + ({'is_superuser': True}, ['federation', 'library'], True), + ({'is_superuser': False}, ['federation'], False), + ({'permission_library': True}, ['library'], True), + ({'permission_library': True}, ['library', 'federation'], False), +]) +def test_has_permissions(args, perms, expected, factories): + user = factories['users.User'](**args) + assert user.has_permissions(*perms) is expected diff --git a/api/tests/users/test_permissions.py b/api/tests/users/test_permissions.py new file mode 100644 index 000000000..1564c761d --- /dev/null +++ b/api/tests/users/test_permissions.py @@ -0,0 +1,56 @@ +import pytest +from rest_framework.views import APIView + +from funkwhale_api.users import permissions + + +def test_has_user_permission_no_user(api_request): + view = APIView.as_view() + permission = permissions.HasUserPermission() + request = api_request.get('/') + assert permission.has_permission(request, view) is False + + +def test_has_user_permission_anonymous(anonymous_user, api_request): + view = APIView.as_view() + permission = permissions.HasUserPermission() + request = api_request.get('/') + setattr(request, 'user', anonymous_user) + assert permission.has_permission(request, view) is False + + +@pytest.mark.parametrize('value', [True, False]) +def test_has_user_permission_logged_in_single(value, factories, api_request): + user = factories['users.User'](permission_federation=value) + + class View(APIView): + required_permissions = ['federation'] + 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') == value + + +@pytest.mark.parametrize('federation,library,expected', [ + (True, False, False), + (False, True, False), + (False, False, False), + (True, True, True), +]) +def test_has_user_permission_logged_in_single( + federation, library, expected, factories, api_request): + user = factories['users.User']( + permission_federation=federation, + permission_library=library, + ) + + class View(APIView): + required_permissions = ['federation', 'library'] + 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 From 6fc4275b681ac65e1ebd289b55e99856408af12f Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 18 May 2018 18:48:46 +0200 Subject: [PATCH 2/6] 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' From 4ce6715dc7a3cf6946dc38b12e02790a9424d2fa Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 18 May 2018 19:09:47 +0200 Subject: [PATCH 3/6] See #152: updated front-end to use new permissions --- front/src/components/About.vue | 2 +- front/src/components/Sidebar.vue | 14 +++++++------- front/src/components/library/Library.vue | 4 ++-- front/src/components/requests/Card.vue | 2 +- front/src/store/auth.js | 2 +- front/test/unit/specs/store/auth.spec.js | 4 +--- 6 files changed, 13 insertions(+), 15 deletions(-) diff --git a/front/src/components/About.vue b/front/src/components/About.vue index b0ae67ef7..59c8411ac 100644 --- a/front/src/components/About.vue +++ b/front/src/components/About.vue @@ -15,7 +15,7 @@

{{ $t('Edit instance info') }} diff --git a/front/src/components/Sidebar.vue b/front/src/components/Sidebar.vue index 9fbc5605c..9f3134c2a 100644 --- a/front/src/components/Sidebar.vue +++ b/front/src/components/Sidebar.vue @@ -60,7 +60,7 @@
{{ $t('Import requests') }}
{{ $t('Federation') }}
{{ $t('Settings') }} @@ -192,8 +192,8 @@ export default { }), showAdmin () { let adminPermissions = [ - this.$store.state.auth.availablePermissions['federation.manage'], - this.$store.state.auth.availablePermissions['import.launch'] + this.$store.state.auth.availablePermissions['federation'], + this.$store.state.auth.availablePermissions['library'] ] return adminPermissions.filter(e => { return e @@ -209,7 +209,7 @@ export default { this.fetchFederationImportRequestsCount() }, fetchFederationNotificationsCount () { - if (!this.$store.state.auth.availablePermissions['federation.manage']) { + if (!this.$store.state.auth.availablePermissions['federation']) { return } let self = this @@ -218,7 +218,7 @@ export default { }) }, fetchFederationImportRequestsCount () { - if (!this.$store.state.auth.availablePermissions['import.launch']) { + if (!this.$store.state.auth.availablePermissions['library']) { return } let self = this diff --git a/front/src/components/library/Library.vue b/front/src/components/library/Library.vue index 820d60a54..e360ccb1c 100644 --- a/front/src/components/library/Library.vue +++ b/front/src/components/library/Library.vue @@ -13,10 +13,10 @@ exact> - + - +
diff --git a/front/src/components/requests/Card.vue b/front/src/components/requests/Card.vue index 9d0bf0b77..7743bb6d4 100644 --- a/front/src/components/requests/Card.vue +++ b/front/src/components/requests/Card.vue @@ -22,7 +22,7 @@
diff --git a/front/src/store/auth.js b/front/src/store/auth.js index 68a15090b..87af081d2 100644 --- a/front/src/store/auth.js +++ b/front/src/store/auth.js @@ -112,7 +112,7 @@ export default { dispatch('playlists/fetchOwn', null, {root: true}) Object.keys(data.permissions).forEach(function (key) { // this makes it easier to check for permissions in templates - commit('permission', {key, status: data.permissions[String(key)].status}) + commit('permission', {key, status: data.permissions[String(key)]}) }) return response.data }, (response) => { diff --git a/front/test/unit/specs/store/auth.spec.js b/front/test/unit/specs/store/auth.spec.js index 3d175e9f9..46901cd16 100644 --- a/front/test/unit/specs/store/auth.spec.js +++ b/front/test/unit/specs/store/auth.spec.js @@ -164,9 +164,7 @@ describe('store/auth', () => { const profile = { username: 'bob', permissions: { - admin: { - status: true - } + admin: true } } moxios.stubRequest('users/users/me/', { From a57d975183e1fc3e73d3ed8b6de145c1697e8890 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 18 May 2018 19:18:40 +0200 Subject: [PATCH 4/6] See #152: updated admin interface --- api/funkwhale_api/users/admin.py | 17 ++++++++++++++++- api/funkwhale_api/users/models.py | 14 +++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/api/funkwhale_api/users/admin.py b/api/funkwhale_api/users/admin.py index 89b67d3df..c772603e9 100644 --- a/api/funkwhale_api/users/admin.py +++ b/api/funkwhale_api/users/admin.py @@ -5,6 +5,7 @@ from django import forms from django.contrib import admin from django.contrib.auth.admin import UserAdmin as AuthUserAdmin from django.contrib.auth.forms import UserChangeForm, UserCreationForm +from django.utils.translation import ugettext_lazy as _ from .models import User @@ -41,8 +42,22 @@ class UserAdmin(AuthUserAdmin): 'email', 'date_joined', 'last_login', - 'privacy_level', + 'is_staff', + 'is_superuser', ] list_filter = [ + 'is_superuser', + 'is_staff', 'privacy_level', + 'permission_settings', + 'permission_library', + 'permission_federation', ] + + fieldsets = ( + (None, {'fields': ('username', 'password', 'privacy_level')}), + (_('Personal info'), {'fields': ('first_name', 'last_name', 'email')}), + (_('Permissions'), {'fields': ('is_active', 'is_staff', 'is_superuser', + 'permission_library', 'permission_settings', 'permission_federation')}), + (_('Important dates'), {'fields': ('last_login', 'date_joined')}), + ) diff --git a/api/funkwhale_api/users/models.py b/api/funkwhale_api/users/models.py index 1de23092a..c16cd62b3 100644 --- a/api/funkwhale_api/users/models.py +++ b/api/funkwhale_api/users/models.py @@ -46,9 +46,17 @@ class User(AbstractUser): blank=True, null=True, max_length=255) # permissions - permission_federation = models.BooleanField(default=False) - permission_library = models.BooleanField(default=False) - permission_settings = models.BooleanField(default=False) + permission_federation = models.BooleanField( + 'Manage library federation', + help_text='Follow other instances, accept/deny library follow requests...', + default=False) + permission_library = models.BooleanField( + 'Manage library', + help_text='Import new content, manage existing content', + default=False) + permission_settings = models.BooleanField( + 'Manage instance-level settings', + default=False) def __str__(self): return self.username From ac7db737857c1a070574174db4b90450a380a331 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 18 May 2018 21:19:20 +0200 Subject: [PATCH 5/6] See #152: added management command to execute one-time migration scripts --- .../common/management/__init__.py | 0 .../common/management/commands/__init__.py | 0 .../common/management/commands/script.py | 66 +++++++++++++++++++ api/funkwhale_api/common/scripts/__init__.py | 2 + .../django_permissions_to_user_permissions.py | 29 ++++++++ api/funkwhale_api/common/scripts/test.py | 8 +++ api/funkwhale_api/users/admin.py | 17 ++++- api/funkwhale_api/users/factories.py | 28 +++++++- api/tests/common/test_scripts.py | 56 ++++++++++++++++ 9 files changed, 202 insertions(+), 4 deletions(-) create mode 100644 api/funkwhale_api/common/management/__init__.py create mode 100644 api/funkwhale_api/common/management/commands/__init__.py create mode 100644 api/funkwhale_api/common/management/commands/script.py create mode 100644 api/funkwhale_api/common/scripts/__init__.py create mode 100644 api/funkwhale_api/common/scripts/django_permissions_to_user_permissions.py create mode 100644 api/funkwhale_api/common/scripts/test.py create mode 100644 api/tests/common/test_scripts.py diff --git a/api/funkwhale_api/common/management/__init__.py b/api/funkwhale_api/common/management/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/api/funkwhale_api/common/management/commands/__init__.py b/api/funkwhale_api/common/management/commands/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/api/funkwhale_api/common/management/commands/script.py b/api/funkwhale_api/common/management/commands/script.py new file mode 100644 index 000000000..9d26a5836 --- /dev/null +++ b/api/funkwhale_api/common/management/commands/script.py @@ -0,0 +1,66 @@ +from django.core.management.base import BaseCommand, CommandError + +from funkwhale_api.common import scripts + + +class Command(BaseCommand): + help = 'Run a specific script from funkwhale_api/common/scripts/' + + def add_arguments(self, parser): + parser.add_argument('script_name', nargs='?', type=str) + parser.add_argument( + '--noinput', '--no-input', action='store_false', dest='interactive', + help="Do NOT prompt the user for input of any kind.", + ) + + def handle(self, *args, **options): + name = options['script_name'] + if not name: + self.show_help() + + available_scripts = self.get_scripts() + try: + script = available_scripts[name] + except KeyError: + raise CommandError( + '{} is not a valid script. Run python manage.py script for a ' + 'list of available scripts'.format(name)) + + self.stdout.write('') + if options['interactive']: + message = ( + 'Are you sure you want to execute the script {}?\n\n' + "Type 'yes' to continue, or 'no' to cancel: " + ).format(name) + if input(''.join(message)) != 'yes': + raise CommandError("Script cancelled.") + script['entrypoint'](self, **options) + + def show_help(self): + indentation = 4 + self.stdout.write('') + self.stdout.write('Available scripts:') + self.stdout.write('Launch with: python manage.py ') + available_scripts = self.get_scripts() + for name, script in sorted(available_scripts.items()): + self.stdout.write('') + self.stdout.write(self.style.SUCCESS(name)) + self.stdout.write('') + for line in script['help'].splitlines(): + self.stdout.write('     {}'.format(line)) + self.stdout.write('') + + def get_scripts(self): + available_scripts = [ + k for k in sorted(scripts.__dict__.keys()) + if not k.startswith('__') + ] + data = {} + for name in available_scripts: + module = getattr(scripts, name) + data[name] = { + 'name': name, + 'help': module.__doc__.strip(), + 'entrypoint': module.main + } + return data diff --git a/api/funkwhale_api/common/scripts/__init__.py b/api/funkwhale_api/common/scripts/__init__.py new file mode 100644 index 000000000..4b2d52520 --- /dev/null +++ b/api/funkwhale_api/common/scripts/__init__.py @@ -0,0 +1,2 @@ +from . import django_permissions_to_user_permissions +from . import test diff --git a/api/funkwhale_api/common/scripts/django_permissions_to_user_permissions.py b/api/funkwhale_api/common/scripts/django_permissions_to_user_permissions.py new file mode 100644 index 000000000..1bc971f80 --- /dev/null +++ b/api/funkwhale_api/common/scripts/django_permissions_to_user_permissions.py @@ -0,0 +1,29 @@ +""" +Convert django permissions to user permissions in the database, +following the work done in #152. +""" +from django.db.models import Q +from funkwhale_api.users import models + +from django.contrib.auth.models import Permission + +mapping = { + 'dynamic_preferences.change_globalpreferencemodel': 'settings', + 'music.add_importbatch': 'library', + 'federation.change_library': 'federation', +} + + +def main(command, **kwargs): + for codename, user_permission in sorted(mapping.items()): + app_label, c = codename.split('.') + p = Permission.objects.get( + content_type__app_label=app_label, codename=c) + users = models.User.objects.filter( + Q(groups__permissions=p) | Q(user_permissions=p)).distinct() + total = users.count() + + command.stdout.write('Updating {} users with {} permission...'.format( + total, user_permission + )) + users.update(**{'permission_{}'.format(user_permission): True}) diff --git a/api/funkwhale_api/common/scripts/test.py b/api/funkwhale_api/common/scripts/test.py new file mode 100644 index 000000000..ab401dca4 --- /dev/null +++ b/api/funkwhale_api/common/scripts/test.py @@ -0,0 +1,8 @@ +""" +This is a test script that does nothing. +You can launch it just to check how it works. +""" + + +def main(command, **kwargs): + command.stdout.write('Test script run successfully') diff --git a/api/funkwhale_api/users/admin.py b/api/funkwhale_api/users/admin.py index c772603e9..7e9062a13 100644 --- a/api/funkwhale_api/users/admin.py +++ b/api/funkwhale_api/users/admin.py @@ -57,7 +57,18 @@ class UserAdmin(AuthUserAdmin): fieldsets = ( (None, {'fields': ('username', 'password', 'privacy_level')}), (_('Personal info'), {'fields': ('first_name', 'last_name', 'email')}), - (_('Permissions'), {'fields': ('is_active', 'is_staff', 'is_superuser', - 'permission_library', 'permission_settings', 'permission_federation')}), + (_('Permissions'), { + 'fields': ( + 'is_active', + 'is_staff', + 'is_superuser', + 'permission_library', + 'permission_settings', + 'permission_federation')}), (_('Important dates'), {'fields': ('last_login', 'date_joined')}), - ) + (_('Useless fields'), { + 'fields': ( + 'user_permissions', + 'groups', + )}) + ) diff --git a/api/funkwhale_api/users/factories.py b/api/funkwhale_api/users/factories.py index 12307f7fd..cd28f4407 100644 --- a/api/funkwhale_api/users/factories.py +++ b/api/funkwhale_api/users/factories.py @@ -1,15 +1,41 @@ import factory -from funkwhale_api.factories import registry +from funkwhale_api.factories import registry, ManyToManyFromList from django.contrib.auth.models import Permission +@registry.register +class GroupFactory(factory.django.DjangoModelFactory): + name = factory.Sequence(lambda n: 'group-{0}'.format(n)) + + class Meta: + model = 'auth.Group' + + @factory.post_generation + def perms(self, create, extracted, **kwargs): + if not create: + # Simple build, do nothing. + return + + if extracted: + perms = [ + Permission.objects.get( + content_type__app_label=p.split('.')[0], + codename=p.split('.')[1], + ) + for p in extracted + ] + # A list of permissions were passed in, use them + self.permissions.add(*perms) + + @registry.register class UserFactory(factory.django.DjangoModelFactory): username = factory.Sequence(lambda n: 'user-{0}'.format(n)) email = factory.Sequence(lambda n: 'user-{0}@example.com'.format(n)) password = factory.PostGenerationMethodCall('set_password', 'test') subsonic_api_token = None + groups = ManyToManyFromList('groups') class Meta: model = 'users.User' diff --git a/api/tests/common/test_scripts.py b/api/tests/common/test_scripts.py new file mode 100644 index 000000000..ce478ba04 --- /dev/null +++ b/api/tests/common/test_scripts.py @@ -0,0 +1,56 @@ +import pytest + +from funkwhale_api.common.management.commands import script +from funkwhale_api.common import scripts + + +@pytest.fixture +def command(): + return script.Command() + + +@pytest.mark.parametrize('script_name', [ + 'django_permissions_to_user_permissions', + 'test', +]) +def test_script_command_list(command, script_name, mocker): + mocked = mocker.patch( + 'funkwhale_api.common.scripts.{}.main'.format(script_name)) + + command.handle(script_name=script_name, interactive=False) + + mocked.assert_called_once_with( + command, script_name=script_name, interactive=False) + + +def test_django_permissions_to_user_permissions(factories, command): + group = factories['auth.Group']( + perms=[ + 'federation.change_library' + ] + ) + user1 = factories['users.User']( + perms=[ + 'dynamic_preferences.change_globalpreferencemodel', + 'music.add_importbatch', + ] + ) + user2 = factories['users.User']( + perms=[ + 'music.add_importbatch', + ], + groups=[group] + ) + + scripts.django_permissions_to_user_permissions.main(command) + + user1.refresh_from_db() + user2.refresh_from_db() + + assert user1.permission_settings is True + assert user1.permission_library is True + assert user1.permission_federation is False + + assert user2.permission_settings is False + assert user2.permission_library is True + assert user2.permission_federation is True From aee792ab478dda746feafe69dc0ed963cb48c7b5 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 18 May 2018 22:13:33 +0200 Subject: [PATCH 6/6] Fix #152: changelog and documentation --- changes/changelog.d/152.feature | 32 ++++++++++++++++++++++++++++++++ docs/configuration.rst | 25 +++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 changes/changelog.d/152.feature diff --git a/changes/changelog.d/152.feature b/changes/changelog.d/152.feature new file mode 100644 index 000000000..a10225288 --- /dev/null +++ b/changes/changelog.d/152.feature @@ -0,0 +1,32 @@ +Simpler permission system (#152) + + +Simpler permission system +========================= + +Starting from this release, the permission system is much simpler. Up until now, +we were using Django's built-in permission system, which was working, but also +quite complex to deal with. + +The new implementation relies on simpler logic, which will make integration +on the front-end in upcoming releases faster and easier. + +If you have manually given permissions to users on your instance, +you can migrate those to the new system. + +On docker setups: + +.. code-block:: shell + + docker-compose run --rm api python manage.py script django_permissions_to_user_permissions --no-input + +On non-docker setups: + +.. code-block:: shell + + # in your virtualenv + python api/manage.py script django_permissions_to_user_permissions --no-input + +There is still no dedicated interface to manage user permissions, but you +can use the admin interface at ``/api/admin/users/user/`` for that purpose in +the meantime. diff --git a/docs/configuration.rst b/docs/configuration.rst index b7df2db42..46756bb26 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -117,3 +117,28 @@ Then, the value of :ref:`setting-MUSIC_DIRECTORY_SERVE_PATH` should be On non-docker setup, you don't need to configure this setting. .. note:: This path should not include any trailing slash + +User permissions +---------------- + +Funkwhale's permission model works as follows: + +- Anonymous users cannot do anything unless configured specifically +- Logged-in users can use the application, but cannot do things that affect + the whole instance +- Superusers can do anything + +To make things more granular and allow some delegation of responsability, +superusers can grant specific permissions to specific users. Available +permissions are: + +- **Manage instance-level settings**: users with this permission can edit instance + settings as described in :ref:`instance-settings` +- **Manage library**: users with this permission can import new music in the + instance +- **Manage library federation**: users with this permission can ask to federate with + other instances, and accept/deny federation requests from other intances + +There is no dedicated interface to manage users permissions, but superusers +can login on the Django's admin at ``/api/admin/`` and grant permissions +to users at ``/api/admin/users/user/``.