feat: LTI 1.3 Passport Refactor + Database Cleanup Support #627
feat: LTI 1.3 Passport Refactor + Database Cleanup Support #627feanil merged 50 commits intoopenedx:masterfrom
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
c391f5e to
45ce7ac
Compare
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
08ac0d2 to
7becfcb
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #627 +/- ##
==========================================
+ Coverage 97.59% 97.64% +0.04%
==========================================
Files 79 84 +5
Lines 6871 7594 +723
==========================================
+ Hits 6706 7415 +709
- Misses 165 179 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
1369128 to
b7c06db
Compare
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
@navinkarkera I like the approach of adding a new model for storing lti credentials independent of the blocks, this would obviate the need for external storage and make it easier to re-use storage. I've got a few questions specific to the implementation but I think this is the idea that we should try to land.
@ayub02 a question for you: Should import/export work for LTI blocks from one open edx instance to another? This has not worked before but this change will not really fix that either.
| f'Failed to parse main LTI configuration location: {self.location}', | ||
| ) | ||
|
|
||
| def create_lti_1p3_passport(self): |
There was a problem hiding this comment.
This is a get or create in practice right? So let's update the name.
| # 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): |
There was a problem hiding this comment.
What's the reason for this?
There was a problem hiding this comment.
Just to allow us to include some fields instead of excluding lot of fields incase we only need few of them.
| """ | ||
| Model to store LTI 1.3 keys. | ||
| """ | ||
| passport_id = models.UUIDField(unique=True, default=uuid.uuid4, editable=False) |
There was a problem hiding this comment.
Does it make sense to add a passport_name field now and let the users set it so that when we make this re-usable they will already have human readable names and we can show them to the user? Also currently these settings have no scope. Since we're newly introducing it, does it make sense to make it scoped to a context key of some sort to begin with?
There was a problem hiding this comment.
Does it make sense to add a passport_name field now
Yes, good idea!
Since we're newly introducing it, does it make sense to make it scoped to a context key of some sort to begin with?
Makes sense. Should we worry about them being used out of context for now? Like if you copy and paste in different courses, they will be using the same passport.
There was a problem hiding this comment.
About passport_name:
- Should we make this field unique in combination with the context_key?
- We'll still need passport_id as it needs to be unique across the table.
There was a problem hiding this comment.
I think let's not worry about the name uniqueness for now, think of it more as a display name. In the future, we may want to have some passports shared across an org or across a whole site, and so that course or library context wouldn't make sense then. I think when we display the key to the user, it could get confusing but only if the authors are creating multiple credentials with the same name. As a future option we will need to add a way to edit all the existing configs but something like that is already in the conversation for Willow so we can worry about it then.
There was a problem hiding this comment.
Got it.
How would the Author specify the name and context_key? Should we add them to the xblock settings editor?
For existing blocks, we could use xblock name as default value for passport name, something like: f"Passport for: {xblock_name}" and add course key as context_key.
There was a problem hiding this comment.
Should we also be dropping these fields from the xblock at the same time? We're not really using them for storage as much as to make it easier to use the old studio block rendering helper. Since we're redoing the frontend, do we need those fields to exist on the block? @rpenido perhaps you're the right person to answer that question?
There was a problem hiding this comment.
- We are moving them (including existing data) to passport model.
- AFAIK, the frontend doesn't really depend on the database models.
There was a problem hiding this comment.
ok, so maybe it's a follow up PR to drop the keys from the block, in-case there are issues and we need to rollback the migration or re-run it we can leave them in for now. Can you make a ticket in this repo to follow up on the removal once the code has been live for a release?
There was a problem hiding this comment.
They are not present in the xblock. They were only present in the LtiConfiguration table.
|
|
||
| @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.get_or_create_lti_1p3_passport() |
There was a problem hiding this comment.
When you call this function as a post-save function for LtiConfiguration for it also calls lti_configuration.save() conditionally. So you have a situation where we get a double save and a double call to the get_or_create function at the creation of each new LtiConfiguration instance. Take a look at the suggestion in the get_or_create function to see how we could avoid the double save firing.
| block.save() | ||
| compat.save_xblock(block) | ||
| self.lti_1p3_passport = passport | ||
| self.save() |
There was a problem hiding this comment.
| self.save() | |
| LtiConfiguration.objects.filter(pk=self.pk).update(lti_1p3_passport=passport) |
The self.lti_1p3_passport = passport keeps the in-memory instance in sync. The update() writes the FK directly to the DB without going through save(), so the signal doesn't re-fire. The sync_configurations() bypass is fine here since we're only updating this FK field. But we should add a comment explaining this here.
| passport.name = f"Passport of {block.display_name}" | ||
| passport.context_key = block.context_id |
There was a problem hiding this comment.
If the try/catch above could throw an exception in which case block might not be set. In that case we'll get further exceptions. Do we need a continue in the Exception clause so that we don't error out here?
There was a problem hiding this comment.
@feanil Nice catch! I was fighting with sandbox deployment, most probably this is the issue.
There was a problem hiding this comment.
This save() fires on every call to get_or_create_local_lti_config, which includes every LTI 1.3 author view render in Studio (via get_lti_1p3_launch_data → config_id_for_block) and every LTI launch. That means an author simply opening a block to check the client ID triggers a DB write even when nothing has changed.
The fix is to only save when something actually changed:
dirty = (
lti_config.config_store != config_store or
lti_config.external_id != block.external_config or
lti_config.version != lti_version or
lti_config.lti_1p3_passport != passport
)
if dirty:
lti_config.save()
The longer-term fix would be to move the reconciliation logic into an override of submit_studio_edits so it only fires when an author actually saves changes in Studio. That doesn't cover all cases though (imports, duplicates) so a dirty-check fallback would still be needed. Worth a follow-up ticket.
There was a problem hiding this comment.
Yes, even I am not happy about the DB changes on each render.
For now I have refactored the whole function with some help from AI. I still need to test it a bit more.
| instance.get_or_create_lti_1p3_passport() | ||
|
|
||
|
|
||
| @receiver(SignalHandler.pre_item_delete if SignalHandler else []) |
There was a problem hiding this comment.
I don't see a pre_item_delete signal in the modulestore SignalHandler. Is this defined elsewhere?
There was a problem hiding this comment.
It is part of this PR: openedx/openedx-platform#38192, I have added this in the description as a dependency.
| 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): |
There was a problem hiding this comment.
nit: Does this need to be a public function? Seems like it's only called from a different private function and should remain an internal function (prefix with an underscore?)
There was a problem hiding this comment.
Yes, at some point I used it outside but not needed anymore.
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
d10135c to
6661c60
Compare
|
@navinkarkera let me know when you want me to take another Pass at reviewing this. |
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
fe8f80a to
f809795
Compare
|
@feanil Yes, it is ready for another round. |
| return | ||
|
|
||
| src_lti_config = LtiConfiguration.objects.get(location=str(xblock_data.source_usage_key)) | ||
| copy = src_lti_config |
There was a problem hiding this comment.
Nit: The pk=None will make this into a new object but it's a bit of a trick. Add a comment to say we're using it to duplicate this object without having to enumerate all the fields here and making this function more brittle.
There's also a risk that if we introduce a new generated key in the future like config_id that this function would not update that correctly. This is hypothetical so no need to code to defensively around it unless you can think of an easy way to do so. Nothing obvious comes to mind for me.
There was a problem hiding this comment.
I refactored it to use model_to_dict and also handle possible errors. c2c7ec7
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
f809795 to
01fb5bf
Compare
@feanil Yes. |
Adds tests covering possible cases
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
1a87017 to
5fae419
Compare
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.
|
@navinkarkera I'm not actually sure that we should remove the save logic completely. Right now, the id does not get saved at all on the original block which seems like it wolud lead to future confusion. I think it's worth making sure that when we instantiate the LtiPassport, that it's What do you think? I think this is okay to land as is also if you disagree but given how hard it is to reason through this code right now, my inclination is to make it more robust and unsurprising where possible. |
|
@feanil I wanted to avoid calling save_xblock at that point as it is also called in LMS but I agree that it could be confusing in future. I'll revert the changes. |
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
bc92fca to
c0267e7
Compare
|
Right, it makes sense to not have that call in the LMS and I guess it's really hard right now to differentiate the call from the LMS vs an instantiation of the block in studio? The critical thing is to save the id to the block when we create the new instance of the block in studio. This happens on copy paste but not on other instantiations. |
navinkarkera
force-pushed
the
navin/fal-4318/split-config
branch
from
c0267e7 to
45f83c0
Compare
github-project-automation
Bot
moved this from Waiting on Author
to Done
in Contributions
|
Merged and is being released now: https://github.com/openedx/xblock-lti-consumer/releases/tag/v11.0.0 |
5 participants
Description
Split LTI 1.3 Configuration into Passport Model
Lti1p3Passportmodel to centralize LTI 1.3 keys and credentialslti_1p3_internal_private_key,lti_1p3_internal_private_key_id,lti_1p3_internal_public_jwk,lti_1p3_client_id,lti_1p3_tool_public_key, andlti_1p3_tool_keyset_urlfields fromLtiConfigurationtoLti1p3PassportForeignKeyrelationship fromLtiConfigurationtoLti1p3Passportclean()validation toLti1p3Passportto ensure at least one oflti_1p3_tool_public_keyorlti_1p3_tool_keyset_urlis setLtiConfiguration.clean()to check for passport presence instead of tool key fieldsget_or_create_local_lti_config()to handle passport creation and sync block/passport key configurationsLti1p3Passportmodelaccess_token_endpointandpublic_keyset_endpointto use passport IDLtiConfigurationtableresource_link_idgenerationSupport Database Cleanup for Deleted Blocks
pre_item_delete(block/children).Related
Test instructions
lti_1p3_tool_keyset_urlin one of the xblocks and save. This should create a new passport entry instead of modifying the original one to avoid changing other block.