Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.97%. Comparing base (440e1a5) to head (c6cead3).
Report is 1 commits behind head on simplesamlphp-2.4.

@@                   Coverage Diff                   @@
##             simplesamlphp-2.4    #2270      +/-   ##
=======================================================
+ Coverage                44.80%   44.97%   +0.16%     
- Complexity                3902     3903       +1     
=======================================================
  Files                      163      163              
  Lines                    13008    13008              
=======================================================
+ Hits                      5828     5850      +22     
+ Misses                    7180     7158      -22     

I was thinking about the update in SAML2.php / getHostedMetadata, that each call to getGenerated has the $entityid passed in now. I am not sure if it is important but I was thinking about a case where this might change things for an install.

In short the thought I had was if you have a few hosted IdP defined and are relying on the value for SingleSignOnService always being the one from the first hosted entity (as it seems to be in the current ssp 2.3 code) then things will change for you with this PR.

There are a few options for this:
(a) document things so folks with 2+ hosted IdP can explicitly set the links for each IdP entry
(b) if an entityid doesn't have a value then do not generate it but check for the value with a lookup without any entityid

For the longer story... Consider a metadata/saml20-idp-hosted.php as follows. I am only interested in the entityid and the SingleSignOnService property in the test added below.

<?php

declare(strict_types=1);

/*
 * Source of multiple hosted IdPs. Can be used to test scenarios of having multiple IdPs defined.
 */

$metadata['urn:x-simplesamlphp:example-idp-1'] = [
    'host' => '__DEFAULT__',
    'auth' => 'example-userpass',
    'SingleSignOnService' => 'https://idp.example.org/ssos1',
];

$metadata['urn:x-simplesamlphp:example-idp-2'] = [
    'host' => '__DEFAULT__',
    'auth' => 'example-userpass',
];

$metadata['urn:x-simplesamlphp:example-idp-3'] = [
    'host' => 'idp.example.org',
    'auth' => 'example-userpass',
    'SingleSignOnService' => 'https://idp.example.org/ssos',
    'SingleLogoutService' => 'https://idp.example.org/slos',
];

For demonstration purposes I have created a bin/test4.php file as follows:

#!/usr/bin/env php
<?php

require __DIR__ . '/../vendor/autoload.php';

use SimpleSAML\Metadata\MetaDataStorageHandler;
use SimpleSAML\Configuration;


$config = [
    'metadata.sources' => [
        ['type' => 'flatfile', 'directory' => __DIR__ . '/metadata/'],
    ],
];

Configuration::loadFromArray($config, '', 'simplesaml');
$h = MetaDataStorageHandler::getMetadataHandler();



echo "-----------\n idp-1 \n";
$v = $h->getGenerated( 'SingleSignOnService', 'saml20-idp-hosted', null, 'urn:x-simplesamlphp:example-idp-1' );
var_dump($v);

echo "-----------\n idp-2 \n";
$v = $h->getGenerated( 'SingleSignOnService', 'saml20-idp-hosted', null, 'urn:x-simplesamlphp:example-idp-2' );
var_dump($v);

echo "-----------\n idp-3 \n";
$v = $h->getGenerated( 'SingleSignOnService', 'saml20-idp-hosted', null, 'urn:x-simplesamlphp:example-idp-3' );
var_dump($v);

echo "-----------\n default \n";
$v = $h->getGenerated( 'SingleSignOnService', 'saml20-idp-hosted', null );
var_dump($v);

This gives the following... Notice that urn:x-simplesamlphp:example-idp-2 has a generated sign on link.

$ php bin/test4.php
-----------
 idp-1 
entityId  urn:x-simplesamlphp:example-idp-1  
string(29) "https://idp.example.org/ssos1"
-----------
 idp-2 
entityId  urn:x-simplesamlphp:example-idp-2  
PHP Warning:  Undefined array key "REQUEST_URI" in /.../src/SimpleSAML/Utils/HTTP.php on line 823
string(67) "http://localhost/simplesaml/module.php/saml/idp/singleSignOnService"
-----------
 idp-3 
entityId  urn:x-simplesamlphp:example-idp-3  
string(28) "https://idp.example.org/ssos"
-----------
 default 
entityId    
string(29) "https://idp.example.org/ssos1"
done

If I change MetaDataStorageHandler::getGenerated to remove the $entityId handling in the PR:

//            $metadataSet = $entityId ? $this->getMetaData($entityId, $set) : $this->getMetaDataCurrent($set);
            $metadataSet = $this->getMetaDataCurrent($set);

Then I get the following where idp-2 has a different SingleSignOnService value.

$ php bin/test4.php
-----------
 idp-1 
entityId  urn:x-simplesamlphp:example-idp-1  
string(29) "https://idp.example.org/ssos1"
-----------
 idp-2 
entityId  urn:x-simplesamlphp:example-idp-2  
string(29) "https://idp.example.org/ssos1"
-----------
 idp-3 
entityId  urn:x-simplesamlphp:example-idp-3  
string(29) "https://idp.example.org/ssos1"
-----------
 default 
entityId    
string(29) "https://idp.example.org/ssos1"
done

Ok, yes, this is not only "fix", since it may come as a surprise for those that have 2+ IdPs and have tried to override for example 'SingleSignOnService' but didn't notice that it actually wasn't overridden in non-default IdPs, since with this PR it will come into effect. So if they have something non-functional set in non-default IdPs, it will break :/.

So, should I point this to master branch, or 2.4? I'm more in favor to postpone this rather to introduce problems for people upgrading to 2.3.*, even if it is a low chance of mentioned scenarios...

I did a bunch of other testing and sniffing around. Playing with falling back to the value from $this->getMetaDataCurrent($set) if it is not explicitly set in the $entityId. I have moved that prose elsewhere (for now, it got quite long). I found that the fallback can make similar functionality to the old code at the expense of complexity.

My experiment was along the lines of updating MetaDataStorageHandler.php / getGenerated().

• explicit for $entityId (return if found)
• explicit for $this->getMetaDataCurrent($set), (return if found)
• then generated for $entityId.

Though it was mostly aimed at replicating the current results with a modification of this PR. Should we try to fallback to the getMetaDataCurrent($set)? That seems to allow the default values from the DEFAULT to be returned if not explicitly set on the $entityId.

Though this doesn't cover the case you mentioned that $entityId might have something non functional set and the old code would not have noticed that.

Should we try to fallback to the getMetaDataCurrent($set)? That seems to allow the default values from the DEFAULT to be returned if not explicitly set on the $entityId.

Since you got deep into it, I would say yes, definitely. The problem with 'something non functional set and not noticed' can be softened with releasing this in next version release and not 2.3, with upgrade docs updated... (I had something set and I didn't notice until this PR).

I guess looking at 2.4 as the rebase then.

The only remaining question is if we fallback to the DEFAULT if nothing is explicit in the $entityId. I am thinking it makes the most sense to do that.

I'm not fond of doing rebases with already shared code... This is small PR, but in general this can cause confusion and conflicts for anyone else who is working with it when trying to synchronize...

However, out of curiosity I've gone ahead with it and some conflicts arose that I don't feel comfortable resolving (only small one in docs/simplesamlphp-upgrade-notes-2.3.md, but still).

Care to do that yourself?

The change here seems to be $this->handler => $this->getHandler()

I have also cherry picked this over to master.

TODO: describe the difference mentioned in #2270 (comment) for 2.4.