One thing that came up frequently during usability testing was how many new users go to "access rules" looking for permissions. At a minimum it should be renamed.

However, drupal_is_denied() is also the slowest query on Drupal.org and runs on every single page.

So it might be a good candidate for breaking out into either 1. an optional core module 2. a contributed module.

This would remove both the confusion and the performance issue in one ;)

So a few questions. Does anyone actually use this? How much? Is there a contrib alternative already?

If we have to keep it in core for whatever reason, what would be a less confusing name for the menu item?

Comments

Susurrus’s picture

Even as a relatively experienced Drupal user, I find that I still sometimes go to user access when I mean to go to the permissions page. I think this is definitely a good idea. I think too many people would want this option to move it out into a contrib module, but I think an optional core module sounds reasonable, especially when the performance of the site is severely affected by this feature, even if it isn't used.

Big +1.

ximo’s picture

+1, "Access rules" has fooled me a few times when I was looking for "Permissions". It should definitely be made optional, if not turned into contrib.

Thinking out loud.. it's a filter towards the username and e-mail address of existing and new users. User filter? Access filter?

BioALIEN’s picture

Title: UMN Usabiility: Access rules? » UMN Usability: Access rules?
mfer’s picture

I have only used this on one site, ever. I can see how some people might want to use it. What about it being a separate module in core?

catch’s picture

A separate module in core (ideally with a more accurate name like "allow/deny rules") sounds good to me.

Wim Leers’s picture

Hardly anybody uses this, so I agree that it should be an optional core module, especially if it's adversely affecting performance.

ximo’s picture

Also, with Dries mentioning he would like to remove some minor modules from core, this would be a perfect pick. Very few use it, it confuses people. Away it goes.

catch’s picture

OK so the next deal would be to isolate all related code into a separate module, then we can decide if it's core-worthy or not.

Looks like everything related to it is in user module + drupal_is_denied() in bootstrap.inc - it runs pretty early so would we have to load more code at that stage if it's done via hook_boot or hook_init?

moshe weitzman’s picture

catch - you are right on track here.

i think a separate contrib module is whats needed here. the only trick is the bootstrap as you mentioned. hook_boot is too late for this code. so, what we need to do is simply remove drupal_is_denied() from core and move it to the README for the new module. Ask its users to the drupal_is_denied() into the site's settings.php. In order to prevent an error, you will want to add an if (function_exists('drupal_is_denied')) before we call that function in bootstrap.inc.

mfer’s picture

So, this would be like custom_url_rewrite_inbound and custom_url_rewrite_outbound in usage?

catch’s picture

moshe: Thanks for confirming hook_boot is too late, I had a feeling it would be but hadn't thought of any clean alternatives. I don't know if I'll have time to work on the actual patch for this, but leaving the (wrapped) function call to drupal_is_denied() in bootstrap.inc, moving the function itself to settings.php (whether commented out in core or in a contrib README.txt) and everything else in a module seems sane enough.

catch’s picture

Title: UMN Usability: split access rules into an optional module » UMN Usability: Access rules?
FileSize
25 KB
22.09 KB

Here's what I've done so far:

Moved drupal_is_denied() to a commented section of settings.php (per Moshe)
Wrapped the call to drupal_is_denied() from bootstrap.inc in if function_exists()

All access rule stuff is out of user module.

All that code moved into a new access.module (also attached as a "png" - rename it to .tar and it should work) - access.module has everything that was taken out of user.module but is currently missing the 10 or so lines of code to actually make it work - i.e. to add the validation against drupal_is_denied back into user registration etc. I briefly tested adding and checking rules and that still works.

Fixed the .install files but not touched that table in an update since it'll have to be left there for a release.

A few things need to be decided:

1. Is this to become an optional core module or a contrib module? My vote is for contrib due to the apparent lack of use. (and potential remaining confusion for new users as to what it does).
2. is "access.module / access rules" the best name we can come up with? How about allow/deny or something?
3. If this is going into contrib, drupal_is_denied() goes into an INSTALL.txt/README.txt or stays in settings.php for an easier upgrade path?
4. If we move this to contrib, it needs a maintainer. Any volunteers.
5. Is there already a module which does this stuff in contrib that we could suggest instead?

TODO: there's now a few if ifs in user.module which could probably be replaced with && for cleanup. I didn't bother with stuff like this for this first run although I did try to fix code comments.
TODO: make access.module actually work.
TODO: CHANGELOG.txt entry - depending on whether this is an optional module or a contrib module.

I wasted an hour trying to work out what I'd broken in all this because user creation from admin/user/user/create is broken in HEAD. Should probably fix that before going much further.

This is at needs review because what's affected in core is pretty much there IMO. If anyone fancies adopting this as a module for D7 then 95% of the work is done, but that needs some discussion.

keith.smith’s picture

Status: Active » Needs review

If this is to be a core module, I guess we need some help text at admin/help/access, which I can help write. It will probably be late today before I have a chance to do that, though.

catch’s picture

Title: UMN Usability: Access rules? » UMN Usability: split access rules into an optional module

Retitling for clarity.

How's this for admin/help/access?

The Access Rules(?) module allows you to create allow and deny rules for visitors to your site. You can exclude ip addresses from your entire site, and also set rules for e-mail addresses and user names which apply to both new and existing user accounts.

For example you could restrict access site to only a few ip addresses, ban poorly behaving bots, or stop users registering at your site with usernames like 'Admin' or e-mail addresses from certain domains.

When you set an access rule for a user name or e-mail address, it will immediately log out any current users who match the rule you've set up, in addition to stopping new registrations. So this feature should be used with care.

