Merge pull request #14092 from edx/cdyer/validate-capa-xml

Use new XMLString field type for CAPA data field.

cms/envs/common.py

@@ -215,6 +215,9 @@ FEATURES = {
 
     # Show Language selector
     'SHOW_LANGUAGE_SELECTOR': False,
+
+    # Set this to False to facilitate cleaning up invalid xml from your modulestore.
+    'ENABLE_XBLOCK_XML_VALIDATION': True,
 }
 
 ENABLE_JASMINE = False

common/lib/xmodule/xmodule/capa_base.py

@@ -6,28 +6,27 @@ import hashlib
 import json
 import logging
 import os
-import traceback
+import re
 import struct
 import sys
-import re
+import traceback
 
+from django.conf import settings
 # We don't want to force a dependency on datadog, so make the import conditional
 try:
     import dogstats_wrapper as dog_stats_api
 except ImportError:
     dog_stats_api = None
+from pytz import utc
 
 from capa.capa_problem import LoncapaProblem, LoncapaSystem
-from capa.responsetypes import StudentInputError, \
-    ResponseError, LoncapaProblemError
+from capa.responsetypes import StudentInputError, ResponseError, LoncapaProblemError
 from capa.util import convert_files_to_filenames, get_inner_html_from_xpath
-from .progress import Progress
-from xmodule.exceptions import NotFoundError
-from xblock.fields import Scope, String, Boolean, Dict, Integer, Float
-from .fields import Timedelta, Date
-from django.utils.timezone import UTC
+from xblock.fields import Boolean, Dict, Float, Integer, Scope, String, XMLString
 from xmodule.capa_base_constants import RANDOMIZATION, SHOWANSWER
-from django.conf import settings
+from xmodule.exceptions import NotFoundError
+from .fields import Date, Timedelta
+from .progress import Progress
 
 from openedx.core.djangolib.markup import HTML, Text
 
@@ -42,6 +41,8 @@ NUM_RANDOMIZATION_BINS = 20
 # Never produce more than this many different seeds, no matter what.
 MAX_RANDOMIZATION_BINS = 1000
 
+FEATURES = getattr(settings, 'FEATURES', {})
+
 
 def randomization_bin(seed, problem_id):
     """
@@ -76,7 +77,7 @@ class ComplexEncoder(json.JSONEncoder):
     """
     Extend the JSON encoder to correctly handle complex numbers
     """
-    def default(self, obj):
+    def default(self, obj):  # pylint: disable=method-hidden
         """
         Print a nicely formatted complex number, or default to the JSON encoder
         """
@@ -157,7 +158,12 @@ class CapaFields(object):
             {"display_name": _("Per Student"), "value": RANDOMIZATION.PER_STUDENT}
         ]
     )
-    data = String(help=_("XML data for the problem"), scope=Scope.content, default="<problem></problem>")
+    data = XMLString(
+        help=_("XML data for the problem"),
+        scope=Scope.content,
+        enforce_type=FEATURES.get('ENABLE_XBLOCK_XML_VALIDATION', True),
+        default="<problem></problem>"
+    )
     correct_map = Dict(help=_("Dictionary with the correctness of current student answers"),
                        scope=Scope.user_state, default={})
     input_state = Dict(help=_("Dictionary for maintaining the state of inputtypes"), scope=Scope.user_state)
@@ -257,10 +263,12 @@ class CapaMixin(CapaFields):
                     )
                 )
                 # create a dummy problem with error message instead of failing
-                problem_text = (u'<problem><text><span class="inline-error">'
+                problem_text = (
+                    u'<problem><text><span class="inline-error">'
                     u'Problem {url} has an error:</span>{msg}</text></problem>'.format(
                         url=self.location.to_deprecated_string(),
-                                    msg=msg)
+                        msg=msg,
+                    )
                 )
                 self.lcp = self.new_lcp(self.get_state_for_lcp(), text=problem_text)
             else:
@@ -349,7 +357,7 @@ class CapaMixin(CapaFields):
         """
         Set the module's last submission time (when the problem was submitted)
         """
-        self.last_submission_time = datetime.datetime.now(UTC())
+        self.last_submission_time = datetime.datetime.now(utc)
 
     def get_score(self):
         """
