Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: LTI 1.3 reusable configuration #2

Merged
merged 4 commits into from Oct 24, 2023
Merged

feat: LTI 1.3 reusable configuration #2

merged 4 commits into from Oct 24, 2023

Conversation

kuipumu
Copy link
Contributor

@kuipumu kuipumu commented Jul 10, 2023 •
edited

Description

This PR adds compatibility for LTI 1.3 external configuration to the openedx-ltistore. This change adds missing fields required for LTI 1.3 external reusable configurations to work.

Type of Change

  • Add LTI 1.3 fields to ExternalLtiConfiguration model.
  • Include unit tests for all added and modified code.

Testing

  1. Install xblock-lti-consumer (Use the related PR: feat: LTI 1.3 reusable configuration openedx/xblock-lti-consumer#390)
  2. Install and configure openedx-ltistore (follow the setup steps from: [BB-5559] Adds support for external LTI configurations using openedx-filters openedx/xblock-lti-consumer#239 and use the PR related to this ticket)
  3. Create an IMS reference tool: https://lti-ri.imsglobal.org/lti/tools
  4. Go to http://localhost:18000/admin/lti_store/externallticonfiguration/ and create a new external LTI 1.3 configuration.
  5. Fill the "LTI 1.3 Private Key", "LTI 1.3 Client ID" and "LTI 1.3 Tool Keyset URL" or "LTI 1.3 Tool Public Key" field.
  6. Create a course and add a LTI 1.3 consumer XBlock.
  7. Set the consumer XBlock to user an external configuration and set the reusable configuration ID (follow the steps on how to get the filter key from: [BB-5559] Adds support for external LTI configurations using openedx-filters openedx/xblock-lti-consumer#239)
  8. Copy the keyset and access token URL from the XBlock preview back to the IMS reference tool previously created.
  9. Go to the course a launch the XBlock.
  10. The LTI 1.3 launch should be valid.

@kuipumu kuipumu mentioned this pull request Jul 10, 2023
Merged
16 tasks
Agrendalath
Agrendalath requested changes Aug 22, 2023
View reviewed changes
lti_store/views.py Outdated Show resolved Hide resolved
@kuipumu @Squirrel18
@alexjmpb @anfbermudezme @sergivalero20
feat: LTI 1.3 reusable configuration
9d1b22b 
Co-authored-by: Squirrel18 <daniel.quiroga@edunext.co>
Co-authored-by: alexjmpb <alexander.mendoza@edunext.co>
Co-authored-by: anfbermudezme <andres.bermudez@edunext.co>
Co-authored-by: sergiovalero20 <sergio.valero@edunext.co>
@kuipumu kuipumu force-pushed the kuipumu/lti_1p3_reusable_configuration branch from dc31a69 to 9d1b22b Compare September 6, 2023 08:58
Agrendalath
Agrendalath reviewed Sep 10, 2023
View reviewed changes
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, this looks good. Only a few small questions.

lti_store/admin.py Outdated Show resolved Hide resolved
lti_store/models.py Outdated Show resolved Hide resolved
lti_store/models.py
Comment on lines +202 to +208
# Regenerate public JWK.
public_keys = jwk.KEYS()
public_keys.append(RSAKey(
kid=self.lti_1p3_private_key_id,
key=RSA.import_key(self.lti_1p3_private_key),
))
self.lti_1p3_public_jwk = json.loads(public_keys.dump_jwks())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to regenerate JWK on every save? The LtiConfiguration model is doing this only when the value is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only executed when the external configuration is saved, I don't think there is much of a performance hit compared to verifying if any key is missing every time they are being accessed. The method used on the LtiConfiguration has a disadvantage, if the LtiConfiguration private key is updated, the public JWK will need to be deleted so it's regenerated. I could add more logic to make sure the public JWK is only re-generated if the private key or private key ID is changed, but I didn't find a simple way to do this on the model save method.

@kuipumu
Copy link
Contributor Author

kuipumu commented Sep 27, 2023

Hi @Agrendalath, sorry for the delay. I just replied your comments in this PR and in openedx/xblock-lti-consumer#390

Agrendalath
Agrendalath approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, please update the version in lti_store/__init__.py. Let's call it 1.0.0 to indicate that we don't want to introduce breaking changes here.

👍

  • I tested this: LTI 1.3 configurations can be reused
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kuipumu kuipumu force-pushed the kuipumu/lti_1p3_reusable_configuration branch from a2a2b04 to f79ada7 Compare October 23, 2023 23:20
@kuipumu
Copy link
Contributor Author

kuipumu commented Oct 23, 2023

@Agrendalath Just applied your suggestions and update the version. I didn't modified the CHANGELOG.md file since I'm not sure what standard should follow this repository.

@Agrendalath Agrendalath merged commit a0d6c36 into open-craft:main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Agrendalath Agrendalath Agrendalath approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kuipumu @Agrendalath