At present if an attempt to authenticate via the user login form is blocked by flood control, nothing useful is logged.
The form gets an error set on it, but the response is still a 200 which means that access log entries cannot be identified by anyone interested in whether a site is being brute-forced.
e.g. core/modules/user/src/Form/UserLoginForm.php
if (!$this->flood->isAllowed('user.failed_login_ip', $flood_config->get('ip_limit'), $flood_config->get('ip_window'))) {
$form_state->set('flood_control_triggered', 'ip');
return;
}
...and then:
if ($flood_control_triggered = $form_state->get('flood_control_triggered')) {
if ($flood_control_triggered == 'user') {
$form_state->setErrorByName('name', $this->formatPlural($flood_config->get('user_limit'), 'There has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', 'There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => $this->url('user.pass')]));
}
else {
// We did not find a uid, so the limit is IP-based.
$form_state->setErrorByName('name', $this->t('Too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => $this->url('user.pass')]));
}
}
Whereas in core/modules/user/src/Controller/UserAuthenticationController.php
if (!$this->flood->isAllowed('user.failed_login_ip', $flood_config->get('ip_limit'), $flood_config->get('ip_window'))) {
throw new AccessDeniedHttpException('Access is blocked because of IP based flood prevention.', NULL, Response::HTTP_TOO_MANY_REQUESTS);
}
...so that yields a 403 in access logs.
At present we see something like this when the login form is brute-forced:
==> syslog <==
Jul 3 15:57:53 xenial-php drupal: http://drupal8x.xp|1530633473|user|10.0.3.1|http://drupal8x.xp/user/login||0||Login attempt failed for foo.
==> access.log <==
drupal8x.xp 10.0.3.1 - - [03/Jul/2018:15:57:53 +0000] "POST /user/login HTTP/1.1" 200 11726
..repeated a few times, until flood control kicks in, at which point we just see:
==> access.log <==
drupal8x.xp 10.0.3.1 - - [03/Jul/2018:15:57:55 +0000] "POST /user/login HTTP/1.1" 200 11726
drupal8x.xp 10.0.3.1 - - [03/Jul/2018:15:57:57 +0000] "POST /user/login HTTP/1.1" 200 11798
drupal8x.xp 10.0.3.1 - - [03/Jul/2018:15:57:58 +0000] "POST /user/login HTTP/1.1" 200 11798
Anyone wanting to gather threat intelligence from a Drupal site's logs would ideally want specific log entries to say "a login from this IP was blocked by flood control".
There may be performance reasons why we wouldn't want to make these log entries (e.g. it may represent a DoS risk especially on sites which are using the default database logging).
Filing this issue to discuss and work on potentially a couple of changes (follow up issues can be filed if appropriate):
* Respond with a 403 (consistently) when a login attempt has been blocked due to flood control limits.
* Log these blocked login attempts to logger (/ watchdog if we backport) - possibly as an opt-in change in order to avoid possible performance regressions.
* Discuss proposed changes.
* Implement them.
* Perhaps an option to turn flood control / brute force logging on and off?
* More / different log entries, if those count as UI.
None?
None?
| Comment | File | Size | Author |
|---|---|---|---|
| #113 | Screen Shot 2020-06-09 at 8.24.42 am.png | 12.49 KB | amjad1233 |
| #112 | 2983395-112.patch | 34.81 KB | mcdruid |
| #112 | interdiff-2983395-109-112.txt | 1.46 KB | mcdruid |
Comments
Comment #2
mcdruid commentedComment #3
mcdruid commentedComment #4
mcdruid commentedI'd be surprised if we ended up implementing the suggested changes exactly like this, but here's an initial patch to get the ball rolling.
This patch changes the behaviour in
\Drupal\user\Form\UserLoginForm::validateFinal- when flood control has been triggered - in the following ways:* Dispenses with setting the form errors (as the form will not be re-displayed).
* Throws an
AccessDeniedHttpExceptionso that we record a403in the logs.* Logs a message to record the IP which was blocked, and the user account the login attempt was for, if applicable.
* Uses
drupal_set_message()to actually display some details about why the request was blocked.I'm probably missing something, but the
$messagesent with theAccessDeniedHttpExceptiondoesn't seem to go anywhere useful which is why I've resorted to a dsm on top.So we end up with something like this in the logs:
This makes me think it may be better to implement a more specific
AccessDeniedHttpExceptionwhich might distil the two log entries into one (and could possibly even display the$messagewithout a dsm if that makes sense.)These changes will almost certainly cause test failures; I'll address that once we've seen exactly what's broken.
Comment #6
mcdruid commentedFixed some borked indentation and the failing tests, and changed the deprecated
drupal_set_message()calls to use the Messenger service instead.At present this removes the
formatPlural()in the messages shown when user-based flood control is triggered. It would be quite easy to add back in again, but I'm personally not sure it's necessary... Happy to hear other opinions.Comment #8
mcdruid commentedAdded settings checks around the logging in order to allow sites to opt out of it if - for example - it's having a negative performance impact. I think this is only really likely to be the case for database logging.
I've not added examples to (default.)settings.php but we could do.
Comment #9
anavarreTagging with scalability since it can impact DBlog.
I'd be ideal to test for the HTTP response code 403 and ensure the log message actually made it through.
Would it be possible you try and refactor this code such as you don't have to duplicate almost entirely those lines?
Comment #10
mcdruid commentedI don't disagree about the duplication, but so far this patch is pretty much making small tweaks to the logic that's already there.
For now, here's a new patch which adds the suggested checks to the tests:
* Check there's a 403 when flood control has blocked a login attempt.
* Check the logger/watchdog messages when flood control has blocked a login attempt.
For testing the logging, there are a couple of patterns in existing tests; both of which involve installing the dblog module during the test.
One approach is then setting up an admin user with appropriate permissions, logging that user in, and doing a GET request for 'admin/reports/dblog' to see if the log entries appear in the output (see, e.g.
\Drupal\Tests\contact\Functional\ContactSitewideTest::testAutoReply). The other option is to check directly in the database (see e.g.\Drupal\system\Tests\Form\StorageTest::testImmutableFormLegacyProtection). (I believe there's actually a bug in the query-based test there though, for which I'll file a follow-up).The db query seems quite a bit more lightweight, so that's what I've gone with here... but you could argue that as the tests here extend
BrowserTestBase, that doing the check with a GET request to 'admin/reports/dblog' would be more appropriate.Comment #11
mcdruid commentedI think this is a better way to test the logging; specifically checking what the last log message was from the user module, rather than just a boolean to indicate whether a particular message is there in the table at all.
Comment #12
mcdruid commentedoops, messed up the file extension on the last interdiff
Comment #14
mcdruid commentedHmm - tried to cancel the test for the messed up interdiff... back to NR for the actual patch in #11
Comment #15
mcdruid commentedSmall tweaks to reduce duplication.. along the lines of the suggestion from anavarre.
Comment #16
borisson_I think that getting it from the database is a good idea in the test, it's not because we are in a browsertest that we need to do everything in the browser, it's not the job of this test to test the dblog UI.
I would like to rtbc this, but I found some nits to pick.
The indentation of this looks off, it should be 2 spaces.
With this now being injected, the documentation of the
__constructmethod should be updated as well.{@inheritdoc}is sufficient.Comment #17
mcdruid commentedThanks for the review.
I've fixed the problems you pointed out in UserLoginForm.php
However, I've left the comment about modules, as this seems to be widespread in tests throughout core:
You could - of course - argue that doesn't mean it's right :) but I've left it for the sake of consistency.
Comment #18
borisson_As daniel always says: sanity > consistency. In this case it doesn't really matter though.
Comment #19
tatarbjHi @mcdruid,
I'm happy to see this issue in RTBC, but was just wondering would it be a nice approach to backport it to D7 in a separated issue? If so, i'm glad to help you on the way :)
Bests,
Balazs.
Comment #20
tatarbjComment #21
tatarbjI've created the d7 backport issue with a similar solution to propose - it needs some valid d7 related log entries and porting this solution that seems pretty easy as only watchdog should be called there. But more details under #2989985: User module's flood controls should do better logging [D7 backport]
Comment #22
mcdruid commentedThanks for the D7 backport follow-up Balazs; I was thinking that would be a good idea.
Comment #23
alexpottIs the granularity that useful here?
We also have authentication by other means too - should we make the logging consistent? I.e. login via REST and \Drupal\user\Controller\UserAuthenticationController::login() which calls \Drupal\user\Controller\UserAuthenticationController::floodControl()
In a joined up world there might be a user flood control service that both the form and UserAuthenticationController use that does the logging.
Comment #24
mcdruid commentedGood point; no it's not.
That sounds like a good idea, but I could do with a shove in the right direction in terms of where the class(es) would live and what they'd need to extend / implement.
I got as far as putting a
FloodControl.phpincore/modules/user/srcbut then realised I don't think I want to implementFloodInterfacethere. Do we want a new interface (UserFloodControlInterface?) which extendsFloodInterfaceand then a new class which implements that perhaps?Comment #25
anavarreDue to the impact this issue might have when you're being brute-forced, realistically this is major.
Comment #26
tatarbjComment #27
fgmRegarding the answer code, I feel like a 429 might be more appropriate than just a 403: this is about flood control, and 429 means "Too Many Requests", which seems a perfect fit.
RFC 6585 ( https://tools.ietf.org/html/rfc6585#section-4 ) describes it as "too many requests in a given amount of time"
The interesting bit is that it allows the response to contain an explanation, and defines the use of the Retry-After header to specify for how long the account is blocked.
Comment #28
mcdruid commented@fgm responding with 429 sounds like an interesting suggestion!
However, wouldn't that be better as a follow-up / separate issue? as this one's really about logging (although to be fair we seem to have strayed from that path quite a bit ;)
Comment #29
mcdruid commentedAdding notes from a discussion in #contribute (slack):
Comment #30
tatarbjComment #31
sunnygambino commentedHello Guys,
The HTTP 429 status code is used to be on the network layer. This is meaning that we should use it in the application layer , what is Drupal in this context. This status code is used by network services, servers, APIs etc... Logic should be implemented in the server application's config files. Don't do it in the application layer's logic.
I think it is not the best idea to use. The good old 403 is more than enough.
I think we should take a bit care about the following sources:
https://httpstatuses.com/429
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/429
RFC Standard: https://tools.ietf.org/html/rfc6585#section-4
I also attached an interdiff for patch #17. This is just simply changing the HTTP resonse global back to 403.
I hope I was able to help to move this issue forward
#ContributionWeekend2019
Comment #32
sunnygambino commentedComment #34
mcdruid commentedRerolled patch #17 for 8.8.x
I'll put this back to "Needs work" based on #23 / #29 but will leave in NR to see if tests pass.
Comment #36
mcdruid commentedAnother reroll, and back to NR for tests.
This will need to go back to "Needs work" based on #23 / #29 but trying to get tests to pass again first.
Comment #37
mcdruid commentedNoting also that the codesniffer patch wants to change
s/true/TRUEin two places in the patch e.g.:Comment #39
mcdruid commentedI think it's a few deprecations which were causing this to fail.
I've hopefully fixed those (and the codesniffer
s/true/TRUE).Should go back to "Needs work" based on #23 / #29 but trying to get tests to pass again first.
Comment #40
mcdruid commentedOk good.
What I really need now is one or more examples I could learn from in order to implement this suggestion from @alexpott:
I'm happy to do the leg work, but could do with a decent example if there's a service that does something similar. Any ideas?
Comment #41
yogeshmpawarComment #42
yogeshmpawarResolved the coding standard issues & added an interdiff.
Comment #43
mcdruid commentedI'd be surprised if this was "right" first time, but here's an attempt to implement the architecture outlined in #40 from @alexpott's comments.
This doesn't change UserAuthenticationController yet, but it hopefully shouldn't be too hard to adapt that to fit.
There are a couple of existing phpcs errors in the files this changes, but I've left those alone for now. Hopefully the patch doesn't introduce any new ones.
The format of what's being logged is not great initially; I'd aim to improve that, but wanted to get an idea of whether this is going in the right direction first.
Comment #44
mcdruid commentedSmall tweak to fix a variable name that I'd somehow clobbered.
Comment #45
mcdruid commentedOops got the variable name wrong again in one place.
Interdiff is against 43 as that shows what I'm trying to fix.
Comment #46
mcdruid commentedSmall tweak to the logging when flood control blocks login for a particular username / uid.
Comment #47
vijaycs85Overall it looks great. IMO, at minimum, we should have this patch to introducing&dispatching flood-related events, if not all. One major concern (split into multiple comments) below:
Probably a BC break?
same here.
If I get it right, we are adding this wrapper class *only* for this method. Why can't we do this in @flood? which would avoid the API changes.
Comment #48
vijaycs85Checked comments again especially #29 which summarizes the preferred solution. I have updated the patch to reflect those requirements:
1. The new service ''UserFloodControl" is a stand-alone service not implementing FloodInterface which is poorly named storage interface. I would love to deprecate @flood and use flood.storage.* something for better DX but probably as follow up, if @alexpott agrees :)
2. Since most of the methods in flood storage are storage's responsibility, removed them from service and added storage() method which can be used for storage-related operations and avoid duplicate methods. This also means, if a storage implementation has a new method, it can be used.
3. Though UserLogin is marked @internal, should we do anything to inform we are changing the args?
Created a CR draft as we are introducing a new service and events.
Comment #49
mcdruid commentedThanks @vijaycs85; I like passing $storage into the new service better.
As discussed, I think one thing we've missed so far is that the event can be dispatched without a proper $identifier set.
For example
\Drupal\Core\Flood\DatabaseBackend::isAllowed()does:As things stand, we need to add something similar to
\Drupal\user\UserFloodControl::isAllowed()otherwise we can end up logging flood control blocking a login by IP without actually including that IP in the event that's dispatched.Here's a new patch which adds that in; I've confirmed that this now logs the IP e.g.
Comment #50
mcdruid commentedAs per the original suggestion in #23 here's a patch which uses the new
UserFloodControlservice in\Drupal\user\Controller\UserAuthenticationController.One quirk I came across was that the controller uses two names for the different types of flood control (per user / per IP), but only one of these is shared with
\Drupal\user\Form\UserLoginForm. The two names relate to essentially the same two different options though.That's led to this in
\Drupal\user\UserFloodControl:Instead of doing that, we could replace
user.http_loginwithuser.failed_login_userwithinUserAuthenticationControllerwhich might be more consistent.I am also not crazy about this code, which handles the different forms of "identifier" used by flood control depending on the different settings.
There are several other ways we could do this which might be more elegant than the
strpos()I threw in there to avoid an error that came up with the previous code when runningDrupal\Tests\user\Functional\UserLoginHttpTest.Both
UserLoginHttpTestandUserLoginTestnow check for the (watchdog) log messages emitted by flood control, and both pass locally. Let's see what the testbot thinks...Comment #51
andypostQuick review - main question is backward compatibility for form class
Also needs work to fix coding standards - 5 https://www.drupal.org/pift-ci-job/1353387
Controllers are fine to override without BC https://www.drupal.org/core/d8-bc-policy#controllers
needs doc-block updates for arguments
I bet it no-go because no BC and this form very probably overriden in contrib/custom code
nit, needs newline
Comment #52
ghost of drupal pastRe #51.3 , if you want to see how it's done you can run
git log -p --pickaxe-regex -S'__construct.*NULL' core/modules|grep deprecated|lessand find examples like:@trigger_error('Calling FileSystemForm::__construct() without the $file_system argument is deprecated in drupal:8.8.0. The $file_system argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3039255', E_USER_DEPRECATED);@trigger_error('Invoking the UpdateManager constructor without the module extension list parameter is deprecated in Drupal 8.8.0 and will no longer be supported in Drupal 9.0.0. The extension list parameter is now required in the ConfigImporter constructor. See https://www.drupal.org/node/2943918', E_USER_DEPRECATED);Comment #53
mcdruid commentedThanks for the reviews!
This is just a reroll initially; I'll work on the changes next.
Comment #54
mcdruid commentedI believe we'll need to add deprecation tests. I'll look at #1681832-145: Password reset form has no flood protection for an example.
We may still want to address #51.1 if that's practical?
Checking whether tests pass with these changes first though.
Comment #55
mcdruid commentedOops, interdiff snafu.
Comment #57
mcdruid commentedThink I managed to re-introduce a couple of Url-related deprecations with the reroll.
Fixed those and a couple of other untidy codesniffer-type issues.
Having removed the type-hinting for UserFloodControl there's now an Unused use statement for it, but I'm guessing we can ignore that in this situation.
Still need to add the deprecation tests if this patch passes.
Comment #58
larowlanThis is looking good, I've got some suggestions around the API for future-proofing and design aspects.
I think we should be typehinting an interface here, but I think we need to remove the typehint altogether and do the BC dance 💃
I think this is an anti-pattern, lets keep these as separate variables and separate arguments/properties on the object with their own getters.
It will use less memory, but also make the API more explicit/discoverable
it feels like we could be smarter about this, and avoid leaking implementation details outside the event by adding methods on the event to get the iP and uid as well as a 'hasIpAddress' method
Otherwise, all the consuming code will have to have knowledge of the identifier's internal format, and if we want to change it later we'll have to do it in N places, or cause a BC break.
If we keep it internal to the event, we can change it later without impacting consuming code.
nit: we could return early here and avoid the else
should we do the same thing for the other controller ?
however, I think what we should be doing here is making UserFloodControl implement FloodInterface and proxy the calls to the decorated storage.
And then this becomes
!$user_flood_control instanceof UserFloodControlInterfacedoesn't the ->storage method type-hint it's return?
however, I think this is a code-smell - we shouldn't be talking to our neighbour's neighbour.
By making UserFloodControl implement flood interface and decorate the existing flood storage, these changes can be reverted
This is a behaviour change - is there a reason for that? Edit: from the issue summary
Are there BC implications to this? We're immediately halting execution, so other form submission/validation handlers won't run. I think that might be an issue. Should we be using $formState->setResponse() instead? So that other processing continues first?
Also, I might be wrong, but I don't see a new test for the 403 status code - if we don't have that - we should add it.
I feel like we should have an interface here if we're making it a service, so people can swap/typehint.
In addition, I think we should be making this implement FloodInterface and proxy all the related calls to the storage.
Then calling code doesn't need to change?
the else isn't needed here, we returned early 🎉
nit: my personal preference would be to have a map of event names to flood names and use that to convert, but only because I think switch is an anti-pattern.
any reason we pass these as a map and not as seperate arguments?
yes, given this is a service, we definitely want an interface for this so we can make it swappable.
Comment #59
larowlanFYI: The neighbour's neighbour thing is called Law of Demeter in OO design - its not just me making up some arbitrary thing :)
Comment #60
kim.pepperUserFloodControlInterface?Maybe we can invert this and do
if (!$user_flood_control instanceof UserFloodControl)I think that if you have a service, then it shouldn't expose the internals like this. Maybe hide
UserFloodControl::storage()and add aUserFloodControl::clear()? I guess then it's looking more like FloodInterface...Have we considered just implementing FloodInterface, and adding an
EventDispatchingFloodWrapperthat wraps a flood?Comment #61
mcdruid commentedThanks for the really detailed reviews and excellent suggestions.
Looking at @larowlan's comments:
...and @kim.pepper's question (which was actually in slack):
I'm trying to get my head around the proposed architecture here; does this make sense?
Noting that this is partially the direction I was going in #29 but @alexpott's feedback made me change tack (which was perhaps down to my misinterpretation?)
For decoration, I'm looking at https://www.previousnext.com.au/blog/decorated-services-drupal-8 and hoping that this now works properly (the core issue that mentions is fixed).
I think the pseudo-code above makes sense to me and I can work on it, but would appreciate a sanity check.
Comment #62
mcdruid commentedHere's a first pass at implementing the architecture changes outlined in #61.
I looked at
\Drupal\jsonapi\Routing\ReadOnlyModeMethodFilterfor some ideas; it looks somewhat similar in terms of the decoration and implementing an interface which extends another interface etc..\Drupal\Tests\user\Functional\UserLoginHttpTestand\Drupal\Tests\user\Functional\UserLoginTestboth pass for me locally, so let's see if anything else is broken.I've fixed a couple of the other things along the way, but still on the todo list:
I'll work my way through these ASAP.
I'm not sure if
{@inheritdoc}is legit for the methods where we're calling the decorated flood service's methods?Comment #63
mcdruid commentedThis patch addresses:
\Drupal\user\Event\UserFloodEvent::__construct()but still feels a little flimsy.I tried to implement the suggestion in 58.7 of using
$form_state->setResponseinstead of throwing an exception:This seems like a great idea (and would mean we don't need the $messenger service which required a BC dance etc..)
However, it doesn't seem to work.
We get through
FormBuilder->processForm()with the 403 Response sitting in the$form_state, but we land at the end ofFormBuilder->buildForm():Because we've not returned an actual
Response, the processing gets all the way to the end here and the form is rendered (as a 200) despite the 403 Response still being in the$form_state. Which is a bit frustrating.We could try returning the Response so that we end up throwing the
EnforcedResponseException, but as we're in a validate function here, I'm not certain how we'd do so.Plus I'm not sure what we're gaining; if we return a basic 403 response (a bit like
\Drupal\ban\BanMiddleware::handle()does) the error messages we're setting won't be displayed, and we'd just end up throwing a different exception a few steps later, as far as I can see.I'm inclined to stick with the messenger / AccessDeniedHttpException implementation unless there's some Form API magic tricks that I'm missing here - which is entirely possible.
That leaves:
\Drupal\user\UserFloodControl::isAllowed()Comment #64
larowlanI don't think we need this line, we're decorating but not in the symfony sense, just in the general OO sense. The symfony sense makes it take over the item being decorated, we want this as a stand-alone service in my book, not to replace the flood service - which I think is what alexpott said in 29 too.
Sorry for using an overloaded term like decorate, really, its just proxying. i.e. in summary, remove the 'decorates:' line from services.yml and see what breaks?
should this have methods - anything public in UserFloodControl above what is in FloodInterface should be here I think?
this could be simplified by adding returns.
Then it becomes
if
...
return
if
...
return
...
And then we have no elseif or else
And having it as an internal implementation detail of the event++
❤️
I think you may need to step through
\Drupal\Core\Form\FormSubmitter::doSubmitForm- it should return a response if you've called $formState->setResponse, which should bubble out to that processForm call. But there are a few code paths in that method, and only the last one returns a response, so its possible that one of the earlier code paths is executing, because of some other value in form state.Comment #65
mcdruid commentedThanks for the quick feedback.
64.1 Okay, thanks for clarifying. Hopefully it'll be easy enough to pass flood as a normal service instead; I'll change that around.
64.2 "anything public in UserFloodControl above what is in FloodInterface" - I don't think there are any methods other than those in FloodInterface; we're effectively just
decoratingadding toisAllowed().64.3 Okay, will put returns in.
64.4 :)
64.5 I think the reason I wasn't getting as far as
doSubmitFormis this (in\Drupal\Core\Form\FormBuilder::processForm):There are errors, as we're setting them on the form when flood control kicks in during validation.
If I change the errors back to messages:
...we do then get the form to submit, but we hit an error because the $uid has not been set:
We can get around this by setting the uid:
...which does then result in the (very plain) 403 being served. The error message we set in
\Drupal\user\Form\UserLoginForm::validateFinaldisappears into the ether. (Well, it will be displayed if the blocked user visits another page.)I believe this is actually throwing an exception from
\Drupal\Core\Form\FormBuilder::buildForm:Do we really want to do that instead of throwing the
AccessDeniedHttpExceptionourselves? Doing so gives us the 403 and renders the access denied page, including displaying the error message.Comment #66
mcdruid commentedThis patch addresses:
So the only thing I haven't done is remove the
AccessDeniedHttpExceptionin UserLoginForm and set a 403 Response using$form_state->setResponse().That would be a fairly easy change to make if it really would be better.
Comment #67
kim.pepperI think this is ready.
Comment #69
andypostneeds formatting according standards https://www.drupal.org/core/deprecation
Comment #70
andypostBasically
s/will be/isComment #72
andypostFix CS and deprecation message in test
Comment #73
mcdruid commentedThanks @andypost - this fixes one tiny typo in your patch.
I think we can go back to RTBC if this passes.
Comment #74
mcdruid commentedD'oh. As soon as I hit Save I noticed a typo I'd made in the comments for
hasIp()/hasUid().Comment #75
mcdruid commentedBack to RTBC per #67 as we've only made a couple of very minor text changes since then to tidy up.
Comment #76
mcdruid commentedAny chance we can get this into the alpha for 8.8.x?
Comment #77
larowlani'm still not sure about this change, it circumvents other form processing, returning immediately and removes the ability for other form validation/submission handlers to run
From #66 you indicated this would be fairly easy?
I think on the basis of the fact we are cutting other validation handlers out of the flow, we need to go that route, sorry.
Comment #79
mcdruid commentedOkay. If we're setting a Response into the form state instead of throwing the exception, I think we can do away with using the messenger service. So that's less code, and removes one of the changes to the form's constructor, all of which is good.
In #65 I mentioned doing:
$form_state->set('uid', 0);...to avoid an error in
UserLoginForm->submitForm().Instead of that hack, I've added a check to the
submitForm()method so we can bail out if there's no uid.Comment #80
andypostComment #81
mcdruid commentedAre we back to RTBC?
Comment #82
mcdruid commentedRerolled for 8.9.x
There was a small conflict caused by adding
dblogas a module to install at the start of\Drupal\Tests\user\Functional\UserLoginTestwhere the$defaultThemehad also been added.Comment #83
darrenwh commentedVery minor, but this method is missing typecasting for the variables
Comment #84
prabha1997 commentedComment #85
prabha1997 commentedComment #86
swatichouhan012 commentedi have Updated patch #82 wrt comment #83, Kindly review new patch.
Comment #88
larowlanRe #83 and #86 we don't do scalar type-hints yet that I'm aware of.
Re #85 - the patch changed by 15Kb, looks like something went wrong there.
So this is a review of #82
this will need to say 9.1.0 now (sorry 😿)
looks like a find and replace went wrong here
Comment #89
narendra.rajwar27Comment #90
narendra.rajwar27Updated patch as mentioned in #88 and also for branch 9.1.x. as patch was not applying for 9.1.x.
Comment #92
narendra.rajwar27As patch applied and working as expected. On multiple failed login attempts it is logging meaningful message. But after every attempt, it is breaking the login form.
See screenshot:
Updating patch.
Comment #94
narendra.rajwar27Comment #95
mcdruid commentedThanks @narendra.rajwar27, however I _think_ the deprecation would now be happening in 9.1.x and the BC layer would be removed in 10.0.0
@larowlan also mentioned in #88 that type hinting of the scalar params is not something we should be adding (yet).
As that review was of #82 I've based this new patch on that one, but interdiff isn't working between 82 and 95.
These are the actual changes, but sadly without the context of the filenames etc..:
We'll need to update the draft CR mentioned in those deprecation messages too.
Comment #97
mcdruid commentedAre you happy for this to go back to unassigned @narendra.rajwar27 ?
I think we should be back at RTBC with any luck, but it'd be good if someone else could confirm that.
Comment #98
narendra.rajwar27Comment #99
narendra.rajwar27@mcdruid, i have mentioned an issue and added screenshot in #92. It is not addressed. you can refer the interdiff for the same in comment #92.
Comment #100
mcdruid commentedSorry @narendra.rajwar27 I don't think I understand.
It's intentional to block logins via flood when too many have been unsuccessful, and part of the improvement we're trying to make here is to return a 403 when this happens (see the IS).
One of the test failures in #92 was because the assertion checking for that 403 received a 200 instead:
What is the issue that needs to be addressed?
Comment #101
narendra.rajwar27@mcdruid, Here it is:
After applying patch, On each login attempt failure. I am getting broken login form. So issue needs work on this part.
Please refer interdiff in comment #92
Comment #102
narendra.rajwar27making this to needs work.
Comment #103
mcdruid commented@narendra.rajwar27 I've just manually re-tested, and I think the behaviour you're describing is the desired functionality of the user login flood control, unless I'm missing something.
First we can lower at least one of the limits in config to make manual testing easier:
If we try a few logins which will fail now, the first few will re-render the login form along with an error message:
Once we exceed the low IP flood limit though, Drupal responds with a very simple error to explain that we've been temporarily blocked, and doesn't re-render the login form:
That's not a bug, that's the desired behaviour.
We don't need to re-render the login form as we won't be allowing login attempts from this IP for a while. It's cheaper to return the simple message; often Drupal will be responding to a bot attempting to brute force logins in this situation, so that's better. We also want to return a 403 as we are denying access, and it's good for that to be logged by the webserver (per the IS).
The behaviour is pretty much the same when flood control is triggered for a particular user name / account, except there's a different threshold and the message is slightly different - that's what your screenshot is showing.
Does that help to explain what you're seeing happen, or is there something else that we need to address?
Comment #104
narendra.rajwar27@mcdruid, If that is the case then it is ok.
Comment #105
amjad1233I applied the patch, which applies cleanly and seems to be working as described.
Also, Flood logging is correct with 403 I can see entries as below screenshot.
Just one concern was if I use a legitimate username it's kind of giving out a hint of that user exists in the system.
For example, if I have a user test123 and I keep entering the wrong password after 5 attempts I get the error of flooding. But if I try with a non-existing username it keeps going.
With that said, I found this is the same behaviour in the password reset screen. When you enter a non-existing username it will say " is not recognized as a username or an email address." while if you enter the right username it will say email has been sent.
Which means someone can figure out if the username exists in the system.
Reference: https://security.stackexchange.com/questions/62661/generic-error-message...
While that's a bigger discussion and I am sure there will be some discussion around it in community. I am RTBTC this one.
Comment #106
larowlanThanks @amjad1233 please note that we have a policy on that - https://www.drupal.org/drupal-security-team/security-team-procedures/dis... - so I think that's ok.
Comment #107
larowlanSorry to do this, but I think we're a bit shy of the docs-gate on this.
Looking at other Event classes in core e.g ConfigEvents they also have a long-description in the constant docblocks describing what sort of things listeners can do and the argument that is passed as well as some @see tags to the argument and the places where the event is fired from.
I think we can add some more information here, so people can get the maximum use of this new extension point.
I really appreciate the effort that's gone into this, and its so close. But I think we need to make sure we document the event to similar levels of existing events.
I'll keep a close eye on this issue.
Comment #108
mcdruid commentedDefinitely agree that additional documentation is beneficial for the new events.
ConfigEvents was a good example, thanks.
Comment #109
mcdruid commentedThe wrapping of one of the comments was a little wrong.
Comment #112
mcdruid commentedOkay, let's try that again.
Comment #113
amjad1233Patch applies cleanly works as expected.
Comment #114
larowlanSaving issue credit
Comment #116
larowlanCommitted 9461f4a and pushed to 9.1.x. Thanks!
Published the change record.
Thanks everyone
Comment #117
andypostIt could use to update it to protected, filed follow-up #3150070: Fix new test modules variable visibility
Comment #118
mxr576Should this new service also used by the
basic_auth.authentication.basic_authservice instead of flood?Comment #120
cilefen commentedSorry for the noise, but did this cause a behavior change? #3198010: User login page broken when there are more than 5 failed login attempts for an account
Comment #121
mcdruid commented@cilefen yes it looks like this did change the way that the flood "access denied" messages are themed. I can't honestly recall whether that was intentional.
Let's continue the discussion in the new issue you linked to. Thanks!