Finish up login rate limit code. Set config item to 0 disables, make

sure config item can't be negative integer.

Author: rouilj

roundup/cgi/actions.py

@@ -1233,32 +1233,35 @@ def handle(self):
         try:
             # Implement rate limiting of logins by login name.
             # Use prefix to prevent key collisions maybe??
-            rlkey="LOGIN-" + self.client.user
-            limit=self.loginLimit
-            gcra=Gcra()
-            otk=self.client.db.Otk
-            try:
-                val=otk.getall(rlkey)
-                gcra.set_tat_as_string(rlkey, val['tat'])
-            except KeyError:
-                # ignore if tat not set, it's 1970-1-1 by default.
-                pass
-            # see if rate limit exceeded and we need to reject the attempt
-            reject=gcra.update(rlkey, limit)
-
-            # Calculate a timestamp that will make OTK expire the
-            # unused entry 1 hour in the future
-            ts = time.time() - (60 * 60 * 24 * 7) + 3600
-            otk.set(rlkey, tat=gcra.get_tat_as_string(rlkey),
-                                   __timestamp=ts)
-            otk.commit()
-
-            if reject:
-                # User exceeded limits: find out how long to wait
-                status=gcra.status(rlkey, limit)
-                raise Reject(_("Logins occurring too fast. Please wait: %d seconds.")%status['Retry-After'])
-            else:
-                self.verifyLogin(self.client.user, password)
+            # set client.db.config.WEB_LOGIN_ATTEMPTS_MIN to 0
+            #  to disable
+            if self.client.db.config.WEB_LOGIN_ATTEMPTS_MIN: # if 0 - off
+                rlkey="LOGIN-" + self.client.user
+                limit=self.loginLimit
+                gcra=Gcra()
+                otk=self.client.db.Otk
+                try:
+                    val=otk.getall(rlkey)
+                    gcra.set_tat_as_string(rlkey, val['tat'])
+                except KeyError:
+                    # ignore if tat not set, it's 1970-1-1 by default.
+                    pass
+                # see if rate limit exceeded and we need to reject the attempt
+                reject=gcra.update(rlkey, limit)
+
+                # Calculate a timestamp that will make OTK expire the
+                # unused entry 1 hour in the future
+                ts = time.time() - (60 * 60 * 24 * 7) + 3600
+                otk.set(rlkey, tat=gcra.get_tat_as_string(rlkey),
+                                       __timestamp=ts)
+                otk.commit()
+
+                if reject:
+                    # User exceeded limits: find out how long to wait
+                    status=gcra.status(rlkey, limit)
+                    raise Reject(_("Logins occurring too fast. Please wait: %d seconds.")%status['Retry-After'])
+
+            self.verifyLogin(self.client.user, password)
         except exceptions.LoginError as err:
             self.client.make_user_anonymous()
             for arg in err.args:

roundup/configuration.py

@@ -474,6 +474,21 @@ def str2value(self, value):
         except ValueError:
             raise OptionValueError(self, value, "Integer number required")
 
+class IntegerNumberGeqZeroOption(Option):
+
+    """Integer numbers greater than or equal to zero."""
+
+    def str2value(self, value):
+        try:
+            v = int(value)
+            if v < 0:
+                raise OptionValueError(self, value,
+                      "Integer number greater than or equal to zero required")
+        except OptionValueError:
+            raise # pass through subclass
+        except ValueError:
+            raise OptionValueError(self, value, "Integer number required")
+
 class OctalNumberOption(Option):
 
     """Octal Integer numbers"""
@@ -775,12 +790,13 @@ def str2value(self, value):
             "variables supplied by your web server (in that order).\n"
             "Set this option to 'no' if you do not wish to use HTTP Basic\n"
             "Authentication in your web interface."),
-        (IntegerNumberOption, 'login_attempts_min', "3",
+        (IntegerNumberGeqZeroOption, 'login_attempts_min', "3",
             "Limit login attempts per user per minute to this number.\n"
             "By default the 4th login attempt in a minute will notify\n"
             "the user that they need to wait 20 seconds before trying to\n"
             "log in again. This limits password guessing attacks and\n"
-            "shouldn't need to be changed.\n"),
+            "shouldn't need to be changed. Rate limiting on login can\n"
+            "be disabled by setting the value to 0."),
         (SameSiteSettingOption, 'samesite_cookie_setting', "Lax",
             """Set the mode of the SameSite cookie option for
 the session cookie. Choices are 'Lax' or

test/test_actions.py

@@ -395,6 +395,19 @@ def testLoginRateLimit(self):
         self.assertRaisesMessage(Reject, LoginAction(self.client).handle,
                'Logins occurring too fast. Please wait: 3 seconds.')
 
+    def testLoginRateLimitOff(self):
+        ''' Set number of logins to 0 per minute. Verify that
+            we can do 1000 which for manual login might as well be off. 
+        '''
+
+        self.client.db.config.WEB_LOGIN_ATTEMPTS_MIN = 0
+
+        # Do the first login setting an invalid login name
+        self.assertLoginLeavesMessages(['Invalid login'], 'nouser')
+        for i in range(1000):
+            self.client._error_message = []
+            self.assertLoginLeavesMessages(['Invalid login'])
+
 class EditItemActionTestCase(ActionTestCase):
     def setUp(self):
         ActionTestCase.setUp(self)

test/test_config.py

@@ -68,3 +68,17 @@ def testTrackerWeb(self):
         self.assertRaises(configuration.OptionValueError,
              config._get_option('TRACKER_WEB').set, "htt://foo.example/bar")
 
+    def testLoginRateLimit(self):
+        config = configuration.CoreConfig()
+
+        self.assertEqual(None,
+                   config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set("0"))
+        self.assertEqual(None,
+                    config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set("200"))
+
+        self.assertRaises(configuration.OptionValueError,
+                   config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set, "fred")
+
+        self.assertRaises(configuration.OptionValueError,
+                   config._get_option('WEB_LOGIN_ATTEMPTS_MIN').set, "-1")
+