ip_match should be a multiple value field, possible changes to schema for better performance

Fixed in DEV by http://drupalcode.org/project/ip_login.git/commit/7a3e639 ...

Understood, thanks... Going past 255 will need a change to the database schema, install file and module ip_login_user code. Probably should become a text area and allow newlines too...

Perhaps you can define the field as a multi-value field. If you're out of space, just create a new line.

Going past 255 will also stop proper indexing on versions of mysql less than what Drupal 7 expects (i.e. on D6), can't speak for sqlite or postregsql.

John's idea of making the field multi-value is a good one. This way each IP address/range could be entered in its own field, and the DB search could be made quicker by looking only at the start of the table (like '[1st_quad].%', rather than '%[1st_quad].%'). Indeed, the database could add a new field for the first_quad. which is byte so the table index would only need to be on the first byte, rather than doing string matches on a text index which is bigger and slower. The rest of the IP range would go into the existing ip addresz field... Hmm.

This needs changes to the admin UI, the validation and save code to handle this, but has performance and usability benefits.

Changes to the schema seems to be problematic. I'm not sure if it concerns changing the schema and creating a new field, or conserving old data. See the following issues:
#937442: Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed)
[1153104#comment-4573350]

But we're not using the field system, just the database API and standard install process...

It's certainly possible, via hook_update, to add a new, byte or int field to the table, plus non-unique index, for matching the first quad. That would be pretty easy, in fact that's what I did when I wrote the code to update from 6.x-1.x to 2.x... Same deal in D7 since we're not going near the field API (and rightly so to avoid having to make the table's data private again).

Further to my last, an int field would probably be more useful than a byte in the future to allow parts of IPv6 to be stored. That means leaving the varchar(255) is ok too since IPv6 fields are much longer than IPv4, plus we've got space in there to serialise data to add more features if needed. Not that I foresee any, but it pays to plan ahead!

We have a need for something bigger than 255 in the D7 version. I suggest the following table change.

CREATE TABLE IF NOT EXISTS `ip_login_user` (
  `ip_id` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'Allow multiple entries',
  `uid` int(10) unsigned NOT NULL DEFAULT '0' COMMENT 'ID of user for IP Login',
  `ip_match` varchar(255) NOT NULL DEFAULT '' COMMENT 'IP ranges and addresses',
  PRIMARY KEY (`ip_id`,`uid`)
) COMMENT='Stores the IP Login address and range matches for users';

I tried adding the auto increment after uid and MySQL/InnoDB complained.

The individual ip_match entries could initially remain the same as the current system. The retrieval code could merge the strings when showing them on the form then split them when saving an update. You minimise the code changes and it should be D6 compatible. At a later date you can enhance with a split to a separate line for every range and the SQL changes mentioned earlier in this issue.

These changes work for me when I edit a user. I have not tried a login to a multiple address user. The select SQL looks like it will work unchanged.

function _ip_login_get_user_range($uid) {
  if (isset($uid) && is_numeric($uid)) {
    $ip_matches = array();
    $result = db_query('SELECT * FROM {ip_login_user} WHERE uid = :uid', array(':uid' => $uid));
    foreach ($result as $record) {
      $ip_match = trim($record->ip_match);
      if ($ip_match != '') {
        $ip_matches[] = $ip_match;
      }
    }
    if (count($ip_matches) > 0) {
      return implode(",\n", $ip_matches);
    }
  }
  return NULL;
}

function _ip_login_set_user_range($uid, $ip_range = '') {
  if (isset($uid) && is_numeric($uid)) {
    $ip_range = str_replace(array("\n", "\r"), ',', $ip_range);
    $ip_ranges = explode(',', $ip_range);
    foreach($ip_ranges as $key => $ip_range) {
      $ip_range = trim($ip_range);
      if($ip_range == '') {
        unset($ip_ranges[$key]);
      } else {
        $ip_ranges[$key] = $ip_range;
      }
    }
    db_delete('ip_login_user')
      ->condition('uid', $uid)
      ->execute();
    foreach($ip_ranges as $ip_range) {
      db_insert('ip_login_user')
        ->fields(array(
        'uid' => $uid, 
        'ip_match' => $ip_range, 
      ))
      ->execute();
    }
  }
  return FALSE;
}

Processing speed needs to be in the lookup, not the updating of ip_match. You could handle the following Apache format case with three fields, the original, a from address and a to address.
188.56.65.24/31

ip_match = 188.56.65.24/31
ip_from = 188.56.65.24
ip_to = 188.56.65.25

You generate the from and to during the ip_match update. During login, you look up an address as >= ip_from and <= ip_to. ip_from and ip_to would both be indexed. ip_from/ip_to combinations could be generated from all the formats used in IP Login and in Apache. Some combinations would generate a lot of entires but that slows down only the ip_range update, not the lookup.

