From c48cda035bbc041a0da4a71b9aebde8ed2210409 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Mon, 21 Aug 2023 15:02:44 +1000 Subject: [PATCH 1/3] feat: improve email ip geo location handling --- factor/email/lang/en/factor_email.php | 1 + factor/email/renderer.php | 37 ++++++++- factor/email/tests/factor_test.php | 111 ++++++++++++++++++++++++++ factor/email/version.php | 2 +- 4 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 factor/email/tests/factor_test.php diff --git a/factor/email/lang/en/factor_email.php b/factor/email/lang/en/factor_email.php index 4d46f54d..c6bc3a57 100644 --- a/factor/email/lang/en/factor_email.php +++ b/factor/email/lang/en/factor_email.php @@ -27,6 +27,7 @@ If you clicked this link by accident, click cancel, and no action will be taken.'; $string['email:browseragent'] = 'The browser details for this request are: \'{$a}\''; $string['email:geoinfo'] = 'This request appears to have originated from approximately {$a->city}, {$a->country}.'; +$string['email:geoinfo:unknown'] = 'The location of the request could not be determined.'; $string['email:ipinfo'] = 'IP Information'; $string['email:link'] = 'this link'; $string['email:message'] = 'You are trying to log in to Moodle. Your confirmation code is \'{$a->secret}\'. diff --git a/factor/email/renderer.php b/factor/email/renderer.php index 17fa2044..2f925005 100644 --- a/factor/email/renderer.php +++ b/factor/email/renderer.php @@ -40,8 +40,7 @@ public function generate_email($instanceid) { ['instance' => $instance->id, 'pass' => 1, 'secret' => $instance->secret]); $authurlstring = \html_writer::link($authurl, get_string('email:link', 'factor_email')); $messagestrings = ['secret' => $instance->secret, 'link' => $authurlstring]; - $geoinfo = iplookup_find_location($instance->createdfromip); - $info = ['city' => $geoinfo['city'], 'country' => $geoinfo['country']]; + $blockurl = new \moodle_url('/admin/tool/mfa/factor/email/email.php', ['instance' => $instanceid]); $blockurlstring = \html_writer::link($blockurl, get_string('email:link', 'factor_email')); @@ -51,11 +50,43 @@ public function generate_email($instanceid) { 'message' => get_string('email:message', 'factor_email', $messagestrings), 'ipinformation' => get_string('email:ipinfo', 'factor_email'), 'ip' => get_string('email:originatingip', 'factor_email', $instance->createdfromip), - 'geoinfo' => get_string('email:geoinfo', 'factor_email', $info), + 'geoinfo' => $this->get_ip_location_origin_string($instance->createdfromip ?: ''), 'uadescription' => get_string('email:uadescription', 'factor_email'), 'ua' => $instance->label, 'linkstring' => get_string('email:revokelink', 'factor_email', $blockurlstring), ]; return $this->render_from_template('factor_email/email', $templateinfo); } + + /** + * Finds the location for the given IP address, handling errors. + * + * Returns a user readable string that explains where the request IP originated from. + * + * @param string $ipaddress + * @return string String with IP location details, or a unknown location message if there was an error. + */ + private function get_ip_location_origin_string(string $ipaddress): string { + try { + $geoinfo = iplookup_find_location($ipaddress); + $city = $geoinfo['city'] ?: ''; + $country = $geoinfo['country'] ?: ''; + + // It's possible for errors to be returned, or the geo lookup to simply be empty. + // In these cases, we want to return the 'unknown' string. + $iserror = !empty($geoinfo['error']); + $islookupempty = empty($city) || empty($country); + + if ($iserror || $islookupempty) { + return get_string('email:geoinfo:unknown', 'factor_email'); + } + + // Location info was found - return details. + return get_string('email:geoinfo', 'factor_email', ['city' => $city, 'country' => $country]); + + } catch (Throwable $e) { + // Some exception was thrown, so we cannot work out the location. + return get_string('email:geoinfo:unknown', 'factor_email'); + } + } } diff --git a/factor/email/tests/factor_test.php b/factor/email/tests/factor_test.php new file mode 100644 index 00000000..02b89c1d --- /dev/null +++ b/factor/email/tests/factor_test.php @@ -0,0 +1,111 @@ +. + +namespace factor_email\tests; + +/** + * Tests for TOTP factor. + * + * @covers \factor_email\factor + * @package factor_email + * @author Matthew Hilton + * @copyright Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class factor_test extends \advanced_testcase { + + /** + * Provides to test_generate_email_ip_address_location test. + * @return array + */ + public function generate_email_ip_address_location_provider(): array { + return [ + 'real ip v4' => [ + // Note - this is the same IP address used by core_iplookup_geoplugin_testcase. + 'ip' => '50.0.184.0', + 'isunknown' => false + ], + 'real ip v6' => [ + // Ipv6 is not supported by geoplugin, so it should be treated as unknown. + 'ip' => '2a01:8900:2:3:8c6c:c0db:3d33:9ce6', + 'isunknown' => true + ], + 'empty ip' => [ + 'ip' => '', + 'isunknown' => true + ], + 'malformed ip' => [ + 'ip' => '1.1.1.1.1.1.1.1.1.1', + 'isunknown' => true + ], + 'localhost' => [ + 'ip' => '0.0.0.0', + 'isunknown' => true + ], + 'malformed ip 2' => [ + 'ip' => 'aaaaaa', + 'isunknown' => true + ] + ]; + } + + /** + * Tests the rendererer generate_email function with regard to its + * + * @param string $ip IP address to test + * @param bool $expectedunknown if the IP given should return an email with the unknown message in it. + * + * @dataProvider generate_email_ip_address_location_provider + */ + public function test_generate_email_ip_address_location(string $ip, bool $expectedunknown) { + global $DB, $PAGE; + $this->resetAfterTest(true); + + // Setup user and email factor. + $user = $this->getDataGenerator()->create_user(); + set_config('enabled', 1, 'factor_email'); + $emailfactor = \tool_mfa\plugininfo\factor::get_factor('email'); + + // Manually insert email factor record so that we can edit the IP address. + $record = [ + 'userid' => $user->id, + 'factor' => $emailfactor->name, + 'label' => $user->email, + 'createdfromip' => $ip, + 'timecreated' => time(), + 'revoked' => 0, + ]; + $record['id'] = $DB->insert_record('tool_mfa', $record, true); + + $renderer = $PAGE->get_renderer('factor_email'); + $email = $renderer->generate_email($record['id']); + + // Debugging will be called by the curl to the geoplugin service failing. + // We ignore this in this unit test. + $this->resetDebugging(); + + $unknownstr = get_string('email:geoinfo:unknown', 'factor_email'); + + if ($expectedunknown) { + $this->assertContains($unknownstr, $email); + } else { + // Ideally we would test the email contains the correct geo info, + // but since the geo location of a real IP address can change + // we instead test that the unknown message is not there. + $this->assertNotContains($unknownstr, $email); + } + } +} diff --git a/factor/email/version.php b/factor/email/version.php index 7f60b349..840f64ae 100644 --- a/factor/email/version.php +++ b/factor/email/version.php @@ -26,7 +26,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2019102400; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2023082100; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2017051500.00; // Support back to 3.3 - Totara 12. Patches required. $plugin->component = 'factor_email'; $plugin->release = 'v0.1'; From 3dcedba811f6102d8a656b1ba61b1b8adb4d2c98 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 22 Aug 2023 08:15:10 +1000 Subject: [PATCH 2/3] phpcpd: refactor get_all_user_factors in various factors to own function --- classes/local/factor/object_factor_base.php | 28 +++++++++++++++++++++ factor/auth/classes/factor.php | 19 +------------- factor/capability/classes/factor.php | 19 +------------- factor/cohort/classes/factor.php | 18 +------------ factor/iprange/classes/factor.php | 19 +------------- factor/loginbanner/classes/factor.php | 19 +------------- factor/nosetup/classes/factor.php | 19 +------------- factor/role/classes/factor.php | 19 +------------- 8 files changed, 35 insertions(+), 125 deletions(-) diff --git a/classes/local/factor/object_factor_base.php b/classes/local/factor/object_factor_base.php index 2e66752b..6152ca21 100644 --- a/classes/local/factor/object_factor_base.php +++ b/classes/local/factor/object_factor_base.php @@ -202,6 +202,34 @@ public function get_all_user_factors($user) { return []; } + /** + * Given the user, returns a singleton record for this factor. + * If the record does not exist, it is created. + * + * @param stdClass $user + * @return array array containing the one factor. + */ + protected function get_singleton_user_factor($user) { + global $DB; + $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); + + if (!empty($records)) { + return $records; + } + + // Null records returned, build new record. + $record = [ + 'userid' => $user->id, + 'factor' => $this->name, + 'timecreated' => time(), + 'createdfromip' => $user->lastip, + 'timemodified' => time(), + 'revoked' => 0, + ]; + $record['id'] = $DB->insert_record('tool_mfa', $record, true); + return [(object) $record]; + } + /** * Returns an array of active user factor records. * Filters get_all_user_factors() output. diff --git a/factor/auth/classes/factor.php b/factor/auth/classes/factor.php index dd5c87e8..891f1567 100644 --- a/factor/auth/classes/factor.php +++ b/factor/auth/classes/factor.php @@ -36,24 +36,7 @@ class factor extends object_factor_base { * @return array */ public function get_all_user_factors($user) { - global $DB; - $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); - - if (!empty($records)) { - return $records; - } - - // Null records returned, build new record. - $record = [ - 'userid' => $user->id, - 'factor' => $this->name, - 'timecreated' => time(), - 'createdfromip' => $user->lastip, - 'timemodified' => time(), - 'revoked' => 0, - ]; - $record['id'] = $DB->insert_record('tool_mfa', $record, true); - return [(object) $record]; + return $this->get_singleton_user_factor($user); } /** diff --git a/factor/capability/classes/factor.php b/factor/capability/classes/factor.php index 8cb94d83..45075bdc 100644 --- a/factor/capability/classes/factor.php +++ b/factor/capability/classes/factor.php @@ -36,24 +36,7 @@ class factor extends object_factor_base { * @return array */ public function get_all_user_factors($user) { - global $DB; - $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); - - if (!empty($records)) { - return $records; - } - - // Null records returned, build new record. - $record = [ - 'userid' => $user->id, - 'factor' => $this->name, - 'timecreated' => time(), - 'createdfromip' => $user->lastip, - 'timemodified' => time(), - 'revoked' => 0, - ]; - $record['id'] = $DB->insert_record('tool_mfa', $record, true); - return [(object) $record]; + return $this->get_singleton_user_factor($user); } /** diff --git a/factor/cohort/classes/factor.php b/factor/cohort/classes/factor.php index 61b5ad9b..6379c9f1 100644 --- a/factor/cohort/classes/factor.php +++ b/factor/cohort/classes/factor.php @@ -39,23 +39,7 @@ class factor extends object_factor_base { * @return array */ public function get_all_user_factors($user) { - global $DB; - $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); - if (!empty($records)) { - return $records; - } - - // Null records returned, build new record. - $record = [ - 'userid' => $user->id, - 'factor' => $this->name, - 'timecreated' => time(), - 'createdfromip' => $user->lastip, - 'timemodified' => time(), - 'revoked' => 0, - ]; - $record['id'] = $DB->insert_record('tool_mfa', $record, true); - return [(object) $record]; + return $this->get_singleton_user_factor($user); } /** diff --git a/factor/iprange/classes/factor.php b/factor/iprange/classes/factor.php index 6905174e..324de3bb 100644 --- a/factor/iprange/classes/factor.php +++ b/factor/iprange/classes/factor.php @@ -36,24 +36,7 @@ class factor extends object_factor_base { * @return array */ public function get_all_user_factors($user) { - global $DB; - $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); - - if (!empty($records)) { - return $records; - } - - // Null records returned, build new record. - $record = [ - 'userid' => $user->id, - 'factor' => $this->name, - 'timecreated' => time(), - 'createdfromip' => $user->lastip, - 'timemodified' => time(), - 'revoked' => 0, - ]; - $record['id'] = $DB->insert_record('tool_mfa', $record, true); - return [(object) $record]; + return $this->get_singleton_user_factor($user); } /** diff --git a/factor/loginbanner/classes/factor.php b/factor/loginbanner/classes/factor.php index 644f0b93..eceea538 100644 --- a/factor/loginbanner/classes/factor.php +++ b/factor/loginbanner/classes/factor.php @@ -36,24 +36,7 @@ class factor extends object_factor_base { * @return array */ public function get_all_user_factors($user) { - global $DB; - $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); - - if (!empty($records)) { - return $records; - } - - // Null records returned, build new record. - $record = [ - 'userid' => $user->id, - 'factor' => $this->name, - 'timecreated' => time(), - 'createdfromip' => $user->lastip, - 'timemodified' => time(), - 'revoked' => 0, - ]; - $record['id'] = $DB->insert_record('tool_mfa', $record, true); - return [(object) $record]; + return $this->get_singleton_user_factor($user); } /** diff --git a/factor/nosetup/classes/factor.php b/factor/nosetup/classes/factor.php index 37aeec2d..52e168fc 100644 --- a/factor/nosetup/classes/factor.php +++ b/factor/nosetup/classes/factor.php @@ -36,24 +36,7 @@ class factor extends object_factor_base { * @return array */ public function get_all_user_factors($user) { - global $DB; - $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); - - if (!empty($records)) { - return $records; - } - - // Null records returned, build new record. - $record = [ - 'userid' => $user->id, - 'factor' => $this->name, - 'timecreated' => time(), - 'createdfromip' => $user->lastip, - 'timemodified' => time(), - 'revoked' => 0, - ]; - $record['id'] = $DB->insert_record('tool_mfa', $record, true); - return [(object) $record]; + return $this->get_singleton_user_factor($user); } /** diff --git a/factor/role/classes/factor.php b/factor/role/classes/factor.php index e84ad33f..c3e82e17 100644 --- a/factor/role/classes/factor.php +++ b/factor/role/classes/factor.php @@ -36,24 +36,7 @@ class factor extends object_factor_base { * @return array */ public function get_all_user_factors($user) { - global $DB; - $records = $DB->get_records('tool_mfa', ['userid' => $user->id, 'factor' => $this->name]); - - if (!empty($records)) { - return $records; - } - - // Null records returned, build new record. - $record = [ - 'userid' => $user->id, - 'factor' => $this->name, - 'timecreated' => time(), - 'createdfromip' => $user->lastip, - 'timemodified' => time(), - 'revoked' => 0, - ]; - $record['id'] = $DB->insert_record('tool_mfa', $record, true); - return [(object) $record]; + return $this->get_singleton_user_factor($user); } /** From d72157c8a0e1e4ecbe973a92832a95003f5204da Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Tue, 22 Aug 2023 13:44:04 +1000 Subject: [PATCH 3/3] bugfix: handle IP lookup errors in user preferences --- lang/en/tool_mfa.php | 2 ++ renderer.php | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/lang/en/tool_mfa.php b/lang/en/tool_mfa.php index b93be039..2142b77c 100644 --- a/lang/en/tool_mfa.php +++ b/lang/en/tool_mfa.php @@ -71,6 +71,8 @@ $string['guidance'] = 'MFA user guide'; $string['inputrequired'] = 'User input'; $string['ipatcreation'] = 'IP address when factor created'; +$string['ip:geoinfo:unknown'] = '{$a} - Location unknown'; +$string['ip:geoinfo'] = '{$a->ip} - {$a->city}, {$a->country}'; $string['lastverified'] = 'Last verified'; $string['lockedusersforallfactors'] = 'Locked users: All factors'; $string['lockedusersforfactor'] = 'Locked users: {$a}'; diff --git a/renderer.php b/renderer.php index 7dbb5b74..2114bc64 100644 --- a/renderer.php +++ b/renderer.php @@ -176,9 +176,8 @@ public function active_factors() { $lastverified .= get_string('ago', 'core_message', format_time(time() - $userfactor->lastverified)); } - $info = iplookup_find_location($userfactor->createdfromip); $ip = $userfactor->createdfromip; - $ip .= '
' . $info['country'] . ' - ' . $info['city']; + $ip = $this->append_location_to_ip($ip); $row = new \html_table_row([ $factor->get_display_name(), @@ -201,6 +200,38 @@ public function active_factors() { return $html; } + /** + * Finds the location for the given IP address, handling errors. + * + * Appends location info to the end of the IP address. + * + * @param string $ip + * @return string String with the IP with location details appended to it, or Unknown location if could not be determined. + */ + private function append_location_to_ip($ip): string { + try { + $geoinfo = iplookup_find_location($ip); + $city = $geoinfo['city'] ?: ''; + $country = $geoinfo['country'] ?: ''; + + // It's possible for errors to be returned, or the geo lookup to simply be empty. + // In these cases, we want to return the 'unknown' string. + $iserror = !empty($geoinfo['error']); + $islookupempty = empty($city) || empty($country); + + if ($iserror || $islookupempty) { + return get_string('ip:geoinfo:unknown', 'tool_mfa', $ip); + } + + // Location info was found - return details. + return get_string('ip:geoinfo', 'tool_mfa', ['ip' => $ip, 'city' => $city, 'country' => $country]); + + } catch (Throwable $e) { + // Some exception was thrown, so we cannot work out the location. + return get_string('ip:geoinfo:unknown', 'tool_mfa', $ip); + } + } + /** * Generates notification text for display when user cannot login. *