UMN Usability: split access rules into an optional module

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.

+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?

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?

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

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 - 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.

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

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.

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.

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.

This is a great idea. +1 from me.

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?

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.

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".

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.

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.

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

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.

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.

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...

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 ;)

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.

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.

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?

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

@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.

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: 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.

* 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. ;-)

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.

$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.

  $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.

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).

@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?

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.)

- 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.

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

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:<ul><li>'. implode('</li><li>', $ips_not_blocked) .'</li></ul>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.

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.

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 ;)

Tested successfully both clean install and upgrade path.

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

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

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.

Committed to CVS HEAD.

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

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.

Cleaning up the issue queue.

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

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

Docs are not critical in this case.

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.

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