Add LTI 1.1 config handler for the /lti11/config route by jgwerner · Pull Request #67 · jupyterhub/ltiauthenticator

jgwerner · 2021-08-29T22:23:33Z

Adds the LTI 1.1 configuration handler to render the authenticator's configuration in XML format
Updates README.md to included common settings section and adds LTI 1.1 configuration endpoint settings

Closes #49

Signed-off-by: Greg Werner <werner.greg@gmail.com>

consideRatio · 2021-08-29T22:32:54Z

@jgwerner from what I can tell given no actual experience with LTI, this LGTM.

Btw, I've not released 1.1.0 yet, I consider it briefly blocked by #65 or #66.

Signed-off-by: Greg Werner <werner.greg@gmail.com>

consideRatio

This looks good to me but there were some issues with the markdown in the README.md after merging some formatting PR I think =/

Let's stop using explicit numbers and use 1. at all time to avoid numbering issues and make sure all numbered bullet points have their content indented under it.

jgwerner · 2021-08-30T07:54:04Z

Hi, @consideRatio thanks for the detailed review comments regarding the docs. I found some issues when testing with our Canvas sandbox and am updating the handlers/tests so will make sure to update the docs as well with the next push.

consideRatio · 2021-08-30T08:01:10Z

@jgwerner absolutely!

Btw, do you think of this as something to release in 1.1.0 or later? Note I never cut that release when I ran into the outdated RELEASE.md docs.

Signed-off-by: Greg Werner <werner.greg@gmail.com>

jgwerner · 2021-08-30T21:26:43Z

@consideRatio this should be ready now. The docs are updated and completed e2e tests with Canvas and Moodle.

Btw, do you think of this as something to release in 1.1.0 or later? Note I never cut that release when I ran into the outdated RELEASE.md docs.

I'm not sure if you follow the Semantic Release convention or not (I think @yuvipanda once mentioned that you do not). If you do, then this PR adds a new feature so it should require a new minor release version. If not, your call!

Signed-off-by: Greg Werner <werner.greg@gmail.com>

consideRatio

I made some documentation suggestions, but otherwise this LGTM from what I can tell.

Thank you for your thorough work on this @jgwerner!! ❤️ 🎉!

consideRatio · 2021-09-01T02:15:50Z

+There are two configuration values to define with LTI 1.1:

-    Now we will use these strings to allow edX and JupyterHub to authenticate each other.
+- The **consumers** setting: the value for the LTI 1.1 configuration `consumer key` and `shared secret`.
+- The **username_key** setting: the parameter from the LTI 1.1 launch request used to set the JupyterHub username.
+- The **config_description** setting:
+- The **config_icon** setting:
+- The **config_title** setting:

-    _Note_: These commands will simply generate strings for you, it will not store them anywhere on the computer. Therefore you do not need to run these commands on your JupyterHub server--we will be supplying them manually in the next few steps.
+| LTI Authenticator Configuration Setting | Required | Description                                                              | Default                            |
+| --------------------------------------- | -------- | ------------------------------------------------------------------------ | ---------------------------------- |
+| config_description                      | No       | The LTI 1.1 external tool description                                    | `JupyterHub LTI 1.1 external tool` |
+| config_icon                             | No       | The http/s URL with the LTI 1.1 icon                                     | `nil`                              |
+| config_title                            | No       | The LTI 1.1 external tool Title                                          | `JupyterHub`                       |
+| consumers                               | Yes      | The key/value pair that represents the client key and shared secret      | `{}`                               |
+| username_key                            | No       | The LTI 1.1 launch parameter that contains the JupyterHub username value | `canvas_custom_user_id`            |


This section is confusing to me.

"There are two configuration values to define with LTI 1.1:"
I wonder both about "two" and "to define". Is the user required to set two values? Then why do I only see one listed as required in the table below?

There is a bullet list table, followed by a markdown formatted table and both include the same things. I assume the bullet list table should be removed as an outdated duplicate.

You're right, I goofed and had left the bullets in there by mistake. Only the table is documented now.

consideRatio · 2021-09-01T02:23:32Z

+# Custom config for JupyterHub's helm chart
+auth:
+  type: "lti"
+  lti:
+    consumers: { "client-key": "client-secret" }
+    username_key: "lis_person_contact_email_primary"
+    config_icon: "https://my.static.assets/img/icon.jpg"


The documentation above is outdated, it should be like this since ~0.10.X or 0.11.X i think, and the chart is now at 1.1.3.

Suggested change

# Custom config for JupyterHub's helm chart

auth:

type: "lti"

lti:

consumers: { "client-key": "client-secret" }

username_key: "lis_person_contact_email_primary"

config_icon: "https://my.static.assets/img/icon.jpg"

# Custom config for JupyterHub's helm chart

hub:

config:

# Additional documentation related to authentication and authorization available at

# https://zero-to-jupyterhub.readthedocs.io/en/latest/administrator/authentication.html

JupyterHub:

authenticator_class: lti

LTI11Authenticator:

consumers: { "client-key": "client-secret" }

username_key: "lis_person_contact_email_primary"

config_icon: "https://my.static.assets/img/icon.jpg"

consideRatio · 2021-09-01T02:26:29Z

