From 5bc09407df76551a9f31012af332452070c39b46 Mon Sep 17 00:00:00 2001 From: Andrew Gardener Date: Tue, 22 Aug 2017 17:16:51 -0700 Subject: [PATCH] Limit user input in xAPI statements - currently only applies to statement.result.response, object.definition.name, object.definition.description - also refactored statements sending to create one job per statement to prevent POST request size issues when sending multiple statements in one request --- README.md | 2 + compair/configuration.py | 2 +- compair/settings.py | 2 + compair/tasks/__init__.py | 2 +- compair/tasks/xapi_statement.py | 5 +- compair/tests/test_compair.py | 31 ++-- compair/tests/xapi/test_remote.py | 232 ++++++++++++++++++++++++++++++ compair/xapi/capture_events.py | 9 +- compair/xapi/xapi.py | 53 ++++--- 9 files changed, 300 insertions(+), 38 deletions(-) create mode 100644 compair/tests/xapi/test_remote.py diff --git a/README.md b/README.md index e2bec6b59..a4ad5efb7 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,8 @@ xAPI statements require an actor (currently logged in user) account information. `LRS_ACTOR_ACCOUNT_CAS_IDENTIFIER`: Optionally set a param to set as the actor's unique key for the CAS account. (uses CAS username by default) +`LRS_USER_INPUT_FIELD_SIZE_LIMIT`: Set the byte limit on xAPI statement fields containing user input. Set this in order to prevent sending large statements to the LRS that it can't handle (1048576 by default or 1MB) + Restart server after making any changes to settings (Optional) Setting up Background Tasks diff --git a/compair/configuration.py b/compair/configuration.py index c7c042c6f..11bc0e33c 100644 --- a/compair/configuration.py +++ b/compair/configuration.py @@ -87,7 +87,7 @@ env_int_overridables = [ 'SQLALCHEMY_POOL_RECYCLE', - 'ATTACHMENT_UPLOAD_LIMIT', + 'ATTACHMENT_UPLOAD_LIMIT', 'LRS_USER_INPUT_FIELD_SIZE_LIMIT', 'MAIL_PORT', 'MAIL_MAX_EMAILS' ] diff --git a/compair/settings.py b/compair/settings.py index 2a8943143..892c2b4ab 100644 --- a/compair/settings.py +++ b/compair/settings.py @@ -60,6 +60,8 @@ LRS_AUTH = None LRS_USERNAME = None LRS_PASSWORD = None +# limit user generated content field text size limit +LRS_USER_INPUT_FIELD_SIZE_LIMIT = 1048576 #1024 * 1024 -> max 1MB LRS_ACTOR_ACCOUNT_USE_CAS = False # set to True to use CAS account information if available LRS_ACTOR_ACCOUNT_CAS_IDENTIFIER = None # set to a param field value to use or None to use unique_identifier diff --git a/compair/tasks/__init__.py b/compair/tasks/__init__.py index e00d4088b..6229b1123 100644 --- a/compair/tasks/__init__.py +++ b/compair/tasks/__init__.py @@ -3,4 +3,4 @@ from .lti_outcomes import update_lti_course_grades, update_lti_assignment_grades from .send_mail import send_message, send_messages from .user_password import set_passwords -from .xapi_statement import send_lrs_statements \ No newline at end of file +from .xapi_statement import send_lrs_statement \ No newline at end of file diff --git a/compair/tasks/xapi_statement.py b/compair/tasks/xapi_statement.py index aebae54c4..78220e610 100644 --- a/compair/tasks/xapi_statement.py +++ b/compair/tasks/xapi_statement.py @@ -7,10 +7,9 @@ @celery.task(bind=True, autoretry_for=(Exception,), ignore_result=True, store_errors_even_if_ignored=True) -def send_lrs_statements(self, statements): +def send_lrs_statement(self, statement_string): try: - statements = [json.loads(statement) for statement in statements] - XAPI._send_lrs_statements(statements) + XAPI._send_lrs_statement(json.loads(statement_string)) except socket.error as error: # don't raise connection refused error when in eager mode if error.errno != socket.errno.ECONNREFUSED: diff --git a/compair/tests/test_compair.py b/compair/tests/test_compair.py index a3017be2d..467687d55 100644 --- a/compair/tests/test_compair.py +++ b/compair/tests/test_compair.py @@ -117,25 +117,28 @@ def generate_tracking(self, with_duration=False, **kargs): return tracking + def _validate_and_cleanup_statement(self, statement): + # check categories + categories = statement['context']['contextActivities']['category'] + self.assertIn(self.compair_source_category, categories) + categories.remove(self.compair_source_category) + + if len(categories) == 0: + del statement['context']['contextActivities']['category'] + if len(statement['context']['contextActivities']) == 0: + del statement['context']['contextActivities'] + if len(statement['context']) == 0: + del statement['context'] + + # check timestamp + self.assertIsNotNone(statement['timestamp']) + def get_and_clear_statement_log(self, has_request=False): statements = [] for xapi_log in XAPILog.query.all(): statement = json.loads(xapi_log.statement) - # check categories - categories = statement['context']['contextActivities']['category'] - self.assertIn(self.compair_source_category, categories) - categories.remove(self.compair_source_category) - - if len(categories) == 0: - del statement['context']['contextActivities']['category'] - if len(statement['context']['contextActivities']) == 0: - del statement['context']['contextActivities'] - if len(statement['context']) == 0: - del statement['context'] - - # check timestamp - self.assertIsNotNone(statement['timestamp']) + self._validate_and_cleanup_statement(statement) statements.append(statement) XAPILog.query.delete() diff --git a/compair/tests/xapi/test_remote.py b/compair/tests/xapi/test_remote.py new file mode 100644 index 000000000..68d6c18d0 --- /dev/null +++ b/compair/tests/xapi/test_remote.py @@ -0,0 +1,232 @@ +import json +import mock +from six import text_type + +from data.fixtures.test_data import SimpleAnswersTestData +from compair.tests.test_compair import ComPAIRXAPITestCase + + +from compair.core import db +from flask_login import current_app +from compair.xapi import XAPIStatement, XAPIVerb, XAPIObject, \ + XAPIContext, XAPIResult, XAPI +from tincan import LRSResponse + +class RemoteXAPITests(ComPAIRXAPITestCase): + + def setUp(self): + super(ComPAIRXAPITestCase, self).setUp() + self.app.config['LRS_STATEMENT_ENDPOINT'] = 'http://example.com' + self.app.config['LRS_USERNAME'] = 'lrs_username' + self.app.config['LRS_PASSWORD'] = 'lrs_password' + self.app.config['LRS_USER_INPUT_FIELD_SIZE_LIMIT'] = 200 # 200 bytes + + self.data = SimpleAnswersTestData() + self.user = self.data.authorized_student + self.course = self.data.main_course + self.assignment = self.data.assignments[0] + self.answer = self.data.create_answer(self.assignment, self.user) + self.sent_statement = None + self.character_limit = int(current_app.config.get('LRS_USER_INPUT_FIELD_SIZE_LIMIT') / len("c".encode('utf-8'))) + + @mock.patch('tincan.RemoteLRS.save_statement') + def test_send_remote_statement(self, mocked_save_statement): + + def save_statement_override(statement): + self.sent_statement = json.loads(statement.to_json(XAPI._version)) + + return LRSResponse( + success=True, + request=None, + response=None, + data=json.dumps(["123"]), + ) + mocked_save_statement.side_effect = save_statement_override + + + # test with answer normal content + statement = XAPIStatement.generate( + user=self.user, + verb=XAPIVerb.generate('submitted'), + object=XAPIObject.answer(self.answer), + context=XAPIContext.answer(self.answer), + result=XAPIResult.answer(self.answer, includeAttachment=True, success=True) + ) + + XAPI._send_lrs_statement(json.loads(statement.to_json(XAPI._version))) + + self._validate_and_cleanup_statement(self.sent_statement) + + self.assertEqual(self.sent_statement['actor'], self.get_compair_actor(self.user)) + self.assertEqual(self.sent_statement['verb'], { + 'id': 'http://activitystrea.ms/schema/1.0/submit', + 'display': {'en-US': 'submitted'} + }) + self.assertEqual(self.sent_statement['object'], { + 'id': 'https://localhost:8888/app/xapi/answer/'+self.answer.uuid, + 'definition': {'type': 'http://id.tincanapi.com/activitytype/solution', 'name': {'en-US': 'Assignment answer'}}, + 'objectType': 'Activity' + }) + self.assertEqual(self.sent_statement['result'], { + 'extensions': { + 'http://xapi.learninganalytics.ubc.ca/extension/character-count': len(self.answer.content), + 'http://xapi.learninganalytics.ubc.ca/extension/word-count': len(self.answer.content.split(" ")) + }, + 'response': self.answer.content, + 'success': True + }) + self.assertEqual(self.sent_statement['context'], { + 'contextActivities': { + 'grouping': [{ + 'id': 'https://localhost:8888/app/xapi/course/'+self.course.uuid, + 'objectType': 'Activity' + },{ + 'id': 'https://localhost:8888/app/xapi/assignment/'+self.assignment.uuid, + 'objectType': 'Activity' + }], + 'parent': [{ + 'id': 'https://localhost:8888/app/xapi/assignment/'+self.assignment.uuid+'/question', + 'objectType': 'Activity' + }] + } + }) + + # test with extremely long answer content + + # content should be ~ LRS_USER_INPUT_FIELD_SIZE_LIMIT bytes long + 100 characters + content = "c" * (self.character_limit + 100) + # expected_result_response should be <= LRS_USER_INPUT_FIELD_SIZE_LIMIT bytes long + " [TEXT TRIMMED]..." + expected_result_response = ("c" * self.character_limit) + " [TEXT TRIMMED]..." + + self.answer.content = content + db.session.commit() + + statement = XAPIStatement.generate( + user=self.user, + verb=XAPIVerb.generate('submitted'), + object=XAPIObject.answer(self.answer), + context=XAPIContext.answer(self.answer), + result=XAPIResult.answer(self.answer, includeAttachment=True, success=True) + ) + + XAPI._send_lrs_statement(json.loads(statement.to_json(XAPI._version))) + + self._validate_and_cleanup_statement(self.sent_statement) + + self.assertEqual(self.sent_statement['actor'], self.get_compair_actor(self.user)) + self.assertEqual(self.sent_statement['verb'], { + 'id': 'http://activitystrea.ms/schema/1.0/submit', + 'display': {'en-US': 'submitted'} + }) + self.assertEqual(self.sent_statement['object'], { + 'id': 'https://localhost:8888/app/xapi/answer/'+self.answer.uuid, + 'definition': {'type': 'http://id.tincanapi.com/activitytype/solution', 'name': {'en-US': 'Assignment answer'}}, + 'objectType': 'Activity' + }) + self.assertEqual(self.sent_statement['result'], { + 'extensions': { + 'http://xapi.learninganalytics.ubc.ca/extension/character-count': len(self.answer.content), + 'http://xapi.learninganalytics.ubc.ca/extension/word-count': len(self.answer.content.split(" ")) + }, + 'response': expected_result_response, + 'success': True + }) + self.assertEqual(self.sent_statement['context'], { + 'contextActivities': { + 'grouping': [{ + 'id': 'https://localhost:8888/app/xapi/course/'+self.course.uuid, + 'objectType': 'Activity' + },{ + 'id': 'https://localhost:8888/app/xapi/assignment/'+self.assignment.uuid, + 'objectType': 'Activity' + },], + 'parent': [{ + 'id': 'https://localhost:8888/app/xapi/assignment/'+self.assignment.uuid+'/question', + 'objectType': 'Activity' + }] + } + }) + + # test with assignment normal content + statement = XAPIStatement.generate( + user=self.user, + verb=XAPIVerb.generate('updated'), + object=XAPIObject.assignment(self.assignment), + context=XAPIContext.assignment(self.assignment) + ) + + XAPI._send_lrs_statement(json.loads(statement.to_json(XAPI._version))) + + self._validate_and_cleanup_statement(self.sent_statement) + + self.assertEqual(self.sent_statement['actor'], self.get_compair_actor(self.user)) + self.assertEqual(self.sent_statement['verb'], { + 'id': 'http://activitystrea.ms/schema/1.0/update', + 'display': {'en-US': 'updated'} + }) + self.assertEqual(self.sent_statement['object'], { + 'id': 'https://localhost:8888/app/xapi/assignment/'+self.assignment.uuid, + 'definition': { + 'type': 'http://adlnet.gov/expapi/activities/assessment', + 'name': {'en-US': self.assignment.name }, + 'description': {'en-US': self.assignment.description } + }, + 'objectType': 'Activity' + }) + self.assertNotIn('result', self.sent_statement) + self.assertEqual(self.sent_statement['context'], { + 'contextActivities': { + 'parent': [{ + 'id': 'https://localhost:8888/app/xapi/course/'+self.course.uuid, + 'objectType': 'Activity' + }] + } + }) + + # test with extremely long answer content + + # content should be ~ LRS_USER_INPUT_FIELD_SIZE_LIMIT bytes long + 100 characters + name = "a" * (self.character_limit + 100) + description = "b" * (self.character_limit + 100) + # expected_result_response should be <= LRS_USER_INPUT_FIELD_SIZE_LIMIT bytes long + " [TEXT TRIMMED]..." + expected_object_name = ("a" * self.character_limit) + " [TEXT TRIMMED]..." + expected_object_description = ("b" * self.character_limit) + " [TEXT TRIMMED]..." + + self.assignment.name = name + self.assignment.description = description + db.session.commit() + + statement = XAPIStatement.generate( + user=self.user, + verb=XAPIVerb.generate('updated'), + object=XAPIObject.assignment(self.assignment), + context=XAPIContext.assignment(self.assignment) + ) + + XAPI._send_lrs_statement(json.loads(statement.to_json(XAPI._version))) + + self._validate_and_cleanup_statement(self.sent_statement) + + self.assertEqual(self.sent_statement['actor'], self.get_compair_actor(self.user)) + self.assertEqual(self.sent_statement['verb'], { + 'id': 'http://activitystrea.ms/schema/1.0/update', + 'display': {'en-US': 'updated'} + }) + self.assertEqual(self.sent_statement['object'], { + 'id': 'https://localhost:8888/app/xapi/assignment/'+self.assignment.uuid, + 'definition': { + 'type': 'http://adlnet.gov/expapi/activities/assessment', + 'name': {'en-US': expected_object_name }, + 'description': {'en-US': expected_object_description } + }, + 'objectType': 'Activity' + }) + self.assertNotIn('result', self.sent_statement) + self.assertEqual(self.sent_statement['context'], { + 'contextActivities': { + 'parent': [{ + 'id': 'https://localhost:8888/app/xapi/course/'+self.course.uuid, + 'objectType': 'Activity' + }] + } + }) \ No newline at end of file diff --git a/compair/xapi/capture_events.py b/compair/xapi/capture_events.py index 2fcf80bfb..b015ee8c4 100644 --- a/compair/xapi/capture_events.py +++ b/compair/xapi/capture_events.py @@ -220,7 +220,8 @@ def xapi_on_answer_comment_modified(sender, user, **extra): ) statements.append(statement) - XAPI.send_statements(statements) + for statement in statements: + XAPI.send_statement(statement) else: # (public or private) statement = XAPIStatement.generate( @@ -300,7 +301,8 @@ def xapi_on_answer_modified(sender, user, **extra): ) statements.append(statement) - XAPI.send_statements(statements) + for statement in statements: + XAPI.send_statement(statement) # on_answer_delete # deleted answer_solution @@ -494,7 +496,8 @@ def xapi_on_comparison_update(sender, user, **extra): ) statements.append(statement) - XAPI.send_statements(statements) + for statement in statements: + XAPI.send_statement(statement) # on_course_create diff --git a/compair/xapi/xapi.py b/compair/xapi/xapi.py index fd8a9fe00..7e228b611 100644 --- a/compair/xapi/xapi.py +++ b/compair/xapi/xapi.py @@ -2,6 +2,9 @@ from tincan import RemoteLRS from compair.models import XAPILog from compair.core import db +from six import text_type + +from tincan import Statement class XAPI(object): _version = '1.0.1' @@ -21,32 +24,41 @@ def get_base_url(cls): @classmethod def send_statement(cls, statement): - cls.send_statements([statement]) - - @classmethod - def send_statements(cls, statements): if not cls.enabled: return + statement_string = statement.to_json(cls._version) + if current_app.config.get('LRS_STATEMENT_ENDPOINT') == 'local': - xapi_logs = [] - for statement in statements: - xapi_logs.append(XAPILog( - statement=statement.to_json(cls._version) - )) - db.session.add_all(xapi_logs) + xapi_log = XAPILog( + statement=statement_string + ) + db.session.add(xapi_log) db.session.commit() else: - from compair.tasks import send_lrs_statements - send_lrs_statements.delay([statement.to_json(cls._version) for statement in statements]) + from compair.tasks import send_lrs_statement + send_lrs_statement.delay(statement_string) + + + @classmethod + def _trim_text_to_size(cls, text): + size_limit = current_app.config.get('LRS_USER_INPUT_FIELD_SIZE_LIMIT') + encoded_text = text.encode('utf-8') + + # if encoded_text is larger than size_limit, trim it + if len(encoded_text) > size_limit: + encoded_text = encoded_text[:size_limit] + return text_type(encoded_text.decode('utf-8', 'ignore'))+" [TEXT TRIMMED]..." + else: + return text @classmethod - def _send_lrs_statements(cls, statements): + def _send_lrs_statement(cls, statement_json): if not cls.enabled: return - # should only be called by delayed task send_lrs_statements + # should only be called by delayed task send_lrs_statement lrs_settings = { 'version': cls._version, 'endpoint': current_app.config.get('LRS_STATEMENT_ENDPOINT') @@ -58,8 +70,17 @@ def _send_lrs_statements(cls, statements): lrs_settings['username'] = current_app.config.get('LRS_USERNAME') lrs_settings['password'] = current_app.config.get('LRS_PASSWORD') - lrs = RemoteLRS(**lrs_settings) - lrs_response = lrs.save_statements(statements) + statement = Statement(statement_json) + # check statement.result.response, object.definition.name, object.definition.description + if statement.result and statement.result.response: + statement.result.response = cls._trim_text_to_size(statement.result.response) + if statement.object and statement.object.definition and statement.object.definition.name: + statement.object.definition.name['en-US'] = cls._trim_text_to_size(statement.object.definition.name['en-US']) + if statement.object and statement.object.definition and statement.object.definition.description: + statement.object.definition.description['en-US'] = cls._trim_text_to_size(statement.object.definition.description['en-US']) + + lrs = RemoteLRS(**lrs_settings) + lrs_response = lrs.save_statement(statement) if not lrs_response.success: current_app.logger.error("xAPI Failed with: " + str(lrs_response.data))