When a visitor attempts to view your site from an ip address with a deny rule, they will see only a message informing them that their domain has been banned, this occurs before any other Drupal functionality (modules, themes) are loaded.

I think I've only ever used this once, so I'm a bit fuzzy on the details of what exactly does what.

RobLoach’s picture

Title: UMN Usability: Access rules? » UMN Usability: split access rules into an optional module

It seems like this functionality could be something part of contrib. The contrib module could go even further too, providing more advanced blocking and administration tools.

keith.smith’s picture

FileSize
30 KB

This is a new version of catch's tar-ed module folder for "access". Remove the .txt extension and extract it for the contents. This makes a few very minor code style changes and incorporates catch's proposed help text with my own changes. The "norules.patch" in #12 is still required along with the new module files.

I'm not 100% certain that the new help text really reflects this module's functionality (I haven't tested it), but need to put this up for a bit to work on something else. For instance, I haven't confirmed that:
- you can allow or deny based on matching hostnames in addition to ip addresses (or, ip addresses in addition to hostnames). I'm not sure that both work but wrote the help text as if they did.

catch’s picture

I spoke to Rob Loach about this a bit on irc, and we decided to volunteer to co-maintain this in contrib if that's an option.

At a minimum we'd keep it maintained for bug fixes/security/upgrades during the 7/8 release cycle and also have some ideas for feature additions (which also wouldn't make much sense in core) for a 7.x-2.x branch. Neither of us actually uses access rules at the moment, although I personally need some extra user admin tools which would fold into this quite nicely.

merlinofchaos’s picture

This is a great idea. +1 from me.

Senpai’s picture

I'd like to propose that we use the word 'prohibit' somewhere in the title, rather than 'access'. How about prohibit_users, prohibited_users, prohibit_access, or prohibit_user_access?

keith.smith’s picture

hmmm. That will sound rough in the help text.

- "The prohibit module..."
- "The prohibit users module..."
- "The prohibited users module..."
- "The prohibit access module..."
- "The prohibit user access module..."

Actually, the last two aren't bad at all. I guess we could also call it something like "access ban" or "user ban" module.

catch’s picture

Prohibit is a good idea. I'd also thought about allow/deny - since that seems to be the original intention of this functionality - to mimic what you can do in apache. Should definitely use either prohibit or deny somewhere to give a more accurate picture of what it does.

floretan’s picture

I would choose "deny" over "prohibit", so that technical people find the same language used in other places, but also because it sounds less daunting for new users.

One of the problems I see with these two words though is that none of them have straightforward translations to other languages (tried with a few European ones). The word "restriction" translates much more easily, so I suggest something like "access restriction module".

Dries’s picture

Status: Needs review » Needs work

I don't like the fact that we have a commented function in settings.php and that there is a function_exists(). Sorry, but we'll have to find a better solution for that. Removing this functionality is fine, but only if it can be removed in an elegant fashion. The proposed solution is far from elegant.

moshe weitzman’s picture

Core expects other functions to be implemented in settings.php. Namely for i18n and for custom_url_rewrite_inbound and outbound. Thats how we implement functionality that has to happen before the bootstrap since non required modules are simply not loaded yet.

I guess this could wait until some Drupal pipes patch lands but thats very far from reality still. So this is now looking like a won't fix to me, despite recommendations of the usability team.

moshe weitzman’s picture

Wait a second. The intent was never to put all that code into the default settings.php. That hunk of code should live in the README for the access module which lives in Contrib. Users will be instructed to paste it into their own settings.php

catch’s picture

Yeah the settings.php hunk was a conservative fallback to allow this to live in core as an optional module if there was resistance to putting it in contrib.Since there doesn't seem to be much resistance, and we now have volunteers to maintain this in contrib (me and Rob Loach), I can do a re-roll without it :) and like Moshe says - stick it in INSTALL.txt in the new contrib module.

This leaves us with function_exists()

DRUPAL_BOOTSTRAP_EARLY_PAGE_CACHE uses a variable check - but that's no good for us here IMO.

cwgordon07 suggests the following via irc:

what about if, in settings.php, we specified $deny_callback = 'my_function';
then if (!empty($deny_callback)) { $deny_callback(); }
and $deny_callback /can/ be an array, with keys 'path' and 'function', 'path' being a file to include before anything else
or maybe two separate vars
$deny_callback and $deny_file
maybe even $deny_arguments
All commented out by default

Except along with the function this would go in INSTALL.txt along with drupal_is_denied().

Does that sound any better?

David_Rothstein’s picture

I have a couple (possibly naive) comments:

First, while I totally agree that access rules for usernames/email addresses should be a contrib module, I think that access rules on hostnames should remain in core. The very fact that it happens so early in the page loading process suggests that it "belongs" in core. The use case is simply to imagine a non-tech-savvy site admin who needs to block an IP address because their site is being attacked. It's very simple for them to look at their Drupal logs, find the offending IP address, and block it within Drupal. Instead we would be asking them to (a) find and download a contrib module that they might not even know exists, and (b) copy and paste code from that into their settings.php. The site admins who would have trouble doing this (especially in what could very well be an "emergency" situation where speed is of the essence) are the very same people who would most need this feature -- i.e., the kind of site admin who doesn't know how or doesn't have access to block IP addresses through Apache.

Second, if hostname access rules are the only thing that stays in core, then the menu item for this could arguably be moved from "User Management" to "Site Configuration"... thereby further eliminating confusion with permissions.

Third, doing this might allow the drupal_is_denied() query to be optimized better; if that function isn't being asked to do so many things, it might be possible to rewrite the query to make it faster.

catch’s picture

