Skip to content

feat: AuthZ for course authoring compatibility layer #38013

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.

Sign up for GitHub

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
rodmgwgu wants to merge 6 commits into
base: master
Choose a base branch
from
+578 −174
Open

feat: AuthZ for course authoring compatibility layer #38013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
587c3b5
feat: AuthZ for Course Authoring compatibility layer
rodmgwgu Feb 13, 2026
789b6a8
squash!: Add active user validation on _authz_add_users
rodmgwgu Feb 26, 2026
2f673c4
squash!: Attend PR comments
rodmgwgu Mar 3, 2026
6a80465
squash!: Emit events on role changes
rodmgwgu Mar 4, 2026
68aad70
squash!: Correct authz_add_role behavior to match legacy behavior
rodmgwgu Mar 4, 2026
7638824
squash!: Update openedx-authz
rodmgwgu Mar 4, 2026
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
7 changes: 5 additions & 2 deletions cms/djangoapps/contentstore/tests/test_course_listing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,16 @@
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
from openedx.core.djangolib.testing.utils import AUTHZ_TABLES
from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order

TOTAL_COURSES_COUNT = 10
USER_COURSES_COUNT = 1

QUERY_COUNT_TABLE_IGNORELIST = WAFFLE_TABLES + AUTHZ_TABLES


@ddt.ddt
class TestCourseListing(ModuleStoreTestCase):
Expand Down Expand Up @@ -303,10 +306,10 @@ def test_course_listing_performance(self):
courses_list, __ = _accessible_courses_list_from_groups(self.request)
self.assertEqual(len(courses_list), USER_COURSES_COUNT)

with self.assertNumQueries(1, table_ignorelist=WAFFLE_TABLES):
with self.assertNumQueries(2, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST):
_accessible_courses_list_from_groups(self.request)

with self.assertNumQueries(2, table_ignorelist=WAFFLE_TABLES):
with self.assertNumQueries(2, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST):
_accessible_courses_iter_for_tests(self.request)

def test_course_listing_errored_deleted_courses(self):
Expand Down
44 changes: 27 additions & 17 deletions cms/djangoapps/contentstore/views/library.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,24 +73,34 @@ def _user_can_create_library_for_org(user, org=None):
elif user.is_staff:
return True
elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False):
org_filter_params = {}
if org:
org_filter_params['org'] = org
is_course_creator = get_course_creator_status(user) == 'granted'
has_org_staff_role = OrgStaffRole().get_orgs_for_user(user).filter(**org_filter_params).exists()
has_course_staff_role = (
UserBasedRole(user=user, role=CourseStaffRole.ROLE)
.courses_with_role()
.filter(**org_filter_params)
.exists()
)
has_course_admin_role = (
UserBasedRole(user=user, role=CourseInstructorRole.ROLE)
.courses_with_role()
.filter(**org_filter_params)
.exists()
)
return is_course_creator or has_org_staff_role or has_course_staff_role or has_course_admin_role
if is_course_creator:
return True

orgs_with_staff_role = OrgStaffRole().get_orgs_for_user(user)
if org is not None:
orgs_with_staff_role = [user_org for user_org in orgs_with_staff_role if user_org == org]
Comment on lines +80 to +82
Copy link
Contributor

@dwong2708 dwong2708 Mar 4, 2026

Choose a reason for hiding this comment

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

I recommend adding a new method in RoleBase called has_org_for_user(user, org) (return bool).

This avoids leaking implementation details:

  • The legacy path returns a QuerySet and supports efficient DB filtering.
  • The new path may return a different type.

By encapsulating the filtering logic inside has_org_for_user, we keep the interface consistent and allow each implementation to optimize independently.

Copy link
Contributor Author

@rodmgwgu rodmgwgu Mar 4, 2026

Choose a reason for hiding this comment

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

Thanks for the suggestion, it is a good strategy for optimizing this, however I don't think in this specific case this would be important, as this code is related to libraries v1 functionality which to my understanding is deprecated. What do you think @bmtcril should we spend more time optimizing this?

Copy link
Contributor

@bmtcril bmtcril Mar 5, 2026

Choose a reason for hiding this comment

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

I think we will have other uses for it and this code for v1 libraries will be hanging around in the code until at least Xylon so it's probably worth it.

has_org_staff_role = len(orgs_with_staff_role) > 0
if has_org_staff_role:
return True

all_courses_with_staff_role = UserBasedRole(user=user, role=CourseStaffRole.ROLE).courses_with_role()
courses_with_staff_role_on_org = all_courses_with_staff_role
if org is not None:
courses_with_staff_role_on_org = [course for course in all_courses_with_staff_role if course.org == org]
Copy link
Contributor

@dwong2708 dwong2708 Mar 4, 2026

Choose a reason for hiding this comment

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

I suggest following the same approach in UserBasedRole by adding a new method to encapsulate the filtering logic

has_course_staff_role = len(courses_with_staff_role_on_org) > 0
if has_course_staff_role:
return True

all_courses_with_admin_role = UserBasedRole(user=user, role=CourseInstructorRole.ROLE).courses_with_role()
Copy link
Contributor

@dwong2708 dwong2708 Mar 4, 2026

Choose a reason for hiding this comment

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

Same here

courses_with_admin_role_on_org = all_courses_with_admin_role
if org is not None:
courses_with_admin_role_on_org = [course for course in all_courses_with_admin_role if course.org == org]
has_course_admin_role = len(courses_with_admin_role_on_org) > 0
if has_course_admin_role:
return True

return False
else:
# EDUCATOR-1924: DISABLE_LIBRARY_CREATION overrides DISABLE_COURSE_CREATION, if present.
disable_library_creation = settings.FEATURES.get('DISABLE_LIBRARY_CREATION', None)
Expand Down
4 changes: 2 additions & 2 deletions common/djangoapps/student/role_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
)
from openedx.core.lib.cache_utils import request_cached
from common.djangoapps.student.roles import (
CourseAccessRole,
AuthzCompatCourseAccessRole,
CourseBetaTesterRole,
CourseInstructorRole,
CourseStaffRole,
Expand Down Expand Up @@ -66,7 +66,7 @@ def get_role_cache(user: User) -> RoleCache:


@request_cached()
def get_course_roles(user: User) -> list[CourseAccessRole]:
def get_course_roles(user: User) -> list[AuthzCompatCourseAccessRole]:
"""
Returns a list of all course-level roles that this user has.

Expand Down
Loading
Loading