auth saml config form does not work

[Rich Trott]

Trying to use the config form for the auth:saml plugin/extension results in the form submission hanging and this appears in the error log:

PHP Fatal error: Cannot use object of type Pieform as array in /web/mahara/htdocs/auth/saml/lib.php on line 461, referer: https://example.com/admin/extensions/pluginconfig.php?plugintype=auth&pluginname=saml

I'm using a checkout of the current Master branch, 1.4.0dev. This problem does not appear to be in the release branches. I'm running Linux (RedHat CentOS 5) + Postgres.

[Richard Mansfield (richard-mansfield)]

This seems like a bad bug. It looks like Mahara forces auth plugins to use the same function names (validate_config_options, save_config_options) for plugin configuration and instance configuration, and then passes different parameters to the function depending on whether it's doing plugin or instance config. It only affects saml, because that's the only auth plugin that has plugin configuration.

To fix it without breaking any contributed plugins out ther, maybe we should make the auth instance config stuff in htdocs/admin/users/addauthority.php check for the existence of validate_instance_config_options, save_instance_config_options functions and if they exist, use those instead of validate_config_options, save_config_options.

Or else just change the names of all auth plugins to use validate_instance_config_options etc., and just let any contrib auth plugins break.

[Rich Trott (richard-trott)]

Here's a patch that will let SAML configuration work whether it receives a Pieform object (which is what triggers this bug) or an array (which it apparently still needs to accept in other situations).

I think this resolves the narrow buggy behavior in SAML reported here. I guess the larger root-cause bug you (Richard M.) refer to above should probably be filed as a separate bug?

[Rich Trott (richard-trott)]

Uh, yeah, let's try that again, but this time I'll make sure my code uses Unix line-endings before making the patch....

[Rich Trott (richard-trott)]

It looks like this bug was introduced by the commit done to fix bug #653839. https://bugs.launchpad.net/mahara/+bug/653839

That patch removed code that does a very similar thing that the patch I submitted here does, except that code produces errors/warnings because it tries to use get_class on a variable that may or may not be an object. The patch I submitted uses instanceof, so it doesn't generate errors/warnings if the variable turns out to be an array rather than an object.

Perhaps a hybrid of my patch and the original code would be best? Attached.

[Rich Trott (richard-trott)]

Looks like I goofed up the whitespace on that patch, per usual, but other than that... Anything I should do to expedite this? SAML cannot be configured (and thus cannot be used) without this patch or a similar fix.

[Rich Trott (richard-trott)]

If it helps at all, here's the patch with the whitespace that shouldn't be there removed.

[PiersHarding (piersharding)]

Hi Richard -

Thanks for your patch. I've applied it and pushed to master.

Cheers,
Piers Harding.