From 1df0481049ef8b8d826550ead48233eef798f714 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 18 Mar 2026 10:49:28 +0530 Subject: [PATCH 01/43] feat(models): split LTI 1.3 configuration into separate Passport model - Introduce Lti1p3Passport model to centralize LTI 1.3 keys and credentials - Move lti_1p3_internal_private_key, lti_1p3_internal_private_key_id, lti_1p3_internal_public_jwk, lti_1p3_client_id, lti_1p3_tool_public_key, and lti_1p3_tool_keyset_url fields from LtiConfiguration to Lti1p3Passport - Add ForeignKey relationship from LtiConfiguration to Lti1p3Passport - Implement passport-based key generation and retrieval - Add clean() validation to Lti1p3Passport to ensure at least one of lti_1p3_tool_public_key or lti_1p3_tool_keyset_url is set - Update validation in LtiConfiguration.clean() to check for passport presence instead of tool key fields - Refactor get_or_create_local_lti_config() to handle passport creation and sync block/passport key configurations - Update API endpoints to work with passport ID instead of configuration ID - Add admin interface for Lti1p3Passport model - Refactor access_token_endpoint and public_keyset_endpoint to use passport ID - Update API and views to work with the new passport model - Generate migration to remove fields from LtiConfiguration table - Update data migration to copy existing configurations to the new Passport model - Update XBlock to store passport ID instead of config ID - Fix copy-paste issue in resource_link_id generation --- lti_consumer/admin.py | 26 ++ lti_consumer/api.py | 60 ++-- lti_consumer/lti_xblock.py | 12 +- ...sport_lticonfiguration_lti_1p3_passport.py | 68 +++++ .../0021_create_lti_1p3_passport.py | 67 +++++ ...onfiguration_lti_1p3_client_id_and_more.py | 37 +++ lti_consumer/models.py | 281 ++++++++++++------ lti_consumer/plugin/compat.py | 26 +- lti_consumer/plugin/urls.py | 4 +- lti_consumer/plugin/views.py | 127 +++++--- lti_consumer/signals/signals.py | 7 +- lti_consumer/tests/unit/test_api.py | 12 +- lti_consumer/utils.py | 4 +- 13 files changed, 559 insertions(+), 172 deletions(-) create mode 100644 lti_consumer/migrations/0020_lti1p3passport_lticonfiguration_lti_1p3_passport.py create mode 100644 lti_consumer/migrations/0021_create_lti_1p3_passport.py create mode 100644 lti_consumer/migrations/0022_remove_lticonfiguration_lti_1p3_client_id_and_more.py diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py index f800726b..8c759197 100644 --- a/lti_consumer/admin.py +++ b/lti_consumer/admin.py @@ -7,6 +7,7 @@ from lti_consumer.forms import CourseAllowPIISharingInLTIAdminForm from lti_consumer.models import ( CourseAllowPIISharingInLTIFlag, + Lti1p3Passport, LtiAgsLineItem, LtiAgsScore, LtiConfiguration, @@ -14,6 +15,22 @@ ) +class LtiConfigurationInline(admin.TabularInline): + model = LtiConfiguration + extra = 0 + can_delete = False + fields = ('location',) + + def has_change_permission(self, request, obj=None): + return False + + def has_delete_permission(self, request, obj=None): + return False + + def has_add_permission(self, request, obj=None): + return False + + @admin.register(LtiConfiguration) class LtiConfigurationAdmin(admin.ModelAdmin): """ @@ -24,6 +41,15 @@ class LtiConfigurationAdmin(admin.ModelAdmin): readonly_fields = ('location', 'config_id') +@admin.register(Lti1p3Passport) +class Lti1p3PassportAdmin(admin.ModelAdmin): + """ + Admin view for Lti1p3Passport models. + """ + list_display = ('passport_id', 'lti_1p3_client_id') + inlines = [LtiConfigurationInline] + + @admin.register(CourseAllowPIISharingInLTIFlag) class CourseAllowPIISharingInLTIFlagAdmin(KeyedConfigurationModelAdmin): """ diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 6b278e7c..2629bd8c 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -10,21 +10,21 @@ from opaque_keys.edx.keys import CourseKey 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, Lti1p3Passport, LtiConfiguration, LtiDlContentItem 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, block_location, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, external_id=None): +def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfiguration.CONFIG_ON_XBLOCK): """ 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. @@ -34,11 +34,38 @@ def _get_or_create_local_lti_config(lti_version, block_location, the XBlock to be the source of truth for the LTI version, which is a user-centric perspective we've adopted. This allows XBlock users to update the LTI version without needing to update the database. """ + from lti_consumer.plugin.compat import save_xblock # pylint: disable=import-outside-toplevel # 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(location=block.scope_ids.usage_id) + passport = lti_config.lti_1p3_passport + if passport and config_store == LtiConfiguration.CONFIG_ON_XBLOCK: + # Copy keyset url and public key to passport row + if passport.lticonfiguration_set.count() == 1 or ( + passport.lti_1p3_tool_keyset_url == '' and passport.lti_1p3_tool_public_key == '' + ): + # If the passport is only used in one LTI configuration or both fields are empty, + # we update its tool key and url to match block fields. + passport.lti_1p3_tool_public_key = str(block.lti_1p3_tool_public_key) + passport.lti_1p3_tool_keyset_url = str(block.lti_1p3_tool_keyset_url) + passport.save() + elif ( + passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key) + or passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url) + ): + # tool public key or url has changed, we create a new passport to avoid conflicts + # with the existing configuration + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key=str(block.lti_1p3_tool_public_key), + lti_1p3_tool_keyset_url=str(block.lti_1p3_tool_keyset_url), + ) + from lti_consumer.plugin.compat import save_xblock # pylint: disable=import-outside-toplevel + block.lti_1p3_passport_id = str(passport.passport_id) + block.save() + save_xblock(block) lti_config.config_store = config_store - lti_config.external_id = external_id + lti_config.external_id = block.external_config + lti_config.lti_1p3_passport = passport if lti_config.version != lti_version: lti_config.version = lti_version @@ -63,9 +90,9 @@ def _get_lti_config_for_block(block): bits of configuration. """ if block.config_type == 'database': - lti_config = _get_or_create_local_lti_config( + lti_config = get_or_create_local_lti_config( block.lti_version, - block.scope_ids.usage_id, + block, LtiConfiguration.CONFIG_ON_DB, ) elif block.config_type == 'external': @@ -73,16 +100,15 @@ 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_config = get_or_create_local_lti_config( config.get("version"), - block.scope_ids.usage_id, + block, LtiConfiguration.CONFIG_EXTERNAL, - external_id=block.external_config, ) else: - lti_config = _get_or_create_local_lti_config( + lti_config = get_or_create_local_lti_config( block.lti_version, - block.scope_ids.usage_id, + block, LtiConfiguration.CONFIG_ON_XBLOCK, ) return lti_config @@ -140,7 +166,7 @@ def get_lti_1p3_launch_info( if dl_content_items.exists(): deep_linking_content_items = [item.attributes for item in dl_content_items] - config_id = lti_config.config_id + config_id = lti_config.passport_id client_id = lti_config.lti_1p3_client_id deployment_id = "1" diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index f0364d42..145571c3 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -55,6 +55,7 @@ import urllib.parse from collections import namedtuple from importlib import import_module +import uuid import bleach from django.conf import settings @@ -311,6 +312,13 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): ) # LTI 1.3 fields + lti_1p3_passport_id = String( + display_name=_("Lti 1.3 passport ID that points to Lti1p3Passport table"), + scope=Scope.settings, + default="", + help=_("Passport ID for a reusable keys.") + ) + lti_1p3_launch_url = String( display_name=_("Tool Launch URL"), default='', @@ -991,7 +999,7 @@ def resource_link_id(self): i4x-2-3-lti-31de800015cf4afb973356dbe81496df this part of resource_link_id: makes resource_link_id to be unique among courses inside same system. """ - return str(urllib.parse.quote(f"{settings.LMS_BASE}-{self.scope_ids.usage_id.html_id()}")) + return str(self.scope_ids.usage_id) @property def lis_result_sourcedid(self): @@ -1668,7 +1676,7 @@ def get_lti_1p3_launch_data(self): user_id=self.lms_user_id, user_role=self.role, config_id=config_id, - resource_link_id=str(location), + resource_link_id=self.resource_link_id, external_user_id=self.external_user_id, preferred_username=username, name=full_name, diff --git a/lti_consumer/migrations/0020_lti1p3passport_lticonfiguration_lti_1p3_passport.py b/lti_consumer/migrations/0020_lti1p3passport_lticonfiguration_lti_1p3_passport.py new file mode 100644 index 00000000..4b536a61 --- /dev/null +++ b/lti_consumer/migrations/0020_lti1p3passport_lticonfiguration_lti_1p3_passport.py @@ -0,0 +1,68 @@ +# Generated by Django 5.2.12 on 2026-03-17 14:56 + +import uuid + +import django.db.models.deletion +from django.db import migrations, models + +import lti_consumer.models + + +class Migration(migrations.Migration): + dependencies = [ + ('lti_consumer', '0019_mariadb_uuid_conversion'), + ] + + operations = [ + migrations.CreateModel( + name='Lti1p3Passport', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('passport_id', models.UUIDField(default=uuid.uuid4, editable=False, unique=True)), + ( + 'lti_1p3_internal_private_key', + models.TextField(blank=True, help_text="Platform's generated Private key. Keep this value secret."), + ), + ( + 'lti_1p3_internal_private_key_id', + models.CharField(blank=True, help_text="Platform's generated Private key ID", max_length=255), + ), + ( + 'lti_1p3_internal_public_jwk', + models.TextField(blank=True, help_text="Platform's generated JWK keyset."), + ), + ( + 'lti_1p3_client_id', + models.CharField( + default=lti_consumer.models.generate_client_id, + help_text='Client ID used by LTI tool', + max_length=255, + ), + ), + ( + 'lti_1p3_tool_public_key', + models.TextField( + blank=True, + help_text="This is the LTI Tool's public key. This should be provided by the LTI Tool. One of either lti_1p3_tool_public_key or lti_1p3_tool_keyset_url must not be blank.", + verbose_name='LTI 1.3 Tool Public Key', + ), + ), + ( + 'lti_1p3_tool_keyset_url', + models.CharField( + blank=True, + help_text="This is the LTI Tool's JWK (JSON Web Key) Keyset (JWKS) URL. This should be provided by the LTI Tool. One of either lti_1p3_tool_public_key or lti_1p3_tool_keyset_url must not be blank.", + max_length=255, + verbose_name='LTI 1.3 Tool Keyset URL', + ), + ), + ], + ), + migrations.AddField( + model_name='lticonfiguration', + name='lti_1p3_passport', + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to='lti_consumer.lti1p3passport' + ), + ), + ] diff --git a/lti_consumer/migrations/0021_create_lti_1p3_passport.py b/lti_consumer/migrations/0021_create_lti_1p3_passport.py new file mode 100644 index 00000000..1bcaf39a --- /dev/null +++ b/lti_consumer/migrations/0021_create_lti_1p3_passport.py @@ -0,0 +1,67 @@ +# 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. +""" +from django.db import migrations + + +def create_lti_1p3_passport(apps, _): + """Copy config_id from LtiConsumer to LtiConsumerXBlock.""" + from lti_consumer.plugin.compat import load_enough_xblock, save_xblock # pylint: disable=import-outside-toplevel + from lti_consumer.utils import model_to_dict # pylint: disable=import-outside-toplevel + + LtiConfiguration = apps.get_model("lti_consumer", "LtiConfiguration") + Lti1p3Passport = apps.get_model("lti_consumer", "Lti1p3Passport") + + for configuration in LtiConfiguration.objects.all(): + try: + block = load_enough_xblock(configuration.location) + block.lti_1p3_passport_id = str(configuration.config_id) + block.save() + save_xblock(block) + except Exception as e: # pylint: disable=broad-exception-caught + print(f"Failed to copy passport_id for configuration {configuration}: {e}") + values = model_to_dict( + configuration, + include=[ + 'lti_1p3_internal_private_key', + 'lti_1p3_internal_private_key_id', + 'lti_1p3_internal_public_jwk', + 'lti_1p3_client_id', + 'lti_1p3_tool_public_key', + 'lti_1p3_tool_keyset_url', + ], + ) + if block.config_type == "new": + # Data is stored xblock + values.update({ + 'lti_1p3_tool_public_key': block.lti_1p3_tool_public_key, + 'lti_1p3_tool_keyset_url': block.lti_1p3_tool_keyset_url, + }) + passport, _ = Lti1p3Passport.objects.update_or_create( + # Use config_id as passport_id to preserve existing urls that use it. + passport_id=configuration.config_id, + defaults=values, + ) + configuration.lti_1p3_passport = passport + configuration.save() + + +def backwards(*_): + """Reverse the migration only for MariaDB databases.""" + + +class Migration(migrations.Migration): + + dependencies = [ + ('lti_consumer', '0020_lti1p3passport_lticonfiguration_lti_1p3_passport'), + ] + + operations = [ + migrations.RunPython( + code=create_lti_1p3_passport, + reverse_code=backwards, + ), + ] diff --git a/lti_consumer/migrations/0022_remove_lticonfiguration_lti_1p3_client_id_and_more.py b/lti_consumer/migrations/0022_remove_lticonfiguration_lti_1p3_client_id_and_more.py new file mode 100644 index 00000000..9df18419 --- /dev/null +++ b/lti_consumer/migrations/0022_remove_lticonfiguration_lti_1p3_client_id_and_more.py @@ -0,0 +1,37 @@ +# Generated by Django 5.2.12 on 2026-03-18 11:53 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('lti_consumer', '0021_create_lti_1p3_passport'), + ] + + operations = [ + migrations.RemoveField( + model_name='lticonfiguration', + name='lti_1p3_client_id', + ), + migrations.RemoveField( + model_name='lticonfiguration', + name='lti_1p3_internal_private_key', + ), + migrations.RemoveField( + model_name='lticonfiguration', + name='lti_1p3_internal_private_key_id', + ), + migrations.RemoveField( + model_name='lticonfiguration', + name='lti_1p3_internal_public_jwk', + ), + migrations.RemoveField( + model_name='lticonfiguration', + name='lti_1p3_tool_keyset_url', + ), + migrations.RemoveField( + model_name='lticonfiguration', + name='lti_1p3_tool_public_key', + ), + ] diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 5a07a724..e265cf64 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -1,40 +1,41 @@ """ 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 + 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__) @@ -47,6 +48,118 @@ def generate_client_id(): return str(uuid.uuid4()) +class Lti1p3Passport(models.Model): + """ + Model to store LTI 1.3 keys. + """ + passport_id = models.UUIDField(unique=True, default=uuid.uuid4, editable=False) + + lti_1p3_internal_private_key = models.TextField( + blank=True, + help_text=_("Platform's generated Private key. Keep this value secret."), + ) + + lti_1p3_internal_private_key_id = models.CharField( + max_length=255, + blank=True, + help_text=_("Platform's generated Private key ID"), + ) + + lti_1p3_internal_public_jwk = models.TextField( + blank=True, + help_text=_("Platform's generated JWK keyset."), + ) + + lti_1p3_client_id = models.CharField( + max_length=255, + default=generate_client_id, + help_text=_("Client ID used by LTI tool"), + ) + + lti_1p3_tool_public_key = models.TextField( + "LTI 1.3 Tool Public Key", + blank=True, + help_text='This is the LTI Tool\'s public key. This should be provided by the LTI Tool. ' + 'One of either lti_1p3_tool_public_key or lti_1p3_tool_keyset_url must not be blank.' + ) + + lti_1p3_tool_keyset_url = models.CharField( + "LTI 1.3 Tool Keyset URL", + max_length=255, + blank=True, + help_text='This is the LTI Tool\'s JWK (JSON Web Key) Keyset (JWKS) URL. This should be provided by the LTI ' + 'Tool. One of either lti_1p3_tool_public_key or lti_1p3_tool_keyset_url must not be blank.' + ) + + def _generate_lti_1p3_keys_if_missing(self): + """ + Generate LTI 1.3 RSA256 keys if missing. + + If either the public or private key are missing, regenerate them. + The LMS provides a keyset endpoint, so key rotations don't cause any issues + for LTI launches (as long as they have a different kid). + """ + # Generate new private key if not present + if not self.lti_1p3_internal_private_key: + # Private key + private_key = RSA.generate(2048) + self.lti_1p3_internal_private_key_id = str(uuid.uuid4()) + self.lti_1p3_internal_private_key = private_key.export_key('PEM').decode('utf-8') + + # Clear public key if any to allow regeneration + # in the code below + self.lti_1p3_internal_public_jwk = '' + + if not self.lti_1p3_internal_public_jwk: + # Public key + key_handler = PlatformKeyHandler( + key_pem=self.lti_1p3_internal_private_key, + kid=self.lti_1p3_internal_private_key_id, + ) + self.lti_1p3_internal_public_jwk = json.dumps( + key_handler.get_public_jwk() + ) + + # Doesn't do anything if model didn't change + self.save() + + @property + def lti_1p3_private_key(self): + """ + Return the platform's private key used in LTI 1.3 authentication flows. + """ + self._generate_lti_1p3_keys_if_missing() + return self.lti_1p3_internal_private_key + + @property + def lti_1p3_private_key_id(self): + """ + Return the platform's private key ID used in LTI 1.3 authentication flows. + """ + self._generate_lti_1p3_keys_if_missing() + return self.lti_1p3_internal_private_key_id + + @property + def lti_1p3_public_jwk(self): + """ + Return the platform's public keys used in LTI 1.3 authentication flows. + """ + self._generate_lti_1p3_keys_if_missing() + return json.loads(self.lti_1p3_internal_public_jwk) + + def __str__(self): + return f'Lti1p3Passport: {self.passport_id}' + + def clean(self): + if self.lti_1p3_tool_public_key == "" and self.lti_1p3_tool_keyset_url == "": + raise ValidationError({ + "config_store": _( + "LTI Configuration stored on the model for LTI 1.3 must have a value for one of " + "lti_1p3_tool_public_key or lti_1p3_tool_keyset_url." + ), + }) + + class LtiConfiguration(models.Model): """ Model to store LTI Configuration for LTI 1.1 and 1.3. @@ -138,26 +251,11 @@ class LtiConfiguration(models.Model): ) # LTI 1.3 Related variables - lti_1p3_internal_private_key = models.TextField( - blank=True, - help_text=_("Platform's generated Private key. Keep this value secret."), - ) - - lti_1p3_internal_private_key_id = models.CharField( - max_length=255, - blank=True, - help_text=_("Platform's generated Private key ID"), - ) - - lti_1p3_internal_public_jwk = models.TextField( + lti_1p3_passport = models.ForeignKey( + Lti1p3Passport, + on_delete=models.SET_NULL, + null=True, blank=True, - help_text=_("Platform's generated JWK keyset."), - ) - - lti_1p3_client_id = models.CharField( - max_length=255, - default=generate_client_id, - help_text=_("Client ID used by LTI tool"), ) lti_1p3_oidc_url = models.CharField( @@ -177,21 +275,6 @@ class LtiConfiguration(models.Model): 'when the resource is actually launched or loaded.' ) - lti_1p3_tool_public_key = models.TextField( - "LTI 1.3 Tool Public Key", - blank=True, - help_text='This is the LTI Tool\'s public key. This should be provided by the LTI Tool. ' - 'One of either lti_1p3_tool_public_key or lti_1p3_tool_keyset_url must not be blank.' - ) - - lti_1p3_tool_keyset_url = models.CharField( - "LTI 1.3 Tool Keyset URL", - max_length=255, - blank=True, - help_text='This is the LTI Tool\'s JWK (JSON Web Key) Keyset (JWKS) URL. This should be provided by the LTI ' - 'Tool. One of either lti_1p3_tool_public_key or lti_1p3_tool_keyset_url must not be blank.' - ) - lti_1p3_redirect_uris = models.JSONField( "LTI 1.3 Redirect URIs", default=list, @@ -261,14 +344,10 @@ def clean(self): '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.lti_1p3_tool_public_key == "" and self.lti_1p3_tool_keyset_url == "": - raise ValidationError({ - "config_store": _( - "LTI Configuration stored on the model for LTI 1.3 must have a value for one of " - "lti_1p3_tool_public_key or lti_1p3_tool_keyset_url." - ), - }) + if self.version == self.LTI_1P3 and self.config_store == self.CONFIG_ON_DB and not self.lti_1p3_passport: + raise ValidationError({ + "config_store": _('LTI Configuration for 1.3 requires passport to be set.'), + }) if (self.version == self.LTI_1P3 and self.config_store in [self.CONFIG_ON_XBLOCK, self.CONFIG_EXTERNAL] and self.lti_1p3_proctoring_enabled): raise ValidationError({ @@ -315,65 +394,87 @@ def sync_configurations(self): f'Failed to parse main LTI configuration location: {self.location}', ) + def create_lti_1p3_passport(self): + """ + Create an LTI 1.3 Passport configuration for this course instance. + """ + passport = self.lti_1p3_passport + if not passport: + block = compat.load_enough_xblock(self.location) + if block.lti_1p3_passport_id: + passport, created = Lti1p3Passport.objects.get_or_create(passport_id=block.lti_1p3_passport_id) + if created: + log.info("Created new LTI 1.3 Passport %s for %s", passport, self) + else: + log.info("Existing LTI 1.3 Passport %s found for %s", passport, self) + else: + passport = Lti1p3Passport.objects.create() + log.info("Created new LTI 1.3 Passport %s for %s", passport, self) + block.lti_1p3_passport_id = str(passport.passport_id) + block.save() + compat.save_xblock(block) + self.lti_1p3_passport = passport + self.save() + def save(self, *args, **kwargs): self.sync_configurations() super().save(*args, **kwargs) - def _generate_lti_1p3_keys_if_missing(self): + @property + def passport_id(self): """ - Generate LTI 1.3 RSA256 keys if missing. - - If either the public or private key are missing, regenerate them. - The LMS provides a keyset endpoint, so key rotations don't cause any issues - for LTI launches (as long as they have a different kid). + Return the passport ID associated with this configuration instance. """ - # Generate new private key if not present - if not self.lti_1p3_internal_private_key: - # Private key - private_key = RSA.generate(2048) - self.lti_1p3_internal_private_key_id = str(uuid.uuid4()) - self.lti_1p3_internal_private_key = private_key.export_key('PEM').decode('utf-8') - - # Clear public key if any to allow regeneration - # in the code below - self.lti_1p3_internal_public_jwk = '' - - if not self.lti_1p3_internal_public_jwk: - # Public key - key_handler = PlatformKeyHandler( - key_pem=self.lti_1p3_internal_private_key, - kid=self.lti_1p3_internal_private_key_id, - ) - self.lti_1p3_internal_public_jwk = json.dumps( - key_handler.get_public_jwk() - ) - - # Doesn't do anything if model didn't change - self.save() + self.create_lti_1p3_passport() + return self.lti_1p3_passport.passport_id @property def lti_1p3_private_key(self): """ Return the platform's private key used in LTI 1.3 authentication flows. """ - self._generate_lti_1p3_keys_if_missing() - return self.lti_1p3_internal_private_key + self.create_lti_1p3_passport() + return self.lti_1p3_passport.lti_1p3_private_key @property def lti_1p3_private_key_id(self): """ Return the platform's private key ID used in LTI 1.3 authentication flows. """ - self._generate_lti_1p3_keys_if_missing() - return self.lti_1p3_internal_private_key_id + self.create_lti_1p3_passport() + return self.lti_1p3_passport.lti_1p3_private_key_id @property def lti_1p3_public_jwk(self): """ Return the platform's public keys used in LTI 1.3 authentication flows. """ - self._generate_lti_1p3_keys_if_missing() - return json.loads(self.lti_1p3_internal_public_jwk) + self.create_lti_1p3_passport() + return self.lti_1p3_passport.lti_1p3_public_jwk + + @property + def lti_1p3_client_id(self): + """ + Return platform client id from passport + """ + self.create_lti_1p3_passport() + return self.lti_1p3_passport.lti_1p3_client_id + + @property + def lti_1p3_tool_keyset_url(self): + """ + Return tool keyset url from passport + """ + self.create_lti_1p3_passport() + return self.lti_1p3_passport.lti_1p3_tool_keyset_url + + @property + def lti_1p3_tool_public_key(self): + """ + Return tool public key from passport + """ + self.create_lti_1p3_passport() + return self.lti_1p3_passport.lti_1p3_tool_public_key @cached_property def external_config(self): diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index dee1170a..3cbd8501 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): @@ -118,6 +117,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 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(None) + 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/urls.py b/lti_consumer/plugin/urls.py index a6117b71..f837c8a4 100644 --- a/lti_consumer/plugin/urls.py +++ b/lti_consumer/plugin/urls.py @@ -22,7 +22,7 @@ app_name = 'lti_consumer' urlpatterns = [ path( - 'lti_consumer/v1/public_keysets/', + 'lti_consumer/v1/public_keysets/', public_keyset_endpoint, name='lti_consumer.public_keyset_endpoint' ), @@ -44,7 +44,7 @@ name='lti_consumer.launch_gate' ), path( - 'lti_consumer/v1/token/', + 'lti_consumer/v1/token/', access_token_endpoint, name='lti_consumer.access_token' ), diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index 2f753bf3..d571eaaa 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -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 +from lti_consumer.models import Lti1p3Passport, LtiAgsLineItem, LtiConfiguration, LtiDlContentItem 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__) @@ -88,7 +105,7 @@ def has_block_access(user, block, course_key): def public_keyset_endpoint( request, usage_id=None, - lti_config_id=None, + passport_id=None, external_app=None, external_slug=None, ): @@ -100,7 +117,7 @@ def public_keyset_endpoint( and run the proper handler. Arguments: - lti_config_id (UUID): config_id of the LtiConfiguration + passport_id (UUID): passport_id of the Lti1p3Passport usage_id (UsageKey): location of the Block external_app (str): App name of the external LTI configuration external_slug (str): Slug of the external LTI configuration. @@ -111,15 +128,12 @@ def public_keyset_endpoint( external_id = f"{external_app}:{external_slug}" try: - version = None public_jwk = {} if usage_id: lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id)) - version = lti_config.version public_jwk = lti_config.lti_1p3_public_jwk - elif lti_config_id: - lti_config = LtiConfiguration.objects.get(config_id=lti_config_id) - version = lti_config.version + elif passport_id: + lti_config = Lti1p3Passport.objects.get(passport_id=passport_id) public_jwk = lti_config.lti_1p3_public_jwk elif external_app and external_slug: lti_config = get_external_config_from_filter({}, external_id) @@ -127,24 +141,24 @@ def public_keyset_endpoint( if not lti_config: raise ExternalConfigurationNotFound("External LTI configuration not found") - version = lti_config.get("version") public_jwk = lti_config.get("lti_1p3_public_jwk", {}) - if version is None or version != LtiConfiguration.LTI_1P3: - raise LtiError( - "LTI Error: LTI 1.1 blocks do not have a public keyset endpoint." - ) - # Retrieve block's Public JWK # The underlying method will generate a new Private-Public Pair if one does # not exist, and retrieve the values. response = JsonResponse(public_jwk) response['Content-Disposition'] = 'attachment; filename=keyset.json' return response - except (InvalidKeyError, LtiConfiguration.DoesNotExist, ExternalConfigurationNotFound, LtiError) as exc: + except ( + InvalidKeyError, + LtiConfiguration.DoesNotExist, + Lti1p3Passport.DoesNotExist, + ExternalConfigurationNotFound, + LtiError, + ) as exc: log.info( "Error while retrieving keyset for ID %s: %s", - usage_id or lti_config_id or external_id, + usage_id or passport_id or external_id, exc, exc_info=True, ) @@ -396,7 +410,7 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume @require_http_methods(["POST"]) def access_token_endpoint( request, - lti_config_id=None, + passport_id=None, usage_id=None, external_app=None, external_slug=None, @@ -407,7 +421,7 @@ def access_token_endpoint( This endpoint is only valid when a LTI 1.3 tool is being used. Arguments: - lti_config_id (UUID): config_id of the LtiConfiguration + passport_id (UUID): passport_id of the Lti1p3Passport usage_id (UsageKey): location of the Block external_app (str): App name of the external LTI configuration external_slug (str): Slug of the external LTI configuration. @@ -422,23 +436,30 @@ def access_token_endpoint( external_id = f"{external_app}:{external_slug}" try: - version = None lti_consumer = None if usage_id: lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id)) - version = lti_config.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 lti_consumer = lti_config.get_lti_consumer() + elif passport_id: + lti_passport = Lti1p3Passport.objects.get(passport_id=passport_id) + lti_consumer = LtiConsumer1p3( + iss=get_lti_api_base(), + lti_oidc_url=None, + lti_launch_url=None, + client_id=lti_passport.lti_1p3_client_id, + deployment_id=None, + rsa_key=lti_passport.lti_1p3_private_key, + rsa_key_id=lti_passport.lti_1p3_private_key_id, + redirect_uris=None, + tool_key=lti_passport.lti_1p3_tool_public_key, + tool_keyset_url=lti_passport.lti_1p3_tool_keyset_url, + ) elif external_app and external_slug: lti_config = get_external_config_from_filter({}, external_id) if not lti_config: raise ExternalConfigurationNotFound("External LTI configuration not found") - version = lti_config.get("version") # External LTI configurations don't have a get_lti_consumer method # so we initialize the LtiConsumer1p3 class using the external config data. lti_consumer = LtiConsumer1p3( @@ -453,18 +474,20 @@ 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, + LtiConfiguration.DoesNotExist, + Lti1p3Passport.DoesNotExist, + ExternalConfigurationNotFound, + ) as exc: log.info( "Error while retrieving access token for ID %s: %s", - usage_id or lti_config_id or external_id, + usage_id or passport_id or external_id, exc, exc_info=True, ) raise Http404 from exc - if version is None or version != LtiConfiguration.LTI_1P3: - return JsonResponse({"error": "invalid_lti_version"}, status=HTTP_404_NOT_FOUND) - if lti_consumer is None: return JsonResponse({"error": "lti_consumer_not_initialized"}, status=HTTP_404_NOT_FOUND) @@ -883,10 +906,18 @@ 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) + passport = Lti1p3Passport.objects.filter(lti_1p3_client_id=iss).first() + lti_config = LtiConfiguration.objects.get(passport=passport, location=resource_link_id) + except Lti1p3Passport.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) except LtiConfiguration.DoesNotExist: - log.error("Invalid iss claim '%s' for LTI 1.3 Proctoring Services start_proctoring_assessment_endpoint" - " callback", iss) + log.error( + "Invalid resource_link id '%s' for LTI 1.3 Proctoring Services start_proctoring_assessment_endpoint", + resource_link_id, + ) return render(request, 'html/lti_proctoring_start_error.html', status=HTTP_404_NOT_FOUND) lti_consumer = lti_config.get_lti_consumer() diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index c2bfedc2..41848fa1 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -6,7 +6,7 @@ from django.db.models.signals import post_save from django.dispatch import receiver, Signal -from lti_consumer.models import LtiAgsScore +from lti_consumer.models import LtiAgsScore, Lti1p3Passport, LtiConfiguration from lti_consumer.plugin import compat @@ -82,4 +82,9 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl raise exc +@receiver(post_save, sender=LtiConfiguration, dispatch_uid='create_lti_1p3_passport') +def create_lti_1p3_passport(sender, instance: LtiConfiguration, **kwargs): # pylint: disable=unused-argument + instance.create_lti_1p3_passport() + + LTI_1P3_PROCTORING_ASSESSMENT_STARTED = Signal() diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index bbd18f94..89f70081 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -11,7 +11,7 @@ from lti_consumer.api import ( _get_config_by_config_id, - _get_or_create_local_lti_config, + get_or_create_local_lti_config, config_id_for_block, get_end_assessment_return, get_lti_1p3_content_url, @@ -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_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_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_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_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_config( lti_version=lti_version, block_location=location, external_id=None diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index e69cb00a..5b25e39e 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -353,7 +353,7 @@ def check_token_claim(token, claim_key, expected_value=None, invalid_claim_error raise InvalidClaimValue(msg) -def model_to_dict(model_object, exclude=None): +def model_to_dict(model_object, exclude: list[str] | None = None, include: list[str] | None = None): """ Get dictionary from model object. @@ -369,7 +369,7 @@ def model_to_dict(model_object, exclude=None): # Remove private and excluded fields. for key in list(object_fields): - if key.startswith('_') or key in exclude: + if key.startswith('_') or key in exclude or (include and key not in include): object_fields.pop(key, None) return object_fields From 229777d83a3dd403ed2fbf3c0aee049b97319e5c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 19 Mar 2026 16:20:17 +0530 Subject: [PATCH 02/43] feat(lti): add signal handlers for LTI configuration deletion * Add signal handlers to delete LTI configurations when xblocks or library blocks are deleted * Ensure LTI configurations are properly cleaned up when associated blocks are removed from the system * Update documentation for LTI 1.3 configuration changes to inform users about potential regeneration of client IDs and URLs when public keys are changed --- lti_consumer/lti_xblock.py | 6 +++ lti_consumer/plugin/compat.py | 24 ++++++++++++ lti_consumer/signals/signals.py | 68 +++++++++++++++++++++++++++++++-- 3 files changed, 95 insertions(+), 3 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 145571c3..46bc301d 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -374,6 +374,9 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): " requests received have the signature from the tool." "
This is not required when doing LTI 1.3 Launches" " without LTI Advantage nor Basic Outcomes requests." + "

