diff --git a/classes/form/application.php b/classes/form/application.php index 98eb8cb..cacd2b0 100644 --- a/classes/form/application.php +++ b/classes/form/application.php @@ -90,6 +90,11 @@ protected function definition() { $mform->disabledIf('clientsecret', 'clientauthmethod', 'neq', AUTH_OIDC_AUTH_METHOD_SECRET); $mform->addElement('static', 'clientsecret_help', '', get_string('clientsecret_help', 'auth_oidc')); + // Auth token secret toggle. + $mform->addElement('advcheckbox', 'accesstokenclientsecret', auth_oidc_config_name_in_form('accesstokenclientsecret'), ''); + $mform->setType('accesstokenclientsecret', PARAM_BOOL); + $mform->addElement('static', 'accesstokenclientsecret_help', '', get_string('accesstokenclientsecret_help', 'auth_oidc')); + // Certificate private key. $mform->addElement('textarea', 'clientprivatekey', auth_oidc_config_name_in_form('clientprivatekey'), ['rows' => 10, 'cols' => 80]); diff --git a/classes/loginflow/authcode.php b/classes/loginflow/authcode.php index a5b41fd..263fbd4 100644 --- a/classes/loginflow/authcode.php +++ b/classes/loginflow/authcode.php @@ -617,14 +617,44 @@ protected function handlelogin(string $oidcuniqid, array $authparams, array $tok } $username = trim(\core_text::strtolower($username)); $tokenrec = $this->createtoken($oidcuniqid, $username, $authparams, $tokenparams, $idtoken, 0, $originalupn); + $userinfo = $this->get_userinfo($username); + + $matchingmethod = get_config('auth_oidc', 'subjectmapping'); + // Email can only be used when allowaccountssameemail is off. + if ($matchingmethod === 'email' && ($CFG->allowaccountssameemail || !array_key_exists('email', $userinfo))) { + // Fall back to username in this case. + $matchingmethod = 'username'; + } + + switch ($matchingmethod) { + case 'idnumberemail': + $isemail = filter_var($username, FILTER_VALIDATE_EMAIL); + $existinguserparams = ['mnethostid' => $CFG->mnet_localhost_id]; + if ($isemail === false) { + $existinguserparams['idnumber'] = $username; + } else { + $existinguserparams['email'] = $username; + } + break; + case 'idnumber': + // In this case, we are treating the "username" from OIDC as the idnumber + $existinguserparams = ['idnumber' => $username, 'mnethostid' => $CFG->mnet_localhost_id]; + break; + case 'email': + $existinguserparams = ['email' => $userinfo['email'], 'mnethostid' => $CFG->mnet_localhost_id]; + break; + case 'username': + default: + $existinguserparams = ['username' => $username, 'mnethostid' => $CFG->mnet_localhost_id]; + break; + } - $existinguserparams = ['username' => $username, 'mnethostid' => $CFG->mnet_localhost_id]; if ($DB->record_exists('user', $existinguserparams) !== true) { // User does not exist. Create user if site allows, otherwise fail. if (empty($CFG->authpreventaccountcreation)) { if (!$CFG->allowaccountssameemail) { - $userinfo = $this->get_userinfo($username); - if ($DB->count_records('user', array('email' => $userinfo['email'], 'deleted' => 0)) > 0) { + if (array_key_exists('email', $userinfo) + && ($DB->count_records('user', array('email' => $userinfo['email'], 'deleted' => 0)) > 0)) { throw new moodle_exception('errorauthloginfaileddupemail', 'auth_oidc', null, null, '1'); } } @@ -638,7 +668,32 @@ protected function handlelogin(string $oidcuniqid, array $authparams, array $tok throw new moodle_exception('errorauthloginfailednouser', 'auth_oidc', null, null, '1'); } } - + // Ensure user objects are in the right shape for future matching before proceeding. + if (array_key_exists('idnumber', $existinguserparams)) { + if (isset($user)) { + // The user has been created, but we need to make the idnumber bound as well. + $user->auth = 'oidc'; + $user->idnumber = $username; + user_update_user($user, false); + } + $founduser = $DB->get_record('user', ['idnumber' => $username]); + // Now update the token to match the found username. + $DB->set_field('auth_oidc_token', 'username', $founduser->username, ['oidcuniqid' => $username]); + $username = $founduser->username; + } else if (array_key_exists('email', $existinguserparams)) { + // We can only match on email if allowaccountssameemail is off. + if (!$CFG->allowaccountssameemail && array_key_exists('email', $userinfo)) { + // Here we know there is atleast 1 account with that email. + // We can just get the first match which is safe as long as allowaccountssameemail is off. + $founduser = $DB->get_record('user', ['email' => $userinfo['email']]); + // Before proceeding, set the user to OIDC if they aren't already. + $founduser->auth = 'oidc'; + user_update_user($founduser, false); + // Now update the token to match the found username. + $DB->set_field('auth_oidc_token', 'username', $founduser->username, ['oidcuniqid' => $username]); + $username = $founduser->username; + } + } $user = authenticate_user_login($username, null, true); if (!empty($user)) { diff --git a/classes/loginflow/base.php b/classes/loginflow/base.php index 61b0b74..6536595 100644 --- a/classes/loginflow/base.php +++ b/classes/loginflow/base.php @@ -257,20 +257,23 @@ public function get_userinfo($username) { if (!isset($userdata['email'])) { $email = $token->claim('email'); if (!empty($email)) { - $userdata['mail'] = $email; + $userdata['email'] = $email; } else { if (!empty($upn)) { $aademailvalidateresult = filter_var($upn, FILTER_VALIDATE_EMAIL); if (!empty($aademailvalidateresult)) { - $userdata['mail'] = $aademailvalidateresult; + $userdata['email'] = $aademailvalidateresult; } } } } } - $updateduser = static::apply_configured_fieldmap_from_token($userdata, $eventtype); + $updateduser = static::apply_configured_fieldmap_from_token($userdata, $eventtype, $token); $userinfo = (array)$updateduser; + if (!empty($userinfo['email'])) { + $userinfo['email'] = strtolower($userinfo['email']); + } } return $userinfo; @@ -281,9 +284,10 @@ public function get_userinfo($username) { * * @param array $userdata * @param string $eventtype + * @param jwt $token * @return stdClass */ - public static function apply_configured_fieldmap_from_token(array $userdata, string $eventtype) { + public static function apply_configured_fieldmap_from_token(array $userdata, string $eventtype, jwt $token) { $user = new stdClass(); $fieldmappings = auth_oidc_get_field_mappings(); @@ -292,13 +296,19 @@ public static function apply_configured_fieldmap_from_token(array $userdata, str $remotefield = $fieldmapping['field_map']; $behavior = $fieldmapping['update_local']; - if ($behavior !== 'on' . $eventtype && $behavior !== 'always') { + if ($behavior === 'on_create' && $eventtype !== 'create') { // Field mapping doesn't apply to this event type. continue; } if (isset($userdata[$remotefield])) { $user->$localfield = $userdata[$remotefield]; + } else { + // Try a manual token claim on the value provided. + $tokenval = $token->claim($remotefield); + if (!is_null($tokenval)) { + $user->$localfield = $tokenval; + } } } diff --git a/classes/oidcclient.php b/classes/oidcclient.php index 6121ba9..226a45c 100644 --- a/classes/oidcclient.php +++ b/classes/oidcclient.php @@ -333,6 +333,7 @@ public function tokenrequest($code) { 'redirect_uri' => $this->redirecturi, ]; + $sendsecret = get_config('auth_oidc', 'accesstokenclientsecret'); switch (get_config('auth_oidc', 'clientauthmethod')) { case AUTH_OIDC_AUTH_METHOD_CERTIFICATE: $params['client_assertion_type'] = 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer'; @@ -340,7 +341,7 @@ public function tokenrequest($code) { $params['tenant'] = 'common'; break; default: - $params['client_secret'] = $this->clientsecret; + $params['client_secret'] = $sendsecret ? $this->clientsecret : null; } $returned = $this->httpclient->post($this->endpoints['token'], $params); return utils::process_json_response($returned, ['id_token' => null]); @@ -365,6 +366,7 @@ public function app_access_token_request() { break; default: $params['client_secret'] = $this->clientsecret; + $params = []; } $tokenendpoint = $this->endpoints['token']; diff --git a/lang/en/auth_oidc.php b/lang/en/auth_oidc.php index 241837b..2002a29 100644 --- a/lang/en/auth_oidc.php +++ b/lang/en/auth_oidc.php @@ -60,6 +60,8 @@ $string['idp_type_microsoft'] = 'Microsoft identity platform (v2.0)'; $string['idp_type_other'] = 'Other'; $string['cfg_authenticationlink_desc'] = 'Link to IdP and authentication configuration'; +$string['accesstokenclientsecret'] = 'Send client secret in access token requests.'; +$string['accesstokenclientsecret_help'] = 'Some IdPs like Azure B2C do not allow the client secret to be sent in token exchange requests. Disable this config to remove the client secret from the request.'; $string['authendpoint'] = 'Authorization Endpoint'; $string['authendpoint_help'] = 'The URI of the Authorization endpoint from your IdP to use.
Note if the site is to be configured to allow users from other tenants to access, tenant specific authorization endpoint cannot be used.'; @@ -83,6 +85,8 @@ $string['clientcert_help'] = 'When using certificate authentication method, this is the public key, or certificate, used in to authenticate with IdP.'; $string['tenantnameorguid'] = 'Tenant name or GUID'; $string['tenantnameorguid_help'] = 'Don\'t include https:// if use tenant name.'; +$string['cfg_custom_mapping'] = 'Use custom data mappings'; +$string['cfg_custom_mapping_desc'] = 'Allow custom attribute entry for data mappings.'; $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'; @@ -325,3 +329,8 @@ $string['settings_fieldmap_field_sds_student_studentNumber'] = 'SDS student number'; $string['settings_fieldmap_field_sds_teacher_externalId'] = 'SDS teacher external ID'; $string['settings_fieldmap_field_sds_teacher_teacherNumber'] = 'SDS teacher number'; + +// Custom strings +$string['subjectmapping'] = 'User matching attribute'; +$string['subjectmapping_desc'] = 'Select the user attribute in the platform to match against the incoming claim. Email will be matched to the \'email\' field in the claim. Username and ID Number will be matched to the subject field in the claim. The IDNumber & Email setting will try to infer the shape of the subject field: If an email address is supplied, it will match against the platform email field, otherwise against the user IDNumber.'; +$string['idnumberemail'] = 'IDNumber & Email'; diff --git a/manageapplication.php b/manageapplication.php index 5d1b43b..8e53956 100644 --- a/manageapplication.php +++ b/manageapplication.php @@ -55,7 +55,7 @@ $form = new application(null, ['oidcconfig' => $oidcconfig]); $formdata = []; -foreach (['idptype', 'clientid', 'clientauthmethod', 'clientsecret', 'clientprivatekey', 'clientcert', 'tenantnameorguid', +foreach (['idptype', 'clientid', 'clientauthmethod', 'clientsecret', 'accesstokenclientsecret', 'clientprivatekey', 'clientcert', 'tenantnameorguid', 'authendpoint', 'tokenendpoint', 'oidcresource', 'oidcscope'] as $field) { if (isset($oidcconfig->$field)) { $formdata[$field] = $oidcconfig->$field; @@ -73,7 +73,7 @@ } // Prepare config settings to save. - $configstosave = ['idptype', 'clientid', 'tenantnameorguid', 'clientauthmethod', 'authendpoint', 'tokenendpoint', + $configstosave = ['idptype', 'clientid', 'tenantnameorguid', 'clientauthmethod', 'accesstokenclientsecret', 'authendpoint', 'tokenendpoint', 'oidcresource', 'oidcscope']; // Depending on the value of clientauthmethod, save clientsecret or (clientprivatekey and clientcert). @@ -110,4 +110,3 @@ $form->display(); echo $OUTPUT->footer(); - diff --git a/settings.php b/settings.php index d4e85cd..364a253 100644 --- a/settings.php +++ b/settings.php @@ -225,12 +225,29 @@ // Other settings page and its settings. $fieldmappingspage = new admin_settingpage('auth_oidc_field_mapping', get_string('settings_page_field_mapping', 'auth_oidc')); + $fieldmappingspage->add(new admin_setting_configcheckbox('auth_oidc/custom_field_mapping', + get_string('cfg_custom_mapping', 'auth_oidc'), get_string('cfg_custom_mapping_desc', 'auth_oidc'), 0)); + $options = [ + 'username' => get_string('username'), + 'email' => get_string('email'), + 'idnumber' => get_string('idnumber'), + 'idnumberemail' => get_string('idnumberemail', 'auth_oidc'), + ]; + $fieldmappingspage->add(new admin_setting_configselect('auth_oidc/subjectmapping', + get_string('subjectmapping', 'auth_oidc'), + get_string('subjectmapping_desc', 'auth_oidc'), 'username', $options)); $ADMIN->add('oidcfolder', $fieldmappingspage); // Display locking / mapping of profile fields. $authplugin = get_auth_plugin('oidc'); - auth_oidc_display_auth_lock_options($fieldmappingspage, $authplugin->authtype, $authplugin->userfields, - get_string('cfg_field_mapping_desc', 'auth_oidc'), true, false, $authplugin->get_custom_user_profile_fields()); + if (get_config('auth_oidc', 'custom_field_mapping')) { + display_auth_lock_options($fieldmappingspage, $authplugin->authtype, $authplugin->userfields, + get_string('cfg_field_mapping_desc', 'auth_oidc'), true, false, $authplugin->get_custom_user_profile_fields()); + } else { + auth_oidc_display_auth_lock_options($fieldmappingspage, $authplugin->authtype, $authplugin->userfields, + get_string('cfg_field_mapping_desc', 'auth_oidc'), true, false, $authplugin->get_custom_user_profile_fields()); + } + } $settings = null; diff --git a/tests/privacy_provider_test.php b/tests/privacy_provider_test.php deleted file mode 100644 index 3c9b96f..0000000 --- a/tests/privacy_provider_test.php +++ /dev/null @@ -1,300 +0,0 @@ -. - -/** - * Privacy test for auth_oidc - * - * @package auth_oidc - * @author Remote-Learner.net Inc - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - * @copyright (C) 2019 Remote Learner.net Inc http://www.remote-learner.net - */ - -defined('MOODLE_INTERNAL') || die(); - -use \auth_oidc\privacy\provider; - -/** - * Privacy test for auth_oidc - * - * @group auth_oidc - * @group auth_oidc_privacy - * @group office365 - * @group office365_privacy - */ -class auth_oidc_privacy_testcase extends \core_privacy\tests\provider_testcase { - /** - * Tests set up. - */ - public function setUp():void { - global $CFG; - $this->resetAfterTest(); - $this->setAdminUser(); - } - - /** - * Check that a user context is returned if there is any user data for this user. - */ - public function test_get_contexts_for_userid() { - $user = $this->getDataGenerator()->create_user(); - $this->assertEmpty(provider::get_contexts_for_userid($user->id)); - - // Create user records. - self::create_token($user->id); - self::create_prevlogin($user->id); - - $contextlist = provider::get_contexts_for_userid($user->id); - // Check that we only get back one context. - $this->assertCount(1, $contextlist); - - // Check that a context is returned and is the expected context. - $usercontext = \context_user::instance($user->id); - $this->assertEquals($usercontext->id, $contextlist->get_contextids()[0]); - } - - /** - * Test that only users with a user context are fetched. - */ - public function test_get_users_in_context() { - $this->resetAfterTest(); - - $component = 'auth_oidc'; - // Create a user. - $user = $this->getDataGenerator()->create_user(); - $usercontext = context_user::instance($user->id); - - // The list of users should not return anything yet (related data still haven't been created). - $userlist = new \core_privacy\local\request\userlist($usercontext, $component); - provider::get_users_in_context($userlist); - $this->assertCount(0, $userlist); - - // Create user records. - self::create_token($user->id); - self::create_prevlogin($user->id); - - // The list of users for user context should return the user. - provider::get_users_in_context($userlist); - $this->assertCount(1, $userlist); - $expected = [$user->id]; - $actual = $userlist->get_userids(); - $this->assertEquals($expected, $actual); - - // The list of users for system context should not return any users. - $userlist = new \core_privacy\local\request\userlist(context_system::instance(), $component); - provider::get_users_in_context($userlist); - $this->assertCount(0, $userlist); - } - - /** - * Test that user data is exported correctly. - */ - public function test_export_user_data() { - // Create a user record. - $user = $this->getDataGenerator()->create_user(); - $tokenrecord = self::create_token($user->id); - $prevloginrecord = self::create_prevlogin($user->id); - - $usercontext = \context_user::instance($user->id); - - $writer = \core_privacy\local\request\writer::with_context($usercontext); - $this->assertFalse($writer->has_any_data()); - $approvedlist = new core_privacy\local\request\approved_contextlist($user, 'auth_oidc', [$usercontext->id]); - provider::export_user_data($approvedlist); - // Token. - $data = $writer->get_data([ - get_string('privacy:metadata:auth_oidc', 'auth_oidc'), - get_string('privacy:metadata:auth_oidc_token', 'auth_oidc') - ]); - $this->assertEquals($tokenrecord->userid, $data->userid); - $this->assertEquals($tokenrecord->token, $data->token); - // Previous login. - $data = $writer->get_data([ - get_string('privacy:metadata:auth_oidc', 'auth_oidc'), - get_string('privacy:metadata:auth_oidc_prevlogin', 'auth_oidc') - ]); - $this->assertEquals($prevloginrecord->userid, $data->userid); - $this->assertEquals($prevloginrecord->method, $data->method); - $this->assertEquals($prevloginrecord->password, $data->password); - } - - /** - * Test deleting all user data for a specific context. - */ - public function test_delete_data_for_all_users_in_context() { - global $DB; - - // Create a user record. - $user1 = $this->getDataGenerator()->create_user(); - self::create_token($user1->id); - self::create_prevlogin($user1->id); - $user1context = \context_user::instance($user1->id); - - $user2 = $this->getDataGenerator()->create_user(); - self::create_token($user2->id); - self::create_prevlogin($user2->id); - - // Get all accounts. There should be two. - $this->assertCount(2, $DB->get_records('auth_oidc_token', [])); - $this->assertCount(2, $DB->get_records('auth_oidc_prevlogin', [])); - - // Delete everything for the first user context. - provider::delete_data_for_all_users_in_context($user1context); - - $this->assertCount(0, $DB->get_records('auth_oidc_token', ['userid' => $user1->id])); - $this->assertCount(0, $DB->get_records('auth_oidc_prevlogin', ['userid' => $user1->id])); - - // Get all accounts. There should be one. - $this->assertCount(1, $DB->get_records('auth_oidc_token', [])); - $this->assertCount(1, $DB->get_records('auth_oidc_prevlogin', [])); - } - - /** - * This should work identical to the above test. - */ - public function test_delete_data_for_user() { - global $DB; - - // Create a user record. - $user1 = $this->getDataGenerator()->create_user(); - self::create_token($user1->id); - self::create_prevlogin($user1->id); - $user1context = \context_user::instance($user1->id); - - $user2 = $this->getDataGenerator()->create_user(); - self::create_token($user2->id); - self::create_prevlogin($user2->id); - - // Get all accounts. There should be two. - $this->assertCount(2, $DB->get_records('auth_oidc_token', [])); - $this->assertCount(2, $DB->get_records('auth_oidc_prevlogin', [])); - - // Delete everything for the first user. - $approvedlist = new \core_privacy\local\request\approved_contextlist($user1, 'auth_oidc', [$user1context->id]); - provider::delete_data_for_user($approvedlist); - - $this->assertCount(0, $DB->get_records('auth_oidc_token', ['userid' => $user1->id])); - $this->assertCount(0, $DB->get_records('auth_oidc_prevlogin', ['userid' => $user1->id])); - - // Get all accounts. There should be one. - $this->assertCount(1, $DB->get_records('auth_oidc_token', [])); - $this->assertCount(1, $DB->get_records('auth_oidc_prevlogin', [])); - } - - /** - * Test that data for users in approved userlist is deleted. - */ - public function test_delete_data_for_users() { - $this->resetAfterTest(); - - $component = 'auth_oidc'; - // Create user1. - $user1 = $this->getDataGenerator()->create_user(); - $usercontext1 = context_user::instance($user1->id); - self::create_token($user1->id); - self::create_prevlogin($user1->id); - - // Create user2. - $user2 = $this->getDataGenerator()->create_user(); - $usercontext2 = context_user::instance($user2->id); - self::create_token($user2->id); - self::create_prevlogin($user2->id); - - // The list of users for usercontext1 should return user1. - $userlist1 = new \core_privacy\local\request\userlist($usercontext1, $component); - provider::get_users_in_context($userlist1); - $this->assertCount(1, $userlist1); - $expected = [$user1->id]; - $actual = $userlist1->get_userids(); - $this->assertEquals($expected, $actual); - - // The list of users for usercontext2 should return user2. - $userlist2 = new \core_privacy\local\request\userlist($usercontext2, $component); - provider::get_users_in_context($userlist2); - $this->assertCount(1, $userlist2); - $expected = [$user2->id]; - $actual = $userlist2->get_userids(); - $this->assertEquals($expected, $actual); - - // Add userlist1 to the approved user list. - $approvedlist = new \core_privacy\local\request\approved_userlist($usercontext1, $component, $userlist1->get_userids()); - - // Delete user data using delete_data_for_user for usercontext1. - provider::delete_data_for_users($approvedlist); - - // Re-fetch users in usercontext1 - The user list should now be empty. - $userlist1 = new \core_privacy\local\request\userlist($usercontext1, $component); - provider::get_users_in_context($userlist1); - $this->assertCount(0, $userlist1); - // Re-fetch users in usercontext2 - The user list should not be empty (user2). - $userlist2 = new \core_privacy\local\request\userlist($usercontext2, $component); - provider::get_users_in_context($userlist2); - $this->assertCount(1, $userlist2); - - // User data should be only removed in the user context. - $systemcontext = context_system::instance(); - // Add userlist2 to the approved user list in the system context. - $approvedlist = new \core_privacy\local\request\approved_userlist($systemcontext, $component, $userlist2->get_userids()); - // Delete user1 data using delete_data_for_user. - provider::delete_data_for_users($approvedlist); - // Re-fetch users in usercontext2 - The user list should not be empty (user2). - $userlist2 = new \core_privacy\local\request\userlist($usercontext2, $component); - provider::get_users_in_context($userlist2); - $this->assertCount(1, $userlist2); - } - - /** - * Create a token record for the specified userid. - * - * @param int $userid - * @return stdClass - * @throws dml_exception - */ - static private function create_token(int $userid) : \stdClass { - global $DB; - $record = new stdClass(); - $record->oidcuniqid = "user@example.com"; - $record->username = "user@example.com"; - $record->userid = $userid; - $record->oidcusername = "user@example.com"; - $record->scope = "All"; - $record->tokenresource = "https://graph.microsoft.com"; - $record->authcode = "authcode123"; - $record->token = "token123"; - $record->expiry = 12345; - $record->refreshtoken = "refresh123"; - $record->idtoken = "idtoken123"; - $record->id = $DB->insert_record('auth_oidc_token', $record); - return $record; - } - - /** - * Create a previous login record for the specified userid. - * - * @param int $userid - * @return stdClass - * @throws dml_exception - */ - static private function create_prevlogin(int $userid) : \stdClass { - global $DB; - $record = new stdClass(); - $record->userid = $userid; - $record->method = "manual"; - $record->password = "abc123"; - $record->id = $DB->insert_record('auth_oidc_prevlogin', $record); - return $record; - } - -} diff --git a/version.php b/version.php index ce94fde..a2736d4 100644 --- a/version.php +++ b/version.php @@ -25,7 +25,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2022112800; +$plugin->version = 2022112802; $plugin->requires = 2016120500; $plugin->release = '4.1.0'; $plugin->component = 'auth_oidc';