search

catalyst / moodle-auth_saml2

SAML done 100% in Moodle, fast, simple, secure
https://moodle.org/plugins/auth_saml2
70 stars 132 forks source link

Issue768 extlib upgrade #774

Closed sarahjcotton closed 9 months ago

sarahjcotton commented 9 months ago

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.

Peterburnett commented 9 months ago

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.

sarahjcotton commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Great work Sarah!!

+1 to merge after 1) update readme to state that php 7.4 and higher is required on the 3.9 stable branch as Peter mentioned. 2) 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 commented 9 months ago

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