feat: Login anomaly detection & suspicious activity alerts#278
feat: Login anomaly detection & suspicious activity alerts#278sirrodgepodge wants to merge 1 commit intorohitdash08:mainfrom
Conversation
- Add LoginHistory and SecurityAlert models with DB migrations - Track login metadata (IP, user agent, timestamp) on every attempt - Detect anomalies: new IP, new device, unusual login hour, brute force - Create in-app security alerts with email notification support - Add GET /auth/login-history (paginated) endpoint - Add GET /auth/security-alerts endpoint - Add POST /auth/security-alerts/<id>/acknowledge endpoint - Return security_warnings in login response when anomalies detected - Add comprehensive test suite (9 tests covering all features) - Add documentation in docs/login-anomaly-detection.md - Fix Python 3.9 compatibility with __future__ annotations Closes rohitdash08#124
There was a problem hiding this comment.
Pull request overview
Adds login anomaly detection, login history tracking, and security alerting to the backend auth flow, including new authenticated endpoints to view history/alerts and acknowledge alerts.
Changes:
- Record successful/failed logins in a new
login_historytable and detect anomalies (new IP/device, unusual time, brute force). - Create in-app security alerts (and attempt email notifications) when anomalies are detected; expose warnings in the
/auth/loginresponse. - Add
/auth/login-history,/auth/security-alerts, and/auth/security-alerts/<id>/acknowledgeendpoints plus a new test suite and docs.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/backend/app/services/login_anomaly.py | New anomaly detection + alert creation/email notification logic. |
| packages/backend/app/routes/auth.py | Integrates login recording/anomaly warnings; adds login-history + security-alert endpoints. |
| packages/backend/app/models.py | Adds LoginHistory and SecurityAlert ORM models. |
| packages/backend/app/db/schema.sql | Adds new tables and initial indexes. |
| packages/backend/tests/test_login_anomaly.py | Adds tests for history endpoints, anomaly flags, and alerts/ack flow. |
| packages/backend/tests/conftest.py | Reworks test Redis handling (currently problematic). |
| docs/login-anomaly-detection.md | Adds API + schema + thresholds documentation. |
| packages/backend/app/services/{expense_import,cache,ai}.py | Adds __future__.annotations for Python 3.9 typing compatibility. |
| packages/backend/app/{observability,config,init.py} | Adds __future__.annotations for Python 3.9 typing compatibility. |
| packages/backend/app/routes/expenses.py | Adds __future__.annotations for Python 3.9 typing compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Detects suspicious login activity by analysing login history for: | ||
| - New / previously-unseen IP addresses | ||
| - New / previously-unseen user agents (device fingerprint proxy) | ||
| - Unusual login hour (outside user's typical window) |
There was a problem hiding this comment.
The module docstring says unusual login hour detection is based on the user’s "typical window", but the implementation uses a fixed UTC off-hours range. Update the docstring to reflect the actual behavior (or adjust the implementation).
| - Unusual login hour (outside user's typical window) | |
| - Unusual login hour (during off-hours in a fixed 01:00–05:00 UTC window) |
| # Use a fake redis for tests so we don't need a running Redis server | ||
| fake_redis = MagicMock() | ||
| fake_redis.get.return_value = "1" # refresh tokens always "valid" | ||
| ext.redis_client = fake_redis | ||
| # Also patch the reference imported in auth routes | ||
| from app.routes import auth as auth_mod | ||
| auth_mod.redis_client = fake_redis |
There was a problem hiding this comment.
The test Redis stub makes refresh tokens effectively never revoked (get always returns "1"), so /auth/logout can’t invalidate a refresh token and test_auth_logout_revokes_refresh_token will fail. Use an in-memory Redis implementation (e.g., fakeredis) or a small dict-backed stub that correctly implements setex/get/delete and resets state per test.
| CREATE INDEX IF NOT EXISTS idx_login_history_user ON login_history(user_id, created_at DESC); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS security_alerts ( | ||
| id SERIAL PRIMARY KEY, | ||
| user_id INT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| alert_type VARCHAR(50) NOT NULL, | ||
| message VARCHAR(500) NOT NULL, | ||
| metadata_json TEXT, | ||
| acknowledged BOOLEAN NOT NULL DEFAULT FALSE, | ||
| created_at TIMESTAMP NOT NULL DEFAULT NOW() | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS idx_security_alerts_user ON security_alerts(user_id, acknowledged, created_at DESC); |
There was a problem hiding this comment.
The login_history index only covers (user_id, created_at), but anomaly checks query on (user_id, ip_address, success), (user_id, user_agent, success), and brute-force counts query on (user_id, success, created_at). Without supporting composite indexes these will degrade to scans as the tables grow. Consider adding composite indexes aligned to those query patterns.
| import json | ||
|
|
||
|
|
There was a problem hiding this comment.
Unused import json will fail flake8 (F401). Remove it or use it in the tests.
| import json |
| send_email(user.email, "FinMind Security Alert", body) | ||
| except Exception: | ||
| logger.debug("Email notification failed for user %s", user_id) |
There was a problem hiding this comment.
send_email() returns a boolean and swallows exceptions internally (see app/services/reminders.py), so this try/except will never log failures and email delivery problems become silent. Check the return value from send_email and log (at least debug/info) when it returns False.
| send_email(user.email, "FinMind Security Alert", body) | |
| except Exception: | |
| logger.debug("Email notification failed for user %s", user_id) | |
| email_sent = send_email(user.email, "FinMind Security Alert", body) | |
| except Exception: | |
| logger.debug("Email notification failed for user %s due to exception", user_id) | |
| else: | |
| if not email_sent: | |
| logger.info( | |
| "Email notification not sent for user %s (send_email returned False)", | |
| user_id, | |
| ) |
| @bp.get("/security-alerts") | ||
| @jwt_required() | ||
| def security_alerts(): | ||
| """Return unacknowledged security alerts for the authenticated user.""" |
There was a problem hiding this comment.
The docstring says this endpoint returns unacknowledged alerts, but the query returns all alerts for the user (no acknowledged=False filter), and tests appear to rely on acknowledged alerts remaining visible. Either filter to unacknowledged alerts or update the docstring/contract to match the actual behavior.
| """Return unacknowledged security alerts for the authenticated user.""" | |
| """Return recent security alerts for the authenticated user.""" |
| import json | ||
| import logging | ||
| from datetime import datetime, timedelta | ||
| from typing import List, Optional |
There was a problem hiding this comment.
Unused import Optional will fail flake8 (F401). Remove it or use it.
| from typing import List, Optional | |
| from typing import List |
| # Best-effort email notification | ||
| _notify_user(user_id, flags, messages) | ||
|
|
There was a problem hiding this comment.
Email deduplication is bypassed because _notify_user() is called with the original flags list even when all alerts were suppressed as duplicates (existing causes continue). This can spam users with repeated emails (especially unusual_time or repeated brute-force attempts) even though no new alert rows are created. Track which alerts were actually created and only notify for those (or move notification inside the if not existing branch).
| entry = LoginHistory( | ||
| user_id=user_id, | ||
| ip_address=ip_address, | ||
| user_agent=user_agent, | ||
| success=success, | ||
| anomaly_flags=json.dumps(anomaly_flags) if anomaly_flags else None, | ||
| ) |
There was a problem hiding this comment.
ip_address and user_agent are persisted directly from request headers without length limiting/normalization. Since the DB columns are VARCHAR(45) / VARCHAR(500), a long X-Forwarded-For or UA header can trigger a DB error and fail login. Normalize/truncate before constructing LoginHistory (and similarly for failed logins).
| | Anomaly | Flag | Description | | ||
| |---------|------|-------------| | ||
| | New IP Address | `new_ip` | Login from an IP not previously seen for this user | | ||
| | New Device | `new_device` | Login from a user agent not previously seen | | ||
| | Unusual Time | `unusual_time` | Login during UTC 01:00–05:00 | | ||
| | Brute Force | `multiple_failed_attempts` | 5+ failed login attempts within 30 minutes | |
There was a problem hiding this comment.
The anomaly table in this markdown uses leading double pipes (||), which renders as an extra empty column in GitHub-flavored markdown. Use single leading pipes (e.g., | Anomaly | Flag | Description |) for correct table formatting.
|
Closing — no longer pursuing this bounty. |
Summary
Implements login anomaly detection and suspicious activity alerts as described in #124.
What's Included
Login History Tracking
login_historydatabase table with proper indexesAnomaly Detection Engine (
app/services/login_anomaly.py)Detects four types of suspicious activity:
Security Alerts
security_alertstablesecurity_warningsarray when anomalies are detectedNew API Endpoints
/auth/login-history/auth/security-alerts/auth/security-alerts/<id>/acknowledgeTests
9 comprehensive tests covering:
Documentation
docs/login-anomaly-detection.mdFiles Changed
packages/backend/app/models.py— LoginHistory + SecurityAlert modelspackages/backend/app/services/login_anomaly.py— Detection engine (new)packages/backend/app/routes/auth.py— Integration + new endpointspackages/backend/app/db/schema.sql— New tables and indexespackages/backend/tests/test_login_anomaly.py— Test suite (new)docs/login-anomaly-detection.md— Documentation (new)__future__annotations)Closes #124