From 31689659ce5c9dd1d99e417e584078096d1f95f0 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 6 Mar 2015 14:57:55 -0500 Subject: [PATCH 01/14] Add query count tests to DjangoKeyValueStore/FieldDataCache --- .../courseware/tests/test_model_data.py | 109 +++++++++++++----- 1 file changed, 80 insertions(+), 29 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 80b4c04512..770391b142 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -106,48 +106,68 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value', 'b_field': 'b_value'})) self.user = student_module.student self.assertEqual(self.user.id, 1) # check our assumption hard-coded in the key functions above. - self.field_data_cache = FieldDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user) + + # There should be only one query to load a single descriptor with a single user_state field + with self.assertNumQueries(1): + self.field_data_cache = FieldDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user) + self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_get_existing_field(self): "Test that getting an existing field in an existing StudentModule works" - self.assertEquals('a_value', self.kvs.get(user_state_key('a_field'))) + # This should only read from the cache, not the database + with self.assertNumQueries(0): + self.assertEquals('a_value', self.kvs.get(user_state_key('a_field'))) def test_get_missing_field(self): "Test that getting a missing field from an existing StudentModule raises a KeyError" - self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) + # This should only read from the cache, not the database + with self.assertNumQueries(0): + self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) def test_set_existing_field(self): "Test that setting an existing user_state field changes the value" - self.kvs.set(user_state_key('a_field'), 'new_value') + # We are updating a problem, so we write to courseware_studentmodulehistory + # as well as courseware_studentmodule + with self.assertNumQueries(3): + self.kvs.set(user_state_key('a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_set_missing_field(self): "Test that setting a new user_state field changes the value" - self.kvs.set(user_state_key('not_a_field'), 'new_value') + # We are updating a problem, so we write to courseware_studentmodulehistory + # as well as courseware_studentmodule + with self.assertNumQueries(3): + self.kvs.set(user_state_key('not_a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_delete_existing_field(self): "Test that deleting an existing field removes it from the StudentModule" - self.kvs.delete(user_state_key('a_field')) + # We are updating a problem, so we write to courseware_studentmodulehistory + # as well as courseware_studentmodule + with self.assertNumQueries(3): + self.kvs.delete(user_state_key('a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) def test_delete_missing_field(self): "Test that deleting a missing field from an existing StudentModule raises a KeyError" - self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field')) + with self.assertNumQueries(0): + self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_has_existing_field(self): "Test that `has` returns True for existing fields in StudentModules" - self.assertTrue(self.kvs.has(user_state_key('a_field'))) + with self.assertNumQueries(0): + self.assertTrue(self.kvs.has(user_state_key('a_field'))) def test_has_missing_field(self): "Test that `has` returns False for missing fields in StudentModule" - self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) + with self.assertNumQueries(0): + self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) def construct_kv_dict(self): """Construct a kv_dict that can be passed to set_many""" @@ -160,7 +180,12 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): def test_set_many(self): "Test setting many fields that are scoped to Scope.user_state" kv_dict = self.construct_kv_dict() - self.kvs.set_many(kv_dict) + + # Scope.user_state is stored in a single row in the database, so we only + # need to send a single update to that table. + # We also are updating a problem, so we write to courseware student module history + with self.assertNumQueries(3): + self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -185,19 +210,26 @@ class TestMissingStudentModule(TestCase): self.user = UserFactory.create(username='user') self.assertEqual(self.user.id, 1) # check our assumption hard-coded in the key functions above. - self.field_data_cache = FieldDataCache([mock_descriptor()], course_id, self.user) + + # The descriptor has no fields, so FDC shouldn't send any queries + with self.assertNumQueries(0): + self.field_data_cache = FieldDataCache([mock_descriptor()], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_get_field_from_missing_student_module(self): "Test that getting a field from a missing StudentModule raises a KeyError" - self.assertRaises(KeyError, self.kvs.get, user_state_key('a_field')) + with self.assertNumQueries(0): + self.assertRaises(KeyError, self.kvs.get, user_state_key('a_field')) def test_set_field_in_missing_student_module(self): "Test that setting a field in a missing StudentModule creates the student module" self.assertEquals(0, len(self.field_data_cache.cache)) self.assertEquals(0, StudentModule.objects.all().count()) - self.kvs.set(user_state_key('a_field'), 'a_value') + # We are updating a problem, so we write to courseware_studentmodulehistory + # as well as courseware_studentmodule + with self.assertNumQueries(6): + self.kvs.set(user_state_key('a_field'), 'a_value') self.assertEquals(1, len(self.field_data_cache.cache)) self.assertEquals(1, StudentModule.objects.all().count()) @@ -210,11 +242,13 @@ class TestMissingStudentModule(TestCase): def test_delete_field_from_missing_student_module(self): "Test that deleting a field from a missing StudentModule raises a KeyError" - self.assertRaises(KeyError, self.kvs.delete, user_state_key('a_field')) + with self.assertNumQueries(0): + self.assertRaises(KeyError, self.kvs.delete, user_state_key('a_field')) def test_has_field_for_missing_student_module(self): "Test that `has` returns False for missing StudentModules" - self.assertFalse(self.kvs.has(user_state_key('a_field'))) + with self.assertNumQueries(0): + self.assertFalse(self.kvs.has(user_state_key('a_field'))) class StorageTestBase(object): @@ -240,51 +274,64 @@ class StorageTestBase(object): self.mock_descriptor = mock_descriptor([ mock_field(self.scope, 'existing_field'), mock_field(self.scope, 'other_existing_field')]) - self.field_data_cache = FieldDataCache([self.mock_descriptor], course_id, self.user) + # Each field is stored as a separate row in the table, + # but we can query them in a single query + with self.assertNumQueries(1): + self.field_data_cache = FieldDataCache([self.mock_descriptor], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_set_and_get_existing_field(self): - self.kvs.set(self.key_factory('existing_field'), 'test_value') - self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) + with self.assertNumQueries(2): + self.kvs.set(self.key_factory('existing_field'), 'test_value') + with self.assertNumQueries(0): + self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) def test_get_existing_field(self): "Test that getting an existing field in an existing Storage Field works" - self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field'))) + with self.assertNumQueries(0): + self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field'))) def test_get_missing_field(self): "Test that getting a missing field from an existing Storage Field raises a KeyError" - self.assertRaises(KeyError, self.kvs.get, self.key_factory('missing_field')) + with self.assertNumQueries(0): + self.assertRaises(KeyError, self.kvs.get, self.key_factory('missing_field')) def test_set_existing_field(self): "Test that setting an existing field changes the value" - self.kvs.set(self.key_factory('existing_field'), 'new_value') + with self.assertNumQueries(2): + self.kvs.set(self.key_factory('existing_field'), 'new_value') self.assertEquals(1, self.storage_class.objects.all().count()) self.assertEquals('new_value', json.loads(self.storage_class.objects.all()[0].value)) def test_set_missing_field(self): "Test that setting a new field changes the value" - self.kvs.set(self.key_factory('missing_field'), 'new_value') + with self.assertNumQueries(4): + self.kvs.set(self.key_factory('missing_field'), 'new_value') self.assertEquals(2, self.storage_class.objects.all().count()) self.assertEquals('old_value', json.loads(self.storage_class.objects.get(field_name='existing_field').value)) self.assertEquals('new_value', json.loads(self.storage_class.objects.get(field_name='missing_field').value)) def test_delete_existing_field(self): "Test that deleting an existing field removes it" - self.kvs.delete(self.key_factory('existing_field')) + with self.assertNumQueries(1): + self.kvs.delete(self.key_factory('existing_field')) self.assertEquals(0, self.storage_class.objects.all().count()) def test_delete_missing_field(self): "Test that deleting a missing field from an existing Storage Field raises a KeyError" - self.assertRaises(KeyError, self.kvs.delete, self.key_factory('missing_field')) + with self.assertNumQueries(0): + self.assertRaises(KeyError, self.kvs.delete, self.key_factory('missing_field')) self.assertEquals(1, self.storage_class.objects.all().count()) def test_has_existing_field(self): "Test that `has` returns True for an existing Storage Field" - self.assertTrue(self.kvs.has(self.key_factory('existing_field'))) + with self.assertNumQueries(0): + self.assertTrue(self.kvs.has(self.key_factory('existing_field'))) def test_has_missing_field(self): "Test that `has` return False for an existing Storage Field" - self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) + with self.assertNumQueries(0): + self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) def construct_kv_dict(self): """Construct a kv_dict that can be passed to set_many""" @@ -298,15 +345,19 @@ class StorageTestBase(object): """Test that setting many regular fields at the same time works""" kv_dict = self.construct_kv_dict() - self.kvs.set_many(kv_dict) + # Each field is a separate row in the database, hence + # a separate query + with self.assertNumQueries(len(kv_dict)*3): + self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) def test_set_many_failure(self): """Test that setting many regular fields with a DB error """ kv_dict = self.construct_kv_dict() - for key in kv_dict: - self.kvs.set(key, 'test value') + with self.assertNumQueries(6): + for key in kv_dict: + self.kvs.set(key, 'test value') with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]): with self.assertRaises(KeyValueMultiSaveError) as exception_context: From af12b1b8f0a5f62f1e612b132fc2bbf961ebd5ce Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 6 Mar 2015 14:58:13 -0500 Subject: [PATCH 02/14] Decrease the number of inserts and updates needed by DjangoKeyValueStore --- lms/djangoapps/courseware/model_data.py | 56 +++++++++---------- .../courseware/tests/test_model_data.py | 23 ++++---- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index eb0993ee4e..3e660d1848 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -266,7 +266,7 @@ class FieldDataCache(object): def find_or_create(self, key): ''' - Find a model data object in this cache, or create it if it doesn't + Find a model data object in this cache, or create a new one if it doesn't exist ''' field_object = self.find(key) @@ -275,28 +275,26 @@ class FieldDataCache(object): return field_object if key.scope == Scope.user_state: - field_object, __ = StudentModule.objects.get_or_create( + field_object = StudentModule( course_id=self.course_id, student_id=key.user_id, module_state_key=key.block_scope_id, - defaults={ - 'state': json.dumps({}), - 'module_type': key.block_scope_id.block_type, - }, + state=json.dumps({}), + module_type=key.block_scope_id.block_type, ) elif key.scope == Scope.user_state_summary: - field_object, __ = XModuleUserStateSummaryField.objects.get_or_create( + field_object = XModuleUserStateSummaryField( field_name=key.field_name, usage_id=key.block_scope_id ) elif key.scope == Scope.preferences: - field_object, __ = XModuleStudentPrefsField.objects.get_or_create( + field_object = XModuleStudentPrefsField( field_name=key.field_name, module_type=BlockTypeKeyV1(key.block_family, key.block_scope_id), student_id=key.user_id, ) elif key.scope == Scope.user_info: - field_object, __ = XModuleStudentInfoField.objects.get_or_create( + field_object = XModuleStudentInfoField( field_name=key.field_name, student_id=key.user_id, ) @@ -362,39 +360,39 @@ class DjangoKeyValueStore(KeyValueStore): """ saved_fields = [] - # field_objects maps a field_object to a list of associated fields - field_objects = dict() - for field in kv_dict: - # Check field for validity - if field.scope not in self._allowed_scopes: - raise InvalidScopeError(field) + # field_objects maps id(field_object) to a the object and a list of associated fields. + # We use id() because FieldDataCache might return django models with no primary key + # set, but will return the same django model each time the same key is passed in. + dirty_field_objects = defaultdict(lambda: (None, [])) + for key in kv_dict: + # Check key for validity + if key.scope not in self._allowed_scopes: + raise InvalidScopeError(key) - # If the field is valid and isn't already in the dictionary, add it. - field_object = self._field_data_cache.find_or_create(field) - if field_object not in field_objects.keys(): - field_objects[field_object] = [] - # Update the list of associated fields - field_objects[field_object].append(field) + field_object = self._field_data_cache.find_or_create(key) + # Update the list dirtied field_objects + _, dirty_names = dirty_field_objects.setdefault(id(field_object), (field_object, [])) + dirty_names.append(key.field_name) # Special case when scope is for the user state, because this scope saves fields in a single row - if field.scope == Scope.user_state: + if key.scope == Scope.user_state: state = json.loads(field_object.state) - state[field.field_name] = kv_dict[field] + state[key.field_name] = kv_dict[key] field_object.state = json.dumps(state) else: # The remaining scopes save fields on different rows, so # we don't have to worry about conflicts - field_object.value = json.dumps(kv_dict[field]) + field_object.value = json.dumps(kv_dict[key]) - for field_object in field_objects: + for field_object, names in dirty_field_objects.values(): try: # Save the field object that we made above - field_object.save() + field_object.save(force_update=field_object.pk is not None) # If save is successful on this scope, add the saved fields to # the list of successful saves - saved_fields.extend([field.field_name for field in field_objects[field_object]]) + saved_fields.extend(names) except DatabaseError: - log.exception('Error saving fields %r', field_objects[field_object]) + log.exception('Error saving fields %r', names) raise KeyValueMultiSaveError(saved_fields) def delete(self, key): @@ -409,7 +407,7 @@ class DjangoKeyValueStore(KeyValueStore): state = json.loads(field_object.state) del state[key.field_name] field_object.state = json.dumps(state) - field_object.save() + field_object.save(force_update=field_object.pk is not None) else: field_object.delete() diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 770391b142..3d203bed6b 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -129,7 +129,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): "Test that setting an existing user_state field changes the value" # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(3): + with self.assertNumQueries(2): self.kvs.set(user_state_key('a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) @@ -138,7 +138,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): "Test that setting a new user_state field changes the value" # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(3): + with self.assertNumQueries(2): self.kvs.set(user_state_key('not_a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) @@ -147,7 +147,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): "Test that deleting an existing field removes it from the StudentModule" # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(3): + with self.assertNumQueries(2): self.kvs.delete(user_state_key('a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) @@ -184,7 +184,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # Scope.user_state is stored in a single row in the database, so we only # need to send a single update to that table. # We also are updating a problem, so we write to courseware student module history - with self.assertNumQueries(3): + with self.assertNumQueries(2): self.kvs.set_many(kv_dict) for key in kv_dict: @@ -228,7 +228,7 @@ class TestMissingStudentModule(TestCase): # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(6): + with self.assertNumQueries(2): self.kvs.set(user_state_key('a_field'), 'a_value') self.assertEquals(1, len(self.field_data_cache.cache)) @@ -281,7 +281,7 @@ class StorageTestBase(object): self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_set_and_get_existing_field(self): - with self.assertNumQueries(2): + with self.assertNumQueries(1): self.kvs.set(self.key_factory('existing_field'), 'test_value') with self.assertNumQueries(0): self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) @@ -298,14 +298,14 @@ class StorageTestBase(object): def test_set_existing_field(self): "Test that setting an existing field changes the value" - with self.assertNumQueries(2): + with self.assertNumQueries(1): self.kvs.set(self.key_factory('existing_field'), 'new_value') self.assertEquals(1, self.storage_class.objects.all().count()) self.assertEquals('new_value', json.loads(self.storage_class.objects.all()[0].value)) def test_set_missing_field(self): "Test that setting a new field changes the value" - with self.assertNumQueries(4): + with self.assertNumQueries(1): self.kvs.set(self.key_factory('missing_field'), 'new_value') self.assertEquals(2, self.storage_class.objects.all().count()) self.assertEquals('old_value', json.loads(self.storage_class.objects.get(field_name='existing_field').value)) @@ -347,7 +347,7 @@ class StorageTestBase(object): # Each field is a separate row in the database, hence # a separate query - with self.assertNumQueries(len(kv_dict)*3): + with self.assertNumQueries(len(kv_dict)): self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -355,8 +355,8 @@ class StorageTestBase(object): def test_set_many_failure(self): """Test that setting many regular fields with a DB error """ kv_dict = self.construct_kv_dict() - with self.assertNumQueries(6): - for key in kv_dict: + for key in kv_dict: + with self.assertNumQueries(1): self.kvs.set(key, 'test value') with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]): @@ -365,7 +365,6 @@ class StorageTestBase(object): exception = exception_context.exception self.assertEquals(len(exception.saved_field_names), 1) - self.assertEquals(exception.saved_field_names[0], 'existing_field') class TestUserStateSummaryStorage(StorageTestBase, TestCase): From 8247ace5957694a8a66b7f9336c260d402ffdf0d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 16 Mar 2015 09:41:40 -0400 Subject: [PATCH 03/14] Revert "Merge pull request #7258 from cpennington/lms-field-data-query-counts" This reverts commit 4856ef78d39719834318ea063b64ddf8d2f1e571, reversing changes made to 7abbbb443a4eb6b5b6cc1650e0d677119bb2293d. --- lms/djangoapps/courseware/model_data.py | 56 ++++----- .../courseware/tests/test_model_data.py | 108 +++++------------- 2 files changed, 58 insertions(+), 106 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 3e660d1848..eb0993ee4e 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -266,7 +266,7 @@ class FieldDataCache(object): def find_or_create(self, key): ''' - Find a model data object in this cache, or create a new one if it doesn't + Find a model data object in this cache, or create it if it doesn't exist ''' field_object = self.find(key) @@ -275,26 +275,28 @@ class FieldDataCache(object): return field_object if key.scope == Scope.user_state: - field_object = StudentModule( + field_object, __ = StudentModule.objects.get_or_create( course_id=self.course_id, student_id=key.user_id, module_state_key=key.block_scope_id, - state=json.dumps({}), - module_type=key.block_scope_id.block_type, + defaults={ + 'state': json.dumps({}), + 'module_type': key.block_scope_id.block_type, + }, ) elif key.scope == Scope.user_state_summary: - field_object = XModuleUserStateSummaryField( + field_object, __ = XModuleUserStateSummaryField.objects.get_or_create( field_name=key.field_name, usage_id=key.block_scope_id ) elif key.scope == Scope.preferences: - field_object = XModuleStudentPrefsField( + field_object, __ = XModuleStudentPrefsField.objects.get_or_create( field_name=key.field_name, module_type=BlockTypeKeyV1(key.block_family, key.block_scope_id), student_id=key.user_id, ) elif key.scope == Scope.user_info: - field_object = XModuleStudentInfoField( + field_object, __ = XModuleStudentInfoField.objects.get_or_create( field_name=key.field_name, student_id=key.user_id, ) @@ -360,39 +362,39 @@ class DjangoKeyValueStore(KeyValueStore): """ saved_fields = [] - # field_objects maps id(field_object) to a the object and a list of associated fields. - # We use id() because FieldDataCache might return django models with no primary key - # set, but will return the same django model each time the same key is passed in. - dirty_field_objects = defaultdict(lambda: (None, [])) - for key in kv_dict: - # Check key for validity - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) + # field_objects maps a field_object to a list of associated fields + field_objects = dict() + for field in kv_dict: + # Check field for validity + if field.scope not in self._allowed_scopes: + raise InvalidScopeError(field) - field_object = self._field_data_cache.find_or_create(key) - # Update the list dirtied field_objects - _, dirty_names = dirty_field_objects.setdefault(id(field_object), (field_object, [])) - dirty_names.append(key.field_name) + # If the field is valid and isn't already in the dictionary, add it. + field_object = self._field_data_cache.find_or_create(field) + if field_object not in field_objects.keys(): + field_objects[field_object] = [] + # Update the list of associated fields + field_objects[field_object].append(field) # Special case when scope is for the user state, because this scope saves fields in a single row - if key.scope == Scope.user_state: + if field.scope == Scope.user_state: state = json.loads(field_object.state) - state[key.field_name] = kv_dict[key] + state[field.field_name] = kv_dict[field] field_object.state = json.dumps(state) else: # The remaining scopes save fields on different rows, so # we don't have to worry about conflicts - field_object.value = json.dumps(kv_dict[key]) + field_object.value = json.dumps(kv_dict[field]) - for field_object, names in dirty_field_objects.values(): + for field_object in field_objects: try: # Save the field object that we made above - field_object.save(force_update=field_object.pk is not None) + field_object.save() # If save is successful on this scope, add the saved fields to # the list of successful saves - saved_fields.extend(names) + saved_fields.extend([field.field_name for field in field_objects[field_object]]) except DatabaseError: - log.exception('Error saving fields %r', names) + log.exception('Error saving fields %r', field_objects[field_object]) raise KeyValueMultiSaveError(saved_fields) def delete(self, key): @@ -407,7 +409,7 @@ class DjangoKeyValueStore(KeyValueStore): state = json.loads(field_object.state) del state[key.field_name] field_object.state = json.dumps(state) - field_object.save(force_update=field_object.pk is not None) + field_object.save() else: field_object.delete() diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 3d203bed6b..80b4c04512 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -106,68 +106,48 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value', 'b_field': 'b_value'})) self.user = student_module.student self.assertEqual(self.user.id, 1) # check our assumption hard-coded in the key functions above. - - # There should be only one query to load a single descriptor with a single user_state field - with self.assertNumQueries(1): - self.field_data_cache = FieldDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user) - + self.field_data_cache = FieldDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_get_existing_field(self): "Test that getting an existing field in an existing StudentModule works" - # This should only read from the cache, not the database - with self.assertNumQueries(0): - self.assertEquals('a_value', self.kvs.get(user_state_key('a_field'))) + self.assertEquals('a_value', self.kvs.get(user_state_key('a_field'))) def test_get_missing_field(self): "Test that getting a missing field from an existing StudentModule raises a KeyError" - # This should only read from the cache, not the database - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) + self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) def test_set_existing_field(self): "Test that setting an existing user_state field changes the value" - # We are updating a problem, so we write to courseware_studentmodulehistory - # as well as courseware_studentmodule - with self.assertNumQueries(2): - self.kvs.set(user_state_key('a_field'), 'new_value') + self.kvs.set(user_state_key('a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_set_missing_field(self): "Test that setting a new user_state field changes the value" - # We are updating a problem, so we write to courseware_studentmodulehistory - # as well as courseware_studentmodule - with self.assertNumQueries(2): - self.kvs.set(user_state_key('not_a_field'), 'new_value') + self.kvs.set(user_state_key('not_a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_delete_existing_field(self): "Test that deleting an existing field removes it from the StudentModule" - # We are updating a problem, so we write to courseware_studentmodulehistory - # as well as courseware_studentmodule - with self.assertNumQueries(2): - self.kvs.delete(user_state_key('a_field')) + self.kvs.delete(user_state_key('a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) def test_delete_missing_field(self): "Test that deleting a missing field from an existing StudentModule raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field')) + self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_has_existing_field(self): "Test that `has` returns True for existing fields in StudentModules" - with self.assertNumQueries(0): - self.assertTrue(self.kvs.has(user_state_key('a_field'))) + self.assertTrue(self.kvs.has(user_state_key('a_field'))) def test_has_missing_field(self): "Test that `has` returns False for missing fields in StudentModule" - with self.assertNumQueries(0): - self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) + self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) def construct_kv_dict(self): """Construct a kv_dict that can be passed to set_many""" @@ -180,12 +160,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): def test_set_many(self): "Test setting many fields that are scoped to Scope.user_state" kv_dict = self.construct_kv_dict() - - # Scope.user_state is stored in a single row in the database, so we only - # need to send a single update to that table. - # We also are updating a problem, so we write to courseware student module history - with self.assertNumQueries(2): - self.kvs.set_many(kv_dict) + self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -210,26 +185,19 @@ class TestMissingStudentModule(TestCase): self.user = UserFactory.create(username='user') self.assertEqual(self.user.id, 1) # check our assumption hard-coded in the key functions above. - - # The descriptor has no fields, so FDC shouldn't send any queries - with self.assertNumQueries(0): - self.field_data_cache = FieldDataCache([mock_descriptor()], course_id, self.user) + self.field_data_cache = FieldDataCache([mock_descriptor()], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_get_field_from_missing_student_module(self): "Test that getting a field from a missing StudentModule raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.get, user_state_key('a_field')) + self.assertRaises(KeyError, self.kvs.get, user_state_key('a_field')) def test_set_field_in_missing_student_module(self): "Test that setting a field in a missing StudentModule creates the student module" self.assertEquals(0, len(self.field_data_cache.cache)) self.assertEquals(0, StudentModule.objects.all().count()) - # We are updating a problem, so we write to courseware_studentmodulehistory - # as well as courseware_studentmodule - with self.assertNumQueries(2): - self.kvs.set(user_state_key('a_field'), 'a_value') + self.kvs.set(user_state_key('a_field'), 'a_value') self.assertEquals(1, len(self.field_data_cache.cache)) self.assertEquals(1, StudentModule.objects.all().count()) @@ -242,13 +210,11 @@ class TestMissingStudentModule(TestCase): def test_delete_field_from_missing_student_module(self): "Test that deleting a field from a missing StudentModule raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.delete, user_state_key('a_field')) + self.assertRaises(KeyError, self.kvs.delete, user_state_key('a_field')) def test_has_field_for_missing_student_module(self): "Test that `has` returns False for missing StudentModules" - with self.assertNumQueries(0): - self.assertFalse(self.kvs.has(user_state_key('a_field'))) + self.assertFalse(self.kvs.has(user_state_key('a_field'))) class StorageTestBase(object): @@ -274,64 +240,51 @@ class StorageTestBase(object): self.mock_descriptor = mock_descriptor([ mock_field(self.scope, 'existing_field'), mock_field(self.scope, 'other_existing_field')]) - # Each field is stored as a separate row in the table, - # but we can query them in a single query - with self.assertNumQueries(1): - self.field_data_cache = FieldDataCache([self.mock_descriptor], course_id, self.user) + self.field_data_cache = FieldDataCache([self.mock_descriptor], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_set_and_get_existing_field(self): - with self.assertNumQueries(1): - self.kvs.set(self.key_factory('existing_field'), 'test_value') - with self.assertNumQueries(0): - self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) + self.kvs.set(self.key_factory('existing_field'), 'test_value') + self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) def test_get_existing_field(self): "Test that getting an existing field in an existing Storage Field works" - with self.assertNumQueries(0): - self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field'))) + self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field'))) def test_get_missing_field(self): "Test that getting a missing field from an existing Storage Field raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.get, self.key_factory('missing_field')) + self.assertRaises(KeyError, self.kvs.get, self.key_factory('missing_field')) def test_set_existing_field(self): "Test that setting an existing field changes the value" - with self.assertNumQueries(1): - self.kvs.set(self.key_factory('existing_field'), 'new_value') + self.kvs.set(self.key_factory('existing_field'), 'new_value') self.assertEquals(1, self.storage_class.objects.all().count()) self.assertEquals('new_value', json.loads(self.storage_class.objects.all()[0].value)) def test_set_missing_field(self): "Test that setting a new field changes the value" - with self.assertNumQueries(1): - self.kvs.set(self.key_factory('missing_field'), 'new_value') + self.kvs.set(self.key_factory('missing_field'), 'new_value') self.assertEquals(2, self.storage_class.objects.all().count()) self.assertEquals('old_value', json.loads(self.storage_class.objects.get(field_name='existing_field').value)) self.assertEquals('new_value', json.loads(self.storage_class.objects.get(field_name='missing_field').value)) def test_delete_existing_field(self): "Test that deleting an existing field removes it" - with self.assertNumQueries(1): - self.kvs.delete(self.key_factory('existing_field')) + self.kvs.delete(self.key_factory('existing_field')) self.assertEquals(0, self.storage_class.objects.all().count()) def test_delete_missing_field(self): "Test that deleting a missing field from an existing Storage Field raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.delete, self.key_factory('missing_field')) + self.assertRaises(KeyError, self.kvs.delete, self.key_factory('missing_field')) self.assertEquals(1, self.storage_class.objects.all().count()) def test_has_existing_field(self): "Test that `has` returns True for an existing Storage Field" - with self.assertNumQueries(0): - self.assertTrue(self.kvs.has(self.key_factory('existing_field'))) + self.assertTrue(self.kvs.has(self.key_factory('existing_field'))) def test_has_missing_field(self): "Test that `has` return False for an existing Storage Field" - with self.assertNumQueries(0): - self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) + self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) def construct_kv_dict(self): """Construct a kv_dict that can be passed to set_many""" @@ -345,10 +298,7 @@ class StorageTestBase(object): """Test that setting many regular fields at the same time works""" kv_dict = self.construct_kv_dict() - # Each field is a separate row in the database, hence - # a separate query - with self.assertNumQueries(len(kv_dict)): - self.kvs.set_many(kv_dict) + self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -356,8 +306,7 @@ class StorageTestBase(object): """Test that setting many regular fields with a DB error """ kv_dict = self.construct_kv_dict() for key in kv_dict: - with self.assertNumQueries(1): - self.kvs.set(key, 'test value') + self.kvs.set(key, 'test value') with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]): with self.assertRaises(KeyValueMultiSaveError) as exception_context: @@ -365,6 +314,7 @@ class StorageTestBase(object): exception = exception_context.exception self.assertEquals(len(exception.saved_field_names), 1) + self.assertEquals(exception.saved_field_names[0], 'existing_field') class TestUserStateSummaryStorage(StorageTestBase, TestCase): From b625e8e37e7d71fabb63af22fb6e8f7fbe14d6c7 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 13 Mar 2015 16:41:47 -0400 Subject: [PATCH 04/14] Skip CSRF referer check for cross-domain requests. This commit extends the workaround in `cors_csrf` middleware to Django Rest Framework's SessionAuthentication, which calls Django's CSRF middleware directly. The workaround checks the cross domain whitelist and skips the CSRF referer check for domains on the whitelist. --- common/djangoapps/cors_csrf/authentication.py | 29 ++++++ common/djangoapps/cors_csrf/helpers.py | 92 +++++++++++++++++++ common/djangoapps/cors_csrf/middleware.py | 84 +---------------- .../cors_csrf/tests/test_authentication.py | 56 +++++++++++ .../djangoapps/enrollment/tests/test_views.py | 80 ++++++++++++++++ common/djangoapps/enrollment/views.py | 8 +- 6 files changed, 268 insertions(+), 81 deletions(-) create mode 100644 common/djangoapps/cors_csrf/authentication.py create mode 100644 common/djangoapps/cors_csrf/helpers.py create mode 100644 common/djangoapps/cors_csrf/tests/test_authentication.py diff --git a/common/djangoapps/cors_csrf/authentication.py b/common/djangoapps/cors_csrf/authentication.py new file mode 100644 index 0000000000..723ec8eed1 --- /dev/null +++ b/common/djangoapps/cors_csrf/authentication.py @@ -0,0 +1,29 @@ +"""Django Rest Framework Authentication classes for cross-domain end-points.""" +from rest_framework import authentication +from cors_csrf.helpers import is_cross_domain_request_allowed, skip_cross_domain_referer_check + + +class SessionAuthenticationCrossDomainCsrf(authentication.SessionAuthentication): + """Session authentication that skips the referer check over secure connections. + + Django Rest Framework's `SessionAuthentication` class calls Django's + CSRF middleware implementation directly, which bypasses the middleware + stack. + + This version of `SessionAuthentication` performs the same workaround + as `CorsCSRFMiddleware` to skip the referer check for whitelisted + domains over a secure connection. See `cors_csrf.middleware` for + more information. + + Since this subclass overrides only the `enforce_csrf()` method, + it can be mixed in with other `SessionAuthentication` subclasses. + + """ + + def enforce_csrf(self, request): + """Skip the referer check if the cross-domain request is allowed. """ + if is_cross_domain_request_allowed(request): + with skip_cross_domain_referer_check(request): + return super(SessionAuthenticationCrossDomainCsrf, self).enforce_csrf(request) + else: + return super(SessionAuthenticationCrossDomainCsrf, self).enforce_csrf(request) diff --git a/common/djangoapps/cors_csrf/helpers.py b/common/djangoapps/cors_csrf/helpers.py new file mode 100644 index 0000000000..b04db29b25 --- /dev/null +++ b/common/djangoapps/cors_csrf/helpers.py @@ -0,0 +1,92 @@ +"""Helper methods for CORS and CSRF checks. """ +import logging +import urlparse +import contextlib + +from django.conf import settings + +log = logging.getLogger(__name__) + + +def is_cross_domain_request_allowed(request): + """Check whether we should allow the cross-domain request. + + We allow a cross-domain request only if: + + 1) The request is made securely and the referer has "https://" as the protocol. + 2) The referer domain has been whitelisted. + + Arguments: + request (HttpRequest) + + Returns: + bool + + """ + referer = request.META.get('HTTP_REFERER') + referer_parts = urlparse.urlparse(referer) if referer else None + referer_hostname = referer_parts.hostname if referer_parts is not None else None + + # Use CORS_ALLOW_INSECURE *only* for development and testing environments; + # it should never be enabled in production. + if not getattr(settings, 'CORS_ALLOW_INSECURE', False): + if not request.is_secure(): + log.debug( + u"Request is not secure, so we cannot send the CSRF token. " + u"For testing purposes, you can disable this check by setting " + u"`CORS_ALLOW_INSECURE` to True in the settings" + ) + return False + + if not referer: + log.debug(u"No referer provided over a secure connection, so we cannot check the protocol.") + return False + + if not referer_parts.scheme == 'https': + log.debug(u"Referer '%s' must have the scheme 'https'") + return False + + domain_is_whitelisted = ( + getattr(settings, 'CORS_ORIGIN_ALLOW_ALL', False) or + referer_hostname in getattr(settings, 'CORS_ORIGIN_WHITELIST', []) + ) + if not domain_is_whitelisted: + if referer_hostname is None: + # If no referer is specified, we can't check if it's a cross-domain + # request or not. + log.debug(u"Referrer hostname is `None`, so it is not on the whitelist.") + elif referer_hostname != request.get_host(): + log.warning( + ( + u"Domain '%s' is not on the cross domain whitelist. " + u"Add the domain to `CORS_ORIGIN_WHITELIST` or set " + u"`CORS_ORIGIN_ALLOW_ALL` to True in the settings." + ), referer_hostname + ) + else: + log.debug( + ( + u"Domain '%s' is the same as the hostname in the request, " + u"so we are not going to treat it as a cross-domain request." + ), referer_hostname + ) + return False + + return True + + +@contextlib.contextmanager +def skip_cross_domain_referer_check(request): + """Skip the cross-domain CSRF referer check. + + Django's CSRF middleware performs the referer check + only when the request is made over a secure connection. + To skip the check, we patch `request.is_secure()` to + False. + """ + is_secure_default = request.is_secure + request.is_secure = lambda: False + try: + yield + finally: + request.is_secure = is_secure_default diff --git a/common/djangoapps/cors_csrf/middleware.py b/common/djangoapps/cors_csrf/middleware.py index 920acaeea9..dfd554d9dc 100644 --- a/common/djangoapps/cors_csrf/middleware.py +++ b/common/djangoapps/cors_csrf/middleware.py @@ -43,82 +43,16 @@ CSRF cookie. """ import logging -import urlparse from django.conf import settings from django.middleware.csrf import CsrfViewMiddleware from django.core.exceptions import MiddlewareNotUsed, ImproperlyConfigured +from cors_csrf.helpers import is_cross_domain_request_allowed, skip_cross_domain_referer_check + log = logging.getLogger(__name__) -def is_cross_domain_request_allowed(request): - """Check whether we should allow the cross-domain request. - - We allow a cross-domain request only if: - - 1) The request is made securely and the referer has "https://" as the protocol. - 2) The referer domain has been whitelisted. - - Arguments: - request (HttpRequest) - - Returns: - bool - - """ - referer = request.META.get('HTTP_REFERER') - referer_parts = urlparse.urlparse(referer) if referer else None - referer_hostname = referer_parts.hostname if referer_parts is not None else None - - # Use CORS_ALLOW_INSECURE *only* for development and testing environments; - # it should never be enabled in production. - if not getattr(settings, 'CORS_ALLOW_INSECURE', False): - if not request.is_secure(): - log.debug( - u"Request is not secure, so we cannot send the CSRF token. " - u"For testing purposes, you can disable this check by setting " - u"`CORS_ALLOW_INSECURE` to True in the settings" - ) - return False - - if not referer: - log.debug(u"No referer provided over a secure connection, so we cannot check the protocol.") - return False - - if not referer_parts.scheme == 'https': - log.debug(u"Referer '%s' must have the scheme 'https'") - return False - - domain_is_whitelisted = ( - getattr(settings, 'CORS_ORIGIN_ALLOW_ALL', False) or - referer_hostname in getattr(settings, 'CORS_ORIGIN_WHITELIST', []) - ) - if not domain_is_whitelisted: - if referer_hostname is None: - # If no referer is specified, we can't check if it's a cross-domain - # request or not. - log.debug(u"Referrer hostname is `None`, so it is not on the whitelist.") - elif referer_hostname != request.get_host(): - log.warning( - ( - u"Domain '%s' is not on the cross domain whitelist. " - u"Add the domain to `CORS_ORIGIN_WHITELIST` or set " - u"`CORS_ORIGIN_ALLOW_ALL` to True in the settings." - ), referer_hostname - ) - else: - log.debug( - ( - u"Domain '%s' is the same as the hostname in the request, " - u"so we are not going to treat it as a cross-domain request." - ), referer_hostname - ) - return False - - return True - - class CorsCSRFMiddleware(CsrfViewMiddleware): """ Middleware for handling CSRF checks with CORS requests @@ -134,18 +68,8 @@ class CorsCSRFMiddleware(CsrfViewMiddleware): log.debug("Could not disable CSRF middleware referer check for cross-domain request.") return - is_secure_default = request.is_secure - - def is_secure_patched(): - """ - Avoid triggering the additional CSRF middleware checks on the referrer - """ - return False - request.is_secure = is_secure_patched - - res = super(CorsCSRFMiddleware, self).process_view(request, callback, callback_args, callback_kwargs) - request.is_secure = is_secure_default - return res + with skip_cross_domain_referer_check(request): + return super(CorsCSRFMiddleware, self).process_view(request, callback, callback_args, callback_kwargs) class CsrfCrossDomainCookieMiddleware(object): diff --git a/common/djangoapps/cors_csrf/tests/test_authentication.py b/common/djangoapps/cors_csrf/tests/test_authentication.py new file mode 100644 index 0000000000..7f2d78fd8a --- /dev/null +++ b/common/djangoapps/cors_csrf/tests/test_authentication.py @@ -0,0 +1,56 @@ +"""Tests for the CORS CSRF version of Django Rest Framework's SessionAuthentication.""" +from mock import patch + +from django.test import TestCase +from django.test.utils import override_settings +from django.test.client import RequestFactory +from django.conf import settings + +from rest_framework.exceptions import AuthenticationFailed + +from cors_csrf.authentication import SessionAuthenticationCrossDomainCsrf + + +class CrossDomainAuthTest(TestCase): + """Tests for the CORS CSRF version of Django Rest Framework's SessionAuthentication. """ + + URL = "/dummy_url" + REFERER = "https://www.edx.org" + CSRF_TOKEN = 'abcd1234' + + def setUp(self): + super(CrossDomainAuthTest, self).setUp() + self.auth = SessionAuthenticationCrossDomainCsrf() + + def test_perform_csrf_referer_check(self): + request = self._fake_request() + with self.assertRaisesRegexp(AuthenticationFailed, 'CSRF'): + self.auth.enforce_csrf(request) + + @patch.dict(settings.FEATURES, { + 'ENABLE_CORS_HEADERS': True, + 'ENABLE_CROSS_DOMAIN_CSRF_COOKIE': True + }) + @override_settings( + CORS_ORIGIN_WHITELIST=["www.edx.org"], + CROSS_DOMAIN_CSRF_COOKIE_NAME="prod-edx-csrftoken", + CROSS_DOMAIN_CSRF_COOKIE_DOMAIN=".edx.org" + ) + def test_skip_csrf_referer_check(self): + request = self._fake_request() + result = self.auth.enforce_csrf(request) + self.assertIs(result, None) + self.assertTrue(request.is_secure()) + + def _fake_request(self): + """Construct a fake request with a referer and CSRF token over a secure connection. """ + factory = RequestFactory() + factory.cookies[settings.CSRF_COOKIE_NAME] = self.CSRF_TOKEN + + request = factory.post( + self.URL, + HTTP_REFERER=self.REFERER, + HTTP_X_CSRFTOKEN=self.CSRF_TOKEN + ) + request.is_secure = lambda: True + return request diff --git a/common/djangoapps/enrollment/tests/test_views.py b/common/djangoapps/enrollment/tests/test_views.py index 948b3e1cc4..a052c94522 100644 --- a/common/djangoapps/enrollment/tests/test_views.py +++ b/common/djangoapps/enrollment/tests/test_views.py @@ -6,6 +6,8 @@ import json import unittest from mock import patch +from django.test import Client +from django.core.handlers.wsgi import WSGIRequest from django.core.urlresolvers import reverse from rest_framework.test import APITestCase from rest_framework import status @@ -365,3 +367,81 @@ class EnrollmentEmbargoTest(UrlResetMixin, ModuleStoreTestCase): url = reverse('courseenrollments') resp = self.client.get(url) return json.loads(resp.content) + + +def cross_domain_config(func): + """Decorator for configuring a cross-domain request. """ + feature_flag_decorator = patch.dict(settings.FEATURES, { + 'ENABLE_CORS_HEADERS': True, + 'ENABLE_CROSS_DOMAIN_CSRF_COOKIE': True + }) + settings_decorator = override_settings( + CORS_ORIGIN_WHITELIST=["www.edx.org"], + CROSS_DOMAIN_CSRF_COOKIE_NAME="prod-edx-csrftoken", + CROSS_DOMAIN_CSRF_COOKIE_DOMAIN=".edx.org" + ) + is_secure_decorator = patch.object(WSGIRequest, 'is_secure', return_value=True) + + return feature_flag_decorator( + settings_decorator( + is_secure_decorator(func) + ) + ) + + +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +class EnrollmentCrossDomainTest(ModuleStoreTestCase): + """Test cross-domain calls to the enrollment end-points. """ + + USERNAME = "Bob" + EMAIL = "bob@example.com" + PASSWORD = "edx" + REFERER = "https://www.edx.org" + + def setUp(self): + """ Create a course and user, then log in. """ + super(EnrollmentCrossDomainTest, self).setUp() + self.course = CourseFactory.create() + self.user = UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) + + self.client = Client(enforce_csrf_checks=True) + self.client.login(username=self.USERNAME, password=self.PASSWORD) + + @cross_domain_config + def test_cross_domain_change_enrollment(self, *args): # pylint: disable=unused-argument + csrf_cookie = self._get_csrf_cookie() + resp = self._cross_domain_post(csrf_cookie) + + # Expect that the request gets through successfully, + # passing the CSRF checks (including the referer check). + self.assertEqual(resp.status_code, 200) + + @cross_domain_config + def test_cross_domain_missing_csrf(self, *args): # pylint: disable=unused-argument + resp = self._cross_domain_post('invalid_csrf_token') + self.assertEqual(resp.status_code, 401) + + def _get_csrf_cookie(self): + """Retrieve the cross-domain CSRF cookie. """ + url = reverse('courseenrollment', kwargs={ + 'course_id': unicode(self.course.id) + }) + resp = self.client.get(url, HTTP_REFERER=self.REFERER) + self.assertEqual(resp.status_code, 200) + self.assertIn('prod-edx-csrftoken', resp.cookies) # pylint: disable=no-member + return resp.cookies['prod-edx-csrftoken'].value # pylint: disable=no-member + + def _cross_domain_post(self, csrf_cookie): + """Perform a cross-domain POST request. """ + url = reverse('courseenrollments') + params = json.dumps({ + 'course_details': { + 'course_id': unicode(self.course.id), + }, + 'user': self.user.username + }) + return self.client.post( + url, params, content_type='application/json', + HTTP_REFERER=self.REFERER, + HTTP_X_CSRFTOKEN=csrf_cookie + ) diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index e2bb848d2c..4143a935be 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -14,6 +14,7 @@ from rest_framework.throttling import UserRateThrottle from rest_framework.views import APIView from opaque_keys.edx.keys import CourseKey from embargo import api as embargo_api +from cors_csrf.authentication import SessionAuthenticationCrossDomainCsrf from cors_csrf.decorators import ensure_csrf_cookie_cross_domain from util.authentication import SessionAuthenticationAllowInactiveUser, OAuth2AuthenticationAllowInactiveUser from util.disable_rate_limit import can_disable_rate_limit @@ -24,6 +25,11 @@ from enrollment.errors import ( ) +class EnrollmentCrossDomainSessionAuth(SessionAuthenticationAllowInactiveUser, SessionAuthenticationCrossDomainCsrf): + """Session authentication that allows inactive users and cross-domain requests. """ + pass + + class EnrollmentUserThrottle(UserRateThrottle): """Limit the number of requests users can make to the enrollment API.""" # TODO Limit significantly after performance testing. # pylint: disable=fixme @@ -267,7 +273,7 @@ class EnrollmentListView(APIView, ApiKeyPermissionMixIn): * user: The ID of the user. """ - authentication_classes = OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser + authentication_classes = OAuth2AuthenticationAllowInactiveUser, EnrollmentCrossDomainSessionAuth permission_classes = ApiKeyHeaderPermissionIsAuthenticated, throttle_classes = EnrollmentUserThrottle, From 5fdc8d666d8dc6c93b80b912457e140c26dad3e5 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Mon, 16 Mar 2015 15:23:21 +0500 Subject: [PATCH 05/14] Update ora2 to release-2015-03-16T17.59. --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 63f67eaca3..acf1d79a99 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -29,7 +29,7 @@ git+https://github.com/mitocw/django-cas.git@60a5b8e5a62e63e0d5d224a87f0b489201a -e git+https://github.com/edx/bok-choy.git@94430eb3e50e84330b81a3f5bc0ba4cc0171516f#egg=bok_choy -e git+https://github.com/edx-solutions/django-splash.git@7579d052afcf474ece1239153cffe1c89935bc4f#egg=django-splash -e git+https://github.com/edx/acid-block.git@e46f9cda8a03e121a00c7e347084d142d22ebfb7#egg=acid-xblock --e git+https://github.com/edx/edx-ora2.git@4773573b79bc530f0fe7c8f90a10491e4224dc2d#egg=edx-ora2 +-e git+https://github.com/edx/edx-ora2.git@release-2015-03-16T17.59#egg=edx-ora2 -e git+https://github.com/edx/edx-submissions.git@8fb070d2a3087dd7656d27022e550d12e3b85ba3#egg=edx-submissions -e git+https://github.com/edx/opaque-keys.git@1254ed4d615a428591850656f39f26509b86d30a#egg=opaque-keys -e git+https://github.com/edx/ease.git@97de68448e5495385ba043d3091f570a699d5b5f#egg=ease From d6046d786d3d72ee298ce6e3a682aa7a946196af Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Mon, 16 Mar 2015 13:50:18 -0400 Subject: [PATCH 06/14] Fixed bug for courses without an honor mode --- lms/djangoapps/commerce/constants.py | 1 + lms/djangoapps/commerce/tests.py | 27 ++++++++++++++++++++- lms/djangoapps/commerce/views.py | 35 ++++++++++++++++------------ 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/commerce/constants.py b/lms/djangoapps/commerce/constants.py index fbbeae0666..a59ff3fa95 100644 --- a/lms/djangoapps/commerce/constants.py +++ b/lms/djangoapps/commerce/constants.py @@ -19,3 +19,4 @@ class Messages(object): NO_SKU_ENROLLED = u'The {enrollment_mode} mode for {course_id} does not have a SKU. Enrolling {username} directly.' ORDER_COMPLETED = u'Order {order_number} was completed.' ORDER_INCOMPLETE_ENROLLED = u'Order {order_number} was created, but is not yet complete. User was enrolled.' + NO_HONOR_MODE = u'Course {course_id} does not have an honor mode.' diff --git a/lms/djangoapps/commerce/tests.py b/lms/djangoapps/commerce/tests.py index b3f79fbd91..e53853abbe 100644 --- a/lms/djangoapps/commerce/tests.py +++ b/lms/djangoapps/commerce/tests.py @@ -237,7 +237,6 @@ class OrdersViewTests(ModuleStoreTestCase): response = self._post_to_view() # Validate the response - self._mock_ecommerce_api() self.assertEqual(response.status_code, 200) msg = Messages.NO_ECOM_API.format(username=self.user.username, course_id=self.course.id) self.assertResponseMessage(response, msg) @@ -245,3 +244,29 @@ class OrdersViewTests(ModuleStoreTestCase): # Ensure that the user is not enrolled and that no calls were made to the E-Commerce API self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id)) self.assertIsInstance(httpretty.last_request(), HTTPrettyRequestEmpty) + + def _test_professional_mode_only(self): + """ Verifies that the view behaves appropriately when the course only has a professional mode. """ + CourseMode.objects.filter(course_id=self.course.id).delete() + mode = 'no-id-professional' + CourseModeFactory.create(course_id=self.course.id, mode_slug=mode, mode_display_name=mode, + sku=uuid4().hex.decode('ascii')) + self._mock_ecommerce_api() + response = self._post_to_view() + self.assertEqual(response.status_code, 406) + msg = Messages.NO_HONOR_MODE.format(course_id=self.course.id) + self.assertResponseMessage(response, msg) + + @httpretty.activate + def test_course_with_professional_mode_only(self): + """ Verifies that the view behaves appropriately when the course only has a professional mode. """ + self._test_professional_mode_only() + + @httpretty.activate + @override_settings(ECOMMERCE_API_URL=None, ECOMMERCE_API_SIGNING_KEY=None) + def test_no_settings_and_professional_mode_only(self): + """ + Verifies that the view behaves appropriately when the course only has a professional mode and + the E-Commerce API is not configured. + """ + self._test_professional_mode_only() diff --git a/lms/djangoapps/commerce/views.py b/lms/djangoapps/commerce/views.py index e029998e9e..3d9249b46a 100644 --- a/lms/djangoapps/commerce/views.py +++ b/lms/djangoapps/commerce/views.py @@ -79,28 +79,33 @@ class OrdersView(APIView): if not valid: return DetailResponse(error, status=HTTP_406_NOT_ACCEPTABLE) + # Ensure that the course has an honor mode with SKU + honor_mode = CourseMode.mode_for_course(course_key, CourseMode.HONOR) + course_id = unicode(course_key) + + # If there is no honor course mode, this most likely a Prof-Ed course. Return an error so that the JS + # redirects to track selection. + if not honor_mode: + msg = Messages.NO_HONOR_MODE.format(course_id=course_id) + return DetailResponse(msg, status=HTTP_406_NOT_ACCEPTABLE) + elif not honor_mode.sku: + # If there are no course modes with SKUs, enroll the user without contacting the external API. + msg = Messages.NO_SKU_ENROLLED.format(enrollment_mode=CourseMode.HONOR, course_id=course_id, + username=user.username) + log.debug(msg) + self._enroll(course_key, user) + return DetailResponse(msg) + # Ensure that the E-Commerce API is setup properly ecommerce_api_url = getattr(settings, 'ECOMMERCE_API_URL', None) ecommerce_api_signing_key = getattr(settings, 'ECOMMERCE_API_SIGNING_KEY', None) if not (ecommerce_api_url and ecommerce_api_signing_key): self._enroll(course_key, user) - msg = Messages.NO_ECOM_API.format(username=user.username, course_id=unicode(course_key)) + msg = Messages.NO_ECOM_API.format(username=user.username, course_id=course_id) log.debug(msg) return DetailResponse(msg) - # Default to honor mode. In the future we may expand this view to support additional modes. - mode = CourseMode.DEFAULT_MODE_SLUG - course_modes = CourseMode.objects.filter(course_id=course_key, mode_slug=mode, sku__isnull=False) - - # If there are no course modes with SKUs, enroll the user without contacting the external API. - if not course_modes.exists(): - msg = Messages.NO_SKU_ENROLLED.format(enrollment_mode=mode, course_id=unicode(course_key), - username=user.username) - log.debug(msg) - self._enroll(course_key, user) - return DetailResponse(msg) - # Contact external API headers = { 'Content-Type': 'application/json', @@ -111,7 +116,7 @@ class OrdersView(APIView): try: timeout = getattr(settings, 'ECOMMERCE_API_TIMEOUT', 5) - response = requests.post(url, data=json.dumps({'sku': course_modes[0].sku}), headers=headers, + response = requests.post(url, data=json.dumps({'sku': honor_mode.sku}), headers=headers, timeout=timeout) except Exception as ex: # pylint: disable=broad-except log.exception('Call to E-Commerce API failed: %s.', ex.message) @@ -143,7 +148,7 @@ class OrdersView(APIView): 'status': order_status, 'complete_status': OrderStatus.COMPLETE, 'username': user.username, - 'course_id': unicode(course_key), + 'course_id': course_id, } log.error(msg, msg_kwargs) From 081f5b4e535b0212c0025a0252bca7de60ca3e48 Mon Sep 17 00:00:00 2001 From: Stephen Sanchez Date: Mon, 16 Mar 2015 19:29:22 +0000 Subject: [PATCH 07/14] Add E-Commerce settings to AWS settings. --- lms/envs/aws.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lms/envs/aws.py b/lms/envs/aws.py index db43cf3c56..37741bd0c5 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -558,3 +558,8 @@ XBLOCK_SETTINGS = ENV_TOKENS.get('XBLOCK_SETTINGS', {}) ##### CDN EXPERIMENT/MONITORING FLAGS ##### PERFORMANCE_GRAPHITE_URL = ENV_TOKENS.get('PERFORMANCE_GRAPHITE_URL', PERFORMANCE_GRAPHITE_URL) CDN_VIDEO_URLS = ENV_TOKENS.get('CDN_VIDEO_URLS', CDN_VIDEO_URLS) + +##### ECOMMERCE API CONFIGURATION SETTINGS ##### +ECOMMERCE_API_URL = ENV_TOKENS.get('ECOMMERCE_API_URL') +ECOMMERCE_API_SIGNING_KEY = AUTH_TOKENS.get('ECOMMERCE_API_SIGNING_KEY') +ECOMMERCE_API_TIMEOUT = ENV_TOKENS.get('ECOMMERCE_API_TIMEOUT', 5) From f552df336395bd3598d789446607eb227c339e26 Mon Sep 17 00:00:00 2001 From: Stephen Sanchez Date: Mon, 16 Mar 2015 20:21:12 +0000 Subject: [PATCH 08/14] Modify commerce view to work without ecommerce configuration. --- lms/djangoapps/commerce/views.py | 7 +++---- lms/envs/aws.py | 6 +++--- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/commerce/views.py b/lms/djangoapps/commerce/views.py index e029998e9e..adde3d06c5 100644 --- a/lms/djangoapps/commerce/views.py +++ b/lms/djangoapps/commerce/views.py @@ -54,17 +54,16 @@ class OrdersView(APIView): return True, course_key, None - def _get_jwt(self, user): + def _get_jwt(self, user, ecommerce_api_signing_key): """ Returns a JWT object with the specified user's info. - Raises AttributeError if settings.ECOMMERCE_API_SIGNING_KEY is not set. """ data = { 'username': user.username, 'email': user.email } - return jwt.encode(data, getattr(settings, 'ECOMMERCE_API_SIGNING_KEY')) + return jwt.encode(data, ecommerce_api_signing_key) def _enroll(self, course_key, user): """ Enroll the user in the course. """ @@ -104,7 +103,7 @@ class OrdersView(APIView): # Contact external API headers = { 'Content-Type': 'application/json', - 'Authorization': 'JWT {}'.format(self._get_jwt(user)) + 'Authorization': 'JWT {}'.format(self._get_jwt(user, ecommerce_api_signing_key)) } url = '{}/orders/'.format(ecommerce_api_url.strip('/')) diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 37741bd0c5..b0a2580ea0 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -560,6 +560,6 @@ PERFORMANCE_GRAPHITE_URL = ENV_TOKENS.get('PERFORMANCE_GRAPHITE_URL', PERFORMANC CDN_VIDEO_URLS = ENV_TOKENS.get('CDN_VIDEO_URLS', CDN_VIDEO_URLS) ##### ECOMMERCE API CONFIGURATION SETTINGS ##### -ECOMMERCE_API_URL = ENV_TOKENS.get('ECOMMERCE_API_URL') -ECOMMERCE_API_SIGNING_KEY = AUTH_TOKENS.get('ECOMMERCE_API_SIGNING_KEY') -ECOMMERCE_API_TIMEOUT = ENV_TOKENS.get('ECOMMERCE_API_TIMEOUT', 5) +ECOMMERCE_API_URL = ENV_TOKENS.get('ECOMMERCE_API_URL', ECOMMERCE_API_URL) +ECOMMERCE_API_SIGNING_KEY = AUTH_TOKENS.get('ECOMMERCE_API_SIGNING_KEY', ECOMMERCE_API_SIGNING_KEY) +ECOMMERCE_API_TIMEOUT = ENV_TOKENS.get('ECOMMERCE_API_TIMEOUT', ECOMMERCE_API_TIMEOUT) From 71ab971c723c8762d5388464b3ecb6ae18050ecf Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Tue, 17 Mar 2015 10:44:48 -0400 Subject: [PATCH 09/14] Revert "Lms should be independent of mathjax" This reverts commit 4a086f54d51f37153f71a1d2079ef742060c978a. --- common/templates/mathjax_include.html | 57 ++++++++++++--------------- lms/static/require-config-lms.js | 1 - 2 files changed, 26 insertions(+), 32 deletions(-) diff --git a/common/templates/mathjax_include.html b/common/templates/mathjax_include.html index d3fef03ec2..bb0c14ece9 100644 --- a/common/templates/mathjax_include.html +++ b/common/templates/mathjax_include.html @@ -6,38 +6,33 @@ ## This enables ASCIIMathJAX, and is used by js_textbox -<%def name="mathjaxConfig()"> - %if mathjax_mode is not Undefined and mathjax_mode == 'wiki': - MathJax.Hub.Config({ - tex2jax: {inlineMath: [ ['$','$'], ["\\(","\\)"]], - displayMath: [ ['$$','$$'], ["\\[","\\]"]]} - }); - %else: - MathJax.Hub.Config({ - tex2jax: { - inlineMath: [ - ["\\(","\\)"], - ['[mathjaxinline]','[/mathjaxinline]'] - ], - displayMath: [ - ["\\[","\\]"], - ['[mathjax]','[/mathjax]'] - ] - } - }); - %endif - MathJax.Hub.Configured(); - window.HUB = MathJax.Hub; - +%if mathjax_mode is not Undefined and mathjax_mode == 'wiki': + +%else: + +%endif - + diff --git a/lms/static/require-config-lms.js b/lms/static/require-config-lms.js index 50b3404f24..3530432f09 100644 --- a/lms/static/require-config-lms.js +++ b/lms/static/require-config-lms.js @@ -67,7 +67,6 @@ "ova": 'js/vendor/ova/ova', "catch": 'js/vendor/ova/catch/js/catch', "handlebars": 'js/vendor/ova/catch/js/handlebars-1.1.2', - "mathjax": 'https://cdn.mathjax.org/mathjax/2.4-latest/MathJax.js?config=TeX-MML-AM_HTMLorMML-full' // end of files needed by OVA }, shim: { From 85c81e45794b59407bb730100f8022d48c1f6a31 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Tue, 17 Mar 2015 09:47:27 -0500 Subject: [PATCH 10/14] Remove use of to_deprecated_string in sandbox regex check. --- common/djangoapps/util/sandboxing.py | 2 +- common/djangoapps/util/tests/test_sandboxing.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/util/sandboxing.py b/common/djangoapps/util/sandboxing.py index 5fea7ed25c..8fcfa6dfbc 100644 --- a/common/djangoapps/util/sandboxing.py +++ b/common/djangoapps/util/sandboxing.py @@ -25,7 +25,7 @@ def can_execute_unsafe_code(course_id): # To others using this: the code as-is is brittle and likely to be changed in the future, # as per the TODO, so please consider carefully before adding more values to COURSES_WITH_UNSAFE_CODE for regex in getattr(settings, 'COURSES_WITH_UNSAFE_CODE', []): - if re.match(regex, course_id.to_deprecated_string()): + if re.match(regex, unicode(course_id)): return True return False diff --git a/common/djangoapps/util/tests/test_sandboxing.py b/common/djangoapps/util/tests/test_sandboxing.py index 7a33fbbe95..21179b1dfa 100644 --- a/common/djangoapps/util/tests/test_sandboxing.py +++ b/common/djangoapps/util/tests/test_sandboxing.py @@ -3,6 +3,7 @@ Tests for sandboxing.py in util app """ from django.test import TestCase +from opaque_keys.edx.locator import LibraryLocator from util.sandboxing import can_execute_unsafe_code from django.test.utils import override_settings from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -12,12 +13,13 @@ class SandboxingTest(TestCase): """ Test sandbox whitelisting """ - @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) + @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*', 'library:v1-edX+.*']) def test_sandbox_exclusion(self): """ Test to make sure that a non-match returns false """ self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'notful', 'empty'))) + self.assertFalse(can_execute_unsafe_code(LibraryLocator('edY', 'test_bank'))) @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) def test_sandbox_inclusion(self): @@ -26,10 +28,12 @@ class SandboxingTest(TestCase): """ self.assertTrue(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2012_Fall'))) self.assertTrue(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2013_Spring'))) + self.assertFalse(can_execute_unsafe_code(LibraryLocator('edX', 'test_bank'))) - def test_courses_with_unsafe_code_default(self): + def test_courselikes_with_unsafe_code_default(self): """ Test that the default setting for COURSES_WITH_UNSAFE_CODE is an empty setting, e.g. we don't use @override_settings in these tests """ self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2012_Fall'))) self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2013_Spring'))) + self.assertFalse(can_execute_unsafe_code(LibraryLocator('edX', 'test_bank'))) From 7f5c54dd31e3379f8965338cccb79570cc3788be Mon Sep 17 00:00:00 2001 From: brianhw Date: Tue, 17 Mar 2015 14:09:55 -0400 Subject: [PATCH 11/14] Revert "Remove use of to_deprecated_string in sandbox regex check." --- common/djangoapps/util/sandboxing.py | 2 +- common/djangoapps/util/tests/test_sandboxing.py | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/util/sandboxing.py b/common/djangoapps/util/sandboxing.py index 8fcfa6dfbc..5fea7ed25c 100644 --- a/common/djangoapps/util/sandboxing.py +++ b/common/djangoapps/util/sandboxing.py @@ -25,7 +25,7 @@ def can_execute_unsafe_code(course_id): # To others using this: the code as-is is brittle and likely to be changed in the future, # as per the TODO, so please consider carefully before adding more values to COURSES_WITH_UNSAFE_CODE for regex in getattr(settings, 'COURSES_WITH_UNSAFE_CODE', []): - if re.match(regex, unicode(course_id)): + if re.match(regex, course_id.to_deprecated_string()): return True return False diff --git a/common/djangoapps/util/tests/test_sandboxing.py b/common/djangoapps/util/tests/test_sandboxing.py index 21179b1dfa..7a33fbbe95 100644 --- a/common/djangoapps/util/tests/test_sandboxing.py +++ b/common/djangoapps/util/tests/test_sandboxing.py @@ -3,7 +3,6 @@ Tests for sandboxing.py in util app """ from django.test import TestCase -from opaque_keys.edx.locator import LibraryLocator from util.sandboxing import can_execute_unsafe_code from django.test.utils import override_settings from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -13,13 +12,12 @@ class SandboxingTest(TestCase): """ Test sandbox whitelisting """ - @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*', 'library:v1-edX+.*']) + @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) def test_sandbox_exclusion(self): """ Test to make sure that a non-match returns false """ self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'notful', 'empty'))) - self.assertFalse(can_execute_unsafe_code(LibraryLocator('edY', 'test_bank'))) @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) def test_sandbox_inclusion(self): @@ -28,12 +26,10 @@ class SandboxingTest(TestCase): """ self.assertTrue(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2012_Fall'))) self.assertTrue(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2013_Spring'))) - self.assertFalse(can_execute_unsafe_code(LibraryLocator('edX', 'test_bank'))) - def test_courselikes_with_unsafe_code_default(self): + def test_courses_with_unsafe_code_default(self): """ Test that the default setting for COURSES_WITH_UNSAFE_CODE is an empty setting, e.g. we don't use @override_settings in these tests """ self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2012_Fall'))) self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2013_Spring'))) - self.assertFalse(can_execute_unsafe_code(LibraryLocator('edX', 'test_bank'))) From 40bb70e9d18136117d2eb8891972c669e8c47e70 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Tue, 17 Mar 2015 09:47:27 -0500 Subject: [PATCH 12/14] Remove use of to_deprecated_string in sandbox regex check. --- common/djangoapps/util/sandboxing.py | 2 +- common/djangoapps/util/tests/test_sandboxing.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/util/sandboxing.py b/common/djangoapps/util/sandboxing.py index 5fea7ed25c..8fcfa6dfbc 100644 --- a/common/djangoapps/util/sandboxing.py +++ b/common/djangoapps/util/sandboxing.py @@ -25,7 +25,7 @@ def can_execute_unsafe_code(course_id): # To others using this: the code as-is is brittle and likely to be changed in the future, # as per the TODO, so please consider carefully before adding more values to COURSES_WITH_UNSAFE_CODE for regex in getattr(settings, 'COURSES_WITH_UNSAFE_CODE', []): - if re.match(regex, course_id.to_deprecated_string()): + if re.match(regex, unicode(course_id)): return True return False diff --git a/common/djangoapps/util/tests/test_sandboxing.py b/common/djangoapps/util/tests/test_sandboxing.py index 7a33fbbe95..21179b1dfa 100644 --- a/common/djangoapps/util/tests/test_sandboxing.py +++ b/common/djangoapps/util/tests/test_sandboxing.py @@ -3,6 +3,7 @@ Tests for sandboxing.py in util app """ from django.test import TestCase +from opaque_keys.edx.locator import LibraryLocator from util.sandboxing import can_execute_unsafe_code from django.test.utils import override_settings from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -12,12 +13,13 @@ class SandboxingTest(TestCase): """ Test sandbox whitelisting """ - @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) + @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*', 'library:v1-edX+.*']) def test_sandbox_exclusion(self): """ Test to make sure that a non-match returns false """ self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'notful', 'empty'))) + self.assertFalse(can_execute_unsafe_code(LibraryLocator('edY', 'test_bank'))) @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/full/.*']) def test_sandbox_inclusion(self): @@ -26,10 +28,12 @@ class SandboxingTest(TestCase): """ self.assertTrue(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2012_Fall'))) self.assertTrue(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2013_Spring'))) + self.assertFalse(can_execute_unsafe_code(LibraryLocator('edX', 'test_bank'))) - def test_courses_with_unsafe_code_default(self): + def test_courselikes_with_unsafe_code_default(self): """ Test that the default setting for COURSES_WITH_UNSAFE_CODE is an empty setting, e.g. we don't use @override_settings in these tests """ self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2012_Fall'))) self.assertFalse(can_execute_unsafe_code(SlashSeparatedCourseKey('edX', 'full', '2013_Spring'))) + self.assertFalse(can_execute_unsafe_code(LibraryLocator('edX', 'test_bank'))) From ae2480e038b688a41f0b80e656aea0b69b630a2f Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Tue, 17 Mar 2015 18:51:17 +0500 Subject: [PATCH 13/14] update instructor grade task state and increase logging --- lms/djangoapps/instructor/views/api.py | 7 +- lms/djangoapps/instructor_task/tasks.py | 15 +++- .../instructor_task/tasks_helper.py | 69 +++++++++++++++---- 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index ec3d2edf27..85950ac3cc 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1938,10 +1938,13 @@ def calculate_grades_csv(request, course_id): course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) try: instructor_task.api.submit_calculate_grades_csv(request, course_key) - success_status = _("Your grade report is being generated! You can view the status of the generation task in the 'Pending Instructor Tasks' section.") + success_status = _("Your grade report is being generated! " + "You can view the status of the generation task in the 'Pending Instructor Tasks' section.") return JsonResponse({"status": success_status}) except AlreadyRunningError: - already_running_status = _("A grade report generation task is already in progress. Check the 'Pending Instructor Tasks' table for the status of the task. When completed, the report will be available for download in the table below.") + already_running_status = _("A grade report generation task is already in progress. " + "Check the 'Pending Instructor Tasks' table for the status of the task. " + "When completed, the report will be available for download in the table below.") return JsonResponse({ "status": already_running_status }) diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index 76c8998090..9e3ae7d6ae 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -19,10 +19,14 @@ a problem URL and optionally a student. These are used to set up the initial va of the query for traversing StudentModule objects. """ +import logging +from functools import partial + from django.conf import settings from django.utils.translation import ugettext_noop + from celery import task -from functools import partial +from bulk_email.tasks import perform_delegate_email_batches from instructor_task.tasks_helper import ( run_main_task, BaseInstructorTask, @@ -34,7 +38,9 @@ from instructor_task.tasks_helper import ( upload_students_csv, cohort_students_and_upload ) -from bulk_email.tasks import perform_delegate_email_batches + + +TASK_LOG = logging.getLogger('edx.celery.task') @task(base=BaseInstructorTask) # pylint: disable=not-callable @@ -140,6 +146,11 @@ def calculate_grades_csv(entry_id, xmodule_instance_args): """ # Translators: This is a past-tense verb that is inserted into task progress messages as {action}. action_name = ugettext_noop('graded') + TASK_LOG.info( + u"Task: %s, InstructorTask ID: %s, Task type: %s, Preparing for task execution", + xmodule_instance_args.get('task_id'), entry_id, action_name + ) + task_fn = partial(upload_grades_csv, xmodule_instance_args) return run_main_task(entry_id, task_fn, action_name) diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index cf2ae7d142..de89f61330 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -226,39 +226,40 @@ def run_main_task(entry_id, task_fcn, action_name): """ - # get the InstructorTask to be updated. If this fails, then let the exception return to Celery. + # Get the InstructorTask to be updated. If this fails then let the exception return to Celery. # There's no point in catching it here. entry = InstructorTask.objects.get(pk=entry_id) + entry.task_state = PROGRESS + entry.save_now() - # get inputs to use in this task from the entry: + # Get inputs to use in this task from the entry task_id = entry.task_id course_id = entry.course_id task_input = json.loads(entry.task_input) - # construct log message: - fmt = u'task "{task_id}": course "{course_id}" input "{task_input}"' - task_info_string = fmt.format(task_id=task_id, course_id=course_id, task_input=task_input) - - TASK_LOG.info('Starting update (nothing %s yet): %s', action_name, task_info_string) + # Construct log message + fmt = u'Task: {task_id}, InstructorTask ID: {entry_id}, Course: {course_id}, Input: {task_input}' + task_info_string = fmt.format(task_id=task_id, entry_id=entry_id, course_id=course_id, task_input=task_input) + TASK_LOG.info(u'%s, Starting update (nothing %s yet)', task_info_string, action_name) # Check that the task_id submitted in the InstructorTask matches the current task # that is running. request_task_id = _get_current_task().request.id if task_id != request_task_id: - fmt = u'Requested task did not match actual task "{actual_id}": {task_info}' - message = fmt.format(actual_id=request_task_id, task_info=task_info_string) + fmt = u'{task_info}, Requested task did not match actual task "{actual_id}"' + message = fmt.format(task_info=task_info_string, actual_id=request_task_id) TASK_LOG.error(message) raise ValueError(message) - # Now do the work: + # Now do the work with dog_stats_api.timer('instructor_tasks.time.overall', tags=[u'action:{name}'.format(name=action_name)]): task_progress = task_fcn(entry_id, course_id, task_input, action_name) - # Release any queries that the connection has been hanging onto: + # Release any queries that the connection has been hanging onto reset_queries() - # log and exit, returning task_progress info as task result: - TASK_LOG.info('Finishing %s: final: %s', task_info_string, task_progress) + # Log and exit, returning task_progress info as task result + TASK_LOG.info(u'%s, Task type: %s, Finishing task: %s', task_info_string, action_name, task_progress) return task_progress @@ -567,6 +568,15 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, enrolled_students = CourseEnrollment.users_enrolled_in(course_id) task_progress = TaskProgress(action_name, enrolled_students.count(), start_time) + fmt = u'Task: {task_id}, InstructorTask ID: {entry_id}, Course: {course_id}, Input: {task_input}' + task_info_string = fmt.format( + task_id=_xmodule_instance_args.get('task_id') if _xmodule_instance_args is not None else None, + entry_id=_entry_id, + course_id=course_id, + task_input=_task_input + ) + TASK_LOG.info(u'%s, Task type: %s, Starting task execution', task_info_string, action_name) + course = get_course_by_id(course_id) cohorts_header = ['Cohort Name'] if course.is_cohorted else [] @@ -578,12 +588,34 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, rows = [] err_rows = [["id", "username", "error_msg"]] current_step = {'step': 'Calculating Grades'} + + total_enrolled_students = enrolled_students.count() + student_counter = 0 + TASK_LOG.info( + u'%s, Task type: %s, Current step: %s, Starting grade calculation for total students: %s', + task_info_string, + action_name, + current_step, + total_enrolled_students + ) for student, gradeset, err_msg in iterate_grades_for(course_id, enrolled_students): # Periodically update task status (this is a cache write) if task_progress.attempted % status_interval == 0: task_progress.update_task_state(extra_meta=current_step) task_progress.attempted += 1 + # Now add a log entry after certain intervals to get a hint that task is in progress + student_counter += 1 + if student_counter % 1000 == 0: + TASK_LOG.info( + u'%s, Task type: %s, Current step: %s, Grade calculation in-progress for students: %s/%s', + task_info_string, + action_name, + current_step, + student_counter, + total_enrolled_students + ) + if gradeset: # We were able to successfully grade this student for this course. task_progress.succeeded += 1 @@ -625,9 +657,19 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, task_progress.failed += 1 err_rows.append([student.id, student.username, err_msg]) + TASK_LOG.info( + u'%s, Task type: %s, Current step: %s, Grade calculation completed for students: %s/%s', + task_info_string, + action_name, + current_step, + student_counter, + total_enrolled_students + ) + # By this point, we've got the rows we're going to stuff into our CSV files. current_step = {'step': 'Uploading CSVs'} task_progress.update_task_state(extra_meta=current_step) + TASK_LOG.info(u'%s, Task type: %s, Current step: %s', task_info_string, action_name, current_step) # Perform the actual upload upload_csv_to_report_store(rows, 'grade_report', course_id, start_date) @@ -637,6 +679,7 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, upload_csv_to_report_store(err_rows, 'grade_report_err', course_id, start_date) # One last update before we close out... + TASK_LOG.info(u'%s, Task type: %s, Finalizing grade task', task_info_string, action_name) return task_progress.update_task_state(extra_meta=current_step) From 9b79797b28213823ae9d2c9ab022c6456122a620 Mon Sep 17 00:00:00 2001 From: alawibaba Date: Wed, 11 Mar 2015 16:37:31 -0400 Subject: [PATCH 14/14] Created performance logging endpoint, changed CDN experiment to point to it. --- common/djangoapps/performance/__init__.py | 0 .../djangoapps/performance/tests/__init__.py | 0 .../djangoapps/performance/tests/test_logs.py | 133 ++++++++++++++++++ .../djangoapps/performance/views/__init__.py | 52 +++++++ .../xmodule/video_module/video_module.py | 6 +- .../courseware/tests/test_video_mongo.py | 7 - lms/envs/aws.py | 1 - lms/envs/common.py | 3 +- lms/templates/video.html | 12 +- lms/urls.py | 1 + 10 files changed, 194 insertions(+), 21 deletions(-) create mode 100644 common/djangoapps/performance/__init__.py create mode 100644 common/djangoapps/performance/tests/__init__.py create mode 100644 common/djangoapps/performance/tests/test_logs.py create mode 100644 common/djangoapps/performance/views/__init__.py diff --git a/common/djangoapps/performance/__init__.py b/common/djangoapps/performance/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/performance/tests/__init__.py b/common/djangoapps/performance/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/performance/tests/test_logs.py b/common/djangoapps/performance/tests/test_logs.py new file mode 100644 index 0000000000..04c795e724 --- /dev/null +++ b/common/djangoapps/performance/tests/test_logs.py @@ -0,0 +1,133 @@ +"""Tests that performance data is successfully logged.""" +import datetime +import dateutil +import json +import mock +import unittest + +import logging +from StringIO import StringIO + +from django.test import TestCase +from django.test.client import RequestFactory +from performance.views import performance_log + + +class PerformanceTrackingTest(TestCase): + """ + Tests that performance logs correctly handle events + """ + + def setUp(self): + self.request_factory = RequestFactory() + self.stream = StringIO() + self.handler = logging.StreamHandler(self.stream) + self.log = logging.getLogger() + self.log.setLevel(logging.INFO) + for handler in self.log.handlers: + self.log.removeHandler(handler) + self.log.addHandler(self.handler) + self.addCleanup(self.log.removeHandler, self.handler) + self.addCleanup(self.handler.close) + + def test_empty_get(self): + request = self.request_factory.get('/performance') + pre_time = datetime.datetime.utcnow() + performance_log(request) + post_time = datetime.datetime.utcnow() + self.handler.flush() + logged_value = json.loads(self.stream.getvalue().strip()) + self.assertEqual(logged_value['accept_language'], '') + self.assertEqual(logged_value['agent'], '') + self.assertEqual(logged_value['event'], '') + self.assertEqual(logged_value['event_source'], 'browser') + self.assertEqual(logged_value['expgroup'], '') + self.assertEqual(logged_value['id'], '') + self.assertEqual(logged_value['page'], '') + self.assertEqual(logged_value['referer'], '') + self.assertEqual(logged_value['value'], '') + logged_time = dateutil.parser.parse(logged_value['time']).replace(tzinfo=None) + self.assertTrue(pre_time <= logged_time) + self.assertTrue(post_time >= logged_time) + + def test_empty_post(self): + request = self.request_factory.post('/performance') + pre_time = datetime.datetime.utcnow() + performance_log(request) + post_time = datetime.datetime.utcnow() + self.handler.flush() + logged_value = json.loads(self.stream.getvalue().strip()) + self.assertEqual(logged_value['accept_language'], '') + self.assertEqual(logged_value['agent'], '') + self.assertEqual(logged_value['event'], '') + self.assertEqual(logged_value['event_source'], 'browser') + self.assertEqual(logged_value['expgroup'], '') + self.assertEqual(logged_value['id'], '') + self.assertEqual(logged_value['page'], '') + self.assertEqual(logged_value['referer'], '') + self.assertEqual(logged_value['value'], '') + logged_time = dateutil.parser.parse(logged_value['time']).replace(tzinfo=None) + self.assertTrue(pre_time <= logged_time) + self.assertTrue(post_time >= logged_time) + + def test_populated_get(self): + request = self.request_factory.get('/performance', + {'event': "a_great_event", + 'id': "12345012345", + 'expgroup': "17", 'page': "atestpage", + 'value': "100234"}) + request.META['HTTP_ACCEPT_LANGUAGE'] = "en" + request.META['HTTP_REFERER'] = "https://www.edx.org/evilpage" + request.META['HTTP_USER_AGENT'] = "Mozilla/5.0" + request.META['REMOTE_ADDR'] = "18.19.20.21" + request.META['SERVER_NAME'] = "some-aws-server" + pre_time = datetime.datetime.utcnow() + performance_log(request) + post_time = datetime.datetime.utcnow() + self.handler.flush() + logged_value = json.loads(self.stream.getvalue().strip()) + self.assertEqual(logged_value['accept_language'], 'en') + self.assertEqual(logged_value['agent'], 'Mozilla/5.0') + self.assertEqual(logged_value['event'], 'a_great_event') + self.assertEqual(logged_value['event_source'], 'browser') + self.assertEqual(logged_value['expgroup'], '17') + self.assertEqual(logged_value['host'], 'some-aws-server') + self.assertEqual(logged_value['id'], '12345012345') + self.assertEqual(logged_value['ip'], '18.19.20.21') + self.assertEqual(logged_value['page'], 'atestpage') + self.assertEqual(logged_value['referer'], 'https://www.edx.org/evilpage') + self.assertEqual(logged_value['value'], '100234') + logged_time = dateutil.parser.parse(logged_value['time']).replace(tzinfo=None) + self.assertTrue(pre_time <= logged_time) + self.assertTrue(post_time >= logged_time) + + def test_populated_post(self): + request = self.request_factory.post('/performance', + {'event': "a_great_event", + 'id': "12345012345", + 'expgroup': "17", 'page': "atestpage", + 'value': "100234"}) + request.META['HTTP_ACCEPT_LANGUAGE'] = "en" + request.META['HTTP_REFERER'] = "https://www.edx.org/evilpage" + request.META['HTTP_USER_AGENT'] = "Mozilla/5.0" + request.META['REMOTE_ADDR'] = "18.19.20.21" + request.META['SERVER_NAME'] = "some-aws-server" + pre_time = datetime.datetime.utcnow() + performance_log(request) + post_time = datetime.datetime.utcnow() + self.handler.flush() + logged_value = json.loads(self.stream.getvalue().strip()) + self.assertEqual(logged_value['accept_language'], 'en') + self.assertEqual(logged_value['agent'], 'Mozilla/5.0') + self.assertEqual(logged_value['event'], 'a_great_event') + self.assertEqual(logged_value['event_source'], 'browser') + self.assertEqual(logged_value['expgroup'], '17') + self.assertEqual(logged_value['host'], 'some-aws-server') + self.assertEqual(logged_value['id'], '12345012345') + self.assertEqual(logged_value['ip'], '18.19.20.21') + self.assertEqual(logged_value['page'], 'atestpage') + self.assertEqual(logged_value['referer'], 'https://www.edx.org/evilpage') + self.assertEqual(logged_value['value'], '100234') + logged_time = dateutil.parser.parse(logged_value['time']).replace(tzinfo=None) + self.assertTrue(pre_time <= logged_time) + self.assertTrue(post_time >= logged_time) diff --git a/common/djangoapps/performance/views/__init__.py b/common/djangoapps/performance/views/__init__.py new file mode 100644 index 0000000000..805e2d96cb --- /dev/null +++ b/common/djangoapps/performance/views/__init__.py @@ -0,0 +1,52 @@ +import datetime +import json +import logging + +from django.http import HttpResponse + +from track.utils import DateTimeJSONEncoder + + +perflog = logging.getLogger("perflog") + + +def _get_request_header(request, header_name, default=''): + """Helper method to get header values from a request's META dict, if present.""" + if request is not None and hasattr(request, 'META') and header_name in request.META: + return request.META[header_name] + else: + return default + + +def _get_request_value(request, value_name, default=''): + """Helper method to get header values from a request's REQUEST dict, if present.""" + if request is not None and hasattr(request, 'REQUEST') and value_name in request.REQUEST: + return request.REQUEST[value_name] + else: + return default + + +def performance_log(request): + """ + Log when POST call to "performance" URL is made by a user. + Request should provide "event" and "page" arguments. + """ + + event = { + "ip": _get_request_header(request, 'REMOTE_ADDR'), + "referer": _get_request_header(request, 'HTTP_REFERER'), + "accept_language": _get_request_header(request, 'HTTP_ACCEPT_LANGUAGE'), + "event_source": "browser", + "event": _get_request_value(request, 'event'), + "agent": _get_request_header(request, 'HTTP_USER_AGENT'), + "page": _get_request_value(request, 'page'), + "id": _get_request_value(request, 'id'), + "expgroup": _get_request_value(request, 'expgroup'), + "value": _get_request_value(request, 'value'), + "time": datetime.datetime.utcnow(), + "host": _get_request_header(request, 'SERVER_NAME'), + } + + perflog.info(json.dumps(event, cls=DateTimeJSONEncoder)) + + return HttpResponse(status=204) diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 46272814a6..85aee1fc65 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -236,9 +236,8 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, # CDN_VIDEO_URLS is only to be used here and will be deleted # TODO(ali@edx.org): Delete this after the CDN experiment has completed. html_id = self.location.html_id() - if getattr(settings, 'PERFORMANCE_GRAPHITE_URL', '') != '' and \ - self.system.user_location == 'CN' and \ - getattr(settings.FEATURES, 'ENABLE_VIDEO_BEACON', False) and \ + if self.system.user_location == 'CN' and \ + settings.FEATURES.get('ENABLE_VIDEO_BEACON', False) and \ html_id in getattr(settings, 'CDN_VIDEO_URLS', {}).keys(): cdn_urls = getattr(settings, 'CDN_VIDEO_URLS', {})[html_id] cdn_exp_group, new_source = random.choice(zip(range(len(cdn_urls)), cdn_urls)) @@ -254,7 +253,6 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, 'autoplay': settings.FEATURES.get('AUTOPLAY_VIDEOS', False), 'branding_info': branding_info, 'cdn_eval': cdn_eval, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': cdn_exp_group, # This won't work when we move to data that # isn't on the filesystem diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index d1a59ef78a..8734ef24e2 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -30,7 +30,6 @@ class TestVideoYouTube(TestVideo): 'autoplay': settings.FEATURES.get('AUTOPLAY_VIDEOS', False), 'branding_info': None, 'cdn_eval': False, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': None, 'data_dir': getattr(self, 'data_dir', None), 'display_name': u'A Name', @@ -97,7 +96,6 @@ class TestVideoNonYouTube(TestVideo): 'ajax_url': self.item_descriptor.xmodule_runtime.ajax_url + '/save_user_state', 'branding_info': None, 'cdn_eval': False, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': None, 'data_dir': getattr(self, 'data_dir', None), 'show_captions': 'true', @@ -204,7 +202,6 @@ class TestGetHtmlMethod(BaseTestXmodule): expected_context = { 'branding_info': None, 'cdn_eval': False, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': None, 'data_dir': getattr(self, 'data_dir', None), 'show_captions': 'true', @@ -324,7 +321,6 @@ class TestGetHtmlMethod(BaseTestXmodule): initial_context = { 'branding_info': None, 'cdn_eval': False, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': None, 'data_dir': getattr(self, 'data_dir', None), 'show_captions': 'true', @@ -467,7 +463,6 @@ class TestGetHtmlMethod(BaseTestXmodule): initial_context = { 'branding_info': None, 'cdn_eval': False, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': None, 'data_dir': getattr(self, 'data_dir', None), 'show_captions': 'true', @@ -588,7 +583,6 @@ class TestGetHtmlMethod(BaseTestXmodule): initial_context = { 'branding_info': None, 'cdn_eval': False, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': None, 'data_dir': getattr(self, 'data_dir', None), 'show_captions': 'true', @@ -710,7 +704,6 @@ class TestGetHtmlMethod(BaseTestXmodule): 'url': 'http://www.xuetangx.com' }, 'cdn_eval': False, - 'cdn_eval_endpoint': getattr(settings, 'PERFORMANCE_GRAPHITE_URL', ''), 'cdn_exp_group': None, 'data_dir': getattr(self, 'data_dir', None), 'show_captions': 'true', diff --git a/lms/envs/aws.py b/lms/envs/aws.py index b0a2580ea0..38bb7b7e7c 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -556,7 +556,6 @@ FACEBOOK_APP_ID = AUTH_TOKENS.get("FACEBOOK_APP_ID") XBLOCK_SETTINGS = ENV_TOKENS.get('XBLOCK_SETTINGS', {}) ##### CDN EXPERIMENT/MONITORING FLAGS ##### -PERFORMANCE_GRAPHITE_URL = ENV_TOKENS.get('PERFORMANCE_GRAPHITE_URL', PERFORMANCE_GRAPHITE_URL) CDN_VIDEO_URLS = ENV_TOKENS.get('CDN_VIDEO_URLS', CDN_VIDEO_URLS) ##### ECOMMERCE API CONFIGURATION SETTINGS ##### diff --git a/lms/envs/common.py b/lms/envs/common.py index 14d998b060..80b97bff69 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -558,7 +558,7 @@ TRACKING_BACKENDS = { # We're already logging events, and we don't want to capture user # names/passwords. Heartbeat events are likely not interesting. -TRACKING_IGNORE_URL_PATTERNS = [r'^/event', r'^/login', r'^/heartbeat', r'^/segmentio/event'] +TRACKING_IGNORE_URL_PATTERNS = [r'^/event', r'^/login', r'^/heartbeat', r'^/segmentio/event', r'^/performance'] EVENT_TRACKING_ENABLED = True EVENT_TRACKING_BACKENDS = { @@ -2075,7 +2075,6 @@ SEARCH_ENGINE = None SEARCH_RESULT_PROCESSOR = "lms.lib.courseware_search.lms_result_processor.LmsSearchResultProcessor" ##### CDN EXPERIMENT/MONITORING FLAGS ##### -PERFORMANCE_GRAPHITE_URL = '' CDN_VIDEO_URLS = {} # The configuration visibility of account fields. diff --git a/lms/templates/video.html b/lms/templates/video.html index 1ad97ee6f0..64534e4aa6 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -154,19 +154,17 @@ % if cdn_eval: