From 8d7b8f99bd930a11128ccb59205d8ceeef8ece11 Mon Sep 17 00:00:00 2001 From: David Woloszyn Date: Tue, 17 Dec 2024 18:00:41 +1100 Subject: [PATCH] MDL-79958 factor_email: Email factor is enabled by default --- admin/tool/mfa/factor/email/db/install.php | 31 ++++++++++ admin/tool/mfa/factor/email/db/upgrade.php | 57 +++++++++++++++++++ admin/tool/mfa/factor/email/settings.php | 2 +- admin/tool/mfa/factor/email/version.php | 2 +- .../mfa/factor/grace/tests/factor_test.php | 3 + .../sms/tests/behat/factor_sms_login.feature | 2 + .../tests/admin_setting_managemfa_test.php | 12 +++- .../tool_mfa_factor_management_table.feature | 4 ++ ..._mfa_setup_and_manage_user_factors.feature | 2 + admin/tool/mfa/tests/manager_test.php | 5 ++ .../tool/mfa/tests/plugininfo_factor_test.php | 13 ++--- 11 files changed, 120 insertions(+), 13 deletions(-) create mode 100644 admin/tool/mfa/factor/email/db/install.php create mode 100644 admin/tool/mfa/factor/email/db/upgrade.php diff --git a/admin/tool/mfa/factor/email/db/install.php b/admin/tool/mfa/factor/email/db/install.php new file mode 100644 index 00000000000..7369c0abc6c --- /dev/null +++ b/admin/tool/mfa/factor/email/db/install.php @@ -0,0 +1,31 @@ +. + +/** + * Installation code for factor_email. + * + * @package factor_email + * @copyright 2024 David Woloszyn + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +/** + * Perform install procedures for factor_email. + */ +function xmldb_factor_email_install(): void { + // The factor_email should be enabled by default. Just ensure it is ordered too. + set_config('factor_order', 'email', 'tool_mfa'); +} diff --git a/admin/tool/mfa/factor/email/db/upgrade.php b/admin/tool/mfa/factor/email/db/upgrade.php new file mode 100644 index 00000000000..2fe2351e6a5 --- /dev/null +++ b/admin/tool/mfa/factor/email/db/upgrade.php @@ -0,0 +1,57 @@ +. + +/** + * factor_email upgrade library. + * + * @package factor_email + * @copyright 2024 David Woloszyn + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +/** + * MFA upgrade helper function. + * + * @param int $oldversion + */ +function xmldb_factor_email_upgrade($oldversion): bool { + if ($oldversion < 2024122400) { + // Check for sites that don't have MFA enabled. + if (!get_config('tool_mfa', 'enabled')) { + // Enable email factor. + set_config('enabled', 1, 'factor_email'); + + // Check factor order config to ensure email is situated in there. + $factororderconfig = get_config('tool_mfa', 'factor_order'); + if (!$factororderconfig) { + set_config('factor_order', 'email', 'tool_mfa'); + } else { + $order = explode(',', $factororderconfig); + // Remove any empty entries (this happens with entries like ',sms,email'). + $order = array_filter($order); + if (!in_array('email', $order)) { + array_unshift($order, 'email'); + $orderstring = implode(',', $order); + set_config('factor_order', $orderstring, 'tool_mfa'); + } + } + } + + upgrade_plugin_savepoint(true, 2024122400, 'factor', 'email'); + } + + return true; +} diff --git a/admin/tool/mfa/factor/email/settings.php b/admin/tool/mfa/factor/email/settings.php index cf44edf0c48..c7df34c8a7a 100644 --- a/admin/tool/mfa/factor/email/settings.php +++ b/admin/tool/mfa/factor/email/settings.php @@ -27,7 +27,7 @@ defined('MOODLE_INTERNAL') || die(); $enabled = new admin_setting_configcheckbox('factor_email/enabled', new lang_string('settings:enablefactor', 'tool_mfa'), - new lang_string('settings:enablefactor_help', 'tool_mfa'), 0); + new lang_string('settings:enablefactor_help', 'tool_mfa'), 1); $enabled->set_updatedcallback(function () { \tool_mfa\manager::do_factor_action('email', get_config('factor_email', 'enabled') ? 'enable' : 'disable'); }); diff --git a/admin/tool/mfa/factor/email/version.php b/admin/tool/mfa/factor/email/version.php index fc435b71441..fa85f1096d8 100644 --- a/admin/tool/mfa/factor/email/version.php +++ b/admin/tool/mfa/factor/email/version.php @@ -26,7 +26,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2024121800; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2024122400; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2024100100; // Requires this Moodle version. $plugin->component = 'factor_email'; // Full name of the plugin (used for diagnostics). $plugin->maturity = MATURITY_STABLE; diff --git a/admin/tool/mfa/factor/grace/tests/factor_test.php b/admin/tool/mfa/factor/grace/tests/factor_test.php index fedee67abf1..46f62bfc76b 100644 --- a/admin/tool/mfa/factor/grace/tests/factor_test.php +++ b/admin/tool/mfa/factor/grace/tests/factor_test.php @@ -37,6 +37,9 @@ final class factor_test extends \advanced_testcase { $user = $this->getDataGenerator()->create_user(); $this->setUser($user); + // Disable the email factor (enabled by default). + set_config('enabled', 0, 'factor_email'); + $grace = \tool_mfa\plugininfo\factor::get_factor('grace'); $affecting = $grace->get_affecting_factors(); $this->assertEquals(0, count($affecting)); diff --git a/admin/tool/mfa/factor/sms/tests/behat/factor_sms_login.feature b/admin/tool/mfa/factor/sms/tests/behat/factor_sms_login.feature index 7ba2c00c059..5b58edea4ac 100644 --- a/admin/tool/mfa/factor/sms/tests/behat/factor_sms_login.feature +++ b/admin/tool/mfa/factor/sms/tests/behat/factor_sms_login.feature @@ -16,6 +16,8 @@ Feature: Login user with sms authentication factor | enabled | 1 | factor_sms | | weight | 100 | factor_sms | | duration | 1800 | factor_sms | + And the following config values are set as admin: + | enabled | 0 | factor_email | And I navigate to "Plugins > Admin tools > Multi-factor authentication" in site administration And I follow "Edit settings for the SMS mobile phone factor" And I set the field "SMS gateway" to "Dummy gateway (AWS)" diff --git a/admin/tool/mfa/tests/admin_setting_managemfa_test.php b/admin/tool/mfa/tests/admin_setting_managemfa_test.php index dc7af45fe68..5fc0fed5d23 100644 --- a/admin/tool/mfa/tests/admin_setting_managemfa_test.php +++ b/admin/tool/mfa/tests/admin_setting_managemfa_test.php @@ -31,6 +31,16 @@ final class admin_setting_managemfa_test extends \advanced_testcase { use \tool_mfa\tests\mfa_settings_trait; + /** + * Setup testcase. + */ + public function setUp(): void { + parent::setUp(); + $this->resetAfterTest(); + // Disable email factor (enabled by default). + $this->set_factor_state('email', 0); + } + /** * Tests getting the factor combinations */ @@ -137,7 +147,6 @@ final class admin_setting_managemfa_test extends \advanced_testcase { * @param int $combinationscount expected count of available combinations */ public function test_get_factor_combinations_with_data_provider(array $factorset, int $combinationscount): void { - $this->resetAfterTest(); $enabledcount = 0; foreach ($factorset as $factor) { @@ -168,7 +177,6 @@ final class admin_setting_managemfa_test extends \advanced_testcase { * Tests checking the factor combinations */ public function test_factor_combination_checker(): void { - $this->resetAfterTest(); $managemfa = new \tool_mfa\table\admin_setting_managemfa(); $user = $this->getDataGenerator()->create_user(); $this->setUser($user); diff --git a/admin/tool/mfa/tests/behat/tool_mfa_factor_management_table.feature b/admin/tool/mfa/tests/behat/tool_mfa_factor_management_table.feature index 97f8cae6d46..0208abe4191 100644 --- a/admin/tool/mfa/tests/behat/tool_mfa_factor_management_table.feature +++ b/admin/tool/mfa/tests/behat/tool_mfa_factor_management_table.feature @@ -24,3 +24,7 @@ Feature: Manage factor plugins And "Grace period" "table_row" should appear before "Trust this device" "table_row" And I click on "Move down" "link" in the "Grace period" "table_row" And "Grace period" "table_row" should appear after "Trust this device" "table_row" + + Scenario: Email factor is enabled by default + Given I navigate to "Plugins > Admin tools > Multi-factor authentication" in site administration + And I should see "Disable Email" in the "Email" "table_row" diff --git a/admin/tool/mfa/tests/behat/tool_mfa_setup_and_manage_user_factors.feature b/admin/tool/mfa/tests/behat/tool_mfa_setup_and_manage_user_factors.feature index 7abfdb43655..dcd45edd48c 100644 --- a/admin/tool/mfa/tests/behat/tool_mfa_setup_and_manage_user_factors.feature +++ b/admin/tool/mfa/tests/behat/tool_mfa_setup_and_manage_user_factors.feature @@ -39,6 +39,8 @@ Feature: Set up and manage user factors | enabled | 1 | factor_webauthn | And the following config values are set as admin: | enabled | 1 | factor_sms | + And the following config values are set as admin: + | enabled | 0 | factor_email | And the following "tool_mfa > User factors" exist: | username | factor | label | | admin | sms | +409111222 | diff --git a/admin/tool/mfa/tests/manager_test.php b/admin/tool/mfa/tests/manager_test.php index 4780d2978a0..9ed278f28b3 100644 --- a/admin/tool/mfa/tests/manager_test.php +++ b/admin/tool/mfa/tests/manager_test.php @@ -86,6 +86,9 @@ final class manager_test extends \advanced_testcase { $user = $this->getDataGenerator()->create_user(); $this->setUser($user); + // Disable the email factor (enabled by default). + set_config('enabled', 0, 'factor_email'); + // Check for fail status with no factors. $this->assertEquals(\tool_mfa\plugininfo\factor::STATE_FAIL, \tool_mfa\manager::get_status()); @@ -369,6 +372,8 @@ final class manager_test extends \advanced_testcase { $this->setUser($user); set_config('enabled', 1, 'factor_nosetup'); set_config('enabled', 1, 'tool_mfa'); + // Disable the email factor (enabled by default). + set_config('enabled', 0, 'factor_email'); // Capability Check. $this->assertTrue(\tool_mfa\manager::is_ready()); diff --git a/admin/tool/mfa/tests/plugininfo_factor_test.php b/admin/tool/mfa/tests/plugininfo_factor_test.php index 08d70f88c41..877e9a88733 100644 --- a/admin/tool/mfa/tests/plugininfo_factor_test.php +++ b/admin/tool/mfa/tests/plugininfo_factor_test.php @@ -44,6 +44,9 @@ final class plugininfo_factor_test extends \advanced_testcase { $user = $this->getDataGenerator()->create_user(); $this->setUser($user); + // Disable the email factor (enabled by default). + set_config('enabled', 0, 'factor_email'); + // Test that with no enabled factors, fallback is returned. $this->assertEquals('fallback', \tool_mfa\plugininfo\factor::get_next_user_login_factor()->name); @@ -90,22 +93,14 @@ final class plugininfo_factor_test extends \advanced_testcase { $user = $this->getDataGenerator()->create_user(); $this->setUser($user); - // Create two active user factors. + // Add another factor (email factor is enabled by default). set_config('enabled', 1, 'factor_totp'); - set_config('enabled', 1, 'factor_webauthn'); $data = new \stdClass(); $data->userid = $user->id; $data->factor = 'totp'; $data->label = 'testtotp'; $data->revoked = 0; - $DB->insert_record('tool_mfa', $data); - - $data = new \stdClass(); - $data->userid = $user->id; - $data->factor = 'webauthn'; - $data->label = 'testwebauthn'; - $data->revoked = 0; $factorid = $DB->insert_record('tool_mfa', $data); // Test there is more than one active factor.