From 5d245c8112c4a73388619dd0002b04469c4bf71e Mon Sep 17 00:00:00 2001 From: Taylor Payne Date: Wed, 2 Sep 2026 15:19:45 -0600 Subject: [PATCH] feat: include authz course role assignments in Meilisearch access_ids get_access_ids_for_request built the Meilisearch tenant-token filter from legacy CourseStaffRole/CourseInstructorRole only, so users holding an authz-only course role (course_editor / course_auditor without a legacy role) were excluded from the search access filter. Their courses returned zero results in the global Studio search modal and the Library Updates "Review Content Updates" tab. Add a _get_authz_course_keys helper that unions the user's authz course-role assignments into the access_id filter when AUTHZ_COURSE_AUTHORING_FLAG is enabled, gated per-course so global-off deployments with per-course/org overrides still resolve correctly. --- .../core/djangoapps/content/search/models.py | 59 +++++- .../content/search/tests/test_models.py | 173 ++++++++++++++++++ 2 files changed, 229 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content/search/models.py b/openedx/core/djangoapps/content/search/models.py index d726f1ead057..4207b69d0bc0 100644 --- a/openedx/core/djangoapps/content/search/models.py +++ b/openedx/core/djangoapps/content/search/models.py @@ -2,15 +2,24 @@ from __future__ import annotations -from django.db import models +import logging + +from django.db import DatabaseError, models from django.utils.translation import gettext_lazy as _ +from opaque_keys import InvalidKeyError from opaque_keys.edx.django.models import LearningContextKeyField +from opaque_keys.edx.keys import CourseKey +from openedx_authz.api.data import CourseOverviewData +from openedx_authz.api.users import get_user_role_assignments_per_scope_type from rest_framework.request import Request from common.djangoapps.student.role_helpers import get_course_roles from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole +from openedx.core import toggles as core_toggles from openedx.core.djangoapps.content_libraries.api import get_libraries_for_user +log = logging.getLogger(__name__) + class SearchAccess(models.Model): # noqa: DJ008 """ @@ -46,14 +55,20 @@ def get_access_ids_for_request(request: Request, omit_orgs: list[str] = None) -> omit_orgs = omit_orgs or [] course_roles = get_course_roles(request.user) - course_clause = models.Q(context_key__in=[ + course_keys = set( role.course_id for role in course_roles if ( role.role in [CourseInstructorRole.ROLE, CourseStaffRole.ROLE] and role.org not in omit_orgs ) - ]) + ) + + # When authz is enabled, also include courses where the user has an authz role assignment. + # This ensures authz-only users (editor/auditor without legacy roles) can search their courses. + course_keys.update(_get_authz_course_keys(request.user.username, omit_orgs)) + + course_clause = models.Q(context_key__in=list(course_keys)) libraries = get_libraries_for_user(user=request.user) library_clause = models.Q(context_key__in=[ @@ -69,6 +84,44 @@ def get_access_ids_for_request(request: Request, omit_orgs: list[str] = None) -> ) +def _get_authz_course_keys(username: str, omit_orgs: list[str]) -> set[CourseKey]: + """ + Returns course keys from the user's authz role assignments where the + authz course authoring flag is enabled. + + Queries authz unconditionally, then filters to only courses where the + flag is active (supporting both global enablement and per-course overrides). + + Fails open: if the authz lookup hits a database error, it is logged and an + empty set is returned so that search degrades to legacy-role access rather + than returning a 500. Any other (unexpected) exception propagates. + """ + try: + assignments = get_user_role_assignments_per_scope_type( + user_external_key=username, + scope_types=(CourseOverviewData,), + ) + except DatabaseError as exc: + log.warning( + "Could not load authz role assignments for user %r; " + "falling back to legacy course roles for search access. Error: %s", + username, + exc, + ) + return set() + + course_keys = set() + for assignment in assignments: + try: + course_key = CourseKey.from_string(assignment.scope.external_key) + except InvalidKeyError: + # A non-course scope (e.g. a library) can legitimately appear here; skip it. + continue + if course_key.org not in omit_orgs and core_toggles.AUTHZ_COURSE_AUTHORING_FLAG.is_enabled(course_key): + course_keys.add(course_key) + return course_keys + + class IncrementalIndexCompleted(models.Model): # noqa: DJ008 """ Stores the contex keys of aleady indexed courses and libraries for incremental indexing. diff --git a/openedx/core/djangoapps/content/search/tests/test_models.py b/openedx/core/djangoapps/content/search/tests/test_models.py index ef42a1c04879..184ffb94b4ef 100644 --- a/openedx/core/djangoapps/content/search/tests/test_models.py +++ b/openedx/core/djangoapps/content/search/tests/test_models.py @@ -1,14 +1,20 @@ """Content search model tests""" from __future__ import annotations +from unittest import mock + import ddt +import pytest +from django.db import OperationalError from django.test import RequestFactory from django.utils.crypto import get_random_string +from edx_toggles.toggles.testutils import override_waffle_flag from organizations.models import Organization from common.djangoapps.student.auth import update_org_role from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, OrgInstructorRole, OrgStaffRole from common.djangoapps.student.tests.factories import UserFactory +from openedx.core import toggles as core_toggles from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -23,6 +29,18 @@ get_access_ids_for_request = lambda request: [] +def _fake_authz_assignment(course_key): + """ + Build a stand-in for openedx_authz's RoleAssignmentData that exposes only + the ``scope.external_key`` attribute that ``_get_authz_course_keys`` reads. + + We stub the assignment rather than provision real authz role rows because + the model helper only cares about the scope's external key; the shape of + the authz storage is exercised by openedx-authz's own test suite. + """ + return mock.Mock(scope=mock.Mock(external_key=str(course_key))) + + class StudioSearchTestMixin: """ Sets up user, org, course, library, and access for studio search tests. @@ -245,3 +263,158 @@ def test_no_access_ids_for_request(self): request.user = self.student access_ids = get_access_ids_for_request(request) assert not access_ids + + +@ddt.ddt +@skip_unless_cms +class StudioSearchAuthzAccessTest(StudioSearchTestMixin, SharedModuleStoreTestCase): + """ + Tests that ``get_access_ids_for_request`` includes courses granted through + openedx-authz role assignments, not just legacy CourseStaffRole / + CourseInstructorRole (openedx/openedx-authz#417). + """ + + AUTHZ_PATH = 'openedx.core.djangoapps.content.search.models.get_user_role_assignments_per_scope_type' + + def _create_course(self, course_location): + """Create a SearchAccess row per course so access_ids can resolve.""" + course = super()._create_course(course_location) + SearchAccess.objects.create(context_key=course.id) + return course + + def _create_library(self, org, num): + """Create a SearchAccess row per library so access_ids can resolve.""" + library = super()._create_library(org, num) + SearchAccess.objects.create(context_key=library.key) + return library + + def _authz_only_user(self): + """A user with no legacy course role — access can only come from authz.""" + return UserFactory.create( + username='authz_editor', + email='authz_editor@example.com', + is_staff=False, + password='authz_editor_pass', + ) + + def _course_access_ids(self, course_keys): + """Resolve the SearchAccess ids for the given course keys.""" + return set( + SearchAccess.objects.filter(context_key__in=course_keys).values_list('id', flat=True) + ) + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_authz_only_user_sees_authz_courses(self): + """ + A user with an authz role assignment but no legacy role gets the + matching course access_ids when the flag is enabled. + """ + user = self._authz_only_user() + granted = self.course_user_keys[:2] # first two are CourseKeys (libraries come later) + request = RequestFactory().get('/course') + request.user = user + + with mock.patch( + self.AUTHZ_PATH, + return_value=[_fake_authz_assignment(key) for key in granted], + ): + access_ids = get_access_ids_for_request(request) + + assert set(access_ids) == self._course_access_ids(granted) + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_authz_courses_respect_omit_orgs(self): + """Authz-granted courses in an omitted org are excluded.""" + user = self._authz_only_user() + granted = self.course_user_keys[:2] + request = RequestFactory().get('/course') + request.user = user + + with mock.patch( + self.AUTHZ_PATH, + return_value=[_fake_authz_assignment(key) for key in granted], + ): + access_ids = get_access_ids_for_request(request, omit_orgs=['Org']) + + assert not access_ids + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=False) + def test_authz_courses_excluded_when_flag_off(self): + """With the flag disabled, authz assignments contribute no access_ids.""" + user = self._authz_only_user() + granted = self.course_user_keys[:2] + request = RequestFactory().get('/course') + request.user = user + + with mock.patch( + self.AUTHZ_PATH, + return_value=[_fake_authz_assignment(key) for key in granted], + ): + access_ids = get_access_ids_for_request(request) + + assert not access_ids + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_authz_db_failure_is_swallowed(self): + """ + A database failure in the authz query must not break search; the + request falls back to legacy roles only (here: none, so no access_ids). + """ + user = self._authz_only_user() + request = RequestFactory().get('/course') + request.user = user + + with mock.patch(self.AUTHZ_PATH, side_effect=OperationalError('authz db down')): + access_ids = get_access_ids_for_request(request) + + assert not access_ids + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_authz_unexpected_error_propagates(self): + """ + Only known operational (DatabaseError) failures are swallowed. An + unexpected error is NOT masked — it propagates so real bugs surface. + """ + user = self._authz_only_user() + request = RequestFactory().get('/course') + request.user = user + + with mock.patch(self.AUTHZ_PATH, side_effect=RuntimeError('unexpected')): + with pytest.raises(RuntimeError): + get_access_ids_for_request(request) + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_authz_ignores_unparseable_scope(self): + """A scope external_key that is not a course key is skipped, not fatal.""" + user = self._authz_only_user() + granted = self.course_user_keys[:1] + request = RequestFactory().get('/course') + request.user = user + assignments = [ + _fake_authz_assignment(granted[0]), + _fake_authz_assignment('not-a-course-key'), + ] + + with mock.patch(self.AUTHZ_PATH, return_value=assignments): + access_ids = get_access_ids_for_request(request) + + assert set(access_ids) == self._course_access_ids(granted) + + @override_waffle_flag(core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, active=True) + def test_authz_union_with_legacy_roles_no_duplicates(self): + """ + When a course is granted through BOTH a legacy role and authz, its + access_id appears exactly once. + """ + request = RequestFactory().get('/course') + request.user = self.course_staff # already has legacy CourseStaffRole on course_user_keys + legacy_courses = self.course_user_keys[:2] + + with mock.patch( + self.AUTHZ_PATH, + return_value=[_fake_authz_assignment(key) for key in legacy_courses], + ): + access_ids = get_access_ids_for_request(request) + + assert len(access_ids) == len(set(access_ids)) + assert self._course_access_ids(legacy_courses).issubset(set(access_ids))