@@ -803,7 +811,7 @@ class CapaMixin(CapaFields):
         Is it now past this problem's due date, including grace period?
         """
         return (self.close_date is not None and
-                datetime.datetime.now(UTC()) > self.close_date)
+                datetime.datetime.now(utc) > self.close_date)
 
     def closed(self):
         """
@@ -1093,7 +1101,7 @@ class CapaMixin(CapaFields):
 
         metric_name = u'capa.check_problem.{}'.format
         # Can override current time
-        current_time = datetime.datetime.now(UTC())
+        current_time = datetime.datetime.now(utc)
         if override_time is not False:
             current_time = override_time
 
@@ -1128,8 +1136,9 @@ class CapaMixin(CapaFields):
 
         # Wait time between resets: check if is too soon for submission.
         if self.last_submission_time is not None and self.submission_wait_seconds != 0:
-            if (current_time - self.last_submission_time).total_seconds() < self.submission_wait_seconds:
-                remaining_secs = int(self.submission_wait_seconds - (current_time - self.last_submission_time).total_seconds())
+            seconds_since_submission = (current_time - self.last_submission_time).total_seconds()
+            if seconds_since_submission < self.submission_wait_seconds:
+                remaining_secs = int(self.submission_wait_seconds - seconds_since_submission)
                 msg = _(u'You must wait at least {wait_secs} between submissions. {remaining_secs} remaining.').format(
                     wait_secs=self.pretty_print_seconds(self.submission_wait_seconds),
                     remaining_secs=self.pretty_print_seconds(remaining_secs))
@@ -1343,7 +1352,7 @@ class CapaMixin(CapaFields):
                 log.warning('Input id %s is not mapped to an input type.', input_id)
 
             answer_response = None
-            for response, responder in self.lcp.responders.iteritems():
+            for responder in self.lcp.responders.itervalues():
                 if input_id in responder.answer_ids:
                     answer_response = responder
 
@@ -1406,8 +1415,10 @@ class CapaMixin(CapaFields):
         if not self.lcp.supports_rescoring():
             event_info['failure'] = 'unsupported'
             self.track_function_unmask('problem_rescore_fail', event_info)
+            # pylint: disable=line-too-long
             # Translators: 'rescoring' refers to the act of re-submitting a student's solution so it can get a new score.
             raise NotImplementedError(_("Problem's definition does not support rescoring."))
+            # pylint: enable=line-too-long
 
         if not self.done:
             event_info['failure'] = 'unanswered'
@@ -1485,8 +1496,10 @@ class CapaMixin(CapaFields):
             self.track_function_unmask('save_problem_fail', event_info)
             return {
                 'success': False,
+                # pylint: disable=line-too-long
                 # Translators: 'closed' means the problem's due date has passed. You may no longer attempt to solve the problem.
-                'msg': _("Problem is closed.")
+                'msg': _("Problem is closed."),
+                # pylint: enable=line-too-long
             }
 
         # Problem submitted. Student should reset before saving
@@ -1538,8 +1551,10 @@ class CapaMixin(CapaFields):
             self.track_function_unmask('reset_problem_fail', event_info)
             return {
                 'success': False,
+                # pylint: disable=line-too-long
                 # Translators: 'closed' means the problem's due date has passed. You may no longer attempt to solve the problem.
                 'msg': _("You cannot select Reset for a problem that is closed."),
+                # pylint: enable=line-too-long
             }
 
         if not self.is_submitted():

common/lib/xmodule/xmodule/tests/test_capa_module.py

@@ -81,14 +81,7 @@ class CapaFactory(object):
         )
 
     @classmethod
-    def create(cls,
-               attempts=None,
-               problem_state=None,
-               correct=False,
-               xml=None,
-               override_get_score=True,
-               **kwargs
-               ):
+    def create(cls, attempts=None, problem_state=None, correct=False, xml=None, override_get_score=True, **kwargs):
         """
         All parameters are optional, and are added to the created problem if specified.
 
@@ -228,7 +221,6 @@ class CapaModuleTest(unittest.TestCase):
         useful as unit-code coverage for this current implementation.  I don't see a layer where LoncapaProblem
         is tested directly
         """
