Access rules: admin can be blocked

I think this is a bug and a critical one at that, since once a rule is assigned perfectly legitimate users are blocked. The problems I can find are:

1. Usernames that are already registered are blocked by new access rules if they match the pattern.
2. The ability to check rules in admin/access/rules/check is not working as far as I can tell.
3. Attempts to create new user accounts that match the patterns are prevented, but users are redirected to /node immediately and no message is displayed as to what just happened, until after a page refresh.

What I saw after a page refresh of /node was:

Your password and further instructions have been sent to your e-mail address.
The name code0range is a reserved username.

What I see when trying to login as code0range (my admin account) is this:

The name code0range is a reserved username.

And the only way around is to delete the rule from the access table.

I tried to break this down, combining the comments of jo1ene and grohk.

* The ability to check rules in admin/access/rules/check is not working as far as I can tell.
* Check rules didn't work. I don't get any output whatsoever

True. The reason was that the #tree attribute wasn't set in the form definitions. The attached patch fixes this. I also took the opportunity to convert these forms to the submit model.

* I was able to create a user admin123 (i.e. the rules don't work)
* Attempts to create new user accounts that match the patterns are prevented (i.e. the rules work), but users are redirected to /node immediately and no message is displayed as to what just happened, until after a page refresh.

I can't reproduce this. On my install (latest HEAD), the rules do work (I can't create protected usernames, not as an admin and not as anonymous). Furthermore, I'm not redirected, and the error message is displayed correctly. (which makes sense, because the redirect is done in user_register_submit, which isn't executed when there are errors in the form, in this case a protected username) Can you try again with latest HEAD and confirm?

* Usernames that are already registered are blocked by new access rules if they match the pattern.
* I would assume that a check would only be performed at registration, not on login.

This is true, but I'm not sure whether this is a bug. According to the current help text, this is a bug: "Set up username and e-mail address access rules for new accounts." Because it doesn't only apply for new accounts, but also for logins, it is now a bug. But you can also see it as a feature (think about it). Here are the options that I see:

1. Leave everything as it is
2. Follow the current help text, and remove drupal_is_denied from user_login_validate. However, this will pose a problem for external authentication, as bad_username[at]drupalsite.com will be able to register and will be inserted in the db. We would have to deal with that.
3. Don't touch user_login_validate. Associate a timestamp with the access rule. When the rule is created, provide a checkbox "apply rule recursively in time". This would require a db change. This will allow to define how the rule works (will usernames et al from before the creation of the rule still be able to login or not?)
4. Don't touch user_login_validate. When creating the rule, provide another set of radios: (a) apply rule upon registration, (b) apply rule upon login, (c) both. Also requires a db change.

If we reach a consensus on this, I'm willing to provide patches. For now, I would vote for option 3.

It always worked like this so solution #1 is fine. Just update the help text / documentation to reflect the correct behavior. Solutions #2, #3 and #4 are feature requests and will have to wait for Drupal 4.8.

Argh, serves me right for not properly checking existing issues! I noticed the same issue and prepared the attached patch. It's virtually identical to DriesK's patch except that it doesn't add the tree attribute [which IMO is unnecessary for such a simple form, or if it is, you can do away with the "type" variable], and adds an additional validate function. Overall, probably slightly more efficient..

@Dries: If you missed DriesK's patch among all the other points being mentioned, it is necessary as the check rules page is currently b0rk3d :P

Thanks
-K

Using 4.7b2: the access rules was not working for me. Check Rules was also not working at all.

