feat: add AuthZ permissions for course schedule & details#38213
feat: add AuthZ permissions for course schedule & details#38213dwong2708 wants to merge 7 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @dwong2708! 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. |
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
rodmgwgu
left a comment
There was a problem hiding this comment.
Just some nits and changes needed due to other PRs already merged in master.
Please make sure to rebase from master to make sure nothing breaks.
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
93e2c75 to
fa7ea64
Compare
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_details.py
Show resolved
Hide resolved
|
|
||
| if not is_schedule_update and not is_details_update: | ||
| # No updatable fields provided in the request | ||
| raise ValidationError("No updatable fields provided in the request.") |
There was a problem hiding this comment.
I think technically if a PUT request comes in with the same data as the current data, the endpoint should return a 200 response with the current object state. So, at least in that case, an error wouldn't be appropriate.
There was a problem hiding this comment.
Right — I decided to return 200 for authorized users and 403 for unauthorized users when both send a payload with no changes.
| # Any non-schedule field counts as details update | ||
| current_value = getattr(course_details, field, None) | ||
| if payload_value != current_value: | ||
| is_details_update = True |
There was a problem hiding this comment.
I was testing this a little and found I got to this line under the if statement with these values:
field: 'certificates_display_behavior'
payload_value: 'CertificatesDisplayBehaviors.END'
current_value: <CertificatesDisplayBehaviors.END: 'end'>
There was a problem hiding this comment.
I left a comment in the code about this. It happens because I used the value directly from the GET response, which resulted in an invalid string. I added a note explaining this and updated the test payload with the correct value.
| f"Invalid date format for field {field}: {payload_value}" | ||
| ) from exc | ||
|
|
||
| if payload_value and getattr(course_details, field) != payload_value: |
There was a problem hiding this comment.
Can payload_value be None?
There was a problem hiding this comment.
Yes, the issue is that there’s no prior validation in the endpoint, so I need to take a defensive programming approach in some parts of the code.
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
cms/djangoapps/contentstore/rest_api/v1/views/course_details.py
Outdated
Show resolved
Hide resolved
fa7ea64 to
cfcc287
Compare
mariajgrimaldi
left a comment
There was a problem hiding this comment.
Only one question left! Thank you for all the clarifications :)
| Returns: | ||
| (is_schedule_update, is_details_update) | ||
| """ | ||
| schedule_fields = frozenset({"start_date", "end_date", "enrollment_start", "enrollment_end"}) |
There was a problem hiding this comment.
Should certificate_available_date also be included here? I'm unclear on what counts as schedule and what doesn't. If we're considering everything datetime to be part of the schedule, it might be safer to not have this here hardcoded and derive it from CourseDetailsSerializer. If a new field is added down the rode then we'd be missing it.
There was a problem hiding this comment.
It's supposed to be this: https://docs.openedx.org/en/latest/educators/how-tos/set_up_course/edit_certificate.html#specify-a-different-certificates-available-date, but I always struggle to set it up. This time I:
- Enabled this waffle switch /admin/waffle/switch/ certificates.auto_certificate_generation
- Configured my course to be instructor paced with a valid certificate (not sure if the certificate was necessary)
It's not only a question of whether it's part of the schedule, but also whether the course has an available date, which is part of the course details. I think this would fail, no? (I think this was already discussed here: #38213 (comment))
@rodmgwgu what do you think?
There was a problem hiding this comment.
I think this would be a product question, @gviedma-aulasneo what do you think?

Description
This PR introduces new AuthZ permission checks for the Schedule & Details section in course settings, implemented behind a feature flag.
The work follows the requirements defined in the ticket, covering both read and update operations for course schedule and details.
Scope of Changes
Permissions introduced:
Endpoints updated:
GET /api/contentstore/v1/course_settings/{course_id}/
GET /api/contentstore/v1/course_details/{course_id}/
PUT /api/contentstore/v1/course_details/{course_id}/
All permission checks are gated behind the corresponding feature flag.
Implementation Details
Testing instructions
Verified that:
Running relevant tests manually:
On a cms container (run with tutor dev exec cms bash), do:
Deadline
Verawood
Other information
Closes openedx/openedx-authz#195