From 243d4f95cf5fd62f5c6d364e963da8ea1df376e2 Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Tue, 4 Aug 2026 13:48:12 +0200 Subject: [PATCH] Count lockout failures over a rolling window via LoginHistory - failed_login_attempts counter replaced by LoginHistory.count_recent_failures() over a configurable LOCKOUT_WINDOW, so old failures age out instead of persisting indefinitely until a successful login - LoginHistory now records every login attempt, successful and failed (including from the DB-sync client), instead of only successes - index migration split out and built CONCURRENTLY so it doesn't hold a write-blocking lock on login_history for the duration of the build - login form shows a dedicated message on AccountLocked instead of the raw backend error text Co-Authored-By: Claude Sonnet 5 --- server/mergin/auth/app.py | 3 - server/mergin/auth/config.py | 2 + server/mergin/auth/models.py | 61 +++++-- server/mergin/tests/test_auth.py | 153 ++++++++++++++---- ...70b252_login_history_index_concurrently.py | 41 +++++ .../a3c8f2e1d947_add_login_lockout_fields.py | 20 +-- .../packages/lib/src/modules/user/store.ts | 13 +- .../packages/lib/src/modules/user/types.ts | 2 + 8 files changed, 233 insertions(+), 62 deletions(-) create mode 100644 server/migrations/community/7a095270b252_login_history_index_concurrently.py diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 34c7b099..a8217e97 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -111,9 +111,6 @@ def authenticate(login, password): needs_commit = True if user.check_password(password): - if user.failed_login_attempts or user.locked_until: - user.reset_lockout() - needs_commit = True if user.needs_rehash(): user.assign_password(password) needs_commit = True diff --git a/server/mergin/auth/config.py b/server/mergin/auth/config.py index 0e9c81ca..24cbc50f 100644 --- a/server/mergin/auth/config.py +++ b/server/mergin/auth/config.py @@ -17,3 +17,5 @@ class Configuration(object): BCRYPT_LOG_ROUNDS = config("BCRYPT_LOG_ROUNDS", default=12, cast=int) # Comma-separated "attempts:seconds" pairs, e.g. "5:300,10:3600" LOCKOUT_POLICY = config("LOCKOUT_POLICY", default="5:300,10:3600") + # trailing window in seconds over which failed login attempts are counted + LOCKOUT_WINDOW = config("LOCKOUT_WINDOW", default=3600, cast=int) diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index e3f2ae90..1d749da7 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -42,9 +42,6 @@ class User(db.Model): default=datetime.datetime.utcnow, ) last_signed_in = db.Column(db.DateTime(), nullable=True) - failed_login_attempts = db.Column( - db.Integer, default=0, nullable=False, server_default="0" - ) locked_until = db.Column(db.DateTime(), nullable=True) receive_notifications = db.Column( db.Boolean, default=True, nullable=False, index=True @@ -103,18 +100,23 @@ def is_locked_out(self) -> bool: return self.locked_until > datetime.datetime.utcnow() def record_failed_login(self) -> Optional[int]: - """Increment the failed-login counter and apply a lockout if a threshold is crossed. + """Record a failed login attempt and apply a lockout if a threshold is crossed, + counting only failed attempts within the trailing LOCKOUT_WINDOW. Returns the lockout duration in seconds if a new lock was just applied, else None. """ - self.failed_login_attempts = (self.failed_login_attempts or 0) + 1 + LoginHistory.add_record(self.id, request, successful=False) + window = current_app.config.get("LOCKOUT_WINDOW", 3600) + since = datetime.datetime.utcnow() - datetime.timedelta(seconds=window) + recent_failures = LoginHistory.count_recent_failures(self.id, since) + policy = _parse_lockout_policy( current_app.config.get("LOCKOUT_POLICY", "5:300,10:3600") ) # find the highest applicable tier duration = None for threshold, seconds in policy: - if self.failed_login_attempts >= threshold: + if recent_failures >= threshold: duration = seconds if duration is not None: self.locked_until = datetime.datetime.utcnow() + datetime.timedelta( @@ -124,7 +126,6 @@ def record_failed_login(self) -> Optional[int]: def reset_lockout(self) -> None: """Clear lockout state after a successful login.""" - self.failed_login_attempts = 0 self.locked_until = None @property @@ -332,29 +333,54 @@ class LoginHistory(db.Model): ip_address = db.Column(db.String, index=True) ip_geolocation_country = db.Column(db.String, index=True) device_id = db.Column(db.String, index=True, nullable=True) + successful = db.Column(db.Boolean, nullable=False, server_default="true") + + __table_args__ = ( + db.Index( + "ix_login_history_user_id_successful_timestamp", + "user_id", + "successful", + "timestamp", + ), + ) - def __init__(self, user_id: int, ua: str, ip: str, device_id: Optional[str] = None): + def __init__( + self, + user_id: int, + ua: str, + ip: str, + device_id: Optional[str] = None, + successful: bool = True, + ): self.user_id = user_id self.user_agent = ua self.ip_address = ip self.device_id = device_id + self.successful = successful self.timestamp = datetime.datetime.now(tz=datetime.timezone.utc) @staticmethod - def add_record(user_id: int, req: request) -> None: + def add_record(user_id: int, req: request, successful: bool = True) -> None: ua = get_user_agent(req) ip = get_ip(req) device_id = get_device_id(req) - # ignore login attempts coming from urllib - related to db sync tool - if "DB-sync" in ua: - return - lh = LoginHistory(user_id, ua, ip, device_id) + lh = LoginHistory(user_id, ua, ip, device_id, successful=successful) db.session.add(lh) - # cache user last login - User.query.filter_by(id=user_id).update({"last_signed_in": lh.timestamp}) + if successful: + # cache user last login + User.query.filter_by(id=user_id).update({"last_signed_in": lh.timestamp}) db.session.commit() + @staticmethod + def count_recent_failures(user_id: int, since: datetime.datetime) -> int: + """Count failed login attempts for a user since the given timestamp.""" + return LoginHistory.query.filter( + LoginHistory.user_id == user_id, + LoginHistory.successful.is_(False), + LoginHistory.timestamp >= since, + ).count() + @staticmethod def get_users_last_signed_in(user_ids: list) -> dict: """Get users last signed in dates. @@ -365,7 +391,10 @@ def get_users_last_signed_in(user_ids: list) -> dict: LoginHistory.user_id, func.max(LoginHistory.timestamp).label("last_signed_in"), ) - .filter(LoginHistory.user_id.in_(user_ids)) + .filter( + LoginHistory.user_id.in_(user_ids), + LoginHistory.successful.is_(True), + ) .group_by(LoginHistory.user_id) .all() ) diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index a4220eea..601060e3 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -101,13 +101,16 @@ def test_logout(client): @patch("mergin.celery.send_email_async.apply_async") def test_login_lockout(send_email_mock, client): - """Test account lockout: progressive tiers, freeze during lock, reset on success. + """Test account lockout: progressive tiers, freeze during lock. - policy: 3 failures → 60s lock, 4 failures → 3600s lock - counter is never reset between lockouts, so tier-2 is reached after one - extra failure following the first expired tier-1 lock + policy: 3 failures → 60s lock, 4 failures → 3600s lock, counted over a + trailing window (LOCKOUT_WINDOW, default 1h) via LoginHistory. A + successful login does not clear the window - only elapsed time does, so + tier-2 is reached after one extra failure following the first expired + tier-1 lock. """ user = add_user("lockoutuser", "correctpassword") + since = datetime.utcnow() - timedelta(hours=1) def assert_locked(): resp = client.post( @@ -138,15 +141,15 @@ def assert_locked(): ) assert resp.status_code == 423 - # counter stays frozen during lockout - assert user.failed_login_attempts == 3 + # no new failures recorded while already locked out + assert LoginHistory.count_recent_failures(user.id, since) == 3 assert user.locked_until is not None # no further emails while already locked out (attempts above were all 423s) assert send_email_mock.call_count == 1 # tier 2 escalation: one more failure after tier-1 expiry - # counter was at 3; one new failure pushes it to 4, crossing tier-2 threshold + # window count was at 3; one new failure pushes it to 4, crossing tier-2 threshold # expire_lock user.locked_until = datetime.utcnow() - timedelta(seconds=1) @@ -160,12 +163,15 @@ def assert_locked(): assert resp.status_code == 401 assert_locked() assert user.locked_until > datetime.utcnow() + timedelta(seconds=60) - assert user.failed_login_attempts == 4 + assert LoginHistory.count_recent_failures(user.id, since) == 4 # second lockout email dispatched for the tier-2 re-lock assert send_email_mock.call_count == 2 - # successful login after expiry resets everything + # successful login after expiry unlocks the account, but does not + # clear the failure window - failures are only cleared by the + # passage of time, so the 4 prior failures still count until they + # individually age out of the window user.locked_until = datetime.utcnow() - timedelta(seconds=1) db.session.commit() resp = client.post( @@ -173,8 +179,8 @@ def assert_locked(): json={"login": "lockoutuser", "password": "correctpassword"}, ) assert resp.status_code == 200 - assert user.failed_login_attempts == 0 assert user.locked_until is None + assert LoginHistory.count_recent_failures(user.id, since) == 4 # no email on successful login assert send_email_mock.call_count == 2 @@ -183,19 +189,23 @@ def assert_locked(): @patch("mergin.celery.send_email_async.apply_async") def test_unlock_account(send_email_mock, client, app): """Test the self-service unlock-account link: valid use, reuse, natural - expiry, and cross-tier reuse, per the token-binding design.""" - user = add_user("unlockuser", "correctpassword") + expiry, and cross-tier reuse, per the token-binding design. + + Each scenario below uses its own user so failure counts (sourced from + LoginHistory over a trailing window) don't bleed between scenarios + within the same test. + """ def unlock(token): return client.post( url_for("/.mergin_auth_controller_unlock_account", token=token) ) - def lock_out(): + def lock_out(username): for _ in range(3): client.post( url_for("/.mergin_auth_controller_login"), - json={"login": "unlockuser", "password": "wrong"}, + json={"login": username, "password": "wrong"}, ) with patch.dict(client.application.config, {"LOCKOUT_POLICY": "3:60,4:3600"}): @@ -209,14 +219,14 @@ def lock_out(): assert resp.status_code == 400 # trigger tier-1 lock and capture its token - lock_out() + user = add_user("unlockuser", "correctpassword") + lock_out("unlockuser") assert user.is_locked_out() tier1_token = generate_unlock_token(app, user) # valid token unlocks successfully resp = unlock(tier1_token) assert resp.status_code == 200 - assert user.failed_login_attempts == 0 assert user.locked_until is None # reuse of the same (now-consumed) token fails @@ -225,34 +235,111 @@ def lock_out(): # naturally-expired lock: token itself still cryptographically valid, # but the lock episode it points to is no longer active - lock_out() - assert user.is_locked_out() - stale_token = generate_unlock_token(app, user) - user.locked_until = datetime.utcnow() - timedelta(seconds=1) + stale_user = add_user("staleuser", "correctpassword") + lock_out("staleuser") + assert stale_user.is_locked_out() + stale_token = generate_unlock_token(app, stale_user) + stale_user.locked_until = datetime.utcnow() - timedelta(seconds=1) db.session.commit() resp = unlock(stale_token) assert resp.status_code == 400 # cross-tier reuse: a token minted for one lock episode must not unlock # a later, different lock episode for the same user - user.locked_until = None - user.failed_login_attempts = 0 - db.session.commit() - lock_out() - tier1_token_2 = generate_unlock_token(app, user) + cross_user = add_user("crossuser", "correctpassword") + lock_out("crossuser") + tier1_token_2 = generate_unlock_token(app, cross_user) # escalate to tier 2 with a new locked_until - user.locked_until = datetime.utcnow() - timedelta(seconds=1) + cross_user.locked_until = datetime.utcnow() - timedelta(seconds=1) db.session.commit() client.post( url_for("/.mergin_auth_controller_login"), - json={"login": "unlockuser", "password": "wrong"}, + json={"login": "crossuser", "password": "wrong"}, ) - assert user.failed_login_attempts == 4 - assert user.locked_until > datetime.utcnow() + timedelta(seconds=60) + assert cross_user.locked_until > datetime.utcnow() + timedelta(seconds=60) resp = unlock(tier1_token_2) assert resp.status_code == 400 +def test_login_lockout_window_expiry(client): + """Failures older than LOCKOUT_WINDOW no longer count toward the threshold.""" + user = add_user("windowuser", "correctpassword") + + with patch.dict( + client.application.config, + {"LOCKOUT_POLICY": "3:60,4:3600", "LOCKOUT_WINDOW": 3600}, + ): + for _ in range(3): + resp = client.post( + url_for("/.mergin_auth_controller_login"), + json={"login": "windowuser", "password": "wrong"}, + ) + assert resp.status_code == 401 + assert user.is_locked_out() + + # push all recorded failures outside the 1h window, and let the lock expire + LoginHistory.query.filter_by(user_id=user.id, successful=False).update( + {"timestamp": datetime.utcnow() - timedelta(hours=2)} + ) + user.locked_until = datetime.utcnow() - timedelta(seconds=1) + db.session.commit() + + # old failures no longer count - a single new failure should not relock + resp = client.post( + url_for("/.mergin_auth_controller_login"), + json={"login": "windowuser", "password": "wrong"}, + ) + assert resp.status_code == 401 + assert not user.is_locked_out() + assert user.locked_until is None + assert ( + LoginHistory.count_recent_failures( + user.id, datetime.utcnow() - timedelta(hours=1) + ) + == 1 + ) + + +def test_login_history_records_failures(client): + """Failed attempts are recorded (successful=False) without touching + last_signed_in; successful logins are recorded as successful=True and + do update last_signed_in.""" + user = add_user("historyuser", "correctpassword") + assert user.last_signed_in is None + + resp = client.post( + url_for("/.mergin_auth_controller_login"), + json={"login": "historyuser", "password": "wrong"}, + ) + assert resp.status_code == 401 + failed = LoginHistory.query.filter_by(user_id=user.id, successful=False).all() + assert len(failed) == 1 + assert user.last_signed_in is None + + resp = client.post( + url_for("/.mergin_auth_controller_login"), + json={"login": "historyuser", "password": "correctpassword"}, + ) + assert resp.status_code == 200 + successful = LoginHistory.query.filter_by(user_id=user.id, successful=True).all() + assert len(successful) == 1 + assert user.last_signed_in is not None + last_signed_in = user.last_signed_in + + # a later failed attempt must not be reported/cached as the last signed-in + # time, even though it's the most recent row for this user overall + resp = client.post( + url_for("/.mergin_auth_controller_login"), + json={"login": "historyuser", "password": "wrong"}, + ) + assert resp.status_code == 401 + user.last_signed_in = None + db.session.commit() + users_last_signed_in = LoginHistory.get_users_last_signed_in([user.id]) + assert users_last_signed_in[user.id] == last_signed_in + assert user.last_signed_in == last_signed_in + + def test_bcrypt_lazy_rehash(app): """Password is transparently rehashed on login when the cost factor changes.""" import bcrypt @@ -591,6 +678,8 @@ def test_api_login(client, data, headers, expected): def test_api_login_from_urllib(client): + """DB-sync logins are recorded in LoginHistory just like any other client, + to keep a full picture of login activity (including for lockout purposes).""" with patch("mergin.auth.models.get_user_agent") as mock: mock.return_value = "DB-sync/0.1" resp = client.post( @@ -605,9 +694,9 @@ def test_api_login_from_urllib(client): .order_by(desc(LoginHistory.timestamp)) .first() ) - assert not login_history - # we do not have recored last login yet - assert user.last_signed_in is None + assert login_history + assert login_history.successful + assert user.last_signed_in == login_history.timestamp def test_api_user_profile(client): diff --git a/server/migrations/community/7a095270b252_login_history_index_concurrently.py b/server/migrations/community/7a095270b252_login_history_index_concurrently.py new file mode 100644 index 00000000..38b6e35f --- /dev/null +++ b/server/migrations/community/7a095270b252_login_history_index_concurrently.py @@ -0,0 +1,41 @@ +"""Create login_history user/successful/timestamp index concurrently + +CONCURRENTLY avoids blocking writes to login_history during the build +(every login attempt writes here), at the cost of running outside the +migration's transaction (autocommit_block) and leaving an INVALID index +behind on failure - upgrade()/downgrade() self-heal by dropping that first. + +Revision ID: 7a095270b252 +Revises: a3c8f2e1d947 +Create Date: 2026-07-24 00:00:00.000000 + +""" + +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "7a095270b252" +down_revision = "a3c8f2e1d947" +branch_labels = None +depends_on = None + + +def upgrade(): + with op.get_context().autocommit_block(): + op.execute( + "DROP INDEX CONCURRENTLY IF EXISTS ix_login_history_user_id_successful_timestamp" + ) + op.create_index( + "ix_login_history_user_id_successful_timestamp", + "login_history", + ["user_id", "successful", "timestamp"], + postgresql_concurrently=True, + ) + + +def downgrade(): + with op.get_context().autocommit_block(): + op.execute( + "DROP INDEX CONCURRENTLY IF EXISTS ix_login_history_user_id_successful_timestamp" + ) diff --git a/server/migrations/community/a3c8f2e1d947_add_login_lockout_fields.py b/server/migrations/community/a3c8f2e1d947_add_login_lockout_fields.py index bcd7f76a..c754085d 100644 --- a/server/migrations/community/a3c8f2e1d947_add_login_lockout_fields.py +++ b/server/migrations/community/a3c8f2e1d947_add_login_lockout_fields.py @@ -1,4 +1,4 @@ -"""Add failed_login_attempts and locked_until to user table +"""Add locked_until to user table and successful flag to login_history Revision ID: a3c8f2e1d947 Revises: f1d9e4a7b823 @@ -21,22 +21,22 @@ def upgrade(): op.add_column( "user", sa.Column( - "failed_login_attempts", - sa.Integer(), - nullable=False, - server_default="0", + "locked_until", + sa.DateTime(), + nullable=True, ), ) op.add_column( - "user", + "login_history", sa.Column( - "locked_until", - sa.DateTime(), - nullable=True, + "successful", + sa.Boolean(), + nullable=False, + server_default="true", ), ) def downgrade(): + op.drop_column("login_history", "successful") op.drop_column("user", "locked_until") - op.drop_column("user", "failed_login_attempts") diff --git a/web-app/packages/lib/src/modules/user/store.ts b/web-app/packages/lib/src/modules/user/store.ts index 34da4d98..af8781ce 100644 --- a/web-app/packages/lib/src/modules/user/store.ts +++ b/web-app/packages/lib/src/modules/user/store.ts @@ -2,6 +2,7 @@ // // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial +import { AxiosError } from 'axios' import { defineStore, getActivePinia } from 'pinia' import { isNavigationFailure } from 'vue-router' @@ -18,6 +19,7 @@ import { ResetPasswordPayload, ChangePasswordWithTokenPayload, ChangePasswordPayload, + ErrorCodes, IsWorkspaceAdminPayload, LoginPayload, UserDetailResponse, @@ -238,11 +240,20 @@ export const useUserStore = defineStore('userModule', { async userLogin(payload: LoginPayload) { const instanceStore = useInstanceStore() const formStore = useFormStore() + const notificationStore = useNotificationStore() try { await UserApi.login(payload.data) await instanceStore.initApp() - } catch (error) { + } catch (err) { + const error = err as AxiosError + const code = error?.response?.data?.code as ErrorCodes + if (code === 'AccountLocked') { + await notificationStore.error({ + text: 'Your account is temporarily locked due to too many failed login attempts. Please check your email for a link to unlock it.' + }) + return + } await formStore.handleError({ componentId: payload.componentId, error, diff --git a/web-app/packages/lib/src/modules/user/types.ts b/web-app/packages/lib/src/modules/user/types.ts index 2bf668c6..c7c03609 100644 --- a/web-app/packages/lib/src/modules/user/types.ts +++ b/web-app/packages/lib/src/modules/user/types.ts @@ -152,4 +152,6 @@ export interface UserRouteParams { reset?: string } +export type ErrorCodes = 'AccountLocked' + /* eslint-enable camelcase */