Using CVS-HEAD as of this comment, access rules seems to be working properly (but anonymous user registration was not working so I couldn't test that - I will look to see if that bug exists and if it persists or if it was a personal problem).

I applied this patch and re-tested and it seems to continue working and now the Check Rules also works.

One note is that I created a user "four", logged in as that user, created an access rule to block this user name (deny %four%) and the user was stil able to stay logged in - so people should just be aware that new rules only impact new users/new logins to the system - any current sessions stay live.

I agree with a modified method #1 - fixing the help text to say "...access rules to control new and existing accounts (existing accounts currently logged in will not be affected)."

Hi Jolene,

6. As others have mentioned previously, this is more a feature issue that can be fixed in the documentation. The access rules affect *both* registration forms *and* login forms. Technically, currently logged in users who match any deny rules should have their sessions removed from the sessions table.. but this is not the case at present..

8. This appears to work fine with HEAD.

Thanks,
-K

Same here. #8 works fine in HEAD, #6 is a feature issue.

Also, I agree with Zen concerning the use of the #tree attribute. Actually, I thought of this myself when hitting the submit button when posting my patch, but I didn't bother to change it. Setting this to 'code needs work' though, as a modified help text should be part of the patch (otherwise, we'll forget about it). I will supply a new one later today, based on Zen's patch.

Here is the new patch, with 2 changes:

1. updated help text
2. although I agree that the rules should also apply to existing accounts (in general) during login, I think jo1ene made a specific point: anyone with the 'administer access control' permission can block user1 (in specific) by writing a deny rule. IMO, this should not be possible. I think user1 should always be able to login. Therefore, I added another line to this patch, which bypasses the drupal_is_denied check for user1. Does anyone disagree with that?

By the way, this has got me thinking further. Anyone with the 'administer users' permission can block user1 (I tried), or change user1's password. However, I think this should not be dealt with at login time, but during the user edit form validation. I'll file a separate issue for that.

fixed 2 typos (e-mail instead of email)

Good idea. The user=1 check is incomplete though, as I could just as easily block the "admin" account's host address. So, a similar thing will have to be done with the drupal_is_denied check in bootstrap.inc. Or in other words, this and the bootstrap.inc fix should probably be in a separate patch :)

Just my 0.02 :)
Thanks,
-K

That is not as straightforward as it sounds. There is no such thing as the user1 host address. The host address is the IP address from which the user is viewing the site. User1 can use different computers, ISP's almost always use dynamic IP's, ... In other words, the host address is dynamic (and not stored in the users table). Furthermore, the host check doesn't happen during login, but on each page view.

So, you (bad guy) can monitor the logs (if you have access), find the IP that user1 is currently using, and ban it. These are the possibilities:

1. user1 returns to the site, but he has another IP: nothing happens.
2. user1 returns to the site with the same IP, but he logged out (or his session expired): nothing we can do, as we can't know that he's user1 during bootstrap.
3. user1 is still logged in, and visits another page with the same IP: we can protect user1 in _drupal_bootstrap (not in drupal_is_denied).

If we want this protection, use/review rules_check_4.patch. If not, use/review rules_check_3.patch.

Let's go with #3.

Both access type and rule type should be required fields, IMO. Currently, only mask is required. Furthermore, when I go to ?q=admin/access/rules/add, there is no default rule type selected (no radio button checked). Plus, I can add a rule without selecting a rule type. This is a bug. While not strictly related to this patch, it might be easy to fix while you're at it?

Yes, I agree that this is not straightforward - hence the request that this be removed and moved to a separate patch :) I personally consider both these user1 issues to be more of a trust issue [The admin shouldn't give people access control permissions unless he trusts them], and don't really believe that they are really necessary. I would prefer to just have a variable in settings.php to override access rules in the case of a lockout. But my basic point is that:
a) This patch probably doesn't belong in this issue
b) Dries is not going to commit the original patch because of this [which is more of a feature enhancement].

Which is why I recommend that this be dealt with as a separate issue :)

------------------

But anyways, my other point is that the user1 patch in user.module is incomplete without the corresponding patch in bootstrap.inc. In fact, adding the bootstrap.inc patch will still be incomplete without the above mentioned settings.php variable override option. Here's why I think this is the case:

-IP addresses aren't as dynamic as you say. I personally have had a new IP exactly once in about 3 years which was when I changed ISPs, and not very often before then.
-I don't have to look at access logs to get your (admin) IP. I can get it off e-mails, IRC and what not..
-I don't necessarily have to block just one IP. I can block a range of them: For e.g. 192.168.0.%. So it doesn't matter even if you have a dynamic address..
-Once I block your IP address (range), as you have said - there is no way for the admin (if logged out) to get in besides deleting the rule from the {access} table.. and if logged in, the bootstrap.inc patch will be required..
-The "logged out" situation can only be solved with the variable override (implemented in drupal_id_denied())..

So all in all, there is no point in adding this one patch if the other situations are not covered as well. I hope that made sense :)

If you'd like to discuss this more, please feel free to e-mail me [via my profile] as this might be slightly off-topic for this issue. Or please let me know if you open a separate issue for this :)

Thanks
-K

