feat: add AuthZ permissions for course schedule & details#38213
feat: add AuthZ permissions for course schedule & details#38213dwong2708 wants to merge 6 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
| serializer = CourseDetailsSerializer(updated_data) | ||
| return Response(serializer.data) | ||
|
|
||
| def _classify_update(self, request: Request, course_key: str) -> tuple[bool, bool]: |
There was a problem hiding this comment.
course_key is a CourseKey.
| self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) | ||
|
|
||
| def test_put_authorized_user_can_edit_course_schedule_and_details(self): | ||
| """ |
There was a problem hiding this comment.
Indetation gets off here and the next method, using 12 spaces rather than 8.
|
|
||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
|
|
||
| def test_put_unauthorized_user_can_edit_course_schedule_and_details(self): |
There was a problem hiding this comment.
| def test_put_unauthorized_user_can_edit_course_schedule_and_details(self): | |
| def test_put_unauthorized_user_cannot_edit_course_schedule_and_details(self): |
|
|
||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
|
|
||
| def test_put_unauthorized_user_can_edit_course_schedule(self): |
There was a problem hiding this comment.
| def test_put_unauthorized_user_can_edit_course_schedule(self): | |
| def test_put_unauthorized_user_cannot_edit_course_schedule(self): |
| Only COURSE_STAFF and COURSE_ADMIN can edit schedule related fields. | ||
| """ | ||
| self.add_user_to_role_in_course( | ||
| self.authorized_user, |
There was a problem hiding this comment.
The method name and docstring say unauthorized user and then we make the authorized_user the one that is unauthorized, which is a bit confusing. This also happens in test_put_unauthorized_user_can_edit_course_schedule_and_details and potentially others.
|
|
||
| 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.
| # 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'>
| 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?
| (is_schedule_update, is_details_update) | ||
| """ | ||
| payload = request.data | ||
| schedule_fields = {"start_date", "end_date", "enrollment_start", "enrollment_end"} |
There was a problem hiding this comment.
How about making this into a class attribute and frozenset?
SCHEDULE_FIELDS = frozenset({"start_date", "end_date", "enrollment_start", "enrollment_end"})
|
|
||
| # Normalize/validate schedule fields | ||
| if field in schedule_fields: | ||
| if payload_value is not None and not is_schedule_update: |
There was a problem hiding this comment.
Should we just check if it is a schedule update right away and skip if we already know it is?
if is_schedule_update:
continueWe could also do this in the else clause:
if is_details_update:
continueIf we do that then raise this up to the top of the for loop:
if is_schedule_update and is_details_update:
break
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