diff --git a/api/funkwhale_api/radios/views.py b/api/funkwhale_api/radios/views.py index 77df238e1..fb2c4d855 100644 --- a/api/funkwhale_api/radios/views.py +++ b/api/funkwhale_api/radios/views.py @@ -1,9 +1,9 @@ from django.db.models import Q -from django.http import Http404 from rest_framework import mixins, permissions, status, viewsets from rest_framework.decorators import detail_route, list_route from rest_framework.response import Response +from funkwhale_api.common import permissions as common_permissions from funkwhale_api.music.serializers import TrackSerializer from . import filters, filtersets, models, serializers @@ -19,21 +19,25 @@ class RadioViewSet( ): serializer_class = serializers.RadioSerializer - permission_classes = [permissions.IsAuthenticated] + permission_classes = [ + permissions.IsAuthenticated, + common_permissions.OwnerPermission, + ] filter_class = filtersets.RadioFilter + owner_field = "user" + owner_checks = ["write"] def get_queryset(self): + queryset = models.Radio.objects.all() query = Q(is_public=True) if self.request.user.is_authenticated: query |= Q(user=self.request.user) - return models.Radio.objects.filter(query) + return queryset.filter(query) def perform_create(self, serializer): return serializer.save(user=self.request.user) def perform_update(self, serializer): - if serializer.instance.user != self.request.user: - raise Http404 return serializer.save(user=self.request.user) @detail_route(methods=["get"]) diff --git a/api/tests/radios/test_api.py b/api/tests/radios/test_api.py index 5f542c880..0ddebe387 100644 --- a/api/tests/radios/test_api.py +++ b/api/tests/radios/test_api.py @@ -1,24 +1,22 @@ -import json - from django.urls import reverse from funkwhale_api.music.serializers import TrackSerializer from funkwhale_api.radios import filters, serializers -def test_can_list_config_options(logged_in_client): +def test_can_list_config_options(logged_in_api_client): url = reverse("api:v1:radios:radios-filters") - response = logged_in_client.get(url) + response = logged_in_api_client.get(url) assert response.status_code == 200 - payload = json.loads(response.content.decode("utf-8")) + payload = response.data expected = [f for f in filters.registry.values() if f.expose_in_api] assert len(payload) == len(expected) -def test_can_validate_config(logged_in_client, factories): +def test_can_validate_config(logged_in_api_client, factories): artist1 = factories["music.Artist"]() artist2 = factories["music.Artist"]() factories["music.Track"].create_batch(3, artist=artist1) @@ -26,13 +24,11 @@ def test_can_validate_config(logged_in_client, factories): candidates = artist1.tracks.order_by("pk") f = {"filters": [{"type": "artist", "ids": [artist1.pk]}]} url = reverse("api:v1:radios:radios-validate") - response = logged_in_client.post( - url, json.dumps(f), content_type="application/json" - ) + response = logged_in_api_client.post(url, f, format="json") assert response.status_code == 200 - payload = json.loads(response.content.decode("utf-8")) + payload = response.data expected = { "count": candidates.count(), @@ -42,66 +38,62 @@ def test_can_validate_config(logged_in_client, factories): assert payload["filters"][0]["errors"] == [] -def test_can_validate_config_with_wrong_config(logged_in_client, factories): +def test_can_validate_config_with_wrong_config(logged_in_api_client, factories): f = {"filters": [{"type": "artist", "ids": [999]}]} url = reverse("api:v1:radios:radios-validate") - response = logged_in_client.post( - url, json.dumps(f), content_type="application/json" - ) + response = logged_in_api_client.post(url, f, format="json") assert response.status_code == 200 - payload = json.loads(response.content.decode("utf-8")) + payload = response.data expected = {"count": None, "sample": None} assert payload["filters"][0]["candidates"] == expected assert len(payload["filters"][0]["errors"]) == 1 -def test_saving_radio_sets_user(logged_in_client, factories): +def test_saving_radio_sets_user(logged_in_api_client, factories): artist = factories["music.Artist"]() f = {"name": "Test", "config": [{"type": "artist", "ids": [artist.pk]}]} url = reverse("api:v1:radios:radios-list") - response = logged_in_client.post( - url, json.dumps(f), content_type="application/json" - ) + response = logged_in_api_client.post(url, f, format="json") assert response.status_code == 201 - radio = logged_in_client.user.radios.latest("id") + radio = logged_in_api_client.user.radios.latest("id") assert radio.name == "Test" - assert radio.user == logged_in_client.user + assert radio.user == logged_in_api_client.user -def test_user_can_detail_his_radio(logged_in_client, factories): - radio = factories["radios.Radio"](user=logged_in_client.user) +def test_user_can_detail_his_radio(logged_in_api_client, factories): + radio = factories["radios.Radio"](user=logged_in_api_client.user) url = reverse("api:v1:radios:radios-detail", kwargs={"pk": radio.pk}) - response = logged_in_client.get(url) + response = logged_in_api_client.get(url) assert response.status_code == 200 -def test_user_can_detail_public_radio(logged_in_client, factories): +def test_user_can_detail_public_radio(logged_in_api_client, factories): radio = factories["radios.Radio"](is_public=True) url = reverse("api:v1:radios:radios-detail", kwargs={"pk": radio.pk}) - response = logged_in_client.get(url) + response = logged_in_api_client.get(url) assert response.status_code == 200 -def test_user_cannot_detail_someone_else_radio(logged_in_client, factories): +def test_user_cannot_detail_someone_else_radio(logged_in_api_client, factories): radio = factories["radios.Radio"](is_public=False) url = reverse("api:v1:radios:radios-detail", kwargs={"pk": radio.pk}) - response = logged_in_client.get(url) + response = logged_in_api_client.get(url) assert response.status_code == 404 -def test_user_can_edit_his_radio(logged_in_client, factories): - radio = factories["radios.Radio"](user=logged_in_client.user) +def test_user_can_edit_his_radio(logged_in_api_client, factories): + radio = factories["radios.Radio"](user=logged_in_api_client.user) url = reverse("api:v1:radios:radios-detail", kwargs={"pk": radio.pk}) - response = logged_in_client.put( - url, json.dumps({"name": "new", "config": []}), content_type="application/json" + response = logged_in_api_client.put( + url, {"name": "new", "config": []}, format="json" ) radio.refresh_from_db() @@ -109,16 +101,24 @@ def test_user_can_edit_his_radio(logged_in_client, factories): assert radio.name == "new" -def test_user_cannot_edit_someone_else_radio(logged_in_client, factories): - radio = factories["radios.Radio"]() +def test_user_cannot_edit_someone_else_radio(logged_in_api_client, factories): + radio = factories["radios.Radio"](is_public=True) url = reverse("api:v1:radios:radios-detail", kwargs={"pk": radio.pk}) - response = logged_in_client.put( - url, json.dumps({"name": "new", "config": []}), content_type="application/json" + response = logged_in_api_client.put( + url, {"name": "new", "config": []}, format="json" ) assert response.status_code == 404 +def test_user_cannot_delete_someone_else_radio(logged_in_api_client, factories): + radio = factories["radios.Radio"](is_public=True) + url = reverse("api:v1:radios:radios-detail", kwargs={"pk": radio.pk}) + response = logged_in_api_client.delete(url) + + assert response.status_code == 404 + + def test_clean_config_is_called_on_serializer_save(mocker, factories): user = factories["users.User"]() artist = factories["music.Artist"]() diff --git a/changes/changelog.d/311.bugfix b/changes/changelog.d/311.bugfix new file mode 100644 index 000000000..f981767c8 --- /dev/null +++ b/changes/changelog.d/311.bugfix @@ -0,0 +1,11 @@ +Ensure radios can only be edited and deleted by their owners (#311) + +Permission issues on radios +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Because of an error in the way we checked user permissions on radios, +public radios could be deleted by any logged-in user, even if they were not +the owner of the radio. + +We recommend instances owners to upgrade as fast as possible to avoid any abuse +and data loss. diff --git a/front/src/views/radios/Detail.vue b/front/src/views/radios/Detail.vue index 26d8a4d83..0975398b5 100644 --- a/front/src/views/radios/Detail.vue +++ b/front/src/views/radios/Detail.vue @@ -17,16 +17,18 @@ - - - Edit… - - - Delete -

Do you want to delete the radio "{{ radio.name }}"?

-

This will completely delete this radio and cannot be undone.

-

Delete radio

- +
@@ -82,7 +84,7 @@ export default { let url = 'radios/radios/' + this.id + '/' axios.get(url).then((response) => { self.radio = response.data - axios.get(url + 'tracks', {params: {page: this.page}}).then((response) => { + axios.get(url + 'tracks/', {params: {page: this.page}}).then((response) => { this.totalTracks = response.data.count this.tracks = response.data.results }).then(() => {