Fix #311: Ensure radios can only be edited and deleted by their owners

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"])

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(
+    response = logged_in_api_client.post(url, f, format="json")
-        url, json.dumps(f), content_type="application/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(
+    response = logged_in_api_client.post(url, f, format="json")
-        url, json.dumps(f), content_type="application/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(
+    response = logged_in_api_client.post(url, f, format="json")
-        url, json.dumps(f), content_type="application/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):
+def test_user_can_detail_his_radio(logged_in_api_client, factories):
-    radio = factories["radios.Radio"](user=logged_in_client.user)
+    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):
+def test_user_can_edit_his_radio(logged_in_api_client, factories):
-    radio = factories["radios.Radio"](user=logged_in_client.user)
+    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(
+    response = logged_in_api_client.put(
-        url, json.dumps({"name": "new", "config": []}), content_type="application/json"
+        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):
+def test_user_cannot_edit_someone_else_radio(logged_in_api_client, factories):
-    radio = factories["radios.Radio"]()
+    radio = factories["radios.Radio"](is_public=True)
     url = reverse("api:v1:radios:radios-detail", kwargs={"pk": radio.pk})
-    response = logged_in_client.put(
+    response = logged_in_api_client.put(
-        url, json.dumps({"name": "new", "config": []}), content_type="application/json"
+        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"]()

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.

front/src/views/radios/Detail.vue

@@ -17,16 +17,18 @@
         </h2>
         <div class="ui hidden divider"></div>
         <radio-button type="custom" :custom-radio-id="radio.id"></radio-button>
-        <router-link class="ui icon button" :to="{name: 'library.radios.edit', params: {id: radio.id}}" exact>
+        <template v-if="$store.state.auth.username === radio.user.username">
-          <i class="pencil icon"></i>
+          <router-link class="ui icon button" :to="{name: 'library.radios.edit', params: {id: radio.id}}" exact>
-          Edit…
+            <i class="pencil icon"></i>
-        </router-link>
+            Edit…
-        <dangerous-button class="labeled icon" :action="deleteRadio">
+          </router-link>
-          <i class="trash icon"></i> Delete
+          <dangerous-button class="labeled icon" :action="deleteRadio">
-          <p slot="modal-header">Do you want to delete the radio "{{ radio.name }}"?</p>
+            <i class="trash icon"></i> Delete
-          <p slot="modal-content">This will completely delete this radio and cannot be undone.</p>
+            <p slot="modal-header">Do you want to delete the radio "{{ radio.name }}"?</p>
-          <p slot="modal-confirm">Delete radio</p>
+            <p slot="modal-content">This will completely delete this radio and cannot be undone.</p>
-        </dangerous-button>
+            <p slot="modal-confirm">Delete radio</p>
+          </dangerous-button>
+        </template>
       </div>
     </div>
     <div class="ui vertical stripe segment">
@@ -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(() => {