Commit 11bcc0d0 authored by Richard Mansfield's avatar Richard Mansfield
Browse files

Fix method names in auth plugin and instance configuration (bug #933937)



The validation/submission methods called during auth *plugin*
configuration and auth *instance* configuration have the same names,
but in each of these cases, the methods deal with completely different
data, and are passed arguments in a different order.

This is a problem for the saml plugin, which needs to detect where
it's being called from, and reverse the order of the arguments if
necessary.

To fix this, the new methods validate_instance_config_options and
save_instance_config_options are called during instance configuration,
and the names 'validate_config_options' and 'save_config_options' are
left for global plugin configuration, as these are the names used for
all other plugin types.

The old names are still usable in case any third party auth plugins
are relying on them, but they'll produce a warning when called during
instance configuration.

Change-Id: I996ed7a2cdbe0e50dcb0c88560d10a2044b4e21c
Signed-off-by: default avatarRichard Mansfield <richard.mansfield@catalyst.net.nz>
parent 65c27b46
......@@ -114,13 +114,20 @@ function auth_config_validate(Pieform $form, $values) {
$plugin = $values['authname'];
$classname = 'PluginAuth' . ucfirst(strtolower($plugin));
if (!method_exists($classname, 'validate_config_options')) {
if (method_exists($classname, 'validate_instance_config_options')) {
$method = 'validate_instance_config_options';
}
else if (method_exists($classname, 'validate_config_options')) {
$method = 'validate_config_options';
log_warn("The validate_config_options method is being called during $plugin auth instance configuration. In future this won't work: the plugin should define the validate_instance_config_options method instead.");
}
if (!isset($method)) {
return;
}
safe_require('auth', strtolower($plugin));
try {
$values = call_static_method($classname, 'validate_config_options', $values, $form);
$values = call_static_method($classname, $method, $values, $form);
} catch (Exception $e) {
if (!$form->has_errors()) {
$form->set_error('instancename', "An unknown error occurred while processing this form");
......@@ -132,9 +139,21 @@ function auth_config_submit(Pieform $form, $values) {
global $SESSION;
$plugin = $values['authname'];
$classname = 'PluginAuth' . ucfirst(strtolower($plugin));
if (method_exists($classname, 'save_instance_config_options')) {
$method = 'save_instance_config_options';
}
else if (method_exists($classname, 'save_config_options')) {
$method = 'save_config_options';
log_warn("The save_config_options method is being called during $plugin auth instance configuration. In future this won't work: the plugin should define the save_instance_config_options method instead.");
}
if (!isset($method)) {
return;
}
safe_require('auth', strtolower($plugin));
try {
$values = call_static_method($classname, 'save_config_options', $values, $form);
$values = call_static_method($classname, $method, $values, $form);
} catch (Exception $e) {
log_info($e->getMessage());
log_info($e->getTrace());
......
......@@ -159,7 +159,7 @@ class PluginAuthBrowserid extends PluginAuth {
);
}
public static function save_config_options($values, $form) {
public static function save_instance_config_options($values, $form) {
$authinstance = new stdClass();
......
......@@ -230,7 +230,7 @@ class PluginAuthImap extends PluginAuth {
);
}
public static function save_config_options($values, $form) {
public static function save_instance_config_options($values, $form) {
$authinstance = new stdClass();
......
......@@ -658,7 +658,7 @@ class PluginAuthLdap extends PluginAuth {
);
}
public static function save_config_options($values, $form) {
public static function save_instance_config_options($values, $form) {
$authinstance = new stdClass();
......
......@@ -476,33 +476,25 @@ class PluginAuthSaml extends PluginAuth {
);
}
public static function validate_config_options($values, $form) {
if ($values instanceof Pieform) {
$tmp = $form;
$form = $values;
$values = $tmp;
public static function validate_config_options(Pieform $form, $values) {
// SimpleSAMLPHP lib directory must have right things
if (!file_exists($values['simplesamlphplib'] . '/lib/_autoload.php')) {
$form->set_error('simplesamlphplib', get_string('errorbadlib', 'auth.saml', $values['simplesamlphplib']));
}
// fix problems with config validation interface incorrect between site/institution
if (isset($values['authglobalconfig'])) {
// SimpleSAMLPHP lib directory must have right things
if (! file_exists($values['simplesamlphplib'].'/lib/_autoload.php')) {
$form->set_error('simplesamlphplib', get_string('errorbadlib', 'auth.saml', $values['simplesamlphplib']));
}
// SimpleSAMLPHP config directory must shape up
if (! file_exists($values['simplesamlphpconfig'].'/config.php')) {
$form->set_error('simplesamlphpconfig', get_string('errorbadconfig', 'auth.saml', $values['simplesamlphpconfig']));
}
// SimpleSAMLPHP config directory must shape up
if (!file_exists($values['simplesamlphpconfig'] . '/config.php')) {
$form->set_error('simplesamlphpconfig', get_string('errorbadconfig', 'auth.saml', $values['simplesamlphpconfig']));
}
}
public static function validate_instance_config_options($values, $form) {
// only allow remoteuser to be unset if usersuniquebyusername is NOT set
if (isset($values['remoteuser']) && !get_config('usersuniquebyusername') && !$values['remoteuser']) {
if (!get_config('usersuniquebyusername') && !$values['remoteuser']) {
$form->set_error('remoteuser', get_string('errorremoteuser', 'auth.saml'));
}
if (isset($values['weautocreateusers'])) {
if ($values['weautocreateusers'] && $values['remoteuser']) {
$form->set_error('weautocreateusers', get_string('errorbadcombo', 'auth.saml'));
}
if ($values['weautocreateusers'] && $values['remoteuser']) {
$form->set_error('weautocreateusers', get_string('errorbadcombo', 'auth.saml'));
}
$dup = get_records_sql_array('SELECT COUNT(instance) AS instance FROM {auth_instance_config}
WHERE ((field = \'institutionattribute\' AND value = ?) OR
......@@ -520,78 +512,78 @@ class PluginAuthSaml extends PluginAuth {
}
}
public static function save_config_options($values) {
$configs = array('simplesamlphplib', 'simplesamlphpconfig');
foreach ($configs as $config) {
set_config_plugin('auth', 'saml', $config, $values[$config]);
}
}
public static function save_config_options($values, $form) {
public static function save_instance_config_options($values, $form) {
$configs = array('simplesamlphplib', 'simplesamlphpconfig');
$authinstance = new stdClass();
if (isset($values['authglobalconfig'])) {
foreach ($configs as $config) {
set_config_plugin('auth', 'saml', $config, $values[$config]);
}
if ($values['instance'] > 0) {
$values['create'] = false;
$current = get_records_assoc('auth_instance_config', 'instance', $values['instance'], '', 'field, value');
$authinstance->id = $values['instance'];
}
else {
$authinstance = new stdClass();
$values['create'] = true;
$lastinstance = get_records_array('auth_instance', 'institution', $values['institution'], 'priority DESC', '*', '0', '1');
if ($values['instance'] > 0) {
$values['create'] = false;
$current = get_records_assoc('auth_instance_config', 'instance', $values['instance'], '', 'field, value');
$authinstance->id = $values['instance'];
if ($lastinstance == false) {
$authinstance->priority = 0;
}
else {
$values['create'] = true;
$lastinstance = get_records_array('auth_instance', 'institution', $values['institution'], 'priority DESC', '*', '0', '1');
if ($lastinstance == false) {
$authinstance->priority = 0;
}
else {
$authinstance->priority = $lastinstance[0]->priority + 1;
}
$authinstance->priority = $lastinstance[0]->priority + 1;
}
}
$authinstance->institution = $values['institution'];
$authinstance->authname = $values['authname'];
$authinstance->instancename = $values['authname'];
$authinstance->institution = $values['institution'];
$authinstance->authname = $values['authname'];
$authinstance->instancename = $values['authname'];
if ($values['create']) {
$values['instance'] = insert_record('auth_instance', $authinstance, 'id', true);
}
else {
update_record('auth_instance', $authinstance, array('id' => $values['instance']));
}
if ($values['create']) {
$values['instance'] = insert_record('auth_instance', $authinstance, 'id', true);
}
else {
update_record('auth_instance', $authinstance, array('id' => $values['instance']));
}
if (empty($current)) {
$current = array();
}
if (empty($current)) {
$current = array();
}
self::$default_config = array('user_attribute' => $values['user_attribute'],
'weautocreateusers' => $values['weautocreateusers'],
'loginlink' => $values['loginlink'],
'remoteuser' => $values['remoteuser'],
'firstnamefield' => $values['firstnamefield'],
'surnamefield' => $values['surnamefield'],
'emailfield' => $values['emailfield'],
'updateuserinfoonlogin' => $values['updateuserinfoonlogin'],
'institutionattribute' => $values['institutionattribute'],
'institutionvalue' => $values['institutionvalue'],
'institutionregex' => $values['institutionregex'],
);
foreach(self::$default_config as $field => $value) {
$record = new stdClass();
$record->instance = $values['instance'];
$record->field = $field;
$record->value = $value;
if ($values['create'] || !array_key_exists($field, $current)) {
insert_record('auth_instance_config', $record);
}
else {
update_record('auth_instance_config', $record, array('instance' => $values['instance'], 'field' => $field));
}
self::$default_config = array(
'user_attribute' => $values['user_attribute'],
'weautocreateusers' => $values['weautocreateusers'],
'loginlink' => $values['loginlink'],
'remoteuser' => $values['remoteuser'],
'firstnamefield' => $values['firstnamefield'],
'surnamefield' => $values['surnamefield'],
'emailfield' => $values['emailfield'],
'updateuserinfoonlogin' => $values['updateuserinfoonlogin'],
'institutionattribute' => $values['institutionattribute'],
'institutionvalue' => $values['institutionvalue'],
'institutionregex' => $values['institutionregex'],
);
foreach(self::$default_config as $field => $value) {
$record = new stdClass();
$record->instance = $values['instance'];
$record->field = $field;
$record->value = $value;
if ($values['create'] || !array_key_exists($field, $current)) {
insert_record('auth_instance_config', $record);
}
else {
update_record('auth_instance_config', $record, array('instance' => $values['instance'], 'field' => $field));
}
}
$configs = array('simplesamlphplib', 'simplesamlphpconfig');
foreach ($configs as $config) {
self::$default_config[$config] = get_config_plugin('auth', 'saml', $config);
}
......
......@@ -862,7 +862,7 @@ class PluginAuthXmlrpc extends PluginAuth {
);
}
public static function validate_config_options($values, $form) {
public static function validate_instance_config_options($values, $form) {
if (false === strpos($values['wwwroot'], '://')) {
$values['wwwroot'] = 'http://' . $values['wwwroot'];
}
......@@ -902,7 +902,7 @@ class PluginAuthXmlrpc extends PluginAuth {
//TODO: test values and set appropriate errors on form
}
public static function save_config_options($values, $form) {
public static function save_instance_config_options($values, $form) {
if (false === strpos($values['wwwroot'], '://')) {
$values['wwwroot'] = 'http://' . $values['wwwroot'];
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment