Merge pull request #429 from catalyst/better-ip-handling-400

Better ip handling 400

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.

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);
     }
 
     /**

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);
     }
 
     /**

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);
     }
 
     /**

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}\'.

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');
+        }
+    }
 }

factor/email/tests/factor_test.php

@@ -0,0 +1,111 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace factor_email\tests;
+
+/**
+ * Tests for TOTP factor.
+ *
+ * @covers      \factor_email\factor
+ * @package     factor_email
+ * @author      Matthew Hilton <[email protected]>
+ * @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);
+        }
+    }
+}

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     = 2022041908; // Support Moodle 4.0 and higher.
 $plugin->component    = 'factor_email';
 $plugin->release      = 'v0.1';

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);
     }
 
     /**

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);
     }
 
     /**

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);
     }
 
     /**

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);
     }
 
     /**