feat: decouple xblock and lti config to fix copy-paste

lti_consumer/admin.py

@@ -11,17 +11,32 @@
     LtiAgsScore,
     LtiConfiguration,
     LtiDlContentItem,
+    LtiXBlockConfig,
 )
 
 
+class LtiXBlockConfigInline(admin.TabularInline):
+    model = LtiXBlockConfig
+    extra = 0
+
+
 @admin.register(LtiConfiguration)
 class LtiConfigurationAdmin(admin.ModelAdmin):
     """
     Admin view for LtiConfiguration models.
 
     Makes the location field read-only to avoid issues.
     """
-    readonly_fields = ('location', 'config_id')
+    readonly_fields = ('config_id',)
+    inlines = [LtiXBlockConfigInline]
+
+
+@admin.register(LtiXBlockConfig)
+class LtiXBlockConfigAdmin(admin.ModelAdmin):
+    """
+    Admin view for LtiXBlockConfig models.
+    """
+    list_display = ('location', 'lti_configuration__config_id')
 
 
 @admin.register(CourseAllowPIISharingInLTIFlag)

lti_consumer/api.py

@@ -7,35 +7,67 @@
 
 import json
 
-from opaque_keys.edx.keys import CourseKey
+from opaque_keys.edx.keys import CourseKey, UsageKey
 
 from lti_consumer.lti_1p3.constants import LTI_1P3_ROLE_MAP
-from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem
+
+from .filters import get_external_config_from_filter
+from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem, LtiXBlockConfig
 from .utils import (
+    CONFIG_EXTERNAL,
+    CONFIG_ON_DB,
+    CONFIG_ON_XBLOCK,
     get_cache_key,
     get_data_from_cache,
-    get_lti_1p3_context_types_claim,
-    get_lti_deeplinking_content_url,
+    get_lms_lti_access_token_link,
     get_lms_lti_keyset_link,
     get_lms_lti_launch_link,
-    get_lms_lti_access_token_link,
+    get_lti_1p3_context_types_claim,
+    get_lti_deeplinking_content_url,
+    model_to_dict,
 )
-from .filters import get_external_config_from_filter
 
 
-def _get_or_create_local_lti_config(lti_version, block_location,
-                                    config_store=LtiConfiguration.CONFIG_ON_XBLOCK, external_id=None):
+def _get_or_create_local_lti_xblock_config(
+    lti_version: str,
+    block_location: UsageKey | str,
+    config_id: str | None = None,
+    config_store=CONFIG_ON_XBLOCK,
+    external_id=None,
+):
     """
     Retrieve the LtiConfiguration for the block described by block_location, if one exists. If one does not exist,
-    create an LtiConfiguration with the LtiConfiguration.CONFIG_ON_XBLOCK config_store.
+    create an LtiConfiguration with the CONFIG_ON_XBLOCK config_store.
 
     Treat the lti_version argument as the source of truth for LtiConfiguration.version and override the
     LtiConfiguration.version with lti_version. This allows, for example, for
     the XBlock to be the source of truth for the LTI version, which is a user-centric perspective we've adopted.
     This allows XBlock users to update the LTI version without needing to update the database.
     """
     # The create operation is only performed when there is no existing configuration for the block
-    lti_config, _ = LtiConfiguration.objects.get_or_create(location=block_location)
+    lti_xblock_config, created = LtiXBlockConfig.objects.get_or_create(location=block_location)
+    lti_config: LtiConfiguration | None = None
+    if created:
+        if config_id:
+            lti_config, _ = LtiConfiguration.objects.get_or_create(config_id=config_id)
+        else:
+            lti_config = LtiConfiguration.objects.create()
+        lti_xblock_config.lti_configuration = lti_config
+        lti_xblock_config.save()
+    else:
+        lti_config = lti_xblock_config.lti_configuration
+        if not lti_config or (config_id and lti_config.config_id != config_id):
+            # This is an edge case, when an existing configuration is lost or this block is imported from another
+            # instance, we create a new configuration to avoid no configuration issue.
+            # OR
+            # The config_id was changed as a result of author changing the config_store type.
+            # In this case we create a copy of the existing configuration with the new config_id.
+            lti_config, _ = LtiConfiguration.objects.get_or_create(
+                config_id=config_id,
+                defaults=model_to_dict(lti_config, ['id', 'config_id', 'location', 'external_config']),
+            )
+            lti_xblock_config.lti_configuration = lti_config
+            lti_xblock_config.save()
 
     lti_config.config_store = config_store
     lti_config.external_id = external_id
