Skip to content

feat(lti13): Allow multiple LTI 1.3 client ids - #152

Merged
consideRatio merged 10 commits into
mainfrom
allow-multiple-lti-client-ids
May 27, 2023
Merged

feat(lti13): Allow multiple LTI 1.3 client ids#152
consideRatio merged 10 commits into
mainfrom
allow-multiple-lti-client-ids

Conversation

@martinclaus

@martinclaus martinclaus commented Apr 26, 2023

Copy link
Copy Markdown
Collaborator

Note that I chose trailtlets.List as container type (not traitlets.Set) to mitigate yaml configuration limitations.

See #151

@martinclaus martinclaus linked an issue Apr 26, 2023 that may be closed by this pull request
@martinclaus

Copy link
Copy Markdown
Collaborator Author

@jeflem: I have implemented your proposed changes with an additional tweak in the ID token validation (azp claim validation). Do you have capacity to test this branch in your setup? I am guessing that you are not using a helm chart for configuration but the standard jupyterhub_config.py, correct?

I will test on a kubernetes deployment configured via yaml files.

@jeflem

jeflem commented Apr 26, 2023

Copy link
Copy Markdown
Contributor

Tested your code on a 'classical' JupyterHub install (with jupyterhub_config.py): works like a charm. LMS was Moodle 4. This feature makes integrating JupyterHub into an LMS much easier. Thank you so much @martinclaus!

@martinclaus

Copy link
Copy Markdown
Collaborator Author

Tested on zero2jupyterhub v1.2.0 with string value, single item list and list with multiple items. Everything works as expected.

Ready to merge IMO.

@consideRatio

Copy link
Copy Markdown
Member

@martinclaus there are limitatiosn in YAML, but the traitlet's Set type accepts being passed a List.

For example Authenicator.admin_users is a traitlets Set type, and we can still pass it Lists parsed from YAML like below:

hub:
  config:
    JupyterHub:
      admin_access: true
      admin_users:
        - jovyan1
        - jovyan2

@martinclaus

Copy link
Copy Markdown
Collaborator Author

there are limitatiosn in YAML, but the traitlet's Set type accepts being passed a List.

Thank you for the clarification. I then go for traitlets.Set as the container type since it feels more appropriate.

@martinclaus
martinclaus marked this pull request as ready for review May 8, 2023 06:18
@martinclaus

Copy link
Copy Markdown
Collaborator Author

@consideRatio Is this ready to be merged?

@consideRatio consideRatio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice this looks great to me!

Thanks for pinging me, keep doing that for any review effort you need to help you drive development @martinclaus!

❤️ 🎉 🌻 thank you for working on this project!!

@consideRatio
consideRatio merged commit 2eedece into main May 27, 2023
@consideRatio
consideRatio deleted the allow-multiple-lti-client-ids branch May 27, 2023 09:36
@martinclaus martinclaus changed the title (feat) (lti13): Allow multiple LTI 1.3 client ids feat(lti13): Allow multiple LTI 1.3 client ids Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow multiple LTI client IDs

3 participants