@Zen: thanks a lot for your input. It sure does make sense :-) And I agree we should file a separate issue for the bootstrap fix. However, I think that we _should_ include the drupal_is_denied('user', bypass for user1 in this patch, as this concerns input validation during login, and because it is more critical. I consider the boostrap fix less critical, as simply using another computer will allow user1 to access the site again, whereas in the other case, db access is required. So I'll follow Dries's request to further use patch #3.

@Dries: ok, I will make a new patch addressing your issues.

@Driesk: OK, sounds good. If you're tied up with other things, let me know and I'll roll this one up - I've nothing pressing to do this evening :)

-K

Thanks for reminding me... I already had the patch ready, just forgot to submit it. Apparently, Dries already committed part of the previous patch, so here is the rest.

Literally setting 'Access Type' and 'Rule type' to #required = TRUE wasn't necessary, simply setting a valid default value was sufficient (as you can't unselect a radio). It was just a case of incorrect forms API conversion.

Committed to HEAD.
I left out the first bulk because I'm going to wrap my head around that some more.

Two years later, the "admin can be blocked" part of this issue (see Dries' comment on the first bulk in #19) is still not committed! And it's still critical (see for example this forum post).

See http://drupal.org/project/userprotect for a current solution available in contrib.

I can think of plenty of valid use cases where you might want to use access rules to block existing user accounts. Mass spam registration, all kinds of things. Especially with the contrib solution, this isn't critical, and may be by design.

catch: I'm not talking about blocking existing user accounts.
I'm talking about blocking the admin account. And that everyone with the 'administer access control' permission can block user 1.
C'mon, that's not by design! And this is nothing to be fixed in contrib. It is to be fixed here.

I withstand the temptation to promote this to critical again, but it would be great if Dries could comment on this. Finally it was him who left out the first bulk to think about it again (#19).

Added is a patch containing the missing part from #18. I changed the implementation though, as it now only queries uid=1 if drupal_is_denied, which means it has performancewise no impact on regular logins. Also I added a warning message if an access rule hits user 1. Please test.

OK now I see :)

I tested the patch - works as advertised. Agree with checking for uid = 1 only after drupal_is_denied

'An access rule tried to block %name.'

Should access rule link to admin/user/rules ? If I got that message unexpectedly I'd want to go straight there to find out what was going on. Otherwise the rest looks decent to me.

afaik, uid 1 can be blocked by any user with administer users, or have their password changed, so this doesn't make it impossible to do so, but it seems a small enough change I'd support it for D6.

Should access rule link to admin/user/rules ?

Yes, surely it should. Can we further improve the message text?

afaik, uid 1 can be blocked by any user with administer users, or have their password changed

That's another story, that we might want to fix as well.

Another issue is that admin ip addresses and the rest can be blocked with access rules - and there's no way to prevent that I can think of (except maybe form validation to stop people blocking the ip they're submitting the form from, but that'd be a stupid thing to do anyway).

Quick run at help text:

%name is currently blocked by an access rule which matches your username, %username. As the site administrator, your account cannot be blocked using this method, however you may want to confirm that your <a href="@access">access rules</a> settings are correct if this is the first time you have seen this message.

"site administrator" isn't right, but "user 1" we don't use anywhere right? Also it's a bit long. Leaving at review since this text is minor and the implementation still needs eyes on it.

I reformulated the warning message, so it should be both short and informative:

An access rule tried to deny access to %name.
As this is the site administrator\'s user account, this rule has been overridden.
However you might want to check the <a href="@link">Access rules configuration</a>.

Also I improved the error messages and added watchdog entries.

To quote Gábor from http://lists.drupal.org/pipermail/development/2007-December/027977.html: "RC1 is string freeze, so whatever needs modifications in the strings is generally postponed to Drupal 7."

This is crazy! A string freeze when there are still so many issues active... another beta would absolutely be appropriate.
I change this back cause we don't have to change strings - introducing a new one should IMO be no problem. It won't be the only one anyway.

So this is a stripped down version of #28 without string changes.

You should avoid escaping like \' and use double quotes instead. Also it's common to split multiple lines into multiple t()s rather than concatenate them.

There are always strings active.

Thanks. Corrected code style.
Didn't get what you're meaning with "There are always strings active." though... :)

OK the escaping is sorted out, should be paragraphs rather than break tags though.

With "always strings active" - I just meant that whenever there's a string freeze there'll always be one active issue that changes some strings - I don't know how hard it is in relation to bug reports though.

Here is the same patch without break tags. Still no strings are changed nor removed, there are only additions. Hope we get it in though.

@pancho - missing patch.

Oops. Thanks for informing me!

Notice: This issue is complementary to and therefore independent from #46149 and #75289.

No longer in core, and re-subscribing so we can fix this in contrib if it's still an issue. http://drupal.org/node/228594