fix: enforce GlobalRestrictedCountry on course access, not just registration - #39079
fix: enforce GlobalRestrictedCountry on course access, not just registration#39079asadali145 wants to merge 4 commits into
Conversation
…stration GlobalRestrictedCountry previously only blocked account registration and profile-country changes - it had no effect on actually accessing a course. The real course-access enforcement (RestrictedCourse + CountryAccessRule) requires a row per course, with no way to block a country across every course at once. Wire GlobalRestrictedCountry into embargo.api.check_course_access() so a listed country blocks every course, regardless of whether it has a RestrictedCourse entry. Per-course CountryAccessRule checks still apply on top for courses that have them, and staff continue to bypass both. Also ensure disable_access_check (a per-course escape hatch) can never bypass a global restriction, and restore a fast path so courses/installs that never use this feature don't pay for the extra lookups. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…block check_course_access() now delegates to an internal _check_course_access() that reports *why* access was denied (blocked_globally) as well as whether it was, instead of redirect_if_blocked() re-deriving the same IP/profile country lookups a second time via a separate helper. This also fixes an ordering bug in that split: a per-course CountryAccessRule match on an earlier IP could return before the profile country was checked against GlobalRestrictedCountry, letting a globally-restricted user slip through disable_access_check just because their IP also happened to fail an unrelated per-course rule first. The global check now runs across every IP and the profile country before any per-course rule is considered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the pull request, @asadali145! 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
Actual query count is 3, not 4: CountryAccessRule.check_country_access caches its per-course allowed-countries list, so the IP-based check and the profile-country check share a single query instead of each paying for their own. Verified by running the full embargo suite (101 passed) after fixing an unrelated missing openedx-learning dependency in the local test environment.
…pass GlobalRestrictedCountry (and CountryAccessRule) previously took effect only through EmbargoMiddleware, which recognizes legacy /course/ and /courses/ URLs. The Learning MFE's course_home_api endpoints (outline, dates, progress, navigation, course_metadata) don't match that URL pattern, so a learner in an embargoed country could view all course content through the MFE even with GlobalRestrictedCountry configured. Wires the check into check_course_access() in courseware/courses.py - the shared choke point every course_home_api view and legacy view already routes through via has_access()/get_course_with_access(). Staff bypass falls out of the existing check_course_access() fallback for free. Verified live against a running devstack: the MFE outline API went from 200 OK (unblocked) to 403 with the embargo error code, while the legacy courseware page continued to redirect as before.
2 participants
Description
GlobalRestrictedCountry(added in #36202 / #36398) only blocks accountregistration and profile-country changes today — it has no effect on course
access. The only mechanism that enforces course access is
RestrictedCourse+CountryAccessRule, which needs a row per course, so there was no way toblock a country from every course at once.
This PR wires
GlobalRestrictedCountryintoembargo.api.check_course_access()so a listed country blocks every course,with or without a
RestrictedCourseentry. Per-courseCountryAccessRulechecks still apply on top where configured, staff still bypass every check,
and a per-course
disable_access_checkoverride can never bypass a globalblock (only a per-course one).
It also closes a related enforcement gap discovered while testing this
change: embargo enforcement previously ran only inside
EmbargoMiddleware,which recognizes legacy
/course///courses/URLs. The Learning MFE'scourse_home_apiendpoints (outline, dates, progress, navigation,course_metadata) don't match that URL pattern, so a learner in an embargoed
country could view all course content through the MFE regardless of any
RestrictedCourse/GlobalRestrictedCountryconfiguration. This PR also wiresthe embargo check into
check_course_access()inlms/djangoapps/courseware/courses.py— the shared choke point everycourse-home BFF view and legacy view already routes through — so both entry
points are covered by one check.
Impact: Operators can now block a country platform-wide via
GlobalRestrictedCountry(Django admin) instead of a row per course.Learners in a globally-restricted country are blocked from every course, not
just ones with a
RestrictedCourserow, and not just on the legacycourseware pages — the Learning MFE is covered too. No change for Course
Authors/Developers, and no behavior change for existing deployments (both
tables are empty by default everywhere).
Supporting information
Motivating use case: mitodl/hq#13170 (OFAC embargo requirement).
Testing instructions
FEATURES['EMBARGO'].GlobalRestrictedCountryin Django Admin (noRestrictedCourserow needed).courseware page, and the Learning MFE (e.g.
/api/course_home/v1/outline/<id>).pytest openedx/core/djangoapps/embargo/ lms/djangoapps/courseware/tests/test_access.py lms/djangoapps/course_home_api/Deadline
None.
Other information
No migration (reuses the existing
GlobalRestrictedCountrytable), nodependency changes, no public API change (
check_course_access()'ssignature/return contract is unchanged).