From 7bf81195ad414204a3cb16b33cdca7424e1961a1 Mon Sep 17 00:00:00 2001 From: Awais Jibran Date: Fri, 10 Jul 2015 12:16:00 +0500 Subject: [PATCH] Strip aways slashes from the asset 'displayname' as they cause export to fail. TNL-2669 --- .../contentstore/tests/test_contentstore.py | 134 ++++++++++++++++++ .../lib/xmodule/xmodule/contentstore/mongo.py | 16 ++- .../xmodule/modulestore/xml_importer.py | 8 +- common/lib/xmodule/xmodule/util/misc.py | 26 ++++ .../import_draft_order/policies/assets.json | 19 ++- .../static/subs-esLhHcdKGWvKs.srt | 3 + 6 files changed, 200 insertions(+), 6 deletions(-) create mode 100644 common/lib/xmodule/xmodule/util/misc.py create mode 100644 common/test/data/import_draft_order/static/subs-esLhHcdKGWvKs.srt diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index cda3896ab2..768566b053 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -114,6 +114,69 @@ class ImportRequiredTestCases(ContentStoreTestCase): err_cnt = perform_xlint(TEST_DATA_DIR, ['toy']) self.assertGreater(err_cnt, 0) + def test_invalid_asset_overwrite(self): + """ + Tests that an asset with invalid displayname can be overwritten if multiple assets have same displayname. + It Verifies that: + During import, if ('/') or ('\') is present in displayname of an asset, it is replaced with underscores '_'. + Export does not fail when an asset has '/' in its displayname. If the converted display matches with + any other asset, then it will be replaced. + + Asset name in XML: "/invalid\\displayname/subs-esLhHcdKGWvKs.srt" + """ + content_store = contentstore() + expected_displayname = '_invalid_displayname_subs-esLhHcdKGWvKs.srt' + + import_course_from_xml( + self.store, + self.user.id, + TEST_DATA_DIR, + ['import_draft_order'], + static_content_store=content_store, + verbose=True, + create_if_not_present=True + ) + + # Verify the course has imported successfully + course = self.store.get_course(self.store.make_course_key( + 'test_org', + 'import_draft_order', + 'import_draft_order' + )) + self.assertIsNotNone(course) + + # Add a new asset in the course, and make sure to name it such that it overwrite the one existing + # asset in the course. (i.e. _invalid_displayname_subs-esLhHcdKGWvKs.srt) + asset_key = course.id.make_asset_key('asset', 'sample_asset.srt') + content = StaticContent( + asset_key, expected_displayname, 'application/text', 'test', + ) + content_store.save(content) + + # Get & verify that course actually has two assets + assets, count = content_store.get_all_content_for_course(course.id) + self.assertEqual(count, 2) + + # Verify both assets have similar `displayname` after saving. + for asset in assets: + self.assertEquals(asset['displayname'], expected_displayname) + + # Test course export does not fail + root_dir = path(mkdtemp_clean()) + print 'Exporting to tempdir = {0}'.format(root_dir) + export_course_to_xml(self.store, content_store, course.id, root_dir, 'test_export') + + filesystem = OSFS(root_dir / 'test_export/static') + exported_static_files = filesystem.listdir() + + # Verify that asset have been overwritten during export. + self.assertEqual(len(exported_static_files), 1) + self.assertTrue(filesystem.exists(expected_displayname)) + self.assertEqual(exported_static_files[0], expected_displayname) + + # Remove exported course + shutil.rmtree(root_dir) + def test_about_overrides(self): ''' This test case verifies that a course can use specialized override for about data, @@ -545,6 +608,7 @@ class ImportRequiredTestCases(ContentStoreTestCase): ) +@ddt.ddt class MiscCourseTests(ContentStoreTestCase): """ Tests that rely on the toy courses. @@ -626,6 +690,76 @@ class MiscCourseTests(ContentStoreTestCase): 'Open Response Assessment', 'Peer Grading Interface', 'split_test'], ) + @ddt.data('/Fake/asset/displayname', '\\Fake\\asset\\displayname') + def test_export_on_invalid_displayname(self, invalid_displayname): + """ Tests that assets with invalid 'displayname' does not cause export to fail """ + content_store = contentstore() + exported_asset_name = '_Fake_asset_displayname' + + # Create an asset with slash `invalid_displayname` ' + asset_key = self.course.id.make_asset_key('asset', "fake_asset.txt") + content = StaticContent( + asset_key, invalid_displayname, 'application/text', 'test', + ) + content_store.save(content) + + # Verify that the course has only one asset and it has been added with an invalid asset name. + assets, count = content_store.get_all_content_for_course(self.course.id) + self.assertEqual(count, 1) + display_name = assets[0]['displayname'] + self.assertEqual(display_name, invalid_displayname) + + # Now export the course to a tempdir and test that it contains assets. The export should pass + root_dir = path(mkdtemp_clean()) + print 'Exporting to tempdir = {0}'.format(root_dir) + export_course_to_xml(self.store, content_store, self.course.id, root_dir, 'test_export') + + filesystem = OSFS(root_dir / 'test_export/static') + exported_static_files = filesystem.listdir() + + # Verify that only single asset has been exported with the expected asset name. + self.assertTrue(filesystem.exists(exported_asset_name)) + self.assertEqual(len(exported_static_files), 1) + + # Remove tempdir + shutil.rmtree(root_dir) + + def test_assets_overwrite(self): + """ Tests that assets will similar 'displayname' will be overwritten during export """ + content_store = contentstore() + asset_displayname = 'Fake_asset.txt' + + # Create two assets with similar 'displayname' + for i in range(2): + asset_path = 'sample_asset_{}.txt'.format(i) + asset_key = self.course.id.make_asset_key('asset', asset_path) + content = StaticContent( + asset_key, asset_displayname, 'application/text', 'test', + ) + content_store.save(content) + + # Fetch & verify course assets to be equal to 2. + assets, count = content_store.get_all_content_for_course(self.course.id) + self.assertEqual(count, 2) + + # Verify both assets have similar 'displayname' after saving. + for asset in assets: + self.assertEquals(asset['displayname'], asset_displayname) + + # Now export the course to a tempdir and test that it contains assets. + root_dir = path(mkdtemp_clean()) + print 'Exporting to tempdir = {0}'.format(root_dir) + export_course_to_xml(self.store, content_store, self.course.id, root_dir, 'test_export') + + # Verify that asset have been overwritten during export. + filesystem = OSFS(root_dir / 'test_export/static') + exported_static_files = filesystem.listdir() + self.assertTrue(filesystem.exists(asset_displayname)) + self.assertEqual(len(exported_static_files), 1) + + # Remove tempdir + shutil.rmtree(root_dir) + def test_advanced_components_require_two_clicks(self): self.check_components_on_page(['word_cloud'], ['Word cloud']) diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index d03de7a8fc..7ff65a6250 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -14,6 +14,7 @@ import json from bson.son import SON from opaque_keys.edx.keys import AssetKey from xmodule.modulestore.django import ASSET_IGNORE_REGEX +from xmodule.util.misc import escape_invalid_characters class MongoContentStore(ContentStore): @@ -131,15 +132,19 @@ class MongoContentStore(ContentStore): def export(self, location, output_directory): content = self.find(location) + filename = content.name if content.import_path is not None: output_directory = output_directory + '/' + os.path.dirname(content.import_path) if not os.path.exists(output_directory): os.makedirs(output_directory) + # Escape invalid char from filename. + export_name = escape_invalid_characters(name=filename, invalid_char_list=['/', '\\']) + disk_fs = OSFS(output_directory) - with disk_fs.open(content.name, 'wb') as asset_file: + with disk_fs.open(export_name, 'wb') as asset_file: asset_file.write(content.data) def export_all_for_course(self, course_key, output_directory, assets_policy_file): @@ -397,7 +402,8 @@ class MongoContentStore(ContentStore): sparse=True ) self.fs_files.create_index( - [('content_son.org', pymongo.ASCENDING), ('content_son.course', pymongo.ASCENDING), ('content_son.name', pymongo.ASCENDING)], + [('content_son.org', pymongo.ASCENDING), ('content_son.course', pymongo.ASCENDING), + ('content_son.name', pymongo.ASCENDING)], sparse=True ) self.fs_files.create_index( @@ -409,11 +415,13 @@ class MongoContentStore(ContentStore): sparse=True ) self.fs_files.create_index( - [('content_son.org', pymongo.ASCENDING), ('content_son.course', pymongo.ASCENDING), ('uploadDate', pymongo.ASCENDING)], + [('content_son.org', pymongo.ASCENDING), ('content_son.course', pymongo.ASCENDING), + ('uploadDate', pymongo.ASCENDING)], sparse=True ) self.fs_files.create_index( - [('content_son.org', pymongo.ASCENDING), ('content_son.course', pymongo.ASCENDING), ('display_name', pymongo.ASCENDING)], + [('content_son.org', pymongo.ASCENDING), ('content_son.course', pymongo.ASCENDING), + ('display_name', pymongo.ASCENDING)], sparse=True ) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 9510bbeea9..53bb517860 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -48,6 +48,7 @@ from xmodule.modulestore.mongo.base import MongoRevisionKey from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots from xmodule.modulestore.tests.utils import LocationMixin +from xmodule.util.misc import escape_invalid_characters log = logging.getLogger(__name__) @@ -106,7 +107,12 @@ def import_static_content( asset_key = StaticContent.compute_location(target_id, fullname_with_subpath) policy_ele = policy.get(asset_key.path, {}) - displayname = policy_ele.get('displayname', filename) + + # During export display name is used to create files, strip away slashes from name + displayname = escape_invalid_characters( + name=policy_ele.get('displayname', filename), + invalid_char_list=['/', '\\'] + ) locked = policy_ele.get('locked', False) mime_type = policy_ele.get('contentType') diff --git a/common/lib/xmodule/xmodule/util/misc.py b/common/lib/xmodule/xmodule/util/misc.py new file mode 100644 index 0000000000..fc977c49b1 --- /dev/null +++ b/common/lib/xmodule/xmodule/util/misc.py @@ -0,0 +1,26 @@ +""" +Miscellaneous utility functions. +""" + + +def escape_invalid_characters(name, invalid_char_list, replace_with='_'): + """ + Remove invalid characters from a variable and replace it with given character. + Few chars are not allowed in asset displayname, during import/export + Escape those chars with `replace_with` and return clean name + + Args: + name (str): variable to escape chars from. + invalid_char_list (list): Must be a list, and it should contain list of chars to be removed + from name + replace_with (str): Char used to replace invalid_char with. + + Returns: + name (str): name without `invalid_char_list`. + + """ + + for char in invalid_char_list: + if char in name: + name = name.replace(char, replace_with) + return name diff --git a/common/test/data/import_draft_order/policies/assets.json b/common/test/data/import_draft_order/policies/assets.json index 0967ef424b..fa45dca364 100644 --- a/common/test/data/import_draft_order/policies/assets.json +++ b/common/test/data/import_draft_order/policies/assets.json @@ -1 +1,18 @@ -{} +{ + "subs-esLhHcdKGWvKs.srt": { + "contentType": "application/octet-stream", + "content_son": { + "category": "asset", + "course": "EdxTest101", + "name": "subs-esLhHcdKGWvKs.srt", + "org": "Edx", + "revision": null, + "tag": "c4x" + }, + "displayname": "/invalid\\displayname/subs-esLhHcdKGWvKs.srt", + "filename": "/c4x/Edx/EdxTest101/asset/MyPathIsHere.srt", + "import_path": "subs-esLhHcdKGWvKs.srt", + "locked": false, + "thumbnail_location": null + } +} diff --git a/common/test/data/import_draft_order/static/subs-esLhHcdKGWvKs.srt b/common/test/data/import_draft_order/static/subs-esLhHcdKGWvKs.srt new file mode 100644 index 0000000000..bc63bee65c --- /dev/null +++ b/common/test/data/import_draft_order/static/subs-esLhHcdKGWvKs.srt @@ -0,0 +1,3 @@ +1 +00:00:03,620 --> 00:00:08,090 +this is a dummy transcript