[BD-24] [TNL-7607] BB-3072: Move key and client management to model l…

…evel. (#113)

* Move LTI 1.3 Key management to model

This:
- Removes the need to load the modulestore on every public keyset endpoint call.
- Simplifies the block structure and parent method overrides.
- Removes private key, client id and related parameters from XBlock fields

It also includes a migration from the data stored in the block to the model.

* Cleanup unused test helpers

* Version bump

* Addressing review comments

lti_consumer/api.py

@@ -5,6 +5,11 @@
 return plaintext to allow easy testing/mocking.
 """
 from .models import LtiConfiguration
+from .utils import (
+    get_lms_lti_keyset_link,
+    get_lms_lti_launch_link,
+    get_lms_lti_access_token_link,
+)
 
 
 def _get_or_create_local_lti_config(lti_version, block_location):
@@ -52,3 +57,32 @@ def get_lti_consumer(config_id=None, block=None):
     # Return an instance of LTI 1.1 or 1.3 consumer, depending
     # on the configuration stored in the model.
     return lti_config.get_lti_consumer()
+
+
+def get_lti_1p3_launch_info(config_id=None, block=None):
+    """
+    Retrieves the Client ID, Keyset URL and other urls used to configure a LTI tool.
+    """
+    if not config_id and not block:
+        raise Exception("You need to pass either a config_id or a block.")
+
+    if config_id:
+        lti_config = LtiConfiguration.objects.get(pk=config_id)
+    elif block:
+        lti_config = _get_or_create_local_lti_config(
+            block.lti_version,
+            block.location,
+        )
+        # Since the block was passed, preload it to avoid
+        # having to instance the modulestore and fetch it again.
+        lti_config.block = block
+
+    launch_info = {
+        'client_id': lti_config.lti_1p3_client_id,
+        'keyset_url': get_lms_lti_keyset_link(lti_config.location),
+        'deployment_id': '1',
+        'oidc_callback': get_lms_lti_launch_link(),
+        'token_url': get_lms_lti_access_token_link(lti_config.location),
+    }
+
+    return launch_info

lti_consumer/lti_1p3/tests/test_consumer.py

@@ -572,54 +572,6 @@ def setUp(self):
             tool_key=RSA_KEY
         )
 
-    def _setup_lti_user(self):
-        """
-        Set up a minimal LTI message with only required parameters.
-
-        Currently, the only required parameters are the user data,
-        but using a helper function to keep the usage consistent accross
-        all tests.
-        """
-        self.lti_consumer.set_user_data(
-            user_id="1",
-            role="student",
-        )
-
-    def _get_lti_message(
-            self,
-            preflight_response=None,
-            resource_link="link"
-    ):
-        """
-        Retrieves a base LTI message with fixed test parameters.
-
-        This function has valid default values, so it can be used to test custom
-        parameters, but allows overriding them.
-        """
-        if preflight_response is None:
-            preflight_response = {
-                "client_id": CLIENT_ID,
-                "redirect_uri": LAUNCH_URL,
-                "nonce": NONCE,
-                "state": STATE
-            }
-
-        return self.lti_consumer.generate_launch_request(
-            preflight_response,
-            resource_link
-        )
-
-    def _decode_token(self, token):
-        """
-        Checks for a valid signarute and decodes JWT signed LTI message
-
-        This also tests the public keyset function.
-        """
-        public_keyset = self.lti_consumer.get_public_keyset()
-        key_set = load_jwks(json.dumps(public_keyset))
-
-        return JWS().verify_compact(token, keys=key_set)
-
     def test_no_ags_returns_failure(self):
         """
         Test that when LTI-AGS isn't configured, the class yields an error.

lti_consumer/lti_xblock.py

@@ -53,15 +53,13 @@
 import logging
 import re
 import urllib.parse
-import uuid
 from collections import namedtuple
 from importlib import import_module
 
 import bleach
 from django.utils import timezone
 from web_fragments.fragment import Fragment
 
-from Cryptodome.PublicKey import RSA
 from webob import Response
 from xblock.core import List, Scope, String, XBlock
 from xblock.fields import Boolean, Float, Integer
@@ -85,8 +83,6 @@
 from .outcomes import OutcomeService
 from .utils import (
     _,
-    get_lms_lti_access_token_link,
-    get_lms_lti_keyset_link,
     get_lms_lti_launch_link,
     lti_1p3_enabled,
 )
@@ -304,26 +300,15 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
             "Basic Outcomes requests.</b>"
         ),
     )
