/auth/saml doesn't redirect to deep-linked pages

[Rich Trott]

/auth/saml/index.php always redirects to $CFG->wwwroot, even when the original page requested is something else.

The attached patch makes it so that it redirects to whatever page sent it to the /auth/saml/index.php in the first place.

A couple of notes:

1) I don't grok what's going on with the SESSION stuff...closing the session to let SAML do its thing, then opening the session again...so I just wrote directly to the $_SESSION array rather than using the abstraction. You may want to refactor that part, unless what I did happens to make sense in the context.

2) I suppose there should be a config option to force redirecting to a front page and forbid deep-linking? Not sure.

Patch applies to both 1.3_STABLE and master.

[Ruslan Kabalin (rkabalin)]

Hello Rich,

I do not have SAML installation to test it unfortinately. Can you please tell me what (! $saml_session->getIdP()) condition statement checks for, is that check a necessary one?
Also, it is probably needed to unset session variable before final redirection.

Can you please test the patch below (apply it instead of yours, not on top)?

[Rich Trott (richard-trott)]

When I run with your patch, I get:
  Fatal error: Call to a member function get() on a non-object in /web/mahara/mahara/htdocs/auth/saml/index.php on line 82

$SESSION has not been initialized, so that would seem to be the problem. Previously, I had played around with instantiating the object early, but that seemed to have other undesirable side effects that I am unable to recall right now.

If I understand correctly, when saml_session->getIdP() returns something, that means that we've been directed back to index.php, so the referer is the IdP. If it's *not* set, then it's our first time through, so the referer should be the Mahara page the user wants. Admittedly, there may be room for improvement in that logic and/or there may be things about SAML/etc. that I'm failing to take into consideration. But it does seem to work in my environment.

[Rich Trott (richard-trott)]

I applied your patch, and then moved these two lines directly above the check for samluserfrom in the session.

require_once(dirname(dirname(dirname(__FILE__))) . '/auth/lib.php');
$SESSION = Session::singleton();

If I did that, a successful login was redirected to /auth/saml (which then reports an error) rather than to the page I requested.

[Rich Trott (richard-trott)]

I'm attaching a slightly improved patch. Improvements from my first attempt:

1) It uses the Session object rather than the $_SESSION variable.
2) As you (Ruslan) recommended, it clears the wantsurl data out of the Session object.

I tried to move the reading of wantsurl (and clearing of the wantsurl data) to immediately before the redirect that uses $wants_url (which would eliminate the passing of $wants_url as a parameter to simplesaml_init()), but by that point, the Session object has been transformed somewhere somehow (part of the SAML process inherently?), and the data is already gone.

[PiersHarding (piersharding)]

Hi -
Thanks for looking into this problem - this is much easier to fix in Moodle than Mahara as Moodle already has the concept of wantsurl built into the authentication system.

I have committed a hybrid fix to HEAD that will preserve a wantsurl query string parameter throughout the saml authentication redirection process, based on what is initially stuffed in the session relating to simplesamlphp. Incidentally, this is where all the grief comes from with saml and sessions as, the session management scheme configured for ssphp, is not necessarily the same as for mahara, and there is an added complication of ssphp registered shutdown handlers.

So - if a user goes to http://mahara.local.net/maharadev/auth/saml/?wantsurl=http://mahara.local.net/maharadev/user/view.php... then they will end up at http://mahara.local.net/maharadev/user/view.php... after loggin in.

I realise that this is not a complete solution as wantsurl is not being automatically determined by the initial access attempt (eg. goto http://mahara.local.net/maharadev/user/view.php?id=2 but get the login to Mahara screen), but there is another step that needs to be resolved here, in that the default login screen needs to detect that the user is not logged in and have the capacity to offer a 'click here to login via SSO' link(plugin would need to calculate this link), or completely override the login challenge screen and redirect to /auth/saml (don't like this option as SAML should not be the only auth mechanism available - need a backup option..).

Cheers,
Piers Harding.

[Rich Trott (richard-trott)]

Piers,

I'm attaching a patch that will detect the referer in the event that wantsurl is not specified.

--Rich

[PiersHarding (piersharding)]

Hi Rich - I've worked in your patch, and would appreciate if you could test.

Thanks,
Piers Harding.

[Rich Trott (richard-trott)]

Clean check-out of master seems to work! It obeys wantsurl if it's there, otherwise it interprets referer correctly.