From 833a25daf354d3773fb523ec9be8618bcc045ac7 Mon Sep 17 00:00:00 2001
From: Andrew Hancox <andrewdchancox@googlemail.com>
Date: Tue, 3 Oct 2017 18:03:02 +0100
Subject: [PATCH] Fixed issues from code review

---
 auth.php                         |  9 ++++-----
 classes/core_userkey_manager.php |  6 +++---
 tests/auth_plugin_test.php       | 30 ++----------------------------
 3 files changed, 9 insertions(+), 36 deletions(-)

diff --git a/auth.php b/auth.php
index a356ad0..04502ef 100644
--- a/auth.php
+++ b/auth.php
@@ -30,7 +30,6 @@ use auth_userkey\userkey_manager_interface;
 require_once($CFG->libdir . "/externallib.php");
 require_once($CFG->libdir.'/authlib.php');
 require_once($CFG->dirroot . '/user/lib.php');
-require_once($CFG->dirroot . '/lib/classes/user.php');
 
 /**
  * User key authentication plugin.
@@ -605,14 +604,14 @@ class auth_plugin_userkey extends auth_plugin_base {
 
         $mappingfield = $this->get_mapping_field();
         if ($this->should_create_user() || $this->should_update_user()) {
-            $parameters['firstname'] = new external_value(core_user::get_property_type('firstname'), 'The first name(s) of the user', VALUE_OPTIONAL);
-            $parameters['lastname']  = new external_value(core_user::get_property_type('lastname'), 'The family name of the user', VALUE_OPTIONAL);
+            $parameters['firstname'] = new external_value(PARAM_NOTAGS, 'The first name(s) of the user', VALUE_OPTIONAL);
+            $parameters['lastname']  = new external_value(PARAM_NOTAGS, 'The family name of the user', VALUE_OPTIONAL);
 
             if ($mappingfield != 'email') {
-                $parameters['email'] = new external_value(core_user::get_property_type('email'), 'A valid and unique email address', VALUE_OPTIONAL);
+                $parameters['email'] = new external_value(PARAM_RAW_TRIMMED, 'A valid and unique email address', VALUE_OPTIONAL);
             }
             if ($mappingfield != 'username') {
-                $parameters['username'] = new external_value(core_user::get_property_type('username'), 'A valid and unique username', VALUE_OPTIONAL);
+                $parameters['username'] = new external_value(PARAM_USERNAME, 'A valid and unique username', VALUE_OPTIONAL);
             }
         }
 
diff --git a/classes/core_userkey_manager.php b/classes/core_userkey_manager.php
index d6e6dfc..3a9102f 100644
--- a/classes/core_userkey_manager.php
+++ b/classes/core_userkey_manager.php
@@ -125,7 +125,9 @@ class core_userkey_manager implements userkey_manager_interface {
         if ($key->iprestriction) {
             $remoteaddr = getremoteaddr(null);
             $whitelist = get_config('auth_userkey', 'ipwhitelist');
-            if (!empty($whitelist)) {
+            if (empty($remoteaddr) ) {
+                print_error('noip', 'auth_userkey');
+            } else if (!empty($whitelist)) {
                 $ips = explode(';', $whitelist);
                 $whitelisted = false;
                 foreach ($ips as $ip) {
@@ -136,8 +138,6 @@ class core_userkey_manager implements userkey_manager_interface {
                 if (!$whitelisted) {
                     print_error('ipmismatch', 'error', '', null, "Remote address: $remoteaddr\nKey IP: $key->iprestriction");
                 }
-            } else if (empty($remoteaddr) ) {
-                print_error('noip', 'auth_userkey');
             } else if (!address_in_subnet($remoteaddr, $key->iprestriction)) {
                 print_error('ipmismatch', 'error', '', null, "Remote address: $remoteaddr\nKey IP: $key->iprestriction");
             }
diff --git a/tests/auth_plugin_test.php b/tests/auth_plugin_test.php
index e061419..4c0ca35 100644
--- a/tests/auth_plugin_test.php
+++ b/tests/auth_plugin_test.php
@@ -844,7 +844,7 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
     }
 
     /**
-     * Test that IP address mismatch exception gets thrown if incorrect IP.
+     * Test that IP address mismatch is ingored if IP is whitelisted.
      *
      * @expectedException moodle_exception
      * @expectedExceptionMessage Unsupported redirect to http://www.example.com/moodle detected, execution terminated.
@@ -870,33 +870,7 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
     }
 
     /**
-     * Test that IP address mismatch exception gets thrown if incorrect IP.
-     *
-     * @expectedException moodle_exception
-     * @expectedExceptionMessage Unsupported redirect to http://www.example.com/moodle detected, execution terminated.
-     */
-    public function test_ipmismatch_exception_notthrown_if_ip_is_whitelisted_temp() {
-        global $DB;
-
-        set_config('ipwhitelist', '192.168.1.2', 'auth_userkey');
-
-        $key = new stdClass();
-        $key->value = 'IpmismatchKey';
-        $key->script = 'auth/userkey';
-        $key->userid = $this->user->id;
-        $key->instance = $this->user->id;
-        $key->iprestriction = '192.168.1.1';
-        $key->validuntil    = time() + 300;
-        $key->timecreated   = time();
-        $DB->insert_record('user_private_key', $key);
-
-        $_POST['key'] = 'IpmismatchKey';
-        $_SERVER['HTTP_CLIENT_IP'] = '192.168.1.2';
-        @$this->auth->user_login_userkey();
-    }
-
-    /**
-     * Test that IP address mismatch exception gets thrown if incorrect IP.
+     * Test that IP address mismatch exception gets thrown if incorrect IP and outside whitelist.
      *
      * @expectedException moodle_exception
      * @expectedExceptionMessage Client IP address mismatch