From 6d5acdbdcc1535b7e9a8dca1b721f4c52ce11812 Mon Sep 17 00:00:00 2001 From: Thomas Legay Date: Thu, 3 Apr 2025 15:07:20 +0200 Subject: [PATCH 01/15] Allow use custom claims from token --- classes/form/application.php | 5 +++++ lang/en/auth_oidc.php | 2 ++ lib.php | 3 +++ manageapplication.php | 4 ++-- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/classes/form/application.php b/classes/form/application.php index b1ffe24b3..606eaa756 100644 --- a/classes/form/application.php +++ b/classes/form/application.php @@ -172,6 +172,11 @@ protected function definition() { $mform->setDefault('oidcscope', 'openid profile email'); $mform->addElement('static', 'oidcscope_help', '', get_string('oidcscope_help', 'auth_oidc')); + // Custom Claim. + $mform->addElement('text', 'customclaims', auth_oidc_config_name_in_form('customclaim'), ['size' => 120]); + $mform->setType('customclaims', PARAM_TEXT); + $mform->setDefault('customclaims', ''); + $mform->addElement('static', 'customclaims_help', '', get_string('customclaim_help', 'auth_oidc')); // Secret expiry notifications recipients. if (auth_oidc_is_local_365_installed()) { $mform->addElement( diff --git a/lang/en/auth_oidc.php b/lang/en/auth_oidc.php index b5e3b6b36..c4245f1d7 100644 --- a/lang/en/auth_oidc.php +++ b/lang/en/auth_oidc.php @@ -154,6 +154,8 @@ Note this is paramater is not supported in Microsoft identity platform (v2.0) IdP type.'; $string['oidcscope'] = 'Scope'; $string['oidcscope_help'] = 'The OIDC Scope to use.'; +$string['customclaims'] = 'Add custom claims'; +$string['customclaims_help'] = 'Custom claims list to get from the token (claims need to be separate by space)'; $string['secretexpiryrecipients'] = 'Secret Expiry Notification Recipients'; $string['secretexpiryrecipients_help'] = 'A comma-separated list of email addresses to send secret expiry notifications to.
If no email address is entered, the main site administrator will be notified.'; diff --git a/lib.php b/lib.php index 3589e8616..0d9289490 100644 --- a/lib.php +++ b/lib.php @@ -376,6 +376,9 @@ function auth_oidc_get_remote_fields() { 'surname' => get_string('settings_fieldmap_field_surname', 'auth_oidc'), 'mail' => get_string('settings_fieldmap_field_mail', 'auth_oidc'), ]; + foreach (explode(" ",get_config('auth_oidc', 'customclaims')) as $value){ + $remotefields[$value] = $value; + } } return $remotefields; diff --git a/manageapplication.php b/manageapplication.php index 79ce389e7..cfa1aaf1b 100644 --- a/manageapplication.php +++ b/manageapplication.php @@ -60,7 +60,7 @@ 'idptype', 'clientid', 'clientauthmethod', 'clientsecret', 'clientprivatekey', 'clientcert', 'clientcertsource', 'clientprivatekeyfile', 'clientcertfile', 'clientcertpassphrase', 'authendpoint', 'tokenendpoint', 'oidcresource', 'oidcscope', 'secretexpiryrecipients', - 'bindingusernameclaim', 'customclaimname', + 'bindingusernameclaim', 'customclaimname', 'customclaims', ] as $field ) { if (isset($oidcconfig->$field)) { @@ -80,7 +80,7 @@ // Prepare config settings to save. $configstosave = ['idptype', 'clientid', 'clientauthmethod', 'authendpoint', 'tokenendpoint', - 'oidcresource', 'oidcscope']; + 'oidcresource', 'oidcscope','customclaims']; // Depending on the value of clientauthmethod, save clientsecret or (clientprivatekey and clientcert). switch ($fromform->clientauthmethod) { From 3998e42d91ce3e742cd1834cb76ac102b62bd8c2 Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Mon, 29 Dec 2025 11:10:06 +0000 Subject: [PATCH 02/15] add validation, support of Microsoft IdP, and improve config instructions --- classes/form/application.php | 16 ++++++++++-- lang/en/auth_oidc.php | 12 +++++++-- lib.php | 47 +++++++++++++++++++++++++++++++++--- manageapplication.php | 2 +- 4 files changed, 69 insertions(+), 8 deletions(-) diff --git a/classes/form/application.php b/classes/form/application.php index 606eaa756..f3d6f6116 100644 --- a/classes/form/application.php +++ b/classes/form/application.php @@ -173,10 +173,10 @@ protected function definition() { $mform->addElement('static', 'oidcscope_help', '', get_string('oidcscope_help', 'auth_oidc')); // Custom Claim. - $mform->addElement('text', 'customclaims', auth_oidc_config_name_in_form('customclaim'), ['size' => 120]); + $mform->addElement('text', 'customclaims', auth_oidc_config_name_in_form('customclaims'), ['size' => 120]); $mform->setType('customclaims', PARAM_TEXT); $mform->setDefault('customclaims', ''); - $mform->addElement('static', 'customclaims_help', '', get_string('customclaim_help', 'auth_oidc')); + $mform->addElement('static', 'customclaims_help', '', get_string('customclaims_help', 'auth_oidc')); // Secret expiry notifications recipients. if (auth_oidc_is_local_365_installed()) { $mform->addElement( @@ -304,6 +304,18 @@ public function validation($data, $files) { } } + // Validate custom claims. + if (!empty($data['customclaims'])) { + $claims = explode(' ', $data['customclaims']); + foreach ($claims as $claim) { + $claim = trim($claim); + if (!empty($claim) && !preg_match('/^[a-zA-Z0-9_-]+$/', $claim)) { + $errors['customclaims'] = get_string('error_invalid_custom_claim', 'auth_oidc'); + break; + } + } + } + return $errors; } } diff --git a/lang/en/auth_oidc.php b/lang/en/auth_oidc.php index c4245f1d7..96510760f 100644 --- a/lang/en/auth_oidc.php +++ b/lang/en/auth_oidc.php @@ -154,8 +154,15 @@ Note this is paramater is not supported in Microsoft identity platform (v2.0) IdP type.'; $string['oidcscope'] = 'Scope'; $string['oidcscope_help'] = 'The OIDC Scope to use.'; -$string['customclaims'] = 'Add custom claims'; -$string['customclaims_help'] = 'Custom claims list to get from the token (claims need to be separate by space)'; +$string['customclaims'] = 'Custom claims'; +$string['customclaims_help'] = 'Space-separated list of custom claim names from your identity provider\'s tokens.
+Important: +'; $string['secretexpiryrecipients'] = 'Secret Expiry Notification Recipients'; $string['secretexpiryrecipients_help'] = 'A comma-separated list of email addresses to send secret expiry notifications to.
If no email address is entered, the main site administrator will be notified.'; @@ -271,6 +278,7 @@ '; $string['error_tenant_specific_endpoint_required'] = 'When using "Microsoft identity platform (v2.0)" IdP type and "Certificate" authentication method, tenant specific endpoint (i.e. not common/organizations/consumers) is required.'; $string['error_empty_oidcresource'] = 'Resource cannot be empty when using Microsoft Entra ID (v1.0) or other types of IdP.'; +$string['error_invalid_custom_claim'] = 'Invalid custom claim name. Custom claims can only contain alphanumeric characters, hyphens, and underscores.'; $string['erroruserwithusernamealreadyexists'] = 'Error occurred when trying to rename your Moodle account. A Moodle user with the new username already exists. Ask your site administrator to resolve this first.'; $string['error_no_response_available'] = 'No responses available.'; diff --git a/lib.php b/lib.php index 0d9289490..61fe0fd23 100644 --- a/lib.php +++ b/lib.php @@ -285,6 +285,44 @@ function auth_oidc_delete_token(int $tokenid): void { $DB->delete_records('auth_oidc_token', ['id' => $tokenid]); } +/** + * Process and add custom claims to remote fields array with validation. + * + * @param array $remotefields Existing remote fields array + * @return array Updated remote fields array with validated custom claims + */ +function auth_oidc_process_custom_claims($remotefields) { + $customclaimsconfig = get_config('auth_oidc', 'customclaims'); + if (empty($customclaimsconfig)) { + return $remotefields; + } + + // Split by space, trim, remove empty values, and remove duplicates. + $customclaimsarray = array_filter(array_map('trim', explode(' ', $customclaimsconfig))); + $customclaimsarray = array_unique($customclaimsarray); + + // Get all existing field names as reserved to prevent overriding. + $reserved = array_keys($remotefields); + + foreach ($customclaimsarray as $value) { + // Validate claim name format (alphanumeric, underscore, hyphen only). + if (!preg_match('/^[a-zA-Z0-9_-]+$/', $value)) { + debugging("Invalid custom claim name skipped: $value", DEBUG_DEVELOPER); + continue; + } + + // Prevent overriding existing fields. + if (in_array($value, $reserved, true)) { + debugging("Reserved custom claim name skipped: $value", DEBUG_DEVELOPER); + continue; + } + + $remotefields[$value] = $value; + } + + return $remotefields; +} + /** * Return the list of remote field options in field mapping. * @@ -366,6 +404,9 @@ function auth_oidc_get_remote_fields() { 'auth_oidc' ); } + + // Add custom claims if configured, with validation. + $remotefields = auth_oidc_process_custom_claims($remotefields); } else { $remotefields = [ '' => get_string('settings_fieldmap_feild_not_mapped', 'auth_oidc'), @@ -376,9 +417,9 @@ function auth_oidc_get_remote_fields() { 'surname' => get_string('settings_fieldmap_field_surname', 'auth_oidc'), 'mail' => get_string('settings_fieldmap_field_mail', 'auth_oidc'), ]; - foreach (explode(" ",get_config('auth_oidc', 'customclaims')) as $value){ - $remotefields[$value] = $value; - } + + // Add custom claims if configured, with validation. + $remotefields = auth_oidc_process_custom_claims($remotefields); } return $remotefields; diff --git a/manageapplication.php b/manageapplication.php index cfa1aaf1b..8f7036a4f 100644 --- a/manageapplication.php +++ b/manageapplication.php @@ -80,7 +80,7 @@ // Prepare config settings to save. $configstosave = ['idptype', 'clientid', 'clientauthmethod', 'authendpoint', 'tokenendpoint', - 'oidcresource', 'oidcscope','customclaims']; + 'oidcresource', 'oidcscope', 'customclaims']; // Depending on the value of clientauthmethod, save clientsecret or (clientprivatekey and clientcert). switch ($fromform->clientauthmethod) { From face1bfb3d67bf0ff5d10beceea45cdd3dc73a05 Mon Sep 17 00:00:00 2001 From: Christian Abila Date: Tue, 22 Jul 2025 15:20:53 +0200 Subject: [PATCH 03/15] toggle visibility of client secret for higher security --- classes/form/application.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/form/application.php b/classes/form/application.php index b1ffe24b3..0d2651f2c 100644 --- a/classes/form/application.php +++ b/classes/form/application.php @@ -80,7 +80,7 @@ protected function definition() { $mform->addElement('static', 'clientauthmethod_help', '', get_string('clientauthmethod_help', 'auth_oidc')); // Secret. - $mform->addElement('text', 'clientsecret', auth_oidc_config_name_in_form('clientsecret'), ['size' => 60]); + $mform->addElement('passwordunmask', 'clientsecret', auth_oidc_config_name_in_form('clientsecret'), ['size' => 60]); $mform->setType('clientsecret', PARAM_TEXT); $mform->disabledIf('clientsecret', 'clientauthmethod', 'neq', AUTH_OIDC_AUTH_METHOD_SECRET); $mform->addElement('static', 'clientsecret_help', '', get_string('clientsecret_help', 'auth_oidc')); From efe6d958372be7fbe98e957ff8a9a18cc61f3fab Mon Sep 17 00:00:00 2001 From: Christian Abila Date: Tue, 22 Jul 2025 15:21:13 +0200 Subject: [PATCH 04/15] toggle visibility of certficate file passphrase for higher security --- classes/form/application.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/form/application.php b/classes/form/application.php index 0d2651f2c..2fac77894 100644 --- a/classes/form/application.php +++ b/classes/form/application.php @@ -133,7 +133,7 @@ protected function definition() { $mform->addElement('static', 'clientcertfile_help', '', get_string('clientcertfile_help', 'auth_oidc')); // Certificate file passphrase. - $mform->addElement('text', 'clientcertpassphrase', auth_oidc_config_name_in_form('clientcertpassphrase'), ['size' => 60]); + $mform->addElement('passwordunmask', 'clientcertpassphrase', auth_oidc_config_name_in_form('clientcertpassphrase'), ['size' => 60]); $mform->setType('clientcertpassphrase', PARAM_TEXT); $mform->disabledIf('clientcertpassphrase', 'clientauthmethod', 'neq', AUTH_OIDC_AUTH_METHOD_CERTIFICATE); $mform->addElement('static', 'clientcertpassphrase_help', '', get_string('clientcertpassphrase_help', 'auth_oidc')); From e07471c3e1663d2d03cea71eef4186663f1f25e0 Mon Sep 17 00:00:00 2001 From: Christian Abila Date: Tue, 2 Sep 2025 13:03:16 +0200 Subject: [PATCH 05/15] simplify return value of handle_user_deleted() --- classes/observers.php | 3 +- tests/observers_test.php | 65 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 tests/observers_test.php diff --git a/classes/observers.php b/classes/observers.php index 645acec77..9f715e511 100644 --- a/classes/observers.php +++ b/classes/observers.php @@ -45,7 +45,6 @@ class observers { public static function handle_user_deleted(user_deleted $event) { global $DB; $userid = $event->objectid; - $DB->delete_records('auth_oidc_token', ['userid' => $userid]); - return true; + return $DB->delete_records('auth_oidc_token', ['userid' => $userid]); } } diff --git a/tests/observers_test.php b/tests/observers_test.php new file mode 100644 index 000000000..b7ccef83c --- /dev/null +++ b/tests/observers_test.php @@ -0,0 +1,65 @@ +. + +namespace auth_oidc; + +use advanced_testcase; +use coding_exception; +use dml_exception; + +/** + * Unit tests for the class \auth_oidc\observers + * + * @package auth_oidc + * @copyright 2025 eDaktik GmbH {@link https://www.edaktik.at/} + * @author Christian Abila + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \auth_oidc\observers + */ +final class observers_test extends advanced_testcase { + /** + * OIDC user token is deleted when the Moodle user is deleted. + * + * @return void + * @throws dml_exception + * @throws coding_exception + */ + public function test_token_is_deleted_when_user_is_deleted(): void { + global $DB; + $this->resetAfterTest(); + // Create user. + $user = $this->getDataGenerator()->create_user([]); + // Insert an entry in auth_oidc_token. + $DB->insert_record( + 'auth_oidc_token', + [ + 'userid' => $user->id, + 'oidcuniqid' => 'oidcuniqid', + 'username' => 'username', + 'scope' => 'scope', + 'authcode' => 'authcode', + 'token' => 'token', + 'expiry' => time(), + 'refreshtoken' => 'refreshtoken', + 'idtoken' => 'idtoken', + ], + ); + // Delete the user. + delete_user($user); + + $this->assertFalse($DB->record_exists('auth_oidc_token', ['userid' => $user->id])); + } +} From 06c5e7963d28bd3b47a63c55770e1dbd970bb0d3 Mon Sep 17 00:00:00 2001 From: Christian Abila Date: Mon, 12 Jan 2026 10:44:08 +0100 Subject: [PATCH 06/15] correct member name --- auth.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/auth.php b/auth.php index 75eb901f3..b276d7234 100644 --- a/auth.php +++ b/auth.php @@ -53,9 +53,9 @@ public function __construct($forceloginflow = null) { if ( isset($SESSION->stateadditionaldata) && !empty($SESSION->stateadditionaldata) && - isset($SESSION->stateadditoinaldata['forceflow']) + isset($SESSION->stateadditionaldata['forceflow']) ) { - $loginflow = $SESSION->stateadditoinaldata['forceflow']; + $loginflow = $SESSION->stateadditionaldata['forceflow']; } else { if (!empty($forceloginflow) && is_string($forceloginflow)) { $loginflow = $forceloginflow; From de88ce806bcfdc7740a17a2fc2be3cdd052fefa7 Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Tue, 20 Jan 2026 16:37:17 +0000 Subject: [PATCH 07/15] Prevent creating duplicate auth_oidc_token records with same username, and show debugging information on failed logins --- classes/loginflow/base.php | 3 +++ index.php | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/classes/loginflow/base.php b/classes/loginflow/base.php index 7ea031cef..72f9b6d35 100644 --- a/classes/loginflow/base.php +++ b/classes/loginflow/base.php @@ -674,6 +674,9 @@ protected function createtoken( // Cleanup old invalid token with the same oidcusername. $DB->delete_records('auth_oidc_token', ['oidcusername' => $oidcusername]); + // Cleanup old token with the same Moodle username to prevent duplicates. + $DB->delete_records('auth_oidc_token', ['username' => $username]); + // Handle "The existing token for this user does not contain a valid user ID" error. if ($userid == 0) { $userrec = $DB->get_record('user', ['username' => $username]); diff --git a/index.php b/index.php index 20082e922..d5f0c7908 100644 --- a/index.php +++ b/index.php @@ -29,4 +29,38 @@ $auth = new \auth_plugin_oidc('authcode'); $auth->set_httpclient(new \auth_oidc\httpclient()); -$auth->handleredirect(); + +try { + $auth->handleredirect(); +} catch (moodle_exception $e) { + // If debugging is off, re-throw to let Moodle handle it with generic message. + if (empty($CFG->debug) || $CFG->debug < DEBUG_MINIMAL) { + throw $e; + } + + // Only display detailed debug information if debug display is enabled. + // This prevents leaking sensitive internal details to unauthenticated users. + $showdetails = !empty($CFG->debugdisplay); + + if ($showdetails) { + // Display error details when debug display is enabled. + $errormessage = $e->getMessage(); + if (!empty($e->debuginfo)) { + $errormessage .= ' (' . $e->debuginfo . ')'; + } + } else { + // Show generic error message to prevent information disclosure. + $errormessage = get_string('errorauthgeneral', 'auth_oidc'); + } + + $PAGE->set_url('/auth/oidc/'); + $PAGE->set_context(context_system::instance()); + $PAGE->set_pagelayout('login'); + $PAGE->set_title(get_string('error')); + + echo $OUTPUT->header(); + echo $OUTPUT->notification($errormessage, 'error'); + echo $OUTPUT->single_button(new moodle_url('/login/index.php'), get_string('login'), 'get'); + echo $OUTPUT->footer(); + exit; +} From 80877cb6d3df288685990d6d7047f35db153685f Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Mon, 9 Feb 2026 12:58:50 +0000 Subject: [PATCH 08/15] add trigger to auth_oidc/event/user_created when new auth_oidc accounts are created from successful authentication --- classes/event/user_created.php | 2 +- classes/loginflow/authcode.php | 10 ++++++++++ classes/loginflow/rocreds.php | 11 +++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/classes/event/user_created.php b/classes/event/user_created.php index 74c8626da..cfcb24b97 100644 --- a/classes/event/user_created.php +++ b/classes/event/user_created.php @@ -44,7 +44,7 @@ public static function get_name() { * @return string */ public function get_description() { - return "A user (user id '{$this->userid}') was creatd using the OpenID Connect authentication plugin."; + return "A user (user id '{$this->userid}') was created using the OpenID Connect authentication plugin."; } /** diff --git a/classes/loginflow/authcode.php b/classes/loginflow/authcode.php index db3474a35..32cfd8022 100644 --- a/classes/loginflow/authcode.php +++ b/classes/loginflow/authcode.php @@ -27,6 +27,7 @@ namespace auth_oidc\loginflow; use auth_oidc\event\user_authed; +use auth_oidc\event\user_created; use auth_oidc\event\user_rename_attempt; use auth_oidc\jwt; use auth_oidc\utils; @@ -801,6 +802,15 @@ protected function handlelogin(string $oidcuniqid, array $authparams, array $tok } } $user = create_user_record($username, '', 'oidc'); + + // Trigger user_created event. + $eventdata = [ + 'objectid' => $user->id, + 'userid' => $user->id, + 'relateduserid' => $user->id, + ]; + $event = user_created::create($eventdata); + $event->trigger(); } else { // Trigger login failed event. $failurereason = AUTH_LOGIN_NOUSER; diff --git a/classes/loginflow/rocreds.php b/classes/loginflow/rocreds.php index c71786a42..0152bae25 100644 --- a/classes/loginflow/rocreds.php +++ b/classes/loginflow/rocreds.php @@ -25,6 +25,7 @@ namespace auth_oidc\loginflow; +use auth_oidc\event\user_created; use auth_oidc\utils; defined('MOODLE_INTERNAL') || die(); @@ -137,6 +138,16 @@ public function loginpage_hook(&$frm, &$user) { } $user = create_user_record($username, $password, $auth); + + // Trigger user_created event. + $eventdata = [ + 'objectid' => $user->id, + 'userid' => $user->id, + 'relateduserid' => $user->id, + ]; + $event = user_created::create($eventdata); + $event->trigger(); + return true; } From bd955df7c94f0e456a2de585bc665838e846c00b Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Wed, 31 Dec 2025 10:42:35 +0000 Subject: [PATCH 09/15] improve test cases for \auth\oidc\classes\observers.php --- tests/observers_test.php | 86 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 7 deletions(-) diff --git a/tests/observers_test.php b/tests/observers_test.php index b7ccef83c..dc942e27a 100644 --- a/tests/observers_test.php +++ b/tests/observers_test.php @@ -17,8 +17,7 @@ namespace auth_oidc; use advanced_testcase; -use coding_exception; -use dml_exception; +use core\event\user_deleted; /** * Unit tests for the class \auth_oidc\observers @@ -27,19 +26,29 @@ * @copyright 2025 eDaktik GmbH {@link https://www.edaktik.at/} * @author Christian Abila * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @group auth_oidc + * @group office365 * @coversDefaultClass \auth_oidc\observers */ final class observers_test extends advanced_testcase { + /** + * Set up test environment. + * + * @return void + */ + protected function setUp(): void { + parent::setUp(); + $this->resetAfterTest(true); + } + /** * OIDC user token is deleted when the Moodle user is deleted. * * @return void - * @throws dml_exception - * @throws coding_exception + * @covers ::handle_user_deleted */ public function test_token_is_deleted_when_user_is_deleted(): void { global $DB; - $this->resetAfterTest(); // Create user. $user = $this->getDataGenerator()->create_user([]); // Insert an entry in auth_oidc_token. @@ -49,7 +58,9 @@ public function test_token_is_deleted_when_user_is_deleted(): void { 'userid' => $user->id, 'oidcuniqid' => 'oidcuniqid', 'username' => 'username', + 'oidcusername' => 'oidcusername', 'scope' => 'scope', + 'tokenresource' => 'tokenresource', 'authcode' => 'authcode', 'token' => 'token', 'expiry' => time(), @@ -57,9 +68,70 @@ public function test_token_is_deleted_when_user_is_deleted(): void { 'idtoken' => 'idtoken', ], ); - // Delete the user. - delete_user($user); + // Verify token exists before deletion. + $this->assertTrue($DB->record_exists('auth_oidc_token', ['userid' => $user->id])); + + // Create the user_deleted event with all required data. + $event = user_deleted::create([ + 'objectid' => $user->id, + 'relateduserid' => $user->id, + 'context' => \context_system::instance(), + 'other' => [ + 'username' => $user->username, + 'email' => $user->email, + 'idnumber' => $user->idnumber, + 'picture' => $user->picture, + 'mnethostid' => $user->mnethostid, + ], + ]); + + // Call the observer directly to test return value. + $result = observers::handle_user_deleted($event); + + // Verify the method returned true (deletion succeeded). + $this->assertTrue($result); + + // Verify the token was actually deleted. + $this->assertFalse($DB->record_exists('auth_oidc_token', ['userid' => $user->id])); + } + + /** + * Observer returns true when user has no tokens (no error on empty deletion). + * + * @return void + * @covers ::handle_user_deleted + */ + public function test_deletion_succeeds_when_user_has_no_tokens(): void { + global $DB; + + // Create user WITHOUT creating any token. + $user = $this->getDataGenerator()->create_user([]); + + // Verify no token exists. + $this->assertFalse($DB->record_exists('auth_oidc_token', ['userid' => $user->id])); + + // Create the user_deleted event with all required data. + $event = user_deleted::create([ + 'objectid' => $user->id, + 'relateduserid' => $user->id, + 'context' => \context_system::instance(), + 'other' => [ + 'username' => $user->username, + 'email' => $user->email, + 'idnumber' => $user->idnumber, + 'picture' => $user->picture, + 'mnethostid' => $user->mnethostid, + ], + ]); + + // Call the observer directly. + $result = observers::handle_user_deleted($event); + + // Verify the method returned true even though no records were deleted. + $this->assertTrue($result); + + // Verify still no token exists (nothing changed). $this->assertFalse($DB->record_exists('auth_oidc_token', ['userid' => $user->id])); } } From df5096a5936de522a3c7e831aa3bb8e34bc27dca Mon Sep 17 00:00:00 2001 From: Christian Abila Date: Tue, 2 Sep 2025 13:26:06 +0200 Subject: [PATCH 10/15] write unit test for \auth_oidc\task\cleanup_oidc_sid --- tests/task/cleanup_oidc_sid_test.php | 76 ++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 tests/task/cleanup_oidc_sid_test.php diff --git a/tests/task/cleanup_oidc_sid_test.php b/tests/task/cleanup_oidc_sid_test.php new file mode 100644 index 000000000..09ea04408 --- /dev/null +++ b/tests/task/cleanup_oidc_sid_test.php @@ -0,0 +1,76 @@ +. + +namespace auth_oidc\task; + +use advanced_testcase; +use dml_exception; + +/** + * Unit tests for the class cleanup_oidc_sid_test + * + * @package auth_oidc + * @copyright 2025 eDaktik GmbH {@link https://www.edaktik.at/} + * @author Christian Abila + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \auth_oidc\task\cleanup_oidc_sid + */ +final class cleanup_oidc_sid_test extends advanced_testcase { + /** + * SIDs created before yesterday are deleted. + * + * @return void + * @throws dml_exception + * @covers ::execute + */ + public function test_sids_older_than_yesterday_are_deleted(): void { + global $DB, $USER; + $this->resetAfterTest(); + $twodaysago = strtotime('-2 day'); + $yesterday = strtotime('-1 day'); + $today = time(); + // Create entries in auth_oidc_sid. + $entry1id = $DB->insert_record( + 'auth_oidc_sid', + ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => $twodaysago], + ); + $entry2id = $DB->insert_record( + 'auth_oidc_sid', + ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => ($twodaysago - 1000)], + ); + $entry3id = $DB->insert_record( + 'auth_oidc_sid', + ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => $today], + ); + $entry4id = $DB->insert_record( + 'auth_oidc_sid', + ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => $yesterday], + ); + + $cleanup = new cleanup_oidc_sid(); + + $cleanup->execute(); + + $records = $DB->get_records('auth_oidc_sid'); + + $this->assertCount(2, $records); + + $this->assertTrue($DB->record_exists('auth_oidc_sid', ['id' => $entry3id])); + $this->assertFalse($DB->record_exists('auth_oidc_sid', ['id' => $entry1id])); + $this->assertFalse($DB->record_exists('auth_oidc_sid', ['id' => $entry2id])); + $this->assertTrue($DB->record_exists('auth_oidc_sid', ['id' => $entry4id])); + } +} From 7f87e929630aaac444406c97a3da510aa64ed314 Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Wed, 31 Dec 2025 11:42:48 +0000 Subject: [PATCH 11/15] Fix time inconsistency, avoid using $USER in unit test --- tests/task/cleanup_oidc_sid_test.php | 37 +++++++++++++++++++--------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/tests/task/cleanup_oidc_sid_test.php b/tests/task/cleanup_oidc_sid_test.php index 09ea04408..040c30f03 100644 --- a/tests/task/cleanup_oidc_sid_test.php +++ b/tests/task/cleanup_oidc_sid_test.php @@ -20,44 +20,59 @@ use dml_exception; /** - * Unit tests for the class cleanup_oidc_sid_test + * Unit tests for the class cleanup_oidc_sid * * @package auth_oidc * @copyright 2025 eDaktik GmbH {@link https://www.edaktik.at/} * @author Christian Abila * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @group auth_oidc + * @group office365 * @coversDefaultClass \auth_oidc\task\cleanup_oidc_sid */ final class cleanup_oidc_sid_test extends advanced_testcase { /** - * SIDs created before yesterday are deleted. + * SIDs older than 1 day are deleted. + * + * The cleanup task deletes records where timecreated < strtotime('-1 day'). + * Records created exactly 1 day ago or more recently are kept. * * @return void * @throws dml_exception * @covers ::execute */ public function test_sids_older_than_yesterday_are_deleted(): void { - global $DB, $USER; + global $DB; $this->resetAfterTest(); - $twodaysago = strtotime('-2 day'); - $yesterday = strtotime('-1 day'); - $today = time(); - // Create entries in auth_oidc_sid. + + // Create a test user to own the SID records. + $user = $this->getDataGenerator()->create_user(); + + // Calculate cutoff time once to avoid timing issues if test runs slowly. + // This matches what the cleanup task will calculate during execute(). + $cutofftime = strtotime('-1 day'); + + // Create timestamps relative to the cutoff time. + $twodaysago = $cutofftime - DAYSECS; // Older than cutoff, should be deleted. + $yesterday = $cutofftime; // Exactly at cutoff, should be kept (< operator). + $today = time(); // Newer than cutoff, should be kept. + + // Create entries in auth_oidc_sid with unique SIDs. $entry1id = $DB->insert_record( 'auth_oidc_sid', - ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => $twodaysago], + ['userid' => $user->id, 'sid' => 'sid_old_1', 'timecreated' => $twodaysago], ); $entry2id = $DB->insert_record( 'auth_oidc_sid', - ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => ($twodaysago - 1000)], + ['userid' => $user->id, 'sid' => 'sid_old_2', 'timecreated' => ($twodaysago - 1000)], ); $entry3id = $DB->insert_record( 'auth_oidc_sid', - ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => $today], + ['userid' => $user->id, 'sid' => 'sid_new_1', 'timecreated' => $today], ); $entry4id = $DB->insert_record( 'auth_oidc_sid', - ['userid' => $USER->id, 'sid' => 'sid', 'timecreated' => $yesterday], + ['userid' => $user->id, 'sid' => 'sid_boundary', 'timecreated' => $yesterday], ); $cleanup = new cleanup_oidc_sid(); From 984e3c5ae219e9baa813db1fb62c17186af633b4 Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Tue, 10 Mar 2026 09:20:19 +0000 Subject: [PATCH 12/15] Further bug fix and performance improvements --- db/install.xml | 1 + db/upgrade.php | 14 ++++++++++++++ version.php | 2 +- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/db/install.xml b/db/install.xml index 93029ea81..023fff036 100644 --- a/db/install.xml +++ b/db/install.xml @@ -58,6 +58,7 @@ + diff --git a/db/upgrade.php b/db/upgrade.php index a7facf0cf..30566e670 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -567,5 +567,19 @@ function xmldb_auth_oidc_upgrade($oldversion) { upgrade_plugin_savepoint(true, 2024100702, 'auth', 'oidc'); } + if ($oldversion < 2025100600.01) { + // Define index to be added to auth_oidc_token. + $table = new xmldb_table('auth_oidc_token'); + $index = new xmldb_index('oidcusername', XMLDB_INDEX_NOTUNIQUE, ['oidcusername']); + + // Conditionally launch add index oidcusername. + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + // Oidc savepoint reached. + upgrade_plugin_savepoint(true, 2025100600.01, 'auth', 'oidc'); + } + return true; } diff --git a/version.php b/version.php index 5d17fdd0d..dec77b4a1 100644 --- a/version.php +++ b/version.php @@ -25,7 +25,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2025100600; +$plugin->version = 2025100600.01; $plugin->requires = 2025100600; $plugin->release = '5.1.0'; $plugin->component = 'auth_oidc'; From a7bee050ef3108d112ef02b934877a2ef2a38812 Mon Sep 17 00:00:00 2001 From: Ivan Date: Fri, 13 Mar 2026 17:49:21 +0300 Subject: [PATCH 13/15] fixed a bug with a non-existent key in the project for custom moodle and idp connection --- classes/loginflow/base.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/loginflow/base.php b/classes/loginflow/base.php index 7ea031cef..8722c0ad8 100644 --- a/classes/loginflow/base.php +++ b/classes/loginflow/base.php @@ -752,7 +752,7 @@ protected function get_oidc_username_from_token_claim(jwt $idtoken, string $bind switch ($bindingusernameclaim) { case 'custom': - $bindingusernameclaim = get_config('auth_oidc', 'custombindingclaim'); + $bindingusernameclaim = get_config('auth_oidc', 'customclaimname'); // No break. case 'preferred_username': case 'email': From 60cbc4eb6d4d2cdacddf8e17731e2684391342e0 Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Tue, 30 Dec 2025 09:44:39 +0000 Subject: [PATCH 14/15] Implement Azure portal style secret / certificate passphrase protection --- classes/form/application.php | 143 +++++++++++++++++++++++++++++++++-- js/module.js | 127 +++++++++++++++++++++++++++++++ lang/en/auth_oidc.php | 5 ++ lib.php | 27 +++++++ manageapplication.php | 59 ++++++++++++++- 5 files changed, 352 insertions(+), 9 deletions(-) diff --git a/classes/form/application.php b/classes/form/application.php index 2fac77894..2fe53bfde 100644 --- a/classes/form/application.php +++ b/classes/form/application.php @@ -79,10 +79,39 @@ protected function definition() { $mform->setDefault('clientauthmethod', AUTH_OIDC_AUTH_METHOD_SECRET); $mform->addElement('static', 'clientauthmethod_help', '', get_string('clientauthmethod_help', 'auth_oidc')); - // Secret. - $mform->addElement('passwordunmask', 'clientsecret', auth_oidc_config_name_in_form('clientsecret'), ['size' => 60]); + // Secret - Check if there's an existing secret to determine if we should add a "change" checkbox. + $hasexistingsecret = isset($this->_customdata['oidcconfig']->clientsecret) && + !empty($this->_customdata['oidcconfig']->clientsecret); + + if ($hasexistingsecret) { + $mform->addElement( + 'advcheckbox', + 'changesecret', + get_string('change_client_secret', 'auth_oidc'), + get_string('change_client_secret_desc', 'auth_oidc') + ); + $mform->setType('changesecret', PARAM_BOOL); + $mform->disabledIf('changesecret', 'clientauthmethod', 'eq', AUTH_OIDC_AUTH_METHOD_CERTIFICATE); + + // Store the original masked value in a hidden field AND as a data attribute for JavaScript to use. + $maskedsecret = auth_oidc_mask_secret($this->_customdata['oidcconfig']->clientsecret); + $mform->addElement('hidden', 'originalsecretmasked', $maskedsecret); + $mform->setType('originalsecretmasked', PARAM_TEXT); + } + + $attributes = ['size' => 60, 'autocomplete' => 'off', 'class' => 'secret-field']; + if ($hasexistingsecret) { + $maskedsecret = auth_oidc_mask_secret($this->_customdata['oidcconfig']->clientsecret); + $attributes['data-original-masked'] = $maskedsecret; + } + $mform->addElement('text', 'clientsecret', auth_oidc_config_name_in_form('clientsecret'), $attributes); $mform->setType('clientsecret', PARAM_TEXT); $mform->disabledIf('clientsecret', 'clientauthmethod', 'neq', AUTH_OIDC_AUTH_METHOD_SECRET); + + if ($hasexistingsecret) { + $mform->disabledIf('clientsecret', 'changesecret', 'notchecked'); + } + $mform->addElement('static', 'clientsecret_help', '', get_string('clientsecret_help', 'auth_oidc')); // Certificate source. @@ -132,10 +161,38 @@ protected function definition() { $mform->disabledIf('clientcertfile', 'clientcertsource', 'neq', AUTH_OIDC_AUTH_CERT_SOURCE_FILE); $mform->addElement('static', 'clientcertfile_help', '', get_string('clientcertfile_help', 'auth_oidc')); - // Certificate file passphrase. - $mform->addElement('passwordunmask', 'clientcertpassphrase', auth_oidc_config_name_in_form('clientcertpassphrase'), ['size' => 60]); + // Certificate file passphrase - Check if there's an existing passphrase. + $hasexistingpassphrase = isset($this->_customdata['oidcconfig']->clientcertpassphrase) && + !empty($this->_customdata['oidcconfig']->clientcertpassphrase); + + if ($hasexistingpassphrase) { + $mform->addElement( + 'advcheckbox', + 'changecertpassphrase', + get_string('change_cert_passphrase', 'auth_oidc'), + get_string('change_cert_passphrase_desc', 'auth_oidc') + ); + $mform->setType('changecertpassphrase', PARAM_BOOL); + + // Store the original masked value in a hidden field AND as a data attribute for JavaScript to use. + $maskedpassphrase = auth_oidc_mask_secret($this->_customdata['oidcconfig']->clientcertpassphrase); + $mform->addElement('hidden', 'originalpassphrasemasked', $maskedpassphrase); + $mform->setType('originalpassphrasemasked', PARAM_TEXT); + } + + $attributes = ['size' => 60, 'autocomplete' => 'off', 'class' => 'secret-field']; + if ($hasexistingpassphrase) { + $maskedpassphrase = auth_oidc_mask_secret($this->_customdata['oidcconfig']->clientcertpassphrase); + $attributes['data-original-masked'] = $maskedpassphrase; + } + $mform->addElement('text', 'clientcertpassphrase', auth_oidc_config_name_in_form('clientcertpassphrase'), $attributes); $mform->setType('clientcertpassphrase', PARAM_TEXT); $mform->disabledIf('clientcertpassphrase', 'clientauthmethod', 'neq', AUTH_OIDC_AUTH_METHOD_CERTIFICATE); + + if ($hasexistingpassphrase) { + $mform->disabledIf('clientcertpassphrase', 'changecertpassphrase', 'notchecked'); + } + $mform->addElement('static', 'clientcertpassphrase_help', '', get_string('clientcertpassphrase_help', 'auth_oidc')); // Endpoints header. @@ -230,7 +287,21 @@ public function validation($data, $files) { // Validate authentication variables. switch ($data['clientauthmethod']) { case AUTH_OIDC_AUTH_METHOD_SECRET: - if (empty(trim($data['clientsecret']))) { + // Check if user is attempting to change the secret. + $changesecret = isset($data['changesecret']) ? $data['changesecret'] : true; + $existingsecret = get_config('auth_oidc', 'clientsecret'); + + if ($changesecret) { + // User wants to change the secret, validate the new value. + if (empty(trim($data['clientsecret']))) { + $errors['clientsecret'] = get_string('error_empty_client_secret', 'auth_oidc'); + } else if (auth_oidc_is_masked_secret($data['clientsecret'])) { + // User checked "change secret" but didn't enter a new value. + $errors['clientsecret'] = get_string('error_masked_secret_not_changed', 'auth_oidc'); + } + } else if (empty($existingsecret) && empty(trim($data['clientsecret']))) { + // No existing secret and field is empty - this is invalid. + // This handles edge cases where checkbox logic fails. $errors['clientsecret'] = get_string('error_empty_client_secret', 'auth_oidc'); } break; @@ -253,6 +324,16 @@ public function validation($data, $files) { } break; } + + // Validate certificate passphrase if user is attempting to change it. + $changecertpassphrase = isset($data['changecertpassphrase']) ? $data['changecertpassphrase'] : true; + + if ($changecertpassphrase && !empty($data['clientcertpassphrase'])) { + if (auth_oidc_is_masked_secret($data['clientcertpassphrase'])) { + // User checked "change passphrase" but didn't enter a new value. + $errors['clientcertpassphrase'] = get_string('error_masked_secret_not_changed', 'auth_oidc'); + } + } break; } @@ -301,4 +382,56 @@ public function validation($data, $files) { return $errors; } + + /** + * Process data after form definition and data loading. + * This is called after set_data() and after validation errors, allowing us to override submitted values. + * + * @return void + */ + public function definition_after_data() { + parent::definition_after_data(); + + $mform =& $this->_form; + + // Get the current checkbox states using getSubmitValue (works for submitted data). + $changesecret = $mform->getSubmitValue('changesecret'); + $changecertpassphrase = $mform->getSubmitValue('changecertpassphrase'); + + // If the "change secret" checkbox is NOT checked and there's an existing secret, + // ensure the field shows the masked value (especially important after validation errors). + if (isset($this->_customdata['oidcconfig']->clientsecret) && !empty($this->_customdata['oidcconfig']->clientsecret)) { + $currentvalue = $mform->getSubmitValue('clientsecret'); + + // If checkbox is not checked and field is empty or doesn't match masked value, restore it. + if (empty($changesecret) && (empty($currentvalue) || !auth_oidc_is_masked_secret($currentvalue))) { + $maskedsecret = auth_oidc_mask_secret($this->_customdata['oidcconfig']->clientsecret); + // Force the element to use the masked value. + $element = $mform->getElement('clientsecret'); + $element->setValue($maskedsecret); + + // Also update the data attribute for JavaScript reliability. + $element->updateAttributes(['data-original-masked' => $maskedsecret]); + } + } + + // Same logic for certificate passphrase. + if ( + isset($this->_customdata['oidcconfig']->clientcertpassphrase) && + !empty($this->_customdata['oidcconfig']->clientcertpassphrase) + ) { + $currentvalue = $mform->getSubmitValue('clientcertpassphrase'); + + // If checkbox is not checked and field is empty or doesn't match masked value, restore it. + if (empty($changecertpassphrase) && (empty($currentvalue) || !auth_oidc_is_masked_secret($currentvalue))) { + $maskedpassphrase = auth_oidc_mask_secret($this->_customdata['oidcconfig']->clientcertpassphrase); + // Force the element to use the masked value. + $element = $mform->getElement('clientcertpassphrase'); + $element->setValue($maskedpassphrase); + + // Also update the data attribute for JavaScript reliability. + $element->updateAttributes(['data-original-masked' => $maskedpassphrase]); + } + } + } } diff --git a/js/module.js b/js/module.js index 08c510f8a..cfecd5f5a 100644 --- a/js/module.js +++ b/js/module.js @@ -13,6 +13,30 @@ M.auth_oidc.init = function(Y, idptype_ms, authmethodsecret, authmethodcertifica var $clientcertpassphrase = $("#id_clientcertpassphrase"); var $clientcertsource = $("#id_clientcertsource"); var $secretexpiryrecipients = $("#id_secretexpiryrecipients"); + var $changesecret = $("#id_changesecret"); + + // Get the original masked value for client secret - needed by multiple handlers. + var originalSecretValue = ''; + if ($changesecret.length) { + // 1. Try data attribute (most reliable, set directly in form definition) + if ($clientsecret.attr('data-original-masked')) { + originalSecretValue = $clientsecret.attr('data-original-masked'); + } + + // 2. Fallback to hidden field + if (!originalSecretValue) { + var $originalsecretmasked = $("#id_originalsecretmasked"); + if ($originalsecretmasked.length) { + originalSecretValue = $originalsecretmasked.val(); + } + } + + // 3. Last fallback: if field value looks masked, use it + // Pattern matches either 10 asterisks (for short secrets) or 2 chars + 10 asterisks. + if (!originalSecretValue && $clientsecret.val() && $clientsecret.val().match(/^(.{2})?\*{10}$/)) { + originalSecretValue = $clientsecret.val(); + } + } $idptype.change(function() { if ($(this).val() != idptype_ms) { @@ -48,8 +72,111 @@ M.auth_oidc.init = function(Y, idptype_ms, authmethodsecret, authmethodcertifica } $clientcertpassphrase.prop('disabled', false); $clientcertsource.prop('disabled', false); + // Disable the "change secret" checkbox when certificate authentication is selected. + if ($changesecret.length) { + // If the checkbox is checked, restore the masked value before disabling. + if ($changesecret.is(':checked') && originalSecretValue) { + $clientsecret.val(originalSecretValue); + } + $changesecret.prop('disabled', true); + $changesecret.prop('checked', false); + } + $clientsecret.prop('disabled', true); } else { $secretexpiryrecipients.prop('disabled', false); + // Re-enable the "change secret" checkbox when secret authentication is selected. + if ($changesecret.length) { + $changesecret.prop('disabled', false); + } } }); + + // Handle "change secret" checkbox - clear field when checked, restore when unchecked. + if ($changesecret.length) { + $changesecret.change(function() { + if ($(this).is(':checked')) { + // Clear the masked value when user wants to change the secret. + $clientsecret.prop('disabled', false); // Ensure it's enabled. + $clientsecret.val(''); + $clientsecret.focus(); + } else { + // Restore the original masked value when unchecked. + if (originalSecretValue) { + // Set the value BEFORE Moodle's disabledIf disables the field. + $clientsecret.prop('disabled', false); // Temporarily enable to set value. + $clientsecret.val(originalSecretValue); + // Small delay to ensure value is set before Moodle's form dependencies disable it. + setTimeout(function() { + // Moodle's disabledIf will handle disabling the field after this. + }, 10); + } + } + }); + + // On page load, if checkbox is unchecked, ensure field shows the masked value. + if (!$changesecret.is(':checked')) { + if (originalSecretValue && $clientsecret.val() !== originalSecretValue) { + $clientsecret.val(originalSecretValue); + } + } + } + + // Handle "change certificate passphrase" checkbox. + var $changecertpassphrase = $("#id_changecertpassphrase"); + if ($changecertpassphrase.length) { + // Get the original masked value - try multiple sources for reliability. + var originalPassphraseValue = ''; + + // 1. Try data attribute (most reliable, set directly in form definition) + if ($clientcertpassphrase.attr('data-original-masked')) { + originalPassphraseValue = $clientcertpassphrase.attr('data-original-masked'); + } + + // 2. Fallback to hidden field + if (!originalPassphraseValue) { + var $originalpassphrasemasked = $("#id_originalpassphrasemasked"); + if ($originalpassphrasemasked.length) { + originalPassphraseValue = $originalpassphrasemasked.val(); + } + } + + // 3. Last fallback: if field value looks masked, use it + // Pattern matches either 10 asterisks (for short passphrases) or 2 chars + 10 asterisks. + if (!originalPassphraseValue && $clientcertpassphrase.val() && $clientcertpassphrase.val().match(/^(.{2})?\*{10}$/)) { + originalPassphraseValue = $clientcertpassphrase.val(); + } + + $changecertpassphrase.change(function() { + if ($(this).is(':checked')) { + // Clear the masked value when user wants to change the passphrase. + $clientcertpassphrase.prop('disabled', false); // Ensure it's enabled. + $clientcertpassphrase.val(''); + $clientcertpassphrase.focus(); + } else { + // Restore the original masked value when unchecked. + if (originalPassphraseValue) { + // Set the value BEFORE Moodle's disabledIf disables the field. + $clientcertpassphrase.prop('disabled', false); // Temporarily enable to set value. + $clientcertpassphrase.val(originalPassphraseValue); + // Small delay to ensure value is set before Moodle's form dependencies disable it. + setTimeout(function() { + // Moodle's disabledIf will handle disabling the field after this. + }, 10); + } + } + }); + + // On page load, if checkbox is unchecked, ensure field shows the masked value. + if (!$changecertpassphrase.is(':checked')) { + if (originalPassphraseValue && $clientcertpassphrase.val() !== originalPassphraseValue) { + $clientcertpassphrase.val(originalPassphraseValue); + } + } + } + + // Initial state: disable "change secret" checkbox if certificate authentication is selected. + if ($changesecret.length && $clientauthmethod.val() == authmethodcertificate) { + $changesecret.prop('disabled', true); + $changesecret.prop('checked', false); + } }; diff --git a/lang/en/auth_oidc.php b/lang/en/auth_oidc.php index b5e3b6b36..38cfb4f85 100644 --- a/lang/en/auth_oidc.php +++ b/lang/en/auth_oidc.php @@ -99,11 +99,16 @@ $string['clientcertfile_help'] = 'When using certificate authentication method and File name certificate source, this is the file name of public key, or certificate, used to authenticate with IdP. The file needs to present in a folder microsoft_certs in the Moodle data folder.'; $string['clientcertpassphrase'] = 'Client certificate passphrase'; $string['clientcertpassphrase_help'] = 'If the client certificate private key is encrypted, this is the passphrase to decrypt it.'; +$string['change_client_secret'] = 'Change client secret'; +$string['change_client_secret_desc'] = 'Check this box to update the client secret. For security, existing secrets are masked and only the first 2 characters are shown.'; +$string['change_cert_passphrase'] = 'Change certificate passphrase'; +$string['change_cert_passphrase_desc'] = 'Check this box to update the certificate passphrase. For security, existing passphrases are masked and only the first 2 characters are shown.'; $string['cfg_domainhint_key'] = 'Domain Hint'; $string['cfg_domainhint_desc'] = 'When using the Authorization Code login flow, pass this value as the "domain_hint" parameter. "domain_hint" is used by some OpenID Connect IdP to make the login process easier for users. Check with your provider to see whether they support this parameter.'; $string['cfg_err_invalidauthendpoint'] = 'Invalid Authorization Endpoint'; $string['cfg_err_invalidtokenendpoint'] = 'Invalid Token Endpoint'; $string['cfg_err_invalidclientid'] = 'Invalid client ID'; +$string['error_masked_secret_not_changed'] = 'Please enter a new value. The masked value cannot be saved.'; $string['cfg_err_invalidclientsecret'] = 'Invalid client secret'; $string['cfg_forceredirect_key'] = 'Force redirect'; $string['cfg_forceredirect_desc'] = 'If enabled, will skip the login index page and redirect to the OpenID Connect page. Can be bypassed with ?noredirect=1 URL param'; diff --git a/lib.php b/lib.php index 3589e8616..dadc04170 100644 --- a/lib.php +++ b/lib.php @@ -849,3 +849,30 @@ function auth_oidc_is_user_sync_enabled() { return false; } + +/** + * Mask a secret value by showing only the first 2 characters followed by asterisks. + * Similar to how Azure Portal masks secrets. + * + * @param string $secret The secret value to mask + * @return string The masked value (e.g., "Ab**********") + */ +function auth_oidc_mask_secret($secret) { + if (empty($secret) || strlen($secret) < 2) { + return '**********'; + } + return substr($secret, 0, 2) . '**********'; +} + +/** + * Check if a value appears to be a masked secret. + * + * @param string $value The value to check + * @return bool True if the value is masked, false otherwise + */ +function auth_oidc_is_masked_secret($value) { + // Check if value matches pattern: exactly 10 asterisks, or 2 chars followed by 10 asterisks. + // This matches both cases: secrets shorter than 2 chars (masked as **********) + // and secrets 2+ chars (masked as XX**********). + return preg_match('/^(.{2})?\*{10}$/', $value) === 1; +} diff --git a/manageapplication.php b/manageapplication.php index 79ce389e7..492e3c689 100644 --- a/manageapplication.php +++ b/manageapplication.php @@ -55,6 +55,11 @@ $form = new application(null, ['oidcconfig' => $oidcconfig]); $formdata = []; +$secretfields = ['clientsecret', 'clientcertpassphrase']; + +// Check if form was submitted (to handle validation errors). +$formsubmitted = optional_param('submitbutton', '', PARAM_TEXT); + foreach ( [ 'idptype', 'clientid', 'clientauthmethod', 'clientsecret', 'clientprivatekey', 'clientcert', @@ -64,7 +69,25 @@ ] as $field ) { if (isset($oidcconfig->$field)) { - $formdata[$field] = $oidcconfig->$field; + // Mask sensitive secret fields for display only. + if (in_array($field, $secretfields) && !empty($oidcconfig->$field)) { + $formdata[$field] = auth_oidc_mask_secret($oidcconfig->$field); + } else { + $formdata[$field] = $oidcconfig->$field; + } + } +} + +// After form validation errors, if the change checkbox wasn't checked, restore the masked value. +if ($formsubmitted) { + $changesecret = optional_param('changesecret', 0, PARAM_BOOL); + $changecertpassphrase = optional_param('changecertpassphrase', 0, PARAM_BOOL); + + if (!$changesecret && !empty($oidcconfig->clientsecret)) { + $formdata['clientsecret'] = auth_oidc_mask_secret($oidcconfig->clientsecret); + } + if (!$changecertpassphrase && !empty($oidcconfig->clientcertpassphrase)) { + $formdata['clientcertpassphrase'] = auth_oidc_mask_secret($oidcconfig->clientcertpassphrase); } } @@ -109,9 +132,37 @@ $settingschanged = false; foreach ($configstosave as $config) { $existingsetting = get_config('auth_oidc', $config); - if ($fromform->$config != $existingsetting) { - add_to_config_log($config, $existingsetting, $fromform->$config, 'auth_oidc'); - set_config($config, $fromform->$config, 'auth_oidc'); + $newvalue = $fromform->$config; + + // Handle secret fields specially. + if (in_array($config, $secretfields)) { + // If the value is a masked secret and hasn't changed, skip updating it. + if (auth_oidc_is_masked_secret($newvalue)) { + // Check if the masked value matches what we would have displayed. + if ($existingsetting && auth_oidc_mask_secret($existingsetting) === $newvalue) { + // Value hasn't been changed, skip this field. + continue; + } + } + + // CRITICAL: Prevent saving empty secret when there's an existing one. + // This handles cases where the field is disabled and submits empty. + if (empty(trim($newvalue)) && !empty($existingsetting)) { + // Don't delete existing secret with empty value - skip this field. + continue; + } + } + + if ($newvalue != $existingsetting) { + // Redact secret fields in the config log to prevent exposing sensitive values. + if (in_array($config, $secretfields)) { + $logoldvalue = !empty($existingsetting) ? '[REDACTED]' : ''; + $lognewvalue = !empty($newvalue) ? '[REDACTED]' : ''; + add_to_config_log($config, $logoldvalue, $lognewvalue, 'auth_oidc'); + } else { + add_to_config_log($config, $existingsetting, $newvalue, 'auth_oidc'); + } + set_config($config, $newvalue, 'auth_oidc'); $settingschanged = true; if ($config != 'secretexpiryrecipients') { $updateapplicationtokenrequired = true; From 744954a2dfde4cb2db7705e00588fc4b0ce58abd Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Wed, 22 Apr 2026 10:26:47 +0100 Subject: [PATCH 15/15] Update plugin version for 5.1.1 release --- version.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/version.php b/version.php index dec77b4a1..806da8749 100644 --- a/version.php +++ b/version.php @@ -25,8 +25,8 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2025100600.01; +$plugin->version = 2025100601; $plugin->requires = 2025100600; -$plugin->release = '5.1.0'; +$plugin->release = '5.1.1'; $plugin->component = 'auth_oidc'; $plugin->maturity = MATURITY_STABLE;