Skip to content

fix(lti13): Add missing oauth handler - #124

Merged
consideRatio merged 9 commits into
jupyterhub:mainfrom
martinclaus:add-oauth-handler
Feb 20, 2023
Merged

fix(lti13): Add missing oauth handler#124
consideRatio merged 9 commits into
jupyterhub:mainfrom
martinclaus:add-oauth-handler

Conversation

@martinclaus

@martinclaus martinclaus commented Feb 9, 2023

Copy link
Copy Markdown
Collaborator

Currently, there is no route set to the OAuth2 login and callback handler in the LTI13Authenticator. This renders this authenticator essentially useless.

This PR adds routes to the respective handler. The paths are prefixed by lti13 to avoid potential conflicts when using multiple authenticators become possible.

A backport of this fix for the most recent release would be great! If desired, I can open another PR for that.

Ref: #123

@martinclaus

martinclaus commented Feb 9, 2023

Copy link
Copy Markdown
Collaborator Author

isort pre-commit hook broken until upgraded to 5.12.0. Locally, it passes the checks.

See #122

@martinclaus

Copy link
Copy Markdown
Collaborator Author

I have removed the get_handler method from MockLTI13Authenticator. Else test_lti_13_handler_paths is just testing the mocked paths.

@martinclaus

Copy link
Copy Markdown
Collaborator Author

Rebased to 7b533f7

@martinclaus martinclaus reopened this Feb 14, 2023
@martinclaus
martinclaus marked this pull request as draft February 14, 2023 13:19
@martinclaus

Copy link
Copy Markdown
Collaborator Author

Convert to draft until end-to-end testing is completed

@martinclaus

Copy link
Copy Markdown
Collaborator Author

After merging #126, the call to guess_callback_url in LTI13LoginHandler.post needs to reflect the changed path of the callback handler.

@consideRatio

Copy link
Copy Markdown
Member

[...] it needs to be decided if the paths to all LTI 1.3 endpoints shall be prefixed by /lti13.

👍 for systematic prefix in the URL for anything handled by LTI13LoginHandler, such as /lti13!


Thank you soo much for your work into this @martinclaus!!! The changes I see here looks good to me so far!


Do you wish to be invited as a maintainer of this repo? I think saying yes would entail:

  • an interst to some degree help out with the maintenance of this project
    • This can include for example to read/respond to issues, review/merge PRs, creating changelog PRs, making a release once a changelog PR is merged
  • skimming through and agreeing with Jupyter CoC
  • enabling 2FA if you don't have it enabled already on your github account

@martinclaus martinclaus reopened this Feb 16, 2023
@martinclaus

Copy link
Copy Markdown
Collaborator Author

Sorry for any confusion, I messed up my history when rebasing this PR.

@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.

  • I pushed a commit refactoring away the single use function with a name set by legacy reasons
  • I raise a question about forcing https protocol

Otherwise, this LGTM! Btw, this is no longer a draft PR right? Or is it?

@martinclaus thank you soo much for your work into this!!! If possible, I'd like to get this merged as soon as you respond and cut a release following it.

Comment thread ltiauthenticator/lti13/handlers.py
Comment thread ltiauthenticator/lti13/auth.py Outdated
@martinclaus

Copy link
Copy Markdown
Collaborator Author

I have also added a route for GET requests to the login handler, since this is formally allowed by the OpenID Conect launch flow. It is simply an alias for the post method.

IMO, this is ready to be merged.

@consideRatio
consideRatio marked this pull request as ready for review February 20, 2023 10:23
Comment thread ltiauthenticator/lti13/handlers.py
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
@consideRatio consideRatio changed the title Add oauth handler LTI13: Add missing oauth handler Feb 20, 2023
@consideRatio
consideRatio merged commit bad1285 into jupyterhub:main Feb 20, 2023
@martinclaus

Copy link
Copy Markdown
Collaborator Author

Do you wish to be invited as a maintainer of this repo? I think saying yes would entail:

  • an interst to some degree help out with the maintenance of this project

    • This can include for example to read/respond to issues, review/merge PRs, creating changelog PRs, making a release once a changelog PR is merged
  • skimming through and agreeing with Jupyter CoC

  • enabling 2FA if you don't have it enabled already on your github account

@consideRatio I am willing to help maintaining this repo. I habe enabled F2A and happily agree to Jupyters CoC. I would then following MinRk's advice and do a release removing LTI13Authenticator and open a PR to reintroduce it.

@consideRatio

Copy link
Copy Markdown
Member

Wieeee!!! Thank you soo much for your contributions once again Martin!

If you want to connect better with the jupyterhub ecosystem, you are also most welcome to join monthly jupyterhub meetings and such. We use https://github.com/jupyterhub/team-compass for coordination across projects and there in the issues is a notice on the upcoming meets.

@minrk @sgibson91 @yuvipanda @jgwerner for transparency I'm sending a maintainer invite to @martinclaus for jupyterhub/ltiauthenticator.

/ Erik from mobile

@martinclaus martinclaus changed the title LTI13: Add missing oauth handler fix(lti13): Add missing oauth handler Feb 28, 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.

2 participants