Skip to content

Conversation

@matejak
Copy link

@matejak matejak commented Jan 13, 2020

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.

I have renamed the relay_state to its semantic meaning redirect_to, and adjusted the code so that it conforms with the method's comments.

This behavior occurs when SERVER_NAME is not specified in the configuration, which also affects get_login_return_url though - in that case, no URL will pass the is_valid_redirect_url validation, as the code calls make_absolute_url which needs the server name set to work properly.

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.
Copy link
Owner

@mx-moth mx-moth left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request!

self,
auth_data: AuthData,
relay_state: str,
redirect_to: str,
Copy link
Owner

Choose a reason for hiding this comment

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

As you've used request.form.get('RelayState') below, the type of this should now be Optional[str]

self.set_auth_data_in_session(auth_data)
return redirect(relay_state)
if not redirect_to:
redirect_to = self.get_login_return_url()
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants