Skip to content
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

feat: course_roles setup #33609

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fa998c5
docs: course_roles readme file
julianpalmerio Sep 12, 2023
00fd5df
feat: course_roles Model Setup (#33229)
julianpalmerio Sep 13, 2023
3615345
docs: adr for data storage in course_roles djangoapp
hsinkoff Sep 13, 2023
cc47ebd
docs: adr for course_level roles
lucascalvino Sep 13, 2023
722eb4c
chore: update new table diagram
lucascalvino Sep 14, 2023
0969d0d
feat: course_roles permission check helper function (#33201)
julianpalmerio Sep 15, 2023
5c56213
feat: course_roles add permission checks back end part 1 (#33347)
julianpalmerio Sep 29, 2023
773eb4f
feat: course_roles Create permissions in db table (#33394)
julianpalmerio Oct 4, 2023
affa8f9
feat: course_roles permission check back end part 2 (#33432)
julianpalmerio Oct 27, 2023
4ac95de
feat: course_roles mfe-course authoring helper function (#33599)
julianpalmerio Oct 31, 2023
447423f
feat: Waffle Flag for course_roles helper functions
hsinkoff Oct 16, 2023
c56064b
feat: add course_roles permisisons checks where roles are currently c…
hsinkoff Oct 23, 2023
7ece66f
feat: remove caching from course_roles
hsinkoff Oct 31, 2023
d1a1bdb
chore: prepare branch to PR to master
hsinkoff Nov 1, 2023
4b8e1f3
feat: expose course_roles.use_permission_checks waffle flag for mfe use
hsinkoff Nov 2, 2023
97c568e
docs: update documenation related to course_roles
hsinkoff Nov 2, 2023
9f766af
feat: rename course_roles helper functions
hsinkoff Nov 8, 2023
a15b42d
feat: remove course roles namespace for models
julianpalmerio Nov 13, 2023
0098ee4
feat: remove course roles namespace for models
julianpalmerio Nov 13, 2023
372235e
feat: update migrations
julianpalmerio Nov 13, 2023
ccebf0d
feat: add unique constraints in many to many relationships
julianpalmerio Nov 13, 2023
ec4c505
feat: update migrations
julianpalmerio Nov 13, 2023
8458fb2
test: Add CourseOverview loading to test_helpers.py
julianpalmerio Nov 14, 2023
ac71c59
test: Add CourseOverview loading int test_views.py
julianpalmerio Nov 14, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/pylint-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ jobs:
- module-name: lms-2
path: "--django-settings-module=lms.envs.test lms/djangoapps/gating/ lms/djangoapps/grades/ lms/djangoapps/instructor/ lms/djangoapps/instructor_analytics/ lms/djangoapps/discussion/ lms/djangoapps/edxnotes/ lms/djangoapps/email_marketing/ lms/djangoapps/experiments/ lms/djangoapps/instructor_task/ lms/djangoapps/learner_dashboard/ lms/djangoapps/learner_recommendations/ lms/djangoapps/learner_home/ lms/djangoapps/lms_initialization/ lms/djangoapps/lms_xblock/ lms/djangoapps/lti_provider/ lms/djangoapps/mailing/ lms/djangoapps/mobile_api/ lms/djangoapps/monitoring/ lms/djangoapps/ora_staff_grader/ lms/djangoapps/program_enrollments/ lms/djangoapps/rss_proxy lms/djangoapps/static_template_view/ lms/djangoapps/staticbook/ lms/djangoapps/support/ lms/djangoapps/survey/ lms/djangoapps/teams/ lms/djangoapps/tests/ lms/djangoapps/user_tours/ lms/djangoapps/verify_student/ lms/djangoapps/mfe_config_api/ lms/envs/ lms/lib/ lms/tests.py"
- module-name: openedx-1
path: "--django-settings-module=lms.envs.test openedx/core/types/ openedx/core/djangoapps/ace_common/ openedx/core/djangoapps/agreements/ openedx/core/djangoapps/api_admin/ openedx/core/djangoapps/auth_exchange/ openedx/core/djangoapps/bookmarks/ openedx/core/djangoapps/cache_toolbox/ openedx/core/djangoapps/catalog/ openedx/core/djangoapps/ccxcon/ openedx/core/djangoapps/commerce/ openedx/core/djangoapps/common_initialization/ openedx/core/djangoapps/common_views/ openedx/core/djangoapps/config_model_utils/ openedx/core/djangoapps/content/ openedx/core/djangoapps/content_libraries/ openedx/core/djangoapps/content_staging/ openedx/core/djangoapps/contentserver/ openedx/core/djangoapps/cookie_metadata/ openedx/core/djangoapps/cors_csrf/ openedx/core/djangoapps/course_apps/ openedx/core/djangoapps/course_date_signals/ openedx/core/djangoapps/course_groups/ openedx/core/djangoapps/courseware_api/ openedx/core/djangoapps/crawlers/ openedx/core/djangoapps/credentials/ openedx/core/djangoapps/credit/ openedx/core/djangoapps/dark_lang/ openedx/core/djangoapps/debug/ openedx/core/djangoapps/demographics/ openedx/core/djangoapps/discussions/ openedx/core/djangoapps/django_comment_common/ openedx/core/djangoapps/embargo/ openedx/core/djangoapps/enrollments/ openedx/core/djangoapps/external_user_ids/ openedx/core/djangoapps/zendesk_proxy/ openedx/core/djangolib/ openedx/core/lib/ openedx/core/tests/ openedx/core/djangoapps/course_live/"
path: "--django-settings-module=lms.envs.test openedx/core/types/ openedx/core/djangoapps/ace_common/ openedx/core/djangoapps/agreements/ openedx/core/djangoapps/api_admin/ openedx/core/djangoapps/auth_exchange/ openedx/core/djangoapps/bookmarks/ openedx/core/djangoapps/cache_toolbox/ openedx/core/djangoapps/catalog/ openedx/core/djangoapps/ccxcon/ openedx/core/djangoapps/commerce/ openedx/core/djangoapps/common_initialization/ openedx/core/djangoapps/common_views/ openedx/core/djangoapps/config_model_utils/ openedx/core/djangoapps/content/ openedx/core/djangoapps/content_libraries/ openedx/core/djangoapps/content_staging/ openedx/core/djangoapps/contentserver/ openedx/core/djangoapps/cookie_metadata/ openedx/core/djangoapps/cors_csrf/ openedx/core/djangoapps/course_apps/ openedx/core/djangoapps/course_date_signals/ openedx/core/djangoapps/course_groups/ openedx/core/djangoapps/course_roles/ openedx/core/djangoapps/courseware_api/ openedx/core/djangoapps/crawlers/ openedx/core/djangoapps/credentials/ openedx/core/djangoapps/credit/ openedx/core/djangoapps/dark_lang/ openedx/core/djangoapps/debug/ openedx/core/djangoapps/demographics/ openedx/core/djangoapps/discussions/ openedx/core/djangoapps/django_comment_common/ openedx/core/djangoapps/embargo/ openedx/core/djangoapps/enrollments/ openedx/core/djangoapps/external_user_ids/ openedx/core/djangoapps/zendesk_proxy/ openedx/core/djangolib/ openedx/core/lib/ openedx/core/tests/ openedx/core/djangoapps/course_live/"
- module-name: openedx-2
path: "--django-settings-module=lms.envs.test openedx/core/djangoapps/geoinfo/ openedx/core/djangoapps/header_control/ openedx/core/djangoapps/heartbeat/ openedx/core/djangoapps/lang_pref/ openedx/core/djangoapps/models/ openedx/core/djangoapps/monkey_patch/ openedx/core/djangoapps/oauth_dispatch/ openedx/core/djangoapps/olx_rest_api/ openedx/core/djangoapps/password_policy/ openedx/core/djangoapps/plugin_api/ openedx/core/djangoapps/plugins/ openedx/core/djangoapps/profile_images/ openedx/core/djangoapps/programs/ openedx/core/djangoapps/safe_sessions/ openedx/core/djangoapps/schedules/ openedx/core/djangoapps/service_status/ openedx/core/djangoapps/session_inactivity_timeout/ openedx/core/djangoapps/signals/ openedx/core/djangoapps/site_configuration/ openedx/core/djangoapps/system_wide_roles/ openedx/core/djangoapps/theming/ openedx/core/djangoapps/user_api/ openedx/core/djangoapps/user_authn/ openedx/core/djangoapps/util/ openedx/core/djangoapps/verified_track_content/ openedx/core/djangoapps/video_config/ openedx/core/djangoapps/video_pipeline/ openedx/core/djangoapps/waffle_utils/ openedx/core/djangoapps/xblock/ openedx/core/djangoapps/xmodule_django/ openedx/core/tests/ openedx/features/ openedx/testing/ openedx/tests/ openedx/core/djangoapps/learner_pathway/ openedx/core/djangoapps/notifications/ openedx/core/djangoapps/staticfiles/ openedx/core/djangoapps/content_tagging/"
path: "--django-settings-module=lms.envs.test openedx/core/djangoapps/geoinfo/ openedx/core/djangoapps/header_control/ openedx/core/djangoapps/heartbeat/ openedx/core/djangoapps/lang_pref/ openedx/core/djangoapps/models/ openedx/core/djangoapps/monkey_patch/ openedx/core/djangoapps/oauth_dispatch/ openedx/core/djangoapps/olx_rest_api/ openedx/core/djangoapps/password_policy/ openedx/core/djangoapps/plugin_api/ openedx/core/djangoapps/plugins/ openedx/core/djangoapps/profile_images/ openedx/core/djangoapps/programs/ openedx/core/djangoapps/safe_sessions/ openedx/core/djangoapps/schedules/ openedx/core/djangoapps/service_status/ openedx/core/djangoapps/session_inactivity_timeout/ openedx/core/djangoapps/signals/ openedx/core/djangoapps/site_configuration/ openedx/core/djangoapps/system_wide_roles/ openedx/core/djangoapps/theming/ openedx/core/djangoapps/user_api/ openedx/core/djangoapps/user_authn/ openedx/core/djangoapps/util/ openedx/core/djangoapps/verified_track_content/ openedx/core/djangoapps/video_config/ openedx/core/djangoapps/video_pipeline/ openedx/core/djangoapps/waffle_utils/ openedx/core/djangoapps/xblock/ openedx/core/djangoapps/xmodule_django/ openedx/core/tests/ openedx/features/ openedx/testing/ openedx/tests/ openedx/core/djangoapps/learner_pathway/ openedx/core/djangoapps/notifications/ openedx/core/djangoapps/staticfiles/ openedx/core/djangoapps/content_tagging/ openedx/core/djangoapps/course_roles/"
- module-name: common
path: "--django-settings-module=lms.envs.test common"
- module-name: cms
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/unit-test-shards.json
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
"openedx/core/djangoapps/course_apps/",
"openedx/core/djangoapps/course_date_signals/",
"openedx/core/djangoapps/course_groups/",
"openedx/core/djangoapps/course_roles/",
"openedx/core/djangoapps/courseware_api/",
"openedx/core/djangoapps/crawlers/",
"openedx/core/djangoapps/credentials/",
Expand Down Expand Up @@ -182,6 +183,7 @@
"openedx/core/djangoapps/course_apps/",
"openedx/core/djangoapps/course_date_signals/",
"openedx/core/djangoapps/course_groups/",
"openedx/core/djangoapps/course_roles/",
"openedx/core/djangoapps/courseware_api/",
"openedx/core/djangoapps/crawlers/",
"openedx/core/djangoapps/credentials/",
Expand Down
11 changes: 10 additions & 1 deletion cms/djangoapps/contentstore/views/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

from common.djangoapps.student import auth
from common.djangoapps.student.roles import CourseInstructorRole
from openedx.core.djangoapps.course_roles.helpers import user_has_permission_course
from openedx.core.djangoapps.course_roles.permissions import CourseRolesPermission


def get_user_role(user, course_id):
Expand All @@ -17,7 +19,14 @@ def get_user_role(user, course_id):
:param course_id: the course_id of the course we're interested in
"""
# afaik, this is only used in lti
if auth.user_has_role(user, CourseInstructorRole(course_id)):

# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_course
# call below will never return true.
# Remove the auth.has_user_role call when course_roles Django app are implemented.
if (
auth.user_has_role(user, CourseInstructorRole(course_id)) or
user_has_permission_course(user, CourseRolesPermission.MANAGE_ALL_USERS.value, course_id)
):
return 'instructor'
else:
return 'staff'
14 changes: 12 additions & 2 deletions cms/djangoapps/contentstore/views/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
from common.djangoapps.student.models import CourseEnrollment
from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, LibraryUserRole
from common.djangoapps.util.json_request import JsonResponse, expect_json
from openedx.core.djangoapps.course_roles.helpers import user_has_permission_course
from openedx.core.djangoapps.course_roles.permissions import CourseRolesPermission

from ..toggles import use_new_course_team_page
from ..utils import get_course_team_url, get_course_team
Expand Down Expand Up @@ -127,7 +129,11 @@ def _course_team_user(request, course_key, email):
}
# what's the highest role that this user has? (How should this report global staff?)
for role in role_hierarchy:
if role(course_key).has_user(user):
# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_course call
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still a TODO? I see the user_has_permission_course checks whether the flag is enabled.

# below will never be true.
# Remove the .has_user call when course_roles Django app are implemented.
if (role(course_key).has_user(user) or
user_has_permission_course(user, CourseRolesPermission.MANAGE_ALL_USERS.value, course_key)):
msg["role"] = role.ROLE
break
return JsonResponse(msg)
Expand Down Expand Up @@ -167,7 +173,11 @@ def _course_team_user(request, course_key, email):
role_added = True
else:
return permissions_error_response
elif role.has_user(user, check_user_activation=False): # pylint: disable=no-value-for-parameter
# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_course call
# below will never return true.
# Remove the .has_user() call below when course_roles Django app are implemented.
elif (role.has_user(user, check_user_activation=False) or # pylint: disable=no-value-for-parameter
user_has_permission_course(user, CourseRolesPermission.MANAGE_ALL_USERS.value, course_key)):
# Remove the user from this old role:
old_roles.add(role)

Expand Down
3 changes: 3 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1809,6 +1809,9 @@
# alternative swagger generator for CMS API
'drf_spectacular',
'openedx_events',

# Course Roles
'openedx.core.djangoapps.course_roles',
]


Expand Down
5 changes: 5 additions & 0 deletions cms/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,8 @@
re_path('^cms-api/ui/', SpectacularSwaggerView.as_view(url_name='schema'), name='swagger-ui'),
re_path('^cms-api/schema/', SpectacularAPIView.as_view(), name='schema'),
]

# Course Roles API
urlpatterns += [
path('api/course_roles/', include('openedx.core.djangoapps.course_roles.urls', namespace='course_roles_api')),
]
10 changes: 8 additions & 2 deletions common/djangoapps/student/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
GlobalStaff,
REGISTERED_ACCESS_ROLES as _REGISTERED_ACCESS_ROLES,
)
from openedx.core.djangoapps.course_roles.helpers import user_has_permission_course
from openedx.core.djangoapps.course_roles.permissions import CourseRolesPermission
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers


Expand Down Expand Up @@ -130,9 +132,13 @@ def is_user_staff_or_instructor_in_course(user, course_key):
"""
if not isinstance(course_key, CourseKey):
course_key = CourseKey.from_string(course_key)

# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_course call
# below will never be true.
# Remove the CourseStaffRole and the CourseInstructorRole .has_user() calls below when course
# roles are implemented.
return (
GlobalStaff().has_user(user) or
CourseStaffRole(course_key).has_user(user) or
CourseInstructorRole(course_key).has_user(user)
CourseInstructorRole(course_key).has_user(user) or
user_has_permission_course(user, CourseRolesPermission.MANAGE_STUDENTS.value, course_key)
)
70 changes: 64 additions & 6 deletions common/djangoapps/student/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
OrgLibraryUserRole,
OrgStaffRole
)
from openedx.core.djangoapps.course_roles.helpers import (
user_has_permission_course_org,
user_has_permission_list_course_org,
user_has_permission_course,
user_has_permission_list_course,
user_has_permission_list_org
)
from openedx.core.djangoapps.course_roles.permissions import CourseRolesPermission

# Studio permissions:
STUDIO_EDIT_ROLES = 8
Expand Down Expand Up @@ -79,6 +87,20 @@ def get_user_permissions(user, course_key, org=None):
Can also set course_key=None and pass in an org to get the user's
permissions for that organization as a whole.
"""
COURSE_INSTRUCTOR_ROLE_PERMISSIONS = [
CourseRolesPermission.MANAGE_CONTENT.value,
CourseRolesPermission.MANAGE_COURSE_SETTINGS.value,
CourseRolesPermission.MANAGE_ADVANCED_SETTINGS.value,
CourseRolesPermission.VIEW_COURSE_SETTINGS.value,
CourseRolesPermission.MANAGE_ALL_USERS.value,
]
STAFF_ROLE_PERMISSIONS = [
CourseRolesPermission.MANAGE_CONTENT.value,
CourseRolesPermission.MANAGE_COURSE_SETTINGS.value,
CourseRolesPermission.MANAGE_ADVANCED_SETTINGS.value,
CourseRolesPermission.VIEW_COURSE_SETTINGS.value,
CourseRolesPermission.MANAGE_USERS_EXCEPT_ADMIN_AND_STAFF.value,
]
Comment on lines +90 to +103
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we move these constants to the top of the file?

if org is None:
org = course_key.org
course_key = course_key.for_branch(None)
Expand All @@ -89,9 +111,23 @@ def get_user_permissions(user, course_key, org=None):
return STUDIO_NO_PERMISSIONS
all_perms = STUDIO_EDIT_ROLES | STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT
Copy link
Member

@mariajgrimaldi mariajgrimaldi Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be replaced later on by these permissions definitions?

# global staff, org instructors, and course instructors have all permissions:
if GlobalStaff().has_user(user) or OrgInstructorRole(org=org).has_user(user):
# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_list_org call
# below will never return true.
# Remove the OrgInstructorRole .has_user call when course_roles Django app are implemented.
if (
GlobalStaff().has_user(user)
or OrgInstructorRole(org=org).has_user(user)
or user_has_permission_list_org(user, COURSE_INSTRUCTOR_ROLE_PERMISSIONS, org)
):
return all_perms
if course_key and user_has_role(user, CourseInstructorRole(course_key)):

# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_list_course call
# below will never return true.
# Remove the user_has_role call when course_roles Django app are implemented.
if course_key and (
user_has_role(user, CourseInstructorRole(course_key))
or user_has_permission_list_course(user, COURSE_INSTRUCTOR_ROLE_PERMISSIONS, course_key)
):
return all_perms
# HACK: Limited Staff should not have studio read access. However, since many LMS views depend on the
# `has_course_author_access` check and `course_author_access_required` decorator, we have to allow write access
Expand All @@ -101,14 +137,31 @@ def get_user_permissions(user, course_key, org=None):
# The permissions matrix from the RBAC project (https://github.com/openedx/platform-roadmap/issues/246) shows that
# the LMS and Studio permissions will be separated as a part of this project. Once this is done (and this code is
# not removed during its implementation), we can replace the Limited Staff permissions with more granular ones.

# Limited Course Staff does not have access to Studio.
# TODO: course roles: Remove this validation when course roles app are implemented
if course_key and user_has_role(user, CourseLimitedStaffRole(course_key)):
return STUDIO_EDIT_CONTENT
# Staff have all permissions except EDIT_ROLES:
if OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key))):
# TODO: course roles: If the course roles feature flag is disabled the
# course_or_user_has_permission_list_org call below will never return true.
# Remove the OrgStaffRole has_user call and the user_has_role call
# when course_roles Django app are implemented.
if (OrgStaffRole(org=org).has_user(user) or
(course_key and user_has_role(user, CourseStaffRole(course_key)))) or (
user_has_permission_list_course_org(user, STAFF_ROLE_PERMISSIONS, course_key, org)
):
return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT
# Otherwise, for libraries, users can view only:

if course_key and isinstance(course_key, LibraryLocator):
if OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key)):
# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_course_org
# call below will never return true.
# Remove the OrgLibraryUserRole has_user call and the user_has_role call
# when course_roles Django app are implemented.
if (OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key))) or (
user_has_permission_course_org(user, CourseRolesPermission.MANAGE_LIBRARIES.value, course_key, org)
):
return STUDIO_VIEW_USERS | STUDIO_VIEW_CONTENT
return STUDIO_NO_PERMISSIONS

Expand Down Expand Up @@ -230,9 +283,14 @@ def _check_caller_authority(caller, role):
# superuser
if GlobalStaff().has_user(caller):
return

if isinstance(role, (GlobalStaff, CourseCreatorRole, OrgContentCreatorRole)): # lint-amnesty, pylint: disable=no-else-raise
raise PermissionDenied
elif isinstance(role, CourseRole): # instructors can change the roles w/in their course
if not user_has_role(caller, CourseInstructorRole(role.course_key)):
# TODO: course roles: If the course roles feature flag is disabled the user_has_permission_course
# call below will never return true.
# Remove the user_has_role call when course_roles Django app are implemented.
if not (
user_has_role(caller, CourseInstructorRole(role.course_key)) or
user_has_permission_course(caller, CourseRolesPermission.MANAGE_ALL_USERS.value, role.course_key)
):
raise PermissionDenied
Loading
Loading