feat: LTI 1.3 reusable configuration #390
Conversation
fix: Improve external config fields validation fix: Improve hide/show fields toggle Co-authored-by: Squirrel18 <[email protected]> Co-authored-by: alexjmpb <[email protected]>
|
Thanks for the pull request, @kuipumu! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
mphilbrick211
added
the
needs test run
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #390 +/- ##
==========================================
+ Coverage 97.94% 97.95% +0.01%
==========================================
Files 77 77
Lines 6555 6686 +131
==========================================
+ Hits 6420 6549 +129
- Misses 135 137 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
mphilbrick211
removed
the
needs test run
|
Hi @kuipumu! Is this all set for review/merge? |
|
@kuipumu thank you! I'll have more opportunity to dig into these PR's more deeply next week. That said this looks really promising. I could have sworn there was a catch to adopting this implementation for 1.3 but this seems pretty straightforward. |
|
@zacharis278 I only know that the OpenCraft team had a plan to decouple the LTI configuration model, that will meant this implementation will need to change to adjust to those changes, right now this implementation depends on having the keyset and access token URLs being on the external app. @mphilbrick211 Yes, this PR is ready for review/merge. |
|
@zacharis278 - are you able to merge this if it's good-to-go? I believe this repo is owned by the Cosmonauts, but I'm not sure who (aside from Chris H.) is on the team. |
|
@mphilbrick211 yes I can merge once this is good to go. Just to set expectations, we've got some other priorities to juggle and I can't get to these right away. I'd also like to solicit some input from OpenCraft as well so that could take additional time before this is approved and merged. |
|
Thanks, @zacharis278! Putting updates like that in here is super helpful. I might check in again, but if you're still not ready, that's OK. Having an ETA on this, or providing another update would also help if possible. |
mphilbrick211
added
the
waiting for eng review
|
This all looks good to me and would fit with the long term plan to decouple configuration/placement from what I can tell. Maybe a little more refactoring with merging this first to move these fields back out of configuration and into placement but I don't think that should hold us up. Brief question on the configurable deployment id. Based on our current model there's no way to have multiple deployment id's for a configuration. How would this get used? Is this just forward thinking for future development of multi-tenancy? I'm looking through the other PR's and plan to manually test each of them. Should get through that by tomorrow |
|
@tecoholic are you the right person to ping on the OpenCraft side to take a look at this? Would like to get your thoughts before merging. |
mphilbrick211
removed
the
waiting for eng review
|
@zacharis278 This was included to make external configs be multi-tenancy compatible. I included the possibility to use a deployment ID set on the external config, I added a field |
|
Think I got it. My understanding of how this would work is right now, setting deployment id doesn't really do anything other than keeping us single tenant with a different deployment id, right? Since deployment id is on the configuration it's always a 1:1 relationship. That is to say one configuration can't be shared across multiple deployments. I'm wondering if this configurable deployment id is adding any value. Please correct me if I'm missing a good use case for it. My concern is that this would be something we need to refactor when we do build the modeling for multi-tenancy. If existing configurations are already using custom deployment ids that could become messy to migrate. |
|
@zacharis278 I think there is some confusion regarding the deployment_id, there are a couple of diagrams that more clearly represent the role of the deployment_id value on the LTI tool deployment: https://www.imsglobal.org/spec/lti/v1p3#multi-tenant-tool-registered-once-deployed-multiple-times. The role of the deployment ID is mainly on the tool deployment, the idea is that you can share the same client ID, public key on a tool while creating a distinction between different deployments with the deployment ID, this means that you would just need to deploy the tool once and reuse this same security contract, correct me if I'm wrong in any of this. I think multi-tenancy is already working by just enabling the use of a different deployment ID. There is certain details, for example with AGS, each score and lineitem should be isolated per deployment, this is addressed in here, I don't know of any other resource that's shared between tool and platform that needs to be isolated per deployment, if any that should be addressed, but I didn't notice anything else on that regards that needs to be addressed. |
|
@zacharis278 Hi, I think @Agrendalath is going to review a couple of PRs related to LTI. I think he might be the better person to handle this here. |
|
Sorry for any confusion, I understand how it works according to the spec but my questions are specifically to the fact that Open edX does not have a way to configure multiple deployments to a single configuration yet. If we do build support for this it would likely be some model separate from the tool configuration itself (probably an 'account' concept). I want to make sure we're designing in a way that keeps that path open. Our upcoming goals around building a marketplace will eventually need this. I'd just like to understand how deployment id on configuration as it exists here would be used right now. If there's a good use case, than yeah we should add it. I wouldn't want to get too hung up on future designs if it would block adding something useful right now. |
|
@zacharis278 OK I wasn't aware about any of this, right know the use case for this is just to be able to set a deployment ID different from the default "1", so we can for example have a tool that will accept multiple XBlocks using the same client_id but with a different deployment ID so we can differentiate between different deployments of this configuration. |
There was a problem hiding this comment.
@kuipumu, thank you for this contribution, and sorry for the delay in the review. I left some comments here and in the openedx-ltistore PR.
lti_consumer/models.py
Outdated
| # Use the xblock values if lti_1p3_oidc_url or lti_1p3_launch_url | ||
| # is not set on the external configuration. | ||
| lti_oidc_url=block.lti_1p3_oidc_url or external.get('lti_1p3_oidc_url'), | ||
| lti_launch_url=block.lti_1p3_launch_url or external.get('lti_1p3_launch_url'), |
There was a problem hiding this comment.
Why do we introduce the XBlock configuration as a fallback? What about the value stored in the DB (self.lti_1p3_oidc_url)?
Having a fallback is helpful, but this behavior should be clearly documented.
There was a problem hiding this comment.
We introduce it so course authors can override the OIDC URL, launch URL or deep linking launch URL, without the need to create a new configuration. We could add also the DB values as a fallback, I didn't wanted to introduce to much options on this fallback but also having the fallback on the DB might be useful. What do you suggest this be documented on?, I just thought about adding a code comment to describe this behavior but I agree it might be better to have it documented.
There was a problem hiding this comment.
@kuipumu , ah, right, we use them as overrides, not fallbacks.
Should we also override other values (like client_id, rsa_key) through the LtiConfiguration? Quick context: I'm talking only about the LtiConfiguration or ExternalConfiguration condition since these values cannot be easily modified (or are not stored) within the XBlock fields.
I believe that the most discoverable solution would be adding this information directly to the help text of each XBlock/DB field that can be overridden this way. Something like "If the configuration is retrieved from an external store, this value overrides the one specified in the external configuration.", but with an indication that this is not applicable when you use the CONFIG_ON_DB option.
@zacharis278, what do you think?
There was a problem hiding this comment.
I think an issue we run into with the override functionality is right now the configuration options are exclusive. You pick either xblock, database, or external. If you choose external it's ambiguous what the next most important configuration location would be. I'd agree making this not applicable to CONFIG_ON_DB is probably the simplest short term solution around that. That said, this would now differ from the 1.1 implementation. If we're going to add an override feature its behavior should be consistent across LTI versions.
I'm leaning towards suggesting we break override behavior out into it's own PR and instead focus on parity with the existing 1.1 solution in here. Taking time to discuss/consider the full list of fields that are potentially overridable at the block level and how we visually communicate to course teams how it would work might hold up the core behavior of this PR.
There was a problem hiding this comment.
@zacharis278 I agree that we should make this PR only about making LTI 1.3 external configuration 1:1 with the LTI 1.1 implementation and move other specific features (multi-tenancy, field override) into separate PRs, I will create these PRs has soon as we merge this changes. I already removed anything related to field overrides and multi-tenancy from the PR.
lti_consumer/models.py
Outdated
| # OIDC URL, Client ID and Deployment ID | ||
| # This allows us to isolate the LineItem per tool deployment | ||
| oidc_url = models.CharField( | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, | ||
| ) | ||
| client_id = models.CharField( | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, | ||
| ) | ||
| deployment_id = models.CharField( | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Is this change backward-compatible? Shouldn't we fill these fields in the migration?
Also, why are we adding these fields to this model? A single LtiConfiguration entity can have only one set of oidc_url, client_id and deployment_id values. The LtiAgsLineItem entity can be identified by the resource_link_id. If I understand this correctly, as long as the deployment ID is defined in the database configs (instead of being tied to, e.g., an organization or a Django Site), this relation should not change.
Using the single-tenant deployments was a deliberate architectural decision, so if we want to change it, it would be reasonable to write a new ADR for this. We should also prepare the documentation that explains how to configure it. We shouldn't add undocumented features.
It would be better to split this functionality into a separate PR.
There was a problem hiding this comment.
A single LtiConfiguration entity can only have one set of oidc_url, client_id, and deployment_id values set on the DB and XBlock, but once we use external configurations, the oidc_url, client_id and deployment_id values on the external configuration could change, this will mean that the gradebook won't change even if the oidc_url, client_id and deployment_id values changed for the LtiConfiguration. Since each lineitem should only expose gradebook information directly tied to the tool deployment, I modified the LtiAgsLineItem model to include the oidc_url, client_id and deployment_id so we are querying the LtiAgsLineItem on LtiAgsLineItemViewset not only by the lti_configuration but also by the values currently being used by the LtiConfiguration. I might be wrong about this, so please correct me if I'm wrong.
Also, you are correct, this fields should be filled in the migration, otherwise, this change wouldn't be backward compatible if applied.
We noticed the possibility of using deployment IDs while using an external configuration, I can separate anything related to implementing the use of multi-tenancy with external configurations on a separate PR to avoid increasing the complexity of the initial implementation of external configurations for LTI 1.3. What do you think if I add an ADR on how to set external configurations for LTI 1.3 and another ADR about external configurations multi-tenancy on this other PR I will open?.
There was a problem hiding this comment.
@kuipumu, yes, please move the multi-tenancy-related things to a separate PR. Once we merge the reusable LTI 1.3 configs, catching potential regressions will be much easier.
What do you think if I add an ADR on how to set external configurations for LTI 1.3 and another ADR about external configurations multi-tenancy on this other PR I will open?.
Sounds reasonable to me. Do we have a rough timeline for this separate PR? It would be good to add the LTI 1.3 external configurations ADR before the Quince release.
There was a problem hiding this comment.
@Agrendalath I removed anything related to multi-tenancy from this PR, I don't have a rough timeline for this PR, I would think that has soon has we end merging this feature I can create a PR for anything related to multi-tenancy and external configurations.
I will work on an ADR ASAP to include in this PR related to LTI 1.3 external configurations, the Quince release is expected to be released when?
There was a problem hiding this comment.
@kuipumu, Quince will be released in December.
|
@Agrendalath Sorry for my delay on the response, I responded a few of your comments here and in in the openedx-ltistore PR. I applied a few of your suggestions to the code, Thanks!. |
fix: Remove multi-tenancy related changes fix: Remove external configuration value overrides
|
@Agrendalath I responded a few of your comments here and in open-craft/openedx-ltistore#2 (comment). According to what you previously commented on the openedx-ltistore PR, I reviewed again the possibility of keep using the keyset/token views from this app instead of creating a new views on the openedx-ltistore app, I was able to do so, I removed the views from the openedx-ltistore PR and modified the keyset/token views on xblock-lti-consumer. |
|
Thanks, @kuipumu! I will review and test both PRs tomorrow. |
lti_consumer/models.py
Outdated
| # OIDC URL, Client ID and Deployment ID | ||
| # This allows us to isolate the LineItem per tool deployment | ||
| oidc_url = models.CharField( | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, | ||
| ) | ||
| client_id = models.CharField( | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, | ||
| ) | ||
| deployment_id = models.CharField( | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
@kuipumu, Quince will be released in December.
|
@kuipumu, please see this comment first, as it can affect other comments. |
|
Hi @Agrendalath , I applied all your suggestions except for this previous comment which I just left a reply, I also responded and applied your suggestions and responded your comments on open-craft/openedx-ltistore#2 |
|
@kuipumu, thank you; I will review this and open-craft/openedx-ltistore#2 tomorrow. |
There was a problem hiding this comment.
@kuipumu, I'm having some trouble with testing it. I explained one issue in the comment. The other one is that when I set the configuration to CONFIG_EXTERNAL in the LtiConfiguration DB entry, it gets reverted back to CONFIG_ON_DB every time I load the XBlock.
I tried setting the external ID to lti_store:1 and lti_store:test-configuration (slug), and some other permutations, but none seem to be working for me.
I already enabled the following flags: lti_consumer.enable_database_config, lti_consumer.enable_external_config_filter. Did I miss anything in my setup?
| # TODO: This needs to change when the LTI 1.3 support is added to the external config_type in the future. | ||
| if consumer and self.lti_version == "lti_1p3" and self.config_type == "database": | ||
| # Get LTI launch URL from consumer if using database or external configuration type. | ||
| if consumer and self.lti_version == 'lti_1p3' and self.config_type in ('database', 'external'): |
There was a problem hiding this comment.
Something odd happens when I try to use the external configuration directly from the XBlock. When I set the Configuration Type to Reusable Configuration and provide a valid LTI Reusable Configuration ID, the XBlock break with Error: (1048, "Column 'version' cannot be null"). The relevant part of the traceback is:
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1177, in author_view
return self.student_view(context)
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1227, in student_view
context.update(self._get_context_for_template())
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1730, in _get_context_for_template
lti_consumer = self._get_lti_consumer()
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1128, in _get_lti_consumer
return get_lti_consumer(config_id_for_block(self))
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/api.py", line 96, in config_id_for_block
config = _get_lti_config_for_block(block)
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/api.py", line 76, in _get_lti_config_for_block
lti_config = _get_or_create_local_lti_config(
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/api.py", line 46, in _get_or_create_local_lti_config
lti_config.save()
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/models.py", line 313, in save
super().save(*args, **kwargs)There was a problem hiding this comment.
This is because the external ID set on the XBlock is not found, this is either because the reusable configuration feature is not properly setup or the external configuration doesn't exist. If you check this specific line you can notice that if no config is found the "version" value will return None, once this value is sent to the _get_or_create_local_lti_config function, when it tries to save the new values to the LtiConfiguration it fails since "version" value cannot be null. This could be fixed by adding an extra validation to any required field from the external config on _get_lti_config_for_block but I think it's out of the scope of this PR, an issue about this should be created.
fix: Improve LtiConfiguration clean method test
|
@Agrendalath in response to this comment. This issue is happening both for configurations using LTI 1.1 and LTI 1.3, It seems that once you load the studio view for the XBlock, the LTI configuration reverts back to the values set on the XBlock, I'm not sure how these values are sync between the XBlock and the model, will need to check further to address this issue, I would think this should be addressed in another PR since it's not directly related to the use of external configurations with LTI 1.3. I followed the steps from this PR to setup the external reusable configuration: #239 Did you properly setup the |
|
@Agrendalath I just responded to your previous comments and applied your suggestions, I also included an ADR on how to setup this feature for both LTI 1.1 and LTI 1.3. |
|
@Agrendalath I just applied your suggestions related to the setup instructions and fixed the sync_configurations method, I also extended the README.md of the openedx-ltistore to include the instructions on how to use the "Filter Key" on this PR open-craft/openedx-ltistore#2 |
There was a problem hiding this comment.
@kuipumu, thank you so much for your persistence!
Before we merge this, please update the version to 9.7.0 and add an entry to the CHANGELOG.
👍
- I tested this: LTI 1.3 configurations can be reused
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation
|
@Agrendalath Just applied your latest suggestions, I also update the package version and added a new entry to the CHANGELOG |
|
@kuipumu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR adds compatibility for LTI 1.3 external configurations to the LTI consumer XBlock. This change allow the XBlock to use values from the external LTI configuration when enabled and setup. This PR also includes a fix that adds validation to the external_config field on the LTI consumer XBlock and also improves the behavior of the JS that handles the visibility of configuration fields on the LTI consumer XBlock.
Type of Change
urlpatternsvariable onplugins/urls.pymodule.get_lti_1p3_launch_infofunction onapi.pymodule.LtiConsumerXBlockvalidate_field_datamethod.LtiConsumerXBlock_get_lti_block_launch_handlermethod.LtiConsumerXBlock_get_lti_1p3_launch_urlmethod.LtiConfigurationcleanmethod.LtiConfigurationget_lti_advantage_ags_modemethod.LtiConfigurationget_lti_advantage_deep_linking_enabledmethod.LtiConfigurationget_lti_advantage_deep_linking_launch_urlmethod.LtiConfigurationget_lti_advantage_nrps_enabledmethod.LtiConfigurationget_lti_1p3_redirect_urismethod.LtiConfiguration_get_lti_1p3_consumermethod.xblock_studio_view.js.public_keyset_endpointfunction onplugin/views.pymodule.access_token_endpointfunction onplugin/views.pymodule.Testing