Changing the public key or keyset url will cause the client ID, block keyset url " + "and access token url to be regenerated if they are shared between blocks. " + "Please check and update them in the LTI tool settings if necessary." ), ) lti_1p3_tool_public_key = String( @@ -388,6 +391,9 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): "from the tool." "
This is not required when doing LTI 1.3 Launches without LTI Advantage nor " "Basic Outcomes requests." + "

Changing the public key or keyset url will cause the client ID, block keyset url " + "and access token url to be regenerated if they are shared between blocks. " + "Please check and update them in the LTI tool settings if necessary." ), ) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 3cbd8501..ac29cf7e 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -353,3 +353,27 @@ 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: + # pylint: disable=import-outside-toplevel + 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: + # 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: + return None diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index 41848fa1..61d6876f 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, Lti1p3Passport, LtiConfiguration +from lti_consumer.models import Lti1p3Passport, LtiAgsScore, LtiConfiguration 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') @@ -87,4 +89,64 @@ def create_lti_1p3_passport(sender, instance: LtiConfiguration, **kwargs): # py instance.create_lti_1p3_passport() +@receiver(SignalHandler.pre_item_delete 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 as e: # pylint: disable=broad-exception-caught + log.warning(f"Cannot find xblock for key {usage_key}. Reason: {str(e)}. ") + return + id_list = {str(deleted_block.location)} + for block in compat.yield_dynamic_block_descendants(deleted_block, kwargs.get('user_id')): + id_list.add(str(block.location)) + + LtiConfiguration.objects.filter( + location__in=id_list + ).delete() + log.info(f"Deleted {len(id_list)} lti configurations in modulestore") + result = Lti1p3Passport.objects.filter(lticonfiguration__isnull=True).delete() + log.info(f"Deleted {result} lti 1.3 passport 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 + + LtiConfiguration.objects.filter( + location=str(xblock_info.usage_key) + ).delete() + result = Lti1p3Passport.objects.filter(lticonfiguration__isnull=True).delete() + log.info(f"Deleted {result} lti 1.3 passport 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 + + LtiConfiguration.objects.filter( + location=str(library_block.usage_key) + ).delete() + result = Lti1p3Passport.objects.filter(lticonfiguration__isnull=True).delete() + log.info(f"Deleted {result} lti 1.3 passport objects in library") + + LTI_1P3_PROCTORING_ASSESSMENT_STARTED = Signal() From 1d618311bdfea6a4d41f35d2a92c1b16fc081b45 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 19 Mar 2026 16:39:07 +0530 Subject: [PATCH 03/43] fix(lti): correct spelling and improve logging in signal handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Fixed spelling errors (configurtion → configuration, url → URL) • Improved log messages to be more informative • Used more descriptive variable names (id_list → block_locations) • Maintained consistent code style and import organization --- lti_consumer/lti_xblock.py | 8 ++++---- lti_consumer/signals/signals.py | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 46bc301d..a4118905 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -374,8 +374,8 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): " requests received have the signature from the tool." "
This is not required when doing LTI 1.3 Launches" " without LTI Advantage nor Basic Outcomes requests." - "

Changing the public key or keyset url will cause the client ID, block keyset url " - "and access token url to be regenerated if they are shared between blocks. " + "

Changing the public key or keyset URL will cause the client ID, block keyset URL " + "and access token URL to be regenerated if they are shared between blocks. " "Please check and update them in the LTI tool settings if necessary." ), ) @@ -391,8 +391,8 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): "from the tool." "
This is not required when doing LTI 1.3 Launches without LTI Advantage nor " "Basic Outcomes requests." - "

Changing the public key or keyset url will cause the client ID, block keyset url " - "and access token url to be regenerated if they are shared between blocks. " + "

Changing the public key or keyset URL will cause the client ID, block keyset URL " + "and access token URL to be regenerated if they are shared between blocks. " "Please check and update them in the LTI tool settings if necessary." ), ) diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index 61d6876f..d0999b16 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -32,7 +32,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl # have permissions to. # TODO: This security mechanism will need to be reworked once we enable LTI 1.3 # reusability to allow one configuration to save scores on multiple placements, - # but still locking down access to the items that are using the LTI configurtion. + # but still locking down access to the items that are using the LTI configuration. if line_item.resource_link_id != lti_config.location: log.warning( "LTI tool tried publishing score %r to block %s (outside allowed scope of: %s).", @@ -92,7 +92,7 @@ def create_lti_1p3_passport(sender, instance: LtiConfiguration, **kwargs): # py @receiver(SignalHandler.pre_item_delete if SignalHandler else []) def delete_child_lti_configurations(**kwargs): """ - Delete lti configurtion from database for this block children. + Delete lti configuration from database for this block children. """ usage_key = kwargs.get('usage_key') if usage_key: @@ -103,14 +103,14 @@ 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 = {str(deleted_block.location)} + block_locations = {str(deleted_block.location)} for block in compat.yield_dynamic_block_descendants(deleted_block, kwargs.get('user_id')): - id_list.add(str(block.location)) + block_locations.add(str(block.location)) LtiConfiguration.objects.filter( - location__in=id_list + location__in=block_locations ).delete() - log.info(f"Deleted {len(id_list)} lti configurations in modulestore") + log.info(f"Deleted {len(block_locations)} LTI configurations for block and its children in modulestore") result = Lti1p3Passport.objects.filter(lticonfiguration__isnull=True).delete() log.info(f"Deleted {result} lti 1.3 passport objects in library") @@ -118,7 +118,7 @@ def delete_child_lti_configurations(**kwargs): @receiver(XBLOCK_DELETED) def delete_lti_configuration(**kwargs): """ - Delete lti configurtion from database for this block. + Delete lti configuration from database for this block. """ xblock_info = kwargs.get("xblock_info", None) if not xblock_info or not isinstance(xblock_info, XBlockData): @@ -135,7 +135,7 @@ def delete_lti_configuration(**kwargs): @receiver(LIBRARY_BLOCK_DELETED) def delete_lib_lti_configuration(**kwargs): """ - Delete lti configurtion from database for this library block. + Delete lti configuration from database for this library block. """ library_block = kwargs.get("library_block", None) if not library_block or not isinstance(library_block, LibraryBlockData): From 3979b16b03dd32938c8079ee10d3ae1768cd85aa Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 19 Mar 2026 20:19:18 +0530 Subject: [PATCH 04/43] fix: lint issues --- lti_consumer/admin.py | 3 +++ lti_consumer/api.py | 5 ++--- lti_consumer/lti_1p3/key_handlers.py | 2 +- lti_consumer/lti_xblock.py | 3 +-- lti_consumer/models.py | 2 +- lti_consumer/tests/unit/test_api.py | 10 +++++----- 6 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py index 8c759197..f08d688a 100644 --- a/lti_consumer/admin.py +++ b/lti_consumer/admin.py @@ -16,6 +16,9 @@ class LtiConfigurationInline(admin.TabularInline): + """ + Inline for the LtiConfiguration models in Lti1p3Passport. + """ model = LtiConfiguration extra = 0 can_delete = False diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 2629bd8c..2df3fc63 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -49,8 +49,8 @@ def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfigura passport.lti_1p3_tool_keyset_url = str(block.lti_1p3_tool_keyset_url) passport.save() elif ( - passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key) - or passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url) + passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key) or + passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url) ): # tool public key or url has changed, we create a new passport to avoid conflicts # with the existing configuration @@ -58,7 +58,6 @@ def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfigura lti_1p3_tool_public_key=str(block.lti_1p3_tool_public_key), lti_1p3_tool_keyset_url=str(block.lti_1p3_tool_keyset_url), ) - from lti_consumer.plugin.compat import save_xblock # pylint: disable=import-outside-toplevel block.lti_1p3_passport_id = str(passport.passport_id) block.save() save_xblock(block) diff --git a/lti_consumer/lti_1p3/key_handlers.py b/lti_consumer/lti_1p3/key_handlers.py index 5dd0d159..b8a92f64 100644 --- a/lti_consumer/lti_1p3/key_handlers.py +++ b/lti_consumer/lti_1p3/key_handlers.py @@ -112,7 +112,7 @@ def validate_and_decode(self, token): message = jwt.decode( token, key, - algorithms=['RS256', 'RS512',], + algorithms=['RS256', 'RS512'], options={ 'verify_signature': True, 'verify_aud': False diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index a4118905..5f291d76 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -55,17 +55,16 @@ import urllib.parse from collections import namedtuple from importlib import import_module -import uuid import bleach 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 diff --git a/lti_consumer/models.py b/lti_consumer/models.py index e265cf64..e43500d3 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -155,7 +155,7 @@ def clean(self): raise ValidationError({ "config_store": _( "LTI Configuration stored on the model for LTI 1.3 must have a value for one of " - "lti_1p3_tool_public_key or lti_1p3_tool_keyset_url." + "lti_1p3_tool_public_key or lti_1p3_tool_keyset_url." ), }) diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 89f70081..2d46662b 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -3,21 +3,21 @@ """ 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 from lti_consumer.api import ( _get_config_by_config_id, - get_or_create_local_lti_config, config_id_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, + get_or_create_local_lti_config, validate_lti_1p3_launch_data, ) from lti_consumer.data import Lti1p3LaunchData, Lti1p3ProctoringLaunchData @@ -129,7 +129,7 @@ def test_store_types(self, mapping_pair, mock_external_config): @ddt.ddt -class TestGetOrCreateLocalLtiConfiguration(TestCase): +class TestGetOrCreateLocalLtiConfiguration(Lti1P3TestCase): """ Unit tests for _get_or_create_local_lti_config API method. """ @@ -146,7 +146,7 @@ def test_create_lti_config_if_inexistent(self): # Call API lti_config = get_or_create_local_lti_config( lti_version=lti_version, - block_location=location + block=self.xblock ) # Check if the object was created From 8a268b3155f0b09ed13b824a3fa232ea25161a9f Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 19 Mar 2026 20:22:59 +0530 Subject: [PATCH 05/43] feat: install openedx-events --- ISSUE.md | 39 +++++++++++++++++++++++++++++++++++++++ requirements/base.in | 1 + requirements/base.txt | 18 +++++++++++++++--- requirements/ci.txt | 37 +++++++++++++++++++++++++++++++------ requirements/dev.txt | 19 ++++++++++++++++--- requirements/quality.txt | 23 ++++++++++++++++++----- requirements/test.txt | 31 +++++++++++++++++++++++++------ 7 files changed, 145 insertions(+), 23 deletions(-) create mode 100644 ISSUE.md diff --git a/ISSUE.md b/ISSUE.md new file mode 100644 index 00000000..0970421f --- /dev/null +++ b/ISSUE.md @@ -0,0 +1,39 @@ +# Background + +`LtiResourceLinkRequest` and NRPS membership container requires roles claim and must contain at least one role URI from the published role vocabularies. + +IMS 1.3 spec lists 3 role vocabularies ([link](https://www.imsglobal.org/spec/lti/v1p3#roles-claim)) but does NOT limit their use to specific cases. +1. System roles +2. Institution roles +3. Context roles + +Spec further subdivides each of these into core and non-core URIs and states: `Core roles are those which are most likely to be relevant within LTI and hence vendors should support them by best practice. Vendors may also use the non-core roles, but they may not be widely used` + +Table in the image below shows roles supported by some LTI tools vs Open edX. + +Image + + +Here are some observations from the table above: + +1. All tools support context roles which makes sense because a typical LTI launch happens from within a course and therefore the role of the user in that course is most relevant to the launch. +2. Open edX does NOT support any context roles. This is why integration fails with most of these tools (see [test results](https://openedx.atlassian.net/wiki/spaces/COMM/pages/5985927169/Tools+tested+with+and+status)). +3. The institution roles that Open edX supports are really context roles because they are tied to a course and not an org. + +The following table shows mapping between role URIs from the spec vs roles in Open edX. + +| Course role | Current URIs in roles claim | +|---|---| +| Course Admin | `/institution/person#Instructor` (non-core) | +| Course Staff | `/system/person#Administrator` (core)
`/institution/person#Instructor` (non-core) | +| Limited Staff | `/system/person#Administrator` (core)
`/institution/person#Instructor` (non-core) | +| None | `/institution/person#Student` (core) | +| All other roles | `/institution/person#Student` (core) | + + +# Proposed way forward + +The table below shows current and proposed mapping between Open edX course roles and roles in the spec. Please refer to the spec to get full URIs and other relevant details: https://www.imsglobal.org/spec/lti/v1p3#roles-claim + +Image + diff --git a/requirements/base.in b/requirements/base.in index 97c12bc8..15fc9330 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 f03d5275..4fafe0f0 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -9,7 +9,9 @@ appdirs==1.4.4 asgiref==3.11.1 # via django attrs==26.1.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.13 # djangorestframework # edx-django-utils # jsonfield + # openedx-events # openedx-filters django-appconf==1.2.0 # via django-statici18n @@ -47,14 +50,21 @@ djangorestframework==3.17.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]==4.0.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==3.1.0 # via -r requirements/base.in psutil==7.2.2 diff --git a/requirements/ci.txt b/requirements/ci.txt index 6a368bec..eba20d90 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -26,7 +26,13 @@ astroid==4.0.4 # pylint # pylint-celery attrs==26.1.0 - # via -r test.txt + # 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 test.txt @@ -117,6 +123,7 @@ django==5.2.13 # djangorestframework # edx-django-utils # jsonfield + # openedx-events # openedx-filters # xblock-sdk django-appconf==1.2.0 @@ -150,18 +157,26 @@ docutils==0.22.4 # -r test.txt # readme-renderer edx-ccx-keys==2.0.2 - # via -r test.txt + # via + # -r requirements/test.txt + # openedx-events edx-django-utils==8.0.1 # via # -r test.txt # django-config-models + # openedx-events edx-lint==6.0.0 # via -r test.txt edx-opaque-keys[django]==4.0.0 # via # -r test.txt # edx-ccx-keys + # openedx-events # openedx-filters +fastavro==1.12.1 + # via + # -r requirements/test.txt + # openedx-events filelock==3.25.2 # via # -r tox.txt @@ -185,6 +200,10 @@ idna==3.11 # via # -r test.txt # requests +importlib-metadata==8.8.0 + # via + # -r requirements/test.txt + # keyring isort==8.0.1 # via # -r test.txt @@ -263,9 +282,11 @@ nh3==0.3.4 # -r test.txt # readme-renderer oauthlib==3.3.1 - # via -r test.txt -openedx-filters==3.1.0 - # via -r test.txt + # via -r requirements/test.txt +openedx-events==10.5.0 + # via -r requirements/test.txt +openedx-filters==3.0.0 + # via -r requirements/test.txt packaging==26.0 # via # -r test.txt @@ -486,7 +507,11 @@ xblock==6.0.0 # -r test.txt # xblock-sdk xblock-sdk==0.14.0 - # via -r test.txt + # 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 6df6f215..ed72b0d2 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -13,7 +13,9 @@ asgiref==3.11.1 # -r requirements/base.txt # django attrs==26.1.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.13 # 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==2.0.0 # via -r requirements/dev.in edx-opaque-keys[django]==4.0.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,7 +115,9 @@ markupsafe==3.0.3 # xblock oauthlib==3.3.1 # via -r requirements/base.txt -openedx-filters==3.1.0 +openedx-events==10.5.0 + # via -r requirements/base.txt +openedx-filters==3.0.0 # via -r requirements/base.txt path==16.16.0 # via edx-i18n-tools diff --git a/requirements/quality.txt b/requirements/quality.txt index ce28f717..c9998b12 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -19,7 +19,9 @@ astroid==4.0.4 # pylint # pylint-celery attrs==26.1.0 - # via -r base.txt + # via + # -r requirements/base.txt + # openedx-events binaryornot==0.6.0 # via cookiecutter bleach==6.3.0 @@ -72,6 +74,7 @@ django==5.2.13 # 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 base.txt # pymongo edx-ccx-keys==2.0.2 - # via -r base.txt + # via + # -r requirements/base.txt + # openedx-events edx-django-utils==8.0.1 # via # -r base.txt # django-config-models + # openedx-events edx-lint==6.0.0 # via -r quality.in edx-opaque-keys[django]==4.0.0 # via # -r 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 base.txt @@ -158,9 +169,11 @@ mccabe==0.7.0 mdurl==0.1.2 # via markdown-it-py oauthlib==3.3.1 - # via -r base.txt -openedx-filters==3.1.0 - # via -r base.txt + # via -r requirements/base.txt +openedx-events==10.5.0 + # via -r requirements/base.txt +openedx-filters==3.0.0 + # via -r requirements/base.txt platformdirs==4.9.4 # via pylint psutil==7.2.2 diff --git a/requirements/test.txt b/requirements/test.txt index e3dcdab4..cce1515a 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -21,7 +21,11 @@ astroid==4.0.4 # pylint # pylint-celery attrs==26.1.0 - # via -r base.txt + # 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 @@ -81,6 +85,7 @@ django==5.2.13 # djangorestframework # edx-django-utils # jsonfield + # openedx-events # openedx-filters # xblock-sdk django-appconf==1.2.0 @@ -113,18 +118,26 @@ dnspython==2.8.0 docutils==0.22.4 # via readme-renderer edx-ccx-keys==2.0.2 - # via -r base.txt + # via + # -r requirements/base.txt + # openedx-events edx-django-utils==8.0.1 # via # -r base.txt # django-config-models + # openedx-events edx-lint==6.0.0 # via -r test.in edx-opaque-keys[django]==4.0.0 # via # -r 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 base.txt @@ -136,6 +149,8 @@ id==1.5.0 # via twine idna==3.11 # via requests +importlib-metadata==8.8.0 + # via keyring isort==8.0.1 # via pylint jaraco-classes==3.4.0 @@ -192,9 +207,11 @@ more-itertools==11.0.1 nh3==0.3.4 # via readme-renderer oauthlib==3.3.1 - # via -r base.txt -openedx-filters==3.1.0 - # via -r base.txt + # via -r requirements/base.txt +openedx-events==10.5.0 + # via -r requirements/base.txt +openedx-filters==3.0.0 + # via -r requirements/base.txt packaging==26.0 # via twine platformdirs==4.9.4 @@ -350,7 +367,9 @@ xblock==6.0.0 # -r base.txt # xblock-sdk xblock-sdk==0.14.0 - # via -r test.in + # 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 From 2e6ce8b273a97ce1ce6f65de6db29be6b6296411 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 19 Mar 2026 20:57:04 +0530 Subject: [PATCH 06/43] fix: model label --- lti_consumer/models.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index e43500d3..e823e0d6 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -159,6 +159,9 @@ def clean(self): ), }) + class Meta: + app_label = 'lti_consumer' + class LtiConfiguration(models.Model): """ From 999156db8b695e2c7e883f0f37d53ea798d668b2 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 19 Mar 2026 22:01:36 +0530 Subject: [PATCH 07/43] fix: handle no location --- lti_consumer/models.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index e823e0d6..1c12bb43 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -402,9 +402,11 @@ def create_lti_1p3_passport(self): Create an LTI 1.3 Passport configuration for this course instance. """ passport = self.lti_1p3_passport + block = None if not passport: - block = compat.load_enough_xblock(self.location) - if block.lti_1p3_passport_id: + if self.location: + block = compat.load_enough_xblock(self.location) + if self.location and block.lti_1p3_passport_id: passport, created = Lti1p3Passport.objects.get_or_create(passport_id=block.lti_1p3_passport_id) if created: log.info("Created new LTI 1.3 Passport %s for %s", passport, self) @@ -413,9 +415,10 @@ def create_lti_1p3_passport(self): else: passport = Lti1p3Passport.objects.create() log.info("Created new LTI 1.3 Passport %s for %s", passport, self) - block.lti_1p3_passport_id = str(passport.passport_id) - block.save() - compat.save_xblock(block) + if block: + block.lti_1p3_passport_id = str(passport.passport_id) + block.save() + compat.save_xblock(block) self.lti_1p3_passport = passport self.save() From de50baedfb6d34e5c331538a5b253819ea13c0a7 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 20 Mar 2026 18:05:53 +0530 Subject: [PATCH 08/43] test: fix tests --- lti_consumer/models.py | 2 +- lti_consumer/plugin/views.py | 30 ++++- lti_consumer/tests/test_utils.py | 30 ++++- .../tests/unit/plugin/test_proctoring.py | 25 ++-- lti_consumer/tests/unit/plugin/test_views.py | 93 +++++++-------- .../tests/unit/plugin/test_views_lti_ags.py | 4 +- .../plugin/test_views_lti_deep_linking.py | 13 +-- .../tests/unit/plugin/test_views_lti_nrps.py | 19 +-- lti_consumer/tests/unit/test_api.py | 110 +++++++++--------- lti_consumer/tests/unit/test_filters.py | 9 +- lti_consumer/tests/unit/test_lti_xblock.py | 9 +- lti_consumer/tests/unit/test_models.py | 48 ++++---- lti_consumer/tests/unit/test_signals.py | 4 +- lti_consumer/tests/unit/test_utils.py | 8 +- 14 files changed, 222 insertions(+), 182 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 1c12bb43..c7a672d3 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -406,7 +406,7 @@ def create_lti_1p3_passport(self): if not passport: if self.location: block = compat.load_enough_xblock(self.location) - if self.location and block.lti_1p3_passport_id: + if self.location and block and block.lti_1p3_passport_id: passport, created = Lti1p3Passport.objects.get_or_create(passport_id=block.lti_1p3_passport_id) if created: log.info("Created new LTI 1.3 Passport %s for %s", passport, self) diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index d571eaaa..e4fc77e8 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 @@ -128,21 +128,33 @@ def public_keyset_endpoint( external_id = f"{external_app}:{external_slug}" try: + version = None public_jwk = {} if usage_id: lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id)) + version = lti_config.version public_jwk = lti_config.lti_1p3_public_jwk elif passport_id: - lti_config = Lti1p3Passport.objects.get(passport_id=passport_id) - public_jwk = lti_config.lti_1p3_public_jwk + lti_passport = Lti1p3Passport.objects.get(passport_id=passport_id) + public_jwk = lti_passport.lti_1p3_public_jwk + # TODO: move version inside passport from config + # We just need any any lti_config that is using this passport to check version + lti_config = LtiConfiguration.objects.filter(lti_1p3_passport=lti_passport).first() + version = lti_config.version elif external_app and external_slug: lti_config = get_external_config_from_filter({}, external_id) if not lti_config: raise ExternalConfigurationNotFound("External LTI configuration not found") + version = lti_config.get("version") public_jwk = lti_config.get("lti_1p3_public_jwk", {}) + if version is None or version != LtiConfiguration.LTI_1P3: + raise LtiError( + "LTI Error: LTI 1.1 blocks do not have a public keyset endpoint." + ) + # Retrieve block's Public JWK # The underlying method will generate a new Private-Public Pair if one does # not exist, and retrieve the values. @@ -436,12 +448,18 @@ def access_token_endpoint( external_id = f"{external_app}:{external_slug}" try: + version = None lti_consumer = None if usage_id: lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id)) + version = lti_config.version lti_consumer = lti_config.get_lti_consumer() elif passport_id: lti_passport = Lti1p3Passport.objects.get(passport_id=passport_id) + # TODO: move version inside passport from config + # We just need any any lti_config that is using this passport to check version + lti_config = LtiConfiguration.objects.filter(lti_1p3_passport=lti_passport).first() + version = lti_config.version lti_consumer = LtiConsumer1p3( iss=get_lti_api_base(), lti_oidc_url=None, @@ -460,6 +478,7 @@ def access_token_endpoint( if not lti_config: raise ExternalConfigurationNotFound("External LTI configuration not found") + version = lti_config.get("version") # External LTI configurations don't have a get_lti_consumer method # so we initialize the LtiConsumer1p3 class using the external config data. lti_consumer = LtiConsumer1p3( @@ -488,6 +507,9 @@ def access_token_endpoint( ) raise Http404 from exc + if version is None or version != LtiConfiguration.LTI_1P3: + return JsonResponse({"error": "invalid_lti_version"}, status=HTTP_404_NOT_FOUND) + if lti_consumer is None: return JsonResponse({"error": "lti_consumer_not_initialized"}, status=HTTP_404_NOT_FOUND) @@ -907,7 +929,7 @@ def start_proctoring_assessment_endpoint(request): try: passport = Lti1p3Passport.objects.filter(lti_1p3_client_id=iss).first() - lti_config = LtiConfiguration.objects.get(passport=passport, location=resource_link_id) + lti_config = LtiConfiguration.objects.get(lti_1p3_passport=passport, location=resource_link_id) except Lti1p3Passport.DoesNotExist: log.error( "Invalid iss claim '%s' for LTI 1.3 Proctoring Services start_proctoring_assessment_endpoint callback", iss diff --git a/lti_consumer/tests/test_utils.py b/lti_consumer/tests/test_utils.py index abe0b2bd..d65826da 100644 --- a/lti_consumer/tests/test_utils.py +++ b/lti_consumer/tests/test_utils.py @@ -2,9 +2,10 @@ Utility functions used within unit tests """ -from unittest.mock import Mock import urllib +from unittest.mock import Mock, patch, MagicMock +from django.test.testcases import TestCase from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LocalId from webob import Request @@ -12,7 +13,6 @@ from xblock.fields import ScopeIds from xblock.runtime import DictKeyValueStore, KvsFieldData - FAKE_USER_ID = 'fake_user_id' @@ -108,3 +108,29 @@ def get_mock_lti_configuration(editable): return_value=editable ) return lti_configuration + + +class TestBaseWithPatch(TestCase): + """Base test class that mocks xblock imports""" + + def setUp(self): + """Set up mock patches for xblock imports""" + self.mock_load_xblock = Mock() + self.mock_load_xblock.return_value = getattr(self, 'xblock', None) + self.mock_save_xblock = Mock() + self.mock_save_xblock.return_value = None + + # Patch the imports + self.patcher_load = patch('lti_consumer.plugin.compat.load_enough_xblock', self.mock_load_xblock) + self.patcher_save = patch('lti_consumer.plugin.compat.save_xblock', self.mock_save_xblock) + + self.patcher_load.start() + self.patcher_save.start() + + super().setUp() + + def tearDown(self): + """Clean up patches""" + self.patcher_load.stop() + self.patcher_save.stop() + super().tearDown() diff --git a/lti_consumer/tests/unit/plugin/test_proctoring.py b/lti_consumer/tests/unit/plugin/test_proctoring.py index 4cd5ea3b..0882a18e 100644 --- a/lti_consumer/tests/unit/plugin/test_proctoring.py +++ b/lti_consumer/tests/unit/plugin/test_proctoring.py @@ -7,19 +7,24 @@ import ddt from Cryptodome.PublicKey import RSA from django.contrib.auth import get_user_model -from django.test.testcases import TestCase 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.tests.test_utils import TestBaseWithPatch from lti_consumer.utils import get_data_from_cache @ddt.ddt -class TestLti1p3ProctoringStartProctoringAssessmentEndpoint(TestCase): +class TestLti1p3ProctoringStartProctoringAssessmentEndpoint(TestBaseWithPatch): """Tests for the start_proctoring_assessment_endpoint endpoint.""" def setUp(self): @@ -30,8 +35,10 @@ def setUp(self): # Set up user. self._setup_user() + self.resource_link_id = "block-v1:course+test+2020+type@problem+block@test" # Set up an LtiConfiguration instance for the integration. self.lti_config = LtiConfiguration.objects.create( + location=self.resource_link_id, version=LtiConfiguration.LTI_1P3, lti_1p3_proctoring_enabled=True, config_store=LtiConfiguration.CONFIG_ON_DB, @@ -45,7 +52,7 @@ def setUp(self): self.private_key = RSA.generate(2048) self.public_key = self.private_key.publickey().export_key().decode() - self.lti_config.lti_1p3_tool_public_key = self.public_key + self.lti_config.lti_1p3_passport.lti_1p3_tool_public_key = self.public_key self.lti_config.save() def _setup_user(self): @@ -61,7 +68,7 @@ def _setup_cached_data(self): self.common_cache_key_arguments = { "app": "lti", "user_id": self.user.id, - "resource_link_id": "resource_link_id", + "resource_link_id": self.resource_link_id, } # Cache session_data. @@ -77,7 +84,7 @@ def _setup_cached_data(self): user_id="1", user_role=None, config_id=self.lti_config.config_id, - resource_link_id="resource_link_id", + resource_link_id=self.resource_link_id, proctoring_launch_data=proctoring_launch_data, context_id="course-v1:testU+DemoX+Demo_Course", context_title="http://localhost:2000", @@ -105,7 +112,7 @@ def create_tool_jwt_token(self, **kwargs): "https://purl.imsglobal.org/spec/lti/claim/message_type": "LtiStartAssessment", "https://purl.imsglobal.org/spec/lti/claim/version": "1.3.0", "https://purl.imsglobal.org/spec/lti-ap/claim/session_data": "session_data", - "https://purl.imsglobal.org/spec/lti/claim/resource_link": {"id": "resource_link_id"}, + "https://purl.imsglobal.org/spec/lti/claim/resource_link": {"id": self.resource_link_id}, "https://purl.imsglobal.org/spec/lti-ap/claim/attempt_number": 2, } @@ -273,7 +280,7 @@ def test_lti_1p3_proctoring_assessment_started_signal(self, mock_assessment_star expected_call_args = call( sender=None, attempt_number=2, - resource_link={'id': 'resource_link_id'}, + resource_link={'id': self.resource_link_id}, user_id=self.user.id, ) self.assertEqual(mock_assessment_started_signal.call_args, expected_call_args) diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index 8a197a4c..0915310c 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -2,36 +2,37 @@ Tests for LTI 1.3 endpoint views. """ import json -from unittest.mock import patch, Mock +import urllib.parse +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.tests.test_utils import make_xblock +from lti_consumer.lti_xblock import LtiConsumerXBlock +from lti_consumer.models import LtiConfiguration, LtiDlContentItem +from lti_consumer.tests.test_utils import TestBaseWithPatch, make_xblock from lti_consumer.utils import cache_lti_1p3_launch_data @ddt.ddt -class TestLti1p3KeysetEndpoint(TestCase): +class TestLti1p3KeysetEndpoint(TestBaseWithPatch): """ Test `public_keyset_endpoint` method. """ @@ -102,7 +103,7 @@ def test_public_keyset_endpoint_using_lti_config_id_in_url(self): """ Check that the endpoint is accessible using the ID of the LTI Config object """ - response = self.client.get(f'/lti_consumer/v1/public_keysets/{self.lti_config.config_id}') + response = self.client.get(f'/lti_consumer/v1/public_keysets/{self.lti_config.passport_id}') # Check response self.assertEqual(response.status_code, 200) @@ -137,7 +138,7 @@ def test_public_keyset_endpoint_using_external_id_in_url(self, get_external_conf @ddt.ddt -class TestLti1p3LaunchGateEndpoint(TestCase): +class TestLti1p3LaunchGateEndpoint(TestBaseWithPatch): """ Tests for the `launch_gate_endpoint` method. """ @@ -661,7 +662,7 @@ def test_launch_non_existing_custom_parameters(self, mock_set_custom_parameters) @ddt.ddt -class TestLti1p3AccessTokenEndpoint(TestCase): +class TestLti1p3AccessTokenEndpoint(TestBaseWithPatch): """ Test `access_token_endpoint` method. """ @@ -672,15 +673,7 @@ def setUp(self): location = 'block-v1:course+test+2020+type@problem+block@test' self.config = LtiConfiguration(version=LtiConfiguration.LTI_1P3, location=location) self.config.save() - self.url = reverse('lti_consumer:lti_consumer.access_token', args=[str(self.config.config_id)]) - # Patch settings calls to LMS method - self.mock_client = Mock() - get_lti_consumer_patcher = patch( - 'lti_consumer.plugin.views.LtiConfiguration.get_lti_consumer', - return_value=self.mock_client - ) - self.addCleanup(get_lti_consumer_patcher.stop) - self._mock_xblock_handler = get_lti_consumer_patcher.start() + self.url = reverse('lti_consumer:lti_consumer.access_token', args=[str(self.config.passport_id)]) # Generate RSA and save exports rsa_key = RSA.generate(2048) algo_obj = jwt.get_algorithm_by_name('RS256') @@ -689,6 +682,8 @@ def setUp(self): private_jwk['kid'] = 1 self.key = PyJWK.from_dict(private_jwk) self.public_key = rsa_key.public_key().export_key('PEM') + self.config.lti_1p3_passport.lti_1p3_tool_public_key = self.public_key.decode() + self.config.lti_1p3_passport.save() def get_body(self, token, **overrides): """ @@ -706,40 +701,28 @@ def test_access_token_endpoint(self): """ Check that the access_token generated by the lti_consumer is returned. """ - token = {"access_token": "test-token"} - self.mock_client.access_token.return_value = token + body = urllib.parse.urlencode(self.get_body(create_jwt(self.key, {}))) + response = self.client.post(self.url, data=body, content_type='application/json') - body = self.get_body(create_jwt(self.key, {})) - response = self.client.post(self.url, data=json.dumps(body), content_type='application/json') - self.mock_client.access_token.assert_called_once() - called_args = self.mock_client.access_token.call_args[0] - actual_arg = called_args[0] - actual_dict = json.loads(next(iter(actual_arg.keys()))) - - self.assertEqual(actual_dict, body) self.assertEqual(response.status_code, 200) - self.assertEqual(response.json(), token) + self.assertContains(response, 'access_token') - def test_access_token_endpoint_with_location_in_url(self): + @patch('lti_consumer.plugin.views.LtiConfiguration.get_lti_consumer') + def test_access_token_endpoint_with_location_in_url(self, mock_client): """ Check that the access_token generated by the lti_consumer is returned. """ token = {"access_token": "test-token"} - self.mock_client.access_token.return_value = token + mock_client().access_token.return_value = token url = reverse( 'lti_consumer:lti_consumer.access_token_via_location', args=[str(self.config.location)] ) - body = self.get_body(create_jwt(self.key, {})) - response = self.client.post(url, data=json.dumps(body), content_type='application/json') - - self.mock_client.access_token.assert_called_once() - called_args = self.mock_client.access_token.call_args[0] - actual_arg = called_args[0] - actual_dict = json.loads(next(iter(actual_arg.keys()))) + body = urllib.parse.urlencode(self.get_body(create_jwt(self.key, {}))) + response = self.client.post(url, data=body, content_type='application/json') - self.assertEqual(actual_dict, body) + mock_client().access_token.assert_called_once() self.assertEqual(response.status_code, 200) self.assertEqual(response.json(), token) @@ -825,48 +808,52 @@ def test_verify_lti_version_is_1p3(self): self.config.version = LtiConfiguration.LTI_1P3 self.config.save() - def test_missing_required_claim(self): + @patch('lti_consumer.lti_1p3.consumer.LtiConsumer1p3.access_token') + def test_missing_required_claim(self, access_token_mock): """ Check that 400 is returned when required attributes are missing in the request """ - self.mock_client.access_token = Mock(side_effect=MissingRequiredClaim()) + access_token_mock.side_effect = MissingRequiredClaim() response = self.client.post(self.url) self.assertEqual(response.status_code, 400) self.assertEqual(response.json(), {'error': 'invalid_request'}) - def test_token_errors(self): + @patch('lti_consumer.lti_1p3.consumer.LtiConsumer1p3.access_token') + def test_token_errors(self, access_token_mock): """ Check invalid_grant error is returned when the token is invalid """ - self.mock_client.access_token = Mock(side_effect=MalformedJwtToken()) + access_token_mock.side_effect = MalformedJwtToken() response = self.client.post(self.url) self.assertEqual(response.status_code, 400) self.assertEqual(response.json(), {'error': 'invalid_grant'}) - self.mock_client.access_token = Mock(side_effect=TokenSignatureExpired()) + access_token_mock.side_effect = TokenSignatureExpired() response = self.client.post(self.url) self.assertEqual(response.status_code, 400) self.assertEqual(response.json(), {'error': 'invalid_grant'}) - def test_client_credential_errors(self): + @patch('lti_consumer.lti_1p3.consumer.LtiConsumer1p3.access_token') + def test_client_credential_errors(self, access_token_mock): """ Check invalid_client error is returned when the client credentials are wrong """ - self.mock_client.access_token = Mock(side_effect=NoSuitableKeys()) + access_token_mock.side_effect = NoSuitableKeys() response = self.client.post(self.url) self.assertEqual(response.status_code, 400) self.assertEqual(response.json(), {'error': 'invalid_client'}) - self.mock_client.access_token = Mock(side_effect=UnknownClientId()) + access_token_mock.side_effect = UnknownClientId() response = self.client.post(self.url) self.assertEqual(response.status_code, 400) self.assertEqual(response.json(), {'error': 'invalid_client'}) - def test_unsupported_grant_error(self): + @patch('lti_consumer.lti_1p3.consumer.LtiConsumer1p3.access_token') + def test_unsupported_grant_error(self, access_token_mock): """ Check unsupported_grant_type is returned when the grant type is wrong """ - self.mock_client.access_token = Mock(side_effect=UnsupportedGrantType()) + access_token_mock.side_effect = UnsupportedGrantType() response = self.client.post(self.url) self.assertEqual(response.status_code, 400) self.assertEqual(response.json(), {'error': 'unsupported_grant_type'}) diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py index 0bb24bb6..9889d8a0 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -13,10 +13,10 @@ from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiAgsLineItem, LtiAgsScore, LtiConfiguration -from lti_consumer.tests.test_utils import make_xblock +from lti_consumer.tests.test_utils import TestBaseWithPatch, make_xblock -class LtiAgsLineItemViewSetTestCase(APITransactionTestCase): +class LtiAgsLineItemViewSetTestCase(TestBaseWithPatch, APITransactionTestCase): """ Test `LtiAgsLineItemViewset` Class. """ diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py b/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py index 3c131fe6..f31094ab 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py @@ -1,23 +1,22 @@ """ Tests for LTI Advantage Assignments and Grades Service views. """ -from unittest.mock import patch, Mock - import re +from unittest.mock import Mock, patch + import ddt from Cryptodome.PublicKey import RSA -from rest_framework.test import APITransactionTestCase from rest_framework.exceptions import ValidationError - +from rest_framework.test import APITransactionTestCase from lti_consumer.data import Lti1p3LaunchData -from lti_consumer.utils import cache_lti_1p3_launch_data 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.tests.test_utils import TestBaseWithPatch, make_xblock +from lti_consumer.utils import cache_lti_1p3_launch_data -class LtiDeepLinkingTestCase(APITransactionTestCase): +class LtiDeepLinkingTestCase(TestBaseWithPatch, APITransactionTestCase): """ Test `LtiAgsLineItemViewset` endpoint. """ diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py index e6ae56cf..6df8f9bc 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py @@ -2,14 +2,15 @@ Tests for LTI Names and Role Provisioning Service views. """ from unittest.mock import Mock, patch + from Cryptodome.PublicKey import RSA -from rest_framework.test import APITransactionTestCase from rest_framework.reverse import reverse +from rest_framework.test import APITransactionTestCase from lti_consumer.exceptions import LtiError from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiConfiguration -from lti_consumer.tests.test_utils import make_xblock +from lti_consumer.tests.test_utils import TestBaseWithPatch, make_xblock def generate_mock_members(num, role='student'): @@ -21,9 +22,9 @@ def generate_mock_members(num, role='student'): for i in range(num): member = { 'id': i, - 'username': 'user_{}'.format(i), - 'email': 'user{}@test.com'.format(i), - 'name': 'User {}'.format(i) + 'username': f'user_{i}', + 'email': f'user{i}@test.com', + 'name': f'User {i}' } if role == 'student': @@ -103,7 +104,7 @@ def _get_memberships(course_key): # pylint: disable=unused-argument return _get_memberships -class LtiNrpsTestCase(APITransactionTestCase): +class LtiNrpsTestCase(TestBaseWithPatch, APITransactionTestCase): # noqa: F821 """ Test LtiNrpsViewSet actions """ @@ -170,7 +171,7 @@ def _set_lti_token(self, scopes=None): "scopes": scopes, }) self.client.credentials( - HTTP_AUTHORIZATION="Bearer {}".format(token) + HTTP_AUTHORIZATION=f"Bearer {token}" ) def _parse_link_headers(self, links): @@ -240,7 +241,7 @@ def test_get_without_pii(self, expose_pii_fields_patcher): """ self._set_lti_token('https://purl.imsglobal.org/spec/lti-nrps/scope/contextmembership.readonly') response = self.client.get(self.context_membership_endpoint) - self.assertEqual(response.data['id'], 'http://testserver{}'.format(self.context_membership_endpoint)) + self.assertEqual(response.data['id'], f'http://testserver{self.context_membership_endpoint}') self.assertEqual(len(response.data['members']), 4) self.assertEqual(response.has_header('Link'), False) @@ -268,7 +269,7 @@ def test_get_with_pii(self, expose_pii_fields_patcher): self._set_lti_token('https://purl.imsglobal.org/spec/lti-nrps/scope/contextmembership.readonly') response = self.client.get(self.context_membership_endpoint) - self.assertEqual(response.data['id'], 'http://testserver{}'.format(self.context_membership_endpoint)) + self.assertEqual(response.data['id'], f'http://testserver{self.context_membership_endpoint}') self.assertEqual(len(response.data['members']), 4) self.assertEqual(response.has_header('Link'), False) diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 2d46662b..52a5a9ee 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -23,14 +23,14 @@ from lti_consumer.data import Lti1p3LaunchData, Lti1p3ProctoringLaunchData 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.tests.test_utils import TestBaseWithPatch, make_xblock from lti_consumer.utils import get_data_from_cache # it's convenient to have this in lowercase to compare to URLs _test_config_id = "6c440bf4-face-beef-face-e8bcfb1e53bd" -class Lti1P3TestCase(TestCase): +class Lti1P3TestCase(TestBaseWithPatch): """ Reusable test case for testing LTI 1.3 configurations. """ @@ -40,15 +40,7 @@ def setUp(self): """ self._setup_lti_block() - # Patch compat method to avoid calls to modulestore - patcher = patch( - 'lti_consumer.plugin.compat.load_enough_xblock', - ) - self.addCleanup(patcher.stop) - self._load_block_patch = patcher.start() - self._load_block_patch.return_value = self.xblock - - return super().setUp() + super().setUp() def _setup_lti_block(self): """ @@ -68,14 +60,7 @@ def _setup_lti_block(self): 'lti_1p3_tool_public_key': public_key, } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, xblock_attributes) - - # Create lti configuration - self.lti_config = LtiConfiguration.objects.create( - config_id=_test_config_id, - location=self.xblock.scope_ids.usage_id, - version=LtiConfiguration.LTI_1P3, - config_store=LtiConfiguration.CONFIG_ON_XBLOCK, - ) + self.location = self.xblock.scope_ids.usage_id def _get_lti_1p3_launch_data(self): return Lti1p3LaunchData( @@ -87,7 +72,7 @@ def _get_lti_1p3_launch_data(self): @ddt.ddt -class TestConfigIdForBlock(TestCase): +class TestConfigIdForBlock(Lti1P3TestCase): """ Test config ID for block which is either a simple lookup or creates the config if it hasn't existed before. Config @@ -137,10 +122,9 @@ def test_create_lti_config_if_inexistent(self): """ Check if the API creates a model if no object matching properties is found. """ - location = 'block-v1:course+test+2020+type@problem+block@test' lti_version = LtiConfiguration.LTI_1P3 - # Check that there's nothing in the models + # Check that there's 1 in the models self.assertEqual(LtiConfiguration.objects.all().count(), 0) # Call API @@ -149,26 +133,26 @@ def test_create_lti_config_if_inexistent(self): block=self.xblock ) + self.assertEqual(LtiConfiguration.objects.all().count(), 1) # Check if the object was created self.assertEqual(lti_config.version, lti_version) - self.assertEqual(str(lti_config.location), location) + self.assertEqual(str(lti_config.location), str(self.location)) self.assertEqual(lti_config.config_store, LtiConfiguration.CONFIG_ON_XBLOCK) def test_retrieve_existing(self): """ Check if the API retrieves a model if the configuration matches. """ - location = 'block-v1:course+test+2020+type@problem+block@test' lti_version = LtiConfiguration.LTI_1P1 lti_config = LtiConfiguration.objects.create( - location=location + location=self.location ) # Call API lti_config_retrieved = get_or_create_local_lti_config( lti_version=lti_version, - block_location=location + block=self.xblock ) # Check if the object was created @@ -179,17 +163,15 @@ def test_update_lti_version(self): """ Check if the API retrieves the config and updates the API version. """ - location = 'block-v1:course+test+2020+type@problem+block@test' - lti_config = LtiConfiguration.objects.create( - location=location, + location=self.location, version=LtiConfiguration.LTI_1P1 ) # Call API get_or_create_local_lti_config( lti_version=LtiConfiguration.LTI_1P3, - block_location=location + block=self.xblock ) # Check if the object was created @@ -202,11 +184,10 @@ 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 the config_store field of the LtiConfiguration model appropriately. """ - location = 'block-v1:course+test+2020+type@problem+block@test' lti_version = LtiConfiguration.LTI_1P3 lti_config = get_or_create_local_lti_config( lti_version=lti_version, - block_location=location, + block=self.xblock, config_store=config_store, ) @@ -216,11 +197,10 @@ def test_external_config_values_are_cleared(self): """ Check if the API clears external configuration values when external id is none """ - location = 'block-v1:course+test+2020+type@problem+block@test' lti_version = LtiConfiguration.LTI_1P3 lti_config = LtiConfiguration.objects.create( - location=location, + location=self.location, version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_EXTERNAL, external_id="test_plugin:test-id" @@ -228,13 +208,12 @@ def test_external_config_values_are_cleared(self): get_or_create_local_lti_config( lti_version=lti_version, - block_location=location, - external_id=None + block=self.xblock, ) lti_config.refresh_from_db() self.assertEqual(lti_config.version, lti_version) - self.assertEqual(str(lti_config.location), location) + self.assertEqual(str(lti_config.location), str(self.location)) self.assertEqual(lti_config.config_store, LtiConfiguration.CONFIG_ON_XBLOCK) self.assertEqual(lti_config.external_id, None) @@ -416,11 +395,12 @@ def test_required_start_assessment_url_for_start_proctoring_message_type(self, s ) -class TestGetLti1p3LaunchInfo(TestCase): +class TestGetLti1p3LaunchInfo(Lti1P3TestCase): """ Unit tests for get_lti_1p3_launch_info API method. """ def setUp(self): + self.maxDiff = 30000 # Patch internal method to avoid calls to modulestore patcher = patch( 'lti_consumer.models.LtiConfiguration.get_lti_consumer', @@ -446,9 +426,8 @@ def test_retrieve_with_id(self): """ Check if the API retrieves the launch with id. """ - location = 'block-v1:course+test+2020+type@problem+block@test' lti_config = LtiConfiguration.objects.create( - location=location, + location=self.location, config_id=_test_config_id, ) @@ -468,7 +447,7 @@ def test_retrieve_with_block(self): # Create LTI Config and Deep linking object lti_config = LtiConfiguration.objects.create( - location='block-v1:course+test+2020+type@problem+block@test', + location=self.location, config_id=_test_config_id, ) LtiDlContentItem.objects.create( @@ -486,14 +465,10 @@ def test_retrieve_with_block(self): launch_info, { 'client_id': lti_config.lti_1p3_client_id, - 'keyset_url': 'https://example.com/api/lti_consumer/v1/public_keysets/{}'.format( - lti_config.config_id - ), + 'keyset_url': f'https://example.com/api/lti_consumer/v1/public_keysets/{lti_config.passport_id}', 'deployment_id': '1', 'oidc_callback': 'https://example.com/api/lti_consumer/v1/launch/', - 'token_url': 'https://example.com/api/lti_consumer/v1/token/{}'.format( - lti_config.config_id - ), + 'token_url': f'https://example.com/api/lti_consumer/v1/token/{lti_config.passport_id}', 'deep_linking_launch_url': 'http://example.com', 'deep_linking_content_items': @@ -523,14 +498,10 @@ def test_launch_info_for_lti_config_without_location(self): launch_info, { 'client_id': lti_config.lti_1p3_client_id, - 'keyset_url': 'https://example.com/api/lti_consumer/v1/public_keysets/{}'.format( - lti_config.config_id - ), + 'keyset_url': f'https://example.com/api/lti_consumer/v1/public_keysets/{lti_config.passport_id}', 'deployment_id': '1', 'oidc_callback': 'https://example.com/api/lti_consumer/v1/launch/', - 'token_url': 'https://example.com/api/lti_consumer/v1/token/{}'.format( - lti_config.config_id - ), + 'token_url': f'https://example.com/api/lti_consumer/v1/token/{lti_config.passport_id}', 'deep_linking_launch_url': 'http://example.com', 'deep_linking_content_items': @@ -582,6 +553,20 @@ class TestGetLti1p3LaunchUrl(Lti1P3TestCase): """ Unit tests for LTI 1.3 launch API methods. """ + def setUp(self): + """ + Method to set up test data. + """ + super().setUp() + # Create lti configuration + self.lti_config = LtiConfiguration.objects.create( + config_id=_test_config_id, + location=self.xblock.scope_ids.usage_id, + version=LtiConfiguration.LTI_1P3, + config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + ) + + def test_get_normal_lti_launch_url(self): """ Check if the correct launch url is retrieved for a normal LTI 1.3 launch. @@ -638,6 +623,20 @@ class TestGetLti1p3ContentUrl(Lti1P3TestCase): """ Unit tests for get_lti_1p3_launch_start_url API method. """ + def setUp(self): + """ + Method to set up test data. + """ + super().setUp() + # Create lti configuration + self.lti_config = LtiConfiguration.objects.create( + config_id=_test_config_id, + location=self.xblock.scope_ids.usage_id, + version=LtiConfiguration.LTI_1P3, + config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + ) + + @patch("lti_consumer.api.get_lti_1p3_launch_start_url") def test_lti_content_presentation(self, mock_get_launch_url): """ @@ -704,7 +703,7 @@ def test_lti_content_presentation_multiple_links(self): ) -class TestGetLtiDlContentItemData(TestCase): +class TestGetLtiDlContentItemData(Lti1P3TestCase): """ Unit tests for get_deep_linking_data API method. """ @@ -712,12 +711,11 @@ def setUp(self): """ Set up an empty block configuration. """ + super().setUp() self.lti_config = LtiConfiguration.objects.create( location='block-v1:course+test+2020+type@problem+block@test', ) - return super().setUp() - def test_lti_retrieve_content_item(self): """ Check if the API return the right content item. diff --git a/lti_consumer/tests/unit/test_filters.py b/lti_consumer/tests/unit/test_filters.py index f4afc5c5..40c42e32 100644 --- a/lti_consumer/tests/unit/test_filters.py +++ b/lti_consumer/tests/unit/test_filters.py @@ -2,14 +2,11 @@ Tests for the LTI consumer filters. """ from unittest.mock import patch -from django.test import TestCase, override_settings - +from django.test import TestCase, override_settings from openedx_filters import PipelineStep -from lti_consumer.filters import ( - LTIConfigurationListed, - get_external_config_from_filter -) + +from lti_consumer.filters import LTIConfigurationListed, get_external_config_from_filter class MyTestPipelineStep(PipelineStep): diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index ab81ff95..e3913ae1 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -29,7 +29,7 @@ get_mock_lti_configuration, make_jwt_request, make_request, - make_xblock, + make_xblock, TestBaseWithPatch ) from lti_consumer.utils import resolve_custom_parameter_template @@ -40,7 +40,7 @@ HTML_IFRAME = ' Date: Fri, 20 Mar 2026 18:09:53 +0530 Subject: [PATCH 09/43] fix: lint issues --- lti_consumer/tests/test_utils.py | 2 +- lti_consumer/tests/unit/plugin/test_views.py | 1 - lti_consumer/tests/unit/test_api.py | 22 +++++++++++++------- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/lti_consumer/tests/test_utils.py b/lti_consumer/tests/test_utils.py index d65826da..b1d8df62 100644 --- a/lti_consumer/tests/test_utils.py +++ b/lti_consumer/tests/test_utils.py @@ -3,7 +3,7 @@ """ import urllib -from unittest.mock import Mock, patch, MagicMock +from unittest.mock import Mock, patch from django.test.testcases import TestCase from opaque_keys.edx.keys import CourseKey diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index 0915310c..410b3d58 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -8,7 +8,6 @@ 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 diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 52a5a9ee..89f8b4d6 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -62,14 +62,6 @@ def _setup_lti_block(self): self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, xblock_attributes) self.location = self.xblock.scope_ids.usage_id - def _get_lti_1p3_launch_data(self): - return Lti1p3LaunchData( - user_id="1", - user_role="student", - config_id=self.lti_config.config_id, - resource_link_id="resource_link_id", - ) - @ddt.ddt class TestConfigIdForBlock(Lti1P3TestCase): @@ -566,6 +558,13 @@ def setUp(self): config_store=LtiConfiguration.CONFIG_ON_XBLOCK, ) + def _get_lti_1p3_launch_data(self): + return Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id=self.lti_config.config_id, + resource_link_id="resource_link_id", + ) def test_get_normal_lti_launch_url(self): """ @@ -636,6 +635,13 @@ def setUp(self): config_store=LtiConfiguration.CONFIG_ON_XBLOCK, ) + def _get_lti_1p3_launch_data(self): + return Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id=self.lti_config.config_id, + resource_link_id="resource_link_id", + ) @patch("lti_consumer.api.get_lti_1p3_launch_start_url") def test_lti_content_presentation(self, mock_get_launch_url): From 64d7aaedff32cc7683b1d42e44c2e69b79ac24d9 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 20 Mar 2026 18:18:08 +0530 Subject: [PATCH 10/43] fixup! fix: lint issues --- lti_consumer/tests/unit/plugin/test_views_lti_ags.py | 2 +- lti_consumer/tests/unit/plugin/test_views_lti_nrps.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py index 9889d8a0..08e4acf8 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -16,7 +16,7 @@ from lti_consumer.tests.test_utils import TestBaseWithPatch, make_xblock -class LtiAgsLineItemViewSetTestCase(TestBaseWithPatch, APITransactionTestCase): +class LtiAgsLineItemViewSetTestCase(APITransactionTestCase, TestBaseWithPatch): """ Test `LtiAgsLineItemViewset` Class. """ diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py index 6df8f9bc..2b0c3509 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py @@ -104,7 +104,7 @@ def _get_memberships(course_key): # pylint: disable=unused-argument return _get_memberships -class LtiNrpsTestCase(TestBaseWithPatch, APITransactionTestCase): # noqa: F821 +class LtiNrpsTestCase(APITransactionTestCase, TestBaseWithPatch): # noqa: F821 """ Test LtiNrpsViewSet actions """ From f31fa6148d24265136872fd9b05005db41d4a766 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Sun, 22 Mar 2026 20:15:55 +0530 Subject: [PATCH 11/43] test: add signal tests --- lti_consumer/tests/unit/test_signals.py | 320 +++++++++++++++++++++++- 1 file changed, 310 insertions(+), 10 deletions(-) diff --git a/lti_consumer/tests/unit/test_signals.py b/lti_consumer/tests/unit/test_signals.py index dcefbcd8..421d3702 100644 --- a/lti_consumer/tests/unit/test_signals.py +++ b/lti_consumer/tests/unit/test_signals.py @@ -1,13 +1,20 @@ """ -Tests for LTI Advantage Assignments and Grades Service views. +Tests for LTI Consumer signal handlers. """ from datetime import datetime from unittest.mock import Mock, patch +from ddt import data, ddt, unpack from django.test import TestCase from opaque_keys.edx.keys import UsageKey +from openedx_events.content_authoring.data import LibraryBlockData, XBlockData from lti_consumer.models import LtiAgsLineItem, LtiAgsScore, LtiConfiguration +from lti_consumer.signals.signals import ( + delete_child_lti_configurations, + delete_lib_lti_configuration, + delete_lti_configuration, +) class PublishGradeOnScoreUpdateTest(TestCase): @@ -23,19 +30,28 @@ def setUp(self): "block-v1:course+test+2020+type@problem+block@test" ) - # Create configuration - self.lti_config = LtiConfiguration.objects.create( - location=self.location, - version=LtiConfiguration.LTI_1P3, - ) - # Patch internal method to avoid calls to modulestore self._block_mock = Mock() - compat_mock = patch("lti_consumer.signals.signals.compat") + compat_mock = patch("lti_consumer.models.compat") self.addCleanup(compat_mock.stop) self._compat_mock = compat_mock.start() self._compat_mock.get_user_from_external_user_id.return_value = Mock() self._compat_mock.load_block_as_user.return_value = self._block_mock + self._compat_mock.load_enough_xblock.return_value = self._block_mock + self._block_mock.lti_1p3_passport_id = "e9feb139-4e4c-4fb1-96ee-e614f1e04356" + + signals_compat_mock = patch("lti_consumer.signals.signals.compat") + self.addCleanup(signals_compat_mock.stop) + self._signals_compat_mock = signals_compat_mock.start() + self._signals_compat_mock.get_user_from_external_user_id.return_value = Mock() + self._signals_compat_mock.load_block_as_user.return_value = self._block_mock + self._signals_compat_mock.load_enough_xblock.return_value = self._block_mock + self._block_mock.lti_1p3_passport_id = "e9feb139-4e4c-4fb1-96ee-e614f1e04356" + # Create configuration + self.lti_config = LtiConfiguration.objects.create( + location=self.location, + version=LtiConfiguration.LTI_1P3, + ) def test_grade_publish_not_done_when_wrong_line_item(self): """ @@ -96,5 +112,289 @@ def test_grade_publish(self): # Check that methods to save grades are called self._block_mock.set_user_module_score.assert_called_once() - self._compat_mock.get_user_from_external_user_id.assert_called_once() - self._compat_mock.load_block_as_user.assert_called_once() + self._signals_compat_mock.get_user_from_external_user_id.assert_called_once() + self._signals_compat_mock.load_block_as_user.assert_called_once() + + +@ddt +class TestDeleteLtiConfiguration(TestCase): + """Tests for delete_lti_configuration function.""" + + def setUp(self): + """Set up test fixtures.""" + self.mock_usage_key = UsageKey.from_string("block-v1:course+101+2024+type@lti_consumer+block@test") + + self.xblock_data = Mock(spec=XBlockData) + self.xblock_data.usage_key = self.mock_usage_key + + @patch('lti_consumer.signals.signals.Lti1p3Passport') + @patch('lti_consumer.signals.signals.LtiConfiguration') + @patch('lti_consumer.signals.signals.log') + def test_delete_lti_configuration_success(self, mock_log, mock_lti_config, mock_passport): + """Test successful deletion with various passport counts.""" + mock_lti_config.objects.filter.return_value.delete.return_value = None + + # Test with multiple passports deleted + mock_passport.objects.filter.return_value.delete.return_value = (5, {'Lti1p3Passport': 5}) + delete_lti_configuration(xblock_info=self.xblock_data) + + mock_lti_config.objects.filter.assert_called_with(location=str(self.xblock_data.usage_key)) + mock_passport.objects.filter.assert_called_with(lticonfiguration__isnull=True) + assert mock_log.info.call_count == 1 + assert "5" in mock_log.info.call_args[0][0] + + # Reset and test with no passports deleted + mock_log.reset_mock() + mock_lti_config.reset_mock() + mock_passport.reset_mock() + mock_lti_config.objects.filter.return_value.delete.return_value = None + mock_passport.objects.filter.return_value.delete.return_value = (0, {}) + + delete_lti_configuration(xblock_info=self.xblock_data) + assert "0" in mock_log.info.call_args[0][0] + + @data( + None, + "invalid_string", + {"usage_key": "test"}, + 123, + ) + @patch('lti_consumer.signals.signals.log') + def test_delete_lti_configuration_invalid_input(self, invalid_input, mock_log): + """Test with invalid xblock_info inputs.""" + delete_lti_configuration(xblock_info=invalid_input) + mock_log.error.assert_called_once_with("Received null or incorrect data for event") + + @patch('lti_consumer.signals.signals.log') + def test_delete_lti_configuration_missing_xblock_info(self, mock_log): + """Test with missing xblock_info kwarg.""" + delete_lti_configuration() + mock_log.error.assert_called_once_with("Received null or incorrect data for event") + + @patch('lti_consumer.signals.signals.Lti1p3Passport') + @patch('lti_consumer.signals.signals.LtiConfiguration') + @patch('lti_consumer.signals.signals.log') + def test_delete_lti_configuration_extra_kwargs_ignored(self, mock_log, mock_lti_config, mock_passport): + """Test that extra kwargs are safely ignored.""" + mock_lti_config.objects.filter.return_value.delete.return_value = None + mock_passport.objects.filter.return_value.delete.return_value = (0, {}) + + delete_lti_configuration( + xblock_info=self.xblock_data, + extra_param="ignored", + another_param=123 + ) + + mock_lti_config.objects.filter.assert_called_once() + mock_log.error.assert_not_called() + + +@ddt +class TestDeleteChildLtiConfigurations(TestCase): + """Tests for delete_child_lti_configurations function.""" + def setUp(self): + """Set up test fixtures.""" + self.usage_key = UsageKey.from_string("block-v1:course+test+2020+type@problem+block@parent") + + def _create_child_block(self, block_id): + """Helper to create a mock child block.""" + child = Mock() + child.location = UsageKey.from_string( + f"block-v1:course+test+2020+type@problem+block@{block_id}" + ) + return child + + def _setup_mocks(self, children_count=0, passport_count=0, load_error=None): + """Helper to setup common mock patches.""" + parent_block = Mock() + parent_block.location = self.usage_key + + children = [self._create_child_block(f"child{i}") for i in range(children_count)] + + patches = { + 'compat': patch("lti_consumer.signals.signals.compat"), + 'lti_config': patch("lti_consumer.signals.signals.LtiConfiguration"), + 'passport': patch("lti_consumer.signals.signals.Lti1p3Passport"), + 'log': patch("lti_consumer.signals.signals.log"), + } + + mocks = {name: p.start() for name, p in patches.items()} + self.addCleanup(lambda: [p.stop() for p in patches.values()]) + + if load_error: + mocks['compat'].load_enough_xblock.side_effect = load_error + else: + mocks['compat'].load_enough_xblock.return_value = parent_block + mocks['compat'].yield_dynamic_block_descendants.return_value = children + + mocks['lti_config'].objects.filter.return_value.delete.return_value = None + mocks['passport'].objects.filter.return_value.delete.return_value = ( + passport_count, + {'Lti1p3Passport': passport_count} if passport_count else {} + ) + + return mocks, parent_block, children + + @data( + (0, 0), # no children, no passports deleted + (2, 2), # 2 children, 2 passports deleted + (1, 5), # 1 child, 5 passports deleted + ) + @unpack + def test_delete_child_lti_configurations_success(self, children_count, passport_count): + """Test successful deletion with various child/passport counts.""" + + mocks, parent_block, children = self._setup_mocks(children_count, passport_count) + + delete_child_lti_configurations(usage_key=self.usage_key, user_id="test_user") + + # Verify load_enough_xblock called with stripped branch + mocks['compat'].load_enough_xblock.assert_called_once_with(self.usage_key.for_branch(None)) + + # Verify descendants iterator called + mocks['compat'].yield_dynamic_block_descendants.assert_called_once_with(parent_block, "test_user") + + # Verify correct locations in filter + call_args = mocks['lti_config'].objects.filter.call_args + locations = call_args[1]['location__in'] + assert len(locations) == children_count + 1 # parent + children + assert str(parent_block.location) in locations + for child in children: + assert str(child.location) in locations + + # Verify deletion logged + assert mocks['log'].info.call_count >= 1 + + @data( + None, + UsageKey.from_string("block-v1:course+test+2020+type@problem+block@parent").for_branch("branch"), + ) + def test_delete_child_lti_configurations_invalid_usage_key(self, usage_key): + """Test with None or missing usage_key.""" + mocks, _, _ = self._setup_mocks() + + if usage_key is None: + delete_child_lti_configurations(usage_key=None) + else: + # Test branch stripping + delete_child_lti_configurations(usage_key=usage_key, user_id="test_user") + mocks['compat'].load_enough_xblock.assert_called_once_with(self.usage_key.for_branch(None)) + + @data( + Exception("Block not found"), + ValueError("Invalid block"), + RuntimeError("Load failed"), + ) + def test_delete_child_lti_configurations_load_block_fails(self, error): + """Test when load_enough_xblock raises exceptions.""" + mocks, _, _ = self._setup_mocks(load_error=error) + + delete_child_lti_configurations(usage_key=self.usage_key, user_id="test_user") + + # Verify warning logged with error details + mocks['log'].warning.assert_called_once() + warning_msg = mocks['log'].warning.call_args[0][0] + assert "Cannot find xblock for key" in warning_msg + assert str(error) in warning_msg + + # Verify no deletion attempted + mocks['lti_config'].objects.filter.assert_not_called() + + def test_delete_child_lti_configurations_no_usage_key(self): + """Test when usage_key is not provided.""" + with patch("lti_consumer.signals.signals.log") as mock_log: + delete_child_lti_configurations(usage_key=None) + + # Should return early without logging + mock_log.warning.assert_not_called() + mock_log.info.assert_not_called() + + +@ddt +class TestDeleteLibLtiConfiguration(TestCase): + """Tests for delete_lib_lti_configuration function.""" + + def setUp(self): + """Set up test fixtures.""" + self.library_block = Mock(spec=LibraryBlockData) + self.library_block.usage_key = UsageKey.from_string( + "lb:TestOrg:TestLibrary:problem:test_problem" + ) + + def _setup_mocks(self, passport_count=0): + """Helper to setup common mock patches.""" + patches = { + 'lti_config': patch("lti_consumer.signals.signals.LtiConfiguration"), + 'passport': patch("lti_consumer.signals.signals.Lti1p3Passport"), + 'log': patch("lti_consumer.signals.signals.log"), + } + + mocks = {name: p.start() for name, p in patches.items()} + self.addCleanup(lambda: [p.stop() for p in patches.values()]) + + mocks['lti_config'].objects.filter.return_value.delete.return_value = None + mocks['passport'].objects.filter.return_value.delete.return_value = ( + passport_count, + {'Lti1p3Passport': passport_count} if passport_count else {} + ) + + return mocks + + @data(0, 1, 5, 3) + def test_delete_lib_lti_configuration_success(self, passport_count): + """Test successful deletion with various passport counts.""" + mocks = self._setup_mocks(passport_count) + + delete_lib_lti_configuration(library_block=self.library_block) + + # Verify LtiConfiguration filter called with correct location + mocks['lti_config'].objects.filter.assert_called_once_with( + location=str(self.library_block.usage_key) + ) + + # Verify orphaned passports deleted + mocks['passport'].objects.filter.assert_called_once_with(lticonfiguration__isnull=True) + + # Verify info logged with passport count + mocks['log'].info.assert_called_once() + log_msg = mocks['log'].info.call_args[0][0] + assert str(passport_count) in log_msg + if passport_count > 0: + assert "lti 1.3 passport" in log_msg + + @data( + None, + "invalid_string", + {"usage_key": "test"}, + 123, + Mock(), # Mock without LibraryBlockData spec + ) + def test_delete_lib_lti_configuration_invalid_input(self, library_block): + """Test with invalid library_block inputs.""" + mocks = self._setup_mocks() + + delete_lib_lti_configuration(library_block=library_block) + + mocks['log'].error.assert_called_once_with("Received null or incorrect data for event") + mocks['lti_config'].objects.filter.assert_not_called() + + def test_delete_lib_lti_configuration_missing_library_block(self): + """Test when library_block kwarg is missing.""" + mocks = self._setup_mocks() + + delete_lib_lti_configuration() + + mocks['log'].error.assert_called_once_with("Received null or incorrect data for event") + + def test_delete_lib_lti_configuration_extra_kwargs_ignored(self): + """Test that extra kwargs are safely ignored.""" + mocks = self._setup_mocks(passport_count=2) + + delete_lib_lti_configuration( + library_block=self.library_block, + extra_param="ignored", + another_param=123 + ) + + mocks['lti_config'].objects.filter.assert_called_once() + mocks['log'].info.assert_called_once() From b34d803aa4cf42d8b8aa2d43619641e22d44cd26 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Sun, 22 Mar 2026 20:47:58 +0530 Subject: [PATCH 12/43] fix: coverage --- .coveragerc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.coveragerc b/.coveragerc index 970aebfd..6ef2c88c 100644 --- a/.coveragerc +++ b/.coveragerc @@ -2,4 +2,7 @@ [run] data_file = .coverage source = lti_consumer -omit = */urls.py +omit = + */urls.py + lti_consumer/migrations/* + lti_consumer/plugin/compat.py From 3ac8dd36e18eded713f2a06df929da1ff35f07ac Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 24 Mar 2026 19:20:08 +0530 Subject: [PATCH 13/43] refactor(models): rename create_lti_1p3_passport to get_or_create_lti_1p3_passport --- lti_consumer/models.py | 16 ++++++++-------- lti_consumer/plugin/compat.py | 4 +--- lti_consumer/signals/signals.py | 2 +- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index c7a672d3..f118d71f 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -397,7 +397,7 @@ def sync_configurations(self): f'Failed to parse main LTI configuration location: {self.location}', ) - def create_lti_1p3_passport(self): + def get_or_create_lti_1p3_passport(self): """ Create an LTI 1.3 Passport configuration for this course instance. """ @@ -431,7 +431,7 @@ def passport_id(self): """ Return the passport ID associated with this configuration instance. """ - self.create_lti_1p3_passport() + self.get_or_create_lti_1p3_passport() return self.lti_1p3_passport.passport_id @property @@ -439,7 +439,7 @@ def lti_1p3_private_key(self): """ Return the platform's private key used in LTI 1.3 authentication flows. """ - self.create_lti_1p3_passport() + self.get_or_create_lti_1p3_passport() return self.lti_1p3_passport.lti_1p3_private_key @property @@ -447,7 +447,7 @@ def lti_1p3_private_key_id(self): """ Return the platform's private key ID used in LTI 1.3 authentication flows. """ - self.create_lti_1p3_passport() + self.get_or_create_lti_1p3_passport() return self.lti_1p3_passport.lti_1p3_private_key_id @property @@ -455,7 +455,7 @@ def lti_1p3_public_jwk(self): """ Return the platform's public keys used in LTI 1.3 authentication flows. """ - self.create_lti_1p3_passport() + self.get_or_create_lti_1p3_passport() return self.lti_1p3_passport.lti_1p3_public_jwk @property @@ -463,7 +463,7 @@ def lti_1p3_client_id(self): """ Return platform client id from passport """ - self.create_lti_1p3_passport() + self.get_or_create_lti_1p3_passport() return self.lti_1p3_passport.lti_1p3_client_id @property @@ -471,7 +471,7 @@ def lti_1p3_tool_keyset_url(self): """ Return tool keyset url from passport """ - self.create_lti_1p3_passport() + self.get_or_create_lti_1p3_passport() return self.lti_1p3_passport.lti_1p3_tool_keyset_url @property @@ -479,7 +479,7 @@ def lti_1p3_tool_public_key(self): """ Return tool public key from passport """ - self.create_lti_1p3_passport() + self.get_or_create_lti_1p3_passport() return self.lti_1p3_passport.lti_1p3_tool_public_key @cached_property diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index ac29cf7e..aee3b7c2 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -119,9 +119,7 @@ def load_enough_xblock(location: UsageKey): # pragma: nocover 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. + Save xblock in modulestore or contentstore. """ # pylint: disable=import-error,import-outside-toplevel from openedx.core.djangoapps.xblock import api as xblock_api diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index d0999b16..2d32ee33 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -86,7 +86,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl @receiver(post_save, sender=LtiConfiguration, dispatch_uid='create_lti_1p3_passport') def create_lti_1p3_passport(sender, instance: LtiConfiguration, **kwargs): # pylint: disable=unused-argument - instance.create_lti_1p3_passport() + instance.get_or_create_lti_1p3_passport() @receiver(SignalHandler.pre_item_delete if SignalHandler else []) From fb89f250af1f71820b05bc26e39b23dbc595ec52 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 26 Mar 2026 11:33:58 +0530 Subject: [PATCH 14/43] fix: copy-paste bug when both public key and keyset url is specified --- lti_consumer/api.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 2df3fc63..4d6c21ca 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -6,6 +6,7 @@ """ import json +import logging from opaque_keys.edx.keys import CourseKey @@ -23,6 +24,8 @@ get_lti_deeplinking_content_url, ) +log = logging.getLogger(__name__) + def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfiguration.CONFIG_ON_XBLOCK): """ @@ -48,9 +51,12 @@ def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfigura passport.lti_1p3_tool_public_key = str(block.lti_1p3_tool_public_key) passport.lti_1p3_tool_keyset_url = str(block.lti_1p3_tool_keyset_url) passport.save() + log.info("Updated LTI passport for %s to match block fields", block.scope_ids.usage_id) elif ( - passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key) or - passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url) + (block.lti_1p3_tool_key_mode == 'public_key' and + passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key)) or + (block.lti_1p3_tool_key_mode == 'keyset_url' and + passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url)) ): # tool public key or url has changed, we create a new passport to avoid conflicts # with the existing configuration @@ -60,6 +66,7 @@ def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfigura ) block.lti_1p3_passport_id = str(passport.passport_id) block.save() + log.info("Created new LTI passport for %s with new keyset and public key", block.scope_ids.usage_id) save_xblock(block) lti_config.config_store = config_store From ce8ab7c0532d560e29344cc1e16af147c617dc67 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 26 Mar 2026 11:44:59 +0530 Subject: [PATCH 15/43] fix: lint issues --- lti_consumer/api.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 4d6c21ca..996e69f3 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -53,10 +53,11 @@ def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfigura passport.save() log.info("Updated LTI passport for %s to match block fields", block.scope_ids.usage_id) elif ( - (block.lti_1p3_tool_key_mode == 'public_key' and - passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key)) or - (block.lti_1p3_tool_key_mode == 'keyset_url' and - passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url)) + block.lti_1p3_tool_key_mode == 'public_key' and + passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key) + ) or ( + block.lti_1p3_tool_key_mode == 'keyset_url' and + passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url) ): # tool public key or url has changed, we create a new passport to avoid conflicts # with the existing configuration From 84c2e78902a0de4b638bd169bc978da64af94f7b Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 26 Mar 2026 12:10:59 +0530 Subject: [PATCH 16/43] test: improve coverage --- .coveragerc | 2 - lti_consumer/admin.py | 6 +- .../0021_create_lti_1p3_passport.py | 2 +- lti_consumer/plugin/compat.py | 4 +- lti_consumer/tests/unit/test_api.py | 147 +++++++++++++++++- 5 files changed, 152 insertions(+), 9 deletions(-) diff --git a/.coveragerc b/.coveragerc index 6ef2c88c..f5c79646 100644 --- a/.coveragerc +++ b/.coveragerc @@ -4,5 +4,3 @@ data_file = .coverage source = lti_consumer omit = */urls.py - lti_consumer/migrations/* - lti_consumer/plugin/compat.py diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py index f08d688a..57cda35b 100644 --- a/lti_consumer/admin.py +++ b/lti_consumer/admin.py @@ -24,13 +24,13 @@ class LtiConfigurationInline(admin.TabularInline): can_delete = False fields = ('location',) - def has_change_permission(self, request, obj=None): + def has_change_permission(self, request, obj=None): # pragma: nocover return False - def has_delete_permission(self, request, obj=None): + def has_delete_permission(self, request, obj=None): # pragma: nocover return False - def has_add_permission(self, request, obj=None): + def has_add_permission(self, request, obj=None): # pragma: nocover return False diff --git a/lti_consumer/migrations/0021_create_lti_1p3_passport.py b/lti_consumer/migrations/0021_create_lti_1p3_passport.py index 1bcaf39a..f5d97cfa 100644 --- a/lti_consumer/migrations/0021_create_lti_1p3_passport.py +++ b/lti_consumer/migrations/0021_create_lti_1p3_passport.py @@ -7,7 +7,7 @@ from django.db import migrations -def create_lti_1p3_passport(apps, _): +def create_lti_1p3_passport(apps, _): # pragma: nocover """Copy config_id from LtiConsumer to LtiConsumerXBlock.""" from lti_consumer.plugin.compat import load_enough_xblock, save_xblock # pylint: disable=import-outside-toplevel from lti_consumer.utils import model_to_dict # pylint: disable=import-outside-toplevel diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index aee3b7c2..922659b3 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -353,7 +353,7 @@ def nrps_pii_disallowed(): settings.LTI_NRPS_DISALLOW_PII is True) -def get_signal_handler(): +def get_signal_handler(): # pragma: nocover """ Import the signal handler from LMS """ @@ -365,7 +365,7 @@ def get_signal_handler(): return None -def yield_dynamic_block_descendants(block, user_id): +def yield_dynamic_block_descendants(block, user_id): # pragma: nocover """ Import and run `yield_dynamic_block_descendants` from LMS """ diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 89f8b4d6..8c7ddcf1 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -8,6 +8,7 @@ from Cryptodome.PublicKey import RSA from django.test.testcases import TestCase from edx_django_utils.cache import get_cache_key +from opaque_keys.edx.keys import UsageKey from lti_consumer.api import ( _get_config_by_config_id, @@ -22,7 +23,7 @@ ) from lti_consumer.data import Lti1p3LaunchData, Lti1p3ProctoringLaunchData from lti_consumer.lti_xblock import LtiConsumerXBlock -from lti_consumer.models import LtiConfiguration, LtiDlContentItem +from lti_consumer.models import Lti1p3Passport, LtiConfiguration, LtiDlContentItem from lti_consumer.tests.test_utils import TestBaseWithPatch, make_xblock from lti_consumer.utils import get_data_from_cache @@ -61,6 +62,7 @@ def _setup_lti_block(self): } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, xblock_attributes) self.location = self.xblock.scope_ids.usage_id + self.other_location = UsageKey.from_string("block-v1:course+101+2024+type@lti_consumer+block@test") @ddt.ddt @@ -209,6 +211,149 @@ def test_external_config_values_are_cleared(self): self.assertEqual(lti_config.config_store, LtiConfiguration.CONFIG_ON_XBLOCK) self.assertEqual(lti_config.external_id, None) + def test_passport_created_on_first_call(self): + """ + Check if a passport is created when none exists. + """ + lti_version = LtiConfiguration.LTI_1P3 + + lti_config = get_or_create_local_lti_config( + lti_version=lti_version, + block=self.xblock + ) + + self.assertIsNotNone(lti_config.lti_1p3_passport) + + def test_passport_updated_when_single_use(self): + """ + Check if passport is updated when it's only used by one config. + """ + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key='old_key', + lti_1p3_tool_keyset_url='old_url' + ) + LtiConfiguration.objects.create( + location=self.location, + lti_1p3_passport=passport + ) + + self.xblock.lti_1p3_tool_public_key = 'new_key' + self.xblock.lti_1p3_tool_keyset_url = 'new_url' + + get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock + ) + + passport.refresh_from_db() + self.assertEqual(passport.lti_1p3_tool_public_key, 'new_key') + self.assertEqual(passport.lti_1p3_tool_keyset_url, 'new_url') + + def test_new_passport_created_on_key_conflict(self): + """ + Check if a new passport is created when block key differs and passport is shared. + """ + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key='shared_key', + lti_1p3_tool_keyset_url='shared_url' + ) + # Create two configs sharing the passport + LtiConfiguration.objects.create(location=self.location, lti_1p3_passport=passport) + LtiConfiguration.objects.create(location=self.other_location, lti_1p3_passport=passport) + + self.xblock.lti_1p3_tool_key_mode = 'public_key' + self.xblock.lti_1p3_tool_public_key = 'new_key' + + get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock + ) + + # Original passport unchanged + passport.refresh_from_db() + self.assertEqual(passport.lti_1p3_tool_public_key, 'shared_key') + + # Block has new passport + self.assertNotEqual(self.xblock.lti_1p3_passport_id, str(passport.passport_id)) + + def test_passport_unchanged_when_keys_match(self): + """ + Check if passport is not updated when block keys already match. + """ + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key='matching_key', + lti_1p3_tool_keyset_url='matching_url' + ) + lti_config = LtiConfiguration.objects.create( + location=self.location, + lti_1p3_passport=passport + ) + + self.xblock.lti_1p3_tool_public_key = 'matching_key' + self.xblock.lti_1p3_tool_keyset_url = 'matching_url' + + original_passport_id = passport.passport_id + + get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock + ) + + # Same passport used + self.assertEqual(Lti1p3Passport.objects.count(), 1) + lti_config.refresh_from_db() + self.assertEqual(str(lti_config.lti_1p3_passport.passport_id), str(original_passport_id)) + + @ddt.data( + ('public_key', 'lti_1p3_tool_public_key'), + ('keyset_url', 'lti_1p3_tool_keyset_url') + ) + @ddt.unpack + def test_new_passport_on_key_mode_change(self, key_mode, key_field): + """ + Check if a new passport is created when key_mode-specific field changes. + """ + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key='old_key', + lti_1p3_tool_keyset_url='old_url' + ) + LtiConfiguration.objects.create(location=self.location, lti_1p3_passport=passport) + LtiConfiguration.objects.create(location=self.other_location, lti_1p3_passport=passport) + + self.xblock.lti_1p3_tool_key_mode = key_mode + setattr(self.xblock, key_field, 'new_value') + + get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock + ) + + self.assertEqual(Lti1p3Passport.objects.count(), 2) + + def test_passport_updated_when_fields_empty(self): + """ + Check if passport is updated when both key fields are empty. + """ + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key='', + lti_1p3_tool_keyset_url='' + ) + LtiConfiguration.objects.create( + location=self.location, + lti_1p3_passport=passport + ) + + self.xblock.lti_1p3_tool_public_key = 'new_key' + self.xblock.lti_1p3_tool_keyset_url = 'new_url' + + get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock + ) + + passport.refresh_from_db() + self.assertEqual(passport.lti_1p3_tool_public_key, 'new_key') + @ddt.ddt class TestValidateLti1p3LaunchData(TestCase): From 1057e62db676b413f2f72be8390b88213fea4f9e Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 26 Mar 2026 15:30:00 +0530 Subject: [PATCH 17/43] refactor(views): remove unnecessary db call --- lti_consumer/plugin/views.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index e4fc77e8..1f00b8f6 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -928,13 +928,10 @@ def start_proctoring_assessment_endpoint(request): resource_link_id = decoded_jwt.get('https://purl.imsglobal.org/spec/lti/claim/resource_link', {}).get('id') try: - passport = Lti1p3Passport.objects.filter(lti_1p3_client_id=iss).first() - lti_config = LtiConfiguration.objects.get(lti_1p3_passport=passport, location=resource_link_id) - except Lti1p3Passport.DoesNotExist: - log.error( - "Invalid iss claim '%s' for LTI 1.3 Proctoring Services start_proctoring_assessment_endpoint callback", iss + lti_config = LtiConfiguration.objects.get( + lti_1p3_passport__lti_1p3_client_id=iss, + location=resource_link_id ) - return render(request, 'html/lti_proctoring_start_error.html', status=HTTP_404_NOT_FOUND) except LtiConfiguration.DoesNotExist: log.error( "Invalid resource_link id '%s' for LTI 1.3 Proctoring Services start_proctoring_assessment_endpoint", From 7e0c0122fd2aab6bd11ef913964a62f37b4f14a7 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 31 Mar 2026 15:00:55 +0530 Subject: [PATCH 18/43] feat: add name and context key to passport and fix race condition - Added 'name' and 'context_key' fields to Lti1p3Passport model - Modified LtiConfiguration to populate these fields when creating passports - Updated admin interface to display new fields - Added migration to populate existing passports with name and context_key - Updated string representation of passport to include name - Made 'location' field in LtiConfiguration unique to prevent race conditions This fixes a race condition that occurred when get_or_create was called with the same location multiple times, which resulted in duplicate rows with identical locations being created simultaneously. --- lti_consumer/admin.py | 7 ++- lti_consumer/api.py | 2 + ...ontext_key_lti1p3passport_name_and_more.py | 58 +++++++++++++++++++ lti_consumer/models.py | 19 +++++- 4 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py index 57cda35b..4a777e14 100644 --- a/lti_consumer/admin.py +++ b/lti_consumer/admin.py @@ -49,7 +49,12 @@ class Lti1p3PassportAdmin(admin.ModelAdmin): """ Admin view for Lti1p3Passport models. """ - list_display = ('passport_id', 'lti_1p3_client_id') + list_display = ( + 'name', + 'context_key', + 'passport_id', + 'lti_1p3_client_id', + ) inlines = [LtiConfigurationInline] diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 996e69f3..507e6e06 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -64,6 +64,8 @@ def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfigura passport = Lti1p3Passport.objects.create( lti_1p3_tool_public_key=str(block.lti_1p3_tool_public_key), lti_1p3_tool_keyset_url=str(block.lti_1p3_tool_keyset_url), + name=f"Passport of {block.display_name}", + context_key=block.context_id, ) block.lti_1p3_passport_id = str(passport.passport_id) block.save() diff --git a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py new file mode 100644 index 00000000..003f9ef8 --- /dev/null +++ b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py @@ -0,0 +1,58 @@ +# Generated by Django 5.2.12 on 2026-03-31 07:34 + +import opaque_keys.edx.django.models +from django.db import migrations, models + + +def set_name_and_context_key(apps, _): + """ + Copy name and context_key from Block to LTI1p3Passport. + """ + from lti_consumer.plugin.compat import load_enough_xblock # pylint: disable=import-outside-toplevel + LtiConfiguration = apps.get_model("lti_consumer", "LtiConfiguration") + for configuration in LtiConfiguration.objects.all(): + if not configuration.location: + continue + try: + block = load_enough_xblock(configuration.location) + except Exception as e: # pylint: disable=broad-exception-caught + print(f"Error loading xblock {configuration.location}: {e}") + passport = configuration.lti_1p3_passport + if passport and not passport.name: + passport.name = f"Passport of {block.display_name}" + passport.context_key = block.context_id + passport.save() + + +def backwards(*_): + """Reverse the migration only for MariaDB databases.""" + + +class Migration(migrations.Migration): + dependencies = [ + ('lti_consumer', '0022_remove_lticonfiguration_lti_1p3_client_id_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='lti1p3passport', + name='context_key', + field=models.CharField(blank=True, max_length=255, null=True), + ), + migrations.AddField( + model_name='lti1p3passport', + name='name', + field=models.CharField(blank=True, max_length=255, null=True), + ), + migrations.AlterField( + model_name='lticonfiguration', + name='location', + field=opaque_keys.edx.django.models.UsageKeyField( + blank=True, db_index=True, max_length=255, null=True, unique=True + ), + ), + migrations.RunPython( + code=set_name_and_context_key, + reverse_code=backwards, + ), + ] diff --git a/lti_consumer/models.py b/lti_consumer/models.py index f118d71f..35374754 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -52,6 +52,8 @@ class Lti1p3Passport(models.Model): """ Model to store LTI 1.3 keys. """ + name = models.CharField(max_length=255, null=True, blank=True) + context_key = models.CharField(max_length=255, null=True, blank=True) passport_id = models.UUIDField(unique=True, default=uuid.uuid4, editable=False) lti_1p3_internal_private_key = models.TextField( @@ -148,7 +150,7 @@ def lti_1p3_public_jwk(self): return json.loads(self.lti_1p3_internal_public_jwk) def __str__(self): - return f'Lti1p3Passport: {self.passport_id}' + return f'Lti1p3Passport: {self.name} - {self.passport_id}' def clean(self): if self.lti_1p3_tool_public_key == "" and self.lti_1p3_tool_keyset_url == "": @@ -225,6 +227,8 @@ class LtiConfiguration(models.Model): db_index=True, null=True, blank=True, + # Duplicate fails if not unique. + unique=True, ) # This is where the configuration is stored in the model if stored on this model. @@ -407,13 +411,22 @@ def get_or_create_lti_1p3_passport(self): if self.location: block = compat.load_enough_xblock(self.location) if self.location and block and block.lti_1p3_passport_id: - passport, created = Lti1p3Passport.objects.get_or_create(passport_id=block.lti_1p3_passport_id) + passport, created = Lti1p3Passport.objects.get_or_create( + passport_id=block.lti_1p3_passport_id, + defaults={ + "name": f"Passport of {block.display_name}", + "context_key": block.context_id, + } + ) if created: log.info("Created new LTI 1.3 Passport %s for %s", passport, self) else: log.info("Existing LTI 1.3 Passport %s found for %s", passport, self) else: - passport = Lti1p3Passport.objects.create() + passport = Lti1p3Passport.objects.create( + name=f"Passport of {block.display_name}", + context_key=block.context_id, + ) log.info("Created new LTI 1.3 Passport %s for %s", passport, self) if block: block.lti_1p3_passport_id = str(passport.passport_id) From d96b929c82c5a7ac909a65a0ba6ff8d141e06310 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 31 Mar 2026 15:30:29 +0530 Subject: [PATCH 19/43] fix: create name only if block is available --- lti_consumer/models.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 35374754..a896ce3a 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -423,12 +423,12 @@ def get_or_create_lti_1p3_passport(self): else: log.info("Existing LTI 1.3 Passport %s found for %s", passport, self) else: - passport = Lti1p3Passport.objects.create( - name=f"Passport of {block.display_name}", - context_key=block.context_id, - ) + passport = Lti1p3Passport.objects.create() log.info("Created new LTI 1.3 Passport %s for %s", passport, self) if block: + passport.name = f"Passport of {block.display_name}" + passport.context_key = block.context_id + passport.save() block.lti_1p3_passport_id = str(passport.passport_id) block.save() compat.save_xblock(block) From 8c8df22d11ddd70fcd4a5f81eec9ef70a772829f Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 31 Mar 2026 16:17:06 +0530 Subject: [PATCH 20/43] fix: test --- lti_consumer/tests/unit/test_models.py | 19 ++++++++++++------- lti_consumer/tests/unit/test_signals.py | 2 ++ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 1b202cd4..e90d33d8 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -50,6 +50,9 @@ def setUp(self): 'lti_advantage_deep_linking_enabled': True, } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) + self.xblock_1 = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) + self.xblock_2 = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) + self.xblock_3 = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) patcher = patch( 'lti_consumer.plugin.compat.load_enough_xblock', @@ -65,12 +68,12 @@ def setUp(self): ) self.lti_1p3_config = LtiConfiguration.objects.create( - location=self.xblock.scope_ids.usage_id, + location=self.xblock_1.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3 ) self.lti_1p3_config_db = LtiConfiguration.objects.create( - location=self.xblock.scope_ids.usage_id, + location=self.xblock_2.scope_ids.usage_id, version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_ON_DB, lti_advantage_ags_mode='programmatic', @@ -80,7 +83,7 @@ def setUp(self): self.lti_1p3_config_external = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_EXTERNAL, - location=self.xblock.scope_ids.usage_id, + location=self.xblock_3.scope_ids.usage_id, ) self.lti_1p1_external = LtiConfiguration.objects.create( @@ -93,11 +96,13 @@ def _get_1p3_config(self, **kwargs): """ Helper function to create a LtiConfiguration object with specific attributes """ - return LtiConfiguration.objects.create( - location=self.xblock.scope_ids.usage_id, - version=LtiConfiguration.LTI_1P3, - **kwargs + defaults = kwargs + defaults["version"] = LtiConfiguration.LTI_1P3 + config, _ = LtiConfiguration.objects.update_or_create( + location=self.xblock_1.scope_ids.usage_id, + defaults=defaults, ) + return config @patch("lti_consumer.models.LtiConfiguration._get_lti_1p3_consumer") @patch("lti_consumer.models.LtiConfiguration._get_lti_1p1_consumer") diff --git a/lti_consumer/tests/unit/test_signals.py b/lti_consumer/tests/unit/test_signals.py index 421d3702..b0c3e41e 100644 --- a/lti_consumer/tests/unit/test_signals.py +++ b/lti_consumer/tests/unit/test_signals.py @@ -32,6 +32,8 @@ def setUp(self): # Patch internal method to avoid calls to modulestore self._block_mock = Mock() + self._block_mock.display_name = "consumer" + self._block_mock.context_id = "some-context-id" compat_mock = patch("lti_consumer.models.compat") self.addCleanup(compat_mock.stop) self._compat_mock = compat_mock.start() From a6f3807060ba8081bea1a3bffe5d74b1673a9989 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 18:09:20 +0530 Subject: [PATCH 21/43] refactor: migration --- ...port_context_key_lti1p3passport_name_and_more.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py index 003f9ef8..63f996e1 100644 --- a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py +++ b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py @@ -17,11 +17,14 @@ def set_name_and_context_key(apps, _): block = load_enough_xblock(configuration.location) except Exception as e: # pylint: disable=broad-exception-caught print(f"Error loading xblock {configuration.location}: {e}") - passport = configuration.lti_1p3_passport - if passport and not passport.name: - passport.name = f"Passport of {block.display_name}" - passport.context_key = block.context_id - passport.save() + try: + passport = configuration.lti_1p3_passport + if passport and not passport.name: + passport.name = f"Passport of {block.display_name}" + passport.context_key = block.context_id + passport.save() + except Exception as e: # pylint: disable=broad-exception-caught + print(f"Error saving passport {passport}: {e}") def backwards(*_): From 17938cc7979f165ea6d8f5786a19f77b434be431 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 19:00:08 +0530 Subject: [PATCH 22/43] chore: upgrade --- requirements/base.txt | 2 +- requirements/ci.txt | 14 +------------- requirements/dev.txt | 2 +- requirements/quality.txt | 2 +- requirements/test.txt | 8 +------- 5 files changed, 5 insertions(+), 23 deletions(-) diff --git a/requirements/base.txt b/requirements/base.txt index 4fafe0f0..a1958d29 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -85,7 +85,7 @@ markupsafe==3.0.3 # xblock oauthlib==3.3.1 # via -r requirements/base.in -openedx-events==10.5.0 +openedx-events==11.1.0 # via -r requirements/base.in openedx-filters==3.1.0 # via -r requirements/base.in diff --git a/requirements/ci.txt b/requirements/ci.txt index eba20d90..6a127c2f 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -29,10 +29,6 @@ attrs==26.1.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 test.txt @@ -200,10 +196,6 @@ idna==3.11 # via # -r test.txt # requests -importlib-metadata==8.8.0 - # via - # -r requirements/test.txt - # keyring isort==8.0.1 # via # -r test.txt @@ -283,7 +275,7 @@ nh3==0.3.4 # readme-renderer oauthlib==3.3.1 # via -r requirements/test.txt -openedx-events==10.5.0 +openedx-events==11.1.0 # via -r requirements/test.txt openedx-filters==3.0.0 # via -r requirements/test.txt @@ -508,10 +500,6 @@ xblock==6.0.0 # xblock-sdk xblock-sdk==0.14.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 ed72b0d2..2b9e49ae 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -115,7 +115,7 @@ markupsafe==3.0.3 # xblock oauthlib==3.3.1 # via -r requirements/base.txt -openedx-events==10.5.0 +openedx-events==11.1.0 # via -r requirements/base.txt openedx-filters==3.0.0 # via -r requirements/base.txt diff --git a/requirements/quality.txt b/requirements/quality.txt index c9998b12..ff4b1d99 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -170,7 +170,7 @@ mdurl==0.1.2 # via markdown-it-py oauthlib==3.3.1 # via -r requirements/base.txt -openedx-events==10.5.0 +openedx-events==11.1.0 # via -r requirements/base.txt openedx-filters==3.0.0 # via -r requirements/base.txt diff --git a/requirements/test.txt b/requirements/test.txt index cce1515a..2bf844d0 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -24,8 +24,6 @@ attrs==26.1.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 @@ -149,8 +147,6 @@ id==1.5.0 # via twine idna==3.11 # via requests -importlib-metadata==8.8.0 - # via keyring isort==8.0.1 # via pylint jaraco-classes==3.4.0 @@ -208,7 +204,7 @@ nh3==0.3.4 # via readme-renderer oauthlib==3.3.1 # via -r requirements/base.txt -openedx-events==10.5.0 +openedx-events==11.1.0 # via -r requirements/base.txt openedx-filters==3.0.0 # via -r requirements/base.txt @@ -368,8 +364,6 @@ xblock==6.0.0 # xblock-sdk xblock-sdk==0.14.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 From 8096344459344817d1ae4d2dbfbfed75eed4178d Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 19:27:22 +0530 Subject: [PATCH 23/43] refactor: avoid duplicate signal triggers --- lti_consumer/api.py | 20 +++++++++++++------ ...ontext_key_lti1p3passport_name_and_more.py | 2 ++ lti_consumer/models.py | 5 +++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 507e6e06..f08c392b 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -72,15 +72,23 @@ def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfigura log.info("Created new LTI passport for %s with new keyset and public key", block.scope_ids.usage_id) save_xblock(block) - lti_config.config_store = config_store - lti_config.external_id = block.external_config - lti_config.lti_1p3_passport = passport - + dirty = False + + if lti_config.config_store != config_store: + lti_config.config_store = config_store + dirty = True + if lti_config.external_id != block.external_config: + lti_config.external_id = block.external_config + dirty = True + if passport and lti_config.lti_1p3_passport != passport.pk: + lti_config.lti_1p3_passport = passport + dirty = True if lti_config.version != lti_version: lti_config.version = lti_version + dirty = True - lti_config.save() - + if dirty: + lti_config.save() return lti_config diff --git a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py index 63f996e1..e85dc761 100644 --- a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py +++ b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py @@ -17,6 +17,7 @@ def set_name_and_context_key(apps, _): block = load_enough_xblock(configuration.location) except Exception as e: # pylint: disable=broad-exception-caught print(f"Error loading xblock {configuration.location}: {e}") + continue try: passport = configuration.lti_1p3_passport if passport and not passport.name: @@ -25,6 +26,7 @@ def set_name_and_context_key(apps, _): passport.save() except Exception as e: # pylint: disable=broad-exception-caught print(f"Error saving passport {passport}: {e}") + continue def backwards(*_): diff --git a/lti_consumer/models.py b/lti_consumer/models.py index a896ce3a..3b5f6683 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -432,8 +432,9 @@ def get_or_create_lti_1p3_passport(self): block.lti_1p3_passport_id = str(passport.passport_id) block.save() compat.save_xblock(block) - self.lti_1p3_passport = passport - self.save() + # We use update to avoid triggering post save signal as this function is itself + # called from the post_save signal handler, this avoids double call to same function. + LtiConfiguration.objects.filter(pk=self.pk).update(lti_1p3_passport=passport) def save(self, *args, **kwargs): self.sync_configurations() From 2a2b3375a4b7841624e231f1a7788f9d092ddd92 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 19:45:12 +0530 Subject: [PATCH 24/43] refactor: api --- lti_consumer/api.py | 119 +++++++++++++++++++++++--------------------- 1 file changed, 63 insertions(+), 56 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index f08c392b..a44db862 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -27,68 +27,75 @@ log = logging.getLogger(__name__) +def _ensure_lti_passport(block, lti_config): + """Ensure passport is synced with block fields, creating a new one if needed.""" + passport = lti_config.lti_1p3_passport + if not passport or lti_config.config_store != LtiConfiguration.CONFIG_ON_XBLOCK: + return passport + + block_public_key = str(block.lti_1p3_tool_public_key) + block_keyset_url = str(block.lti_1p3_tool_keyset_url) + + # Update existing passport if safe + if passport.lticonfiguration_set.count() == 1 or ( + not passport.lti_1p3_tool_public_key and not passport.lti_1p3_tool_keyset_url + ): + if passport.lti_1p3_tool_public_key != block_public_key or passport.lti_1p3_tool_keyset_url != block_keyset_url: + passport.lti_1p3_tool_public_key = block_public_key + passport.lti_1p3_tool_keyset_url = block_keyset_url + passport.save() + log.info("Updated LTI passport for %s", block.scope_ids.usage_id) + return passport + + # Create new passport if keys changed and passport is shared + key_mismatch = ( + block.lti_1p3_tool_key_mode == 'public_key' and passport.lti_1p3_tool_public_key != block_public_key + ) or ( + block.lti_1p3_tool_key_mode == 'keyset_url' and passport.lti_1p3_tool_keyset_url != block_keyset_url + ) + + if key_mismatch: + from lti_consumer.plugin.compat import save_xblock + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key=block_public_key, + lti_1p3_tool_keyset_url=block_keyset_url, + name=f"Passport of {block.display_name}", + context_key=block.context_id, + ) + block.lti_1p3_passport_id = str(passport.passport_id) + save_xblock(block) + log.info("Created new LTI passport for %s", block.scope_ids.usage_id) + + return passport + + def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfiguration.CONFIG_ON_XBLOCK): """ - 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. + Retrieve or create an LtiConfiguration for the block. - Treat the lti_version argument as the source of truth for LtiConfiguration.version and override the - LtiConfiguration.version with lti_version. This allows, for example, for - the XBlock to be the source of truth for the LTI version, which is a user-centric perspective we've adopted. - This allows XBlock users to update the LTI version without needing to update the database. + The lti_version parameter is treated as the source of truth, overriding + any stored version to allow XBlocks to control LTI version without DB updates. """ - from lti_consumer.plugin.compat import save_xblock # pylint: disable=import-outside-toplevel - # The create operation is only performed when there is no existing configuration for the block lti_config, _ = LtiConfiguration.objects.get_or_create(location=block.scope_ids.usage_id) - passport = lti_config.lti_1p3_passport - if passport and config_store == LtiConfiguration.CONFIG_ON_XBLOCK: - # Copy keyset url and public key to passport row - if passport.lticonfiguration_set.count() == 1 or ( - passport.lti_1p3_tool_keyset_url == '' and passport.lti_1p3_tool_public_key == '' - ): - # If the passport is only used in one LTI configuration or both fields are empty, - # we update its tool key and url to match block fields. - passport.lti_1p3_tool_public_key = str(block.lti_1p3_tool_public_key) - passport.lti_1p3_tool_keyset_url = str(block.lti_1p3_tool_keyset_url) - passport.save() - log.info("Updated LTI passport for %s to match block fields", block.scope_ids.usage_id) - elif ( - block.lti_1p3_tool_key_mode == 'public_key' and - passport.lti_1p3_tool_public_key != str(block.lti_1p3_tool_public_key) - ) or ( - block.lti_1p3_tool_key_mode == 'keyset_url' and - passport.lti_1p3_tool_keyset_url != str(block.lti_1p3_tool_keyset_url) - ): - # tool public key or url has changed, we create a new passport to avoid conflicts - # with the existing configuration - passport = Lti1p3Passport.objects.create( - lti_1p3_tool_public_key=str(block.lti_1p3_tool_public_key), - lti_1p3_tool_keyset_url=str(block.lti_1p3_tool_keyset_url), - name=f"Passport of {block.display_name}", - context_key=block.context_id, - ) - block.lti_1p3_passport_id = str(passport.passport_id) - block.save() - log.info("Created new LTI passport for %s with new keyset and public key", block.scope_ids.usage_id) - save_xblock(block) - - dirty = False - - if lti_config.config_store != config_store: - lti_config.config_store = config_store - dirty = True - if lti_config.external_id != block.external_config: - lti_config.external_id = block.external_config - dirty = True - if passport and lti_config.lti_1p3_passport != passport.pk: - lti_config.lti_1p3_passport = passport - dirty = True - if lti_config.version != lti_version: - lti_config.version = lti_version - dirty = True - - if dirty: + + # Ensure passport is synced with block + passport = _ensure_lti_passport(block, lti_config) + + # Batch updates + updates = { + 'config_store': config_store, + 'external_id': block.external_config, + 'version': lti_version, + } + if passport: + updates['lti_1p3_passport'] = passport + + # Only save if changed + if any(getattr(lti_config, key) != value for key, value in updates.items()): + for key, value in updates.items(): + setattr(lti_config, key, value) lti_config.save() + return lti_config From 901d76bace88a0a08441de0d6b64d943f4251c3b Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 19:59:00 +0530 Subject: [PATCH 25/43] refactor: rename --- lti_consumer/api.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index a44db862..686383f8 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -69,7 +69,7 @@ def _ensure_lti_passport(block, lti_config): return passport -def get_or_create_local_lti_config(lti_version, block, config_store=LtiConfiguration.CONFIG_ON_XBLOCK): +def _get_or_create_local_lti_config(lti_version, block, config_store=LtiConfiguration.CONFIG_ON_XBLOCK): """ Retrieve or create an LtiConfiguration for the block. @@ -114,7 +114,7 @@ def _get_lti_config_for_block(block): bits of configuration. """ if block.config_type == 'database': - lti_config = get_or_create_local_lti_config( + lti_config = _get_or_create_local_lti_config( block.lti_version, block, LtiConfiguration.CONFIG_ON_DB, @@ -124,13 +124,13 @@ 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_config = _get_or_create_local_lti_config( config.get("version"), block, LtiConfiguration.CONFIG_EXTERNAL, ) else: - lti_config = get_or_create_local_lti_config( + lti_config = _get_or_create_local_lti_config( block.lti_version, block, LtiConfiguration.CONFIG_ON_XBLOCK, From 114e2088e298582d1996bae1e0aa127c6c327f96 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 20:07:26 +0530 Subject: [PATCH 26/43] fix: tests --- lti_consumer/tests/unit/test_api.py | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 8c7ddcf1..4d4db55f 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -18,7 +18,7 @@ get_lti_1p3_content_url, get_lti_1p3_launch_info, get_lti_1p3_launch_start_url, - get_or_create_local_lti_config, + _get_or_create_local_lti_config, validate_lti_1p3_launch_data, ) from lti_consumer.data import Lti1p3LaunchData, Lti1p3ProctoringLaunchData @@ -122,7 +122,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_config( lti_version=lti_version, block=self.xblock ) @@ -144,7 +144,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_config( lti_version=lti_version, block=self.xblock ) @@ -163,7 +163,7 @@ def test_update_lti_version(self): ) # Call API - get_or_create_local_lti_config( + _get_or_create_local_lti_config( lti_version=LtiConfiguration.LTI_1P3, block=self.xblock ) @@ -179,7 +179,7 @@ def test_create_lti_config_config_store(self, config_store): the config_store field of the LtiConfiguration model appropriately. """ lti_version = LtiConfiguration.LTI_1P3 - lti_config = get_or_create_local_lti_config( + lti_config = _get_or_create_local_lti_config( lti_version=lti_version, block=self.xblock, config_store=config_store, @@ -200,7 +200,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_config( lti_version=lti_version, block=self.xblock, ) @@ -217,10 +217,11 @@ def test_passport_created_on_first_call(self): """ lti_version = LtiConfiguration.LTI_1P3 - lti_config = get_or_create_local_lti_config( + lti_config = _get_or_create_local_lti_config( lti_version=lti_version, block=self.xblock ) + lti_config.refresh_from_db() self.assertIsNotNone(lti_config.lti_1p3_passport) @@ -240,7 +241,7 @@ def test_passport_updated_when_single_use(self): self.xblock.lti_1p3_tool_public_key = 'new_key' self.xblock.lti_1p3_tool_keyset_url = 'new_url' - get_or_create_local_lti_config( + _get_or_create_local_lti_config( lti_version=LtiConfiguration.LTI_1P3, block=self.xblock ) @@ -264,7 +265,7 @@ def test_new_passport_created_on_key_conflict(self): self.xblock.lti_1p3_tool_key_mode = 'public_key' self.xblock.lti_1p3_tool_public_key = 'new_key' - get_or_create_local_lti_config( + _get_or_create_local_lti_config( lti_version=LtiConfiguration.LTI_1P3, block=self.xblock ) @@ -294,7 +295,7 @@ def test_passport_unchanged_when_keys_match(self): original_passport_id = passport.passport_id - get_or_create_local_lti_config( + _get_or_create_local_lti_config( lti_version=LtiConfiguration.LTI_1P3, block=self.xblock ) @@ -323,7 +324,7 @@ def test_new_passport_on_key_mode_change(self, key_mode, key_field): self.xblock.lti_1p3_tool_key_mode = key_mode setattr(self.xblock, key_field, 'new_value') - get_or_create_local_lti_config( + _get_or_create_local_lti_config( lti_version=LtiConfiguration.LTI_1P3, block=self.xblock ) @@ -346,7 +347,7 @@ def test_passport_updated_when_fields_empty(self): self.xblock.lti_1p3_tool_public_key = 'new_key' self.xblock.lti_1p3_tool_keyset_url = 'new_url' - get_or_create_local_lti_config( + _get_or_create_local_lti_config( lti_version=LtiConfiguration.LTI_1P3, block=self.xblock ) @@ -572,6 +573,7 @@ def test_retrieve_with_id(self): # Call and check returns launch_info = get_lti_1p3_launch_info(launch_data) + lti_config.refresh_from_db() # Not checking all data here, there's a test specific for that self.assertEqual(launch_info['client_id'], lti_config.lti_1p3_client_id) @@ -622,6 +624,7 @@ def test_launch_info_for_lti_config_without_location(self): version=LtiConfiguration.LTI_1P3, config_id=_test_config_id, ) + lti_config.refresh_from_db() LtiDlContentItem.objects.create( lti_configuration=lti_config, content_type=LtiDlContentItem.IMAGE, From d9af4ddb12d767d8977c8d455adfe76a6d697bdf Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 20:12:35 +0530 Subject: [PATCH 27/43] chore: fix lint issues --- lti_consumer/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 686383f8..c18765b6 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -55,7 +55,7 @@ def _ensure_lti_passport(block, lti_config): ) if key_mismatch: - from lti_consumer.plugin.compat import save_xblock + from lti_consumer.plugin.compat import save_xblock # pylint: disable=import-outside-toplevel passport = Lti1p3Passport.objects.create( lti_1p3_tool_public_key=block_public_key, lti_1p3_tool_keyset_url=block_keyset_url, From dfffc98b37d995d7683f986acc33b0b80115d73e Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 20:22:52 +0530 Subject: [PATCH 28/43] test: add some more --- lti_consumer/tests/unit/test_api.py | 77 +++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 4d4db55f..8578b4f2 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -250,6 +250,83 @@ def test_passport_updated_when_single_use(self): self.assertEqual(passport.lti_1p3_tool_public_key, 'new_key') self.assertEqual(passport.lti_1p3_tool_keyset_url, 'new_url') + def test_passport_not_updated_when_config_store_not_on_xblock(self): + """ + Ensure that when the existing LtiConfiguration is not stored on the XBlock + (e.g., CONFIG_ON_DB), the passport is not updated even if block key values differ. + """ + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key='shared_key', + lti_1p3_tool_keyset_url='shared_url' + ) + # Create a configuration that uses the passport but is stored on DB + LtiConfiguration.objects.create( + location=self.location, + lti_1p3_passport=passport, + config_store=LtiConfiguration.CONFIG_ON_DB, + ) + + # Block has different keys, but since the config is not on XBlock, + # _ensure_lti_passport should not update the shared passport. + self.xblock.lti_1p3_tool_public_key = 'new_key' + self.xblock.lti_1p3_tool_keyset_url = 'new_url' + + _get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock, + config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + ) + + passport.refresh_from_db() + # passport should remain unchanged + self.assertEqual(passport.lti_1p3_tool_public_key, 'shared_key') + self.assertEqual(passport.lti_1p3_tool_keyset_url, 'shared_url') + + def test_passport_not_saved_when_no_changes(self): + """ + Ensure that passport.save() is not called when passport fields already match the + XBlock values and no update is required. + """ + passport = Lti1p3Passport.objects.create( + lti_1p3_tool_public_key='matching_key', + lti_1p3_tool_keyset_url='matching_url' + ) + LtiConfiguration.objects.create( + location=self.location, + lti_1p3_passport=passport + ) + + # Make block keys match the passport + self.xblock.lti_1p3_tool_public_key = 'matching_key' + self.xblock.lti_1p3_tool_keyset_url = 'matching_url' + + with patch.object(Lti1p3Passport, 'save') as mock_save: + _get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock + ) + mock_save.assert_not_called() + + def test_config_not_saved_when_no_changes(self): + """ + Ensure that LtiConfiguration.save() is not called when no fields change. + """ + # Create a config that already matches the block's current values + LtiConfiguration.objects.create( + location=self.location, + version=LtiConfiguration.LTI_1P3, + config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + ) + + # Ensure block has default external_config of None and LTI version matches + with patch.object(LtiConfiguration, 'save') as mock_save: + _get_or_create_local_lti_config( + lti_version=LtiConfiguration.LTI_1P3, + block=self.xblock, + config_store=LtiConfiguration.CONFIG_ON_XBLOCK, + ) + mock_save.assert_not_called() + def test_new_passport_created_on_key_conflict(self): """ Check if a new passport is created when block key differs and passport is shared. From df6c7fee98319663143f6667dd515c6e1ed625ed Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 20:33:01 +0530 Subject: [PATCH 29/43] fix: tests --- lti_consumer/models.py | 2 ++ lti_consumer/tests/unit/test_api.py | 4 ---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 3b5f6683..8aa5e2d8 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -435,6 +435,8 @@ def get_or_create_lti_1p3_passport(self): # We use update to avoid triggering post save signal as this function is itself # called from the post_save signal handler, this avoids double call to same function. LtiConfiguration.objects.filter(pk=self.pk).update(lti_1p3_passport=passport) + # Refresh block from db to get passport id. + self.refresh_from_db() def save(self, *args, **kwargs): self.sync_configurations() diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 8578b4f2..8c0c8f76 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -221,7 +221,6 @@ def test_passport_created_on_first_call(self): lti_version=lti_version, block=self.xblock ) - lti_config.refresh_from_db() self.assertIsNotNone(lti_config.lti_1p3_passport) @@ -379,7 +378,6 @@ def test_passport_unchanged_when_keys_match(self): # Same passport used self.assertEqual(Lti1p3Passport.objects.count(), 1) - lti_config.refresh_from_db() self.assertEqual(str(lti_config.lti_1p3_passport.passport_id), str(original_passport_id)) @ddt.data( @@ -650,7 +648,6 @@ def test_retrieve_with_id(self): # Call and check returns launch_info = get_lti_1p3_launch_info(launch_data) - lti_config.refresh_from_db() # Not checking all data here, there's a test specific for that self.assertEqual(launch_info['client_id'], lti_config.lti_1p3_client_id) @@ -701,7 +698,6 @@ def test_launch_info_for_lti_config_without_location(self): version=LtiConfiguration.LTI_1P3, config_id=_test_config_id, ) - lti_config.refresh_from_db() LtiDlContentItem.objects.create( lti_configuration=lti_config, content_type=LtiDlContentItem.IMAGE, From 6bed5938476eecb0f1acda1cb039939c8d30eb2f Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 3 Apr 2026 20:48:12 +0530 Subject: [PATCH 30/43] fix: coverage issue --- ...3_lti1p3passport_context_key_lti1p3passport_name_and_more.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py index e85dc761..4710aaca 100644 --- a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py +++ b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py @@ -4,7 +4,7 @@ from django.db import migrations, models -def set_name_and_context_key(apps, _): +def set_name_and_context_key(apps, _): # pragma: nocover """ Copy name and context_key from Block to LTI1p3Passport. """ From aa034e6a92b4cca1e41622dd0d974274d587672a Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 7 Apr 2026 19:00:41 +0530 Subject: [PATCH 31/43] chore: upgrade --- requirements/ci.txt | 4 ++-- requirements/dev.txt | 2 +- requirements/quality.txt | 4 ++-- requirements/test.txt | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/requirements/ci.txt b/requirements/ci.txt index 6a127c2f..905156fb 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -34,7 +34,7 @@ binaryornot==0.6.0 # -r test.txt # cookiecutter bleach==6.3.0 - # via -r test.txt + # via -r requirements/test.txt boto3==1.42.84 # via # -r test.txt @@ -277,7 +277,7 @@ oauthlib==3.3.1 # via -r requirements/test.txt openedx-events==11.1.0 # via -r requirements/test.txt -openedx-filters==3.0.0 +openedx-filters==3.1.0 # via -r requirements/test.txt packaging==26.0 # via diff --git a/requirements/dev.txt b/requirements/dev.txt index 2b9e49ae..aa3ba68d 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -117,7 +117,7 @@ oauthlib==3.3.1 # via -r requirements/base.txt openedx-events==11.1.0 # via -r requirements/base.txt -openedx-filters==3.0.0 +openedx-filters==3.1.0 # via -r requirements/base.txt path==16.16.0 # via edx-i18n-tools diff --git a/requirements/quality.txt b/requirements/quality.txt index ff4b1d99..32a725b3 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -25,7 +25,7 @@ attrs==26.1.0 binaryornot==0.6.0 # via cookiecutter bleach==6.3.0 - # via -r base.txt + # via -r requirements/base.txt boto3==1.42.84 # via fs-s3fs botocore==1.42.84 @@ -172,7 +172,7 @@ oauthlib==3.3.1 # via -r requirements/base.txt openedx-events==11.1.0 # via -r requirements/base.txt -openedx-filters==3.0.0 +openedx-filters==3.1.0 # via -r requirements/base.txt platformdirs==4.9.4 # via pylint diff --git a/requirements/test.txt b/requirements/test.txt index 2bf844d0..8accc09c 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -27,7 +27,7 @@ attrs==26.1.0 binaryornot==0.6.0 # via cookiecutter bleach==6.3.0 - # via -r base.txt + # via -r requirements/base.txt boto3==1.42.84 # via fs-s3fs botocore==1.42.84 @@ -206,7 +206,7 @@ oauthlib==3.3.1 # via -r requirements/base.txt openedx-events==11.1.0 # via -r requirements/base.txt -openedx-filters==3.0.0 +openedx-filters==3.1.0 # via -r requirements/base.txt packaging==26.0 # via twine From 6269d917b8051b6151f92cac66db80dd2b8e3361 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 7 Apr 2026 19:56:33 +0530 Subject: [PATCH 32/43] fix: handle duplicate block explicitly --- lti_consumer/signals/signals.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index 2d32ee33..b5f239b1 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -2,11 +2,12 @@ LTI Consumer related Signal handlers """ import logging +import uuid from django.db.models.signals import post_save 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 openedx_events.content_authoring.data import DuplicatedXBlockData, LibraryBlockData, XBlockData +from openedx_events.content_authoring.signals import LIBRARY_BLOCK_DELETED, XBLOCK_DELETED, XBLOCK_DUPLICATED from lti_consumer.models import Lti1p3Passport, LtiAgsScore, LtiConfiguration from lti_consumer.plugin import compat @@ -149,4 +150,22 @@ def delete_lib_lti_configuration(**kwargs): log.info(f"Deleted {result} lti 1.3 passport objects in library") +@receiver(XBLOCK_DUPLICATED) +def duplicate_xblock_lti_configuration(**kwargs): + """ + Duplicate LTI configuration from the source to the target xblock. + """ + xblock_data = kwargs.get("xblock_info", None) + if not xblock_data or not isinstance(xblock_data, DuplicatedXBlockData): + log.error("Received null or incorrect data for event") + return + + src_lti_config = LtiConfiguration.objects.get(location=str(xblock_data.source_usage_key)) + copy = src_lti_config + copy.pk = None + copy.location = str(xblock_data.usage_key) + copy.config_id = uuid.uuid4() + copy.save() + + LTI_1P3_PROCTORING_ASSESSMENT_STARTED = Signal() From 01fb5bf0d1eb69fbdf47a05ec6ceca076bc393e5 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 13 Apr 2026 16:53:31 +0530 Subject: [PATCH 33/43] fix: upgrade conflicts --- requirements/base.txt | 4 +- requirements/ci.txt | 244 ++++++++++++++++++------------------- requirements/dev.txt | 4 +- requirements/pip_tools.txt | 2 +- requirements/quality.txt | 106 ++++++++-------- requirements/test.txt | 115 +++++++++-------- requirements/tox.txt | 8 +- 7 files changed, 241 insertions(+), 242 deletions(-) diff --git a/requirements/base.txt b/requirements/base.txt index a1958d29..7c0124f3 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -71,7 +71,7 @@ jsonfield==3.2.0 # via -r requirements/base.in lazy==1.6 # via -r requirements/base.in -lxml==6.0.2 +lxml==6.0.4 # via # -r requirements/base.in # xblock @@ -85,7 +85,7 @@ markupsafe==3.0.3 # xblock oauthlib==3.3.1 # via -r requirements/base.in -openedx-events==11.1.0 +openedx-events==11.1.1 # via -r requirements/base.in openedx-filters==3.1.0 # via -r requirements/base.in diff --git a/requirements/ci.txt b/requirements/ci.txt index 905156fb..6abf7976 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -6,23 +6,23 @@ # annotated-doc==0.0.4 # via - # -r test.txt + # -r requirements/test.txt # typer appdirs==1.4.4 # via - # -r test.txt + # -r requirements/test.txt # fs arrow==1.4.0 # via - # -r test.txt + # -r requirements/test.txt # cookiecutter asgiref==3.11.1 # via - # -r test.txt + # -r requirements/test.txt # django astroid==4.0.4 # via - # -r test.txt + # -r requirements/test.txt # pylint # pylint-celery attrs==26.1.0 @@ -31,39 +31,39 @@ attrs==26.1.0 # openedx-events binaryornot==0.6.0 # via - # -r test.txt + # -r requirements/test.txt # cookiecutter bleach==6.3.0 # via -r requirements/test.txt -boto3==1.42.84 +boto3==1.42.88 # via - # -r test.txt + # -r requirements/test.txt # fs-s3fs -botocore==1.42.84 +botocore==1.42.88 # via - # -r test.txt + # -r requirements/test.txt # boto3 # s3transfer cachetools==7.0.5 # via - # -r tox.txt + # -r requirements/tox.txt # tox certifi==2026.2.25 # via - # -r test.txt + # -r requirements/test.txt # requests cffi==2.0.0 # via - # -r test.txt + # -r requirements/test.txt # cryptography # pynacl charset-normalizer==3.4.7 # via - # -r test.txt + # -r requirements/test.txt # requests click==8.3.2 # via - # -r test.txt + # -r requirements/test.txt # click-log # code-annotations # cookiecutter @@ -72,44 +72,44 @@ click==8.3.2 # typer click-log==0.4.0 # via - # -r test.txt + # -r requirements/test.txt # edx-lint code-annotations==3.0.0 # via - # -r test.txt + # -r requirements/test.txt # edx-lint colorama==0.4.6 # via - # -r tox.txt + # -r requirements/tox.txt # tox cookiecutter==2.7.1 # via - # -r test.txt + # -r requirements/test.txt # xblock-sdk coverage[toml]==7.13.5 # via - # -r test.txt + # -r requirements/test.txt # coveralls coveralls==4.1.0 - # via -r test.txt + # via -r requirements/test.txt cryptography==46.0.7 # via - # -r test.txt + # -r requirements/test.txt # secretstorage ddt==1.7.2 - # via -r test.txt + # via -r requirements/test.txt dill==0.4.1 # via - # -r test.txt + # -r requirements/test.txt # pylint distlib==0.4.0 # via - # -r tox.txt + # -r requirements/tox.txt # virtualenv django==5.2.13 # via - # -c common_constraints.txt - # -r test.txt + # -c requirements/common_constraints.txt + # -r requirements/test.txt # django-appconf # django-config-models # django-crum @@ -124,33 +124,33 @@ django==5.2.13 # xblock-sdk django-appconf==1.2.0 # via - # -r test.txt + # -r requirements/test.txt # django-statici18n django-config-models==3.0.0 - # via -r test.txt + # via -r requirements/test.txt django-crum==0.7.9 # via - # -r test.txt + # -r requirements/test.txt # edx-django-utils django-filter==25.2 - # via -r test.txt + # via -r requirements/test.txt django-statici18n==2.7.1 - # via -r test.txt + # via -r requirements/test.txt django-waffle==5.0.0 # via - # -r test.txt + # -r requirements/test.txt # edx-django-utils djangorestframework==3.17.1 # via - # -r test.txt + # -r requirements/test.txt # django-config-models dnspython==2.8.0 # via - # -r test.txt + # -r requirements/test.txt # pymongo docutils==0.22.4 # via - # -r test.txt + # -r requirements/test.txt # readme-renderer edx-ccx-keys==2.0.2 # via @@ -158,14 +158,14 @@ edx-ccx-keys==2.0.2 # openedx-events edx-django-utils==8.0.1 # via - # -r test.txt + # -r requirements/test.txt # django-config-models # openedx-events edx-lint==6.0.0 - # via -r test.txt + # via -r requirements/test.txt edx-opaque-keys[django]==4.0.0 # via - # -r test.txt + # -r requirements/test.txt # edx-ccx-keys # openedx-events # openedx-filters @@ -175,217 +175,217 @@ fastavro==1.12.1 # openedx-events filelock==3.25.2 # via - # -r tox.txt + # -r requirements/tox.txt # python-discovery # tox # virtualenv fs==2.4.16 # via - # -r test.txt + # -r requirements/test.txt # fs-s3fs # xblock fs-s3fs==1.1.1 # via - # -r test.txt + # -r requirements/test.txt # xblock-sdk id==1.5.0 # via - # -r test.txt + # -r requirements/test.txt # twine idna==3.11 # via - # -r test.txt + # -r requirements/test.txt # requests isort==8.0.1 # via - # -r test.txt + # -r requirements/test.txt # pylint jaraco-classes==3.4.0 # via - # -r test.txt + # -r requirements/test.txt # keyring jaraco-context==6.1.2 # via - # -r test.txt + # -r requirements/test.txt # keyring jaraco-functools==4.4.0 # via - # -r test.txt + # -r requirements/test.txt # keyring jeepney==0.9.0 # via - # -r test.txt + # -r requirements/test.txt # keyring # secretstorage jinja2==3.1.6 # via - # -r test.txt + # -r requirements/test.txt # code-annotations # cookiecutter jmespath==1.1.0 # via - # -r test.txt + # -r requirements/test.txt # boto3 # botocore jsonfield==3.2.0 - # via -r test.txt + # via -r requirements/test.txt keyring==25.7.0 # via - # -r test.txt + # -r requirements/test.txt # twine lazy==1.6 - # via -r test.txt -lxml==6.0.2 + # via -r requirements/test.txt +lxml==6.0.4 # via - # -r test.txt + # -r requirements/test.txt # xblock # xblock-sdk mako==1.3.10 # via - # -r test.txt + # -r requirements/test.txt # xblock markdown-it-py==4.0.0 # via - # -r test.txt + # -r requirements/test.txt # rich markupsafe==3.0.3 # via - # -r test.txt + # -r requirements/test.txt # jinja2 # mako # xblock mccabe==0.7.0 # via - # -r test.txt + # -r requirements/test.txt # pylint mdurl==0.1.2 # via - # -r test.txt + # -r requirements/test.txt # markdown-it-py mock==5.2.0 - # via -r test.txt -more-itertools==11.0.1 + # via -r requirements/test.txt +more-itertools==11.0.2 # via - # -r test.txt + # -r requirements/test.txt # jaraco-classes # jaraco-functools nh3==0.3.4 # via - # -r test.txt + # -r requirements/test.txt # readme-renderer oauthlib==3.3.1 # via -r requirements/test.txt -openedx-events==11.1.0 +openedx-events==11.1.1 # via -r requirements/test.txt openedx-filters==3.1.0 # via -r requirements/test.txt packaging==26.0 # via - # -r test.txt - # -r tox.txt + # -r requirements/test.txt + # -r requirements/tox.txt # pyproject-api # tox # twine -platformdirs==4.9.4 +platformdirs==4.9.6 # via - # -r test.txt - # -r tox.txt + # -r requirements/test.txt + # -r requirements/tox.txt # pylint # python-discovery # tox # virtualenv pluggy==1.6.0 # via - # -r tox.txt + # -r requirements/tox.txt # tox psutil==7.2.2 # via - # -r test.txt + # -r requirements/test.txt # edx-django-utils pycodestyle==2.14.0 - # via -r test.txt + # via -r requirements/test.txt pycparser==3.0 # via - # -r test.txt + # -r requirements/test.txt # cffi pycryptodomex==3.23.0 - # via -r test.txt + # via -r requirements/test.txt pygments==2.20.0 # via - # -r test.txt + # -r requirements/test.txt # readme-renderer # rich pyjwt==2.12.1 - # via -r test.txt + # via -r requirements/test.txt pylint==4.0.5 # via - # -r test.txt + # -r requirements/test.txt # edx-lint # pylint-celery # pylint-django # pylint-plugin-utils pylint-celery==0.3 # via - # -r test.txt + # -r requirements/test.txt # edx-lint pylint-django==2.7.0 # via - # -r test.txt + # -r requirements/test.txt # edx-lint pylint-plugin-utils==0.9.0 # via - # -r test.txt + # -r requirements/test.txt # pylint-celery # pylint-django pymongo==4.16.0 # via - # -r test.txt + # -r requirements/test.txt # edx-opaque-keys pynacl==1.6.2 # via - # -r test.txt + # -r requirements/test.txt # edx-django-utils pypng==0.20220715.0 # via - # -r test.txt + # -r requirements/test.txt # xblock-sdk pyproject-api==1.10.0 # via - # -r tox.txt + # -r requirements/tox.txt # tox python-dateutil==2.9.0.post0 # via - # -r test.txt + # -r requirements/test.txt # arrow # botocore # xblock -python-discovery==1.2.1 +python-discovery==1.2.2 # via - # -r tox.txt + # -r requirements/tox.txt # tox # virtualenv python-slugify==8.0.4 # via - # -r test.txt + # -r requirements/test.txt # code-annotations # cookiecutter pytz==2026.1.post1 # via - # -r test.txt + # -r requirements/test.txt # xblock pyyaml==6.0.3 # via - # -r test.txt + # -r requirements/test.txt # code-annotations # cookiecutter # xblock readme-renderer==44.0 # via - # -r test.txt + # -r requirements/test.txt # twine requests==2.33.1 # via - # -r test.txt + # -r requirements/test.txt # cookiecutter # coveralls # id @@ -394,38 +394,38 @@ requests==2.33.1 # xblock-sdk requests-toolbelt==1.0.0 # via - # -r test.txt + # -r requirements/test.txt # twine rfc3986==2.0.0 # via - # -r test.txt + # -r requirements/test.txt # twine -rich==14.3.3 +rich==15.0.0 # via - # -r test.txt + # -r requirements/test.txt # cookiecutter # twine # typer s3transfer==0.16.0 # via - # -r test.txt + # -r requirements/test.txt # boto3 secretstorage==3.5.0 # via - # -r test.txt + # -r requirements/test.txt # keyring shellingham==1.5.4 # via - # -r test.txt + # -r requirements/test.txt # typer simplejson==3.20.2 # via - # -r test.txt + # -r requirements/test.txt # xblock # xblock-sdk six==1.17.0 # via - # -r test.txt + # -r requirements/test.txt # edx-ccx-keys # edx-lint # fs @@ -433,70 +433,70 @@ six==1.17.0 # python-dateutil sqlparse==0.5.5 # via - # -r test.txt + # -r requirements/test.txt # django stevedore==5.7.0 # via - # -r test.txt + # -r requirements/test.txt # code-annotations # edx-django-utils # edx-opaque-keys text-unidecode==1.3 # via - # -r test.txt + # -r requirements/test.txt # python-slugify tomli-w==1.2.0 # via - # -r tox.txt + # -r requirements/tox.txt # tox tomlkit==0.14.0 # via - # -r test.txt + # -r requirements/test.txt # pylint -tox==4.52.0 - # via -r tox.txt +tox==4.52.1 + # via -r requirements/tox.txt twine==6.2.0 - # via -r test.txt + # via -r requirements/test.txt typer==0.24.1 # via - # -r test.txt + # -r requirements/test.txt # coveralls typing-extensions==4.15.0 # via - # -r test.txt + # -r requirements/test.txt # edx-opaque-keys tzdata==2026.1 # via - # -r test.txt + # -r requirements/test.txt # arrow urllib3==1.26.20 # via - # -c constraints.txt - # -r test.txt + # -c requirements/constraints.txt + # -r requirements/test.txt # botocore # requests # twine -virtualenv==21.2.0 +virtualenv==21.2.1 # via - # -r tox.txt + # -r requirements/tox.txt # tox web-fragments==4.0.0 # via - # -r test.txt + # -r requirements/test.txt # xblock # xblock-sdk webencodings==0.5.1 # via - # -r test.txt + # -r requirements/test.txt # bleach webob==1.8.9 # via - # -r test.txt + # -r requirements/test.txt # xblock # xblock-sdk xblock==6.0.0 # via - # -r test.txt + # -r requirements/test.txt # xblock-sdk xblock-sdk==0.14.0 # via -r requirements/test.txt diff --git a/requirements/dev.txt b/requirements/dev.txt index aa3ba68d..d0e616b9 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -96,7 +96,7 @@ jsonfield==3.2.0 # via -r requirements/base.txt lazy==1.6 # via -r requirements/base.txt -lxml[html-clean]==6.0.2 +lxml[html-clean]==6.0.4 # via # -r requirements/base.txt # edx-i18n-tools @@ -115,7 +115,7 @@ markupsafe==3.0.3 # xblock oauthlib==3.3.1 # via -r requirements/base.txt -openedx-events==11.1.0 +openedx-events==11.1.1 # via -r requirements/base.txt openedx-filters==3.1.0 # via -r requirements/base.txt diff --git a/requirements/pip_tools.txt b/requirements/pip_tools.txt index 6669aebe..81dcfcd5 100644 --- a/requirements/pip_tools.txt +++ b/requirements/pip_tools.txt @@ -4,7 +4,7 @@ # # make upgrade # -build==1.4.2 +build==1.4.3 # via pip-tools click==8.3.2 # via pip-tools diff --git a/requirements/quality.txt b/requirements/quality.txt index 32a725b3..d6a98589 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -6,13 +6,13 @@ # appdirs==1.4.4 # via - # -r base.txt + # -r requirements/base.txt # fs arrow==1.4.0 # via cookiecutter asgiref==3.11.1 # via - # -r base.txt + # -r requirements/base.txt # django astroid==4.0.4 # via @@ -26,9 +26,9 @@ binaryornot==0.6.0 # via cookiecutter bleach==6.3.0 # via -r requirements/base.txt -boto3==1.42.84 +boto3==1.42.88 # via fs-s3fs -botocore==1.42.84 +botocore==1.42.88 # via # boto3 # s3transfer @@ -36,14 +36,14 @@ certifi==2026.2.25 # via requests cffi==2.0.0 # via - # -r base.txt + # -r requirements/base.txt # cryptography # pynacl charset-normalizer==3.4.7 # via requests click==8.3.2 # via - # -r base.txt + # -r requirements/base.txt # click-log # code-annotations # cookiecutter @@ -56,15 +56,15 @@ code-annotations==3.0.0 cookiecutter==2.7.1 # via xblock-sdk cryptography==46.0.7 - # via -r quality.in + # via -r requirements/quality.in ddt==1.7.2 - # via -r quality.in + # via -r requirements/quality.in dill==0.4.1 # via pylint django==5.2.13 # via - # -c common_constraints.txt - # -r base.txt + # -c requirements/common_constraints.txt + # -r requirements/base.txt # django-appconf # django-config-models # django-crum @@ -79,29 +79,29 @@ django==5.2.13 # xblock-sdk django-appconf==1.2.0 # via - # -r base.txt + # -r requirements/base.txt # django-statici18n django-config-models==3.0.0 - # via -r base.txt + # via -r requirements/base.txt django-crum==0.7.9 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils django-filter==25.2 - # via -r base.txt + # via -r requirements/base.txt django-statici18n==2.7.1 - # via -r base.txt + # via -r requirements/base.txt django-waffle==5.0.0 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils djangorestframework==3.17.1 # via - # -r base.txt + # -r requirements/base.txt # django-config-models dnspython==2.8.0 # via - # -r base.txt + # -r requirements/base.txt # pymongo edx-ccx-keys==2.0.2 # via @@ -109,14 +109,14 @@ edx-ccx-keys==2.0.2 # openedx-events edx-django-utils==8.0.1 # via - # -r base.txt + # -r requirements/base.txt # django-config-models # openedx-events edx-lint==6.0.0 - # via -r quality.in + # via -r requirements/quality.in edx-opaque-keys[django]==4.0.0 # via - # -r base.txt + # -r requirements/base.txt # edx-ccx-keys # openedx-events # openedx-filters @@ -126,7 +126,7 @@ fastavro==1.12.1 # openedx-events fs==2.4.16 # via - # -r base.txt + # -r requirements/base.txt # fs-s3fs # xblock fs-s3fs==1.1.1 @@ -144,23 +144,23 @@ jmespath==1.1.0 # boto3 # botocore jsonfield==3.2.0 - # via -r base.txt + # via -r requirements/base.txt lazy==1.6 - # via -r base.txt -lxml==6.0.2 + # via -r requirements/base.txt +lxml==6.0.4 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk mako==1.3.10 # via - # -r base.txt + # -r requirements/base.txt # xblock markdown-it-py==4.0.0 # via rich markupsafe==3.0.3 # via - # -r base.txt + # -r requirements/base.txt # jinja2 # mako # xblock @@ -170,31 +170,31 @@ mdurl==0.1.2 # via markdown-it-py oauthlib==3.3.1 # via -r requirements/base.txt -openedx-events==11.1.0 +openedx-events==11.1.1 # via -r requirements/base.txt openedx-filters==3.1.0 # via -r requirements/base.txt -platformdirs==4.9.4 +platformdirs==4.9.6 # via pylint psutil==7.2.2 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils pycodestyle==2.14.0 - # via -r quality.in + # via -r requirements/quality.in pycparser==3.0 # via - # -r base.txt + # -r requirements/base.txt # cffi pycryptodomex==3.23.0 - # via -r base.txt + # via -r requirements/base.txt pygments==2.20.0 # via rich pyjwt==2.12.1 - # via -r base.txt + # via -r requirements/base.txt pylint==4.0.5 # via - # -r quality.in + # -r requirements/quality.in # edx-lint # pylint-celery # pylint-django @@ -209,17 +209,17 @@ pylint-plugin-utils==0.9.0 # pylint-django pymongo==4.16.0 # via - # -r base.txt + # -r requirements/base.txt # edx-opaque-keys pynacl==1.6.2 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils pypng==0.20220715.0 # via xblock-sdk python-dateutil==2.9.0.post0 # via - # -r base.txt + # -r requirements/base.txt # arrow # botocore # xblock @@ -229,11 +229,11 @@ python-slugify==8.0.4 # cookiecutter pytz==2026.1.post1 # via - # -r base.txt + # -r requirements/base.txt # xblock pyyaml==6.0.3 # via - # -r base.txt + # -r requirements/base.txt # code-annotations # cookiecutter # xblock @@ -241,18 +241,18 @@ requests==2.33.1 # via # cookiecutter # xblock-sdk -rich==14.3.3 +rich==15.0.0 # via cookiecutter s3transfer==0.16.0 # via boto3 simplejson==3.20.2 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk six==1.17.0 # via - # -r base.txt + # -r requirements/base.txt # edx-ccx-keys # edx-lint # fs @@ -260,11 +260,11 @@ six==1.17.0 # python-dateutil sqlparse==0.5.5 # via - # -r base.txt + # -r requirements/base.txt # django stevedore==5.7.0 # via - # -r base.txt + # -r requirements/base.txt # code-annotations # edx-django-utils # edx-opaque-keys @@ -274,35 +274,35 @@ tomlkit==0.14.0 # via pylint typing-extensions==4.15.0 # via - # -r base.txt + # -r requirements/base.txt # edx-opaque-keys tzdata==2026.1 # via arrow urllib3==1.26.20 # via - # -c constraints.txt + # -c requirements/constraints.txt # botocore # requests web-fragments==4.0.0 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk webencodings==0.5.1 # via - # -r base.txt + # -r requirements/base.txt # bleach webob==1.8.9 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk xblock==6.0.0 # via - # -r base.txt + # -r requirements/base.txt # xblock-sdk xblock-sdk==0.14.0 - # via -r quality.in + # via -r requirements/quality.in # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/requirements/test.txt b/requirements/test.txt index 8accc09c..ecdf438b 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -8,13 +8,13 @@ annotated-doc==0.0.4 # via typer appdirs==1.4.4 # via - # -r base.txt + # -r requirements/base.txt # fs arrow==1.4.0 # via cookiecutter asgiref==3.11.1 # via - # -r base.txt + # -r requirements/base.txt # django astroid==4.0.4 # via @@ -28,9 +28,9 @@ binaryornot==0.6.0 # via cookiecutter bleach==6.3.0 # via -r requirements/base.txt -boto3==1.42.84 +boto3==1.42.88 # via fs-s3fs -botocore==1.42.84 +botocore==1.42.88 # via # boto3 # s3transfer @@ -38,14 +38,14 @@ certifi==2026.2.25 # via requests cffi==2.0.0 # via - # -r base.txt + # -r requirements/base.txt # cryptography # pynacl charset-normalizer==3.4.7 # via requests click==8.3.2 # via - # -r base.txt + # -r requirements/base.txt # click-log # code-annotations # cookiecutter @@ -61,19 +61,18 @@ cookiecutter==2.7.1 coverage[toml]==7.13.5 # via coveralls coveralls==4.1.0 - # via -r test.in + # via -r requirements/test.in cryptography==46.0.7 # via - # -r test.in + # -r requirements/test.in # secretstorage ddt==1.7.2 - # via -r test.in + # via -r requirements/test.in dill==0.4.1 # via pylint -django==5.2.13 # via - # -c common_constraints.txt - # -r base.txt + # -c requirements/common_constraints.txt + # -r requirements/base.txt # django-appconf # django-config-models # django-crum @@ -88,30 +87,30 @@ django==5.2.13 # xblock-sdk django-appconf==1.2.0 # via - # -r base.txt + # -r requirements/base.txt # django-statici18n django-config-models==3.0.0 - # via -r base.txt + # via -r requirements/base.txt django-crum==0.7.9 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils django-filter==25.2 - # via -r base.txt + # via -r requirements/base.txt django-statici18n==2.7.1 - # via -r base.txt + # via -r requirements/base.txt django-waffle==5.0.0 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils djangorestframework==3.17.1 # via - # -r base.txt - # -r test.in + # -r requirements/base.txt + # -r requirements/test.in # django-config-models dnspython==2.8.0 # via - # -r base.txt + # -r requirements/base.txt # pymongo docutils==0.22.4 # via readme-renderer @@ -121,14 +120,14 @@ edx-ccx-keys==2.0.2 # openedx-events edx-django-utils==8.0.1 # via - # -r base.txt + # -r requirements/base.txt # django-config-models # openedx-events edx-lint==6.0.0 - # via -r test.in + # via -r requirements/test.in edx-opaque-keys[django]==4.0.0 # via - # -r base.txt + # -r requirements/base.txt # edx-ccx-keys # openedx-events # openedx-filters @@ -138,7 +137,7 @@ fastavro==1.12.1 # openedx-events fs==2.4.16 # via - # -r base.txt + # -r requirements/base.txt # fs-s3fs # xblock fs-s3fs==1.1.1 @@ -168,25 +167,25 @@ jmespath==1.1.0 # boto3 # botocore jsonfield==3.2.0 - # via -r base.txt + # via -r requirements/base.txt keyring==25.7.0 # via twine lazy==1.6 - # via -r base.txt -lxml==6.0.2 + # via -r requirements/base.txt +lxml==6.0.4 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk mako==1.3.10 # via - # -r base.txt + # -r requirements/base.txt # xblock markdown-it-py==4.0.0 # via rich markupsafe==3.0.3 # via - # -r base.txt + # -r requirements/base.txt # jinja2 # mako # xblock @@ -195,8 +194,8 @@ mccabe==0.7.0 mdurl==0.1.2 # via markdown-it-py mock==5.2.0 - # via -r test.in -more-itertools==11.0.1 + # via -r requirements/test.in +more-itertools==11.0.2 # via # jaraco-classes # jaraco-functools @@ -204,32 +203,32 @@ nh3==0.3.4 # via readme-renderer oauthlib==3.3.1 # via -r requirements/base.txt -openedx-events==11.1.0 +openedx-events==11.1.1 # via -r requirements/base.txt openedx-filters==3.1.0 # via -r requirements/base.txt packaging==26.0 # via twine -platformdirs==4.9.4 +platformdirs==4.9.6 # via pylint psutil==7.2.2 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils pycodestyle==2.14.0 - # via -r test.in + # via -r requirements/test.in pycparser==3.0 # via - # -r base.txt + # -r requirements/base.txt # cffi pycryptodomex==3.23.0 - # via -r base.txt + # via -r requirements/base.txt pygments==2.20.0 # via # readme-renderer # rich pyjwt==2.12.1 - # via -r base.txt + # via -r requirements/base.txt pylint==4.0.5 # via # edx-lint @@ -246,17 +245,17 @@ pylint-plugin-utils==0.9.0 # pylint-django pymongo==4.16.0 # via - # -r base.txt + # -r requirements/base.txt # edx-opaque-keys pynacl==1.6.2 # via - # -r base.txt + # -r requirements/base.txt # edx-django-utils pypng==0.20220715.0 # via xblock-sdk python-dateutil==2.9.0.post0 # via - # -r base.txt + # -r requirements/base.txt # arrow # botocore # xblock @@ -266,17 +265,17 @@ python-slugify==8.0.4 # cookiecutter pytz==2026.1.post1 # via - # -r base.txt + # -r requirements/base.txt # xblock pyyaml==6.0.3 # via - # -r base.txt + # -r requirements/base.txt # code-annotations # cookiecutter # xblock readme-renderer==44.0 # via - # -r test.in + # -r requirements/test.in # twine requests==2.33.1 # via @@ -290,7 +289,7 @@ requests-toolbelt==1.0.0 # via twine rfc3986==2.0.0 # via twine -rich==14.3.3 +rich==15.0.0 # via # cookiecutter # twine @@ -303,12 +302,12 @@ shellingham==1.5.4 # via typer simplejson==3.20.2 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk six==1.17.0 # via - # -r base.txt + # -r requirements/base.txt # edx-ccx-keys # edx-lint # fs @@ -316,11 +315,11 @@ six==1.17.0 # python-dateutil sqlparse==0.5.5 # via - # -r base.txt + # -r requirements/base.txt # django stevedore==5.7.0 # via - # -r base.txt + # -r requirements/base.txt # code-annotations # edx-django-utils # edx-opaque-keys @@ -329,38 +328,38 @@ text-unidecode==1.3 tomlkit==0.14.0 # via pylint twine==6.2.0 - # via -r test.in + # via -r requirements/test.in typer==0.24.1 # via coveralls typing-extensions==4.15.0 # via - # -r base.txt + # -r requirements/base.txt # edx-opaque-keys tzdata==2026.1 # via arrow urllib3==1.26.20 # via - # -c constraints.txt + # -c requirements/constraints.txt # botocore # requests # twine web-fragments==4.0.0 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk webencodings==0.5.1 # via - # -r base.txt + # -r requirements/base.txt # bleach webob==1.8.9 # via - # -r base.txt + # -r requirements/base.txt # xblock # xblock-sdk xblock==6.0.0 # via - # -r base.txt + # -r requirements/base.txt # xblock-sdk xblock-sdk==0.14.0 # via -r requirements/test.in diff --git a/requirements/tox.txt b/requirements/tox.txt index 39d4c836..25b2180b 100644 --- a/requirements/tox.txt +++ b/requirements/tox.txt @@ -19,7 +19,7 @@ packaging==26.0 # via # pyproject-api # tox -platformdirs==4.9.4 +platformdirs==4.9.6 # via # python-discovery # tox @@ -28,13 +28,13 @@ pluggy==1.6.0 # via tox pyproject-api==1.10.0 # via tox -python-discovery==1.2.1 +python-discovery==1.2.2 # via # tox # virtualenv tomli-w==1.2.0 # via tox -tox==4.52.0 +tox==4.52.1 # via -r requirements/tox.in -virtualenv==21.2.0 +virtualenv==21.2.1 # via tox From c2c7ec70875bfc5cb1e4a963be15759acab9e3de Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 13 Apr 2026 20:20:01 +0530 Subject: [PATCH 34/43] refactor: robust duplicate signal handler --- lti_consumer/signals/signals.py | 41 ++++++++++++-- lti_consumer/tests/unit/test_signals.py | 75 ++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/lti_consumer/signals/signals.py b/lti_consumer/signals/signals.py index b5f239b1..26e477f6 100644 --- a/lti_consumer/signals/signals.py +++ b/lti_consumer/signals/signals.py @@ -11,6 +11,7 @@ from lti_consumer.models import Lti1p3Passport, LtiAgsScore, LtiConfiguration from lti_consumer.plugin import compat +from lti_consumer.utils import model_to_dict log = logging.getLogger(__name__) SignalHandler = compat.get_signal_handler() @@ -160,12 +161,40 @@ def duplicate_xblock_lti_configuration(**kwargs): log.error("Received null or incorrect data for event") return - src_lti_config = LtiConfiguration.objects.get(location=str(xblock_data.source_usage_key)) - copy = src_lti_config - copy.pk = None - copy.location = str(xblock_data.usage_key) - copy.config_id = uuid.uuid4() - copy.save() + source_usage_key = str(xblock_data.source_usage_key) + target_usage_key = str(xblock_data.usage_key) + + src_lti_config = LtiConfiguration.objects.filter(location=source_usage_key).first() + if not src_lti_config: + log.warning("No LTI configuration found to duplicate. source=%s", source_usage_key) + return + + if LtiConfiguration.objects.filter(location=target_usage_key).exists(): + log.info( + "Target already has LTI configuration. Skipping duplicate. target=%s source=%s", + target_usage_key, + source_usage_key, + ) + return + + # Convert the LTI configuration to a dictionary and set the new location and config_id + # to copy lticonfiguration data to the new location and config_id + payload = model_to_dict( + src_lti_config, + # Include all unique fields and generated ones. + exclude=["id", "pk", "location", "config_id"], + ) + payload["location"] = target_usage_key + payload["config_id"] = uuid.uuid4() + + try: + LtiConfiguration.objects.create(**payload) + except Exception: # pylint: disable=broad-exception-caught + log.exception( + "Failed duplicating LTI configuration. source=%s target=%s", + source_usage_key, + target_usage_key, + ) LTI_1P3_PROCTORING_ASSESSMENT_STARTED = Signal() diff --git a/lti_consumer/tests/unit/test_signals.py b/lti_consumer/tests/unit/test_signals.py index b0c3e41e..e26bd5d9 100644 --- a/lti_consumer/tests/unit/test_signals.py +++ b/lti_consumer/tests/unit/test_signals.py @@ -7,13 +7,14 @@ from ddt import data, ddt, unpack from django.test import TestCase from opaque_keys.edx.keys import UsageKey -from openedx_events.content_authoring.data import LibraryBlockData, XBlockData +from openedx_events.content_authoring.data import DuplicatedXBlockData, LibraryBlockData, XBlockData from lti_consumer.models import LtiAgsLineItem, LtiAgsScore, LtiConfiguration from lti_consumer.signals.signals import ( delete_child_lti_configurations, delete_lib_lti_configuration, delete_lti_configuration, + duplicate_xblock_lti_configuration, ) @@ -400,3 +401,75 @@ def test_delete_lib_lti_configuration_extra_kwargs_ignored(self): mocks['lti_config'].objects.filter.assert_called_once() mocks['log'].info.assert_called_once() + + +class TestDuplicateXblockLtiConfiguration(TestCase): + """Tests for duplicate_xblock_lti_configuration function.""" + + def setUp(self): + self.source_usage_key = UsageKey.from_string( + "block-v1:course+101+2024+type@lti_consumer+block@source" + ) + self.target_usage_key = UsageKey.from_string( + "block-v1:course+101+2024+type@lti_consumer+block@target" + ) + self.xblock_data = Mock(spec=DuplicatedXBlockData) + self.xblock_data.source_usage_key = self.source_usage_key + self.xblock_data.usage_key = self.target_usage_key + + @patch('lti_consumer.signals.signals.model_to_dict') + @patch('lti_consumer.signals.signals.LtiConfiguration') + def test_duplicate_success(self, mock_lti_config, mock_model_to_dict): + """Duplicate creates copy when source exists and target empty.""" + source_config = Mock() + mock_lti_config.objects.filter.return_value.first.side_effect = [source_config] + mock_lti_config.objects.filter.return_value.exists.return_value = False + mock_model_to_dict.return_value = { + 'field_a': 'value_a', + 'field_b': 'value_b', + } + + duplicate_xblock_lti_configuration(xblock_info=self.xblock_data) + + mock_lti_config.objects.filter.assert_any_call(location=str(self.source_usage_key)) + mock_lti_config.objects.filter.assert_any_call(location=str(self.target_usage_key)) + mock_model_to_dict.assert_called_once_with( + source_config, + exclude=["id", "pk", "location", "config_id"], + ) + mock_lti_config.objects.create.assert_called_once() + create_kwargs = mock_lti_config.objects.create.call_args.kwargs + assert create_kwargs['field_a'] == 'value_a' + assert create_kwargs['field_b'] == 'value_b' + assert create_kwargs['location'] == str(self.target_usage_key) + assert create_kwargs['config_id'] is not None + + @patch('lti_consumer.signals.signals.log') + @patch('lti_consumer.signals.signals.LtiConfiguration') + def test_duplicate_source_missing(self, mock_lti_config, mock_log): + """No source config triggers warning and exits.""" + mock_lti_config.objects.filter.return_value.first.return_value = None + mock_lti_config.objects.filter.return_value.exists.return_value = False + + duplicate_xblock_lti_configuration(xblock_info=self.xblock_data) + + mock_log.warning.assert_called_once() + mock_lti_config.objects.create.assert_not_called() + + @patch('lti_consumer.signals.signals.log') + @patch('lti_consumer.signals.signals.LtiConfiguration') + def test_duplicate_target_exists(self, mock_lti_config, mock_log): + """Existing target config skips duplication.""" + mock_lti_config.objects.filter.return_value.first.return_value = Mock() + mock_lti_config.objects.filter.return_value.exists.return_value = True + + duplicate_xblock_lti_configuration(xblock_info=self.xblock_data) + + mock_log.info.assert_called_once() + mock_lti_config.objects.create.assert_not_called() + + @patch('lti_consumer.signals.signals.log') + def test_duplicate_invalid_input(self, mock_log): + """Invalid xblock_info logs error.""" + duplicate_xblock_lti_configuration(xblock_info=None) + mock_log.error.assert_called_once_with("Received null or incorrect data for event") From 3a4115062d6bec592068f412a85607060822595e Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 14 Apr 2026 15:50:18 +0530 Subject: [PATCH 35/43] fix: migration for missing location field in configurations Adds tests covering possible cases --- .../0021_create_lti_1p3_passport.py | 29 +- ...ontext_key_lti1p3passport_name_and_more.py | 2 +- lti_consumer/tests/unit/test_migrations.py | 299 ++++++++++++++++++ 3 files changed, 315 insertions(+), 15 deletions(-) create mode 100644 lti_consumer/tests/unit/test_migrations.py diff --git a/lti_consumer/migrations/0021_create_lti_1p3_passport.py b/lti_consumer/migrations/0021_create_lti_1p3_passport.py index f5d97cfa..d3ba7886 100644 --- a/lti_consumer/migrations/0021_create_lti_1p3_passport.py +++ b/lti_consumer/migrations/0021_create_lti_1p3_passport.py @@ -7,7 +7,7 @@ from django.db import migrations -def create_lti_1p3_passport(apps, _): # pragma: nocover +def create_lti_1p3_passport(apps, _): """Copy config_id from LtiConsumer to LtiConsumerXBlock.""" from lti_consumer.plugin.compat import load_enough_xblock, save_xblock # pylint: disable=import-outside-toplevel from lti_consumer.utils import model_to_dict # pylint: disable=import-outside-toplevel @@ -16,13 +16,6 @@ def create_lti_1p3_passport(apps, _): # pragma: nocover Lti1p3Passport = apps.get_model("lti_consumer", "Lti1p3Passport") for configuration in LtiConfiguration.objects.all(): - try: - block = load_enough_xblock(configuration.location) - block.lti_1p3_passport_id = str(configuration.config_id) - block.save() - save_xblock(block) - except Exception as e: # pylint: disable=broad-exception-caught - print(f"Failed to copy passport_id for configuration {configuration}: {e}") values = model_to_dict( configuration, include=[ @@ -34,12 +27,20 @@ def create_lti_1p3_passport(apps, _): # pragma: nocover 'lti_1p3_tool_keyset_url', ], ) - if block.config_type == "new": - # Data is stored xblock - values.update({ - 'lti_1p3_tool_public_key': block.lti_1p3_tool_public_key, - 'lti_1p3_tool_keyset_url': block.lti_1p3_tool_keyset_url, - }) + if configuration.location: + try: + block = load_enough_xblock(configuration.location) + block.lti_1p3_passport_id = str(configuration.config_id) + block.save() + save_xblock(block) + if block.config_type == "new": + # Data is stored xblock + values.update({ + 'lti_1p3_tool_public_key': block.lti_1p3_tool_public_key, + 'lti_1p3_tool_keyset_url': block.lti_1p3_tool_keyset_url, + }) + except Exception as e: # pylint: disable=broad-exception-caught + print(f"Failed to copy passport_id for configuration {configuration}: {e}") passport, _ = Lti1p3Passport.objects.update_or_create( # Use config_id as passport_id to preserve existing urls that use it. passport_id=configuration.config_id, diff --git a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py index 4710aaca..e85dc761 100644 --- a/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py +++ b/lti_consumer/migrations/0023_lti1p3passport_context_key_lti1p3passport_name_and_more.py @@ -4,7 +4,7 @@ from django.db import migrations, models -def set_name_and_context_key(apps, _): # pragma: nocover +def set_name_and_context_key(apps, _): """ Copy name and context_key from Block to LTI1p3Passport. """ diff --git a/lti_consumer/tests/unit/test_migrations.py b/lti_consumer/tests/unit/test_migrations.py new file mode 100644 index 00000000..91589ec1 --- /dev/null +++ b/lti_consumer/tests/unit/test_migrations.py @@ -0,0 +1,299 @@ +import importlib +import uuid +from unittest import mock + +from django.db import connection +from django.db.migrations.executor import MigrationExecutor +from django.test import TransactionTestCase + + +class Test0021CreateLti1p3Passport(TransactionTestCase): + """Exercise data migration 0021 across success and edge-case paths.""" + + # MigrationExecutor changes real DB state, so use TransactionTestCase. + reset_sequences = True + + migrate_from = [("lti_consumer", "0020_lti1p3passport_lticonfiguration_lti_1p3_passport")] + migrate_to = [("lti_consumer", "0021_create_lti_1p3_passport")] + + def setUp(self): + super().setUp() + self.executor = MigrationExecutor(connection) + # Start from schema/state immediately before migration under test. + self.executor.migrate(self.migrate_from) + + old_apps = self.executor.loader.project_state(self.migrate_from).apps + LtiConfiguration = old_apps.get_model("lti_consumer", "LtiConfiguration") + + self.config_id = uuid.uuid4() + self.location = "block-v1:org+course+run+type@lti+block@block" + + # Seed historical row using old app registry, not current model class. + self.configuration = LtiConfiguration.objects.create( + config_id=self.config_id, + version="lti_1p3", + config_store="CONFIG_ON_XBLOCK", + location=self.location, + lti_1p3_internal_private_key="db-private-key", + lti_1p3_internal_private_key_id="db-kid", + lti_1p3_internal_public_jwk='{"kty": "RSA"}', + lti_1p3_client_id="db-client-id", + lti_1p3_tool_public_key="db-tool-public-key", + lti_1p3_tool_keyset_url="https://db.example/jwks.json", + ) + + def test_migration_creates_and_links_passport(self): + """New-style block overrides tool key values from block fields.""" + class FakeBlock: + config_type = "new" + lti_1p3_tool_public_key = "xblock-tool-public-key" + lti_1p3_tool_keyset_url = "https://xblock.example/jwks.json" + + def __init__(self): + self.lti_1p3_passport_id = None + self.save = mock.Mock() + + fake_block = FakeBlock() + values = { + "lti_1p3_internal_private_key": "db-private-key", + "lti_1p3_internal_private_key_id": "db-kid", + "lti_1p3_internal_public_jwk": '{"kty": "RSA"}', + "lti_1p3_client_id": "db-client-id", + "lti_1p3_tool_public_key": "db-tool-public-key", + "lti_1p3_tool_keyset_url": "https://db.example/jwks.json", + } + + with mock.patch("lti_consumer.plugin.compat.load_enough_xblock", return_value=fake_block) as mock_load, \ + mock.patch("lti_consumer.plugin.compat.save_xblock") as mock_save_xblock, \ + mock.patch("lti_consumer.utils.model_to_dict", return_value=values): + self.executor.loader.build_graph() + self.executor.migrate(self.migrate_to) + + new_apps = self.executor.loader.project_state(self.migrate_to).apps + LtiConfiguration = new_apps.get_model("lti_consumer", "LtiConfiguration") + Lti1p3Passport = new_apps.get_model("lti_consumer", "Lti1p3Passport") + + configuration = LtiConfiguration.objects.get(pk=self.configuration.pk) + passport = Lti1p3Passport.objects.get(passport_id=self.config_id) + + self.assertEqual(configuration.lti_1p3_passport_id, passport.id) + self.assertEqual(passport.lti_1p3_internal_private_key, "db-private-key") + self.assertEqual(passport.lti_1p3_internal_private_key_id, "db-kid") + self.assertEqual(passport.lti_1p3_internal_public_jwk, '{"kty": "RSA"}') + self.assertEqual(passport.lti_1p3_client_id, "db-client-id") + self.assertEqual(passport.lti_1p3_tool_public_key, "xblock-tool-public-key") + self.assertEqual(passport.lti_1p3_tool_keyset_url, "https://xblock.example/jwks.json") + + self.assertEqual(fake_block.lti_1p3_passport_id, str(self.config_id)) + fake_block.save.assert_called_once_with() + mock_load.assert_called_once() + self.assertEqual(str(mock_load.call_args.args[0]), self.location) + mock_save_xblock.assert_called_once_with(fake_block) + + def test_migration_creates_passport_when_xblock_load_fails(self): + """Missing location skips XBlock path but still creates passport from DB.""" + # No location means migration should not try loading or saving block. + self.configuration.location = None + self.configuration.save(update_fields=["location"]) + values = { + "lti_1p3_internal_private_key": "db-private-key", + "lti_1p3_internal_private_key_id": "db-kid", + "lti_1p3_internal_public_jwk": '{"kty": "RSA"}', + "lti_1p3_client_id": "db-client-id", + "lti_1p3_tool_public_key": "db-tool-public-key", + "lti_1p3_tool_keyset_url": "https://db.example/jwks.json", + } + + with mock.patch( + "lti_consumer.plugin.compat.load_enough_xblock", + side_effect=Exception("boom"), + ) as mock_load, mock.patch("lti_consumer.plugin.compat.save_xblock") as mock_save_xblock, \ + mock.patch("lti_consumer.utils.model_to_dict", return_value=values), \ + mock.patch("builtins.print") as mock_print: + self.executor.loader.build_graph() + self.executor.migrate(self.migrate_to) + + new_apps = self.executor.loader.project_state(self.migrate_to).apps + LtiConfiguration = new_apps.get_model("lti_consumer", "LtiConfiguration") + Lti1p3Passport = new_apps.get_model("lti_consumer", "Lti1p3Passport") + + configuration = LtiConfiguration.objects.get(pk=self.configuration.pk) + passport = Lti1p3Passport.objects.get(passport_id=self.config_id) + + self.assertEqual(configuration.lti_1p3_passport_id, passport.id) + self.assertEqual(passport.lti_1p3_internal_private_key, "db-private-key") + self.assertEqual(passport.lti_1p3_internal_private_key_id, "db-kid") + self.assertEqual(passport.lti_1p3_internal_public_jwk, '{"kty": "RSA"}') + self.assertEqual(passport.lti_1p3_client_id, "db-client-id") + self.assertEqual(passport.lti_1p3_tool_public_key, "db-tool-public-key") + self.assertEqual(passport.lti_1p3_tool_keyset_url, "https://db.example/jwks.json") + + mock_load.assert_not_called() + mock_save_xblock.assert_not_called() + mock_print.assert_not_called() + + def test_migration_keeps_db_values_when_block_not_new(self): + """Non-new block keeps DB tool key values even when block loads.""" + class FakeBlock: + config_type = "legacy" + lti_1p3_tool_public_key = "xblock-tool-public-key" + lti_1p3_tool_keyset_url = "https://xblock.example/jwks.json" + + def __init__(self): + self.lti_1p3_passport_id = None + self.save = mock.Mock() + + fake_block = FakeBlock() + values = { + "lti_1p3_internal_private_key": "db-private-key", + "lti_1p3_internal_private_key_id": "db-kid", + "lti_1p3_internal_public_jwk": '{"kty": "RSA"}', + "lti_1p3_client_id": "db-client-id", + "lti_1p3_tool_public_key": "db-tool-public-key", + "lti_1p3_tool_keyset_url": "https://db.example/jwks.json", + } + + with mock.patch("lti_consumer.plugin.compat.load_enough_xblock", return_value=fake_block) as mock_load, \ + mock.patch("lti_consumer.plugin.compat.save_xblock") as mock_save_xblock, \ + mock.patch("lti_consumer.utils.model_to_dict", return_value=values): + self.executor.loader.build_graph() + self.executor.migrate(self.migrate_to) + + new_apps = self.executor.loader.project_state(self.migrate_to).apps + LtiConfiguration = new_apps.get_model("lti_consumer", "LtiConfiguration") + Lti1p3Passport = new_apps.get_model("lti_consumer", "Lti1p3Passport") + + configuration = LtiConfiguration.objects.get(pk=self.configuration.pk) + passport = Lti1p3Passport.objects.get(passport_id=self.config_id) + + self.assertEqual(configuration.lti_1p3_passport_id, passport.id) + self.assertEqual(passport.lti_1p3_tool_public_key, "db-tool-public-key") + self.assertEqual(passport.lti_1p3_tool_keyset_url, "https://db.example/jwks.json") + + self.assertEqual(fake_block.lti_1p3_passport_id, str(self.config_id)) + fake_block.save.assert_called_once_with() + self.assertEqual(str(mock_load.call_args.args[0]), self.location) + mock_save_xblock.assert_called_once_with(fake_block) + + +class Test0023SetPassportNameAndContextKey(TransactionTestCase): + """Exercise data migration 0023 across success and edge-case paths.""" + + reset_sequences = True + + migrate_from = [("lti_consumer", "0022_remove_lticonfiguration_lti_1p3_client_id_and_more")] + migrate_to = [("lti_consumer", "0023_lti1p3passport_context_key_lti1p3passport_name_and_more")] + + def setUp(self): + super().setUp() + self.executor = MigrationExecutor(connection) + # Start from schema/state immediately before migration under test. + self.executor.migrate(self.migrate_from) + + old_apps = self.executor.loader.project_state(self.migrate_from).apps + LtiConfiguration = old_apps.get_model("lti_consumer", "LtiConfiguration") + Lti1p3Passport = old_apps.get_model("lti_consumer", "Lti1p3Passport") + + self.config_id = uuid.uuid4() + self.location = "block-v1:org+course+run+type@lti+block@passport" + + # Seed historical rows using old app registry, not current model classes. + self.passport = Lti1p3Passport.objects.create( + passport_id=self.config_id, + lti_1p3_internal_private_key="db-private-key", + lti_1p3_internal_private_key_id="db-kid", + lti_1p3_internal_public_jwk='{"kty": "RSA"}', + lti_1p3_client_id="db-client-id", + lti_1p3_tool_public_key="db-tool-public-key", + lti_1p3_tool_keyset_url="https://db.example/jwks.json", + ) + self.configuration = LtiConfiguration.objects.create( + config_id=uuid.uuid4(), + version="lti_1p3", + config_store="CONFIG_ON_XBLOCK", + location=self.location, + lti_1p3_passport=self.passport, + ) + + def test_migration_sets_name_and_context_key_from_block(self): + """Populate new passport fields from block when passport has no name yet.""" + + class FakeBlock: + display_name = "Unit 1 LTI" + context_id = "course-v1:org+course+run" + + with mock.patch("lti_consumer.plugin.compat.load_enough_xblock", return_value=FakeBlock()) as mock_load: + self.executor.loader.build_graph() + self.executor.migrate(self.migrate_to) + + new_apps = self.executor.loader.project_state(self.migrate_to).apps + Lti1p3Passport = new_apps.get_model("lti_consumer", "Lti1p3Passport") + + passport = Lti1p3Passport.objects.get(pk=self.passport.pk) + + self.assertEqual(passport.name, "Passport of Unit 1 LTI") + self.assertEqual(passport.context_key, "course-v1:org+course+run") + mock_load.assert_called_once() + self.assertEqual(str(mock_load.call_args.args[0]), self.location) + + def test_migration_skips_xblock_path_when_location_missing(self): + """Missing location skips block load and leaves new passport fields empty.""" + self.configuration.location = None + self.configuration.save(update_fields=["location"]) + + with mock.patch( + "lti_consumer.plugin.compat.load_enough_xblock", + side_effect=Exception("boom"), + ) as mock_load, mock.patch("builtins.print") as mock_print: + self.executor.loader.build_graph() + self.executor.migrate(self.migrate_to) + + new_apps = self.executor.loader.project_state(self.migrate_to).apps + Lti1p3Passport = new_apps.get_model("lti_consumer", "Lti1p3Passport") + + passport = Lti1p3Passport.objects.get(pk=self.passport.pk) + + self.assertIsNone(passport.name) + self.assertIsNone(passport.context_key) + mock_load.assert_not_called() + mock_print.assert_not_called() + + def test_migration_keeps_existing_name_and_context_key(self): + """Existing passport name prevents overwrite from block values.""" + + class InitialBlock: + display_name = "Initial title" + context_id = "initial-context" + + with mock.patch("lti_consumer.plugin.compat.load_enough_xblock", return_value=InitialBlock()): + self.executor.loader.build_graph() + self.executor.migrate(self.migrate_to) + + new_apps = self.executor.loader.project_state(self.migrate_to).apps + LtiConfiguration = new_apps.get_model("lti_consumer", "LtiConfiguration") + Lti1p3Passport = new_apps.get_model("lti_consumer", "Lti1p3Passport") + + passport = Lti1p3Passport.objects.get(pk=self.passport.pk) + passport.name = "Existing passport name" + passport.context_key = "existing-context-key" + passport.save(update_fields=["name", "context_key"]) + + configuration = LtiConfiguration.objects.get(pk=self.configuration.pk) + + class FakeBlock: + display_name = "Changed title" + context_id = "changed-context" + + with mock.patch("lti_consumer.plugin.compat.load_enough_xblock", return_value=FakeBlock()) as mock_load: + migration_module = importlib.import_module( + "lti_consumer.migrations.0023_lti1p3passport_context_key_lti1p3passport_name_and_more" + ) + migration_module.set_name_and_context_key(new_apps, None) + + passport.refresh_from_db() + configuration.refresh_from_db() + + self.assertEqual(configuration.lti_1p3_passport_id, passport.id) + self.assertEqual(passport.name, "Existing passport name") + self.assertEqual(passport.context_key, "existing-context-key") + self.assertEqual(str(mock_load.call_args.args[0]), self.location) From 4831f178ba19d51c60f70d3a4dd5151fa9a33930 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 14 Apr 2026 15:52:24 +0530 Subject: [PATCH 36/43] fix: lint issues --- lti_consumer/tests/unit/test_migrations.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lti_consumer/tests/unit/test_migrations.py b/lti_consumer/tests/unit/test_migrations.py index 91589ec1..43c32501 100644 --- a/lti_consumer/tests/unit/test_migrations.py +++ b/lti_consumer/tests/unit/test_migrations.py @@ -1,3 +1,6 @@ +""" +Tests for custom migrations scripts +""" import importlib import uuid from unittest import mock @@ -45,6 +48,7 @@ def setUp(self): def test_migration_creates_and_links_passport(self): """New-style block overrides tool key values from block fields.""" class FakeBlock: + """Dummy class for testing new-style block overrides""" config_type = "new" lti_1p3_tool_public_key = "xblock-tool-public-key" lti_1p3_tool_keyset_url = "https://xblock.example/jwks.json" @@ -135,6 +139,7 @@ def test_migration_creates_passport_when_xblock_load_fails(self): def test_migration_keeps_db_values_when_block_not_new(self): """Non-new block keeps DB tool key values even when block loads.""" class FakeBlock: + """Dummy class for testing new-style block overrides""" config_type = "legacy" lti_1p3_tool_public_key = "xblock-tool-public-key" lti_1p3_tool_keyset_url = "https://xblock.example/jwks.json" @@ -219,6 +224,7 @@ def test_migration_sets_name_and_context_key_from_block(self): """Populate new passport fields from block when passport has no name yet.""" class FakeBlock: + """Dummy class for testing new-style block overrides""" display_name = "Unit 1 LTI" context_id = "course-v1:org+course+run" @@ -281,6 +287,7 @@ class InitialBlock: configuration = LtiConfiguration.objects.get(pk=self.configuration.pk) class FakeBlock: + """Dummy class for testing new-style block overrides""" display_name = "Changed title" context_id = "changed-context" From cf4bd3393eb4302bb1d427ee11284174fbe82e5a Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 16 Apr 2026 15:56:34 +0530 Subject: [PATCH 37/43] refactor: remove logic that update block fields in migration --- .../migrations/0021_create_lti_1p3_passport.py | 5 +---- lti_consumer/tests/unit/test_migrations.py | 16 ++-------------- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/lti_consumer/migrations/0021_create_lti_1p3_passport.py b/lti_consumer/migrations/0021_create_lti_1p3_passport.py index d3ba7886..cf1ee616 100644 --- a/lti_consumer/migrations/0021_create_lti_1p3_passport.py +++ b/lti_consumer/migrations/0021_create_lti_1p3_passport.py @@ -9,7 +9,7 @@ def create_lti_1p3_passport(apps, _): """Copy config_id from LtiConsumer to LtiConsumerXBlock.""" - from lti_consumer.plugin.compat import load_enough_xblock, save_xblock # pylint: disable=import-outside-toplevel + from lti_consumer.plugin.compat import load_enough_xblock # pylint: disable=import-outside-toplevel from lti_consumer.utils import model_to_dict # pylint: disable=import-outside-toplevel LtiConfiguration = apps.get_model("lti_consumer", "LtiConfiguration") @@ -30,9 +30,6 @@ def create_lti_1p3_passport(apps, _): if configuration.location: try: block = load_enough_xblock(configuration.location) - block.lti_1p3_passport_id = str(configuration.config_id) - block.save() - save_xblock(block) if block.config_type == "new": # Data is stored xblock values.update({ diff --git a/lti_consumer/tests/unit/test_migrations.py b/lti_consumer/tests/unit/test_migrations.py index 43c32501..5c076284 100644 --- a/lti_consumer/tests/unit/test_migrations.py +++ b/lti_consumer/tests/unit/test_migrations.py @@ -53,10 +53,6 @@ class FakeBlock: lti_1p3_tool_public_key = "xblock-tool-public-key" lti_1p3_tool_keyset_url = "https://xblock.example/jwks.json" - def __init__(self): - self.lti_1p3_passport_id = None - self.save = mock.Mock() - fake_block = FakeBlock() values = { "lti_1p3_internal_private_key": "db-private-key", @@ -88,11 +84,9 @@ def __init__(self): self.assertEqual(passport.lti_1p3_tool_public_key, "xblock-tool-public-key") self.assertEqual(passport.lti_1p3_tool_keyset_url, "https://xblock.example/jwks.json") - self.assertEqual(fake_block.lti_1p3_passport_id, str(self.config_id)) - fake_block.save.assert_called_once_with() mock_load.assert_called_once() self.assertEqual(str(mock_load.call_args.args[0]), self.location) - mock_save_xblock.assert_called_once_with(fake_block) + mock_save_xblock.assert_not_called() def test_migration_creates_passport_when_xblock_load_fails(self): """Missing location skips XBlock path but still creates passport from DB.""" @@ -144,10 +138,6 @@ class FakeBlock: lti_1p3_tool_public_key = "xblock-tool-public-key" lti_1p3_tool_keyset_url = "https://xblock.example/jwks.json" - def __init__(self): - self.lti_1p3_passport_id = None - self.save = mock.Mock() - fake_block = FakeBlock() values = { "lti_1p3_internal_private_key": "db-private-key", @@ -175,10 +165,8 @@ def __init__(self): self.assertEqual(passport.lti_1p3_tool_public_key, "db-tool-public-key") self.assertEqual(passport.lti_1p3_tool_keyset_url, "https://db.example/jwks.json") - self.assertEqual(fake_block.lti_1p3_passport_id, str(self.config_id)) - fake_block.save.assert_called_once_with() self.assertEqual(str(mock_load.call_args.args[0]), self.location) - mock_save_xblock.assert_called_once_with(fake_block) + mock_save_xblock.assert_not_called() class Test0023SetPassportNameAndContextKey(TransactionTestCase): From 1ca3e5f352f90be0b89dc1ca805ca610461eaa51 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 16 Apr 2026 16:36:07 +0530 Subject: [PATCH 38/43] refactor: add passport id to xml --- lti_consumer/lti_xblock.py | 28 +++++++++++++---- lti_consumer/tests/unit/test_lti_xblock.py | 35 +++++++++++++++++++++- 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 5f291d76..19cf7015 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -58,6 +58,7 @@ import bleach from django.conf import settings +from django.core.exceptions import ObjectDoesNotExist from django.utils import timezone from web_fragments.fragment import Fragment from webob import Response @@ -74,19 +75,19 @@ 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, 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__) @@ -1846,3 +1847,20 @@ def index_dictionary(self): xblock_body["content_type"] = "LTI Consumer" return xblock_body + + def add_xml_to_node(self, node): + """ + Export XBlock XML and include passport_id from database when available. + """ + super().add_xml_to_node(node) + + try: + from .models import LtiConfiguration # pylint: disable=import-outside-toplevel + + configuration = LtiConfiguration.objects.select_related("lti_1p3_passport").get( + location=self.scope_ids.usage_id, + ) + if configuration.lti_1p3_passport: + node.set("lti_1p3_passport_id", str(configuration.lti_1p3_passport.passport_id)) + except (LtiConfiguration.DoesNotExist, ObjectDoesNotExist): + pass diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index e3913ae1..a97138b3 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -7,6 +7,7 @@ from datetime import timedelta from itertools import product from unittest.mock import Mock, PropertyMock, patch +from xml.etree.ElementTree import Element import ddt import jwt @@ -23,13 +24,15 @@ from lti_consumer.exceptions import LtiError from lti_consumer.lti_1p3.tests.utils import create_jwt from lti_consumer.lti_xblock import LtiConsumerXBlock, parse_handler_suffix, valid_config_type_values +from lti_consumer.models import Lti1p3Passport, LtiConfiguration from lti_consumer.tests import test_utils from lti_consumer.tests.test_utils import ( FAKE_USER_ID, + TestBaseWithPatch, get_mock_lti_configuration, make_jwt_request, make_request, - make_xblock, TestBaseWithPatch + make_xblock, ) from lti_consumer.utils import resolve_custom_parameter_template @@ -73,6 +76,36 @@ def setUp(self): self.compat.get_external_id_for_user.return_value = "12345" +class TestAddXmlToNode(TestCase): + """Unit tests for export XML on LtiConsumerXBlock.""" + + def test_add_xml_to_node_includes_passport_id_from_database(self): + xblock = make_xblock( + 'lti_consumer', + LtiConsumerXBlock, + { + 'lti_version': 'lti_1p3', + 'lti_1p3_launch_url': 'http://tool.example/launch', + 'lti_1p3_oidc_url': 'http://tool.example/oidc', + 'lti_1p3_passport_id': '', + }, + ) + passport = Lti1p3Passport.objects.create() + LtiConfiguration.objects.create( + location=xblock.scope_ids.usage_id, + version=LtiConfiguration.LTI_1P3, + config_store=LtiConfiguration.CONFIG_ON_DB, + lti_1p3_passport=passport, + ) + node = Element('lti_consumer') + + with patch('xblock.core.XBlock.add_xml_to_node') as mock_super: + xblock.add_xml_to_node(node) + + mock_super.assert_called_once_with(node) + self.assertEqual(node.get('lti_1p3_passport_id'), str(passport.passport_id)) + + class TestIndexibility(TestCase): """ Test indexibility of Lti Consumer XBlock From 9391906955a6deaeff725c78bce91ca941f90189 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 9 Apr 2026 19:40:35 +0530 Subject: [PATCH 39/43] fix: ags result endpoint to work with or without ending slash Fixes: https://github.com/openedx/xblock-lti-consumer/issues/628 --- .../extensions/rest_framework/serializers.py | 12 +++-- lti_consumer/plugin/views.py | 2 +- .../tests/unit/plugin/test_views_lti_ags.py | 51 +++++++++++++++---- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py index 953cee40..a8f3323c 100644 --- a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py +++ b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py @@ -194,13 +194,15 @@ class LtiAgsResultSerializer(serializers.ModelSerializer): def get_id(self, obj): request = self.context.get('request') + kwargs = { + 'lti_config_id': obj.line_item.lti_configuration.id, + 'pk': obj.line_item.pk, + } + if obj.user_id: + kwargs['user_id'] = obj.user_id return reverse( 'lti_consumer:lti-ags-view-results', - kwargs={ - 'lti_config_id': obj.line_item.lti_configuration.id, - 'pk': obj.line_item.pk, - 'user_id': obj.user_id, - }, + kwargs=kwargs, request=request, ) diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index 1f00b8f6..f42279cb 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -748,7 +748,7 @@ def perform_create(self, serializer): @action( detail=True, methods=['GET'], - url_path='results/(?P[^/.]+)?', + url_path=r'results(?:/(?P[^/.]+))?/?', renderer_classes=[LineItemResultsRenderer], content_negotiation_class=IgnoreContentNegotiation, ) diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py index 08e4acf8..c1335ab6 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -177,10 +177,7 @@ def test_lti_ags_list(self): response.data, [ { - 'id': 'http://testserver/lti_consumer/v1/lti/{}/lti-ags/{}'.format( - self.lti_config.id, - line_item.id - ), + 'id': f'http://testserver/lti_consumer/v1/lti/{self.lti_config.id}/lti-ags/{line_item.id}', 'resourceId': 'test', 'scoreMaximum': 100, 'label': 'test label', @@ -221,10 +218,7 @@ def test_lti_ags_retrieve(self): self.assertEqual( response.data, { - 'id': 'http://testserver/lti_consumer/v1/lti/{}/lti-ags/{}'.format( - self.lti_config.id, - line_item.id - ), + 'id': f'http://testserver/lti_consumer/v1/lti/{self.lti_config.id}/lti-ags/{line_item.id}', 'resourceId': 'test', 'scoreMaximum': 100, 'label': 'test label', @@ -936,11 +930,14 @@ def test_retrieve_results(self): # Create Score response = self.client.get(self.results_endpoint) + response_with_trailing_slash = self.client.get(self.results_endpoint + '/') self.assertEqual(response.status_code, 200) + self.assertEqual(response_with_trailing_slash.status_code, 200) # There should be 2 results (not include the empty score user's result) self.assertEqual(len(response.data), 2) + self.assertEqual(len(response_with_trailing_slash.data), 2) # Check the data primary_user_results_endpoint = reverse( @@ -983,7 +980,7 @@ def test_retrieve_results(self): def test_retrieve_results_for_user_id(self): """ - Test the LTI AGS LineItem Resul Retrieval for a single user. + Test the LTI AGS LineItem Result Retrieval for a single user. """ self._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/result.readonly') @@ -996,14 +993,23 @@ def test_retrieve_results_for_user_id(self): } ) + results_user_endpoint_with_trailing_slash = results_user_endpoint + '/' + # Request results with userId response = self.client.get(results_user_endpoint, data={"userId": self.secondary_user_id}) + response_with_trailing_slash = self.client.get( + results_user_endpoint_with_trailing_slash, + data={"userId": self.secondary_user_id}, + ) self.assertEqual(response.status_code, 200) + self.assertEqual(response_with_trailing_slash.status_code, 200) # There should be 1 result for that user self.assertEqual(len(response.data), 1) + self.assertEqual(len(response_with_trailing_slash.data), 1) self.assertEqual(response.data[0]['userId'], self.secondary_user_id) + self.assertEqual(response_with_trailing_slash.data[0]['userId'], self.secondary_user_id) def test_retrieve_results_with_limit(self): """ @@ -1023,3 +1029,30 @@ def test_retrieve_results_with_limit(self): # `primary_user_id` was assigned to the record with the `late_timestamp` self.assertEqual(len(response.data), 1) self.assertEqual(response.data[0]['userId'], self.primary_user_id) + + def test_results_serializer_id_includes_user_id_separator(self): + """ + Test that the results serializer builds a valid URL for a user-specific result. + """ + self._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/result.readonly') + + results_user_endpoint = reverse( + 'lti_consumer:lti-ags-view-results', + kwargs={ + "lti_config_id": self.lti_config.id, + "pk": self.line_item.id, + "user_id": self.secondary_user_id, + } + ) + + response = self.client.get(results_user_endpoint, data={"userId": self.secondary_user_id}) + + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data), 1) + self.assertEqual( + response.data[0]['id'], + ( + f'http://testserver/lti_consumer/v1/lti/{self.lti_config.id}/lti-ags' + f'/{self.line_item.id}/results/{self.secondary_user_id}' + ), + ) From 1234199b109617accbf29b09a89d57f6da0ff5a0 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 9 Apr 2026 20:00:59 +0530 Subject: [PATCH 40/43] fix: allow blank comment in scores api endpoint Fixes: https://github.com/openedx/xblock-lti-consumer/issues/637 --- .../extensions/rest_framework/serializers.py | 2 +- .../tests/unit/plugin/test_views_lti_ags.py | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py index a8f3323c..fa24a78b 100644 --- a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py +++ b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py @@ -125,7 +125,7 @@ class LtiAgsScoreSerializer(serializers.ModelSerializer): timestamp = serializers.DateTimeField(input_formats=[ISO_8601], format=ISO_8601, default_timezone=timezone.utc) scoreGiven = serializers.FloatField(source='score_given', required=False, allow_null=True, default=None) scoreMaximum = serializers.FloatField(source='score_maximum', required=False, allow_null=True, default=None) - comment = serializers.CharField(required=False, allow_null=True) + comment = serializers.CharField(required=False, allow_null=True, allow_blank=True) activityProgress = serializers.CharField(source='activity_progress') gradingProgress = serializers.CharField(source='grading_progress') userId = serializers.CharField(source='user_id') diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py index c1335ab6..8bb376ca 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -395,6 +395,43 @@ def test_create_score(self): self.assertEqual(score.grading_progress, LtiAgsScore.FULLY_GRADED) self.assertEqual(score.user_id, self.primary_user_id) + @ddt.data(None, "") + def test_create_score_without_comment(self, comment): + """ + Test the LTI AGS LineItem Score Creation when comment is omitted or blank. + """ + self._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/score') + + data = { + "timestamp": self.early_timestamp, + "scoreGiven": 83, + "scoreMaximum": 100, + "activityProgress": LtiAgsScore.COMPLETED, + "gradingProgress": LtiAgsScore.FULLY_GRADED, + "userId": self.primary_user_id, + } + if comment is not None: + data["comment"] = comment + + response = self.client.post( + self.scores_endpoint, + data=json.dumps(data), + content_type="application/vnd.ims.lis.v1.score+json", + ) + + self.assertEqual(response.status_code, 201) + self.assertEqual(LtiAgsScore.objects.filter( + line_item=self.line_item, + user_id=self.primary_user_id + ).count(), 1) + + score = LtiAgsScore.objects.get(line_item=self.line_item, user_id=self.primary_user_id) + if comment is None: + self.assertIsNone(score.comment) + else: + self.assertEqual(score.comment, comment) + score.delete() + def _post_lti_score(self, override_data=None): """ Helper method to post a LTI score From d5dbe3fb31be6b14d0fd7c7560bbc739de622fd3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Sun, 12 Apr 2026 19:52:54 +0530 Subject: [PATCH 41/43] fix: use correct deep linking launch uri as target_link_uri --- lti_consumer/lti_1p3/consumer.py | 33 ++++++++++++++++- lti_consumer/lti_1p3/tests/test_consumer.py | 41 +++++++++++++++++++-- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py index 56984ad0..9cba4e10 100644 --- a/lti_consumer/lti_1p3/consumer.py +++ b/lti_consumer/lti_1p3/consumer.py @@ -77,6 +77,15 @@ def __init__( # Extra claims - used by LTI Advantage self.extra_claims = {} + def _get_target_link_uri(self, launch_data): # pylint: disable=unused-argument + """ + Return the target_link_uri to use for the provided launch data. + + Subclasses can override this to customize the target link selection for + different message types. + """ + return self.launch_url + @staticmethod def _get_user_roles(role): """ @@ -125,12 +134,14 @@ def prepare_preflight_url( oidc_url = self.oidc_url + "?" + target_link_uri = self._get_target_link_uri(launch_data) + login_hint = user_id parameters = { "iss": self.iss, "client_id": self.client_id, "lti_deployment_id": self.deployment_id, - "target_link_uri": self.launch_url, + "target_link_uri": target_link_uri, "login_hint": login_hint, "lti_message_hint": launch_data_key, } @@ -302,6 +313,7 @@ def set_custom_parameters( def get_lti_launch_message( self, include_extra_claims=True, + target_link_uri=None, ): """ Build LTI message from class parameters @@ -312,6 +324,8 @@ def get_lti_launch_message( # Start from base message lti_message = LTI_BASE_MESSAGE.copy() + target_link_uri = target_link_uri or self.launch_url + # Add base parameters lti_message.update({ # Issuer @@ -329,7 +343,7 @@ def get_lti_launch_message( # Target Link URI: actual endpoint for the LTI resource to display # MUST be the same value as the target_link_uri passed by the platform in the OIDC login request # http://www.imsglobal.org/spec/lti/v1p3/#target-link-uri - "https://purl.imsglobal.org/spec/lti/claim/target_link_uri": self.launch_url, + "https://purl.imsglobal.org/spec/lti/claim/target_link_uri": target_link_uri, }) # Check if user data is set, then append it to lti message @@ -609,6 +623,19 @@ def lti_dl_enabled(self): else: return False + def _get_target_link_uri(self, launch_data): + """ + Use the deep linking launch URL for deep linking requests. + """ + if ( + getattr(self, 'dl', None) and + launch_data and + getattr(launch_data, 'message_type', None) == 'LtiDeepLinkingRequest' + ): + return self.dl.deep_linking_launch_url + + return super()._get_target_link_uri(launch_data) + def enable_ags( self, lineitems_url, @@ -663,12 +690,14 @@ def generate_launch_request( # Check if Deep Linking is enabled and that this is a Deep Link Launch if self.dl and launch_data.message_type == "LtiDeepLinkingRequest": + target_link_uri = self._get_target_link_uri(launch_data) # Validate preflight response self._validate_preflight_response(preflight_response) # Get LTI Launch Message lti_launch_message = self.get_lti_launch_message( include_extra_claims=False, + target_link_uri=target_link_uri, ) # Update message type to LtiDeepLinkingRequest, diff --git a/lti_consumer/lti_1p3/tests/test_consumer.py b/lti_consumer/lti_1p3/tests/test_consumer.py index 5f2eeb25..5c810289 100644 --- a/lti_consumer/lti_1p3/tests/test_consumer.py +++ b/lti_consumer/lti_1p3/tests/test_consumer.py @@ -28,7 +28,8 @@ ISS = "http://test-platform.example/" OIDC_URL = "http://test-platform/oidc" LAUNCH_URL = "http://test-platform/launch" -REDIRECT_URIS = [LAUNCH_URL] +DEEP_LINK_LAUNCH_URL = "http://test-platform/deep_link_launch" +REDIRECT_URIS = [LAUNCH_URL, DEEP_LINK_LAUNCH_URL] CLIENT_ID = "1" DEPLOYMENT_ID = "1" NONCE = "1234" @@ -739,14 +740,14 @@ def _setup_deep_linking(self): """ Set's up deep linking class in LTI consumer. """ - self.lti_consumer.enable_deep_linking("launch-url", "return-url") + self.lti_consumer.enable_deep_linking(DEEP_LINK_LAUNCH_URL, "return-url") lti_message_hint = self._setup_lti_message_hint() # Set LTI Consumer parameters self.preflight_response = { "client_id": CLIENT_ID, - "redirect_uri": LAUNCH_URL, + "redirect_uri": DEEP_LINK_LAUNCH_URL, "nonce": NONCE, "state": STATE, "lti_message_hint": lti_message_hint, @@ -819,6 +820,40 @@ def test_deep_linking_enabled_launch_request(self): "return-url" ) + def test_deep_linking_preflight_uses_deep_link_launch_url(self): + """ + Ensure deep linking launches send the deep linking launch URL as target_link_uri during login initiation. + """ + self.lti_consumer.enable_deep_linking(DEEP_LINK_LAUNCH_URL, "return-url") + launch_data = Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id="1", + resource_link_id="resource_link_id", + message_type="LtiDeepLinkingRequest", + ) + + preflight_request_data = self.lti_consumer.prepare_preflight_url(launch_data) + parameters = parse_qs(urlparse(preflight_request_data).query) + + self.assertEqual(parameters['target_link_uri'][0], DEEP_LINK_LAUNCH_URL) + + def test_deep_linking_launch_request_sets_target_link_uri(self): + """ + Ensure the ID token for deep linking launches uses the deep linking launch URL as target_link_uri. + """ + self._setup_deep_linking() + + token = self.lti_consumer.generate_launch_request( + self.preflight_response, + )['id_token'] + + decoded_token = self.lti_consumer.key_handler.validate_and_decode(token) + self.assertEqual( + decoded_token['https://purl.imsglobal.org/spec/lti/claim/target_link_uri'], + DEEP_LINK_LAUNCH_URL, + ) + def test_deep_linking_token_decode_no_dl(self): """ Check that trying to run the Deep Linking decoding fails if service is not set up. From f1471d0edd8609394229e7696205a53f82221756 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Sun, 12 Apr 2026 20:13:28 +0530 Subject: [PATCH 42/43] fix: lint issues --- lti_consumer/lti_1p3/extensions/rest_framework/serializers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py index fa24a78b..c1d48aaa 100644 --- a/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py +++ b/lti_consumer/lti_1p3/extensions/rest_framework/serializers.py @@ -193,6 +193,9 @@ class LtiAgsResultSerializer(serializers.ModelSerializer): comment = serializers.CharField() def get_id(self, obj): + """ + Return result URL for score. Include user_id when score scoped to learner. + """ request = self.context.get('request') kwargs = { 'lti_config_id': obj.line_item.lti_configuration.id, From 66c31f644990f830daa56877358d82f32b6c8b9f Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 17 Apr 2026 18:03:13 +0530 Subject: [PATCH 43/43] fix: use correct key-secret pair for lti 1.1 --- lti_consumer/lti_1p1/exceptions.py | 3 ++- lti_consumer/lti_xblock.py | 13 ++++++------- lti_consumer/outcomes.py | 12 ++++++++++-- lti_consumer/tests/unit/test_lti_xblock.py | 9 ++++++--- lti_consumer/tests/unit/test_outcomes.py | 8 ++++---- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/lti_consumer/lti_1p1/exceptions.py b/lti_consumer/lti_1p1/exceptions.py index 80383ae8..748cfe49 100644 --- a/lti_consumer/lti_1p1/exceptions.py +++ b/lti_consumer/lti_1p1/exceptions.py @@ -1,9 +1,10 @@ """ Exceptions for the LTI1.1 Consumer. """ +from lti_consumer.exceptions import LtiError -class Lti1p1Error(Exception): +class Lti1p1Error(LtiError): """ General error class for LTI1.1 Consumer usage. """ diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 19cf7015..f5dd9504 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -1110,7 +1110,7 @@ def is_past_due(self): close_date = due_date return close_date is not None and timezone.now() > close_date - def _get_lti_consumer(self): + def get_lti_consumer(self): """ Returns a preconfigured LTI consumer depending on the value. @@ -1283,7 +1283,7 @@ def lti_launch_handler(self, request, suffix=''): # pylint: disable=unused-argu Returns: webob.response: HTML LTI launch form """ - lti_consumer = self._get_lti_consumer() + lti_consumer = self.get_lti_consumer() # Occassionally, users try to do an LTI launch while they are unauthenticated. It is not known why this occurs. # Sometimes, it is due to a web crawlers; other times, it is due to actual users of the platform. Regardless, @@ -1386,7 +1386,7 @@ def lti_1p3_access_token(self, request, suffix=''): # pylint: disable=unused-ar # Asserting that the consumer can be created. This makes sure that the LtiConfiguration # object exists before calling the Django View - assert self._get_lti_consumer() + assert self.get_lti_consumer() # Runtime import because this can only be run in the LMS/Studio Django # environments. Importing the views on the top level will cause RuntimeErorr from lti_consumer.plugin.views import access_token_endpoint # pylint: disable=import-outside-toplevel @@ -1442,12 +1442,11 @@ def result_service_handler(self, request, suffix=''): Returns: webob.response: response to this request. See above for details. """ - lti_consumer = self._get_lti_consumer() + lti_consumer = self.get_lti_consumer() lti_consumer.set_outcome_service_url(self.outcome_service_url) if settings.DEBUG: - lti_provider_key, lti_provider_secret = self.lti_provider_key_secret - log_authorization_header(request, lti_provider_key, lti_provider_secret) + log_authorization_header(request, lti_consumer.oauth_key, lti_consumer.oauth_secret) if not self.accept_grades_past_due and self.is_past_due: return Response(status=404) # have to do 404 due to spec, but 400 is better, with error msg in body @@ -1759,7 +1758,7 @@ def _get_context_for_template(self): # Don't pull from the Django database unless the config_type is one that stores the LTI configuration in the # database. if self.config_type in ("database", "external"): - lti_consumer = self._get_lti_consumer() + lti_consumer = self.get_lti_consumer() launch_url = self._get_lti_launch_url(lti_consumer) lti_block_launch_handler = self._get_lti_block_launch_handler() diff --git a/lti_consumer/outcomes.py b/lti_consumer/outcomes.py index 72bc759c..7de7587b 100644 --- a/lti_consumer/outcomes.py +++ b/lti_consumer/outcomes.py @@ -6,10 +6,12 @@ """ import logging -from xml.sax.saxutils import escape from urllib.parse import unquote +from xml.sax.saxutils import escape +from django.conf import settings from lxml import etree + try: from xblock.utils.resources import ResourceLoader except ModuleNotFoundError: # For backward compatibility with releases older than Quince. @@ -176,7 +178,13 @@ def handle_request(self, request): return response_xml_template.format(**failure_values) # Verify OAuth signing. - __, secret = self.xblock.lti_provider_key_secret + secret = self.xblock.get_lti_consumer().oauth_secret + if settings.DEBUG: + log.debug( + "[LTI]: verifying OAuth body signature for outcomes. service_url=%s request.url=%s", + self.xblock.outcome_service_url, + request.url, + ) try: verify_oauth_body_signature(request, secret, self.xblock.outcome_service_url) except (ValueError, LtiError) as ex: diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index a97138b3..0f5fdcd0 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -1196,12 +1196,15 @@ def setUp(self): @override_settings(DEBUG=True) @patch('lti_consumer.lti_xblock.log_authorization_header') - @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret') - def test_runtime_debug_true(self, mock_lti_provider_key_secret, mock_log_auth_header): + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock._get_lti_consumer') + def test_runtime_debug_true(self, mock_get_lti_consumer, mock_log_auth_header): """ Test `log_authorization_header` is called when settings.DEBUG is True """ - mock_lti_provider_key_secret.__get__ = Mock(return_value=(self.lti_provider_key, self.lti_provider_secret)) + mock_get_lti_consumer.return_value = Mock( + oauth_key=self.lti_provider_key, + oauth_secret=self.lti_provider_secret, + ) request = make_request('', 'GET') self.xblock.result_service_handler(request) diff --git a/lti_consumer/tests/unit/test_outcomes.py b/lti_consumer/tests/unit/test_outcomes.py index 8008f9f7..c5854d3c 100644 --- a/lti_consumer/tests/unit/test_outcomes.py +++ b/lti_consumer/tests/unit/test_outcomes.py @@ -348,7 +348,7 @@ def setUp(self): self.mock_get_user_id_patcher_enabled.return_value = mock_user @patch('lti_consumer.outcomes.verify_oauth_body_signature', Mock(return_value=True)) - @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's'))) + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock._get_lti_consumer', Mock(return_value=Mock(oauth_secret='s'))) @patch('lti_consumer.outcomes.parse_grade_xml_body', Mock(return_value=('', '', 0.5, 'replaceResultRequest'))) def test_handle_replace_result_success(self): """ @@ -407,7 +407,7 @@ def test_xml_parse_lti_error(self, mock_parse): self.assertIn('Request body XML parsing error', response) @patch('lti_consumer.outcomes.verify_oauth_body_signature') - @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's'))) + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock._get_lti_consumer', Mock(return_value=Mock(oauth_secret='s'))) @patch('lti_consumer.outcomes.parse_grade_xml_body', Mock(return_value=('', '', 0.5, 'replaceResultRequest'))) def test_invalid_signature(self, mock_verify): """ @@ -422,7 +422,7 @@ def test_invalid_signature(self, mock_verify): self.assertIn('failure', self.outcome_service.handle_request(request)) @patch('lti_consumer.outcomes.verify_oauth_body_signature', Mock(return_value=True)) - @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's'))) + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock._get_lti_consumer', Mock(return_value=Mock(oauth_secret='s'))) @patch('lti_consumer.outcomes.parse_grade_xml_body', Mock(return_value=('', '', 0.5, 'replaceResultRequest'))) def test_user_not_found(self): """ @@ -437,7 +437,7 @@ def test_user_not_found(self): self.assertIn('User not found', response) @patch('lti_consumer.outcomes.verify_oauth_body_signature', Mock(return_value=True)) - @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's'))) + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock._get_lti_consumer', Mock(return_value=Mock(oauth_secret='s'))) @patch('lti_consumer.outcomes.parse_grade_xml_body', Mock(return_value=('', '', 0.5, 'unsupportedRequest'))) def test_unsupported_action(self): """