Sounds pretty sane to me. We could leave drupal_denied where it is but remove the e-mail/username checking - which means no more LOWER().

Then core gets a lightweight interface just for hostnames (which the new contrib module and others could populate from other places - watchdog, comment moderation etc. - there's already an action iirc).

I'd wanted to split the two (three) things up in contrib eventually, but it makes sense to do it this way 'round.

Site configuration sounds like a reasonable place for the ip blocking to live as well.

Anyone spot potential problems with this approach? It'll probably be a week before I get to work on it in any depth, but it ought to be relatively clean. I'd still want to take the ip banning out of user.module - could we move it to system maybe?

David_Rothstein’s picture

Hm, I was about to say you still need LOWER() in order to check hostnames ... but then I took a look at the code and it seems like Drupal never checks the hostname, only the IP address!!! That seems like a bug (to say the least), since the admin interface very much makes it sound like you can block hostnames in addition to IP addresses... something to keep in mind during the rewrite.

But ... if the goal is to optimize the query absolutely as much as possible, then maybe the bare minimum thing to keep in core is just the ability to block individual IP addresses? It seems to me like that is the only "crucial" functionality. Then everything else (complex allow/deny rules, IP address masks, and something that actually allows you to block by hostname) could move to contrib too? You could then have something like:

function drupal_is_denied() {
  // Allow contrib modules to override this function.
  if (function_exists('drupal_is_denied_advanced')) {
    return drupal_is_denied_advanced();
  }
  // Here we just do a simple check to see if the IP address is banned.
  return ip_is_denied();
}

function ip_is_denied() {
  return db_result(db_query_range("SELECT 1 FROM {access} WHERE type = 'host' AND mask = '%s' AND status = 0", ip_address(), 0, 1));
}

Or maybe use the idea from #26 instead of function_exists(), but the basic idea is that drupal_is_denied_advanced() is a function that an "Advanced Hostname Access Rules" contrib module could define via settings.php. This function could (or, rather, should) call ip_is_denied() but also do whatever extra processing it needed to.

David_Rothstein’s picture

The other thing to think about is whether you want all access modules to use the same old {access} table for whatever they're doing, or whether instead Drupal core should have its own very lightweight {banned_addresses} table just for banning IP addresses? In that case you could get rid of the unneeded columns and just do something like:

function ip_is_denied() {
  return db_result(db_query_range("SELECT 1 FROM {banned_addresses} WHERE address = '%s'", ip_address(), 0, 1));
}

But that would make the upgrade from Drupal 6 more complicated, and it also might make it more complicated for different contrib modules to work together...

boydjd’s picture

Status: Needs work » Needs review
FileSize
7.33 KB

Patch removes drupal_is_denied from core.

Access rules should NOT be at the application level, but should atleast at the server level, if not the firewall level. Additionally, IPs can be added into .htaccess or the hosts file to deny for shared hosting. On dedicated hosting, this should be done at the ipfw level.

boydjd’s picture

Whoops, forgot to remove the admin menu stuff. Done.

boydjd’s picture

Remove DRUPAL_ACCESS_BOOTSTRAP constant and some other cleanup.

catch’s picture

OK so following a long discussion in IRC we came up with this:

Overall, the consensus was that ip blocking shouldn't be done at the application level. Which is sensible.

chx said a contrib module could have the function (with some probable cleanup by separating ips from names/e-mail) in INSTALL.txt to be copied into settings.php - this means we no longer need a function_exists() and can cleanly remove everything associated with access rules/drupal_is_denied() but still provide an upgrade path. That means our contrib module isn't so elegant, but core is. Which is great, IMO.

Also, people on shared hosting will almost always have access to .htaccess. Those on IIS are (I assume) likely to have access to similar tools on that platform. Either way, we can recommend using .htaccess or lower level tools, but still have the same functionality in contrib if someone really needs it (and again, myself and Rob Loach are happy to maintain this). I think these are strong enough arguments to remove it entirely from core as long as the upgrade path is well documented.

In terms of the contrib module, the above tar still needs some tidying up to replicate the exact functionality and these changes. We could do some tidy up/ui improvements/features in a 7.x-2.x version. Also boydjd's patch found some stuff I missed in the original one, but didn't catch every reference.

In short, I'll roll a new patch to combine #33 and #12 so we can get some reviews on the removal. If there's agreement then it should be short work to get a module ready.

David_Rothstein’s picture

I don't understand what it means to say that IP address blocking "shouldn't" be done at the application level.

I think it really depends on why you're doing the blocking. Certainly if you're trying to protect yourself from a "nefarious evil hacker hellbent on destroying you", then I agree it needs to be done at the firewall or server level. But that is not the only reason to block an IP address. Sometimes it's just a matter of convenience, if someone is annoying your site or attacking it in a mild way. For example, as mentioned above, Drupal 6 has a "ban IP address of current user" action. It's a very nice feature to be able to have someone blocked immediately after they do something you don't want... even if you (the site admin) is asleep at the time. It isn't a foolproof security measure, but it doesn't have to be.

The way I look at it (especially since this is a "usability" thread!) is that 99% of the world's population doesn't like copying and pasting computer code between files. They're scared to death of it. Instead, they really like clicking on buttons ;) I suspect this is the reason that the access rules were added to Drupal core in the first place...(?) So I think it would be a step backwards to completely remove them. Simplify them down to their bare essentials, absolutely. Warn people not to use them as a serious security measure?... definitely a good idea. But (for whatever it's worth) my vote would definitely be against removing them entirely ;)

Jaza’s picture

