Issue768 extlib upgrade #774

sarahjcotton · 2023-10-03T10:56:46Z

Fix for issue #768

Some re-factoring was required in order to make the plugin compatible with simplesamlphp 2.0 as per the docs: https://simplesamlphp.org/docs/2.0/simplesamlphp-upgrade-notes-2.0.html

In addition, a new BINDING_HTTP_ARTIFACT const seems to cause artifact to always take priority instead of redirect. Removal rather than re-order might be preferable here but I've left it in as there are settings in the plugin that refer to artifact bindings, although I'm not sure sure how to configure and test if the two are related. I couldn't see an obvious way to achieve this programmatically, unfortunately.

A number of unit tests are skipped when I run locally due to Redis and the Prophecy library not being available, again, not sure how to test.

Otherwise, @Peterburnett has kindly offered to peer review this. Happy to make any further changes as per your guidance.

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

.extlib/simplesamlphp/src/SimpleSAML/Utils/HTTP.php

classes/store.php

db/upgrade.php

sp/saml2-acs.php

sp/saml2-logout.php

db/upgrade.php

Peterburnett · 2023-10-04T00:06:18Z

Hi @sarahjcotton

All is largely looking good. Couple of comments could help to explain some stuff, and I think that there are a bunch of asserts in place that we dont need anymore as they are superseded by type guards on the parameters. Additionally there is a small syntax issue causing CI fails that ive put a comment on.

Other than that, I've seen this working with keycloak, so I think this is good to start rolling out. Anything else will have to be found in whatever complex integrations surface the issues.

extlib-simplesaml: backport mods to extlib. 3063297 Added support for IdP initiation flow catalyst#105 6ebf501 Fixed bug with Idp initiatied entityid ca34393 Edited 04/10/23 for peer review of pull catalyst#774

…uthnRequest()

https://simplesamlphp.org/docs/2.0/simplesamlphp-upgrade-notes-2.0.html

sarahjcotton · 2023-10-04T10:39:47Z

Thanks so much Peter; I've addressed all the issues you flagged if you wouldn't mind reviewing those, however whilst trying to run some unit tests yesterday I set the $CFG vars up for Redis and on trying to login using SAML today a bunch of new errors have been thrown that need fixing, so marking this PR as draft for now as it's not ready to be merged.

sarahjcotton · 2023-10-04T11:42:43Z

New commit added to fix the redis_store class. I then also managed to get the redis unit tests to run locally and they all passed:

./vendor/bin/phpunit --verbose --filter auth_saml2_redis_store_testcase
Runtime:       PHP 8.0.29
Configuration: /var/www/moodle41/phpunit.xml

.......                                                             7 / 7 (100%)

Time: 00:01.598, Memory: 324.00 MB

OK (7 tests, 7 assertions)

Peterburnett · 2023-10-04T22:23:46Z

Hi @sarahjcotton

This is looking much better now, things are looking really clean. The CI has exposed something interesting, in that PHP 7.4 is now the minimum PHP version supported. All 7 versions are EOL, so I dont think its a major issue to set the minimum to 7.4, but we should update the README table to reflect that, and additionally update the CI grid so that PHP 7.2 and 7.3 are excluded (see https://github.com/catalyst/catalyst-moodle-workflows#with-options)

I think thats the last thing before we can land this one.

danmarsden · 2023-10-05T07:53:15Z

Great work Sarah!!

+1 to merge after

update readme to state that php 7.4 and higher is required on the 3.9 stable branch as Peter mentioned.
add the min_php setting to the github actions workflow config to only run 7.4 and higher tests (see: https://github.com/catalyst/catalyst-moodle-workflows/#with-options)

sarahjcotton · 2023-10-05T10:33:06Z

Thanks both, hopefully that's good to go now!

cameron1729 · 2023-10-06T08:03:16Z

locallib.php

@@ -57,16 +57,16 @@ function auth_saml2_get_sp_metadata($baseurl = '') {

    $entityId = $source->getEntityId();
    $spconfig = $source->getMetadata();
-    $store = SimpleSAML\Store::getInstance();
+    \SimpleSAML\Store\StoreFactory::getInstance('\\auth_saml2\\store');


A bit confused by this. It doesn't seem that $store was ever used. So could we not just delete this line?

extlib-simplesaml: backport mods to extlib. 3063297 Added support for IdP initiation flow #105 6ebf501 Fixed bug with Idp initiatied entityid ca34393 Edited 04/10/23 for peer review of pull #774

Issue catalyst#768 - Update simplesamlphp to version 2.0.5

b890997

sarahjcotton force-pushed the issue768_extlib-upgrade branch 3 times, most recently from a260e4d to 1017990 Compare October 3, 2023 15:09