mirror of
https://github.com/langflow-ai/langflow.git
synced 2026-07-25 10:32:22 +08:00
fix: add failed login logging with IP tracking and other relevant details (#13409)
* feat(security): add failed login logging with IP tracking and comprehensive tests * fix: remove PII from login logs and fix structlog format * docs: agent improvements (#13269) * docs: add structured output for agents * docs: add structured output note * docs: clarify both agent outputs and add release notes * docs: when langflow sends events to the playground and chat history * fix(playground): expand session checkbox click target to full row height (#13349) * fix(playground): expand session checkbox click target to full row height The selectable-row checkbox wrapper was ``w-4 h-4`` (16x16 px) but the row is ``h-8`` (32 px). The 8 px band above and the 8 px band below the visible icon were dead zones that bubbled to the row's ``toggleVisibility`` and navigated to the session instead of toggling selection — exactly the accidental-navigation UX the bug ticket describes. Grow the wrapper to ``w-4 h-8`` so the click target captures the full row height. Width stays narrow so the text alignment is unchanged; the icon itself stays ``h-4 w-4``, centered via the existing ``flex items-center justify-center``. Visual position is identical. Adds ``__tests__/session-selector-checkbox-click-target.test.tsx`` (3 cases): wrapper className guard (h-8 + w-4, not h-4), click on the wrapper outside the icon triggers selection without bubbling to toggleVisibility, and click on the inner icon still toggles selection. * test(playground): drop checkbox-click-target test file (not needed per review) * [autofix.ci] apply automated fixes * test: align session-selector checkbox assertions with full-height click target --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * fix: improve trace search functionality (#13383) * improve trace search functionality * [autofix.ci] apply automated fixes --------- Co-authored-by: Olayinka Adelakun <olayinkaadelakun@Olayinkas-MacBook-Pro.local> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * test: fix test_login_logs_real_output_format to use mocking - Updated test to use patch() instead of capsys for consistency - Verifies structlog kwargs format (not nested extra dict) - All 7 login logging tests now passing --------- Co-authored-by: Janardan S Kavia <janardanskavia@Janardans-MacBook-Pro.local> Co-authored-by: Mendon Kissling <59585235+mendonk@users.noreply.github.com> Co-authored-by: keval shah <kevalvirat@gmail.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: olayinkaadelakun <olayinka.adelakun@ibm.com> Co-authored-by: Olayinka Adelakun <olayinkaadelakun@Olayinkas-MacBook-Pro.local>
This commit is contained in:
committed by
GitHub
parent
42ad7ebf80
commit
3f445d1532
@ -26,6 +26,7 @@ class SessionResponse(BaseModel):
|
||||
|
||||
@router.post("/login", response_model=Token, include_in_schema=False)
|
||||
async def login_to_get_access_token(
|
||||
request: Request,
|
||||
response: Response,
|
||||
form_data: Annotated[OAuth2PasswordRequestForm, Depends()],
|
||||
db: DbSession,
|
||||
@ -33,7 +34,7 @@ async def login_to_get_access_token(
|
||||
auth_settings = get_settings_service().auth_settings
|
||||
try:
|
||||
auth = get_auth_service()
|
||||
user = await auth.authenticate_user(form_data.username, form_data.password, db)
|
||||
user = await auth.authenticate_user(form_data.username, form_data.password, db, request)
|
||||
except Exception as exc:
|
||||
if isinstance(exc, HTTPException):
|
||||
raise
|
||||
|
||||
@ -1,5 +1,6 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import hashlib
|
||||
import warnings
|
||||
from collections.abc import Coroutine
|
||||
from datetime import datetime, timedelta, timezone
|
||||
@ -662,18 +663,57 @@ class AuthService(BaseAuthService):
|
||||
detail="Invalid refresh token",
|
||||
) from e
|
||||
|
||||
async def authenticate_user(self, username: str, password: str, db: AsyncSession) -> User | None:
|
||||
async def authenticate_user(
|
||||
self, username: str, password: str, db: AsyncSession, request: Request | None = None
|
||||
) -> User | None:
|
||||
user = await get_user_by_username(db, username)
|
||||
|
||||
if not user:
|
||||
if request and request.client:
|
||||
# Hash username for correlation without exposing PII
|
||||
username_hash = hashlib.sha256(username.lower().encode()).hexdigest()[:16]
|
||||
logger.warning(
|
||||
"Login failed: user not found",
|
||||
auth_event="login_failed",
|
||||
reason="user_not_found",
|
||||
username_hash=username_hash,
|
||||
client_ip=request.client.host,
|
||||
)
|
||||
return None
|
||||
|
||||
if not user.is_active:
|
||||
if request and request.client:
|
||||
logger.warning(
|
||||
"Login failed: inactive user",
|
||||
auth_event="login_failed",
|
||||
reason="user_inactive",
|
||||
auth_id=str(user.id),
|
||||
client_ip=request.client.host,
|
||||
)
|
||||
if not user.last_login_at:
|
||||
raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Waiting for approval")
|
||||
raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Inactive user")
|
||||
|
||||
return user if self.verify_password(password, user.password) else None
|
||||
if not self.verify_password(password, user.password):
|
||||
if request and request.client:
|
||||
logger.warning(
|
||||
"Login failed: incorrect password",
|
||||
auth_event="login_failed",
|
||||
reason="incorrect_password",
|
||||
auth_id=str(user.id),
|
||||
client_ip=request.client.host,
|
||||
)
|
||||
return None
|
||||
|
||||
# Successful login
|
||||
if request and request.client:
|
||||
logger.info(
|
||||
"Login successful",
|
||||
auth_event="login_success",
|
||||
auth_id=str(user.id),
|
||||
client_ip=request.client.host,
|
||||
)
|
||||
return user
|
||||
|
||||
def _get_fernet(self) -> Fernet:
|
||||
from langflow.services.auth.utils import ensure_fernet_key
|
||||
|
||||
205
src/backend/tests/unit/test_login_logging.py
Normal file
205
src/backend/tests/unit/test_login_logging.py
Normal file
@ -0,0 +1,205 @@
|
||||
"""Tests for login logging functionality."""
|
||||
|
||||
import hashlib
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
from httpx import AsyncClient
|
||||
from langflow.services.database.models.user.model import User
|
||||
from langflow.services.deps import session_scope
|
||||
from sqlmodel import select
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_failed_login_nonexistent_user_logs(client: AsyncClient):
|
||||
"""Test that failed login for non-existent user is logged with hashed username."""
|
||||
with patch("langflow.services.auth.service.logger") as mock_logger:
|
||||
response = await client.post(
|
||||
"/api/v1/login",
|
||||
data={"username": "nonexistent_user", "password": "wrong_password"}, # pragma: allowlist secret
|
||||
)
|
||||
|
||||
assert response.status_code == 401
|
||||
assert response.json()["detail"] == "Incorrect username or password"
|
||||
|
||||
# Verify warning was logged with structlog kwargs
|
||||
mock_logger.warning.assert_called_once()
|
||||
call_args = mock_logger.warning.call_args
|
||||
assert call_args[0][0] == "Login failed: user not found"
|
||||
# Structlog uses kwargs, not extra dict
|
||||
assert call_args[1]["auth_event"] == "login_failed"
|
||||
assert call_args[1]["reason"] == "user_not_found"
|
||||
# Username should be hashed, not plain text
|
||||
assert "username_hash" in call_args[1]
|
||||
assert call_args[1]["username_hash"] == hashlib.sha256(b"nonexistent_user").hexdigest()[:16]
|
||||
assert "client_ip" in call_args[1]
|
||||
# Should NOT contain plain username
|
||||
assert "username" not in call_args[1]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_failed_login_wrong_password_logs(client: AsyncClient, active_user):
|
||||
"""Test that failed login with wrong password is logged."""
|
||||
with patch("langflow.services.auth.service.logger") as mock_logger:
|
||||
response = await client.post(
|
||||
"/api/v1/login",
|
||||
data={"username": active_user.username, "password": "wrong_password"}, # pragma: allowlist secret
|
||||
)
|
||||
|
||||
assert response.status_code == 401
|
||||
assert response.json()["detail"] == "Incorrect username or password"
|
||||
|
||||
# Verify warning was logged with structlog kwargs
|
||||
mock_logger.warning.assert_called_once()
|
||||
call_args = mock_logger.warning.call_args
|
||||
assert call_args[0][0] == "Login failed: incorrect password"
|
||||
assert call_args[1]["auth_event"] == "login_failed"
|
||||
assert call_args[1]["reason"] == "incorrect_password"
|
||||
assert "auth_id" in call_args[1]
|
||||
assert "client_ip" in call_args[1]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_failed_login_inactive_user_logs(client: AsyncClient):
|
||||
"""Test that failed login for inactive user is logged."""
|
||||
# Create an inactive user with last_login_at set (so it returns 401, not 400)
|
||||
async with session_scope() as session:
|
||||
from datetime import datetime, timezone
|
||||
|
||||
from langflow.services.deps import get_auth_service
|
||||
|
||||
inactive_user = User(
|
||||
username="inactiveuser",
|
||||
password=get_auth_service().get_password_hash("testpassword"), # pragma: allowlist secret
|
||||
is_active=False,
|
||||
is_superuser=False,
|
||||
last_login_at=datetime.now(timezone.utc), # Set to avoid "Waiting for approval"
|
||||
)
|
||||
session.add(inactive_user)
|
||||
await session.commit()
|
||||
await session.refresh(inactive_user)
|
||||
user_id = str(inactive_user.id)
|
||||
|
||||
with patch("langflow.services.auth.service.logger") as mock_logger:
|
||||
response = await client.post(
|
||||
"/api/v1/login",
|
||||
data={"username": "inactiveuser", "password": "testpassword"}, # pragma: allowlist secret
|
||||
)
|
||||
|
||||
assert response.status_code == 401
|
||||
assert response.json()["detail"] == "Inactive user"
|
||||
|
||||
# Verify warning was logged with structlog kwargs
|
||||
mock_logger.warning.assert_called_once()
|
||||
call_args = mock_logger.warning.call_args
|
||||
assert call_args[0][0] == "Login failed: inactive user"
|
||||
assert call_args[1]["auth_event"] == "login_failed"
|
||||
assert call_args[1]["reason"] == "user_inactive"
|
||||
assert call_args[1]["auth_id"] == user_id
|
||||
assert "client_ip" in call_args[1]
|
||||
|
||||
# Cleanup
|
||||
async with session_scope() as session:
|
||||
stmt = select(User).where(User.username == "inactiveuser")
|
||||
user = (await session.exec(stmt)).first()
|
||||
if user:
|
||||
await session.delete(user)
|
||||
await session.commit()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_successful_login_logs(client: AsyncClient, active_user):
|
||||
"""Test that successful login is logged."""
|
||||
with patch("langflow.services.auth.service.logger") as mock_logger:
|
||||
response = await client.post(
|
||||
"/api/v1/login",
|
||||
data={"username": active_user.username, "password": "testpassword"}, # pragma: allowlist secret
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert "access_token" in response.json()
|
||||
|
||||
# Verify info log was called with structlog kwargs
|
||||
mock_logger.info.assert_called_once()
|
||||
call_args = mock_logger.info.call_args
|
||||
assert call_args[0][0] == "Login successful"
|
||||
assert call_args[1]["auth_event"] == "login_success"
|
||||
assert "auth_id" in call_args[1]
|
||||
assert "client_ip" in call_args[1]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_login_logs_contain_ip_address(client: AsyncClient):
|
||||
"""Test that login logs contain IP address."""
|
||||
with patch("langflow.services.auth.service.logger") as mock_logger:
|
||||
response = await client.post(
|
||||
"/api/v1/login",
|
||||
data={"username": "nonexistent", "password": "wrong"}, # pragma: allowlist secret
|
||||
)
|
||||
|
||||
assert response.status_code == 401
|
||||
|
||||
# Verify IP was captured in structlog kwargs
|
||||
call_args = mock_logger.warning.call_args
|
||||
assert "client_ip" in call_args[1]
|
||||
# IP should be a string (testclient uses "testclient" as default)
|
||||
assert isinstance(call_args[1]["client_ip"], str)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_login_logs_no_pii(client: AsyncClient):
|
||||
"""Test that login logs do not contain PII (email, full name, plain username)."""
|
||||
with patch("langflow.services.auth.service.logger") as mock_logger:
|
||||
response = await client.post(
|
||||
"/api/v1/login",
|
||||
data={"username": "test_user", "password": "wrong"}, # pragma: allowlist secret
|
||||
)
|
||||
|
||||
assert response.status_code == 401
|
||||
|
||||
# Verify no PII in logs (structlog kwargs)
|
||||
call_args = mock_logger.warning.call_args
|
||||
kwargs = call_args[1]
|
||||
|
||||
# Should NOT contain these PII fields
|
||||
assert "email" not in kwargs
|
||||
assert "first_name" not in kwargs
|
||||
assert "last_name" not in kwargs
|
||||
assert "full_name" not in kwargs
|
||||
assert "phone" not in kwargs
|
||||
assert "address" not in kwargs
|
||||
assert "username" not in kwargs # Plain username is PII
|
||||
|
||||
# Should contain safe identifiers
|
||||
assert "username_hash" in kwargs or "auth_id" in kwargs
|
||||
assert "auth_event" in kwargs
|
||||
assert "reason" in kwargs
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_login_logs_real_output_format(client: AsyncClient):
|
||||
"""Test that login logs use structlog kwargs format (not nested extra dict)."""
|
||||
with patch("langflow.services.auth.service.logger") as mock_logger:
|
||||
response = await client.post(
|
||||
"/api/v1/login",
|
||||
data={"username": "nonexistent_user", "password": "wrong"}, # pragma: allowlist secret
|
||||
)
|
||||
|
||||
assert response.status_code == 401
|
||||
|
||||
# Verify logger was called with kwargs (flat format), not extra dict
|
||||
mock_logger.warning.assert_called_once()
|
||||
call_args = mock_logger.warning.call_args
|
||||
|
||||
# Verify it's using kwargs format (call_args[1] contains kwargs)
|
||||
assert "auth_event" in call_args[1]
|
||||
assert "reason" in call_args[1]
|
||||
assert "username_hash" in call_args[1]
|
||||
assert "client_ip" in call_args[1]
|
||||
|
||||
# Verify it's NOT using the old extra dict format
|
||||
assert "extra" not in call_args[1]
|
||||
|
||||
# Verify the actual values
|
||||
assert call_args[1]["auth_event"] == "login_failed"
|
||||
assert call_args[1]["reason"] == "user_not_found"
|
||||
@ -94,8 +94,19 @@ class BaseAuthService(Service, abc.ABC):
|
||||
username: str,
|
||||
password: str,
|
||||
db: Any,
|
||||
request: Any | None = None,
|
||||
) -> Any | None:
|
||||
"""Authenticate with username and password. Returns user or None."""
|
||||
"""Authenticate with username and password. Returns user or None.
|
||||
|
||||
Args:
|
||||
username: Username to authenticate
|
||||
password: Password to verify
|
||||
db: Database session
|
||||
request: Optional HTTP request for logging IP address
|
||||
|
||||
Returns:
|
||||
User object if authentication succeeds, None otherwise
|
||||
"""
|
||||
|
||||
# -------------------------------------------------------------------------
|
||||
# User validation
|
||||
|
||||
@ -72,6 +72,7 @@ class AuthService(BaseAuthService):
|
||||
username: str,
|
||||
password: str,
|
||||
db: Any,
|
||||
request: Any | None = None,
|
||||
) -> Any | None:
|
||||
logger.debug("Auth: authenticate_user (no-op)")
|
||||
return None
|
||||
|
||||
Reference in New Issue
Block a user