+```

-_Note_: You will see **Client Key** and **Consumer Key** used interchangably.
+> _Note_: all settings available with the `jupyterhub-ltiauthenticator` package are available with the `lti` key when defining a custom config for the JupyterHub helm chart.


Suggested change

> _Note_: all settings available with the `jupyterhub-ltiauthenticator` package are available with the `lti` key when defining a custom config for the JupyterHub helm chart.

> _Note_: Configuring for example `hub.config.LTI11Authenticator.username_key=lis_person_contact_email_primary` above is the same as in a `jupyterhub_config.py writing `c.LTI11Authenticator.username_key = lis_person_contact_email_primary`.

consideRatio · 2021-09-01T02:39:10Z

@jgwerner how is the endpoint /lti11/config that is returning the rendered XML template used in practice? Is it required to be around by LTI v1.1 or similar for example? Perhaps you can answer by updating the PR description for anyone reading info about this PR later as well.

consideRatio · 2021-09-01T02:42:21Z

+
+class LTI11ConfigHandler(BaseHandler):
+    """
+    Renders LTI 1.1 configuration file in XML format.


Could you elaborate a bit in this docstring, perhaps link out to relevant schemas as described in #49?

…ings

jgwerner · 2021-09-01T05:25:27Z

@consideRatio updates are pushed based on your suggestions

consideRatio

Wieee nice! Ready for merge I'd say, do you want to review this also @yuvipanda?

yuvipanda · 2021-09-01T10:00:30Z

Go for it, @consideRatio. Let's be generous with our minor versions :)

consideRatio · 2021-09-01T11:34:26Z

Wieeee great work as always @jgwerner, I appreciate how caring you are for this project!!

jgwerner added the enhancement label Aug 29, 2021

jgwerner requested a review from consideRatio August 29, 2021 22:23

jgwerner self-assigned this Aug 29, 2021

jgwerner added 7 commits August 29, 2021 18:25

create config xml from template

19c38c2

Signed-off-by: Greg Werner <werner.greg@gmail.com>

update lti11 generic config

391aae2

update config descriptions and defaults

96c5e81

add lti 1.1 config handler

cda2941

update config instructions

80ea7c1

Signed-off-by: Greg Werner <werner.greg@gmail.com>

adds notebook dependency to dev reqs

2279fe6

Signed-off-by: Greg Werner <werner.greg@gmail.com>

remove merge commit text from source

fb81fdc

Signed-off-by: Greg Werner <werner.greg@gmail.com>

consideRatio reviewed Aug 29, 2021

View reviewed changes

Comment thread ltiauthenticator/lti11/auth.py

remove merge conflicts

b435d86

Signed-off-by: Greg Werner <werner.greg@gmail.com>

jgwerner added 3 commits August 29, 2021 18:33

lint files

cfc1883

Signed-off-by: Greg Werner <werner.greg@gmail.com>

fix merge and rebase conflicts

4f04712

clarify config_icon config

1adaa25

Signed-off-by: Greg Werner <werner.greg@gmail.com>

consideRatio reviewed Aug 30, 2021

View reviewed changes

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md

Comment thread ltiauthenticator/lti11/handlers.py Outdated

update docs and code cleanup

27ddd4b

Signed-off-by: Greg Werner <werner.greg@gmail.com>

jgwerner requested a review from consideRatio August 30, 2021 21:22

changes docstring from gets to renders

f3f7e13

Signed-off-by: Greg Werner <werner.greg@gmail.com>

consideRatio reviewed Sep 1, 2021

View reviewed changes

consideRatio changed the title ~~Add LTI 1.1 config handler~~ Add LTI 1.1 config handler for the /lti11/config route Sep 1, 2021

consideRatio reviewed Sep 1, 2021

View reviewed changes

remove superfluous bullets in readme and update config handler docstr…

9848a67

…ings

consideRatio approved these changes Sep 1, 2021

View reviewed changes

consideRatio merged commit 307e95e into jupyterhub:main Sep 1, 2021

consideRatio mentioned this pull request Sep 1, 2021

Add changelog for 1.2.0 #69

Merged

	> _Note_: all settings available with the `jupyterhub-ltiauthenticator` package are available with the `lti` key when defining a custom config for the JupyterHub helm chart.
	> _Note_: Configuring for example `hub.config.LTI11Authenticator.username_key=lis_person_contact_email_primary` above is the same as in a `jupyterhub_config.py writing `c.LTI11Authenticator.username_key = lis_person_contact_email_primary`.

Uh oh!

Conversation

jgwerner commented Aug 29, 2021

Uh oh!

Uh oh!

consideRatio commented Aug 29, 2021 • edited by jgwerner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

consideRatio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgwerner commented Aug 30, 2021

Uh oh!

consideRatio commented Aug 30, 2021

Uh oh!

jgwerner commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

consideRatio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

consideRatio Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

jgwerner Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

consideRatio Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

jgwerner Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

consideRatio Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

consideRatio commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

consideRatio Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

jgwerner commented Sep 1, 2021

Uh oh!

consideRatio left a comment

Choose a reason for hiding this comment

Uh oh!

yuvipanda commented Sep 1, 2021

Uh oh!

consideRatio commented Sep 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

consideRatio commented Aug 29, 2021 •

edited by jgwerner

Loading

jgwerner commented Aug 30, 2021 •

edited

Loading

consideRatio left a comment •

edited

Loading

consideRatio commented Sep 1, 2021 •

edited

Loading