Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController

Thanks @znerol. Looks very good. Here are few comments.

1. +++ b/core/modules/basic_auth/basic_auth.services.yml
   @@ -1,6 +1,9 @@
   +    class: Drupal\user\Flood\LoginFlood
   

   Nice! we don't need to duplicate basic_auth and contrib can extend LoginFlood, if overrides needed.

2. +++ b/core/modules/user/src/Flood/LoginFlood.php
   @@ -0,0 +1,139 @@
   + * Contains \Drupal\user\Flood\UserLoginFlood
   ...
   +class LoginFlood implements LoginFloodInterface {
   

   Just has loginFlood, but I do prefer UserLoginFlood.

3. +++ b/core/modules/user/src/Flood/LoginFlood.php
   @@ -0,0 +1,139 @@
   +  }
   

   ideally, @flood should be @flood.default_storage and alias of @flood.storage.db

3. I don't think that is needed or how we do it elsewhere? https://www.drupal.org/node/2306083

1. +++ b/core/modules/basic_auth/basic_auth.services.yml
   @@ -1,7 +1,10 @@
   +  basic_auth.login_flood:
   +    class: Drupal\user\Flood\LoginFlood
   +    arguments: ['@flood', '@entity.manager', '@config.factory', 'basic_auth']
   ...
   +    arguments: ['@config.factory', '@user.auth', '@basic_auth.login_flood', '@entity.manager']
   
   +++ b/core/modules/user/src/Flood/LoginFlood.php
   @@ -0,0 +1,139 @@
   +  /**
   ...
   +    $this->config = $config_factory->get('user.flood');
   

   We have here potentially a little bit more conservative ... given that authentication providers are constructors quite early. Better do it lazy, to avoid all kind of potential problems.

2. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -82,49 +82,20 @@ public function applies(Request $request) {
   +    $ip = $request->getClientIP();
   +
   +    if ($this->flood->isAllowedHost($ip) && $this->flood->isAllowedAccount($ip, $username)) {
   

   I curious whether the two methods could / should be moved into a common helper method. IsAllowedRequest($ip, $username) or something like that.

3. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -82,49 +82,20 @@ public function applies(Request $request) {
   +        $this->flood->clearAccount($ip, $username);
   

   Should we name the service loginFlood?

4. +++ b/core/modules/user/src/Flood/LoginFlood.php
   @@ -0,0 +1,139 @@
   +class LoginFlood implements LoginFloodInterface {
   

   I first thought it would be a good idea to move the namespace into the methods, but it turned out, just no, it would be horrible DX wise.

5. +++ b/core/modules/user/src/Flood/LoginFloodInterface.php
   @@ -0,0 +1,73 @@
   +
   +/**
   + * Provides a flood service tailored to user login.
   + */
   +interface LoginFloodInterface {
   +
   

   If you compare FloodInterface with LoginFloodInterface it almost seems to be as if it could be named IpAwareFloodInterface ... at least parts of it.

1. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -82,49 +82,20 @@ public function applies(Request $request) {
   +    if ($this->loginFlood->isAllowedHost($ip) && $this->loginFlood->isAllowedAccount($ip, $username)) {
   

   isAllowedHost() looks more related to ban module, no idea now how to do that right

2. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -82,49 +82,20 @@ public function applies(Request $request) {
   +      if ($uid && !user_is_blocked($username)) {
   +        $this->loginFlood->clearAccount($ip, $username);
   

   user_is_blocked looks the same as isAllowedAccount()

3. +++ b/core/modules/user/src/Flood/LoginFlood.php
   @@ -0,0 +1,161 @@
   +    $config = $this->configFactory->get($this->configName);
   ...
   +    $config = $this->configFactory->get($this->configName);
   ...
   +    $config = $this->configFactory->get($this->configName);
   ...
   +    $config = $this->configFactory->get($this->configName);
   

   too much calls

4. +++ b/core/modules/user/src/Flood/LoginFlood.php
   @@ -0,0 +1,161 @@
   +    $this->flood->register($this->floodPrefix . '.failed_login_ip', $config->get('ip_window'), $ip);
   ...
   +      $this->flood->register($this->floodPrefix . '.failed_login_user', $config->get('user_window'), $identifier);
   ...
   +    return $this->flood->isAllowed($this->floodPrefix . '.failed_login_ip', $config->get('ip_limit'), $config->get('ip_window'), $ip);
   ...
   +      $allowed = $this->flood->isAllowed($this->floodPrefix . '.failed_login_user', $config->get('user_limit'), $config->get('user_window'), $identifier);
   ...
   +      if ($config->get('uid_only')) {
   

   seems easy to pass a readonly config object to constructor

5. +++ b/core/modules/user/src/Flood/LoginFlood.php
   @@ -0,0 +1,161 @@
   +  protected function getAccountIdentifier($ip, $name) {
   ...
   +        return $account->id();
   ...
   +        return $account->id() . '-' . $ip;
   
   +++ b/core/modules/user/src/Flood/LoginFloodInterface.php
   @@ -0,0 +1,73 @@
   +  public function register($ip, $name);
   ...
   +  public function clearAccount($ip, $name);
   ...
   +  public function isAllowedAccount($ip, $name);
   

   I'd swap order of arguments to be more like result

6. +++ b/core/modules/user/src/Form/UserLoginForm.php
   @@ -166,40 +165,19 @@ public function validateName(array &$form, FormStateInterface $form_state) {
   -        $form_state->set('flood_control_user_identifier', $identifier);
   ...
   +        $form_state->set('flood_control_triggered', 'user');
   
   @@ -213,17 +191,13 @@ public function validateAuthentication(array &$form, FormStateInterface $form_st
   -      if ($flood_control_user_identifier = $form_state->get('flood_control_user_identifier')) {
   

   this naming change breaks BC for the login form, better to change back

This would be useful for the user edit form as well. #2339399: User edit form does not use flood control and allow for password brute force attacks The issue summary should be updated accordingly.

Since #2339399: User edit form does not use flood control and allow for password brute force attacks is major and is blocked on this, moving this to Major (right?).

I think that CredentialCheckFlood is a good name for it since it is not only about Login.

We should also re-use this on the used edit form when the user enters their password - if they leave the account logged in, there is no brute-force protection

@pwolanin +100 but maybe we need separate issue for that (scope is not clear)

@pwolanin - is that #2339399: User edit form does not use flood control and allow for password brute force attacks ?

@greggles Yes.

This looks pretty good! Dug pretty deep through the changes and things almost identical just refactored which is great. Have a couple concerns though.

1. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -82,49 +82,20 @@ public function applies(Request $request) {
   +    if ($this->credentialsCheckFlood->isAllowedHost($ip) && $this->credentialsCheckFlood->isAllowedAccount($ip, $username)) {
   ...
   +        $this->credentialsCheckFlood->clearAccount($ip, $username);
   ...
   +    $this->credentialsCheckFlood->register($ip, $username);
   

   isAllowedAccount() calls getAccountIdentifier() does an uncached property lookup and then both the clearAccount() and register() call it again and do another uncached lookup of the same account. That means on both the pass and fail path we do at least 2 lookups and then the cached entity retrieval just in the flood code. That doesn't seem ideal.

2. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -82,49 +82,20 @@ public function applies(Request $request) {
   -    return [];
   

   is loosing this ok?

3. +++ b/core/modules/user/src/FThis seems pretty different. I need to see if this is addressed in the comments.orm/UserLoginForm.php
   @@ -244,10 +218,8 @@ public function validateFinal(array &$form, FormStateInterface $form_state) {
   -    elseif ($flood_control_user_identifier = $form_state->get('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.
   -      $this->flood->clear('user.failed_login_user', $flood_control_user_identifier);
   +    else {
   +      $this->credentialsCheckFlood->clearAccount($ip, $form_state->getValue('name'));
   

   This confused me at first too but for future reviewers it is because clearAccount has access to the id builder in the flood class so it doesn't have to cache the earlier resolution and use it. Previous comments about adding uncached lookups applies though.

1: Added drupal_static caching to \Drupal\user\Flood\CredentialsCheckFlood::getAccountIdentifier()
2: according to the \Drupal\Core\Authentication\AuthenticationProviderInterface, it should either return

• AccountInterface - in case of a successful authentication, or
• NULL - in case where authentication failed.

Since we have a failed authentication, not returning (NULL) seems better than returning an empty array.

I feel stupid now.

The bots like this now. What manual testing is needed?

re-uploading the patch for the bot.

+++ b/core/modules/basic_auth/basic_auth.services.yml
@@ -1,7 +1,10 @@
+  basic_auth.credentials_check_flood:

Why can't basic_auth.authentication.basic_auth use the service provided by user module?

The service needs to inject EntityTypeManager instead of EntityManager.

The code has no reason to do $config = $this->configFactory->get($this->configName); in every method, that can be done in the constructor.

$accounts = &drupal_static(__FUNCTION__);

We should not be using drupal_static in a class. Just use a class attribute.

I'm interested in the answer to #34 too.

For now here's a reroll with fixes for #36.

1. +++ b/core/modules/user/src/Flood/CredentialsCheckFlood.php
   @@ -0,0 +1,158 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\user\Flood\CredentialsCheckFlood.
   + */
   
   +++ b/core/modules/user/src/Flood/CredentialsCheckFloodInterface.php
   @@ -0,0 +1,71 @@
   +/**
   + * @file
   + * Contains \Drupal\user\Flood\CredentialsCheckFloodInterface.
   + */
   

   Unneeded now

2. +++ b/core/modules/user/src/Flood/CredentialsCheckFlood.php
   @@ -0,0 +1,158 @@
   +class CredentialsCheckFlood implements CredentialsCheckFloodInterface {
   ...
   +  public function register($ip, $name) {
   

   All the methods deal with IPs so I'm wondering whether it should be part of the name of the interface

3. +++ b/core/modules/user/src/Flood/CredentialsCheckFlood.php
   @@ -0,0 +1,158 @@
   +  protected function getAccountIdentifier($ip, $name) {
   +    if (!isset($this->accounts[$name])) {
   +      $storage = $this->entityTypeManager->getStorage('user');
   +      $account_by_name = $storage->loadByProperties(['name' => $name]);
   +      $this->accounts[$name] = reset($account_by_name);
   +    }
   +    /** @var \Drupal\Core\Session\AccountInterface $account **/
   +    $account = $this->accounts[$name];
   +    if ($account) {
   

   IMHO classes which provide internal state/storage should also have a reset method. On the other hand I seriously wonder whether this static cache is helpful. How likely will you call methods multiple times per request?

Removed the @file docblocks, fixed a stupid mistake that caused many of the test fails.

+++ b/core/modules/user/src/Flood/CredentialsCheckFlood.php
@@ -0,0 +1,153 @@
+  protected $config;
...
+    $this->configFactory = $config_factory->get($config_name);

Should be $this->config =

@tim.plunkett
D'oh. Rushing things is my middle name.

Interdiff got eaten.

Rerolled it a bit and worked on using it for \Drupal\user\Controller\UserAuthenticationController as well

Patch #47 applies cleanly, removing the tag...

#47 changed the wording on the access-denied-by-flood-protection-messages, which is causing the 2 test failures. Updated the messages in the test to make the testbot happy.

+++ b/core/modules/basic_auth/basic_auth.services.yml
@@ -1,7 +1,10 @@
+    arguments: ['@config.factory', '@user.auth', '@basic_auth.credentials_check_flood', '@entity.manager']

I think this is classified as a BC break.

i.e. we might need to retain the original signature and add the new service as a fifth optional argument

But I'd check with @alexpott or @catch to make sure that I'm right about this first, it might be an allowed BC break.

Looking good though!

The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

- https://www.drupal.org/core/d8-bc-policy#constructors

That's correct but we've started using the pattern of keeping the argument optional with a fallback to the controller in a few places recently and I like that. We usually use that if it's a class that is quite likely extend somewhere, which is probably not the case here, but there's an argument to be made about this case too:

If you use basic authentication or have the user login block placed on your site then applying this patch gives you a fatal error on every page, you can't even log in. Accessing update.php as anonymous won't clear the cache, so you have to change settings.php to allow anonymous access or use rebuild.php or so.

We could have a single issue to remove all those fallbacks in 9.x and add the same @todo everywhere.

I think this patch does not got far enough. BasicAuth.php should be just a thin wrapper that extracts username and password from the request, then calls some user-authentication-user-is-blocked-plus-flood-control service. That puts the flood and authentication logic into one central place and can be used from multiple locations. Because we always need to check if the user is blocked + the password matches + flood control.

This is security critical code and everytime people have to write an authentication provider they need to get this right. They should not have to think about this.

user is blocked + the password matches + flood control

In case of external auth password could be skipped

In case of external auth systems you can swap out the user.auth service. I'm saying we should have a user.all_inclusive_with_flood_control_plus_blocked_check service which uses user.auth (wraps it). With a better name :) user.login? user.login_flood? user.auth_flood? user.auth_check_flood?

The service can return a result object with a bool success true/false and a fail message in case authentication failed.

Let me try.

need to run now, work in progress patch.

Now demonstrating that we can share code between basic auth and user auth controller.

Still some rough edges in the patch:
* usage of t()
* no interface for the new service
* is it ok to keep the AuthResult class as data junk with public properties?
* very long authenticate() method in the new service, can we split it up in a meaningful way?

The next step will be to try to refactor UserLoginForm to use this new service. The biggest difference between HTTP API call messages and login form messages is that the latter can contain links (for example to reset your password). I'm not sure yet how we can deal with that difference.

Implemented UserLoginForm. I solved the problem with the messages by introducing constants as error codes for each error state. That way a consumer such as UserLoginForm can override the specific messages easily.

The code unification to one service also surfaced inconsistent behaviour for the HTTP API user login and the form login. I adapted the test case accordingly so that we have the same messages on forms as well as in API responses.

I was able remove large swaths of duplicated code, yay! Let's see where this blows up now.

TODO:
* Refactor authenticate() method because is has too many if/else levels
* Finalize the naming. How do you like the current user.auth_flood, UserAuthFlood and UserAuthFloodInterface names?

Cool, that went better than I thought.

1. +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
   @@ -77,49 +52,13 @@ public function applies(Request $request) {
   -    return [];
   

   How do we deal with this empty case here, don't we break the API somehow here?

2. +++ b/core/modules/user/src/Authentication/AuthResult.php
   @@ -0,0 +1,52 @@
   + * Represents the result after an authentication attempt.
   

   I really like the idea of a result object, that makes it clear to understand what is going on.

3. +++ b/core/modules/user/src/Authentication/AuthResult.php
   @@ -0,0 +1,52 @@
   +class AuthResult {
   ...
   +  public $success = FALSE;
   

   I was wondering whether we should add public methods. I've seen some committers freaking out about objects having no public methods. Personally I agree with you, this object is not meant to be swapped out

4. +++ b/core/modules/user/src/Authentication/UserAuthFlood.php
   @@ -0,0 +1,145 @@
   +          $auth_result->errorCode = AuthResult::ERROR_USER_BLOCKED;
   +          $auth_result->errorMessage = $this->t('The username @name has not been activated or is blocked.', array('@name' => $username));
   

   Is there a way we introduce custom static factories like: AuthResult::userBlocked($message) and AuthResult::success($user). We could then even have different objects one for success one for failure or so, basically similarily to the way how AccessResult works.

1. BasicAuth.php wrongly returned [], but AuthenticationProviderInterface says you should return NULL on authentication failure.

4. AccessResultInterface is quite a different beast because it offers functionality to combine multiple access results into one. It has quite a bit more of complexity than our authentication result here. I would avoid adding complexity if we don't have to - all we want to do here is provide a multidemensional result after the authentication. More like an array in the old days, but by using a class we can make the contents explicit and better document it. What would be the benefit of having 2 classes for success and failure? Same for having public methods, they would just be dumb getters for the properties.

I just feels wrong to write methods for on object that has zero behavior attached to it. It is just a data container. Anyone else think we should blow up the public methods on the AuthResult class?

More feedback from Crell and bojanz says that we should not do public properties, so let's do a full blown class with getters and an AuthResultInterface in the next iteration of this patch.

Non-public properties doesn't mean we need an interface. I agree with them and think we should not have public properties but that doesn't mean we need an interface.

Yeah I think single class + protected properties + methods is OK for this. The class with public properties is not much more defined than stdClass() so methods is good though, but no point in an interface unless we were going to add another class, which we've no intention of (and could add one then if we wanted).

now with public methods on AuthResult.

Also updated issue summary.

better title.

Forgot to convert one line in BasicAuth, tests now passing locally.

Rerolled the patch.

I was beginning to re-roll this... but I see that there is now a UserFloodControl service that separates out the logic around allowing requests and the registration of flood events.

It does not centralise the error messaging the way the patches here do.

Is this issue defunct or does it still have some value?

I see that there is now a UserFloodControl service that separates out the logic around allowing requests and the registration of flood events...Is this issue defunct or does it still have some value?

Yes, this issue is still active in so far as there's still repeated logic regarding user authentication... the user flood control service helps abstract out some of the inner workings/wrapping around the flood service, but it's not re-usable (e.g., by other authentication providers) for auth.