Feature/symfony responses (part 2) - #2648

The LogoutTest.php is failing because it gets a redirect which is not seen as success, https://github.com/symfony/symfony/blob/b066d276a261458c1ad93d9418c424a8ebefac25/src/Symfony/Component/HttpFoundation/Response.php#L1184

That can work if replaced with

$this->assertTrue($response->isRedirection());

I think the IndexTest.php are failing because the sendContent in RunnableResponse needs to get the Response from call_user_func_array and run that rather than just ignoring it...

--- a/src/SimpleSAML/HTTP/RunnableResponse.php
+++ b/src/SimpleSAML/HTTP/RunnableResponse.php
@@ -65,7 +65,10 @@ class RunnableResponse extends Response
      */
     public function sendContent(): static
     {
-        call_user_func_array($this->callable, $this->arguments);
+        $response = call_user_func_array($this->callable, $this->arguments);
+        if ($response instanceof Response) {
+            $response->send();
+        }
         return $this;
     }
 }

Looking at the error in MetaDataStorageHandlerTest.php:98 it seems some of the metadata in tests/src/SimpleSAML/Metadata/test-metadata/source1 were updated to other path/entitiyid.

./tests/src/SimpleSAML/Metadata/test-metadata/source1/saml20-sp-remote.php

$metadata['http://localhost/simplesaml'] = [
    'entityid' => 'http://localhost/simplesaml',
   ...

That array key and entityid seems to remain unchanged but the test suite file is now looking for $entities['https://simplesamlphp.org/simplesaml'] circa line 100 in the PR for MetaDataStorageHandlerTest. So it may be we should just change the line 100 back to localhost/simplesaml in the test suite.

For tests/modules/saml/src/Controller/ServiceProviderTest.php
The old code was passing base64_encode($xml) the update is using base64_encode(gzdeflate($xml)).

One way to support the new code is to add in HTTPPost.php

public function receive(ServerRequestInterface $request): AbstractMessage
...
        $haveToDeflate = false;
        if (array_key_exists('SAMLEncoding', $query)) {
            if( $query['SAMLEncoding'] == 'urn:oasis:names:tc:SAML:2.0:bindings:URL-Encoding:DEFLATE' ) {
                $haveToDeflate = true;
            }
        }
...
        $msgStr = base64_decode($msgStr, true);  // existing line
        if( $haveToDeflate ) {
            $msgStr = gzinflate($msgStr);
        }
...

This makes this particular --filter work but may impact other tests?

at array key and entityid seems to remain unchanged but the test suite file is now looking for $entities['https://simplesamlphp.org/simplesaml'] circa line 100 in the PR for MetaDataSt

Yes, changing the test-data was probably not a good idea

For tests/modules/saml/src/Controller/ServiceProviderTest.php The old code was passing base64_encode($xml) the update is using base64_encode(gzdeflate($xml)).

I would have to look into this again.. I added the gzdeflate() because I thought it fixed something, but it may have been a wrong call

The SPTest.php:282 test failure seems to be boolean conversion issues coming from the conversion in sendSAML2AuthnRequest().

In vendor/simplesamlphp/saml2/src/XML/samlp/AuthnRequest.php in AuthnRequest::toUnsignedXML()

The getForceAuthn call doing a trip= on true doesn't fire, adding something like the below near to top to handle the forcen attribute changes things to working...

        if ($this->getForceAuthn()->toBoolean() === true ) {
            $e->setAttribute('ForceAuthn', strval($this->getForceAuthn()));
        }

The SPTest.php:282 test failure seems to be boolean conversion issues coming from the conversion in sendSAML2AuthnRequest().

In vendor/simplesamlphp/saml2/src/XML/samlp/AuthnRequest.php in AuthnRequest::toUnsignedXML()

The getForceAuthn call doing a trip= on true doesn't fire, adding something like the below near to top to handle the forcen attribute changes things to working...

        if ($this->getForceAuthn()->toBoolean() === true ) {
            $e->setAttribute('ForceAuthn', strval($this->getForceAuthn()));
        }

I think you're looking at the wrong version of the saml2-library. toBoolean() is v6+ and the AuthnRequest here is still one from v4 (saml2-legacy).
The test on itself is correct, but it's just that it's not picking up the ForceAuthn=true from the state.

It might have been cleaner to have the two saml libraries having more distinct namespaces.

saml2 = use SimpleSAML\SAML2\XML\samlp\AuthnRequest
saml2-legacy = use SAML2\AuthnRequest;

It seems both are in use in SP.php:

use SimpleSAML\SAML2\XML\samlp\AuthnRequest as AuthnRequestNew;
...
        // Convert legacy AuthnRequest to new one until we are fully migrated to saml2v6
        $newar = AuthnRequestNew::fromXML($ar->toUnsignedXML()); // [1]

[1]

The SPTester.php has

use SimpleSAML\SAML2\XML\samlp\AuthnRequest;
...
    public function sendSAML2AuthnRequest(Binding $binding, AuthnRequest $ar): never
    {
        ...
        $ar = Message::fromXML($ar->toXML()); // this calls the toXML I cited  // ABC //

It is not obvious to a new reader but createAuthnRequest() calls startSSO2Test which calls startSSO2 which will call back into sptester->sendSAML2AuthnRequest. That will throw back the $ar made above by calling Message::fromXML using an exception. That exception is caught in createAuthnRequest and marshalled back into a return value for the method.

So if the conversion on the ABC line above is not complete the test will fail.

It is also possible I am missing something. I did a composer update to make sure I am on the selected vendor libraries. That also wiped out my changes in there (no big deal). Adding back the toBoolean allowed the testForcedAuthn test to pass again.

It seems both are in use in SP.php

Yes, there was no other option. The good part is that we can gradually rewrite code to use the new library.

Adding back the toBoolean allowed the testForcedAuthn test to pass again.

No it doesn't :/

1) SimpleSAML\Test\Module\saml\Auth\Source\SPTest::testForcedAuthn
Error: Call to a member function toBoolean() on false

The test is correct. Something appears to be wrong with the conversion from old AuthnRequest to NewAuthnRequest

ExampleAuthTest.php almost works. For me locally I am getting a 303 response which isSuccessful() fails on. So the assert must be for redirection optionally.

        $this->assertTrue($response->isSuccessful() || $response->isRedirection());

Again, I would not be surprised if there were differences between what I get here and what happens on the test suite :/

Maybe we could follow a 303 to get the real result instead in the suite. If the redirection is to an error page then the test suite should maybe try to detect that situation.

I also added to the use list:

use Symfony\Component\HttpFoundation\Response;

I guess we have tracked down the SPTest to being that conversion.

I'll have to work out where/why my local setup for this differs for the support library versions to be of more assistance. I am still working locally on the checkout for part1 of the PR.

edit composer.json

        "simplesamlphp/saml2": "~6.1",
        "simplesamlphp/saml2-legacy": "~4.20",

composer show

simplesamlphp/saml2                            6.2.3  SAML2 PHP library from SimpleSAMLphp
simplesamlphp/saml2-legacy                     4.20.3 SAML2 PHP library from SimpleSAMLphp

Making a clean directory, copying composer.json to it and doing composer install gives me:

$ composer show|grep saml2
simplesamlphp/saml2                            6.2.3  SAML2 PHP library from SimpleSAMLphp
simplesamlphp/saml2-legacy                     4.20.3 SAML2 PHP library from SimpleSAMLphp

The same deflate code in HTTPPost::receive that I mention in #2648 (comment) will also allow ./tests/modules/saml/src/Controller/ServiceProviderTest.php --filter testACSExpectedIssuerMismatchHardFails to pass.

Thanks for looking into this! My priorities have shifted to the libraries because of an upcoming code-review.

My plan is to incorporate a few of the comments I have made and work out what is still outstanding to fix for the PR.