From 9c502f332caf2a14fa14272aee05858de23ebd76 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 16 Oct 2026 13:27:06 -0500 Subject: [PATCH 01/17] chore: temporarily add Instructor Dashboard authz permission constants Declare the permission identifiers used by the Instructor Dashboard API v2. Temporary: the permissions not defined by openedx-authz depend on a separate issue that will define them there. They are declared here only for testing. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/__init__.py | 3 + lms/djangoapps/instructor/authz/constants.py | 56 +++++++++++++++++++ .../instructor/authz/tests/__init__.py | 0 3 files changed, 59 insertions(+) create mode 100644 lms/djangoapps/instructor/authz/__init__.py create mode 100644 lms/djangoapps/instructor/authz/constants.py create mode 100644 lms/djangoapps/instructor/authz/tests/__init__.py diff --git a/lms/djangoapps/instructor/authz/__init__.py b/lms/djangoapps/instructor/authz/__init__.py new file mode 100644 index 000000000000..7e55aa23848e --- /dev/null +++ b/lms/djangoapps/instructor/authz/__init__.py @@ -0,0 +1,3 @@ +""" +AuthZ integration for the Instructor Dashboard. +""" diff --git a/lms/djangoapps/instructor/authz/constants.py b/lms/djangoapps/instructor/authz/constants.py new file mode 100644 index 000000000000..61a6349625fb --- /dev/null +++ b/lms/djangoapps/instructor/authz/constants.py @@ -0,0 +1,56 @@ +""" +AuthZ permission identifiers used by the Instructor Dashboard. + +The ``courses.manage_course_team``, ``courses.view_course_team`` and +``courses.view_grading_settings`` permissions are shared with Authoring and +defined by openedx-authz. + +TEMPORARY: the rest depend on a separate issue that will define them in +openedx-authz. They are declared here only for testing. +""" + +# Course info +COURSES_VIEW_COURSE_INFO = "courses.view_course_info" + +# Enrollments +COURSES_VIEW_ENROLLMENTS = "courses.view_enrollments" +COURSES_ENROLL_LEARNERS = "courses.enroll_learners" +COURSES_UNENROLL_LEARNERS = "courses.unenroll_learners" + +# Course team (shared with Authoring) +COURSES_VIEW_COURSE_TEAM = "courses.view_course_team" +COURSES_MANAGE_COURSE_TEAM = "courses.manage_course_team" + +# Grading +COURSES_VIEW_GRADEBOOK = "courses.view_gradebook" +COURSES_VIEW_GRADING_CONFIGURATION = "courses.view_grading_configuration" +# Shared with Authoring. +COURSES_VIEW_GRADING_SETTINGS = "courses.view_grading_settings" +COURSES_MANAGE_LEARNER_GRADING = "courses.manage_learner_grading" + +# Date extensions +COURSES_VIEW_LEARNER_EXTENSIONS = "courses.view_learner_extensions" +COURSES_MANAGE_LEARNER_EXTENSIONS = "courses.manage_learner_extensions" + +# Cohorts +COURSES_VIEW_COHORTS = "courses.view_cohorts" +COURSES_MANAGE_COHORTS = "courses.manage_cohorts" + +# Every permission whose effective value is returned to the MFE in +# ``authz_permissions``. +INSTRUCTOR_DASHBOARD_PERMISSIONS = ( + COURSES_VIEW_COURSE_INFO, + COURSES_VIEW_ENROLLMENTS, + COURSES_ENROLL_LEARNERS, + COURSES_UNENROLL_LEARNERS, + COURSES_VIEW_COURSE_TEAM, + COURSES_MANAGE_COURSE_TEAM, + COURSES_VIEW_GRADEBOOK, + COURSES_VIEW_GRADING_CONFIGURATION, + COURSES_VIEW_GRADING_SETTINGS, + COURSES_MANAGE_LEARNER_GRADING, + COURSES_VIEW_LEARNER_EXTENSIONS, + COURSES_MANAGE_LEARNER_EXTENSIONS, + COURSES_VIEW_COHORTS, + COURSES_MANAGE_COHORTS, +) diff --git a/lms/djangoapps/instructor/authz/tests/__init__.py b/lms/djangoapps/instructor/authz/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 From 4d71feae3deeb80d05ad99072032ca95e05332f9 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 16 Oct 2026 17:10:45 -0500 Subject: [PATCH 02/17] feat: enforce AuthZ permissions in the Instructor Dashboard API v2 Add the shared infrastructure to check Instructor Dashboard permissions with openedx-authz, driven by the authz.enable_course_authoring flag. * With the flag enabled for the course only AuthZ is consulted, like in Studio. With the flag disabled the legacy check is used, so the previous behavior is preserved. * Add user_has_instructor_permission, get_instructor_authz_permissions and the InstructorAuthzPermission DRF permission class, logging which path (authz or legacy) resolved each check. * Add the legacy equivalent of each permission. * CourseMetadataView resolves the Course Info, Enrollments, Course Team, Grading, Date Extensions and Cohorts tabs through AuthZ, and returns the effective permissions in the new authz_permissions field. The existing fields do not change. * CourseEnrollmentsView requires courses.view_enrollments. * Add test helpers (InstructorAuthzTestMixin) and tests. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 42 ++++ lms/djangoapps/instructor/authz/helpers.py | 111 +++++++++ lms/djangoapps/instructor/authz/legacy.py | 150 ++++++++++++ .../instructor/authz/permissions.py | 74 ++++++ .../instructor/authz/tests/mixins.py | 109 +++++++++ .../instructor/authz/tests/test_helpers.py | 127 ++++++++++ .../authz/tests/test_permissions.py | 106 ++++++++ .../instructor/tests/test_api_v2_authz.py | 227 ++++++++++++++++++ lms/djangoapps/instructor/views/api_v2.py | 28 ++- .../instructor/views/serializers_v2.py | 101 ++++---- 10 files changed, 1024 insertions(+), 51 deletions(-) create mode 100644 lms/djangoapps/instructor/authz/README.rst create mode 100644 lms/djangoapps/instructor/authz/helpers.py create mode 100644 lms/djangoapps/instructor/authz/legacy.py create mode 100644 lms/djangoapps/instructor/authz/permissions.py create mode 100644 lms/djangoapps/instructor/authz/tests/mixins.py create mode 100644 lms/djangoapps/instructor/authz/tests/test_helpers.py create mode 100644 lms/djangoapps/instructor/authz/tests/test_permissions.py create mode 100644 lms/djangoapps/instructor/tests/test_api_v2_authz.py diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst new file mode 100644 index 000000000000..ac832a918d68 --- /dev/null +++ b/lms/djangoapps/instructor/authz/README.rst @@ -0,0 +1,42 @@ +Instructor Dashboard AuthZ +########################## + +Integration of the Instructor Dashboard (``/api/instructor/v2``) with ``openedx-authz``. + +What it contains +**************** + +- ``constants.py``: the permission identifiers. Do not repeat the strings in the code. TEMPORARY: the permissions not defined by openedx-authz depend on a separate issue and are declared here only for testing. +- ``helpers.py``: ``user_has_instructor_permission`` and ``get_instructor_authz_permissions``. +- ``permissions.py``: ``InstructorAuthzPermission``, the DRF permission class for the views. +- ``legacy.py``: the pre-RBAC check equivalent to each permission. +- ``tests/mixins.py``: ``InstructorAuthzTestMixin`` to assign AuthZ roles in tests. + +Behavior +******** + +It is driven by the ``authz.enable_course_authoring`` waffle flag, evaluated per course, with the same semantics as Studio: + +- **Flag enabled:** only AuthZ is consulted. Legacy roles are ignored (openedx-authz already grants the global staff and superuser bypass). +- **Flag disabled:** only the legacy check runs, so the previous behavior is unchanged. + +Every decision is logged with the path that resolved it (``authz`` or ``legacy``) in ``access_path``. + +Migrating a view +**************** + +Replace ``InstructorPermission`` by ``InstructorAuthzPermission`` and declare the AuthZ permission next to the legacy one: + +.. code-block:: python + + from lms.djangoapps.instructor.authz import constants as authz_constants + from lms.djangoapps.instructor.authz.permissions import InstructorAuthzPermission + + class MyView(APIView): + permission_classes = (IsAuthenticated, InstructorAuthzPermission) + # A permission, or a mapping such as {"GET": ..., "POST": ...}. + authz_permission = authz_constants.COURSES_VIEW_ENROLLMENTS + # The legacy bridgekeeper permission. + permission_name = permissions.VIEW_ENROLLMENTS + +When the legacy rule is not a bridgekeeper permission, define ``legacy_permission_check(user, course_key)`` on the view. To check a permission in other code use ``user_has_instructor_permission``. diff --git a/lms/djangoapps/instructor/authz/helpers.py b/lms/djangoapps/instructor/authz/helpers.py new file mode 100644 index 000000000000..77e9f149079a --- /dev/null +++ b/lms/djangoapps/instructor/authz/helpers.py @@ -0,0 +1,111 @@ +""" +Helpers to enforce AuthZ permissions in the Instructor Dashboard. + +When the ``authz.enable_course_authoring`` flag is enabled for a course, the +permission is resolved exclusively through AuthZ (same semantics as Studio, see +``openedx.core.djangoapps.authz.decorators``). When it is disabled, the legacy +check is used, so the pre-RBAC behavior is preserved. +""" + +import logging +from collections.abc import Callable, Iterable + +from django.contrib.auth.models import AbstractUser +from opaque_keys.edx.keys import CourseKey +from openedx_authz import api as authz_api + +from common.djangoapps.student.roles import enable_authz_course_authoring +from lms.djangoapps.instructor.authz.constants import INSTRUCTOR_DASHBOARD_PERMISSIONS +from lms.djangoapps.instructor.authz.legacy import LEGACY_PERMISSION_CHECKS + +log = logging.getLogger(__name__) + +# Signature of a legacy check: ``(user, course_key) -> bool``. +LegacyCheck = Callable[[AbstractUser, CourseKey], bool] + +ACCESS_PATH_AUTHZ = "authz" +ACCESS_PATH_LEGACY = "legacy" + + +def user_has_instructor_permission( + user: AbstractUser, + authz_permission: str, + course_key: CourseKey, + legacy_check: LegacyCheck | None = None, +) -> bool: + """ + Return whether the user holds an AuthZ permission in the course. + + When ``authz.enable_course_authoring`` is enabled for the course, only AuthZ + is consulted and legacy roles are ignored. When it is disabled, only the + legacy check is evaluated. The path that resolved the check (``authz`` or + ``legacy``) is logged. + + Args: + user (AbstractUser): The user whose access is checked. + authz_permission (str): The AuthZ permission identifier, e.g. + ``courses.view_enrollments``. + course_key (CourseKey): The course where the permission is checked. + legacy_check (LegacyCheck | None): The pre-RBAC check, used only when + AuthZ is disabled for the course. If omitted, the default one + registered for ``authz_permission`` in ``LEGACY_PERMISSION_CHECKS`` + is used, and a permission without any legacy check is denied. + + Returns: + bool: True if the user holds the permission. + """ + if enable_authz_course_authoring(course_key): + is_allowed = authz_api.is_user_allowed(user.username, authz_permission, str(course_key)) + _log_decision(ACCESS_PATH_AUTHZ, is_allowed, user, authz_permission, course_key) + return is_allowed + + legacy_check = legacy_check or LEGACY_PERMISSION_CHECKS.get(authz_permission) + is_allowed = bool(legacy_check and legacy_check(user, course_key)) + _log_decision(ACCESS_PATH_LEGACY, is_allowed, user, authz_permission, course_key) + return is_allowed + + +def get_instructor_authz_permissions( + user: AbstractUser, course_key: CourseKey, permissions: Iterable[str] = INSTRUCTOR_DASHBOARD_PERMISSIONS +) -> dict[str, bool]: + """ + Return the effective value of each permission for the user in the course. + + Each value follows the same rules as ``user_has_instructor_permission``, + using the default legacy check of the permission. + + Args: + user (AbstractUser): The user whose permissions are resolved. + course_key (CourseKey): The course where the permissions are resolved. + permissions (Iterable[str]): The permission identifiers to resolve. + Defaults to all the Instructor Dashboard ones. + + Returns: + dict[str, bool]: Mapping of permission identifier to its value, e.g. + ``{"courses.enroll_learners": True}``. + """ + return {permission: user_has_instructor_permission(user, permission, course_key) for permission in permissions} + + +def _log_decision(path: str, is_allowed: bool, user: AbstractUser, authz_permission: str, course_key: CourseKey): + """ + Log the path that resolved a permission check, and its outcome. + + Args: + path (str): The path that resolved the check. + is_allowed (bool): Whether access was granted. + user (AbstractUser): The user checked. + authz_permission (str): The AuthZ permission identifier checked. + course_key (CourseKey): The course where it was checked. + """ + log.info( + "Instructor dashboard permission %s via %s", + "granted" if is_allowed else "denied", + path, + extra={ + "user_id": user.id, + "authz_permission": authz_permission, + "course_key": str(course_key), + "access_path": path, + }, + ) diff --git a/lms/djangoapps/instructor/authz/legacy.py b/lms/djangoapps/instructor/authz/legacy.py new file mode 100644 index 000000000000..0d93eb3b4d53 --- /dev/null +++ b/lms/djangoapps/instructor/authz/legacy.py @@ -0,0 +1,150 @@ +""" +Legacy (pre-RBAC) checks of the Instructor Dashboard AuthZ permissions. + +They are evaluated only when ``authz.enable_course_authoring`` is disabled for +the course, and must keep mirroring the behavior the dashboard had before AuthZ. +Each check has the ``(user, course_key) -> bool`` signature. +""" + +from django.contrib.auth.models import AbstractUser +from opaque_keys.edx.keys import CourseKey + +from common.djangoapps.student.roles import GlobalStaff +from lms.djangoapps.courseware.access import administrative_accesses_to_course_for_user, has_access +from lms.djangoapps.discussion.django_comment_client.utils import get_user_role_names, has_forum_access +from lms.djangoapps.instructor import permissions +from lms.djangoapps.instructor.authz import constants +from openedx.core.djangoapps.django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_MODERATOR, +) +from openedx.core.lib.courses import get_course_by_id + + +def has_staff_access(user: AbstractUser, course_key: CourseKey) -> bool: + """ + Check whether the user has course staff access. + + Args: + user (AbstractUser): The user to check. + course_key (CourseKey): The course to check. + + Returns: + bool: True for course staff, course instructors and global staff. + """ + return bool(has_access(user, "staff", course_key)) + + +def has_instructor_access(user: AbstractUser, course_key: CourseKey) -> bool: + """ + Check whether the user has course instructor access. + + Args: + user (AbstractUser): The user to check. + course_key (CourseKey): The course to check. + + Returns: + bool: True for course instructors and global staff. + """ + return bool(has_access(user, "instructor", course_key)) + + +def has_course_team_access(user: AbstractUser, course_key: CourseKey) -> bool: + """ + Check the access to the course team. + + It follows the same rule as ``permissions.CourseTeamPermission``. + + Args: + user (AbstractUser): The user to check. + course_key (CourseKey): The course to check. + + Returns: + bool: True with instructor access, or with staff access plus the forum + Administrator role. + """ + return has_instructor_access(user, course_key) or ( + has_staff_access(user, course_key) and has_forum_access(user, course_key, FORUM_ROLE_ADMINISTRATOR) + ) + + +def has_cohort_manage_access(user: AbstractUser, course_key: CourseKey) -> bool: + """ + Check the legacy access to change cohorts. + + It applies to any non-GET request of the cohorts API. + + Args: + user (AbstractUser): The user to check. + course_key (CourseKey): The course to check. + + Returns: + bool: True for global staff, superusers, course staff and course + instructors. + """ + if GlobalStaff().has_user(user) or user.is_superuser: + return True + _, staff_access, instructor_access = administrative_accesses_to_course_for_user(user, course_key) + return bool(staff_access or instructor_access) + + +def has_cohort_view_access(user: AbstractUser, course_key: CourseKey) -> bool: + """ + Check the legacy access to read cohorts (GET requests of the cohorts API). + + Args: + user (AbstractUser): The user to check. + course_key (CourseKey): The course to check. + + Returns: + bool: True for everyone who can manage cohorts, plus the forum + Administrator, Moderator and Community TA roles. + """ + if has_cohort_manage_access(user, course_key): + return True + return bool( + get_user_role_names(user, course_key) & {FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA} + ) + + +def bridgekeeper_check(permission_name: str): + """ + Build a legacy check from a ``bridgekeeper`` permission. + + The permissions are those of ``lms.djangoapps.instructor.permissions``. + + Args: + permission_name (str): The bridgekeeper permission, e.g. + ``instructor.view_enrollments``. + + Returns: + Callable[[AbstractUser, CourseKey], bool]: A check that evaluates the + permission against the course. + """ + + def _check(user: AbstractUser, course_key: CourseKey) -> bool: + return user.has_perm(permission_name, get_course_by_id(course_key)) + + return _check + + +# Default legacy check of every permission, as used for the tab visibility and +# ``authz_permissions``. Endpoints whose legacy rule differs (e.g. beta testers, +# which need instructor access) pass their own check. +LEGACY_PERMISSION_CHECKS = { + constants.COURSES_VIEW_COURSE_INFO: has_staff_access, + constants.COURSES_VIEW_ENROLLMENTS: bridgekeeper_check(permissions.VIEW_ENROLLMENTS), + constants.COURSES_ENROLL_LEARNERS: bridgekeeper_check(permissions.CAN_ENROLL), + constants.COURSES_UNENROLL_LEARNERS: bridgekeeper_check(permissions.CAN_ENROLL), + constants.COURSES_VIEW_COURSE_TEAM: has_course_team_access, + constants.COURSES_MANAGE_COURSE_TEAM: has_course_team_access, + constants.COURSES_VIEW_GRADEBOOK: has_staff_access, + constants.COURSES_VIEW_GRADING_CONFIGURATION: has_staff_access, + constants.COURSES_VIEW_GRADING_SETTINGS: has_staff_access, + constants.COURSES_MANAGE_LEARNER_GRADING: bridgekeeper_check(permissions.OVERRIDE_GRADES), + constants.COURSES_VIEW_LEARNER_EXTENSIONS: bridgekeeper_check(permissions.GIVE_STUDENT_EXTENSION), + constants.COURSES_MANAGE_LEARNER_EXTENSIONS: bridgekeeper_check(permissions.GIVE_STUDENT_EXTENSION), + constants.COURSES_VIEW_COHORTS: has_cohort_view_access, + constants.COURSES_MANAGE_COHORTS: has_cohort_manage_access, +} diff --git a/lms/djangoapps/instructor/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py new file mode 100644 index 000000000000..2f6491c8f91c --- /dev/null +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -0,0 +1,74 @@ +""" +DRF permission class to enforce AuthZ in the Instructor Dashboard views. +""" + +from collections.abc import Mapping + +from django.core.exceptions import ImproperlyConfigured +from django.http import Http404 +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework.permissions import BasePermission + +from lms.djangoapps.instructor.authz.helpers import user_has_instructor_permission +from lms.djangoapps.instructor.authz.legacy import bridgekeeper_check + + +class InstructorAuthzPermission(BasePermission): + """ + Replacement of ``InstructorPermission`` for views migrated to AuthZ. + + It is a drop-in replacement of + ``lms.djangoapps.instructor.permissions.InstructorPermission``. + + With the flag enabled only AuthZ is consulted; legacy roles are ignored. + + Attributes expected in the view: + authz_permission (str | Mapping[str, str]): The AuthZ permission + identifier required, or a mapping of HTTP method to identifier when + the view needs a different permission per method. Methods missing + from the mapping are checked with the legacy permission only. + permission_name (str): The legacy ``bridgekeeper`` permission, the one + ``InstructorPermission`` uses. It is enforced when + ``authz.enable_course_authoring`` is disabled for the course. + legacy_permission_check (Callable[[User, CourseKey], bool]): Optional. + Replaces the ``permission_name`` check, for legacy rules that are + not a bridgekeeper permission. + """ + + def has_permission(self, request, view): + """ + Check whether the user holds the permission the view requires. + + Args: + request (Request): The DRF request. + view (APIView): The view, with ``authz_permission`` and the legacy + permission attributes. + + Returns: + bool: True if access is granted. + + Raises: + Http404: If the ``course_id`` kwarg of the view is not a valid + course key. + ImproperlyConfigured: If the view does not define + ``authz_permission``. + """ + try: + course_key = CourseKey.from_string(view.kwargs.get("course_id")) + except InvalidKeyError as exc: + raise Http404("Invalid course key") from exc + + authz_permission = getattr(view, "authz_permission", None) + if authz_permission is None: + raise ImproperlyConfigured(f"{type(view).__name__} must define 'authz_permission'.") + if isinstance(authz_permission, Mapping): + authz_permission = authz_permission.get(request.method) + + legacy_check = getattr(view, "legacy_permission_check", None) + if legacy_check is None and getattr(view, "permission_name", None): + legacy_check = bridgekeeper_check(view.permission_name) + + if authz_permission is None: + return bool(legacy_check and legacy_check(request.user, course_key)) + return user_has_instructor_permission(request.user, authz_permission, course_key, legacy_check) diff --git a/lms/djangoapps/instructor/authz/tests/mixins.py b/lms/djangoapps/instructor/authz/tests/mixins.py new file mode 100644 index 000000000000..ec376f550047 --- /dev/null +++ b/lms/djangoapps/instructor/authz/tests/mixins.py @@ -0,0 +1,109 @@ +""" +Test helpers to assign AuthZ roles in Instructor Dashboard tests. +""" + +from unittest.mock import patch + +from openedx_authz.api.users import assign_role_to_user_in_scope +from openedx_authz.engine.enforcer import AuthzEnforcer + +from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.instructor.authz import constants as authz_constants +from openedx.core import toggles as core_toggles +from openedx.core.djangoapps.authz.tests.fixtures import seed_policies + +COURSE_SCOPE_POLICY = "course-v1^*" + +# TEMPORARY: the role assignments of the Instructor Dashboard permissions depend +# on a separate issue that will define them in openedx-authz. They are seeded +# here only for testing. +_INSTRUCTOR_DASHBOARD_ROLE_PERMISSIONS = ( + authz_constants.COURSES_VIEW_COURSE_INFO, + authz_constants.COURSES_VIEW_ENROLLMENTS, + authz_constants.COURSES_ENROLL_LEARNERS, + authz_constants.COURSES_UNENROLL_LEARNERS, + authz_constants.COURSES_VIEW_GRADEBOOK, + authz_constants.COURSES_VIEW_GRADING_CONFIGURATION, + authz_constants.COURSES_MANAGE_LEARNER_GRADING, + authz_constants.COURSES_VIEW_LEARNER_EXTENSIONS, + authz_constants.COURSES_MANAGE_LEARNER_EXTENSIONS, + authz_constants.COURSES_VIEW_COHORTS, + authz_constants.COURSES_MANAGE_COHORTS, +) +INSTRUCTOR_DASHBOARD_ROLE_PERMISSIONS = { + "course_admin": _INSTRUCTOR_DASHBOARD_ROLE_PERMISSIONS, + "course_staff": _INSTRUCTOR_DASHBOARD_ROLE_PERMISSIONS, +} + + +def seed_instructor_dashboard_policies(): + """ + Add to the enforcer the policies granted by the Instructor Dashboard. + + They are not part of the static policies that ``seed_policies`` loads. + """ + enforcer = AuthzEnforcer.get_enforcer() + for role, permissions in INSTRUCTOR_DASHBOARD_ROLE_PERMISSIONS.items(): + for permission in permissions: + enforcer.add_policy(f"role^{role}", f"act^{permission}", COURSE_SCOPE_POLICY, "allow") + + +class InstructorAuthzTestMixin: + """ + Mixin to test the Instructor Dashboard with the AuthZ flag on or off. + + By default the flag is enabled; call ``set_authz_enabled(False)`` to test + the legacy behavior. The enforcer is seeded with the static AuthZ policies + plus the Instructor Dashboard ones. + """ + + def setUp(self): + super().setUp() + self.authz_flag_patcher = patch.object( + core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, "is_enabled", return_value=True + ) + self.authz_flag_mock = self.authz_flag_patcher.start() + self.addCleanup(self.authz_flag_patcher.stop) + + seed_policies() + seed_instructor_dashboard_policies() + self.addCleanup(AuthzEnforcer.get_enforcer().clear_policy) + + def set_authz_enabled(self, enabled: bool): + """ + Enable or disable ``authz.enable_course_authoring`` for every course. + + Args: + enabled (bool): The value of the flag. + """ + self.authz_flag_mock.return_value = enabled + + def assign_authz_role(self, user, role, course_key): + """ + Assign an AuthZ role to the user in the course. + + The enforcer policy is reloaded afterwards. + + Args: + user (User): The user receiving the role. + role (str): The AuthZ role external key. + course_key (CourseKey): The course where the role is assigned. + """ + assign_role_to_user_in_scope(user.username, role, str(course_key)) + AuthzEnforcer.get_enforcer().load_policy() + + def create_user_with_authz_role(self, role, course_key, **kwargs): + """ + Create a user and assign it an AuthZ role in the course. + + Args: + role (str): The AuthZ role external key. + course_key (CourseKey): The course where the role is assigned. + **kwargs: Extra arguments for ``UserFactory``. + + Returns: + User: The new user. + """ + user = UserFactory(**kwargs) + self.assign_authz_role(user, role, course_key) + return user diff --git a/lms/djangoapps/instructor/authz/tests/test_helpers.py b/lms/djangoapps/instructor/authz/tests/test_helpers.py new file mode 100644 index 000000000000..c147c6221f03 --- /dev/null +++ b/lms/djangoapps/instructor/authz/tests/test_helpers.py @@ -0,0 +1,127 @@ +""" +Tests for the Instructor Dashboard AuthZ helpers. +""" + +from unittest.mock import Mock, patch + +import ddt +from django.test import TestCase +from opaque_keys.edx.locator import CourseLocator +from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_AUDITOR, COURSE_STAFF + +from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.instructor.authz import constants +from lms.djangoapps.instructor.authz.helpers import get_instructor_authz_permissions, user_has_instructor_permission +from lms.djangoapps.instructor.authz.tests.mixins import InstructorAuthzTestMixin + +COURSE_KEY = CourseLocator("edX", "Demo", "2026") +HELPERS = "lms.djangoapps.instructor.authz.helpers" + + +@ddt.ddt +class UserHasInstructorPermissionTest(InstructorAuthzTestMixin, TestCase): + """ + Permissions use AuthZ with the flag on, and legacy checks otherwise. + """ + + def setUp(self): + super().setUp() + self.user = UserFactory() + + @ddt.data( + (COURSE_ADMIN.external_key, constants.COURSES_VIEW_COURSE_INFO, True), + (COURSE_STAFF.external_key, constants.COURSES_ENROLL_LEARNERS, True), + (COURSE_STAFF.external_key, constants.COURSES_MANAGE_COHORTS, True), + (COURSE_ADMIN.external_key, constants.COURSES_MANAGE_COURSE_TEAM, True), + # Course Staff must not get ``manage_course_team``. + (COURSE_STAFF.external_key, constants.COURSES_MANAGE_COURSE_TEAM, False), + (COURSE_AUDITOR.external_key, constants.COURSES_VIEW_ENROLLMENTS, False), + ) + @ddt.unpack + def test_flag_enabled_uses_authz(self, role, permission, expected): + self.assign_authz_role(self.user, role, COURSE_KEY) + + assert user_has_instructor_permission(self.user, permission, COURSE_KEY) == expected + + def test_flag_enabled_ignores_legacy_check(self): + legacy_check = Mock(return_value=True) + + allowed = user_has_instructor_permission( + self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY, legacy_check + ) + + assert not allowed + legacy_check.assert_not_called() + + def test_flag_disabled_ignores_authz(self): + self.set_authz_enabled(False) + self.assign_authz_role(self.user, COURSE_ADMIN.external_key, COURSE_KEY) + legacy_check = Mock(return_value=False) + + allowed = user_has_instructor_permission( + self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY, legacy_check + ) + + assert not allowed + legacy_check.assert_called_once_with(self.user, COURSE_KEY) + + @ddt.data(True, False) + def test_flag_disabled_returns_legacy_result(self, legacy_result): + self.set_authz_enabled(False) + + allowed = user_has_instructor_permission( + self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY, Mock(return_value=legacy_result) + ) + + assert allowed == legacy_result + + def test_flag_disabled_uses_default_legacy_check(self): + self.set_authz_enabled(False) + default_check = Mock(return_value=True) + + with patch.dict(f"{HELPERS}.LEGACY_PERMISSION_CHECKS", {constants.COURSES_VIEW_GRADEBOOK: default_check}): + allowed = user_has_instructor_permission(self.user, constants.COURSES_VIEW_GRADEBOOK, COURSE_KEY) + + assert allowed + default_check.assert_called_once_with(self.user, COURSE_KEY) + + def test_flag_disabled_without_legacy_check_denies(self): + self.set_authz_enabled(False) + + assert not user_has_instructor_permission(self.user, "courses.unknown_permission", COURSE_KEY) + + def test_enabled_per_course(self): + """The flag is evaluated for the course being checked.""" + self.authz_flag_mock.side_effect = lambda course_key=None: course_key == COURSE_KEY + other_course = CourseLocator("edX", "Other", "2026") + self.assign_authz_role(self.user, COURSE_ADMIN.external_key, COURSE_KEY) + legacy_check = Mock(return_value=False) + + assert user_has_instructor_permission(self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY, legacy_check) + assert not user_has_instructor_permission( + self.user, constants.COURSES_VIEW_ENROLLMENTS, other_course, legacy_check + ) + + def test_logs_the_access_path(self): + self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) + + with self.assertLogs(HELPERS, level="INFO") as logs: + user_has_instructor_permission(self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY) + self.set_authz_enabled(False) + user_has_instructor_permission(self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY, lambda *_: True) + + assert [record.access_path for record in logs.records] == ["authz", "legacy"] + assert [record.getMessage() for record in logs.records] == [ + "Instructor dashboard permission granted via authz", + "Instructor dashboard permission granted via legacy", + ] + + def test_get_instructor_authz_permissions(self): + self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) + + result = get_instructor_authz_permissions(self.user, COURSE_KEY) + + assert set(result) == set(constants.INSTRUCTOR_DASHBOARD_PERMISSIONS) + assert result[constants.COURSES_VIEW_COHORTS] + assert result[constants.COURSES_VIEW_COURSE_TEAM] + assert not result[constants.COURSES_MANAGE_COURSE_TEAM] diff --git a/lms/djangoapps/instructor/authz/tests/test_permissions.py b/lms/djangoapps/instructor/authz/tests/test_permissions.py new file mode 100644 index 000000000000..b6d63d44ac87 --- /dev/null +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -0,0 +1,106 @@ +""" +Tests for the ``InstructorAuthzPermission`` DRF permission class. +""" + +from unittest.mock import Mock, patch + +import ddt +import pytest +from django.core.exceptions import ImproperlyConfigured +from django.http import Http404 +from django.test import TestCase +from opaque_keys.edx.locator import CourseLocator +from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_STAFF +from rest_framework.test import APIRequestFactory + +from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.instructor.authz import constants +from lms.djangoapps.instructor.authz.permissions import InstructorAuthzPermission +from lms.djangoapps.instructor.authz.tests.mixins import InstructorAuthzTestMixin + +COURSE_KEY = CourseLocator("edX", "Demo", "2026") + + +class FakeView: + """Minimal view with the attributes read by the permission class.""" + + def __init__(self, course_id=str(COURSE_KEY), **attrs): + self.kwargs = {"course_id": course_id} + for name, value in attrs.items(): + setattr(self, name, value) + + +@ddt.ddt +class InstructorAuthzPermissionTest(InstructorAuthzTestMixin, TestCase): + """Checks the AuthZ and legacy paths of the permission class.""" + + def setUp(self): + super().setUp() + self.user = UserFactory() + self.factory = APIRequestFactory() + + def _request(self, method="get"): + request = getattr(self.factory, method)("/") + request.user = self.user + return request + + def test_authz_enabled_grants_access_by_authz_role(self): + self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) + view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: False) + + assert InstructorAuthzPermission().has_permission(self._request(), view) + + def test_authz_enabled_ignores_legacy(self): + view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: True) + + assert not InstructorAuthzPermission().has_permission(self._request(), view) + + def test_authz_disabled_uses_legacy_permission_check(self): + self.set_authz_enabled(False) + self.assign_authz_role(self.user, COURSE_ADMIN.external_key, COURSE_KEY) + legacy_check = Mock(return_value=True) + view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=legacy_check) + + assert InstructorAuthzPermission().has_permission(self._request(), view) + legacy_check.assert_called_once_with(self.user, COURSE_KEY) + + def test_authz_disabled_uses_bridgekeeper_permission_name(self): + self.set_authz_enabled(False) + view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, permission_name="instructor.dashboard") + with ( + patch("lms.djangoapps.instructor.authz.legacy.get_course_by_id", return_value="course"), + patch.object(type(self.user), "has_perm", return_value=True) as has_perm, + ): + assert InstructorAuthzPermission().has_permission(self._request(), view) + has_perm.assert_called_once_with("instructor.dashboard", "course") + + @ddt.data(("get", True), ("post", False)) + @ddt.unpack + def test_permission_per_http_method(self, method, expected): + self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) + view = FakeView( + authz_permission={ + "GET": constants.COURSES_VIEW_COURSE_TEAM, + # Course Staff does not hold it. + "POST": constants.COURSES_MANAGE_COURSE_TEAM, + } + ) + + assert InstructorAuthzPermission().has_permission(self._request(method), view) == expected + + def test_method_missing_from_mapping_uses_legacy(self): + view = FakeView( + authz_permission={"GET": constants.COURSES_VIEW_COURSE_TEAM}, legacy_permission_check=lambda *_: True + ) + + assert InstructorAuthzPermission().has_permission(self._request("delete"), view) + + def test_invalid_course_key(self): + view = FakeView(course_id="not-a-course", authz_permission=constants.COURSES_VIEW_ENROLLMENTS) + + with pytest.raises(Http404): + InstructorAuthzPermission().has_permission(self._request(), view) + + def test_view_without_authz_permission(self): + with pytest.raises(ImproperlyConfigured): + InstructorAuthzPermission().has_permission(self._request(), FakeView()) diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py new file mode 100644 index 000000000000..7e65c1a2018e --- /dev/null +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -0,0 +1,227 @@ +""" +Tests for the AuthZ enforcement of the Instructor Dashboard API v2. + +They cover the tabs, ``authz_permissions`` and the migrated endpoints. +""" + +from unittest.mock import patch + +import ddt +from django.urls import reverse +from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_AUDITOR, COURSE_STAFF +from rest_framework import status +from rest_framework.test import APIClient + +from common.djangoapps.student.tests.factories import ( + CourseEnrollmentFactory, + InstructorFactory, + StaffFactory, + UserFactory, +) +from lms.djangoapps.instructor.authz import constants +from lms.djangoapps.instructor.authz.tests.mixins import InstructorAuthzTestMixin +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + +HELPERS = "lms.djangoapps.instructor.authz.helpers" + +# Tabs migrated to AuthZ, and the permission that shows each of them. +MIGRATED_TABS = { + "course_info": constants.COURSES_VIEW_COURSE_INFO, + "enrollments": constants.COURSES_VIEW_ENROLLMENTS, + "course_team": constants.COURSES_VIEW_COURSE_TEAM, + "grading": constants.COURSES_VIEW_GRADEBOOK, + "cohorts": constants.COURSES_VIEW_COHORTS, +} + + +@ddt.ddt +class CourseMetadataViewAuthzTest(InstructorAuthzTestMixin, SharedModuleStoreTestCase): + """ + ``CourseMetadataView`` tabs and permissions follow the AuthZ flag. + + With the flag on they are resolved through AuthZ, and through the legacy + roles when it is off. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + cls.course_key = cls.course.id + + def setUp(self): + super().setUp() + self.client = APIClient() + self.url = reverse("instructor_api_v2:course_metadata", kwargs={"course_id": str(self.course_key)}) + + def _get(self, user): + self.client.force_authenticate(user=user) + return self.client.get(self.url) + + def _tab_ids(self, response): + return {tab["tab_id"] for tab in response.data["tabs"]} + + def test_existing_permissions_field_is_unchanged(self): + user = self.create_user_with_authz_role(COURSE_ADMIN.external_key, self.course_key) + + response = self._get(user) + + assert response.status_code == status.HTTP_200_OK + assert set(response.data["permissions"]) == { + "admin", + "instructor", + "finance_admin", + "sales_admin", + "staff", + "forum_admin", + "data_researcher", + } + + def test_authz_course_admin(self): + user = self.create_user_with_authz_role(COURSE_ADMIN.external_key, self.course_key) + + response = self._get(user) + + assert response.status_code == status.HTTP_200_OK + assert set(MIGRATED_TABS) <= self._tab_ids(response) + assert set(response.data["authz_permissions"]) == set(constants.INSTRUCTOR_DASHBOARD_PERMISSIONS) + assert all(response.data["authz_permissions"].values()) + + def test_authz_course_staff(self): + user = self.create_user_with_authz_role(COURSE_STAFF.external_key, self.course_key) + + response = self._get(user) + + tabs = self._tab_ids(response) + assert set(MIGRATED_TABS) <= tabs + permissions = response.data["authz_permissions"] + assert permissions[constants.COURSES_VIEW_COURSE_TEAM] + # Course Staff does not manage the course team. + assert not permissions[constants.COURSES_MANAGE_COURSE_TEAM] + assert permissions[constants.COURSES_MANAGE_COHORTS] + + def test_authz_denied_overrides_legacy_roles(self): + """ + With the flag on, AuthZ denying a legacy instructor means no access. + """ + instructor = InstructorFactory(course_key=self.course_key) + + with patch(f"{HELPERS}.authz_api.is_user_allowed", return_value=False): + response = self._get(instructor) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + @ddt.data(*MIGRATED_TABS.items()) + @ddt.unpack + def test_authz_tab_depends_on_its_permission(self, tab_id, tab_permission): + """ + Each migrated tab needs its own permission besides the dashboard one. + """ + user = UserFactory() + + def allowed_only(*permissions): + return lambda _username, permission, _scope: permission in permissions + + with patch( + f"{HELPERS}.authz_api.is_user_allowed", side_effect=allowed_only(constants.COURSES_VIEW_COURSE_INFO) + ): + tabs_without_permission = self._tab_ids(self._get(user)) + with patch( + f"{HELPERS}.authz_api.is_user_allowed", + side_effect=allowed_only(constants.COURSES_VIEW_COURSE_INFO, tab_permission), + ): + tabs_with_permission = self._tab_ids(self._get(user)) + + assert tab_id in tabs_with_permission + if tab_id != "course_info": + assert tab_id not in tabs_without_permission + + def test_legacy_instructor_sees_migrated_tabs(self): + self.set_authz_enabled(False) + instructor = InstructorFactory(course_key=self.course_key) + + with patch("lms.djangoapps.instructor.views.serializers_v2.is_enabled_for_course", return_value=True): + response = self._get(instructor) + + assert response.status_code == status.HTTP_200_OK + assert "date_extensions" in self._tab_ids(response) + assert set(MIGRATED_TABS) <= self._tab_ids(response) + assert response.data["authz_permissions"][constants.COURSES_MANAGE_COURSE_TEAM] + + def test_legacy_staff_has_staff_only_tabs(self): + """ + Staff without the forum Administrator role does not see Course Team. + """ + self.set_authz_enabled(False) + staff = StaffFactory(course_key=self.course_key) + + response = self._get(staff) + + tabs = self._tab_ids(response) + assert tabs & set(MIGRATED_TABS) == set(MIGRATED_TABS) - {"course_team"} + assert not response.data["authz_permissions"][constants.COURSES_MANAGE_COURSE_TEAM] + + def test_legacy_does_not_query_authz(self): + self.set_authz_enabled(False) + instructor = InstructorFactory(course_key=self.course_key) + + with patch(f"{HELPERS}.authz_api.is_user_allowed") as is_user_allowed: + response = self._get(instructor) + + assert response.status_code == status.HTTP_200_OK + is_user_allowed.assert_not_called() + + +class CourseEnrollmentsViewAuthzTest(InstructorAuthzTestMixin, SharedModuleStoreTestCase): + """ + ``CourseEnrollmentsView`` requires ``courses.view_enrollments``. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def setUp(self): + super().setUp() + self.client = APIClient() + self.url = reverse("instructor_api_v2:course_enrollments", kwargs={"course_id": str(self.course.id)}) + CourseEnrollmentFactory(user=UserFactory(), course_id=self.course.id, is_active=True) + + def _get(self, user): + self.client.force_authenticate(user=user) + return self.client.get(self.url) + + def test_authz_course_staff_allowed(self): + user = self.create_user_with_authz_role(COURSE_STAFF.external_key, self.course.id) + + assert self._get(user).status_code == status.HTTP_200_OK + + def test_authz_without_permission_denied(self): + user = self.create_user_with_authz_role(COURSE_AUDITOR.external_key, self.course.id) + + assert self._get(user).status_code == status.HTTP_403_FORBIDDEN + + def test_authz_denied_overrides_legacy_staff(self): + staff = StaffFactory(course_key=self.course.id) + + with patch(f"{HELPERS}.authz_api.is_user_allowed", return_value=False): + response = self._get(staff) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_legacy_staff_allowed(self): + self.set_authz_enabled(False) + staff = StaffFactory(course_key=self.course.id) + + with patch(f"{HELPERS}.authz_api.is_user_allowed") as is_user_allowed: + response = self._get(staff) + + assert response.status_code == status.HTTP_200_OK + is_user_allowed.assert_not_called() + + def test_legacy_without_role_denied(self): + self.set_authz_enabled(False) + + assert self._get(UserFactory()).status_code == status.HTTP_403_FORBIDDEN diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index 6d38d3cc191b..ddbf04c74a83 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -103,6 +103,11 @@ revoke_access, update_forum_role, ) +from lms.djangoapps.instructor.authz import constants as authz_constants +from lms.djangoapps.instructor.authz import legacy as authz_legacy +from lms.djangoapps.instructor.authz.helpers import user_has_instructor_permission +from lms.djangoapps.instructor.authz.legacy import bridgekeeper_check +from lms.djangoapps.instructor.authz.permissions import InstructorAuthzPermission from lms.djangoapps.instructor.constants import ReportType from lms.djangoapps.instructor.enrollment import ( enroll_email, @@ -204,7 +209,8 @@ class CourseMetadataView(DeveloperErrorViewMixin, APIView): permissions, and navigation sections. """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_classes = (IsAuthenticated, InstructorAuthzPermission) + authz_permission = authz_constants.COURSES_VIEW_COURSE_INFO permission_name = permissions.VIEW_DASHBOARD @extend_schema( @@ -298,15 +304,26 @@ def get(self, request, course_id): course_key = CourseKey.from_string(course_id) course = get_course_by_id(course_key) - original_user_is_staff = has_access(request.user, 'staff', course_key).has_access + def can_view_dashboard(user): + # Flag on: AuthZ ``view_course_info``. + # Flag off: the legacy dashboard permission. + return user_has_instructor_permission( + user, + authz_constants.COURSES_VIEW_COURSE_INFO, + course_key, + bridgekeeper_check(permissions.VIEW_DASHBOARD), + ) + + original_user_is_staff = user_has_instructor_permission( + request.user, authz_constants.COURSES_VIEW_COURSE_INFO, course_key, authz_legacy.has_staff_access + ) _, request.user = setup_masquerade( request, course_key, staff_access=original_user_is_staff, ) - if get_masquerade_role(request.user, course_key) == 'student' or \ - not request.user.has_perm(permissions.VIEW_DASHBOARD, course): + if get_masquerade_role(request.user, course_key) == 'student' or not can_view_dashboard(request.user): raise PermissionDenied('This user does not have access to the instructor dashboard.') tabs = get_course_tab_list(request.user, course) @@ -2474,7 +2491,8 @@ class CourseEnrollmentsView(DeveloperErrorViewMixin, ListAPIView): * 401: Unauthorized - User is not authenticated * 403: Forbidden - User lacks instructor permissions """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_classes = (IsAuthenticated, InstructorAuthzPermission) + authz_permission = authz_constants.COURSES_VIEW_ENROLLMENTS permission_name = permissions.VIEW_ENROLLMENTS serializer_class = CourseEnrollmentSerializerV2 filter_backends = [DjangoFilterBackend] diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index 5b6144d27298..a119f07be57d 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -33,6 +33,9 @@ from lms.djangoapps.grades.api import is_writable_gradebook_enabled from lms.djangoapps.instructor import permissions from lms.djangoapps.instructor.access import FORUM_ROLES, ROLES +from lms.djangoapps.instructor.authz import constants as authz_constants +from lms.djangoapps.instructor.authz import legacy as authz_legacy +from lms.djangoapps.instructor.authz.helpers import get_instructor_authz_permissions, user_has_instructor_permission from lms.djangoapps.instructor.views.instructor_dashboard import get_analytics_dashboard_message from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -84,6 +87,13 @@ class CourseInformationSerializerV2(serializers.Serializer): help_text="URL to the admin console (requires instructor access and MFE configuration, null if not accessible)" ) permissions = serializers.SerializerMethodField(help_text="User permissions for instructor dashboard features") + authz_permissions = serializers.SerializerMethodField( + help_text=( + "Effective value of each AuthZ permission of the instructor dashboard for the user in the course, keyed by " + "permission identifier. Resolved through AuthZ when it is enabled for the course, and through the " + "equivalent legacy roles otherwise." + ) + ) tabs = serializers.SerializerMethodField(help_text="List of course tabs with configuration and display information") disable_buttons = serializers.SerializerMethodField( help_text="Whether to disable certain bulk action buttons due to large course size" @@ -158,51 +168,35 @@ def get_tabs(self, data): # Without explicit sort_order values, there's no deterministic way to interleave # backend tabs with plugin-inserted tabs, and tab order could shift based on # load/config timing. - if access['staff']: - tabs.extend([ - { - 'tab_id': 'course_info', - 'title': _('Course Info'), - 'url': self._build_tab_url( - 'INSTRUCTOR_MICROFRONTEND_URL', - course_key, - 'course_info' - ), - 'sort_order': 10, - }, - { - 'tab_id': 'enrollments', - 'title': _('Enrollments'), - 'url': self._build_tab_url( - 'INSTRUCTOR_MICROFRONTEND_URL', - course_key, - 'enrollments' - ), - 'sort_order': 20, - }, - { - 'tab_id': 'grading', - 'title': _('Grading'), - 'url': self._build_tab_url( - 'INSTRUCTOR_MICROFRONTEND_URL', - course_key, - 'grading' - ), - 'sort_order': 40, - }, - { - 'tab_id': 'cohorts', - 'title': _('Cohorts'), - 'url': self._build_tab_url( - 'INSTRUCTOR_MICROFRONTEND_URL', - course_key, - 'cohorts' - ), - 'sort_order': 90, - }, - ]) - - if access['instructor'] or (access['staff'] and access['forum_admin']): + # The tabs below are enforced through AuthZ when + # ``authz.enable_course_authoring`` is enabled for the course; + # otherwise they keep the legacy role checks passed as the second + # argument. + def can(permission, legacy_check=None): + return user_has_instructor_permission(request.user, permission, course_key, legacy_check) + + # (tab_id, title, sort_order, AuthZ permission, legacy check). A + # ``None`` legacy check uses the default one of the permission, which + # matches the legacy tab condition. + authz_tabs = [ + ('course_info', _('Course Info'), 10, authz_constants.COURSES_VIEW_COURSE_INFO, None), + ('enrollments', _('Enrollments'), 20, authz_constants.COURSES_VIEW_ENROLLMENTS, None), + ('grading', _('Grading'), 40, authz_constants.COURSES_VIEW_GRADEBOOK, None), + # The default legacy check of ``view_cohorts`` also lets the forum + # moderators read the cohorts API, but the tab has always required + # staff access. + ('cohorts', _('Cohorts'), 90, authz_constants.COURSES_VIEW_COHORTS, authz_legacy.has_staff_access), + ] + for tab, title, sort_order, authz_perm, legacy_check in authz_tabs: + if can(authz_perm, legacy_check): + tabs.append({ + 'tab_id': tab, + 'title': title, + 'url': self._build_tab_url('INSTRUCTOR_MICROFRONTEND_URL', course_key, tab), + 'sort_order': sort_order, + }) + + if can(authz_constants.COURSES_VIEW_COURSE_TEAM): tabs.append({ 'tab_id': 'course_team', 'title': _('Course Team'), @@ -226,7 +220,9 @@ def get_tabs(self, data): } ) - if access['instructor'] and is_enabled_for_course(course_key): + if is_enabled_for_course(course_key) and can( + authz_constants.COURSES_VIEW_LEARNER_EXTENSIONS, authz_legacy.has_instructor_access + ): tabs.append({ 'tab_id': 'date_extensions', 'title': _('Date Extensions'), @@ -421,6 +417,19 @@ def get_num_sections(self, data): course = data['course'] return len(course.get_children()) if hasattr(course, 'get_children') else 0 + def get_authz_permissions(self, data) -> dict[str, bool]: + """ + Get the effective AuthZ permissions of the user for the course. + + Args: + data (dict): The serializer context, with ``user`` and ``course``. + + Returns: + dict[str, bool]: Mapping of AuthZ permission identifier to its + value. + """ + return get_instructor_authz_permissions(data['user'], data['course'].id) + def get_permissions(self, data): """Get user permissions for the course.""" user = data['user'] From b4d426e67230b897c839403e5c73dd76afebaa38 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 13:14:40 -0500 Subject: [PATCH 03/17] docs: expand the Instructor Dashboard AuthZ migration guide * Add examples to "Migrating a view": a single permission, a permission per HTTP method, a custom legacy check, direct use of user_has_instructor_permission and the errors returned. * State in the cohort legacy checks that they follow the same rule as IsStaffOrAdmin. * Wrap the long line in has_cohort_view_access and move the "Shared with Authoring" note of COURSES_VIEW_GRADING_SETTINGS to the same line. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 77 +++++++++++++++++++- lms/djangoapps/instructor/authz/constants.py | 3 +- lms/djangoapps/instructor/authz/legacy.py | 10 ++- 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst index ac832a918d68..9fa61001f317 100644 --- a/lms/djangoapps/instructor/authz/README.rst +++ b/lms/djangoapps/instructor/authz/README.rst @@ -25,7 +25,12 @@ Every decision is logged with the path that resolved it (``authz`` or ``legacy`` Migrating a view **************** -Replace ``InstructorPermission`` by ``InstructorAuthzPermission`` and declare the AuthZ permission next to the legacy one: +Replace ``InstructorPermission`` by ``InstructorAuthzPermission`` and declare the AuthZ permission next to the legacy one. ``course_id`` must be a URL kwarg: the permission class reads it from ``view.kwargs`` and returns a ``404`` if it is not a valid course key. + +Single permission +================= + +The same AuthZ permission for every HTTP method, with the legacy bridgekeeper permission the view already used: .. code-block:: python @@ -34,9 +39,75 @@ Replace ``InstructorPermission`` by ``InstructorAuthzPermission`` and declare th class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) - # A permission, or a mapping such as {"GET": ..., "POST": ...}. authz_permission = authz_constants.COURSES_VIEW_ENROLLMENTS # The legacy bridgekeeper permission. permission_name = permissions.VIEW_ENROLLMENTS -When the legacy rule is not a bridgekeeper permission, define ``legacy_permission_check(user, course_key)`` on the view. To check a permission in other code use ``user_has_instructor_permission``. +Different permission per HTTP method +==================================== + +Use a mapping when reading and changing need different permissions. A method missing from the mapping skips AuthZ and is checked with the legacy rule only: + +.. code-block:: python + + class MyView(APIView): + permission_classes = (IsAuthenticated, InstructorAuthzPermission) + authz_permission = { + "GET": authz_constants.COURSES_VIEW_LEARNER_EXTENSIONS, + "POST": authz_constants.COURSES_MANAGE_LEARNER_EXTENSIONS, + } + permission_name = permissions.GIVE_STUDENT_EXTENSION + +Legacy rule that is not a bridgekeeper permission +================================================= + +Define ``legacy_permission_check(user, course_key)`` on the view. It replaces ``permission_name`` and receives the same arguments for every method. Reuse the checks of ``legacy.py`` when one matches: + +.. code-block:: python + + from lms.djangoapps.instructor.authz import legacy as authz_legacy + + class MyView(APIView): + permission_classes = (IsAuthenticated, InstructorAuthzPermission) + authz_permission = { + "GET": authz_constants.COURSES_VIEW_COHORTS, + "POST": authz_constants.COURSES_MANAGE_COHORTS, + } + + @staticmethod + def legacy_permission_check(user, course_key): + return authz_legacy.has_cohort_manage_access(user, course_key) + +Without ``@staticmethod`` the method would receive ``self`` as the first argument. The legacy check does not receive the request, so it cannot vary by HTTP method. If the legacy rule differs per method, call ``user_has_instructor_permission`` in the handler with the legacy check of each case. + +Checking a permission in other code +=================================== + +Use ``user_has_instructor_permission`` inside a handler, a serializer or any other code. The legacy check is optional and defaults to the one registered in ``legacy.LEGACY_PERMISSION_CHECKS``: + +.. code-block:: python + + from opaque_keys.edx.keys import CourseKey + from lms.djangoapps.instructor.authz.helpers import user_has_instructor_permission + + def post(self, request, course_id): + course_key = CourseKey.from_string(course_id) + if user_has_instructor_permission(request.user, authz_constants.COURSES_MANAGE_COHORTS, course_key): + ... + + # With an explicit legacy check, used only when the flag is disabled. + user_has_instructor_permission( + request.user, + authz_constants.COURSES_VIEW_ENROLLMENTS, + course_key, + authz_legacy.has_instructor_access, + ) + +To get the effective value of several permissions at once (as ``authz_permissions`` in ``CourseMetadataView``) use ``get_instructor_authz_permissions(user, course_key)``. + +Errors +====== + +- No ``authz_permission`` on the view: ``ImproperlyConfigured``. +- Invalid ``course_id``: ``404``. +- Denied by the active path (AuthZ with the flag enabled, legacy otherwise): ``403``. diff --git a/lms/djangoapps/instructor/authz/constants.py b/lms/djangoapps/instructor/authz/constants.py index 61a6349625fb..668c2b4e6aa3 100644 --- a/lms/djangoapps/instructor/authz/constants.py +++ b/lms/djangoapps/instructor/authz/constants.py @@ -24,8 +24,7 @@ # Grading COURSES_VIEW_GRADEBOOK = "courses.view_gradebook" COURSES_VIEW_GRADING_CONFIGURATION = "courses.view_grading_configuration" -# Shared with Authoring. -COURSES_VIEW_GRADING_SETTINGS = "courses.view_grading_settings" +COURSES_VIEW_GRADING_SETTINGS = "courses.view_grading_settings" # Shared with Authoring. COURSES_MANAGE_LEARNER_GRADING = "courses.manage_learner_grading" # Date extensions diff --git a/lms/djangoapps/instructor/authz/legacy.py b/lms/djangoapps/instructor/authz/legacy.py index 0d93eb3b4d53..47ae239010b4 100644 --- a/lms/djangoapps/instructor/authz/legacy.py +++ b/lms/djangoapps/instructor/authz/legacy.py @@ -73,7 +73,9 @@ def has_cohort_manage_access(user: AbstractUser, course_key: CourseKey) -> bool: """ Check the legacy access to change cohorts. - It applies to any non-GET request of the cohorts API. + It applies to any non-GET request of the cohorts API and uses the same rule + as ``IsStaffOrAdmin`` (``openedx.core.djangoapps.course_groups.permissions``) + for those requests. Args: user (AbstractUser): The user to check. @@ -93,6 +95,9 @@ def has_cohort_view_access(user: AbstractUser, course_key: CourseKey) -> bool: """ Check the legacy access to read cohorts (GET requests of the cohorts API). + It uses the same rule as ``IsStaffOrAdmin`` + (``openedx.core.djangoapps.course_groups.permissions``) for GET requests. + Args: user (AbstractUser): The user to check. course_key (CourseKey): The course to check. @@ -104,7 +109,8 @@ def has_cohort_view_access(user: AbstractUser, course_key: CourseKey) -> bool: if has_cohort_manage_access(user, course_key): return True return bool( - get_user_role_names(user, course_key) & {FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA} + get_user_role_names(user, course_key) + & {FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA} ) From c0f6e46ebd5de6a398d5e86f5f13ee91297dcc23 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 14:14:02 -0500 Subject: [PATCH 04/17] test: share the AuthZ flag and policy setup between test mixins * Add AuthzTestMixin with the code common to the course authoring and Instructor Dashboard test mixins: the flag patch, the policy seeding and cleanup, and assign_authz_role. * Add set_authz_enabled and set_authz_enabled_for to turn the flag off or enable it only for some courses, instead of ad hoc patches in each test. * CourseAuthoringAuthzTestMixin and InstructorAuthzTestMixin extend it and keep only what is specific to them. * Add docstrings to the Instructor Dashboard AuthZ tests. Co-Authored-By: Claude Sonnet 5.5 --- .../instructor/authz/tests/mixins.py | 39 +-------- .../instructor/authz/tests/test_helpers.py | 8 ++ .../authz/tests/test_permissions.py | 8 ++ .../instructor/tests/test_api_v2_authz.py | 10 +++ openedx/core/djangoapps/authz/tests/mixins.py | 87 +++++++++++++++---- 5 files changed, 96 insertions(+), 56 deletions(-) diff --git a/lms/djangoapps/instructor/authz/tests/mixins.py b/lms/djangoapps/instructor/authz/tests/mixins.py index ec376f550047..8dc2eabe5046 100644 --- a/lms/djangoapps/instructor/authz/tests/mixins.py +++ b/lms/djangoapps/instructor/authz/tests/mixins.py @@ -2,15 +2,11 @@ Test helpers to assign AuthZ roles in Instructor Dashboard tests. """ -from unittest.mock import patch - -from openedx_authz.api.users import assign_role_to_user_in_scope from openedx_authz.engine.enforcer import AuthzEnforcer from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.instructor.authz import constants as authz_constants -from openedx.core import toggles as core_toggles -from openedx.core.djangoapps.authz.tests.fixtures import seed_policies +from openedx.core.djangoapps.authz.tests.mixins import AuthzTestMixin COURSE_SCOPE_POLICY = "course-v1^*" @@ -48,7 +44,7 @@ def seed_instructor_dashboard_policies(): enforcer.add_policy(f"role^{role}", f"act^{permission}", COURSE_SCOPE_POLICY, "allow") -class InstructorAuthzTestMixin: +class InstructorAuthzTestMixin(AuthzTestMixin): """ Mixin to test the Instructor Dashboard with the AuthZ flag on or off. @@ -59,38 +55,7 @@ class InstructorAuthzTestMixin: def setUp(self): super().setUp() - self.authz_flag_patcher = patch.object( - core_toggles.AUTHZ_COURSE_AUTHORING_FLAG, "is_enabled", return_value=True - ) - self.authz_flag_mock = self.authz_flag_patcher.start() - self.addCleanup(self.authz_flag_patcher.stop) - - seed_policies() seed_instructor_dashboard_policies() - self.addCleanup(AuthzEnforcer.get_enforcer().clear_policy) - - def set_authz_enabled(self, enabled: bool): - """ - Enable or disable ``authz.enable_course_authoring`` for every course. - - Args: - enabled (bool): The value of the flag. - """ - self.authz_flag_mock.return_value = enabled - - def assign_authz_role(self, user, role, course_key): - """ - Assign an AuthZ role to the user in the course. - - The enforcer policy is reloaded afterwards. - - Args: - user (User): The user receiving the role. - role (str): The AuthZ role external key. - course_key (CourseKey): The course where the role is assigned. - """ - assign_role_to_user_in_scope(user.username, role, str(course_key)) - AuthzEnforcer.get_enforcer().load_policy() def create_user_with_authz_role(self, role, course_key, **kwargs): """ diff --git a/lms/djangoapps/instructor/authz/tests/test_helpers.py b/lms/djangoapps/instructor/authz/tests/test_helpers.py index c147c6221f03..682e0d56a1a9 100644 --- a/lms/djangoapps/instructor/authz/tests/test_helpers.py +++ b/lms/djangoapps/instructor/authz/tests/test_helpers.py @@ -39,11 +39,13 @@ def setUp(self): ) @ddt.unpack def test_flag_enabled_uses_authz(self, role, permission, expected): + """With the flag on, the result is the one of the AuthZ role of the user.""" self.assign_authz_role(self.user, role, COURSE_KEY) assert user_has_instructor_permission(self.user, permission, COURSE_KEY) == expected def test_flag_enabled_ignores_legacy_check(self): + """With the flag on, the legacy check is never called.""" legacy_check = Mock(return_value=True) allowed = user_has_instructor_permission( @@ -54,6 +56,7 @@ def test_flag_enabled_ignores_legacy_check(self): legacy_check.assert_not_called() def test_flag_disabled_ignores_authz(self): + """With the flag off, AuthZ roles are ignored and only the legacy check is called.""" self.set_authz_enabled(False) self.assign_authz_role(self.user, COURSE_ADMIN.external_key, COURSE_KEY) legacy_check = Mock(return_value=False) @@ -67,6 +70,7 @@ def test_flag_disabled_ignores_authz(self): @ddt.data(True, False) def test_flag_disabled_returns_legacy_result(self, legacy_result): + """With the flag off, the result is the one of the legacy check.""" self.set_authz_enabled(False) allowed = user_has_instructor_permission( @@ -76,6 +80,7 @@ def test_flag_disabled_returns_legacy_result(self, legacy_result): assert allowed == legacy_result def test_flag_disabled_uses_default_legacy_check(self): + """With the flag off and no legacy check given, the one registered for the permission is used.""" self.set_authz_enabled(False) default_check = Mock(return_value=True) @@ -86,6 +91,7 @@ def test_flag_disabled_uses_default_legacy_check(self): default_check.assert_called_once_with(self.user, COURSE_KEY) def test_flag_disabled_without_legacy_check_denies(self): + """With the flag off, a permission without any legacy check is denied.""" self.set_authz_enabled(False) assert not user_has_instructor_permission(self.user, "courses.unknown_permission", COURSE_KEY) @@ -103,6 +109,7 @@ def test_enabled_per_course(self): ) def test_logs_the_access_path(self): + """Each decision is logged with the path (``authz`` or ``legacy``) that resolved it.""" self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) with self.assertLogs(HELPERS, level="INFO") as logs: @@ -117,6 +124,7 @@ def test_logs_the_access_path(self): ] def test_get_instructor_authz_permissions(self): + """All the dashboard permissions are resolved with their effective value for the user.""" self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) result = get_instructor_authz_permissions(self.user, COURSE_KEY) diff --git a/lms/djangoapps/instructor/authz/tests/test_permissions.py b/lms/djangoapps/instructor/authz/tests/test_permissions.py index b6d63d44ac87..5d899acf7b4d 100644 --- a/lms/djangoapps/instructor/authz/tests/test_permissions.py +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -45,17 +45,20 @@ def _request(self, method="get"): return request def test_authz_enabled_grants_access_by_authz_role(self): + """With the flag on, an AuthZ role grants access even if the legacy check denies.""" self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: False) assert InstructorAuthzPermission().has_permission(self._request(), view) def test_authz_enabled_ignores_legacy(self): + """With the flag on, a passing legacy check does not grant access without an AuthZ role.""" view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: True) assert not InstructorAuthzPermission().has_permission(self._request(), view) def test_authz_disabled_uses_legacy_permission_check(self): + """With the flag off, the view legacy check decides and AuthZ roles are ignored.""" self.set_authz_enabled(False) self.assign_authz_role(self.user, COURSE_ADMIN.external_key, COURSE_KEY) legacy_check = Mock(return_value=True) @@ -65,6 +68,7 @@ def test_authz_disabled_uses_legacy_permission_check(self): legacy_check.assert_called_once_with(self.user, COURSE_KEY) def test_authz_disabled_uses_bridgekeeper_permission_name(self): + """With the flag off and no legacy check, ``permission_name`` is evaluated as a bridgekeeper permission.""" self.set_authz_enabled(False) view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, permission_name="instructor.dashboard") with ( @@ -77,6 +81,7 @@ def test_authz_disabled_uses_bridgekeeper_permission_name(self): @ddt.data(("get", True), ("post", False)) @ddt.unpack def test_permission_per_http_method(self, method, expected): + """The AuthZ permission is picked from the mapping by HTTP method.""" self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) view = FakeView( authz_permission={ @@ -89,6 +94,7 @@ def test_permission_per_http_method(self, method, expected): assert InstructorAuthzPermission().has_permission(self._request(method), view) == expected def test_method_missing_from_mapping_uses_legacy(self): + """A method missing from the mapping is checked with the legacy rule only.""" view = FakeView( authz_permission={"GET": constants.COURSES_VIEW_COURSE_TEAM}, legacy_permission_check=lambda *_: True ) @@ -96,11 +102,13 @@ def test_method_missing_from_mapping_uses_legacy(self): assert InstructorAuthzPermission().has_permission(self._request("delete"), view) def test_invalid_course_key(self): + """An invalid ``course_id`` raises ``Http404``.""" view = FakeView(course_id="not-a-course", authz_permission=constants.COURSES_VIEW_ENROLLMENTS) with pytest.raises(Http404): InstructorAuthzPermission().has_permission(self._request(), view) def test_view_without_authz_permission(self): + """A view that does not define ``authz_permission`` raises ``ImproperlyConfigured``.""" with pytest.raises(ImproperlyConfigured): InstructorAuthzPermission().has_permission(self._request(), FakeView()) diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py index 7e65c1a2018e..c1429f0374ff 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2_authz.py +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -63,6 +63,7 @@ def _tab_ids(self, response): return {tab["tab_id"] for tab in response.data["tabs"]} def test_existing_permissions_field_is_unchanged(self): + """The legacy ``permissions`` field keeps the same keys.""" user = self.create_user_with_authz_role(COURSE_ADMIN.external_key, self.course_key) response = self._get(user) @@ -79,6 +80,7 @@ def test_existing_permissions_field_is_unchanged(self): } def test_authz_course_admin(self): + """A Course Admin sees every migrated tab and holds every permission.""" user = self.create_user_with_authz_role(COURSE_ADMIN.external_key, self.course_key) response = self._get(user) @@ -89,6 +91,7 @@ def test_authz_course_admin(self): assert all(response.data["authz_permissions"].values()) def test_authz_course_staff(self): + """A Course Staff sees every migrated tab but cannot manage the course team.""" user = self.create_user_with_authz_role(COURSE_STAFF.external_key, self.course_key) response = self._get(user) @@ -138,6 +141,7 @@ def allowed_only(*permissions): assert tab_id not in tabs_without_permission def test_legacy_instructor_sees_migrated_tabs(self): + """With the flag off, a legacy instructor sees every migrated tab and manages the course team.""" self.set_authz_enabled(False) instructor = InstructorFactory(course_key=self.course_key) @@ -163,6 +167,7 @@ def test_legacy_staff_has_staff_only_tabs(self): assert not response.data["authz_permissions"][constants.COURSES_MANAGE_COURSE_TEAM] def test_legacy_does_not_query_authz(self): + """With the flag off, AuthZ is not queried.""" self.set_authz_enabled(False) instructor = InstructorFactory(course_key=self.course_key) @@ -194,16 +199,19 @@ def _get(self, user): return self.client.get(self.url) def test_authz_course_staff_allowed(self): + """With the flag on, a Course Staff can list the enrollments.""" user = self.create_user_with_authz_role(COURSE_STAFF.external_key, self.course.id) assert self._get(user).status_code == status.HTTP_200_OK def test_authz_without_permission_denied(self): + """With the flag on, a role without ``view_enrollments`` gets ``403``.""" user = self.create_user_with_authz_role(COURSE_AUDITOR.external_key, self.course.id) assert self._get(user).status_code == status.HTTP_403_FORBIDDEN def test_authz_denied_overrides_legacy_staff(self): + """With the flag on, AuthZ denying a legacy staff user means ``403``.""" staff = StaffFactory(course_key=self.course.id) with patch(f"{HELPERS}.authz_api.is_user_allowed", return_value=False): @@ -212,6 +220,7 @@ def test_authz_denied_overrides_legacy_staff(self): assert response.status_code == status.HTTP_403_FORBIDDEN def test_legacy_staff_allowed(self): + """With the flag off, a legacy staff user can list the enrollments without querying AuthZ.""" self.set_authz_enabled(False) staff = StaffFactory(course_key=self.course.id) @@ -222,6 +231,7 @@ def test_legacy_staff_allowed(self): is_user_allowed.assert_not_called() def test_legacy_without_role_denied(self): + """With the flag off, a user without any role gets ``403``.""" self.set_authz_enabled(False) assert self._get(UserFactory()).status_code == status.HTTP_403_FORBIDDEN diff --git a/openedx/core/djangoapps/authz/tests/mixins.py b/openedx/core/djangoapps/authz/tests/mixins.py index 2c2f227875ae..01fa79fb62a1 100644 --- a/openedx/core/djangoapps/authz/tests/mixins.py +++ b/openedx/core/djangoapps/authz/tests/mixins.py @@ -12,15 +12,17 @@ from openedx.core.djangoapps.authz.tests.fixtures import seed_policies -class CourseAuthoringAuthzTestMixin: +class AuthzTestMixin: """ - Base mixin for testing AuthZ in the course authoring context. + Base mixin for tests that run with the AuthZ course authoring flag. Responsibilities: - - Enable course authoring AuthZ feature flag - - Seed policies into the AuthZ enforcer - - Provide authenticated test clients - - Provide helpers for assigning roles within a course scope + - Enable the ``authz.enable_course_authoring`` flag, and let tests change it + - Seed policies into the AuthZ enforcer and clear them afterwards + - Provide a helper to assign roles within a course scope + + The flag is enabled for every course at the start of each test. The patched + ``is_enabled`` is available as ``authz_flag_mock``. """ @classmethod @@ -30,8 +32,7 @@ def setUpClass(cls): "is_enabled", return_value=True, ) - cls.toggle_patcher.start() - cls.password = 'test' + cls.authz_flag_mock = cls.toggle_patcher.start() super().setUpClass() @classmethod @@ -41,9 +42,66 @@ def tearDownClass(cls): def setUp(self): super().setUp() - + self.set_authz_enabled(True) seed_policies() + def tearDown(self): + super().tearDown() + AuthzEnforcer.get_enforcer().clear_policy() + + def set_authz_enabled(self, enabled: bool): + """ + Enable or disable ``authz.enable_course_authoring`` for every course. + + Args: + enabled (bool): The value of the flag. + """ + self.authz_flag_mock.side_effect = None + self.authz_flag_mock.return_value = enabled + + def set_authz_enabled_for(self, *course_keys): + """ + Enable ``authz.enable_course_authoring`` only for the given courses. + + It is disabled for any other course and when no course is given. + + Args: + *course_keys (CourseKey): The courses with the flag enabled. + """ + enabled_keys = {str(course_key) for course_key in course_keys} + self.authz_flag_mock.side_effect = lambda course_key=None, **_: str(course_key) in enabled_keys + + def assign_authz_role(self, user, role, course_key): + """ + Assign an AuthZ role to the user in the course. + + The enforcer policy is reloaded afterwards. + + Args: + user (User): The user receiving the role. + role (str): The AuthZ role external key. + course_key (CourseKey): The course where the role is assigned. + """ + assign_role_to_user_in_scope(user.username, role, str(course_key)) + AuthzEnforcer.get_enforcer().load_policy() + + +class CourseAuthoringAuthzTestMixin(AuthzTestMixin): + """ + Base mixin for testing AuthZ in the course authoring context. + + Besides what ``AuthzTestMixin`` provides: + - Provide authenticated test clients + """ + + @classmethod + def setUpClass(cls): + cls.password = 'test' + super().setUpClass() + + def setUp(self): + super().setUp() + self.authorized_user = UserFactory(password=self.password) self.unauthorized_user = UserFactory(password=self.password) @@ -61,18 +119,9 @@ def setUp(self): self.staff_client = APIClient() self.staff_client.force_authenticate(user=self.staff_user) - def tearDown(self): - super().tearDown() - AuthzEnforcer.get_enforcer().clear_policy() - def add_user_to_role_in_course(self, user, role, course_key): """Helper method to add a user to a role for the course.""" - assign_role_to_user_in_scope( - user.username, - role, - str(course_key) - ) - AuthzEnforcer.get_enforcer().load_policy() + self.assign_authz_role(user, role, course_key) class CourseAuthzTestMixin(CourseAuthoringAuthzTestMixin): From 8ea80be1f79ad308fa0c07491d76fe1e06652ec9 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 17:24:05 -0500 Subject: [PATCH 05/17] fix: align the legacy checks of the Instructor Dashboard AuthZ permissions Make the default legacy check of each permission the rule of the endpoint that the MFE calls, and keep the tab visibility rules explicit. * view_course_info, view_grading_configuration and view_learner_extensions default to the VIEW_DASHBOARD permission. The Course Info and Grading tabs pass has_staff_access, as the Date Extensions tab passes has_instructor_access. * view_gradebook follows the gradebook API (course author access) and view_grading_settings follows the Studio grading settings (Studio read access). * The Grading tab is resolved with view_grading_configuration. * CourseMetadataView uses the default check of view_course_info, and documents that masquerading is resolved with that permission. * Use other permissions in the README example of a permission per HTTP method. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 6 +-- lms/djangoapps/instructor/authz/legacy.py | 43 ++++++++++++++++--- .../instructor/tests/test_api_v2_authz.py | 2 +- lms/djangoapps/instructor/views/api_v2.py | 13 +++--- .../instructor/views/serializers_v2.py | 20 ++++++++- 5 files changed, 65 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst index 9fa61001f317..0c360cb7025c 100644 --- a/lms/djangoapps/instructor/authz/README.rst +++ b/lms/djangoapps/instructor/authz/README.rst @@ -53,10 +53,10 @@ Use a mapping when reading and changing need different permissions. A method mis class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) authz_permission = { - "GET": authz_constants.COURSES_VIEW_LEARNER_EXTENSIONS, - "POST": authz_constants.COURSES_MANAGE_LEARNER_EXTENSIONS, + "GET": authz_constants.COURSES_VIEW_ENROLLMENTS, + "POST": authz_constants.COURSES_ENROLL_LEARNERS, } - permission_name = permissions.GIVE_STUDENT_EXTENSION + permission_name = permissions.CAN_ENROLL Legacy rule that is not a bridgekeeper permission ================================================= diff --git a/lms/djangoapps/instructor/authz/legacy.py b/lms/djangoapps/instructor/authz/legacy.py index 47ae239010b4..27fa2be4bc0c 100644 --- a/lms/djangoapps/instructor/authz/legacy.py +++ b/lms/djangoapps/instructor/authz/legacy.py @@ -9,6 +9,7 @@ from django.contrib.auth.models import AbstractUser from opaque_keys.edx.keys import CourseKey +from common.djangoapps.student.auth import has_course_author_access, has_studio_read_access from common.djangoapps.student.roles import GlobalStaff from lms.djangoapps.courseware.access import administrative_accesses_to_course_for_user, has_access from lms.djangoapps.discussion.django_comment_client.utils import get_user_role_names, has_forum_access @@ -114,6 +115,38 @@ def has_cohort_view_access(user: AbstractUser, course_key: CourseKey) -> bool: ) +def has_gradebook_access(user: AbstractUser, course_key: CourseKey) -> bool: + """ + Check the legacy access to the gradebook. + + It follows the same rule as the gradebook API (``course_author_access_required``). + + Args: + user (AbstractUser): The user to check. + course_key (CourseKey): The course to check. + + Returns: + bool: True with course author (Studio write) access. + """ + return bool(has_course_author_access(user, course_key)) + + +def has_grading_settings_access(user: AbstractUser, course_key: CourseKey) -> bool: + """ + Check the legacy access to the grading settings. + + It follows the same rule as the Studio grading settings (``grading_handler``). + + Args: + user (AbstractUser): The user to check. + course_key (CourseKey): The course to check. + + Returns: + bool: True with Studio read access. + """ + return bool(has_studio_read_access(user, course_key)) + + def bridgekeeper_check(permission_name: str): """ Build a legacy check from a ``bridgekeeper`` permission. @@ -139,17 +172,17 @@ def _check(user: AbstractUser, course_key: CourseKey) -> bool: # ``authz_permissions``. Endpoints whose legacy rule differs (e.g. beta testers, # which need instructor access) pass their own check. LEGACY_PERMISSION_CHECKS = { - constants.COURSES_VIEW_COURSE_INFO: has_staff_access, + constants.COURSES_VIEW_COURSE_INFO: bridgekeeper_check(permissions.VIEW_DASHBOARD), constants.COURSES_VIEW_ENROLLMENTS: bridgekeeper_check(permissions.VIEW_ENROLLMENTS), constants.COURSES_ENROLL_LEARNERS: bridgekeeper_check(permissions.CAN_ENROLL), constants.COURSES_UNENROLL_LEARNERS: bridgekeeper_check(permissions.CAN_ENROLL), constants.COURSES_VIEW_COURSE_TEAM: has_course_team_access, constants.COURSES_MANAGE_COURSE_TEAM: has_course_team_access, - constants.COURSES_VIEW_GRADEBOOK: has_staff_access, - constants.COURSES_VIEW_GRADING_CONFIGURATION: has_staff_access, - constants.COURSES_VIEW_GRADING_SETTINGS: has_staff_access, + constants.COURSES_VIEW_GRADEBOOK: has_gradebook_access, + constants.COURSES_VIEW_GRADING_CONFIGURATION: bridgekeeper_check(permissions.VIEW_DASHBOARD), + constants.COURSES_VIEW_GRADING_SETTINGS: has_grading_settings_access, constants.COURSES_MANAGE_LEARNER_GRADING: bridgekeeper_check(permissions.OVERRIDE_GRADES), - constants.COURSES_VIEW_LEARNER_EXTENSIONS: bridgekeeper_check(permissions.GIVE_STUDENT_EXTENSION), + constants.COURSES_VIEW_LEARNER_EXTENSIONS: bridgekeeper_check(permissions.VIEW_DASHBOARD), constants.COURSES_MANAGE_LEARNER_EXTENSIONS: bridgekeeper_check(permissions.GIVE_STUDENT_EXTENSION), constants.COURSES_VIEW_COHORTS: has_cohort_view_access, constants.COURSES_MANAGE_COHORTS: has_cohort_manage_access, diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py index c1429f0374ff..c4c9697de435 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2_authz.py +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -30,7 +30,7 @@ "course_info": constants.COURSES_VIEW_COURSE_INFO, "enrollments": constants.COURSES_VIEW_ENROLLMENTS, "course_team": constants.COURSES_VIEW_COURSE_TEAM, - "grading": constants.COURSES_VIEW_GRADEBOOK, + "grading": constants.COURSES_VIEW_GRADING_CONFIGURATION, "cohorts": constants.COURSES_VIEW_COHORTS, } diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index ddbf04c74a83..13e59b0989d0 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -106,7 +106,6 @@ from lms.djangoapps.instructor.authz import constants as authz_constants from lms.djangoapps.instructor.authz import legacy as authz_legacy from lms.djangoapps.instructor.authz.helpers import user_has_instructor_permission -from lms.djangoapps.instructor.authz.legacy import bridgekeeper_check from lms.djangoapps.instructor.authz.permissions import InstructorAuthzPermission from lms.djangoapps.instructor.constants import ReportType from lms.djangoapps.instructor.enrollment import ( @@ -306,14 +305,12 @@ def get(self, request, course_id): def can_view_dashboard(user): # Flag on: AuthZ ``view_course_info``. - # Flag off: the legacy dashboard permission. - return user_has_instructor_permission( - user, - authz_constants.COURSES_VIEW_COURSE_INFO, - course_key, - bridgekeeper_check(permissions.VIEW_DASHBOARD), - ) + # Flag off: the legacy dashboard permission (the default check). + return user_has_instructor_permission(user, authz_constants.COURSES_VIEW_COURSE_INFO, course_key) + # Masquerading has always required staff access. With the flag on it is + # resolved with ``view_course_info``, so any role granted that permission + # in AuthZ can masquerade, even if it is not staff. original_user_is_staff = user_has_instructor_permission( request.user, authz_constants.COURSES_VIEW_COURSE_INFO, course_key, authz_legacy.has_staff_access ) diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index a119f07be57d..a337e7510a73 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -179,9 +179,25 @@ def can(permission, legacy_check=None): # ``None`` legacy check uses the default one of the permission, which # matches the legacy tab condition. authz_tabs = [ - ('course_info', _('Course Info'), 10, authz_constants.COURSES_VIEW_COURSE_INFO, None), + # The default legacy check of ``view_course_info`` is the dashboard + # permission, but the tab has always required staff access. + ( + 'course_info', + _('Course Info'), + 10, + authz_constants.COURSES_VIEW_COURSE_INFO, + authz_legacy.has_staff_access, + ), ('enrollments', _('Enrollments'), 20, authz_constants.COURSES_VIEW_ENROLLMENTS, None), - ('grading', _('Grading'), 40, authz_constants.COURSES_VIEW_GRADEBOOK, None), + # The default legacy check of ``view_grading_configuration`` is the + # dashboard permission, but the tab has always required staff access. + ( + 'grading', + _('Grading'), + 40, + authz_constants.COURSES_VIEW_GRADING_CONFIGURATION, + authz_legacy.has_staff_access, + ), # The default legacy check of ``view_cohorts`` also lets the forum # moderators read the cohorts API, but the tab has always required # staff access. From eef709635152331f9fd49c6271f438a7a342968d Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 17:38:01 -0500 Subject: [PATCH 06/17] docs: explain the legacy check of the Date Extensions tab Co-Authored-By: Claude Sonnet 5.5 --- .../instructor/views/serializers_v2.py | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index a337e7510a73..8bb94fef6410 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -168,6 +168,7 @@ def get_tabs(self, data): # Without explicit sort_order values, there's no deterministic way to interleave # backend tabs with plugin-inserted tabs, and tab order could shift based on # load/config timing. + # The tabs below are enforced through AuthZ when # ``authz.enable_course_authoring`` is enabled for the course; # otherwise they keep the legacy role checks passed as the second @@ -182,18 +183,18 @@ def can(permission, legacy_check=None): # The default legacy check of ``view_course_info`` is the dashboard # permission, but the tab has always required staff access. ( - 'course_info', - _('Course Info'), + "course_info", + _("Course Info"), 10, authz_constants.COURSES_VIEW_COURSE_INFO, authz_legacy.has_staff_access, ), - ('enrollments', _('Enrollments'), 20, authz_constants.COURSES_VIEW_ENROLLMENTS, None), + ("enrollments", _("Enrollments"), 20, authz_constants.COURSES_VIEW_ENROLLMENTS, None), # The default legacy check of ``view_grading_configuration`` is the # dashboard permission, but the tab has always required staff access. ( - 'grading', - _('Grading'), + "grading", + _("Grading"), 40, authz_constants.COURSES_VIEW_GRADING_CONFIGURATION, authz_legacy.has_staff_access, @@ -201,16 +202,18 @@ def can(permission, legacy_check=None): # The default legacy check of ``view_cohorts`` also lets the forum # moderators read the cohorts API, but the tab has always required # staff access. - ('cohorts', _('Cohorts'), 90, authz_constants.COURSES_VIEW_COHORTS, authz_legacy.has_staff_access), + ("cohorts", _("Cohorts"), 90, authz_constants.COURSES_VIEW_COHORTS, authz_legacy.has_staff_access), ] for tab, title, sort_order, authz_perm, legacy_check in authz_tabs: if can(authz_perm, legacy_check): - tabs.append({ - 'tab_id': tab, - 'title': title, - 'url': self._build_tab_url('INSTRUCTOR_MICROFRONTEND_URL', course_key, tab), - 'sort_order': sort_order, - }) + tabs.append( + { + "tab_id": tab, + "title": title, + "url": self._build_tab_url("INSTRUCTOR_MICROFRONTEND_URL", course_key, tab), + "sort_order": sort_order, + } + ) if can(authz_constants.COURSES_VIEW_COURSE_TEAM): tabs.append({ @@ -236,6 +239,8 @@ def can(permission, legacy_check=None): } ) + # The default legacy check of ``view_learner_extensions`` is the dashboard + # permission, but the tab has always required instructor access. if is_enabled_for_course(course_key) and can( authz_constants.COURSES_VIEW_LEARNER_EXTENSIONS, authz_legacy.has_instructor_access ): From 998dc29eec0876522f0c8466e31d358949fafa59 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 23:52:22 -0500 Subject: [PATCH 07/17] feat: show Instructor Dashboard tabs with any of their AuthZ permissions A tab is now shown when the user holds at least one of the permissions validated in it, instead of a single specific permission. * Add TAB_PERMISSIONS with the permissions of each tab. The Enrollments tab includes manage_course_team because it manages the beta testers. * Add user_has_any_instructor_permission. With the flag enabled it consults only AuthZ; with it disabled it uses the explicit legacy check, or the default one of each permission. * CourseInformationSerializerV2.get_tabs resolves the tabs through it. The explicit legacy checks (staff, instructor) are preserved. * Cover the helper and every (tab, permission) pair in the tests. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/constants.py | 23 ++++- lms/djangoapps/instructor/authz/helpers.py | 33 ++++++++ .../instructor/authz/tests/test_helpers.py | 27 +++++- .../instructor/tests/test_api_v2_authz.py | 21 +++-- .../instructor/views/serializers_v2.py | 84 ++++++++----------- 5 files changed, 124 insertions(+), 64 deletions(-) diff --git a/lms/djangoapps/instructor/authz/constants.py b/lms/djangoapps/instructor/authz/constants.py index 668c2b4e6aa3..a65271f2dc08 100644 --- a/lms/djangoapps/instructor/authz/constants.py +++ b/lms/djangoapps/instructor/authz/constants.py @@ -24,7 +24,7 @@ # Grading COURSES_VIEW_GRADEBOOK = "courses.view_gradebook" COURSES_VIEW_GRADING_CONFIGURATION = "courses.view_grading_configuration" -COURSES_VIEW_GRADING_SETTINGS = "courses.view_grading_settings" # Shared with Authoring. +COURSES_VIEW_GRADING_SETTINGS = "courses.view_grading_settings" # Shared with Authoring. COURSES_MANAGE_LEARNER_GRADING = "courses.manage_learner_grading" # Date extensions @@ -53,3 +53,24 @@ COURSES_VIEW_COHORTS, COURSES_MANAGE_COHORTS, ) + +# Permissions that give access to each tab of the dashboard. A tab is shown when +# the user holds at least one of them, not a specific one. +TAB_PERMISSIONS = { + "course_info": (COURSES_VIEW_COURSE_INFO,), + "enrollments": ( + COURSES_VIEW_ENROLLMENTS, + COURSES_ENROLL_LEARNERS, + COURSES_UNENROLL_LEARNERS, + COURSES_MANAGE_COURSE_TEAM, # Its included because the tab manages the beta testers. + ), + "course_team": (COURSES_VIEW_COURSE_TEAM, COURSES_MANAGE_COURSE_TEAM), + "grading": ( + COURSES_VIEW_GRADEBOOK, + COURSES_VIEW_GRADING_CONFIGURATION, + COURSES_VIEW_GRADING_SETTINGS, + COURSES_MANAGE_LEARNER_GRADING, + ), + "date_extensions": (COURSES_VIEW_LEARNER_EXTENSIONS, COURSES_MANAGE_LEARNER_EXTENSIONS), + "cohorts": (COURSES_VIEW_COHORTS, COURSES_MANAGE_COHORTS), +} diff --git a/lms/djangoapps/instructor/authz/helpers.py b/lms/djangoapps/instructor/authz/helpers.py index 77e9f149079a..6fb5699daccb 100644 --- a/lms/djangoapps/instructor/authz/helpers.py +++ b/lms/djangoapps/instructor/authz/helpers.py @@ -65,6 +65,39 @@ def user_has_instructor_permission( return is_allowed +def user_has_any_instructor_permission( + user: AbstractUser, + authz_permissions: Iterable[str], + course_key: CourseKey, + legacy_check: LegacyCheck | None = None, +) -> bool: + """ + Return whether the user holds at least one of the AuthZ permissions. + + It is meant for the visibility of a tab, which is shown when the user can do + anything in it, not only a specific action. With the flag enabled for the + course, only AuthZ is consulted. With it disabled, ``legacy_check`` is used + if given; otherwise the default legacy check of each permission is tried. + + Args: + user (AbstractUser): The user whose access is checked. + authz_permissions (Iterable[str]): The AuthZ permission identifiers, any + of which grants access. + course_key (CourseKey): The course where the permissions are checked. + legacy_check (LegacyCheck | None): The pre-RBAC check of the whole set, + used only when AuthZ is disabled for the course. + + Returns: + bool: True if the user holds at least one of the permissions. + """ + if legacy_check is not None and not enable_authz_course_authoring(course_key): + # The legacy check covers the whole set; evaluate and log it once. + return user_has_instructor_permission(user, next(iter(authz_permissions)), course_key, legacy_check) + return any( + user_has_instructor_permission(user, permission, course_key, legacy_check) for permission in authz_permissions + ) + + def get_instructor_authz_permissions( user: AbstractUser, course_key: CourseKey, permissions: Iterable[str] = INSTRUCTOR_DASHBOARD_PERMISSIONS ) -> dict[str, bool]: diff --git a/lms/djangoapps/instructor/authz/tests/test_helpers.py b/lms/djangoapps/instructor/authz/tests/test_helpers.py index 682e0d56a1a9..e8bd4e3a1f5a 100644 --- a/lms/djangoapps/instructor/authz/tests/test_helpers.py +++ b/lms/djangoapps/instructor/authz/tests/test_helpers.py @@ -11,7 +11,11 @@ from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.instructor.authz import constants -from lms.djangoapps.instructor.authz.helpers import get_instructor_authz_permissions, user_has_instructor_permission +from lms.djangoapps.instructor.authz.helpers import ( + get_instructor_authz_permissions, + user_has_any_instructor_permission, + user_has_instructor_permission, +) from lms.djangoapps.instructor.authz.tests.mixins import InstructorAuthzTestMixin COURSE_KEY = CourseLocator("edX", "Demo", "2026") @@ -123,6 +127,27 @@ def test_logs_the_access_path(self): "Instructor dashboard permission granted via legacy", ] + def test_any_permission_flag_enabled(self): + """With the flag on, one granted permission of the set is enough.""" + self.set_authz_enabled(True) + with patch(f"{HELPERS}.authz_api.is_user_allowed", side_effect=lambda _u, perm, _s: perm == "b"): + assert user_has_any_instructor_permission(self.user, ["a", "b"], COURSE_KEY) + assert not user_has_any_instructor_permission(self.user, ["a", "c"], COURSE_KEY) + + def test_any_permission_flag_disabled_uses_default_checks(self): + """With the flag off, the default legacy check of any permission grants access.""" + self.set_authz_enabled(False) + checks = {"a": lambda *_: False, "b": lambda *_: True} + with patch.dict(f"{HELPERS}.LEGACY_PERMISSION_CHECKS", checks, clear=True): + assert user_has_any_instructor_permission(self.user, ["a", "b"], COURSE_KEY) + assert not user_has_any_instructor_permission(self.user, ["a"], COURSE_KEY) + + def test_any_permission_flag_disabled_explicit_legacy_check(self): + """With the flag off, an explicit legacy check replaces the defaults.""" + self.set_authz_enabled(False) + assert user_has_any_instructor_permission(self.user, ["a", "b"], COURSE_KEY, lambda *_: True) + assert not user_has_any_instructor_permission(self.user, ["a", "b"], COURSE_KEY, lambda *_: False) + def test_get_instructor_authz_permissions(self): """All the dashboard permissions are resolved with their effective value for the user.""" self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py index c4c9697de435..233d02a9f993 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2_authz.py +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -25,14 +25,13 @@ HELPERS = "lms.djangoapps.instructor.authz.helpers" -# Tabs migrated to AuthZ, and the permission that shows each of them. -MIGRATED_TABS = { - "course_info": constants.COURSES_VIEW_COURSE_INFO, - "enrollments": constants.COURSES_VIEW_ENROLLMENTS, - "course_team": constants.COURSES_VIEW_COURSE_TEAM, - "grading": constants.COURSES_VIEW_GRADING_CONFIGURATION, - "cohorts": constants.COURSES_VIEW_COHORTS, -} +# Tabs migrated to AuthZ, each with one of the permissions that show it. +MIGRATED_TABS = {tab: permissions[0] for tab, permissions in constants.TAB_PERMISSIONS.items()} + +# Every (tab, permission) pair: a single permission of the tab is enough to show it. +TAB_PERMISSION_PAIRS = [ + (tab, permission) for tab, permissions in constants.TAB_PERMISSIONS.items() for permission in permissions +] @ddt.ddt @@ -115,11 +114,11 @@ def test_authz_denied_overrides_legacy_roles(self): assert response.status_code == status.HTTP_403_FORBIDDEN - @ddt.data(*MIGRATED_TABS.items()) + @ddt.data(*TAB_PERMISSION_PAIRS) @ddt.unpack - def test_authz_tab_depends_on_its_permission(self, tab_id, tab_permission): + def test_authz_tab_shown_with_any_of_its_permissions(self, tab_id, tab_permission): """ - Each migrated tab needs its own permission besides the dashboard one. + A tab is shown with any one of its permissions, and hidden without all of them. """ user = UserFactory() diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index 8bb94fef6410..56751af09385 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -35,7 +35,10 @@ from lms.djangoapps.instructor.access import FORUM_ROLES, ROLES from lms.djangoapps.instructor.authz import constants as authz_constants from lms.djangoapps.instructor.authz import legacy as authz_legacy -from lms.djangoapps.instructor.authz.helpers import get_instructor_authz_permissions, user_has_instructor_permission +from lms.djangoapps.instructor.authz.helpers import ( + get_instructor_authz_permissions, + user_has_any_instructor_permission, +) from lms.djangoapps.instructor.views.instructor_dashboard import get_analytics_dashboard_message from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -169,43 +172,28 @@ def get_tabs(self, data): # backend tabs with plugin-inserted tabs, and tab order could shift based on # load/config timing. - # The tabs below are enforced through AuthZ when - # ``authz.enable_course_authoring`` is enabled for the course; - # otherwise they keep the legacy role checks passed as the second - # argument. - def can(permission, legacy_check=None): - return user_has_instructor_permission(request.user, permission, course_key, legacy_check) + # The tabs below are enforced through AuthZ when ``authz.enable_course_authoring`` + # is enabled for the course. A tab is shown when the user holds at least one of + # its permissions (``authz_constants.TAB_PERMISSIONS``). Otherwise they keep the + # legacy role check passed as the second argument, or the default ones of the + # permissions when it is ``None``. + def can_see(tab, legacy_check=None): + return user_has_any_instructor_permission( + request.user, authz_constants.TAB_PERMISSIONS[tab], course_key, legacy_check + ) - # (tab_id, title, sort_order, AuthZ permission, legacy check). A - # ``None`` legacy check uses the default one of the permission, which - # matches the legacy tab condition. + # (tab_id, title, sort_order, legacy check). The legacy check is + # explicit when the default ones of the permissions are broader than + # the tab condition, which has always required staff access. authz_tabs = [ - # The default legacy check of ``view_course_info`` is the dashboard - # permission, but the tab has always required staff access. - ( - "course_info", - _("Course Info"), - 10, - authz_constants.COURSES_VIEW_COURSE_INFO, - authz_legacy.has_staff_access, - ), - ("enrollments", _("Enrollments"), 20, authz_constants.COURSES_VIEW_ENROLLMENTS, None), - # The default legacy check of ``view_grading_configuration`` is the - # dashboard permission, but the tab has always required staff access. - ( - "grading", - _("Grading"), - 40, - authz_constants.COURSES_VIEW_GRADING_CONFIGURATION, - authz_legacy.has_staff_access, - ), - # The default legacy check of ``view_cohorts`` also lets the forum - # moderators read the cohorts API, but the tab has always required - # staff access. - ("cohorts", _("Cohorts"), 90, authz_constants.COURSES_VIEW_COHORTS, authz_legacy.has_staff_access), + ("course_info", _("Course Info"), 10, authz_legacy.has_staff_access), + ("enrollments", _("Enrollments"), 20, None), + ("grading", _("Grading"), 40, authz_legacy.has_staff_access), + ("cohorts", _("Cohorts"), 90, authz_legacy.has_staff_access), ] - for tab, title, sort_order, authz_perm, legacy_check in authz_tabs: - if can(authz_perm, legacy_check): + + for tab, title, sort_order, legacy_check in authz_tabs: + if can_see(tab, legacy_check): tabs.append( { "tab_id": tab, @@ -215,17 +203,15 @@ def can(permission, legacy_check=None): } ) - if can(authz_constants.COURSES_VIEW_COURSE_TEAM): - tabs.append({ - 'tab_id': 'course_team', - 'title': _('Course Team'), - 'url': self._build_tab_url( - 'INSTRUCTOR_MICROFRONTEND_URL', - course_key, - 'course_team' - ), - 'sort_order': 30, - }) + if can_see("course_team"): + tabs.append( + { + "tab_id": "course_team", + "title": _("Course Team"), + "url": self._build_tab_url("INSTRUCTOR_MICROFRONTEND_URL", course_key, "course_team"), + "sort_order": 30, + } + ) if access['staff'] and is_bulk_email_feature_enabled(course_key): tabs.append( @@ -239,11 +225,7 @@ def can(permission, legacy_check=None): } ) - # The default legacy check of ``view_learner_extensions`` is the dashboard - # permission, but the tab has always required instructor access. - if is_enabled_for_course(course_key) and can( - authz_constants.COURSES_VIEW_LEARNER_EXTENSIONS, authz_legacy.has_instructor_access - ): + if is_enabled_for_course(course_key) and can_see("date_extensions", authz_legacy.has_instructor_access): tabs.append({ 'tab_id': 'date_extensions', 'title': _('Date Extensions'), From bdbf834a2aec8090036c6f43f18e8807b7a4881f Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 00:10:31 -0500 Subject: [PATCH 08/17] test: enable edx-when in the Instructor Dashboard AuthZ tab tests The Date Extensions tab is only shown when edx-when is enabled for the course, so the tab tests did not see it now that it is part of the migrated tabs. * Patch is_enabled_for_course for the whole CourseMetadataViewAuthzTest. * Legacy staff does not see Date Extensions, which requires instructor access when the flag is disabled. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/tests/test_api_v2_authz.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py index 233d02a9f993..8adb679e2846 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2_authz.py +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -52,6 +52,10 @@ def setUpClass(cls): def setUp(self): super().setUp() self.client = APIClient() + # The Date Extensions tab is only shown when edx-when is enabled for the course. + patcher = patch("lms.djangoapps.instructor.views.serializers_v2.is_enabled_for_course", return_value=True) + patcher.start() + self.addCleanup(patcher.stop) self.url = reverse("instructor_api_v2:course_metadata", kwargs={"course_id": str(self.course_key)}) def _get(self, user): @@ -144,8 +148,7 @@ def test_legacy_instructor_sees_migrated_tabs(self): self.set_authz_enabled(False) instructor = InstructorFactory(course_key=self.course_key) - with patch("lms.djangoapps.instructor.views.serializers_v2.is_enabled_for_course", return_value=True): - response = self._get(instructor) + response = self._get(instructor) assert response.status_code == status.HTTP_200_OK assert "date_extensions" in self._tab_ids(response) @@ -154,7 +157,8 @@ def test_legacy_instructor_sees_migrated_tabs(self): def test_legacy_staff_has_staff_only_tabs(self): """ - Staff without the forum Administrator role does not see Course Team. + Staff without the forum Administrator role does not see Course Team, and + without instructor access does not see Date Extensions. """ self.set_authz_enabled(False) staff = StaffFactory(course_key=self.course_key) @@ -162,7 +166,7 @@ def test_legacy_staff_has_staff_only_tabs(self): response = self._get(staff) tabs = self._tab_ids(response) - assert tabs & set(MIGRATED_TABS) == set(MIGRATED_TABS) - {"course_team"} + assert tabs & set(MIGRATED_TABS) == set(MIGRATED_TABS) - {"course_team", "date_extensions"} assert not response.data["authz_permissions"][constants.COURSES_MANAGE_COURSE_TEAM] def test_legacy_does_not_query_authz(self): From c70f6cc0da23004090cd4961ea9b9390595dfedb Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 14:53:07 -0500 Subject: [PATCH 09/17] revert: drop the AuthZ migration of the Enrollments endpoint CourseEnrollmentsView goes back to the legacy InstructorPermission check, and its AuthZ tests are removed. Co-Authored-By: Claude Sonnet 5.5 --- .../instructor/tests/test_api_v2_authz.py | 62 +------------------ lms/djangoapps/instructor/views/api_v2.py | 3 +- 2 files changed, 2 insertions(+), 63 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py index 8adb679e2846..793e2a2e116f 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2_authz.py +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -8,12 +8,11 @@ import ddt from django.urls import reverse -from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_AUDITOR, COURSE_STAFF +from openedx_authz.constants.roles import COURSE_ADMIN, COURSE_STAFF from rest_framework import status from rest_framework.test import APIClient from common.djangoapps.student.tests.factories import ( - CourseEnrollmentFactory, InstructorFactory, StaffFactory, UserFactory, @@ -179,62 +178,3 @@ def test_legacy_does_not_query_authz(self): assert response.status_code == status.HTTP_200_OK is_user_allowed.assert_not_called() - - -class CourseEnrollmentsViewAuthzTest(InstructorAuthzTestMixin, SharedModuleStoreTestCase): - """ - ``CourseEnrollmentsView`` requires ``courses.view_enrollments``. - """ - - @classmethod - def setUpClass(cls): - super().setUpClass() - cls.course = CourseFactory.create() - - def setUp(self): - super().setUp() - self.client = APIClient() - self.url = reverse("instructor_api_v2:course_enrollments", kwargs={"course_id": str(self.course.id)}) - CourseEnrollmentFactory(user=UserFactory(), course_id=self.course.id, is_active=True) - - def _get(self, user): - self.client.force_authenticate(user=user) - return self.client.get(self.url) - - def test_authz_course_staff_allowed(self): - """With the flag on, a Course Staff can list the enrollments.""" - user = self.create_user_with_authz_role(COURSE_STAFF.external_key, self.course.id) - - assert self._get(user).status_code == status.HTTP_200_OK - - def test_authz_without_permission_denied(self): - """With the flag on, a role without ``view_enrollments`` gets ``403``.""" - user = self.create_user_with_authz_role(COURSE_AUDITOR.external_key, self.course.id) - - assert self._get(user).status_code == status.HTTP_403_FORBIDDEN - - def test_authz_denied_overrides_legacy_staff(self): - """With the flag on, AuthZ denying a legacy staff user means ``403``.""" - staff = StaffFactory(course_key=self.course.id) - - with patch(f"{HELPERS}.authz_api.is_user_allowed", return_value=False): - response = self._get(staff) - - assert response.status_code == status.HTTP_403_FORBIDDEN - - def test_legacy_staff_allowed(self): - """With the flag off, a legacy staff user can list the enrollments without querying AuthZ.""" - self.set_authz_enabled(False) - staff = StaffFactory(course_key=self.course.id) - - with patch(f"{HELPERS}.authz_api.is_user_allowed") as is_user_allowed: - response = self._get(staff) - - assert response.status_code == status.HTTP_200_OK - is_user_allowed.assert_not_called() - - def test_legacy_without_role_denied(self): - """With the flag off, a user without any role gets ``403``.""" - self.set_authz_enabled(False) - - assert self._get(UserFactory()).status_code == status.HTTP_403_FORBIDDEN diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index 13e59b0989d0..9cc82bb28cb9 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -2488,8 +2488,7 @@ class CourseEnrollmentsView(DeveloperErrorViewMixin, ListAPIView): * 401: Unauthorized - User is not authenticated * 403: Forbidden - User lacks instructor permissions """ - permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permission = authz_constants.COURSES_VIEW_ENROLLMENTS + permission_classes = (IsAuthenticated, permissions.InstructorPermission) permission_name = permissions.VIEW_ENROLLMENTS serializer_class = CourseEnrollmentSerializerV2 filter_backends = [DjangoFilterBackend] From e1cf3518a1bd5b1eb9c65d28936564fe7a4aa53a Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 16:19:52 -0500 Subject: [PATCH 10/17] feat: show the Instructor dashboard with any of its AuthZ permissions With authz.enable_course_authoring enabled, the Instructor course tab of the LMS and CourseMetadataView grant access when the user holds at least one Instructor Dashboard permission, instead of only view_course_info. With the flag disabled they keep requiring the legacy VIEW_DASHBOARD permission. InstructorAuthzPermission accepts a collection of permissions in authz_permission, and bridgekeeper_check evaluates the permission against the course key without loading the course. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 12 ++++ lms/djangoapps/instructor/authz/legacy.py | 3 +- .../instructor/authz/permissions.py | 15 ++-- .../authz/tests/test_permissions.py | 41 +++++++++-- .../instructor/tests/test_api_v2_authz.py | 72 +++++++++++++++++++ lms/djangoapps/instructor/views/api_v2.py | 16 +++-- .../instructor/views/instructor_dashboard.py | 23 ++++-- 7 files changed, 160 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst index 0c360cb7025c..b2a57adad159 100644 --- a/lms/djangoapps/instructor/authz/README.rst +++ b/lms/djangoapps/instructor/authz/README.rst @@ -58,6 +58,18 @@ Use a mapping when reading and changing need different permissions. A method mis } permission_name = permissions.CAN_ENROLL +Any of several permissions +========================== + +Declare a collection when holding any one of them is enough. With the flag disabled the legacy check of the view decides, evaluated once: + +.. code-block:: python + + class MyView(APIView): + permission_classes = (IsAuthenticated, InstructorAuthzPermission) + authz_permission = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS + permission_name = permissions.VIEW_DASHBOARD + Legacy rule that is not a bridgekeeper permission ================================================= diff --git a/lms/djangoapps/instructor/authz/legacy.py b/lms/djangoapps/instructor/authz/legacy.py index 27fa2be4bc0c..5c906b6e6aea 100644 --- a/lms/djangoapps/instructor/authz/legacy.py +++ b/lms/djangoapps/instructor/authz/legacy.py @@ -20,7 +20,6 @@ FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_MODERATOR, ) -from openedx.core.lib.courses import get_course_by_id def has_staff_access(user: AbstractUser, course_key: CourseKey) -> bool: @@ -163,7 +162,7 @@ def bridgekeeper_check(permission_name: str): """ def _check(user: AbstractUser, course_key: CourseKey) -> bool: - return user.has_perm(permission_name, get_course_by_id(course_key)) + return user.has_perm(permission_name, course_key) return _check diff --git a/lms/djangoapps/instructor/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py index 2f6491c8f91c..4bf82b9428b5 100644 --- a/lms/djangoapps/instructor/authz/permissions.py +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -10,7 +10,7 @@ from opaque_keys.edx.keys import CourseKey from rest_framework.permissions import BasePermission -from lms.djangoapps.instructor.authz.helpers import user_has_instructor_permission +from lms.djangoapps.instructor.authz.helpers import user_has_any_instructor_permission, user_has_instructor_permission from lms.djangoapps.instructor.authz.legacy import bridgekeeper_check @@ -24,10 +24,11 @@ class InstructorAuthzPermission(BasePermission): With the flag enabled only AuthZ is consulted; legacy roles are ignored. Attributes expected in the view: - authz_permission (str | Mapping[str, str]): The AuthZ permission - identifier required, or a mapping of HTTP method to identifier when - the view needs a different permission per method. Methods missing - from the mapping are checked with the legacy permission only. + authz_permission (str | Collection[str] | Mapping[str, str | Collection[str]]): + The AuthZ permission identifier required, a collection of them when + holding any one is enough, or a mapping of HTTP method to either + when the view needs a different permission per method. Methods + missing from the mapping are checked with the legacy permission only. permission_name (str): The legacy ``bridgekeeper`` permission, the one ``InstructorPermission`` uses. It is enforced when ``authz.enable_course_authoring`` is disabled for the course. @@ -71,4 +72,6 @@ def has_permission(self, request, view): if authz_permission is None: return bool(legacy_check and legacy_check(request.user, course_key)) - return user_has_instructor_permission(request.user, authz_permission, course_key, legacy_check) + if isinstance(authz_permission, str): + return user_has_instructor_permission(request.user, authz_permission, course_key, legacy_check) + return user_has_any_instructor_permission(request.user, authz_permission, course_key, legacy_check) diff --git a/lms/djangoapps/instructor/authz/tests/test_permissions.py b/lms/djangoapps/instructor/authz/tests/test_permissions.py index 5d899acf7b4d..995c699f9d70 100644 --- a/lms/djangoapps/instructor/authz/tests/test_permissions.py +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -51,6 +51,40 @@ def test_authz_enabled_grants_access_by_authz_role(self): assert InstructorAuthzPermission().has_permission(self._request(), view) + def test_authz_collection_grants_access_with_any_permission(self): + """With the flag on, a collection of permissions grants access when the user holds any one of them.""" + view = FakeView( + authz_permission=(constants.COURSES_VIEW_ENROLLMENTS, constants.COURSES_VIEW_GRADEBOOK), + legacy_permission_check=lambda *_: False, + ) + + with patch( + "lms.djangoapps.instructor.authz.helpers.authz_api.is_user_allowed", + side_effect=lambda _u, permission, _s: permission == constants.COURSES_VIEW_GRADEBOOK, + ): + assert InstructorAuthzPermission().has_permission(self._request(), view) + + def test_authz_collection_denied_without_any_permission(self): + """With the flag on, a collection of permissions denies a user that holds none of them.""" + view = FakeView( + authz_permission=(constants.COURSES_VIEW_ENROLLMENTS, constants.COURSES_VIEW_GRADEBOOK), + legacy_permission_check=lambda *_: True, + ) + + assert not InstructorAuthzPermission().has_permission(self._request(), view) + + def test_authz_disabled_collection_uses_legacy_check_once(self): + """With the flag off, a collection of permissions is resolved by the view legacy check alone.""" + self.set_authz_enabled(False) + legacy_check = Mock(return_value=True) + view = FakeView( + authz_permission=(constants.COURSES_VIEW_ENROLLMENTS, constants.COURSES_VIEW_GRADEBOOK), + legacy_permission_check=legacy_check, + ) + + assert InstructorAuthzPermission().has_permission(self._request(), view) + legacy_check.assert_called_once_with(self.user, COURSE_KEY) + def test_authz_enabled_ignores_legacy(self): """With the flag on, a passing legacy check does not grant access without an AuthZ role.""" view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: True) @@ -71,12 +105,9 @@ def test_authz_disabled_uses_bridgekeeper_permission_name(self): """With the flag off and no legacy check, ``permission_name`` is evaluated as a bridgekeeper permission.""" self.set_authz_enabled(False) view = FakeView(authz_permission=constants.COURSES_VIEW_ENROLLMENTS, permission_name="instructor.dashboard") - with ( - patch("lms.djangoapps.instructor.authz.legacy.get_course_by_id", return_value="course"), - patch.object(type(self.user), "has_perm", return_value=True) as has_perm, - ): + with patch.object(type(self.user), "has_perm", return_value=True) as has_perm: assert InstructorAuthzPermission().has_permission(self._request(), view) - has_perm.assert_called_once_with("instructor.dashboard", "course") + has_perm.assert_called_once_with("instructor.dashboard", COURSE_KEY) @ddt.data(("get", True), ("post", False)) @ddt.unpack diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py index 793e2a2e116f..4eae8a36b12b 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2_authz.py +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -17,6 +17,7 @@ StaffFactory, UserFactory, ) +from lms.djangoapps.courseware.tabs import get_course_tab_list from lms.djangoapps.instructor.authz import constants from lms.djangoapps.instructor.authz.tests.mixins import InstructorAuthzTestMixin from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -117,6 +118,16 @@ def test_authz_denied_overrides_legacy_roles(self): assert response.status_code == status.HTTP_403_FORBIDDEN + @ddt.data(*constants.INSTRUCTOR_DASHBOARD_PERMISSIONS) + def test_authz_any_dashboard_permission_grants_access(self, permission): + """With the flag on, a single dashboard permission, not only ``view_course_info``, grants access.""" + user = UserFactory() + + with patch(f"{HELPERS}.authz_api.is_user_allowed", side_effect=lambda _u, perm, _s: perm == permission): + response = self._get(user) + + assert response.status_code == status.HTTP_200_OK + @ddt.data(*TAB_PERMISSION_PAIRS) @ddt.unpack def test_authz_tab_shown_with_any_of_its_permissions(self, tab_id, tab_permission): @@ -178,3 +189,64 @@ def test_legacy_does_not_query_authz(self): assert response.status_code == status.HTTP_200_OK is_user_allowed.assert_not_called() + + +@ddt.ddt +class InstructorDashboardTabAuthzTest(InstructorAuthzTestMixin, SharedModuleStoreTestCase): + """ + The "Instructor" course tab of the LMS follows the AuthZ flag. + + With the flag on it is shown when the user holds at least one of the Instructor + Dashboard permissions, and through the legacy roles when it is off. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create() + + def _has_instructor_tab(self, user): + return any(tab.type == "instructor" for tab in get_course_tab_list(user, self.course)) + + def test_authz_role_with_permissions_shown(self): + """With the flag on, a role that holds dashboard permissions sees the tab.""" + user = self.create_user_with_authz_role(COURSE_STAFF.external_key, self.course.id) + + assert self._has_instructor_tab(user) + + @ddt.data(*constants.INSTRUCTOR_DASHBOARD_PERMISSIONS) + def test_authz_any_permission_shown(self, permission): + """With the flag on, a single dashboard permission is enough.""" + user = UserFactory() + + with patch(f"{HELPERS}.authz_api.is_user_allowed", side_effect=lambda _u, perm, _c: perm == permission): + assert self._has_instructor_tab(user) + + def test_authz_without_permissions_hidden(self): + """With the flag on, a user holding none of the permissions does not see the tab.""" + user = UserFactory() + + assert not self._has_instructor_tab(user) + + def test_authz_denied_overrides_legacy_staff(self): + """With the flag on, legacy staff without AuthZ permissions does not see the tab.""" + staff = StaffFactory(course_key=self.course.id) + + with patch(f"{HELPERS}.authz_api.is_user_allowed", return_value=False): + assert not self._has_instructor_tab(staff) + + def test_legacy_staff_shown(self): + """With the flag off, a legacy staff user sees the tab without querying AuthZ.""" + self.set_authz_enabled(False) + staff = StaffFactory(course_key=self.course.id) + + with patch(f"{HELPERS}.authz_api.is_user_allowed") as is_user_allowed: + assert self._has_instructor_tab(staff) + + is_user_allowed.assert_not_called() + + def test_legacy_without_role_hidden(self): + """With the flag off, a user without any role does not see the tab.""" + self.set_authz_enabled(False) + + assert not self._has_instructor_tab(UserFactory()) diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index 9cc82bb28cb9..b3d9a2d29cb4 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -105,7 +105,7 @@ ) from lms.djangoapps.instructor.authz import constants as authz_constants from lms.djangoapps.instructor.authz import legacy as authz_legacy -from lms.djangoapps.instructor.authz.helpers import user_has_instructor_permission +from lms.djangoapps.instructor.authz.helpers import user_has_any_instructor_permission, user_has_instructor_permission from lms.djangoapps.instructor.authz.permissions import InstructorAuthzPermission from lms.djangoapps.instructor.constants import ReportType from lms.djangoapps.instructor.enrollment import ( @@ -209,7 +209,8 @@ class CourseMetadataView(DeveloperErrorViewMixin, APIView): """ permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permission = authz_constants.COURSES_VIEW_COURSE_INFO + # Holding any of the dashboard permissions is enough, as for the "Instructor" course tab. + authz_permission = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS permission_name = permissions.VIEW_DASHBOARD @extend_schema( @@ -304,9 +305,14 @@ def get(self, request, course_id): course = get_course_by_id(course_key) def can_view_dashboard(user): - # Flag on: AuthZ ``view_course_info``. - # Flag off: the legacy dashboard permission (the default check). - return user_has_instructor_permission(user, authz_constants.COURSES_VIEW_COURSE_INFO, course_key) + # Flag on: any of the dashboard AuthZ permissions. + # Flag off: the legacy dashboard permission. + return user_has_any_instructor_permission( + user, + authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS, + course_key, + authz_legacy.bridgekeeper_check(permissions.VIEW_DASHBOARD), + ) # Masquerading has always required staff access. With the flag on it is # resolved with ``view_course_info``, so any role granted that permission diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index af05056ef9f8..1901ae00d602 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -46,6 +46,9 @@ from lms.djangoapps.courseware.masquerade import get_masquerade_role from lms.djangoapps.discussion.django_comment_client.utils import has_forum_access from lms.djangoapps.grades.api import is_writable_gradebook_enabled +from lms.djangoapps.instructor.authz import constants as authz_constants +from lms.djangoapps.instructor.authz import legacy as authz_legacy +from lms.djangoapps.instructor.authz.helpers import user_has_any_instructor_permission from lms.djangoapps.instructor.constants import INSTRUCTOR_DASHBOARD_PLUGIN_VIEW_NAME from openedx.core.djangoapps.course_groups.cohorts import DEFAULT_COHORT_NAME, get_course_cohorts, is_course_cohorted from openedx.core.djangoapps.discussions.config.waffle_utils import legacy_discussion_experience_enabled @@ -90,11 +93,23 @@ def link_func(course, reverse_func): @classmethod def is_enabled(cls, course, user=None): """ - Returns true if the specified user has staff access. + Returns true if the user can see the Instructor Dashboard. + + With ``authz.enable_course_authoring`` enabled for the course, the tab is shown when + the user holds at least one of the Instructor Dashboard AuthZ permissions. Otherwise + the legacy ``VIEW_DASHBOARD`` permission is required. """ - return bool(user and user.is_authenticated and - get_masquerade_role(user, course.id) != 'student' and - user.has_perm(permissions.VIEW_DASHBOARD, course.id)) + return bool( + user + and user.is_authenticated + and get_masquerade_role(user, course.id) != "student" + and user_has_any_instructor_permission( + user, + authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS, + course.id, + authz_legacy.bridgekeeper_check(permissions.VIEW_DASHBOARD), + ) + ) def show_analytics_dashboard_message(course_key): From 4d84445347db81e2cfa8b063c27c766050c6f6c7 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 17:04:08 -0500 Subject: [PATCH 11/17] test: adapt the non-existent course test to the permission check The permission check no longer loads the course, so a user without access gets 403 for a non-existent course and only global staff reaches the view and its 404. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/tests/test_api_v2.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/tests/test_api_v2.py b/lms/djangoapps/instructor/tests/test_api_v2.py index 753cffd00251..a046fc183f0a 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2.py +++ b/lms/djangoapps/instructor/tests/test_api_v2.py @@ -266,8 +266,11 @@ def test_get_course_metadata_invalid_course_id(self): def test_get_course_metadata_nonexistent_course(self): """ Test error handling for non-existent course. + + The permission check does not load the course, so only a user that passes it + (global staff) reaches the view and gets the 404. """ - self.client.force_authenticate(user=self.instructor) + self.client.force_authenticate(user=self.admin) nonexistent_course_id = 'course-v1:edX+NonExistent+2024' response = self.client.get(self._get_url(course_id=nonexistent_course_id)) @@ -275,6 +278,15 @@ def test_get_course_metadata_nonexistent_course(self): error_code = "Course not found: course-v1:edX+NonExistent+2024." self.assertEqual(response.data['developer_message'], error_code) # noqa: PT009 + def test_get_course_metadata_nonexistent_course_without_access(self): + """ + A user without access to a non-existent course gets 403, as for any other course. + """ + self.client.force_authenticate(user=self.instructor) + response = self.client.get(self._get_url(course_id='course-v1:edX+NonExistent+2024')) + + assert response.status_code == status.HTTP_403_FORBIDDEN + def test_instructor_permissions_reflected(self): """ Test that instructor permissions are correctly reflected in response. From 2f1cee5e805795441208251b29003c2495734e37 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 21 Oct 2026 11:24:03 -0500 Subject: [PATCH 12/17] revert: keep masquerade on the legacy staff check Masquerading is not migrated to AuthZ yet, so the Instructor Dashboard metadata view requires legacy staff access again, whatever the authz.enable_course_authoring flag. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/views/api_v2.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index b3d9a2d29cb4..a9d6b1d978d2 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -105,7 +105,7 @@ ) from lms.djangoapps.instructor.authz import constants as authz_constants from lms.djangoapps.instructor.authz import legacy as authz_legacy -from lms.djangoapps.instructor.authz.helpers import user_has_any_instructor_permission, user_has_instructor_permission +from lms.djangoapps.instructor.authz.helpers import user_has_any_instructor_permission from lms.djangoapps.instructor.authz.permissions import InstructorAuthzPermission from lms.djangoapps.instructor.constants import ReportType from lms.djangoapps.instructor.enrollment import ( @@ -314,12 +314,8 @@ def can_view_dashboard(user): authz_legacy.bridgekeeper_check(permissions.VIEW_DASHBOARD), ) - # Masquerading has always required staff access. With the flag on it is - # resolved with ``view_course_info``, so any role granted that permission - # in AuthZ can masquerade, even if it is not staff. - original_user_is_staff = user_has_instructor_permission( - request.user, authz_constants.COURSES_VIEW_COURSE_INFO, course_key, authz_legacy.has_staff_access - ) + # Masquerading is not migrated to AuthZ yet: it keeps requiring legacy staff access. + original_user_is_staff = has_access(request.user, 'staff', course_key).has_access _, request.user = setup_masquerade( request, course_key, From 79efda0536ed826bf9aa6d0ef405218d23404a47 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:29:47 -0500 Subject: [PATCH 13/17] feat: log the legacy-only fallback of InstructorAuthzPermission Views or methods without an AuthZ permission silently fall back to the legacy check. Log them to track what is left to migrate. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/permissions.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lms/djangoapps/instructor/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py index 4bf82b9428b5..95f0e04bf68f 100644 --- a/lms/djangoapps/instructor/authz/permissions.py +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -2,6 +2,7 @@ DRF permission class to enforce AuthZ in the Instructor Dashboard views. """ +import logging from collections.abc import Mapping from django.core.exceptions import ImproperlyConfigured @@ -13,6 +14,8 @@ from lms.djangoapps.instructor.authz.helpers import user_has_any_instructor_permission, user_has_instructor_permission from lms.djangoapps.instructor.authz.legacy import bridgekeeper_check +log = logging.getLogger(__name__) + class InstructorAuthzPermission(BasePermission): """ @@ -71,6 +74,13 @@ def has_permission(self, request, view): legacy_check = bridgekeeper_check(view.permission_name) if authz_permission is None: + # Not migrated to AuthZ yet for this method; log it to track what is left. + log.info( + "%s %s has no AuthZ permission, using the legacy check only", + type(view).__name__, + request.method, + extra={"course_key": str(course_key)}, + ) return bool(legacy_check and legacy_check(request.user, course_key)) if isinstance(authz_permission, str): return user_has_instructor_permission(request.user, authz_permission, course_key, legacy_check) From c524e8f359fa381d7fa499c6ca19cb717de6d73b Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:31:30 -0500 Subject: [PATCH 14/17] refactor: log the AuthZ decisions at debug level Checking a set of permissions logs one line per permission, which is too noisy at info level. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/helpers.py | 2 +- lms/djangoapps/instructor/authz/tests/test_helpers.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/authz/helpers.py b/lms/djangoapps/instructor/authz/helpers.py index 6fb5699daccb..eb9d5a4a102d 100644 --- a/lms/djangoapps/instructor/authz/helpers.py +++ b/lms/djangoapps/instructor/authz/helpers.py @@ -131,7 +131,7 @@ def _log_decision(path: str, is_allowed: bool, user: AbstractUser, authz_permiss authz_permission (str): The AuthZ permission identifier checked. course_key (CourseKey): The course where it was checked. """ - log.info( + log.debug( "Instructor dashboard permission %s via %s", "granted" if is_allowed else "denied", path, diff --git a/lms/djangoapps/instructor/authz/tests/test_helpers.py b/lms/djangoapps/instructor/authz/tests/test_helpers.py index e8bd4e3a1f5a..d5a68283b148 100644 --- a/lms/djangoapps/instructor/authz/tests/test_helpers.py +++ b/lms/djangoapps/instructor/authz/tests/test_helpers.py @@ -116,7 +116,7 @@ def test_logs_the_access_path(self): """Each decision is logged with the path (``authz`` or ``legacy``) that resolved it.""" self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) - with self.assertLogs(HELPERS, level="INFO") as logs: + with self.assertLogs(HELPERS, level="DEBUG") as logs: user_has_instructor_permission(self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY) self.set_authz_enabled(False) user_has_instructor_permission(self.user, constants.COURSES_VIEW_ENROLLMENTS, COURSE_KEY, lambda *_: True) From 2364150a899aca51186761160bdd131b06a3b5b5 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:32:44 -0500 Subject: [PATCH 15/17] refactor: drop the unused forum_admin access from the course tabs Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/views/serializers_v2.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index 56751af09385..e45bd37ea447 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -159,7 +159,6 @@ def get_tabs(self, data): 'finance_admin': CourseFinanceAdminRole(course_key).has_user(request.user), 'sales_admin': CourseSalesAdminRole(course_key).has_user(request.user), 'staff': bool(has_access(request.user, 'staff', course)), - 'forum_admin': has_forum_access(request.user, course_key, FORUM_ROLE_ADMINISTRATOR), 'data_researcher': request.user.has_perm(permissions.CAN_RESEARCH, course_key), } tabs = [] From 089988af0bdee67486fc40f0987cd1df1192a124 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:47:12 -0500 Subject: [PATCH 16/17] refactor: build the legacy dashboard check once Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/views/api_v2.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index a9d6b1d978d2..467ba0f84290 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -179,6 +179,9 @@ VALID_TEAM_ROLES = frozenset(ROLES.keys()) | frozenset(FORUM_ROLES) +# Legacy check of the dashboard +LEGACY_VIEW_DASHBOARD_CHECK = authz_legacy.bridgekeeper_check(permissions.VIEW_DASHBOARD) + # Shared OpenAPI parameters, each used identically by several endpoints below. COURSE_ID_PATH_PARAMETER = OpenApiParameter( 'course_id', @@ -311,7 +314,7 @@ def can_view_dashboard(user): user, authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS, course_key, - authz_legacy.bridgekeeper_check(permissions.VIEW_DASHBOARD), + LEGACY_VIEW_DASHBOARD_CHECK, ) # Masquerading is not migrated to AuthZ yet: it keeps requiring legacy staff access. From 7d48414d92b0b27056691db37e8f9143bdf17eb2 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:48:29 -0500 Subject: [PATCH 17/17] refactor: import the shared AuthZ permissions from openedx-authz Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/constants.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/instructor/authz/constants.py b/lms/djangoapps/instructor/authz/constants.py index a65271f2dc08..8249de509b88 100644 --- a/lms/djangoapps/instructor/authz/constants.py +++ b/lms/djangoapps/instructor/authz/constants.py @@ -3,12 +3,14 @@ The ``courses.manage_course_team``, ``courses.view_course_team`` and ``courses.view_grading_settings`` permissions are shared with Authoring and -defined by openedx-authz. +imported from openedx-authz. TEMPORARY: the rest depend on a separate issue that will define them in openedx-authz. They are declared here only for testing. """ +from openedx_authz.constants import permissions as authz_permissions + # Course info COURSES_VIEW_COURSE_INFO = "courses.view_course_info" @@ -18,13 +20,13 @@ COURSES_UNENROLL_LEARNERS = "courses.unenroll_learners" # Course team (shared with Authoring) -COURSES_VIEW_COURSE_TEAM = "courses.view_course_team" -COURSES_MANAGE_COURSE_TEAM = "courses.manage_course_team" +COURSES_VIEW_COURSE_TEAM = authz_permissions.COURSES_VIEW_COURSE_TEAM.identifier +COURSES_MANAGE_COURSE_TEAM = authz_permissions.COURSES_MANAGE_COURSE_TEAM.identifier # Grading COURSES_VIEW_GRADEBOOK = "courses.view_gradebook" COURSES_VIEW_GRADING_CONFIGURATION = "courses.view_grading_configuration" -COURSES_VIEW_GRADING_SETTINGS = "courses.view_grading_settings" # Shared with Authoring. +COURSES_VIEW_GRADING_SETTINGS = authz_permissions.COURSES_VIEW_GRADING_SETTINGS.identifier COURSES_MANAGE_LEARNER_GRADING = "courses.manage_learner_grading" # Date extensions