From 1cbf1da6bf5491c4abe282c300f22d92f914dc1d Mon Sep 17 00:00:00 2001 From: mrveiss Date: Mon, 16 Mar 2026 20:55:32 +0200 Subject: [PATCH] refactor(slm): consolidate User models to single UUID-PK model (#1900) Remove the legacy integer-PK User class from models/database.py and consolidate all auth onto the user_management UUID-PK User model. - Remove User class from models/database.py (was slm_users table) - Update services/auth.py to import from user_management.models.user - Update main.py _ensure_admin_user to use UserService - Update api/auth.py to use slm_users database session - Update models/schemas.py UserResponse for UUID compatibility - Add consolidate_slm_users_to_uuid migration to move legacy rows --- autobot-slm-backend/api/auth.py | 15 +- autobot-slm-backend/main.py | 36 ++-- .../consolidate_slm_users_to_uuid.py | 183 ++++++++++++++++++ autobot-slm-backend/migrations/runner.py | 3 + autobot-slm-backend/models/database.py | 19 -- autobot-slm-backend/models/schemas.py | 20 +- autobot-slm-backend/services/auth.py | 15 +- 7 files changed, 244 insertions(+), 47 deletions(-) create mode 100644 autobot-slm-backend/migrations/consolidate_slm_users_to_uuid.py diff --git a/autobot-slm-backend/api/auth.py b/autobot-slm-backend/api/auth.py index 202a9bc8c..afe714b18 100644 --- a/autobot-slm-backend/api/auth.py +++ b/autobot-slm-backend/api/auth.py @@ -10,7 +10,7 @@ from api.security import create_audit_log from fastapi import APIRouter, Depends, HTTPException, Request, status from models.schemas import TokenRequest, TokenResponse, UserCreate, UserResponse -from services.auth import auth_service, get_current_user, require_admin +from services.auth import auth_service, get_current_user, get_slm_db, require_admin from services.database import get_db from sqlalchemy.ext.asyncio import AsyncSession from typing_extensions import Annotated @@ -23,7 +23,8 @@ async def login( http_request: Request, body: TokenRequest, - db: Annotated[AsyncSession, Depends(get_db)], + db: Annotated[AsyncSession, Depends(get_slm_db)], + audit_db: Annotated[AsyncSession, Depends(get_db)], ) -> TokenResponse: """Authenticate and get access token. Records audit log entry (Issue #998).""" client_ip = http_request.client.host if http_request.client else None @@ -31,7 +32,7 @@ async def login( if not user: await create_audit_log( - db, + audit_db, category="auth", action="login", username=body.username, @@ -42,6 +43,7 @@ async def login( success=False, error_message="Invalid username or password", ) + await audit_db.commit() raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid username or password", @@ -50,7 +52,7 @@ async def login( logger.info("User logged in: %s", user.username) await create_audit_log( - db, + audit_db, category="auth", action="login", user_id=str(user.id), @@ -61,13 +63,14 @@ async def login( response_status=200, success=True, ) + await audit_db.commit() return await auth_service.create_token_response(user) @router.post("/users", response_model=UserResponse) async def create_user( user_data: UserCreate, - db: Annotated[AsyncSession, Depends(get_db)], + db: Annotated[AsyncSession, Depends(get_slm_db)], _: Annotated[dict, Depends(require_admin)], ) -> UserResponse: """Create a new user (admin only).""" @@ -88,7 +91,7 @@ async def get_current_user_info( @router.post("/refresh", response_model=TokenResponse) async def refresh_token( current_user: Annotated[dict, Depends(get_current_user)], - db: Annotated[AsyncSession, Depends(get_db)], + db: Annotated[AsyncSession, Depends(get_slm_db)], ) -> TokenResponse: """Refresh access token.""" username = current_user.get("sub") diff --git a/autobot-slm-backend/main.py b/autobot-slm-backend/main.py index 471ecfeae..6b4841bf2 100644 --- a/autobot-slm-backend/main.py +++ b/autobot-slm-backend/main.py @@ -185,24 +185,29 @@ async def _ensure_admin_user(): When SLM_ADMIN_PASSWORD is set (Ansible-managed), ensures the admin password always matches. This makes the secrets file the single source of truth for the admin credential. + + Uses the user_management UserService and slm_users database (Issue #1900). """ import os import secrets - from models.database import User from services.auth import auth_service - from sqlalchemy import select + from user_management.database import get_slm_session + from user_management.services import TenantContext, UserService + from user_management.services.user_service import DuplicateUserError env_password = os.getenv("SLM_ADMIN_PASSWORD", "") - async with db_service.session() as db: - result = await db.execute(select(User).where(User.username == "admin")) - existing = result.scalar_one_or_none() + async with get_slm_session() as db: + context = TenantContext(is_platform_admin=True) + user_service = UserService(db, context) + + existing = await user_service.get_user_by_username("admin") if existing: if env_password: existing.password_hash = auth_service.hash_password(env_password) - await db.commit() + await db.flush() logger.info("Admin password synced from SLM_ADMIN_PASSWORD") return @@ -211,14 +216,17 @@ async def _ensure_admin_user(): password = secrets.token_urlsafe(16) logger.critical("Initial admin password set — CHANGE IMMEDIATELY") - admin_user = User( - username="admin", - password_hash=auth_service.hash_password(password), - is_admin=True, - ) - db.add(admin_user) - await db.commit() - logger.warning("Created default admin user (username: admin)") + try: + await user_service.create_user( + email="admin@slm.local", + username="admin", + password=password, + display_name="SLM Admin", + is_platform_admin=True, + ) + logger.warning("Created default admin user (username: admin)") + except DuplicateUserError: + logger.info("Admin user already exists (race condition avoided)") async def _seed_default_roles(): diff --git a/autobot-slm-backend/migrations/consolidate_slm_users_to_uuid.py b/autobot-slm-backend/migrations/consolidate_slm_users_to_uuid.py new file mode 100644 index 000000000..647d25f3c --- /dev/null +++ b/autobot-slm-backend/migrations/consolidate_slm_users_to_uuid.py @@ -0,0 +1,183 @@ +# AutoBot - AI-Powered Automation Platform +# Copyright (c) 2025 mrveiss +# Author: mrveiss +""" +Migration: Consolidate slm_users (integer PK) into users (UUID PK) (#1900). + +The slm_users table (formerly the legacy integer-PK 'users' table, renamed by +the rename_users_to_slm_users migration for issue #1854) is no longer backed by +a SQLAlchemy model. All SLM admin authentication now flows through the +user_management User model with UUID primary key in the 'users' table. + +This migration: +1. Reads any existing rows from slm_users (if the table exists). +2. Inserts them into the users table (UUID PK) using a derived email address. +3. Drops the slm_users table. + +Safe to re-run: checks whether slm_users table exists before acting. +""" + +import logging +import uuid + +logger = logging.getLogger(__name__) + + +def _get_slm_users_db_url() -> str: + """Get the slm_users database URL (sync) from environment. + + The slm_users table was in the main SLM database (renamed from 'users'). + The target users (UUID PK) table lives in the slm_users database, + configured via SLM_USERS_DATABASE_URL. + + Returns the sync URL for psycopg2 (strips asyncpg prefix). + """ + import os + + url = os.getenv("SLM_USERS_DATABASE_URL", "") + if url: + return url.replace("postgresql+asyncpg://", "postgresql://") + + # Fall back to component env vars matching user_management/config.py + host = os.getenv("SLM_POSTGRES_HOST", "127.0.0.1") + port = os.getenv("SLM_POSTGRES_PORT", "5432") + database = os.getenv("SLM_POSTGRES_DB", "slm_users") + user = os.getenv("SLM_POSTGRES_USER", "slm_app") + password = os.getenv("SLM_POSTGRES_PASSWORD", "") + if password: + return f"postgresql://{user}:{password}@{host}:{port}/{database}" + return f"postgresql://{user}@{host}:{port}/{database}" + + +def migrate(db_url: str) -> None: + """Migrate slm_users rows into UUID-PK users table and drop slm_users. + + slm_users lives in the main SLM database (db_url). + users (UUID PK) lives in the separate slm_users database. + Two connections are required — one per database. + + Args: + db_url: Main SLM PostgreSQL sync URL (from migration runner). + """ + import psycopg2 + from migrations.runner import _parse_db_url + + # Connection to main SLM database (contains slm_users table) + main_params = _parse_db_url(db_url) + if main_params.get("password") is None: + main_params.pop("password", None) + + # Connection to slm_users database (contains users UUID table) + slm_users_db_url = _get_slm_users_db_url() + slm_params = _parse_db_url(slm_users_db_url) + if slm_params.get("password") is None: + slm_params.pop("password", None) + + main_conn = psycopg2.connect(**main_params) + try: + with main_conn.cursor() as cur: + if not _table_exists(cur, "slm_users"): + logger.info( + "Table 'slm_users' not found in main SLM database — " + "nothing to migrate (fresh install or already done)" + ) + return + + cur.execute( + "SELECT id, username, password_hash, is_active, is_admin, " + "created_at, last_login FROM slm_users" + ) + rows = cur.fetchall() + + if not rows: + logger.info("No rows in slm_users — dropping empty table") + _drop_slm_users(main_conn) + return + + # Migrate rows into the slm_users database + slm_conn = psycopg2.connect(**slm_params) + try: + with slm_conn.cursor() as cur: + if not _table_exists(cur, "users"): + logger.warning( + "Table 'users' not found in slm_users database — " + "user_management tables may not be initialised yet. " + "Skipping row migration; slm_users table retained." + ) + return + for row in rows: + _migrate_row(cur, row) + slm_conn.commit() + finally: + slm_conn.close() + + _drop_slm_users(main_conn) + logger.info("Migration consolidate_slm_users_to_uuid completed") + finally: + main_conn.close() + + +def _table_exists(cur, table_name: str) -> bool: + """Return True if the named table exists in the current database.""" + cur.execute( + """ + SELECT 1 FROM information_schema.tables + WHERE table_name = %s AND table_schema = 'public' + """, + (table_name,), + ) + return cur.fetchone() is not None + + +def _drop_slm_users(conn) -> None: + """Drop slm_users table from the main SLM database.""" + with conn.cursor() as cur: + cur.execute("DROP TABLE IF EXISTS slm_users CASCADE") + conn.commit() + logger.info("Dropped table 'slm_users' from main SLM database") + + +def _migrate_row(cur, row: tuple) -> None: + """Insert a single slm_users row into the users table. + + Args: + cur: psycopg2 cursor pointing at slm_users database. + row: (id, username, password_hash, is_active, is_admin, created_at, last_login) + """ + _, username, password_hash, is_active, is_admin, created_at, last_login = row + + # Check if a user with this username already exists in users table. + cur.execute("SELECT id FROM users WHERE username = %s", (username,)) + if cur.fetchone(): + logger.info("User '%s' already exists in users table — skipping", username) + return + + new_id = str(uuid.uuid4()) + email = f"{username}@slm.local" + + cur.execute( + """ + INSERT INTO users ( + id, email, username, password_hash, + is_active, is_verified, mfa_enabled, is_platform_admin, + preferences, created_at, updated_at, last_login_at + ) VALUES ( + %s, %s, %s, %s, + %s, false, false, %s, + '{}', COALESCE(%s, NOW()), NOW(), %s + ) + """, + ( + new_id, + email, + username, + password_hash, + is_active, + is_admin, # maps to is_platform_admin + created_at, + last_login, + ), + ) + logger.info( + "Migrated user '%s' from slm_users to users (new id=%s)", username, new_id + ) diff --git a/autobot-slm-backend/migrations/runner.py b/autobot-slm-backend/migrations/runner.py index 4eedfdad7..90afbb9e4 100644 --- a/autobot-slm-backend/migrations/runner.py +++ b/autobot-slm-backend/migrations/runner.py @@ -51,6 +51,9 @@ # Issue #1854: rename SLM admin table to avoid FK collision with # user_management users table (UUID PK). "rename_users_to_slm_users", + # Issue #1900: consolidate slm_users (integer PK) into users (UUID PK) + # and drop the now-orphaned slm_users table. + "consolidate_slm_users_to_uuid", ] diff --git a/autobot-slm-backend/models/database.py b/autobot-slm-backend/models/database.py index 81b4ce739..ed6472d5a 100644 --- a/autobot-slm-backend/models/database.py +++ b/autobot-slm-backend/models/database.py @@ -192,25 +192,6 @@ class Setting(Base): updated_at = Column(DateTime, default=datetime.utcnow, onupdate=datetime.utcnow) -class User(Base): - """SLM admin user model for authentication. - - Stored in the slm_users table to avoid a table-name collision with the - user_management.models.user.User model, which owns the 'users' table with - a UUID primary key (Issue #1854). - """ - - __tablename__ = "slm_users" - - id = Column(Integer, primary_key=True, autoincrement=True) - username = Column(String(64), unique=True, nullable=False, index=True) - password_hash = Column(String(255), nullable=False) - is_active = Column(Boolean, default=True) - is_admin = Column(Boolean, default=False) - created_at = Column(DateTime, default=datetime.utcnow) - last_login = Column(DateTime, nullable=True) - - class EventType(str, enum.Enum): """Node event type enumeration.""" diff --git a/autobot-slm-backend/models/schemas.py b/autobot-slm-backend/models/schemas.py index c304659f2..a9f0a7938 100644 --- a/autobot-slm-backend/models/schemas.py +++ b/autobot-slm-backend/models/schemas.py @@ -46,15 +46,31 @@ class UserCreate(BaseModel): class UserResponse(BaseModel): """User response (excludes password).""" - id: int + id: Any username: str is_active: bool - is_admin: bool + is_admin: bool = False created_at: datetime last_login: Optional[datetime] = None model_config = {"from_attributes": True} + @classmethod + def model_validate(cls, obj, **kwargs): + """Map UUID User model fields to legacy schema fields (#1900).""" + if hasattr(obj, "is_platform_admin") and not hasattr(obj, "is_admin"): + # user_management.models.user.User uses is_platform_admin + data = { + "id": obj.id, + "username": obj.username, + "is_active": obj.is_active, + "is_admin": obj.is_platform_admin, + "created_at": obj.created_at, + "last_login": getattr(obj, "last_login_at", None), + } + return cls(**data) + return super().model_validate(obj, **kwargs) + # ============================================================================= # Role Detection Schemas (Issue #779) diff --git a/autobot-slm-backend/services/auth.py b/autobot-slm-backend/services/auth.py index 0626bcbfb..8ff98bc73 100644 --- a/autobot-slm-backend/services/auth.py +++ b/autobot-slm-backend/services/auth.py @@ -16,11 +16,11 @@ from fastapi import Depends, Header, HTTPException, status from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer from jwt.exceptions import InvalidTokenError -from models.database import User from models.schemas import TokenResponse, UserCreate, UserResponse from passlib.context import CryptContext from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession +from user_management.models.user import User logger = logging.getLogger(__name__) @@ -81,7 +81,7 @@ async def authenticate_user( if not self.verify_password(password, user.password_hash): return None - user.last_login = datetime.utcnow() + user.last_login_at = datetime.utcnow() await db.commit() return user @@ -100,9 +100,14 @@ async def create_user( ) user = User( + email=f"{user_data.username}@slm.local", username=user_data.username, password_hash=self.hash_password(user_data.password), - is_admin=user_data.is_admin, + is_platform_admin=user_data.is_admin, + is_active=True, + is_verified=False, + mfa_enabled=False, + preferences={}, ) db.add(user) await db.commit() @@ -120,7 +125,7 @@ async def get_user_by_username( async def create_token_response(self, user: User) -> TokenResponse: """Create a token response for a user.""" access_token = self.create_access_token( - data={"sub": user.username, "admin": user.is_admin} + data={"sub": user.username, "admin": user.is_platform_admin} ) return TokenResponse( access_token=access_token, @@ -230,7 +235,5 @@ async def _get_user_for_api_key(db: AsyncSession, user_id): Returns: User instance or None """ - from user_management.models.user import User - result = await db.execute(select(User).where(User.id == user_id)) return result.scalar_one_or_none()