Hi Peterx, thanks.

Yes, this is exactly what I was thinking - it's obviously the lookups that need to be fast.

I wonder if this, #1271976: Subnet matching and the growing need for IPv6 support can be dealt with at the same time... In other words, as per mtcs' suggestion, we convert IP range strings to longs, with a lower and upper bound stored in the database for quicker matching as you suggest. If this could work with part of an IPv6 address too, we're really onto something.

Does anyone know much about IPv6? I know it's a huge address space, but matching it can't be much harder than IPv4 - just longer...

The longest IPv6 address is 39 characters so make the from and to address 39 characters. The separator is a colon. Almost everything else is the same.
1234:1234:1234:1234:1234:1234:1234:1234

1234:0:0:0:0:0:0:76 can be abbreviated to 1234::76. To make the database from/to work, you would store the addresses expanded and expand incoming addresses for comparison.

The parts of the address, 1234, are hex, 4f5c, and can be any mix of case. Store all of them as lower case and change incoming addresses to lower before comparison.

ip_match could use an index.

function ip_login_schema() {
  $schema['ip_login_user'] = array(
    'description' => t('Stores the IP Login address and range matches for users'),
    'fields' => array(
      'ip_id' => array(
        'description' => t('ID of IP Login entry'),
        'type' => 'serial',
        'unsigned' => TRUE,
        'not null' => TRUE,
      ),
      'uid' => array(
        'description' => t('ID of user for IP Login'),
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
      ),
      'ip_match' => array(
        'description' => t('IP ranges and addresses'),
        'type' => 'varchar',
        'length' => 255,
        'not null' => TRUE,
        'default' => ''
      ),
    ),
    'primary key' => array('ip_id'),
  );
  return $schema;
}

Messy quick test expand of IPv6 address. Add a strtolower() and compare to the addresses in the database.

Test address: 
$address = '123::45';
print $address;

New address: 
$new = array();
if(strpos($address, '::') !== false) {
  $parts  = explode(':', $address);
  if(count($parts) < 8) {
    foreach($parts as $part) {
      if($part == '') {
        $missing = 8 - count($parts) +1;
        $new[] = substr('0:0:0:0:0:0:0:0', 0, ($missing+$missing-1));
      } else {
        $new[] = $part;
      }
    }
  }
}
print implode(':', $new);

Thanks Peterx... I'm flat out on other things but will be back on this soon.

In the mean time, please check out #1288728: Restrict by IP-address and add your thoughts if you have any

Thanks again...

Postponing major changes like this to the forthcoming 2.1 branch, want to get 2.0 out the way and bug-free first.

Using D7.9 and about to go to D7.12. I deleted all my mods to IP Login, went back to vanilla code with just one change, ip_match length in install changed from 255 to 2000. Our longest string is 304 characters.

+1, I need this functionnality too. Could we upgrade Priority to normal instead of minor? I'm using 6.24.

Interesting reason to use a multiple value field:
I found a very long IP range string and split it up by , then sorted into order. Lots of duplicates and overlapping entries. If the entries were in a multiple value field with the values displayed in sequence, there is a higher change of someone spotting duplicates and overlapping entries.

The overlapping entries include cases where many addresses are specified as unique entries then a range is introduced to cover them but the originals are not deleted.

+1 - it's really not unusual to need several IP ranges.

OK, bumping this up... There are several tasks here, but I think the best approach is to either allow:

1. multiple IP Login text entry fields, one for each IP range.
2. a single IP Login textarea field, with multiple IP ranges separated by newlines or commas.

Option 2 is probably quicker and easier and has a nicer/simpler UI, but option 1 may be more flexible as it potentially allows ordering via weight to prioritise matches... Though for #1271714: User ordering when multiple matches exists. we need weights between user/IP ranges, not within a user's IP ranges so ordering on ranges on a user's edit page is a bit pointless...

So I lean towards option 2. Your thoughts?

I'd be very happy to see patches for D7 or D6 DEV that does option 2 above, and ensures the current lookup/matching and display code works properly by saving each range to its own row in the ip_login_user table.

Remember that we cannot make the DB field longer as this would make it unindexable, slow, or unsupported (by older MySQL variants) per comment #5. Also want to ensure as little difference between D6 and D7 branches as possible.

I reckon it's mainly some user form API tweaks and some string splitting/joining (same process used all over Drupal), plus minor changes to saving and loading. Reckon the matching code will just work as before... Anyone fancy a go?

I reckon Peterx's comment in #10 is nearly there, just need it in patch form and with the form changes.

And one this done, we can look at speeding up lookups with a numeric ip_from and ip_to fields on each row...

Since davidwhthomas has started a 7.x-3.x branch that handles this and much more, this is now a won't fix for the D6/7 2.x branch.

See: #1587134: Views & IP Address Field module integration for IP Login