Repository navigation
feat: enforce AuthZ permissions in the Instructor Dashboard API v2 #39255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
9e34808
3439caf
57206d6
c3e27fe
7b3ecab
0b89821
56daa45
25c487e
94f31b1
99dd45e
6519e5a
7b2ab73
9cf3fcb
0d1fdd3
bfb3a96
7ade6a1
ebfa5ed
84408cb
cc98eff
9af73c3
df2efbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| 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. | ||
| - ``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. | ||
|
|
||
| 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. Pick the case that matches the view: | ||
|
|
||
| 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: | ||
|
|
||
| .. 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_permissions = (authz_constants.COURSES_VIEW_ENROLLMENTS,) | ||
| # The legacy bridgekeeper permission. | ||
| permission_name = permissions.VIEW_ENROLLMENTS | ||
|
|
||
| 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 = authz_constants.INSTRUCTOR_DASHBOARD_PERMISSIONS | ||
| permission_name = permissions.VIEW_DASHBOARD | ||
|
|
||
| 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: | ||
|
|
||
| .. 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 | ||
|
|
||
| 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: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| from lms.djangoapps.instructor.authz import legacy as authz_legacy | ||
|
|
||
| class MyView(APIView): | ||
| permission_classes = (IsAuthenticated, InstructorAuthzPermission) | ||
| authz_permissions = { | ||
| "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.api 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 ``authz.enable_course_authoring`` 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_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``. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| """ | ||
| AuthZ integration for the Instructor Dashboard. | ||
| """ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| """ | ||
| 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 | ||
| ``openedx.core.djangoapps.authz.decorators``). When it is disabled, the legacy | ||
| check is used, so the pre-AuthZ 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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This helper behaves much like the one here: Nitpick (non blocking), could we consider a follow up to collapse both helper methods?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, they overlap. I'd rather keep this PR focused on the Instructor Dashboard, so I'll leave it as is and consider unifying both helpers in a follow-up once the per-tab migrations land and the final shape of the helper is clear.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I agree with leaving it for a follow-up. Can we create an issue so we don't lose track of it? I'd also include the legacy checks and the per-method permissions in it, since other apps will need the same pieces when we migrate them. The idea would be to study what can live in |
||
| 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-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. | ||
|
|
||
| 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. | ||
|
|
||
| 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 ``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: | ||
| 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-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, 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 | ||
| ) | ||
|
|
||
|
|
||
| def get_instructor_authz_permissions( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't this be useful to have in openedx-authz?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, part of it would. The AuthZ-only piece (user + scope + list of permissions → I'd rather not move it in this PR, since the shape of these api helpers will be clearer once the per-tab migrations land. I'll include it in the follow-up issue we discussed in this thread, to decide what lives in |
||
| 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_permissions: Iterable[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_permissions (Iterable[str]): The AuthZ permission identifiers 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_permissions": list(authz_permissions), | ||
| "course_key": str(course_key), | ||
| "access_path": path, | ||
| }, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Comment on lines
+28
to
+29
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to differentiate between these two in openedx/openedx-authz#552?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need both. |
||
| 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, # It's 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), | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openedx-authz already has a way to set permissions per method: the
@authz_permissionsdecorator on each handler. Is there a reason not to use it here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into it, and it doesn't fit this case as is.
@authz_permissions(withMethodPermissionMixin) reads the scope fromrequest.data["scope"]/the query params, while here the course comes from thecourse_idURL kwarg; it requires all the permissions (AND) while the dashboard needs any of them (OR); it allows methods that have no decorator, while we deny them when the flag is on; and it knows nothing about theauthz.enable_course_authoringflag or the legacy fallback.