jupyterhub / ltiauthenticator

A JupyterHub authenticator for LTI
https://ltiauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
67 stars 54 forks source link

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

Closed martinclaus closed 1 year ago

martinclaus commented 1 year ago

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

See #151

martinclaus commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@consideRatio Is this ready to be merged?