[D7] Password reset form has no flood protection

This looks like a good start. Couple of todo's though:

1) Should we clear the flood event if the password reset's successful in the same way that user_login_final_validate() does? Looks like that might need to happen in user_pass_reset()?

2) Tests :)

It doesn't look like the D8 patch over in #1681832: Password reset form has no flood protection clears the flood event when a reset is successful either.

It looks like it could be tricky to do so as we need to know the $identifier that was used to register the event.

In the case of the login form, that's carried along by the form API, e.g.:

  elseif (isset($form_state['flood_control_user_identifier'])) {
    // Clear past failures for this user so as not to block a user who might
    // log in and out more than once in an hour.
    flood_clear_event('failed_login_attempt_user', $form_state['flood_control_user_identifier']);
  }

IIUC password resets rely entirely on the URL to carry state, so I don't see that the identifier used in user_pass_validate() is available in user_pass_reset() (unless we added it to the URL, or stored in the db).

The IP may well be different, so I don't think deriving it from the ip_address() of the current request would work.

So perhaps not very straightforwards.

Unless perhaps we say that the user-based flood event should only use the uid as the identifier (i.e. not have user_pass_reset_identifier_uid_only as an option, but instead make that mandatory), then we would know the identifier on the successful password reset.

Doesn't look like the IP-only identifier has flood events cleared on login either, so perhaps just using the uid would be one way to go here.

Yup, it's probably an edge case but I suppose someone might do multiple password resets if e.g. they're struggling with their password manager or something similar.

I don't think it's essential that the flood events are cleared.

If we were going to add clearing, we'd need to do so in the D8 patch first.

Added some basic tests for password reset flood control, similar to those in the D8 parent issue.

This revealed that flood control was being triggered before the limit was exceeded e.g. if user_pass_reset_ip_limit is set to 2, the second password reset request is blocked by flood control. So an off-by-one error pretty much.

This was because we were registering the flood events before calling flood_is_allowed(), so this patch changes that order around (see the interdiff).

This wasn't obvious because in user.module user_login_authenticate_validate() calls flood_is_allowed() before user_login_final_validate() calls flood_register_event() whereas we're doing both from the same function here.

I think this works as expected now that the check and the registration of the event have been swapped around.

Oh, just noticed the comments need a tweak as they still mention "to catch attempts from one IP to log in" etc.. which should be changed to refer to password resets.

I'll let the tests run first.

...and here's the tweak to the comment.

The parent issue was fixed / committed to D8.

We did implement clearing per-user flood events when a password's reset successfully, and that meant using only the uid as an identifier.

So here's an update to the D7 patch which does the same.

The test-only patch doesn't clear the flood events, so it should fail.

The D8 version of this was committed; I'd like to get this Security Improvement into the next D7 release if possible.

I think this one is ready (and per @drumm in #14 has been in place on d.o for over a year now).

RTBM, +1 from me for merge

Running tests on re-rolled patch.

(no interdiff as it was more confusing than helpful)

Thank you!