From ab3c9e924aa1a9eac5e5bd9742c9d4883b616d7e Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Fri, 29 May 2015 15:03:40 -0400 Subject: [PATCH 1/4] update to course import (TNL-2270) --- .../commands/export_convert_format.py | 5 +-- .../tests/test_export_convert_format.py | 7 ++-- .../views/tests/test_import_export.py | 35 ++++++++++++++++--- cms/envs/test.py | 1 + openedx/core/lib/extract_tar.py | 17 +++++---- 5 files changed, 49 insertions(+), 16 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/export_convert_format.py b/cms/djangoapps/contentstore/management/commands/export_convert_format.py index 5b1b1d7cfdb2..f97ff305fc76 100644 --- a/cms/djangoapps/contentstore/management/commands/export_convert_format.py +++ b/cms/djangoapps/contentstore/management/commands/export_convert_format.py @@ -7,6 +7,7 @@ import os from path import path from django.core.management.base import BaseCommand, CommandError +from django.conf import settings from tempfile import mkdtemp import tarfile @@ -32,8 +33,8 @@ def handle(self, *args, **options): output_path = args[1] # Create temp directories to extract the source and create the target archive. - temp_source_dir = mkdtemp() - temp_target_dir = mkdtemp() + temp_source_dir = mkdtemp(dir=settings.DATA_DIR) + temp_target_dir = mkdtemp(dir=settings.DATA_DIR) try: extract_source(source_archive, temp_source_dir) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py b/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py index fd83d58f890b..ddcdb725fbed 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_export_convert_format.py @@ -3,6 +3,7 @@ """ from unittest import TestCase from django.core.management import call_command, CommandError +from django.conf import settings from tempfile import mkdtemp import shutil from path import path @@ -18,7 +19,7 @@ def setUp(self): """ Common setup. """ super(ConvertExportFormat, self).setUp() - self.temp_dir = mkdtemp() + self.temp_dir = mkdtemp(dir=settings.DATA_DIR) self.addCleanup(shutil.rmtree, self.temp_dir) self.data_dir = path(__file__).realpath().parent / 'data' self.version0 = self.data_dir / "Version0_drafts.tar.gz" @@ -52,8 +53,8 @@ def _verify_archive_equality(self, file1, file2): """ Helper function for determining if 2 archives are equal. """ - temp_dir_1 = mkdtemp() - temp_dir_2 = mkdtemp() + temp_dir_1 = mkdtemp(dir=settings.DATA_DIR) + temp_dir_2 = mkdtemp(dir=settings.DATA_DIR) try: extract_source(file1, temp_dir_1) extract_source(file2, temp_dir_2) diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 3375a30d0916..f251d0a29597 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -209,6 +209,19 @@ def _outside_tar2(self): return outside_tar + def _edx_platform_tar(self): + """ + Tarfile with file that extracts to edx-platform directory. + + Extracting this tarfile in directory will also put its contents + directly in (rather than ). + """ + outside_tar = self.unsafe_common_dir / "unsafe_file.tar.gz" + with tarfile.open(outside_tar, "w:gz") as tar: + tar.addfile(tarfile.TarInfo(os.path.join(os.path.abspath("."), "a_file"))) + + return outside_tar + def test_unsafe_tar(self): """ Check that safety measure work. @@ -233,6 +246,12 @@ def try_tar(tarpath): try_tar(self._symlink_tar()) try_tar(self._outside_tar()) try_tar(self._outside_tar2()) + try_tar(self._edx_platform_tar()) + + # test trying to open a tar outside of the normal data directory + with self.settings(DATA_DIR='/not/the/data/dir'): + try_tar(self._edx_platform_tar()) + # Check that `import_status` returns the appropriate stage (i.e., # either 3, indicating all previous steps are completed, or 0, # indicating no upload in progress) @@ -294,13 +313,19 @@ def test_library_import(self): self.assertIn(test_block3.url_name, children) self.assertIn(test_block4.url_name, children) - extract_dir = path(tempfile.mkdtemp()) + extract_dir = path(tempfile.mkdtemp(dir=settings.DATA_DIR)) + # the extract_dir needs to be passed as a relative dir to + # import_library_from_xml + extract_dir_relative = path.relpath(extract_dir, settings.DATA_DIR) + try: - tar = tarfile.open(path(TEST_DATA_DIR) / 'imports' / 'library.HhJfPD.tar.gz') - safetar_extractall(tar, extract_dir) + with tarfile.open(path(TEST_DATA_DIR) / 'imports' / 'library.HhJfPD.tar.gz') as tar: + safetar_extractall(tar, extract_dir) library_items = import_library_from_xml( - self.store, self.user.id, - settings.GITHUB_REPO_ROOT, [extract_dir / 'library'], + self.store, + self.user.id, + settings.GITHUB_REPO_ROOT, + [extract_dir_relative / 'library'], load_error_modules=False, static_content_store=contentstore(), target_id=lib_key diff --git a/cms/envs/test.py b/cms/envs/test.py index ba6d9e7df120..289c3c67d378 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -65,6 +65,7 @@ STATIC_ROOT = TEST_ROOT / "staticfiles" GITHUB_REPO_ROOT = TEST_ROOT / "data" +DATA_DIR = TEST_ROOT / "data" COMMON_TEST_DATA_ROOT = COMMON_ROOT / "test" / "data" # For testing "push to lms" diff --git a/openedx/core/lib/extract_tar.py b/openedx/core/lib/extract_tar.py index ea464880ea18..1fd664497706 100644 --- a/openedx/core/lib/extract_tar.py +++ b/openedx/core/lib/extract_tar.py @@ -7,6 +7,7 @@ """ from os.path import abspath, realpath, dirname, join as joinpath from django.core.exceptions import SuspiciousOperation +from django.conf import settings import logging log = logging.getLogger(__name__) @@ -28,19 +29,23 @@ def _is_bad_path(path, base): def _is_bad_link(info, base): """ - Does the file sym- ord hard-link to files outside `base`? + Does the file sym- or hard-link to files outside `base`? """ # Links are interpreted relative to the directory containing the link tip = resolved(joinpath(base, dirname(info.name))) return _is_bad_path(info.linkname, base=tip) -def safemembers(members): +def safemembers(members, base): """ Check that all elements of a tar file are safe. """ - base = resolved(".") + base = resolved(base) + + # check that we're not trying to import outside of the data_dir + if not base.startswith(resolved(settings.DATA_DIR)): + raise SuspiciousOperation("Attempted to import course outside of data dir") for finfo in members: if _is_bad_path(finfo.name, base): @@ -61,8 +66,8 @@ def safemembers(members): return members -def safetar_extractall(tarf, *args, **kwargs): +def safetar_extractall(tar_file, path=".", members=None): """ - Safe version of `tarf.extractall()`. + Safe version of `tar_file.extractall()`. """ - return tarf.extractall(members=safemembers(tarf), *args, **kwargs) + return tar_file.extractall(path, safemembers(tar_file, path)) From 4c4afb4eaeaceb64277bc746ef73176adeb5ac25 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Wed, 8 Jul 2015 11:48:57 -0400 Subject: [PATCH 2/4] when removing field from _field_data_cache when rebinding a module to a new user, also remove it from _dirty_fields (TNL-2640) --- common/lib/xmodule/xmodule/x_module.py | 6 +++++ .../courseware/tests/test_module_render.py | 25 +++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 028065596597..4943b2610e4a 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -587,6 +587,12 @@ def bind_for_student(self, xmodule_runtime, user_id, wrappers=None): if field.scope.user == UserScope.ONE: field._del_cached_value(self) # pylint: disable=protected-access + # not the most elegant way of doing this, but if we're removing + # a field from the module's field_data_cache, we should also + # remove it from its _dirty_fields + # pylint: disable=protected-access + if field in self._dirty_fields: + del self._dirty_fields[field] # Set the new xmodule_runtime and field_data (which are user-specific) self.xmodule_runtime = xmodule_runtime diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index ba9331fa6b0d..796a7a846f65 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1357,23 +1357,44 @@ def setUp(self): super(TestRebindModule, self).setUp() self.homework = self.add_graded_section_to_course('homework') self.lti = ItemFactory.create(category='lti', parent=self.homework) + self.problem = ItemFactory.create(category='problem', parent=self.homework) self.user = UserFactory.create() self.anon_user = AnonymousUser() - def get_module_for_user(self, user): + def get_module_for_user(self, user, item=None): """Helper function to get useful module at self.location in self.course_id for user""" mock_request = MagicMock() mock_request.user = user field_data_cache = FieldDataCache.cache_for_descriptor_descendents( self.course.id, user, self.course, depth=2) + if item is None: + item = self.lti + return render.get_module( # pylint: disable=protected-access user, mock_request, - self.lti.location, + item.location, field_data_cache, )._xmodule + def test_rebind_module_to_new_users(self): + module = self.get_module_for_user(self.user, self.problem) + + # Bind the module to another student, which will remove "correct_map" + # from the module's _field_data_cache and _dirty_fields. + user2 = UserFactory.create() + module.descriptor.bind_for_student(module.system, user2.id) + + # XBlock's save method assumes that if a field is in _dirty_fields, + # then it's also in _field_data_cache. If this assumption + # doesn't hold, then we get an error trying to bind this module + # to a third student, since we've removed "correct_map" from + # _field_data cache, but not _dirty_fields, when we bound + # this module to the second student. (TNL-2640) + user3 = UserFactory.create() + module.descriptor.bind_for_student(module.system, user3.id) + def test_rebind_noauth_module_to_user_not_anonymous(self): """ Tests that an exception is thrown when rebind_noauth_module_to_user is run from a From e57f69f54ae10d4d94e4e7da2da08dbe1e35fe9a Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Mon, 13 Jul 2015 15:09:53 -0400 Subject: [PATCH 3/4] address quality violation --- openedx/core/lib/extract_tar.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/lib/extract_tar.py b/openedx/core/lib/extract_tar.py index 1fd664497706..58079ad065a2 100644 --- a/openedx/core/lib/extract_tar.py +++ b/openedx/core/lib/extract_tar.py @@ -66,7 +66,7 @@ def safemembers(members, base): return members -def safetar_extractall(tar_file, path=".", members=None): +def safetar_extractall(tar_file, path=".", members=None): # pylint: disable=unused-argument """ Safe version of `tar_file.extractall()`. """ From 3969607c7fc4c1ccd4d159fcdfcfe2bfe65264ce Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Mon, 13 Jul 2015 17:10:32 -0400 Subject: [PATCH 4/4] add data_dir to bok_choy test settings --- cms/envs/bok_choy.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/envs/bok_choy.py b/cms/envs/bok_choy.py index 8c7e7f8585ee..84916debe96b 100644 --- a/cms/envs/bok_choy.py +++ b/cms/envs/bok_choy.py @@ -39,6 +39,7 @@ TEST_ROOT = REPO_ROOT / "test_root" # pylint: disable=no-value-for-parameter GITHUB_REPO_ROOT = (TEST_ROOT / "data").abspath() LOG_DIR = (TEST_ROOT / "log").abspath() +DATA_DIR = TEST_ROOT / "data" # Configure modulestore to use the test folder within the repo update_module_store_settings(