MDL-58692 auth: Improve the migration of auth setting names

Some auth plugins used to have a mix of the legacy style of plugin names
in config_plugins table (such as 'auth/mnet') and the new correct
style (such as 'auth_mnet'). Attempting to rename the setting plugin via
low level SQL UPDATE could lead to duplicate key violation.

The patch introduces a new helper function to safely migrate the old
settings to the new ones, eventually informing the admin about the
values mismatch.

diff --git a/auth/cas/db/upgrade.php b/auth/cas/db/upgrade.php
index 9de6515..dc0a265 100644 (file)
--- a/auth/cas/db/upgrade.php
+++ b/auth/cas/db/upgrade.php
@@ -61,7 +61,7 @@ function xmldb_auth_cas_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/cas to auth_cas.
-        $DB->set_field('config_plugins', 'plugin', 'auth_cas', array('plugin' => 'auth/cas'));
+        upgrade_fix_config_auth_plugin_names('cas');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'cas');
     }
 

diff --git a/auth/db/db/upgrade.php b/auth/db/db/upgrade.php
index 08db727..f40ea86 100644 (file)
--- a/auth/db/db/upgrade.php
+++ b/auth/db/db/upgrade.php
@@ -37,7 +37,7 @@ function xmldb_auth_db_upgrade($oldversion) {
 
     if ($oldversion < 2017032800) {
         // Convert info in config plugins from auth/db to auth_db
-        $DB->set_field('config_plugins', 'plugin', 'auth_db', array('plugin' => 'auth/db'));
+        upgrade_fix_config_auth_plugin_names('db');
         upgrade_plugin_savepoint(true, 2017032800, 'auth', 'db');
     }
 

diff --git a/auth/email/db/upgrade.php b/auth/email/db/upgrade.php
index 36d6d1f..f1b9ca7 100644 (file)
--- a/auth/email/db/upgrade.php
+++ b/auth/email/db/upgrade.php
@@ -37,10 +37,9 @@ function xmldb_auth_email_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/email to auth_email.
-        $DB->set_field('config_plugins', 'plugin', 'auth_email', array('plugin' => 'auth/email'));
+        upgrade_fix_config_auth_plugin_names('email');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'email');
     }
 
     return true;
 }
-

diff --git a/auth/fc/db/upgrade.php b/auth/fc/db/upgrade.php
index 6909e6c..4aefd46 100644 (file)
--- a/auth/fc/db/upgrade.php
+++ b/auth/fc/db/upgrade.php
@@ -37,7 +37,7 @@ function xmldb_auth_fc_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/fc to auth_fc.
-        $DB->set_field('config_plugins', 'plugin', 'auth_fc', array('plugin' => 'auth/fc'));
+        upgrade_fix_config_auth_plugin_names('fc');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'fc');
     }
 

diff --git a/auth/imap/db/upgrade.php b/auth/imap/db/upgrade.php
index 96dc3df..e89b077 100644 (file)
--- a/auth/imap/db/upgrade.php
+++ b/auth/imap/db/upgrade.php
@@ -37,10 +37,9 @@ function xmldb_auth_imap_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/imap to auth_imap.
-        $DB->set_field('config_plugins', 'plugin', 'auth_imap', array('plugin' => 'auth/imap'));
+        upgrade_fix_config_auth_plugin_names('imap');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'imap');
     }
 
     return true;
 }
-

diff --git a/auth/ldap/db/upgrade.php b/auth/ldap/db/upgrade.php
index e6c02cd..4b8bab0 100644 (file)
--- a/auth/ldap/db/upgrade.php
+++ b/auth/ldap/db/upgrade.php
@@ -61,7 +61,7 @@ function xmldb_auth_ldap_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/ldap to auth_ldap.
-        $DB->set_field('config_plugins', 'plugin', 'auth_ldap', array('plugin' => 'auth/ldap'));
+        upgrade_fix_config_auth_plugin_names('ldap');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'ldap');
     }
 

