From fc401bd7d00ecdb1e91deb9429443c97f99c3068 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 13 Feb 2026 11:25:09 +0530 Subject: [PATCH 01/14] refactor: decouple config_id and location --- lti_consumer/api.py | 16 +- .../rest_framework/authentication.py | 11 +- .../extensions/rest_framework/serializers.py | 2 +- lti_consumer/lti_xblock.py | 11 + .../migrations/0020_ltixblockconfig.py | 33 +++ .../0021_migrate_config_id_to_blocks.py | 49 +++++ lti_consumer/models.py | 194 +++++++++--------- lti_consumer/plugin/compat.py | 19 ++ lti_consumer/plugin/views.py | 20 +- 9 files changed, 232 insertions(+), 123 deletions(-) create mode 100644 lti_consumer/migrations/0020_ltixblockconfig.py create mode 100644 lti_consumer/migrations/0021_migrate_config_id_to_blocks.py diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 6b278e7c..db4381c9 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -23,8 +23,12 @@ 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_config( + lti_version, + config_id, + config_store=LtiConfiguration.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. @@ -35,7 +39,7 @@ def _get_or_create_local_lti_config(lti_version, block_location, 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_config, _ = LtiConfiguration.objects.get_or_create(config_id=config_id) lti_config.config_store = config_store lti_config.external_id = external_id @@ -65,7 +69,7 @@ def _get_lti_config_for_block(block): if block.config_type == 'database': lti_config = _get_or_create_local_lti_config( block.lti_version, - block.scope_ids.usage_id, + block.config_id, LtiConfiguration.CONFIG_ON_DB, ) elif block.config_type == 'external': @@ -75,14 +79,14 @@ def _get_lti_config_for_block(block): ) lti_config = _get_or_create_local_lti_config( config.get("version"), - block.scope_ids.usage_id, + block.config_id, LtiConfiguration.CONFIG_EXTERNAL, external_id=block.external_config, ) else: lti_config = _get_or_create_local_lti_config( block.lti_version, - block.scope_ids.usage_id, + block.config_id, LtiConfiguration.CONFIG_ON_XBLOCK, ) return lti_config diff --git a/lti_consumer/lti_1p3/extensions/rest_framework/authentication.py b/lti_consumer/lti_1p3/extensions/rest_framework/authentication.py index ff4e8f31..63fe49b7 100644 --- a/lti_consumer/lti_1p3/extensions/rest_framework/authentication.py +++ b/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 diff --git a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py index 953cee40..677dac15 100644 --- a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py +++ b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py @@ -76,7 +76,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, diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index f0364d42..ecbecc3a 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -49,6 +49,7 @@ Numeric grades between 0 and 1 and text + basic HTML feedback comments are supported, via GET / PUT / DELETE HTTP methods respectively """ +import uuid import logging import re @@ -288,6 +289,12 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): "'Reusable Configuration' and enter it in the text field below." ) ) + config_id = String( + display_name=_("Configuration ID"), + scope=Scope.settings, + default="", + help=_("Config ID for a reusable configuration.") + ) lti_version = String( display_name=_("LTI Version"), @@ -708,6 +715,10 @@ def validate(self): validation.add(ValidationMessage(ValidationMessage.WARNING, str( _("The specified LTI ID is not configured in this course's Advanced Settings.") ))) + if self.lti_version == "lti_1p3" and not self.config_id: + self.config_id = str(uuid.uuid4()) + # __AUTO_GENERATED_PRINT_VAR_START__ + print(f"""======================================= LtiConsumerXBlock#validate config_id: {self.config_id}""") # __AUTO_GENERATED_PRINT_VAR_END__ return validation def get_settings(self): diff --git a/lti_consumer/migrations/0020_ltixblockconfig.py b/lti_consumer/migrations/0020_ltixblockconfig.py new file mode 100644 index 00000000..0030772a --- /dev/null +++ b/lti_consumer/migrations/0020_ltixblockconfig.py @@ -0,0 +1,33 @@ +# Generated by Django 5.2.11 on 2026-02-12 10:40 + +import django.db.models.deletion +import opaque_keys.edx.django.models +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('lti_consumer', '0019_mariadb_uuid_conversion'), + ] + + operations = [ + migrations.CreateModel( + name='LtiXBlockConfig', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ( + 'location', + opaque_keys.edx.django.models.UsageKeyField(blank=True, db_index=True, max_length=255, null=True), + ), + ( + 'lti_configuration', + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to='lti_consumer.lticonfiguration', + ), + ), + ], + ), + ] diff --git a/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py new file mode 100644 index 00000000..8c7bbbc1 --- /dev/null +++ b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py @@ -0,0 +1,49 @@ +# Generated migration for copying config_id into modulestore from database (Django 5.2) +""" +This migration will copy config_id from LtiConsumer database to LtiConsumerXBlock. + +This will help us link xblocks with LtiConsumer database rows without relying on the location or usage_key of xblocks. +""" +from django.db import migrations + + +def copy_config_id(apps, schema_editor): + """Copy config_id from LtiConsumer to LtiConsumerXBlock.""" + from lti_consumer.plugin.compat import load_enough_xblock, save_xblock + + LtiConfiguration = apps.get_model("lti_consumer", "LtiConfiguration") + LtiXBlockConfig = apps.get_model("lti_consumer", "LtiXBlockConfig") + + for configuration in LtiConfiguration.objects.all(): + LtiXBlockConfig.objects.update_or_create( + location=configuration.location, + defaults={ + "lti_configuration": configuration, + } + ) + try: + blockstore = load_enough_xblock(configuration.location) + blockstore.config_id = str(configuration.config_id) + blockstore.save() + save_xblock(blockstore) + except Exception as e: + print(f"Failed to copy config_id for configuration {configuration}: {e}") + + +def backwards(apps, schema_editor): + """Reverse the migration only for MariaDB databases.""" + pass + + +class Migration(migrations.Migration): + + dependencies = [ + ('lti_consumer', '0020_ltixblockconfig'), + ] + + operations = [ + migrations.RunPython( + code=copy_config_id, + reverse_code=backwards, + ), + ] diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 5a07a724..6ebe37ed 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -1,40 +1,39 @@ """ LTI configuration and linking models. """ +import json import logging import uuid -import json -from django.db import models -from django.core.validators import MinValueValidator -from django.core.exceptions import ValidationError - -from jsonfield import JSONField -from Cryptodome.PublicKey import RSA -from ccx_keys.locator import CCXBlockUsageLocator -from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField -from opaque_keys.edx.keys import CourseKey from config_models.models import ConfigurationModel +from Cryptodome.PublicKey import RSA +from django.core.exceptions import ValidationError +from django.core.validators import MinValueValidator +from django.db import models from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ from edx_django_utils.monitoring import function_trace +from jsonfield import JSONField +from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField +from opaque_keys.edx.keys import CourseKey, UsageKey + from lti_consumer.filters import get_external_config_from_filter # LTI 1.1 from lti_consumer.lti_1p1.consumer import LtiConsumer1p1 + # LTI 1.3 from lti_consumer.lti_1p3.consumer import LtiAdvantageConsumer, LtiProctoringConsumer from lti_consumer.lti_1p3.key_handlers import PlatformKeyHandler from lti_consumer.plugin import compat from lti_consumer.utils import ( - get_lti_api_base, + EXTERNAL_ID_REGEX, + choose_lti_1p3_redirect_uris, + external_multiple_launch_urls_enabled, get_lti_ags_lineitems_url, + get_lti_api_base, get_lti_deeplinking_response_url, get_lti_nrps_context_membership_url, - choose_lti_1p3_redirect_uris, - model_to_dict, - EXTERNAL_ID_REGEX, - external_multiple_launch_urls_enabled, ) log = logging.getLogger(__name__) @@ -251,10 +250,6 @@ class LtiConfiguration(models.Model): ) def clean(self): - if self.config_store == self.CONFIG_ON_XBLOCK and self.location is None: - raise ValidationError({ - "config_store": _("LTI Configuration stores on XBlock needs a block location set."), - }) if self.config_store == self.CONFIG_EXTERNAL and not EXTERNAL_ID_REGEX.match(str(self.external_id)): raise ValidationError({ "config_store": _( @@ -275,49 +270,6 @@ def clean(self): "config_store": _("CONFIG_ON_XBLOCK and CONFIG_EXTERNAL are not supported for " "LTI 1.3 Proctoring Services."), }) - try: - consumer = self.get_lti_consumer() - except NotImplementedError: - consumer = None - if consumer is None: - raise ValidationError(_("Invalid LTI configuration.")) - - @function_trace('lti_consumer.models.LtiConfiguration.sync_configurations') - def sync_configurations(self): - """Syncronize main/children configurations. - - This method will synchronize the field values of main/children configurations. - On a configuration with a CCX location, it will copy the values from the main course configuration, - otherwise, it will try to query any children configuration and update their fields using - the current configuration values. - """ - EXCLUDED_FIELDS = ['id', 'config_id', 'location', 'external_config'] - - if isinstance(self.location, CCXBlockUsageLocator): - # Query main configuration using main location. - main_config = LtiConfiguration.objects.filter(location=self.location.to_block_locator()).first() - # Copy fields from main configuration. - for field in model_to_dict(main_config, EXCLUDED_FIELDS).items(): - setattr(self, field[0], field[1]) - else: - try: - # Query child CCX configurations using location block ID and CCX namespace. - child_configs = LtiConfiguration.objects.filter( - location__endswith=str(self.location).split('@')[-1], - ).filter( - location__startswith=CCXBlockUsageLocator.CANONICAL_NAMESPACE, - ).exclude(id=self.pk) - # Copy fields to child CCX configurations. - child_configs.update(**model_to_dict(self, EXCLUDED_FIELDS)) - except IndexError: - log.exception( - f'Failed to query children CCX LTI configurations: ' - f'Failed to parse main LTI configuration location: {self.location}', - ) - - def save(self, *args, **kwargs): - self.sync_configurations() - super().save(*args, **kwargs) def _generate_lti_1p3_keys_if_missing(self): """ @@ -382,13 +334,13 @@ def external_config(self): """ return get_external_config_from_filter({}, self.external_id) - def _get_lti_1p1_consumer(self): + def _get_lti_1p1_consumer(self, location: UsageKey): """ Return a class of LTI 1.1 consumer. """ # If LTI configuration is stored in the XBlock. if self.config_store == self.CONFIG_ON_XBLOCK: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) key, secret = block.lti_provider_key_secret launch_url = block.launch_url elif self.config_store == self.CONFIG_EXTERNAL: @@ -402,7 +354,7 @@ def _get_lti_1p1_consumer(self): return LtiConsumer1p1(launch_url, key, secret) - def get_lti_advantage_ags_mode(self): + def get_lti_advantage_ags_mode(self, location: UsageKey): """ Return LTI 1.3 Advantage Assignment and Grade Services mode. """ @@ -411,10 +363,10 @@ def get_lti_advantage_ags_mode(self): elif self.config_store == self.CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_ags_mode') else: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) return block.lti_advantage_ags_mode - def get_lti_advantage_deep_linking_enabled(self): + def get_lti_advantage_deep_linking_enabled(self, location: UsageKey): """ Return whether LTI 1.3 Advantage Deep Linking is enabled. """ @@ -423,10 +375,10 @@ def get_lti_advantage_deep_linking_enabled(self): elif self.config_store == self.CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_deep_linking_enabled') else: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) return block.lti_advantage_deep_linking_enabled - def get_lti_advantage_deep_linking_launch_url(self): + def get_lti_advantage_deep_linking_launch_url(self, location: UsageKey): """ Return the LTI 1.3 Advantage Deep Linking launch URL. """ @@ -435,10 +387,10 @@ def get_lti_advantage_deep_linking_launch_url(self): elif self.config_store == self.CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_deep_linking_launch_url') else: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) return block.lti_advantage_deep_linking_launch_url - def get_lti_advantage_nrps_enabled(self): + def get_lti_advantage_nrps_enabled(self, location: UsageKey): """ Return whether LTI 1.3 Advantage Names and Role Provisioning Services is enabled. """ @@ -447,16 +399,16 @@ def get_lti_advantage_nrps_enabled(self): elif self.config_store == self.CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_enable_nrps') else: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) return block.lti_1p3_enable_nrps - def _setup_lti_1p3_ags(self, consumer): + def _setup_lti_1p3_ags(self, consumer, location: UsageKey): """ Set up LTI 1.3 Advantage Assigment and Grades Services. """ try: - lti_advantage_ags_mode = self.get_lti_advantage_ags_mode() + lti_advantage_ags_mode = self.get_lti_advantage_ags_mode(location) except NotImplementedError as exc: log.exception("Error setting up LTI 1.3 Advantage Assignment and Grade Services: %s", exc) return @@ -472,13 +424,13 @@ def _setup_lti_1p3_ags(self, consumer): # and manage lineitems using the AGS endpoints. if not lineitem and lti_advantage_ags_mode == self.LTI_ADVANTAGE_AGS_DECLARATIVE: try: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) except ValueError: # There is no location to load the block block = None if block: default_values = { - 'resource_id': self.location, + 'resource_id': location, 'score_maximum': block.weight, 'label': block.display_name, } @@ -490,15 +442,15 @@ def _setup_lti_1p3_ags(self, consumer): else: # TODO find a way to make these defaults more sensible default_values = { - 'resource_id': self.location, + 'resource_id': location, 'score_maximum': 100, - 'label': 'LTI Consumer at ' + str(self.location) + 'label': 'LTI Consumer at ' + str(location) } # create LineItem if there is none for current lti configuration lineitem = LtiAgsLineItem.objects.create( lti_configuration=self, - resource_link_id=self.location, + resource_link_id=location, **default_values ) @@ -510,30 +462,30 @@ def _setup_lti_1p3_ags(self, consumer): ) ) - def _setup_lti_1p3_deep_linking(self, consumer): + def _setup_lti_1p3_deep_linking(self, consumer, location: UsageKey): """ Set up LTI 1.3 Advantage Deep Linking. """ try: - if self.get_lti_advantage_deep_linking_enabled(): + if self.get_lti_advantage_deep_linking_enabled(location): consumer.enable_deep_linking( - self.get_lti_advantage_deep_linking_launch_url(), + self.get_lti_advantage_deep_linking_launch_url(location), get_lti_deeplinking_response_url(self.id), ) except NotImplementedError as exc: log.exception("Error setting up LTI 1.3 Advantage Deep Linking: %s", exc) - def _setup_lti_1p3_nrps(self, consumer): + def _setup_lti_1p3_nrps(self, consumer, location: UsageKey): """ Set up LTI 1.3 Advantage Names and Role Provisioning Services. """ try: - if self.get_lti_advantage_nrps_enabled(): + if self.get_lti_advantage_nrps_enabled(location): consumer.enable_nrps(get_lti_nrps_context_membership_url(self.id)) except NotImplementedError as exc: log.exception("Error setting up LTI 1.3 Advantage Names and Role Provisioning Services: %s", exc) - def _get_lti_1p3_consumer(self): + def _get_lti_1p3_consumer(self, location: UsageKey): """ Return a class of LTI 1.3 consumer. @@ -549,7 +501,7 @@ def _get_lti_1p3_consumer(self): consumer_class = LtiProctoringConsumer if self.config_store == self.CONFIG_ON_XBLOCK: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) consumer = consumer_class( iss=get_lti_api_base(), @@ -563,7 +515,7 @@ def _get_lti_1p3_consumer(self): rsa_key=self.lti_1p3_private_key, rsa_key_id=self.lti_1p3_private_key_id, # Registered redirect uris - redirect_uris=self.get_lti_1p3_redirect_uris(), + redirect_uris=self.get_lti_1p3_redirect_uris(location), # LTI 1.3 Tool key/keyset url tool_key=block.lti_1p3_tool_public_key, tool_keyset_url=block.lti_1p3_tool_keyset_url, @@ -581,7 +533,7 @@ def _get_lti_1p3_consumer(self): rsa_key=self.lti_1p3_private_key, rsa_key_id=self.lti_1p3_private_key_id, # Registered redirect uris - redirect_uris=self.get_lti_1p3_redirect_uris(), + redirect_uris=self.get_lti_1p3_redirect_uris(location), # LTI 1.3 Tool key/keyset url tool_key=self.lti_1p3_tool_public_key, tool_keyset_url=self.lti_1p3_tool_keyset_url, @@ -589,8 +541,8 @@ def _get_lti_1p3_consumer(self): elif self.config_store == self.CONFIG_EXTERNAL: lti_launch_url = self.external_config.get('lti_1p3_launch_url') - if external_multiple_launch_urls_enabled(self.location.course_key): - block = compat.load_enough_xblock(self.location) + if external_multiple_launch_urls_enabled(location.course_key): + block = compat.load_enough_xblock(location) lti_launch_url = block.lti_1p3_launch_url or lti_launch_url @@ -603,7 +555,7 @@ def _get_lti_1p3_consumer(self): rsa_key=self.external_config.get('lti_1p3_private_key'), rsa_key_id=self.external_config.get('lti_1p3_private_key_id'), # Registered redirect uris - redirect_uris=self.get_lti_1p3_redirect_uris(), + redirect_uris=self.get_lti_1p3_redirect_uris(location), tool_key=self.external_config.get('lti_1p3_tool_public_key'), tool_keyset_url=self.external_config.get('lti_1p3_tool_keyset_url'), ) @@ -613,23 +565,23 @@ def _get_lti_1p3_consumer(self): raise NotImplementedError if isinstance(consumer, LtiAdvantageConsumer): - self._setup_lti_1p3_ags(consumer) - self._setup_lti_1p3_deep_linking(consumer) - self._setup_lti_1p3_nrps(consumer) + self._setup_lti_1p3_ags(consumer, location) + self._setup_lti_1p3_deep_linking(consumer, location) + self._setup_lti_1p3_nrps(consumer, location) return consumer @function_trace('lti_consumer.models.LtiConfiguration.get_lti_consumer') - def get_lti_consumer(self): + def get_lti_consumer(self, location: UsageKey): """ Returns an instanced class of LTI 1.1 or 1.3 consumer. """ if self.version == self.LTI_1P3: - return self._get_lti_1p3_consumer() + return self._get_lti_1p3_consumer(location) - return self._get_lti_1p1_consumer() + return self._get_lti_1p1_consumer(location) - def get_lti_1p3_redirect_uris(self): + def get_lti_1p3_redirect_uris(self, location): """ Return pre-registered redirect uris or sensible defaults """ @@ -642,7 +594,7 @@ def get_lti_1p3_redirect_uris(self): launch_url = self.lti_1p3_launch_url deep_link_launch_url = self.lti_advantage_deep_linking_launch_url else: - block = compat.load_enough_xblock(self.location) + block = compat.load_enough_xblock(location) redirect_uris = block.lti_1p3_redirect_uris launch_url = block.lti_1p3_launch_url deep_link_launch_url = block.lti_advantage_deep_linking_launch_url @@ -670,7 +622,37 @@ def pii_share_email(self, value): self.lti_config['pii_share_email'] = value # pylint: disable=unsupported-assignment-operation def __str__(self): - return f"[{self.config_store}] {self.version} - {self.location}" + return f"[{self.config_store}] {self.version} - {self.config_id}" + + class Meta: + app_label = 'lti_consumer' + + +class LtiXBlockConfig(models.Model): + """ + Modal to store xblock and lti configurations link. + """ + # Block location where the configuration is stored. + location = UsageKeyField( + max_length=255, + db_index=True, + ) + lti_configuration = models.ForeignKey( + LtiConfiguration, + null=True, + blank=True, + on_delete=models.CASCADE, + ) + + def __str__(self): + return f"[{self.location}] - {self.lti_configuration}" + + @function_trace('lti_consumer.models.LtiXBlockConfig.get_lti_consumer') + def get_lti_consumer(self): + """ + Returns an instanced class of LTI 1.1 or 1.3 consumer. + """ + return self.lti_configuration._get_lti_1p1_consumer(self.location) class Meta: app_label = 'lti_consumer' @@ -700,6 +682,12 @@ class LtiAgsLineItem(models.Model): blank=True ) + lti_xblock_config = models.ForeignKey( + LtiXBlockConfig, + on_delete=models.CASCADE, + null=True, + blank=True + ) # Tool resource identifier, not used by the LMS. resource_id = models.CharField(max_length=255, blank=True) @@ -844,6 +832,12 @@ class LtiDlContentItem(models.Model): blank=True ) + lti_xblock_config = models.ForeignKey( + LtiXBlockConfig, + on_delete=models.CASCADE, + null=True, + blank=True + ) # Content Item Types # Values based on http://www.imsglobal.org/spec/lti-dl/v2p0#content-item-types # to make type matching easier. diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index dee1170a..0261c95a 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -118,6 +118,25 @@ def load_enough_xblock(location): # pragma: nocover return xblock_api.load_block(location, None) +def save_xblock(block): # pragma: nocover + """ + Load enough of an xblock to read from for LTI values stored on the block. + The block may or may not be bound to the user for actual use depending on + what has happened in the request so far. + """ + # pylint: disable=import-error,import-outside-toplevel + from xmodule.modulestore.django import modulestore + from openedx.core.djangoapps.xblock import api as xblock_api + + # Save course block using modulestore + if isinstance(block.scope_ids.usage_id.context_key, CourseKey): + return modulestore().update_item(block, None) + # Save library block using the XBlock API + else: + runtime = xblock_api.get_runtime() + return runtime.save_block(block) + + def load_block_as_user(location): # pragma: nocover """ Load a block as the current user, or load as the anonymous user if no user is available. diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index 2f753bf3..08ac283e 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -45,7 +45,7 @@ LtiNrpsContextMembershipBasicSerializer, LtiNrpsContextMembershipPIISerializer) from lti_consumer.lti_1p3.extensions.rest_framework.utils import IgnoreContentNegotiation -from lti_consumer.models import LtiAgsLineItem, LtiConfiguration, LtiDlContentItem +from lti_consumer.models import LtiAgsLineItem, LtiConfiguration, LtiDlContentItem, LtiXBlockConfig from lti_consumer.plugin import compat from lti_consumer.signals.signals import LTI_1P3_PROCTORING_ASSESSMENT_STARTED from lti_consumer.track import track_event @@ -516,7 +516,7 @@ def deep_linking_response_endpoint(request, lti_config_id=None): """ try: # Retrieve LTI configuration - lti_config = LtiConfiguration.objects.get(id=lti_config_id) + lti_config = LtiXBlockConfig.objects.get(id=lti_config_id) # Get LTI consumer lti_consumer = lti_config.get_lti_consumer() @@ -536,7 +536,7 @@ def deep_linking_response_endpoint(request, lti_config_id=None): # verify and save each content item passed from the tool. with transaction.atomic(): # Erase older deep linking selection - LtiDlContentItem.objects.filter(lti_configuration=lti_config).delete() + LtiDlContentItem.objects.filter(lti_xblock_config=lti_config).delete() for content_item in content_items: content_type = content_item.get('type') @@ -553,7 +553,7 @@ def deep_linking_response_endpoint(request, lti_config_id=None): # Save content item LtiDlContentItem.objects.create( - lti_configuration=lti_config, + lti_xblock_config=lti_config, content_type=content_type, attributes=serializer.validated_data, ) @@ -685,7 +685,7 @@ class LtiAgsLineItemViewset(viewsets.ModelViewSet): ] def get_queryset(self): - lti_configuration = self.request.lti_configuration + lti_xblock_config = self.request.lti_xblock_config # Return all LineItems related to the LTI configuration. # TODO: @@ -693,12 +693,12 @@ def get_queryset(self): # to each resource link (block), and this filter needs # improved once we start reusing LTI configurations. return LtiAgsLineItem.objects.filter( - lti_configuration=lti_configuration + lti_xblock_config=lti_xblock_config ) def perform_create(self, serializer): - lti_configuration = self.request.lti_configuration - serializer.save(lti_configuration=lti_configuration) + lti_xblock_config = self.request.lti_xblock_config + serializer.save(lti_xblock_config=lti_xblock_config) @action( detail=True, @@ -819,7 +819,7 @@ def get_serializer_class(self): Checks if PII fields can be exposed and returns appropiate serializer. """ if (not compat.nrps_pii_disallowed() and - get_lti_pii_sharing_state_for_course(self.request.lti_configuration.location.course_key)): + get_lti_pii_sharing_state_for_course(self.request.lti_xblock_config.location.course_key)): return LtiNrpsContextMembershipPIISerializer else: return LtiNrpsContextMembershipBasicSerializer @@ -831,7 +831,7 @@ def list(self, *args, **kwargs): """ # get course key - course_key = self.request.lti_configuration.location.course_key + course_key = self.request.lti_xblock_config.location.course_key try: data = compat.get_course_members(course_key) From 48a184209fdada7f93f435a19e89ee3a8067747c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 13 Feb 2026 16:18:40 +0530 Subject: [PATCH 02/14] fixup! refactor: decouple config_id and location --- lti_consumer/api.py | 81 ++++++++++++++-------- lti_consumer/lti_xblock.py | 16 +++-- lti_consumer/tests/unit/test_api.py | 20 +++--- lti_consumer/tests/unit/test_lti_xblock.py | 4 +- 4 files changed, 75 insertions(+), 46 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index db4381c9..028d9366 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -4,28 +4,31 @@ Some methods are meant to be used inside the XBlock, so they return plaintext to allow easy testing/mocking. """ +from uuid import UUID 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 ( 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, ) -from .filters import get_external_config_from_filter -def _get_or_create_local_lti_config( - lti_version, - config_id, +def _get_or_create_local_lti_xblock_config( + lti_version: str, + block_location: UsageKey | str, + config_id: str | None = None, config_store=LtiConfiguration.CONFIG_ON_XBLOCK, external_id=None, ): @@ -39,7 +42,14 @@ def _get_or_create_local_lti_config( 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(config_id=config_id) + lti_xblock_config, created = LtiXBlockConfig.objects.get_or_create(location=block_location) + if created: + if config_id: + lti_config, _ = LtiConfiguration.objects.get_or_create(config_id=config_id) + else: + lti_config = LtiConfiguration.objects.create() + else: + lti_config = lti_xblock_config.lti_configuration lti_config.config_store = config_store lti_config.external_id = external_id @@ -49,7 +59,7 @@ def _get_or_create_local_lti_config( lti_config.save() - return lti_config + return lti_xblock_config def _get_config_by_config_id(config_id): @@ -59,16 +69,24 @@ def _get_config_by_config_id(config_id): return LtiConfiguration.objects.get(config_id=config_id) +def _get_config_by_location(location: UsageKey | str): + """ + Gets the LTI xblock config by its UUID config ID + """ + return LtiXBlockConfig.objects.get(location=location) + + 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, block.config_id, LtiConfiguration.CONFIG_ON_DB, ) @@ -77,51 +95,54 @@ def _get_lti_config_for_block(block): {"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, block.config_id, LtiConfiguration.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, block.config_id, LtiConfiguration.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(location: UsageKey): """ - 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_location(location).get_lti_consumer() def get_lti_1p3_launch_info( launch_data, + location: UsageKey, ): """ 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_xblock_config = _get_config_by_location(location) + lti_consumer = lti_xblock_config.get_lti_consumer() # Check if deep Linking is available, if so, add some extra context: # Deep linking launch URL, and if deep linking is already configured @@ -138,12 +159,13 @@ def get_lti_1p3_launch_info( # Retrieve LTI Content Items (if any was set up) dl_content_items = LtiDlContentItem.objects.filter( - lti_configuration=lti_config + lti_xblock_config=lti_xblock_config ) # Add content item attributes to context if dl_content_items.exists(): deep_linking_content_items = [item.attributes for item in dl_content_items] + lti_config = lti_xblock_config.lti_configuration config_id = lti_config.config_id client_id = lti_config.lti_1p3_client_id deployment_id = "1" @@ -171,6 +193,7 @@ def get_lti_1p3_launch_info( def get_lti_1p3_launch_start_url( launch_data, + location: UsageKey, deep_link_launch=False, dl_content_id=None, ): @@ -178,7 +201,7 @@ def get_lti_1p3_launch_start_url( 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(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 @@ -196,6 +219,7 @@ def get_lti_1p3_launch_start_url( def get_lti_1p3_content_url( launch_data, + location: UsageKey, ): """ Computes and returns which URL the LTI consumer should launch to. @@ -215,13 +239,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, ) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index ecbecc3a..5b1d4e8b 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -715,11 +715,14 @@ def validate(self): validation.add(ValidationMessage(ValidationMessage.WARNING, str( _("The specified LTI ID is not configured in this course's Advanced Settings.") ))) + return validation + + def save(self): if self.lti_version == "lti_1p3" and not self.config_id: self.config_id = str(uuid.uuid4()) # __AUTO_GENERATED_PRINT_VAR_START__ print(f"""======================================= LtiConsumerXBlock#validate config_id: {self.config_id}""") # __AUTO_GENERATED_PRINT_VAR_END__ - return validation + super().save() def get_settings(self): """ @@ -1126,9 +1129,9 @@ def _get_lti_consumer(self): # Runtime import since this will only run in the # Open edX LMS/Studio environments. # pylint: disable=import-outside-toplevel - from lti_consumer.api import config_id_for_block, get_lti_consumer + from lti_consumer.api import config_for_block - return get_lti_consumer(config_id_for_block(self)) + return config_for_block(self).get_lti_consumer() def extract_real_user_data(self): """ @@ -1654,8 +1657,8 @@ def get_lti_1p3_launch_data(self): # Open edX LMS/Studio environments. # TODO: Replace this with a more appropriate API function that is intended for public use. # pylint: disable=import-outside-toplevel - from lti_consumer.api import config_id_for_block - config_id = config_id_for_block(self) + from lti_consumer.api import config_for_block + xblock_config = config_for_block(self) location = self.scope_ids.usage_id course_key = str(location.context_key) @@ -1678,7 +1681,7 @@ def get_lti_1p3_launch_data(self): launch_data = Lti1p3LaunchData( user_id=self.lms_user_id, user_role=self.role, - config_id=config_id, + config_id=xblock_config.config.config_id, resource_link_id=str(location), external_user_id=self.external_user_id, preferred_username=username, @@ -1719,6 +1722,7 @@ def _get_lti_block_launch_handler(self): from lti_consumer.api import get_lti_1p3_content_url # pylint: disable=import-outside-toplevel lti_block_launch_handler = get_lti_1p3_content_url( launch_data, + self.scope_ids.usage_id, ) return lti_block_launch_handler diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index bbd18f94..aa5ebf31 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -11,8 +11,8 @@ from lti_consumer.api import ( _get_config_by_config_id, - _get_or_create_local_lti_config, - config_id_for_block, + _get_or_create_local_lti_xblock_config, + config_for_block, get_end_assessment_return, get_lti_1p3_content_url, get_deep_linking_data, @@ -103,13 +103,13 @@ def setUp(self): def test_double_fetch(self): self.xblock.config_type = 'database' - config_id = config_id_for_block(self.xblock) + config_id = config_for_block(self.xblock) self.assertIsNotNone(config_id) config = _get_config_by_config_id(config_id) self.assertEqual(LtiConfiguration.CONFIG_ON_DB, config.config_store) # fetch again, shouldn't make a new one - second_config_id = config_id_for_block(self.xblock) + second_config_id = config_for_block(self.xblock) self.assertEqual(config_id, second_config_id) @ddt.data( @@ -122,7 +122,7 @@ def test_store_types(self, mapping_pair, mock_external_config): mock_external_config.return_value = {"version": LtiConfiguration.LTI_1P3} str_store, result_store = mapping_pair self.xblock.config_type = str_store - config_id = config_id_for_block(self.xblock) + config_id = config_for_block(self.xblock) self.assertIsNotNone(config_id) config = _get_config_by_config_id(config_id) self.assertEqual(result_store, config.config_store) @@ -144,7 +144,7 @@ def test_create_lti_config_if_inexistent(self): self.assertEqual(LtiConfiguration.objects.all().count(), 0) # Call API - lti_config = _get_or_create_local_lti_config( + lti_config = _get_or_create_local_lti_xblock_config( lti_version=lti_version, block_location=location ) @@ -166,7 +166,7 @@ def test_retrieve_existing(self): ) # Call API - lti_config_retrieved = _get_or_create_local_lti_config( + lti_config_retrieved = _get_or_create_local_lti_xblock_config( lti_version=lti_version, block_location=location ) @@ -187,7 +187,7 @@ def test_update_lti_version(self): ) # Call API - _get_or_create_local_lti_config( + _get_or_create_local_lti_xblock_config( lti_version=LtiConfiguration.LTI_1P3, block_location=location ) @@ -204,7 +204,7 @@ def test_create_lti_config_config_store(self, config_store): """ location = 'block-v1:course+test+2020+type@problem+block@test' lti_version = LtiConfiguration.LTI_1P3 - lti_config = _get_or_create_local_lti_config( + lti_config = _get_or_create_local_lti_xblock_config( lti_version=lti_version, block_location=location, config_store=config_store, @@ -226,7 +226,7 @@ def test_external_config_values_are_cleared(self): external_id="test_plugin:test-id" ) - _get_or_create_local_lti_config( + _get_or_create_local_lti_xblock_config( lti_version=lti_version, block_location=location, external_id=None diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index ab81ff95..4923888c 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -18,7 +18,7 @@ from jwt.api_jwk import PyJWK, PyJWKSet from xblock.validation import Validation -from lti_consumer.api import config_id_for_block +from lti_consumer.api import config_for_block from lti_consumer.data import Lti1p3LaunchData from lti_consumer.exceptions import LtiError from lti_consumer.lti_1p3.tests.utils import create_jwt @@ -1894,7 +1894,7 @@ def test_get_lti_1p3_launch_data( expected_launch_data_kwargs = { "user_id": 1, "user_role": "instructor", - "config_id": config_id_for_block(self.xblock), + "config_id": config_for_block(self.xblock), "resource_link_id": str(self.xblock.scope_ids.usage_id), "external_user_id": "external_user_id", "launch_presentation_document_target": "iframe", From 07dea920ba93f91dd9e6d0199ebb1b846767e56f Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 9 Mar 2026 12:32:09 +0530 Subject: [PATCH 03/14] fixup! refactor: decouple config_id and location --- lti_consumer/api.py | 1 - lti_consumer/lti_xblock.py | 2 -- lti_consumer/models.py | 2 +- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 028d9366..09e88fba 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -4,7 +4,6 @@ Some methods are meant to be used inside the XBlock, so they return plaintext to allow easy testing/mocking. """ -from uuid import UUID import json diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 5b1d4e8b..68488b34 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -720,8 +720,6 @@ def validate(self): def save(self): if self.lti_version == "lti_1p3" and not self.config_id: self.config_id = str(uuid.uuid4()) - # __AUTO_GENERATED_PRINT_VAR_START__ - print(f"""======================================= LtiConsumerXBlock#validate config_id: {self.config_id}""") # __AUTO_GENERATED_PRINT_VAR_END__ super().save() def get_settings(self): diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 6ebe37ed..06652671 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -652,7 +652,7 @@ def get_lti_consumer(self): """ Returns an instanced class of LTI 1.1 or 1.3 consumer. """ - return self.lti_configuration._get_lti_1p1_consumer(self.location) + return self.lti_configuration.get_lti_consumer(self.location) class Meta: app_label = 'lti_consumer' From 00420abacd68e85aa280602b9427ad775cba41be Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 9 Mar 2026 16:07:24 +0530 Subject: [PATCH 04/14] fixup! refactor: decouple config_id and location --- ...agslineitem_lti_xblock_config_and_more.py} | 21 ++++++++++++---- .../0021_migrate_config_id_to_blocks.py | 24 ++++++++++++++++++- 2 files changed, 39 insertions(+), 6 deletions(-) rename lti_consumer/migrations/{0020_ltixblockconfig.py => 0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py} (53%) diff --git a/lti_consumer/migrations/0020_ltixblockconfig.py b/lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py similarity index 53% rename from lti_consumer/migrations/0020_ltixblockconfig.py rename to lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py index 0030772a..bad1644f 100644 --- a/lti_consumer/migrations/0020_ltixblockconfig.py +++ b/lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 5.2.11 on 2026-02-12 10:40 +# Generated by Django 5.2.12 on 2026-03-09 10:27 import django.db.models.deletion import opaque_keys.edx.django.models @@ -15,10 +15,7 @@ class Migration(migrations.Migration): name='LtiXBlockConfig', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ( - 'location', - opaque_keys.edx.django.models.UsageKeyField(blank=True, db_index=True, max_length=255, null=True), - ), + ('location', opaque_keys.edx.django.models.UsageKeyField(db_index=True, max_length=255)), ( 'lti_configuration', models.ForeignKey( @@ -30,4 +27,18 @@ class Migration(migrations.Migration): ), ], ), + migrations.AddField( + model_name='ltiagslineitem', + name='lti_xblock_config', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='lti_consumer.ltixblockconfig' + ), + ), + migrations.AddField( + model_name='ltidlcontentitem', + name='lti_xblock_config', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='lti_consumer.ltixblockconfig' + ), + ), ] diff --git a/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py index 8c7bbbc1..8732691f 100644 --- a/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py +++ b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py @@ -29,6 +29,28 @@ def copy_config_id(apps, schema_editor): except Exception as e: print(f"Failed to copy config_id for configuration {configuration}: {e}") + LtiAgsLineItem = apps.get_model("lti_consumer", "LtiAgsLineItem") + for line_item in LtiAgsLineItem.objects.all(): + xblock_config = LtiXBlockConfig.objects.filter( + lti_configuration=line_item.lti_configuration, + location=line_item.lti_configuration.location, + ).first() + if not xblock_config: + print(f"Invalid configuration linked to AGS line item: {line_item}.") + line_item.xblock_config = xblock_config + line_item.save() + + LtiDlContentItem = apps.get_model("lti_consumer", "LtiDlContentItem") + for content_item in LtiDlContentItem.objects.all(): + xblock_config = LtiXBlockConfig.objects.filter( + lti_configuration=content_item.lti_configuration, + location=content_item.lti_configuration.location, + ).first() + if not xblock_config: + print(f"Invalid configuration linked to Dl Conent Item: {content_item}.") + content_item.xblock_config = xblock_config + content_item.save() + def backwards(apps, schema_editor): """Reverse the migration only for MariaDB databases.""" @@ -38,7 +60,7 @@ def backwards(apps, schema_editor): class Migration(migrations.Migration): dependencies = [ - ('lti_consumer', '0020_ltixblockconfig'), + ('lti_consumer', '0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more'), ] operations = [ From 2d71c3495b7944c979fa8af38703ae20fda18dff Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 9 Mar 2026 20:54:49 +0530 Subject: [PATCH 05/14] fixup! refactor: decouple config_id and location --- lti_consumer/admin.py | 9 ++++ lti_consumer/api.py | 3 ++ lti_consumer/lti_xblock.py | 1 + lti_consumer/plugin/views.py | 96 +++++++++++++++++++++++------------- 4 files changed, 76 insertions(+), 33 deletions(-) diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py index f800726b..feb40689 100644 --- a/lti_consumer/admin.py +++ b/lti_consumer/admin.py @@ -11,6 +11,7 @@ LtiAgsScore, LtiConfiguration, LtiDlContentItem, + LtiXBlockConfig, ) @@ -24,6 +25,14 @@ class LtiConfigurationAdmin(admin.ModelAdmin): readonly_fields = ('location', 'config_id') +@admin.register(LtiXBlockConfig) +class LtiXBlockConfigAdmin(admin.ModelAdmin): + """ + Admin view for LtiXBlockConfig models. + """ + list_display = ('location', 'lti_configuration__config_id') + + @admin.register(CourseAllowPIISharingInLTIFlag) class CourseAllowPIISharingInLTIFlagAdmin(KeyedConfigurationModelAdmin): """ diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 09e88fba..5a1feb99 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -42,11 +42,14 @@ def _get_or_create_local_lti_xblock_config( """ # The create operation is only performed when there is no existing configuration for the block 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 diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 68488b34..d955c963 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -1176,6 +1176,7 @@ def _add_author_view(self, context, loader, fragment): context.update( get_lti_1p3_launch_info( launch_data, + self.scope_ids.usage_id ) ) diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index 08ac283e..cdd42548 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -3,7 +3,7 @@ """ import logging import sys -import urllib +import urllib.parse import jwt from django.conf import settings @@ -26,31 +26,48 @@ from rest_framework.status import HTTP_200_OK, HTTP_400_BAD_REQUEST, HTTP_403_FORBIDDEN, HTTP_404_NOT_FOUND from lti_consumer.api import get_lti_pii_sharing_state_for_course, validate_lti_1p3_launch_data -from lti_consumer.exceptions import LtiError, ExternalConfigurationNotFound +from lti_consumer.exceptions import ExternalConfigurationNotFound, LtiError +from lti_consumer.filters import get_external_config_from_filter from lti_consumer.lti_1p3.consumer import LtiConsumer1p3, LtiProctoringConsumer -from lti_consumer.lti_1p3.exceptions import (BadJwtSignature, InvalidClaimValue, Lti1p3Exception, - LtiDeepLinkingContentTypeNotSupported, MalformedJwtToken, - MissingRequiredClaim, NoSuitableKeys, TokenSignatureExpired, - UnknownClientId, UnsupportedGrantType) +from lti_consumer.lti_1p3.exceptions import ( + BadJwtSignature, + InvalidClaimValue, + Lti1p3Exception, + LtiDeepLinkingContentTypeNotSupported, + MalformedJwtToken, + MissingRequiredClaim, + NoSuitableKeys, + TokenSignatureExpired, + UnknownClientId, + UnsupportedGrantType, +) from lti_consumer.lti_1p3.extensions.rest_framework.authentication import Lti1p3ApiAuthentication from lti_consumer.lti_1p3.extensions.rest_framework.constants import LTI_DL_CONTENT_TYPE_SERIALIZER_MAP from lti_consumer.lti_1p3.extensions.rest_framework.parsers import LineItemParser, LineItemScoreParser -from lti_consumer.lti_1p3.extensions.rest_framework.permissions import (LtiAgsPermissions, - LtiNrpsContextMembershipsPermissions) -from lti_consumer.lti_1p3.extensions.rest_framework.renderers import (LineItemRenderer, LineItemResultsRenderer, - LineItemScoreRenderer, LineItemsRenderer, - MembershipResultRenderer) -from lti_consumer.lti_1p3.extensions.rest_framework.serializers import (LtiAgsLineItemSerializer, - LtiAgsResultSerializer, LtiAgsScoreSerializer, - LtiNrpsContextMembershipBasicSerializer, - LtiNrpsContextMembershipPIISerializer) +from lti_consumer.lti_1p3.extensions.rest_framework.permissions import ( + LtiAgsPermissions, + LtiNrpsContextMembershipsPermissions, +) +from lti_consumer.lti_1p3.extensions.rest_framework.renderers import ( + LineItemRenderer, + LineItemResultsRenderer, + LineItemScoreRenderer, + LineItemsRenderer, + MembershipResultRenderer, +) +from lti_consumer.lti_1p3.extensions.rest_framework.serializers import ( + LtiAgsLineItemSerializer, + LtiAgsResultSerializer, + LtiAgsScoreSerializer, + LtiNrpsContextMembershipBasicSerializer, + LtiNrpsContextMembershipPIISerializer, +) from lti_consumer.lti_1p3.extensions.rest_framework.utils import IgnoreContentNegotiation from lti_consumer.models import LtiAgsLineItem, LtiConfiguration, LtiDlContentItem, LtiXBlockConfig from lti_consumer.plugin import compat from lti_consumer.signals.signals import LTI_1P3_PROCTORING_ASSESSMENT_STARTED from lti_consumer.track import track_event from lti_consumer.utils import _, get_data_from_cache, get_lti_1p3_context_types_claim, get_lti_api_base -from lti_consumer.filters import get_external_config_from_filter log = logging.getLogger(__name__) @@ -217,16 +234,22 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume ) config_id = launch_data.config_id + location = urllib.parse.unquote(launch_data.resource_link_id) try: - lti_config = LtiConfiguration.objects.get( - config_id=config_id + lti_xblock_config = LtiXBlockConfig.objects.get( + lti_configuration__config_id=config_id, + location=location + ) + except (LtiXBlockConfig.DoesNotExist, ValidationError) as exc: + log.error( + "Invalid config_id '%s' and resource_link_id '%s' for LTI 1.3 Launch callback", + config_id, + location, ) - except (LtiConfiguration.DoesNotExist, ValidationError) as exc: - log.error("Invalid config_id '%s' for LTI 1.3 Launch callback", config_id) raise Http404 from exc - if lti_config.version != LtiConfiguration.LTI_1P3: - error_msg = f"The LTI Version of the following configuration is not LTI 1.3: {lti_config}" + if lti_xblock_config.lti_configuration.version != LtiConfiguration.LTI_1P3: + error_msg = f"The LTI Version of the following configuration is not LTI 1.3: {lti_xblock_config}" log.error(error_msg) return render( request, @@ -238,7 +261,7 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume context = {} try: - lti_consumer = lti_config.get_lti_consumer() + lti_consumer = lti_xblock_config.get_lti_consumer() # Set sub and roles claims. user_id = launch_data.external_user_id if launch_data.external_user_id else launch_data.user_id @@ -315,7 +338,7 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume # different parameters, set by instructors when running the DL configuration flow. elif deep_linking_content_item_id and lti_consumer.dl: # Retrieve Deep Linking parameters using the parameter. - content_item = lti_config.ltidlcontentitem_set.get(pk=deep_linking_content_item_id) + content_item = lti_xblock_config.lti_configuration.ltidlcontentitem_set.get(pk=deep_linking_content_item_id) # Only filter DL content item from content item set in the same LTI configuration. # This avoids a malicious user to input a random LTI id and perform LTI DL # content launches outside the scope of its configuration. @@ -361,7 +384,7 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume ) }) event = { - 'lti_version': lti_config.version, + 'lti_version': lti_xblock_config.lti_configuration.version, 'user_roles': user_role, 'launch_url': context['launch_url'] } @@ -425,12 +448,16 @@ def access_token_endpoint( version = None lti_consumer = None if usage_id: - lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id)) - version = lti_config.version + lti_config = LtiXBlockConfig.objects.get(location=UsageKey.from_string(usage_id)) + version = lti_config.lti_configuration.version lti_consumer = lti_config.get_lti_consumer() elif lti_config_id: - lti_config = LtiConfiguration.objects.get(config_id=lti_config_id) - version = lti_config.version + # It doesn't matter if multiple xblock refer to same configuration, we just need to find the first one + # as the access_token generation does not depend of any xblock data. + lti_config = LtiXBlockConfig.objects.filter(lti_configuration__config_id=lti_config_id).first() + if not lti_config: + raise LtiError("LTI configuration not found") + version = lti_config.lti_configuration.version lti_consumer = lti_config.get_lti_consumer() elif external_app and external_slug: lti_config = get_external_config_from_filter({}, external_id) @@ -883,7 +910,9 @@ def start_proctoring_assessment_endpoint(request): resource_link_id = decoded_jwt.get('https://purl.imsglobal.org/spec/lti/claim/resource_link', {}).get('id') try: - lti_config = LtiConfiguration.objects.get(lti_1p3_client_id=iss) + lti_config = LtiXBlockConfig.objects.filter(lti_configuration__lti_1p3_client_id=iss).first() + if not lti_config: + raise LtiConfiguration.DoesNotExist except LtiConfiguration.DoesNotExist: log.error("Invalid iss claim '%s' for LTI 1.3 Proctoring Services start_proctoring_assessment_endpoint" " callback", iss) @@ -892,7 +921,8 @@ def start_proctoring_assessment_endpoint(request): lti_consumer = lti_config.get_lti_consumer() if not isinstance(lti_consumer, LtiProctoringConsumer): - log.info("Proctoring Services for LTIConfiguration with config_id %s are not enabled", lti_config.config_id) + log.info("Proctoring Services for LTIConfiguration with config_id %s are not enabled", + lti_config.lti_configuration.config_id) return render(request, 'html/lti_proctoring_start_error.html', status=HTTP_400_BAD_REQUEST) # Grab the data we need from the cache: launch_data and session_data. @@ -908,7 +938,7 @@ def start_proctoring_assessment_endpoint(request): log.warning( f'There was a cache miss trying to fetch the launch data during an LTI 1.3 proctoring StartAssessment ' f'launch when using the cache key {launch_data_key}. The LtiConfiguration config_id is ' - f'{lti_config.config_id} the user_id is {request.user.id}.' + f'{lti_config.lti_configuration.config_id} the user_id is {request.user.id}.' ) return render(request, 'html/lti_proctoring_start_error.html', status=HTTP_400_BAD_REQUEST) @@ -945,7 +975,7 @@ def start_proctoring_assessment_endpoint(request): # for this optional claim. log.error( "An error occurred during the handling of an LtiStartAssessment LTI lauch message for LTIConfiguration " - f"with config_id {lti_config.config_id} and resource_link_id {resource_link_id}. The " + f"with config_id {lti_config.lti_configuration.config_id} and resource_link_id {resource_link_id}. The " "end_assessment_return Tool JWT claim is not a boolean value. An LtiEndAssessment LTI launch message " "will not be sent as part of the end assessment workflow." ) From 11ef9139ac316dfb2c03e38492df06bd4f7203b3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Mar 2026 20:30:40 +0530 Subject: [PATCH 06/14] fixup! refactor: decouple config_id and location --- lti_consumer/lti_xblock.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index d955c963..2014927c 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -718,7 +718,7 @@ def validate(self): return validation def save(self): - if self.lti_version == "lti_1p3" and not self.config_id: + if not self.config_id: self.config_id = str(uuid.uuid4()) super().save() From 7ddac865badd858549265284b904d7eb1a7f0b7d Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Mar 2026 21:06:18 +0530 Subject: [PATCH 07/14] fix: delete configurations from database on xblock delete --- lti_consumer/plugin/compat.py | 22 +++++++++++ lti_consumer/signals/signals.py | 67 +++++++++++++++++++++++++++++++-- requirements/base.in | 1 + requirements/base.txt | 22 ++++++++--- requirements/ci.txt | 49 ++++++++++++++++++------ requirements/dev.txt | 21 +++++++++-- requirements/pip.txt | 4 +- requirements/pip_tools.txt | 2 +- requirements/quality.txt | 27 +++++++++---- requirements/test.txt | 33 ++++++++++++---- requirements/tox.txt | 12 +++--- 11 files changed, 213 insertions(+), 47 deletions(-) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 0261c95a..25522275 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -354,3 +354,25 @@ def nrps_pii_disallowed(): """ return (hasattr(settings, 'LTI_NRPS_DISALLOW_PII') and settings.LTI_NRPS_DISALLOW_PII is True) + + +def get_signal_handler(): + """ + Import the signal handler from LMS + """ + try: + from xmodule.modulestore.django import SignalHandler + return SignalHandler + except ImportError: + return None + + +def yield_dynamic_block_descendants(block, user_id): + """ + Import and run `yield_dynamic_block_descendants` from LMS + """ + try: + from common.djangoapps.util.block_utils import yield_dynamic_block_descendants + return yield_dynamic_block_descendants(block, user_id) + except ImportError: + None diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index c2bfedc2..51981ff6 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -4,13 +4,15 @@ import logging from django.db.models.signals import post_save -from django.dispatch import receiver, Signal +from django.dispatch import Signal, receiver +from openedx_events.content_authoring.data import LibraryBlockData, XBlockData +from openedx_events.content_authoring.signals import LIBRARY_BLOCK_DELETED, XBLOCK_DELETED -from lti_consumer.models import LtiAgsScore +from lti_consumer.models import LtiAgsScore, LtiConfiguration, LtiXBlockConfig from lti_consumer.plugin import compat - log = logging.getLogger(__name__) +SignalHandler = compat.get_signal_handler() @receiver(post_save, sender=LtiAgsScore, dispatch_uid='publish_grade_on_score_update') @@ -83,3 +85,62 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl LTI_1P3_PROCTORING_ASSESSMENT_STARTED = Signal() + + +@receiver(SignalHandler.item_deleted if SignalHandler else []) +def delete_child_lti_configurations(**kwargs): + """ + Delete lti configurtion from database for this block children. + """ + usage_key = kwargs.get('usage_key') + if usage_key: + # Strip branch info + usage_key = usage_key.for_branch(None) + try: + deleted_block = compat.load_enough_xblock(usage_key) + except Exception: + return + id_list = {deleted_block.location} + for block in compat.yield_dynamic_block_descendants(deleted_block, kwargs.get('user_id')): + id_list.add(block.location) + + LtiXBlockConfig.objects.filter( + location__in=id_list + ).delete() + log.info(f"Deleted {len(id_list)} lti xblock configurations in modulestore") + result = LtiConfiguration.objects.filter(ltixblockconfig__isnull=True).delete() + log.info(f"Deleted {result} lti configuration objects in library") + + +@receiver(XBLOCK_DELETED) +def delete_lti_configuration(**kwargs): + """ + Delete lti configurtion from database for this block. + """ + xblock_info = kwargs.get("xblock_info", None) + if not xblock_info or not isinstance(xblock_info, XBlockData): + log.error("Received null or incorrect data for event") + return + + LtiXBlockConfig.objects.filter( + location=xblock_info.usage_key + ).delete() + result = LtiConfiguration.objects.filter(ltixblockconfig__isnull=True).delete() + log.info(f"Deleted {result} lti configuration objects in library") + + +@receiver(LIBRARY_BLOCK_DELETED) +def delete_lib_lti_configuration(**kwargs): + """ + Delete lti configurtion from database for this library block. + """ + library_block = kwargs.get("library_block", None) + if not library_block or not isinstance(library_block, LibraryBlockData): + log.error("Received null or incorrect data for event") + return + + LtiXBlockConfig.objects.filter( + location=library_block.usage_key + ).delete() + result = LtiConfiguration.objects.filter(ltixblockconfig__isnull=True).delete() + log.info(f"Deleted {result} lti configuration objects in library") diff --git a/requirements/base.in b/requirements/base.in index 97c12bc8..413a76dd 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -18,3 +18,4 @@ jsonfield django-config-models # Configuration models for Django allowing config management with auditing openedx-filters django-statici18n +openedx-events # Open edX events from Hooks extension framework (OEP-50) diff --git a/requirements/base.txt b/requirements/base.txt index c45104dc..62ec50e8 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade @@ -9,7 +9,9 @@ appdirs==1.4.4 asgiref==3.11.1 # via django attrs==25.4.0 - # via -r requirements/base.in + # via + # -r requirements/base.in + # openedx-events bleach==6.3.0 # via -r requirements/base.in cffi==2.0.0 @@ -29,6 +31,7 @@ django==5.2.12 # djangorestframework # edx-django-utils # jsonfield + # openedx-events # openedx-filters django-appconf==1.2.0 # via django-statici18n @@ -47,14 +50,21 @@ djangorestframework==3.16.1 dnspython==2.8.0 # via pymongo edx-ccx-keys==2.0.2 - # via -r requirements/base.in + # via + # -r requirements/base.in + # openedx-events edx-django-utils==8.0.1 - # via django-config-models + # via + # django-config-models + # openedx-events edx-opaque-keys[django]==3.1.0 # via # -r requirements/base.in # edx-ccx-keys + # openedx-events # openedx-filters +fastavro==1.12.1 + # via openedx-events fs==2.4.16 # via xblock jsonfield==3.2.0 @@ -75,6 +85,8 @@ markupsafe==3.0.3 # xblock oauthlib==3.3.1 # via -r requirements/base.in +openedx-events==10.5.0 + # via -r requirements/base.in openedx-filters==2.1.0 # via -r requirements/base.in psutil==7.2.2 @@ -110,7 +122,7 @@ stevedore==5.7.0 # edx-opaque-keys typing-extensions==4.15.0 # via edx-opaque-keys -web-fragments==3.1.0 +web-fragments==4.0.0 # via xblock webencodings==0.5.1 # via bleach diff --git a/requirements/ci.txt b/requirements/ci.txt index 13d794c3..fd8e8479 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade @@ -26,23 +26,29 @@ astroid==4.0.4 # pylint # pylint-celery attrs==25.4.0 - # via -r requirements/test.txt -binaryornot==0.5.0 + # via + # -r requirements/test.txt + # openedx-events +backports-tarfile==1.2.0 + # via + # -r requirements/test.txt + # jaraco-context +binaryornot==0.6.0 # via # -r requirements/test.txt # cookiecutter bleach==6.3.0 # via -r requirements/test.txt -boto3==1.42.63 +boto3==1.42.64 # via # -r requirements/test.txt # fs-s3fs -botocore==1.42.63 +botocore==1.42.64 # via # -r requirements/test.txt # boto3 # s3transfer -cachetools==7.0.3 +cachetools==7.0.5 # via # -r requirements/tox.txt # tox @@ -117,6 +123,7 @@ django==5.2.12 # djangorestframework # edx-django-utils # jsonfield + # openedx-events # openedx-filters # xblock-sdk django-appconf==1.2.0 @@ -150,19 +157,27 @@ docutils==0.22.4 # -r requirements/test.txt # readme-renderer edx-ccx-keys==2.0.2 - # via -r requirements/test.txt + # via + # -r requirements/test.txt + # openedx-events edx-django-utils==8.0.1 # via # -r requirements/test.txt # django-config-models + # openedx-events edx-lint==5.6.0 # via -r requirements/test.txt edx-opaque-keys[django]==3.1.0 # via # -r requirements/test.txt # edx-ccx-keys + # openedx-events # openedx-filters -filelock==3.25.0 +fastavro==1.12.1 + # via + # -r requirements/test.txt + # openedx-events +filelock==3.25.1 # via # -r requirements/tox.txt # python-discovery @@ -185,6 +200,10 @@ idna==3.11 # via # -r requirements/test.txt # requests +importlib-metadata==8.7.1 + # via + # -r requirements/test.txt + # keyring isort==8.0.1 # via # -r requirements/test.txt @@ -264,6 +283,8 @@ nh3==0.3.3 # readme-renderer oauthlib==3.3.1 # via -r requirements/test.txt +openedx-events==10.5.0 + # via -r requirements/test.txt openedx-filters==2.1.0 # via -r requirements/test.txt packaging==26.0 @@ -346,7 +367,7 @@ python-dateutil==2.9.0.post0 # arrow # botocore # xblock -python-discovery==1.1.1 +python-discovery==1.1.3 # via # -r requirements/tox.txt # virtualenv @@ -439,7 +460,7 @@ tomlkit==0.14.0 # via # -r requirements/test.txt # pylint -tox==4.49.0 +tox==4.49.1 # via -r requirements/tox.txt twine==6.2.0 # via -r requirements/test.txt @@ -462,11 +483,11 @@ urllib3==1.26.20 # botocore # requests # twine -virtualenv==21.1.0 +virtualenv==21.2.0 # via # -r requirements/tox.txt # tox -web-fragments==3.1.0 +web-fragments==4.0.0 # via # -r requirements/test.txt # xblock @@ -486,6 +507,10 @@ xblock==5.3.0 # xblock-sdk xblock-sdk==0.13.0 # via -r requirements/test.txt +zipp==3.23.0 + # via + # -r requirements/test.txt + # importlib-metadata # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements/dev.txt b/requirements/dev.txt index 12b841df..bcd5fee2 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade @@ -13,7 +13,9 @@ asgiref==3.11.1 # -r requirements/base.txt # django attrs==25.4.0 - # via -r requirements/base.txt + # via + # -r requirements/base.txt + # openedx-events bleach==6.3.0 # via -r requirements/base.txt cffi==2.0.0 @@ -37,6 +39,7 @@ django==5.2.12 # edx-django-utils # edx-i18n-tools # jsonfield + # openedx-events # openedx-filters django-appconf==1.2.0 # via @@ -65,18 +68,26 @@ dnspython==2.8.0 # -r requirements/base.txt # pymongo edx-ccx-keys==2.0.2 - # via -r requirements/base.txt + # via + # -r requirements/base.txt + # openedx-events edx-django-utils==8.0.1 # via # -r requirements/base.txt # django-config-models + # openedx-events edx-i18n-tools==1.9.0 # via -r requirements/dev.in edx-opaque-keys[django]==3.1.0 # via # -r requirements/base.txt # edx-ccx-keys + # openedx-events # openedx-filters +fastavro==1.12.1 + # via + # -r requirements/base.txt + # openedx-events fs==2.4.16 # via # -r requirements/base.txt @@ -104,6 +115,8 @@ markupsafe==3.0.3 # xblock oauthlib==3.3.1 # via -r requirements/base.txt +openedx-events==10.5.0 + # via -r requirements/base.txt openedx-filters==2.1.0 # via -r requirements/base.txt path==16.16.0 @@ -166,7 +179,7 @@ typing-extensions==4.15.0 # via # -r requirements/base.txt # edx-opaque-keys -web-fragments==3.1.0 +web-fragments==4.0.0 # via # -r requirements/base.txt # xblock diff --git a/requirements/pip.txt b/requirements/pip.txt index 084d708e..341a251b 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade @@ -12,5 +12,5 @@ wheel==0.46.3 # The following packages are considered to be unsafe in a requirements file: pip==26.0.1 # via -r requirements/pip.in -setuptools==82.0.0 +setuptools==82.0.1 # via -r requirements/pip.in diff --git a/requirements/pip_tools.txt b/requirements/pip_tools.txt index 107789a1..c7474098 100644 --- a/requirements/pip_tools.txt +++ b/requirements/pip_tools.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade diff --git a/requirements/quality.txt b/requirements/quality.txt index 8f957cdd..f5b2ac11 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade @@ -19,14 +19,16 @@ astroid==4.0.4 # pylint # pylint-celery attrs==25.4.0 - # via -r requirements/base.txt -binaryornot==0.5.0 + # via + # -r requirements/base.txt + # openedx-events +binaryornot==0.6.0 # via cookiecutter bleach==6.3.0 # via -r requirements/base.txt -boto3==1.42.63 +boto3==1.42.64 # via fs-s3fs -botocore==1.42.63 +botocore==1.42.64 # via # boto3 # s3transfer @@ -72,6 +74,7 @@ django==5.2.12 # djangorestframework # edx-django-utils # jsonfield + # openedx-events # openedx-filters # xblock-sdk django-appconf==1.2.0 @@ -101,18 +104,26 @@ dnspython==2.8.0 # -r requirements/base.txt # pymongo edx-ccx-keys==2.0.2 - # via -r requirements/base.txt + # via + # -r requirements/base.txt + # openedx-events edx-django-utils==8.0.1 # via # -r requirements/base.txt # django-config-models + # openedx-events edx-lint==5.6.0 # via -r requirements/quality.in edx-opaque-keys[django]==3.1.0 # via # -r requirements/base.txt # edx-ccx-keys + # openedx-events # openedx-filters +fastavro==1.12.1 + # via + # -r requirements/base.txt + # openedx-events fs==2.4.16 # via # -r requirements/base.txt @@ -159,6 +170,8 @@ mdurl==0.1.2 # via markdown-it-py oauthlib==3.3.1 # via -r requirements/base.txt +openedx-events==10.5.0 + # via -r requirements/base.txt openedx-filters==2.1.0 # via -r requirements/base.txt platformdirs==4.9.4 @@ -270,7 +283,7 @@ urllib3==1.26.20 # -c requirements/constraints.txt # botocore # requests -web-fragments==3.1.0 +web-fragments==4.0.0 # via # -r requirements/base.txt # xblock diff --git a/requirements/test.txt b/requirements/test.txt index 3647d264..427981dc 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade @@ -21,14 +21,18 @@ astroid==4.0.4 # pylint # pylint-celery attrs==25.4.0 - # via -r requirements/base.txt -binaryornot==0.5.0 + # via + # -r requirements/base.txt + # openedx-events +backports-tarfile==1.2.0 + # via jaraco-context +binaryornot==0.6.0 # via cookiecutter bleach==6.3.0 # via -r requirements/base.txt -boto3==1.42.63 +boto3==1.42.64 # via fs-s3fs -botocore==1.42.63 +botocore==1.42.64 # via # boto3 # s3transfer @@ -80,6 +84,7 @@ dill==0.4.1 # djangorestframework # edx-django-utils # jsonfield + # openedx-events # openedx-filters # xblock-sdk django-appconf==1.2.0 @@ -112,18 +117,26 @@ dnspython==2.8.0 docutils==0.22.4 # via readme-renderer edx-ccx-keys==2.0.2 - # via -r requirements/base.txt + # via + # -r requirements/base.txt + # openedx-events edx-django-utils==8.0.1 # via # -r requirements/base.txt # django-config-models + # openedx-events edx-lint==5.6.0 # via -r requirements/test.in edx-opaque-keys[django]==3.1.0 # via # -r requirements/base.txt # edx-ccx-keys + # openedx-events # openedx-filters +fastavro==1.12.1 + # via + # -r requirements/base.txt + # openedx-events fs==2.4.16 # via # -r requirements/base.txt @@ -135,6 +148,8 @@ id==1.5.0 # via twine idna==3.11 # via requests +importlib-metadata==8.7.1 + # via keyring isort==8.0.1 # via pylint jaraco-classes==3.4.0 @@ -192,6 +207,8 @@ nh3==0.3.3 # via readme-renderer oauthlib==3.3.1 # via -r requirements/base.txt +openedx-events==10.5.0 + # via -r requirements/base.txt openedx-filters==2.1.0 # via -r requirements/base.txt packaging==26.0 @@ -330,7 +347,7 @@ urllib3==1.26.20 # botocore # requests # twine -web-fragments==3.1.0 +web-fragments==4.0.0 # via # -r requirements/base.txt # xblock @@ -350,6 +367,8 @@ xblock==5.3.0 # xblock-sdk xblock-sdk==0.13.0 # via -r requirements/test.in +zipp==3.23.0 + # via importlib-metadata # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements/tox.txt b/requirements/tox.txt index 5aa86e57..9e105f42 100644 --- a/requirements/tox.txt +++ b/requirements/tox.txt @@ -1,16 +1,16 @@ # -# This file is autogenerated by pip-compile with Python 3.12 +# This file is autogenerated by pip-compile with Python 3.11 # by the following command: # # make upgrade # -cachetools==7.0.3 +cachetools==7.0.5 # via tox colorama==0.4.6 # via tox distlib==0.4.0 # via virtualenv -filelock==3.25.0 +filelock==3.25.1 # via # python-discovery # tox @@ -28,11 +28,11 @@ pluggy==1.6.0 # via tox pyproject-api==1.10.0 # via tox -python-discovery==1.1.1 +python-discovery==1.1.3 # via virtualenv tomli-w==1.2.0 # via tox -tox==4.49.0 +tox==4.49.1 # via -r requirements/tox.in -virtualenv==21.1.0 +virtualenv==21.2.0 # via tox From 4e5c2e4ef597ba7ebd15b8dfeaa9285600832c57 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 11 Mar 2026 17:57:50 +0530 Subject: [PATCH 08/14] refactor: make location optional --- lti_consumer/models.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 06652671..1312db1d 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -334,7 +334,7 @@ def external_config(self): """ return get_external_config_from_filter({}, self.external_id) - def _get_lti_1p1_consumer(self, location: UsageKey): + def _get_lti_1p1_consumer(self, location: UsageKey | None = None): """ Return a class of LTI 1.1 consumer. """ @@ -485,7 +485,7 @@ def _setup_lti_1p3_nrps(self, consumer, location: UsageKey): except NotImplementedError as exc: log.exception("Error setting up LTI 1.3 Advantage Names and Role Provisioning Services: %s", exc) - def _get_lti_1p3_consumer(self, location: UsageKey): + def _get_lti_1p3_consumer(self, location: UsageKey | None = None): """ Return a class of LTI 1.3 consumer. @@ -572,10 +572,12 @@ def _get_lti_1p3_consumer(self, location: UsageKey): return consumer @function_trace('lti_consumer.models.LtiConfiguration.get_lti_consumer') - def get_lti_consumer(self, location: UsageKey): + def get_lti_consumer(self, location: UsageKey | None = None): """ Returns an instanced class of LTI 1.1 or 1.3 consumer. """ + if self.config_store == self.CONFIG_ON_XBLOCK and location is None: + raise ValueError("UsageKey is required if you are using CONFIG_ON_XBLOCK") if self.version == self.LTI_1P3: return self._get_lti_1p3_consumer(location) From cde31940b4c2222bb2b3f43596194d96289f2aa2 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 11 Mar 2026 19:51:13 +0530 Subject: [PATCH 09/14] feat: handle edge case --- lti_consumer/api.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 5a1feb99..b56af6a4 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -52,6 +52,12 @@ def _get_or_create_local_lti_xblock_config( lti_xblock_config.save() else: lti_config = lti_xblock_config.lti_configuration + if not lti_config: + # 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. + lti_config = LtiConfiguration.objects.create() + lti_xblock_config.lti_configuration = lti_config + lti_xblock_config.save() lti_config.config_store = config_store lti_config.external_id = external_id From a5f472d9e84e38fc56c50a33621186e328ad2f23 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 11 Mar 2026 21:01:02 +0530 Subject: [PATCH 10/14] fix: delete signal handlers --- lti_consumer/plugin/compat.py | 7 +++---- lti_consumer/signals/signals.py | 5 +++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 25522275..405c2ee1 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -7,11 +7,10 @@ from django.conf import settings from django.core.exceptions import ValidationError from django.forms import ModelForm -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey from lti_consumer.exceptions import LtiError - log = logging.getLogger(__name__) @@ -100,15 +99,15 @@ def get_external_multiple_launch_urls_waffle_flag(): # pragma: nocover return CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.{ENABLE_EXTERNAL_MULTIPLE_LAUNCH_URLS}', __name__) -def load_enough_xblock(location): # pragma: nocover +def load_enough_xblock(location: UsageKey): # pragma: nocover """ Load enough of an xblock to read from for LTI values stored on the block. The block may or may not be bound to the user for actual use depending on what has happened in the request so far. """ # pylint: disable=import-error,import-outside-toplevel - from xmodule.modulestore.django import modulestore from openedx.core.djangoapps.xblock import api as xblock_api + from xmodule.modulestore.django import modulestore # Retrieve course block from modulestore if isinstance(location.context_key, CourseKey): diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index 51981ff6..e331089d 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -87,7 +87,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl LTI_1P3_PROCTORING_ASSESSMENT_STARTED = Signal() -@receiver(SignalHandler.item_deleted if SignalHandler else []) +@receiver(SignalHandler.pre_item_deleted if SignalHandler else []) def delete_child_lti_configurations(**kwargs): """ Delete lti configurtion from database for this block children. @@ -98,7 +98,8 @@ def delete_child_lti_configurations(**kwargs): usage_key = usage_key.for_branch(None) try: deleted_block = compat.load_enough_xblock(usage_key) - except Exception: + except Exception as e: + log.warning(f"Cannot find xblock for key {usage_key}. Reason: {str(e)}. ") return id_list = {deleted_block.location} for block in compat.yield_dynamic_block_descendants(deleted_block, kwargs.get('user_id')): From 1945fa56f74394385c66a039650af328df895625 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 12 Mar 2026 19:39:50 +0530 Subject: [PATCH 11/14] feat: branch off new configuration on change Also make location optional and required only when config type is set to xblock. --- lti_consumer/admin.py | 8 +- lti_consumer/api.py | 56 +++--- lti_consumer/lti_xblock.py | 23 ++- ...ig_and_more.py => 0020_ltixblockconfig.py} | 16 +- .../0021_migrate_config_id_to_blocks.py | 11 +- lti_consumer/models.py | 124 ++++++-------- lti_consumer/plugin/compat.py | 10 +- lti_consumer/signals/signals.py | 2 +- .../tests/unit/plugin/test_proctoring.py | 13 +- lti_consumer/tests/unit/plugin/test_views.py | 26 +-- lti_consumer/tests/unit/test_api.py | 26 +-- lti_consumer/tests/unit/test_models.py | 161 ++++++------------ lti_consumer/utils.py | 31 +++- 13 files changed, 232 insertions(+), 275 deletions(-) rename lti_consumer/migrations/{0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py => 0020_ltixblockconfig.py} (59%) diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py index feb40689..b9266783 100644 --- a/lti_consumer/admin.py +++ b/lti_consumer/admin.py @@ -15,6 +15,11 @@ ) +class LtiXBlockConfigInline(admin.TabularInline): + model = LtiXBlockConfig + extra = 0 + + @admin.register(LtiConfiguration) class LtiConfigurationAdmin(admin.ModelAdmin): """ @@ -22,7 +27,8 @@ class LtiConfigurationAdmin(admin.ModelAdmin): Makes the location field read-only to avoid issues. """ - readonly_fields = ('location', 'config_id') + readonly_fields = ('config_id',) + inlines = [LtiXBlockConfigInline] @admin.register(LtiXBlockConfig) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index b56af6a4..ec76c2fb 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -14,6 +14,9 @@ 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_lms_lti_access_token_link, @@ -21,6 +24,7 @@ get_lms_lti_launch_link, get_lti_1p3_context_types_claim, get_lti_deeplinking_content_url, + model_to_dict, ) @@ -28,12 +32,12 @@ def _get_or_create_local_lti_xblock_config( lti_version: str, block_location: UsageKey | str, config_id: str | None = None, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + 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 @@ -52,10 +56,16 @@ def _get_or_create_local_lti_xblock_config( lti_xblock_config.save() else: lti_config = lti_xblock_config.lti_configuration - if not lti_config: + 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. - lti_config = LtiConfiguration.objects.create() + # 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() @@ -70,18 +80,21 @@ def _get_or_create_local_lti_xblock_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 _get_config_by_location(location: UsageKey | str): +def try_get_config_by_id(config_id) -> LtiConfiguration | None: """ - Gets the LTI xblock config by its UUID config ID + Tries to get the LTI config by its UUID config ID """ - return LtiXBlockConfig.objects.get(location=location) + try: + return _get_config_by_config_id(config_id) + except LtiConfiguration.DoesNotExist: + return None def _get_lti_config_for_block(block): @@ -96,7 +109,7 @@ def _get_lti_config_for_block(block): block.lti_version, block.scope_ids.usage_id, block.config_id, - LtiConfiguration.CONFIG_ON_DB, + CONFIG_ON_DB, ) elif block.config_type == 'external': config = get_external_config_from_filter( @@ -107,7 +120,7 @@ def _get_lti_config_for_block(block): config.get("version"), block.scope_ids.usage_id, block.config_id, - LtiConfiguration.CONFIG_EXTERNAL, + CONFIG_EXTERNAL, external_id=block.external_config, ) else: @@ -115,7 +128,7 @@ def _get_lti_config_for_block(block): block.lti_version, block.scope_ids.usage_id, block.config_id, - LtiConfiguration.CONFIG_ON_XBLOCK, + CONFIG_ON_XBLOCK, ) return lti_xblock_config @@ -129,7 +142,7 @@ def config_for_block(block): return xblock_config -def get_lti_consumer(location: UsageKey): +def get_lti_consumer(config_id: str, location: UsageKey | None = None): """ Retrieves an LTI Consumer instance for a given location. @@ -138,19 +151,19 @@ def get_lti_consumer(location: UsageKey): """ # Return an instance of LTI 1.1 or 1.3 consumer, depending # on the configuration stored in the model. - return _get_config_by_location(location).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, + 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_xblock_config = _get_config_by_location(location) - lti_consumer = lti_xblock_config.get_lti_consumer() + lti_config = _get_config_by_config_id(launch_data.config_id) + 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 @@ -167,20 +180,19 @@ def get_lti_1p3_launch_info( # Retrieve LTI Content Items (if any was set up) dl_content_items = LtiDlContentItem.objects.filter( - lti_xblock_config=lti_xblock_config + lti_configuration=lti_config ) # Add content item attributes to context if dl_content_items.exists(): deep_linking_content_items = [item.attributes for item in dl_content_items] - lti_config = lti_xblock_config.lti_configuration config_id = lti_config.config_id client_id = lti_config.lti_1p3_client_id deployment_id = "1" # 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') @@ -201,7 +213,7 @@ def get_lti_1p3_launch_info( def get_lti_1p3_launch_start_url( launch_data, - location: UsageKey, + location: UsageKey | None = None, deep_link_launch=False, dl_content_id=None, ): @@ -209,7 +221,7 @@ def get_lti_1p3_launch_start_url( Computes and retrieves the LTI URL that starts the OIDC flow. """ # Retrieve LTI consumer - lti_consumer = get_lti_consumer(location) + 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 @@ -227,7 +239,7 @@ def get_lti_1p3_launch_start_url( def get_lti_1p3_content_url( launch_data, - location: UsageKey, + location: UsageKey | None = None, ): """ Computes and returns which URL the LTI consumer should launch to. diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 2014927c..59765da8 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -49,11 +49,10 @@ Numeric grades between 0 and 1 and text + basic HTML feedback comments are supported, via GET / PUT / DELETE HTTP methods respectively """ -import uuid - import logging import re import urllib.parse +import uuid from collections import namedtuple from importlib import import_module @@ -61,11 +60,11 @@ from django.conf import settings from django.utils import timezone from web_fragments.fragment import Fragment - from webob import Response from xblock.core import List, Scope, String, XBlock from xblock.fields import Boolean, Float, Integer from xblock.validation import ValidationMessage + try: from xblock.utils.resources import ResourceLoader from xblock.utils.studio_editable import StudioEditableXBlockMixin @@ -75,19 +74,20 @@ from .data import Lti1p3LaunchData from .exceptions import LtiError -from .lti_1p1.consumer import LtiConsumer1p1, parse_result_json, LTI_PARAMETERS +from .lti_1p1.consumer import LTI_PARAMETERS, LtiConsumer1p1, parse_result_json from .lti_1p1.oauth import log_authorization_header from .outcomes import OutcomeService from .plugin import compat from .track import track_event from .utils import ( + EXTERNAL_ID_REGEX, _, - resolve_custom_parameter_template, - external_config_filter_enabled, - external_user_id_1p1_launches_enabled, + compare_config_type, database_config_enabled, - EXTERNAL_ID_REGEX, + external_config_filter_enabled, external_multiple_launch_urls_enabled, + external_user_id_1p1_launches_enabled, + resolve_custom_parameter_template, ) log = logging.getLogger(__name__) @@ -720,6 +720,13 @@ def validate(self): def save(self): if not self.config_id: self.config_id = str(uuid.uuid4()) + else: + from lti_consumer.api import try_get_config_by_id # pylint: disable=import-outside-toplevel + + row = try_get_config_by_id(self.config_id) + if row and not compare_config_type(self.config_type, row.config_store): + # The configuration type has been changed since it was saved. Create a new config row. + self.config_id = str(uuid.uuid4()) super().save() def get_settings(self): diff --git a/lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py b/lti_consumer/migrations/0020_ltixblockconfig.py similarity index 59% rename from lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py rename to lti_consumer/migrations/0020_ltixblockconfig.py index bad1644f..05496dac 100644 --- a/lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py +++ b/lti_consumer/migrations/0020_ltixblockconfig.py @@ -1,4 +1,4 @@ -# Generated by Django 5.2.12 on 2026-03-09 10:27 +# Generated by Django 5.2.12 on 2026-03-12 13:24 import django.db.models.deletion import opaque_keys.edx.django.models @@ -27,18 +27,4 @@ class Migration(migrations.Migration): ), ], ), - migrations.AddField( - model_name='ltiagslineitem', - name='lti_xblock_config', - field=models.ForeignKey( - blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='lti_consumer.ltixblockconfig' - ), - ), - migrations.AddField( - model_name='ltidlcontentitem', - name='lti_xblock_config', - field=models.ForeignKey( - blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='lti_consumer.ltixblockconfig' - ), - ), ] diff --git a/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py index 8732691f..09644039 100644 --- a/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py +++ b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py @@ -7,9 +7,9 @@ from django.db import migrations -def copy_config_id(apps, schema_editor): +def copy_config_id(apps, _): """Copy config_id from LtiConsumer to LtiConsumerXBlock.""" - from lti_consumer.plugin.compat import load_enough_xblock, save_xblock + from lti_consumer.plugin.compat import load_enough_xblock, save_xblock # pylint: disable=import-outside-toplevel LtiConfiguration = apps.get_model("lti_consumer", "LtiConfiguration") LtiXBlockConfig = apps.get_model("lti_consumer", "LtiXBlockConfig") @@ -26,7 +26,7 @@ def copy_config_id(apps, schema_editor): blockstore.config_id = str(configuration.config_id) blockstore.save() save_xblock(blockstore) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught print(f"Failed to copy config_id for configuration {configuration}: {e}") LtiAgsLineItem = apps.get_model("lti_consumer", "LtiAgsLineItem") @@ -52,15 +52,14 @@ def copy_config_id(apps, schema_editor): content_item.save() -def backwards(apps, schema_editor): +def backwards(*_): """Reverse the migration only for MariaDB databases.""" - pass class Migration(migrations.Migration): dependencies = [ - ('lti_consumer', '0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more'), + ('lti_consumer', '0020_ltixblockconfig'), ] operations = [ diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 1312db1d..e1aaeb03 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -27,6 +27,9 @@ from lti_consumer.lti_1p3.key_handlers import PlatformKeyHandler from lti_consumer.plugin import compat from lti_consumer.utils import ( + CONFIG_EXTERNAL, + CONFIG_ON_DB, + CONFIG_ON_XBLOCK, EXTERNAL_ID_REGEX, choose_lti_1p3_redirect_uris, external_multiple_launch_urls_enabled, @@ -72,14 +75,6 @@ class LtiConfiguration(models.Model): default=LTI_1P1, ) - # Configuration storage - # Initally, this only supported the configuration - # stored on the block. Now it has been expanded to - # enable storing LTI configuration in the model itself or in an external - # configuration service fetchable using openedx-filters - CONFIG_ON_XBLOCK = 'CONFIG_ON_XBLOCK' - CONFIG_ON_DB = 'CONFIG_ON_DB' - CONFIG_EXTERNAL = 'CONFIG_EXTERNAL' CONFIG_STORE_CHOICES = [ (CONFIG_ON_XBLOCK, _('Configuration Stored on XBlock fields')), (CONFIG_ON_DB, _('Configuration Stored on this model')), @@ -102,14 +97,6 @@ class LtiConfiguration(models.Model): # A secondary ID for this configuration that can be used in URLs without leaking primary id. config_id = models.UUIDField(unique=True, default=uuid.uuid4, editable=False) - # Block location where the configuration is stored. - location = UsageKeyField( - max_length=255, - db_index=True, - null=True, - blank=True, - ) - # This is where the configuration is stored in the model if stored on this model. lti_config = JSONField( null=False, @@ -250,13 +237,13 @@ class LtiConfiguration(models.Model): ) def clean(self): - if self.config_store == self.CONFIG_EXTERNAL and not EXTERNAL_ID_REGEX.match(str(self.external_id)): + if self.config_store == CONFIG_EXTERNAL and not EXTERNAL_ID_REGEX.match(str(self.external_id)): raise ValidationError({ "config_store": _( 'LTI Configuration using reusable configuration needs a external ID in "x:y" format.', ), }) - if self.version == self.LTI_1P3 and self.config_store == self.CONFIG_ON_DB: + if self.version == self.LTI_1P3 and self.config_store == CONFIG_ON_DB: if self.lti_1p3_tool_public_key == "" and self.lti_1p3_tool_keyset_url == "": raise ValidationError({ "config_store": _( @@ -264,7 +251,7 @@ def clean(self): "lti_1p3_tool_public_key or lti_1p3_tool_keyset_url." ), }) - if (self.version == self.LTI_1P3 and self.config_store in [self.CONFIG_ON_XBLOCK, self.CONFIG_EXTERNAL] and + if (self.version == self.LTI_1P3 and self.config_store in [CONFIG_ON_XBLOCK, CONFIG_EXTERNAL] and self.lti_1p3_proctoring_enabled): raise ValidationError({ "config_store": _("CONFIG_ON_XBLOCK and CONFIG_EXTERNAL are not supported for " @@ -339,11 +326,13 @@ def _get_lti_1p1_consumer(self, location: UsageKey | None = None): Return a class of LTI 1.1 consumer. """ # If LTI configuration is stored in the XBlock. - if self.config_store == self.CONFIG_ON_XBLOCK: + if self.config_store == CONFIG_ON_XBLOCK: + if not location: + raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) key, secret = block.lti_provider_key_secret launch_url = block.launch_url - elif self.config_store == self.CONFIG_EXTERNAL: + elif self.config_store == CONFIG_EXTERNAL: key = self.external_config.get("lti_1p1_client_key") secret = self.external_config.get("lti_1p1_client_secret") launch_url = self.external_config.get("lti_1p1_launch_url") @@ -354,55 +343,63 @@ def _get_lti_1p1_consumer(self, location: UsageKey | None = None): return LtiConsumer1p1(launch_url, key, secret) - def get_lti_advantage_ags_mode(self, location: UsageKey): + def get_lti_advantage_ags_mode(self, location: UsageKey | None = None): """ Return LTI 1.3 Advantage Assignment and Grade Services mode. """ - if self.config_store == self.CONFIG_ON_DB: + if self.config_store == CONFIG_ON_DB: return self.lti_advantage_ags_mode - elif self.config_store == self.CONFIG_EXTERNAL: + elif self.config_store == CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_ags_mode') else: + if not location: + raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) return block.lti_advantage_ags_mode - def get_lti_advantage_deep_linking_enabled(self, location: UsageKey): + def get_lti_advantage_deep_linking_enabled(self, location: UsageKey | None = None): """ Return whether LTI 1.3 Advantage Deep Linking is enabled. """ - if self.config_store == self.CONFIG_ON_DB: + if self.config_store == CONFIG_ON_DB: return self.lti_advantage_deep_linking_enabled - elif self.config_store == self.CONFIG_EXTERNAL: + elif self.config_store == CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_deep_linking_enabled') else: + if not location: + raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) return block.lti_advantage_deep_linking_enabled - def get_lti_advantage_deep_linking_launch_url(self, location: UsageKey): + def get_lti_advantage_deep_linking_launch_url(self, location: UsageKey | None = None): """ Return the LTI 1.3 Advantage Deep Linking launch URL. """ - if self.config_store == self.CONFIG_ON_DB: + if self.config_store == CONFIG_ON_DB: return self.lti_advantage_deep_linking_launch_url - elif self.config_store == self.CONFIG_EXTERNAL: + elif self.config_store == CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_deep_linking_launch_url') else: + if not location: + raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) return block.lti_advantage_deep_linking_launch_url - def get_lti_advantage_nrps_enabled(self, location: UsageKey): + def get_lti_advantage_nrps_enabled(self, location: UsageKey | None = None): """ Return whether LTI 1.3 Advantage Names and Role Provisioning Services is enabled. """ - if self.config_store == self.CONFIG_ON_DB: + if self.config_store == CONFIG_ON_DB: return self.lti_advantage_enable_nrps - elif self.config_store == self.CONFIG_EXTERNAL: + elif self.config_store == CONFIG_EXTERNAL: return self.external_config.get('lti_advantage_enable_nrps') else: + if not location: + raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) return block.lti_1p3_enable_nrps - def _setup_lti_1p3_ags(self, consumer, location: UsageKey): + def _setup_lti_1p3_ags(self, consumer, location: UsageKey | None = None): """ Set up LTI 1.3 Advantage Assigment and Grades Services. """ @@ -424,7 +421,10 @@ def _setup_lti_1p3_ags(self, consumer, location: UsageKey): # and manage lineitems using the AGS endpoints. if not lineitem and lti_advantage_ags_mode == self.LTI_ADVANTAGE_AGS_DECLARATIVE: try: - block = compat.load_enough_xblock(location) + if location: + block = compat.load_enough_xblock(location) + else: + block = None except ValueError: # There is no location to load the block block = None @@ -462,7 +462,7 @@ def _setup_lti_1p3_ags(self, consumer, location: UsageKey): ) ) - def _setup_lti_1p3_deep_linking(self, consumer, location: UsageKey): + def _setup_lti_1p3_deep_linking(self, consumer, location: UsageKey | None = None): """ Set up LTI 1.3 Advantage Deep Linking. """ @@ -475,7 +475,7 @@ def _setup_lti_1p3_deep_linking(self, consumer, location: UsageKey): except NotImplementedError as exc: log.exception("Error setting up LTI 1.3 Advantage Deep Linking: %s", exc) - def _setup_lti_1p3_nrps(self, consumer, location: UsageKey): + def _setup_lti_1p3_nrps(self, consumer, location: UsageKey | None = None): """ Set up LTI 1.3 Advantage Names and Role Provisioning Services. """ @@ -497,10 +497,12 @@ def _get_lti_1p3_consumer(self, location: UsageKey | None = None): # NOTE: This currently prevents an LTI Consumer from supporting both the LTI 1.3 proctoring feature and the LTI # Advantage services. We plan to address this. Follow this issue: # https://github.com/openedx/xblock-lti-consumer/issues/303. - if self.lti_1p3_proctoring_enabled and self.config_store == self.CONFIG_ON_DB: + if self.lti_1p3_proctoring_enabled and self.config_store == CONFIG_ON_DB: consumer_class = LtiProctoringConsumer - if self.config_store == self.CONFIG_ON_XBLOCK: + if self.config_store == CONFIG_ON_XBLOCK: + if not location: + raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) consumer = consumer_class( @@ -520,7 +522,7 @@ def _get_lti_1p3_consumer(self, location: UsageKey | None = None): tool_key=block.lti_1p3_tool_public_key, tool_keyset_url=block.lti_1p3_tool_keyset_url, ) - elif self.config_store == self.CONFIG_ON_DB: + elif self.config_store == CONFIG_ON_DB: consumer = consumer_class( iss=get_lti_api_base(), lti_oidc_url=self.lti_1p3_oidc_url, @@ -538,10 +540,10 @@ def _get_lti_1p3_consumer(self, location: UsageKey | None = None): tool_key=self.lti_1p3_tool_public_key, tool_keyset_url=self.lti_1p3_tool_keyset_url, ) - elif self.config_store == self.CONFIG_EXTERNAL: + elif self.config_store == CONFIG_EXTERNAL: lti_launch_url = self.external_config.get('lti_1p3_launch_url') - if external_multiple_launch_urls_enabled(location.course_key): + if location and external_multiple_launch_urls_enabled(location.course_key): block = compat.load_enough_xblock(location) lti_launch_url = block.lti_1p3_launch_url or lti_launch_url @@ -576,8 +578,8 @@ def get_lti_consumer(self, location: UsageKey | None = None): """ Returns an instanced class of LTI 1.1 or 1.3 consumer. """ - if self.config_store == self.CONFIG_ON_XBLOCK and location is None: - raise ValueError("UsageKey is required if you are using CONFIG_ON_XBLOCK") + if self.config_store == CONFIG_ON_XBLOCK and location is None: + raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") if self.version == self.LTI_1P3: return self._get_lti_1p3_consumer(location) @@ -587,11 +589,11 @@ def get_lti_1p3_redirect_uris(self, location): """ Return pre-registered redirect uris or sensible defaults """ - if self.config_store == self.CONFIG_EXTERNAL: + if self.config_store == CONFIG_EXTERNAL: redirect_uris = self.external_config.get('lti_1p3_redirect_uris') launch_url = self.external_config.get('lti_1p3_launch_url') deep_link_launch_url = self.external_config.get('lti_advantage_deep_linking_launch_url') - elif self.config_store == self.CONFIG_ON_DB: + elif self.config_store == CONFIG_ON_DB: redirect_uris = self.lti_1p3_redirect_uris launch_url = self.lti_1p3_launch_url deep_link_launch_url = self.lti_advantage_deep_linking_launch_url @@ -684,12 +686,6 @@ class LtiAgsLineItem(models.Model): blank=True ) - lti_xblock_config = models.ForeignKey( - LtiXBlockConfig, - on_delete=models.CASCADE, - null=True, - blank=True - ) # Tool resource identifier, not used by the LMS. resource_id = models.CharField(max_length=255, blank=True) @@ -713,10 +709,7 @@ class LtiAgsLineItem(models.Model): end_date_time = models.DateTimeField(blank=True, null=True) def __str__(self): - return "{} - {}".format( - self.resource_link_id, - self.label, - ) + return f"{self.resource_link_id} - {self.label}" class Meta: app_label = 'lti_consumer' @@ -801,11 +794,9 @@ def save(self, *args, **kwargs): super().save(*args, **kwargs) def __str__(self): - return "LineItem {line_item_id}: score {score_given} out of {score_maximum} - {grading_progress}".format( - line_item_id=self.line_item.id, - score_given=self.score_given, - score_maximum=self.score_maximum, - grading_progress=self.grading_progress + return ( + f"LineItem {self.line_item.id}: score {self.score_given} out of {self.score_maximum} -" + f" {self.grading_progress}" ) class Meta: @@ -834,12 +825,6 @@ class LtiDlContentItem(models.Model): blank=True ) - lti_xblock_config = models.ForeignKey( - LtiXBlockConfig, - on_delete=models.CASCADE, - null=True, - blank=True - ) # Content Item Types # Values based on http://www.imsglobal.org/spec/lti-dl/v2p0#content-item-types # to make type matching easier. @@ -864,10 +849,7 @@ class LtiDlContentItem(models.Model): attributes = JSONField() def __str__(self): - return "{}: {}".format( - self.lti_configuration, - self.content_type, - ) + return f"{self.lti_configuration}: {self.content_type}" class Meta: app_label = 'lti_consumer' diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 405c2ee1..4ae92e2a 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -124,15 +124,15 @@ def save_xblock(block): # pragma: nocover what has happened in the request so far. """ # pylint: disable=import-error,import-outside-toplevel - from xmodule.modulestore.django import modulestore from openedx.core.djangoapps.xblock import api as xblock_api + from xmodule.modulestore.django import modulestore # Save course block using modulestore if isinstance(block.scope_ids.usage_id.context_key, CourseKey): return modulestore().update_item(block, None) # Save library block using the XBlock API else: - runtime = xblock_api.get_runtime() + runtime = xblock_api.get_runtime(None) return runtime.save_block(block) @@ -141,7 +141,7 @@ def load_block_as_user(location): # pragma: nocover Load a block as the current user, or load as the anonymous user if no user is available. """ # pylint: disable=import-error,import-outside-toplevel - from crum import get_current_user, get_current_request + from crum import get_current_request, get_current_user from lms.djangoapps.courseware.block_render import get_block_for_descriptor # Retrieve block from modulestore @@ -360,6 +360,7 @@ def get_signal_handler(): Import the signal handler from LMS """ try: + # pylint: disable=import-error,import-outside-toplevel from xmodule.modulestore.django import SignalHandler return SignalHandler except ImportError: @@ -371,7 +372,8 @@ def yield_dynamic_block_descendants(block, user_id): Import and run `yield_dynamic_block_descendants` from LMS """ try: + # pylint: disable=import-error,import-outside-toplevel,redefined-outer-name from common.djangoapps.util.block_utils import yield_dynamic_block_descendants return yield_dynamic_block_descendants(block, user_id) except ImportError: - None + return None diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index e331089d..c52201a3 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -98,7 +98,7 @@ def delete_child_lti_configurations(**kwargs): usage_key = usage_key.for_branch(None) try: deleted_block = compat.load_enough_xblock(usage_key) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught log.warning(f"Cannot find xblock for key {usage_key}. Reason: {str(e)}. ") return id_list = {deleted_block.location} diff --git a/lti_consumer/tests/unit/plugin/test_proctoring.py b/lti_consumer/tests/unit/plugin/test_proctoring.py index 4cd5ea3b..c2bd82e7 100644 --- a/lti_consumer/tests/unit/plugin/test_proctoring.py +++ b/lti_consumer/tests/unit/plugin/test_proctoring.py @@ -11,11 +11,16 @@ from edx_django_utils.cache import TieredCache, get_cache_key from lti_consumer.data import Lti1p3LaunchData, Lti1p3ProctoringLaunchData -from lti_consumer.lti_1p3.exceptions import (BadJwtSignature, InvalidClaimValue, MalformedJwtToken, - MissingRequiredClaim, NoSuitableKeys) +from lti_consumer.lti_1p3.exceptions import ( + BadJwtSignature, + InvalidClaimValue, + MalformedJwtToken, + MissingRequiredClaim, + NoSuitableKeys, +) from lti_consumer.lti_1p3.key_handlers import PlatformKeyHandler from lti_consumer.models import LtiConfiguration -from lti_consumer.utils import get_data_from_cache +from lti_consumer.utils import CONFIG_ON_DB, get_data_from_cache @ddt.ddt @@ -34,7 +39,7 @@ def setUp(self): self.lti_config = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, lti_1p3_proctoring_enabled=True, - config_store=LtiConfiguration.CONFIG_ON_DB, + config_store=CONFIG_ON_DB, ) # Set up cached data necessary for this endpoint: launch_data and session_data. diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index 8a197a4c..d85698a2 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -2,32 +2,32 @@ Tests for LTI 1.3 endpoint views. """ import json -from unittest.mock import patch, Mock +from unittest.mock import Mock, patch import ddt import jwt +from Cryptodome.PublicKey import RSA from django.test.testcases import TestCase from django.urls import reverse from edx_django_utils.cache import TieredCache, get_cache_key from jwt.api_jwk import PyJWK - -from Cryptodome.PublicKey import RSA from opaque_keys.edx.keys import UsageKey + from lti_consumer.data import Lti1p3LaunchData, Lti1p3ProctoringLaunchData -from lti_consumer.models import LtiConfiguration, LtiDlContentItem from lti_consumer.lti_1p3.exceptions import ( - MissingRequiredClaim, MalformedJwtToken, - TokenSignatureExpired, + MissingRequiredClaim, NoSuitableKeys, + PreflightRequestValidationFailure, + TokenSignatureExpired, UnknownClientId, UnsupportedGrantType, - PreflightRequestValidationFailure, ) -from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.lti_1p3.tests.utils import create_jwt +from lti_consumer.lti_xblock import LtiConsumerXBlock +from lti_consumer.models import LtiConfiguration, LtiDlContentItem from lti_consumer.tests.test_utils import make_xblock -from lti_consumer.utils import cache_lti_1p3_launch_data +from lti_consumer.utils import CONFIG_ON_DB, CONFIG_ON_XBLOCK, cache_lti_1p3_launch_data @ddt.ddt @@ -45,7 +45,7 @@ def setUp(self): # Set up LTI Configuration self.lti_config = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + config_store=CONFIG_ON_XBLOCK, location=UsageKey.from_string(self.location) ) @@ -150,7 +150,7 @@ def setUp(self): self.config = LtiConfiguration( version=LtiConfiguration.LTI_1P3, location=self.location, - config_store=LtiConfiguration.CONFIG_ON_DB, + config_store=CONFIG_ON_DB, lti_1p3_redirect_uris=["https://tool.example", "http://tool.example/launch"] ) self.config.save() @@ -382,7 +382,7 @@ def _setup_deep_linking(self, user_role='staff'): """ Set up deep linking for data and mocking for testing. """ - self.config.config_store = LtiConfiguration.CONFIG_ON_XBLOCK + self.config.config_store = CONFIG_ON_XBLOCK self.config.save() self.compat.get_user_role.return_value = user_role @@ -431,7 +431,7 @@ def test_launch_callback_endpoint_deep_linking_database_config(self, dl_enabled) LtiConfiguration.objects.filter(id=self.config.id).update( location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_DB, + config_store=CONFIG_ON_DB, lti_advantage_deep_linking_enabled=dl_enabled, lti_advantage_deep_linking_launch_url=url, ) diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index aa5ebf31..d7659eb0 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -3,8 +3,8 @@ """ from unittest.mock import Mock, patch from urllib.parse import parse_qs, urlparse -import ddt +import ddt from Cryptodome.PublicKey import RSA from django.test.testcases import TestCase from edx_django_utils.cache import get_cache_key @@ -13,9 +13,9 @@ _get_config_by_config_id, _get_or_create_local_lti_xblock_config, config_for_block, + get_deep_linking_data, get_end_assessment_return, get_lti_1p3_content_url, - get_deep_linking_data, get_lti_1p3_launch_info, get_lti_1p3_launch_start_url, validate_lti_1p3_launch_data, @@ -24,7 +24,7 @@ from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiConfiguration, LtiDlContentItem from lti_consumer.tests.test_utils import make_xblock -from lti_consumer.utils import get_data_from_cache +from lti_consumer.utils import CONFIG_EXTERNAL, CONFIG_ON_DB, CONFIG_ON_XBLOCK, get_data_from_cache # it's convenient to have this in lowercase to compare to URLs _test_config_id = "6c440bf4-face-beef-face-e8bcfb1e53bd" @@ -74,7 +74,7 @@ def _setup_lti_block(self): config_id=_test_config_id, location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + config_store=CONFIG_ON_XBLOCK, ) def _get_lti_1p3_launch_data(self): @@ -106,16 +106,16 @@ def test_double_fetch(self): config_id = config_for_block(self.xblock) self.assertIsNotNone(config_id) config = _get_config_by_config_id(config_id) - self.assertEqual(LtiConfiguration.CONFIG_ON_DB, config.config_store) + self.assertEqual(CONFIG_ON_DB, config.config_store) # fetch again, shouldn't make a new one second_config_id = config_for_block(self.xblock) self.assertEqual(config_id, second_config_id) @ddt.data( - ('external', LtiConfiguration.CONFIG_EXTERNAL), - ('database', LtiConfiguration.CONFIG_ON_DB), - ('any other val', LtiConfiguration.CONFIG_ON_XBLOCK), + ('external', CONFIG_EXTERNAL), + ('database', CONFIG_ON_DB), + ('any other val', CONFIG_ON_XBLOCK), ) @patch('lti_consumer.api.get_external_config_from_filter') def test_store_types(self, mapping_pair, mock_external_config): @@ -152,7 +152,7 @@ def test_create_lti_config_if_inexistent(self): # Check if the object was created self.assertEqual(lti_config.version, lti_version) self.assertEqual(str(lti_config.location), location) - self.assertEqual(lti_config.config_store, LtiConfiguration.CONFIG_ON_XBLOCK) + self.assertEqual(lti_config.config_store, CONFIG_ON_XBLOCK) def test_retrieve_existing(self): """ @@ -196,7 +196,7 @@ def test_update_lti_version(self): lti_config.refresh_from_db() self.assertEqual(lti_config.version, LtiConfiguration.LTI_1P3) - @ddt.data(LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_EXTERNAL, LtiConfiguration.CONFIG_ON_DB) + @ddt.data(CONFIG_ON_XBLOCK, CONFIG_EXTERNAL, CONFIG_ON_DB) def test_create_lti_config_config_store(self, config_store): """ Check if the config_store parameter to _get_or_create_local_lti_config is used to change @@ -222,7 +222,7 @@ def test_external_config_values_are_cleared(self): lti_config = LtiConfiguration.objects.create( location=location, version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_EXTERNAL, + config_store=CONFIG_EXTERNAL, external_id="test_plugin:test-id" ) @@ -235,7 +235,7 @@ def test_external_config_values_are_cleared(self): lti_config.refresh_from_db() self.assertEqual(lti_config.version, lti_version) self.assertEqual(str(lti_config.location), location) - self.assertEqual(lti_config.config_store, LtiConfiguration.CONFIG_ON_XBLOCK) + self.assertEqual(lti_config.config_store, CONFIG_ON_XBLOCK) self.assertEqual(lti_config.external_id, None) @@ -553,7 +553,7 @@ def test_launch_info_for_lti_config_with_external_configuration( lti_config = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, config_id=_test_config_id, - config_store=LtiConfiguration.CONFIG_EXTERNAL, + config_store=CONFIG_EXTERNAL, external_id=external_id, ) diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index bab3504f..a244393f 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -3,20 +3,25 @@ """ from contextlib import contextmanager from datetime import datetime, timedelta, timezone -from unittest.mock import patch, call +from unittest.mock import patch import ddt from Cryptodome.PublicKey import RSA from django.core.exceptions import ValidationError from django.test.testcases import TestCase from edx_django_utils.cache import RequestCache -from ccx_keys.locator import CCXBlockUsageLocator from opaque_keys.edx.locator import CourseLocator from lti_consumer.lti_xblock import LtiConsumerXBlock -from lti_consumer.models import (CourseAllowPIISharingInLTIFlag, LtiAgsLineItem, LtiAgsScore, LtiConfiguration, - LtiDlContentItem) +from lti_consumer.models import ( + CourseAllowPIISharingInLTIFlag, + LtiAgsLineItem, + LtiAgsScore, + LtiConfiguration, + LtiDlContentItem, +) from lti_consumer.tests.test_utils import make_xblock +from lti_consumer.utils import CONFIG_EXTERNAL, CONFIG_ON_DB, CONFIG_ON_XBLOCK LAUNCH_URL = 'http://tool.example/launch' DEEP_LINK_URL = 'http://tool.example/deep-link/launch' @@ -68,20 +73,20 @@ def setUp(self): self.lti_1p3_config_db = LtiConfiguration.objects.create( location=self.xblock.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_DB, + config_store=CONFIG_ON_DB, lti_advantage_ags_mode='programmatic', lti_advantage_deep_linking_enabled=True, ) self.lti_1p3_config_external = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_EXTERNAL, + config_store=CONFIG_EXTERNAL, location=self.xblock.scope_ids.usage_id, ) self.lti_1p1_external = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P1, - config_store=LtiConfiguration.CONFIG_EXTERNAL, + config_store=CONFIG_EXTERNAL, external_id="test-external-id" ) @@ -149,9 +154,9 @@ def test_repr(self): ) @ddt.data( - LtiConfiguration.CONFIG_ON_XBLOCK, - LtiConfiguration.CONFIG_ON_DB, - LtiConfiguration.CONFIG_EXTERNAL, + CONFIG_ON_XBLOCK, + CONFIG_ON_DB, + CONFIG_EXTERNAL, ) @patch('lti_consumer.models.get_external_config_from_filter') @patch('lti_consumer.models.external_multiple_launch_urls_enabled') @@ -186,9 +191,9 @@ def test_lti_consumer_ags_enabled(self, config_store, external_multiple_launch_u ) @ddt.data( - {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, - {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': 'disabled'}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'external'}, + {'config_store': CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, + {'config_store': CONFIG_ON_DB, 'expected_value': 'disabled'}, + {'config_store': CONFIG_EXTERNAL, 'expected_value': 'external'}, ) @ddt.unpack @patch('lti_consumer.models.get_external_config_from_filter') @@ -204,9 +209,9 @@ def test_get_lti_advantage_ags_mode(self, filter_mock, config_store, expected_va self.assertEqual(config.get_lti_advantage_ags_mode(), expected_value) @ddt.data( - LtiConfiguration.CONFIG_ON_XBLOCK, - LtiConfiguration.CONFIG_ON_DB, - LtiConfiguration.CONFIG_EXTERNAL, + CONFIG_ON_XBLOCK, + CONFIG_ON_DB, + CONFIG_EXTERNAL, ) @patch('lti_consumer.models.get_external_config_from_filter') @patch('lti_consumer.models.external_multiple_launch_urls_enabled') @@ -245,9 +250,9 @@ def test_lti_consumer_ags_declarative(self, config_store, external_multiple_laun ) @ddt.data( - LtiConfiguration.CONFIG_ON_XBLOCK, - LtiConfiguration.CONFIG_ON_DB, - LtiConfiguration.CONFIG_EXTERNAL, + CONFIG_ON_XBLOCK, + CONFIG_ON_DB, + CONFIG_EXTERNAL, ) @patch('lti_consumer.models.get_external_config_from_filter') @patch('lti_consumer.models.external_multiple_launch_urls_enabled') @@ -269,9 +274,9 @@ def test_lti_consumer_deep_linking_enabled(self, config_store, external_multiple self.assertTrue(consumer.dl) @ddt.data( - {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': False}, - {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': True}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': True}, + {'config_store': CONFIG_ON_XBLOCK, 'expected_value': False}, + {'config_store': CONFIG_ON_DB, 'expected_value': True}, + {'config_store': CONFIG_EXTERNAL, 'expected_value': True}, ) @ddt.unpack @patch('lti_consumer.models.get_external_config_from_filter') @@ -287,9 +292,9 @@ def test_get_lti_advantage_deep_linking_enabled(self, filter_mock, config_store, self.assertEqual(config.get_lti_advantage_deep_linking_enabled(), expected_value) @ddt.data( - {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, - {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': 'database'}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'external'}, + {'config_store': CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, + {'config_store': CONFIG_ON_DB, 'expected_value': 'database'}, + {'config_store': CONFIG_EXTERNAL, 'expected_value': 'external'}, ) @ddt.unpack @patch('lti_consumer.models.get_external_config_from_filter') @@ -305,9 +310,9 @@ def test_get_lti_advantage_deep_linking_launch_url(self, filter_mock, config_sto self.assertEqual(config.get_lti_advantage_deep_linking_launch_url(), expected_value) @ddt.data( - {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': False}, - {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': True}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': True}, + {'config_store': CONFIG_ON_XBLOCK, 'expected_value': False}, + {'config_store': CONFIG_ON_DB, 'expected_value': True}, + {'config_store': CONFIG_EXTERNAL, 'expected_value': True}, ) @ddt.unpack @patch('lti_consumer.models.get_external_config_from_filter') @@ -328,7 +333,7 @@ def test_generate_private_key(self): """ lti_config = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + config_store=CONFIG_ON_XBLOCK, location='block-v1:course+test+2020+type@problem+block@test' ) @@ -351,7 +356,7 @@ def test_generate_public_key_only(self): """ lti_config = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + config_store=CONFIG_ON_XBLOCK, location='block-v1:course+test+2020+type@problem+block@test' ) # Create and retrieve public keys @@ -365,13 +370,13 @@ def test_generate_public_key_only(self): self.assertEqual(regenerated_public_key, public_key) def test_clean(self): - self.lti_1p3_config.config_store = self.lti_1p3_config.CONFIG_ON_XBLOCK + self.lti_1p3_config.config_store = CONFIG_ON_XBLOCK self.lti_1p3_config.location = None with self.assertRaises(ValidationError): self.lti_1p3_config.clean() - self.lti_1p3_config.config_store = self.lti_1p3_config.CONFIG_EXTERNAL + self.lti_1p3_config.config_store = CONFIG_EXTERNAL self.lti_1p3_config.external_id = None with self.assertRaises(ValidationError): @@ -382,7 +387,7 @@ def test_clean(self): with self.assertRaises(ValidationError): self.lti_1p3_config.clean() - self.lti_1p3_config.config_store = self.lti_1p3_config.CONFIG_ON_DB + self.lti_1p3_config.config_store = CONFIG_ON_DB self.lti_1p3_config_db.lti_1p3_tool_keyset_url = '' self.lti_1p3_config_db.lti_1p3_tool_public_key = '' @@ -393,7 +398,7 @@ def test_clean(self): self.lti_1p3_config.lti_1p3_proctoring_enabled = True self.lti_1p3_config.external_id = 'test_id' - for config_store in [self.lti_1p3_config.CONFIG_ON_XBLOCK, self.lti_1p3_config.CONFIG_EXTERNAL]: + for config_store in [CONFIG_ON_XBLOCK, CONFIG_EXTERNAL]: self.lti_1p3_config.config_store = config_store with self.assertRaises(ValidationError): self.lti_1p3_config.clean() @@ -412,7 +417,9 @@ def test_get_redirect_uris_for_xblock_model_returns_expected( self.xblock.lti_advantage_deep_linking_launch_url = deep_link_url self.xblock.lti_1p3_redirect_uris = redirect_uris - assert self.lti_1p3_config.get_lti_1p3_redirect_uris() == expected + assert self.lti_1p3_config.get_lti_1p3_redirect_uris( + self.xblock.scope_ids.usage_id + ) == expected @ddt.data( (LAUNCH_URL, DEEP_LINK_URL, [], [LAUNCH_URL, DEEP_LINK_URL]), @@ -429,7 +436,9 @@ def test_get_redirect_uris_for_db_model_returns_expected( self.lti_1p3_config_db.lti_1p3_redirect_uris = redirect_uris self.lti_1p3_config_db.save() - assert self.lti_1p3_config_db.get_lti_1p3_redirect_uris() == expected + assert self.lti_1p3_config_db.get_lti_1p3_redirect_uris( + self.xblock.scope_ids.usage_id + ) == expected @patch('lti_consumer.models.choose_lti_1p3_redirect_uris', return_value=None) @patch('lti_consumer.models.get_external_config_from_filter') @@ -448,7 +457,9 @@ def test_get_redirect_uris_with_external_config( } get_external_config_from_filter_mock.return_value = external_config - self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) + self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris( + self.xblock.scope_ids.usage_id + ), None) get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) choose_lti_1p3_redirect_uris.assert_called_once_with( external_config['lti_1p3_redirect_uris'], @@ -462,80 +473,6 @@ def test_save(self, sync_configurations_mock): self.assertEqual(self.lti_1p3_config.save(), None) sync_configurations_mock.assert_called_once_with() - @patch('lti_consumer.models.isinstance', return_value=True) - @patch.object(LtiConfiguration.objects, 'filter') - @patch('lti_consumer.models.model_to_dict') - @patch('lti_consumer.models.setattr') - def test_sync_configurations_with_ccx_location( - self, - setattr_mock, - model_to_dict_mock, - filter_mock, - isinstance_mock, - ): - """ - Test sync_configurations method with CCX location. - """ - model_to_dict_mock.return_value = {'test': 'test'} - self.lti_1p3_config.location = 'ccx-block-v1:course+test+2020+ccx@1+type@problem+block@test' - - self.assertEqual(self.lti_1p3_config.sync_configurations(), None) - isinstance_mock.assert_called_once_with(self.lti_1p3_config.location, CCXBlockUsageLocator) - filter_mock.assert_has_calls([ - call(location=self.lti_1p3_config.location.to_block_locator()), - call().first(), - ]) - model_to_dict_mock.assert_called_once_with( - filter_mock.return_value.first(), - ['id', 'config_id', 'location', 'external_config'], - ) - setattr_mock.assert_called_once_with(self.lti_1p3_config, 'test', 'test') - - @patch('lti_consumer.models.isinstance', return_value=False) - @patch.object(LtiConfiguration.objects, 'filter') - @patch('lti_consumer.models.model_to_dict') - def test_sync_configurations_with_location( - self, - model_to_dict_mock, - filter_mock, - isinstance_mock, - ): - """ - Test sync_configurations method with location. - """ - self.assertEqual(self.lti_1p3_config.sync_configurations(), None) - isinstance_mock.assert_called_once_with(self.lti_1p3_config.location, CCXBlockUsageLocator) - filter_mock.assert_has_calls([ - call(location__endswith=str(self.lti_1p3_config.location).split('@')[-1]), - call().filter(location__startswith=CCXBlockUsageLocator.CANONICAL_NAMESPACE), - call().filter().exclude(id=self.lti_1p3_config.pk), - call().filter().exclude().update(**model_to_dict_mock), - ]) - model_to_dict_mock.assert_called_once_with( - self.lti_1p3_config, - ['id', 'config_id', 'location', 'external_config'], - ) - - @patch('lti_consumer.models.isinstance', return_value=False) - @patch.object(LtiConfiguration.objects, 'filter', side_effect=IndexError()) - @patch('lti_consumer.models.log.exception') - def test_sync_configurations_with_invalid_location( - self, - log_exception_mock, - filter_mock, - isinstance_mock, - ): - """ - Test sync_configurations method with invalid location. - """ - self.assertEqual(self.lti_1p3_config.sync_configurations(), None) - isinstance_mock.assert_called_once_with(self.lti_1p3_config.location, CCXBlockUsageLocator) - filter_mock.assert_called_once_with(location__endswith=str(self.lti_1p3_config.location).split('@')[-1]) - log_exception_mock.assert_called_once_with( - f'Failed to query children CCX LTI configurations: ' - f'Failed to parse main LTI configuration location: {self.lti_1p3_config.location}' - ) - @patch('lti_consumer.models.get_external_config_from_filter') @patch('lti_consumer.models.external_multiple_launch_urls_enabled') def test_external_lti_consumer_1p3_returns_launch_url_from_block( @@ -608,7 +545,7 @@ def setUp(self): config_id='6c440bf4-face-beef-face-e8bcfb1e53bd', location=self.dummy_location, version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + config_store=CONFIG_ON_XBLOCK, ) self.line_item = LtiAgsLineItem.objects.create( diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index e69cb00a..9bb4b0a0 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -8,21 +8,42 @@ from urllib.parse import urlencode from django.conf import settings -from edx_django_utils.cache import get_cache_key, TieredCache +from edx_django_utils.cache import TieredCache, get_cache_key +from lti_consumer.lti_1p3.constants import LTI_1P3_CONTEXT_TYPE +from lti_consumer.lti_1p3.exceptions import InvalidClaimValue, MissingRequiredClaim from lti_consumer.plugin.compat import ( - get_external_config_waffle_flag, - get_external_user_id_1p1_launches_waffle_flag, get_database_config_waffle_flag, + get_external_config_waffle_flag, get_external_multiple_launch_urls_waffle_flag, + get_external_user_id_1p1_launches_waffle_flag, ) -from lti_consumer.lti_1p3.constants import LTI_1P3_CONTEXT_TYPE -from lti_consumer.lti_1p3.exceptions import InvalidClaimValue, MissingRequiredClaim log = logging.getLogger(__name__) SLUG_CHARACTER_CLASS = '[-a-zA-Z0-9_]' EXTERNAL_ID_REGEX = re.compile(rf'^({SLUG_CHARACTER_CLASS}+:{SLUG_CHARACTER_CLASS}+)$') +# Configuration storage +# Initally, this only supported the configuration +# stored on the block. Now it has been expanded to +# enable storing LTI configuration in the model itself or in an external +# configuration service fetchable using openedx-filters +CONFIG_ON_XBLOCK = 'CONFIG_ON_XBLOCK' +CONFIG_ON_DB = 'CONFIG_ON_DB' +CONFIG_EXTERNAL = 'CONFIG_EXTERNAL' + + +def compare_config_type(block_field, db_field): + """ + As value of config_type stored in database and xblock differ, we use this function + compare them + """ + db_to_xblock_map = { + CONFIG_ON_XBLOCK: 'new', + CONFIG_ON_DB: 'database', + CONFIG_EXTERNAL: 'external', + } + return db_to_xblock_map[db_field] == block_field def _(text): From 5defb3145d1e1c48f6a55f753aa1b172b6ad4be3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 12 Mar 2026 19:46:02 +0530 Subject: [PATCH 12/14] fix: lint issues --- lti_consumer/plugin/compat.py | 4 ++-- lti_consumer/tests/unit/plugin/test_views.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 4ae92e2a..6ea2ab59 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -360,7 +360,7 @@ def get_signal_handler(): Import the signal handler from LMS """ try: - # pylint: disable=import-error,import-outside-toplevel + # pylint: disable=import-outside-toplevel from xmodule.modulestore.django import SignalHandler return SignalHandler except ImportError: @@ -372,7 +372,7 @@ def yield_dynamic_block_descendants(block, user_id): Import and run `yield_dynamic_block_descendants` from LMS """ try: - # pylint: disable=import-error,import-outside-toplevel,redefined-outer-name + # pylint: disable=import-outside-toplevel,redefined-outer-name from common.djangoapps.util.block_utils import yield_dynamic_block_descendants return yield_dynamic_block_descendants(block, user_id) except ImportError: diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index d85698a2..af31ba39 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -669,8 +669,8 @@ class TestLti1p3AccessTokenEndpoint(TestCase): def setUp(self): super().setUp() - location = 'block-v1:course+test+2020+type@problem+block@test' - self.config = LtiConfiguration(version=LtiConfiguration.LTI_1P3, location=location) + self.location = 'block-v1:course+test+2020+type@problem+block@test' + self.config = LtiConfiguration(version=LtiConfiguration.LTI_1P3) self.config.save() self.url = reverse('lti_consumer:lti_consumer.access_token', args=[str(self.config.config_id)]) # Patch settings calls to LMS method @@ -729,7 +729,7 @@ def test_access_token_endpoint_with_location_in_url(self): url = reverse( 'lti_consumer:lti_consumer.access_token_via_location', - args=[str(self.config.location)] + args=[str(self.location)] ) body = self.get_body(create_jwt(self.key, {})) response = self.client.post(url, data=json.dumps(body), content_type='application/json') From 397db77d218f94cb1a48de54ecfe686212e1fabf Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 12 Mar 2026 19:48:48 +0530 Subject: [PATCH 13/14] refactor: remove location from config table --- .../0022_remove_lticonfiguration_location.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 lti_consumer/migrations/0022_remove_lticonfiguration_location.py diff --git a/lti_consumer/migrations/0022_remove_lticonfiguration_location.py b/lti_consumer/migrations/0022_remove_lticonfiguration_location.py new file mode 100644 index 00000000..69beb237 --- /dev/null +++ b/lti_consumer/migrations/0022_remove_lticonfiguration_location.py @@ -0,0 +1,16 @@ +# Generated by Django 5.2.12 on 2026-03-12 14:18 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ('lti_consumer', '0021_migrate_config_id_to_blocks'), + ] + + operations = [ + migrations.RemoveField( + model_name='lticonfiguration', + name='location', + ), + ] From 7ec0136c7395346f29ebaf5cc0292b21a4bb14d2 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 17 Mar 2026 17:11:28 +0530 Subject: [PATCH 14/14] refactor: use lti_xblock_config in place of lti_configuration Allow location to be any string instead of limiting to usage_key to allow lti to work out of xblock context. --- lti_consumer/api.py | 76 ++-- lti_consumer/filters.py | 6 +- .../migrations/0020_ltixblockconfig.py | 30 -- ...iagslineitem_lti_xblock_config_and_more.py | 51 +++ .../0021_migrate_config_id_to_blocks.py | 18 +- .../0022_remove_lticonfiguration_location.py | 16 - lti_consumer/models.py | 390 ++++++++++-------- lti_consumer/plugin/views.py | 49 ++- lti_consumer/signals/signals.py | 8 +- lti_consumer/utils.py | 11 +- 10 files changed, 368 insertions(+), 287 deletions(-) delete mode 100644 lti_consumer/migrations/0020_ltixblockconfig.py create mode 100644 lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py delete mode 100644 lti_consumer/migrations/0022_remove_lticonfiguration_location.py diff --git a/lti_consumer/api.py b/lti_consumer/api.py index ec76c2fb..cd983390 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -6,10 +6,12 @@ """ import json +from typing import Any from opaque_keys.edx.keys import CourseKey, UsageKey from lti_consumer.lti_1p3.constants import LTI_1P3_ROLE_MAP +from lti_consumer.lti_1p3.exceptions import Lti1p3Exception from .filters import get_external_config_from_filter from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem, LtiXBlockConfig @@ -87,6 +89,17 @@ def _get_config_by_config_id(config_id) -> LtiConfiguration: return LtiConfiguration.objects.get(config_id=config_id) +def get_lti_config_by_location(location: str, **filters: dict[str, Any]) -> LtiXBlockConfig: + """ + Gets the LTI xblock config by location + """ + config = LtiXBlockConfig.objects.get( + location=location, + **filters, + ) + return config + + def try_get_config_by_id(config_id) -> LtiConfiguration | None: """ Tries to get the LTI config by its UUID config ID @@ -142,28 +155,19 @@ def config_for_block(block): return xblock_config -def get_lti_consumer(config_id: str, location: UsageKey | None = None): - """ - 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(location) - - def get_lti_1p3_launch_info( launch_data, - location: UsageKey | None = None, + location: UsageKey, ): """ 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(location) + lti_xblock_config = get_lti_config_by_location( + str(location), + lti_configuration__config_id=launch_data.config_id, + ) + lti_consumer = lti_xblock_config.get_lti_consumer() # Check if deep Linking is available, if so, add some extra context: # Deep linking launch URL, and if deep linking is already configured @@ -180,19 +184,22 @@ def get_lti_1p3_launch_info( # Retrieve LTI Content Items (if any was set up) dl_content_items = LtiDlContentItem.objects.filter( - lti_configuration=lti_config + lti_xblock_config=lti_xblock_config ) # Add content item attributes to context if dl_content_items.exists(): deep_linking_content_items = [item.attributes for item in dl_content_items] + lti_config = lti_xblock_config.lti_configuration + if not lti_config: + raise Lti1p3Exception("LTI configuration not found.") config_id = lti_config.config_id client_id = lti_config.lti_1p3_client_id deployment_id = "1" # Display LTI launch information from external configuration. # if an external configuration is being used. - if lti_config.config_store == CONFIG_EXTERNAL: + if lti_config.config_store == CONFIG_EXTERNAL and lti_config.external_id: 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') @@ -213,7 +220,7 @@ def get_lti_1p3_launch_info( def get_lti_1p3_launch_start_url( launch_data, - location: UsageKey | None = None, + location: UsageKey, deep_link_launch=False, dl_content_id=None, ): @@ -221,7 +228,11 @@ def get_lti_1p3_launch_start_url( Computes and retrieves the LTI URL that starts the OIDC flow. """ # Retrieve LTI consumer - lti_consumer = get_lti_consumer(launch_data.config_id, location) + lti_xblock_config = get_lti_config_by_location( + str(location), + lti_configuration__config_id=launch_data.config_id, + ) + lti_consumer = lti_xblock_config.get_lti_consumer() # 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 @@ -239,7 +250,7 @@ def get_lti_1p3_launch_start_url( def get_lti_1p3_content_url( launch_data, - location: UsageKey | None = None, + location: UsageKey, ): """ Computes and returns which URL the LTI consumer should launch to. @@ -252,10 +263,13 @@ def get_lti_1p3_content_url( Lti DL content in the database. """ # Retrieve LTI consumer - lti_config = _get_config_by_config_id(launch_data.config_id) + lti_xblock_config = get_lti_config_by_location( + str(location), + lti_configuration__config_id=launch_data.config_id, + ) # List content items - content_items = lti_config.ltidlcontentitem_set.all() + content_items = lti_xblock_config.ltidlcontentitem_set.all() # If there's no content items, return normal LTI launch URL if not content_items.count(): @@ -271,23 +285,7 @@ def get_lti_1p3_content_url( ) # If there's more than one content item, return content presentation URL - return get_lti_deeplinking_content_url(lti_config.id, launch_data) - - -def get_deep_linking_data(deep_linking_id, config_id): - """ - Retrieves deep linking attributes. - - Only works with a single line item, this is a limitation in the - current content presentation implementation. - """ - # Retrieve LTI Configuration - lti_config = _get_config_by_config_id(config_id) - # Only filter DL content item from content item set in the same LTI configuration. - # This avoids a malicious user to input a random LTI id and perform LTI DL - # content launches outside the scope of its configuration. - content_item = lti_config.ltidlcontentitem_set.get(pk=deep_linking_id) - return content_item.attributes + return get_lti_deeplinking_content_url(lti_xblock_config.id, launch_data) def get_lti_pii_sharing_state_for_course(course_key: CourseKey) -> bool: diff --git a/lti_consumer/filters.py b/lti_consumer/filters.py index a5275215..22b3648c 100644 --- a/lti_consumer/filters.py +++ b/lti_consumer/filters.py @@ -1,7 +1,7 @@ """ Module that contains the openedx filters for this XBlock """ -from typing import Dict +from typing import Any, Dict from openedx_filters.tooling import OpenEdxPublicFilter @@ -15,7 +15,7 @@ class LTIConfigurationListed(OpenEdxPublicFilter): filter_type = "org.openedx.xblock.lti_consumer.configuration.listed.v1" @classmethod - def run_filter(cls, context: Dict, config_id: str, configurations: Dict): + def run_filter(cls, context: Dict, config_id: str, configurations: Dict) -> tuple[Dict, str, Dict]: """ Execute the filter with the signature specified. @@ -28,7 +28,7 @@ def run_filter(cls, context: Dict, config_id: str, configurations: Dict): return data.get("context"), data.get("config_id"), data.get("configurations") -def get_external_config_from_filter(context, config_id=''): +def get_external_config_from_filter(context, config_id='') -> dict[str, Any]: """ Thin wrapper around the LTIConfigurationListed filter to get the external configuration values using a certain context and config_id. diff --git a/lti_consumer/migrations/0020_ltixblockconfig.py b/lti_consumer/migrations/0020_ltixblockconfig.py deleted file mode 100644 index 05496dac..00000000 --- a/lti_consumer/migrations/0020_ltixblockconfig.py +++ /dev/null @@ -1,30 +0,0 @@ -# Generated by Django 5.2.12 on 2026-03-12 13:24 - -import django.db.models.deletion -import opaque_keys.edx.django.models -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ('lti_consumer', '0019_mariadb_uuid_conversion'), - ] - - operations = [ - migrations.CreateModel( - name='LtiXBlockConfig', - fields=[ - ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('location', opaque_keys.edx.django.models.UsageKeyField(db_index=True, max_length=255)), - ( - 'lti_configuration', - models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.CASCADE, - to='lti_consumer.lticonfiguration', - ), - ), - ], - ), - ] diff --git a/lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py b/lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py new file mode 100644 index 00000000..02226fc0 --- /dev/null +++ b/lti_consumer/migrations/0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more.py @@ -0,0 +1,51 @@ +# Generated by Django 5.2.12 on 2026-03-17 11:55 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('lti_consumer', '0019_mariadb_uuid_conversion'), + ] + + operations = [ + migrations.CreateModel( + name='LtiXBlockConfig', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ( + 'location', + models.CharField( + db_index=True, + help_text='Normally, this is the location of xblock. But it can any string to allow it work outside of xblock context.', + max_length=255, + unique=True, + ), + ), + ( + 'lti_configuration', + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to='lti_consumer.lticonfiguration', + ), + ), + ], + ), + migrations.AddField( + model_name='ltiagslineitem', + name='lti_xblock_config', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='lti_consumer.ltixblockconfig' + ), + ), + migrations.AddField( + model_name='ltidlcontentitem', + name='lti_xblock_config', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='lti_consumer.ltixblockconfig' + ), + ), + ] diff --git a/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py index 09644039..7c19082f 100644 --- a/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py +++ b/lti_consumer/migrations/0021_migrate_config_id_to_blocks.py @@ -1,9 +1,11 @@ -# Generated migration for copying config_id into modulestore from database (Django 5.2) +# Generated migration for copying config_id into modulestore from database (Django 6.2) """ This migration will copy config_id from LtiConsumer database to LtiConsumerXBlock. This will help us link xblocks with LtiConsumer database rows without relying on the location or usage_key of xblocks. """ +import uuid + from django.db import migrations @@ -15,8 +17,10 @@ def copy_config_id(apps, _): LtiXBlockConfig = apps.get_model("lti_consumer", "LtiXBlockConfig") for configuration in LtiConfiguration.objects.all(): + # Create a new unique location for cconfiguration with no location. + location = configuration.location or str(uuid.uuid4()) LtiXBlockConfig.objects.update_or_create( - location=configuration.location, + location=str(location), defaults={ "lti_configuration": configuration, } @@ -33,22 +37,22 @@ def copy_config_id(apps, _): for line_item in LtiAgsLineItem.objects.all(): xblock_config = LtiXBlockConfig.objects.filter( lti_configuration=line_item.lti_configuration, - location=line_item.lti_configuration.location, ).first() if not xblock_config: print(f"Invalid configuration linked to AGS line item: {line_item}.") - line_item.xblock_config = xblock_config + continue + line_item.lti_xblock_config = xblock_config line_item.save() LtiDlContentItem = apps.get_model("lti_consumer", "LtiDlContentItem") for content_item in LtiDlContentItem.objects.all(): xblock_config = LtiXBlockConfig.objects.filter( lti_configuration=content_item.lti_configuration, - location=content_item.lti_configuration.location, ).first() if not xblock_config: print(f"Invalid configuration linked to Dl Conent Item: {content_item}.") - content_item.xblock_config = xblock_config + continue + content_item.lti_xblock_config = xblock_config content_item.save() @@ -59,7 +63,7 @@ def backwards(*_): class Migration(migrations.Migration): dependencies = [ - ('lti_consumer', '0020_ltixblockconfig'), + ('lti_consumer', '0020_ltixblockconfig_ltiagslineitem_lti_xblock_config_and_more'), ] operations = [ diff --git a/lti_consumer/migrations/0022_remove_lticonfiguration_location.py b/lti_consumer/migrations/0022_remove_lticonfiguration_location.py deleted file mode 100644 index 69beb237..00000000 --- a/lti_consumer/migrations/0022_remove_lticonfiguration_location.py +++ /dev/null @@ -1,16 +0,0 @@ -# Generated by Django 5.2.12 on 2026-03-12 14:18 - -from django.db import migrations - - -class Migration(migrations.Migration): - dependencies = [ - ('lti_consumer', '0021_migrate_config_id_to_blocks'), - ] - - operations = [ - migrations.RemoveField( - model_name='lticonfiguration', - name='location', - ), - ] diff --git a/lti_consumer/models.py b/lti_consumer/models.py index e1aaeb03..e3858355 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -4,6 +4,7 @@ import json import logging import uuid +from typing import Any from config_models.models import ConfigurationModel from Cryptodome.PublicKey import RSA @@ -14,6 +15,7 @@ from django.utils.translation import gettext_lazy as _ from edx_django_utils.monitoring import function_trace from jsonfield import JSONField +from opaque_keys import InvalidKeyError from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField from opaque_keys.edx.keys import CourseKey, UsageKey @@ -31,6 +33,10 @@ CONFIG_ON_DB, CONFIG_ON_XBLOCK, EXTERNAL_ID_REGEX, + LTI_ADVANTAGE_AGS_CHOICES, + LTI_ADVANTAGE_AGS_DECLARATIVE, + LTI_ADVANTAGE_AGS_DISABLED, + LTI_ADVANTAGE_AGS_PROGRAMMATIC, choose_lti_1p3_redirect_uris, external_multiple_launch_urls_enabled, get_lti_ags_lineitems_url, @@ -97,6 +103,14 @@ class LtiConfiguration(models.Model): # A secondary ID for this configuration that can be used in URLs without leaking primary id. config_id = models.UUIDField(unique=True, default=uuid.uuid4, editable=False) + # Block location where the configuration is stored. + location = UsageKeyField( + max_length=255, + db_index=True, + null=True, + blank=True, + ) + # This is where the configuration is stored in the model if stored on this model. lti_config = JSONField( null=False, @@ -210,14 +224,6 @@ class LtiConfiguration(models.Model): 'use the same value as lti_1p3_launch_url.' ) - LTI_ADVANTAGE_AGS_DISABLED = 'disabled' - LTI_ADVANTAGE_AGS_DECLARATIVE = 'declarative' - LTI_ADVANTAGE_AGS_PROGRAMMATIC = 'programmatic' - LTI_ADVANTAGE_AGS_CHOICES = [ - (LTI_ADVANTAGE_AGS_DISABLED, 'Disabled'), - (LTI_ADVANTAGE_AGS_DECLARATIVE, 'Allow tools to submit grades only (declarative)'), - (LTI_ADVANTAGE_AGS_PROGRAMMATIC, 'Allow tools to manage and submit grade (programmatic)') - ] lti_advantage_ags_mode = models.CharField( "LTI Advantage Assignment and Grade Services Mode", max_length=20, @@ -315,20 +321,20 @@ def lti_1p3_public_jwk(self): return json.loads(self.lti_1p3_internal_public_jwk) @cached_property - def external_config(self): + def external_config(self) -> dict[str, Any]: """ Return the external resuable configuration. """ return get_external_config_from_filter({}, self.external_id) - def _get_lti_1p1_consumer(self, location: UsageKey | None = None): + def _get_lti_1p1_consumer(self, location: UsageKey | str | None = None): """ Return a class of LTI 1.1 consumer. """ # If LTI configuration is stored in the XBlock. if self.config_store == CONFIG_ON_XBLOCK: - if not location: - raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") + if not location or not isinstance(location, UsageKey): + raise ValueError("Valid location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) key, secret = block.lti_provider_key_secret launch_url = block.launch_url @@ -343,149 +349,10 @@ def _get_lti_1p1_consumer(self, location: UsageKey | None = None): return LtiConsumer1p1(launch_url, key, secret) - def get_lti_advantage_ags_mode(self, location: UsageKey | None = None): - """ - Return LTI 1.3 Advantage Assignment and Grade Services mode. - """ - if self.config_store == CONFIG_ON_DB: - return self.lti_advantage_ags_mode - elif self.config_store == CONFIG_EXTERNAL: - return self.external_config.get('lti_advantage_ags_mode') - else: - if not location: - raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") - block = compat.load_enough_xblock(location) - return block.lti_advantage_ags_mode - - def get_lti_advantage_deep_linking_enabled(self, location: UsageKey | None = None): - """ - Return whether LTI 1.3 Advantage Deep Linking is enabled. - """ - if self.config_store == CONFIG_ON_DB: - return self.lti_advantage_deep_linking_enabled - elif self.config_store == CONFIG_EXTERNAL: - return self.external_config.get('lti_advantage_deep_linking_enabled') - else: - if not location: - raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") - block = compat.load_enough_xblock(location) - return block.lti_advantage_deep_linking_enabled - - def get_lti_advantage_deep_linking_launch_url(self, location: UsageKey | None = None): - """ - Return the LTI 1.3 Advantage Deep Linking launch URL. - """ - if self.config_store == CONFIG_ON_DB: - return self.lti_advantage_deep_linking_launch_url - elif self.config_store == CONFIG_EXTERNAL: - return self.external_config.get('lti_advantage_deep_linking_launch_url') - else: - if not location: - raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") - block = compat.load_enough_xblock(location) - return block.lti_advantage_deep_linking_launch_url - - def get_lti_advantage_nrps_enabled(self, location: UsageKey | None = None): - """ - Return whether LTI 1.3 Advantage Names and Role Provisioning Services is enabled. - """ - if self.config_store == CONFIG_ON_DB: - return self.lti_advantage_enable_nrps - elif self.config_store == CONFIG_EXTERNAL: - return self.external_config.get('lti_advantage_enable_nrps') - else: - if not location: - raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") - block = compat.load_enough_xblock(location) - return block.lti_1p3_enable_nrps - - def _setup_lti_1p3_ags(self, consumer, location: UsageKey | None = None): - """ - Set up LTI 1.3 Advantage Assigment and Grades Services. - """ - - try: - lti_advantage_ags_mode = self.get_lti_advantage_ags_mode(location) - except NotImplementedError as exc: - log.exception("Error setting up LTI 1.3 Advantage Assignment and Grade Services: %s", exc) - return - - if lti_advantage_ags_mode == self.LTI_ADVANTAGE_AGS_DISABLED: - log.info('LTI Advantage AGS is disabled for %s', self) - return - - lineitem = self.ltiagslineitem_set.first() - - # If using the declarative approach, we should create a LineItem if it - # doesn't exist. This is because on this mode the tool is not able to create - # and manage lineitems using the AGS endpoints. - if not lineitem and lti_advantage_ags_mode == self.LTI_ADVANTAGE_AGS_DECLARATIVE: - try: - if location: - block = compat.load_enough_xblock(location) - else: - block = None - except ValueError: # There is no location to load the block - block = None - - if block: - default_values = { - 'resource_id': location, - 'score_maximum': block.weight, - 'label': block.display_name, - } - if hasattr(block, 'start'): - default_values['start_date_time'] = block.start - - if hasattr(block, 'due'): - default_values['end_date_time'] = block.due - else: - # TODO find a way to make these defaults more sensible - default_values = { - 'resource_id': location, - 'score_maximum': 100, - 'label': 'LTI Consumer at ' + str(location) - } - - # create LineItem if there is none for current lti configuration - lineitem = LtiAgsLineItem.objects.create( - lti_configuration=self, - resource_link_id=location, - **default_values - ) - - consumer.enable_ags( - lineitems_url=get_lti_ags_lineitems_url(self.id), - lineitem_url=get_lti_ags_lineitems_url(self.id, lineitem.id) if lineitem else None, - allow_programmatic_grade_interaction=( - lti_advantage_ags_mode == self.LTI_ADVANTAGE_AGS_PROGRAMMATIC - ) - ) - - def _setup_lti_1p3_deep_linking(self, consumer, location: UsageKey | None = None): - """ - Set up LTI 1.3 Advantage Deep Linking. - """ - try: - if self.get_lti_advantage_deep_linking_enabled(location): - consumer.enable_deep_linking( - self.get_lti_advantage_deep_linking_launch_url(location), - get_lti_deeplinking_response_url(self.id), - ) - except NotImplementedError as exc: - log.exception("Error setting up LTI 1.3 Advantage Deep Linking: %s", exc) - - def _setup_lti_1p3_nrps(self, consumer, location: UsageKey | None = None): - """ - Set up LTI 1.3 Advantage Names and Role Provisioning Services. - """ - try: - if self.get_lti_advantage_nrps_enabled(location): - consumer.enable_nrps(get_lti_nrps_context_membership_url(self.id)) - except NotImplementedError as exc: - log.exception("Error setting up LTI 1.3 Advantage Names and Role Provisioning Services: %s", exc) - - def _get_lti_1p3_consumer(self, location: UsageKey | None = None): + def _get_lti_1p3_consumer( + self, + location: UsageKey | str | None = None, + ): """ Return a class of LTI 1.3 consumer. @@ -501,8 +368,8 @@ def _get_lti_1p3_consumer(self, location: UsageKey | None = None): consumer_class = LtiProctoringConsumer if self.config_store == CONFIG_ON_XBLOCK: - if not location: - raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") + if not location or not isinstance(location, UsageKey): + raise ValueError("Valid location is required if you are using CONFIG_ON_XBLOCK") block = compat.load_enough_xblock(location) consumer = consumer_class( @@ -543,7 +410,9 @@ def _get_lti_1p3_consumer(self, location: UsageKey | None = None): elif self.config_store == CONFIG_EXTERNAL: lti_launch_url = self.external_config.get('lti_1p3_launch_url') - if location and external_multiple_launch_urls_enabled(location.course_key): + if location and isinstance(location, UsageKey) and external_multiple_launch_urls_enabled( + location.course_key + ): block = compat.load_enough_xblock(location) lti_launch_url = block.lti_1p3_launch_url or lti_launch_url @@ -566,20 +435,13 @@ def _get_lti_1p3_consumer(self, location: UsageKey | None = None): # CONFIG_ON_XBLOCK, CONFIG_ON_DB or CONFIG_EXTERNAL. raise NotImplementedError - if isinstance(consumer, LtiAdvantageConsumer): - self._setup_lti_1p3_ags(consumer, location) - self._setup_lti_1p3_deep_linking(consumer, location) - self._setup_lti_1p3_nrps(consumer, location) - return consumer @function_trace('lti_consumer.models.LtiConfiguration.get_lti_consumer') - def get_lti_consumer(self, location: UsageKey | None = None): + def get_lti_consumer(self, location: UsageKey | str | None = None): """ Returns an instanced class of LTI 1.1 or 1.3 consumer. """ - if self.config_store == CONFIG_ON_XBLOCK and location is None: - raise ValueError("Location is required if you are using CONFIG_ON_XBLOCK") if self.version == self.LTI_1P3: return self._get_lti_1p3_consumer(location) @@ -637,26 +499,198 @@ class LtiXBlockConfig(models.Model): Modal to store xblock and lti configurations link. """ # Block location where the configuration is stored. - location = UsageKeyField( + location = models.CharField( max_length=255, db_index=True, + unique=True, + help_text=_( + "Normally, this is the location of xblock. But it can any string " + "to allow it work outside of xblock context." + ), ) lti_configuration = models.ForeignKey( LtiConfiguration, - null=True, - blank=True, on_delete=models.CASCADE, ) def __str__(self): return f"[{self.location}] - {self.lti_configuration}" + @property + def location_as_usage_key(self): + """ + Try to parse the location string into a usage key + """ + try: + usage_key = UsageKey.from_string(self.location) + return usage_key + except InvalidKeyError: + return None + + def get_lti_advantage_nrps_enabled(self): + """ + Return whether LTI 1.3 Advantage Names and Role Provisioning Services is enabled. + """ + if self.lti_configuration.config_store == CONFIG_ON_DB: + return self.lti_configuration.lti_advantage_enable_nrps + elif self.lti_configuration.config_store == CONFIG_EXTERNAL: + return self.lti_configuration.external_config.get('lti_advantage_enable_nrps') + else: + block = compat.load_enough_xblock(self.location_as_usage_key) + return block.lti_1p3_enable_nrps + + def _setup_lti_1p3_nrps(self, consumer): + """ + Set up LTI 1.3 Advantage Names and Role Provisioning Services. + """ + try: + if self.get_lti_advantage_nrps_enabled(): + consumer.enable_nrps(get_lti_nrps_context_membership_url(self.id)) + except NotImplementedError as exc: + log.exception("Error setting up LTI 1.3 Advantage Names and Role Provisioning Services: %s", exc) + + def get_lti_advantage_ags_mode(self): + """ + Return LTI 1.3 Advantage Assignment and Grade Services mode. + """ + if self.lti_configuration.config_store == CONFIG_ON_DB: + return self.lti_configuration.lti_advantage_ags_mode + elif self.lti_configuration.config_store == CONFIG_EXTERNAL: + return self.lti_configuration.external_config.get('lti_advantage_ags_mode') + else: + block = compat.load_enough_xblock(self.location_as_usage_key) + return block.lti_advantage_ags_mode + + def _setup_lti_1p3_ags(self, consumer): + """ + Set up LTI 1.3 Advantage Assigment and Grades Services. + """ + + try: + lti_advantage_ags_mode = self.get_lti_advantage_ags_mode() + except NotImplementedError as exc: + log.exception("Error setting up LTI 1.3 Advantage Assignment and Grade Services: %s", exc) + return + + if lti_advantage_ags_mode == LTI_ADVANTAGE_AGS_DISABLED: + log.info('LTI Advantage AGS is disabled for %s', self) + return + + lineitem = self.ltiagslineitem_set.first() + + # If using the declarative approach, we should create a LineItem if it + # doesn't exist. This is because on this mode the tool is not able to create + # and manage lineitems using the AGS endpoints. + if not lineitem and lti_advantage_ags_mode == LTI_ADVANTAGE_AGS_DECLARATIVE: + try: + if self.location_as_usage_key: + block = compat.load_enough_xblock(self.location_as_usage_key) + else: + block = None + except ValueError: # There is no location to load the block + block = None + + if block: + default_values = { + 'resource_id': self.location_as_usage_key, + 'score_maximum': block.weight, + 'label': block.display_name, + } + if hasattr(block, 'start'): + default_values['start_date_time'] = block.start + + if hasattr(block, 'due'): + default_values['end_date_time'] = block.due + else: + # TODO find a way to make these defaults more sensible + default_values = { + 'resource_id': self.location, + 'score_maximum': 100, + 'label': 'LTI Consumer at ' + str(self.location) + } + + # create LineItem if there is none for current lti configuration + lineitem = LtiAgsLineItem.objects.create( + lti_configuration=self, + resource_link_id=self.location, + **default_values + ) + + consumer.enable_ags( + lineitems_url=get_lti_ags_lineitems_url(self.id), + lineitem_url=get_lti_ags_lineitems_url(self.id, lineitem.id) if lineitem else None, + allow_programmatic_grade_interaction=( + lti_advantage_ags_mode == LTI_ADVANTAGE_AGS_PROGRAMMATIC + ) + ) + + def get_lti_advantage_deep_linking_enabled(self): + """ + Return whether LTI 1.3 Advantage Deep Linking is enabled. + """ + if self.lti_configuration.config_store == CONFIG_ON_DB: + return self.lti_configuration.lti_advantage_deep_linking_enabled + elif self.lti_configuration.config_store == CONFIG_EXTERNAL: + return self.lti_configuration.external_config.get('lti_advantage_deep_linking_enabled') + else: + block = compat.load_enough_xblock(self.location_as_usage_key) + return block.lti_advantage_deep_linking_enabled + + def get_lti_advantage_deep_linking_launch_url(self): + """ + Return the LTI 1.3 Advantage Deep Linking launch URL. + """ + if self.lti_configuration.config_store == CONFIG_ON_DB: + return self.lti_configuration.lti_advantage_deep_linking_launch_url + elif self.lti_configuration.config_store == CONFIG_EXTERNAL: + return self.lti_configuration.external_config.get('lti_advantage_deep_linking_launch_url') + else: + block = compat.load_enough_xblock(self.location_as_usage_key) + return block.lti_advantage_deep_linking_launch_url + + def _setup_lti_1p3_deep_linking(self, consumer): + """ + Set up LTI 1.3 Advantage Deep Linking. + """ + try: + if self.get_lti_advantage_deep_linking_enabled(): + consumer.enable_deep_linking( + self.get_lti_advantage_deep_linking_launch_url(), + get_lti_deeplinking_response_url(self.id), + ) + except NotImplementedError as exc: + log.exception("Error setting up LTI 1.3 Advantage Deep Linking: %s", exc) + + def _setup_advantage_conf( + self, + consumer: LtiAdvantageConsumer | LtiProctoringConsumer, + ) -> LtiAdvantageConsumer | LtiProctoringConsumer: + """ + Setup the appropriate LTI consumer class. + """ + if isinstance(consumer, LtiAdvantageConsumer): + # FIXME: move below methods to LtiXBlockConfig and use its id as lti_config_id in urls + self._setup_lti_1p3_ags(consumer) + self._setup_lti_1p3_deep_linking(consumer) + self._setup_lti_1p3_nrps(consumer) + return consumer + @function_trace('lti_consumer.models.LtiXBlockConfig.get_lti_consumer') def get_lti_consumer(self): """ Returns an instanced class of LTI 1.1 or 1.3 consumer. """ - return self.lti_configuration.get_lti_consumer(self.location) + consumer = self.lti_configuration.get_lti_consumer(self.location_as_usage_key or self.location) + return self._setup_advantage_conf(consumer) + + def clean(self): + super().clean() + if ( + self.lti_configuration + and self.lti_configuration.config_store == CONFIG_ON_XBLOCK + and not self.location_as_usage_key + ): + raise ValidationError("Location must not be null if configuration is stored in xblock.") class Meta: app_label = 'lti_consumer' @@ -675,6 +709,16 @@ class LtiAgsLineItem(models.Model): .. no_pii: """ + # LTI xblock Configuration link + # This ties the LineItem to each tool configuration + # and allows easily retrieving LTI credentials for + # API authentication. + lti_xblock_config = models.ForeignKey( + LtiXBlockConfig, + on_delete=models.CASCADE, + null=True, + blank=True + ) # LTI Configuration link # This ties the LineItem to each tool configuration # and allows easily retrieving LTI credentials for @@ -814,6 +858,12 @@ class LtiDlContentItem(models.Model): .. no_pii: """ + lti_xblock_config = models.ForeignKey( + LtiXBlockConfig, + on_delete=models.CASCADE, + null=True, + blank=True + ) # LTI Configuration link # This ties the LineItem to each tool configuration # and allows easily retrieving LTI credentials for diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index cdd42548..aa3873dc 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -25,7 +25,11 @@ from rest_framework.response import Response from rest_framework.status import HTTP_200_OK, HTTP_400_BAD_REQUEST, HTTP_403_FORBIDDEN, HTTP_404_NOT_FOUND -from lti_consumer.api import get_lti_pii_sharing_state_for_course, validate_lti_1p3_launch_data +from lti_consumer.api import ( + get_lti_config_by_location, + get_lti_pii_sharing_state_for_course, + validate_lti_1p3_launch_data, +) from lti_consumer.exceptions import ExternalConfigurationNotFound, LtiError from lti_consumer.filters import get_external_config_from_filter from lti_consumer.lti_1p3.consumer import LtiConsumer1p3, LtiProctoringConsumer @@ -234,13 +238,13 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume ) config_id = launch_data.config_id - location = urllib.parse.unquote(launch_data.resource_link_id) + location = launch_data.resource_link_id try: - lti_xblock_config = LtiXBlockConfig.objects.get( + lti_xblock_config = get_lti_config_by_location( + location, lti_configuration__config_id=config_id, - location=location ) - except (LtiXBlockConfig.DoesNotExist, ValidationError) as exc: + except LtiXBlockConfig.DoesNotExist as exc: log.error( "Invalid config_id '%s' and resource_link_id '%s' for LTI 1.3 Launch callback", config_id, @@ -480,7 +484,7 @@ def access_token_endpoint( tool_key=lti_config.get("lti_1p3_tool_public_key"), tool_keyset_url=lti_config.get("lti_1p3_tool_keyset_url"), ) - except (InvalidKeyError, LtiConfiguration.DoesNotExist, ExternalConfigurationNotFound) as exc: + except (InvalidKeyError, LtiXBlockConfig.DoesNotExist, ExternalConfigurationNotFound) as exc: log.info( "Error while retrieving access token for ID %s: %s", usage_id or lti_config_id or external_id, @@ -590,8 +594,8 @@ def deep_linking_response_endpoint(request, lti_config_id=None): return render(request, 'html/lti-dl/dl_response_saved.html') # If LtiConfiguration doesn't exist, error with 404 status. - except LtiConfiguration.DoesNotExist as exc: - log.info("LtiConfiguration %r does not exist: %s", lti_config_id, exc) + except LtiXBlockConfig.DoesNotExist as exc: + log.info("LtiXBlockConfig %r does not exist: %s", lti_config_id, exc) raise Http404 from exc # If the deep linking content type is not supported except LtiDeepLinkingContentTypeNotSupported as exc: @@ -651,11 +655,21 @@ def deep_linking_content_endpoint(request, lti_config_id): ) try: # Get LTI Configuration - lti_config = LtiConfiguration.objects.get(id=lti_config_id) - except LtiConfiguration.DoesNotExist as exc: - log.info("LtiConfiguration %r does not exist: %s", lti_config_id, exc) + lti_config = LtiXBlockConfig.objects.get(id=lti_config_id) + except LtiXBlockConfig.DoesNotExist as exc: + log.info("LtiXBlockConfig %r does not exist: %s", lti_config_id, exc) raise Http404 from exc + # Currently, deep linking only works in xblock context + if not lti_config.location: + error_msg = f'Missing LTI location for LTI xblock Configuration {lti_config}' + log.warning(error_msg) + return render( + request, + 'html/lti_launch_error.html', + context={"error_msg": error_msg}, + status=HTTP_400_BAD_REQUEST + ) # check if user has proper access block = compat.load_block_as_user(lti_config.location) if not has_block_access(request.user, block, lti_config.location.course_key): @@ -667,7 +681,7 @@ def deep_linking_content_endpoint(request, lti_config_id): raise PermissionDenied # Get all LTI-DL contents - content_items = LtiDlContentItem.objects.filter(lti_configuration=lti_config) + content_items = LtiDlContentItem.objects.filter(lti_xblock_config=lti_config) # If no LTI-DL contents found for current configuration, throw 404 error if not content_items.exists(): @@ -910,10 +924,11 @@ def start_proctoring_assessment_endpoint(request): resource_link_id = decoded_jwt.get('https://purl.imsglobal.org/spec/lti/claim/resource_link', {}).get('id') try: - lti_config = LtiXBlockConfig.objects.filter(lti_configuration__lti_1p3_client_id=iss).first() - if not lti_config: - raise LtiConfiguration.DoesNotExist - except LtiConfiguration.DoesNotExist: + lti_config = get_lti_config_by_location( + resource_link_id, + lti_configuration__lti_1p3_client_id=iss + ) + except LtiXBlockConfig.DoesNotExist: log.error("Invalid iss claim '%s' for LTI 1.3 Proctoring Services start_proctoring_assessment_endpoint" " callback", iss) return render(request, 'html/lti_proctoring_start_error.html', status=HTTP_404_NOT_FOUND) @@ -921,7 +936,7 @@ def start_proctoring_assessment_endpoint(request): lti_consumer = lti_config.get_lti_consumer() if not isinstance(lti_consumer, LtiProctoringConsumer): - log.info("Proctoring Services for LTIConfiguration with config_id %s are not enabled", + log.info("Proctoring Services for LtiXBlockConfig with config_id %s are not enabled", lti_config.lti_configuration.config_id) return render(request, 'html/lti_proctoring_start_error.html', status=HTTP_400_BAD_REQUEST) diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index c52201a3..16c49873 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -101,9 +101,9 @@ def delete_child_lti_configurations(**kwargs): except Exception as e: # pylint: disable=broad-exception-caught log.warning(f"Cannot find xblock for key {usage_key}. Reason: {str(e)}. ") return - id_list = {deleted_block.location} + id_list = {str(deleted_block.location)} for block in compat.yield_dynamic_block_descendants(deleted_block, kwargs.get('user_id')): - id_list.add(block.location) + id_list.add(str(block.location)) LtiXBlockConfig.objects.filter( location__in=id_list @@ -124,7 +124,7 @@ def delete_lti_configuration(**kwargs): return LtiXBlockConfig.objects.filter( - location=xblock_info.usage_key + location=str(xblock_info.usage_key) ).delete() result = LtiConfiguration.objects.filter(ltixblockconfig__isnull=True).delete() log.info(f"Deleted {result} lti configuration objects in library") @@ -141,7 +141,7 @@ def delete_lib_lti_configuration(**kwargs): return LtiXBlockConfig.objects.filter( - location=library_block.usage_key + location=str(library_block.usage_key) ).delete() result = LtiConfiguration.objects.filter(ltixblockconfig__isnull=True).delete() log.info(f"Deleted {result} lti configuration objects in library") diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index 9bb4b0a0..e8e950c9 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -10,6 +10,7 @@ from django.conf import settings from edx_django_utils.cache import TieredCache, get_cache_key +from lti_consumer.data import Lti1p3LaunchData from lti_consumer.lti_1p3.constants import LTI_1P3_CONTEXT_TYPE from lti_consumer.lti_1p3.exceptions import InvalidClaimValue, MissingRequiredClaim from lti_consumer.plugin.compat import ( @@ -31,6 +32,14 @@ CONFIG_ON_XBLOCK = 'CONFIG_ON_XBLOCK' CONFIG_ON_DB = 'CONFIG_ON_DB' CONFIG_EXTERNAL = 'CONFIG_EXTERNAL' +LTI_ADVANTAGE_AGS_DISABLED = 'disabled' +LTI_ADVANTAGE_AGS_DECLARATIVE = 'declarative' +LTI_ADVANTAGE_AGS_PROGRAMMATIC = 'programmatic' +LTI_ADVANTAGE_AGS_CHOICES = [ + (LTI_ADVANTAGE_AGS_DISABLED, 'Disabled'), + (LTI_ADVANTAGE_AGS_DECLARATIVE, 'Allow tools to submit grades only (declarative)'), + (LTI_ADVANTAGE_AGS_PROGRAMMATIC, 'Allow tools to manage and submit grade (programmatic)') +] def compare_config_type(block_field, db_field): @@ -342,7 +351,7 @@ def cache_lti_1p3_launch_data(launch_data): return launch_data_key -def get_data_from_cache(cache_key): +def get_data_from_cache(cache_key) -> Lti1p3LaunchData: """ Return data stored in the cache with the cache key, if it exists. If not, return none.