@@ -45,79 +77,93 @@ def _get_or_create_local_lti_config(lti_version, block_location,
 
     lti_config.save()
 
-    return lti_config
+    return lti_xblock_config
 
 
-def _get_config_by_config_id(config_id):
+def _get_config_by_config_id(config_id) -> LtiConfiguration:
     """
     Gets the LTI config by its UUID config ID
     """
     return LtiConfiguration.objects.get(config_id=config_id)
 
 
+def try_get_config_by_id(config_id) -> LtiConfiguration | None:
+    """
+    Tries to get the LTI config by its UUID config ID
+    """
+    try:
+        return _get_config_by_config_id(config_id)
+    except LtiConfiguration.DoesNotExist:
+        return None
+
+
 def _get_lti_config_for_block(block):
     """
-    Retrieves or creates a LTI Configuration for a block.
+    Retrieves or creates a LTI Xblock Configuration for a block.
 
-    This wraps around `_get_or_create_local_lti_config` and handles the block and modulestore
+    This wraps around `_get_or_create_local_lti_xblock_config` and handles the block and modulestore
     bits of configuration.
     """
     if block.config_type == 'database':
-        lti_config = _get_or_create_local_lti_config(
+        lti_xblock_config = _get_or_create_local_lti_xblock_config(
             block.lti_version,
             block.scope_ids.usage_id,
-            LtiConfiguration.CONFIG_ON_DB,
+            block.config_id,
+            CONFIG_ON_DB,
         )
     elif block.config_type == 'external':
         config = get_external_config_from_filter(
             {"course_key": block.scope_ids.usage_id.context_key},
             block.external_config
         )
-        lti_config = _get_or_create_local_lti_config(
+        lti_xblock_config = _get_or_create_local_lti_xblock_config(
             config.get("version"),
             block.scope_ids.usage_id,
-            LtiConfiguration.CONFIG_EXTERNAL,
+            block.config_id,
+            CONFIG_EXTERNAL,
             external_id=block.external_config,
         )
     else:
-        lti_config = _get_or_create_local_lti_config(
+        lti_xblock_config = _get_or_create_local_lti_xblock_config(
             block.lti_version,
             block.scope_ids.usage_id,
-            LtiConfiguration.CONFIG_ON_XBLOCK,
+            block.config_id,
+            CONFIG_ON_XBLOCK,
         )
-    return lti_config
+    return lti_xblock_config
 
 
-def config_id_for_block(block):
+def config_for_block(block):
     """
     Returns the externally facing config_id of the LTI Configuration used by this block,
     creating one if required. That ID is suitable for use in launch data or get_consumer.
     """
-    config = _get_lti_config_for_block(block)
-    return config.config_id
+    xblock_config = _get_lti_config_for_block(block)
+    return xblock_config
 
 
-def get_lti_consumer(config_id):
+def get_lti_consumer(config_id: str, location: UsageKey | None = None):
     """
-    Retrieves an LTI Consumer instance for a given configuration.
+    Retrieves an LTI Consumer instance for a given location.
 
     Returns an instance of LtiConsumer1p1 or LtiConsumer1p3 depending
     on the configuration.
     """
     # Return an instance of LTI 1.1 or 1.3 consumer, depending
     # on the configuration stored in the model.
-    return _get_config_by_config_id(config_id).get_lti_consumer()
+    return _get_config_by_config_id(config_id).get_lti_consumer(location)
 
 
 def get_lti_1p3_launch_info(
     launch_data,
+    location: UsageKey | None = None,
 ):
     """
     Retrieves the Client ID, Keyset URL and other urls used to configure a LTI tool.
     """
     # Retrieve LTI Config and consumer
     lti_config = _get_config_by_config_id(launch_data.config_id)
-    lti_consumer = lti_config.get_lti_consumer()
+    lti_consumer = lti_config.get_lti_consumer(location)
 
     # Check if deep Linking is available, if so, add some extra context:
     # Deep linking launch URL, and if deep linking is already configured
@@ -146,7 +192,7 @@ def get_lti_1p3_launch_info(
 
     # Display LTI launch information from external configuration.
     # if an external configuration is being used.
-    if lti_config.config_store == lti_config.CONFIG_EXTERNAL:
+    if lti_config.config_store == CONFIG_EXTERNAL:
         external_config = get_external_config_from_filter({}, lti_config.external_id)
         config_id = lti_config.external_id.replace(':', '/')
         client_id = external_config.get('lti_1p3_client_id')
@@ -167,14 +213,15 @@ def get_lti_1p3_launch_info(
 
 def get_lti_1p3_launch_start_url(
     launch_data,
+    location: UsageKey | None = None,
     deep_link_launch=False,
     dl_content_id=None,
 ):
     """
     Computes and retrieves the LTI URL that starts the OIDC flow.
     """
     # Retrieve LTI consumer
-    lti_consumer = get_lti_consumer(launch_data.config_id)
+    lti_consumer = get_lti_consumer(launch_data.config_id, location)
 
     # Include a message hint in the launch_data depending on LTI launch type
     # Case 1: Performs Deep Linking configuration flow. Triggered by staff users to
@@ -192,6 +239,7 @@ def get_lti_1p3_launch_start_url(
 
 def get_lti_1p3_content_url(
     launch_data,
+    location: UsageKey | None = None,
 ):
     """
     Computes and returns which URL the LTI consumer should launch to.
@@ -211,13 +259,14 @@ def get_lti_1p3_content_url(
 
     # If there's no content items, return normal LTI launch URL
     if not content_items.count():
-        return get_lti_1p3_launch_start_url(launch_data)
+        return get_lti_1p3_launch_start_url(launch_data, location)
 
     # If there's a single `ltiResourceLink` content, return the launch
     # url for that specific deep link
     if content_items.count() == 1 and content_items.get().content_type == LtiDlContentItem.LTI_RESOURCE_LINK:
         return get_lti_1p3_launch_start_url(
             launch_data,
+            location,
             dl_content_id=content_items.get().id,
         )
 

lti_consumer/lti_1p3/extensions/rest_framework/authentication.py

@@ -5,10 +5,9 @@
 """
 from django.contrib.auth.models import AnonymousUser
 from django.utils.translation import gettext as _
-from rest_framework import authentication
-from rest_framework import exceptions
+from rest_framework import authentication, exceptions
 
-from lti_consumer.models import LtiConfiguration
+from lti_consumer.models import LtiXBlockConfig
 
 
 class Lti1p3ApiAuthentication(authentication.BaseAuthentication):
@@ -51,8 +50,8 @@ def authenticate(self, request):
 
         # Retrieve LTI configuration or fail if it doesn't exist
         try:
-            lti_configuration = LtiConfiguration.objects.get(pk=lti_config_id)
-            lti_consumer = lti_configuration.get_lti_consumer()
+            lti_xblock_config = LtiXBlockConfig.objects.get(pk=lti_config_id)
+            lti_consumer = lti_xblock_config.get_lti_consumer()
         except Exception as err:
             msg = _('LTI configuration not found.')
             raise exceptions.AuthenticationFailed(msg) from err
@@ -72,7 +71,7 @@ def authenticate(self, request):
         # With the LTI Configuration and consumer attached to the request
         # the views and permissions classes can make use of the
         # current LTI context to retrieve settings and decode the token passed.
-        request.lti_configuration = lti_configuration
+        request.lti_xblock_config = lti_xblock_config
         request.lti_consumer = lti_consumer
 
         # This isn't tied to any authentication backend on Django (it's just

lti_consumer/lti_1p3/extensions/rest_framework/serializers.py

@@ -80,7 +80,7 @@ def get_id(self, obj):
         return reverse(
             'lti_consumer:lti-ags-view-detail',
             kwargs={
-                'lti_config_id': obj.lti_configuration.id,
+                'lti_config_id': obj.lti_xblock_config.id,
                 'pk': obj.pk
             },
             request=request,