-
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
Open
BryanttV
wants to merge
5
commits into
openedx:master
Choose a base branch
from
eduNEXT:bav/fix-can-see-about-page-with-authz
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+220
−76
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
9e8f3d1
fix: preserve catalog and staff checks for authZ about-page access
BryanttV 98c59e0
refactor: use native assert instead unittest-style
BryanttV 28b71be
test: enhance course access tests for various catalog visibility scen…
BryanttV b7d8b9a
refactor: streamline authz see_about_page access tests by removing re…
BryanttV 7663bb6
refactor: consolidate about-page access logic and enhance clarity of …
BryanttV File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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,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
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
Contributor
Author
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. 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, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.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.
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.