-        from capa.correctmap import CorrectMap
         student_answers = {'1_2_1': 'abcd'}
         correct_map = CorrectMap(answer_id='1_2_1', correctness="correct", npoints=0.9)
         module = CapaFactory.create(correct=True, override_get_score=False)
@@ -623,6 +615,7 @@ class CapaModuleTest(unittest.TestCase):
 
         module.submit_problem(get_request_dict)
 
+        # pylint: disable=line-too-long
         # _http_post is called like this:
         #   _http_post(
         #       'http://example.com/xqueue/xqueue/submit/',
@@ -639,9 +632,10 @@ class CapaModuleTest(unittest.TestCase):
         #               <open file u'/home/ned/edx/edx-platform/common/test/data/uploads/textbook.pdf', mode 'r' at 0x49c5a50>,
         #       },
         #   )
+        # pylint: enable=line-too-long
 
         self.assertEqual(xqueue_interface._http_post.call_count, 1)
-        _, kwargs = xqueue_interface._http_post.call_args
+        _, kwargs = xqueue_interface._http_post.call_args  # pylint: disable=unpacking-non-sequence
         self.assertItemsEqual(fpaths, kwargs['files'].keys())
         for fpath, fileobj in kwargs['files'].iteritems():
             self.assertEqual(fpath, fileobj.name)
@@ -674,7 +668,7 @@ class CapaModuleTest(unittest.TestCase):
         module.handle('xmodule_handler', request, 'problem_check')
 
         self.assertEqual(xqueue_interface._http_post.call_count, 1)
-        _, kwargs = xqueue_interface._http_post.call_args
+        _, kwargs = xqueue_interface._http_post.call_args  # pylint: disable=unpacking-non-sequence
         self.assertItemsEqual(fnames, kwargs['files'].keys())
         for fpath, fileobj in kwargs['files'].iteritems():
             self.assertEqual(fpath, fileobj.name)
@@ -2487,18 +2481,15 @@ class CapaDescriptorTest(unittest.TestCase):
 
     def test_invalid_xml_handling(self):
         """
-        Tests to confirm that invalid XML does not throw a wake-up-ops level error.
-        See TNL-5057 for quick fix, TNL-5245 for full resolution.
+        Tests to confirm that invalid XML throws errors during xblock creation,
+        so as not to allow bad data into modulestore.
         """
         sample_invalid_xml = textwrap.dedent("""
             <problem>
             </proble-oh no my finger broke and I can't close the problem tag properly...
         """)
-        descriptor = self._create_descriptor(sample_invalid_xml, name="Invalid XML")
-        try:
-            descriptor.has_support(None, "multi_device")
-        except etree.XMLSyntaxError:
-            self.fail("Exception raised during XML parsing, this method should be resilient to such errors")
+        with self.assertRaises(etree.XMLSyntaxError):
+            self._create_descriptor(sample_invalid_xml, name="Invalid XML")
 
 
 class ComplexEncoderTest(unittest.TestCase):

common/test/acceptance/tests/studio/test_import_export.py

@@ -7,17 +7,14 @@ from datetime import datetime
 from flaky import flaky
 
 from abc import abstractmethod
-from bok_choy.promise import EmptyPromise
 
 from common.test.acceptance.tests.studio.base_studio_test import StudioLibraryTest, StudioCourseTest
-from common.test.acceptance.fixtures.course import XBlockFixtureDesc
 from common.test.acceptance.pages.studio.import_export import (
     ExportLibraryPage,
     ExportCoursePage,
     ImportLibraryPage,
     ImportCoursePage)
 from common.test.acceptance.pages.studio.library import LibraryEditPage
-from common.test.acceptance.pages.studio.container import ContainerPage
 from common.test.acceptance.pages.studio.overview import CourseOutlinePage
 from common.test.acceptance.pages.lms.courseware import CoursewarePage
 from common.test.acceptance.pages.lms.staff_view import StaffPage
@@ -86,84 +83,6 @@ class TestLibraryExport(ExportTestMixin, StudioLibraryTest):
         self.assertEqual(self.export_page.header_text, 'Library Export')
 
 
