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({