Refactor LTI 1.1 validator #44
Conversation
|
|
||
| launch_url = protocol + "://" + handler.request.host + handler.request.uri | ||
| self.log.debug( | ||
| "Original arguments received in request: %s" % handler.request.arguments |
There was a problem hiding this comment.
Will these be sensitive?
There was a problem hiding this comment.
@yuvipanda Not that I am aware of. The launch request arguments could contain sensitive information if the integration with the LMS is set to public, in which case the username is fetched from one of the personally identifiable arguments, such as email. When the application is set to private, the user_id is an opaque identifier and not considered personally identifiable information (PII). But in these cases the arguments that would contain PII values are empty. The other arguments are related to context (courses) or assignments/modules (resources), etc. Lastly, there are oauth_* arguments but these shouldn't be that sensitive. We could obfuscate some or all arguments from the request for logging, what do you think?
There was a problem hiding this comment.
Given that it's debug logging I think it's ok. We already log user_ids elsewhere in JupyterHub.
There was a problem hiding this comment.
@yuvipanda thanks for the review and the suggestions! Perhaps we could contribute a PR to add a feature to We can truncate the string length based on the specific needs for Kubernetes / Docker names separately.esapism so users can truncate the string to a predetermined number of characters and have the string length default to 253, although it's unlikely that it would be longer. I don't see that option in the esapism package.
|
@yuvipanda PR #43 is now updated to replace the custom utility functions with |
|
@yuvipanda to make the review process a little simpler for you I rebased this branch with #43. |
Signed-off-by: Greg Werner <werner.greg@gmail.com>
Signed-off-by: Greg Werner <werner.greg@gmail.com>
|
Thank you very much for your patience, @jgwerner! |
|
Wieeee thank you for your effort on this @jgwerner ❤️ 🎉 🌻 !! |
The LTI 1.3 validator has more validation requirements than LTI 1.1. This PR refactors the validator to establish a more common pattern between both LTI versions.
Related to #39