From fa7c80b40abe1418768e421a94a0036e6510409b Mon Sep 17 00:00:00 2001 From: Jesse Shapiro Date: Tue, 1 Nov 2016 09:55:45 -0400 Subject: [PATCH 1/5] 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/5] 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/5] 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): From 562aeee0ba9ca853262dc427b53fb5570dbe1765 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Mon, 28 Nov 2016 07:51:03 -0500 Subject: [PATCH 4/5] Fix CAPA's max_score computation --- common/lib/capa/capa/capa_problem.py | 75 ++++++++++++++------------- common/lib/capa/capa/responsetypes.py | 41 ++++++++------- 2 files changed, 60 insertions(+), 56 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 555b3ee6c7..bdb66229af 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -187,7 +187,7 @@ class LoncapaProblem(object): # construct script processor context (eg for customresponse problems) if minimal_init: - self.context = {'script_code': ""} + self.context = {} else: self.context = self._extract_context(self.tree) @@ -195,24 +195,24 @@ class LoncapaProblem(object): # transformations. This also creates the dict (self.responders) of Response # instances for each question in the problem. The dict has keys = xml subtree of # Response, values = Response instance - self.problem_data = self._preprocess_problem(self.tree) - - if not self.student_answers: # True when student_answers is an empty dict - self.set_initial_display() - - # dictionary of InputType objects associated with this problem - # input_id string -> InputType object - self.inputs = {} - - # Run response late_transforms last (see MultipleChoiceResponse) - # Sort the responses to be in *_1 *_2 ... order. - responses = self.responders.values() - responses = sorted(responses, key=lambda resp: int(resp.id[resp.id.rindex('_') + 1:])) - for response in responses: - if hasattr(response, 'late_transforms'): - response.late_transforms(self) + self.problem_data = self._preprocess_problem(self.tree, minimal_init) if not minimal_init: + if not self.student_answers: # True when student_answers is an empty dict + self.set_initial_display() + + # dictionary of InputType objects associated with this problem + # input_id string -> InputType object + self.inputs = {} + + # Run response late_transforms last (see MultipleChoiceResponse) + # Sort the responses to be in *_1 *_2 ... order. + responses = self.responders.values() + responses = sorted(responses, key=lambda resp: int(resp.id[resp.id.rindex('_') + 1:])) + for response in responses: + if hasattr(response, 'late_transforms'): + response.late_transforms(self) + self.extracted_tree = self._extract_html(self.tree) def make_xml_compatible(self, tree): @@ -869,7 +869,7 @@ class LoncapaProblem(object): return tree - def _preprocess_problem(self, tree): # private + def _preprocess_problem(self, tree, minimal_init): # private """ Assign IDs to all the responses Assign sub-IDs to all entries (textline, schematic, etc.) @@ -907,28 +907,31 @@ class LoncapaProblem(object): # instantiate capa Response responsetype_cls = responsetypes.registry.get_class_for_tag(response.tag) - responder = responsetype_cls(response, inputfields, self.context, self.capa_system, self.capa_module) + responder = responsetype_cls( + response, inputfields, self.context, self.capa_system, self.capa_module, minimal_init + ) # save in list in self self.responders[response] = responder - # get responder answers (do this only once, since there may be a performance cost, - # eg with externalresponse) - self.responder_answers = {} - for response in self.responders.keys(): - try: - self.responder_answers[response] = self.responders[response].get_answers() - except: - log.debug('responder %s failed to properly return get_answers()', - self.responders[response]) # FIXME - raise + if not minimal_init: + # get responder answers (do this only once, since there may be a performance cost, + # eg with externalresponse) + self.responder_answers = {} + for response in self.responders.keys(): + try: + self.responder_answers[response] = self.responders[response].get_answers() + except: + log.debug('responder %s failed to properly return get_answers()', + self.responders[response]) # FIXME + raise - # ... may not be associated with any specific response; give - # IDs for those separately - # TODO: We should make the namespaces consistent and unique (e.g. %s_problem_%i). - solution_id = 1 - for solution in tree.findall('.//solution'): - solution.attrib['id'] = "%s_solution_%i" % (self.problem_id, solution_id) - solution_id += 1 + # ... may not be associated with any specific response; give + # IDs for those separately + # TODO: We should make the namespaces consistent and unique (e.g. %s_problem_%i). + solution_id = 1 + for solution in tree.findall('.//solution'): + solution.attrib['id'] = "%s_solution_%i" % (self.problem_id, solution_id) + solution_id += 1 return problem_data diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 2720f0e28a..6d064845a5 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -154,7 +154,7 @@ class LoncapaResponse(object): # By default, we set this to False, allowing subclasses to override as appropriate. multi_device_support = False - def __init__(self, xml, inputfields, context, system, capa_module): + def __init__(self, xml, inputfields, context, system, capa_module, minimal_init): """ Init is passed the following arguments: @@ -213,28 +213,29 @@ class LoncapaResponse(object): maxpoints = inputfield.get('points', '1') self.maxpoints.update({inputfield.get('id'): int(maxpoints)}) - # dict for default answer map (provided in input elements) - self.default_answer_map = {} - for entry in self.inputfields: - answer = entry.get('correct_answer') - if answer: - self.default_answer_map[entry.get( - 'id')] = contextualize_text(answer, self.context) + if not minimal_init: + # dict for default answer map (provided in input elements) + self.default_answer_map = {} + for entry in self.inputfields: + answer = entry.get('correct_answer') + if answer: + self.default_answer_map[entry.get( + 'id')] = contextualize_text(answer, self.context) - # Does this problem have partial credit? - # If so, what kind? Get it as a list of strings. - partial_credit = xml.xpath('.')[0].get('partial_credit', default=False) + # Does this problem have partial credit? + # If so, what kind? Get it as a list of strings. + partial_credit = xml.xpath('.')[0].get('partial_credit', default=False) - if str(partial_credit).lower().strip() == 'false': - self.has_partial_credit = False - self.credit_type = [] - else: - self.has_partial_credit = True - self.credit_type = partial_credit.split(',') - self.credit_type = [word.strip().lower() for word in self.credit_type] + if str(partial_credit).lower().strip() == 'false': + self.has_partial_credit = False + self.credit_type = [] + else: + self.has_partial_credit = True + self.credit_type = partial_credit.split(',') + self.credit_type = [word.strip().lower() for word in self.credit_type] - if hasattr(self, 'setup_response'): - self.setup_response() + if hasattr(self, 'setup_response'): + self.setup_response() def get_max_score(self): """ From f69b2c41cb29f848b66766cce29e66aa92deec03 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Mon, 28 Nov 2016 17:10:33 -0500 Subject: [PATCH 5/5] Additional Logging for task queue operation --- openedx/core/lib/celery/routers.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/openedx/core/lib/celery/routers.py b/openedx/core/lib/celery/routers.py index c8f6b4196b..5018b9d1ef 100644 --- a/openedx/core/lib/celery/routers.py +++ b/openedx/core/lib/celery/routers.py @@ -5,6 +5,9 @@ For more, see http://celery.readthedocs.io/en/latest/userguide/routing.html#rout """ from abc import ABCMeta, abstractproperty from django.conf import settings +import logging + +log = logging.getLogger(__name__) class AlternateEnvironmentRouter(object): @@ -30,6 +33,13 @@ class AlternateEnvironmentRouter(object): If None is returned from this method, default routing logic is used. """ alternate_env = self.alternate_env_tasks.get(task, None) + if 'update_course_in_cache' in task: + log.info("TNL-5408: task={task}, args={args}, alternate_env={alt_env}, queues={queues}".format( + task=task, + args=args, + alt_env=alternate_env, + queues=getattr(settings, 'CELERY_QUEUES', []).keys() + )) if alternate_env: return self.ensure_queue_env(alternate_env) return None