From 3c5e7073e9e3bac6fe4d2cab434606550b0cfd1f Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Thu, 10 Sep 2026 13:52:05 +0500 Subject: [PATCH] fix: return 400 instead of 500 when Course Live provider is unresolved `CourseLiveConfigurationView.post` dereferences `provider` before checking it, while the very next line already treats it as optional: provider = ProviderManager().get_enabled_providers().get( request.data.get('provider_type', ''), None) if not pii_sharing_allowed and provider.requires_pii_sharing(): ... if provider and not provider.additional_parameters and ... `provider` is None whenever `provider_type` is missing from the request or names a provider that is not enabled, so those requests raise an AttributeError and return 500 on a path the API documents as 400. The existing `test_post_error_messages` covers a missing `provider_type` but does not catch this, because it enables `CourseAllowPIISharingInLTIFlag` first. That makes `pii_sharing_allowed` true, which short-circuits the `and` before `provider` is touched. Without the flag - the default for a course - the dereference happens. With the guard in place both cases fall through to the serializer, which already reports them properly: "This field is required." for a missing `provider_type`, and "Provider type ... does not exist" for an unknown one. Co-Authored-By: Claude Opus 5 --- .../course_live/tests/test_views.py | 32 +++++++++++++++++++ openedx/core/djangoapps/course_live/views.py | 5 ++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/course_live/tests/test_views.py b/openedx/core/djangoapps/course_live/tests/test_views.py index 495dfccc969a..fd77799c3d48 100644 --- a/openedx/core/djangoapps/course_live/tests/test_views.py +++ b/openedx/core/djangoapps/course_live/tests/test_views.py @@ -292,6 +292,38 @@ def test_post_error_messages(self): self.assertEqual(content, expected_data) # noqa: PT009 self.assertEqual(response.status_code, 400) # noqa: PT009 + def test_post_missing_provider_type_without_pii_flag(self): + """ + Test a missing provider_type is reported as a 400, with no PII sharing flag set. + + Distinct from `test_post_error_messages`, which enables `CourseAllowPIISharingInLTIFlag`. + That flag makes `pii_sharing_allowed` true, which short-circuits the `and` in the view's + PII check before the unresolved (None) provider is dereferenced. Without the flag the + provider *is* dereferenced, and the view used to raise an AttributeError -- a 500 on the + documented 400 path. + """ + response = self._post({}) + + self.assertEqual(response.status_code, 400) # noqa: PT009 + content = json.loads(response.content.decode('utf-8')) + self.assertEqual(content, {'provider_type': ['This field is required.']}) # noqa: PT009 + + def test_post_unknown_provider_type_without_pii_flag(self): + """ + Test a provider_type that names no enabled provider is reported as a 400. + + Same unresolved-provider path as above, reached with a provider_type that is present but + does not match an enabled provider. + """ + response = self._post({ + 'enabled': True, + 'provider_type': 'not_a_real_provider', + }) + + self.assertEqual(response.status_code, 400) # noqa: PT009 + content = json.loads(response.content.decode('utf-8')) + self.assertIn('does not exist', str(content)) # noqa: PT009 + def test_non_staff_user_access(self): """ Test non staff user has no access to API diff --git a/openedx/core/djangoapps/course_live/views.py b/openedx/core/djangoapps/course_live/views.py index a25358571f4c..b47e9c72c153 100644 --- a/openedx/core/djangoapps/course_live/views.py +++ b/openedx/core/djangoapps/course_live/views.py @@ -116,7 +116,10 @@ def post(self, request, course_id: str) -> Response: """ pii_sharing_allowed = get_lti_pii_sharing_state_for_course(course_id) provider = ProviderManager().get_enabled_providers().get(request.data.get('provider_type', ''), None) - if not pii_sharing_allowed and provider.requires_pii_sharing(): + # `provider` is None when `provider_type` is missing or names a provider that is not + # enabled. Let the serializer report that as a 400 rather than raising an AttributeError + # here -- the check below already treats `provider` as optional. + if not pii_sharing_allowed and provider and provider.requires_pii_sharing(): return Response({ "pii_sharing_allowed": pii_sharing_allowed, "message": "PII sharing is not allowed on this course"