Repository navigation
fix: return 400 instead of 500 when Course Live provider is unresolved - #39088
taimoor-ahmed-1 wants to merge 2 commits into
Conversation
`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 <noreply@anthropic.com>
|
Thanks for the pull request, @taimoor-ahmed-1! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
mphilbrick211
added
the
needs reviewer assigned
Faraz32123
left a comment
Faraz32123
left a comment
There was a problem hiding this comment.
Guard's right, but the serializer dereferences provider in two more places, so some payloads still 500. Ran these on your branch:
POST {"free_tier": true}
views.py:140 -> serializers.py:126 validate_free_tier
AttributeError: 'NoneType' object has no attribute 'has_free_tier'
POST {"enabled": true, "provider_type": "not_a_real_provider", "lti_configuration": {...}}
views.py:140 -> serializers.py:40 validate_lti_config
AttributeError: 'NoneType' object has no attribute 'requires_custom_email'
| # `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(): |
There was a problem hiding this comment.
Passing a None provider down just moves the crash — see the two tracebacks in the review. The second one is the shape Studio posts for Zoom; line 127 skips the empty-config injection when provider is None, so the client's block reaches the validator.
Probably simplest to 400 right here when provider is None instead of leaving it to the serializer. Those two payloads would make good tests.
| # 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({ |
There was a problem hiding this comment.
Not your change, but it's the branch you're editing — this refusal falls through to a 200, so Studio reads a rejected save as success. HTTP_400_BAD_REQUEST is imported and unused right here. Fine as a separate PR.
The previous guard only stopped the PII check from dereferencing a None
provider. It then passed that None into the serializer context, where two more
dereferences wait, so the 500 moved rather than went away:
POST {"free_tier": true}
views.py -> serializers.py:126 validate_free_tier
AttributeError: 'NoneType' object has no attribute 'has_free_tier'
POST {"enabled": true, "provider_type": "not_a_real_provider",
"lti_configuration": {...}}
views.py -> serializers.py:40 validate_lti_config
AttributeError: 'NoneType' object has no attribute 'requires_custom_email'
Both run during `is_valid()`, before `_update_course_live_instance` gets to
report the unknown provider, so neither payload ever reached the error it was
supposed to get. The second is the shape Studio posts for Zoom: the view skips
its empty-config injection when the provider is unresolved, so the client's own
`lti_config` block reaches the validator.
Reject the request in the view instead, as soon as the provider fails to
resolve. The response bodies are unchanged for the payloads that already
returned 400 -- a named but unknown provider still gets "Provider type X does
not exist", a missing one still gets the required-field error, and a blank one
still gets the non-blank field error.
With the provider guaranteed non-None past that point, the `provider and`
conjuncts it was threaded through are no longer needed.
Adds a regression test per payload, both verified to fail on the parent commit
at the two lines above.
Description
CourseLiveConfigurationView.postdereferencesproviderbefore checking it, while the very next line already treats it as optional:providerisNonewheneverprovider_typeis missing from the request or names a provider that is not enabled, so those requests raise anAttributeErrorand return 500 on a path the API documents as 400.The existing
test_post_error_messagesdoes cover a missingprovider_type, but it does not catch this: it createsCourseAllowPIISharingInLTIFlagfirst, which makespii_sharing_allowedtrue and short-circuits theandbeforeprovideris ever touched. Without that flag — the default for a course — the dereference happens.With the guard added, both cases fall through to the serializer, which already reports them correctly:
"This field is required."for a missingprovider_type, and"Provider type ... does not exist"for an unknown one.Testing
Two regression tests, both exercising the no-PII-flag path the existing test does not reach:
test_post_missing_provider_type_without_pii_flag— empty payload, no flag → 400 withprovider_type: ["This field is required."]test_post_unknown_provider_type_without_pii_flag— an unrecognisedprovider_type, no flag → 400 with "does not exist"I was not able to run the edx-platform suite locally (these tests need
ModuleStoreTestCaseand the full dev requirements), so I am relying on CI here — happy to adjust if the assertions need tightening.Relationship to #39021
I found this while investigating #39021 (Course Live LTI 1.1 save returning 500 in Studio). It is not confirmed to be that issue's cause and I am not closing it. Two things do not line up: the reporter's first failure was
PATCH /api/course_apps/v1/apps/{course_id}, a different endpoint from the one changed here, and they had selected a provider, soprovidershould not have beenNonefor them.For what it is worth, I did rule out the LTI consumer side of #39021: the Course Live save path (
LtiConfiguration.objects.create(config_store=CONFIG_ON_DB)followed by a save with the LTI 1.1 fields) runs clean against both currentxblock-lti-consumermaster and the 11.4.0 that was released at the time of the report. Diagnosing #39021 properly still needs the server traceback behind those 500s.This change stands on its own regardless: a documented 400 should not be a 500.
🤖 Generated with Claude Code