This patch adds validation to prevent users blocking their own IP addresses from admin/settings/ip-blocking (which'd result in immediate locking out from the site via drupal_is_denied()).

To get into good habits I've added a test for this and related stuff in system.module that wasn't explicitly covered by the integration tests that just got committed to statistics.module.

Ironically the only test that fails is the one for the new validation against ip_address(). I've tested it manually, and it works on my machine - so assuming it's something to do with the internal browser and $_SERVER['REMOTE_ADDR']. Not sure what to do in this case, so uploading for review anyway.

CommentFileSizeAuthor
#2 ip.patch5.31 KBcatch
#1 ip.patch5.27 KBcatch
ip.patch4.75 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new ip.patch5.27 KB

Was cut and pasting IP addresses around and it's confusing if you get a validation error with trailing whitespace, so added a trim().

catch’s picture

catch
he/him
Primary language English
commented
Assigned: Unassigned » catch
StatusFileSize
new ip.patch5.31 KB

That patch had a bad string in the test. I've just tried this on a different machine and got 36 passes, 0 fails. Wondering if it might be a curl version issue or something? Debian Etch apparently runs libcurl3 (>= 7.15.5-1) (fails) Ubuntu gutsy runs libcurl3 (>= 7.16.2-1)

dries’s picture

dries commented
Status: Needs review » Needs work

I reviewed the patch and it looks good.

I tested the patch and that one test is failing as well. I'm running MAMP on OSX.

Nevertheless, I committed this patch. More tests are better, and the fact that a test fails will remind us to fix it. Thanks for the tests catch.

dries’s picture

dries commented

This is exactly why testing is good. It helps us to make progress faster while maintaining quality.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Fixed

I've split the ip_address() inconsistency over to http://drupal.org/node/256886 - since there's other places that'll need to be tested eventually. Going to mark this one to fixed, thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.