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.

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

feat(lti13): Reintroduce lti13 support #134

Merged
merged 33 commits into from
Mar 23, 2023

Conversation

martinclaus
Copy link
Collaborator

@martinclaus martinclaus commented Mar 1, 2023

This PR will reintroduce LTI 1.3 support. In particular, it will only implement support for the login flow initiated by a Resource Link Launch Message sent by an LMS platform. This message will trigger an OpenID Connect Implicit flow for authentication which is initiated from a third party.

image

LTI Deep Linking will not be implemented.

Required handlers:

  • init login (GET and POST)
  • auth callback (only POST)

Note: since we are not sending tool-originating messages, there is no need for an JWKS endpoint exposed by the authenticator.

Validation:

@martinclaus martinclaus marked this pull request as draft March 1, 2023 11:44
@martinclaus
Copy link
Collaborator Author

martinclaus commented Mar 1, 2023

PyJWT offers extended verification options to verify / validate aud, iss, exp and iat. It also allows to specify a list of required claims.

IMO, this is the most elegant way to do much of the non-LTI specific validation.

@martinclaus
Copy link
Collaborator Author

Nonce validation and replay attack mitigation (#127) is implemented following the OpenID Connect Implementation Notes.

@martinclaus martinclaus marked this pull request as ready for review March 7, 2023 12:34
@martinclaus martinclaus marked this pull request as draft March 7, 2023 12:34
@martinclaus martinclaus marked this pull request as ready for review March 8, 2023 09:44
@martinclaus
Copy link
Collaborator Author

martinclaus commented Mar 8, 2023

@consideRatio I'm sorry 🙏 , this became a huge PR. It mostly adds additional (sometimes required) validation steps defined in the OICD Core specs. Some of id_token validation is done by PyJWT (aud, iss, exp, iat).

I have tested f75ae0f on our staging deployment and it works.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Amazing work on this @martinclaus!!! This looks great to me overall and my comments are just smaller details! I reviewed this commit by commit, but not the initial commit reverting what was there before, and couldn't find issues in the changes!

  • I suggested a version bump of dependencies to ensure tests pass and that we don't get outdated.
  • I suggest we remove LTI13_ environment variables and force users to configure this with c.LTI13Authenticator.issuer directly instead of partially supporting also doing it via environment variables. I think this can reduce the complexity of maintenance and documentation overall long term, and reduce the complexity for someone to understand configuration of a jupyterhub using this authenticator. I believe that the main value of allowing for environment variables are for passwords etc, but there aren't any secrets configured for this authenticator right?

@minrk @manics I think this looks very solid overall, what do you think? I'd appreciate having at one additional review pass on this before merging it.

pyproject.toml Outdated Show resolved Hide resolved
martinclaus and others added 2 commits March 8, 2023 14:41
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
Also remove configuration via env variables.
@martinclaus
Copy link
Collaborator Author

  • I suggest we remove LTI13_ environment variables and force users to configure this with c.LTI13Authenticator.issuer directly instead of partially supporting also doing it via environment variables.

Done. I still leave the option to use env variables in examples/jupyterhub_config_lti13.py.

@consideRatio
Copy link
Member

@martinclaus this is amazing work! I think it looks really thorough and well thought through! I asked Min to have a look just now who thought it looked fine as well, so let's go for a merge at this point.

Thank you soo much for your efforts into this!!!! ❤️ 🎉 🌻

@consideRatio consideRatio merged commit 414bc29 into jupyterhub:main Mar 23, 2023
@consideRatio
Copy link
Member

@martinclaus if you think it makes sense to go for a release or similar at this point in time or later, just open a changelog PR and ping for review.

@martinclaus
Copy link
Collaborator Author

I will take a look through the documentation once again, to make sure that it is consistent with the code base before preparing the next release.

@martinclaus martinclaus deleted the reintroduce-lti13-support branch March 23, 2023 13:38
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.

None yet

2 participants