From 11b6493ae14211eefedb469acd680f762dc08c8f Mon Sep 17 00:00:00 2001 From: Richard Tibbles Date: Fri, 5 Jun 2026 14:08:26 -0700 Subject: [PATCH] Handle network failures in portal and remote channel endpoints Fixes a family of crashes in the views that proxy to the Kolibri Data Portal and Studio, surfaced while removing real network access from the test suite: - NetworkLocationResponseFailure.response is None when the request failed without an HTTP response, so accessing response.status_code or response.json() on it raised AttributeError (500) in the portal register and validate_token actions, the remote channel lookup, and the v2 community library lookup. - A non-404 upstream error in the remote channel lookup fell off the end of the except block, so list returned 200 with a null body and retrieve returned a spurious 404. - NetworkLocationResponseTimeout was uncaught in the remote channel and validate_token actions, and connection failures were uncaught in register, all returning 500. - A non-JSON error body (e.g. a Cloudflare HTML error page) crashed register, and validate_token reflected the raw bytes back to the client. Network-level failures now consistently return 503 with an "offline" status, and 404 from the content server still maps to Http404. The portal endpoints no longer reflect upstream response bodies wholesale: error bodies are reduced to the recognized error constants the frontend matches on (with ALREADY_REGISTERED_FOR_COMMUNITY now mirrored in error_constants.py), the validate_token success response is reduced to the project name the frontend uses, and a missing or non-JSON body is treated as another network-level failure: 503 offline. Co-Authored-By: Claude Opus 4.8 (1M context) --- kolibri/core/api.py | 54 +++++++-- kolibri/core/auth/test/test_api.py | 106 +++++++++++++++++ kolibri/core/content/api.py | 53 ++++----- kolibri/core/content/test/test_content_app.py | 112 ++++++++++++++++++ kolibri/core/error_constants.py | 1 + 5 files changed, 289 insertions(+), 37 deletions(-) diff --git a/kolibri/core/api.py b/kolibri/core/api.py index c9b12628107..9a941fb642e 100644 --- a/kolibri/core/api.py +++ b/kolibri/core/api.py @@ -40,10 +40,11 @@ from rest_framework.status import HTTP_503_SERVICE_UNAVAILABLE from .utils.portal import registerfacility +from kolibri.core import error_constants from kolibri.core.auth.models import Facility from kolibri.core.auth.tasks import enqueue_automatic_kdp_sync from kolibri.core.discovery.utils.network.client import NetworkClient -from kolibri.core.discovery.utils.network.errors import NetworkLocationConnectionFailure +from kolibri.core.discovery.utils.network.errors import NetworkClientError from kolibri.core.discovery.utils.network.errors import NetworkLocationResponseFailure from kolibri.core.utils.serializer_introspection import derive_values_from_serializer from kolibri.core.utils.serializer_introspection import normalize_field_map @@ -51,14 +52,48 @@ from kolibri.utils import conf +# Error constants the portal returns that the frontend matches on - the only +# parts of a portal error body that are reflected to the client. +PORTAL_REFLECTED_ERRORS = ( + error_constants.INVALID_KDP_REGISTRATION_TOKEN, + error_constants.ALREADY_REGISTERED_FOR_COMMUNITY, +) + + +def _portal_error_response(response): + """ + Build the response to return to the client for an error response from the + portal, reflecting only recognized error constants from the body. No + response, or a non-JSON body (e.g. a CDN error page), is treated as a + network-level failure. + """ + offline = Response({"status": "offline"}, status=HTTP_503_SERVICE_UNAVAILABLE) + if response is None: + return offline + try: + data = response.json() + except ValueError: + return offline + if not isinstance(data, list): + data = [] + errors = [ + {"id": error["id"]} + for error in data + if isinstance(error, dict) and error.get("id") in PORTAL_REFLECTED_ERRORS + ] + return Response(errors, status=response.status_code) + + class KolibriDataPortalViewSet(viewsets.ViewSet): @action(detail=False, methods=["post"]) def register(self, request): facility = Facility.objects.get(id=request.data.get("facility_id")) try: response = registerfacility(request.data.get("token"), facility) - except NetworkLocationResponseFailure as e: # bubble up any response error - return Response(e.response.json(), status=e.response.status_code) + except NetworkLocationResponseFailure as e: + return _portal_error_response(e.response) + except NetworkClientError: + return Response({"status": "offline"}, status=HTTP_503_SERVICE_UNAVAILABLE) enqueue_automatic_kdp_sync(facility) return Response(status=response.status_code) @@ -72,17 +107,16 @@ def validate_token(self, request): "portal/api/public/v1/registerfacility/validate_token", params=request.query_params, ) - except NetworkLocationConnectionFailure: - return Response({"status": "offline"}, status=HTTP_503_SERVICE_UNAVAILABLE) except NetworkLocationResponseFailure as e: - # bubble up for any other response error - response = e.response - # handle any invalid json type responses + return _portal_error_response(e.response) + except NetworkClientError: + return Response({"status": "offline"}, status=HTTP_503_SERVICE_UNAVAILABLE) try: data = response.json() except ValueError: - data = response.content - return Response(data, status=response.status_code) + return Response({"status": "offline"}, status=HTTP_503_SERVICE_UNAVAILABLE) + name = data.get("name") if isinstance(data, dict) else None + return Response({"name": name}) class ValuesViewsetOrderingFilter(OrderingFilter): diff --git a/kolibri/core/auth/test/test_api.py b/kolibri/core/auth/test/test_api.py index d61b8dbcf39..413435b531f 100644 --- a/kolibri/core/auth/test/test_api.py +++ b/kolibri/core/auth/test/test_api.py @@ -13,6 +13,7 @@ from django.test.utils import CaptureQueriesContext from django.urls import reverse from django.utils import timezone +from mock import Mock from mock import patch from morango.constants import transfer_stages from morango.constants import transfer_statuses @@ -51,6 +52,10 @@ from kolibri.core.auth.tasks import assign_picture_passwords_to_facility from kolibri.core.device.models import OSUser from kolibri.core.device.utils import set_device_settings +from kolibri.core.discovery.utils.network.client import NetworkClient +from kolibri.core.discovery.utils.network.errors import NetworkLocationConnectionFailure +from kolibri.core.discovery.utils.network.errors import NetworkLocationResponseFailure +from kolibri.core.discovery.utils.network.errors import NetworkLocationResponseTimeout from kolibri.core.tasks.job import Job @@ -3527,6 +3532,107 @@ def test_register_enqueues_automatic_sync( ) mock_enqueue_sync.assert_called_once_with(self.facility) + def _register(self): + return self.client.post( + reverse("kolibri:core:portal-register"), + {"facility_id": self.facility.id, "token": "test-token"}, + format="json", + ) + + @patch( + "kolibri.core.api.registerfacility", + side_effect=NetworkLocationConnectionFailure, + ) + def test_register_offline(self, mock_registerfacility): + response = self._register() + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @patch( + "kolibri.core.api.registerfacility", + side_effect=NetworkLocationResponseFailure(response=None), + ) + def test_register_response_failure_without_response(self, mock_registerfacility): + response = self._register() + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @patch("kolibri.core.api.registerfacility") + def test_register_non_json_error_response(self, mock_registerfacility): + portal_response = Mock(status_code=521, content=b"error") + portal_response.json.side_effect = ValueError + mock_registerfacility.side_effect = NetworkLocationResponseFailure( + response=portal_response + ) + response = self._register() + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @patch("kolibri.core.api.registerfacility") + def test_register_reflects_portal_error_constants(self, mock_registerfacility): + portal_response = Mock(status_code=400) + portal_response.json.return_value = [ + {"id": "ALREADY_REGISTERED_FOR_COMMUNITY", "metadata": {"some": "detail"}}, + {"id": "SOME_UNRECOGNIZED_ERROR"}, + ] + mock_registerfacility.side_effect = NetworkLocationResponseFailure( + response=portal_response + ) + response = self._register() + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json(), [{"id": "ALREADY_REGISTERED_FOR_COMMUNITY"}]) + + def _validate_token(self): + return self.client.get( + reverse("kolibri:core:portal-validate-token"), {"token": "test-token"} + ) + + @patch.object( + NetworkClient, + "get", + side_effect=NetworkLocationResponseFailure(response=None), + ) + def test_validate_token_response_failure_without_response(self, mock_get): + response = self._validate_token() + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @patch.object(NetworkClient, "get", side_effect=NetworkLocationResponseTimeout) + def test_validate_token_timeout(self, mock_get): + response = self._validate_token() + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @patch.object(NetworkClient, "get") + def test_validate_token_non_json_error_response(self, mock_get): + portal_response = Mock(status_code=521, content=b"error") + portal_response.json.side_effect = ValueError + mock_get.side_effect = NetworkLocationResponseFailure(response=portal_response) + response = self._validate_token() + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @patch.object(NetworkClient, "get") + def test_validate_token_reflects_invalid_token_error(self, mock_get): + portal_response = Mock(status_code=400) + portal_response.json.return_value = [{"id": "INVALID_KDP_REGISTRATION_TOKEN"}] + mock_get.side_effect = NetworkLocationResponseFailure(response=portal_response) + response = self._validate_token() + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.json(), [{"id": "INVALID_KDP_REGISTRATION_TOKEN"}]) + + @patch.object(NetworkClient, "get") + def test_validate_token_returns_project_name(self, mock_get): + portal_response = Mock(status_code=200) + portal_response.json.return_value = { + "name": "My Project", + "internal": "detail", + } + mock_get.return_value = portal_response + response = self._validate_token() + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json(), {"name": "My Project"}) + class PicturePasswordSerializerTestCase(APITestCase): databases = "__all__" diff --git a/kolibri/core/content/api.py b/kolibri/core/content/api.py index fb9f3aa1453..2c8d23f5711 100644 --- a/kolibri/core/content/api.py +++ b/kolibri/core/content/api.py @@ -94,6 +94,7 @@ from kolibri.core.device.models import ContentCacheKey from kolibri.core.device.permissions import FromAppContextPermission from kolibri.core.discovery.utils.network.client import NetworkClient +from kolibri.core.discovery.utils.network.errors import NetworkClientError from kolibri.core.discovery.utils.network.errors import NetworkLocationConnectionFailure from kolibri.core.discovery.utils.network.errors import NetworkLocationNotFound from kolibri.core.discovery.utils.network.errors import NetworkLocationResponseFailure @@ -1914,15 +1915,14 @@ def _make_channel_endpoint_request( ) try: resp = client.get(url) - # map the channel list into the format the Kolibri client-side expects - channels = list(map(self._studio_response_to_kolibri_response, resp.json())) - - return channels except NetworkLocationResponseFailure as e: - if e.response.status_code == 404: + if e.response is not None and e.response.status_code == 404: raise Http404( "The requested channel does not exist on the content server" ) + raise + # map the channel list into the format the Kolibri client-side expects + return list(map(self._studio_response_to_kolibri_response, resp.json())) @staticmethod def _get_lang_native_name(code): @@ -1986,7 +1986,7 @@ def list(self, request, *args, **kwargs): channels = self._make_channel_endpoint_request( identifier=token, baseurl=baseurl, keyword=keyword, language=language ) - except NetworkLocationConnectionFailure: + except NetworkClientError: return Response( {"status": "offline"}, status=status.HTTP_503_SERVICE_UNAVAILABLE ) @@ -1998,17 +1998,26 @@ def _retrieve_from_v2(self, channel_id): url = get_v2_channel_lookup_url(channel_id) try: resp = client.get(url) - return Response(self._studio_response_to_kolibri_response(resp.json())) except NetworkLocationResponseFailure as e: - if e.response.status_code == 404: + if e.response is not None and e.response.status_code == 404: raise Http404( "The requested channel does not exist on the content server" ) raise - except NetworkLocationConnectionFailure: - return Response( - {"status": "offline"}, status=status.HTTP_503_SERVICE_UNAVAILABLE + return Response(self._studio_response_to_kolibri_response(resp.json())) + + @staticmethod + def _is_community_channel(channel_id): + try: + library = ( + models.ChannelMetadata.objects.filter(id=channel_id) + .values_list("library", flat=True) + .first() ) + except ValueError: + # an unparseable id cannot correspond to an installed channel + return False + return library == library_constants.COMMUNITY def retrieve(self, request, pk=None): """ @@ -2019,29 +2028,19 @@ def retrieve(self, request, pk=None): language = request.GET.get("language", None) token = request.GET.get("token", None) - # Use v2 only for installed community library channels queried through - # the default Studio base URL (v2 is Studio-specific). Skip when a token - # is provided — token lookups are for draft channels, not community library. - if baseurl is None and token is None: - try: - library = ( - models.ChannelMetadata.objects.filter(id=pk) - .values_list("library", flat=True) - .first() - ) - if library == library_constants.COMMUNITY: - return self._retrieve_from_v2(pk) - except ValueError: - pass - try: + # Use v2 only for installed community library channels queried through + # the default Studio base URL (v2 is Studio-specific). Skip when a token + # is provided — token lookups are for draft channels, not community library. + if baseurl is None and token is None and self._is_community_channel(pk): + return self._retrieve_from_v2(pk) channels = self._make_channel_endpoint_request( identifier=token or pk, baseurl=baseurl, keyword=keyword, language=language, ) - except NetworkLocationConnectionFailure: + except NetworkClientError: return Response( {"status": "offline"}, status=status.HTTP_503_SERVICE_UNAVAILABLE ) diff --git a/kolibri/core/content/test/test_content_app.py b/kolibri/core/content/test/test_content_app.py index b79d14ce806..ea8cdd46919 100644 --- a/kolibri/core/content/test/test_content_app.py +++ b/kolibri/core/content/test/test_content_app.py @@ -37,6 +37,7 @@ from kolibri.core.discovery.utils.network.client import NetworkClient from kolibri.core.discovery.utils.network.errors import NetworkLocationConnectionFailure from kolibri.core.discovery.utils.network.errors import NetworkLocationResponseFailure +from kolibri.core.discovery.utils.network.errors import NetworkLocationResponseTimeout from kolibri.core.lessons.models import Lesson from kolibri.core.lessons.models import LessonAssignment from kolibri.core.logger.models import ContentSessionLog @@ -2654,6 +2655,117 @@ def test_channel_list_offline(self, mock_get): self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) self.assertEqual(response.json()["status"], "offline") + @mock.patch.object( + NetworkClient, + "get", + side_effect=NetworkLocationResponseFailure(response=None), + ) + def test_channel_list_response_failure_without_response(self, mock_get): + response = self.client.get( + reverse("kolibri:core:remotechannel-list"), format="json" + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @mock.patch.object( + NetworkClient, + "get", + side_effect=NetworkLocationResponseFailure(response=None), + ) + def test_channel_retrieve_response_failure_without_response(self, mock_get): + response = self.client.get( + reverse("kolibri:core:remotechannel-detail", kwargs={"pk": "abc"}), + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @mock.patch.object( + NetworkClient, + "get", + side_effect=NetworkLocationResponseFailure(response=mock.Mock(status_code=502)), + ) + def test_channel_list_upstream_error(self, mock_get): + response = self.client.get( + reverse("kolibri:core:remotechannel-list"), format="json" + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @mock.patch.object( + NetworkClient, + "get", + side_effect=NetworkLocationResponseFailure(response=mock.Mock(status_code=502)), + ) + def test_channel_retrieve_upstream_error(self, mock_get): + response = self.client.get( + reverse("kolibri:core:remotechannel-detail", kwargs={"pk": "abc"}), + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @mock.patch.object(NetworkClient, "get", side_effect=NetworkLocationResponseTimeout) + def test_channel_list_timeout(self, mock_get): + response = self.client.get( + reverse("kolibri:core:remotechannel-list"), format="json" + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + @mock.patch.object(NetworkClient, "get", side_effect=NetworkLocationResponseTimeout) + def test_channel_retrieve_timeout(self, mock_get): + response = self.client.get( + reverse("kolibri:core:remotechannel-detail", kwargs={"pk": "abc"}), + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + def _create_community_channel(self): + builder = ChannelBuilder() + builder.insert_into_default_db() + channel = content.ChannelMetadata.objects.get(id=builder.channel["id"]) + channel.library = library_constants.COMMUNITY + channel.save() + return channel.id + + def test_community_channel_retrieve_response_failure_without_response(self): + channel_id = self._create_community_channel() + with mock.patch.object( + NetworkClient, + "get", + side_effect=NetworkLocationResponseFailure(response=None), + ): + response = self.client.get( + reverse( + "kolibri:core:remotechannel-detail", + kwargs={"pk": str(channel_id)}, + ), + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + + def test_community_channel_retrieve_upstream_error(self): + channel_id = self._create_community_channel() + with mock.patch.object( + NetworkClient, + "get", + side_effect=NetworkLocationResponseFailure( + response=mock.Mock(status_code=502) + ), + ): + response = self.client.get( + reverse( + "kolibri:core:remotechannel-detail", + kwargs={"pk": str(channel_id)}, + ), + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE) + self.assertEqual(response.json()["status"], "offline") + def test_community_channel_retrieve_uses_v2_api(self): """retrieve() queries v2 API when the installed channel is COMMUNITY.""" builder = ChannelBuilder() diff --git a/kolibri/core/error_constants.py b/kolibri/core/error_constants.py index e75c2807e01..d66840c92a3 100644 --- a/kolibri/core/error_constants.py +++ b/kolibri/core/error_constants.py @@ -13,6 +13,7 @@ INVALID_NETWORK_LOCATION_FORMAT = "INVALID_NETWORK_LOCATION_FORMAT" NETWORK_LOCATION_NOT_FOUND = "NETWORK_LOCATION_NOT_FOUND" INVALID_KDP_REGISTRATION_TOKEN = "INVALID_KDP_REGISTRATION_TOKEN" +ALREADY_REGISTERED_FOR_COMMUNITY = "ALREADY_REGISTERED_FOR_COMMUNITY" PASSWORD_NOT_SPECIFIED = "PASSWORD_NOT_SPECIFIED" # 401 error constants INVALID_CREDENTIALS = "INVALID_CREDENTIALS"