diff --git a/auth/manual/db/upgrade.php b/auth/manual/db/upgrade.php
index d9c6e3c..e0dc4a6 100644 (file)
--- a/auth/manual/db/upgrade.php
+++ b/auth/manual/db/upgrade.php
@@ -49,7 +49,7 @@ function xmldb_auth_manual_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/manual to auth_manual.
-        $DB->set_field('config_plugins', 'plugin', 'auth_manual', array('plugin' => 'auth/manual'));
+        upgrade_fix_config_auth_plugin_names('manual');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'manual');
     }
 

diff --git a/auth/mnet/db/upgrade.php b/auth/mnet/db/upgrade.php
index 5cccdeb..1aceca6 100644 (file)
--- a/auth/mnet/db/upgrade.php
+++ b/auth/mnet/db/upgrade.php
@@ -48,7 +48,7 @@ function xmldb_auth_mnet_upgrade($oldversion) {
     // Put any upgrade step following this.
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/mnet to auth_mnet.
-        $DB->set_field('config_plugins', 'plugin', 'auth_mnet', array('plugin' => 'auth/mnet'));
+        upgrade_fix_config_auth_plugin_names('mnet');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'mnet');
     }
 

diff --git a/auth/nntp/db/upgrade.php b/auth/nntp/db/upgrade.php
index 129d8e9..4fd893d 100644 (file)
--- a/auth/nntp/db/upgrade.php
+++ b/auth/nntp/db/upgrade.php
@@ -37,7 +37,7 @@ function xmldb_auth_nntp_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/nntp to auth_nntp.
-        $DB->set_field('config_plugins', 'plugin', 'auth_nntp', array('plugin' => 'auth/nntp'));
+        upgrade_fix_config_auth_plugin_names('nntp');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'nntp');
     }
 

diff --git a/auth/none/db/upgrade.php b/auth/none/db/upgrade.php
index d54035e..2f858cb 100644 (file)
--- a/auth/none/db/upgrade.php
+++ b/auth/none/db/upgrade.php
@@ -37,10 +37,9 @@ function xmldb_auth_none_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/none to auth_none.
-        $DB->set_field('config_plugins', 'plugin', 'auth_none', array('plugin' => 'auth/none'));
+        upgrade_fix_config_auth_plugin_names('none');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'none');
     }
 
     return true;
 }
-

diff --git a/auth/pam/db/upgrade.php b/auth/pam/db/upgrade.php
index 3bff038..480fe39 100644 (file)
--- a/auth/pam/db/upgrade.php
+++ b/auth/pam/db/upgrade.php
@@ -37,7 +37,7 @@ function xmldb_auth_pam_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/pam to auth_pam.
-        $DB->set_field('config_plugins', 'plugin', 'auth_pam', array('plugin' => 'auth/pam'));
+        upgrade_fix_config_auth_plugin_names('pam');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'pam');
     }
 

diff --git a/auth/pop3/db/upgrade.php b/auth/pop3/db/upgrade.php
index 702fe51..ea91a44 100644 (file)
--- a/auth/pop3/db/upgrade.php
+++ b/auth/pop3/db/upgrade.php
@@ -37,7 +37,7 @@ function xmldb_auth_pop3_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/pop3 to auth_pop3.
-        $DB->set_field('config_plugins', 'plugin', 'auth_pop3', array('plugin' => 'auth/pop3'));
+        upgrade_fix_config_auth_plugin_names('pop3');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'pop3');
     }
 

