From 22de4a98c5213fd99d922c1accc5d8e8eb431ff8 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sun, 18 Nov 2018 23:17:31 +0100 Subject: [PATCH] Fix #616: Fixed inconsistencies in subsonic error responses --- api/funkwhale_api/subsonic/renderers.py | 39 ++++++++++---------- api/funkwhale_api/subsonic/views.py | 5 ++- api/tests/subsonic/test_renderers.py | 48 ++++++++++++++++++++++++- api/tests/subsonic/test_views.py | 14 ++++++++ changes/changelog.d/616.bugfix | 1 + 5 files changed, 84 insertions(+), 23 deletions(-) create mode 100644 changes/changelog.d/616.bugfix diff --git a/api/funkwhale_api/subsonic/renderers.py b/api/funkwhale_api/subsonic/renderers.py index 95b437a55..e4b647051 100644 --- a/api/funkwhale_api/subsonic/renderers.py +++ b/api/funkwhale_api/subsonic/renderers.py @@ -5,23 +5,27 @@ from rest_framework import renderers import funkwhale_api +def structure_payload(data): + payload = { + "status": "ok", + "version": "1.16.0", + "type": "funkwhale", + "funkwhaleVersion": funkwhale_api.__version__, + } + payload.update(data) + if "detail" in payload: + payload["error"] = {"code": 0, "message": payload.pop("detail")} + if "error" in payload: + payload["status"] = "failed" + return payload + + class SubsonicJSONRenderer(renderers.JSONRenderer): def render(self, data, accepted_media_type=None, renderer_context=None): if not data: # when stream view is called, we don't have any data return super().render(data, accepted_media_type, renderer_context) - final = { - "subsonic-response": { - "status": "ok", - "version": "1.16.0", - "type": "funkwhale", - "funkwhaleVersion": funkwhale_api.__version__, - } - } - final["subsonic-response"].update(data) - if "error" in final: - # an error was returned - final["subsonic-response"]["status"] = "failed" + final = {"subsonic-response": structure_payload(data)} return super().render(final, accepted_media_type, renderer_context) @@ -32,15 +36,8 @@ class SubsonicXMLRenderer(renderers.JSONRenderer): if not data: # when stream view is called, we don't have any data return super().render(data, accepted_media_type, renderer_context) - final = { - "xmlns": "http://subsonic.org/restapi", - "status": "ok", - "version": "1.16.0", - } - final.update(data) - if "error" in final: - # an error was returned - final["status"] = "failed" + final = structure_payload(data) + final["xmlns"] = "http://subsonic.org/restapi" tree = dict_to_xml_tree("subsonic-response", final) return b'\n' + ET.tostring( tree, encoding="utf-8" diff --git a/api/funkwhale_api/subsonic/views.py b/api/funkwhale_api/subsonic/views.py index 8aa9c9dbe..3f485bdff 100644 --- a/api/funkwhale_api/subsonic/views.py +++ b/api/funkwhale_api/subsonic/views.py @@ -97,7 +97,10 @@ class SubsonicViewSet(viewsets.GenericViewSet): def handle_exception(self, exc): # subsonic API sends 200 status code with custom error # codes in the payload - mapping = {exceptions.AuthenticationFailed: (40, "Wrong username or password.")} + mapping = { + exceptions.AuthenticationFailed: (40, "Wrong username or password."), + exceptions.NotAuthenticated: (10, "Required parameter is missing."), + } payload = {"status": "failed"} if exc.__class__ in mapping: code, message = mapping[exc.__class__] diff --git a/api/tests/subsonic/test_renderers.py b/api/tests/subsonic/test_renderers.py index 7e977ac45..acd5500e6 100644 --- a/api/tests/subsonic/test_renderers.py +++ b/api/tests/subsonic/test_renderers.py @@ -1,4 +1,5 @@ import json +import pytest import xml.etree.ElementTree as ET import funkwhale_api @@ -6,6 +7,50 @@ import funkwhale_api from funkwhale_api.subsonic import renderers +@pytest.mark.parametrize( + "data,expected", + [ + ( + {"hello": "world"}, + { + "status": "ok", + "version": "1.16.0", + "type": "funkwhale", + "funkwhaleVersion": funkwhale_api.__version__, + "hello": "world", + }, + ), + ( + { + "hello": "world", + "error": {"code": 10, "message": "something went wrong"}, + }, + { + "status": "failed", + "version": "1.16.0", + "type": "funkwhale", + "funkwhaleVersion": funkwhale_api.__version__, + "hello": "world", + "error": {"code": 10, "message": "something went wrong"}, + }, + ), + ( + {"hello": "world", "detail": "something went wrong"}, + { + "status": "failed", + "version": "1.16.0", + "type": "funkwhale", + "funkwhaleVersion": funkwhale_api.__version__, + "hello": "world", + "error": {"code": 0, "message": "something went wrong"}, + }, + ), + ], +) +def test_structure_payload(data, expected): + assert renderers.structure_payload(data) == expected + + def test_json_renderer(): data = {"hello": "world"} expected = { @@ -32,7 +77,8 @@ def test_xml_renderer_dict_to_xml(): def test_xml_renderer(): payload = {"hello": "world"} - expected = b'\n' # noqa + expected = '\n' # noqa + expected = expected.format(funkwhale_api.__version__).encode() renderer = renderers.SubsonicXMLRenderer() rendered = renderer.render(payload) diff --git a/api/tests/subsonic/test_views.py b/api/tests/subsonic/test_views.py index c0ae84073..6be583231 100644 --- a/api/tests/subsonic/test_views.py +++ b/api/tests/subsonic/test_views.py @@ -43,6 +43,20 @@ def test_exception_wrong_credentials(f, db, api_client): assert response.data == expected +@pytest.mark.parametrize("f", ["json"]) +def test_exception_missing_credentials(f, db, api_client): + url = reverse("api:subsonic-get-artists") + response = api_client.get(url) + + expected = { + "status": "failed", + "error": {"code": 10, "message": "Required parameter is missing."}, + } + + assert response.status_code == 200 + assert response.data == expected + + def test_disabled_subsonic(preferences, api_client): preferences["subsonic__enabled"] = False url = reverse("api:subsonic-ping") diff --git a/changes/changelog.d/616.bugfix b/changes/changelog.d/616.bugfix new file mode 100644 index 000000000..fa6db30ee --- /dev/null +++ b/changes/changelog.d/616.bugfix @@ -0,0 +1 @@ +Fixed inconsistencies in subsonic error responses (#616)