Procházet zdrojové kódy

fix(auth): make password_hash nullable on upgraded SQLite installs (#794)

  LDAP auto-provisioning hit a NOT NULL constraint error on upgraded SQLite
  installs because the existing migration only ran on PostgreSQL. The SQLite
  branch now patches sqlite_master via writable_schema and bumps schema_version
  so the change takes effect without a restart. Fresh installs were unaffected.
maziggy před 1 měsícem
rodič
revize
3d893b22f3

+ 1 - 0
CHANGELOG.md

@@ -14,6 +14,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **LDAP Default Fallback Group** — Settings → Authentication → LDAP → Advanced now has a "Default group" selector. When an LDAP user authenticates but is not listed in any mapped LDAP group, they are automatically assigned to this fallback group instead of being left without permissions. Previously such users could log in successfully but landed on empty pages because every permission check failed. Leave the setting empty to preserve the old behavior. A warning is logged each time the fallback is applied so administrators can spot missing group assignments.
 - **LDAP Default Fallback Group** — Settings → Authentication → LDAP → Advanced now has a "Default group" selector. When an LDAP user authenticates but is not listed in any mapped LDAP group, they are automatically assigned to this fallback group instead of being left without permissions. Previously such users could log in successfully but landed on empty pages because every permission check failed. Leave the setting empty to preserve the old behavior. A warning is logged each time the fallback is applied so administrators can spot missing group assignments.
 
 
 ### Fixed
 ### Fixed
+- **LDAP Auto-Provisioning Fails on Upgraded SQLite Installs** ([#794](https://github.com/maziggy/bambuddy/issues/794)) — First LDAP login on an upgraded SQLite install hit `sqlite3.IntegrityError: NOT NULL constraint failed: users.password_hash` and fell through to a 500 response, because the `users` table on disk had been created before LDAP support landed with `password_hash VARCHAR(255) NOT NULL`. The model was already `nullable=True` and the migration to drop the constraint existed, but only ran on PostgreSQL — SQLite was skipped entirely because it has no `ALTER COLUMN ... DROP NOT NULL`. The migration now patches `sqlite_master` directly via `PRAGMA writable_schema` and bumps `PRAGMA schema_version` so the current connection reloads the table definition without requiring a restart. Fresh installs were never affected (they go through `Base.metadata.create_all` which uses the current nullable model). Thanks to @DylanBrass for reporting.
 - **Energy Statistics Empty for Week/Month/Day in Total Consumption Mode** ([#941](https://github.com/maziggy/bambuddy/issues/941)) — With "Total consumption" selected as the energy tracking mode, the Statistics page showed the correct kWh total for All Time but zero for every time-filtered range (Today, This Week, This Month, …). The backend fell back to summing per-print archive energy whenever a date filter was active, but in total-consumption mode the per-print column was often empty for two reasons: (1) the starting-kWh value was held in an in-memory dict (`_print_energy_start`) that was lost on any backend restart mid-print, so prints that spanned a restart never got an energy delta computed; (2) historical prints from before a smart plug was added had no value at all. The fix replaces the in-memory dict with a persisted `energy_start_kwh` column on the archive row, and adds an hourly snapshot loop (`smart_plug_energy_snapshots` table) that captures each plug's lifetime counter. The `/archives/stats` endpoint now computes date-range totals via per-plug `(last-in-range − baseline)` deltas from those snapshots, clamping counter resets to zero. A warming-up flag is returned (and rendered as a tooltip next to the Energy stats on StatsPage) when the query runs on incomplete snapshot history — e.g. right after upgrade, before the hourly loop has built up a baseline before the selected range — so the "low" values during the first hours after upgrading are explained in-product rather than misread as a bug. Fully localized across all 7 UI languages. Per-print energy tracking is now restart-resilient in all modes as a side-effect. Thanks to Mike (@TheMadMike23) for reporting.
 - **Energy Statistics Empty for Week/Month/Day in Total Consumption Mode** ([#941](https://github.com/maziggy/bambuddy/issues/941)) — With "Total consumption" selected as the energy tracking mode, the Statistics page showed the correct kWh total for All Time but zero for every time-filtered range (Today, This Week, This Month, …). The backend fell back to summing per-print archive energy whenever a date filter was active, but in total-consumption mode the per-print column was often empty for two reasons: (1) the starting-kWh value was held in an in-memory dict (`_print_energy_start`) that was lost on any backend restart mid-print, so prints that spanned a restart never got an energy delta computed; (2) historical prints from before a smart plug was added had no value at all. The fix replaces the in-memory dict with a persisted `energy_start_kwh` column on the archive row, and adds an hourly snapshot loop (`smart_plug_energy_snapshots` table) that captures each plug's lifetime counter. The `/archives/stats` endpoint now computes date-range totals via per-plug `(last-in-range − baseline)` deltas from those snapshots, clamping counter resets to zero. A warming-up flag is returned (and rendered as a tooltip next to the Energy stats on StatsPage) when the query runs on incomplete snapshot history — e.g. right after upgrade, before the hourly loop has built up a baseline before the selected range — so the "low" values during the first hours after upgrading are explained in-product rather than misread as a bug. Fully localized across all 7 UI languages. Per-print energy tracking is now restart-resilient in all modes as a side-effect. Thanks to Mike (@TheMadMike23) for reporting.
 - **Virtual Printer "Synchronizing device information" Times Out in Orca** ([#927](https://github.com/maziggy/bambuddy/issues/927)) — OrcaSlicer's "Send job" flow sat on "Synchronizing device information…" until it gave up, even though the FTP upload itself worked when the user clicked "Send job anyway". The virtual printer's MQTT server gated all incoming command handling on `f"device/{self.serial}/request" in topic` — if the slicer's cached serial for the VP didn't exactly equal the VP's computed `self.serial` (which depends on model prefix + per-VP `serial_suffix`), every `get_version`, `pushall`, and `project_file` publish was silently dropped. Nothing was logged past the initial "MQTT publish to …" line, so the slicer never received a `push_status` or `get_version` response on its subscribed `device/{serial}/report` topic and hit its sync timeout. Status pushes, version responses, and project_file acknowledgments were *also* being published on `device/{self.serial}/report`, so even when the incoming check happened to pass, replies targeted a topic the slicer wasn't listening on if its serial had drifted. Both directions are now serial-adaptive: the handler accepts any authenticated publish on a `device/*/request` topic, extracts the serial the slicer is actually using from the topic, stores it per-connection, and uses it for every outgoing status report, version response, print acknowledgment, and periodic push so responses always land on the topic the slicer subscribed to. The client's serial is cleared when the connection closes and when the server stops. Regression tests cover the mismatched-serial publish path, the non-request-topic rejection path, the pushall→status_report routing, and the client-serial lifecycle.
 - **Virtual Printer "Synchronizing device information" Times Out in Orca** ([#927](https://github.com/maziggy/bambuddy/issues/927)) — OrcaSlicer's "Send job" flow sat on "Synchronizing device information…" until it gave up, even though the FTP upload itself worked when the user clicked "Send job anyway". The virtual printer's MQTT server gated all incoming command handling on `f"device/{self.serial}/request" in topic` — if the slicer's cached serial for the VP didn't exactly equal the VP's computed `self.serial` (which depends on model prefix + per-VP `serial_suffix`), every `get_version`, `pushall`, and `project_file` publish was silently dropped. Nothing was logged past the initial "MQTT publish to …" line, so the slicer never received a `push_status` or `get_version` response on its subscribed `device/{serial}/report` topic and hit its sync timeout. Status pushes, version responses, and project_file acknowledgments were *also* being published on `device/{self.serial}/report`, so even when the incoming check happened to pass, replies targeted a topic the slicer wasn't listening on if its serial had drifted. Both directions are now serial-adaptive: the handler accepts any authenticated publish on a `device/*/request` topic, extracts the serial the slicer is actually using from the topic, stores it per-connection, and uses it for every outgoing status report, version response, print acknowledgment, and periodic push so responses always land on the topic the slicer subscribed to. The client's serial is cleared when the connection closes and when the server stops. Regression tests cover the mismatched-serial publish path, the non-request-topic rejection path, the pushall→status_report routing, and the client-serial lifecycle.
 - **External Sidebar Link Icon Not Showing** ([#878](https://github.com/maziggy/bambuddy/issues/878)) — Custom icons uploaded for external sidebar links rendered correctly in the edit dialog but were missing from the sidebar itself, and opening the icon URL directly returned `{"detail":"Valid camera stream token required..."}`. The sidebar `<img>` tag in `Layout.tsx` used a raw `/api/v1/external-links/{id}/icon` URL, but that endpoint is protected by a query-string stream token (the same mechanism used for camera streams and archive thumbnails, because `<img>` tags cannot send Authorization headers). The edit dialog already routed through `api.getExternalLinkIconUrl()`, which wraps the URL via `withStreamToken()`; the sidebar now does the same, so icons appear when auth is enabled.
 - **External Sidebar Link Icon Not Showing** ([#878](https://github.com/maziggy/bambuddy/issues/878)) — Custom icons uploaded for external sidebar links rendered correctly in the edit dialog but were missing from the sidebar itself, and opening the icon URL directly returned `{"detail":"Valid camera stream token required..."}`. The sidebar `<img>` tag in `Layout.tsx` used a raw `/api/v1/external-links/{id}/icon` URL, but that endpoint is protected by a query-string stream token (the same mechanism used for camera streams and archive thumbnails, because `<img>` tags cannot send Authorization headers). The edit dialog already routed through `api.getExternalLinkIconUrl()`, which wraps the URL via `withStreamToken()`; the sidebar now does the same, so icons appear when auth is enabled.

+ 28 - 1
backend/app/core/database.py

@@ -1372,7 +1372,34 @@ async def run_migrations(conn):
     await _safe_execute(conn, "ALTER TABLE users ADD COLUMN auth_source VARCHAR(20) DEFAULT 'local' NOT NULL")
     await _safe_execute(conn, "ALTER TABLE users ADD COLUMN auth_source VARCHAR(20) DEFAULT 'local' NOT NULL")
 
 
     # Migration: Make password_hash nullable for LDAP users (#794)
     # Migration: Make password_hash nullable for LDAP users (#794)
-    if not is_sqlite():
+    # LDAP users have no local password — the column must allow NULL so auto-provisioning
+    # doesn't hit a NOT NULL constraint failure on upgraded installs whose users table was
+    # originally created before LDAP support landed.
+    if is_sqlite():
+        # SQLite can't ALTER COLUMN; patch sqlite_master directly via writable_schema.
+        # Bump schema_version afterwards so SQLite reloads the table definition from disk —
+        # without that bump, the current connection keeps enforcing the old NOT NULL from
+        # its cached schema. Safe because row data is untouched and the replace() is a
+        # no-op if the constraint has already been removed.
+        try:
+            result = await conn.execute(text("SELECT sql FROM sqlite_master WHERE type='table' AND name='users'"))
+            users_sql = result.scalar()
+            if users_sql and "password_hash VARCHAR(255) NOT NULL" in users_sql:
+                version_result = await conn.execute(text("PRAGMA schema_version"))
+                schema_version = version_result.scalar() or 0
+                await conn.execute(text("PRAGMA writable_schema = ON"))
+                await conn.execute(
+                    text(
+                        "UPDATE sqlite_master "
+                        "SET sql = replace(sql, 'password_hash VARCHAR(255) NOT NULL', 'password_hash VARCHAR(255)') "
+                        "WHERE type = 'table' AND name = 'users'"
+                    )
+                )
+                await conn.execute(text(f"PRAGMA schema_version = {schema_version + 1}"))
+                await conn.execute(text("PRAGMA writable_schema = OFF"))
+        except (OperationalError, ProgrammingError):
+            pass
+    else:
         await _safe_execute(conn, "ALTER TABLE users ALTER COLUMN password_hash DROP NOT NULL")
         await _safe_execute(conn, "ALTER TABLE users ALTER COLUMN password_hash DROP NOT NULL")
 
 
     # Migration: Add energy_start_kwh to print_archives (#941)
     # Migration: Add energy_start_kwh to print_archives (#941)

+ 154 - 0
backend/tests/unit/test_ldap_migration.py

@@ -0,0 +1,154 @@
+"""Regression test for #794 — LDAP auto-provisioning on legacy SQLite schemas.
+
+Pre-LDAP databases created the `users` table with `password_hash VARCHAR(255) NOT NULL`.
+The LDAP provisioning path inserts users with `password_hash=None`, which crashes on
+upgrade until the migration strips the NOT NULL constraint.
+"""
+
+from __future__ import annotations
+
+import pytest
+from sqlalchemy import text
+from sqlalchemy.exc import IntegrityError
+from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine
+
+from backend.app.core.database import run_migrations
+
+
+@pytest.fixture(autouse=True)
+def force_sqlite_dialect(monkeypatch):
+    """The test engine is SQLite but settings.database_url may point to Postgres in dev
+    configs — that would make run_migrations take the Postgres branch and skip the
+    SQLite-specific writable_schema patch we're verifying. Force the sqlite dialect."""
+    from backend.app.core import db_dialect
+
+    monkeypatch.setattr(db_dialect, "is_sqlite", lambda: True)
+    monkeypatch.setattr(db_dialect, "is_postgres", lambda: False)
+    # database.py imported is_sqlite at module load time — patch there too.
+    from backend.app.core import database as database_module
+
+    monkeypatch.setattr(database_module, "is_sqlite", lambda: True)
+
+
+@pytest.fixture
+async def legacy_engine():
+    """Simulate an older install by creating all current tables via create_all, then
+    dropping the `users` table and re-creating it with the legacy NOT NULL schema.
+    This matches the real upgrade path — everything else in the DB looks modern, only
+    the users table carries a stale constraint."""
+    # Import every model so Base.metadata knows about them (same set as conftest).
+    from backend.app.core.database import Base
+    from backend.app.models import (  # noqa: F401
+        ams_history,
+        ams_label,
+        api_key,
+        archive,
+        color_catalog,
+        external_link,
+        filament,
+        group,
+        kprofile_note,
+        maintenance,
+        notification,
+        notification_template,
+        print_queue,
+        printer,
+        project,
+        project_bom,
+        settings,
+        slot_preset,
+        smart_plug,
+        smart_plug_energy_snapshot,
+        spool,
+        spool_assignment,
+        spool_catalog,
+        spool_k_profile,
+        spool_usage_history,
+        spoolbuddy_device,
+        user,
+        user_email_pref,
+        virtual_printer,
+    )
+
+    engine = create_async_engine("sqlite+aiosqlite:///:memory:", echo=False)
+    async with engine.begin() as conn:
+        await conn.run_sync(Base.metadata.create_all)
+        # Drop the users table created from the current (nullable) model and replace it
+        # with the pre-LDAP schema that real upgrading installations have on disk.
+        await conn.execute(text("DROP TABLE IF EXISTS user_groups"))
+        await conn.execute(text("DROP TABLE users"))
+        await conn.execute(
+            text("""
+            CREATE TABLE users (
+                id INTEGER PRIMARY KEY,
+                username VARCHAR(100) NOT NULL UNIQUE,
+                password_hash VARCHAR(255) NOT NULL,
+                role VARCHAR(20) NOT NULL DEFAULT 'user',
+                is_active BOOLEAN NOT NULL DEFAULT 1,
+                created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
+                updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
+            )
+            """)
+        )
+    yield engine
+    await engine.dispose()
+
+
+async def test_legacy_schema_rejects_null_password_before_migration(legacy_engine):
+    """Sanity check: without the migration, inserting a NULL password_hash fails.
+
+    Guards against a false-positive where a future schema change silently allows NULL
+    and the real migration test below becomes meaningless.
+    """
+    async with legacy_engine.begin() as conn:
+        with pytest.raises(IntegrityError):
+            await conn.execute(
+                text(
+                    "INSERT INTO users (username, password_hash, role, is_active) "
+                    "VALUES ('ldap_alice', NULL, 'user', 1)"
+                )
+            )
+
+
+async def test_migration_allows_null_password_hash_for_ldap_users(legacy_engine):
+    """After running migrations on a legacy DB, LDAP users (password_hash=NULL) insert
+    successfully — reproduces and verifies the #794 bug reported by DylanBrass."""
+    async with legacy_engine.begin() as conn:
+        await run_migrations(conn)
+
+    session_maker = async_sessionmaker(legacy_engine, class_=AsyncSession, expire_on_commit=False)
+    async with session_maker() as session:
+        await session.execute(
+            text(
+                "INSERT INTO users (username, email, password_hash, role, auth_source, is_active) "
+                "VALUES (:u, :e, NULL, 'user', 'ldap', 1)"
+            ),
+            {"u": "ldap_bob", "e": "bob@example.com"},
+        )
+        await session.commit()
+
+        result = await session.execute(
+            text("SELECT username, password_hash, auth_source FROM users WHERE username = 'ldap_bob'")
+        )
+        row = result.one()
+        assert row.username == "ldap_bob"
+        assert row.password_hash is None
+        assert row.auth_source == "ldap"
+
+
+async def test_migration_is_idempotent(legacy_engine):
+    """Running migrations twice must not break the writable_schema patch."""
+    async with legacy_engine.begin() as conn:
+        await run_migrations(conn)
+    async with legacy_engine.begin() as conn:
+        await run_migrations(conn)
+
+    session_maker = async_sessionmaker(legacy_engine, class_=AsyncSession, expire_on_commit=False)
+    async with session_maker() as session:
+        await session.execute(
+            text(
+                "INSERT INTO users (username, password_hash, role, auth_source, is_active) "
+                "VALUES ('ldap_carol', NULL, 'user', 'ldap', 1)"
+            )
+        )
+        await session.commit()