From 9e3480838568f88ecec99e0861a1f1bf63f6c14f Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 16 Oct 2026 13:27:06 -0500 Subject: [PATCH 01/21] 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 3439caf5f16187a2d5d266bb012970852c1d3c4a Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 16 Oct 2026 17:10:45 -0500 Subject: [PATCH 02/21] 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 57206d632cd8807b5569f084b571104541a27ce6 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 13:14:40 -0500 Subject: [PATCH 03/21] 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 c3e27fee90b91dbdfd4c85b144213e4bd00253a9 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 14:14:02 -0500 Subject: [PATCH 04/21] 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 7b3ecabc5c7ec52c636f49f39bab6250af71bc78 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 17:24:05 -0500 Subject: [PATCH 05/21] 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 0b898218056d09cc7c75322ea5f2abfc4504137d Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 17:38:01 -0500 Subject: [PATCH 06/21] 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 56daa450e8b5bdad8f72a9ce45b2bc39536f2cc7 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 19 Oct 2026 23:52:22 -0500 Subject: [PATCH 07/21] 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 25c487ec2b967196c733dd5e2445902c2e09a945 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 00:10:31 -0500 Subject: [PATCH 08/21] 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 94f31b12f0ae4fd36394710ee41ecde9b0c73d7b Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 14:53:07 -0500 Subject: [PATCH 09/21] 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 99dd45e1e248eaa938109b5650c0a9e87eead9c8 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 16:19:52 -0500 Subject: [PATCH 10/21] 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 6519e5ab0b9523ece3beb49a03b268ed40641c6e Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Tue, 20 Oct 2026 17:04:08 -0500 Subject: [PATCH 11/21] 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 7b2ab7307840254875c4f39c8b1d1e8f69e6a8eb Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Wed, 21 Oct 2026 11:24:03 -0500 Subject: [PATCH 12/21] 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 9cf3fcb8fb145a88b34580b89a7f7f7ffd99668a Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:29:47 -0500 Subject: [PATCH 13/21] 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 0d1fdd39651a4c8ad744d38311027743dc3761bc Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:31:30 -0500 Subject: [PATCH 14/21] 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 bfb3a9639960c0283561d653acf3e5866d5ebea7 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:32:44 -0500 Subject: [PATCH 15/21] 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 7ade6a1e8620d8a7018fb9d7b0cfa5de0818171a Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:47:12 -0500 Subject: [PATCH 16/21] 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 ebfa5edee9a91c0099c1958feddf7d5dc4fb7e2e Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Thu, 22 Oct 2026 10:48:29 -0500 Subject: [PATCH 17/21] 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 From 84408cbf69237438da1dc5f205c9bee17158577e Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 23 Oct 2026 09:42:51 -0500 Subject: [PATCH 18/21] refactor: address review feedback on naming and docs - Rename the view attribute authz_permission to authz_permissions, since it can hold a collection or a mapping. - Say "pre-AuthZ" instead of "pre-RBAC" in the instructor AuthZ files. - Name authz.enable_course_authoring instead of "the flag". - Fix the CourseTeamPermission citation, a typo and a README heading. - Deny instead of raising StopIteration when user_has_any_instructor_permission gets an empty collection. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 26 ++++++++-------- lms/djangoapps/instructor/authz/constants.py | 2 +- lms/djangoapps/instructor/authz/helpers.py | 17 ++++++---- lms/djangoapps/instructor/authz/legacy.py | 5 +-- .../instructor/authz/permissions.py | 31 ++++++++++--------- .../instructor/authz/tests/test_helpers.py | 7 +++++ .../authz/tests/test_permissions.py | 22 ++++++------- lms/djangoapps/instructor/views/api_v2.py | 2 +- 8 files changed, 64 insertions(+), 48 deletions(-) diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst index b2a57adad159..cb77044d5472 100644 --- a/lms/djangoapps/instructor/authz/README.rst +++ b/lms/djangoapps/instructor/authz/README.rst @@ -9,7 +9,7 @@ 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. +- ``legacy.py``: the pre-AuthZ check equivalent to each permission. - ``tests/mixins.py``: ``InstructorAuthzTestMixin`` to assign AuthZ roles in tests. Behavior @@ -17,7 +17,7 @@ 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 enabled:** only AuthZ is consulted. Legacy roles are ignored (openedx-authz already grants the global staff and superuser bypass). The only exception is an HTTP method missing from a mapping of ``authz_permissions``, which is checked with the legacy rule (see `Different permissions per HTTP method`_). - **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``. @@ -39,20 +39,20 @@ The same AuthZ permission for every HTTP method, with the legacy bridgekeeper pe class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permission = authz_constants.COURSES_VIEW_ENROLLMENTS + authz_permissions = authz_constants.COURSES_VIEW_ENROLLMENTS # The legacy bridgekeeper permission. permission_name = permissions.VIEW_ENROLLMENTS -Different permission per HTTP method -==================================== +Different permissions 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: +Use a mapping when reading and changing need different permissions. Each value can be a single permission or a collection of them. A method missing from the mapping skips AuthZ and is checked with the legacy rule only, even with ``authz.enable_course_authoring`` enabled for the course. This is meant as a temporary state while the method is migrated, and it is logged: .. code-block:: python class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permission = { + authz_permissions = { "GET": authz_constants.COURSES_VIEW_ENROLLMENTS, "POST": authz_constants.COURSES_ENROLL_LEARNERS, } @@ -61,13 +61,13 @@ Use a mapping when reading and changing need different permissions. A method mis 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: +Declare a collection when holding any one of them is enough. With ``authz.enable_course_authoring`` disabled for the course 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 + authz_permissions = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS permission_name = permissions.VIEW_DASHBOARD Legacy rule that is not a bridgekeeper permission @@ -81,7 +81,7 @@ Define ``legacy_permission_check(user, course_key)`` on the view. It replaces `` class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permission = { + authz_permissions = { "GET": authz_constants.COURSES_VIEW_COHORTS, "POST": authz_constants.COURSES_MANAGE_COHORTS, } @@ -107,7 +107,7 @@ Use ``user_has_instructor_permission`` inside a handler, a serializer or any oth 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. + # With an explicit legacy check, used only when ``authz.enable_course_authoring`` is disabled. user_has_instructor_permission( request.user, authz_constants.COURSES_VIEW_ENROLLMENTS, @@ -120,6 +120,6 @@ To get the effective value of several permissions at once (as ``authz_permission Errors ====== -- No ``authz_permission`` on the view: ``ImproperlyConfigured``. +- No ``authz_permissions`` on the view: ``ImproperlyConfigured``. - Invalid ``course_id``: ``404``. -- Denied by the active path (AuthZ with the flag enabled, legacy otherwise): ``403``. +- Denied by the active path (AuthZ with ``authz.enable_course_authoring`` enabled, legacy otherwise): ``403``. diff --git a/lms/djangoapps/instructor/authz/constants.py b/lms/djangoapps/instructor/authz/constants.py index 8249de509b88..525d39ab009f 100644 --- a/lms/djangoapps/instructor/authz/constants.py +++ b/lms/djangoapps/instructor/authz/constants.py @@ -64,7 +64,7 @@ COURSES_VIEW_ENROLLMENTS, COURSES_ENROLL_LEARNERS, COURSES_UNENROLL_LEARNERS, - COURSES_MANAGE_COURSE_TEAM, # Its included because the tab manages the beta testers. + COURSES_MANAGE_COURSE_TEAM, # It's included because the tab manages the beta testers. ), "course_team": (COURSES_VIEW_COURSE_TEAM, COURSES_MANAGE_COURSE_TEAM), "grading": ( diff --git a/lms/djangoapps/instructor/authz/helpers.py b/lms/djangoapps/instructor/authz/helpers.py index eb9d5a4a102d..f32f917b06f5 100644 --- a/lms/djangoapps/instructor/authz/helpers.py +++ b/lms/djangoapps/instructor/authz/helpers.py @@ -4,7 +4,7 @@ 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. +check is used, so the pre-AuthZ behavior is preserved. """ import logging @@ -46,7 +46,7 @@ def user_has_instructor_permission( 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 + legacy_check (LegacyCheck | None): The pre-AuthZ 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. @@ -74,9 +74,11 @@ def user_has_any_instructor_permission( """ Return whether the user holds at least one of the AuthZ permissions. + An empty collection grants no access. + 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 + anything in it, not only a specific action. With ``authz.enable_course_authoring`` + 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: @@ -84,15 +86,18 @@ def user_has_any_instructor_permission( 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, + legacy_check (LegacyCheck | None): The pre-AuthZ 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. """ + authz_permissions = tuple(authz_permissions) + if not authz_permissions: + return False 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 user_has_instructor_permission(user, authz_permissions[0], course_key, legacy_check) return any( user_has_instructor_permission(user, permission, course_key, legacy_check) for permission in authz_permissions ) diff --git a/lms/djangoapps/instructor/authz/legacy.py b/lms/djangoapps/instructor/authz/legacy.py index 5c906b6e6aea..305f93a3ce3f 100644 --- a/lms/djangoapps/instructor/authz/legacy.py +++ b/lms/djangoapps/instructor/authz/legacy.py @@ -1,5 +1,5 @@ """ -Legacy (pre-RBAC) checks of the Instructor Dashboard AuthZ permissions. +Legacy (pre-AuthZ) 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. @@ -54,7 +54,8 @@ 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``. + It follows the same rule as ``CourseTeamPermission`` + (``lms.djangoapps.instructor.permissions``). Args: user (AbstractUser): The user to check. diff --git a/lms/djangoapps/instructor/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py index 95f0e04bf68f..181260adda2e 100644 --- a/lms/djangoapps/instructor/authz/permissions.py +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -24,14 +24,17 @@ class InstructorAuthzPermission(BasePermission): It is a drop-in replacement of ``lms.djangoapps.instructor.permissions.InstructorPermission``. - With the flag enabled only AuthZ is consulted; legacy roles are ignored. + With ``authz.enable_course_authoring`` enabled for the course only AuthZ is + consulted; legacy roles are ignored, except for the HTTP methods missing from + a mapping of ``authz_permissions``, which are checked with the legacy rule. Attributes expected in the view: - authz_permission (str | Collection[str] | Mapping[str, str | Collection[str]]): + authz_permissions (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. + missing from the mapping are checked with the legacy permission only, + even with ``authz.enable_course_authoring`` enabled. permission_name (str): The legacy ``bridgekeeper`` permission, the one ``InstructorPermission`` uses. It is enforced when ``authz.enable_course_authoring`` is disabled for the course. @@ -46,7 +49,7 @@ def has_permission(self, request, view): Args: request (Request): The DRF request. - view (APIView): The view, with ``authz_permission`` and the legacy + view (APIView): The view, with ``authz_permissions`` and the legacy permission attributes. Returns: @@ -56,24 +59,24 @@ def has_permission(self, request, view): Http404: If the ``course_id`` kwarg of the view is not a valid course key. ImproperlyConfigured: If the view does not define - ``authz_permission``. + ``authz_permissions``. """ 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) + authz_permissions = getattr(view, "authz_permissions", None) + if authz_permissions is None: + raise ImproperlyConfigured(f"{type(view).__name__} must define 'authz_permissions'.") + if isinstance(authz_permissions, Mapping): + authz_permissions = authz_permissions.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: + if authz_permissions 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", @@ -82,6 +85,6 @@ def has_permission(self, request, view): 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) - return user_has_any_instructor_permission(request.user, authz_permission, course_key, legacy_check) + if isinstance(authz_permissions, str): + return user_has_instructor_permission(request.user, authz_permissions, course_key, legacy_check) + return user_has_any_instructor_permission(request.user, authz_permissions, course_key, legacy_check) diff --git a/lms/djangoapps/instructor/authz/tests/test_helpers.py b/lms/djangoapps/instructor/authz/tests/test_helpers.py index d5a68283b148..4ed95904c0b1 100644 --- a/lms/djangoapps/instructor/authz/tests/test_helpers.py +++ b/lms/djangoapps/instructor/authz/tests/test_helpers.py @@ -148,6 +148,13 @@ def test_any_permission_flag_disabled_explicit_legacy_check(self): 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_any_permission_empty_collection_denies(self): + """An empty collection denies instead of raising, whatever the flag and the legacy check.""" + for enabled in (True, False): + self.set_authz_enabled(enabled) + assert not user_has_any_instructor_permission(self.user, [], COURSE_KEY) + assert not user_has_any_instructor_permission(self.user, [], COURSE_KEY, lambda *_: True) + 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/authz/tests/test_permissions.py b/lms/djangoapps/instructor/authz/tests/test_permissions.py index 995c699f9d70..76f377c6ec6e 100644 --- a/lms/djangoapps/instructor/authz/tests/test_permissions.py +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -47,14 +47,14 @@ def _request(self, method="get"): 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) + view = FakeView(authz_permissions=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: False) 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), + authz_permissions=(constants.COURSES_VIEW_ENROLLMENTS, constants.COURSES_VIEW_GRADEBOOK), legacy_permission_check=lambda *_: False, ) @@ -67,7 +67,7 @@ def test_authz_collection_grants_access_with_any_permission(self): 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), + authz_permissions=(constants.COURSES_VIEW_ENROLLMENTS, constants.COURSES_VIEW_GRADEBOOK), legacy_permission_check=lambda *_: True, ) @@ -78,7 +78,7 @@ def test_authz_disabled_collection_uses_legacy_check_once(self): self.set_authz_enabled(False) legacy_check = Mock(return_value=True) view = FakeView( - authz_permission=(constants.COURSES_VIEW_ENROLLMENTS, constants.COURSES_VIEW_GRADEBOOK), + authz_permissions=(constants.COURSES_VIEW_ENROLLMENTS, constants.COURSES_VIEW_GRADEBOOK), legacy_permission_check=legacy_check, ) @@ -87,7 +87,7 @@ def test_authz_disabled_collection_uses_legacy_check_once(self): 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) + view = FakeView(authz_permissions=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: True) assert not InstructorAuthzPermission().has_permission(self._request(), view) @@ -96,7 +96,7 @@ 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) + view = FakeView(authz_permissions=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) @@ -104,7 +104,7 @@ def test_authz_disabled_uses_legacy_permission_check(self): 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") + view = FakeView(authz_permissions=constants.COURSES_VIEW_ENROLLMENTS, permission_name="instructor.dashboard") 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_KEY) @@ -115,7 +115,7 @@ 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={ + authz_permissions={ "GET": constants.COURSES_VIEW_COURSE_TEAM, # Course Staff does not hold it. "POST": constants.COURSES_MANAGE_COURSE_TEAM, @@ -127,19 +127,19 @@ def test_permission_per_http_method(self, method, 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 + authz_permissions={"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): """An invalid ``course_id`` raises ``Http404``.""" - view = FakeView(course_id="not-a-course", authz_permission=constants.COURSES_VIEW_ENROLLMENTS) + view = FakeView(course_id="not-a-course", authz_permissions=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``.""" + """A view that does not define ``authz_permissions`` raises ``ImproperlyConfigured``.""" with pytest.raises(ImproperlyConfigured): InstructorAuthzPermission().has_permission(self._request(), FakeView()) diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index 467ba0f84290..fcbd09589bd9 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -213,7 +213,7 @@ class CourseMetadataView(DeveloperErrorViewMixin, APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) # Holding any of the dashboard permissions is enough, as for the "Instructor" course tab. - authz_permission = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS + authz_permissions = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS permission_name = permissions.VIEW_DASHBOARD @extend_schema( From cc98eff65db76cec6d3e6ab26b31ed5938ed02f9 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 23 Oct 2026 09:43:11 -0500 Subject: [PATCH 19/21] feat: deny the HTTP methods missing from an authz_permissions mapping With authz.enable_course_authoring enabled for the course, a method that is not in the mapping used to fall back to the legacy rule, so a missed key silently kept the method outside AuthZ. It is now denied. With the flag disabled the legacy check decides as before. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 4 ++-- .../instructor/authz/permissions.py | 24 ++++++++++--------- .../authz/tests/test_permissions.py | 14 +++++++++-- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst index cb77044d5472..47c7c2ad018e 100644 --- a/lms/djangoapps/instructor/authz/README.rst +++ b/lms/djangoapps/instructor/authz/README.rst @@ -17,7 +17,7 @@ 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). The only exception is an HTTP method missing from a mapping of ``authz_permissions``, which is checked with the legacy rule (see `Different permissions per HTTP method`_). +- **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``. @@ -46,7 +46,7 @@ The same AuthZ permission for every HTTP method, with the legacy bridgekeeper pe Different permissions per HTTP method ===================================== -Use a mapping when reading and changing need different permissions. Each value can be a single permission or a collection of them. A method missing from the mapping skips AuthZ and is checked with the legacy rule only, even with ``authz.enable_course_authoring`` enabled for the course. This is meant as a temporary state while the method is migrated, and it is logged: +Use a mapping when reading and changing need different permissions. Each value can be a single permission or a collection of them. A method missing from the mapping is denied (``403``) with ``authz.enable_course_authoring`` enabled for the course, so a method is never left on the legacy rule by mistake. With it disabled, the legacy check decides as usual: .. code-block:: python diff --git a/lms/djangoapps/instructor/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py index 181260adda2e..0c8eba4385f8 100644 --- a/lms/djangoapps/instructor/authz/permissions.py +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -11,6 +11,7 @@ from opaque_keys.edx.keys import CourseKey from rest_framework.permissions import BasePermission +from common.djangoapps.student.roles import enable_authz_course_authoring 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 @@ -25,16 +26,15 @@ class InstructorAuthzPermission(BasePermission): ``lms.djangoapps.instructor.permissions.InstructorPermission``. With ``authz.enable_course_authoring`` enabled for the course only AuthZ is - consulted; legacy roles are ignored, except for the HTTP methods missing from - a mapping of ``authz_permissions``, which are checked with the legacy rule. + consulted; legacy roles are ignored. Attributes expected in the view: authz_permissions (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, - even with ``authz.enable_course_authoring`` enabled. + missing from the mapping are denied with + ``authz.enable_course_authoring`` enabled for the course. permission_name (str): The legacy ``bridgekeeper`` permission, the one ``InstructorPermission`` uses. It is enforced when ``authz.enable_course_authoring`` is disabled for the course. @@ -77,13 +77,15 @@ def has_permission(self, request, view): legacy_check = bridgekeeper_check(view.permission_name) if authz_permissions 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)}, - ) + if enable_authz_course_authoring(course_key): + # Fail closed: a method without an AuthZ permission is never allowed by AuthZ. + log.warning( + "%s %s has no AuthZ permission, denying access", + type(view).__name__, + request.method, + extra={"course_key": str(course_key)}, + ) + return False return bool(legacy_check and legacy_check(request.user, course_key)) if isinstance(authz_permissions, str): return user_has_instructor_permission(request.user, authz_permissions, 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 76f377c6ec6e..afe9315d1557 100644 --- a/lms/djangoapps/instructor/authz/tests/test_permissions.py +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -124,8 +124,18 @@ 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.""" + def test_method_missing_from_mapping_denied_with_flag_enabled(self): + """With the flag on, a method missing from the mapping is denied even if the legacy rule allows it.""" + self.set_authz_enabled(True) + view = FakeView( + authz_permissions={"GET": constants.COURSES_VIEW_COURSE_TEAM}, legacy_permission_check=lambda *_: True + ) + + assert not InstructorAuthzPermission().has_permission(self._request("delete"), view) + + def test_method_missing_from_mapping_uses_legacy_with_flag_disabled(self): + """With the flag off, a method missing from the mapping is checked with the legacy rule.""" + self.set_authz_enabled(False) view = FakeView( authz_permissions={"GET": constants.COURSES_VIEW_COURSE_TEAM}, legacy_permission_check=lambda *_: True ) From 9af73c3becc5e766950f72529d8cf3cc913f1d05 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Fri, 23 Oct 2026 10:05:24 -0500 Subject: [PATCH 20/21] refactor: make authz_permissions always a collection Like permission_classes, the view attribute is now a collection of permissions (or a mapping of HTTP method to a collection), even for a single one. A bare string raises ImproperlyConfigured, since it would otherwise be iterated character by character. Unlike permission_classes, where all the classes must pass (AND), holding any one of the permissions is enough (OR); the docs say so. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 39 +++++++++---------- .../instructor/authz/permissions.py | 38 +++++++++++++----- .../authz/tests/test_permissions.py | 28 ++++++++----- 3 files changed, 65 insertions(+), 40 deletions(-) diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst index 47c7c2ad018e..9c21d75b8d76 100644 --- a/lms/djangoapps/instructor/authz/README.rst +++ b/lms/djangoapps/instructor/authz/README.rst @@ -27,10 +27,10 @@ Migrating a view 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 -================= +Same permissions for every HTTP method +====================================== -The same AuthZ permission for every HTTP method, with the legacy bridgekeeper permission the view already used: +``authz_permissions`` is always a collection, like ``permission_classes``, even for a single permission. Unlike ``permission_classes``, where all the classes must pass (AND), holding **any one** of the permissions is enough (OR). Declare it with the legacy bridgekeeper permission the view already used: .. code-block:: python @@ -39,36 +39,33 @@ The same AuthZ permission for every HTTP method, with the legacy bridgekeeper pe class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permissions = authz_constants.COURSES_VIEW_ENROLLMENTS + authz_permissions = (authz_constants.COURSES_VIEW_ENROLLMENTS,) # The legacy bridgekeeper permission. permission_name = permissions.VIEW_ENROLLMENTS -Different permissions per HTTP method -===================================== - -Use a mapping when reading and changing need different permissions. Each value can be a single permission or a collection of them. A method missing from the mapping is denied (``403``) with ``authz.enable_course_authoring`` enabled for the course, so a method is never left on the legacy rule by mistake. With it disabled, the legacy check decides as usual: +A bare string raises ``ImproperlyConfigured``. With several permissions and ``authz.enable_course_authoring`` disabled for the course, the legacy check of the view decides, evaluated once: .. code-block:: python class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permissions = { - "GET": authz_constants.COURSES_VIEW_ENROLLMENTS, - "POST": authz_constants.COURSES_ENROLL_LEARNERS, - } - permission_name = permissions.CAN_ENROLL + authz_permissions = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS + permission_name = permissions.VIEW_DASHBOARD -Any of several permissions -========================== +Different permissions per HTTP method +===================================== -Declare a collection when holding any one of them is enough. With ``authz.enable_course_authoring`` disabled for the course the legacy check of the view decides, evaluated once: +Use a mapping when reading and changing need different permissions. Each value is a collection of permissions. A method missing from the mapping is denied (``403``) with ``authz.enable_course_authoring`` enabled for the course, so a method is never left on the legacy rule by mistake. With it disabled, the legacy check decides as usual: .. code-block:: python class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) - authz_permissions = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS - permission_name = permissions.VIEW_DASHBOARD + authz_permissions = { + "GET": (authz_constants.COURSES_VIEW_ENROLLMENTS,), + "POST": (authz_constants.COURSES_ENROLL_LEARNERS,), + } + permission_name = permissions.CAN_ENROLL Legacy rule that is not a bridgekeeper permission ================================================= @@ -82,8 +79,8 @@ Define ``legacy_permission_check(user, course_key)`` on the view. It replaces `` class MyView(APIView): permission_classes = (IsAuthenticated, InstructorAuthzPermission) authz_permissions = { - "GET": authz_constants.COURSES_VIEW_COHORTS, - "POST": authz_constants.COURSES_MANAGE_COHORTS, + "GET": (authz_constants.COURSES_VIEW_COHORTS,), + "POST": (authz_constants.COURSES_MANAGE_COHORTS,), } @staticmethod @@ -120,6 +117,6 @@ To get the effective value of several permissions at once (as ``authz_permission Errors ====== -- No ``authz_permissions`` on the view: ``ImproperlyConfigured``. +- No ``authz_permissions`` on the view, or a bare string instead of a collection: ``ImproperlyConfigured``. - Invalid ``course_id``: ``404``. - Denied by the active path (AuthZ with ``authz.enable_course_authoring`` enabled, legacy otherwise): ``403``. diff --git a/lms/djangoapps/instructor/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py index 0c8eba4385f8..529fb8e999b1 100644 --- a/lms/djangoapps/instructor/authz/permissions.py +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -12,7 +12,7 @@ from rest_framework.permissions import BasePermission from common.djangoapps.student.roles import enable_authz_course_authoring -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.legacy import bridgekeeper_check log = logging.getLogger(__name__) @@ -29,12 +29,15 @@ class InstructorAuthzPermission(BasePermission): consulted; legacy roles are ignored. Attributes expected in the view: - authz_permissions (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 denied with - ``authz.enable_course_authoring`` enabled for the course. + authz_permissions (Collection[str] | Mapping[str, Collection[str]]): + The AuthZ permission identifiers required, or a mapping of HTTP + method to them when the view needs different permissions per + method. It is always a collection, even for a single permission, + like ``permission_classes``. Unlike ``permission_classes``, where + all the classes must pass (AND), holding any one of the + permissions is enough (OR). Methods missing from the mapping are + denied with ``authz.enable_course_authoring`` enabled for the + course. permission_name (str): The legacy ``bridgekeeper`` permission, the one ``InstructorPermission`` uses. It is enforced when ``authz.enable_course_authoring`` is disabled for the course. @@ -59,7 +62,8 @@ def has_permission(self, request, view): Http404: If the ``course_id`` kwarg of the view is not a valid course key. ImproperlyConfigured: If the view does not define - ``authz_permissions``. + ``authz_permissions``, or defines a bare string instead of a + collection. """ try: course_key = CourseKey.from_string(view.kwargs.get("course_id")) @@ -69,6 +73,7 @@ def has_permission(self, request, view): authz_permissions = getattr(view, "authz_permissions", None) if authz_permissions is None: raise ImproperlyConfigured(f"{type(view).__name__} must define 'authz_permissions'.") + _validate_authz_permissions(view, authz_permissions) if isinstance(authz_permissions, Mapping): authz_permissions = authz_permissions.get(request.method) @@ -87,6 +92,19 @@ def has_permission(self, request, view): ) return False return bool(legacy_check and legacy_check(request.user, course_key)) - if isinstance(authz_permissions, str): - return user_has_instructor_permission(request.user, authz_permissions, course_key, legacy_check) return user_has_any_instructor_permission(request.user, authz_permissions, course_key, legacy_check) + + +def _validate_authz_permissions(view, authz_permissions): + """ + Reject a bare string where a collection is expected. + + A string is iterable, so a forgotten comma in ``(PERMISSION)`` would be + read character by character instead of failing. + """ + values = authz_permissions.values() if isinstance(authz_permissions, Mapping) else (authz_permissions,) + if any(isinstance(value, str) for value in values): + raise ImproperlyConfigured( + f"{type(view).__name__}.authz_permissions must be a collection of permissions, " + "e.g. (PERMISSION,), not a string." + ) diff --git a/lms/djangoapps/instructor/authz/tests/test_permissions.py b/lms/djangoapps/instructor/authz/tests/test_permissions.py index afe9315d1557..4b50f5d5d4b3 100644 --- a/lms/djangoapps/instructor/authz/tests/test_permissions.py +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -47,7 +47,9 @@ def _request(self, method="get"): 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_permissions=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: False) + view = FakeView( + authz_permissions=(constants.COURSES_VIEW_ENROLLMENTS,), legacy_permission_check=lambda *_: False + ) assert InstructorAuthzPermission().has_permission(self._request(), view) @@ -87,7 +89,9 @@ def test_authz_disabled_collection_uses_legacy_check_once(self): 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_permissions=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=lambda *_: True) + view = FakeView( + authz_permissions=(constants.COURSES_VIEW_ENROLLMENTS,), legacy_permission_check=lambda *_: True + ) assert not InstructorAuthzPermission().has_permission(self._request(), view) @@ -96,7 +100,7 @@ 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_permissions=constants.COURSES_VIEW_ENROLLMENTS, legacy_permission_check=legacy_check) + view = FakeView(authz_permissions=(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) @@ -104,7 +108,7 @@ def test_authz_disabled_uses_legacy_permission_check(self): 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_permissions=constants.COURSES_VIEW_ENROLLMENTS, permission_name="instructor.dashboard") + view = FakeView(authz_permissions=(constants.COURSES_VIEW_ENROLLMENTS,), permission_name="instructor.dashboard") 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_KEY) @@ -116,9 +120,9 @@ def test_permission_per_http_method(self, method, expected): self.assign_authz_role(self.user, COURSE_STAFF.external_key, COURSE_KEY) view = FakeView( authz_permissions={ - "GET": constants.COURSES_VIEW_COURSE_TEAM, + "GET": (constants.COURSES_VIEW_COURSE_TEAM,), # Course Staff does not hold it. - "POST": constants.COURSES_MANAGE_COURSE_TEAM, + "POST": (constants.COURSES_MANAGE_COURSE_TEAM,), } ) @@ -128,7 +132,7 @@ def test_method_missing_from_mapping_denied_with_flag_enabled(self): """With the flag on, a method missing from the mapping is denied even if the legacy rule allows it.""" self.set_authz_enabled(True) view = FakeView( - authz_permissions={"GET": constants.COURSES_VIEW_COURSE_TEAM}, legacy_permission_check=lambda *_: True + authz_permissions={"GET": (constants.COURSES_VIEW_COURSE_TEAM,)}, legacy_permission_check=lambda *_: True ) assert not InstructorAuthzPermission().has_permission(self._request("delete"), view) @@ -137,18 +141,24 @@ def test_method_missing_from_mapping_uses_legacy_with_flag_disabled(self): """With the flag off, a method missing from the mapping is checked with the legacy rule.""" self.set_authz_enabled(False) view = FakeView( - authz_permissions={"GET": constants.COURSES_VIEW_COURSE_TEAM}, legacy_permission_check=lambda *_: True + authz_permissions={"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): """An invalid ``course_id`` raises ``Http404``.""" - view = FakeView(course_id="not-a-course", authz_permissions=constants.COURSES_VIEW_ENROLLMENTS) + view = FakeView(course_id="not-a-course", authz_permissions=(constants.COURSES_VIEW_ENROLLMENTS,)) with pytest.raises(Http404): InstructorAuthzPermission().has_permission(self._request(), view) + @ddt.data(constants.COURSES_VIEW_ENROLLMENTS, {"GET": constants.COURSES_VIEW_ENROLLMENTS}) + def test_bare_string_raises(self, authz_permissions): + """A string instead of a collection, alone or in a mapping, raises ``ImproperlyConfigured``.""" + with pytest.raises(ImproperlyConfigured): + InstructorAuthzPermission().has_permission(self._request(), FakeView(authz_permissions=authz_permissions)) + def test_view_without_authz_permission(self): """A view that does not define ``authz_permissions`` raises ``ImproperlyConfigured``.""" with pytest.raises(ImproperlyConfigured): From df2efbe8925d1fa9ed058537243c1a5329360189 Mon Sep 17 00:00:00 2001 From: Bryann Valderrama Date: Mon, 26 Oct 2026 16:45:48 -0500 Subject: [PATCH 21/21] refactor: address review feedback on the Instructor Dashboard AuthZ helpers - Rename the authz helpers module to api.py, as the public interface the dashboard tabs import. - Run the explicit legacy check of a permission set directly instead of through its first permission, and log the whole set. - Number the view migration cases in the README. - Document can_view_dashboard and why masquerading keeps the legacy staff check. - Assert the exact tabs in the AuthZ tests and give the tests clearer names. - Mark the Instructor Dashboard policy seeding as temporary. - Drop the redundant policy reload when assigning an AuthZ role in tests. Co-Authored-By: Claude Sonnet 5.5 --- lms/djangoapps/instructor/authz/README.rst | 18 +++++----- .../instructor/authz/{helpers.py => api.py} | 21 ++++++----- .../instructor/authz/permissions.py | 2 +- .../instructor/authz/tests/mixins.py | 5 ++- .../tests/{test_helpers.py => test_api.py} | 26 ++++++++++---- .../authz/tests/test_permissions.py | 2 +- .../instructor/tests/test_api_v2_authz.py | 35 +++++++++---------- lms/djangoapps/instructor/views/api_v2.py | 13 +++++-- .../instructor/views/instructor_dashboard.py | 2 +- .../instructor/views/serializers_v2.py | 2 +- openedx/core/djangoapps/authz/tests/mixins.py | 3 -- 11 files changed, 76 insertions(+), 53 deletions(-) rename lms/djangoapps/instructor/authz/{helpers.py => api.py} (86%) rename lms/djangoapps/instructor/authz/tests/{test_helpers.py => test_api.py} (86%) diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst index 9c21d75b8d76..09804c9657a6 100644 --- a/lms/djangoapps/instructor/authz/README.rst +++ b/lms/djangoapps/instructor/authz/README.rst @@ -7,7 +7,7 @@ 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``. +- ``api.py``: ``user_has_instructor_permission`` and ``get_instructor_authz_permissions``. - ``permissions.py``: ``InstructorAuthzPermission``, the DRF permission class for the views. - ``legacy.py``: the pre-AuthZ check equivalent to each permission. - ``tests/mixins.py``: ``InstructorAuthzTestMixin`` to assign AuthZ roles in tests. @@ -25,10 +25,10 @@ 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. ``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. +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. Pick the case that matches the view: -Same permissions for every HTTP method -====================================== +Case 1: Same permissions for every HTTP method +============================================== ``authz_permissions`` is always a collection, like ``permission_classes``, even for a single permission. Unlike ``permission_classes``, where all the classes must pass (AND), holding **any one** of the permissions is enough (OR). Declare it with the legacy bridgekeeper permission the view already used: @@ -52,8 +52,8 @@ A bare string raises ``ImproperlyConfigured``. With several permissions and ``au authz_permissions = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS permission_name = permissions.VIEW_DASHBOARD -Different permissions per HTTP method -===================================== +Case 2: Different permissions per HTTP method +============================================= Use a mapping when reading and changing need different permissions. Each value is a collection of permissions. A method missing from the mapping is denied (``403``) with ``authz.enable_course_authoring`` enabled for the course, so a method is never left on the legacy rule by mistake. With it disabled, the legacy check decides as usual: @@ -67,8 +67,8 @@ Use a mapping when reading and changing need different permissions. Each value i } permission_name = permissions.CAN_ENROLL -Legacy rule that is not a bridgekeeper permission -================================================= +Case 3: 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: @@ -97,7 +97,7 @@ Use ``user_has_instructor_permission`` inside a handler, a serializer or any oth .. code-block:: python from opaque_keys.edx.keys import CourseKey - from lms.djangoapps.instructor.authz.helpers import user_has_instructor_permission + from lms.djangoapps.instructor.authz.api import user_has_instructor_permission def post(self, request, course_id): course_key = CourseKey.from_string(course_id) diff --git a/lms/djangoapps/instructor/authz/helpers.py b/lms/djangoapps/instructor/authz/api.py similarity index 86% rename from lms/djangoapps/instructor/authz/helpers.py rename to lms/djangoapps/instructor/authz/api.py index f32f917b06f5..cfc699d4dadb 100644 --- a/lms/djangoapps/instructor/authz/helpers.py +++ b/lms/djangoapps/instructor/authz/api.py @@ -1,5 +1,5 @@ """ -Helpers to enforce AuthZ permissions in the Instructor Dashboard. +Public API 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 @@ -56,12 +56,12 @@ def user_has_instructor_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) + _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) + _log_decision(ACCESS_PATH_LEGACY, is_allowed, user, (authz_permission,), course_key) return is_allowed @@ -96,8 +96,11 @@ def user_has_any_instructor_permission( if not authz_permissions: return False 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, authz_permissions[0], course_key, legacy_check) + # The legacy check covers the whole set, so it does not depend on which + # permission is evaluated: run and log it once instead of once per permission. + is_allowed = bool(legacy_check(user, course_key)) + _log_decision(ACCESS_PATH_LEGACY, is_allowed, user, authz_permissions, course_key) + return is_allowed return any( user_has_instructor_permission(user, permission, course_key, legacy_check) for permission in authz_permissions ) @@ -125,7 +128,9 @@ def get_instructor_authz_permissions( 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): +def _log_decision( + path: str, is_allowed: bool, user: AbstractUser, authz_permissions: Iterable[str], course_key: CourseKey +): """ Log the path that resolved a permission check, and its outcome. @@ -133,7 +138,7 @@ def _log_decision(path: str, is_allowed: bool, user: AbstractUser, authz_permiss 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. + authz_permissions (Iterable[str]): The AuthZ permission identifiers checked. course_key (CourseKey): The course where it was checked. """ log.debug( @@ -142,7 +147,7 @@ def _log_decision(path: str, is_allowed: bool, user: AbstractUser, authz_permiss path, extra={ "user_id": user.id, - "authz_permission": authz_permission, + "authz_permissions": list(authz_permissions), "course_key": str(course_key), "access_path": path, }, diff --git a/lms/djangoapps/instructor/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py index 529fb8e999b1..f0dc8c4ae25b 100644 --- a/lms/djangoapps/instructor/authz/permissions.py +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -12,7 +12,7 @@ from rest_framework.permissions import BasePermission from common.djangoapps.student.roles import enable_authz_course_authoring -from lms.djangoapps.instructor.authz.helpers import user_has_any_instructor_permission +from lms.djangoapps.instructor.authz.api import user_has_any_instructor_permission from lms.djangoapps.instructor.authz.legacy import bridgekeeper_check log = logging.getLogger(__name__) diff --git a/lms/djangoapps/instructor/authz/tests/mixins.py b/lms/djangoapps/instructor/authz/tests/mixins.py index 8dc2eabe5046..58da3eb99d1e 100644 --- a/lms/djangoapps/instructor/authz/tests/mixins.py +++ b/lms/djangoapps/instructor/authz/tests/mixins.py @@ -36,7 +36,10 @@ 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. + TEMPORARY: openedx-authz has no API to grant a permission to a role, so the + policies are added straight to the enforcer. Once the Instructor Dashboard + permissions are assigned to the default roles in the openedx-authz static + schema (loaded with ``load_authz_schema``), this helper is removed. """ enforcer = AuthzEnforcer.get_enforcer() for role, permissions in INSTRUCTOR_DASHBOARD_ROLE_PERMISSIONS.items(): diff --git a/lms/djangoapps/instructor/authz/tests/test_helpers.py b/lms/djangoapps/instructor/authz/tests/test_api.py similarity index 86% rename from lms/djangoapps/instructor/authz/tests/test_helpers.py rename to lms/djangoapps/instructor/authz/tests/test_api.py index 4ed95904c0b1..ad86c3719e8e 100644 --- a/lms/djangoapps/instructor/authz/tests/test_helpers.py +++ b/lms/djangoapps/instructor/authz/tests/test_api.py @@ -1,5 +1,5 @@ """ -Tests for the Instructor Dashboard AuthZ helpers. +Tests for the Instructor Dashboard AuthZ API. """ from unittest.mock import Mock, patch @@ -11,7 +11,7 @@ from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.instructor.authz import constants -from lms.djangoapps.instructor.authz.helpers import ( +from lms.djangoapps.instructor.authz.api import ( get_instructor_authz_permissions, user_has_any_instructor_permission, user_has_instructor_permission, @@ -19,7 +19,7 @@ from lms.djangoapps.instructor.authz.tests.mixins import InstructorAuthzTestMixin COURSE_KEY = CourseLocator("edX", "Demo", "2026") -HELPERS = "lms.djangoapps.instructor.authz.helpers" +API = "lms.djangoapps.instructor.authz.api" @ddt.ddt @@ -88,7 +88,7 @@ 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}): + with patch.dict(f"{API}.LEGACY_PERMISSION_CHECKS", {constants.COURSES_VIEW_GRADEBOOK: default_check}): allowed = user_has_instructor_permission(self.user, constants.COURSES_VIEW_GRADEBOOK, COURSE_KEY) assert allowed @@ -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="DEBUG") as logs: + with self.assertLogs(API, 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) @@ -130,7 +130,7 @@ def test_logs_the_access_path(self): 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"): + with patch(f"{API}.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) @@ -138,7 +138,7 @@ 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): + with patch.dict(f"{API}.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) @@ -148,6 +148,18 @@ def test_any_permission_flag_disabled_explicit_legacy_check(self): 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_any_permission_flag_disabled_explicit_legacy_check_runs_once(self): + """With the flag off, an explicit legacy check covers the whole set: it runs and is logged once.""" + self.set_authz_enabled(False) + legacy_check = Mock(return_value=False) + + with self.assertLogs(API, level="DEBUG") as logs: + assert not user_has_any_instructor_permission(self.user, ["a", "b", "c"], COURSE_KEY, legacy_check) + + legacy_check.assert_called_once_with(self.user, COURSE_KEY) + assert [record.access_path for record in logs.records] == ["legacy"] + assert logs.records[0].authz_permissions == ["a", "b", "c"] + def test_any_permission_empty_collection_denies(self): """An empty collection denies instead of raising, whatever the flag and the legacy check.""" for enabled in (True, False): diff --git a/lms/djangoapps/instructor/authz/tests/test_permissions.py b/lms/djangoapps/instructor/authz/tests/test_permissions.py index 4b50f5d5d4b3..0a162cca232f 100644 --- a/lms/djangoapps/instructor/authz/tests/test_permissions.py +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -61,7 +61,7 @@ def test_authz_collection_grants_access_with_any_permission(self): ) with patch( - "lms.djangoapps.instructor.authz.helpers.authz_api.is_user_allowed", + "lms.djangoapps.instructor.authz.api.authz_api.is_user_allowed", side_effect=lambda _u, permission, _s: permission == constants.COURSES_VIEW_GRADEBOOK, ): assert InstructorAuthzPermission().has_permission(self._request(), view) diff --git a/lms/djangoapps/instructor/tests/test_api_v2_authz.py b/lms/djangoapps/instructor/tests/test_api_v2_authz.py index 4eae8a36b12b..d91337e65b08 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2_authz.py +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -23,7 +23,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -HELPERS = "lms.djangoapps.instructor.authz.helpers" +API = "lms.djangoapps.instructor.authz.api" # 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()} @@ -82,25 +82,24 @@ def test_existing_permissions_field_is_unchanged(self): "data_researcher", } - def test_authz_course_admin(self): - """A Course Admin sees every migrated tab and holds every permission.""" + def test_authz_course_admin_sees_migrated_tabs_and_holds_every_permission(self): + """A Course Admin sees exactly the migrated tabs and holds every permission.""" 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 self._tab_ids(response) == set(MIGRATED_TABS) assert set(response.data["authz_permissions"]) == set(constants.INSTRUCTOR_DASHBOARD_PERMISSIONS) 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.""" + def test_authz_course_staff_sees_migrated_tabs_but_cannot_manage_course_team(self): + """A Course Staff sees exactly the migrated tabs but cannot manage the course team.""" 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 + assert self._tab_ids(response) == set(MIGRATED_TABS) permissions = response.data["authz_permissions"] assert permissions[constants.COURSES_VIEW_COURSE_TEAM] # Course Staff does not manage the course team. @@ -113,7 +112,7 @@ def test_authz_denied_overrides_legacy_roles(self): """ instructor = InstructorFactory(course_key=self.course_key) - with patch(f"{HELPERS}.authz_api.is_user_allowed", return_value=False): + with patch(f"{API}.authz_api.is_user_allowed", return_value=False): response = self._get(instructor) assert response.status_code == status.HTTP_403_FORBIDDEN @@ -123,7 +122,7 @@ 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): + with patch(f"{API}.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 @@ -140,11 +139,11 @@ 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) + f"{API}.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", + f"{API}.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)) @@ -184,7 +183,7 @@ 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: + with patch(f"{API}.authz_api.is_user_allowed") as is_user_allowed: response = self._get(instructor) assert response.status_code == status.HTTP_200_OK @@ -215,11 +214,11 @@ def test_authz_role_with_permissions_shown(self): 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.""" + def test_authz_instructor_course_tab_shown_with_any_dashboard_permission(self, permission): + """With the flag on, a single dashboard permission is enough to show the "Instructor" course tab.""" user = UserFactory() - with patch(f"{HELPERS}.authz_api.is_user_allowed", side_effect=lambda _u, perm, _c: perm == permission): + with patch(f"{API}.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): @@ -232,7 +231,7 @@ 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): + with patch(f"{API}.authz_api.is_user_allowed", return_value=False): assert not self._has_instructor_tab(staff) def test_legacy_staff_shown(self): @@ -240,7 +239,7 @@ def test_legacy_staff_shown(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: + with patch(f"{API}.authz_api.is_user_allowed") as is_user_allowed: assert self._has_instructor_tab(staff) is_user_allowed.assert_not_called() diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index fcbd09589bd9..a7fe325c89f7 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 +from lms.djangoapps.instructor.authz.api 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 ( @@ -308,8 +308,13 @@ def get(self, request, course_id): course = get_course_by_id(course_key) def can_view_dashboard(user): - # Flag on: any of the dashboard AuthZ permissions. - # Flag off: the legacy dashboard permission. + """ + Return whether the user can view the instructor dashboard. + + With ``authz.enable_course_authoring`` enabled for the course, holding any of + the dashboard AuthZ permissions is enough. With it disabled, the legacy + dashboard permission is used. + """ return user_has_any_instructor_permission( user, authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS, @@ -318,6 +323,8 @@ def can_view_dashboard(user): ) # Masquerading is not migrated to AuthZ yet: it keeps requiring legacy staff access. + # The AuthZ course staff and course admin roles satisfy it through the legacy + # compatibility layer, which maps them to the legacy roles. original_user_is_staff = has_access(request.user, 'staff', course_key).has_access _, request.user = setup_masquerade( request, diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 1901ae00d602..326d52a373f7 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -48,7 +48,7 @@ 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.authz.api 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 diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index e45bd37ea447..c0bde705b1b1 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -35,7 +35,7 @@ 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 ( +from lms.djangoapps.instructor.authz.api import ( get_instructor_authz_permissions, user_has_any_instructor_permission, ) diff --git a/openedx/core/djangoapps/authz/tests/mixins.py b/openedx/core/djangoapps/authz/tests/mixins.py index 01fa79fb62a1..74ff2ba27133 100644 --- a/openedx/core/djangoapps/authz/tests/mixins.py +++ b/openedx/core/djangoapps/authz/tests/mixins.py @@ -75,15 +75,12 @@ 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):