fix(fusion_login_audit): avoid duplicate row on bad-password
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) <noreply@anthropic.com>
This commit is contained in:
@@ -288,28 +288,44 @@ class ResUsers(models.Model):
|
|||||||
"""Catch the unknown-user branch of upstream _login.
|
"""Catch the unknown-user branch of upstream _login.
|
||||||
|
|
||||||
In Odoo 19 ``_login`` is an *instance* method (not a classmethod as in
|
In Odoo 19 ``_login`` is an *instance* method (not a classmethod as in
|
||||||
earlier versions). Upstream raises ``AccessDenied`` when the login
|
earlier versions). Upstream raises ``AccessDenied`` in three cases:
|
||||||
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'``.
|
|
||||||
|
|
||||||
``_fc_record_login_event`` already writes through an INDEPENDENT
|
1. Unknown login string — ``_assert_can_auth`` or the user-lookup
|
||||||
cursor (``self.env.registry.cursor()``), so the audit row survives
|
``search()`` returns empty → ``_check_credentials`` never fires →
|
||||||
the outer transaction rollback that follows the re-raised
|
THIS override is the only chance to record the attempt.
|
||||||
``AccessDenied``. We never block the re-raise: any audit-side
|
2. Wrong password — user exists, ``_check_credentials`` raises →
|
||||||
exception is caught + logged inside the helper.
|
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:
|
try:
|
||||||
return super()._login(credential, user_agent_env)
|
return super()._login(credential, user_agent_env)
|
||||||
except AccessDenied:
|
except AccessDenied:
|
||||||
self._fc_record_login_event(
|
login = (credential or {}).get('login') or ''
|
||||||
result='failure',
|
try:
|
||||||
failure_reason='unknown_user',
|
user_exists = bool(self.sudo().search(
|
||||||
user_id=False,
|
[('login', '=', login)], limit=1))
|
||||||
attempted_login=(credential or {}).get('login') or 'unknown',
|
except Exception:
|
||||||
_credential=credential,
|
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
|
raise
|
||||||
|
|
||||||
# ──────────────────────────────────────────────────────────────────
|
# ──────────────────────────────────────────────────────────────────
|
||||||
|
|||||||
@@ -237,6 +237,44 @@ class TestFusionLoginAuditModel(TransactionCase):
|
|||||||
self.assertEqual(row.failure_reason, 'unknown_user')
|
self.assertEqual(row.failure_reason, 'unknown_user')
|
||||||
self.assertEqual(row.result, 'failure')
|
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):
|
def test_computed_last_successful_login(self):
|
||||||
"""x_fc_last_successful_login reflects the latest success row."""
|
"""x_fc_last_successful_login reflects the latest success row."""
|
||||||
user = self.env['res.users'].sudo().create({
|
user = self.env['res.users'].sudo().create({
|
||||||
|
|||||||
Reference in New Issue
Block a user