From ddcccf1cb96859b9e438312cb513b7a2c53724bd Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 19 Sep 2023 17:07:27 -0300 Subject: [PATCH 01/18] test: add test cases for permission list check functions --- .../course_roles/tests/test_helpers.py | 114 +++++++++++++++++- 1 file changed, 111 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/course_roles/tests/test_helpers.py b/openedx/core/djangoapps/course_roles/tests/test_helpers.py index da2117e00080..24f7156a39a3 100644 --- a/openedx/core/djangoapps/course_roles/tests/test_helpers.py +++ b/openedx/core/djangoapps/course_roles/tests/test_helpers.py @@ -4,7 +4,12 @@ from organizations.tests.factories import OrganizationFactory from common.djangoapps.student.tests.factories import UserFactory -from openedx.core.djangoapps.course_roles.helpers import course_permission_check, organization_permission_check +from openedx.core.djangoapps.course_roles.helpers import ( + course_list_permission_check, + course_permission_check, + organization_list_permission_check, + organization_permission_check +) from openedx.core.djangoapps.course_roles.models import ( CourseRolesPermission, CourseRolesRole, @@ -35,6 +40,11 @@ def setUp(self): self.role_1.services.add(self.service) self.permission_1 = CourseRolesPermission.objects.create(name="test_permission_1") self.role_1.permissions.add(self.permission_1) + self.role_2 = CourseRolesRole.objects.create(name="test_role_3") + self.role_2.services.add(self.service) + self.role_2.permissions.add(self.permission_1) + self.permission_2 = CourseRolesPermission.objects.create(name="test_permission_2") + self.role_2.permissions.add(self.permission_2) def test_course_permission_check_with_course_level_permission(self): """ @@ -70,7 +80,7 @@ def test_course_permission_check_with_instance_level_permission(self): def test_course_permission_check_with_permission_in_another_course(self): """ - Test that course_permission_check returns False when the user has the permission but at the course level, + Test that course_permission_check returns False when the user has the permission at the course level, but in another course """ CourseRolesUserRole.objects.create( @@ -113,8 +123,106 @@ def test_organization_permission_check_with_instance_level_permission(self): def test_organization_permission_check_with_permission_in_another_organization(self): """ - Test that organization_permission_check returns False when the user has the permission but at the + Test that organization_permission_check returns False when the user has the permission at the organization level, but in another organization """ CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_2) assert not organization_permission_check(self.user_1, self.permission_1.name, self.organization_1.name) + + def test_course_list_permission_check_with_course_level_permission(self): + """ + Test that course_list_permission_check returns True when the user has the correct list of permissions + at the course level + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_2, course_id=self.course_1.id, org=self.organization_1 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + + def test_course_list_permission_check_without_course_level_permission(self): + """ + Test that course_list_permission_check returns False when the user does not have the correct list of + permissions at the course level + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_1, course_id=self.course_1.id, org=self.organization_1 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + + def test_course_list_permission_check_with_organization_level_permission(self): + """ + Test that course_list_permission_check returns False when the user has the list of permissions but at the + organization level, and has not been granted the permissions at the course level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_1) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + + def test_course_list_permission_check_with_instance_level_permission(self): + """ + Test that course_list_permission_check returns False when the user has the list of permissions but at the + instance level, and has not been granted the permissions at the course level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + + def test_course_list_permission_check_with_permission_in_another_course(self): + """ + Test that course_list_permission_check returns False when the user has the list of permissions at the + course level, but in another course + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_2, course_id=self.course_2.id, org=self.organization_1 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + + def test_organization_list_permission_check_with_organization_level_permission(self): + """ + Test that organization_list_permission_check returns True when the user has the correct list of permissions + at the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_1) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + + def test_organization_list_permission_check_without_organization_level_permission(self): + """ + Test that organization_list_permission_check returns False when the user does not have the correct list of + permissions at the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_1) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + + def test_organization_list_permission_check_with_course_level_permission(self): + """ + Test that organization_list_permission_check returns False when the user has the list of permissions but at + the course level, and has not been granted the permissions at the organization level + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_2, course_id=self.course_1.id, org=self.organization_1 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + + def test_organization_list_permission_check_with_instance_level_permission(self): + """ + Test that organization_list_permission_check returns False when the user has the list of permissions but at + the instance level, and has not been granted the permissions at the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + + def test_organization_list_permission_check_with_permission_in_another_organization(self): + """ + Test that organization_list_permission_check returns False when the user has the list of permissions at + the organization level, but in another organization + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_2) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) From 6c79a3d34fbeb258ef6b33b87eb80be3baf004ff Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 19 Sep 2023 17:13:49 -0300 Subject: [PATCH 02/18] test: update tests --- .../course_roles/tests/test_helpers.py | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/openedx/core/djangoapps/course_roles/tests/test_helpers.py b/openedx/core/djangoapps/course_roles/tests/test_helpers.py index 24f7156a39a3..367a22f107ff 100644 --- a/openedx/core/djangoapps/course_roles/tests/test_helpers.py +++ b/openedx/core/djangoapps/course_roles/tests/test_helpers.py @@ -5,9 +5,9 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.course_roles.helpers import ( - course_list_permission_check, + course_permissions_list_check, course_permission_check, - organization_list_permission_check, + organization_permissions_list_check, organization_permission_check ) from openedx.core.djangoapps.course_roles.models import ( @@ -129,100 +129,100 @@ def test_organization_permission_check_with_permission_in_another_organization(s CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_2) assert not organization_permission_check(self.user_1, self.permission_1.name, self.organization_1.name) - def test_course_list_permission_check_with_course_level_permission(self): + def test_course_permissions_list_check_with_course_level_permission(self): """ - Test that course_list_permission_check returns True when the user has the correct list of permissions + Test that course_permissions_list_check returns True when the user has the correct list of permissions at the course level """ CourseRolesUserRole.objects.create( user=self.user_1, role=self.role_2, course_id=self.course_1.id, org=self.organization_1 ) test_permissions = [self.permission_1.name, self.permission_2.name] - assert course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + assert course_permissions_list_check(self.user_1, test_permissions, self.course_1.id) - def test_course_list_permission_check_without_course_level_permission(self): + def test_course_permissions_list_check_without_course_level_permission(self): """ - Test that course_list_permission_check returns False when the user does not have the correct list of + Test that course_permissions_list_check returns False when the user does not have the correct list of permissions at the course level """ CourseRolesUserRole.objects.create( user=self.user_1, role=self.role_1, course_id=self.course_1.id, org=self.organization_1 ) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + assert not course_permissions_list_check(self.user_1, test_permissions, self.course_1.id) - def test_course_list_permission_check_with_organization_level_permission(self): + def test_course_permissions_list_check_with_organization_level_permission(self): """ - Test that course_list_permission_check returns False when the user has the list of permissions but at the + Test that course_permissions_list_check returns False when the user has the list of permissions but at the organization level, and has not been granted the permissions at the course level """ CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_1) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + assert not course_permissions_list_check(self.user_1, test_permissions, self.course_1.id) - def test_course_list_permission_check_with_instance_level_permission(self): + def test_course_permissions_list_check_with_instance_level_permission(self): """ - Test that course_list_permission_check returns False when the user has the list of permissions but at the + Test that course_permissions_list_check returns False when the user has the list of permissions but at the instance level, and has not been granted the permissions at the course level """ CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + assert not course_permissions_list_check(self.user_1, test_permissions, self.course_1.id) - def test_course_list_permission_check_with_permission_in_another_course(self): + def test_course_permissions_list_check_with_permission_in_another_course(self): """ - Test that course_list_permission_check returns False when the user has the list of permissions at the + Test that course_permissions_list_check returns False when the user has the list of permissions at the course level, but in another course """ CourseRolesUserRole.objects.create( user=self.user_1, role=self.role_2, course_id=self.course_2.id, org=self.organization_1 ) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not course_list_permission_check(self.user_1, test_permissions, self.course_1.id) + assert not course_permissions_list_check(self.user_1, test_permissions, self.course_1.id) - def test_organization_list_permission_check_with_organization_level_permission(self): + def test_organization_permissions_list_check_with_organization_level_permission(self): """ - Test that organization_list_permission_check returns True when the user has the correct list of permissions + Test that organization_permissions_list_check returns True when the user has the correct list of permissions at the organization level """ CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_1) test_permissions = [self.permission_1.name, self.permission_2.name] - assert organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + assert organization_permissions_list_check(self.user_1, test_permissions, self.organization_1.name) - def test_organization_list_permission_check_without_organization_level_permission(self): + def test_organization_permissions_list_check_without_organization_level_permission(self): """ - Test that organization_list_permission_check returns False when the user does not have the correct list of + Test that organization_permissions_list_check returns False when the user does not have the correct list of permissions at the organization level """ CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_1) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + assert not organization_permissions_list_check(self.user_1, test_permissions, self.organization_1.name) - def test_organization_list_permission_check_with_course_level_permission(self): + def test_organization_permissions_list_check_with_course_level_permission(self): """ - Test that organization_list_permission_check returns False when the user has the list of permissions but at + Test that organization_permissions_list_check returns False when the user has the list of permissions but at the course level, and has not been granted the permissions at the organization level """ CourseRolesUserRole.objects.create( user=self.user_1, role=self.role_2, course_id=self.course_1.id, org=self.organization_1 ) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + assert not organization_permissions_list_check(self.user_1, test_permissions, self.organization_1.name) - def test_organization_list_permission_check_with_instance_level_permission(self): + def test_organization_permissions_list_check_with_instance_level_permission(self): """ - Test that organization_list_permission_check returns False when the user has the list of permissions but at + Test that organization_permissions_list_check returns False when the user has the list of permissions but at the instance level, and has not been granted the permissions at the organization level """ CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + assert not organization_permissions_list_check(self.user_1, test_permissions, self.organization_1.name) - def test_organization_list_permission_check_with_permission_in_another_organization(self): + def test_organization_permissions_list_check_with_permission_in_another_organization(self): """ - Test that organization_list_permission_check returns False when the user has the list of permissions at + Test that organization_permissions_list_check returns False when the user has the list of permissions at the organization level, but in another organization """ CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_2) test_permissions = [self.permission_1.name, self.permission_2.name] - assert not organization_list_permission_check(self.user_1, test_permissions, self.organization_1.name) + assert not organization_permissions_list_check(self.user_1, test_permissions, self.organization_1.name) From 8341962134bb4eaaa9def3c0dfe2a29e3233e3d4 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 19 Sep 2023 17:14:22 -0300 Subject: [PATCH 03/18] feat: add helper functions to check lists of permissions --- openedx/core/djangoapps/course_roles/helpers.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/openedx/core/djangoapps/course_roles/helpers.py b/openedx/core/djangoapps/course_roles/helpers.py index e7cc35775f4c..698f3cc46f62 100644 --- a/openedx/core/djangoapps/course_roles/helpers.py +++ b/openedx/core/djangoapps/course_roles/helpers.py @@ -17,6 +17,14 @@ def course_permission_check(user, permission_name, course_id): ).exists() +@request_cached() +def course_permissions_list_check(user, permission_names, course_id): + """ + Check if a user has all of the given permissions in a course. + """ + return all(course_permission_check(user, permission_name, course_id) for permission_name in permission_names) + + @request_cached() def organization_permission_check(user, permission_name, organization_name): """ @@ -28,3 +36,12 @@ def organization_permission_check(user, permission_name, organization_name): course__isnull=True, org__name=organization_name, ).exists() + + +@request_cached() +def organization_permissions_list_check(user, permission_names, organization_name): + """ + Check if a user has all of the given permissions in an organization. + """ + return all(organization_permission_check(user, permission_name, organization_name) + for permission_name in permission_names) From ee11185bd13aef1f05053bd4f3cb6164ac50ea98 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 19 Sep 2023 19:09:35 -0300 Subject: [PATCH 04/18] style: improve code style --- openedx/core/djangoapps/course_roles/helpers.py | 5 +++-- openedx/core/djangoapps/course_roles/tests/test_helpers.py | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/course_roles/helpers.py b/openedx/core/djangoapps/course_roles/helpers.py index 698f3cc46f62..caf73bacf9f2 100644 --- a/openedx/core/djangoapps/course_roles/helpers.py +++ b/openedx/core/djangoapps/course_roles/helpers.py @@ -43,5 +43,6 @@ def organization_permissions_list_check(user, permission_names, organization_nam """ Check if a user has all of the given permissions in an organization. """ - return all(organization_permission_check(user, permission_name, organization_name) - for permission_name in permission_names) + return all( + organization_permission_check(user, permission_name, organization_name) for permission_name in permission_names + ) diff --git a/openedx/core/djangoapps/course_roles/tests/test_helpers.py b/openedx/core/djangoapps/course_roles/tests/test_helpers.py index 367a22f107ff..660ef9eaba98 100644 --- a/openedx/core/djangoapps/course_roles/tests/test_helpers.py +++ b/openedx/core/djangoapps/course_roles/tests/test_helpers.py @@ -8,13 +8,13 @@ course_permissions_list_check, course_permission_check, organization_permissions_list_check, - organization_permission_check + organization_permission_check, ) from openedx.core.djangoapps.course_roles.models import ( CourseRolesPermission, CourseRolesRole, CourseRolesService, - CourseRolesUserRole + CourseRolesUserRole, ) from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -24,6 +24,7 @@ class PermissionCheckTestCase(SharedModuleStoreTestCase): """ Tests of the permission check functions in course_roles.helpers module """ + def setUp(self): super().setUp() self.user_1 = UserFactory(username="test_user_1") From 883d15ba3a9dd5ac043a663405f7f968ed420faf Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 26 Sep 2023 19:21:17 -0300 Subject: [PATCH 05/18] feat: add course roles checks in the contentstore app --- cms/djangoapps/contentstore/views/user.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index 80a09db96dd3..e3ff36fc420c 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -19,6 +19,7 @@ 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 course_permission_check from ..toggles import use_new_course_team_page from ..utils import get_course_team_url, get_course_team @@ -118,6 +119,7 @@ def _course_team_user(request, course_key, email): else: role_hierarchy = (CourseInstructorRole, CourseStaffRole) + MANAGE_ALL_USERS_PERMISSION = "manage_all_users" if request.method == "GET": # just return info about the user msg = { @@ -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 course_permission_check call + # below will never be true. + # Remove the .has_user call when course roles are implemented. + if (role(course_key).has_user(user) or + course_permission_check(user, course_key, MANAGE_ALL_USERS_PERMISSION)): msg["role"] = role.ROLE break return JsonResponse(msg) @@ -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 course_permission_check call + # below will never return true. + # Remove the .has_user() call below when course roles are implemented. + elif (role.has_user(user, check_user_activation=False) or + course_permission_check(user, course_key, MANAGE_ALL_USERS_PERMISSION)): # pylint: disable=no-value-for-parameter # Remove the user from this old role: old_roles.add(role) From 4f887d3f252ceaa418fb7c24c9b20e736fefeb28 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 26 Sep 2023 19:21:48 -0300 Subject: [PATCH 06/18] feat: add course roles checks in the student app --- common/djangoapps/student/api.py | 10 ++++-- common/djangoapps/student/auth.py | 59 ++++++++++++++++++++++++++++--- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/student/api.py b/common/djangoapps/student/api.py index 2bf42f48289c..b866bac2d988 100644 --- a/common/djangoapps/student/api.py +++ b/common/djangoapps/student/api.py @@ -30,6 +30,7 @@ GlobalStaff, REGISTERED_ACCESS_ROLES as _REGISTERED_ACCESS_ROLES, ) +from openedx.core.djangoapps.course_roles.helpers import course_permission_check from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -130,9 +131,14 @@ def is_user_staff_or_instructor_in_course(user, course_key): """ if not isinstance(course_key, CourseKey): course_key = CourseKey.from_string(course_key) - + MANAGE_STUDENTS_PERMISSION = "manage_students" + # TODO: course roles: If the course roles feature flag is disabled the course_permission_check 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 + course_permission_check(user, course_key, MANAGE_STUDENTS_PERMISSION) ) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index abb98dcaffdc..6ae1e6d2715b 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -25,6 +25,12 @@ OrgLibraryUserRole, OrgStaffRole ) +from openedx.core.djangoapps.course_roles.helpers import ( + course_permission_check, + course_permissions_list_check, + organization_permission_check, + organization_permissions_list_check +) # Studio permissions: STUDIO_EDIT_ROLES = 8 @@ -79,6 +85,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 = [ + "edit_content", + "manage_course_settings", + "manage_adv_settings", + "view_course_settings", + "manage_all_users" + ] + STAFF_ROLE_PERMISSIONS = [ + "edit_content", + "manage_course_settings", + "manage_adv_settings", + "view_course_settings", + "manage_users_except_admin_and_staff" + ] if org is None: org = course_key.org course_key = course_key.for_branch(None) @@ -89,19 +109,49 @@ 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 # 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 organization_permissions_list_check call + # below will never return true. + # Remove the OrgInstructorRole .has_user call when course roles are implemented. + if ( + GlobalStaff().has_user(user) + or OrgInstructorRole(org=org).has_user(user) + or organization_permissions_list_check(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 course_permissions_list_check call + # below will never return true. + # Remove the user_has_role call when course roles are implemented. + if course_key and ( + user_has_role(user, CourseInstructorRole(course_key)) + or course_permissions_list_check(user, COURSE_INSTRUCTOR_ROLE_PERMISSIONS, course_key) + ): return all_perms + # Limited Course Staff does not have access to Studio. + # TODO: course roles: Remove this validation when course roles are implemented if course_key and user_has_role(user, CourseLimitedStaffRole(course_key)): return STUDIO_NO_PERMISSIONS # 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 the organization_permissions_list_check call + # and course_permissions_list_check call below will never return true. + # Remove the OrgStaffRole has_user call and the user_has_role call when course roles are implemented. + if (OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key)))) or ( + organization_permissions_list_check(user, STAFF_ROLE_PERMISSIONS, org) + or (course_key and course_permissions_list_check(user, STAFF_ROLE_PERMISSIONS, course_key)) + ): return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT # Otherwise, for libraries, users can view only: + LIBRARY_USER_ROLE_PERMISSION = "view_library" 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 organization_permission_check call + # below and the course_permission_check call will never return true. + # Remove the OrgLibraryUserRole has_user call and the user_has_role call + # when course roles are implemented. + if (OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key))) or ( + organization_permission_check(user, LIBRARY_USER_ROLE_PERMISSION, org) + or (course_key and course_permission_check(user, LIBRARY_USER_ROLE_PERMISSION, course_key)) + ): return STUDIO_VIEW_USERS | STUDIO_VIEW_CONTENT return STUDIO_NO_PERMISSIONS @@ -223,7 +273,6 @@ 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 From 6e5de1623db06fbeb7c8698c2e5ea967b80abe3f Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 26 Sep 2023 19:22:05 -0300 Subject: [PATCH 07/18] feat: add course roles checks in the lms discussion app --- lms/djangoapps/discussion/rest_api/api.py | 11 +++++++++-- lms/djangoapps/discussion/views.py | 8 +++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 2e200fbd3c25..cb7cf98543be 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -40,6 +40,7 @@ from lms.djangoapps.discussion.toggles import ENABLE_DISCUSSIONS_MFE, ENABLE_LEARNERS_TAB_IN_DISCUSSIONS_MFE from lms.djangoapps.discussion.toggles_utils import reported_content_email_notification_enabled from lms.djangoapps.discussion.views import is_privileged_user +from openedx.core.djangoapps.course_roles.helpers import course_permission_check from openedx.core.djangoapps.discussions.models import ( DiscussionsConfiguration, DiscussionTopicLink, @@ -333,6 +334,7 @@ def _format_datetime(dt): course.get_discussion_blackout_datetimes() ) + MODEREATE_DISCUSSION_FORUM_PERMISSION = 'moderate_discussion_forum' return { "id": str(course_key), "is_posting_enabled": is_posting_enabled, @@ -358,8 +360,13 @@ def _format_datetime(dt): }), "is_group_ta": bool(user_roles & {FORUM_ROLE_GROUP_MODERATOR}), "is_user_admin": request.user.is_staff, - "is_course_staff": CourseStaffRole(course_key).has_user(request.user), - "is_course_admin": CourseInstructorRole(course_key).has_user(request.user), + # TODO: course roles: If the course roles feature flag is disabled the calls to course_permission_check + # below will never be true. + # Remove .has_user() calls below when course roles are implemented. + "is_course_staff": (CourseStaffRole(course_key).has_user(request.user) or + course_permission_check(request.user, MODEREATE_DISCUSSION_FORUM_PERMISSION, course_key)), + "is_course_admin": (CourseInstructorRole(course_key).has_user(request.user) or + course_permission_check(request.user, MODEREATE_DISCUSSION_FORUM_PERMISSION, course_key)), "provider": course_config.provider_type, "enable_in_context": course_config.enable_in_context, "group_at_subsection": course_config.plugin_configuration.get("group_at_subsection", False), diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index bfa511a575bd..63eba620da10 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -50,6 +50,7 @@ from lms.djangoapps.discussion.toggles import ENABLE_DISCUSSIONS_MFE from lms.djangoapps.experiments.utils import get_experiment_user_metadata_context from lms.djangoapps.teams import api as team_api +from openedx.core.djangoapps.course_roles.helpers import course_permission_check from openedx.core.djangoapps.discussions.utils import ( available_division_schemes, get_discussion_categories_ids, @@ -756,7 +757,12 @@ def is_course_staff(course_key: CourseKey, user: User): """ Check if user has course instructor or course staff role. """ - return CourseInstructorRole(course_key).has_user(user) or CourseStaffRole(course_key).has_user(user) + MODEREATE_DISCUSSION_FORUM_PERMISSION = 'moderate_discussion_forum' + # TODO: course roles: If the course roles feature flag is disabled the course_permission_check + # below will never return true. + # Remove .has_user() calls when implementing course roles. + return ((CourseInstructorRole(course_key).has_user(user) or CourseStaffRole(course_key).has_user(user)) or + course_permission_check(user, MODEREATE_DISCUSSION_FORUM_PERMISSION, course_key)) def is_privileged_user(course_key: CourseKey, user: User): From 56ed87980ba9516bd0a83603e985d7e63b3bf550 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 26 Sep 2023 19:22:19 -0300 Subject: [PATCH 08/18] feat: add course roles checks in the lms instructor app --- lms/djangoapps/instructor/views/instructor_dashboard.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 4f82a7d7339f..90ee1b5d79cb 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -50,6 +50,7 @@ from lms.djangoapps.grades.api import is_writable_gradebook_enabled 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.course_roles.helpers import course_permission_check from openedx.core.djangoapps.discussions.config.waffle_utils import legacy_discussion_experience_enabled from openedx.core.djangoapps.discussions.utils import available_division_schemes from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR, CourseDiscussionSettings @@ -185,10 +186,15 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable # Gate access to Special Exam tab depending if either timed exams or proctored exams # are enabled in the course + MANAGE_STUDENTS_PERMISSION = "manage_students" + # TODO: course roles: If the course roles feature flag is disabled the course_permission_check call + # below will never return true. + # Remove .has_user() calls below when course roles are implemented. user_has_access = any([ request.user.is_staff, CourseStaffRole(course_key).has_user(request.user), - CourseInstructorRole(course_key).has_user(request.user) + CourseInstructorRole(course_key).has_user(request.user), + course_permission_check(request.user, MANAGE_STUDENTS_PERMISSION, course_key) ]) course_has_special_exams = course.enable_proctored_exams or course.enable_timed_exams can_see_special_exams = course_has_special_exams and user_has_access and settings.FEATURES.get( From 64c4146487eeb2e8255fc75b4cd5f9d5d001282c Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 26 Sep 2023 19:23:34 -0300 Subject: [PATCH 09/18] feat: add course roles checks in the Learning Sequences package --- .../learning_sequences/api/processors/schedule.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py index 7d6e373b43e1..45dff43cc616 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py @@ -8,6 +8,7 @@ from edx_when.api import get_dates_for_course from opaque_keys.edx.keys import UsageKey, CourseKey # lint-amnesty, pylint: disable=unused-import from openedx.core import types +from openedx.core.djangoapps.course_roles.helpers import course_permission_check from common.djangoapps.student.auth import user_has_role from common.djangoapps.student.roles import CourseBetaTesterRole @@ -60,7 +61,15 @@ def load_data(self, full_course_outline): course_usage_key = self.course_key.make_usage_key('course', 'course') self._course_start = self.keys_to_schedule_fields[course_usage_key].get('start') self._course_end = self.keys_to_schedule_fields[course_usage_key].get('end') - self._is_beta_tester = user_has_role(self.user, CourseBetaTesterRole(self.course_key)) + VIEW_ALL_PUBLISHED_CONTENT_PERMISSION = "view_all_published_content" + VIEW_ONLY_LIVE_PUBLISHED_CONTENT_PERMISSION = "view_only_live_published_content" + # TODO: course roles: If the course roles feature flag is disabled the course_permission_check calls + # below will never return true. + # Remove the user_has_role call when course roles are implemented. + self._is_beta_tester = user_has_role(self.user, CourseBetaTesterRole(self.course_key)) or ( + course_permission_check(self.user, VIEW_ALL_PUBLISHED_CONTENT_PERMISSION, self.course_key) + or course_permission_check(self.user, VIEW_ONLY_LIVE_PUBLISHED_CONTENT_PERMISSION, self.course_key) + ) def inaccessible_sequences(self, full_course_outline): """ From e511114693a90aed43ee8656697afcdb127121da Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 26 Sep 2023 19:47:05 -0300 Subject: [PATCH 10/18] style: fix code style --- common/djangoapps/student/auth.py | 4 ++-- .../content/learning_sequences/api/processors/schedule.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 6ae1e6d2715b..a66aafb0484d 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -91,14 +91,14 @@ def get_user_permissions(user, course_key, org=None): "manage_adv_settings", "view_course_settings", "manage_all_users" - ] + ] STAFF_ROLE_PERMISSIONS = [ "edit_content", "manage_course_settings", "manage_adv_settings", "view_course_settings", "manage_users_except_admin_and_staff" - ] + ] if org is None: org = course_key.org course_key = course_key.for_branch(None) diff --git a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py index 45dff43cc616..e52bff5dd6b2 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py @@ -67,8 +67,8 @@ def load_data(self, full_course_outline): # below will never return true. # Remove the user_has_role call when course roles are implemented. self._is_beta_tester = user_has_role(self.user, CourseBetaTesterRole(self.course_key)) or ( - course_permission_check(self.user, VIEW_ALL_PUBLISHED_CONTENT_PERMISSION, self.course_key) - or course_permission_check(self.user, VIEW_ONLY_LIVE_PUBLISHED_CONTENT_PERMISSION, self.course_key) + course_permission_check(self.user, VIEW_ALL_PUBLISHED_CONTENT_PERMISSION, self.course_key) + or course_permission_check(self.user, VIEW_ONLY_LIVE_PUBLISHED_CONTENT_PERMISSION, self.course_key) ) def inaccessible_sequences(self, full_course_outline): From 19c003950a07b490c9abb04ae0486f33905462cb Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Tue, 26 Sep 2023 19:51:21 -0300 Subject: [PATCH 11/18] fix: course_permission_check calls --- cms/djangoapps/contentstore/views/user.py | 4 ++-- common/djangoapps/student/api.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index e3ff36fc420c..ea4f0296ab18 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -133,7 +133,7 @@ def _course_team_user(request, course_key, email): # below will never be true. # Remove the .has_user call when course roles are implemented. if (role(course_key).has_user(user) or - course_permission_check(user, course_key, MANAGE_ALL_USERS_PERMISSION)): + course_permission_check(user, MANAGE_ALL_USERS_PERMISSION, course_key)): msg["role"] = role.ROLE break return JsonResponse(msg) @@ -177,7 +177,7 @@ def _course_team_user(request, course_key, email): # below will never return true. # Remove the .has_user() call below when course roles are implemented. elif (role.has_user(user, check_user_activation=False) or - course_permission_check(user, course_key, MANAGE_ALL_USERS_PERMISSION)): # pylint: disable=no-value-for-parameter + course_permission_check(user, MANAGE_ALL_USERS_PERMISSION, course_key)): # pylint: disable=no-value-for-parameter # Remove the user from this old role: old_roles.add(role) diff --git a/common/djangoapps/student/api.py b/common/djangoapps/student/api.py index b866bac2d988..f9b0e39774c5 100644 --- a/common/djangoapps/student/api.py +++ b/common/djangoapps/student/api.py @@ -140,5 +140,5 @@ def is_user_staff_or_instructor_in_course(user, course_key): GlobalStaff().has_user(user) or CourseStaffRole(course_key).has_user(user) or CourseInstructorRole(course_key).has_user(user) or - course_permission_check(user, course_key, MANAGE_STUDENTS_PERMISSION) + course_permission_check(user, MANAGE_STUDENTS_PERMISSION, course_key) ) From 570d12dca3bb5bd2c000609cff17c51480676bbf Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Thu, 28 Sep 2023 10:44:03 -0300 Subject: [PATCH 12/18] feat: add validation for AnonymousUser in course permission check helper functions --- openedx/core/djangoapps/course_roles/helpers.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/openedx/core/djangoapps/course_roles/helpers.py b/openedx/core/djangoapps/course_roles/helpers.py index caf73bacf9f2..f5898cbb03e7 100644 --- a/openedx/core/djangoapps/course_roles/helpers.py +++ b/openedx/core/djangoapps/course_roles/helpers.py @@ -1,6 +1,8 @@ """ Helpers for the course roles app. """ +from django.contrib.auth.models import AnonymousUser + from openedx.core.djangoapps.course_roles.models import CourseRolesUserRole from openedx.core.lib.cache_utils import request_cached @@ -10,6 +12,8 @@ def course_permission_check(user, permission_name, course_id): """ Check if a user has a permission in a course. """ + if isinstance(user, AnonymousUser): + return False return CourseRolesUserRole.objects.filter( user=user, role__permissions__name=permission_name, @@ -30,6 +34,8 @@ def organization_permission_check(user, permission_name, organization_name): """ Check if a user has a permission in an organization. """ + if isinstance(user, AnonymousUser): + return False return CourseRolesUserRole.objects.filter( user=user, role__permissions__name=permission_name, From d977e3767f4675922424cc4bde7ee2a867a424d3 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Thu, 28 Sep 2023 11:08:02 -0300 Subject: [PATCH 13/18] fix: disable some pylint warnings --- cms/djangoapps/contentstore/views/user.py | 4 ++-- common/djangoapps/student/auth.py | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index ea4f0296ab18..c848758d4ceb 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -176,8 +176,8 @@ def _course_team_user(request, course_key, email): # TODO: course roles: If the course roles feature flag is disabled the course_permission_check call # below will never return true. # Remove the .has_user() call below when course roles are implemented. - elif (role.has_user(user, check_user_activation=False) or - course_permission_check(user, MANAGE_ALL_USERS_PERMISSION, course_key)): # pylint: disable=no-value-for-parameter + elif (role.has_user(user, check_user_activation=False) or # pylint: disable=no-value-for-parameter + course_permission_check(user, MANAGE_ALL_USERS_PERMISSION, course_key)): # Remove the user from this old role: old_roles.add(role) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index a66aafb0484d..02aad23e4740 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -136,7 +136,8 @@ def get_user_permissions(user, course_key, org=None): # TODO: course roles: If the course roles feature flag is disabled the the organization_permissions_list_check call # and course_permissions_list_check call below will never return true. # Remove the OrgStaffRole has_user call and the user_has_role call when course roles are implemented. - if (OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key)))) or ( + if (OrgStaffRole(org=org).has_user(user) or # pylint: disable=too-many-boolean-expressions + (course_key and user_has_role(user, CourseStaffRole(course_key)))) or ( organization_permissions_list_check(user, STAFF_ROLE_PERMISSIONS, org) or (course_key and course_permissions_list_check(user, STAFF_ROLE_PERMISSIONS, course_key)) ): From 782c05605e9f2906e5017bdc9480c8ebbd66c885 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Thu, 28 Sep 2023 11:10:08 -0300 Subject: [PATCH 14/18] test: update number of querys asserted in has_course_author_access --- openedx/core/djangoapps/embargo/tests/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/embargo/tests/test_api.py b/openedx/core/djangoapps/embargo/tests/test_api.py index 8459337efde1..2e2374a050e5 100644 --- a/openedx/core/djangoapps/embargo/tests/test_api.py +++ b/openedx/core/djangoapps/embargo/tests/test_api.py @@ -177,7 +177,7 @@ def test_caching(self): # (restricted course, but pass all the checks) # This is the worst case, so it will hit all of the # caching code. - with self.assertNumQueries(3): + with self.assertNumQueries(5): embargo_api.check_course_access(self.course.id, user=self.user, ip_addresses=['0.0.0.0']) with self.assertNumQueries(0): From e64f1f608cbc969a5e064b5d789e3e1e9059f1e8 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Thu, 28 Sep 2023 18:15:25 -0300 Subject: [PATCH 15/18] feat: add helper functions to check course or organization permissions --- .../core/djangoapps/course_roles/helpers.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/openedx/core/djangoapps/course_roles/helpers.py b/openedx/core/djangoapps/course_roles/helpers.py index f5898cbb03e7..a882d15495e8 100644 --- a/openedx/core/djangoapps/course_roles/helpers.py +++ b/openedx/core/djangoapps/course_roles/helpers.py @@ -5,6 +5,7 @@ from openedx.core.djangoapps.course_roles.models import CourseRolesUserRole from openedx.core.lib.cache_utils import request_cached +from xmodule.modulestore.django import modulestore @request_cached() @@ -52,3 +53,28 @@ def organization_permissions_list_check(user, permission_names, organization_nam return all( organization_permission_check(user, permission_name, organization_name) for permission_name in permission_names ) + + +@request_cached() +def course_or_organization_permission_check(user, permission_name, course_id, organization_name=None): + """ + Check if a user has a permission in an organization or a course. + """ + if isinstance(user, AnonymousUser): + return False + if organization_name is None: + organization_name = modulestore().get_course(course_id).org + return (course_permission_check(user, permission_name, course_id) or + organization_permission_check(user, permission_name, organization_name) + ) + + +@request_cached() +def course_or_organization_permission_list_check(user, permission_names, course_id, organization_name=None): + """ + Check if a user has all of the given permissions in an organization or a course. + """ + return all( + course_or_organization_permission_check(user, permission_name, course_id, organization_name) + for permission_name in permission_names + ) From b2d6932614362bf4594db68a0598803e9902793a Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Thu, 28 Sep 2023 18:15:45 -0300 Subject: [PATCH 16/18] test: update course_roles tests --- .../course_roles/tests/test_helpers.py | 234 +++++++++++++++++- 1 file changed, 230 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/course_roles/tests/test_helpers.py b/openedx/core/djangoapps/course_roles/tests/test_helpers.py index 660ef9eaba98..8f20eee5be8e 100644 --- a/openedx/core/djangoapps/course_roles/tests/test_helpers.py +++ b/openedx/core/djangoapps/course_roles/tests/test_helpers.py @@ -3,18 +3,20 @@ """ from organizations.tests.factories import OrganizationFactory -from common.djangoapps.student.tests.factories import UserFactory +from common.djangoapps.student.tests.factories import AnonymousUserFactory, UserFactory from openedx.core.djangoapps.course_roles.helpers import ( - course_permissions_list_check, + course_or_organization_permission_check, + course_or_organization_permission_list_check, course_permission_check, - organization_permissions_list_check, + course_permissions_list_check, organization_permission_check, + organization_permissions_list_check ) from openedx.core.djangoapps.course_roles.models import ( CourseRolesPermission, CourseRolesRole, CourseRolesService, - CourseRolesUserRole, + CourseRolesUserRole ) from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -27,6 +29,7 @@ class PermissionCheckTestCase(SharedModuleStoreTestCase): def setUp(self): super().setUp() + self.anonymous_user = AnonymousUserFactory() self.user_1 = UserFactory(username="test_user_1") self.organization_1 = OrganizationFactory(name="test_organization_1") self.organization_2 = OrganizationFactory(name="test_organization_2") @@ -47,6 +50,12 @@ def setUp(self): self.permission_2 = CourseRolesPermission.objects.create(name="test_permission_2") self.role_2.permissions.add(self.permission_2) + def test_course_permission_check_with_anonymus_user(self): + """ + Test that course_permission_check returns False when the user is anonymous + """ + assert not course_permission_check(self.anonymous_user, self.permission_1.name, self.course_1.id) + def test_course_permission_check_with_course_level_permission(self): """ Test that course_permission_check returns True when the user has the correct permission at the course level @@ -89,6 +98,12 @@ def test_course_permission_check_with_permission_in_another_course(self): ) assert not course_permission_check(self.user_1, self.permission_1.name, self.course_1.id) + def test_organization_permission_check_with_anonymous_user(self): + """ + Test that organization_permission_check returns False when the user is anonymous + """ + assert not organization_permission_check(self.anonymous_user, self.permission_1.name, self.organization_1.name) + def test_organization_permission_check_with_organization_level_permission(self): """ Test that organization_permission_check returns True when the user has the correct permission at the @@ -130,6 +145,12 @@ def test_organization_permission_check_with_permission_in_another_organization(s CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_2) assert not organization_permission_check(self.user_1, self.permission_1.name, self.organization_1.name) + def test_course_permissions_list_check_with_anonymous_user(self): + """ + Test that course_permissions_list_check returns False when the user is anonymous + """ + assert not course_permissions_list_check(self.anonymous_user, [self.permission_1.name], self.course_1.id) + def test_course_permissions_list_check_with_course_level_permission(self): """ Test that course_permissions_list_check returns True when the user has the correct list of permissions @@ -181,6 +202,14 @@ def test_course_permissions_list_check_with_permission_in_another_course(self): test_permissions = [self.permission_1.name, self.permission_2.name] assert not course_permissions_list_check(self.user_1, test_permissions, self.course_1.id) + def test_organization_permissions_list_check_with_anonymous_user(self): + """ + Test that organization_permissions_list_check returns False when the user is anonymous + """ + assert not organization_permissions_list_check( + self.anonymous_user, [self.permission_1.name], self.organization_1.name + ) + def test_organization_permissions_list_check_with_organization_level_permission(self): """ Test that organization_permissions_list_check returns True when the user has the correct list of permissions @@ -227,3 +256,200 @@ def test_organization_permissions_list_check_with_permission_in_another_organiza CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_2) test_permissions = [self.permission_1.name, self.permission_2.name] assert not organization_permissions_list_check(self.user_1, test_permissions, self.organization_1.name) + + def test_course_or_organization_permission_check_with_anonymous_user(self): + """ + Test that course_or_organization_permission_check returns False when the user is anonymous + """ + assert not course_or_organization_permission_check( + self.anonymous_user, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_with_course_level_permission(self): + """ + Test that course_or_organization_permission_check returns True when the user has the correct permission at + the course level + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_1, course_id=self.course_1.id, org=self.organization_1 + ) + assert course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_without_course_level_permission(self): + """ + Test that course_or_organization_permission_check returns False when the user does not have the correct + permission at the course level + """ + assert not course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_with_organization_level_permission(self): + """ + Test that course_or_organization_permission_check returns True when the user has the correct permission at + the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_1) + assert course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_with_organization_level_permission_without_org_param(self): + """ + Test that course_or_organization_permission_check returns True when the user has the correct permission at + the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_1) + assert course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id) + + def test_course_or_organization_permission_check_without_organization_level_permission(self): + """ + Test that course_or_organization_permission_check returns False when the user does not have the correct + permission at the organization level + """ + assert not course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_with_instance_level_permission(self): + """ + Test that course_or_organization_permission_check returns False when the user has the permission but at the + instance level, and has not been granted the permission at the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1) + assert not course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_with_permission_in_another_course(self): + """ + Test that course_or_organization_permission_check returns False when the user has the permission at the + course level, but in another course + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_1, course_id=self.course_2.id, org=self.organization_1 + ) + assert not course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_with_permission_in_another_organization(self): + """ + Test that course_or_organization_permission_check returns False when the user has the permission at the + organization level, but in another organization + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_2) + assert not course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_permission_check_with_permission_in_another_course_and_organization(self): + """ + Test that course_or_organization_permission_check returns False when the user has the permission at the + course level, but in another course and the organization level, but in another organization + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_1, course_id=self.course_2.id, org=self.organization_2 + ) + assert not course_or_organization_permission_check( + self.user_1, self.permission_1.name, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_with_course_level_permission(self): + """ + Test that course_or_organization_permission_check returns True when the user has the correct list of + permissions at the course level + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_2, course_id=self.course_1.id, org=self.organization_1 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_without_course_level_permission(self): + """ + Test that course_or_organization_permission_check returns False when the user does not have the correct list + of permissions at the course level + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_1, course_id=self.course_1.id, org=self.organization_1 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_with_organization_level_permission(self): + """ + Test that course_or_organization_permission_check returns True when the user has the correct list of + permissions at the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_1) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_without_organization_level_permission(self): + """ + Test that course_or_organization_permission_check returns False when the user does not have the correct list + of permissions at the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_1, org=self.organization_1) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_with_instance_level_permission(self): + """ + Test that course_or_organization_permission_check returns False when the user has the list of permissions but + at the instance level, and has not been granted the permissions at the organization level + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_with_permission_in_another_course(self): + """ + Test that course_or_organization_permission_check returns False when the user has the list of permissions at + the course level, but in another course + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_2, course_id=self.course_2.id, org=self.organization_1 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_with_permission_in_another_organization(self): + """ + Test that course_or_organization_permission_check returns False when the user has the list of permissions at + the organization level, but in another organization + """ + CourseRolesUserRole.objects.create(user=self.user_1, role=self.role_2, org=self.organization_2) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) + + def test_course_or_organization_list_permission_check_with_permission_in_another_course_and_organization(self): + """ + Test that course_or_organization_permission_check returns False when the user has the list of permissions at + the course level, but in another course and the organization level, but in another organization + """ + CourseRolesUserRole.objects.create( + user=self.user_1, role=self.role_2, course_id=self.course_2.id, org=self.organization_2 + ) + test_permissions = [self.permission_1.name, self.permission_2.name] + assert not course_or_organization_permission_list_check( + self.user_1, test_permissions, self.course_1.id, self.organization_1.name + ) From 3a6d2ebd23d705236cc9a5f2723552c932475d38 Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Thu, 28 Sep 2023 18:18:00 -0300 Subject: [PATCH 17/18] feat: replace course or organization helper functions in auth --- common/djangoapps/student/auth.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 02aad23e4740..79f9116fa1df 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -26,9 +26,9 @@ OrgStaffRole ) from openedx.core.djangoapps.course_roles.helpers import ( - course_permission_check, course_permissions_list_check, - organization_permission_check, + course_or_organization_permission_check, + course_or_organization_permission_list_check, organization_permissions_list_check ) @@ -133,25 +133,23 @@ def get_user_permissions(user, course_key, org=None): if course_key and user_has_role(user, CourseLimitedStaffRole(course_key)): return STUDIO_NO_PERMISSIONS # Staff have all permissions except EDIT_ROLES: - # TODO: course roles: If the course roles feature flag is disabled the the organization_permissions_list_check call - # and course_permissions_list_check call below will never return true. + # TODO: course roles: If the course roles feature flag is disabled the + # course_or_organization_permissions_list_check call below will never return true. # Remove the OrgStaffRole has_user call and the user_has_role call when course roles are implemented. - if (OrgStaffRole(org=org).has_user(user) or # pylint: disable=too-many-boolean-expressions + if (OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key)))) or ( - organization_permissions_list_check(user, STAFF_ROLE_PERMISSIONS, org) - or (course_key and course_permissions_list_check(user, STAFF_ROLE_PERMISSIONS, course_key)) + course_or_organization_permission_list_check(user, STAFF_ROLE_PERMISSIONS, course_key, org) ): return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT # Otherwise, for libraries, users can view only: LIBRARY_USER_ROLE_PERMISSION = "view_library" if course_key and isinstance(course_key, LibraryLocator): - # TODO: course roles: If the course roles feature flag is disabled the organization_permission_check call - # below and the course_permission_check call will never return true. + # TODO: course roles: If the course roles feature flag is disabled the course_or_organization_permission_check + # call below will never return true. # Remove the OrgLibraryUserRole has_user call and the user_has_role call # when course roles are implemented. if (OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key))) or ( - organization_permission_check(user, LIBRARY_USER_ROLE_PERMISSION, org) - or (course_key and course_permission_check(user, LIBRARY_USER_ROLE_PERMISSION, course_key)) + course_or_organization_permission_check(user, LIBRARY_USER_ROLE_PERMISSION, course_key, org) ): return STUDIO_VIEW_USERS | STUDIO_VIEW_CONTENT return STUDIO_NO_PERMISSIONS From 2233cb091a5ef6095a55d5b900a1bf9ca12585ea Mon Sep 17 00:00:00 2001 From: Julian Palmerio Date: Fri, 29 Sep 2023 16:56:05 -0300 Subject: [PATCH 18/18] docs: update course_roles docstrings --- cms/djangoapps/contentstore/views/user.py | 4 ++-- common/djangoapps/student/auth.py | 10 +++++----- lms/djangoapps/discussion/rest_api/api.py | 2 +- lms/djangoapps/discussion/views.py | 2 +- .../instructor/views/instructor_dashboard.py | 2 +- .../learning_sequences/api/processors/schedule.py | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index c848758d4ceb..168d6a5a349b 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -131,7 +131,7 @@ def _course_team_user(request, course_key, email): for role in role_hierarchy: # TODO: course roles: If the course roles feature flag is disabled the course_permission_check call # below will never be true. - # Remove the .has_user call when course roles are implemented. + # Remove the .has_user call when course_roles Django app are implemented. if (role(course_key).has_user(user) or course_permission_check(user, MANAGE_ALL_USERS_PERMISSION, course_key)): msg["role"] = role.ROLE @@ -175,7 +175,7 @@ def _course_team_user(request, course_key, email): return permissions_error_response # TODO: course roles: If the course roles feature flag is disabled the course_permission_check call # below will never return true. - # Remove the .has_user() call below when course roles are implemented. + # 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 course_permission_check(user, MANAGE_ALL_USERS_PERMISSION, course_key)): # Remove the user from this old role: diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 79f9116fa1df..46fade830514 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -111,7 +111,7 @@ def get_user_permissions(user, course_key, org=None): # global staff, org instructors, and course instructors have all permissions: # TODO: course roles: If the course roles feature flag is disabled the organization_permissions_list_check call # below will never return true. - # Remove the OrgInstructorRole .has_user call when course roles are implemented. + # 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) @@ -121,7 +121,7 @@ def get_user_permissions(user, course_key, org=None): # TODO: course roles: If the course roles feature flag is disabled the course_permissions_list_check call # below will never return true. - # Remove the user_has_role call when course roles are implemented. + # 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 course_permissions_list_check(user, COURSE_INSTRUCTOR_ROLE_PERMISSIONS, course_key) @@ -129,13 +129,13 @@ def get_user_permissions(user, course_key, org=None): return all_perms # Limited Course Staff does not have access to Studio. - # TODO: course roles: Remove this validation when course roles are implemented + # 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_NO_PERMISSIONS # Staff have all permissions except EDIT_ROLES: # TODO: course roles: If the course roles feature flag is disabled the # course_or_organization_permissions_list_check call below will never return true. - # Remove the OrgStaffRole has_user call and the user_has_role call when course roles are implemented. + # 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 ( course_or_organization_permission_list_check(user, STAFF_ROLE_PERMISSIONS, course_key, org) @@ -147,7 +147,7 @@ def get_user_permissions(user, course_key, org=None): # TODO: course roles: If the course roles feature flag is disabled the course_or_organization_permission_check # call below will never return true. # Remove the OrgLibraryUserRole has_user call and the user_has_role call - # when course roles are implemented. + # when course_roles Django app are implemented. if (OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key))) or ( course_or_organization_permission_check(user, LIBRARY_USER_ROLE_PERMISSION, course_key, org) ): diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index cb7cf98543be..29fbb9273f68 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -362,7 +362,7 @@ def _format_datetime(dt): "is_user_admin": request.user.is_staff, # TODO: course roles: If the course roles feature flag is disabled the calls to course_permission_check # below will never be true. - # Remove .has_user() calls below when course roles are implemented. + # Remove .has_user() calls below when course_roles Django app are implemented. "is_course_staff": (CourseStaffRole(course_key).has_user(request.user) or course_permission_check(request.user, MODEREATE_DISCUSSION_FORUM_PERMISSION, course_key)), "is_course_admin": (CourseInstructorRole(course_key).has_user(request.user) or diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index 63eba620da10..cf371adf590d 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -760,7 +760,7 @@ def is_course_staff(course_key: CourseKey, user: User): MODEREATE_DISCUSSION_FORUM_PERMISSION = 'moderate_discussion_forum' # TODO: course roles: If the course roles feature flag is disabled the course_permission_check # below will never return true. - # Remove .has_user() calls when implementing course roles. + # Remove .has_user() calls when implementing course_roles Django app. return ((CourseInstructorRole(course_key).has_user(user) or CourseStaffRole(course_key).has_user(user)) or course_permission_check(user, MODEREATE_DISCUSSION_FORUM_PERMISSION, course_key)) diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 90ee1b5d79cb..8d3d45c1cbaa 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -189,7 +189,7 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable MANAGE_STUDENTS_PERMISSION = "manage_students" # TODO: course roles: If the course roles feature flag is disabled the course_permission_check call # below will never return true. - # Remove .has_user() calls below when course roles are implemented. + # Remove .has_user() calls below when course_roles Django app are implemented. user_has_access = any([ request.user.is_staff, CourseStaffRole(course_key).has_user(request.user), diff --git a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py index e52bff5dd6b2..fdb5b329be44 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py @@ -65,7 +65,7 @@ def load_data(self, full_course_outline): VIEW_ONLY_LIVE_PUBLISHED_CONTENT_PERMISSION = "view_only_live_published_content" # TODO: course roles: If the course roles feature flag is disabled the course_permission_check calls # below will never return true. - # Remove the user_has_role call when course roles are implemented. + # Remove the user_has_role call when course_roles Django app are implemented. self._is_beta_tester = user_has_role(self.user, CourseBetaTesterRole(self.course_key)) or ( course_permission_check(self.user, VIEW_ALL_PUBLISHED_CONTENT_PERMISSION, self.course_key) or course_permission_check(self.user, VIEW_ONLY_LIVE_PUBLISHED_CONTENT_PERMISSION, self.course_key)