-class BadExportMixin(object):
-    """
-    Test mixin for bad exports.
-    """
-    def test_bad_export(self):
-        """
-        Scenario: I should receive an error when attempting to export a broken course or library.
-            Given that I have a course or library
-            No error modal should be showing
-            When I click the export button
-            An error modal should be shown
-            When I click the modal's action button
-            I should arrive at the edit page for the broken component
-        """
-        # No error should be there to start.
-        self.assertFalse(self.export_page.is_error_modal_showing())
-        self.export_page.click_export()
-        self.export_page.wait_for_error_modal()
-        self.export_page.click_modal_button()
-        self.edit_page.wait_for_page()
-
-
-@attr(shard=7)
-class TestLibraryBadExport(BadExportMixin, StudioLibraryTest):
-    """
-    Verify exporting a bad library causes an error.
-    """
-
-    def setUp(self):
-        """
-        Set up the pages and start the tests.
-        """
-        super(TestLibraryBadExport, self).setUp()
-        self.export_page = ExportLibraryPage(self.browser, self.library_key)
-        self.edit_page = LibraryEditPage(self.browser, self.library_key)
-        self.export_page.visit()
-
-    def populate_library_fixture(self, library_fixture):
-        """
-        Create a library with a bad component.
-        """
-        library_fixture.add_children(
-            XBlockFixtureDesc("problem", "Bad Problem", data='<'),
-        )
-
-
-@attr(shard=7)
-class TestCourseBadExport(BadExportMixin, StudioCourseTest):
-    """
-    Verify exporting a bad course causes an error.
-    """
-    ready_method = 'wait_for_component_menu'
-
-    def setUp(self):  # pylint: disable=arguments-differ
-        super(TestCourseBadExport, self).setUp()
-        self.export_page = ExportCoursePage(
-            self.browser,
-            self.course_info['org'], self.course_info['number'], self.course_info['run'],
-        )
-        self.edit_page = ContainerPage(self.browser, self.unit.locator)
-        self.export_page.visit()
-
-    def populate_course_fixture(self, course_fixture):
-        """
-        Populate the course with a unit that has a bad problem.
-        """
-        self.unit = XBlockFixtureDesc('vertical', 'Unit')
-        course_fixture.add_children(
-            XBlockFixtureDesc('chapter', 'Main Section').add_children(
-                XBlockFixtureDesc('sequential', 'Subsection').add_children(
-                    self.unit.add_children(
-                        XBlockFixtureDesc("problem", "Bad Problem", data='<')
-                    )
-                )
-            )
-        )
-
-
 @attr(shard=7)
 class ImportTestMixin(object):
     """

lms/envs/common.py

@@ -365,6 +365,9 @@ FEATURES = {
     # Note: This has no effect unless ANALYTICS_DASHBOARD_URL is already set,
     #       because without that setting, the tab does not show up for any courses.
     'ENABLE_CCX_ANALYTICS_DASHBOARD_URL': False,
+
+    # Set this to False to facilitate cleaning up invalid xml from your modulestore.
+    'ENABLE_XBLOCK_XML_VALIDATION': True,
 }
 
 # Ignore static asset files on import which match this pattern

requirements/edx/github.txt

@@ -71,7 +71,7 @@ git+https://github.com/edx/rfc6266.git@v0.0.5-edx#egg=rfc6266==0.0.5-edx
 git+https://github.com/edx/lettuce.git@0.2.20.002#egg=lettuce==0.2.20.002
 
 # Our libraries:
-git+https://github.com/edx/XBlock.git@xblock-0.4.12#egg=XBlock==0.4.12
+git+https://github.com/edx/XBlock.git@xblock-0.4.13#egg=XBlock==0.4.13
 -e git+https://github.com/edx/codejail.git@6b17c33a89bef0ac510926b1d7fea2748b73aadd#egg=codejail
 -e git+https://github.com/edx/event-tracking.git@0.2.1#egg=event-tracking==0.2.1
 -e git+https://github.com/edx/django-splash.git@v0.2#egg=django-splash==0.2