Merge pull request #20 from andrewhancox/master

Bug in whitelisting logic
This commit is contained in:
Dmitrii Metelkin 2017-12-13 09:10:15 +11:00 committed by GitHub
commit 747b4d301c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 68 additions and 32 deletions

View file

@ -122,38 +122,45 @@ class core_userkey_manager implements userkey_manager_interface {
print_error('expiredkey');
}
if ($key->iprestriction) {
$remoteaddr = getremoteaddr(null);
if (isset($this->config->ipwhitelist)) {
$whitelist = $this->config->ipwhitelist;
} else {
$whitelist = false;
}
if (empty($remoteaddr) ) {
print_error('noip', 'auth_userkey');
} else if (!empty($whitelist)) {
$ips = explode(';', $whitelist);
$whitelisted = false;
foreach ($ips as $ip) {
if (address_in_subnet($remoteaddr, $ip)) {
$whitelisted = true;
}
}
if (!$whitelisted) {
print_error('ipmismatch', 'error', '', null, "Remote address: $remoteaddr\nKey IP: $key->iprestriction");
}
} else if (!address_in_subnet($remoteaddr, $key->iprestriction)) {
print_error('ipmismatch', 'error', '', null, "Remote address: $remoteaddr\nKey IP: $key->iprestriction");
}
}
$this->validate_ip_address($key);
if (!$user = $DB->get_record('user', array('id' => $key->userid))) {
print_error('invaliduserid');
}
return $key;
}
/**
* Validates key IP address and returns true if valid.
*
* @param object $key Key object including userid property.
*
* @throws \moodle_exception If provided key is not valid.
*/
protected function validate_ip_address($key) {
if (!$key->iprestriction) {
return true;
}
$remoteaddr = getremoteaddr(null);
if (empty($remoteaddr)) {
print_error('noip', 'auth_userkey');
}
if (address_in_subnet($remoteaddr, $key->iprestriction)) {
return true;
}
if (isset($this->config->ipwhitelist)) {
$ips = explode(';', $this->config->ipwhitelist);
foreach ($ips as $ip) {
if (address_in_subnet($remoteaddr, $ip)) {
return true;
}
}
}
print_error('ipmismatch', 'error', '', null, "Remote address: $remoteaddr\nKey IP: $key->iprestriction");
}
}

View file

@ -311,7 +311,11 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
/**
* Test that we can request a key for a new user.
*
* @expectedException \invalid_parameter_exception
* @expectedExceptionMessage Unable to create user, missing value(s): username,firstname,lastname
*/
public function test_missing_data_to_create_user() {
global $CFG, $DB;
@ -325,12 +329,13 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
$user->email = 'username@test.com';
$user->ip = '192.168.1.1';
$this->setExpectedException('invalid_parameter_exception', 'Unable to create user, missing value(s): username,firstname,lastname');
$this->auth->get_login_url($user);
}
/**
* Test that when we attempt to create a new user duplicate usernames are caught.
* @expectedException \invalid_parameter_exception
* @expectedExceptionMessage Username already exists: username
*/
public function test_create_refuse_duplicate_username() {
set_config('createuser', true, 'auth_userkey');
@ -352,12 +357,14 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
$duplicateuser = clone($originaluser);
$duplicateuser->email = 'duplicateuser@test.com';
$this->setExpectedException('invalid_parameter_exception', 'Username already exists: username');
$this->auth->get_login_url($duplicateuser);
}
/**
* Test that when we attempt to create a new user duplicate emails are caught.
*
* @expectedException \invalid_parameter_exception
* @expectedExceptionMessage Email address already exists: username@test.com
*/
public function test_create_refuse_duplicate_email() {
set_config('createuser', true, 'auth_userkey');
@ -380,7 +387,6 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
$duplicateuser = clone($originaluser);
$duplicateuser->username = 'duplicateuser';
$this->setExpectedException('invalid_parameter_exception', 'Email address already exists: username@test.com');
$this->auth->get_login_url($duplicateuser);
}
@ -428,6 +434,9 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
/**
* Test that when we attempt to update a user duplicate emails are caught.
*
* @expectedException \invalid_parameter_exception
* @expectedExceptionMessage Email address already exists: trytoduplicate@test.com
*/
public function test_update_refuse_duplicate_email() {
set_config('updateuser', true, 'auth_userkey');
@ -448,12 +457,14 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
$originaluser->city = 'brighton';
$originaluser->ip = '192.168.1.1';
$this->setExpectedException('invalid_parameter_exception', 'Email address already exists: trytoduplicate@test.com');
$this->auth->get_login_url($originaluser);
}
/**
* Test that when we attempt to update a user duplicate usernames are caught.
*
* @expectedException \invalid_parameter_exception
* @expectedExceptionMessage Username already exists: trytoduplicate
*/
public function test_update_refuse_duplicate_username() {
set_config('updateuser', true, 'auth_userkey');
@ -473,7 +484,6 @@ class auth_plugin_userkey_testcase extends advanced_testcase {
$originaluser->city = 'brighton';
$originaluser->ip = '192.168.1.1';
$this->setExpectedException('invalid_parameter_exception', 'Username already exists: trytoduplicate');
$this->auth->get_login_url($originaluser);
}

View file

@ -169,6 +169,25 @@ class core_userkey_manager_testcase extends advanced_testcase {
$manager->validate_key($value);
}
/**
* Test that IP address mismatch exception gets thrown if incorrect IP and outside whitelist.
*/
public function test_create_correct_key_if_ip_correct_not_whitelisted_and_whitelist_set() {
global $DB;
$this->config->iprestriction = true;
$this->config->ipwhitelist = '10.0.0.0/8;172.16.0.0/12;192.168.0.0/16';
$manager = new core_userkey_manager($this->config);
$value = $manager->create_key($this->user->id, '193.168.1.1');
$_SERVER['HTTP_CLIENT_IP'] = '193.168.1.1';
$key = $manager->validate_key($value);
$this->assertEquals($this->user->id, $key->userid);
}
/**
* Test that key is accepted if incorrect IP and within whitelist.
*/