Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 58 additions & 61 deletions lms/djangoapps/course_api/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@
from rest_framework.request import Request
from rest_framework.test import APIRequestFactory

from common.djangoapps.student.roles import CourseLimitedStaffRole
from common.djangoapps.student.tests.factories import StaffFactory, UserFactory
from lms.djangoapps.courseware.exceptions import CourseAccessRedirect
from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from xmodule.course_block import CATALOG_VISIBILITY_ABOUT, CATALOG_VISIBILITY_NONE
from xmodule.modulestore.exceptions import ItemNotFoundError # pylint: disable=wrong-import-order
from xmodule.modulestore.tests.django_utils import ( # pylint: disable=wrong-import-order
ModuleStoreTestCase,
Expand Down Expand Up @@ -146,98 +149,92 @@ def setUpClass(cls):

cls.course = cls.create_course()
cls.hidden_course = cls.create_course(
course='hidden',
visible_to_staff_only=True
course="hidden",
visible_to_staff_only=True,
catalog_visibility=CATALOG_VISIBILITY_NONE,
)
cls.about_only_course = cls.create_course(
course="aboutonly",
catalog_visibility=CATALOG_VISIBILITY_ABOUT,
)

def test_get_existing_course_as_authorized_user(self):
"""User with COURSE_EDITOR role can access course."""
self.add_user_to_role_in_course(
self.authorized_user,
COURSE_EDITOR.external_key,
self.course.id
)
self.add_user_to_role_in_course(self.authorized_user, COURSE_EDITOR.external_key, self.course.id)

course = self._make_api_call(
self.authorized_user,
self.authorized_user,
self.course.id
)
course = self._make_api_call(self.authorized_user, self.authorized_user, self.course.id)

self.verify_course(course)

def test_get_existing_course_without_authz_role_when_catalog_visible(self):
"""User without AuthZ role can still access when catalog visibility allows."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change? Is it not better to maintain the behavior of not showing the info if the user is unauthorized, even though it is visible in the catalog? - Or is there a view that needs this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need this, we should modify the test name and add more tests to make our intentions explicit (for example, test that if it is visible in the catalog, it is visible for everyone, or other cases). This applies to the modifications on test_user_gains_access_after_role_assignment as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the test since it's no longer valid with the new changes. I followed your suggestion and included more tests to cover all scenarios.

course = self._make_api_call(self.unauthorized_user, self.unauthorized_user, self.course.id)

self.verify_course(course)

def test_get_existing_course_as_unauthorized_user(self):
"""User without role should be denied."""
def test_about_only_catalog_visibility_without_authz_role(self):
"""User without AuthZ role can access when catalog visibility is about-only."""
course = self._make_api_call(self.unauthorized_user, self.unauthorized_user, self.about_only_course.id)

self.verify_course(course, course_id=str(self.about_only_course.id))

def test_hidden_course_denied_without_authz_role(self):
"""User without AuthZ role is denied when catalog visibility is none."""
with pytest.raises(CourseAccessRedirect):
self._make_api_call(
self.unauthorized_user,
self.unauthorized_user,
self.course.id
)
self._make_api_call(self.unauthorized_user, self.unauthorized_user, self.hidden_course.id)

def test_get_nonexistent_course(self):
"""Nonexistent course should raise 404."""
course_key = CourseKey.from_string('edX/toy/nope')
course_key = CourseKey.from_string("edX/toy/nope")

with pytest.raises(Http404):
self._make_api_call(
self.authorized_user,
self.authorized_user,
course_key
)
self._make_api_call(self.authorized_user, self.authorized_user, course_key)

def test_course_staff_bypasses_authz_on_hidden_course(self):
"""Course staff can access a hidden course without an AuthZ role."""
course_staff = StaffFactory.create(course_key=self.hidden_course.id)

course = self._make_api_call(course_staff, course_staff, self.hidden_course.id)

self.verify_course(course, course_id=str(self.hidden_course.id))

def test_limited_staff_bypasses_authz_on_hidden_course(self):
"""Limited course staff can access a hidden course without an AuthZ role."""
limited_staff = UserFactory(password=self.password)
CourseLimitedStaffRole(self.hidden_course.id).add_users(limited_staff)

course = self._make_api_call(limited_staff, limited_staff, self.hidden_course.id)

self.verify_course(course, course_id=str(self.hidden_course.id))

def test_hidden_course_for_staff(self):
"""Staff can access hidden course."""
course = self._make_api_call(
self.staff_user,
self.staff_user,
self.hidden_course.id
)
course = self._make_api_call(self.staff_user, self.staff_user, self.hidden_course.id)

self.verify_course(
course,
course_id='course-v1:edX+hidden+2012_Fall'
)
self.verify_course(course, course_id=str(self.hidden_course.id))

def test_hidden_course_for_staff_as_unauthorized_user(self):
"""
Staff requesting data for another user without permissions
should not bypass visibility rules.
"""
with pytest.raises(CourseAccessRedirect):
self._make_api_call(
self.staff_user,
self.unauthorized_user,
self.hidden_course.id
)
self._make_api_call(self.staff_user, self.unauthorized_user, self.hidden_course.id)

def test_user_gains_access_after_role_assignment(self):
"""User initially denied, then allowed after role assignment."""
"""User denied when catalog is hidden, then allowed after role assignment."""
with pytest.raises(CourseAccessRedirect):
self._make_api_call(
self.unauthorized_user,
self.unauthorized_user,
self.course.id
)
self.add_user_to_role_in_course(
self.unauthorized_user,
COURSE_EDITOR.external_key,
self.course.id
)
course = self._make_api_call(
self.unauthorized_user,
self.unauthorized_user,
self.course.id
)
self.verify_course(course)
self._make_api_call(self.unauthorized_user, self.unauthorized_user, self.hidden_course.id)

self.add_user_to_role_in_course(self.unauthorized_user, COURSE_EDITOR.external_key, self.hidden_course.id)

course = self._make_api_call(self.unauthorized_user, self.unauthorized_user, self.hidden_course.id)

self.verify_course(course, course_id=str(self.hidden_course.id))

def test_staff_access_without_authz_role(self):
"""Staff bypasses AuthZ roles."""
course = self._make_api_call(
self.staff_user,
self.staff_user,
self.course.id
)
course = self._make_api_call(self.staff_user, self.staff_user, self.course.id)

self.verify_course(course)

Expand Down
71 changes: 56 additions & 15 deletions lms/djangoapps/courseware/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException
from lms.djangoapps.ccx.models import CustomCourseForEdX
from lms.djangoapps.courseware.access_response import (
AccessResponse,
CatalogVisibilityError,
IncorrectPartitionGroupError,
MilestoneAccessError,
Expand All @@ -64,6 +65,7 @@
from lms.djangoapps.courseware.toggles import course_is_invitation_only
from lms.djangoapps.mobile_api.models import IgnoreMobileAvailableFlagConfig
from openedx.core import toggles as core_toggles
from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission
from openedx.core.djangoapps.authz.decorators import user_has_course_permission
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from openedx.features.course_duration_limits.access import check_course_expired
Expand Down Expand Up @@ -434,30 +436,69 @@ def can_see_in_catalog():
# can provide a meaningful error message instead of a generic 404.
return catalog_response

def legacy_can_see_about_page():
def _about_page_catalog_visibility_access() -> AccessResponse | None:
"""
Shared catalog-visibility gate for about-page access.

Grants access when catalog_visibility is CATALOG_AND_ABOUT or ABOUT only.

Returns ACCESS_GRANTED when either applies, or None when catalog visibility
does not allow the about page (callers fall through to staff/AuthZ checks).
"""
both_response = _has_catalog_visibility(courselike, CATALOG_VISIBILITY_CATALOG_AND_ABOUT)
if both_response:
return ACCESS_GRANTED
about_response = _has_catalog_visibility(courselike, CATALOG_VISIBILITY_ABOUT)
if about_response:
return ACCESS_GRANTED
if _has_staff_access_to_block(user, courselike, courselike.id):
return ACCESS_GRANTED
# Return the typed CatalogVisibilityError so downstream handlers
# can provide a meaningful error message instead of a generic 404.
return both_response
return None

@function_trace('can_see_about_page')
def can_see_about_page():
def _about_page_catalog_visibility_error() -> AccessResponse | CatalogVisibilityError:
"""
Implements the "can see course about page" logic if a course about page should be visible
In this case we use the catalog_visibility property on the course block
but also allow course staff to see this.
Return the typed CatalogVisibilityError so downstream handlers
can provide a meaningful error message instead of a generic 404.
"""
return _has_catalog_visibility(courselike, CATALOG_VISIBILITY_CATALOG_AND_ABOUT)

@function_trace("can_see_about_page")
def can_see_about_page() -> AccessResponse | CatalogVisibilityError:
"""
if user and not user.is_anonymous and core_toggles.enable_authz_course_authoring(courselike.id):
is_authz_allowed = user_has_course_permission(user, COURSES_VIEW_COURSE.identifier, courselike.id)
return ACCESS_GRANTED if is_authz_allowed else CatalogVisibilityError()
return legacy_can_see_about_page()
Entry point for about-page visibility checks.

Grants access when any of the following is true:
- the course catalog_visibility allows the about page, or
- the user has course staff access (including limited staff via role inheritance), or
- the user is authenticated, AuthZ course authoring is enabled for the course,
and the user has COURSES_VIEW_COURSE (including legacy Studio read access
as a fallback during RBAC migration).

Learners, beta testers, and other course-team roles without staff access rely on
catalog visibility only; they are not checked explicitly here.

AuthZ must not replace catalog visibility or staff bypass; those checks run
first so enrolled learners and beta testers are not blocked by authoring
permissions they do not hold.

Returns CatalogVisibilityError when all checks fail.
"""
catalog_visibility_access = _about_page_catalog_visibility_access()
if catalog_visibility_access:
return catalog_visibility_access
Comment on lines +484 to +486

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, do we still need legacy_can_see_about_page as a fallback?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the current code a bit, reducing code duplication


if _has_staff_access_to_block(user, courselike, courselike.id):
return ACCESS_GRANTED

if (
user
and not user.is_anonymous
and core_toggles.enable_authz_course_authoring(courselike.id)
and user_has_course_permission(
user, COURSES_VIEW_COURSE.identifier, courselike.id, LegacyAuthoringPermission.READ
)
):
return ACCESS_GRANTED

return _about_page_catalog_visibility_error()

checkers = {
'load': can_load,
Expand Down
37 changes: 37 additions & 0 deletions lms/djangoapps/courseware/tests/test_about.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import ddt
import pytz
from crum import set_current_request
from django.conf import settings
from django.test.utils import override_settings
from django.urls import reverse
Expand All @@ -19,6 +20,7 @@
from common.djangoapps.student.tests.factories import CourseEnrollmentAllowedFactory, UserFactory
from common.djangoapps.track.tests import EventTrackingTestCase
from common.djangoapps.util.milestones_helpers import get_prerequisite_courses_display, set_prerequisite_courses
from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin
from openedx.core.djangoapps.models.course_details import CourseDetails
from openedx.features.course_experience import COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, course_home_url
from openedx.features.course_experience.waffle import ENABLE_COURSE_ABOUT_SIDEBAR_HTML
Expand Down Expand Up @@ -223,6 +225,41 @@ def test_about_page_public_view(self, course_visibility):
self.assertContains(resp, "Enroll Now")


class AuthzAboutPageTestCase(
CourseAuthoringAuthzTestMixin,
LoginEnrollmentTestCase,
SharedModuleStoreTestCase,
EventTrackingTestCase,
):
"""
About page HTTP access when AuthZ course authoring is enabled.
"""

@classmethod
def setUpClass(cls):
super().setUpClass()
cls.course_without_about = CourseFactory.create(catalog_visibility=CATALOG_VISIBILITY_NONE)
cls.course_with_about = CourseFactory.create(catalog_visibility=CATALOG_VISIBILITY_ABOUT)
CourseDetails.update_about_item(cls.course_without_about, "overview", "WITHOUT ABOUT", None)
CourseDetails.update_about_item(cls.course_with_about, "overview", "WITH ABOUT", None)

def setUp(self):
super().setUp()
self.addCleanup(set_current_request, None)
assert self.client.login(username=self.unauthorized_user.username, password=self.password)

@override_settings(COURSE_ABOUT_VISIBILITY_PERMISSION="see_about_page")
def test_about_page_honors_catalog_visibility_without_authz_role(self):
"""A learner without AuthZ roles can view catalog-visible about pages."""
url = reverse("about_course", args=[str(self.course_with_about.id)])
resp = self.client.get(url)
self.assertContains(resp, "WITH ABOUT")

url = reverse("about_course", args=[str(self.course_without_about.id)])
resp = self.client.get(url)
self.assertRedirects(resp, reverse("dashboard"), fetch_redirect_response=False)


class AboutTestCaseXML(LoginEnrollmentTestCase, ModuleStoreTestCase):
"""
Tests for the course about page
Expand Down
Loading
Loading