Merge pull request #8871 from edx/aj/tnl2669-invalid-display-name-export-not-working

Slashes in Asset Displayname cause `export` to fail

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'])
 

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
         )
 

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')
 

common/lib/xmodule/xmodule/util/misc.py

+"""
+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

common/test/data/import_draft_order/policies/assets.json

-{}
+{
+  "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
+  }
+}

common/test/data/import_draft_order/static/subs-esLhHcdKGWvKs.srt

+1
+00:00:03,620 --> 00:00:08,090
+this is a dummy transcript