-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: preserve catalog and staff checks for authZ about-page access #38736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -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 | ||
|
|
@@ -434,29 +436,96 @@ 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 | ||
| return None | ||
|
|
||
| def _about_page_catalog_visibility_error() -> AccessResponse | CatalogVisibilityError: | ||
| """ | ||
| 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) | ||
|
|
||
| def legacy_can_see_about_page() -> AccessResponse | CatalogVisibilityError: | ||
| """ | ||
| Legacy about-page access when AuthZ course authoring is disabled. | ||
|
|
||
| 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). | ||
|
|
||
| Learners, beta testers, and other course-team roles without staff access rely on | ||
| catalog visibility only; they are not checked explicitly here. | ||
|
|
||
| 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
+476
to
+478
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are we catching here that we can't |
||
|
|
||
| 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 | ||
|
|
||
| @function_trace('can_see_about_page') | ||
| def can_see_about_page(): | ||
| return _about_page_catalog_visibility_error() | ||
|
|
||
| def authz_can_see_about_page() -> 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. | ||
| About-page access when AuthZ course authoring is enabled for the course. | ||
|
|
||
| Applies the same course-level and staff checks as legacy_can_see_about_page, | ||
| and additionally grants access to users with COURSES_VIEW_COURSE (including | ||
| legacy Studio read access as a fallback during RBAC migration). | ||
|
|
||
| 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
+499
to
+501
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this change, do we still need |
||
|
|
||
| if _has_staff_access_to_block(user, courselike, courselike.id): | ||
| return ACCESS_GRANTED | ||
|
|
||
| if user_has_course_permission( | ||
| user, | ||
| COURSES_VIEW_COURSE.identifier, | ||
| courselike.id, | ||
| LegacyAuthoringPermission.READ, | ||
| ): | ||
| return ACCESS_GRANTED | ||
|
|
||
| return _about_page_catalog_visibility_error() | ||
|
|
||
| @function_trace("can_see_about_page") | ||
| def can_see_about_page() -> AccessResponse | CatalogVisibilityError: | ||
| """ | ||
| Entry point for about-page visibility checks. | ||
|
|
||
| Routes authenticated users on courses with AuthZ course authoring enabled to | ||
| authz_can_see_about_page; all other callers use legacy_can_see_about_page. | ||
|
|
||
| Both paths grant access via catalog_visibility and course staff bypass. The AuthZ | ||
| path additionally allows users with COURSES_VIEW_COURSE. | ||
| """ | ||
| 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 authz_can_see_about_page() | ||
| return legacy_can_see_about_page() | ||
|
|
||
| checkers = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,11 +22,12 @@ | |
| from enterprise.api.v1.serializers import EnterpriseCustomerSerializer | ||
| from milestones.tests.utils import MilestonesTestCaseMixin | ||
| from opaque_keys.edx.locator import CourseLocator | ||
| from openedx_authz.constants.roles import COURSE_EDITOR | ||
|
|
||
| import lms.djangoapps.courseware.access as access | ||
| import lms.djangoapps.courseware.access_response as access_response | ||
| from common.djangoapps.student.models import CourseEnrollment | ||
| from common.djangoapps.student.roles import CourseCcxCoachRole, CourseStaffRole | ||
| from common.djangoapps.student.roles import CourseCcxCoachRole, CourseLimitedStaffRole, CourseStaffRole | ||
| from common.djangoapps.student.tests.factories import ( | ||
| AdminFactory, | ||
| AnonymousUserFactory, | ||
|
|
@@ -43,6 +44,8 @@ | |
| from lms.djangoapps.courseware.masquerade import CourseMasquerade | ||
| from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase, masquerade_as_group_member | ||
| from lms.djangoapps.courseware.toggles import course_is_invitation_only | ||
| from openedx.core import toggles as core_toggles | ||
| from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin | ||
| from openedx.core.djangoapps.content.course_overviews.models import CourseOverview | ||
| from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory | ||
| from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES | ||
|
|
@@ -1019,3 +1022,109 @@ def test_course_catalog_access_num_queries_enterprise(self, user_attr_name, cour | |
| course_overview = CourseOverview.get_from_id(course.id) | ||
| with self.assertNumQueries(num_queries, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): | ||
| bool(access.has_access(user, 'see_exists', course_overview, course_key=course.id)) | ||
|
|
||
|
|
||
| class AuthzSeeAboutPageAccessTestCase(CourseAuthoringAuthzTestMixin, SharedModuleStoreTestCase): | ||
| """ | ||
| see_about_page access when AuthZ course authoring is enabled for the course. | ||
| """ | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls): | ||
| super().setUpClass() | ||
| cls.course_public = CourseFactory.create( | ||
| catalog_visibility=CATALOG_VISIBILITY_CATALOG_AND_ABOUT, | ||
| course="authzpublic", | ||
| ) | ||
| cls.course_about_only = CourseFactory.create( | ||
| catalog_visibility=CATALOG_VISIBILITY_ABOUT, | ||
| course="authzabout", | ||
| ) | ||
| cls.course_hidden = CourseFactory.create( | ||
| catalog_visibility=CATALOG_VISIBILITY_NONE, | ||
| course="authzhidden", | ||
| ) | ||
|
|
||
| def _see_about_page_response(self, user, course): | ||
| course_overview = CourseOverview.get_from_id(course.id) | ||
| return access.has_access(user, "see_about_page", course_overview, course_key=course.id) | ||
|
|
||
| def test_learner_granted_via_catalog_visibility_both(self): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's good we're adding more coverage here but if the course about visibility is already being covered by other tests I don't think we should add more. What do you think? |
||
| """Learners without AuthZ roles can view the about page when catalog allows it.""" | ||
| response = self._see_about_page_response(self.unauthorized_user, self.course_public) | ||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| def test_learner_granted_via_catalog_visibility_about_only(self): | ||
| """Learners without AuthZ roles can view about-only courses.""" | ||
| response = self._see_about_page_response(self.unauthorized_user, self.course_about_only) | ||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| def test_enrolled_learner_denied_when_catalog_hidden(self): | ||
| """Enrollment alone does not grant about-page access when catalog is hidden.""" | ||
| CourseEnrollmentFactory(user=self.unauthorized_user, course_id=self.course_hidden.id) | ||
|
|
||
| response = self._see_about_page_response(self.unauthorized_user, self.course_hidden) | ||
|
|
||
| self.assertFalse(response) # noqa: PT009 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we drop #noqa and fix the quality issue? |
||
| self.assertIsInstance(response, access_response.CatalogVisibilityError) # noqa: PT009 | ||
|
|
||
| def test_beta_tester_granted_via_catalog_about(self): | ||
| """Beta testers rely on catalog visibility, not AuthZ authoring permissions.""" | ||
| beta_tester = BetaTesterFactory.create(course_key=self.course_about_only.id) | ||
|
|
||
| response = self._see_about_page_response(beta_tester, self.course_about_only) | ||
|
|
||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| def test_course_staff_bypass_when_catalog_hidden(self): | ||
| """Course staff can preview the about page when catalog visibility is none.""" | ||
| course_staff = StaffFactory.create(course_key=self.course_hidden.id) | ||
|
|
||
| response = self._see_about_page_response(course_staff, self.course_hidden) | ||
|
|
||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| def test_limited_staff_bypass_when_catalog_hidden(self): | ||
| """Limited staff inherit staff bypass for about-page access.""" | ||
| limited_staff = UserFactory.create() | ||
| CourseLimitedStaffRole(self.course_hidden.id).add_users(limited_staff) | ||
|
|
||
| response = self._see_about_page_response(limited_staff, self.course_hidden) | ||
|
|
||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| def test_authz_role_grants_access_when_catalog_hidden(self): | ||
| """Users with COURSES_VIEW_COURSE can access hidden about pages.""" | ||
| self.add_user_to_role_in_course(self.unauthorized_user, COURSE_EDITOR.external_key, self.course_hidden.id) | ||
|
|
||
| response = self._see_about_page_response(self.unauthorized_user, self.course_hidden) | ||
|
|
||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| def test_anonymous_user_uses_legacy_path(self): | ||
| """Anonymous users are routed to the legacy path and follow catalog visibility.""" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are we checking for the paths being used here? |
||
| anonymous_user = AnonymousUserFactory.create() | ||
|
|
||
| response = self._see_about_page_response(anonymous_user, self.course_public) | ||
|
|
||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| def test_denied_returns_catalog_visibility_error(self): | ||
| """AuthZ path returns CatalogVisibilityError when all checks fail.""" | ||
| response = self._see_about_page_response(self.unauthorized_user, self.course_hidden) | ||
|
|
||
| self.assertFalse(response) # noqa: PT009 | ||
| self.assertIsInstance(response, access_response.CatalogVisibilityError) # noqa: PT009 | ||
| self.assertEqual(response.error_code, "not_visible_in_catalog") # noqa: PT009 | ||
|
|
||
| def test_legacy_path_when_authz_disabled(self): | ||
| """When AuthZ is off, catalog visibility rules still apply.""" | ||
| with patch.object(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, "is_enabled", return_value=False): | ||
| response = self._see_about_page_response(self.unauthorized_user, self.course_public) | ||
|
|
||
| self.assertTrue(response) # noqa: PT009 | ||
|
|
||
| hidden_response = self._see_about_page_response(self.unauthorized_user, self.course_hidden) | ||
|
|
||
| self.assertFalse(hidden_response) # noqa: PT009 | ||
| self.assertIsInstance(hidden_response, access_response.CatalogVisibilityError) # noqa: PT009 | ||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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_assignmentas well.