diff --git a/auth/shibboleth/db/upgrade.php b/auth/shibboleth/db/upgrade.php
index 8b95151..3bd2d13 100644 (file)
--- a/auth/shibboleth/db/upgrade.php
+++ b/auth/shibboleth/db/upgrade.php
@@ -37,7 +37,7 @@ function xmldb_auth_shibboleth_upgrade($oldversion) {
 
     if ($oldversion < 2017020700) {
         // Convert info in config plugins from auth/shibboleth to auth_shibboleth.
-        $DB->set_field('config_plugins', 'plugin', 'auth_shibboleth', array('plugin' => 'auth/shibboleth'));
+        upgrade_fix_config_auth_plugin_names('shibboleth');
         upgrade_plugin_savepoint(true, 2017020700, 'auth', 'shibboleth');
     }
 

diff --git a/auth/upgrade.txt b/auth/upgrade.txt
index def7fa2..1a3a50a 100644 (file)
--- a/auth/upgrade.txt
+++ b/auth/upgrade.txt
@@ -5,6 +5,8 @@ information provided here is intended especially for developers.
 
 * Authentication plugins have been migrated to use the admin settings API.
   Plugins should use a settings.php file to manage configurations rather than using the config.html files.
+  See how the helper function upgrade_fix_config_auth_plugin_names() can be used to convert the legacy settings to the
+  new ones.
 * The function 'print_auth_lock_options' has been replaced by 'display_auth_lock_options' which uses the admin settings API.
   See auth_manual as an exmple of how it can be used.  More information can be found in MDL-12689.
 * The list of supported identity providers (SSO IdP) returned by the 'loginpage_idp_list' method (used to render the

diff --git a/lang/en/auth.php b/lang/en/auth.php
index c289b3b..2dbef7e 100644 (file)
--- a/lang/en/auth.php
+++ b/lang/en/auth.php
@@ -144,6 +144,7 @@ $string['recaptcha_link'] = 'auth/email';
 $string['security_question'] = 'Security question';
 $string['selfregistration'] = 'Self registration';
 $string['selfregistration_help'] = 'If an authentication plugin, such as email-based self-registration, is selected, then it enables potential users to register themselves and create accounts. This results in the possibility of spammers creating accounts in order to use forum posts, blog entries etc. for spam. To avoid this risk, self-registration should be disabled or limited by <em>Allowed email domains</em> setting.';
+$string['settingmigrationmismatch'] = 'Values mismatch detected while correcting the plugin setting names! The authentication plugin \'{$a->plugin}\' had the setting \'{$a->setting}\' configured to \'{$a->legacy}\' under the legacy name and to \'{$a->current}\' under the current name. The latter value has been set as the valid one but you should check and confirm that it is expected.';
 $string['sha1'] = 'SHA-1 hash';
 $string['showguestlogin'] = 'You can hide or show the guest login button on the login page.';
 $string['stdchangepassword'] = 'Use standard page for changing password';

diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php
index 275323b..dd684ad 100644 (file)
--- a/lib/tests/upgradelib_test.php
+++ b/lib/tests/upgradelib_test.php
@@ -925,4 +925,55 @@ class core_upgradelib_testcase extends advanced_testcase {
         $this->assertEquals(count($blockinstances), $DB->count_records('block_positions', ['subpage' => $page1->id, 'pagetype' => 'my-index', 'contextid' => $context1->id]));
         $this->assertEquals(0, $DB->count_records('block_positions', ['subpage' => $page2->id, 'pagetype' => 'my-index']));
     }
+
+    /**
+     * Test the conversion of auth plugin settings names.
+     */
+    public function test_upgrade_fix_config_auth_plugin_names() {
+        $this->resetAfterTest();
+
+        // Let the plugin auth_foo use legacy format only.
+        set_config('name1', 'val1', 'auth/foo');
+        set_config('name2', 'val2', 'auth/foo');
+
+        // Let the plugin auth_bar use new format only.
+        set_config('name1', 'val1', 'auth_bar');
+        set_config('name2', 'val2', 'auth_bar');
+
+        // Let the plugin auth_baz use a mix of legacy and new format, with no conflicts.
+        set_config('name1', 'val1', 'auth_baz');
+        set_config('name1', 'val1', 'auth/baz');
+        set_config('name2', 'val2', 'auth/baz');
+        set_config('name3', 'val3', 'auth_baz');
+
+        // Let the plugin auth_qux use a mix of legacy and new format, with conflicts.
+        set_config('name1', 'val1', 'auth_qux');
+        set_config('name1', 'val2', 'auth/qux');
+
+        // Execute the migration.
+        upgrade_fix_config_auth_plugin_names('foo');
+        upgrade_fix_config_auth_plugin_names('bar');
+        upgrade_fix_config_auth_plugin_names('baz');
+        upgrade_fix_config_auth_plugin_names('qux');
+
+        // Assert that legacy settings are gone and no new were introduced.
+        $this->assertEmpty((array) get_config('auth/foo'));
+        $this->assertEmpty((array) get_config('auth/bar'));
+        $this->assertEmpty((array) get_config('auth/baz'));
+        $this->assertEmpty((array) get_config('auth/qux'));
+
+        // Assert values were simply kept where there was no conflict.
+        $this->assertSame('val1', get_config('auth_foo', 'name1'));
+        $this->assertSame('val2', get_config('auth_foo', 'name2'));
+
+        $this->assertSame('val1', get_config('auth_bar', 'name1'));
+        $this->assertSame('val2', get_config('auth_bar', 'name2'));
+
+        $this->assertSame('val1', get_config('auth_baz', 'name1'));
+        $this->assertSame('val2', get_config('auth_baz', 'name2'));
+        $this->assertSame('val3', get_config('auth_baz', 'name3'));
+
+        // Assert the new format took precedence in case of conflict.
+        $this->assertSame('val1', get_config('auth_qux', 'name1'));
+    }
 }

diff --git a/lib/upgradelib.php b/lib/upgradelib.php
index 2cdd6eb..12c7a6e 100644 (file)
--- a/lib/upgradelib.php
+++ b/lib/upgradelib.php
@@ -2516,3 +2516,56 @@ function check_libcurl_version(environment_results $result) {
 
     return null;
 }
+
+/**
+ * Fix how auth plugins are called in the 'config_plugins' table.
+ *
+ * For legacy reasons, the auth plugins did not always use their frankenstyle
+ * component name in the 'plugin' column of the 'config_plugins' table. This is
+ * a helper function to correctly migrate the legacy settings into the expected
+ * and consistent way.
+ *
+ * @param string $plugin the auth plugin name such as 'cas', 'manual' or 'mnet'
+ */
+function upgrade_fix_config_auth_plugin_names($plugin) {
+    global $CFG, $DB, $OUTPUT;
+
+    $legacy = (array) get_config('auth/'.$plugin);
+    $current = (array) get_config('auth_'.$plugin);
+
+    // I don't want to rely on array_merge() and friends here just in case
+    // there was some crazy setting with a numerical name.
+
+    if ($legacy) {
+        $new = $legacy;
+    } else {
+        $new = [];
+    }
+
+    if ($current) {
+        foreach ($current as $name => $value) {
+            if (isset($legacy[$name]) && ($legacy[$name] !== $value)) {
+                // No need to pollute the output during unit tests.
+                if (!empty($CFG->upgraderunning)) {
+                    $message = get_string('settingmigrationmismatch', 'core_auth', [
+                        'plugin' => 'auth_'.$plugin,
+                        'setting' => s($name),
+                        'legacy' => s($legacy[$name]),
+                        'current' => s($value),
+                    ]);
+                    echo $OUTPUT->notification($message, \core\output\notification::NOTIFY_ERROR);
+
+                    upgrade_log(UPGRADE_LOG_NOTICE, 'auth_'.$plugin, 'Setting values mismatch detected',
+                        'SETTING: '.$name. ' LEGACY: '.$legacy[$name].' CURRENT: '.$value);
+                }
+            }
+
+            $new[$name] = $value;
+        }
+    }
+
+    foreach ($new as $name => $value) {
+        set_config($name, $value, 'auth_'.$plugin);
+        unset_config($name, 'auth/'.$plugin);
+    }
+}