Skip to content

Add username_key as an LTI 1.1 Authenticator configurable - #48

Merged
yuvipanda merged 6 commits into
jupyterhub:masterfrom
jgwerner:feat/username-with-config
Jun 16, 2021
Merged

Add username_key as an LTI 1.1 Authenticator configurable#48
yuvipanda merged 6 commits into
jupyterhub:masterfrom
jgwerner:feat/username-with-config

Conversation

@jgwerner

@jgwerner jgwerner commented Jun 2, 2021

Copy link
Copy Markdown
Collaborator
  • Adds the LTI11Authenticator.username_key to set the username returned by the authenticator to something other than custom_canvas_user_id or user_id if specified
  • Adds basic tests for LTI11AuthenticateHandler class
  • Updates Readme

Continuation of #14

Closes #30

cc/ @BenGig

Signed-off-by: Greg Werner <werner.greg@gmail.com>
jgwerner added 2 commits June 2, 2021 19:52
Signed-off-by: Greg Werner <werner.greg@gmail.com>
Signed-off-by: Greg Werner <werner.greg@gmail.com>

@yuvipanda yuvipanda left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome! Couple minor things, happy to merge afterwards

Comment thread ltiauthenticator/lti11/auth.py Outdated
Comment thread ltiauthenticator/lti11/auth.py Outdated
@yuvipanda

Copy link
Copy Markdown
Collaborator

What do you think of making a release after this gets merged?

@jgwerner

jgwerner commented Jun 3, 2021

Copy link
Copy Markdown
Collaborator Author

@yuvipanda creating a new release sounds good to me! I'll update this PR based on your comments, thanks for the review.

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

@jgwerner wow amazing work!!! I appreciate how thorough you are!

Excellent documentation, excellent testing! ❤️ 🎉

Assuming @yuvipanda comments are addressed, this LGTM so I'll press approve at this point!

Comment thread README.md
Signed-off-by: Greg Werner <werner.greg@gmail.com>
@jgwerner

jgwerner commented Jun 8, 2021

Copy link
Copy Markdown
Collaborator Author

@yuvipanda I updated the PR with the following logic:

  • Log a deprecation warning if the username_key = "custom_canvas_user_id"
  • Check if the username_key is in the launch request
  • If the username_key is not in the launch request raise an HTTP error
  • If the username is empty or None, fallback to the user_id value
  • If the username is still empty raise an HTTP error

There were also some duplicate validator tests in the authentication tests file so removed them. Perhaps we should have refactored tests in another PR, but wanted to make sure the flow above had passing tests without duplicates.

Signed-off-by: Greg Werner <werner.greg@gmail.com>
Comment thread ltiauthenticator/lti11/auth.py Outdated
Comment thread ltiauthenticator/lti11/auth.py Outdated
@yuvipanda

Copy link
Copy Markdown
Collaborator

@jgwerner thank you for your patience :) This lgtm for the most part - I left some documentation change suggestions, what do you think? Happy to merge this after.

@jgwerner

Copy link
Copy Markdown
Collaborator Author

@yuvipanda I update the PR based on your suggestions, thanks for the documentation tips 👍

Signed-off-by: Greg Werner <werner.greg@gmail.com>
@yuvipanda
yuvipanda merged commit 01009c5 into jupyterhub:master Jun 16, 2021
@yuvipanda

Copy link
Copy Markdown
Collaborator

Thank you, @jgwerner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use usernames instead of user IDs

3 participants