fix: preserve catalog and staff checks for authZ about-page access - #38736
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @BryanttV! 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. |
95a51b7 to
462c398
Compare
462c398 to
28efc11
Compare
28efc11 to
32ef4b1
Compare
| 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.""" |
There was a problem hiding this comment.
How are we checking for the paths being used here?
| 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): |
There was a problem hiding this comment.
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?
5f12ce4 to
9a6ce3d
Compare
9a6ce3d to
7663bb6
Compare
| ): | ||
| return ACCESS_GRANTED | ||
|
|
||
| return _about_page_catalog_visibility_error() |
There was a problem hiding this comment.
It's a bit confusing that we'd have to call _has_catalog_visibility to get an error, does it have to check for permissions again?
There was a problem hiding this comment.
Yes, there's no need to check again. I'm returning the error directly now. Thanks!
|
Can we deploy this into our sandbox to test everything's working correctly? Thanks! You can post here when you run some QA there, so we can check as well. Thank you! |
mariajgrimaldi
left a comment
mariajgrimaldi
left a comment
There was a problem hiding this comment.
I don't want to block this further so I'll leave my approval since the code looks good to me! Thanks
7663bb6 to
dba65df
Compare
dba65df to
0e56ab1
Compare
…urn CatalogVisibilityError directly
|
@mariajgrimaldi, I deployed the changes, ran some tests, and everything is working correctly. You can test it if you'd like. |
github-project-automation
Bot
moved this from Ready for Review
to Done
in Contributions
…penedx#38736) * fix: preserve catalog and staff checks for authZ about-page access * refactor: remove about page catalog visibility error function and return CatalogVisibilityError directly
* feat: add platform glob scope support (#38660) * feat: add platform-wide authz scope support * feat: enhance course listing authorization with global toggle support * chore: upgrade openedx-authz to 1.19.0 * docs: add docstring for mock authorization toggle in course listing tests * fix: add missing org scope support in instructor dashboard (#38721) * feat: enhance role assignment handling for users with org-wide scopes * refactor: update role assertion methods * refactor: replace external_key initialization with build_external_key method * refactor: streamline role assignment by directly using build_external_key method * refactor: update role assignment scope initialization to use ScopeData * refactor: introduce helper function to extract org and course ID from AuthZ scope * refactor: replace has_access with administrative_accesses_to_course_for_user * docs: clarify legacy-only CourseAccessRole query in studio course list * refactor: simplify user role assignment retrieval by using scoped api method * fix: preserve catalog and staff checks for authZ about-page access (#38736) * fix: preserve catalog and staff checks for authZ about-page access * refactor: remove about page catalog visibility error function and return CatalogVisibilityError directly
5 participants
Description
When AuthZ course authoring is enabled for a course,
can_see_about_pagepreviously checked onlyCOURSES_VIEW_COURSEand ignored catalog visibility and staff bypass. Learners, data researchers, beta testers, and course staff could be blocked from the about page even when legacy rules would have allowed access.Supporting information
Testing instructions
Note
RBAC must be enabled globally, at the organization level, or for specific courses.
Modify the Catalog Visibility Setting and Public Course Content from Studio > Advanced Settings. Everything should work according to the configuration (docs)
Screenshots
Before
After
Deadline
Verawood