Basic auth has no login flood control

On #2041885: Move HTTP basic authentication provider to a separate module there was some discussion about removing this from core, however:

1. I don't think there's an issue yet

2. This issue would be critical against the contrib module too.

#2159937: Remove basic_auth

Thanks. Re-opened.

Note that restws_basic_auth uses drupal_form_submit('user_login', $form_state); which processes all user login validators including flood control, so this works as designed in D7 RESTWS.

So what would the equivalent be for Drupal 8 for flood protection?

While I investigate on how the Flood service works for the user login form; isn't it possible to suggest to install a piece of software like Failtoban at the server to prevent this?

First pass.

I manually verified that both IP based and user based blocking mechanisms work. But for some reason one request implies 4 new entries written in the flood table instead of two (one for ip based and a second for user based, if the username was an existing one).

I am also using Drupal::service at the constructor (ugh). How could I use the dependency injector?

Finally, I still have to add tests to this.

Apart from the above. It works.

+++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
@@ -29,13 +31,31 @@ class BasicAuth implements AuthenticationProviderInterface {
+    $this->userStorage = \Drupal::service('entity.manager')->getStorageController('user');

You can inject entity.manager, and then in the constructor only save the result of calling getStorageController if that's all you need.

The flood problem seems not to be specific with the basic_auth module. Could we move this logic maybe to a base class and let people reuse it?

Thanks @crell. Now injecting the entity manager.

I saw that the authenticate method is called twice: one to authenticate the user and then a second time to authenticate the request. This is the reason why two entries are logged at the flood table.

As for @dawehner's suggestion, I totally agree. I felt I was copying and pasting too much code while implementing this. Could we use a UserFlood service that loads the user.flood settings and wraps the long method calls that the flood service is doing at the applies method implementation of this patch?

The way this works for basic auth is quite different from cookie/session based logins. \Drupal\Core\Authentication\Provider\Cookie doesn't have to care about flood protection because all it does is validate the cookie, login/authentication/flood protection happens somewhere else and always will.

Same for OAuth, the authentiation provider just needs to validate the token, which doesn't need flood protection.

So not sure if it really makes sense to have this in a default implementation, what other mechanism could actually use it?

Also wondering if basic auth flood protection should affect normal logins or if they should be separated?

Added a separate config file for basic_auth flood protection, as suggested by @berdir. It gives admins more flexibility to decide whether it should behave differently than normal logins or not.

Also added a test. I still need to figure out why a request is authenticated twice and therefore two flood entries are logged in for a failed login.

Adding configuration schema for the new config basic_auth.flood...

The following dependency chain provokes an extra authentication before we actually want to authenticate the user against the request:

Note: this is extracted from the generated DependencyInjector container.

    /**
     * Gets the 'route_processor_manager' service.
     *
     * This service is shared.
     * This method always returns the same instance of the service.
     *
     * @return Drupal\Core\RouteProcessor\RouteProcessorManager A Drupal\Core\RouteProcessor\RouteProcessorManager instance.
     */
    protected function getRouteProcessorManagerService()
    {
        $this->services['route_processor_manager'] = $instance = new \Drupal\Core\RouteProcessor\RouteProcessorManager();

        $instance->addOutbound($this->get('route_processor_csrf'), 0);

        return $instance;
    }

route_processor_csrf service instantiates csrf_token service, which in turn requests the current_user service and therefore it authenticates the request before expected.

I need some guidance here. There is one test that fails because flood attempts are logged twice because of this issue.

Adding a label to the config schema definition and setting the right number of login attempts, which will fail because of what I explained in the previous comment.

15: 2160021-basic_auth-flood-15.patch queued for re-testing.

Good. So now the testbot reported one failing test which demonstrates my open question at #14

I'm not sure how possible that is, as you don't really know if the current user service has a user. You can't check that in the container; has() will always have the service definition, and calling get() will invoke the authenticate stuff.

We may need to look at what exactly is calling current_user earlier than auth providers.

Since yesterday I am facing this error while debugging this issue. There is a circular dependency reference. I have no idea why this is not happening on tests:

Steps:

1. Enable REST and allow GET on node with basic auth.
2. Create a user. Allow permission on GET method to authenticated users on Content.
3. Create a node.
4. Open http://d8.local/entity/node/1 >> provide username and password.

Yes, I just had that as well, the problem for me was devel, which does a user_access() in devel_shutdown_real().

Created #2182055: Comment module causes Circular reference error on a REST request to address the circular reference issue before continuing on this one.

Issue #2182055: Comment module causes Circular reference error on a REST request is not about comment module, this only about csrf that executed before the auth happens

1. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
   @@ -51,12 +74,47 @@ public function applies(Request $request) {
   +    // Do not allow any login from the current user's IP if the limit has been
   

   So these are all copied comments and code. It's a bit sad that we need to duplicate this. Please add at least a @see reference and that this is duplicated code.

2. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
   @@ -51,12 +74,47 @@ public function applies(Request $request) {
   +          $uid = user_authenticate($username, $password);
   

   Calling global functions means it will be hard to unit test this at all. Looks like there is no OOP replacement yet for user_authenticate()?

3. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
   @@ -51,12 +74,47 @@ public function applies(Request $request) {
   +            $account = user_load($uid);
   

   Do not call user_load() here, you already got the user storage controller injected, so we should load from there.

4. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Tests/Authentication/BasicAuthTest.php
   @@ -62,6 +62,67 @@ public function testBasicAuth() {
      /**
   +   * Test the global login flood control.
   +   */
   +  function testGlobalLoginFloodControl() {
   

   So this is a slow web test. I would rather like to see a PHPUnit test to test the code in isolation. But on the other hand an integration test also makes sense here, so not sure about that.

Calling global functions means it will be hard to unit test this at all. Looks like there is no OOP replacement yet for user_authenticate()?

I already had a quick branch I created locally for that a while ago but forgot to open an issue/post the patch. So here we go #2206687: Replace user_authenticate with a UserAuth service

klausi opened a new pull request for this issue.

So this is one of the worst hacks I ever did, but it ensures that we can make progress here while the authentication system is wrongly called twice. Interdiff: https://github.com/klausi/drupal/commit/a9c3656981fc4137c755eeb29219f1bc...

This introduces a workaround, but solves another critical. On top of that, this would certainly not make it harder to fix the current user issue.

1. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
   @@ -60,12 +84,61 @@ public function applies(Request $request) {
   +    // @todo Authentication providers are called twice due to a bug, so we
   +    // statically remember the outcome of the authentication from the first
   +    // invocation. Obviously this is a ugly hack and should be fixed in
   +    // https://drupal.org/node/2180109
   +    static $called = FALSE;
   +    static $cached_user = NULL;
   +    if ($called) {
   +      return $cached_user;
   +    }
   +    $called = TRUE;
   

   It is kind of good to know that we found out about the two-times counting.

2. +++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
   @@ -60,12 +84,61 @@ public function applies(Request $request) {
   +            $this->flood->clear('basic_auth.failed_login_user', $identifier);
   +            $account = $this->userStorage->load($uid);
   

   this is soooooo much nicer than the cookie based one.

I still think we should just wait on https://drupal.org/comment/8615067#comment-8615067, as that will fix this issue.

So we can remove that now current user issue just got in? :)

Why do we need to duplicate the user.flood config file into basic_auth's configuration?

It appears that basic_auth has a dependency on User module anyway already? (next to the sad fact that User module is a required module...)

That is a good point, I had not twigged this was user.flood incarnate :) I agree totally - Let's remove it.

+++ b/core/modules/basic_auth/lib/Drupal/basic_auth/Authentication/Provider/BasicAuth.php
@@ -37,14 +39,36 @@ class BasicAuth implements AuthenticationProviderInterface {
+    $this->userStorage = $entity_manager->getStorageController('user');

was renamed to getStorage()

Yes, just realised :) Thanks. I think klausi is working on this now.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Looks like the last reroll had a rebasing issue, well - lack of :)

klausi opened a new pull request for this issue.

Sorry for the noise, somehow my pull request was rebased/broken.

The problem is that the authentication is still triggered twice if an exception occurs, the request is changed and the account proxy runs the authentication again triggering the flood control too early.

So I think we can just remove the synchronized request from the account proxy, as authentication should really happen once and not on subrequests.

Interdiff: https://github.com/klausi/drupal/commit/a2eed573b0dea17822c77b6fde290239...

Awesome!

Yes, I think this looks good. Just removes the setRequest stuff and passes it into the constructor, which makes sense if we don't want to change it.

EDIT: x-posted with dawehner. I was also going to RTBC!

This looks great, bonus to have one less subrequest.

Committed/pushed to 8.x, thanks!