Skip to content

Commit 11d7df5

Browse files
navinkarkerafeanil
andauthored
feat: LTI 1.3 Passport Refactor + Database Cleanup Support (#627)
* 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 * 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 * fix(lti): correct spelling and improve logging in signal handlers • 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 * fix: lint issues * feat: install openedx-events * fix: model label * fix: handle no location * test: fix tests * fix: lint issues * fixup! fix: lint issues * test: add signal tests * fix: coverage * refactor(models): rename create_lti_1p3_passport to get_or_create_lti_1p3_passport * fix: copy-paste bug when both public key and keyset url is specified * fix: lint issues * test: improve coverage * refactor(views): remove unnecessary db call * 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. * fix: create name only if block is available * fix: test * refactor: migration * chore: upgrade * refactor: avoid duplicate signal triggers * refactor: api * refactor: rename * fix: tests * chore: fix lint issues * test: add some more * fix: tests * fix: coverage issue * chore: upgrade * fix: handle duplicate block explicitly * fix: upgrade conflicts * refactor: robust duplicate signal handler * fix: migration for missing location field in configurations Adds tests covering possible cases * fix: lint issues * refactor: remove logic that update block fields in migration * refactor: add passport id to xml * fix(migrations): restore config fields from passport on reverse * refactor: apply suggestions and update docs * feat: bump version and update changelog * docs: Update lti_consumer/lti_xblock.py * docs: Update lti_consumer/migrations/0021_create_lti_1p3_passport.py * fix: remove unrelated file * fix: update requirements * refactor: remove save_xblock helper As we add passport_id from database while exporting xml, we don't need to add it to the block explicitly. This avoids modifying the xblock outside of author edit cycle. * test: fix tests * Revert "refactor: remove save_xblock helper" This reverts commit 8e5b926. * Revert "test: fix tests" This reverts commit fac60e2. * chore: fix typo --------- Co-authored-by: Feanil Patel <feanil@axim.org>
1 parent 4fb7bba commit 11d7df5

39 files changed

Lines changed: 2219 additions & 421 deletions

.coveragerc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@
22
[run]
33
data_file = .coverage
44
source = lti_consumer
5-
omit = */urls.py
5+
omit =
6+
*/urls.py

CHANGELOG.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ Please See the `releases tab <https://github.com/openedx/xblock-lti-consumer/rel
1616
Unreleased
1717
~~~~~~~~~~
1818

19+
11.0.0 - 2026-04-20
20+
--------------------
21+
* Split LTI 1.3 Configuration into Passport Model
22+
* Fix duplicate, copy-paste for LTI xblocks
23+
* Add signal handlers for events like delete, duplicate etc.
24+
1925
10.0.1 - 2026-03-17
2026
--------------------
2127
* Revert the quoting of location/usage_keys done in version 9.14.4 & 9.14.5.

lti_consumer/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,4 @@
44
from .apps import LTIConsumerApp
55
from .lti_xblock import LtiConsumerXBlock
66

7-
__version__ = '10.0.1'
7+
__version__ = '11.0.0'

lti_consumer/admin.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,33 @@
77
from lti_consumer.forms import CourseAllowPIISharingInLTIAdminForm
88
from lti_consumer.models import (
99
CourseAllowPIISharingInLTIFlag,
10+
Lti1p3Passport,
1011
LtiAgsLineItem,
1112
LtiAgsScore,
1213
LtiConfiguration,
1314
LtiDlContentItem,
1415
)
1516

1617

18+
class LtiConfigurationInline(admin.TabularInline):
19+
"""
20+
Inline for the LtiConfiguration models in Lti1p3Passport.
21+
"""
22+
model = LtiConfiguration
23+
extra = 0
24+
can_delete = False
25+
fields = ('location',)
26+
27+
def has_change_permission(self, request, obj=None): # pragma: nocover
28+
return False
29+
30+
def has_delete_permission(self, request, obj=None): # pragma: nocover
31+
return False
32+
33+
def has_add_permission(self, request, obj=None): # pragma: nocover
34+
return False
35+
36+
1737
@admin.register(LtiConfiguration)
1838
class LtiConfigurationAdmin(admin.ModelAdmin):
1939
"""
@@ -24,6 +44,20 @@ class LtiConfigurationAdmin(admin.ModelAdmin):
2444
readonly_fields = ('location', 'config_id')
2545

2646

47+
@admin.register(Lti1p3Passport)
48+
class Lti1p3PassportAdmin(admin.ModelAdmin):
49+
"""
50+
Admin view for Lti1p3Passport models.
51+
"""
52+
list_display = (
53+
'name',
54+
'context_key',
55+
'passport_id',
56+
'lti_1p3_client_id',
57+
)
58+
inlines = [LtiConfigurationInline]
59+
60+
2761
@admin.register(CourseAllowPIISharingInLTIFlag)
2862
class CourseAllowPIISharingInLTIFlagAdmin(KeyedConfigurationModelAdmin):
2963
"""

lti_consumer/api.py

Lines changed: 97 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,44 +6,117 @@
66
"""
77

88
import json
9+
import logging
910

1011
from opaque_keys.edx.keys import CourseKey
1112

1213
from lti_consumer.lti_1p3.constants import LTI_1P3_ROLE_MAP
13-
from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem
14+
15+
from .filters import get_external_config_from_filter
16+
from .models import CourseAllowPIISharingInLTIFlag, Lti1p3Passport, LtiConfiguration, LtiDlContentItem
1417
from .utils import (
1518
get_cache_key,
1619
get_data_from_cache,
17-
get_lti_1p3_context_types_claim,
18-
get_lti_deeplinking_content_url,
20+
get_lms_lti_access_token_link,
1921
get_lms_lti_keyset_link,
2022
get_lms_lti_launch_link,
21-
get_lms_lti_access_token_link,
23+
get_lti_1p3_context_types_claim,
24+
get_lti_deeplinking_content_url,
2225
)
23-
from .filters import get_external_config_from_filter
26+
27+
log = logging.getLogger(__name__)
2428

2529

26-
def _get_or_create_local_lti_config(lti_version, block_location,
27-
config_store=LtiConfiguration.CONFIG_ON_XBLOCK, external_id=None):
30+
def _ensure_lti_passport(block, lti_config):
2831
"""
29-
Retrieve the LtiConfiguration for the block described by block_location, if one exists. If one does not exist,
30-
create an LtiConfiguration with the LtiConfiguration.CONFIG_ON_XBLOCK config_store.
32+
Keep block-backed LTI passport aligned with current block key fields.
33+
Function updates passport in place when safe, and splits to new passport
34+
when current passport is shared and active key value changed.
35+
36+
Flow
37+
38+
passport missing or non-CONFIG_ON_XBLOCK
39+
-> return current passport
40+
41+
passport unshared or tool fields blank
42+
-> update in place from block if changed
43+
-> return passport
44+
45+
passport shared
46+
-> active tool key mode matches block
47+
-> return passport
48+
-> active key mode differs
49+
-> create new passport
50+
-> save new passport_id on block
51+
-> return new passport
52+
"""
53+
passport = lti_config.lti_1p3_passport
54+
if not passport or lti_config.config_store != LtiConfiguration.CONFIG_ON_XBLOCK:
55+
return passport
56+
57+
block_public_key = str(block.lti_1p3_tool_public_key)
58+
block_keyset_url = str(block.lti_1p3_tool_keyset_url)
59+
60+
# Update in place when passport not shared, or when key fields still empty.
61+
if passport.lticonfiguration_set.count() == 1 or (
62+
not passport.lti_1p3_tool_public_key and not passport.lti_1p3_tool_keyset_url
63+
):
64+
if passport.lti_1p3_tool_public_key != block_public_key or passport.lti_1p3_tool_keyset_url != block_keyset_url:
65+
passport.lti_1p3_tool_public_key = block_public_key
66+
passport.lti_1p3_tool_keyset_url = block_keyset_url
67+
passport.save()
68+
log.info("Updated LTI passport for %s", block.scope_ids.usage_id)
69+
return passport
70+
71+
# For shared passport, check only active key mode before splitting passport.
72+
key_mismatch = (
73+
block.lti_1p3_tool_key_mode == 'public_key' and passport.lti_1p3_tool_public_key != block_public_key
74+
) or (
75+
block.lti_1p3_tool_key_mode == 'keyset_url' and passport.lti_1p3_tool_keyset_url != block_keyset_url
76+
)
77+
78+
if key_mismatch:
79+
from lti_consumer.plugin.compat import save_xblock # pylint: disable=import-outside-toplevel
80+
passport = Lti1p3Passport.objects.create(
81+
lti_1p3_tool_public_key=block_public_key,
82+
lti_1p3_tool_keyset_url=block_keyset_url,
83+
name=f"Passport of {block.display_name}",
84+
context_key=block.context_id,
85+
)
86+
# Persist new passport link on block so future loads use split passport.
87+
block.lti_1p3_passport_id = str(passport.passport_id)
88+
save_xblock(block)
89+
log.info("Created new LTI passport for %s", block.scope_ids.usage_id)
90+
91+
return passport
3192

32-
Treat the lti_version argument as the source of truth for LtiConfiguration.version and override the
33-
LtiConfiguration.version with lti_version. This allows, for example, for
34-
the XBlock to be the source of truth for the LTI version, which is a user-centric perspective we've adopted.
35-
This allows XBlock users to update the LTI version without needing to update the database.
93+
94+
def _get_or_create_local_lti_config(lti_version, block, config_store=LtiConfiguration.CONFIG_ON_XBLOCK):
95+
"""
96+
Retrieve or create an LtiConfiguration for the block.
97+
98+
The lti_version parameter is treated as the source of truth, overriding
99+
any stored version to allow XBlocks to control LTI version without DB updates.
36100
"""
37-
# The create operation is only performed when there is no existing configuration for the block
38-
lti_config, _ = LtiConfiguration.objects.get_or_create(location=block_location)
101+
lti_config, _ = LtiConfiguration.objects.get_or_create(location=block.scope_ids.usage_id)
39102

40-
lti_config.config_store = config_store
41-
lti_config.external_id = external_id
103+
# Ensure passport is synced with block
104+
passport = _ensure_lti_passport(block, lti_config)
42105

43-
if lti_config.version != lti_version:
44-
lti_config.version = lti_version
106+
# Batch updates
107+
updates = {
108+
'config_store': config_store,
109+
'external_id': block.external_config,
110+
'version': lti_version,
111+
}
112+
if passport:
113+
updates['lti_1p3_passport'] = passport
45114

46-
lti_config.save()
115+
# Only save if changed
116+
if any(getattr(lti_config, key) != value for key, value in updates.items()):
117+
for key, value in updates.items():
118+
setattr(lti_config, key, value)
119+
lti_config.save()
47120

48121
return lti_config
49122

@@ -65,7 +138,7 @@ def _get_lti_config_for_block(block):
65138
if block.config_type == 'database':
66139
lti_config = _get_or_create_local_lti_config(
67140
block.lti_version,
68-
block.scope_ids.usage_id,
141+
block,
69142
LtiConfiguration.CONFIG_ON_DB,
70143
)
71144
elif block.config_type == 'external':
@@ -75,14 +148,13 @@ def _get_lti_config_for_block(block):
75148
)
76149
lti_config = _get_or_create_local_lti_config(
77150
config.get("version"),
78-
block.scope_ids.usage_id,
151+
block,
79152
LtiConfiguration.CONFIG_EXTERNAL,
80-
external_id=block.external_config,
81153
)
82154
else:
83155
lti_config = _get_or_create_local_lti_config(
84156
block.lti_version,
85-
block.scope_ids.usage_id,
157+
block,
86158
LtiConfiguration.CONFIG_ON_XBLOCK,
87159
)
88160
return lti_config
@@ -140,7 +212,7 @@ def get_lti_1p3_launch_info(
140212
if dl_content_items.exists():
141213
deep_linking_content_items = [item.attributes for item in dl_content_items]
142214

143-
config_id = lti_config.config_id
215+
config_id = lti_config.passport_id
144216
client_id = lti_config.lti_1p3_client_id
145217
deployment_id = "1"
146218

lti_consumer/lti_1p3/key_handlers.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ def validate_and_decode(self, token):
112112
message = jwt.decode(
113113
token,
114114
key,
115-
algorithms=['RS256', 'RS512',],
115+
algorithms=['RS256', 'RS512'],
116116
options={
117117
'verify_signature': True,
118118
'verify_aud': False

lti_consumer/lti_xblock.py

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,11 @@
6060
from django.conf import settings
6161
from django.utils import timezone
6262
from web_fragments.fragment import Fragment
63-
6463
from webob import Response
6564
from xblock.core import List, Scope, String, XBlock
6665
from xblock.fields import Boolean, Float, Integer
6766
from xblock.validation import ValidationMessage
67+
6868
try:
6969
from xblock.utils.resources import ResourceLoader
7070
from xblock.utils.studio_editable import StudioEditableXBlockMixin
@@ -74,19 +74,19 @@
7474

7575
from .data import Lti1p3LaunchData
7676
from .exceptions import LtiError
77-
from .lti_1p1.consumer import LtiConsumer1p1, parse_result_json, LTI_PARAMETERS
77+
from .lti_1p1.consumer import LTI_PARAMETERS, LtiConsumer1p1, parse_result_json
7878
from .lti_1p1.oauth import log_authorization_header
7979
from .outcomes import OutcomeService
8080
from .plugin import compat
8181
from .track import track_event
8282
from .utils import (
83+
EXTERNAL_ID_REGEX,
8384
_,
84-
resolve_custom_parameter_template,
85-
external_config_filter_enabled,
86-
external_user_id_1p1_launches_enabled,
8785
database_config_enabled,
88-
EXTERNAL_ID_REGEX,
86+
external_config_filter_enabled,
8987
external_multiple_launch_urls_enabled,
88+
external_user_id_1p1_launches_enabled,
89+
resolve_custom_parameter_template,
9090
)
9191

9292
log = logging.getLogger(__name__)
@@ -311,6 +311,13 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
311311
)
312312

313313
# LTI 1.3 fields
314+
lti_1p3_passport_id = String(
315+
display_name=_("Lti 1.3 passport ID that points to Lti1p3Passport table"),
316+
scope=Scope.settings,
317+
default="",
318+
help=_("Passport ID for a reusable keys.")
319+
)
320+
314321
lti_1p3_launch_url = String(
315322
display_name=_("Tool Launch URL"),
316323
default='',
@@ -366,6 +373,9 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
366373
" requests received have the signature from the tool."
367374
"<br /><b>This is not required when doing LTI 1.3 Launches"
368375
" without LTI Advantage nor Basic Outcomes requests.</b>"
376+
"<br /><br /><b>Changing the public key or keyset URL will cause the client ID, block keyset URL "
377+
"and access token URL to be regenerated if they are shared between blocks. "
378+
"Please check and update them in the LTI tool settings if necessary.</b>"
369379
),
370380
)
371381
lti_1p3_tool_public_key = String(
@@ -380,6 +390,9 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
380390
"from the tool."
381391
"<br /><b>This is not required when doing LTI 1.3 Launches without LTI Advantage nor "
382392
"Basic Outcomes requests.</b>"
393+
"<br /><br /><b>Changing the public key or keyset URL will cause the client ID, block keyset URL "
394+
"and access token URL to be regenerated if they are shared between blocks. "
395+
"Please check and update them in the LTI tool settings if necessary.</b>"
383396
),
384397
)
385398

@@ -991,7 +1004,7 @@ def resource_link_id(self):
9911004
i4x-2-3-lti-31de800015cf4afb973356dbe81496df this part of resource_link_id:
9921005
makes resource_link_id to be unique among courses inside same system.
9931006
"""
994-
return str(urllib.parse.quote(f"{settings.LMS_BASE}-{self.scope_ids.usage_id.html_id()}"))
1007+
return str(self.scope_ids.usage_id)
9951008

9961009
@property
9971010
def lis_result_sourcedid(self):
@@ -1668,7 +1681,7 @@ def get_lti_1p3_launch_data(self):
16681681
user_id=self.lms_user_id,
16691682
user_role=self.role,
16701683
config_id=config_id,
1671-
resource_link_id=str(location),
1684+
resource_link_id=self.resource_link_id,
16721685
external_user_id=self.external_user_id,
16731686
preferred_username=username,
16741687
name=full_name,
@@ -1833,3 +1846,26 @@ def index_dictionary(self):
18331846
xblock_body["content_type"] = "LTI Consumer"
18341847

18351848
return xblock_body
1849+
1850+
def add_xml_to_node(self, node):
1851+
"""
1852+
The lti_1p3_passport_id XBlock field may be empty on blocks that existed before
1853+
the Lti1p3Passport model was introduced (migration 0021). Rather than backfilling
1854+
the field in the migration (which requires the XBlock runtime and can fail silently),
1855+
we read the authoritative passport_id from the DB at export time. This ensures that
1856+
when a block is duplicated or exported/imported, the receiving block's
1857+
lti_1p3_passport_id field is populated and can be used to find the shared passport
1858+
instead of creating new credentials.
1859+
"""
1860+
super().add_xml_to_node(node)
1861+
1862+
try:
1863+
from .models import LtiConfiguration # pylint: disable=import-outside-toplevel
1864+
1865+
configuration = LtiConfiguration.objects.select_related("lti_1p3_passport").get(
1866+
location=self.scope_ids.usage_id,
1867+
)
1868+
if configuration.lti_1p3_passport:
1869+
node.set("lti_1p3_passport_id", str(configuration.lti_1p3_passport.passport_id))
1870+
except LtiConfiguration.DoesNotExist:
1871+
pass

0 commit comments

Comments
 (0)