From fa7c80b40abe1418768e421a94a0036e6510409b Mon Sep 17 00:00:00 2001 From: Jesse Shapiro Date: Tue, 1 Nov 2016 09:55:45 -0400 Subject: [PATCH 1/3] Remove access to CCX courses from Studio entirely and test that revocation --- common/djangoapps/student/auth.py | 15 +++++++ common/djangoapps/student/tests/test_authz.py | 42 ++++++++++++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index f2e7b7f914..62f5e39a85 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -20,6 +20,18 @@ STUDIO_VIEW_CONTENT = 1 # In addition to the above, one is always allowed to "demote" oneself to a lower role within a course, or remove oneself +def is_ccx_course(course_key): + """ + Check whether the course locator maps to a CCX course; this is important + because we don't allow access to CCX courses in Studio. + """ + ccx_namespaces = ( + 'ccx-v1', + 'ccx-block-v1', + ) + return course_key.CANONICAL_NAMESPACE in ccx_namespaces + + def user_has_role(user, role): """ Check whether this user has access to this role (either direct or implied) @@ -61,6 +73,9 @@ def get_user_permissions(user, course_key, org=None): else: assert course_key is None all_perms = STUDIO_EDIT_ROLES | STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT + # No one has studio permissions for CCX courses + if is_ccx_course(course_key): + return 0 # global staff, org instructors, and course instructors have all permissions: if GlobalStaff().has_user(user) or OrgInstructorRole(org=org).has_user(user): return all_perms diff --git a/common/djangoapps/student/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py index e66da9622b..77d2fbb885 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -9,8 +9,9 @@ from django.core.exceptions import PermissionDenied from student.roles import CourseInstructorRole, CourseStaffRole, CourseCreatorRole from student.tests.factories import AdminFactory -from student.auth import user_has_role, add_users, remove_users +from student.auth import user_has_role, add_users, remove_users, has_studio_write_access, has_studio_read_access from opaque_keys.edx.locations import SlashSeparatedCourseKey +from ccx_keys.locator import CCXLocator class CreatorGroupTest(TestCase): @@ -132,6 +133,45 @@ class CreatorGroupTest(TestCase): remove_users(self.admin, CourseCreatorRole(), self.user) +class CCXCourseGroupTest(TestCase): + """ + Test that access to a CCX course in Studio is disallowed + """ + def setUp(self): + """ + Set up test variables + """ + super(CourseGroupTest, self).setUp() + self.global_admin = AdminFactory() + self.staff = User.objects.create_user('teststaff', 'teststaff+courses@edx.org', 'foo') + self.ccx_course_key = CCXLocator.from_string('ccx-v1:edX+DemoX+Demo_Course+ccx@1') + add_users(self.global_admin, CourseStaffRole(self.ccx_course_key), self.staff) + + def test_no_global_admin_write_access(self): + """ + Test that global admins have no write access + """ + self.assertFalse(has_studio_write_access(self.global_admin, self.ccx_course_key)) + + def test_no_staff_write_access(self): + """ + Test that course staff have no write access + """ + self.assertFalse(has_studio_write_access(self.staff, self.ccx_course_key)) + + def test_no_global_admin_read_access(self): + """ + Test that global admins have no read access + """ + self.assertFalse(has_studio_write_access(self.global_admin, self.ccx_course_key)) + + def test_no_staff_write_access(self): + """ + Test that course staff have no read access + """ + self.assertFalse(has_studio_write_access(self.staff, self.ccx_course_key)) + + class CourseGroupTest(TestCase): """ Tests for instructor and staff groups for a particular course. From bd9f8594199d0a3a7121e17e4ab523037a205b9b Mon Sep 17 00:00:00 2001 From: Jesse Shapiro Date: Tue, 1 Nov 2016 11:12:54 -0400 Subject: [PATCH 2/3] Fix the stupid errors that copy-paste makes for --- common/djangoapps/student/tests/test_authz.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/student/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py index 77d2fbb885..bafa96ca9c 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -141,7 +141,7 @@ class CCXCourseGroupTest(TestCase): """ Set up test variables """ - super(CourseGroupTest, self).setUp() + super(CCXCourseGroupTest, self).setUp() self.global_admin = AdminFactory() self.staff = User.objects.create_user('teststaff', 'teststaff+courses@edx.org', 'foo') self.ccx_course_key = CCXLocator.from_string('ccx-v1:edX+DemoX+Demo_Course+ccx@1') @@ -163,13 +163,13 @@ class CCXCourseGroupTest(TestCase): """ Test that global admins have no read access """ - self.assertFalse(has_studio_write_access(self.global_admin, self.ccx_course_key)) + self.assertFalse(has_studio_read_access(self.global_admin, self.ccx_course_key)) - def test_no_staff_write_access(self): + def test_no_staff_read_access(self): """ Test that course staff have no read access """ - self.assertFalse(has_studio_write_access(self.staff, self.ccx_course_key)) + self.assertFalse(has_studio_read_access(self.staff, self.ccx_course_key)) class CourseGroupTest(TestCase): From 088dab365a0a00b71a41088ad701eaa09c8ff9ce Mon Sep 17 00:00:00 2001 From: Jesse Shapiro Date: Tue, 1 Nov 2016 13:04:09 -0400 Subject: [PATCH 3/3] Review changes --- common/djangoapps/student/auth.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 62f5e39a85..efb93cde52 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -7,6 +7,7 @@ to decide whether to check course creator role, and other such functions. from django.core.exceptions import PermissionDenied from django.conf import settings from opaque_keys.edx.locator import LibraryLocator +from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator from student.roles import GlobalStaff, CourseCreatorRole, CourseStaffRole, CourseInstructorRole, CourseRole, \ CourseBetaTesterRole, OrgInstructorRole, OrgStaffRole, LibraryUserRole, OrgLibraryUserRole @@ -17,6 +18,7 @@ STUDIO_EDIT_ROLES = 8 STUDIO_VIEW_USERS = 4 STUDIO_EDIT_CONTENT = 2 STUDIO_VIEW_CONTENT = 1 +STUDIO_NO_PERMISSIONS = 0 # In addition to the above, one is always allowed to "demote" oneself to a lower role within a course, or remove oneself @@ -25,11 +27,7 @@ def is_ccx_course(course_key): Check whether the course locator maps to a CCX course; this is important because we don't allow access to CCX courses in Studio. """ - ccx_namespaces = ( - 'ccx-v1', - 'ccx-block-v1', - ) - return course_key.CANONICAL_NAMESPACE in ccx_namespaces + return isinstance(course_key, CCXLocator) or isinstance(course_key, CCXBlockUsageLocator) def user_has_role(user, role): @@ -72,10 +70,10 @@ def get_user_permissions(user, course_key, org=None): course_key = course_key.for_branch(None) else: assert course_key is None - all_perms = STUDIO_EDIT_ROLES | STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT # No one has studio permissions for CCX courses if is_ccx_course(course_key): - return 0 + 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): return all_perms @@ -88,7 +86,7 @@ def get_user_permissions(user, course_key, org=None): if course_key and isinstance(course_key, LibraryLocator): if OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key)): return STUDIO_VIEW_USERS | STUDIO_VIEW_CONTENT - return 0 + return STUDIO_NO_PERMISSIONS def has_studio_write_access(user, course_key):