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

Refactor LTI11 source #41

Merged
merged 7 commits into from
Apr 13, 2021
Merged

Refactor LTI11 source #41

merged 7 commits into from
Apr 13, 2021

Conversation

jgwerner
Copy link
Collaborator

Related to #38 #39

@jgwerner
Copy link
Collaborator Author

@yuvipanda this is the second PR related to #39. Once we merge this one I'll update #40 based on an agreed structure.

By the way, adding ltiauthenticator.lti11... and ltiauthenticator.lti13... to the package namespace is a breaking change for existing imports. Is that ok with you or would you rather have all classes in the ltiauthenticator root?

@yuvipanda
Copy link
Collaborator

This looks good too! What do you think about keeping ltiauthenticator.LTIAuthenticator as an alias so we don't break existing installs?

@jgwerner
Copy link
Collaborator Author

Good idea on the alias. I'll push and update with that change.

@jgwerner
Copy link
Collaborator Author

jgwerner commented Apr 8, 2021

@yuvipanda the requested changes have been pushed.

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Minor changes, + merge conflict resolution - otherwise LGTM!

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
_version.py Outdated Show resolved Hide resolved
ltiauthenticator/lti11/auth.py Outdated Show resolved Hide resolved
ltiauthenticator/lti11/handlers.py Outdated Show resolved Hide resolved
from collections import OrderedDict


class LTILaunchValidator:
Copy link
Collaborator

Choose a reason for hiding this comment

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

LTI11LaunchValidator?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yuvipanda this is a class I created as an abstraction to validate the LTI 1.1 requests since some are required and some optional and wanted to offer end users the option of fine tuning their own validations. I would be glad to remove this though if it's noisy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does 1.3 use the same validation logic? If so, then we can leave it as is! Otherwise just wanted a rename.

Thank you for going through this with me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does, but we can rename it to something else if you'd like.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I did not know that! If it's using the same validation logic, we can leave it as is. Thank you for the explanation :)

Copy link
Collaborator Author

@jgwerner jgwerner Apr 8, 2021

Choose a reason for hiding this comment

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

Actually, the Launch part in the class name is due to how the LTI 1.1 specification refers to logins as "Launch Requests". With LTI 1.3 the term changed to "Login Initiation Requests" so that it's more in line with OIDC/OAuth2 but we kept using "...Launch..." with LTI 1.3. We were planning to refactor the class name from LTI13LaunchValidator to LTI13LoginInitiationValidator for clarity but haven't gotten around to it.

Signed-off-by: Greg Werner <werner.greg@gmail.com>
Signed-off-by: Greg Werner <werner.greg@gmail.com>
Signed-off-by: Greg Werner <werner.greg@gmail.com>
Signed-off-by: Greg Werner <werner.greg@gmail.com>
Signed-off-by: Greg Werner <werner.greg@gmail.com>
@jgwerner
Copy link
Collaborator Author

jgwerner commented Apr 8, 2021

@yuvipanda I pushed the requested changes and added more tests (mostly for the validator). If possible, we would like to submit one more PR before tackling #39 so we update the logic for the LTI 1.1 validator so that it's more consistent with the LTI 1.3 validator logic.

Signed-off-by: Greg Werner <werner.greg@gmail.com>
Signed-off-by: Greg Werner <werner.greg@gmail.com>
@yuvipanda yuvipanda merged commit 214840a into jupyterhub:master Apr 13, 2021
@yuvipanda
Copy link
Collaborator

AWESOME! Thanks, @jgwerner :)

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.

3 participants