-    # Client ID and block key
+    # DEPRECATED - These variables were moved to the LtiConfiguration Model
     lti_1p3_client_id = String(
-        display_name=_("LTI 1.3 Block Client ID"),
+        display_name=_("LTI 1.3 Block Client ID - DEPRECATED"),
         default='',
         scope=Scope.settings,
-        help=_(
-            "Enter the LTI ID for the external LTI provider. "
-            "This value must be the same LTI ID that you entered in the "
-            "LTI Passports setting on the Advanced Settings page."
-            "<br />See the {docs_anchor_open}edX LTI documentation{anchor_close} for more details on this setting."
-        ).format(
-            docs_anchor_open=DOCS_ANCHOR_TAG_OPEN,
-            anchor_close="</a>"
-        ),
+        help=_("DEPRECATED - This is now stored in the LtiConfiguration model."),
     )
-    # This key isn't editable, and should be regenerated
-    # for every block created (and not be carried over)
-    # This isn't what happens right now though
     lti_1p3_block_key = String(
-        display_name=_("LTI 1.3 Block Key"),
+        display_name=_("LTI 1.3 Block Key - DEPRECATED"),
         default='',
         scope=Scope.settings
     )
@@ -892,28 +877,6 @@ def studio_view(self, context):
 
         return fragment
 
-    def clean_studio_edits(self, data):
-        """
-        This is a handler to set hidden Studio variables for LTI 1.3.
-
-        These variables shouldn't be editable by the user, but need
-        to be automatically generated for each instance:
-        * lti_1p3_client_id: random uuid (requirement: must be unique)
-        * lti_1p3_block_key: PEM export of 2048-bit RSA key.
-
-        TODO: Remove this once the XBlock Fields API support using
-        a default computed value.
-        """
-        if data.get('lti_version') == 'lti_1p3':
-            # Check if values already exist before populating
-            # to avoid overwriting these keys on every edit.
-            if not self.lti_1p3_client_id:
-                data['lti_1p3_client_id'] = str(uuid.uuid4())
-            if not self.lti_1p3_block_key:
-                data['lti_1p3_block_key'] = RSA.generate(2048).export_key('PEM')
-
-        return super(LtiConsumerXBlock, self).clean_studio_edits(data)
-
     def author_view(self, context):
         """
         XBlock author view of this component.
@@ -925,16 +888,17 @@ def author_view(self, context):
         if self.lti_version == "lti_1p1":
             return self.student_view(context)
 
+        # Runtime import since this will only run in the
+        # Open edX LMS/Studio environments.
+        # pylint: disable=import-outside-toplevel
+        from lti_consumer.api import get_lti_1p3_launch_info
+
+        # Retrieve LTI 1.3 Launch information
+        context.update(get_lti_1p3_launch_info(block=self))
+
+        # Render template
         fragment = Fragment()
         loader = ResourceLoader(__name__)
-        context = {
-            "client": self.lti_1p3_client_id,
-            "deployment_id": "1",
-            "keyset_url": get_lms_lti_keyset_link(self.location),  # pylint: disable=no-member
-            "oidc_callback": get_lms_lti_launch_link(),
-            "token_url": get_lms_lti_access_token_link(self.location),  # pylint: disable=no-member
-            "launch_url": self.lti_1p3_launch_url,
-        }
         fragment.add_content(loader.render_mako_template('/templates/html/lti_1p3_studio.html', context))
         fragment.add_css(loader.load_unicode('static/css/student.css'))
         fragment.add_javascript(loader.load_unicode('static/js/xblock_lti_consumer.js'))
@@ -1034,6 +998,7 @@ def lti_1p3_launch_handler(self, request, suffix=''):
             webob.response: HTML LTI launch form
         """
         lti_consumer = self._get_lti_consumer()
