Make the relay_state optional in the response. #15
Conversation
If relay_state isn't part of the outgoing request, it won't come back as a response. In that case, the code wouldn't work.
matejak
force-pushed
the
no_relay_needed
branch
from
232146a
to
253d1c1
Compare
| @@ -37,7 +37,7 @@ class ServiceProvider: | |||
| def login_successful( | |||
| self, | |||
| auth_data: AuthData, | |||
| relay_state: str, | |||
| redirect_to: str, | |||
There was a problem hiding this comment.
As you've used request.form.get('RelayState') below, the type of this should now be Optional[str]
| @@ -49,7 +49,9 @@ def login_successful( | |||
| but they *must* call ``super()``. | |||
| """ | |||
| self.set_auth_data_in_session(auth_data) | |||
| return redirect(relay_state) | |||
| if not redirect_to: | |||
| redirect_to = self.get_login_return_url() | |||
There was a problem hiding this comment.
This should be get_default_login_return_url(), as get_login_return_url() will check request.args.get('next'), potentially allowing the IdP to inject its own redirect parameter. (Ignoring that the IdP could alter RelayState to achieve the same thing).
Both get_login_return_url() and get_default_login_return_url() can return None in their default implementations. In this case, it would be appropriate to raise an error in login_successful(), as there is nothing else we can do. If the developer has not overridden get_default_login_return_url() then there is no where to redirect to.
There was a problem hiding this comment.
Thanks for your review!
As this is a web application, I am a bit reluctant to raise an error "by default". What about returning / as default login return URL, so it is not so easy to get a traceback?
If
relay_stateisn't part of the outgoing request, it won't come back as a response. In that case, the code wouldn't work.I have renamed the
relay_stateto its semantic meaningredirect_to, and adjusted the code so that it conforms with the method's comments.This behavior occurs when
SERVER_NAMEis not specified in the configuration, which also affectsget_login_return_urlthough - in that case, no URL will pass theis_valid_redirect_urlvalidation, as the code callsmake_absolute_urlwhich needs the server name set to work properly.