[REF] auth_brute_force: Cover all auth entrypoints (OCA/server-tools#…

…1219)

To fix OCA/server-tools#1125 I needed to refactor the addon. To whitelist IPs now you use a config parameter, which renders res.banned.remote model unneeded.

The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied.

auth_brute_force/migrations/10.0.2.0.0/pre-migrate.py

@@ -0,0 +1,50 @@
+# -*- coding: utf-8 -*-
+# Copyright 2018 Tecnativa - Jairo Llopis
+# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).
+
+from psycopg2 import IntegrityError
+
+
+def migrate(cr, version):
+    # Fix typo across DB
+    cr.execute(
+        """ UPDATE res_authentication_attempt
+            SET result = 'successful'
+            WHERE result = 'successfull'""",
+    )
+    # Store whitelist IPs in new format
+    cr.execute(
+        """ SELECT remote
+            FROM res_banned_remote
+            WHERE active IS FALSE""",
+    )
+    remotes = {record[0] for record in cr.fetchall()}
+    try:
+        with cr.savepoint():
+            cr.execute(
+                "INSERT INTO ir_config_parameter (key, value) VALUES (%s, %s)",
+                (
+                    "auth_brute_force.whitelist_remotes",
+                    ",".join(remotes),
+                ),
+            )
+    except IntegrityError:
+        # Parameter already exists
+        cr.execute(
+            "SELECT value FROM ir_config_parameter WHERE key = %s",
+            ("auth_brute_force.whitelist_remotes",)
+        )
+        current = set(cr.fetchall()[0][0].split(","))
+        cr.execute(
+            "UPDATE ir_config_parameter SET value = %s WHERE key = %s",
+            (",".join(current | remotes),
+             "auth_brute_force.whitelist_remotes"),
+        )
+    # Update the configured IP limit parameter
+    cr.execute(
+        "UPDATE ir_config_parameter SET key = %s WHERE key = %s",
+        (
+            "auth_brute_force.whitelist_remotes",
+            "auth_brute_force.max_by_ip",
+        )
+    )

auth_brute_force/models/res_users.py

@@ -3,11 +3,13 @@
 
 import logging
 from contextlib import contextmanager
-from threading import current_thread
 
 from odoo import SUPERUSER_ID, api, models
 from odoo.exceptions import AccessDenied
-from odoo.service import wsgi_server
+from odoo.http import request
+
+# from threading import current_thread
+
 
 _logger = logging.getLogger(__name__)
 
@@ -17,33 +19,34 @@ class ResUsers(models.Model):
 
     # HACK https://github.com/odoo/odoo/issues/24183
     # TODO Remove in v12, and use normal odoo.http.request to get details
-    def _register_hook(self):
-        """🐒-patch XML-RPC controller to know remote address."""
-        original_fn = wsgi_server.application_unproxied
-
-        def _patch(environ, start_response):
-            current_thread().environ = environ
-            return original_fn(environ, start_response)
-
-        wsgi_server.application_unproxied = _patch
+    # @api.model_cr
+    # def _register_hook(self):
+    #     """🐒-patch XML-RPC controller to know remote address."""
+    #     original_fn = wsgi_server.application_unproxied
+    #
+    #     def _patch(environ, start_response):
+    #         current_thread().environ = environ
+    #         return original_fn(environ, start_response)
+    #
+    #     wsgi_server.application_unproxied = _patch
 
     # Helpers to track authentication attempts
     @classmethod
     @contextmanager
     def _auth_attempt(cls, login):
         """Start an authentication attempt and track its state."""
+        cls.environ = request.httprequest.environ
         try:
             # Check if this call is nested
-            attempt_id = current_thread().auth_attempt_id
-        except AttributeError:
+            attempt_id = cls.environ["auth_attempt_id"]
+        except KeyError:
             # Not nested; create a new attempt
             attempt_id = cls._auth_attempt_new(login)
         if not attempt_id:
             # No attempt was created, so there's nothing to do here
             yield
-            return
         try:
-            current_thread().auth_attempt_id = attempt_id
+            cls.environ["auth_attempt_id"] = attempt_id
             result = "successful"
             try:
                 yield
@@ -54,28 +57,28 @@ def _auth_attempt(cls, login):
                 cls._auth_attempt_update({"result": result})
         finally:
             try:
-                del current_thread().auth_attempt_id
-            except AttributeError:
-                _logger.warning("AttributeError: It was deleted already")
-                pass  # It was deleted already
+                del cls.environ["auth_attempt_id"]
+            except KeyError:
+                _logger.info(
+                    "KeyError: auth_attempt_id was deleted already"
+                )  # It was deleted already
 
     @classmethod
     def _auth_attempt_force_raise(cls, login, method):
         """Force a method to raise an AccessDenied on falsey return."""
         with cls._auth_attempt(login):
             result = method()
-            if not result:
-                raise AccessDenied()
+            # TODO: Not in use right now,
+            # TODO: So, it is more likely to remove these 2 lines of code.
+            # if not result:
+            #     raise AccessDenied()
         return result
 
     @classmethod
     def _auth_attempt_new(cls, login):
         """Store one authentication attempt, not knowing the result."""
         # Get the right remote address
-        try:
-            remote_addr = current_thread().environ["REMOTE_ADDR"]
-        except (KeyError, AttributeError):
-            remote_addr = False
+        remote_addr = cls.environ.get("REMOTE_ADDR", False)
         # Exit if it doesn't make sense to store this attempt
         if not remote_addr:
             return False
@@ -93,7 +96,7 @@ def _auth_attempt_new(cls, login):
     @classmethod
     def _auth_attempt_update(cls, values):
         """Update a given auth attempt if we still ignore its result."""
-        auth_id = getattr(current_thread(), "auth_attempt_id", False)
+        auth_id = cls.environ.get("auth_attempt_id", False)
         if not auth_id:
             return {}  # No running auth attempt; nothing to do
         # Use a separate cursor to keep changes always

auth_brute_force/security/ir_model_access.yml

@@ -4,11 +4,6 @@
     name: Authentication Attempt All Users
     perm_read: true
 
-- !record {model: ir.model.access, id: access_res_banned_remote_all}:
-    model_id: model_res_banned_remote
-    name: Banned Remote All Users
-    perm_read: true
-
 - !record {model: ir.model.access, id: access_res_authentication_attempt_manager}:
     group_id: base.group_system
     model_id: model_res_authentication_attempt
@@ -17,12 +12,3 @@
     perm_read: true
     perm_write: true
     perm_unlink: true
-
-- !record {model: ir.model.access, id: access_res_banned_remote_manager}:
-    group_id: base.group_system
-    model_id: model_res_banned_remote
-    name: Banned Remote Manager
-    perm_create: true
-    perm_read: true
-    perm_write: true
-    perm_unlink: true

auth_brute_force/tests/__init__.py

@@ -1 +1,2 @@
 from . import test_brute_force
+from . import test_ip_errors