onelogin / drupal-saml

MIT License
14 stars 17 forks source link

Bug : an extra set of redirects from relay state. #14

Closed chriscalip closed 8 years ago

chriscalip commented 9 years ago

drupal-saml requires these form fields:

IdP Entity Id Single Sign On Service Url X.509 Certificate *

Is IdP Entity Id really necessary? ; It seems that my idp source does not have a url resource for metadata they only provide service url, x.509 cert.

chriscalip commented 9 years ago

We were troubleshooting a situation .. were drupal-saml was adding a relayState query and that caused an extra set of redirects .. i figured that since we added idp entity id with the same value of single sign on service url.. and that was causing the exta set of redirects.. it does not seem to to be the case...

jasonbell commented 9 years ago

It appears that in onelogin_saml/functions.php lines 50-53, the RelayState appends the original page request:

if (isset($_POST['RelayState'])) {
drupal_goto($_POST['RelayState']);
} else {
drupal_goto('');

At least in our use case we need to skip the RelayState since it is required to be returned in response per SAML 2.0 spec (discussion… https://support.google.com/a/answer/2463723?hl=en). Could this be configurable via the admin screen?

chriscalip commented 9 years ago

drupal-saml-acs-sso-multiredirect

A charles proxy screenshot showing a second revisit on sso-acs track

pitbulk commented 9 years ago

@jasonbell Is the common workflow to redirect the user to the RelayState after process the ACS. What is your proposal?

If you implement in the admin panel a functionality to skip the RelayState and redirect to a "default" page. I'm ok with this and will accept the PR.

jasonbell commented 9 years ago

@pitbulk from what we can see, the SAML 2.0 spec requires that if a RelayState is provided by the service provider, then the identity provider should not modify it.

In onelogin_saml/functions.php lines 50-53, the initial page request in Drupal to /onelogin_saml/sso is passed along as RelayState in the redirect to the Single Sign On Service Url. What we have seen in our uses is that this creates a loop…

  1. Click on "Login using SAML"
  2. Redirect to ID provider
  3. Authenticate and return to /onelogin_saml/acs?destination=onelogin_saml/sso
  4. Redirect to ID provider
  5. Authenticate and return to /onelogin_saml/acs and now we’re in

This is all new to us and I don't have a specific proposal. It seems like it might be helpful to have an option to disable or override the RelayState or Target values that are sent to the IdP. We have just put a little code hack in those lines to skip goto($_POST['RelayState']) and direct all traffic to the root.

pitbulk commented 9 years ago

@jasonbell The problem is that the value of the destination never should be set to onelogin_saml/sso

Destination is the "Drupal Destination" that was required here: https://github.com/onelogin/drupal-saml/issues/10

That works for me: https://sp.example.com/drupal/?q=onelogin_saml/sso&destination=admin/modules With this url I access to the admin/modules view

I will take a look on this in a couple of days.

chriscalip commented 9 years ago

@pitbulk

For Clarity sake here's a video bug report showing the configuration settings and the extra set of redirects happening.. and the post variables in play with those redirects.

https://www.youtube.com/watch?v=jBILz7bKsL4

This video shows the steps and situation laid out by @jasonbell

chriscalip commented 9 years ago

@pitbulk

15 pull request should minimize bug occurrence but bug will still occur if user

does manual browser url entry : http://example.com/onelogin_saml/sso

pitbulk commented 9 years ago

I tried to reproduce your error, but in my drupal environment all is working as expected. http://youtu.be/XRUARixJzBc I introduced some changes, can you use the master branch and say to me if the error disappeared?

P.S My drupal SAML link is: https://pitbulk.no-ip.org/drupal/?q=onelogin_saml/sso Maybe you getting the error due your link is https://pitbulk.no-ip.org/drupal/onelogin_saml/sso ??

chriscalip commented 8 years ago

Clean up note, lets close issue for now as unable to reproduce.