Incorrect redirection when you are banned

Actually, deleting the line solves the problem altogether. It is redundant here.

So, should we redirect a banned user? we can prepare a better landing place when the module takes an action fixing it, or do nothing and remove the line.. Suggestions?

It's a little bit more complicated than just removing the line. This code refers to "soft blocking", and means, user is not blocked, ip address is not blocked, but ip address will not be able to submit the login form anymore.

Removing the line will not prevent the softblocking, because the drupal forms flow continues. We need to interrupt the form submission at this point, and a redirection was the first choice. We can alter any other field just to avoid the login operation being completed, but it can't be removed deleting the line, or soft-blocking will stop working.

any tip is welcome here..

Just to keep track of this..

There is code that should be executed in the login_security_validate, so we can't just drupal_goto out of the function if the host is banned. If we just jump (// Check for host login attempts: Soft), the $_SESSION variable has not been cleared, and then the core message for invalid username or password will not disappear.

// this loop is instead of doing t() because t() can only translate static strings, not variables.
foreach ($variables as $key => $value) {
$variables[$key] = theme('placeholder', $value);
}
form_set_error('submit', strtr(variable_get('login_security_host_soft_banned', LOGIN_SECURITY_HOST_SOFT_BANNED), $variables));
drupal_goto(substr(drupal_get_destination(), 12));

And later in the code

if (variable_get('login_security_disable_core_login_error', LOGIN_SECURITY_DISABLE_CORE_LOGIN_ERROR)) {
// resets the form error status so no form fields are highlighted in red
form_set_error(NULL, '', TRUE);
// removes "Sorry, unrecognized username or password. Have you forgotten your password?"
unset($_SESSION['messages']['error']);
}

Also other notices (number of attempts, for example) will not be shown.

Fix:

Currently, this is the order of "validators" for the login form:

[0] => login_security_set_login_timestamp
[1] => user_login_name_validate
[2] => user_login_authenticate_validate
[3] => user_login_final_validate
[4] => login_security_validate

If the user enters a right user/password pair, the account will be logged in at [2], and we are checking at [4].. So the code currently doesn't work.

should we include a new validator before [2]? we can do some tricks in the hook_form_alter we already have... is there any other idea? Remember that soft-blocking punishes the person who's trying to login too much times by just rendering login forms useless.

After a quick review, I guess a good solution could be:

- in the login_security_validate show the message "This host is not allowed to log in to %site. Please contact your site administrator." if the user tries to submit the form again, and remove the drupal_goto statement.

- in login_security_form_alter: check if %ip_current_count is > %soft_block_attempts and slightly modify the validation part so user_login_authenticate_validate nevers get executed. We could also remove the submit button (this could break themes) or disable some fields (but not sure if other form_alter from other modules in the login form will break).

Deekayen, need your expertise here! could this approach be very evil?

Ok, I ran my own on this. This is the patch for the 5.x-1.x-dev version fixing this. Basically, the submit button is removed from the form once the soft-blocking protection is activated. The operation is done in the login form alter hook.

The "This host is not allowed to login" now appears everytime a login form is displayed (the login_block_form is displayed in every page).

I'm moving this to the D6 branch using the same approach as for D5. Including the tests for this protection.

Commited to: 5.x-1.x-dev, 6.x-1.x-dev and HEAD.

I'm not entirely satisfied with this solution. Still feels like something should happen at validation time rather than just removing the submit button. Someone who's going to be brute forcing an account isn't going to be likely using a submit button anyway, rather looping through posts. I'm going to try moving the validation ahead of the core validation like I did with setting the login timestamp.

Deekayen, I also considered doing something like that, but fapi protects the form submission as the operation "log in" button is not in the form, therefore it's not being submitted never. What I think could be nasty is about themes and templates hoping to find a submit button, where now there's nothing.

In your validation function we can remove "user_login_authenticate_validate" from the validators array instead of the button.. or just change the 'pass' value of the submission..

Diff against 6--1.

• Moves the soft blocking to validation. It should validation soft-block before core's validation through the ordering of the array_merge() in the form_alter().
• Moves assertResponse:200 to drupalLoginLite()
• Adds another validation attempt to test with and without soft block error message.
• No longer removes the submit button for soft blocked IPs.
• Does not attempt to do what should have been failing variable replacement in assertText() tests for soft block error messages.
• Remove duplicate field check for user_login form.

Patched and tested. I also added a line in the last test of the softblocking case to restore the valid password before the last drupalLoginLite call, to be sure the form doesn't get submitted correctly (this was happening previous to this issue). Just wanted to be sure the authentication_validate doesn't logs the user in.

$normal_user->pass_raw = $good_pass;

Submitting a valid login after the blocking didn't work so I guess all cases were tested.

Committed #12 to 6--1 and HEAD with an additional test to make sure a valid login is still soft blocked at the end.