+
         context = lti_consumer.prepare_preflight_url(
             callback_url=get_lms_lti_launch_link(),
             hint=str(self.location),  # pylint: disable=no-member
@@ -1105,19 +1070,6 @@ def lti_1p3_launch_callback(self, request, suffix=''):
             template = loader.render_mako_template('/templates/html/lti_1p3_launch_error.html', context)
             return Response(template, status=400, content_type='text/html')
 
-    @XBlock.handler
-    def public_keyset_endpoint(self, request, suffix=''):
-        """
-        XBlock handler for launching the LTI provider.
-        """
-        if self.lti_version == "lti_1p3":
-            return Response(
-                json_body=self._get_lti_consumer().get_public_keyset(),
-                content_type='application/json',
-                content_disposition='attachment; filename=keyset.json'
-            )
-        return Response(status=404)
-
     @XBlock.handler
     def lti_1p3_access_token(self, request, suffix=''):
         """

lti_consumer/migrations/0004_keyset_mgmt_to_model.py

@@ -0,0 +1,34 @@
+# Generated by Django 2.2.16 on 2020-10-24 11:47
+from django.db import migrations, models
+
+import lti_consumer
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('lti_consumer', '0003_ltiagsscore'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='lticonfiguration',
+            name='lti_1p3_internal_private_key',
+            field=models.TextField(blank=True, help_text="Platform's generated Private key. Keep this value secret."),
+        ),
+        migrations.AddField(
+            model_name='lticonfiguration',
+            name='lti_1p3_internal_private_key_id',
+            field=models.CharField(blank=True, help_text="Platform's generated Private key ID", max_length=255),
+        ),
+        migrations.AddField(
+            model_name='lticonfiguration',
+            name='lti_1p3_internal_public_jwk',
+            field=models.TextField(blank=True, help_text="Platform's generated JWK keyset."),
+        ),
+        migrations.AddField(
+            model_name='lticonfiguration',
+            name='lti_1p3_client_id',
+            field=models.CharField(default=lti_consumer.models.generate_client_id, help_text='Client ID used by LTI tool', max_length=255),
+        ),
+    ]

lti_consumer/migrations/0005_migrate_keyset_to_model.py

@@ -0,0 +1,62 @@
+# Generated by Django 2.2.16 on 2020-10-24 11:47
+
+from django.db import migrations, models, transaction
+
+
+def _load_block(location):
+    """
+    Loads block from modulestore.
+    """
+    from xmodule.modulestore.django import modulestore
+    return modulestore().get_item(location)
+
+
+def forwards_func(apps, schema_editor):
+    """
+    Forward migration - copy client ID and save it in Model
+    """
+    LtiConfiguration = apps.get_model("lti_consumer", "LtiConfiguration")
+    db_alias = schema_editor.connection.alias
+
+    lti_configs = LtiConfiguration.objects.select_for_update().filter(
+        version="lti_1p3",
+    )
+    with transaction.atomic():
+        for lti_config in lti_configs:
+            block = _load_block(lti_config.location)
+
+            # If client_id exists, move it to model
+            if hasattr(block, 'lti_1p3_client_id'):
+                lti_config.lti_1p3_client_id = block.lti_1p3_client_id
+
+                # If key exists, move it to model. Set kid as client_id, respecting
+                # old implementation
+                if hasattr(block, 'lti_1p3_block_key'):
+                    lti_config.lti_1p3_internal_private_key_id = block.lti_1p3_client_id
+                    lti_config.lti_1p3_internal_private_key = block.lti_1p3_block_key
+
+            # Save if something changed
+            lti_config.save()
+
+
+def reverse_func(apps, schema_editor):
+    """
+    Reverse migration - No op.
+
+    Leave XBlock to regenerate Client ID and secrets.
+    """
+    pass
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('lti_consumer', '0004_keyset_mgmt_to_model'),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            forwards_func,
+            reverse_func
+        ),
+    ]