Merge pull request #44 from appsembler/omar/pluggable-params

Allow plugins for the LTI XBlock to pass extra parameters to the provider

README.rst

@@ -76,6 +76,75 @@ http://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/lat
    tests for this repo running inside an LMS container).  From here, you can see the contents of the
    messages that we are sending as an LTI Consumer in the "Message Parameters" part of the "Message" tab.
 
+Custom LTI Parameters
+---------------------
+This XBlock sends a number of parameters to the provider including some optional parameters. To keep the XBlock
+somewhat minimal, some parameters were omitted like ``lis_person_name_full`` among others.
+At the same time the XBlock allows passing extra parameters to the LTI provider via parameter processor functions.
+
+Defining an LTI Parameter Processors
+====================================
+The parameter processor is a function that expects an XBlock instance, and returns a ``dict`` of
+additional parameters for the LTI.
+If a processor throws an exception, the exception is logged and suppressed.
+If a processor returns ``None`` or any falsy value, no parameters will be added.
+
+.. code:: python
+
+    def team_info(xblock):
+        course = get_team(xblock.user, lti_params.course.id)
+        if not course:
+            return
+
+        return {
+            'custom_course_id': unicode(course.id),
+            'custom_course_name': course.name,
+        }
+
+A processor can define a list of default parameters ``lti_xblock_default_params``,
+which is useful in case the processor had an exception.
+
+It is recommended to define default parameters anyway, because it can simplify the implementation of the processor
+function. Below is an example:
+
+.. code:: python
+
+    def dummy_processor(xblock):
+        course = get_team(xblock.user, lti_params.course.id)  # If something went wrong default params will be used
+        if not course:
+            return  # Will use the default params
+
+        return {
+            'custom_course_id': unicode(course.id),
+            'custom_course_name': course.name,
+        }
+
+    dummy_processor.lti_xblock_default_params = {
+        'custom_course_id': '',
+        'custom_course_name': '',
+    }
+
+If you're looking for a more realistic example, you can check the
+`Tahoe LTI <https://github.com/appsembler/tahoe-lti>`_ repository at the
+`Appsembler GitHub organization <https://github.com/appsembler/>`_.
+
+Configuring the Parameter Processors Settings
+=============================================
+
+Using the standard XBlock settings interface the developer can provide a list of processor functions:
+Those parameters are not sent by default. The course author can enable that on per XBlock instance
+(aka module) by setting the **Send extra parameters** to ``true`` in Studio.
+
+To configure parameter processors add the following snippet to your Ansible variable files:
+
+.. code:: yaml
+
+    EDXAPP_XBLOCK_SETTINGS:
+      lti_consumer:
+        parameter_processors:
+          - 'customer_package.lti_processors:team_and_cohort'
+          - 'example_package.lti_processors:extra_lti_params'
+
 Workbench installation and settings
 -----------------------------------
 

lti_consumer/lti.py

@@ -174,6 +174,16 @@ class LtiConsumer(object):
         # Appending custom parameter for signing.
         lti_parameters.update(self.xblock.prefixed_custom_parameters)
 
