feat: return 403 with descriptive error for catalog visibility restricted courses#38121
Conversation
|
Thanks for the pull request, @Anas12091101! 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. |
61b19c6 to
1318e76
Compare
ormsbee
left a comment
There was a problem hiding this comment.
Minor questions and request. Thank you!
lms/djangoapps/courseware/access.py
Outdated
| Returns whether the given course has the given visibility type | ||
| """ | ||
| return ACCESS_GRANTED if course.catalog_visibility == visibility_type else ACCESS_DENIED | ||
| from lms.djangoapps.courseware.access_response import CatalogVisibilityError |
There was a problem hiding this comment.
[Question]: Why is this import local to the function?
lms/djangoapps/courseware/access.py
Outdated
| """ | ||
| return ACCESS_GRANTED if course.catalog_visibility == visibility_type else ACCESS_DENIED | ||
| from lms.djangoapps.courseware.access_response import CatalogVisibilityError | ||
| return ACCESS_GRANTED if course.catalog_visibility == visibility_type else CatalogVisibilityError() |
There was a problem hiding this comment.
[Question]: What kinds of values can visibility_type be?
There was a problem hiding this comment.
_has_catalog_visibility is only ever called with "both" or "about", never "none". When catalog_visibility="none", neither check matches, so CatalogVisibilityError is returned implicitly.
| return ACCESS_GRANTED | ||
| if _has_staff_access_to_block(user, courselike, courselike.id): | ||
| return ACCESS_GRANTED | ||
| return catalog_response |
There was a problem hiding this comment.
Please put a comment here to explain why this isn't equivalent to returning ACCESS_DENIED (like you did with can_see_about_page().
6b18b50 to
2d02a9e
Compare
Description
Description
When a course has its
catalog_visibilityset to"none", non-staff users attempting to access it via the Learning MFE receive a generic "Course not found" 404 error, which provides no useful information about why access was denied. This change introduces a newCatalogVisibilityErroraccess response type and ensures it propagates through the access control chain, so the API returns a 403 with a meaningful message instead.The core of the fix is in the access control layer. The
_has_catalog_visibility()helper now returns a typedCatalogVisibilityErrorinstead of a bareACCESS_DENIED. Thecan_see_about_page()andcan_see_in_catalog()functions were restructured to preserve this typed error through their logic (the previousor-chain pattern would discard it). A new handler incheck_course_access_with_redirectcatchesCatalogVisibilityErrorand raises aCourseAccessRedirectwith the error attached.On the API side,
CourseHomeMetadataViewnow catchesCourseAccessRedirectexceptions fromcourse_detail()and converts them into a DRFPermissionDeniedresponse (HTTP 403), including theuser_messageanderror_codefrom the access error. This allows frontend clients to display a descriptive message like "This course is not currently accessible. The course team has restricted access to this content." instead of a confusing "Course not found" error.Useful information to include:
Before
After
changes.
Supporting information
Learning MFE PR: openedx/frontend-app-learning#1871
Testing Instructions
"none". Save.http://local.openedx.io:8000/learning/course/course-v1:Org+Course+Run/home.GET /api/course_home/course_metadata/course-v1:Org+Course+Run.Expected (after this change):
"detail": "This course is not currently accessible. The course team has restricted access to this content. Please contact the course team for further assistance.""error_code": "not_visible_in_catalog"Expected (before this change):
"detail": "Course not found.".Log in as a staff/admin user and repeat step 4 or 5.
Set Course Visibility In Catalog back to
"both"and verify the course loads normally for all users.Deadline
"None"
Other information
Include anything else that will help reviewers and consumers understand the change.