Skip to content

Commit 7510e57

Browse files
authored
fix: Course Auditor gets 403 navigating to a course unit (#38986)
* fix: Course Auditor gets 403 navigating to a course unit xblock_outline_handler (the course outline tree) was already migrated to the AuthZ-aware user_has_course_permission(..., COURSES_VIEW_COURSE, ..., LegacyAuthoringPermission.READ) check, so it correctly recognizes AuthZ- native roles that have no legacy equivalent, like course_auditor and course_editor. xblock_container_handler (the unit/container page — what's hit when navigating to a unit) and xblock_view_handler (renders each child block's preview fragment on that page), plus xblock_edit_view, were never migrated the same way: they still called the legacy-only has_studio_read_access directly, which only recognizes roles with a legacy equivalent (staff/instructor/limited_staff). A Course Auditor has none, so get_user_permissions() returned no permissions and these handlers raised PermissionDenied, even though the outline (using the correct pattern) let the same user in. Migrate the three remaining read checks in block.py to the same user_has_course_permission pattern xblock_outline_handler already uses. The xblock_handler/handle_xblock CRUD endpoint was already correctly AuthZ-aware (via _check_xblock_permission) and needed no change. Fixes openedx/openedx-authz#384 * fix: also fix the REST API v1 container view that the Authoring MFE actually uses Manual testing against a real devstack found that the block.py fix alone wasn't enough: the modern Authoring MFE calls the REST API v1 ContainerHandlerView (/api/contentstore/v1/container_handler/...), which still 403'd for course_auditor. That view (and container_handler/container_embed_handler/xblock_edit_view in the legacy views) all route through the shared _get_item_in_course() helper in component.py, which gated on has_course_author_access — a legacy-only *write* check — even though all four callers only need read access to render a view. course_auditor has no legacy role equivalent, so it never had write access and always got PermissionDenied here, regardless of the block.py fix. Migrate _get_item_in_course() to the same user_has_course_permission read check, fixing all four callers (including the REST API v1 view) at their single shared choke point instead of patching each call site. Verified locally end-to-end: mounted this branch into a real devstack, assigned course_auditor to a test user, confirmed the unit page 403'd before this commit and loads correctly after it. Also ran the full test_block.py + test_vertical_block.py suites against the same devstack (186 passed). * fix: sort imports (ruff I001) * refactor: use plain assert instead of self.assertEqual in new tests Per review feedback from @BryanttV on #38986 (same feedback given earlier on #38984/#38980) — new test code in this repo should use plain assert, not unittest-style assertions with # noqa: PT009.
1 parent b13ef66 commit 7510e57

4 files changed

Lines changed: 164 additions & 5 deletions

File tree

cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,18 @@
44

55
from urllib.parse import quote
66

7+
import ddt
78
from django.urls import reverse
89
from edx_toggles.toggles.testutils import override_waffle_flag
10+
from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF
911
from rest_framework import status
1012
from xblock.core import XBlock
1113
from xblock.utils.studio_editable import NestedXBlockSpec, StudioContainerWithNestedXBlocksMixin
1214
from xblock.validation import ValidationMessage
1315

1416
from cms.djangoapps.contentstore.tests.utils import CourseTestCase
17+
from common.djangoapps.student.tests.factories import UserFactory
18+
from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin
1519
from openedx.core.djangoapps.content_libraries.tests import ContentLibrariesRestApiTest
1620
from openedx.core.djangoapps.content_tagging.toggles import DISABLE_TAGGING_FEATURE
1721
from xmodule.modulestore import ModuleStoreEnum # pylint: disable=wrong-import-order
@@ -275,6 +279,39 @@ def test_component_templates_for_non_mixin_xblock(self):
275279
self.assertIn('video', group_types) # noqa: PT009
276280

277281

282+
@ddt.ddt
283+
class ContainerHandlerViewAuthzTest(CourseAuthoringAuthzTestMixin, BaseXBlockContainer):
284+
"""
285+
Regression test for openedx-authz#384: ContainerHandlerView (the endpoint the
286+
Authoring MFE's unit page calls to render a unit) required legacy write access
287+
via _get_item_in_course(), so AuthZ-native roles with no legacy equivalent
288+
(course_auditor, course_editor) got a 403 despite holding COURSES_VIEW_COURSE.
289+
"""
290+
291+
view_name = "container_handler"
292+
293+
@ddt.data(
294+
COURSE_STAFF.external_key,
295+
COURSE_ADMIN.external_key,
296+
COURSE_AUDITOR.external_key,
297+
COURSE_EDITOR.external_key,
298+
)
299+
def test_course_roles_can_view_unit_container(self, role_key):
300+
role_user = UserFactory(password=self.password)
301+
self.add_user_to_role_in_course(role_user, role_key, self.course.id)
302+
303+
self.client.login(username=role_user.username, password=self.password)
304+
response = self.client.get(self.get_reverse_url(self.vertical.location))
305+
306+
assert response.status_code == status.HTTP_200_OK
307+
308+
def test_unauthorized_user_gets_permission_denied(self):
309+
self.client.login(username=self.unauthorized_user.username, password=self.password)
310+
response = self.client.get(self.get_reverse_url(self.vertical.location))
311+
312+
assert response.status_code == status.HTTP_403_FORBIDDEN
313+
314+
278315
class ContainerVerticalViewTest(BaseXBlockContainer):
279316
"""
280317
Unit tests for the ContainerVerticalViewTest.

cms/djangoapps/contentstore/views/block.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,12 @@ def xblock_view_handler(request, usage_key_string, view_name):
142142
the second is the resource description
143143
"""
144144
usage_key = usage_key_with_run(usage_key_string)
145-
if not has_studio_read_access(request.user, usage_key.course_key):
145+
if not user_has_course_permission(
146+
request.user,
147+
COURSES_VIEW_COURSE.identifier,
148+
usage_key.course_key,
149+
LegacyAuthoringPermission.READ,
150+
):
146151
raise PermissionDenied()
147152

148153
accept_header = request.META.get("HTTP_ACCEPT", "application/json")
@@ -299,7 +304,12 @@ def xblock_edit_view(request, usage_key_string):
299304
Allows editing of an XBlock specified by the usage key.
300305
"""
301306
usage_key = usage_key_with_run(usage_key_string)
302-
if not has_studio_read_access(request.user, usage_key.course_key):
307+
if not user_has_course_permission(
308+
request.user,
309+
COURSES_VIEW_COURSE.identifier,
310+
usage_key.course_key,
311+
LegacyAuthoringPermission.READ,
312+
):
303313
raise PermissionDenied()
304314

305315
store = modulestore()
@@ -371,7 +381,12 @@ def xblock_container_handler(request, usage_key_string):
371381
"""
372382
usage_key = usage_key_with_run(usage_key_string)
373383

374-
if not has_studio_read_access(request.user, usage_key.course_key):
384+
if not user_has_course_permission(
385+
request.user,
386+
COURSES_VIEW_COURSE.identifier,
387+
usage_key.course_key,
388+
LegacyAuthoringPermission.READ,
389+
):
375390
raise PermissionDenied()
376391

377392
response_format = request.GET.get("format", "html")

cms/djangoapps/contentstore/views/component.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
from django.views.decorators.http import require_GET
1616
from opaque_keys import InvalidKeyError
1717
from opaque_keys.edx.keys import UsageKey
18+
from openedx_authz.constants.permissions import COURSES_VIEW_COURSE
1819
from xblock.core import XBlock
1920
from xblock.django.request import django_to_webob_request, webob_to_django_response
2021
from xblock.exceptions import NoSuchHandlerError
@@ -28,6 +29,8 @@
2829
from common.djangoapps.student.auth import has_course_author_access
2930
from common.djangoapps.xblock_django.api import authorable_xblocks, disabled_xblocks
3031
from common.djangoapps.xblock_django.models import XBlockStudioConfigurationFlag
32+
from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission
33+
from openedx.core.djangoapps.authz.decorators import user_has_course_permission
3134
from openedx.core.djangoapps.content_tagging.api import get_object_tags
3235
from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration
3336
from openedx.core.lib.xblock_utils import get_aside_from_xblock, is_xblock_aside
@@ -491,7 +494,11 @@ def _get_item_in_course(request, usage_key):
491494
Helper method for getting the old location, containing course,
492495
item, lms_link, and preview_lms_link for a given locator.
493496
494-
Verifies that the caller has permission to access this item.
497+
Verifies that the caller has permission to view this item. All current callers
498+
(container_handler, container_embed_handler, xblock_edit_view, and the REST API v1
499+
ContainerHandlerView) are read-only, so this only requires view access, not write
500+
access — actual mutations are gated separately (e.g. component_handler's own
501+
has_course_author_access check before persisting).
495502
"""
496503

497504
from ..utils import get_lms_link_for_item
@@ -501,7 +508,12 @@ def _get_item_in_course(request, usage_key):
501508

502509
course_key = usage_key.course_key
503510

504-
if not has_course_author_access(request.user, course_key):
511+
if not user_has_course_permission(
512+
request.user,
513+
COURSES_VIEW_COURSE.identifier,
514+
course_key,
515+
LegacyAuthoringPermission.READ,
516+
):
505517
raise PermissionDenied()
506518

507519
course = modulestore().get_course(course_key)

cms/djangoapps/contentstore/views/tests/test_block.py

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3627,6 +3627,101 @@ def test_unauthorized_chapter_outline(self):
36273627
assert resp.status_code == 403
36283628

36293629

3630+
@ddt.ddt
3631+
class TestXBlockContainerAndViewHandlerAuthz(CourseAuthoringAuthzTestMixin, ItemTest):
3632+
"""
3633+
Unit tests for xblock_container_handler and xblock_view_handler authorization.
3634+
3635+
Regression test for openedx-authz#384: navigating to a unit (the container page,
3636+
which in turn renders each child block via the view handler) returned 403 for
3637+
roles like course_auditor that have no legacy role equivalent, because these two
3638+
handlers checked the legacy-only has_studio_read_access instead of the AuthZ-aware
3639+
user_has_course_permission already used by xblock_outline_handler.
3640+
"""
3641+
3642+
def setUp(self):
3643+
super().setUp()
3644+
user_id = self.user.id
3645+
self.chapter = BlockFactory.create(
3646+
parent_location=self.course.location,
3647+
category="chapter",
3648+
display_name="Week 1",
3649+
user_id=user_id,
3650+
)
3651+
self.sequential = BlockFactory.create(
3652+
parent_location=self.chapter.location,
3653+
category="sequential",
3654+
display_name="Lesson 1",
3655+
user_id=user_id,
3656+
)
3657+
self.vertical = BlockFactory.create(
3658+
parent_location=self.sequential.location,
3659+
category="vertical",
3660+
display_name="Unit 1",
3661+
user_id=user_id,
3662+
)
3663+
3664+
@ddt.data(
3665+
COURSE_STAFF.external_key,
3666+
COURSE_ADMIN.external_key,
3667+
COURSE_AUDITOR.external_key,
3668+
COURSE_EDITOR.external_key,
3669+
)
3670+
def test_course_roles_can_view_unit_container(self, role_key):
3671+
"""
3672+
Any role with COURSES_VIEW_COURSE, including the legacy-less course_auditor
3673+
and course_editor roles, can open the unit (container) page.
3674+
"""
3675+
role_user = UserFactory(password=self.password)
3676+
self.add_user_to_role_in_course(role_user, role_key, self.course.id)
3677+
3678+
container_url = reverse_usage_url("xblock_container_handler", self.vertical.location)
3679+
self.client.login(username=role_user.username, password=self.password)
3680+
resp = self.client.get(container_url, HTTP_ACCEPT="application/json")
3681+
3682+
assert resp.status_code == 200
3683+
3684+
@ddt.data(
3685+
COURSE_STAFF.external_key,
3686+
COURSE_ADMIN.external_key,
3687+
COURSE_AUDITOR.external_key,
3688+
COURSE_EDITOR.external_key,
3689+
)
3690+
def test_course_roles_can_render_unit_preview(self, role_key):
3691+
"""
3692+
Any role with COURSES_VIEW_COURSE can render the unit's preview fragment,
3693+
which is what the frontend fetches for each block shown on the unit page.
3694+
"""
3695+
role_user = UserFactory(password=self.password)
3696+
self.add_user_to_role_in_course(role_user, role_key, self.course.id)
3697+
3698+
preview_url = reverse_usage_url(
3699+
"xblock_view_handler", self.vertical.location, {"view_name": "container_preview"}
3700+
)
3701+
self.client.login(username=role_user.username, password=self.password)
3702+
resp = self.client.get(preview_url, HTTP_ACCEPT="application/json")
3703+
3704+
assert resp.status_code == 200
3705+
3706+
def test_unauthorized_user_gets_permission_denied_on_unit_container(self):
3707+
container_url = reverse_usage_url("xblock_container_handler", self.vertical.location)
3708+
3709+
self.client.login(username=self.unauthorized_user.username, password=self.password)
3710+
resp = self.client.get(container_url, HTTP_ACCEPT="application/json")
3711+
3712+
assert resp.status_code == 403
3713+
3714+
def test_unauthorized_user_gets_permission_denied_on_unit_preview(self):
3715+
preview_url = reverse_usage_url(
3716+
"xblock_view_handler", self.vertical.location, {"view_name": "container_preview"}
3717+
)
3718+
3719+
self.client.login(username=self.unauthorized_user.username, password=self.password)
3720+
resp = self.client.get(preview_url, HTTP_ACCEPT="application/json")
3721+
3722+
assert resp.status_code == 403
3723+
3724+
36303725
class TestGetMetadataWithProblemDefaults(ModuleStoreTestCase):
36313726
"""
36323727
Unit tests for _get_metadata_with_problem_defaults.

0 commit comments

Comments
 (0)