Use textareas and allow "one IP per line"

Sorry about the delay, this patch will remove the maxlength for that field so it's set to the browser default (5000 ish). This should probably be converted to a text area though and use line breaks instead of semicolons, but I don't wanna hold you up right now.

Here's an updated patch to remove the maxlength on all IP fields (global login, user login, roles). I went ahead and committed this, but I do want to change to one IP per line at some point.

Hi All,

I'm attaching a patch for 7.x-3 which changes the IP whitelist fields to textareas and also allows to separate the values on semi-colons and new lines.

Regards,
Jeff

I'm confused on this one. The title of the issue doesn't seem to match the issue summary and the first patch is for changing sizes. ?

Ah... buried in #2 is "but I do want to change to one IP per line at some point." It would be good to update the issue summary as this is confusing.

I think all of these were in the original code but some nitpicks:

1. +++ b/restrict_by_ip.module
   @@ -215,10 +215,10 @@ function restrict_by_ip_login_settings() {
   +    '#description' => t('To restrict login for ALL users, enter global IP Address Ranges in CIDR Notation seperated with semi-colons or new lines. E.G. 10.20.30.0/24;192.168.199.1/32;1.0.0.0/8<br />For more information on CIDR notation click <a href="http://www.brassy.net/2007/mar/cidr_basic_subnetting">here</a>.<br />Leave field blank to disable IP restrictions for user login.'),
   

   "seperated" => "separated"

2. +++ b/restrict_by_ip.module
   @@ -257,9 +257,9 @@ function restrict_by_ip_login_add_edit_user($form, &$form_state, $account = NULL
   +    '#description' => t('Enter IP Address Ranges in CIDR Notation seperated with semi-colons or new lines. E.G. 10.20.30.0/24;192.168.199.1/32;1.0.0.0/8<br />For more information on CIDR notation click <a href="http://www.brassy.net/2007/mar/cidr_basic_subnetting">here</a>.'),
   
   @@ -336,10 +336,10 @@ function restrict_by_ip_role_settings() {
   +      '#description' => t('Enter IP Address Ranges in CIDR Notation seperated with semi-colons or new lines. E.G. 10.20.30.0/24;192.168.199.1/32;1.0.0.0/8<br />For more information on CIDR notation click <a href="http://www.brassy.net/2007/mar/cidr_basic_subnetting">here</a>.<br />Leave field blank to disable IP restrictions for ' . $name),
   

   "seperated" => "separated"

3. +++ b/restrict_by_ip.module
   @@ -594,6 +594,30 @@ function _restrict_by_ip_validate_ip($ip_address) {
   +  $ipaddresses_semi_colons_processed = explode(";", $ip_whitelist);
   

   Use single quotes?

The only other thing is there is an inconsistency between using $ipaddresses and $ip_address in different places but that is in the original code so I wouldn't worry about trying to make this consistent.

Sorry for the confusion. I actually want to convert from semi-colons to line breaks, not both. So only one IP per line, which is more consistent with Drupal core. I've updated the issue summary.

In addition, variable_get() and variable_set() will automatically serialize arrays, so I'd like to see something like this in the form submit variable_set('restrict_by_ip_login_range', explode("/n", $form_values[])). Then everywhere you need the data you only need to do variable_get('restrict_by_ip_login_range', array()) and then iterate over the array. This requires more work than what you provided already, but I think it's cleaner.

An update function will also be needed to convert semi-colons to line breaks.

Thanks for the patch and reviews! There are coding standards problems with the original code, so I don't expect you to fix/catch them all in this patch.

Regarding #9

Perhaps cleaning up comments could be done in another issue since there are quite a few places in the code where cleanup would be nice.

Regarding #11:

Converting semi-colons to line breaks in an update hook seems like it would be difficult. I think that it would be difficult to run the update reliably without using drush because the user might not have access to the front end. Any suggestions about this would be appreciated... it's getting late today and I'm running out of time to make further updates.

Using arrays in variables is a good idea too but perhaps that could be a separate issue?

Regards,
Jeff

I don't think an update function would be difficult, but it'd have to be tested for sure.

I guess we can avoid it by not changing how the data is stored at all. Instead, let's just change the admin forms so that it converts ; to \n on load, and vice-versa on save. That way the UI is improved, but the storage and rest of the code doesn't have to change.

I've been using all my spare time to complete the D8 port, so I'm afraid I won't get to this yet.

Although it's documented in the UI, the fact that the configuration cannot have a trailing semi-colon makes it especially easy for admins to shoot themselves in foot. The configuration could be provided thru other means like config files, environment variables, drush, etc. where there is no documentation...

A trailing semi-colon seems to be interpreted as matching any IP address, i.e. disables the IP address restriction :/