Settings page exception on save after clean installation

[ebithell]

This may be connected to #652, because get_file_idp_metadata_file is also affecting the end-to-end flow of clean installation of this plugin. Using HEAD of the MOODLE_39_STABLE branch at commit #858a056f, I can install the plugin and proceed through the installation as far as the plugin settings page but saving that page without changing any default values causes an exception:

Exception - Call to a member function get_file_sp_metadata_file() on null
Debug info:
Error code: generalexceptionmessage
Stack trace:
line 284 of /auth/saml2/locallib.php: Error thrown
line 2051 of /lib/adminlib.php: call to auth_saml2_update_sp_metadata()
line 8968 of /lib/adminlib.php: call to admin_setting->post_write_settings()
line 26 of /admin/upgradesettings.php: call to admin_write_settings()

This seems to be because /var/moodledata/saml2 has not been populated with .cert, .pem and .xml files at this point, so auth/saml2/setup.php fails at the try/catch. Requesting an XML download populates the directory with those files, but this is not possible during the install process. It seems to me that it should be possible to install the plugin without configuring it at the outset. If it does need to be configured at installation time, a note on the documentation page to say so would be a huge help!

[ebithell]

Also of note: ignoring the error and continuing does complete installation of the plugin correctly and it can be configured post-install. My concern is that if the plugin is installed at the same time as anything else (e.g. during a Moodle version upgrade), the throwing of the exception could have an unknown and unpredictable effect on saving settings for other plugins.

[danmarsden]

What Moodle version are you trying to install this in? - install is also covered pretty well by our automated tests so I'm surprised that hasn't been picked up in the CI tests

[ebithell]

It is happening in Moodle 3.9. To clarify, the plugin does install correctly. What I can't do is save the plugin settings page when it is first presented during the upgrade workflow. Steps to reproduce are:

1. Complete clean install of Moodle using MOODLE_39_STABLE branch.
2. Git clone SAML2 plugin into auth directory.
3. Run Moodle upgrade process: environment check OK; plugins check OK; plugin installation OK; new settings page loads.
4. No changes made to settings, accept defaults or leave blank.
5. Attempt to save new settings fails with the error and stack trace quoted above.

But then:

1. Navigate back to the plugin settings page via Site administration > Plugins > Authentication > Manage authentication > SAML2 settings.
2. Make no changes: saving settings now succeeds.

I get the same result both on localhost, and also on an outward-facing evaluation server that is set up for SSL.

It is possible that I have misunderstood something fundamental about how to install this plugin. Is it the case that Step 3 of https://github.com/catalyst/moodle-auth_saml2/blob/MOODLE_39_STABLE/README.md#installation is a mandatory part of the initial settings, even if the plugin is to remain otherwise unconfigured and not enabled?

[brendanheywood]

I have a hunch this is a regression caused by https://github.com/catalyst/moodle-auth_saml2/issues/616

[cwarwicker]

Hi, I've picked up an internal ticket for this in WRMS. I've had a look at the plugin code, but I'm hesitant to apply a fix, because however I do it, will probably end up not being the way you guys would want it.

Do you have any thoughts on the best way you would like this fixed? My initial thought was to see if the setup.php file can be required upon submission of the settings, but don't know if that is how you'd want it to work or not.

Conn

[brendanheywood]

I think just use your best judgement and we'll evolve it once we see some proposed code

[ebithell]

A good outcome would be to enable the plugin to be cleanly installed with default settings and nothing else. That would match the expected behaviour for plugins generally, even if the functionality cannot then be used without additional configuration.

[danmarsden]

should be sorted with the latest version - thanks!