diff --git a/lms/djangoapps/instructor/authz/README.rst b/lms/djangoapps/instructor/authz/README.rst new file mode 100644 index 000000000000..b2a57adad159 --- /dev/null +++ b/lms/djangoapps/instructor/authz/README.rst @@ -0,0 +1,125 @@ +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. ``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 + + 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) + authz_permission = authz_constants.COURSES_VIEW_ENROLLMENTS + # The legacy bridgekeeper permission. + permission_name = permissions.VIEW_ENROLLMENTS + +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_ENROLLMENTS, + "POST": authz_constants.COURSES_ENROLL_LEARNERS, + } + 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 +================================================= + +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/__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..8249de509b88 --- /dev/null +++ b/lms/djangoapps/instructor/authz/constants.py @@ -0,0 +1,78 @@ +""" +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 +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" + +# 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 = 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 = authz_permissions.COURSES_VIEW_GRADING_SETTINGS.identifier +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, +) + +# 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 new file mode 100644 index 000000000000..eb9d5a4a102d --- /dev/null +++ b/lms/djangoapps/instructor/authz/helpers.py @@ -0,0 +1,144 @@ +""" +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 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]: + """ + 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.debug( + "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..5c906b6e6aea --- /dev/null +++ b/lms/djangoapps/instructor/authz/legacy.py @@ -0,0 +1,188 @@ +""" +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.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 +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, +) + + +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 and uses the same rule + as ``IsStaffOrAdmin`` (``openedx.core.djangoapps.course_groups.permissions``) + for those requests. + + 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). + + 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. + + 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 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. + + 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, 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: 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_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.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/authz/permissions.py b/lms/djangoapps/instructor/authz/permissions.py new file mode 100644 index 000000000000..95f0e04bf68f --- /dev/null +++ b/lms/djangoapps/instructor/authz/permissions.py @@ -0,0 +1,87 @@ +""" +DRF permission class to enforce AuthZ in the Instructor Dashboard views. +""" + +import logging +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_any_instructor_permission, user_has_instructor_permission +from lms.djangoapps.instructor.authz.legacy import bridgekeeper_check + +log = logging.getLogger(__name__) + + +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 | 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. + 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: + # 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) + return user_has_any_instructor_permission(request.user, authz_permission, course_key, legacy_check) 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 diff --git a/lms/djangoapps/instructor/authz/tests/mixins.py b/lms/djangoapps/instructor/authz/tests/mixins.py new file mode 100644 index 000000000000..8dc2eabe5046 --- /dev/null +++ b/lms/djangoapps/instructor/authz/tests/mixins.py @@ -0,0 +1,74 @@ +""" +Test helpers to assign AuthZ roles in Instructor Dashboard tests. +""" + +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.djangoapps.authz.tests.mixins import AuthzTestMixin + +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(AuthzTestMixin): + """ + 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() + seed_instructor_dashboard_policies() + + 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..d5a68283b148 --- /dev/null +++ b/lms/djangoapps/instructor/authz/tests/test_helpers.py @@ -0,0 +1,160 @@ +""" +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_any_instructor_permission, + 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): + """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( + 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): + """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) + + 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): + """With the flag off, the result is the one of the legacy check.""" + 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): + """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) + + 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): + """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) + + 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): + """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: + 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_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) + + 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..995c699f9d70 --- /dev/null +++ b/lms/djangoapps/instructor/authz/tests/test_permissions.py @@ -0,0 +1,145 @@ +""" +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): + """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_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) + + 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) + 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): + """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.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) + + @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={ + "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): + """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 + ) + + 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.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. 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..4eae8a36b12b --- /dev/null +++ b/lms/djangoapps/instructor/tests/test_api_v2_authz.py @@ -0,0 +1,252 @@ +""" +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_STAFF +from rest_framework import status +from rest_framework.test import APIClient + +from common.djangoapps.student.tests.factories import ( + InstructorFactory, + 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 +from xmodule.modulestore.tests.factories import CourseFactory + +HELPERS = "lms.djangoapps.instructor.authz.helpers" + +# 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 +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() + # 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): + 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): + """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) + + 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): + """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) + + 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): + """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) + + 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(*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): + """ + A tab is shown with any one of its permissions, and hidden without all of them. + """ + 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): + """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) + + 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, and + without instructor access does not see Date Extensions. + """ + 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", "date_extensions"} + 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) + + 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() + + +@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 6d38d3cc191b..467ba0f84290 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -103,6 +103,10 @@ 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_any_instructor_permission +from lms.djangoapps.instructor.authz.permissions import InstructorAuthzPermission from lms.djangoapps.instructor.constants import ReportType from lms.djangoapps.instructor.enrollment import ( enroll_email, @@ -175,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', @@ -204,7 +211,9 @@ class CourseMetadataView(DeveloperErrorViewMixin, APIView): permissions, and navigation sections. """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) + 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 permission_name = permissions.VIEW_DASHBOARD @extend_schema( @@ -298,6 +307,17 @@ def get(self, request, course_id): course_key = CourseKey.from_string(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 user_has_any_instructor_permission( + user, + authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS, + course_key, + LEGACY_VIEW_DASHBOARD_CHECK, + ) + + # 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, @@ -305,8 +325,7 @@ def get(self, request, course_id): 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) 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): diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index 5b6144d27298..e45bd37ea447 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -33,6 +33,12 @@ 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_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 @@ -84,6 +90,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" @@ -146,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 = [] @@ -158,61 +170,47 @@ 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']): - tabs.append({ - 'tab_id': 'course_team', - 'title': _('Course Team'), - 'url': self._build_tab_url( - 'INSTRUCTOR_MICROFRONTEND_URL', - course_key, - 'course_team' - ), - 'sort_order': 30, - }) + # 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, 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 = [ + ("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, legacy_check in authz_tabs: + if can_see(tab, 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_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( @@ -226,7 +224,7 @@ def get_tabs(self, data): } ) - if access['instructor'] and is_enabled_for_course(course_key): + 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'), @@ -421,6 +419,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'] 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):