From 9df3262d30b51f0a4f012a577834267260c2d857 Mon Sep 17 00:00:00 2001 From: gsinghpal Date: Tue, 26 May 2026 23:01:09 -0400 Subject: [PATCH] fix(fusion_login_audit): avoid duplicate row on bad-password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the login string resolves to an existing user and the password is wrong, BOTH overrides used to write a failure row: - _check_credentials wrapper: result=failure, reason=bad_password - _login wrapper (catching the propagating AccessDenied): result= failure, reason=unknown_user Discovered in production smoke on westin-v19 after the deploy: a single failed login for info@gsafinancialconsulting.com produced two audit rows (one bad_password, one unknown_user). The unknown_user label was wrong — the user IS in the system. Fix: _login now checks whether the login string resolves to any user BEFORE writing the unknown_user row. If yes, _check_credentials already logged the attempt and _login skips. If no, the user lookup in super() failed and _login is the only chance to log. Regression test test_login_known_user_bad_password_single_row asserts exactly one row per attempt and that the row carries bad_password (not unknown_user) when the user exists. 30 tests green locally; production smoke on westin-v19 confirms: one row per failed login, bad_password, IP 172.18.0.1 captured. Co-Authored-By: Claude Opus 4.7 (1M context) --- fusion_login_audit/models/res_users.py | 50 +++++++++++++------- fusion_login_audit/tests/test_login_audit.py | 38 +++++++++++++++ 2 files changed, 71 insertions(+), 17 deletions(-) diff --git a/fusion_login_audit/models/res_users.py b/fusion_login_audit/models/res_users.py index 65034bca..c776b244 100644 --- a/fusion_login_audit/models/res_users.py +++ b/fusion_login_audit/models/res_users.py @@ -288,28 +288,44 @@ class ResUsers(models.Model): """Catch the unknown-user branch of upstream _login. In Odoo 19 ``_login`` is an *instance* method (not a classmethod as in - earlier versions). Upstream raises ``AccessDenied`` when the login - string does not resolve to any user — at that point no user record - exists, so the ``bad_password`` path in ``_check_credentials`` never - fires. We catch the propagating exception here and write a row with - ``user_id=NULL`` and ``failure_reason='unknown_user'``. + earlier versions). Upstream raises ``AccessDenied`` in three cases: - ``_fc_record_login_event`` already writes through an INDEPENDENT - cursor (``self.env.registry.cursor()``), so the audit row survives - the outer transaction rollback that follows the re-raised - ``AccessDenied``. We never block the re-raise: any audit-side - exception is caught + logged inside the helper. + 1. Unknown login string — ``_assert_can_auth`` or the user-lookup + ``search()`` returns empty → ``_check_credentials`` never fires → + THIS override is the only chance to record the attempt. + 2. Wrong password — user exists, ``_check_credentials`` raises → + our ``_check_credentials`` override already wrote a ``bad_password`` + row → re-raise propagates up to here. We MUST NOT write a second + row. + 3. 2FA failure — same as #2 but ``failure_reason='2fa_failed'``. + + We distinguish #1 from #2/#3 by checking whether the login string + resolves to any user. If it does, ``_check_credentials`` ran (and + already logged); if it doesn't, the user lookup failed and we log + ``unknown_user`` here. + + ``_fc_record_login_event`` writes through an INDEPENDENT cursor + (``self.env.registry.cursor()``), so the audit row survives the + outer transaction rollback that follows the re-raised + ``AccessDenied``. Audit-side exceptions never block the re-raise. """ try: return super()._login(credential, user_agent_env) except AccessDenied: - self._fc_record_login_event( - result='failure', - failure_reason='unknown_user', - user_id=False, - attempted_login=(credential or {}).get('login') or 'unknown', - _credential=credential, - ) + login = (credential or {}).get('login') or '' + try: + user_exists = bool(self.sudo().search( + [('login', '=', login)], limit=1)) + except Exception: + user_exists = False # be permissive — log the row anyway + if not user_exists: + self._fc_record_login_event( + result='failure', + failure_reason='unknown_user', + user_id=False, + attempted_login=login or 'unknown', + _credential=credential, + ) raise # ────────────────────────────────────────────────────────────────── diff --git a/fusion_login_audit/tests/test_login_audit.py b/fusion_login_audit/tests/test_login_audit.py index d7767c77..e6926e64 100644 --- a/fusion_login_audit/tests/test_login_audit.py +++ b/fusion_login_audit/tests/test_login_audit.py @@ -237,6 +237,44 @@ class TestFusionLoginAuditModel(TransactionCase): self.assertEqual(row.failure_reason, 'unknown_user') self.assertEqual(row.result, 'failure') + def test_login_known_user_bad_password_single_row(self): + """When _login is the entry point for an existing user with the + wrong password, only ONE failure row is written (bad_password from + _check_credentials) — NOT two (bad_password + unknown_user). The + unknown_user branch must only fire when the login string does not + resolve to any user. + + Regression test for the duplicate-row bug discovered during the + production deploy smoke on westin-v19: a single failed login for + an existing user was creating two audit rows. + """ + from odoo.exceptions import AccessDenied + user = self.env['res.users'].sudo().create({ + 'name': 'NoDupTester', + 'login': 'nodup-tester@example.com', + 'password': 'nodup-tester-pw-1', + }) + Audit = self.env['fusion.login.audit'].sudo() + before = Audit.search_count([('attempted_login', '=', user.login)]) + raised = False + try: + self.env['res.users']._login( + {'login': user.login, 'password': 'wrong-not-the-real-one', + 'type': 'password'}, + {'interactive': False}, + ) + except AccessDenied: + raised = True + self.assertTrue(raised) + after = Audit.search_count([('attempted_login', '=', user.login)]) + self.assertEqual(after - before, 1, + "Exactly one row per failed login attempt — not two") + row = Audit.search([('attempted_login', '=', user.login)], + order='event_time desc', limit=1) + self.assertEqual(row.failure_reason, 'bad_password', + "Existing-user failure must record bad_password, " + "not unknown_user (the user IS in the system)") + def test_computed_last_successful_login(self): """x_fc_last_successful_login reflects the latest success row.""" user = self.env['res.users'].sudo().create({