Add note about Entity IDs and their validity check in upgrade docs for v2.0

[cicnavi]

While updating SimpleSAMLphp instance from v1.19 to v2.0.10, I have encountered an "...entity ID is not a valid RFC3986 compliant URI" error.

I don't see this noted in the upgrade docs, so here is a suggestion to add it.

But, it also seems to me that there is inconsistency in the check for the IdP part. For example, when I visit a federation overview page at ".../module.php/admin/federation", the hosted IdP with invalid entity ID was not listed, and in the logs I saw: "Federation: Error loading saml20-idp: 'some invalid entity id' is not a valid RFC3986 compliant URI. This check is done here:

simplesamlphp/modules/admin/src/Controller/Federation.php

Line 223 in 59a2c36

Assert::validURI($entity['entityid']);

However, the whole SSO authentication process using that IdP goes without any issues. The check for entity ID being valid URI is not done in WebBrowserSingleSignOn::singleSignOnService()

simplesamlphp/modules/saml/src/Controller/WebBrowserSingleSignOn.php

Line 132 in 59a2c36

$idpEntityId = $metadata->getMetaDataCurrentEntityID('saml20-idp-hosted');

Was this left out on purpose?

On the other hand, for any hosted SP, this check is done in SP constructor and you can't initiate authn if entity ID is not valid:

simplesamlphp/modules/saml/src/Auth/Source/SP.php

Line 100 in 59a2c36

Assert::validURI($entityId);

[monkeyiq]

It makes sense to me to validate the URI in all places.

[tvdijen]

I wouldn't say the one for WebSingleSignOn was left out on purpose, but I think we can be fairly sure that getMetaDataCurrentEntityID will produce a valid string.

Was this PR marked WIP on purpose, or can I just merge it?

[monkeyiq]

It seems the first paragraph is merged already. The second point "EntityIDs are now checked for validity..." remains to be merged. I am happy to split that out and merge it to 2.2+.

[cicnavi]

I have only added "EntityIDs are now checked for validity...". Other lines that are now "green" are from someone else / previous commits... :/.

Another thing that still bothers me is that in the admin UI there is no indication that something is wrong with the entity. The entity is simply not listed. I was thinking to add a kind of "alert" functionality in admin UI when something is wrong, if you would wait a bit more :$... Currently we only see this kind of error in logs.

[cicnavi]

I'll leave this as is for now (only add note that the entity ID is now checked for valid URI).

[tvdijen]

Closed in ff350e9