search

catalyst / moodle-auth_saml2

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

Debugging messages on save after clean installation #698

Open ebithell opened 2 years ago

ebithell commented 2 years ago

Connected with #656. Please excuse the copy/paste from the initial description of that issue, but some of the symptoms are the same, as is my interpretation of the cause.

Using HEAD of the MOODLE_39_STABLE branch at commit #16ae37b2, 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 debugging warnings which interrupt the settings save process:

= Missing cert pem file! = = Missing cert crt file! = Now regenerating saml2 certificates... line 57 of /auth/saml2/setup.php: call to debugging() line 283 of /auth/saml2/locallib.php: call to require_once() 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 only occurs on when the plugin has never been installed before, and happens because the .cert, .pem and .xml files do not exist yet, so auth/saml2/setup.php emits a debugging message at the try/catch.

Installing the plugin with debugging disabled does of course resolve the issue, but the purpose of these messages is to provide information when something goes wrong so my feeling is that a clean installation should be possible with debugging enabled. A nice solution would be for the catch to detect whether this is a first installation or a return visit to the settings page. Is this possible?

cwarwicker commented 2 years ago

I think simply removing the debugging message is probably the best solution. It's not really required, unless there is an error creating the directory or files.

ebithell commented 2 years ago

@cwarwicker having looked at what this section of code does, I agree with you. I don't think the debugging adds much at this point. The default directory is under /var/moodledata, so if there is an issue with creating it there are likely to be other problems as well and I'm not sure I see how this condition could arise on a normally functioning instance of Moodle.

danmarsden commented 2 years ago

I do think there's some value there - we used to just write these direct to the error_log rather than output to screen using debugging calls - but @kabalin converted those error_log calls to debugging ones. I suspect we should just modify it to rely on the auth_saml2 debug setting and only trigger when debugging is turned on - probably set it to use Developer level too.

ebithell commented 2 years ago

@danmarsden that sounds like a good clean solution. I'd definitely be happy to see that implemented!