Skip to content

User specified hostnames for redirect #148

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

Merged

Conversation

chander
Copy link
Contributor

@chander chander commented Apr 15, 2019

Post-login redirect URL's may often be generated from a client that is accessing an API, however the current implementation only allows redirect urls to point to the same server. This results in the need for confusing redirect-to-another-route-so-we-can-redirect elsewhere type cases. This fixes that by allowing the user to list a set of domains that may be redirected to. The default is to work as it has in the past - so no end user work is needed on upgrading. However, by specifying a SAML_ALLOWED_HOSTS parameter they can open up the allowable redirect hostnames.

chander and others added 2 commits April 15, 2019 17:19
Post-login redirect URL's may often be generated from a client that is accessing an API, however the current implementation only allows redirect urls to point to the same server.  This results in the need for confusing redirect-to-another-route-so-we-can-redirect elsewhere type cases.  This fixes that by allowing the user to list a set of domains that may be redirected to.  The default is to work as it has in the past - so no end user work is needed on upgrading.  However, by specifying a SAML_ALLOWED_HOSTS parameter they can open up the allowable redirect hostnames.
@peppelinux
Copy link
Member

Hi @chander, that sound like a good feature. My only doubt is if a redirect ULR that not belongs to the requester FQDN could be compliant with the good practice, we know that the data release agreement show to the user the entity where his attributes will be send.

@sheilatron can you help us to decide what to do? :)
Let us know, this could be merged in v0.18.2

@peppelinux peppelinux added this to the v0.18.2 milestone Apr 29, 2020
@chander
Copy link
Contributor Author

chander commented Apr 29, 2020

Hi @chander, that sound like a good feature. My only doubt is if a redirect ULR that not belongs to the requester FQDN could be compliant with the good practice, we know that the data release agreement show to the user the entity where his attributes will be send.

So the redirect is essentially a bare URL, without the appropriate CORS headers the SAML related data wouldn't be present to the redirected host. Furthermore, the SAML_ALLOWED_HOSTS parameter would limit the allowed hosts for the redirect, allowing the server admin to prevent the redirect. Redirecting elsewhere - like a third party site - would be the same as clicking a link to that external site (without valid CORS setup) since no credential information is contained in the redirect .

As a side note, my specific use case is that my SAML Auth happens via a RESTful API server (let's call it api.gridgeo.com) and the UI is present at a different hostname - let's call that (www.gridgeo.com) - after login via SAML we might want the user to end up where they started (www.gridgeo.com/contact) in which case the server can redirect the user there after login - rather than some generic UI location. Fundamentally, this gives the developer the ability to maintain the state of the user's interface post-login.

Hopefully that example lays out the basic premise for this enhancement a bit better.

@peppelinux
Copy link
Member

Sounds good, I just have to ask you to update is_safe_url_compat with is_safe_url or the suggested in latters django releases (@francoisfreitag you can tell something important here if you agree).

Then I'd ask you to put a simple description of this feature in the README and a general refactor to handle the lates conflicts. We're in run to merge our contribution in v0.18.2, many thanks

@peppelinux
Copy link
Member

Sounds good, I just have to ask you to update is_safe_url_compat with is_safe_url or the suggested in latters django releases (@francoisfreitag you can tell something important here if you agree).

Then I'd ask you to put a simple description of this feature in the README and a general refactor to handle the lates conflicts. We're in run to merge our contribution in v0.18.2, many thanks

I meant this:
162abf6#diff-4900f8e6ae1c6fa3531d6830554a5f08R362

chander added 2 commits April 30, 2020 13:32
Update the readme to include documentation regarding how SAML_ALLOWED_HOSTS works and is used.
@chander
Copy link
Contributor Author

chander commented Apr 30, 2020

Okay, I believe the merge conflicts are fixed and the README has been updated as requested.

Thank You

@chander
Copy link
Contributor Author

chander commented Apr 30, 2020

I think you can just merge. Is travis not working right? I'm looking at the failures and they look unrelated to my code.

@peppelinux peppelinux merged commit 2f80dbb into IdentityPython:master Apr 30, 2020
@peppelinux
Copy link
Member

Yes, we have some trouble with py38 but we'll move ahead soon

@peppelinux
Copy link
Member

Hi @chander I made a Refactor of your code here:
a343d10

This specilized a function to do that and reduce code repetition

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.

2 participants