I have a fresh thought on how we can proceed with this patch. Why not adopt the approach that we took with the watchdog API in D6? That is, leave drupal_is_denied() where it is now, but move its functionality out to a separate module. Since the main function of it right now is to ban specific IP addresses, I suggest that the new module be called 'ipban.module'.

Make this an API, so that ipban is just one example of how site-wide 'access denying' can be accomplished - contrib modules could be developed to deny access based on anything, from browser type (or user agent) to referring host. I vote to leave ipban.module in core, for example purposes if for nothing else.

Leave ipban off by default, and... voila, drupal_is_denied() suddenly has zero performance impact for 99% of sites. Plus, it's not displayed in the admin interface and doesn't confuse users.

moshe weitzman’s picture

Fresh thinking is great, but in this case I don't think we need all that flexibility. Despite David's passionate argument, this is just the sort of functionality that Contrib provides. I think Catch is on a good path here.

catch’s picture

FileSize
22.67 KB

Jaza, not all that fresh, that was my first attempt and Dries didn't like it ;)

@David: I think if you know enough to block an ip address, you can probably manage copying a hunk of text from one file to another (or for that matter putting some rules in .htaccess which is a much better solution anyway). Not to mention ip blocking is often the wrong tool - you can easily block a public library, or yourself (and argument against leaving ip blocking in since it's much harder to guard against than usernames), and your attacker can get a new ip very easily.

Here's a new patch combining #33 and #12. This removes everything I could find plus a couple more mentions in code comments etc., and should be ready for some more reviews.

edit: and the patch!

catch’s picture

FileSize
23.33 KB

Added a CHANGELOG.txt entry.

Dries’s picture

I think David's comment in #27 is right on -- especially combined with #30. Remove e-mail and username blocking, an go with a simplified interface for IP blocking (not hostname blocking).

I think there is room for blocking IPs at the application level. On drupal.org we have a long list of IP addresses that we block, and it just wouldn't be practical or scalable to do this at the OS level. Too many people would need (root?) access to the servers -- and we'd need to put these IP bans in place on all web nodes?

catch’s picture

Dries, thanks for the feedback!

Going from David's comment in #30, we should rename and simplify the function, then provide an update which moves the ip addresses only into a new core table ({banned_ips}?). Leaving {access} for contrib to deal with later. Since it looks like this will still be running on d.o we may as well address the performance issue as part of the cleanup.

chx said in irc that we'd ideally want to avoid this query altogether when it's not being used, after the new database layer gets in - since it'll be the only one necessary for cached page views in some configurations, but we agreed that's for a followup patch since it's going to be non-trivial.

He also said we should absolutely remove the LIKE because MySQL is unable to index it. That means no ip range banning (although that's probably a good thing usability wise since banning ip ranges is very much brute force, and we can make the interface leaner). If we don't ban ip ranges, then we also don't support allow rules - because they only make sense as exceptions to a range ban.

so drupal_is_denied() looks more like this:

function ip_is_denied() {
  return db_result(db_query_range("SELECT 1 FROM {banned_ips} WHERE ip_address = '%s'", ip_address(), 1));
}

So we'd have a new "IP address banning" menu item from site configuration, a page to add and delete denied ip addresses, and the action. All of which moves from user.module to system.module

Marking as needs work since there's no patch which does this yet.

chx’s picture

Status: Needs review » Needs work

Yes, if we must have a query in the critical path then make that at least indexable and nice.

boydjd’s picture

@Dries what's wrong with moving this into contrib and having the contrib module modify .htaccess to include a list of banned IPs, and have the blocking done via htaccess?

floretan’s picture

@boydjd - This won't be possible because .htaccess files are not (and should not) be writable by php. Generally, letting php modify executable or configuration files is a serious security risk.

boydjd’s picture

@flobruit Sure it's possible, have .htaccess include a separate file of banned IPs.

Dries’s picture

catch: the suggested approach of simplifying the query looks good to me. (Your example query has a typo though.)

boydjd: I'm not sure how that would work but I'm interested to learn more -- be it as part of this patch or as a follow-up patch. Plus, would it work on IE and Lighttpd? To date, we don't ship with Apache-only features. It might take some additional research?

boydjd’s picture

@Dries I'm advocating having a separate contribution for IP banning, and then have plugins in that contribution for IIS, Apache, Lighttpd, whatever it is that people need. All of them support denying IPs in one way or another.

Dries’s picture

boydjd: that makes it a lot harder to discover and figure out for end users. I'd go with a simple in-core solution for now.

boydjd’s picture

@Dries Okay, what about this ... what if we keep the same functionality, but split it into a core module that can be enabled/disabled and not required? Shouldn't be too difficult of a hack.

catch’s picture

FileSize
25.04 KB

Still work to do, but I'm posting what I've got. I did some light testing of this and what's there should be working.

What this patch does:

*Takes out all the existing access rules stuff as before.
*Creates a new table "blocked_ips" with just a single column for ip addresses.

*Modifies drupal_is_denied() in the following ways:
It now accepts an array from $conf() (commented by default). This is checked first, then it matches ip_address() to the array, ignoring the database table.

If the array isn't set, it checks the blocked_ips database table with a much simplified query - this is the default behaviour.

This has a couple of benefits:

1. If you use the admin interface to block ip addresses, the query is much quicker, indexable, cacheable (i.e. no LOWER() or LIKE())
2. If you're running memcache or something, you can serve pages to anonymous users without accessing the database at all and with no need to hack core (or one less query anyway).

TODO:

Add back a simple interface just for adding and deleting ip addresses from the blocked_ips table. I think this should live in either system.module or an optional core module - but not sure which. I'm also not sure what to call the menu item apart from "Blocked IP addresses" or "IP address blocking". Any ideas for this would be great.

drupal_is_denied() could probably be micro-optimised with an additional if (!empty) check before the in_array(). I left it as short as possible for now.

There's an update function to create the table but this doesn't yet move data from {access}.

boydjd’s picture

Just read through the code, haven't applied it. Looks good catch. +1 once we get the UI back in, and moving IP addresses from the access table into the blocked_ips table.

Dries’s picture

* I would put the UI in system.module (or system.admin.inc).
* I would use 'IP address blocking" or "IP blocking". We can rename this later, if we find a better name.
* It would be interesting to see a quick performance comparison of the SQL query before and after the change. This should be trivial -- and is optional. ;-)

David_Rothstein’s picture

Some quick comments (I haven't studied the code in too much depth though):

1. Overall this looks great. And +1 on "IP address blocking" for the menu item.

2. Was there a final decision on how a contributed module could hook into this to do more advanced IP blocking? Is the contrib module supposed to (a) set $conf['blocked_ips'], (b) insert entries in the {blocked_ips} table, or (c) is the idea that contributed modules should take @boydjd's approach of writing files to be used directly by the web server? If (a) or (b), then I'm not sure I understand how something like masking or allow/deny rules could be implemented in contrib...

3. Is the "Ban IP address of current user" action being removed or staying core? It looks like you left the action defined, but took the functionality out. (This could very well be something you were planning to get to but didn't yet.)

4. Why is $blocked_ips an input parameter to drupal_is_denied()? What about checking $conf['blocked_ips'] directly inside the function? That way drupal_is_denied($ip) would be safe to use elsewhere, any time you want to check if a specific IP address is blocked.

drumm’s picture

$conf['blocked_ips'] should be accessed via variable_get('blocked_ips', array()). I would just do that part in drupal_is_denied() instead of passing it as an argument.

The convention for 3rd-party IP blocking would follow caching, put the default in access.inc and override it with $conf['access_inc'] = 'some/other/path'. However, fancy access blocking would usually want to be at the web server or firewall level, so I am not sure that is really needed in Drupal.

catch’s picture

Status: Needs work » Needs review
FileSize
33.32 KB

This is pretty close now so marking back to CNR.

This adds the action back in to system.module, a new permission, interface, uses variable_get() directly in drupal_is_denied() etc. I think I've incorporated all the feedback so far.

Personally I don't think we need pluggable IP address blocking - if someone's advanced enough to need that, they should be doing it at the system or webserver level anyway. I'm still not 100% convinced that Drupal should support it at all, although I'm fairly happy with how this is going.

Only thing I've not done yet is the additional system_update to migrate the old data, but that should be easy enough. Also I changed the schema of blocked_ips to add an auto increment id, but the index on ip address might as well be unique. I've done some testing, but a bit more wouldn't hurt.

This is (hopefully) pretty minor stuff left, so have at it.

RobLoach’s picture

Status: Needs review » Needs work
  $blocked_ips = variable_get('blocked_ips', array());
  if (isset($blocked_ips)) { // <------------------ Always true
    return in_array($ip, $blocked_ips);
  }
  else {
    $sql = "SELECT 1 FROM {blocked_ips} WHERE ip = '%s'";
    return db_result(db_query($sql, $ip));
  }

If you don't create a $conf['blocked_ips'] in settings.php, $blocked_ips defaults to array(). A call to isset with array() always returns TRUE. This means that if you're not using $conf['blocked_ips'], then it will never check the {blocked_ips} table to see if the IP is blocked.

Therefore, it might make more sense to use:

  $blocked_ips = variable_get('blocked_ips', NULL);
  if (is_array($blocked_ips)) {
    return in_array($ip, $blocked_ips);
  }
  else {
    $sql = "SELECT 1 FROM {blocked_ips} WHERE ip = '%s'";
    return db_result(db_query($sql, $ip));
  }

That being said, is there anyway we could improve the performance even more here? It would be great if we could save the query to the database every time we want to check if an IP is blocked.

floretan’s picture

FileSize
33.29 KB

Fixed a few syntax errors in the admin pages and the upgrade function.

However, I tested blocking an IP on a local network, and the patch didn't have any effect (the computer at the blocked IP was able to access the site as an anonymous user, and log in with an existing account).

David_Rothstein’s picture

@flobruit, I think Rob Loach's comment in #56 is probably the reason it didn't work? (I don't have time right now, but I hope to get a chance to test this patch out over the weekend.)

As for fancy allow/deny rules on IP addresses, I guess it wouldn't be the end of the world if those weren't possible within Drupal (and they are at least indirectly possible if a contrib module follows @boydjd's approach discussed in #43 above). The only problem I can think of is that since they are supported now, and if there is no obvious contrib module that will be able to support them for Drupal 7, what should be done in the upgrade path to deal with this?

catch’s picture

FileSize
33.24 KB

doppelpost.

catch’s picture

Status: Needs review » Needs work

@Rob Loach: is_array doesn't work either, although !empty does.

However... I wanted to leave the option for a dummy array in settings.php. Leaving a.b.c.d there as a dummy value works exactly the same though, so maybe that's fine as it is. I was using !empty for ages but was short of time last night and wanted to post something, seems like I got caught in the middle of trying to work that last bit out. Here it is with !empty, not sure if we need to document the dummy value thing.

function drupal_is_denied($ip) {
  $blocked_ips = variable_get('blocked_ips', array());
  if (!empty($blocked_ips)) {
    return in_array($ip, $blocked_ips);
  }
  else {
    $sql = "SELECT 1 FROM {blocked_ips} WHERE ip = '%s'";
    return db_result(db_query($sql, $ip));
  }
}

So at the moment, if you have this in settings.php:

$conf['blocked_ips'] = array(
   'a.b.c.d',
 );

You skip the database check.

But if you have this:

$conf['blocked_ips'] = array(
 );

You don't. Unless we go back to checking $conf['blocked_ips'] directly I don't see a way around that apart from maybe array_key_exists(0, $blocked_ips) which'd be a bit slower than empty.

If we need to document this specific behaviour, suggestions welcome!

Also fixed a couple more typos so hook_help() actually shows up now and removed a bit of cruft.

@David: IMO, we should document the change in the D7 upgrade instructions, and have a handbook page with ways of doing this in .htaccess (and elsewhere).

@flobruit: It should work now, thanks for the re-roll :)

catch’s picture

Status: Needs work » Needs review
FileSize
33.4 KB

Found a stale bit in the last patch just after I uploaded, and it seems my duplicate message didn't get the attachment anyway. Fun.

I'll try again.

Note - still no upgrade path for existing IP addresses, but that's pretty much it now with a bit of luck.

David_Rothstein’s picture

Status: Needs work » Needs review

A quick thought (not tested). Would this work?

$blocked_ips = variable_get('blocked_ips', NULL);
if (isset($blocked_ips) && is_array($blocked_ips)) {
...

I think that should let people set $conf['blocked_ips'] = array(); in settings.php to indicate they do not want any IP address blocking.

As for documenting the change in the D7 upgrade instructions... that certainly sounds like a plan, but I think it would be wise to document this particular one in big blinking red letters ;) If people don't realize it, then after the upgrade, their previously private site could suddenly be available to the world. Maybe also putting a message on the screen during the upgrade process?? (It probably wouldn't be too hard to check if they used wildcard characters in any of their entries in the {access} table and only print the message in that case.)

catch’s picture

FileSize
33.44 KB
33.4 KB

Works :) New patch attached.

IP ranges:

chx mentioned this in IRC. And I'd also discussed it with Beeradb, Rob Loach and others in irc a week or so ago (although we'd ruled it out as impossible to do in a scalable way).

A contrib module (which we'll need for username and e-mail restrictions anyway), could hook_form_alter that page to allow for wildcards to be added.

So, you enter 192.168.1.* and it inserts 192.168.1.0 all the way to 192.168.1.255 in your db. With an extra column added via schema API to group these (so they can be deleted in one go as well). You'd have to not allow this for 192.168.* because that would be crazy. But it'd do for most sites I think.

With the upgrade, we're going to have to check that that ip addresses are valid ones (using long2ip(ip2long($access_rules->ip))) or similar) before migrating them into the ip address table. If we find masks that aren't valid ip addresses somewhere, then yeah a message would help. "This ip address range or hostname will no longer be subject to access restrictions, please see documentation in UPDATE.txt" - something like that?

catch’s picture

FileSize
34.15 KB

New patch.

Has a system_update() function for the data migration from {access} table, and proper PHP5 IP address validation courtesy of chx on irc.

IMO this is ready to go unless I've missed something, so please review!

catch’s picture

FileSize
34.17 KB

Couple of strings hadn't kept up to date with the rest of the patch.

Dries’s picture

Status: Needs review » Needs work

- In documentation we write 'IP' and not 'ip' (consistency).
- In documentation we write 'e-mail' and not 'email' (consistency).
- We don't capitalize words in a sentence: "Primary Key: Unique ID for IP addresses" should be "Primary key: unique ID for IP addresses".
- Typically we use iid instead of ipid.
- In this case it _might_ make sense to make the IP address the primary key.
- In the documentation, we should be a bit more verbose/explicit about the overwrite mechanism. We should give an example.

Otherwise looks great.

Dries’s picture

I just wanted to throw in my support for this patch. I'll review it once it is RTBC.

catch’s picture

Status: Needs work » Needs review
FileSize
34.64 KB

Thanks for taking a look Dries.

This patch should deal with all of the concerns in #66. The only thing I didn't change was the primary key - access rules currently doesn't have any protection from duplicate entries afaik, so I didn't want to risk duplicate inserts, or add complexity to the update function. Additionally we may have a use case for extending this table via schema api in contrib, so an arbitrary primary key seems alright for that.

The documentation in settings.php is quite a bit more verbose now. This could do with review and the patch still needs some testing.

David_Rothstein’s picture

Status: Needs review » Needs work

I just finished putting the above patch through some tests. (Unfortunately I used the patch from #65 since I downloaded it before #68 was posted, but it doesn't look like the differences are too big.)

Everything worked great, although I do have a few comments below. The tests I ran were as follows:

  • Added/deleted a couple IP addresses via the admin interface and made sure that access from those computers was blocked/allowed as appropriate.
  • Checked that invalid/duplicate addresses could not be added via the admin interface.
  • Set the variable in settings.php to either an empty array or an array of IP addresses and checked that it correctly used those and bypassed anything in the {blocked_ips} table.
  • Added some fancy access rules in Drupal 6, ran the update, and checked that they were correctly put into {blocked_ips} or printed to the screen as invalid.

All the tests went fine, no problems. Yay!

My comments:

1. The message in system_update_7003() should definitely be at the 'warning' level, and I think it might be useful to give a little more help to the user. Maybe something like this?

function system_update_7003() {
  $ret = array();
  $ips_not_blocked = array();
  $type = 'host';
  $result = db_query("SELECT mask FROM {access} WHERE status = %d AND TYPE = '%s'", 0, $type);
  while ($access = db_fetch_object($result)) {
    if (filter_var($access->mask, FILTER_VALIDATE_IP, FILTER_FLAG_NO_RES_RANGE) !== FALSE) {
      $ret[] = update_sql("INSERT INTO {blocked_ips} (ip) VALUES ('$access->mask')");
    }
    else {
      $ips_not_blocked[] = check_plain($access->mask);
    }
  }
  if (!empty($ips_not_blocked)) {
    drupal_set_message('The following hosts are no longer blocked from accessing your site, either because they are not valid IP addresses or because they contain wildcards:
  • '. implode('
  • ', $ips_not_blocked) .'
Wildcard IP address blocking is no longer supported in Drupal. If you take your site out of maintenance mode now, visitors whose IP addresses match the above patterns may be able to access your site\'s content. To continue blocking any of these visitors, you should ........ (put link to a handbook page here??????????)', 'warning'); $ret[] = array( 'success' => TRUE, 'query' => 'Wildcard IP address blocking is no longer supported.', ); } return $ret; }

2. I noticed that "allow" rules on IP addresses are ignored in the update function. I think this means that it's possible for addresses to be blocked after the update that should not be... it's a bit of a pathological edge case, but it should be easy to protect against by adding a little code near the end of the update function... something like the following?

// Make sure not to block any IP addresses that were specifically allowed.
$result = db_query("SELECT mask FROM {access} WHERE status = %d AND type = '%s'", 1, $type);
while ($allowed = db_fetch_object($result)) {
  db_query("DELETE FROM {blocked_ips} WHERE LOWER(ip) LIKE LOWER('%s')", $allowed->mask);
}

3. I think the last line of drupal_is_denied() should be changed to return (bool) db_result(db_query($sql, $ip));. Right now, it's not guaranteed to return a boolean, and for a function as important as this one it seems wise to make sure that the return value type is always consistent (in case someone ever checks it with === TRUE).

4. A few grammar/wording suggestions:

  • The title on the form field ("IP address.") probably shouldn't have a period at the end.
  • The error message This IP address is already blocked should have a period at the end.
  • The message %ip was deleted should probably become The IP address %ip was deleted.

5. I'm a little suspicious of having the IP address form underneath the list of already-blocked addresses... that could get annoying if the list is very long. (Not a big deal either way, though.)

6. The new documentation in settings.php looks very good to me.

catch’s picture

David, thanks for the detailed review!

1. Yes, that's a good plan, and the revised wording looks fine to me.
2. I'm pretty sure selecting from access where status = 0 means no allow rules get touched. Am I mis-informed?
3. Seems sane.
4. Yes, all reasonable.
5. Hmm, I tried to follow conventions elsewhere in core (say the roles admin page). Personally I like to see what's going on, then fill in the form. It'd be nice to explore alternatives, but I don't think that's for this patch (and I specifically avoided putting the add form on another page).
6. Great!

Revised patch today hopefully, or at the latest tomorrow.

catch’s picture

Status: Needs work » Needs review
FileSize
35.09 KB

Here we go, changes:

Grammar suggestions from #68 applied.

Adds a warning level to the drupal_set_message(). I used some of David Rothstein's suggested text, but kept the message up top generic and moved the specific ranges in the query output - since we could potentially end up with a long list up there, and X amount of IP addresses in a list will be a bit distracting from other stuff that's potentially as important. It also means admins who are interested get an overview of what's still blocked and what isn't.

I had a look through the handbook, and this seems to be the most appropriate section: http://drupal.org/node/24302 within the troubleshooting guide. It doesn't deal with IP blocking specifically, but I'm prepared to reorganise that page so it's current content is a child page and it becomes a more generalised reference for blocking IPs, hosts and referrers. For now I just changed the title of the page, but we have somewhere to link to at least. I'll admit to knowing only the very basics in this area, so if anyone fancies refreshing the page themselves, that'd be lovely.

I also tested the upgrade path and 'allowed' IP addresses definitely don't get blocked, so no need for a change there.

drupal_is_denied() now returns boolean explicitly.

David_Rothstein’s picture

Status: Needs review » Needs work

Took a quick look -- seems good to me. Your idea of moving the list of unused IP address masks down to the query output definitely makes sense, although maybe it would then be good to add an extra sentence or parenthetical remark to the warning message telling people where on the page to find that list? (It's not particularly obvious that the list is there at all, and especially not obvious to look for it in the output from update #7003.) The list could be important for some people, and this page is basically the last chance they'll get to see it on the screen.

Other things about the new update function:

  • Given the new function structure, I think !empty needs to be changed to isset.
  • Does the line $ret[] = array('success' => TRUE, 'query' => ''); serve any purpose? (It seems like it doesn't, although I have to admit I don't fully understand all the details of how the return values of these update functions get used.)
  • Maybe "Handbook" should be "Handbook page"?

As for allow rules, in order to see the problem you need to have a specific deny rule which is overridden by an allow rule. If you're doing this on a local test installation, you can have some fun by using 127.0.0.1 and watch yourself get banned. In Drupal 6, first set an allow rule on 127.0.0.1 (or something more general like 127.0.0.%) and then set a deny rule on 127.0.0.1. Allow rules take precedence over deny rules, so you won't be banned. But the update function will happily put 127.0.0.1 in {blocked_ips} and ignore the allow rule, so after the update, you will be banned.

It's a pretty weird edge case and I can't think of too many ways it would come up in real life, but on the other hand, who knows... maybe an admin didn't know what they were doing and banned some wrong addresses which they later decided to fix with an allow rule? For a few lines of update code I think it's probably worth fixing.

catch’s picture

Status: Needs work » Needs review
FileSize
35.06 KB

Changed !empty to isset, good call. Even if some weird edge case meant we got a false positive, it hardly does any harm to show the message.

As to drawing more attention to IP addresses in the update messages, I think linking to a handbook page and having them in the update is good enough - the information is there if people need it. IP blocking is generally against bad bots, which is as much a performance issue as anything (and one which the new query ameliorates). Intranets or similar should not be using access rules. Like I said, this could be competing with more upgrade messages by the time we get to a release, and it's already quite in depth. afaik there's no id for system updates to link to, and a "look below to see IP addresses etc. etc." seems like overkill to me. Having said that, it's easy to add if people think it's absolutely necessary.

The success array against $ret[] gets rid of a warning: undefined index, so it's both necessary and redundant - cleaner approaches welcome!

Handbook -> Handbook page also done, thanks.

Denying and allowing the exact same IP address with conflicting rules. I think this gets into "Drupal should let me drown kittens" territory to be honest, I can't think of a valid use case for doing this at all.

David_Rothstein’s picture

OK, then this is basically RTBC! But I'll leave it as is for the moment, in case anyone else wants to weigh in on these last issues and the final patch...

Regarding the allow rules, the way I look at it is that the code's already written, so why not paste it in? -- it's update code that runs only once, so it's not like extra database calls are a big deal. I agree it's unlikely to matter in the vast majority of cases, but update functions that can mess up people's data are a really bad thing (in my opinion), so even if it protects 1 person out of 100,000, it's done its job. (Otherwise, for that person the appropriate analogy would actually be more like "I started to drown some kittens with Drupal 6 and then changed my mind, but Drupal 7 came along and drowned them for me"... or something like that ;)

catch’s picture

OK, well I can think of as few edge cases for allowing an IP address you wanted to deny by mistake, as blocking one you wanted to allow. I think what this really says is how confusing the UI is at the moment and why we need to fix it ;)

I'll probably roll a patch for that later today - or if you have time to re-roll with that bit added that'd be great too.

catch’s picture

FileSize
35.47 KB

This one has a slightly modified version of David's suggested sanity checking in #69. I've checked the install and upgrade path, and everything seems fine. Could do with another review. I think this covers everything.

floretan’s picture

Tested successfully both clean install and upgrade path.

Just fixing a warning from the coder module, otherwise it all looks good.

Dries’s picture

Status: Needs review » Fixed

I've reviewed and tested the code ... and committed it to CVS HEAD. Thanks for all the work, this is great.

catch’s picture

Priority: Critical » Normal

Excellent, thanks!

I'm going to mark this to CNW until we've got a project for the contrib module and some IP blocking documentation on that page.

catch’s picture

Status: Fixed » Needs work
catch’s picture

Priority: Normal » Minor
Status: Needs work » Needs review
FileSize
1.79 KB

Minor follow patch. Renames a comment from "Admin access pages." to "Access permissions pages." since we've no longer got access control or access rules now. Also cleaned up a couple of if statements while I was in there.

For a module name, we're probably going to go with "User restrictions" unless there's any objections.

floretan’s picture

The patch actually renames "Admin access pages." to "Admin permissions pages."

The word "admin" is generally used in core to mean "administrator" or "administration" (nouns), but in this case it's being used as "administer" (verb).

Here's the patch from #81, with the comment renamed to "Permission administration pages.". "User administration pages." is also replaced with "Admin user pages" for consistency.

catch’s picture

#82 looks much better, thanks!

Additionally, I managed to break search module :( separate issue opened here: http://drupal.org/node/248436

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD.

catch’s picture

Priority: Minor » Normal
Status: Fixed » Needs work

Thanks!

Back to needs work until the contrib module is active + docs.

dbeall’s picture

Please don't call me a duufus, but i do have a question about all this.. Which way is actually better? Blocking ip and or http with htaccess or using the function from within drupal? I don't know what the visible difference is to the blocked ip either. advice?
thanks..dave

Dave Reid’s picture

Priority: Normal » Critical

Using .htaccess will always be better because Drupal won't need to be loaded. But in the case that you don't have access to edit .htaccess (or using non-Apache webserver), the Drupal-provided function works great.

Marking as critical since the contrib module certainly needs to exist and be ready before we can release D7.

catch’s picture

There's a project here at http://drupal.org/project/user_restrictions - but at the moment it's just the code that was removed with function names find/replaced so definitely needs fixing up.

Wim Leers’s picture

Status: Needs work » Closed (fixed)

Cleaning up the issue queue.

David_Rothstein’s picture

Are the comments in #87 and #88 still accurate? If so, it sounds like this issue should remain open.

David_Rothstein’s picture

Status: Closed (fixed) » Needs work

I think this is still "needs work" based on the comments above.

catch’s picture

Yes user_restrictions doesn't have a release yet, or work at all, 'cos I get distracted by core, um, every few hours. If anyone wants to co-maintain / take it over please contact me.

sun.core’s picture

Priority: Critical » Normal

Docs are not critical in this case.

mrfelton’s picture

Are we likely to see some kind of backport to Drupal 6? The slow query in drupal_is_denied accounts for 30% of slow queries on my site. I don't even use the access restrictions.

Anonymous’s picture

Status: Needs work » Fixed

Drupal 7 has been already released, and it contains only the code to block the access by IP; the code to block users by their usernames is part of a third-party module.

I guess this can be considered fixed.

Status: Fixed » Closed (fixed)

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

c960657’s picture

I just filed #1061348: Backport $conf['blocked_ips'] to D6 about backporting the $conf['blocked_ips'] part of this patch for performance reasons.