From 98e3bb9cfce12abf57f7c5ba5723a21f63ea9321 Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Sat, 16 Jun 2018 14:42:26 +0200
Subject: [PATCH] Fix #311: Ensure radios can only be edited and deleted by
their owners
---
api/funkwhale_api/radios/views.py | 14 +++---
api/tests/radios/test_api.py | 72 +++++++++++++++----------------
changes/changelog.d/311.bugfix | 11 +++++
front/src/views/radios/Detail.vue | 24 ++++++-----
4 files changed, 69 insertions(+), 52 deletions(-)
create mode 100644 changes/changelog.d/311.bugfix
diff --git a/api/funkwhale_api/radios/views.py b/api/funkwhale_api/radios/views.py
index 77df238e..fb2c4d85 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 5f542c88..0ddebe38 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 00000000..f981767c
--- /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 26d8a4d8..0975398b 100644
--- a/front/src/views/radios/Detail.vue
+++ b/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>
- <i class="pencil icon"></i>
- Edit…
- </router-link>
- <dangerous-button class="labeled icon" :action="deleteRadio">
- <i class="trash icon"></i> Delete
- <p slot="modal-header">Do you want to delete the radio "{{ radio.name }}"?</p>
- <p slot="modal-content">This will completely delete this radio and cannot be undone.</p>
- <p slot="modal-confirm">Delete radio</p>
- </dangerous-button>
+ <template v-if="$store.state.auth.username === radio.user.username">
+ <router-link class="ui icon button" :to="{name: 'library.radios.edit', params: {id: radio.id}}" exact>
+ <i class="pencil icon"></i>
+ Edit…
+ </router-link>
+ <dangerous-button class="labeled icon" :action="deleteRadio">
+ <i class="trash icon"></i> Delete
+ <p slot="modal-header">Do you want to delete the radio "{{ radio.name }}"?</p>
+ <p slot="modal-content">This will completely delete this radio and cannot be undone.</p>
+ <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(() => {
--
GitLab