Part of #1775842: [meta] Convert all variables to state and/or config systems

The failed user login settings need to be converted to the new configuration management system

The variables are:

  • user_failed_login_identifier_uid_only
  • user_failed_login_ip_limit
  • user_failed_login_ip_window
  • user_failed_login_user_limit
  • user_failed_login_user_window
CommentFileSizeAuthor
#29 1777490-29-convert-user-failed-login-settings.patch8.15 KBkim.pepper
#25 1777490-25-convert-user-failed-login-settings.patch7.86 KBkim.pepper
#23 1777490-23-convert-user-failed-login-settings.patch8.1 KBkim.pepper
#18 1777490-18-convert-user-failed-login-settings.patch8.11 KBkim.pepper
#16 1777490-16-convert-user-failed-login-settings.patch8.12 KBkim.pepper
#12 1777490-12-convert-user-failed-login-settings.patch8.14 KBkim.pepper
#10 1777490-10-convert-user-failed-login-settings.patch8.25 KBkim.pepper
#7 1777490-7-convert-user-failed-login-settings.patch7.43 KBkim.pepper
#5 1777490-5-convert-user-failed-login-settings.patch7.27 KBkim.pepper
#1 1777490-convert-user-failed-login-settings.patch1.14 KBkim.pepper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Active » Needs review
FileSize
1777490-convert-user-failed-login-settings.patch1.14 KB

First variable is user_failed_login_identifier_uid_only

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Can you explain what these variables do?
It's possible we need to find better names for them.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

I think these are the flood settings for failed logins to prevent brute force password attempts.
There is no UI on core, but one is available in contrib.
There are a few bug reports regarding the lack of UI.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

What about this than:

failed_login:
  uid_only
  ip_limit
  ip_window
  user_limit
  user_window

I'm not 100% happy with this it's missing a flood reference. Maybe we need a user.flood.yml to save this settings?

So second proposal: user.flood.yml

uid_only
ip_limit
ip_window
user_limit
user_window
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
FileSize
1777490-5-convert-user-failed-login-settings.patch7.27 KB

Thanks for the review aspilicious. I agree with your 2nd approach and have attached a patch that moves these to user.flood.yml and converted the rest of the settings.

Kim

Status: Needs review » Needs work

The last submitted patch, 1777490-5-convert-user-failed-login-settings.patch, failed testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
FileSize
1777490-7-convert-user-failed-login-settings.patch7.43 KB

Forgot to save config

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1777490-7-convert-user-failed-login-settings.patch, failed testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review
FileSize
1777490-10-convert-user-failed-login-settings.patch8.25 KB

Added hook_update_N() for converting variables to config.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Needs work

Looks good, just some minor whitespace issues.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.phpundefined
@@ -25,11 +25,13 @@ class UserLoginTest extends WebTestBase {
+    $flood_config->save();
+    ¶

whitespace

+    $flood_config->set('user_limit', 3);
+    $flood_config->save();
+    ¶
     $user1 = $this->drupalCreateUser(array());
     $incorrect_user1 = clone $user1;
     $incorrect_user1->pass_raw .= 'incorrect';
@@ -139,7 +143,7 @@ class UserLoginTest extends WebTestBase {

whitespace

+
+/**
+ * Moves login flood settings from variable to config.
+ *
+ * @ingroup config_upgrade
+ */
+function user_update_8006() {
+  update_variables_to_config('user.flood', array(
+    'user_failed_login_identifier_uid_only' => 'identifier_uid_only',
+    'user_failed_login_ip_limit' => 'ip_limit',
+    'user_failed_login_ip_window' => 'ip_window',
+    'user_failed_login_user_limit' => 'user_limit',
+    'user_failed_login_user_window' => 'user_window',
+  )); ¶
+}
+  ¶

whitespace

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.phpundefined
@@ -61,11 +63,13 @@ class UserLoginTest extends WebTestBase {
+    ¶

whitespace

+++ b/core/modules/user/user.installundefined
@@ -525,6 +525,22 @@ function user_update_8005() {
+    'user_failed_login_user_window' => 'user_window',
+  )); ¶
+}
+  ¶

whitespace

+++ b/core/modules/user/user.installundefined
@@ -525,6 +525,22 @@ function user_update_8005() {
+  ¶

whitespace

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
FileSize
1777490-12-convert-user-failed-login-settings.patch8.14 KB

Fixed whitespace issues

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

identifier_uid_only sounds a bit verbose what is wrong with uid_only?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented

I was struggling with the right amount of verbosity in order to convey enough detail. I agree that identifier_uid_only could be changed to uid_only.

I wonder if we should change the settings file from user.flood.yml to user.login-flood.yml to give it more meaning?

Kim

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
FileSize
1777490-16-convert-user-failed-login-settings.patch8.12 KB

Renamed identifier_uid_only to uid_only

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Needs work 
+++ b/core/modules/user/user.installundefined
@@ -525,6 +525,22 @@ function user_update_8005() {
+    'user_failed_login_identifier_uid_only' => 'identifier_uid_only',

should be uid_only

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review
FileSize
1777490-18-convert-user-failed-login-settings.patch8.11 KB

Fixed missing rename of uid_only in update_hook

Status: Needs review » Needs work

The last submitted patch, 1777490-18-convert-user-failed-login-settings.patch, failed testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review

#18: 1777490-18-convert-user-failed-login-settings.patch queued for re-testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Issue tags: +Configuration system

Tagging as configuration system

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work
Issue tags: -Configuration system

looks good. only nitpick is code like this:

+    $flood_config = config('user.flood');
     // Set the global login limit.
-    variable_set('user_failed_login_ip_limit', 10);
+    $flood_config->set('ip_limit', 10);
     // Set a high per-user limit out so that it is not relevant in the test.
-    variable_set('user_failed_login_user_limit', 4000);
+    $flood_config->set('user_limit', 4000);
+    $flood_config->save();

should probably be:

  config('user.flood')
    ->set('user_failed_login_ip_limit', 10)
    ->set('user_limit', 4000)
    ...
    ->save();
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review
FileSize
1777490-23-convert-user-failed-login-settings.patch8.1 KB

I think the inline comments are important there, so that's why they are in expanded format. Is there a better way to put those comments around the config->set() calls?

I've re-rolled against 8.x.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

php allows code like this:

  config('user.flood')
    ->set('user_failed_login_ip_limit', 10)
    // Useful comment about why we do something.
    ->set('user_limit', 4000)
    ->save();

but, meh, it's mainly a style thing, so i don't think it should hold up the patch.

on the comments, "// Set the global login limit." is unnecessary and can go. the other comment seems useful because it has some 'why' in it.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
FileSize
1777490-25-convert-user-failed-login-settings.patch7.86 KB

Thanks for the feedback beejeebus. I've updated the patch to match your style recommendations, and removed the unnecessary comments.

Status: Needs review » Needs work

The last submitted patch, 1777490-25-convert-user-failed-login-settings.patch, failed testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review

#25: 1777490-25-convert-user-failed-login-settings.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1777490-25-convert-user-failed-login-settings.patch, failed testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review
FileSize
1777490-29-convert-user-failed-login-settings.patch8.15 KB

Re-rolled against latest 8.x

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

nice work, i think this is RTBC now.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Great! I committed it to 8.x. Thanks for the patch ping-pong; i.e. the attention to detail and persistence to get it right.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Added variables