diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 328c65ea5cb5d1b5b61a43aafe0d8ba964246b6b..b204c59551c2af9e6bee7d5ffe2b70d045756993 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -215,6 +215,11 @@ class FieldDataCache(object): returns the found object, or None if the object doesn't exist ''' + if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): + # If we're getting user data, we expect that the key matches the + # user we were constructed for. + assert key.user_id == self.user.id + return self.cache.get(self._cache_key_from_kvs_key(key)) def find_or_create(self, key): @@ -227,11 +232,6 @@ class FieldDataCache(object): if field_object is not None: return field_object - if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): - # If we're getting user data, we expect that the key matches the - # user we were constructed for. - assert key.user_id == self.user.id - if key.scope == Scope.user_state: field_object, _ = StudentModule.objects.get_or_create( course_id=self.course_id, diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 7b0eecf2c3dd49d0451f92495dee2edbdb5badd0..148dcc0e726b5599abf440907023f290657dcf97 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -74,7 +74,34 @@ class TestInvalidScopes(TestCase): self.assertRaises(InvalidScopeError, self.kvs.set_many, {key: 'value'}) -class TestStudentModuleStorage(TestCase): +class OtherUserFailureTestMixin(object): + """ + Mixin class to add test cases for failures when a user trying to use the kvs is not + the one that instantiated the kvs. + Doing a mixin rather than modifying StorageTestBase (below) because some scopes don't fail in this case, because + they aren't bound to a particular user + + assumes that this is mixed into a class that defines other_key_factory and existing_field_name + """ + def test_other_user_kvs_get_failure(self): + """ + Test for assert failure when a user who didn't create the kvs tries to get from it it + """ + with self.assertRaises(AssertionError): + self.kvs.get(self.other_key_factory(self.existing_field_name)) + + def test_other_user_kvs_set_failure(self): + """ + Test for assert failure when a user who didn't create the kvs tries to get from it it + """ + with self.assertRaises(AssertionError): + self.kvs.set(self.other_key_factory(self.existing_field_name), "new_value") + + +class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): + """Tests for user_state storage via StudentModule""" + other_key_factory = partial(DjangoKeyValueStore.Key, Scope.user_state, 2, location('usage_id')) # user_id=2, not 1 + existing_field_name = "a_field" def setUp(self): student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value', 'b_field': 'b_value'})) @@ -291,21 +318,28 @@ class StorageTestBase(object): class TestContentStorage(StorageTestBase, TestCase): + """Tests for ContentStorage""" factory = UserStateSummaryFactory scope = Scope.user_state_summary key_factory = user_state_summary_key storage_class = XModuleUserStateSummaryField -class TestStudentPrefsStorage(StorageTestBase, TestCase): +class TestStudentPrefsStorage(OtherUserFailureTestMixin, StorageTestBase, TestCase): + """Tests for StudentPrefStorage""" factory = StudentPrefsFactory scope = Scope.preferences key_factory = prefs_key storage_class = XModuleStudentPrefsField + other_key_factory = partial(DjangoKeyValueStore.Key, Scope.preferences, 2, 'mock_problem') # user_id=2, not 1 + existing_field_name = "existing_field" -class TestStudentInfoStorage(StorageTestBase, TestCase): +class TestStudentInfoStorage(OtherUserFailureTestMixin, StorageTestBase, TestCase): + """Tests for StudentInfoStorage""" factory = StudentInfoFactory scope = Scope.user_info key_factory = user_info_key storage_class = XModuleStudentInfoField + other_key_factory = partial(DjangoKeyValueStore.Key, Scope.user_info, 2, 'mock_problem') # user_id=2, not 1 + existing_field_name = "existing_field"