sp_request_initiation_protocol

[ioigoume]

Addresses issue #174

[pradtke]

@ioigoume It is looking pretty good. I'll test it out in our of our environments.

[pradtke]

I ran some tests in our dev environment and things behaved like I expected. Thank you @ioigoume

[ioigoume]

Hi @tvdijen,
this pull request is ready for review. We introduced a new method, the loginHandler, in order to facilitate testing.

Thank you,
Ioannis Igoumenos

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.86%. Comparing base (37bb64f) to head (1517e08).

Additional details and impacted files

@@                   Coverage Diff                   @@
##             simplesamlphp-2.3    #2073      +/-   ##
=======================================================
+ Coverage                44.71%   44.86%   +0.14%     
- Complexity                3849     3865      +16     
=======================================================
  Files                      162      162              
  Lines                    12876    12902      +26     
=======================================================
+ Hits                      5758     5788      +30     
+ Misses                    7118     7114       -4     

[monkeyiq]

I should probably also include a link here to the previous PR #2046

[monkeyiq]

One thing I notice is that in general this will accept RelayState and turn it into ReturnTo if there is no existing returnTo. If that is ok for a general case in SAML then all is well, otherwise we are introducing a potential change.

From my reading it seems RelayState might be a deep pointer the SP wants mirrored back from the IdP. Does the RelayState have potential other semantics with SP Request Initiation Protocol interactions?

It may also make sense to hint to the reader that they might like to investigate the trusted.url.domains configuration option to best validate the target. I imagine sites will already have trusted domains set up but mentioning it in the docs seems like a good idea to follow section 2.5 of the spec.

[ioigoume]

@monkeyiq

One thing I notice is that in general this will accept RelayState and turn it into ReturnTo if there is no existing returnTo. If that is ok for a general case in SAML then all is well, otherwise we are introducing a potential change.

According to the SAML specification

Some bindings define a "RelayState" mechanism for preserving and conveying state information. When such a mechanism is used in conveying a request message as the initial step of a SAML protocol, it places requirements on the selection and use of the binding subsequently used to convey the response. Namely, if a SAML request message is accompanied by RelayState data, then the SAML responder MUST return its SAML protocol response using a binding that also supports a RelayState mechanism, and it MUST place the exact RelayState data it received with the request into the corresponding RelayState parameter in the response.

and

Sometimes a binding-specific field called RelayState is used to coordinate messages and actions of IdPs and SPs, for example, to allow an IdP (with which SSO was initiated) to indicate the URL of a desired resource when communicating with an SP."

Based on above descriptions from the specification, the RelayState is just a pointer that the Service Provider will use to find out the final TARGET or ReturnTo URL.

From my reading it seems RelayState might be a deep pointer the SP wants mirrored back from the IdP. Does the RelayState have potential other semantics with SP Request Initiation Protocol interactions?

According to the protocol if the target is omitted then the SP MUST use a default value, which it unilaterally determines. In other words, the RelayState is the fallback in case the target URL is omitted.

It may also make sense to hint to the reader that they might like to investigate the trusted.url.domains configuration option to best validate the target. I imagine sites will already have trusted domains set up but mentioning it in the docs seems like a good idea to follow section 2.5 of the spec.

I added a sentence in the md file

[monkeyiq]

I am happy with the RelayState being used for the target if that is not set in a Sstc-request-initiation setting. As mentioned in 2.3.1 Defined Parameters in Sstc-request-initiation-cs-01 the SP must provide a default if nothing is given and has the choice of what to provide there.

[monkeyiq]

Actually that seems ok in general. The returnto is coming from the request first in both the old and new code and only from the sp metadata as a fallback in the PR. There isn't any call facing change here and if the sp->md has a relaystate it should come from the configuration and so be chosen by the admin anyway.

--- $returnTo = $request->query->get('ReturnTo');
+++ $request->query->get('ReturnTo') ?? $spSource->getMetadata()->getString('RelayState')

[monkeyiq]

It also helps the review that this has been tested in a dev environment as well :)