Merge pull request #110 from edx/bug/dhm/baddrafts

Have the draft module store actively prevent non-leaf nodes from being

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -37,6 +37,9 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 from contentstore.views.component import ADVANCED_COMPONENT_TYPES
 
 from django_comment_common.utils import are_permissions_roles_seeded
+from xmodule.exceptions import InvalidVersionError
+import datetime
+from pytz import UTC
 
 TEST_DATA_MODULESTORE = copy.deepcopy(settings.MODULESTORE)
 TEST_DATA_MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data')
@@ -415,6 +418,32 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
             resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()}))
             self.assertEqual(resp.status_code, 200)
 
+    def test_illegal_draft_crud_ops(self):
+        draft_store = modulestore('draft')
+        direct_store = modulestore('direct')
+
+        CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
+
+        location = Location('i4x://MITx/999/chapter/neuvo')
+        self.assertRaises(InvalidVersionError, draft_store.clone_item, 'i4x://edx/templates/chapter/Empty',
+            location)
+        direct_store.clone_item('i4x://edx/templates/chapter/Empty', location)
+        self.assertRaises(InvalidVersionError, draft_store.clone_item, location,
+            location)
+
+        self.assertRaises(InvalidVersionError, draft_store.update_item, location,
+            'chapter data')
+
+        # taking advantage of update_children and other functions never checking that the ids are valid
+        self.assertRaises(InvalidVersionError, draft_store.update_children, location,
+            ['i4x://MITx/999/problem/doesntexist'])
+
+        self.assertRaises(InvalidVersionError, draft_store.update_metadata, location,
+            {'due': datetime.datetime.now(UTC)})
+
+        self.assertRaises(InvalidVersionError, draft_store.unpublish, location)
+
+
     def test_bad_contentstore_request(self):
         resp = self.client.get('http://localhost:8001/c4x/CDX/123123/asset/&images_circuits_Lab7Solution2.png')
         self.assertEqual(resp.status_code, 400)

cms/djangoapps/contentstore/utils.py

@@ -6,11 +6,10 @@ from django.core.urlresolvers import reverse
 import copy
 import logging
 import re
+from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES
 
 log = logging.getLogger(__name__)
 
-DIRECT_ONLY_CATEGORIES = ['course', 'chapter', 'sequential', 'about', 'static_tab', 'course_info']
-
 #In order to instantiate an open ended tab automatically, need to have this data
 OPEN_ENDED_PANEL = {"name": "Open Ended Panel", "type": "open_ended"}
 NOTES_PANEL = {"name": "My Notes", "type": "notes"}
@@ -229,7 +228,7 @@ def add_extra_panel_tab(tab_type, course):
     course_tabs = copy.copy(course.tabs)
     changed = False
     #Check to see if open ended panel is defined in the course
-    
+
     tab_panel = EXTRA_TAB_PANELS.get(tab_type)
     if tab_panel not in course_tabs:
         #Add panel to the tabs if it is not defined

common/lib/xmodule/xmodule/exceptions.py

@@ -12,3 +12,12 @@ class ProcessingError(Exception):
     For example: if an exception occurs while checking a capa problem.
     '''
     pass
+
+class InvalidVersionError(Exception):
+    """
+    Tried to save an item with a location that a store cannot support (e.g., draft version
+    for a non-leaf node)
+    """
+    def __init__(self, location):
+        super(InvalidVersionError, self).__init__()
+        self.location = location

common/lib/xmodule/xmodule/modulestore/draft.py

@@ -3,8 +3,11 @@ from datetime import datetime
 from . import ModuleStoreBase, Location, namedtuple_to_son
 from .exceptions import ItemNotFoundError
 from .inheritance import own_metadata
+from xmodule.exceptions import InvalidVersionError
 
 DRAFT = 'draft'
+# Things w/ these categories should never be marked as version='draft'
+DIRECT_ONLY_CATEGORIES = ['course', 'chapter', 'sequential', 'about', 'static_tab', 'course_info']
 
 
 def as_draft(location):
@@ -111,6 +114,8 @@ class DraftModuleStore(ModuleStoreBase):
         Clone a new item that is a copy of the item at the location `source`
         and writes it to `location`
         """
+        if Location(location).category in DIRECT_ONLY_CATEGORIES:
+            raise InvalidVersionError(location)
         return wrap_draft(super(DraftModuleStore, self).clone_item(source, as_draft(location)))
 
     def update_item(self, location, data, allow_not_found=False):
@@ -203,6 +208,8 @@ class DraftModuleStore(ModuleStoreBase):
         """
         Turn the published version into a draft, removing the published version
         """
+        if Location(location).category in DIRECT_ONLY_CATEGORIES:
+            raise InvalidVersionError(location)
         super(DraftModuleStore, self).clone_item(location, as_draft(location))
         super(DraftModuleStore, self).delete_item(location)