+        for processor in self.xblock.get_parameter_processors():
+            try:
+                default_params = getattr(processor, 'lti_xblock_default_params', {})
+                lti_parameters.update(default_params)
+                lti_parameters.update(processor(self.xblock) or {})
+            except Exception:  # pylint: disable=broad-except
+                # Log the error without causing a 500-error.
+                # Useful for catching casual runtime errors in the processors.
+                log.exception('Error in XBlock LTI parameter processor "%s"', processor)
+
         headers = {
             # This is needed for body encoding:
             'Content-Type': 'application/x-www-form-urlencoded',

lti_consumer/lti_consumer.py

@@ -53,6 +53,7 @@ What is supported:
 import logging
 import bleach
 import re
+from importlib import import_module
 import json
 import urllib
 
@@ -164,6 +165,7 @@ class LaunchTarget(object):
 
 
 @XBlock.needs('i18n')
+@XBlock.wants('settings')
 @XBlock.wants('lti-configuration')
 class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
     """
@@ -247,6 +249,8 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
         Otherwise error message from LTI provider is generated.
     """
 
+    block_settings_key = 'lti_consumer'
+
     display_name = String(
         display_name=_("Display Name"),
         help=_(
@@ -421,20 +425,85 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
         default=False,
         scope=Scope.settings
     )
+    enable_processors = Boolean(
+        display_name=_("Send extra parameters"),
+        help=_("Select True to send the extra parameters, which might contain Personally Identifiable Information. "
+               "The processors are site-wide, please consult the site administrator if you have any questions."),
+        default=False,
+        scope=Scope.settings
+    )
 
     # Possible editable fields
     editable_field_names = (
         'display_name', 'description', 'lti_id', 'launch_url', 'custom_parameters',
         'launch_target', 'button_text', 'inline_height', 'modal_height', 'modal_width',
         'has_score', 'weight', 'hide_launch', 'accept_grades_past_due', 'ask_to_send_username',
-        'ask_to_send_email'
+        'ask_to_send_email', 'enable_processors',
     )
 
+    @staticmethod
+    def workbench_scenarios():
+        """
+        Gather scenarios to be displayed in the workbench
+        """
+        scenarios = [
+            ('LTI Consumer XBlock',
+             '''<sequence_demo>
+                    <lti_consumer
+                        display_name="LTI Consumer - New Window"
+                        lti_id="test"
+                        description=""
+                        ask_to_send_username="False"
+                        ask_to_send_email="False"
+                        enable_processors="True"
+                        launch_target="new_window"
+                        launch_url="https://lti.tools/saltire/tp" />
+
+                    <lti_consumer
+                        display_name="LTI Consumer - IFrame"
+                        lti_id="test"
+                        ask_to_send_username="False"
+                        ask_to_send_email="False"
+                        enable_processors="True"
+                        description=""
+                        launch_target="iframe"
+                        launch_url="https://lti.tools/saltire/tp" />
+                </sequence_demo>
+             '''),
+        ]
+        return scenarios
+
     def validate_field_data(self, validation, data):
         if not isinstance(data.custom_parameters, list):
             _ = self.runtime.service(self, "i18n").ugettext
             validation.add(ValidationMessage(ValidationMessage.ERROR, unicode(_("Custom Parameters must be a list"))))
 
+    def get_settings(self):
+        """
+        Get the XBlock settings bucket via the SettingsService.
+        """
+        settings_service = self.runtime.service(self, 'settings')
+        if settings_service:
+            return settings_service.get_settings_bucket(self)
+
+        return {}
+
+    def get_parameter_processors(self):
+        """
+        Read the parameter processor functions from the settings and return their functions.
+        """
+        if not self.enable_processors:
+            return
+
+        try:
+            for path in self.get_settings().get('parameter_processors', []):
+                module_path, func_name = path.split(':', 1)
+                module = import_module(module_path)
+                yield getattr(module, func_name)
+        except Exception:
+            log.exception('Something went wrong in reading the LTI XBlock configuration.')
+            raise
+
     @property
     def editable_fields(self):
         """

lti_consumer/tests/unit/test_lti.py

@@ -11,7 +11,7 @@ from six import text_type
 
 from django.utils import timezone
 
-from lti_consumer.tests.unit.test_utils import FAKE_USER_ID, make_request
+from lti_consumer.tests.unit.test_utils import make_request, patch_signed_parameters
 from lti_consumer.tests.unit.test_lti_consumer import TestLtiConsumerXBlock
 
 from lti_consumer.lti import parse_result_json, LtiConsumer
@@ -129,30 +129,27 @@ class TestLtiConsumer(TestLtiConsumerXBlock):
         super(TestLtiConsumer, self).setUp()
         self.lti_consumer = LtiConsumer(self.xblock)
 
-    @patch(
-        'lti_consumer.lti.get_oauth_request_signature',
-        Mock(return_value=(
-            'OAuth oauth_nonce="fake_nonce", '
-            'oauth_timestamp="fake_timestamp", oauth_version="fake_version", oauth_signature_method="fake_method", '
-            'oauth_consumer_key="fake_consumer_key", oauth_signature="fake_signature"'
-        ))
-    )
-    @patch(
-        'lti_consumer.lti_consumer.LtiConsumerXBlock.prefixed_custom_parameters',
-        PropertyMock(return_value={u'custom_param_1': 'custom1', u'custom_param_2': 'custom2'})
-    )
-    @patch(
-        'lti_consumer.lti_consumer.LtiConsumerXBlock.lti_provider_key_secret',
-        PropertyMock(return_value=('t', 's'))
-    )
-    @patch('lti_consumer.lti_consumer.LtiConsumerXBlock.user_id', PropertyMock(return_value=FAKE_USER_ID))
-    def test_get_signed_lti_parameters(self):
+    def _update_xblock_for_signed_parameters(self):
         """
-        Test `get_signed_lti_parameters` returns the correct dict
+        Prepare the LTI XBlock for signing the parameters.
         """
         self.lti_consumer.xblock.due = timezone.now()
         self.lti_consumer.xblock.graceperiod = timedelta(days=1)
+        self.lti_consumer.xblock.has_score = True
+        self.lti_consumer.xblock.ask_to_send_username = True
+        self.lti_consumer.xblock.ask_to_send_email = True
+        self.lti_consumer.xblock.runtime.get_real_user.return_value = Mock(
+            email='edx@example.com',
+            username='edx',
+            preferences=Mock(filter=Mock(return_value=[Mock(value='en')]))
+        )
 
+    @patch_signed_parameters
+    def test_get_signed_lti_parameters(self):
+        """
+        Test `get_signed_lti_parameters` returns the correct dict
+        """
+        self._update_xblock_for_signed_parameters()
         expected_lti_parameters = {
             text_type('user_id'): self.lti_consumer.xblock.user_id,
             text_type('oauth_callback'): 'about:blank',
@@ -181,14 +178,6 @@ class TestLtiConsumer(TestLtiConsumerXBlock):
             text_type('context_label'): self.lti_consumer.xblock.course.display_org_with_default,
             text_type('context_title'): self.lti_consumer.xblock.course.display_name_with_default,
         }
-        self.lti_consumer.xblock.has_score = True
-        self.lti_consumer.xblock.ask_to_send_username = True
-        self.lti_consumer.xblock.ask_to_send_email = True
-        self.lti_consumer.xblock.runtime.get_real_user.return_value = Mock(
-            email='edx@example.com',
-            username='edx',
-            preferences=Mock(filter=Mock(return_value=[Mock(value='en')]))
-        )
         self.assertEqual(self.lti_consumer.get_signed_lti_parameters(), expected_lti_parameters)
 
         # Test that `lis_person_sourcedid`, `lis_person_contact_email_primary`, and `launch_presentation_locale`
@@ -199,6 +188,53 @@ class TestLtiConsumer(TestLtiConsumerXBlock):
         del expected_lti_parameters['launch_presentation_locale']
         self.assertEqual(self.lti_consumer.get_signed_lti_parameters(), expected_lti_parameters)
 
+    @patch_signed_parameters
+    @patch('lti_consumer.lti.log')
+    def test_parameter_processors(self, mock_log):
+        self._update_xblock_for_signed_parameters()
+        self.xblock.enable_processors = True
+
+        with patch('lti_consumer.lti_consumer.LtiConsumerXBlock.get_settings', return_value={
+            'parameter_processors': [
+                'lti_consumer.tests.unit.test_utils:dummy_processor',
+            ],
+        }):
+            params = self.lti_consumer.get_signed_lti_parameters()
+            assert '' == params['custom_author_country']
+            assert 'author@example.com' == params['custom_author_email']
+            assert not mock_log.exception.called
+
+    @patch_signed_parameters
+    @patch('lti_consumer.lti.log')
+    def test_default_params(self, mock_log):
+        self._update_xblock_for_signed_parameters()
+        self.xblock.enable_processors = True
+
+        with patch('lti_consumer.lti_consumer.LtiConsumerXBlock.get_settings', return_value={
+            'parameter_processors': [
+                'lti_consumer.tests.unit.test_utils:defaulting_processor',
+            ],
+        }):
+            params = self.lti_consumer.get_signed_lti_parameters()
+            assert '' == params['custom_country']
+            assert 'Lex' == params['custom_name']
+            assert not mock_log.exception.called
+
+    @patch_signed_parameters
+    @patch('lti_consumer.lti.log')
+    def test_default_params_with_error(self, mock_log):
+        self._update_xblock_for_signed_parameters()
+        self.xblock.enable_processors = True
+
+        with patch('lti_consumer.lti_consumer.LtiConsumerXBlock.get_settings', return_value={
+            'parameter_processors': [
+                'lti_consumer.tests.unit.test_utils:faulty_processor',
+            ],
+        }):
+            params = self.lti_consumer.get_signed_lti_parameters()
+            assert 'Lex' == params['custom_name']
+            assert mock_log.exception.called
+
     def test_get_result(self):
         """
         Test `get_result` returns valid json response
@@ -259,7 +295,8 @@ class TestLtiConsumer(TestLtiConsumerXBlock):
 
         with self.assertRaises(LtiError):
             self.lti_consumer.verify_result_headers(request)
-            self.assertTrue(mock_log.called)
+
+        assert mock_log.error.called
 
     @patch('lti_consumer.lti.verify_oauth_body_signature', Mock(return_value=True))
     @patch('lti_consumer.lti_consumer.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's')))
@@ -297,7 +334,8 @@ class TestLtiConsumer(TestLtiConsumerXBlock):
 
         with self.assertRaises(LtiError):
             self.lti_consumer.verify_result_headers(request)
-            self.assertTrue(mock_log.called)
+
+        assert mock_log.error.called
 
     @patch('lti_consumer.lti.verify_oauth_body_signature', Mock(side_effect=ValueError))
     @patch('lti_consumer.lti_consumer.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's')))
@@ -311,4 +349,5 @@ class TestLtiConsumer(TestLtiConsumerXBlock):
 
         with self.assertRaises(LtiError):
             self.lti_consumer.verify_result_headers(request)
-            self.assertTrue(mock_log.called)
+
+        assert mock_log.error.called

lti_consumer/tests/unit/test_lti_consumer.py

@@ -2,17 +2,18 @@
 Unit tests for LtiConsumerXBlock
 """
 
-import unittest
-
 from datetime import timedelta
+import ddt
 from mock import Mock, PropertyMock, patch
 
+from django.test.testcases import TestCase
 from django.utils import timezone
 
 from lti_consumer.tests.unit.test_utils import FAKE_USER_ID, make_xblock, make_request
 
 from lti_consumer.lti_consumer import LtiConsumerXBlock, parse_handler_suffix
 from lti_consumer.exceptions import LtiError
+from lti_consumer.tests.unit import test_utils
 
 
 HTML_PROBLEM_PROGRESS = '<div class="problem-progress">'
@@ -22,7 +23,7 @@ HTML_LAUNCH_NEW_WINDOW_BUTTON = 'btn-lti-new-window'
 HTML_IFRAME = '<iframe'
 
 
-class TestLtiConsumerXBlock(unittest.TestCase):
+class TestLtiConsumerXBlock(TestCase):
     """
     Unit tests for LtiConsumerXBlock.max_score()
     """
@@ -46,6 +47,41 @@ class TestProperties(TestLtiConsumerXBlock):
         """
         self.assertEqual(self.xblock.descriptor, self.xblock)
 
+    def test_workbench_scenarios(self):
+        """
+        Basic tests that `workbench_scenarios()` returns a well formed scenario.
+        """
+        scenarios = self.xblock.workbench_scenarios()
+        assert isinstance(scenarios, list)
+        assert len(scenarios) == 1, 'Keep it to a single scenario with multiple squences.'
+
+        scenario = scenarios[0]
+        assert scenario[0] == 'LTI Consumer XBlock'
+        assert '<lti_consumer' in scenario[1]
+
+    def test_settings(self):
+        """
+        Test that the XBlock is using the SettingsService correctly.
+        """
+        sample_settings_bucket = {
+            'parameter_processors': [],
+        }
+
+        self.xblock.runtime.service = Mock(
+            return_value=Mock(
+                get_settings_bucket=Mock(return_value=sample_settings_bucket)
+            )
+        )
+
+        assert self.xblock.get_settings() == sample_settings_bucket
+
+    def test_settings_without_service(self):
+        """
+        Test that the XBlock can work without the SettingsService.
+        """
+        self.xblock.runtime.service = Mock(return_value=None)
+        assert self.xblock.get_settings() == {}
+
     def test_context_id(self):
         """
         Test `context_id` returns unicode course id
@@ -790,6 +826,59 @@ class TestGetContext(TestLtiConsumerXBlock):
             self.assertIn(key, context)
 
 
+@ddt.ddt
+class TestProcessorSettings(TestLtiConsumerXBlock):
+    """
+    Unit tests for the adding custom LTI parameters.
+    """
+    def test_no_processors_by_default(self):
+        processors = list(self.xblock.get_parameter_processors())
+        assert not processors, 'The processor list should empty by default.'
+
+    def test_enable_processor(self):
+        self.xblock.enable_processors = True
+        with patch('lti_consumer.lti_consumer.LtiConsumerXBlock.get_settings', return_value={
+            'parameter_processors': [
+                'lti_consumer.tests.unit.test_utils:dummy_processor',
+            ],
+        }):
+            processors = list(self.xblock.get_parameter_processors())
+            assert len(processors) == 1, 'One processor should be enabled'
+            assert processors[0] == test_utils.dummy_processor, 'Should load the correct function'
+
+    def test_disabled_processors(self):
+        self.xblock.enable_processors = False
+        with patch('lti_consumer.lti_consumer.LtiConsumerXBlock.get_settings', return_value={
+            'parameter_processors': [
+                'lti_consumer.tests.unit.test_utils:dummy_processor',
+            ],
+        }):
+            processors = list(self.xblock.get_parameter_processors())
+            assert not processors, 'No processor should be enabled'
+
+    @ddt.data({
+        # Bad processor list
+        'parameter_processors': False,
+    }, {
+        # Bad object path, no separator
+        'parameter_processors': [
+            'zzzzz',
+        ],
+    }, {
+        # Non-existent processor
+        'parameter_processors': [
+            'lti_consumer.tests.unit.test_utils:non_existent',
+        ],
+    })
+    @patch('lti_consumer.lti_consumer.log')
+    def test_faulty_configs(self, settings, mock_log):
+        self.xblock.enable_processors = True
+        with patch('lti_consumer.lti_consumer.LtiConsumerXBlock.get_settings', return_value=settings):
+            with self.assertRaises(Exception):
+                list(self.xblock.get_parameter_processors())
+            assert mock_log.exception.called
+
+
 class TestGetModalPositionOffset(TestLtiConsumerXBlock):
     """
     Unit tests for LtiConsumerXBlock._get_modal_position_offset()

lti_consumer/tests/unit/test_utils.py

@@ -3,7 +3,7 @@ Utility functions used within unit tests
 """
 
 from webob import Request
-from mock import Mock
+from mock import patch, Mock, PropertyMock
 
 from xblock.fields import ScopeIds
 from xblock.runtime import KvsFieldData, DictKeyValueStore
@@ -54,3 +54,68 @@ def make_request(body, method='POST'):
     request.body = body.encode('utf-8')
     request.method = method
     return request
+
+
+def patch_signed_parameters(func):
+    """
+    Prepare the patches for the get_signed_lti_parameters function for tests.
+    """
+    func = patch(
+        'lti_consumer.lti.get_oauth_request_signature',
+        Mock(return_value=(
+            'OAuth oauth_nonce="fake_nonce", '
+            'oauth_timestamp="fake_timestamp", oauth_version="fake_version", oauth_signature_method="fake_method", '
+            'oauth_consumer_key="fake_consumer_key", oauth_signature="fake_signature"'
+        ))
+    )(func)
+
+    func = patch(
+        'lti_consumer.lti_consumer.LtiConsumerXBlock.prefixed_custom_parameters',
+        PropertyMock(return_value={u'custom_param_1': 'custom1', u'custom_param_2': 'custom2'})
+    )(func)
+
+    func = patch(
+        'lti_consumer.lti_consumer.LtiConsumerXBlock.lti_provider_key_secret',
+        PropertyMock(return_value=('t', 's'))
+    )(func)
+
+    func = patch(
+        'lti_consumer.lti_consumer.LtiConsumerXBlock.user_id', PropertyMock(return_value=FAKE_USER_ID)
+    )(func)
+
+    return func
+
+
+def dummy_processor(_xblock):
+    """
+    A dummy LTI parameter processor.
+    """
+    return {
+        'custom_author_email': 'author@example.com',
+        'custom_author_country': '',
+    }
+
+
+def defaulting_processor(_xblock):
+    """
+    A dummy LTI parameter processor with default params.
+    """
+    pass
+
+
+defaulting_processor.lti_xblock_default_params = {
+    'custom_name': 'Lex',
+    'custom_country': '',
+}
+
+
+def faulty_processor(_xblock):
+    """
+    A dummy LTI parameter processor with default params that throws an error.
+    """
+    raise Exception()
+
+
+faulty_processor.lti_xblock_default_params = {
+    'custom_name': 'Lex',
+}

setup.py

@@ -22,7 +22,7 @@ def package_data(pkg, roots):
 
 setup(
     name='lti_consumer-xblock',
-    version='1.1.8',
+    version='1.2.0',
     description='This XBlock implements the consumer side of the LTI specification.',
     packages=[
         'lti_consumer',

test_requirements.txt

@@ -2,6 +2,7 @@
 
 django-nose==1.4.4
 astroid==1.3.8  # Pinning to avoid backwards incompatibility issue with pylint/pylint-django
+ddt
 coveralls
 mock
 pep8