-
Notifications
You must be signed in to change notification settings - Fork 44
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
added url as a parameter to allow user to set the url directly, becau… #65
base: master
Are you sure you want to change the base?
Conversation
…se oauth requires the exact url that LTI used, but the url can be changed by a redirect, rewrite, proxy, ssl offload, or etc.
@rmundkowsky please fix pep8 issues |
I corrected pep 8 issues. I am not sure if I need to create new pull request or if this request will pull the corrected code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for multiple instance under load balancer, users need to change change url in each instance?
@@ -327,7 +327,7 @@ def close_session(): | |||
session[LTI_SESSION_KEY] = False | |||
|
|||
|
|||
def lti(app=None, request='any', error=default_error, role='any', | |||
def lti(app=None, request='any', error=default_error, role='any', url=None, | |||
*lti_args, **lti_kwargs): | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update the doc string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment as requested. Hopefully the comment will answer your question.
pylti/flask.py
Outdated
@@ -339,6 +339,13 @@ def lti(app=None, request='any', error=default_error, role='any', | |||
:param: request - Request type from | |||
:py:attr:`pylti.common.LTI_REQUEST_TYPE`. (default: any) | |||
:param: roles - LTI Role (default: any) | |||
:param: url - This is an optional setting that allows you to set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pep8 issues
pep8 corrected again. Sorry, about that. |
please add unit test to assert that ur changed, pylti uses that url |
finally got back to this. I am not sure how to make a unit test for this. |
Added url as a parameter to allow user to set the url directly, because oauth requires the exact url that LTI used, but the url can be changed by a redirect, rewrite, proxy, ssl offload, or etc.
For example, the LTI consumer might use https://foo.bar.com, but your app might be behind a load balancer, so url by the time it gets to the Flask app is http://server1.foo.bar.com:8443/
I had tried to make a decorator to do this, but was pretty complex. This approach is easier and will be commonly needed by LTI users anyways.